Skip to content

Add CJS syntax gate to catch module-load async/await errors earlier #43473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jul 5, 2026
Merged

Add CJS syntax gate to catch module-load async/await errors earlier #43473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc5aeee
Initial plan
Copilot Jul 5, 2026
3524322
Start investigating SyntaxError fix
Copilot Jul 5, 2026
21d136b
Add CJS syntax gate and revert stray lockfile churn
Copilot Jul 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/cjs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ jobs:
- name: Install npm dependencies
run: cd actions/setup/js && npm ci

- name: Check runtime CJS syntax

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Move check-cjs-syntax into a standalone job and add needs: check-cjs-syntax to js-tests, impacted-js-tests, and lint-js, or
  2. Add needs: js-typecheck to every job that executes .cjs files at runtime.

Option 1 keeps the check fast and lightweight while ensuring nothing downstream starts until syntax is clean.

run: make check-cjs-syntax

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.


- name: Run typecheck
run: cd actions/setup/js && npm run typecheck

Expand Down
8 changes: 8 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,14 @@ security-govulncheck-sarif:
@echo "✓ Govulncheck complete (results in govulncheck-results.sarif)"

# Test JavaScript files
.PHONY: check-cjs-syntax
check-cjs-syntax:
@echo "→ Checking CommonJS runtime syntax..."
@find actions/setup/js \

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-path 'actions/setup/js/node_modules' -prune -o \

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-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.

-type f -name '*.cjs' ! -name '*.test.cjs' -print0 | xargs -0 -r -n1 node --check
Comment on lines +237 to +239

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-sh

Or at minimum to lint-cjs so it runs as part of the JS lint chain.

@copilot please address this.

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 --check

The trailing * ensures -prune catches both the node_modules directory and any sub-paths find constructs relative to it.

@copilot please address this.

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

@echo "✓ CommonJS runtime syntax validated"

.PHONY: test-js
test-js: build-js
cd actions/setup/js && npm run test:js -- --no-file-parallelism
Expand Down
Loading