fix(audit): detect JSON-RPC errors in responses larger than the detection buffer

[breken-ai]

Summary

Audit error detection parsed a 512-byte prefix of the response with json.Unmarshal. A cut-off JSON object does not unmarshal, so any JSON-RPC error body over 512 bytes was recorded as outcome: success with no error code or message.

• When the prefix fills the buffer and the normal parse finds no error, detectApplicationError now falls back to a token walk of the top-level object that stops at the error key. It only needs the bytes before the cut.
• New mcp.ParseTruncatedMCPResponse does the walk. The code is kept if it was read before the cut; the message is kept only if it was read completely. Nested error keys are ignored, a truncated result is still a success, and "error": null is not an error.
• The buffer size and the recorded event shape are unchanged.

Fixes #6720

Type of change

• Bug fix

Test plan

I ran these with go directly. task is not installed in my environment, so the task targets and CI are not run yet.

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

go test -race -count=1 ./pkg/mcp/ ./pkg/audit/, go vet and gofmt are clean. New tests: an 11-case table for the walk (complete error, cut inside the message, cut after the code, cut inside the key name, cut right after "error":{, data before message, truncated result, nested error key, null error, non-object, empty), and a middleware test with bodies of 511, 512, 513 and 2048 bytes that expects application_error and the code. The middleware test fails without the auditor.go change.

API Compatibility

• This PR does not break the v1beta1 API. No field is added to the event; only the existing outcome and metadata are set.

Changes

File	Change
pkg/mcp/response.go	add ParseTruncatedMCPResponse
pkg/audit/auditor.go	fall back to it when the prefix fills the buffer
pkg/mcp/response_test.go, pkg/audit/auditor_test.go	tests above

Does this introduce a user-facing change?

Yes. Large JSON-RPC error responses are now audited as application_error with the error code, instead of success.

Open question

If the cut lands before the error key (for example a very large id), the record still says success. Do you want an explicit marker for "body was cut off, detection incomplete" as well, or is that out of scope here?

(Found, fixed and tested by an AI coding agent working for the breken-ai account.)

[codecov]

Codecov Report

❌ Patch coverage is 90.24390% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.66%. Comparing base (0dcd0ad) to head (2d86075).

Files with missing lines	Patch %	Lines
pkg/mcp/response.go	89.18%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6763      +/-   ##
==========================================
+ Coverage   79.64%   79.66%   +0.01%     
==========================================
  Files         816      816              
  Lines       83168    83208      +40     
==========================================
+ Hits        66242    66285      +43     
+ Misses      16921    16918       -3     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jhrozek]

Thanks for picking this up. The token walk approach looks right to me and the tests are thorough. There are a few things to sort out before this goes in, details inline. The main ones are the truncated code value and the IncludeResponseData path, which still logs large errors as success.

One more I couldn't leave inline because the file isn't in the diff: docs/middleware.md (around line 531) lists outcome as success/failure/denied/error and doesn't mention application_error. This PR makes that outcome a lot more common, so please add it there. Something like "a 2xx response whose JSON-RPC body has a top-level error, controlled by detectApplicationErrors", plus a note about the jsonrpc_error_code / jsonrpc_error_message metadata.

[jhrozek]

json.Decoder accepts a number that ends at EOF, so if the 512-byte cut lands inside the code digits this returns a truncated value. A prefix ending in ..."error":{"code":-320 (really -32603) gives HasError=true, ErrorCode=-320, and that gets logged as if it were the real code. It also contradicts the doc comment above, which says code is only filled in when it was read completely.

Could you keep the code only if something follows it, e.g. dec.InputOffset() < int64(len(body)), and add a table case with a cut in the middle of the number that expects ErrorCode 0? You can only hit this when the id or other fields push code up to the edge, but when it happens the audit record is quietly wrong.

[jhrozek]

Large errors are still missed when IncludeResponseData is on. In that case errorDetectionBody isn't allocated (L269) and detection reuses rw.body, but rw.body only takes a write if the whole thing fits under MaxDataSize (L141, 1024 by default). A single 1500-byte error write leaves rw.body empty, so the prefix is empty, the fallback never runs, and the event stays success. A response written in several chunks can also leave a truncated rw.body shorter than 512 bytes, which then skips the len(prefix) >= errorDetectionBufferSize check below.

I think the simplest fix is to always allocate errorDetectionBody regardless of IncludeResponseData (it's 512 bytes per request) and stop reusing rw.body for detection. A test with IncludeResponseData: true and a body larger than MaxDataSize would cover it. Since #6720 is about any error body over the buffer size, I'd like to see this fixed in this PR.

[jhrozek]

Related, though it predates this PR: the prefix[0] == '{' check rejects bodies with leading whitespace, which is still valid JSON. Something like \n{"jsonrpc":"2.0",...,"error":{...}} is never inspected, whatever its size. The new walker would handle it fine since the decoder skips whitespace; it's only this guard that rejects it. Trimming with bytes.TrimLeft(prefix, " \t\r\n") before the check would fix it. A follow-up is fine if you'd rather keep this PR narrow.

[jhrozek]

ParseMCPResponse has only this one caller, and the token walk gives the same answer on complete bodies. So I think detectApplicationError could just call the walker unconditionally (or make the walker the implementation of ParseMCPResponse) and drop the size check and the Unmarshal envelope. That also removes the case where a truncated prefix shorter than 512 bytes never reaches the fallback.

The behavior differences (case-sensitive key match, the first duplicate error key wins, a non-integer code gives HasError with code 0) look fine to me, arguably closer to the spec. If you want to keep both parsers, a comment explaining why there are two would help.