diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 5f117c024b2..a90bd45d91e 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -415,6 +415,8 @@ jobs: - uses: ./.github/actions/prepare-git-compatibility - name: Verify Git binary compatibility matrix + env: + ORCA_BACKGROUND_LAUNCH: '1' run: | specs=( "alpine/git:edge-2.38.1|2.38.1" @@ -433,7 +435,9 @@ jobs: ORCA_GIT_COMPAT_VERSION="2.25.5" \ pnpm exec vitest run --config config/vitest.config.ts \ src/shared/git-binary-compatibility.test.ts \ - src/main/git/worktree-safety-real-git.test.ts + src/main/git/worktree-safety-real-git.test.ts \ + src/main/git/worktree-rebase-update-refs-real-git.test.ts \ + src/relay/git-review-draft-binary-compatibility.test.ts ) & pids+=("$!") @@ -444,7 +448,9 @@ jobs: ORCA_GIT_COMPAT_IMAGE="$image" ORCA_GIT_COMPAT_VERSION="$version" \ pnpm exec vitest run --config config/vitest.config.ts \ src/shared/git-binary-compatibility.test.ts \ - src/main/git/worktree-safety-real-git.test.ts + src/main/git/worktree-safety-real-git.test.ts \ + src/main/git/worktree-rebase-update-refs-real-git.test.ts \ + src/relay/git-review-draft-binary-compatibility.test.ts ) & pids+=("$!") done diff --git a/config/scripts/git-binary-compatibility-workflow.test.mjs b/config/scripts/git-binary-compatibility-workflow.test.mjs index 6acd6c4df50..f6e6a32b720 100644 --- a/config/scripts/git-binary-compatibility-workflow.test.mjs +++ b/config/scripts/git-binary-compatibility-workflow.test.mjs @@ -22,6 +22,8 @@ describe('Git binary compatibility PR gate', () => { expect(run).toContain('ORCA_GIT_COMPAT_IMAGE="$image"') expect(run).toContain('src/shared/git-binary-compatibility.test.ts') expect(run).toContain('src/main/git/worktree-safety-real-git.test.ts') + expect(run).toContain('src/main/git/worktree-rebase-update-refs-real-git.test.ts') + expect(run).toContain('src/relay/git-review-draft-binary-compatibility.test.ts') expect(run).toContain('pids+=("$!")') expect(run).toContain('wait "$pid" || status=1') }) diff --git a/config/scripts/runtime-serve-terminal-smoke.mjs b/config/scripts/runtime-serve-terminal-smoke.mjs index c1d998717ff..2b23115859c 100644 --- a/config/scripts/runtime-serve-terminal-smoke.mjs +++ b/config/scripts/runtime-serve-terminal-smoke.mjs @@ -198,7 +198,7 @@ function resolveLaunch(userDataDir) { label: `electron (${serveEntry})`, command: override ?? 'npx', args: override ? serveArgs : ['electron', ...serveArgs], - env: {} + env: { ORCA_DEV_USER_DATA_PATH: userDataDir } } } diff --git a/src/cli/handlers/search.test.ts b/src/cli/handlers/search.test.ts index 971fccaf3a2..c65a96ebe5c 100644 --- a/src/cli/handlers/search.test.ts +++ b/src/cli/handlers/search.test.ts @@ -4,6 +4,7 @@ import { AiVaultSearchResponseSchema } from '../../shared/ai-vault-search-contra import type { AiVaultSearchResponse, AiVaultSearchStatus } from '../../shared/ai-vault-search-types' import { REPEATED_FLAG_SEPARATOR } from '../args' import { RuntimeClientError } from '../runtime/types' +import { AI_VAULT_AGENTS } from '../../shared/ai-vault-types' afterEach(() => vi.restoreAllMocks()) @@ -105,6 +106,7 @@ describe('orca search over the runtime RPC', () => { expect(call).toHaveBeenCalledWith('aiVault.searchSessions', { query: 'resize race', limit: 20, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) @@ -164,6 +166,7 @@ describe('orca search over the runtime RPC', () => { expect(call).toHaveBeenCalledWith('aiVault.searchSessions', { ...params, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) diff --git a/src/main/ai-vault-search/session-search-agent-compatibility.test.ts b/src/main/ai-vault-search/session-search-agent-compatibility.test.ts new file mode 100644 index 00000000000..a8c1ba96e75 --- /dev/null +++ b/src/main/ai-vault-search/session-search-agent-compatibility.test.ts @@ -0,0 +1,194 @@ +import { afterEach, expect, test } from 'vitest' +import { AI_VAULT_AGENTS } from '../../shared/ai-vault-types' +import { + fakeSearchService, + searchHit, + searchResults +} from '../../shared/ai-vault-search-test-fixture' +import { + searchSessionService, + setSessionSearchService, + sessionSearchServiceStatus +} from './session-search-service-registry' +import { + encodeSessionSearchCursor, + decodeSessionSearchCursor, + sessionSearchPageKey, + SessionSearchCursorError +} from './session-search-page-cursor' + +afterEach(() => setSessionSearchService(null)) + +test.each(['runtime', 'relay'] as const)( + 'publishes the current supported catalog independently of indexed data on %s', + async (transport) => { + expect(await sessionSearchServiceStatus({}, transport)).toMatchObject({ + enabled: false, + supportedAgents: [...AI_VAULT_AGENTS], + supportsQoderHistory: true, + supportsJcodeHistory: true + }) + const service = fakeSearchService() + setSessionSearchService(service) + expect((await sessionSearchServiceStatus({}, transport)).supportedAgents).toEqual( + AI_VAULT_AGENTS + ) + expect(service.status.mock.lastCall).toEqual([]) + } +) + +test.each(['runtime', 'relay'] as const)( + 'keeps an empty decoder catalog authoritative over both history flags on %s', + async (transport) => { + const service = fakeSearchService() + setSessionSearchService(service) + expect( + await searchSessionService( + { + query: 'proof', + supportedAgents: [], + supportsQoderHistory: true, + supportsJcodeHistory: true, + freshness: 'wait-until-current' + }, + transport + ) + ).toEqual({ kind: 'unavailable', reason: 'unsupported-agent' }) + expect(service.search).not.toHaveBeenCalled() + expect(service.reconcile).not.toHaveBeenCalled() + } +) + +test.each(['qoder', 'jcode'] as const)( + 'projects the independent %s history capability before retrieval', + async (agent) => { + const service = fakeSearchService() + setSessionSearchService(service) + const supportField = agent === 'qoder' ? 'supportsQoderHistory' : 'supportsJcodeHistory' + await searchSessionService({ query: 'proof', [supportField]: true }, 'relay') + expect(service.search).toHaveBeenCalledExactlyOnceWith( + { + query: 'proof', + limit: 20, + filters: { + agents: AI_VAULT_AGENTS.filter((candidate) => + agent === 'qoder' + ? candidate !== 'jcode' + : !['codebuddy', 'zcode', 'qoder'].includes(candidate) + ) + } + }, + undefined + ) + } +) + +test.each(['runtime', 'relay'] as const)( + 'does not query or reconcile an unsupported-only filter on %s', + async (transport) => { + const request = { + query: 'proof', + supportedAgents: ['codex'], + filters: { agents: ['jcode'] as const }, + freshness: 'wait-until-current' as const + } + expect( + await searchSessionService( + { ...request, filters: { agents: [...request.filters.agents] } }, + transport + ) + ).toEqual({ kind: 'unavailable', reason: 'no-service' }) + const service = fakeSearchService() + setSessionSearchService(service) + expect( + await searchSessionService( + { ...request, filters: { agents: [...request.filters.agents] } }, + transport + ) + ).toEqual({ kind: 'unavailable', reason: 'unsupported-agent' }) + expect(service.search).not.toHaveBeenCalled() + expect(service.status).not.toHaveBeenCalled() + expect(service.reconcile).not.toHaveBeenCalled() + for (const reason of ['disabled', 'not-ready'] as const) { + service.search.mockResolvedValue({ kind: 'unavailable', reason }) + expect( + await searchSessionService({ query: 'proof', filters: { agents: ['codex'] } }, transport) + ).toEqual({ kind: 'unavailable', reason }) + } + } +) + +test('projects agents before cursor identity and preserves every other ranking field', async () => { + const service = fakeSearchService() + service.search.mockImplementation(async (request) => { + const key = sessionSearchPageKey(request) + if (request.cursor) { + try { + decodeSessionSearchCursor(request.cursor, 7, key, 'test-index') + } catch (error) { + if (error instanceof SessionSearchCursorError) { + return { kind: 'malformed-cursor' } + } + throw error + } + } + return { + ...searchResults(), + page: { cursor: encodeSessionSearchCursor(7, 1, key, 'test-index'), hasMore: true } + } + }) + setSessionSearchService(service) + const request = { + query: 'proof', + supportedAgents: ['codex'], + scope: 'conversation' as const, + limit: 42, + debug: true, + filters: { + agents: ['codex', 'jcode'] as const, + scopePaths: ['/host/folder'], + since: '2026-08-01T00:00:00Z', + sort: 'newest' as const + } + } + const first = await searchSessionService( + { ...request, filters: { ...request.filters, agents: [...request.filters.agents] } }, + 'relay' + ) + if (first.kind !== 'results' || !first.page.cursor) { + throw new Error('Expected a first page with a cursor') + } + const next = { + ...request, + filters: { ...request.filters, agents: [...request.filters.agents] }, + cursor: first.page.cursor + } + expect(await searchSessionService(next, 'relay')).toMatchObject({ kind: 'results' }) + const { supportedAgents: _supportedAgents, ...expected } = next + expect(service.search).toHaveBeenLastCalledWith( + { ...expected, filters: { ...next.filters, agents: ['codex'] } }, + undefined + ) + expect( + await searchSessionService({ ...next, supportedAgents: [...AI_VAULT_AGENTS] }, 'relay') + ).toEqual({ kind: 'malformed-cursor' }) +}) + +test('keeps current Jcode hits and removes unrequested agents before publication', async () => { + const service = fakeSearchService() + service.search.mockResolvedValue({ + ...searchResults(), + hits: [{ ...searchHit(), agent: 'jcode' }, searchHit()] + }) + setSessionSearchService(service) + expect( + await searchSessionService( + { query: 'proof', supportedAgents: [...AI_VAULT_AGENTS], filters: { agents: ['jcode'] } }, + 'relay' + ) + ).toMatchObject({ hits: [{ agent: 'jcode' }] }) + expect(service.search).toHaveBeenCalledExactlyOnceWith( + { query: 'proof', limit: 20, filters: { agents: ['jcode'] } }, + undefined + ) +}) diff --git a/src/main/ai-vault-search/session-search-scope-entry-points.test.ts b/src/main/ai-vault-search/session-search-scope-entry-points.test.ts index 854211d3367..0eefb2fd498 100644 --- a/src/main/ai-vault-search/session-search-scope-entry-points.test.ts +++ b/src/main/ai-vault-search/session-search-scope-entry-points.test.ts @@ -3,6 +3,7 @@ import { AiVaultHandler } from '../../relay/ai-vault-handler' import type { RelayDispatcher } from '../../relay/dispatcher' import { createSessionSearchClient } from '../../shared/ai-vault-search-client' import { fakeSearchService } from '../../shared/ai-vault-search-test-fixture' +import { AI_VAULT_AGENTS } from '../../shared/ai-vault-types' import { RpcDispatcher } from '../runtime/rpc/dispatcher' import { OrcaRuntimeService } from '../runtime/orca-runtime' import { AI_VAULT_METHODS } from '../runtime/rpc/methods/ai-vault' @@ -80,6 +81,7 @@ describe('every search entry point carries the scope identity through', () => { installSessionSearchScopeCatalogSource(() => CATALOG) await relayHandler()({ query: 'needle', + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true, within: WITHIN @@ -95,6 +97,7 @@ describe('every search entry point carries the scope identity through', () => { setSessionSearchService(service) await relayHandler()({ query: 'needle', + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true, within: WITHIN diff --git a/src/main/ai-vault-search/session-search-service-registry.test.ts b/src/main/ai-vault-search/session-search-service-registry.test.ts index ff5de45c4fd..d8fb2c081bc 100644 --- a/src/main/ai-vault-search/session-search-service-registry.test.ts +++ b/src/main/ai-vault-search/session-search-service-registry.test.ts @@ -58,8 +58,7 @@ describe('session search service registry', () => { expect(service.search).toHaveBeenCalledWith( { query: 'needle', - limit: 20, - filters: { agents: AI_VAULT_AGENTS.filter((a) => a !== 'qoder' && a !== 'jcode') } + limit: 20 }, undefined ) @@ -69,13 +68,13 @@ describe('session search service registry', () => { ) }) describe.each(['qoder', 'jcode'] as const)('%s history compatibility', (agent) => { - it.each(['ipc', 'runtime', 'relay'] as const)( + it.each(['runtime', 'relay'] as const)( 'negotiates hits before retrieval on %s, without widening explicit filters', async (transport) => { const service = fakeSearchService() setSessionSearchService(service) await searchSessionService( - { query: 'proof', filters: { agents: ['claude', agent] } }, + { query: 'proof', supportedAgents: ['claude'], filters: { agents: ['claude', agent] } }, transport ) expect(service.search).toHaveBeenLastCalledWith( @@ -83,8 +82,13 @@ describe('session search service registry', () => { undefined ) expect( - await searchSessionService({ query: 'proof', filters: { agents: [agent] } }, transport) - ).toMatchObject({ kind: 'results', hits: [], page: { cursor: null, hasMore: false } }) + await searchSessionService( + { query: 'proof', supportedAgents: ['claude'], filters: { agents: [agent] } }, + transport + ) + ).toEqual({ kind: 'unavailable', reason: 'unsupported-agent' }) + expect(service.search).toHaveBeenCalledTimes(1) + expect(service.reconcile).not.toHaveBeenCalled() const client = createSessionSearchClient( (method, request) => method === 'aiVault.searchStatus' @@ -100,6 +104,16 @@ describe('session search service registry', () => { } ) }) + it('keeps the current catalog for same-build direct IPC calls', async () => { + const service = fakeSearchService() + setSessionSearchService(service) + await searchSessionService({ query: 'proof', filters: { agents: ['qoder', 'jcode'] } }, 'ipc') + expect(service.search).toHaveBeenCalledExactlyOnceWith( + { query: 'proof', limit: 20, filters: { agents: ['qoder', 'jcode'] } }, + undefined + ) + expect((await sessionSearchServiceStatus({}, 'ipc')).supportedAgents).toEqual(AI_VAULT_AGENTS) + }) it('waits for reconcile before search, and clears its timeout', async () => { vi.useFakeTimers() const service = fakeSearchService() diff --git a/src/main/ai-vault-search/session-search-service-registry.ts b/src/main/ai-vault-search/session-search-service-registry.ts index 9464668bd14..2f339185e64 100644 --- a/src/main/ai-vault-search/session-search-service-registry.ts +++ b/src/main/ai-vault-search/session-search-service-registry.ts @@ -15,6 +15,7 @@ import { } from '../../shared/ai-vault-search-transport' import type { SessionSearchService } from './session-search-service' import { AI_VAULT_AGENTS } from '../../shared/ai-vault-types' +import { compatibleSearchAgents } from '../../shared/ai-vault-search-agent-compatibility' let service: SessionSearchService | null = null @@ -35,23 +36,33 @@ export async function searchSessionService( // The choke point every entry point funnels through, so every host kind // resolves alike; the verdict goes to the service, which answers off and // not-ready first. - const { within, supportsQoderHistory, supportsJcodeHistory, ...request } = parsed + const { within, supportedAgents, supportsQoderHistory, supportsJcodeHistory, ...request } = parsed // Older clients reject the whole page when a hit has an unknown agent tag. const requestedAgents = request.filters?.agents - const compatibleAgents = (requestedAgents?.length ? requestedAgents : AI_VAULT_AGENTS).filter( - (agent) => - (supportsQoderHistory || agent !== 'qoder') && (supportsJcodeHistory || agent !== 'jcode') + const agents = requestedAgents?.length ? requestedAgents : AI_VAULT_AGENTS + const compatibleAgents = compatibleSearchAgents( + agents, + transport === 'ipc' + ? { supportedAgents: [...AI_VAULT_AGENTS] } + : { + // An explicit tag also proves the requesting parser understands that agent. + supportedAgents: + supportedAgents ?? (requestedAgents?.length ? requestedAgents : undefined), + supportsQoderHistory, + supportsJcodeHistory + } ) + if (compatibleAgents.length === 0) { + return { kind: 'unavailable', reason: 'unsupported-agent' } + } const compatibleRequest = - supportsQoderHistory && supportsJcodeHistory + compatibleAgents.length === agents.length ? request : { ...request, filters: { ...request.filters, - agents: compatibleAgents.length - ? compatibleAgents - : AI_VAULT_AGENTS.filter((agent) => agent !== 'qoder' && agent !== 'jcode') + agents: compatibleAgents } } const hostScope = within @@ -67,13 +78,12 @@ export async function searchSessionService( if (result.kind !== 'results') { return result } - if (compatibleAgents.length === 0) { - return { ...result, hits: [], page: { cursor: null, hasMore: false } } - } const { debug, ...fields } = result return { ...fields, - hits: result.hits.map((hit) => redactForTransport(hit, transport)), + hits: result.hits + .filter((hit) => compatibleAgents.includes(hit.agent)) + .map((hit) => redactForTransport(hit, transport)), truncated: { ...result.truncated, freshness: result.truncated.freshness || freshness }, ...(request.debug && debug ? { debug } : {}) } @@ -87,6 +97,7 @@ export async function sessionSearchServiceStatus( return redactStatusForTransport( AiVaultSearchStatusSchema.parse({ ...(service ? await service.status() : unavailableSessionSearchStatus()), + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }), diff --git a/src/main/git/command-runner/git-exec-file.ts b/src/main/git/command-runner/git-exec-file.ts index f581c807211..aadff165f9c 100644 --- a/src/main/git/command-runner/git-exec-file.ts +++ b/src/main/git/command-runner/git-exec-file.ts @@ -27,6 +27,7 @@ import { nonInteractiveGitEnv, untranslatedGitOutputEnv } from './git-process-en import { acquireGitAdmission } from './git-subprocess-admission' import { GitCommandTimeoutError, gitCommandTimeoutMs } from './git-command-timeout' import { classifyGitCommand } from '../../../shared/git-command-classification' +import { createAbortError } from './abort-error' /** * Async git command execution. Drop-in replacement for @@ -74,7 +75,7 @@ async function gitExecFileAsyncUnlocked( cwd: options.cwd, wslDistro: options.wslDistro, tier: options.admissionTier, - signal: options.signal + signal: options.admissionSignal ?? options.signal }) span?.setAttribute('git.queue_wait_ms', grant.queueWaitMs) const timeoutMs = gitCommandTimeoutMs(args, options.timeout, options.timeoutMsForTest) @@ -153,6 +154,10 @@ async function gitExecFileAsyncUnlocked( } } try { + options.admissionSignal?.throwIfAborted() + if (options.canStart?.() === false) { + throw createAbortError() + } return await runCapturedCommand() } finally { const termination = terminationState.current diff --git a/src/main/git/command-runner/git-exec-options.ts b/src/main/git/command-runner/git-exec-options.ts index 93ad47b6520..2aad9cfab7b 100644 --- a/src/main/git/command-runner/git-exec-options.ts +++ b/src/main/git/command-runner/git-exec-options.ts @@ -16,6 +16,10 @@ export type GitExecOptions = { stdin?: string env?: NodeJS.ProcessEnv signal?: AbortSignal + /** Cancels admission only; a running child still finishes. */ + admissionSignal?: AbortSignal + /** Rechecks owner eligibility after admission, before starting the child. */ + canStart?: () => boolean wslDistro?: string preferWslDirectGit?: boolean useConfiguredSshCommandForNetwork?: boolean diff --git a/src/main/git/local-repo-ref-maintenance-probe-retry.test.ts b/src/main/git/local-repo-ref-maintenance-probe-retry.test.ts new file mode 100644 index 00000000000..2c944b2f46b --- /dev/null +++ b/src/main/git/local-repo-ref-maintenance-probe-retry.test.ts @@ -0,0 +1,189 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GitRunner from './runner' +import { RepoRefMaintenance } from '../../shared/repo-ref-maintenance' +import { + REF_MAINTENANCE_ATTEMPT_DEADLINE_MS, + REF_MAINTENANCE_CLEAN_COOLDOWN_MS +} from '../../shared/repo-ref-maintenance-policy' + +const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) +vi.mock('./runner', async (importOriginal) => ({ + ...(await importOriginal()), + gitExecFileAsync: gitExecFileAsyncMock +})) + +import { createLocalRepoRefMaintenanceTarget } from './local-repo-ref-maintenance' + +const QUIET_MS = 1000 +const engines: RepoRefMaintenance[] = [] + +function createHarness() { + const spans: Record[] = [] + const onError = vi.fn() + const maintenance = new RepoRefMaintenance({ + quietPeriodMs: QUIET_MS, + onError, + observe: (attempt) => { + const attributes: Record = {} + spans.push(attributes) + return attempt({ + setAttribute: (key, value) => { + attributes[key] = value + } + }) + } + }) + engines.push(maintenance) + const maintainPackIndex = vi.fn(async () => {}) + const resolveRefsDirectory = vi.fn(async () => undefined) + const packRefs = vi.fn(async () => {}) + const target = { + ...createLocalRepoRefMaintenanceTarget({ key: 'local::/repo/.git', repoPath: '/repo' }), + maintainPackIndex, + resolveRefsDirectory, + packRefs + } + return { maintenance, target, spans, onError, maintainPackIndex, resolveRefsDirectory, packRefs } +} + +async function elapse(maintenance: RepoRefMaintenance, milliseconds: number) { + await vi.advanceTimersByTimeAsync(milliseconds) + await maintenance.whenAttemptSettled() +} + +beforeEach(() => { + vi.useFakeTimers() + gitExecFileAsyncMock.mockReset().mockResolvedValue({ stdout: '', stderr: '' }) +}) + +afterEach(() => { + for (const engine of engines.splice(0)) { + engine.dispose() + } + vi.useRealTimers() +}) + +describe('local maintenance config probe recovery', () => { + it('retries a transient config failure before admitting either maintenance phase', async () => { + const error = new Error('temporary Git spawn failure') + gitExecFileAsyncMock.mockRejectedValueOnce(error) + const h = createHarness() + h.maintenance.arm(h.target) + await elapse(h.maintenance, QUIET_MS) + expect(h.spans[0]).toMatchObject({ + 'repo.maintenance_outcome': 'failed', + 'repo.maintenance_error': String(error) + }) + expect(h.onError).toHaveBeenCalledExactlyOnceWith(error) + expect(h.maintainPackIndex).not.toHaveBeenCalled() + expect(h.resolveRefsDirectory).not.toHaveBeenCalled() + expect(h.packRefs).not.toHaveBeenCalled() + await elapse(h.maintenance, 2 * QUIET_MS - 1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + await elapse(h.maintenance, 1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(h.maintainPackIndex).toHaveBeenCalledTimes(1) + expect(h.resolveRefsDirectory).toHaveBeenCalledTimes(1) + expect(vi.getTimerCount()).toBe(0) + }) + + it('retains the clean cooldown only for an explicit user opt-out', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'maintenance.auto false\n', stderr: '' }) + const h = createHarness() + h.maintenance.arm(h.target) + await elapse(h.maintenance, QUIET_MS) + expect(h.spans[0]['repo.maintenance_outcome']).toBe('opted_out') + h.maintenance.arm(h.target) + await elapse(h.maintenance, REF_MAINTENANCE_CLEAN_COOLDOWN_MS - 1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + await elapse(h.maintenance, 1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(h.onError).not.toHaveBeenCalled() + expect(h.maintainPackIndex).not.toHaveBeenCalled() + expect(h.resolveRefsDirectory).not.toHaveBeenCalled() + expect(h.packRefs).not.toHaveBeenCalled() + }) + + it('caps a persistent config failure at seven probes and six backed-off retries', async () => { + gitExecFileAsyncMock.mockRejectedValue(new Error('invalid config')) + const h = createHarness() + h.maintenance.arm(h.target) + for (const [index, multiplier] of [1, 2, 4, 8, 8, 8, 8].entries()) { + await elapse(h.maintenance, QUIET_MS * multiplier - 1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(index) + await elapse(h.maintenance, 1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(index + 1) + } + await elapse(h.maintenance, REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(7) + expect(h.onError).toHaveBeenCalledTimes(7) + expect(h.spans.every((span) => span['repo.maintenance_outcome'] === 'failed')).toBe(true) + expect(h.maintainPackIndex).not.toHaveBeenCalled() + expect(h.resolveRefsDirectory).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + }) + + it('keeps a newer arm when a stale probe fails', async () => { + let rejectProbe: (error: Error) => void = () => {} + gitExecFileAsyncMock.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectProbe = reject + }) + ) + const h = createHarness() + h.maintenance.arm(h.target) + await vi.advanceTimersByTimeAsync(QUIET_MS) + const newIndex = vi.fn(async () => {}) + h.maintenance.arm({ ...h.target, maintainPackIndex: newIndex }) + rejectProbe(new Error('stale failure')) + await h.maintenance.whenAttemptSettled() + await elapse(h.maintenance, QUIET_MS) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(newIndex).toHaveBeenCalledOnce() + expect(h.maintainPackIndex).not.toHaveBeenCalled() + }) + + it.each(['failure', 'opt_out'])('does not re-arm a disposed probe after %s', async (result) => { + let settle: () => void = () => {} + gitExecFileAsyncMock.mockImplementationOnce( + () => + new Promise((resolve, reject) => { + settle = () => + result === 'failure' + ? reject(new Error('disposed probe')) + : resolve({ stdout: 'maintenance.auto false\n', stderr: '' }) + }) + ) + const h = createHarness() + h.maintenance.arm(h.target) + await vi.advanceTimersByTimeAsync(QUIET_MS) + h.maintenance.dispose() + settle() + await h.maintenance.whenAttemptSettled() + expect(h.spans[0]['repo.maintenance_outcome']).toBe('interrupted') + expect(vi.getTimerCount()).toBe(0) + expect(h.maintainPackIndex).not.toHaveBeenCalled() + expect(h.resolveRefsDirectory).not.toHaveBeenCalled() + }) + + it('treats a canceled deadline probe as timed out without scheduling retries', async () => { + gitExecFileAsyncMock.mockImplementationOnce( + (_argv, options) => + new Promise((_resolve, reject) => { + options.signal.addEventListener('abort', () => reject(options.signal.reason), { + once: true + }) + }) + ) + const h = createHarness() + h.maintenance.arm(h.target) + await vi.advanceTimersByTimeAsync(QUIET_MS) + await elapse(h.maintenance, REF_MAINTENANCE_ATTEMPT_DEADLINE_MS) + expect(h.spans[0]['repo.maintenance_outcome']).toBe('timed_out') + expect(gitExecFileAsyncMock).toHaveBeenCalledOnce() + expect(vi.getTimerCount()).toBe(0) + expect(h.maintainPackIndex).not.toHaveBeenCalled() + expect(h.resolveRefsDirectory).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/git/local-repo-ref-maintenance.test.ts b/src/main/git/local-repo-ref-maintenance.test.ts index 7d084a4bd5a..d84c0d6f84e 100644 --- a/src/main/git/local-repo-ref-maintenance.test.ts +++ b/src/main/git/local-repo-ref-maintenance.test.ts @@ -100,13 +100,13 @@ describe('local repo ref maintenance target', () => { await expect(target().isOptedOut?.(NO_ABORT)).resolves.toBe(false) }) - it('fails closed when the auto-maintenance config cannot be read', async () => { + it('reports unreadable auto-maintenance config without claiming a user opt-out', async () => { for (const error of [ new Error('spawn failed'), Object.assign(new Error('bad config'), { code: 128 }) ]) { gitExecFileAsyncMock.mockRejectedValue(error) - await expect(target().isOptedOut?.(NO_ABORT)).resolves.toBe(true) + await expect(target().isOptedOut?.(NO_ABORT)).rejects.toBe(error) } }) diff --git a/src/main/git/local-repo-ref-maintenance.ts b/src/main/git/local-repo-ref-maintenance.ts index 6c0836b58b1..46638a7aee2 100644 --- a/src/main/git/local-repo-ref-maintenance.ts +++ b/src/main/git/local-repo-ref-maintenance.ts @@ -248,8 +248,11 @@ export function createLocalRepoRefMaintenanceTarget( ) return isGitAutoMaintenanceDisabled(stdout) } catch (error) { - // An unset key is consent; unreadable or invalid config must fail closed. - return !isUnsetGitConfigError(error) + if (isUnsetGitConfigError(error)) { + return false + } + // A failed probe blocks writers and retries without claiming a user opt-out. + throw error } }, async maintainPackIndex(signal, span, canWrite) { @@ -266,7 +269,11 @@ export function createLocalRepoRefMaintenanceTarget( } return 'failed' }, - async packRefs(lock: PackedRefsLockReporter) { + async packRefs( + lock: PackedRefsLockReporter, + admissionSignal?: AbortSignal, + canStart?: () => boolean + ) { const resolved = await resolveCommonDir() const owner = resolved ? new PackRefsLockOwnership(gitCommonDirForMainProcess(resolved, args.wslDistro)) @@ -275,7 +282,7 @@ export function createLocalRepoRefMaintenanceTarget( if (!claim.ok) { throw new RefMaintenanceRepoLocked(claim.reason) } - // Report the rewrite window rather than accepting a signal. A pack that is + // Only admission is cancellable. A pack that is // killed mid-prune strands a `refs/**` lock about one time in five, and // Git never clears those; waiting out the window costs at most ~1.4s. const watch = owner?.watchLock((held) => lock.setHeld(held)) @@ -284,7 +291,9 @@ export function createLocalRepoRefMaintenanceTarget( cwd: args.repoPath, ...gitOptions, admissionTier: 'background', - timeout: PACK_REFS_TIMEOUT_MS + timeout: PACK_REFS_TIMEOUT_MS, + admissionSignal, + canStart }) } finally { watch?.stop() diff --git a/src/main/git/remove-worktree-branch-cleanup.test.ts b/src/main/git/remove-worktree-branch-cleanup.test.ts index 6c31b5f393b..c8343dfa805 100644 --- a/src/main/git/remove-worktree-branch-cleanup.test.ts +++ b/src/main/git/remove-worktree-branch-cleanup.test.ts @@ -1,3 +1,8 @@ +// Ref mutation tests use fictitious repositories; administrative reservations have real-Git coverage. +vi.mock('../../shared/git-worktree-admin', () => ({ + isBranchReservedByWorktreeOperation: vi.fn().mockResolvedValue(false) +})) + import type * as FsPromises from 'node:fs/promises' import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' diff --git a/src/main/git/repo-maintenance-admission.test.ts b/src/main/git/repo-maintenance-admission.test.ts new file mode 100644 index 00000000000..2eda10f5f50 --- /dev/null +++ b/src/main/git/repo-maintenance-admission.test.ts @@ -0,0 +1,609 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { REF_MAINTENANCE_ATTEMPT_DEADLINE_MS } from '../../shared/repo-ref-maintenance-policy' + +type CaptureOptions = { signal?: AbortSignal; onChildTerminated?: () => void } +const { capture, probe, stamp, count, claimOwner, releaseOwner, stopWatch } = vi.hoisted(() => ({ + capture: + vi.fn< + ( + binary: string, + argv: string[], + options: CaptureOptions + ) => Promise<{ stdout: string; stderr: string }> + >(), + probe: vi.fn(), + stamp: vi.fn(), + count: vi.fn(), + claimOwner: vi.fn(), + releaseOwner: vi.fn(), + stopWatch: vi.fn() +})) + +vi.mock('../../shared/loose-ref-count', () => ({ countLooseRefs: count })) +vi.mock('./worktree-list-reader', () => ({ + readRepoCommonDirFromGit: async () => '/repo/.git' +})) +vi.mock('./pack-refs-lock-ownership', () => ({ + PackRefsLockOwnership: class { + claim = claimOwner + watchLock = () => ({ stop: stopWatch }) + release = releaseOwner + } +})) +vi.mock('../observability/tracer', () => ({ + withSpan: async (_name: string, run: (span: unknown) => unknown) => + run({ setAttribute: () => {} }) +})) +vi.mock('./repo-pack-index-state', () => ({ + probeRepoPackIndexDirectory: probe, + readRepoPackDirectoryStamp: stamp +})) +vi.mock('./runner', async () => import('./command-runner/git-exec-file')) +vi.mock('./command-runner/exec-file-capture', () => ({ + execFileCapture: capture, + execFileCaptureToTermination: capture +})) +vi.mock('../observability/instrumentation', () => ({ + withGitSpan: async (_args: unknown, run: (span: unknown) => unknown) => + run({ setAttribute: () => {} }) +})) + +import { RepoRefMaintenance } from '../../shared/repo-ref-maintenance' +import { createLocalRepoRefMaintenanceTarget } from './local-repo-ref-maintenance' +import { clearRepoPackIndexMaintenanceCache } from './repo-pack-index-maintenance' +import { gitExecFileAsync } from './command-runner/git-exec-file' +import { + GENERAL_CAP, + GitAdmissionScheduler, + acquireGitAdmission, + _gitAdmissionSnapshotForTests, + _resetGitAdmissionForTests +} from './command-runner/git-subprocess-admission' + +beforeEach(() => { + vi.resetAllMocks() + clearRepoPackIndexMaintenanceCache() + _resetGitAdmissionForTests(new GitAdmissionScheduler()) + stamp.mockResolvedValue('stable-directory') + probe.mockResolvedValue({ protected: false, packCountFloor: 64 }) + count.mockResolvedValue({ count: 1001, saturated: true }) + claimOwner.mockResolvedValue({ ok: true }) + releaseOwner.mockResolvedValue(undefined) + capture.mockImplementation(async (_binary, argv, options) => { + options.onChildTerminated?.() + if (argv[0] === 'config') { + throw Object.assign(new Error('unset'), { code: 1 }) + } + return { stdout: '', stderr: '' } + }) +}) + +afterEach(() => _resetGitAdmissionForTests()) + +function arm( + writer: 'multi-pack-index' | 'pack-refs', + isBusy: () => boolean = () => false, + quietPeriodMs = 1 +): RepoRefMaintenance { + const maintenance = new RepoRefMaintenance({ quietPeriodMs, isBusy }) + maintenance.arm(writerTarget(writer)) + return maintenance +} + +function writerTarget(writer: 'multi-pack-index' | 'pack-refs', repoPath = '/repo') { + const target = createLocalRepoRefMaintenanceTarget({ + key: `local::${repoPath}/.git`, + repoPath + }) + return writer === 'pack-refs' ? { ...target, maintainPackIndex: undefined } : target +} + +describe('maintenance writer admission cancellation', () => { + describe.each(['multi-pack-index', 'pack-refs'] as const)('%s re-arm admission', (writer) => { + it.each(['low count', 'missing directory'] as const)( + 'discards a superseded %s probe and writes after the renewed quiet period', + async (result) => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const quietPeriodMs = 1000 + let finishProbe: (() => void) | undefined + const oldTarget = writerTarget(writer) + count.mockResolvedValue({ count: 0, saturated: false }) + if (writer === 'multi-pack-index' && result === 'missing directory') { + stamp.mockImplementationOnce( + () => + new Promise((resolve) => { + finishProbe = () => resolve(undefined) + }) + ) + } else if (writer === 'multi-pack-index') { + probe.mockImplementationOnce( + () => + new Promise((resolve) => { + finishProbe = () => resolve({ protected: false, packCountFloor: 0 }) + }) + ) + } else if (result === 'missing directory') { + oldTarget.resolveRefsDirectory = () => + new Promise((resolve) => { + finishProbe = () => resolve(undefined) + }) + } else { + count.mockImplementationOnce( + () => + new Promise((resolve) => { + finishProbe = () => resolve({ count: 0, saturated: false }) + }) + ) + } + const maintenance = new RepoRefMaintenance({ quietPeriodMs }) + maintenance.arm(oldTarget) + const writerCalls = () => capture.mock.calls.filter(([, argv]) => argv[0] === writer) + try { + await vi.advanceTimersByTimeAsync(quietPeriodMs) + await vi.waitFor(() => expect(finishProbe).toBeTypeOf('function')) + const oldAttempt = maintenance.whenAttemptSettled() + maintenance.arm(writerTarget(writer)) + finishProbe?.() + await oldAttempt + expect(writerCalls()).toHaveLength(0) + await vi.advanceTimersByTimeAsync(quietPeriodMs - 1) + expect(writerCalls()).toHaveLength(0) + if (writer === 'pack-refs') { + count.mockResolvedValueOnce({ count: 1001, saturated: true }) + } + await vi.advanceTimersByTimeAsync(1) + await maintenance.whenAttemptSettled() + expect(writerCalls()).toHaveLength(1) + } finally { + maintenance.dispose() + finishProbe?.() + vi.useRealTimers() + } + } + ) + + it('discards a superseded opt-out probe before applying either phase cooldown', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const quietPeriodMs = 1000 + let finishProbe: (() => void) | undefined + capture.mockImplementationOnce( + (_binary, _argv, options) => + new Promise((resolve) => { + finishProbe = () => { + options.onChildTerminated?.() + resolve({ stdout: 'maintenance.auto 0\n', stderr: '' }) + } + }) + ) + count.mockResolvedValue({ count: 0, saturated: false }) + const maintenance = arm(writer, undefined, quietPeriodMs) + const writerCalls = () => capture.mock.calls.filter(([, argv]) => argv[0] === writer) + try { + await vi.advanceTimersByTimeAsync(quietPeriodMs) + await vi.waitFor(() => expect(finishProbe).toBeTypeOf('function')) + const oldAttempt = maintenance.whenAttemptSettled() + maintenance.arm(writerTarget(writer)) + finishProbe?.() + await oldAttempt + expect(writerCalls()).toHaveLength(0) + await vi.advanceTimersByTimeAsync(quietPeriodMs - 1) + expect(writerCalls()).toHaveLength(0) + if (writer === 'pack-refs') { + count.mockResolvedValueOnce({ count: 1001, saturated: true }) + } + await vi.advanceTimersByTimeAsync(1) + await maintenance.whenAttemptSettled() + expect(writerCalls()).toHaveLength(1) + } finally { + maintenance.dispose() + finishProbe?.() + vi.useRealTimers() + } + }) + + it.each([ + 'fresh quiet period', + 'quiet timer rollover', + 'replacement eviction', + 'another repository' + ] as const)('checks queued admission after %s', async (replacement) => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const quietPeriodMs = 1000 + const blockers: { release: () => void }[] = [] + const saturate = async () => { + blockers.push( + ...(await Promise.all( + Array.from({ length: GENERAL_CAP }, () => + acquireGitAdmission({ args: ['status'], cwd: '/blocker', tier: 'background' }) + ) + )) + ) + } + count.mockResolvedValue({ count: 0, saturated: false }) + if (writer === 'multi-pack-index') { + probe.mockImplementationOnce(async () => { + await saturate() + return { protected: false, packCountFloor: 64 } + }) + } else { + count.mockImplementationOnce(async () => { + await saturate() + return { count: 1001, saturated: true } + }) + } + let busy = false + const maintenance = arm(writer, () => busy, quietPeriodMs) + const writerCalls = () => capture.mock.calls.filter(([, argv]) => argv[0] === writer) + try { + await vi.advanceTimersByTimeAsync(quietPeriodMs) + await vi.waitFor(() => expect(_gitAdmissionSnapshotForTests().queued).toBe(1)) + const oldAttempt = maintenance.whenAttemptSettled() + expect(writerCalls()).toHaveLength(0) + busy = true + maintenance.arm( + replacement === 'another repository' + ? { ...writerTarget(writer, '/other'), isOptedOut: async () => true } + : writerTarget(writer) + ) + if (replacement === 'replacement eviction') { + for (let index = 0; index < 64; index += 1) { + maintenance.arm({ + ...writerTarget(writer, `/other-${index}`), + isOptedOut: async () => true + }) + } + } else if (replacement === 'quiet timer rollover') { + await vi.advanceTimersByTimeAsync(quietPeriodMs) + } + busy = false + blockers.forEach((blocker) => blocker.release()) + await oldAttempt + expect(writerCalls()).toHaveLength(replacement === 'another repository' ? 1 : 0) + expect(_gitAdmissionSnapshotForTests().queued).toBe(0) + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) + if (replacement === 'fresh quiet period' || replacement === 'quiet timer rollover') { + await vi.advanceTimersByTimeAsync(quietPeriodMs - 1) + expect(writerCalls()).toHaveLength(0) + if (writer === 'pack-refs') { + count.mockResolvedValueOnce({ count: 1001, saturated: true }) + } + await vi.advanceTimersByTimeAsync(1) + await maintenance.whenAttemptSettled() + expect(writerCalls()).toHaveLength(1) + } else if (replacement === 'replacement eviction') { + await vi.advanceTimersByTimeAsync(quietPeriodMs * 2) + await maintenance.whenAttemptSettled() + expect(writerCalls()).toHaveLength(0) + expect( + capture.mock.calls.filter( + ([, argv]) => argv[0] === 'config' && argv[1] === '--get-regexp' + ) + ).toHaveLength(1) + } + } finally { + maintenance.dispose() + blockers.forEach((blocker) => blocker.release()) + vi.useRealTimers() + } + }) + + it('lets an admitted writer finish while preserving the newer quiet-period timer', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const quietPeriodMs = 1000 + let finishWriter: (() => void) | undefined + capture.mockImplementation(async (_binary, argv, options) => { + if (argv[0] === 'config') { + options.onChildTerminated?.() + throw Object.assign(new Error('unset'), { code: 1 }) + } + await new Promise((resolve) => { + finishWriter = resolve + }) + options.onChildTerminated?.() + return { stdout: '', stderr: '' } + }) + count.mockResolvedValue({ count: 0, saturated: false }) + if (writer === 'pack-refs') { + count.mockResolvedValueOnce({ count: 1001, saturated: true }) + } + const maintenance = arm(writer, undefined, quietPeriodMs) + const optOutProbes = () => + capture.mock.calls.filter(([, argv]) => argv[0] === 'config' && argv[1] === '--get-regexp') + try { + await vi.advanceTimersByTimeAsync(quietPeriodMs) + await vi.waitFor(() => expect(finishWriter).toBeTypeOf('function')) + maintenance.arm(writerTarget(writer)) + const starts = capture.mock.calls.filter(([, argv]) => argv[0] === writer) + expect(starts).toHaveLength(1) + expect(starts[0]?.[2].signal).toBeUndefined() + await vi.advanceTimersByTimeAsync(quietPeriodMs - 1) + expect(optOutProbes()).toHaveLength(1) + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(1) + if (writer === 'pack-refs') { + expect(releaseOwner).not.toHaveBeenCalled() + } + finishWriter?.() + await maintenance.whenAttemptSettled() + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) + if (writer === 'pack-refs') { + expect(stopWatch).toHaveBeenCalledOnce() + expect(releaseOwner).toHaveBeenCalledOnce() + } + await vi.advanceTimersByTimeAsync(1) + await maintenance.whenAttemptSettled() + expect(optOutProbes()).toHaveLength(2) + expect(capture.mock.calls.filter(([, argv]) => argv[0] === writer)).toHaveLength(1) + } finally { + maintenance.dispose() + finishWriter?.() + vi.useRealTimers() + } + }) + }) + + it.each(['packed', 'failed', 'locked', 'deadline'] as const)( + 'discards a superseded %s verdict and admits the fresh ref writer', + async (verdict) => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const quietPeriodMs = 1000 + let finishProbe: (() => void) | undefined + count.mockResolvedValue({ count: 0, saturated: false }) + if (verdict === 'deadline') { + count.mockImplementationOnce( + (_refs, _budget, signal: AbortSignal) => + new Promise((resolve) => { + finishProbe = () => resolve({ count: 0, saturated: false }) + signal.addEventListener('abort', finishProbe, { once: true }) + }) + ) + } else { + count.mockResolvedValueOnce({ count: 1001, saturated: true }) + if (verdict === 'locked') { + claimOwner.mockImplementationOnce( + () => + new Promise((resolve) => { + finishProbe = () => resolve({ ok: false, reason: 'Another lock owner' }) + }) + ) + } else { + count.mockImplementationOnce( + () => + new Promise((resolve) => { + finishProbe = () => + resolve({ + count: verdict === 'failed' ? 1001 : 0, + saturated: verdict === 'failed' + }) + }) + ) + if (verdict === 'failed') { + let failedWriter = false + capture.mockImplementation(async (_binary, argv, options) => { + options.onChildTerminated?.() + if (argv[0] === 'config') { + throw Object.assign(new Error('unset'), { code: 1 }) + } + if (argv[0] === 'pack-refs' && !failedWriter) { + failedWriter = true + throw new Error('Partial ref packing') + } + return { stdout: '', stderr: '' } + }) + } + } + } + const maintenance = arm('pack-refs', undefined, quietPeriodMs) + const writerCalls = () => capture.mock.calls.filter(([, argv]) => argv[0] === 'pack-refs') + const admittedWriters = verdict === 'packed' || verdict === 'failed' ? 1 : 0 + try { + await vi.advanceTimersByTimeAsync(quietPeriodMs) + await vi.waitFor(() => expect(finishProbe).toBeTypeOf('function')) + const oldAttempt = maintenance.whenAttemptSettled() + if (verdict === 'deadline') { + maintenance.arm(writerTarget('pack-refs')) + await vi.advanceTimersByTimeAsync(REF_MAINTENANCE_ATTEMPT_DEADLINE_MS - 1) + } + maintenance.arm(writerTarget('pack-refs')) + if (verdict === 'deadline') { + await vi.advanceTimersByTimeAsync(1) + } else { + finishProbe?.() + } + await oldAttempt + expect(writerCalls()).toHaveLength(admittedWriters) + await vi.advanceTimersByTimeAsync(quietPeriodMs - (verdict === 'deadline' ? 2 : 1)) + expect(writerCalls()).toHaveLength(admittedWriters) + count.mockResolvedValueOnce({ count: 1001, saturated: true }) + await vi.advanceTimersByTimeAsync(1) + await maintenance.whenAttemptSettled() + expect(writerCalls()).toHaveLength(admittedWriters + 1) + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) + } finally { + maintenance.dispose() + finishProbe?.() + vi.useRealTimers() + } + } + ) + + it.each(['multi-pack-index', 'pack-refs'] as const)( + 'rechecks %s activity after admission and still writes on the next idle attempt', + async (writer) => { + const blockers: { release: () => void }[] = [] + const saturate = async () => { + blockers.push( + ...(await Promise.all( + Array.from({ length: GENERAL_CAP }, () => + acquireGitAdmission({ args: ['status'], cwd: '/blocker', tier: 'background' }) + ) + )) + ) + } + count.mockResolvedValue({ count: 0, saturated: false }) + if (writer === 'multi-pack-index') { + probe.mockImplementationOnce(async () => { + await saturate() + return { protected: false, packCountFloor: 64 } + }) + } else { + count.mockImplementationOnce(async () => { + await saturate() + return { count: 1001, saturated: true } + }) + } + let busy = false + const maintenance = arm(writer, () => busy) + try { + await vi.waitFor(() => expect(_gitAdmissionSnapshotForTests().queued).toBe(1)) + busy = true + blockers.forEach((blocker) => blocker.release()) + await maintenance.whenAttemptSettled() + expect(capture.mock.calls.filter(([, argv]) => argv[0] === writer)).toHaveLength(0) + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) + if (writer === 'multi-pack-index') { + expect(stamp).toHaveBeenCalledOnce() + } else { + expect(count).toHaveBeenCalledOnce() + } + busy = false + if (writer === 'pack-refs') { + count.mockResolvedValueOnce({ count: 1001, saturated: true }) + } + const target = createLocalRepoRefMaintenanceTarget({ + key: 'local::/repo/.git', + repoPath: '/repo' + }) + maintenance.arm( + writer === 'pack-refs' ? { ...target, maintainPackIndex: undefined } : target + ) + await vi.waitFor(() => + expect(capture.mock.calls.filter(([, argv]) => argv[0] === writer)).toHaveLength(1) + ) + await maintenance.whenAttemptSettled() + if (writer === 'multi-pack-index') { + expect(stamp).toHaveBeenCalledTimes(3) + } else { + expect(count).toHaveBeenCalledTimes(3) + } + } finally { + maintenance.dispose() + blockers.forEach((blocker) => blocker.release()) + } + } + ) + + it('releases a grant canceled before the child starts', async () => { + const controller = new AbortController() + const reason = new Error('Owner disposed between grant and spawn') + _resetGitAdmissionForTests( + new GitAdmissionScheduler({ + onAdmissionEvent: (event) => { + if (event.phase === 'grant') { + queueMicrotask(() => queueMicrotask(() => controller.abort(reason))) + } + } + }) + ) + await expect( + gitExecFileAsync(['multi-pack-index', 'write'], { + cwd: '/repo', + admissionTier: 'background', + admissionSignal: controller.signal + }) + ).rejects.toBe(reason) + expect(capture).not.toHaveBeenCalled() + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) + }) + + it.each(['multi-pack-index', 'pack-refs'] as const)( + 'removes queued %s immediately on disposal and never starts it after slots reopen', + async (writer) => { + const blockers: { release: () => void }[] = [] + const saturate = async () => { + blockers.push( + ...(await Promise.all( + Array.from({ length: GENERAL_CAP }, () => + acquireGitAdmission({ args: ['status'], cwd: '/blocker', tier: 'background' }) + ) + )) + ) + } + if (writer === 'multi-pack-index') { + probe.mockImplementationOnce(async () => { + await saturate() + return { protected: false, packCountFloor: 64 } + }) + } else { + count.mockImplementationOnce(async () => { + await saturate() + return { count: 1001, saturated: true } + }) + } + const maintenance = arm(writer) + try { + await vi.waitFor(() => + expect(_gitAdmissionSnapshotForTests().queuedWaiters).toEqual([ + expect.objectContaining({ + args: + writer === 'pack-refs' + ? ['pack-refs', '--all', '--prune'] + : ['multi-pack-index', 'write'] + }) + ]) + ) + maintenance.dispose() + expect(_gitAdmissionSnapshotForTests().queued).toBe(0) + blockers.forEach((blocker) => blocker.release()) + await maintenance.whenAttemptSettled() + expect(capture.mock.calls.filter(([, argv]) => argv[0] === writer)).toHaveLength(0) + if (writer === 'pack-refs') { + expect(stopWatch).toHaveBeenCalledOnce() + expect(releaseOwner).toHaveBeenCalledOnce() + } + } finally { + maintenance.dispose() + blockers.forEach((blocker) => blocker.release()) + } + } + ) + + it.each(['multi-pack-index', 'pack-refs'] as const)( + 'lets live %s settle after owner disposal without sending a child abort signal', + async (writer) => { + let finishWriter: (() => void) | undefined + capture.mockImplementation(async (_binary, argv, options) => { + if (argv[0] === 'config') { + options.onChildTerminated?.() + throw Object.assign(new Error('unset'), { code: 1 }) + } + await new Promise((resolve) => { + finishWriter = resolve + }) + options.onChildTerminated?.() + return { stdout: '', stderr: '' } + }) + const maintenance = arm(writer) + try { + await vi.waitFor(() => expect(finishWriter).toBeTypeOf('function')) + maintenance.dispose() + const starts = capture.mock.calls.filter(([, argv]) => argv[0] === writer) + expect(starts).toHaveLength(1) + expect(starts[0]?.[2].signal).toBeUndefined() + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(1) + if (writer === 'pack-refs') { + expect(releaseOwner).not.toHaveBeenCalled() + } + finishWriter?.() + await maintenance.whenAttemptSettled() + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) + if (writer === 'pack-refs') { + expect(stopWatch).toHaveBeenCalledOnce() + expect(releaseOwner).toHaveBeenCalledOnce() + } + } finally { + maintenance.dispose() + finishWriter?.() + } + } + ) +}) diff --git a/src/main/git/repo-pack-index-maintenance.test.ts b/src/main/git/repo-pack-index-maintenance.test.ts index 1fee9644f03..d907a420578 100644 --- a/src/main/git/repo-pack-index-maintenance.test.ts +++ b/src/main/git/repo-pack-index-maintenance.test.ts @@ -76,7 +76,9 @@ describe('idle pack index maintenance', () => { expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith(['multi-pack-index', 'write'], { cwd: '/repo', admissionTier: 'background', - timeout: PACK_INDEX_TIMEOUT_MS + timeout: PACK_INDEX_TIMEOUT_MS, + admissionSignal: options.signal, + canStart: options.canWrite }) expect(options.attributes['git.pack_index_outcome']).toBe('written') }) diff --git a/src/main/git/repo-pack-index-maintenance.ts b/src/main/git/repo-pack-index-maintenance.ts index 2177f4d2166..578586f281b 100644 --- a/src/main/git/repo-pack-index-maintenance.ts +++ b/src/main/git/repo-pack-index-maintenance.ts @@ -106,7 +106,9 @@ export async function maintainRepoPackIndex( cwd: args.repoPath, ...(args.wslDistro ? { wslDistro: args.wslDistro } : {}), admissionTier: 'background', - timeout: PACK_INDEX_TIMEOUT_MS + timeout: PACK_INDEX_TIMEOUT_MS, + admissionSignal: signal, + canStart: args.canWrite }) const writtenStamp = await readRepoPackDirectoryStamp(directory).catch(() => undefined) // A racing new pack remains readable; the forced refresh bounds a missed directory change. @@ -116,6 +118,9 @@ export async function maintainRepoPackIndex( span.setAttribute('git.pack_index_write_ms', Date.now() - startedAt) return outcome('written') } catch (error) { + if (signal.aborted || !args.canWrite()) { + return outcome('deferred') + } span.setAttribute('git.pack_index_error', String(error)) return outcome('failed') } diff --git a/src/main/git/runner-network-ssh-policy.test.ts b/src/main/git/runner-network-ssh-policy.test.ts index 6c4263e1a48..756f0a1ed5b 100644 --- a/src/main/git/runner-network-ssh-policy.test.ts +++ b/src/main/git/runner-network-ssh-policy.test.ts @@ -61,7 +61,7 @@ describe('network Git SSH policy', () => { expect(calls[0]?.env.GIT_SSH_COMMAND).toBeUndefined() expect(calls[1]?.args).toEqual(['fetch', 'origin']) expect(calls[1]?.env.GIT_SSH_COMMAND).toBe( - 'ssh -F ~/.ssh/github-work -i ~/.ssh/work_key -o BatchMode=yes' + 'ssh -o BatchMode=yes -F ~/.ssh/github-work -i ~/.ssh/work_key' ) }) @@ -114,7 +114,9 @@ describe('network Git SSH policy', () => { useConfiguredSshCommandForNetwork: true }) - expect(capturedEnv?.GIT_SSH_COMMAND).toBe('ssh -i ~/.ssh/personal -o BatchMode=yes') + expect(capturedEnv?.GIT_SSH_COMMAND).toBe( + 'ssh -o BatchMode=yes -o BatchMode=no -i ~/.ssh/personal' + ) }) it('merges quoted ssh.exe command shapes for opted-in network calls', async () => { @@ -137,7 +139,7 @@ describe('network Git SSH policy', () => { }) expect(capturedEnv?.GIT_SSH_COMMAND).toBe( - "'C:/Program Files/Git/usr/bin/ssh.exe' -F ~/.ssh/config -o BatchMode=yes" + '"C:/Program Files/Git/usr/bin/ssh.exe" -o BatchMode=yes -F ~/.ssh/config' ) }) @@ -165,7 +167,7 @@ describe('network Git SSH policy', () => { }) expect(capturedEnv?.GIT_SSH_COMMAND).toBe( - String.raw`'C:\Git\usr\bin\ssh.exe' -i 'C:\Users\me\.ssh\work_key' -o BatchMode=yes` + String.raw`'C:\Git\usr\bin\ssh.exe' -o BatchMode=yes -i 'C:\Users\me\.ssh\work_key'` ) }) diff --git a/src/main/git/runner-wsl-network-ssh-policy.test.ts b/src/main/git/runner-wsl-network-ssh-policy.test.ts index 695e57d419f..229ce8b9726 100644 --- a/src/main/git/runner-wsl-network-ssh-policy.test.ts +++ b/src/main/git/runner-wsl-network-ssh-policy.test.ts @@ -108,7 +108,9 @@ describe('WSL network Git SSH policy startup', () => { expect(probeScript.indexOf('core.sshCommand=')).toBeLessThan(probeScript.indexOf("'config'")) expect(calls[0].script).toContain('__ORCA_WSL_CAPTURE_BEGIN_') expect(calls[0].env.GIT_SSH_COMMAND).toBeUndefined() - expect(networkCalls(calls)[0].env.GIT_SSH_COMMAND).toBe(`${SSH_COMMAND} -o BatchMode=yes`) + expect(networkCalls(calls)[0].env.GIT_SSH_COMMAND).toBe( + SSH_COMMAND.replace('ssh', 'ssh -o BatchMode=yes') + ) expect(networkCalls(calls)[0].env.WSLENV?.split(':')).toContain('GIT_SSH_COMMAND') }) diff --git a/src/main/git/source-control/branch-compare.ts b/src/main/git/source-control/branch-compare.ts index 734051463d5..d4388c3347b 100644 --- a/src/main/git/source-control/branch-compare.ts +++ b/src/main/git/source-control/branch-compare.ts @@ -95,6 +95,15 @@ export async function getBranchCompare( } try { + // Git must confirm equal raw tips are the same commit before skipping the reads. + if (baseOid === headOid && mergeBase === headOid) { + options.signal?.throwIfAborted() + summary.commitsAhead = 0 + summary.commitsBehind = 0 + summary.status = 'ready' + return { summary, entries: [] } + } + const [entries, divergence] = await Promise.all([ loadBranchChanges(worktreePath, mergeBase, headOid, options), countCompareDivergence(worktreePath, baseOid, headOid, options) diff --git a/src/main/git/status-branch-compare-real-ref.test.ts b/src/main/git/status-branch-compare-real-ref.test.ts index ce8400d511e..561fe5fab7a 100644 --- a/src/main/git/status-branch-compare-real-ref.test.ts +++ b/src/main/git/status-branch-compare-real-ref.test.ts @@ -1,9 +1,11 @@ import { execFileSync } from 'node:child_process' -import { mkdtemp, rm } from 'node:fs/promises' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import * as path from 'node:path' import { afterEach, describe, expect, it } from 'vitest' import { getBranchCompare } from './status' +import { branchCompare } from '../../relay/git-handler-ops' +import { gitChangeListArgs, parseGitChangeList } from '../../shared/git-change-list' const tempRoots: string[] = [] @@ -15,11 +17,70 @@ function git(repo: string, args: string[]): string { }).trim() } +function relayCompare(repo: string, baseRef: string) { + return branchCompare( + async (args, cwd) => ({ stdout: git(cwd, args), stderr: '' }), + repo, + baseRef, + async (mergeBase, headOid) => + parseGitChangeList(git(repo, gitChangeListArgs(mergeBase, headOid))) + ) +} + afterEach(async () => { await Promise.all(tempRoots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) }) describe('getBranchCompare real refs', () => { + it('reports equal commits as empty on native and relay despite staged and unstaged changes', async () => { + const repo = await mkdtemp(path.join(tmpdir(), 'orca-equal-branch-compare-')) + tempRoots.push(repo) + git(repo, ['init', '-q']) + git(repo, ['config', 'user.email', 'test@example.com']) + git(repo, ['config', 'user.name', 'Test User']) + git(repo, ['-c', 'commit.gpgSign=false', 'commit', '--allow-empty', '-m', 'initial']) + git(repo, ['branch', 'base']) + git(repo, ['checkout', '-q', '-b', 'feature']) + const oid = git(repo, ['rev-parse', 'HEAD']) + await writeFile(path.join(repo, 'changes.txt'), 'staged\n') + git(repo, ['add', 'changes.txt']) + await writeFile(path.join(repo, 'changes.txt'), 'unstaged\n') + + for (const result of await Promise.all([ + getBranchCompare(repo, 'base'), + relayCompare(repo, 'base') + ])) { + expect(result).toEqual({ + summary: { + baseRef: 'base', + baseOid: oid, + compareRef: 'feature', + headOid: oid, + mergeBase: oid, + changedFiles: 0, + commitsAhead: 0, + commitsBehind: 0, + status: 'ready' + }, + entries: [] + }) + } + }) + + it('keeps the merge-base failure for identical blob tips on the relay', async () => { + const repo = await mkdtemp(path.join(tmpdir(), 'orca-blob-branch-compare-')) + tempRoots.push(repo) + git(repo, ['init', '-q']) + await writeFile(path.join(repo, 'blob.txt'), 'not a commit\n') + const oid = git(repo, ['hash-object', '-w', 'blob.txt']) + await writeFile(path.join(repo, '.git', 'HEAD'), `${oid}\n`) + + await expect(relayCompare(repo, oid)).resolves.toMatchObject({ + summary: { headOid: oid, baseOid: oid, status: 'no-merge-base', mergeBase: null }, + entries: [] + }) + }) + it('preserves the raw oid of a remote-tracking ref that stores an annotated tag', async () => { const root = await mkdtemp(path.join(tmpdir(), 'orca-branch-compare-ref-')) tempRoots.push(root) @@ -48,8 +109,19 @@ describe('getBranchCompare real refs', () => { ]) expect(rawOid).not.toBe(peeledOid) - const result = await getBranchCompare(client, 'origin/tagbase') - - expect(result.summary).toMatchObject({ baseOid: rawOid, status: 'ready' }) + for (const result of await Promise.all([ + getBranchCompare(client, 'origin/tagbase'), + relayCompare(client, 'origin/tagbase') + ])) { + expect(result.summary).toMatchObject({ + baseOid: rawOid, + headOid: peeledOid, + mergeBase: peeledOid, + changedFiles: 0, + commitsAhead: 0, + commitsBehind: 0, + status: 'ready' + }) + } }) }) diff --git a/src/main/git/status-branch-compare.test.ts b/src/main/git/status-branch-compare.test.ts index bade46d8a6d..e690735b7ba 100644 --- a/src/main/git/status-branch-compare.test.ts +++ b/src/main/git/status-branch-compare.test.ts @@ -69,6 +69,7 @@ describe('getBranchCompare', () => { headOid?: string | Error baseOid?: string | Error mergeBase?: string | Error + onMergeBase?: () => void nameStatus?: string | Error numstat?: string | Error revList?: string | Error @@ -99,6 +100,7 @@ describe('getBranchCompare', () => { return reply(responses.baseOid, `rev-parse ${args.at(-1)}`) } if (args[0] === 'merge-base') { + responses.onMergeBase?.() return reply(responses.mergeBase, 'merge-base') } if (args.includes('--raw')) { @@ -128,6 +130,62 @@ describe('getBranchCompare', () => { }) } + it.each([40, 64])( + 'skips change and count reads for identical %i-character commit tips', + async (length) => { + const oid = 'a'.repeat(length) + mockBranchCompareGit({ + branch: 'feature\n', + probe: { 'refs/remotes/origin/main^{commit}': `${oid}\n` }, + headOid: `${oid}\n`, + baseOid: `${oid}\n`, + mergeBase: `${oid}\n` + }) + + await expect(getBranchCompare('/repo', 'origin/main')).resolves.toEqual({ + summary: { + baseRef: 'origin/main', + baseOid: oid, + compareRef: 'feature', + headOid: oid, + mergeBase: oid, + changedFiles: 0, + commitsAhead: 0, + commitsBehind: 0, + status: 'ready' + }, + entries: [] + }) + expect(gitExecFileAsyncMock.mock.calls.some(([args]) => args[0] === 'merge-base')).toBe(true) + expect( + gitExecFileAsyncMock.mock.calls.some(([args]) => ['diff', 'rev-list'].includes(args[0])) + ).toBe(false) + } + ) + + it('keeps cancellation effective when equal-tip validation settles', async () => { + const oid = 'a'.repeat(40) + const controller = new AbortController() + mockBranchCompareGit({ + branch: 'feature\n', + probe: { 'refs/remotes/origin/main^{commit}': `${oid}\n` }, + headOid: `${oid}\n`, + baseOid: `${oid}\n`, + mergeBase: `${oid}\n`, + onMergeBase: () => controller.abort(new Error('compare canceled')) + }) + + await expect( + getBranchCompare('/repo', 'origin/main', { signal: controller.signal }) + ).resolves.toMatchObject({ + summary: { status: 'error', errorMessage: 'compare canceled' }, + entries: [] + }) + expect( + gitExecFileAsyncMock.mock.calls.some(([args]) => ['diff', 'rev-list'].includes(args[0])) + ).toBe(false) + }) + it('returns a pinned branch compare snapshot and parsed branch entries', async () => { mockBranchCompareGit({ branch: 'main\n', diff --git a/src/main/git/worktree-branch-removal-host.test.ts b/src/main/git/worktree-branch-removal-host.test.ts index 71119029825..3f6964f63ad 100644 --- a/src/main/git/worktree-branch-removal-host.test.ts +++ b/src/main/git/worktree-branch-removal-host.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, vi } from 'vitest' const { detachedBranchUse } = vi.hoisted(() => ({ detachedBranchUse: vi.fn(async () => false) })) vi.mock('../../shared/git-worktree-admin', () => ({ - isBranchInDetachedWorktree: detachedBranchUse + isBranchReservedByWorktreeOperation: detachedBranchUse })) vi.mock('./local-repo-ref-maintenance', () => ({ withRepoRefMaintenancePaused: (_reason: string, run: () => unknown) => run() diff --git a/src/main/git/worktree-branch-removal.ts b/src/main/git/worktree-branch-removal.ts index 22a33f9da25..4120b124069 100644 --- a/src/main/git/worktree-branch-removal.ts +++ b/src/main/git/worktree-branch-removal.ts @@ -6,7 +6,7 @@ import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import { withLocalGitCapabilityCacheForExecution } from './git-capability-state' import { withRepoRefMaintenancePaused } from './local-repo-ref-maintenance' import { gitExecFileAsync } from './runner' -import { isBranchInDetachedWorktree } from '../../shared/git-worktree-admin' +import { isBranchReservedByWorktreeOperation } from '../../shared/git-worktree-admin' import { parseWorktreeList } from '../../shared/git-worktree-porcelain-parser' import { isBranchCheckedOutInWorktreeError } from '../../shared/git-branch-delete-refusal' import type { GitWorktreeExecOptions, RemoveWorktreeOptions } from './worktree-operation-options' @@ -198,6 +198,6 @@ async function isLocalBranchCheckedOut( const worktrees = parseWorktreeList(stdout) return ( worktrees.some((worktree) => normalizeLocalBranchRef(worktree.branch) === branchName) || - isBranchInDetachedWorktree(repoPath, branchName, worktrees, options) + isBranchReservedByWorktreeOperation(repoPath, branchName, worktrees, options) ) } diff --git a/src/main/git/worktree-preparation-discard-real-git.test.ts b/src/main/git/worktree-preparation-discard-real-git.test.ts new file mode 100644 index 00000000000..90fb41af1c8 --- /dev/null +++ b/src/main/git/worktree-preparation-discard-real-git.test.ts @@ -0,0 +1,170 @@ +import { mkdir, mkdtemp, readFile, realpath, rm, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { afterEach, expect, it, vi } from 'vitest' +import { createWorktreePreparationLockReason } from '../../shared/worktree/create-preparation' +import * as runner from './runner' +import { performDiscardPreparedWorktree } from './worktree-preparation-discard' +import { WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS } from './worktree-operation-options' + +const roots: string[] = [] +afterEach(async () => { + vi.restoreAllMocks() + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +async function fixture(): Promise<{ + repo: string + prepared: string + lock: string + reason: string +}> { + const root = await realpath(await mkdtemp(join(tmpdir(), 'orca-discard-registration-'))) + roots.push(root) + const repo = join(root, 'repo') + const prepared = join(root, 'prepared') + const reason = createWorktreePreparationLockReason('deleted-checkout') + const git = async (cwd: string, args: string[]): Promise => + (await runner.gitExecFileAsync(args, { cwd })).stdout.trim() + await git(root, ['init', '--quiet', repo]) + await git(repo, [ + '-c', + 'user.name=Test', + '-c', + 'user.email=test@example.com', + 'commit', + '--allow-empty', + '--quiet', + '-m', + 'initial' + ]) + await git(repo, ['worktree', 'add', '--detach', '--no-checkout', prepared, 'HEAD']) + await git(repo, ['worktree', 'lock', '--reason', reason, prepared]) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + return { repo, prepared, lock, reason } +} + +it('removes only the owned registration after external deletion of its checkout', async () => { + const { repo, prepared, lock, reason } = await fixture() + await rm(prepared, { recursive: true }) + expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) + const spy = vi.spyOn(runner, 'gitExecFileAsync') + + await performDiscardPreparedWorktree(repo, prepared, { signal: AbortSignal.abort() }, reason) + + expect(spy.mock.calls.map(([args]) => args)).toEqual([ + ['rev-parse', '--git-path', 'locked', '--git-common-dir'], + ['worktree', 'remove', '--force', '--force', prepared] + ]) + expect(spy.mock.calls[1]?.[1]).toEqual({ + cwd: repo, + timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + }) + await expect(readFile(lock, 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }) + expect( + (await runner.gitExecFileAsync(['worktree', 'list', '--porcelain'], { cwd: repo })).stdout + ).not.toContain(prepared) +}) + +it.each(['foreign', 'empty', 'missing', 'unterminated', 'CRLF'])( + 'preserves a missing checkout registration with a %s ownership marker', + async (kind) => { + const { repo, prepared, lock, reason } = await fixture() + const contents = + kind === 'foreign' + ? 'manual lock\n' + : kind === 'empty' + ? '' + : `${reason}${kind === 'CRLF' ? '\r\n' : ''}` + await (kind === 'missing' ? rm(lock) : writeFile(lock, contents)) + await rm(prepared, { recursive: true }) + const spy = vi.spyOn(runner, 'gitExecFileAsync') + + await expect(performDiscardPreparedWorktree(repo, prepared, {}, reason)).rejects.toThrow( + 'lock owner changed' + ) + + expect(spy.mock.calls.some(([args]) => args.includes('remove'))).toBe(false) + expect(await readFile(join(dirname(lock), 'gitdir'), 'utf8')).toBe( + `${join(prepared, '.git')}\n` + ) + if (kind !== 'missing') { + expect(await readFile(lock, 'utf8')).toBe(contents) + } + } +) + +it('preserves a folder that reappears between ownership verification and Git removal', async () => { + const { repo, prepared, lock, reason } = await fixture() + await rm(prepared, { recursive: true }) + const run = runner.gitExecFileAsync + vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + if (args.includes('remove')) { + await mkdir(prepared) + await writeFile(join(prepared, 'user.txt'), 'preserve this folder\n') + } + return run(args, options) + }) + + await expect(performDiscardPreparedWorktree(repo, prepared, {}, reason)).rejects.toThrow( + 'validation failed' + ) + + expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) + expect(await readFile(join(prepared, 'user.txt'), 'utf8')).toBe('preserve this folder\n') +}) + +it('rechecks an owner replaced after the checkout path query fails', async () => { + const { repo, prepared, lock, reason } = await fixture() + await rm(prepared, { recursive: true }) + const run = runner.gitExecFileAsync + const spy = vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + try { + return await run(args, options) + } catch (error) { + if (options?.cwd === prepared) { + await writeFile(lock, 'replacement during cleanup\n') + } + throw error + } + }) + + await expect(performDiscardPreparedWorktree(repo, prepared, {}, reason)).rejects.toThrow( + 'lock owner changed' + ) + + expect(spy.mock.calls.some(([args]) => args.includes('remove'))).toBe(false) + expect(await readFile(lock, 'utf8')).toBe('replacement during cleanup\n') +}) + +it('does not borrow ownership from a different repository', async () => { + const { repo, prepared, lock, reason } = await fixture() + const otherRepo = join(dirname(repo), 'other-repo') + await runner.gitExecFileAsync(['init', '--quiet', otherRepo], { cwd: dirname(repo) }) + await rm(prepared, { recursive: true }) + const spy = vi.spyOn(runner, 'gitExecFileAsync') + + await expect(performDiscardPreparedWorktree(otherRepo, prepared, {}, reason)).rejects.toThrow( + 'lock owner changed' + ) + + expect(spy.mock.calls.some(([args]) => args.includes('remove'))).toBe(false) + expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) +}) + +it.runIf(process.platform !== 'win32')('does not follow a symlink ownership marker', async () => { + const { repo, prepared, lock, reason } = await fixture() + const externalLock = join(dirname(repo), 'external-lock') + await writeFile(externalLock, `${reason}\n`) + await rm(lock) + await symlink(externalLock, lock) + await rm(prepared, { recursive: true }) + const spy = vi.spyOn(runner, 'gitExecFileAsync') + + await expect(performDiscardPreparedWorktree(repo, prepared, {}, reason)).rejects.toThrow( + 'lock owner changed' + ) + + expect(spy.mock.calls.some(([args]) => args.includes('remove'))).toBe(false) + expect(await readFile(externalLock, 'utf8')).toBe(`${reason}\n`) +}) diff --git a/src/main/git/worktree-preparation-discard.ts b/src/main/git/worktree-preparation-discard.ts index bca815e8dc3..10d3c790c1f 100644 --- a/src/main/git/worktree-preparation-discard.ts +++ b/src/main/git/worktree-preparation-discard.ts @@ -1,12 +1,23 @@ +import { lstat } from 'node:fs/promises' +import { join } from 'node:path' +import { waitForPromiseWithSignal } from '../../shared/abort-signal-reason' +import { resolveWorktreeHostPath } from '../../shared/git-metadata-path' +import { findLinkedWorktreeGitDirectory } from '../../shared/git-worktree-admin' import { windowsLongPathGitArgs } from '../../shared/windows-long-path-git-args' +import { toHostFilesystemPath } from '../host-tree-removal' import { gitExecFileAsync } from './runner' import { + getErrorCode, gitExecOptions, WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS, type GitExecOptionsForWorktree, type GitWorktreeExecOptions } from './worktree-operation-options' -import { verifyWorktreePreparationLock } from './worktree-preparation-lock' +import { + verifyWorktreePreparationLock, + verifyWorktreePreparationLockAtPath, + WorktreePreparationLockOwnershipError +} from './worktree-preparation-lock' import { invalidateWslLinkedWorktreeGitRouting } from './wsl-linked-worktree-git-routing' export function gitCleanupOptions( @@ -17,6 +28,59 @@ export function gitCleanupOptions( return gitExecOptions(cwd, { ...options, signal: undefined }) } +async function verifyDiscardPreparationOwner( + repoPath: string, + worktreePath: string, + expectedLockReason: string, + options: GitExecOptionsForWorktree +): Promise { + try { + await verifyWorktreePreparationLock(worktreePath, expectedLockReason, options) + } catch (error) { + if (error instanceof WorktreePreparationLockOwnershipError) { + throw error + } + const hostPath = resolveWorktreeHostPath(worktreePath, options) + const hostRepo = resolveWorktreeHostPath(repoPath, options) + if (!hostPath || !hostRepo) { + throw error + } + const signal = AbortSignal.timeout(options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS) + const absent = await waitForPromiseWithSignal( + lstat(toHostFilesystemPath(hostPath)), + signal + ).then( + () => false, + (failure: unknown) => { + if (getErrorCode(failure) === 'ENOENT') { + return true + } + throw failure + } + ) + if (!absent) { + throw error + } + const gitDir = await findLinkedWorktreeGitDirectory(hostRepo, hostPath, { ...options, signal }) + if (!gitDir) { + throw new WorktreePreparationLockOwnershipError(error) + } + const lockPath = toHostFilesystemPath(join(gitDir, 'locked')) + const marker = await waitForPromiseWithSignal(lstat(lockPath), signal).catch( + (failure: unknown) => { + throw new WorktreePreparationLockOwnershipError(failure) + } + ) + if (!marker.isFile()) { + throw new WorktreePreparationLockOwnershipError() + } + await waitForPromiseWithSignal( + verifyWorktreePreparationLockAtPath(lockPath, expectedLockReason, signal), + signal + ) + } +} + export async function performDiscardPreparedWorktree( repoPath: string, worktreePath: string, @@ -29,7 +93,12 @@ export async function performDiscardPreparedWorktree( } try { if (expectedLockReason !== undefined) { - await verifyWorktreePreparationLock(worktreePath, expectedLockReason, cleanupGitOptions) + await verifyDiscardPreparationOwner( + repoPath, + worktreePath, + expectedLockReason, + cleanupGitOptions + ) } // Double force requires a freshly verified ownership marker. await gitExecFileAsync( diff --git a/src/main/git/worktree-preparation-lock-real-git.test.ts b/src/main/git/worktree-preparation-lock-real-git.test.ts index 55c31fdc44d..f4aa3f800f3 100644 --- a/src/main/git/worktree-preparation-lock-real-git.test.ts +++ b/src/main/git/worktree-preparation-lock-real-git.test.ts @@ -10,6 +10,7 @@ import { prepareWorktreeCreateCheckout } from './worktree-create-preparation' import { unlockWorktreePreparation } from './worktree-preparation-lock' +import { readWorktreeList } from './worktree-list-reader' import { _resetPreparationPoolForTests, listPreparations, @@ -28,10 +29,12 @@ async function git(cwd: string, args: string[]): Promise { return (await runner.gitExecFileAsync(args, { cwd })).stdout.trim() } -async function fixture(): Promise<{ root: string; repo: string; prepared: string; final: string }> { +async function fixture( + repoName = 'repo' +): Promise<{ root: string; repo: string; prepared: string; final: string }> { const root = await realpath(await mkdtemp(join(tmpdir(), 'orca-preparation-lock-'))) roots.push(root) - const repo = join(root, 'repo') + const repo = join(root, repoName) await git(root, ['init', '--quiet', repo]) await git(repo, ['symbolic-ref', 'HEAD', 'refs/heads/main']) await writeFile(join(repo, 'tracked.txt'), 'original\n') @@ -58,8 +61,11 @@ it('creates and consumes its marker without worktree lock or unlock inventory sc expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) spy.mockClear() await finalizePreparedWorktree(repo, prepared, final, 'feature', 'main', false, {}, reason) - expect(spy.mock.calls.filter(([args]) => args.includes('--git-path'))).toHaveLength(1) - expect(spy.mock.calls.filter(([args]) => args.includes('--git-common-dir'))).toHaveLength(1) + const lockQueries = spy.mock.calls.filter( + ([args]) => args.includes('--git-path') || args.includes('--git-common-dir') + ) + expect(lockQueries).toHaveLength(1) + expect(lockQueries[0]?.[0]).toEqual(['rev-parse', '--git-path', 'locked', '--git-common-dir']) expect(await git(final, ['symbolic-ref', '--short', 'HEAD'])).toBe('feature') expect(await git(final, ['status', '--porcelain'])).toBe('') expect(await readFile(join(final, 'tracked.txt'), 'utf8')).toBe('original\n') @@ -69,6 +75,29 @@ it('creates and consumes its marker without worktree lock or unlock inventory sc ) }) +it.runIf(process.platform !== 'win32')( + 'unlocks an owned preparation whose repository path contains a newline', + async () => { + const { repo, prepared } = await fixture('repo\nnewline') + const reason = createWorktreePreparationLockReason('newline-path') + await prepareWorktreeCreateCheckout(repo, prepared, 'main', reason) + const lock = await git(prepared, ['rev-parse', '--git-path', 'locked']) + expect(lock).toContain('\n') + expect(await readFile(lock, 'utf8')).toBe(`${reason}\n`) + const spy = vi.spyOn(runner, 'gitExecFileAsync') + + await unlockWorktreePreparation(prepared, reason, {}) + expect(spy.mock.calls.map(([args]) => args)).toEqual([ + ['rev-parse', '--git-path', 'locked', '--git-common-dir'], + ['rev-parse', '--git-path', 'locked'], + ['rev-parse', '--git-common-dir'] + ]) + await expect(readFile(lock, 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }) + expect(await readFile(join(prepared, 'tracked.txt'), 'utf8')).toBe('original\n') + expect(await git(prepared, ['status', '--porcelain'])).toBe('') + } +) + it('has its exact ownership marker before the atomic add returns', async () => { const { repo, prepared } = await fixture() const reason = createWorktreePreparationLockReason('atomic-add') @@ -94,8 +123,14 @@ it('has its exact ownership marker before the atomic add returns', async () => { expect(observed).toBe(minor >= 33) }) -it('cleans only its newly registered marker when cancellation follows atomic add', async () => { +it('cleans only its newly registered marker when cancellation follows atomic add', async ({ + skip +}) => { const { repo, prepared } = await fixture() + const version = (await git(repo, ['--version'])).match(/git version (\d+)\.(\d+)/) + if (Number(version?.[1]) === 2 && Number(version?.[2]) < 33) { + skip() + } const reason = createWorktreePreparationLockReason('atomic-cancellation') const controller = new AbortController() const run = runner.gitExecFileAsync @@ -170,8 +205,12 @@ it('preserves a competing marker and registration through preparation failure an await _resetPreparationPoolForTests() expect(await readFile(lock, 'utf8')).toBe('manual competing preparation\n') expect(await readFile(join(prepared, 'tracked.txt'), 'utf8')).toBe('original\n') - expect(await git(repo, ['worktree', 'list', '--porcelain'])).toContain( - 'manual competing preparation' + expect(await readWorktreeList(repo)).toContainEqual( + expect.objectContaining({ + path: prepared, + locked: true, + lockReason: 'manual competing preparation' + }) ) }) diff --git a/src/main/git/worktree-preparation-lock.test.ts b/src/main/git/worktree-preparation-lock.test.ts index 8120514a695..4bb2b0cadd8 100644 --- a/src/main/git/worktree-preparation-lock.test.ts +++ b/src/main/git/worktree-preparation-lock.test.ts @@ -32,7 +32,10 @@ const lockPath = toHostFilesystemPath(gitLockPath) beforeEach(() => { mocks.git.mockReset().mockImplementation(async (args: string[]) => ({ - stdout: `${args.includes('--git-path') ? gitLockPath : commonDir}\n` + stdout: + args.includes('--git-path') && args.includes('--git-common-dir') + ? `${gitLockPath}\n${commonDir}\n` + : `${args.includes('--git-path') ? gitLockPath : commonDir}\n` })) mocks.readFile.mockReset().mockResolvedValue(`${lockReason}\n`) mocks.writeFile.mockReset().mockResolvedValue(undefined) @@ -41,17 +44,18 @@ beforeEach(() => { describe('targeted preparation lock ownership', () => { it('creates Git’s reason marker exclusively without enumerating worktrees', async () => { - const options = { wslDistro: 'Ubuntu', timeout: 8000, admissionTier: 'interactive' as const } + const controller = new AbortController() + const options = { + wslDistro: 'Ubuntu', + signal: controller.signal, + timeout: 8000, + admissionTier: 'interactive' as const + } await lockWorktreePreparation('/prepared', lockReason, options) - expect(mocks.git).toHaveBeenCalledTimes(2) - expect(mocks.git).toHaveBeenCalledWith(['rev-parse', '--git-path', 'locked'], { - cwd: '/prepared', - ...options - }) - expect(mocks.git).toHaveBeenCalledWith(['rev-parse', '--git-common-dir'], { - cwd: '/prepared', - ...options - }) + expect(mocks.git).toHaveBeenCalledExactlyOnceWith( + ['rev-parse', '--git-path', 'locked', '--git-common-dir'], + { cwd: '/prepared', ...options } + ) expect(mocks.writeFile).toHaveBeenCalledExactlyOnceWith(lockPath, `${lockReason}\n`, { flag: 'wx' }) @@ -105,13 +109,15 @@ describe('targeted preparation lock ownership', () => { it('honors cancellation after the path probe before writing a lock', async () => { const controller = new AbortController() + const cancellation = new Error('lock canceled') mocks.git.mockImplementationOnce(async () => { - controller.abort() - return { stdout: `${gitLockPath}\n` } + controller.abort(cancellation) + return { stdout: `${gitLockPath}\n${commonDir}\n` } }) await expect( lockWorktreePreparation('/prepared', lockReason, { signal: controller.signal }) - ).rejects.toThrow() + ).rejects.toBe(cancellation) + expect(mocks.git).toHaveBeenCalledOnce() expect(mocks.writeFile).not.toHaveBeenCalled() }) @@ -129,6 +135,99 @@ describe('targeted preparation lock ownership', () => { }) }) +describe('combined preparation lock query', () => { + it('reads embedded-newline pointers independently without changing their paths', async () => { + const controller = new AbortController() + const options = { signal: controller.signal, wslDistro: 'Ubuntu', timeout: 6000 } + const newlineCommon = join(tmpdir(), 'repo\nnewline', '.git') + const newlineLock = join(newlineCommon, 'worktrees', 'prepared', 'locked') + mocks.git.mockImplementation(async (args: string[]) => ({ + stdout: + args.includes('--git-path') && args.includes('--git-common-dir') + ? `${newlineLock}\n${newlineCommon}\n` + : `${args.includes('--git-path') ? newlineLock : newlineCommon}\n` + })) + + await expect(lockWorktreePreparation('/prepared', lockReason, options)).resolves.toBe( + toHostFilesystemPath(newlineLock) + ) + expect(mocks.git.mock.calls).toEqual([ + [['rev-parse', '--git-path', 'locked', '--git-common-dir'], { cwd: '/prepared', ...options }], + [['rev-parse', '--git-path', 'locked'], { cwd: '/prepared', ...options }], + [['rev-parse', '--git-common-dir'], { cwd: '/prepared', ...options }] + ]) + expect(mocks.writeFile).toHaveBeenCalledExactlyOnceWith( + toHostFilesystemPath(newlineLock), + `${lockReason}\n`, + { flag: 'wx' } + ) + }) + + it.each(['create', 'unlock'] as const)( + 'refuses a foreign administrative path before %s touches its marker', + async (operation) => { + const foreignLock = join(tmpdir(), 'other', '.git', 'worktrees', 'prepared', 'locked') + mocks.git.mockResolvedValueOnce({ stdout: `${foreignLock}\n${commonDir}\n` }) + const run = operation === 'create' ? lockWorktreePreparation : unlockWorktreePreparation + await expect(run('/prepared', lockReason, {})).rejects.toThrow('linked worktree lock path') + expect(mocks.git).toHaveBeenCalledOnce() + expect(mocks.readFile).not.toHaveBeenCalled() + expect(mocks.writeFile).not.toHaveBeenCalled() + expect(mocks.unlink).not.toHaveBeenCalled() + } + ) + + it.each(['create', 'unlock'] as const)( + 'keeps a failed combined Git read visible before %s touches a marker', + async (operation) => { + const failure = new Error('Git cannot read the preparation') + mocks.git.mockRejectedValueOnce(failure) + const run = operation === 'create' ? lockWorktreePreparation : unlockWorktreePreparation + await expect(run('/prepared', lockReason, {})).rejects.toBe(failure) + expect(mocks.git).toHaveBeenCalledOnce() + expect(mocks.readFile).not.toHaveBeenCalled() + expect(mocks.writeFile).not.toHaveBeenCalled() + expect(mocks.unlink).not.toHaveBeenCalled() + } + ) + + it('keeps a failed independent pointer read visible after an ambiguous response', async () => { + const failure = new Error('common directory is unavailable') + mocks.git.mockResolvedValueOnce({ stdout: 'ambiguous\n' }).mockImplementation(async (args) => { + if (args.includes('--git-common-dir')) { + throw failure + } + return { stdout: `${gitLockPath}\n` } + }) + await expect(lockWorktreePreparation('/prepared', lockReason, {})).rejects.toBe(failure) + expect(mocks.git).toHaveBeenCalledTimes(3) + expect(mocks.writeFile).not.toHaveBeenCalled() + expect(mocks.unlink).not.toHaveBeenCalled() + }) + + it('forwards cancellation to both independent reads and never creates a marker', async () => { + const controller = new AbortController() + const cancellation = new Error('fallback canceled') + const options = { signal: controller.signal, wslDistro: 'Ubuntu', timeout: 6000 } + mocks.git.mockResolvedValueOnce({ stdout: 'ambiguous\n' }).mockImplementation(async (args) => { + if (args.includes('--git-path')) { + controller.abort(cancellation) + return { stdout: `${gitLockPath}\n` } + } + return { stdout: `${commonDir}\n` } + }) + await expect(lockWorktreePreparation('/prepared', lockReason, options)).rejects.toBe( + cancellation + ) + expect(mocks.git.mock.calls).toEqual([ + [['rev-parse', '--git-path', 'locked', '--git-common-dir'], { cwd: '/prepared', ...options }], + [['rev-parse', '--git-path', 'locked'], { cwd: '/prepared', ...options }], + [['rev-parse', '--git-common-dir'], { cwd: '/prepared', ...options }] + ]) + expect(mocks.writeFile).not.toHaveBeenCalled() + }) +}) + describe('Git preparation lock path resolution', () => { it.each([ { diff --git a/src/main/git/worktree-preparation-lock.ts b/src/main/git/worktree-preparation-lock.ts index ba60280b90e..d0ea67935a7 100644 --- a/src/main/git/worktree-preparation-lock.ts +++ b/src/main/git/worktree-preparation-lock.ts @@ -44,24 +44,27 @@ async function readPreparationLockPath( worktreePath: string, options: GitWorktreeExecOptions ): Promise { - const results = await Promise.allSettled([ - gitExecFileAsync(['rev-parse', '--git-path', 'locked'], gitExecOptions(worktreePath, options)), - gitExecFileAsync(['rev-parse', '--git-common-dir'], gitExecOptions(worktreePath, options)) - ]) - const [lock, common] = results - if (lock.status === 'rejected') { - throw lock.reason - } - if (common.status === 'rejected') { - throw common.reason - } + const { stdout } = await gitExecFileAsync( + ['rev-parse', '--git-path', 'locked', '--git-common-dir'], + gitExecOptions(worktreePath, options) + ) + options.signal?.throwIfAborted() + const [lock, common, end, ...extra] = stdout.split('\n') + // Newlines in a path make the combined response ambiguous; read each pointer separately. + const [rawLock, rawCommon] = + lock && common && end === '' && extra.length === 0 + ? [lock, common] + : await Promise.all( + [ + ['rev-parse', '--git-path', 'locked'], + ['rev-parse', '--git-common-dir'] + ].map(async (args) => { + const result = await gitExecFileAsync(args, gitExecOptions(worktreePath, options)) + return result.stdout + }) + ) return toHostFilesystemPath( - resolveWorktreePreparationLockPath( - worktreePath, - lock.value.stdout, - common.value.stdout, - options - ) + resolveWorktreePreparationLockPath(worktreePath, rawLock, rawCommon, options) ) } diff --git a/src/main/git/worktree-rebase-update-refs-real-git.test.ts b/src/main/git/worktree-rebase-update-refs-real-git.test.ts new file mode 100644 index 00000000000..8625b77490a --- /dev/null +++ b/src/main/git/worktree-rebase-update-refs-real-git.test.ts @@ -0,0 +1,133 @@ +import { mkdir, mkdtemp, readFile, realpath, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { runProcess } from '../../shared/child-process/run-process' + +vi.mock('./runner', () => ({ gitExecFileAsync: vi.fn(), gitExecFileSync: vi.fn() })) +vi.mock('./local-repo-ref-maintenance', () => ({ + withRepoRefMaintenancePaused: (_reason: string, run: () => unknown) => run() +})) + +import { forceDeleteLocalBranch } from './worktree-branch-removal' +import { forceDeletePreservedRelayBranch } from '../../relay/git-handler-branch-cleanup' + +const image = process.env.ORCA_GIT_COMPAT_IMAGE +const binary = process.env.ORCA_GIT_COMPAT_BINARY ?? 'git' +const dockerUser = + typeof process.getuid === 'function' && typeof process.getgid === 'function' + ? ['--user', `${process.getuid()}:${process.getgid()}`] + : [] +let root = '' +let repo = '' +let checkout = '' + +async function git(args: string[], cwd = repo): Promise<{ stdout: string; stderr: string }> { + const result = await runProcess({ + program: image ? 'docker' : binary, + args: image + ? [ + 'run', + '--rm', + '--network=none', + ...dockerUser, + '-v', + `${root}:${root}`, + '-w', + cwd, + image, + '-c', + `safe.directory=${cwd}`, + ...args + ] + : args, + cwd, + env: { + ...process.env, + GIT_CONFIG_GLOBAL: join(root, 'empty-config'), + GIT_CONFIG_NOSYSTEM: '1' + }, + maxOutputBytes: 2 * 1024 * 1024 + }) + if (result.code !== 0) { + throw Object.assign(new Error(result.stderr), { code: result.code, stderr: result.stderr }) + } + return result +} + +beforeEach(async () => { + root = await realpath(await mkdtemp(join(tmpdir(), 'orca-rebase-reservation-'))) + repo = join(root, 'repo') + checkout = join(root, 'checkout') + await mkdir(repo) + await writeFile(join(root, 'empty-config'), '') + await git(['init', '-q']) + await git(['symbolic-ref', 'HEAD', 'refs/heads/main']) + for (const [key, value] of [ + ['user.name', 'Reservation Safety'], + ['user.email', 'safety@example.invalid'], + ['commit.gpgSign', 'false'], + ['core.hooksPath', '.git/no-hooks'], + ['gc.auto', '0'] + ]) { + await git(['config', key, value]) + } + await writeFile(join(repo, 'seed'), 'seed\n') + await git(['add', 'seed']) + await git(['commit', '-qm', 'seed']) +}) + +afterEach(async () => { + await rm(root, { recursive: true, force: true }) +}) + +describe('rebase branch reservations with real Git', () => { + it.for(['native', 'relay'] as const)( + '%s retains auxiliary branches reserved by a paused real rebase --update-refs', + { timeout: 120_000 }, + async (host, context) => { + await git(['worktree', 'add', '-q', '-b', 'feature', checkout]) + await writeFile(join(checkout, 'first'), 'first\n') + await git(['add', 'first'], checkout) + await git(['commit', '-qm', 'first'], checkout) + await git(['branch', 'auxiliary'], checkout) + const expected = (await git(['rev-parse', 'refs/heads/auxiliary'])).stdout.trim() + await writeFile(join(checkout, 'second'), 'second\n') + await git(['add', 'second'], checkout) + await git(['commit', '-qm', 'second'], checkout) + await writeFile(join(repo, 'base'), 'advanced main\n') + await git(['add', 'base']) + await git(['commit', '-qm', 'advance main']) + try { + await git(['rebase', '--update-refs', '--exec', 'false', 'main'], checkout) + throw new Error('Rebase did not pause') + } catch (error) { + if (error instanceof Error && /unknown option[^\n]*update-refs/.test(error.message)) { + context.skip() + } + if (!(error instanceof Error) || !/execution failed/.test(error.message)) { + throw error + } + } + const gitDir = (await git(['rev-parse', '--absolute-git-dir'], checkout)).stdout.trim() + expect(await readFile(join(gitDir, 'rebase-merge', 'update-refs'), 'utf8')).toContain( + 'refs/heads/auxiliary\n' + ) + await expect(git(['branch', '-D', 'auxiliary'])).rejects.toThrow( + /checked out|used by worktree/ + ) + const cleanupGit = vi.fn((args: string[], cwd: string) => git(args, cwd)) + const remove = () => + host === 'native' + ? forceDeleteLocalBranch(repo, 'auxiliary', expected, cleanupGit) + : forceDeletePreservedRelayBranch(cleanupGit, repo, 'auxiliary', expected) + await expect(remove()).rejects.toThrow('checked out in another worktree') + expect(cleanupGit.mock.calls.map(([args]) => args)).toEqual([ + ['worktree', 'list', '--porcelain'] + ]) + expect((await git(['rev-parse', 'refs/heads/auxiliary'])).stdout.trim()).toBe(expected) + await git(['rebase', '--abort'], checkout) + await expect(remove()).resolves.toBeUndefined() + } + ) +}) diff --git a/src/main/git/worktree-sparse-checkout-cache.ts b/src/main/git/worktree-sparse-checkout-cache.ts index 56f8dedc8e8..c87c2ecbfe0 100644 --- a/src/main/git/worktree-sparse-checkout-cache.ts +++ b/src/main/git/worktree-sparse-checkout-cache.ts @@ -2,11 +2,8 @@ import type { GitRuntimeOptions } from './git-runtime-options' import { canonicalWorktreePath } from './worktree-path-comparison' import { detectSparseCheckout } from './worktree-sparse-state' -// Why: `git worktree list` only emits a `sparse` porcelain line on newer Git (annotateSparseCheckoutStatus -// already skips rows where that's set), but Orca's compatibility baseline is Git 2.25, which predates it — -// so every listing still paid a per-worktree fs.stat + config read on the fallback path, measured at ~9x -// the cost of the `git worktree list` call it decorates on a 1000-worktree repo. Cache the result, scoped -// per repo so churn in one repo can't evict another's warm entries. +// Worktree porcelain omits sparse state, so cache the per-worktree filesystem detection. +// Keys isolate repositories and distros; one global budget bounds retained results. // // Invalidation coverage: // - Orca-driven remove/move: explicit calls below (worktree-removal.ts, worktree-move.ts). diff --git a/src/main/ipc/ai-vault-search.test.ts b/src/main/ipc/ai-vault-search.test.ts index 420d23f56b3..19191f9d873 100644 --- a/src/main/ipc/ai-vault-search.test.ts +++ b/src/main/ipc/ai-vault-search.test.ts @@ -26,6 +26,7 @@ import { registerAiVaultSearchHandlers } from './ai-vault-search' import { aiVaultApi } from '../../preload/api/ai-vault-bridge' import { setSessionSearchService } from '../ai-vault-search/session-search-service-registry' import { unavailableSessionSearchStatus } from '../../shared/ai-vault-search-client' +import { AI_VAULT_AGENTS } from '../../shared/ai-vault-types' import { fakeSearchService, searchHit, @@ -86,6 +87,7 @@ describe('desktop IPC and preload search boundary', () => { expect(sshSearch).toHaveBeenCalledWith('ssh-host', 'aiVault.searchSessions', { query: 'needle', limit: 20, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) @@ -108,6 +110,7 @@ describe('desktop IPC and preload search boundary', () => { expect(runtimeSearch).toHaveBeenCalledWith('env-1', 'aiVault.searchSessions', { query: 'needle', limit: 20, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) @@ -120,6 +123,38 @@ describe('desktop IPC and preload search boundary', () => { expect(await aiVaultApi.searchStatus('runtime:env-1')).toEqual(unavailableSessionSearchStatus()) expect(runtimeSearch).toHaveBeenLastCalledWith('env-1', 'aiVault.searchStatus', {}) }) + it.each(['ssh:ssh-host', 'runtime:env-1', 'all'] as const)( + 'negotiates each actual remote leg once through preload scope %s', + async (scope) => { + const local = fakeSearchService() + setSessionSearchService(local) + sshHostInfos.mockReturnValue([{ targetId: 'ssh-host' }]) + const remoteReply = async (_host: string, method: string) => + method === 'aiVault.searchStatus' + ? { ...unavailableSessionSearchStatus(), supportedAgents: [...AI_VAULT_AGENTS] } + : { ...searchResults(), hits: [{ ...searchHit(), agent: 'jcode' }] } + sshSearch.mockImplementation(remoteReply) + runtimeSearch.mockImplementation(remoteReply) + const result = await aiVaultApi.searchSessions( + { query: 'needle', filters: { agents: [...AI_VAULT_AGENTS] } }, + scope + ) + const calls = scope === 'runtime:env-1' ? runtimeSearch : sshSearch + expect(calls.mock.calls.map((call) => call[1])).toEqual([ + 'aiVault.searchStatus', + 'aiVault.searchSessions' + ]) + const remoteHits = + result.kind === 'results' + ? result.hits.filter((hit) => hit.executionHostId !== 'local') + : [] + expect(remoteHits).toMatchObject([{ agent: 'jcode', source: { presence: 'present' } }]) + expect(JSON.stringify(remoteHits)).not.toContain('/host/transcript') + expect(JSON.stringify(remoteHits)).not.toContain('resumeCommand') + expect(local.search).toHaveBeenCalledTimes(scope === 'all' ? 1 : 0) + expect(scope === 'runtime:env-1' ? sshSearch : runtimeSearch).not.toHaveBeenCalled() + } + ) it('maps a runtime unknown-method refusal to unavailable and keeps transport errors', async () => { runtimeSearch.mockRejectedValue( Object.assign(new Error('unknown method'), { code: 'method_not_found' }) diff --git a/src/main/ipc/filesystem/filesystem-git-pull-request-generation-handlers.ts b/src/main/ipc/filesystem/filesystem-git-pull-request-generation-handlers.ts index 2361bbb4f4c..6cefc50f3eb 100644 --- a/src/main/ipc/filesystem/filesystem-git-pull-request-generation-handlers.ts +++ b/src/main/ipc/filesystem/filesystem-git-pull-request-generation-handlers.ts @@ -18,6 +18,7 @@ import { import { resolveRegisteredWorktreePath } from '../registered-worktree-roots-cache' import { getLocalGitOptionsForRegisteredWorktree } from '../local-worktree-runtime-options' import { gitExecFileAsync } from '../../git/runner' +import { execSshReviewDraft } from '../../providers/ssh-review-draft-context' import { withLinkedIssueDraftContext } from '../../../shared/source-control-ai-action-variables' import { resolveSourceControlAiLinkedIssueMeta } from '../source-control-ai-linked-issue' import { resolveHostedReviewBodyForGeneration } from '../../source-control/pull-request-template' @@ -102,11 +103,7 @@ export function registerFilesystemGitPullRequestGenerationHandlers( }) context = await getPullRequestDraftContext( (argv, commandOptions) => - commandOptions?.timeoutMs !== undefined - ? provider.exec(argv, args.worktreePath, { timeoutMs: commandOptions.timeoutMs }) - : commandOptions?.timeout !== undefined - ? provider.exec(argv, args.worktreePath, { timeoutMs: commandOptions.timeout }) - : provider.exec(argv, args.worktreePath), + execSshReviewDraft(provider, argv, args.worktreePath, commandOptions), { base: args.base, currentTitle: args.title, diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 5fd475a0698..b2f8f88d76c 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -9,8 +9,47 @@ import { requestGitStreamable } from '../ssh/ssh-git-response-stream-reader' import type { IGitProvider } from './types' import { isJsonRpcMethodNotFoundError } from './ssh-git-relay-errors' import { SshGitWorktreeProvider } from './ssh-git-worktree-provider' +import { ReviewDraftContextError } from '../../shared/review-draft-context-error' export class SshGitProvider extends SshGitWorktreeProvider implements IGitProvider { + async readReviewDiff( + worktreePath: string, + mergeBase: string, + format: 'name-status' | 'patch', + options?: { timeoutMs?: number } + ): Promise<{ stdout: string; stderr: string }> { + let result: unknown + try { + result = await requestGitStreamable( + this.mux, + 'git.reviewDiff', + { worktreePath, mergeBase, format }, + options + ) + } catch (error) { + if (isJsonRpcMethodNotFoundError(error)) { + throw new ReviewDraftContextError( + 'SSH review draft support is unavailable on this relay. Reconnect the SSH target to update Orca on the host, then try again.' + ) + } + throw new ReviewDraftContextError( + error instanceof Error ? error.message : 'SSH review diff failed.', + { cause: error } + ) + } + if ( + !result || + typeof result !== 'object' || + !('stdout' in result) || + !('stderr' in result) || + typeof result.stdout !== 'string' || + typeof result.stderr !== 'string' + ) { + throw new ReviewDraftContextError('Invalid SSH review diff response.') + } + return { stdout: result.stdout, stderr: result.stderr } + } + async getStagedCommitContext(worktreePath: string): Promise { const branchPromise = this.exec(['branch', '--show-current'], worktreePath).catch(() => ({ stdout: '' diff --git a/src/main/providers/ssh-review-draft-context.test.ts b/src/main/providers/ssh-review-draft-context.test.ts new file mode 100644 index 00000000000..9703d5fc71f --- /dev/null +++ b/src/main/providers/ssh-review-draft-context.test.ts @@ -0,0 +1,334 @@ +import { mkdir, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { runProcess } from '../../shared/child-process/run-process' +import { ReviewDraftContextError } from '../../shared/review-draft-context-error' +import { RelayContext } from '../../relay/context' +import { RelayDispatcher } from '../../relay/dispatcher' +import { GitHandler } from '../../relay/git-handler' +import { createGitTempDir, removeGitTempDir } from '../../relay/git-handler-test-harness' +import { gitCommit, gitInit } from '../../relay/git-handler-test-setup' +import { + encodeJsonRpcFrame, + FrameDecoder, + MessageType, + parseJsonRpcMessage +} from '../../relay/protocol' +import type { SshChannelMultiplexer } from '../ssh/ssh-channel-multiplexer' +import { getPullRequestDraftContext } from '../text-generation/pull-request-context' +import { SshGitProvider } from './ssh-git-provider' +import { createMockMux } from './ssh-git-provider-test-harness' +import { execSshReviewDraft } from './ssh-review-draft-context' + +const input = { + base: 'main', + currentTitle: 'Existing review title', + currentBody: 'Existing review body', + currentDraft: true +} +const mergeBase = 'a'.repeat(40) +const disposals: (() => void)[] = [] +const directories: string[] = [] + +afterEach(async () => { + disposals.splice(0).forEach((dispose) => dispose()) + await Promise.all(directories.splice(0).map(removeGitTempDir)) + vi.restoreAllMocks() + vi.unstubAllEnvs() +}) + +function providerForMux(mux: ReturnType): SshGitProvider { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: this harness implements every request, stream notification, disposal and notify method used by these provider calls. + return new SshGitProvider('review-host', mux as unknown as SshChannelMultiplexer) +} + +function createWireProvider() { + const mux = createMockMux() + const pending = new Map< + number, + { resolve: (value: unknown) => void; reject: (error: Error) => void } + >() + const notifications = new Map) => void>>() + const decoder = new FrameDecoder((frame) => { + if (frame.type !== MessageType.Regular) { + return + } + const message = parseJsonRpcMessage(frame.payload) + if ('method' in message) { + notifications.get(message.method)?.forEach((listener) => listener(message.params ?? {})) + return + } + const request = pending.get(message.id) + if (!request) { + return + } + pending.delete(message.id) + if (message.error) { + request.reject(Object.assign(new Error(message.error.message), { code: message.error.code })) + } else { + request.resolve(message.result) + } + }) + const dispatcher = new RelayDispatcher((frame) => decoder.feed(frame)) + const handler = new GitHandler(dispatcher, new RelayContext()) + let sequence = 0 + mux.request.mockImplementation((method: string, params: Record) => { + const id = ++sequence + return new Promise((resolve, reject) => { + pending.set(id, { resolve, reject }) + dispatcher.feed(encodeJsonRpcFrame({ jsonrpc: '2.0', id, method, params }, id, 0)) + }) + }) + mux.notify.mockImplementation((method: string, params: Record) => { + dispatcher.feed(encodeJsonRpcFrame({ jsonrpc: '2.0', method, params }, ++sequence, 0)) + }) + mux.onNotificationByMethod.mockImplementation( + (method: string, listener: (params: Record) => void) => { + const listeners = notifications.get(method) ?? new Set() + notifications.set(method, listeners) + listeners.add(listener) + return () => listeners.delete(listener) + } + ) + disposals.push(() => { + handler.dispose() + dispatcher.dispose() + pending.forEach((request) => request.reject(new Error('Test relay disposed'))) + pending.clear() + }) + return { provider: providerForMux(mux), mux, dispatcher, notifications } +} + +async function git(cwd: string, args: string[]) { + const result = await runProcess({ program: 'git', args, cwd, timeoutMs: 10_000 }) + if (result.code !== 0) { + throw Object.assign(new Error(result.stderr), { code: result.code }) + } + return { stdout: result.stdout, stderr: result.stderr } +} + +async function createReviewFixture(large: boolean) { + const root = createGitTempDir() + directories.push(root) + const globalConfig = join(root, 'global.gitconfig') + await writeFile(globalConfig, '') + // Newer desktop Git preferences may be rejected while a baseline fixture loads config. + vi.stubEnv('GIT_CONFIG_GLOBAL', globalConfig) + vi.stubEnv('GIT_CONFIG_NOSYSTEM', '1') + const remote = join(root, 'remote repo') + const worktree = join(root, 'review worktree') + await mkdir(remote) + gitInit(remote) + await git(remote, ['checkout', '-b', 'main']) + await writeFile(join(remote, 'README.md'), 'Original content\n') + gitCommit(remote, 'Initial base') + await git(root, ['clone', remote, worktree]) + const originalBase = (await git(worktree, ['rev-parse', 'HEAD'])).stdout.trim() + await writeFile(join(remote, 'base-only.txt'), 'Advance the remote base after cloning\n') + gitCommit(remote, 'Advance base') + await git(worktree, ['checkout', '-b', 'feature/review']) + await writeFile(join(worktree, 'README.md'), 'Committed review evidence\n') + await writeFile( + join(worktree, 'new file.txt'), + large + ? 'Committed line with unicode café for a streamed review.\n'.repeat(36_000) + : 'New evidence\n' + ) + gitCommit(worktree, 'Add review evidence') + await writeFile(join(worktree, 'README.md'), 'Uncommitted content must stay out of the draft\n') + return { worktree, originalBase } +} + +describe('SSH review draft through the real relay dispatcher', () => { + it.each([false, true])('matches complete local context with streamed patch=%s', async (large) => { + const { worktree, originalBase } = await createReviewFixture(large) + const local = await getPullRequestDraftContext((args) => git(worktree, args), input) + expect(local).toMatchObject({ + branch: 'feature/review', + commitSummary: '- Add review evidence', + changeSummary: expect.stringContaining('new file.txt'), + patch: expect.stringContaining('+Committed review evidence') + }) + await git(worktree, ['update-ref', 'refs/remotes/origin/main', originalBase]) + const { provider, mux, notifications } = createWireProvider() + + const remote = await getPullRequestDraftContext( + (args, options) => execSshReviewDraft(provider, args, worktree, options), + input + ) + + expect(remote).toEqual(local) + expect(remote?.patch).not.toContain('Uncommitted content') + expect(mux.request).toHaveBeenCalledWith('git.fetchRemoteTrackingRef', { + worktreePath: worktree, + remote: 'origin', + branch: 'main', + ref: 'refs/remotes/origin/main' + }) + expect(mux.request.mock.calls.filter(([method]) => method === 'git.reviewDiff')).toHaveLength(2) + expect( + mux.request.mock.calls.some( + ([method, params]) => + method === 'git.exec' && (params.args[0] === 'fetch' || params.args[0] === 'diff') + ) + ).toBe(false) + expect([...notifications.values()].every((listeners) => listeners.size === 0)).toBe(true) + if (large) { + expect(mux.notify).toHaveBeenCalledWith('git.responseAck', expect.any(Object)) + } + }) +}) + +function createContextProvider() { + const mux = createMockMux() + const runRequest = async (method: string, params: Record) => { + if (method === 'git.fetchRemoteTrackingRef') { + return undefined + } + if (method === 'git.reviewDiff') { + return { stdout: 'remote evidence', stderr: '' } + } + const args = params.args + if (!Array.isArray(args)) { + throw new Error('Missing context Git arguments') + } + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return { stdout: '', stderr: '' } + } + if (args[0] === 'branch') { + return { stdout: 'feature/review\n', stderr: '' } + } + if (args[0] === 'merge-base') { + return { stdout: `${mergeBase}\n`, stderr: '' } + } + if (args[0] === 'log') { + return { stdout: '- committed evidence\n', stderr: '' } + } + throw new Error(`Unexpected method ${method}`) + } + mux.request.mockImplementation(runRequest) + return { mux, provider: providerForMux(mux), runRequest } +} + +describe('SSH review draft failures and routing', () => { + it('rejects an old-host missing diff method instead of returning commit-only context', async () => { + const { mux, provider, runRequest } = createContextProvider() + mux.request.mockImplementation((method, params) => { + if (method === 'git.reviewDiff') { + return Promise.reject(Object.assign(new Error('Method not found'), { code: -32601 })) + } + return runRequest(method, params) + }) + + await expect( + getPullRequestDraftContext( + (args, options) => execSshReviewDraft(provider, args, '/repo', options), + input + ) + ).rejects.toThrow('Reconnect the SSH target') + }) + + it.each(['SSH connection closed', 'Git request timed out', 'git output exceeded maxBuffer.'])( + 'rejects incomplete context after %s', + async (message) => { + const { mux, provider, runRequest } = createContextProvider() + mux.request.mockImplementation((method, params) => { + if (method === 'git.reviewDiff') { + return Promise.reject(new Error(message)) + } + return runRequest(method, params) + }) + const draft = getPullRequestDraftContext( + (args, options) => execSshReviewDraft(provider, args, '/repo', options), + input + ) + await expect(draft).rejects.toBeInstanceOf(ReviewDraftContextError) + await expect(draft).rejects.toThrow(message) + } + ) + + it('stops before reading evidence when an old host lacks the narrow fetch method', async () => { + const { mux, provider, runRequest } = createContextProvider() + mux.request.mockImplementation((method, params) => { + if (method === 'git.fetchRemoteTrackingRef') { + return Promise.reject( + Object.assign(new Error('Method not found: git.fetchRemoteTrackingRef'), { code: -32601 }) + ) + } + return runRequest(method, params) + }) + await expect( + getPullRequestDraftContext( + (args, options) => execSshReviewDraft(provider, args, '/repo', options), + input + ) + ).rejects.toThrow('Fetch before generating PR details failed') + expect(mux.request.mock.calls.some(([method]) => method === 'git.reviewDiff')).toBe(false) + }) + + it.each([ + undefined, + null, + {}, + { stdout: 'patch' }, + { stdout: 3, stderr: '' }, + { stdout: '', stderr: null } + ])('refuses malformed remote evidence %j', async (reply) => { + const mux = createMockMux() + mux.request.mockResolvedValue(reply) + await expect(providerForMux(mux).readReviewDiff('/repo', mergeBase, 'patch')).rejects.toThrow( + ReviewDraftContextError + ) + }) + + it.each([{ timeout: 321 }, { timeoutMs: 654 }, { timeout: 321, timeoutMs: 654 }])( + 'forwards draft deadlines %j to the narrow diff request', + async (options) => { + const mux = createMockMux() + mux.request.mockResolvedValue({ stdout: 'patch', stderr: '' }) + await execSshReviewDraft( + providerForMux(mux), + ['diff', '--name-status', `${mergeBase}..HEAD`], + '/repo', + options + ) + expect(mux.request).toHaveBeenCalledWith( + 'git.reviewDiff', + { + worktreePath: '/repo', + mergeBase, + format: 'name-status', + __streamResponse: true + }, + { signal: undefined, timeoutMs: options.timeoutMs ?? options.timeout } + ) + } + ) + + it('keeps non-draft fetch and diff shapes on the restricted generic path', async () => { + const mux = createMockMux() + mux.request.mockResolvedValue({ stdout: '', stderr: '' }) + const provider = providerForMux(mux) + for (const args of [ + ['fetch', '--all'], + ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/heads/overwrite'], + ['diff', '--name-status', '--no-ext-diff', `${mergeBase}..HEAD`], + ['diff', '--patch', '--minimal', '--no-color', '--no-ext-diff', `${mergeBase}...HEAD`] + ]) { + await execSshReviewDraft(provider, args, '/repo', { timeoutMs: 456 }) + expect(mux.request).toHaveBeenLastCalledWith( + 'git.exec', + { + args, + cwd: '/repo', + __streamResponse: true + }, + { signal: undefined, timeoutMs: 456 } + ) + } + expect(mux.request.mock.calls.every(([method]) => method === 'git.exec')).toBe(true) + }) +}) diff --git a/src/main/providers/ssh-review-draft-context.ts b/src/main/providers/ssh-review-draft-context.ts new file mode 100644 index 00000000000..b3d7a913ab0 --- /dev/null +++ b/src/main/providers/ssh-review-draft-context.ts @@ -0,0 +1,35 @@ +import type { getPullRequestDraftContext } from '../text-generation/pull-request-context' +import type { SshGitProvider } from './ssh-git-provider' + +type DraftExecOptions = Parameters[0]>[1] + +export async function execSshReviewDraft( + provider: Pick, + args: string[], + cwd: string, + commandOptions?: DraftExecOptions +): Promise<{ stdout: string; stderr: string }> { + const timeoutMs = commandOptions?.timeoutMs ?? commandOptions?.timeout + const options = timeoutMs === undefined ? undefined : { timeoutMs } + if (args.length === 4 && args[0] === 'fetch' && args[1] === '--no-tags') { + const match = /^\+refs\/heads\/([^:]+):(.+)$/.exec(args[3]) + if (match && match[2] === `refs/remotes/${args[2]}/${match[1]}`) { + await provider.fetchRemoteTrackingRef(cwd, args[2], match[1], match[2]) + return { stdout: '', stderr: '' } + } + } + const range = /^([0-9a-f]{40}(?:[0-9a-f]{24})?)\.\.HEAD$/i.exec(args.at(-1) ?? '') + if (args[0] === 'diff' && range) { + const flags = args.slice(1, -1).join('\0') + const format = + flags === '--name-status' + ? 'name-status' + : flags === '--patch\0--minimal\0--no-color\0--no-ext-diff' + ? 'patch' + : null + if (format) { + return provider.readReviewDiff(cwd, range[1], format, options) + } + } + return options ? provider.exec(args, cwd, options) : provider.exec(args, cwd) +} diff --git a/src/main/runtime/rpc/methods/ai-vault-search.test.ts b/src/main/runtime/rpc/methods/ai-vault-search.test.ts index 902c65c9110..16f998e84e9 100644 --- a/src/main/runtime/rpc/methods/ai-vault-search.test.ts +++ b/src/main/runtime/rpc/methods/ai-vault-search.test.ts @@ -57,7 +57,9 @@ describe('session search runtime RPC', () => { query: 'needle', limit: 20, filters: { - agents: AI_VAULT_AGENTS.filter((agent) => agent !== 'qoder' && agent !== 'jcode') + agents: AI_VAULT_AGENTS.filter( + (agent) => !['codebuddy', 'zcode', 'qoder', 'jcode'].includes(agent) + ) } }, undefined @@ -70,7 +72,7 @@ describe('session search runtime RPC', () => { } ) it.each([undefined, 'runtime', 'mobile'] as const)( - 'preserves explicitly supported Qoder filters for client kind %s', + 'retains the current catalog for an attested client kind %s', async (clientKind) => { const service = fakeSearchService() setSessionSearchService(service) @@ -78,18 +80,42 @@ describe('session search runtime RPC', () => { await dispatcher().dispatch( request({ query: 'needle', - supportsQoderHistory: true, - filters: { agents: ['qoder', 'codex'] } + supportedAgents: [...AI_VAULT_AGENTS], + filters: { agents: ['jcode'] } }), { clientKind } ) ).toMatchObject({ ok: true }) expect(service.search).toHaveBeenCalledExactlyOnceWith( - { query: 'needle', limit: 20, filters: { agents: ['qoder', 'codex'] } }, + { query: 'needle', limit: 20, filters: { agents: ['jcode'] } }, undefined ) } ) + describe.each(['qoder', 'jcode'] as const)('%s history capability', (agent) => { + const supportField = agent === 'qoder' ? 'supportsQoderHistory' : 'supportsJcodeHistory' + it.each([undefined, 'runtime', 'mobile'] as const)( + 'preserves explicitly supported filters for client kind %s', + async (clientKind) => { + const service = fakeSearchService() + setSessionSearchService(service) + expect( + await dispatcher().dispatch( + request({ + query: 'needle', + [supportField]: true, + filters: { agents: [agent, 'codex'] } + }), + { clientKind } + ) + ).toMatchObject({ ok: true }) + expect(service.search).toHaveBeenCalledExactlyOnceWith( + { query: 'needle', limit: 20, filters: { agents: [agent, 'codex'] } }, + undefined + ) + } + ) + }) it('maps the old runtime dispatcher refusal and rejects malformed responses', async () => { const legacy = dispatcher(true) const client = createSessionSearchClient(async (method, params) => { diff --git a/src/main/runtime/runtime-git-generation-context.ts b/src/main/runtime/runtime-git-generation-context.ts index 74ad56a3818..7da7651ebec 100644 --- a/src/main/runtime/runtime-git-generation-context.ts +++ b/src/main/runtime/runtime-git-generation-context.ts @@ -9,6 +9,7 @@ import type { SourceControlAiOperation } from '../../shared/source-control-ai-ty import type { CommitMessageAgentRuntimeTarget } from '../text-generation/commit-message-agent-environment' import type { CommitMessageGenerationTarget } from '../text-generation/commit-message-text-generation' import type { PullRequestLinkedIssueMeta } from '../source-control/pull-request-linked-issue' +import { execSshReviewDraft } from '../providers/ssh-review-draft-context' import { localGitOptionsForTarget, type RuntimeGitCommandHost, @@ -28,12 +29,7 @@ export function pullRequestDraftGitExec( if (!provider) { throw new Error('ssh_git_provider_unavailable') } - return (argv, options) => { - const timeoutMs = options?.timeoutMs ?? options?.timeout - return timeoutMs === undefined - ? provider.exec(argv, target.worktree.path) - : provider.exec(argv, target.worktree.path, { timeoutMs }) - } + return (argv, options) => execSshReviewDraft(provider, argv, target.worktree.path, options) } return (argv, options) => gitExecFileAsync(argv, { diff --git a/src/main/text-generation/pull-request-context.ts b/src/main/text-generation/pull-request-context.ts index 74fa4e8c3b7..16b64048283 100644 --- a/src/main/text-generation/pull-request-context.ts +++ b/src/main/text-generation/pull-request-context.ts @@ -1,6 +1,7 @@ import type { PullRequestDraftContext } from '../../shared/pull-request-generation' import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' import { isSafeReviewHeadFetchRemote } from '../../shared/review-head-tracking-ref' +import { ReviewDraftContextError } from '../../shared/review-draft-context-error' import { canQueryRemoteBaseRefs, getPullRequestRemoteRefState, @@ -25,7 +26,10 @@ async function safeExec(execGit: GitExec, args: string[]): Promise { try { const { stdout } = await execGit(args, { maxBuffer: MAX_PULL_REQUEST_CONTEXT_BYTES }) return stdout.trim() - } catch { + } catch (error) { + if (error instanceof ReviewDraftContextError) { + throw error + } return '' } } diff --git a/src/main/zsh-scoped-histfile.live-shell.test.ts b/src/main/zsh-scoped-histfile.live-shell.test.ts index 566ce8ae4b1..68ee27d24e8 100644 --- a/src/main/zsh-scoped-histfile.live-shell.test.ts +++ b/src/main/zsh-scoped-histfile.live-shell.test.ts @@ -18,7 +18,7 @@ * Orca would not wrap cannot pass here by construction. */ import { execFileSync } from 'node:child_process' -import { mkdtempSync, rmSync } from 'node:fs' +import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, expect, it } from 'vitest' @@ -427,3 +427,42 @@ describe.skipIf(process.platform === 'win32')('the relay variant of the hook', ( }) ) }) + +describe.skipIf(process.platform === 'win32')('the real-zsh harness', () => { + itWithZsh( + 'waits for delayed first output before answering compinit and scoping history', + withHome( + { + '.zshenv': 'sleep 0.6\nfpath=("$HOME/insecure-completions" $fpath)\n', + '.zshrc': + 'if (( ! $+_comps )); then\n' + + ' autoload -Uz compinit\n' + + ' compinit -D\n' + + 'fi\n' + + 'export ORCA_TEST_COMPINIT_READY=$+_comps\n' + }, + async (home) => { + const completions = join(home, 'insecure-completions') + mkdirSync(completions) + chmodSync(completions, 0o777) + writeFileSync( + join(completions, '_orca_compinit_fixture'), + '#compdef orca_compinit_fixture\n' + ) + const scoped = join(home, 'scoped_history') + const { env } = launchPane(home, scoped) + + const { output, values } = await runZshPty({ + env, + report: ['HISTFILE', 'ORCA_TEST_COMPINIT_READY'] + }) + + expect(output).toContain('Ignore insecure directories') + expect(output).not.toContain('initialization aborted') + expect(output).not.toContain('command not found: y') + expect(values.ORCA_TEST_COMPINIT_READY).toBe('1') + expect(values.HISTFILE).toBe(scoped) + } + ) + ) +}) diff --git a/src/main/zsh-startup-hook-pty-harness.ts b/src/main/zsh-startup-hook-pty-harness.ts index a4de4e3bba9..0020be616fa 100644 --- a/src/main/zsh-startup-hook-pty-harness.ts +++ b/src/main/zsh-startup-hook-pty-harness.ts @@ -85,8 +85,8 @@ function parseValues(resultPath: string): Record { * Runs an interactive login zsh under a PTY, waits for its first prompt, runs * the requested commands, and reports the requested shell variables. * - * Readiness is detected by a sentinel baked into PS1 rather than a fixed sleep, - * so a slow prompt framework makes the run slower, never flaky. + * A PS1 sentinel confirms readiness after the first input; observed output + * quiescence gates that input but does not prove startup has finished. */ export async function runZshPty(options: ZshPtyOptions): Promise { const sentinel = '@@ORCA-PTY-READY@@' @@ -109,7 +109,7 @@ export async function runZshPty(options: ZshPtyOptions): Promise { let output = '' let answeredCompinit = false - let lastDataAt = Date.now() + let lastDataAt: number | null = null let resolveReady: (() => void) | undefined const ready = new Promise((resolve) => { resolveReady = resolve @@ -160,7 +160,8 @@ export async function runZshPty(options: ZshPtyOptions): Promise { */ async function waitForQuiet(quietMs: number): Promise { while (!hasExited) { - const idleFor = Date.now() - lastDataAt + // Silence before the first output is not evidence that startup has settled. + const idleFor = lastDataAt === null ? 0 : Date.now() - lastDataAt if (idleFor >= quietMs) { return } diff --git a/src/preload/api/ai-vault-bridge.ts b/src/preload/api/ai-vault-bridge.ts index 9089c82220c..9647f72d5dd 100644 --- a/src/preload/api/ai-vault-bridge.ts +++ b/src/preload/api/ai-vault-bridge.ts @@ -1,11 +1,6 @@ import { createSessionSearchClient } from '../../shared/ai-vault-search-client' import type { AiVaultSearchRequest, AiVaultSearchStatus } from '../../shared/ai-vault-search-types' -import { - ALL_EXECUTION_HOSTS_SCOPE, - LOCAL_EXECUTION_HOST_ID, - type ExecutionHostId, - type ExecutionHostScope -} from '../../shared/execution-host' +import type { ExecutionHostId, ExecutionHostScope } from '../../shared/execution-host' import { ipcRenderer } from 'electron' import type { AiVaultDeleteSessionArgs, @@ -23,17 +18,13 @@ import type { PreloadApi } from '../api-types' function searchClient( executionHostScope?: ExecutionHostScope ): ReturnType { - // `all` is merged by this desktop, which already redacted each remote leg. - const remote = - executionHostScope !== undefined && - executionHostScope !== LOCAL_EXECUTION_HOST_ID && - executionHostScope !== ALL_EXECUTION_HOSTS_SCOPE + // Main negotiates and redacts every remote leg before answering this same-build IPC boundary. return createSessionSearchClient( (method, params) => method === 'aiVault.searchSessions' ? ipcRenderer.invoke('aiVault:searchSessions', params, executionHostScope) : ipcRenderer.invoke('aiVault:searchStatus', executionHostScope), - remote ? 'relay' : 'ipc' + 'ipc' ) } diff --git a/src/relay/fs-handler-ripgrep-fallback.test.ts b/src/relay/fs-handler-ripgrep-fallback.test.ts index 36e49f87403..64cdcf601d8 100644 --- a/src/relay/fs-handler-ripgrep-fallback.test.ts +++ b/src/relay/fs-handler-ripgrep-fallback.test.ts @@ -1,25 +1,27 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import type * as ChildProcessModule from 'node:child_process' +import type * as RunProcessModule from '../shared/child-process/run-process' import type * as FsHandlerGitFallback from './fs-handler-git-fallback' import type * as FsHandlerUtils from './fs-handler-utils' const { - execFileMock, + runProcessMock, listFilesWithGitMock, + listFilesWithReaddirMock, listFilesWithRgMock, searchWithGitGrepMock, searchWithRgMock } = vi.hoisted(() => ({ - execFileMock: vi.fn(), + runProcessMock: vi.fn(), listFilesWithGitMock: vi.fn(), + listFilesWithReaddirMock: vi.fn(), listFilesWithRgMock: vi.fn(), searchWithGitGrepMock: vi.fn(), searchWithRgMock: vi.fn() })) -vi.mock('node:child_process', async (importOriginal) => ({ - ...(await importOriginal()), - execFile: execFileMock +vi.mock('../shared/child-process/run-process', async (importOriginal) => ({ + ...(await importOriginal()), + runProcess: runProcessMock })) vi.mock('./fs-handler-utils', async (importOriginal) => ({ @@ -34,11 +36,16 @@ vi.mock('./fs-handler-git-fallback', async (importOriginal) => ({ searchWithGitGrep: searchWithGitGrepMock })) +vi.mock('./fs-handler-readdir-fallback', () => ({ + listFilesWithReaddir: listFilesWithReaddirMock +})) + import { FileListingCancelledError } from '../shared/file-listing-cancellation' import { RipgrepUnavailableError } from '../shared/ripgrep-process-availability' import { RelayContext } from './context' import { FsHandler } from './fs-handler' import { runListFilesScan } from './fs-list-files-fallback-chain' +import { buildRelayCommandEnv } from './relay-command-env' type FsHandlerInternals = { search(params: Record): Promise @@ -81,20 +88,86 @@ describe('relay direct ripgrep admission', () => { expect(searchWithGitGrepMock).toHaveBeenCalledTimes(1) }) - it('falls back only for a tagged listing launch failure', async () => { + it.each(['true\n', 'false\n'])( + 'uses the Git listing after a successful probe (%s)', + async (stdout) => { + const controller = new AbortController() + listFilesWithRgMock.mockRejectedValueOnce(new RipgrepUnavailableError()) + runProcessMock.mockResolvedValueOnce({ + code: 0, + signal: null, + stdout, + stderr: '', + timedOut: false + }) + listFilesWithGitMock.mockResolvedValueOnce(['src/index.ts']) + + await expect(runListFilesScan('/repo', [], controller.signal)).resolves.toEqual([ + 'src/index.ts' + ]) + expect(listFilesWithRgMock).toHaveBeenCalledTimes(1) + expect(runProcessMock).toHaveBeenCalledWith({ + program: 'git', + args: ['rev-parse', '--is-inside-work-tree'], + cwd: '/repo', + env: buildRelayCommandEnv(), + timeoutMs: 5_000, + signal: controller.signal + }) + expect(listFilesWithGitMock).toHaveBeenCalledTimes(1) + expect(listFilesWithReaddirMock).not.toHaveBeenCalled() + } + ) + + it.each([ + { code: 128, signal: null, timedOut: false }, + { code: null, signal: 'SIGTERM', timedOut: false }, + { code: 0, signal: null, timedOut: true } + ] as const)('uses the directory walk when the Git probe fails (%j)', async (result) => { const controller = new AbortController() listFilesWithRgMock.mockRejectedValueOnce(new RipgrepUnavailableError()) - execFileMock.mockImplementationOnce((_command, _args, _options, callback) => { - callback(null) - return undefined - }) - listFilesWithGitMock.mockResolvedValueOnce(['src/index.ts']) + runProcessMock.mockResolvedValueOnce({ ...result, stdout: '', stderr: '' }) + listFilesWithReaddirMock.mockResolvedValueOnce(['src/index.ts']) - await expect(runListFilesScan('/repo', [], controller.signal)).resolves.toEqual([ + await expect(runListFilesScan('/folder', [], controller.signal)).resolves.toEqual([ 'src/index.ts' ]) - expect(listFilesWithRgMock).toHaveBeenCalledTimes(1) - expect(listFilesWithGitMock).toHaveBeenCalledTimes(1) + expect(listFilesWithGitMock).not.toHaveBeenCalled() + expect(listFilesWithReaddirMock).toHaveBeenCalledWith('/folder', [], { + signal: controller.signal, + maxResults: undefined + }) + }) + + it('uses the directory walk when Git cannot start', async () => { + const controller = new AbortController() + listFilesWithRgMock.mockRejectedValueOnce(new RipgrepUnavailableError()) + runProcessMock.mockRejectedValueOnce(new Error('spawn git ENOENT')) + listFilesWithReaddirMock.mockResolvedValueOnce(['src/index.ts']) + + await expect(runListFilesScan('/folder', [], controller.signal)).resolves.toEqual([ + 'src/index.ts' + ]) + expect(listFilesWithGitMock).not.toHaveBeenCalled() + expect(listFilesWithReaddirMock).toHaveBeenCalledTimes(1) + }) + + it.each([0, 128])('lets cancellation during the Git probe win its exit (%s)', async (code) => { + const controller = new AbortController() + const cancellation = new FileListingCancelledError('superseded') + const probe = Promise.withResolvers() + listFilesWithRgMock.mockRejectedValueOnce(new RipgrepUnavailableError()) + runProcessMock.mockReturnValueOnce(probe.promise) + + const scan = runListFilesScan('/repo', [], controller.signal) + await vi.waitFor(() => expect(runProcessMock).toHaveBeenCalledTimes(1)) + expect(runProcessMock.mock.calls[0][0].signal).toBe(controller.signal) + controller.abort(cancellation) + probe.resolve({ code, signal: null, stdout: '', stderr: '', timedOut: false }) + + await expect(scan).rejects.toBe(cancellation) + expect(listFilesWithGitMock).not.toHaveBeenCalled() + expect(listFilesWithReaddirMock).not.toHaveBeenCalled() }) it('lets cancellation win an unavailable-listing race before Git starts', async () => { @@ -106,8 +179,9 @@ describe('relay direct ripgrep admission', () => { }) await expect(runListFilesScan('/repo', [], controller.signal)).rejects.toBe(cancellation) - expect(execFileMock).not.toHaveBeenCalled() + expect(runProcessMock).not.toHaveBeenCalled() expect(listFilesWithGitMock).not.toHaveBeenCalled() + expect(listFilesWithReaddirMock).not.toHaveBeenCalled() }) it('requires ripgrep for bounded query ranking instead of retaining a full Git inventory', async () => { @@ -117,7 +191,7 @@ describe('relay direct ripgrep admission', () => { await expect(runListFilesScan('/repo', [], controller.signal, 33, 'target')).rejects.toThrow( 'Quick Open search requires ripgrep' ) - expect(execFileMock).not.toHaveBeenCalled() + expect(runProcessMock).not.toHaveBeenCalled() expect(listFilesWithGitMock).not.toHaveBeenCalled() }) }) diff --git a/src/relay/fs-list-files-fallback-chain.ts b/src/relay/fs-list-files-fallback-chain.ts index d60cfff80d5..5fab9cbf124 100644 --- a/src/relay/fs-list-files-fallback-chain.ts +++ b/src/relay/fs-list-files-fallback-chain.ts @@ -1,4 +1,4 @@ -import { execFile } from 'node:child_process' +import { runProcess } from '../shared/child-process/run-process' import { listFilesWithRg } from './fs-handler-utils' import { listFilesWithGit } from './fs-handler-git-fallback' import { listFilesWithReaddir } from './fs-handler-readdir-fallback' @@ -35,19 +35,18 @@ export async function runListFilesScan( if (searchQuery !== undefined) { throw new Error(await buildRipgrepRequiredMessage()) } - // Why: git ls-files only works inside git repos. Use rev-parse to detect - // git ancestry — unlike checking for a local .git entry, this works from - // subdirectories of a checkout (e.g. /repo/packages/app added as a folder). - // Without this, a git subdirectory would fall through to readdir and - // surface .gitignore'd build artifacts. - const isGitRepo = await new Promise((resolve) => { - execFile( - 'git', - ['rev-parse', '--is-inside-work-tree'], - { cwd: rootPath, env: buildRelayCommandEnv() }, - (err) => resolve(!err) - ) + // Detect Git ancestry so folder roots inside a checkout still honor its ignores. + const isGitRepo = await runProcess({ + program: 'git', + args: ['rev-parse', '--is-inside-work-tree'], + cwd: rootPath, + env: buildRelayCommandEnv(), + timeoutMs: 5_000, + signal }) + .then((result) => result.code === 0 && !result.timedOut) + .catch(() => false) + throwIfFileListingCancelled(signal) if (isGitRepo) { // Why: a git monorepo parent fills nested-repo subtrees via the readdir // walk, which can exhaust the same cap/deadline. Translate only those diff --git a/src/relay/git-handler-branch-cleanup.test.ts b/src/relay/git-handler-branch-cleanup.test.ts index 40607bdac0d..d1ab16bb6b4 100644 --- a/src/relay/git-handler-branch-cleanup.test.ts +++ b/src/relay/git-handler-branch-cleanup.test.ts @@ -1,3 +1,8 @@ +// Ref mutation tests use fictitious repositories; administrative reservations have real-Git coverage. +vi.mock('../shared/git-worktree-admin', () => ({ + isBranchReservedByWorktreeOperation: vi.fn().mockResolvedValue(false) +})) + import { describe, expect, it, vi } from 'vitest' import * as path from 'node:path' import { GitCapabilityCache } from '../shared/git-capability-cache' diff --git a/src/relay/git-handler-branch-cleanup.ts b/src/relay/git-handler-branch-cleanup.ts index c2bfcae599f..759269aba1d 100644 --- a/src/relay/git-handler-branch-cleanup.ts +++ b/src/relay/git-handler-branch-cleanup.ts @@ -5,7 +5,7 @@ import { import type { GitCapabilityCache } from '../shared/git-capability-cache' import type { GitExec } from './git-handler-ops' import { expandTilde } from './context' -import { isBranchInDetachedWorktree } from '../shared/git-worktree-admin' +import { isBranchReservedByWorktreeOperation } from '../shared/git-worktree-admin' import { parseWorktreeList } from '../shared/git-worktree-porcelain-parser' export async function deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure( @@ -106,6 +106,6 @@ async function isRelayBranchCheckedOut( (worktree) => typeof worktree.branch === 'string' && worktree.branch.replace(/^refs\/heads\//, '') === branchName - ) || isBranchInDetachedWorktree(expandTilde(repoPath), branchName, worktrees) + ) || isBranchReservedByWorktreeOperation(expandTilde(repoPath), branchName, worktrees) ) } diff --git a/src/relay/git-handler-branch-compare.test.ts b/src/relay/git-handler-branch-compare.test.ts index 38ff6bfc735..98c61c2928d 100644 --- a/src/relay/git-handler-branch-compare.test.ts +++ b/src/relay/git-handler-branch-compare.test.ts @@ -10,6 +10,41 @@ function deferred(): { promise: Promise; resolve: (value: T) => void } { } describe('relay branchCompare', () => { + it.each([40, 64])( + 'skips change and count reads for identical %i-character commit tips', + async (length) => { + const oid = 'a'.repeat(length) + const git = vi.fn(async (args) => { + if (args[0] === 'branch') { + return { stdout: 'feature\n', stderr: '' } + } + if (['rev-parse', 'merge-base'].includes(args[0] ?? '')) { + return { stdout: `${oid}\n`, stderr: '' } + } + throw new Error(`Unexpected git command: ${args.join(' ')}`) + }) + const loadBranchChanges = vi.fn(async () => []) + + await expect(branchCompare(git, '/repo', 'main', loadBranchChanges)).resolves.toEqual({ + summary: { + baseRef: 'main', + baseOid: oid, + compareRef: 'feature', + headOid: oid, + mergeBase: oid, + changedFiles: 0, + commitsAhead: 0, + commitsBehind: 0, + status: 'ready' + }, + entries: [] + }) + expect(git.mock.calls.some(([args]) => args[0] === 'merge-base')).toBe(true) + expect(loadBranchChanges).not.toHaveBeenCalled() + expect(git.mock.calls.some(([args]) => args[0] === 'rev-list')).toBe(false) + } + ) + it('launches independent Git reads before waiting for any result', async () => { const branch = deferred<{ stdout: string; stderr: string }>() const head = deferred<{ stdout: string; stderr: string }>() diff --git a/src/relay/git-handler-branch-diff.test.ts b/src/relay/git-handler-branch-diff.test.ts index ce60c8cd439..32ffd4a6475 100644 --- a/src/relay/git-handler-branch-diff.test.ts +++ b/src/relay/git-handler-branch-diff.test.ts @@ -45,25 +45,25 @@ describe('pinned relay branch diff operation', () => { it.each([ { name: 'addition', - status: `A\t${FILE_PATH}\n`, + status: `A\0${FILE_PATH}\0`, left: new Error('missing'), right: Buffer.from('added\n') }, { name: 'deletion', - status: `D\t${FILE_PATH}\n`, + status: `D\0${FILE_PATH}\0`, left: Buffer.from('deleted\n'), right: new Error('missing') }, { name: 'binary content', - status: `M\t${FILE_PATH}\n`, + status: `M\0${FILE_PATH}\0`, left: Buffer.from([0, 1]), right: Buffer.from([0, 2]) }, { name: 'blob read failure', - status: `M\t${FILE_PATH}\n`, + status: `M\0${FILE_PATH}\0`, left: new Error('left failed'), right: new Error('right failed') } @@ -174,7 +174,7 @@ describe('GitHandler pinned branch diff route', () => { }) } - function mockLegacyGit(nameStatus = '') { + function mockLegacyGit(rawDiff = '') { return vi.spyOn(handler as unknown as GitTarget, 'git').mockImplementation(async (args) => { if (args[0] === 'rev-parse' && args.includes('HEAD')) { return { stdout: `${HEAD_OID}\n`, stderr: '' } @@ -185,7 +185,7 @@ describe('GitHandler pinned branch diff route', () => { if (args[0] === 'merge-base') { return { stdout: `${MERGE_BASE_OID}\n`, stderr: '' } } - return { stdout: nameStatus, stderr: '' } + return { stdout: rawDiff, stderr: '' } }) } @@ -289,7 +289,7 @@ describe('GitHandler pinned branch diff route', () => { // Why: a symbolic base ref used to select the pinned route and then throw. it('selects the pinned route only when the base ref is a full object id', async () => { - const gitSpy = mockLegacyGit(`M\t${FILE_PATH}\n`) + const gitSpy = mockLegacyGit(`M\0${FILE_PATH}\0`) const gitBufferSpy = vi .spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') .mockResolvedValue(Buffer.from('content\n')) @@ -300,7 +300,7 @@ describe('GitHandler pinned branch diff route', () => { 'rev-parse --verify HEAD', 'rev-parse --verify origin/main', `merge-base ${BASE_OID} ${HEAD_OID}`, - `-c core.quotePath=false diff --name-status -M -C ${MERGE_BASE_OID} ${HEAD_OID}` + `diff --name-status -z -M -C ${MERGE_BASE_OID} ${HEAD_OID} --` ]) expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([ `${MERGE_BASE_OID}:${FILE_PATH}`, @@ -319,7 +319,7 @@ describe('GitHandler pinned branch diff route', () => { }) it('serves an explicitly null head OID through the legacy path', async () => { - const gitSpy = mockLegacyGit(`M\t${FILE_PATH}\n`) + const gitSpy = mockLegacyGit(`M\0${FILE_PATH}\0`) const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') await expect(request({ headOid: null, includePatch: false })).resolves.toEqual([ diff --git a/src/relay/git-handler-diff-read-coalescing.test.ts b/src/relay/git-handler-diff-read-coalescing.test.ts index 75fb7ea65c5..dd511545949 100644 --- a/src/relay/git-handler-diff-read-coalescing.test.ts +++ b/src/relay/git-handler-diff-read-coalescing.test.ts @@ -241,7 +241,10 @@ describe('GitHandler', () => { return { stdout: `${'a'.repeat(40)}\n`, stderr: '' } } if (args.includes('--name-status')) { - return { stdout: 'M\tsrc/file.ts\n', stderr: '' } + return { + stdout: `M\0src/file.ts\0`, + stderr: '' + } } throw new Error(`unexpected git args: ${args.join(' ')}`) }) @@ -362,7 +365,10 @@ describe('GitHandler', () => { return { stdout: `${'a'.repeat(40)}\n`, stderr: '' } } if (args.includes('--name-status')) { - return { stdout: 'M\tsrc/file.ts\n', stderr: '' } + return { + stdout: `M\0src/file.ts\0`, + stderr: '' + } } throw new Error(`unexpected git args: ${args.join(' ')}`) }) @@ -447,31 +453,32 @@ describe('GitHandler', () => { await expect(retry).rejects.toThrow('commitOid must be a full git object id') }) - // Why: regression for #1503 on git.branchDiff — branchDiffEntries is a separate quotePath=false path that must round-trip UTF-8. - it('preserves UTF-8 paths in branch-diff entries', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'base.txt'), 'base') - gitCommit(tmpDir, 'initial') + it.each(process.platform === 'win32' ? ['sample.md'] : ['sample.md', 'tab\tline\n"日本語.md'])( + 'preserves literal branch-diff path characters: %s', + async (name) => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'base.txt'), 'base') + gitCommit(tmpDir, 'initial') - const baseRef = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() + const baseRef = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: tmpDir, + encoding: 'utf-8' + }).trim() - execFileSync('git', ['checkout', '-b', 'feature'], { cwd: tmpDir, stdio: 'pipe' }) - const utf8Dir = path.join(tmpDir, 'docs', '日本語') - mkdirSync(utf8Dir, { recursive: true }) - writeFileSync(path.join(utf8Dir, 'sample.md'), 'hello') - gitCommit(tmpDir, 'feature commit') + execFileSync('git', ['checkout', '-b', 'feature'], { cwd: tmpDir, stdio: 'pipe' }) + const utf8Dir = path.join(tmpDir, 'docs', '日本語') + mkdirSync(utf8Dir, { recursive: true }) + writeFileSync(path.join(utf8Dir, name), 'hello') + gitCommit(tmpDir, 'feature commit') - const result = (await dispatcher.callRequest('git.branchDiff', { - worktreePath: tmpDir, - baseRef, - filePath: 'docs/日本語/sample.md' - })) as Record[] + const result = await dispatcher.callRequest('git.branchDiff', { + worktreePath: tmpDir, + baseRef, + filePath: `docs/日本語/${name}` + }) - // length===1 confirms the path filter matched the raw UTF-8 path; octal-quoted (default quotePath) wouldn't match. - expect(result).toHaveLength(1) - }) + expect(result).toHaveLength(1) + } + ) }) }) diff --git a/src/relay/git-handler-object-diff-operations.ts b/src/relay/git-handler-object-diff-operations.ts index f513741df04..c335deb0c5e 100644 --- a/src/relay/git-handler-object-diff-operations.ts +++ b/src/relay/git-handler-object-diff-operations.ts @@ -10,6 +10,29 @@ import { commitDiffEntry } from './git-handler-commit-diff-ops' import { stableInFlightKey } from '../shared/in-flight-promise-dedupe' export class GitHandlerObjectDiffOperations extends GitHandlerOperationContext { + async reviewDiff(params: Record, context?: RequestContext) { + const { worktreePath, mergeBase, format } = params + if ( + typeof worktreePath !== 'string' || + !worktreePath || + worktreePath.includes('\0') || + !isFullGitObjectId(mergeBase) || + (format !== 'name-status' && format !== 'patch') + ) { + throw new Error('Invalid review diff request.') + } + const flags = + format === 'name-status' + ? ['--name-status'] + : ['--patch', '--minimal', '--no-color', '--no-ext-diff'] + const result = await this.git(['diff', ...flags, `${mergeBase}..HEAD`, '--'], worktreePath, { + signal: context?.signal, + disableOptionalLocks: true + }) + context?.signal?.throwIfAborted() + return this.maybeStreamResponse(result, params, context) + } + async branchDiff(params: Record, context?: RequestContext) { const worktreePath = params.worktreePath as string const baseRef = params.baseRef as string diff --git a/src/relay/git-handler-ops.ts b/src/relay/git-handler-ops.ts index 788addc5bc8..4339808d652 100644 --- a/src/relay/git-handler-ops.ts +++ b/src/relay/git-handler-ops.ts @@ -7,7 +7,8 @@ */ import * as path from 'node:path' import { isMissingGitBlobPath } from '../shared/git-blob-absence' -import { bufferToBlob, parseBranchDiff } from './git-handler-utils' +import { bufferToBlob } from './git-handler-utils' +import { parseGitChangeList } from '../shared/git-change-list' import { buildDiffResult } from './git-diff-result' import { isGitBufferOverflowError, isGitReadInterruptedError } from './git-buffer-overflow' import { readWorkingDiffFile } from './git-working-file-read' @@ -228,6 +229,14 @@ export async function branchCompare( return { summary, entries: [] } } + // Git must confirm equal raw tips are the same commit before skipping the reads. + if (baseOid === headOid && mergeBase === headOid) { + summary.commitsAhead = 0 + summary.commitsBehind = 0 + summary.status = 'ready' + return { summary, entries: [] } + } + try { const [entries, { stdout: countOut }] = await Promise.all([ loadBranchChanges(mergeBase, headOid), @@ -273,12 +282,11 @@ export async function branchDiffEntries( return [] } - // Why: see core.quotePath rationale in getStatusOp — keep UTF-8 paths intact. const { stdout } = await git( - ['-c', 'core.quotePath=false', 'diff', '--name-status', '-M', '-C', mergeBase, headOid], + ['diff', '--name-status', '-z', '-M', '-C', mergeBase, headOid, '--'], worktreePath ) - const allChanges = parseBranchDiff(stdout) + const allChanges = parseGitChangeList(stdout, 'name-status') // Why: the IPC handler for single-file branch diff sends filePath/oldPath // to avoid reading blobs for every changed file — only the matched file. @@ -304,8 +312,8 @@ export async function branchDiffEntries( const results: Record[] = [] for (const change of changes) { - const fp = change.path as string - const oldP = (change.oldPath as string) ?? fp + const fp = change.path + const oldP = change.oldPath ?? fp try { const left = await readBlobAtOid(gitBuffer, worktreePath, mergeBase, oldP) const right = await readBlobAtOid(gitBuffer, worktreePath, headOid, fp) diff --git a/src/relay/git-handler-registration.ts b/src/relay/git-handler-registration.ts index 79bfa0c57b1..69eadd5a888 100644 --- a/src/relay/git-handler-registration.ts +++ b/src/relay/git-handler-registration.ts @@ -61,6 +61,7 @@ export function registerGitHandlers( handlers.sync.rebaseFromBase(p, context) ) dispatcher.onRequest('git.branchDiff', (p, context) => handlers.objectDiff.branchDiff(p, context)) + dispatcher.onRequest('git.reviewDiff', (p, context) => handlers.objectDiff.reviewDiff(p, context)) dispatcher.onRequest('git.commitDiff', (p, context) => handlers.objectDiff.commitDiff(p, context)) dispatcher.onRequest('git.listWorktrees', (p, context) => handlers.worktree.listWorktrees(p, context) diff --git a/src/relay/git-handler-review-draft.test.ts b/src/relay/git-handler-review-draft.test.ts new file mode 100644 index 00000000000..c28bd63e872 --- /dev/null +++ b/src/relay/git-handler-review-draft.test.ts @@ -0,0 +1,266 @@ +import { readFile, unlink, writeFile } from 'node:fs/promises' +import * as path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import * as processRunner from '../shared/child-process/run-process' +import { GitAdmissionScheduler } from '../shared/git-admission-scheduler' +import { GIT_READ_TIMEOUT_MS } from '../shared/git-command-timeout' +import type { GitHandler } from './git-handler' +import type { GitHandlerOperationHost } from './git-handler-operation-context' +import { + createGitHandlerRelay, + createGitTempDir, + removeGitTempDir +} from './git-handler-test-harness' +import { gitCommit, gitInit, type MockDispatcher } from './git-handler-test-setup' +import { _resetRelayGitAdmissionForTests } from './git-handler-command-termination' + +const BASE_OID = 'a'.repeat(40) +const commandResult = { stdout: 'M\tfile.txt\n', stderr: '' } +const processResult = { ...commandResult, code: 0, signal: null, timedOut: false } + +function checkedCommandResult(result: unknown): { stdout: string; stderr: string } { + if ( + !result || + typeof result !== 'object' || + !('stdout' in result) || + typeof result.stdout !== 'string' || + !('stderr' in result) || + typeof result.stderr !== 'string' + ) { + throw new Error('Expected the review diff command response') + } + return { stdout: result.stdout, stderr: result.stderr } +} + +describe.each([2, 4])('relay review draft diff with %i Git slots', (generalCap) => { + let dispatcher: MockDispatcher + let handler: GitHandler + let repo: string + let scheduler: GitAdmissionScheduler + + beforeEach(() => { + scheduler = new GitAdmissionScheduler({ generalCap, generalHeadroom: 0 }) + _resetRelayGitAdmissionForTests(scheduler) + repo = createGitTempDir() + ;({ dispatcher, handler } = createGitHandlerRelay()) + }) + + afterEach(async () => { + handler.dispose() + vi.restoreAllMocks() + try { + await removeGitTempDir(repo) + expect(scheduler.snapshot().queued).toBe(0) + expect( + Object.values(scheduler.snapshot().budgets).every( + (budget) => budget.baseUsed === 0 && budget.headroomUsed === 0 + ) + ).toBe(true) + } finally { + _resetRelayGitAdmissionForTests() + } + }) + + function reviewDiff(overrides: Record = {}, signal?: AbortSignal) { + return dispatcher.callRequest( + 'git.reviewDiff', + { worktreePath: repo, mergeBase: BASE_OID, format: 'name-status', ...overrides }, + { isStale: () => false, signal } + ) + } + + async function runRepoGit(args: string[]): Promise { + const result = await processRunner.runProcess({ program: 'git', args, cwd: repo }) + if (result.code !== 0) { + throw new Error(result.stderr || `Git fixture command failed: ${args[0]}`) + } + return result.stdout + } + + it.each([ + { worktreePath: undefined }, + { worktreePath: null }, + { worktreePath: 1 }, + { worktreePath: '' }, + { worktreePath: 'repo\0other' }, + { mergeBase: undefined }, + { mergeBase: null }, + { mergeBase: 'main' }, + { mergeBase: 'HEAD~1..HEAD' }, + { mergeBase: '--output=result.patch' }, + { mergeBase: 'a'.repeat(39) }, + { mergeBase: 'a'.repeat(41) }, + { mergeBase: 'a'.repeat(63) }, + { mergeBase: 'a'.repeat(65) }, + { mergeBase: 'g'.repeat(40) }, + { mergeBase: `${BASE_OID}\0` }, + { format: undefined }, + { format: null }, + { format: '' }, + { format: 'raw' }, + { format: ['patch'] }, + { format: '--output=result.patch' } + ])('rejects malformed request fields before running Git: %j', async (overrides) => { + const git = vi.fn().mockResolvedValue(commandResult) + Object.assign(handler, { git }) + + await expect(reviewDiff(overrides)).rejects.toThrow('Invalid review diff request.') + expect(git).not.toHaveBeenCalled() + }) + + it.each([ + { format: 'name-status', mergeBase: BASE_OID, flags: ['--name-status'] }, + { + format: 'patch', + mergeBase: 'A'.repeat(64), + flags: ['--patch', '--minimal', '--no-color', '--no-ext-diff'] + } + ])('routes $format through the fixed command with a full object id', async (request) => { + const git = vi.fn().mockResolvedValue(commandResult) + Object.assign(handler, { git }) + const controller = new AbortController() + + await expect( + reviewDiff( + { ...request, args: ['fetch', 'origin'], filePath: 'other.txt', timeout: 0 }, + controller.signal + ) + ).resolves.toEqual(commandResult) + expect(git).toHaveBeenCalledExactlyOnceWith( + ['diff', ...request.flags, `${request.mergeBase}..HEAD`, '--'], + repo, + { signal: controller.signal, disableOptionalLocks: true } + ) + }) + + it.each([ + ['fetch', 'origin'], + ['range-diff', 'HEAD~2..HEAD~1', 'HEAD~1..HEAD'], + ['diff', '--name-status', `${BASE_OID}..HEAD`], + ['diff', '--cached', '--patch', `${BASE_OID}..HEAD`] + ])('keeps generic git.exec restricted for %j', async (...args) => { + const git = vi.fn().mockResolvedValue(commandResult) + Object.assign(handler, { git }) + + await expect(dispatcher.callRequest('git.exec', { args, cwd: repo })).rejects.toThrow( + /not allowed|restricted to staged changes/ + ) + expect(git).not.toHaveBeenCalled() + }) + + it('passes the existing read timeout and cancellation signal to the process runner', async () => { + const runProcess = vi.spyOn(processRunner, 'runProcess').mockImplementation(async (spec) => { + spec.onChildTerminated?.() + return processResult + }) + const controller = new AbortController() + + await expect(reviewDiff({ timeout: 0 }, controller.signal)).resolves.toEqual(commandResult) + expect(runProcess).toHaveBeenCalledTimes(1) + const spec = runProcess.mock.calls[0]?.[0] + expect(spec?.program).toBe('git') + expect(spec?.args).toEqual(['diff', '--name-status', `${BASE_OID}..HEAD`, '--']) + expect(spec?.cwd).toBe(repo) + expect(spec?.signal).toBe(controller.signal) + expect(spec?.timeoutMs).toBe(GIT_READ_TIMEOUT_MS) + expect(spec?.terminationBarrier).toBe(true) + expect(spec?.env?.GIT_OPTIONAL_LOCKS).toBe('0') + }) + + it('rejects a read that crossed its timeout instead of returning a partial diff', async () => { + vi.spyOn(processRunner, 'runProcess').mockImplementation(async (spec) => { + spec.onChildTerminated?.() + return { ...processResult, timedOut: true } + }) + + await expect(reviewDiff()).rejects.toMatchObject({ timedOut: true }) + }) + + it('holds a timed-out read grant until the child termination is reported', async () => { + let reportTermination: (() => void) | undefined + vi.spyOn(processRunner, 'runProcess').mockImplementation(async (spec) => { + reportTermination = spec.onChildTerminated + expect(spec.terminationBarrier).toBe(true) + return { ...processResult, timedOut: true } + }) + + try { + await expect(reviewDiff()).rejects.toMatchObject({ timedOut: true }) + expect(reportTermination).toBeTypeOf('function') + expect(scheduler.snapshot().budgets.general?.baseUsed).toBe(1) + } finally { + reportTermination?.() + } + expect(scheduler.snapshot().budgets.general?.baseUsed).toBe(0) + }) + + it('rejects cancellation before spawning any Git process', async () => { + const runProcess = vi.spyOn(processRunner, 'runProcess') + const controller = new AbortController() + controller.abort() + + await expect(reviewDiff({}, controller.signal)).rejects.toMatchObject({ name: 'AbortError' }) + expect(runProcess).not.toHaveBeenCalled() + }) + + it('rejects a completed read when the request was cancelled during execution', async () => { + const controller = new AbortController() + vi.spyOn(processRunner, 'runProcess').mockImplementation(async (spec) => { + expect(spec.signal).toBe(controller.signal) + controller.abort() + spec.onChildTerminated?.() + return processResult + }) + + await expect(reviewDiff({}, controller.signal)).rejects.toMatchObject({ name: 'AbortError' }) + }) + + it('checks cancellation again before publishing a successful command response', async () => { + const controller = new AbortController() + const reason = new Error('Draft request cancelled') + const git = vi.fn(async () => { + controller.abort(reason) + return commandResult + }) + Object.assign(handler, { git }) + + await expect(reviewDiff({}, controller.signal)).rejects.toBe(reason) + }) + + it('reads only committed changes and preserves dirty files, the index, and HEAD', async () => { + gitInit(repo) + await writeFile(path.join(repo, 'file.txt'), 'base\n') + await writeFile(path.join(repo, 'removed.txt'), 'removed\n') + gitCommit(repo, 'base') + const mergeBase = (await runRepoGit(['rev-parse', 'HEAD'])).trim() + await writeFile(path.join(repo, 'file.txt'), 'committed\n') + await writeFile(path.join(repo, 'added.txt'), 'added\n') + await unlink(path.join(repo, 'removed.txt')) + gitCommit(repo, 'feature') + await writeFile(path.join(repo, 'file.txt'), 'staged\n') + await runRepoGit(['add', 'file.txt']) + await writeFile(path.join(repo, 'file.txt'), 'unstaged\n') + await writeFile(path.join(repo, 'untracked.txt'), 'untracked\n') + await runRepoGit(['config', 'color.ui', 'always']) + await runRepoGit(['config', 'diff.external', path.join(repo, 'must-not-run')]) + const indexBefore = await readFile(path.join(repo, '.git', 'index')) + const statusBefore = await runRepoGit(['status', '--porcelain']) + const headBefore = await runRepoGit(['rev-parse', 'HEAD']) + + const names = checkedCommandResult(await reviewDiff({ mergeBase })) + const patch = checkedCommandResult(await reviewDiff({ mergeBase, format: 'patch' })) + + expect(names.stdout).toBe('A\tadded.txt\nM\tfile.txt\nD\tremoved.txt\n') + expect(patch.stdout).toContain('-base\n+committed\n') + expect(patch.stdout).toContain('new file mode') + expect(patch.stdout).toContain('deleted file mode') + expect(patch.stdout).not.toMatch(/staged|unstaged|untracked/) + expect(patch.stdout).not.toContain('\u001b[') + expect(names.stderr).toBe('') + expect(patch.stderr).toBe('') + expect(await readFile(path.join(repo, '.git', 'index'))).toEqual(indexBefore) + expect(await runRepoGit(['status', '--porcelain'])).toBe(statusBefore) + expect(await runRepoGit(['rev-parse', 'HEAD'])).toBe(headBefore) + expect(await readFile(path.join(repo, 'file.txt'), 'utf8')).toBe('unstaged\n') + }) +}) diff --git a/src/relay/git-handler-utils.ts b/src/relay/git-handler-utils.ts index 1c667618b75..ab0d5a5439b 100644 --- a/src/relay/git-handler-utils.ts +++ b/src/relay/git-handler-utils.ts @@ -7,60 +7,8 @@ */ import * as path from 'node:path' import { isBinaryBuffer } from '../shared/binary-buffer' -import type { GitLineStats } from '../shared/git-uncommitted-line-stats' export { isUnsupportedWorktreeListZError } from '../shared/git-worktree-command-capabilities' -export function parseBranchStatusChar(char: string): string { - switch (char) { - case 'M': - return 'modified' - case 'A': - return 'added' - case 'D': - return 'deleted' - case 'R': - return 'renamed' - case 'C': - return 'copied' - default: - return 'modified' - } -} - -// ─── Branch diff parsing ───────────────────────────────────────────── - -/** - * Parse `git diff --name-status` output into structured change entries. - */ -export function parseBranchDiff( - stdout: string, - statsByPath = new Map() -): Record[] { - const entries: Record[] = [] - for (const line of stdout.split(/\r?\n/)) { - if (!line) { - continue - } - const parts = line.split('\t') - const rawStatus = parts[0] ?? '' - const status = parseBranchStatusChar(rawStatus[0] ?? 'M') - - if (rawStatus.startsWith('R') || rawStatus.startsWith('C')) { - const oldPath = parts[1] - const filePath = parts[2] - if (filePath) { - entries.push({ path: filePath, oldPath, status, ...statsByPath.get(filePath) }) - } - } else { - const filePath = parts[1] - if (filePath) { - entries.push({ path: filePath, status, ...statsByPath.get(filePath) }) - } - } - } - return entries -} - // ─── Binary / blob helpers ─────────────────────────────────────────── export const PREVIEWABLE_MIME: Record = { diff --git a/src/relay/git-review-draft-binary-compatibility.test.ts b/src/relay/git-review-draft-binary-compatibility.test.ts new file mode 100644 index 00000000000..bbc9fca7c59 --- /dev/null +++ b/src/relay/git-review-draft-binary-compatibility.test.ts @@ -0,0 +1,101 @@ +import { writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { runProcess } from '../shared/child-process/run-process' +import { + createGitHandlerRelay, + createGitTempDir, + removeGitTempDir +} from './git-handler-test-harness' + +const binary = process.env.ORCA_GIT_COMPAT_BINARY +const image = process.env.ORCA_GIT_COMPAT_IMAGE +const expectedVersion = process.env.ORCA_GIT_COMPAT_VERSION +const dockerUser = + typeof process.getuid === 'function' && typeof process.getgid === 'function' + ? ['--user', `${process.getuid()}:${process.getgid()}`] + : [] + +describe.skipIf(!binary && !image)('review draft real Git compatibility', () => { + let fixturePath = '' + let fixtureCwd = '' + + async function git(args: string[]): Promise<{ stdout: string; stderr: string }> { + const result = await runProcess({ + program: image ? 'docker' : (binary ?? 'git'), + args: image + ? [ + 'run', + '--rm', + '--network=none', + ...dockerUser, + '-v', + `${fixturePath}:/repo`, + '-w', + '/repo', + image, + '-c', + 'safe.directory=/repo', + ...args + ] + : args, + cwd: fixturePath, + env: { + PATH: process.env.PATH, + SystemRoot: process.env.SystemRoot, + GIT_EXEC_PATH: process.env.GIT_EXEC_PATH, + GIT_CONFIG_NOSYSTEM: '1', + GIT_TERMINAL_PROMPT: '0' + }, + timeoutMs: 30_000, + maxOutputBytes: 2 * 1024 * 1024 + }) + if (result.code !== 0 || result.signal || result.timedOut || result.outputTruncated) { + throw new Error(result.stderr || 'Git compatibility command failed.') + } + return { stdout: result.stdout, stderr: result.stderr } + } + + beforeAll(async () => { + fixturePath = createGitTempDir() + fixtureCwd = image ? '/repo' : fixturePath + expect((await git(['--version'])).stdout).toContain(`git version ${expectedVersion}`) + }) + afterAll(async () => { + if (fixturePath) { + await removeGitTempDir(fixturePath) + } + }) + it('reads complete review drafts through the relay fixed diff formats', async () => { + await git(['init', '-q']) + await git(['config', 'user.name', 'Review Compatibility']) + await git(['config', 'user.email', 'review@example.invalid']) + await writeFile(join(fixturePath, 'evidence.txt'), 'before\n') + await git(['add', 'evidence.txt']) + await git(['commit', '-qm', 'initial']) + const mergeBase = (await git(['rev-parse', 'HEAD'])).stdout.trim() + await writeFile(join(fixturePath, 'evidence.txt'), 'after\n') + await git(['commit', '-qam', 'review evidence']) + const before = (await git(['status', '--porcelain'])).stdout + const { dispatcher, handler } = createGitHandlerRelay() + Object.assign(handler, { git }) + try { + await expect( + dispatcher.callRequest('git.reviewDiff', { + worktreePath: fixtureCwd, + mergeBase, + format: 'name-status' + }) + ).resolves.toEqual({ stdout: 'M\tevidence.txt\n', stderr: '' }) + const patch = await dispatcher.callRequest('git.reviewDiff', { + worktreePath: fixtureCwd, + mergeBase, + format: 'patch' + }) + expect(patch).toMatchObject({ stdout: expect.stringContaining('-before\n+after\n') }) + expect((await git(['status', '--porcelain'])).stdout).toBe(before) + } finally { + handler.dispose() + } + }) +}) diff --git a/src/relay/session-search-transport.test.ts b/src/relay/session-search-transport.test.ts index 9b9a1cb676d..69d2a736974 100644 --- a/src/relay/session-search-transport.test.ts +++ b/src/relay/session-search-transport.test.ts @@ -58,7 +58,9 @@ describe('session search over real relay frames', () => { query: 'needle', limit: 20, filters: { - agents: AI_VAULT_AGENTS.filter((agent) => agent !== 'qoder' && agent !== 'jcode') + agents: AI_VAULT_AGENTS.filter( + (agent) => !['codebuddy', 'zcode', 'qoder', 'jcode'].includes(agent) + ) } }, undefined diff --git a/src/renderer/src/web/preload-api/web-ai-vault-search-compat.test.ts b/src/renderer/src/web/preload-api/web-ai-vault-search-compat.test.ts index e81506967e3..6e0ff9520c0 100644 --- a/src/renderer/src/web/preload-api/web-ai-vault-search-compat.test.ts +++ b/src/renderer/src/web/preload-api/web-ai-vault-search-compat.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { searchResults } from '../../../../shared/ai-vault-search-test-fixture' import { unavailableSessionSearchStatus } from '../../../../shared/ai-vault-search-client' +import { AI_VAULT_AGENTS } from '../../../../shared/ai-vault-types' const callRuntimeResult = vi.hoisted(() => vi.fn()) vi.mock('./web-runtime-calls', () => ({ callRuntimeResult })) @@ -21,6 +22,7 @@ describe('web session search preload compatibility', () => { expect(callRuntimeResult).toHaveBeenCalledExactlyOnceWith('aiVault.searchSessions', { query: 'needle', limit: 20, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) diff --git a/src/shared/ai-vault-search-agent-compatibility.ts b/src/shared/ai-vault-search-agent-compatibility.ts new file mode 100644 index 00000000000..ecda310a04e --- /dev/null +++ b/src/shared/ai-vault-search-agent-compatibility.ts @@ -0,0 +1,48 @@ +import type { AiVaultAgent } from './ai-vault-types' +import type { AiVaultSearchStatus } from './ai-vault-search-types' + +// Frozen to the v1.4.211 parser; extending the current catalog must not widen old-host requests. +const LEGACY_SEARCH_AGENTS: readonly AiVaultAgent[] = [ + 'claude', + 'codex', + 'hermes', + 'pi', + 'omp', + 'prime-agent', + 'cursor', + 'gemini', + 'antigravity', + 'rovo', + 'copilot', + 'opencode', + 'opencode2', + 'grok', + 'openclaw', + 'devin', + 'droid', + 'cline', + 'kimi', + 'muse' +] + +export function needsSearchAgentNegotiation(agents: readonly AiVaultAgent[]): boolean { + return agents.some((agent) => !LEGACY_SEARCH_AGENTS.includes(agent)) +} + +export function compatibleSearchAgents( + agents: readonly AiVaultAgent[], + status: Pick< + AiVaultSearchStatus, + 'supportedAgents' | 'supportsQoderHistory' | 'supportsJcodeHistory' + > +): AiVaultAgent[] { + // Qoder's shipped capability also proves the earlier CodeBuddy and ZCode enum additions. + const supported = new Set( + status.supportedAgents ?? [ + ...LEGACY_SEARCH_AGENTS, + ...(status.supportsQoderHistory === true ? ['codebuddy', 'zcode', 'qoder'] : []), + ...(status.supportsJcodeHistory === true ? ['jcode'] : []) + ] + ) + return agents.filter((agent) => supported.has(agent)) +} diff --git a/src/shared/ai-vault-search-agent-history.test.ts b/src/shared/ai-vault-search-agent-history.test.ts index 1eb4f9deb7d..0d31393e8ed 100644 --- a/src/shared/ai-vault-search-agent-history.test.ts +++ b/src/shared/ai-vault-search-agent-history.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import { createSessionSearchClient, unavailableSessionSearchStatus } from './ai-vault-search-client' import { AiVaultSearchRequestSchema as LegacyRequestSchema } from './__fixtures__/pre-qoder-search-request' import { searchHit, searchResults } from './ai-vault-search-test-fixture' +import { AI_VAULT_AGENTS } from './ai-vault-types' describe.each(['qoder', 'jcode'] as const)('%s search negotiation', (agent) => { const supportField = agent === 'qoder' ? 'supportsQoderHistory' : 'supportsJcodeHistory' @@ -88,6 +89,7 @@ describe.each(['qoder', 'jcode'] as const)('%s search negotiation', (agent) => { query: 'q', limit: 20, filters: { agents: [agent] }, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) @@ -110,6 +112,7 @@ describe.each(['qoder', 'jcode'] as const)('%s search negotiation', (agent) => { query: 'q', limit: 20, filters: { agents: ['codex'] }, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) @@ -125,6 +128,7 @@ describe.each(['qoder', 'jcode'] as const)('%s search negotiation', (agent) => { query: 'q', limit: 20, filters: { agents: [agent] }, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) @@ -142,4 +146,66 @@ describe.each(['qoder', 'jcode'] as const)('%s search negotiation', (agent) => { ).rejects.toThrow('host disconnected') expect(call).toHaveBeenCalledExactlyOnceWith('aiVault.searchStatus', {}) }) + + it.each(['runtime', 'relay'] as const)( + 'does not send sole Jcode to a host that only attests Qoder over %s', + async (transport) => { + const call = vi.fn(async () => ({ + ...unavailableSessionSearchStatus(), + supportsQoderHistory: true + })) + expect( + await createSessionSearchClient(call, transport).searchSessions({ + query: 'q', + filters: { agents: ['jcode'] } + }) + ).toEqual({ kind: 'unavailable', reason: 'unsupported-agent' }) + expect(call).toHaveBeenCalledExactlyOnceWith('aiVault.searchStatus', {}) + } + ) + + it.each(['runtime', 'relay'] as const)( + 'retains current Jcode support even with no indexed sessions over %s', + async (transport) => { + const call = vi.fn(async (method: string) => + method === 'aiVault.searchStatus' + ? { + ...unavailableSessionSearchStatus(), + supportedAgents: [...AI_VAULT_AGENTS, 'future-agent'], + sessionsByAgent: {} + } + : { ...searchResults(), hits: [{ ...searchHit(), agent: 'jcode' }] } + ) + expect( + await createSessionSearchClient(call, transport).searchSessions({ + query: 'q', + filters: { agents: ['jcode'] } + }) + ).toMatchObject({ hits: [{ agent: 'jcode' }] }) + expect(call).toHaveBeenLastCalledWith('aiVault.searchSessions', { + query: 'q', + limit: 20, + filters: { agents: ['jcode'] }, + supportedAgents: [...AI_VAULT_AGENTS], + supportsQoderHistory: true, + supportsJcodeHistory: true + }) + } + ) + + it('uses an explicit host catalog before either historical capability', async () => { + const call = vi.fn(async () => ({ + ...unavailableSessionSearchStatus(), + supportedAgents: [], + supportsQoderHistory: true, + supportsJcodeHistory: true + })) + expect( + await createSessionSearchClient(call, 'relay').searchSessions({ + query: 'q', + filters: { agents: ['qoder', 'jcode'] } + }) + ).toEqual({ kind: 'unavailable', reason: 'unsupported-agent' }) + expect(call).toHaveBeenCalledExactlyOnceWith('aiVault.searchStatus', {}) + }) }) diff --git a/src/shared/ai-vault-search-client.ts b/src/shared/ai-vault-search-client.ts index 255e434f2d9..c00b27e6b2c 100644 --- a/src/shared/ai-vault-search-client.ts +++ b/src/shared/ai-vault-search-client.ts @@ -13,6 +13,11 @@ import { redactStatusForTransport, type SessionSearchTransport } from './ai-vault-search-transport' +import { + compatibleSearchAgents, + needsSearchAgentNegotiation +} from './ai-vault-search-agent-compatibility' +import { AI_VAULT_AGENTS } from './ai-vault-types' export function unavailableSessionSearchStatus(): AiVaultSearchStatus { return { @@ -52,23 +57,19 @@ export function createSessionSearchClient( // IPC and its all-hosts merge are this build; each remote leg negotiates its own host. if ( transport !== 'ipc' && - parsed.filters?.agents?.some((agent) => agent === 'qoder' || agent === 'jcode') + parsed.filters?.agents && + needsSearchAgentNegotiation(parsed.filters.agents) ) { const status = AiVaultSearchStatusSchema.parse(await call('aiVault.searchStatus', {})) - const agents = parsed.filters.agents.filter( - (agent) => - (agent !== 'qoder' || status.supportsQoderHistory === true) && - (agent !== 'jcode' || status.supportsJcodeHistory === true) - ) + const agents = compatibleSearchAgents(parsed.filters.agents, status) if (agents.length === 0) { return { kind: 'unavailable', reason: 'unsupported-agent' } } - if (agents.length !== parsed.filters.agents.length) { - hostRequest = { ...parsed, filters: { ...parsed.filters, agents } } - } + hostRequest = { ...parsed, filters: { ...parsed.filters, agents } } } raw = await call('aiVault.searchSessions', { ...hostRequest, + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) diff --git a/src/shared/ai-vault-search-contract.test.ts b/src/shared/ai-vault-search-contract.test.ts index 5ecfee93aec..08ced82accd 100644 --- a/src/shared/ai-vault-search-contract.test.ts +++ b/src/shared/ai-vault-search-contract.test.ts @@ -57,6 +57,20 @@ describe('session search public contract', () => { }).success ).toBe(false) }) + it('accepts future agent names only in optional capability lists', () => { + const supportedAgents = ['codex', 'jcode', 'future-agent'] + expect( + AiVaultSearchRequestSchema.parse({ query: 'q', supportedAgents }).supportedAgents + ).toEqual(supportedAgents) + expect( + AiVaultSearchStatusSchema.parse({ ...unavailableSessionSearchStatus(), supportedAgents }) + .supportedAgents + ).toEqual(supportedAgents) + expect( + AiVaultSearchRequestSchema.safeParse({ query: 'q', filters: { agents: ['future-agent'] } }) + .success + ).toBe(false) + }) it('never accepts resume commands for an unverified or missing source', () => { for (const presence of ['unverifiable', 'missing'] as const) { const response = searchResults() diff --git a/src/shared/ai-vault-search-contract.ts b/src/shared/ai-vault-search-contract.ts index ec9caf5f3d6..18a2c6df1cc 100644 --- a/src/shared/ai-vault-search-contract.ts +++ b/src/shared/ai-vault-search-contract.ts @@ -23,6 +23,7 @@ export const AiVaultSearchRequestSchema = z limit: z.number().optional().transform(resolveSessionSearchLimit), cursor: z.string().optional(), filters: AiVaultSearchFiltersSchema.optional(), + supportedAgents: z.array(z.string()).optional(), supportsQoderHistory: z.boolean().optional(), supportsJcodeHistory: z.boolean().optional(), /** Scope by identity, resolved into paths by whichever host answers. */ @@ -135,6 +136,8 @@ export const AiVaultSearchStatusRequestSchema = z.object({}) /** Consent flip for one host's index. Answered with that host's status after the change is applied. */ export const AiVaultSetSearchEnabledParamsSchema = z.object({ enabled: z.boolean() }) export const AiVaultSearchStatusSchema = z.object({ + // Strings keep a future host's larger catalog readable by this client. + supportedAgents: z.array(z.string()).optional(), supportsQoderHistory: z.boolean().optional(), supportsJcodeHistory: z.boolean().optional(), enabled: z.boolean(), diff --git a/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt b/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt index c0a268d1680..2e086367d8b 100644 --- a/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt +++ b/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt @@ -162,7 +162,6 @@ src/relay/ai-vault-service-spawn.ts src/relay/external-automations-handler.ts src/relay/fs-handler-git-fallback.ts src/relay/fs-handler-list-files.ts -src/relay/fs-list-files-fallback-chain.ts src/relay/preflight-handler.ts src/relay/pty-shell-utils.ts src/relay/subprocess-tree-termination.ts diff --git a/src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt b/src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt index d94df517b43..71bd79c7221 100644 --- a/src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt +++ b/src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt @@ -52,7 +52,6 @@ main/zsh-startup-hook-pty-harness.ts relay/agent-exec-handler.ts relay/external-automations-handler.ts relay/fs-handler-git-fallback.ts -relay/fs-list-files-fallback-chain.ts relay/pty-shell-utils.ts relay/subprocess-tree-termination.ts relay/workspace-space-scan.ts diff --git a/src/shared/child-process/windows-console-visibility.test.ts b/src/shared/child-process/windows-console-visibility.test.ts index 128d790d5ce..267c6815db8 100644 --- a/src/shared/child-process/windows-console-visibility.test.ts +++ b/src/shared/child-process/windows-console-visibility.test.ts @@ -34,7 +34,7 @@ const ALLOWLIST: readonly string[] = readAllowlist( * the allowlist does not bound this: a swap (one file fixed and delisted, one * new file added with its entry) satisfies both membership assertions. */ -const UNHIDDEN_SPAWNER_PIN = 59 +const UNHIDDEN_SPAWNER_PIN = 58 const CHILD_PROCESS_IMPORT = /from\s+['"](?:node:)?child_process['"]|require\(\s*['"](?:node:)?child_process['"]/ diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index 2e251d73d85..139990cd582 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -200,6 +200,19 @@ describeBinaryCompatibility('real Git binary compatibility', () => { expect(parseGitChangeList(root.stdout)).toEqual([ { path: 'tracked.txt', status: 'added', added: 1, removed: 0 } ]) + const names = await runGit([ + 'diff-tree', + '--root', + '--no-commit-id', + '-r', + '--name-status', + '-z', + head, + '--' + ]) + expect(parseGitChangeList(names.stdout, 'name-status')).toEqual([ + { path: 'tracked.txt', status: 'added' } + ]) }) it('reads signed history without launching configured signature verification', async () => { @@ -394,12 +407,21 @@ describeBinaryCompatibility('real Git binary compatibility', () => { await expect(readFile(lockPath, 'utf8')).resolves.toBe(lockReason) await expect(readFile(join(repoPath, 'compat-prepared', 'tracked.txt'))).rejects.toThrow() await runGit(['-C', 'compat-prepared', 'reset', '--hard', 'HEAD']) - expect( - (await runGit(['-C', 'compat-prepared', 'rev-parse', '--git-path', 'locked'])).stdout.trim() - ).toContain('worktrees/compat-prepared/locked') - expect( - (await runGit(['-C', 'compat-prepared', 'rev-parse', '--git-common-dir'])).stdout.trim() - ).toContain('.git') + const lockPointers = await runGit([ + '-C', + 'compat-prepared', + 'rev-parse', + '--git-path', + 'locked', + '--git-common-dir' + ]) + const pointerLines = lockPointers.stdout.split('\n') + expect(pointerLines).toHaveLength(3) + expect(pointerLines[0]?.replace(/\r$/, '').replaceAll('\\', '/')).toMatch( + /\.git\/worktrees\/compat-prepared\/locked$/ + ) + expect(pointerLines[1]?.replace(/\r$/, '').replaceAll('\\', '/')).toMatch(/(?:^|\/)\.git$/) + expect(pointerLines[2]).toBe('') // Why: `-f -f` moves a locked preparation while preserving its lock reason (Git >=2.25). await runGit(['worktree', 'move', '-f', '-f', 'compat-prepared', 'compat-final']) await runGit([ diff --git a/src/shared/git-change-list.test.ts b/src/shared/git-change-list.test.ts index cf6fd2ff96a..13f252a5ab7 100644 --- a/src/shared/git-change-list.test.ts +++ b/src/shared/git-change-list.test.ts @@ -11,6 +11,21 @@ const raw = (status: string, ...paths: string[]): string => `:100644 100644 abc def ${status}\0${paths.join('\0')}\0` describe('Git change lists', () => { + it('reads compact NUL name/status records with literal paths and rename pairs', () => { + const path = ':tab\tnewline\n"日本語" => file' + const records = `M\0${path}\0R100\0old\0new\0C080\0source\0copy\0T\0type\0` + expect(parseGitChangeList(records, 'name-status')).toEqual([ + { path, status: 'modified' }, + { path: 'new', oldPath: 'old', status: 'renamed' }, + { path: 'copy', oldPath: 'source', status: 'copied' }, + { path: 'type', status: 'modified' } + ]) + expect(parseGitChangeList('', 'name-status')).toEqual([]) + expect(() => parseGitChangeList('R100\0old\0', 'name-status')).toThrow('Incomplete') + expect(() => parseGitChangeList('invalid\0path\0', 'name-status')).toThrow('Invalid') + expect(() => parseGitChangeList('M\0\0', 'name-status')).toThrow('Missing') + }) + it('preserves delimiters, quotes, Unicode and rename markers in paths', () => { const name = ':tab\tnewline\n"日本語" => file' const oldPath = 'old\t\nfile' @@ -101,6 +116,12 @@ describe('Git change lists', () => { ]) ) expect(entries).toHaveLength(6) + expect( + parseGitChangeList( + await git(['diff', '--name-status', '-z', '-M', '-C', base, head, '--']), + 'name-status' + ) + ).toEqual(entries.map(({ added: _added, removed: _removed, ...entry }) => entry)) expect(parseGitChangeList(await git(gitChangeListArgs(head, head)))).toEqual([]) } finally { await rm(repo, { recursive: true, force: true }) diff --git a/src/shared/git-change-list.ts b/src/shared/git-change-list.ts index 72ff5fdbf89..5e6cb0be4e1 100644 --- a/src/shared/git-change-list.ts +++ b/src/shared/git-change-list.ts @@ -16,7 +16,10 @@ export function gitChangeListArgs(fromOid: string | null, toOid: string): string : ['diff-tree', '--root', '--no-commit-id', '-r', ...format, toOid, '--'] } -export function parseGitChangeList(stdout: string): GitBranchChangeEntry[] { +export function parseGitChangeList( + stdout: string, + format: 'raw' | 'name-status' = 'raw' +): GitBranchChangeEntry[] { const entries: GitBranchChangeEntry[] = [] let offset = 0 function readField(): string { @@ -30,9 +33,12 @@ export function parseGitChangeList(stdout: string): GitBranchChangeEntry[] { } // Raw records precede numstat records; filenames are separate NUL-delimited fields. - while (stdout[offset] === ':') { + while (offset < stdout.length && (format === 'name-status' || stdout[offset] === ':')) { const header = readField() - const match = /^:[0-7]{6} [0-7]{6} [0-9a-f]+ [0-9a-f]+ ([A-Z])\d*$/.exec(header) + const match = + format === 'name-status' + ? /^([A-Z])\d*$/.exec(header) + : /^:[0-7]{6} [0-7]{6} [0-9a-f]+ [0-9a-f]+ ([A-Z])\d*$/.exec(header) if (!match) { throw new Error('Invalid Git change record') } @@ -49,6 +55,9 @@ export function parseGitChangeList(stdout: string): GitBranchChangeEntry[] { ...(oldPath === undefined ? {} : { oldPath }) }) } + if (offset === stdout.length) { + return entries + } const statsByPath = parseNumstat(stdout.slice(offset)) return entries.map((entry) => ({ ...entry, ...statsByPath.get(entry.path) })) } diff --git a/src/shared/git-resolution-binary-compatibility.test-cases.ts b/src/shared/git-resolution-binary-compatibility.test-cases.ts index 807c66984d9..dbfc14b285e 100644 --- a/src/shared/git-resolution-binary-compatibility.test-cases.ts +++ b/src/shared/git-resolution-binary-compatibility.test-cases.ts @@ -65,7 +65,7 @@ export function registerGitResolutionBinaryCompatibilityCases( ) expect( buildGitSshPolicyEnv({}, overridden.command, overridden.variant).env.GIT_SSH_COMMAND - ).toBe("ssh -i 'key with spaces' -o BatchMode=yes") + ).toBe('ssh -o BatchMode=yes -i "key with spaces"') }) it('resolves default bases from exact refs with symbolic chains and dangling targets', async () => { diff --git a/src/shared/git-ssh-policy-env.test.ts b/src/shared/git-ssh-policy-env.test.ts index 41cdace5f05..1e0cc53eece 100644 --- a/src/shared/git-ssh-policy-env.test.ts +++ b/src/shared/git-ssh-policy-env.test.ts @@ -1,3 +1,8 @@ +import { chmod, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { runProcess } from './child-process/run-process' +import { quotePosixShell } from './wsl-login-shell-command' import { describe, expect, it } from 'vitest' import { buildGitSshPolicyEnv, parseGitSshConfig } from './git-ssh-policy-env' @@ -46,7 +51,12 @@ describe('Git network SSH policy', () => { 'ssh -i key\nrecord-access', 'ssh -i key && record-access', 'wrapper --account work', - 'plink.exe -i key' + 'plink.exe -i key', + 'ssh -i key | record-access', + 'ssh -i key > log', + 'ssh -i "unterminated', + 'ssh -o "ProxyCommand=proxy `date`"', + 'ssh\\ -i key' ])('preserves shell and wrapper semantics (%s)', (command) => { expect(buildGitSshPolicyEnv({}, command).env.GIT_SSH_COMMAND).toBeUndefined() }) @@ -57,6 +67,128 @@ describe('Git network SSH policy', () => { {}, '"C:/Program Files/Git/usr/bin/ssh.exe" -i "C:/Users/test/key file" -oBatchMode=no' ).env.GIT_SSH_COMMAND - ).toBe("'C:/Program Files/Git/usr/bin/ssh.exe' -i 'C:/Users/test/key file' -o BatchMode=yes") + ).toBe( + '"C:/Program Files/Git/usr/bin/ssh.exe" -o BatchMode=yes -i "C:/Users/test/key file" -oBatchMode=no' + ) + }) + + it('preserves a bare UNC executable path', () => { + const executable = String.raw`\\server\share\ssh.exe` + expect(buildGitSshPolicyEnv({}, `${executable} -i key`).env.GIT_SSH_COMMAND).toBe( + `${quotePosixShell(executable)} -o BatchMode=yes -i key` + ) + }) +}) + +describe.skipIf(process.platform === 'win32')('SSH policy with a real POSIX shell', () => { + it.each([ + [String.raw` -i C:\keys\work`, ['-i', String.raw`C:\keys\work`]], + [String.raw` -i C:\keys\work\&key`, ['-i', String.raw`C:\keys\work&key`]], + [String.raw` -i C:\keys\work\;key`, ['-i', String.raw`C:\keys\work;key`]], + [String.raw` -i C:\keys\work\|key`, ['-i', String.raw`C:\keys\work|key`]], + [String.raw` -i C:\keys\work\key`, ['-i', String.raw`C:\keys\work>key`]], + [String.raw` -i C:\keys\work\(key`, ['-i', String.raw`C:\keys\work(key`]], + [String.raw` -i C:\keys\work\)key`, ['-i', String.raw`C:\keys\work)key`]], + [' -i C:\\keys\\work\\`key', ['-i', 'C:\\keys\\work`key']], + [String.raw` -iC:\keys\work`, [String.raw`-iC:\keys\work`]], + [String.raw` -i \\server\share\work`, ['-i', String.raw`\\server\share\work`]], + [String.raw` -i \\\\server\share\work`, ['-i', String.raw`\\server\share\work`]], + [String.raw` -i\\server\share\work`, [String.raw`-i\\server\share\work`]], + [ + String.raw` -oIdentityFile=\\server\share\work`, + [String.raw`-oIdentityFile=\\server\share\work`] + ], + [ + String.raw` -o IdentityFile=\\server\share\work`, + ['-o', String.raw`IdentityFile=\\server\share\work`] + ], + [String.raw` -FC:\ssh\config`, [String.raw`-FC:\ssh\config`]], + [String.raw` -oIdentityFile=C:\keys\work`, [String.raw`-oIdentityFile=C:\keys\work`]], + [String.raw` -o IdentityFile=C:\keys\work`, ['-o', String.raw`IdentityFile=C:\keys\work`]], + [String.raw` -o "IdentityFile=C:\keys\work"`, ['-o', String.raw`IdentityFile=C:\keys\work`]], + [ + String.raw` -o 'ProxyCommand=C:\bin\proxy %h'`, + ['-o', String.raw`ProxyCommand=C:\bin\proxy %h`] + ] + ])('retains unquoted Windows option paths: %s', async (suffix, expected) => { + const command = buildGitSshPolicyEnv({}, `ssh${suffix}`).env.GIT_SSH_COMMAND + if (!command) { + throw new Error('Missing SSH command') + } + const result = await runProcess({ + program: '/bin/sh', + args: ['-c', `ssh() { printf '%s\\0' "$@"; }; ${command}`] + }) + expect(result.code).toBe(0) + expect(result.stdout.split('\0').slice(0, -1)).toEqual(['-o', 'BatchMode=yes', ...expected]) + }) + + it('keeps the enforced first value even when a later option disables BatchMode', async ({ + skip + }) => { + const result = await runProcess({ + program: 'ssh', + args: [ + '-G', + '-F', + '/dev/null', + '-o', + 'BatchMode=yes', + '-o', + 'BatchMode=no', + 'example.invalid' + ] + }).catch((error: unknown) => { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + skip('OpenSSH is not installed') + } + throw error + }) + expect(result.code).toBe(0) + expect(result.stdout).toMatch(/^batchmode yes$/m) + }) + + it.each([ + String.raw` -i 'C:\keys\work\\ key' -o 'ProxyCommand=proxy -i C:\keys\proxy\\ key %h' -oBatchMode=no`, + String.raw` -i "C:\keys\work\\ key" -o "ProxyCommand=proxy \"C:\keys\proxy\\ key\" %h" -o "BatchMode no"`, + String.raw` -i 'key (work); and literal & operators' -o 'ProxyCommand=proxy (work) | other'` + ])('preserves shell-produced arguments: %s', async (suffix) => { + const root = await mkdtemp(path.join(tmpdir(), 'orca-ssh-policy-')) + try { + const directory = path.join(root, 'Program Files (x86)') + await mkdir(directory) + const ssh = path.join(directory, 'ssh') + await writeFile(ssh, '#!/bin/sh\nprintf \'%s\\0\' "$@"\n') + await chmod(ssh, 0o700) + const command = `${quotePosixShell(ssh)}${suffix}` + const policy = buildGitSshPolicyEnv({}, command) + expect(policy.mode).toBe('configured-openssh') + const enforced = policy.env.GIT_SSH_COMMAND + if (!enforced) { + throw new Error('Missing SSH command') + } + expect(enforced).toBe(`${quotePosixShell(ssh)} -o BatchMode=yes${suffix}`) + const argv = async (shellCommand: string) => { + const result = await runProcess({ + program: '/bin/sh', + args: [ + '-c', + `${shellCommand} "$@"`, + shellCommand, + 'example.invalid', + 'git-upload-pack repo' + ] + }) + expect(result.code).toBe(0) + return result.stdout.split('\0').slice(0, -1) + } + const original = await argv(command) + const rewritten = await argv(enforced) + expect(rewritten.slice(0, 2)).toEqual(['-o', 'BatchMode=yes']) + expect(rewritten.slice(2)).toEqual(original) + } finally { + await rm(root, { recursive: true, force: true }) + } }) }) diff --git a/src/shared/git-ssh-policy-env.ts b/src/shared/git-ssh-policy-env.ts index d4a77f5c8dc..3abd9376d97 100644 --- a/src/shared/git-ssh-policy-env.ts +++ b/src/shared/git-ssh-policy-env.ts @@ -1,3 +1,4 @@ +import { isWindowsAbsolutePathLike } from './cross-platform-path' import { quotePosixShell } from './wsl-login-shell-command' export type GitSshPolicyMode = @@ -41,99 +42,90 @@ function isMergeableOpenSshCommand(command: string): boolean { return basename === 'ssh' || basename === 'ssh.exe' } -function shellTokenize(command: string): string[] | null { - const tokens: string[] = [] - let current = '' - let quote: "'" | '"' | null = null - let escaped = false - - for (let i = 0; i < command.length; i++) { - const char = command[i] - if (escaped) { - current += char - escaped = false - continue - } - if (char === '\\') { - const next = command[i + 1] - if (next && /[\s'"\\]/.test(next)) { - escaped = true - } else { - current += char - } - continue - } - if (quote) { - if (char === quote) { - quote = null - } else { - current += char - } - continue - } - if (char === "'" || char === '"') { - quote = char - continue - } - if (/\s/.test(char)) { - if (current) { - tokens.push(current) - current = '' - } - continue - } - if (';&|<>()`'.includes(char)) { - return null - } - current += char - } - - if (escaped || quote) { - return null - } - if (current) { - tokens.push(current) - } - return tokens -} - -function shellQuoteToken(token: string): string { - return /^[A-Za-z0-9_@%+=:,./~-]+$/.test(token) ? token : quotePosixShell(token) -} - function containsShellExpansionSyntax(command: string): boolean { return /[$#*?[\]{}\r\n]/.test(command) || /(?:^|\s)['"]~/.test(command) || command.includes('\\~') } -function withoutBatchModeOptions(tokens: string[]): string[] { - const next: string[] = [] - for (let i = 0; i < tokens.length; i++) { - const token = tokens[i] - const lower = token.toLowerCase() - if (lower === '-o') { - const option = tokens[i + 1]?.toLowerCase() - if (option?.startsWith('batchmode')) { - i += 1 - continue +function containsShellControlSyntax(command: string): boolean { + let quote: "'" | '"' | null = null + let escaped = false + for (const char of command) { + if (escaped) { + escaped = false + } else if (quote === "'") { + if (char === quote) { + quote = null } + } else if (char === '\\') { + escaped = true + } else if (quote === '"') { + if (char === quote) { + quote = null + } else if (char === '`') { + return true + } + } else if (char === "'" || char === '"') { + quote = char + } else if (';&|<>()`'.includes(char)) { + return true } - if (lower.startsWith('-obatchmode')) { - continue - } - next.push(token) } - return next + return escaped || quote !== null +} + +function openSshExecutableEnd(command: string): number | null { + const match = /^[ \t]*(?:'([^']*)'|"((?:\\.|[^"\\])*)"|([^ \t'"]+))(?=[ \t]|$)/.exec(command) + const executable = match?.[1] ?? match?.[2] ?? match?.[3] ?? '' + if ( + !match || + (match[3]?.includes('\\') && !isWindowsAbsolutePathLike(executable)) || + !isMergeableOpenSshCommand(executable) + ) { + return null + } + return match[0].length +} + +function quoteBareWindowsPathWords(command: string): string { + return command.replace( + /'[^']*'|"(?:\\.|[^"\\])*"|(?:\\.|[^ \t'"\\])+/g, + (word: string, offset: number) => { + const end = offset + word.length + const windowsPath = /(?:^|=|^-[A-Za-z])([A-Za-z]:\\|\\\\)/.exec(word) + if ( + word.startsWith("'") || + word.startsWith('"') || + (offset > 0 && !/[ \t]/.test(command[offset - 1])) || + (end < command.length && !/[ \t]/.test(command[end])) || + !windowsPath + ) { + return word + } + const uncPrefixOffset = + windowsPath[1] === '\\\\' ? windowsPath.index + windowsPath[0].length - 2 : -1 + return quotePosixShell( + word.replace(/\\([ \t'"\\;&|<>()`])/g, (match, escaped: string, offset: number) => + offset === uncPrefixOffset && word[offset + 2] !== '\\' ? match : escaped + ) + ) + } + ) } function buildOpenSshBatchModeCommand(configuredCommand: string): string | null { - if (containsShellExpansionSyntax(configuredCommand)) { + if ( + containsShellExpansionSyntax(configuredCommand) || + containsShellControlSyntax(configuredCommand) + ) { return null } - const tokens = shellTokenize(configuredCommand) - if (!tokens || tokens.length === 0 || !isMergeableOpenSshCommand(tokens[0])) { + const command = quoteBareWindowsPathWords(configuredCommand) + const end = openSshExecutableEnd(command) + if (end === null) { return null } - return [...withoutBatchModeOptions(tokens), '-o', 'BatchMode=yes'].map(shellQuoteToken).join(' ') + // OpenSSH keeps the first value; avoid rebuilding the configured argument list. + return `${command.slice(0, end)} -o BatchMode=yes${command.slice(end)}` } export function buildGitSshPolicyEnv( diff --git a/src/shared/git-worktree-admin.test.ts b/src/shared/git-worktree-admin.test.ts index 2ae0ca39ef9..090afaf0a21 100644 --- a/src/shared/git-worktree-admin.test.ts +++ b/src/shared/git-worktree-admin.test.ts @@ -1,9 +1,13 @@ -import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, rename, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import path from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { resolveGitCommonDirectory } from './git-common-directory' -import { annotateWorktreeLocksFromAdmin, isBranchInDetachedWorktree } from './git-worktree-admin' +import { + annotateWorktreeLocksFromAdmin, + findLinkedWorktreeGitDirectory, + isBranchReservedByWorktreeOperation +} from './git-worktree-admin' import type { GitWorktreeInfo } from './worktree/types' import { isWorktreeCreatePreparation } from './worktree/create-preparation' @@ -40,6 +44,67 @@ afterEach(async () => { }) describe('owning-host worktree administrative reads', () => { + it('finds one exact linked registration after its checkout disappears', async () => { + await rm(linked, { recursive: true }) + await expect(findLinkedWorktreeGitDirectory(repo, linked)).resolves.toBe(admin) + await expect(findLinkedWorktreeGitDirectory(repo, `${linked}-other`)).resolves.toBeNull() + await expect(findLinkedWorktreeGitDirectory(repo, repo)).resolves.toBeNull() + await writeFile( + path.join(admin, 'gitdir'), + `${path.relative(admin, path.join(linked, '.git'))}\n` + ) + await expect(findLinkedWorktreeGitDirectory(repo, linked)).resolves.toBe(admin) + }) + + it('rejects duplicate registrations instead of choosing an owner marker', async () => { + const duplicate = path.join(common, 'worktrees', 'duplicate') + await mkdir(duplicate) + await writeFile(path.join(duplicate, 'gitdir'), `${path.join(linked, '.git')}\n`) + await expect(findLinkedWorktreeGitDirectory(repo, linked)).rejects.toThrow( + 'Cannot verify linked worktree administration' + ) + }) + + it('rejects a backlink to a file other than the checkout gitfile', async () => { + await writeFile(path.join(admin, 'gitdir'), `${path.join(linked, 'other')}\n`) + await expect(findLinkedWorktreeGitDirectory(repo, linked)).rejects.toThrow( + 'Cannot verify linked worktree administration' + ) + }) + + it.runIf(process.platform !== 'win32')( + 'rejects administrative directories redirected outside the common directory', + async () => { + const outside = path.join(root, 'outside-administration') + const worktrees = path.join(common, 'worktrees') + await rename(worktrees, outside) + await symlink(outside, worktrees) + await expect(findLinkedWorktreeGitDirectory(repo, linked)).rejects.toThrow( + 'Cannot verify linked worktree administration' + ) + } + ) + + it.runIf(process.platform !== 'win32')('rejects symlinked backlink files', async () => { + const outside = path.join(root, 'outside-backlink') + await writeFile(outside, `${path.join(linked, '.git')}\n`) + await rm(path.join(admin, 'gitdir')) + await symlink(outside, path.join(admin, 'gitdir')) + await expect(findLinkedWorktreeGitDirectory(repo, linked)).rejects.toThrow( + 'Cannot verify linked worktree administration' + ) + }) + + it.runIf(process.platform !== 'win32')( + 'rejects symlink administrative entries instead of overlooking duplicates', + async () => { + await symlink(admin, path.join(common, 'worktrees', 'duplicate-link')) + await expect(findLinkedWorktreeGitDirectory(repo, linked)).rejects.toThrow( + 'Cannot verify linked worktree administration' + ) + } + ) + it('resolves normal, linked, separate-git-dir and bare layouts without subprocesses', async () => { await expect(resolveGitCommonDirectory(repo)).resolves.toBe(common) await expect(resolveGitCommonDirectory(linked)).resolves.toBe(common) @@ -95,24 +160,81 @@ describe('owning-host worktree administrative reads', () => { await mkdir(path.dirname(file), { recursive: true }) await writeFile(file, 'refs/heads/feature\n') const detachedMain = rows().map((row) => (row.isMainWorktree ? { ...row, branch: '' } : row)) - await expect(isBranchInDetachedWorktree(repo, 'feature', detachedMain)).resolves.toBe(true) - await expect(isBranchInDetachedWorktree(repo, 'other', detachedMain)).resolves.toBe(false) + await expect( + isBranchReservedByWorktreeOperation(repo, 'feature', detachedMain) + ).resolves.toBe(true) + await expect(isBranchReservedByWorktreeOperation(repo, 'other', detachedMain)).resolves.toBe( + false + ) } ) it('fails closed when a detached registration cannot be associated with admin metadata', async () => { await rm(path.join(admin, 'gitdir')) - await expect(isBranchInDetachedWorktree(repo, 'feature', rows())).rejects.toThrow( - 'Cannot verify detached worktree branch usage' + await expect(isBranchReservedByWorktreeOperation(repo, 'feature', rows())).rejects.toThrow( + 'Cannot verify worktree branch usage' ) }) + it.each([40, 64])( + 'protects auxiliary rebase refs with %i-character OIDs on attached worktrees', + async (length) => { + await mkdir(path.join(admin, 'rebase-merge')) + const before = 'a'.repeat(length) + const after = '0'.repeat(length) + await writeFile( + path.join(admin, 'rebase-merge', 'update-refs'), + `refs/heads/other\r\n${before}\r\n${after}\r\nrefs/heads/feature\r\n${before}\r\n${after}\r\n` + ) + const attached = rows().map((row) => ({ ...row, branch: 'refs/heads/main' })) + await expect(isBranchReservedByWorktreeOperation(repo, 'feature', attached)).resolves.toBe( + true + ) + await expect(isBranchReservedByWorktreeOperation(repo, 'unreserved', attached)).resolves.toBe( + false + ) + } + ) + + it('checks update-refs in the main worktree and compares only ref-name fields', async () => { + await mkdir(path.join(common, 'rebase-merge')) + await writeFile( + path.join(common, 'rebase-merge', 'update-refs'), + `refs/heads/feature\n${'a'.repeat(40)}\n${'0'.repeat(40)}\n` + ) + await expect(isBranchReservedByWorktreeOperation(repo, 'feature', rows())).resolves.toBe(true) + await expect(isBranchReservedByWorktreeOperation(repo, 'a'.repeat(40), rows())).resolves.toBe( + false + ) + }) + + it.each([ + `refs/heads/feature\n${'a'.repeat(40)}\n`, + `refs/heads/feature\n${'a'.repeat(40)}\n${'g'.repeat(40)}\n`, + `refs/heads/feature\n${'a'.repeat(40)}\n${'0'.repeat(64)}\n`, + `refs/heads/feature\n${'a'.repeat(40)}\n${'0'.repeat(40)}\ninvalid\n` + ])('fails closed on malformed update-refs records: %s', async (contents) => { + await mkdir(path.join(admin, 'rebase-merge')) + await writeFile(path.join(admin, 'rebase-merge', 'update-refs'), contents) + await expect(isBranchReservedByWorktreeOperation(repo, 'feature', rows())).rejects.toThrow( + 'Cannot verify rebase update-refs branch usage' + ) + }) + + it('fails closed when update-refs cannot be read', async () => { + await mkdir(path.join(admin, 'rebase-merge', 'update-refs'), { recursive: true }) + await expect(isBranchReservedByWorktreeOperation(repo, 'feature', rows())).rejects.toThrow() + }) + it('propagates cancellation during administrative reads', async () => { const signal = AbortSignal.abort(new Error('cancelled')) await expect(annotateWorktreeLocksFromAdmin(repo, rows(), { signal })).rejects.toThrow( 'cancelled' ) - await expect(isBranchInDetachedWorktree(repo, 'feature', rows(), { signal })).rejects.toThrow( + await expect( + isBranchReservedByWorktreeOperation(repo, 'feature', rows(), { signal }) + ).rejects.toThrow('cancelled') + await expect(findLinkedWorktreeGitDirectory(repo, linked, { signal })).rejects.toThrow( 'cancelled' ) }) diff --git a/src/shared/git-worktree-admin.ts b/src/shared/git-worktree-admin.ts index c2d7ed266a9..8dbdee3b66b 100644 --- a/src/shared/git-worktree-admin.ts +++ b/src/shared/git-worktree-admin.ts @@ -1,5 +1,5 @@ import type { Dirent } from 'node:fs' -import { readdir } from 'node:fs/promises' +import { lstat, readdir, realpath } from 'node:fs/promises' import path from 'node:path' import { waitForPromiseWithSignal } from './abort-signal-reason' import { @@ -14,7 +14,12 @@ import { foldWslUncPathCaseInsensitiveParts } from './wsl-paths' import type { GitWorktreeInfo } from './worktree/types' const ADMIN_READ_CONCURRENCY = 8 -type WorktreeAdminDirectory = { gitDir: string; worktreePath?: string; isMain?: true } +type WorktreeAdminDirectory = { + gitDir: string + gitFilePath?: string + worktreePath?: string + isMain?: true +} function hostPathKey(value: string): string { const normalized = path.resolve(value) @@ -26,7 +31,8 @@ function hostPathKey(value: string): string { async function readWorktreeAdminDirectories( repoPath: string, - options: GitAdminReadOptions + options: GitAdminReadOptions, + requireDirectDirectories = false ): Promise { const commonDir = await resolveGitCommonDirectory(repoPath, options) if (!commonDir) { @@ -45,6 +51,9 @@ async function readWorktreeAdminDirectories( } throw error } + if (requireDirectDirectories && entries.some((entry) => entry.isSymbolicLink())) { + throw new Error('Cannot verify linked worktree administration.') + } const linked = await mapWithConcurrency( entries.filter((entry) => entry.isDirectory()), ADMIN_READ_CONCURRENCY, @@ -52,12 +61,50 @@ async function readWorktreeAdminDirectories( const gitDir = path.join(adminDir, entry.name) const gitdir = await readGitAdminFile(path.join(gitDir, 'gitdir'), options.signal) const target = gitdir && resolveGitMetadataPath(gitDir, gitdir, options) - return { gitDir, ...(target ? { worktreePath: path.dirname(target) } : {}) } + return { + gitDir, + ...(target ? { gitFilePath: target, worktreePath: path.dirname(target) } : {}) + } } ) return [{ gitDir: commonDir, isMain: true }, ...linked] } +/** A missing checkout still has a backlink in its owning repository's administration. */ +export async function findLinkedWorktreeGitDirectory( + repoPath: string, + worktreePath: string, + options: GitAdminReadOptions = {} +): Promise { + const hostPath = resolveWorktreeHostPath(worktreePath, options) + if (!hostPath) { + return null + } + const directories = await readWorktreeAdminDirectories(repoPath, options, true) + const targetKey = hostPathKey(hostPath) + const matches = directories.filter( + (entry) => entry.worktreePath && hostPathKey(entry.worktreePath) === targetKey + ) + if (matches.length === 0) { + return null + } + const entry = matches[0] + if (matches.length !== 1 || !entry.gitFilePath || path.basename(entry.gitFilePath) !== '.git') { + throw new Error('Cannot verify linked worktree administration.') + } + const [commonDir, gitDir, backlink] = await Promise.all([ + waitForPromiseWithSignal(realpath(directories[0].gitDir), options.signal), + waitForPromiseWithSignal(realpath(entry.gitDir), options.signal), + waitForPromiseWithSignal(lstat(path.join(entry.gitDir, 'gitdir')), options.signal) + ]) + const relative = path.relative(commonDir, gitDir).split(path.sep) + if (relative.length !== 2 || relative[0] !== 'worktrees' || !relative[1] || !backlink.isFile()) { + throw new Error('Cannot verify linked worktree administration.') + } + options.signal?.throwIfAborted() + return entry.gitDir +} + /** Older porcelain omits locks; the marker remains the authoritative ownership proof. */ export async function annotateWorktreeLocksFromAdmin( repoPath: string, @@ -88,19 +135,48 @@ export async function annotateWorktreeLocksFromAdmin( }) } -/** Detached HEAD during rebase or bisect still reserves the original branch. */ -export async function isBranchInDetachedWorktree( +function updateRefsReserveBranch(contents: string | null, branchName: string): boolean { + if (!contents) { + return false + } + const lines = contents.split(/\r?\n/) + if (lines.at(-1) === '') { + lines.pop() + } + if (lines.length % 3 !== 0) { + throw new Error('Cannot verify rebase update-refs branch usage.') + } + let reserved = false + for (let i = 0; i < lines.length; i += 3) { + const ref = lines[i] + const before = lines[i + 1] ?? '' + const after = lines[i + 2] ?? '' + if ( + !ref || + !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(before) || + !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(after) || + before.length !== after.length + ) { + throw new Error('Cannot verify rebase update-refs branch usage.') + } + reserved ||= ref === `refs/heads/${branchName}` + } + return reserved +} + +/** Rebase and bisect reserve branches even when HEAD no longer points to them. */ +export async function isBranchReservedByWorktreeOperation( repoPath: string, branchName: string, worktrees: GitWorktreeInfo[], options: GitAdminReadOptions = {} ): Promise { - const detached = worktrees.filter((worktree) => !worktree.branch && !worktree.isBare) - if (detached.length === 0) { + const nonBare = worktrees.filter((worktree) => !worktree.isBare) + if (nonBare.length === 0) { return false } const targets = new Set( - detached.map((worktree) => { + nonBare.map((worktree) => { const hostPath = resolveWorktreeHostPath(worktree.path, options) return hostPath ? hostPathKey(hostPath) : worktree.path }) @@ -108,19 +184,28 @@ export async function isBranchInDetachedWorktree( const directories = await readWorktreeAdminDirectories(repoPath, options) const relevant = directories.filter((entry) => entry.isMain - ? detached.some((worktree) => worktree.isMainWorktree) + ? nonBare.some((worktree) => worktree.isMainWorktree) : entry.worktreePath && targets.has(hostPathKey(entry.worktreePath)) ) - if (relevant.length < detached.length) { - throw new Error('Cannot verify detached worktree branch usage.') + if (relevant.length < nonBare.length) { + throw new Error('Cannot verify worktree branch usage.') } const matches = await mapWithConcurrency(relevant, ADMIN_READ_CONCURRENCY, async (entry) => { - const markers = await Promise.all( - ['rebase-merge/head-name', 'rebase-apply/head-name', 'BISECT_START'].map((name) => - readGitAdminFile(path.join(entry.gitDir, ...name.split('/')), options.signal) + const [rebaseMerge, rebaseApply, bisect, updateRefs] = await Promise.all( + [ + 'rebase-merge/head-name', + 'rebase-apply/head-name', + 'BISECT_START', + 'rebase-merge/update-refs' + ].map((name) => readGitAdminFile(path.join(entry.gitDir, ...name.split('/')), options.signal)) + ) + const reserved = updateRefsReserveBranch(updateRefs, branchName) + return ( + reserved || + [rebaseMerge, rebaseApply, bisect].some( + (marker) => marker?.trim().replace(/^refs\/heads\//, '') === branchName ) ) - return markers.some((marker) => marker?.trim().replace(/^refs\/heads\//, '') === branchName) }) options.signal?.throwIfAborted() return matches.some(Boolean) diff --git a/src/shared/repo-maintenance-schedule.ts b/src/shared/repo-maintenance-schedule.ts index cbc5764b695..da3c26e9d38 100644 --- a/src/shared/repo-maintenance-schedule.ts +++ b/src/shared/repo-maintenance-schedule.ts @@ -44,6 +44,26 @@ export class RepoMaintenanceSchedule { this.refCooldownUntil.set(key, this.now() + cooldownMs) } + async probeOptOut( + target: RepoRefMaintenanceTarget, + signal: AbortSignal, + span: RefMaintenanceSpan, + canWrite: () => boolean + ): Promise { + try { + if (!(await target.isOptedOut?.(signal)) || signal.aborted || !canWrite()) { + return false + } + this.postponeIndex(target.key, REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + this.settleRefs(target.key, span, 'opted_out', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) + return true + } catch (error) { + span.setAttribute('repo.maintenance_error', String(error)) + span.setAttribute('repo.maintenance_outcome', 'failed' satisfies RefMaintenanceOutcome) + return { error } + } + } + async maintain( target: RepoRefMaintenanceTarget, signal: AbortSignal, @@ -57,6 +77,9 @@ export class RepoMaintenanceSchedule { if (signal.aborted || outcome === 'deferred') { return outcome } + if (outcome !== 'written' && !canWrite()) { + return 'deferred' + } const cooldown = outcome === 'failed' ? PACK_INDEX_MAINTENANCE_FAILURE_COOLDOWN_MS diff --git a/src/shared/repo-ref-maintenance-policy.ts b/src/shared/repo-ref-maintenance-policy.ts index 763bfb1d0dc..09894ce3016 100644 --- a/src/shared/repo-ref-maintenance-policy.ts +++ b/src/shared/repo-ref-maintenance-policy.ts @@ -126,12 +126,16 @@ export type RepoRefMaintenanceTarget = { canWrite: () => boolean ): Promise /** - * Runs `pack-refs` to completion. Deliberately takes no abort signal: killing + * Runs `pack-refs` to completion. Its signal cancels admission only: killing * a pack is measurably worse than waiting for it (see `PACKED_REFS_LOCK_*`). * It must report `packed-refs.lock` transitions through `lock` so callers can * wait for the short window that actually blocks them. */ - packRefs(lock: PackedRefsLockReporter): Promise + packRefs( + lock: PackedRefsLockReporter, + admissionSignal?: AbortSignal, + canStart?: () => boolean + ): Promise } /** How `packRefs` tells the scheduler whether the exclusive write window is open. */ diff --git a/src/shared/repo-ref-maintenance.ts b/src/shared/repo-ref-maintenance.ts index bc4cf3ed9d8..bef6d71e07f 100644 --- a/src/shared/repo-ref-maintenance.ts +++ b/src/shared/repo-ref-maintenance.ts @@ -35,15 +35,11 @@ type TrackedRepo = { target: RepoRefMaintenanceTarget timer: ReturnType | null deferrals: number + superseded?: boolean } const noopSpan: RefMaintenanceSpan = { setAttribute: () => {} } -/** A deadline means something is stuck: back off instead of retrying straight away. */ -function hitDeadline(signal: AbortSignal): boolean { - return signal.reason instanceof RefMaintenanceInterrupted && signal.reason.deadline -} - export class RepoRefMaintenance { private readonly tracked = new Map() private readonly phases: RepoMaintenanceSchedule @@ -59,6 +55,7 @@ export class RepoRefMaintenance { // is still running -- an interrupt cancels the work and waits for it to stop. private inFlight: Promise | null = null private inFlightAbort: AbortController | null = null + private inFlightTracked: TrackedRepo | null = null private readonly lockGate = new PackedRefsLockGate() // Why a count, not a flag: several ref-touching operations overlap routinely // (a create's fetch inside a create), and the last one out reopens the window. @@ -85,13 +82,27 @@ export class RepoRefMaintenance { if (this.disposed) { return } + // Invalidation survives eviction of the replacement quiet-period timer. + if (this.inFlightTracked?.target.key === target.key) { + this.inFlightTracked.superseded = true + } const existing = this.tracked.get(target.key) if (existing?.timer) { clearTimeout(existing.timer) } const tracked: TrackedRepo = { target, timer: null, deferrals: existing?.deferrals ?? 0 } this.tracked.delete(target.key) - this.evictOldestBeyondCap() + while (this.tracked.size >= MAX_TRACKED_REPOS) { + const oldest = this.tracked.keys().next() + if (oldest.done) { + break + } + const evicted = this.tracked.get(oldest.value) + if (evicted?.timer) { + clearTimeout(evicted.timer) + } + this.tracked.delete(oldest.value) + } this.tracked.set(target.key, tracked) this.schedule(target.key, tracked) } @@ -173,7 +184,7 @@ export class RepoRefMaintenance { } private isBusy(tracked: TrackedRepo): boolean { - return this.isAppBusy() || (tracked.target.isBusy?.() ?? false) + return tracked.superseded || this.isAppBusy() || (tracked.target.isBusy?.() ?? false) } private schedule(key: string, tracked: TrackedRepo, delayMs = this.quietPeriodMs): void { @@ -186,30 +197,16 @@ export class RepoRefMaintenance { tracked.timer = timer } - private evictOldestBeyondCap(): void { - while (this.tracked.size >= MAX_TRACKED_REPOS) { - const oldest = this.tracked.keys().next() - if (oldest.done) { - return - } - const evicted = this.tracked.get(oldest.value) - if (evicted?.timer) { - clearTimeout(evicted.timer) - } - this.tracked.delete(oldest.value) - } - } - /** * `counted` spends the give-up budget. Waiting behind another repository's * pack, or yielding to work Orca asked us to yield to, does not: both end on * their own, so charging for them would let a busy machine starve a repo - * until its next fetch. Only "the app is busy" is charged. + * until its next fetch. Only app activity and failed probes are charged. */ private defer(key: string, tracked: TrackedRepo, counted: boolean): void { // A fetch that landed while this attempt was probing already re-armed the // repo; that entry is fresher, so the deferral must not overwrite it. - if (this.disposed || this.tracked.has(key)) { + if (this.disposed || tracked.superseded || this.tracked.has(key)) { return } if (counted) { @@ -251,6 +248,7 @@ export class RepoRefMaintenance { REF_MAINTENANCE_ATTEMPT_DEADLINE_MS ) deadline.unref?.() + this.inFlightTracked = tracked const run = this.observe((span) => this.packIfNeeded(key, tracked, span, abort.signal)) this.inFlight = run this.inFlightAbort = abort @@ -261,6 +259,7 @@ export class RepoRefMaintenance { if (this.inFlight === run) { this.inFlight = null this.inFlightAbort = null + this.inFlightTracked = null } } } @@ -272,40 +271,25 @@ export class RepoRefMaintenance { signal: AbortSignal ): Promise { span.setAttribute('repo.maintenance_key', key) + const canWrite = () => !signal.aborted && this.suspensions === 0 && !this.isBusy(tracked) // Probes are cancellable; admitted index and ref writers finish before releasing the slot. - if (await tracked.target.isOptedOut?.(signal)) { - this.phases.postponeIndex(key, REF_MAINTENANCE_CLEAN_COOLDOWN_MS) - this.phases.settleRefs(key, span, 'opted_out', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) - return - } + const optedOut = await this.phases.probeOptOut(tracked.target, signal, span, canWrite) if (signal.aborted) { - this.yieldTo(key, tracked, span, signal) + return this.yieldTo(key, tracked, span, signal) + } + if (typeof optedOut === 'object') { + this.defer(key, tracked, true) + throw optedOut.error + } + if (optedOut) { return } if (this.suspensions > 0 || this.isBusy(tracked)) { - span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) - this.defer(key, tracked, true) - return + return this.yieldTo(key, tracked, span, signal) } - const indexOutcome = await this.phases.maintain( - tracked.target, - signal, - span, - () => !signal.aborted && this.suspensions === 0 && !this.isBusy(tracked) - ) - if (indexOutcome === 'deferred') { - span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) - this.defer(key, tracked, true) - return - } - if (signal.aborted) { - this.yieldTo(key, tracked, span, signal) - return - } - if (this.suspensions > 0 || this.isBusy(tracked)) { - span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) - this.defer(key, tracked, true) - return + const indexOutcome = await this.phases.maintain(tracked.target, signal, span, canWrite) + if (indexOutcome === 'deferred' || !canWrite()) { + return this.yieldTo(key, tracked, span, signal) } if (this.now() < this.phases.refDueAt(key)) { span.setAttribute('repo.maintenance_outcome', 'index_only' satisfies RefMaintenanceOutcome) @@ -317,15 +301,17 @@ export class RepoRefMaintenance { return } const refsDirectory = await tracked.target.resolveRefsDirectory(signal) + if (!canWrite()) { + return this.yieldTo(key, tracked, span, signal) + } if (!refsDirectory) { this.phases.settleRefs(key, span, 'unresolved', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) return } const budget = this.looseRefThreshold + 1 const before = await countLooseRefs(refsDirectory, budget, signal) - if (signal.aborted) { - this.yieldTo(key, tracked, span, signal) - return + if (!canWrite()) { + return this.yieldTo(key, tracked, span, signal) } span.setAttribute('git.loose_ref_count', before.count) span.setAttribute('git.loose_ref_threshold', this.looseRefThreshold) @@ -334,20 +320,17 @@ export class RepoRefMaintenance { this.phases.settleRefs(key, span, 'below_threshold', REF_MAINTENANCE_CLEAN_COOLDOWN_MS) return } - // The quiet window can close while the probe walks; re-check before spending a git slot. - if (this.suspensions > 0 || this.isBusy(tracked)) { - span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) - this.defer(key, tracked, true) - return - } const startedAt = this.now() let partial = false try { - // No signal: the pack runs to completion. Callers that need the refs wait + // Only admission is cancellable. Callers that need the refs wait // out the rewrite window through `pause()` instead of killing it. - await tracked.target.packRefs(this.lockGate) + await tracked.target.packRefs(this.lockGate, signal, canWrite) } catch (error) { span.setAttribute('repo.maintenance_error', String(error)) + if (!canWrite()) { + return this.yieldTo(key, tracked, span, signal) + } if (error instanceof RefMaintenanceRepoLocked) { this.phases.settleRefs(key, span, 'locked', REF_MAINTENANCE_LOCKED_COOLDOWN_MS) return @@ -357,11 +340,17 @@ export class RepoRefMaintenance { this.lockGate.setHeld(false) } span.setAttribute('git.pack_refs_ms', this.now() - startedAt) + if (!canWrite()) { + return this.yieldTo(key, tracked, span, signal) + } // Judge by the backlog, not by the exit code. On a machine running several // Orca sessions a branch moving mid-pack is the normal case, and Git's // response -- leave that one ref loose, pack the rest -- is the correct one. // Measured in the field: 36,688 loose refs down to 3, reported as an error. const after = await countLooseRefs(refsDirectory, budget, signal) + if (!canWrite()) { + return this.yieldTo(key, tracked, span, signal) + } span.setAttribute('git.loose_ref_count_after', after.count) if (partial && (after.saturated || after.count >= this.looseRefThreshold)) { this.phases.settleRefs(key, span, 'failed', REF_MAINTENANCE_FAILURE_COOLDOWN_MS) @@ -371,14 +360,19 @@ export class RepoRefMaintenance { this.phases.settleRefs(key, span, 'packed', REF_MAINTENANCE_PACKED_COOLDOWN_MS) } - /** Record an aborted attempt: retry soon if Orca yielded, back off if it stalled. */ + /** Retry ineligible work; back off only for activity or deadlines. */ private yieldTo( key: string, tracked: TrackedRepo, span: RefMaintenanceSpan, signal: AbortSignal ): void { - if (hitDeadline(signal)) { + if (!signal.aborted || tracked.superseded) { + span.setAttribute('repo.maintenance_outcome', 'deferred' satisfies RefMaintenanceOutcome) + this.defer(key, tracked, true) + return + } + if (signal.reason instanceof RefMaintenanceInterrupted && signal.reason.deadline) { this.phases.settleRefs(key, span, 'timed_out', REF_MAINTENANCE_FAILURE_COOLDOWN_MS) return } diff --git a/src/shared/review-draft-context-error.ts b/src/shared/review-draft-context-error.ts new file mode 100644 index 00000000000..6375023d4e5 --- /dev/null +++ b/src/shared/review-draft-context-error.ts @@ -0,0 +1,3 @@ +export class ReviewDraftContextError extends Error { + override readonly name = 'ReviewDraftContextError' +} diff --git a/tests/e2e/cross-version-wire/qoder-history-search-downgrade.unit.test.ts b/tests/e2e/cross-version-wire/qoder-history-search-downgrade.unit.test.ts index cac22a9c3f0..c9bdc70b6b6 100644 --- a/tests/e2e/cross-version-wire/qoder-history-search-downgrade.unit.test.ts +++ b/tests/e2e/cross-version-wire/qoder-history-search-downgrade.unit.test.ts @@ -3,35 +3,58 @@ import { createSessionSearchClient, unavailableSessionSearchStatus } from '../../../src/shared/ai-vault-search-client' -import { searchResults, fakeSearchService } from '../../../src/shared/ai-vault-search-test-fixture' +import { + searchHit, + searchResults, + fakeSearchService +} from '../../../src/shared/ai-vault-search-test-fixture' import { searchSessionService, + sessionSearchServiceStatus, setSessionSearchService } from '../../../src/main/ai-vault-search/session-search-service-registry' import { AI_VAULT_AGENTS } from '../../../src/shared/ai-vault-types' +import { redactForTransport } from '../../../src/shared/ai-vault-search-transport' import { importReleaseCheckoutModule, materializeReleaseCheckout } from './release-checkout' +const PRE_QODER_HOSTS = ['v1.4.211', 'b49abdb1f4da6b3d62dfa9ccf3c74dc9e74d291c'] +const QODER_HOST = 'f97ca2a49d9c711dab54a656a7f8a47ae6c6749c' +const LEGACY_AGENTS = AI_VAULT_AGENTS.filter( + (agent) => !['codebuddy', 'zcode', 'qoder', 'jcode'].includes(agent) +) afterEach(() => setSessionSearchService(null)) +async function releaseSearchSchemas(ref: string) { + const checkout = await materializeReleaseCheckout(ref) + return importReleaseCheckoutModule(checkout, 'src/shared/ai-vault-search-contract.ts') +} + +function releaseParser(schema: unknown) { + if ( + !schema || + typeof schema !== 'object' || + !('parse' in schema) || + typeof schema.parse !== 'function' || + !('safeParse' in schema) || + typeof schema.safeParse !== 'function' + ) { + throw new Error('Pinned release has no search parser') + } + const parse = schema.parse + const safeParse = schema.safeParse + return { + parse: (value: unknown) => parse.call(schema, value), + safeParse: (value: unknown) => safeParse.call(schema, value) + } +} + test.each(['qoder', 'jcode'] as const)( 'a pre-agent release can read current %s search pages without losing other agents', async (agent) => { - const checkout = await materializeReleaseCheckout('v1.4.211') - const baseline = await importReleaseCheckoutModule( - checkout, - 'src/shared/ai-vault-search-contract.ts' - ) - const schema = baseline.AiVaultSearchResponseSchema - if ( - !schema || - typeof schema !== 'object' || - !('safeParse' in schema) || - typeof schema.safeParse !== 'function' - ) { - throw new Error('The pinned release has no search response parser') - } - const newHit = { ...searchResults().hits[0], agent } - expect(schema.safeParse({ ...searchResults(), hits: [newHit] })).toHaveProperty( + const baseline = await releaseSearchSchemas('v1.4.211') + const responseParser = releaseParser(baseline.AiVaultSearchResponseSchema) + const newHit = { ...searchHit(), agent } + expect(responseParser.safeParse({ ...searchResults(), hits: [newHit] })).toHaveProperty( 'success', false ) @@ -45,27 +68,17 @@ test.each(['qoder', 'jcode'] as const)( })) setSessionSearchService(service) const oldResponse = await searchSessionService({ query: 'proof' }, 'relay') - expect(schema.safeParse(oldResponse)).toHaveProperty('success', true) + expect(responseParser.safeParse(oldResponse)).toHaveProperty('success', true) expect(oldResponse).toMatchObject({ hits: [{ agent: 'codex' }] }) const client = createSessionSearchClient( (_method, request) => searchSessionService(request, 'relay'), 'relay' ) - expect(await client.searchSessions({ query: 'proof' })).toMatchObject({ - hits: [{ agent }] - }) - const oldRequest = baseline.AiVaultSearchRequestSchema - if ( - !oldRequest || - typeof oldRequest !== 'object' || - !('safeParse' in oldRequest) || - typeof oldRequest.safeParse !== 'function' - ) { - throw new Error('The pinned release has no search request parser') - } + expect(await client.searchSessions({ query: 'proof' })).toMatchObject({ hits: [{ agent }] }) expect( - oldRequest.safeParse({ + releaseParser(baseline.AiVaultSearchRequestSchema).safeParse({ query: 'proof', + supportedAgents: [...AI_VAULT_AGENTS], supportsQoderHistory: true, supportsJcodeHistory: true }) @@ -73,75 +86,171 @@ test.each(['qoder', 'jcode'] as const)( } ) -test.each(['v1.4.211', 'b49abdb1f4da6b3d62dfa9ccf3c74dc9e74d291c'])( - 'a current client narrows new agent filters before calling the actual %s request parser', +test.each(PRE_QODER_HOSTS)( + 'the actual %s response parser accepts complete current-host legacy pages', async (ref) => { - const checkout = await materializeReleaseCheckout(ref) - const baseline = await importReleaseCheckoutModule( - checkout, - 'src/shared/ai-vault-search-contract.ts' + const baseline = await releaseSearchSchemas(ref) + const responseParser = releaseParser(baseline.AiVaultSearchResponseSchema) + const hits = (['codex', 'codebuddy', 'zcode', 'qoder', 'jcode'] as const).map((agent) => ({ + ...searchHit(), + agent, + sessionId: agent + })) + expect(responseParser.safeParse({ ...searchResults(), hits })).toHaveProperty('success', false) + const service = fakeSearchService() + service.search.mockImplementation(async (request) => ({ + ...searchResults(), + hits: hits.filter( + (hit) => !request.filters?.agents || request.filters.agents.includes(hit.agent) + ), + page: { cursor: 'legacy-next-page', hasMore: true } + })) + setSessionSearchService(service) + const oldResponse = await searchSessionService({ query: 'proof' }, 'relay') + expect(responseParser.safeParse(oldResponse)).toHaveProperty('success', true) + expect(oldResponse).toMatchObject({ + hits: [{ agent: 'codex' }], + page: { cursor: 'legacy-next-page', hasMore: true } + }) + expect(service.search).toHaveBeenLastCalledWith( + { query: 'proof', limit: 20, filters: { agents: LEGACY_AGENTS } }, + undefined ) - const requestSchema = baseline.AiVaultSearchRequestSchema - if ( - !requestSchema || - typeof requestSchema !== 'object' || - !('parse' in requestSchema) || - typeof requestSchema.parse !== 'function' - ) { - throw new Error('Pinned host has no search request parser') - } + const nextPage = await searchSessionService( + { query: 'proof', cursor: 'legacy-next-page' }, + 'relay' + ) + expect(responseParser.safeParse(nextPage)).toHaveProperty('success', true) + expect(service.search).toHaveBeenLastCalledWith( + { query: 'proof', cursor: 'legacy-next-page', limit: 20, filters: { agents: LEGACY_AGENTS } }, + undefined + ) + const client = createSessionSearchClient( + (method, request) => + method === 'aiVault.searchStatus' + ? sessionSearchServiceStatus(request, 'relay') + : searchSessionService(request, 'relay'), + 'relay' + ) + expect(await client.searchSessions({ query: 'proof' })).toMatchObject({ + hits: hits.map((hit) => redactForTransport(hit, 'relay')) + }) + } +) + +test.each(PRE_QODER_HOSTS)( + 'a current client narrows its entire catalog before the actual %s request parser', + async (ref) => { + const baseline = await releaseSearchSchemas(ref) + const requestParser = releaseParser(baseline.AiVaultSearchRequestSchema) + expect( + requestParser.safeParse({ query: 'proof', filters: { agents: [...AI_VAULT_AGENTS] } }) + ).toHaveProperty('success', false) const call = vi.fn(async (method: string, request: Record) => { if (method === 'aiVault.searchStatus') { return { ...unavailableSessionSearchStatus(), enabled: true, phase: 'current' } } - requestSchema.parse(request) + requestParser.parse(request) return searchResults() }) - const within = { kind: 'workspace' as const, worktreeId: 'folder:/task-owned/folder' } const client = createSessionSearchClient(call, 'relay') + const within = { kind: 'workspace' as const, worktreeId: 'folder:/task-owned/folder' } expect( await client.searchSessions({ query: 'proof', - filters: { agents: ['codex', 'qoder', 'jcode'] }, + filters: { agents: [...AI_VAULT_AGENTS] }, within }) ).toMatchObject({ hits: [{ agent: 'codex' }] }) + expect(call).toHaveBeenCalledTimes(2) expect(call).toHaveBeenLastCalledWith( 'aiVault.searchSessions', - expect.objectContaining({ - filters: { agents: ['codex'] }, - within - }) + expect.objectContaining({ filters: { agents: LEGACY_AGENTS }, within }) ) call.mockClear() expect( await client.searchSessions({ query: 'proof', - filters: { agents: ['qoder', 'jcode'] }, + filters: { agents: ['qoder', 'jcode', 'codebuddy', 'zcode'] }, within }) ).toEqual({ kind: 'unavailable', reason: 'unsupported-agent' }) + expect(call).toHaveBeenCalledExactlyOnceWith('aiVault.searchStatus', {}) + call.mockClear() + expect( + await client.searchSessions({ query: 'proof', filters: { agents: ['codex'] }, within }) + ).toMatchObject({ hits: [{ agent: 'codex' }] }) expect(call).toHaveBeenCalledTimes(1) - expect(call).toHaveBeenCalledWith('aiVault.searchStatus', {}) - if (ref.startsWith('b49')) { - expect( - await client.searchSessions({ query: 'proof', filters: { agents: [...AI_VAULT_AGENTS] } }) - ).toMatchObject({ hits: [{ agent: 'codex' }] }) - expect(call).toHaveBeenLastCalledWith( - 'aiVault.searchSessions', - expect.objectContaining({ - filters: { - agents: AI_VAULT_AGENTS.filter((agent) => { - try { - requestSchema.parse({ query: 'proof', filters: { agents: [agent] } }) - return true - } catch { - return false - } - }) - } - }) - ) - } } ) + +test.each(['codebuddy', 'zcode'] as const)( + 'an actual b49 explicit %s request proves the old client can read that agent', + async (agent) => { + const baseline = await releaseSearchSchemas(PRE_QODER_HOSTS[1]) + const request = releaseParser(baseline.AiVaultSearchRequestSchema).parse({ + query: 'proof', + filters: { agents: [agent] } + }) + const service = fakeSearchService() + service.search.mockResolvedValue({ ...searchResults(), hits: [{ ...searchHit(), agent }] }) + setSessionSearchService(service) + const response = await searchSessionService(request, 'relay') + expect(releaseParser(baseline.AiVaultSearchResponseSchema).safeParse(response)).toHaveProperty( + 'success', + true + ) + expect(response).toMatchObject({ hits: [{ agent }] }) + expect(service.search).toHaveBeenCalledExactlyOnceWith( + { query: 'proof', limit: 20, filters: { agents: [agent] } }, + undefined + ) + } +) + +test('the shipped Qoder capability proves its historical trio while withholding later Jcode', async () => { + const baseline = await releaseSearchSchemas(QODER_HOST) + const requestParser = releaseParser(baseline.AiVaultSearchRequestSchema) + const responseParser = releaseParser(baseline.AiVaultSearchResponseSchema) + const request = requestParser.parse({ query: 'proof', supportsQoderHistory: true }) + const hits = [ + searchHit(), + ...(['codebuddy', 'zcode', 'qoder', 'jcode'] as const).map((agent) => ({ + ...searchHit(), + agent + })) + ] + const service = fakeSearchService() + service.search.mockImplementation(async (projected) => ({ + ...searchResults(), + hits: hits.filter( + (hit) => !projected.filters?.agents || projected.filters.agents.includes(hit.agent) + ) + })) + setSessionSearchService(service) + const response = await searchSessionService(request, 'relay') + expect(responseParser.safeParse(response)).toHaveProperty('success', true) + expect(response).toMatchObject({ + hits: hits.filter((hit) => hit.agent !== 'jcode').map((hit) => redactForTransport(hit, 'relay')) + }) + expect( + responseParser.safeParse({ ...searchResults(), hits: [{ ...searchHit(), agent: 'jcode' }] }) + ).toHaveProperty('success', false) + const call = vi.fn(async (method: string, params: Record) => { + if (method === 'aiVault.searchStatus') { + return { ...unavailableSessionSearchStatus(), supportsQoderHistory: true } + } + requestParser.parse(params) + return searchResults() + }) + await createSessionSearchClient(call, 'relay').searchSessions({ + query: 'proof', + filters: { agents: [...AI_VAULT_AGENTS] } + }) + expect(call).toHaveBeenLastCalledWith( + 'aiVault.searchSessions', + expect.objectContaining({ + filters: { agents: AI_VAULT_AGENTS.filter((agent) => agent !== 'jcode') } + }) + ) +})