diff --git a/CHANGELOG.md b/CHANGELOG.md index a2201632b2..7e862caf2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Cross-clone worktree isolation**: prevent workflows in one local clone from silently adopting worktrees or DB state owned by another local clone of the same remote. Two clones sharing a remote previously resolved to the same `codebase_id`, causing the isolation resolver's DB-driven paths (`findReusable`, `findLinkedIssueEnv`, `tryBranchAdoption`) to return the other clone's environment. All adoption paths now verify the worktree's `.git` pointer matches the requesting clone and throw a classified error on mismatch. `archon-implement` prompt was also tightened to stop AI agents from adopting unrelated branches they see via `git branch`. Thanks to @halindrome for the three-issue root-cause mapping. (#1193, #1188, #1183, #1198, #1206) + ## [0.3.6] - 2026-04-12 Web UI workflow experience improvements, CWD environment leak protection, and bug fixes. diff --git a/packages/docs-web/src/content/docs/reference/troubleshooting.md b/packages/docs-web/src/content/docs/reference/troubleshooting.md index 50805c7911..2c866166db 100644 --- a/packages/docs-web/src/content/docs/reference/troubleshooting.md +++ b/packages/docs-web/src/content/docs/reference/troubleshooting.md @@ -299,3 +299,40 @@ ARCHON_SUPPRESS_NESTED_CLAUDE_WARNING=1 archon workflow run ... ```bash ARCHON_CLAUDE_FIRST_EVENT_TIMEOUT_MS=120000 archon workflow run ... ``` + +## Worktree Belongs to a Different Clone + +**Symptom:** Running a workflow (especially with `--branch `) from one local clone surfaces one of these errors: + +- `Worktree at belongs to a different clone (). Remove it from that clone or use a different codebase registration.` +- `Cannot verify worktree ownership at : ` +- `Cannot adopt : path contains a full git checkout, not a worktree.` +- `Cannot adopt : .git pointer is not a git-worktree reference.` + +**Cause:** Archon derives codebase identity from the remote URL (`owner/repo`), so two local clones of the same remote share one `codebase_id`. Worktrees are stored under a shared path (`~/.archon/workspaces///worktrees/`), which means a worktree created by clone A is visible on disk from clone B. The isolation system refuses to silently adopt across clones because it would operate on the wrong filesystem state. + +**Fix — pick one:** + +1. **Remove the other clone's worktree.** If you no longer need the other clone's in-progress work: + + ```bash + # From the other clone's directory, find and remove the conflicting worktree + archon isolation list + archon complete # graceful cleanup + # or, if no work to preserve: + git worktree remove --force + ``` + +2. **Use a different branch name** for this run so the two clones don't compete for the same worktree path: + + ```bash + archon workflow run --branch "task" + ``` + +3. **Work from a single clone.** If both local checkouts are for the same project, consolidate to one. Archon's codebase registration currently assumes one local path per remote; true multi-clone support is tracked in [#1192](https://github.com/coleam00/Archon/issues/1192). + +**Other variants:** + +- `path contains a full git checkout, not a worktree`: something non-Archon created a full git repo at the worktree path. Remove or move it. +- `.git pointer is not a git-worktree reference`: the `.git` file at that path points somewhere unexpected (submodule, malformed). Inspect it with `cat /.git` and clean up manually. +- `Cannot verify worktree ownership`: filesystem permission or I/O error reading `/.git`. Check `ls -la ` and file permissions on `~/.archon/workspaces`. diff --git a/packages/git/src/git.test.ts b/packages/git/src/git.test.ts index 9c3287b04b..8f59d3b49c 100644 --- a/packages/git/src/git.test.ts +++ b/packages/git/src/git.test.ts @@ -1894,4 +1894,119 @@ branch refs/heads/feature/auth ); }); }); + + describe('verifyWorktreeOwnership', () => { + test('resolves for matching worktree pointer', async () => { + await writeFile( + join(testDir, '.git'), + 'gitdir: /workspace/my-repo/.git/worktrees/issue-42\n' + ); + + await expect( + git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ) + ).resolves.toBeUndefined(); + }); + + test('throws with "belongs to a different clone" when gitdir points elsewhere', async () => { + await writeFile(join(testDir, '.git'), 'gitdir: /other/clone/.git/worktrees/issue-42\n'); + + await expect( + git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ) + ).rejects.toThrow(/belongs to a different clone \(\/other\/clone\)/); + }); + + test('normalizes trailing slashes in both paths', async () => { + await writeFile( + join(testDir, '.git'), + 'gitdir: /workspace/my-repo/.git/worktrees/issue-42\n' + ); + + await expect( + git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo/') + ) + ).resolves.toBeUndefined(); + }); + + test('throws EISDIR when .git is a directory (full checkout at path)', async () => { + await realMkdir(join(testDir, '.git')); + + const promise = git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ); + await expect(promise).rejects.toThrow(/path contains a full git checkout/); + // Original errno is preserved on the wrapped error for robust + // classification downstream (not just a fragile substring match). + try { + await git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ); + } catch (err) { + expect((err as NodeJS.ErrnoException).code).toBe('EISDIR'); + } + }); + + test('throws ENOENT when .git file is missing', async () => { + await expect( + git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ) + ).rejects.toThrow(/Cannot verify worktree ownership/); + try { + await git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ); + } catch (err) { + expect((err as NodeJS.ErrnoException).code).toBe('ENOENT'); + } + }); + + test('throws on submodule pointer (gitdir into .git/modules/...)', async () => { + await writeFile( + join(testDir, '.git'), + 'gitdir: /workspace/my-repo/.git/modules/vendor/submodule\n' + ); + + await expect( + git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ) + ).rejects.toThrow(/not a git-worktree reference/); + }); + + test('throws on corrupted .git content (no gitdir prefix)', async () => { + await writeFile(join(testDir, '.git'), 'this is not a git pointer at all'); + + await expect( + git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ) + ).rejects.toThrow(/not a git-worktree reference/); + }); + + test('preserves original error via `cause` chain on fs errors', async () => { + try { + await git.verifyWorktreeOwnership( + git.toWorktreePath(testDir), + git.toRepoPath('/workspace/my-repo') + ); + } catch (err) { + expect((err as Error).cause).toBeDefined(); + expect(((err as Error).cause as NodeJS.ErrnoException).code).toBe('ENOENT'); + } + }); + }); }); diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts index 8cfdc865f7..adfac78b49 100644 --- a/packages/git/src/index.ts +++ b/packages/git/src/index.ts @@ -24,6 +24,7 @@ export { isWorktreePath, removeWorktree, getCanonicalRepoPath, + verifyWorktreeOwnership, } from './worktree'; // Branch operations diff --git a/packages/git/src/worktree.ts b/packages/git/src/worktree.ts index a7fa309385..62f6d1413e 100644 --- a/packages/git/src/worktree.ts +++ b/packages/git/src/worktree.ts @@ -1,5 +1,5 @@ import { readFile, access } from 'fs/promises'; -import { join } from 'path'; +import { join, resolve } from 'path'; import { createLogger, getArchonWorktreesPath, @@ -256,6 +256,82 @@ export async function getCanonicalRepoPath(path: string): Promise { return toRepoPath(path); } +/** + * Verify that the worktree at the given path belongs to the expected repo. + * + * Throws if the worktree's parent repo doesn't match the request, or if + * ownership cannot be determined. The caller relies on the throw-or-return + * contract: a successful return means the caller may safely adopt the + * worktree. This is intentionally strict — a permissive fallback here + * would re-introduce the cross-checkout bug this guard exists to prevent. + * + * Paths are normalized with `resolve()` before comparison to handle trailing + * slashes and relative components. Symlinked paths (where canonical vs + * registered paths differ by symlink resolution) are not equated — callers + * should register codebases with consistent path forms. + * + * Error classification (surfaced via `classifyIsolationError` in + * `@archon/isolation/errors.ts`): + * - "path contains a full git checkout" → EISDIR + * - "Cannot verify worktree ownership" → ENOENT / EACCES / EIO + * - "not a git-worktree reference" → submodule pointer or malformed + * - "belongs to a different clone" → cross-checkout + */ +export async function verifyWorktreeOwnership( + worktreePath: WorktreePath, + expectedRepo: RepoPath +): Promise { + let gitContent: string; + try { + gitContent = await readFile(join(worktreePath, '.git'), 'utf-8'); + } catch (error) { + const err = error as NodeJS.ErrnoException; + // Preserve the original errno on the wrapped error so downstream + // classifiers can match by `.code` instead of substring — resilient to + // Node.js message format changes. The original error is also kept via + // `cause` for debugging. + const wrap = (message: string): Error => { + const wrapped = new Error(message, { cause: err }); + if (err.code) (wrapped as NodeJS.ErrnoException).code = err.code; + return wrapped; + }; + // EISDIR: .git is a directory — path holds a full checkout, not a + // worktree. Refusing adoption prevents accidentally treating an + // unrelated repo at this path as ours. + if (err.code === 'EISDIR') { + throw wrap( + `Cannot adopt ${worktreePath}: path contains a full git checkout, not a worktree.` + ); + } + // ENOENT: .git file missing despite worktreeExists() reporting true — + // a TOCTOU race or filesystem corruption. Fail fast. + // EACCES/EIO/etc.: cannot verify ownership — fail fast rather than + // defaulting to permissive adoption. + throw wrap(`Cannot verify worktree ownership at ${worktreePath}: ${err.message}`); + } + + // gitdir: /path/to/repo/.git/worktrees/branch-name + const match = /gitdir: (.+)\/\.git\/worktrees\//.exec(gitContent); + if (!match) { + // Not a git-worktree pointer (e.g., submodule pointer, or malformed). + // We cannot confirm this is our worktree, so refuse adoption. + throw new Error(`Cannot adopt ${worktreePath}: .git pointer is not a git-worktree reference.`); + } + + // Compare on resolved paths (normalizes trailing slashes and relative + // components) but display the raw path from the .git pointer so the user + // sees the value they'd recognize. On Windows, `resolve()` would prepend + // a drive letter to the POSIX-style gitdir, making the error message + // misleading and causing platform-specific test breakage. + const existingRepoRaw = match[1]; + if (resolve(existingRepoRaw) !== resolve(expectedRepo)) { + throw new Error( + `Worktree at ${worktreePath} belongs to a different clone (${existingRepoRaw}). ` + + 'Remove it from that clone or use a different codebase registration.' + ); + } +} + /** * Extract owner and repo name from the last two segments of a repository path. * Throws if the path has fewer than 2 non-empty segments. diff --git a/packages/isolation/src/providers/worktree.test.ts b/packages/isolation/src/providers/worktree.test.ts index d231f1d898..f76f9f794d 100644 --- a/packages/isolation/src/providers/worktree.test.ts +++ b/packages/isolation/src/providers/worktree.test.ts @@ -590,6 +590,8 @@ describe('WorktreeProvider', () => { worktreeExistsSpy.mockResolvedValueOnce(false); // findWorktreeByBranch finds existing worktree findWorktreeByBranchSpy.mockResolvedValue('/workspace/worktrees/repo/feature-auth'); + // Same-clone ownership match so adoption proceeds + mockReadFile.mockResolvedValue('gitdir: /workspace/repo/.git/worktrees/feature-auth\n'); const env = await provider.create(request); @@ -605,6 +607,25 @@ describe('WorktreeProvider', () => { expect(addCalls).toHaveLength(0); }); + test('throws when PR-branch-adopted worktree belongs to a different clone', async () => { + const request: PRIsolationRequest = { + codebaseId: 'cb-123', + canonicalRepoPath: '/workspace/repo', + workflowType: 'pr', + identifier: '42', + prBranch: 'feature/auth', + isForkPR: false, + }; + + // Primary path misses, secondary findWorktreeByBranch hits + worktreeExistsSpy.mockResolvedValueOnce(false); + findWorktreeByBranchSpy.mockResolvedValue('/workspace/worktrees/repo/feature-auth'); + // .git points to a different clone + mockReadFile.mockResolvedValue('gitdir: /other/clone/.git/worktrees/feature-auth\n'); + + await expect(provider.create(request)).rejects.toThrow(/belongs to a different clone/); + }); + test('resets stale branch to start-point when it already exists', async () => { let callCount = 0; execSpy.mockImplementation(async (_cmd: string, args: string[]) => { diff --git a/packages/isolation/src/providers/worktree.ts b/packages/isolation/src/providers/worktree.ts index 326cafc9c8..4dd271027d 100644 --- a/packages/isolation/src/providers/worktree.ts +++ b/packages/isolation/src/providers/worktree.ts @@ -5,8 +5,8 @@ */ import { createHash } from 'crypto'; -import { access, readFile, rm } from 'fs/promises'; -import { join, resolve } from 'path'; +import { access, rm } from 'fs/promises'; +import { join } from 'path'; import { createLogger } from '@archon/paths'; import { @@ -20,6 +20,7 @@ import { mkdirAsync, removeWorktree, syncWorkspace, + verifyWorktreeOwnership, worktreeExists, toRepoPath, toWorktreePath, @@ -490,7 +491,21 @@ export class WorktreeProvider implements IIsolationProvider { // Throws on cross-checkout or unverifiable state — surfacing the problem // is safer than falling through to createNewBranch (which would report // a confusing "branch already exists" cascade) or silently adopting. - await this.verifyWorktreeOwnership(worktreePath, request.canonicalRepoPath, branchName); + try { + await verifyWorktreeOwnership(toWorktreePath(worktreePath), request.canonicalRepoPath); + } catch (err) { + getLog().warn( + { + worktreePath, + branchName, + codebaseId: request.codebaseId, + canonicalRepoPath: request.canonicalRepoPath, + err: (err as Error).message, + }, + 'worktree.adoption_refused_cross_checkout' + ); + throw err; + } getLog().info({ worktreePath, branchName }, 'worktree_adopted'); return this.buildAdoptedEnvironment(worktreePath, branchName, request); @@ -503,6 +518,25 @@ export class WorktreeProvider implements IIsolationProvider { request.prBranch ); if (existingByBranch) { + // Same cross-clone guard as the primary adoption path above — a + // worktree matching the PR branch might still belong to a different + // clone of the same remote. + try { + await verifyWorktreeOwnership(existingByBranch, request.canonicalRepoPath); + } catch (err) { + getLog().warn( + { + worktreePath: existingByBranch, + branchName: request.prBranch, + codebaseId: request.codebaseId, + canonicalRepoPath: request.canonicalRepoPath, + err: (err as Error).message, + }, + 'worktree.adoption_refused_cross_checkout' + ); + throw err; + } + getLog().info( { worktreePath: existingByBranch, branchName: request.prBranch }, 'worktree_adopted' @@ -514,69 +548,6 @@ export class WorktreeProvider implements IIsolationProvider { return null; } - /** - * Verify that the worktree at the given path belongs to the expected repo. - * - * Throws if the worktree's parent repo doesn't match the request, or if - * ownership cannot be determined. The caller relies on the throw-or-return - * contract: a successful return means the caller may safely adopt the - * worktree. This is intentionally strict — a permissive fallback here - * would re-introduce the cross-checkout bug this guard exists to prevent. - * - * Note: string comparison uses `resolve()` to normalize trailing slashes - * and relative components. Symlinked paths (where canonical vs registered - * paths differ by symlink resolution) are not equated — callers should - * register codebases with consistent path forms. - */ - private async verifyWorktreeOwnership( - worktreePath: string, - expectedRepo: string, - branchName: string - ): Promise { - let gitContent: string; - try { - gitContent = await readFile(join(worktreePath, '.git'), 'utf-8'); - } catch (error) { - const err = error as NodeJS.ErrnoException; - // EISDIR: .git is a directory — path holds a full checkout, not a - // worktree. Refusing adoption prevents accidentally treating an - // unrelated repo at this path as ours. - if (err.code === 'EISDIR') { - throw new Error( - `Cannot adopt ${worktreePath}: path contains a full git checkout, not a worktree.` - ); - } - // ENOENT: .git file missing despite worktreeExists() reporting true — - // a TOCTOU race or filesystem corruption. Fail fast. - // EACCES/EIO/etc.: cannot verify ownership — fail fast rather than - // defaulting to permissive adoption. - throw new Error(`Cannot verify worktree ownership at ${worktreePath}: ${err.message}`); - } - - // gitdir: /path/to/repo/.git/worktrees/branch-name - const match = /gitdir: (.+)\/\.git\/worktrees\//.exec(gitContent); - if (!match) { - // Not a git-worktree pointer (e.g., submodule pointer, or malformed). - // We cannot confirm this is our worktree, so refuse adoption. - throw new Error( - `Cannot adopt ${worktreePath}: .git pointer is not a git-worktree reference.` - ); - } - - const existingRepo = resolve(match[1]); - const expectedResolved = resolve(expectedRepo); - if (existingRepo !== expectedResolved) { - getLog().warn( - { worktreePath, branchName, existingRepo, expectedRepo: expectedResolved }, - 'worktree_adoption_refused_cross_checkout' - ); - throw new Error( - `Worktree at ${worktreePath} belongs to a different clone (${existingRepo}). ` + - 'Remove it from that clone or use a different codebase registration.' - ); - } - } - private buildAdoptedEnvironment( path: string, branchName: string, diff --git a/packages/isolation/src/resolver.test.ts b/packages/isolation/src/resolver.test.ts index ccc250e6dc..fa67b81d75 100644 --- a/packages/isolation/src/resolver.test.ts +++ b/packages/isolation/src/resolver.test.ts @@ -86,6 +86,7 @@ describe('IsolationResolver', () => { let getCanonicalSpy: ReturnType; let findWorktreeByBranchSpy: ReturnType; let isAncestorOfSpy: ReturnType; + let verifyWorktreeOwnershipSpy: ReturnType; beforeEach(() => { worktreeExistsSpy = spyOn(git, 'worktreeExists').mockResolvedValue(true); @@ -94,6 +95,9 @@ describe('IsolationResolver', () => { ); findWorktreeByBranchSpy = spyOn(git, 'findWorktreeByBranch').mockResolvedValue(null); isAncestorOfSpy = spyOn(git, 'isAncestorOf').mockResolvedValue(true); + // Default: ownership verification passes. Tests that exercise cross-clone + // behavior override this with a rejection. + verifyWorktreeOwnershipSpy = spyOn(git, 'verifyWorktreeOwnership').mockResolvedValue(undefined); }); afterEach(() => { @@ -101,6 +105,7 @@ describe('IsolationResolver', () => { getCanonicalSpy.mockRestore(); findWorktreeByBranchSpy.mockRestore(); isAncestorOfSpy.mockRestore(); + verifyWorktreeOwnershipSpy.mockRestore(); }); function createResolver(overrides?: Partial): IsolationResolver { @@ -792,4 +797,194 @@ describe('IsolationResolver', () => { expect(isAncestorOfSpy).not.toHaveBeenCalled(); }); + + // ------------------------------------------------------------------------- + // Cross-checkout ownership guard (#1183, #1188 part 1) + // + // Two clones of the same remote share codebase_id because identity is + // derived from owner/repo. Without these guards, clone B would adopt + // worktrees owned by clone A via the DB-driven resolver paths, bypassing + // the WorktreeProvider.findExisting guard. + // ------------------------------------------------------------------------- + describe('cross-checkout guard', () => { + test('findReusable throws when worktree belongs to a different clone', async () => { + const env = makeEnvRow(); + const updateStatusSpy = mock(() => Promise.resolve()); + const resolver = createResolver({ + store: makeMockStore({ + findActiveByWorkflow: async () => env, + updateStatus: updateStatusSpy, + }), + }); + // .git file points to a different clone than request.canonicalRepoPath + verifyWorktreeOwnershipSpy.mockRejectedValue( + new Error( + 'Worktree at /worktrees/issue-42 belongs to a different clone (/other/clone). ' + + 'Remove it from that clone or use a different codebase registration.' + ) + ); + + await expect( + resolver.resolve({ + existingEnvId: null, + codebase: defaultCodebase, + hints: { workflowType: 'issue', workflowId: '42' }, + platformType: 'web', + }) + ).rejects.toThrow(/belongs to a different clone/); + + // DB row is preserved — it legitimately belongs to the other clone + expect(updateStatusSpy).not.toHaveBeenCalled(); + }); + + test('findReusable succeeds when worktree belongs to the same clone', async () => { + const env = makeEnvRow(); + const resolver = createResolver({ + store: makeMockStore({ findActiveByWorkflow: async () => env }), + }); + // Default ownership spy resolves — same-clone match + + const result = await resolver.resolve({ + existingEnvId: null, + codebase: defaultCodebase, + hints: { workflowType: 'issue', workflowId: '42' }, + platformType: 'web', + }); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.method.type).toBe('workflow_reuse'); + } + expect(verifyWorktreeOwnershipSpy).toHaveBeenCalledWith( + '/worktrees/issue-42', + '/repos/myrepo' + ); + }); + + test('findLinkedIssueEnv throws when linked env belongs to a different clone', async () => { + const linkedEnv = makeEnvRow({ + workflow_type: 'issue', + workflow_id: '100', + working_path: '/worktrees/issue-100', + branch_name: 'issue-100', + }); + const updateStatusSpy = mock(() => Promise.resolve()); + const resolver = createResolver({ + store: makeMockStore({ + // First path (findReusable) misses — no active env for requested workflowId + // Second path (findLinkedIssueEnv) returns linkedEnv for issue 100 + findActiveByWorkflow: async (_c, type, id) => + type === 'issue' && id === '100' ? linkedEnv : null, + updateStatus: updateStatusSpy, + }), + }); + verifyWorktreeOwnershipSpy.mockRejectedValue( + new Error( + 'Worktree at /worktrees/issue-100 belongs to a different clone (/other/clone). ' + + 'Remove it from that clone or use a different codebase registration.' + ) + ); + + await expect( + resolver.resolve({ + existingEnvId: null, + codebase: defaultCodebase, + hints: { + workflowType: 'thread', + workflowId: 'some-thread', + linkedIssues: [100], + }, + platformType: 'web', + }) + ).rejects.toThrow(/belongs to a different clone/); + + // Linked DB row preserved — belongs to the other clone + expect(updateStatusSpy).not.toHaveBeenCalled(); + }); + + test('findLinkedIssueEnv succeeds when linked env belongs to the same clone', async () => { + const linkedEnv = makeEnvRow({ + workflow_type: 'issue', + workflow_id: '100', + working_path: '/worktrees/issue-100', + branch_name: 'issue-100', + }); + const resolver = createResolver({ + store: makeMockStore({ + findActiveByWorkflow: async (_c, type, id) => + type === 'issue' && id === '100' ? linkedEnv : null, + }), + }); + // Default ownership spy resolves — same-clone match + + const result = await resolver.resolve({ + existingEnvId: null, + codebase: defaultCodebase, + hints: { + workflowType: 'thread', + workflowId: 'some-thread', + linkedIssues: [100], + }, + platformType: 'web', + }); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.method.type).toBe('linked_issue_reuse'); + } + }); + + test('tryBranchAdoption throws when discovered worktree belongs to a different clone', async () => { + findWorktreeByBranchSpy.mockResolvedValue('/worktrees/feature-auth'); + verifyWorktreeOwnershipSpy.mockRejectedValue( + new Error( + 'Worktree at /worktrees/feature-auth belongs to a different clone (/other/clone). ' + + 'Remove it from that clone or use a different codebase registration.' + ) + ); + const createSpy = mock(async () => makeEnvRow()); + const resolver = createResolver({ store: makeMockStore({ create: createSpy }) }); + + await expect( + resolver.resolve({ + existingEnvId: null, + codebase: defaultCodebase, + hints: { + workflowType: 'pr', + workflowId: 'pr-42', + prBranch: git.toBranchName('feature-auth'), + }, + platformType: 'web', + }) + ).rejects.toThrow(/belongs to a different clone/); + + // Symmetry with paths 1+2: no DB mutation on cross-clone rejection. + // Here it's create (vs updateStatus) because tryBranchAdoption writes + // a new row rather than reusing an existing one. + expect(createSpy).not.toHaveBeenCalled(); + }); + + test('tryBranchAdoption succeeds when discovered worktree belongs to the same clone', async () => { + findWorktreeByBranchSpy.mockResolvedValue('/worktrees/feature-auth'); + // Default ownership spy resolves — same-clone match + + const resolver = createResolver(); + + const result = await resolver.resolve({ + existingEnvId: null, + codebase: defaultCodebase, + hints: { + workflowType: 'pr', + workflowId: 'pr-42', + prBranch: git.toBranchName('feature-auth'), + }, + platformType: 'web', + }); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.method.type).toBe('branch_adoption'); + } + }); + }); }); diff --git a/packages/isolation/src/resolver.ts b/packages/isolation/src/resolver.ts index 8ed57b07f9..935be19a2f 100644 --- a/packages/isolation/src/resolver.ts +++ b/packages/isolation/src/resolver.ts @@ -14,8 +14,9 @@ import { findWorktreeByBranch, toBranchName, isAncestorOf, + verifyWorktreeOwnership, } from '@archon/git'; -import type { RepoPath, BranchName } from '@archon/git'; +import type { RepoPath, BranchName, WorktreePath } from '@archon/git'; import type { IIsolationProvider, @@ -105,8 +106,38 @@ export class IsolationResolver { const workflowType: IsolationWorkflowType = hints?.workflowType ?? 'thread'; const workflowId = hints?.workflowId ?? ''; + // 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 } + ); + } + // 3. Check for existing environment with same workflow - const reusable = await this.findReusable(codebase.id, workflowType, workflowId, baseBranch); + const reusable = await this.findReusable( + codebase.id, + canonicalPath, + workflowType, + workflowId, + baseBranch + ); if (reusable) { return { status: 'resolved', @@ -119,7 +150,7 @@ export class IsolationResolver { // 4. Check linked issues for sharing if (hints?.linkedIssues?.length) { - const linked = await this.findLinkedIssueEnv(codebase.id, hints.linkedIssues); + const linked = await this.findLinkedIssueEnv(codebase.id, canonicalPath, hints.linkedIssues); if (linked) return linked; } @@ -127,6 +158,7 @@ export class IsolationResolver { if (hints?.prBranch) { const adopted = await this.tryBranchAdoption( codebase, + canonicalPath, hints, workflowType, workflowId, @@ -136,7 +168,6 @@ export class IsolationResolver { } // 6. Create new environment - const canonicalPath = await getCanonicalRepoPath(codebase.defaultCwd); return this.createNewEnvironment( codebase, workflowType, @@ -205,11 +236,43 @@ export class IsolationResolver { return null; } + /** + * Verify that an on-disk worktree belongs to the expected repo before + * adopting. Wraps the shared `verifyWorktreeOwnership` with logging that + * includes structured fields for incident debugging — the error message + * alone is not enough because stack traces and call sites vary. + * + * Throws on mismatch (re-throws the original error so `classifyIsolationError` + * and `isKnownIsolationError` pattern-match against the user-facing message). + */ + private async assertWorktreeOwnership( + worktreePath: WorktreePath, + canonicalRepoPath: RepoPath, + logContext: Record, + logEvent: string + ): Promise { + try { + await verifyWorktreeOwnership(worktreePath, canonicalRepoPath); + } catch (err) { + getLog().warn( + { ...logContext, worktreePath, canonicalRepoPath, err: (err as Error).message }, + logEvent + ); + throw err; + } + } + /** * Find a reusable environment by workflow identity. + * + * Verifies that the on-disk worktree belongs to `canonicalRepoPath` before + * returning. On cross-clone mismatch, throws — the DB row belongs to the + * other clone and we must not adopt it. The other clone's row is preserved + * (no markDestroyed) so the other clone's work continues. */ private async findReusable( codebaseId: string, + canonicalRepoPath: RepoPath, workflowType: IsolationWorkflowType, workflowId: string, baseBranch?: BranchName @@ -217,7 +280,15 @@ export class IsolationResolver { const existing = await this.store.findActiveByWorkflow(codebaseId, workflowType, workflowId); if (!existing) return null; - if (await worktreeExists(toWorktreePath(existing.working_path))) { + const worktreePath = toWorktreePath(existing.working_path); + if (await worktreeExists(worktreePath)) { + await this.assertWorktreeOwnership( + worktreePath, + canonicalRepoPath, + { codebaseId, workflowType, workflowId }, + 'isolation.reuse_refused_cross_checkout' + ); + getLog().debug({ workflowType, workflowId }, 'isolation_reuse_existing'); const warnings = await this.collectBaseBranchWarnings(existing, baseBranch, { workflowType, @@ -232,9 +303,17 @@ export class IsolationResolver { /** * Find an environment linked to one of the given issue numbers. + * + * Verifies each candidate worktree belongs to `canonicalRepoPath` before + * adopting. On cross-clone mismatch, throws — this stops iteration over any + * remaining linked issues. Intentional: if a linked env is owned by another + * clone, the user's machine state is anomalous (two clones of the same + * remote) and they should resolve it explicitly rather than have us skip + * past the signal. For the 99% single-clone case, this path always succeeds. */ private async findLinkedIssueEnv( codebaseId: string, + canonicalRepoPath: RepoPath, linkedIssues: number[] ): Promise { for (const issueNum of linkedIssues) { @@ -245,7 +324,15 @@ export class IsolationResolver { ); if (!linkedEnv) continue; - if (await worktreeExists(toWorktreePath(linkedEnv.working_path))) { + const worktreePath = toWorktreePath(linkedEnv.working_path); + if (await worktreeExists(worktreePath)) { + await this.assertWorktreeOwnership( + worktreePath, + canonicalRepoPath, + { codebaseId, issueNum }, + 'isolation.linked_issue_refused_cross_checkout' + ); + getLog().debug({ issueNum, codebaseId }, 'isolation_share_linked_issue'); return { status: 'resolved', @@ -262,9 +349,14 @@ export class IsolationResolver { /** * Try adopting an existing worktree matching a PR branch. + * + * Verifies ownership of the discovered worktree before recording it in the + * DB. On cross-clone mismatch, throws — adopting another clone's worktree + * would create a stale DB row pointing at someone else's filesystem state. */ private async tryBranchAdoption( codebase: ResolveRequest['codebase'] & object, + canonicalRepoPath: RepoPath, hints: IsolationHints, workflowType: IsolationWorkflowType, workflowId: string, @@ -273,9 +365,15 @@ export class IsolationResolver { const prBranch = hints.prBranch; if (!prBranch) return null; - const canonicalPath = await getCanonicalRepoPath(codebase.defaultCwd); - const adoptedPath = await findWorktreeByBranch(canonicalPath, prBranch); + const adoptedPath = await findWorktreeByBranch(canonicalRepoPath, prBranch); if (adoptedPath && (await worktreeExists(adoptedPath))) { + await this.assertWorktreeOwnership( + adoptedPath, + canonicalRepoPath, + { codebaseId: codebase.id, prBranch }, + 'isolation.branch_adoption_refused_cross_checkout' + ); + getLog().info({ adoptedPath, prBranch }, 'isolation_worktree_adopted'); const env = await this.store.create({ codebase_id: codebase.id,