fix review finding: an impossible scripted permission click rejects the run

A client-callback throw only becomes a JSON-RPC error RESPONSE to the
agent's session/request_permission — runScenario itself kept going, so a
tolerant agent could treat the error as a denial and the scenario would
pass, or worse, record: the impossible click baked into fixture and
golden, green on every replay. The mismatch is now captured as a harness
error while the agent is answered plain cancelled (a well-defined path
it cannot reinterpret), and the step loop rejects the run on it as soon
as the in-flight step settles. The spec asserts the rejection instead of
the agent-side error echo.

docs/rfc/implemented/testing/2026-07-08-shared-acp-snapshot-package.md

@@ -12,7 +12,7 @@ A second ACP example wanting snapshot coverage — the sandbox/approval composit
 
 The machinery lives in [`packages/support/acp-snapshot`](../../../../packages/support/acp-snapshot/README.md) (`@deepseek-ai/dsh-acp-snapshot`); an example's `*.snapshot.ts` is its scenario table, its agent paths, and one factory call, over its own `snapshots/` fixtures and `cordis.snapshot.yml` overlay ([single-source replay config](2026-07-04-single-source-acp-replay-config.md)). Reading `DSH_SNAPSHOT` stays at that edge — the library takes a resolved `mode`.
 
-**`src/harness.ts`** — `runScenario` and the input-script/result types, parameterized by an `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath`; absolute paths the consuming suite resolves from its own `import.meta.url`). The client's `session/request_permission` handler consumes an optional `InputScript.permissionAnswers` FIFO queue, each entry selecting by option **kind** (ids are agent-issued randoms a committed script cannot know; kinds are the ACP-stable vocabulary, mapped to the offered `optionId` at answer time); an absent or exhausted queue answers `cancelled`, and a kind the request never offered fails loud. This is what lets an approval suite drive allow/reject round-trips deterministically from `input.json`.
+**`src/harness.ts`** — `runScenario` and the input-script/result types, parameterized by an `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath`; absolute paths the consuming suite resolves from its own `import.meta.url`). The client's `session/request_permission` handler consumes an optional `InputScript.permissionAnswers` FIFO queue, each entry selecting by option **kind** (ids are agent-issued randoms a committed script cannot know; kinds are the ACP-stable vocabulary, mapped to the offered `optionId` at answer time); an absent or exhausted queue answers `cancelled`, and a kind the request never offered rejects the run — the agent itself is answered `cancelled`, so the scenario bug fails the harness rather than being absorbed as an agent-side denial. This is what lets an approval suite drive allow/reject round-trips deterministically from `input.json`.
 
 **`src/normalize.ts`** — the pure normalizers, hook-free by policy: when a future event carries a new volatile field (an approval duration, say), the shared normalizer learns it in the same change, keeping one home for what "normalized" means rather than per-suite scrub extensions.
 

packages/support/acp-snapshot/README.md

@@ -33,4 +33,4 @@ defineAcpSnapshotSuite({
 
 The example also ships a `cordis.snapshot.yml` replay overlay next to its `cordis.yml` (the bin swaps them under `DSH_SNAPSHOT=replay` — [single-source replay config RFC](../../../docs/rfc/implemented/testing/2026-07-04-single-source-acp-replay-config.md)); replay fixtures are served by [`dsh-llm-replay`](../llm-replay/README.md), which this package points at via the `DSH_SNAPSHOT_*` env vars it sets on the child. Fixture roles, record/replay semantics, and scenario-table fields are documented on `Scenario` and in the [snapshot RFC](../../../docs/rfc/implemented/testing/2026-06-19-acp-snapshot-tests.md).
 
-Constraints: `suite.ts` imports vitest, so the package is importable only inside a vitest run (the harness and normalizers have no such dependency but ship from the same entry). ACP-specific by design — the harness speaks the SDK's `ClientSideConnection`. Permission round-trips are scriptable: `InputScript.permissionAnswers` is a FIFO queue of option-kind selections (`allow_once`, `reject_once`, …) the client maps to the agent-issued `optionId` at answer time; an absent or exhausted queue answers `cancelled`, and a kind the request never offered fails loud.
+Constraints: `suite.ts` imports vitest, so the package is importable only inside a vitest run (the harness and normalizers have no such dependency but ship from the same entry). ACP-specific by design — the harness speaks the SDK's `ClientSideConnection`. Permission round-trips are scriptable: `InputScript.permissionAnswers` is a FIFO queue of option-kind selections (`allow_once`, `reject_once`, …) the client maps to the agent-issued `optionId` at answer time; an absent or exhausted queue answers `cancelled`, and a kind the request never offered rejects the run (the agent is answered `cancelled`, so a tolerant agent cannot absorb the scenario bug).

packages/support/acp-snapshot/src/harness.ts

@@ -97,7 +97,9 @@ export interface InputScript {
    * kind → the offered `optionId` at answer time. A request beyond the queue
    * (or with no queue at all) is answered `cancelled` — the stub behavior a
    * scenario without approvals relies on. A scripted kind the request does
-   * not offer fails loud: the scenario scripted an impossible click.
+   * not offer REJECTS the run: the scenario scripted an impossible click,
+   * and {@link runScenario} throws once the in-flight step settles (the
+   * agent itself just sees `cancelled`, so it cannot absorb the bug).
    */
   permissionAnswers?: PermissionAnswer[]
 }
@@ -240,6 +242,14 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise
     // Permission answers are consumed FIFO across the whole run; exhaustion
     // falls back to `cancelled` so approval-free scenarios keep the plain stub.
     const permissionQueue = [...input.permissionAnswers ?? []]
+    // A scenario bug detected inside a client callback (a scripted permission
+    // kind the agent never offered). It cannot fail the run from in there: a
+    // callback throw only becomes a JSON-RPC error RESPONSE to the agent, and
+    // a tolerant agent treats that as a denial and carries on — the run (or
+    // worse, a record) would absorb the impossible click silently. So the
+    // callback answers `cancelled` (a well-defined path for the agent),
+    // captures the error here, and the step loop fails the run on it.
+    let scriptError: Error | undefined
     const makeClient = (_agent: AcpAgent): Client => ({
       sessionUpdate(params: SessionNotification): Promise<void> {
         for (let i = updateWaiters.length - 1; i >= 0; i--) {
@@ -262,12 +272,13 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise
         const option = params.options.find(o => o.kind === answer.kind)
         if (option === undefined) {
           // The scenario scripted a click the agent never offered — a scenario
-          // bug. Throwing here surfaces as a JSON-RPC error on the permission
-          // request, which the transcript (and usually the run) fails on.
-          throw new Error(
+          // bug. Captured (last one wins; same bug class either way) and
+          // answered `cancelled`; the step loop rejects the run on it.
+          scriptError = new Error(
             `snapshot-harness: scripted permission answer ${answer.kind} not among `
             + `the offered options [${params.options.map(o => o.kind).join(', ')}]`,
           )
+          return Promise.resolve({ outcome: { outcome: 'cancelled' } })
         }
         return Promise.resolve({ outcome: { outcome: 'selected', optionId: option.optionId } })
       },
@@ -276,6 +287,11 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise
 
     for (const step of input.steps) {
       await runStep(client, step, cwd, waitForUpdate, () => sessionId, (id) => { sessionId = id })
+      // A permission exchange happens while a step's request is in flight, so
+      // by the time the step settles any script bug it exposed is captured —
+      // fail the run HERE, as a harness error, rather than hoping the agent's
+      // reaction to the answer perturbs the transcript.
+      if (scriptError !== undefined) throw scriptError
     }
     // Done driving: close stdin so the server disposes gracefully (flushing
     // persistence) and exits. Then await exit so the harvested log is complete.

packages/support/acp-snapshot/tests/harness.spec.ts

@@ -257,16 +257,16 @@ describe('runScenario', () => {
     expect(result.rawStdout).toContain('permission:{\\"outcome\\":\\"selected\\",\\"optionId\\":\\"opt-reject\\"}')
   })
 
-  it('fails loud on a scripted permission kind the agent never offered', { timeout: 20_000 }, async () => {
+  it('rejects the run on a scripted permission kind the agent never offered', { timeout: 20_000 }, async () => {
     const { fixtureFile } = await scenario({ permissionProbe: true })
     // The fake bin offers allow_once/reject_once; scripting allow_always is a
-    // scenario bug. The client handler throws, the SDK surfaces it as a
-    // JSON-RPC error on the permission request, and the fake bin echoes the
-    // missing outcome as null.
-    const result = await runScenario(
+    // scenario bug. The agent is answered `cancelled` (it must not be able to
+    // absorb the bug as an error-means-denial), and the RUN fails: a callback
+    // throw would only reach the agent as a JSON-RPC error response, letting
+    // a tolerant agent carry on and the scenario pass — or record.
+    await expect(runScenario(
       { steps: [...boot, { op: 'prompt', text: 'impossible click' }], permissionAnswers: [{ kind: 'allow_always' }] },
       { agent: AGENT, mode: 'replay', fixtureFile },
-    )
-    expect(result.rawStdout).toContain('permission:null')
+    )).rejects.toThrow(/allow_always not among the offered options \[allow_once, reject_once\]/)
   })
 })