Skip to content

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

Open
kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/policy-dry-run-no-double-count
Open

Score each action once in a policy dry-run, not once per audit row it wrote#294
kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/policy-dry-run-no-double-count

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

What this changes

POST /policy-dry-run lets an admin test a candidate boundary against recent history before saving it: the drafted policy is replayed over recorded computer actions, and the report says which it would decide differently. The replay double-counts.

The gateway records a permitted action that fails at runtime with two rows (server/src/computer/gateway.ts): the decision row written before the attempt (computer.action_allowed / computer.action_refused, line 524), and a failure row written in the catch when it did not succeed (computer.action_failed, line 594). Both carry the same action payload. REPLAYABLE_EVENT_TYPES listed all three, so the trail query returned both rows for a failed action and dryRunAgainstHistory scored each twice — inflating scanned, wouldRefuse/wouldAllow and unchanged.

Worse in dry-run mode, where a refused action is still carried out (policy.ts sets forward: true) and so can fail. Its two rows then disagree on the baseline: the decision row records refused, but the failure row has no decision of its own and fell to the "allowed" branch (policy-dry-run.ts:142). A candidate policy that refused the same action identically was reported as a new refusal it never introduced — a phantom change in the exact report an admin uses to decide whether a rule is safe to save.

The failure row is an outcome, not a decision. The fix scores each action once, from its decision row: dryRunAgainstHistory skips computer.action_failed, and REPLAYABLE_EVENT_TYPES no longer lists it (so the query stops spending its row budget on rows that would be dropped). A permitted-but-failed action's baseline stays allowed — that is what its decision row says.

Reproduced before the fix (both now fixed): a permitted-but-failed action gave scanned: 2; a dry-run refused-then-failed action gave wouldRefuse: 1 against a policy that changed nothing.

Where it runs

dryRunAgainstHistory is a pure function over an AuditEvent[] the caller queried; the query lives in server/src/computer/routes.ts (POST /policy-dry-run). No new state.

  • New state that outlives a request? None.
  • What happens on the second replica? Identical: a pure function of the rows it is given, which come from the shared audit table.
  • Anything serialised? N/A.
  • Anything fanned out to a browser? No — a request/response report.
  • New listener, port, or schedule? None.

Boundary and audit

  • Every acting call still goes through the gateway: this only reads the trail to preview a policy; it decides and acts on nothing.
  • New refusals and new failures each write a row: unchanged — the gateway still writes both the decision row and the failure row. This changes only how the replay reads them.
  • Nothing new is trusted from the client that the server can resolve itself: unchanged.

Changelog

  • Added under Unreleased: "A policy dry-run no longer counts a failed action twice, or invents a change it did not make". (The dry-run feature shipped in 0.0.5, so a deployment's report behaves differently after upgrading.)

Proof

Verified locally (bun 1.3.14):

  • bun test server/tests/policy-dry-run.test.ts10 pass / 0 fail. The prior test that asserted a lone failure row scores as allowed (a shape the query never returns on its own) is replaced by two that reflect production: a permitted-but-failed action (decision row + failure row) is counted once from its decision row; a dry-run refused-then-failed action invents no change. Existing cases (new deny, loosened policy, neutral absent facts, count-past-cap) still pass.
  • cd server && bun run typecheck — clean.
  • bunx biome format / bunx biome lint on the changed source and test — clean.

(Integration tests needing Postgres fail locally with Connection closed; that is the absent local DB. This suite is pure and needs none.)

… wrote

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 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant