fix: prevent worktree isolation bypass via prompt and git-level adoption

[Wirasm]

Summary

Fixes two compounding bugs that cause workflow runs to silently operate on the wrong branch:

• Prompt-level (bug(implement): archon-implement reuses pre-existing branch from unrelated task, implementing wrong fix #1193): archon-implement command's branch decision table is ambiguous — AI can adopt unrelated pre-existing branches instead of using its assigned worktree
• Code-level (Git-level worktree adoption bypasses cross-checkout guard and allows parallel collision #1188): WorktreeProvider.findExisting() adopts any worktree at the computed path regardless of which clone created it, and createNewBranch() silently inherits stale branch commits

Changes

File	Change
.archon/commands/defaults/archon-implement.md	Replace flat branch table with decision tree; add "never switch branches" guard; use $BASE_BRANCH explicitly
packages/isolation/src/providers/worktree.ts	findExisting(): verify worktree parent repo matches request before adopting
packages/isolation/src/providers/worktree.ts	createNewBranch(): git branch -f stale orphan branches to intended start-point before checkout
packages/isolation/src/providers/worktree.test.ts	Tests for cross-checkout rejection, repo-root matching, backward compat, and stale branch reset

Root Cause

Prompt: The archon-implement command's Phase 2.2 used a flat table with "On feature branch → Use it" which the AI could interpret as "use any existing feature branch." The other implement commands (archon-fix-issue, archon-implement-issue) already use a proper decision tree with IN WORKTREE? as the first check.

Code: findExisting() checks only worktreeExists(path) — two clones of the same remote resolve to the same worktree base directory, so clone B adopts clone A's worktree. createNewBranch() falls back to git worktree add path branch (without -b) when the branch exists, inheriting whatever stale commits were on it.

Testing

• Type check passes
• All 123 isolation tests pass (3 new tests added)
• Lint passes
• Full test suite passes

Fixes #1193
Supersedes #1186 (thanks to @halindrome for the initial framing and scoping — their PR identified the root cause class and explicitly flagged this provider-layer gap as a known limitation, which directly shaped #1198's approach).
Relates to #1188

Summary by CodeRabbit

• Documentation

  • Reworked Git-state decision flow: clearer checks, show current branch and repo root, forbid switching/creating branches inside worktrees, and tightened STOP messages for uncommitted work.

• Bug Fixes

  • Refused adoption of worktrees that belong to a different repository; added branch reset-before-reuse when a branch already exists; surfaced clearer isolation/worktree error messages.

• Tests

  • Broadened tests for worktree adoption, ownership verification, branch-reset and error propagation scenarios.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Added strict worktree ownership verification and refined branch-reuse logic in the isolation provider: existing worktrees are now verified by reading and resolving their .git pointer before adoption; mismatches or unreadable pointers abort adoption. Existing branches are forcibly reset to the requested start point before reuse. Docs updated to tighten git-state decision flow.

Changes

Cohort / File(s)	Summary
Worktree provider implementation packages/isolation/src/providers/worktree.ts	Added verifyWorktreeOwnership() that reads worktree/.git, resolves the referenced repo root, compares to request.canonicalRepoPath, and throws/refuses adoption on mismatch; adjusted findExisting() to verify ownership before adopting; createNewBranch() now resets existing branches with git branch -f when necessary; added readFile/resolve imports.
Worktree provider tests packages/isolation/src/providers/worktree.test.ts	Expanded mocks for fs/promises (readFile, rm); added and updated tests covering .git pointer validation (cross-checkout, unreadable .git, directory .git, non-worktree pointers), successful adoption with normalized paths, branch-reset-on-exist behavior, and error propagation when reset fails.
Error classification packages/isolation/src/errors.ts	Extended classifyIsolationError and isKnownIsolationError to recognize and surface new worktree-related failure patterns (different clone, cannot verify ownership, cannot adopt).
Documentation / CLI guidance .archon/commands/defaults/archon-implement.md, .archon/commands/defaults/archon-fix-issue.md, .archon/commands/defaults/archon-implement-issue.md, .archon/commands/defaults/archon-plan-setup.md	Replaced table-based branch-decision logic with an ordered decision-tree; added explicit detection commands (branch, worktree root) and directives forbidding branch switching/creation inside worktrees; generalized references to $BASE_BRANCH and tightened STOP messages for dirty state.

Sequence Diagram(s)

sequenceDiagram
  participant Req as Request (Isolation)
  participant Provider as WorktreeProvider
  participant FS as Filesystem
  participant Git as Git CLI
  participant Log as Logger

  Req->>Provider: findExisting(request)
  Provider->>FS: readFile(worktreePath + "/.git")
  alt read succeeds & contains "gitdir: <path>"
    Provider->>Provider: resolve referenced repo root
    Provider->>Req: compare resolvedRoot == request.canonicalRepoPath?
    alt match
      Provider->>Log: worktree_adopted
      Provider->>Git: (no branch switch) use existing branch / proceed
    else mismatch
      Provider->>Log: worktree_adoption_refused_cross_checkout
      Provider->>Req: throw ownership verification error
    end
  else read fails / invalid format
    Provider->>Log: worktree_adoption_refused_invalid_dotgit
    Provider->>Req: throw verification error
  end

  Note over Provider,Git: When creating/using branch
  Provider->>Git: git branch -f <branch> <startPoint>  (if branch exists)
  Provider->>Git: git worktree add ...

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• #1193 — Addresses the symptom where runs reused pre-existing branches from other tasks by verifying worktree ownership and resetting branches before reuse.
• Worktree reuse matches environments from different local clones of the same remote #1183 — New .git-based ownership check prevents adoption of worktrees belonging to different clones (matches this issue).
• Git-level worktree adoption bypasses cross-checkout guard and allows parallel collision #1188 — The verify-and-refuse adoption behavior and new error patterns directly address the cross-checkout adoption gap described.

Poem

🐰 In tangled trees where branches roam,
I sniff the .git that calls a home.
If roots don't match, I hop away —
New branches sprout to save the day.
Hooray for clean worktrees! 🌱✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: preventing worktree adoption across clones via verification checks and prompt clarification.
Description check	✅ Passed	The description covers summary bullets, root cause analysis, changes table, testing validation, and linked issue references according to the template structure.
Linked Issues check	✅ Passed	Code changes implement all core objectives from #1193: decision tree prevents branch confusion, worktree ownership verification blocks cross-checkout, and branch reset prevents stale commits.
Out of Scope Changes check	✅ Passed	All changes directly address the linked issues: prompt clarification, worktree ownership verification, branch reset logic, error classification, and corresponding test coverage.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/worktree-isolation-1193-1188

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

packages/isolation/src/providers/worktree.ts (1)

527-540: Windows path separator may not match.

The regex /gitdir: (.+)\/\.git\/worktrees\// expects forward slashes. On Windows, the .git file content typically uses forward slashes even on Windows, but git's behavior can vary by version/configuration. If a Windows system writes backslashes, the regex won't match.

Consider normalizing the path or using a more flexible regex:

-      const match = /gitdir: (.+)\/\.git\/worktrees\//.exec(gitContent);
+      const match = /gitdir: (.+)[/\\]\.git[/\\]worktrees[/\\]/.exec(gitContent);

Also, consider adding an explicit return type annotation for getWorktreeSourceRepo per the strict TypeScript coding guideline.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/isolation/src/providers/worktree.ts` around lines 527 - 540, The
getWorktreeSourceRepo function's regex (/gitdir: (.+)\/\.git\/worktrees\//)
assumes forward slashes and can fail on Windows; update getWorktreeSourceRepo to
be tolerant of either separator by normalizing gitContent path separators or
using a flexible regex that accepts both forward and backslashes (e.g., match
both '/' and '\\') when extracting the repo root from the "gitdir:" line, and
keep the existing try/catch behavior returning null on failure; reference the
getWorktreeSourceRepo function and the readFile call to locate the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.archon/commands/defaults/archon-implement.md:
- Around line 111-127: The doc contains a contradiction: the directive "Never
switch branches or create new branches" conflicts with the decision-tree path
that runs "git checkout -b feature/{plan-slug}", and the fenced diagram is
missing a markdown language specifier; update the decision tree to match the
directive by removing or disabling the branch-creation path (the node that
invokes "git checkout -b feature/{plan-slug}" and the branch-creation branch),
or alternatively revise the directive to clearly allow branch creation only in
non-worktree contexts (explicitly mention the exception), and add a language tag
(e.g., change the opening fence to ```text) to the fenced diagram to satisfy
markdownlint MD040.

In `@packages/isolation/src/providers/worktree.test.ts`:
- Around line 37-43: Add clearing for the additional mocks to prevent test
pollution: in the afterEach block where mockAccess.mockClear() is called, also
call mockReadFile.mockClear() and mockRm.mockClear() so the mock state for
readFile (mockReadFile) and rm (mockRm) is reset between tests; locate the
afterEach in worktree.test.ts and add these two mockClear() calls beside
mockAccess.mockClear().

In `@packages/isolation/src/providers/worktree.ts`:
- Around line 487-503: Normalize both repository paths before comparing: when
you call getWorktreeSourceRepo(worktreePath) and compare its result
(existingRepo) to request.canonicalRepoPath, run both through path.resolve (or
path.normalize) to remove trailing slashes and normalize separators (and require
importing Node's path). Update the comparison to use normalizedExistingRepo and
normalizedRequestRepo so worktree_adoption_skipped_cross_checkout is triggered
reliably across platforms; keep the same logging fields (worktreePath,
branchName, existingRepo, requestRepo) but consider logging the normalized
values if helpful.

---

Nitpick comments:
In `@packages/isolation/src/providers/worktree.ts`:
- Around line 527-540: The getWorktreeSourceRepo function's regex (/gitdir:
(.+)\/\.git\/worktrees\//) assumes forward slashes and can fail on Windows;
update getWorktreeSourceRepo to be tolerant of either separator by normalizing
gitContent path separators or using a flexible regex that accepts both forward
and backslashes (e.g., match both '/' and '\\') when extracting the repo root
from the "gitdir:" line, and keep the existing try/catch behavior returning null
on failure; reference the getWorktreeSourceRepo function and the readFile call
to locate the change.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8fb26aaf-e91c-4821-af27-c04de20f2cd3

📥 Commits

Reviewing files that changed from the base of the PR and between d6e24f5 and f4a429d.

📒 Files selected for processing (3)

• .archon/commands/defaults/archon-implement.md
• packages/isolation/src/providers/worktree.test.ts
• packages/isolation/src/providers/worktree.ts

[Wirasm]

PR Review Summary

Ran 4 specialized reviewers: code-reviewer, docs-impact, pr-test-analyzer, silent-failure-hunter.

Critical Issues (2)

Agent	Issue	Location
code-reviewer	Cross-checkout detection returns null instead of throwing. Falls through to createNewBranch, where git fails with "'<path>' already exists". The catch at line 936 uses err.stderr?.includes('already exists') — the same substring appears in path-exists and branch-exists errors, so the code enters the stale-branch-reset path, runs git branch -f, then retries worktree add at the conflicting path, which fails again with a confusing cascade.	packages/isolation/src/providers/worktree.ts:492-502 + :936
silent-failure-hunter	getWorktreeSourceRepo bare catch {} swallows all filesystem errors. Only ENOENT is an expected "not a worktree" signal — EACCES, EIO, etc. should not collapse to null. Returning null takes the permissive adoption branch, which defeats the cross-checkout protection this PR introduces (a permission error on .git would let clone B silently adopt clone A's worktree — the exact bug being fixed).	packages/isolation/src/providers/worktree.ts:531-540

Important Issues (2)

Agent	Issue	Location
pr-test-analyzer	git branch -f failure path is untested (rating 8/10). If reset fails (protected branch, detached HEAD, policy block), the raw error propagates without classification via classifyIsolationError(). Add a test that mocks branch -f rejection and asserts clean failure without a third worktree add call.	packages/isolation/src/providers/worktree.test.ts
docs-impact	archon-fix-issue.md and archon-implement-issue.md have identical Phase 3 decision trees that predate the new rule. They lack the "Never switch branches or create new branches in a worktree" guard, and they hardcode MAIN/MASTER instead of using $BASE_BRANCH. Apply the same decision-tree pattern used in archon-implement.md.	.archon/commands/defaults/archon-fix-issue.md, .archon/commands/defaults/archon-implement-issue.md

Suggestions

• Add a comment in getWorktreeSourceRepo noting the submodule false-negative trade-off — a plain .git file (non-worktree pointer) returns null and allows adoption. Intentional backward-compat, but worth documenting.

Strengths

• createNewBranch's double-execFileAsync sequence (branch -f then worktree add) has no silent-failure risk — both reject on non-zero exit, so a failed reset short-circuits cleanly before the second call.
• The fromBranch + existing-branch collision now throws an explicit, actionable error instead of silently ignoring the --from flag.
• Backward-compat test (readFile throws ENOENT → adopt anyway) correctly preserves single-clone behavior.
• Bun mock.module + spyOn pattern follows the documented isolation rules.

Verdict: NEEDS FIXES

The stale-branch-reset logic and the .git-file verification are each correct in isolation, but their interaction (via the null return + permissive catch) defeats the core invariant the PR is meant to establish. Both critical issues should be fixed before merge.

Recommended Actions

1. Throw IsolationBlockedError (or a plain Error) directly from findExisting on cross-checkout detection, instead of returning null. Update the cross-checkout test to .rejects.toThrow(...).
2. Narrow getWorktreeSourceRepo's catch to ENOENT only. Log (or rethrow) other errors rather than silently returning null.
3. Add the git branch -f failure test.
4. Sync archon-fix-issue.md and archon-implement-issue.md with archon-implement.md's new decision tree and $BASE_BRANCH variable.

[Wirasm]

Review Response

Addressed all critical + important findings from CodeRabbit and self-review.

Critical fixes

1. Cross-checkout detection now throws (worktree.ts:487-493)

Previously findExisting() returned null on cross-checkout, which fell through to createNewBranch() and produced a confusing "'path' already exists" cascade that got reclassified as "branch already exists". Now it throws directly with a classified user message:

Error: A worktree at the target path was created by a different local clone.
Remove it from that clone, or register this codebase from the same local path.

2. Narrowed catch in ownership check (worktree.ts:531-580)

Replaced getWorktreeSourceRepo (bare catch returning null) with verifyWorktreeOwnership that handles each .git error precisely:

• EISDIR → "path contains a full git checkout, not a worktree"
• ENOENT / EACCES / EIO → "Cannot verify worktree ownership"
• Unmatched gitdir regex (submodule, malformed) → "not a git-worktree reference"
• Mismatched repo → "belongs to a different clone"

No permissive fallback — the bug this PR fixes was exactly "default to adoption when unsure," so defaulting that way on error recreates the vulnerability.

Important fixes

3. git branch -f failure test added (worktree.test.ts)

Test propagates error if branch -f reset fails (protected branch, etc.) asserts that a failed reset surfaces cleanly without a third worktree add retry.

4. Aligned sibling command prompts

Swept all 36 default commands. Found 3 more with the same ambiguous decision tree and fixed them:

• archon-fix-issue.md: was "ON FEATURE/FIX BRANCH → Use it" + hardcoded MAIN/MASTER
• archon-implement-issue.md: identical to above
• archon-plan-setup.md: worst case — "In a worktree" was the last row in a table; "Already on correct feature branch" came first. Converted to ordered decision tree with IN WORKTREE? first.

All three now match archon-implement.md with the explicit "Do NOT switch branches. Do NOT create new branches." guard and use $BASE_BRANCH instead of hardcoded values.

Other 33 commands are clean: legitimate PR-branch checkouts (implement-review-fixes, auto-fix-review, etc.), user bootstrap commands, error-message documentation, or worktree-path artifacts.

Also fixed (minor)

• mockReadFile / mockRm now .mockClear() in afterEach
• Path comparison uses path.resolve() for cross-platform normalization (trailing slashes, relative components)
• Prompt contradiction resolved: "Never switch branches" is now scoped to the IN WORKTREE case; non-worktree branch creation still allowed for manual CLI usage

Validation

• Type check: pass
• Lint: pass (0 warnings)
• Tests: 126 pass in packages/isolation/src/providers/worktree.test.ts (6 new, 3 updated)
• Full test suite: all green

Deliberately not addressed

• Symlink resolution in path comparison: documented as a known limitation in the verifyWorktreeOwnership comment. Users should register codebases with consistent path forms. realpath-based resolution is a larger change that belongs in a separate PR if needed.

[Wirasm]

Credit: @halindrome's #1186 identified the root cause class and explicitly flagged this provider-layer gap as a known limitation, which directly shaped this PR's approach. Thanks for the groundwork!