fix(acp): address Thufir pass-4 review findings

- Every ask terminal routes through finish_permission(); applied path stops on false (poison) immediately instead of looping back. Cancel path uses None idle sentinel instead of dummy instant. - Deadline equality: process expired entries first at entry.deadline == hard_deadline, then return HardTimeout — fail-closed response is always written before exit. - Wire-truth tests: production-path, cancel, and nine-request tests now capture child stdin NDJSON and assert parsed exact lines/ids. Temporal tests rebuilt around one continuously running loop per scenario. - Desktop nonce-present = nonce-only: unknown nonce drops the frame without falling back to the id map. Legacy fallback keyed by compound (channel:session:turn:id), never bare id. Both indexes cleaned on every terminal (acp_write, permission_terminal) and backstop (turn_completed, turn_error). New tests: FOREIGN-nonce drop + cleanup assertions on both indexes for all four terminal paths. - NIP-AO: permission_terminal in frame-kind table; synchronous policy outcomes (rejected/allowed/allow_failed_closed) in reason table with explanatory note distinguishing ask vs. synchronous paths. - Desktop: permission_terminal handler uses pinned uncertain copy; tests for live replay and lifecycle-only archive replay. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
2026-08-18 06:50:31 +02:00 · 2026-08-07 12:25:10 -04:00
parent 6607eaf55e
commit 7d277dab4d
5 changed files with 1234 additions and 362 deletions
@@ -73,7 +73,7 @@ fn new_observer_handle() -> ObserverHandle {
 }

 /// Event delivered through the in-process observer bus.
-#[derive(Clone, Serialize)]
+#[derive(Clone, Debug, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct ObserverEvent {
    /// Monotonic process-local sequence number.
@@ -2392,3 +2392,337 @@ test("buildTranscript_control_result_sent_does_not_mark_delivery_failed", () =>
    "deliveryFailed must not be set on sent control_result",
  );
 });
+
+// ─── permission index cleanup + FOREIGN-nonce tests (Pass 4) ─────────────────
+
+import { buildTranscriptState } from "./agentSessionTranscript.ts";
+
+function makePermissionWriteWithNonce(
+  seq,
+  requestId,
+  nonce,
+  outcome = "selected",
+  optionId = "allow_once",
+  { channelId = "ch-1", sessionId = "session-1", turnId = "turn-1" } = {},
+) {
+  const resultOutcome =
+    outcome === "selected" ? { outcome: "selected", optionId } : { outcome };
+  return {
+    seq,
+    timestamp: "2026-07-01T10:00:01.000Z",
+    kind: "acp_write",
+    agentIndex: 0,
+    channelId,
+    sessionId,
+    turnId,
+    payload: {
+      jsonrpc: "2.0",
+      id: requestId,
+      result: { outcome: resultOutcome },
+    },
+    authorization: {
+      requestNonce: nonce,
+      actionable: false,
+      reason: "applied",
+    },
+  };
+}
+
+function makePermissionTerminalEvent(
+  seq,
+  requestId,
+  nonce,
+  { channelId = "ch-1", sessionId = "session-1", turnId = "turn-1" } = {},
+) {
+  return {
+    seq,
+    timestamp: "2026-07-01T10:00:02.000Z",
+    kind: "permission_terminal",
+    agentIndex: 0,
+    channelId,
+    sessionId,
+    turnId,
+    payload: { id: requestId },
+    authorization: {
+      requestNonce: nonce,
+      actionable: false,
+      reason: "uncertain",
+    },
+  };
+}
+
+function makeTurnCompleted(
+  seq,
+  { channelId = "ch-1", sessionId = "session-1", turnId = "turn-1" } = {},
+) {
+  return {
+    seq,
+    timestamp: "2026-07-01T10:00:05.000Z",
+    kind: "turn_completed",
+    agentIndex: 0,
+    channelId,
+    sessionId,
+    turnId,
+    payload: {},
+  };
+}
+
+function makeTurnError(
+  seq,
+  { channelId = "ch-1", sessionId = "session-1", turnId = "turn-1" } = {},
+) {
+  return {
+    seq,
+    timestamp: "2026-07-01T10:00:05.000Z",
+    kind: "turn_error",
+    agentIndex: 0,
+    channelId,
+    sessionId,
+    turnId,
+    payload: { message: "process died" },
+  };
+}
+
+// ─── FOREIGN-nonce: unknown nonce is dropped, wrong card not mutated ─────────
+
+test("buildTranscript_foreign_nonce_acp_write_does_not_mutate_any_card", () => {
+  // Register card A with nonce-A. Send an acp_write with nonce-FOREIGN
+  // (not in the index). The response must be silently dropped — card A
+  // must remain actionable and have no outcome appended.
+  const events = [
+    makePermissionRequestWithAuth(1, "req-a", "nonce-A"),
+    makePermissionWriteWithNonce(
+      2,
+      "req-a",
+      "nonce-FOREIGN",
+      "selected",
+      "allow_once",
+    ),
+  ];
+  const state = buildTranscriptState(events);
+  const transcript = state.items;
+
+  assert.equal(transcript.length, 1, "only one card must exist");
+  const card = transcript[0];
+  assert.equal(card.renderClass, "permission");
+  assert.equal(card.requestNonce, "nonce-A");
+  assert.equal(
+    card.actionable,
+    true,
+    "card A must remain actionable — FOREIGN nonce must not retire it",
+  );
+  assert.equal(
+    card.outcome,
+    undefined,
+    "no outcome must be appended — FOREIGN nonce write must be dropped",
+  );
+
+  // The nonce index must still contain nonce-A (FOREIGN was silently dropped).
+  assert.ok(
+    state.pendingPermissionsByNonce.has("nonce-A"),
+    "nonce-A must remain in the index after FOREIGN write is dropped",
+  );
+  assert.ok(
+    !state.pendingPermissionsByNonce.has("nonce-FOREIGN"),
+    "nonce-FOREIGN must never appear in the index",
+  );
+});
+
+test("buildTranscript_foreign_nonce_does_not_resolve_other_card_by_id", () => {
+  // card-1 (nonce-X) and card-2 (nonce-Y) are registered.
+  // An acp_write arrives with the id of card-1 but carries nonce-FOREIGN.
+  // Neither card must be mutated (nonce-FOREIGN lookup fails → drop).
+  const events = [
+    makePermissionRequestWithAuth(1, "req-x", "nonce-X"),
+    makePermissionRequestWithAuth(2, "req-x", "nonce-Y", { turnId: "turn-2" }),
+    // Same wire id as req-x but an unknown nonce → must be dropped entirely.
+    makePermissionWriteWithNonce(
+      3,
+      "req-x",
+      "nonce-FOREIGN",
+      "selected",
+      "allow_once",
+    ),
+  ];
+  const state = buildTranscriptState(events);
+  const cards = state.items.filter((i) => i.renderClass === "permission");
+
+  assert.equal(cards.length, 2, "both permission cards must exist");
+  for (const card of cards) {
+    assert.equal(
+      card.actionable,
+      true,
+      `card ${card.requestNonce} must remain actionable — FOREIGN nonce write must not touch it`,
+    );
+    assert.equal(
+      card.outcome,
+      undefined,
+      "no outcome must be set by a FOREIGN nonce write",
+    );
+  }
+});
+
+// ─── Index cleanup: both indexes cleared on acp_write terminal ────────────────
+
+test("buildTranscript_acp_write_terminal_clears_both_indexes", () => {
+  // After a known-nonce acp_write outcome, both pendingPermissions (legacy key)
+  // and pendingPermissionsByNonce must be cleared for that entry.
+  const events = [
+    makePermissionRequestWithAuth(1, "req-b", "nonce-B"),
+    makePermissionWriteWithNonce(
+      2,
+      "req-b",
+      "nonce-B",
+      "selected",
+      "allow_once",
+    ),
+  ];
+  const state = buildTranscriptState(events);
+
+  assert.ok(
+    !state.pendingPermissionsByNonce.has("nonce-B"),
+    "pendingPermissionsByNonce must be cleared after nonce-B acp_write terminal",
+  );
+  // Legacy key: JSON-encoded requestId scoped by channel:session:turn:id.
+  const legacyKey = `ch-1:session-1:turn-1:${JSON.stringify("req-b")}`;
+  assert.ok(
+    !state.pendingPermissions.has(legacyKey),
+    "pendingPermissions legacy key must be cleared after acp_write terminal",
+  );
+  // Card outcome must be set.
+  const card = state.items[0];
+  assert.ok(card.outcome, "card must have an outcome after acp_write terminal");
+  assert.equal(card.actionable, false);
+});
+
+// ─── Index cleanup: permission_terminal clears both indexes ───────────────────
+
+test("buildTranscript_permission_terminal_clears_both_indexes", () => {
+  // After a permission_terminal event, both indexes must be cleared for that nonce.
+  const events = [
+    makePermissionRequestWithAuth(1, "req-pt", "nonce-PT"),
+    makePermissionTerminalEvent(2, "req-pt", "nonce-PT"),
+  ];
+  const state = buildTranscriptState(events);
+
+  assert.ok(
+    !state.pendingPermissionsByNonce.has("nonce-PT"),
+    "pendingPermissionsByNonce must be cleared by permission_terminal",
+  );
+  const legacyKey = `ch-1:session-1:turn-1:${JSON.stringify("req-pt")}`;
+  assert.ok(
+    !state.pendingPermissions.has(legacyKey),
+    "pendingPermissions legacy key must be cleared by permission_terminal",
+  );
+});
+
+// ─── Index cleanup: turn_completed backstop clears both indexes ───────────────
+
+test("buildTranscript_turn_completed_backstop_clears_both_indexes", () => {
+  // A turn_completed event must clear any remaining live permission entries
+  // in both indexes (the backstop for cards not yet retired by their terminal).
+  const events = [
+    makePermissionRequestWithAuth(1, "req-tc", "nonce-TC"),
+    makeTurnCompleted(2),
+  ];
+  const state = buildTranscriptState(events);
+
+  assert.ok(
+    !state.pendingPermissionsByNonce.has("nonce-TC"),
+    "pendingPermissionsByNonce must be cleared by turn_completed backstop",
+  );
+  const legacyKey = `ch-1:session-1:turn-1:${JSON.stringify("req-tc")}`;
+  assert.ok(
+    !state.pendingPermissions.has(legacyKey),
+    "pendingPermissions legacy key must be cleared by turn_completed backstop",
+  );
+  // Card must be retired (not actionable).
+  const card = state.items.find(
+    (i) => i.renderClass === "permission" && i.requestNonce === "nonce-TC",
+  );
+  assert.ok(card, "permission card must still exist after turn_completed");
+  assert.equal(
+    card.actionable,
+    false,
+    "card must be non-actionable after turn_completed backstop",
+  );
+});
+
+test("buildTranscript_turn_error_backstop_clears_both_indexes", () => {
+  // Same as turn_completed: a turn_error must also clear both indexes.
+  const events = [
+    makePermissionRequestWithAuth(1, "req-te", "nonce-TE"),
+    makeTurnError(2),
+  ];
+  const state = buildTranscriptState(events);
+
+  assert.ok(
+    !state.pendingPermissionsByNonce.has("nonce-TE"),
+    "pendingPermissionsByNonce must be cleared by turn_error backstop",
+  );
+  const legacyKey = `ch-1:session-1:turn-1:${JSON.stringify("req-te")}`;
+  assert.ok(
+    !state.pendingPermissions.has(legacyKey),
+    "pendingPermissions legacy key must be cleared by turn_error backstop",
+  );
+});
+
+// ─── permission_terminal live replay + archive replay ────────────────────────
+
+test("buildTranscript_permission_terminal_retires_card_with_pinned_uncertain_copy", () => {
+  // permission_terminal must retire the card with the verbatim pinned
+  // uncertain copy, NOT "denied" or "failed closed".
+  const events = [
+    makePermissionRequestWithAuth(1, "req-live", "nonce-LIVE"),
+    makePermissionTerminalEvent(2, "req-live", "nonce-LIVE"),
+  ];
+  const transcript = buildTranscript(events);
+
+  assert.equal(transcript.length, 1);
+  const card = transcript[0];
+  assert.equal(card.renderClass, "permission");
+  assert.equal(
+    card.actionable,
+    false,
+    "card must be non-actionable after permission_terminal",
+  );
+  assert.match(
+    card.outcome ?? "",
+    /Approval outcome unknown.*agent process stopped/i,
+    "permission_terminal must use the pinned uncertain copy",
+  );
+  assert.doesNotMatch(card.outcome ?? "", /denied/i);
+  assert.doesNotMatch(card.outcome ?? "", /failed closed/i);
+});
+
+test("buildTranscript_permission_terminal_in_archive_replay_retires_card", () => {
+  // In an archive (lifecycle-only) replay the card must be retired by
+  // permission_terminal. The sequence of events is the same as live replay;
+  // what changes is the assertion that the card is retired even with no
+  // subsequent acp_write.
+  const events = [
+    makePermissionRequestWithAuth(1, "req-arc", "nonce-ARC"),
+    makePermissionTerminalEvent(2, "req-arc", "nonce-ARC"),
+  ];
+  const state = buildTranscriptState(events);
+  const card = state.items.find(
+    (i) => i.renderClass === "permission" && i.requestNonce === "nonce-ARC",
+  );
+
+  assert.ok(card, "permission card must exist in archive replay");
+  assert.equal(
+    card.actionable,
+    false,
+    "card must be non-actionable after permission_terminal in archive replay",
+  );
+  assert.match(
+    card.outcome ?? "",
+    /Approval outcome unknown/i,
+    "archive replay permission_terminal must set the uncertain outcome copy",
+  );
+  // Both indexes must be clean.
+  assert.ok(
+    !state.pendingPermissionsByNonce.has("nonce-ARC"),
+    "nonce index must be clean after archive replay",
+  );
+});
@@ -349,6 +349,20 @@ function retireAllLivePermissionCards(d: TranscriptDraft, channelId: string) {
      }
    }
  }
+  // Clean up all pendingPermissions entries scoped to this channel.
+  // Keys use the compound format `ch:session:turn:id` — drop any that start
+  // with the channel prefix.
+  const chPrefix = `${channelId}:`;
+  let permsMutated = false;
+  for (const key of d.pendingPermissions.keys()) {
+    if (key.startsWith(chPrefix)) {
+      if (!permsMutated) {
+        d.pendingPermissions = new Map(d.pendingPermissions);
+        permsMutated = true;
+      }
+      d.pendingPermissions.delete(key);
+    }
+  }
 }

 /**
@@ -911,12 +925,22 @@ export function processTranscriptEvent(
        if (existing?.type === "lifecycle") {
          replaceItem(d, itemId, {
            ...existing,
-            outcome: "Uncertain (process restarting)",
+            outcome:
+              "Approval outcome unknown; agent process stopped before it could continue.",
            actionable: false,
          });
        }
        d.pendingPermissionsByNonce = new Map(d.pendingPermissionsByNonce);
        d.pendingPermissionsByNonce.delete(nonce);
+        // Clean up any matching compound legacy entry.
+        const responseId = jsonRpcId(asRecord(event.payload).id);
+        if (responseId) {
+          const legacyKey = `${ch}:${ctx.sessionId ?? ""}:${ctx.turnId ?? ""}:${responseId}`;
+          if (d.pendingPermissions.has(legacyKey)) {
+            d.pendingPermissions = new Map(d.pendingPermissions);
+            d.pendingPermissions.delete(legacyKey);
+          }
+        }
      }
    }
  } else if (event.kind === "acp_read" || event.kind === "acp_write") {
@@ -963,12 +987,14 @@ export function processTranscriptEvent(
        d.pendingPermissionsByNonce.set(auth.requestNonce, itemId);
      }

-      // Index by JSON-RPC id so the response (acp_write with result.outcome,
-      // no method) can correlate by id rather than by turn/seq.
+      // Legacy id index: keyed by compound (channel, session, turn, id) to
+      // prevent cross-channel / cross-session JSON-RPC id collisions.
+      // Only used by authorized frames that carry NO nonce (non-ask paths).
      const requestId = jsonRpcId(payload.id);
      if (requestId) {
+        const legacyKey = `${ch}:${ctx.sessionId ?? ""}:${ctx.turnId ?? ""}:${requestId}`;
        d.pendingPermissions = new Map(d.pendingPermissions);
-        d.pendingPermissions.set(requestId, {
+        d.pendingPermissions.set(legacyKey, {
          itemId,
          optionNames: request.optionNames,
        });
@@ -976,10 +1002,12 @@ export function processTranscriptEvent(
    } else if (event.kind === "acp_write" && !method) {
      // Permission response: {"id": <same as request>, "result": {"outcome": {...}}}
      //
-      // Primary correlation: by `authorization.requestNonce` — a nonce-keyed
-      // lookup is immune to JSON-RPC id reuse across channels/sessions.
-      // Legacy fallback: by JSON-RPC id, scoped to channel `ch` so at least
-      // cross-channel collisions are avoided.
+      // Nonce-keyed correlation is primary and exclusive:
+      //   - If the frame carries a nonce, we look it up in pendingPermissionsByNonce.
+      //     If the nonce is present but unknown (stale/foreign), we DROP the frame —
+      //     we never fall back to the id map, which could resolve the wrong card.
+      //   - If the frame carries NO nonce, we fall back to the legacy compound-key
+      //     id map (channel+session+turn+id) for non-ask synchronized outcomes.
      const auth = event.authorization;
      const nonce = auth?.requestNonce;
      const responseId = jsonRpcId(payload.id);
@@ -991,56 +1019,58 @@ export function processTranscriptEvent(
      // outcome kind directly (which says "reject_once", not "Timed out").
      const terminalReason = auth?.reason;

-      // Resolve the permission card: nonce-keyed wins; fall back to id-keyed.
-      const itemIdByNonce = nonce
-        ? d.pendingPermissionsByNonce.get(nonce)
-        : null;
-      const pendingById = responseId
-        ? d.pendingPermissions.get(responseId)
-        : null;
-
-      if (itemIdByNonce) {
-        // Nonce-correlated path: resolve the card and derive copy from reason.
-        const existing = d.itemsById.get(itemIdByNonce);
-        if (existing?.type === "lifecycle") {
-          const outcomeText = describePermissionTerminalReason(
-            terminalReason,
-            outcomeKind,
-            asString(result.optionId) ?? null,
-            existing.options,
-          );
-          replaceItem(d, itemIdByNonce, {
-            ...existing,
-            outcome: outcomeText,
-            actionable: false,
-          });
-        }
-        // Clean up both indexes.
-        if (nonce) {
+      if (nonce !== undefined && nonce !== null) {
+        // Nonce present: nonce-only path. Do NOT fall back on unknown nonce.
+        const itemIdByNonce = d.pendingPermissionsByNonce.get(nonce);
+        if (itemIdByNonce) {
+          const existing = d.itemsById.get(itemIdByNonce);
+          if (existing?.type === "lifecycle") {
+            const outcomeText = describePermissionTerminalReason(
+              terminalReason,
+              outcomeKind,
+              asString(result.optionId) ?? null,
+              existing.options,
+            );
+            replaceItem(d, itemIdByNonce, {
+              ...existing,
+              outcome: outcomeText,
+              actionable: false,
+            });
+          }
+          // Clean up nonce index.
          d.pendingPermissionsByNonce = new Map(d.pendingPermissionsByNonce);
          d.pendingPermissionsByNonce.delete(nonce);
+          // Clean up compound legacy key if it matches.
+          if (responseId) {
+            const legacyKey = `${ch}:${ctx.sessionId ?? ""}:${ctx.turnId ?? ""}:${responseId}`;
+            if (d.pendingPermissions.has(legacyKey)) {
+              d.pendingPermissions = new Map(d.pendingPermissions);
+              d.pendingPermissions.delete(legacyKey);
+            }
+          }
        }
-        if (responseId) {
+        // Unknown nonce: drop frame — do not mutate any card.
+      } else if (outcomeKind && responseId) {
+        // No nonce: legacy compound-key fallback for non-ask paths.
+        const legacyKey = `${ch}:${ctx.sessionId ?? ""}:${ctx.turnId ?? ""}:${responseId}`;
+        const pendingById = d.pendingPermissions.get(legacyKey);
+        if (pendingById) {
+          const optionId = asString(result.optionId) ?? null;
+          const outcomeText = describePermissionOutcome(
+            outcomeKind,
+            optionId,
+            pendingById.optionNames,
+          );
+          const existing = d.itemsById.get(pendingById.itemId);
+          if (existing?.type === "lifecycle") {
+            replaceItem(d, pendingById.itemId, {
+              ...existing,
+              outcome: outcomeText,
+              actionable: false,
+            });
+          }
          d.pendingPermissions = new Map(d.pendingPermissions);
-          d.pendingPermissions.delete(responseId);
-        }
-      } else if (pendingById && outcomeKind && responseId) {
-        // Legacy id-correlation fallback (non-ask paths with no nonce).
-        const optionId = asString(result.optionId) ?? null;
-        const outcomeText = describePermissionOutcome(
-          outcomeKind,
-          optionId,
-          pendingById.optionNames,
-        );
-        const existing = d.itemsById.get(pendingById.itemId);
-        if (existing?.type === "lifecycle") {
-          replaceItem(d, pendingById.itemId, {
-            ...existing,
-            outcome: outcomeText,
-            actionable: false,
-          });
-          d.pendingPermissions = new Map(d.pendingPermissions);
-          d.pendingPermissions.delete(responseId);
+          d.pendingPermissions.delete(legacyKey);
        }
      }
    } else if (event.kind === "acp_write" && method === "session/prompt") {
@@ -124,12 +124,22 @@ below). It is omitted on all other frame kinds.
 | `turn_completed`   | Terminal lifecycle — emitted when a turn ends (success, cancel, or timeout)                  |
 | `turn_error`       | Terminal lifecycle — emitted when a turn ends with an error or process death                  |
 | `control_result`   | Acknowledgement telemetry emitted after processing a control frame |
+| `permission_terminal` | Observer-only terminal for uncertain permission outcomes (process poison or cancel-during-write). No ACP wire response was confirmed. Carries an `authorization` envelope with `reason = "uncertain"`. Desktop uses this to retire the card without a JSON-RPC response. |

 Permission `acp_read` frames (carrying `session/request_permission` calls) always
 include an `authorization` envelope. The corresponding `acp_write` (the harness
 response) also includes an `authorization` envelope correlated by the same nonce —
 this pairs the challenge and answer in the observer log.

+Synchronous policy outcomes (`reject`, `allow`, preflight denial) also produce
+`acp_write` frames with `authorization` envelopes. Their `reason` values are:
+
+| Policy path | `reason` |
+|-------------|----------|
+| `reject` policy, preflight denial, ask-unavailable downgrade | `"rejected"` |
+| `allow` policy (auto-approval succeeded) | `"allowed"` |
+| `allow` policy (fail-closed, no unique allow_once option) | `"allow_failed_closed"` |
+
 **One-write / one-observe contract.** Each pending permission entry produces at most
 one ACP wire write and at most one authorized `acp_write` observer event. The write
 and the observer event are always emitted together; if the write fails the observer
@@ -165,10 +175,16 @@ call, the `ObserverEvent` carries an `authorization` field:
  | `"applied"` | Owner decision was received and written to the agent pipe. |
  | `"timed_out"` | No decision arrived before the 300-second per-request deadline; request failed closed (denial). |
  | `"cancelled"` | The turn was cancelled while the request was pending; request failed closed (denial). |
+  | `"rejected"` | `reject` policy, preflight denial, or ask-unavailable downgrade; request denied synchronously without an actionable card. |
+  | `"allowed"` | `allow` policy auto-approval succeeded; request granted synchronously. |
+  | `"allow_failed_closed"` | `allow` policy but no unique `allow_once` option available; request denied synchronously. |

-  The `uncertain` terminal (cancel arriving while the write is in flight) does NOT
-  produce an `acp_write` observer event — instead the harness emits a
-  `permission_terminal` observer event with `authorization.reason = "uncertain"` so
+  `"rejected"`, `"allowed"`, and `"allow_failed_closed"` are emitted on `acp_write` frames
+  for synchronous policy paths (see [Synchronous policy outcomes](#synchronous-policy-outcomes)).
+  They are NOT emitted for `ask`-policy pending-map entries.
+
+  The `uncertain` outcome does NOT produce an `acp_write` observer event — instead the
+  harness emits a `permission_terminal` observer event with `authorization.reason = "uncertain"` so
  Desktop clients can retire the card without an ACP wire response. The process is
  irrecoverably poisoned and will be respawned by the pool. Desktop clients MUST NOT
  expect an `acp_write` for every `acp_read` they receive; the corresponding