Resolve explicit review push owners before host inventory

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[] = []

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<string, Partial<WorktreeMeta>> = {
+    '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'])
+})

src/main/ipc/worktree-review-push-target.test.ts

@@ -30,7 +30,8 @@ function fixture(meta: Partial<WorktreeMeta> | undefined, connectionId?: string)
   const entries: Record<string, Partial<WorktreeMeta>> = 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 }
 }

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.')
   }