Skip to content

feat: opt-in passthrough fallback when a policy modification causes an upstream 4xx - #797

Open
scottwofford wants to merge 2 commits into
mainfrom
feat/passthrough-fallback
Open

feat: opt-in passthrough fallback when a policy modification causes an upstream 4xx#797
scottwofford wants to merge 2 commits into
mainfrom
feat/passthrough-fallback

Conversation

@scottwofford

@scottwofford scottwofford commented Jul 7, 2026

Copy link
Copy Markdown
Member

Implements Trello: COE audit passthrough fallback (P1 follow-up from the 2026-03-25 COE audit of PR #204).

Summary

Opt-in passthrough fallback: when a policy-modified request is rejected upstream with a request-shaped 4xx, the gateway retries once with the original unmodified request. Design principle from the card: the proxy should never make things worse than direct API access.

This is deliberately the narrow slice of PR #204 that survived its review. #204 bundled sanitization and auto-fix layers that imposed unrequested changes to the client-server interaction and was closed. Passthrough fallback is the opposite move: on failure it removes the proxy's own modifications and sends exactly what the client sent, rather than adding new ones. No sanitization, no pattern-matched auto-fixes, no error rewriting.

Trigger condition (all must hold)

  1. PASSTHROUGH_FALLBACK_ENABLED is on (default: off, see tradeoff below)
  2. The backend call raised AnthropicStatusError with status in {400, 404, 413, 422} (request-shaped failures). Excluded: 401/403 (credential-scoped, a different body won't change the outcome), 429 (retry amplifies load under throttling), 5xx/529 (SDK already retries server errors)
  3. The request sent upstream differs from a pristine deepcopy snapshot of the request as it entered the policy. If the policy didn't change the request, direct API access would have failed identically and a retry is pure waste
  4. Streaming only: zero backend events received yet (failure at stream connect). There is no mid-stream recovery; once events have flowed, re-sending would duplicate content

Exactly one retry. If the original request also fails, that error propagates: the client sees exactly what direct API access would have returned.

Why intentional blocks cannot be overridden

Checked before choosing the trigger: policies in this codebase block by rewriting responses (ToolCallJudgePolicy replaces tool_use blocks with block-message text), emitting synthetic events, or raising from a hook (fail-secure judge pattern). None of these surface as an upstream AnthropicStatusError from the backend call. The fallback lives inside _AnthropicPolicyIO.complete()/stream() at the backend-call site and triggers only on upstream status errors, so it is structurally unable to fire on a policy block. Two unit tests pin this down (test_intentional_response_block_is_untouched, test_policy_raised_error_is_not_a_fallback_trigger).

Observability

Fallback is recorded BEFORE the retry is attempted, so a policy failure is never silently masked:

  • pipeline.passthrough_fallback event (status code, error message, session/user id) via the emitter, visible in the activity monitor
  • WARNING log with call id
  • luthien.passthrough_fallback = true attribute on the send_upstream span
  • The retry itself is recorded as a second pipeline.backend_request, so the event trail shows both attempts

Load-bearing tradeoff: default off

Falling back means sending the request WITHOUT the policy's modifications. For a policy that rewrites requests for safety (e.g. redaction), that is fail-open: a bug in the policy would leak exactly what the policy exists to remove. For an AI-control proxy, fail-closed (surface the error) is the safer default, so the flag ships off and an operator opts in per deployment (PASSTHROUGH_FALLBACK_ENABLED=true, or live via the config dashboard / admin API, no restart needed). Flipping the default later is a one-character change in config_fields.py.

Related judgment call, flagged for review: the "original request" snapshot is taken as the request enters the policy, i.e. after the INJECT_POLICY_CONTEXT system-prompt injection. Fallback removes what the active policy changed, not the injection. Strictest reading of "never worse than direct API access" would fall back to the raw client body; that would also silently disable policy-context injection and is left out of scope (the card scopes this to policy modifications).

When disabled the request path is unchanged (the snapshot deepcopy is only taken when enabled), so no-op stays no-op.

Files changed

File Change
src/luthien_proxy/pipeline/anthropic_processor.py Fallback in _AnthropicPolicyIO.complete()/stream(), trigger predicate, observability
src/luthien_proxy/config_fields.py passthrough_fallback_enabled field (db-settable, no restart)
src/luthien_proxy/settings.py, .env.example Regenerated
tests/luthien_proxy/unit_tests/pipeline/test_anthropic_processor.py 16 new unit tests (TestPassthroughFallback)
tests/luthien_proxy/e2e_tests/test_mock_passthrough_fallback.py 3 mock e2e tests against the real gateway + mock backend
changelog.d/passthrough-fallback.md Changelog fragment

Test plan

  • Unit: 103 passed in test_anthropic_processor.py (16 new: fallback fires on 400 with modified request; disabled by default; unmodified request never retries; 401/403/429/500/529 never fall back; retry failure propagates; in-place policy mutation detected via deepcopy snapshot; streaming connect-failure fallback; mid-stream never falls back; intentional blocks untouched)
  • Mock e2e: 211 passed (3 new: modified request 400s then original forwarded and 200 returned to client with both backend requests asserted; default-off propagates the 400 with exactly one backend request; streaming fallback delivers a clean SSE stream of the original request)
  • sqlite e2e: 47 passed
  • ./scripts/dev_checks.sh passes (format, lint, pyright, unit tests, complexity)

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

CHANGELOG reminder — This PR has no changelog fragment.

Add a file to changelog.d/ (see changelog.d/README.md for format).
If this is a chore/infra-only change, add the skip-changelog or chore label to suppress this reminder.

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Status: No Changes to Review

This draft PR currently contains only the objective-setting empty commit (chore: set objective to implement opt-in passthrough fallback for policy 4xx). There are 0 files changed, 0 additions, and 0 deletions, so there is nothing to substantively review yet.

Happy to review once implementation lands. A few things I'll be watching for when the code arrives, given the objective ("opt-in passthrough fallback when a policy modification causes an upstream 4xx"):

Correctness / semantics

  • Attribution of the 4xx: how will the code decide the 4xx was caused by the policy modification vs. a legitimate client-side error the upstream would have returned regardless? Blindly retrying every 4xx with the unmodified request risks:
    • Bypassing the policy for prompts that were legitimately 400-worthy (masking real client bugs).
    • Turning the policy into an advisory layer instead of an enforcement layer — the whole point of a blocking policy is that it can refuse.
  • Which 4xx codes are eligible? 400 (schema drift from policy edits) is the plausible one. 401/403 (auth) and 429 (rate limit) should never trigger passthrough. Please explicitly enumerate.
  • Streaming: 4xx can arrive after event: message_start in some SSE flows. Make sure the fallback path handles both "4xx on the HTTP response" and "error event mid-stream" cases, and doesn't emit a partial event stream followed by a fresh one.
  • Idempotency / side effects: if the policy already emitted observability events, ran judge calls, or logged to request_logs before the upstream 4xx, the fallback retry needs a clear story about duplication (retry a second request row? amend the first? emit a fallback_triggered event?).

Opt-in surface

  • Where does the opt-in live — per-policy config, per-request header, or a gateway-wide config field? Per-policy config is the most auditable; header-based opt-in is easy to abuse. If it's a config field, it belongs in config_fields.py (see CLAUDE.md "Configuration System") and needs a matching entry in the generated settings + .env.example.
  • Default should be off. Users have to explicitly acknowledge they're trading enforcement for availability.

Security

  • Passthrough of the original unmodified request is exactly the failure mode a red-team would want. Please make sure this can't be triggered by an attacker who can influence the upstream 4xx (e.g. by crafting a prompt that reliably produces a 400 → policy silently drops out).
  • Log every fallback event loudly (structured event, dashboard-visible). Silent fallback is a security-critical misconfiguration waiting to happen.

Tests (per CLAUDE.md "Test Requirements")

  • Unit tests for the fallback decision logic (which 4xx codes, which conditions).
  • Integration test in sqlite_e2e or mock_e2e tier that:
    • Confirms policy modification → 4xx → unmodified retry → success path.
    • Confirms 4xx unrelated to policy (auth, rate limit) does not trigger fallback.
    • Confirms opt-in-off → 4xx surfaces to client unchanged.
  • Streaming variant of the above.

Docs / changelog

  • changelog.d/ fragment.
  • Note the security tradeoff prominently wherever the opt-in is documented — this is an availability-vs-enforcement knob and users need to understand what they're turning off.

Ping me once there's a diff and I'll do a real review.

…n upstream 4xx

Implements https://trello.com/c/kRPRjGUx (COE audit follow-up to PR #204).
Design principle: the proxy should never make things worse than direct
API access.

Trigger (all must hold):
- PASSTHROUGH_FALLBACK_ENABLED is on (default off)
- upstream AnthropicStatusError with status in {400, 404, 413, 422}
- the request sent differs from the pristine pre-policy snapshot
- (streaming) zero backend events received yet

The fallback lives at the backend-call site (_AnthropicPolicyIO), so
intentional policy blocks — which are response rewrites or policy-raised
errors, never upstream errors — structurally cannot be overridden.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@scottwofford
scottwofford marked this pull request as ready for review July 7, 2026 05:57
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Code Review: Passthrough Fallback

Overall this is a clean, focused PR that lands a well-scoped follow-up to the PR #204 audit. The trigger predicate is conservative in the right ways, the deep-copy snapshot is the right shape for the in-place-mutation case, and the observability + test coverage are solid. Findings below are minor.

Correctness observations

1. transaction.request_recorded still describes the modified request even when fallback sent the original.
ensure_request_recorded is one-shot (guarded by self._request_recorded), so on the fallback path only the first call fires — with final_request set to the modified request. The second _record_backend_request(fallback_request) in the fallback branch only emits pipeline.backend_request and the request-log row. Any consumer reading only the transaction.request_recorded event will believe the modified request reached the backend, when in fact the fallback body was sent. The ground truth is recoverable by cross-referencing the pipeline.passthrough_fallback and second pipeline.backend_request events, but that's a load-bearing correlation. Consider either:

  • adding the fallback request into the pipeline.passthrough_fallback event payload (so it's self-contained), or
  • documenting the correlation contract in the fallback event's summary.

Not blocking — but worth being explicit about for downstream observability.

2. Pre-existing in-place-mutation risk on _initial_request (out of scope, flagging for context).
self._initial_request = initial_request in __init__ doesn't copy, so if a policy mutates the request dict in place, dict(self._initial_request) inside ensure_request_recorded sees the mutated payload — meaning original_request == final_request in the emitted transaction event. This PR is unaffected (the fallback deepcopy is a separate protected snapshot) but the transaction event's usefulness for detecting policy modifications is quietly weakened. Might be worth a follow-up ticket to always deepcopy _initial_request at construction, mirroring what this PR does for the fallback snapshot.

3. Streaming: except AnthropicStatusError inside the yield-loop could theoretically catch a caller-athrown exception.

async for mse in _iterate(final_request):
    events_yielded += 1
    yield mse

If a downstream consumer of _stream() calls .athrow(AnthropicStatusError(...)) at the yield point, it would enter the except block. In practice events_yielded > 0 at that point and the if events_yielded: raise short-circuits, so this is safe. But it's the sort of coincidence that's worth a one-line comment ("events_yielded > 0 check also prevents re-entering fallback if the consumer athrows this exception type") because a future edit that moves the counter increment could silently break the invariant.

Nits

  • str(exc.message) in _record_passthrough_fallback is redundant — APIStatusError.message is already a str. Not worth blocking.
  • if sent_request == original is deep dict equality, which is O(n) over the payload. Fine here because it's only on the error path, but noting it in case the predicate ever moves to a hot path.
  • The comment above _PASSTHROUGH_FALLBACK_STATUS_CODES is excellent — reads exactly like a design doc's rationale section and future editors will thank you.

Test coverage

Comprehensive. The parametrized test_non_request_shaped_errors_never_fall_back [401, 403, 429, 500, 529] closes the door cleanly on the excluded status codes, test_in_place_policy_mutation_is_detected pins down the whole reason the snapshot is a deepcopy rather than a dict(), and test_intentional_response_block_is_untouched + test_policy_raised_error_is_not_a_fallback_trigger are exactly the right pair to nail the "structurally cannot override a block" claim from the PR body.

One suggestion: add a test asserting the pipeline.backend_request event count is 2 on fallback and 1 without it. test_modified_request_400_falls_back_to_original currently asserts client.complete.call_count == 2 (the wire-level effect) but not the event-level count — and future changes to _record_backend_request's emission behavior wouldn't get caught by the existing test.

Design principle

Falling back inside _AnthropicPolicyIO.complete()/stream() at the backend-call site is exactly the right layer — high enough to see the modified vs. original request pair, low enough that intentional policy blocks (which surface as response rewrites or hook-raised errors, not upstream AnthropicStatusError) structurally cannot trigger it. The PR body's argument for this is convincing and the tests back it up. The default-off decision with the fail-open tradeoff spelled out in the field description is the right call for an AI-control proxy.

Style / conventions

  • Config field follows the established ConfigFieldMeta pattern with db_settable=True, restart_required=False for the live-toggle path via the admin dashboard — matches dogfood_mode, inject_policy_context.
  • Docstrings are appropriately Google-style; the WHY-focused comments on _PASSTHROUGH_FALLBACK_STATUS_CODES and the deepcopy snapshot follow the CLAUDE.md convention nicely.
  • Changelog fragment present, .env.example regenerated. Good hygiene.

LGTM with the observability note above worth considering before merge. Nothing here is blocking.

@scottwofford

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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: merge, after #799 is closed (see the comment there).

Verified in the diff: PASSTHROUGH_FALLBACK_ENABLED defaults to false, the request snapshot is only taken when the flag is on (a true no-op stays a no-op, which was the objection that soft-closed the original card), the trigger set is conservative (policy-actually-modified, 400/404/413/422 only, streaming pre-first-event only, exactly one retry), and the fallback is recorded before retrying so policy failures are not masked. Default-off means zero change for existing deployments; it exists as an opt-in escape hatch for when a policy modification breaks a request. To be clear, this is new scope rather than a bug fix, but it is the defensible sibling of the #797/#799 pair.

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