6krrt/plans/pinch-relevance-and-failover-review.md

Review: embedding-based pinch relevance + upstream failover/circuit breaker (commit 03f62e2)

Status: done -- review of shipped work

What it was reviewing: opencode/Atlas's implementation of both plans/pinch-embedding-relevance.md and plans/upstream-failover-and-circuit-breaker.md in one commit, per the plans/.omo/plans/pinch-embedding-relevance.md work plan approved earlier. Verified against the actual diff (git show 03f62e2) rather than the commit message, and ran the full suite directly: 673 passed (up from 562).

Verdict: correct and faithful except one integration gap the delivered tests don't catch

Pinch embedding relevance — correct

• order_by_relevance (context_prune.py): ascending cosine similarity, correct empty/single-candidate handling.
• trim_candidates: extracted cleanly from the old inline logic; both branches (no-user-turn path, num_protected_turns <= 0 path) reproduced exactly, confirmed by diffing line-for-line against the pre-existing prune_context body.
• prune_context(relevance_order=...): the None branch is a byte-for-byte reproduction of the original uniform-compression code — confirmed by direct diff, not just the docstring's claim. The relevance-ordered branch is structurally bounded to never compress more candidates than the uniform pass: it walks a single permutation of the same candidate list and stops early, so its absolute worst case degrades to exactly today's behavior, never worse. A minor imprecision exists in the savings estimate used to decide when to stop (compares len(text) where the final compression step uses len(prose)), but it cannot violate the "never compress more than today" invariant and isn't worth a fix on its own.
• _embed_for_relevance (dispatcher.py): fails closed to None on every path checked — RequestException, non-200, unparseable JSON, missing or malformed embedding vectors — each logging relevance_unavailable and never raising. Matches the spec's core safety requirement exactly.
• _relevance_order_for: gates on cfg.pinch.enabled and cfg.pinch.relevance.enabled and min_candidates, uses _text_only (not extract_text) for candidate text and _last_user_text for the query — both match the decisions adopted at approval time.
• config.yaml/config.py: PinchRelevanceConfig ships enabled: false, matching spec verbatim.

Upstream failover — correct, including the hard architectural part

• Non-streaming loop (dispatcher.py, the while True: retry loop): on a >=400 response, retries the next alternatives candidate instead of raising immediately, and does not touch attempts_used — confirmed by reading the actual diff hunk, not inferring it from a comment. The quality retry budget and the availability failover are correctly kept separate.
• Streaming path: this was the part of the spec hardest to get right, and it's genuinely done, not superficially matching the words. The old requests.post(...) call that used to live inside proxy() was removed entirely; a new pre-flight loop opens and status-checks each candidate via _open_upstream before StreamingResponse is ever constructed, closing discarded failed connections (attempt.close()), and proxy() now takes the already-open healthy connection as a parameter instead of opening its own. A dead replica genuinely never reaches the client as a 200 with a broken body.
• routing.py's exclude_models filter: a clean 4-line addition matching the existing exclude_stale/exclude_deprecated shape exactly, default frozenset() so it's a no-op when unpopulated, no new import into routing.py (the exclusion set is computed in dispatcher._open_circuits and passed in, exactly as specified).
• circuit_breaker.py itself is correct and matches session_cache.py's pure/injected-time pattern precisely: is_down, record_failure (exponential backoff, capped at max_cooldown_seconds), record_success, clear are all implemented as specified and unit-tested in isolation (tests/test_circuit_breaker.py).
• config.yaml ships circuit_breaker.enabled: false with the exact cooldown values from the spec (30 / 600 / 2.0). The failover retry itself is correctly unconditional — only the circuit-breaker bookkeeping (record_failure) is gated behind cfg.circuit_breaker.enabled — matching the spec's explicit recommendation to ship failover default-on since its worst case matches today's behavior exactly.

record_success is never called — the one real gap

File: dispatcher.py. circuit_breaker.record_success is defined correctly (circuit_breaker.py:57) and unit-tested in isolation, but a repo-wide grep for record_success turns up exactly two hits: its own definition and its own isolated unit test. It is called from nowhere in the actual dispatch path — not the non-streaming loop, not the streaming pre-flight loop, only mentioned in a docstring comment (dispatcher.py:1843, "...becomes the probe that can clear the entry via record_success") that describes behavior the code next to it doesn't implement.

Consequence. record_failure's cooldown math is min(prev.cooldown_seconds * backoff_multiplier, max_cooldown) — it only ever looks at whatever is currently stored, with no signal that a success happened in between. Without record_success clearing the entry, any model with a failure history has its cooldown monotonically ratchet toward max_cooldown_seconds on every subsequent failure, even failures separated by long stretches of trouble-free service. That is the opposite of "passive recovery rebuilds trust," which was the explicit point of the design (plans/upstream-failover-and-circuit-breaker.md: "a success clears the entry entirely... the next failure after a success restarts at initial_cooldown_seconds, not wherever the backoff had climbed to").

Why it passed a green suite. T12's own acceptance criteria (.omo/plans/pinch-embedding-relevance.md) explicitly required asserting .record_success was called "with the survivor" — that assertion was never written. There is no integration test anywhere at the chat_completions level for the actual failover path; only the isolated helpers (_open_circuits, _embed_for_relevance, _open_upstream) and circuit_breaker.py's own unit tests are covered (tests/test_dispatcher_helpers.py, tests/test_circuit_breaker.py). The gap is invisible to the delivered tests because nothing exercises the real wiring end to end.

Severity. Dormant today — circuit_breaker.enabled: false by default, so nothing is affected until it's turned on. Once enabled, the "skip a dead model for a while" behavior still works correctly (is_down's time comparison doesn't need record_success to function), but the backoff-resets-after-recovery half of the design does not, which would show up as a real model gradually becoming permanently circuit-shy over the service's lifetime rather than what was actually specified.

Fix. Call circuit_breaker.record_success(current_model, provider) after a successful (< 400) response in the non-streaming loop, and after the streaming pre-flight loop confirms a healthy connection — both gated by cfg.circuit_breaker.enabled, matching how record_failure is already gated. Then add the integration test T12 already called for: a 503 on the first candidate, 200 on the second, asserting record_failure fired for the first and record_success for the second.

Recommendation

Land everything else as-is — it's a faithful, carefully verified implementation of both specs, including the one part (streaming failover) that was genuinely tricky to get right. Fix record_success before flipping circuit_breaker.enabled: true in any real deployment; it's a two-line addition plus the one integration test that would have caught its absence.