diff --git a/src/wrappers/with-assembly.ts b/src/wrappers/with-assembly.ts index bdaa407ca..bd7486881 100644 --- a/src/wrappers/with-assembly.ts +++ b/src/wrappers/with-assembly.ts @@ -133,6 +133,51 @@ function hasInvoke( return typeof tool.invoke === "function"; } +/** + * Hand a pre-execution deny to the gateway's audit sink (AAASM-5665). + * + * Before this, `withAssembly` called `check` and never `record`, so a denied + * call produced no audit event at all — the thrown `PolicyViolationError` was + * the only trace, and it never leaves the process. + * + * `GatewayRecordEvent` carries no tool-name field and adding one is a wire + * change, so the tool is named inside `reason` — reusing the thrown error's own + * message, so the audit event and the error a caller sees cannot drift apart. + * + * **Every failure mode is swallowed, synchronous throws included.** A failing + * audit sink must not convert a policy deny into some other error: the throw + * that follows this call is the enforcement decision and has to survive. The + * `try`/`catch` is load-bearing rather than stylistic — `gatewayClient` is a + * documented public injection point, so a caller-supplied `record` that throws + * synchronously never produces a promise for a trailing `.catch()` to attach + * to, and would escape past the `PolicyViolationError`. Mirrors the Python + * companion's `try/except Exception` around the same hook (AAASM-4782). + * + * This is deliberately awaited, unlike the `void ... .catch()` in + * `recordToolResultNonBlocking` (`hooks/ai-sdk.ts`, `hooks/openai-agents.ts`): + * the event is handed over before the deny is thrown, so it cannot be lost if + * the caller exits on the error. The cost is that a `record` which hangs delays + * the deny — there is no timeout here, unlike `waitForApprovalWithTimeout`. + * + * What this does *not* do is make a deny observable in a released binary. Both + * shipped clients discard the event — `createNoopGatewayClient` returns + * undefined, `createNativeGatewayClient` fires only a one-time `AA_DEBUG` note + * — so a deny stays Unmeasured in audit evidence (ADR 0033 §6). Supplying a + * sink that retains it is tracked as AAASM-5681. + */ +async function recordDeny( + gateway: GatewayClient, + action: string, + runId: string, + reason: string +): Promise { + try { + await gateway.record({ action, runId, reason }); + } catch { + // Intentionally ignored — see the note above on why the deny must survive. + } +} + /** * Run the full pre-execution governance chain for one wrapped tool call. * @@ -161,18 +206,34 @@ async function enforceGovernance( runId }); - if (decision.denied) { - throw new PolicyViolationError(`Tool '${name}' blocked: ${decision.reason ?? "Denied"}`); - } + // Both refusal routes converge on a single record-then-throw below rather + // than each carrying its own copy (AAASM-5665). Two call sites is an + // invitation to fix one and miss the other — which is exactly what happened + // in review: the approval-rejected copy could be deleted outright with the + // whole suite still green. One site cannot drift from itself. + let denial: { action: string; error: PolicyViolationError } | undefined; - if (decision.pending) { + if (decision.denied) { + denial = { + action: "tool_call_denied", + error: new PolicyViolationError(`Tool '${name}' blocked: ${decision.reason ?? "Denied"}`) + }; + } else if (decision.pending) { const finalDecision = await waitForApprovalWithTimeout(gateway, name, runId, approvalTimeoutMs); if (finalDecision.denied) { - throw new PolicyViolationError( - `Approval rejected for '${name}': ${finalDecision.reason ?? "Rejected"}` - ); + denial = { + action: "tool_call_approval_rejected", + error: new PolicyViolationError( + `Approval rejected for '${name}': ${finalDecision.reason ?? "Rejected"}` + ) + }; } } + + if (denial) { + await recordDeny(gateway, denial.action, runId, denial.error.message); + throw denial.error; + } } /** diff --git a/tests/helpers/negative-control.ts b/tests/helpers/negative-control.ts index 38867ac40..024c6ff59 100644 --- a/tests/helpers/negative-control.ts +++ b/tests/helpers/negative-control.ts @@ -163,6 +163,93 @@ export interface PolicyGatewayClient extends GatewayClient { readonly auditResults: readonly GatewayResultRecord[]; } +/** + * A {@link GatewayClient} whose `check` returns `pending` and whose approval is + * then rejected — the second refusal route through `enforceGovernance`, which + * emits a different audit action from a straight policy deny. + */ +export function createPendingThenRejectGatewayClient( + approvalReason?: string +): PolicyGatewayClient { + const decisions: RecordedCheck[] = []; + const checkRequests: GatewayCheckRequest[] = []; + const auditEvents: GatewayRecordEvent[] = []; + const auditResults: GatewayResultRecord[] = []; + + return { + mode: "sdk-only", + decisions, + checkRequests, + auditEvents, + auditResults, + start: async () => undefined, + close: async () => undefined, + check: async (request: GatewayCheckRequest): Promise => { + checkRequests.push(request); + decisions.push({ + toolName: request.toolName, + action: request.action, + runId: request.runId, + denied: false + }); + return { denied: false, pending: true }; + }, + // Omitting the reason is the case that exercises the wrapper's `?? "Rejected"` + // fallback — a gateway is not obliged to explain itself. + waitForApproval: async () => + approvalReason === undefined ? { denied: true } : { denied: true, reason: approvalReason }, + record: async (event: GatewayRecordEvent) => { + auditEvents.push(event); + }, + recordResult: async (record: GatewayResultRecord) => { + auditResults.push(record); + }, + scanPrompts: async () => undefined + }; +} + +/** A denying {@link GatewayClient} whose audit `record` always fails. */ +export interface FailingRecordGatewayClient extends GatewayClient { + /** How many times `record` was entered, so a test can tell "failed" from "never called". */ + readonly recordAttempts: number; +} + +/** + * A denying client whose `record` fails in one of two shapes. + * + * `sync-throw` is the load-bearing one: it throws *before* returning a promise, + * so a trailing `.catch()` on the return value never sees it. Only a + * `try`/`catch` around the call contains it. + */ +export function createFailingRecordGatewayClient( + mode: "async-reject" | "sync-throw" +): FailingRecordGatewayClient { + let attempts = 0; + return { + mode: "sdk-only", + get recordAttempts() { + return attempts; + }, + start: async () => undefined, + close: async () => undefined, + check: async (request: GatewayCheckRequest): Promise => ({ + denied: true, + pending: false, + reason: `tool '${request.toolName}' is denied by policy` + }), + waitForApproval: async () => ({ denied: true }), + record: (() => { + attempts += 1; + if (mode === "sync-throw") { + throw new Error("audit sink down (sync)"); + } + return Promise.reject(new Error("audit sink down (async)")); + }) as GatewayClient["record"], + recordResult: async () => undefined, + scanPrompts: async () => undefined + }; +} + export function createPolicyGatewayClient(options: { denyTools: readonly string[]; }): PolicyGatewayClient { diff --git a/tests/quickstart-negative-control.test.ts b/tests/quickstart-negative-control.test.ts index 71a5ff616..0956cb06f 100644 --- a/tests/quickstart-negative-control.test.ts +++ b/tests/quickstart-negative-control.test.ts @@ -30,8 +30,10 @@ import { PolicyViolationError } from "../src/errors/policy-violation-error.js"; import { initAssembly } from "../src/core/init-assembly.js"; import { withAssembly } from "../src/wrappers/with-assembly.js"; import { + createFailingRecordGatewayClient, createFileSideEffect, createNetworkSideEffect, + createPendingThenRejectGatewayClient, createPolicyGatewayClient, type FileSideEffect, type NetworkSideEffect @@ -239,6 +241,109 @@ describe("quick-start negative control: what a deny is and is not attributed to" }); }); +describe("quick-start negative control: a deny is handed to the gateway's record call", () => { + it("hands the gateway an audit event naming the denied tool and its run", async () => { + const effect = fileEffect(); + const gateway = createPolicyGatewayClient({ denyTools: ["write_file"] }); + const tools = { + write_file: { execute: async (content: string) => effect.write(content) } + }; + + withAssembly(tools, { gatewayClient: gateway, agentId: AGENT_ID }); + + const outcome = await settle(tools.write_file.execute("denied-content")); + + // Absence of the effect first, as everywhere else in this file. + expect(effect.occurred()).toBe(false); + expect(outcome).toBeInstanceOf(PolicyViolationError); + + // The load-bearing assertion for AAASM-5665: the audit event the wrapper + // handed the gateway. Before this, `withAssembly` called `check` and never + // `record`, so `auditEvents` stayed empty on a deny while `decisions` was + // 1 — a deny existed only in the decision log. + // + // Scope of the evidence: this is the fixture's in-process array. Both + // shipped GatewayClient implementations discard the event + // (createNoopGatewayClient, createNativeGatewayClient), so a deny is + // Unmeasured in audit evidence on the shipped path — AAASM-5681. What this + // pins is the wrapper's call. + expect(gateway.auditEvents).toHaveLength(1); + const event = gateway.auditEvents[0]; + expect(event?.action).toBe("tool_call_denied"); + // Correlates the audit event with the decision that produced it. + expect(event?.runId).toBe(gateway.decisions[0]?.runId); + // GatewayRecordEvent has no tool-name field, so the tool is named in the + // reason. Asserting that keeps the event attributable without a wire change. + expect(event?.reason).toContain("write_file"); + + // The allowed-result sink is still untouched on a deny: the tool never ran, + // so there is no result to record. Without this, an implementation that + // recorded a bogus empty result would pass the assertions above. + expect(gateway.auditResults).toHaveLength(0); + }); + + it("hands the gateway a distinct audit event when an approval is rejected", async () => { + // The second refusal route. Review round 1 found this call site executed by + // five tests and asserted by none: deleting it left the suite bit-identical, + // while deleting its policy-deny sibling failed a test. Execution is not + // pinning, so the action string gets its own control. + // Run with and without an approver reason. A gateway is not obliged to + // explain a rejection, and attribution must survive that: the tool name + // comes from the wrapper's own message, not from the gateway's text. + for (const approvalReason of ["approver said no", undefined]) { + const effect = fileEffect(); + const gateway = createPendingThenRejectGatewayClient(approvalReason); + const tools = { + write_file: { execute: async (content: string) => effect.write(content) } + }; + const label = approvalReason === undefined ? "no reason" : "with reason"; + + withAssembly(tools, { gatewayClient: gateway, agentId: AGENT_ID }); + + const outcome = await settle(tools.write_file.execute("denied-content")); + + expect(effect.occurred(), `[${label}] the rejected tool body ran`).toBe(false); + expect(outcome, `[${label}]`).toBeInstanceOf(PolicyViolationError); + + expect(gateway.auditEvents, `[${label}]`).toHaveLength(1); + const event = gateway.auditEvents[0]; + // Distinct from the policy-deny action, so the two routes stay + // distinguishable in whatever sink eventually retains them. + expect(event?.action, `[${label}]`).toBe("tool_call_approval_rejected"); + expect(event?.runId, `[${label}]`).toBe(gateway.decisions[0]?.runId); + expect(event?.reason, `[${label}]`).toContain("write_file"); + expect(gateway.auditResults, `[${label}]`).toHaveLength(0); + } + }); + + it("still throws the policy violation when the record call itself fails", async () => { + // A caller-supplied gatewayClient is a documented public injection point, so + // a record() that throws is reachable user code. Both failure shapes are + // covered: a rejected promise, and a SYNCHRONOUS throw — the latter produces + // no promise for a trailing .catch() to attach to and escaped past the deny + // before this was a try/catch. + for (const mode of ["async-reject", "sync-throw"] as const) { + const effect = fileEffect(); + const gateway = createFailingRecordGatewayClient(mode); + const tools = { + write_file: { execute: async (content: string) => effect.write(content) } + }; + + withAssembly(tools, { gatewayClient: gateway, agentId: AGENT_ID }); + + const outcome = await settle(tools.write_file.execute("denied-content")); + + expect(effect.occurred(), `[${mode}] the denied tool body ran`).toBe(false); + // The enforcement decision survives the audit failure — a caller doing + // `catch (e) { if (e instanceof PolicyViolationError) ... }` still sees it. + expect(outcome, `[${mode}] the audit failure replaced the deny`).toBeInstanceOf( + PolicyViolationError + ); + expect(gateway.recordAttempts, `[${mode}] record() was never attempted`).toBe(1); + } + }); +}); + describe("quick-start negative control: an ungoverned seam cannot look protected", () => { it("a tool with no execute/invoke seam is warned about and still performs its effect", async () => { const effect = fileEffect();