Skip to content

Fix: make formatError's fallback stringification total - #824

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/error-utils-total-base-message
Open

Fix: make formatError's fallback stringification total#824
AmaadMartin wants to merge 2 commits into
mainfrom
fix/error-utils-total-base-message

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

No issue. core/src/utils/error_utils.ts landed in google#527.

  1. Or, if no issue exists, describe the change:

Problem: The formatError doc comment promises that it never throws, but the function had no guard. String(err) in baseMessage throws on a null-prototype object and on any value whose toString throws, and extractHttpDetails throws on a value with a throwing status, code, response or statusText getter. formatError backs the MCP transport error handler (mcp_session_manager.ts:22), so such a throw escapes into an async I/O callback and the error handler becomes the crash. String() also reduces {code: -32601, message: 'Method not found'} to '[object Object]', discarding every field.

Solution: baseMessage now serializes a record with JSON.stringify and truncates it with the existing truncateBody. A plain object therefore surfaces its fields under the same 1000-character bound as an HTTP response body. The String(err) fall-through stays for a value with no serializable own fields, so a class instance with a useful toString is unchanged. One try/catch at the exported boundary returns <unstringifiable value>, mirroring safeJsonSerialize in core/src/telemetry/tracing.ts.

Scope notes:

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Ten cases added to core/test/utils/error_utils_test.ts. All 27 existing cases are unedited and still pass: 37 passed.

npx vitest run --project unit:core core/test/utils/error_utils_test.ts

Coverage of core/src/utils/error_utils.ts: 100% of statements, branches, functions and lines.

Proof each test can fail. I ran four mutations and recorded the failures.

Mutation A — revert baseMessage to its original three-line body:

New case Failure
plain object surfaces its fields expected '[object Object]' to contain 'errorCode'
circular object expected '[object Object]' to be '<unstringifiable value>'
BigInt field expected '[object Object]' to be '<unstringifiable value>'
null-prototype object expected '<unstringifiable value>' to contain 'no prototype'
oversized object truncated expected '[object Object]' to contain '... [truncated]'

Mutation B — delete the try/catch from formatError:

New case Failure
throwing toString expected [Function] to not throw an error but 'Error: toString exploded' was thrown
throwing getter expected [Function] to not throw an error but 'Error: getter exploded' was thrown
circular object expected [Function] to not throw an error but 'TypeError: Converting circular structure...' was thrown
BigInt field Do not know how to serialize a BigInt

Mutation C — drop the typeof json === 'string' guard: toJSON returns nothing fails with expected '<unstringifiable value>' to be '[object Object]'.

Mutation D — drop the json !== '{}' guard: custom toString fails with expected '{}' to be 'ErrorLike: connection reset', and throwing toString fails with expected '{}' to be '<unstringifiable value>'.

The tenth case, formatError(42), survives all four. It is a regression guard for the primitive path, not a failing-first test.

Manual End-to-End (E2E) Tests:

Not applicable. The change is one utility module, and the unit test file drives it through the public formatError. To reproduce the old behaviour in plain node:

node -e "console.log(String(Object.create(null)))"                      # throws TypeError
node -e "console.log(String({code:-32601,message:'Method not found'}))" # [object Object]

Other checks on the pushed commit:

  • npx eslint core/src/utils/error_utils.ts core/test/utils/error_utils_test.ts — clean.
  • npx prettier --check on both files — clean.
  • npm run ts:check reports 292 errors on this branch and 292 on main. None are in either file I touched.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 8, 2026 14:06
baseMessage ended in `String(err)`, which throws for a null-prototype
object or a value with a throwing `toString`, and reduces a plain object
to `[object Object]`. Both contradict the documented promise that
`formatError` never throws.

Serialize a record with `JSON.stringify` (truncated at the existing
response-body limit), keep the `toString` fall-through for a value with
no serializable own fields, and wrap the whole helper in one try/catch
that degrades to `<unstringifiable value>`.
The try/catch sat in baseMessage, but extractHttpDetails and
formatErrorRecursive read `status`, `code`, `response`, `statusText` and
`cause` before baseMessage runs. A throwing getter therefore still
escaped, so the documented "never throws" promise stayed false.

Move the guard to formatError, which covers every read on the path, and
inline the single-use EMPTY_JSON_OBJECT constant. Add a test for a
throwing `status` getter to pin the new placement.
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.

1 participant