Score each action once in a policy dry-run, not once per audit row it wrote

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

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;

server/tests/policy-dry-run.test.ts

@@ -122,23 +122,62 @@ 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"'],
       allow: ["true"],
     };
     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",