From 1cdb26a68b244468c8e1caf8ac25223db891acd2 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 24 Oct 2026 10:47:01 -0700 Subject: [PATCH 1/2] fix(server): separate tool outcome persistence from provider errors Co-Authored-By: Paperclip --- doc/idle-approved-tool-recovery.md | 10 +- ...2026-10-24-recurring-server-sleep-stack.md | 11 ++ .../__tests__/tool-gateway-service.test.ts | 13 +- server/src/services/tool-gateway.ts | 126 +++++++++--------- 4 files changed, 92 insertions(+), 68 deletions(-) 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) { From 7a1cf03e6270d658a41ec332bfa4c36c8a0ae8b0 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 24 Oct 2026 10:49:28 -0700 Subject: [PATCH 2/2] test(server): require settled outcomes for approved Test-tab calls Co-Authored-By: Paperclip --- doc/idle-approved-tool-recovery.md | 6 ++++++ doc/plans/2026-10-24-recurring-server-sleep-stack.md | 10 +++++++++- server/src/__tests__/tool-access-service.test.ts | 8 +++++--- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/doc/idle-approved-tool-recovery.md b/doc/idle-approved-tool-recovery.md index 4f33170558f..9d8f28d7ae8 100644 --- a/doc/idle-approved-tool-recovery.md +++ b/doc/idle-approved-tool-recovery.md @@ -88,3 +88,9 @@ 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. + +The existing tool-access service regression now expects a completed Test-tab +request to be executed and verifies exactly one provider call before asking for +a new approval. Its full 469-test suite passes. The initial local command skipped +its database cases; the suite now honors its supplied disposable database through +the standard test-database guard independently of embedded Postgres support. 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 d96322303a6..ad4e1e3f1be 100644 --- a/doc/plans/2026-10-24-recurring-server-sleep-stack.md +++ b/doc/plans/2026-10-24-recurring-server-sleep-stack.md @@ -18,7 +18,7 @@ Do not mark an item done from a timer removal or a green check alone. | 04 | Decision retention and notifications | Retention deadlines and notification delivery are separate reasons | Pending | | 05 | Decision expiry and interrupted effects | Expiry, effect recovery, continuation recovery | Pending | | 06 | Tool connection health | Configuration/use activation and defined proactive health deadlines | Pending | -| 07 | Secret proposals, tool reviews, gateway tokens | Separate expiry/scrub/recovery owners | Secret proposals: [#17446](https://github.com/paperclipai/paperclip/pull/17446), checks passed at `d41590ac0e`; gateway credentials: [#17450](https://github.com/paperclipai/paperclip/pull/17450), Greptile 5/5 at `c5c53fc43f`, fresh CI running; pending tool-review expiry: [#17453](https://github.com/paperclipai/paperclip/pull/17453), local verification passed and CI/review running; other review duties pending | +| 07 | Secret proposals, tool reviews, gateway tokens | Separate expiry/scrub/recovery owners | Secret proposals: [#17446](https://github.com/paperclipai/paperclip/pull/17446), checks passed at `d41590ac0e`; gateway credentials: [#17450](https://github.com/paperclipai/paperclip/pull/17450), all checks passed, Greptile 5/5 at `c5c53fc43f`; pending tool-review expiry: [#17453](https://github.com/paperclipai/paperclip/pull/17453), all checks passed, Greptile 5/5 at `ce6a260a83`; approved recovery: [#17454](https://github.com/paperclipai/paperclip/pull/17454), review repair pushed; other review duties pending | | 08 | GitHub connection events and continuity | Event discovery/replay and credential continuity separately | Pending | | 09 | Merged-PR confirmations | Candidate writes, webhook activation where supported, outstanding-candidate fallback | Pending | | 10 | External-object refresh | Configuration changes, refresh deadlines, source events | Pending | @@ -190,3 +190,11 @@ tests, workspace typecheck and build. Fresh CI/review remains required. 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. + +#17453 now has passing checks after its single Runner retry. #17454's initial +server shard found an old expected approved state for a completed Test-tab call; +the updated regression requires executed and one provider call. All 469 +tool-access service tests pass after enabling the existing external database +path independently of embedded support. Its initial browser shard failed during +unrelated plan-task fixture company deletion with a concurrent activity-log FK +violation. Inspect fresh-head CI before deciding whether that needs a repair. diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index 38c49754c4b..88c936be756 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -1,3 +1,4 @@ +import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; import * as fs from "node:fs/promises"; import { createHash, generateKeyPairSync, randomUUID } from "node:crypto"; import { createServer } from "node:http"; @@ -97,8 +98,8 @@ import { HttpError } from "../errors.js"; import type { VercelConnectClient } from "../services/vercel-connect.js"; import { invalidatePaperclipCloudConnectorCapabilities, PaperclipCloudConnectorError, type PaperclipCloudConnector } from "../services/paperclip-cloud-connector.js"; -const externalDatabaseUrl = process.env.PAPERCLIP_TOOL_ACCESS_TEST_DATABASE_URL?.trim(); -const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const externalDatabaseUrl = resolveExternalTestDatabaseUrl("PAPERCLIP_TOOL_ACCESS_TEST_DATABASE_URL"); +const embeddedPostgresSupport = externalDatabaseUrl ? { supported: true } : await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; @@ -4111,8 +4112,9 @@ describeEmbeddedPostgres("tool access service", () => { .from(toolActionRequests) .where(eq(toolActionRequests.companyId, company.id)); expect(requests).toHaveLength(2); + expect(globalThis.fetch).toHaveBeenCalledTimes(1); expect(requests.map((row) => row.status).sort()).toEqual([ - "approved", + "executed", "pending", ]); });