diff --git a/CHANGELOG.md b/CHANGELOG.md index 6149eb5a7..63f6c6e6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,17 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A tool call that failed no longer reads as one that worked + +The audit page draws a row it does not recognise as `Allowed`, which is right for the many rows that +are neither a refusal nor a failure. Two rows that are failures were falling through to it: a +connector tool call this deployment permitted and the vendor did not complete, and a component's +data read that was granted and then broke. Both were drawn in the same muted colour as a call that +went through, and neither appeared under `Did not happen`, so the view an administrator opens to ask +what did not work here was short by exactly the rows they came for. A per-person connector fails on +this path every time somebody's token expires, so this was the most common failure the product has +and the one the trail was quietest about. Both now read as `Did not happen`, and both are in that +saved view. Neither is filed as a refusal: nothing was forbidden on either row. ### A stray space in NODE_ENV no longer lets the public example key through A deployment that never changed `KEY_ENCRYPTION_KEY` encrypts its credential vault with the key diff --git a/app/src/lib/audit/outcome.ts b/app/src/lib/audit/outcome.ts index be2af9e1b..a114211c0 100644 --- a/app/src/lib/audit/outcome.ts +++ b/app/src/lib/audit/outcome.ts @@ -61,6 +61,27 @@ export const DID_NOT_HAPPEN_EVENT_TYPES = [ "agent.handoff_failed", /** A question that reached nobody: the Bot stopped, and the person was never asked. */ "agent.escalation_failed", + /* + * A tool call this deployment permitted and the vendor did not complete. + * + * The direct sibling of `computer.action_failed`, written by the same decide-record-then-act + * order on the other acting surface: `callTool` writes the decision row first, attempts the + * call, and then writes this instead of `mcp.call_succeeded` when the vendor answers `isError` + * or the attempt throws. Every failure of a per-person connector lands here — no connection for + * the asker, a refresh token the vendor stopped accepting, an API not enabled for the project — + * which makes it the row somebody most often comes to this page to find, and it was the one row + * of the family drawn in the same muted colour as a call that worked. + */ + "mcp.call_failed", + /* + * A component's read that was granted and then broke. + * + * `server/src/audit.ts` says this one is "deliberately not a refusal", and that is right: nothing + * was forbidden. But it is not an allowance either. The row exists precisely because the read did + * not happen, and the page already labels it "Could not be read" while colouring it as though it + * had been. + */ + "component.function_failed", ] as const; export type AuditOutcome = "refused" | "did-not-happen" | "allowed"; diff --git a/app/tests/audit-outcome.test.ts b/app/tests/audit-outcome.test.ts index 8aa1e5916..9487655c0 100644 --- a/app/tests/audit-outcome.test.ts +++ b/app/tests/audit-outcome.test.ts @@ -53,6 +53,30 @@ describe("what the trail says a row was", () => { expect(outcomeOf("agent.stream_stalled")).toBe("did-not-happen"); }); + /* + * The two acting surfaces have to answer this the same way. + * + * A browser action that was permitted and then failed was already drawn as "Did not happen". The + * same shape on the other two surfaces — a tool call the vendor did not complete, a component read + * that broke — fell through to "Allowed", which is what the page says about a call that worked. + */ + test("tells a permitted call that failed from one that went through", () => { + // Written by `callTool` after the policy allowed the call and the vendor answered `isError` or + // the attempt threw. A per-person connector fails here on every expired refresh token. + expect(outcomeOf("mcp.call_failed")).toBe("did-not-happen"); + // Granted, attempted, and the read broke. Not a refusal, and not an allowance either. + expect(outcomeOf("component.function_failed")).toBe("did-not-happen"); + // The surface that already got this right, kept here so the three cannot drift apart again. + expect(outcomeOf("computer.action_failed")).toBe("did-not-happen"); + }); + + test("a failed call is not filed as something this deployment refused", () => { + // The other wrong answer. Nothing was forbidden on either row, and filing a broken call as a + // policy event teaches a reader to distrust the policy events that are real. + expect(outcomeOf("mcp.call_failed")).not.toBe("refused"); + expect(outcomeOf("component.function_failed")).not.toBe("refused"); + }); + test("still calls something that went through allowed", () => { for (const eventType of [ "computer.action_allowed", @@ -101,6 +125,20 @@ describe("the saved views ask the same question the rows do", () => { } }); + /* + * The half of the failure that is harder to see. A row drawn in the wrong colour is at least on + * the page; a row missing from this view is absent from the answer to "what did not happen here", + * and the view is not empty, it is just short. + */ + test("Did not happen names the calls that were permitted and failed", () => { + const filtered = eventTypeFilter(DID_NOT_HAPPEN_EVENT_TYPES) + .replace("?eventType=", "") + .split(","); + + expect(filtered).toContain("mcp.call_failed"); + expect(filtered).toContain("component.function_failed"); + }); + test("no event type is in both families", () => { const refused = new Set(REFUSED_EVENT_TYPES); for (const eventType of DID_NOT_HAPPEN_EVENT_TYPES) {