fix: prevent worktree reuse across different local clones#1186
fix: prevent worktree reuse across different local clones#1186halindrome wants to merge 5 commits intocoleam00:devfrom
Conversation
) When two local clones of the same remote exist, they share a codebase_id (derived from the remote URL). findActiveByWorkflow could return an isolation environment created from a sibling checkout, causing the workflow to operate in the wrong directory. Store source_repo_root in the isolation environment metadata at creation time, and verify it matches the current working directory's repo root before reusing. Environments without source_repo_root (pre-existing) are reused as before for backward compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughWorktree reuse is now gated by comparing the current repository canonical root to Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (workflow command)
participant Git as git (findRepoRoot / getCanonicalRepoPath)
participant Env as existingEnv (metadata)
participant Provider as Isolation Provider
participant Logger as Logger
CLI->>Git: detect currentRepoRoot
CLI->>Env: read metadata.source_repo_root
alt currentRepoRoot undetectable
CLI->>Logger: warn worktree.reuse_root_undetectable
CLI->>Provider: create new environment (store source_repo_root if available)
else both roots present and equal
CLI->>Provider: health-check existingEnv
Provider-->>CLI: healthy?
alt healthy
CLI->>Logger: reuse existingEnv
else unhealthy
CLI->>Provider: create new environment (store currentRepoRoot)
end
else roots present but different
CLI->>Logger: warn worktree.reuse_different_checkout
CLI->>Provider: create new environment (store currentRepoRoot)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 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 |
![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
🤖 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/cli/src/commands/workflow.ts`:
- Around line 431-437: The reuse guard currently compares a worktree-local path
from git.findRepoRoot(cwd) against existingEnv.metadata.source_repo_root, which
can differ for linked worktrees; change the logic to compare clone-stable
canonical repo identifiers instead: call getCanonicalRepoPath(cwd) (or use git
rev-parse --git-common-dir via an existing helper) to normalize the current
working directory and, if present, normalize
existingEnv.metadata.source_repo_root before computing reuseSameCheckout; update
the reuseSameCheckout assignment to use these canonical paths (or the shared
.git path) so the comparison is stable across linked worktrees and matches the
value persisted later as metadata.source_repo_root.
🪄 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: bea74955-cbf2-418d-ac15-8f8362eaf6b4
📒 Files selected for processing (1)
packages/cli/src/commands/workflow.ts
Add 3 tests for the cross-checkout guard: mismatch (skip reuse), absent metadata (backward compat), and matching root (normal reuse). Add warning log when currentRepoRoot is null but envSourceRoot exists. Document the intentional fallback behavior with a comment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QA Round 1 ReportCritical Issues
Added tests for:
Important Issues
Advisory Notes
VerdictCritical issue fixed (tests added). QA round 1 complete. 🤖 Generated with Claude Code |
Add test for worktree.reuse_root_undetectable: when findRepoRoot returns null but the existing env has a source_repo_root, reuse is allowed (fallback) with a warning logged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QA Round 2 ReportFindings
The four logical branches of the guard are now all tested:
All other aspects verified clean:
VerdictQA round 2: CLEAN — ready for review. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli/src/commands/workflow.test.ts`:
- Around line 1118-1163: The test description says “with warning” but never
asserts a warning; update the test for the workflowRunCommand scenario (where
isolationDb.findActiveByWorkflow returns metadata.source_repo_root and
gitModule.findRepoRoot returns null) to assert the logger warning was emitted:
mock or grab the logger used by workflowRunCommand (e.g., processLogger or the
module logger), ensure its warn method is spied/mocked before calling
workflowRunCommand('/clone-a', 'assist', 'hello', { branchName: 'my-feature' }),
and add an expectation that logger.warn was called (or called with a message
containing identifiers like "repo root" or "source_repo_root") after execution
so the warning path is actually verified.
🪄 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: cddfa05a-389d-4adb-9c87-fd5d17ae81f0
📒 Files selected for processing (2)
packages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/commands/workflow.ts
Address CodeRabbit review: git rev-parse --show-toplevel returns different paths inside linked worktrees. Use getCanonicalRepoPath instead, which resolves any worktree path back to the main repo via .git/worktrees/ gitdir linkage. This makes the source_repo_root comparison stable whether the user runs from the main checkout or from inside an Archon-managed worktree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CodeRabbit Review — AddressedCodeRabbit flagged that Fixed in e2af7a2: Replaced The Tests updated to mock 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli/src/commands/workflow.ts`:
- Around line 432-448: The code should resolve the repo top-level before
canonicalizing and must not silently swallow real errors: call findRepoRoot(cwd)
first and if it returns a string pass that value into getCanonicalRepoPath(...)
to compute currentRepoRoot (treat null as “not a git repo” but do not ignore
thrown errors); replace the empty catch with logic that logs the caught error
via processLogger (or appropriate logger) and rethrows unexpected errors, while
only allowing a benign fallback when findRepoRoot returned null; ensure the
value persisted into existingEnv.metadata.source_repo_root and the
reuseSameCheckout comparison use this canonicalized root.
🪄 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: 5499dbc9-54b7-46a3-8292-be192d2350e0
📒 Files selected for processing (2)
packages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/commands/workflow.test.ts
| let currentRepoRoot: string | null = null; | ||
| try { | ||
| currentRepoRoot = await git.getCanonicalRepoPath(cwd); | ||
| } catch { | ||
| // Non-fatal: cwd may not be a git repo (handled later) or git unavailable | ||
| } | ||
| const envSourceRoot = | ||
| existingEnv && typeof existingEnv.metadata?.source_repo_root === 'string' | ||
| ? existingEnv.metadata.source_repo_root | ||
| : undefined; | ||
|
|
||
| // Intentional: when currentRepoRoot or envSourceRoot is unavailable, fall back | ||
| // to allowing reuse. This preserves backward compat for pre-existing envs and | ||
| // avoids blocking workflows when git is temporarily unavailable. Risk: could | ||
| // allow cross-checkout reuse in edge cases — logged as a warning below. | ||
| const reuseSameCheckout = | ||
| !existingEnv || !envSourceRoot || !currentRepoRoot || envSourceRoot === currentRepoRoot; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "workflow.ts changed block:"
sed -n '426,536p' packages/cli/src/commands/workflow.ts
echo
echo "findRepoRoot() implementation:"
sed -n '14,40p' packages/git/src/repo.ts
echo
echo "getCanonicalRepoPath() implementation:"
sed -n '232,257p' packages/git/src/worktree.tsRepository: coleam00/Archon
Length of output: 6635
Resolve the repo root before canonicalizing it.
Line 434 calls getCanonicalRepoPath(cwd) directly, but that function returns non-worktree paths unchanged. If the command runs from a subdirectory like /repo/subdir, it returns /repo/subdir as-is, causing same-clone comparisons to fail across different directory invocations. This unstable value is then persisted into metadata.source_repo_root at line 535. Additionally, the empty catch block swallows all exceptions—including real worktree metadata failures—and silently falls back to reuse-allowed behavior.
Call findRepoRoot(cwd) first to resolve any subdirectory to the actual repo top-level (returning null only for non-repo cases), then canonicalize that root. Log and throw on real errors rather than silencing them.
🛠️ Proposed fix
let currentRepoRoot: string | null = null;
try {
- currentRepoRoot = await git.getCanonicalRepoPath(cwd);
- } catch {
- // Non-fatal: cwd may not be a git repo (handled later) or git unavailable
+ const repoRoot = await git.findRepoRoot(cwd);
+ currentRepoRoot = repoRoot ? await git.getCanonicalRepoPath(repoRoot) : null;
+ } catch (error) {
+ const err = error as Error;
+ getLog().error({ err, cwd }, 'worktree.reuse_root_detection_failed');
+ throw new Error(`Failed to resolve repository root for ${cwd}: ${err.message}`);
}Violates: "Never silently swallow errors related to unsupported or unsafe states — throw early with clear error messages."
Also applies to: 535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/workflow.ts` around lines 432 - 448, The code
should resolve the repo top-level before canonicalizing and must not silently
swallow real errors: call findRepoRoot(cwd) first and if it returns a string
pass that value into getCanonicalRepoPath(...) to compute currentRepoRoot (treat
null as “not a git repo” but do not ignore thrown errors); replace the empty
catch with logic that logs the caught error via processLogger (or appropriate
logger) and rethrows unexpected errors, while only allowing a benign fallback
when findRepoRoot returned null; ensure the value persisted into
existingEnv.metadata.source_repo_root and the reuseSameCheckout comparison use
this canonicalized root.
…g errors, assert warning - Call findRepoRoot(cwd) first to resolve subdirectories to repo top-level, then getCanonicalRepoPath to normalize worktree paths. - Replace empty catch with debug log for unexpected detection errors. - Add logger assertion to the undetectable-root test per CodeRabbit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CodeRabbit Comments 2 & 3 — AddressedComment 2 (Minor): Test says "with warning" but didn't assert the warning was logged. Comment 3 (Major): All 4 guard tests updated to mock the 🤖 Generated with Claude Code |
Related IssuesThis PR is a targeted fix for the DB-level worktree reuse path. During investigation, we identified the broader issue family:
This PR (#1186) mitigates the symptom at the isolation layer. #1192 describes the architectural root cause: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/workflow.test.ts (1)
980-1026: Assert the mismatch warning in the cross-checkout test.This case validates recreate behavior, but it should also assert
worktree.reuse_different_checkoutso the warning branch is explicitly covered.🧪 Suggested assertion
expect(provider.create).toHaveBeenCalled(); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.objectContaining({ envSourceRoot: '/clone-a', currentRepoRoot: '/clone-b' }), + 'worktree.reuse_different_checkout' + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/workflow.test.ts` around lines 980 - 1026, The test "skips reuse when existing env has different source_repo_root (GH-1183)" is missing an assertion that the reuse-different-checkout warning branch was taken; after calling workflowRunCommand add an assertion that conversationDb.updateConversation was invoked with worktree.reuse_different_checkout set true (e.g. expect(conversationDb.updateConversation).toHaveBeenCalledWith(expect.objectContaining({ worktree: { reuse_different_checkout: true } }))). This uses the existing mocked symbols conversationDb.updateConversation, workflowRunCommand, and the scenario set up via isolationDb.findActiveByWorkflow to ensure the warning branch is explicitly covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/commands/workflow.test.ts`:
- Around line 980-1026: The test "skips reuse when existing env has different
source_repo_root (GH-1183)" is missing an assertion that the
reuse-different-checkout warning branch was taken; after calling
workflowRunCommand add an assertion that conversationDb.updateConversation was
invoked with worktree.reuse_different_checkout set true (e.g.
expect(conversationDb.updateConversation).toHaveBeenCalledWith(expect.objectContaining({
worktree: { reuse_different_checkout: true } }))). This uses the existing mocked
symbols conversationDb.updateConversation, workflowRunCommand, and the scenario
set up via isolationDb.findActiveByWorkflow to ensure the warning branch is
explicitly covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ac3ece2-c2d9-4bdc-85e2-9ce65ae92eed
📒 Files selected for processing (2)
packages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/commands/workflow.ts
|
Thanks for this contribution, @halindrome! 🙏 Closing as superseded by #1198, which landed a stronger fix for the same class of bug at the provider layer ( Key differences in coverage:
Your work shaped the fix — the problem framing, the repro approach, and the clear scoping of limitations all fed directly into the investigation for #1198. Credit noted in that PR as well. Thanks again for digging into this! |
3 participants
Summary
Fixes #1183
source_repo_rootin isolation environment metadata when creating worktrees via the CLI--branchflag), verify the stored repo root matches the currentcwd's repo rootsource_repo_root(pre-existing) are reused as beforeRoot Cause
The
remote_agent_codebasestable derives codebase identity from the remote URL. Two local clones of the same remote (e.g.,~/project-aand~/project-b) resolve to the samecodebase_id.findActiveByWorkflowthen returns isolation environments from the wrong local clone since it only filters oncodebase_id + workflow_type + workflow_id.Approach
Rather than changing the database schema or codebase identity model, this fix adds a guard at the reuse check site in
workflow.ts. The current repo root is stored in the existingmetadataJSONB column at creation time, and compared at reuse time. This is the minimal change with the smallest blast radius.Known Limitations
IsolationResolver(Slack/Telegram/GitHub/Web) has the same theoretical gap but lower risk since server-side paths don't involve multiple local clones.WorktreeProvider.findExisting(), which checks filesystem paths that are shared across clones of the same remote.Test plan
bun run type-check— all packages passbun run lint— zero warningsbun run test— all tests passsource_repo_rootmetadata are still reused (backward compat)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests