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

* Record a failed action the same way it was decided

* Record the audit row fix in the changelog

Author: zopeVaibhav

CHANGELOG.md

@@ -54,6 +54,21 @@ the admin screen, which mints a token and points at the one it just made. If a d
 custom server pointing at a credential of another kind, adding it again will now be refused, and the
 answer is to give the server its own token.
 
+### A failed action is recorded the same way it was decided
+
+An action the policy allowed and the computer then failed is recorded twice, once for the decision
+and once for the outcome, so the trail can tell an action that happened from one that was permitted
+and did not. The second row was leaving out the command and the key that the first one carried.
+
+A shell command that failed part-way therefore said a Bot had run something without saying what, in
+the row somebody reading an incident reaches for first. The same omission picked the wrong element
+branch, so that row also claimed the command had been looked for in the page snapshot and not found
+— a page element a shell call never had. A failed file write kept its path throughout and is
+unchanged.
+
+Both rows now carry the same subject. Nothing about the boundary moves: the policy decided on a
+complete context before and after, and no action is permitted that was not permitted before.
+
 ### Upgrading
 
 **A deployment that sets `AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true` with `NODE_ENV=production` no

server/src/computer/gateway.ts

@@ -534,13 +534,22 @@ export function createComputerGateway(
        * Writing the decision before acting is still right, because an allowed action may have partial
        * effects before failing. The failure row records the outcome separately from the policy
        * decision.
+       *
+       * It carries the same subject the decision row did, and for the same reasons: a shell call
+       * that failed part-way is the row somebody most needs to name the command from, and a keypress
+       * that failed is still the difference between a submitted form and a typed letter. Leaving
+       * them off also chose the wrong element branch below, because "this action never had an
+       * element" is decided by the command and the file path — so a failed command claimed a
+       * snapshot lookup that never happened.
        */
       await write(auditStore, {
         toolName,
         botId,
         actor,
         element,
         ref,
+        ...(subject.key ? { key: subject.key } : {}),
+        ...(subject.command ? { command: subject.command } : {}),
         filePath,
         pageUrl,
         decision,

server/tests/computer-gateway.test.ts

@@ -201,6 +201,11 @@ async function gatewayWith(
     resetResult?: { cleared: boolean };
     locations?: ComputerLocation[];
     token?: string;
+    /** Endpoints the computer answers differently, for the calls that have to fail. */
+    routes?: Record<
+      string,
+      (init?: RequestInit) => Response | Promise<Response>
+    >;
   },
 ) {
   const { provider, fetchImpl, calls, addressedAs, requests } =
@@ -444,6 +449,68 @@ describe("the computer gateway", () => {
     expect(rows[0]?.payload.command).toBe("cat secrets.txt");
   });
 
+  /**
+   * The two rows describe one action, so they have to describe the same one.
+   *
+   * Asserted as agreement rather than field by field on purpose: the failure row is a second
+   * hand-maintained argument list, and what goes wrong with one of those is not a particular field
+   * being wrong, it is a field being added to one and not the other. Comparing the payloads catches
+   * the next one too.
+   */
+  test("a permitted action that fails is recorded the same way it was decided", async () => {
+    const failing = () =>
+      Response.json({ error: "device or resource busy" }, { status: 500 });
+
+    for (const action of [
+      {
+        what: "a command",
+        route: "/exec",
+        run: (gateway: Awaited<ReturnType<typeof gatewayWith>>["gateway"]) =>
+          gateway.runCommand("bot-1", ACTOR, {
+            command: "rm -rf /workspace/build",
+          }),
+      },
+      {
+        what: "a keypress",
+        route: "/key",
+        run: (gateway: Awaited<ReturnType<typeof gatewayWith>>["gateway"]) =>
+          gateway.key("bot-1", ACTOR, {
+            ref: "e1",
+            snapshotId: 7,
+            key: "Enter",
+          }),
+      },
+      {
+        what: "a file write",
+        route: "/files/write",
+        run: (gateway: Awaited<ReturnType<typeof gatewayWith>>["gateway"]) =>
+          gateway.writeFile("bot-1", ACTOR, { path: "notes.md", text: "kept" }),
+      },
+    ]) {
+      const { gateway, rows } = await gatewayWith(PERMISSIVE, {
+        routes: { [action.route]: failing },
+      });
+      rows.length = 0;
+
+      await expect(action.run(gateway)).rejects.toThrow();
+
+      const allowed = rows.find(
+        (row) => row.eventType === "computer.action_allowed",
+      );
+      const failed = rows.find(
+        (row) => row.eventType === "computer.action_failed",
+      );
+      expect(allowed, action.what).toBeDefined();
+      expect(failed, action.what).toBeDefined();
+
+      // The outcome is the only thing the failure row adds. Everything describing what was attempted
+      // is the same action and reads the same on both rows.
+      const { failure, ...attempted } = failed?.payload ?? {};
+      expect(failure, action.what).toBeString();
+      expect(attempted, action.what).toEqual(allowed?.payload ?? {});
+    }
+  });
+
   test("the computer is told WHICH Bot is asking", async () => {
     // Every per-Bot behaviour on the computer keys off this id: the profile it opens, the logins it
     // has, the proxy its traffic leaves through, and who holds its wheel.