From b9b9984d73af0b31d0ee69883985cf46ec8a5332 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 26 Jul 2026 21:12:48 -0700 Subject: [PATCH] fix(runtime): select the folder-workspace child repo with a fail-loud index probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getStatus` is built for display, and two of its display affordances make it wrong as a selection predicate: it catches a failed git read and resolves an empty result, and it caps entries at 1000. Under it, "no staged entry" means *not staged*, *unreadable*, or *truncated* — indistinguishable. Selection that cannot tell those apart commits a different child repo and reports success. Replace it for selection with `git diff --cached --quiet`: no entry cap, and a broken index exits 128 rather than looking clean. The abort path keeps reading `conflictOperation` from `getStatus` (its gitdir markers are the only reliable merge-vs-rebase discriminator — REBASE_HEAD survives a *completed* rebase) but now asserts the index is readable first, so an unreadable repo fails closed instead of handing the abort to a different one. Also route commit-message cancellation: generation re-enters by the child's own selector, so its cancel lane is keyed to that child's path. The folder selector resolved to no worktree and threw `selector_not_found`, and every caller is fire-and-forget — Cancel silently did nothing. --- .../folder-workspace-child-repo-probe.ts | 97 +++++++++++++++++ .../folder-workspace-git-routing.test.ts | 102 ++++++++++++++++++ src/main/runtime/orca-runtime-git.ts | 2 +- src/main/runtime/orca-runtime.ts | 76 +++++++++++-- 4 files changed, 266 insertions(+), 11 deletions(-) create mode 100644 src/main/runtime/folder-workspace-child-repo-probe.ts diff --git a/src/main/runtime/folder-workspace-child-repo-probe.ts b/src/main/runtime/folder-workspace-child-repo-probe.ts new file mode 100644 index 00000000000..5cc34fa9bd1 --- /dev/null +++ b/src/main/runtime/folder-workspace-child-repo-probe.ts @@ -0,0 +1,97 @@ +import { gitExecFileAsync } from '../git/runner' +import { + getSshGitProvider, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE +} from '../providers/ssh-git-dispatch' +import { localGitOptionsForTarget, type RuntimeGitTarget } from './orca-runtime-git' + +const PROBE_TIMEOUT_MS = 15_000 + +/** + * Does this child repo have staged changes — and can we even tell? + * + * Why not read this off `getStatus`: it is built for display, and two of its + * display affordances are wrong for selection. It catches a failed status read + * and resolves an empty result, and it caps entries (default 1000, surfaced as + * `didHitLimit`). Under `getStatus`, "no staged entry" means *not staged*, + * *unreadable*, or *truncated* — indistinguishable. Selection that cannot tell + * those apart commits the wrong child repo and reports success (#6357). + * + * `diff --cached --quiet` has neither problem: no entry cap, and a broken index + * exits 128 instead of looking clean. Throwing here is the point — the caller + * fails closed rather than guessing. + */ +export async function probeChildRepoHasStagedChanges(target: RuntimeGitTarget): Promise { + // rc 0 = nothing staged, rc 1 = staged, anything else is a real failure. + // Correct on an unborn HEAD too, unlike `diff --cached HEAD`. + const { code, stderr } = await runChildRepoGit(target, ['diff', '--cached', '--quiet']) + if (code === 0) { + return false + } + if (code === 1) { + return true + } + throw new Error(stderr.trim() || `git diff --cached exited with ${code}`) +} + +/** + * Throw unless this child repo's index is readable. + * + * Why reuse the staged probe for a conflict-state read: the conflict operation + * still comes from `getStatus` (its gitdir-marker check is the only reliable way + * to tell merge from rebase — `REBASE_HEAD` survives a *completed* rebase and + * would report a phantom one). But that read swallows failures the same way, so + * an unreadable repo would present as merely un-conflicted and quietly lose the + * abort to a different repo. This turns that silence back into an error. + */ +export async function assertChildRepoIndexReadable(target: RuntimeGitTarget): Promise { + await probeChildRepoHasStagedChanges(target) +} + +async function runChildRepoGit( + target: RuntimeGitTarget, + args: string[] +): Promise<{ code: number; stderr: string }> { + if (target.connectionId) { + const provider = getSshGitProvider(target.connectionId) + if (!provider) { + throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) + } + const result = await provider.execNonInteractive( + 'git', + args, + target.worktree.path, + PROBE_TIMEOUT_MS + ) + if (result.spawnError) { + throw new Error(result.spawnError) + } + if (result.timedOut) { + throw new Error(`git ${args.join(' ')} timed out after ${PROBE_TIMEOUT_MS}ms`) + } + // Why not default a null exit code to 0: a signal-killed probe would read as + // "nothing staged" — the exact false negative this module exists to prevent. + if (result.exitCode === null) { + throw new Error(result.stderr.trim() || `git ${args.join(' ')} exited without a status`) + } + return { code: result.exitCode, stderr: result.stderr } + } + try { + await gitExecFileAsync(args, { + cwd: target.worktree.path, + ...localGitOptionsForTarget(target) + }) + return { code: 0, stderr: '' } + } catch (error) { + const failure = error as { code?: unknown; stderr?: unknown } + // Why surface the code rather than rethrow: for this probe a non-zero exit is + // an answer, not a fault. A spawn failure carries no numeric code and rethrows. + if (typeof failure.code !== 'number') { + throw error + } + return { + code: failure.code, + stderr: typeof failure.stderr === 'string' ? failure.stderr : '' + } + } +} diff --git a/src/main/runtime/folder-workspace-git-routing.test.ts b/src/main/runtime/folder-workspace-git-routing.test.ts index a39789c2daa..7549a4d7ed7 100644 --- a/src/main/runtime/folder-workspace-git-routing.test.ts +++ b/src/main/runtime/folder-workspace-git-routing.test.ts @@ -472,6 +472,108 @@ describe('#6357 monorepo folder workspace', () => { expect(result).toEqual({ success: false, error: 'No staged changes to summarize.' }) }) + it('K6: a child repo with a corrupt index fails the commit instead of committing around it', async () => { + buildFixture() + const portal = join(CONTAINER, 'fint-portal') + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + await runtime.stageRuntimeGitPath(selector, 'fint_api/src/app.ts') + // Why: `getStatus` catches a failed git read and resolves an EMPTY result, so a + // repo whose index cannot be parsed looks exactly like a clean one. Selecting on + // that reading commits a different repo and reports success — the corrupt repo + // could be the one the user staged. K5 covers an unresolvable target; this covers + // a resolvable target with an unreadable index, which took a different code path. + writeFileSync(join(portal, '.git', 'index'), 'not an index') + const result = await runtime.commitRuntimeGit(selector, 'api only') + expect(result.success).toBe(false) + expect(result.error).toContain('fint-portal-repo') + expect(git(CHILD_API, 'rev-list', '--count', 'HEAD').trim()).toBe('1') + }) + + it('K7: a staged file sorting past the status entry cap is still seen as staged', async () => { + buildFixture() + const portal = join(CONTAINER, 'fint-portal') + git(CHILD_API, 'checkout', '-q', '--', '.') + rmSync(join(CHILD_API, 'src', 'new-file.ts'), { force: true }) + // Why: `getStatus` stops at DEFAULT_GIT_STATUS_LIMIT (1000) entries. Git emits + // porcelain rows in path order, so >1000 merely-modified files sorting ahead of + // the staged one push it past the cap — the staged row never reaches the caller + // and the repo reads as clean. The commit then goes to a different child repo and + // reports success. The probe asks git a yes/no question, which has no cap. + mkdirSync(join(portal, 'bulk'), { recursive: true }) + for (let index = 0; index < 1200; index += 1) { + writeFileSync(join(portal, 'bulk', `aaa-${String(index).padStart(4, '0')}.ts`), 'export {}\n') + } + writeFileSync(join(portal, 'zzz-staged.ts'), 'export const staged = 1\n') + git(portal, 'add', '-A') + git(portal, 'commit', '-qm', 'bulk baseline') + for (let index = 0; index < 1200; index += 1) { + writeFileSync(join(portal, 'bulk', `aaa-${String(index).padStart(4, '0')}.ts`), 'edited\n') + } + writeFileSync(join(portal, 'zzz-staged.ts'), 'export const staged = 2\n') + git(portal, 'add', 'zzz-staged.ts') + // Guard the premise: the staged row really is past the cap, so this test would + // pass for the wrong reason if the fixture ever stopped exceeding it. + const rows = git(portal, 'status', '--porcelain=v2', '--untracked-files=all').split('\n') + expect(rows.findIndex((row) => row.includes('zzz-staged.ts'))).toBeGreaterThan(1000) + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + await expect(runtime.commitRuntimeGit(selector, 'bulk portal')).resolves.toEqual({ + success: true + }) + expect(git(portal, 'log', '-1', '--pretty=%s').trim()).toBe('bulk portal') + expect(git(CHILD_API, 'rev-list', '--count', 'HEAD').trim()).toBe('1') + }) + + it('L5: an unreadable child repo fails the abort instead of aborting a different one', async () => { + buildFixture() + const portal = join(CONTAINER, 'fint-portal') + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + git(CHILD_API, 'checkout', '-q', '--', '.') + git(CHILD_API, 'checkout', '-q', '-b', 'other') + writeFileSync(join(CHILD_API, 'src', 'app.ts'), 'other branch\n') + git(CHILD_API, 'commit', '-qam', 'other') + git(CHILD_API, 'checkout', '-q', 'master') + writeFileSync(join(CHILD_API, 'src', 'app.ts'), 'master branch\n') + git(CHILD_API, 'commit', '-qam', 'master') + expect(() => git(CHILD_API, 'merge', 'other')).toThrow() + // Why: an unreadable repo reads as merely un-conflicted, so the abort would go to + // the one repo we CAN read — which may not be the one the user is looking at. + writeFileSync(join(portal, '.git', 'index'), 'not an index') + await expect(runtime.abortRuntimeGitMerge(selector)).rejects.toThrow('fint-portal-repo') + expect(await runtime.getRuntimeGitConflictOperation(`path:${CHILD_API}`)).toBe('merge') + }) + + it('M3: cancelling generation reaches the child repo instead of dying on the folder selector', async () => { + buildFixture() + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + await runtime.stageRuntimeGitPath(selector, 'fint_api/src/app.ts') + // Why: generation re-enters by the child's selector, so its cancel lane is keyed + // to the child path. Passing the folder selector straight through threw + // `selector_not_found`, and every caller is fire-and-forget — so Cancel silently + // did nothing and the draft kept running. + const inner = vi + .spyOn( + runtime['gitCommands'] as { + cancelRuntimeGenerateCommitMessage: (...args: never[]) => unknown + }, + 'cancelRuntimeGenerateCommitMessage' + ) + .mockResolvedValue({ ok: true } as never) + await expect(runtime.cancelRuntimeGenerateCommitMessage(selector)).resolves.toEqual({ + ok: true + }) + const routed = inner.mock.calls.map((call) => String(call[0])) + expect(routed).not.toContain(selector) + expect(routed.length).toBeGreaterThan(0) + // Why: the lane the generation actually started under must be among them. + const staged = await runtime.getRuntimeGitStatus(routed[0] as string) + expect(staged.entries.length).toBeGreaterThan(0) + inner.mockRestore() + }) + it('E: the real child git worktree resolves fine when its repo is registered', async () => { buildFixture() const runtime = new OrcaRuntimeService(makeStore() as never) diff --git a/src/main/runtime/orca-runtime-git.ts b/src/main/runtime/orca-runtime-git.ts index 22b513a4326..7ea2b1b76ca 100644 --- a/src/main/runtime/orca-runtime-git.ts +++ b/src/main/runtime/orca-runtime-git.ts @@ -135,7 +135,7 @@ export type RuntimeGitTarget = { rebasedRelativePath?: string } -function localGitOptionsForTarget(target: RuntimeGitTarget): GitRuntimeOptions { +export function localGitOptionsForTarget(target: RuntimeGitTarget): GitRuntimeOptions { return target.connectionId ? {} : (target.localGitOptions ?? {}) } diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 37a8a3a6043..de1b093cc8c 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -458,6 +458,10 @@ import { import { RuntimeEmulatorCommands, setEmulatorBridge } from './orca-runtime-emulator' import type { EmulatorBridge } from '../emulator/emulator-bridge' import { RuntimeFileCommands } from './orca-runtime-files' +import { + assertChildRepoIndexReadable, + probeChildRepoHasStagedChanges +} from './folder-workspace-child-repo-probe' import { abortRuntimeGitForTarget, commitRuntimeGitForTarget, @@ -7162,7 +7166,9 @@ export class OrcaRuntimeService { discoverRuntimeCommitMessageModels: RuntimeGitCommands['discoverRuntimeCommitMessageModels'] = this.gitCommands.discoverRuntimeCommitMessageModels.bind(this.gitCommands) cancelRuntimeGenerateCommitMessage: RuntimeGitCommands['cancelRuntimeGenerateCommitMessage'] = - this.gitCommands.cancelRuntimeGenerateCommitMessage.bind(this.gitCommands) + async (selector) => + (await this.cancelFolderWorkspaceGenerateCommitMessage(selector)) ?? + (await this.gitCommands.cancelRuntimeGenerateCommitMessage(selector)) generateRuntimePullRequestFields: RuntimeGitCommands['generateRuntimePullRequestFields'] = this.gitCommands.generateRuntimePullRequestFields.bind(this.gitCommands) cancelRuntimeGeneratePullRequestFields: RuntimeGitCommands['cancelRuntimeGeneratePullRequestFields'] = @@ -24235,7 +24241,7 @@ export class OrcaRuntimeService { */ private async inspectFolderWorkspaceChildRepos( folderPath: string, - isSubject: (status: GitStatusResult) => boolean + isSubject: (target: RuntimeGitTarget) => Promise ): Promise[]> { const childRepos = listFolderWorkspaceChildRepos(this.requireStore().getRepos(), folderPath) return Promise.all( @@ -24246,8 +24252,7 @@ export class OrcaRuntimeService { return { repoName, error: 'no resolvable git worktree' } } try { - const status = await getRuntimeGitStatusForTarget(target) - return { target, repoName, selected: isSubject(status) } + return { target, repoName, selected: await isSubject(target) } } catch (error) { return { target, @@ -24272,8 +24277,9 @@ export class OrcaRuntimeService { return null } return selectFolderWorkspaceCommitTarget( - await this.inspectFolderWorkspaceChildRepos(folderWorkspace.folderPath, (status) => - status.entries.some((entry) => entry.area === 'staged') + await this.inspectFolderWorkspaceChildRepos( + folderWorkspace.folderPath, + probeChildRepoHasStagedChanges ) ) } @@ -24339,6 +24345,52 @@ export class OrcaRuntimeService { ) } + /** + * Cancel a commit-message generation started under a folder-workspace selector. + * The generation re-entered by the child's own selector, so its cancel lane is + * keyed to that child's path — the folder selector resolves to no worktree and + * would just throw `selector_not_found`, leaving Cancel silently dead. + * + * Why cancel every child rather than re-select the staged one: selection is a + * fresh read, and staging can change between generate and cancel, so it can + * name a different repo than the one actually generating. Both cancel paths are + * cwd-keyed best-effort no-ops when nothing is in flight, so the broadest + * correct move is to signal every lane this selector could have started. + * Returns null when the selector is not a folder workspace. + */ + private async cancelFolderWorkspaceGenerateCommitMessage( + selector: string + ): Promise<{ ok: true } | null> { + const scope = await this.resolveFolderWorkspaceLaunchScope(selector) + const folderWorkspace = scope?.folderWorkspace + if (!folderWorkspace) { + return null + } + const childRepos = listFolderWorkspaceChildRepos( + this.requireStore().getRepos(), + folderWorkspace.folderPath + ) + await Promise.all( + childRepos.map(async (repo) => { + const target = await this.resolveChildRepoGitTarget(repo) + if (!target) { + return + } + // Why swallow: a child that cannot be reached has nothing in flight to + // cancel, and one unreachable repo must not strand the others. + await this.gitCommands + .cancelRuntimeGenerateCommitMessage(`id:${target.worktree.id}`) + .catch((error: unknown) => { + console.warn('[runtime] folder-workspace commit-message cancel failed', { + repo: repo.path, + error + }) + }) + }) + ) + return { ok: true } + } + /** * Abort an in-progress merge/rebase for a folder-workspace selector. Like commit * this carries no path, so it resolves the one child repo actually in that @@ -24356,10 +24408,14 @@ export class OrcaRuntimeService { return null } const selected = selectFolderWorkspaceAbortTarget( - await this.inspectFolderWorkspaceChildRepos( - folderWorkspace.folderPath, - (status) => status.conflictOperation === operation - ), + await this.inspectFolderWorkspaceChildRepos(folderWorkspace.folderPath, async (target) => { + // Why both reads: only `getStatus` distinguishes merge from rebase (its + // gitdir markers), but it reports an unreadable repo as un-conflicted. + // The probe throws on that, so an unreadable repo fails closed here + // instead of handing the abort to a different repo. + await assertChildRepoIndexReadable(target) + return (await getRuntimeGitStatusForTarget(target)).conflictOperation === operation + }), operation ) if (!selected.ok) {