Uh oh!
There was an error while loading. Please reload this page.
fix(ci): fix stale extensionless require() calls left after .cjs renames - #2360
Merged
Merged
Conversation
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.
Contributor
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
|
Contributor
Merge Queue Status
This pull request spent 20 seconds in the queue, including 2 seconds running CI. Required conditions to merge |
Contributor
Milestone Allocation |
4 tasks
mergifyBot
pushed a commit
that referenced
this pull request
Aug 27, 2026
… only (#2361) merge_protections_settings.auto_merge_conditions was set to `true`, which per Mergify's own docs means unconditional auto-merge of ANY mergeable pull request, from ANY author, targeting ANY base branch, the instant GitHub branch protection is satisfied - completely bypassing the author/base/check scoping deliberately written into every pull_request_rule in this same file (all three intentional auto-merge rules restrict themselves to dependabot/imgbot/ lightspeed-bot on develop, gated on check-success=All Checks Passed). Confirmed via Mergify's own merge-queue status comments on #2359 and #2360: both were auto-merged "triggered by merge protections" within 20-27 seconds of opening, with an empty required_conditions list - not because any pull_request_rule matched (Mergify's own check-run explicitly listed all 6 rules as "not applicable" for those PRs). release/v1.0.0 has no branch protection configured at all, so "mergeable" was true immediately, and every human/agent-authored fix PR against it got squash-merged before CI had even finished running, including once before a CI-discovered bug fix could be pushed to the same PR. Restricts auto_merge_conditions to a condition list matching the same author identities already used throughout this file, so the three intended bots keep instant auto-merge and everyone else must merge manually - matching what the rest of this config was clearly already trying to express.
5 tasks
mergifyBot
pushed a commit
that referenced
this pull request
Aug 27, 2026
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issues
Relates to #2351. Follow-up to #2359 — this commit was pushed to that PR's branch AFTER Mergify had already auto-merged it (#2359 merged at 12:06:32; this fix was pushed at 12:11), so it never actually landed. See PR comment for the full explanation.
Changelog
Fixed
handle-pr-opened.cjs(and 6 sibling handlers) still crashed withCannot find module '../includes/audit-logger'after fix(ci): pin Node 24 LTS repo-wide; fix CommonJS scripts crashing under ESM #2358 renamedaudit-logger.jsto.cjs— nothing updated the extensionlessrequire()calls pointing at it. Node's defaultrequire()resolution does not auto-append.cjsto an extensionless path..jest.config.cjssetsmoduleFileExtensionsexplicitly to['js','ts','jsx','tsx','json']— nocjs— so every test file with an extensionless require of a renamed target would also fail under Jest, not just in production.require()calls across 12 files (7 handlers + 5 test files), found via a full reference scan across every.js/.cjsfile inscripts/,agents/,.github/scripts/,workflows/,.github/workflows/,.jest-skip/, andtests/— not just the files touched by prior PRs.scripts/workflows/orchestrate-phase-progression.js, currently unreferenced by any workflow but still a landmine if ever wired up) for consistency.Risk Assessment
Risk Level: Low
Potential Impact: CI automation scripts only (issue/PR handlers). A bug here fails a workflow run; no production/user-facing impact.
Mitigation Steps: Re-ran the full stale-reference scan after fixing — zero remaining anywhere in the tree.
How to Test
Prerequisites
None.
Test Steps
openspec-progress-phase.yml's "Handle PR opened" step (or any workflow callinghandle-pr-opened.cjs,handle-issue-created.cjs, etc.) — should no longer throwCannot find module '../includes/audit-logger'.npm test) — the 5 updated test files should resolve their requires correctly.Expected Results
No
MODULE_NOT_FOUNDerrors from any of the 7 handler scripts or their tests.Edge Cases to Verify
Checklist (Global DoD / PR)