Files
orca/src/main/text-generation/commit-message-text-generation-cancellation.test.ts
T
Neil 0a821e5bc8 fix(crash-reporting): make the own-Chromium gate a real choke point, and stop a refusal leaking the root (#18459)
* fix(crash-reporting): make the own-Chromium gate a real choke point

Round-3 review found the guard was not the choke point its own comments
claimed: six pid-addressed `taskkill /pid <pid> /t /f` families in main were
ungated and uninstrumented, so the stale-pid shape stayed producible and a
`selfInitiatedTreeKillCount: 0` could read as exculpatory when it was not.

- Gate the remaining main-process families: the git command-runner abort, the
  notebook-cell and automation-precheck timeouts.
- Turn the `src/shared` seam into the gate itself (`process-tree-kill-gate`), so
  the runProcess choke point, the codex app-server deadline kill and the
  ephemeral-VM recipe kill ask the same decision. Those three are compiled into
  the CLI/relay too and cannot import main; main installs the guard at preflight.
- Ratchet (`main-process-tree-kill-gate.test.ts`): a new pid-addressed taskkill
  in main that skips the gate fails, and the allowlist entries must still exist.
- Give pid-addressed kills eviction priority in the 32-entry ring: 32 routine
  `win-pty-job` teardowns from a window-close burst no longer evict the one
  entry that discriminates a self-kill from an external one.
- Correct the coverage doc, which described the uninstrumented Windows sites as
  POSIX `process.kill(-pid)` group kills and omitted the git and codex paths.

* fix(crash-reporting): keep a refused tree-kill from leaking the root it owns

A refusal must block the pid-addressed tree walk, not the termination. Five of
the six gated sites returned on refusal with no fallback, so a refused
`taskkill /pid /t /f` left git.exe, a timed-out notebook cell, an automation
precheck or an ephemeral-VM recipe running while the caller reported it stopped.
The root kill is addressed by the child handle, which cannot reach the recycled
pid the refusal is about, so it stays correct and required on that path.

Also fixes the ring eviction the scope preference introduced: with the ring
saturated by pid-addressed kills, the only non-pid-addressed entry is the one
just pushed, so the splice evicted itself and the detail came back `{}` --
byte-identical to the external-kill arm, in the window-close case the guard
exists for. Eviction now excludes the newest entry and falls back to FIFO.

Tests: refusal now asserts the root kill at all six sites, and the ring covers
the saturated-pid ordering as well as round 3's group-burst ordering.

* fix(crash-reporting): stop a refused tree-kill leaking the commit-message agent, and count call sites

Two round-5 blocking findings, both open on main and on both branches.

`killSourceControlAgentProcess` had no root-kill fallback on its win32 arm: the
taskkill was the only termination, so once the own-Chromium gate could refuse it
the promise resolved having killed nothing. Both callers do
`terminationComplete ??= killSourceControlAgentProcess(child)` and then release
the managed-home lock on that promise, so a refusal left the local Codex/Claude
commit-message agent running while the caller reported it stopped -- the
lock-contention failure the taskkill was added for. Same fix as the six sibling
sites: the handle-addressed root kill cannot reach the recycled pid the refusal
is about, so it stays correct and required on that path.

The ratchet was file-granular, not call-site granular: one gate mention anywhere
in a file exempted every taskkill in it, which left the six files that now ask
the gate ratchet-blind -- the inverse of what it is for. It now counts `/pid`
call sites against gate admissions per file, so a second ungated kill inside an
existing family fails. Keying on the `/pid` argument rather than a quoted
`taskkill` also catches a kill whose program name comes from a constant. The
three comments that claimed more than the old scan enforced now state the rule
and its two remaining blind spots.

Also: the recording in `admitSelfInitiatedTreeKill` is now wrapped the way the
`admitProcessTreeKill` seam already wraps it, with the refusal decision taken
before anything that can throw so a diagnostics failure cannot flip it; and
`orca-chromium-process-pids` documents the false-positive direction (a stale
`getAppMetrics()` entry plus pid reuse refuses a live unrelated child), which is
the mechanism the root-kill fallback exists to bound.

Tests: refusal now asserts the root kill at all seven sites; the ratchet asserts
call-site counting and the constant-program form.

* test(crash-reporting): run the own-Chromium gate against real Windows trees

Nothing on this branch had ever executed on Windows. The unit tests pin the
gate's decision against a mocked taskkill, which cannot show that the decision
does anything to a real process: that `/T /F` reaps a detached grandchild, that
a refusal leaves that tree standing, or that the handle-addressed root kill the
refusal path falls back to reaps the root while orphaning descendants.

Adds a win32-gated live test covering all four, registered in both the
`package_windows` CI lane and `WINDOWS_PACKAGE_TESTS` as
`win32-test-lane-registration` requires.

Also completes the coverage doc's "never instrumented" list, which omitted the
macOS keyboard-input-source probe's POSIX group kill in `ipc/app.ts`.

* fix(crash-reporting): pin the commit-message root kill on the Windows arm

The first Windows run of this branch found nine failures the macOS suite
cannot see: `commit-message-text-generation-test-harness` asserts
`expect(child.kill).not.toHaveBeenCalled()` on `process.platform === 'win32'`,
which is the contract the previous commit deliberately replaced — and it
branches on the real platform, so it is dead code everywhere CI runs today.

The harness now asserts the handle-addressed root kill on every platform. On
win32 it lands after the tree walk, so the expectation waits rather than reading
one tick early, and its ten call sites await it. Red against the pre-fix arm at
all seven sites; the production code is unchanged.

* test(crash-reporting): remove the Windows lane marker tree through the retrying helper

The new win32 spec teardown used a raw rmSync, which the windows-lane-tree-removal
boundary ratchet rejects — and which is exactly the EPERM the ratchet exists to
prevent, since this spec's marker directory is written by processes it has just
force-killed.

* fix(crash-reporting): only refuse pid-addressed tree walks, disclose the handle-less codex site

The own-Chromium gate refused the POSIX process-group arm of
signalProcessTree as well, which was new macOS/Linux behaviour: a stale
getAppMetrics() entry plus pid reuse would orphan a group that main reaps
today. A POSIX group only holds what Orca put in it, so the refusal is now
scoped to win-taskkill-tree and the POSIX arm is recorded and admitted like
the other group kills in main. That also drops the synchronous
getAppMetrics() read from every POSIX termination.

codex-turn-added-roots kills roots found by a table walk, so a refusal has
no handle to fall back to. Pin that the refusal is visible - crumb written,
turn reported as not cancelled - rather than fixing what cannot be fixed.

* test(crash-reporting): detach the Windows survival fixture and observe real spawns
2026-09-04 16:42:42 -07:00

482 lines
16 KiB
TypeScript

import { spawn } from 'node:child_process'
import type * as ChildProcess from 'node:child_process'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import {
cancelGenerateCommitMessageLocal,
cancelGeneratePullRequestFieldsLocal,
discoverCommitMessageModelsLocal,
generateCommitMessageFromContext,
generatePullRequestFieldsFromContext
} from './commit-message-text-generation'
import {
createChildTerminationExpectation,
createMockDiscoveryChild
} from './commit-message-text-generation-test-harness'
const { terminateWindowsProcessTreeMock } = vi.hoisted(() => ({
terminateWindowsProcessTreeMock: vi.fn(async () => {})
}))
vi.mock('../windows-process-tree-kill', () => ({
terminateWindowsProcessTree: terminateWindowsProcessTreeMock
}))
vi.mock('child_process', async (importOriginal) => {
const actual = await importOriginal<typeof ChildProcess>()
return {
...actual,
spawn: vi.fn(actual.spawn)
}
})
const spawnMock = vi.mocked(spawn)
const expectChildTerminated = createChildTerminationExpectation(terminateWindowsProcessTreeMock)
beforeEach(() => {
terminateWindowsProcessTreeMock.mockClear()
terminateWindowsProcessTreeMock.mockResolvedValue(undefined)
spawnMock.mockClear()
})
describe('generateCommitMessageFromContext', () => {
it('keeps local commit-message and pull-request cancellation lanes separate', async () => {
const children: {
pid: number
kill: ReturnType<typeof vi.fn>
listeners: Map<string, (value: unknown) => void>
}[] = []
spawnMock.mockImplementation(() => {
const listeners = new Map<string, (value: unknown) => void>()
const child = {
pid: 123 + children.length,
kill: vi.fn(),
stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) },
stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) },
stdin: { end: vi.fn() },
on: vi.fn((event, callback) => listeners.set(event, callback))
}
children.push({ pid: child.pid, kill: child.kill, listeners })
return child as never
})
const commit = generateCommitMessageFromContext(
{
branch: 'main',
stagedSummary: 'M\tREADME.md',
stagedPatch: '+hello'
},
{
agentId: 'custom',
model: '',
customAgentCommand: 'agent'
},
{
kind: 'local',
cwd: '/repo'
}
)
const pullRequest = generatePullRequestFieldsFromContext(
{
branch: 'feature/pr-fields',
base: 'main',
branchChangedByPreparation: false,
currentTitle: '',
currentBody: '',
currentDraft: false,
commitSummary: '- feat: update README',
changeSummary: 'M\tREADME.md',
patch: '+hello'
},
{
agentId: 'custom',
model: '',
customAgentCommand: 'agent'
},
{
kind: 'local',
cwd: '/repo'
}
)
cancelGenerateCommitMessageLocal('/repo')
await expectChildTerminated(children[0]!)
expect(children[1]?.kill).not.toHaveBeenCalled()
children[0]?.listeners.get('close')?.(null)
const pullRequestStdout = children[1]?.listeners.get('stdout:data')
pullRequestStdout?.(
Buffer.from('{"base":"main","title":"Update README","body":"Details","draft":false}')
)
children[1]?.listeners.get('close')?.(0)
await expect(commit).resolves.toEqual({
success: false,
error: 'Generation canceled.',
canceled: true
})
await expect(pullRequest).resolves.toMatchObject({
success: true,
fields: {
base: 'main',
title: 'Update README',
body: 'Details',
draft: false
}
})
cancelGeneratePullRequestFieldsLocal('/repo')
expect(children[1]?.kill).not.toHaveBeenCalled()
})
it('keeps local pull-request cancellation from stopping commit-message generation', async () => {
const children: {
pid: number
kill: ReturnType<typeof vi.fn>
listeners: Map<string, (value: unknown) => void>
}[] = []
spawnMock.mockImplementation(() => {
const listeners = new Map<string, (value: unknown) => void>()
const child = {
pid: 123 + children.length,
kill: vi.fn(),
stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) },
stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) },
stdin: { end: vi.fn() },
on: vi.fn((event, callback) => listeners.set(event, callback))
}
children.push({ pid: child.pid, kill: child.kill, listeners })
return child as never
})
const commit = generateCommitMessageFromContext(
{
branch: 'main',
stagedSummary: 'M\tREADME.md',
stagedPatch: '+hello'
},
{
agentId: 'custom',
model: '',
customAgentCommand: 'agent'
},
{
kind: 'local',
cwd: '/repo'
}
)
const pullRequest = generatePullRequestFieldsFromContext(
{
branch: 'feature/pr-fields',
base: 'main',
branchChangedByPreparation: false,
currentTitle: '',
currentBody: '',
currentDraft: false,
commitSummary: '- feat: update README',
changeSummary: 'M\tREADME.md',
patch: '+hello'
},
{
agentId: 'custom',
model: '',
customAgentCommand: 'agent'
},
{
kind: 'local',
cwd: '/repo'
}
)
cancelGeneratePullRequestFieldsLocal('/repo')
expect(children[0]?.kill).not.toHaveBeenCalled()
await expectChildTerminated(children[1]!)
const commitStdout = children[0]?.listeners.get('stdout:data')
commitStdout?.(Buffer.from('Update README\n'))
children[0]?.listeners.get('close')?.(0)
children[1]?.listeners.get('close')?.(null)
await expect(commit).resolves.toEqual({
success: true,
message: 'Update README',
agentLabel: 'agent'
})
await expect(pullRequest).resolves.toEqual({
success: false,
error: 'Generation canceled.',
canceled: true,
branchChangedByPreparation: false
})
})
it('reports branch changes when pull request generation is canceled', async () => {
const listeners = new Map<string, (value: unknown) => void>()
const child = {
pid: 123,
kill: vi.fn(),
stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) },
stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) },
stdin: { end: vi.fn() },
on: vi.fn((event, callback) => listeners.set(event, callback))
}
spawnMock.mockReturnValue(child as never)
const pullRequest = generatePullRequestFieldsFromContext(
{
branch: 'feature/pr-fields',
base: 'main',
branchChangedByPreparation: true,
currentTitle: '',
currentBody: '',
currentDraft: false,
commitSummary: '- feat: update README',
changeSummary: 'M\tREADME.md',
patch: '+hello'
},
{
agentId: 'custom',
model: '',
customAgentCommand: 'agent'
},
{
kind: 'local',
cwd: '/repo'
}
)
cancelGeneratePullRequestFieldsLocal('/repo')
listeners.get('close')?.(null)
await expectChildTerminated(child)
await expect(pullRequest).resolves.toEqual({
success: false,
error: 'Generation canceled.',
canceled: true,
branchChangedByPreparation: true
})
})
it('settles local commit-message cancellation even when the killed child does not close', async () => {
vi.useFakeTimers()
try {
const listeners = new Map<string, (value: unknown) => void>()
const removeListener = (key: string, callback: (value: unknown) => void): void => {
if (listeners.get(key) === callback) {
listeners.delete(key)
}
}
const child = {
pid: 123,
kill: vi.fn(),
stdout: {
on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)),
off: vi.fn((event, callback) => removeListener(`stdout:${event}`, callback))
},
stderr: {
on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)),
off: vi.fn((event, callback) => removeListener(`stderr:${event}`, callback))
},
stdin: { end: vi.fn() },
on: vi.fn((event, callback) => listeners.set(event, callback)),
off: vi.fn((event, callback) => removeListener(event, callback))
}
spawnMock.mockReturnValue(child as never)
const pending = generateCommitMessageFromContext(
{
branch: 'main',
stagedSummary: 'M\tREADME.md',
stagedPatch: '+hello'
},
{
agentId: 'custom',
model: '',
customAgentCommand: 'agent'
},
{
kind: 'local',
cwd: '/repo'
}
)
const outcomePromise = pending.then((result) =>
!result.success && result.canceled ? 'canceled' : 'other'
)
cancelGenerateCommitMessageLocal('/repo')
await expectChildTerminated(child)
await Promise.resolve()
await Promise.resolve()
await Promise.resolve()
await Promise.resolve()
const outcome = await Promise.race([outcomePromise, Promise.resolve('pending')])
expect(outcome).toBe('canceled')
expect(listeners.has('stdout:data')).toBe(false)
expect(listeners.has('stderr:data')).toBe(false)
expect(listeners.has('error')).toBe(false)
expect(listeners.has('close')).toBe(false)
} finally {
vi.useRealTimers()
}
})
it('publishes Codex cancellation immediately but holds its home lock until close', async () => {
const firstChild = createMockDiscoveryChild()
const secondChild = createMockDiscoveryChild()
spawnMock.mockReturnValueOnce(firstChild as never).mockReturnValueOnce(secondChild as never)
const env = { CODEX_HOME: '/managed/codex-generation-home' }
const context = { branch: 'main', stagedSummary: 'M\tREADME.md', stagedPatch: '+hello' }
const params = { agentId: 'codex' as const, model: 'gpt-5.5' }
const first = generateCommitMessageFromContext(context, params, {
kind: 'local',
cwd: '/repo',
env
})
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1))
cancelGenerateCommitMessageLocal('/repo')
await expect(first).resolves.toEqual({
success: false,
error: 'Generation canceled.',
canceled: true
})
await expectChildTerminated(firstChild)
const second = generateCommitMessageFromContext(context, params, {
kind: 'local',
cwd: '/repo-2',
env
})
await Promise.resolve()
expect(spawnMock).toHaveBeenCalledTimes(1)
firstChild.emit('close', null)
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2))
secondChild.stdout.emit('data', Buffer.from('Update README\n'))
secondChild.emit('close', 0)
await expect(second).resolves.toMatchObject({ success: true, message: 'Update README' })
})
it('releases the Codex home lock when the child exits with a descendant holding its stdio', async () => {
const firstChild = createMockDiscoveryChild()
const secondChild = createMockDiscoveryChild()
spawnMock.mockReturnValueOnce(firstChild as never).mockReturnValueOnce(secondChild as never)
const env = { CODEX_HOME: '/managed/codex-descendant-home' }
const context = { branch: 'main', stagedSummary: 'M\tREADME.md', stagedPatch: '+hello' }
const params = { agentId: 'codex' as const, model: 'gpt-5.5' }
const first = generateCommitMessageFromContext(context, params, {
kind: 'local',
cwd: '/descendant-repo',
env
})
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1))
cancelGenerateCommitMessageLocal('/descendant-repo')
await expect(first).resolves.toMatchObject({ canceled: true })
await expectChildTerminated(firstChild)
// SIGKILL reaches the codex process but not a grandchild that inherited its
// stdout, so 'exit' arrives and 'close' never does.
firstChild.emit('exit', null, 'SIGKILL')
const second = generateCommitMessageFromContext(context, params, {
kind: 'local',
cwd: '/descendant-repo-2',
env
})
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2))
secondChild.stdout.emit('data', Buffer.from('Update README\n'))
secondChild.emit('close', 0)
await expect(second).resolves.toMatchObject({ success: true, message: 'Update README' })
})
it('holds the Codex home lock until Windows tree termination and wrapper close', async () => {
const originalPlatform = process.platform
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
let finishTreeKill!: () => void
terminateWindowsProcessTreeMock.mockReturnValueOnce(
new Promise<void>((resolve) => {
finishTreeKill = resolve
})
)
const firstChild = createMockDiscoveryChild()
const secondChild = createMockDiscoveryChild()
spawnMock.mockReturnValueOnce(firstChild as never).mockReturnValueOnce(secondChild as never)
const env = { CODEX_HOME: 'C:\\managed\\codex-generation-home' }
const context = { branch: 'main', stagedSummary: 'M\tREADME.md', stagedPatch: '+hello' }
const params = { agentId: 'codex' as const, model: 'gpt-5.5' }
try {
const first = generateCommitMessageFromContext(context, params, {
kind: 'local',
cwd: 'C:\\repo',
env
})
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1))
cancelGenerateCommitMessageLocal('C:\\repo')
await expect(first).resolves.toMatchObject({ canceled: true })
const second = generateCommitMessageFromContext(context, params, {
kind: 'local',
cwd: 'C:\\repo-2',
env
})
firstChild.emit('close', null)
await Promise.resolve()
expect(spawnMock).toHaveBeenCalledTimes(1)
finishTreeKill()
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2))
secondChild.stdout.emit('data', Buffer.from('Update README\n'))
secondChild.emit('close', 0)
await expect(second).resolves.toMatchObject({ success: true, message: 'Update README' })
} finally {
Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
}
})
it('cancels Codex generation promptly while it is queued behind the home lock', async () => {
const discoveryChild = createMockDiscoveryChild()
const laterChild = createMockDiscoveryChild()
spawnMock.mockReturnValueOnce(discoveryChild as never).mockReturnValueOnce(laterChild as never)
const env = { CODEX_HOME: '/managed/codex-queued-home' }
const blocker = discoverCommitMessageModelsLocal('codex', env)
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1))
const queued = generateCommitMessageFromContext(
{ branch: 'main', stagedSummary: 'M\tREADME.md', stagedPatch: '+hello' },
{ agentId: 'codex', model: 'gpt-5.5' },
{ kind: 'local', cwd: '/queued-repo', env }
)
cancelGenerateCommitMessageLocal('/queued-repo')
await expect(queued).resolves.toEqual({
success: false,
error: 'Generation canceled.',
canceled: true
})
expect(spawnMock).toHaveBeenCalledTimes(1)
discoveryChild.stdout.emit(
'data',
Buffer.from(JSON.stringify({ models: [{ slug: 'gpt-5.5', display_name: 'GPT-5.5' }] }))
)
discoveryChild.emit('close', 0)
await expect(blocker).resolves.toMatchObject({ success: true })
const later = generateCommitMessageFromContext(
{ branch: 'main', stagedSummary: 'M\tREADME.md', stagedPatch: '+later' },
{ agentId: 'codex', model: 'gpt-5.5' },
{ kind: 'local', cwd: '/later-repo', env }
)
await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2))
laterChild.stdout.emit('data', Buffer.from('Update later\n'))
laterChild.emit('close', 0)
await expect(later).resolves.toMatchObject({ success: true, message: 'Update later' })
})
})