feat(agent-bff): map agent errors to the BFF error contract

[nbouliol]

Context

Slice-3 BFF data/action endpoints (PRD-671..674) need one shared error layer producing the BFF envelope { error: { type, status, message, details? } }, so consumers branch on error.type + error.status, never on a message substring. This ships first and is a hard dependency of every Slice-3 endpoint ticket.

What this adds

• src/http/agent-error-mapper.ts — mapAgentError(error, { logger }): BffHttpError:
  • transport failure (raw error, not an AgentHttpError) → network_error (502), with a generic message (the real cause is logged, never sent to the client, to avoid leaking internal hostnames/IPs)
  • agent 5xx → agent_unavailable (503, normalized — including a 5xx errors[0].status carried in a non-5xx response)
  • JSON:API errors[0] → type from an explicit name registry (NotFoundError→not_found, …); status from errors[0].status (ignored if not >= 400, falling back to the outer AgentHttpError.status); message from detail (then message); details from data
  • unknown agent name → status-derived type (preserving errors[0].status) + a Warn log (an unrecognized name keeps the agent's real status/category instead of collapsing to a 500; the log surfaces it so an explicit registry entry can be added)
  • defensive parse of an Error whose message is a JSON { errors: [...] } string
• src/http/bff-local-errors.ts — the complete registry of BFF-local factories the Slice-3 endpoints emit: unknown_* (404), *_not_allowed (403), invalid_request (400), relation_field_not_supported (422), mapping_error (500), unsupported_action_result (501).
• package.json — adds @forestadmin/agent-client (the committed BFF→agent transport; AgentHttpError is the mapper input).

Produced BffHttpError instances flow through the existing error-middleware unchanged. No barrel exports: in-package endpoints import relatively.

Notes / deviations from the ticket text

The ticket and the global spec were stale on three points; the implementation follows the actual code:

• agent-client throws a typed AgentHttpError (not Error(JSON.stringify(...))); the string case is kept as a defensive fallback.
• Agent JSON:API errors carry data, not meta/source; details is sourced from data.
• Agent error names are suffixed (NotFoundError, not NotFound), so an explicit registry replaces literal snake_case.

Deliberate contract choices (unspecified by the spec):

• network_error=502 / agent_unavailable=503.
• An unmapped agent error name resolves to the status-derived type (e.g. a 400 subclass like MissingCollectionError → invalid_request 400), never collapsing to a generic 500, and is logged at Warn so a curated registry entry can be added. This preserves the agent's real status and category for unrecognized names.
• An error is never surfaced with a 2xx/3xx status: a non-error errors[0].status is ignored in favour of the enclosing status.

Tests

• test/http/agent-error-mapper.test.ts — registry coverage, JSON-in-message parse, outer-status fallback, non-numeric status fallback, message/body.message fallback, transport→502 (generic message + logged cause), 5xx→503 (both paths), non-error status ignored, unmapped-name→status-derived+Warn log.
• test/http/bff-local-errors.test.ts — every factory + details.

Full suite: 400/400 pass; build + lint clean.

Fixes PRD-670

🤖 Generated with Claude Code

Note

Map agent errors to the BFF error contract in agent-bff

• Adds mapAgentError to translate thrown agent errors into BffHttpError objects: transport failures become 502 network_error, agent 5xx become 503 agent_unavailable, JSON:API errors map to typed BFF errors, and flat bodies fall back to status-derived types.
• Adds bff-local-errors.ts with factory functions for BFF-local errors (e.g. unknownCollection, invalidRequest, mappingError) using predefined types and status codes.
• Adds @forestadmin/agent-client as a runtime dependency for AgentHttpError detection.

^{Macroscope summarized fc1d08e.}

[linear-code]

PRD-670

[qltysh]

1 new issue

Tool	Category	Rule	Count
qlty	Structure	Function with many returns (count = 5): mapAgentError	1

[qltysh]

Coverage Impact

⬆️ Merging this pull request will increase total coverage on main by 0.05%.

Modified Files with Diff Coverage (2)

Rating	File	% Diff	Uncovered Line #s
	packages/agent-bff/src/http/agent-error-mapper.ts	100.0%
	packages/agent-bff/src/http/bff-local-errors.ts	100.0%
	Total	100.0%

🚦 See full report on Qlty Cloud »

🛟 Help

• Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

• Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

• File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

  • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

[ShohanRahman]

1. Stale PR description contradicts the shipped behavior (documentation)
   The PR body states "unknown agent name → mapping_error (500) + an Error-level log." The actual code (agent-error-mapper.ts:104-116) does the opposite: an unmapped name keeps the status-derived type (e.g. 400→invalid_request) and logs at Warn. This was a deliberate change (commit 2e43f2d "preserve agent status/category for unmapped error names"); the Macroscope auto-summary at the bottom is correct, but the hand-written "What this adds" / "Deliberate contract choices" sections were never updated. The code and its comments are accurate — only the PR text is wrong. Fix: update the PR description so reviewers/future readers aren't misled.

2. network_error forwards the raw error.message to clients — agent-error-mapper.ts:151-153
   const message = error instanceof Error ? error.message : DEFAULT_NETWORK_MESSAGE;
   return new BffHttpError(STATUS_BAD_GATEWAY, TYPE_NETWORK_ERROR, message);
   Node transport errors embed internal topology: connect ECONNREFUSED 10.0.4.23:8080, getaddrinfo ENOTFOUND internal-agent.prod.svc.cluster.local. Since BffHttpError messages are serialized to the (browser) consumer via error-middleware, this leaks internal hostnames/IPs. The safe DEFAULT_NETWORK_MESSAGE already exists. Recommend: send the default to the client, and logger('Error'/'Warn', …) the real message instead. (Verify this branch isn't already relied on to surface a useful reason before changing.)

3. The catch-all transport branch has no logging — agent-error-mapper.ts:144-153
   Any non-AgentHttpError that isn't a parseable JSON:API message falls through to network_error (502) silently. That includes: a wrapped message whose JSON.parse fails, valid JSON with no errors[], or a stray semantic error class (ActionRequiresApprovalError, etc.). A real 4xx wrapped in a malformed body becomes a 502 with zero operator signal. The design comment says callers must scope their try/catch — but nothing enforces it. Recommend a single logger('Warn', 'Non-HTTP error mapped to network_error', { name }) before the return, mirroring the existing unmapped-name log.

4. Test coverage gaps (missing behavioral tests) — all confirmed against the source:

• mapFlatBody with a body carrying only .message (not .error) — the error > message > responseText > default priority chain is untested (agent-error-mapper.ts:134-138).
• coerceStatus with a non-numeric/empty string status ("abc", "" → NaN → fallback) — only "404" is tested (:58-62).
• 5xx JSON:API body inside an AgentHttpError whose outer status is <500 (e.g. outer 400, body status:503) → should normalize to agent_unavailable. Only the wrapped-message 5xx path is tested (:92-96).
• parseJsonApiFromMessage with valid JSON lacking an errors array (e.g. '{"foo":"bar"}') → falls to network_error with the raw JSON as the message (:119-127).
• The existing network_error test (agent-error-mapper.test.ts:117) asserts only {type, status} — not message, so neither the error.message nor the non-Error default branch is actually pinned. Add the message assertion.

[ShohanRahman]

A) type values are untyped string where a fixed registry exists — agent-error-mapper.ts:31,41
AGENT_ERROR_TYPE_MAP and FALLBACK_TYPE_BY_STATUS are Record<string, string>. A typo in a value ('unauthorzed') compiles silently and emits a type no client recognizes. Since the TYPE_* constants already exist, typing the record values as a BffErrorType union derived from them is near-zero cost and catches this at compile time.

▎ ⚠️ Conflict to resolve: the simplifier (below) recommends deleting these single-use TYPE_/STATUS_ constants; the type analyst recommends keeping the TYPE_* ones to build the union. You can't do both. My recommendation: keep the TYPE_* constants and add the union (compile-time typo protection > shaving a few lines), and separately inline the STATUS_* map keys (no such benefit there).

B) What-comments against project style — agent-error-mapper.ts:90-91, 94-95
CLAUDE.md says comment only the non-obvious why. // A JSON:API error status must be a 4xx/5xx… and // Normalize a 5xx here too… largely restate adjacent code. Trim them. Keep :101-103 (the MissingCollectionError subclass rationale is a genuine hidden constraint) and :148-150 (documents the caller try/catch-scoping invariant — a real why).

[nbouliol]

Thanks @ShohanRahman — all addressed in 8c1f281 (PR description updated too).

Comment 1

1. Stale PR description — fixed. The "What this adds" and "Deliberate contract choices" sections now describe the shipped behaviour (unmapped name → status-derived type + Warn, not mapping_error 500).
2. network_error leaking error.message — good catch, real info-leak. The client now gets the generic DEFAULT_NETWORK_MESSAGE; the real cause is logged at Warn instead. The message isn't part of the contract (consumers branch on type), so nothing relied on surfacing it.
3. Silent catch-all transport branch — the same branch now logs Warn with the cause before returning, so a 4xx wrapped in a malformed body isn't a zero-signal 502.
4. Test gaps — added: mapFlatBody body.message priority, non-numeric status → fallback, a 5xx errors[0].status inside a non-5xx AgentHttpError → agent_unavailable, valid JSON without errors[] → network_error+log, and the network_error message assertion (generic message, cause logged).

Comment 2

A. Untyped map values — added a BffErrorType union built from the TYPE_* constants; AGENT_ERROR_TYPE_MAP and FALLBACK_TYPE_BY_STATUS are now Record<…, BffErrorType>, so a typo'd value is a compile error. On the conflict you flagged: kept the TYPE_* constants for the union, and inlined the STATUS_* constants to raw HTTP literals (matching bff-http-error.ts / bff-local-errors.ts local precedent — HTTP codes are the textbook magic-number exception).
B. What-comments — trimmed the two flagged comments to a single why-line each; kept the MissingCollectionError subclass rationale and the caller try/catch-scoping note.

Build + lint clean, 400 tests pass.