Record a failed action the same way it was decided - #211
Conversation
cd2447b to
1686832
Compare
davidmckayv
left a comment
There was a problem hiding this comment.
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.
1686832 to
7fd4862
Compare
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
writehelper, and the argument lists haddrifted: the failure row was never passed
keyorcommand. So acomputer_run_commandthat failedpart-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
commandandfilePath, so withcommandgone a shell call fell through to the branch meant fora 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 halfthat is wrong on its own terms rather than merely missing.
The fix
The failure row is passed the same
keyandcommandthe decision row already spreads in. That isthe whole change: two conditional spreads, three lines. Restoring
commandalso settles the elementfallback, 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 —
keyandcommandwerealways 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
mainand passes with the fix. Reverting only the two spreads:It asserts the two rows agree rather than checking
commandby name — the fault is drift between twoargument lists, so a test naming one field would miss the next field added to one row and not the
other. It strips
failureoff the failed row and compares the remainder to the allowed row, across acommand, 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 untouchedmainshows at this commit — thecredential and OAuth tests want a Postgres database, and
agent-langgraph's history test cannotresolve
@langchain/core/messages. ACHANGELOG.mdline underUnreleased. No surface, so nothingto screenshot.
Out of scope
Building the entry once before the policy call and spreading
failureinto a copy for the secondwrite 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.