fix(sdk): refuse consult when info/exclude resolves outside the project - #1850
Conversation
setupConsultMode hides .squad/ by appending to git's info/exclude, but
resolved that path with `git rev-parse --git-path info/exclude`, which
answers for whichever repository ENCLOSES the directory. The guard that
refuses an already-squadified project checked the worktree-local .squad/,
so the two disagreed about scope: the check was local, the write global.
Two ways that write escapes:
- from a linked worktree, info/exclude resolves to the MAIN checkout;
- from a directory that is not itself a repo root, it resolves to an
ANCESTOR repository.
Either way .squad/ is hidden in checkouts the caller never named, and
info/exclude is untracked and per-clone, so nothing in the repo can undo
it. That is why .squad/e2e/ was invisible despite being absent from
.gitignore, and why `git add -f` was needed for #1819-#1821.
This test suite was itself a live instance of the second case. The
fixture built a fabricated `.git/` directory, which is not a valid
repository, so git walked out of the fixture and every run appended the
consult block to the developer's own checkout. Byte math matches the
forensics in #1826 exactly: 240 B template + 73 B block = the 313 B
observed there. CI never surfaced it because CI clones are discarded.
The fixture now runs `git init`, and the exclude file stays put.
Writing to a worktree-local exclude is not an available alternative:
git keeps no per-worktree info/exclude, and a file placed at
.git/worktrees/<id>/info/exclude is never read. Verified directly. So
refusing is the only safe behaviour, and setupConsultMode now checks
containment via a new isExcludeOwnedBy() export and points the caller at
the main checkout.
Closes #1826
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 20afe6d2-444e-414e-8a39-e67ab67ca6df
🟡 Impact Analysis — PR #1850Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (1 file)
squad-sdk (1 file)
tests (1 file)
This report is generated automatically for every PR. See #733 for details. |
🛫 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 | 2 unresolved Copilot thread(s) — fix and resolve before merging |
| ✅ | CI passing | All checks passing |
Files Changed (3 files, +138 −5)
| File | +/− |
|---|---|
.changeset/lucky-moons-shake.md |
+19 −0 |
packages/squad-sdk/src/sharing/consult.ts |
+55 −2 |
test/sdk/consult.test.ts |
+64 −3 |
Total: +138 −5
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.
🟡 Changes recommended
The new containment logic (and its matching test assertion) uses startsWith('..'), which can misclassify valid in-project paths whose first segment begins with .. (e.g., ..foo) and should be tightened to detect only actual parent traversal (../ or ..\\).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a consult-mode safety bug where setupConsultMode() could append .squad/ to a different checkout’s shared info/exclude (e.g., when invoked from a linked worktree or when git rev-parse resolves an enclosing repository), by refusing to proceed unless the resolved git common dir is contained by projectRoot.
Changes:
- Added
isExcludeOwnedBy(projectRoot)and a refusal guard insetupConsultMode()before mutatinginfo/exclude. - Updated
resolveGitExcludePath()documentation to explicitly warn aboutgit rev-parsescoping behavior. - Made the
setupConsultModetest fixture hermetic (git init) and added regression tests for the linked-worktree and fabricated-.gitcases; added a changeset for the SDK patch.
File summaries
| File | Description |
|---|---|
| packages/squad-sdk/src/sharing/consult.ts | Adds containment check (isExcludeOwnedBy) and refuses consult mode when info/exclude would resolve outside the project; updates docstring. |
| test/sdk/consult.test.ts | Switches fixture to git init to avoid leaking into the enclosing repo; adds regression tests for refusal scenarios. |
| .changeset/lucky-moons-shake.md | Documents the behavior change and ships it as a patch release for @bradygaster/squad-sdk. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const rel = path.relative(path.resolve(projectRoot), commonDir); | ||
| return rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel); |
| const rel = relative(PROJECT_ROOT, result.gitExclude); | ||
| expect( | ||
| rel.startsWith('..') || isAbsolute(rel), | ||
| `consult wrote to ${result.gitExclude}, outside ${PROJECT_ROOT}. ` + | ||
| `That is a different repository's exclude file.`, | ||
| ).toBe(false); |
Closes #1826
Summary
setupConsultModehides.squad/by appending to git'sinfo/exclude, but resolved that path withgit rev-parse --git-path info/exclude— which answers for whichever repository encloses the directory, not one rooted at it. The "already squadified -> refuse" guard checked the worktree-local.squad/. The check was local; the write was global.Two ways the write escapes:
info/excluderesolves to the main checkout;Either way
.squad/is hidden in checkouts the caller never named. Sinceinfo/excludeis untracked and per-clone, no repo-side change can repair it — which is why.squad/e2e/was invisible despite being absent from.gitignore, and whygit add -fwas needed for #1819-#1821.The route was not unprovable — this test suite was doing it
#1826 recorded the worktree route as "unproven and now unprovable." It turns out a different route was live in the repo, and it is reproducible on demand.
The fixture built a fabricated
.git/directory:That satisfies the "is there a
.githere" check but is not a valid repository, sogit rev-parsewalked out of the fixture and answered for the enclosing squad checkout. Every run of these tests appended the consult block to the developer's own.git/info/exclude.Measured directly on my clone:
240 + 73 = 313 — byte-identical to the forensic math in #1826, which measured the live poisoned file at exactly 313 B. CI never surfaced it because CI clones are discarded, precisely as the issue predicted.
After this change, the same measurement holds steady across all five consult-adjacent suites (122 tests):
One of the issue's two suggested fixes is not available
#1826 offered: "refuse ... or write to a worktree-local exclude."
The second option does not work. Git keeps no per-worktree
info/exclude— a file placed at.git/worktrees/<id>/info/excludeis simply never read. Verified directly:Taking that option would have produced a quieter bug: consult mode silently failing to hide
.squad/, leaving it exposed to accidental commits. Refusing is the only safe behaviour, so that is what this implements.Change
isExcludeOwnedBy(projectRoot)(new export) — verifies the git common dir is contained byprojectRoot. One check covers all three cases: normal repo passes; linked worktree fails; nested-in-ancestor fails.setupConsultModerefuses before any mutation, with a message naming the cause and directing the caller to the main checkout.resolveGitExcludePathdocstring corrected. It claimed to handle "worktrees/submodules" — the inverse of what it does. That claim is the proximate cause of this defect and is now documented as a hazard with an explicit pointer to the containment check.git initinstead of a fabricated.git/..gitdirectory (the exact old-fixture shape).Validation
npm run buildpasses. Changeset included (patch, squad-sdk).Follow-ups, not in this PR
.squad/identity/prd-consult-mode.md:229advises "Do not hard-coderesolve(cwd, '.git/info/exclude'). In git worktrees..." and prescribesgit rev-parse --git-path info/exclude. That guidance is what produced this bug. It needs correcting, but it is identity/PRD material — flagging for Scribe rather than editing here.squad doctorcheck — suggestion (2) in squad consult writes .squad/ to the shared info/exclude, silently hiding state in main and all sibling worktrees #1826, to detect and repair clones already poisoned. This PR stops new poisonings; it cannot heal existing ones. Worth its own issue.# Squad consult mode (local only)block from.git/info/exclude.Routing note: #1826 names EECOM for
consult.tswith CAPCOM/FIDO for the doctor check. Handled here directly because it gates the E2E re-run this morning — EECOM should review.