Skip to content

Say Did not happen about a call the audit page had been calling Allowed - #351

Merged
davidmckayv merged 2 commits into
CopilotKit:mainfrom
kevin9327:audit-failed-calls-not-allowed
Sep 4, 2026
Merged

davidmckayv merged 2 commits into
CopilotKit:mainfrom
kevin9327:audit-failed-calls-not-allowed

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

What this changes

The audit page draws a row it does not recognise as Allowed. That is right for the many rows that
are neither a refusal nor a failure — a credential saved, a person's role changed. Two rows that
are failures were falling through to it.

mcp.call_failed. Written by callTool (server/src/plugins/store.ts:2957 and :2992) after
the policy allowed the call: the vendor answered isError, or the attempt threw. It is the direct
sibling of computer.action_failed — 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. So this is the most common failure the product has,
and it was the one row of the family drawn in the same muted colour as a call that worked.

component.function_failed. Written at server/src/components/routes.ts:278 when a granted
data read throws. server/src/audit.ts:279 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 were 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. 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.

This is the same drift #302 was about, on the two surfaces #302 did not reach. The file's own
header says the lists exist so "a new refusal is added in one place or in none" — that held for
refusals and not for failures.

Deliberately left alone: connector.sync_failed matches the same shape and is declared in the
same union, but 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.

Where it runs

  • New state that outlives a request? None. Two entries added to a frozen array of string
    literals in app/src/lib/audit/outcome.ts.
  • What happens on the second replica? Identical. This is a pure classifier in the browser
    bundle; the rows it classifies were already written by the server, unchanged.
  • Anything serialised? Nothing.
  • Anything fanned out to a browser? No. The Did not happen view's ?eventType= query
    grows by two names; the audit read path already accepts a comma list (server/src/audit.ts:515).
  • New listener, port, or schedule? None.

Boundary and audit

  • Every acting call still goes through the gateway: no acting path is touched.
  • New refusals and new failures each write a row: both rows were already written. This changes
    only what the page says they mean, and nothing is reclassified as a refusal.
  • Nothing new is trusted from the client.

Changelog

  • A tool call that failed no longer reads as one that worked, under Unreleased.

Proof

Fail before, pass after, confirmed by reverting the fix in outcome.ts and re-running.

With mcp.call_failed not in the list:

66 |     expect(outcomeOf("mcp.call_failed")).toBe("did-not-happen");
error: expect(received).toBe(expected)
Expected: "did-not-happen"
Received: "allowed"

138 |     expect(filtered).toContain("mcp.call_failed");
error: expect(received).toContain(expected)
Expected to contain: "mcp.call_failed"
Received: [ "computer.action_failed", "agent.stream_stalled", "agent.handoff_failed",
  "agent.escalation_failed" ]

 12 pass
 2 fail

And with only component.function_failed removed:

68 |     expect(outcomeOf("component.function_failed")).toBe("did-not-happen");
Expected: "did-not-happen"
139 |     expect(filtered).toContain("component.function_failed");
Received: [ "computer.action_failed", "agent.stream_stalled", "agent.handoff_failed",
  "agent.escalation_failed", "mcp.call_failed" ]

With the fix:

$ bun test app/tests/audit-outcome.test.ts
 14 pass
 0 fail
 52 expect() calls

Gates:

$ bun run format:check   Checked 515 files in 808ms. No fixes applied.
$ bun run lint           Checked 518 files in 882ms. No fixes applied.
$ bun run typecheck      app / server / worker: Exited with code 0

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 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@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.

Reviewed diff against current main; resolved CHANGELOG keep-both; validated locally (format + composed test run). CI green.

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.

2 participants