test(worktree): regression guard for .git file vs directory (#521)#523
Merged
bradygaster merged 3 commits intobradygaster:devfrom Mar 23, 2026
Merged
test(worktree): regression guard for .git file vs directory (#521)#523bradygaster merged 3 commits intobradygaster:devfrom
bradygaster merged 3 commits intobradygaster:devfrom
Conversation
diberry
pushed a commit
to diberry/squad
that referenced
this pull request
Mar 22, 2026
…, add statSync guard
- Remove vi.mock('child_process') and all helpers — implementation reads
.git via fs.readFileSync, never calls child_process; the mock was dead
code that gave false confidence
- Fix gitdir in all worktree test fixtures:
'../../.git/worktrees/feature-521' resolved above the temp dir;
corrected to '../main/.git/worktrees/feature-521' which resolves to
tmp/main/.git/worktrees/feature-521 as expected
- Create main/.git/worktrees/feature-521/ directory in each fixture so
the real directory structure matches the gitdir pointer
- Add statSync guard to getMainWorktreePath() (resolution.ts) and
resolveWorktreeMainCheckout() (detect-squad-dir.ts): verify that
mainCheckout/.git exists and is a directory before trusting it; guards
against crafted .git files that redirect Squad to attacker-controlled paths
- Add two statSync guard tests: resolveSquad() and detectSquadDir() both
return null/fallback (not crash) when gitdir points to a non-existent path
All 9 worktree regression tests pass.
Fixes PR bradygaster#523 blockers as requested by Dina.
Working as FIDO (Quality Owner)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
squad obo dina — PR #523 is fully team-reviewed and approved.
Fixes #521 (worktree bug reported by Yoni Ben-Ami). 3 commits, 3 fix cycles, 5 reviewers. Ready for your review @bradygaster. |
…ygaster#521) Adds test/worktree.test.ts — 7 tests covering the two root causes identified in Flight's worktree investigation: resolveSquad() (packages/squad-sdk/src/resolution.ts) - Stops at a .git FILE as if it were a repo root boundary - Should fall back to \git worktree list --porcelain\ to find the main checkout, then return its .squad/ detectSquadDir() (packages/squad-cli/src/cli/core/detect-squad-dir.ts) - Returns worktree/.squad (non-existent) when called from a worktree - Should use the same worktree fallback to find the main checkout 4 tests FAIL on current code (proving the bugs). 3 tests PASS as positive controls. All tests will PASS once the fixes described in the investigation doc are applied. Also exposes detect-squad-dir via the CLI package exports so it can be imported by tests without reaching into dist/ directly. Requested by: Dina Investigation by: Flight (Keaton) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
resolveSquad() and findSquadDir() in packages/squad-sdk/src/resolution.ts now distinguish .git as a directory (real repo root) from .git as a file (git worktree pointer). When .git is a file, the gitdir: pointer is parsed to derive the main checkout path, and .squad/ is checked there as a fallback (main-checkout strategy). Worktree-local .squad/ continues to take priority. detectSquadDir() in packages/squad-cli/src/cli/core/detect-squad-dir.ts gains the same worktree fallback via a new exported helper resolveWorktreeMainCheckout(). All 8 CLI commands that call detectSquadDir now transparently find the main checkout's .squad/ when running from a worktree root. squad init gains an explicit worktree guard: when running from a worktree that already has .squad/ in the main checkout, the user is prompted to choose between shared strategy (use main checkout, no files created) and worktree-local strategy (scaffold .squad/ in the worktree). Non-interactive sessions default to shared to prevent silent duplicate creation. Three new tests in test/resolution.test.ts cover the main-checkout fallback, worktree-local priority, and the full walk-up scenario. Closes bradygaster#521 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…, add statSync guard
- Remove vi.mock('child_process') and all helpers — implementation reads
.git via fs.readFileSync, never calls child_process; the mock was dead
code that gave false confidence
- Fix gitdir in all worktree test fixtures:
'../../.git/worktrees/feature-521' resolved above the temp dir;
corrected to '../main/.git/worktrees/feature-521' which resolves to
tmp/main/.git/worktrees/feature-521 as expected
- Create main/.git/worktrees/feature-521/ directory in each fixture so
the real directory structure matches the gitdir pointer
- Add statSync guard to getMainWorktreePath() (resolution.ts) and
resolveWorktreeMainCheckout() (detect-squad-dir.ts): verify that
mainCheckout/.git exists and is a directory before trusting it; guards
against crafted .git files that redirect Squad to attacker-controlled paths
- Add two statSync guard tests: resolveSquad() and detectSquadDir() both
return null/fallback (not crash) when gitdir points to a non-existent path
All 9 worktree regression tests pass.
Fixes PR bradygaster#523 blockers as requested by Dina.
Working as FIDO (Quality Owner)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
diberry
force-pushed
the
squad/521-worktree-tests
branch
from
ebc0efc to
961cbba
Compare
chrislomonico
pushed a commit
to clomonico/squad
that referenced
this pull request
Mar 26, 2026
…r#546) Consolidated 15 remaining issues into a single PR. Redesigns help UX with grouped categories, surfaces all /help commands, aligns docs with actual CLI behavior, adds first-run welcome, standardizes naming. Closes bradygaster#510, Closes bradygaster#513, Closes bradygaster#514, Closes bradygaster#515, Closes bradygaster#516, Closes bradygaster#517, Closes bradygaster#518, Closes bradygaster#521, Closes bradygaster#522, Closes bradygaster#523, Closes bradygaster#524, Closes bradygaster#525, Closes bradygaster#527, Closes bradygaster#528, Closes bradygaster#529 Co-authored-by: Copilot <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.
Summary
Closes #521
Working as EECOM (Core Dev)
Adds \ est/worktree.test.ts\ — 7 regression tests covering the root causes identified in Flight's worktree investigation.
What this does
4 tests that FAIL on current code (proving the bugs exist):
esolveSquad()\ treats a .git\ FILE the same as a directory — returns
ull\ instead of falling back to the main checkout's .squad/\
3 tests that PASS as positive controls:
esolveSquad()\ returns null when neither location has .squad/\
Files changed
CI
The test file matches the \ est/**/*.test.ts\ include pattern in \�itest.config.ts\ and runs with
pm test\ — no special setup required.
Once the SDK and CLI fixes from the investigation land, all 4 failing tests should flip green.
Investigation by: Flight (Keaton) · Requested by: Dina