docs: document exec approval replay contracts

steipete · steipete · commit 896def06ad3a · 2026-06-02T07:30:47.000-04:00
diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts
@@ -60,6 +60,7 @@ export type ExecApprovalIdLookupResult =
 export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
   private pending = new Map<string, PendingEntry<TPayload>>();
 
+  /** Builds an approval record without registering it, so callers can publish first. */
   create(request: TPayload, timeoutMs: number, id?: string | null): ExecApprovalRecord<TPayload> {
     const now = Date.now();
     const resolvedTimeoutMs = resolveApprovalTimeoutMs(timeoutMs);
@@ -101,7 +102,8 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
       resolvePromise = resolve;
       rejectPromise = reject;
     });
-    // Create entry first so we can capture it in the closure (not re-fetch from map)
+    // Capture the entry object in timeout/cleanup closures; map lookups can see
+    // a newer record with the same id during the resolved-entry grace window.
     const entry: PendingEntry<TPayload> = {
       record,
       resolve: resolvePromise!,
@@ -132,16 +134,17 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
     if (!pending) {
       return false;
     }
-    // Prevent double-resolve (e.g., if called after timeout already resolved)
     if (pending.record.resolvedAtMs !== undefined) {
+      // Timeout and operator resolution can race; only the first transition is
+      // allowed to publish a decision.
       return false;
     }
     clearTimeout(pending.timer);
     pending.record.resolvedAtMs = Date.now();
     pending.record.decision = decision;
     pending.record.resolvedBy = resolvedBy ?? null;
-    // Resolve the promise first, then delete after a grace period.
-    // This allows in-flight awaitDecision calls to find the resolved entry.
+    // Resolve first, then keep the record briefly so late awaitDecision calls
+    // and replay guards can still inspect the authoritative decision.
     pending.resolve(decision);
     scheduleResolvedEntryCleanup(() => {
       // Only delete if the entry hasn't been replaced
@@ -178,6 +181,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
     return entry?.record ?? null;
   }
 
+  /** Returns unresolved records that should still be visible to operators. */
   listPendingRecords(): ExecApprovalRecord<TPayload>[] {
     return Array.from(this.pending.values())
       .map((entry) => entry.record)
@@ -200,10 +204,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
     return true;
   }
 
-  /**
-   * Wait for decision on an already-registered approval.
-   * Returns the decision promise if the ID is pending, null otherwise.
-   */
+  /** Returns the decision promise for an already-registered approval id. */
   awaitDecision(recordId: string): Promise<ExecApprovalDecision | null> | null {
     const entry = this.pending.get(recordId);
     return entry?.promise ?? null;
@@ -239,6 +240,8 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
         continue;
       }
       if (normalizeLowercaseStringOrEmpty(id).startsWith(lowerPrefix)) {
+        // Prefix lookup is operator-facing convenience only; ambiguity is
+        // surfaced instead of picking a record.
         matches.push(id);
       }
     }
diff --git a/src/gateway/node-invoke-system-run-approval.ts b/src/gateway/node-invoke-system-run-approval.ts
@@ -93,6 +93,7 @@ function canBridgeNoDeviceApprovalFromBackend(params: {
   );
 }
 
+/** Returns whether an approval is bound to channel turn metadata for replay checks. */
 function hasChatApprovalReplayBinding(request: ExecApprovalRecord["request"]): boolean {
   return (
     normalizeComparableString(request.turnSourceChannel, { lowercase: true }) !== null ||
@@ -155,6 +156,8 @@ function canBridgeNoDeviceChatApprovalFromBackend(params: {
 
   const request = params.snapshot.request;
   const plan = request.systemRunPlan ?? null;
+  // Backend chat replays are allowed only when the replay carries the same
+  // channel/session/agent binding as the original approval request.
   return (
     matchesRequiredString({
       expected: request.turnSourceChannel,
@@ -356,6 +359,8 @@ export function sanitizeSystemRunParamsForForwarding(opts: {
     };
   }
   if (runtimeContext.plan) {
+    // Rehydrate forwarded params from the approved plan so caller-supplied
+    // command/cwd/session fields cannot drift after approval.
     next.command = [...runtimeContext.plan.argv];
     next.systemRunPlan = runtimeContext.plan;
     if (runtimeContext.commandText) {
@@ -394,11 +399,12 @@ export function sanitizeSystemRunParamsForForwarding(opts: {
     return toSystemRunApprovalMismatchError({ runId, match: approvalMatch });
   }
 
-  // Normal path: enforce the decision recorded by the gateway.
   if (snapshot.decision === "allow-once") {
     if (typeof manager.consumeAllowOnce !== "function" || !manager.consumeAllowOnce(runId)) {
       return systemRunApprovalRequired(runId);
     }
+    // allow-once is consumed before forwarding so the same approval cannot be
+    // replayed through the manager's resolved-entry grace period.
     next.approved = true;
     next.approvalDecision = "allow-once";
     return { ok: true, params: next };
@@ -411,7 +417,7 @@ export function sanitizeSystemRunParamsForForwarding(opts: {
   }
 
   // If the approval request timed out (decision=null), allow askFallback-driven
-  // "allow-once" ONLY for clients that are allowed to use exec approvals.
+  // "allow-once" only for clients that can use exec approvals.
   const timedOut =
     snapshot.resolvedAtMs !== undefined &&
     snapshot.decision === undefined &&
