From 7bd1f59edde95f87bfb23303f9dfe8d40e5a582a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 26 Jul 2026 20:32:40 -0700 Subject: [PATCH] fix(runtime): route folder-workspace commit and abort to one child repo instead of fanning out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git has no cross-repo transaction. Fanning `git.commit` out across every staged child repo could commit repos 1..k and fail on k+1, and neither {success,error} nor {ok:true} can describe that half-done state. Three false-success paths came out of it: - an unresolvable or unreadable child was dropped to null and never reached the summarizer, so another repo committed and the RPC returned success while the user's staged files were never attempted - a failed abort was logged and the method still returned {ok:true}, so the user saw success on a still-conflicted repo - merged status reported the first conflicted child's operation, offering one abort button that silently meant whichever repo came first Commit and abort now select exactly one child — by staged index and by live conflict state respectively — and fail before mutating anything on 0, >1, or any repo that could not be inspected. generateRuntimeCommitMessage routes the same way; it previously threw selector_not_found for every folder workspace. Also fixes deepest-first child ordering: it sorted on raw path length, so \\wsl$\Ubuntu\work\repo\nested (30 chars) lost to its own ancestor \\wsl.localhost\Ubuntu\work\repo (32) and the nested repo's files routed to the ancestor. Sort on the normalized path instead. Co-authored-by: Orca --- .../folder-workspace-child-repos.test.ts | 39 +++- .../runtime/folder-workspace-child-repos.ts | 45 ++-- ...er-workspace-git-operation-routing.test.ts | 109 +++++++++ .../folder-workspace-git-operation-routing.ts | 81 +++++++ .../folder-workspace-git-routing.test.ts | 151 +++++++++++-- src/main/runtime/orca-runtime-git.ts | 2 +- src/main/runtime/orca-runtime.ts | 211 ++++++++++-------- 7 files changed, 492 insertions(+), 146 deletions(-) create mode 100644 src/main/runtime/folder-workspace-git-operation-routing.test.ts create mode 100644 src/main/runtime/folder-workspace-git-operation-routing.ts diff --git a/src/main/runtime/folder-workspace-child-repos.test.ts b/src/main/runtime/folder-workspace-child-repos.test.ts index 9248e4ca423..e6c2e0633e7 100644 --- a/src/main/runtime/folder-workspace-child-repos.test.ts +++ b/src/main/runtime/folder-workspace-child-repos.test.ts @@ -32,6 +32,33 @@ describe('listFolderWorkspaceChildRepos', () => { expect(listFolderWorkspaceChildRepos([repo('self', FOLDER)], FOLDER)).toEqual([]) }) + it('orders deepest-first by normalized path, not raw string length', () => { + // Why: `\\wsl$\...\repo\nested` (30 chars) is shorter than its own ancestor + // `\\wsl.localhost\...\repo` (32) even though it is one level deeper. Sorting + // on raw length puts the ancestor first and routes the nested repo's files + // to it. Both spellings normalize to the same //wsl/ubuntu root. + const wslFolder = '\\\\wsl.localhost\\Ubuntu\\work' + const ancestor = repo('ancestor', '\\\\wsl.localhost\\Ubuntu\\work\\repo') + const nested = repo('nested-wsl', '\\\\wsl$\\Ubuntu\\work\\repo\\nested') + expect(ancestor.path.length).toBeGreaterThan(nested.path.length) + expect( + listFolderWorkspaceChildRepos([ancestor, nested], wslFolder).map((entry) => entry.id) + ).toEqual(['nested-wsl', 'ancestor']) + }) + + it('routes a file to the nested repo when the two spell the WSL host differently', () => { + const wslFolder = '\\\\wsl.localhost\\Ubuntu\\work' + const ancestor = repo('ancestor', '\\\\wsl.localhost\\Ubuntu\\work\\repo') + const nested = repo('nested-wsl', '\\\\wsl$\\Ubuntu\\work\\repo\\nested') + const match = matchFolderWorkspaceChildRepo( + [ancestor, nested], + wslFolder, + 'repo/nested/src/app.ts' + ) + expect(match?.repo.id).toBe('nested-wsl') + expect(match?.rebasedRelativePath).toBe('src/app.ts') + }) + it('keeps one entry when the same directory is registered twice', () => { // Why: a duplicate registration would list every file twice in the merged // status and run the commit against that repo twice. @@ -126,7 +153,7 @@ describe('mergeFolderWorkspaceGitStatus', () => { expect(merged.upstreamStatus).toBeUndefined() }) - it('surfaces a conflict from any child repo', () => { + it('surfaces a conflict when exactly one child repo has one', () => { const merged = mergeFolderWorkspaceGitStatus(FOLDER, [ { repo: API, status: status([]) }, { repo: PORTAL, status: { entries: [], conflictOperation: 'rebase' } } @@ -134,6 +161,16 @@ describe('mergeFolderWorkspaceGitStatus', () => { expect(merged.conflictOperation).toBe('rebase') }) + it('reports unknown when two child repos are conflicted', () => { + // Why: "rebase" here would offer one abort button for two repos, and it would + // silently mean whichever repo happened to come first. + const merged = mergeFolderWorkspaceGitStatus(FOLDER, [ + { repo: API, status: { entries: [], conflictOperation: 'merge' } }, + { repo: PORTAL, status: { entries: [], conflictOperation: 'rebase' } } + ]) + expect(merged.conflictOperation).toBe('unknown') + }) + it('propagates a truncation flag so the UI still warns', () => { const merged = mergeFolderWorkspaceGitStatus(FOLDER, [ { repo: API, status: { ...status([]), didHitLimit: true, statusLength: 9000 } }, diff --git a/src/main/runtime/folder-workspace-child-repos.ts b/src/main/runtime/folder-workspace-child-repos.ts index 770e3b72928..0f3a4817e2a 100644 --- a/src/main/runtime/folder-workspace-child-repos.ts +++ b/src/main/runtime/folder-workspace-child-repos.ts @@ -18,7 +18,15 @@ export function listFolderWorkspaceChildRepos(repos: readonly Repo[], folderPath (candidate) => !isFolderRepo(candidate) && isPathInsideOrEqual(folderPath, candidate.path) ) .filter((candidate) => relativePathInsideRoot(folderPath, candidate.path) !== '') - .sort((left, right) => right.path.length - left.path.length) + // Why: deepest first, measured on the normalized path. Raw length inverts when + // two repos spell the same host differently — `\\wsl$\Ubuntu\repo\nested` (30) + // is shorter than its own ancestor `\\wsl.localhost\Ubuntu\repo` (32), which + // would route the nested repo's files to the ancestor. + .sort( + (left, right) => + normalizeRuntimePathForComparison(right.path).length - + normalizeRuntimePathForComparison(left.path).length + ) // Why: the same directory can be registered twice (imported once directly and // once by a folder scan). Without this the merged status lists every file twice // and a commit runs against the repo twice. @@ -71,31 +79,6 @@ export function matchFolderWorkspaceChildRepo( return null } -/** - * Combine per-child-repo results for an op that has no path to route on. Every - * child either succeeded or reports why it did not, so a partial outcome is - * described rather than reported as one blanket success or failure. - */ -export function summarizeFolderWorkspaceFanOut( - results: readonly { repoName: string; error?: string }[] -): { success: boolean; error?: string } { - const failures = results.filter((result) => result.error !== undefined) - if (failures.length === 0) { - return { success: true } - } - // Why: naming the repo matters most when only some of them failed — the user - // has to know which ones still need attention. - const detail = failures.map((failure) => `${failure.repoName}: ${failure.error}`).join('; ') - if (failures.length === results.length) { - return { success: false, error: detail } - } - const succeeded = results.length - failures.length - return { - success: false, - error: `Committed ${succeeded} of ${results.length} repos. Failed — ${detail}` - } -} - /** Prefix a child repo's status entry path so it stays addressable from the workspace root. */ export function prefixFolderWorkspaceEntryPath( folderPath: string, @@ -136,13 +119,15 @@ export function mergeFolderWorkspaceGitStatus( didHitLimit ||= status.didHitLimit === true statusLength += status.statusLength ?? status.entries.length } + // Why: no single HEAD/branch/upstream describes N repos, so those stay unset + // rather than reporting one child's state as the whole workspace's. The same + // applies to a conflict: two repos mid-merge have no one answer, and picking + // the first would offer an abort button that silently means "abort that one". + const conflicted = perRepo.filter(({ status }) => status.conflictOperation !== 'unknown') return { entries, - // Why: no single HEAD/branch/upstream describes N repos, so those stay unset - // rather than reporting one child's state as the whole workspace's. conflictOperation: - perRepo.find(({ status }) => status.conflictOperation !== 'unknown')?.status - .conflictOperation ?? 'unknown', + conflicted.length === 1 ? conflicted[0]!.status.conflictOperation : 'unknown', ...(ignoredPaths.length > 0 ? { ignoredPaths } : {}), ...(didHitLimit ? { didHitLimit: true } : {}), statusLength diff --git a/src/main/runtime/folder-workspace-git-operation-routing.test.ts b/src/main/runtime/folder-workspace-git-operation-routing.test.ts new file mode 100644 index 00000000000..4898c2778cf --- /dev/null +++ b/src/main/runtime/folder-workspace-git-operation-routing.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it } from 'vitest' +import { + selectFolderWorkspaceAbortTarget, + selectFolderWorkspaceCommitTarget, + type FolderWorkspaceChildCandidate +} from './folder-workspace-git-operation-routing' + +function readable(repoName: string, selected: boolean): FolderWorkspaceChildCandidate { + return { repoName, target: repoName, selected } +} + +function unreadable(repoName: string, error: string): FolderWorkspaceChildCandidate { + return { repoName, error } +} + +describe('selectFolderWorkspaceCommitTarget', () => { + it('picks the single staged repo', () => { + expect( + selectFolderWorkspaceCommitTarget([readable('api', true), readable('portal', false)]) + ).toEqual({ ok: true, target: 'api' }) + }) + + it('reports "nothing to commit" when no repo is staged, matching the single-repo path', () => { + expect( + selectFolderWorkspaceCommitTarget([readable('api', false), readable('portal', false)]) + ).toEqual({ ok: false, error: 'nothing to commit' }) + }) + + it('refuses to commit when two repos are staged, naming both', () => { + // Why: git has no cross-repo transaction — repo 1 committing then repo 2 + // failing leaves a half-done state no result shape can describe. + const result = selectFolderWorkspaceCommitTarget([ + readable('api', true), + readable('portal', true) + ]) + expect(result.ok).toBe(false) + expect(result.ok === false && result.error).toContain('api') + expect(result.ok === false && result.error).toContain('portal') + }) + + it('fails closed on an unreadable repo even when exactly one other is staged', () => { + // Why: the unreadable repo may be the one the user staged. Committing the + // readable one and reporting success silently skips their work. + const result = selectFolderWorkspaceCommitTarget([ + readable('api', true), + unreadable('portal', 'not a git repository') + ]) + expect(result.ok).toBe(false) + expect(result.ok === false && result.error).toContain('portal: not a git repository') + }) + + it('fails closed on an unreadable repo before reporting "nothing to commit"', () => { + const result = selectFolderWorkspaceCommitTarget([ + readable('api', false), + unreadable('portal', 'permission denied') + ]) + expect(result.ok === false && result.error).toContain('permission denied') + }) + + it('reports every unreadable repo, not just the first', () => { + const result = selectFolderWorkspaceCommitTarget([ + unreadable('api', 'boom'), + unreadable('portal', 'bang') + ]) + expect(result.ok === false && result.error).toContain('api: boom') + expect(result.ok === false && result.error).toContain('portal: bang') + }) + + it('reports "nothing to commit" for an empty workspace rather than throwing', () => { + expect(selectFolderWorkspaceCommitTarget([])).toEqual({ + ok: false, + error: 'nothing to commit' + }) + }) +}) + +describe('selectFolderWorkspaceAbortTarget', () => { + it('picks the single repo in the operation', () => { + expect( + selectFolderWorkspaceAbortTarget([readable('api', true), readable('portal', false)], 'merge') + ).toEqual({ ok: true, target: 'api' }) + }) + + it('names the operation when nothing is in progress', () => { + const result = selectFolderWorkspaceAbortTarget([readable('api', false)], 'rebase') + expect(result).toEqual({ + ok: false, + error: 'No repository in this workspace has a rebase in progress.' + }) + }) + + it('refuses when two repos are mid-operation rather than aborting an arbitrary one', () => { + const result = selectFolderWorkspaceAbortTarget( + [readable('api', true), readable('portal', true)], + 'merge' + ) + expect(result.ok).toBe(false) + expect(result.ok === false && result.error).toContain('More than one repository is mid-merge') + }) + + it('fails closed on an unreadable repo', () => { + const result = selectFolderWorkspaceAbortTarget( + [readable('api', true), unreadable('portal', 'gone')], + 'merge' + ) + expect(result.ok).toBe(false) + expect(result.ok === false && result.error).toContain('portal: gone') + }) +}) diff --git a/src/main/runtime/folder-workspace-git-operation-routing.ts b/src/main/runtime/folder-workspace-git-operation-routing.ts new file mode 100644 index 00000000000..1768b0f37c1 --- /dev/null +++ b/src/main/runtime/folder-workspace-git-operation-routing.ts @@ -0,0 +1,81 @@ +/** + * A folder workspace holds N independent git repos. Operations that carry no path + * — commit, merge/rebase abort — have nothing to route on, so the owning repo has + * to be inferred from repository state instead. + * + * Git has no cross-repo transaction: fanning one click out to N repos can leave + * repos 1..k committed when k+1 fails, and neither the `{ success, error? }` nor + * the `{ ok: true }` result contract can describe that. So these pick exactly one + * target and reject ambiguity *before* anything is mutated. + */ +export type FolderWorkspaceOperationTarget = + | { ok: true; target: T } + | { ok: false; error: string } + +export type FolderWorkspaceChildCandidate = + | { repoName: string; target: T; selected: boolean } + | { repoName: string; error: string } + +type UninspectableChild = { repoName: string; error: string } + +function isUninspectable( + candidate: FolderWorkspaceChildCandidate +): candidate is UninspectableChild { + return 'error' in candidate +} + +function selectExactlyOne( + candidates: readonly FolderWorkspaceChildCandidate[], + messages: { none: string; many: (repoNames: string[]) => string } +): FolderWorkspaceOperationTarget { + const unreadable = candidates.filter(isUninspectable) + if (unreadable.length > 0) { + // Why: fail closed. An unreadable repo could be the one holding the staged + // changes or the active merge; acting on the others would silently skip it + // and report success for work that never happened. + return { + ok: false, + error: `Could not inspect every repository in this workspace: ${unreadable + .map((candidate) => `${candidate.repoName}: ${candidate.error}`) + .join('; ')}` + } + } + const selected = candidates.filter( + (candidate): candidate is { repoName: string; target: T; selected: boolean } => + !isUninspectable(candidate) && candidate.selected + ) + if (selected.length === 0) { + return { ok: false, error: messages.none } + } + if (selected.length > 1) { + return { ok: false, error: messages.many(selected.map((candidate) => candidate.repoName)) } + } + return { ok: true, target: selected[0]!.target } +} + +/** Pick the sole child repo with staged changes. */ +export function selectFolderWorkspaceCommitTarget( + candidates: readonly FolderWorkspaceChildCandidate[] +): FolderWorkspaceOperationTarget { + return selectExactlyOne(candidates, { + // Why: matches what a single-repo commit reports for an empty index, so the + // renderer's existing failure copy stays accurate. + none: 'nothing to commit', + many: (repoNames) => + `Staged changes span multiple repositories (${repoNames.join(', ')}). ` + + 'Commit one repository at a time.' + }) +} + +/** Pick the sole child repo currently running `operation`. */ +export function selectFolderWorkspaceAbortTarget( + candidates: readonly FolderWorkspaceChildCandidate[], + operation: 'merge' | 'rebase' +): FolderWorkspaceOperationTarget { + return selectExactlyOne(candidates, { + none: `No repository in this workspace has a ${operation} in progress.`, + many: (repoNames) => + `More than one repository is mid-${operation} (${repoNames.join(', ')}). ` + + `Abort one repository at a time.` + }) +} diff --git a/src/main/runtime/folder-workspace-git-routing.test.ts b/src/main/runtime/folder-workspace-git-routing.test.ts index a0767c7e09a..a39789c2daa 100644 --- a/src/main/runtime/folder-workspace-git-routing.test.ts +++ b/src/main/runtime/folder-workspace-git-routing.test.ts @@ -290,23 +290,7 @@ describe('#6357 monorepo folder workspace', () => { expect(status.entries.every((entry) => entry.area !== 'staged')).toBe(true) }) - it('K: commit has no path to route on, so it commits every child repo with a staged index', async () => { - buildFixture() - const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) - const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` - await runtime.bulkStageRuntimeGitPaths(selector, [ - 'fint_api/src/app.ts', - 'fint-portal/src/app.ts' - ]) - await expect(runtime.commitRuntimeGit(selector, 'span two repos')).resolves.toEqual({ - success: true - }) - for (const dir of [CHILD_API, join(CONTAINER, 'fint-portal')]) { - expect(git(dir, 'log', '-1', '--pretty=%s').trim()).toBe('span two repos') - } - }) - - it('K2: a child repo with nothing staged is skipped, not committed empty', async () => { + it('K: commit routes to the one child repo that has a staged index', async () => { buildFixture() const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` @@ -314,10 +298,47 @@ describe('#6357 monorepo folder workspace', () => { // Why: staging only one repo is the common case — the user edited one project. await runtime.stageRuntimeGitPath(selector, 'fint_api/src/app.ts') await expect(runtime.commitRuntimeGit(selector, 'only api')).resolves.toEqual({ success: true }) - expect(git(CHILD_API, 'rev-list', '--count', 'HEAD').trim()).toBe('2') + expect(git(CHILD_API, 'log', '-1', '--pretty=%s').trim()).toBe('only api') expect(git(portal, 'rev-list', '--count', 'HEAD').trim()).toBe('1') }) + it('K2: staging across two repos is refused before either is committed', async () => { + buildFixture() + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + await runtime.bulkStageRuntimeGitPaths(selector, [ + 'fint_api/src/app.ts', + 'fint-portal/src/app.ts' + ]) + // Why: git has no cross-repo transaction. Committing repo 1 then failing on + // repo 2 leaves a half-done commit that `{ success, error }` cannot describe + // and the user cannot undo from the UI — so refuse before mutating anything. + const result = await runtime.commitRuntimeGit(selector, 'span two repos') + expect(result.success).toBe(false) + expect(result.error).toContain('fint-api-repo') + expect(result.error).toContain('fint-portal-repo') + for (const dir of [CHILD_API, join(CONTAINER, 'fint-portal')]) { + expect(git(dir, 'rev-list', '--count', 'HEAD').trim()).toBe('1') + } + }) + + it('K5: a child repo that cannot be inspected fails the commit instead of committing around it', async () => { + buildFixture() + // Why: the repo we cannot read could be the one the user staged. Committing + // the others and reporting success is how a staged file is silently skipped. + const unreadable = join(CONTAINER, 'fint-detached') + mkdirSync(unreadable, { recursive: true }) + const runtime = new OrcaRuntimeService( + makeStore({ repos: [...allRepos(), repo('fint-detached-repo', unreadable, 'git')] }) as never + ) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + await runtime.stageRuntimeGitPath(selector, 'fint_api/src/app.ts') + const result = await runtime.commitRuntimeGit(selector, 'api only') + expect(result.success).toBe(false) + expect(result.error).toContain('fint-detached-repo') + expect(git(CHILD_API, 'rev-list', '--count', 'HEAD').trim()).toBe('1') + }) + it('K3: committing with nothing staged anywhere reports it instead of silently succeeding', async () => { buildFixture() const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) @@ -357,14 +378,98 @@ describe('#6357 monorepo folder workspace', () => { expect(await runtime.getRuntimeGitConflictOperation(`path:${CHILD_API}`)).toBe('unknown') }) - it('L2: aborting when no child repo is mid-operation settles instead of erroring', async () => { + it('L2: aborting when no child repo is mid-operation says so instead of reporting ok', async () => { buildFixture() const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` - // Why: `git merge --abort` errors with no merge in progress; fanning that out - // would make a no-op abort fail for a workspace but succeed for a single repo. - await expect(runtime.abortRuntimeGitMerge(selector)).resolves.toEqual({ ok: true }) - await expect(runtime.abortRuntimeGitRebase(selector)).resolves.toEqual({ ok: true }) + // Why: a single repo's `git merge --abort` errors with no merge in progress. + // Returning ok here would be the only path where the user is told a conflict + // was cleared that never existed. + await expect(runtime.abortRuntimeGitMerge(selector)).rejects.toThrow( + 'No repository in this workspace has a merge in progress.' + ) + await expect(runtime.abortRuntimeGitRebase(selector)).rejects.toThrow( + 'No repository in this workspace has a rebase in progress.' + ) + }) + + it('L3: a failed abort propagates rather than reporting ok on a still-conflicted repo', async () => { + buildFixture() + 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: a terminal running git in the same repo holds index.lock, and the + // abort fails against it. The repo stays conflicted, so the user must not be + // told the conflict was cleared. + writeFileSync(join(CHILD_API, '.git', 'index.lock'), '') + await expect(runtime.abortRuntimeGitMerge(selector)).rejects.toThrow() + rmSync(join(CHILD_API, '.git', 'index.lock'), { force: true }) + expect(await runtime.getRuntimeGitConflictOperation(`path:${CHILD_API}`)).toBe('merge') + }) + + it('L4: two child repos mid-merge is refused rather than aborting an arbitrary one', async () => { + buildFixture() + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + for (const dir of [CHILD_API, join(CONTAINER, 'fint-portal')]) { + git(dir, 'checkout', '-q', '--', '.') + git(dir, 'checkout', '-q', '-b', 'other') + writeFileSync(join(dir, 'src', 'app.ts'), 'other branch\n') + git(dir, 'commit', '-qam', 'other') + git(dir, 'checkout', '-q', 'master') + writeFileSync(join(dir, 'src', 'app.ts'), 'master branch\n') + git(dir, 'commit', '-qam', 'master') + expect(() => git(dir, 'merge', 'other')).toThrow() + } + await expect(runtime.abortRuntimeGitMerge(selector)).rejects.toThrow('More than one repository') + // Why: neither may be aborted — the user picked no repo, so we pick none. + for (const dir of [CHILD_API, join(CONTAINER, 'fint-portal')]) { + expect(await runtime.getRuntimeGitConflictOperation(`path:${dir}`)).toBe('merge') + } + // Why: with two conflicts there is no single workspace-level answer either. + expect((await runtime.getRuntimeGitStatus(selector)).conflictOperation).toBe('unknown') + }) + + it('M: commit-message generation routes to the same child repo the commit would', 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: before routing, this threw `selector_not_found` — the mobile commit + // screen's Generate button was dead for every folder workspace. Stub the + // single-repo implementation so the assertion is about which selector it is + // handed, not about reaching a drafting model. + const inner = vi + .spyOn( + runtime['gitCommands'] as { generateRuntimeCommitMessage: (...args: never[]) => unknown }, + 'generateRuntimeCommitMessage' + ) + .mockResolvedValue({ success: true, message: 'drafted' } as never) + await expect(runtime.generateRuntimeCommitMessage(selector)).resolves.toEqual({ + success: true, + message: 'drafted' + }) + const routedSelector = String(inner.mock.calls[0]?.[0]) + expect(routedSelector).not.toBe(selector) + // Why: the child repo's own worktree id — the same target the commit picks. + const commitTarget = await runtime.getRuntimeGitStatus(routedSelector) + expect(commitTarget.entries.map((entry) => entry.path)).toContain('src/app.ts') + inner.mockRestore() + }) + + it('M2: generation reports no staged changes rather than a resolution error', async () => { + buildFixture() + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + const result = await runtime.generateRuntimeCommitMessage(selector) + expect(result).toEqual({ success: false, error: 'No staged changes to summarize.' }) }) it('E: the real child git worktree resolves fine when its repo is registered', async () => { diff --git a/src/main/runtime/orca-runtime-git.ts b/src/main/runtime/orca-runtime-git.ts index b03e1ed52a1..22b513a4326 100644 --- a/src/main/runtime/orca-runtime-git.ts +++ b/src/main/runtime/orca-runtime-git.ts @@ -126,7 +126,7 @@ function normalizeRuntimeGitRelativePath(filePath: string): string { return relativePath } -type RuntimeGitTarget = { +export type RuntimeGitTarget = { worktree: ResolvedRuntimeGitWorktree repo?: Repo connectionId?: string diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 6f9ba347e0a..37a8a3a6043 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -303,9 +303,13 @@ import type { GitStatusResult } from '../../shared/types' import { listFolderWorkspaceChildRepos, matchFolderWorkspaceChildRepo, - mergeFolderWorkspaceGitStatus, - summarizeFolderWorkspaceFanOut + mergeFolderWorkspaceGitStatus } from './folder-workspace-child-repos' +import { + selectFolderWorkspaceAbortTarget, + selectFolderWorkspaceCommitTarget, + type FolderWorkspaceChildCandidate +} from './folder-workspace-git-operation-routing' import { resolveTerminalStartupCwd } from '../../shared/terminal-startup-cwd' import { isWslUncPath, parseWslUncPath } from '../../shared/wsl-paths' import { @@ -459,6 +463,7 @@ import { commitRuntimeGitForTarget, getRuntimeGitStatusForTarget, RuntimeGitCommands, + type RuntimeGitTarget, type RuntimeGitTargetPathGroup } from './orca-runtime-git' import { @@ -7148,8 +7153,12 @@ export class OrcaRuntimeService { commitRuntimeGit: RuntimeGitCommands['commitRuntimeGit'] = async (selector, message) => (await this.commitFolderWorkspaceGit(selector, message)) ?? (await this.gitCommands.commitRuntimeGit(selector, message)) - generateRuntimeCommitMessage: RuntimeGitCommands['generateRuntimeCommitMessage'] = - this.gitCommands.generateRuntimeCommitMessage.bind(this.gitCommands) + generateRuntimeCommitMessage: RuntimeGitCommands['generateRuntimeCommitMessage'] = async ( + selector, + settingsOverride + ) => + (await this.generateFolderWorkspaceCommitMessage(selector, settingsOverride)) ?? + (await this.gitCommands.generateRuntimeCommitMessage(selector, settingsOverride)) discoverRuntimeCommitMessageModels: RuntimeGitCommands['discoverRuntimeCommitMessageModels'] = this.gitCommands.discoverRuntimeCommitMessageModels.bind(this.gitCommands) cancelRuntimeGenerateCommitMessage: RuntimeGitCommands['cancelRuntimeGenerateCommitMessage'] = @@ -24218,20 +24227,71 @@ export class OrcaRuntimeService { ) } + /** + * Inspect every child repo of a folder workspace and mark the ones matching + * `isSubject`. A repo that cannot be resolved or read is reported rather than + * dropped: the selection policy fails closed on it, because that unreadable repo + * could be exactly the one the user meant to act on. + */ + private async inspectFolderWorkspaceChildRepos( + folderPath: string, + isSubject: (status: GitStatusResult) => boolean + ): Promise[]> { + const childRepos = listFolderWorkspaceChildRepos(this.requireStore().getRepos(), folderPath) + return Promise.all( + childRepos.map(async (repo) => { + const repoName = repo.displayName || basename(repo.path) + const target = await this.resolveChildRepoGitTarget(repo) + if (!target) { + return { repoName, error: 'no resolvable git worktree' } + } + try { + const status = await getRuntimeGitStatusForTarget(target) + return { target, repoName, selected: isSubject(status) } + } catch (error) { + return { + target, + repoName, + error: error instanceof Error ? error.message : 'status read failed' + } + } + }) + ) + } + + /** + * The one child repo a staged-index operation belongs to. Null when the selector + * is not a folder workspace, so callers can fall through to the single-repo path. + */ + private async selectFolderWorkspaceStagedTarget( + selector: string + ): Promise<{ ok: true; target: RuntimeGitTarget } | { ok: false; error: string } | null> { + const scope = await this.resolveFolderWorkspaceLaunchScope(selector) + const folderWorkspace = scope?.folderWorkspace + if (!folderWorkspace) { + return null + } + return selectFolderWorkspaceCommitTarget( + await this.inspectFolderWorkspaceChildRepos(folderWorkspace.folderPath, (status) => + status.entries.some((entry) => entry.area === 'staged') + ) + ) + } + /** * Commit for a folder-workspace selector. `git.commit` carries no path, so there - * is nothing to route on — instead commit every child repo that actually has - * something staged, which is the set the merged status just showed the user as - * staged. Child repos with an empty index are skipped rather than reporting - * "nothing to commit". Returns null when the selector is not a folder workspace. + * is nothing to route on — resolve the single child repo that has staged changes + * and commit that one. Git has no cross-repo transaction, so committing several + * cannot be undone as a unit if a later repo fails; 0 or >1 candidates is + * rejected before anything is mutated. Returns null when the selector is not a + * folder workspace. */ private async commitFolderWorkspaceGit( selector: string, message: string ): Promise<{ success: boolean; error?: string } | null> { - const scope = await this.resolveFolderWorkspaceLaunchScope(selector) - const folderWorkspace = scope?.folderWorkspace - if (!folderWorkspace) { + const selected = await this.selectFolderWorkspaceStagedTarget(selector) + if (!selected) { return null } // Why: the single-repo path validates this before resolving; routing around it @@ -24239,53 +24299,51 @@ export class OrcaRuntimeService { if (message.trim().length === 0) { throw new Error('Commit message is required') } - const childRepos = listFolderWorkspaceChildRepos( - this.requireStore().getRepos(), - folderWorkspace.folderPath - ) - const staged = await Promise.all( - childRepos.map(async (repo) => { - const target = await this.resolveChildRepoGitTarget(repo) - if (!target) { - return null - } - // Why: a status failure here silently drops the repo from the commit, so the - // user sees a success that skipped their staged files. Log which one. - const status = await getRuntimeGitStatusForTarget(target).catch((error: unknown) => { - console.warn('[runtime] folder-workspace commit skipped a child repo', { - repo: repo.path, - error - }) - return null - }) - const hasStaged = status?.entries.some((entry) => entry.area === 'staged') === true - return hasStaged ? { repo, target } : null - }) - ) - const targets = staged.filter((entry) => entry !== null) - if (targets.length === 0) { - // Why: matches what a single-repo commit reports for an empty index, so the - // renderer's existing failure copy stays accurate. - return { success: false, error: 'nothing to commit' } + if (!selected.ok) { + return { success: false, error: selected.error } } - const results = await Promise.all( - targets.map(async ({ repo, target }) => { - const repoName = repo.displayName || basename(repo.path) - try { - const result = await commitRuntimeGitForTarget(target, message) - return { repoName, ...(result.success ? {} : { error: result.error ?? 'Commit failed' }) } - } catch (error) { - return { repoName, error: error instanceof Error ? error.message : 'Commit failed' } - } - }) + try { + return await commitRuntimeGitForTarget(selected.target, message) + } catch (error) { + return { success: false, error: error instanceof Error ? error.message : 'Commit failed' } + } + } + + /** + * Draft a commit message for a folder-workspace selector. Routes to the same + * child repo the commit itself would hit — otherwise the message is generated + * for one repo and committed to another. Re-enters by the child's own selector + * so SSH and WSL keep their tested single-repo path. + * Returns null when the selector is not a folder workspace. + */ + private async generateFolderWorkspaceCommitMessage( + selector: string, + settingsOverride?: Parameters[1] + ): Promise> | null> { + const selected = await this.selectFolderWorkspaceStagedTarget(selector) + if (!selected) { + return null + } + if (!selected.ok) { + return { + success: false, + error: + selected.error === 'nothing to commit' + ? 'No staged changes to summarize.' + : selected.error + } + } + return this.gitCommands.generateRuntimeCommitMessage( + `id:${selected.target.worktree.id}`, + settingsOverride ) - return summarizeFolderWorkspaceFanOut(results) } /** * Abort an in-progress merge/rebase for a folder-workspace selector. Like commit - * this carries no path, so it targets every child repo that is actually in that - * conflict state — aborting a repo that is not mid-operation would error. + * this carries no path, so it resolves the one child repo actually in that + * conflict state. A failed abort propagates — reporting `ok` for a repo that is + * still conflicted is how the user loses the conflict. * Returns null when the selector is not a folder workspace. */ private async abortFolderWorkspaceGit( @@ -24297,46 +24355,17 @@ export class OrcaRuntimeService { if (!folderWorkspace) { return null } - const childRepos = listFolderWorkspaceChildRepos( - this.requireStore().getRepos(), - folderWorkspace.folderPath - ) - const inOperation = await Promise.all( - childRepos.map(async (repo) => { - const target = await this.resolveChildRepoGitTarget(repo) - if (!target) { - return null - } - const detected = await this.gitCommands - .getRuntimeGitConflictOperation(`id:${target.worktree.id}`) - .catch((error: unknown) => { - console.warn('[runtime] folder-workspace abort could not read conflict state', { - repo: repo.path, - error - }) - return 'unknown' as const - }) - // Why: belt-and-braces. Git already refuses `merge --abort` on a mid-rebase - // repo (and vice versa), so this narrows what we run rather than what we - // prevent — it keeps N-1 pointless failing git invocations off the wire. - return detected === operation ? target : null - }) - ) - const targets = inOperation.filter((target) => target !== null) - // Why: aborting nothing is not an error — the conflict may have been resolved - // in a terminal already, and the renderer only needs the state to settle. - await Promise.all( - targets.map((target) => - abortRuntimeGitForTarget(target, operation).catch((error: unknown) => { - // Why: one repo refusing the abort must not block the others, but a - // still-conflicted repo the user believes is clean needs a trail. - console.warn(`[runtime] folder-workspace ${operation} abort failed`, { - repo: target.worktree.path, - error - }) - }) - ) + const selected = selectFolderWorkspaceAbortTarget( + await this.inspectFolderWorkspaceChildRepos( + folderWorkspace.folderPath, + (status) => status.conflictOperation === operation + ), + operation ) + if (!selected.ok) { + throw new Error(selected.error) + } + await abortRuntimeGitForTarget(selected.target, operation) return { ok: true } }