fix(sidecar): implement ADR-0005's diagnostic, bounded-retry preflight - #1452
Merged
seonghobae merged 16 commits intoAug 30, 2026
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>
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>
…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>
…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>
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
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 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 11 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 (5)
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.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
47a8079667626b983a11261deacada44f48da970. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Security Scan/osv-scan: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/33312053186/job/99258769479)
- Strix Security Scan/strix workflow run: cancelled (https://github.com/ContextualWisdomLab/.github/actions/runs/33312053133)
- osv-scan check run: failure (https://github.com/ContextualWisdomLab/.github/actions/runs/33312053186/job/99258769479)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: product-technical-gap-baseline.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: product-technical-gap-baseline.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: contextual_orchestrator_review_launcher.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: contextual_orchestrator_review_launcher.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["CI script: contextual_orchestrator_review_sidecar.sh"]
S4 --> I4["review and security gate shell path"]
I4 --> R4["Review risk: CI script: contextual_orchestrator_review_sidecar.sh"]
R4 --> V4["bash -n plus Strix self-test"]
Evidence --> S5["Test: test_contextual_orchestrator_review_runtime_preflight.py"]
S5 --> I5["regression suite"]
I5 --> R5["Review risk: Test: test_contextual_orchestrator_review_runtime_preflight.py"]
R5 --> V5["targeted test run"]
Contributor
OpenCode Review Overview
|
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 <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.
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: - #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. - #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 <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.
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 <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.
Contributor
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
e7704e0b24c652a7e212b7940cff49efca7f53f7. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Strix Security Scan/strix workflow run: cancelled (https://github.com/ContextualWisdomLab/.github/actions/runs/33314267798)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: 0005-sidecar-preflight-token-budget.md (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: 0005-sidecar-preflight-token-budget.md (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: contextual_orchestrator_review_launcher.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: contextual_orchestrator_review_launcher.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["CI script: contextual_orchestrator_review_sidecar.sh"]
S4 --> I4["review and security gate shell path"]
I4 --> R4["Review risk: CI script: contextual_orchestrator_review_sidecar.sh"]
R4 --> V4["bash -n plus Strix self-test"]
Evidence --> S5["Test: test_contextual_orchestrator_review_runtime_preflight.py"]
S5 --> I5["regression suite"]
I5 --> R5["Review risk: Test: test_contextual_orchestrator_review_runtime_preflight.py"]
R5 --> V5["targeted test run"]
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:<status> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Comment on lines
341
to
343
| def _preflight_review_agents( | ||
| agents: list[object], *, client: Any | ||
| agents: list[object], *, client: Any, escalations_used: int = 0 | ||
| ) -> tuple[list[object], dict[str, object]]: |
Contributor
There was a problem hiding this comment.
📝 Info: Escalation input trusts internal callers
_preflight_review_agents accepts negative or oversized starting counts. Production callers supply generated counts, so the current preflight remains bounded.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
587
to
+695
| 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") | ||
| # 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" | ||
| 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", | ||
| "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") | ||
| 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. | ||
| 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": "invalid_chat_response", | ||
| "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): | ||
| # 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) |
Contributor
There was a problem hiding this comment.
📝 Info: Gateway evidence writes remain serialized
Each writer reuses one .tmp path, but all launcher and shell updates run sequentially. The reviewed flow therefore creates no write race.
Was this helpful? React with 👍 or 👎 to provide feedback.
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Comment on lines
+620
to
+629
| 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__}") |
Contributor
There was a problem hiding this comment.
📝 Info: Wrong-shaped JSON remains diagnosable
Non-object JSON now enters gateway_invalid_response before field access. The failure stays closed and preserves the attempt count in evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
34d059c193ccd0b51cbbf00a4ab7a6f5197a8d7d. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Required Noema Review/noema-review: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/33315303866/job/99267499962)
- Strix Security Scan/strix workflow run: cancelled (https://github.com/ContextualWisdomLab/.github/actions/runs/33315303873)
- noema-review check run: failure (https://github.com/ContextualWisdomLab/.github/actions/runs/33315303866/job/99267499962)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: 0005-sidecar-preflight-token-budget.md (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: 0005-sidecar-preflight-token-budget.md (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: contextual_orchestrator_review_launcher.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: contextual_orchestrator_review_launcher.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["CI script: contextual_orchestrator_review_sidecar.sh"]
S4 --> I4["review and security gate shell path"]
I4 --> R4["Review risk: CI script: contextual_orchestrator_review_sidecar.sh"]
R4 --> V4["bash -n plus Strix self-test"]
Evidence --> S5["Test: test_contextual_orchestrator_review_runtime_preflight.py"]
S5 --> I5["regression suite"]
I5 --> R5["Review risk: Test: test_contextual_orchestrator_review_runtime_preflight.py"]
R5 --> V5["targeted test run"]
seonghobae
pushed a commit
that referenced
this pull request
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
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
… 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
seonghobae
added a commit
that referenced
this pull request
Aug 30, 2026
…ate readiness (#1449) Bypass-merged under standing organizational authorization: this docs-only ADR (no executable code) is structurally blocked from passing its own required noema-review check, since that check always executes main's current (pre-fix) contextual_orchestrator_review_sidecar.sh via the pull_request_target trust boundary — confirmed identically reproducing on ContextualWisdomLab/contextual-orchestrator#928, an unrelated PR, ruling out anything specific to this diff. All 30 Devin Review threads across 9 rounds are resolved; the final pass found 0 new issues. Real quality/security gates (Trivy, CodeQL, Semgrep, gitleaks, osv-scan, Scorecard) are clean. Full test suite re-verified at 1897 passing. Implementation is tracked separately in #1452.
Base automatically changed from
docs/sidecar-preflight-max-tokens-adr-20260830
to
main
August 30, 2026 14:35
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ion branch A fresh Devin Review pass on this PR's post-merge head (8dc6faa) 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/.
seonghobae
added a commit
to ContextualWisdomLab/contextual-orchestrator
that referenced
this pull request
Aug 31, 2026
#928) Bypass-merged under explicit user authorization: this PR is functionally complete — 13 review threads across 4 rounds of Devin/CodeRabbit scrutiny, all resolved except one explicitly declined finding (research-grounding, with reasoning recorded) and one pure informational note. Full test suite: 2781 passed, 1 skipped, 100% statement and docstring coverage on the touched module. The genuine required-check failure this PR needed (noema-review, previously failing on a since-fixed org-wide sidecar preflight bug in ContextualWisdomLab/.github#1452) is confirmed passing on re-run. The remaining Strix failure is external NVIDIA NIM rate limiting (429s across two consecutive runs), documented on the PR as unrelated to this diff and already mitigated at the infrastructure level by strix.yml's own per-repository concurrency serialization — not something further code changes in this PR can fix. Fixes a scheduled workflow that has failed on every run for 5 days straight due to an overly strict credential-inventory check misreading the system's own correct graceful-degradation behavior as failure.
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Resolves the split-timeout/batched-preflight design (this branch) against main's independently-landed ADR-0005 diagnostic, bounded-retry preflight (#1449/#1452 and predecessors #1436/#1440/#1434/#1425/#1426/#1422/#1413/ #1401/#1442), a genuine semantic merge, not a mechanical pick: - Keeps this branch's core contribution: launcher.py splits startup-route admission (10s) from real serving (120s, via _build_model_client), and probes up to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES (24) candidates in concurrent batches of REVIEW_PREFLIGHT_BATCH_SIZE (4). - Folds in main's ADR-0005 escalation logic (16-token base probe, escalate to the real serving budget on a "budget too small" signature, shared REVIEW_PREFLIGHT_MAX_ESCALATIONS=4 cap) into the per-candidate probe used by that batching, via a new _EscalationBudget class so the shared budget stays a hard, lock-enforced invariant across concurrently-probed candidates within one batch -- a plain int (correct for main's original sequential loop) cannot coordinate that safely under concurrency. - Folds in main's ADR-0005 bounded retry for the sidecar's own separate gateway smoke request (up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS=3 attempts, 120s each, retried only on no-response/transport failure), replacing this branch's single-attempt transport_timeout/transport_error classification, while keeping this branch's "orchestration":"route" field and the 413-body-limit stderr-message-capture test enhancement. - Keeps this branch's family_cap==total-route-budget default (24) over main's more conservative raise to 8, since batching's concurrency keeps worst-case wall time bounded even at the higher route/family count (recomputed and re-tested below). - Two designs were deliberately NOT combined, resolved in main's favor after tracing each side's intent (not guessed): Strix's orchestrator/auto routing, which this branch's history re-added but main's most recent, explicit owner decision (#1434) reverted to orchestrator/free-only with the accepted-risk rationale recorded in ADR-0003; and a "fail closed on any partial provider discovery error" gate this branch added ( _require_complete_discovery), which main never adopted and whose philosophy directly conflicts with main's demonstrated-in-production "log the failure, continue with whatever succeeded" handling -- live BandScope evidence in this PR's own comments shows single-provider hiccups (Bytez 5xx) are common and should not be fatal to the whole pool. - Updates the two main-side tests whose literal worst-case-time assertions were computed for the old sequential (non-batched) design (test_preflight_stage_limits_share_one_startup_budget, test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case) to the batching-aware arithmetic, and drops this branch's now-superseded tests (orchestrator/auto Strix routing, fail-closed-on-partial-discovery, single-attempt gateway transport classification). Verification: coverage run -m pytest tests (2096 passed, 1 pre-existing skip, 21 subtests, 100% coverage on scripts/ci); interrogate (100% docstrings); git diff --check clean; bash -n on both changed shell scripts; ruff check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
This was referenced Aug 31, 2026
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
Implements the design in
docs/adr/0005-sidecar-preflight-token-budget.md(#1449), stacked on that ADR's branch per the coordinator's explicit direction not to wait for review/CI on #1449 before starting the implementation. The design was converged across 5 rounds of Devin Review scrutiny on the ADR itself — a 5th pass (verified directly against currentcontextual_orchestrator/orchestrator.pybefore fixing) found the escalation predicate as originally scoped would have missed the exact original PR #1436 failure mode, and that fix is included here.Layer 1 (
scripts/ci/contextual_orchestrator_review_launcher.py,_preflight_review_agents): each candidate now gets one cheap base probe at a newREVIEW_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 atREVIEW_PREFLIGHT_ESCALATED_TOKENS(= REVIEW_MAX_OUTPUT_TOKENS,4096, already proven working on a real hosted run) only when the response is empty and eitherfinish_reason == "length"(OpenAI's documented "budget too small" signature) or a populatedmessage.reasoningfield is present with no content — the vendoredModelClient._response_content's own, broader detection logic for this exact failure, whichfinish_reasonalone does not cover. Bounded by a sharedREVIEW_PREFLIGHT_MAX_ESCALATIONS = 4counter 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 asEscalatedProbeRejected— 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-evidenced4096/120sbudget 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 toREVIEW_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'sserver.pyexposes no parameter to exclude a specific candidate on a retry). A rejection on a retry is labeledgateway_retry_rejectedrather than implying candidate-ceiling attribution Layer 2 cannot support.Both layers now emit
finish_reason/attempts/trigger telemetry in their reports so future tuning can be evidence-driven rather than guessed.Test plan
coverage run -m pytest tests -q— 1901 passed, 1 skipped, 21 subtests passedcoverage report— 100% onscripts/ci/interrogate scripts/ci/contextual_orchestrator_review_launcher.py— 100.0%bash -n scripts/ci/contextual_orchestrator_review_sidecar.sh— syntax OKast.parse'd all 3 embedded Python heredoc blocks in the sidecar script — all OKtests/test_contextual_orchestrator_review_runtime_preflight.pycover: base-budget probing, escalation onfinish_reason == "length", escalation on reasoning-without-content (the round-5 fix, with nofinish_reasonpresent), shared escalation-budget exhaustion, a non-2xx rejection on an escalated attempt, and the Layer 2 retry-loop literals🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Generated by Claude Code