feat: actionable error messages and retry-with-fix for fixable 400s

[scottwofford]

Implements Trello: COE audit: Replace raw API JSON errors with actionable user-facing messages and implement retry-with-fix for fixable 400 errors.

What changed

1. Actionable error messages (every error path in the Anthropic pipeline)

New pipeline/error_advice.py maps known upstream error shapes (status code + message pattern) to a short actionable suggestion, appended to the client-facing message as a Suggestion: line. Wired into:

• _handle_anthropic_error (non-streaming errors -> BackendAPIError -> JSON error response)
• _build_error_event (mid-stream SSE error events)
• the generic policy-execution 500 path

Design invariants:

• Raw upstream message is always preserved. Advice is appended after the original text, never substituted, so clients that parse error.type / error.message programmatically keep working and no information is destroyed. The Anthropic error response shape ({"type": "error", "error": {"type", "message"}}) is unchanged.
• Every status code gets advice (specific rules for 400 extra-field / max_tokens / credit-balance, 401, 403, 404 model-not-found, 413, 422, 429, 500, 503, 529; generic retry guidance otherwise).
• append_advice is idempotent (no double-appending if an error is formatted twice).
• Sanitized paths stay sanitized: with VERBOSE_CLIENT_ERRORS=false, internal exception details are still withheld; only the generic message + advice go out.

2. Retry-with-fix for a known fixable 400 pattern

New pipeline/request_repair.py handles the "invalid extra field" pattern: when the upstream API rejects a request with "<field.path>: Extra inputs are not permitted", the pipeline locates that field in the payload, strips it from a deep copy, and retries the backend call exactly once (_AnthropicPolicyIO.complete() and .stream()).

Safety rails:

• Observable, never silent: every repair emits a pipeline.retry_with_fix event (original error, removed field, session/user id) and logs a warning; the repaired backend request is also recorded via the normal pipeline.backend_request event, so the audit trail shows both attempts.
• One retry per request: the repaired call is not wrapped, so a second failure propagates through normal error handling.
• Streaming guard: retries only fire if the 400 arrived before any stream event was yielded; after that a retry would duplicate delivered events, so the error event is emitted instead (regression-tested).
• Protected fields: model, messages, max_tokens, stream are never auto-removed.
• Original request object is never mutated (policies and diff recording see true originals).

Tests

• tests/.../pipeline/test_error_advice.py (20 tests): advice lookup per status/pattern, raw-message preservation, idempotence.
• tests/.../pipeline/test_request_repair.py (8 tests): top-level and nested field stripping, protected fields, unresolvable paths, no mutation, field path embedded in longer SDK message.
• tests/.../pipeline/test_anthropic_processor.py (+5 tests in TestRetryWithFix): non-streaming retry succeeds with field stripped + pipeline.retry_with_fix emitted; retry capped at one attempt; unfixable 400 not retried; streaming pre-first-event retry; streaming post-event 400 NOT retried. Existing error-path tests extended to assert Suggestion: presence and that internal details stay sanitized.

Results: ./scripts/dev_checks.sh fully green (ruff, pyright 0 errors, 2873 unit tests passed). ./scripts/run_e2e.sh sqlite and ./scripts/run_e2e.sh mock both pass.

COE context (card source: COE audit 2026-03-25, PR #204)

• Root cause: the pipeline forwarded raw upstream error JSON verbatim and made no recovery attempt for recoverable 400s; PR Self-healing Anthropic request pipeline #204's self-healing design was closed unmerged, leaving both gaps open.
• Why it wasn't caught: error-path tests asserted only status codes and error types, not message usefulness; no test exercised a recoverable 400.
• Why it won't recur: the advice table is the single choke point for client-facing error text (new error paths route through append_advice), and TestRetryWithFix pins the retry contract (one attempt, observable event, streaming guard).

Follow-ups (out of scope, separate concern)

• Pre-existing scripts/run_e2e.sh bug on macOS: mktemp /tmp/mock-gateway-XXXXXX.json does not expand the template on BSD mktemp (suffix after XXXXXX is unsupported), creating a literal mock-gateway-XXXXXX.json file; the next run fails with "File exists". Deserves its own small fix PR.
• More fixable-400 patterns can be added to request_repair.py as they are observed (the module is pure and pattern-driven).

🤖 Generated with Claude Code

[claude]

Code Review — PR #799 (actionable errors + retry-with-fix)

Nice PR overall. The design is tight (single choke point for advice, pure repair module, one-retry cap, explicit streaming safety rail), tests are focused, and the invariants called out in the description are actually enforced in code. Below are concrete observations, ordered by importance.

Correctness

1. append_advice idempotence check is a substring match on "Suggestion:" (src/luthien_proxy/pipeline/error_advice.py:146). If an upstream error message ever happens to include the literal token Suggestion: (a user's message echoed back into a 400 error, an SDK wrapping that already added its own suggestion), advice will silently be skipped. Very unlikely in practice, but the check is cheap to harden — e.g. look for \n\nSuggestion: (the exact join string) rather than the bare prefix. Not a blocker, worth a follow-up.

2. max_tokens advice fires on any 400 whose message contains "max_tokens", including the "Extra inputs are not permitted" case for a field named max_tokens_v2 or similar (src/luthien_proxy/pipeline/error_advice.py:52). The rule ordering saves us — "Extra inputs are not permitted" matches first — but a 400 mentioning max_tokens in an unrelated way ("invalid ratio of max_tokens to output length") would still get the "lower it and resend" advice. Consider tightening to max_tokens.*(exceed|maximum|limit|too large) or similar. Minor.

3. Regex allows leading digits and dashes in top-level segment names (src/luthien_proxy/pipeline/request_repair.py:26, [A-Za-z0-9_][A-Za-z0-9_.\-]*). Anthropic's real field names don't start with digits or contain dashes, but the pattern is permissive enough that adversarial upstream error text ("0.important: Extra inputs...") could theoretically produce odd path resolutions. Given the safety net (path must resolve in the payload before stripping) this is defensive-only, but worth noting.

4. Protected-field guard only checks single-segment paths (request_repair.py:72). If Anthropic ever returned a nested path resolving through a top-level protected key (e.g. messages.0 as a leaf key stripped by index-into-list logic), the guard wouldn't catch it. In practice the leaf-key check on the parent (if not isinstance(container, dict) or leaf not in container: return None) already blocks list-index leaves, so this is safe today — but the invariant is implicit rather than explicit. Consider either a comment tying the two together or extending the protected check to catch the nested case.

Minor / suggestions

5. _attempt_fixable_400_repair mutates self._request via set_request(fixed_request) (anthropic_processor.py:246, :292). Documented and reasonable — but this means anything downstream reading io.request after a retry sees the repaired payload as the "current" request. That's the intended semantics for transaction.request_recorded (final_request reflects the repair), but if any observer downstream reasoned "io.request is what the policy handed us," it would now be wrong. Worth a one-line comment on set_request or a distinct set_repaired_request method for grep-ability.

6. Duplicate _record_backend_request on retry emits two pipeline.backend_request events — that's by design per the PR description (audit trail shows both attempts), but consumers of that event now see 2× the volume for repaired requests. Downstream metrics that count pipeline.backend_request as a proxy for "requests forwarded upstream" should be aware. Consider adding an attempt: 1|2 field to disambiguate.

7. _ANTHROPIC_STATUS_ERROR_TYPE_MAP has no entry for 413, so a 413 falls back to api_error at _build_error_event / _handle_anthropic_error — but you added a specific 413 advice line. The advice arrives but the error.type field is generic. Not a blocker (Anthropic doesn't emit 413 today), just an asymmetry in the maps.

8. get_error_advice docstring says "so callers can rely on always getting advice" — good invariant, but there's no test asserting that a None-status + empty message still returns a non-empty string. test_none_status_gets_generic_advice covers the None-status branch; a test with get_error_advice(None, "") would nail the invariant.

Testing

• Coverage is solid for the specific patterns targeted (idempotence, protected fields, nested paths, streaming pre/post-event, retry cap). The TestRetryWithFix streaming tests are especially valuable — they pin the "no retry after any event yielded" contract that would otherwise silently regress.
• Missing: a test for the SDK-embedded-message shape where the message string is JSON-quoted (e.g., '"tools.0.custom": Extra inputs are not permitted') — the regex handles this via the '" chars in the leading anchor, but no test verifies it. test_field_path_embedded_in_longer_message is close but uses unquoted JSON-string content.
• Missing: a test that pipeline.backend_request is emitted twice on retry — this is a documented audit-trail guarantee ("the audit trail shows both attempts") but only pipeline.retry_with_fix is asserted.

Security / privacy

• Raw upstream str(e.message) is passed through verbatim to the client for AnthropicStatusError (anthropic_processor.py:1361). This is pre-existing behavior and out of scope for this PR — but worth flagging that the VERBOSE_CLIENT_ERRORS=false sanitization only applies to the credential/connection/internal paths (which do go through client_error_detail). The status-error path still exposes whatever Anthropic returned. That's presumably fine (Anthropic's own message is what a direct client would see), but if you ever want end-to-end sanitization, this is the gap.
• No new secrets exposure paths introduced.

Performance

• attempt_request_fix does a copy.deepcopy on every fixable 400 — negligible for typical payloads, but if a large messages array + tools gets rejected repeatedly, the deep-copy dominates the retry. Not worth optimizing until it shows up.

Quality of the design

The two design invariants — "raw message always preserved" and "one retry max, observable" — are enforced by structure (single append_advice choke point; the repaired call in .complete() is not recursively wrapped) rather than by convention. That's the right shape for something this security-adjacent. The pipeline.retry_with_fix event carrying both the original error and the removed field means an operator auditing after the fact can reconstruct exactly what happened.

Nits

• request_repair.py:75: repaired: dict = copy.deepcopy(dict(request)) — the dict(request) wrapper is redundant since request is already a dict at runtime (TypedDict). copy.deepcopy(request) reads cleaner.
• error_advice.py:44: comment "First match wins" is accurate but combined with "A None pattern is the fallback for that status code, so pattern-specific rules must come before their status fallback" would be clearer as an invariant asserted in code (or via a test that regenerates the ordering).

Overall: 👍 ship. The follow-ups above are minor.

Reviewed by Claude (Opus 4.7)

[claude]

Code review — PR #799

Focused pass on the three new modules + touch-ups in anthropic_processor.py. Overall this is a well-scoped, well-tested change that respects the stated invariants. Findings below are minor — no blockers.

Strengths

• Design invariants are enforced, not just documented: raw upstream message preserved (append vs replace), idempotence guard against double-appending (SUGGESTION_PREFIX in message), one retry max (repaired call not wrapped), streaming guard (yielded_any before catch), protected-fields frozenset, and deep-copy so the original request is never mutated. Each is pinned by a test.
• Repair is observable, never silent: pipeline.retry_with_fix event + warning log, and the repaired request is captured via the normal pipeline.backend_request event. Good audit-trail hygiene.
• request_repair.py is pure — no I/O, easy to reason about, cleanly separated from the pipeline mutation point in _AnthropicPolicyIO.
• Fallback advice for every status code so no error path silently loses actionable text.
• Test coverage is thorough: 33 new/updated tests covering happy path, streaming pre-first-event vs post-event, retry cap, unfixable 400, protected fields, unresolvable paths, and (importantly) sanitized-path assertions that raw exception text does not leak when VERBOSE_CLIENT_ERRORS=false.

Concerns / suggestions (all minor)

1. transaction.request_recorded captures the pre-repair request as final_request (anthropic_processor.py:156-174).

On retry, _record_backend_request(fixed_request) calls ensure_request_recorded(fixed_request) — but the guard flag is already set from the first attempt, so the call no-ops. The transaction.request_recorded event's final_request field ends up showing the original request that failed, not the repaired request that succeeded. The per-attempt pipeline.backend_request events + pipeline.retry_with_fix do capture the truth, so the audit trail is complete — just not from a single event. Consider either allowing ensure_request_recorded to overwrite on repair, or explicitly documenting this precedence in the observability data model so consumers know to prefer pipeline.backend_request for "what was actually sent."

2. _EXTRA_FIELD_PATTERN boundary set is narrow (request_repair.py:26).

The leading boundary character class accepts whitespace, quotes, backtick, or open-paren before the field path. It does not include ,, [, {, or <. A message formatted like [banana_mode: Extra inputs are not permitted] (leading [) would silently miss the match. Real Anthropic messages don't use those wrappers today, so this is a latent risk more than a defect. The safety net (verify the field actually resolves in the payload before removing) means the failure mode is "no repair attempted," not "wrong field removed." Worth a comment explaining why the set is intentionally narrow, or expanding it if you want more coverage.

3. append_advice idempotence check is a substring test (error_advice.py:146).

if SUGGESTION_PREFIX in message will skip advice-appending if an upstream message happens to contain the literal string Suggestion: for any reason (unlikely from Anthropic, more plausible if a policy rewrites the message). Corner case, low impact — probably fine, but a marker-style check (e.g. verifying the message ends with the expected sentinel and advice, or an explicit "already-annotated" flag) would be more precise.

4. INTERNAL_ERROR_ADVICE phrasing (error_advice.py:28-31).

Advising "Retry once" for internal errors is fine for transient issues but may mislead users when the error is a persistent code bug. Consider "the issue may be transient — retry once" or similar to hedge. Style/UX only.

5. Deep copy is unconditional (request_repair.py:75).

The deepcopy runs before path resolution, so an unresolvable-path return-None case still pays the copy cost. Only fires on 400s so effectively free — style-only.

Style / conventions

• Docstrings correctly focus on WHY and non-obvious behavior (matches CLAUDE.md guidance).
• Tests are stateless, linear, use parametrization for the advice table — aligns with the unit-test guidelines.
• No mocking of the DB or Redis in the new tests; retry-with-fix tests exercise the real pipeline flow with only the AnthropicClient and FastAPI request mocked.

Not seen in this PR (out of scope, but worth tracking)

• Second failure of the repaired request is asserted for non-streaming (test_non_streaming_retry_capped_at_one_attempt) but not explicitly for streaming. The code path is symmetric so it likely works, but a streaming counterpart would tighten the contract pin.
• Additional fixable-400 patterns could plug into attempt_request_fix — the module is already shaped for it.

Nice work. LGTM once #1 is either fixed or explicitly documented.

🤖 Generated with Claude Code

[claude]

Code review

Overall this is a solid, well-scoped PR. The invariants are stated up-front and the tests enforce them; the two modules (error_advice.py, request_repair.py) are pure and easy to reason about; the observability wiring (pipeline.retry_with_fix + double pipeline.backend_request events) is thoughtful. Nice work.

A handful of things worth considering before merging — none blocking, one worth thinking about.

Potential issue: transaction.request_recorded.final_request freezes the pre-repair payload

_AnthropicPolicyIO.ensure_request_recorded early-returns when self._request_recorded is True, so the event is emitted exactly once with whichever request is passed on the first call. In the retry path (anthropic_processor.py:227-253), the first _record_backend_request(final_request) fires with the original (unrepaired) request, marking it as the "final" one; the subsequent repaired call records another pipeline.backend_request but the transaction.request_recorded event still shows final_request=<request with banana_mode>. Audit consumers that only look at transaction.request_recorded will believe the pipeline sent the bad field to the backend as the final decision. Two options:

• Update the event on retry (drop the _request_recorded gate for the retry path, or emit a "corrected" event).
• Document that consumers must join with pipeline.backend_request for the truthful final payload.

Given the COE-driven audit-trail rationale in the PR body, I'd lean toward fixing the event.

Test gap: streaming, unfixable 400 before first event

TestRetryWithFix covers non-streaming unfixable (test_non_streaming_unfixable_400_is_not_retried) and streaming post-event (test_streaming_fixable_400_after_events_is_not_retried), but no test asserts that a streaming 400 raised before any event whose message doesn't match _EXTRA_FIELD_PATTERN propagates without a retry. That's the natural symmetric case and would pin the streaming branch's if fixed_request is None: raise line.

Consider: _PROTECTED_TOP_LEVEL_FIELDS guards only top-level fields

request_repair.py:75 only protects at the top level. A hypothetical messages.0.role: Extra inputs are not permitted would delete role from messages[0] and produce an invalid retry. The API is unlikely to report a required field as "extra input" — but if you want fully defensive behavior, add a per-segment protected set (e.g., inside messages.*, don't remove role, content, type). Optional hardening.

Minor: _INVALID_REQUEST_ADVICE wording assumes a named field

"Fix the field named in the message above and resend."

For 400s like "messages: roles must alternate" (see test_non_streaming_unfixable_400_is_not_retried), no specific field is named — the advice reads awkwardly. Softening to something like "Adjust the request based on the message above and resend." would generalize better.

Minor: DoS amplification (bounded, but worth naming)

Each request that trips the fixable pattern doubles backend calls. Cap is 1 retry, so worst case is 2x per request — not a real amplification vector, but a hostile client that repeatedly sends bogus fields does inflate backend costs. Probably fine to note and move on; a metric on pipeline.retry_with_fix frequency would make it observable.

Nits

• error_advice.py:44 — the _ADVICE_RULES docstring says "First match wins" but rules are ordered so specific patterns precede the (status, None, …) fallback. Consider a short comment on the fallback rows (# 400 fallback — must come after pattern-specific 400 rules) to make the ordering constraint explicit at each site, so future edits don't accidentally reorder into a shadowed rule.
• request_repair.py:28 — [A-Za-z0-9_.\-]* allows both dots (segment separator) and dashes; a segment name containing a literal dot would be misinterpreted. Anthropic field names don't have dots in practice, but a comment noting the assumption would help.
• anthropic_processor.py:246-248 — the comment says "One retry max: the repaired call is not wrapped, so a second failure propagates". True, but it took a second to see. Could tighten to # No wrap → second failure propagates to normal error handling (single-retry contract).

What I like

• Idempotence check in append_advice uses the exact join string, not just the prefix — the test test_upstream_message_mentioning_suggestion_still_gets_advice is the right regression to pin that.
• attempt_request_fix deep-copies via copy.deepcopy(dict(request)) and asserts non-mutation via a test. Correct.
• Streaming retry gated on yielded_any is the important safety property; the test_streaming_fixable_400_after_events_is_not_retried test locks it in.
• Raw upstream error.type and message are preserved — programmatic clients that key off error.type keep working.

🤖 Reviewed by Claude Opus 4.7

[scottwofford]

---

Post-review note (adversarial verification pass, Jul 7): this PR and its sibling both add a one-shot upstream retry to the same two methods in anthropic_processor.py (#797 passthrough-fallback, #799 retry-with-fix). git merge-tree off the shared base shows 4 real conflict hunks, and merged naively both retries can fire on the same 400. Merge ONE first, then rebase the other and reconcile the retry semantics (order: repair-then-fallback, or unify into one retry ladder) before merging the second.

[scottwofford]

Claude-generated merge-queue triage of all open Luthien PRs, requested by Scott (Jul 7, 2026). Advisory only; Scott has not yet acted on these recommendations.

Recommendation: close; salvage the error-advice half separately if wanted.

The retry-with-fix half is always-on, ungated client-request mutation: any 400 matching "Extra inputs are not permitted" gets the offending field stripped from the client's own request and retried (logged and evented, but not configurable). That conflicts with three settled positions at once: the transparency contract pinned in #798 (whose cache-control-extra-field fixture is exactly the pattern this repair strips, meaning it would disable caching a client explicitly opted into), the closure of #204, and the default-off design #797 was deliberately built around. It also hard-conflicts with #797 in anthropic_processor.py, with a double-retry hazard if both landed.

The first half (error_advice.py, appending a Suggestion: line while preserving the upstream error) is separable, does not mutate requests, and maps to its own Trello card; it could be re-landed as a small default-off PR if that card is still wanted.