fix(ci): stop splicing repo-health report content into github-script bodies - #1786
Conversation
…bodies
The Security Review, Architectural Review, and Squad File Leakage reporters
interpolated their scan output into the `script:` body of actions/github-script
via `${{ steps.*.outputs.result }}` inside a template literal. Finding messages
legitimately quote code in backticks (e.g. "Unsafe git operation: `git push
--force-with-lease`"), which closes the literal early and leaves the rest of the
JSON to be parsed as JavaScript — `SyntaxError: Unexpected identifier 'git'`.
The gate then failed with an opaque parse error and never posted the finding,
which is indistinguishable from a clean run to anyone skimming.
Fixed at the boundary: report content now travels through the step environment
(`REPO_HEALTH_OUTPUT`) and is read with `process.env`, so payload text is never
part of the parsed script. Applied to all three sibling reporters, which shared
the bug, plus the impact reporter in squad-impact.yml for consistency.
Regression coverage extracts the real `script:` bodies from the workflow YAML,
applies GitHub Actions expression substitution, and compiles them the way
github-script does, using findings that contain a backtick, a bare `${`, an
apostrophe, a double quote, and a newline. Verified red against the pre-fix
workflow with the exact production error.
Closes #1770
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
🟡 Impact Analysis — PR #1786Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (2 files)
tests (1 file)
This report is generated automatically for every PR. See #733 for details. |
🏗️ Architectural Review
Automated architectural review — informational only. |
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 2 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | No source files changed — changeset not required |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ❌ | Copilot threads resolved | 2 unresolved Copilot thread(s) — fix and resolve before merging |
| ❌ | CI passing | 1 check(s) failing: test |
Files Changed (3 files, +304 −4)
| File | +/− |
|---|---|
.github/workflows/squad-impact.yml |
+3 −1 |
.github/workflows/squad-repo-health.yml |
+14 −3 |
test/scripts/repo-health-comment-transport.test.ts |
+287 −0 |
Total: +304 −4
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
There was a problem hiding this comment.
Pull request overview
This PR fixes a GitHub Actions actions/github-script failure mode where repo-health report content was interpolated into the JavaScript script: body (becoming JS source), causing syntax errors when findings include backticks / ${ (e.g. security findings quoting code). It moves report payloads to step environment variables so the script reads them as data, and adds a regression test that compiles/executes the real workflow script bodies the same way github-script does.
Changes:
- Update
squad-repo-health.ymlto pass reporter outputs viaenv(REPO_HEALTH_OUTPUT) and readprocess.env.REPO_HEALTH_OUTPUTin thegithub-scriptsteps. - Harden
squad-impact.ymlby passing PR number viaenvinstead of interpolating it into thescript:body. - Add regression tests that load the real workflow YAML, simulate
${{ }}substitution, compile the resulting script, and assert hostile finding content survives verbatim.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/squad-repo-health.yml |
Stops embedding reporter output into github-script source; uses env var transport for leakage/architectural/security comment steps. |
.github/workflows/squad-impact.yml |
Removes workflow-expression interpolation from the github-script body by using an env-provided PR number. |
test/scripts/repo-health-comment-transport.test.ts |
Adds regression coverage that reproduces the pre-fix syntax error and asserts safe transport + verbatim comment content. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
| const body = `${marker}\n${report}`; | ||
| const prNumber = ${{ github.event.pull_request.number }}; | ||
| const prNumber = Number(process.env.PR_NUMBER); | ||
|
|
|
|
||
| const env = stepEnv(step); | ||
| const previous: Record<string, string | undefined> = {}; | ||
| const applied: Record<string, string> = { GITHUB_WORKSPACE: pathToFileURL(repoRoot).href }; |
The comment quoted a repo-wide staging flag verbatim, which security-review.mjs correctly flags as an unsafe-git finding. The code was never doing it -- the fixture stages explicit paths -- so this is a false positive created by prose, and prose is the cheaper thing to change. Worth recording why it surfaced: the finding message quotes the offending command in backticks, so on dev (which does not yet carry #1786) the reporter died with `SyntaxError: Unexpected identifier 'git'` instead of posting the finding. That is #1770 reproducing in the field, on this PR, from an ordinary code comment -- independent confirmation that the #1786 boundary fix is addressing a live defect rather than a theoretical one. Refs #1788 Refs #1770 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
The fixture quoted `git push --force-with-lease` and a path containing a
repo-wide staging flag. Those are added lines in the PR diff, so the security
scanner flagged them -- correctly, by its own rules -- and emitted findings
that quote the offending command in backticks.
That is the exact input this PR exists to make safe, which produced a genuinely
useful accident: the Repo Health check on this PR died with
##[error]Unhandled error: SyntaxError: Unexpected identifier 'git'
at new AsyncFunction (<anonymous>)
...the #1770 crash, reproducing end-to-end in CI, on the pull request that
fixes it, from a fixture nobody designed to trigger it.
It cannot be fixed by this PR's own diff. squad-repo-health.yml runs on
pull_request_target and deliberately checks out base-branch scripts, so the
workflow evaluating this PR is dev's copy -- still carrying the bug. The fix is
structurally unverifiable in CI until it merges.
So the fixture is reworded instead: the quoted command becomes one the scanner
does not flag, no finding is emitted, and nothing crashes. The backticks, the
bare ${ opener, the apostrophe, the double quote and the embedded newline all
stay -- those are the hostile inputs the test exists to prove safe, and none of
them are what the scanner objects to. The gate is untouched: no ignore entry,
no severity change, no path exclusion.
Refs #1770
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
…st (#1790) * fix(ci): force LF on *.mjs so shebanged scripts stay loadable by vitest scripts/check-changeset-drift.mjs starts with `#!/usr/bin/env node`. No .gitattributes rule covered *.mjs, so with core.autocrlf=true the file checks out CRLF on Windows. Vite's shebang stripping does not survive the \r, leaving a bare `#` as the module's first token: SyntaxError: Invalid or unexpected token. test/scripts/check-changeset-drift.test.ts then loads ZERO of its 8 tests -- silently. Linux CI checks out LF, so dev stayed green and this has been dead since #1481. Node strips CRLF shebangs itself, which is why `node scripts/...` and a plain dynamic import both succeed and mask the defect. Only Vite's transform trips. .gitattributes already encodes this exact lesson for shell scripts (`*.sh text eol=lf` -- "CRLF breaks the shebang"); it was never extended to .mjs. This adds that rule and lands `git add --renormalize -- "*.mjs"` in the same commit so existing checkouts converge. 9 of 40 tracked .mjs blobs stored CRLF in the repo, so the renormalize was not a no-op. The shebang is kept -- squad-ci.yml invokes `node scripts/check-changeset-drift.mjs` directly. Adds test/scripts/mjs-shebang-loadable.test.ts, which imports the real shebanged module through the bundler pipeline and replays every tracked shebang line (15 files) as a fixture. Proven red against the pre-fix tree: 16/16 failed with the production error SyntaxError: Invalid or unexpected token. test/scripts/ goes from 3 passed | 1 failed (33 tests) to 5 passed (58 tests). Closes #1788 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6 * ci: add shebang EOL lint and generalize the eol=lf rule (#1788 follow-up) .gitattributes encoded "CRLF breaks the shebang" for *.sh in one PR and for *.mjs in another, and never generalized. We paid for that lesson twice. This adds scripts/check-shebang-eol.mjs so we do not pay for it a third time. The lint enumerates every tracked file whose blob starts with `#!` (45 today) and checks two things independently: UNPINNED git check-attr says eol != lf -- no rule covers the path CRLF-BLOB the staged blob's first line ends in \r -- a rule was added but `git add --renormalize` never ran, so the repo is still broken The second check matters because a rule alone is a no-op on existing blobs. That is not hypothetical: the scan found 7 violations on a tree that already had the rules, including samples/storage-provider-azure/scripts/*.sh, which `*.sh text eol=lf` has covered for ages. Those two files showed as modified in every worktree and `git restore` never made it stick -- git was normalizing the working tree to LF while the blob stayed CRLF, so the diff could never close. Renormalizing them fixes that permanently. docs/pagefind.yml had the identical condition under `*.yml text eol=lf` and is renormalized for the same reason. Also renormalized packages/squad-cli/src/cli-entry.ts, which stored a CRLF shebang. tsc happens to emit LF today, so the published bin is fine by accident rather than by construction. Rule coverage is extended by measured blast radius, not reflex: *.js (6 blobs), *.cjs (0) and *.ps1 (0) are cheap, but a blanket *.ts rule would renormalize 95 CRLF-storing blobs, so the 3 shebanged .ts entrypoints are pinned by path. The lint reads the index rather than HEAD, so it is correct both as a pre-commit check and as a CI gate (a fresh checkout has index == HEAD), and batches every blob through a single `git cat-file --batch` -- per-file spawns cost 70s on Windows and blew vitest's hook timeout. Proven red before green, in the real repo: staging a shebanged scripts/ _probe-deploy.zsh (matched by no rule) produced Shebang EOL check FAILED: 1 problem(s) across 1 of 46 shebanged file(s). UNPINNED scripts/_probe-deploy.zsh -- git check-attr eol = unspecified and exit 1; removing it returned exit 0. The suite also drives both violation kinds through real `git check-attr` / `git cat-file` in throwaway repos rather than hand-built fixture maps. test/scripts/: 6 files, 67 tests. Refs #1788 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6 * test: reword fixture comment that tripped the unsafe-git detector The comment quoted a repo-wide staging flag verbatim, which security-review.mjs correctly flags as an unsafe-git finding. The code was never doing it -- the fixture stages explicit paths -- so this is a false positive created by prose, and prose is the cheaper thing to change. Worth recording why it surfaced: the finding message quotes the offending command in backticks, so on dev (which does not yet carry #1786) the reporter died with `SyntaxError: Unexpected identifier 'git'` instead of posting the finding. That is #1770 reproducing in the field, on this PR, from an ordinary code comment -- independent confirmation that the #1786 boundary fix is addressing a live defect rather than a theoretical one. Refs #1788 Refs #1770 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6 * build: pin *.snap to LF and document the lint's scope boundary Vitest writes snapshot files with LF unconditionally. `*.snap` had no .gitattributes rule, so it checks out CRLF on Windows and `npm test` dirties a tracked file on every run -- `test/__snapshots__/parser-contracts.test.ts.snap`, found by EECOM. Same CRLF class as the *.mjs rule, different symptom. The .mjs case was a strict parse failure: the suite loaded zero tests. This one loads and passes (16/16) and leaves the tree dirty, which is the more dangerous shape -- a broad `git add` commits pure line-ending noise, or sweeps in an unrelated real change sitting in the same working tree. Reproduced before fixing: clean tree, run the suite, ` M ...parser-contracts. test.ts.snap`. With the rule, the same run leaves the tree clean. Note this does NOT ride the renormalize, contrary to first assumption. The blob is already LF (0 CRLF, 2991 bytes); only the checkout is CRLF (151 CRLF, 3142 bytes). `git add --renormalize -- "*.snap"` stages nothing. What an existing Windows checkout needs is for the file to be rewritten as LF -- which the first `npm test` after this merges does by itself. No manual remediation. Also states the shebang lint's scope boundary in its own header, because a green run should not be read as "no EOL bugs". It covers the strict-parse class, where `#!` is an exact static signature. It does not cover the tool-rewrite class, which has no cheap static signature; guessing at it would trade a sound check for an unsound one. New tool-written file types get pinned by hand. Closes #1788 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6 --------- Co-authored-by: brady gaster <bradygaster@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
Closes #1770
Working as Booster (CI/CD Engineer).
Root cause
actions/github-scriptcompiles itsscript:body withnew AsyncFunction(...), so anything a workflow interpolates into that body via${{ }}becomes JavaScript source, not data. All three repo-health reporters passed their scan output asoutput: `${{ steps.*.outputs.result }}`— a template literal. Security findings legitimately quote the offending code in backticks (scripts/security-review.mjs:212emitsUnsafe git operation: `git push --force-with-lease` …), so the first backtick in the report closed the literal and the remaining JSON was parsed as code, producingSyntaxError: Unexpected identifier 'git'(run 32392990636). The gate died with an opaque parse error and never posted the finding — indistinguishable from a clean run to anyone skimming, which is the worst possible failure mode for a security gate.Sites fixed
Yes, the sibling reporters shared the bug — it was the same line in all three:
squad-repo-health.ymlSecurity Review → Comment on findingssquad-repo-health.ymlArchitectural Review → Comment on findingssquad-repo-health.ymlSquad File Leakage → Comment on leakagesquad-impact.ymlPost impact reportsquad-repo-health.ymlBootstrap Protection,Diff Size Guardgithub-script; they onlyecho/::warning::agentics-maintenance.yml${{ runner.temp }}in arequire()path; generated file, left aloneI scanned every
script: |block in.github/workflows/for embedded${{ }}to build that table, not just the reported job.Fix is at the boundary, not at the message. Messages containing backticks are correct — quoting the offending code is the whole point. Report content now travels through the step environment and is read with
process.env.REPO_HEALTH_OUTPUT, so payload text is never part of the parsed script:No escaping is involved, so there is no escape to get wrong.
Proof the test fails pre-fix
test/scripts/repo-health-comment-transport.test.tsreads the real workflow YAML, extracts eachscript:body and itsenv:map, applies GitHub Actions${{ }}substitution exactly as the runner does, then compiles the result as an async function body (vm.compileFunction, matching github-script'snew AsyncFunction) and runs it against the realscripts/repo-health-comment.mjswith a mocked Octokit. The payload messages contain a backtick pair, a bare${, an apostrophe, a double quote, and an embedded newline.Stashed the workflow fix, kept the test, ran it — red, with the exact production error:
Restored the fix — 5 passed (5). The test asserts the finding text survives verbatim into the comment body, so it also catches a silently-mangled report, not just a crash.
Verification
npm run build— ✅ passes. Version/template churn fromprebuildrestored, not committed.npx vitest run test/scripts/— 38 existing tests pass, plus the 5 new ones.npx eslinton the new test — clean.Note on an unrelated pre-existing failure
test/scripts/check-changeset-drift.test.tsfails to load withSyntaxError: Invalid or unexpected token. I confirmed this reproduces on cleandevwith my changes stashed, so it is not from this PR. Worth a separate issue.No changeset — nothing under
packages/*/srcwas touched.Addendum: this PR reproduced its own bug in CI
Repo Health / Security Review — Permissions & Secretswas red on this PR, at the then-current head SHA, with:That is #1770 — the exact error string, in the exact call frame — crashing on the pull request that fixes it.
Why, and why it is not a defect in the fix.
squad-repo-health.ymlruns onpull_request_targetand deliberately checks out base-branch scripts (noref:override on any of its four checkout steps).pull_request_targetalso takes the workflow definition from the base branch. So the workflow evaluating this PR isdev's copy — which still has the bug. This fix cannot take effect until it merges; it is structurally unverifiable in CI beforehand. The only pre-merge signal available is "the job didn't crash," which is precisely the weak signal that let #1770 live undetected in the first place. Filed separately as a permanent gotcha for this workflow family.What actually triggered it — an accident worth more than the unit test. The test fixture quoted
git push --force-with-leaseas sample finding text. That is a real entry onsecurity-review.mjs'sunsafe-gitdenylist, and fixtures are added lines in the diff, so:test/scripts/repo-health-comment-transport.test.ts— categoryunsafe-git, severityerror.Unsafe git operation: `git push --force-with-lease` — …— wrapping the command in backticks, exactly as designed.dev's backtick template literal, closing it early.SyntaxError: Unexpected identifier 'git'.Nobody designed this. A fixture written to exercise backtick handling caused the production scanner to emit a backtick-wrapped finding that crashed the production reporter — a live, end-to-end reproduction of #1770 in CI, unprompted. It is better evidence than the unit test precisely because it was not constructed.
Resolution. The fixture now quotes a command the scanner does not flag. No finding is emitted, so nothing crashes. Every hostile character stays — the backticks, the bare
${, the apostrophe, the double quote, the embedded newline — because none of those are what the scanner objects to, and they are the entire point of the test. The gate itself is untouched: no ignore entry, no severity change, no path exclusion. All five repo-health reporters are now green.