Skip to content

chore: centralize workspace yarn fix pipeline (RHIDP-14035) - #4568

Open
ciiay wants to merge 4 commits into
redhat-developer:mainfrom
ciiay:rhidp-14035-yarn-fix
Open

chore: centralize workspace yarn fix pipeline (RHIDP-14035)#4568
ciiay wants to merge 4 commits into
redhat-developer:mainfrom
ciiay:rhidp-14035-yarn-fix

Conversation

@ciiay

@ciiay ciiay commented Sep 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Add scripts/workspace-fix.mjs, a shared fix pipeline invoked by yarn fix from each workspace root
  • Wire all workspaces to the shared script; document fixer order in CONTRIBUTING.md
  • Support yarn fix --check for CI (runs backstage-cli repo fix --check only)
  • Set NODE_OPTIONS=--max-old-space-size=8192 for memory-heavy fixers, matching CI
  • Exclude dist-dynamic / dist-scalprum from global-header lint/format passes to avoid OOM on generated bundles
  • Update the repo-tools workspace template so new workspaces get the same setup

Fixed

  • RHIDP-14035 — Implement unified yarn fix command at repo root

Story: Create a single yarn fix script that auto-corrects fixable lint, format, and code issues with a deterministic fixer order and central extensibility.

Implementation notes: CI runs yarn fix per changed workspace (not from the monorepo root), so the shared logic lives in scripts/workspace-fix.mjs and each workspace's fix script delegates to it. Optional fixers (sort-package-json, markdownlint, knip) are skipped when not installed; knip --fix remains opt-in via rhdhFix.knip or --knip.

Fixes: https://redhat.atlassian.net/browse/RHIDP-14035

Test plan

  • yarn test:workspace-fix (20 unit tests)
  • yarn fix --check in workspaces/global-header
  • yarn fix in workspaces/global-header (lint no longer traverses dist-dynamic)
  • CI yarn fix --check step on changed workspaces

Made with Cursor

Add a shared fix script with CI-compatible --check mode, wire all
workspaces to it, and exclude generated dist-dynamic output from
global-header lint/format passes.

Fixes: https://redhat.atlassian.net/browse/RHIDP-14035
Signed-off-by: Yi Cai <yicai@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:38 PM UTC · Completed 5:45 PM UTC

Commit: 438ef3b · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.20

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.37%. Comparing base (42ec657) to head (9f08171).
⚠️ Report is 12 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4568   +/-   ##
=======================================
  Coverage   62.36%   62.37%           
=======================================
  Files        2607     2607           
  Lines      104739   104807   +68     
  Branches    29427    29450   +23     
=======================================
+ Hits        65324    65371   +47     
- Misses      38857    38873   +16     
- Partials      558      563    +5     
Flag Coverage Δ *Carryforward flag
adoption-insights 84.77% <ø> (ø)
ai-integrations 78.80% <ø> (ø)
app-defaults 56.22% <ø> (ø)
augment 46.67% <ø> (ø)
boost 80.51% <ø> (ø)
bulk-import 73.12% <ø> (ø)
cost-management 13.35% <ø> (ø)
dcm 73.47% <ø> (ø)
e2e-adoption-insights 60.00% <ø> (ø) Carriedforward from 3c11a2d
e2e-extensions 62.32% <ø> (ø) Carriedforward from 3c11a2d
e2e-global-header 50.35% <ø> (ø) Carriedforward from 3c11a2d
e2e-homepage 61.11% <ø> (ø) Carriedforward from 3c11a2d
e2e-intelligent-assistant 47.04% <ø> (ø) Carriedforward from 3c11a2d
e2e-orchestrator 49.52% <ø> (ø) Carriedforward from 3c11a2d
e2e-orchestrator-plugin 49.51% <ø> (ø) Carriedforward from 3c11a2d
e2e-quickstart 55.21% <ø> (ø) Carriedforward from 3c11a2d
e2e-scorecard 50.21% <ø> (ø) Carriedforward from 3c11a2d
e2e-theme 16.36% <ø> (ø) Carriedforward from 3c11a2d
extensions 57.37% <ø> (+0.71%) ⬆️
global-floating-action-button 71.18% <ø> (ø) Carriedforward from 3c11a2d
global-header 68.09% <ø> (ø)
homepage 48.39% <ø> (ø)
install-dynamic-plugins 71.31% <ø> (ø)
intelligent-assistant 76.43% <ø> (ø)
konflux 91.98% <ø> (ø)
lightspeed 69.02% <ø> (ø) Carriedforward from 3c11a2d
mcp-integrations 84.14% <ø> (ø)
orchestrator 71.13% <ø> (ø)
quickstart 63.74% <ø> (ø)
sandbox 79.56% <ø> (ø) Carriedforward from 3c11a2d
scorecard 87.96% <ø> (+0.05%) ⬆️
theme 87.91% <ø> (ø)
translations 5.12% <ø> (ø)
x2a 77.10% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42ec657...9f08171. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review — approve

PR: chore: centralize workspace yarn fix pipeline (RHIDP-14035)
Scope: 36 files · +764 / −23

Summary

This PR introduces a centralized scripts/workspace-fix.mjs script that defines a deterministic fixer pipeline (repo fix → sort-package-json → lint --fix → markdownlint → prettier → knip), then migrates all 22 non-noop workspaces from inline backstage-cli repo fix calls to node ../../scripts/workspace-fix.mjs. The workspace template is also updated so new workspaces inherit the shared pipeline. Documentation in CONTRIBUTING.md and README.md is comprehensive and accurate.

Correctness

  • Pipeline logic is sound. The buildSteps / runPipeline separation keeps step definition pure and execution injectable — enabling the thorough 20-test suite that covers flag parsing, config merging, step building, and pipeline execution (including error paths and required-fixer enforcement).
  • x2a --publish migration is correct. The only workspace that previously passed --publish (x2a) is migrated to rhdhFix.publish: true in package.json, which resolveConfig picks up and forwards to the repo-fix step args.
  • noop workspace correctly excluded. It retains its "fix": "exit 0" stub and is documented as the exception.
  • --check mode is well-scoped. In check mode, only backstage-cli repo fix --check runs. The documentation correctly explains that CI uses separate workflow steps for lint, prettier, and publish validation.
  • mergeNodeOptions handles all edge cases. Existing heap limits are preserved unless explicitly overridden via rhdhFix.nodeOptions, non-heap options are appended cleanly, and workspace-level overrides correctly replace the default.
  • spawnStep error handling is safe. Both error (spawn failure) and close events resolve the promise; duplicate resolution is harmless.

Security

  • No user-controlled input flows into spawn commands — all commands and arguments are hardcoded string constants derived from the step definitions. No injection surface.
  • NODE_OPTIONS is constructed from trusted configuration (rhdhFix.nodeOptions in package.json) which lives in version-controlled code.
  • No secrets, credentials, or sensitive data are exposed.

Intent & coherence

  • The change directly addresses RHIDP-14035: a single yarn fix entry point with deterministic fixer ordering and central extensibility.
  • The approach (shared script referenced by each workspace, not a monorepo-root runner) is architecturally appropriate given that CI runs per-workspace, not from the repo root.
  • The scope is well-contained: one new script, one test file, workspace package.json updates, ignore file hygiene, and documentation.

Style & conventions

  • Code follows the project's ESM conventions and uses Node.js built-in test runner (node:test) consistent with repo patterns.
  • Error objects with exitCode properties enable clean exit-code propagation from the main function.
  • The exported function design supports both direct invocation and testability.

Documentation

  • CONTRIBUTING.md documents the full fixer order, --check mode, memory settings, and extension instructions.
  • README.md provides a concise pointer to the detailed docs.
  • The instruction "The noop workspace is the exception and stays a no-op" prevents confusion.

Ignore-file hygiene

  • dist-dynamic and dist-scalprum exclusions added to .eslintignore and .prettierignore in global-header (workspace + plugin level), orchestrator (.prettierignore only — .eslintignore already had them), and the workspace template. This prevents OOM from linting/formatting generated bundles.

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • scripts/workspace-fix.mjs
  • scripts/workspace-fix.test.mjs
Previous run

Review — Approve

Clean, well-structured change that centralizes the workspace yarn fix pipeline into a single shared script. The implementation is correct, well-tested (20 unit tests using node:test), properly documented, and tightly scoped to the linked RHIDP-14035 issue.

What was reviewed

Dimension Verdict Notes
Correctness ✅ Pass Logic is sound; all workspace migrations verified; x2a --publish preserved via rhdhFix.publish; noop workspace correctly excluded
Security ✅ Pass No injection risk — all spawn() args are static; NODE_OPTIONS source is version-controlled package.json; no secrets/auth/permissions touched
Intent & coherence ✅ Pass Well-scoped to authorized issue; per-workspace execution model fits CI architecture; scripts/ placement follows existing patterns
Style & conventions ✅ Pass Two minor divergences noted below
Documentation ✅ Pass CONTRIBUTING.md fixer order matches code exactly; all config keys (rhdhFix.publish, .knip, .nodeOptions) documented
Cross-repo contracts ⏭ Skipped No exported interfaces modified

Findings (low — non-blocking)

1. mergeNodeOptions drops non-heap options in override path

File: scripts/workspace-fix.mjs:206 · Severity: low · Category: logic-error

When overrideHeapLimit is true and existing already contains a heap-limit flag, the function extracts only the --max-old-space-size=N token from additional and replaces it in existing. Any other options in additional (e.g., --inspect) are silently discarded.

Example: mergeNodeOptions('--max-old-space-size=8192', '--max-old-space-size=16384 --inspect', { overrideHeapLimit: true }) returns '--max-old-space-size=16384' — the --inspect is lost.

This is currently not triggered in practice (no workspace sets multi-option rhdhFix.nodeOptions), but would surprise future workspace authors who do.

Suggested fix: After the heap-limit replacement, strip the matched token from additional and append the remainder.

2. .mjs extension diverges from repo convention

File: scripts/workspace-fix.mjs:1 · Severity: low · Category: naming-conventions

All ~20 existing scripts in scripts/ and scripts/ci/ use .js. Since the root package.json declares "type": "module", .js files are already treated as ESM — the .mjs extension is redundant and introduces a convention split that propagates into ~20 workspace package.json references.

3. Object.assign(new Error(...), { exitCode }) diverges from repo error idiom

File: scripts/workspace-fix.mjs:143 · Severity: low · Category: error-handling-idiom

Used in five places across the file. No other script in scripts/ uses this pattern — the established idiom is to throw a plain Error and always process.exit(1) in the catch handler. Consider extracting a small FixerError class to make the intent explicit and grep-able.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • scripts/workspace-fix.mjs
  • scripts/workspace-fix.test.mjs
Previous run (2)

Review — request-changes

PR: #4568 — chore: centralize workspace yarn fix pipeline (RHIDP-14035)
Scope: Adds scripts/workspace-fix.mjs, a shared fix pipeline invoked by yarn fix from each workspace root. Wires all 22 non-noop workspaces, documents fixer order, supports --check for CI, excludes dist-dynamic/dist-scalprum from lint/format passes, and updates the repo-tools template.

This is a well-structured change with clear authorization (RHIDP-14035), good test coverage (20 unit tests), and thorough documentation. However, one logic bug in mergeNodeOptions needs to be fixed before merging — it silently drops workspace-level memory overrides in CI, contradicting the escape hatch documented in CONTRIBUTING.md.


High

1. mergeNodeOptions silently drops workspace memory overrides in CI

File: scripts/workspace-fix.mjs:196–206
Category: logic-error

mergeNodeOptions checks existing.includes('max-old-space-size') and returns existing unchanged, dropping additional entirely. In CI, the job-level env sets NODE_OPTIONS=--max-old-space-size=8192 (.github/workflows/ci.yml line 62). If a workspace sets rhdhFix.nodeOptions to --max-old-space-size=16384 to request more memory (as CONTRIBUTING.md instructs), the workspace's override is silently discarded — mergeNodeOptions('--max-old-space-size=8192', '--max-old-space-size=16384') returns 8192.

Remediation: When additional comes from config.nodeOptions (an explicit workspace override), it should replace the existing max-old-space-size value rather than being dropped. One approach: have resolveSpawnEnv replace the entire NODE_OPTIONS when config.nodeOptions is set, rather than merging. Alternatively, parse and replace the specific --max-old-space-size= token.


Medium

2. Test masks the mergeNodeOptions bug

File: scripts/workspace-fix.test.mjs:511
Category: test-integrity

The test 'resolveSpawnEnv honors rhdhFix.nodeOptions' passes an empty object {} as baseEnv, so baseEnv.NODE_OPTIONS is undefined. This means the test never exercises the conflict path where an existing NODE_OPTIONS value collides with the workspace override. The test asserts the feature works, but only in the exact scenario where the override is least needed.

Remediation: Add a test case with baseEnv: { NODE_OPTIONS: '--max-old-space-size=8192' } and config.nodeOptions: '--max-old-space-size=16384' to verify the workspace override takes precedence.

3. .fullsend/AGENTS.md has stale guidance

File: .fullsend/AGENTS.md:10
Category: stale-docs

Line 10 instructs: "run yarn backstage-cli repo fix --publish at the repo root after the package is set up." While the old command still works (it bypasses the new pipeline), this guidance does not direct users to the new centralized yarn fix pipeline, which runs additional fixers (lint, prettier, sort-package-json). The documentation should be updated to reference yarn fix from the workspace root.

Remediation: Update to: "run yarn fix (or yarn fix --publish) from the workspace root (workspaces/<name>)."


Low

4. .mjs file extension departs from codebase convention

File: scripts/workspace-fix.mjs
Category: file-naming

All 22 existing scripts in scripts/ and scripts/ci/ use the .js extension. The root package.json sets "type": "module", so .js files are already parsed as ESM. Using .mjs is functionally equivalent but breaks the naming convention. The .mjs choice may be intentional to disambiguate from the Jest-oriented ecosystem (this is the first script with co-located tests using node:test), but it is worth noting.

5. Novel error-handling idiom

File: scripts/workspace-fix.mjs (6 occurrences)
Category: error-handling

Object.assign(new Error(...), { exitCode: N }) is used in 6 places but does not appear in any existing script. Existing scripts vary between throw new Error(...) and process.exit(1). The pattern is internally consistent and enables testability, but introduces a new convention.

6. spawnStep discards spawn errors

File: scripts/workspace-fix.mjs:333
Category: error-handling

child.on('error', () => resolvePromise(1)) discards the actual Error object. When spawn fails (e.g., ENOENT for a missing binary), the user sees only "Fixer 'X' failed with exit code 1" with no root-cause diagnostic. Consider logging err.message before resolving.

7. Test coverage gaps for mergeNodeOptions and markdownlintArgs

File: scripts/workspace-fix.test.mjs
Category: test-adequacy

mergeNodeOptions has only one test case. The concatenation path (both args present, existing lacks max-old-space-size) and the markdownlintArgs branching between markdownlint-cli2 and markdownlint-cli are untested.


What looks good

  • Authorization & scope: Change traces cleanly to RHIDP-14035. The .eslintignore/.prettierignore additions for dist-dynamic/dist-scalprum are necessary prerequisites for the new pipeline (prevents OOM when repo lint --fix and prettier --write would otherwise traverse generated bundles).
  • x2a migration: The --publish flag is correctly migrated from a hardcoded script argument to a declarative rhdhFix.publish: true config field, preserving behavior through the resolveConfigrepoFixArgs path.
  • Completeness: All 22 non-noop workspaces are updated; the noop workspace is intentionally excluded and documented. The repo-tools template is updated for future workspaces.
  • Security: No injection vectors, no secrets exposure, no auth/RBAC/permission changes. All spawn calls use array-form arguments with hardcoded commands.
  • Cross-repo contracts: No exported interfaces, package exports, or public APIs are modified.
Previous run (3)

Review

Verdict: approve

This PR introduces a centralized yarn fix pipeline via scripts/workspace-fix.mjs, replacing per-workspace backstage-cli repo fix invocations with a shared script that runs a deterministic sequence of fixers. The change is well-structured, thoroughly tested, and properly documented.

What the PR does

  • Adds scripts/workspace-fix.mjs — a shared pipeline with a defined fixer order: backstage-cli repo fixsort-package-jsonbackstage-cli repo lint --fixmarkdownlint --fixprettier --write .knip --fix (opt-in)
  • Adds 20 unit tests covering arg parsing, config resolution, step building, pipeline execution, and edge cases
  • Updates all 22 non-noop workspace package.json files to delegate to the shared script
  • Preserves x2a's --publish flag via rhdhFix.publish: true in package.json
  • Adds dist-dynamic/dist-scalprum exclusions to global-header's eslint/prettier ignore files to prevent OOM on generated bundles
  • Updates the workspace template so new workspaces get the shared script and exclusions
  • Documents the fixer order and usage in CONTRIBUTING.md and README.md
  • Correctly leaves the noop workspace unchanged ("fix": "exit 0")

Correctness

  • The code is well-factored with pure, exported functions that are independently testable.
  • --check mode correctly narrows to only backstage-cli repo fix --check, matching current CI behavior (ci.yml line 84: yarn fix --check).
  • Error handling is solid: unknown flags are rejected, missing required fixers fail loudly, exit codes are propagated from child processes.
  • The isMainModule() guard correctly uses import.meta.url comparison for ESM entry-point detection.
  • Optional tools (sort-package-json, markdownlint, knip) are gracefully skipped with informative messages when not installed.
  • The knip skipReason ternary correctly distinguishes "opted in but not installed" from "not opted in".

Behavioral change note

Previously yarn fix only ran backstage-cli repo fix. After this PR, the full (non---check) pipeline also runs lint --fix and prettier --write. This is the intended design. CI is unaffected because it uses --check mode, which remains scoped to backstage-cli repo fix --check only.

Security

No security concerns. The script uses spawn without shell: true, reads only local package.json files, and all arguments are statically constructed.

Low-severity notes for follow-up

  1. orchestrator missing prettierignore exclusions — The orchestrator workspace has dist-dynamic/dist-scalprum in .eslintignore but not in .prettierignore. Since the new pipeline adds prettier --write ., running yarn fix after a local dynamic plugin build could cause prettier to traverse large generated bundles. The PR correctly documents this as a workspace-maintainer responsibility (CONTRIBUTING.md), but orchestrator is a known workspace with dynamic builds that should be updated.

  2. rhdhFix.nodeOptions applies globally — When rhdhFix.nodeOptions is set, resolveSpawnEnv applies it to all steps (including lightweight ones like sort-package-json), not just MEMORY_HEAVY_STEPS. This is harmless but slightly inconsistent with the documentation framing it as a memory-relief override for heavy fixers.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • scripts/workspace-fix.mjs
  • scripts/workspace-fix.test.mjs

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Sep 3, 2026
Exclude generated dynamic plugin bundles from prettier --write so
yarn fix does not traverse dist-dynamic/dist-scalprum output.

Signed-off-by: Yi Cai <yicai@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:43 PM UTC · Completed 7:59 PM UTC

Commit: 45dd958 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $5.30

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Sep 3, 2026
Replace the existing heap limit when a workspace sets an explicit
rhdhFix.nodeOptions override, and add tests for the CI conflict path.

Signed-off-by: Yi Cai <yicai@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 2:51 AM UTC · Ended 3:04 AM UTC

Commit: 3c11a2d · View workflow run →

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review September 4, 2026 03:04

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Sep 4, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:51 AM UTC · Completed 3:04 AM UTC

Commit: 3c11a2d · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $4.92

Add an explicit pipeline assertion in the integration test and use
top-level await for the CLI entry point.

Signed-off-by: Yi Cai <yicai@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 2:17 PM UTC · Ended 2:23 PM UTC

Commit: 9f08171 · View workflow run →

@sonarqubecloud

sonarqubecloud Bot commented Sep 4, 2026

Copy link
Copy Markdown

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Sep 4, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:17 PM UTC · Completed 2:23 PM UTC

Commit: 9f08171 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant