Skip to content

Record a failed action the same way it was decided - #211

Merged
davidmckayv merged 2 commits into
CopilotKit:mainfrom
zopeVaibhav:fix/audit-failure-row-fields
Aug 24, 2026
Merged

Record a failed action the same way it was decided#211
davidmckayv merged 2 commits into
CopilotKit:mainfrom
zopeVaibhav:fix/audit-failure-row-fields

Conversation

@zopeVaibhav

Copy link
Copy Markdown
Contributor

Fixes #210.

What was open

The gateway records a permitted action twice: once for the policy decision, and again if the action
was allowed and then failed. The second row exists so a reader can tell an action that happened from
one that was allowed and did not — without it the allowed row reads as "it happened".

The two rows are written by separate calls to the same write helper, and the argument lists had
drifted: the failure row was never passed key or command. So a computer_run_command that failed
part-way was recorded without the command — the row an incident opens first, saying a Bot ran
something and not what.

The same drop fabricated an element. The element fallback reads "this action never had an element"
off command and filePath, so with command gone a shell call fell through to the branch meant for
a browser action whose ref did not resolve, and the row claimed "element": "not in the current snapshot" — a lookup that never happened for an element a shell call never has. That is the half
that is wrong on its own terms rather than merely missing.

The fix

The failure row is passed the same key and command the decision row already spreads in. That is
the whole change: two conditional spreads, three lines. Restoring command also settles the element
fallback, since the fallback keys on it. A failed file write was already correct and is untouched.

No boundary moves. The policy still decides on a fully populated context — key and command were
always bound into it correctly; only the record of the outcome was short. Nothing is permitted that
was not before, no refusal or failure path changes, and no new value is trusted from the client.

Proof

The new test fails on main and passes with the fix. Reverting only the two spreads:

(fail) the computer gateway > a permitted action that fails is recorded the same way it was decided
error: a command
- Expected  - 2
+ Received  + 1

It asserts the two rows agree rather than checking command by name — the fault is drift between two
argument lists, so a test naming one field would miss the next field added to one row and not the
other. It strips failure off the failed row and compares the remainder to the allowed row, across a
command, a keypress and a file write, the write being the control that was already right.

server/tests/computer-gateway.test.ts: 56 pass. bun run typecheck, lint, format:check: clean.
bun test: 1355 pass, with the same 10 failures an untouched main shows at this commit — the
credential and OAuth tests want a Postgres database, and agent-langgraph's history test cannot
resolve @langchain/core/messages. A CHANGELOG.md line under Unreleased. No surface, so nothing
to screenshot.

Out of scope

Building the entry once before the policy call and spreading failure into a copy for the second
write would make this drift structurally impossible instead of fixing the one instance of it. It is a
larger edit to a deliberately explicit file, so it is left as the maintainers' call — happy to do it
here instead if that is preferred.

davidmckayv
davidmckayv previously approved these changes Aug 24, 2026

@davidmckayv davidmckayv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the old code before the description, and there are two faults here rather than one.

The obvious one is the omission: the failure row left out command, so a shell call that died part-way recorded that a Bot had run something without saying what — in the row somebody reading an incident reaches for first.

The one worth naming is the consequence. The element fallback keys on entry.filePath || entry.command, so with command dropped a failed shell call fell through to the browser branch and the row asserted "element": "not in the current snapshot" — a snapshot lookup that never happened, for an element a shell call never had. That is the trail stating a falsehood, which is exactly what the three-outcome design exists to prevent. This is consistent with that philosophy, not a dilution of it.

The test bites. Reverting both spreads turns the new test red; reverting only the key spread fails independently on the keypress case. And it compares whole payloads rather than named fields, so it will also catch the next field added to one row and not the other — the right shape for a drift bug, since this is a bug about two argument lists falling out of step.

Good call leaving the deeper refactor — build the entry once, spread failure into a copy — as a separate decision. For a template the small fix plus a payload-equality test is the better trade: it closes the hole now and guards the gap that caused it.

Checks: typecheck, lint, format clean; gateway suite 56 pass; CI green.

@davidmckayv
davidmckayv force-pushed the fix/audit-failure-row-fields branch from 1686832 to 7fd4862 Compare August 24, 2026 15:59
@davidmckayv
davidmckayv merged commit 434902f into CopilotKit:main Aug 24, 2026
@zopeVaibhav
zopeVaibhav deleted the fix/audit-failure-row-fields branch August 28, 2026 04:33
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.

A permitted action that fails is recorded without the command that ran, and with an element it never had

2 participants