From 9eb01669281a75af0d06ea5faa0ad401b1bbcd0f Mon Sep 17 00:00:00 2001 From: kevin9327 Date: Sun, 30 Aug 2026 15:21:50 +0900 Subject: [PATCH] Score each action once in a policy dry-run, not once per audit row it wrote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Testing a candidate boundary against recent history replays the audit trail's computer-action rows. It replayed three event types, and one is a duplicate: a permitted action that fails is recorded twice — the decision row written before it was attempted (action_allowed / action_refused) and a failure row written when it did not succeed (action_failed) — and both carry the same action and are returned by the query. So every failed action was scanned and scored twice. And because a dry-run policy carries a refused action out, a refused action can fail too, leaving two rows that disagree on the baseline: the decision row records "refused", while the failure row has no decision of its own and fell to the "allowed" branch. A candidate policy that refused the same action identically was then reported as a new refusal it never introduced. The failure row is an outcome, not a decision. The replay now skips it and scores each action once from its decision row, and the query no longer fetches it — so it also stops spending the scan budget on rows that would be dropped. The baseline for a permitted-but-failed action is still "allowed", because that is what its decision row says. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 10 +++++++ server/src/computer/policy-dry-run.ts | 21 +++++++++++-- server/tests/policy-dry-run.test.ts | 43 +++++++++++++++++++++++++-- 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c851fcd3..255ccac5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A policy dry-run no longer counts a failed action twice, or invents a change it did not make + +Testing a boundary against recent history replayed three kinds of audit row, and one of them is a +duplicate: a permitted action that fails is recorded both as the decision that allowed it and as a +separate failure row, so every failed action was scanned and scored twice. Worse, a dry-run policy +carries a refused action out, so a refused action can fail too — and its two rows disagree, the +decision row saying "refused" and the failure row reading as "allowed", so a candidate policy that +refused it identically was reported as a new refusal it never introduced. The replay now scores each +action once, from the row that recorded its decision. + ### A Bot's shell can no longer reach the embedded database without a password In the all-in-one image the cluster was `trust`-auth on loopback, and the Bot's shell runs in the diff --git a/server/src/computer/policy-dry-run.ts b/server/src/computer/policy-dry-run.ts index ba291d61..c79972d0 100644 --- a/server/src/computer/policy-dry-run.ts +++ b/server/src/computer/policy-dry-run.ts @@ -20,11 +20,17 @@ import { type PolicyContext, } from "./policy"; -/** The event types the gateway writes for a judged computer action. In one place, for the query. */ +/** + * The event types that record a policy DECISION about a computer action. In one place, for the query. + * + * Not `computer.action_failed`: the gateway writes that beside the decision row when a permitted + * action is attempted and does not succeed, so it is a second row for an action already recorded + * here, not a decision of its own. Replaying it too would score the action twice — see + * {@link dryRunAgainstHistory}. + */ export const REPLAYABLE_EVENT_TYPES = [ "computer.action_allowed", "computer.action_refused", - "computer.action_failed", ] as const; /** One action the candidate policy would have decided differently. */ @@ -38,7 +44,7 @@ export type DryRunChange = { element: { role: string; name: string } | null; command: string | null; file: string | null; - /** What actually happened, from the trail. A failed action was permitted first, so it was allowed. */ + /** What the policy in force decided, from the action's decision row. */ was: "allowed" | "refused"; would: "allowed" | "refused"; /** The candidate rule that decided it, or null for the default refusal. */ @@ -120,6 +126,14 @@ export function contextFromAuditPayload( * a dry-run policy's refusals, which were recorded and then carried out. That is the honest * baseline: the question this answers is "what would decide differently than was decided", not * "what would run differently than ran". + * + * A `computer.action_failed` row is skipped rather than scored. It is the outcome of an action whose + * decision row is already in this history, so counting it would score that action twice — and worse, + * a dry-run policy carries a refused action out, so a refused-then-failed action would arrive as a + * decision row that says "refused" and a failure row that reads as "allowed", inventing a change no + * policy made. The decision is on the decision row; the failure row only says it did not finish. + * Callers should exclude it from the query too ({@link REPLAYABLE_EVENT_TYPES}); this guards the + * function against being handed one regardless. */ export function dryRunAgainstHistory( policy: ActionPolicy, @@ -134,6 +148,7 @@ export function dryRunAgainstHistory( }; for (const event of events) { + if (event.eventType === "computer.action_failed") continue; const context = contextFromAuditPayload(event.payload); if (!context) continue; report.scanned += 1; diff --git a/server/tests/policy-dry-run.test.ts b/server/tests/policy-dry-run.test.ts index f6e3a07e..e5933733 100644 --- a/server/tests/policy-dry-run.test.ts +++ b/server/tests/policy-dry-run.test.ts @@ -122,7 +122,11 @@ describe("dryRunAgainstHistory", () => { expect(report.changes[0]?.would).toBe("allowed"); }); - test("a failed action was permitted first, so it counts as allowed", () => { + test("a permitted action that failed is counted once, from its decision row", () => { + // The gateway records a permitted-but-failed action twice: the decision row written before the + // attempt, and a failure row written when it did not succeed. Both carry the same action, and + // both are returned by the trail query, so scoring the failure row too would count the action a + // second time. Its baseline is still "allowed" — from the decision row that permitted it. const deny: ActionPolicy = { mode: "enforce", deny: ['tool.name == "computer_click"'], @@ -130,15 +134,50 @@ describe("dryRunAgainstHistory", () => { }; const report = dryRunAgainstHistory(deny, [ event({ - id: "f", + id: "dec", + eventType: "computer.action_allowed", + payload: CLICK_SUBMIT, + }), + event({ + id: "fail", eventType: "computer.action_failed", payload: CLICK_SUBMIT, }), ]); + expect(report.scanned).toBe(1); expect(report.wouldRefuse).toBe(1); + expect(report.changes).toHaveLength(1); + expect(report.changes[0]?.id).toBe("dec"); expect(report.changes[0]?.was).toBe("allowed"); }); + test("a dry-run refusal that was carried out and then failed invents no change", () => { + // In dry-run mode a refused action is still carried out, so a refused action can also fail. The + // decision row says "refused"; the failure row, read on its own, would read as "allowed" and a + // candidate that refuses the same action would then look like a new refusal. Skipping the failure + // row leaves only the honest baseline: it was refused, a policy that refuses it changes nothing. + const denyClicks: ActionPolicy = { + mode: "enforce", + deny: ['tool.name == "computer_click"'], + allow: ["true"], + }; + const report = dryRunAgainstHistory(denyClicks, [ + event({ + id: "dec", + eventType: "computer.action_refused", + payload: CLICK_SUBMIT, + }), + event({ + id: "fail", + eventType: "computer.action_failed", + payload: CLICK_SUBMIT, + }), + ]); + expect(report.scanned).toBe(1); + expect(report.wouldRefuse).toBe(0); + expect(report.unchanged).toBe(1); + }); + test("a rule naming a command does not refuse a click, because absent facts are neutral", () => { const candidate: ActionPolicy = { mode: "enforce",