From 8ac1c6e2acd0fcf0191e4875f0e1996c9b25aa62 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:27:28 -0700 Subject: [PATCH] perf(git): bound ref and worktree scans (#17655) * perf(git): bound ref and worktree scans * fix(repo-search): clamp oversized ref limits * fix(worktree): keep strict worktree listing unshared The shared-scan re-export flipped every `listWorktreesStrict` caller from an isolated subprocess to the coalesced scan. `git worktree prune` in the removal recovery path does not bump the scan generation, so a post-prune verification could join a pre-prune scan, see the stale row, and report a successful removal as a stale registration. The same gap defeats the post-archive-hook rechecks that exist to catch an external Git client locking the row. Restore the unshared export and make coalescing opt-in via `listWorktreesSharedStrict`, which existing callers already use deliberately. * fix(git): separate a proven absent ref from a failed probe `show-ref --verify --quiet` exits 1 for a missing ref, but so does `wsl.exe` when its own launch fails, so reading any exit 1 as absence collapsed `unverifiable` into `exited`. A genuine miss prints nothing while a wrapper failure always explains itself, so require empty stderr alongside the exit code; a runner that reports no stderr at all keeps its exit-code contract. That same signal removes a spawn regression: `show-ref` is a direct-git read under WSL, and the runner retried any numeric exit through the user's interactive login shell. The replaced `for-each-ref` exited 0 on a miss, so absence never retried; every absent probe now would. Treat a quiet exit 1 as Git control flow and skip the fallback. Also narrow the hosted-review suffix fallback: the replaced `refs/remotes/*/` could not cross a slash, but `show-ref -- ` matches at any depth, so `origin/feature/main` answered a query for `main` and submitted a review against a base the provider rejects. Refresh the real-binary compatibility contract to the shipped excludes, and assert exact probe concurrency rather than an upper bound so a regression to serial probing fails. --- AGENTS.md | 6 + .../command-runner/git-command-resolution.ts | 20 + src/main/git/command-runner/git-exec-file.ts | 7 +- src/main/git/exact-ref-probe.test.ts | 131 ++++++ src/main/git/exact-ref-probe.ts | 128 ++++++ src/main/git/repo-base-ref-search.ts | 95 +++- src/main/git/repo-branch-conflict.test.ts | 136 ++++++ src/main/git/repo-branch-conflict.ts | 100 ++++- src/main/git/repo-search-ref-compat.test.ts | 14 +- src/main/git/repo.test.ts | 71 ++- .../worktree-add-local-base-refresh.test.ts | 21 +- src/main/git/worktree-base-ref-probe.test.ts | 54 +++ src/main/git/worktree-base-ref-probe.ts | 26 +- .../git/worktree-scan-cache-sharing.test.ts | 125 +++++- src/main/git/worktree-scan-cache.ts | 33 +- src/main/git/worktree.ts | 6 +- ...stem-pull-request-field-generation.test.ts | 51 +++ src/main/ipc/filesystem.ts | 16 +- .../ipc/repos-remote-base-ref-queries.test.ts | 49 ++- src/main/ipc/repos/base-ref-query-handlers.ts | 12 +- ...orktree-remote-ssh-branch-conflict.test.ts | 11 +- src/main/ipc/worktree-remote.ts | 5 +- .../worktrees-ssh-base-ref-resolution.test.ts | 15 + ...rees-ssh-branch-conflict-suffixing.test.ts | 16 +- ...worktrees-ssh-create-base-prefetch.test.ts | 25 ++ ...ktrees-ssh-fork-push-target-remote.test.ts | 12 + .../worktrees-ssh-local-base-refresh.test.ts | 24 +- .../ipc/worktrees-ssh-pr-head-fetch.test.ts | 6 + .../ipc/worktrees-ssh-setup-launch.test.ts | 9 + src/main/runtime/orca-runtime.test.ts | 65 ++- src/main/runtime/orca-runtime.ts | 25 +- src/main/runtime/rpc/methods/repo.test.ts | 24 + .../runtime-git-generation-admission.test.ts | 10 + .../runtime-git-generation-commands.ts | 25 +- .../hosted-review-base-ref-suffix.test.ts | 77 ++++ ...hosted-review-creation-eligibility.test.ts | 269 +++++++++++- .../hosted-review-creation-git-state.ts | 160 ++++++- .../hosted-review-creation.test.ts | 23 +- .../pull-request-context-errors.test.ts | 156 ++++++- .../pull-request-context.test.ts | 409 +++++++++++++++++- .../text-generation/pull-request-context.ts | 88 ++-- .../pull-request-remote-ref-probes.ts | 199 +++++++++ src/relay/git-exec-validator.test.ts | 1 + src/shared/git-binary-compatibility.test.ts | 62 ++- src/shared/hosted-review-refs.test.ts | 27 +- src/shared/hosted-review-refs.ts | 14 + src/shared/repo-search-limits.test.ts | 69 +++ src/shared/repo-search-limits.ts | 45 ++ 48 files changed, 2750 insertions(+), 222 deletions(-) create mode 100644 src/main/git/exact-ref-probe.test.ts create mode 100644 src/main/git/exact-ref-probe.ts create mode 100644 src/main/git/repo-branch-conflict.test.ts create mode 100644 src/main/git/worktree-base-ref-probe.test.ts create mode 100644 src/main/source-control/hosted-review-base-ref-suffix.test.ts create mode 100644 src/main/text-generation/pull-request-remote-ref-probes.ts create mode 100644 src/shared/repo-search-limits.test.ts create mode 100644 src/shared/repo-search-limits.ts diff --git a/AGENTS.md b/AGENTS.md index 1fbce202438..9817cc41cc8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,6 +76,12 @@ When adding or changing a Git command: - Keep the real-binary compatibility contract in PR CI current. When adopting a newer Git feature, add its version boundary so the preferred command and fallback both run against representative Git releases. - Preserve commands that begin with global Git options such as `-c` before the subcommand, including auto-maintenance suppression used by worktree-create fetches. +## Git Scan Safety + +- Never enumerate every ref and then run `git ls-tree -r` or `git show` once per ref. That ref × tree fan-out can retain gigabytes of output before a downstream `sort -u` or search can make progress. +- Prefer `rg` over the checked-out files for source searches. For history or refs, use a named ref, an explicit namespace/path, `--max-count`, and a bounded output; do not use an unqualified `--all` scan as a first diagnostic. +- Keep repository-wide commands targeted to the current repository and worktree. If an unbounded scan is genuinely required, measure the ref count first, explain the cost, and get confirmation before running it. + ## Git Provider Compatibility Source-control and review changes must consider GitLab and other supported git providers, not only GitHub. Keep provider-specific behavior behind explicit checks, and avoid GitHub-only naming for generic review concepts. diff --git a/src/main/git/command-runner/git-command-resolution.ts b/src/main/git/command-runner/git-command-resolution.ts index ddde988b933..ffd022ad5ab 100644 --- a/src/main/git/command-runner/git-command-resolution.ts +++ b/src/main/git/command-runner/git-command-resolution.ts @@ -137,6 +137,26 @@ export function directWslGitExitCode(error: unknown, resolved: ResolvedCommand): return typeof code === 'number' ? code : null } +/** + * Exit 1 with nothing on stderr is Git answering "no" (a missing ref under + * `show-ref --verify --quiet`, no differences under `diff --quiet`), not a + * broken environment. `wsl.exe` always explains its own launch failures, so + * this cannot mask a dead distro. Retrying such an exit through the user's + * interactive login shell doubles the spawn count and runs the distro's rc + * files for a result the direct route already produced correctly. + */ +export function isQuietGitControlFlowExit(error: unknown): boolean { + if (!error || typeof error !== 'object') { + return false + } + const record = error as Record + if (record.code !== 1) { + return false + } + const stderr = record.stderr + return stderr !== undefined && stderr !== null && String(stderr).trim().length === 0 +} + export function invalidateMissingDirectWslGit(error: unknown, resolved: ResolvedCommand): boolean { const isMissing = isDirectWslGitNotFound(error, resolved) if (isMissing && resolved.wsl) { diff --git a/src/main/git/command-runner/git-exec-file.ts b/src/main/git/command-runner/git-exec-file.ts index 4fb28760d61..dbd861d4897 100644 --- a/src/main/git/command-runner/git-exec-file.ts +++ b/src/main/git/command-runner/git-exec-file.ts @@ -15,6 +15,7 @@ import { execFileCapture, execFileCaptureToTermination } from './exec-file-captu import { pendingWslDirectGitReadEnvironment, directWslGitExitCode, + isQuietGitControlFlowExit, disableDirectWslGitAfterSuccessfulFallback, invalidateMissingDirectWslGit, resolveGitCommand @@ -109,7 +110,11 @@ async function gitExecFileAsyncUnlocked( try { result = await capture(resolved) } catch (error) { - if (directWslGitExitCode(error, resolved) !== null && !options.signal?.aborted) { + if ( + directWslGitExitCode(error, resolved) !== null && + !isQuietGitControlFlowExit(error) && + !options.signal?.aborted + ) { await terminationState.current const wasMissing = invalidateMissingDirectWslGit(error, resolved) const fallback = resolveGitCommand( diff --git a/src/main/git/exact-ref-probe.test.ts b/src/main/git/exact-ref-probe.test.ts new file mode 100644 index 00000000000..b1ba0a3cd12 --- /dev/null +++ b/src/main/git/exact-ref-probe.test.ts @@ -0,0 +1,131 @@ +import { describe, expect, it, vi } from 'vitest' +import { isShowRefNoMatchError, probeAnyExactRef, probeExactRefs } from './exact-ref-probe' + +describe('isShowRefNoMatchError', () => { + it('accepts only the numeric Git no-match exit status', () => { + expect(isShowRefNoMatchError({ code: 1 })).toBe(true) + expect(isShowRefNoMatchError({ code: '1' })).toBe(false) + expect(isShowRefNoMatchError({ code: 'CONNECTION_LOST' })).toBe(false) + expect(isShowRefNoMatchError({ code: -32000 })).toBe(false) + }) +}) + +describe('probeExactRefs', () => { + it('preserves present, absent, and unknown states while deduplicating refs', async () => { + const present = 'refs/remotes/origin/main' + const absent = 'refs/remotes/upstream/main' + const unknown = 'refs/remotes/fork/main' + const invalid = 'refs/remotes/origin/bad*' + const runGit = vi.fn(async (argv: string[]) => { + if (argv.at(-1) === absent) { + throw Object.assign(new Error('no match'), { code: 1 }) + } + if (argv.at(-1) === unknown) { + throw Object.assign(new Error('transport failed'), { code: 128 }) + } + return { stdout: '' } + }) + + await expect( + probeExactRefs(runGit, [present, absent, unknown, invalid, '', present]) + ).resolves.toEqual({ + presentRefs: [present], + absentRefs: [absent], + unknownRefs: [unknown, invalid, ''] + }) + expect(runGit).toHaveBeenCalledTimes(3) + expect( + runGit.mock.calls.every( + ([argv]) => argv.slice(0, 4).join(' ') === 'show-ref --verify --quiet --' + ) + ).toBe(true) + }) + + it('forwards max-buffer and timeout options', async () => { + const runGit = vi.fn(async () => ({ stdout: '' })) + const ref = 'refs/remotes/origin/main' + + await probeExactRefs(runGit, [ref], { maxBuffer: 4096, timeoutMs: 30_000 }) + + expect(runGit).toHaveBeenCalledWith(['show-ref', '--verify', '--quiet', '--', ref], { + maxBuffer: 4096, + timeoutMs: 30_000 + }) + }) + + it('bounds concurrent exact lookups', async () => { + const refs = Array.from({ length: 20 }, (_, index) => `refs/remotes/remote-${index}/main`) + let active = 0 + let maxActive = 0 + const runGit = vi.fn(async () => { + active += 1 + maxActive = Math.max(maxActive, active) + await new Promise((resolve) => setTimeout(resolve, 0)) + active -= 1 + throw Object.assign(new Error('no match'), { code: 1 }) + }) + + await probeExactRefs(runGit, refs) + + expect(runGit).toHaveBeenCalledTimes(20) + // Equality, not a ceiling: 20 refs saturate the pool, so a regression to + // serial probing has to fail here. + expect(maxActive).toBe(8) + }) +}) + +describe('probeAnyExactRef', () => { + it('stops scheduling once a ref is present', async () => { + const refs = Array.from({ length: 20 }, (_, index) => `refs/remotes/remote-${index}/main`) + const runGit = vi.fn(async (argv: string[]) => { + if (argv.at(-1) === refs[0]) { + return { stdout: '' } + } + await new Promise((resolve) => setTimeout(resolve, 0)) + throw Object.assign(new Error('no match'), { code: 1 }) + }) + + await expect(probeAnyExactRef(runGit, refs)).resolves.toEqual({ + found: true, + unknown: false + }) + expect(runGit.mock.calls.length).toBeLessThanOrEqual(8) + }) + it('treats exit 1 with stderr as an inconclusive probe, not an absent ref', async () => { + // Why: `wsl.exe` exits 1 for its own launch failures. Reading that as + // absence would collapse `unverifiable` into `exited`. + const runGit = vi.fn(async () => { + throw Object.assign(new Error('wsl fail'), { + code: 1, + stderr: 'There is no distribution with the supplied name.' + }) + }) + + const result = await probeExactRefs(runGit, ['refs/remotes/origin/main']) + + expect(result.unknownRefs).toEqual(['refs/remotes/origin/main']) + expect(result.absentRefs).toEqual([]) + }) + + it('still reads a quiet exit 1 as an absent ref', async () => { + const runGit = vi.fn(async () => { + throw Object.assign(new Error('missing'), { code: 1, stderr: '' }) + }) + + const result = await probeExactRefs(runGit, ['refs/remotes/origin/main']) + + expect(result.absentRefs).toEqual(['refs/remotes/origin/main']) + expect(result.unknownRefs).toEqual([]) + }) + + it('keeps the exit-code contract for a runner that reports no stderr', async () => { + // The SSH provider rejects without a stderr field; absence must still resolve. + const runGit = vi.fn(async () => { + throw Object.assign(new Error('missing'), { code: 1 }) + }) + + const result = await probeExactRefs(runGit, ['refs/remotes/origin/main']) + + expect(result.absentRefs).toEqual(['refs/remotes/origin/main']) + }) +}) diff --git a/src/main/git/exact-ref-probe.ts b/src/main/git/exact-ref-probe.ts new file mode 100644 index 00000000000..6b13cc718c5 --- /dev/null +++ b/src/main/git/exact-ref-probe.ts @@ -0,0 +1,128 @@ +import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' + +export type ExactRefProbeExecOptions = { + maxBuffer?: number + timeoutMs?: number +} + +export type ExactRefProbeExec = ( + argv: string[], + options?: ExactRefProbeExecOptions +) => Promise<{ stdout: string }> + +export type ExactRefProbeSetResult = { + presentRefs: string[] + absentRefs: string[] + unknownRefs: string[] +} + +type ExactRefPresence = 'present' | 'absent' | 'unknown' + +const EXACT_REF_PROBE_CONCURRENCY = 8 + +export function isShowRefNoMatchError(error: unknown): boolean { + const record = error && typeof error === 'object' ? (error as Record) : undefined + // Git reports a missing ref as numeric exit status 1. Keep string-valued + // transport/error codes (including a relay that happens to use `"1"`) in + // the unknown bucket so SSH loss cannot look like an absent ref. + if (record?.code !== 1) { + return false + } + // `--quiet` makes Git print nothing for a missing ref, but a wrapper that + // also exits 1 always explains itself: `wsl.exe` on a dead distro, a relay + // transport error. Empty stderr is what separates proven absence from a + // probe that never ran. A runner that reports no stderr at all (the SSH + // provider) keeps its existing exit-code contract. + const stderr = record.stderr + return stderr === undefined || stderr === null || String(stderr).trim().length === 0 +} + +function commandOptions(options: ExactRefProbeExecOptions): ExactRefProbeExecOptions | undefined { + if (options.maxBuffer === undefined && options.timeoutMs === undefined) { + return undefined + } + return { ...options } +} + +async function probeExactRef( + runGit: ExactRefProbeExec, + ref: string, + options: ExactRefProbeExecOptions +): Promise { + if (!isSafeGitRefName(ref)) { + return 'unknown' + } + try { + const argv = ['show-ref', '--verify', '--quiet', '--', ref] + const forwardedOptions = commandOptions(options) + await (forwardedOptions ? runGit(argv, forwardedOptions) : runGit(argv)) + return 'present' + } catch (error) { + return isShowRefNoMatchError(error) ? 'absent' : 'unknown' + } +} + +/** Probe full ref names with bounded subprocess concurrency and exact lookups. */ +export async function probeExactRefs( + runGit: ExactRefProbeExec, + refs: readonly string[], + options: ExactRefProbeExecOptions = {} +): Promise { + const uniqueRefs = [...new Set(refs)] + const states: (ExactRefPresence | undefined)[] = Array.from( + { length: uniqueRefs.length }, + () => undefined + ) + let nextIndex = 0 + + async function probeNext(): Promise { + while (true) { + const index = nextIndex++ + if (index >= uniqueRefs.length) { + return + } + const ref = uniqueRefs[index] + states[index] = await probeExactRef(runGit, ref, options) + } + } + + const workerCount = Math.min(EXACT_REF_PROBE_CONCURRENCY, uniqueRefs.length) + await Promise.all(Array.from({ length: workerCount }, () => probeNext())) + for (let index = 0; index < states.length; index += 1) { + states[index] ??= 'unknown' + } + return { + presentRefs: uniqueRefs.filter((_, index) => states[index] === 'present'), + absentRefs: uniqueRefs.filter((_, index) => states[index] === 'absent'), + unknownRefs: uniqueRefs.filter((_, index) => states[index] === 'unknown') + } +} + +/** Stop scheduling exact lookups once any requested ref is present. */ +export async function probeAnyExactRef( + runGit: ExactRefProbeExec, + refs: readonly string[], + options: ExactRefProbeExecOptions = {} +): Promise<{ found: boolean; unknown: boolean }> { + const uniqueRefs = [...new Set(refs)] + let nextIndex = 0 + let found = false + let unknown = false + + async function probeNext(): Promise { + while (!found) { + const index = nextIndex++ + if (index >= uniqueRefs.length) { + return + } + const ref = uniqueRefs[index] + const state = await probeExactRef(runGit, ref, options) + found ||= state === 'present' + unknown ||= state === 'unknown' + } + } + + const workerCount = Math.min(EXACT_REF_PROBE_CONCURRENCY, uniqueRefs.length) + await Promise.all(Array.from({ length: workerCount }, () => probeNext())) + return { found, unknown } +} diff --git a/src/main/git/repo-base-ref-search.ts b/src/main/git/repo-base-ref-search.ts index 213868fe5a7..69e5e8eb999 100644 --- a/src/main/git/repo-base-ref-search.ts +++ b/src/main/git/repo-base-ref-search.ts @@ -1,5 +1,14 @@ import type { BaseRefSearchResult } from '../../shared/repo-types' import { isForEachRefExcludeUnsupportedError } from '../../shared/git-ref-command-capabilities' +import { + clampRepoSearchRefsLimit, + clampRepoSearchRefsScanLimit, + REPO_SEARCH_REFS_DEFAULT_LIMIT, + isRepoSearchRefsRequestLimit, + isRepoSearchRefsScanLimit +} from '../../shared/repo-search-limits' +import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' +import { isRemoteHeadRef } from '../../shared/hosted-review-refs' import { getLocalGitCapabilityCache } from './git-capability-state' import { gitExecOptions, type LocalGitExecOptions } from './repo-default-base-ref' import { gitExecFileAsync } from './runner' @@ -14,13 +23,34 @@ function getRefSearchTokens(normalizedQuery: string): string[] { } function getRefSearchCandidateCount(limit: number, excludesRemoteHead: boolean): number { - if (!Number.isInteger(limit) || limit <= 0) { + if (!isRepoSearchRefsScanLimit(limit)) { throw new Error('invalid_limit') } const baseCount = limit * REF_SEARCH_CANDIDATE_MULTIPLIER return excludesRemoteHead ? baseCount : baseCount + REF_SEARCH_LEGACY_HEADROOM } +/** Build excludes for the symbolic `/HEAD` slot without hiding + * nested branch names such as `/feature/HEAD`. */ +function getRemoteHeadExcludes(remoteNames: readonly string[] | undefined): string[] { + if (remoteNames && remoteNames.length > 0) { + // A single-component wildcard covers the overwhelmingly common remote + // shape. Keep exact excludes only for slash-containing remote names, where + // that wildcard cannot reach the direct `/HEAD` slot without also + // hiding legal nested branches such as `/feature/HEAD`. + const slashRemotes = [...new Set(remoteNames)].filter( + (remote) => remote.includes('/') && isSafeGitRefName(`refs/remotes/${remote}/HEAD`) + ) + return [ + '--exclude=refs/remotes/*/HEAD', + ...slashRemotes.map((remote) => `--exclude=refs/remotes/${remote}/HEAD`) + ] + } + // `*` does not cross `/`, unlike `**`; this keeps unknown nested remotes + // eligible for the parser's branch-name matching. + return ['--exclude=refs/remotes/*/HEAD'] +} + /** Build the bounded `for-each-ref` argv shared by local and remote searches. */ export function buildSearchBaseRefsArgv( normalizedQuery: string, @@ -32,12 +62,15 @@ export function buildSearchBaseRefsArgv( } = {} ): string[] { const excludeRemoteHead = options.excludeRemoteHead ?? true - const candidateCount = getRefSearchCandidateCount(limit, excludeRemoteHead) + // A caller may ask for more rows than the retained-result cap. Keep that + // request useful while bounding the Git command to the safe probe window. + const boundedScanLimit = clampRepoSearchRefsScanLimit(limit) + const candidateCount = getRefSearchCandidateCount(boundedScanLimit, excludeRemoteHead) const base = [ 'for-each-ref', '--format=%(refname)%00%(refname:short)', '--sort=-committerdate', - ...(excludeRemoteHead ? ['--exclude=refs/remotes/**/HEAD'] : []), + ...(excludeRemoteHead ? getRemoteHeadExcludes(options.remoteNames) : []), `--count=${candidateCount}` ] const tokens = getRefSearchTokens(normalizedQuery) @@ -128,18 +161,27 @@ export function mergeBaseRefSearchResultGroups( return merged } -export async function searchBaseRefs(path: string, query: string, limit = 25): Promise { - return (await searchBaseRefDetails(path, query, limit)).map((entry) => entry.refName) +export async function searchBaseRefs( + path: string, + query: string, + limit = REPO_SEARCH_REFS_DEFAULT_LIMIT +): Promise { + if (!isRepoSearchRefsRequestLimit(limit)) { + return [] + } + const boundedLimit = clampRepoSearchRefsLimit(limit) + return (await searchBaseRefDetails(path, query, boundedLimit)).map((entry) => entry.refName) } export async function searchBaseRefDetails( path: string, query: string, - limit = 25 + limit = REPO_SEARCH_REFS_DEFAULT_LIMIT ): Promise { - if (!Number.isInteger(limit) || limit <= 0) { + if (!isRepoSearchRefsRequestLimit(limit)) { return [] } + const boundedScanLimit = clampRepoSearchRefsScanLimit(limit) const normalizedQuery = normalizeRefSearchQuery(query) try { @@ -147,25 +189,27 @@ export async function searchBaseRefDetails( const tokens = getRefSearchTokens(normalizedQuery) if (tokens.length > 1) { const results = await Promise.all([ - runSearchBaseRefsGit(path, normalizedQuery, limit, { + runSearchBaseRefsGit(path, normalizedQuery, boundedScanLimit, { remoteNames: remotes, patternGroup: 'segmented' }), - runSearchBaseRefsGit(path, normalizedQuery, limit, { + runSearchBaseRefsGit(path, normalizedQuery, boundedScanLimit, { remoteNames: remotes, patternGroup: 'branchRoot' }) ]) return mergeBaseRefSearchResultGroups( - results.map((entry) => parseAndFilterSearchRefDetails(entry.stdout, limit, remotes)), - limit + results.map((entry) => + parseAndFilterSearchRefDetails(entry.stdout, boundedScanLimit, remotes) + ), + boundedScanLimit ) } - const result = await runSearchBaseRefsGit(path, normalizedQuery, limit, { + const result = await runSearchBaseRefsGit(path, normalizedQuery, boundedScanLimit, { remoteNames: remotes }) - return parseAndFilterSearchRefDetails(result.stdout, limit, remotes) + return parseAndFilterSearchRefDetails(result.stdout, boundedScanLimit, remotes) } catch (err) { console.warn('[searchBaseRefs] for-each-ref failed', { path, err }) return [] @@ -194,16 +238,37 @@ export function parseAndFilterSearchRefDetails( ): BaseRefSearchResult[] { const seen = new Set() const sortedRemotes = [...remotes].sort((a, b) => b.length - a.length) + + const canonicalShortRef = (fullRef: string, gitShortRef: string): string => { + // Git's refname:short DWIM rule can strip a trailing `/HEAD` (for example, + // `refs/remotes/origin/feature/HEAD` becomes `origin/feature`). Derive the + // display name only for that case; otherwise Git's disambiguation prefixes + // (such as `heads/` and `remotes/`) are significant and must be retained. + if ( + fullRef.startsWith('refs/remotes/') && + fullRef.endsWith('/HEAD') && + !gitShortRef.endsWith('/HEAD') + ) { + return fullRef.slice('refs/remotes/'.length) + } + return gitShortRef + } + return stdout .split('\n') .map((line) => line.trim()) .filter((line) => line.length > 0) .map((line) => { const nul = line.indexOf('\0') - return nul === -1 ? null : { full: line.slice(0, nul), short: line.slice(nul + 1) } + if (nul === -1) { + return null + } + const full = line.slice(0, nul) + const gitShort = line.slice(nul + 1) + return { full, short: canonicalShortRef(full, gitShort) } }) .filter((entry): entry is { full: string; short: string } => entry !== null) - .filter(({ full }) => !/^refs\/remotes\/.+\/HEAD$/.test(full)) + .filter(({ full }) => !isRemoteHeadRef(full, sortedRemotes)) .filter(({ short }) => { if (seen.has(short)) { return false diff --git a/src/main/git/repo-branch-conflict.test.ts b/src/main/git/repo-branch-conflict.test.ts new file mode 100644 index 00000000000..dab8dd396d6 --- /dev/null +++ b/src/main/git/repo-branch-conflict.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it, vi } from 'vitest' + +import { getBranchConflictKindViaExec } from './repo-branch-conflict' + +describe('getBranchConflictKindViaExec', () => { + it('probes exact configured remote refs instead of enumerating the remote namespace', async () => { + const calls: string[][] = [] + const exec = async (argv: string[]): Promise<{ stdout: string }> => { + calls.push(argv) + if (argv[0] === 'rev-parse') { + throw new Error('local branch is absent') + } + if (argv[0] === 'remote') { + return { stdout: 'origin\nfoo/bar\n' } + } + if (argv[0] === 'show-ref') { + return { stdout: 'abc refs/remotes/foo/bar/feature/fix\n' } + } + throw new Error(`unexpected git command: ${argv.join(' ')}`) + } + + await expect(getBranchConflictKindViaExec(exec, 'feature/fix')).resolves.toBe('remote') + expect(calls).toEqual([ + ['rev-parse', '--verify', 'refs/heads/feature/fix'], + ['remote'], + ['show-ref', '--verify', '--quiet', '--', 'refs/remotes/foo/bar/feature/fix'], + ['show-ref', '--verify', '--quiet', '--', 'refs/remotes/origin/feature/fix'] + ]) + }) + + it('does not run a ref query when the only candidate is the allowed base', async () => { + const calls: string[][] = [] + const exec = async (argv: string[]): Promise<{ stdout: string }> => { + calls.push(argv) + if (argv[0] === 'remote') { + return { stdout: 'origin\n' } + } + throw new Error('the local branch and remote ref are absent') + } + + await expect( + getBranchConflictKindViaExec(exec, 'feature/fix', 'origin/feature/fix') + ).resolves.toBeNull() + expect(calls).toEqual([['rev-parse', '--verify', 'refs/heads/feature/fix'], ['remote']]) + }) + + it('keeps longest configured remote-name matching semantics', async () => { + const calls: string[][] = [] + const exec = async (argv: string[]): Promise<{ stdout: string }> => { + calls.push(argv) + if (argv[0] === 'rev-parse') { + throw new Error('local branch is absent') + } + if (argv[0] === 'remote') { + return { stdout: 'foo\nfoo/bar\n' } + } + if (argv[0] === 'show-ref') { + return { stdout: 'abc refs/remotes/foo/bar/bar/feature\n' } + } + throw new Error(`unexpected git command: ${argv.join(' ')}`) + } + + await expect(getBranchConflictKindViaExec(exec, 'bar/feature')).resolves.toBe('remote') + expect(calls.at(-1)).toEqual([ + 'show-ref', + '--verify', + '--quiet', + '--', + 'refs/remotes/foo/bar/bar/feature' + ]) + }) + + it('does not treat a nested branch ref as an exact conflict', async () => { + const calls: string[][] = [] + const exec = async (argv: string[]): Promise<{ stdout: string }> => { + calls.push(argv) + if (argv[0] === 'rev-parse') { + throw new Error('local branch is absent') + } + if (argv[0] === 'remote') { + return { stdout: 'origin\n' } + } + if (argv[0] === 'show-ref') { + // The exact ref is absent even though a descendant exists. + throw new Error('missing exact ref') + } + throw new Error(`unexpected git command: ${argv.join(' ')}`) + } + + await expect(getBranchConflictKindViaExec(exec, 'feature')).resolves.toBeNull() + expect(calls.at(-1)).toEqual([ + 'show-ref', + '--verify', + '--quiet', + '--', + 'refs/remotes/origin/feature' + ]) + }) + + it('bounds concurrent exact probes when a repository has many remotes', async () => { + const remoteNames = Array.from({ length: 12 }, (_, index) => `remote-${index}`) + let probeCount = 0 + let activeProbes = 0 + let maxActiveProbes = 0 + const exec = async (argv: string[]): Promise<{ stdout: string }> => { + if (argv[0] === 'rev-parse') { + throw new Error('local branch is absent') + } + if (argv[0] === 'remote') { + return { stdout: `${remoteNames.join('\n')}\n` } + } + if (argv[0] === 'show-ref') { + probeCount += 1 + activeProbes += 1 + maxActiveProbes = Math.max(maxActiveProbes, activeProbes) + await new Promise((resolve) => setTimeout(resolve, 0)) + activeProbes -= 1 + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + throw new Error(`unexpected git command: ${argv.join(' ')}`) + } + + await expect(getBranchConflictKindViaExec(exec, 'feature')).resolves.toBeNull() + expect(probeCount).toBe(12) + // Equality, not a ceiling: 12 candidates saturate the pool, so a regression + // to serial probing has to fail here. + expect(maxActiveProbes).toBe(8) + }) + + it('does not turn an invalid branch name into a ref glob', async () => { + const exec = vi.fn(async () => ({ stdout: '' })) + + await expect(getBranchConflictKindViaExec(exec, 'feature*')).resolves.toBeNull() + expect(exec).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/git/repo-branch-conflict.ts b/src/main/git/repo-branch-conflict.ts index fcb6d8dc101..162d5ef53b3 100644 --- a/src/main/git/repo-branch-conflict.ts +++ b/src/main/git/repo-branch-conflict.ts @@ -1,21 +1,51 @@ import { resolveConfiguredRemoteBranchName } from './repo-base-ref-search' -import { gitExecOptions, type GitExec, type LocalGitExecOptions } from './repo-default-base-ref' +import { gitExecOptions, type LocalGitExecOptions } from './repo-default-base-ref' import { gitExecFileAsync } from './runner' +import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' +import { + probeAnyExactRef, + type ExactRefProbeExec, + type ExactRefProbeExecOptions +} from './exact-ref-probe' export type BranchConflictKind = 'local' | 'remote' -async function hasGitRefAsync(exec: GitExec, ref: string): Promise { +function runGit( + exec: ExactRefProbeExec, + args: string[], + options: ExactRefProbeExecOptions +): Promise<{ stdout: string }> { + return options.maxBuffer === undefined && options.timeoutMs === undefined + ? exec(args) + : exec(args, options) +} + +function canQueryRemoteBranchName(branchName: string): boolean { + // Validate the complete local ref before interpolating the name into any Git + // argument. This rejects glob/control/refspec syntax while retaining valid + // slash-containing branch names. + return !branchName.startsWith('-') && isSafeGitRefName(`refs/heads/${branchName}`) +} + +async function hasGitRefAsync( + exec: ExactRefProbeExec, + ref: string, + options: ExactRefProbeExecOptions +): Promise { try { - const { stdout } = await exec(['rev-parse', '--verify', ref]) + const { stdout } = await runGit(exec, ['rev-parse', '--verify', ref], options) return stdout.trim().length > 0 } catch { return false } } -async function listRemoteNamesViaExec(exec: GitExec): Promise { +async function listRemoteNamesViaExec( + exec: ExactRefProbeExec, + options: ExactRefProbeExecOptions +): Promise { try { - const { stdout } = await exec(['remote']) + const { stdout } = await runGit(exec, ['remote'], options) return stdout .split(/\r?\n/) .map((line) => line.trim()) @@ -26,26 +56,55 @@ async function listRemoteNamesViaExec(exec: GitExec): Promise { } } +function buildRemoteBranchConflictRefs( + remoteNames: readonly string[], + branchName: string, + allowedBaseRef: string | undefined +): string[] { + const refs = new Set() + for (const remoteName of remoteNames) { + const ref = `refs/remotes/${remoteName}/${branchName}` + if (!isSafeGitRefName(ref)) { + continue + } + // Match the conflict policy's longest-remote-prefix interpretation when + // remote names overlap (for example, `foo` and `foo/bar`). + if ( + !isAllowedRemoteBaseRef(ref, allowedBaseRef) && + resolveConfiguredRemoteBranchName(ref, remoteNames) === branchName + ) { + refs.add(ref) + } + } + return [...refs] +} + /** Run branch-conflict policy through the host that owns Git execution. */ export async function getBranchConflictKindViaExec( - exec: GitExec, + exec: ExactRefProbeExec, branchName: string, - allowedBaseRef?: string + allowedBaseRef?: string, + options: ExactRefProbeExecOptions = {} ): Promise { - if (await hasGitRefAsync(exec, `refs/heads/${branchName}`)) { + if (!canQueryRemoteBranchName(branchName)) { + return null + } + // Preserve the host runner's existing output/timeout contract. Exact probes + // are quiet, so introducing a smaller implicit cap would only make a large + // remote configuration look like a missing conflict. + const probeOptions: ExactRefProbeExecOptions = options + if (await hasGitRefAsync(exec, `refs/heads/${branchName}`, probeOptions)) { return 'local' } try { - const remoteNames = await listRemoteNamesViaExec(exec) - const { stdout } = await exec(['for-each-ref', '--format=%(refname)', 'refs/remotes']) - const hasRemoteConflict = stdout.split(/\r?\n/).some((ref) => { - const trimmed = ref.trim() - if (isAllowedRemoteBaseRef(trimmed, allowedBaseRef)) { - return false - } - return resolveConfiguredRemoteBranchName(trimmed, remoteNames) === branchName - }) + const remoteNames = await listRemoteNamesViaExec(exec, probeOptions) + const candidateRefs = buildRemoteBranchConflictRefs(remoteNames, branchName, allowedBaseRef) + if (candidateRefs.length === 0) { + return null + } + + const { found: hasRemoteConflict } = await probeAnyExactRef(exec, candidateRefs, probeOptions) return hasRemoteConflict ? 'remote' : null } catch { @@ -61,7 +120,12 @@ export function getBranchConflictKind( ): Promise { const execOptions = gitExecOptions(path, options) return getBranchConflictKindViaExec( - (argv) => gitExecFileAsync(argv, execOptions), + (argv, commandOptions) => + gitExecFileAsync(argv, { + ...execOptions, + ...(commandOptions?.maxBuffer === undefined ? {} : { maxBuffer: commandOptions.maxBuffer }), + ...(commandOptions?.timeoutMs === undefined ? {} : { timeout: commandOptions.timeoutMs }) + }), branchName, allowedBaseRef ) diff --git a/src/main/git/repo-search-ref-compat.test.ts b/src/main/git/repo-search-ref-compat.test.ts index 7c29357f55d..226f3278317 100644 --- a/src/main/git/repo-search-ref-compat.test.ts +++ b/src/main/git/repo-search-ref-compat.test.ts @@ -23,7 +23,7 @@ describe('searchBaseRefs git compatibility', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } - if (args.includes('--exclude=refs/remotes/**/HEAD')) { + if (args.some((arg) => arg.startsWith('--exclude=refs/remotes/'))) { throw Object.assign(new Error("unknown option `exclude'"), { stderr: "error: unknown option `exclude'" }) @@ -43,9 +43,15 @@ describe('searchBaseRefs git compatibility', () => { (call) => (call[0] as string[])[0] === 'for-each-ref' ) expect(forEachRefCalls).toHaveLength(3) - expect(forEachRefCalls[0][0]).toContain('--exclude=refs/remotes/**/HEAD') - expect(forEachRefCalls[1][0]).not.toContain('--exclude=refs/remotes/**/HEAD') + expect( + (forEachRefCalls[0][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(true) + expect( + (forEachRefCalls[1][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(false) expect(forEachRefCalls[1][0]).toContain('--count=104') - expect(forEachRefCalls[2][0]).not.toContain('--exclude=refs/remotes/**/HEAD') + expect( + (forEachRefCalls[2][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(false) }) }) diff --git a/src/main/git/repo.test.ts b/src/main/git/repo.test.ts index 7500735d72f..f08ce25eca3 100644 --- a/src/main/git/repo.test.ts +++ b/src/main/git/repo.test.ts @@ -14,6 +14,10 @@ import { searchBaseRefDetails, searchBaseRefs } from './repo' +import { + REPO_SEARCH_REFS_MAX_LIMIT, + REPO_SEARCH_REFS_MAX_SCAN_LIMIT +} from '../../shared/repo-search-limits' // Why: use real git state (not mocked) because the bug is in the for-each-ref glob shape a mock would miss. @@ -43,7 +47,7 @@ describe('buildSearchBaseRefsArgv', () => { it('caps broad local ref searches before parsing results', () => { const argv = buildSearchBaseRefsArgv('feature', 25) - expect(argv).toContain('--exclude=refs/remotes/**/HEAD') + expect(argv).toContain('--exclude=refs/remotes/*/HEAD') expect(argv).toContain('--count=100') expect(argv).toContain('refs/heads/**/*feature*') expect(argv).toContain('refs/remotes/**/*feature*/**') @@ -52,7 +56,7 @@ describe('buildSearchBaseRefsArgv', () => { it('keeps segmented display-format searches bounded', () => { const argv = buildSearchBaseRefsArgv('upstream/main', 10) - expect(argv).toContain('--exclude=refs/remotes/**/HEAD') + expect(argv).toContain('--exclude=refs/remotes/*/HEAD') expect(argv).toContain('--count=40') expect(argv).toContain('refs/remotes/*upstream*/*main*') expect(argv).toContain('refs/heads/*upstream*/*main*') @@ -60,6 +64,21 @@ describe('buildSearchBaseRefsArgv', () => { expect(argv).toContain('refs/heads/upstream/main*') }) + it('keeps remote HEAD excludes compact when many remotes are configured', () => { + const ordinaryRemotes = Array.from({ length: 200 }, (_, index) => `remote-${index}`) + const argv = buildSearchBaseRefsArgv('feature', 10, { + remoteNames: [...ordinaryRemotes, 'origin', 'upstream', 'origin', 'foo/bar', 'foo/bar'] + }) + const excludes = argv.filter((arg) => arg.startsWith('--exclude=')) + + // One wildcard handles ordinary remotes; slash-containing names need an + // exact pattern because `*` does not cross the remote-name slash. + expect(excludes).toEqual([ + '--exclude=refs/remotes/*/HEAD', + '--exclude=refs/remotes/foo/bar/HEAD' + ]) + }) + it('anchors local-branch-name searches below configured remotes', () => { const argv = buildSearchBaseRefsArgv('plan/docs', 10, { remoteNames: ['origin', 'foo/bar'] }) @@ -321,7 +340,7 @@ describe('searchBaseRefs (widened glob)', () => { it('caps broad ref-search argv before git output is captured', () => { const argv = buildSearchBaseRefsArgv('', 12) - expect(argv).toContain('--exclude=refs/remotes/**/HEAD') + expect(argv).toContain('--exclude=refs/remotes/*/HEAD') expect(argv).toContain('--count=48') }) @@ -331,9 +350,24 @@ describe('searchBaseRefs (widened glob)', () => { expect(argv).toContain('--count=2400') }) - it('returns [] for invalid search limits instead of running an uncapped search', async () => { + it('clamps oversized limits before constructing an unbounded Git count', () => { + expect(buildSearchBaseRefsArgv('', REPO_SEARCH_REFS_MAX_LIMIT)).toContain('--count=4000') + expect(buildSearchBaseRefsArgv('', REPO_SEARCH_REFS_MAX_SCAN_LIMIT)).toContain('--count=4004') + expect(buildSearchBaseRefsArgv('', REPO_SEARCH_REFS_MAX_SCAN_LIMIT + 1)).toContain( + '--count=4004' + ) + expect(buildSearchBaseRefsArgv('', Number.MAX_SAFE_INTEGER)).toContain('--count=4004') + expect(() => buildSearchBaseRefsArgv('', Number.MAX_VALUE)).toThrow('invalid_limit') + }) + + it('rejects malformed limits instead of running an uncapped search', async () => { await expect(searchBaseRefs(tmpDir, '', 0.5)).resolves.toEqual([]) await expect(searchBaseRefs(tmpDir, '', Number.NaN)).resolves.toEqual([]) + await expect( + searchBaseRefs(tmpDir, '', REPO_SEARCH_REFS_MAX_SCAN_LIMIT + 1) + ).resolves.toContain('main') + await expect(searchBaseRefs(tmpDir, '', Number.MAX_SAFE_INTEGER)).resolves.toContain('main') + await expect(searchBaseRefs(tmpDir, '', Number.MAX_VALUE)).resolves.toEqual([]) }) // Why: users retype the displayed `/` format, so a slashed query must still match. @@ -381,6 +415,35 @@ describe('searchBaseRefs (widened glob)', () => { expect(results).not.toContain('upstream/HEAD') }) + it('keeps nested branches whose final component is HEAD', async () => { + const sha = getHeadSha(tmpDir) + git(tmpDir, ['remote', 'add', 'upstream', 'https://example.invalid/upstream.git']) + createRemoteRef(tmpDir, 'upstream/main', sha) + createRemoteRef(tmpDir, 'upstream/feature/HEAD', sha) + git(tmpDir, ['symbolic-ref', 'refs/remotes/upstream/HEAD', 'refs/remotes/upstream/main']) + + const results = await searchBaseRefs(tmpDir, 'feature/HEAD') + + expect(results).toContain('upstream/feature/HEAD') + expect(results).not.toContain('upstream/HEAD') + }) + + it('preserves Git disambiguation prefixes for colliding local and remote refs', () => { + const results = parseAndFilterSearchRefDetails( + [ + 'refs/heads/origin/main\0heads/origin/main', + 'refs/remotes/origin/main\0remotes/origin/main' + ].join('\n'), + 10, + ['origin'] + ) + + expect(results.map((result) => result.refName)).toEqual([ + 'heads/origin/main', + 'remotes/origin/main' + ]) + }) + it('tolerates trailing, leading, and doubled slashes in the query', async () => { const sha = getHeadSha(tmpDir) createRemoteRef(tmpDir, 'upstream/main', sha) diff --git a/src/main/git/worktree-add-local-base-refresh.test.ts b/src/main/git/worktree-add-local-base-refresh.test.ts index cec1151e0eb..dba4303647c 100644 --- a/src/main/git/worktree-add-local-base-refresh.test.ts +++ b/src/main/git/worktree-add-local-base-refresh.test.ts @@ -347,7 +347,7 @@ describe('addWorktree', () => { it('skips updating the local branch when it has diverged', async () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} gitExecFileAsyncMock.mockRejectedValueOnce(new Error('not a fast-forward')) - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // for-each-ref refs/heads/main (exists) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // show-ref refs/heads/main (exists) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree add resolveCreationBaseConfigWrite() gitExecFileAsyncMock.mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -370,7 +370,7 @@ describe('addWorktree', () => { expect.objectContaining({ cwd: '/repo' }) ], [ - ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/heads/main'], + ['show-ref', '--verify', '--quiet', '--', 'refs/heads/main'], expect.objectContaining({ cwd: '/repo' }) ], [ @@ -415,7 +415,7 @@ describe('addWorktree', () => { "fatal: ambiguous argument 'refs/heads/feature-x...refs/remotes/origin/feature-x': unknown revision or path not in the working tree." ) ) // rev-list: refs/heads/feature-x does not exist yet - .mockResolvedValueOnce({ stdout: '' }) // for-each-ref refs/heads/feature-x (missing) + .mockRejectedValueOnce(Object.assign(new Error('missing ref'), { code: 1 })) // show-ref refs/heads/feature-x (missing) .mockResolvedValueOnce({ stdout: '' }) // worktree add .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -449,7 +449,7 @@ describe('addWorktree', () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} .mockRejectedValueOnce(new Error('unknown revision refs/heads/main')) // rev-list: no local main - .mockResolvedValueOnce({ stdout: '' }) // for-each-ref refs/heads/main (missing) + .mockRejectedValueOnce(Object.assign(new Error('missing ref'), { code: 1 })) // show-ref refs/heads/main (missing) .mockResolvedValueOnce({ stdout: '' }) // worktree add .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -459,9 +459,10 @@ describe('addWorktree', () => { expect(result.localBaseRefRefresh).toBeUndefined() expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).toContainEqual([ - 'for-each-ref', - '--count=1', - '--format=%(refname)', + 'show-ref', + '--verify', + '--quiet', + '--', 'refs/heads/main' ]) }) @@ -471,7 +472,7 @@ describe('addWorktree', () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} .mockRejectedValueOnce(new Error('rev-list failed')) // drift probe - .mockRejectedValueOnce(new Error('fatal: not a git repository')) // for-each-ref probe could not run + .mockRejectedValueOnce(new Error('fatal: not a git repository')) // show-ref probe could not run .mockResolvedValueOnce({ stdout: '' }) // worktree add .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -511,7 +512,7 @@ describe('addWorktree', () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'new-local\n' }) // rev-parse refs/heads/main^{commit} gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/origin/main^{commit} gitExecFileAsyncMock.mockRejectedValueOnce(new Error('not an ancestor')) // merge-base captured OIDs - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // for-each-ref refs/heads/main (exists) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // show-ref refs/heads/main (exists) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree add resolveCreationBaseConfigWrite() gitExecFileAsyncMock.mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -546,7 +547,7 @@ describe('addWorktree', () => { expect.objectContaining({ cwd: '/repo' }) ], [ - ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/heads/main'], + ['show-ref', '--verify', '--quiet', '--', 'refs/heads/main'], expect.objectContaining({ cwd: '/repo' }) ], [ diff --git a/src/main/git/worktree-base-ref-probe.test.ts b/src/main/git/worktree-base-ref-probe.test.ts new file mode 100644 index 00000000000..d27d26290c8 --- /dev/null +++ b/src/main/git/worktree-base-ref-probe.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it, vi } from 'vitest' +import { probeWorktreeBaseRefPresence } from './worktree-base-ref-probe' + +describe('probeWorktreeBaseRefPresence', () => { + it('uses an exact show-ref probe and reports a present ref', async () => { + const runGit = vi.fn().mockResolvedValue({ stdout: '' }) + + await expect(probeWorktreeBaseRefPresence(runGit, 'refs/heads/release/2026')).resolves.toBe( + 'present' + ) + expect(runGit).toHaveBeenCalledWith([ + 'show-ref', + '--verify', + '--quiet', + '--', + 'refs/heads/release/2026' + ]) + }) + + it('treats show-ref exit 1 as an absent ref', async () => { + const runGit = vi.fn().mockRejectedValue(Object.assign(new Error('missing ref'), { code: 1 })) + + await expect(probeWorktreeBaseRefPresence(runGit, 'refs/heads/release/2026')).resolves.toBe( + 'absent' + ) + }) + + it('keeps repository and transport failures inconclusive', async () => { + const runGit = vi + .fn() + .mockRejectedValue(Object.assign(new Error('not a git repository'), { code: 128 })) + + await expect(probeWorktreeBaseRefPresence(runGit, 'refs/heads/release/2026')).resolves.toBe( + 'unknown' + ) + }) + + it('does not treat a string transport code as a missing ref', async () => { + const runGit = vi + .fn() + .mockRejectedValue(Object.assign(new Error('connection lost'), { code: '1' })) + + await expect(probeWorktreeBaseRefPresence(runGit, 'refs/heads/release/2026')).resolves.toBe( + 'unknown' + ) + }) + + it('does not execute malformed ref input', async () => { + const runGit = vi.fn() + + await expect(probeWorktreeBaseRefPresence(runGit, 'refs/heads/*')).resolves.toBe('unknown') + expect(runGit).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/git/worktree-base-ref-probe.ts b/src/main/git/worktree-base-ref-probe.ts index 85c8c8baa8f..6ed19860eb3 100644 --- a/src/main/git/worktree-base-ref-probe.ts +++ b/src/main/git/worktree-base-ref-probe.ts @@ -1,4 +1,6 @@ import { gitExecFileAsync } from './runner' +import { isShowRefNoMatchError } from './exact-ref-probe' +import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' type GitExecOptions = { wslDistro?: string @@ -43,10 +45,9 @@ export type WorktreeBaseRefPresence = 'present' | 'absent' | 'unknown' /** * Distinguish "the ref does not exist" from "the probe itself failed". * - * Why for-each-ref: it exits 0 whether or not the pattern matches, so an empty result - * proves absence while a rejection still means the probe never ran (broken repo, dead - * SSH transport). `rev-parse --verify --quiet` exits 1 for both, and reading that as - * "absent" would silently drop warnings the caller must still surface. + * `show-ref --verify` is an exact lookup: exit 1 means a valid ref is absent, + * while other failures (for example a broken repo or dead SSH transport) stay + * inconclusive so callers can preserve their warning/error behavior. * * Executor-injected so the SSH path can route the same argv through the relay. */ @@ -54,15 +55,14 @@ export async function probeWorktreeBaseRefPresence( runGit: (args: string[]) => Promise<{ stdout: string }>, qualifiedRef: string ): Promise { - try { - const { stdout } = await runGit([ - 'for-each-ref', - '--count=1', - '--format=%(refname)', - qualifiedRef - ]) - return stdout.trim() === qualifiedRef ? 'present' : 'absent' - } catch { + // Reject malformed persisted metadata before passing it to Git. + if (!isSafeGitRefName(qualifiedRef)) { return 'unknown' } + try { + await runGit(['show-ref', '--verify', '--quiet', '--', qualifiedRef]) + return 'present' + } catch (error) { + return isShowRefNoMatchError(error) ? 'absent' : 'unknown' + } } diff --git a/src/main/git/worktree-scan-cache-sharing.test.ts b/src/main/git/worktree-scan-cache-sharing.test.ts index 1b9e702fbb3..d64ec2d4852 100644 --- a/src/main/git/worktree-scan-cache-sharing.test.ts +++ b/src/main/git/worktree-scan-cache-sharing.test.ts @@ -1,4 +1,4 @@ -// listWorktrees scan sharing: in-flight coalescing and mutation-generation retirement. +// Worktree scan sharing: in-flight coalescing and mutation-generation retirement. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { @@ -29,11 +29,13 @@ vi.mock('../worktree-trash', () => ({ import { _getWorktreeScanCacheSizesForTests, _resetWorktreeScanCacheForTests, + listWorktreeGraph, listWorktrees, listWorktreesSharedStrict, listWorktreesStrict, moveWorktree, removeWorktree, + notifyPreparedWorktreeMutation, WORKTREE_LIST_TIMEOUT_MS } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' @@ -76,6 +78,104 @@ describe('listWorktrees in-flight sharing', () => { expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) }) + it('shares one graph scan across concurrent callers without sparse probes', async () => { + let resolveScan!: () => void + const scanOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' + gitExecFileAsyncMock.mockImplementation( + () => + new Promise((resolve) => { + resolveScan = () => resolve({ stdout: scanOutput }) + }) + ) + + const first = listWorktreeGraph('/repo') + const second = listWorktreeGraph('/repo') + resolveScan() + const [a, b] = await Promise.all([first, second]) + + expect(a).toEqual(b) + expect(a[0]?.path).toBe('/repo') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('keeps graph and annotated scans separate despite sharing the same Git listing', async () => { + const resolvers: ((value: { stdout: string }) => void)[] = [] + gitExecFileAsyncMock.mockImplementation( + () => + new Promise((resolve) => { + resolvers.push(resolve) + }) + ) + + const graphScan = listWorktreeGraph('/repo') + const annotatedScan = listWorktrees('/repo') + expect(resolvers).toHaveLength(2) + + for (const resolve of resolvers) { + resolve({ stdout: 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' }) + } + await Promise.all([graphScan, annotatedScan]) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('keeps graph scans with an AbortSignal isolated from shared callers', async () => { + const scanOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' + gitExecFileAsyncMock.mockResolvedValue({ stdout: scanOutput }) + const controller = new AbortController() + + await Promise.all([ + listWorktreeGraph('/repo'), + listWorktreeGraph('/repo', { signal: controller.signal }) + ]) + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('keeps graph scans with different visibility or timeout contracts separate', async () => { + const resolvers: ((value: { stdout: string }) => void)[] = [] + gitExecFileAsyncMock.mockImplementation( + () => + new Promise((resolve) => { + resolvers.push(resolve) + }) + ) + + const defaultScan = listWorktreeGraph('/repo') + const preparationScan = listWorktreeGraph('/repo', { includeCreatePreparations: true }) + const shorterScan = listWorktreeGraph('/repo', { timeout: 5_000 }) + expect(resolvers).toHaveLength(3) + + for (const resolve of resolvers) { + resolve({ stdout: 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' }) + } + await Promise.all([defaultScan, preparationScan, shorterScan]) + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(3) + }) + + it('retires a graph scan that overlaps a worktree mutation', async () => { + const resolvers: ((value: { stdout: string }) => void)[] = [] + gitExecFileAsyncMock.mockImplementation((args: string[]) => + args[0] === 'worktree' && args[1] === 'list' + ? new Promise((resolve) => { + resolvers.push(resolve) + }) + : Promise.resolve({ stdout: '' }) + ) + + const staleScan = listWorktreeGraph('/repo') + expect(resolvers).toHaveLength(1) + notifyPreparedWorktreeMutation('/repo') + const freshScan = listWorktreeGraph('/repo') + expect(resolvers).toHaveLength(2) + + resolvers[1]?.({ stdout: 'worktree /repo\nHEAD fresh\nbranch refs/heads/main\n' }) + resolvers[0]?.({ stdout: 'worktree /repo\nHEAD stale\nbranch refs/heads/main\n' }) + expect((await freshScan)[0]?.head).toBe('fresh') + expect((await staleScan)[0]?.head).toBe('stale') + expect(_getWorktreeScanCacheSizesForTests()).toEqual({ inFlight: 0, generations: 0 }) + }) + // Why (#16520): create verification moved off the fail-soft listing so a Git failure stops being // hidden behind "created but not found in listing". That must not cost the in-flight coalescing, // and sharing must never hand a strict caller a softened result. @@ -129,6 +229,29 @@ describe('listWorktrees in-flight sharing', () => { await expect(strict).rejects.toThrow('git timed out.') }) + it('keeps listWorktreesStrict unshared for post-mutation verification', async () => { + // Why: a raw `git worktree prune` never bumps the scan generation, so a + // coalesced strict read could return the pre-prune row and report a + // successful removal as a stale registration. + const scanOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' + const resolvers: (() => void)[] = [] + gitExecFileAsyncMock.mockImplementation( + () => + new Promise((resolve) => { + resolvers.push(() => resolve({ stdout: scanOutput })) + }) + ) + + const first = listWorktreesStrict('/repo') + const second = listWorktreesStrict('/repo') + for (const resolve of resolvers) { + resolve() + } + await Promise.all([first, second]) + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + it('does not share scans across different timeout contracts', async () => { const scanOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' gitExecFileAsyncMock.mockResolvedValue({ stdout: scanOutput }) diff --git a/src/main/git/worktree-scan-cache.ts b/src/main/git/worktree-scan-cache.ts index 9c744afeba8..327aead5f2e 100644 --- a/src/main/git/worktree-scan-cache.ts +++ b/src/main/git/worktree-scan-cache.ts @@ -1,11 +1,17 @@ import type { GitWorktreeInfo } from '../../shared/worktree/types' -import { listWorktreesStrict, listWorktreesUnshared } from './worktree-listing' +import { + listWorktreeGraph as listWorktreeGraphUnshared, + listWorktreesStrict as listWorktreesStrictUnshared, + listWorktreesUnshared +} from './worktree-listing' import type { GitWorktreeExecOptions } from './worktree-operation-options' import { WORKTREE_LIST_TIMEOUT_MS } from './worktree-operation-options' // Why: share concurrent `git worktree list` scans, which are expensive on Windows. const inFlightWorktreeScans = new Map>() +type WorktreeScanKind = 'graph' | 'lenient' | 'strict' + // Why: mutation generations prevent listings from joining stale scans. const worktreeScanGenerations = new Map() @@ -50,14 +56,16 @@ export function _resetWorktreeScanCacheForTests(): void { } /** - * Share one in-flight scan per (repo, distro, deadline, generation, runner). Coalescing keeps a + * Share one in-flight scan per (repo, distro, deadline, generation, kind). Coalescing keeps a * sidebar refresh from spawning a second `git worktree list` (expensive on Windows), but only * within one failure discipline: a strict joiner must never inherit a lenient scan's softened `[]`, - * so the strict and lenient runners scan separately by design. + * so the strict and lenient runners scan separately by design. The explicit kind is intentional: + * function names can be rewritten by a production bundler and must not define cache identity. */ function shareWorktreeScan( repoPath: string, options: GitWorktreeExecOptions, + kind: WorktreeScanKind, run: (repoPath: string, options: GitWorktreeExecOptions) => Promise ): Promise { if (options.signal) { @@ -66,8 +74,8 @@ function shareWorktreeScan( const generation = worktreeScanGenerations.get(repoPath) ?? 0 const timeout = options.timeout ?? WORKTREE_LIST_TIMEOUT_MS // Why: callers with different deadlines cannot safely share which timeout wins the scan. - // Why `run.name`: a strict joiner must never receive a softened `[]` from a lenient scan. - const key = `${repoPath}\0${options.wslDistro ?? ''}\0${timeout}\0${options.includeCreatePreparations === true}\0${generation}\0${run.name}` + // Why `kind`: a strict joiner must never receive a softened `[]` from a lenient scan. + const key = `${repoPath}\0${options.wslDistro ?? ''}\0${timeout}\0${options.includeCreatePreparations === true}\0${generation}\0${kind}` const inFlight = inFlightWorktreeScans.get(key) if (inFlight) { return inFlight @@ -91,7 +99,18 @@ export function listWorktrees( repoPath: string, options: GitWorktreeExecOptions = {} ): Promise { - return shareWorktreeScan(repoPath, options, listWorktreesUnshared) + return shareWorktreeScan(repoPath, options, 'lenient', listWorktreesUnshared) +} + +/** + * List the worktree graph without sparse-checkout probes. Concurrent callers share the same + * generation-fenced scan, while callers with an AbortSignal retain an isolated subprocess. + */ +export function listWorktreeGraph( + repoPath: string, + options: GitWorktreeExecOptions = {} +): Promise { + return shareWorktreeScan(repoPath, options, 'graph', listWorktreeGraphUnshared) } /** @@ -102,5 +121,5 @@ export function listWorktreesSharedStrict( repoPath: string, options: GitWorktreeExecOptions = {} ): Promise { - return shareWorktreeScan(repoPath, options, listWorktreesStrict) + return shareWorktreeScan(repoPath, options, 'strict', listWorktreesStrictUnshared) } diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index aee68fc0949..d7daa09da1f 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -6,7 +6,10 @@ export { } from './worktree-add' export { forceDeleteLocalBranch } from './worktree-branch-removal' export { parseWorktreeList } from './worktree-list-parser' -export { describeCreatedWorktree, listWorktreeGraph, listWorktreesStrict } from './worktree-listing' +// Unshared by design: verification-after-mutation callers must not join an +// in-flight scan that predates a raw `git worktree prune` or an external client. +// Opt into coalescing with `listWorktreesSharedStrict`. +export { describeCreatedWorktree, listWorktreesStrict } from './worktree-listing' export { moveWorktree } from './worktree-move' export { WORKTREE_ADD_TIMEOUT_MAX_MS, @@ -27,6 +30,7 @@ export { removeWorktree } from './worktree-removal' export { _getWorktreeScanCacheSizesForTests, _resetWorktreeScanCacheForTests, + listWorktreeGraph, listWorktrees, listWorktreesSharedStrict } from './worktree-scan-cache' diff --git a/src/main/ipc/filesystem-pull-request-field-generation.test.ts b/src/main/ipc/filesystem-pull-request-field-generation.test.ts index a80b51b9c4e..b12e5d2b010 100644 --- a/src/main/ipc/filesystem-pull-request-field-generation.test.ts +++ b/src/main/ipc/filesystem-pull-request-field-generation.test.ts @@ -1,5 +1,6 @@ import path from 'node:path' import { beforeEach, describe, expect, it, vi } from 'vitest' +import * as gitRunner from '../git/runner' import { handlers, store, @@ -175,5 +176,55 @@ describe('registerFilesystemHandlers', () => { 'linkedIssue' ) }) + + it('forwards PR-context command limits through local and SSH adapters', async () => { + const localGitExec = vi + .spyOn(gitRunner, 'gitExecFileAsync') + .mockResolvedValue({ stdout: '', stderr: '' }) + const sshExec = vi.fn().mockResolvedValue({ stdout: '', stderr: '' }) + getSshGitProviderMock.mockReturnValue({ + exec: sshExec, + executeCommitMessagePlan: vi.fn() + }) + getPullRequestDraftContextMock.mockImplementation(async (execute) => { + await execute(['show-ref', '--verify'], { maxBuffer: 123, timeoutMs: 456 }) + await execute(['legacy-probe'], { timeout: 789 }) + return PULL_REQUEST_CONTEXT + }) + + try { + registerFilesystemHandlers(store as never) + + await handlers.get('git:generatePullRequestFields')!(null, { + ...PULL_REQUEST_ARGS, + worktreePath: WORKTREE_FEATURE_PATH + }) + + expect(localGitExec).toHaveBeenCalledWith(['show-ref', '--verify'], { + cwd: WORKTREE_FEATURE_PATH, + maxBuffer: 123, + timeout: 456 + }) + expect(localGitExec).toHaveBeenCalledWith(['legacy-probe'], { + cwd: WORKTREE_FEATURE_PATH, + timeout: 789 + }) + + await handlers.get('git:generatePullRequestFields')!(null, { + ...PULL_REQUEST_ARGS, + worktreePath: '/remote/repo', + connectionId: 'conn-1' + }) + + expect(sshExec).toHaveBeenCalledWith(['show-ref', '--verify'], '/remote/repo', { + timeoutMs: 456 + }) + expect(sshExec).toHaveBeenCalledWith(['legacy-probe'], '/remote/repo', { + timeoutMs: 789 + }) + } finally { + localGitExec.mockRestore() + } + }) }) }) diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 2bdc6faa9db..41b8b8e8da7 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -1709,7 +1709,12 @@ export function registerFilesystemHandlers( useTemplate: args.useTemplate }) context = await getPullRequestDraftContext( - (argv) => provider.exec(argv, args.worktreePath), + (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), { base: args.base, currentTitle: args.title, @@ -1767,7 +1772,14 @@ export function registerFilesystemHandlers( }) context = await getPullRequestDraftContext( (argv, options) => - gitExecFileAsync(argv, { cwd: worktreePath, ...gitOptions, ...options }), + gitExecFileAsync(argv, { + cwd: worktreePath, + ...gitOptions, + ...(options?.maxBuffer === undefined ? {} : { maxBuffer: options.maxBuffer }), + ...(options?.timeoutMs === undefined && options?.timeout === undefined + ? {} + : { timeout: options?.timeoutMs ?? options?.timeout }) + }), { base: args.base, currentTitle: args.title, diff --git a/src/main/ipc/repos-remote-base-ref-queries.test.ts b/src/main/ipc/repos-remote-base-ref-queries.test.ts index f96ff9c4da6..8ee9cb1cfc8 100644 --- a/src/main/ipc/repos-remote-base-ref-queries.test.ts +++ b/src/main/ipc/repos-remote-base-ref-queries.test.ts @@ -32,6 +32,7 @@ import { registerRepoHandlers } from './repos' import { clearGitCapabilityStateForTests } from '../git/git-capability-state' import { resetSshProviderAuthorities } from '../ssh/ssh-provider-authority' import { createRepoHandlerHarness } from './repos-remote-test-harness' +import { REPO_SEARCH_REFS_MAX_LIMIT } from '../../shared/repo-search-limits' const { handleMock, mockStore, mockGitProvider, prepareLocalWorktreeRootForRepoMock } = reposMocks @@ -282,7 +283,7 @@ describe('repos:searchBaseRefs SSH relay', () => { const [argv] = mockGitProvider.exec.mock.calls.find( (call) => (call[0] as string[])[0] === 'for-each-ref' )! - expect(argv).toContain('--exclude=refs/remotes/**/HEAD') + expect(argv.some((arg: string) => arg.startsWith('--exclude=refs/remotes/'))).toBe(true) expect(argv).toContain('--count=100') expect(argv).toContain('refs/heads/**/**') expect(argv).toContain('refs/heads/**/**/**') @@ -308,7 +309,7 @@ describe('repos:searchBaseRefs SSH relay', () => { const [argv] = mockGitProvider.exec.mock.calls.find( (call) => (call[0] as string[])[0] === 'for-each-ref' )! - expect(argv).toContain('--exclude=refs/remotes/**/HEAD') + expect(argv.some((arg: string) => arg.startsWith('--exclude=refs/remotes/'))).toBe(true) expect(argv).toContain('--count=100') expect(argv).toContain('refs/heads/**/**') expect(argv).toContain('refs/heads/**/**/**') @@ -334,6 +335,36 @@ describe('repos:searchBaseRefs SSH relay', () => { expect(mockGitProvider.exec).not.toHaveBeenCalled() }) + it('clamps oversized limits before building broad relay searches', async () => { + mockGitProvider.exec = vi.fn().mockImplementation((argv: string[]) => { + if (argv[0] === 'remote') { + return Promise.resolve({ stdout: 'origin\n', stderr: '' }) + } + return Promise.resolve({ + stdout: 'refs/remotes/origin/main\0origin/main', + stderr: '' + }) + }) + mockStore.getRepo.mockReturnValue({ + id: 'r1', + path: '/remote/repo', + connectionId: 'conn-1', + kind: 'git' + }) + + const result = await handlers.get('repos:searchBaseRefs')!(null, { + repoId: 'r1', + query: '', + limit: REPO_SEARCH_REFS_MAX_LIMIT + 1 + }) + + expect(result).toEqual(['origin/main']) + const forEachRefCall = mockGitProvider.exec.mock.calls.find( + (call) => (call[0] as string[])[0] === 'for-each-ref' + ) + expect(forEachRefCall?.[0]).toContain('--count=4000') + }) + it('retries without --exclude for older git on SSH hosts', async () => { const stdout = [ 'refs/remotes/origin/main\0origin/main', @@ -343,7 +374,7 @@ describe('repos:searchBaseRefs SSH relay', () => { if (argv[0] === 'remote') { return Promise.resolve({ stdout: 'origin\n', stderr: '' }) } - if (argv.includes('--exclude=refs/remotes/**/HEAD')) { + if (argv.some((arg) => arg.startsWith('--exclude=refs/remotes/'))) { return Promise.reject( Object.assign(new Error("unknown option `exclude'"), { stderr: "error: unknown option `exclude'" @@ -376,10 +407,16 @@ describe('repos:searchBaseRefs SSH relay', () => { (call) => (call[0] as string[])[0] === 'for-each-ref' ) expect(forEachRefCalls).toHaveLength(3) - expect(forEachRefCalls[0][0]).toContain('--exclude=refs/remotes/**/HEAD') - expect(forEachRefCalls[1][0]).not.toContain('--exclude=refs/remotes/**/HEAD') + expect( + (forEachRefCalls[0][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(true) + expect( + (forEachRefCalls[1][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(false) expect(forEachRefCalls[1][0]).toContain('--count=104') - expect(forEachRefCalls[2][0]).not.toContain('--exclude=refs/remotes/**/HEAD') + expect( + (forEachRefCalls[2][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(false) }) it('sends the widened `**` argv so all remotes and slash-named branches are discoverable', async () => { diff --git a/src/main/ipc/repos/base-ref-query-handlers.ts b/src/main/ipc/repos/base-ref-query-handlers.ts index b3f6f9d25ff..f638e7ce849 100644 --- a/src/main/ipc/repos/base-ref-query-handlers.ts +++ b/src/main/ipc/repos/base-ref-query-handlers.ts @@ -1,6 +1,11 @@ import { ipcMain } from 'electron' import type { Store } from '../../persistence' import type { BaseRefDefaultResult, BaseRefSearchResult, Repo } from '../../../shared/repo-types' +import { + clampRepoSearchRefsLimit, + REPO_SEARCH_REFS_DEFAULT_LIMIT, + isRepoSearchRefsRequestLimit +} from '../../../shared/repo-search-limits' import { isFolderRepo } from '../../../shared/repo-kind' import { getRepoExecutionHostId, type ExecutionHostId } from '../../../shared/execution-host' import { @@ -111,10 +116,13 @@ async function searchBaseRefDetailsForRepo( if (!repo || isFolderRepo(repo)) { return [] } - const limit = args.limit ?? 25 - if (!Number.isInteger(limit) || limit <= 0) { + const requestedLimit = args.limit ?? REPO_SEARCH_REFS_DEFAULT_LIMIT + if (!isRepoSearchRefsRequestLimit(requestedLimit)) { return [] } + // Keep the public IPC shape forgiving while bounding Git and retained results + // for callers that request an unusually large page. + const limit = clampRepoSearchRefsLimit(requestedLimit) // Why: remote repos need the relay to list branches on the remote host. if (repo.connectionId) { const provider = getSshGitProvider(repo.connectionId) diff --git a/src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts b/src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts index 7ced18eb173..e3e24e96a08 100644 --- a/src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts +++ b/src/main/ipc/worktree-remote-ssh-branch-conflict.test.ts @@ -19,8 +19,15 @@ function providerAnswering(answers: { } return { stdout: `${answers.localBranchHead}\n`, stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: `${answers.remoteRefs.join('\n')}\n`, stderr: '' } + if (args[0] === 'show-ref') { + const matches = answers.remoteRefs.filter((ref) => args.includes(ref)) + if (matches.length > 0) { + return { + stdout: `${matches.map((ref) => `abc ${ref}`).join('\n')}\n`, + stderr: '' + } + } + throw Object.assign(new Error('missing remote ref'), { code: 1 }) } throw new Error(`unexpected git ${args.join(' ')}`) }) diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 6fa40e5b1d1..e023b41dc60 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -854,7 +854,10 @@ export function getSshBranchConflictKind( allowedBaseRef: string ): Promise<'local' | 'remote' | null> { return getBranchConflictKindViaExec( - (argv) => provider.exec(argv, repoPath), + (argv, commandOptions) => + commandOptions?.timeoutMs === undefined + ? provider.exec(argv, repoPath) + : provider.exec(argv, repoPath, { timeoutMs: commandOptions.timeoutMs }), branchName, allowedBaseRef ) diff --git a/src/main/ipc/worktrees-ssh-base-ref-resolution.test.ts b/src/main/ipc/worktrees-ssh-base-ref-resolution.test.ts index 599acb1bcc7..1a32ce36a55 100644 --- a/src/main/ipc/worktrees-ssh-base-ref-resolution.test.ts +++ b/src/main/ipc/worktrees-ssh-base-ref-resolution.test.ts @@ -110,6 +110,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('ref not found'), { code: 1 }) + } if (args[0] === 'sparse-checkout' && args[1] === 'init') { throw setupError } @@ -168,6 +171,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('ref not found'), { code: 1 }) + } if (args[0] === 'fetch') { throw new Error('network unavailable') } @@ -226,6 +232,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'symbolic-ref') { return { stdout: 'refs/remotes/origin/main\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('ref not found'), { code: 1 }) + } if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/master^{commit}')) { return { stdout: '', stderr: '' } } @@ -301,6 +310,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'symbolic-ref') { return { stdout: 'refs/remotes/origin/main\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('ref not found'), { code: 1 }) + } if (args[0] === 'rev-parse' && args.includes('refs/heads/develop^{commit}')) { return { stdout: repoRootRegistered ? 'develop-sha\n' : '', stderr: '' } } @@ -373,6 +385,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'symbolic-ref') { return { stdout: 'refs/remotes/origin/main\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('ref not found'), { code: 1 }) + } if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/main')) { return { stdout: 'main-sha\n', stderr: '' } } diff --git a/src/main/ipc/worktrees-ssh-branch-conflict-suffixing.test.ts b/src/main/ipc/worktrees-ssh-branch-conflict-suffixing.test.ts index 73605e2332e..2d300fa6f60 100644 --- a/src/main/ipc/worktrees-ssh-branch-conflict-suffixing.test.ts +++ b/src/main/ipc/worktrees-ssh-branch-conflict-suffixing.test.ts @@ -206,8 +206,12 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'branch' && args.includes('feature/something')) { return { stdout: '', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'refs/remotes/origin/feature/something\n', stderr: '' } + if (args[0] === 'show-ref') { + const ref = 'refs/remotes/origin/feature/something' + if (args.includes(ref)) { + return { stdout: `abc ${ref}\n`, stderr: '' } + } + throw Object.assign(new Error('missing remote ref'), { code: 1 }) } if (args[0] === 'rev-parse' && args.includes('refs/heads/feature/something^{commit}')) { throw new Error('missing local branch') @@ -268,8 +272,12 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\nfoo/bar\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'refs/remotes/foo/bar/feature/something\n', stderr: '' } + if (args[0] === 'show-ref') { + const ref = 'refs/remotes/foo/bar/feature/something' + if (args.includes(ref)) { + return { stdout: `abc ${ref}\n`, stderr: '' } + } + throw Object.assign(new Error('missing remote ref'), { code: 1 }) } if (args[0] === 'rev-parse' && args.includes('refs/heads/feature/something^{commit}')) { throw new Error('missing local branch') diff --git a/src/main/ipc/worktrees-ssh-create-base-prefetch.test.ts b/src/main/ipc/worktrees-ssh-create-base-prefetch.test.ts index a14d7572f03..6b09ce2a743 100644 --- a/src/main/ipc/worktrees-ssh-create-base-prefetch.test.ts +++ b/src/main/ipc/worktrees-ssh-create-base-prefetch.test.ts @@ -2,6 +2,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { getSshGitProviderMock, getActiveMultiplexerMock } from './worktrees-test-module-mocks' import { handlers, setupWorktreeHandlers, store } from './worktrees-test-harness' +function missingShowRefError(): Error & { code: number } { + return Object.assign(new Error('missing exact ref'), { code: 1 }) +} + vi.mock('electron', async () => (await import('./worktrees-test-module-mocks')).electronModuleMock() ) @@ -104,6 +108,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw missingShowRefError() + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), @@ -175,6 +182,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw missingShowRefError() + } if (args[0] === 'for-each-ref') { return { stdout: '', stderr: '' } } @@ -244,6 +254,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw missingShowRefError() + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockReturnValue(pendingFetch), @@ -314,6 +327,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw missingShowRefError() + } if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/master^{commit}')) { throw new Error('missing stale base') } @@ -387,6 +403,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'team\norigin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw missingShowRefError() + } if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/main')) { return { stdout: 'main-sha\n', stderr: '' } } @@ -463,6 +482,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return pendingRemoteList } + if (args[0] === 'show-ref') { + throw missingShowRefError() + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), @@ -529,6 +551,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw missingShowRefError() + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockReturnValue(pendingExactFetch), diff --git a/src/main/ipc/worktrees-ssh-fork-push-target-remote.test.ts b/src/main/ipc/worktrees-ssh-fork-push-target-remote.test.ts index f7d7ff45796..c952a6be9df 100644 --- a/src/main/ipc/worktrees-ssh-fork-push-target-remote.test.ts +++ b/src/main/ipc/worktrees-ssh-fork-push-target-remote.test.ts @@ -110,6 +110,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote' && args.length === 1) { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }) const provider = { @@ -188,6 +191,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote' && args.length === 1) { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), @@ -233,6 +239,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote' && args.length === 1) { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }) const provider = { @@ -296,6 +305,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote' && args.length === 1) { return { stdout: 'origin\npr-contributor-orca\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }) const provider = { diff --git a/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts b/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts index 0b76025a979..c8a4e5088b1 100644 --- a/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts +++ b/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts @@ -105,6 +105,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing remote ref'), { code: 1 }) + } if (args[0] === 'merge-base') { return { stdout: '', stderr: '' } } @@ -202,6 +205,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing remote ref'), { code: 1 }) + } if (args[0] === 'merge-base') { return { stdout: '', stderr: '' } } @@ -306,6 +312,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing remote ref'), { code: 1 }) + } if (args[0] === 'merge-base' || args[0] === 'log') { if (!registeredRoots) { throw new Error('Path outside authorized workspace') @@ -420,6 +429,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing remote ref'), { code: 1 }) + } if (args[0] === 'merge-base') { return { stdout: '', stderr: '' } } @@ -493,11 +505,17 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } - if (args[0] === 'for-each-ref' && args.at(-1) === 'refs/heads/main') { + if (args[0] === 'show-ref' && args.at(-1) === 'refs/heads/main') { if (presence === 'probe-failed') { throw new Error('ssh: connection closed by remote host') } - return { stdout: presence === 'present' ? 'refs/heads/main\n' : '', stderr: '' } + if (presence === 'present') { + return { stdout: '', stderr: '' } + } + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing ref'), { code: 1 }) } if (args[0] === 'rev-parse') { const ref = args.at(-1) ?? '' @@ -552,7 +570,7 @@ describe('registerWorktreeHandlers', () => { })) as CreateWorktreeResult expect(provider.exec).toHaveBeenCalledWith( - ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/heads/main'], + ['show-ref', '--verify', '--quiet', '--', 'refs/heads/main'], '/remote/repo' ) expect(result.localBaseRefRefresh).toBeUndefined() diff --git a/src/main/ipc/worktrees-ssh-pr-head-fetch.test.ts b/src/main/ipc/worktrees-ssh-pr-head-fetch.test.ts index 109dca653d3..8d4966dd187 100644 --- a/src/main/ipc/worktrees-ssh-pr-head-fetch.test.ts +++ b/src/main/ipc/worktrees-ssh-pr-head-fetch.test.ts @@ -388,6 +388,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), @@ -480,6 +483,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), diff --git a/src/main/ipc/worktrees-ssh-setup-launch.test.ts b/src/main/ipc/worktrees-ssh-setup-launch.test.ts index a0735bfd50d..fa98b5b62a1 100644 --- a/src/main/ipc/worktrees-ssh-setup-launch.test.ts +++ b/src/main/ipc/worktrees-ssh-setup-launch.test.ts @@ -121,6 +121,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'rev-parse') { throw new Error('missing local branch') } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), @@ -226,6 +229,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'rev-parse') { throw new Error('missing local branch') } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), @@ -313,6 +319,9 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }), fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 57cb6ef889b..9d93d56dafa 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -31,6 +31,7 @@ import { reviewHeadRemoteRefComponent, REVIEW_HEAD_FETCH_TIMEOUT_MS } from '../../shared/review-head-tracking-ref' +import { REPO_SEARCH_REFS_MAX_LIMIT } from '../../shared/repo-search-limits' // Why: durable review-head refs are scoped by remote identity (name + URL hash). const ORIGIN_REMOTE_URL = 'git@example.com:group/repo.git' @@ -42050,6 +42051,9 @@ describe('OrcaRuntimeService', () => { await expect(runtime.getWorktreePs(-1)).rejects.toThrow('invalid_limit') await expect(runtime.listManagedWorktrees(undefined, 0)).rejects.toThrow('invalid_limit') await expect(runtime.searchRepoRefs('id:repo-1', 'main', -5)).rejects.toThrow('invalid_limit') + await expect(runtime.searchRepoRefs('id:repo-1', 'main', Number.MAX_VALUE)).rejects.toThrow( + 'invalid_limit' + ) }) it('returns capped SSH refs for empty runtime repo searches', async () => { @@ -42097,7 +42101,7 @@ describe('OrcaRuntimeService', () => { }) expect(provider.exec).toHaveBeenCalledWith( expect.arrayContaining([ - '--exclude=refs/remotes/**/HEAD', + '--exclude=refs/remotes/*/HEAD', '--count=12', 'refs/heads/**/**', 'refs/heads/**/**/**', @@ -42109,6 +42113,51 @@ describe('OrcaRuntimeService', () => { expect(provider.exec).toHaveBeenCalledWith(['remote'], '/home/user/repo') }) + it('clamps oversized SSH ref-search limits and reports the execution cap', async () => { + const remoteRepo = { + id: 'remote-repo-large-limit', + path: '/home/user/repo', + displayName: 'remote', + badgeColor: 'blue', + addedAt: 1, + connectionId: 'ssh-large-limit' + } + const runtimeStore = { + ...store, + getRepos: () => [remoteRepo], + getRepo: () => remoteRepo + } + const provider = { + exec: vi.fn().mockImplementation((argv: string[]) => { + if (argv[0] === 'remote') { + return Promise.resolve({ stdout: 'origin\n', stderr: '' }) + } + return Promise.resolve({ + stdout: 'refs/remotes/origin/main\0origin/main', + stderr: '' + }) + }) + } + registerSshGitProvider('ssh-large-limit', provider as never) + const runtime = new OrcaRuntimeService(runtimeStore as never) + + const result = await runtime.searchRepoRefs( + 'id:remote-repo-large-limit', + '', + REPO_SEARCH_REFS_MAX_LIMIT + 1 + ) + + expect(result).toEqual({ + refs: ['origin/main'], + refDetails: [{ refName: 'origin/main', localBranchName: 'main' }], + truncated: true + }) + const forEachRefCall = provider.exec.mock.calls.find( + (call) => (call[0] as string[])[0] === 'for-each-ref' + ) + expect(forEachRefCall?.[0]).toContain('--count=4004') + }) + it('retries runtime SSH ref searches without --exclude for older git hosts', async () => { const remoteRepo = { id: 'remote-repo', @@ -42128,7 +42177,7 @@ describe('OrcaRuntimeService', () => { if (argv[0] === 'remote') { return Promise.resolve({ stdout: 'origin\n', stderr: '' }) } - if (argv.includes('--exclude=refs/remotes/**/HEAD')) { + if (argv.some((arg) => arg.startsWith('--exclude=refs/remotes/'))) { return Promise.reject( Object.assign(new Error("unknown option `exclude'"), { stderr: "error: unknown option `exclude'" @@ -42161,10 +42210,16 @@ describe('OrcaRuntimeService', () => { (call) => (call[0] as string[])[0] === 'for-each-ref' ) expect(forEachRefCalls).toHaveLength(3) - expect(forEachRefCalls[0][0]).toContain('--exclude=refs/remotes/**/HEAD') - expect(forEachRefCalls[1][0]).not.toContain('--exclude=refs/remotes/**/HEAD') + expect( + (forEachRefCalls[0][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(true) + expect( + (forEachRefCalls[1][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(false) expect(forEachRefCalls[1][0]).toContain('--count=108') - expect(forEachRefCalls[2][0]).not.toContain('--exclude=refs/remotes/**/HEAD') + expect( + (forEachRefCalls[2][0] as string[]).some((arg) => arg.startsWith('--exclude=refs/remotes/')) + ).toBe(false) }) it('resolves SSH worktrees when manually updating lineage', async () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 405e87ef4bb..0d5ccf2be6e 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -19,6 +19,12 @@ import { isAgentSkillSharingEnabled } from '../../shared/agent-skill-sharing-gate' import { resolveNestedWorkerMaxDepth } from '../../shared/nested-worker-depth' +import { + clampRepoSearchRefsLimit, + REPO_SEARCH_REFS_DEFAULT_LIMIT, + getRepoSearchRefsProbeLimit, + isRepoSearchRefsRequestLimit +} from '../../shared/repo-search-limits' import { sortDirEntries } from '../../shared/file-name-sort' import { isServerDriveListRequest, listWindowsDrives } from './windows-drive-listing' import { extractLastOsc7Uri, extractOscScanTail } from '../daemon/osc7-uri-extraction' @@ -24267,9 +24273,11 @@ export class OrcaRuntimeService { query: string, limit = DEFAULT_REPO_SEARCH_REFS_LIMIT ): Promise { - if (!Number.isInteger(limit) || limit <= 0) { + if (!isRepoSearchRefsRequestLimit(limit)) { throw new Error('invalid_limit') } + const effectiveLimit = clampRepoSearchRefsLimit(limit) + const probeLimit = getRepoSearchRefsProbeLimit(effectiveLimit) const repo = await this.resolveRepoSelector(repoSelector) if (isFolderRepo(repo)) { return { @@ -24278,12 +24286,15 @@ export class OrcaRuntimeService { } } const refDetails = repo.connectionId - ? await this.searchRemoteRepoRefs(repo, query, limit + 1) - : await searchBaseRefDetails(repo.path, query, limit + 1) + ? await this.searchRemoteRepoRefs(repo, query, probeLimit) + : await searchBaseRefDetails(repo.path, query, probeLimit) return { - refs: refDetails.slice(0, limit).map((entry) => entry.refName), - refDetails: refDetails.slice(0, limit), - truncated: refDetails.length > limit + refs: refDetails.slice(0, effectiveLimit).map((entry) => entry.refName), + refDetails: refDetails.slice(0, effectiveLimit), + // An oversized request is intentionally reported as truncated even when + // this repo has fewer refs: the execution cap prevented fulfilling the + // requested page size. + truncated: limit > effectiveLimit || refDetails.length > effectiveLimit } } @@ -41957,7 +41968,7 @@ const WORKTREE_STATUS_PRIORITY: Record = { working: 3, permission: 4 } -const DEFAULT_REPO_SEARCH_REFS_LIMIT = 25 +const DEFAULT_REPO_SEARCH_REFS_LIMIT = REPO_SEARCH_REFS_DEFAULT_LIMIT const DEFAULT_TERMINAL_LIST_LIMIT = 200 const DEFAULT_WORKTREE_LIST_LIMIT = 200 const DEFAULT_WORKTREE_PS_LIMIT = 200 diff --git a/src/main/runtime/rpc/methods/repo.test.ts b/src/main/runtime/rpc/methods/repo.test.ts index a267b705cb7..a9813823f01 100644 --- a/src/main/runtime/rpc/methods/repo.test.ts +++ b/src/main/runtime/rpc/methods/repo.test.ts @@ -4,12 +4,36 @@ import type { RpcRequest } from '../core' import type { OrcaRuntimeService } from '../../orca-runtime' import { REPO_METHODS } from './repo' import { WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY } from '../../../../shared/protocol-version' +import { REPO_SEARCH_REFS_MAX_LIMIT } from '../../../../shared/repo-search-limits' function makeRequest(method: string, params?: unknown): RpcRequest { return { id: 'req-1', authToken: 'tok', method, params } } describe('repo RPC methods', () => { + it('passes oversized safe ref-search limits to the runtime clamp', async () => { + const runtime = { + getRuntimeId: () => 'test-runtime', + searchRepoRefs: vi.fn().mockResolvedValue({ refs: [], truncated: true }) + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: REPO_METHODS }) + + const response = await dispatcher.dispatch( + makeRequest('repo.searchRefs', { + repo: 'id:repo-1', + query: 'main', + limit: REPO_SEARCH_REFS_MAX_LIMIT + 1 + }) + ) + + expect(response).toMatchObject({ ok: true, result: { refs: [], truncated: true } }) + expect(runtime.searchRepoRefs).toHaveBeenCalledWith( + 'id:repo-1', + 'main', + REPO_SEARCH_REFS_MAX_LIMIT + 1 + ) + }) + it('projects inherited visibility for old clients but preserves inheritance for capable clients', async () => { const runtime = { getRuntimeId: () => 'test-runtime', diff --git a/src/main/runtime/runtime-git-generation-admission.test.ts b/src/main/runtime/runtime-git-generation-admission.test.ts index 2569ae47749..4d93779c7a8 100644 --- a/src/main/runtime/runtime-git-generation-admission.test.ts +++ b/src/main/runtime/runtime-git-generation-admission.test.ts @@ -104,6 +104,7 @@ describe('RuntimeGitGenerationCommands admission', () => { it('marks local pull-request context and linked-issue reads as interactive', async () => { mocks.getPullRequestDraftContext.mockImplementation(async (execute) => { await execute(['fetch', 'origin', 'main'], { timeout: 123 }) + await execute(['show-ref', '--verify'], { maxBuffer: 456, timeoutMs: 789 }) return pullRequestContext }) const commands = makeCommands( @@ -122,6 +123,13 @@ describe('RuntimeGitGenerationCommands admission', () => { timeout: 123, admissionTier: 'interactive' }) + expect(mocks.gitExecFileAsync).toHaveBeenCalledWith(['show-ref', '--verify'], { + cwd: 'C:\\repo', + wslDistro: 'Ubuntu', + maxBuffer: 456, + timeout: 789, + admissionTier: 'interactive' + }) expect(mocks.loadPullRequestLinkedIssue).toHaveBeenCalledWith( expect.objectContaining({ connectionId: undefined, @@ -135,6 +143,7 @@ describe('RuntimeGitGenerationCommands admission', () => { mocks.getSshGitProvider.mockReturnValue({ exec, executeCommitMessagePlan: vi.fn() }) mocks.getPullRequestDraftContext.mockImplementation(async (execute) => { await execute(['log', '--oneline']) + await execute(['show-ref', '--verify'], { maxBuffer: 456, timeoutMs: 789 }) return pullRequestContext }) const commands = makeCommands( @@ -151,6 +160,7 @@ describe('RuntimeGitGenerationCommands admission', () => { ) expect(exec).toHaveBeenCalledWith(['log', '--oneline'], '/remote/repo') + expect(exec).toHaveBeenCalledWith(['show-ref', '--verify'], '/remote/repo', { timeoutMs: 789 }) expect(mocks.gitExecFileAsync).not.toHaveBeenCalled() expect(mocks.loadPullRequestLinkedIssue).toHaveBeenCalledWith( expect.objectContaining({ connectionId: 'conn-1', localGitOptions: {} }) diff --git a/src/main/runtime/runtime-git-generation-commands.ts b/src/main/runtime/runtime-git-generation-commands.ts index 868b97acd89..c741525e0a0 100644 --- a/src/main/runtime/runtime-git-generation-commands.ts +++ b/src/main/runtime/runtime-git-generation-commands.ts @@ -186,18 +186,29 @@ export class RuntimeGitGenerationCommands { useTemplate: input.useTemplate }) context = target.connectionId - ? await getPullRequestDraftContext((argv) => provider!.exec(argv, target.worktree.path), { - base: input.base, - currentTitle: input.title, - currentBody, - currentDraft: input.draft - }) + ? await getPullRequestDraftContext( + (argv, commandOptions) => { + const timeoutMs = commandOptions?.timeoutMs ?? commandOptions?.timeout + return timeoutMs === undefined + ? provider!.exec(argv, target.worktree.path) + : provider!.exec(argv, target.worktree.path, { timeoutMs }) + }, + { + base: input.base, + currentTitle: input.title, + currentBody, + currentDraft: input.draft + } + ) : await getPullRequestDraftContext( (argv, options) => gitExecFileAsync(argv, { cwd: target.worktree.path, ...localGitOptionsForTarget(target), - ...options, + ...(options?.maxBuffer === undefined ? {} : { maxBuffer: options.maxBuffer }), + ...(options?.timeoutMs === undefined && options?.timeout === undefined + ? {} + : { timeout: options?.timeoutMs ?? options?.timeout }), admissionTier: 'interactive' }), { diff --git a/src/main/source-control/hosted-review-base-ref-suffix.test.ts b/src/main/source-control/hosted-review-base-ref-suffix.test.ts new file mode 100644 index 00000000000..a93f7fedd5a --- /dev/null +++ b/src/main/source-control/hosted-review-base-ref-suffix.test.ts @@ -0,0 +1,77 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, getSshGitProviderMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + getSshGitProviderMock: vi.fn() +})) + +vi.mock('../github/gh-utils', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock +})) + +import { baseRefExistsOnRemote } from './hosted-review-creation-git-state' + +describe('baseRefExistsOnRemote suffix fallback', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + getSshGitProviderMock.mockReset() + }) + + it('does not answer a bare base with a nested branch of the same final segment', async () => { + // The replaced `refs/remotes/*/main` could not cross a slash. A repo with + // `origin/feature/main` but no `origin/main` must still read as absent, or + // the review is submitted against a base the provider will reject. + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1, stderr: '' }) + } + if (args[0] === 'show-ref') { + return { stdout: 'abc123 refs/remotes/origin/feature/main\n', stderr: '' } + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(false) + }) + + it('still resolves a stale single-segment remote through the suffix fallback', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1, stderr: '' }) + } + if (args[0] === 'show-ref') { + return { stdout: 'abc123 refs/remotes/orphan/main\n', stderr: '' } + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(true) + }) + + it('treats a suffix lookup that never ran as inconclusive', async () => { + // A transport failure or output overflow must fail open, not assert absence. + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1, stderr: '' }) + } + throw Object.assign(new Error('maxBuffer exceeded'), { + code: 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER' + }) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(true) + }) +}) diff --git a/src/main/source-control/hosted-review-creation-eligibility.test.ts b/src/main/source-control/hosted-review-creation-eligibility.test.ts index 2cf262e8cca..e282a8dc47b 100644 --- a/src/main/source-control/hosted-review-creation-eligibility.test.ts +++ b/src/main/source-control/hosted-review-creation-eligibility.test.ts @@ -116,6 +116,7 @@ vi.mock('./hosted-review', () => ({ })) import { createHostedReview, getHostedReviewCreationEligibility } from './hosted-review-creation' +import { baseRefExistsOnRemote } from './hosted-review-creation-git-state' import { _resetOriginGitHubApiRepositoryCache } from '../github/github-api-repository' @@ -230,6 +231,16 @@ describe('getHostedReviewCreationEligibility', () => { }) it('treats short remote base refs as the default branch name', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => ({ + stdout: + args[0] === 'remote' + ? 'origin\n' + : args[0] === 'show-ref' + ? 'abc refs/remotes/origin/main\n' + : 'Feature title\n', + stderr: '' + })) + await expect( getHostedReviewCreationEligibility({ repoPath: '/repo', @@ -247,6 +258,223 @@ describe('getHostedReviewCreationEligibility', () => { }) }) + it('uses exact remote-ref probes instead of a wildcard ref scan', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\nfork\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/fork/main' + ? { stdout: '', stderr: '' } + : Promise.reject(Object.assign(new Error('missing ref'), { code: 1 })) + } + throw Object.assign(new Error('missing ref'), { code: 1 }) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(true) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).toContainEqual([ + 'show-ref', + '--verify', + '--quiet', + '--', + 'refs/remotes/fork/main' + ]) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['show-ref', '--verify', '--quiet', '--', 'refs/remotes/fork/main'], + expect.objectContaining({ maxBuffer: 10 * 1024 * 1024 }) + ) + }) + + it.each(['origin', 'upstream'])( + 'keeps stale suffix refs discoverable when the conventional remote is configured (%s)', + async (remote) => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: `${remote}\nfork\n`, stderr: '' } + } + if (args[0] === 'show-ref' && args[1] === '--verify') { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + expect(args).toEqual(['show-ref', '--', 'main']) + throw Object.assign(new Error('missing ref'), { code: 1 }) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(false) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).toContainEqual([ + 'show-ref', + '--', + 'main' + ]) + } + ) + + it('keeps qualified stale tracking refs discoverable without a configured remote', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: '', stderr: '' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/orphan/main' + ? { stdout: '', stderr: '' } + : Promise.reject(Object.assign(new Error('missing ref'), { code: 1 })) + } + throw Object.assign(new Error('missing ref'), { code: 1 }) + }) + + await expect(baseRefExistsOnRemote('orphan/main', '/repo')).resolves.toBe(true) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).toContainEqual([ + 'show-ref', + '--verify', + '--quiet', + '--', + 'refs/remotes/orphan/main' + ]) + }) + + it('keeps suffix matching for an absent configured-qualified base bounded', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'orphan\n', stderr: '' } + } + if (args[0] === 'show-ref' && args[1] === '--verify') { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + expect(args).toEqual(['show-ref', '--', 'orphan/main']) + throw Object.assign(new Error('missing ref'), { code: 1 }) + }) + + await expect(baseRefExistsOnRemote('orphan/main', '/repo')).resolves.toBe(false) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).toContainEqual([ + 'show-ref', + '--', + 'orphan/main' + ]) + }) + + it('keeps a nested bare branch discoverable on an unconfigured stale remote', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + expect(args).toEqual(['show-ref', '--', 'feature/fix']) + return { stdout: 'abc123 refs/remotes/orphan/feature/fix\n' } + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('feature/fix', '/repo')).resolves.toBe(true) + }) + + it('finds a unique bare suffix on an unconfigured stale remote', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + expect(args).toEqual(['show-ref', '--', 'main']) + return { stdout: 'abc123 refs/remotes/orphan/main\n', stderr: '' } + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(true) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['show-ref', '--', 'main'], + expect.objectContaining({ maxBuffer: 10 * 1024 * 1024 }) + ) + }) + + it('does not treat a nested branch ending in HEAD as the bare remote HEAD', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + expect(args).toEqual(['show-ref', '--', 'HEAD']) + return { stdout: 'abc123 refs/remotes/fork/feature/HEAD\n', stderr: '' } + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('HEAD', '/repo')).resolves.toBe(false) + }) + + it('keeps a bare suffix present when stale refs are ambiguous across remotes', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + return { + stdout: 'abc123 refs/remotes/orphan-a/main\nabc123 refs/remotes/orphan-b/main\n' + } + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(true) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['show-ref', '--', 'main'], + expect.objectContaining({ maxBuffer: 10 * 1024 * 1024 }) + ) + }) + + it('continues ref probes when listing configured remotes fails', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + throw new Error('remote listing unavailable') + } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(false) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['show-ref', '--', 'main'], + expect.objectContaining({ maxBuffer: 10 * 1024 * 1024 }) + ) + }) + + it.each(['git stdout exceeded maxBuffer', 'ssh: connection refused'])( + 'keeps a bare candidate on suffix fallback failure (%s)', + async (failure) => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n', stderr: '' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + throw new Error(failure) + } + throw new Error(`unexpected git command: ${args.join(' ')}`) + }) + + await expect(baseRefExistsOnRemote('main', '/repo')).resolves.toBe(true) + } + ) + + it('fails closed for malformed base input without invoking Git', async () => { + await expect(baseRefExistsOnRemote('main*', '/repo')).resolves.toBe(false) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + it('reports reviewLookupOutcome: found when an existing review is returned', async () => { getHostedReviewForBranchMock.mockResolvedValue({ number: 7, url: 'https://x/pull/7' }) await expect( @@ -362,11 +590,25 @@ describe('getHostedReviewCreationEligibility', () => { if (args[0] === 'symbolic-ref') { return { stdout: opts.symbolicRef ?? '', stderr: '' } } - if (args[0] === 'for-each-ref') { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref') { if (opts.forEachThrows) { throw new Error('ssh: connect: connection refused') } - return { stdout: opts.forEachRef ?? '', stderr: '' } + const availableRefs = (opts.forEachRef ?? '') + .split(/\r?\n/) + .map((ref) => ref.trim()) + .filter(Boolean) + const matches = availableRefs.filter((ref) => args.includes(ref)) + if (matches.length > 0) { + return { + stdout: `${matches.map((ref) => `abc ${ref}`).join('\n')}\n`, + stderr: '' + } + } + throw Object.assign(new Error('missing ref'), { code: 1 }) } if (args[0] === 'rev-parse' && opts.revParseThrows) { throw new Error('unknown revision') @@ -456,6 +698,16 @@ describe('getHostedReviewCreationEligibility', () => { }) it('enables creation for clean, in-sync, authenticated GitHub feature branches', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => ({ + stdout: + args[0] === 'remote' + ? 'origin\n' + : args[0] === 'show-ref' + ? 'abc refs/remotes/origin/main\n' + : 'Feature title\n', + stderr: '' + })) + await expect( getHostedReviewCreationEligibility({ repoPath: '/repo', @@ -505,7 +757,15 @@ describe('getHostedReviewCreationEligibility', () => { it('resolves remote eligibility through SSH repo metadata without generating PR copy', async () => { const remoteGit = { - exec: vi.fn(async () => ({ stdout: '', stderr: '' })) + exec: vi.fn(async (args: string[]) => ({ + stdout: + args[0] === 'remote' + ? 'origin\n' + : args[0] === 'show-ref' + ? 'abc refs/remotes/origin/main\n' + : '', + stderr: '' + })) } getSshGitProviderMock.mockReturnValue(remoteGit) @@ -533,8 +793,9 @@ describe('getHostedReviewCreationEligibility', () => { ) // Why: the base-on-remote probe must run on the SSH host that will execute // the provider create, so it flows through the relay exec, not local git. + expect(remoteGit.exec).toHaveBeenCalledWith(['remote'], '/remote/repo') expect(remoteGit.exec).toHaveBeenCalledWith( - ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/remotes/*/main'], + ['show-ref', '--verify', '--quiet', '--', 'refs/remotes/origin/main'], '/remote/repo' ) }) diff --git a/src/main/source-control/hosted-review-creation-git-state.ts b/src/main/source-control/hosted-review-creation-git-state.ts index 5c891e97f06..b2457424d2a 100644 --- a/src/main/source-control/hosted-review-creation-git-state.ts +++ b/src/main/source-control/hosted-review-creation-git-state.ts @@ -1,10 +1,13 @@ import { + isRemoteHeadRef, normalizeHostedReviewBaseRef, normalizeHostedReviewHeadRef } from '../../shared/hosted-review-refs' import { isNoUpstreamError, normalizeGitErrorMessage } from '../../shared/git-remote-error' +import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' import type { GitUpstreamStatus } from '../../shared/git-status-types' import { gitExecFileAsync } from '../github/gh-utils' +import { isShowRefNoMatchError, probeAnyExactRef } from '../git/exact-ref-probe' import { gitOptionalLocksDisabledEnv } from '../git/runner' import { parsePorcelainV1Records, type PorcelainV1Record } from '../git/porcelain-v1-records' import { resolveDefaultBaseRefViaExec } from '../git/repo' @@ -27,11 +30,99 @@ export function hostedReviewExecutionContext( return Object.keys(localGitExecOptions).length > 0 ? { localGitExecOptions } : {} } +const MAX_REMOTE_REF_OUTPUT_BYTES = 10 * 1024 * 1024 + +type HostedReviewGitRunOptions = { maxBuffer?: number; timeoutMs?: number } +type HostedReviewGitRun = ( + argv: string[], + options?: HostedReviewGitRunOptions +) => Promise<{ stdout: string }> + +function* iterateGitOutputLines(output: string): Generator { + let lineStart = 0 + for (let index = 0; index < output.length; index++) { + const code = output.charCodeAt(index) + if (code !== 10 && code !== 13) { + continue + } + yield output.slice(lineStart, index) + if (code === 13 && output.charCodeAt(index + 1) === 10) { + index++ + } + lineStart = index + 1 + } + if (lineStart <= output.length) { + yield output.slice(lineStart) + } +} + +function parseSuffixRemoteRefs(output: string, base: string, remotes: readonly string[]): string[] { + const refs = new Set() + for (const line of iterateGitOutputLines(output)) { + const separator = line.indexOf(' ') + if (separator === -1) { + continue + } + const fullRef = line.slice(separator + 1).trim() + if (!fullRef.startsWith('refs/remotes/') || !isSafeGitRefName(fullRef)) { + continue + } + const shortRef = fullRef.slice('refs/remotes/'.length) + // A remote-tracking ref has both a remote and branch component. Ignore a + // malformed bare `refs/remotes/` entry from the suffix stream. + if (!shortRef.includes('/')) { + continue + } + // The replaced query was `refs/remotes/*/`, where `*` cannot cross a + // slash. `show-ref -- ` matches a suffix at any depth, so require the + // remote component to be exactly one segment; otherwise a branch named + // `origin/feature/main` would answer a query for `main`. + const isSingleRemoteSegmentMatch = + shortRef.endsWith(`/${base}`) && shortRef.split('/').length === base.split('/').length + 1 + if ( + isRemoteHeadRef(shortRef, remotes) || + // A bare `HEAD` denotes the remote's symbolic slot, not every branch + // whose final component happens to be `HEAD` (for example `feature/HEAD`). + (base === 'HEAD' && shortRef.endsWith('/HEAD')) || + (shortRef !== base && !isSingleRemoteSegmentMatch) + ) { + continue + } + refs.add(shortRef) + // Two candidates are enough to establish that a suffix is not unique; + // retaining more only spends memory without changing the boolean result. + if (refs.size >= 2) { + break + } + } + return [...refs] +} + +async function listSuffixRemoteBaseRefs( + run: HostedReviewGitRun, + base: string, + remotes: readonly string[] +): Promise<{ refs: string[]; unknown: boolean }> { + try { + // The local runner and SSH relay both cap generic Git stdout at 10 MiB. + const { stdout } = await run(['show-ref', '--', base], { + maxBuffer: MAX_REMOTE_REF_OUTPUT_BYTES + }) + return { refs: parseSuffixRemoteRefs(stdout, base, remotes), unknown: false } + } catch (error) { + // Unlike --verify, a pattern query exits 1 when it simply has no matches. + // Other failures (overflow, a broken repository, or dropped SSH) remain + // inconclusive so callers preserve the submitted candidate (fail-open). + return isShowRefNoMatchError(error) ? { refs: [], unknown: false } : { refs: [], unknown: true } + } +} + async function runGitForHostedReview( repoPath: string, args: string[], connectionId?: string | null, - options: HostedReviewExecutionOptions = {} + options: HostedReviewExecutionOptions = {}, + commandOptions: HostedReviewGitRunOptions = {} ): Promise<{ stdout: string; stderr?: string }> { if (connectionId) { const provider = getSshGitProvider(connectionId) @@ -40,9 +131,16 @@ async function runGitForHostedReview( 'Remote connection dropped. Click Reconnect on the SSH target before retrying.' ) } - return provider.exec(args, repoPath) + return commandOptions.timeoutMs === undefined + ? provider.exec(args, repoPath) + : provider.exec(args, repoPath, { timeoutMs: commandOptions.timeoutMs }) } - return gitExecFileAsync(args, { cwd: repoPath, ...getHostedReviewLocalGitOptions(options) }) + return gitExecFileAsync(args, { + cwd: repoPath, + ...getHostedReviewLocalGitOptions(options), + ...(commandOptions.maxBuffer === undefined ? {} : { maxBuffer: commandOptions.maxBuffer }), + ...(commandOptions.timeoutMs === undefined ? {} : { timeout: commandOptions.timeoutMs }) + }) } export async function getDefaultBaseRef( @@ -71,20 +169,58 @@ export async function baseRefExistsOnRemote( if (!base) { return false } - const run = (argv: string[]): Promise<{ stdout: string }> => - runGitForHostedReview(repoPath, argv, connectionId, options) + const run: HostedReviewGitRun = (argv, commandOptions) => + runGitForHostedReview(repoPath, argv, connectionId, options, commandOptions) - const patterns = [`refs/remotes/*/${base}`] - // `*` does not cross `/`, so a remote-qualified candidate (e.g. `fork/main`) needs its exact tracking ref too. - if (base.includes('/')) { - patterns.push(`refs/remotes/${base}`) + // Validate the complete tracking ref before interpolating user/repo metadata + // into Git arguments. In particular, never let `*`, `?`, or control bytes + // turn this check back into a namespace scan. + if (!isSafeGitRefName(`refs/remotes/${base}`)) { + return false + } + + let configuredRemotes: string[] = [] + try { + const { stdout: remoteOutput } = await run(['remote']) + configuredRemotes = remoteOutput + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + } catch { + // Ref probes can still prove presence or absence without configured names. } try { - // for-each-ref exits 0 on no match: empty means absent, a thrown error means transport failure (preserve the candidate). - const { stdout } = await run(['for-each-ref', '--count=1', '--format=%(refname)', ...patterns]) - return stdout.trim().length > 0 + // Keep the conventional names in the probe set even when a stale tracking + // ref remains after its remote was removed. This also preserves the old + // behavior for the common origin/upstream fork workflow without a wildcard. + const remoteNames = new Set(['origin', 'upstream', ...configuredRemotes]) + const candidateRefs = new Set() + if (base.includes('/')) { + // A qualified candidate (e.g. `fork/main`) is itself a complete tracking + // ref and must remain discoverable even when `fork` is no longer configured. + candidateRefs.add(`refs/remotes/${base}`) + } + for (const remote of remoteNames) { + const ref = `refs/remotes/${remote}/${base}` + if (isSafeGitRefName(ref)) { + candidateRefs.add(ref) + } + } + const exactResult = await probeAnyExactRef(run, [...candidateRefs], { + maxBuffer: MAX_REMOTE_REF_OUTPUT_BYTES + }) + if (exactResult.found || exactResult.unknown) { + return true + } + // The previous wildcard query considered every remote-tracking ref, + // including stale refs left behind after a remote was removed. Keep that + // behavior after the cheap exact probes; the fallback captures at most + // 10 MiB, so it cannot recreate the unbounded metadata allocation. + const suffixResult = await listSuffixRemoteBaseRefs(run, base, configuredRemotes) + return suffixResult.refs.length > 0 || suffixResult.unknown } catch { + // An unexpected ref-probe failure is inconclusive, so preserve the candidate. return true } } diff --git a/src/main/source-control/hosted-review-creation.test.ts b/src/main/source-control/hosted-review-creation.test.ts index 83df9010d65..c2d61a5af4e 100644 --- a/src/main/source-control/hosted-review-creation.test.ts +++ b/src/main/source-control/hosted-review-creation.test.ts @@ -238,10 +238,11 @@ describe('createHostedReview', () => { if (args[0] === 'status') { return { stdout: '', stderr: '' } } - // Why: base-on-remote probe (Change 2 enforcement) — the default base - // resolves to a remote-tracking branch so create-time validation passes. - if (args[0] === 'for-each-ref') { - return { stdout: 'refs/remotes/origin/main\n', stderr: '' } + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return { stdout: 'abc refs/remotes/origin/main\n', stderr: '' } } if (args[0] === 'log' && args.includes('--pretty=%s')) { return { stdout: 'Feature title\n', stderr: '' } @@ -322,12 +323,14 @@ describe('createHostedReview', () => { }) it('blocks creation with actionable copy when the submitted base is local-only', async () => { - // for-each-ref falls through to '' → the submitted stacked parent is not on - // the remote, so create-time enforcement blocks with actionable copy. + // A bulk show-ref probe exits 1 when none of its requested refs exist. gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { if (args[0] === 'rev-parse') { return { stdout: 'feature\n', stderr: '' } } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing ref'), { code: 1 }) + } return { stdout: '', stderr: '' } }) @@ -550,9 +553,11 @@ describe('createHostedReview', () => { if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref' && args[2] === 'HEAD') { return { stdout: 'feature\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - // Base-on-remote probe (Change 2) runs on the SSH host; base is pushed. - return { stdout: 'refs/remotes/origin/main\n', stderr: '' } + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return { stdout: 'abc refs/remotes/origin/main\n', stderr: '' } } if (args[0] === 'log' && args.includes('--pretty=%s')) { return { stdout: 'Feature title\n', stderr: '' } diff --git a/src/main/text-generation/pull-request-context-errors.test.ts b/src/main/text-generation/pull-request-context-errors.test.ts index 38e597fe035..6bdcea3970b 100644 --- a/src/main/text-generation/pull-request-context-errors.test.ts +++ b/src/main/text-generation/pull-request-context-errors.test.ts @@ -27,8 +27,8 @@ describe('getPullRequestDraftContext error handling', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'origin/main\n', stderr: '' } + if (args[0] === 'show-ref') { + return { stdout: '', stderr: '' } } if (args[0] === 'branch') { return { stdout: 'feature\n', stderr: '' } @@ -59,8 +59,8 @@ describe('getPullRequestDraftContext error handling', () => { if (args[0] === 'remote') { return { stdout: 'origin\nstale-fork\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'origin/main\nstale-fork/main\n', stderr: '' } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) } if (args[0] === 'fetch') { if (args[2] !== 'origin') { @@ -84,8 +84,8 @@ describe('getPullRequestDraftContext error handling', () => { if (args[0] === 'remote') { return { stdout: `${'\r\n'.repeat(10_000)}origin\r\n`, stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: `${'\r\n'.repeat(10_000)}origin/main\r\n`, stderr: '' } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) } if (args[0] === 'fetch') { throw new Error( @@ -115,4 +115,148 @@ describe('getPullRequestDraftContext error handling', () => { ) expect(execGit).not.toHaveBeenCalled() }) + + it.each(['origin/feature*', 'origin/feature?', 'origin/feature:other', 'origin/feature\u0000'])( + 'does not turn unsafe characters in a qualified base into a fetch refspec (%s)', + async (base) => { + const execGit = vi.fn() + + await expect(getPullRequestDraftContext(execGit, createContextInput(base))).resolves.toBe( + null + ) + expect(execGit).not.toHaveBeenCalled() + } + ) + + it('ignores stale tracking refs whose remote name could be parsed as a fetch option', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: '--upload-pack=x\n' } + } + if (args[0] === 'show-ref' && args.includes('--verify')) { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + return { stdout: 'abc refs/remotes/--upload-pack=x/main\n' } + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('main') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect(getPullRequestDraftContext(execGit, createContextInput())).resolves.toMatchObject({ + branch: 'feature' + }) + expect(execGit).not.toHaveBeenCalledWith(expect.arrayContaining(['fetch']), expect.any(Object)) + }) + + it('preserves the existing bare HEAD remote-base behavior', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n' } + } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + if (args[0] === 'fetch') { + return { stdout: '' } + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('origin/HEAD') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect( + getPullRequestDraftContext(execGit, createContextInput('HEAD')) + ).resolves.toMatchObject({ base: 'HEAD', branch: 'feature' }) + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'origin', '+refs/heads/HEAD:refs/remotes/origin/HEAD'], + expect.any(Object) + ) + }) + + it('does not resolve bare HEAD to a nested branch ending in HEAD', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + expect(args).toEqual(['show-ref', '--', 'HEAD']) + return { stdout: 'abc123 refs/remotes/fork/feature/HEAD\n' } + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('HEAD') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect( + getPullRequestDraftContext(execGit, createContextInput('HEAD')) + ).resolves.toMatchObject({ base: 'HEAD', branch: 'feature' }) + expect(execGit).not.toHaveBeenCalledWith( + ['fetch', '--no-tags', 'fork', '+refs/heads/feature/HEAD:refs/remotes/fork/feature/HEAD'], + expect.any(Object) + ) + }) + + it('preserves an explicitly qualified remote HEAD ref', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/origin/HEAD' + ? { stdout: '' } + : Promise.reject(Object.assign(new Error('missing exact ref'), { code: 1 })) + } + if (args[0] === 'fetch') { + return { stdout: '' } + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('origin/HEAD') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect( + getPullRequestDraftContext(execGit, createContextInput('origin/HEAD')) + ).resolves.toMatchObject({ base: 'origin/HEAD', branch: 'feature' }) + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'origin', '+refs/heads/HEAD:refs/remotes/origin/HEAD'], + expect.any(Object) + ) + }) }) diff --git a/src/main/text-generation/pull-request-context.test.ts b/src/main/text-generation/pull-request-context.test.ts index d32609d346d..17d9c7c1a15 100644 --- a/src/main/text-generation/pull-request-context.test.ts +++ b/src/main/text-generation/pull-request-context.test.ts @@ -28,8 +28,11 @@ describe('getPullRequestDraftContext', () => { if (args[0] === 'remote') { return { stdout: 'origin\nupstream\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'origin/HEAD\norigin/main\nupstream/main\n', stderr: '' } + if (args[0] === 'show-ref') { + return { + stdout: 'abc refs/remotes/origin/main\n' + 'def refs/remotes/upstream/main\n', + stderr: '' + } } if (args[0] === 'branch') { return { stdout: 'feature/pr-details\n', stderr: '' } @@ -62,6 +65,17 @@ describe('getPullRequestDraftContext', () => { ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], expect.any(Object) ) + expect(execGit).toHaveBeenCalledWith( + ['show-ref', '--verify', '--quiet', '--', 'refs/remotes/origin/main'], + expect.objectContaining({ maxBuffer: 10 * 1024 * 1024 }) + ) + expect(execGit).toHaveBeenCalledWith( + ['show-ref', '--verify', '--quiet', '--', 'refs/remotes/upstream/main'], + expect.objectContaining({ maxBuffer: 10 * 1024 * 1024 }) + ) + const exactRefCalls = execGit.mock.calls.filter(([args]) => args[0] === 'show-ref') + expect(exactRefCalls).toHaveLength(2) + expect(exactRefCalls.every(([, options]) => options?.timeoutMs === undefined)).toBe(true) expect(execGit).not.toHaveBeenCalledWith(expect.arrayContaining(['rebase']), expect.anything()) expect(execGit).not.toHaveBeenCalledWith( expect.arrayContaining(['rev-parse']), @@ -73,7 +87,7 @@ describe('getPullRequestDraftContext', () => { expect(commandNames.indexOf('fetch')).toBeLessThan(commandNames.indexOf('merge-base')) }) - it('fetches the preferred remote base even when the tracking ref is absent locally', async () => { + it('fetches a configured preferred remote without a suffix scan when its ref is absent', async () => { const execGit = vi.fn(async (args) => { if (args[0] === 'fetch') { return { stdout: '', stderr: '' } @@ -81,8 +95,8 @@ describe('getPullRequestDraftContext', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: '', stderr: '' } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) } if (args[0] === 'branch') { return { stdout: 'feature/pr-details\n', stderr: '' } @@ -105,6 +119,7 @@ describe('getPullRequestDraftContext', () => { ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], expect.any(Object) ) + expect(execGit.mock.calls.some(([args]) => args.join(' ') === 'show-ref -- main')).toBe(false) expect(execGit).not.toHaveBeenCalledWith(expect.arrayContaining(['rebase']), expect.anything()) }) @@ -118,11 +133,8 @@ describe('getPullRequestDraftContext', () => { if (args[0] === 'remote') { return { stdout: 'origin\nstale-fork\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { - stdout: 'origin/main\nstale-fork/feature/from-stale-fork\n', - stderr: '' - } + if (args[0] === 'show-ref') { + return { stdout: '', stderr: '' } } if (args[0] === 'branch') { return { stdout: 'feature/pr-details\n', stderr: '' } @@ -158,8 +170,8 @@ describe('getPullRequestDraftContext', () => { if (args[0] === 'remote') { return { stdout: 'contributor-a\ncontributor-b\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'contributor-a/main\ncontributor-b/main\n', stderr: '' } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) } if (args[0] === 'branch') { return { stdout: 'feature\n', stderr: '' } @@ -179,6 +191,21 @@ describe('getPullRequestDraftContext', () => { await getPullRequestDraftContext(execGit, createContextInput()) + expect(execGit.mock.calls.filter(([args]) => args[0] === 'for-each-ref')).toHaveLength(0) + const showRefCalls = execGit.mock.calls.filter(([args]) => args[0] === 'show-ref') + const exactRefs = showRefCalls + .filter(([args]) => args.includes('--verify')) + .map(([args]) => args.at(-1)) + expect(exactRefs).toEqual( + expect.arrayContaining([ + 'refs/remotes/origin/main', + 'refs/remotes/upstream/main', + 'refs/remotes/contributor-a/main', + 'refs/remotes/contributor-b/main' + ]) + ) + expect(showRefCalls.some(([args]) => args.join(' ') === 'show-ref -- main')).toBe(true) + expect(execGit).not.toHaveBeenCalledWith( expect.arrayContaining(['contributor-a']), expect.any(Object) @@ -190,6 +217,256 @@ describe('getPullRequestDraftContext', () => { expect(execGit).not.toHaveBeenCalledWith(expect.arrayContaining(['rebase']), expect.anything()) }) + it('does not choose another remote when an exact probe is inconclusive', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'first\nsecond\n' } + } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('remote probe failed'), { code: 128 }) + } + if (args[0] === 'fetch') { + throw new Error(`Unexpected fetch: ${args.join(' ')}`) + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('main') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect(getPullRequestDraftContext(execGit, createContextInput())).resolves.toMatchObject({ + branch: 'feature' + }) + expect(execGit).not.toHaveBeenCalledWith(expect.arrayContaining(['fetch']), expect.any(Object)) + }) + + it('bounds exact remote-ref probe concurrency for a large remote list', async () => { + const remoteNames = Array.from({ length: 20 }, (_, index) => `remote-${index}`) + let exactProbeCount = 0 + let activeProbes = 0 + let maxActiveProbes = 0 + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: `${remoteNames.join('\n')}\n`, stderr: '' } + } + if (args[0] === 'show-ref') { + if (args.some((arg) => arg.startsWith('refs/remotes/'))) { + exactProbeCount += 1 + activeProbes += 1 + maxActiveProbes = Math.max(maxActiveProbes, activeProbes) + await new Promise((resolve) => setTimeout(resolve, 0)) + activeProbes -= 1 + } + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + if (args[0] === 'branch') { + return { stdout: 'feature\n', stderr: '' } + } + if (args[0] === 'merge-base') { + return { stdout: 'abc123\n', stderr: '' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n', stderr: '' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect(getPullRequestDraftContext(execGit, createContextInput())).resolves.toMatchObject({ + branch: 'feature' + }) + + expect(exactProbeCount).toBe(22) + // Equality, not a ceiling: 22 candidates saturate the pool, so a regression + // to serial probing has to fail here. + expect(maxActiveProbes).toBe(8) + }) + + it('resolves a unique slash-containing remote with an exact probe', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'foo/bar\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/foo/bar/feature/fix' + ? { stdout: '' } + : Promise.reject(Object.assign(new Error('missing exact ref'), { code: 1 })) + } + if (args[0] === 'fetch') { + return { stdout: '', stderr: '' } + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('foo/bar/feature/fix') + return { stdout: 'abc123\n', stderr: '' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n', stderr: '' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect( + getPullRequestDraftContext(execGit, createContextInput('feature/fix')) + ).resolves.toMatchObject({ branchChangedByPreparation: false }) + + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'foo/bar', '+refs/heads/feature/fix:refs/remotes/foo/bar/feature/fix'], + expect.any(Object) + ) + expect(execGit.mock.calls.filter(([args]) => args[0] === 'for-each-ref')).toHaveLength(0) + }) + + it('preserves Git-valid punctuation in configured remote names', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'team@corp\n' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/team@corp/main' + ? { stdout: '' } + : Promise.reject(Object.assign(new Error('missing exact ref'), { code: 1 })) + } + if (args[0] === 'fetch' || args[0] === 'branch') { + return { stdout: '' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('team@corp/main') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await getPullRequestDraftContext(execGit, createContextInput()) + + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'team@corp', '+refs/heads/main:refs/remotes/team@corp/main'], + expect.any(Object) + ) + }) + + it('resolves a unique unconfigured remote suffix with a bounded show-ref fallback', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + return { stdout: 'abc123 refs/remotes/orphan/main\n' } + } + if (args[0] === 'fetch') { + return { stdout: '', stderr: '' } + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('orphan/main') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect(getPullRequestDraftContext(execGit, createContextInput())).resolves.toMatchObject({ + branch: 'feature' + }) + + expect(execGit).toHaveBeenCalledWith(['show-ref', '--', 'main'], expect.any(Object)) + expect(execGit).toHaveBeenCalledWith( + ['show-ref', '--', 'main'], + expect.objectContaining({ maxBuffer: 10 * 1024 * 1024 }) + ) + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'orphan', '+refs/heads/main:refs/remotes/orphan/main'], + expect.any(Object) + ) + }) + + it('does not guess when the suffix fallback finds multiple remotes', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n' } + } + if (args[0] === 'show-ref' && args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + if (args[0] === 'show-ref') { + return { + stdout: 'abc123 refs/remotes/orphan-a/main\nabc123 refs/remotes/orphan-b/main\n' + } + } + if (args[0] === 'fetch') { + throw new Error(`Unexpected fetch: ${args.join(' ')}`) + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('main') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect(getPullRequestDraftContext(execGit, createContextInput())).resolves.toMatchObject({ + branch: 'feature' + }) + expect(execGit).not.toHaveBeenCalledWith(expect.arrayContaining(['fetch']), expect.any(Object)) + }) + + it('treats a suffix fallback overflow or transport error as no match', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n' } + } + if (args[0] === 'show-ref') { + if (args.some((arg) => arg.startsWith('refs/remotes/'))) { + throw Object.assign(new Error('missing exact ref'), { code: 1 }) + } + throw new Error('git stdout exceeded maxBuffer') + } + if (args[0] === 'fetch') { + throw new Error(`Unexpected fetch: ${args.join(' ')}`) + } + if (args[0] === 'branch') { + return { stdout: 'feature\n' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('main') + return { stdout: 'abc123\n' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await expect(getPullRequestDraftContext(execGit, createContextInput())).resolves.toMatchObject({ + branch: 'feature' + }) + expect(execGit).toHaveBeenCalledWith(['show-ref', '--', 'main'], expect.any(Object)) + expect(execGit).not.toHaveBeenCalledWith(expect.arrayContaining(['fetch']), expect.any(Object)) + }) + it('reports no branch change because PR context preparation is read-only', async () => { const execGit = vi.fn(async (args) => { if (args[0] === 'fetch') { @@ -198,8 +475,8 @@ describe('getPullRequestDraftContext', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'origin/main\n', stderr: '' } + if (args[0] === 'show-ref') { + return { stdout: '', stderr: '' } } if (args[0] === 'branch') { return { stdout: 'feature\n', stderr: '' } @@ -234,8 +511,8 @@ describe('getPullRequestDraftContext', () => { if (args[0] === 'remote') { return { stdout: 'origin\nupstream\n', stderr: '' } } - if (args[0] === 'for-each-ref') { - return { stdout: 'origin/main\nupstream/main\n', stderr: '' } + if (args[0] === 'show-ref') { + return { stdout: '', stderr: '' } } if (args[0] === 'branch') { return { stdout: 'feature\n', stderr: '' } @@ -264,4 +541,104 @@ describe('getPullRequestDraftContext', () => { expect.any(Object) ) }) + + it('preserves a legal branch whose final component is HEAD', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/origin/feature/HEAD' + ? { stdout: '', stderr: '' } + : Promise.reject(Object.assign(new Error('missing exact ref'), { code: 1 })) + } + if (args[0] === 'fetch') { + return { stdout: '', stderr: '' } + } + if (args[0] === 'branch') { + return { stdout: 'feature/current\n', stderr: '' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('origin/feature/HEAD') + return { stdout: 'abc123\n', stderr: '' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n', stderr: '' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await getPullRequestDraftContext(execGit, createContextInput('feature/HEAD')) + + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'origin', '+refs/heads/feature/HEAD:refs/remotes/origin/feature/HEAD'], + expect.any(Object) + ) + }) + + it('retains exact-ref precedence when many other remotes share the suffix', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'origin\nfirst\nsecond\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/orphan/main' + ? { stdout: '', stderr: '' } + : Promise.reject(Object.assign(new Error('missing exact ref'), { code: 1 })) + } + if (args[0] === 'fetch' || args[0] === 'branch') { + return { stdout: '', stderr: '' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('orphan/main') + return { stdout: 'abc123\n', stderr: '' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n', stderr: '' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await getPullRequestDraftContext(execGit, createContextInput('orphan/main')) + + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'orphan', '+refs/heads/main:refs/remotes/orphan/main'], + expect.any(Object) + ) + }) + + it('retains preferred origin precedence when its remote is not configured', async () => { + const execGit = vi.fn(async (args) => { + if (args[0] === 'remote') { + return { stdout: 'fork\n', stderr: '' } + } + if (args[0] === 'show-ref') { + return args.at(-1) === 'refs/remotes/origin/main' + ? { stdout: '', stderr: '' } + : Promise.reject(Object.assign(new Error('missing exact ref'), { code: 1 })) + } + if (args[0] === 'fetch' || args[0] === 'branch') { + return { stdout: '', stderr: '' } + } + if (args[0] === 'merge-base') { + expect(args[1]).toBe('origin/main') + return { stdout: 'abc123\n', stderr: '' } + } + if (args[0] === 'log' || args[0] === 'diff') { + return { stdout: 'change\n', stderr: '' } + } + throw new Error(`Unexpected git args: ${args.join(' ')}`) + }) + + await getPullRequestDraftContext(execGit, createContextInput()) + + expect(execGit).toHaveBeenCalledWith( + ['fetch', '--no-tags', 'origin', '+refs/heads/main:refs/remotes/origin/main'], + expect.any(Object) + ) + expect( + execGit.mock.calls.filter(([args]) => args[0] === 'show-ref' && args.includes('--verify')) + ).toHaveLength(3) + expect(execGit.mock.calls.filter(([args]) => args[0] === 'for-each-ref')).toHaveLength(0) + }) }) diff --git a/src/main/text-generation/pull-request-context.ts b/src/main/text-generation/pull-request-context.ts index 24e13265688..2840256075d 100644 --- a/src/main/text-generation/pull-request-context.ts +++ b/src/main/text-generation/pull-request-context.ts @@ -1,10 +1,17 @@ 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 { + canQueryRemoteBaseRefs, + getPullRequestRemoteRefState, + type PullRequestRemoteRefState +} from './pull-request-remote-ref-probes' const MAX_PULL_REQUEST_CONTEXT_BYTES = 10 * 1024 * 1024 type GitExec = ( args: string[], - options?: { maxBuffer?: number } + options?: { maxBuffer?: number; timeout?: number; timeoutMs?: number } ) => Promise<{ stdout: string; stderr?: string }> export type PullRequestContextInput = { @@ -45,48 +52,7 @@ async function requiredExec(execGit: GitExec, args: string[], label: string): Pr } } -type RemoteState = { - remotes: string[] - refs: string[] -} - -type RemoteBranch = { - remote: string - branch: string - ref: string -} - -function splitGitLines(output: string): string[] { - const lines: string[] = [] - for (const rawLine of iterateGitOutputLines(output)) { - const line = rawLine.trim() - if (line.length > 0) { - lines.push(line) - } - } - return lines -} - -function* iterateGitOutputLines(output: string): Generator { - let lineStart = 0 - - for (let index = 0; index < output.length; index++) { - const code = output.charCodeAt(index) - if (code !== 10 && code !== 13) { - continue - } - - yield output.slice(lineStart, index) - if (code === 13 && output.charCodeAt(index + 1) === 10) { - index++ - } - lineStart = index + 1 - } - - if (lineStart <= output.length) { - yield output.slice(lineStart) - } -} +type RemoteBranch = { remote: string; branch: string; ref: string } function* iterateGitOutputLinesFromEnd(output: string): Generator { let lineEnd = output.length @@ -109,17 +75,6 @@ function* iterateGitOutputLinesFromEnd(output: string): Generator { yield output.slice(0, lineEnd) } -async function getRemoteState(execGit: GitExec): Promise { - const [remoteOutput, refOutput] = await Promise.all([ - safeExec(execGit, ['remote']), - safeExec(execGit, ['for-each-ref', '--format=%(refname:short)', 'refs/remotes']) - ]) - return { - remotes: splitGitLines(remoteOutput), - refs: splitGitLines(refOutput).filter((line) => !line.endsWith('/HEAD')) - } -} - function parseRemoteBranch(ref: string, remotes: string[]): RemoteBranch | null { const remote = [...remotes] .sort((a, b) => b.length - a.length) @@ -140,8 +95,12 @@ function parseRemoteRef(ref: string, remotes: string[]): RemoteBranch | null { if (slashIndex <= 0 || slashIndex === ref.length - 1) { return null } + const remote = ref.slice(0, slashIndex) + if (!isSafeReviewHeadFetchRemote(remote)) { + return null + } return { - remote: ref.slice(0, slashIndex), + remote, branch: ref.slice(slashIndex + 1), ref } @@ -149,7 +108,7 @@ function parseRemoteRef(ref: string, remotes: string[]): RemoteBranch | null { function resolveComparisonBase( base: string, - state: RemoteState + state: PullRequestRemoteRefState ): { comparisonBase: string fetchTarget: RemoteBranch | null @@ -170,7 +129,9 @@ function resolveComparisonBase( } } - const matchingRefs = state.refs.filter((ref) => ref.endsWith(`/${base}`)) + const matchingRefs = state.probeUnknown + ? [] + : state.refs.filter((ref) => ref.endsWith(`/${base}`)) if (matchingRefs.length === 1) { const ref = matchingRefs[0] return { comparisonBase: ref, fetchTarget: parseRemoteRef(ref, state.remotes) } @@ -180,7 +141,11 @@ function resolveComparisonBase( } async function fetchComparisonBase(execGit: GitExec, target: RemoteBranch | null): Promise { - if (!target) { + if ( + !target || + !isSafeReviewHeadFetchRemote(target.remote) || + !isSafeGitRefName(`refs/remotes/${target.remote}/${target.branch}`) + ) { return } await requiredExec( @@ -204,7 +169,10 @@ async function preparePullRequestBranch( execGit: GitExec, base: string ): Promise { - const { comparisonBase, fetchTarget } = resolveComparisonBase(base, await getRemoteState(execGit)) + const { comparisonBase, fetchTarget } = resolveComparisonBase( + base, + await getPullRequestRemoteRefState(execGit, base, MAX_PULL_REQUEST_CONTEXT_BYTES) + ) // Why: PR generation only needs the selected base branch. A repo-wide // `fetch --all` makes stale contributor fork remotes block unrelated PRs. await fetchComparisonBase(execGit, fetchTarget) @@ -221,7 +189,7 @@ export async function getPullRequestDraftContext( input: PullRequestContextInput ): Promise { const base = input.base.trim() - if (!base || base.startsWith('-')) { + if (!base || base.startsWith('-') || !canQueryRemoteBaseRefs(base)) { return null } diff --git a/src/main/text-generation/pull-request-remote-ref-probes.ts b/src/main/text-generation/pull-request-remote-ref-probes.ts new file mode 100644 index 00000000000..5ba7d346585 --- /dev/null +++ b/src/main/text-generation/pull-request-remote-ref-probes.ts @@ -0,0 +1,199 @@ +import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' +import { isRemoteHeadRef } from '../../shared/hosted-review-refs' +import { isSafeReviewHeadFetchRemote } from '../../shared/review-head-tracking-ref' +import { probeExactRefs, type ExactRefProbeExecOptions } from '../git/exact-ref-probe' + +export type PullRequestGitExec = ( + args: string[], + options?: { maxBuffer?: number; timeoutMs?: number } +) => Promise<{ stdout: string; stderr?: string }> + +export type PullRequestRemoteRefState = { + remotes: string[] + refs: string[] + probeUnknown: boolean +} + +function* iterateGitOutputLines(output: string): Generator { + let lineStart = 0 + for (let index = 0; index < output.length; index++) { + const code = output.charCodeAt(index) + if (code !== 10 && code !== 13) { + continue + } + yield output.slice(lineStart, index) + if (code === 13 && output.charCodeAt(index + 1) === 10) { + index++ + } + lineStart = index + 1 + } + if (lineStart <= output.length) { + yield output.slice(lineStart) + } +} + +function splitGitLines(output: string): string[] { + const lines: string[] = [] + for (const rawLine of iterateGitOutputLines(output)) { + const line = rawLine.trim() + if (line.length > 0 && isSafeReviewHeadFetchRemote(line)) { + lines.push(line) + } + } + return lines +} + +/** Keep stale tracking refs from turning an unconfigured remote into a fetch option. */ +function hasSafeRemoteComponent(ref: string, remotes: readonly string[]): boolean { + const shortRef = ref.startsWith('refs/remotes/') ? ref.slice('refs/remotes/'.length) : ref + const remote = [...remotes] + .sort((left, right) => right.length - left.length) + .find((candidate) => shortRef.startsWith(`${candidate}/`)) + const fallbackRemote = shortRef.split('/')[0] ?? '' + return isSafeReviewHeadFetchRemote(remote ?? fallbackRemote) +} + +async function safeExec( + execGit: PullRequestGitExec, + args: string[], + options: ExactRefProbeExecOptions +): Promise { + try { + const { stdout } = await execGit(args, options) + return stdout.trim() + } catch { + return '' + } +} + +export function canQueryRemoteBaseRefs(base: string): boolean { + return isSafeGitRefName(`refs/remotes/${base}`) +} + +async function listExactRemoteBaseRefs( + execGit: PullRequestGitExec, + base: string, + remotes: readonly string[], + options: ExactRefProbeExecOptions +): Promise<{ refs: string[]; probeUnknown: boolean }> { + // A base is qualified only when its first components name a configured + // remote. A slash-containing branch such as `feature/fix` still needs the + // conventional and configured-remote candidates below. + const isConfiguredQualifiedBase = + base.includes('/') && remotes.some((remote) => base.startsWith(`${remote}/`)) + // A bare name is a branch suffix, not a complete remote-tracking ref. Only + // probe the complete spelling when the caller supplied a slash-qualified + // candidate; this avoids an extra process for refs/remotes/. + const exactRefNames = new Set([ + ...(base.includes('/') ? [base] : []), + ...(isConfiguredQualifiedBase + ? [] + : [`origin/${base}`, `upstream/${base}`, ...remotes.map((remote) => `${remote}/${base}`)]) + ]) + const refs = [...exactRefNames].filter((ref) => isSafeGitRefName(`refs/remotes/${ref}`)) + const qualifiedRefs = refs.map((ref) => `refs/remotes/${ref}`) + const result = await probeExactRefs(execGit, qualifiedRefs, options) + const present = new Set(result.presentRefs) + return { + refs: refs.filter((ref) => present.has(`refs/remotes/${ref}`)), + probeUnknown: result.unknownRefs.length > 0 + } +} + +function parseSuffixRemoteRefs(output: string, base: string, remotes: readonly string[]): string[] { + const refs = new Set() + for (const line of iterateGitOutputLines(output)) { + const separator = line.indexOf(' ') + if (separator === -1) { + continue + } + const fullRef = line.slice(separator + 1).trim() + if (!fullRef.startsWith('refs/remotes/') || !isSafeGitRefName(fullRef)) { + continue + } + if (!hasSafeRemoteComponent(fullRef, remotes)) { + continue + } + const shortRef = fullRef.slice('refs/remotes/'.length) + // A remote-tracking ref has both a remote and branch component. Ignore a + // malformed bare `refs/remotes/` entry from the suffix stream. + if ( + !shortRef.includes('/') || + isRemoteHeadRef(shortRef, remotes) || + // A bare `HEAD` denotes the remote's symbolic slot, not every branch + // whose final component happens to be `HEAD` (for example `feature/HEAD`). + (base === 'HEAD' && shortRef.endsWith('/HEAD')) || + (shortRef !== base && !shortRef.endsWith(`/${base}`)) + ) { + continue + } + refs.add(shortRef) + // Resolution only distinguishes zero, one, and multiple candidates; cap + // retained state once ambiguity is proven. + if (refs.size >= 2) { + break + } + } + return [...refs] +} + +async function listSuffixRemoteBaseRefs( + execGit: PullRequestGitExec, + base: string, + remotes: readonly string[], + options: ExactRefProbeExecOptions +): Promise { + // show-ref streams; maxBuffer bounds the captured suffix fallback. + try { + const { stdout } = await execGit(['show-ref', '--', base], options) + return parseSuffixRemoteRefs(stdout, base, remotes) + } catch { + // Output overflow or transport failure is an inconclusive suffix lookup; + // retain the bare candidate rather than blocking generation. + return [] + } +} + +function shouldSearchSuffixRemoteRefs( + base: string, + remotes: readonly string[], + refs: readonly string[], + probeUnknown: boolean +): boolean { + if ( + probeUnknown || + refs.length >= 2 || + refs.includes(base) || + remotes.some((remote) => base.startsWith(`${remote}/`)) + ) { + return false + } + return !['origin', 'upstream'].some( + (remote) => remotes.includes(remote) || refs.includes(`${remote}/${base}`) + ) +} + +export async function getPullRequestRemoteRefState( + execGit: PullRequestGitExec, + base: string, + maxBuffer: number +): Promise { + const probeOptions: ExactRefProbeExecOptions = { maxBuffer } + const queryable = canQueryRemoteBaseRefs(base) + const remotes = splitGitLines(await safeExec(execGit, ['remote'], probeOptions)) + const exactResult = queryable + ? await listExactRemoteBaseRefs(execGit, base, remotes, probeOptions) + : { refs: [], probeUnknown: false } + const suffixRefs = + queryable && + shouldSearchSuffixRemoteRefs(base, remotes, exactResult.refs, exactResult.probeUnknown) + ? await listSuffixRemoteBaseRefs(execGit, base, remotes, probeOptions) + : [] + return { + remotes, + refs: [...new Set([...exactResult.refs, ...suffixRefs])].filter( + (ref) => !isRemoteHeadRef(ref, remotes) && hasSafeRemoteComponent(ref, remotes) + ), + probeUnknown: exactResult.probeUnknown + } +} diff --git a/src/relay/git-exec-validator.test.ts b/src/relay/git-exec-validator.test.ts index 1da2d41cdbd..3f1dd0f2413 100644 --- a/src/relay/git-exec-validator.test.ts +++ b/src/relay/git-exec-validator.test.ts @@ -16,6 +16,7 @@ describe('validateGitExecArgs', () => { [['branch', '--list']], [['log', '--oneline', '-10']], [['show-ref', '--heads']], + [['show-ref', '--verify', '--quiet', '--', 'refs/remotes/origin/main']], [['ls-remote', 'origin']], [['remote', '-v']], [['remote', 'get-url', 'origin']], diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index 11fede2ae10..debab394649 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -190,11 +190,51 @@ describeBinaryCompatibility('real Git binary compatibility', () => { isNoWriteFetchHeadUnsupportedError ) await expect(readFile(fetchHeadPath, 'utf-8')).resolves.toBe('sentinel\n') + // Why: ref search ships the excludes built by `getRemoteHeadExcludes` + // (src/main/git/repo-base-ref-search.ts) — a single-component wildcard plus + // an exact exclude per slash-containing remote name. The correctness of + // that split rests on `*` not crossing `/` under wildmatch, which only a + // real binary can prove. + const commitOid = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() + for (const ref of [ + 'refs/remotes/origin/main', + 'refs/remotes/origin/compat-nested/HEAD', + 'refs/remotes/foo/bar/main', + 'refs/remotes/foo/bar/compat-nested/HEAD' + ]) { + await runGit(['update-ref', ref, commitOid]) + } + await runGit(['symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/main']) + await runGit(['symbolic-ref', 'refs/remotes/foo/bar/HEAD', 'refs/remotes/foo/bar/main']) + const exactRemoteHeadExclude = '--exclude=refs/remotes/foo/bar/HEAD' + const shippedExcludeArgv = [ + 'for-each-ref', + '--format=%(refname)', + '--exclude=refs/remotes/*/HEAD', + exactRemoteHeadExclude, + '--count=100', + 'refs/remotes/**' + ] + const wildcardExcludeArgv = shippedExcludeArgv.filter((arg) => arg !== exactRemoteHeadExclude) await expectPreferredOrRecognizedFallback( - ['for-each-ref', '--format=%(refname)', '--exclude=refs/remotes/**/HEAD', '--count=10'], + shippedExcludeArgv, supports(2, 42), isForEachRefExcludeUnsupportedError ) + if (supports(2, 42)) { + const listRefs = async (argv: string[]): Promise => + (await runGit(argv)).stdout.split(/\r?\n/).filter(Boolean) + + expect(await listRefs(shippedExcludeArgv)).toEqual([ + 'refs/remotes/foo/bar/compat-nested/HEAD', + 'refs/remotes/foo/bar/main', + 'refs/remotes/origin/compat-nested/HEAD', + 'refs/remotes/origin/main' + ]) + // The wildcard cannot reach a slash-containing remote's HEAD slot, which + // is the whole reason the exact excludes are emitted alongside it. + expect(await listRefs(wildcardExcludeArgv)).toContain('refs/remotes/foo/bar/HEAD') + } await expect( runGit(['for-each-ref', '--format=%(refname)', '--count=10']) ).resolves.toBeDefined() @@ -216,6 +256,26 @@ describeBinaryCompatibility('real Git binary compatibility', () => { } }) + it('supports exact show-ref probes', async () => { + const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() + const originRef = 'refs/remotes/origin/compat-exact' + const missingRef = 'refs/remotes/missing/compat-exact' + await runGit(['update-ref', originRef, head]) + + await expect( + runGit(['show-ref', '--verify', '--quiet', '--', originRef]) + ).resolves.toBeDefined() + await expect( + runGit(['show-ref', '--verify', '--quiet', '--', missingRef]) + ).rejects.toMatchObject({ code: 1 }) + + const nestedRef = 'refs/remotes/origin/compat-parent/nested' + await runGit(['update-ref', nestedRef, head]) + await expect( + runGit(['show-ref', '--verify', '--quiet', '--', 'refs/remotes/origin/compat-parent']) + ).rejects.toMatchObject({ code: 1 }) + }) + it('fetches hosted review heads into dedicated refs', async () => { const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() await runGit(['update-ref', 'refs/pull/42/head', head]) diff --git a/src/shared/hosted-review-refs.test.ts b/src/shared/hosted-review-refs.test.ts index 63f7c9c8cd9..b4d8b7c493b 100644 --- a/src/shared/hosted-review-refs.test.ts +++ b/src/shared/hosted-review-refs.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from 'vitest' -import { normalizeHostedReviewBaseRef, normalizeHostedReviewHeadRef } from './hosted-review-refs' +import { + isRemoteHeadRef, + normalizeHostedReviewBaseRef, + normalizeHostedReviewHeadRef +} from './hosted-review-refs' describe('hosted review ref normalization', () => { it('normalizes local and remote head refs to branch names', () => { @@ -14,3 +18,24 @@ describe('hosted review ref normalization', () => { expect(normalizeHostedReviewBaseRef('refs/remotes/upstream/release/1.0')).toBe('release/1.0') }) }) + +describe('isRemoteHeadRef', () => { + it('recognizes only a remote symbolic HEAD slot', () => { + expect(isRemoteHeadRef('origin/HEAD', ['origin'])).toBe(true) + expect(isRemoteHeadRef('refs/remotes/origin/HEAD', ['origin'])).toBe(true) + expect(isRemoteHeadRef('origin/feature/HEAD', ['origin'])).toBe(false) + expect(isRemoteHeadRef('refs/remotes/origin/feature/HEAD', ['origin'])).toBe(false) + expect(isRemoteHeadRef('refs/heads/feature/HEAD', ['origin'])).toBe(false) + }) + + it('uses the longest configured remote prefix', () => { + const remotes = ['foo', 'foo/bar'] + expect(isRemoteHeadRef('foo/bar/HEAD', remotes)).toBe(true) + expect(isRemoteHeadRef('foo/bar/feature/HEAD', remotes)).toBe(false) + }) + + it('recognizes the conventional unconfigured remote shape', () => { + expect(isRemoteHeadRef('orphan/HEAD')).toBe(true) + expect(isRemoteHeadRef('orphan/feature/HEAD')).toBe(false) + }) +}) diff --git a/src/shared/hosted-review-refs.ts b/src/shared/hosted-review-refs.ts index 6dc7255fc14..2a738413b94 100644 --- a/src/shared/hosted-review-refs.ts +++ b/src/shared/hosted-review-refs.ts @@ -9,3 +9,17 @@ export function normalizeHostedReviewBaseRef(ref: string): string { const normalized = normalizeHostedReviewHeadRef(ref) return normalized.replace(/^(origin|upstream)\//, '') } + +/** Exclude only a remote's direct symbolic HEAD, preserving branches like feature/HEAD. */ +export function isRemoteHeadRef(ref: string, remotes: readonly string[] = []): boolean { + const shortRef = ref.startsWith('refs/remotes/') ? ref.slice('refs/remotes/'.length) : ref + const remote = [...remotes] + .sort((left, right) => right.length - left.length) + .find((candidate) => shortRef.startsWith(`${candidate}/`)) + if (remote) { + return shortRef.slice(remote.length + 1) === 'HEAD' + } + // A stale ref whose remote is no longer configured is unambiguous only in + // the conventional two-component `/HEAD` shape. + return shortRef.split('/').length === 2 && shortRef.endsWith('/HEAD') +} diff --git a/src/shared/repo-search-limits.test.ts b/src/shared/repo-search-limits.test.ts new file mode 100644 index 00000000000..e0151f54fbc --- /dev/null +++ b/src/shared/repo-search-limits.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from 'vitest' +import { + clampRepoSearchRefsLimit, + clampRepoSearchRefsScanLimit, + getRepoSearchRefsProbeLimit, + isRepoSearchRefsRequestLimit, + isRepoSearchRefsLimit, + isRepoSearchRefsScanLimit, + REPO_SEARCH_REFS_DEFAULT_LIMIT, + REPO_SEARCH_REFS_MAX_LIMIT, + REPO_SEARCH_REFS_MAX_SCAN_LIMIT +} from './repo-search-limits' + +describe('repository ref-search limits', () => { + it('accepts normal UI and CLI limits and adds one bounded probe row', () => { + expect(REPO_SEARCH_REFS_DEFAULT_LIMIT).toBe(25) + expect(isRepoSearchRefsLimit(20)).toBe(true) + expect(isRepoSearchRefsLimit(REPO_SEARCH_REFS_DEFAULT_LIMIT)).toBe(true) + expect(isRepoSearchRefsLimit(600)).toBe(true) + expect(isRepoSearchRefsLimit(REPO_SEARCH_REFS_MAX_LIMIT)).toBe(true) + expect(getRepoSearchRefsProbeLimit(20)).toBe(21) + expect(getRepoSearchRefsProbeLimit(REPO_SEARCH_REFS_MAX_LIMIT)).toBe( + REPO_SEARCH_REFS_MAX_SCAN_LIMIT + ) + expect(isRepoSearchRefsRequestLimit(REPO_SEARCH_REFS_MAX_LIMIT + 1)).toBe(true) + expect(clampRepoSearchRefsLimit(REPO_SEARCH_REFS_MAX_LIMIT + 1)).toBe( + REPO_SEARCH_REFS_MAX_LIMIT + ) + expect(clampRepoSearchRefsScanLimit(Number.MAX_SAFE_INTEGER)).toBe( + REPO_SEARCH_REFS_MAX_SCAN_LIMIT + ) + }) + + it('accepts only the extra max scan row as a scan limit', () => { + expect(isRepoSearchRefsScanLimit(REPO_SEARCH_REFS_MAX_LIMIT)).toBe(true) + expect(isRepoSearchRefsScanLimit(REPO_SEARCH_REFS_MAX_SCAN_LIMIT)).toBe(true) + expect(isRepoSearchRefsLimit(REPO_SEARCH_REFS_MAX_SCAN_LIMIT)).toBe(false) + }) + + it.each([ + 0, + -1, + 1.5, + Number.NaN, + Number.POSITIVE_INFINITY, + Number.MAX_SAFE_INTEGER + 1, + Number.MAX_VALUE + ])('rejects malformed request limit %s', (value) => { + expect(isRepoSearchRefsRequestLimit(value)).toBe(false) + expect(isRepoSearchRefsLimit(value)).toBe(false) + expect(() => getRepoSearchRefsProbeLimit(value as number)).toThrow('invalid_limit') + expect(() => clampRepoSearchRefsLimit(value as number)).toThrow('invalid_limit') + }) + + it('rejects a scan limit that would overflow the candidate multiplier', () => { + expect(isRepoSearchRefsScanLimit(REPO_SEARCH_REFS_MAX_SCAN_LIMIT + 1)).toBe(false) + expect(isRepoSearchRefsScanLimit(Number.MAX_SAFE_INTEGER)).toBe(false) + expect(isRepoSearchRefsScanLimit(Number.MAX_VALUE)).toBe(false) + }) + + it('uses the capped scan sentinel for oversized safe requests without overflowing', () => { + expect(getRepoSearchRefsProbeLimit(REPO_SEARCH_REFS_MAX_LIMIT + 1)).toBe( + REPO_SEARCH_REFS_MAX_SCAN_LIMIT + ) + expect(getRepoSearchRefsProbeLimit(Number.MAX_SAFE_INTEGER)).toBe( + REPO_SEARCH_REFS_MAX_SCAN_LIMIT + ) + }) +}) diff --git a/src/shared/repo-search-limits.ts b/src/shared/repo-search-limits.ts new file mode 100644 index 00000000000..bd85f5ea009 --- /dev/null +++ b/src/shared/repo-search-limits.ts @@ -0,0 +1,45 @@ +/** The branch picker asks for 20; keep the API default aligned with legacy callers. */ +export const REPO_SEARCH_REFS_DEFAULT_LIMIT = 25 + +/** Keep Git's `for-each-ref --count` finite for API and CLI callers. */ +export const REPO_SEARCH_REFS_MAX_LIMIT = 1_000 + +/** One extra row lets runtime callers report whether the page was truncated. */ +export const REPO_SEARCH_REFS_MAX_SCAN_LIMIT = REPO_SEARCH_REFS_MAX_LIMIT + 1 + +/** A positive safe integer is a valid caller request; the execution cap is applied separately. */ +export function isRepoSearchRefsRequestLimit(value: unknown): value is number { + return typeof value === 'number' && Number.isSafeInteger(value) && value > 0 +} + +export function isRepoSearchRefsLimit(value: unknown): value is number { + return isRepoSearchRefsRequestLimit(value) && value <= REPO_SEARCH_REFS_MAX_LIMIT +} + +export function isRepoSearchRefsScanLimit(value: unknown): value is number { + return isRepoSearchRefsRequestLimit(value) && value <= REPO_SEARCH_REFS_MAX_SCAN_LIMIT +} + +/** Clamp a validated request before it reaches a Git command or retained result array. */ +export function clampRepoSearchRefsLimit(limit: number): number { + if (!isRepoSearchRefsRequestLimit(limit)) { + throw new Error('invalid_limit') + } + return Math.min(limit, REPO_SEARCH_REFS_MAX_LIMIT) +} + +/** Clamp an internal probe count, retaining room for the runtime truncation sentinel. */ +export function clampRepoSearchRefsScanLimit(limit: number): number { + if (!isRepoSearchRefsRequestLimit(limit)) { + throw new Error('invalid_limit') + } + return Math.min(limit, REPO_SEARCH_REFS_MAX_SCAN_LIMIT) +} + +export function getRepoSearchRefsProbeLimit(limit: number): number { + if (!isRepoSearchRefsRequestLimit(limit)) { + throw new Error('invalid_limit') + } + // Avoid `limit + 1` at MAX_SAFE_INTEGER; the capped sentinel is all callers need. + return limit >= REPO_SEARCH_REFS_MAX_LIMIT ? REPO_SEARCH_REFS_MAX_SCAN_LIMIT : limit + 1 +}