mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 16:03:20 +00:00
fix(runtime): select the folder-workspace child repo with a fail-loud index probe
`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.
This commit is contained in:
@@ -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<boolean> {
|
||||
// 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<void> {
|
||||
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 : ''
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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 ?? {})
|
||||
}
|
||||
|
||||
|
||||
@@ -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<boolean>
|
||||
): Promise<FolderWorkspaceChildCandidate<RuntimeGitTarget>[]> {
|
||||
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) {
|
||||
|
||||
Reference in New Issue
Block a user