3523dcf93e
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.
Now `grep -H '^Status:' plans/*.md` is the answer:
50 done 3 in progress 2 planned 2 reference 1 parked
Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.
The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.
test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.
Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
135 lines
7.8 KiB
Markdown
135 lines
7.8 KiB
Markdown
# 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.
|