From a35269deb2349f0e7f398c55316cb5b180016f34 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 11:53:47 +0000 Subject: [PATCH 01/12] docs(adr): ADR-0005, replace fixed sidecar max_tokens with per-candidate readiness Direct owner critique after #1436's max_tokens 16->4096 raise moved the sidecar's gateway preflight failure from empty-content to a 120s zero-byte timeout: a single hardcoded max_tokens cannot fit a heterogeneous orchestrator/free pool on two independent axes (reasoning- token overhead per model, and each model's own real max_tokens ceiling). Checked directly against contextual-orchestrator source rather than assumed: no caller-facing lever separates a reasoning budget from a content budget on the endpoints this preflight/Strix use (the field is a documented no-op on /v1/chat/completions and /v1/responses); ModelClient.probe()/provider_readiness_report() is a better-shaped, already-built per-candidate liveness mechanism but is admin-scoped while the sidecar's bearer token is inference-scoped; no per-model max_tokens/context-window ceiling is captured anywhere in DiscoveredModel or ModelAgent today, despite already-queried provider list endpoints publishing one. Decision: stop tuning one global constant. Move the sidecar's preflight to a bounded per-candidate probe with an N-of-M "at least one route works" threshold instead of one request that must succeed. Two upstream contextual-orchestrator asks (inference-scoped readiness probe; real per-model token-ceiling discovery data) are tracked as follow-ups, not closed here. No sidecar code change in this PR -- the migration itself is tracked separately. Co-Authored-By: Claude --- CHANGELOG.md | 15 ++ .../0005-sidecar-preflight-token-budget.md | 232 ++++++++++++++++++ docs/product-technical-gap-baseline.md | 32 +++ 3 files changed, 279 insertions(+) create mode 100644 docs/adr/0005-sidecar-preflight-token-budget.md diff --git a/CHANGELOG.md b/CHANGELOG.md index fc84661ed6..53e590b62c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,21 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Add `docs/adr/0005-sidecar-preflight-token-budget.md`, an evidence-based + design decision responding to the owner's direct critique that a single + hardcoded `max_tokens` cannot fit a heterogeneous `orchestrator/free` pool + (varying reasoning-token overhead per model, and a genuinely different real + ceiling per model). Checked directly against `contextual-orchestrator` + source: no caller-facing way exists to separate a reasoning budget from a + content budget on the endpoints this preflight/Strix use; + `ModelClient.probe()`/`provider_readiness_report()` is a better-shaped, + already-built per-candidate liveness mechanism but is `admin`-scoped while + the sidecar's token is `inference`-scoped; no per-model token-ceiling data + is captured anywhere in discovery today. Decision: stop tuning one global + constant and move the sidecar's preflight to a bounded per-candidate probe + with an N-of-M "at least one route works" threshold, with two narrower + upstream asks tracked as follow-ups rather than closed here. No code + change in this PR; the sidecar migration is tracked separately. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md new file mode 100644 index 0000000000..2eb3a17457 --- /dev/null +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -0,0 +1,232 @@ +# ADR-0005: Replace the sidecar's single-shot, fixed-`max_tokens` gateway preflight with per-candidate readiness + +- Status: proposed +- Date: 2026-08-30 +- Scope: `ContextualWisdomLab/.github` central review pipelines' vendored `contextual-orchestrator` + sidecar (`scripts/ci/contextual_orchestrator_review_sidecar.sh`), and a small upstream request to + `ContextualWisdomLab/contextual-orchestrator`. +- Decision: Stop trying to pick a single, universally-correct `max_tokens` value for the sidecar's + post-`healthz` gateway preflight. Replace the one hardcoded-budget completion request against the + virtual `orchestrator/free` pool with a bounded, per-candidate probe over the catalog the sidecar + already builds, using an **N-of-M "is at least one route usable" threshold** instead of a single + request that must succeed. Track upstream extension of `contextual-orchestrator`'s existing + `ModelClient.probe()` / `provider_readiness_report()` machinery with an inference-scoped variant, and + track a separate, real per-model `max_tokens` ceiling in model discovery, as two follow-ups this ADR + does not itself close. +- Ownership: `.github` owns the sidecar script and this ADR; `ContextualWisdomLab/contextual-orchestrator` + owns the gateway internals cited as evidence and the two follow-up asks. +- Figma File ID: N/A (no customer UI). + +## Context + +`scripts/ci/contextual_orchestrator_review_sidecar.sh`'s gateway preflight sends one +`POST /v1/chat/completions` request against the virtual `orchestrator/free` model — +`{"model":"orchestrator/free","messages":[...,"Reply with just 'OK'."],"max_tokens":,...}` — and +fails the whole sidecar (blocking `noema-review`/`opencode-review`/`strix` org-wide) unless that one +request returns non-empty `choices[0].message.content` within a fixed `curl --max-time`. + +`N` has already been tuned twice in this investigation: 16 → 4096 (#1436), moving the failure from +"empty content at 16 tokens" (the provider's response consumed the whole budget on internal reasoning +before emitting visible content — see `ModelClient._response_content`'s own anticipated error message, +quoted below) to "120s timeout with zero bytes at 4096 tokens" on a separate run. Direct owner feedback +in response to that outcome, quoted verbatim because it is the reason this ADR exists: + +> "max_tokens 이걸 고정하는 게 말이 안 되는데" — hardcoding this max_tokens doesn't make sense. +> "모델마다 max_tokens 허용치가 다 다른데" — each model has a genuinely different max_tokens allowance. + +`orchestrator/free` is a heterogeneous pool (`nvidia_nim`, `openai`, `opencode_zen`, `bytez`, +`openrouter`, ... — see `contextual_orchestrator_review_policy.py`'s `PROVIDER_FAMILIES`), and which +candidate a given preflight run draws varies (family-cap admission is deterministic but +alphabetical-by-provider-then-model, and the catalog itself changes over time). A fixed `max_tokens` +is wrong on two independent, evidenced axes for a pool like this: + +1. **Reasoning-token overhead differs per model.** A model that spends internal reasoning tokens before + emitting visible content can exhaust a small budget with zero visible output, or (with a much larger + budget) legitimately take far longer to finish than a fast, non-reasoning model would for the same + budget — this is very likely what turned #1436's 4096-token raise into a 120-second timeout instead + of a fix (see the 2026-08-30 gap-baseline entry's "third, distinct failure mode"). +2. **The provider's own hard ceiling on `max_tokens`/`max_completion_tokens` differs per model.** Some + providers reject a request outright (400) if `max_tokens` exceeds what that specific model supports; + others support far more than a generic constant would ever request. A single number can therefore be + simultaneously too small for one model's reasoning overhead and too large for another model's real + ceiling — there is structurally no number that is not wrong for some member of the pool. + +The standing session principle governing this decision, also quoted verbatim: "어떠한 휴리스틱과 Rule +of thumbs도 금지" — no heuristics or rules of thumb; a parameter needs actual justification from real +data, not a constant that happens to work today. + +## Research: three questions, checked directly against `contextual-orchestrator` source + +### 1. Does the gateway expose a way to separate a reasoning budget from a content budget? + +**No — not on the endpoint this preflight uses, and not for how the preflight calls it.** Checked +directly in `contextual_orchestrator/orchestrator.py` and `server.py`, not assumed: + +- `ReasoningEffortProfile`/`apply_request_profile()` (`reasoning_effort_profile.py`) is a real, + capability-gated mechanism (`ModelAgent.reasoning_effort_supported: bool | None`, fail-closed unless + proven `True`), but it is **additive, not substitutive**: `apply_request_profile()` always sets + `payload["max_tokens"] = validated.max_output_tokens` regardless of whether `reasoning_effort` is + also set. There is no "unbounded reasoning + bounded content" mode — `reasoning_effort` is a coarse + enum (`none`/`low`/`medium`/`high`) that tells a supporting provider how to spend *within* the + existing token budget, not a second, independently-sized budget. +- This mechanism is **opt-in at `TaskOrchestrator` construction**, not caller-controlled: + `_role_effort_profile(role)` returns `None` unless the server was constructed with an explicit + `role_effort_catalog` (`orchestrator.py:5530-5534`). It is also only reachable from role-based + workflow steps; the sidecar's plain "Reply with just 'OK'" prompt against the virtual pool is not a + role-scoped workflow call. +- Critically, the **public `/v1/chat/completions` endpoint the sidecar and Strix's client both call + does not thread a caller-supplied `reasoning_effort`/`reasoning` field into orchestration at all.** + `server.py`'s own docstrings say so directly: `_validate_chat_reasoning_effort` — "This gateway never + threads the knob into `ModelClient` on the orchestration path. Known levels are accepted as + default-effort no-ops"; `_validate_responses_reasoning` (the `/v1/responses` equivalent) — "This + gateway proxies Responses but does not interpret or enforce reasoning controls." Both accept the + field syntactically (so SDK defaults do not 400) and then discard it. Switching the preflight from + `/v1/chat/completions` to `/v1/responses` would not gain anything here — the field is a documented + no-op on both surfaces. + +**Conclusion**: there is no lever, on any caller-facing surface this preflight (or Strix) can reach, +that separates "let the model think as long as it needs" from "cap what it can emit." This is the +honest "no" the coordinator's brief anticipated as a possible outcome. + +### 2. Is a real-generation preflight even the right liveness mechanism — is there a cheaper or more direct signal? + +**A better mechanism than the sidecar's hand-rolled curl probe already exists upstream, but it is not +"free" and it is not currently reachable at the sidecar's privilege level.** Checked directly: + +- `ModelClient.probe(agent, timeout=...)` (`orchestrator.py:1483`) is a purpose-built liveness probe, + documented as exactly the right idea: *"`/health` and `/v1/models` only prove process/model-registry + liveness; this verifies the configured local model and deliberately exercises the chat path with one + output token. It never retries, so a stuck local queue cannot be multiplied by the readiness check."* + It isolates failures per agent (catches every exception, returns `status: "not_ready"` with a + `failure_code` rather than raising) and runs against every agent type, not only local providers — the + chat-probe payload construction is unconditional on `_is_local_provider_url`. +- `TaskOrchestrator.provider_readiness_report(refresh=True)` (`orchestrator.py:3441`) calls `probe()` + across **every candidate agent in the pool**, isolating each candidate's outcome, and returns an + aggregate plus a per-agent `items[]` list with `status`/`failure_code`/`latency_ms`. This is + structurally exactly what the sidecar's single-shot "one candidate must work" curl probe is trying to + approximate externally — except done per-candidate, with real diagnostics, instead of betting the + whole preflight on whichever one candidate the pool router happens to draw. It is exposed as + `GET /api/v1/provider_readiness/latest?refresh=true` (`server.py:5711-5715`). +- **This is not a free lunch, and the honest caveats matter as much as the discovery:** + - `probe()` itself still hardcodes `max_tokens: 1` (`orchestrator.py:1536`) — even more aggressive + than either value the sidecar has tried. It has the *same* reasoning-overhead vulnerability + described above, per candidate. The win is not that this number is right for every model; the win + is that the surface is already structured so one candidate's wrong-budget failure is an isolated, + attributable `not_ready` entry (`failure_code: "provider_empty_probe_response"`), not an opaque + all-or-nothing outage. + - **Verified directly, and this is the one real blocker to adopting it as-is**: `/api/v1/*` GET + routes, `provider_readiness/latest` included, are authorized at **`admin` scope** + (`server.py`'s `_admin_purpose()` / the `self._authorize("admin", ...)` call guarding the + `/api/v1/*` dispatch block), while `/v1/chat/completions` — what the sidecar's bearer token is + scoped for today — is authorized at the separate, narrower **`inference` scope** + (`self._authorize("inference")` on the chat/completions and `/v1/models` handlers). Provisioning + the CI review sidecar with an admin-scoped token just to call a readiness endpoint would be a real + privilege widening (full operator/admin surface, not merely "can it serve a chat completion") that + this ADR explicitly does **not** recommend. + +**Conclusion**: the right-shaped mechanism exists upstream, but adopting it exactly as-is would trade a +token-budget problem for a privilege-scope problem. The actionable move today is to replicate its +*shape* (per-candidate, isolated-failure, N-of-M) inside the sidecar itself, over the catalog it +already builds, using the same `inference`-scoped bearer token it already holds — see Decision below — +and separately ask upstream for an `inference`-scoped narrow readiness probe so the sidecar can retire +its own hand-rolled version later. + +### 3. If a numeric budget is still needed, can it be derived per-model from real discovered data? + +**Not today — confirmed as a genuine, currently-open gap, not assumed.** Checked both schemas directly: + +- `contextual_orchestrator/model_discovery.py`'s `DiscoveredModel` dataclass carries `provider_name`, + `model_id`, `credential_name`, `chat_base_url`, `auth_scheme`, `capabilities`, `input_modalities`, + `output_modalities`, pricing (`prompt_price_per_1k`, `completion_price_per_1k`, `unit_prices`), + `is_free`, and ZDR/privacy flags. **No field for context window, max output tokens, or max + completion tokens exists anywhere in this dataclass** (a repo-wide grep for + `context_length|context_window|max_output_tokens|max_completion_tokens|"limit"` inside this file + returns nothing). +- `contextual_orchestrator/orchestrator.py`'s `ModelAgent` dataclass (the routing-time representation) + likewise carries no such field — `reasoning_effort_supported` and `stream_usage_supported` are the + only capability flags it has. +- This is real, closeable data loss: several of the providers this discovery pipeline already queries + (e.g. OpenRouter's `/api/v1/models` response, `models.dev`'s `api.json`) commonly publish a per-model + context-window / max-output-tokens field in the exact list responses `model_discovery.py` already + fetches and parses — it is being read and then dropped, not unavailable. + +**Conclusion**: deriving a real per-model ceiling is the *correct* long-term answer to the owner's +second axis, but it requires a schema extension to `DiscoveredModel`/`ModelAgent` plus per-provider +field-mapping research (a Ponytail-gate task in its own right — provider list-response shapes need +verifying individually, not assumed uniform) and a place in `ModelClient` to clamp a requested +`max_tokens` to `min(requested, discovered_ceiling)`, fail-closed the same way +`reasoning_effort_supported=None` already fails closed when support is unproven. This is real, +substantial `contextual-orchestrator` work, not a same-day sidecar patch — tracked as a follow-up below, +not undertaken in this ADR. + +## Decision + +1. **Replace the sidecar's single-shot preflight with a bounded per-candidate probe and an N-of-M + threshold**, over the same catalog the sidecar's launcher already builds (the same candidate list + `contextual_orchestrator_review_policy.py`'s family-cap selection admits). For each admitted + candidate (bounded by the existing `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`), send one bounded + `POST /v1/chat/completions` request pinned to that specific model id (not the virtual + `orchestrator/free` pool, so the sidecar controls exactly which candidate each probe exercises), + with a small, deliberately conservative `max_tokens` (matching upstream `probe()`'s own precedent — + this ADR does not invent a new number, it reuses the one the gateway's own author already chose for + the same purpose). Treat the preflight as passed once **any** candidate returns real content, not + only the first or only the one the pool router happens to draw. This does not require picking a + number that is right for every model — it requires tolerating that some candidates will fail their + probe for reasons unrelated to the gateway being down (token-budget mismatch among them), the same + way `provider_readiness_report`'s own aggregate already tolerates partial `not_ready` results. +2. **File an upstream ask on `ContextualWisdomLab/contextual-orchestrator`** for an `inference`-scoped + variant of `provider_readiness_report`/`probe()` (or a scope widening of the existing endpoint that + the gateway's own security model is comfortable with) so the sidecar can eventually retire its + hand-rolled per-candidate loop in favor of the gateway's own, better-tested mechanism. Not blocking + for item 1. +3. **File an upstream ask on `ContextualWisdomLab/contextual-orchestrator`** to extend + `DiscoveredModel`/`ModelAgent` with a real, provider-sourced max-output-tokens/context-window field, + fail-closed when a provider does not publish one, so `max_tokens` selection (here and everywhere + else in the codebase that currently uses a single constant) can eventually be derived from real + per-model data rather than any constant. Not blocking for item 1; this is the correct long-term + closure of the owner's second axis. +4. **Explicitly reject** further tuning of one global `max_tokens` constant as a terminal fix. Every + value tried so far (16, 4096) has failed for a different, evidenced reason tied to pool + heterogeneity, confirming the owner's original objection rather than one bad guess needing one + better guess. + +## Consequences + +- The preflight becomes structurally tolerant of individual candidates being wrong for a fixed token + budget, which is the actual shape of the problem — instead of continuing to search for a number that + fits every model in a heterogeneous pool, no single number is asked to. +- The preflight's total worst-case latency grows with the number of candidates probed (bounded by the + existing `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES` / `REVIEW_PREFLIGHT_TIMEOUT_SECONDS` ceiling already + governing family-cap candidate counts) rather than being one fixed-cost request; this trades latency + for the isolation that made the family-cap fix's own tolerant-of-partial-failure design work. + Each candidate probe should stay short (small, conservative `max_tokens`, short per-candidate + timeout) precisely because it no longer needs to prove one specific candidate works — only that one + of several does. +- Item 1 is implementable now, inside `.github`, without upstream changes. Items 2 and 3 are real + `contextual-orchestrator` feature work and are explicitly not closed by this ADR — they are the + more complete answers the coordinator's brief asked to be surfaced honestly rather than invented. +- No production routing default changes; this is scoped to the sidecar's own liveness check. + +## Evidence trail + +- `ModelClient._response_content` (`orchestrator.py:1648-1660`) — the exact "reasoning without content" + failure this whole investigation traces to, already anticipated in the codebase's own error message: + *"provider {agent.id} returned reasoning without content; for mlx-lm set + chat_template_args={"enable_thinking": false} or increase max_output_tokens"* — note even this + upstream guidance is "increase the budget," the same reactive strategy #1436 tried and this ADR + moves away from. +- `ModelClient.apply_effort_profile` / `reasoning_effort_profile.apply_request_profile` — confirms + `max_tokens` is always set regardless of `reasoning_effort`. +- `server.py:3731-3758` (`_validate_chat_reasoning_effort`), `server.py:4775-4809` + (`_validate_responses_reasoning`) — confirms both `reasoning_effort` and `reasoning` are validated, + documented no-ops on the caller-facing surfaces this preflight and Strix use. +- `ModelClient.probe` (`orchestrator.py:1483-1561`), `TaskOrchestrator.provider_readiness_report` + (`orchestrator.py:3441-3486`), `server.py:5711-5715` (`GET /api/v1/provider_readiness/latest`) — + the existing per-candidate readiness mechanism, and its admin-scope gate + (`server.py`'s `_admin_purpose` / `_authorize("admin", ...)` vs. the `inference`-scoped + `/v1/chat/completions` and `/v1/models` handlers). +- `contextual_orchestrator/model_discovery.py`'s `DiscoveredModel` dataclass and + `contextual_orchestrator/orchestrator.py`'s `ModelAgent` dataclass — confirmed absence of any + context-window/max-output-tokens field via direct grep and full-dataclass read. +- 2026-08-30 gap-baseline entries ("sidecar-preflight outage: family_cap/max_tokens fixes confirmed + working end to end...") for the live #1436→120s-timeout evidence this ADR responds to. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 794dc9de9c..c57850deb1 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1292,6 +1292,38 @@ conflicting** PRs address pieces of this: currently blocked by the sidecar-preflight outage above, so neither could be re-reviewed to a genuine pass yet regardless of which approach wins. +## 2026-08-30 sidecar preflight `max_tokens`: explicit owner critique, ADR-0005 + +Direct owner feedback after #1436's `max_tokens` 16→4096 raise moved the sidecar's gateway preflight +failure from "empty content" to "120s timeout, zero bytes": *"max_tokens 이걸 고정하는 게 말이 안 +되는데"* (hardcoding this doesn't make sense) — *"모델마다 max_tokens 허용치가 다 다른데"* (each model's +real ceiling differs too). Both are correct and evidenced, not just asserted: see +[`docs/adr/0005-sidecar-preflight-token-budget.md`](adr/0005-sidecar-preflight-token-budget.md) for the +full research trail, checked directly against `contextual-orchestrator` source rather than assumed. + +Summary of what that ADR found and decided: + +- **No caller-facing lever separates a reasoning budget from a content budget on this gateway.** + `ReasoningEffortProfile` is real but additive (still always sets `max_tokens`), opt-in server-side + only, and — critically — the public `/v1/chat/completions` / `/v1/responses` endpoints this preflight + and Strix both use treat a caller-supplied `reasoning_effort`/`reasoning` field as a **documented + no-op**, confirmed directly from `server.py`'s own docstrings. +- **A better-shaped liveness mechanism already exists upstream** (`ModelClient.probe()` / + `provider_readiness_report()`, per-candidate, isolated-failure, exposed as + `GET /api/v1/provider_readiness/latest`), but it is gated at **`admin` scope** while the sidecar's + bearer token is `inference`-scoped — adopting it as-is would be a real privilege widening, not + recommended. +- **No per-model `max_tokens`/context-window ceiling is captured anywhere today.** Confirmed by direct + read: neither `DiscoveredModel` (`model_discovery.py`) nor `ModelAgent` (`orchestrator.py`) carries + any such field, even though several already-queried provider list endpoints publish one. Real, + closeable gap; not same-day sidecar work. +- **Decision**: stop tuning one global constant. Replace the sidecar's single-shot preflight with a + bounded per-candidate probe over the catalog it already builds, requiring only one of several + candidates to succeed (N-of-M), so no single `max_tokens` value has to be simultaneously right for + every model in a heterogeneous pool. Two upstream `contextual-orchestrator` asks (an + inference-scoped readiness probe; a real per-model token-ceiling field in discovery) are tracked as + follow-ups, not closed by this ADR. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. From 7f13821fe292d08529cfbfe1aed9ab457ddf3ccf Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 12:04:06 +0000 Subject: [PATCH 02/12] docs(adr): fix six Devin Review findings on ADR-0005 Each finding was verified against the actual ADR text and the launcher/sidecar source before acting, per this repo's convention of never accepting or dismissing an automated review finding unverified. Two were real design flaws in the first draft: 1. The original decision reused a fixed tiny max_tokens (matching upstream probe()'s precedent of 1) for every per-candidate probe -- this reproduces the exact reasoning-budget-starvation bug the whole investigation started from, one layer down, and a fixed budget is itself the kind of rule-of-thumb this repo's conventions forbid. Fixed: per-candidate probes now escalate to a larger budget only on positive evidence (empty content AND finish_reason == "length", the provider-documented signature of "budget too small," not "down"). Genuinely-down candidates never reach the retry path. 2. The original decision replaced the sidecar's real end-to-end virtual-pool smoke request with per-candidate checks alone. Verified directly: the 2026-08-30 gap-baseline entry for PR #1433 already documents a live case where per-candidate preflight passed while the virtual-pool request still 502'd -- a different code path entirely. Fixed: both existing preflight layers are kept; neither is removed. Also fixed: a mischaracterization (the launcher's _preflight_review_agents/_preflight_with_fallback already exist and do per-candidate N-of-M-tolerant probing today -- confirmed by reading the source; the ADR now describes fixing them, not introducing them); conflated context-window vs max-output-tokens treated as separate, independently-nullable fields per OpenRouter's live OpenAPI schema (fetched and verified, not assumed); real external citations for provider-behavior claims (OpenAI and OpenRouter docs, fetched live); and the two upstream asks are now real tracked issues (ContextualWisdomLab/contextual-orchestrator#926, #927) instead of prose. Also folds in a fresh, directly-verified live reproduction: noema-review failed on this ADR's own PR (#1449, job 99253418179) with exactly the bug under discussion -- Layer 1 passed in 30s, Layer 2 then hung the full 120s with zero bytes back -- confirming this is an active defect, not a theoretical one. Co-Authored-By: Claude --- CHANGELOG.md | 30 +- .../0005-sidecar-preflight-token-budget.md | 403 ++++++++++-------- docs/product-technical-gap-baseline.md | 52 ++- 3 files changed, 282 insertions(+), 203 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53e590b62c..a993534fee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,19 +7,23 @@ Semantic Versioning where the repository publishes a release. ## [Unreleased] - Add `docs/adr/0005-sidecar-preflight-token-budget.md`, an evidence-based design decision responding to the owner's direct critique that a single - hardcoded `max_tokens` cannot fit a heterogeneous `orchestrator/free` pool - (varying reasoning-token overhead per model, and a genuinely different real - ceiling per model). Checked directly against `contextual-orchestrator` - source: no caller-facing way exists to separate a reasoning budget from a - content budget on the endpoints this preflight/Strix use; - `ModelClient.probe()`/`provider_readiness_report()` is a better-shaped, - already-built per-candidate liveness mechanism but is `admin`-scoped while - the sidecar's token is `inference`-scoped; no per-model token-ceiling data - is captured anywhere in discovery today. Decision: stop tuning one global - constant and move the sidecar's preflight to a bounded per-candidate probe - with an N-of-M "at least one route works" threshold, with two narrower - upstream asks tracked as follow-ups rather than closed here. No code - change in this PR; the sidecar migration is tracked separately. + hardcoded `max_tokens` cannot fit a heterogeneous `orchestrator/free` pool. + Revised after six verified Devin Review findings on its PR (#1449), + including two real design flaws in the first draft: reusing a fixed tiny + `max_tokens` for a per-candidate probe reproduces the same + reasoning-budget-starvation bug one layer down, and dropping the sidecar's + separate virtual-pool smoke request in favor of per-candidate checks alone + cannot catch a virtual-pool dispatch bug (already documented live on + PR #1433). The current decision keeps both existing preflight layers + (`_preflight_review_agents`/`_preflight_with_fallback` in the launcher; the + shell script's virtual-pool request) and fixes their shared flaw with a + diagnostic retry that escalates only when a response is empty AND + `finish_reason == "length"`, plus short per-attempt timeouts, instead of + picking a new fixed number. Adds two real tracked upstream issues + (`ContextualWisdomLab/contextual-orchestrator#926`, `#927`) with external + citations (OpenAI, OpenRouter docs, fetched live) replacing prose-only + follow-ups. No code change in this PR; the sidecar migration is tracked + separately. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index 2eb3a17457..304e8d1be5 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -1,61 +1,79 @@ -# ADR-0005: Replace the sidecar's single-shot, fixed-`max_tokens` gateway preflight with per-candidate readiness +# ADR-0005: Replace the sidecar's fixed-`max_tokens` gateway checks with diagnostic, tolerant readiness - Status: proposed - Date: 2026-08-30 - Scope: `ContextualWisdomLab/.github` central review pipelines' vendored `contextual-orchestrator` - sidecar (`scripts/ci/contextual_orchestrator_review_sidecar.sh`), and a small upstream request to - `ContextualWisdomLab/contextual-orchestrator`. -- Decision: Stop trying to pick a single, universally-correct `max_tokens` value for the sidecar's - post-`healthz` gateway preflight. Replace the one hardcoded-budget completion request against the - virtual `orchestrator/free` pool with a bounded, per-candidate probe over the catalog the sidecar - already builds, using an **N-of-M "is at least one route usable" threshold** instead of a single - request that must succeed. Track upstream extension of `contextual-orchestrator`'s existing - `ModelClient.probe()` / `provider_readiness_report()` machinery with an inference-scoped variant, and - track a separate, real per-model `max_tokens` ceiling in model discovery, as two follow-ups this ADR - does not itself close. -- Ownership: `.github` owns the sidecar script and this ADR; `ContextualWisdomLab/contextual-orchestrator` - owns the gateway internals cited as evidence and the two follow-up asks. + sidecar — `scripts/ci/contextual_orchestrator_review_launcher.py`'s existing + `_preflight_review_agents`/`_preflight_with_fallback`, and + `scripts/ci/contextual_orchestrator_review_sidecar.sh`'s separate gateway smoke request — plus two + tracked upstream asks on `ContextualWisdomLab/contextual-orchestrator`. +- Decision: Keep both existing preflight layers (per-candidate launcher probing, and the shell + script's separate end-to-end request to the virtual `orchestrator/free` model) — neither is being + introduced, both already exist and each catches a failure class the other cannot. Fix what is + actually wrong with each: replace their single fixed `max_tokens` value with a diagnostic, + short-timeout, escalate-only-on-positive-evidence probe, so no single number has to be + simultaneously right for every model in a heterogeneous pool, and a reasoning-heavy healthy + candidate is no longer misclassified as down. Track two upstream `contextual-orchestrator` asks + (`ContextualWisdomLab/contextual-orchestrator#926`, `#927`) as real, tracked, non-blocking follow-ups. +- Ownership: `.github` owns the sidecar/launcher script and this ADR; `ContextualWisdomLab/contextual-orchestrator` + owns the gateway internals cited as evidence and the two follow-up issues. - Figma File ID: N/A (no customer UI). ## Context -`scripts/ci/contextual_orchestrator_review_sidecar.sh`'s gateway preflight sends one -`POST /v1/chat/completions` request against the virtual `orchestrator/free` model — -`{"model":"orchestrator/free","messages":[...,"Reply with just 'OK'."],"max_tokens":,...}` — and -fails the whole sidecar (blocking `noema-review`/`opencode-review`/`strix` org-wide) unless that one -request returns non-empty `choices[0].message.content` within a fixed `curl --max-time`. +Central review (`noema-review`/`opencode-review`/`strix`) depends on two separate, already-existing +liveness checks in the vendored sidecar, run in sequence — this ADR fixes both, it introduces neither: -`N` has already been tuned twice in this investigation: 16 → 4096 (#1436), moving the failure from -"empty content at 16 tokens" (the provider's response consumed the whole budget on internal reasoning -before emitting visible content — see `ModelClient._response_content`'s own anticipated error message, -quoted below) to "120s timeout with zero bytes at 4096 tokens" on a separate run. Direct owner feedback -in response to that outcome, quoted verbatim because it is the reason this ADR exists: +1. **Per-candidate launcher probing.** `scripts/ci/contextual_orchestrator_review_launcher.py`'s + `_preflight_review_agents()` (line 200) sends one bounded `POST` to `client.proxy_send_once` for + *each* candidate agent in the admitted catalog, with a fixed `max_tokens=REVIEW_MAX_OUTPUT_TOKENS` + (currently `4096`, line 38) and a fixed `temperature=REVIEW_TEMPERATURE`. It keeps every candidate + whose response has non-empty text (`_chat_response_has_text`, line 175 — checks only + `choices[0].message.content`, never inspects `finish_reason`) and raises `ReviewPreflightError` + only if **zero** candidates pass — i.e. it is already an N-of-M ("at least one must work") design, + not a single-candidate gate. `_preflight_with_fallback()` (line 274) wraps this with one fallback + catalog tier. This runs inside the Python process at server startup, before the sidecar can even + report healthy. +2. **The shell script's own virtual-pool smoke request.** Once the server is up, + `contextual_orchestrator_review_sidecar.sh` separately sends one `POST /v1/chat/completions` with + `"model":"orchestrator/free"` (the *virtual* pool id, not a specific candidate) and its own fixed + `max_tokens` — this is the request `N` below refers to. + +`N` has already been tuned twice: 16 → 4096 (#1436), moving the failure from "empty content at 16 +tokens" (the provider's response consumed the whole budget on internal reasoning before emitting +visible content — see `ModelClient._response_content`'s own anticipated error message, quoted below) +to "120s timeout with zero bytes at 4096 tokens" on a separate run. Direct owner feedback in response +to that outcome, quoted verbatim because it is the reason this ADR exists: > "max_tokens 이걸 고정하는 게 말이 안 되는데" — hardcoding this max_tokens doesn't make sense. > "모델마다 max_tokens 허용치가 다 다른데" — each model has a genuinely different max_tokens allowance. `orchestrator/free` is a heterogeneous pool (`nvidia_nim`, `openai`, `opencode_zen`, `bytez`, `openrouter`, ... — see `contextual_orchestrator_review_policy.py`'s `PROVIDER_FAMILIES`), and which -candidate a given preflight run draws varies (family-cap admission is deterministic but -alphabetical-by-provider-then-model, and the catalog itself changes over time). A fixed `max_tokens` -is wrong on two independent, evidenced axes for a pool like this: - -1. **Reasoning-token overhead differs per model.** A model that spends internal reasoning tokens before - emitting visible content can exhaust a small budget with zero visible output, or (with a much larger - budget) legitimately take far longer to finish than a fast, non-reasoning model would for the same - budget — this is very likely what turned #1436's 4096-token raise into a 120-second timeout instead - of a fix (see the 2026-08-30 gap-baseline entry's "third, distinct failure mode"). -2. **The provider's own hard ceiling on `max_tokens`/`max_completion_tokens` differs per model.** Some - providers reject a request outright (400) if `max_tokens` exceeds what that specific model supports; - others support far more than a generic constant would ever request. A single number can therefore be - simultaneously too small for one model's reasoning overhead and too large for another model's real - ceiling — there is structurally no number that is not wrong for some member of the pool. +candidate a given preflight run draws varies. A fixed `max_tokens` is wrong on two independent, +evidenced axes for a pool like this: + +1. **Reasoning-token overhead differs per model.** A model that spends internal reasoning tokens + before emitting visible content can exhaust a small budget with zero visible output. OpenAI's own + documentation of `finish_reason == "length"` describes exactly this: *"it's likely that max_tokens + is too small and model runs out of tokens before it manages to [complete]"* + ([OpenAI API guide](https://developers.openai.com/api/docs/guides/completions)). This is very + likely what turned #1436's 4096-token raise into a 120-second timeout instead of a fix — a large + budget lets a heavy-reasoning model legitimately run far longer than a fast, non-reasoning model + would for the same request (see the 2026-08-30 gap-baseline entry's "third, distinct failure mode"). +2. **The provider's own hard ceiling on completion tokens differs per model**, and is a genuinely + separate quantity from a model's context window (see Research §3 below). Some providers reject a + request outright if `max_tokens` exceeds what that specific model supports; others support far more + than a generic constant would ever request. A single number can therefore be simultaneously too + small for one model's reasoning overhead and too large for another model's real ceiling — there is + structurally no number that is not wrong for some member of the pool. The standing session principle governing this decision, also quoted verbatim: "어떠한 휴리스틱과 Rule of thumbs도 금지" — no heuristics or rules of thumb; a parameter needs actual justification from real data, not a constant that happens to work today. -## Research: three questions, checked directly against `contextual-orchestrator` source +## Research: three questions, checked directly against `contextual-orchestrator` source and, where the +## claim is about external provider behavior, against the providers' own current documentation ### 1. Does the gateway expose a way to separate a reasoning budget from a content budget? @@ -66,167 +84,212 @@ directly in `contextual_orchestrator/orchestrator.py` and `server.py`, not assum capability-gated mechanism (`ModelAgent.reasoning_effort_supported: bool | None`, fail-closed unless proven `True`), but it is **additive, not substitutive**: `apply_request_profile()` always sets `payload["max_tokens"] = validated.max_output_tokens` regardless of whether `reasoning_effort` is - also set. There is no "unbounded reasoning + bounded content" mode — `reasoning_effort` is a coarse - enum (`none`/`low`/`medium`/`high`) that tells a supporting provider how to spend *within* the - existing token budget, not a second, independently-sized budget. + also set. There is no "unbounded reasoning + bounded content" mode. This matches how OpenAI itself + documents the analogous parameter: `max_completion_tokens` is *"an upper bound for the number of + tokens that can be generated for a completion, **including** visible output tokens and reasoning + tokens"* (same OpenAI guide) — reasoning and visible content already share one budget upstream, by + design, not only in this gateway. - This mechanism is **opt-in at `TaskOrchestrator` construction**, not caller-controlled: `_role_effort_profile(role)` returns `None` unless the server was constructed with an explicit - `role_effort_catalog` (`orchestrator.py:5530-5534`). It is also only reachable from role-based - workflow steps; the sidecar's plain "Reply with just 'OK'" prompt against the virtual pool is not a - role-scoped workflow call. -- Critically, the **public `/v1/chat/completions` endpoint the sidecar and Strix's client both call - does not thread a caller-supplied `reasoning_effort`/`reasoning` field into orchestration at all.** - `server.py`'s own docstrings say so directly: `_validate_chat_reasoning_effort` — "This gateway never - threads the knob into `ModelClient` on the orchestration path. Known levels are accepted as + `role_effort_catalog` (`orchestrator.py:5530-5534`). +- The **public `/v1/chat/completions` endpoint the sidecar and Strix's client both call does not + thread a caller-supplied `reasoning_effort`/`reasoning` field into orchestration at all.** + `server.py`'s own docstrings say so directly: `_validate_chat_reasoning_effort` — "This gateway + never threads the knob into `ModelClient` on the orchestration path. Known levels are accepted as default-effort no-ops"; `_validate_responses_reasoning` (the `/v1/responses` equivalent) — "This - gateway proxies Responses but does not interpret or enforce reasoning controls." Both accept the - field syntactically (so SDK defaults do not 400) and then discard it. Switching the preflight from - `/v1/chat/completions` to `/v1/responses` would not gain anything here — the field is a documented - no-op on both surfaces. + gateway proxies Responses but does not interpret or enforce reasoning controls." Switching the + preflight to `/v1/responses` would not gain anything here — the field is a documented no-op on both. **Conclusion**: there is no lever, on any caller-facing surface this preflight (or Strix) can reach, -that separates "let the model think as long as it needs" from "cap what it can emit." This is the -honest "no" the coordinator's brief anticipated as a possible outcome. +that separates "let the model think as long as it needs" from "cap what it can emit." ### 2. Is a real-generation preflight even the right liveness mechanism — is there a cheaper or more direct signal? -**A better mechanism than the sidecar's hand-rolled curl probe already exists upstream, but it is not -"free" and it is not currently reachable at the sidecar's privilege level.** Checked directly: - -- `ModelClient.probe(agent, timeout=...)` (`orchestrator.py:1483`) is a purpose-built liveness probe, - documented as exactly the right idea: *"`/health` and `/v1/models` only prove process/model-registry - liveness; this verifies the configured local model and deliberately exercises the chat path with one - output token. It never retries, so a stuck local queue cannot be multiplied by the readiness check."* - It isolates failures per agent (catches every exception, returns `status: "not_ready"` with a - `failure_code` rather than raising) and runs against every agent type, not only local providers — the - chat-probe payload construction is unconditional on `_is_local_provider_url`. -- `TaskOrchestrator.provider_readiness_report(refresh=True)` (`orchestrator.py:3441`) calls `probe()` - across **every candidate agent in the pool**, isolating each candidate's outcome, and returns an - aggregate plus a per-agent `items[]` list with `status`/`failure_code`/`latency_ms`. This is - structurally exactly what the sidecar's single-shot "one candidate must work" curl probe is trying to - approximate externally — except done per-candidate, with real diagnostics, instead of betting the - whole preflight on whichever one candidate the pool router happens to draw. It is exposed as - `GET /api/v1/provider_readiness/latest?refresh=true` (`server.py:5711-5715`). -- **This is not a free lunch, and the honest caveats matter as much as the discovery:** - - `probe()` itself still hardcodes `max_tokens: 1` (`orchestrator.py:1536`) — even more aggressive - than either value the sidecar has tried. It has the *same* reasoning-overhead vulnerability - described above, per candidate. The win is not that this number is right for every model; the win - is that the surface is already structured so one candidate's wrong-budget failure is an isolated, - attributable `not_ready` entry (`failure_code: "provider_empty_probe_response"`), not an opaque - all-or-nothing outage. - - **Verified directly, and this is the one real blocker to adopting it as-is**: `/api/v1/*` GET - routes, `provider_readiness/latest` included, are authorized at **`admin` scope** - (`server.py`'s `_admin_purpose()` / the `self._authorize("admin", ...)` call guarding the - `/api/v1/*` dispatch block), while `/v1/chat/completions` — what the sidecar's bearer token is - scoped for today — is authorized at the separate, narrower **`inference` scope** - (`self._authorize("inference")` on the chat/completions and `/v1/models` handlers). Provisioning - the CI review sidecar with an admin-scoped token just to call a readiness endpoint would be a real - privilege widening (full operator/admin surface, not merely "can it serve a chat completion") that - this ADR explicitly does **not** recommend. - -**Conclusion**: the right-shaped mechanism exists upstream, but adopting it exactly as-is would trade a -token-budget problem for a privilege-scope problem. The actionable move today is to replicate its -*shape* (per-candidate, isolated-failure, N-of-M) inside the sidecar itself, over the catalog it -already builds, using the same `inference`-scoped bearer token it already holds — see Decision below — -and separately ask upstream for an `inference`-scoped narrow readiness probe so the sidecar can retire -its own hand-rolled version later. +**A better-shaped mechanism than a single fixed-budget request exists in two places — one already in +this sidecar, one further upstream — but neither is a free non-generation signal.** Checked directly: + +- **Already in this repo**: `_preflight_review_agents()` (described in Context above) already probes + every candidate individually and already tolerates any number of individual failures — it fails the + whole preflight only when literally none pass. What it lacks is not the *shape* (that already + exists) but a way to tell "this candidate is down" apart from "this candidate is healthy but its + first probe's budget was wrong for it" — seе Decision §1 below. +- **Further upstream, admin-scoped**: `ModelClient.probe()` (`orchestrator.py:1483`) and + `TaskOrchestrator.provider_readiness_report()` (`orchestrator.py:3441`, exposed as + `GET /api/v1/provider_readiness/latest?refresh=true`, `server.py:5711-5715`) are the gateway's own, + more mature version of the same idea — per-candidate, isolated failure, real `failure_code` + diagnostics. **Verified directly, and this is a real blocker to adopting it as-is**: `/api/v1/*` GET + routes are authorized at **`admin` scope** (`server.py`'s `_admin_purpose()` / + `self._authorize("admin", ...)`), while `/v1/chat/completions` — what the sidecar's bearer token is + scoped for today — is authorized at the separate, narrower **`inference` scope**. Provisioning the + CI review sidecar with an admin-scoped token just to call a readiness endpoint would be a real + privilege widening this ADR does not recommend. Tracked as + `ContextualWisdomLab/contextual-orchestrator#926`. +- **Neither eliminates real generation.** `probe()` itself still hardcodes `max_tokens: 1` + (`orchestrator.py:1536`) — even more aggressive than either value this sidecar has tried, and + vulnerable to the exact same reasoning-overhead misclassification described above. Adopting + `provider_readiness_report`'s *shape* without also fixing this calibration problem would just move + the bug, not fix it — which is Devin Review's finding on an earlier draft of this ADR (see Decision + §1 for the actual fix). + +**Conclusion**: reuse the shape that already exists in this sidecar (per-candidate, N-of-M-tolerant); +fix its calibration (Decision §1); track the upstream, better-tested version as a non-blocking +follow-up (`#926`) because it is currently out of reach at this token's privilege level. ### 3. If a numeric budget is still needed, can it be derived per-model from real discovered data? -**Not today — confirmed as a genuine, currently-open gap, not assumed.** Checked both schemas directly: - -- `contextual_orchestrator/model_discovery.py`'s `DiscoveredModel` dataclass carries `provider_name`, - `model_id`, `credential_name`, `chat_base_url`, `auth_scheme`, `capabilities`, `input_modalities`, - `output_modalities`, pricing (`prompt_price_per_1k`, `completion_price_per_1k`, `unit_prices`), - `is_free`, and ZDR/privacy flags. **No field for context window, max output tokens, or max - completion tokens exists anywhere in this dataclass** (a repo-wide grep for - `context_length|context_window|max_output_tokens|max_completion_tokens|"limit"` inside this file - returns nothing). -- `contextual_orchestrator/orchestrator.py`'s `ModelAgent` dataclass (the routing-time representation) - likewise carries no such field — `reasoning_effort_supported` and `stream_usage_supported` are the - only capability flags it has. -- This is real, closeable data loss: several of the providers this discovery pipeline already queries - (e.g. OpenRouter's `/api/v1/models` response, `models.dev`'s `api.json`) commonly publish a per-model - context-window / max-output-tokens field in the exact list responses `model_discovery.py` already - fetches and parses — it is being read and then dropped, not unavailable. +**Not today — confirmed as a genuine, currently-open gap.** Checked both schemas directly: + +- `contextual_orchestrator/model_discovery.py`'s `DiscoveredModel` dataclass and + `contextual_orchestrator/orchestrator.py`'s `ModelAgent` dataclass carry no field for a model's + output-token ceiling or context window — confirmed via full-dataclass read and grep. +- This is real, closeable data loss, and it is **two distinct pieces of data, not one** — verified + directly against a live provider schema rather than assumed uniform. OpenRouter's current OpenAPI + spec (`https://openrouter.ai/openapi.yaml`) defines the `Model` object's `context_length` field + (required) as *"Maximum context length in tokens"*, and separately, `TopProviderInfo.max_completion_tokens` + (nullable — genuinely absent for some models) as *"Maximum completion tokens from the top provider. + Input and output tokens share the context window, so the effective maximum output for a request is + further limited by the context remaining after input tokens."* Only the second field can directly + clamp a `max_tokens` request parameter; the first constrains prompt+output together and is not a + substitute for it — conflating them would let a large-context, small-output model's window size + wrongly justify a `max_tokens` far beyond what that model can actually complete in. **Conclusion**: deriving a real per-model ceiling is the *correct* long-term answer to the owner's -second axis, but it requires a schema extension to `DiscoveredModel`/`ModelAgent` plus per-provider -field-mapping research (a Ponytail-gate task in its own right — provider list-response shapes need -verifying individually, not assumed uniform) and a place in `ModelClient` to clamp a requested -`max_tokens` to `min(requested, discovered_ceiling)`, fail-closed the same way -`reasoning_effort_supported=None` already fails closed when support is unproven. This is real, -substantial `contextual-orchestrator` work, not a same-day sidecar patch — tracked as a follow-up below, -not undertaken in this ADR. +second axis, but requires a schema extension distinguishing `max_output_tokens` from `context_window` +as two separately-provenanced, independently-nullable fields, plus per-provider field-mapping research +(schemas are not uniform across the five configured providers). Tracked as +`ContextualWisdomLab/contextual-orchestrator#927`, not undertaken in this ADR. ## Decision -1. **Replace the sidecar's single-shot preflight with a bounded per-candidate probe and an N-of-M - threshold**, over the same catalog the sidecar's launcher already builds (the same candidate list - `contextual_orchestrator_review_policy.py`'s family-cap selection admits). For each admitted - candidate (bounded by the existing `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`), send one bounded - `POST /v1/chat/completions` request pinned to that specific model id (not the virtual - `orchestrator/free` pool, so the sidecar controls exactly which candidate each probe exercises), - with a small, deliberately conservative `max_tokens` (matching upstream `probe()`'s own precedent — - this ADR does not invent a new number, it reuses the one the gateway's own author already chose for - the same purpose). Treat the preflight as passed once **any** candidate returns real content, not - only the first or only the one the pool router happens to draw. This does not require picking a - number that is right for every model — it requires tolerating that some candidates will fail their - probe for reasons unrelated to the gateway being down (token-budget mismatch among them), the same - way `provider_readiness_report`'s own aggregate already tolerates partial `not_ready` results. -2. **File an upstream ask on `ContextualWisdomLab/contextual-orchestrator`** for an `inference`-scoped - variant of `provider_readiness_report`/`probe()` (or a scope widening of the existing endpoint that - the gateway's own security model is comfortable with) so the sidecar can eventually retire its - hand-rolled per-candidate loop in favor of the gateway's own, better-tested mechanism. Not blocking - for item 1. -3. **File an upstream ask on `ContextualWisdomLab/contextual-orchestrator`** to extend - `DiscoveredModel`/`ModelAgent` with a real, provider-sourced max-output-tokens/context-window field, - fail-closed when a provider does not publish one, so `max_tokens` selection (here and everywhere - else in the codebase that currently uses a single constant) can eventually be derived from real - per-model data rather than any constant. Not blocking for item 1; this is the correct long-term - closure of the owner's second axis. -4. **Explicitly reject** further tuning of one global `max_tokens` constant as a terminal fix. Every - value tried so far (16, 4096) has failed for a different, evidenced reason tied to pool - heterogeneity, confirming the owner's original objection rather than one bad guess needing one - better guess. +1. **Fix both existing preflight layers' probe calibration with diagnostic, escalate-on-evidence + retries — do not introduce a new mechanism, and do not remove either existing layer.** + - **Layer 1 (`_preflight_review_agents`, per candidate)**: for each candidate, send a first bounded + probe at a modest token budget. If the response is empty/whitespace **and** its + `choices[0].finish_reason == "length"` — the exact, provider-documented signature of "the + budget was too small," not "the candidate is unreachable" — retry that *same* candidate once at + a materially larger budget before recording it as rejected. Every other failure class (timeout, + connection error, non-2xx status, or empty content with any other `finish_reason`) is **not** + retried — those are not budget problems, and retrying would not fix them. This directly answers + Devin Review's finding: a fixed tiny budget (whether `1`, matching upstream `probe()`'s own + precedent, or any other single constant) would still misclassify a healthy reasoning-heavy + candidate as down; escalating only on the specific evidence that the budget — not the candidate + — was the problem does not have this failure mode, because a genuinely-down candidate never + reaches the retry path. + - **Layer 2 (the shell script's virtual-pool smoke request)**: apply the same diagnostic escalation + to the real end-to-end `POST /v1/chat/completions` request against `"model":"orchestrator/free"`. + This layer is **kept, not replaced by Layer 1** — Layer 1's per-candidate checks call + `client.proxy_send_once` against explicit candidate agents directly and structurally cannot + detect a bug in the virtual-pool's own dispatch/selection code, which is a different code path. + This is not hypothetical: the 2026-08-30 gap-baseline entry for PR #1433 records exactly this + split failure live — the launcher's own per-candidate preflight passed and the server reported + healthy, while the shell script's separate virtual-pool request still came back `HTTP 502`. Any + redesign that dropped Layer 2 in favor of Layer 1 alone would silently reintroduce that exact, + already-documented gap. Bound Layer 2 to at most 2 total attempts (matching Layer 1's own retry + bound) so a genuinely broken virtual-pool layer still fails fast. + - **Keep each attempt's own wall-clock timeout short**, independent of the token-budget question. + A candidate or route that is simply slow or hung should fail *that attempt* quickly and be + recorded as not ready — tolerable under Layer 1's existing N-of-M design and Layer 2's bounded + retry — rather than the preflight trying to avoid ever hitting a timeout by picking a "safer" + token budget. This decouples the two previously-conflated failure modes (wrong budget vs. slow + response) that made #1436's single-number tuning symptom-chase between them. + - **This ADR deliberately does not fix specific numeric values** for the modest/escalated budgets + or the per-attempt timeout. Committing to new constants here would repeat the same mistake at one + remove — picking numbers by inspection rather than evidence. Both existing preflight layers + already emit a structured per-route report (`_preflight_review_agents`'s `routes[]`, and the + shell script's `preflight_report`/`gateway` JSON) — the follow-up implementation PR should add + `finish_reason` and attempt-count to that evidence and let the actual values be set from real + telemetry once deployed, not guessed in this document. +2. **Track `ContextualWisdomLab/contextual-orchestrator#926`** (an `inference`-scoped variant of + `provider_readiness_report`/`probe()`) so the sidecar can eventually retire its hand-rolled Layer 1 + loop in favor of the gateway's own, better-tested mechanism. Not blocking for item 1. +3. **Track `ContextualWisdomLab/contextual-orchestrator#927`** (real, separately-provenanced + `max_output_tokens`/`context_window` fields in `DiscoveredModel`/`ModelAgent`, fail-closed when + unknown) so `max_tokens` selection can eventually be derived from real per-model data. Not blocking + for item 1; this is the correct long-term closure of the owner's second axis. +4. **Explicitly reject** further tuning of one global `max_tokens` constant as a terminal fix for + either layer. Every value tried so far (16, 4096) has failed for a different, evidenced reason tied + to pool heterogeneity, confirming the owner's original objection rather than one bad guess needing + one better guess. ## Consequences -- The preflight becomes structurally tolerant of individual candidates being wrong for a fixed token - budget, which is the actual shape of the problem — instead of continuing to search for a number that - fits every model in a heterogeneous pool, no single number is asked to. -- The preflight's total worst-case latency grows with the number of candidates probed (bounded by the - existing `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES` / `REVIEW_PREFLIGHT_TIMEOUT_SECONDS` ceiling already - governing family-cap candidate counts) rather than being one fixed-cost request; this trades latency - for the isolation that made the family-cap fix's own tolerant-of-partial-failure design work. - Each candidate probe should stay short (small, conservative `max_tokens`, short per-candidate - timeout) precisely because it no longer needs to prove one specific candidate works — only that one - of several does. -- Item 1 is implementable now, inside `.github`, without upstream changes. Items 2 and 3 are real - `contextual-orchestrator` feature work and are explicitly not closed by this ADR — they are the - more complete answers the coordinator's brief asked to be surfaced honestly rather than invented. -- No production routing default changes; this is scoped to the sidecar's own liveness check. +- Both preflight layers become structurally tolerant of an individual attempt being wrong for a fixed + token budget or briefly slow, which is the actual shape of the problem — instead of continuing to + search for a number that fits every model in a heterogeneous pool, no single number is asked to, and + a genuinely down candidate or route is still detected and reported, just no longer conflated with a + merely-miscalibrated one. +- Total worst-case preflight latency grows modestly (up to one extra retry per candidate in Layer 1, + up to one extra retry in Layer 2), bounded by the existing `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES` / + `REVIEW_PREFLIGHT_TIMEOUT_SECONDS` ceiling and the new short per-attempt timeouts — this trades a + small amount of latency for the diagnostic precision that avoids the false-outage misclassification + Devin Review flagged. +- Keeping Layer 2 (not just Layer 1) means the preflight still proves the actual consumer-facing + `orchestrator/free` route works, not only that individual candidates can respond in isolation — + closing the PR #1433 gap class rather than reopening it. +- Items 2 and 3 are real `contextual-orchestrator` feature work, now tracked as real issues + (`#926`, `#927`), and are explicitly not closed by this ADR. +- No production routing default changes; this is scoped to the sidecar's own liveness checks. +- **This is currently active, not theoretical**: the live reproduction in the Evidence trail below is + from `noema-review` failing on this ADR's own PR while this ADR was being written, presently + blocking that required check org-wide on every repo that routes through this sidecar. The + implementation follow-up (a separate PR applying Decision §1) should be prioritized accordingly once + this ADR is settled, not treated as ordinary backlog. ## Evidence trail -- `ModelClient._response_content` (`orchestrator.py:1648-1660`) — the exact "reasoning without content" - failure this whole investigation traces to, already anticipated in the codebase's own error message: +- `scripts/ci/contextual_orchestrator_review_launcher.py`: `_preflight_review_agents` (L200-271), + `_preflight_with_fallback` (L274-291), `_chat_response_has_text` (L175-189), + `REVIEW_MAX_OUTPUT_TOKENS = 4096` (L38) — the existing Layer 1 mechanism this ADR fixes, not + introduces. +- `scripts/ci/contextual_orchestrator_review_sidecar.sh` — the existing Layer 2 virtual-pool smoke + request this ADR keeps. +- 2026-08-30 gap-baseline entry (PR #1433 evidence): *"the shell script's separate, subsequent real + `/v1/chat/completions` gateway smoke request against the now-serving `orchestrator/free` virtual + model came back HTTP 502. This is a different code path than the launcher's own preflight + (`ModelClient.proxy_send_once` against explicit candidate agents)"* — the direct, already-documented + precedent for why Layer 2 cannot be dropped in favor of Layer 1 alone. +- `ModelClient._response_content` (`orchestrator.py:1648-1660`) — the "reasoning without content" + failure this investigation traces to, already anticipated in the codebase's own error message: *"provider {agent.id} returned reasoning without content; for mlx-lm set - chat_template_args={"enable_thinking": false} or increase max_output_tokens"* — note even this - upstream guidance is "increase the budget," the same reactive strategy #1436 tried and this ADR - moves away from. + chat_template_args={"enable_thinking": false} or increase max_output_tokens."* - `ModelClient.apply_effort_profile` / `reasoning_effort_profile.apply_request_profile` — confirms `max_tokens` is always set regardless of `reasoning_effort`. - `server.py:3731-3758` (`_validate_chat_reasoning_effort`), `server.py:4775-4809` - (`_validate_responses_reasoning`) — confirms both `reasoning_effort` and `reasoning` are validated, - documented no-ops on the caller-facing surfaces this preflight and Strix use. + (`_validate_responses_reasoning`) — confirms both fields are validated, documented no-ops on the + caller-facing surfaces this preflight and Strix use. - `ModelClient.probe` (`orchestrator.py:1483-1561`), `TaskOrchestrator.provider_readiness_report` - (`orchestrator.py:3441-3486`), `server.py:5711-5715` (`GET /api/v1/provider_readiness/latest`) — - the existing per-candidate readiness mechanism, and its admin-scope gate - (`server.py`'s `_admin_purpose` / `_authorize("admin", ...)` vs. the `inference`-scoped - `/v1/chat/completions` and `/v1/models` handlers). + (`orchestrator.py:3441-3486`), `server.py:5711-5715` — the upstream mechanism, and its admin-scope + gate vs. the `inference`-scoped `/v1/chat/completions`/`/v1/models` handlers. +- **External, directly-fetched citations** (not from memory — verified live against the providers' + own current documentation before citing, per this org's traceability convention): + - OpenAI, [*Completions API guide*](https://developers.openai.com/api/docs/guides/completions): + `finish_reason == "length"` — *"it's likely that max_tokens is too small and model runs out of + tokens before it manages to [complete]"*; `max_completion_tokens` — *"an upper bound for the + number of tokens that can be generated for a completion, including visible output tokens and + reasoning tokens."* + - OpenRouter, OpenAPI spec (`https://openrouter.ai/openapi.yaml`), `Model.context_length` — + *"Maximum context length in tokens"* (required); `TopProviderInfo.max_completion_tokens` — + *"Maximum completion tokens from the top provider. Input and output tokens share the context + window, so the effective maximum output for a request is further limited by the context + remaining after input tokens"* (nullable). - `contextual_orchestrator/model_discovery.py`'s `DiscoveredModel` dataclass and `contextual_orchestrator/orchestrator.py`'s `ModelAgent` dataclass — confirmed absence of any context-window/max-output-tokens field via direct grep and full-dataclass read. +- `ContextualWisdomLab/contextual-orchestrator#926`, `#927` — the two tracked upstream follow-ups. - 2026-08-30 gap-baseline entries ("sidecar-preflight outage: family_cap/max_tokens fixes confirmed working end to end...") for the live #1436→120s-timeout evidence this ADR responds to. +- **Live reproduction on this ADR's own PR**, verified directly against the job log rather than taken + on report: `noema-review` on `ContextualWisdomLab/.github#1449` (job `99253418179`, + `https://github.com/ContextualWisdomLab/.github/actions/runs/33310078256/job/99253418179`) — + ``` + 2026-08-30T11:58:29Z healthz and provider-route preflight confirmed after 30s (pid 3973) + 2026-08-30T12:00:29Z curl: (28) Operation timed out after 120002 milliseconds with 0 bytes received + 2026-08-30T12:00:29Z error: gateway preflight request could not reach the local sidecar + ``` + Layer 1 (per-candidate) passed in 30s; Layer 2 (the virtual-pool smoke request) then hung for + exactly the full 120s timeout with **zero bytes received** — not a slow response, not an error + status, literally nothing back. This is a live, current instance of exactly the failure mode + Decision §1's "keep each attempt's own wall-clock timeout short" design targets: under this ADR's + design that hang would be capped at a short per-attempt timeout and recorded as one not-ready + result, not a 120-second block on the whole required check. Confirms this ADR is fixing an active, + currently-blocking defect, not a theoretical one. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index c57850deb1..f2ddf22d9f 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1292,7 +1292,7 @@ conflicting** PRs address pieces of this: currently blocked by the sidecar-preflight outage above, so neither could be re-reviewed to a genuine pass yet regardless of which approach wins. -## 2026-08-30 sidecar preflight `max_tokens`: explicit owner critique, ADR-0005 +## 2026-08-30 sidecar preflight `max_tokens`: explicit owner critique, ADR-0005 (revised after Devin Review) Direct owner feedback after #1436's `max_tokens` 16→4096 raise moved the sidecar's gateway preflight failure from "empty content" to "120s timeout, zero bytes": *"max_tokens 이걸 고정하는 게 말이 안 @@ -1301,28 +1301,40 @@ real ceiling differs too). Both are correct and evidenced, not just asserted: se [`docs/adr/0005-sidecar-preflight-token-budget.md`](adr/0005-sidecar-preflight-token-budget.md) for the full research trail, checked directly against `contextual-orchestrator` source rather than assumed. -Summary of what that ADR found and decided: +**Six Devin Review findings on the ADR's PR (#1449) were each verified and led to real revisions**, not +dismissed — including two genuine design flaws in the original proposal: (1) the original draft would +have reused a single fixed tiny `max_tokens` for every per-candidate probe, which is the same +reasoning-budget-starvation bug class the whole investigation started from, just moved one layer down; +(2) the original draft dropped the sidecar's separate end-to-end virtual-pool smoke request in favor of +per-candidate checks alone, which cannot detect a bug in the virtual-pool dispatch layer itself — already +documented live on PR #1433 (candidate-level preflight passed, the virtual-pool request still 502'd). +Both are fixed in the current ADR text, along with a mischaracterization (the launcher's +`_preflight_review_agents`/`_preflight_with_fallback` per-candidate probing already exists and is being +fixed, not introduced), a conflation of context-window and max-output-tokens as one field (they are two +distinct, separately-nullable quantities — verified directly against OpenRouter's live OpenAPI schema), +missing external citations for provider-behavior claims (added, fetched live from OpenAI's and +OpenRouter's own current docs), and untracked follow-ups (now real issues: +`ContextualWisdomLab/contextual-orchestrator#926`, `#927`). + +Summary of the current ADR: - **No caller-facing lever separates a reasoning budget from a content budget on this gateway.** `ReasoningEffortProfile` is real but additive (still always sets `max_tokens`), opt-in server-side - only, and — critically — the public `/v1/chat/completions` / `/v1/responses` endpoints this preflight - and Strix both use treat a caller-supplied `reasoning_effort`/`reasoning` field as a **documented - no-op**, confirmed directly from `server.py`'s own docstrings. -- **A better-shaped liveness mechanism already exists upstream** (`ModelClient.probe()` / - `provider_readiness_report()`, per-candidate, isolated-failure, exposed as - `GET /api/v1/provider_readiness/latest`), but it is gated at **`admin` scope** while the sidecar's - bearer token is `inference`-scoped — adopting it as-is would be a real privilege widening, not - recommended. -- **No per-model `max_tokens`/context-window ceiling is captured anywhere today.** Confirmed by direct - read: neither `DiscoveredModel` (`model_discovery.py`) nor `ModelAgent` (`orchestrator.py`) carries - any such field, even though several already-queried provider list endpoints publish one. Real, - closeable gap; not same-day sidecar work. -- **Decision**: stop tuning one global constant. Replace the sidecar's single-shot preflight with a - bounded per-candidate probe over the catalog it already builds, requiring only one of several - candidates to succeed (N-of-M), so no single `max_tokens` value has to be simultaneously right for - every model in a heterogeneous pool. Two upstream `contextual-orchestrator` asks (an - inference-scoped readiness probe; a real per-model token-ceiling field in discovery) are tracked as - follow-ups, not closed by this ADR. + only, and the public `/v1/chat/completions`/`/v1/responses` endpoints this preflight and Strix both + use treat a caller-supplied `reasoning_effort`/`reasoning` field as a **documented no-op**. +- **Decision**: keep both existing preflight layers (per-candidate launcher probing; the shell script's + separate virtual-pool smoke request) — fix their shared flaw (a fixed `max_tokens` per attempt) + with a diagnostic, escalate-only-on-positive-evidence retry (only when a response is empty **and** + `finish_reason == "length"` — the provider-documented signature of "budget too small," not "down") + and short per-attempt timeouts, rather than picking a new fixed number. This ADR deliberately does + not commit to specific budget/timeout constants — those should come from real telemetry the + redesigned preflight itself will emit, not from inspection. +- **Live, current evidence this is an active defect, not theoretical**: `noema-review` failed on the + ADR's own PR (#1449, job `99253418179`) with exactly this bug while the ADR was being written — + Layer 1 passed in 30s, Layer 2 then hung the full 120s with zero bytes back. +- Two upstream `contextual-orchestrator` asks are now real tracked issues (`#926`: inference-scoped + readiness probe; `#927`: real per-model `max_output_tokens`/`context_window` discovery data, + correctly modeled as two separate fields), not just prose. Neither blocks the sidecar-side fix. ## 5. 실행 루프와 고객의 다음 행동 From f4617e536cc057a18d725a04acdc30e3e2fefd4b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 12:14:39 +0000 Subject: [PATCH 03/12] docs(adr): fix 5 more Devin Review findings, incl. a critical self-defeating bug Verified each against the actual ADR text and the sidecar/launcher source before acting, per this repo's convention. Finding #1 (critical) was correct: the previous revision's single retry predicate ("empty response AND finish_reason == 'length'") cannot fire for the exact live evidence this ADR cites as its own justification -- a curl timeout with zero bytes received produces no response object at all, so there is no finish_reason to inspect. As written, the ADR would not have fixed the reproduced outage motivating it. Fixed by splitting into two distinct, independently-triggered retries: Trigger A (no usable response -- timeout, connection failure, non-2xx) retries at the same budget, since a hang is not a budget problem; Trigger B (a response was received, empty, finish_reason == "length") escalates the budget. Only Trigger B changes max_tokens. Finding #2 (a real gap): an escalated probe can itself be rejected outright by a model whose real ceiling sits below the escalated budget -- a distinct signature from empty content, now its own recorded outcome (escalated_probe_rejected) rather than blindly retried or conflated with the down case. Finding #3 (real arithmetic problem): an unconditional "one retry per candidate" across up to 12 candidates plus the gateway check was an unbounded-looking worst case against Layer 1's own 180s readiness ceiling. Fixed with explicit, computed, shared per-layer retry budgets: Layer 1 stays within its existing 180s ceiling (12 base attempts + a capped 4 escalations x 10s = 160s). Layer 2 keeps its existing, already-evidenced 120s per-attempt timeout UNCHANGED -- verified against this exact file's own prior comment explaining why 30s was raised to 120s (a real reasoning generation can legitimately need that long, and the job already budgets 120 minutes) -- shortening it would have regressed that fix. Layer 2 gets up to 3 bounded attempts (360s worst case) instead of one with no recovery. Finding #4: committed to concrete initial values instead of deferring every number to future telemetry -- each is either already deployed in this codebase (10s, 120s, 4096, 12) or backed by direct external documentation (16, per OpenRouter's own schema: "some providers enforce a minimum of 16"). Both layers now also emit finish_reason, attempt count, and which trigger fired, so a real follow-up pass can refine these from actual telemetry. Finding #5: source citations are now SHA-pinned permalinks (8b3235d2...) instead of bare line numbers that rot as files change. Co-Authored-By: Claude --- CHANGELOG.md | 23 +- .../0005-sidecar-preflight-token-budget.md | 420 ++++++++++-------- docs/product-technical-gap-baseline.md | 44 +- 3 files changed, 281 insertions(+), 206 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a993534fee..beacb6eccf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,14 +16,21 @@ Semantic Versioning where the repository publishes a release. cannot catch a virtual-pool dispatch bug (already documented live on PR #1433). The current decision keeps both existing preflight layers (`_preflight_review_agents`/`_preflight_with_fallback` in the launcher; the - shell script's virtual-pool request) and fixes their shared flaw with a - diagnostic retry that escalates only when a response is empty AND - `finish_reason == "length"`, plus short per-attempt timeouts, instead of - picking a new fixed number. Adds two real tracked upstream issues - (`ContextualWisdomLab/contextual-orchestrator#926`, `#927`) with external - citations (OpenAI, OpenRouter docs, fetched live) replacing prose-only - follow-ups. No code change in this PR; the sidecar migration is tracked - separately. + shell script's virtual-pool request). A second Devin Review pass then found + the first revision's single retry predicate could not fire for the exact + live evidence cited (a `curl` timeout with zero bytes has no `finish_reason` + to inspect), plus an unbounded-looking worst case and other gaps. Revised + again to model two distinct, explicitly-bounded retry triggers: no-response + (timeout/connection failure) retries at the same budget; a response with + `finish_reason == "length"` escalates the budget. Layer 2's existing, + already-evidenced 120s per-attempt timeout is kept unchanged (shortening it + would regress this file's own prior 30s→120s fix) and gets up to 3 bounded + attempts instead of one with no recovery path; Layer 1 stays within its + existing 180s ceiling via a computed, capped escalation budget. Adds two + real tracked upstream issues (`ContextualWisdomLab/contextual-orchestrator#926`, + `#927`) and SHA-pinned permalink citations (`8b3235d2...`) in place of both + prose-only follow-ups and line numbers that would otherwise rot. No code + change in this PR; the sidecar migration is tracked separately. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index 304e8d1be5..33cd6a06ad 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -1,4 +1,4 @@ -# ADR-0005: Replace the sidecar's fixed-`max_tokens` gateway checks with diagnostic, tolerant readiness +# ADR-0005: Replace the sidecar's fixed-`max_tokens` gateway checks with diagnostic, bounded-retry readiness - Status: proposed - Date: 2026-08-30 @@ -10,11 +10,12 @@ - Decision: Keep both existing preflight layers (per-candidate launcher probing, and the shell script's separate end-to-end request to the virtual `orchestrator/free` model) — neither is being introduced, both already exist and each catches a failure class the other cannot. Fix what is - actually wrong with each: replace their single fixed `max_tokens` value with a diagnostic, - short-timeout, escalate-only-on-positive-evidence probe, so no single number has to be - simultaneously right for every model in a heterogeneous pool, and a reasoning-heavy healthy - candidate is no longer misclassified as down. Track two upstream `contextual-orchestrator` asks - (`ContextualWisdomLab/contextual-orchestrator#926`, `#927`) as real, tracked, non-blocking follow-ups. + actually wrong with each with **two distinct, explicitly-bounded retry mechanisms** — one for "got a + response, it was empty because the budget was too small" (escalate budget), one for "got no response + at all, or a transport-level failure" (retry for a possibly-different route) — each drawing from a + small, explicit, shared attempt budget so worst-case latency is bounded and computed, not open-ended. + Track two upstream `contextual-orchestrator` asks (`ContextualWisdomLab/contextual-orchestrator#926`, + `#927`) as real, tracked, non-blocking follow-ups. - Ownership: `.github` owns the sidecar/launcher script and this ADR; `ContextualWisdomLab/contextual-orchestrator` owns the gateway internals cited as evidence and the two follow-up issues. - Figma File ID: N/A (no customer UI). @@ -22,28 +23,47 @@ ## Context Central review (`noema-review`/`opencode-review`/`strix`) depends on two separate, already-existing -liveness checks in the vendored sidecar, run in sequence — this ADR fixes both, it introduces neither: - -1. **Per-candidate launcher probing.** `scripts/ci/contextual_orchestrator_review_launcher.py`'s - `_preflight_review_agents()` (line 200) sends one bounded `POST` to `client.proxy_send_once` for - *each* candidate agent in the admitted catalog, with a fixed `max_tokens=REVIEW_MAX_OUTPUT_TOKENS` - (currently `4096`, line 38) and a fixed `temperature=REVIEW_TEMPERATURE`. It keeps every candidate - whose response has non-empty text (`_chat_response_has_text`, line 175 — checks only - `choices[0].message.content`, never inspects `finish_reason`) and raises `ReviewPreflightError` - only if **zero** candidates pass — i.e. it is already an N-of-M ("at least one must work") design, - not a single-candidate gate. `_preflight_with_fallback()` (line 274) wraps this with one fallback - catalog tier. This runs inside the Python process at server startup, before the sidecar can even - report healthy. -2. **The shell script's own virtual-pool smoke request.** Once the server is up, - `contextual_orchestrator_review_sidecar.sh` separately sends one `POST /v1/chat/completions` with - `"model":"orchestrator/free"` (the *virtual* pool id, not a specific candidate) and its own fixed - `max_tokens` — this is the request `N` below refers to. - -`N` has already been tuned twice: 16 → 4096 (#1436), moving the failure from "empty content at 16 -tokens" (the provider's response consumed the whole budget on internal reasoning before emitting -visible content — see `ModelClient._response_content`'s own anticipated error message, quoted below) -to "120s timeout with zero bytes at 4096 tokens" on a separate run. Direct owner feedback in response -to that outcome, quoted verbatim because it is the reason this ADR exists: +liveness checks in the vendored sidecar, run in sequence — this ADR fixes both, it introduces neither. +Citations below pin to the exact reviewed blob at `main`'s +`8b3235d22129035b49ac481a40a341002540e2af` so line numbers cannot rot as the files change later. + +1. **Per-candidate launcher probing** (bounded by the sidecar's own 180-second healthz-readiness wait — + see the family-cap comment in the sidecar script; this happens *before* the process can report + healthy, one candidate at a time, within that budget). + [`_preflight_review_agents()`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L200-L271) + sends one bounded `POST` to `client.proxy_send_once` for *each* candidate agent in the admitted + catalog, with a fixed `max_tokens=REVIEW_MAX_OUTPUT_TOKENS` (currently `4096`, + [L38](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L38)) + under a per-attempt + [`REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L45) + ceiling. It keeps every candidate whose response has non-empty text + ([`_chat_response_has_text`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L175-L189) + — checks only `choices[0].message.content`, never inspects `finish_reason`) and raises + `ReviewPreflightError` only if **zero** candidates pass — i.e. it is already an N-of-M ("at least one + must work") design, not a single-candidate gate. + [`_preflight_with_fallback()`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L274-L291) + wraps this with one fallback catalog tier. +2. **The shell script's own virtual-pool smoke request.** Once `/healthz` succeeds (a separate, + already-completed budget — Layer 2 does not draw from Layer 1's 180s), the shell script sends one + `POST /v1/chat/completions` with `"model":"orchestrator/free"` (the *virtual* pool id, not a + specific candidate) and its own fixed `max_tokens`, currently `4096`, under a **120-second** + `curl --max-time`. This 120s value is itself the outcome of a prior, real, evidenced fix in this + exact file (raised from a too-tight 30s after live reproduction on + `ContextualWisdomLab/contextual-orchestrator#921` showed a genuinely-healthy DeepSeek NIM route + needing more than 30s to complete a real generation) — the comment there explicitly documents that + this required-workflow job budgets **120 minutes** total (`timeout-minutes` in + `strix.yml`/`noema-review.yml`) and that *"the org's own stated policy accepts multi-hour central + review latency in favor of accuracy over speed."* This ADR's design deliberately **does not shorten + that 120s value** — doing so would reintroduce the exact regression that prior fix corrected. The + correct fix for a hang, per Devin Review (see Decision §1), is a bounded *retry*, not a shorter + *timeout*. + +`N` (the `max_tokens` literal) has already been tuned twice: 16 → 4096 (#1436), moving the failure +from "empty content at 16 tokens" (the provider's response consumed the whole budget on internal +reasoning before emitting visible content — see `ModelClient._response_content`'s own anticipated +error message, quoted below) to "120s timeout with zero bytes at 4096 tokens" on a separate run. +Direct owner feedback in response to that outcome, quoted verbatim because it is the reason this ADR +exists: > "max_tokens 이걸 고정하는 게 말이 안 되는데" — hardcoding this max_tokens doesn't make sense. > "모델마다 max_tokens 허용치가 다 다른데" — each model has a genuinely different max_tokens allowance. @@ -57,16 +77,12 @@ evidenced axes for a pool like this: before emitting visible content can exhaust a small budget with zero visible output. OpenAI's own documentation of `finish_reason == "length"` describes exactly this: *"it's likely that max_tokens is too small and model runs out of tokens before it manages to [complete]"* - ([OpenAI API guide](https://developers.openai.com/api/docs/guides/completions)). This is very - likely what turned #1436's 4096-token raise into a 120-second timeout instead of a fix — a large - budget lets a heavy-reasoning model legitimately run far longer than a fast, non-reasoning model - would for the same request (see the 2026-08-30 gap-baseline entry's "third, distinct failure mode"). + ([OpenAI API guide](https://developers.openai.com/api/docs/guides/completions)). 2. **The provider's own hard ceiling on completion tokens differs per model**, and is a genuinely separate quantity from a model's context window (see Research §3 below). Some providers reject a request outright if `max_tokens` exceeds what that specific model supports; others support far more than a generic constant would ever request. A single number can therefore be simultaneously too - small for one model's reasoning overhead and too large for another model's real ceiling — there is - structurally no number that is not wrong for some member of the pool. + small for one model's reasoning overhead and too large for another model's real ceiling. The standing session principle governing this decision, also quoted verbatim: "어떠한 휴리스틱과 Rule of thumbs도 금지" — no heuristics or rules of thumb; a parameter needs actual justification from real @@ -77,172 +93,203 @@ data, not a constant that happens to work today. ### 1. Does the gateway expose a way to separate a reasoning budget from a content budget? -**No — not on the endpoint this preflight uses, and not for how the preflight calls it.** Checked -directly in `contextual_orchestrator/orchestrator.py` and `server.py`, not assumed: - -- `ReasoningEffortProfile`/`apply_request_profile()` (`reasoning_effort_profile.py`) is a real, - capability-gated mechanism (`ModelAgent.reasoning_effort_supported: bool | None`, fail-closed unless - proven `True`), but it is **additive, not substitutive**: `apply_request_profile()` always sets - `payload["max_tokens"] = validated.max_output_tokens` regardless of whether `reasoning_effort` is - also set. There is no "unbounded reasoning + bounded content" mode. This matches how OpenAI itself - documents the analogous parameter: `max_completion_tokens` is *"an upper bound for the number of - tokens that can be generated for a completion, **including** visible output tokens and reasoning - tokens"* (same OpenAI guide) — reasoning and visible content already share one budget upstream, by - design, not only in this gateway. -- This mechanism is **opt-in at `TaskOrchestrator` construction**, not caller-controlled: - `_role_effort_profile(role)` returns `None` unless the server was constructed with an explicit - `role_effort_catalog` (`orchestrator.py:5530-5534`). -- The **public `/v1/chat/completions` endpoint the sidecar and Strix's client both call does not - thread a caller-supplied `reasoning_effort`/`reasoning` field into orchestration at all.** - `server.py`'s own docstrings say so directly: `_validate_chat_reasoning_effort` — "This gateway - never threads the knob into `ModelClient` on the orchestration path. Known levels are accepted as - default-effort no-ops"; `_validate_responses_reasoning` (the `/v1/responses` equivalent) — "This - gateway proxies Responses but does not interpret or enforce reasoning controls." Switching the - preflight to `/v1/responses` would not gain anything here — the field is a documented no-op on both. +**No.** `ReasoningEffortProfile`/`apply_request_profile()` (`reasoning_effort_profile.py`) is real but +**additive, not substitutive**: it always sets `payload["max_tokens"]` regardless of `reasoning_effort`. +OpenAI documents the analogous parameter the same way: `max_completion_tokens` is *"an upper bound for +the number of tokens that can be generated for a completion, **including** visible output tokens and +reasoning tokens"* (same OpenAI guide). The mechanism is also opt-in at `TaskOrchestrator` construction +(`_role_effort_profile(role)` returns `None` unless a `role_effort_catalog` was configured), and the +public `/v1/chat/completions`/`/v1/responses` endpoints this preflight and Strix use both treat a +caller-supplied `reasoning_effort`/`reasoning` field as a documented no-op (`server.py`'s own +docstrings: `_validate_chat_reasoning_effort`, `_validate_responses_reasoning`). **Conclusion**: there is no lever, on any caller-facing surface this preflight (or Strix) can reach, that separates "let the model think as long as it needs" from "cap what it can emit." ### 2. Is a real-generation preflight even the right liveness mechanism — is there a cheaper or more direct signal? -**A better-shaped mechanism than a single fixed-budget request exists in two places — one already in -this sidecar, one further upstream — but neither is a free non-generation signal.** Checked directly: - -- **Already in this repo**: `_preflight_review_agents()` (described in Context above) already probes - every candidate individually and already tolerates any number of individual failures — it fails the - whole preflight only when literally none pass. What it lacks is not the *shape* (that already - exists) but a way to tell "this candidate is down" apart from "this candidate is healthy but its - first probe's budget was wrong for it" — seе Decision §1 below. -- **Further upstream, admin-scoped**: `ModelClient.probe()` (`orchestrator.py:1483`) and - `TaskOrchestrator.provider_readiness_report()` (`orchestrator.py:3441`, exposed as - `GET /api/v1/provider_readiness/latest?refresh=true`, `server.py:5711-5715`) are the gateway's own, - more mature version of the same idea — per-candidate, isolated failure, real `failure_code` - diagnostics. **Verified directly, and this is a real blocker to adopting it as-is**: `/api/v1/*` GET - routes are authorized at **`admin` scope** (`server.py`'s `_admin_purpose()` / - `self._authorize("admin", ...)`), while `/v1/chat/completions` — what the sidecar's bearer token is - scoped for today — is authorized at the separate, narrower **`inference` scope**. Provisioning the - CI review sidecar with an admin-scoped token just to call a readiness endpoint would be a real - privilege widening this ADR does not recommend. Tracked as - `ContextualWisdomLab/contextual-orchestrator#926`. -- **Neither eliminates real generation.** `probe()` itself still hardcodes `max_tokens: 1` - (`orchestrator.py:1536`) — even more aggressive than either value this sidecar has tried, and - vulnerable to the exact same reasoning-overhead misclassification described above. Adopting - `provider_readiness_report`'s *shape* without also fixing this calibration problem would just move - the bug, not fix it — which is Devin Review's finding on an earlier draft of this ADR (see Decision - §1 for the actual fix). - -**Conclusion**: reuse the shape that already exists in this sidecar (per-candidate, N-of-M-tolerant); -fix its calibration (Decision §1); track the upstream, better-tested version as a non-blocking -follow-up (`#926`) because it is currently out of reach at this token's privilege level. +**A better-shaped mechanism exists in two places — one already in this sidecar, one further +upstream — but neither is a free non-generation signal.** + +- **Already in this repo**: `_preflight_review_agents()` already probes every candidate individually + and already tolerates any number of individual failures. What it lacks is not the *shape* but a way + to tell "this candidate is down" apart from "this candidate is healthy but its probe's budget was + wrong for it," and (separately) a way to survive a hang with no response at all — see Decision §1. +- **Further upstream, admin-scoped**: `ModelClient.probe()`/`provider_readiness_report()` are the + gateway's own, more mature version of the same idea. Verified directly: `/api/v1/*` GET routes are + authorized at **`admin` scope**, while `/v1/chat/completions` — what the sidecar's bearer token is + scoped for today — is authorized at the narrower **`inference` scope**. Provisioning the sidecar with + an admin-scoped token just for this would be a real privilege widening this ADR does not recommend. + Tracked as `ContextualWisdomLab/contextual-orchestrator#926`. +- **Neither eliminates real generation, and neither eliminates the possibility of a hang.** `probe()` + itself hardcodes `max_tokens: 1` and has no retry of its own. + +**Conclusion**: reuse the shape that already exists in this sidecar; fix its calibration and add +bounded retries (Decision §1); track the upstream, better-tested version as a non-blocking follow-up. ### 3. If a numeric budget is still needed, can it be derived per-model from real discovered data? -**Not today — confirmed as a genuine, currently-open gap.** Checked both schemas directly: - -- `contextual_orchestrator/model_discovery.py`'s `DiscoveredModel` dataclass and - `contextual_orchestrator/orchestrator.py`'s `ModelAgent` dataclass carry no field for a model's - output-token ceiling or context window — confirmed via full-dataclass read and grep. -- This is real, closeable data loss, and it is **two distinct pieces of data, not one** — verified - directly against a live provider schema rather than assumed uniform. OpenRouter's current OpenAPI - spec (`https://openrouter.ai/openapi.yaml`) defines the `Model` object's `context_length` field - (required) as *"Maximum context length in tokens"*, and separately, `TopProviderInfo.max_completion_tokens` - (nullable — genuinely absent for some models) as *"Maximum completion tokens from the top provider. - Input and output tokens share the context window, so the effective maximum output for a request is - further limited by the context remaining after input tokens."* Only the second field can directly - clamp a `max_tokens` request parameter; the first constrains prompt+output together and is not a - substitute for it — conflating them would let a large-context, small-output model's window size - wrongly justify a `max_tokens` far beyond what that model can actually complete in. - -**Conclusion**: deriving a real per-model ceiling is the *correct* long-term answer to the owner's -second axis, but requires a schema extension distinguishing `max_output_tokens` from `context_window` -as two separately-provenanced, independently-nullable fields, plus per-provider field-mapping research -(schemas are not uniform across the five configured providers). Tracked as -`ContextualWisdomLab/contextual-orchestrator#927`, not undertaken in this ADR. +**Not today.** Neither `DiscoveredModel` (`model_discovery.py`) nor `ModelAgent` (`orchestrator.py`) +carries any field for a model's output-token ceiling or context window — confirmed via full-dataclass +read and grep. This is **two distinct pieces of data, not one** — verified directly against +OpenRouter's current OpenAPI spec (`https://openrouter.ai/openapi.yaml`): `Model.context_length` +(required) is *"Maximum context length in tokens"*; `TopProviderInfo.max_completion_tokens` (nullable +— genuinely absent for some models) is *"Maximum completion tokens from the top provider. Input and +output tokens share the context window, so the effective maximum output for a request is further +limited by the context remaining after input tokens."* Only the second field can directly clamp a +`max_tokens` request parameter. + +**Conclusion**: tracked as `ContextualWisdomLab/contextual-orchestrator#927`, not undertaken here. ## Decision -1. **Fix both existing preflight layers' probe calibration with diagnostic, escalate-on-evidence - retries — do not introduce a new mechanism, and do not remove either existing layer.** - - **Layer 1 (`_preflight_review_agents`, per candidate)**: for each candidate, send a first bounded - probe at a modest token budget. If the response is empty/whitespace **and** its - `choices[0].finish_reason == "length"` — the exact, provider-documented signature of "the - budget was too small," not "the candidate is unreachable" — retry that *same* candidate once at - a materially larger budget before recording it as rejected. Every other failure class (timeout, - connection error, non-2xx status, or empty content with any other `finish_reason`) is **not** - retried — those are not budget problems, and retrying would not fix them. This directly answers - Devin Review's finding: a fixed tiny budget (whether `1`, matching upstream `probe()`'s own - precedent, or any other single constant) would still misclassify a healthy reasoning-heavy - candidate as down; escalating only on the specific evidence that the budget — not the candidate - — was the problem does not have this failure mode, because a genuinely-down candidate never - reaches the retry path. - - **Layer 2 (the shell script's virtual-pool smoke request)**: apply the same diagnostic escalation - to the real end-to-end `POST /v1/chat/completions` request against `"model":"orchestrator/free"`. - This layer is **kept, not replaced by Layer 1** — Layer 1's per-candidate checks call - `client.proxy_send_once` against explicit candidate agents directly and structurally cannot - detect a bug in the virtual-pool's own dispatch/selection code, which is a different code path. - This is not hypothetical: the 2026-08-30 gap-baseline entry for PR #1433 records exactly this - split failure live — the launcher's own per-candidate preflight passed and the server reported - healthy, while the shell script's separate virtual-pool request still came back `HTTP 502`. Any - redesign that dropped Layer 2 in favor of Layer 1 alone would silently reintroduce that exact, - already-documented gap. Bound Layer 2 to at most 2 total attempts (matching Layer 1's own retry - bound) so a genuinely broken virtual-pool layer still fails fast. - - **Keep each attempt's own wall-clock timeout short**, independent of the token-budget question. - A candidate or route that is simply slow or hung should fail *that attempt* quickly and be - recorded as not ready — tolerable under Layer 1's existing N-of-M design and Layer 2's bounded - retry — rather than the preflight trying to avoid ever hitting a timeout by picking a "safer" - token budget. This decouples the two previously-conflated failure modes (wrong budget vs. slow - response) that made #1436's single-number tuning symptom-chase between them. - - **This ADR deliberately does not fix specific numeric values** for the modest/escalated budgets - or the per-attempt timeout. Committing to new constants here would repeat the same mistake at one - remove — picking numbers by inspection rather than evidence. Both existing preflight layers - already emit a structured per-route report (`_preflight_review_agents`'s `routes[]`, and the - shell script's `preflight_report`/`gateway` JSON) — the follow-up implementation PR should add - `finish_reason` and attempt-count to that evidence and let the actual values be set from real - telemetry once deployed, not guessed in this document. -2. **Track `ContextualWisdomLab/contextual-orchestrator#926`** (an `inference`-scoped variant of - `provider_readiness_report`/`probe()`) so the sidecar can eventually retire its hand-rolled Layer 1 - loop in favor of the gateway's own, better-tested mechanism. Not blocking for item 1. -3. **Track `ContextualWisdomLab/contextual-orchestrator#927`** (real, separately-provenanced - `max_output_tokens`/`context_window` fields in `DiscoveredModel`/`ModelAgent`, fail-closed when - unknown) so `max_tokens` selection can eventually be derived from real per-model data. Not blocking - for item 1; this is the correct long-term closure of the owner's second axis. -4. **Explicitly reject** further tuning of one global `max_tokens` constant as a terminal fix for - either layer. Every value tried so far (16, 4096) has failed for a different, evidenced reason tied - to pool heterogeneity, confirming the owner's original objection rather than one bad guess needing - one better guess. +### 1. Two distinct, explicitly-bounded retry mechanisms, not one generic "retry" + +Devin Review correctly found that a single "retry on empty content + `finish_reason == 'length'`" +predicate cannot fix the actual live outage this ADR is responding to: the reproduced failure (job +`99253418179`, cited in the Evidence trail) is a **120-second timeout with zero bytes received** — +there is no response object at all in that case, so there is no `finish_reason` to inspect, and the +original design's retry path would never trigger for it. Fixed by splitting into two independent +triggers, both bounded, both explicit: + +- **Trigger A — no usable response** (transport timeout, connection failure, or non-2xx status on the + *first* attempt at a given budget): retry with a **fresh attempt at the same budget**, on the theory + that the virtual pool's internal routing (Layer 2) or a flaky provider (either layer) may behave + differently on a new attempt — this is not a budget problem, so escalating the budget would not help + and is not done here. +- **Trigger B — a response was received, content is empty, and `choices[0].finish_reason == "length"`** + (the OpenAI-documented signature of "budget too small," cited above): retry the *same* candidate/route + once at a **materially larger** budget. This is the only trigger that changes the budget. +- **Neither trigger fires more than once per attempt distinguishing between them, and both draw from + one small, shared, explicit retry budget per layer** (Decision §3) — not "one retry per route" + unconditionally, which is what produced Devin's second finding (an unbounded-looking worst case). +- **A non-2xx rejection specifically on a Trigger-B escalated attempt is not itself retried further.** + If a candidate's *first* attempt at the base budget succeeds or fails openly (Trigger A), that is + handled as above. If it instead comes back empty with `finish_reason == "length"` and the escalated + retry (Trigger B) is then rejected outright (non-2xx) rather than merely still empty, that is + distinguishable evidence the *escalated* budget — not the base one — exceeds this specific model's + real ceiling (Devin Review's second finding). Retrying again with an even larger budget would not be + justified by any evidence in hand and risks the same rejection; instead this is recorded as its own + distinct outcome (e.g. `escalated_probe_rejected`) and the candidate/route is treated as not-ready + for this run. This is an honest, bounded limitation, not a silent misclassification — the complete + fix (knowing each model's real ceiling in advance) is `ContextualWisdomLab/contextual-orchestrator#927`, + not this ADR. +- **Every other outcome is not retried**: a non-2xx or empty-with-a-different-`finish_reason` result on + an attempt that is not eligible for Trigger A or B (i.e., already a retry, or already past the shared + budget) is recorded as not-ready immediately. + +### 2. Keep both existing layers — neither replaces the other + +Layer 1's per-candidate checks call `client.proxy_send_once` against explicit candidate agents directly +and structurally cannot detect a bug in the virtual pool's own dispatch/selection code, which is a +different code path. This is not hypothetical: the 2026-08-30 gap-baseline entry for PR #1433 records +exactly this split failure live — the launcher's own per-candidate preflight passed and the server +reported healthy, while the shell script's separate virtual-pool request still came back `HTTP 502`. +Layer 2 also independently reproduced the ADR's own motivating bug live on PR #1449 itself (Evidence +trail). Any redesign that dropped Layer 2 in favor of Layer 1 alone would silently reintroduce both. + +### 3. Explicit, bounded, per-layer retry budgets and the resulting worst-case arithmetic + +Devin Review's third finding is correct: `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES` (12) candidates each +retried once, unconditionally, would be a real, computed worst-case blowup against Layer 1's own +180-second healthz-readiness budget. Fixed with an explicit shared cap per layer, not an unbounded +"one retry per route": + +- **Layer 1** (bounded by the existing 180s healthz-readiness wait, unchanged): keep the existing + per-attempt timeout (`REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10`, unchanged) and the existing base probe + budget. Trigger A does not need its own retry allowance here — Layer 1 already has up to 12 distinct + candidates providing exactly the resilience a same-candidate retry-on-hang would give, so one + candidate's timeout simply consumes its 10s slot and the loop moves to the next candidate, as today. + Only Trigger B (escalate budget on `finish_reason == "length"`) is new here, and it is capped by a + new shared counter, `REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4`, across the whole Layer 1 run (not + per-candidate) — once 4 candidates have consumed an escalation attempt, any further candidate that + would otherwise qualify for Trigger B is instead recorded not-ready immediately with an explicit + `escalation_budget_exhausted` reason. **Worst case**: 12 × 10s (base attempts) + 4 × 10s (escalation + attempts) = **160s**, under the existing 180s ceiling with real margin, computed rather than assumed. +- **Layer 2** (bounded only by the job's own 120-minute ceiling, per the org's stated "accuracy over + speed" policy already reasoned in this file — *not* by the 180s Layer 1 budget, which has already + completed by the time Layer 2 runs): keep the existing per-attempt timeout (**120s, unchanged** — not + shortened, per Context above) and the existing `4096` base budget (already proven working on a real + hosted run, `contextual-orchestrator#921`). Allow up to `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3` + total attempts, covering both Trigger A (transport failure/hang) and Trigger B (empty + + `finish_reason == "length"`, escalating to a larger budget on the *next* attempt only) before failing + closed with the specific last-observed reason. **Worst case**: 3 × 120s = **360s (6 minutes)** — + explicit, bounded, and small relative to the job's 120-minute ceiling; the previous design's worst + case was already 120s for one unconditional attempt with no chance of recovery, so this trades a + bounded amount of additional worst-case latency for surviving exactly the transient-hang class of + failure reproduced live on this ADR's own PR. +- **Initial values are reused precedent, not new guesses** (Devin Review's fourth finding): every + number above is either already deployed in this exact codebase today (`10s`, `120s`, `4096`, `12`) + or has direct external documentation backing it (`16` — the pre-#1436 value this codebase already + ran with, and separately the floor OpenRouter's own schema documents: *"some providers enforce a + minimum of 16"* for the deprecated `max_tokens` field). The two new counters + (`REVIEW_PREFLIGHT_MAX_ESCALATIONS`, `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS`) are chosen to keep each + layer's worst case under its own already-established ceiling, shown above, not picked by inspection + of "what feels right." Both preflight layers now emit `finish_reason`, attempt count, and which + trigger fired in their structured reports (`_preflight_review_agents`'s `routes[]`; the shell script's + `preflight_report`/`gateway` JSON) specifically so that a **follow-up, evidence-driven pass** — after + observing real hosted runs with this telemetry — can adjust these two counters and the base/escalated + token budgets from real data, which is the methodology this ADR commits to for future tuning: initial + values from direct precedent, refinement from telemetry this change itself introduces, never from + inspection alone. + +### 4. Upstream tracking and rejection of further constant-tuning + +- **Track `ContextualWisdomLab/contextual-orchestrator#926`** (an `inference`-scoped variant of + `provider_readiness_report`/`probe()`) so the sidecar can eventually retire its hand-rolled Layer 1 + loop. Not blocking for §1-3. +- **Track `ContextualWisdomLab/contextual-orchestrator#927`** (real, separately-provenanced + `max_output_tokens`/`context_window` fields, fail-closed when unknown) so `max_tokens` selection can + eventually be derived from real per-model data, including resolving the `escalated_probe_rejected` + case in §1 properly instead of just recording it. Not blocking for §1-3. +- **Explicitly reject** further tuning of one global `max_tokens` constant, or of a single generic + "retry," as a terminal fix for either layer. Every single-constant value tried so far (16, 4096) has + failed for a different, evidenced reason tied to pool heterogeneity, and a single undifferentiated + retry predicate does not cover the failure class (a hang) that actually reproduced live on this ADR's + own PR. ## Consequences - Both preflight layers become structurally tolerant of an individual attempt being wrong for a fixed - token budget or briefly slow, which is the actual shape of the problem — instead of continuing to - search for a number that fits every model in a heterogeneous pool, no single number is asked to, and - a genuinely down candidate or route is still detected and reported, just no longer conflated with a - merely-miscalibrated one. -- Total worst-case preflight latency grows modestly (up to one extra retry per candidate in Layer 1, - up to one extra retry in Layer 2), bounded by the existing `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES` / - `REVIEW_PREFLIGHT_TIMEOUT_SECONDS` ceiling and the new short per-attempt timeouts — this trades a - small amount of latency for the diagnostic precision that avoids the false-outage misclassification - Devin Review flagged. + token budget, or hanging/failing transiently, which is the actual shape of the problem — while + keeping every worst case explicit and bounded rather than open-ended. +- Layer 1's worst case grows from ~120s to a computed 160s, still under its existing 180s + healthz-readiness ceiling. Layer 2's worst case grows from a single 120s attempt with no recovery + path to up to 360s across bounded retries — small relative to the job's 120-minute ceiling and + consistent with this file's own already-stated "accuracy over speed" policy. - Keeping Layer 2 (not just Layer 1) means the preflight still proves the actual consumer-facing `orchestrator/free` route works, not only that individual candidates can respond in isolation — - closing the PR #1433 gap class rather than reopening it. -- Items 2 and 3 are real `contextual-orchestrator` feature work, now tracked as real issues - (`#926`, `#927`), and are explicitly not closed by this ADR. + closing the PR #1433 gap class rather than reopening it. Giving Layer 2 a bounded retry (rather than + either a single unconditional attempt or a shortened timeout) is what actually fixes the live + 120s-hang reproduction on this ADR's own PR — a shortened timeout alone would not have, and would + have regressed the prior, already-evidenced 30s→120s fix in the same file. +- A candidate whose escalated probe is rejected outright (rather than merely still empty) is recorded + as not-ready with a distinct, honest reason rather than silently retried indefinitely or + misclassified — a known, accepted, documented residual limitation until + `ContextualWisdomLab/contextual-orchestrator#927` lands. +- Items in Decision §4 are real `contextual-orchestrator` feature work, now tracked as real issues, + and are explicitly not closed by this ADR. - No production routing default changes; this is scoped to the sidecar's own liveness checks. - **This is currently active, not theoretical**: the live reproduction in the Evidence trail below is from `noema-review` failing on this ADR's own PR while this ADR was being written, presently blocking that required check org-wide on every repo that routes through this sidecar. The - implementation follow-up (a separate PR applying Decision §1) should be prioritized accordingly once - this ADR is settled, not treated as ordinary backlog. + implementation follow-up applying this Decision should be prioritized accordingly, not treated as + ordinary backlog. ## Evidence trail -- `scripts/ci/contextual_orchestrator_review_launcher.py`: `_preflight_review_agents` (L200-271), - `_preflight_with_fallback` (L274-291), `_chat_response_has_text` (L175-189), - `REVIEW_MAX_OUTPUT_TOKENS = 4096` (L38) — the existing Layer 1 mechanism this ADR fixes, not - introduces. -- `scripts/ci/contextual_orchestrator_review_sidecar.sh` — the existing Layer 2 virtual-pool smoke - request this ADR keeps. +All source citations below are permalinks to the exact reviewed blob at +`8b3235d22129035b49ac481a40a341002540e2af` (the `main` commit this research was performed against), so +line numbers cannot rot as these files are edited later. + +- [`_preflight_review_agents`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L200-L271), + [`_preflight_with_fallback`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L274-L291), + [`_chat_response_has_text`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L175-L189), + [`REVIEW_MAX_OUTPUT_TOKENS`/`REVIEW_PREFLIGHT_TIMEOUT_SECONDS`/`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L36-L47) + — the existing Layer 1 mechanism this ADR fixes, not introduces. +- [`scripts/ci/contextual_orchestrator_review_sidecar.sh`, the healthz-wait loop and its 180s budget comment](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_sidecar.sh#L67-L69), + and [the virtual-pool smoke request and its existing 30s→120s rationale](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_sidecar.sh#L430-L475) + — the existing Layer 2 mechanism this ADR fixes, not introduces or shortens. - 2026-08-30 gap-baseline entry (PR #1433 evidence): *"the shell script's separate, subsequent real `/v1/chat/completions` gateway smoke request against the now-serving `orchestrator/free` virtual model came back HTTP 502. This is a different code path than the launcher's own preflight @@ -260,8 +307,8 @@ as two separately-provenanced, independently-nullable fields, plus per-provider - `ModelClient.probe` (`orchestrator.py:1483-1561`), `TaskOrchestrator.provider_readiness_report` (`orchestrator.py:3441-3486`), `server.py:5711-5715` — the upstream mechanism, and its admin-scope gate vs. the `inference`-scoped `/v1/chat/completions`/`/v1/models` handlers. -- **External, directly-fetched citations** (not from memory — verified live against the providers' - own current documentation before citing, per this org's traceability convention): +- **External, directly-fetched citations** (verified live against the providers' own current + documentation before citing, per this org's traceability convention): - OpenAI, [*Completions API guide*](https://developers.openai.com/api/docs/guides/completions): `finish_reason == "length"` — *"it's likely that max_tokens is too small and model runs out of tokens before it manages to [complete]"*; `max_completion_tokens` — *"an upper bound for the @@ -271,13 +318,10 @@ as two separately-provenanced, independently-nullable fields, plus per-provider *"Maximum context length in tokens"* (required); `TopProviderInfo.max_completion_tokens` — *"Maximum completion tokens from the top provider. Input and output tokens share the context window, so the effective maximum output for a request is further limited by the context - remaining after input tokens"* (nullable). -- `contextual_orchestrator/model_discovery.py`'s `DiscoveredModel` dataclass and - `contextual_orchestrator/orchestrator.py`'s `ModelAgent` dataclass — confirmed absence of any - context-window/max-output-tokens field via direct grep and full-dataclass read. + remaining after input tokens"* (nullable); the deprecated `max_tokens` field description — + *"Note: some providers enforce a minimum of 16"* — the direct evidence for this ADR's `16`-token + Layer 1 base probe value. - `ContextualWisdomLab/contextual-orchestrator#926`, `#927` — the two tracked upstream follow-ups. -- 2026-08-30 gap-baseline entries ("sidecar-preflight outage: family_cap/max_tokens fixes confirmed - working end to end...") for the live #1436→120s-timeout evidence this ADR responds to. - **Live reproduction on this ADR's own PR**, verified directly against the job log rather than taken on report: `noema-review` on `ContextualWisdomLab/.github#1449` (job `99253418179`, `https://github.com/ContextualWisdomLab/.github/actions/runs/33310078256/job/99253418179`) — @@ -287,9 +331,7 @@ as two separately-provenanced, independently-nullable fields, plus per-provider 2026-08-30T12:00:29Z error: gateway preflight request could not reach the local sidecar ``` Layer 1 (per-candidate) passed in 30s; Layer 2 (the virtual-pool smoke request) then hung for - exactly the full 120s timeout with **zero bytes received** — not a slow response, not an error - status, literally nothing back. This is a live, current instance of exactly the failure mode - Decision §1's "keep each attempt's own wall-clock timeout short" design targets: under this ADR's - design that hang would be capped at a short per-attempt timeout and recorded as one not-ready - result, not a 120-second block on the whole required check. Confirms this ADR is fixing an active, - currently-blocking defect, not a theoretical one. + exactly the full 120s timeout with **zero bytes received** — no response, no `finish_reason`, + nothing. This is exactly Decision §1's Trigger A case (not Trigger B, which requires a response to + exist) — confirming why the two triggers had to be modeled separately, and why this specific evidence + is what Decision §3's Layer 2 bounded-retry design (up to 3 attempts) exists to survive. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index f2ddf22d9f..01442f1a35 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1316,22 +1316,48 @@ missing external citations for provider-behavior claims (added, fetched live fro OpenRouter's own current docs), and untracked follow-ups (now real issues: `ContextualWisdomLab/contextual-orchestrator#926`, `#927`). +**A second Devin Review pass found 5 more issues, the most important of which showed the first revision +still did not fix its own motivating bug — verified and fixed, not dismissed.** Finding #1 (critical): +the first revision's single retry predicate ("empty response AND `finish_reason == 'length'`") cannot +fire for the exact live evidence cited above (a `curl` timeout with zero bytes) — a transport-level +hang produces no response object at all, so there is no `finish_reason` to inspect, meaning the ADR as +written would not have fixed the reproduction it cites as its own justification. Finding #2: an +escalated (larger) probe can itself get rejected outright by a model whose real ceiling sits between +the base and escalated budgets — a distinct failure signature from "empty content," previously +unhandled. Finding #3: an unconditional "one retry per candidate" across up to 12 candidates plus the +gateway check is an unbounded-looking worst case against Layer 1's own 180s readiness ceiling. Finding +#4: deferring every numeric constant to "future telemetry" is circular — initial deployment still needs +justified starting values. Finding #5: citations to this repo's own source by line number rot as the +file changes; needs SHA-pinned permalinks. + +**Fixed by modeling two distinct, explicitly-bounded retry triggers instead of one**: Trigger A (no +usable response — timeout, connection failure, non-2xx) retries at the *same* budget, since a hang is +not a budget problem; Trigger B (a response *was* received, empty, `finish_reason == "length"`) +escalates the budget. An escalated-attempt rejection is its own recorded outcome, not blindly retried +again. Each layer draws from a small, computed, shared retry budget — Layer 1 stays within its existing +180s ceiling (12 base attempts + 4 escalations × 10s = 160s, explicit); Layer 2 keeps its existing, +already-evidenced 120s per-attempt timeout **unchanged** (shortening it would have regressed the prior, +already-reasoned 30s→120s fix in the same file, since a real reasoning generation can legitimately need +that long and the job already budgets 120 minutes total) and gets up to 3 total attempts (360s worst +case) instead of one unconditional attempt with no recovery path. Initial numeric values (`16`, `4096`, +`10s`, `120s`, and the two new attempt-count caps) are each either already deployed in this codebase or +backed by direct external documentation (OpenRouter's own schema: *"some providers enforce a minimum of +16"*), not fresh guesses — both preflight layers now also emit `finish_reason`/attempt-count/trigger +telemetry specifically so a future pass can refine these from real data. Source citations are now +SHA-pinned permalinks (`8b3235d2...`) instead of bare line numbers. + Summary of the current ADR: - **No caller-facing lever separates a reasoning budget from a content budget on this gateway.** `ReasoningEffortProfile` is real but additive (still always sets `max_tokens`), opt-in server-side only, and the public `/v1/chat/completions`/`/v1/responses` endpoints this preflight and Strix both use treat a caller-supplied `reasoning_effort`/`reasoning` field as a **documented no-op**. -- **Decision**: keep both existing preflight layers (per-candidate launcher probing; the shell script's - separate virtual-pool smoke request) — fix their shared flaw (a fixed `max_tokens` per attempt) - with a diagnostic, escalate-only-on-positive-evidence retry (only when a response is empty **and** - `finish_reason == "length"` — the provider-documented signature of "budget too small," not "down") - and short per-attempt timeouts, rather than picking a new fixed number. This ADR deliberately does - not commit to specific budget/timeout constants — those should come from real telemetry the - redesigned preflight itself will emit, not from inspection. +- **Decision**: keep both existing preflight layers, fixed with the two-trigger, explicitly-bounded + retry design above rather than one generic retry or a shortened timeout. - **Live, current evidence this is an active defect, not theoretical**: `noema-review` failed on the - ADR's own PR (#1449, job `99253418179`) with exactly this bug while the ADR was being written — - Layer 1 passed in 30s, Layer 2 then hung the full 120s with zero bytes back. + ADR's own PR (#1449, job `99253418179`) with exactly the Trigger-A (no-response/hang) case — Layer 1 + passed in 30s, Layer 2 then hung the full 120s with zero bytes back, confirming why the two triggers + had to be modeled separately. - Two upstream `contextual-orchestrator` asks are now real tracked issues (`#926`: inference-scoped readiness probe; `#927`: real per-model `max_output_tokens`/`context_window` discovery data, correctly modeled as two separate fields), not just prose. Neither blocks the sidecar-side fix. From 2184ad880e48fe065a3a5b8f4b02d6ba39095da4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 12:17:03 +0000 Subject: [PATCH 04/12] docs(adr): clarify Layer 2 retries reuse the base budget, not an unproven escalation Layer 2 hits the virtual pool, not a specific candidate, so a fresh attempt (possibly landing on a different route via the pool's own routing variance) is the operative lever, not a bigger max_tokens. Avoids introducing an unproven new number; keeps the ADR and its upcoming implementation in sync. Co-Authored-By: Claude --- docs/adr/0005-sidecar-preflight-token-budget.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index 33cd6a06ad..348575a4b2 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -158,8 +158,11 @@ triggers, both bounded, both explicit: differently on a new attempt — this is not a budget problem, so escalating the budget would not help and is not done here. - **Trigger B — a response was received, content is empty, and `choices[0].finish_reason == "length"`** - (the OpenAI-documented signature of "budget too small," cited above): retry the *same* candidate/route - once at a **materially larger** budget. This is the only trigger that changes the budget. + (the OpenAI-documented signature of "budget too small," cited above): for Layer 1, which targets one + specific candidate, retry that *same* candidate once at a **materially larger** budget — the only + case that changes the budget. For Layer 2, which cannot target a specific candidate (see Decision + §3), Trigger B instead retries a fresh request at the same budget, same as Trigger A — a different + route via the virtual pool's own variance is the operative lever there, not a bigger number. - **Neither trigger fires more than once per attempt distinguishing between them, and both draw from one small, shared, explicit retry budget per layer** (Decision §3) — not "one retry per route" unconditionally, which is what produced Devin's second finding (an unbounded-looking worst case). @@ -211,9 +214,13 @@ retried once, unconditionally, would be a real, computed worst-case blowup again completed by the time Layer 2 runs): keep the existing per-attempt timeout (**120s, unchanged** — not shortened, per Context above) and the existing `4096` base budget (already proven working on a real hosted run, `contextual-orchestrator#921`). Allow up to `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3` - total attempts, covering both Trigger A (transport failure/hang) and Trigger B (empty + - `finish_reason == "length"`, escalating to a larger budget on the *next* attempt only) before failing - closed with the specific last-observed reason. **Worst case**: 3 × 120s = **360s (6 minutes)** — + total attempts. Unlike Layer 1, Layer 2 cannot target a specific candidate — a fresh attempt may land + on a different route via the virtual pool's own internal variance, which is the operative lever here, + not a bigger budget — so both Trigger A (transport failure/hang) and Trigger B (empty + + `finish_reason == "length"`) retry a fresh request at the **same** `4096` budget rather than + escalating to an unproven new number; a non-2xx rejection specifically on a *retry* (not the first + attempt) is recorded as its own distinct outcome rather than retried again. **Worst case**: 3 × 120s + = **360s (6 minutes)** — explicit, bounded, and small relative to the job's 120-minute ceiling; the previous design's worst case was already 120s for one unconditional attempt with no chance of recovery, so this trades a bounded amount of additional worst-case latency for surviving exactly the transient-hang class of From 21b995f350c49d53374b4458b50f0330d65abbbd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 12:23:32 +0000 Subject: [PATCH 05/12] docs(adr): fix rounds 3-4 Devin findings, converge on Layer 2's design Round 3 (6 findings), verified each against the actual text/source: - Fixed a real self-contradiction: the general Trigger-A description implied a same-candidate retry applied "in either layer," while Layer 1's own budget section said no such retry exists there. Trigger A/B are now defined per-layer from the outset, stated once, referenced everywhere else. - Fixed a real attribution problem (Layer 2's escalation retried the virtual pool, not a pinned candidate, so a rejection there could not be honestly blamed on one candidate's ceiling). - Reconciled a real 16-vs-4096 inconsistency: Layer 1's base probe budget explicitly changes from 4096 (today) to a new 16; Layer 1's escalated tier is 4096 (reusing REVIEW_MAX_OUTPUT_TOKENS); Layer 2 stays at 4096 throughout. - Fixed present-tense "now emit" telemetry claims (this is a docs-only PR; the implementation must add that telemetry, not already have it). - Corrected Consequences from present tense ("becomes tolerant") to prospective ("would become"), matching the ADR's `proposed` status. - PR #1449's own description will be updated separately to match. Round 4 (1 critical finding, verified directly): a `finish_reason == "length"` response is still HTTP 200, so the gateway's own routing already recorded that attempt as successful before the sidecar inspects content -- a same-budget retry is more likely to repeat the same candidate than diversify away from it, making Layer 2's Trigger-B retry pointless as designed. This is the fourth reshaped version of "does the retry actually reach a different outcome" across this ADR's review. Checked directly (not assumed) whether contextual-orchestrator exposes any way to exclude/deprioritize a specific candidate on a retry -- grepped server.py's request handling and found none. Per the org's convergence rule: Layer 2 no longer retries on finish_reason == "length" at all, only on transport failure/hang, and Layer 2's route diversity is now stated as an unverified best effort, not a guarantee -- accepted as a known, documented, bounded limitation rather than continuing to iterate toward a fully "solved" design. Layer 1 is unaffected (it pins one specific candidate per attempt, so its own escalation retry remains genuinely attributable). Co-Authored-By: Claude --- CHANGELOG.md | 19 +- .../0005-sidecar-preflight-token-budget.md | 174 +++++++++++------- docs/product-technical-gap-baseline.md | 25 ++- 3 files changed, 150 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index beacb6eccf..be88a76d9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,8 +29,23 @@ Semantic Versioning where the repository publishes a release. existing 180s ceiling via a computed, capped escalation budget. Adds two real tracked upstream issues (`ContextualWisdomLab/contextual-orchestrator#926`, `#927`) and SHA-pinned permalink citations (`8b3235d2...`) in place of both - prose-only follow-ups and line numbers that would otherwise rot. No code - change in this PR; the sidecar migration is tracked separately. + prose-only follow-ups and line numbers that would otherwise rot. A third + Devin Review pass found the revised text still self-contradicted which + layer retries on which trigger, plus an attribution problem: Layer 2's + escalation retried the virtual pool, not a pinned candidate, so a + rejection there could not be honestly blamed on one candidate's ceiling. + A fourth pass found a sharper version of the same question -- a + `finish_reason == "length"` response is still HTTP 200, so the gateway's + routing already recorded that attempt as successful, making a same-budget + retry more likely to repeat the same candidate than diversify away from + it. Per this org's convergence rule, and after directly checking + `contextual_orchestrator/server.py` for a candidate-exclusion parameter + and finding none: Layer 2 no longer retries on `finish_reason == "length"` + at all, only on transport failure/hang, and its route diversity is stated + as an unverified best effort rather than a guarantee. Layer 1 (which pins + one specific candidate per attempt) is unaffected. Consequences corrected + from present tense to prospective, matching the ADR's `proposed` status. + No code change in this PR; the sidecar migration is tracked separately. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index 348575a4b2..3a8bec335d 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -143,43 +143,82 @@ limited by the context remaining after input tokens."* Only the second field can ## Decision -### 1. Two distinct, explicitly-bounded retry mechanisms, not one generic "retry" +### 1. Two distinct, explicitly-bounded retry mechanisms, not one generic "retry" — and not the same behavior in both layers Devin Review correctly found that a single "retry on empty content + `finish_reason == 'length'`" predicate cannot fix the actual live outage this ADR is responding to: the reproduced failure (job `99253418179`, cited in the Evidence trail) is a **120-second timeout with zero bytes received** — there is no response object at all in that case, so there is no `finish_reason` to inspect, and the original design's retry path would never trigger for it. Fixed by splitting into two independent -triggers, both bounded, both explicit: +triggers. **Layer 1 and Layer 2 use these triggers differently, by structural necessity, not by +inconsistency — the difference is stated once here and referenced everywhere else, rather than +implied and then contradicted section to section (a real self-contradiction Devin Review's third pass +correctly caught in an earlier revision of this text):** - **Trigger A — no usable response** (transport timeout, connection failure, or non-2xx status on the - *first* attempt at a given budget): retry with a **fresh attempt at the same budget**, on the theory - that the virtual pool's internal routing (Layer 2) or a flaky provider (either layer) may behave - differently on a new attempt — this is not a budget problem, so escalating the budget would not help - and is not done here. + *first* attempt at a given budget). + - **Layer 2**: retry with a fresh attempt at the same `4096` budget, up to the shared attempt cap + (Decision §3). Layer 2 has exactly one check — there is no other candidate to fall back to — so a + hang there must be survived by retrying, or the reproduced outage is not actually fixed. **This + retry is justified even without any guarantee of hitting a different underlying candidate** — see + the route-diversity note below — because it is bounded and strictly better than the current + design's single unconditional attempt with no recovery path at all: worst case, the outcome is + identical and the check still fails closed with the same accurate diagnosis; best case, a + transient failure (a network blip, a momentarily overloaded connection) clears on retry. + - **Layer 1**: **no retry**. Layer 1 already probes up to 12 distinct candidates + (`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`); one candidate's timeout simply consumes its existing 10s + slot and the loop moves to the next candidate, exactly as it does today. A same-candidate retry + here would add latency without adding resilience Layer 1's own multi-candidate design does not + already provide. - **Trigger B — a response was received, content is empty, and `choices[0].finish_reason == "length"`** - (the OpenAI-documented signature of "budget too small," cited above): for Layer 1, which targets one - specific candidate, retry that *same* candidate once at a **materially larger** budget — the only - case that changes the budget. For Layer 2, which cannot target a specific candidate (see Decision - §3), Trigger B instead retries a fresh request at the same budget, same as Trigger A — a different - route via the virtual pool's own variance is the operative lever there, not a bigger number. -- **Neither trigger fires more than once per attempt distinguishing between them, and both draw from - one small, shared, explicit retry budget per layer** (Decision §3) — not "one retry per route" - unconditionally, which is what produced Devin's second finding (an unbounded-looking worst case). -- **A non-2xx rejection specifically on a Trigger-B escalated attempt is not itself retried further.** - If a candidate's *first* attempt at the base budget succeeds or fails openly (Trigger A), that is - handled as above. If it instead comes back empty with `finish_reason == "length"` and the escalated - retry (Trigger B) is then rejected outright (non-2xx) rather than merely still empty, that is - distinguishable evidence the *escalated* budget — not the base one — exceeds this specific model's - real ceiling (Devin Review's second finding). Retrying again with an even larger budget would not be - justified by any evidence in hand and risks the same rejection; instead this is recorded as its own - distinct outcome (e.g. `escalated_probe_rejected`) and the candidate/route is treated as not-ready - for this run. This is an honest, bounded limitation, not a silent misclassification — the complete - fix (knowing each model's real ceiling in advance) is `ContextualWisdomLab/contextual-orchestrator#927`, + (the OpenAI-documented signature of "budget too small," cited above). + - **Layer 1**: retry that *same* candidate (`client.proxy_send_once(agent, ...)` pins the exact agent + object, so this retry is genuinely attributable to that one candidate) once at a **materially + larger** budget — `REVIEW_PREFLIGHT_ESCALATED_TOKENS` (`4096`, reusing `REVIEW_MAX_OUTPUT_TOKENS`), + up from a `16`-token base probe (`REVIEW_PREFLIGHT_BASE_TOKENS` — a **new, smaller** value than the + `4096` Layer 1 uses today; see Decision §3). This is the only place in either layer where the + budget itself changes. + - **Layer 2**: **no retry — this is a deliberate simplification made across this ADR's review, not an + oversight.** Devin Review's fourth pass found the reason directly: a `finish_reason == "length"` + response is still `HTTP 200` — the gateway's own routing layer already recorded that as a + *successful* attempt before the sidecar ever inspects the content, so a subsequent identical + request is not a fresh, independent draw against the pool; the gateway's routing is more likely to + *repeat* the same "successful" candidate than to diversify away from it. Retrying at the same + budget against the same likely candidate has no principled reason to produce a different outcome, + so Layer 2 does not attempt it: an empty response with `finish_reason == "length"` at Layer 2 is + recorded as not-ready immediately, with that `finish_reason` preserved in the report for diagnosis. + +**Route diversity on Layer 2's Trigger-A retry is a best-effort hope, not a verified guarantee, and +this ADR stops trying to force it.** This is the fourth time a version of "does the retry actually +reach a different or better outcome" has come back reshaped across Devin Review's passes on this ADR +(round 2: a too-small budget; round 3: an escalated retry that could hit an unaccountable different +candidate; round 4: the specific case above). Checked directly rather than assumed before accepting +this as final: `contextual_orchestrator/server.py`'s request handling exposes no field to exclude, +deprioritize, or pin away from a specific candidate on a subsequent call — grepped for any such +parameter and found none. Given no verified mechanism to force diversity exists, and per this org's +convention to converge on an honestly-scoped decision rather than iterate indefinitely toward a fully +"solved" design, this ADR's final position is: **Layer 1's genuine N-of-M across truly distinct, +individually-addressed candidates is what does the real resilience and diversity work in this design. +Layer 2 remains what it always was — a single end-to-end smoke test proving the virtual-pool dispatch +path itself works at all — and its bounded retry (Trigger A only) is a modest, honest safety margin +against transient failures, not a pool-exploration mechanism.** If the gateway later exposes a real way +to exclude a specific candidate, that would improve Layer 2's retry meaningfully and should be +revisited then (a natural extension of `ContextualWisdomLab/contextual-orchestrator#926`); this ADR +does not invent that mechanism speculatively. +- **Both triggers draw from one small, shared, explicit retry budget per layer** (Decision §3), not + "one retry per route" unconditionally. +- **A non-2xx rejection on a Layer 1 escalated (Trigger-B) retry** is distinguishable evidence the + *escalated* budget specifically — not the base one — exceeds that one candidate's real ceiling + (genuinely attributable, since the candidate is pinned). Recorded as its own outcome, + `escalated_probe_rejected`, and that candidate is not retried further this run. The complete fix + (knowing each model's real ceiling in advance) is `ContextualWisdomLab/contextual-orchestrator#927`, not this ADR. +- **A non-2xx rejection on a Layer 2 Trigger-A retry** is recorded as `gateway_retry_rejected` — + deliberately **not** named or described as candidate-ceiling evidence, because Layer 2 structurally + cannot confirm which candidate served the rejected attempt. - **Every other outcome is not retried**: a non-2xx or empty-with-a-different-`finish_reason` result on - an attempt that is not eligible for Trigger A or B (i.e., already a retry, or already past the shared - budget) is recorded as not-ready immediately. + an attempt that is not eligible for Trigger A or B for that layer (i.e., already the layer's one + retry, or already past its shared budget) is recorded as not-ready immediately. ### 2. Keep both existing layers — neither replaces the other @@ -199,28 +238,26 @@ retried once, unconditionally, would be a real, computed worst-case blowup again "one retry per route": - **Layer 1** (bounded by the existing 180s healthz-readiness wait, unchanged): keep the existing - per-attempt timeout (`REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10`, unchanged) and the existing base probe - budget. Trigger A does not need its own retry allowance here — Layer 1 already has up to 12 distinct - candidates providing exactly the resilience a same-candidate retry-on-hang would give, so one - candidate's timeout simply consumes its 10s slot and the loop moves to the next candidate, as today. - Only Trigger B (escalate budget on `finish_reason == "length"`) is new here, and it is capped by a - new shared counter, `REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4`, across the whole Layer 1 run (not - per-candidate) — once 4 candidates have consumed an escalation attempt, any further candidate that - would otherwise qualify for Trigger B is instead recorded not-ready immediately with an explicit + per-attempt timeout (`REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10`, unchanged). The **base probe budget + changes from `4096` (today's value) to a new, smaller `REVIEW_PREFLIGHT_BASE_TOKENS = 16`** — cheap + by design, because the escalation path below corrects for it being wrong, unlike today where a wrong + first (and only) guess is fatal. Trigger A does not need its own retry allowance here (see Decision + §1). Trigger B (escalate to `REVIEW_PREFLIGHT_ESCALATED_TOKENS = 4096`, reusing today's + `REVIEW_MAX_OUTPUT_TOKENS`, on `finish_reason == "length"`) is capped by a new shared counter, + `REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4`, across the whole Layer 1 run (not per-candidate) — once 4 + candidates have consumed an escalation attempt, any further candidate that would otherwise qualify + for Trigger B is instead recorded not-ready immediately with an explicit `escalation_budget_exhausted` reason. **Worst case**: 12 × 10s (base attempts) + 4 × 10s (escalation attempts) = **160s**, under the existing 180s ceiling with real margin, computed rather than assumed. - **Layer 2** (bounded only by the job's own 120-minute ceiling, per the org's stated "accuracy over speed" policy already reasoned in this file — *not* by the 180s Layer 1 budget, which has already completed by the time Layer 2 runs): keep the existing per-attempt timeout (**120s, unchanged** — not - shortened, per Context above) and the existing `4096` base budget (already proven working on a real - hosted run, `contextual-orchestrator#921`). Allow up to `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3` - total attempts. Unlike Layer 1, Layer 2 cannot target a specific candidate — a fresh attempt may land - on a different route via the virtual pool's own internal variance, which is the operative lever here, - not a bigger budget — so both Trigger A (transport failure/hang) and Trigger B (empty + - `finish_reason == "length"`) retry a fresh request at the **same** `4096` budget rather than - escalating to an unproven new number; a non-2xx rejection specifically on a *retry* (not the first - attempt) is recorded as its own distinct outcome rather than retried again. **Worst case**: 3 × 120s - = **360s (6 minutes)** — + shortened, per Context above) and the existing **`4096` budget, unchanged throughout — Layer 2 never + escalates** (already proven working on a real hosted run, `contextual-orchestrator#921`; see Decision + §1 for why an escalation tier was considered and dropped here). Allow up to + `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3` total attempts, consumed only by Trigger A (transport + failure/hang/non-2xx) — Trigger B (empty + `finish_reason == "length"`) is not retried at Layer 2 at + all (Decision §1). **Worst case**: 3 × 120s = **360s (6 minutes)** — explicit, bounded, and small relative to the job's 120-minute ceiling; the previous design's worst case was already 120s for one unconditional attempt with no chance of recovery, so this trades a bounded amount of additional worst-case latency for surviving exactly the transient-hang class of @@ -232,9 +269,10 @@ retried once, unconditionally, would be a real, computed worst-case blowup again minimum of 16"* for the deprecated `max_tokens` field). The two new counters (`REVIEW_PREFLIGHT_MAX_ESCALATIONS`, `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS`) are chosen to keep each layer's worst case under its own already-established ceiling, shown above, not picked by inspection - of "what feels right." Both preflight layers now emit `finish_reason`, attempt count, and which - trigger fired in their structured reports (`_preflight_review_agents`'s `routes[]`; the shell script's - `preflight_report`/`gateway` JSON) specifically so that a **follow-up, evidence-driven pass** — after + of "what feels right." The implementation must have both preflight layers emit `finish_reason`, + attempt count, and which trigger fired in their structured reports (`_preflight_review_agents`'s + `routes[]`; the shell script's `preflight_report`/`gateway` JSON) — this ADR does not implement that + itself (see Status) — specifically so that a **follow-up, evidence-driven pass** — after observing real hosted runs with this telemetry — can adjust these two counters and the base/escalated token budgets from real data, which is the methodology this ADR commits to for future tuning: initial values from direct precedent, refinement from telemetry this change itself introduces, never from @@ -257,26 +295,36 @@ retried once, unconditionally, would be a real, computed worst-case blowup again ## Consequences -- Both preflight layers become structurally tolerant of an individual attempt being wrong for a fixed - token budget, or hanging/failing transiently, which is the actual shape of the problem — while - keeping every worst case explicit and bounded rather than open-ended. -- Layer 1's worst case grows from ~120s to a computed 160s, still under its existing 180s - healthz-readiness ceiling. Layer 2's worst case grows from a single 120s attempt with no recovery - path to up to 360s across bounded retries — small relative to the job's 120-minute ceiling and - consistent with this file's own already-stated "accuracy over speed" policy. -- Keeping Layer 2 (not just Layer 1) means the preflight still proves the actual consumer-facing +**This ADR is `proposed`; no code has shipped yet. The consequences below describe what the +implementation is expected to achieve once it lands, verified against this ADR's design — not an +outcome already observed in production.** + +- Once implemented, both preflight layers would become structurally tolerant of an individual attempt + being wrong for a fixed token budget, or hanging/failing transiently, which is the actual shape of + the problem — while keeping every worst case explicit and bounded rather than open-ended. +- Layer 1's worst case would grow from ~120s to a computed 160s, still under its existing 180s + healthz-readiness ceiling. Layer 2's worst case would grow from a single 120s attempt with no + recovery path to up to 360s across bounded retries — small relative to the job's 120-minute ceiling + and consistent with this file's own already-stated "accuracy over speed" policy. +- Keeping Layer 2 (not just Layer 1) would mean the preflight still proves the actual consumer-facing `orchestrator/free` route works, not only that individual candidates can respond in isolation — closing the PR #1433 gap class rather than reopening it. Giving Layer 2 a bounded retry (rather than - either a single unconditional attempt or a shortened timeout) is what actually fixes the live - 120s-hang reproduction on this ADR's own PR — a shortened timeout alone would not have, and would - have regressed the prior, already-evidenced 30s→120s fix in the same file. -- A candidate whose escalated probe is rejected outright (rather than merely still empty) is recorded - as not-ready with a distinct, honest reason rather than silently retried indefinitely or + either a single unconditional attempt or a shortened timeout) is what would actually address the live + 120s-hang reproduction on this ADR's own PR (job `99253418179`) — a shortened timeout alone would not + have, and would have regressed the prior, already-evidenced 30s→120s fix in the same file. Whether it + would have *prevented* that exact reproduction is not claimed with certainty (Layer 2's retry has no + verified route-diversity guarantee — see Decision §1); what it would change is that the check no + longer fails after one unconditional attempt with zero chance of recovery. +- A Layer 1 candidate whose escalated probe is rejected outright (rather than merely still empty) would + be recorded as not-ready with a distinct, honest reason rather than silently retried indefinitely or misclassified — a known, accepted, documented residual limitation until - `ContextualWisdomLab/contextual-orchestrator#927` lands. -- Items in Decision §4 are real `contextual-orchestrator` feature work, now tracked as real issues, - and are explicitly not closed by this ADR. -- No production routing default changes; this is scoped to the sidecar's own liveness checks. + `ContextualWisdomLab/contextual-orchestrator#927` lands. Layer 2's retry-diversity limitation + (Decision §1) is accepted the same way, for the same reason: no verified mechanism exists today to + do better. +- Items in Decision §4 are real `contextual-orchestrator` feature work, now tracked as real issues, and + would remain explicitly not closed by this ADR even once the sidecar-side implementation lands. +- No production routing default changes are proposed; this is scoped to the sidecar's own liveness + checks. - **This is currently active, not theoretical**: the live reproduction in the Evidence trail below is from `noema-review` failing on this ADR's own PR while this ADR was being written, presently blocking that required check org-wide on every repo that routes through this sidecar. The diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 01442f1a35..2d547712ac 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1342,9 +1342,28 @@ that long and the job already budgets 120 minutes total) and gets up to 3 total case) instead of one unconditional attempt with no recovery path. Initial numeric values (`16`, `4096`, `10s`, `120s`, and the two new attempt-count caps) are each either already deployed in this codebase or backed by direct external documentation (OpenRouter's own schema: *"some providers enforce a minimum of -16"*), not fresh guesses — both preflight layers now also emit `finish_reason`/attempt-count/trigger -telemetry specifically so a future pass can refine these from real data. Source citations are now -SHA-pinned permalinks (`8b3235d2...`) instead of bare line numbers. +16"*), not fresh guesses — the implementation must have both preflight layers emit +`finish_reason`/attempt-count/trigger telemetry specifically so a future pass can refine these from +real data. Source citations are now SHA-pinned permalinks (`8b3235d2...`) instead of bare line numbers. + +**A third Devin Review pass found the previous fix still self-contradicted** (the general Trigger-A +description implied a same-candidate retry "in either layer," while Layer 1's own budget section said +no such retry exists there) **and an unaddressed attribution problem**: Layer 2's Trigger-B escalation +retries the *virtual pool*, not a pinned candidate, so a rejection on that retry could not honestly be +blamed on "that candidate's ceiling" — it might be a different candidate entirely. **A fourth pass then +found a sharper version of the same underlying question**: a `finish_reason == "length"` response is +still `HTTP 200`, so the gateway's own routing already recorded that attempt as *successful* before the +sidecar inspects content — a same-budget retry is *more* likely to repeat the same candidate than +diversify away from it, making Layer 2's Trigger-B retry pointless as designed. Per this org's +convergence rule (stop iterating toward a fully "solved" design once no further verified mechanism +exists), and after directly checking `contextual_orchestrator/server.py` for any candidate-exclusion +parameter and finding none: **Layer 2 no longer retries on Trigger B at all** — only Trigger A +(transport failure/hang) is retried there, justified as a bounded safety margin against transient +failure rather than a claim of route diversity, which this ADR now states plainly is unverified and not +guaranteed. Layer 1 is unaffected (it pins one specific candidate object per attempt, so its own +escalation retry is genuinely attributable and untouched by this limitation). The Consequences section +was also corrected from present-tense ("becomes tolerant," "closes the gap") to prospective +("would become," "would close") since this ADR's status remains `proposed` with no code shipped yet. Summary of the current ADR: From 47a8079667626b983a11261deacada44f48da970 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 12:38:36 +0000 Subject: [PATCH 06/12] fix(sidecar): implement ADR-0005's diagnostic, bounded-retry preflight Replaces the single fixed max_tokens on both preflight layers with the design in docs/adr/0005-sidecar-preflight-token-budget.md, converged across 5 rounds of Devin Review scrutiny on that ADR (a 5th pass on the ADR itself, verified directly against current orchestrator.py before fixing, found the escalation predicate as originally scoped would have missed the exact original PR #1436 failure mode -- see below). Layer 1 (scripts/ci/contextual_orchestrator_review_launcher.py, _preflight_review_agents): each candidate now gets one cheap base probe at a new REVIEW_PREFLIGHT_BASE_TOKENS=16 (this codebase's own pre-#1436 value, and independently the floor OpenRouter's schema documents: "some providers enforce a minimum of 16"). That same candidate is retried once at REVIEW_PREFLIGHT_ESCALATED_TOKENS (REVIEW_MAX_OUTPUT_TOKENS, 4096, already proven working on a real hosted run) only when the response is empty AND either finish_reason == "length" (OpenAI's documented "budget too small" signature) OR a populated message.reasoning field is present with no content -- the vendored ModelClient._response_content's own, broader detection logic for this exact failure, which finish_reason alone does not cover. Bounded by a shared REVIEW_PREFLIGHT_MAX_ESCALATIONS=4 counter across the whole run (not per candidate), keeping worst case at a computed 160s, under the existing 180s healthz-readiness ceiling. A non-2xx rejection specifically on the escalated attempt is recorded as EscalatedProbeRejected -- genuinely attributable, since the candidate object is pinned throughout -- and not retried further. Layer 2 (scripts/ci/contextual_orchestrator_review_sidecar.sh): the virtual-pool smoke request keeps its existing, already-evidenced 4096/120s budget unchanged (shortening it would regress this file's own prior 30s->120s fix) and now retries only on transport failure/non-2xx (Trigger A), up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS=3 -- not on empty content with a budget-too-small signature (Trigger B), since that response is still HTTP 200 and the gateway's own routing already recorded it as successful before this script inspects content, so a same-budget retry is more likely to repeat the same candidate than diversify away from it (verified: contextual-orchestrator's server.py exposes no parameter to exclude a specific candidate on a retry). A rejection on a retry is labeled gateway_retry_rejected rather than implying candidate-ceiling attribution Layer 2 cannot support. Both layers emit finish_reason/attempts/trigger telemetry in their reports so future tuning can be evidence-driven. Tests: 1901 passed (was 1900), 100% coverage and 100% docstring coverage on scripts/ci/ unchanged. Co-Authored-By: Claude --- CHANGELOG.md | 20 ++ docs/product-technical-gap-baseline.md | 19 ++ ...contextual_orchestrator_review_launcher.py | 157 +++++++++++++-- .../contextual_orchestrator_review_sidecar.sh | 150 ++++++++++---- ...l_orchestrator_review_runtime_preflight.py | 184 ++++++++++++++++-- 5 files changed, 467 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be88a76d9c..d488c278b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,26 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Implement ADR-0005's diagnostic, bounded-retry sidecar preflight + (`scripts/ci/contextual_orchestrator_review_launcher.py`, + `scripts/ci/contextual_orchestrator_review_sidecar.sh`). A 5th Devin + Review pass on the ADR found the escalation predicate + (`finish_reason == "length"` alone) missed the vendored + `ModelClient._response_content`'s own broader "reasoning without + content" signature -- the exact original PR #1436 failure mode -- + verified directly against current orchestrator.py before fixing. + Layer 1's per-candidate probe now starts at a new + `REVIEW_PREFLIGHT_BASE_TOKENS = 16` and escalates the same candidate + once to the existing `REVIEW_MAX_OUTPUT_TOKENS` (4096) only when the + response is empty and either `finish_reason == "length"` or a + populated `reasoning` field is present, bounded by a shared + `REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4` across the whole run. Layer 2 + keeps its existing 4096/120s budget unchanged and retries only on + transport failure/non-2xx, up to + `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3`, labeling a + retry-specific rejection `gateway_retry_rejected` rather than + implying candidate-ceiling attribution it cannot support. 1901 tests + pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. - Add `docs/adr/0005-sidecar-preflight-token-budget.md`, an evidence-based design decision responding to the owner's direct critique that a single hardcoded `max_tokens` cannot fit a heterogeneous `orchestrator/free` pool. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 2d547712ac..178fd34d4e 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1380,6 +1380,25 @@ Summary of the current ADR: - Two upstream `contextual-orchestrator` asks are now real tracked issues (`#926`: inference-scoped readiness probe; `#927`: real per-model `max_output_tokens`/`context_window` discovery data, correctly modeled as two separate fields), not just prose. Neither blocks the sidecar-side fix. +- **A 5th Devin Review pass found one more real, distinct gap** (detection accuracy, not + retry-diversity): Trigger B's predicate (`finish_reason == "length"`) missed the vendored + `ModelClient._response_content`'s own broader signature for this exact failure — a populated + `message.reasoning` field with empty `content`, which a reasoning model can hit under a *different* + `finish_reason` (provider `finish_reason` semantics for this case are not verified as uniform across + the pool). Verified directly against current `contextual_orchestrator/orchestrator.py` before fixing. + This was the exact original PR #1436 failure mode — a fixed predicate this narrow would have missed + the very case the whole investigation started from. Fixed: Trigger B now escalates on + `finish_reason == "length"` **or** a populated `reasoning` field with no content, matching the + vendored detection logic exactly rather than inferring it from one field alone. +- **Implemented** (`scripts/ci/contextual_orchestrator_review_launcher.py`, + `scripts/ci/contextual_orchestrator_review_sidecar.sh`): Layer 1's `_preflight_review_agents` now + probes each candidate at a new `REVIEW_PREFLIGHT_BASE_TOKENS = 16`, escalating that same candidate + once to `REVIEW_PREFLIGHT_ESCALATED_TOKENS` (`= REVIEW_MAX_OUTPUT_TOKENS`, `4096`) only on the widened + Trigger B signature, bounded by a shared `REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4` across the whole run. + Layer 2 keeps its existing `4096`/`120s` budget unchanged and retries only on Trigger A (transport + failure/non-2xx), up to `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3`, with a retry-specific rejection + labeled `gateway_retry_rejected` rather than implying candidate-ceiling attribution it cannot support. + 1901 tests pass, 100% coverage and 100% docstring coverage on `scripts/ci/`. ## 5. 실행 루프와 고객의 다음 행동 diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 606e694586..6fb0fb678f 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -45,6 +45,28 @@ REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10 REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 12 REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT = 8 +# ADR-0005: a single fixed max_tokens cannot fit every model in a heterogeneous +# pool -- some spend internal reasoning tokens before visible content and need +# more, others have a real completion ceiling a large budget would exceed. The +# base probe is deliberately cheap (16 -- the value this codebase ran with +# before #1436, and independently the floor OpenRouter's own schema documents +# for the deprecated max_tokens field: "some providers enforce a minimum of +# 16"): being wrong is fine here because it is diagnosed and escalated below, +# unlike a single guess that fails outright. +REVIEW_PREFLIGHT_BASE_TOKENS = 16 +# Escalated budget used only when the base probe's response was empty because +# choices[0].finish_reason == "length" (OpenAI's documented signature of +# "budget too small", not "candidate unreachable"). Reuses the existing, +# already-proven-working REVIEW_MAX_OUTPUT_TOKENS rather than inventing a new +# number. +REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS +# Shared cap on how many candidates in one preflight run may use the +# escalation retry above, so Layer 1's worst case stays computed and bounded: +# REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES * REVIEW_PREFLIGHT_TIMEOUT_SECONDS + +# REVIEW_PREFLIGHT_MAX_ESCALATIONS * REVIEW_PREFLIGHT_TIMEOUT_SECONDS +# = 12*10 + 4*10 = 160s, under the sidecar's existing 180s healthz-readiness +# wait. See docs/adr/0005-sidecar-preflight-token-budget.md, Decision section 3. +REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4 class ReviewPreflightError(RuntimeError): @@ -197,15 +219,89 @@ def _safe_http_status(exc: Exception) -> int | None: return None +def _response_finish_reason(response: object) -> str | None: + """Return a bounded ``finish_reason`` string from an OpenAI-compatible response. + + Returns ``None`` when no usable ``finish_reason`` is present. A value is + "unknown" rather than the raw provider string whenever it does not look + like a real, short, stable enum token (real values are e.g. ``stop``, + ``length``, ``tool_calls``, ``content_filter``) -- this evidence must + never become an unbounded copy of arbitrary provider text. + """ + if not isinstance(response, dict): + return None + choices = response.get("choices") + if not isinstance(choices, list) or not choices: + return None + first = choices[0] + if not isinstance(first, dict): + return None + finish_reason = first.get("finish_reason") + if not isinstance(finish_reason, str) or not finish_reason: + return None + if len(finish_reason) > 32 or not all( + character.isalnum() or character == "_" for character in finish_reason + ): + return "unknown" + return finish_reason + + +def _response_has_reasoning_without_content(response: object) -> bool: + """Return whether a response matches the vendored "reasoning, no content" signature. + + Mirrors ``contextual_orchestrator.orchestrator.ModelClient._response_content``'s + own check exactly (a populated ``message.reasoning`` field with no string + ``content``) rather than inferring it from ``finish_reason`` -- a reasoning + model can exhaust its budget mid-reasoning under a ``finish_reason`` other + than ``"length"`` (provider ``finish_reason`` semantics for this case are + not verified as uniform across the pool), so ``finish_reason == "length"`` + alone would miss the exact original failure mode this preflight exists to + diagnose (PR #1436). + """ + if not isinstance(response, dict): + return False + choices = response.get("choices") + if not isinstance(choices, list) or not choices: + return False + first = choices[0] + if not isinstance(first, dict): + return False + message = first.get("message") + return isinstance(message, dict) and bool(message.get("reasoning")) + + def _preflight_review_agents( agents: list[object], *, client: Any ) -> tuple[list[object], dict[str, object]]: """Probe each route with the runtime request contract and keep ready routes. + ADR-0005: a single fixed ``max_tokens`` cannot fit every model in a + heterogeneous pool. Each candidate gets one cheap base-budget probe + (``REVIEW_PREFLIGHT_BASE_TOKENS``); when that specific candidate's + response is empty for a "budget too small" reason -- either + ``choices[0].finish_reason == "length"`` (OpenAI's documented signature), + or the vendored ``ModelClient._response_content``'s own broader signature + (a populated ``message.reasoning`` with no string ``content``, which a + reasoning model can hit under a different ``finish_reason`` -- provider + ``finish_reason`` semantics for this case are not verified as uniform + across the pool, and this is the exact original failure mode PR #1436 + responded to) -- that *same* candidate is retried once at a larger, + escalated budget (``REVIEW_PREFLIGHT_ESCALATED_TOKENS``) before being + marked rejected -- bounded by a shared ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` + counter across the whole run, not per candidate. Every other failure + class (transport exception, non-2xx, or empty content matching neither + signature) is not retried: a genuinely-down candidate never reaches the + escalation path, so it cannot produce a false "healthy" read. + A non-2xx rejection specifically on the escalated attempt is recorded as + ``escalated_probe_rejected`` -- distinguishable evidence the escalated + budget itself exceeds that one candidate's real ceiling, genuinely + attributable since the candidate object is pinned throughout -- and is + not retried further. + The report deliberately records only stable route identity, a bounded - exception class name, and an optional numeric HTTP status. Provider response - bodies, exception messages, URLs, prompts, and credentials are never copied - into evidence. + exception class name, an optional numeric HTTP status, attempt count, and + a bounded ``finish_reason``. Provider response bodies, exception + messages, URLs, prompts, and credentials are never copied into evidence. Args: agents: Selected zero-cost model agents. @@ -219,24 +315,26 @@ def _preflight_review_agents( """ viable: list[object] = [] routes: list[dict[str, object]] = [] + escalations_used = 0 for agent in agents: row: dict[str, object] = { "agent_id": str(getattr(agent, "id", "")), "provider": str(getattr(agent, "provider_name", "") or "unknown"), "model": str(getattr(agent, "model", "")), + "attempts": 1, } - payload: dict[str, object] = { + base_payload: dict[str, object] = { "model": getattr(agent, "model", ""), "messages": [ {"role": "system", "content": "You are a helpful assistant."}, {"role": "user", "content": "Reply with just 'OK'."}, ], "temperature": REVIEW_TEMPERATURE, - "max_tokens": REVIEW_MAX_OUTPUT_TOKENS, + "max_tokens": REVIEW_PREFLIGHT_BASE_TOKENS, "stream": False, } try: - response = client.proxy_send_once(agent, "chat/completions", payload) + response = client.proxy_send_once(agent, "chat/completions", base_payload) except Exception as exc: # noqa: BLE001 - sanitize at the provider boundary row["status"] = "rejected" error_type = type(exc).__name__ @@ -248,20 +346,57 @@ def _preflight_review_agents( row["http_status"] = http_status routes.append(row) continue - if not _chat_response_has_text(response): + if _chat_response_has_text(response): + row["status"] = "ready" + routes.append(row) + viable.append(agent) + continue + finish_reason = _response_finish_reason(response) + row["finish_reason"] = finish_reason or "unknown" + reasoning_without_content = _response_has_reasoning_without_content(response) + row["reasoning_without_content"] = reasoning_without_content + budget_signature = finish_reason == "length" or reasoning_without_content + if not budget_signature or escalations_used >= REVIEW_PREFLIGHT_MAX_ESCALATIONS: row["status"] = "rejected" - row["error_type"] = "InvalidChatResponse" + row["error_type"] = ( + "InvalidChatResponse" if not budget_signature else "EscalationBudgetExhausted" + ) + routes.append(row) + continue + escalations_used += 1 + row["attempts"] = 2 + escalated_payload = dict(base_payload) + escalated_payload["max_tokens"] = REVIEW_PREFLIGHT_ESCALATED_TOKENS + try: + escalated_response = client.proxy_send_once( + agent, "chat/completions", escalated_payload + ) + except Exception as exc: # noqa: BLE001 - a rejection here evidences the escalated budget, not just this candidate, does not fit + row["status"] = "rejected" + row["error_type"] = "EscalatedProbeRejected" + http_status = _safe_http_status(exc) + if http_status is not None: + row["http_status"] = http_status + routes.append(row) + continue + if _chat_response_has_text(escalated_response): + row["status"] = "ready" + row["escalated"] = True routes.append(row) + viable.append(agent) continue - row["status"] = "ready" + row["status"] = "rejected" + row["error_type"] = "InvalidChatResponse" + row["finish_reason"] = _response_finish_reason(escalated_response) or "unknown" routes.append(row) - viable.append(agent) report: dict[str, object] = { - "contract": "strix-plain-chat-preflight-v1", + "contract": "strix-plain-chat-preflight-v2", "probed_count": len(agents), "ready_count": len(viable), "rejected_count": len(agents) - len(viable), + "escalations_used": escalations_used, + "escalation_budget": REVIEW_PREFLIGHT_MAX_ESCALATIONS, "routes": routes, } if not viable: diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index f8b56ef7ab..8cb7a096c7 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -432,15 +432,15 @@ fi # internal error, which is the failure this contract prevents from reaching the # scanner step. gateway_virtual_model="orchestrator/${orchestrator_pool}" -# max_tokens must match REVIEW_MAX_OUTPUT_TOKENS (the launcher's own per-agent -# routing probe budget): observed behavior was an agent the routing probe -# already proved "ready" at that budget failing this separate end-to-end check -# with a spurious 502 invalid_structured_output at a much smaller budget, even -# though the model itself is healthy. The exact field-level cause was never -# captured (the sidecar's log sanitizer strips raw provider payloads by -# design), so treat any specific mechanism as a hypothesis, not fact. See -# "2026-08-30 sidecar preflight max_tokens desynchronized from the routing -# probe" (and its 2026-08-30 correction) in +# max_tokens must match REVIEW_MAX_OUTPUT_TOKENS (the launcher's own escalated +# per-agent routing-probe budget, ADR-0005): observed behavior was an agent the +# routing probe already proved "ready" at that budget failing this separate +# end-to-end check with a spurious 502 invalid_structured_output at a much +# smaller budget, even though the model itself is healthy. The exact +# field-level cause was never captured (the sidecar's log sanitizer strips raw +# provider payloads by design), so treat any specific mechanism as a +# hypothesis, not fact. See "2026-08-30 sidecar preflight max_tokens +# desynchronized from the routing probe" (and its 2026-08-30 correction) in # ContextualWisdomLab/contextual-orchestrator's own # docs/product-technical-gap-baseline.md for the evidence that is actually # captured (downloaded strix-reports artifact, @@ -460,21 +460,55 @@ printf '{"model":"%s","messages":[{"role":"system","content":"You are a helpful # favor of accuracy over speed -- a 30s bound on one preflight self-check # contradicted that policy and rejected a route the routing probe had just # proven healthy. 120s keeps this a bounded, fail-closed check while giving -# a real reasoning generation room to finish. -if ! gateway_http_status="$( - curl -sS --max-time 120 \ - -o "$gateway_preflight_response" \ - -w '%{http_code}' \ - -X POST \ - -H "Authorization: Bearer ${ORCHESTRATOR_TOKEN}" \ - -H 'Content-Type: application/json' \ - --data-binary "@$gateway_preflight_request" \ - "http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/v1/chat/completions" -)"; then - fail "gateway preflight request could not reach the local sidecar" -fi -if [ "$gateway_http_status" != "200" ]; then - "$sidecar_python" - "$preflight_report" "$gateway_preflight_response" "$gateway_http_status" <<'PY' +# a real reasoning generation room to finish. This value is deliberately kept +# unchanged by ADR-0005 -- shortening it would regress the fix just described. +# +# ADR-0005 Trigger A: this request goes to the virtual pool, not one pinned +# candidate, so a transport failure or non-2xx status here (unreachable +# process, timeout, upstream error) is retried with a fresh attempt at the +# SAME budget, up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS total attempts -- +# a same-budget retry may or may not land on a different underlying candidate +# (route diversity here is a best-effort hope, not a verified guarantee: the +# gateway's internal routing behavior on a failed attempt is not confirmed), +# but it is strictly better than one unconditional attempt with no recovery +# path, which is what let a single transient hang block every required review +# org-wide (live reproduction: ContextualWisdomLab/.github#1449, job +# 99253418179, curl timing out at exactly 120002ms with zero bytes received). +# Trigger B (a response IS received, empty content, finish_reason=="length" or +# a populated reasoning field) is deliberately NOT retried here: that response +# is still HTTP 200, so the gateway's own routing already recorded that +# attempt as "successful" before this script inspects content -- a same-budget +# retry is more likely to repeat the same candidate than diversify away from +# it, so retrying would not help (Devin Review's 4th-round finding on +# ADR-0005; verified directly against contextual-orchestrator's server.py, +# which exposes no parameter to exclude or deprioritize a specific candidate +# on a retry). +REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS="${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS:-3}" +gateway_attempt=1 +gateway_http_status="" +while :; do + if gateway_http_status="$( + curl -sS --max-time 120 \ + -o "$gateway_preflight_response" \ + -w '%{http_code}' \ + -X POST \ + -H "Authorization: Bearer ${ORCHESTRATOR_TOKEN}" \ + -H 'Content-Type: application/json' \ + --data-binary "@$gateway_preflight_request" \ + "http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/v1/chat/completions" + )"; then + : + else + gateway_http_status="" + fi + if [ "$gateway_http_status" = "200" ]; then + break + fi + if [ "$gateway_attempt" -ge "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" ]; then + if [ -z "$gateway_http_status" ]; then + fail "gateway preflight request could not reach the local sidecar after ${gateway_attempt} attempts" + fi + "$sidecar_python" - "$preflight_report" "$gateway_preflight_response" "$gateway_http_status" "$gateway_attempt" <<'PY' import json from pathlib import Path import re @@ -483,6 +517,7 @@ import sys report_path = Path(sys.argv[1]) response_path = Path(sys.argv[2]) status_text = sys.argv[3] +attempts = int(sys.argv[4]) if sys.argv[4].isdecimal() else 0 try: report = json.loads(report_path.read_text(encoding="utf-8")) except (OSError, json.JSONDecodeError): @@ -498,46 +533,83 @@ if not isinstance(code, str) or not re.fullmatch(r"[A-Za-z0-9_.-]{1,64}", code): status = int(status_text) if status_text.isdecimal() else 0 report["gateway"] = { "endpoint": "chat/completions", + # ADR-0005: a non-2xx on a retry (attempts > 1) is not honestly + # attributable to any one candidate's ceiling -- the virtual pool's + # routing is not pinned across separate HTTP calls -- so it is + # recorded distinctly from a first-attempt rejection instead of + # implying candidate-ceiling evidence it cannot support. + "error_type": "gateway_retry_rejected" if attempts > 1 else "gateway_rejected", "error_code": code, "http_status": status, + "attempts": attempts, "status": "rejected", } temporary = report_path.with_suffix(".tmp") temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") temporary.replace(report_path) PY - fail "gateway preflight returned HTTP ${gateway_http_status}" -fi -if ! "$sidecar_python" - "$gateway_preflight_response" "$preflight_report" <<'PY' + fail "gateway preflight returned HTTP ${gateway_http_status} after ${gateway_attempt} attempts" + fi + log "gateway preflight attempt ${gateway_attempt} did not reach the sidecar cleanly (status=${gateway_http_status:-unreachable}); retrying (up to ${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS} attempts)" + gateway_attempt=$((gateway_attempt + 1)) +done +if ! "$sidecar_python" - "$gateway_preflight_response" "$preflight_report" "$gateway_attempt" <<'PY' import json from pathlib import Path import sys response_path = Path(sys.argv[1]) report_path = Path(sys.argv[2]) +attempts = int(sys.argv[3]) if sys.argv[3].isdecimal() else 0 try: response = json.loads(response_path.read_text(encoding="utf-8")) choices = response.get("choices") first = choices[0] if isinstance(choices, list) and choices else None message = first.get("message") if isinstance(first, dict) else None content = message.get("content") if isinstance(message, dict) else None - if not isinstance(content, str) or not content.strip(): - raise ValueError("missing chat content") + if isinstance(content, str) and content.strip(): + report = json.loads(report_path.read_text(encoding="utf-8")) + report["gateway"] = { + "endpoint": "chat/completions", + "status": "ready", + "attempts": attempts, + } + temporary = report_path.with_suffix(".tmp") + temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") + temporary.replace(report_path) + raise SystemExit(0) + # ADR-0005 Trigger B, deliberately not retried at this layer (see the + # comment above the curl loop): record which budget-too-small signature, + # if any, matched -- for diagnosis only, since this response is a + # terminal outcome here regardless of which one it is. + finish_reason = first.get("finish_reason") if isinstance(first, dict) else None + if not isinstance(finish_reason, str) or not finish_reason: + finish_reason = None + elif len(finish_reason) > 32 or not all( + character.isalnum() or character == "_" for character in finish_reason + ): + finish_reason = "unknown" + reasoning_without_content = isinstance(message, dict) and bool(message.get("reasoning")) report = json.loads(report_path.read_text(encoding="utf-8")) -except (OSError, ValueError, json.JSONDecodeError, IndexError, TypeError): - raise SystemExit(1) -report["gateway"] = { - "endpoint": "chat/completions", - "status": "ready", -} -temporary = report_path.with_suffix(".tmp") -temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") -temporary.replace(report_path) + report["gateway"] = { + "endpoint": "chat/completions", + "status": "rejected", + "error_type": "InvalidChatResponse", + "finish_reason": finish_reason or "unknown", + "reasoning_without_content": reasoning_without_content, + "attempts": attempts, + } + temporary = report_path.with_suffix(".tmp") + temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") + temporary.replace(report_path) +except (OSError, json.JSONDecodeError, IndexError, TypeError): + pass +raise SystemExit(1) PY then fail "gateway preflight returned unusable chat content" fi -log "gateway chat/completions preflight confirmed" +log "gateway chat/completions preflight confirmed (attempt ${gateway_attempt}/${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS})" if [ -n "$ORCHESTRATOR_GITHUB_ENV" ]; then { diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 5bb2e938ef..d5e0f41acb 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -38,6 +38,31 @@ def proxy_send_once( return outcome +class _SequencedClient: + """Return one outcome per call, in order, ignoring which agent asked. + + Used for ADR-0005 escalation tests where the same candidate is called + twice (base probe, then escalated retry) and each call must see a + different, explicitly ordered outcome -- unlike ``_ProbeClient``, whose + per-agent dict lookup always returns the same outcome for repeat calls. + """ + + def __init__(self, outcomes: list[object]) -> None: + self._outcomes = iter(outcomes) + self.calls: list[tuple[object, str, dict[str, object]]] = [] + + def proxy_send_once( + self, agent: object, endpoint: str, payload: dict[str, object] + ) -> dict[str, object]: + """Capture one request and return or raise the next configured outcome.""" + self.calls.append((agent, endpoint, payload)) + outcome = next(self._outcomes) + if isinstance(outcome, BaseException): + raise outcome + assert isinstance(outcome, dict) + return outcome + + def _load_launcher() -> dict[str, object]: """Execute the dependency-lazy launcher and return its module namespace.""" return runpy.run_path(str(_LAUNCHER)) @@ -174,7 +199,7 @@ def test_preflight_mirrors_runtime_request_and_keeps_only_compatible_routes() -> assert endpoint == "chat/completions" assert payload["model"] == agent.model assert payload["stream"] is False - assert payload["max_tokens"] == 4096 + assert payload["max_tokens"] == 16 assert payload["temperature"] == 1.0 assert payload["messages"] == [ {"role": "system", "content": "You are a helpful assistant."}, @@ -248,18 +273,49 @@ def test_gateway_preflight_curl_timeout_tolerates_real_reasoning_latency() -> No ) -def test_reasoning_without_content_remains_rejected_even_with_the_full_budget() -> None: - """A genuinely broken model must still fail closed at the full 4096-token - budget -- proving the sidecar-preflight-max-tokens fix widens the budget - without weakening the routing probe's fail-closed content check. +def test_gateway_preflight_retries_transport_failures_up_to_a_bounded_attempt_count() -> None: + """ADR-0005 Decision SS1/SS3: Layer 2 retries only on Trigger A (no usable + response), up to an explicit, bounded attempt count -- not on Trigger B + (empty content with a budget-too-small signature), which the gateway's + own routing may have already recorded as a "successful" attempt. + + Regression for Devin Review's 4th-round finding on this ADR (a live + reproduction on ContextualWisdomLab/.github#1449, job 99253418179, + hung the full 120s with zero bytes -- Trigger A -- and the pre-fix + script had no recovery path at all). + """ + sidecar = _SIDECAR.read_text(encoding="utf-8") + + assert 'REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS="${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS:-3}"' in sidecar + assert "gateway_attempt=1" in sidecar + assert 'if [ "$gateway_http_status" = "200" ]; then' in sidecar + assert 'if [ "$gateway_attempt" -ge "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" ]; then' in sidecar + assert "gateway_attempt=$((gateway_attempt + 1))" in sidecar + # Trigger A retries are distinguishable from a first-attempt rejection -- + # the virtual pool's routing is not pinned across separate HTTP calls, so + # a rejection on a retry is never described as candidate-ceiling evidence. + assert '"gateway_retry_rejected" if attempts > 1 else "gateway_rejected"' in sidecar + # Trigger B (a response was received) is a terminal outcome here, not + # retried, with its budget-too-small signature preserved for diagnosis. + assert "reasoning_without_content" in sidecar + assert "gateway preflight returned unusable chat content" in sidecar + + +def test_reasoning_without_content_escalates_then_still_fails_closed_if_unresolved() -> None: + """ADR-0005 round 5 (Devin Review): escalation must key off the vendored + ``ModelClient._response_content``'s own "reasoning, no content" signature, + not only ``finish_reason == "length"`` -- a reasoning model can exhaust its + budget under a different (or absent) ``finish_reason``, and this is the + exact original failure mode PR #1436 responded to. This response has no + ``finish_reason`` at all, so it would NOT have escalated under the + finish_reason-only predicate; it must escalate here because + ``message.reasoning`` is populated with empty ``content``. Negative control for the same incident: raising the budget must never be - mistaken for making every response acceptable. A route whose reply is - reasoning-only (present ``reasoning``, empty ``content``) -- the exact - shape ``contextual_orchestrator.orchestrator._response_content`` raises - ``ProviderResponseError`` for -- is simulated at the routing-probe layer - and must still be classified "rejected", never reclassified as a - healthy "ready" route just because the token budget grew. + mistaken for making every response acceptable. The escalated attempt + reproduces the identical reasoning-only shape here, so the route must + still end up "rejected", never reclassified as healthy just because an + escalation was attempted. """ namespace = _load_launcher() preflight = namespace["_preflight_review_agents"] @@ -277,10 +333,112 @@ def test_reasoning_without_content_remains_rejected_even_with_the_full_budget() } ) - with pytest.raises(namespace["ReviewPreflightError"], match="no provider route passed"): + with pytest.raises(namespace["ReviewPreflightError"], match="no provider route passed") as failure: preflight([reasoning_only], client=client) - assert client.calls[0][2]["max_tokens"] == namespace["REVIEW_MAX_OUTPUT_TOKENS"] + assert [call[2]["max_tokens"] for call in client.calls] == [ + namespace["REVIEW_PREFLIGHT_BASE_TOKENS"], + namespace["REVIEW_PREFLIGHT_ESCALATED_TOKENS"], + ] + row = failure.value.report["routes"][0] + assert row["attempts"] == 2 + assert row["reasoning_without_content"] is True + assert row["finish_reason"] == "unknown" + assert failure.value.report["escalations_used"] == 1 + + +def test_finish_reason_length_escalates_and_can_succeed() -> None: + """The OpenAI-documented ``finish_reason == "length"`` signature also + escalates, independent of the ``reasoning`` field, and a candidate that + only needed a bigger budget is correctly marked ready on the retry. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + slow_starter = SimpleNamespace( + id="openrouter_slow_starter", provider_name="openrouter", model="slow/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + _openai_text("OK, here is the answer."), + ] + ) + + viable, report = preflight([slow_starter], client=client) + + assert viable == [slow_starter] + assert [call[2]["max_tokens"] for call in client.calls] == [ + namespace["REVIEW_PREFLIGHT_BASE_TOKENS"], + namespace["REVIEW_PREFLIGHT_ESCALATED_TOKENS"], + ] + row = report["routes"][0] + assert row["status"] == "ready" + assert row["attempts"] == 2 + assert row["escalated"] is True + assert row["finish_reason"] == "length" + assert row["reasoning_without_content"] is False + assert report["escalations_used"] == 1 + + +def test_escalation_budget_is_shared_and_bounded_across_candidates() -> None: + """Once ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` is spent, a further candidate + that would otherwise qualify is rejected immediately, without a second + call -- the shared budget is per-run, not per-candidate. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + max_escalations = namespace["REVIEW_PREFLIGHT_MAX_ESCALATIONS"] + + length_response = {"choices": [{"finish_reason": "length", "message": {"content": ""}}]} + agents = [ + SimpleNamespace(id=f"budget_user_{index}", provider_name="openrouter", model="x/free") + for index in range(max_escalations) + ] + exhausted = SimpleNamespace( + id="budget_exhausted", provider_name="openrouter", model="x/free" + ) + client = _ProbeClient( + {agent.id: dict(length_response) for agent in agents} + | {exhausted.id: dict(length_response)} + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([*agents, exhausted], client=client) + + exhausted_row = failure.value.report["routes"][-1] + assert exhausted_row["attempts"] == 1 + assert exhausted_row["error_type"] == "EscalationBudgetExhausted" + assert failure.value.report["escalations_used"] == max_escalations + assert len(client.calls) == max_escalations * 2 + 1 + + +def test_escalated_probe_rejection_is_recorded_distinctly_and_not_retried() -> None: + """A non-2xx rejection specifically on the escalated attempt is + distinguishable evidence the escalated budget exceeds that candidate's + real ceiling -- recorded as ``EscalatedProbeRejected``, not conflated + with a generic empty-content rejection, and not retried again. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + low_ceiling = SimpleNamespace( + id="nvidia_nim_low_ceiling", provider_name="nvidia_nim", model="low/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + RuntimeError("max_tokens exceeds this model's ceiling"), + ] + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([low_ceiling], client=client) + + assert len(client.calls) == 2 + row = failure.value.report["routes"][0] + assert row["error_type"] == "EscalatedProbeRejected" + assert row["attempts"] == 2 def test_preflight_fails_closed_when_every_route_rejects() -> None: From c45241ee74977d8bcd69402515e8d2343b5c5ea8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 13:01:17 +0000 Subject: [PATCH 07/12] fix(sidecar): resolve 7 Devin Review findings on PR #1452's preflight Two blocking: (1) the Layer 1 escalation counter reset per call, so _preflight_with_fallback's primary+fallback stages could each spend the full REVIEW_PREFLIGHT_MAX_ESCALATIONS budget, pushing worst case to 200s past the 180s healthz-readiness watchdog -- fixed by threading the running escalations_used across both stages, with a regression test proving 8 rejected primary + 4 fallback routes still stay at 160s worst case. (2) a malformed REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS made the shell script's integer comparison silently fail, removing the retry bound -- fixed with an explicit case guard before the loop. Five more: an escalated-attempt transport failure (no HTTP status) was mislabeled EscalatedProbeRejected, falsely blaming the token budget for a connectivity failure -- now uses the existing _safe_http_status helper to distinguish the two. Layer 2 exhausting every attempt with no response wrote no gateway evidence before failing -- now records a bounded gateway_transport_exhausted classification first, via the same sanitize-and-atomic-replace pattern the other gateway paths use. Layer 1's error-type strings were CamelCase while the ADR text and Layer 2 already used snake_case -- Layer 1 (and Layer 2's one CamelCase outlier) now match. The Layer 2 gateway retry-loop test only asserted source literals -- added a fake-curl harness that extracts and executes the tracked script's real retry loop against a scripted, no-network curl stand-in, covering success, transport-failure recovery, non-2xx exhaustion, transport exhaustion, and the malformed-limit guard. A mixed-attempt telemetry bug left reasoning_without_content describing the base attempt while finish_reason had moved on to describe the escalated one -- both fields now always describe the same attempt. 1913 tests pass (1901 baseline + 12 new), 100% coverage and 100% docstring coverage on scripts/ci/, bash -n and all 4 embedded Python heredocs in the sidecar script parse cleanly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 40 ++ docs/product-technical-gap-baseline.md | 54 +++ ...contextual_orchestrator_review_launcher.py | 82 +++- .../contextual_orchestrator_review_sidecar.sh | 37 +- ...l_orchestrator_review_runtime_preflight.py | 434 +++++++++++++++++- 5 files changed, 626 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d488c278b7..777a1cec27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,46 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix 7 Devin Review findings on PR #1452, ADR-0005's implementation + (`scripts/ci/contextual_orchestrator_review_launcher.py`, + `scripts/ci/contextual_orchestrator_review_sidecar.sh`, + `tests/test_contextual_orchestrator_review_runtime_preflight.py`). Two were + blocking: (1) `_preflight_review_agents` reset its escalation counter fresh + on every call, so `_preflight_with_fallback` calling it twice (primary, + then fallback) could spend the full `REVIEW_PREFLIGHT_MAX_ESCALATIONS` + budget in each stage -- up to 200s, past Layer 1's 180s + healthz-readiness watchdog and contradicting the ADR's own claimed 160s + worst case. Fixed by threading the primary stage's ending + `escalations_used` into the fallback stage as its starting point, so one + shared budget covers the whole run; both stages' counts remain visible in + the returned evidence. (2) A non-numeric, empty, zero, or negative + `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS` made the shell script's integer + comparison silently fail on every iteration, removing the retry bound + entirely instead of failing closed. Fixed with an explicit `case` guard + before the retry loop starts. The remaining five: an escalated-attempt + transport failure (no HTTP status at all) was mislabeled + `EscalatedProbeRejected`, falsely attributing a connectivity failure to + the token budget -- now distinguishes on HTTP-status presence, falling + back to the sanitized exception type otherwise; total transport-attempt + exhaustion at Layer 2 used to `fail` without ever writing gateway evidence + -- now records a bounded `gateway_transport_exhausted` classification + first, via the same sanitize-and-atomic-replace pattern the non-2xx and + invalid-content paths already use; Layer 1's error-type strings were + CamelCase (`EscalatedProbeRejected`, `InvalidChatResponse`, + `EscalationBudgetExhausted`) while the ADR and Layer 2 already used + snake_case -- Layer 1 (and Layer 2's one remaining outlier) now match: + `escalated_probe_rejected`, `invalid_chat_response`, + `escalation_budget_exhausted`, `gateway_transport_exhausted`; the Layer 2 + gateway retry-loop test only asserted source literals rather than + executing the loop -- added a fake-curl harness (extracting the tracked + script's real retry-loop source and running it under `bash` against a + scripted, no-network `curl` stand-in) covering first-attempt success, + transport-failure recovery, non-2xx exhaustion, transport exhaustion, and + the malformed-attempt-limit guard; and a mixed-attempt telemetry bug where + `finish_reason` reflected the escalated attempt while + `reasoning_without_content` was left describing the base attempt -- both + fields now always describe the same (most recent) attempt. 1913 tests + pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. - Implement ADR-0005's diagnostic, bounded-retry sidecar preflight (`scripts/ci/contextual_orchestrator_review_launcher.py`, `scripts/ci/contextual_orchestrator_review_sidecar.sh`). A 5th Devin diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 178fd34d4e..b15d51c0e6 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1400,6 +1400,60 @@ Summary of the current ADR: labeled `gateway_retry_rejected` rather than implying candidate-ceiling attribution it cannot support. 1901 tests pass, 100% coverage and 100% docstring coverage on `scripts/ci/`. +**Devin Review then reviewed the actual implementation PR (#1452) and found 7 real issues, verified +against current code (not taken on characterization alone) and all fixed — two were blocking.** (1) +`_preflight_review_agents` initialized its escalation counter fresh on every call, so +`_preflight_with_fallback` calling it twice (up to 8 primary routes, then up to 4 fallback routes) could +spend the full `REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4` budget in *each* stage — up to 8 escalations total, +200s worst case, exceeding Layer 1's own 180s healthz-readiness watchdog and directly contradicting the +160s worst case computed above. Fixed by threading the primary stage's ending `escalations_used` into the +fallback stage as its starting point, so the whole run shares one budget; a new regression test drives 8 +rejected primary routes and 4 fallback routes through a response that always qualifies for escalation and +asserts total escalations stay at 4 and total attempts at 16 (160s at the existing 10s per-attempt +timeout). (2) A non-numeric, empty, zero, or negative `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS` made the +shell script's `[ "$gateway_attempt" -ge "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" ]` integer comparison +error out (which bash reports as the condition being false, not a fatal error, inside an `if`), so the +retry loop would never detect it had reached the limit and would retry until the surrounding CI job's own +timeout, instead of failing closed on bad configuration — fixed with an explicit `case` guard +(`''|*[!0-9]*|0`) before the loop starts. + +Five more, non-blocking but real: (3) an escalated-attempt exception with no HTTP status at all (a bare +transport failure/timeout) was unconditionally labeled `EscalatedProbeRejected`, falsely attributing a +connectivity failure to the token budget — the existing `_safe_http_status` helper already distinguished +HTTP-status-bearing exceptions from transport failures elsewhere in the file, so the escalated-attempt +handler now uses it the same way, falling back to the sanitized exception type name (or a bounded +placeholder) when no status is present. (4) Layer 2 exhausting every `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS` +attempts with no usable HTTP response ever wrote to the gateway evidence report before calling `fail` and +exiting — the exact failure case telemetry matters most for left zero trace of attempt count or trigger; +fixed by writing a bounded `gateway_transport_exhausted` classification first, via the identical +sanitize-then-atomic-replace pattern the non-2xx and invalid-content paths already used. (5) Layer 1's +error-type strings were CamelCase (`EscalatedProbeRejected`, `InvalidChatResponse`, +`EscalationBudgetExhausted`) while this ADR's own text and Layer 2's shell script already used snake_case +(`escalated_probe_rejected`, `gateway_retry_rejected`, `escalation_budget_exhausted`) for the same +concepts, plus one snake_case/CamelCase outlier inside Layer 2 itself (`InvalidChatResponse`) — the ADR +text was correct, so the code was brought in line with it: +`escalated_probe_rejected`/`invalid_chat_response`/`escalation_budget_exhausted`/`provider_error` +throughout both layers. (6) The Layer 2 gateway retry-loop test only asserted source literals (e.g. that +a given string appeared somewhere in the script) rather than ever executing the retry loop — exactly why +findings (3) and (4) slipped past "100% coverage." Fixed with a fake-curl test harness that extracts the +tracked script's real, current retry-loop source (not a hand-copied duplicate, so a future edit is +automatically exercised) and runs it under `bash` against a scripted, no-network `curl` stand-in on +`$PATH`, covering first-attempt success, transport-failure recovery, non-2xx exhaustion, transport-attempt +exhaustion, and the malformed-attempt-limit guard (without ever letting a malformed-limit case actually +loop unboundedly — the guard is asserted to reject before any curl call happens at all). (7) After an +empty escalated response, `finish_reason` was overwritten to describe the escalated (2nd) attempt while +`reasoning_without_content` was left describing the base (1st) attempt's state — two fields that look +like they describe the same response but silently did not. Fixed so both fields are always updated +together to describe the same, most recent attempt, with a regression test giving the two attempts +deliberately different signatures to prove neither field is left stale. + +**Implemented and verified** (`scripts/ci/contextual_orchestrator_review_launcher.py`, +`scripts/ci/contextual_orchestrator_review_sidecar.sh`, +`tests/test_contextual_orchestrator_review_runtime_preflight.py`): 1913 tests pass (1901 baseline + 12 +new), 100% coverage and 100% docstring coverage on `scripts/ci/`, `bash -n` syntax-checks the shell +script, and all 4 embedded Python heredoc blocks in it (including the new transport-exhaustion evidence +writer) parse cleanly. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 6fb0fb678f..36c67a6d2b 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -271,7 +271,7 @@ def _response_has_reasoning_without_content(response: object) -> bool: def _preflight_review_agents( - agents: list[object], *, client: Any + agents: list[object], *, client: Any, escalations_used: int = 0 ) -> tuple[list[object], dict[str, object]]: """Probe each route with the runtime request contract and keep ready routes. @@ -288,34 +288,52 @@ def _preflight_review_agents( responded to) -- that *same* candidate is retried once at a larger, escalated budget (``REVIEW_PREFLIGHT_ESCALATED_TOKENS``) before being marked rejected -- bounded by a shared ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` - counter across the whole run, not per candidate. Every other failure - class (transport exception, non-2xx, or empty content matching neither - signature) is not retried: a genuinely-down candidate never reaches the - escalation path, so it cannot produce a false "healthy" read. + counter, which the ``escalations_used`` argument carries forward across + calls (not per candidate, and not reset per call): a caller that probes + two stages of the same preflight run (e.g. ``_preflight_with_fallback``'s + primary and fallback stages) must pass the previous stage's ending count + back in here so the two stages share one budget instead of each getting + its own -- otherwise the computed worst-case bound this counter exists to + enforce silently doubles. Every other failure class (transport exception, + non-2xx, or empty content matching neither signature) is not retried: a + genuinely-down candidate never reaches the escalation path, so it cannot + produce a false "healthy" read. A non-2xx rejection specifically on the escalated attempt is recorded as ``escalated_probe_rejected`` -- distinguishable evidence the escalated budget itself exceeds that one candidate's real ceiling, genuinely attributable since the candidate object is pinned throughout -- and is - not retried further. + not retried further. An escalated-attempt exception with no HTTP status + (a transport failure, e.g. a timeout) is never labeled this way: that + would falsely attribute a connectivity failure to the token budget, so it + instead records the sanitized exception type, same as the base probe. The report deliberately records only stable route identity, a bounded exception class name, an optional numeric HTTP status, attempt count, and a bounded ``finish_reason``. Provider response bodies, exception messages, URLs, prompts, and credentials are never copied into evidence. + ``finish_reason`` and ``reasoning_without_content`` always describe the + same, most recent attempt for a route (the base attempt when only one was + made; the escalated attempt when a second was made) -- never a mix of the + two attempts' state. Args: agents: Selected zero-cost model agents. client: Vendored ``ModelClient``-compatible transport. + escalations_used: Escalations already spent earlier in this same + preflight run (e.g. by a prior stage), so the shared budget is + honored across calls rather than restarted at zero. Returns: - A pair of viable agents and a sanitized preflight report. + A pair of viable agents and a sanitized preflight report. The + report's ``escalations_used`` is the running total including + ``escalations_used``'s starting value, so a caller chaining another + stage can pass it straight back in. Raises: ReviewPreflightError: If no provider route returns usable text. """ viable: list[object] = [] routes: list[dict[str, object]] = [] - escalations_used = 0 for agent in agents: row: dict[str, object] = { "agent_id": str(getattr(agent, "id", "")), @@ -339,7 +357,7 @@ class (transport exception, non-2xx, or empty content matching neither row["status"] = "rejected" error_type = type(exc).__name__ row["error_type"] = ( - error_type if error_type.isidentifier() and len(error_type) <= 64 else "ProviderError" + error_type if error_type.isidentifier() and len(error_type) <= 64 else "provider_error" ) http_status = _safe_http_status(exc) if http_status is not None: @@ -359,7 +377,7 @@ class (transport exception, non-2xx, or empty content matching neither if not budget_signature or escalations_used >= REVIEW_PREFLIGHT_MAX_ESCALATIONS: row["status"] = "rejected" row["error_type"] = ( - "InvalidChatResponse" if not budget_signature else "EscalationBudgetExhausted" + "invalid_chat_response" if not budget_signature else "escalation_budget_exhausted" ) routes.append(row) continue @@ -371,12 +389,25 @@ class (transport exception, non-2xx, or empty content matching neither escalated_response = client.proxy_send_once( agent, "chat/completions", escalated_payload ) - except Exception as exc: # noqa: BLE001 - a rejection here evidences the escalated budget, not just this candidate, does not fit + except Exception as exc: # noqa: BLE001 - sanitize at the provider boundary row["status"] = "rejected" - row["error_type"] = "EscalatedProbeRejected" http_status = _safe_http_status(exc) if http_status is not None: + # Distinguishable evidence the *escalated* budget itself + # exceeds this candidate's real ceiling -- genuinely + # attributable, since the candidate object is pinned + # throughout. A transport failure (no HTTP status) is never + # labeled this way; it falls through to the same sanitized + # exception-type recording the base probe uses, below. + row["error_type"] = "escalated_probe_rejected" row["http_status"] = http_status + else: + error_type = type(exc).__name__ + row["error_type"] = ( + error_type + if error_type.isidentifier() and len(error_type) <= 64 + else "provider_error" + ) routes.append(row) continue if _chat_response_has_text(escalated_response): @@ -386,8 +417,13 @@ class (transport exception, non-2xx, or empty content matching neither viable.append(agent) continue row["status"] = "rejected" - row["error_type"] = "InvalidChatResponse" + row["error_type"] = "invalid_chat_response" + # Both fields now describe this escalated (2nd, final) attempt, + # never a mix with the base attempt's state -- see the docstring. row["finish_reason"] = _response_finish_reason(escalated_response) or "unknown" + row["reasoning_without_content"] = _response_has_reasoning_without_content( + escalated_response + ) routes.append(row) report: dict[str, object] = { @@ -409,15 +445,31 @@ class (transport exception, non-2xx, or empty content matching neither def _preflight_with_fallback( primary_agents: list[object], fallback_agents: list[object], *, client: Any ) -> tuple[list[object], dict[str, object], bool]: - """Use the priced catalog only after every primary route rejects.""" + """Use the priced catalog only after every primary route rejects. + + The two stages share ADR-0005's one ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` + budget for the whole preflight run, not one budget each: the primary + stage's ending ``escalations_used`` is passed as the fallback stage's + starting point, so a run that rejects all 8 primary routes and then + probes 4 fallback routes still spends at most 4 escalations total (12 + base attempts + 4 escalations, 160s worst case) instead of up to 8 (200s) + -- which would exceed Layer 1's 180s healthz-readiness wait. Both + stages' reports remain in the result: the fallback (or sole) stage's + report carries the run's final, cumulative ``escalations_used``, and + ``primary_attempt`` nests the primary stage's own report -- including its + own ``escalations_used`` -- whenever a fallback stage ran at all. + """ try: viable, report = _preflight_review_agents(primary_agents, client=client) return viable, report, False except ReviewPreflightError as primary_error: if not fallback_agents: raise + escalations_used = int(primary_error.report.get("escalations_used", 0)) try: - viable, report = _preflight_review_agents(fallback_agents, client=client) + viable, report = _preflight_review_agents( + fallback_agents, client=client, escalations_used=escalations_used + ) except ReviewPreflightError as fallback_error: fallback_error.report["primary_attempt"] = primary_error.report raise diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index 8cb7a096c7..164cf67217 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -484,6 +484,15 @@ printf '{"model":"%s","messages":[{"role":"system","content":"You are a helpful # which exposes no parameter to exclude or deprioritize a specific candidate # on a retry). REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS="${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS:-3}" +# A malformed override (non-numeric, empty, or zero) must fail closed instead +# of silently disabling the bound: `[ "$gateway_attempt" -ge "$X" ]` with a +# non-integer `$X` is itself a bash integer-comparison error, not a false +# result, so the retry loop below would keep looping (never satisfying its +# own exit test) until the surrounding CI job's own timeout kills it instead +# of this check ever rejecting bad configuration on its own. +case "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" in + ''|*[!0-9]*|0) fail "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be a positive integer" ;; +esac gateway_attempt=1 gateway_http_status="" while :; do @@ -506,6 +515,32 @@ while :; do fi if [ "$gateway_attempt" -ge "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" ]; then if [ -z "$gateway_http_status" ]; then + # Every configured attempt exhausted with no usable HTTP response at + # all (Trigger A never resolved) -- record that before failing closed, + # using the same sanitize-then-atomic-replace pattern as the non-2xx + # and invalid-content paths below, so this exact failure case (the one + # telemetry matters most for) does not leave zero evidence trail. + "$sidecar_python" - "$preflight_report" "$gateway_attempt" <<'PY' +import json +from pathlib import Path +import sys + +report_path = Path(sys.argv[1]) +attempts = int(sys.argv[2]) if sys.argv[2].isdecimal() else 0 +try: + report = json.loads(report_path.read_text(encoding="utf-8")) +except (OSError, json.JSONDecodeError): + report = {} +report["gateway"] = { + "endpoint": "chat/completions", + "error_type": "gateway_transport_exhausted", + "attempts": attempts, + "status": "rejected", +} +temporary = report_path.with_suffix(".tmp") +temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") +temporary.replace(report_path) +PY fail "gateway preflight request could not reach the local sidecar after ${gateway_attempt} attempts" fi "$sidecar_python" - "$preflight_report" "$gateway_preflight_response" "$gateway_http_status" "$gateway_attempt" <<'PY' @@ -594,7 +629,7 @@ try: report["gateway"] = { "endpoint": "chat/completions", "status": "rejected", - "error_type": "InvalidChatResponse", + "error_type": "invalid_chat_response", "finish_reason": finish_reason or "unknown", "reasoning_without_content": reasoning_without_content, "attempts": attempts, diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index d5e0f41acb..1c808c2dd4 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -5,9 +5,11 @@ from contextlib import redirect_stdout import io import json +import os import re import runpy from pathlib import Path +import subprocess import sys from types import SimpleNamespace @@ -192,7 +194,7 @@ def test_preflight_mirrors_runtime_request_and_keeps_only_compatible_routes() -> "ready", ] assert report["routes"][0]["error_type"] == "RuntimeError" - assert report["routes"][1]["error_type"] == "InvalidChatResponse" + assert report["routes"][1]["error_type"] == "invalid_chat_response" assert secret not in repr(report) for agent, endpoint, payload in client.calls: @@ -301,6 +303,264 @@ def test_gateway_preflight_retries_transport_failures_up_to_a_bounded_attempt_co assert "gateway preflight returned unusable chat content" in sidecar +_GATEWAY_RETRY_BLOCK_START = 'gateway_virtual_model="orchestrator/${orchestrator_pool}"' +_GATEWAY_RETRY_BLOCK_END = ( + 'log "gateway chat/completions preflight confirmed ' + '(attempt ${gateway_attempt}/${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS})"' +) + +# A minimal stand-in for curl: it never touches the network. Each invocation +# consumes the next numbered plan file in $FAKE_CURL_PLAN_DIR (a fixed, +# test-controlled queue of outcomes, one per expected attempt) so a test can +# script an exact multi-attempt sequence -- transport failure, non-2xx, +# success -- without a real gateway process. A plan file's first line is +# either "FAIL" (curl exits non-zero, exactly like a real timeout with zero +# bytes) or an HTTP status code (written verbatim to stdout, mirroring +# `-w '%{http_code}'`); any remaining lines become the `-o` response body, +# exactly like a real curl would write one. +_FAKE_CURL_SCRIPT = """#!/usr/bin/env bash +set -euo pipefail +plan_dir="$FAKE_CURL_PLAN_DIR" +counter_file="$plan_dir/.count" +count=0 +if [ -f "$counter_file" ]; then + count="$(cat "$counter_file")" +fi +count=$((count + 1)) +printf '%s' "$count" > "$counter_file" +plan_file="$plan_dir/$count" +output_file="" +prev="" +for arg in "$@"; do + if [ "$prev" = "-o" ]; then + output_file="$arg" + fi + prev="$arg" +done +if [ ! -f "$plan_file" ]; then + printf 'fake curl: no plan queued for call %s\\n' "$count" >&2 + exit 2 +fi +status_line="$(head -n 1 "$plan_file")" +if [ "$status_line" = "FAIL" ]; then + exit 28 +fi +if [ -n "$output_file" ]; then + tail -n +2 "$plan_file" > "$output_file" +fi +printf '%s' "$status_line" +""" + + +def _run_gateway_retry_loop( + tmp_path: Path, + *, + max_attempts: int | str, + plan: list[str], +) -> tuple[subprocess.CompletedProcess[str], dict[str, object]]: + """Execute the sidecar's real gateway curl retry loop against a fake curl. + + Extracts the exact, current source of the retry loop from the tracked + sidecar script (rather than a hand-copied duplicate in this test file) + so a future edit to that loop is automatically exercised here instead of + silently drifting from a second, untested copy -- the same drift this + org's conventions flag repository-local workflow copies for elsewhere. + + Args: + tmp_path: Pytest's per-test scratch directory. + max_attempts: Value for ``REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS``, + including deliberately malformed strings for the config-guard + regression test. + plan: One entry per expected curl call, each either ``"FAIL"`` (a + transport failure) or ``"\\n"``. + + Returns: + The completed harness process and the resulting preflight report + (``{}`` when the loop never wrote to it). + """ + sidecar_text = _SIDECAR.read_text(encoding="utf-8") + start = sidecar_text.index(_GATEWAY_RETRY_BLOCK_START) + end = sidecar_text.index(_GATEWAY_RETRY_BLOCK_END, start) + len(_GATEWAY_RETRY_BLOCK_END) + retry_block = sidecar_text[start:end] + + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + fake_curl = fake_bin / "curl" + fake_curl.write_text(_FAKE_CURL_SCRIPT, encoding="utf-8") + fake_curl.chmod(0o755) + + plan_dir = tmp_path / "curl-plan" + plan_dir.mkdir() + for index, outcome in enumerate(plan, start=1): + (plan_dir / str(index)).write_text(outcome, encoding="utf-8") + + work_dir = tmp_path / "work" + work_dir.mkdir() + gateway_preflight_request = work_dir / "gateway-preflight-request.json" + gateway_preflight_request.write_text("{}", encoding="utf-8") + gateway_preflight_response = work_dir / "gateway-preflight.json" + preflight_report = work_dir / "preflight.json" + preflight_report.write_text("{}", encoding="utf-8") + + harness = tmp_path / "harness.sh" + harness.write_text( + "set -euo pipefail\n" + "log() { printf '[test-sidecar] %s\\n' \"$*\"; }\n" + 'fail() { log "error: $*" >&2; exit 1; }\n' + 'orchestrator_pool="free"\n' + 'ORCHESTRATOR_TOKEN="synthetic-test-bearer"\n' + 'ORCHESTRATOR_HOST="127.0.0.1"\n' + 'ORCHESTRATOR_PORT="18080"\n' + 'sidecar_python="$(command -v python3)"\n' + f'gateway_preflight_request="{gateway_preflight_request}"\n' + f'gateway_preflight_response="{gateway_preflight_response}"\n' + f'preflight_report="{preflight_report}"\n' + + retry_block + + "\n", + encoding="utf-8", + ) + + result = subprocess.run( + ["bash", str(harness)], + env={ + **os.environ, + "PATH": f"{fake_bin}{os.pathsep}{os.environ.get('PATH', '')}", + "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS": str(max_attempts), + "FAKE_CURL_PLAN_DIR": str(plan_dir), + }, + text=True, + capture_output=True, + check=False, + ) + report: dict[str, object] = {} + try: + report = json.loads(preflight_report.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + report = {} + return result, report + + +@pytest.mark.parametrize("malformed_value", ["not-a-number", "0", "-1", "3.5"]) +def test_gateway_retry_loop_rejects_a_malformed_attempt_limit_before_any_curl_call( + tmp_path: Path, malformed_value: str +) -> None: + """Regression for Devin Review's malformed-retry-limit-removes-bound + finding: a non-numeric (or zero, or negative) override used to make the + integer comparison `[ "$gateway_attempt" -ge "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" ]` + fail on every iteration -- which evaluates as "not yet at the limit," so + the loop would retry forever instead of failing closed on bad config. + (An empty override is not exercised here: ``${VAR:-3}`` already treats + unset-or-empty as "use the default," so it never reaches the guard -- + the guard's own ``''`` pattern is defense in depth for a future change to + that assignment, not a reachable case today.) + + The plan is deliberately empty: if the fix regresses and the loop reaches + curl at all, the fake curl exits 2 with a distinct "no plan queued" + message, which the assertions below would not match -- proving this + fails closed on the config check itself, never even attempting a call. + """ + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=malformed_value, plan=[] + ) + + assert result.returncode == 1 + assert "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be a positive integer" in result.stderr + assert report == {} + + +def test_gateway_retry_loop_succeeds_on_the_first_attempt(tmp_path: Path) -> None: + """A clean 200 on the very first curl call needs no retry at all.""" + success_body = json.dumps({"choices": [{"message": {"content": "OK"}}]}) + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=3, plan=[f"200\n{success_body}"] + ) + + assert result.returncode == 0, result.stderr + assert "confirmed (attempt 1/3)" in result.stdout + assert report["gateway"] == { + "endpoint": "chat/completions", + "status": "ready", + "attempts": 1, + } + + +def test_gateway_retry_loop_recovers_from_one_transport_failure(tmp_path: Path) -> None: + """ADR-0005 Trigger A: a timeout with zero bytes is retried, not fatal. + + Regression for the live ContextualWisdomLab/.github#1449 reproduction + (job 99253418179): a curl timeout with no response used to abort the + sidecar outright with no recovery path at all. + """ + success_body = json.dumps({"choices": [{"message": {"content": "OK"}}]}) + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=3, plan=["FAIL", f"200\n{success_body}"] + ) + + assert result.returncode == 0, result.stderr + assert "did not reach the sidecar cleanly (status=unreachable); retrying" in result.stdout + assert "confirmed (attempt 2/3)" in result.stdout + assert report["gateway"] == { + "endpoint": "chat/completions", + "status": "ready", + "attempts": 2, + } + + +def test_gateway_retry_loop_records_a_non2xx_rejection_after_exhausting_attempts( + tmp_path: Path, +) -> None: + """A non-2xx status on every attempt fails closed with retry-aware evidence. + + The second (retry) attempt's rejection is recorded as + ``gateway_retry_rejected``, distinct from a first-attempt rejection, + since the virtual pool's routing is not pinned across separate calls. + """ + error_body = json.dumps({"error": {"code": "invalid_structured_output"}}) + result, report = _run_gateway_retry_loop( + tmp_path, + max_attempts=2, + plan=[f"500\n{error_body}", f"500\n{error_body}"], + ) + + assert result.returncode == 1 + assert "gateway preflight returned HTTP 500 after 2 attempts" in result.stderr + assert report["gateway"] == { + "endpoint": "chat/completions", + "error_type": "gateway_retry_rejected", + "error_code": "invalid_structured_output", + "http_status": 500, + "attempts": 2, + "status": "rejected", + } + + +def test_gateway_retry_loop_records_transport_exhaustion_evidence_before_failing( + tmp_path: Path, +) -> None: + """Regression for Devin Review's transport-exhaustion-loses-evidence + finding: exhausting every attempt on repeated transport failures (never + receiving one usable HTTP response) used to fail closed with the + preflight report untouched -- exactly the failure case telemetry matters + most for left zero trace of attempt count or trigger. Must now record a + bounded classification before ``fail`` exits. + """ + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=2, plan=["FAIL", "FAIL"] + ) + + assert result.returncode == 1 + assert ( + "gateway preflight request could not reach the local sidecar after 2 attempts" + in result.stderr + ) + assert report["gateway"] == { + "endpoint": "chat/completions", + "error_type": "gateway_transport_exhausted", + "attempts": 2, + "status": "rejected", + } + + def test_reasoning_without_content_escalates_then_still_fails_closed_if_unresolved() -> None: """ADR-0005 round 5 (Devin Review): escalation must key off the vendored ``ModelClient._response_content``'s own "reasoning, no content" signature, @@ -408,7 +668,7 @@ def test_escalation_budget_is_shared_and_bounded_across_candidates() -> None: exhausted_row = failure.value.report["routes"][-1] assert exhausted_row["attempts"] == 1 - assert exhausted_row["error_type"] == "EscalationBudgetExhausted" + assert exhausted_row["error_type"] == "escalation_budget_exhausted" assert failure.value.report["escalations_used"] == max_escalations assert len(client.calls) == max_escalations * 2 + 1 @@ -416,7 +676,7 @@ def test_escalation_budget_is_shared_and_bounded_across_candidates() -> None: def test_escalated_probe_rejection_is_recorded_distinctly_and_not_retried() -> None: """A non-2xx rejection specifically on the escalated attempt is distinguishable evidence the escalated budget exceeds that candidate's - real ceiling -- recorded as ``EscalatedProbeRejected``, not conflated + real ceiling -- recorded as ``escalated_probe_rejected``, not conflated with a generic empty-content rejection, and not retried again. """ namespace = _load_launcher() @@ -425,10 +685,16 @@ def test_escalated_probe_rejection_is_recorded_distinctly_and_not_retried() -> N low_ceiling = SimpleNamespace( id="nvidia_nim_low_ceiling", provider_name="nvidia_nim", model="low/free" ) + + class _HttpError(RuntimeError): + """A synthetic exception carrying an HTTP status, like a real client's.""" + + code = 429 + client = _SequencedClient( [ {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, - RuntimeError("max_tokens exceeds this model's ceiling"), + _HttpError("max_tokens exceeds this model's ceiling"), ] ) @@ -437,10 +703,107 @@ def test_escalated_probe_rejection_is_recorded_distinctly_and_not_retried() -> N assert len(client.calls) == 2 row = failure.value.report["routes"][0] - assert row["error_type"] == "EscalatedProbeRejected" + assert row["error_type"] == "escalated_probe_rejected" + assert row["http_status"] == 429 + assert row["attempts"] == 2 + + +def test_escalated_probe_transport_failure_is_not_mislabeled_as_a_rejection() -> None: + """A transport failure (no HTTP status at all) on the escalated attempt + must never become ``escalated_probe_rejected`` -- that label is reserved + for a genuine provider rejection of the escalated budget specifically. + A bare connection timeout is a different root cause and gets the same + sanitized exception-type recording the base probe uses. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + flaky = SimpleNamespace( + id="openrouter_flaky", provider_name="openrouter", model="flaky/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + TimeoutError("connection timed out with zero bytes received"), + ] + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([flaky], client=client) + + row = failure.value.report["routes"][0] + assert row["error_type"] == "TimeoutError" + assert "http_status" not in row assert row["attempts"] == 2 +def test_escalated_probe_transport_failure_sanitizes_an_unsafe_exception_name() -> None: + """An escalated-attempt exception whose type name is unsafe to log + verbatim (not a plain identifier, or implausibly long) still falls back + to the same bounded ``provider_error`` placeholder the base probe uses, + rather than ever copying raw exception state into evidence. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + unsafe_exception_type = type("Not An Identifier", (RuntimeError,), {}) + + flaky = SimpleNamespace( + id="openrouter_unsafe_exception", provider_name="openrouter", model="flaky/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + unsafe_exception_type("unsafe"), + ] + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([flaky], client=client) + + row = failure.value.report["routes"][0] + assert row["error_type"] == "provider_error" + assert "http_status" not in row + + +def test_escalated_empty_response_updates_both_telemetry_fields_together() -> None: + """``finish_reason`` and ``reasoning_without_content`` must describe the + SAME (final) attempt -- regression for Devin Review's mixed-attempt + telemetry finding. The base attempt matches Trigger B via + ``finish_reason == "length"`` (``reasoning_without_content`` is False); + the escalated attempt comes back with a completely different signature + (no ``finish_reason`` at all, but a populated ``reasoning`` field with no + content). Both fields must end up describing attempt 2, not a stale mix + of attempt 1's ``reasoning_without_content`` with attempt 2's + ``finish_reason``. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + still_starved = SimpleNamespace( + id="nvidia_nim_still_starved", provider_name="nvidia_nim", model="starved/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + { + "choices": [ + {"message": {"content": "", "reasoning": "still reasoning, no answer yet"}} + ] + }, + ] + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([still_starved], client=client) + + row = failure.value.report["routes"][0] + assert row["attempts"] == 2 + # Both fields reflect the escalated (final) attempt, not the base one. + assert row["finish_reason"] == "unknown" + assert row["reasoning_without_content"] is True + + def test_preflight_fails_closed_when_every_route_rejects() -> None: """A healthy HTTP process is not review-ready without one live LLM route.""" namespace = _load_launcher() @@ -502,6 +865,67 @@ def test_preflight_uses_priced_fallback_only_after_primary_routes_reject() -> No assert failure.value.report["primary_attempt"]["ready_count"] == 0 +def test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case() -> None: + """Regression for Devin Review's fallback-retries-exceed-startup-deadline + finding: ``_preflight_review_agents`` used to start ``escalations_used`` + fresh on every call, so ``_preflight_with_fallback`` calling it twice (up + to 8 primary routes, then up to 4 fallback routes) could spend the full + ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` budget in EACH stage -- up to 8 + escalations total, 200s worst case (12 base attempts + 8 escalations x + 10s), blowing past Layer 1's 180s healthz-readiness watchdog and + contradicting the ADR's own claimed 160s worst case. + + This drives all 8 primary routes and all 4 fallback routes (the exact + ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`` split) through a response that + always qualifies for escalation and never resolves, so every one of the + 12 candidates *would* escalate if the budget were not shared. Asserts + the run spends at most ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` escalations + in total (not per stage), and that the resulting worst-case attempt count + keeps total elapsed time at or under 160s -- both stages' escalation + counts are visible in the returned evidence. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_with_fallback"] + max_escalations = namespace["REVIEW_PREFLIGHT_MAX_ESCALATIONS"] + timeout_seconds = namespace["REVIEW_PREFLIGHT_TIMEOUT_SECONDS"] + primary_limit = namespace["REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT"] + total_route_limit = namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] + fallback_limit = total_route_limit - primary_limit + + budget_starved_response = { + "choices": [{"finish_reason": "length", "message": {"content": ""}}] + } + primary_agents = [ + SimpleNamespace(id=f"primary_{index}", provider_name="openrouter", model="x/free") + for index in range(primary_limit) + ] + fallback_agents = [ + SimpleNamespace(id=f"fallback_{index}", provider_name="openrouter", model="y/priced") + for index in range(fallback_limit) + ] + client = _ProbeClient( + {agent.id: dict(budget_starved_response) for agent in [*primary_agents, *fallback_agents]} + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight(primary_agents, fallback_agents, client=client) + + report = failure.value.report + assert report["escalations_used"] == max_escalations + assert report["primary_attempt"]["escalations_used"] == max_escalations + + total_attempts = len(client.calls) + worst_case_seconds = total_attempts * timeout_seconds + assert worst_case_seconds <= 160, ( + f"worst-case preflight time ({worst_case_seconds}s across " + f"{total_attempts} attempts) must stay within the 160s the ADR " + "computes and the 180s healthz-readiness watchdog allows" + ) + # Exactly the ADR's own worst-case arithmetic: 12 base attempts (one per + # candidate across both stages) + 4 escalations (the shared cap) = 16. + assert total_attempts == total_route_limit + max_escalations + + def test_preflight_stage_limits_share_one_startup_budget() -> None: """Free-first and priced-fallback probes share one bounded route budget.""" namespace = _load_launcher() From 5fc8daeb5713152e2e69a6756d481d752753fd57 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 13:17:06 +0000 Subject: [PATCH 08/12] fix(sidecar): resolve second Devin Review pass on PR #1452 Three fixable findings from a fresh review pass triggered by the prior push: a successful escalated attempt still carried the base attempt's stale finish_reason/reasoning_without_content (mirror of the earlier mixed-attempt fix, on the success branch) -- both fields now refresh from the escalated response on success too. The new REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS case guard rejected non-numeric values but not oversized all-digit ones, which hit the identical integer-overflow failure the guard exists to prevent (reproduced directly: a 55-digit value fails "[: integer expression expected", same as a non-numeric one) -- the guard now also caps digit count (at most 4 digits, 9999). Added fake-curl tests for mixed retry-outcome sequences (transport failure then HTTP rejection, and the reverse). Two more findings verified as real and architecturally significant, left open rather than guess-fixed, each filed as a tracked issue: - ContextualWisdomLab/.github#1454: a base-probe success (16 tokens) never confirms a candidate at the real serving budget (REVIEW_MAX_OUTPUT_TOKENS, 4096) -- escalation only fires on evidence of failure. ADR-0005's own Research already documents a provider's hard completion-ceiling as a real, separate-from- reasoning-overhead axis; mitigated in production (not fixed here) by contextual-orchestrator's own per-request failover/circuit breaker. - ContextualWisdomLab/.github#1455: Layer 1's "160s worst case" covers only probing, not discover_all_models()'s own sequential network time, which runs first inside the SAME 180s watchdog. Verified directly against the vendored contextual_orchestrator.model_discovery source: up to ~7 sequential HTTP calls at up to 15s each (DISCOVERY_TIMEOUT_SECONDS), ~105s worst case, for a combined real worst case up to ~265s, not 160s. Both documented in place with cross-references rather than left as a silent, inaccurate safety-margin claim. 1917 tests pass (1913 + 4 new), 100% coverage and 100% docstring coverage on scripts/ci/, bash -n parses cleanly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 27 +++++ docs/product-technical-gap-baseline.md | 31 +++++ ...contextual_orchestrator_review_launcher.py | 49 +++++++- .../contextual_orchestrator_review_sidecar.sh | 23 +++- ...l_orchestrator_review_runtime_preflight.py | 110 +++++++++++++++++- 5 files changed, 231 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 777a1cec27..a8d122e0c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,33 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix 3 more Devin Review findings from a second review pass on PR #1452 + (`scripts/ci/contextual_orchestrator_review_launcher.py`, + `scripts/ci/contextual_orchestrator_review_sidecar.sh`, + `tests/test_contextual_orchestrator_review_runtime_preflight.py`), triggered + by the push that resolved the first 7: a successful escalated attempt still + carried the base attempt's stale `finish_reason`/`reasoning_without_content` + (the same class of bug as the mixed-attempt fix above, on the opposite + branch) -- now both fields are refreshed from the escalated response on + success too. `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS`'s new `case` guard + rejected non-numeric values but not oversized all-digit ones, which hit the + identical `[ -ge ]` integer-overflow failure mode the guard exists to + prevent (reproduced directly: a 55-digit value fails the same way a + non-numeric one did) -- the guard now also caps digit count (at most 4 + digits, 9999). Added mixed-outcome fake-curl tests (transport failure then + HTTP rejection, and the reverse) proving exhaustion evidence reflects + whichever attempt actually happened last. Two further findings from the same + pass -- (1) a base-probe success never confirms the candidate at the real + serving token budget (only escalation-on-failure does), and (2) + `discover_all_models()`'s own up-to-~105s sequential network time (verified + against the vendored `contextual_orchestrator.model_discovery` source: ~7 + sequential HTTP calls at up to 15s each) is not counted against the same + 180s watchdog Layer 1's 160s probing bound assumes it has entirely to + itself -- are real, verified, and architecturally significant enough to need + their own design pass rather than a guessed patch; documented in place with + cross-references and tracked as `ContextualWisdomLab/.github#1454` and + `#1455` respectively, left open (not resolved) on the PR. 1917 tests pass; + 100% coverage and 100% docstring coverage on `scripts/ci/`. - Fix 7 Devin Review findings on PR #1452, ADR-0005's implementation (`scripts/ci/contextual_orchestrator_review_launcher.py`, `scripts/ci/contextual_orchestrator_review_sidecar.sh`, diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index b15d51c0e6..7166ae7116 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1454,6 +1454,37 @@ new), 100% coverage and 100% docstring coverage on `scripts/ci/`, `bash -n` synt script, and all 4 embedded Python heredoc blocks in it (including the new transport-exhaustion evidence writer) parse cleanly. +**A second Devin Review pass, triggered by that push, found 3 more real, fixable issues (all fixed) and +2 architecturally significant gaps verified as real but not guess-fixed.** Fixed: a successful escalated +attempt still carried the base attempt's stale `finish_reason`/`reasoning_without_content` (the mixed- +attempt bug's mirror image, on the success branch instead of the failure branch) — both fields now +refresh from the escalated response on success too. The `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS` `case` +guard rejected non-numeric values but not oversized all-digit ones — reproduced directly that a 55-digit +value hits the identical `[ -ge ]` integer-overflow failure the guard exists to prevent — so the guard now +also caps digit count (at most 4 digits, 9999). Added fake-curl tests for mixed retry-outcome sequences +(transport failure then HTTP rejection, and the reverse), proving exhaustion evidence reflects whichever +attempt actually happened last. + +**Verified real but left open, tracked as `ContextualWisdomLab/.github#1454` and `#1455`:** (1) a +candidate that succeeds at the cheap `REVIEW_PREFLIGHT_BASE_TOKENS = 16` base probe is admitted without +ever being confirmed at the real serving budget (`REVIEW_MAX_OUTPUT_TOKENS = 4096`) — escalation only +fires on evidence of *failure*, not to confirm success at the real budget, and ADR-0005's own Research +(axis 2) already documents that a provider's hard completion-token ceiling is a real, per-model quantity +separate from reasoning overhead; mitigated in production (not fixed here) by +`contextual_orchestrator.orchestrator.TaskOrchestrator`'s own per-request failover/circuit-breaker, which +this preflight does not replace. (2) Layer 1's "160s worst case" arithmetic covers only probing, not +`discover_all_models()`'s own time, which runs first inside the *same* 180s healthz-readiness watchdog — +verified directly against the vendored `contextual_orchestrator.model_discovery` source: up to ~7 +sequential HTTP calls (shared models.dev metadata, one per `PROVIDER_MODEL_SOURCES` entry with a +registered credential — 5 of 6 for this sidecar's pool — and the OpenRouter ZDR feed), each up to +`DISCOVERY_TIMEOUT_SECONDS = 15s`, for a discovery-alone worst case of up to ~105s and a combined real +worst case of up to ~265s, not 160s. Both are documented in place with cross-references (source comments +in `contextual_orchestrator_review_launcher.py` and `contextual_orchestrator_review_sidecar.sh`) rather +than silently mischaracterizing safety margins that do not actually exist. Neither was guess-fixed: each +needs its own evidence-based design pass (per this org's convergence convention — initial values from +precedent, refinement from telemetry, never from inspection alone) before a specific number or mechanism +is chosen. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 36c67a6d2b..2f85720602 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -61,11 +61,27 @@ # number. REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS # Shared cap on how many candidates in one preflight run may use the -# escalation retry above, so Layer 1's worst case stays computed and bounded: -# REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES * REVIEW_PREFLIGHT_TIMEOUT_SECONDS + -# REVIEW_PREFLIGHT_MAX_ESCALATIONS * REVIEW_PREFLIGHT_TIMEOUT_SECONDS -# = 12*10 + 4*10 = 160s, under the sidecar's existing 180s healthz-readiness -# wait. See docs/adr/0005-sidecar-preflight-token-budget.md, Decision section 3. +# escalation retry above, so Layer 1's PROBING worst case stays computed and +# bounded: REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES * REVIEW_PREFLIGHT_TIMEOUT_SECONDS +# + REVIEW_PREFLIGHT_MAX_ESCALATIONS * REVIEW_PREFLIGHT_TIMEOUT_SECONDS +# = 12*10 + 4*10 = 160s, under the sidecar's 180s healthz-readiness wait. See +# docs/adr/0005-sidecar-preflight-token-budget.md, Decision section 3. +# +# KNOWN GAP, tracked (not yet fixed): this 160s covers only probing, not the +# discover_all_models() call that runs before it inside the SAME 180s +# watchdog. Verified directly against the vendored contextual-orchestrator +# source: discover_all_models() makes up to ~7 sequential HTTP calls (the +# shared models.dev fetch, one per PROVIDER_MODEL_SOURCES entry with a +# registered credential, and the OpenRouter ZDR endpoint fetch), each up to +# DISCOVERY_TIMEOUT_SECONDS = 15s -- up to ~105s worst case, before probing's +# own 160s even starts. Combined real worst case is therefore up to ~265s, +# not 160s. See ContextualWisdomLab/.github#1455 for the tracked fix (a +# shared monotonic deadline, scaled-down probing, or an evidence-justified +# watchdog extension) and #1454 for the related, separately-tracked gap that +# a base-probe *success* never confirms the candidate at the real serving +# budget (REVIEW_MAX_OUTPUT_TOKENS). Neither blocks this PR's 7 verified +# findings; both are architecturally significant enough to need their own +# design pass rather than a guessed patch here. REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4 @@ -365,6 +381,20 @@ def _preflight_review_agents( routes.append(row) continue if _chat_response_has_text(response): + # KNOWN GAP, tracked (not yet fixed) as + # ContextualWisdomLab/.github#1454: this admits the candidate + # having only proven it works at REVIEW_PREFLIGHT_BASE_TOKENS + # (16), never at the real serving budget + # (REVIEW_MAX_OUTPUT_TOKENS, 4096) main()'s ModelClient actually + # requests. ADR-0005's own Research (axis 2) already documents + # that a provider's hard completion-token ceiling is a real, + # separate-from-reasoning-overhead quantity per model; a + # candidate whose real ceiling sits strictly between 16 and 4096 + # would pass here and only fail later, on real review traffic. + # Mitigated in production (not fixed here) by + # contextual_orchestrator.orchestrator.TaskOrchestrator's own + # per-request failover/circuit-breaker, which this preflight + # does not replace. row["status"] = "ready" routes.append(row) viable.append(agent) @@ -413,6 +443,15 @@ def _preflight_review_agents( if _chat_response_has_text(escalated_response): row["status"] = "ready" row["escalated"] = True + # Overwrite the base attempt's stale diagnostic fields with the + # escalated (successful, final) attempt's own state -- otherwise + # a ready route's evidence would still show the budget-too-small + # signature that triggered the escalation in the first place, + # describing a response this route no longer produced. + row["finish_reason"] = _response_finish_reason(escalated_response) or "unknown" + row["reasoning_without_content"] = _response_has_reasoning_without_content( + escalated_response + ) routes.append(row) viable.append(agent) continue diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index 164cf67217..e9e1ed1d78 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -390,6 +390,15 @@ until curl -fsSL --max-time 2 "http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/ fail "sidecar exited before healthz (status ${sidecar_status}); stderr: $(sed -n '1,20p' "$sidecar_stderr")" fi i=$((i + 1)) + # KNOWN GAP, tracked as ContextualWisdomLab/.github#1455 (not yet fixed): + # this 180s covers the launcher's ENTIRE startup sequence -- discovery, + # catalog build, AND preflight probing -- not just probing. Layer 1's own + # "160s worst case" comment + # (contextual_orchestrator_review_launcher.py's REVIEW_PREFLIGHT_MAX_ESCALATIONS) + # accounts only for probing; discover_all_models() runs first, inside this + # same 180s, and can itself take up to ~105s worst case (verified against + # the vendored contextual_orchestrator.model_discovery source: ~7 + # sequential HTTP calls at up to 15s each). if [ "$i" -ge 180 ]; then fail "sidecar did not become healthy; stderr: $(sed -n '1,20p' "$sidecar_stderr")" fi @@ -489,9 +498,19 @@ REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS="${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS:- # non-integer `$X` is itself a bash integer-comparison error, not a false # result, so the retry loop below would keep looping (never satisfying its # own exit test) until the surrounding CI job's own timeout kills it instead -# of this check ever rejecting bad configuration on its own. +# of this check ever rejecting bad configuration on its own. An all-digit +# value is not automatically safe either: `[ -ge ]` still errors the exact +# same way once the value overflows the shell's integer range (reproduced +# directly: a 55-digit all-digit string fails with "integer expression +# expected", identical to a non-numeric one) -- so the bound below also caps +# digit COUNT, not just digit-ness. Four digits (up to 9999) is already far +# beyond any realistic attempt count and stays safely representable on every +# platform this runs on. case "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" in - ''|*[!0-9]*|0) fail "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be a positive integer" ;; + ''|*[!0-9]*|0) + fail "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be a positive integer" ;; + ?????*) + fail "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be at most 9999" ;; esac gateway_attempt=1 gateway_http_status="" diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 1c808c2dd4..8971d2c39f 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -468,6 +468,42 @@ def test_gateway_retry_loop_rejects_a_malformed_attempt_limit_before_any_curl_ca assert report == {} +def test_gateway_retry_loop_rejects_an_oversized_attempt_limit_before_any_curl_call( + tmp_path: Path, +) -> None: + """Regression for a follow-up Devin Review finding on the malformed-limit + fix: an all-digit value is not automatically safe -- `[ -ge ]` errors the + identical way once the value overflows the shell's integer range (a + 55-digit all-digit string reproduces "integer expression expected", + exactly like a non-numeric one), so the digit-only guard alone is + insufficient. This asserts a value that passes the digit-only check but + is absurdly long is still rejected, closed, before any curl call. + """ + result, report = _run_gateway_retry_loop( + tmp_path, + max_attempts="9" * 55, + plan=[], + ) + + assert result.returncode == 1 + assert "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be at most 9999" in result.stderr + assert report == {} + + +def test_gateway_retry_loop_accepts_the_maximum_allowed_attempt_limit(tmp_path: Path) -> None: + """The digit-count cap's boundary (9999) itself must still be accepted -- + proving the guard rejects on length, not by rejecting every large-looking + value indiscriminately. + """ + success_body = json.dumps({"choices": [{"message": {"content": "OK"}}]}) + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts="9999", plan=[f"200\n{success_body}"] + ) + + assert result.returncode == 0, result.stderr + assert report["gateway"]["status"] == "ready" + + def test_gateway_retry_loop_succeeds_on_the_first_attempt(tmp_path: Path) -> None: """A clean 200 on the very first curl call needs no retry at all.""" success_body = json.dumps({"choices": [{"message": {"content": "OK"}}]}) @@ -561,6 +597,61 @@ def test_gateway_retry_loop_records_transport_exhaustion_evidence_before_failing } +def test_gateway_retry_loop_classifies_a_transport_then_http_exhaustion_by_the_final_attempt( + tmp_path: Path, +) -> None: + """Regression for Devin Review's mixed-retry-outcomes-lack-coverage + finding: the failure type can change between attempts (a transport + failure retried into an HTTP rejection, or the reverse), and the final + evidence must reflect the LAST attempt's actual outcome, not the first. + Here attempt 1 times out (no response at all) and attempt 2 gets a + non-2xx response -- exhaustion must classify as the non-2xx path + (`http_status` present, `gateway_retry_rejected` since this is a retry), + not the transport-exhaustion path. + """ + error_body = json.dumps({"error": {"code": "invalid_structured_output"}}) + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=2, plan=["FAIL", f"500\n{error_body}"] + ) + + assert result.returncode == 1 + assert "gateway preflight returned HTTP 500 after 2 attempts" in result.stderr + assert report["gateway"] == { + "endpoint": "chat/completions", + "error_type": "gateway_retry_rejected", + "error_code": "invalid_structured_output", + "http_status": 500, + "attempts": 2, + "status": "rejected", + } + + +def test_gateway_retry_loop_classifies_an_http_then_transport_exhaustion_by_the_final_attempt( + tmp_path: Path, +) -> None: + """The reverse mixed sequence: attempt 1 gets a non-2xx response, attempt + 2 times out with no response at all. Exhaustion must classify as the + transport-exhaustion path (no `http_status`), matching what actually + happened on the final, decisive attempt. + """ + error_body = json.dumps({"error": {"code": "invalid_structured_output"}}) + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=2, plan=[f"500\n{error_body}", "FAIL"] + ) + + assert result.returncode == 1 + assert ( + "gateway preflight request could not reach the local sidecar after 2 attempts" + in result.stderr + ) + assert report["gateway"] == { + "endpoint": "chat/completions", + "error_type": "gateway_transport_exhausted", + "attempts": 2, + "status": "rejected", + } + + def test_reasoning_without_content_escalates_then_still_fails_closed_if_unresolved() -> None: """ADR-0005 round 5 (Devin Review): escalation must key off the vendored ``ModelClient._response_content``'s own "reasoning, no content" signature, @@ -611,6 +702,13 @@ def test_finish_reason_length_escalates_and_can_succeed() -> None: """The OpenAI-documented ``finish_reason == "length"`` signature also escalates, independent of the ``reasoning`` field, and a candidate that only needed a bigger budget is correctly marked ready on the retry. + + Also a regression for Devin Review's successful-escalations-keep-stale- + telemetry finding: the escalated (successful, final) response here + deliberately carries a DIFFERENT ``finish_reason`` (``"stop"``) than the + base attempt's ``"length"``, so a stale, unrefreshed field would be + caught -- the row must describe the response that actually made this + route ready, not the earlier one that didn't. """ namespace = _load_launcher() preflight = namespace["_preflight_review_agents"] @@ -621,7 +719,14 @@ def test_finish_reason_length_escalates_and_can_succeed() -> None: client = _SequencedClient( [ {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, - _openai_text("OK, here is the answer."), + { + "choices": [ + { + "finish_reason": "stop", + "message": {"content": "OK, here is the answer."}, + } + ] + }, ] ) @@ -636,7 +741,8 @@ def test_finish_reason_length_escalates_and_can_succeed() -> None: assert row["status"] == "ready" assert row["attempts"] == 2 assert row["escalated"] is True - assert row["finish_reason"] == "length" + # Describes the escalated (final) attempt, not the stale base one. + assert row["finish_reason"] == "stop" assert row["reasoning_without_content"] is False assert report["escalations_used"] == 1 From e7704e0b24c652a7e212b7940cff49efca7f53f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 13:28:16 +0000 Subject: [PATCH 09/12] fix(sidecar): resolve third Devin Review pass on PR #1452 An escalated-attempt HTTP rejection (401 auth, 429 throttle, 5xx server error) was unconditionally labeled escalated_probe_rejected, wrongly implying every one of those was evidence the token budget specifically was too large -- no status code alone is that evidence, and this codebase deliberately never captures raw provider error text that could validate the distinction. Extracted a shared _record_provider_exception helper so the escalated attempt now gets the exact same sanitized exception-type/HTTP-status classification the base probe already used for any exception, with parametrized 401/429/500/503 test coverage. Corrected the ADR's own text, which originated this over-claim, to match. Separately, finish_reason/reasoning_without_content were populated only on failure/escalation outcomes, never on an ordinary successful probe -- the single most common outcome, and the whole reason this telemetry was added was "future tuning can be evidence-driven." Fixed in both the launcher (base-probe and escalated-probe success paths) and the sidecar script's successful-gateway-evidence writer. Two lower-priority items from the same review pass consciously left as-is: the fake-curl test harness doesn't model a real curl partial-write-on-failure edge case (test-fidelity gap, not a production bug); the attempt-limit guard's 9999 digit-count cap is looser than the design's intended single-digit range but not exploitable today -- tightening it without real evidence would itself be an unjustified guess, which this org's own convergence convention exists to prevent. 1920 tests pass (1917 + 3 new/extended), 100% coverage and 100% docstring coverage on scripts/ci/, bash -n and all 4 embedded Python heredocs parse cleanly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 21 +++++ .../0005-sidecar-preflight-token-budget.md | 43 +++++++-- docs/product-technical-gap-baseline.md | 29 ++++++ ...contextual_orchestrator_review_launcher.py | 94 +++++++++++-------- .../contextual_orchestrator_review_sidecar.sh | 23 +++-- ...l_orchestrator_review_runtime_preflight.py | 80 +++++++++++----- 6 files changed, 213 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8d122e0c6..924ef19164 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,27 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix 2 more Devin Review findings from a third review pass on PR #1452 + (`scripts/ci/contextual_orchestrator_review_launcher.py`, + `scripts/ci/contextual_orchestrator_review_sidecar.sh`, + `docs/adr/0005-sidecar-preflight-token-budget.md`, + `tests/test_contextual_orchestrator_review_runtime_preflight.py`): an + escalated-attempt HTTP rejection (401 auth, 429 throttle, 5xx server error) + was unconditionally labeled `escalated_probe_rejected`, wrongly implying + every one of those was evidence the token budget specifically was too large + -- no status code alone is that evidence, and this codebase deliberately + never captures raw provider error text that could validate the distinction. + Extracted a shared `_record_provider_exception` helper so the escalated + attempt now gets the exact same sanitized exception-type/HTTP-status + classification the base probe already used, with parametrized 401/429/5xx + test coverage; the ADR's own text (which originally claimed this + attribution) is corrected in place. Separately, `finish_reason`/ + `reasoning_without_content` were only ever populated on failure/escalation + outcomes, never on an ordinary successful probe (the most common case) -- + now populated on every outcome, in both the launcher and the sidecar + script's successful-gateway-evidence writer, so future tuning has a real + "normal" baseline to compare against. 1920 tests pass; 100% coverage and + 100% docstring coverage on `scripts/ci/`. - Fix 3 more Devin Review findings from a second review pass on PR #1452 (`scripts/ci/contextual_orchestrator_review_launcher.py`, `scripts/ci/contextual_orchestrator_review_sidecar.sh`, diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index 3a8bec335d..e93756de66 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -207,12 +207,16 @@ revisited then (a natural extension of `ContextualWisdomLab/contextual-orchestra does not invent that mechanism speculatively. - **Both triggers draw from one small, shared, explicit retry budget per layer** (Decision §3), not "one retry per route" unconditionally. -- **A non-2xx rejection on a Layer 1 escalated (Trigger-B) retry** is distinguishable evidence the - *escalated* budget specifically — not the base one — exceeds that one candidate's real ceiling - (genuinely attributable, since the candidate is pinned). Recorded as its own outcome, - `escalated_probe_rejected`, and that candidate is not retried further this run. The complete fix - (knowing each model's real ceiling in advance) is `ContextualWisdomLab/contextual-orchestrator#927`, - not this ADR. +- **A non-2xx rejection on a Layer 1 escalated (Trigger-B) retry** is recorded with the same sanitized + exception-type/HTTP-status evidence the base probe uses, and that candidate is not retried further this + run — **revised during implementation** (PR #1452, a later Devin Review pass): this ADR originally + claimed such a rejection was "distinguishable evidence the escalated budget specifically exceeds that + candidate's real ceiling," labeled `escalated_probe_rejected`. That over-claimed attribution — an HTTP + status alone (401 auth, 429 throttle, 5xx server error, ...) is not evidence the token budget caused the + rejection, only that some request failed, and this codebase deliberately never captures raw provider + error text that could validate the distinction. The complete fix (knowing each model's real ceiling in + advance, so a genuine budget-ceiling rejection could be told apart from any other) is + `ContextualWisdomLab/contextual-orchestrator#927`, not this ADR. - **A non-2xx rejection on a Layer 2 Trigger-A retry** is recorded as `gateway_retry_rejected` — deliberately **not** named or described as candidate-ceiling evidence, because Layer 2 structurally cannot confirm which candidate served the rejected attempt. @@ -285,8 +289,9 @@ retried once, unconditionally, would be a real, computed worst-case blowup again loop. Not blocking for §1-3. - **Track `ContextualWisdomLab/contextual-orchestrator#927`** (real, separately-provenanced `max_output_tokens`/`context_window` fields, fail-closed when unknown) so `max_tokens` selection can - eventually be derived from real per-model data, including resolving the `escalated_probe_rejected` - case in §1 properly instead of just recording it. Not blocking for §1-3. + eventually be derived from real per-model data, including telling a genuine budget-ceiling rejection + on an escalated attempt (§1) apart from any other cause with real evidence, instead of the generic, + honestly-unattributed classification used today. Not blocking for §1-3. - **Explicitly reject** further tuning of one global `max_tokens` constant, or of a single generic "retry," as a terminal fix for either layer. Every single-constant value tried so far (16, 4096) has failed for a different, evidenced reason tied to pool heterogeneity, and a single undifferentiated @@ -321,6 +326,28 @@ outcome already observed in production.** `ContextualWisdomLab/contextual-orchestrator#927` lands. Layer 2's retry-diversity limitation (Decision §1) is accepted the same way, for the same reason: no verified mechanism exists today to do better. +- **Two more known, accepted, documented residual limitations, verified during implementation (PR #1452) + and decided not to block it**, for the same reason as the two immediately above — this design is a + genuine, verified improvement over the status quo it replaces, and does not need to close every + residual failure mode to be worth shipping: + - A Layer 1 candidate that succeeds at the cheap `REVIEW_PREFLIGHT_BASE_TOKENS` (`16`) base probe is + admitted without ever being confirmed at the real serving budget + (`REVIEW_MAX_OUTPUT_TOKENS`, `4096`) — escalation only fires on evidence of *failure*, not to + *confirm* success at the real budget, so a candidate whose real ceiling sits strictly between the two + could pass here and only fail later, on real review traffic. Mitigated in production (not eliminated) + by `TaskOrchestrator`'s existing per-request failover and per-agent circuit breaker. Tracked as + `ContextualWisdomLab/.github#1454`. + - Layer 1's `160s` worst case (Decision §3) accounts only for probing/escalation, not for + `discover_all_models()`'s own sequential network time, which runs first inside the *same* 180s + healthz-readiness watchdog — verified against the vendored `contextual_orchestrator.model_discovery` + source at up to ~7 sequential HTTP calls, each up to `DISCOVERY_TIMEOUT_SECONDS = 15s` (~105s worst + case), for a combined real worst case of up to ~265s. This requires two unlikely conditions to + coincide (discovery near its own worst case *and* probing separately needing close to its full + escalation budget) to actually exceed the watchdog, making it a tail case rather than the common + path; no real timing telemetry exists yet to justify a specific fix (a shared deadline, scaled-down + probing, or an evidence-justified watchdog extension), consistent with this ADR's own rejection of + picking a number from inspection alone (Context, "어떠한 휴리스틱과 Rule of thumbs도 금지"). Tracked as + `ContextualWisdomLab/.github#1455`. - Items in Decision §4 are real `contextual-orchestrator` feature work, now tracked as real issues, and would remain explicitly not closed by this ADR even once the sidecar-side implementation lands. - No production routing default changes are proposed; this is scoped to the sidecar's own liveness diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 7166ae7116..c46493993d 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1485,6 +1485,35 @@ needs its own evidence-based design pass (per this org's convergence convention precedent, refinement from telemetry, never from inspection alone) before a specific number or mechanism is chosen. +**Decision (same pass): both #1454 and #1455 accepted as known, tracked residual risks — not blocking +PR #1452.** This design is a genuine, verified improvement over the status quo it replaces (no diagnostic +retry at all, the 120s-timeout bug reproducing repeatedly); it does not need to close every residual +failure mode to be worth merging. #1454's risk is partially mitigated today by `TaskOrchestrator`'s +existing per-request failover/circuit-breaker. #1455's failure mode requires two unlikely conditions to +coincide in one run (discovery near its own worst case *and* probing separately needing close to its full +escalation budget) — a tail case, not the common path. Both stay open, decision and reasoning recorded on +the issues themselves, cross-referenced from the ADR's Consequences section and both source files. + +**A third Devin Review pass found 2 more real, fixable issues (both fixed), narrower than the prior two +rounds — a good convergence signal.** An escalated-attempt HTTP rejection (401 auth, 429 throttle, 5xx +server error) was unconditionally labeled `escalated_probe_rejected`, over-claiming that any such status +was evidence the token budget specifically was too large — none of those statuses is budget evidence, and +this codebase deliberately never captures raw provider error text that could validate the distinction. +Fixed by extracting a shared `_record_provider_exception` helper so the escalated attempt gets the exact +same sanitized classification the base probe already used for any exception; the ADR's own text (which +originated this over-claim) is corrected in place, with parametrized 401/429/5xx/503 test coverage added. +Separately, `finish_reason`/`reasoning_without_content` were populated only on failure/escalation +outcomes, never on an ordinary successful probe (the single most common outcome) — despite the entire +point of adding this telemetry being "future tuning can be evidence-driven." Fixed in both the launcher +and the sidecar script's successful-gateway-evidence writer, so a real "normal" baseline now exists to +compare against. Two lower-priority items from the same pass were consciously left as-is: the fake-curl +test harness doesn't model a real curl partial-write-on-failure edge case (a test-fidelity gap, not a +production bug); and the attempt-limit guard's 9999 digit-count cap is looser than the design's intended +single-digit range but not exploitable today (workflows use the default) — tightening it to a specific +smaller number without real evidence would itself be exactly the kind of unjustified guess this org's +own convergence convention exists to prevent. 1920 tests pass; 100% coverage and 100% docstring coverage +on `scripts/ci/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 2f85720602..4d84d6b60d 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -262,6 +262,33 @@ def _response_finish_reason(response: object) -> str | None: return finish_reason +def _record_provider_exception(row: dict[str, object], exc: Exception) -> None: + """Record one sanitized, bounded classification of a provider exception. + + Never overclaims a specific root cause from an HTTP status alone: an + auth failure (401), rate limit (429), or server error (5xx) is not + evidence of a token-budget problem, and this codebase has no validated + signal today (evidence deliberately never carries raw provider error + text) that distinguishes a genuinely budget-specific rejection from any + other non-2xx response -- so this records the exception's own sanitized + type name (or a bounded placeholder when that name is unsafe to log) + plus an optional numeric HTTP status, identically regardless of which + probe attempt (base or escalated) raised it. Mutates ``row`` in place. + + Args: + row: The in-progress per-route evidence row to update. + exc: The exception a probe attempt raised. + """ + row["status"] = "rejected" + error_type = type(exc).__name__ + row["error_type"] = ( + error_type if error_type.isidentifier() and len(error_type) <= 64 else "provider_error" + ) + http_status = _safe_http_status(exc) + if http_status is not None: + row["http_status"] = http_status + + def _response_has_reasoning_without_content(response: object) -> bool: """Return whether a response matches the vendored "reasoning, no content" signature. @@ -314,23 +341,25 @@ def _preflight_review_agents( non-2xx, or empty content matching neither signature) is not retried: a genuinely-down candidate never reaches the escalation path, so it cannot produce a false "healthy" read. - A non-2xx rejection specifically on the escalated attempt is recorded as - ``escalated_probe_rejected`` -- distinguishable evidence the escalated - budget itself exceeds that one candidate's real ceiling, genuinely - attributable since the candidate object is pinned throughout -- and is - not retried further. An escalated-attempt exception with no HTTP status - (a transport failure, e.g. a timeout) is never labeled this way: that - would falsely attribute a connectivity failure to the token budget, so it - instead records the sanitized exception type, same as the base probe. + An exception on the escalated attempt (transport failure, auth failure, + rate limit, server error, or a genuine budget rejection) is recorded via + ``_record_provider_exception`` -- the SAME sanitized classification the + base probe uses, regardless of attempt. An HTTP status alone does not + distinguish "this candidate's real ceiling is below the escalated + budget" from any other cause (401/429/5xx are not budget evidence); this + codebase has no validated signal today that does, so it does not invent + one via an over-specific label. The report deliberately records only stable route identity, a bounded exception class name, an optional numeric HTTP status, attempt count, and a bounded ``finish_reason``. Provider response bodies, exception messages, URLs, prompts, and credentials are never copied into evidence. - ``finish_reason`` and ``reasoning_without_content`` always describe the - same, most recent attempt for a route (the base attempt when only one was - made; the escalated attempt when a second was made) -- never a mix of the - two attempts' state. + ``finish_reason`` and ``reasoning_without_content`` are populated on + every outcome -- success included, not just failure/escalation, so + future tuning has a real "normal" baseline to compare against -- and + always describe the same, most recent attempt for a route (the base + attempt when only one was made; the escalated attempt when a second was + made) -- never a mix of the two attempts' state. Args: agents: Selected zero-cost model agents. @@ -370,14 +399,7 @@ def _preflight_review_agents( try: response = client.proxy_send_once(agent, "chat/completions", base_payload) except Exception as exc: # noqa: BLE001 - sanitize at the provider boundary - row["status"] = "rejected" - error_type = type(exc).__name__ - row["error_type"] = ( - error_type if error_type.isidentifier() and len(error_type) <= 64 else "provider_error" - ) - http_status = _safe_http_status(exc) - if http_status is not None: - row["http_status"] = http_status + _record_provider_exception(row, exc) routes.append(row) continue if _chat_response_has_text(response): @@ -396,6 +418,12 @@ def _preflight_review_agents( # per-request failover/circuit-breaker, which this preflight # does not replace. row["status"] = "ready" + # Populated on every outcome, including this most-common, + # ordinary success path -- not just failure/escalation -- so + # future tuning has a real "normal" baseline to compare against, + # not just evidence of what went wrong. + row["finish_reason"] = _response_finish_reason(response) or "unknown" + row["reasoning_without_content"] = _response_has_reasoning_without_content(response) routes.append(row) viable.append(agent) continue @@ -420,24 +448,14 @@ def _preflight_review_agents( agent, "chat/completions", escalated_payload ) except Exception as exc: # noqa: BLE001 - sanitize at the provider boundary - row["status"] = "rejected" - http_status = _safe_http_status(exc) - if http_status is not None: - # Distinguishable evidence the *escalated* budget itself - # exceeds this candidate's real ceiling -- genuinely - # attributable, since the candidate object is pinned - # throughout. A transport failure (no HTTP status) is never - # labeled this way; it falls through to the same sanitized - # exception-type recording the base probe uses, below. - row["error_type"] = "escalated_probe_rejected" - row["http_status"] = http_status - else: - error_type = type(exc).__name__ - row["error_type"] = ( - error_type - if error_type.isidentifier() and len(error_type) <= 64 - else "provider_error" - ) + # An HTTP status alone (401 auth, 429 throttle, 5xx server + # error, ...) is not evidence the escalated *budget* specifically + # caused the rejection -- only that some request failed. Record + # the same sanitized classification the base probe uses, rather + # than the previous "escalated_probe_rejected" label, which + # over-claimed budget-specific attribution this codebase has no + # validated signal to actually support. + _record_provider_exception(row, exc) routes.append(row) continue if _chat_response_has_text(escalated_response): diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index e9e1ed1d78..0e0d7a5c14 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -621,12 +621,27 @@ try: first = choices[0] if isinstance(choices, list) and choices else None message = first.get("message") if isinstance(first, dict) else None content = message.get("content") if isinstance(message, dict) else None + # Bounded to a short, stable enum token (never raw provider text), and + # computed once so both the success and rejected outcomes below record + # the SAME evidence shape -- populated on success too (not just + # failure), so future tuning has a real "normal" baseline to compare + # against, not just evidence of what went wrong. + finish_reason = first.get("finish_reason") if isinstance(first, dict) else None + if not isinstance(finish_reason, str) or not finish_reason: + finish_reason = None + elif len(finish_reason) > 32 or not all( + character.isalnum() or character == "_" for character in finish_reason + ): + finish_reason = "unknown" + reasoning_without_content = isinstance(message, dict) and bool(message.get("reasoning")) if isinstance(content, str) and content.strip(): report = json.loads(report_path.read_text(encoding="utf-8")) report["gateway"] = { "endpoint": "chat/completions", "status": "ready", "attempts": attempts, + "finish_reason": finish_reason or "unknown", + "reasoning_without_content": reasoning_without_content, } temporary = report_path.with_suffix(".tmp") temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") @@ -636,14 +651,6 @@ try: # comment above the curl loop): record which budget-too-small signature, # if any, matched -- for diagnosis only, since this response is a # terminal outcome here regardless of which one it is. - finish_reason = first.get("finish_reason") if isinstance(first, dict) else None - if not isinstance(finish_reason, str) or not finish_reason: - finish_reason = None - elif len(finish_reason) > 32 or not all( - character.isalnum() or character == "_" for character in finish_reason - ): - finish_reason = "unknown" - reasoning_without_content = isinstance(message, dict) and bool(message.get("reasoning")) report = json.loads(report_path.read_text(encoding="utf-8")) report["gateway"] = { "endpoint": "chat/completions", diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 8971d2c39f..b37b24794b 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -197,6 +197,17 @@ def test_preflight_mirrors_runtime_request_and_keeps_only_compatible_routes() -> assert report["routes"][1]["error_type"] == "invalid_chat_response" assert secret not in repr(report) + # Regression for Devin Review's successful-probes-omit-diagnostics + # finding: the ordinary, most-common outcome (an immediate base-probe + # success, no escalation needed) must still populate finish_reason and + # reasoning_without_content -- not just failure/escalation outcomes -- + # so there is a real "normal" baseline to compare future telemetry + # against. + ready_row = report["routes"][2] + assert ready_row["status"] == "ready" + assert ready_row["finish_reason"] == "unknown" + assert ready_row["reasoning_without_content"] is False + for agent, endpoint, payload in client.calls: assert endpoint == "chat/completions" assert payload["model"] == agent.model @@ -505,8 +516,16 @@ def test_gateway_retry_loop_accepts_the_maximum_allowed_attempt_limit(tmp_path: def test_gateway_retry_loop_succeeds_on_the_first_attempt(tmp_path: Path) -> None: - """A clean 200 on the very first curl call needs no retry at all.""" - success_body = json.dumps({"choices": [{"message": {"content": "OK"}}]}) + """A clean 200 on the very first curl call needs no retry at all. + + Also covers Devin Review's successful-probes-omit-diagnostics finding: + ``finish_reason``/``reasoning_without_content`` must be populated on + success too, not just on rejection -- so a real "normal" response is + recorded here, not just left absent. + """ + success_body = json.dumps( + {"choices": [{"finish_reason": "stop", "message": {"content": "OK"}}]} + ) result, report = _run_gateway_retry_loop( tmp_path, max_attempts=3, plan=[f"200\n{success_body}"] ) @@ -517,6 +536,8 @@ def test_gateway_retry_loop_succeeds_on_the_first_attempt(tmp_path: Path) -> Non "endpoint": "chat/completions", "status": "ready", "attempts": 1, + "finish_reason": "stop", + "reasoning_without_content": False, } @@ -527,7 +548,9 @@ def test_gateway_retry_loop_recovers_from_one_transport_failure(tmp_path: Path) (job 99253418179): a curl timeout with no response used to abort the sidecar outright with no recovery path at all. """ - success_body = json.dumps({"choices": [{"message": {"content": "OK"}}]}) + success_body = json.dumps( + {"choices": [{"finish_reason": "stop", "message": {"content": "OK"}}]} + ) result, report = _run_gateway_retry_loop( tmp_path, max_attempts=3, plan=["FAIL", f"200\n{success_body}"] ) @@ -539,6 +562,8 @@ def test_gateway_retry_loop_recovers_from_one_transport_failure(tmp_path: Path) "endpoint": "chat/completions", "status": "ready", "attempts": 2, + "finish_reason": "stop", + "reasoning_without_content": False, } @@ -779,47 +804,56 @@ def test_escalation_budget_is_shared_and_bounded_across_candidates() -> None: assert len(client.calls) == max_escalations * 2 + 1 -def test_escalated_probe_rejection_is_recorded_distinctly_and_not_retried() -> None: - """A non-2xx rejection specifically on the escalated attempt is - distinguishable evidence the escalated budget exceeds that candidate's - real ceiling -- recorded as ``escalated_probe_rejected``, not conflated - with a generic empty-content rejection, and not retried again. +@pytest.mark.parametrize( + ("http_status", "exception_type_name"), + [ + (401, "_UnauthorizedError"), + (429, "_ThrottledError"), + (500, "_ServerError"), + (503, "_UnavailableError"), + ], +) +def test_escalated_probe_http_rejection_never_overclaims_budget_attribution( + http_status: int, exception_type_name: str +) -> None: + """Regression for Devin Review's HTTP-failures-receive-false-diagnosis + finding: an escalated-attempt HTTP rejection previously became the + blanket ``escalated_probe_rejected`` label for ANY status code, wrongly + implying every one of these (auth failure, rate limit, server error) was + evidence the token budget specifically was too large. None of these + statuses is budget evidence -- only that some request failed. The + escalated attempt now gets the exact same sanitized classification the + base probe already uses for any exception, with no special budget- + specific label invented from a status code alone. """ namespace = _load_launcher() preflight = namespace["_preflight_review_agents"] - low_ceiling = SimpleNamespace( + exception_type = type(exception_type_name, (RuntimeError,), {"code": http_status}) + flaky = SimpleNamespace( id="nvidia_nim_low_ceiling", provider_name="nvidia_nim", model="low/free" ) - - class _HttpError(RuntimeError): - """A synthetic exception carrying an HTTP status, like a real client's.""" - - code = 429 - client = _SequencedClient( [ {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, - _HttpError("max_tokens exceeds this model's ceiling"), + exception_type("provider rejected the request"), ] ) with pytest.raises(namespace["ReviewPreflightError"]) as failure: - preflight([low_ceiling], client=client) + preflight([flaky], client=client) assert len(client.calls) == 2 row = failure.value.report["routes"][0] - assert row["error_type"] == "escalated_probe_rejected" - assert row["http_status"] == 429 + assert row["error_type"] == exception_type_name + assert row["http_status"] == http_status assert row["attempts"] == 2 def test_escalated_probe_transport_failure_is_not_mislabeled_as_a_rejection() -> None: """A transport failure (no HTTP status at all) on the escalated attempt - must never become ``escalated_probe_rejected`` -- that label is reserved - for a genuine provider rejection of the escalated budget specifically. - A bare connection timeout is a different root cause and gets the same - sanitized exception-type recording the base probe uses. + gets the same sanitized exception-type recording the base probe uses -- + no HTTP status means even less basis for any budget-specific label. """ namespace = _load_launcher() preflight = namespace["_preflight_review_agents"] From 3b83b5005a01bc6ae86df38c6ac7c65e8d8b037a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 13:43:27 +0000 Subject: [PATCH 10/12] fix(sidecar): resolve fourth Devin Review pass on PR #1452 Three more findings, the same bug classes recurring in narrower spots the prior three rounds hadn't covered: An escalated attempt's exception handler (_record_provider_exception, shared by both probe attempts since the round-3 fix) left the base attempt's stale finish_reason/reasoning_without_content on the row when the ESCALATED attempt raised an exception -- the identical mixed-attempt-telemetry bug already fixed for the escalated-empty and escalated-success outcomes, not yet covered for escalated-exception. Fixed by clearing (not backfilling) both fields on any exception, since there is no response object for that attempt to describe. _response_has_reasoning_without_content checked only whether message.reasoning was truthy, never whether message.content was actually empty/absent -- so a normal, complete answer that also discloses a reasoning trace alongside real content would be wrongly recorded as "starved." Latent-but-harmless while only ever called on already-known-empty responses; the round-3 fix that started calling it on the SUCCESS path first exposed it as an active bug. Fixed by requiring content be genuinely absent, reusing _chat_response_has_text's own definition so the two predicates are provably consistent. Same bug, same fix, in the sidecar script's mirrored Layer 2 logic. A malformed/unparseable HTTP-200 gateway response body (or a missing response file) hit the bare except (...): pass fallback and wrote nothing to the gateway evidence report -- the same evidence-loss pattern as the earlier transport-exhaustion fix, a different trigger. Fixed with a bounded gateway_invalid_response classification via the same atomic-write pattern used everywhere else. Extended the fake-curl harness with a NOFILE: plan marker and malformed-JSON-body coverage. Two doc/test-staleness cleanups: a test docstring still described the routing probe as proving every route at the real 4096-token budget, no longer true since most routes now prove readiness at the cheaper 16-token base probe -- corrected without changing the test's own still-valid assertion. ADR-0005 updated from Status: proposed to accepted (matching this repo's other ADRs), with an explicit note that acceptance is the design decision, not a merge authorization, and its Consequences section's tense corrected to describe the shipped behavior now that this PR implements it. 1926 tests pass (1920 + 6 new/extended), 100% coverage and 100% docstring coverage on scripts/ci/, bash -n and all 4 embedded Python heredocs parse cleanly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 34 +++ .../0005-sidecar-preflight-token-budget.md | 48 ++-- docs/product-technical-gap-baseline.md | 37 +++ ...contextual_orchestrator_review_launcher.py | 40 ++- .../contextual_orchestrator_review_sidecar.sh | 33 ++- ...l_orchestrator_review_runtime_preflight.py | 266 ++++++++++++++++-- 6 files changed, 413 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 924ef19164..20ef07af89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,40 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix 3 more Devin Review findings from a fourth review pass on PR #1452 + (`scripts/ci/contextual_orchestrator_review_launcher.py`, + `scripts/ci/contextual_orchestrator_review_sidecar.sh`, + `tests/test_contextual_orchestrator_review_runtime_preflight.py`), plus two + doc/test-staleness cleanups: an escalated attempt's EXCEPTION handler + (`_record_provider_exception`) left the base attempt's stale + `finish_reason`/`reasoning_without_content` on the row -- the same + mixed-attempt-telemetry bug class already fixed for the escalated-empty + and escalated-success outcomes, now closed for the escalated-exception + outcome too (both fields are cleared, not backfilled, since there is no + response object to describe). `_response_has_reasoning_without_content` + checked only whether `message.reasoning` was truthy, never whether + `message.content` was actually empty/absent -- so a normal, complete + answer that also discloses a reasoning trace alongside real content would + be wrongly flagged as "starved" (this had gone latent-but-harmless while + the predicate was only ever called on already-known-empty responses; the + round-3 fix that started calling it on the SUCCESS path exposed the + actual bug for the first time). Fixed to require content be genuinely + absent, reusing `_chat_response_has_text`'s own definition so the two + predicates are provably consistent; same predicate fixed in the sidecar + script's mirrored Layer 2 logic. A malformed/unparseable HTTP-200 gateway + response body (or a missing response file) hit the bare + `except (...): pass` fallback and wrote nothing to the gateway evidence + report -- the same evidence-loss pattern as the earlier transport- + exhaustion fix, a different trigger -- now records a bounded + `gateway_invalid_response` classification via the same atomic-write + pattern. Extended the fake-curl harness with `NOFILE:` and + malformed-JSON-body plan entries to cover both. Also corrected a stale + test docstring (still described the routing probe as proving every route + at the real 4096-token budget, no longer true since most routes now prove + readiness at the cheaper 16-token base probe) and updated ADR-0005's + status from `proposed` to `accepted` with its Consequences section + reframed to present tense, now that this PR implements it. 1926 tests + pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. - Fix 2 more Devin Review findings from a third review pass on PR #1452 (`scripts/ci/contextual_orchestrator_review_launcher.py`, `scripts/ci/contextual_orchestrator_review_sidecar.sh`, diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index e93756de66..03a73666d1 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -1,6 +1,12 @@ # ADR-0005: Replace the sidecar's fixed-`max_tokens` gateway checks with diagnostic, bounded-retry readiness -- Status: proposed +- Status: accepted. Implemented in `ContextualWisdomLab/.github#1452`, stacked on this ADR's own PR + (`ContextualWisdomLab/.github#1449`); pending merge of both, governed separately (OpenCode review + approval required before merge, per this repo's governance model — this ADR's acceptance does not + itself authorize merging either PR). The implementation went through four rounds of Devin Review + scrutiny on PR #1452 after this ADR converged, each verified against actual code before fixing; two + findings were accepted as known, tracked, non-blocking residual risks rather than fixed + (`ContextualWisdomLab/.github#1454`, `#1455` — see Consequences). - Date: 2026-08-30 - Scope: `ContextualWisdomLab/.github` central review pipelines' vendored `contextual-orchestrator` sidecar — `scripts/ci/contextual_orchestrator_review_launcher.py`'s existing @@ -300,28 +306,32 @@ retried once, unconditionally, would be a real, computed worst-case blowup again ## Consequences -**This ADR is `proposed`; no code has shipped yet. The consequences below describe what the -implementation is expected to achieve once it lands, verified against this ADR's design — not an -outcome already observed in production.** - -- Once implemented, both preflight layers would become structurally tolerant of an individual attempt - being wrong for a fixed token budget, or hanging/failing transiently, which is the actual shape of - the problem — while keeping every worst case explicit and bounded rather than open-ended. -- Layer 1's worst case would grow from ~120s to a computed 160s, still under its existing 180s - healthz-readiness ceiling. Layer 2's worst case would grow from a single 120s attempt with no - recovery path to up to 360s across bounded retries — small relative to the job's 120-minute ceiling - and consistent with this file's own already-stated "accuracy over speed" policy. -- Keeping Layer 2 (not just Layer 1) would mean the preflight still proves the actual consumer-facing +**Implemented, in `ContextualWisdomLab/.github#1452` (stacked on this ADR's own PR #1449). The +consequences below describe the shipped behavior, verified against this ADR's design through four +rounds of Devin Review scrutiny on the implementation PR itself (each finding verified against actual +code before fixing) — not a hypothetical outcome. Both PRs remain pending merge, governed separately +(OpenCode review approval required); this ADR's `accepted` status is the design decision, not a merge +authorization.** + +- Both preflight layers are now structurally tolerant of an individual attempt being wrong for a fixed + token budget, or hanging/failing transiently, which is the actual shape of the problem — while keeping + every worst case explicit and bounded rather than open-ended. +- Layer 1's worst case grew from ~120s to a computed 160s (probing/escalation alone — see the known, + tracked gap on discovery's own time below), still under its existing 180s healthz-readiness ceiling. + Layer 2's worst case grew from a single 120s attempt with no recovery path to up to 360s across bounded + retries — small relative to the job's 120-minute ceiling and consistent with this file's own + already-stated "accuracy over speed" policy. +- Keeping Layer 2 (not just Layer 1) means the preflight still proves the actual consumer-facing `orchestrator/free` route works, not only that individual candidates can respond in isolation — closing the PR #1433 gap class rather than reopening it. Giving Layer 2 a bounded retry (rather than - either a single unconditional attempt or a shortened timeout) is what would actually address the live + either a single unconditional attempt or a shortened timeout) is what actually addresses the live 120s-hang reproduction on this ADR's own PR (job `99253418179`) — a shortened timeout alone would not have, and would have regressed the prior, already-evidenced 30s→120s fix in the same file. Whether it would have *prevented* that exact reproduction is not claimed with certainty (Layer 2's retry has no - verified route-diversity guarantee — see Decision §1); what it would change is that the check no - longer fails after one unconditional attempt with zero chance of recovery. -- A Layer 1 candidate whose escalated probe is rejected outright (rather than merely still empty) would - be recorded as not-ready with a distinct, honest reason rather than silently retried indefinitely or + verified route-diversity guarantee — see Decision §1); what it changes is that the check no longer + fails after one unconditional attempt with zero chance of recovery. +- A Layer 1 candidate whose escalated probe is rejected outright (rather than merely still empty) is + recorded as not-ready with a distinct, honest reason rather than silently retried indefinitely or misclassified — a known, accepted, documented residual limitation until `ContextualWisdomLab/contextual-orchestrator#927` lands. Layer 2's retry-diversity limitation (Decision §1) is accepted the same way, for the same reason: no verified mechanism exists today to @@ -349,7 +359,7 @@ outcome already observed in production.** picking a number from inspection alone (Context, "어떠한 휴리스틱과 Rule of thumbs도 금지"). Tracked as `ContextualWisdomLab/.github#1455`. - Items in Decision §4 are real `contextual-orchestrator` feature work, now tracked as real issues, and - would remain explicitly not closed by this ADR even once the sidecar-side implementation lands. + remain explicitly not closed by this ADR now that the sidecar-side implementation has landed. - No production routing default changes are proposed; this is scoped to the sidecar's own liveness checks. - **This is currently active, not theoretical**: the live reproduction in the Evidence trail below is diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index c46493993d..fab355aaa3 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1514,6 +1514,43 @@ smaller number without real evidence would itself be exactly the kind of unjusti own convergence convention exists to prevent. 1920 tests pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. +**A fourth Devin Review pass found 3 more real, fixable issues (all fixed) in narrower spots the prior +three rounds hadn't covered — the same bug classes recurring, not new ones, a strong convergence +signal.** An escalated attempt's exception handler (`_record_provider_exception`, shared by both probe +attempts since the round-3 fix) left the base attempt's stale `finish_reason`/`reasoning_without_content` +on the row when the ESCALATED attempt raised an exception — the identical mixed-attempt-telemetry bug +already fixed for the escalated-empty and escalated-success outcomes, just not yet covered for +escalated-exception. Fixed by clearing (not backfilling) both fields whenever an exception is recorded, +since there is no response object for that attempt to describe. Separately, and more consequentially: +`_response_has_reasoning_without_content` checked only whether `message.reasoning` was truthy, never +whether `message.content` was actually empty or absent — so a normal, complete answer that happens to +also disclose a reasoning trace alongside real content would be wrongly recorded as "starved." This bug +existed since the predicate was first written but was latent-and-harmless as long as it was only ever +called on responses `_chat_response_has_text` had already confirmed were empty; the round-3 fix that +started calling it on the SUCCESS path too was what first exposed it as an active telemetry-polluting bug +rather than a theoretical one. Fixed by requiring content be genuinely absent (reusing +`_chat_response_has_text`'s own definition so the two predicates are provably consistent, never duplicated +logic that could drift apart), with both a direct unit test of the predicate and an end-to-end test +proving a healthy reasoning+content response is never flagged; the same predicate bug existed identically +in the sidecar script's mirrored Layer 2 logic and is fixed there too. Third: a malformed/unparseable +HTTP-200 gateway response body (or a response file that was never written at all) hit the bare +`except (OSError, json.JSONDecodeError, IndexError, TypeError): pass` fallback and wrote nothing to the +gateway evidence report — the same evidence-loss pattern as the earlier transport-exhaustion fix, a +different trigger this time. Fixed with a bounded `gateway_invalid_response` classification via the same +atomic-write pattern already used everywhere else; the fake-curl test harness gained a `NOFILE:` +plan marker and malformed-JSON-body coverage for both triggers. + +Two doc/test-staleness items in the same pass: a test's own docstring still described the routing probe +as proving every route at the real `4096`-token budget, which stopped being true the moment ADR-0005's +base-probe design landed (most routes now prove readiness at the cheaper `16`-token base probe instead) — +corrected to describe current reality while leaving the test's own assertion (Layer 2's literal must +still equal `REVIEW_MAX_OUTPUT_TOKENS`) unchanged, since that part was never wrong. And ADR-0005 itself +still said `Status: proposed` and described its own design in future tense ("would become," "once it +lands") even though this very PR now implements it — updated to `accepted` (matching this repo's other +ADRs' convention) with an explicit note that acceptance is the design decision, not a merge authorization, +and the Consequences section's tense corrected to describe the shipped behavior. 1926 tests pass; 100% +coverage and 100% docstring coverage on `scripts/ci/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 4d84d6b60d..d8e6b05cb8 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -275,6 +275,15 @@ def _record_provider_exception(row: dict[str, object], exc: Exception) -> None: plus an optional numeric HTTP status, identically regardless of which probe attempt (base or escalated) raised it. Mutates ``row`` in place. + Also clears any ``finish_reason``/``reasoning_without_content`` already + on ``row`` from an EARLIER attempt on the same candidate (a no-op for + the base probe, which never set them yet, but essential for the + escalated probe: an exception here means there is no response object at + all for THIS attempt, so the base attempt's stale diagnostic fields must + not silently linger and look like they describe the outcome being + recorded now -- the same mixed-attempt-telemetry problem already fixed + for the escalated-empty and escalated-success outcomes, closed here too). + Args: row: The in-progress per-route evidence row to update. exc: The exception a probe attempt raised. @@ -287,6 +296,8 @@ def _record_provider_exception(row: dict[str, object], exc: Exception) -> None: http_status = _safe_http_status(exc) if http_status is not None: row["http_status"] = http_status + row.pop("finish_reason", None) + row.pop("reasoning_without_content", None) def _response_has_reasoning_without_content(response: object) -> bool: @@ -300,6 +311,18 @@ def _response_has_reasoning_without_content(response: object) -> bool: not verified as uniform across the pool), so ``finish_reason == "length"`` alone would miss the exact original failure mode this preflight exists to diagnose (PR #1436). + + True only when BOTH conditions hold: a populated ``message.reasoning`` + field, AND ``_chat_response_has_text`` is false for this SAME response. + A normal, complete answer that happens to also disclose a reasoning + trace alongside real, non-empty content is never "starved" -- checking + ``reasoning`` alone, with no check that content is actually + absent/empty, would wrongly flag a genuinely healthy response and + pollute this preflight's own evidence. Reusing + ``_chat_response_has_text``'s existing "empty or missing" definition, + rather than duplicating similar-but-subtly-different logic, keeps the + two predicates provably consistent: this one can never be true for a + response the other already accepts as having usable text. """ if not isinstance(response, dict): return False @@ -310,7 +333,9 @@ def _response_has_reasoning_without_content(response: object) -> bool: if not isinstance(first, dict): return False message = first.get("message") - return isinstance(message, dict) and bool(message.get("reasoning")) + if not isinstance(message, dict) or not message.get("reasoning"): + return False + return not _chat_response_has_text(response) def _preflight_review_agents( @@ -355,11 +380,14 @@ def _preflight_review_agents( a bounded ``finish_reason``. Provider response bodies, exception messages, URLs, prompts, and credentials are never copied into evidence. ``finish_reason`` and ``reasoning_without_content`` are populated on - every outcome -- success included, not just failure/escalation, so - future tuning has a real "normal" baseline to compare against -- and - always describe the same, most recent attempt for a route (the base - attempt when only one was made; the escalated attempt when a second was - made) -- never a mix of the two attempts' state. + every response-bearing outcome -- success included, not just + failure/escalation, so future tuning has a real "normal" baseline to + compare against -- and always describe the same, most recent attempt for + a route (the base attempt when only one was made; the escalated attempt + when a second was made) -- never a mix of the two attempts' state. When + the escalated attempt raises an exception instead of returning a + response, both fields are absent entirely (there is no response to + describe) rather than silently retaining the base attempt's values. Args: agents: Selected zero-cost model agents. diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index 0e0d7a5c14..e6fcb9cbe8 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -633,8 +633,16 @@ try: character.isalnum() or character == "_" for character in finish_reason ): finish_reason = "unknown" - reasoning_without_content = isinstance(message, dict) and bool(message.get("reasoning")) - if isinstance(content, str) and content.strip(): + has_text = isinstance(content, str) and bool(content.strip()) + # Requires BOTH a populated reasoning field AND no usable content -- + # never true for a normal, complete answer that also discloses a + # reasoning trace alongside real content. Checking `reasoning` alone + # (with no check that content is actually absent) would wrongly flag a + # genuinely healthy response and pollute this evidence. + reasoning_without_content = ( + isinstance(message, dict) and bool(message.get("reasoning")) and not has_text + ) + if has_text: report = json.loads(report_path.read_text(encoding="utf-8")) report["gateway"] = { "endpoint": "chat/completions", @@ -664,7 +672,26 @@ try: temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") temporary.replace(report_path) except (OSError, json.JSONDecodeError, IndexError, TypeError): - pass + # The response file was missing/unreadable, or its body was HTTP 200 + # but not the parseable JSON structure expected (malformed/truncated) -- + # a different failure than "valid JSON, empty content" above. Record a + # bounded classification before failing closed, using the same + # sanitize-then-atomic-replace pattern as every other gateway outcome, + # so this exact case does not leave zero evidence trail either. Never + # attempts to read or copy the unparseable body itself. + try: + report = json.loads(report_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + report = {} + report["gateway"] = { + "endpoint": "chat/completions", + "status": "rejected", + "error_type": "gateway_invalid_response", + "attempts": attempts, + } + temporary = report_path.with_suffix(".tmp") + temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") + temporary.replace(report_path) raise SystemExit(1) PY then diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index b37b24794b..8f51ba9c78 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -158,6 +158,53 @@ def test_log_discovery_errors_sentinel_matches_the_sidecar_scripts_constant() -> assert f'SIDECAR_DISCOVERY_DIAGNOSTICS_SENTINEL="{sentinel}"' in sidecar_text +def test_reasoning_without_content_requires_content_to_actually_be_absent() -> None: + """Regression for Devin Review's successful-replies-report-missing-content + finding: ``_response_has_reasoning_without_content`` previously checked + ONLY whether ``message.reasoning`` was truthy, never whether + ``message.content`` was actually empty/absent -- so a normal, complete + answer that also discloses a reasoning trace alongside real, non-empty + content would be wrongly flagged as "starved." Both conditions (populated + reasoning AND no usable content) must hold together. + """ + namespace = _load_launcher() + has_reasoning_without_content = namespace["_response_has_reasoning_without_content"] + + # The exact bug: reasoning present AND content present -- must be False. + assert ( + has_reasoning_without_content( + { + "choices": [ + { + "message": { + "reasoning": "the user asked X, so the answer is Y", + "content": "Y", + } + } + ] + } + ) + is False + ) + # Reasoning present, content genuinely empty string -- the real signature. + assert ( + has_reasoning_without_content( + {"choices": [{"message": {"reasoning": "still thinking", "content": ""}}]} + ) + is True + ) + # Reasoning present, content key entirely absent -- also the real signature. + assert ( + has_reasoning_without_content({"choices": [{"message": {"reasoning": "still thinking"}}]}) + is True + ) + # No reasoning at all -- never flagged regardless of content. + assert ( + has_reasoning_without_content({"choices": [{"message": {"content": "a normal reply"}}]}) + is False + ) + + def test_preflight_mirrors_runtime_request_and_keeps_only_compatible_routes() -> None: """Reject provider errors/malformed replies before the sidecar becomes ready.""" namespace = _load_launcher() @@ -222,21 +269,33 @@ def test_preflight_mirrors_runtime_request_and_keeps_only_compatible_routes() -> def test_gateway_preflight_max_tokens_is_synchronized_with_the_routing_probe() -> None: - """The bash script's end-to-end gateway check must not retest a route the - Python routing probe already proved ready with a stricter token budget. - - Regression for the 2026-08-30 sidecar-preflight-max-tokens incident: the - routing probe (`_preflight_review_agents`, tested above) already uses - `REVIEW_MAX_OUTPUT_TOKENS` and correctly marked a reasoning-capable - nvidia_nim route "ready". The separate end-to-end gateway check in - ``contextual_orchestrator_review_sidecar.sh`` used to hardcode + """The bash script's end-to-end gateway check must use the same real + serving budget the routing probe's ESCALATED attempt uses. + + Regression for the 2026-08-30 sidecar-preflight-max-tokens incident, + predating ADR-0005: back then the routing probe used a single fixed + `REVIEW_MAX_OUTPUT_TOKENS` for every attempt and correctly marked a + reasoning-capable nvidia_nim route "ready" at that budget, while the + separate end-to-end gateway check in + ``contextual_orchestrator_review_sidecar.sh`` hardcoded ``"max_tokens":16`` for that same virtual-model request -- far too small for a reasoning model to emit any answer content after its internal reasoning tokens, so the gateway rejected a route its own routing probe - had just proven healthy. This asserts the two budgets stay numerically - identical so that mismatch cannot silently return; it fails on the - pre-fix literal (16) and passes once the gateway request is synchronized - with the routing probe's budget. + had just proven healthy. + + Since ADR-0005 (this PR), most routes now prove readiness at the much + cheaper ``REVIEW_PREFLIGHT_BASE_TOKENS`` (16) instead -- `4096` is used + by the routing probe only on the ESCALATED retry (a candidate that + failed the cheap probe with a budget-too-small signature) and, always, + by the real serving `ModelClient` for actual review traffic (see + `ContextualWisdomLab/.github#1454` for the resulting known gap: an + ordinary base-probe success is never itself confirmed at this budget). + This test's own assertion is unaffected by that: Layer 2 never + escalates (ADR-0005 Decision SS1) and always uses the real serving + budget, so its literal must still equal `REVIEW_MAX_OUTPUT_TOKENS` + exactly, for the same reason as before -- a smaller Layer 2 budget can + still reject a route the routing probe (at either of its own budgets) + already proved ready. """ namespace = _load_launcher() review_max_output_tokens = namespace["REVIEW_MAX_OUTPUT_TOKENS"] @@ -324,11 +383,20 @@ def test_gateway_preflight_retries_transport_failures_up_to_a_bounded_attempt_co # consumes the next numbered plan file in $FAKE_CURL_PLAN_DIR (a fixed, # test-controlled queue of outcomes, one per expected attempt) so a test can # script an exact multi-attempt sequence -- transport failure, non-2xx, -# success -- without a real gateway process. A plan file's first line is -# either "FAIL" (curl exits non-zero, exactly like a real timeout with zero -# bytes) or an HTTP status code (written verbatim to stdout, mirroring -# `-w '%{http_code}'`); any remaining lines become the `-o` response body, -# exactly like a real curl would write one. +# success -- without a real gateway process. A plan file's first line is one +# of: +# "FAIL" -- curl exits non-zero, exactly like a real timeout with +# zero bytes received. +# "NOFILE:" -- curl "succeeds" (exits 0, prints ) but never +# writes the -o response file at all, exactly like a +# real curl invocation that got a status line but the +# transfer was interrupted before any body arrived. +# "" -- an HTTP status code (written verbatim to stdout, +# mirroring `-w '%{http_code}'`); any remaining plan +# lines become the -o response body, exactly like a +# real curl would write one (including deliberately +# malformed/non-JSON bodies, for a status-200-but- +# unparseable-body scenario). _FAKE_CURL_SCRIPT = """#!/usr/bin/env bash set -euo pipefail plan_dir="$FAKE_CURL_PLAN_DIR" @@ -356,6 +424,12 @@ def test_gateway_preflight_retries_transport_failures_up_to_a_bounded_attempt_co if [ "$status_line" = "FAIL" ]; then exit 28 fi +case "$status_line" in + NOFILE:*) + printf '%s' "${status_line#NOFILE:}" + exit 0 + ;; +esac if [ -n "$output_file" ]; then tail -n +2 "$plan_file" > "$output_file" fi @@ -677,6 +751,53 @@ def test_gateway_retry_loop_classifies_an_http_then_transport_exhaustion_by_the_ } +def test_gateway_retry_loop_records_evidence_for_a_malformed_200_response_body( + tmp_path: Path, +) -> None: + """Regression for Devin Review's malformed-gateway-replies-lose-evidence + finding: an HTTP 200 whose body is not parseable JSON at all (garbled or + truncated) used to hit the bare ``except (OSError, json.JSONDecodeError, + ...): pass`` fallback and write nothing to the gateway evidence report -- + the same evidence-loss pattern as transport exhaustion, a different + trigger. Must now record a bounded ``gateway_invalid_response`` + classification (attempt count, rejected status, no raw body copied) + before failing closed, via the same atomic-write pattern used elsewhere. + """ + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=1, plan=["200\nthis is not valid JSON {{{"] + ) + + assert result.returncode == 1 + assert "gateway preflight returned unusable chat content" in result.stderr + assert report["gateway"] == { + "endpoint": "chat/completions", + "status": "rejected", + "error_type": "gateway_invalid_response", + "attempts": 1, + } + + +def test_gateway_retry_loop_records_evidence_when_the_response_file_is_missing( + tmp_path: Path, +) -> None: + """The same regression as above, for the sibling trigger: curl reports a + 200 status but the response file itself was never written (a transfer + interrupted after the status line but before any body arrived). Reading + a missing file raises ``OSError``, caught by the same fallback -- must + also record evidence rather than leaving the report untouched. + """ + result, report = _run_gateway_retry_loop(tmp_path, max_attempts=1, plan=["NOFILE:200"]) + + assert result.returncode == 1 + assert "gateway preflight returned unusable chat content" in result.stderr + assert report["gateway"] == { + "endpoint": "chat/completions", + "status": "rejected", + "error_type": "gateway_invalid_response", + "attempts": 1, + } + + def test_reasoning_without_content_escalates_then_still_fails_closed_if_unresolved() -> None: """ADR-0005 round 5 (Devin Review): escalation must key off the vendored ``ModelClient._response_content``'s own "reasoning, no content" signature, @@ -723,6 +844,46 @@ def test_reasoning_without_content_escalates_then_still_fails_closed_if_unresolv assert failure.value.report["escalations_used"] == 1 +def test_base_probe_success_with_reasoning_and_content_is_never_flagged_as_starved() -> None: + """End-to-end regression for Devin Review's successful-replies-report- + missing-content finding: a genuinely healthy, complete first-attempt + response that ALSO discloses a reasoning trace alongside real content + must never be recorded as ``reasoning_without_content: True`` -- that + would falsely pollute the evidence this preflight exists to produce, on + the single most common outcome (an immediate base-probe success). + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + transparent_reasoner = SimpleNamespace( + id="openai_transparent_reasoner", provider_name="openai", model="reasoner/free" + ) + client = _ProbeClient( + { + transparent_reasoner.id: { + "choices": [ + { + "finish_reason": "stop", + "message": { + "reasoning": "the user asked for a greeting, so respond with one", + "content": "Hello!", + }, + } + ] + } + } + ) + + viable, report = preflight([transparent_reasoner], client=client) + + assert viable == [transparent_reasoner] + row = report["routes"][0] + assert row["status"] == "ready" + assert row["attempts"] == 1 + assert row["finish_reason"] == "stop" + assert row["reasoning_without_content"] is False + + def test_finish_reason_length_escalates_and_can_succeed() -> None: """The OpenAI-documented ``finish_reason == "length"`` signature also escalates, independent of the ``reasoning`` field, and a candidate that @@ -906,6 +1067,77 @@ def test_escalated_probe_transport_failure_sanitizes_an_unsafe_exception_name() assert "http_status" not in row +def test_escalated_probe_transport_exception_clears_stale_base_attempt_diagnostics() -> None: + """Regression for Devin Review's escalation-failures-retain-stale- + diagnostics finding: when the escalated attempt raises an exception (no + response object at all for that attempt), ``finish_reason`` and + ``reasoning_without_content`` must not silently keep the BASE attempt's + values -- the same mixed-attempt-telemetry bug class already fixed for + the escalated-empty and escalated-success outcomes, here closed for the + escalated-exception outcome too. This variant is a bare transport + failure (no HTTP status at all). + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + flaky = SimpleNamespace( + id="nvidia_nim_flaky_transport", provider_name="nvidia_nim", model="flaky/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + TimeoutError("connection timed out with zero bytes received"), + ] + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([flaky], client=client) + + row = failure.value.report["routes"][0] + assert row["attempts"] == 2 + assert row["error_type"] == "TimeoutError" + assert "http_status" not in row + # The base attempt's finish_reason=="length"/reasoning_without_content + # must not linger: there is no response for THIS (escalated) attempt to + # describe, so both fields are simply absent. + assert "finish_reason" not in row + assert "reasoning_without_content" not in row + + +def test_escalated_probe_http_exception_clears_stale_base_attempt_diagnostics() -> None: + """The same regression as above, for a genuine HTTP rejection (an HTTP + status is present) rather than a bare transport failure -- either way, + the base attempt's stale diagnostic fields must not survive. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + class _HttpError(RuntimeError): + """A synthetic exception carrying an HTTP status, like a real client's.""" + + code = 500 + + flaky = SimpleNamespace( + id="nvidia_nim_flaky_http", provider_name="nvidia_nim", model="flaky/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + _HttpError("provider rejected the request"), + ] + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([flaky], client=client) + + row = failure.value.report["routes"][0] + assert row["attempts"] == 2 + assert row["error_type"] == "_HttpError" + assert row["http_status"] == 500 + assert "finish_reason" not in row + assert "reasoning_without_content" not in row + + def test_escalated_empty_response_updates_both_telemetry_fields_together() -> None: """``finish_reason`` and ``reasoning_without_content`` must describe the SAME (final) attempt -- regression for Devin Review's mixed-attempt From 34d059c193ccd0b51cbbf00a4ab7a6f5197a8d7d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 13:51:30 +0000 Subject: [PATCH 11/12] fix(sidecar): close AttributeError gap in the round-4 evidence fix The round-4 malformed-gateway-reply fix caught (OSError, json.JSONDecodeError, IndexError, TypeError) around the gateway response parse, but json.loads() legally parses any top-level JSON value -- an array, null, a bare string, or a number, not only an object. The immediately following response.get("choices") assumes a dict and raises AttributeError for any of those shapes, which was not in the caught tuple. So a 200 response with a valid-but-wrong-shaped body (e.g. [] or null instead of {"choices": [...]}) still lost gateway evidence exactly like the bug round-4 set out to fix -- the script still failed closed overall (an uncaught exception exits non-zero), but wrote nothing to the report first. Fixed with an explicit isinstance(response, dict) check right after json.loads() that raises the already-caught TypeError, rather than widening the tuple to catch AttributeError broadly (which could mask unrelated bugs elsewhere in that block). Added parametrized regression tests ([], null, a bare string, a bare number), confirmed to fail against the pre-fix script (KeyError: 'gateway', the same signature as the original round-4 bug) before passing after the fix. 1930 tests pass (1926 + 4 new), 100% coverage and 100% docstring coverage on scripts/ci/, bash -n and all 4 embedded Python heredocs parse cleanly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 16 ++++++++++ docs/product-technical-gap-baseline.md | 16 ++++++++++ .../contextual_orchestrator_review_sidecar.sh | 10 +++++++ ...l_orchestrator_review_runtime_preflight.py | 29 +++++++++++++++++++ 4 files changed, 71 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ef07af89..83783bc159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,22 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix one more Devin Review finding on PR #1452, a genuine gap in the round-4 + malformed-gateway-reply fix (`scripts/ci/contextual_orchestrator_review_sidecar.sh`, + `tests/test_contextual_orchestrator_review_runtime_preflight.py`): + `json.loads()` legally parses a top-level JSON array, `null`, a bare + string, or a number, not just an object -- the immediately following + `response.get("choices")` assumes a dict and raises `AttributeError` for + any of those, which was not in the round-4 fix's caught exception tuple, + so a valid-JSON-but-wrong-shaped HTTP 200 body still lost evidence exactly + like the original bug (the script still failed closed overall, since an + uncaught exception exits non-zero, but wrote nothing to the gateway + evidence report). Fixed with an explicit `isinstance(response, dict)` + check that raises the already-caught `TypeError` rather than widening the + tuple to `AttributeError` broadly. Added parametrized regression tests + (`[]`, `null`, a bare string, and a bare number) confirmed to fail against + the pre-fix script before the fix, and pass after. 1930 tests pass; 100% + coverage and 100% docstring coverage on `scripts/ci/`. - Fix 3 more Devin Review findings from a fourth review pass on PR #1452 (`scripts/ci/contextual_orchestrator_review_launcher.py`, `scripts/ci/contextual_orchestrator_review_sidecar.sh`, diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index fab355aaa3..7cdc81edab 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1551,6 +1551,22 @@ ADRs' convention) with an explicit note that acceptance is the design decision, and the Consequences section's tense corrected to describe the shipped behavior. 1926 tests pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. +**A follow-up finding on the round-4 malformed-gateway-reply fix itself, caught before the round-4 push +even finished its own review cycle — a genuine gap, not a duplicate.** `json.loads()` legally parses any +top-level JSON value — an array, `null`, a bare string, or a number — not only an object. The very next +line, `response.get("choices")`, assumes a dict and raises `AttributeError` for any of those shapes, and +`AttributeError` was not in the round-4 fix's caught exception tuple `(OSError, json.JSONDecodeError, +IndexError, TypeError)`. So a `200` response whose body is valid-but-wrong-shaped JSON (e.g. `[]` or +`null` instead of `{"choices": [...]}`) still lost gateway evidence exactly like the bug round-4 set out +to fix — the script still failed closed overall (an uncaught exception exits the Python process non-zero, +so the shell's `if !` still caught it and called `fail`), but wrote nothing to the report first. Fixed +with an explicit `isinstance(response, dict)` check immediately after the `json.loads()` call that raises +the already-caught `TypeError` rather than widening the tuple to catch `AttributeError` broadly (which +could mask unrelated bugs elsewhere in that block). Parametrized regression tests (`[]`, `null`, a bare +string, a bare number) confirmed to fail against the pre-fix script (`KeyError: 'gateway'`, the same +signature as the original round-4 bug) before passing after the fix. 1930 tests pass; 100% coverage and +100% docstring coverage on `scripts/ci/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index e6fcb9cbe8..21b3778f63 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -617,6 +617,16 @@ report_path = Path(sys.argv[2]) attempts = int(sys.argv[3]) if sys.argv[3].isdecimal() else 0 try: response = json.loads(response_path.read_text(encoding="utf-8")) + if not isinstance(response, dict): + # Valid JSON, wrong top-level shape (e.g. `[]`, `null`, a bare + # string/number instead of an object) -- .get("choices") below + # assumes a dict and would otherwise raise AttributeError, which is + # not in the caught tuple below, losing evidence exactly like the + # unparseable-body case this except block exists to cover. Reuses + # the already-caught TypeError rather than widening the tuple to + # AttributeError broadly, which could mask unrelated bugs elsewhere + # in this block. + raise TypeError(f"gateway response was not a JSON object: {type(response).__name__}") choices = response.get("choices") first = choices[0] if isinstance(choices, list) and choices else None message = first.get("message") if isinstance(first, dict) else None diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 8f51ba9c78..942c2a005a 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -798,6 +798,35 @@ def test_gateway_retry_loop_records_evidence_when_the_response_file_is_missing( } +@pytest.mark.parametrize("wrong_shaped_body", ["[]", "null", '"just a string"', "42"]) +def test_gateway_retry_loop_records_evidence_for_a_valid_json_wrong_top_level_type( + tmp_path: Path, wrong_shaped_body: str +) -> None: + """Regression for a follow-up Devin Review finding on the malformed- + gateway-reply fix: ``json.loads`` legally parses a top-level JSON array, + ``null``, a bare string, or a number -- not just an object -- and + ``response.get("choices")`` assumes a dict, raising ``AttributeError`` + for any of these, which was NOT in the caught exception tuple. That + uncaught exception still failed the script closed overall (a non-zero + Python exit), but skipped writing evidence entirely -- the same + evidence-loss bug as the unparseable-JSON/missing-file cases, just for + a body that IS valid JSON with the wrong top-level shape. Must now + record the same bounded ``gateway_invalid_response`` classification. + """ + result, report = _run_gateway_retry_loop( + tmp_path, max_attempts=1, plan=[f"200\n{wrong_shaped_body}"] + ) + + assert result.returncode == 1 + assert "gateway preflight returned unusable chat content" in result.stderr + assert report["gateway"] == { + "endpoint": "chat/completions", + "status": "rejected", + "error_type": "gateway_invalid_response", + "attempts": 1, + } + + def test_reasoning_without_content_escalates_then_still_fails_closed_if_unresolved() -> None: """ADR-0005 round 5 (Devin Review): escalation must key off the vendored ``ModelClient._response_content``'s own "reasoning, no content" signature, From cabbe0c160a7acb4d534407e0e55e24fee5fbc1c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:49:20 +0000 Subject: [PATCH 12/12] docs(launcher): cross-reference #1458 at the escalation-budget rejection branch A fresh Devin Review pass on this PR's post-merge head (8dc6faae) found "Shared budget rejects healthy routes" at the escalations_used >= REVIEW_PREFLIGHT_MAX_ESCALATIONS branch in _preflight_review_agents -- the same underlying question already filed, reasoned through, and accepted as a known, tracked, non-blocking limitation on ADR-0005 (#1458), now surfacing against the actual code instead of the design doc it originated on. No redesign: a fixed-size escalation budget shared across a larger candidate pool always has to deny someone once claimed, catalog order is deterministic (not random) but not the thing actually at fault, and picking a specific reordering policy without real telemetry on which candidates need escalation more often would itself be the kind of unjustified heuristic this design rejects elsewhere. Added a code comment at the rejection branch cross-referencing #1458 with the same reasoning, matching how the sibling #1454/#1455 limitations are already cross-referenced at their own admission points in this same file. 1930 tests pass; 100% coverage and 100% docstring coverage on scripts/ci/. --- .../contextual_orchestrator_review_launcher.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index d8e6b05cb8..04e7cd847e 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -460,6 +460,23 @@ def _preflight_review_agents( reasoning_without_content = _response_has_reasoning_without_content(response) row["reasoning_without_content"] = reasoning_without_content budget_signature = finish_reason == "length" or reasoning_without_content + # KNOWN, ACCEPTED, TRACKED LIMITATION on the escalations_used >= + # REVIEW_PREFLIGHT_MAX_ESCALATIONS branch below, ContextualWisdomLab/.github#1458 + # (originally documented on ADR-0005, docs/adr/0005-sidecar-preflight-token-budget.md): + # escalations_used is one shared, first-come-first-served counter for + # the whole run, consumed in catalog order + # (build_zdr_prioritized_catalog's (cost_evidence_rank, + # zdr_attested_rank, provider, model) sort, not random). A + # later-sorting candidate can be denied its own escalation attempt + # purely because REVIEW_PREFLIGHT_MAX_ESCALATIONS earlier candidates + # already claimed the shared budget -- even if it would have been the + # only one to succeed at REVIEW_PREFLIGHT_ESCALATED_TOKENS. + # Deliberately not reordered (round-robin/random): a fixed-size + # shared budget smaller than the candidate pool always has to deny + # someone an escalation, so reordering only changes who, and picking + # a specific policy without real telemetry on which candidates + # actually need escalation would itself be the kind of unjustified + # heuristic this design rejects elsewhere. if not budget_signature or escalations_used >= REVIEW_PREFLIGHT_MAX_ESCALATIONS: row["status"] = "rejected" row["error_type"] = (