From 29dc77ea4f417f4665bfe3452f405976c3f21437 Mon Sep 17 00:00:00 2001 From: Orca Worker Date: Tue, 8 Sep 2026 00:37:29 -0700 Subject: [PATCH] Resolve explicit review push owners before host inventory --- .../github/review-push-endpoint-real.test.ts | 3 +- .../ipc/worktree-review-push-owner.test.ts | 183 ++++++++++++++++++ .../ipc/worktree-review-push-target.test.ts | 3 +- src/main/ipc/worktree-review-push-target.ts | 53 +++-- 4 files changed, 228 insertions(+), 14 deletions(-) create mode 100644 src/main/ipc/worktree-review-push-owner.test.ts diff --git a/src/main/github/review-push-endpoint-real.test.ts b/src/main/github/review-push-endpoint-real.test.ts index 5881a4f4d1f..52342f0cd55 100644 --- a/src/main/github/review-push-endpoint-real.test.ts +++ b/src/main/github/review-push-endpoint-real.test.ts @@ -299,7 +299,8 @@ it('carries provider-produced authority through omitted-target SSH IPC aliases t } const store = { getRepos: () => [{ id: 'repo', path: canonical, connectionId: 'identity-fixture' }], - getAllWorktreeMetaForHost: () => metadata + getAllWorktreeMetaForHost: () => metadata, + getWorktreeMetaForHost: (id: string) => metadata[id] } as unknown as Store const mux = createMockMux() const pushes: string[] = [] diff --git a/src/main/ipc/worktree-review-push-owner.test.ts b/src/main/ipc/worktree-review-push-owner.test.ts new file mode 100644 index 00000000000..6c9a15e72d4 --- /dev/null +++ b/src/main/ipc/worktree-review-push-owner.test.ts @@ -0,0 +1,183 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import { reviewTarget } from '../../shared/__fixtures__/git-review-target' +import type { Store } from '../persistence' +import type { WorktreeMeta } from '../../shared/worktree/meta-types' +import type { Repo } from '../../shared/repo-types' + +const { canonical, catalog, localAccess, options } = vi.hoisted(() => ({ + canonical: vi.fn(), + catalog: vi.fn(), + localAccess: vi.fn(), + options: vi.fn() +})) +vi.mock('../providers/ssh-filesystem-dispatch', () => ({ + requireSshFilesystemProvider: () => ({ realpath: canonical }) +})) +vi.mock('../local-worktree-filesystem', () => ({ getLocalWorktreePathAccess: localAccess })) +vi.mock('../repo-worktrees', () => ({ listRepoWorktreesForDetectedScan: catalog })) +vi.mock('./local-worktree-runtime-options', () => ({ getLocalGitOptionsForRepo: options })) +import { resolveReviewPushWorkspace } from './worktree-review-push-target' + +beforeEach(() => { + canonical + .mockReset() + .mockImplementation(async (path: string) => (path === '/alias' ? '/good/wt' : path)) + catalog.mockReset().mockResolvedValue([]) + localAccess.mockReset().mockImplementation(() => ({ realpath: canonical })) + options.mockReset().mockReturnValue({}) +}) +function fixture(connectionId?: string) { + const entries: Record> = { + 'good::/good/wt': { pushTarget: reviewTarget('origin', 'feature') } + } + const repos = [{ id: 'good', path: '/good', connectionId }] as Repo[] + const all = vi.fn(() => entries) + const single = vi.fn((id: string) => entries[id]) + const store = { + getRepos: () => repos, + getAllWorktreeMetaForHost: all, + getWorktreeMetaForHost: single + } as unknown as Store + const args = { worktreePath: '/alias', worktreeId: 'good::/good/wt', connectionId } + return { entries, repos, store, args, all, single } +} +for (const connectionId of [undefined, 'ssh-fixture']) { + for (const stale of [ + { linkedPR: 42 }, + { linkedGitLabMR: 42 }, + { pushTarget: reviewTarget('origin', 'stale') } + ]) { + it(`ignores unrelated removed policy ${JSON.stringify(stale)} on ${connectionId ?? 'local'}`, async () => { + const { entries, store, args, all, single } = fixture(connectionId) + entries['good::/removed'] = stale + canonical.mockImplementation(async (path: string) => { + if (path === '/removed') { + throw Object.assign(new Error('missing'), { code: 'ENOENT' }) + } + return '/good/wt' + }) + await expect(resolveReviewPushWorkspace(store, args)).resolves.toMatchObject({ + pushTarget: entries[args.worktreeId].pushTarget + }) + expect(canonical.mock.calls.map(([path]) => path)).toEqual(['/good/wt', '/alias']) + expect(catalog).not.toHaveBeenCalled() + expect(all).not.toHaveBeenCalled() + expect(single.mock.calls.every(([id]) => id === args.worktreeId)).toBe(true) + }) + } + it(`does not depend on unrelated catalog availability on ${connectionId ?? 'local'}`, async () => { + const { store, repos, args } = fixture(connectionId) + repos.push({ id: 'unrelated', path: '/unmounted', connectionId } as Repo) + catalog.mockRejectedValue(new Error('unrelated catalog unavailable')) + await expect(resolveReviewPushWorkspace(store, args)).resolves.toMatchObject({ + worktreePath: '/good/wt' + }) + expect(catalog).not.toHaveBeenCalled() + }) + it(`keeps requested unavailable owner unverifiable on ${connectionId ?? 'local'}`, async () => { + const { store, args } = fixture(connectionId) + canonical.mockRejectedValue(new Error('owner host unavailable')) + await expect(resolveReviewPushWorkspace(store, args)).rejects.toThrow('owner host unavailable') + expect(catalog).not.toHaveBeenCalled() + }) + it(`corroborates unlinked registration only in requested catalog on ${connectionId ?? 'local'}`, async () => { + const { entries, store, repos, args } = fixture(connectionId) + delete entries[args.worktreeId] + repos.push({ id: 'unrelated', path: '/unmounted', connectionId } as Repo) + catalog.mockResolvedValue([{ path: '/good/wt' }, { path: '/removed' }]) + await expect(resolveReviewPushWorkspace(store, args)).resolves.toMatchObject({ + pushTarget: undefined + }) + expect(catalog).toHaveBeenCalledExactlyOnceWith(repos[0], {}) + expect(canonical).toHaveBeenCalledTimes(2) + catalog.mockRejectedValue(new Error('owner catalog unavailable')) + await expect(resolveReviewPushWorkspace(store, args)).rejects.toThrow( + 'owner catalog unavailable' + ) + }) + for (const policy of [{ linkedPR: 42 }, { linkedGitLabMR: 42 }]) { + it(`rereads requested ${JSON.stringify(policy)} after realpath on ${connectionId ?? 'local'}`, async () => { + const { entries, store, args } = fixture(connectionId) + canonical.mockImplementation(async () => { + entries[args.worktreeId] = policy + return '/good/wt' + }) + await expect(resolveReviewPushWorkspace(store, args)).rejects.toThrow('unresolved') + }) + } + it(`rejects stale explicit target and removed policy on ${connectionId ?? 'local'}`, async () => { + const { entries, store, args } = fixture(connectionId) + await expect( + resolveReviewPushWorkspace(store, { ...args, pushTarget: reviewTarget('other', 'feature') }) + ).rejects.toThrow('changed') + canonical.mockImplementation(async () => { + delete entries[args.worktreeId] + return '/good/wt' + }) + await expect(resolveReviewPushWorkspace(store, args)).rejects.toThrow('metadata changed') + }) +} +for (const dimension of ['repos', 'metadata']) { + it(`explicit owner avoids global ${dimension} admission bound`, async () => { + const { store, repos, entries, args, all } = fixture() + if (dimension === 'repos') { + for (let i = 0; i < 128; i++) { + repos.push({ id: `unrelated${i}`, path: `/unrelated${i}` } as Repo) + } + } else { + for (let i = 0; i < 512; i++) { + entries[`good::/removed${i}`] = { linkedPR: 42 } + } + } + await expect(resolveReviewPushWorkspace(store, args)).resolves.toMatchObject({ + worktreePath: '/good/wt' + }) + expect(catalog).not.toHaveBeenCalled() + expect(all).not.toHaveBeenCalled() + expect(canonical).toHaveBeenCalledTimes(2) + }) +} +it('does two owner realpaths and zero catalogs with ten unrelated repositories', async () => { + const { store, repos, args } = fixture('ssh-fixture') + for (let i = 0; i < 10; i++) { + repos.push({ id: `unrelated${i}`, path: `/unrelated${i}`, connectionId: 'ssh-fixture' } as Repo) + } + await resolveReviewPushWorkspace(store, args) + expect(canonical).toHaveBeenCalledTimes(2) + expect(catalog).not.toHaveBeenCalled() +}) +it('uses only the selected same-host WSL distro even when paths collide', async () => { + const { store, repos, args } = fixture() + repos.unshift({ id: 'other', path: '/good', connectionId: undefined } as Repo) + options.mockImplementation((_store, repo: Repo) => ({ + wslDistro: repo.id === 'good' ? 'Ubuntu' : 'Debian' + })) + localAccess.mockImplementation(({ wslDistro }) => { + if (wslDistro !== 'Ubuntu') { + throw new Error('unrelated distro unavailable') + } + return { realpath: canonical } + }) + await expect(resolveReviewPushWorkspace(store, args)).resolves.toMatchObject({ + gitOptions: { wslDistro: 'Ubuntu' } + }) + expect(localAccess).toHaveBeenCalledExactlyOnceWith({ wslDistro: 'Ubuntu' }) +}) +it('rejects wrong-host owners and unregistered IDs before accepting their path', async () => { + const { store, repos, args, entries } = fixture() + repos[0].connectionId = 'elsewhere' + await expect(resolveReviewPushWorkspace(store, args)).rejects.toThrow('unverifiable') + expect(canonical).not.toHaveBeenCalled() + repos[0].connectionId = undefined + delete entries[args.worktreeId] + await expect(resolveReviewPushWorkspace(store, args)).rejects.toThrow('unverifiable') +}) +it('retains explicit folder instance identity without putting its suffix in realpath', async () => { + const { entries, store, args } = fixture() + const id = 'good::/good/wt::workspace:11111111-1111-1111-1111-111111111111' + entries[id] = entries[args.worktreeId] + await expect( + resolveReviewPushWorkspace(store, { ...args, worktreeId: id }) + ).resolves.toMatchObject({ worktreePath: '/good/wt' }) + expect(canonical.mock.calls.map(([path]) => path)).toEqual(['/good/wt', '/alias']) +}) diff --git a/src/main/ipc/worktree-review-push-target.test.ts b/src/main/ipc/worktree-review-push-target.test.ts index 3df179cb97b..42e8e33fc3f 100644 --- a/src/main/ipc/worktree-review-push-target.test.ts +++ b/src/main/ipc/worktree-review-push-target.test.ts @@ -30,7 +30,8 @@ function fixture(meta: Partial | undefined, connectionId?: string) const entries: Record> = meta ? { 'repo::/repo/wt': meta } : {} const store = { getRepos: () => [{ id: 'repo', path: '/repo', connectionId }], - getAllWorktreeMetaForHost: vi.fn(() => entries) + getAllWorktreeMetaForHost: vi.fn(() => entries), + getWorktreeMetaForHost: vi.fn((id: string) => entries[id]) } as unknown as Store return { store, entries } } diff --git a/src/main/ipc/worktree-review-push-target.ts b/src/main/ipc/worktree-review-push-target.ts index 2c29d3b2241..9ab48f4f7d0 100644 --- a/src/main/ipc/worktree-review-push-target.ts +++ b/src/main/ipc/worktree-review-push-target.ts @@ -7,7 +7,10 @@ import { splitWorktreeIdForFilesystem } from '../../shared/worktree/id' import { linkedReviewOperationTarget } from '../../shared/linked-review-operation-target' import type { GitPushTarget } from '../../shared/worktree/types' import type { WorktreeMeta } from '../../shared/worktree/meta-types' -import { readAllWorktreeMetaForHost } from '../persistence/host-qualified-worktree-meta' +import { + readAllWorktreeMetaForHost, + readWorktreeMetaForHost +} from '../persistence/host-qualified-worktree-meta' import { requireSshFilesystemProvider } from '../providers/ssh-filesystem-dispatch' import { getLocalWorktreePathAccess } from '../local-worktree-filesystem' import { listRepoWorktreesForDetectedScan } from '../repo-worktrees' @@ -30,18 +33,46 @@ export async function resolveReviewPushWorkspace( }> { const host = args.connectionId ? toSshExecutionHostId(args.connectionId) : LOCAL_EXECUTION_HOST_ID const repos = store.getRepos().filter((repo) => getRepoExecutionHostId(repo) === host) - const metadata = Object.entries(readAllWorktreeMetaForHost(store, host)) - if ( - !args.worktreePath || - args.worktreePath.includes('\0') || - repos.length > 128 || - metadata.length > 512 - ) { + if (!args.worktreePath || args.worktreePath.includes('\0')) { throw new Error('Review push workspace identity is unverifiable.') } const remoteFilesystem = args.connectionId ? requireSshFilesystemProvider(args.connectionId) : null + if (args.worktreeId) { + const parsed = splitWorktreeIdForFilesystem(args.worktreeId) + const repo = repos.find((candidate) => candidate.id === parsed?.repoId) + if (!repo || !parsed?.worktreePath || parsed.worktreePath.includes('\0')) { + throw new Error('Review push workspace identity is unverifiable.') + } + const gitOptions = remoteFilesystem ? {} : getLocalGitOptionsForRepo(store, repo) + const filesystem = remoteFilesystem ?? getLocalWorktreePathAccess(gitOptions) + const initialMeta = readWorktreeMetaForHost(store, args.worktreeId, host) + const path = await filesystem.realpath(parsed.worktreePath) + if (path !== (await filesystem.realpath(args.worktreePath))) { + throw new Error('Review push workspace identity is unverifiable.') + } + if (!initialMeta) { + // Only this owner's catalog can establish an unlinked workspace. + const rows = await listRepoWorktreesForDetectedScan(repo, gitOptions) + if (!rows.some((row) => `${repo.id}::${row.path}` === args.worktreeId)) { + throw new Error('Review push workspace identity is unverifiable.') + } + } + const currentMeta = readWorktreeMetaForHost(store, args.worktreeId, host) + if (initialMeta && !currentMeta) { + throw new Error('Review push workspace metadata changed during identity resolution.') + } + return { + worktreePath: path, + gitOptions, + pushTarget: linkedReviewOperationTarget(currentMeta, args.pushTarget) + } + } + const metadata = Object.entries(readAllWorktreeMetaForHost(store, host)) + if (repos.length > 128 || metadata.length > 512) { + throw new Error('Review push workspace identity is unverifiable.') + } const matches: { id: string path: string @@ -79,16 +110,14 @@ export async function resolveReviewPushWorkspace( } } } - const selected = args.worktreeId - ? matches.filter((match) => match.id === args.worktreeId) - : matches + const selected = matches if (selected.length !== 1) { throw new Error( `Review push workspace identity is ${selected.length ? 'ambiguous' : 'unverifiable'}.` ) } const owner = selected[0]! - const currentMeta = readAllWorktreeMetaForHost(store, host)[owner.id] + const currentMeta = readWorktreeMetaForHost(store, owner.id, host) if (owner.meta && !currentMeta) { throw new Error('Review push workspace metadata changed during identity resolution.') }