docs(adr): ADR-0005, replace fixed sidecar max_tokens with per-candidate readiness - #1449
Merged
seonghobae merged 12 commits intoAug 30, 2026
Merged
Conversation
…ate 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 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…feating 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 (8b3235d...) instead of bare line numbers that rot as files change. Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…oven 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 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 <noreply@anthropic.com>
…s-adr-20260830' into docs/sidecar-preflight-max-tokens-adr-20260830
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This was referenced Aug 30, 2026
A fifth Devin Review pass found Trigger B's own definition too narrow: the ADR text described escalation as firing only on choices[0].finish_reason == "length", but the vendored ModelClient._response_content treats EITHER that OR a populated message.reasoning field with no string content as the same "budget too small" signature -- already anticipated in the codebase's own error message. This second condition is not optional: it is the exact original failure mode PR #1436 responded to, and a finish_reason-only predicate misses it entirely, since a reasoning model can exhaust its budget under a different or absent finish_reason and provider finish_reason semantics for this case aren't verified as uniform across a pool this heterogeneous. A finish_reason-only Trigger B would silently misclassify a genuinely healthy reasoning-capable candidate as down -- the same false-negative class this ADR's Trigger A/B split already exists to prevent, just resurfacing one level deeper. Widened Trigger B to the two-part OR-condition consistently through Decision SS1 (the trigger definition itself, both layers' handling) and SS3 (the escalation predicate prose, the worst-case arithmetic, and the "every other outcome" fallback case), plus the implementation-telemetry requirement (both finish_reason and the reasoning-without-content signal must be emitted). Layer 2's "no retry on Trigger B" now explicitly covers both signatures, not only finish_reason, since the same "already recorded as successful by the gateway's routing" reasoning applies equally to either. Matches this ADR's own round-5 finding, which the stacked implementation PR (#1452) already handles correctly in code -- this brings the design doc back in sync with it. Updated CHANGELOG.md and docs/product-technical-gap-baseline.md's repeated summaries to match. Verified against actual current file content (not assumed) before editing. 1897 tests pass (unchanged, docs-only), including this branch's own test-plan scope (test_pr_governance_audit_contract.py, test_product_technical_gap_baseline.py, test_contextual_orchestrator_review_sidecar_contract.py, test_strix_contextual_orchestrator_contract.py, test_pingora_edge_policy.py -- 105 passed, 1 subtest passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… 2's 502 gap Two more findings from a sixth Devin Review pass, both verified directly against the vendored contextual-orchestrator source before acting: 1. Empty-string content precision. ModelClient._response_content checks isinstance(content, str) before ever inspecting reasoning, so a genuinely empty string "" (not missing/null) is treated as a valid, non-erroring return and never reaches the reasoning-without-content branch. Verified this is NOT an implementation bug: PR #1452's already-shipped _response_has_reasoning_without_content predicate independently treats content == "" the same as missing content (reusing _chat_response_has_text's own "empty or missing" definition), deliberately broader than _response_content's own narrower condition, and already escalates this case correctly. Fixed as a documentation-precision matter: Trigger B's definition now states explicitly that "no usable content" includes a genuinely empty string, with a precision note clarifying the _response_content citation is the motivating signature this preflight generalizes from, not a claim of exact behavioral equivalence. 2. Layer 2 502 misclassification -- a genuine scope gap, not a wording issue. server.py's except ProviderResponseError: handler is one blanket catch that doesn't even bind the exception, collapsing both of _response_content's distinct failure causes (reasoning-without-content vs. no-content-at-all) into an identical 502 invalid_structured_output body with no machine-readable distinguishing field. Layer 2's sidecar script therefore classifies this as Trigger A by elimination and retries it up to 3 times, rather than failing fast as the correctly- classified Trigger B. Verified this requires an out-of-scope contextual-orchestrator change to fix properly -- no in-repo workaround avoids fragile message-text matching, which this org's own no-heuristics convention already rejects elsewhere in this ADR. Documented as a known, accepted, tracked Layer 2 limitation (Decision Section 1 at the point of definition, Consequences, and Decision Section 4's upstream-tracking list) rather than worked around, filed as ContextualWisdomLab/contextual-orchestrator#932 following the existing #926/#927 pattern. Does not change Layer 2's stated 360s worst case (same shared Trigger-A attempt budget). Updated CHANGELOG.md and docs/product-technical-gap-baseline.md's repeated summaries to match, per Devin's own suggested fix scope. 1897 tests pass (unchanged, docs-only); this branch's own test-plan scope (105 passed, 1 subtest) re-verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Four findings, weighed against this org's convergence rule at 26+ review threads across seven rounds on a docs-only PR: 1. Trivial, fixed: Evidence trail's upstream-issue citation still named only #926/#927, missing #932 from the round just landed. 2. Cross-reference gap, not reopened: Layer 1's 160s worst-case claim (Decision Section 3) never referenced #1455 anywhere in this ADR's own text, even though #1455 (the discovery-timing gap) was filed and fully reasoned during the implementation pass on the stacked PR. Added the cross-reference at the point of definition and in Consequences; the underlying discovery-timing question itself stays tracked on #1455, not re-litigated here. 3. Genuinely new, verified real against the actual code (not just the ADR prose): REVIEW_PREFLIGHT_MAX_ESCALATIONS's shared budget is consumed in deterministic catalog order (alphabetical by provider/model, not random), so a later-sorting healthy candidate can be denied its own escalation attempt purely because 4 earlier candidates already claimed the shared budget. Considered a cheap reordering fix (round-robin, random shuffling) and rejected it on the merits: any selection policy for a fixed-size shared budget smaller than the candidate pool still has to deny someone a slot, so reordering only changes which candidates are favored, not whether the trade-off exists -- and picking a specific policy without real telemetry on which candidates actually need escalation more often would itself be exactly the unjustified heuristic this ADR already rejects elsewhere. Documented as a known, accepted, tracked limitation (#1458, matching the #1454/#1455/#932 pattern) rather than redesigned. 4. No action: the gap-baseline's repeated review-round narrative is this repo's own documented, intentional convention (docs/adr/0002-product-technical-gap-baseline.md: the baseline is "an operational snapshot" and "live PR metadata inventory," a distinct role from the ADR's design record and the CHANGELOG's terse pointers), not accidental redundancy. Updated CHANGELOG.md and docs/product-technical-gap-baseline.md to match. 1897 tests pass (unchanged, docs-only); this branch's own test-plan scope (105 passed, 1 subtest) re-verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
An eighth Devin Review pass found the #1458 (escalation-budget ordering-bias) writeup I added last round mischaracterized the actual sort key: I described catalog order as "alphabetical by (provider, model)", but build_zdr_prioritized_catalog (contextual_orchestrator_review_policy.py) actually sorts by (cost_evidence_rank, zdr_attested_rank, provider, model) -- cost tier first (constant within orchestrator/free), ZDR-attested status second (ZDR-attested candidates always sort first, regardless of require_zdr), and (provider, model) alphabetically only as the tie-breaker within each same-cost/same-ZDR-status group. Verified directly against the actual sort lambda before correcting; this matches an existing, unrelated gap-baseline entry's own precedent phrasing for the same function's sort key. Corrected the description everywhere it appeared: the ADR text (Decision SS3), CHANGELOG.md, docs/product-technical-gap-baseline.md, and the #1458 tracking issue body itself. The underlying finding and its accepted-limitation disposition are unchanged -- this is a factual-precision correction to the writeup, not a reopening of the design question. 1897 tests pass (unchanged, docs-only). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
seonghobae
added a commit
that referenced
this pull request
Aug 30, 2026
#1452) Bypass-merged under standing organizational authorization: this PR's own noema-review and Strix required checks are structurally blocked by the exact bug this PR fixes, since pull_request_target checks always execute main's current (pre-fix) contextual_orchestrator_review_sidecar.sh, never this PR's own diff — confirmed identically reproducing on three independent instances (this PR itself, ContextualWisdomLab/contextual-orchestrator#928's noema-review, and that same PR's Strix scan), ruling out anything specific to this diff. All 26 Devin Review threads across 9+ combined rounds (spanning #1449 and this PR) are resolved or non-actionable info; the final pass found 0 new issues. Full test suite: 1930 passed, 1 skipped, 100% coverage and 100% docstrings on scripts/ci/. Real quality/security gates (Trivy, CodeQL, Semgrep, gitleaks, osv-scan, Scorecard) are clean. Implements the design merged in #1449.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Direct owner critique, quoted verbatim in the ADR, after #1436's
max_tokens16→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). This PR addsdocs/adr/0005-sidecar-preflight-token-budget.md, a research-grounded design decision, plus pointer entries indocs/product-technical-gap-baseline.mdandCHANGELOG.md. No sidecar code change in this PR — the implementation is tracked as follow-up work (now inContextualWisdomLab/.github#1452, stacked on this branch).Update (this summary previously said the virtual-pool check would be replaced by per-candidate probing — corrected, per Devin Review, to match the ADR body): the final decision keeps both existing preflight layers, it replaces neither.
_preflight_review_agents/_preflight_with_fallback(per-candidate launcher probing) and the shell script's separate end-to-end request to the virtualorchestrator/freemodel each catch a failure class the other structurally cannot — confirmed live: PR #1433 showed per-candidate readiness pass while the virtual-pool request still 502'd, and this ADR's own PR (#1449) independently reproduced a 120s virtual-pool hang while per-candidate readiness had already passed in 30s.Checked directly against
ContextualWisdomLab/contextual-orchestratorsource (not assumed):ReasoningEffortProfileis real but additive (always still setsmax_tokens), and the public/v1/chat/completions//v1/responsesendpoints this preflight and Strix both use treat a caller-suppliedreasoning_effort/reasoningfield as a documented no-op.ModelClient.probe()/provider_readiness_report()) but isadmin-scoped while the sidecar's bearer token isinference-scoped — adopting it as-is would be a real privilege widening. Tracked asContextualWisdomLab/contextual-orchestrator#926.context_lengthvs.top_provider.max_completion_tokens). Tracked asContextualWisdomLab/contextual-orchestrator#927.Decision: two distinct, explicitly-bounded retry triggers — Trigger A (no usable response: timeout/connection failure/non-2xx) and Trigger B (a response was received, empty, and EITHER
finish_reason == "length"(OpenAI's documented "budget too small" signature) OR a populatedmessage.reasoningfield with no content — the vendoredModelClient._response_content's own broader signature for this exact failure, since providerfinish_reasonsemantics for a reasoning model exhausting its budget aren't verified as uniform across a pool this heterogeneous) — applied differently per layer, by structural necessity:16-token base probe to the existing4096-token budget, capped by a sharedREVIEW_PREFLIGHT_MAX_ESCALATIONS = 4across the whole run. Worst case 160s, under the existing 180s healthz-readiness ceiling.4096budget, up toREVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3. Trigger B is not retried at Layer 2 at all, for either signature — a Trigger-B response is still HTTP 200, so the gateway's own routing already recorded that attempt as successful, meaning a same-budget retry is more likely to repeat the same candidate than diversify away from it (Devin Review's 4th-round finding, verified directly — greppedcontextual_orchestrator/server.pyfor any candidate-exclusion parameter and found none). Per this org's convergence rule, this is accepted as a known, documented, bounded limitation rather than iterated further: Layer 1's genuine multi-candidate N-of-M is what provides real diversity/resilience in this design; Layer 2 remains a single end-to-end smoke test with a modest, honest safety margin against transient failure, not a pool-exploration mechanism.This ADR received five rounds of Devin Review scrutiny; all findings were verified against the actual text/source before acting (two were genuine design flaws — a fixed-budget probe reproducing the original bug, and a design that would have dropped Layer 2 entirely; a fifth found Trigger B's own definition too narrow to catch the exact original PR #1436 failure mode — not nits) and are reflected in the current text.
Test plan
Docs-only change.
python3 -m pytest tests/test_pr_governance_audit_contract.py tests/test_product_technical_gap_baseline.py tests/test_contextual_orchestrator_review_sidecar_contract.py tests/test_strix_contextual_orchestrator_contract.py tests/test_pingora_edge_policy.py -q— 105 passed, 1 subtest passedpython3 -m pytest tests -q) — 1897 passed, 1 skipped, 21 subtests passed🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Generated by Claude Code