fix: translate 400, 429, 5xx and network failures into CliErrors

[lmjabreu]

Overview

wrapResult only translated 401, 403, 404 and 409. Any other failure, notably a 400, reached the user as a raw CommsRequestError stack trace with the server's error_string dropped ({"error":{"code":"INTERNAL_ERROR","message":"Request failed with status 400"}} under --json). 429, 5xx and network-down took the same path. Found by an offline review of #77.

These now map to CliErrors, exit 1, parseable under --json:

• 400 → INVALID_REQUEST, with the server's reason as the message
• 429 → RATE_LIMITED
• 5xx → API_ERROR (existing code)
• no response → NETWORK_ERROR, with the SDK's message (which already carries the cause and retry count)

Not in this PR: other 4xx statuses (413, 422 and so on) still pass through as before, and tdc search / tdc mentions use their own fetch outside this path.

The SDK has no predicates for these statuses, so the branches read CommsRequestError.httpStatusCode. 5xx messages leave out the server body on purpose, since it is often proxy HTML.

Reference

Found while reviewing #77. No flag.

Changelog

Bad requests, rate limits, server errors and network failures from the Comms API now show a clear message instead of a stack trace.

Test plan

1. On main, point the CLI at a local server that answers every request with 400 {"error_string": "Status message must be between 1 and 80 characters"}, then run COMMS_API_TOKEN=x COMMS_BASE_URL=http://127.0.0.1:<port> tdc user.
   • stderr is a stack trace, and --json gives INTERNAL_ERROR.
2. Run the same command on this branch.
   • stderr is Comms rejected the request: Status message must be between 1 and 80 characters, exit 1.
   • With --json, the output parses and error.code is INVALID_REQUEST.
3. Change the server's status to 429, then 503.
   • The codes are RATE_LIMITED and API_ERROR, each with a retry hint.
4. Stop the server.
   • The code is NETWORK_ERROR, with the cause in the message.

Adversarial review

Reviewer	Head	Raised	Fixed	Refuted	Deferred	Reviewer tokens
Fable x3 + Sonnet 5.5	89dec11	12	8	1	3	713,035

🤖 Generated with Claude Code

[lmjabreu]

@doistbot /review

[doistbot]

This PR extends wrapResult's error translation to cover 400 (INVALID_REQUEST), 429 (RATE_LIMITED), 5xx (API_ERROR) and no-response (NETWORK_ERROR) failures, mapping them onto CliErrors with the server's reason surfaced instead of a raw stack trace. No inline issues were flagged; the new branches were reviewed as cleanly following the existing central-translation pattern and ErrorCode conventions, correctly ordered after the more specific 401/403/404/409 mappings, with tests covering each new branch, the missing-error_string fallback, the unmapped-error fallthrough (updated to 418), and non-Comms errors passing through.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (2)

• src/lib/api.ts:242: Reuse: errors.ts already centralizes status checks behind predicates built on hasCommsStatusCode (isInsufficientScope, isForbidden, isInvalidToken, etc.), and this file imports those instead of touching httpStatusCode directly. Consider adding isRateLimited / isServerError / isBadRequest-style predicates there (using hasCommsStatusCode or taking the status) so the 400/429/5xx checks follow the same pattern and stay in one place for the next status mapping.
• src/lib/api.test.ts:354: All 5xx cases pass {} as the SDK response, so none protect the stated decision that 5xx messages omit the server body (often proxy HTML). If wrapResult later includes getCommsErrorString(error) in the 5xx message, these tests still pass. Add a 5xx case with a non-empty error_string and assert that body is not part of the message.

_{Share Feedback • Review Logs}

[lmjabreu]

Reuse: errors.ts already centralizes status checks behind predicates built on hasCommsStatusCode … Consider adding isRateLimited / isServerError / isBadRequest-style predicates there

Done in 89dec11. Added isBadRequest, isRateLimited, isServerError and isNoResponse to errors.ts, and wrapResult uses them now.

All 5xx cases pass {} as the SDK response, so none protect the stated decision that 5xx messages omit the server body … Add a 5xx case with a non-empty error_string and assert that body is not part of the message.

Done in 89dec11. Added a 502 case with an error_string and asserted it stays out of the message. It fails if the body is added to the message.

[lmjabreu]

@doistbot /review

[doistbot]

This PR cleanly extends wrapResult's error translation so 400, 429, 5xx, and no-response failures surface as well-formed CliErrors instead of raw stack traces, with the new status predicates (built on the existing hasCommsStatusCode / range helper in errors.ts) correctly centralized alongside the established patterns and correct branch ordering. No inline issues were flagged. Tests cover each new mapping, the error_string fallback paths, the 5xx range edges, the deliberate omission of server bodies for 5xx messages, and pass-through for non-Comms and unmapped statuses, and a security-focused pass found no trust-boundary concerns — messages only interpolate the same SDK/server fields already used by the 403/404/409 branches.

I also left one optional follow-up note in the details below.

Optional follow-up note (1)

• src/lib/api.test.ts:331: This standalone no-error_string case overlaps with the it.each below, which already exercises the same getCommsErrorString(error) || '400 Bad Request' fallback for other unusable-body shapes ('', 42, 'not json'). Fold {} into that it.each as a fourth case and delete this test — the fallback path stays protected with one less test to maintain.

_{Share Feedback • Review Logs}

[lmjabreu]

This standalone no-error_string case overlaps with the it.each below … Fold {} into that it.each as a fourth case and delete this test

Done in d77d6c0. {} is now a case in the it.each and the standalone test is gone.