diff --git a/doc/idle-approved-tool-recovery.md b/doc/idle-approved-tool-recovery.md index f7fe02cc90f..4f33170558f 100644 --- a/doc/idle-approved-tool-recovery.md +++ b/doc/idle-approved-tool-recovery.md @@ -75,8 +75,16 @@ Controlled Test-tab provider tests cover both foreground/recovery race winners, completed legacy outcomes, interrupted legacy calls, target changes, and a failed atomic outcome write followed by quarantine without another provider call. -The service/startup selection passes 122 tests. The existing gateway acceptance +The service/startup selection passes 123 tests. The existing gateway acceptance suite passes 92 tests. The strengthened paging and outcome-failure cases pass again after their final edits. Workspace typecheck and build pass. Full CI and review remain separate PR gates; no real external provider was invoked by these fixtures. + +Review found that a one-shot success-write failure could fall into the provider +error handler and save a false failure on its second attempt. The added sequence- +based PostgreSQL fault reproduces that bug before the fix. Provider execution, +pre-dispatch validation, and outcome persistence now have separate error +boundaries. Both persistent and one-shot result-write failures retain the claim +for quarantine without repeating the call. The complete 123-test selection and +92 gateway acceptance cases pass after the repair, as do typecheck and build. diff --git a/doc/plans/2026-10-24-recurring-server-sleep-stack.md b/doc/plans/2026-10-24-recurring-server-sleep-stack.md index 89a9e404a99..d96322303a6 100644 --- a/doc/plans/2026-10-24-recurring-server-sleep-stack.md +++ b/doc/plans/2026-10-24-recurring-server-sleep-stack.md @@ -179,3 +179,14 @@ login-cleanup timeout after all 16 local tests passed. #17453 has a Runner prepared-launch close failure. Seven focused local Runner cases also fail, so this is under investigation rather than assumed flaky. No retry of that run has been requested. Neither parent is reported green without fresh evidence. + + +Slice 05 is [#17454](https://github.com/paperclipai/paperclip/pull/17454). +Review identified a real one-shot outcome-write error path. A new regression +failed before the repair; result persistence now runs outside provider-error +handling. The repaired head passes 123 service/startup tests, 92 acceptance +tests, workspace typecheck and build. Fresh CI/review remains required. +#17450 now has passing checks after its single retry. The local Runner +reproduction for #17453 was missing the fake-provider binary: its durable state +showed ENOENT hidden by close failure. After building the fixture all seven +cases pass; one CI retry is running. diff --git a/server/src/__tests__/tool-gateway-service.test.ts b/server/src/__tests__/tool-gateway-service.test.ts index 166f8c3dbaa..0e9fe4533bf 100644 --- a/server/src/__tests__/tool-gateway-service.test.ts +++ b/server/src/__tests__/tool-gateway-service.test.ts @@ -276,6 +276,7 @@ describeEmbeddedPostgres("tool gateway service", () => { vi.useRealTimers(); vi.restoreAllMocks(); await db.execute(sql`drop trigger if exists sleep_review_fail on tool_invocations`); await db.execute(sql`drop function if exists sleep_review_fail()`); + await db.execute(sql`drop sequence if exists sleep_review_failure_attempt`); expect(errors).toEqual([]); } async function start() { @@ -461,12 +462,18 @@ describeEmbeddedPostgres("tool gateway service", () => { await quiet(); expect(remoteCall.provider).not.toHaveBeenCalled(); }); - it("does not replay a Test-tab call when persisting its external result fails", async () => { + it.each(["always", "once"])("does not replay a Test-tab call when persisting its external result fails %s", async mode => { const remoteCall = providerFixture(); remoteCall.release(); const { pending } = await testRequest(remoteCall.provider); await approve(pending); - await db.execute(sql`create function sleep_review_fail() returns trigger language plpgsql as $$ - begin raise exception 'approval result fixture failure'; end $$`); + await db.execute(sql`create sequence sleep_review_failure_attempt`); + await db.execute(sql.raw(`create function sleep_review_fail() returns trigger language plpgsql as $$ + begin + if ${mode === "always" ? "true" : "nextval('sleep_review_failure_attempt') = 1"} then + raise exception 'approval result fixture failure'; + end if; + return new; + end $$`)); await db.execute(sql`create trigger sleep_review_fail before update on tool_action_requests for each row when (new.status in ('executed', 'failed')) execute function sleep_review_fail()`); try { diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index 4846e53bdf1..41a4d8a1103 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -7452,9 +7452,11 @@ export function createToolGatewayService( }); const startedAt = Date.now(); + let connectedMcpExecution: Awaited> | Awaited> | null; + let resultValidation: Awaited>; try { const executionTimeoutMs = timeoutMs(args.timeoutMs, args.tool); - const connectedMcpExecution = + connectedMcpExecution = (args.tool.providerType === "mcp_remote_http" || args.tool.providerType === "provider_rest") ? await executeRemoteHttpTool( args.session, @@ -7485,7 +7487,7 @@ export function createToolGatewayService( ); } const result = connectedMcpExecution.result; - const resultValidation = await validateToolContent({ + resultValidation = await validateToolContent({ value: result, direction: "result", sensitiveMode: "redact", @@ -7496,58 +7498,6 @@ export function createToolGatewayService( throw new ToolGatewayHttpError(502, String(providerResult.content || providerResult.error), "tool_error", { execution: connectedMcpExecution.execution }); } - await persistToolTestOutcome(db, args.invocationId, { - status: "succeeded", result: resultValidation.summary, - }, args.approvedClaim); - await writeToolCallEvent({ - invocationId: args.invocationId, - session: args.session, - eventType: "call_completed", - outcome: "success", - toolName: args.tool.name, - policyDecision: "allow", - reasonCode: "tool_completed", - argumentsSummary: args.argumentsSummary, - resultSummary: resultValidation.summary, - detectorMetadata: resultValidation.detectorMetadata, - metadata: { - source: "test", - headerSummary: connectedMcpExecution.headerSummary ?? undefined, - execution: connectedMcpExecution.execution, - }, - tool: args.tool, - }); - await writeAudit({ - session: args.session, - companyId: args.companyId, - agentId: args.agentId, - runId: null, - issueId: null, - actorType: "user", - actorId: args.userId, - action: "tool_gateway.call_completed", - details: { - source: "test", - invocationId: args.invocationId, - decision: "allow", - reasonCode: "tool_completed", - tool: args.tool.name, - ...toolAuditMetadata(args.tool), - durationMs: Date.now() - startedAt, - argumentsSummary: args.argumentsSummary, - result: summarizeResult(resultValidation.value), - resultSummary: resultValidation.summary, - promptInjection: resultValidation.detectorMetadata, - headerSummary: connectedMcpExecution.headerSummary ?? undefined, - execution: connectedMcpExecution.execution, - }, - }); - void emitConnectionInvoked(db, args.invocationId); - return { - decision: "allowed" as const, - invocationId: args.invocationId, - result: resultValidation.value, - }; } catch (err) { const status = err instanceof ToolGatewayHttpError ? err.status : 502; const reasonCode = @@ -7619,16 +7569,61 @@ export function createToolGatewayService( : {}), }; } + await persistToolTestOutcome(db, args.invocationId, { + status: "succeeded", result: resultValidation.summary, + }, args.approvedClaim); + await writeToolCallEvent({ + invocationId: args.invocationId, + session: args.session, + eventType: "call_completed", + outcome: "success", + toolName: args.tool.name, + policyDecision: "allow", + reasonCode: "tool_completed", + argumentsSummary: args.argumentsSummary, + resultSummary: resultValidation.summary, + detectorMetadata: resultValidation.detectorMetadata, + metadata: { + source: "test", + headerSummary: connectedMcpExecution.headerSummary ?? undefined, + execution: connectedMcpExecution.execution, + }, + tool: args.tool, + }); + await writeAudit({ + session: args.session, + companyId: args.companyId, + agentId: args.agentId, + runId: null, + issueId: null, + actorType: "user", + actorId: args.userId, + action: "tool_gateway.call_completed", + details: { + source: "test", + invocationId: args.invocationId, + decision: "allow", + reasonCode: "tool_completed", + tool: args.tool.name, + ...toolAuditMetadata(args.tool), + durationMs: Date.now() - startedAt, + argumentsSummary: args.argumentsSummary, + result: summarizeResult(resultValidation.value), + resultSummary: resultValidation.summary, + promptInjection: resultValidation.detectorMetadata, + headerSummary: connectedMcpExecution.headerSummary ?? undefined, + execution: connectedMcpExecution.execution, + }, + }); + void emitConnectionInvoked(db, args.invocationId); + return { + decision: "allowed" as const, + invocationId: args.invocationId, + result: resultValidation.value, + }; + } - /** - * Drive a freshly-approved test-origin ask-first request to completion. The - * Test tab has no agent run to re-invoke the parked call, so approving it in - * the Review tab is what executes it — the live status panel then surfaces the - * real result. Reconstructs the impersonated test session and the signed - * arguments, and never throws: any failure is recorded on the invocation so it - * shows up as an error in the panel rather than rolling back the approval. - */ async function runApprovedTestInvocation( invocation: typeof toolInvocations.$inferSelect, signedPayload: NonNullable>, @@ -7636,6 +7631,7 @@ export function createToolGatewayService( ): Promise { const claim = await claimToolTestApproval(db, actionRequestId, invocation.id); if (!claim) return; + let execution: Parameters[0]; try { const agentId = invocation.agentId; if (!invocation.connectionId || !agentId) { @@ -7662,16 +7658,18 @@ export function createToolGatewayService( } const argumentsSummary = (await validateToolContent({ value: signedPayload.arguments, direction: "arguments", sensitiveMode: "redact", promptInjectionMode: "ignore" })).summary; - await runTestToolInvocation({ session, tool, parameters: signedPayload.arguments, invocationId: invocation.id, + execution = { session, tool, parameters: signedPayload.arguments, invocationId: invocation.id, companyId: invocation.companyId, agentId, userId, argumentsSummary, reasonCode: "approval_granted", - approvedClaim: claim, matchedPolicyIds: invocation.matchedPolicyIds ?? [] }); + approvedClaim: claim, matchedPolicyIds: invocation.matchedPolicyIds ?? [] }; } catch (err) { const message = err instanceof Error ? err.message : String(err); const errorCode = err instanceof ToolGatewayHttpError || err instanceof ToolContentValidationError ? err.reasonCode : "tool_execution_failed"; const settled = await persistToolTestOutcome(db, invocation.id, { status: "failed", errorCode, errorMessage: message }, claim); if (settled) void emitConnectionInvoked(db, invocation.id); + return; } + await runTestToolInvocation(execution); } async function waitForActionRequestExecution(actionRequestId: string) {