fix(test): stop init-scaffolding from mutating tracked repo files (#1796) - #1798
Conversation
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: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6
🟢 Impact Analysis — PR #1798Risk tier: 🟢 LOW 📊 Summary
🎯 Risk Factors
📦 Modules Affectedtests (2 files)
This report is generated automatically for every PR. See #733 for details. |
🛫 PR Readiness Check
PR Scope: 🔧 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 | No source files changed — changeset not required |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ❌ | Copilot threads resolved | 1 unresolved Copilot thread(s) — fix and resolve before merging |
| ✅ | CI passing | All checks passing |
Files Changed (2 files, +67 −13)
| File | +/− |
|---|---|
test/init-scaffolding.test.ts |
+53 −3 |
test/template-sync.test.ts |
+14 −10 |
Total: +67 −13
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 test isolation issue where the init-scaffolding test suite could accidentally run init against a sandbox located inside the repository, allowing runInit()/initSquad() to walk up to the real git root and mutate tracked files (notably .github/agents/squad.agent.md). It also adds a regression guard and updates template-sync comments to reflect the new defense-in-depth posture.
Changes:
- Move the
init-scaffoldingsandbox root toos.tmpdir()to prevent git-root “escape” into the real repo. - Add a working-tree containment / mutation guard to detect future regressions.
- Update
template-synccommentary to document the historical cause and the retained defense layers.
Show a summary per file
| File | Description |
|---|---|
| test/init-scaffolding.test.ts | Move sandbox to OS temp dir and add a git-status guard to ensure tracked files aren’t mutated by the suite. |
| test/template-sync.test.ts | Update comments explaining why template re-syncs remain as defense-in-depth after #1796. |
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/2 changed files
- Comments generated: 1
- Review effort level: Lite
| // A path inside REPO_ROOT produces a relative path that neither escapes | ||
| // upward nor is absolute. Anything else means the sandbox is contained. | ||
| const isInsideRepo = rel !== '' && !rel.startsWith('..') && !isAbsolute(rel); |
…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
… 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
Closes #1796.
What was actually mutating the tree
Not the acceptance suite.
test/acceptance/acceptance.test.tsrun alone leavesgit statusempty — the worktree guard short-circuitssquad inithere, so the obvious hypothesis is wrong.The writer is
test/init-scaffolding.test.ts:21:The sandbox lives inside the repository. Several cases deliberately exercise init against a sandbox with no
.gitof its own (initSquad succeeds when git is not initialized at all, and the two monorepo-subfolder cases). Init then walks upward to find the enclosing git root, finds the real repository, andstampVersion()rewrites tracked.github/agents/squad.agent.mdfrom0.0.0-sourceto the package version.Controlled proof
Clean tree, that suite alone:
24 tests, all green, silently mutating tracked source. That is the hazard: an agent running a broad
git addafter a test run commits a spurious version bump.It was known, and worked around rather than fixed
test/template-sync.test.tscarries twobeforeAllre-syncs ofscripts/sync-templates.mjsagainst the real repo. Their comments describe the cause accurately — a race withinit-scaffoldingrunningrunInit()under the repo root — and then choose to "minimis[e] the window to near-zero".That is why it survived: the repair ran on every full run, so the damage was intermittent and read as environmental. This is the same shape as #1788 and #1795 — the system reports success while the real outcome is invisible.
The fix
One line: sandbox moves to
tmpdir(), so upward git-root resolution can no longer reach the repository.Plus a two-part guard that is red against the pre-fix state:
Produced by reverting only the one-line sandbox move and re-running — the rest of the change held constant, so the red isolates the mechanism rather than the diff.
The status guard compares porcelain output against a file-level
beforeAllsnapshot instead of asserting emptiness, so a developer with a pre-existing local edit to those paths does not get 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 rather than leaving a silently mutated tree. Their comments are updated to describe that role instead of the fixed race.
Verification
init-scaffoldingpost-fixtemplate-sync+init+init-scaffolding+init-sdknpm run buildgit diff --cached --statgit diff --cached --diff-filter=D --name-onlyNo changeset: test-only, nothing under
packages/*/src.Deliberately not included
npm testalso rewritestest/__snapshots__/parser-contracts.test.ts.snap(reproduced:parser-contracts.test.tsalone, 16 passed, file modified). That diff is pure EOL with zero content lines — the file isi/lf w/crlfand vitest writes LF. It is the #1793 CRLF class, not this one.Its fix is
*.snap text eol=lf, and I checked #1790's diff: it pins*.mjs,*.js,*.cjs,*.ps1and three.tspaths but does not cover*.snap. So this is a real gap in #1790, not a duplicate. It belongs there rather than here, because per #1793 the attribute repairs nothing on an existing checkout — it only helps after the renormalize #1790 already requires. Flagged for routing; not taken unilaterally, since #1790 is editing that exact region of.gitattributestonight.