fix(services): resume a run when its approval is answered outside the playground (#5593)

api/oss/src/core/sessions/interactions/dtos.py

@@ -2,7 +2,7 @@
 from typing import Any, Dict, Optional
 from uuid import UUID
 
-from pydantic import BaseModel
+from pydantic import BaseModel, ConfigDict
 
 from oss.src.core.shared.dtos import Identifier, Lifecycle, Reference, Selector
 
@@ -21,8 +21,25 @@ class SessionInteractionStatus(str, Enum):
     cancelled = "cancelled"  # runner abandoned the gate; no one is waiting on the token
 
 
+class SessionInteractionRequest(BaseModel):
+    # The gated call this interaction is asking about.
+    #
+    # `tool_call_id` is the harness's id for the call, which the row's `token` is NOT (that is the
+    # permission gate's id). Both ride the live event stream, so the playground can answer without
+    # it; a caller building an answer from the stored row alone cannot, and names the wrong call.
+    # Optional because rows written before the field exists carry only the token.
+    #
+    # Extra keys are kept: producers other than the approval gate write their own request shapes
+    # here, and dropping what this model does not name would lose them on any round-trip.
+    model_config = ConfigDict(extra="allow")
+
+    tool: Optional[str] = None
+    args: Optional[Any] = None
+    tool_call_id: Optional[str] = None
+
+
 class SessionInteractionData(BaseModel):
-    request: Optional[Dict[str, Any]] = None
+    request: Optional[SessionInteractionRequest] = None
     references: Optional[Dict[str, Reference]] = None
     selector: Optional[Selector] = None
     resolution: Optional[Dict[str, Any]] = None

api/oss/tests/pytest/unit/sessions/test_interaction_request_tool_call_id.py

@@ -0,0 +1,72 @@
+"""The durable interaction row must carry the harness's tool-call id.
+
+A row's ``token`` is the permission gate's id, not the id of the call the agent parked on. Both
+ride the live event stream, which is why the playground can answer a gate correctly; a caller
+working from the stored row alone (an inbox, a webhook, a CLI) has only the row, so the tool-call
+id has to be stored on it. See issue #5593.
+"""
+
+from uuid import uuid4
+
+from oss.src.core.sessions.interactions.dtos import (
+    SessionInteractionCreate,
+    SessionInteractionData,
+    SessionInteractionKind,
+    SessionInteractionRequest,
+)
+from oss.src.dbs.postgres.sessions.interactions.mappings import (
+    map_interaction_dto_to_dbe_create,
+)
+
+
+def test_create_persists_the_tool_call_id_alongside_the_gate_token():
+    project_id = uuid4()
+    dbe = map_interaction_dto_to_dbe_create(
+        project_id=project_id,
+        user_id=None,
+        interaction=SessionInteractionCreate(
+            project_id=project_id,
+            session_id="sess-1",
+            turn_id="turn-1",
+            token="gate-token",
+            kind=SessionInteractionKind.user_approval,
+            data=SessionInteractionData(
+                request=SessionInteractionRequest(
+                    tool="Write",
+                    args={"file_path": "/tmp/x"},
+                    tool_call_id="toolu_01abc",
+                )
+            ),
+        ),
+    )
+
+    assert dbe.token == "gate-token"
+    assert dbe.data["request"] == {
+        "tool": "Write",
+        "args": {"file_path": "/tmp/x"},
+        "tool_call_id": "toolu_01abc",
+    }
+
+
+def test_a_row_written_before_the_field_existed_still_parses():
+    # Rows already in the table carry only tool + args. They must keep loading, and must not
+    # invent a tool-call id.
+    data = SessionInteractionData.model_validate(
+        {"request": {"tool": "Terminal", "args": {"command": "ls"}}}
+    )
+
+    assert data.request is not None
+    assert data.request.tool == "Terminal"
+    assert data.request.tool_call_id is None
+
+
+def test_unknown_request_keys_survive_the_round_trip():
+    # Other producers write their own request shapes here; naming three fields must not drop
+    # everything else on a load-then-store cycle.
+    data = SessionInteractionData.model_validate(
+        {"request": {"tool": "test_run", "args": {}, "custom": {"a": 1}}}
+    )
+
+    assert data.model_dump(mode="json", exclude_none=True)["request"]["custom"] == {
+        "a": 1
+    }

api/oss/tests/pytest/unit/sessions/test_wp5_dao_fanout.py

@@ -25,6 +25,7 @@
     SessionInteractionData,
     SessionInteractionKind,
     SessionInteractionQuery,
+    SessionInteractionRequest,
     SessionInteractionStatus,
     SessionInteractionTransition,
 )
@@ -182,7 +183,7 @@ async def test_interaction_transition_preserves_data_and_optionally_adds_resolut
     assert transitioned is not None
     assert transitioned.status == SessionInteractionStatus.resolved
     assert transitioned.data is not None
-    assert transitioned.data.request == request
+    assert transitioned.data.request == SessionInteractionRequest(**request)
     assert transitioned.data.resolution == {
         "verdict": "approved",
         "tool_call_id": "tool-1",
@@ -210,7 +211,9 @@ async def test_interaction_transition_preserves_data_and_optionally_adds_resolut
 
     assert transitioned_without_resolution is not None
     assert transitioned_without_resolution.data is not None
-    assert transitioned_without_resolution.data.request == request
+    assert transitioned_without_resolution.data.request == SessionInteractionRequest(
+        **request
+    )
     assert transitioned_without_resolution.data.resolution is None
 
 

services/runner/src/engines/sandbox_agent/acp-interactions.ts

@@ -63,12 +63,18 @@ export interface AttachPermissionResponderInput {
    * the cold path can multiplex that mixed set. An approval gate does NOT fire it.
    */
   onNonParkablePause?: () => void;
-  /** Called on pause to record the pending gate as an interaction (fire-and-forget). */
+  /**
+   * Called on pause to record the pending gate as an interaction (fire-and-forget). `toolCallId`
+   * is the harness's id for the gated call, which is NOT the interaction token (that is the
+   * permission gate's id). Both ride the live `interaction_request` event; persisting the tool-call
+   * id too is what lets a caller working from the durable row alone name the right call.
+   */
   onCreateInteraction?: (
     token: string,
     toolName: string | undefined,
     toolArgs: unknown,
     kind: "user_approval" | "client_tool",
+    toolCallId: string | undefined,
   ) => void;
   /** Called after a stored decision was successfully forwarded to the harness. */
   onResolveInteraction?: (
@@ -195,7 +201,13 @@ export function attachPermissionResponder({
       },
     });
     createdInteractionIds.add(eventId);
-    onCreateInteraction?.(eventId, gate.toolName, gate.args, "user_approval");
+    onCreateInteraction?.(
+      eventId,
+      gate.toolName,
+      gate.args,
+      "user_approval",
+      toolCallId,
+    );
     onPause?.();
   };
 
@@ -224,7 +236,13 @@ export function attachPermissionResponder({
       },
     });
     createdInteractionIds.add(eventId);
-    onCreateInteraction?.(eventId, gate.toolName, gate.args, "client_tool");
+    onCreateInteraction?.(
+      eventId,
+      gate.toolName,
+      gate.args,
+      "client_tool",
+      toolCallId,
+    );
     onPause?.();
   };
 

services/runner/src/engines/sandbox_agent/reconstruct-history.ts

@@ -8,7 +8,8 @@
  *
  * When it DOES apply it is no longer best-effort, because the client kept no copy of the
  * conversation: an unreadable log, or one known to have dropped a record, fails the turn rather
- * than letting the agent answer as though the conversation had just started.
+ * than letting the agent answer as though the conversation had just started. For an approval
+ * reply, which carries no task of its own, "nothing to rebuild" is itself such a failure.
  *
  * The record log already contains the CURRENT turn by the time this runs: the runner persists the
  * inbound user message before it starts the engine, and acquiring a sandbox takes seconds. Its
@@ -20,7 +21,10 @@ import type { AgentRunRequest } from "../../protocol.ts";
 import { fetchSessionRecords } from "../../sessions/records-query.ts";
 import { recordsIncomplete } from "../../sessions/persist.ts";
 import { reconstructMessages } from "../../sessions/reconstruct.ts";
-import { carriesMinimalHistory } from "./session-identity.ts";
+import {
+  carriesApprovalReplyOnly,
+  carriesMinimalHistory,
+} from "./session-identity.ts";
 
 // ON unless the literal "false"; absent AND empty both mean on (compose passes `${VAR:-}`,
 // which sets an empty string when the shell has no value).
@@ -40,10 +44,35 @@ export async function reconstructHistoryIfNeeded(
   auth: () => string,
   log?: (msg: string) => void,
 ): Promise<AgentRunRequest | null> {
-  if (!reconstructEnabled() || !sessionId) return null;
   const inbound = request.messages ?? [];
-  // The client still asserts the conversation itself — nothing to rebuild.
-  if (!carriesMinimalHistory(request)) return null;
+  // An approval reply carries no task of its own, only the answered gate. Returning null for it
+  // is not "keep the inbound history", it is "run with no conversation at all": `buildRunPlan`
+  // spares this shape the empty-prompt check, so the turn would go out as the approval-resume
+  // frame alone — no task, no context — and report success. A prior turn exists by construction
+  // for an approval reply, so every no-history path below is an anomaly for this shape and fails
+  // the turn instead. On a live resume `runTurn` catches this and continues on the inbound
+  // request, because the harness there still holds the conversation.
+  const approvalReplyOnly = carriesApprovalReplyOnly(request);
+  const refuse = (why: string): never => {
+    throw new Error(
+      `session ${sessionId ?? "(none)"}: cannot resume an approval reply with ` +
+        `no conversation to rebuild (${why})`,
+    );
+  };
+
+  if (!reconstructEnabled() || !sessionId) {
+    if (approvalReplyOnly) {
+      refuse(!sessionId ? "no session id" : "reconstruction is disabled");
+    }
+    return null;
+  }
+  // The client still asserts the conversation itself — nothing to rebuild. Two shapes assert
+  // nothing: a last-message-only client (a fresh user turn alone) and an out-of-band approval
+  // reply built from the durable interaction row (an approval envelope alone). Both need the
+  // prior turns rebuilt here or the agent answers as though the conversation had just started.
+  if (!carriesMinimalHistory(request) && !approvalReplyOnly) {
+    return null;
+  }
 
   // The client kept no copy of the conversation, so there is no history to fall back to. Answering
   // anyway would silently produce an agent that forgot everything, which reads as a correct reply.
@@ -66,10 +95,24 @@ export async function reconstructHistoryIfNeeded(
   const prior = currentTurnId
     ? records.filter((row) => row.turn_id !== currentTurnId)
     : records;
-  if (prior.length === 0) return null;
+  // Reachable in practice: a caller that builds its answer from the durable interaction row can
+  // echo the row's stored `turn_id`, which drops exactly the turn that parked.
+  if (prior.length === 0) {
+    if (approvalReplyOnly) {
+      refuse(
+        `the log holds no turn other than ${currentTurnId ?? "the current one"}`,
+      );
+    }
+    return null;
+  }
 
   const reconstructed = reconstructMessages(prior);
-  if (reconstructed.length === 0) return null;
+  if (reconstructed.length === 0) {
+    if (approvalReplyOnly) {
+      refuse(`${prior.length} prior record(s) rebuilt into no messages`);
+    }
+    return null;
+  }
 
   log?.(
     `[reconstruct] session=${sessionId} records=${records.length} ` +

services/runner/src/engines/sandbox_agent/run-plan.ts

@@ -28,6 +28,7 @@ import {
 } from "../skills.ts";
 import { assert } from "./capabilities.ts";
 import type { ClientToolPauseDisposition } from "./client-tools.ts";
+import { carriesApprovalReplyOnly } from "./session-identity.ts";
 import { buildTurnText } from "./transcript.ts";
 import {
   KNOWN_SANDBOX_PROVIDER_IDS,
@@ -311,7 +312,12 @@ export function buildRunPlan(
   );
 
   const prompt = resolvePromptText(request);
-  if (!prompt) {
+  // An out-of-band approval reply legitimately carries no user text: the human answered a parked
+  // gate from the durable interaction row, not from the conversation. Its prior turns are rebuilt
+  // from the record log inside `runTurn` (`reconstructHistoryIfNeeded`), which runs AFTER this
+  // plan is built — so rejecting here would kill the run before the conversation could be
+  // supplied. Every other empty-prompt request is still a caller bug and fails loudly.
+  if (!prompt && !carriesApprovalReplyOnly(request)) {
     return {
       ok: false,
       error: "No user message to send (prompt/messages empty).",

services/runner/src/engines/sandbox_agent/run-turn.ts

@@ -77,6 +77,7 @@ import {
   sessionContinuityStore,
 } from "./session-continuity.ts";
 import { reconstructHistoryIfNeeded } from "./reconstruct-history.ts";
+import { carriesApprovalReplyOnly } from "./session-identity.ts";
 import { buildTurnText, priorMessages } from "./transcript.ts";
 import { resolveRunUsage } from "./usage.ts";
 
@@ -155,20 +156,45 @@ export async function runTurn(
     // record log so a cold turn still has full context. Runs before the current user turn is
     // persisted, so records hold only prior turns. Reassigns `request` so every downstream
     // reader (turnText, priorMessages, responder, otel) sees the same reconstructed history.
-    const reconstructed = await reconstructHistoryIfNeeded(
-      request,
-      sessionId,
-      () => runCredential(request),
-      logger,
-    );
+    // An out-of-band approval reply carries no user text of its own, so this must be decided from
+    // the INBOUND request: reconstruction prepends the original user turn, after which
+    // `resolvePromptText` would hand back that stale command and the model would restart the task.
+    const approvalReplyOnly = carriesApprovalReplyOnly(request);
+    const inboundRequest = request;
+    // On a LIVE approval resume the rebuilt history is never sent to the harness: the resume
+    // continues the ORIGINAL prompt promise (`opts.resume` below), so `turnText` is discarded and
+    // the harness already holds the conversation. Reconstruction there only enriches the trace and
+    // the responder's view, so a failed records fetch must NOT fail the turn: `{ok:false}` makes
+    // the dispatch evict the live session and retry cold, where reconstruction throws again — one
+    // 500 from the records endpoint would lose the human's approval AND the parked session. Keep
+    // the inbound request and continue. A cold turn still fails loudly, where it matters.
+    let reconstructed: AgentRunRequest | null = null;
+    try {
+      reconstructed = await reconstructHistoryIfNeeded(
+        request,
+        sessionId,
+        () => runCredential(request),
+        logger,
+      );
+    } catch (err) {
+      if (!opts.resume) throw err;
+      const detail = err instanceof Error ? err.message : String(err);
+      logger(
+        `[reconstruct] live resume: keeping the inbound history (${detail})`,
+      );
+    }
     if (reconstructed) request = reconstructed;
 
     const promptText = resolvePromptText(request);
     // Cold: replay the full transcript. Continuation or loaded: send only new text. When history
     // was rebuilt from records, recompute the transcript from it — the prebuilt plan.turnText
-    // predates the reconstruction.
+    // predates the reconstruction. An approval reply has no new text either way, so it sends the
+    // approval-resume frame `buildTurnText` renders (the harness already holds the prior turns
+    // when the session was loaded natively; otherwise the rebuilt transcript comes with it).
     const turnText = sendLastMessageOnly(opts)
-      ? promptText
+      ? approvalReplyOnly
+        ? buildTurnText(inboundRequest, logger)
+        : promptText
       : reconstructed
         ? buildTurnText(request, logger)
         : plan.turnText;
@@ -458,6 +484,7 @@ export async function runTurn(
       toolName: string | undefined,
       toolArgs: unknown,
       kind: "user_approval" | "client_tool" = "user_approval",
+      toolCallId?: string,
     ): void => {
       const cred = runCredential(request);
       if (!cred) return;
@@ -468,7 +495,16 @@ export async function runTurn(
         request.turnId ?? "",
         token,
         kind,
-        { request: { tool: toolName ?? token, args: toolArgs }, references },
+        {
+          request: {
+            tool: toolName ?? token,
+            args: toolArgs,
+            // The gate id (`token`) and the harness's tool-call id differ; an out-of-band answer
+            // needs the latter to name the call it is answering.
+            ...(toolCallId ? { tool_call_id: toolCallId } : {}),
+          },
+          references,
+        },
         () => cred,
       );
     };