fix: repair CRLF working trees that .gitattributes cannot reach (#1793) - #1831
Conversation
🛫 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 | Changeset file found |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | 1 active Copilot thread(s) resolved (2 outdated skipped) |
| ❌ | CI passing | 1 check(s) failing: test |
Files Changed (7 files, +548 −3)
| File | +/− |
|---|---|
.changeset/doctor-worktree-eol-check.md |
+5 −0 |
CONTRIBUTING.md |
+30 −0 |
package.json |
+1 −0 |
packages/squad-cli/src/cli/commands/doctor.ts |
+99 −0 |
scripts/check-shebang-eol.mjs |
+74 −3 |
scripts/fix-crlf-worktree.mjs |
+126 −0 |
test/scripts/crlf-worktree-repair.test.ts |
+213 −0 |
Total: +548 −3
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🟡 Impact Analysis — PR #1831Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (3 files)
scripts (2 files)
squad-cli (1 file)
tests (1 file)
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes implement the stated remediation and diagnostics with good safety checks and integration test coverage, with only minor output/test-hardening nits noted.
Pull request overview
Adds a local-only remediation for the “LF in index, CRLF on disk” residual after introducing eol=lf rules, and surfaces detection/repair guidance via squad doctor and a single documented npm script.
Changes:
- Introduces
npm run fix:crlf(scripts/fix-crlf-worktree.mjs) to rewrite LF-pinned files from the index while skipping real content edits. - Extends
scripts/check-shebang-eol.mjswith shared working-tree CRLF detection (listWorktreeCrlf) and a safety gate (listContentModified). - Adds a new
squad doctorcheck (checkWorktreeEol) plus documentation and tests covering real temp git repos.
File summaries
| File | Description |
|---|---|
| test/scripts/crlf-worktree-repair.test.ts | New integration tests validating detection, doctor behavior, and safe repair in real git repos. |
| scripts/fix-crlf-worktree.mjs | New repair script that forces checkout from index for LF-pinned paths that are CRLF on disk. |
| scripts/check-shebang-eol.mjs | Exports shared helpers to detect LF-pinned files with CRLF in worktree and detect true content modifications. |
| packages/squad-cli/src/cli/commands/doctor.ts | Adds working tree line endings doctor check and wires it into runDoctor. |
| package.json | Adds fix:crlf npm script entry point. |
| CONTRIBUTING.md | Documents the failure mode, symptom, correct repair, and why common “fixes” don’t work. |
| .changeset/doctor-worktree-eol-check.md | Changeset documenting the new doctor check and repair command. |
Review details
Suppressed comments (1)
scripts/fix-crlf-worktree.mjs:106
- This verify hint is currently scoped to
*.mjs, butrepair()operates on every tracked path pinned toeol=lf(perlistWorktreeCrlf). Printing a verification command that matches that broader scope makes the output more accurate.
console.log('\nVerify with: git ls-files --eol "*.mjs" (every entry should read w/lf)');
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bradygaster
left a comment
There was a problem hiding this comment.
Verdict: APPROVE WITH NITS
Independent FIDO repro passed. I built the PR worktree with npm run build, then verified the local-only failure mode CI cannot exercise:
- Baseline doctor:
174 LF-pinned file(s) all LF on disk;Summary: 13 passed, 0 failed. - Forced these three LF-pinned files to CRLF on disk without changing the index:
packages/squad-cli/scripts/patch-esm-imports.mjs,scripts/check-changeset-drift.mjs,scripts/promote-insider-tag.mjs. git diff --name-only -- <files>showed no content diff, whilegit ls-files --eolshowedi/lf w/crlf attr/text eol=lffor all three.- Poisoned doctor reported the right failure and named all three files:
3 of 174 LF-pinned file(s) still have CRLF on disk;Summary: 12 passed, 1 failed. npm run fix:crlfrepaired all three and exited 0; post-repair.mjscount was0 of 42withw/crlf; doctor returned toSummary: 13 passed, 0 failed.- Guard check held: after adding a real content edit plus CRLF to
scripts/promote-insider-tag.mjs,npm run fix:crlfskipped it, exited 1, and preserved myFIDO_PRECIOUS_GUARDmarker. No clobber.
Targeted tests also ran with the claimed collection counts: check-changeset-drift 8, promote-insider-tag 15, patch-esm-imports 6. I also ran the new repair suite: 13. Total targeted run: 42 passed. node scripts/check-shebang-eol.mjs still passes, so the refactor did not weaken the original index/blob gate.
The two deliberate non-actions hold. git add --renormalize . would attack the wrong side of this defect and cause unrelated line-ending churn; I measured 94 CRLF-storing .ts blobs in this worktree, so the documented 95 is stale by one but the argument is still correct. Not adding a CI gate is also correct: a fresh checkout makes the working-tree check permanently green, which is decoration rather than a gate.
Nits only: my local totals are 174 LF-pinned files / 42 .mjs files, not EECOM's 173 / 41. That appears explained by the added repair script and is not a blocker.
`.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>
4d0d79d to
f294111
Compare
…chives (#1838) * chore(squad): record #1793 session state, decisions and logs Merges both decision inbox records from the #1793/PR #1831 work (EECOM's CRLF working-tree repair, FIDO's independent review) and elevates the gate-instructions principle as its own entry: a correct check paired with a remediation hint that cannot observe the failure is still a broken gate. Also captures the gh-aw Tier 1/2/3 triage decisions and the .squad-must-not-be-gitignored decision, plus orchestration and session logs from both waves. Log files are force-added: .squad/ is hidden by the shared .git/info/exclude that #1826 re-creates, which makes new files under .squad/ silently unstageable while git status reads clean. Tracked files are unaffected. Refs #1793 #1826 #1830 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e20e987-a753-41c4-a6f8-992d863a32cd * chore(squad): merge 3 gh-aw decision records, log #1824 wave Merges FIDO's #1832 review, RETRO's shell-input security contract, and Procedures' fail-loud command-parse record into decisions.md (48 -> 56 entries). Adds orchestration + session logs for the wave. Archival correctly skipped: all entries within a 4-day window, none older than the 7-day threshold. eecom/history.md held back pending repair of a dangling archive pointer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e20e987-a753-41c4-a6f8-992d863a32cd * fix(squad): restore 3 archives lost by the 2026-08-20 summarization run A Scribe run at 2026-08-20T11:59:44-0700 wrote provenance pointing at history-archive-2026-08-20T11-59-44-0700.md into eecom, pao and procedures. None of the three files was ever created. The pointers dangled for two days with `git status` clean, because .squad/ was in .git/info/exclude (#1826) and new files there are silently unstageable. The trim landed in c508d86 -- titled "record gh-aw triage session state and repair archives". It performed the loss it claimed to repair. Recovered from the last committed pre-trim state (c508d86^ / 3dace32e). Not a proven byte-exact snapshot of disk at pointer-write time; uncommitted drift may have existed. Verified by blob SHA equality via `git ls-files -s`: eecom 771d9e0 15,063 B / 129 lines pao 012ea60 13,605 B / 120 lines procedures 1298528 13,653 B / 119 lines Content loss was invisible to size: procedures/history.md is 15,370 B today vs 13,653 B pre-trim -- 1,717 bytes LARGER -- and was still missing 35 non-blank lines. Later sessions appended while trimmed material stayed gone. Only content comparison detects this. pao was missing 44 lines. Also restored eecom/history.md provenance (H1 + 4 lines) stripped in an earlier repair attempt. Swept all 9 archive pointers across every agent: 0 remaining dangling. Refs #1836, #1826 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e20e987-a753-41c4-a6f8-992d863a32cd * docs(squad): record the declared-vs-enforced principle and its corollary A requirement with no observer is documentation, not a rule. Six confirmations measured this working day, including the positive case -- RETRO's hop-1 shell contract, which openly labels itself unenforced and files the gate (#1834). That is the safe form; the failure mode is prose that reads as a guarantee. Sub-pattern: an unenforced requirement does not merely fail silently, it can emit an affirmative false claim of compliance -- "Roster set read from team.md" over a preset list; a commit titled "repair archives" that performed the loss (c508d86). False provenance is the worst case. Corollary -- a measured number that misstates its unit collapses the same way: the observation happened, but the number does not mean what it claims. Six instances on one file in one day, terminating in a false verdict: Scribe's "UNRECOVERABLE -- all commits < 10KB" against paths holding 41 commits at 13,605 B. A false measurement that becomes a conclusion to stop looking. Two rules: state the unit and the producing command for every number; prefer a measure nothing can reinterpret. During the repair, char count, line count and heading containment all passed on a BOM'd, newline-stripped file. Only the blob SHA could observe it. The record carries zero navigational line numbers -- every citation is a grep anchor that revalidates on read. Drift pressure scales with writer count, so decisions.md (union merge, every agent appending) is the last place to cite a line: the permanently-green restatement moved ~L604 -> L598 in one session, unannounced. Refs #1836, #1824, #1812, #1834 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e20e987-a753-41c4-a6f8-992d863a32cd * chore(squad): merge 3 decision records, log the #1812 roster-binding wave Merges the gh-aw wave-1 decision inbox into decisions.md (56 -> 59 entries) and records the #1812 orchestration. Records merged: - procedures-1812-activate-roster-binding (Team Guard Step TG-2) - fido-1837-review (adversarial review, APPROVE WITH NITS) - flight-declared-vs-enforced (the governing principle) Archival was DISABLED for this run by coordinator override. decisions.md is 89,955 B, past the 51,200 aggressive gate, and that archival path has silently destroyed history twice in four days (repaired by #1782, then again in 6d8bb3f). A large decisions.md is a performance problem; a lossy archive is a correctness one. Tracked as #1836. Scribe reported this as "skipped by override" rather than "not required" -- it did not claim a measurement it never took. New files under .squad/log/ and .squad/orchestration-log/ required 'git add -f' (#1826). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e20e987-a753-41c4-a6f8-992d863a32cd * fix(squad): repair BOM, CR, heading and link defects found in review GitHub's automated reviewer flagged four defect classes on PR #1838. All independently verified against the filesystem before repair. BOM (regression introduced by this PR): four history.md files carried a UTF-8 BOM (EF BB BF). parseHistory() preserves it but serializeHistory() matches titles with ^#\s+, so a BOM'd H1 fails to match and the title is silently dropped on round-trip. Char counts, line counts and heading containment all pass on a BOM'd file -- .NET ReadAllText strips BOMs silently, so only byte-level inspection observes it. Verified clean on origin/dev and BOM'd at HEAD, confirming this PR introduced them. Bare CR: eecom/history.md held one bare CR that had consumed the "r" of "research", rendering "esearch". HEAD bytes read "implement, " 0D "esearch" -- the CR replaced the letter rather than splitting around it, so restoring it required CR->r, not CR->space. Verified: 0 bare CR, 46 CRLF pairs preserved, 75 lines before and after, exactly one line differing. Adjacent H3 headings: three dated wrappers in decisions.md were each followed by an undemoted duplicate source heading, plus one ADDENDUM subsection using H3. Each caused the parser to flush a record with no body, producing four phantom decisions. The Scribe merge contract requires a spliced body's shallowest heading to land at H4; its parenthetical example shows only "## -> ####", and the rule was applied faithfully to H2 (measured: 0 stray H2) but never generalized to H3. Demoted to H4: 55 H3 / 85 H4, conserved, zero adjacent pairs remaining. Wrong repository: ten links pointed at bradygaster/friendly-guide -- a worktree name, not a repository -- across five log and orchestration-log files. Corrected to bradygaster/squad. Left untouched: the H3 inside a fenced block at decisions.md:986. The content is a normative example and is correct; the parser is the defect. Tracked separately as #1839. Refs #1838, #1839 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e20e987-a753-41c4-a6f8-992d863a32cd --------- Co-authored-by: brady gaster <bradygaster@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e20e987-a753-41c4-a6f8-992d863a32cd
Closes #1793. Working as EECOM (Core Dev).
The residual, not the fix
#1790 added
*.mjs text eol=lf. That rule governs checkout. Git only rewrites a working file when a pull also changes that path's index content — and these files are already stored LF. So the merge is a no-op on disk, and every pre-#1790 Windows checkout stays CRLF indefinitely. Pulling the fix does not repair your tree.I verified the mechanism rather than assuming it. In this state the file is content-clean: git's checkin filter normalizes the CRLF away, so
git hash-object --pathequalsgit rev-parse :<path>exactly andgit diffis empty. Git has nothing to restore, which is why the condition is so durable — and whygit checkout -- <path>is not a reliable remedy for it.The failure is silent. A CRLF shebang survives Vite's shebang stripping as a bare
#, the module fails to parse, and vitest reportsno tests— a zero that reads as green (#1788).What this adds
npm run fix:crlfgit checkout-index -f, which writes unconditionally. Gated on "no content difference" — a file with real uncommitted edits is skipped and reported, never overwritten. Re-measures after writing rather than trusting the write.squad doctor→working tree line endingseol=lf-pinned file still CRLF on disk, names up to 5 plus a+N more, and cites the fix. Returns not applicable outside a git repo or when nothing is pinned.listWorktreeCrlf/listContentModifiedexported fromscripts/check-shebang-eol.mjs, which already owns this invariant family — not a third parallel implementation.CONTRIBUTING.mdTwo things I deliberately did not do
No
git add --renormalize .— that rewrites the index, the opposite side of the defect; the blobs are already correct, the disk is what's wrong. It would also sweep nearly every CRLF-storing.tsblob in the repo into one churn commit, an exclusion.gitattributesdocuments on purpose. The repair correctly produces no commit at all. Diff here is 7 files.No CI gate — CI always has a fresh checkout, so a working-tree assertion there could never observe the failure it exists to catch. That is a permanently-green gate, which is equivalent to no gate (
.squad/decisions.md, 2026-08-20 test bar; the shared root cause of #1824/#1812/#1801/#1822/#1827). This condition is local-only by nature, so it belongs insquad doctor, which runs on the developer's actual disk. The omission is documented in-comment incheck-shebang-eol.mjsso nobody "fixes" it later.Proof the check can fail
A check that only passes on a clean tree proves nothing. I forced the three files from the issue to CRLF and measured.
Before — known-bad input:
The three suites in that state — the original symptom, reproduced:
The remediation:
After:
Acceptance criteria
eol=lf-pinned paths reportsw/lf, the 42.mjsamong them included.check-changeset-drift8 +promote-insider-tag15 +patch-esm-imports6 = 29, none reportingno tests.squad doctorflags anyw/crlf.mjs, proven to fail on known-bad input and pass after repair. The check is in fact wider than the criterion asks: it covers everyeol=lf-pinned path, not just.mjs.Tests
14 new in
test/scripts/crlf-worktree-repair.test.ts, driving real temp git repos (core.autocrlf=false, committed*.mjs text eol=lf) — not mocks. Coverage includes:failfirst, so a stuck-green check cannot pass it.repair()never overwrites a file with real uncommitted edits.git ls-files --eolrecord shape is pinned on both sides, since the attr value contains a space and cannot be whitespace-split.43 passed(14 new + the 29 restored).npm run lintclean,npm run buildclean,node scripts/check-shebang-eol.mjsclean (46 shebanged files pinned to LF, 174 LF-pinned files all storing LF blobs).Review follow-ups (FIDO)
The remediation hint was scoped narrower than the check. Both
doctor.tsandfix-crlf-worktree.mjsprintedgit ls-files --eol "*.mjs", but the check counts everyeol=lf-pinned path — 174, only 42 of them.mjs. A pinned non-.mjsfile left CRLF would have verified all-clear. That is the same defect class this PR fixes, so it needed fixing rather than waving off. Both hints widened; a test now asserts the message does not re-narrow, mutation-proven against the old text.Spaced-path coverage for
checkWorktreeEol. It has its own record parser, separate fromlistWorktreeCrlf, so the two could silently diverge. The new test asserts the check names the spaced file rather than merely failing — mutation-testing showed a truncating parser still returnsfailand still counts1 of 1, so a status-only assertion would have passed it while reporting a filename that does not exist on disk.Batching comment no longer over-promises. It bounds file count, not argv length. Measured rather than assumed: longest tracked path is 80 chars, so a full 200-file batch is ~16K against the 32767 limit, and overflowing would take a ~164-character mean path. Length accounting deliberately declined, and the comment now says so.
Stopped pinning drift-prone counts. The CRLF-storing
.tsblob figure is now described rather than hardcoded — it measures 94 today, and.gitattributesstill says 95. Left that file alone; correcting its comment is unrelated churn.Full suite: 9 failures, all confirmed pre-existing on
dev. Two doctor tests failed only under parallel load and pass in isolation; the other 5 I verified by stashing this branch's edits and re-running — they fail identically without my changes. None relate to EOL handling.Note for review
parseEolRecordis intentionally duplicated betweenscripts/check-shebang-eol.mjsanddoctor.ts. The repo script is not published insidepackages/squad-cli, sodist/cannot import it without breaking every installed CLI. Documented in-comment on both sides, with the parse shape pinned by tests on both sides.I hit this exact bug while building the fix: the editor wrote
scripts/fix-crlf-worktree.mjswith CRLF, it wasn't in the index yet so.gitattributesnever smudged it, and the new suite reportedno testswithSyntaxError: Invalid or unexpected token. Good confirmation the class is real and easy to reintroduce.#1788 and #1790 stay open on their own merits — this is the residual, not a duplicate.