fix: extend worktree ownership guard to resolver adoption paths#1206
Merged
fix: extend worktree ownership guard to resolver adoption paths#1206
Conversation
#1188) PR #1198 guarded WorktreeProvider.findExisting(), but IsolationResolver has three earlier adoption paths that bypass the provider layer: - findReusable (DB lookup by workflow identity) - findLinkedIssueEnv (cross-reference via linked issues) - tryBranchAdoption (PR branch discovery) Two clones of the same remote share codebase_id (identity is derived from owner/repo). Without these guards, clone B silently adopts clone A's worktree via any of the three paths. Changes: - Extract verifyWorktreeOwnership from WorktreeProvider (private) to @archon/git/src/worktree.ts as an exported function, sitting next to getCanonicalRepoPath which parses the same .git file format - Call the shared function from all three resolver paths; throw on cross-clone mismatch (DB rows are preserved — they legitimately belong to the other clone) - Compute canonicalRepoPath once at the top of resolve() - Six new tests in resolver.test.ts covering each guarded path's cross-checkout and same-clone behaviors Fixes #1183 Fixes #1188 (part 1 — cross-checkout; part 2 parallel collision deferred to follow-up alongside #1036)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracted a shared Changes
Sequence Diagram(s)sequenceDiagram
participant Resolver
participant Provider
participant GitPkg
participant FS
Resolver->>Provider: resolve/adopt worktree request (canonicalRepoPath)
Provider->>GitPkg: verifyWorktreeOwnership(worktreePath, canonicalRepoPath)
GitPkg->>FS: read <worktreePath>/.git
FS-->>GitPkg: .git contents (gitdir: .../.git/worktrees/...)
GitPkg->>GitPkg: resolve paths & compare normalized repo path
alt match
GitPkg-->>Provider: ok
Provider-->>Resolver: proceed with adoption/return
else mismatch or error
GitPkg-->>Provider: throw error (belongs to different clone / EISDIR / ENOENT)
Provider-->>Resolver: log warning and rethrow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Collaborator
Author
PR Review Summary (multi-agent)Reviewed by: Verdict: NEEDS MINOR FIXESCore correctness is solid — Important Issues
Suggestions
Documentation
Strengths
Recommended Actions
|
Addresses the multi-agent review on #1206: Code fixes: - worktree.adoption_refused_cross_checkout log event renamed to match CLAUDE.md {domain}.{action}_{state} convention - verifyWorktreeOwnership now preserves err.code and err via { cause } when wrapping fs errors, so classifyIsolationError is robust to Node message format changes - Structured fields (codebaseId, canonicalRepoPath) added to all cross-clone rejection logs for incident debugging - Wrap getCanonicalRepoPath at top of resolve() with classified error instead of letting it propagate as an unclassified crash - Extract assertWorktreeOwnership helper on IsolationResolver — centralizes warn-then-rethrow contract, removes duplication - Dedupe toWorktreePath(existing.working_path) calls in resolver paths - Add code comment on findLinkedIssueEnv explaining why throw-on-first is intentional (user decision — surfaces anomaly instead of masking) Secondary gap closed: - WorktreeProvider.findExisting PR-branch adoption path (findWorktreeByBranch) now also verifies ownership — same class of bug as the main path, just via a different lookup Tests: - 8 new unit tests for verifyWorktreeOwnership in @archon/git (matching pointer, different clone, EISDIR/ENOENT errno preservation, submodule pointer, corrupted .git, trailing-slash normalization, cause chain) - tryBranchAdoption cross-clone test now asserts store.create was never called (symmetry with paths 1+2 asserting updateStatus) - New test for cross-clone rejection in the PR-branch-adoption secondary path in worktree.test.ts Docs: - CHANGELOG.md Unreleased entry for the cross-clone fix series - troubleshooting.md "Worktree Belongs to a Different Clone" section documenting all four new error patterns with resolution steps and pointer to #1192 for the architectural fix
Collaborator
Author
Review ResponseAddressed all findings from the multi-agent review. New commit: `c8f2e0c`. Important Issues — fixed
Suggestions — addressed
Unit tests for `verifyWorktreeOwnership` — added8 new tests in `packages/git/src/git.test.ts` covering the actual `.git` file parsing:
Secondary gap — fixed in this PR (not follow-up)The `findWorktreeByBranch` PR-branch-adoption path inside `WorktreeProvider.findExisting` also lacked ownership verification. Same bug class, different lookup. Added the same guard + a new test in `worktree.test.ts`. No follow-up issue needed. Documentation — added
Validation
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/isolation/src/resolver.ts (1)
248-260: Log the full ownership error, not justerr.message.
verifyWorktreeOwnership()now preservescodeandcause, but this helper drops that signal by flattening the error to a string. Logging the error object directly, or at least addingcode, would make the new cross-checkout warnings much more useful for incident triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/isolation/src/resolver.ts` around lines 248 - 260, The catch in assertWorktreeOwnership is currently logging only err.message which drops important fields like code and cause from verifyWorktreeOwnership; update the getLog().warn call in assertWorktreeOwnership to log the full error object (or at least include (err as any).code and (err as any).cause) instead of only err.message so cross-checkout warnings retain the original error metadata from verifyWorktreeOwnership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/isolation/src/resolver.ts`:
- Around line 109-131: The canonical-path error is currently rethrown from the
getCanonicalRepoPath block which bypasses resolve()'s existing classification in
createNewEnvironment(); instead of throwing a raw Error, convert this failure
into the resolver's classified result (map to the same "blocked" outcome
createNewEnvironment uses). Replace the throw with a return of the appropriate
IsolationResult (or construct and throw the resolver's domain-specific error
type) carrying status "blocked" (or the creation_failed payload if more
appropriate), include codebase.id, defaultCwd and the original err as cause, and
ensure this path is handled by resolve() the same way createNewEnvironment()
failures are handled so permission/malformed-repo errors become actionable
isolation results; key symbols: getCanonicalRepoPath, canonicalPath, resolve(),
createNewEnvironment(), and the creation_failed/blocked classification.
---
Nitpick comments:
In `@packages/isolation/src/resolver.ts`:
- Around line 248-260: The catch in assertWorktreeOwnership is currently logging
only err.message which drops important fields like code and cause from
verifyWorktreeOwnership; update the getLog().warn call in
assertWorktreeOwnership to log the full error object (or at least include (err
as any).code and (err as any).cause) instead of only err.message so
cross-checkout warnings retain the original error metadata from
verifyWorktreeOwnership.
🪄 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: 0ddf2511-63f2-466c-b890-008c530dcee7
📒 Files selected for processing (8)
CHANGELOG.mdpackages/docs-web/src/content/docs/reference/troubleshooting.mdpackages/git/src/git.test.tspackages/git/src/worktree.tspackages/isolation/src/providers/worktree.test.tspackages/isolation/src/providers/worktree.tspackages/isolation/src/resolver.test.tspackages/isolation/src/resolver.ts
✅ Files skipped from review due to trivial changes (2)
- packages/docs-web/src/content/docs/reference/troubleshooting.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/isolation/src/providers/worktree.ts
- packages/git/src/worktree.ts
Comment on lines
+109
to
+131
| // Compute canonical repo path once — paths 3-6 all need it either for | ||
| // ownership verification (cross-clone guard) or for worktree creation. | ||
| // Wrap failures so they classify as known isolation errors with actionable | ||
| // messages instead of propagating as unclassified crashes. | ||
| let canonicalPath: RepoPath; | ||
| try { | ||
| canonicalPath = await getCanonicalRepoPath(codebase.defaultCwd); | ||
| } catch (error) { | ||
| const err = error as Error; | ||
| getLog().error( | ||
| { | ||
| err, | ||
| errorType: err.constructor.name, | ||
| codebaseId: codebase.id, | ||
| defaultCwd: codebase.defaultCwd, | ||
| }, | ||
| 'isolation.canonical_repo_path_resolution_failed' | ||
| ); | ||
| throw new Error( | ||
| `Cannot determine canonical repo path for ${codebase.defaultCwd}: ${err.message}`, | ||
| { cause: err } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Canonical-path failures still escape resolve() as thrown exceptions.
This branch rethrows before the resolver reaches its existing creation_failed/blocked handling in createNewEnvironment(), so permission or malformed-repo failures here will still surface as unclassified crashes instead of an actionable isolation result. Please route this path through the same classification flow, or map it to blocked directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/isolation/src/resolver.ts` around lines 109 - 131, The
canonical-path error is currently rethrown from the getCanonicalRepoPath block
which bypasses resolve()'s existing classification in createNewEnvironment();
instead of throwing a raw Error, convert this failure into the resolver's
classified result (map to the same "blocked" outcome createNewEnvironment uses).
Replace the throw with a return of the appropriate IsolationResult (or construct
and throw the resolver's domain-specific error type) carrying status "blocked"
(or the creation_failed payload if more appropriate), include codebase.id,
defaultCwd and the original err as cause, and ensure this path is handled by
resolve() the same way createNewEnvironment() failures are handled so
permission/malformed-repo errors become actionable isolation results; key
symbols: getCanonicalRepoPath, canonicalPath, resolve(), createNewEnvironment(),
and the creation_failed/blocked classification.
verifyWorktreeOwnership previously called path.resolve() on the gitdir path before embedding it in the error message. On Windows, resolve() prepends a drive letter to a POSIX-style path (e.g., /other/clone → C:\other\clone), which: 1. Misled users by showing a path that doesn't match what's actually in their .git file 2. Broke a Windows-only test asserting the error contains the literal /other/clone path Compare on resolved paths (correct — normalizes trailing slashes and relative components for the equality check) but display the raw match in the error message (recognizable to the user).
This was referenced Apr 14, 2026
Wirasm
added a commit
that referenced
this pull request
Apr 14, 2026
Follow-up to #1206 review: the early getCanonicalRepoPath() wrap in resolve() threw directly, escaping the classification flow that createNewEnvironment uses. Permission errors, malformed worktree pointers, ENOENT, etc. surfaced as unclassified crashes instead of becoming an actionable `blocked` result. Mirror createNewEnvironment's contract: - isKnownIsolationError → return { status: 'blocked', reason: 'creation_failed', userMessage: classifyIsolationError(err) + suffix } - unknown errors → throw (programming bugs stay visible as crashes, not silent isolation failures) Adds two tests in resolver.test.ts: - EACCES classifies to "Permission denied" blocked message - Unknown error propagates as throw Addresses CodeRabbit review comment on #1206.
4 tasks
Wirasm
added a commit
that referenced
this pull request
Apr 14, 2026
) Follow-up to #1206 review: the early getCanonicalRepoPath() wrap in resolve() threw directly, escaping the classification flow that createNewEnvironment uses. Permission errors, malformed worktree pointers, ENOENT, etc. surfaced as unclassified crashes instead of becoming an actionable `blocked` result. Mirror createNewEnvironment's contract: - isKnownIsolationError → return { status: 'blocked', reason: 'creation_failed', userMessage: classifyIsolationError(err) + suffix } - unknown errors → throw (programming bugs stay visible as crashes, not silent isolation failures) Adds two tests in resolver.test.ts: - EACCES classifies to "Permission denied" blocked message - Unknown error propagates as throw Addresses CodeRabbit review comment on #1206.
Closed
4 tasks
Wirasm
pushed a commit
that referenced
this pull request
Apr 15, 2026
Worktrees created via `git worktree add` do not initialize submodules — monorepo workflows that need submodule content find empty directories. Auto-detect `.gitmodules` and run `git submodule update --init --recursive` after worktree creation; classify failures through the isolation error pipeline. Behavior: - `.gitmodules` absent → skip silently (zero-cost probe, no effect on non-submodule repos) - `.gitmodules` present → run submodule init by default (opt out via `worktree.initSubmodules: false`) - submodule init or `.gitmodules` read failure → throw with classified error including opt-out guidance - Only `ENOENT` on `.gitmodules` is treated as "no submodules"; other access errors (EACCES, EIO) surface as failures to prevent silent empty-dir worktrees Changes: - `packages/isolation/src/providers/worktree.ts` — `initSubmodules()` method + call site in `createWorktree()` - `packages/isolation/src/errors.ts` — collapsed `errorPatterns` + `knownPatterns` into single `ERROR_PATTERNS` source of truth with `known: boolean` per entry; added submodule pattern with opt-out guidance - `packages/isolation/src/types.ts` + `packages/core/src/config/config-types.ts` — new `initSubmodules?: boolean` config option - `packages/docs-web/src/content/docs/reference/configuration.md` — documented the new option and submodule behavior - Tests: default-on, explicit opt-in, explicit opt-out, skip-when-absent, fail-fast on EACCES, fail-fast on git failure, fail-fast on timeout Credit to @halindrome for the original implementation and root-cause mapping across #1183, #1187, #1188, #1192. Follow-up: #1192 (codebase identity rearchitect) would retire the cross-clone guard code in `resolver.ts` and `worktree.ts` that #1198, #1206 added. Separate PR. Closes #1187
7 tasks
Merged
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
…am00#1206) * fix: extend worktree ownership guard to resolver adoption paths (coleam00#1183, coleam00#1188) PR coleam00#1198 guarded WorktreeProvider.findExisting(), but IsolationResolver has three earlier adoption paths that bypass the provider layer: - findReusable (DB lookup by workflow identity) - findLinkedIssueEnv (cross-reference via linked issues) - tryBranchAdoption (PR branch discovery) Two clones of the same remote share codebase_id (identity is derived from owner/repo). Without these guards, clone B silently adopts clone A's worktree via any of the three paths. Changes: - Extract verifyWorktreeOwnership from WorktreeProvider (private) to @archon/git/src/worktree.ts as an exported function, sitting next to getCanonicalRepoPath which parses the same .git file format - Call the shared function from all three resolver paths; throw on cross-clone mismatch (DB rows are preserved — they legitimately belong to the other clone) - Compute canonicalRepoPath once at the top of resolve() - Six new tests in resolver.test.ts covering each guarded path's cross-checkout and same-clone behaviors Fixes coleam00#1183 Fixes coleam00#1188 (part 1 — cross-checkout; part 2 parallel collision deferred to follow-up alongside coleam00#1036) * fix: address PR review — polish, observability, secondary gap, docs Addresses the multi-agent review on coleam00#1206: Code fixes: - worktree.adoption_refused_cross_checkout log event renamed to match CLAUDE.md {domain}.{action}_{state} convention - verifyWorktreeOwnership now preserves err.code and err via { cause } when wrapping fs errors, so classifyIsolationError is robust to Node message format changes - Structured fields (codebaseId, canonicalRepoPath) added to all cross-clone rejection logs for incident debugging - Wrap getCanonicalRepoPath at top of resolve() with classified error instead of letting it propagate as an unclassified crash - Extract assertWorktreeOwnership helper on IsolationResolver — centralizes warn-then-rethrow contract, removes duplication - Dedupe toWorktreePath(existing.working_path) calls in resolver paths - Add code comment on findLinkedIssueEnv explaining why throw-on-first is intentional (user decision — surfaces anomaly instead of masking) Secondary gap closed: - WorktreeProvider.findExisting PR-branch adoption path (findWorktreeByBranch) now also verifies ownership — same class of bug as the main path, just via a different lookup Tests: - 8 new unit tests for verifyWorktreeOwnership in @archon/git (matching pointer, different clone, EISDIR/ENOENT errno preservation, submodule pointer, corrupted .git, trailing-slash normalization, cause chain) - tryBranchAdoption cross-clone test now asserts store.create was never called (symmetry with paths 1+2 asserting updateStatus) - New test for cross-clone rejection in the PR-branch-adoption secondary path in worktree.test.ts Docs: - CHANGELOG.md Unreleased entry for the cross-clone fix series - troubleshooting.md "Worktree Belongs to a Different Clone" section documenting all four new error patterns with resolution steps and pointer to coleam00#1192 for the architectural fix * fix(git): use raw .git pointer in cross-clone error message verifyWorktreeOwnership previously called path.resolve() on the gitdir path before embedding it in the error message. On Windows, resolve() prepends a drive letter to a POSIX-style path (e.g., /other/clone → C:\other\clone), which: 1. Misled users by showing a path that doesn't match what's actually in their .git file 2. Broke a Windows-only test asserting the error contains the literal /other/clone path Compare on resolved paths (correct — normalizes trailing slashes and relative components for the equality check) but display the raw match in the error message (recognizable to the user).
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
…leam00#1211) Follow-up to coleam00#1206 review: the early getCanonicalRepoPath() wrap in resolve() threw directly, escaping the classification flow that createNewEnvironment uses. Permission errors, malformed worktree pointers, ENOENT, etc. surfaced as unclassified crashes instead of becoming an actionable `blocked` result. Mirror createNewEnvironment's contract: - isKnownIsolationError → return { status: 'blocked', reason: 'creation_failed', userMessage: classifyIsolationError(err) + suffix } - unknown errors → throw (programming bugs stay visible as crashes, not silent isolation failures) Adds two tests in resolver.test.ts: - EACCES classifies to "Permission denied" blocked message - Unknown error propagates as throw Addresses CodeRabbit review comment on coleam00#1206.
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
Worktrees created via `git worktree add` do not initialize submodules — monorepo workflows that need submodule content find empty directories. Auto-detect `.gitmodules` and run `git submodule update --init --recursive` after worktree creation; classify failures through the isolation error pipeline. Behavior: - `.gitmodules` absent → skip silently (zero-cost probe, no effect on non-submodule repos) - `.gitmodules` present → run submodule init by default (opt out via `worktree.initSubmodules: false`) - submodule init or `.gitmodules` read failure → throw with classified error including opt-out guidance - Only `ENOENT` on `.gitmodules` is treated as "no submodules"; other access errors (EACCES, EIO) surface as failures to prevent silent empty-dir worktrees Changes: - `packages/isolation/src/providers/worktree.ts` — `initSubmodules()` method + call site in `createWorktree()` - `packages/isolation/src/errors.ts` — collapsed `errorPatterns` + `knownPatterns` into single `ERROR_PATTERNS` source of truth with `known: boolean` per entry; added submodule pattern with opt-out guidance - `packages/isolation/src/types.ts` + `packages/core/src/config/config-types.ts` — new `initSubmodules?: boolean` config option - `packages/docs-web/src/content/docs/reference/configuration.md` — documented the new option and submodule behavior - Tests: default-on, explicit opt-in, explicit opt-out, skip-when-absent, fail-fast on EACCES, fail-fast on git failure, fail-fast on timeout Credit to @halindrome for the original implementation and root-cause mapping across coleam00#1183, coleam00#1187, coleam00#1188, coleam00#1192. Follow-up: coleam00#1192 (codebase identity rearchitect) would retire the cross-clone guard code in `resolver.ts` and `worktree.ts` that coleam00#1198, coleam00#1206 added. Separate PR. Closes coleam00#1187
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
1 participant
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 the cross-clone gap that #1198 left open. That PR guarded `WorktreeProvider.findExisting()` (path 4 of 4), but `IsolationResolver` has three earlier adoption paths that bypass the provider layer entirely. Two clones of the same remote share `codebase_id` (identity is derived from `owner/repo`), so clone B could still silently adopt clone A's worktree via any of them.
Fixes #1183
Fixes #1188 (part 1 — cross-checkout)
Paths guarded
DB rows are never destroyed on cross-clone — they legitimately belong to the other clone.
Changes
Deferred
Explicitly not in this PR, to keep scope tight:
Behavior decision
On cross-checkout, all three paths throw consistently with `findExisting`. Rationale:
Testing
Attribution
Thanks to @halindrome for the three-issue root-cause mapping (#1183, #1186, #1188, #1192). #1186's metadata-based DB-level guard framed exactly the resolver paths this PR targets with a stricter live-check approach.
Summary by CodeRabbit