From 46db6088436d948aaee311f68151aa5d2d095fe1 Mon Sep 17 00:00:00 2001 From: pku-xht Date: Tue, 4 Aug 2026 23:42:35 +0800 Subject: [PATCH] Align Codex provider review evidence --- packages/subagent/subagent-acp/src/run.ts | 44 +++--------- .../subagent-acp/tests/subagent-acp.spec.ts | 68 +------------------ .../subagent-codex/tests/real-product.spec.ts | 6 +- vitest.config.ts | 1 - 4 files changed, 13 insertions(+), 106 deletions(-) diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index f264261b0e..fba0403739 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -90,41 +90,15 @@ export const DEFAULT_DISPOSE_EOF_GRACE_MS = 6_000 /** Default POSIX grace between SIGTERM and SIGKILL on dispose (the `disposeGraceMs` config). */ export const DEFAULT_DISPOSE_GRACE_MS = 3_000 -/** Largest delay Node schedules without collapsing it to one millisecond. */ -const MAX_TIMER_DELAY_MS = 2_147_483_647n - -function scaledFiniteMilliseconds(ms: number, scale: number): bigint { - const whole = Math.floor(ms) - return BigInt(whole) * BigInt(scale) - + BigInt(Math.ceil((ms - whole) * scale)) -} - -/** - * Bounded whole-tree exit wait across Node-safe timer segments. - * @param child - process tree whose liveness is authoritative. - * @param ms - positive finite base window in milliseconds. - * @param scale - integer multiplier applied without Number overflow. - */ -async function treeExitsWithin( - child: SubprocessHandle, - ms: number, - scale = 1, -): Promise { - let remaining = scaledFiniteMilliseconds(ms, scale) - while (remaining > 0n) { - const chunk = remaining > MAX_TIMER_DELAY_MS - ? MAX_TIMER_DELAY_MS - : remaining - remaining -= chunk - const controller = new AbortController() - const timer = setTimeout(() => { controller.abort() }, Number(chunk)) - try { - if (await child.waitForExit(controller.signal)) return true - } finally { - clearTimeout(timer) - } +/** Bounded whole-tree exit wait: polls the handle's tree liveness until it exits or `ms` elapses. */ +async function treeExitsWithin(child: SubprocessHandle, ms: number): Promise { + const controller = new AbortController() + const timer = setTimeout(() => { controller.abort() }, ms) + try { + return await child.waitForExit(controller.signal) + } finally { + clearTimeout(timer) } - return false } /** @@ -151,7 +125,7 @@ export async function disposeAcpChild(child: SubprocessHandle, eofGraceMs: numbe // (this plugin passes disposeGraceMs there), so the bound covers both the // escalation window and an equal confirmation window after the SIGKILL. child.terminate() - if (!(await treeExitsWithin(child, graceMs, 2))) { + if (!(await treeExitsWithin(child, graceMs * 2))) { throw new Error('ACP child process tree did not exit within its dispose windows') } } diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index 1a8bc577c1..f2cbeda27b 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from 'vitest' +import { describe, expect, it } from 'vitest' import { Context } from 'cordis' import Loader from '@cordisjs/plugin-loader' import { chmodSync, existsSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs' @@ -190,72 +190,6 @@ describe('disposeAcpChild (the backend-owned teardown ladder over seam verbs)', await expect(disposeAcpChild(never, 20, 20)).rejects.toThrow(/did not exit within its dispose windows/) }) - it('keeps an oversized finite escalation window instead of collapsing it to one millisecond', async () => { - vi.useFakeTimers() - try { - let waitCount = 0 - let reportExited!: (exited: boolean) => void - const terminate = vi.fn() - const waitForExit = vi.fn((signal?: AbortSignal) => { - waitCount += 1 - return new Promise((resolve) => { - signal?.addEventListener('abort', () => { resolve(false) }, { once: true }) - if (waitCount === 2) reportExited = resolve - }) - }) - const child: Parameters[0] = { - pid: 1, - stdin: undefined, - stdout: undefined, - stderr: undefined, - collected: {}, - done: new Promise(() => {}), - terminate, - waitForExit, - } - const disposal = disposeAcpChild(child, 0.25, Number.MAX_VALUE) - await vi.advanceTimersByTimeAsync(1) - expect(terminate).toHaveBeenCalledOnce() - expect(waitForExit).toHaveBeenCalledTimes(2) - const escalationSignal = waitForExit.mock.calls[1]?.[0] - await vi.advanceTimersByTimeAsync(1) - expect(escalationSignal?.aborted).toBe(false) - reportExited(true) - await expect(disposal).resolves.toBeUndefined() - expect(vi.getTimerCount()).toBe(0) - } finally { - vi.useRealTimers() - } - }) - - it('chains a doubled grace beyond one Node timer segment', async () => { - vi.useFakeTimers() - try { - const waitForExit = vi.fn((signal?: AbortSignal) => new Promise((resolve) => { - signal?.addEventListener('abort', () => { resolve(false) }, { once: true }) - })) - const child: Parameters[0] = { - pid: 1, - stdin: undefined, - stdout: undefined, - stderr: undefined, - collected: {}, - done: new Promise(() => {}), - terminate: vi.fn(), - waitForExit, - } - const disposal = disposeAcpChild(child, 0.25, 1_073_741_823.75) - const rejected = expect(disposal).rejects.toThrow(/did not exit within its dispose windows/) - await vi.advanceTimersByTimeAsync(1) - await vi.advanceTimersByTimeAsync(2_147_483_647) - expect(waitForExit).toHaveBeenCalledTimes(3) - await vi.advanceTimersByTimeAsync(1) - await rejected - } finally { - vi.useRealTimers() - } - }) - it('observes a spawn-level rejection and returns without a process to reap', async () => { const child = spawnSubprocess({ argv: ['bash', '-c', 'true'], diff --git a/packages/subagent/subagent-codex/tests/real-product.spec.ts b/packages/subagent/subagent-codex/tests/real-product.spec.ts index dd7c0c458f..5f73adaf7e 100644 --- a/packages/subagent/subagent-codex/tests/real-product.spec.ts +++ b/packages/subagent/subagent-codex/tests/real-product.spec.ts @@ -170,7 +170,7 @@ describe('real @openai/codex 0.146.0 product', () => { expect(recorded.headers.authorization).toBe('Bearer dsh-fake-openai-key') expect(responseInputTexts(recorded.body)).toContain(task) await expectQuiescent(harness.handles) - }, 20_000) + }, 60_000) it('cancels a real app-server command approval without executing the command', async () => { const { harness, fixture } = await realHarness([ @@ -206,7 +206,7 @@ describe('real @openai/codex 0.146.0 product', () => { requestEntry.headers.authorization === 'Bearer dsh-fake-openai-key', )).toBe(true) await expectQuiescent(harness.handles) - }, 20_000) + }, 60_000) it('settles cancellation locally and leaves the real app-server tree quiescent', async () => { const { harness, fixture } = await realHarness([{ kind: 'hold' }]) @@ -221,5 +221,5 @@ describe('real @openai/codex 0.146.0 product', () => { await expect(run.result).resolves.toMatchObject({ stopReason: 'aborted' }) await run.dispose() await expectQuiescent(harness.handles) - }, 20_000) + }, 60_000) }) diff --git a/vitest.config.ts b/vitest.config.ts index eac84d8d20..ddf7741716 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -54,7 +54,6 @@ const coverageExemptExcludes = coverageExemptRaw === '1' // Keep the narrow exception in forks while the rest of the inventory avoids per-file processes. const processBoundTests = [ 'packages/subprocess/subprocess-local/tests/spawn.spec.ts', - 'packages/subagent/subagent-codex/tests/real-product.spec.ts', 'packages/context/time-context/tests/time-context.spec.ts', 'packages/llm/llm-pi-ai/tests/adapter.spec.ts', 'packages/ui/app-boot/tests/app-boot.spec.ts',