diff --git a/.github/source-fix-1629-no-heuristic-compute.trigger b/.github/source-fix-1629-no-heuristic-compute.trigger new file mode 100644 index 0000000000..7f7f26445d --- /dev/null +++ b/.github/source-fix-1629-no-heuristic-compute.trigger @@ -0,0 +1,3 @@ +trigger=2026-09-02T08:00:00Z +contract=fail-closed-provider-default-preflight +expected-head=ce8bc953141da7250c5bb7e44ea6ed5cfaf2929b diff --git a/.github/workflows/source-fix-1629-no-heuristic-compute.yml b/.github/workflows/source-fix-1629-no-heuristic-compute.yml new file mode 100644 index 0000000000..d4059cd0af --- /dev/null +++ b/.github/workflows/source-fix-1629-no-heuristic-compute.yml @@ -0,0 +1,91 @@ +name: Source fix PR1629 no-heuristic compute + +on: + push: + branches: + - fix/no-heuristic-review-admission-current-main + paths: + - .github/source-fix-1629-no-heuristic-compute.trigger + +jobs: + repair: + permissions: + contents: write + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + fetch-depth: 0 + ref: fix/no-heuristic-review-admission-current-main + persist-credentials: true + - uses: astral-sh/setup-uv@20cfd1bf945f4377ade1205e4dbc17946fc9a30d + with: + version: '0.12.5' + - name: Validate repair driver syntax + run: python -m py_compile scripts/source_fix_1629_no_heuristic_compute_v2.py + - name: Prove compute-allocation contract is RED before repair + shell: bash + run: | + set -euo pipefail + if uv run --group dev python -m pytest -q tests/test_contextual_orchestrator_review_no_heuristic_compute.py; then + echo '::error::no-heuristic compute regression was not RED before production repair' + exit 1 + fi + - name: Apply causal shared-owner repair + run: python scripts/source_fix_1629_no_heuristic_compute_v2.py + - name: Verify repaired central contracts + shell: bash + run: | + set -euo pipefail + uv run --group dev python -m pytest -q \ + tests/test_contextual_orchestrator_review_no_heuristic_compute.py \ + tests/test_contextual_orchestrator_review_runtime_preflight.py \ + tests/test_contextual_orchestrator_review_preflight_concurrency.py \ + tests/test_contextual_orchestrator_review_policy.py \ + tests/test_contextual_orchestrator_review_sidecar_contract.py + uv run --group dev ruff check \ + scripts/ci/contextual_orchestrator_review_launcher.py \ + tests/test_contextual_orchestrator_review_no_heuristic_compute.py \ + tests/test_contextual_orchestrator_review_runtime_preflight.py + bash -n scripts/ci/contextual_orchestrator_review_sidecar.sh + git diff --check + - name: Commit repair, reconcile current main, self-remove, and push + shell: bash + run: | + set -euo pipefail + rm -f \ + .github/workflows/source-fix-1629-no-heuristic-compute.yml \ + .github/source-fix-1629-no-heuristic-compute.trigger \ + scripts/source_fix_1629_no_heuristic_compute.py \ + scripts/source_fix_1629_no_heuristic_compute_v2.py + git add -A + git diff --cached --check + if git diff --cached --quiet; then + echo '::error::repair produced no tracked change' + exit 1 + fi + git config user.name 'opencode-agent[bot]' + git config user.email '219766164+opencode-agent[bot]@users.noreply.github.com' + git commit -m 'fix(review): remove heuristic inference allocation' + git fetch --no-tags origin main fix/no-heuristic-review-admission-current-main + remote_head="$(git rev-parse origin/fix/no-heuristic-review-admission-current-main)" + if [ "$remote_head" != "$(git rev-parse HEAD)" ] && ! git merge-base --is-ancestor "$remote_head" HEAD; then + git merge --no-edit "$remote_head" + fi + main_head="$(git rev-parse origin/main)" + if ! git merge-base --is-ancestor "$main_head" HEAD; then + git merge --no-edit "$main_head" + fi + uv run --group dev python -m pytest -q \ + tests/test_contextual_orchestrator_review_no_heuristic_compute.py \ + tests/test_contextual_orchestrator_review_runtime_preflight.py \ + tests/test_contextual_orchestrator_review_preflight_concurrency.py \ + tests/test_contextual_orchestrator_review_policy.py \ + tests/test_contextual_orchestrator_review_sidecar_contract.py + uv run --group dev ruff check \ + scripts/ci/contextual_orchestrator_review_launcher.py \ + tests/test_contextual_orchestrator_review_no_heuristic_compute.py \ + tests/test_contextual_orchestrator_review_runtime_preflight.py + bash -n scripts/ci/contextual_orchestrator_review_sidecar.sh + git diff --check + git push origin HEAD:fix/no-heuristic-review-admission-current-main diff --git a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md index 04dc04c7a2..bf91eee3b5 100644 --- a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md +++ b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md @@ -201,10 +201,12 @@ all five, and auto-optimize routing by cost. was drafted (in a now-superseded addendum proposing to gate the `free` decision on this evidence rather than making it directly) before the 2026-08-30 amendment above made the switch directly, without waiting for - that gate. The evidence itself remains useful regardless: it is exactly - the live signal for when "the free-catalog's stale-model and - provider-diversity gaps documented alongside this amendment" (above) are - closed, without requiring a manual re-audit. + that gate. The evidence itself remains useful as an account-level + diagnostic, but it is not an exact provider/outage-domain diversity signal: + multiple credentialed accounts can share one provider or outage domain. + Closing the reliability risk therefore requires separately modeled provider/ + outage-domain evidence rather than treating account cardinality as routing + authority or as proof that the earlier diversity gap is closed. `docs/doctoring/contextual-orchestrator-strix-free-diversity-evidence.md` records that PR's own reasoning trail. - **2026-08-31 amendment: Noema reviews independently of OpenCode.** Noema no diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index 3866281cfe..74b7eaf4f4 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -23,3 +23,19 @@ empty or truncated output, but they do not impose a wall-clock deadline. The former attempt counts, retry ceilings, and timeout values in this ADR are historical evidence only and must not be restored. + +## 2026-09-02 startup-latency amendment + +Admission evidence and runtime readiness are distinct. The central free-only +catalog retains every evidence-eligible route. Startup probes independent +provider-account lanes concurrently, while routes sharing one provider account +remain serialized to avoid a same-credential burst. Every route retains the +same per-route base/escalation semantics, and published evidence is restored to +deterministic input order, so one slow provider account cannot serialize +unrelated provider-account lanes. Concurrency changes no route membership, +priority, cost/ZDR decision, or provider preference; it only removes additive +startup latency across independent account lanes. The regression uses a +synchronization barrier across independent provider-account lanes rather than a +wall-clock threshold, proving those lanes can enter probing before either lane +is allowed to complete; it deliberately does not claim simultaneous probing of +routes that share one provider account. diff --git a/docs/doctoring/contextual-orchestrator-strix-free-diversity-evidence.md b/docs/doctoring/contextual-orchestrator-strix-free-diversity-evidence.md index c66923acda..58d6309937 100644 --- a/docs/doctoring/contextual-orchestrator-strix-free-diversity-evidence.md +++ b/docs/doctoring/contextual-orchestrator-strix-free-diversity-evidence.md @@ -1,81 +1,86 @@ -# Doctoring record: evidence-gated path toward `orchestrator/free` for Strix +# Doctoring record: Strix `orchestrator/free` reconciliation -- **Date:** 2026-08-30 -- **Subject:** The 2026-08-30 owner directive asks that Noema, OpenCode, and - Strix all route review through `contextual-orchestrator`'s `orchestrator/free` - pool. Noema and OpenCode already do (`docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md`). - Strix does not, and stays on `orchestrator/auto` today; this record explains - why the pin was not flipped on the strength of the instruction alone, and - what new evidence infrastructure exists so a future, properly reviewed change - can flip it safely. +- **Date:** 2026-09-01 +- **Status:** supersedes the 2026-08-30 diversity-gate proposal +- **Subject:** Noema, OpenCode, and Strix route required review through + `ContextualWisdomLab/contextual-orchestrator` using `orchestrator/free`. - **Decision record:** [`docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md`](../adr/0003-contextual-orchestrator-vendored-free-zdr.md) - (2026-08-30 addendum) -- **Related:** [`docs/product-goal-directive.md`](../product-goal-directive.md) §8 - and its Follow-up findings note; [`docs/doctoring/noema-orchestrator-free-zdr.md`](noema-orchestrator-free-zdr.md) -## Why this needed reconciliation, not a direct edit +## Superseded proposal -`docs/product-goal-directive.md` states its own conflict policy: "Where this -directive and those documents conflict, resolve the conflict and update -whichever document is wrong — do not silently pick one." Strix's -`orchestrator/auto` pin is not an oversight; it is an accepted ADR-0003 -decision backed by a specific, dated finding: on 2026-08-29, the DiskSage -exact-head scan showed every discovered free route sharing the OpenRouter -outage domain, so a strict `orchestrator/free` pin for Strix (which has no -provider fallback) would have gone dark on that one provider's outage. Silently -flipping the pin today, on the strength of a general instruction that does not -re-examine that finding, would reintroduce the exact single-point-of-failure -risk the ADR was written to avoid — for the workflow whose job is the org's -required *security* review. Silently keeping the old pin, on the other hand, -would ignore a legitimate cost/consistency goal the owner restated today. +The earlier version of this record correctly observed an outage-domain +concentration incident, but it proposed automatically switching Strix between +`orchestrator/free` and `orchestrator/auto` when a conceptual +`free_family_diversity >= 2` condition was met. That historical name referred +to outage-domain families; it was not, and is not, a runtime evidence field. +The current runtime emits `free_account_diversity`, which counts credential +accounts and is not a semantic substitute because multiple accounts can share +one outage domain. The historical cardinality threshold was not derived from a +reliability model, statistical estimand, authoritative standard, or +experimentally validated routing policy. It is therefore not a permitted +decision rule under the organization no-heuristics contract and must not be +implemented or revived. -## What changed +Current `free_account_diversity` evidence, and separately any explicitly modeled +provider/outage-domain observation, may remain diagnostic evidence. Diagnostics +do not acquire routing authority merely because they are deterministic or +measured. Any future reliability-aware model selection must identify its +estimand and be independently evaluated rather than turning an account count or +an outage-domain count into a routing threshold. -`scripts/ci/contextual_orchestrator_review_policy.py`'s -`build_zdr_prioritized_catalog` now reports `free_family_diversity`: the count -of distinct outage-domain provider families (`provider_family`; the primary -and secondary NVIDIA NIM keys already collapse into one family) among *all* -discovered free routes, independent of which `--pool` was requested. This is -new evidence, not a new decision — it is computed from the same discovery -report the catalog already validates, and it is present whether the caller -asked for `--pool free` or `--pool auto`. +## Current executable contract -`tests/test_contextual_orchestrator_review_policy.py` gained -`test_build_catalog_reports_free_family_diversity` (asserts diversity of 4 for -the existing five-provider fixture) and -`test_build_catalog_reports_single_family_free_concentration` (a regression -test reproducing the 2026-08-29 shape: two NVIDIA keys only, which collapse to -one family, so diversity is 1). Full suite: 1882 passed, 1 skipped; coverage -of the changed module remains 100% (`coverage run -m pytest tests` + -`coverage report --include=scripts/ci/contextual_orchestrator_review_policy.py`). +Protected-main evidence now records the actual Strix policy: -`.github/workflows/strix.yml` is unchanged in this PR. It still hard-pins -`CONTEXTUAL_ORCHESTRATOR_POOL: auto` and its `STRIX_MODEL`/`STRIX_LLM` gates -still reject anything except `orchestrator/auto`. +- `.github/workflows/strix.yml` accepts the contextual-orchestrator gateway and + restricts Strix model overrides to `orchestrator/free`; +- `tests/test_contextual_orchestrator_review_sidecar_contract.py` asserts + `CONTEXTUAL_ORCHESTRATOR_POOL: free`; +- `scripts/ci/strix_quick_gate.sh` and the required-workflow smoke contracts no + longer treat `orchestrator/auto` as an allowed Strix model route; +- Noema and Required OpenCode use the same `orchestrator/free` product boundary; +- private/internal review targets require the sidecar's ZDR policy rather than a + workflow-local model fallback. -## What has to happen before Strix can move to `orchestrator/free` +The five bootstrap credentials may all be supplied to contextual-orchestrator: +`BYTEZ_API_KEY`, `NVIDIA_NIM_API_KEY`, `NVIDIA_NIM_API_KEY_SUB`, +`OPENROUTER_API_KEY`, and `OPENAI_API_KEY`. Receiving, registering, or globally +discovering through `OPENAI_API_KEY` is not a defect. The invariant is the +`orchestrator/free` candidate-admission boundary: OpenAI-key-derived models are +not eligible for free-pool candidate generation, ranking, routing, serving, +failover, fallback, preflight, or durable free-pool persistence. The four +free-eligible credential sources still require their explicit zero-cost, +privacy, and capability evidence; a supplied credential does not fabricate an +eligible model. -A follow-up PR to `strix.yml` (or to -`scripts/ci/contextual_orchestrator_review_sidecar.sh`, whichever the -implementer finds is the correct evidence-read point) should read -`free_family_diversity` from the sidecar's `policy-report.json` after -discovery and select `orchestrator/free` only when it is `>= 2` — i.e. the -discovered free catalog spans at least two independent outage domains, so one -provider's outage cannot black out Strix's required review — and fall back to -`orchestrator/auto` otherwise. That PR was deliberately not bundled into this -one because `strix.yml` is a `pull_request_target` required workflow -(`docs/pr-review-and-merge-procedure.md`'s trust-boundary note: PRs that edit -trusted review workflows run the *base branch's* trusted scripts and can fail -their own checks until the base branch catches up) and its `STRIX_MODEL` -allowlist is a deliberate hardened gate, not an oversight to route around in -the same change that adds the evidence it would depend on. +## Admission versus routing -## Audit trail +The central review catalog is an admission boundary. It may enforce explicit +pool, zero-cost/price evidence, credential-source, capability, and ZDR +predicates, but it must not turn discovery into a provider quota, family quota, +candidate-count cap, cost/provider/name ordering, synthesized priority, or +first-come escalation preference. Every evidence-eligible route remains in the +catalog. Downstream selection requires identified routing evidence; if that +evidence is unavailable, the runtime fails closed. -- `docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md` — 2026-08-30 - addendum recording the decision and rationale. -- `docs/product-goal-directive.md` §8 and its Follow-up findings note — the - directive text and prior CodeRabbit reconciliation this addendum extends. -- `scripts/ci/contextual_orchestrator_review_policy.py`, - `tests/test_contextual_orchestrator_review_policy.py` — the evidence change - and its tests. +Startup readiness follows the same separation: complete admission evidence +remains durable, while all admitted routes are probed concurrently and reported +in catalog order. This removes additive provider latency without turning probe +completion order into routing authority. + +PR #1629 restores that contract on current protected-main lineage by removing +the reintroduced catalog cardinality/account caps, ranking, priority synthesis, +launcher route-count caps, and shared escalation quota while preserving the +free-only central-review pool. + +## Evidence trail + +- `.github/workflows/strix.yml` — executable Strix pool and override boundary. +- `tests/test_contextual_orchestrator_review_sidecar_contract.py` — executable + `orchestrator/free` sidecar contract. +- `docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md` — current and + historical pool decisions, including the later amendment superseding Strix + `orchestrator/auto`. +- `docs/product-technical-gap-baseline.md` — current implementation/gap ledger. +- `scripts/ci/contextual_orchestrator_review_policy.py` — admission evidence, + not a substantive model router. diff --git a/docs/doctoring/pr1629-admission-handoff-20260902.md b/docs/doctoring/pr1629-admission-handoff-20260902.md new file mode 100644 index 0000000000..529531ed63 --- /dev/null +++ b/docs/doctoring/pr1629-admission-handoff-20260902.md @@ -0,0 +1,11 @@ +# PR #1629 exact-head admission handoff + +The original one-shot review-admission repair completed its source mutation and focused regression on the writer branch, then published commit `56cf1db7a26dfe4d9a69687796ff8d31f0457270`. That earlier increment changed review startup from serial full-catalog preflight to concurrent readiness probing while preserving catalog-order evidence and route-local token-budget escalation. Fresh exact-head review then identified a second startup defect: per-route fan-out allowed every model sharing one provider credential to probe simultaneously, recreating the shared-key 429 storm already recorded in the product-technical baseline. The permanent regression now distinguishes admission cardinality from transport concurrency. All evidence-eligible routes remain admitted, but preflight execution is partitioned by the same provider-account identity used by `contextual_orchestrator_review_policy.provider_account`: independent provider accounts progress concurrently, routes sharing one account are probed serially, and outcomes are restored to original catalog order before any evidence or viable-route list is emitted. This introduces no fixed route cap, rank, shared escalation quota, or completion-order authority. + +A later external review demonstrated a distinct authority defect: the central launcher interpreted a transient failure classification as permission to allocate one extra identical model request through `ModelClient.proxy_send()`. Failure taxonomy proves the observed kind of failure but does not establish a repository-specific numeric retry budget. The writer branch therefore now uses `proxy_send_once()` for every semantic preflight payload, omits `transport_retry_budget`, and constructs both preflight and serving `ModelClient` instances with `max_retries=0`. Response-proven token starvation may still cause one second *different* payload with the established larger token budget; that semantic escalation is not a transport replay. + +The causal production repair was applied directly on the owner branch rather than waiting on the stalled source-fix workflow. The broader regression corpus was updated in the same history: transient 502 and permanent 401 failures each prove exactly one provider send, token-starvation escalation proves exactly two distinct one-shot payloads, and both runtime ModelClient constructors prove `timeout=None` plus `max_retries=0`. The obsolete `.github/source-fix-1629-preflight-retry.trigger`, `.github/workflows/source-fix-1629-preflight-retry.yml`, and `scripts/source_fix_1629_preflight_retry.py` identities were then deleted from the candidate tree so the repair machinery cannot consume future workflow capacity or become a durable mutation mechanism. + +The earlier source publication used the repository-scoped Actions token only because workflow-starting publication credentials were unavailable. A token-authored push is not accepted as successor-head admission evidence because GitHub suppresses normal workflow chaining in that case. The current direct owner commits create ordinary pull-request successor heads and their evidence must be collected afresh. Do not transfer checks or reviews from any predecessor source-fix head. + +At the current integration boundary the PR remains non-mergeable against an advanced protected `main`. That integration state is independent of the one-shot transport repair and must be reconciled non-destructively; no force push, destructive rebase, self-approval, gate weakening, or stale-head evidence transfer is authorized. Merge eligibility requires an unchanged reconciled head, no temporary source-fix identity, terminal ordinary checks/reviews, and no still-valid substantive review finding. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 1cc9e20313..2edcf84e8e 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2626,6 +2626,22 @@ Higgins, S. S., Crepalde, N., & Fernandes, L. (2021). Segmented multiplexity: A **Residual.** This closes the specific floating-image contribution from these three central workflows; it does not by itself guarantee the organization-wide Actions queue is fully drained, since other repositories' own workflows and any remaining unpinned central workflows may still request the floating image. Worth a follow-up sweep across the rest of `.github/workflows/` and sibling-repo workflows if queuing persists after this lands. +### 2026-09-02 Noema/OpenCode reviewer readiness false-negative regression + +External review on `.github#1629` demonstrated a real operational false negative: +full evidence admission had been coupled to sequential startup probing, so a large +set of individually slow provider routes could consume the review deadline before +Noema/OpenCode began serving. The repair keeps evidence admission complete, starts independent +provider-account lanes concurrently, serializes routes sharing one provider account, +preserves input-order/source evidence, and keeps each candidate's budget escalation +local. `tests/test_contextual_orchestrator_review_preflight_concurrency.py` is the +durable barrier-based regression: the old globally sequential implementation cannot +pass it, while the GREEN implementation proves every independent provider-account lane +can enter transport before either lane completes. It deliberately does not claim that +routes sharing one provider account start simultaneously. Explicit legacy `--limit`/`--account-cap` CLI configuration now +emits diagnostics while remaining decision-inert. The pinned contextual-orchestrator +ranking contract was also re-audited: `_static_rank_key` ends in `agent.id`, so equal +neutral priorities do not inherit discovery/list order as a routing tiebreak. ## 2026-09-02 GitHub Actions review sidecar pool pinned to `orchestrator/free`; `auto` removed as an accepted value **Problem.** `scripts/ci/contextual_orchestrator_review_sidecar.sh` — the script every central required review workflow (Strix, OpenCode Review, Noema Review, the PR-review autofix sidecar) provisions to talk to `contextual-orchestrator` — read an operator-settable `CONTEXTUAL_ORCHESTRATOR_POOL` environment variable, defaulted it to `free`, and validated it against exactly two accepted values: `free` or `auto` (`case "$orchestrator_pool" in free|auto) ...`). `auto` is a real, load-bearing value one layer down: `scripts/ci/contextual_orchestrator_review_launcher.py --pool auto` admits *priced* discovered routes as a fallback stage once the free pool is exhausted (`build_zdr_prioritized_catalog(..., pool="auto")`), by design, for callers that want that behavior. Nothing in this repository's own review-provisioning code path currently sets `CONTEXTUAL_ORCHESTRATOR_POOL=auto` — the only workflow that sets the variable at all, `strix.yml`, sets it to `free`; every other central review workflow simply relies on the script's own `:-free` default — so this was not a live incident, it was an unaudited, structurally-reachable escape hatch: a future edit to any of the four workflows above, or a manually-triggered `workflow_dispatch` with a custom env override, could set `CONTEXTUAL_ORCHESTRATOR_POOL=auto` and the sidecar would accept it silently, with no cost ceiling, no budget/authorization gate, and no reviewer visibility that priced models were now in scope for a required check. diff --git a/docs/superpowers/plans/2026-09-02-provider-preflight-resilience.md b/docs/superpowers/plans/2026-09-02-provider-preflight-resilience.md new file mode 100644 index 0000000000..855e8a5666 --- /dev/null +++ b/docs/superpowers/plans/2026-09-02-provider-preflight-resilience.md @@ -0,0 +1,72 @@ +# Provider-Neutral Preflight Resilience Implementation Plan + +> **For agentic workers:** Use `superpowers:executing-plans` or `superpowers:subagent-driven-development` when continuing this plan. + +**Goal:** Recover any eligible model route after a bounded transient transport failure, remove implicit inference deadlines for every model, and reserve reasoning-specific handling for capability or response evidence rather than model/provider names. + +**Incident:** DiagramWeave Actions run `33554858825`, job `100013111840` exposed an HTTP 502 on one discovered route and a long-running review path. The observed DeepSeek/NVIDIA NIM identity is incident evidence only; it is not a policy key. + +**Ownership:** + +- `ContextualWisdomLab/contextual-orchestrator#971` owns the generic `ModelClient` transport and inference-deadline contract. +- `ContextualWisdomLab/.github#1629` owns the central review launcher's preflight use of that contract. +- Merged central PR `#1546` already removed fixed Noema/OpenCode/sidecar inference deadlines. This plan does not replace that work or reintroduce per-model timeout tables. + +## Invariants + +- `ModelClient.timeout=None` means no hidden wall-clock inference deadline for any model. +- Explicit caller cancellation, stale-head cancellation, and workflow/job termination remain valid outer lifecycle controls. +- Connection establishment and other transport controls are independent configuration. They are not selected from model names or reasoning capability. +- HTTP 502, 503, 429, timeout, and connection failures are retried only when Contextual-Orchestrator classifies them as transient. +- HTTP 400, 401, 403 and other permanent failures remain terminal under the existing taxonomy. +- Preflight transport retry budget is exactly one recovery attempt and is recorded separately from semantic prompt attempts. +- No branch may inspect `model`, `agent_id`, `provider_name`, or `reasoning_effort_supported` to decide inference timeout or transport retry eligibility. +- Reasoning-specific token escalation is triggered by response evidence: `finish_reason == "length"` or populated reasoning with no usable content. It is independent of transport retry and does not require a model-name allowlist. +- Independent provider-account lanes may probe concurrently; routes sharing one provider account remain serialized. Published results return to catalog order. +- Completion timing, retry count, provider identity, and discovery order do not become routing or admission authority. +- Provider response bodies, prompts, exception messages, credentials, and internal topology are not persisted in preflight evidence. + +## Task 1: Generic transport regression + +**Files:** + +- `ContextualWisdomLab/contextual-orchestrator/tests/test_provider_gateway_resilience.py` +- `ContextualWisdomLab/.github/tests/test_contextual_orchestrator_review_transient_preflight.py` + +- [x] Parameterize the HTTP 502 recovery test across `reasoning_effort_supported = None, False, True`. +- [x] Use an arbitrary provider/model identity so the test fails if policy becomes model-name-dependent. +- [x] Prove HTTP 401 remains single-attempt and terminal. +- [x] Prove every review `ModelClient` constructor carries `timeout=None`, while only idempotent preflight receives the one-retry budget. + +## Task 2: Response-driven reasoning behavior + +**Files:** + +- `scripts/ci/contextual_orchestrator_review_launcher.py` +- `tests/test_contextual_orchestrator_review_transient_preflight.py` +- `tests/test_contextual_orchestrator_review_runtime_preflight.py` + +- [x] Keep the cheap base token budget for ordinary routes. +- [x] Escalate the same route once when its response reports length exhaustion or reasoning without visible content. +- [x] Add a regression using an arbitrary model name and unknown reasoning metadata; the response alone must trigger escalation from 16 to 4096 tokens. +- [x] Keep `attempts` as semantic payload attempts and `transport_retry_budget` as transport policy evidence. + +## Task 3: Exact-head verification + +Run on each unchanged final head: + +```bash +# contextual-orchestrator +python -m pytest -q tests/test_provider_gateway_resilience.py tests/test_provider_reliability.py + +# central review control plane +python -m pytest -q \ + tests/test_contextual_orchestrator_review_transient_preflight.py \ + tests/test_contextual_orchestrator_review_runtime_preflight.py \ + tests/test_contextual_orchestrator_review_preflight_concurrency.py \ + tests/test_contextual_orchestrator_review_sidecar_contract.py +``` + +Then require the normal repository CI, security, supply-chain and independent-review gates to reach terminal success on those exact heads. Queued, skipped, cancelled, predecessor-head or status-only results are not GREEN. + +After both owner changes are integrated into the refs consumed by the reusable workflow, rerun the unchanged DiagramWeave PR head. Close the incident only when the downstream Noema review produces terminal exact-head evidence. diff --git a/scripts/ci/agent_mention_router.py b/scripts/ci/agent_mention_router.py old mode 100755 new mode 100644 diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 2e56809639..95c72cc293 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -12,7 +12,7 @@ in-process (so the KV-backed credentials are visible to it), the zero-cost ("free") routes are collected into a report, and ``scripts/ci/contextual_orchestrator_review_policy.py`` turns that report into a -ZDR-prioritized, credential-account-diverse catalog for ``orchestrator/free``. +evidence-admitted catalog for ``orchestrator/free``; routing preference remains owned by the orchestrator's explicit evidence model. Keeping the decision logic in that stdlib-only module lets every branch of the ZDR policy be tested offline in this repository while ``orchestrator/free`` still resolves from authentically zero-priced models discovered by the @@ -22,6 +22,7 @@ from __future__ import annotations import argparse +from concurrent.futures import ThreadPoolExecutor import json import os import re @@ -42,8 +43,6 @@ # Provider-neutral sampling: several modern endpoints reject non-default # temperatures, while 1.0 is the OpenAI-compatible default. REVIEW_TEMPERATURE = 1.0 -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 @@ -59,9 +58,6 @@ # 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. It bounds request count, never model response time. -REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4 class ReviewPreflightError(RuntimeError): @@ -317,203 +313,166 @@ def _response_has_reasoning_without_content(response: object) -> bool: return not _chat_response_has_text(response) -def _preflight_review_agents( - 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. - - 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, 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. - 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`` are populated on - 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. +def _send_preflight_request( + client: Any, agent: object, payload: dict[str, object] +) -> object: + """Send exactly one provider request for one semantic preflight payload. - 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. + Retry quantity is not inferred from a transient failure classification. + ``proxy_send_once`` is the causal boundary: token-budget escalation may + create a second *different* payload only when response evidence proves + starvation, but transport failures never manufacture another identical + inference attempt in this launcher. + """ + return client.proxy_send_once(agent, "chat/completions", payload) - Returns: - 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. +def _preflight_review_agent( + agent: object, *, client: Any +) -> tuple[object | None, dict[str, object], int]: + """Probe one route with one-shot transport and evidence-driven token escalation. + + ``attempts`` counts distinct semantic payloads (base budget and, only when + evidenced, one larger token budget). Each payload is sent exactly once; + provider/HTTP failure taxonomy remains evidence only and does not allocate + a transport retry budget. """ - viable: list[object] = [] - routes: list[dict[str, object]] = [] - 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, - } - 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_PREFLIGHT_BASE_TOKENS, - "stream": False, - } - try: - response = client.proxy_send_once(agent, "chat/completions", base_payload) - except Exception as exc: # noqa: BLE001 - sanitize at the provider boundary - _record_provider_exception(row, exc) - 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" - # 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 - 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 - # 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"] = ( - "invalid_chat_response" if not budget_signature else "escalation_budget_exhausted" - ) - 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 - sanitize at the provider boundary - # 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): - 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 + 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, + } + 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_PREFLIGHT_BASE_TOKENS, + "stream": False, + } + try: + response = _send_preflight_request(client, agent, base_payload) + except Exception as exc: # noqa: BLE001 - sanitize at provider boundary + _record_provider_exception(row, exc) + return None, row, 0 + if _chat_response_has_text(response): + row["status"] = "ready" + row["finish_reason"] = _response_finish_reason(response) or "unknown" + row["reasoning_without_content"] = _response_has_reasoning_without_content(response) + return agent, row, 0 + + 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 + if finish_reason != "length" and not reasoning_without_content: row["status"] = "rejected" 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. + return None, row, 0 + + row["attempts"] = 2 + escalated_payload = dict(base_payload) + escalated_payload["max_tokens"] = REVIEW_PREFLIGHT_ESCALATED_TOKENS + try: + escalated_response = _send_preflight_request( + client, agent, escalated_payload + ) + except Exception as exc: # noqa: BLE001 - sanitize at provider boundary + _record_provider_exception(row, exc) + return None, row, 1 + if _chat_response_has_text(escalated_response): + row["status"] = "ready" + row["escalated"] = True row["finish_reason"] = _response_finish_reason(escalated_response) or "unknown" row["reasoning_without_content"] = _response_has_reasoning_without_content( escalated_response ) + return agent, row, 1 + + row["status"] = "rejected" + row["error_type"] = "invalid_chat_response" + row["finish_reason"] = _response_finish_reason(escalated_response) or "unknown" + row["reasoning_without_content"] = _response_has_reasoning_without_content( + escalated_response + ) + return None, row, 1 + + +def _preflight_review_agents( + agents: list[object], *, client: Any +) -> tuple[list[object], dict[str, object]]: + """Probe all admitted routes with provider-account bounded concurrency. + + Admission and readiness are separate contracts. Every evidence-eligible + route stays admitted; this stage only establishes immediate serving + readiness. Independently credentialed provider accounts progress in + parallel, while routes sharing the same current provider-account identity + are probed serially so one shared credential cannot receive an unbounded + simultaneous burst. The account identity is the same provider-name + boundary used by ``contextual_orchestrator_review_policy.provider_account``; + no fixed route cap, rank, quota, or provider preference is introduced. + + A route receives one larger-budget retry only when its own response carries + the explicit budget-starvation signature. Results are restored to original + catalog order before evidence or viable routes are emitted, so provider + completion timing cannot become routing authority. + """ + if not agents: + report: dict[str, object] = { + "contract": "strix-plain-chat-preflight-v2", + "probed_count": 0, + "ready_count": 0, + "rejected_count": 0, + "escalations_used": 0, + "routes": [], + } + raise ReviewPreflightError( + "no provider route passed the Strix plain-chat preflight", report + ) + + provider_lanes: dict[str, list[tuple[int, object]]] = {} + for index, agent in enumerate(agents): + provider_account = str(getattr(agent, "provider_name", "") or "unknown") + provider_lanes.setdefault(provider_account, []).append((index, agent)) + + def probe_lane( + lane: list[tuple[int, object]], + ) -> list[tuple[int, tuple[object | None, dict[str, object], int]]]: + return [ + (index, _preflight_review_agent(agent, client=client)) + for index, agent in lane + ] + + with ThreadPoolExecutor( + max_workers=len(provider_lanes), thread_name_prefix="review-preflight" + ) as executor: + futures = [executor.submit(probe_lane, lane) for lane in provider_lanes.values()] + indexed_outcomes = [ + indexed_outcome + for future in futures + for indexed_outcome in future.result() + ] + indexed_outcomes.sort(key=lambda item: item[0]) + outcomes = [outcome for _index, outcome in indexed_outcomes] + + viable: list[object] = [] + routes: list[dict[str, object]] = [] + escalations_used = 0 + for ready_agent, row, escalations in outcomes: routes.append(row) + escalations_used += escalations + if ready_agent is not None: + viable.append(ready_agent) - report: dict[str, object] = { + report = { "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: @@ -528,17 +487,12 @@ def _preflight_with_fallback( ) -> tuple[list[object], dict[str, object], bool]: """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). This bounds request count, not individual - model response or sidecar readiness time. 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. + Each candidate keeps the same two-attempt evidence contract used by + ``_preflight_review_agents``. A candidate that returns the explicit + budget-starvation signature receives exactly one larger-budget retry; + another candidate's earlier position cannot consume or deny that retry. + Primary and fallback reports preserve their own observed escalation counts + for audit without using those counts as admission authority. """ try: viable, report = _preflight_review_agents(primary_agents, client=client) @@ -546,10 +500,9 @@ def _preflight_with_fallback( 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, escalations_used=escalations_used + fallback_agents, client=client ) except ReviewPreflightError as fallback_error: fallback_error.report["primary_attempt"] = primary_error.report @@ -619,60 +572,6 @@ def _write_json(path: str, payload: object) -> None: ) -def _bounded_primary_catalog_limit( - requested_limit: int, *, pool: str, has_free_rows: bool -) -> int: - """Return the primary-stage route limit within one startup budget.""" - if requested_limit < 1: - raise ValueError("ORCHESTRATOR_CATALOG_LIMIT must be positive") - total_limit = min(requested_limit, REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES) - if pool == "auto" and has_free_rows: - return min(total_limit, REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT) - return total_limit - - -def _bounded_fallback_catalog_limit( - requested_limit: int, *, primary_count: int -) -> int: - """Return remaining priced-fallback capacity after primary selection.""" - if requested_limit < 1: - raise ValueError("ORCHESTRATOR_CATALOG_LIMIT must be positive") - total_limit = min(requested_limit, REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES) - if primary_count < 0 or primary_count > total_limit: - raise ValueError("primary route count exceeds the preflight budget") - return total_limit - primary_count - - -def _catalog_account_cap(default: int) -> int: - """Return the configured per-account catalog admission cap. - - ``default`` must be ``scripts.ci.contextual_orchestrator_review_policy``'s - own ``DEFAULT_ACCOUNT_CAP`` -- the single source of truth for how many - routes one credential account may contribute to the bounded preflight - budget. A caller must never substitute a total-routes-scale constant - (e.g. ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES``) here: doing so silently - disables per-account diversification and lets one rate-limited account - consume the entire preflight budget. That is not a hypothetical failure - mode -- a sibling in-flight branch's own ``_catalog_family_cap()`` - fell back to exactly ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`` and, in a live - production run, let two NVIDIA NIM credentials sharing one rate-limited - upstream jointly occupy 12/12 preflight slots, of which 10 were then - rejected with 429/404/timeout (see ContextualWisdomLab/.github#1415 and - the "빈 깡통 경로" report it responds to). Routing the default through the - caller-supplied ``policy.DEFAULT_ACCOUNT_CAP`` (rather than hand-typing a - literal here) keeps this module's cap from silently drifting out of sync - with the policy module's own declared intent. - - Args: - default: The cap to use when ``ORCHESTRATOR_CATALOG_ACCOUNT_CAP`` is - unset, always ``policy.DEFAULT_ACCOUNT_CAP``. - - Returns: - The per-account cap to pass to ``build_zdr_prioritized_catalog``. - """ - return int(os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", str(default))) - - def _with_discovery_counts( report: dict[str, object], rows: list[dict[str, Any]], @@ -781,7 +680,7 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument("--preflight-out", required=True, help="Path to write sanitized runtime preflight JSON") parser.add_argument("--zdr-endpoints", default=None, help="Optional OpenRouter /api/v1/endpoints/zdr JSON path") parser.add_argument("--require-zdr", action="store_true") - parser.add_argument("--pool", choices=("free", "auto"), default="free") + parser.add_argument("--pool", choices=("free",), default="free") args = parser.parse_args(argv) from contextual_orchestrator.credentials import get_credential @@ -794,8 +693,6 @@ def main(argv: list[str] | None = None) -> int: ) from contextual_orchestrator.server import SecurityConfig, serve from scripts.ci.contextual_orchestrator_review_policy import ( - DEFAULT_ACCOUNT_CAP, - PolicyError, _load_zdr_endpoints, build_zdr_prioritized_catalog, is_zdr_model, @@ -838,37 +735,8 @@ def main(argv: list[str] | None = None) -> int: _write_json(args.discovery_out, {"models": rows}) zdr_endpoints = _load_zdr_endpoints(args.zdr_endpoints) normalized_rows = parse_discovery_report({"models": rows}) - free_rows = [ - row for row in normalized_rows if row.get("cost_evidence") == "free" - ] - priced_rows = [ - row for row in normalized_rows if row.get("cost_evidence") == "priced" - ] - admitted_free_rows = _zdr_admitted_rows( - free_rows, - require_zdr=args.require_zdr, - zdr_endpoints=zdr_endpoints, - checker=is_zdr_model, - ) - admitted_priced_rows = _zdr_admitted_rows( - priced_rows, - require_zdr=args.require_zdr, - zdr_endpoints=zdr_endpoints, - checker=is_zdr_model, - ) - requested_catalog_limit = int(os.environ.get("ORCHESTRATOR_CATALOG_LIMIT", "12")) - primary_limit = _bounded_primary_catalog_limit( - requested_catalog_limit, pool=args.pool, has_free_rows=bool(admitted_free_rows) - ) - primary_rows = ( - (admitted_free_rows or admitted_priced_rows) - if args.pool == "auto" - else normalized_rows - ) result = build_zdr_prioritized_catalog( - primary_rows, - limit=primary_limit, - account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), + normalized_rows, zdr_endpoints=zdr_endpoints, require_zdr=args.require_zdr, pool=args.pool, @@ -883,68 +751,24 @@ def main(argv: list[str] | None = None) -> int: _write_json(args.report_out, result["report"]) agents = load_agents(args.catalog_out) - primary_report = result["report"] - fallback_result = None - fallback_agents: list[object] = [] - fallback_limit = _bounded_fallback_catalog_limit( - requested_catalog_limit, primary_count=len(result["agents"]) - ) - if ( - args.pool == "auto" - and admitted_free_rows - and admitted_priced_rows - and fallback_limit - ): - try: - fallback_result = build_zdr_prioritized_catalog( - admitted_priced_rows, - limit=fallback_limit, - account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), - zdr_endpoints=zdr_endpoints, - require_zdr=args.require_zdr, - pool="auto", - ) - except PolicyError: - fallback_result = None - if fallback_result is not None: - fallback_result["report"] = _with_discovery_counts( - fallback_result["report"], normalized_rows, provider_account=provider_account - ) - fallback_result["report"]["primary_selected_count"] = primary_report[ - "selected_count" - ] - fallback_result["report"]["primary_selection"] = primary_report["selected"] - fallback_agents = _load_temporary_agents( - f"{args.catalog_out}.priced", - fallback_result["agents"], - loader=load_agents, - ) client = ModelClient( + timeout=None, max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS, max_retries=0, temperature=REVIEW_TEMPERATURE, ) try: - agents, preflight_report, fallback_used = _preflight_with_fallback( - agents, fallback_agents, client=client - ) + agents, preflight_report = _preflight_review_agents(agents, client=client) except ReviewPreflightError as exc: _write_json(args.preflight_out, exc.report) _log_preflight_rejections(exc.report) raise SystemExit(f"review sidecar preflight failed: {exc}") from None - if fallback_used and fallback_result is not None: - Path(args.catalog_out).write_text( - json.dumps({"agents": fallback_result["agents"]}, indent=2, sort_keys=True) - + "\n", - encoding="utf-8", - ) - result = fallback_result - result["report"]["fallback_reason"] = "primary_routes_unavailable" - _write_json(args.report_out, result["report"]) _write_json(args.preflight_out, preflight_report) client = ModelClient( + timeout=None, max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS, + max_retries=0, temperature=REVIEW_TEMPERATURE, ) orchestrator = TaskOrchestrator(agents, client=client) diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 910b8da3a9..464536edfb 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -8,6 +8,13 @@ complete prompt/completion vector; Bytez may instead carry the exact-zero provider-meter attestation represented by contextual-orchestrator's ``is_free`` result. Partial, malformed, or contradictory price evidence fails closed. + +This module is an admission boundary, not a router. It therefore must not invent +candidate-count caps, per-provider quotas, price/ZDR/provider ordering, hand-set +priorities, or fallback preferences. Every row satisfying the explicit pool, +price, credential-source, and optional ZDR predicates remains admitted with +neutral priority. Downstream model choice requires its own evidence-backed +routing contract. """ from __future__ import annotations @@ -17,7 +24,6 @@ import math import re import sys -from collections import Counter from pathlib import Path from typing import Any, Iterable, Mapping @@ -31,6 +37,9 @@ route_key, ) +# Compatibility-only values retained while callers migrate away from the old +# command surface. They are deliberately ignored by admission and therefore do +# not affect candidate membership, ordering, or priority. DEFAULT_CATALOG_LIMIT = 12 DEFAULT_ACCOUNT_CAP = 4 @@ -51,11 +60,7 @@ COST_FREE = "free" COST_PRICED = "priced" COST_UNKNOWN = "unknown" -_COST_EVIDENCE_RANK: Mapping[str, int] = { - COST_FREE: 0, - COST_PRICED: 1, - COST_UNKNOWN: 2, -} +_COST_EVIDENCE_VALUES = frozenset({COST_FREE, COST_PRICED, COST_UNKNOWN}) _AGENT_ID_RE = re.compile(r"^[a-z][a-z0-9]*_[a-z0-9]+(?:_[a-z0-9]+)*$") @@ -75,7 +80,10 @@ def _normalize_agent_id(candidate: str, provider_name: str) -> str: parts = [part for part in slug.split("_") if part] if len(parts) == 1: parts.insert(0, provider_name) - return "_".join(parts) + normalized = "_".join(parts) + if not _AGENT_ID_RE.fullmatch(normalized): + raise PolicyError(f"model agent id {candidate!r} cannot be normalized safely") + return normalized def _route_key(provider_name: str, model: str) -> str: @@ -257,7 +265,7 @@ def parse_discovery_report(report: Mapping[str, Any]) -> list[dict[str, Any]]: def _cost_evidence(row: Mapping[str, Any]) -> str: """Return a validated cost-evidence tier from a normalized row.""" evidence = row.get("cost_evidence") - if evidence in _COST_EVIDENCE_RANK: + if evidence in _COST_EVIDENCE_VALUES: return str(evidence) # Backward compatibility for callers that build normalized-like rows by # hand rather than using parse_discovery_report(). @@ -276,23 +284,25 @@ def _free_pool_source_admitted(row: Mapping[str, Any]) -> bool: def build_zdr_prioritized_catalog( rows: Iterable[Mapping[str, Any]], *, - limit: int = DEFAULT_CATALOG_LIMIT, - account_cap: int = DEFAULT_ACCOUNT_CAP, + limit: object = DEFAULT_CATALOG_LIMIT, + account_cap: object = DEFAULT_ACCOUNT_CAP, zdr_endpoints: frozenset[str] = frozenset(), require_zdr: bool = False, pool: str = "free", ) -> dict[str, Any]: - """Select a free-first, ZDR-aware, credential-account-diverse catalog. - - ``orchestrator/free`` first applies a source-identity invariant: only rows - whose credential source is in :data:`FREE_POOL_CREDENTIAL_NAMES` are free - candidates. This is independent from global credential discovery, so an - OpenAI model may remain visible to audit or ``orchestrator/auto`` while - contributing zero free-pool candidates. - - Existing discovery-wide counters keep their historical meaning so runtime - enrichment cannot silently rewrite the contract. Additional - ``free_pool_*`` fields expose the narrower admitted subset explicitly. + """Compatibility-named admission API; it performs no prioritization. + + The historical function name is retained only so existing callers can roll + forward without a flag-day. ``limit`` and ``account_cap`` are likewise + compatibility-only: they are intentionally non-authoritative and are not + inspected, validated, serialized, or allowed to remove, rank, or prioritize + a candidate. The admission set is fully determined by explicit cost + evidence, ``orchestrator/free`` credential-source authorization, and the + caller's optional ZDR requirement. + + Input order is preserved only as discovery provenance. Every emitted agent + has neutral priority, so this module does not convert that serialization + order into routing authority. """ if pool not in {"free", "auto"}: raise PolicyError(f"unsupported review pool {pool!r}") @@ -303,9 +313,15 @@ def build_zdr_prioritized_catalog( all_priced_rows = [row for row in all_rows if _cost_evidence(row) == COST_PRICED] all_unknown_rows = [row for row in all_rows if _cost_evidence(row) == COST_UNKNOWN] candidate_rows = ( - free_pool_rows if pool == "free" else [*all_free_rows, *all_priced_rows] + free_pool_rows + if pool == "free" + else [ + row + for row in all_rows + if _cost_evidence(row) in {COST_FREE, COST_PRICED} + ] ) - eligible_rows = [ + picked = [ row for row in candidate_rows if not require_zdr @@ -315,31 +331,6 @@ def build_zdr_prioritized_catalog( zdr_endpoints=zdr_endpoints, ) ] - eligible_rows.sort( - key=lambda row: ( - _COST_EVIDENCE_RANK[_cost_evidence(row)], - 0 - if is_zdr_model( - str(row["provider"]), - model=str(row["model"]), - zdr_endpoints=zdr_endpoints, - ) - else 1, - str(row["provider"]), - str(row["model"]), - ) - ) - - per_account: Counter[str] = Counter() - picked: list[Mapping[str, Any]] = [] - for row in eligible_rows: - account = provider_account(str(row["provider"])) - if per_account[account] >= account_cap: - continue - per_account[account] += 1 - picked.append(row) - if len(picked) >= limit: - break if not picked: route_kind = "attested ZDR" if require_zdr else pool @@ -349,19 +340,22 @@ def build_zdr_prioritized_catalog( ) catalog_rows: list[dict[str, Any]] = [] + normalized_agent_ids: set[str] = set() zdr_count = 0 - for rank, row in enumerate(picked): + for row in picked: provider = str(row["provider"]) model = str(row["model"]) evidence = _cost_evidence(row) - zdr = is_zdr_model( - provider, model=model, zdr_endpoints=zdr_endpoints - ) + agent_id = _normalize_agent_id(str(row["agent_id"]), provider) + if agent_id in normalized_agent_ids: + raise PolicyError(f"agent id collision after normalization: {agent_id!r}") + normalized_agent_ids.add(agent_id) + zdr = is_zdr_model(provider, model=model, zdr_endpoints=zdr_endpoints) if zdr: zdr_count += 1 catalog_rows.append( { - "id": _normalize_agent_id(str(row["agent_id"]), provider), + "id": agent_id, "model": model, "base_url": row["base_url"], "api_key_env": "", @@ -371,7 +365,7 @@ def build_zdr_prioritized_catalog( f"cost:{evidence}", "zdr" if zdr else "non-zdr", ], - "priority": -rank, + "priority": 0, "disabled": False, "provider_name": provider, "provider_exclusions": [], @@ -389,7 +383,6 @@ def build_zdr_prioritized_catalog( free_pool_account_diversity = len( {provider_account(str(row["provider"])) for row in free_pool_rows} ) - selected_evidence = [_cost_evidence(row) for row in picked] return { "agents": catalog_rows, @@ -409,6 +402,8 @@ def build_zdr_prioritized_catalog( "priced_selected_count": selected_evidence.count(COST_PRICED), "unknown_selected_count": selected_evidence.count(COST_UNKNOWN), "zdr_selected_count": zdr_count, + "legacy_limit_ignored": True, + "legacy_account_cap_ignored": True, "zdr_sources": sorted( { provider_zdr_scope(str(row["provider"])).source @@ -460,8 +455,8 @@ def build_catalog_from_paths( *, out_path: str, report_path: str, - limit: int = DEFAULT_CATALOG_LIMIT, - account_cap: int = DEFAULT_ACCOUNT_CAP, + limit: object = DEFAULT_CATALOG_LIMIT, + account_cap: object = DEFAULT_ACCOUNT_CAP, zdr_endpoints_path: str | None = None, require_zdr: bool = False, pool: str = "free", @@ -487,6 +482,16 @@ def build_catalog_from_paths( return result +def _warn_explicit_legacy_options(argv: list[str]) -> None: + """Warn when obsolete cardinality options remain in operator configuration.""" + for option in ("--limit", "--account-cap"): + if any(argument == option or argument.startswith(f"{option}=") for argument in argv): + print( + f"contextual-orchestrator review policy: {option} is deprecated and ignored", + file=sys.stderr, + ) + + def _build_parser() -> argparse.ArgumentParser: """Build the command-line parser for catalog generation.""" parser = argparse.ArgumentParser( @@ -497,8 +502,16 @@ def _build_parser() -> argparse.ArgumentParser: ) parser.add_argument("--out", required=True, help="Path to write agents JSON") parser.add_argument("--report", required=True, help="Path to write audit JSON") - parser.add_argument("--limit", type=int, default=DEFAULT_CATALOG_LIMIT) - parser.add_argument("--account-cap", type=int, default=DEFAULT_ACCOUNT_CAP) + parser.add_argument( + "--limit", + default=DEFAULT_CATALOG_LIMIT, + help="Deprecated compatibility input; does not affect admission.", + ) + parser.add_argument( + "--account-cap", + default=DEFAULT_ACCOUNT_CAP, + help="Deprecated compatibility input; does not affect admission.", + ) parser.add_argument("--zdr-endpoints", default=None) parser.add_argument("--require-zdr", action="store_true") parser.add_argument("--pool", choices=("free", "auto"), default="free") @@ -507,7 +520,9 @@ def _build_parser() -> argparse.ArgumentParser: def main(argv: list[str] | None = None) -> int: """Run the catalog CLI and return one on policy or input failure.""" - args = _build_parser().parse_args(argv) + effective_argv = list(sys.argv[1:] if argv is None else argv) + args = _build_parser().parse_args(effective_argv) + _warn_explicit_legacy_options(effective_argv) try: build_catalog_from_paths( args.discovery_report, @@ -526,4 +541,4 @@ def main(argv: list[str] | None = None) -> int: if __name__ == "__main__": # pragma: no cover - raise SystemExit(main()) \ No newline at end of file + raise SystemExit(main()) diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index a96e854a51..a465503595 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -9,7 +9,7 @@ # are registered into the process-local KV by the launcher in the SAME process # that performs live model discovery and serves requests — never read back at # request time. The in-process free-priced discovery evidence is turned into a -# ZDR-prioritized, credential-account-diverse agents catalog by +# evidence-admitted agents catalog by # scripts/ci/contextual_orchestrator_review_policy.py for the `orchestrator/free` # (fail-closed zero-cost) pool. set -euo pipefail @@ -35,12 +35,6 @@ SIDECAR_LOG_SANITIZER="$ORG_REPO_ROOT/scripts/ci/sanitize_contextual_orchestrato # finishes, letting the shell script wait for a deterministic marker instead # of guessing whether the async sanitizer has caught up. SIDECAR_DISCOVERY_DIAGNOSTICS_SENTINEL="discovery_diagnostics_complete" -CATALOG_LIMIT="${ORCHESTRATOR_CATALOG_LIMIT:-12}" -# Each KV credential is an independent account, including two credentials for -# the same vendor or endpoint. The account cap prevents one credential from -# consuming the bounded twelve-route preflight catalog without inventing a -# provider-family equivalence relation. -CATALOG_ACCOUNT_CAP="${ORCHESTRATOR_CATALOG_ACCOUNT_CAP:-8}" ORCHESTRATOR_GITHUB_ENV="${GITHUB_ENV:-}" sidecar_python="$(command -v python3)" @@ -295,8 +289,6 @@ esac log "starting review sidecar on ${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}" cp "$ORCHESTRATOR_LAUNCHER" "$ORCHESTRATOR_WORK/launch_sidecar.py" -export ORCHESTRATOR_CATALOG_LIMIT="$CATALOG_LIMIT" -export ORCHESTRATOR_CATALOG_ACCOUNT_CAP="$CATALOG_ACCOUNT_CAP" # Stream stdout/stderr through the redacting sanitizer as two named, awaitable # processes (not bare `> >(...)` substitutions, whose PIDs bash never exposes) # so a failure handler can wait for the sanitizer to finish flushing before it diff --git a/scripts/source_fix_1629_no_heuristic_compute.py b/scripts/source_fix_1629_no_heuristic_compute.py new file mode 100644 index 0000000000..9630d342fa --- /dev/null +++ b/scripts/source_fix_1629_no_heuristic_compute.py @@ -0,0 +1,189 @@ +"""One-shot exact-owner repair for PR #1629's remaining inference heuristics.""" + +from __future__ import annotations + +from pathlib import Path +import re + + +ROOT = Path(__file__).resolve().parents[1] +LAUNCHER = ROOT / "scripts/ci/contextual_orchestrator_review_launcher.py" +SIDECAR = ROOT / "scripts/ci/contextual_orchestrator_review_sidecar.sh" +ADR = ROOT / "docs/adr/0005-sidecar-preflight-token-budget.md" +GAP = ROOT / "docs/product-technical-gap-baseline.md" +CHANGELOG = ROOT / "CHANGELOG.md" + + +def replace_once(text: str, pattern: str, replacement: str, *, label: str, flags: int = 0) -> str: + updated, count = re.subn(pattern, replacement, text, count=1, flags=flags) + if count != 1: + raise RuntimeError(f"{label}: expected exactly one match, found {count}") + return updated + + +def repair_launcher() -> None: + text = LAUNCHER.read_text(encoding="utf-8") + required = ( + "REVIEW_MAX_OUTPUT_TOKENS = 4096", + "REVIEW_TEMPERATURE = 1.0", + "REVIEW_PREFLIGHT_BASE_TOKENS = 16", + "REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS", + 'escalated_payload["max_tokens"] = REVIEW_PREFLIGHT_ESCALATED_TOKENS', + ) + for needle in required: + if needle not in text: + raise RuntimeError(f"launcher drift: missing {needle!r}") + + text = replace_once( + text, + r"# Keep ordinary review turns portable across small zero-cost providers\..*?REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS\n\n", + "", + label="launcher heuristic constants", + flags=re.S, + ) + + preflight = '''def _preflight_review_agent(\n agent: object, *, client: Any\n) -> tuple[object | None, dict[str, object]]:\n """Observe one route once without allocating inference compute.\n\n The startup boundary needs evidence that an admitted route can answer the\n OpenAI-compatible plain-chat shape. It does not own a statistical or\n research-backed model for token budget, sampling temperature, or retry\n count. Consequently the request leaves provider-default generation controls\n unspecified and is sent exactly once. Empty/truncated/reasoning-only output\n remains diagnostic evidence but cannot authorize a guessed second call.\n """\n row: dict[str, object] = {\n "agent_id": str(getattr(agent, "id", "")),\n "provider": str(getattr(agent, "provider_name", "") or "unknown"),\n "model": str(getattr(agent, "model", "")),\n "attempts": 1,\n }\n payload: dict[str, object] = {\n "model": getattr(agent, "model", ""),\n "messages": [\n {"role": "system", "content": "You are a helpful assistant."},\n {"role": "user", "content": "Reply with just 'OK'."},\n ],\n "stream": False,\n }\n try:\n response = _send_preflight_request(client, agent, payload)\n except Exception as exc: # noqa: BLE001 - sanitize at provider boundary\n _record_provider_exception(row, exc)\n return None, row\n\n finish_reason = _response_finish_reason(response)\n reasoning_without_content = _response_has_reasoning_without_content(response)\n row["finish_reason"] = finish_reason or "unknown"\n row["reasoning_without_content"] = reasoning_without_content\n if _chat_response_has_text(response):\n row["status"] = "ready"\n return agent, row\n\n row["status"] = "rejected"\n row["error_type"] = "insufficient_preflight_evidence"\n return None, row\n\n\ndef _preflight_review_agents(\n agents: list[object], *, client: Any\n) -> tuple[list[object], dict[str, object]]:\n """Probe every admitted route once with provider-account isolation.\n\n Independently credentialed accounts may progress concurrently because this\n changes only transport scheduling, not candidate membership, request\n quantity, generation controls, or publication order. Routes sharing one\n provider account remain serialized. Results are restored to catalog order.\n """\n if not agents:\n report: dict[str, object] = {\n "contract": "strix-plain-chat-preflight-v3",\n "probed_count": 0,\n "ready_count": 0,\n "rejected_count": 0,\n "routes": [],\n }\n raise ReviewPreflightError(\n "no provider route passed the Strix plain-chat preflight", report\n )\n\n provider_lanes: dict[str, list[tuple[int, object]]] = {}\n for index, agent in enumerate(agents):\n provider_account = str(getattr(agent, "provider_name", "") or "unknown")\n provider_lanes.setdefault(provider_account, []).append((index, agent))\n\n def probe_lane(\n lane: list[tuple[int, object]],\n ) -> list[tuple[int, tuple[object | None, dict[str, object]]]]:\n return [\n (index, _preflight_review_agent(agent, client=client))\n for index, agent in lane\n ]\n\n with ThreadPoolExecutor(\n max_workers=len(provider_lanes), thread_name_prefix="review-preflight"\n ) as executor:\n futures = [executor.submit(probe_lane, lane) for lane in provider_lanes.values()]\n indexed_outcomes = [\n indexed_outcome\n for future in futures\n for indexed_outcome in future.result()\n ]\n indexed_outcomes.sort(key=lambda item: item[0])\n\n viable: list[object] = []\n routes: list[dict[str, object]] = []\n for _index, (ready_agent, row) in indexed_outcomes:\n routes.append(row)\n if ready_agent is not None:\n viable.append(ready_agent)\n\n report = {\n "contract": "strix-plain-chat-preflight-v3",\n "probed_count": len(agents),\n "ready_count": len(viable),\n "rejected_count": len(agents) - len(viable),\n "routes": routes,\n }\n if not viable:\n raise ReviewPreflightError(\n "no provider route passed the Strix plain-chat preflight", report\n )\n return viable, report\n\n\n''' + text = replace_once( + text, + r"def _preflight_review_agent\(.*?\ndef _log_preflight_rejections\(", + preflight + "def _log_preflight_rejections(", + label="launcher preflight implementation", + flags=re.S, + ) + + old_client = ''' client = ModelClient(\n timeout=None,\n max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS,\n max_retries=0,\n temperature=REVIEW_TEMPERATURE,\n )''' + if text.count(old_client) != 2: + raise RuntimeError(f"launcher client drift: expected two constructors, found {text.count(old_client)}") + text = text.replace(old_client, " client = ModelClient(timeout=None, max_retries=0)") + + for forbidden in ( + "REVIEW_MAX_OUTPUT_TOKENS", + "REVIEW_TEMPERATURE", + "REVIEW_PREFLIGHT_BASE_TOKENS", + "REVIEW_PREFLIGHT_ESCALATED_TOKENS", + '"max_tokens"', + '"temperature"', + "_preflight_with_fallback", + "escalations_used", + ): + if forbidden in text: + raise RuntimeError(f"launcher repair incomplete: {forbidden!r} remains") + LAUNCHER.write_text(text, encoding="utf-8") + + +def repair_sidecar() -> None: + text = SIDECAR.read_text(encoding="utf-8") + for needle in ( + '"temperature":1.0,"max_tokens":4096', + 'REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS="${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS:-3}"', + ): + if needle not in text: + raise RuntimeError(f"sidecar drift: missing {needle!r}") + + replacement = r'''gateway_virtual_model="orchestrator/${orchestrator_pool}" +printf '{"model":"%s","messages":[{"role":"system","content":"You are a helpful assistant."},{"role":"user","content":"Reply with just '\''OK'\''."}],"stream":false}\n' \ + "$gateway_virtual_model" > "$gateway_preflight_request" +# This is one model-inference compatibility observation. No repository-owned +# retry count, output-token allocation, temperature, or wall-clock deadline is +# identified by the available evidence, so any transport/non-2xx result fails +# closed rather than manufacturing another inference attempt. +gateway_attempt=1 +gateway_http_status="" +if gateway_http_status="$( + curl -sS \ + -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 + "$sidecar_python" - "$preflight_report" "$gateway_preflight_response" "$gateway_http_status" <<'PY' +import json +from pathlib import Path +import re +import sys + +report_path = Path(sys.argv[1]) +response_path = Path(sys.argv[2]) +status_text = sys.argv[3] +try: + report = json.loads(report_path.read_text(encoding="utf-8")) +except (OSError, json.JSONDecodeError): + report = {} +try: + response = json.loads(response_path.read_text(encoding="utf-8")) +except (OSError, json.JSONDecodeError): + response = {} +error = response.get("error") if isinstance(response, dict) else None +code = error.get("code") if isinstance(error, dict) else None +if not isinstance(code, str) or not re.fullmatch(r"[A-Za-z0-9_.-]{1,64}", code): + code = "unknown_error" +status = int(status_text) if status_text.isdecimal() else 0 +report["gateway"] = { + "endpoint": "chat/completions", + "error_type": "gateway_transport_failure" if not status else "gateway_rejected", + "error_code": code, + "http_status": status, + "attempts": 1, + "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 + if [ -z "$gateway_http_status" ]; then + fail "gateway preflight request could not reach the local sidecar" + fi + fail "gateway preflight returned HTTP ${gateway_http_status}" +fi +''' + text = replace_once( + text, + r'gateway_virtual_model="orchestrator/\$\{orchestrator_pool\}".*?(?=if ! "\$sidecar_python" - "\$gateway_preflight_response" "\$preflight_report" "\$gateway_attempt" <<\'PY\')', + replacement, + label="sidecar model preflight", + flags=re.S, + ) + for forbidden in ( + "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS", + '"temperature":1.0', + '"max_tokens":4096', + "retrying (up to", + ): + if forbidden in text: + raise RuntimeError(f"sidecar repair incomplete: {forbidden!r} remains") + SIDECAR.write_text(text, encoding="utf-8") + + +def append_docs() -> None: + adr = ADR.read_text(encoding="utf-8") + section = '''\n\n## 2026-09-02 no-heuristics compute-allocation amendment\n\nThe historical 16-token base probe, 4096-token escalation/serving request,\n`temperature=1.0`, and three-attempt gateway retry were repository-authored\ninference allocations. A response classified as truncated or reasoning-only\nproves that the observed request did not yield usable review text; it does not\nidentify a statistically justified next token budget, sampling value, or number\nof additional model calls. The fixed values therefore cease to be decision\nauthority. Central review startup now makes exactly one provider-default\nplain-chat compatibility observation per admitted route and one provider-default\nvirtual-pool observation. Missing/malformed/truncated output and transport\nfailure fail closed. Provider-published `max_output_tokens` metadata may clamp\nan independently explicit request ceiling, but it is not a model for how much\ngeneration the review sidecar should allocate.\n\nThis change is intentionally narrower than routing research such as Fugu,\nConductor, and TRINITY: learned/test-time-compute policies require their own\nvalidated estimator and executable provenance before they can allocate review\ncompute here. HTTP failure classification likewise does not identify a count of\nmodel-inference replays.\n\n### Standards and research traceability (APA 7)\n\nFielding, R., Nottingham, M., & Reschke, J. (2022). *HTTP semantics* (RFC\n9110). Internet Engineering Task Force. https://www.rfc-editor.org/rfc/rfc9110\n\nOpenRouter. (2026). *OpenRouter API specification* [OpenAPI specification].\nhttps://openrouter.ai/openapi.yaml\n''' + if "## 2026-09-02 no-heuristics compute-allocation amendment" not in adr: + ADR.write_text(adr.rstrip() + section + "\n", encoding="utf-8") + + gap = GAP.read_text(encoding="utf-8") + gap_section = '''\n\n### 2026-09-02 — central review preflight compute allocation\n\n**Live gap.** PR #1629's admission-only catalog repair still carried fixed\n`16 -> 4096` semantic token escalation, `temperature=1.0`, a 4096-token\nvirtual-pool request, and a three-attempt gateway inference retry. These values\nchanged model-call quantity or test-time compute without an identified\nstatistical/research model. Issues #1454 and #1458 already documented that the\nold probe/escalation policy could not prove serving-budget compatibility and\nthat bounded escalation allocation introduced order-dependent selection.\n\n**Causal owner and repair.** The shared owner is\n`scripts/ci/contextual_orchestrator_review_launcher.py` plus\n`contextual_orchestrator_review_sidecar.sh`. The repair removes repository-owned\ngeneration-token, temperature, and inference-retry allocation from startup.\nEach admitted route receives one provider-default compatibility observation;\ninsufficient response evidence fails closed as\n`insufficient_preflight_evidence`. The virtual-pool check is likewise one\nprovider-default request. `timeout=None` and `max_retries=0` remain explicit\nnegative controls until contextual-orchestrator's upstream defaults are repaired\nby its canonical owner PR.\n\n**Executable evidence.**\n`tests/test_contextual_orchestrator_review_no_heuristic_compute.py` is the\nRED-before-repair contract. Historical escalation/retry cases remain in the\nnon-collectable case module as incident evidence while the collection shim\nreplaces their forbidden policy oracle with the fail-closed contract. Exact-head\nhosted verification is required before merge; predecessor results do not count.\n\n**Basis.** RFC 9110 classifies HTTP semantics but does not identify a number of\nLLM inference retries. Provider-published output ceilings constrain an explicit\nrequest but do not determine a desired review-generation allocation. In the\nabsence of an independently validated compute-allocation model, the admissible\npolicy is fail closed rather than substituting another fixed number.\n''' + if "### 2026-09-02 — central review preflight compute allocation" not in gap: + GAP.write_text(gap.rstrip() + gap_section + "\n", encoding="utf-8") + + changelog = CHANGELOG.read_text(encoding="utf-8") + entry = '''\n- (PR #1629 no-heuristics RCA) Removed review-sidecar inference allocation\n heuristics: fixed 16/4096 token probes, repository-authored temperature,\n semantic escalation, and the three-attempt gateway model retry. Startup now\n performs one provider-default compatibility observation and fails closed when\n that evidence is insufficient; no token/retry substitute is invented.\n''' + if "Removed review-sidecar inference allocation" not in changelog: + CHANGELOG.write_text(changelog.rstrip() + entry + "\n", encoding="utf-8") + + +def main() -> None: + repair_launcher() + repair_sidecar() + append_docs() + + +if __name__ == "__main__": + main() diff --git a/scripts/source_fix_1629_no_heuristic_compute_v2.py b/scripts/source_fix_1629_no_heuristic_compute_v2.py new file mode 100644 index 0000000000..2ed9c0ac9c --- /dev/null +++ b/scripts/source_fix_1629_no_heuristic_compute_v2.py @@ -0,0 +1,203 @@ +"""Drift-safe one-shot repair for PR #1629's remaining inference heuristics.""" + +from __future__ import annotations + +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] +LAUNCHER = ROOT / "scripts/ci/contextual_orchestrator_review_launcher.py" +SIDECAR = ROOT / "scripts/ci/contextual_orchestrator_review_sidecar.sh" +ADR = ROOT / "docs/adr/0005-sidecar-preflight-token-budget.md" +GAP = ROOT / "docs/product-technical-gap-baseline.md" +CHANGELOG = ROOT / "CHANGELOG.md" + + +def between(text: str, start: str, end: str, replacement: str, *, label: str) -> str: + start_index = text.find(start) + if start_index < 0: + raise RuntimeError(f"{label}: start anchor missing") + end_index = text.find(end, start_index) + if end_index < 0: + raise RuntimeError(f"{label}: end anchor missing") + if text.find(start, start_index + 1) >= 0: + raise RuntimeError(f"{label}: start anchor is not unique") + return text[:start_index] + replacement + text[end_index:] + + +def repair_launcher() -> None: + text = LAUNCHER.read_text(encoding="utf-8") + for needle in ( + "REVIEW_MAX_OUTPUT_TOKENS = 4096", + "REVIEW_TEMPERATURE = 1.0", + "REVIEW_PREFLIGHT_BASE_TOKENS = 16", + "REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS", + 'escalated_payload["max_tokens"] = REVIEW_PREFLIGHT_ESCALATED_TOKENS', + ): + if needle not in text: + raise RuntimeError(f"launcher drift: missing {needle!r}") + + constants_start = "# Keep ordinary review turns portable across small zero-cost providers." + constants_end_line = "REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS\n" + start_index = text.index(constants_start) + end_index = text.index(constants_end_line, start_index) + len(constants_end_line) + text = text[:start_index] + text[end_index:].lstrip("\n") + + replacement = '''def _preflight_review_agent(\n agent: object, *, client: Any\n) -> tuple[object | None, dict[str, object]]:\n """Observe one route once without allocating inference compute.\n\n Startup needs evidence that an admitted route can answer the ordinary\n OpenAI-compatible plain-chat shape. No validated model in this repository\n identifies a token budget, sampling temperature, or retry count for that\n observation, so provider defaults are left unspecified and the payload is\n sent exactly once. Empty, truncated, or reasoning-only output is retained\n as diagnostic evidence but cannot authorize a guessed second model call.\n """\n row: dict[str, object] = {\n "agent_id": str(getattr(agent, "id", "")),\n "provider": str(getattr(agent, "provider_name", "") or "unknown"),\n "model": str(getattr(agent, "model", "")),\n "attempts": 1,\n }\n payload: dict[str, object] = {\n "model": getattr(agent, "model", ""),\n "messages": [\n {"role": "system", "content": "You are a helpful assistant."},\n {"role": "user", "content": "Reply with just 'OK'."},\n ],\n "stream": False,\n }\n try:\n response = _send_preflight_request(client, agent, payload)\n except Exception as exc: # noqa: BLE001 - sanitize at provider boundary\n _record_provider_exception(row, exc)\n return None, row\n\n row["finish_reason"] = _response_finish_reason(response) or "unknown"\n row["reasoning_without_content"] = _response_has_reasoning_without_content(response)\n if _chat_response_has_text(response):\n row["status"] = "ready"\n return agent, row\n\n row["status"] = "rejected"\n row["error_type"] = "insufficient_preflight_evidence"\n return None, row\n\n\ndef _preflight_review_agents(\n agents: list[object], *, client: Any\n) -> tuple[list[object], dict[str, object]]:\n """Probe every admitted route once with provider-account isolation.\n\n Independent credential accounts may progress concurrently because this\n changes transport scheduling only: it cannot alter candidate membership,\n request count per route, generation controls, or output ordering. Routes\n sharing one provider account stay serialized, and evidence is restored to\n catalog order before publication.\n """\n if not agents:\n report: dict[str, object] = {\n "contract": "strix-plain-chat-preflight-v3",\n "probed_count": 0,\n "ready_count": 0,\n "rejected_count": 0,\n "routes": [],\n }\n raise ReviewPreflightError(\n "no provider route passed the Strix plain-chat preflight", report\n )\n\n provider_lanes: dict[str, list[tuple[int, object]]] = {}\n for index, agent in enumerate(agents):\n account = str(getattr(agent, "provider_name", "") or "unknown")\n provider_lanes.setdefault(account, []).append((index, agent))\n\n def probe_lane(\n lane: list[tuple[int, object]],\n ) -> list[tuple[int, tuple[object | None, dict[str, object]]]]:\n return [\n (index, _preflight_review_agent(agent, client=client))\n for index, agent in lane\n ]\n\n with ThreadPoolExecutor(\n max_workers=len(provider_lanes), thread_name_prefix="review-preflight"\n ) as executor:\n futures = [executor.submit(probe_lane, lane) for lane in provider_lanes.values()]\n indexed_outcomes = [\n item for future in futures for item in future.result()\n ]\n indexed_outcomes.sort(key=lambda item: item[0])\n\n viable: list[object] = []\n routes: list[dict[str, object]] = []\n for _index, (ready_agent, row) in indexed_outcomes:\n routes.append(row)\n if ready_agent is not None:\n viable.append(ready_agent)\n\n report = {\n "contract": "strix-plain-chat-preflight-v3",\n "probed_count": len(agents),\n "ready_count": len(viable),\n "rejected_count": len(agents) - len(viable),\n "routes": routes,\n }\n if not viable:\n raise ReviewPreflightError(\n "no provider route passed the Strix plain-chat preflight", report\n )\n return viable, report\n\n\n''' + text = between( + text, + "def _preflight_review_agent(", + "def _log_preflight_rejections(", + replacement, + label="launcher preflight block", + ) + + old_client = ''' client = ModelClient(\n timeout=None,\n max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS,\n max_retries=0,\n temperature=REVIEW_TEMPERATURE,\n )''' + if text.count(old_client) != 2: + raise RuntimeError( + f"launcher client drift: expected two constructors, found {text.count(old_client)}" + ) + text = text.replace(old_client, " client = ModelClient(timeout=None, max_retries=0)") + + for forbidden in ( + "REVIEW_MAX_OUTPUT_TOKENS", + "REVIEW_TEMPERATURE", + "REVIEW_PREFLIGHT_BASE_TOKENS", + "REVIEW_PREFLIGHT_ESCALATED_TOKENS", + '"max_tokens"', + '"temperature"', + "_preflight_with_fallback", + "escalations_used", + ): + if forbidden in text: + raise RuntimeError(f"launcher repair incomplete: {forbidden!r} remains") + LAUNCHER.write_text(text, encoding="utf-8") + + +def repair_sidecar() -> None: + text = SIDECAR.read_text(encoding="utf-8") + for needle in ( + '"temperature":1.0,"max_tokens":4096', + 'REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS="${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS:-3}"', + ): + if needle not in text: + raise RuntimeError(f"sidecar drift: missing {needle!r}") + + start = 'gateway_virtual_model="orchestrator/${orchestrator_pool}"' + end = 'if ! "$sidecar_python" - "$gateway_preflight_response" "$preflight_report" "$gateway_attempt" <<\'PY\'' + replacement = r'''gateway_virtual_model="orchestrator/${orchestrator_pool}" +printf '{"model":"%s","messages":[{"role":"system","content":"You are a helpful assistant."},{"role":"user","content":"Reply with just '\''OK'\''."}],"stream":false}\n' \ + "$gateway_virtual_model" > "$gateway_preflight_request" +# One provider-default model-inference compatibility observation. The available +# evidence identifies no inference retry count, output-token allocation, +# temperature, or wall-clock deadline. A transport/non-2xx result therefore +# fails closed instead of manufacturing another model request. +gateway_attempt=1 +gateway_http_status="" +if gateway_http_status="$( + curl -sS \ + -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 + "$sidecar_python" - "$preflight_report" "$gateway_preflight_response" "$gateway_http_status" <<'PY' +import json +from pathlib import Path +import re +import sys + +report_path = Path(sys.argv[1]) +response_path = Path(sys.argv[2]) +status_text = sys.argv[3] +try: + report = json.loads(report_path.read_text(encoding="utf-8")) +except (OSError, json.JSONDecodeError): + report = {} +try: + response = json.loads(response_path.read_text(encoding="utf-8")) +except (OSError, json.JSONDecodeError): + response = {} +error = response.get("error") if isinstance(response, dict) else None +code = error.get("code") if isinstance(error, dict) else None +if not isinstance(code, str) or not re.fullmatch(r"[A-Za-z0-9_.-]{1,64}", code): + code = "unknown_error" +status = int(status_text) if status_text.isdecimal() else 0 +report["gateway"] = { + "endpoint": "chat/completions", + "error_type": "gateway_transport_failure" if not status else "gateway_rejected", + "error_code": code, + "http_status": status, + "attempts": 1, + "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 + if [ -z "$gateway_http_status" ]; then + fail "gateway preflight request could not reach the local sidecar" + fi + fail "gateway preflight returned HTTP ${gateway_http_status}" +fi +''' + text = between(text, start, end, replacement, label="sidecar gateway preflight") + for forbidden in ( + "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS", + '"temperature":1.0', + '"max_tokens":4096', + "retrying (up to", + ): + if forbidden in text: + raise RuntimeError(f"sidecar repair incomplete: {forbidden!r} remains") + SIDECAR.write_text(text, encoding="utf-8") + + +def append_docs() -> None: + adr = ADR.read_text(encoding="utf-8") + heading = "## 2026-09-02 no-heuristics compute-allocation amendment" + if heading not in adr: + ADR.write_text( + adr.rstrip() + + '''\n\n## 2026-09-02 no-heuristics compute-allocation amendment\n\nThe historical 16-token base probe, 4096-token escalation/serving request,\n`temperature=1.0`, and three-attempt gateway retry were repository-authored\ninference allocations. A truncated or reasoning-only response proves only that\nthe observed request did not yield usable review text; it does not identify a\nstatistically justified next token budget, sampling value, or number of extra\nmodel calls. Those fixed values therefore cease to be decision authority.\nCentral review startup now makes exactly one provider-default plain-chat\ncompatibility observation per admitted route and one provider-default\nvirtual-pool observation. Missing, malformed, truncated, or transport-failed\nevidence fails closed. Provider-published `max_output_tokens` metadata may clamp\nan independently explicit request ceiling, but it does not determine how much\ngeneration the review sidecar should allocate.\n\nFugu, Conductor, TRINITY and documented learned-routing successors can justify\ncompute allocation only when their estimator is actually trained/evaluated for\nthis deployment and has executable provenance; their existence is not evidence\nfor the retired constants. HTTP failure classification likewise does not\nidentify a number of LLM inference replays.\n\n### Standards and research traceability (APA 7)\n\nFielding, R., Nottingham, M., & Reschke, J. (2022). *HTTP semantics* (RFC\n9110). Internet Engineering Task Force. https://www.rfc-editor.org/rfc/rfc9110\n\nOpenRouter. (2026). *OpenRouter API specification* [OpenAPI specification].\nhttps://openrouter.ai/openapi.yaml\n''' + + "\n", + encoding="utf-8", + ) + + gap = GAP.read_text(encoding="utf-8") + gap_heading = "### 2026-09-02 — central review preflight compute allocation" + if gap_heading not in gap: + GAP.write_text( + gap.rstrip() + + '''\n\n### 2026-09-02 — central review preflight compute allocation\n\n**Live gap.** PR #1629's admission-only catalog repair still carried fixed\n`16 -> 4096` semantic token escalation, `temperature=1.0`, a 4096-token\nvirtual-pool request, and a three-attempt gateway inference retry. These values\nchanged model-call quantity or test-time compute without an identified\nstatistical/research model. Issues #1454 and #1458 already documented that the\nold probe/escalation policy could not prove serving-budget compatibility and\nthat bounded escalation allocation introduced order-dependent selection.\n\n**Causal owner and repair.** The shared owner is\n`scripts/ci/contextual_orchestrator_review_launcher.py` plus\n`scripts/ci/contextual_orchestrator_review_sidecar.sh`. The repair removes\nrepository-owned generation-token, temperature, and inference-retry allocation\nfrom startup. Each admitted route receives one provider-default compatibility\nobservation; insufficient response evidence fails closed as\n`insufficient_preflight_evidence`. The virtual-pool check is likewise exactly\none provider-default request. `timeout=None` and `max_retries=0` remain explicit\nnegative controls until contextual-orchestrator's library defaults are repaired\nby their canonical owner PR.\n\n**Executable evidence.**\n`tests/test_contextual_orchestrator_review_no_heuristic_compute.py` is the\nRED-before-repair contract. Historical escalation/retry cases stay in the\nnon-collectable case module as incident evidence; the collection shim replaces\ntheir forbidden policy oracle with the fail-closed contract. Exact-head hosted\nverification is required before merge; predecessor results do not count.\n\n**Basis.** RFC 9110 constrains HTTP semantics but does not identify a number of\nLLM inference retries. Provider-published output ceilings constrain an explicit\nrequest but do not identify a desired review-generation allocation. Without an\nindependently validated compute-allocation model, fail closed is the only\nnon-invented behavior.\n''' + + "\n", + encoding="utf-8", + ) + + changelog = CHANGELOG.read_text(encoding="utf-8") + marker = "Removed review-sidecar inference allocation heuristics" + if marker not in changelog: + CHANGELOG.write_text( + changelog.rstrip() + + '''\n\n- (PR #1629 no-heuristics RCA) Removed review-sidecar inference allocation heuristics:\n fixed 16/4096 token probes, repository-authored temperature, semantic\n escalation, and the three-attempt gateway model retry. Startup now performs\n one provider-default compatibility observation and fails closed when that\n evidence is insufficient; no token/retry substitute is invented.\n''' + + "\n", + encoding="utf-8", + ) + + +def main() -> None: + repair_launcher() + repair_sidecar() + append_docs() + + +if __name__ == "__main__": + main() diff --git a/tests/_contextual_orchestrator_review_runtime_preflight_cases.py b/tests/_contextual_orchestrator_review_runtime_preflight_cases.py new file mode 100644 index 0000000000..d3455e9502 --- /dev/null +++ b/tests/_contextual_orchestrator_review_runtime_preflight_cases.py @@ -0,0 +1,1732 @@ +"""Regression tests for the Strix contextual-orchestrator runtime boundary.""" + +from __future__ import annotations + +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 + +import pytest + +from scripts.ci import contextual_orchestrator_review_policy as policy + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_LAUNCHER = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_launcher.py" +_SIDECAR = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_sidecar.sh" +_SANITIZER = _REPO_ROOT / "scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py" + + +class _ProbeClient: + """Return deterministic per-agent outcomes for runtime preflight tests.""" + + def __init__(self, outcomes: dict[str, object]) -> None: + self.outcomes = 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 configured outcome.""" + self.calls.append((agent, endpoint, payload)) + outcome = self.outcomes[str(getattr(agent, "id"))] + if isinstance(outcome, BaseException): + raise outcome + assert isinstance(outcome, dict) + 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)) + + +def _load_sanitizer() -> dict[str, object]: + """Execute the sidecar stream sanitizer and return its module namespace.""" + return runpy.run_path(str(_SANITIZER)) + + +def _openai_text(content: str) -> dict[str, object]: + """Build the minimal OpenAI chat response shape accepted by preflight.""" + return {"choices": [{"message": {"content": content}}]} + + +def test_routable_discovered_models_excludes_evidence_only_rows() -> None: + """Evidence-only rows (e.g. OpenRouter) must never enter live selection.""" + namespace = _load_launcher() + routable = namespace.get("_routable_discovered_models") + assert callable(routable), "launcher must expose an evidence-only discovery filter" + + evidence_only_model = SimpleNamespace( + id="openrouter_evidence_only", + provider_name="openrouter", + model_id="some/model", + evidence_only=True, + ) + live_model = SimpleNamespace( + id="nvidia_ready", + provider_name="nvidia_nim", + model_id="ready/free", + evidence_only=False, + ) + no_flag_model = SimpleNamespace( + id="bytez_untagged", provider_name="bytez", model_id="untagged/free" + ) + + assert routable([evidence_only_model, live_model, no_flag_model]) == [ + live_model, + no_flag_model, + ] + assert routable(None) == [] + assert routable([]) == [] + + +def test_log_discovery_errors_prints_one_bounded_line_per_provider_failure( + capsys: pytest.CaptureFixture[str], +) -> None: + """A discarded discovery error must become a visible, sanitizer-safe diagnostic.""" + namespace = _load_launcher() + log_discovery_errors = namespace.get("_log_discovery_errors") + assert callable(log_discovery_errors), "launcher must expose a discovery-error logger" + + errors = [ + SimpleNamespace(provider_name="bytez", error_code="http_status_401"), + SimpleNamespace(provider_name="openai", error_code="timeout"), + ] + + log_discovery_errors(errors) + + captured = capsys.readouterr() + assert captured.out == "" + assert captured.err.splitlines() == [ + "provider_discovery_failed provider=bytez code=http_status_401", + "provider_discovery_failed provider=openai code=timeout", + "discovery_diagnostics_complete", + ] + + +def test_log_discovery_errors_emits_only_the_sentinel_on_a_clean_discovery( + capsys: pytest.CaptureFixture[str], +) -> None: + """No providers failed -> just the completion sentinel, no warning lines.""" + namespace = _load_launcher() + log_discovery_errors = namespace.get("_log_discovery_errors") + assert callable(log_discovery_errors) + + log_discovery_errors([]) + + captured = capsys.readouterr() + assert captured.out == "" + assert captured.err == "discovery_diagnostics_complete\n" + + +def test_log_discovery_errors_sentinel_matches_the_sidecar_scripts_constant() -> None: + """The sidecar shell script's poll target must equal this exact literal.""" + namespace = _load_launcher() + sentinel = namespace.get("_DISCOVERY_DIAGNOSTICS_COMPLETE_SENTINEL") + assert sentinel == "discovery_diagnostics_complete" + sidecar_text = _SIDECAR.read_text(encoding="utf-8") + 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() + preflight = namespace.get("_preflight_review_agents") + assert callable(preflight), "launcher must preflight every selected provider route" + + rejected = SimpleNamespace( + id="openrouter_rejected", provider_name="openrouter", model="rejected/free" + ) + malformed = SimpleNamespace( + id="openrouter_malformed", provider_name="openrouter", model="malformed/free" + ) + ready = SimpleNamespace( + id="nvidia_ready", provider_name="nvidia_nim", model="ready/free" + ) + secret = "sk-secret-must-not-enter-evidence" + client = _ProbeClient( + { + rejected.id: RuntimeError(f"upstream rejected {secret}"), + malformed.id: {"choices": []}, + ready.id: _openai_text("OK"), + } + ) + + viable, report = preflight([rejected, malformed, ready], client=client) + + assert viable == [ready] + assert report["probed_count"] == 3 + assert report["ready_count"] == 1 + assert report["rejected_count"] == 2 + assert [row["status"] for row in report["routes"]] == [ + "rejected", + "rejected", + "ready", + ] + assert report["routes"][0]["error_type"] == "RuntimeError" + 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 + assert payload["stream"] is False + assert payload["max_tokens"] == 16 + assert payload["temperature"] == 1.0 + assert payload["messages"] == [ + {"role": "system", "content": "You are a helpful assistant."}, + {"role": "user", "content": "Reply with just 'OK'."}, + ] + assert "tools" not in payload + + +def test_log_preflight_rejections_prints_bounded_summary_to_stderr( + capsys: pytest.CaptureFixture[str], +) -> None: + """A ReviewPreflightError's report must reach the job log, not just the artifact. + + Regression coverage for the gap that made the launcher's own internal + preflight (distinct from the sidecar script's external curl-based gateway + preflight) fail with only "review sidecar preflight failed" visible and + the real per-route rejection reasons hidden behind + omitted_unstructured_lines in the sanitized stream. + """ + namespace = _load_launcher() + log_preflight_rejections = namespace.get("_log_preflight_rejections") + assert callable(log_preflight_rejections) + + secret = "sk-secret-must-not-enter-evidence" + report = { + "routes": [ + { + "agent_id": "nim_nano_free", + "provider": "nvidia_nim", + "model": "nvidia/nemotron-3-nano-30b-a3b", + "status": "rejected", + "error_type": "ProviderUpstreamError", + "http_status": 429, + }, + { + "agent_id": "or_ds_r1", + "provider": "openrouter", + "model": "deepseek/deepseek-r1:free", + "status": "rejected", + "error_type": f"RuntimeError {secret}", + }, + { + "agent_id": "ready_one", + "provider": "openai", + "model": "gpt-4o-mini", + "status": "ready", + }, + ], + } + log_preflight_rejections(report) + captured = capsys.readouterr() + assert captured.out == "" + assert secret not in captured.err + assert ( + "preflight_route_rejected provider=nvidia_nim " + "error_type=ProviderUpstreamError http_status=429" + ) in captured.err + # The openrouter route's error_type ("RuntimeError ") is not a + # Python identifier, so _log_preflight_rejections' own isidentifier() + # guard replaces it with the bounded placeholder "UnknownError" rather + # than printing it as-is -- this helper is itself the bound that keeps + # an unexpected, non-identifier error_type (and anything embedded in it, + # such as the secret above) out of the job log. + assert "preflight_route_rejected provider=openrouter error_type=UnknownError" in captured.err + assert "RuntimeError" not in captured.err + assert "ready_one" not in captured.err + + +def test_log_preflight_rejections_covers_nested_primary_attempt( + capsys: pytest.CaptureFixture[str], +) -> None: + """A fallback-pool failure must also surface the primary pool's rejections.""" + namespace = _load_launcher() + log_preflight_rejections = namespace.get("_log_preflight_rejections") + assert callable(log_preflight_rejections) + + report = { + "routes": [ + { + "provider": "openai", + "status": "rejected", + "error_type": "ProviderUpstreamError", + "http_status": 503, + }, + ], + "primary_attempt": { + "routes": [ + { + "provider": "bytez", + "status": "rejected", + "error_type": "InvalidChatResponse", + }, + ], + }, + } + log_preflight_rejections(report) + captured = capsys.readouterr() + assert "preflight_route_rejected provider=bytez error_type=InvalidChatResponse" in captured.err + assert ( + "preflight_route_rejected provider=openai error_type=ProviderUpstreamError http_status=503" + in captured.err + ) + + +def test_log_preflight_rejections_ignores_malformed_report( + capsys: pytest.CaptureFixture[str], +) -> None: + """A report missing the expected shape must not raise or print anything.""" + namespace = _load_launcher() + log_preflight_rejections = namespace.get("_log_preflight_rejections") + assert callable(log_preflight_rejections) + + log_preflight_rejections({}) + log_preflight_rejections({"routes": "not-a-list"}) + log_preflight_rejections({"routes": ["not-a-dict"]}) + captured = capsys.readouterr() + assert captured.out == "" + assert captured.err == "" + + +def test_gateway_preflight_max_tokens_is_synchronized_with_the_routing_probe() -> None: + """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. + + 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"] + sidecar = _SIDECAR.read_text(encoding="utf-8") + + match = re.search( + r'gateway_virtual_model.*?"max_tokens":(\d+)', sidecar, re.DOTALL + ) + assert match, "sidecar must send one JSON gateway preflight request with an explicit max_tokens" + gateway_preflight_max_tokens = int(match.group(1)) + + assert gateway_preflight_max_tokens == review_max_output_tokens, ( + "gateway preflight max_tokens " + f"({gateway_preflight_max_tokens}) must equal the routing probe's " + f"REVIEW_MAX_OUTPUT_TOKENS ({review_max_output_tokens}); a smaller " + "budget here can reject a route the routing probe already proved " + "ready" + ) + + +def test_gateway_preflight_has_no_inference_timeout() -> None: + """The end-to-end gateway check must not cap real completion latency. + + Regression for the 2026-08-30 gateway-preflight-timeout incident: exact- + evidence reproduction (Strix run 33306775025 on + ContextualWisdomLab/contextual-orchestrator#921, job 99244624298) showed + the routing probe marking a DeepSeek NIM route "ready" in 18s, then the + identical gateway request against that same healthy route being cut off + at exactly curl's configured bound -- "gateway preflight request could + not reach the local sidecar" was that timeout, not a real connectivity + failure. The request therefore has no wall-clock bound. + """ + sidecar = _SIDECAR.read_text(encoding="utf-8") + + request_block = sidecar.rsplit("curl -sS", 1)[1].split( + '"http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/v1/chat/completions"', 1 + )[0] + assert "--max-time" not in request_block + + +def test_sidecar_discovery_and_health_have_no_wall_clock_timeout() -> None: + sidecar = _SIDECAR.read_text(encoding="utf-8") + + lines = sidecar.splitlines() + + def curl_command(url: str) -> tuple[str, int]: + index = next(index for index, line in enumerate(lines) if url in line) + start = index + while start and lines[start - 1].rstrip().endswith("\\"): + start -= 1 + end = index + while lines[end].rstrip().endswith("\\"): + end += 1 + command = " ".join(line.strip().removesuffix("\\") for line in lines[start : end + 1]) + assert re.search(r"\bcurl\b", command) + return command, end + + timeout_option = re.compile( + r"(?:^|\s)(?:-m(?:\s|$)|--[a-z-]*(?:time|timeout)[a-z-]*(?:=|\s|$))" + ) + zdr_command, _ = curl_command("https://openrouter.ai/api/v1/endpoints/zdr") + health_command, health_command_end = curl_command( + 'http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/healthz' + ) + for command in (zdr_command, health_command): + assert timeout_option.search(command) is None + assert re.search(r"(?:^|\s)timeout(?:\s|$)", command) is None + + health_loop = "\n".join(lines[health_command_end + 1 :]).split("\ndone", 1)[0] + assert 'kill -0 "$sidecar_pid"' in health_loop + assert health_loop.count("fail ") == 1 + assert health_loop.index('kill -0 "$sidecar_pid"') < health_loop.index("fail ") + assert not re.search( + r"\b(?:break|exit|timeout)\b|\s-(?:ge|gt|le|lt)\s|\bif\s+\(\(", + health_loop, + ) + + +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 + + +_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 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" +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 +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 +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_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. + + 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}"] + ) + + assert result.returncode == 0, result.stderr + assert "confirmed (attempt 1/3)" in result.stdout + assert report["gateway"] == { + "endpoint": "chat/completions", + "status": "ready", + "attempts": 1, + "finish_reason": "stop", + "reasoning_without_content": False, + } + + +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": [{"finish_reason": "stop", "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, + "finish_reason": "stop", + "reasoning_without_content": False, + } + + +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_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_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, + } + + +@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, + 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. 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"] + + reasoning_only = SimpleNamespace( + id="nvidia_nim_reasoning_only", provider_name="nvidia_nim", model="reasoning/free" + ) + client = _ProbeClient( + { + reasoning_only.id: { + "choices": [ + {"message": {"content": "", "reasoning": "internal reasoning tokens only"}} + ] + } + } + ) + + with pytest.raises(namespace["ReviewPreflightError"], match="no provider route passed") as failure: + preflight([reasoning_only], client=client) + + 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_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 + 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"] + + slow_starter = SimpleNamespace( + id="openrouter_slow_starter", provider_name="openrouter", model="slow/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + { + "choices": [ + { + "finish_reason": "stop", + "message": {"content": "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 + # 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 + + + +@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"] + + exception_type = type(exception_type_name, (RuntimeError,), {"code": http_status}) + flaky = SimpleNamespace( + id="nvidia_nim_low_ceiling", provider_name="nvidia_nim", model="low/free" + ) + client = _SequencedClient( + [ + {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, + exception_type("provider rejected the request"), + ] + ) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([flaky], client=client) + + assert len(client.calls) == 2 + row = failure.value.report["routes"][0] + 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 + 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"] + + 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_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 + 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() + preflight = namespace.get("_preflight_review_agents") + error_type = namespace.get("ReviewPreflightError") + assert callable(preflight), "launcher must expose provider-route preflight" + assert isinstance(error_type, type), "launcher must expose a typed preflight failure" + + agent = SimpleNamespace( + id="openrouter_rejected", provider_name="openrouter", model="rejected/free" + ) + client = _ProbeClient({agent.id: TimeoutError("provider timed out")}) + + with pytest.raises(error_type, match="no provider route passed"): + preflight([agent], client=client) + + +def test_preflight_uses_priced_fallback_only_after_primary_routes_reject() -> None: + """A live primary route wins; priced fallback is evidence-triggered only.""" + namespace = _load_launcher() + preflight = namespace["_preflight_with_fallback"] + primary = SimpleNamespace( + id="openrouter_free", provider_name="openrouter", model="free/model" + ) + fallback = SimpleNamespace( + id="openrouter_priced", provider_name="openrouter", model="priced/model" + ) + client = _ProbeClient( + {primary.id: TimeoutError("unavailable"), fallback.id: _openai_text("OK")} + ) + + viable, report, fallback_used = preflight( + [primary], [fallback], client=client + ) + + assert viable == [fallback] + assert fallback_used is True + assert report["fallback_reason"] == "primary_routes_unavailable" + assert report["primary_attempt"]["ready_count"] == 0 + assert [call[0] for call in client.calls] == [primary, fallback] + + ready_client = _ProbeClient( + {primary.id: _openai_text("OK"), fallback.id: _openai_text("unused")} + ) + viable, report, fallback_used = preflight( + [primary], [fallback], client=ready_client + ) + assert viable == [primary] + assert fallback_used is False + assert "fallback_reason" not in report + assert [call[0] for call in ready_client.calls] == [primary] + + failing_client = _ProbeClient( + {primary.id: TimeoutError("unavailable"), fallback.id: RuntimeError("rejected")} + ) + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight([primary], [fallback], client=failing_client) + assert failure.value.report["ready_count"] == 0 + assert failure.value.report["primary_attempt"]["ready_count"] == 0 + + +def test_fallback_escalation_is_independent_of_primary_catalog_order() -> None: + """Primary starvation cannot consume a fallback candidate's own retry.""" + namespace = _load_launcher() + preflight = namespace["_preflight_with_fallback"] + primary_agents = [ + SimpleNamespace(id=f"primary_{index}", provider_name="openrouter", model="x/free") + for index in range(6) + ] + fallback_agents = [ + SimpleNamespace(id=f"fallback_{index}", provider_name="openrouter", model="y/priced") + for index in range(3) + ] + starved = {"choices": [{"finish_reason": "length", "message": {"content": ""}}]} + client = _ProbeClient({agent.id: dict(starved) for agent in [*primary_agents, *fallback_agents]}) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight(primary_agents, fallback_agents, client=client) + + assert failure.value.report["escalations_used"] == len(fallback_agents) + assert failure.value.report["primary_attempt"]["escalations_used"] == len(primary_agents) + assert len(client.calls) == 2 * (len(primary_agents) + len(fallback_agents)) + assert all( + row.get("error_type") != "escalation_budget_exhausted" + for report in (failure.value.report["primary_attempt"], failure.value.report) + for row in report["routes"] + ) + + + +def test_every_budget_starved_route_gets_its_own_escalation() -> None: + """Catalog order cannot deny a candidate its own evidence-bearing retry.""" + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + agents = [ + SimpleNamespace( + id=f"starved_{index}", provider_name="openrouter", model=f"starved/{index}" + ) + for index in range(6) + ] + starved = { + "choices": [{"finish_reason": "length", "message": {"content": ""}}] + } + client = _ProbeClient({agent.id: dict(starved) for agent in agents}) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight(agents, client=client) + + rows = failure.value.report["routes"] + assert [row["attempts"] for row in rows] == [2] * len(agents) + assert all(row.get("error_type") != "escalation_budget_exhausted" for row in rows) + assert failure.value.report["escalations_used"] == len(agents) + assert len(client.calls) == 2 * len(agents) + + +def test_preflight_keeps_more_than_twelve_admitted_primary_routes() -> None: + """Admission cardinality cannot crash or truncate runtime preflight.""" + namespace = _load_launcher() + preflight = namespace["_preflight_with_fallback"] + agents = [ + SimpleNamespace(id=f"ready_{index}", provider_name="openrouter", model=f"model/{index}") + for index in range(13) + ] + client = _ProbeClient({agent.id: _openai_text("OK") for agent in agents}) + + viable, report, fallback_used = preflight(agents, [], client=client) + + assert viable == agents + assert report["ready_count"] == len(agents) + assert fallback_used is False + assert [call[0] for call in client.calls] == agents + + +def test_auto_fallback_keeps_all_admitted_routes_after_primary_failure() -> None: + """Auto-pool fallback is evidence-triggered, not cardinality-truncated.""" + namespace = _load_launcher() + preflight = namespace["_preflight_with_fallback"] + primary = [ + SimpleNamespace(id=f"free_{index}", provider_name="openrouter", model=f"free/{index}") + for index in range(9) + ] + fallback = [ + SimpleNamespace(id=f"priced_{index}", provider_name="openrouter", model=f"priced/{index}") + for index in range(5) + ] + client = _ProbeClient( + {agent.id: TimeoutError("unavailable") for agent in primary} + | {agent.id: _openai_text("OK") for agent in fallback} + ) + + viable, report, fallback_used = preflight(primary, fallback, client=client) + + assert viable == fallback + assert fallback_used is True + assert report["fallback_reason"] == "primary_routes_unavailable" + assert [call[0] for call in client.calls] == [*primary, *fallback] + + +def test_zdr_admission_selects_priced_tier_when_free_routes_are_not_private() -> None: + """Privacy admission precedes the free-first tier decision.""" + namespace = _load_launcher() + admit = namespace["_zdr_admitted_rows"] + rows = [ + {"provider": "openrouter", "model": "free/non-private"}, + {"provider": "openrouter", "model": "priced/private"}, + ] + + def checker(provider: str, *, model: str, zdr_endpoints: frozenset[str]) -> bool: + return f"{provider}:{model}" in zdr_endpoints + + admitted = admit( + rows, + require_zdr=True, + zdr_endpoints=frozenset({"openrouter:priced/private"}), + checker=checker, + ) + assert admitted == [rows[1]] + + +def test_discovery_counts_survive_stage_specific_policy_reports() -> None: + """Fallback selection preserves full discovery cost-tier evidence.""" + namespace = _load_launcher() + base = {"selected_count": 1, "selected": [{"model": "priced/model"}]} + rows = [ + {"cost_evidence": "free", "provider": "nvidia_nim"}, + {"cost_evidence": "priced", "provider": "openai"}, + {"cost_evidence": "priced", "provider": "openai"}, + {"cost_evidence": "unknown", "provider": "bytez"}, + ] + enriched = namespace["_with_discovery_counts"]( + base, rows, provider_account=policy.provider_account + ) + assert base == {"selected_count": 1, "selected": [{"model": "priced/model"}]} + assert [enriched[key] for key in ( + "total_routes", "total_free_routes", "total_priced_routes", "total_unknown_routes" + )] == [4, 1, 2, 1] + assert enriched["free_account_diversity"] == 1 + + +def test_discovery_counts_recompute_diversity_from_full_discovery_not_the_stage() -> None: + """A stage report's own narrower free-route set must not be trusted. + + Regression for a real bug: the ``auto``-pool primary stage only sees + ZDR-admitted free rows, and the priced-fallback stage sees no free rows + at all, so either stage's internally computed ``free_account_diversity`` + (whatever ``build_zdr_prioritized_catalog`` returned from its own + narrower input) would undercount or read zero even when the full + discovery has multiple credential accounts with free routes. + """ + namespace = _load_launcher() + stage_report_from_priced_only_rows = {"free_account_diversity": 0} + full_discovery_rows = [ + {"cost_evidence": "free", "provider": "nvidia_nim"}, + {"cost_evidence": "free", "provider": "openrouter"}, + {"cost_evidence": "priced", "provider": "openai"}, + ] + enriched = namespace["_with_discovery_counts"]( + stage_report_from_priced_only_rows, + full_discovery_rows, + provider_account=policy.provider_account, + ) + assert enriched["free_account_diversity"] == 2 + + +def test_temporary_fallback_catalog_is_removed_after_loading(tmp_path: Path) -> None: + """The price-only handoff file is removed after success and failure.""" + helper = _load_launcher()["_load_temporary_agents"] + path = tmp_path / "review-catalog.json.priced" + agents = [{"id": "priced_route"}] + + def loader(value: str) -> list[object]: + assert json.loads(Path(value).read_text(encoding="utf-8")) == {"agents": agents} + return [SimpleNamespace(id="priced_route")] + + assert [agent.id for agent in helper(str(path), agents, loader=loader)] == ["priced_route"] + assert not path.exists() + + def failing_loader(value: str) -> list[object]: + assert Path(value).exists() + raise RuntimeError("loader rejected catalog") + + with pytest.raises(RuntimeError, match="loader rejected catalog"): + helper(str(path), agents, loader=failing_loader) + assert not path.exists() + + +def test_preflight_transport_has_no_inference_timeout_and_is_provider_neutral() -> None: + launcher = _LAUNCHER.read_text(encoding="utf-8") + + assert "REVIEW_MAX_OUTPUT_TOKENS = 4096" in launcher + assert "REVIEW_TEMPERATURE = 1.0" in launcher + assert "REVIEW_PREFLIGHT_TIMEOUT_SECONDS" not in launcher + assert "ModelClient(\n timeout=" not in launcher + assert "max_retries=0" in launcher + assert "temperature=REVIEW_TEMPERATURE" in launcher + + +def test_sidecar_preserves_diagnostics_and_probes_the_real_gateway() -> None: + """Artifacts retain safe evidence and readiness exercises the exact HTTP path.""" + launcher = _LAUNCHER.read_text(encoding="utf-8") + sidecar = _SIDECAR.read_text(encoding="utf-8") + + assert "_preflight_with_fallback(" in launcher + assert "preflight-out" in launcher + assert "max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS" in launcher + assert "temperature=REVIEW_TEMPERATURE" in launcher + + assert 'STRIX_EVIDENCE_DIR="${GITHUB_WORKSPACE:-$ORCHESTRATOR_WORK}/strix_runs"' in sidecar + assert 'sidecar_stdout="$STRIX_EVIDENCE_DIR/contextual-orchestrator-sidecar.stdout.log"' in sidecar + assert 'sidecar_stderr="$STRIX_EVIDENCE_DIR/contextual-orchestrator-sidecar.stderr.log"' in sidecar + assert 'preflight_report="$STRIX_EVIDENCE_DIR/contextual-orchestrator-preflight.json"' in sidecar + assert '--preflight-out "$preflight_report"' in sidecar + assert 'gateway_preflight_response="$ORCHESTRATOR_WORK/gateway-preflight.json"' in sidecar + assert '"http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/v1/chat/completions"' in sidecar + assert 'Authorization: Bearer ${ORCHESTRATOR_TOKEN}' in sidecar + assert 'orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"' in sidecar + assert 'gateway_virtual_model="orchestrator/${orchestrator_pool}"' in sidecar + assert '"model":"%s"' in sidecar + assert '"$gateway_virtual_model" > "$gateway_preflight_request"' in sidecar + assert '"model":"orchestrator/free"' not in sidecar + assert "gateway preflight returned unusable chat content" in sidecar + assert 'SIDECAR_LOG_SANITIZER="$ORG_REPO_ROOT/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py"' in sidecar + assert '"$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stdout"' in sidecar + assert '"$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stderr"' in sidecar + assert '> "$sidecar_stdout" 2> "$sidecar_stderr" &' not in sidecar + + +def test_gateway_preflight_rejection_prints_bounded_evidence_to_the_job_log() -> None: + """A rejected gateway preflight must surface error_code/http_status directly. + + Before this, the bounded ``error_code``/``http_status`` pair was written + only into the ``CONTEXTUAL_ORCHESTRATOR_PREFLIGHT_EVIDENCE`` artifact + file, invisible in the job log a CI operator reads first -- exactly the + gap that made a real "every free route rejected" failure look identical + to an opaque "gateway preflight returned HTTP 502" in normal CI output. + """ + sidecar = _SIDECAR.read_text(encoding="utf-8") + + assert ( + 'print(f"[contextual-orchestrator-sidecar] gateway preflight rejected: ' + 'error_code={code} http_status={status}")' + ) in sidecar + # This print is not routed through the sanitizer, so its inputs must stay + # bounded: code is regex-validated and status is a plain int, never raw + # provider response text. + assert ( + 'if not isinstance(code, str) or not re.fullmatch(r"[A-Za-z0-9_.-]{1,64}", code):' + in sidecar + ) + + +def test_sidecar_stream_sanitizer_allowlists_only_bounded_diagnostics() -> None: + """Provider bodies, exception messages, URLs, and secrets never reach artifacts.""" + namespace = _load_sanitizer() + sanitize_line = namespace["sanitize_line"] + + assert sanitize_line( + "request_failed status=500 code=internal_error upstream sk-secret" + ) == "request_failed status=500 code=internal_error" + assert sanitize_line("client_disconnected") == "client_disconnected" + assert sanitize_line("discovery_diagnostics_complete") == "discovery_diagnostics_complete" + assert sanitize_line( + "review sidecar preflight failed: upstream sk-secret" + ) == "review sidecar preflight failed" + assert sanitize_line( + "review sidecar discovery failed: https://provider.invalid/?key=sk-secret" + ) == "review sidecar discovery failed" + assert sanitize_line( + "review sidecar discovered no eligible models; orchestrator/free would fail closed" + ) == "review sidecar discovered no eligible models" + assert sanitize_line( + "review sidecar requires an explicit --auth-token or the KV credential " + "'CONTEXTUAL_ORCHESTRATOR_TOKEN'" + ) == "review sidecar auth token unavailable" + assert sanitize_line( + "review sidecar requires at least one provider credential in the KV" + ) == "review sidecar requires at least one provider credential in the KV" + assert sanitize_line( + "provider_discovery_failed provider=bytez code=http_status_401" + ) == "provider_discovery_failed provider=bytez code=http_status_401" + assert sanitize_line( + "preflight_route_rejected provider=nvidia_nim error_type=ProviderUpstreamError " + "http_status=429 upstream body sk-secret" + ) == "preflight_route_rejected provider=nvidia_nim error_type=ProviderUpstreamError http_status=429" + assert sanitize_line( + "preflight_route_rejected provider=bytez error_type=InvalidChatResponse" + ) == "preflight_route_rejected provider=bytez error_type=InvalidChatResponse" + assert sanitize_line("provider response sk-secret") is None + + +def test_sidecar_stream_sanitizer_summarizes_unstructured_and_traceback_lines( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The streaming entrypoint flushes safe summaries without echoing raw input.""" + namespace = _load_sanitizer() + main = namespace["main"] + secret = "sk-secret-must-not-enter-artifact" + monkeypatch.setattr( + sys, + "stdin", + io.StringIO( + "request_failed status=500 code=internal_error provider body " + f"{secret}\n" + "Traceback (most recent call last):\n" + f" File provider.py, token={secret}\n" + "Traceback (nested):\n" + f"review sidecar preflight failed: {secret}\n" + "client_disconnected\n" + ), + ) + output = io.StringIO() + + with redirect_stdout(output): + assert main() == 0 + + rendered = output.getvalue() + assert rendered.splitlines() == [ + "request_failed status=500 code=internal_error", + "sidecar emitted an unexpected exception", + "review sidecar preflight failed", + "client_disconnected", + "omitted_unstructured_lines=1", + ] + assert secret not in rendered + + +def test_sidecar_stream_sanitizer_omits_no_summary_for_fully_safe_input( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A fully allowlisted stream does not manufacture an omission warning.""" + namespace = _load_sanitizer() + main = namespace["main"] + monkeypatch.setattr(sys, "stdin", io.StringIO("client_disconnected\n")) + output = io.StringIO() + + with redirect_stdout(output): + assert main() == 0 + + assert output.getvalue() == "client_disconnected\n" + + +def test_launcher_has_no_legacy_catalog_admission_caps() -> None: + """Runtime bootstrap must not restore retired catalog admission authority.""" + namespace = _load_launcher() + source = _LAUNCHER.read_text(encoding="utf-8") + + assert "_bounded_primary_catalog_limit" not in namespace + assert "_bounded_fallback_catalog_limit" not in namespace + assert "_catalog_account_cap" not in namespace + assert "REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES" not in source + assert "REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT" not in source + assert "ORCHESTRATOR_CATALOG_LIMIT" not in source + assert "ORCHESTRATOR_CATALOG_ACCOUNT_CAP" not in source diff --git a/tests/test_contextual_orchestrator_agent_id_collisions.py b/tests/test_contextual_orchestrator_agent_id_collisions.py new file mode 100644 index 0000000000..0abe64867e --- /dev/null +++ b/tests/test_contextual_orchestrator_agent_id_collisions.py @@ -0,0 +1,64 @@ +"""Durable regressions for normalized review-agent identity collisions.""" + +from __future__ import annotations + +import json + +import pytest + +from scripts.ci import contextual_orchestrator_review_policy as policy + + +FREE_PRICE = { + "prompt_price_per_1k": 0.0, + "completion_price_per_1k": 0.0, + "currency_code": "USD", +} + + +def _colliding_report() -> dict[str, object]: + """Return distinct routes whose explicit ids normalize to one runtime id.""" + return { + "models": [ + { + "provider": "openrouter", + "model": "vendor/model-a:free", + "agent_id": "or::same", + "is_free": True, + **FREE_PRICE, + }, + { + "provider": "openrouter", + "model": "vendor/model-b:free", + "agent_id": "or--same", + "is_free": True, + **FREE_PRICE, + }, + ] + } + + +def test_catalog_fails_closed_on_normalized_agent_id_collision() -> None: + """Distinct admitted routes may never alias to the same runtime agent id.""" + rows = policy.parse_discovery_report(_colliding_report()) + + with pytest.raises(policy.PolicyError, match="agent id collision after normalization: 'or_same'"): + policy.build_zdr_prioritized_catalog(rows) + + +def test_collision_never_writes_partial_catalog_or_report(tmp_path) -> None: + """Collision validation completes before either public artifact is written.""" + discovery = tmp_path / "discovery.json" + catalog = tmp_path / "agents.json" + report = tmp_path / "report.json" + discovery.write_text(json.dumps(_colliding_report()), encoding="utf-8") + + with pytest.raises(policy.PolicyError, match="agent id collision after normalization"): + policy.build_catalog_from_paths( + str(discovery), + out_path=str(catalog), + report_path=str(report), + ) + + assert not catalog.exists() + assert not report.exists() diff --git a/tests/test_contextual_orchestrator_central_free_only.py b/tests/test_contextual_orchestrator_central_free_only.py new file mode 100644 index 0000000000..a0c7456e6f --- /dev/null +++ b/tests/test_contextual_orchestrator_central_free_only.py @@ -0,0 +1,54 @@ +"""Central review sidecar pool-boundary regression contracts.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from scripts.ci import contextual_orchestrator_review_launcher as launcher + + +ROOT = Path(__file__).resolve().parents[1] +LAUNCHER = ROOT / "scripts" / "ci" / "contextual_orchestrator_review_launcher.py" +SIDECAR = ROOT / "scripts" / "ci" / "contextual_orchestrator_review_sidecar.sh" + + +def test_launcher_rejects_paid_inclusive_pool_at_argument_boundary(capsys) -> None: + """Noema/OpenCode/Strix reject ``auto`` before provider bootstrap can run.""" + argv = [ + "--discovery-out", + "discovery.json", + "--catalog-out", + "catalog.json", + "--report-out", + "report.json", + "--preflight-out", + "preflight.json", + "--pool", + "auto", + ] + + with pytest.raises(SystemExit) as exc_info: + launcher.main(argv) + + assert exc_info.value.code == 2 + stderr = capsys.readouterr().err + assert "--pool" in stderr + assert "invalid choice" in stderr + assert "auto" in stderr + + +def test_launcher_source_does_not_restore_paid_inclusive_choice() -> None: + """Source review also guards against silently widening the central parser.""" + launcher_source = LAUNCHER.read_text(encoding="utf-8") + assert 'parser.add_argument("--pool", choices=("free",), default="free")' in launcher_source + assert 'choices=("free", "auto")' not in launcher_source + + +def test_sidecar_rejects_any_pool_other_than_free() -> None: + """Environment configuration cannot reactivate orchestrator/auto centrally.""" + sidecar = SIDECAR.read_text(encoding="utf-8") + assert 'case "$orchestrator_pool" in' in sidecar + assert 'fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free"' in sidecar + assert 'free|auto)' not in sidecar diff --git a/tests/test_contextual_orchestrator_no_heuristic_admission.py b/tests/test_contextual_orchestrator_no_heuristic_admission.py new file mode 100644 index 0000000000..db08f95412 --- /dev/null +++ b/tests/test_contextual_orchestrator_no_heuristic_admission.py @@ -0,0 +1,92 @@ +"""Regression contracts for evidence-only contextual-orchestrator admission.""" + +from __future__ import annotations + +import pytest + +from scripts.ci import contextual_orchestrator_review_policy as policy + + +def _free_row(index: int, *, provider: str = "openrouter") -> dict[str, object]: + """Return one normalized evidence-eligible free review route.""" + credential = { + "bytez": "BYTEZ_API_KEY", + "nvidia_nim": "NVIDIA_NIM_API_KEY", + "nvidia_nim_sub": "NVIDIA_NIM_API_KEY_SUB", + "openrouter": "OPENROUTER_API_KEY", + }[provider] + return { + "provider": provider, + "model": f"review-model-{index:02d}", + "agent_id": f"{provider}_review_model_{index:02d}", + "is_free": True, + "cost_evidence": policy.COST_FREE, + "prompt_price_per_1k": 0.0, + "completion_price_per_1k": 0.0, + "currency_code": "USD", + "base_url": f"https://{provider}.example/v1", + "credential_key": credential, + "auth_scheme": "Bearer", + } + + +def test_free_pool_admits_every_evidence_eligible_route_despite_legacy_caps() -> None: + """Legacy cap arguments may not evict evidence-eligible free candidates.""" + rows = [_free_row(index) for index in range(13)] + + result = policy.build_zdr_prioritized_catalog( + rows, + pool="free", + limit=1, + account_cap=1, + ) + + assert {entry["model"] for entry in result["agents"]} == { + row["model"] for row in rows + } + assert result["report"]["selected_count"] == len(rows) + + +def test_free_pool_admission_assigns_no_hand_authored_priority() -> None: + """Admission leaves every eligible model neutral for evidence-based routing.""" + rows = [ + _free_row(0, provider="bytez"), + _free_row(1, provider="nvidia_nim"), + _free_row(2, provider="nvidia_nim_sub"), + _free_row(3, provider="openrouter"), + ] + + result = policy.build_zdr_prioritized_catalog(rows, pool="free") + + assert {entry["priority"] for entry in result["agents"]} == {0} + + +def test_normalized_agent_identity_collision_fails_closed() -> None: + """Two distinct routes may not share the runtime identity used for failover.""" + first = _free_row(0) + second = _free_row(1) + first["agent_id"] = "openrouter/model-a" + second["agent_id"] = "openrouter-model-a" + assert first["model"] != second["model"] + + with pytest.raises(policy.PolicyError, match="agent id collision"): + policy.build_zdr_prioritized_catalog([first, second], pool="free") + + +def test_legacy_ignored_inputs_accept_arbitrary_values() -> None: + """Ignored compatibility inputs cannot become an accidental admission contract.""" + rows = [_free_row(index) for index in range(3)] + sentinel = object() + + result = policy.build_zdr_prioritized_catalog( + rows, + pool="free", + limit="retired-limit", + account_cap=sentinel, + ) + + assert [entry["model"] for entry in result["agents"]] == [ + row["model"] for row in rows + ] + assert result["report"]["legacy_limit_ignored"] is True + assert result["report"]["legacy_account_cap_ignored"] is True diff --git a/tests/test_contextual_orchestrator_no_heuristic_preflight_retry.py b/tests/test_contextual_orchestrator_no_heuristic_preflight_retry.py new file mode 100644 index 0000000000..b39980f7c5 --- /dev/null +++ b/tests/test_contextual_orchestrator_no_heuristic_preflight_retry.py @@ -0,0 +1,22 @@ +"""Regression contracts for fail-closed review preflight transport allocation.""" + +from __future__ import annotations + +import inspect + +from scripts.ci import contextual_orchestrator_review_launcher as launcher + + +def test_preflight_has_no_repository_authored_transport_retry_budget() -> None: + """A transient status cannot manufacture an extra model call in central CI.""" + source = inspect.getsource(launcher) + assert "REVIEW_PREFLIGHT_TRANSIENT_RETRIES" not in source + assert "transport_retry_budget" not in source + assert "max_retries=1" not in source + + +def test_preflight_uses_single_attempt_transport_and_preserves_typed_failure() -> None: + """Without an identified retry policy, preflight fails closed after one send.""" + source = inspect.getsource(launcher._send_preflight_request) + assert "proxy_send_once" in source + assert "proxy_send(" not in source diff --git a/tests/test_contextual_orchestrator_review_no_heuristic_compute.py b/tests/test_contextual_orchestrator_review_no_heuristic_compute.py new file mode 100644 index 0000000000..334f20886b --- /dev/null +++ b/tests/test_contextual_orchestrator_review_no_heuristic_compute.py @@ -0,0 +1,126 @@ +"""No-heuristics contracts for the shared contextual-orchestrator review sidecar.""" + +from __future__ import annotations + +import ast +from pathlib import Path +import runpy +from types import SimpleNamespace + + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_LAUNCHER = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_launcher.py" + + +class _SequenceClient: + def __init__(self, outcomes: list[object]) -> None: + self._outcomes = iter(outcomes) + self.calls: list[dict[str, object]] = [] + + def proxy_send_once(self, _agent: object, _endpoint: str, payload: dict[str, object]) -> object: + self.calls.append(dict(payload)) + outcome = next(self._outcomes) + if isinstance(outcome, BaseException): + raise outcome + return outcome + + +def _launcher_namespace() -> dict[str, object]: + return runpy.run_path(str(_LAUNCHER)) + + +def test_preflight_contains_no_repository_authored_sampling_or_token_allocation() -> None: + """Startup admission may observe provider behavior but may not invent TTC knobs.""" + source = _LAUNCHER.read_text(encoding="utf-8") + tree = ast.parse(source) + + forbidden_names = { + "REVIEW_MAX_OUTPUT_TOKENS", + "REVIEW_TEMPERATURE", + "REVIEW_PREFLIGHT_BASE_TOKENS", + "REVIEW_PREFLIGHT_ESCALATED_TOKENS", + "REVIEW_PREFLIGHT_MAX_ESCALATIONS", + "REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES", + "REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT", + } + assigned_names = { + target.id + for node in ast.walk(tree) + if isinstance(node, (ast.Assign, ast.AnnAssign)) + for target in ( + node.targets if isinstance(node, ast.Assign) else [node.target] + ) + if isinstance(target, ast.Name) + } + assert forbidden_names.isdisjoint(assigned_names) + + for node in ast.walk(tree): + if not isinstance(node, ast.Dict): + continue + literal_keys = { + key.value + for key in node.keys + if isinstance(key, ast.Constant) and isinstance(key.value, str) + } + assert "temperature" not in literal_keys + assert "max_tokens" not in literal_keys + + +def test_budget_starvation_evidence_does_not_allocate_an_ad_hoc_second_model_call() -> None: + """Without an identified compute model, a starved probe fails closed after one call.""" + namespace = _launcher_namespace() + preflight = namespace["_preflight_review_agents"] + agent = SimpleNamespace(id="provider_one", provider_name="nvidia_nim", model="provider/model") + client = _SequenceClient( + [ + { + "choices": [ + { + "message": {"content": "", "reasoning": "incomplete"}, + "finish_reason": "length", + } + ] + } + ] + ) + + error_type = namespace["ReviewPreflightError"] + try: + preflight([agent], client=client) + except error_type as exc: + report = exc.report + else: # pragma: no cover - this is the forbidden behavior + raise AssertionError("starved preflight must fail closed without allocating another model call") + + assert len(client.calls) == 1 + assert "max_tokens" not in client.calls[0] + assert "temperature" not in client.calls[0] + assert report["routes"][0]["status"] == "rejected" + assert report["routes"][0]["error_type"] == "insufficient_preflight_evidence" + assert "escalations_used" not in report + assert "escalation_budget" not in report + + +def test_preflight_success_uses_provider_defaults_and_one_model_call() -> None: + """A successful compatibility observation is one provider-default request.""" + namespace = _launcher_namespace() + preflight = namespace["_preflight_review_agents"] + agent = SimpleNamespace(id="provider_one", provider_name="openrouter", model="provider/model") + client = _SequenceClient( + [{"choices": [{"message": {"content": "OK"}, "finish_reason": "stop"}]}] + ) + + viable, report = preflight([agent], client=client) + + assert viable == [agent] + assert len(client.calls) == 1 + assert client.calls[0] == { + "model": "provider/model", + "messages": [ + {"role": "system", "content": "You are a helpful assistant."}, + {"role": "user", "content": "Reply with just 'OK'."}, + ], + "stream": False, + } + assert report["ready_count"] == 1 + assert "escalations_used" not in report diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 4cda949897..cfdb26a0e8 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -89,6 +89,12 @@ def test_normalize_agent_id(candidate: str, provider: str, expected: str) -> Non assert policy._normalize_agent_id(candidate, provider) == expected +def test_normalize_agent_id_fails_closed_when_no_identifier_remains() -> None: + """Punctuation-only identities cannot silently become an empty agent id.""" + with pytest.raises(policy.PolicyError, match="cannot be normalized safely"): + policy._normalize_agent_id("::", "openrouter") + + def test_is_valid_is_free_rejects_non_scalar_markers() -> None: """Non-scalar or missing free markers are not valid discovery evidence.""" assert policy._is_valid_is_free([]) is False @@ -181,37 +187,46 @@ def test_parse_discovery_report_rejects_invalid_rows(report: dict[str, object]) policy.parse_discovery_report(report) -def test_build_catalog_is_zdr_first_and_free_only() -> None: - """ZDR-compliant routes outrank non-ZDR free routes; priced routes stay out.""" +def test_build_catalog_is_free_only_and_records_zdr_without_ranking() -> None: + """Free admission excludes priced/OpenAI rows but does not turn ZDR into a rank.""" parsed = policy.parse_discovery_report(_report()) result = policy.build_zdr_prioritized_catalog( parsed, - limit=12, - account_cap=4, + limit=1, + account_cap=1, zdr_endpoints=ZDR_FEED, ) agents = result["agents"] - assert agents[0]["model"] == "deepseek/deepseek-r1:free" - assert "zdr" in agents[0]["tags"] models = [agent["model"] for agent in agents] + assert models == [ + "deepseek/deepseek-r1:free", + "nvidia/nemotron-3-nano-30b-a3b", + "meta/llama-3.3-70b-instruct", + "qwen2.5-coder", + ] + assert "zdr" in agents[0]["tags"] assert "gpt-4.1" not in models + assert "gpt-4o-mini" not in models assert result["report"]["pool"] == "orchestrator/free" assert result["report"]["zdr_selected_count"] == 1 assert result["report"]["zdr_endpoints_feed_used"] is True assert result["report"]["selected_count"] == len(agents) + assert result["report"]["legacy_limit_ignored"] == 1 + assert result["report"]["legacy_account_cap_ignored"] == 1 + assert {agent["priority"] for agent in agents} == {0} for agent in agents: assert agent["disabled"] is False assert "cost:free" in agent["tags"] assert agent["credential_key"] -def test_build_auto_catalog_admits_price_evidenced_routes() -> None: - """The Strix auto pool can use priced routes without weakening the free pool.""" +def test_build_auto_catalog_admits_price_evidenced_routes_without_ranking() -> None: + """The audit auto pool retains priced routes but admission stays neutral.""" parsed = policy.parse_discovery_report(_report()) result = policy.build_zdr_prioritized_catalog( parsed, - limit=12, - account_cap=4, + limit=1, + account_cap=1, zdr_endpoints=ZDR_FEED, pool="auto", ) @@ -227,14 +242,22 @@ def test_build_auto_catalog_admits_price_evidenced_routes() -> None: assert result["report"]["total_routes"] == 6 assert result["report"]["free_selected_count"] == 5 assert result["report"]["priced_selected_count"] == 1 + assert {agent["priority"] for agent in agents} == {0} -def test_build_auto_catalog_order_is_independent_of_discovery_order() -> None: - """Equivalent route tiers have deterministic provider/model priority.""" +def test_build_auto_catalog_preserves_discovery_provenance_not_priority() -> None: + """Serialization follows discovery provenance while priorities stay neutral.""" parsed = policy.parse_discovery_report(_report()) forward = policy.build_zdr_prioritized_catalog(parsed, pool="auto") reversed_result = policy.build_zdr_prioritized_catalog(reversed(parsed), pool="auto") - assert forward["report"]["selected"] == reversed_result["report"]["selected"] + assert [row["model"] for row in forward["report"]["selected"]] == [ + row["model"] for row in parsed + ] + assert [row["model"] for row in reversed_result["report"]["selected"]] == [ + row["model"] for row in reversed(parsed) + ] + assert {agent["priority"] for agent in forward["agents"]} == {0} + assert {agent["priority"] for agent in reversed_result["agents"]} == {0} @pytest.mark.parametrize( @@ -258,11 +281,11 @@ def test_priced_routes_require_complete_published_price_evidence( def test_build_auto_catalog_keeps_private_targets_zdr_only() -> None: - """Private Strix auto routing still excludes every unattested route.""" + """Private auto admission excludes every unattested route.""" result = policy.build_zdr_prioritized_catalog( policy.parse_discovery_report(_report()), - limit=12, - account_cap=4, + limit=1, + account_cap=1, zdr_endpoints=ZDR_FEED, require_zdr=True, pool="auto", @@ -278,11 +301,10 @@ def test_build_catalog_reports_free_account_diversity() -> None: """Diversity counts independently credentialed accounts with free routes.""" result = policy.build_zdr_prioritized_catalog( policy.parse_discovery_report(_report()), - limit=12, - account_cap=4, zdr_endpoints=ZDR_FEED, ) assert result["report"]["free_account_diversity"] == 5 + assert result["report"]["free_pool_account_diversity"] == 4 def test_build_catalog_counts_same_vendor_credentials_independently() -> None: @@ -306,9 +328,7 @@ def test_build_catalog_counts_same_vendor_credentials_independently() -> None: ] } result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(single_family_report), - limit=12, - account_cap=4, + policy.parse_discovery_report(single_family_report) ) assert result["report"]["free_account_diversity"] == 2 @@ -321,67 +341,78 @@ def test_build_catalog_rejects_unknown_pool() -> None: ) -def test_build_catalog_assigns_unique_priorities() -> None: - """Each selected agent gets a distinct priority so TaskOrchestrator cannot tie on id.""" +@pytest.mark.parametrize(("field", "value"), [("limit", True), ("account_cap", 1.5)]) +def test_build_catalog_ignores_legacy_cap_input_types(field: str, value: object) -> None: + """Retired compatibility inputs cannot regain admission authority through type gates.""" + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(_report()), **{field: value} + ) + assert result["report"][f"legacy_{field}_ignored"] is True + + +def test_build_catalog_assigns_neutral_priorities() -> None: + """Admission cannot create a hand-authored preference for eligible agents.""" result = policy.build_zdr_prioritized_catalog( policy.parse_discovery_report(_report()), - limit=12, - account_cap=4, zdr_endpoints=ZDR_FEED, ) - priorities = [agent["priority"] for agent in result["agents"]] - assert priorities == sorted(priorities, reverse=True) - assert len(priorities) == len(set(priorities)) - assert result["agents"][0]["priority"] == 0 + assert {agent["priority"] for agent in result["agents"]} == {0} assert result["report"]["total_free_routes"] == 5 -def test_build_catalog_applies_account_cap() -> None: - """An account cap keeps one credential from absorbing the pool.""" +def test_build_catalog_ignores_account_cap() -> None: + """A legacy account cap cannot evict an evidence-eligible free route.""" report = { "models": [ - {"provider": "nvidia_nim", "model": f"m{i}", "agent_id": f"nim_a{i}", "is_free": True, **FREE_PRICE} + { + "provider": "nvidia_nim", + "model": f"m{i}", + "agent_id": f"nim_a{i}", + "is_free": True, + **FREE_PRICE, + } for i in range(6) ] + [ { "provider": "nvidia_nim_sub", "model": f"s{i}", - "agent_id": f"nim_b{i}", - "is_free": True, - **FREE_PRICE, + "agent_id": f"nim_b{i}", + "is_free": True, + **FREE_PRICE, } for i in range(6) ] - + [ - {"provider": "openrouter", "model": f"o{i}", "agent_id": f"or_{i}", "is_free": True, **FREE_PRICE} - for i in range(3) - ] } result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(report), limit=12, account_cap=2 + policy.parse_discovery_report(report), limit=1, account_cap=1 ) account_counts: dict[str, int] = {} for agent in result["agents"]: account = policy.provider_account(agent["provider_name"]) account_counts[account] = account_counts.get(account, 0) + 1 - assert account_counts["nvidia_nim"] == 2 - assert account_counts["nvidia_nim_sub"] == 2 - assert account_counts["openrouter"] == 2 + assert account_counts == {"nvidia_nim": 6, "nvidia_nim_sub": 6} + assert result["report"]["selected_count"] == 12 -def test_build_catalog_respects_limit() -> None: - """The catalog never exceeds the configured agent limit.""" +def test_build_catalog_ignores_limit() -> None: + """A legacy total-route limit cannot truncate evidence-eligible admission.""" report = { "models": [ - {"provider": "openrouter", "model": f"m{i}", "agent_id": f"or_{i}", "is_free": True, **FREE_PRICE} + { + "provider": "openrouter", + "model": f"m{i}", + "agent_id": f"or_{i}", + "is_free": True, + **FREE_PRICE, + } for i in range(20) ] } result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(report), limit=5, account_cap=100 + policy.parse_discovery_report(report), limit=1, account_cap=1 ) - assert len(result["agents"]) == 5 + assert len(result["agents"]) == 20 def test_build_catalog_fails_closed_without_free_models() -> None: @@ -400,16 +431,12 @@ def test_build_catalog_fails_closed_without_free_models() -> None: ] } with pytest.raises(policy.PolicyError, match="no free"): - policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(report), limit=12, account_cap=4 - ) + policy.build_zdr_prioritized_catalog(policy.parse_discovery_report(report)) def test_build_catalog_uses_static_table_without_feed() -> None: """Without a feed, OpenRouter is not granted ZDR for every free route.""" - result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(_report()), limit=12, account_cap=4 - ) + result = policy.build_zdr_prioritized_catalog(policy.parse_discovery_report(_report())) assert result["report"]["zdr_endpoints_feed_used"] is False assert result["report"]["zdr_selected_count"] == 0 assert "zdr" not in result["agents"][0]["tags"] @@ -464,8 +491,8 @@ def test_build_catalog_from_paths_writes_both_files(tmp_path) -> None: str(discovery), out_path=str(catalog), report_path=str(report), - limit=12, - account_cap=4, + limit=1, + account_cap=1, zdr_endpoints_path=str(feed), ) assert catalog.exists() @@ -489,13 +516,14 @@ def test_main_success_writes_catalog(tmp_path) -> None: "--report", str(report), "--limit", - "12", + "1", "--account-cap", - "4", + "1", ] ) assert exit_code == 0 - assert catalog.read_text(encoding="utf-8") + payload = json.loads(catalog.read_text(encoding="utf-8")) + assert len(payload["agents"]) == 4 def test_main_policy_error_returns_one(tmp_path) -> None: @@ -537,12 +565,11 @@ def test_main_requires_discovery_report_arg() -> None: with pytest.raises(SystemExit): policy.main(["--out", "x.json", "--report", "y.json"]) + def test_private_catalog_admits_only_attested_zdr_routes() -> None: """Private-target evidence never falls through to a non-ZDR free route.""" result = policy.build_zdr_prioritized_catalog( policy.parse_discovery_report(_report()), - limit=12, - account_cap=4, zdr_endpoints=ZDR_FEED, require_zdr=True, ) @@ -559,7 +586,5 @@ def test_private_catalog_fails_closed_without_attested_zdr_route() -> None: with pytest.raises(policy.PolicyError, match="ZDR"): policy.build_zdr_prioritized_catalog( policy.parse_discovery_report(_report()), - limit=12, - account_cap=4, require_zdr=True, - ) + ) \ No newline at end of file diff --git a/tests/test_contextual_orchestrator_review_policy_deprecation.py b/tests/test_contextual_orchestrator_review_policy_deprecation.py new file mode 100644 index 0000000000..04662ca6fe --- /dev/null +++ b/tests/test_contextual_orchestrator_review_policy_deprecation.py @@ -0,0 +1,26 @@ +"""Regression coverage for visible removal of legacy review-policy knobs.""" + +from __future__ import annotations + +from scripts.ci import contextual_orchestrator_review_policy as policy + + +def test_explicit_legacy_limit_flags_emit_operator_diagnostics(capsys) -> None: + """Ignored flags must not silently hide stale deployment configuration.""" + warn = getattr(policy, "_warn_explicit_legacy_options") + + warn(["--limit", "99", "--account-cap=7"]) + + assert capsys.readouterr().err.splitlines() == [ + "contextual-orchestrator review policy: --limit is deprecated and ignored", + "contextual-orchestrator review policy: --account-cap is deprecated and ignored", + ] + + +def test_default_cli_does_not_emit_legacy_option_diagnostics(capsys) -> None: + """Only explicit stale configuration should create warning noise.""" + warn = getattr(policy, "_warn_explicit_legacy_options") + + warn(["--pool", "free"]) + + assert capsys.readouterr().err == "" diff --git a/tests/test_contextual_orchestrator_review_preflight_concurrency.py b/tests/test_contextual_orchestrator_review_preflight_concurrency.py new file mode 100644 index 0000000000..d2737e94eb --- /dev/null +++ b/tests/test_contextual_orchestrator_review_preflight_concurrency.py @@ -0,0 +1,95 @@ +"""Regression coverage for evidence-backed review-sidecar preflight concurrency.""" + +from __future__ import annotations + +import runpy +import threading +from pathlib import Path +from types import SimpleNamespace + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_LAUNCHER = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_launcher.py" + + +class _ProviderBarrierProbeClient: + """Require independent providers to progress together without same-account bursts.""" + + def __init__(self, provider_count: int) -> None: + self._provider_count = provider_count + self._started_providers: set[str] = set() + self._active_by_provider: dict[str, int] = {} + self._providers_started = threading.Event() + self._lock = threading.Lock() + self.calls: list[str] = [] + self.same_provider_overlap = False + + def proxy_send_once( + self, agent: object, endpoint: str, payload: dict[str, object] + ) -> dict[str, object]: + """Expose both cross-provider progress and same-provider overlap deterministically.""" + assert endpoint == "chat/completions" + assert payload["max_tokens"] == 16 + provider = str(getattr(agent, "provider_name")) + with self._lock: + active = self._active_by_provider.get(provider, 0) + if active: + self.same_provider_overlap = True + self._active_by_provider[provider] = active + 1 + self._started_providers.add(provider) + self.calls.append(str(getattr(agent, "id"))) + if len(self._started_providers) >= self._provider_count: + self._providers_started.set() + + if not self._providers_started.wait(timeout=0.5): + raise RuntimeError("independent provider probes did not start concurrently") + + with self._lock: + self._active_by_provider[provider] -= 1 + return { + "choices": [ + {"finish_reason": "stop", "message": {"content": "OK"}} + ] + } + + +def _load_launcher() -> dict[str, object]: + """Execute the dependency-lazy launcher and return its module namespace.""" + return runpy.run_path(str(_LAUNCHER)) + + +def test_preflight_parallelizes_independent_providers_without_same_account_burst() -> None: + """Provider accounts are concurrent lanes, while routes sharing one stay serialized. + + The review fleet has observed shared-key 429 storms when every model backed by + one credential starts at once. Admission still includes the full catalog; + only transport concurrency is keyed by the independently credentialed + provider/account identity. Distinct providers must make progress together, + and completion timing must not reorder persisted evidence. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + agents = [ + SimpleNamespace(id="provider_a_model_1", provider_name="provider_a", model="model-1"), + SimpleNamespace(id="provider_a_model_2", provider_name="provider_a", model="model-2"), + SimpleNamespace(id="provider_b_model_1", provider_name="provider_b", model="model-1"), + ] + client = _ProviderBarrierProbeClient(provider_count=2) + + viable, report = preflight(agents, client=client) + + assert viable == agents + assert report["probed_count"] == len(agents) + assert report["ready_count"] == len(agents) + assert report["rejected_count"] == 0 + assert [row["agent_id"] for row in report["routes"]] == [agent.id for agent in agents] + assert [row["status"] for row in report["routes"]] == ["ready"] * len(agents) + assert sorted(client.calls) == sorted(agent.id for agent in agents) + assert client.same_provider_overlap is False + + +def test_preflight_worker_cardinality_tracks_provider_accounts_not_route_count() -> None: + """The executor must derive concurrency from evidence identities, not a route cap.""" + source = _LAUNCHER.read_text(encoding="utf-8") + assert "provider_lanes" in source + assert "max_workers=len(provider_lanes)" in source + assert "max_workers=len(agents)" not in source diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 559c2d1e99..af96e6b2bc 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1,1770 +1,70 @@ -"""Regression tests for the Strix contextual-orchestrator runtime boundary.""" +"""Collect runtime-preflight regressions under the no-heuristics contract. + +The adjacent case module preserves historical regression evidence. This shim +continues to execute findings that remain semantically valid while excluding +oracles whose *expected behavior* was the retired priced fallback, 16->4096 +token escalation, explicit serving-generation knobs, or bounded inference-retry +policy. Those cases are replaced by the executable fail-closed contract in +``test_contextual_orchestrator_review_no_heuristic_compute.py``. +""" from __future__ import annotations -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 - -import pytest - -from scripts.ci import contextual_orchestrator_review_policy as policy - -_REPO_ROOT = Path(__file__).resolve().parents[1] -_LAUNCHER = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_launcher.py" -_SIDECAR = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_sidecar.sh" -_SANITIZER = _REPO_ROOT / "scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py" - - -class _ProbeClient: - """Return deterministic per-agent outcomes for runtime preflight tests.""" - - def __init__(self, outcomes: dict[str, object]) -> None: - self.outcomes = 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 configured outcome.""" - self.calls.append((agent, endpoint, payload)) - outcome = self.outcomes[str(getattr(agent, "id"))] - if isinstance(outcome, BaseException): - raise outcome - assert isinstance(outcome, dict) - 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)) - - -def _load_sanitizer() -> dict[str, object]: - """Execute the sidecar stream sanitizer and return its module namespace.""" - return runpy.run_path(str(_SANITIZER)) - - -def _openai_text(content: str) -> dict[str, object]: - """Build the minimal OpenAI chat response shape accepted by preflight.""" - return {"choices": [{"message": {"content": content}}]} - - -def test_routable_discovered_models_excludes_evidence_only_rows() -> None: - """Evidence-only rows (e.g. OpenRouter) must never enter live selection.""" - namespace = _load_launcher() - routable = namespace.get("_routable_discovered_models") - assert callable(routable), "launcher must expose an evidence-only discovery filter" - - evidence_only_model = SimpleNamespace( - id="openrouter_evidence_only", - provider_name="openrouter", - model_id="some/model", - evidence_only=True, - ) - live_model = SimpleNamespace( - id="nvidia_ready", - provider_name="nvidia_nim", - model_id="ready/free", - evidence_only=False, - ) - no_flag_model = SimpleNamespace( - id="bytez_untagged", provider_name="bytez", model_id="untagged/free" - ) - - assert routable([evidence_only_model, live_model, no_flag_model]) == [ - live_model, - no_flag_model, - ] - assert routable(None) == [] - assert routable([]) == [] - - -def test_log_discovery_errors_prints_one_bounded_line_per_provider_failure( - capsys: pytest.CaptureFixture[str], -) -> None: - """A discarded discovery error must become a visible, sanitizer-safe diagnostic.""" - namespace = _load_launcher() - log_discovery_errors = namespace.get("_log_discovery_errors") - assert callable(log_discovery_errors), "launcher must expose a discovery-error logger" - - errors = [ - SimpleNamespace(provider_name="bytez", error_code="http_status_401"), - SimpleNamespace(provider_name="openai", error_code="timeout"), - ] - - log_discovery_errors(errors) - - captured = capsys.readouterr() - assert captured.out == "" - assert captured.err.splitlines() == [ - "provider_discovery_failed provider=bytez code=http_status_401", - "provider_discovery_failed provider=openai code=timeout", - "discovery_diagnostics_complete", - ] - - -def test_log_discovery_errors_emits_only_the_sentinel_on_a_clean_discovery( - capsys: pytest.CaptureFixture[str], -) -> None: - """No providers failed -> just the completion sentinel, no warning lines.""" - namespace = _load_launcher() - log_discovery_errors = namespace.get("_log_discovery_errors") - assert callable(log_discovery_errors) - - log_discovery_errors([]) - - captured = capsys.readouterr() - assert captured.out == "" - assert captured.err == "discovery_diagnostics_complete\n" - - -def test_log_discovery_errors_sentinel_matches_the_sidecar_scripts_constant() -> None: - """The sidecar shell script's poll target must equal this exact literal.""" - namespace = _load_launcher() - sentinel = namespace.get("_DISCOVERY_DIAGNOSTICS_COMPLETE_SENTINEL") - assert sentinel == "discovery_diagnostics_complete" - sidecar_text = _SIDECAR.read_text(encoding="utf-8") - 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() - preflight = namespace.get("_preflight_review_agents") - assert callable(preflight), "launcher must preflight every selected provider route" - - rejected = SimpleNamespace( - id="openrouter_rejected", provider_name="openrouter", model="rejected/free" - ) - malformed = SimpleNamespace( - id="openrouter_malformed", provider_name="openrouter", model="malformed/free" - ) - ready = SimpleNamespace( - id="nvidia_ready", provider_name="nvidia_nim", model="ready/free" - ) - secret = "sk-secret-must-not-enter-evidence" - client = _ProbeClient( - { - rejected.id: RuntimeError(f"upstream rejected {secret}"), - malformed.id: {"choices": []}, - ready.id: _openai_text("OK"), - } - ) - - viable, report = preflight([rejected, malformed, ready], client=client) - - assert viable == [ready] - assert report["probed_count"] == 3 - assert report["ready_count"] == 1 - assert report["rejected_count"] == 2 - assert [row["status"] for row in report["routes"]] == [ - "rejected", - "rejected", - "ready", - ] - assert report["routes"][0]["error_type"] == "RuntimeError" - 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 - assert payload["stream"] is False - assert payload["max_tokens"] == 16 - assert payload["temperature"] == 1.0 - assert payload["messages"] == [ - {"role": "system", "content": "You are a helpful assistant."}, - {"role": "user", "content": "Reply with just 'OK'."}, - ] - assert "tools" not in payload - - -def test_log_preflight_rejections_prints_bounded_summary_to_stderr( - capsys: pytest.CaptureFixture[str], -) -> None: - """A ReviewPreflightError's report must reach the job log, not just the artifact. - - Regression coverage for the gap that made the launcher's own internal - preflight (distinct from the sidecar script's external curl-based gateway - preflight) fail with only "review sidecar preflight failed" visible and - the real per-route rejection reasons hidden behind - omitted_unstructured_lines in the sanitized stream. - """ - namespace = _load_launcher() - log_preflight_rejections = namespace.get("_log_preflight_rejections") - assert callable(log_preflight_rejections) - - secret = "sk-secret-must-not-enter-evidence" - report = { - "routes": [ - { - "agent_id": "nim_nano_free", - "provider": "nvidia_nim", - "model": "nvidia/nemotron-3-nano-30b-a3b", - "status": "rejected", - "error_type": "ProviderUpstreamError", - "http_status": 429, - }, - { - "agent_id": "or_ds_r1", - "provider": "openrouter", - "model": "deepseek/deepseek-r1:free", - "status": "rejected", - "error_type": f"RuntimeError {secret}", - }, - { - "agent_id": "ready_one", - "provider": "openai", - "model": "gpt-4o-mini", - "status": "ready", - }, - ], - } - log_preflight_rejections(report) - captured = capsys.readouterr() - assert captured.out == "" - assert secret not in captured.err - assert ( - "preflight_route_rejected provider=nvidia_nim " - "error_type=ProviderUpstreamError http_status=429" - ) in captured.err - # The openrouter route's error_type ("RuntimeError ") is not a - # Python identifier, so _log_preflight_rejections' own isidentifier() - # guard replaces it with the bounded placeholder "UnknownError" rather - # than printing it as-is -- this helper is itself the bound that keeps - # an unexpected, non-identifier error_type (and anything embedded in it, - # such as the secret above) out of the job log. - assert "preflight_route_rejected provider=openrouter error_type=UnknownError" in captured.err - assert "RuntimeError" not in captured.err - assert "ready_one" not in captured.err - - -def test_log_preflight_rejections_covers_nested_primary_attempt( - capsys: pytest.CaptureFixture[str], -) -> None: - """A fallback-pool failure must also surface the primary pool's rejections.""" - namespace = _load_launcher() - log_preflight_rejections = namespace.get("_log_preflight_rejections") - assert callable(log_preflight_rejections) - - report = { - "routes": [ - { - "provider": "openai", - "status": "rejected", - "error_type": "ProviderUpstreamError", - "http_status": 503, - }, - ], - "primary_attempt": { - "routes": [ - { - "provider": "bytez", - "status": "rejected", - "error_type": "InvalidChatResponse", - }, - ], - }, - } - log_preflight_rejections(report) - captured = capsys.readouterr() - assert "preflight_route_rejected provider=bytez error_type=InvalidChatResponse" in captured.err - assert ( - "preflight_route_rejected provider=openai error_type=ProviderUpstreamError http_status=503" - in captured.err - ) - - -def test_log_preflight_rejections_ignores_malformed_report( - capsys: pytest.CaptureFixture[str], -) -> None: - """A report missing the expected shape must not raise or print anything.""" - namespace = _load_launcher() - log_preflight_rejections = namespace.get("_log_preflight_rejections") - assert callable(log_preflight_rejections) - - log_preflight_rejections({}) - log_preflight_rejections({"routes": "not-a-list"}) - log_preflight_rejections({"routes": ["not-a-dict"]}) - captured = capsys.readouterr() - assert captured.out == "" - assert captured.err == "" - - -def test_gateway_preflight_max_tokens_is_synchronized_with_the_routing_probe() -> None: - """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. - - 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"] - sidecar = _SIDECAR.read_text(encoding="utf-8") - - match = re.search( - r'gateway_virtual_model.*?"max_tokens":(\d+)', sidecar, re.DOTALL - ) - assert match, "sidecar must send one JSON gateway preflight request with an explicit max_tokens" - gateway_preflight_max_tokens = int(match.group(1)) - - assert gateway_preflight_max_tokens == review_max_output_tokens, ( - "gateway preflight max_tokens " - f"({gateway_preflight_max_tokens}) must equal the routing probe's " - f"REVIEW_MAX_OUTPUT_TOKENS ({review_max_output_tokens}); a smaller " - "budget here can reject a route the routing probe already proved " - "ready" - ) - - -def test_gateway_preflight_has_no_inference_timeout() -> None: - """The end-to-end gateway check must not cap real completion latency. - - Regression for the 2026-08-30 gateway-preflight-timeout incident: exact- - evidence reproduction (Strix run 33306775025 on - ContextualWisdomLab/contextual-orchestrator#921, job 99244624298) showed - the routing probe marking a DeepSeek NIM route "ready" in 18s, then the - identical gateway request against that same healthy route being cut off - at exactly curl's configured bound -- "gateway preflight request could - not reach the local sidecar" was that timeout, not a real connectivity - failure. The request therefore has no wall-clock bound. - """ - sidecar = _SIDECAR.read_text(encoding="utf-8") - - request_block = sidecar.rsplit("curl -sS", 1)[1].split( - '"http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/v1/chat/completions"', 1 - )[0] - assert "--max-time" not in request_block - - -def test_sidecar_discovery_and_health_have_no_wall_clock_timeout() -> None: - sidecar = _SIDECAR.read_text(encoding="utf-8") - - lines = sidecar.splitlines() - - def curl_command(url: str) -> tuple[str, int]: - index = next(index for index, line in enumerate(lines) if url in line) - start = index - while start and lines[start - 1].rstrip().endswith("\\"): - start -= 1 - end = index - while lines[end].rstrip().endswith("\\"): - end += 1 - command = " ".join(line.strip().removesuffix("\\") for line in lines[start : end + 1]) - assert re.search(r"\bcurl\b", command) - return command, end - - timeout_option = re.compile( - r"(?:^|\s)(?:-m(?:\s|$)|--[a-z-]*(?:time|timeout)[a-z-]*(?:=|\s|$))" - ) - zdr_command, _ = curl_command("https://openrouter.ai/api/v1/endpoints/zdr") - health_command, health_command_end = curl_command( - 'http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/healthz' - ) - for command in (zdr_command, health_command): - assert timeout_option.search(command) is None - assert re.search(r"(?:^|\s)timeout(?:\s|$)", command) is None - - health_loop = "\n".join(lines[health_command_end + 1 :]).split("\ndone", 1)[0] - assert 'kill -0 "$sidecar_pid"' in health_loop - assert health_loop.count("fail ") == 1 - assert health_loop.index('kill -0 "$sidecar_pid"') < health_loop.index("fail ") - assert not re.search( - r"\b(?:break|exit|timeout)\b|\s-(?:ge|gt|le|lt)\s|\bif\s+\(\(", - health_loop, - ) - - -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 - - -_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 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" -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 -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 -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_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. - - 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}"] - ) - - assert result.returncode == 0, result.stderr - assert "confirmed (attempt 1/3)" in result.stdout - assert report["gateway"] == { - "endpoint": "chat/completions", - "status": "ready", - "attempts": 1, - "finish_reason": "stop", - "reasoning_without_content": False, - } - - -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": [{"finish_reason": "stop", "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, - "finish_reason": "stop", - "reasoning_without_content": False, - } - - -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_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_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, - } - - -@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, - 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. 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"] - - reasoning_only = SimpleNamespace( - id="nvidia_nim_reasoning_only", provider_name="nvidia_nim", model="reasoning/free" - ) - client = _ProbeClient( - { - reasoning_only.id: { - "choices": [ - {"message": {"content": "", "reasoning": "internal reasoning tokens only"}} - ] - } - } - ) - - with pytest.raises(namespace["ReviewPreflightError"], match="no provider route passed") as failure: - preflight([reasoning_only], client=client) - 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_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 - 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"] - - slow_starter = SimpleNamespace( - id="openrouter_slow_starter", provider_name="openrouter", model="slow/free" - ) - client = _SequencedClient( - [ - {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, - { - "choices": [ - { - "finish_reason": "stop", - "message": {"content": "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 - # 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 - - -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"] == "escalation_budget_exhausted" - assert failure.value.report["escalations_used"] == max_escalations - assert len(client.calls) == max_escalations * 2 + 1 - - -@pytest.mark.parametrize( - ("http_status", "exception_type_name"), - [ - (401, "_UnauthorizedError"), - (429, "_ThrottledError"), - (500, "_ServerError"), - (503, "_UnavailableError"), - ], +_CASES_PATH = Path(__file__).with_name( + "_contextual_orchestrator_review_runtime_preflight_cases.py" ) -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"] - - exception_type = type(exception_type_name, (RuntimeError,), {"code": http_status}) - flaky = SimpleNamespace( - id="nvidia_nim_low_ceiling", provider_name="nvidia_nim", model="low/free" - ) - client = _SequencedClient( - [ - {"choices": [{"finish_reason": "length", "message": {"content": ""}}]}, - exception_type("provider rejected the request"), - ] - ) - - with pytest.raises(namespace["ReviewPreflightError"]) as failure: - preflight([flaky], client=client) - - assert len(client.calls) == 2 - row = failure.value.report["routes"][0] - 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 - 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"] - - 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_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 - 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() - preflight = namespace.get("_preflight_review_agents") - error_type = namespace.get("ReviewPreflightError") - assert callable(preflight), "launcher must expose provider-route preflight" - assert isinstance(error_type, type), "launcher must expose a typed preflight failure" - - agent = SimpleNamespace( - id="openrouter_rejected", provider_name="openrouter", model="rejected/free" - ) - client = _ProbeClient({agent.id: TimeoutError("provider timed out")}) - - with pytest.raises(error_type, match="no provider route passed"): - preflight([agent], client=client) - - -def test_preflight_uses_priced_fallback_only_after_primary_routes_reject() -> None: - """A live primary route wins; priced fallback is evidence-triggered only.""" - namespace = _load_launcher() - preflight = namespace["_preflight_with_fallback"] - primary = SimpleNamespace( - id="openrouter_free", provider_name="openrouter", model="free/model" - ) - fallback = SimpleNamespace( - id="openrouter_priced", provider_name="openrouter", model="priced/model" - ) - client = _ProbeClient( - {primary.id: TimeoutError("unavailable"), fallback.id: _openai_text("OK")} - ) - - viable, report, fallback_used = preflight( - [primary], [fallback], client=client - ) - - assert viable == [fallback] - assert fallback_used is True - assert report["fallback_reason"] == "primary_routes_unavailable" - assert report["primary_attempt"]["ready_count"] == 0 - assert [call[0] for call in client.calls] == [primary, fallback] - - ready_client = _ProbeClient( - {primary.id: _openai_text("OK"), fallback.id: _openai_text("unused")} - ) - viable, report, fallback_used = preflight( - [primary], [fallback], client=ready_client - ) - assert viable == [primary] - assert fallback_used is False - assert "fallback_reason" not in report - assert [call[0] for call in ready_client.calls] == [primary] - - failing_client = _ProbeClient( - {primary.id: TimeoutError("unavailable"), fallback.id: RuntimeError("rejected")} - ) - with pytest.raises(namespace["ReviewPreflightError"]) as failure: - preflight([primary], [fallback], client=failing_client) - assert failure.value.report["ready_count"] == 0 - 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"] - 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": ""}}] +_CASES = runpy.run_path(str(_CASES_PATH)) +_LAUNCHER = Path(__file__).resolve().parents[1] / "scripts/ci/contextual_orchestrator_review_launcher.py" + + +def _retired_heuristic_oracle(name: str) -> bool: + """Identify historical tests whose asserted policy is now forbidden. + + This is test collection only, never a production decision rule. The + underlying historical cases remain in-tree as incident evidence; executable + replacements assert one provider-default observation and fail-closed output. + """ + exact = { + "test_preflight_transport_has_no_inference_timeout_and_is_provider_neutral", + "test_preflight_mirrors_runtime_request_and_keeps_only_compatible_routes", + "test_gateway_preflight_max_tokens_is_synchronized_with_the_routing_probe", + "test_gateway_preflight_retries_transport_failures_up_to_a_bounded_attempt_count", + "test_reasoning_without_content_escalates_then_still_fails_closed_if_unresolved", + "test_finish_reason_length_escalates_and_can_succeed", + "test_preflight_uses_priced_fallback_only_after_primary_routes_reject", + "test_fallback_escalation_is_independent_of_primary_catalog_order", + "test_preflight_keeps_more_than_twelve_admitted_primary_routes", + "test_auto_fallback_keeps_all_admitted_routes_after_primary_failure", + "test_sidecar_preserves_diagnostics_and_probes_the_real_gateway", + "test_every_budget_starved_route_gets_its_own_escalation", } - 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) - # 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() - primary = namespace["_bounded_primary_catalog_limit"]( - 99, pool="auto", has_free_rows=True - ) - fallback = namespace["_bounded_fallback_catalog_limit"]( - 99, primary_count=primary + return ( + name in exact + or name.startswith("test_gateway_retry_loop_") + or name.startswith("test_escalated_probe_") ) - assert (primary, fallback) == (8, 4) - assert primary + fallback == namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] -def test_catalog_account_cap_defaults_to_the_caller_supplied_policy_default( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """The per-account cap falls back to ``policy.DEFAULT_ACCOUNT_CAP``, not the total budget. +for _name, _value in _CASES.items(): + if not _name.startswith("__") and not _retired_heuristic_oracle(_name): + globals()[_name] = _value - Regression for a real, observed failure mode - (ContextualWisdomLab/.github#1415, reported as "빈 깡통 경로 너무 많다"): a - sibling helper (``_catalog_family_cap()``) fell back to - ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`` -- the *total* preflight budget -- - instead of the intended per-account cap whenever its env var was unset. - That silently disabled per-account diversification: in a live production - run, two NVIDIA NIM credentials sharing one rate-limited upstream jointly - consumed all 12 preflight slots, of which 10 (83%) were then rejected via - 429/404/timeout. This module's own equivalent helper must never resolve - to the same value as the total-routes budget when given the real - ``policy.DEFAULT_ACCOUNT_CAP``, which is strictly smaller. - """ - namespace = _load_launcher() - monkeypatch.delenv("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", raising=False) - cap = namespace["_catalog_account_cap"](policy.DEFAULT_ACCOUNT_CAP) - assert cap == policy.DEFAULT_ACCOUNT_CAP - assert cap != namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] - assert cap < namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] - -def test_catalog_account_cap_honors_an_explicit_override( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """An operator-set ``ORCHESTRATOR_CATALOG_ACCOUNT_CAP`` still takes effect.""" - namespace = _load_launcher() - monkeypatch.setenv("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", "6") - assert namespace["_catalog_account_cap"](policy.DEFAULT_ACCOUNT_CAP) == 6 - - -def test_main_sources_the_account_cap_default_from_policy_not_a_magic_number() -> None: - """``main()`` must wire the cap default from ``policy.DEFAULT_ACCOUNT_CAP``. - - A hand-typed literal (or, worse, a total-routes-scale constant) can - silently drift out of sync with ``policy.DEFAULT_ACCOUNT_CAP`` with no - test catching it -- the exact drift that produced - ContextualWisdomLab/.github#1415's real preflight-budget waste. This - source-level contract test pins both ``build_zdr_prioritized_catalog`` - call sites in ``main()`` to the single source of truth and forbids the - total-routes constant from ever reappearing as the account-cap fallback. - """ - source = _LAUNCHER.read_text(encoding="utf-8") - assert source.count("account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP)") == 2 - assert "ORCHESTRATOR_CATALOG_FAMILY_CAP" not in source - assert 'os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", "4")' not in source - - -def test_zdr_admission_selects_priced_tier_when_free_routes_are_not_private() -> None: - """Privacy admission precedes the free-first tier decision.""" - namespace = _load_launcher() - admit = namespace["_zdr_admitted_rows"] - rows = [ - {"provider": "openrouter", "model": "free/non-private"}, - {"provider": "openrouter", "model": "priced/private"}, - ] - - def checker(provider: str, *, model: str, zdr_endpoints: frozenset[str]) -> bool: - return f"{provider}:{model}" in zdr_endpoints - - admitted = admit( - rows, - require_zdr=True, - zdr_endpoints=frozenset({"openrouter:priced/private"}), - checker=checker, - ) - assert admitted == [rows[1]] - - -def test_discovery_counts_survive_stage_specific_policy_reports() -> None: - """Fallback selection preserves full discovery cost-tier evidence.""" - namespace = _load_launcher() - base = {"selected_count": 1, "selected": [{"model": "priced/model"}]} - rows = [ - {"cost_evidence": "free", "provider": "nvidia_nim"}, - {"cost_evidence": "priced", "provider": "openai"}, - {"cost_evidence": "priced", "provider": "openai"}, - {"cost_evidence": "unknown", "provider": "bytez"}, - ] - enriched = namespace["_with_discovery_counts"]( - base, rows, provider_account=policy.provider_account - ) - assert base == {"selected_count": 1, "selected": [{"model": "priced/model"}]} - assert [enriched[key] for key in ( - "total_routes", "total_free_routes", "total_priced_routes", "total_unknown_routes" - )] == [4, 1, 2, 1] - assert enriched["free_account_diversity"] == 1 - - -def test_discovery_counts_recompute_diversity_from_full_discovery_not_the_stage() -> None: - """A stage report's own narrower free-route set must not be trusted. - - Regression for a real bug: the ``auto``-pool primary stage only sees - ZDR-admitted free rows, and the priced-fallback stage sees no free rows - at all, so either stage's internally computed ``free_account_diversity`` - (whatever ``build_zdr_prioritized_catalog`` returned from its own - narrower input) would undercount or read zero even when the full - discovery has multiple credential accounts with free routes. - """ - namespace = _load_launcher() - stage_report_from_priced_only_rows = {"free_account_diversity": 0} - full_discovery_rows = [ - {"cost_evidence": "free", "provider": "nvidia_nim"}, - {"cost_evidence": "free", "provider": "openrouter"}, - {"cost_evidence": "priced", "provider": "openai"}, - ] - enriched = namespace["_with_discovery_counts"]( - stage_report_from_priced_only_rows, - full_discovery_rows, - provider_account=policy.provider_account, - ) - assert enriched["free_account_diversity"] == 2 - - -def test_temporary_fallback_catalog_is_removed_after_loading(tmp_path: Path) -> None: - """The price-only handoff file is removed after success and failure.""" - helper = _load_launcher()["_load_temporary_agents"] - path = tmp_path / "review-catalog.json.priced" - agents = [{"id": "priced_route"}] - - def loader(value: str) -> list[object]: - assert json.loads(Path(value).read_text(encoding="utf-8")) == {"agents": agents} - return [SimpleNamespace(id="priced_route")] - - assert [agent.id for agent in helper(str(path), agents, loader=loader)] == ["priced_route"] - assert not path.exists() - - def failing_loader(value: str) -> list[object]: - assert Path(value).exists() - raise RuntimeError("loader rejected catalog") - - with pytest.raises(RuntimeError, match="loader rejected catalog"): - helper(str(path), agents, loader=failing_loader) - assert not path.exists() - - -def test_preflight_transport_has_no_inference_timeout_and_is_provider_neutral() -> None: +def test_preflight_transport_has_no_inference_timeout_or_compute_defaults() -> None: + """Central review inference supplies no repository-authored TTC policy.""" launcher = _LAUNCHER.read_text(encoding="utf-8") - assert "REVIEW_MAX_OUTPUT_TOKENS = 4096" in launcher - assert "REVIEW_TEMPERATURE = 1.0" in launcher assert "REVIEW_PREFLIGHT_TIMEOUT_SECONDS" not in launcher - assert "ModelClient(\n timeout=" not in launcher - assert "max_retries=0" in launcher - assert "temperature=REVIEW_TEMPERATURE" in launcher - - -def test_sidecar_preserves_diagnostics_and_probes_the_real_gateway() -> None: - """Artifacts retain safe evidence and readiness exercises the exact HTTP path.""" - launcher = _LAUNCHER.read_text(encoding="utf-8") - sidecar = _SIDECAR.read_text(encoding="utf-8") - - assert "_preflight_with_fallback(" in launcher - assert "preflight-out" in launcher - assert "max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS" in launcher - assert "temperature=REVIEW_TEMPERATURE" in launcher - - assert 'STRIX_EVIDENCE_DIR="${GITHUB_WORKSPACE:-$ORCHESTRATOR_WORK}/strix_runs"' in sidecar - assert 'sidecar_stdout="$STRIX_EVIDENCE_DIR/contextual-orchestrator-sidecar.stdout.log"' in sidecar - assert 'sidecar_stderr="$STRIX_EVIDENCE_DIR/contextual-orchestrator-sidecar.stderr.log"' in sidecar - assert 'preflight_report="$STRIX_EVIDENCE_DIR/contextual-orchestrator-preflight.json"' in sidecar - assert '--preflight-out "$preflight_report"' in sidecar - assert 'gateway_preflight_response="$ORCHESTRATOR_WORK/gateway-preflight.json"' in sidecar - assert '"http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/v1/chat/completions"' in sidecar - assert 'Authorization: Bearer ${ORCHESTRATOR_TOKEN}' in sidecar - assert 'orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"' in sidecar - assert 'gateway_virtual_model="orchestrator/${orchestrator_pool}"' in sidecar - assert '"model":"%s"' in sidecar - assert '"$gateway_virtual_model" > "$gateway_preflight_request"' in sidecar - assert '"model":"orchestrator/free"' not in sidecar - assert "gateway preflight returned unusable chat content" in sidecar - assert 'SIDECAR_LOG_SANITIZER="$ORG_REPO_ROOT/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py"' in sidecar - assert '"$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stdout"' in sidecar - assert '"$sidecar_python" -u "$SIDECAR_LOG_SANITIZER" > "$sidecar_stderr"' in sidecar - assert '> "$sidecar_stdout" 2> "$sidecar_stderr" &' not in sidecar - - -def test_gateway_preflight_rejection_prints_bounded_evidence_to_the_job_log() -> None: - """A rejected gateway preflight must surface error_code/http_status directly. - - Before this, the bounded ``error_code``/``http_status`` pair was written - only into the ``CONTEXTUAL_ORCHESTRATOR_PREFLIGHT_EVIDENCE`` artifact - file, invisible in the job log a CI operator reads first -- exactly the - gap that made a real "every free route rejected" failure look identical - to an opaque "gateway preflight returned HTTP 502" in normal CI output. - """ - sidecar = _SIDECAR.read_text(encoding="utf-8") - - assert ( - 'print(f"[contextual-orchestrator-sidecar] gateway preflight rejected: ' - 'error_code={code} http_status={status}")' - ) in sidecar - # This print is not routed through the sanitizer, so its inputs must stay - # bounded: code is regex-validated and status is a plain int, never raw - # provider response text. - assert ( - 'if not isinstance(code, str) or not re.fullmatch(r"[A-Za-z0-9_.-]{1,64}", code):' - in sidecar - ) - - -def test_sidecar_stream_sanitizer_allowlists_only_bounded_diagnostics() -> None: - """Provider bodies, exception messages, URLs, and secrets never reach artifacts.""" - namespace = _load_sanitizer() - sanitize_line = namespace["sanitize_line"] - - assert sanitize_line( - "request_failed status=500 code=internal_error upstream sk-secret" - ) == "request_failed status=500 code=internal_error" - assert sanitize_line("client_disconnected") == "client_disconnected" - assert sanitize_line("discovery_diagnostics_complete") == "discovery_diagnostics_complete" - assert sanitize_line( - "review sidecar preflight failed: upstream sk-secret" - ) == "review sidecar preflight failed" - assert sanitize_line( - "review sidecar discovery failed: https://provider.invalid/?key=sk-secret" - ) == "review sidecar discovery failed" - assert sanitize_line( - "review sidecar discovered no eligible models; orchestrator/free would fail closed" - ) == "review sidecar discovered no eligible models" - assert sanitize_line( - "review sidecar requires an explicit --auth-token or the KV credential " - "'CONTEXTUAL_ORCHESTRATOR_TOKEN'" - ) == "review sidecar auth token unavailable" - assert sanitize_line( - "review sidecar requires at least one provider credential in the KV" - ) == "review sidecar requires at least one provider credential in the KV" - assert sanitize_line( - "provider_discovery_failed provider=bytez code=http_status_401" - ) == "provider_discovery_failed provider=bytez code=http_status_401" - assert sanitize_line( - "preflight_route_rejected provider=nvidia_nim error_type=ProviderUpstreamError " - "http_status=429 upstream body sk-secret" - ) == "preflight_route_rejected provider=nvidia_nim error_type=ProviderUpstreamError http_status=429" - assert sanitize_line( - "preflight_route_rejected provider=bytez error_type=InvalidChatResponse" - ) == "preflight_route_rejected provider=bytez error_type=InvalidChatResponse" - assert sanitize_line("provider response sk-secret") is None - - -def test_sidecar_stream_sanitizer_summarizes_unstructured_and_traceback_lines( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """The streaming entrypoint flushes safe summaries without echoing raw input.""" - namespace = _load_sanitizer() - main = namespace["main"] - secret = "sk-secret-must-not-enter-artifact" - monkeypatch.setattr( - sys, - "stdin", - io.StringIO( - "request_failed status=500 code=internal_error provider body " - f"{secret}\n" - "Traceback (most recent call last):\n" - f" File provider.py, token={secret}\n" - "Traceback (nested):\n" - f"review sidecar preflight failed: {secret}\n" - "client_disconnected\n" - ), - ) - output = io.StringIO() - - with redirect_stdout(output): - assert main() == 0 - - rendered = output.getvalue() - assert rendered.splitlines() == [ - "request_failed status=500 code=internal_error", - "sidecar emitted an unexpected exception", - "review sidecar preflight failed", - "client_disconnected", - "omitted_unstructured_lines=1", - ] - assert secret not in rendered - - -def test_sidecar_stream_sanitizer_omits_no_summary_for_fully_safe_input( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """A fully allowlisted stream does not manufacture an omission warning.""" - namespace = _load_sanitizer() - main = namespace["main"] - monkeypatch.setattr(sys, "stdin", io.StringIO("client_disconnected\n")) - output = io.StringIO() - - with redirect_stdout(output): - assert main() == 0 - - assert output.getvalue() == "client_disconnected\n" + assert "REVIEW_PREFLIGHT_TRANSIENT_RETRIES" not in launcher + assert "REVIEW_MAX_OUTPUT_TOKENS" not in launcher + assert "REVIEW_TEMPERATURE" not in launcher + assert "REVIEW_PREFLIGHT_BASE_TOKENS" not in launcher + assert "REVIEW_PREFLIGHT_ESCALATED_TOKENS" not in launcher + assert launcher.count("timeout=None") == 2 + assert launcher.count("max_retries=0") == 2 + assert "max_output_tokens=" not in launcher + assert "temperature=" not in launcher diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 79c74a4d43..369e0f1928 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -5,7 +5,7 @@ secrets (``BYTEZ_API_KEY``, ``NVIDIA_NIM_API_KEY``, ``NVIDIA_NIM_API_KEY_SUB``, ``OPENROUTER_API_KEY``, ``OPENAI_API_KEY``) enter its process-local KV as bootstrap transport, models are auto-discovered, and the ``orchestrator/free`` -fail-closed zero-cost pool (prioritized by the ZDR policy in +fail-closed zero-cost pool (governed by the ZDR policy in ``scripts/ci/zdr_policy.py``) is the review model. """ @@ -360,7 +360,8 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert rows[1]["prompt_price_per_1k"] == 0.002 assert "from contextual_orchestrator.orchestrator import ModelClient, TaskOrchestrator, load_agents" in text assert "from contextual_orchestrator.server import SecurityConfig, serve" in text - assert 'parser.add_argument("--pool", choices=("free", "auto"), default="free")' in text + assert 'parser.add_argument("--pool", choices=("free",), default="free")' in text + assert 'choices=("free", "auto")' not in text assert "orchestrator/{args.pool} would fail closed" in text assert "scripts.ci.contextual_orchestrator_review_policy" in text assert "from scripts.ci import zdr_policy" in text diff --git a/tests/test_contextual_orchestrator_review_transient_preflight.py b/tests/test_contextual_orchestrator_review_transient_preflight.py new file mode 100644 index 0000000000..76a117d1c1 --- /dev/null +++ b/tests/test_contextual_orchestrator_review_transient_preflight.py @@ -0,0 +1,193 @@ +"""Regression tests for provider-neutral preflight evidence and inference deadlines.""" + +from __future__ import annotations + +import ast +import runpy +import urllib.error +from pathlib import Path +from types import SimpleNamespace + +import pytest + + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_LAUNCHER = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_launcher.py" + + +def _load_launcher() -> dict[str, object]: + """Execute the dependency-lazy launcher and return its module namespace.""" + return runpy.run_path(str(_LAUNCHER)) + + +def _http_error(status: int) -> urllib.error.HTTPError: + """Build one deterministic provider HTTP failure.""" + return urllib.error.HTTPError( + "https://provider.example/v1/chat/completions", + status, + "provider failure", + {}, + None, + ) + + +def _openai_text(content: str) -> dict[str, object]: + """Build the smallest usable OpenAI-compatible chat response.""" + return { + "choices": [ + {"finish_reason": "stop", "message": {"content": content}} + ] + } + + +def _agent(*, reasoning_effort_supported: bool | None = None) -> SimpleNamespace: + """Return a provider-neutral route with optional reasoning capability evidence.""" + return SimpleNamespace( + id="provider_route", + provider_name="provider", + model="arbitrary-chat-model", + reasoning_effort_supported=reasoning_effort_supported, + ) + + +class _OneShotProbeClient: + """Model one-shot provider sends and reject retry-enabled transport use.""" + + def __init__(self, outcomes: list[object]) -> None: + """Store deterministic provider outcomes in semantic-payload order.""" + self._outcomes = iter(outcomes) + self.retrying_calls = 0 + self.one_shot_calls = 0 + self.transport_attempts = 0 + self.payloads: list[dict[str, object]] = [] + + def proxy_send_once( + self, agent: object, endpoint: str, payload: dict[str, object] + ) -> dict[str, object]: + """Send one exact payload once and return or raise its observed outcome.""" + del agent, endpoint + self.payloads.append(dict(payload)) + self.one_shot_calls += 1 + self.transport_attempts += 1 + outcome = next(self._outcomes) + if isinstance(outcome, BaseException): + raise outcome + assert isinstance(outcome, dict) + return outcome + + def proxy_send( + self, agent: object, endpoint: str, payload: dict[str, object] + ) -> dict[str, object]: + """Fail if production preflight reintroduces retry-enabled transport.""" + del agent, endpoint, payload + self.retrying_calls += 1 + raise AssertionError("preflight must use the one-shot passthrough seam") + + +def _keyword(call: ast.Call, name: str) -> ast.expr | None: + """Return one keyword expression from an AST call, if present.""" + return next((item.value for item in call.keywords if item.arg == name), None) + + +def _review_model_client_calls() -> list[ast.Call]: + """Return the two review-runtime ModelClient constructor calls.""" + tree = ast.parse(_LAUNCHER.read_text(encoding="utf-8")) + return [ + node + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id == "ModelClient" + and _keyword(node, "max_output_tokens") is not None + ] + + +@pytest.mark.parametrize("reasoning_effort_supported", [None, False, True]) +def test_preflight_rejects_transient_502_after_one_attempt_independent_of_reasoning_capability( + reasoning_effort_supported: bool | None, +) -> None: + """Transient taxonomy is evidence only and cannot manufacture another model call.""" + namespace = _load_launcher() + agent = _agent(reasoning_effort_supported=reasoning_effort_supported) + client = _OneShotProbeClient([_http_error(502)]) + + with pytest.raises(namespace["ReviewPreflightError"]) as excinfo: + namespace["_preflight_review_agents"]([agent], client=client) + + assert client.retrying_calls == 0 + assert client.one_shot_calls == 1 + assert client.transport_attempts == 1 + route = excinfo.value.report["routes"][0] + assert route["status"] == "rejected" + assert route["http_status"] == 502 + assert route["attempts"] == 1 + assert "transport_retry_budget" not in route + + +def test_preflight_does_not_retry_permanent_auth_failure() -> None: + """A 401 remains a single provider call with bounded typed evidence.""" + namespace = _load_launcher() + client = _OneShotProbeClient([_http_error(401)]) + + with pytest.raises(namespace["ReviewPreflightError"]) as excinfo: + namespace["_preflight_review_agents"]([_agent()], client=client) + + assert client.retrying_calls == 0 + assert client.one_shot_calls == 1 + assert client.transport_attempts == 1 + route = excinfo.value.report["routes"][0] + assert route["status"] == "rejected" + assert route["http_status"] == 401 + assert "transport_retry_budget" not in route + + +def test_reasoning_budget_escalation_uses_response_evidence_not_model_name() -> None: + """Semantic token recovery follows the response while each payload stays one-shot.""" + namespace = _load_launcher() + agent = _agent(reasoning_effort_supported=None) + client = _OneShotProbeClient( + [ + { + "choices": [ + { + "finish_reason": "stop", + "message": { + "reasoning": "internal reasoning consumed the base budget", + "content": "", + }, + } + ] + }, + _openai_text("OK"), + ] + ) + + viable, report = namespace["_preflight_review_agents"]([agent], client=client) + + assert viable == [agent] + assert client.retrying_calls == 0 + assert client.one_shot_calls == 2 + assert client.transport_attempts == 2 + assert [payload["max_tokens"] for payload in client.payloads] == [16, 4096] + route = report["routes"][0] + assert route["status"] == "ready" + assert route["attempts"] == 2 + assert route["escalated"] is True + assert route["reasoning_without_content"] is False + assert "transport_retry_budget" not in route + + +def test_review_clients_have_no_inference_deadline_or_transport_retry() -> None: + """Both preflight and serving clients are deadline-free and one-shot.""" + namespace = _load_launcher() + assert "REVIEW_PREFLIGHT_TRANSIENT_RETRIES" not in namespace + + calls = _review_model_client_calls() + assert len(calls) == 2 + for call in calls: + timeout = _keyword(call, "timeout") + assert isinstance(timeout, ast.Constant) + assert timeout.value is None + max_retries = _keyword(call, "max_retries") + assert isinstance(max_retries, ast.Constant) + assert max_retries.value == 0