# 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.