fix(ci): enumerate the EOL blob check by rule, not by shebang - #1805
Conversation
check-shebang-eol.mjs enumerated candidates by `#!` at byte 0 and then asked two questions about that set: is it pinned to LF, and does its blob store CRLF. The second question has the wrong enumeration. The churn class is defined by `eol=lf rule + CRLF blob`, not by shebangs. The two sets overlap and neither contains the other, so the gate was blind to every pinned file without a `#!` -- which is most of them. On dev @ dbb4c33 that blindness was live: tracked files : 1811 files with eol=lf attribute: 173 of those, CRLF blob : 2 - docs/src/pages/rss.xml.js (no shebang: gate blind) - packages/squad-cli/src/remote-ui/app.js (no shebang: gate blind) Both are .js, both covered by the `*.js text eol=lf` rule added in #1790, and neither was renormalized when that rule landed -- so the rule itself created the condition while the gate reported "all 45 shebanged files are pinned to LF", exit 0. That half is the dangerous half. A rule over a CRLF blob means git normalizes the working tree to LF while the blob never moves, so the diff can never close: modified in every worktree forever, `git restore` does not stick, and a broad `git add` sweeps it along with whatever real change shares the tree. Only `git add --renormalize` clears it. Nothing about it is fatal, which is why it survives for years -- rss.xml.js has exactly ONE CRLF line and has presumably churned unnoticed for as long as the rule has existed. The fix is a union, not a replacement. Each invariant now walks its own set: UNPINNED over files starting with `#!` -- only this walk finds a shebanged file that no rule pins CRLF-BLOB over files where check-attr eol=lf -- the churn class eolAttributes now feeds paths over stdin rather than argv, because it is called with every tracked file (~1800 paths, >70KB) and that overflows the 32767-char Windows command line. Both offending files renormalized. Pure EOL: `git diff --ignore-cr-at-eol` is empty across the pair, zero content lines. Proven red before green. Against dev's pre-widening gate the two new tests fail: FAIL > flags a NON-shebang file pinned to LF whose blob stores CRLF - [ ObjectContaining{file: "web/app.js", kind: "crlf-blob"} ] + [] FAIL > scans far more LF-pinned files than shebanged ones TypeError: Cannot read properties of undefined (reading 'length') Tests 2 failed | 9 passed (11) Restored: 11 passed. The non-shebang test asserts the fixture is absent from the shebang enumeration first, so it cannot pass by accident if the scan is ever narrowed back down. The scope boundary is documented in the file rather than left to be rediscovered: still not covered is a file that ought to be pinned, has no shebang, and has no rule yet -- vitest snapshots until `*.snap` was added. There is no cheap static signature for "some tool writes this with LF", and guessing would trade a sound check for an unsound one. Closes #1804 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
🟡 Impact Analysis — PR #1805Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affecteddocs (1 file)
scripts (1 file)
squad-cli (1 file)
tests (1 file)
This report is generated automatically for every PR. See #733 for details. |
|
This PR touches No shipped behavior changes, so a changeset would announce a fix that no consumer experienced. That is manufactured release-note signal, which is the same species of false coverage this workstream has spent the night removing. Same reasoning accepted for |
🛫 PR Readiness Check
PR Scope: 📦🔧 Mixed (product + infrastructure)
|
| Status | Check | Details |
|---|---|---|
| ✅ | Single commit | 1 commit — clean history |
| ✅ | 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 | Missing .changeset/*.md or CHANGELOG.md edit — run npx changeset add (or add skip-changelog label) |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | No Copilot review threads |
| ✅ | CI passing | All checks passing |
Files Changed (4 files, +470 −365)
| File | +/− |
|---|---|
docs/src/pages/rss.xml.js |
+1 −1 |
packages/squad-cli/src/remote-ui/app.js |
+334 −334 |
scripts/check-shebang-eol.mjs |
+94 −30 |
test/scripts/check-shebang-eol.test.ts |
+41 −0 |
Total: +470 −365
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
Updates CI EOL validation to detect CRLF blobs across all LF-pinned files and normalizes two affected files.
Changes:
- Separates shebang and LF-pinned file enumeration.
- Uses stdin-safe attribute checks.
- Adds regression coverage and removes CRLF-only churn.
Show a summary per file
| File | Summary |
|---|---|
test/scripts/check-shebang-eol.test.ts |
Adds regression tests for widened enumeration. |
scripts/check-shebang-eol.mjs |
Expands EOL validation to all LF-pinned files. |
packages/squad-cli/src/remote-ui/app.js |
Removes CRLF line endings only. |
docs/src/pages/rss.xml.js |
Removes CRLF line endings only. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/4 changed files
- Comments generated: 0
- Review effort level: Lite
Closes #1804. Follow-up to #1788 / #1790.
Root cause
scripts/check-shebang-eol.mjsenumerated candidate files by#!at byte 0, then asked two questions about that one set: is it pinned to LF, and does its blob store CRLF.The second question has the wrong enumeration. The churn class is defined by
eol=lfrule + CRLF blob -- not by shebangs. The two sets overlap and neither contains the other, so the gate was blind to every pinned file without a#!, which is most of them.The blindness was live on
devMeasured at
dbb4c33b:Both are
.js, both covered by the*.js text eol=lfrule added in #1790, and neither was renormalized when that rule landed. So the rule created the condition while the gate it shipped alongside reported:A/B against the same tree, old gate vs new:
devpassed: all 45 shebanged files are pinned to LF- EXIT=0FAILED: 2 problem(s), bothCRLF-BLOB- EXIT=1Why this half is the dangerous half
A rule over a CRLF blob means git normalizes the working tree to LF while the blob never moves, so the diff can never close. Modified in every worktree forever,
git restoredoes not stick, and a broad staging command sweeps it up along with whatever real change happens to share the tree. Only--renormalizeclears it.Nothing about it is fatal, which is exactly why it survives for years.
rss.xml.jshas one CRLF line -- the failure mode in miniature, and proof it is scale-independent.Fix -- a union, not a replacement
Each invariant now walks its own set:
UNPINNED#!#!walk finds a shebanged file that no rule pinsCRLF-BLOBgit check-attr eol=lfAlso:
eolAttributesnow feeds paths over stdin rather than argv. It is called with every tracked file now (~1800 paths, >70KB), which overflows the 32767-character Windows command-line limit.Both offending files renormalized. Pure EOL --
git diff --cached --ignore-cr-at-eol --statacross the pair is empty, zero content lines.Proof the tests fail pre-fix
Swapped in dev''s pre-widening
check-shebang-eol.mjs, kept the new tests:Restored the fix -- 11 passed. The non-shebang test asserts its fixture is absent from the shebang enumeration before asserting the violation, so it cannot pass by accident if the scan is ever narrowed back down.
Scope boundary -- written into the file, not left to be rediscovered
Still not covered: a file that ought to be pinned, has no shebang, and has no rule yet. Vitest snapshots were exactly this until
*.snap text eol=lfwas added. There is no cheap static signature for "some tool writes this file with LF" -- it cannot be determined by reading the file -- so guessing would trade a sound check for an unsound one. New tool-written types get pinned by hand; from the moment a rule exists,CRLF-BLOBenforces it.Verification
npm run build-- passes.prebuildversion/template churn restored, not committed.npx vitest run test/scripts/-- 7 files, 75 tests, all passing.node scripts/check-shebang-eol.mjs--EOL check passed: 45 shebanged file(s) all pinned to LF, 173 LF-pinned file(s) all storing LF blobs.EXIT=0.node scripts/security-review.mjsandarchitectural-review.mjs-- both clean locally, pre-checked becausedevdoes not yet carry fix(ci): stop splicing repo-health report content into github-script bodies #1786 and any emitted finding would crash the base-branch reporter.Changeset
packages/squad-cli/src/remote-ui/app.jsis touched, sochangelog-gatefires -- but the change is zero content lines, purely CR removal in the stored blob. There is no behavior to announce and a release note would be manufactured signal. Requestingskip-changelogon the same reasoning accepted forcli-entry.tsin #1790.Measurement note
CR counts here come from reading blob bytes directly into a Buffer via
git cat-file. Pipinggit cat-filethrough PowerShell re-encodes line endings and returns a confidently wrong number.