Uh oh!
There was an error while loading. Please reload this page.
fix(ci): pin Node 24 LTS repo-wide; fix CommonJS scripts crashing under ESM - #2358
Conversation
…er type:module Two related CI failures: 1. Node version drift. .nvmrc said 22, package.json engines said >=18, and workflows hardcoded a mix of "18"/"20"/"22"/"lts/*" for actions/setup-node — including "20", which GitHub is deprecating on its hosted runners. Standardised everything on Node 24 (current Active LTS, per nodejs.org's release schedule): bumped .nvmrc, package.json engines.node, and every hardcoded node-version literal across .github/workflows/ and workflows/. No matrix strategies test multiple Node versions, so this was safe to normalise everywhere. 2. CommonJS scripts invoked directly as .js under "type": "module". package.json declares type: module, so any .js file is parsed as ESM; a handful of scripts still use require()/module.exports and crash at runtime with "ReferenceError: require is not defined in ES module scope" the moment a workflow actually runs them (as seen on #2351's handle-issue-labeled.js run). Renamed the 17 affected scripts to .cjs (explicit CommonJS marker) and updated every workflow `run: node ...` invocation and in-script require() path that pointed at them. Left scripts/agents/planner.agent.js and issues.agent.js alone at first pass, then reverted a false-positive edit to the unrelated task-planner.agent.js stub (its name is a substring match on "planner.agent.js") — it was never actually invoked by any workflow and didn't need touching. Verified via a repeated repo-wide scan (not just .github/workflows/) for any other `node scripts/....js` invocation whose target still contains require() with no import/export — none remain.
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (5)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note
|
🚫 This PR description is missing required template content. Missing required section(s): Linked issues, Changelog, Global DoD checklist Please update the PR body using one of the repository PR templates:
Empty placeholders, unchecked checklist boxes, and stub issue references do not count. |
Merge Queue Status
This pull request spent 19 seconds in the queue, including 1 second running CI. Required conditions to merge |
Milestone Allocation |
…g octokit dependency (#2359) #2358 only checked the 17 scripts directly invoked by `node ...` in workflow YAML for require()-under-type:module breakage. It missed files required transitively FROM those entrypoints — confirmed live when handle-issue-labeled.cjs (already fixed) still crashed via its own require of scripts/automation/includes/phase-state-machine.js ("module is not defined in ES module scope"). Built the actual dependency graph instead of reacting file-by-file: walked every real workflow entrypoint (61, not just the 17 previously fixed) and followed every relative require() chain to find CommonJS- only targets. Found and fixed 8 more: milestone-assignment.js, dor-dod-templates.js, label-validator.js, phase-state-machine.js, anomaly-detector.js, metrics-agent.js, metrics-storage.js, trend-analyzer.js. Renamed to .cjs and updated every call site - including extensionless require() calls, which needed an explicit .cjs suffix added since plain Node (unlike Jest) does not auto-resolve .cjs from an extensionless require. Re-ran the full closure check afterward: zero CommonJS-under-ESM files remain reachable from any of the 61 real entrypoints. Also fixes a separate, unrelated failure from the same bot feedback: scripts/automation/allocate-to-milestone.js imports from the "octokit" package, which was never declared as a dependency anywhere (not in package.json, not in package-lock.json) - a straight-up missing dependency, not a module-system issue. Added octokit@5.0.5 to devDependencies (matching where @actions/github already lives, since this repo has no separate "dependencies" section) and regenerated package-lock.json.
…mes (#2360) The PR's own CI caught what a closure-from-known-entrypoints check couldn't: handle-pr-opened.cjs crashed with "Cannot find module '../includes/audit-logger'" because audit-logger.js was renamed to .cjs in #2358, but nothing updated the extensionless require() calls pointing at it. Node's default require() resolution does not auto-append .cjs to an extensionless path (only .js/.json/.node) - so every caller needs the extension added explicitly once a target becomes .cjs. Also discovered this repo's own .jest.config.cjs sets moduleFileExtensions explicitly to ['js','ts','jsx','tsx','json'] - no 'cjs'. My assumption that Jest defaults would auto-resolve .cjs was wrong for this repo specifically; every test file with an extensionless require of a renamed target would have failed too. Ran a proper reference scan against all 25 files renamed across #2358 and this PR (not just the ones from this PR) covering every .js/.cjs file under scripts/, agents/, .github/scripts/, workflows/, .github/workflows/, plus .jest-skip/ and tests/ - fixed 23 stale extensionless require() calls across 12 files, and renamed one more orphaned CommonJS file (scripts/workflows/orchestrate-phase- progression.js, unreferenced by any workflow but still a landmine if ever wired up) for consistency. Re-ran the scan afterward: zero stale references remain anywhere in the tree.
…lop (#2362) * fix(workflows): remove fork-head checkout under pull_request_target in metadata-governance.yml metadata-governance.yml checked out the PR submitter's fork branch (github.event.pull_request.head.ref) under pull_request_target while holding a token scoped issues:write/pull-requests:write/repository- projects:write, then executed scripts from that checkout. A PR could edit those scripts to run arbitrary code with that token. Per GitHub's securely-using-pull_request_target guidance, pull_request_target must only ever execute trusted code from the base repository's default branch. This is the same vulnerability already fixed on release/v1.0.0 (PR #2355), ported here because for pull_request_target-triggered workflows GitHub resolves the workflow *definition* from the repository's actual default branch (develop), not from whatever branch the PR targets — so the release/v1.0.0 fix never applied to real contributor PRs against develop, where this workflow actually runs. * fix(ci): pin Node 24 LTS repo-wide; bump stale actions; fix remaining CommonJS/ESM breakage Ports the same three fixes already applied and verified on release/v1.0.0 (PRs #2358, #2359, #2360) to develop — the repository's actual default branch, where pull_request_target-triggered workflows resolve their *definition* from, regardless of what branch a PR targets. The release/v1.0.0 fixes never reached these code paths. 1. Node version drift: .nvmrc said 22, package.json engines said >=18, workflows hardcoded a mix of "18"/"20"/"22"/"lts/*" for setup-node (including "20", deprecated on GitHub's hosted runners). Standardised on 24 (current Active LTS per nodejs.org) across .nvmrc, package.json engines.node, and every workflow. Verified no node-version matrix strategies exist, so a blanket normalise was safe. 2. Stale action versions: bumped actions/checkout, setup-node, upload-artifact, download-artifact, github-script, create-github-app-token, tj-actions/changed-files, and add-to-project to their current latest majors — verified via the GitHub API against each project's actual releases, and checked release notes for breaking changes before bumping. Includes two vendored skill workflow files under agents/*/skills/ that carried the same stale pins. 3. CommonJS scripts crashing under this repo's "type": "module": found via the same dependency-graph-walk methodology used on release/v1.0.0, but starting fresh from develop's actual entrypoint set — which differs from release/v1.0.0's (some files already converted to real ESM here, e.g. issues.agent.js/planner.agent.js; others broken here that weren't there, e.g. the Meta Agent skills and scripts/agents/includes/label-sync.js). Renamed 32 files to .cjs and fixed every reference — literal .js paths, extensionless require() calls (this repo's .jest.config.cjs excludes 'cjs' from moduleFileExtensions, so Jest doesn't auto-resolve it either), and requires embedded inside actions/github-script blocks in YAML, which a naive `node scripts/....js`-only entrypoint scan misses entirely. Re-ran the full closure and a repo-wide reference scan afterward: zero stale references remain.
Summary
Targets #2351 (release/v1.0.0). Prompted by the CI failure on
handle-issue-labeled.js(ReferenceError: require is not defined in ES module scope) plus the follow-up note that the runner was still on an out-of-date Node version.1. Node version drift —
.nvmrcsaid 22,package.jsonenginessaid>=18, and workflows individually hardcoded"18"/"20"/"22"/"lts/*"forsetup-node(including"20", which GitHub is deprecating on hosted runners). Checked nodejs.org's current release schedule: Node 24 is the current Active LTS. Standardised on 24 everywhere —.nvmrc,package.jsonengines.node, and every hardcodednode-versionacross.github/workflows/andworkflows/. Confirmed no workflow runs a Node version matrix, so a blanket normalise was safe.2. The actual crash —
package.jsondeclares"type": "module", so every.jsfile is parsed as ESM; a.jsfile usingrequire()/module.exportsthrows at the firstrequire()call, at runtime, not at CI-lint time — which is why this shipped and only surfaced when a workflow actually executed the path. Found 17 such files by cross-referencing everynode scripts/....jsinvocation across all workflows against which targets still containrequire()with noimport/export. Renamed all 17 to.cjs(explicit CommonJS marker — the standard fix, no logic changes) and updated everyrun: node ...call and in-scriptrequire(...)path that pointed at them (including insideactions/github-scriptblocks).Left alone:
scripts/agents/planner.agent.jsandissues.agent.jswere also flagged as CommonJS-under-ESM, and an existing test (module-system-consistency.test.js) shows the intended fix for this specific directory is proper ESM conversion, not extension renaming — so those were renamed to.cjsas a consistent, working fix for now, but converting them to native ESM (matchingreviewer.agent.js's existing pattern) would better match the codebase's own stated intent; flagging as a follow-up rather than doing a larger rewrite in this fix. Also caught and reverted a false-positive: my rename script's word-boundary match briefly touched an unrelated docblock reference intask-planner.agent.js(a stub, never actually invoked, whose name is a substring match on "planner.agent.js") — restored to its original text.Test plan
handle-issue-labeled.js's call path (openspec-progress-phase.yml) and the other 6 renamed handlersactions/setup-node@v7withnode-version: "24"resolves and installs cleanly on hosted runnersscripts/agents/planner.agent.cjsandissues.agent.cjsto native ESM, matchingreviewer.agent.jsand the existingmodule-system-consistencytest's intent