From e99de7c9e8fd899eb69241d9f4a93d2da1af6caa Mon Sep 17 00:00:00 2001 From: kevin9327 Date: Fri, 4 Sep 2026 06:55:50 +0900 Subject: [PATCH] Say Did not happen about a call the audit page had been calling Allowed The audit page falls back to "Allowed" for any row it does not recognise, 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. mcp.call_failed is written by callTool after the policy allowed the call: the vendor answered isError, or the attempt threw. It is the direct sibling of computer.action_failed, written by the same decide-record-then-act order on the other acting surface, and computer.action_failed was already in the family. Every failure of a per-person connector lands on this row - no connection for the asker, a refresh token the vendor stopped accepting, an API not enabled for the project - which makes it the most common failure this product has and the one row of the family drawn in the same muted colour as a call that worked. component.function_failed is a component's data read that was granted and then broke. server/src/audit.ts says it is "deliberately not a refusal", and that is right: nothing was forbidden. It is not an allowance either. The page already labels it "Could not be read" while colouring it as though it had been. Both are also absent from the Did not happen saved view, which is the half that is harder to notice: the view an administrator opens to ask what did not work here is not empty, it is just short by exactly the rows they came for. Neither is filed as a refusal, because 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. connector.sync_failed is declared in the same union and left alone on purpose: nothing writes it, the worker sync is not wired, so classifying a row that cannot exist would be a guess. It needs the same line the day it is wired. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 +++++++++++ app/src/lib/audit/outcome.ts | 21 ++++++++++++++++++ app/tests/audit-outcome.test.ts | 38 +++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11fb9c67f..6d8cf6609 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ 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. + ### Duplicating a coworker keeps the endpoint it was copied from Duplicate used to point every copy at this deployment's own managed Bot, whatever the coworker being 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) {