Merge approved-action review fixes into interrupted recovery

Co-Authored-By: Paperclip <noreply@paperclip.ing>

doc/idle-approved-tool-recovery.md

@@ -75,8 +75,22 @@ 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.
+
+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.

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 |
@@ -179,3 +179,22 @@ 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.
+
+#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.

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",
     ]);
   });

server/src/__tests__/tool-gateway-service.test.ts

@@ -278,6 +278,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() {
@@ -614,12 +615,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 {

server/src/services/tool-gateway.ts

@@ -7452,9 +7452,11 @@ export function createToolGatewayService(
     });
 
     const startedAt = Date.now();
+    let connectedMcpExecution: Awaited<ReturnType<typeof executeRemoteHttpTool>> | Awaited<ReturnType<typeof executeLocalStdioTool>> | null;
+    let resultValidation: Awaited<ReturnType<typeof validateToolContent>>;
     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<ReturnType<typeof readSignedToolArgumentsPayload>>,
@@ -7636,6 +7631,7 @@ export function createToolGatewayService(
   ): Promise<void> {
     const claim = await claimToolTestApproval(db, actionRequestId, invocation.id);
     if (!claim) return;
+    let execution: Parameters<typeof runTestToolInvocation>[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) {