From 0fda32fce931edaff77c68396bbfd908f582de2b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 14 Aug 2026 09:34:54 -0700 Subject: [PATCH] refactor(test): trim agent phase seams (#123714) * refactor(test): trim agent phase seams * fix(test): preserve direct node cancellation * test(cli): synchronize MCP loopback readiness --- .../bash-tools.exec-host-node-failure.ts | 14 +++---- .../bash-tools.exec-host-node-phases.test.ts | 40 ++++--------------- .../run/attempt-execution-phase.test.ts | 32 ++------------- src/cli/mcp-cli.login-loopback.test.ts | 14 +++++-- 4 files changed, 28 insertions(+), 72 deletions(-) diff --git a/src/agents/bash-tools.exec-host-node-failure.ts b/src/agents/bash-tools.exec-host-node-failure.ts index d3d731535d56..07bc1b558c75 100644 --- a/src/agents/bash-tools.exec-host-node-failure.ts +++ b/src/agents/bash-tools.exec-host-node-failure.ts @@ -154,14 +154,12 @@ export async function invokeNodeSystemRun(params: { ...(params.signal ? { signal: params.signal } : {}), } : undefined; - const raw = callOptions - ? await callGatewayTool( - "node.invoke", - { timeoutMs: params.invokeWaitMs }, - params.invoke, - callOptions, - ) - : await callGatewayTool("node.invoke", { timeoutMs: params.invokeWaitMs }, params.invoke); + const raw = await callGatewayTool( + "node.invoke", + { timeoutMs: params.invokeWaitMs }, + params.invoke, + callOptions, + ); if (typeof asNullableRecord(asNullableRecord(raw)?.payload)?.success !== "boolean") { return { ok: false, diff --git a/src/agents/bash-tools.exec-host-node-phases.test.ts b/src/agents/bash-tools.exec-host-node-phases.test.ts index 3aa45f08b0f1..b7d7129be795 100644 --- a/src/agents/bash-tools.exec-host-node-phases.test.ts +++ b/src/agents/bash-tools.exec-host-node-phases.test.ts @@ -74,13 +74,6 @@ describe("invokeNodeSystemRun failure classification", () => { nodeCommandDispatched: false, }), }, - { - name: "disconnect after dispatch", - error: gatewayNodeInvokeError({ - code: "DISCONNECTED", - nodeCommandDispatched: true, - }), - }, { name: "missing dispatch provenance", error: gatewayNodeInvokeError({ code: "NOT_CONNECTED" }), @@ -118,8 +111,6 @@ describe("invokeNodeSystemRun failure classification", () => { command: "printf 'one\\ntwo'\necho done", }); - expect(text).toContain("Exec outcome unknown (node=node-1 id=approval-1, outcome-unknown)"); - expect(text).toContain("The command may have executed. Do not rerun it automatically."); expect(text).toContain("Command:\nprintf 'one\\ntwo'\necho done"); }); }); @@ -159,30 +150,21 @@ describe("direct node run", () => { }); }); - it.each([ - { name: "with its original cancellation signal", withSignal: true }, - { name: "without a signal argument", withSignal: false }, - ])("forwards the gateway call $name", async ({ withSignal }) => { + it("forwards the original cancellation signal to the gateway", async () => { const controller = new AbortController(); - await invokeNodeSystemRunDirect( - createDirectNodeRun(withSignal ? controller.signal : undefined), - ); + await invokeNodeSystemRunDirect(createDirectNodeRun(controller.signal)); - const baseArgs = [ + expect(callGatewayToolMock).toHaveBeenCalledWith( "node.invoke", { timeoutMs: 35_000 }, expect.objectContaining({ command: "system.run" }), - ] as const; - if (withSignal) { - expect(callGatewayToolMock).toHaveBeenCalledWith(...baseArgs, { signal: controller.signal }); - } else { - expect(callGatewayToolMock).toHaveBeenCalledWith(...baseArgs); - } + { signal: controller.signal }, + ); }); - it("surfaces capped stderr and the node error when stdout is also present", async () => { + it("combines stdout, stderr, and the node error", async () => { const stdout = "small stdout"; - const stderr = `${"x".repeat(200_000)}\n... (truncated)`; + const stderr = "node stderr"; const errorText = "node command failed"; callGatewayToolMock.mockResolvedValueOnce({ payload: { @@ -197,14 +179,8 @@ describe("direct node run", () => { const result = await invokeNodeSystemRunDirect(createDirectNodeRun()); const visibleText = result.content[0]?.type === "text" ? result.content[0].text : ""; - expect(visibleText).toContain("... (truncated)"); - expect(visibleText).toContain(errorText); expect(visibleText).toBe(`${stdout}\n${stderr}\n${errorText}`); - expect(result.details).toMatchObject({ - status: "failed", - exitCode: 1, - aggregated: visibleText, - }); + expect(result.details).toMatchObject({ aggregated: visibleText }); }); it("never dispatches a direct node run after cancellation", async () => { diff --git a/src/agents/embedded-agent-runner/run/attempt-execution-phase.test.ts b/src/agents/embedded-agent-runner/run/attempt-execution-phase.test.ts index 0c44010a9830..5444b226f292 100644 --- a/src/agents/embedded-agent-runner/run/attempt-execution-phase.test.ts +++ b/src/agents/embedded-agent-runner/run/attempt-execution-phase.test.ts @@ -284,7 +284,6 @@ describe("runEmbeddedAttemptExecutionPhase", () => { const settledInput = mocks.runSettledPhase.mock.calls[0]?.[0]; expect(settledInput).toEqual( expect.objectContaining({ - getRepairedRejectedProviderReplay: expect.any(Function), preparedStreamRuntime: expect.objectContaining({ cache: { observabilityEnabled: true, @@ -297,7 +296,6 @@ describe("runEmbeddedAttemptExecutionPhase", () => { }), }), ); - expect(settledInput.getRepairedRejectedProviderReplay()).toBe(true); const guardInput = mocks.installStreamGuards.mock.calls[0]?.[0]; expect(guardInput).toEqual( @@ -338,14 +336,11 @@ describe("runEmbeddedAttemptExecutionPhase", () => { expect(mocks.withOwnedSessionTranscriptWrites).toHaveBeenCalledOnce(); }); - it.each([ - { label: "external cancellation", message: "run cancelled" }, - { label: "run timeout", message: "run timed out" }, - ])("does not start a prompt after $label", async ({ message }) => { + it("does not start a prompt after external cancellation", async () => { const fixture = createFixture(); await runEmbeddedAttemptExecutionPhase(fixture.input); - const reason = new Error(message); - const abortError = new Error(message, { cause: reason }); + const reason = new Error("run cancelled"); + const abortError = new Error("run cancelled", { cause: reason }); abortError.name = "AbortError"; fixture.input.runAbortController.abort(reason); mocks.abortable.mockImplementationOnce((_signal, _promise) => Promise.reject(abortError)); @@ -353,11 +348,9 @@ describe("runEmbeddedAttemptExecutionPhase", () => { await expect( settledInput.preparedStreamRuntime.promptActiveSession("must not start"), - ).rejects.toBe(abortError); + ).rejects.toThrow("run cancelled"); expect(fixture.activeSession.prompt).not.toHaveBeenCalled(); - expect(fixture.trackPromptSettlePromise).not.toHaveBeenCalled(); - expect(mocks.abortable).toHaveBeenCalledOnce(); }); it("flushes pending tool results and disposes the session when history preparation fails", async () => { @@ -374,22 +367,5 @@ describe("runEmbeddedAttemptExecutionPhase", () => { timeoutMs: 0, }); expect(fixture.activeSession.dispose).toHaveBeenCalledOnce(); - expect(mocks.createRunAbort).not.toHaveBeenCalled(); - expect(mocks.prepareStream).not.toHaveBeenCalled(); - expect(mocks.prepareTimeout).not.toHaveBeenCalled(); - expect(mocks.runSettledPhase).not.toHaveBeenCalled(); - }); - - it("does not enter settlement when stream preparation fails", async () => { - const fixture = createFixture(); - mocks.prepareStream.mockImplementationOnce(() => { - throw new Error("stream setup failed"); - }); - - await expect(runEmbeddedAttemptExecutionPhase(fixture.input)).rejects.toThrow( - "stream setup failed", - ); - - expect(mocks.runSettledPhase).not.toHaveBeenCalled(); }); }); diff --git a/src/cli/mcp-cli.login-loopback.test.ts b/src/cli/mcp-cli.login-loopback.test.ts index 8da27aa6b158..b10cdc3a82ee 100644 --- a/src/cli/mcp-cli.login-loopback.test.ts +++ b/src/cli/mcp-cli.login-loopback.test.ts @@ -58,9 +58,13 @@ async function createWorkspace(): Promise { return dir; } -async function waitForLog(text: string): Promise { - await vi.waitFor(() => { - expect(mocks.runtime.log.mock.calls.some(([line]) => String(line).includes(text))).toBe(true); +function waitForLog(text: string): Promise { + return new Promise((resolve) => { + mocks.runtime.log.mockImplementation((line) => { + if (String(line).includes(text)) { + resolve(); + } + }); }); } @@ -94,6 +98,7 @@ function mockRedirectFlow(redirectUrl: string): void { describe("mcp login loopback callback", () => { beforeEach(() => { vi.clearAllMocks(); + mocks.runtime.log.mockReset(); program = new Command().exitOverride(); registerMcpCli(program); mocks.readMcpOAuthCredentialsStatus.mockResolvedValue({ @@ -115,8 +120,9 @@ describe("mcp login loopback callback", () => { const redirectUrl = `http://127.0.0.1:${port}/oauth/callback`; mockRedirectFlow(redirectUrl); + const waitingForBrowser = waitForLog("Waiting for the browser"); const login = program.parseAsync(["mcp", "login", "docs"], { from: "user" }); - await waitForLog("Waiting for the browser"); + await waitingForBrowser; const printedUrlIndex = mocks.runtime.log.mock.calls.findIndex(([line]) => String(line).startsWith("https://auth.example.com/authorize"), );