fix(ci): force LF on *.mjs so shebanged scripts stay loadable by vitest - #1790
Merged
Conversation
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
Contributor
🛫 PR Readiness Check
PR Scope: 📦🔧 Mixed (product + infrastructure)
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 4 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 | 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 | 3 check(s) failing: test, samples-build, docs-quality |
Files Changed (22 files, +5396 −4905)
| File | +/− |
|---|---|
.gitattributes |
+27 −0 |
.github/workflows/squad-ci.yml |
+17 −0 |
.squad-templates/ralph-triage.js |
+567 −567 |
docs/pagefind.yml |
+12 −12 |
docs/playwright.config.mjs |
+13 −13 |
docs/src/plugins/rehype-pagefind-attrs.mjs |
+21 −21 |
docs/tests/api-reference.spec.mjs |
+127 −127 |
docs/tests/astro-features.test.mjs |
+1 −1 |
docs/tests/search.spec.mjs |
+230 −230 |
packages/squad-cli/src/cli-entry.ts |
+1156 −1156 |
packages/squad-cli/templates/ralph-triage.js |
+567 −567 |
packages/squad-sdk/templates/ralph-triage.js |
+567 −567 |
samples/storage-provider-azure/scripts/create-storage.sh |
+67 −67 |
samples/storage-provider-azure/scripts/delete-storage.sh |
+28 −28 |
scripts/capture-pr-screenshots.mjs |
+42 −42 |
scripts/check-shebang-eol.mjs |
+196 −0 |
scripts/generate-api-docs.mjs |
+160 −160 |
scripts/pr-readiness.mjs |
+654 −654 |
scripts/sync-templates.mjs |
+126 −126 |
templates/ralph-triage.js |
+567 −567 |
test/scripts/check-shebang-eol.test.ts |
+157 −0 |
test/scripts/mjs-shebang-loadable.test.ts |
+94 −0 |
Total: +5396 −4905
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
Contributor
🟠 Impact Analysis — PR #1790Risk tier: 🟠 HIGH 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (1 file)
docs (6 files)
root (4 files)
scripts (5 files)
squad-cli (2 files)
squad-sdk (1 file)
templates (1 file)
tests (2 files)
This report is generated automatically for every PR. See #733 for details. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-only Vitest failure mode where CRLF in a shebanged .mjs leaves a stray # after Vite/esbuild “shebang stripping”, causing SyntaxError: Invalid or unexpected token and resulting in suites loading zero tests (silently masking coverage).
Changes:
- Pin
*.mjstoeol=lfvia.gitattributes(and renormalize tracked.mjsfiles) to keep shebanged ESM scripts importable by Vitest on Windows. - Add a Vitest regression suite that imports the real shebanged script via the Vite pipeline and validates all tracked shebanged
.mjsheaders remain parseable when replayed byte-exact. - Apply line-ending normalization across existing
.mjsfiles (no functional content changes intended beyond EOL normalization).
Show a summary per file
| File | Description |
|---|---|
.gitattributes |
Adds *.mjs text eol=lf with rationale to prevent CRLF shebang parsing failures under Vitest/Vite on Windows. |
test/scripts/mjs-shebang-loadable.test.ts |
New regression test that exercises the Vite/Vitest module runner path and validates shebang headers remain loadable. |
scripts/sync-templates.mjs |
Line-ending normalization under the new *.mjs LF rule. |
scripts/pr-readiness.mjs |
Line-ending normalization under the new *.mjs LF rule. |
scripts/generate-api-docs.mjs |
Line-ending normalization under the new *.mjs LF rule. |
scripts/capture-pr-screenshots.mjs |
Line-ending normalization under the new *.mjs LF rule. |
docs/playwright.config.mjs |
Line-ending normalization under the new *.mjs LF rule. |
docs/src/plugins/rehype-pagefind-attrs.mjs |
Line-ending normalization under the new *.mjs LF rule. |
docs/tests/api-reference.spec.mjs |
Line-ending normalization under the new *.mjs LF rule. |
docs/tests/search.spec.mjs |
Line-ending normalization under the new *.mjs LF rule. |
docs/tests/astro-features.test.mjs |
Line-ending normalization under the new *.mjs LF rule. |
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/11 changed files
- Comments generated: 0
- Review effort level: Lite
This was referenced Aug 21, 2026
…-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
Contributor
🏗️ Architectural Review
Automated architectural review — informational only. |
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
Owner
Author
|
Applied |
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
bradygaster
added a commit
that referenced
this pull request
Aug 21, 2026
Triages all 9 failing test files on Windows against a fully-green Linux CI run, so tomorrow's E2E series can tell a real red from expected noise. Key findings: - Separates LOAD FAILURES (suite ran zero tests, coverage silently zero) from assertion failures. Found 2 previously-unknown instances of the #1788 CRLF-shebang dead gate: promote-insider-tag (0 of 15 tests) and patch-esm-imports (3 of 6 tests), the latter previously mislabeled as 'ESM patching noise'. - Proves #1790 does not repair an existing working tree; a forced re-checkout is required or three suites stay red after the merge. - Reclassifies 'scheduler timing' as a real cross-platform argv defect. - Documents that the baseline count moves with terminal (WT_SESSION) and checkout topology (worktree vs clone), independent of any code change. Every real-defect verdict is backed by a controlled single-variable experiment, not by reading code. One item is labeled unknown rather than guessed. Filed: #1793, #1794, #1795, #1796. 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
bradygaster
pushed a commit
that referenced
this pull request
Aug 21, 2026
…istinct mutating paths Two corrections to .squad/e2e/windows-test-baseline.md, both from measurements taken after it was written. 1. The failing SET is not stable run-to-run. Three full runs at 369bba8 gave 9, 8 and 7 failed files. template-sync, consumer-imports, promote-insider-tag and patch-esm-imports move in and out; all pass in isolation (340 passed when run alone). The single-number rule the document exists to provide does not survive this, so it now states a range (7-9) and instructs the reader to treat the failing SET as the signal rather than the count, listing which suites are known to move and which are stable. template-sync's instability is explained: it byte-compares the tracked .github/agents/squad.agent.md while init-scaffolding rewrites that same file from a parallel worker (#1796). PR #1798 should stabilise it. The other three are recorded as UNKNOWN rather than guessed, and filed as #1803. This matters more than the fix it corrects: a flaky suite is indistinguishable from a real regression at 9am. 2. The two working-tree mutations are separate bugs and are now tabulated as such - mechanism, risk and owner per row. squad.agent.md is a CONTENT change (version stamp, high risk, a broad `git add` commits a spurious bump, #1796 -> PR #1798). The .snap churn is PURE EOL with zero content lines (#1793 class, cosmetic, folded into #1790 so it rides the same renormalize rather than stranding a second file needing a second remediation). Both suites PASS while mutating - the seventh instance of the silent-success class tonight, and the one where the unreliable reporter was a green test suite. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
bradygaster
added a commit
that referenced
this pull request
Aug 21, 2026
) test/init-scaffolding.test.ts created its sandbox at join(process.cwd(), '.test-init-scaffold-<hex>') — inside the repository working tree. Several of its cases deliberately run initSquad()/runInit() against a sandbox that has no .git of its own, so init walks upward to find the enclosing git root, finds the *real* repository, and stampVersion() rewrites the tracked .github/agents/squad.agent.md from 0.0.0-source to the package version. The suite passes while doing it. Controlled proof on a clean tree, that file alone: 24 tests passed, then `git status --short` reports `M .github/agents/squad.agent.md`. Any agent running a broad `git add` after a test run commits a spurious version bump. This was known and worked around rather than fixed: test/template-sync.test.ts carries two beforeAll re-syncs whose comments describe it as a race condition to be "minimised to near-zero". The repair ran every time, which is exactly why the damage looked intermittent and environmental. Move the sandbox to the OS temp dir so upward git-root resolution can no longer reach the repository, and add a two-part guard that goes red against the pre-fix state: AssertionError: expected 'M .github/agents/squad.agent.md' to be '' AssertionError: TEST_ROOT (...\.test-init-scaffold-99246f71) must not live under ...\bradygaster-fictional-waddle: expected true to be false The guard compares porcelain status against a file-level beforeAll snapshot rather than asserting emptiness, so a pre-existing local edit to those paths does not produce a false red. The template-sync re-syncs are retained as defence-in-depth — a future suite that reintroduces an in-repo sandbox now fails loudly there instead of leaving a silently mutated tree — and their comments updated to say so. Post-fix: 26 passed, tree clean; 327 passed across the four init/template suites, tree clean. Not addressed here: `npm test` also rewrites test/__snapshots__/parser-contracts.test.ts.snap. That diff is pure EOL with zero content lines (file is i/lf w/crlf; vitest writes LF) — the #1793 CRLF class. Its fix is `*.snap text eol=lf`, which belongs with #1790 so it rides the same renormalize; the attribute alone repairs no existing checkout. Closes #1796 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
bradygaster
added a commit
that referenced
this pull request
Aug 21, 2026
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: brady gaster <bradygaster@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
bradygaster
added a commit
that referenced
this pull request
Aug 21, 2026
… add the #1793 red herring (#1808) Corrections to .squad/e2e/windows-test-baseline.md, all from measurements taken after it merged in #1797. Three of the four are corrections to my own claims. 1. RETRACTED: "#1798 should stabilise template-sync." That prediction was wrong. Flight ran the full suite with #1798 applied and template-sync still failed, with an unrelated mechanism (EBUSY in its own beforeAll). I then confirmed it independently against merged dev - failed one run, passed the next, no code change between. The suite's own beforeAll shells out to sync-templates.mjs, which writes 65 files into the live tree while 273 other files run in parallel workers. It is its own counterparty. The #1796 byte-compare race is retained as a possible SECOND contributor, but demoted to a hypothesis with its evidence rather than a closed case. The original beforeAll comment in that file diagnosed a race, named the wrong counterparty, and closed the case - which is exactly why the bug survived. A correction that repeats the shape of the thing it corrects is worth nothing. 2. NEW, and the most likely false alarm tomorrow: Booster's #1788 guard (mjs-shebang-loadable) fails 13 tests on any un-renormalized checkout, and check-changeset-drift still loads 0 tests. This looks exactly like "the #1788 fix is broken." It is not - the guard is working as designed on a tree that is still CRLF. Verified: attr/text eol=lf with w/crlf, 113 CRLF pairs on disk. Controlled proof, one variable flipped: 13 failed -> 18 passed, and 0 tests -> 8 passed. 26 tests recovered by changing only bytes on disk. Remediation steps included; a plain `git checkout --` is a no-op, the files must be deleted first. 3. #1796 is NOT fully fixed, and my own #1798 guard is the detector. On merged dev a full run still stamps squad.agent.md 0.0.0-source -> 0.13.0. The guard PASSES in isolation (26 passed) and FAILS in the full suite, so the remaining writer is a different suite in a parallel worker. At least 16 suites build sandboxes inside the repo via join(process.cwd(), ...); #1798 fixed one instance of a recurring pattern. Filed as #1807 rather than expanded into. 4. The headline number is marked superseded in place, and the range is now conditioned on tree state rather than stated flat. Stable/moving/EOL-dependent sets are separated, since a suite that fails only on an un-renormalized tree is not noise and must not be filed as such. Also: working-tree side effects are now three rows with mechanism, risk and owner each - squad.agent.md (content, high risk), the .snap churn (pure EOL, folded into #1790 so it rides one renormalize instead of stranding a second file), and the docs/pagefind.yml + .sh + .ps1 churn. 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
This was referenced Aug 21, 2026
bradygaster
pushed a commit
that referenced
this pull request
Aug 23, 2026
`.gitattributes` governs checkout, not files already on disk. #1790 added `*.mjs text eol=lf`, but git only rewrites a working file when a pull also changes that path's index content. Those files are already stored LF, so the merge is a no-op and every pre-#1790 Windows checkout stays CRLF forever. The failure is silent: a CRLF shebang survives Vite's shebang stripping as a bare `#`, the module fails to parse, and the suite reports `no tests` -- a zero that reads as green (#1788). - `npm run fix:crlf` repairs a tree from the index with `git checkout-index -f`. Gated on "no content difference", so a file with real uncommitted edits is skipped and reported, never overwritten. Re-measures after writing rather than trusting the write. - `squad doctor` gains a `working tree line endings` check that flags any eol=lf-pinned file still CRLF on disk, names them, and cites the fix. - Detection lives in `scripts/check-shebang-eol.mjs`, which already owns this invariant family, rather than a third parallel implementation. Deliberately not a `git add --renormalize .`: that rewrites the index -- the opposite side of the defect -- and would sweep nearly every CRLF-storing `.ts` blob into one churn commit, an exclusion `.gitattributes` documents on purpose. And deliberately not a CI gate: CI always has a fresh checkout, so a working-tree assertion there could never observe the failure it exists to catch. Verified capable of failing: with the three files from #1793 forced to CRLF, doctor reports `3 of 174 ... still have CRLF on disk` and the suites collapse from 29 tests to 6. After `npm run fix:crlf`, doctor passes and all 29 run. Review follow-ups (FIDO on #1831): - The check covers every eol=lf-pinned path, but both remediation hints printed `git ls-files --eol "*.mjs"`. That verification cannot observe a pinned non-.mjs file left CRLF -- the same defect class this PR fixes. Widened both hints to the real scope; a test now asserts the message does not re-narrow. - Added spaced-path coverage for `checkWorktreeEol`, asserting it *names* the file rather than merely failing. Its record parsing is a separate implementation from `listWorktreeCrlf` and could otherwise silently diverge. - The batching comment claimed an argv-length guarantee it does not provide; it batches by file count. Reworded to state the actual bound and the math. - Stopped pinning the CRLF-storing `.ts` blob count, which drifts with the tree. Closes #1793 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
bradygaster
added a commit
that referenced
this pull request
Aug 23, 2026
… (#1831) `.gitattributes` governs checkout, not files already on disk. #1790 added `*.mjs text eol=lf`, but git only rewrites a working file when a pull also changes that path's index content. Those files are already stored LF, so the merge is a no-op and every pre-#1790 Windows checkout stays CRLF forever. The failure is silent: a CRLF shebang survives Vite's shebang stripping as a bare `#`, the module fails to parse, and the suite reports `no tests` -- a zero that reads as green (#1788). - `npm run fix:crlf` repairs a tree from the index with `git checkout-index -f`. Gated on "no content difference", so a file with real uncommitted edits is skipped and reported, never overwritten. Re-measures after writing rather than trusting the write. - `squad doctor` gains a `working tree line endings` check that flags any eol=lf-pinned file still CRLF on disk, names them, and cites the fix. - Detection lives in `scripts/check-shebang-eol.mjs`, which already owns this invariant family, rather than a third parallel implementation. Deliberately not a `git add --renormalize .`: that rewrites the index -- the opposite side of the defect -- and would sweep nearly every CRLF-storing `.ts` blob into one churn commit, an exclusion `.gitattributes` documents on purpose. And deliberately not a CI gate: CI always has a fresh checkout, so a working-tree assertion there could never observe the failure it exists to catch. Verified capable of failing: with the three files from #1793 forced to CRLF, doctor reports `3 of 174 ... still have CRLF on disk` and the suites collapse from 29 tests to 6. After `npm run fix:crlf`, doctor passes and all 29 run. Review follow-ups (FIDO on #1831): - The check covers every eol=lf-pinned path, but both remediation hints printed `git ls-files --eol "*.mjs"`. That verification cannot observe a pinned non-.mjs file left CRLF -- the same defect class this PR fixes. Widened both hints to the real scope; a test now asserts the message does not re-narrow. - Added spaced-path coverage for `checkWorktreeEol`, asserting it *names* the file rather than merely failing. Its record parsing is a separate implementation from `listWorktreeCrlf` and could otherwise silently diverge. - The batching comment claimed an argv-length guarantee it does not provide; it batches by file count. Reworded to state the actual bound and the math. - Stopped pinning the CRLF-storing `.ts` blob count, which drifts with the tree. Closes #1793 Co-authored-by: brady gaster <bradygaster@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Root cause
scripts/check-changeset-drift.mjsstarts with#!/usr/bin/env node..gitattributeshad no rule for*.mjs, so withcore.autocrlf=trueit checks out CRLF on Windows. Vite/esbuild'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.tsthen loads 0 of its 8 tests, silently. Linux CI checks out LF, sodevstayed green and this has been dead since #1481 (2026-07-19).Node strips CRLF shebangs itself, which is why
node scripts/check-changeset-drift.mjsand a plainimport()of afile://URL both succeed and mask the defect. Only Vite's transform trips on it.Fix
.gitattributesalready encodes this exact lesson for shell scripts (*.sh text eol=lf— "CRLF breaks the shebang"). It was never extended to.mjs:The shebang is kept —
squad-ci.yml:202invokesnode scripts/check-changeset-drift.mjsdirectly. Verifiednode scripts/check-changeset-drift.mjs --mode=warnstill exits 0 post-renormalize.git add --renormalize -- "*.mjs"lands in the same commit so existing checkouts converge rather than just the rule. 9 of 40 tracked.mjsblobs actually stored CRLF in the repo, so this was not a no-op.Renormalize was scoped to
*.mjsrather than.on purpose:samples/**/*.shanddocs/pagefind.ymlchurn CRLF<->LF in every worktree and a bare--renormalize .sweeps them in.Sweep — every tracked
.mjscarrying a shebang (15)All 15 are covered by
*.mjs text eol=lf. Onlycheck-changeset-drift.test.tsstatically imports a.mjs; every other test reference shells out vianode <path>, where Node's own shebang handling makes CRLF harmless. Shebangs also exist in 5.js, 3.ts, and 1.cjstracked file — none imported by a suite, left out of scope deliberately.Proof the test fails pre-fix
test/scripts/mjs-shebang-loadable.test.tsimports the real shebanged module through the bundler pipeline and replays every tracked shebang line (byte-exact, including its real terminator) as a fixture. Against a CRLF tree, 16/16 red with the production error:Getting this honest took two rejected mechanisms, both of which would have shipped a test that could not fail:
vite.transformWithEsbuildthrows in Vite 7 ("deprecated, migrate to transformWithOxc"), andtransformWithOxcdoes not reproduce the bug — it normalizes CRLF->LF itself. Neither is a valid oracle.`./${dir}/${name}.mjs`). That triggersvite:dynamic-import-vars, which pre-globs candidates at transform time and cannot see runtime-written fixtures — the 15 sweep tests failed with a falseUnknown variable dynamic importinstead of the real error. Switching to a fully opaque specifier routes through Vite's real module runner and produces the genuine failure above.The guard asserts the loaded/parsed outcome —
evaluateDriftandlistFragmentsare callable functions — not the presence of a.gitattributesline.Verification
npx vitest run test/scripts/before1 failed | 3 passed (4), 33 testsnpx vitest run test/scripts/afternode scripts/check-changeset-drift.mjs --mode=warnnpm run buildnpx eslinton new testStaged diffstat (11 files, deletion check empty):
git diff --cached --ignore-cr-at-eol --statreduces to.gitattributes+ the new test only — proving the 9 renormalized files carry nothing but line-ending changes. Nosamples/**/*.shordocs/pagefind.ymlstaged.Closes #1788
Addendum:
*.snappinned to LF, and the lint's scope boundary written downEECOM found that
npm testdirties a second tracked file:test/__snapshots__/parser-contracts.test.ts.snap. Vitest writes snapshots with LF unconditionally;*.snaphad no rule, so it checks out CRLF on Windows. Added*.snap text eol=lf.Reproduced red before fixing (this is the same bar as the rest of the PR):
With the rule, the same run leaves the tree clean.
It does not ride the renormalize — correcting my own first assumption
I expected this to need
git add --renormalizelike the.mjsfiles did. It does not, and the measurement says why:The blob is already LF.
git add --renormalize -- "*.snap"stages nothing — verified, empty diffstat. This is the mirror image of the.mjscase, where the blobs were CRLF and renormalize was the whole fix.What an existing Windows checkout needs is for the working file to be rewritten as LF. A CRLF worktree under the new rule still reports
M(git diff is empty; it's git telling you it will replace the CRLF next time it touches the file). The firstnpm testafter this merges does that rewrite by itself, and the tree stays clean from then on. No manual remediation step.Scope boundary — what this lint does not catch
scripts/check-shebang-eol.mjswould have been green the entire time the snapshot file was dirty, because a.snapfile has no shebang. That limit is now written into the lint's own header rather than left to be rediscovered:.mjsCRLF shebang →SyntaxError, zero tests rungit addcommits noise.snapwritten LF, checked out CRLFClass 1 has an exact, cheap, static signature — the first two bytes are
#!. Class 2 does not: "files some tool writes with LF" cannot be determined by reading the file. Extending the lint to guess would trade a sound check for an unsound one, so the boundary is deliberate and documented. New generated or tool-written file types get pinned in.gitattributesby hand.Class 2 is the one with the data-loss fuse — it is the shape where a broad
git addsweeps up an unrelated real change sitting in the same tree.