From de6fe8b7eabd220d100e4cc652413461d444d2ba Mon Sep 17 00:00:00 2001 From: Lesley Murfin Date: Thu, 27 Aug 2026 00:35:18 -0600 Subject: [PATCH] fix(worktrees): resolve id: worktree selectors by path equivalence (#16243) (#16494) * test(worktrees): cover id: selector path-spelling parity with path: (#16243) The renderer can only address a workspace by id (toRuntimeWorktreeSelector always emits id:::), and the runtime matches that id byte for byte while a path: selector has always compared through normalizeRuntimePathForComparison. A stored id that spells its path differently from `git worktree list` therefore resolves for the CLI and answers selector_not_found for the UI, which reads that as a stale local mirror, calls forgetLocal, reports success, and lets the row return on the next catalog refresh: a silent delete. These tests fail on both resolution sites -- the fleet `id:` branch of resolveWorktreeSelector and the scoped resolveScopedWorktreeIdRow a host-qualified removal takes -- and pin what must stay closed: an exact repo id (STA-4343), host qualification, dot segments neither selector canonicalizes, and a refusal rather than a guess when two rows spell one path. 13 failing, 35 passing. * fix(worktrees): resolve id: worktree selectors by path equivalence (#16243) worktreeIdComparisonKey names one repo, one filesystem location, and one folder-workspace instance, folding exactly the path spellings normalizeRuntimePathForComparison already folds for a path: selector -- and nothing more, so dot segments stay unresolved for both shapes. Both id: resolution sites consult it only after an exact match finds nothing: the fleet branch of resolveWorktreeSelector and resolveScopedWorktreeIdRow, which a host-qualified removal takes. runtimeWorktreeIdsEqual now derives from the same key so the runtime has one normalizer rather than a parallel one. Not a pure refactor at that last site: runtimeWorktreeIdsEqual used to normalize-compare ids that parse but carry an empty repoId or an empty path ('::/p', or 'repo::' against 'repo::/'), and worktreeIdComparisonKey returns null for those, so across its call sites (PTY identity, refresh, mutation queue) such ids now compare byte-exact instead. That narrows matching rather than widening it, no real worktree carries such an id, and it is the behavior #15616 guarantees for malformed ids -- but it is a behavior delta, not just a tidy-up. Perf (#14399): the exact match is still tried first and still wins outright, so a resolvable id costs exactly what it did before. Neither site adds a scan -- the fleet branch re-filters the array it had already listed, the scoped lookup re-filters the single owning repo's projected rows -- so an explicit id still never scans every repo. Fail-closed behavior is unchanged: the repo id compares exactly (STA-4343), host qualification is untouched, the folder-workspace instance suffix stays part of the path, and a scoped lookup with two equivalent rows refuses instead of guessing. The bare unprefixed selector branch keeps byte-exact id matching, since only the id: shape reaches a renderer caller. Shares src/shared/worktree/id.ts with the open #15616, which introduces worktreeIdComparisonKey for the same divergence in lineage pruning and authoritative-scan purging; this adopts that helper rather than adding a second one. Complementary to the open #16295, which makes the miss visible; this removes the miss. * chore(worktrees): satisfy oxfmt and oxlint on #16243 tests oxfmt --check flagged both new test files and oxlint's unicorn/no-useless-fallback-in-spread flagged the store mock; the full lint and format gates now match the pre-change baseline. * test(worktrees): pin Windows spellings and malformed-id exactness (#16243) Review found two axes the first pass left unproven at the two id: resolution sites. Both are the invariants the open #15616 guarantees for the shared worktreeIdComparisonKey it introduces for #15598, so violating either here would break a contract a sibling PR depends on. Windows: #15598's whole defect is that one checkout is recorded under both `D:\Agentic\game2` and `D:/Agentic/game2`. The fleet branch, the scoped removal lookup, and the key itself now each resolve the backslash spelling against the forward-slash spelling git reports, and fold drive-letter case -- while a backslash inside a POSIX path stays a filename character and a POSIX root stays case-sensitive, exactly as normalizeRuntimePathForComparison already decides for a path: selector. Malformed ids keep exact matching at both sites: an id with no repo boundary or an empty path still refuses, and the scoped lookup still refuses it without scanning. Four of these fail without the production change (three fleet/removal Windows cases and the scoped one); the malformed-id and POSIX-backslash cases are invariant guards that hold either way. Verified: 58 passed in the three files; 18 fail with the production hunks reverted; orca-runtime.test.ts and worktree-teardown-unstopped-pty.test.ts green (1270 passed | 1 skipped); pnpm tc:node clean. * test(worktrees): pin Windows id: spelling folds and fleet ambiguity refusal (#16243) The Windows backslash spelling now rides the ID_SPELLINGS rows, so it is driven through both id: sites -- resolveWorktreeSelector and the scoped removal target -- and compared against what the same workspace's path: selector resolves, rather than only through worktreeIdComparisonKey. That is the spelling #15598/#15616 found in the wild and the one the owner's Windows client produces. The fleet path's ambiguity refusal had no test: two same-repo rows spelling one directory, an id: matching neither exactly, must reject selector_ambiguous. It is the fail-closed guard on a delete-capable resolver, and the property a later refactor is most likely to turn into a silent pick. Also records two limits at the source instead of leaving them to be rediscovered: a UNC or WSL root never folds into a drive-letter location (while Windows' two WSL UNC aliases do name one location), and a folder-workspace id keeps a trailing slash placed before the ::workspace: suffix, so that spelling stays exact-match-only. Neither behavior changes here. The file docblock overclaimed parity. path: collapses duplicate same-host registrations to the first row while a folded id: refuses them; the contract this file pins is path-spelling parity, not dedup parity, and the divergence is deliberate because this resolver also serves delete. Non-vacuity, verified by temporarily reverting the production hunks: neutralizing both id: fallbacks turns 10 of these tests red, including both new Windows rows and the ambiguity refusal (it degrades to selector_not_found). Making the fleet fallback pick the first folded match instead of collecting all of them turns the ambiguity test red on its own. The remaining cases -- malformed ids, dot segments, the POSIX backslash, the folder-workspace slash -- pass against the pre-fix code too: they guard against future widening rather than proving this fix. Drops the two Windows cases the ID_SPELLINGS row subsumes. The drive-letter case test asserted only the Windows half its name promised; it now also pins that a POSIX root does NOT fold case, since an unconditional lowercase would merge /data/Foo with /data/foo on the platform CI runs on. Fixture paths use the upstream-attested /srv/projects prefix (and a neutral plugin-host leaf) instead of a local install root; the spelling variations the tests exist to pin -- doubled separator, dot segment, trailing slash, uppercase POSIX, cafe NFC/NFD, and the Windows D: rows -- are unchanged in form. * docs(worktrees): trim the id: selector test header and document the comparison key (#16243) --- src/main/runtime/orca-runtime.ts | 22 +- .../repo-worktree-row-resolution.test.ts | 85 ++++++ .../runtime/repo-worktree-row-resolution.ts | 21 +- ...ktree-rm-id-selector-path-spelling.test.ts | 273 ++++++++++++++++++ src/shared/worktree/id.test.ts | 65 ++++- src/shared/worktree/id.ts | 17 ++ 6 files changed, 473 insertions(+), 10 deletions(-) create mode 100644 src/main/runtime/worktree-rm-id-selector-path-spelling.test.ts diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index c33d741bc29..ee1f977ca88 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -534,7 +534,8 @@ import { WORKTREE_ID_SEPARATOR, getRepoIdFromWorktreeId, splitWorktreeId, - splitWorktreeIdForFilesystem + splitWorktreeIdForFilesystem, + worktreeIdComparisonKey } from '../../shared/worktree/id' import { getProjectIdForProviderIdentity, @@ -32294,6 +32295,7 @@ export class OrcaRuntimeService { return worktreeId } + /** Resolves one workspace or throws `selector_not_found` / `selector_ambiguous` — never picks a winner. */ private async resolveWorktreeSelector(selector: string): Promise { const explicitWorktreeId = this.getValidatedExplicitWorktreeIdSelector(selector) // Why only `id:`: every other selector kind is matched across the whole fleet, and their @@ -32315,6 +32317,16 @@ export class OrcaRuntimeService { if (selector.startsWith('id:')) { const worktreeId = explicitWorktreeId ?? selector.slice(3) candidates = worktrees.filter((worktree) => worktree.id === worktreeId) + if (candidates.length === 0) { + // Why (#16243): `id:` is the only shape the renderer can send, and a stored id can spell + // its path differently from the scan — the divergence `path:` has always absorbed. + // The bare unprefixed branch below stays byte-exact on purpose: only `id:` reaches a + // renderer caller, so `id:repo::p/` folds here while bare `repo::p/` still misses. + const comparisonKey = worktreeIdComparisonKey(worktreeId) + candidates = comparisonKey + ? worktrees.filter((worktree) => worktreeIdComparisonKey(worktree.id) === comparisonKey) + : candidates + } if (candidates.length === 0) { const parsed = splitWorktreeIdForFilesystem(worktreeId) const repo = parsed ? this.store?.getRepo(parsed.repoId) : null @@ -41428,12 +41440,8 @@ function runtimePathsEqual(left: string, right: string): boolean { * Windows/WSL/SSH ids still match themselves across hosts. */ function runtimeWorktreeIdsEqual(left: string, right: string): boolean { - const parsedLeft = splitWorktreeId(left) - const parsedRight = splitWorktreeId(right) - return parsedLeft && parsedRight - ? parsedLeft.repoId === parsedRight.repoId && - runtimePathsEqual(parsedLeft.worktreePath, parsedRight.worktreePath) - : left === right + const leftKey = worktreeIdComparisonKey(left) + return leftKey === null ? left === right : leftKey === worktreeIdComparisonKey(right) } function runtimeWorktreeIdentityKey(worktreeId: string): string { diff --git a/src/main/runtime/repo-worktree-row-resolution.test.ts b/src/main/runtime/repo-worktree-row-resolution.test.ts index be50f5678a7..d4bff4a9442 100644 --- a/src/main/runtime/repo-worktree-row-resolution.test.ts +++ b/src/main/runtime/repo-worktree-row-resolution.test.ts @@ -200,3 +200,88 @@ describe('host-qualified scoped worktree resolution', () => { expect(getRepos).not.toHaveBeenCalled() }) }) + +/** + * #16243: the renderer can only address a workspace by `id:::`, and this scoped + * lookup is what a host-qualified removal resolves through. It matched the id byte for byte while a + * `path:` selector has always compared through `normalizeRuntimePathForComparison`, so a stored id + * spelling its path differently from `git worktree list` resolved for the CLI and not for the UI. + */ +describe('scoped worktree id resolution across path spellings (#16243)', () => { + it.each([ + ['a trailing slash', '/same/worktree', 'shared::/same/worktree/'], + ['a doubled separator', '/same/worktree', 'shared::/same//worktree'], + ['an NFD name', '/same/café', `shared::${'/same/café'.normalize('NFD')}`] + ])('resolves the scanned row when the id carries %s', async (_label, scannedPath, worktreeId) => { + const owner = repo('shared', '/local/repo', { executionHostId: 'local' }) + const deps = createDeps([owner]) + deps.scanRepo.mockImplementation(async () => ({ + ok: true, + worktrees: [gitWorktree(scannedPath)] + })) + + await expect(resolveScopedWorktreeIdRow(deps, worktreeId, 'local')).resolves.toMatchObject({ + id: `shared::${scannedPath}`, + path: scannedPath + }) + }) + + it('still refuses the same path under a different repo id', async () => { + const deps = createDeps([ + repo('shared', '/local/repo', { executionHostId: 'local' }), + repo('unrelated', '/unrelated/repo', { executionHostId: 'local' }) + ]) + + await expect( + resolveScopedWorktreeIdRow(deps, 'unrelated::/same/worktree/', 'local') + ).resolves.toBeNull() + }) + + it('refuses rather than guessing when two rows spell one path', async () => { + const owner = repo('shared', '/local/repo', { executionHostId: 'local' }) + const deps = createDeps([owner]) + deps.scanRepo.mockImplementation(async () => ({ + ok: true, + worktrees: [gitWorktree('/same/worktree'), gitWorktree('/same//worktree')] + })) + + await expect( + resolveScopedWorktreeIdRow(deps, 'shared::/same/worktree/', 'local') + ).resolves.toBeNull() + }) + + it('prefers the exactly matching row over an equivalent spelling', async () => { + const owner = repo('shared', '/local/repo', { executionHostId: 'local' }) + const deps = createDeps([owner]) + deps.scanRepo.mockImplementation(async () => ({ + ok: true, + worktrees: [gitWorktree('/same//worktree'), gitWorktree('/same/worktree')] + })) + + await expect( + resolveScopedWorktreeIdRow(deps, 'shared::/same//worktree', 'local') + ).resolves.toMatchObject({ id: 'shared::/same//worktree' }) + }) + + // #15598/#15616: the backslash spelling is what a pre-restart Windows registration recorded. + it('resolves a Windows backslash id against the forward-slash spelling git reports', async () => { + const path = 'D:/Agentic/game2/battle-core' + const owner = repo('shared', 'D:/Agentic/game2', { executionHostId: 'runtime:windows' }) + const deps = createDeps([owner]) + deps.scanRepo.mockImplementation(async () => ({ ok: true, worktrees: [gitWorktree(path)] })) + + await expect( + resolveScopedWorktreeIdRow(deps, 'shared::D:\\Agentic\\game2\\battle-core', 'runtime:windows') + ).resolves.toMatchObject({ id: `shared::${path}`, path }) + }) + + it.each([ + ['no repo boundary', 'not-an-id'], + ['an empty path', 'shared::'] + ])('keeps exact matching for a malformed id with %s', async (_label, worktreeId) => { + const deps = createDeps([repo('shared', '/local/repo', { executionHostId: 'local' })]) + + await expect(resolveScopedWorktreeIdRow(deps, worktreeId, 'local')).resolves.toBeNull() + expect(deps.scanRepo).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/runtime/repo-worktree-row-resolution.ts b/src/main/runtime/repo-worktree-row-resolution.ts index b65fd9295f6..a9ef618ced7 100644 --- a/src/main/runtime/repo-worktree-row-resolution.ts +++ b/src/main/runtime/repo-worktree-row-resolution.ts @@ -1,4 +1,8 @@ -import { splitWorktreeId, splitWorktreeIdForFilesystem } from '../../shared/worktree/id' +import { + splitWorktreeId, + splitWorktreeIdForFilesystem, + worktreeIdComparisonKey +} from '../../shared/worktree/id' import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import { isFolderRepo } from '../../shared/repo-kind' import { projectResolvedWorktreeLineage } from '../../shared/resolved-worktree-lineage' @@ -195,5 +199,18 @@ export async function resolveScopedWorktreeIdRow( resolveLocalProjectRuntimesForRepos(store, [repo]) ) const projected = projectResolvedWorktreeLineage(rows, store.getAllWorktreeLineage?.() ?? {}) - return projected.find((worktree) => worktree.id === worktreeId) ?? null + const exact = projected.find((worktree) => worktree.id === worktreeId) + if (exact) { + return exact + } + // Why (#16243): the scan can spell this id's path differently — the divergence `path:` absorbs. + // One equivalent row may stand in; two is an ambiguity a scoped lookup must refuse, not guess. + const comparisonKey = worktreeIdComparisonKey(worktreeId) + if (comparisonKey === null) { + return null + } + const equivalent = projected.filter( + (worktree) => worktreeIdComparisonKey(worktree.id) === comparisonKey + ) + return equivalent.length === 1 ? equivalent[0] : null } diff --git a/src/main/runtime/worktree-rm-id-selector-path-spelling.test.ts b/src/main/runtime/worktree-rm-id-selector-path-spelling.test.ts new file mode 100644 index 00000000000..f4e61a76381 --- /dev/null +++ b/src/main/runtime/worktree-rm-id-selector-path-spelling.test.ts @@ -0,0 +1,273 @@ +/** + * Pins path-SPELLING parity between an `id:` selector and `path:`, deliberately not dedupe parity: + * where two same-host rows spell one directory, `path:` collapses them to the first while `id:` + * refuses as ambiguous, because this resolver also serves delete (#16243). + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const electronMocks = vi.hoisted(() => { + const ipcMain = { + on: vi.fn(() => ipcMain), + removeListener: vi.fn(() => ipcMain), + emit: vi.fn(() => true) + } + return { + BrowserWindow: { fromId: vi.fn((): unknown => null) }, + webContents: { fromId: vi.fn((): unknown => null) }, + ipcMain, + app: { getPath: vi.fn(() => '/tmp'), isPackaged: false } + } +}) +vi.mock('electron', () => electronMocks) + +const getSshGitProviderMock = vi.hoisted(() => vi.fn()) +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: vi.fn(() => 0), + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE: 'unavailable', + requireSshGitProvider: (connectionId: string) => getSshGitProviderMock(connectionId) +})) + +const listWorktreesStrictMock = vi.hoisted(() => vi.fn()) +vi.mock('../git/worktree', async (importOriginal) => ({ + ...(await importOriginal>()), + listWorktreesStrict: listWorktreesStrictMock +})) + +import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' +import { OrcaRuntimeService } from './orca-runtime' + +const REPO_ID = 'repo-local' +const REPO_PATH = '/srv/projects/app' +/** The spelling `git worktree list` reports. */ +const WORKTREE_PATH = '/srv/projects/workspaces/plugin-host' +const CANONICAL_ID = `${REPO_ID}::${WORKTREE_PATH}` + +/** One directory, other spellings a stored id can legitimately carry. */ +const ID_SPELLINGS: [label: string, worktreePath: string, repoPath?: string][] = [ + ['a trailing slash', `${WORKTREE_PATH}/`], + ['a doubled separator', '/srv/projects//workspaces/plugin-host'], + ['an NFD workspace name', '/srv/projects/workspaces/café-plugin'.normalize('NFD')], + // #15598/#15616: a Windows registration records backslashes; git reports forward slashes. + ['a backslash Windows spelling', 'D:\\Agentic\\game2\\battle-core', 'D:/Agentic/game2'] +] + +/** What a scan reports for a stored id: the same directory, canonically spelled. */ +function scannedSpellingOf(storedPath: string): string { + const slashed = /^[A-Za-z]:[\\/]/.test(storedPath) ? storedPath.replace(/\\/g, '/') : storedPath + return slashed.normalize('NFC').replace(/\/+/g, '/').replace(/\/$/, '') +} + +/** One registered repo whose worktree meta is writable, so a delete's `forgetLocal` is observable. */ +function makeStore(repoPath: string = REPO_PATH) { + const metaById: Record> = {} + const store = { + getRepo: (id: string) => store.getRepos().find((repo) => repo.id === id), + getRepos: () => [ + { id: REPO_ID, path: repoPath, displayName: 'app', badgeColor: 'blue', addedAt: 1 } + ], + getAllWorktreeMeta: vi.fn(() => metaById), + getWorktreeMeta: (id: string) => metaById[id], + setWorktreeMeta: (id: string, meta: Record) => { + metaById[id] = { ...metaById[id], ...meta } + return metaById[id] + }, + removeWorktreeMeta: () => {}, + getAllWorktreeLineage: () => ({}), + getAllWorkspaceLineage: () => ({}), + removeWorktreeLineage: vi.fn(), + removeWorkspaceLineage: vi.fn(), + getGitHubCache: () => undefined as never, + getSettings: () => ({ + workspaceDir: '/tmp/workspaces', + nestWorkspaces: false, + refreshLocalBaseRefOnWorktreeCreate: false, + branchPrefix: 'none', + branchPrefixCustom: '' + }), + getProjects: () => [] + } + return store +} + +/** `git worktree list` output: the main checkout plus one workspace at `worktreePath`. */ +function scanReports(worktreePath: string, repoPath: string = REPO_PATH): void { + listWorktreesStrictMock.mockResolvedValue([ + { path: repoPath, head: 'abc', branch: 'main', isBare: false, isMainWorktree: true }, + { path: worktreePath, head: 'def', branch: 'feature', isBare: false, isMainWorktree: false } + ]) +} + +type RemovalInternals = { + resolveWorktreeRemovalTarget: ( + worktreeSelector: string, + requiredHostId?: string + ) => Promise<{ id: string; repoId: string; path: string }> +} + +beforeEach(() => { + getSshGitProviderMock.mockReset() + listWorktreesStrictMock.mockReset() +}) + +describe('worktree id selectors vs. the path spelling git reports (#16243)', () => { + it.each(ID_SPELLINGS)( + 'resolves through the fleet path what `path:` resolves when the id carries %s', + async (_label, storedPath, repoPath) => { + scanReports(scannedSpellingOf(storedPath), repoPath) + const runtime = new OrcaRuntimeService(makeStore(repoPath) as never) + + // The CLI's shape, and the live data point: `path:` already resolves it. + const byPath = await runtime.showManagedWorktree(`path:${storedPath}`) + // The only shape the renderer can send must resolve the SAME workspace. + await expect( + runtime.showManagedWorktree(`id:${REPO_ID}::${storedPath}`) + ).resolves.toMatchObject({ id: byPath.id, path: byPath.path }) + } + ) + + it.each(ID_SPELLINGS)( + 'resolves a host-qualified removal target when the id carries %s', + async (_label, storedPath, repoPath) => { + scanReports(scannedSpellingOf(storedPath), repoPath) + const runtime = new OrcaRuntimeService(makeStore(repoPath) as never) + const internals = runtime as unknown as RemovalInternals + + // The scoped path the UI's delete takes must agree with the fleet path above. + await expect( + internals.resolveWorktreeRemovalTarget( + `id:${REPO_ID}::${storedPath}`, + LOCAL_EXECUTION_HOST_ID + ) + ).resolves.toMatchObject({ repoId: REPO_ID, path: scannedSpellingOf(storedPath) }) + } + ) + + it('still refuses an id whose path names a different workspace', async () => { + scanReports(WORKTREE_PATH) + const runtime = new OrcaRuntimeService(makeStore() as never) + + await expect( + runtime.showManagedWorktree(`id:${REPO_ID}::/srv/projects/workspaces/other-plugin`) + ).rejects.toThrow('selector_not_found') + }) + + // STA-4343: matching across repo ids would delete a workspace the caller never confirmed. + it('still refuses the same path under a different repo id', async () => { + scanReports(WORKTREE_PATH) + const runtime = new OrcaRuntimeService(makeStore() as never) + + await expect(runtime.showManagedWorktree(`id:other-repo::${WORKTREE_PATH}/`)).rejects.toThrow( + 'selector_not_found' + ) + }) + + it('still refuses a removal qualified to a host that does not own the repo id', async () => { + scanReports(WORKTREE_PATH) + const runtime = new OrcaRuntimeService(makeStore() as never) + const internals = runtime as unknown as RemovalInternals + + await expect( + internals.resolveWorktreeRemovalTarget(`id:${CANONICAL_ID}/`, 'runtime:env-b') + ).rejects.toThrow('selector_not_found') + }) + + it('keeps `id:` and `path:` agreeing on a dot segment neither canonicalizes', async () => { + scanReports(WORKTREE_PATH) + const runtime = new OrcaRuntimeService(makeStore() as never) + const dotted = '/srv/projects/./workspaces/plugin-host' + + await expect(runtime.showManagedWorktree(`path:${dotted}`)).rejects.toThrow( + 'selector_not_found' + ) + await expect(runtime.showManagedWorktree(`id:${REPO_ID}::${dotted}`)).rejects.toThrow( + 'selector_not_found' + ) + }) + + // #15598/#15616: on Windows one checkout is recorded under both spellings, and git reports the + // forward-slash one. The backslash id itself rides the ID_SPELLINGS rows above; these pin the + // folding limits around it. + describe('Windows drive-letter spellings', () => { + const WINDOWS_REPO = 'D:/Agentic/game2' + const WINDOWS_WORKTREE = 'D:/Agentic/game2/battle-core' + + it('folds drive-letter case only for Windows paths, never for a POSIX path', async () => { + scanReports(WINDOWS_WORKTREE, WINDOWS_REPO) + const windowsRuntime = new OrcaRuntimeService(makeStore(WINDOWS_REPO) as never) + + // A Windows root is case-insensitive, as `path:` already treats it. + await expect( + windowsRuntime.showManagedWorktree(`id:${REPO_ID}::d:/agentic/game2/battle-core`) + ).resolves.toMatchObject({ path: WINDOWS_WORKTREE }) + + // A POSIX root is not: folding case there would merge distinct directories for a delete. + scanReports(WORKTREE_PATH) + const posixRuntime = new OrcaRuntimeService(makeStore() as never) + + await expect( + posixRuntime.showManagedWorktree(`id:${REPO_ID}::/SRV/projects/workspaces/plugin-host`) + ).rejects.toThrow('selector_not_found') + }) + + it('does not fold a backslash inside a POSIX path, where it is a valid filename character', async () => { + scanReports(WORKTREE_PATH) + const runtime = new OrcaRuntimeService(makeStore() as never) + + await expect( + runtime.showManagedWorktree(`id:${REPO_ID}::/srv/projects\\workspaces\\plugin-host`) + ).rejects.toThrow('selector_not_found') + }) + }) + + // The fail-closed guard on a delete-capable resolver: two rows spelling one directory must not + // let a folded id pick one. `path:` collapses same-host duplicates; `id:` deliberately refuses. + it('refuses a folded id when two same-repo rows spell one directory', async () => { + listWorktreesStrictMock.mockResolvedValue([ + { path: REPO_PATH, head: 'abc', branch: 'main', isBare: false, isMainWorktree: true }, + { path: WORKTREE_PATH, head: 'def', branch: 'feature', isBare: false, isMainWorktree: false }, + { + path: '/srv/projects//workspaces/plugin-host', + head: 'ghi', + branch: 'feature-2', + isBare: false, + isMainWorktree: false + } + ]) + const runtime = new OrcaRuntimeService(makeStore() as never) + + // Matches neither row exactly, folds to both. + await expect(runtime.showManagedWorktree(`id:${CANONICAL_ID}/`)).rejects.toThrow( + 'selector_ambiguous' + ) + }) + + // Live-proof limit, pinned so nobody "fixes" the trimming: a folder-workspace id only trims a + // trailing slash at end of string, so one placed before the instance suffix stays exact-only. + it('keeps a folder-workspace id exact when a slash precedes the instance suffix', async () => { + const instanceSuffix = '::workspace:123e4567-e89b-12d3-a456-426614174000' + scanReports(WORKTREE_PATH) + const runtime = new OrcaRuntimeService(makeStore() as never) + + await expect( + runtime.showManagedWorktree(`id:${REPO_ID}::${WORKTREE_PATH}/${instanceSuffix}`) + ).rejects.toThrow('selector_not_found') + }) + + // #15616 guarantees malformed ids keep exact-match behavior; both sites must honour that too. + it.each([ + ['no repo boundary', 'not-an-id'], + ['an empty path', `${REPO_ID}::`] + ])('keeps exact matching for a malformed id with %s', async (_label, malformedId) => { + scanReports(WORKTREE_PATH) + const runtime = new OrcaRuntimeService(makeStore() as never) + const internals = runtime as unknown as RemovalInternals + + await expect(runtime.showManagedWorktree(`id:${malformedId}`)).rejects.toThrow( + 'selector_not_found' + ) + await expect( + internals.resolveWorktreeRemovalTarget(`id:${malformedId}`, LOCAL_EXECUTION_HOST_ID) + ).rejects.toThrow('selector_not_found') + }) +}) diff --git a/src/shared/worktree/id.test.ts b/src/shared/worktree/id.test.ts index e64d27784f5..726e53abe5a 100644 --- a/src/shared/worktree/id.test.ts +++ b/src/shared/worktree/id.test.ts @@ -4,7 +4,8 @@ import { getRepoIdFromWorktreeId, getWorktreePathBasenameFromId, splitWorktreeId, - splitWorktreeIdForFilesystem + splitWorktreeIdForFilesystem, + worktreeIdComparisonKey } from './id' describe('WORKTREE_ID_SEPARATOR', () => { @@ -119,3 +120,65 @@ describe('getWorktreePathBasenameFromId', () => { expect(getWorktreePathBasenameFromId('repo-123::')).toBeNull() }) }) + +/** + * #16243: the renderer can only address a workspace by `id:::`, so the key must fold + * exactly the path spellings a `path:` selector already folds — and nothing more. + */ +describe('worktreeIdComparisonKey path-spelling parity for id: selectors (#16243)', () => { + const canonical = 'repo-123::/srv/workspaces/plugin' + const key = (worktreeId: string): string | null => worktreeIdComparisonKey(worktreeId) + + it('folds the path spellings a `path:` selector already accepts', () => { + expect(key('repo-123::/srv/workspaces/plugin/')).toBe(key(canonical)) + expect(key('repo-123::/srv//workspaces/plugin')).toBe(key(canonical)) + expect(key('repo-123::/srv/workspaces/Café'.normalize('NFD'))).toBe( + key('repo-123::/srv/workspaces/Café'.normalize('NFC')) + ) + }) + + it('folds no more loosely than `path:` does', () => { + // A leading `//` is a UNC root, not a doubled separator. + expect(key('repo-123://srv/workspaces/plugin')).not.toBe(key(canonical)) + // Dot segments are not canonicalized, so `id:` and `path:` still agree on refusing them. + expect(key('repo-123::/srv/./workspaces/plugin')).not.toBe(key(canonical)) + }) + + // #15598/#15616: the same Windows checkout is recorded with both separators. + it('folds Windows separator and drive-letter case, as `path:` already does', () => { + const windows = 'repo-123::D:/Agentic/game2' + expect(key('repo-123::D:\\Agentic\\game2')).toBe(key(windows)) + expect(key('repo-123::d:/agentic/game2')).toBe(key(windows)) + // A backslash is a valid POSIX filename character, so a POSIX path never folds it. + expect(key('repo-123::/srv\\workspaces')).not.toBe(key('repo-123::/srv/workspaces')) + // POSIX roots stay case-sensitive. + expect(key('repo-123::/srv/Workspaces')).not.toBe(key('repo-123::/srv/workspaces')) + }) + + it('keeps a UNC or WSL root distinct from a drive-letter location', () => { + // Different roots naming different filesystems must never collapse into one key. + expect(key('repo-123://server/share/game2')).not.toBe(key('repo-123::D:/server/share/game2')) + expect(key('repo-123://wsl.localhost/Ubuntu/home/dev/game2')).not.toBe( + key('repo-123::D:/home/dev/game2') + ) + // The two UNC aliases Windows exposes for one WSL distro are the same location. + expect(key('repo-123://wsl.localhost/Ubuntu/home/dev/game2')).toBe( + key('repo-123://wsl$/Ubuntu/home/dev/game2') + ) + }) + + it('never merges different repos, workspaces, or folder sessions', () => { + // STA-4343: the repo id stays exact, or a removal lands on a repo the caller never confirmed. + expect(key('repo-999::/srv/workspaces/plugin')).not.toBe(key(canonical)) + expect(key('repo-123::/srv/workspaces/other')).not.toBe(key(canonical)) + expect(key('repo-a::/srv/folder::workspace:123e4567-e89b-12d3-a456-426614174000')).not.toBe( + key('repo-a::/srv/folder::workspace:123e4567-e89b-12d3-a456-426614174001') + ) + }) + + it('returns null for ids with no repo boundary so callers keep exact matching', () => { + expect(key('repo-a')).toBeNull() + expect(key('repo-a::')).toBeNull() + expect(key('/srv/workspaces/plugin')).toBeNull() + }) +}) diff --git a/src/shared/worktree/id.ts b/src/shared/worktree/id.ts index d71cbac66eb..478ac6bfa76 100644 --- a/src/shared/worktree/id.ts +++ b/src/shared/worktree/id.ts @@ -1,3 +1,4 @@ +import { normalizeRuntimePathForComparison } from '../cross-platform-path' import { WORKTREE_ID_SEPARATOR } from '../pty-session-id-format' export { WORKTREE_ID_SEPARATOR } from '../pty-session-id-format' @@ -17,6 +18,22 @@ export function getRepoIdFromWorktreeId(worktreeId: string): string { return separatorIdx === -1 ? worktreeId : worktreeId.slice(0, separatorIdx) } +/** + * Canonical comparison form of a worktree id: the repoId is compared EXACT and only the path folds, + * through the same `normalizeRuntimePathForComparison` a `path:` selector has always applied and + * byte-exact id matching denied the renderer (#16243). Null for a malformed id, so callers keep + * exact matching for it. Comparison only — never persist or return this key. + */ +export function worktreeIdComparisonKey(worktreeId: string): string | null { + const parsed = splitWorktreeId(worktreeId) + if (!parsed || !parsed.repoId || !parsed.worktreePath) { + return null + } + return `${parsed.repoId}${WORKTREE_ID_SEPARATOR}${normalizeRuntimePathForComparison( + parsed.worktreePath + )}` +} + export function splitWorktreeId(worktreeId: string): ParsedWorktreeId | null { const separatorIdx = worktreeId.indexOf(WORKTREE_ID_SEPARATOR) if (separatorIdx === -1) {