Add CJS syntax gate to catch module-load async/await errors earlier#43473
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43473 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Adds an early CI guard to catch CommonJS runtime syntax errors (e.g., invalid await usage) before .cjs modules are loaded during workflow execution, preventing module-load failures from breaking downstream CI steps.
Changes:
- Introduces a reusable
make check-cjs-syntaxtarget that runsnode --checkover runtime.cjsfiles underactions/setup/js(excluding*.test.cjs). - Wires the syntax check into the
CJSworkflow right afternpm ci, ahead of typechecking.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds check-cjs-syntax target to perform syntax-only validation of runtime .cjs files. |
| .github/workflows/cjs.yml | Runs the new syntax gate in CI immediately after JS dependencies are installed. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| @find actions/setup/js \ | ||
| -path 'actions/setup/js/node_modules' -prune -o \ | ||
| -type f -name '*.cjs' ! -name '*.test.cjs' -print0 | xargs -0 -r -n1 node --check |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 48 AIC · ⌖ 5.94 AIC · ⊞ 4.9K
| @echo "→ Checking CommonJS runtime syntax..." | ||
| @find actions/setup/js \ | ||
| -path 'actions/setup/js/node_modules' -prune -o \ | ||
| -type f -name '*.cjs' ! -name '*.test.cjs' -print0 | xargs -0 -r -n1 node --check |
There was a problem hiding this comment.
The check-cjs-syntax target is only wired into CI (via cjs.yml) but not into the local lint umbrella target. This means make lint and make agent-report-progress won't catch CJS syntax errors locally, reducing the 'earlier' benefit to CI-only.
Consider adding it to the lint target:
lint: fmt-check fmt-check-json lint-cjs check-cjs-syntax golint validate-model-alias-chains lint-action-shOr at minimum to lint-cjs so it runs as part of the JS lint chain.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — three observations worth addressing before this lands.
📋 Key Themes & Highlights
Key Themes
- Gate coverage gap: The
check-cjs-syntaxstep lives only injs-typecheck; parallel jobs (js-tests,impacted-js-tests) can still load broken CJS files without hitting the gate. - No regression test for the gate itself: There is no test that verifies the gate rejects a file with invalid
awaitsyntax. A future refactor of the Makefile target could silently regress. - Minor robustness: The
find -path '...node_modules' -prunepattern is slightly narrower than intended — a trailing wildcard (node_modules*) is the idiomatic guard.
Positive Highlights
- ✅ Correct use of
node --check(parse-only, no execution) — exactly the right tool for this gate. - ✅ Excludes
*.test.cjsfiles from scanning — good scope control. - ✅
-rflag onxargspreventsnode --checkfrom being invoked with zero arguments when no CJS files exist. - ✅
@prefix suppresses noisymakecommand echoing;echosentinels give clear CI output. - ✅ Placed immediately after
npm ciand before typecheck — correct ordering so the gate catches errors at the earliest useful point.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 68.7 AIC · ⌖ 5.49 AIC · ⊞ 6.6K
Comment /matt to run again
| @echo "→ Checking CommonJS runtime syntax..." | ||
| @find actions/setup/js \ | ||
| -path 'actions/setup/js/node_modules' -prune -o \ | ||
| -type f -name '*.cjs' ! -name '*.test.cjs' -print0 | xargs -0 -r -n1 node --check |
There was a problem hiding this comment.
[/diagnosing-bugs] The find prune pattern 'actions/setup/js/node_modules' matches only the exact directory entry — nested paths may still be visited. Adding a trailing wildcard makes the prune more robust.
💡 Suggested fix
`@find` actions/setup/js \
-path 'actions/setup/js/node_modules*' -prune -o \
-type f -name '*.cjs' ! -name '*.test.cjs' -print0 | xargs -0 -r -n1 node --checkThe trailing * ensures -prune catches both the node_modules directory and any sub-paths find constructs relative to it.
@copilot please address this.
| @echo "→ Checking CommonJS runtime syntax..." | ||
| @find actions/setup/js \ | ||
| -path 'actions/setup/js/node_modules' -prune -o \ | ||
| -type f -name '*.cjs' ! -name '*.test.cjs' -print0 | xargs -0 -r -n1 node --check |
There was a problem hiding this comment.
[/tdd] No regression test verifies the gate actually fails when a CJS file contains invalid await syntax. Without a failing-case test, a future refactor of this Makefile target could silently stop catching the original bug class.
💡 Suggested test
Add a shell-level test or a Makefile test target:
# test/check-cjs-syntax.sh
echo 'async function wrong() {} const bad = await wrong();' > /tmp/test-bad.cjs
if node --check /tmp/test-bad.cjs 2>/dev/null; then
echo 'FAIL: expected syntax error not detected'; exit 1
fi
echo 'PASS: invalid await correctly rejected'This follows /diagnosing-bugs: the fix should be accompanied by a regression test that would have caught the original failure.
@copilot please address this.
| run: cd actions/setup/js && npm ci | ||
|
|
||
| - name: Check runtime CJS syntax | ||
| run: make check-cjs-syntax |
There was a problem hiding this comment.
[/diagnosing-bugs] The syntax gate only runs in the js-typecheck job. The js-tests, impacted-js-tests, and lint-js jobs all install npm dependencies independently but don't run check-cjs-syntax. A broken CJS file can still reach test execution in those jobs.
💡 Options to close the gap
Option A (simplest): Add the same step to each job that runs CJS files (js-tests, impacted-js-tests).
Option B (preferred): Make js-tests depend on js-typecheck with needs: js-typecheck — the typecheck job's gate will block the test jobs if CJS syntax is invalid, without duplicating the step.
js-tests:
needs: js-typecheck
name: JS Tests (shard ...)
...This ensures any job that could fail due to bad CJS is gated earlier.
@copilot please address this.
There was a problem hiding this comment.
Review: Three issues need fixing before merge
The intent is correct — a pre-load syntax gate is the right tool for this class of bug — but the implementation has gaps that undermine its effectiveness.
Blocking issues:
-
Scope gap (Makefile:237) — are runtime workflow entrypoints that into . They are not scanned, so the check cannot catch the exact failure class it was designed to prevent if that failure happens in a driver file.
-
No job dependency (cjs.yml:53) — and run in parallel with (no ). A syntax error will still trigger 4+ test shards before the gate fires, defeating the 'fail early' goal.
-
Fragile prune (Makefile:238) — only prunes that exact directory. Any nested would be traversed. Use instead.
The portability issue is already noted in an existing review thread.
🔎 Code quality review by PR Code Quality Reviewer · 154.6 AIC · ⌖ 5.45 AIC · ⊞ 5.4K
Comment /review to run again
| .PHONY: check-cjs-syntax | ||
| check-cjs-syntax: | ||
| @echo "→ Checking CommonJS runtime syntax..." | ||
| @find actions/setup/js \ |
There was a problem hiding this comment.
Scope gap: runtime driver .cjs files outside actions/setup/js are not scanned, so the gate does not protect the most direct workflow entrypoints.
💡 Detail and suggested fix
The repo has two runtime CJS files that workflows execute directly, and both live outside actions/setup/js:
.github/drivers/copilot_sdk_driver_sample_node.cjs
.github/drivers/pi_agent_core_driver_sample_node.cjs
Both require() into actions/setup/js/ (e.g. copilot_sdk_multi_provider.cjs). A syntax error in either driver — the exact class of bug this PR targets — would cause an immediate workflow crash that this check silently misses.
Suggested fix — expand the search scope:
check-cjs-syntax:
`@echo` "→ Checking CommonJS runtime syntax..."
`@find` actions/setup/js .github/drivers \
\( -path "*/node_modules" -prune \) -o \
-type f -name "*.cjs" ! -name "*.test.cjs" -print0 | xargs -0 -n1 node --check
`@echo` "✓ CommonJS runtime syntax validated"If the drivers are intentionally excluded, that decision should be documented, but given the PR's stated goal of covering "runtime CommonJS files ... before they are loaded by workflows," excluding known workflow entrypoints undermines that goal.
| - name: Install npm dependencies | ||
| run: cd actions/setup/js && npm ci | ||
|
|
||
| - name: Check runtime CJS syntax |
There was a problem hiding this comment.
Syntax gate is wired only into js-typecheck; js-tests and impacted-js-tests run in parallel with no dependency on it, so a broken CJS file can still reach test execution.
💡 Detail
The js-tests (4 shards) and impacted-js-tests jobs have no needs: js-typecheck declaration, so they start at the same time as js-typecheck. The syntax check runs inside the js-typecheck job, not as a blocking prerequisite. Under normal CI, this means a SyntaxError that fails js-typecheck will have already triggered 4+ parallel test-shard runs — the exact class of noisy runtime failure the PR intends to prevent.
The cleanest fix is to either:
- Move
check-cjs-syntaxinto a standalone job and addneeds: check-cjs-syntaxtojs-tests,impacted-js-tests, andlint-js, or - Add
needs: js-typecheckto every job that executes.cjsfiles at runtime.
Option 1 keeps the check fast and lightweight while ensuring nothing downstream starts until syntax is clean.
| check-cjs-syntax: | ||
| @echo "→ Checking CommonJS runtime syntax..." | ||
| @find actions/setup/js \ | ||
| -path 'actions/setup/js/node_modules' -prune -o \ |
There was a problem hiding this comment.
-path prune expression is overly specific and will miss nested node_modules if the directory structure changes or a new sub-package is added under actions/setup/js.
💡 Detail and suggested fix
-path 'actions/setup/js/node_modules' matches only that exact path. If any sub-package under actions/setup/js ever gets its own node_modules (e.g., actions/setup/js/some-pkg/node_modules/), find will descend into it, checking hundreds of vendored .cjs files and producing spurious errors or very slow runs.
The established portable idiom for pruning all node_modules regardless of depth is:
`@find` actions/setup/js \
-name node_modules -prune -o \
-type f -name *.cjs ! -name *.test.cjs -print0 | ...Using -name 'node_modules' instead of -path 'actions/setup/js/node_modules' prunes any node_modules directory at any depth, which is the intended behaviour here.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey One thing that would make this even more robust:
If you'd like to add that coverage, here's a prompt you can pass to your coding agent: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
The original failure was a module-load
SyntaxError:parse_mcp_gateway_log.cjswas beingrequire()d with invalidawaitusage in non-asyncscope, which broke both the gateway-log summary step and downstream failure handling. This change adds an explicit syntax gate so runtime.cjsfiles fail in CI before they are loaded by workflows.What changed
make check-cjs-syntaxtarget that runsnode --checkacross runtime CommonJS files underactions/setup/js, excluding test files.CJSworkflow immediately after JS dependencies are installed, before typecheck and test execution.Why this closes the gap
awaitwhere the enclosing function is notasync.Example
Net effect
parse_mcp_gateway_log.cjschange before merge.