diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index 3866281cfe..59c56f13af 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -14,12 +14,37 @@ OpenRouter ZDR lookup, DNS/TLS setup, and local `/healthz` checks. ## Superseding decision ADR 0003 governs these operations. Inference, initial ping/preflight, warmup, -retry/repair, provider discovery, OpenRouter ZDR lookup, DNS/TLS setup, and local -health checks have no fixed wall-clock timeout. Work ends only through an -operator action or cancellation of an obsolete PR head. +provider discovery, OpenRouter ZDR lookup, DNS/TLS setup, and local health checks +have no repository-authored total model deadline. Explicit user cancellation, +provider termination, obsolete-head cancellation and administrative termination +remain distinct lifecycle events. -Response validation remains fail closed. Token-budget diagnostics may explain -empty or truncated output, but they do not impose a wall-clock deadline. +Central review preflight is evidence-only. Each admitted route receives one +provider-default semantic observation. The central repository does not author +`max_tokens`, `temperature`, inference retry counts or semantic token +escalation. A reasoning-only, length-exhausted, malformed or transport-failed +response is bounded rejection evidence and does not allocate another model call. +The shell provisioner does not replay a second live `/v1/chat/completions` +request after launcher preflight; `/healthz` plus the persisted per-route report +form the readiness boundary before the real review consumer exercises the +OpenAI-compatible endpoint. -The former attempt counts, retry ceilings, and timeout values in this ADR are -historical evidence only and must not be restored. +The former token budgets, 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. Each route still receives +exactly one provider-default observation, and published evidence is restored to +deterministic input order, so completion timing cannot become routing +preference. Concurrency changes no route membership, priority, cost/ZDR +decision, provider preference or compute allocation. + +The regression uses a synchronization barrier across independent +provider-account lanes rather than a wall-clock threshold. It proves those +lanes can enter probing before either lane is allowed to complete and +deliberately does not claim simultaneous probing of routes that share one +provider account. diff --git a/docs/adr/0029-sidecar-preflight-lazy-fill.md b/docs/adr/0029-sidecar-preflight-lazy-fill.md index 166d49f9a8..bd7ad92fbd 100644 --- a/docs/adr/0029-sidecar-preflight-lazy-fill.md +++ b/docs/adr/0029-sidecar-preflight-lazy-fill.md @@ -1,83 +1,44 @@ -# ADR-0029: Review sidecar preflight fills the served set lazily to a readiness target +# ADR-0029: Review sidecar lazy-fill preflight -- **Status:** Proposed -- **Date:** 2026-09-06 -- **Scope:** `scripts/ci/contextual_orchestrator_review_launcher.py` (`_preflight_review_agents`, the stage limits), `scripts/ci/contextual_orchestrator_review_sidecar.sh` (`ORCHESTRATOR_CATALOG_LIMIT` default), ADR-0003 §2's stage budget sentence -- **Amends:** ADR-0003 (the "twelve-route startup budget" clause). ADR-0005's attempt counts are historical and are not restored. +- **Status:** Superseded on 2026-09-14 by ADR-0003 and the `#1629` one-shot review-admission contract +- **Original date:** 2026-09-06 +- **Scope:** Historical central review-sidecar preflight policy -## Problem +## Historical decision -The review sidecar selected a fixed catalog of twelve routes and probed every one of them, then served whatever was ready. `.github#1939` made the selection diverse (round-robin across credential accounts inside each cost/ZDR tier, four routes per account), which was right, but it exposed a second defect: the per-account slice is filled from an alphabetically sorted model list, and for both NVIDIA NIM keys the first four models are `deepseek-v4-flash`, `deepseek-v4-pro`, `gemma-3-12b`, `gemma-3-4b`. NIM lists the two `gemma-3` models but answers `404` to every chat request on every run observed. Each NVIDIA key therefore served two working routes, both the most contended models, while the pre-#1939 eight-slot fill had reached `meta/llama-3.2-11b`, `llama-3.2-90b` and `meta/muse-glimmer-30b`, which were ready in every Strix artifact of that afternoon. +This ADR proposed a repository-owned lazy-fill algorithm for review-sidecar startup. The proposal bounded candidate and probe counts, used a readiness target, retried or postponed routes after selected transport outcomes, allocated a small initial output-token budget with a larger semantic escalation, and retained an evidence-triggered priced fallback path. -Measured on `ContextualWisdomLab/.github` (lane jan's census on `#1948`, verdict-step conclusions only, draft skips excluded): +The policy was implemented in protected source while this ADR still had `Proposed` status. Subsequent organization evidence showed that the design mixed three authorities that must remain separate: provider/model routing owned by Contextual-Orchestrator, review-workflow admission owned by the central repository, and hosted-runner occupancy/reclamation owned by the workflow control plane. It also caused central CI to author model-compute policy (`max_tokens`, sampling, retry/escalation budgets) and made sidecar provisioning perform more live inference than was required to establish compatibility. -| window | preflight ready of 12 | `noema-review` success / failure | -|---|---|---| -| before `#1939` (`main@f2f91b80`, 2026-09-05T17:25Z) | 6, 6, 5 (16:37–16:56Z artifacts) | 7 / 14 | -| after | 1–3 (23:47Z onward) | 0 / 22 | +The original measurements, amendments, probe-count analyses, `Retry-After` observations, and rate-limit evidence remain available in this file's Git history and in the linked issue/PR evidence (`#1948`, `#1949`, `#1957`). They remain useful incident evidence but are no longer executable policy. -The evening's rate-limit pressure is a confound; the mechanism is not. A fixed slice from a list with dead entries wastes the slice, and probing every candidate regardless of how many are already ready spends per-key rate budget (`#1948`) for nothing. +## Superseding decision -## Constraints +ADR-0003 is normative for model lifecycle and timeout semantics. For central GitHub review admission: -1. No model name is hard-coded anywhere in the fill; a dead candidate is discovered by its probe, not by a list. -2. Probe spend per sidecar boot stays bounded and is stated as a number, because the probes themselves consume the per-key budgets the served routes need (`#1948`). -3. `#1947`'s deferral (a probed route that answered a transient status is kept behind the ready routes) applies unchanged to whatever was probed. -4. ADR-0003's evidence-triggered priced fallback (only after every free candidate rejects) keeps its shape; the two stages still share one startup budget. -5. `ready_count` keeps its meaning (routes proven ready by a probe) so the peers' post-merge discriminators stay comparable. - -## Decision - -The catalog is a **candidate list**, not the served set. `build_zdr_prioritized_catalog` keeps its tier-then-round-robin order (`#1939`) and is asked for up to `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24` candidates (per-account cap unchanged at 8; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default rises from 12 to 24). `_preflight_review_agents` probes candidates **in that order and stops** as soon as `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes have been spent, whichever comes first. The auto pool's split becomes 16 free candidates and up to 8 priced fallback candidates; the production `free` pool (the sidecar default; it has no fallback stage) lists all 24. A silent candidate's probe costs up to one transport timeout (one artifact spent 805 s on 19 probes), so the probe cap bounds preflight wall time as well as request count. - -**Account skip.** *(The "skipped without a probe" and "two probes per account" claims in this paragraph are superseded by the 2026-09-06 amendment below: such a candidate is postponed, and the leftover budget is spent on it.)* A 429 at preflight is a per-key answer, not a per-model one. Once one credential account has answered 429 to `REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429 = 2` consecutive probes, its remaining candidates are skipped without a probe and the walk continues with the other accounts' next candidates; the two probed routes are still deferred. Under the real 2026-09-06 candidate order (lane jan's table on `#1949`, rebuilt from `#1938`'s Strix artifact: both NVIDIA keys list deepseek ×2, gemma-3 ×2 (404), gemma-4-31b (empty), then the llama and muse routes; every OpenRouter free route answers 429) the plain sixteen-probe walk yields about five ready and five deferred and the readiness target is unreachable, because five probes go to an account whose every route had answered 429 in every artifact since 21:00Z and four to the dead gemma-3 entries. With the skip, the same sixteen probes reach both keys' `llama-3.2` routes and the target of eight. This is why the free pool lists 24 candidates while probing at most 16: the tail is reachable exactly when an account is skipped, and the report separates `skipped_count` from the unreached remainder (`candidate_count − probed_count − skipped_count`). A rate-limited hour therefore costs two probes per account instead of the full budget. - -The sidecar's job-log echo of the preflight JSON (`sed -n '1,400p'`, previously 160 lines) now fits 16 probed routes; the artifact copy was always complete. - -The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` now counts probes actually sent, and `rejected_count` is `probed − ready − deferred`. Unprobed candidates get no `routes` row. +1. Each evidence-eligible route receives at most one provider-default semantic compatibility observation during launcher preflight. +2. Central CI does not author `max_tokens`, `temperature`, model-name allowlists, provider/model/group preference, paid fallback, inference retry counts, semantic token escalation, or a total model wall-clock deadline. +3. Reasoning-only, length-exhausted, malformed, transport-failed, or otherwise unusable observations are recorded as bounded rejection evidence and do not allocate another model call. +4. Independently credentialed provider-account lanes may progress concurrently; routes sharing one account remain serialized so startup does not create a same-credential burst. Completion order does not become routing preference. +5. The shell provisioner does not replay `/v1/chat/completions` after launcher preflight. `/healthz` plus persisted per-route evidence form the provisioning readiness boundary. The real review consumer is the first post-provisioning gateway workload. +6. Model timeout defaults to `None`. Explicit user cancellation, provider termination, stale-head cancellation, and administrative workflow termination remain distinct lifecycle events. Progress/idle-based runner reclamation is owned separately and must not become an elapsed-time model cutoff. +7. Provider discovery, credentials, free-pool eligibility, routing, TTC policy, and provider capability belong to the versioned Contextual-Orchestrator owner boundary. Central CI consumes that boundary and fails closed when the required capability is unavailable. ## Consequences -- **Good:** a dead candidate costs one probe and yields its place to the next candidate in the same account's list; a healthy hour stops after about eight to twelve probes instead of always twelve; a bad hour is bounded at sixteen probes per stage. -- **Cost:** in an hour where nothing is ready the sidecar sends up to 16 probes per stage where it sent 12, a third more against already exhausted keys. This is the price of finding routes past the dead ones; `#1948`'s shared rate ledger is the lever above it. The cap is also a wall-time bound: a 16-token probe can hold the full 90 s receive timeout (`#1661` run 34008191123, 04:48Z, both NVIDIA keys' deepseek-v4-pro probes at 90.06 s and 90.10 s), so a fully silent hour costs at most 16 × 90 s = 24 minutes of preflight against 18 today, and the account-skip rule cuts a rate-limited hour to two probes per account. *(That last clause is superseded by the 2026-09-06 amendment: a rate-limited hour now spends the whole probe budget rather than two probes per account.)* -- **Unchanged:** a route that answers the probe and then goes silent at request time still costs the gateway's full retry budget (`contextual-orchestrator#1045`); readiness is measured at 16 tokens (`#1454`). -- **Discriminator:** post-merge, `probed_count` versus `candidate_count` per boot and `ready_count` of the served set, read from the `runtime preflight summary` in the job log or the `noema-sidecar-evidence` artifact, compared with the table above. - -## Alternatives considered - -- **Raise the per-account cap back to 8 with a 12-route limit** — restores the pre-#1939 pool but reintroduces the single-account fill that `#1939` fixed; the 404s would still occupy slots. -- **Exclude models that 404 by name** — a hard-coded exclusion list the next discovery change silently invalidates; rejected by constraint 1. The discovery-side question (why NIM lists models it does not serve) remains open in `contextual-orchestrator`. -- **Family-level interleave inside each account's list before the cap** (jan's second layer) — would make each NVIDIA key's first six candidates span deepseek, gemma, llama, muse, minimax, mistral, but it needs a model-family equivalence derived from names, which ADR-0003/#1468 deliberately avoid; kept in reserve if the post-merge census shows same-family contention as the residual after the account skip. -- **Probe all 24 candidates** — best served set, double the probe spend in the hour that can least afford it; rejected by constraint 2. - -## Amendment 2026-09-06: a set-aside candidate is postponed, not banned - -**Evidence.** Sixteen sidecar artifacts were collected on 2026-09-06 across `.github`, `argos`, `bandscope` and `naruon`; **fourteen** ran the merged rule (two, `argos` 34013128112 and `bandscope` 34013146167, still carry the pre-`#1949` report shape and are excluded). The fourteen fall into three classes, not two. - -| class | boots | `probed / skipped / ready` | second pass? | outcome | -|---|---|---|---|---| -| budget spent in the first pass | 8 | 16 / 4 / 5–6 | no — budget already gone | served; the sixth ready route (`llama-3.2-11b` on the second NVIDIA key, catalog position 17, ready in exactly these 8 artifacts) is reached **only** because four OpenRouter probes were set aside — the benefit the rule was designed for | -| candidates exhausted, budget left | 1 | 12 / 12 / 3 (`argos` 34014143870, 06:56Z) | **yes**, up to 4 probes | served with 5 deferred, but the target of 8 was unmet with 4 probes unspent | -| every account set aside | 5 | 6 / 18 / 0 (`rejected 6`, all 429) | **yes**, up to 10 probes | preflight failed closed | - -So the change is not confined to bursts: one served, ordinary-minute boot also ends its first pass under target with budget in hand. Only a boot that spends all sixteen probes in the first pass is untouched. - -The sidecar stderr of `.github` run 34016207820 shows its six probes (both NVIDIA keys' two deepseek routes, two OpenRouter routes) refused 429 between 07:49:35.111Z and 07:49:35.767Z. Because the walk is a round-robin across three accounts, "two consecutive 429s" on one account is two requests about **310 ms** apart (`nvidia_nim` at .111 and .422), not two probes a tenth of a second apart. The rule set all three accounts aside, the walk ended **with ten of its sixteen probes unspent**, and because deferral requires one ready route (`#1947`) nothing was served either. The five boots of that class span 07:24:50Z to 08:04:41Z. - -A refusal is not a verdict on the account. Run 34016093772 was inside its *own* preflight while that burst happened (its probes run from 07:46:21Z), and its `llama-3.2-11b` probes on the **same two NVIDIA keys** answered ready at 07:50:58.7Z and 07:50:59.0Z — 84 seconds after those keys refused 429 at 07:49:35Z. That boot ended `probed 16 / ready 5`. - -What is **not** measured: whether the ten unspent probes would have found a ready route *inside* the burst itself. No artifact answers it, because nothing records how long a refusal lasts — hence `retry_after_s` below. The pre-`#1949` walk failed similar windows for a different reason (`.github` runs 34006939646 / 34008191123 / 34008575125, 04:24–05:11Z: the same six 429s, then six gemma 404s, `ready 0` at `probed 12`), so the ban is not a regression this amendment invents; it is the ban meeting a 24-candidate list whose tail it can no longer reach. - -**Decision.** A candidate set aside by the account rule is appended to a postponed list in catalog order. Once the first pass ends with the readiness target unmet and probe budget left, the postponed candidates are probed in that order until the budget is spent; no account rule applies in that second pass. A boot that spends all sixteen probes in the first pass is unchanged; the other two classes above gain a second pass. The justification is not that the second pass rescues a burst — that is unmeasured — but that ending a walk under target with probe budget in hand is indefensible when the catalog's tail is where the ready routes live. Constraint 2 holds unchanged: at most sixteen probes per stage, and a silent second-pass probe is bounded by that count, not by a clock (ADR-0003 admits no time rule here). - -**Cost.** The second pass spends probes the walk used to abandon, so it lengthens the boot it rescues and the boot it does not. A refused probe costs about 120 ms. A **silent** one costs up to the full 90 s receive timeout (`#1661` run 34008191123, both NVIDIA keys' `deepseek-v4-pro` probes at 90.06 s and 90.10 s), and the postponed tail is full of them: `google/gemma-4-31b-it` answered `TimeoutError` in 15 of the 19 probes that reached it across these artifacts. The measured burst is therefore not a 1.2-second case — replaying 34016207820's catalog, its second pass would reach both `gemma-4-31b-it` entries, so about 3 minutes — and the worst case is 10 × 90 s ≈ **15 minutes** added to a boot that will still fail, taking a dead window from about 4 minutes to about 19 and holding the runner slot for it. +The former `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`, readiness targets, probe budgets, fixed 16→4096 token escalation, repository-authored sampling values, transient inference retry budgets, account-specific lazy-fill rules, and priced fallback behavior are retired from the central review-admission contract. They must not be restored from historical tests or documentation without a new accepted ADR and owner-boundary evidence. -**The two-stage path costs more than the free pool's figure.** Whenever a stage lists no more candidates than the probe budget — which is exactly the auto split, 16 free primary and 8 priced fallback — the account rule now saves nothing there, because the second pass re-probes everything it set aside. Measured on a two-account, all-429 auto run: `origin/main` sends 8 requests (4 primary, 4 priced), this design sends 24 (16 primary, 8 priced). The priced stage spends paid credit, so it doubles from 4 probes to 8 in a rate-limited hour. That is accepted for the same reason as the free pool — the priced stage only runs after every free route rejected, and stopping it half-probed is the same defect one layer down — but it is a real, stated cost, not a side effect. +This supersession does **not** claim that provider failures disappear or that all review jobs immediately obtain hosted runners. Queue admission and runner occupancy remain separately observable control-plane concerns. Current-head jobs with no runner and no executed steps are incomplete admission evidence, not source GREEN or RED. -Two things are deliberately **not** traded away. The second pass never draws on the shared escalation budget (`REVIEW_PREFLIGHT_MAX_ESCALATIONS`, one counter for the whole run, carried into the priced stage by `#1458`): a postponed candidate that answers with the budget-too-small signature is rejected as `escalation_reserved_for_first_pass` rather than escalating, because otherwise candidates the previous design never probed would take escalations from the priced stage that had them, and a two-stage run measurably stops serving a route it used to serve. +## Verification -That competes directly with the org's 60-job ceiling work, and `#1949`'s measured benefit ("a dead window fails closed in about 4 minutes and returns the slot") is partly traded back for the chance to reach the catalog tail. It stays inside the probe budget this ADR bounds, `postponed_probed_count` plus the provisioning step's duration make the trade visible per boot, and `REVIEW_PREFLIGHT_MAX_PROBES` is the lever if the census says the exchange is bad. +`#1629` owns the source transition and exact-head regression evidence. The acceptance path is: -The report adds `postponed_probed_count`; `skipped_count` now means "postponed and never reached", and `candidate_count − probed_count − skipped_count` keeps its meaning. A refused probe additionally records `retry_after_s` when the response carried a whole-seconds `Retry-After` header (the HTTP-date form and out-of-range values record nothing). Nothing waits on that value; it exists so the next census can answer the question this amendment could not. +- focused one-shot/provider-default preflight contracts, +- normal repository CI/security/supply-chain gates on the same exact head, +- independent review, +- protected integration without bypass or force push, +- immutable Contextual-Orchestrator release for the shared owner contract, +- unchanged downstream-consumer evidence. -**Discriminator.** `postponed_probed_count > 0` marks any boot that reached a second pass, which includes the `12 / 12 / 3` class as well as the burst class. To isolate the all-429 class, read the first `probed_count − postponed_probed_count` rows of `routes` (they are in probe order) and require every one to carry `http_status` 429. The next census asks (a) whether such boots end with `ready_count ≥ 1`, (b) what fraction of 429 rows carry `retry_after_s` and how long the refusals claim to last, (c) whether the healthy-minute figures (`ready 5–6`) are unchanged, and (d) the provisioning step's duration on those boots, so the benefit in (a) and the cost above are read from one table. If (a) is consistently 0 **and** (b) shows providers publishing a usable delay, the follow-up is to spend the second pass after that delay rather than immediately — a decision this ADR deliberately leaves to that data. `#1948`'s shared rate ledger remains the lever above all of it. +Until those gates complete, this ADR's supersession records the intended contract but does not by itself authorize merge or release. 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/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..83381548d9 --- /dev/null +++ b/docs/superpowers/plans/2026-09-02-provider-preflight-resilience.md @@ -0,0 +1,73 @@ +# Provider-Neutral Preflight Resilience Implementation Plan + +**Goal:** Keep central review preflight evidence-only and provider-neutral: one provider-default request per admitted route, no repository-authored token/sampling allocation, no inference retry budget, no fixed total model deadline, and concurrent progress across independently credentialed provider-account lanes. + +**Incident history:** 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. Later organization evidence in `.github#712` showed that review-sidecar provisioning could hold hosted runner slots for hours after the protected Contextual-Orchestrator client moved to an intentionally unbounded default model timeout. That operational evidence does not justify restoring a 90-second model deadline or inventing repository-local retry/token policy. + +**Ownership:** + +- `ContextualWisdomLab/contextual-orchestrator#1106` owns the generic free-pool admission, routing and test-time-compute contract; immutable released gateway/client/schema artifacts are the final shared boundary. +- `ContextualWisdomLab/.github#1629` owns the central review launcher's temporary consumer-side admission/preflight behavior while that migration is incomplete. +- `.github#2139/#2140` own progress/idle continuation and runner-occupancy semantics. A progress/idle control must not become a total elapsed inference deadline. +- `.github#1150/#712` own read-only queue-health evidence and classification, not review-lane mutation. + +## Invariants + +- `ModelClient.timeout=None` means no hidden total wall-clock inference deadline for any model. +- Explicit user cancellation, provider termination, stale-head cancellation and administrative workflow termination remain distinct lifecycle events. +- Preflight sends one semantic provider-default request once. HTTP 502, 503, 429, timeout, connection reset and other transport outcomes remain evidence; they do not allocate another model call in the central launcher. +- HTTP 400, 401, 403 and other permanent failures remain bounded rejection evidence under the owner taxonomy. +- Central preflight does not author `max_tokens`, `temperature`, retry budgets, model-name allowlists or semantic token escalation. Token/sampling/TTC policy belongs to the Contextual-Orchestrator owner boundary. +- `model`, `agent_id`, `provider_name`, `reasoning_effort_supported`, completion timing and discovery order do not decide inference deadlines or compute allocation. +- A response containing reasoning but no usable content is rejected as observed evidence after the single provider-default request; it is not retried with a larger token budget. +- Independent provider-account lanes may probe concurrently; routes sharing one provider account remain serialized. Published results return to catalog order, so completion timing cannot become routing preference. +- The shell provisioner does not replay provider inference after launcher preflight. `/healthz` plus persisted route evidence prove startup readiness; the actual review consumer is the first post-provisioning live gateway workload. +- Provider response bodies, prompts, exception messages, credentials and internal topology are not persisted in preflight evidence. +- Queue/runner admission and repository source correctness are classified separately. A job with no runner and no executed steps is incomplete admission evidence, not source GREEN or RED. +- Historical regression tests whose asserted behavior was fixed token allocation, semantic escalation, priced fallback or shell-level inference retry are retired structurally. Current test collection does not hide them through function-name or prefix suppression. + +## Task 1: One-shot provider-neutral regression + +**Files:** + +- `tests/test_contextual_orchestrator_review_no_heuristic_compute.py` +- `tests/test_contextual_orchestrator_review_transient_preflight.py` +- `tests/test_contextual_orchestrator_review_preflight_concurrency.py` +- `tests/test_contextual_orchestrator_review_runtime_preflight.py` +- `tests/_contextual_orchestrator_review_runtime_preflight_cases.py` + +- [x] Parameterize transient HTTP failure evidence across `reasoning_effort_supported = None, False, True` without model-name policy. +- [x] Prove HTTP 401 remains single-attempt and terminal. +- [x] Prove concurrency across independent provider-account lanes while same-account routes remain serialized. +- [x] Prove the concurrency fixture itself does not inject `max_tokens` or `temperature`. +- [x] Require a reasoning-only/content-less response to be rejected after one provider-default request. +- [x] Remove launcher-side fixed token/sampling constants and semantic escalation. +- [x] Remove the sidecar gateway inference replay and shell-local inference retry budget. +- [x] Replace legacy function-name/prefix suppression with a current executable regression corpus; retain historical incidents in ADR/doctoring records instead of dormant test oracles. + +## Task 2: Progress and runner occupancy + +**Files / owners:** `.github#712`, `.github#1150`, `.github#2139/#2140`, `contextual-orchestrator#1106`. + +- [x] Preserve exact `{repo, PR, head, base, workflow, run, job, runner assignment}` identity when classifying queue delay. +- [x] Preserve current-head review work rather than cancelling it merely to free capacity. +- [ ] Bound avoidable provisioning occupancy structurally through provider-account concurrency and progress/idle semantics, not total inference duration. +- [ ] Demonstrate that unrelated current-head required jobs regain hosted runner admission without changing leaf `runs-on`, weakening gates or synthesizing statuses. +- [ ] Migrate provider probing/credential/routing authority to an immutable released Contextual-Orchestrator gateway contract and remove duplicated central policy. + +## Task 3: Exact-head verification + +Run the focused central contract suite on the unchanged final owner head: + +```bash +python -m pytest -q \ + tests/test_contextual_orchestrator_review_no_heuristic_compute.py \ + 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 normal repository CI, security, supply-chain and independent-review gates to reach terminal success on that same exact head. Queued, skipped, cancelled, predecessor-head or status-only results are not GREEN. The owner branch must contain no purpose-complete source-rewriting workflow, trigger or repair driver before protected integration. + +After the central owner repair is normally integrated and the Contextual-Orchestrator boundary is immutably released, verify an unchanged downstream review consumer. Close the incident only when that consumer obtains real runner assignment and terminal exact-head review evidence without local provider/model hard-coding, paid fallback or timeout rollback. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index e8c462abcc..7e559221ca 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -1,147 +1,30 @@ """Serve the librarian-controlled ``orchestrator/free`` review sidecar. -This launcher runs with the vendored ``contextual-orchestrator`` source on -``PYTHONPATH``; it deliberately mirrors ``contextual_orchestrator.review_gateway`` -(the org's reference CI sidecar) so that the five provider credentials and the -gateway bearer token enter the process-local KV exactly once, in the same -process that performs model discovery and serves requests. Provincial -credentials never cross a process boundary and are never read from ``os.environ`` -at request time — env is bootstrap transport into the KV. - -The difference from ``review_gateway.main()`` is the agent pool: discovery runs -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``. -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 -orchestrator itself. This module is exercised at CI runtime only. +The launcher discovers zero-priced routes through contextual-orchestrator, +records bounded admission evidence, performs one provider-default compatibility +observation per route, and then serves the admitted pool. Provider/model routing, +compute allocation, retry policy, and test-time-compute remain owner-side +contextual-orchestrator responsibilities rather than repository heuristics. """ from __future__ import annotations import argparse -import copy -import dataclasses +from concurrent.futures import ThreadPoolExecutor import json -import logging import os import re import sys from pathlib import Path -from typing import Any, Callable +from typing import Any -from scripts.ci.contextual_orchestrator_review_policy import ( - FREE_POOL_CREDENTIAL_NAMES, - provider_account, -) +from scripts.ci.contextual_orchestrator_review_policy import FREE_POOL_CREDENTIAL_NAMES # The vendored server's generic 64 KiB default is intentionally conservative. # This loopback, bearer-authenticated review sidecar accepts OpenAI's image-input # request ceiling so repository context can include inline image inputs. REVIEW_MAX_BODY_BYTES = 512 * 1024 * 1024 -# Keep ordinary review turns portable across small zero-cost providers. The -# failing Strix run used 32768 for every call, including its two-word warm-up. -REVIEW_MAX_OUTPUT_TOKENS = 4096 -# Provider-neutral sampling: several modern endpoints reject non-default -# temperatures, while 1.0 is the OpenAI-compatible default. -REVIEW_TEMPERATURE = 1.0 -# Lazy fill (ADR-0029): the catalog is a *candidate* list, probed in its -# tier-then-round-robin order until REVIEW_PREFLIGHT_TARGET_READY routes are -# ready or REVIEW_PREFLIGHT_MAX_PROBES probes are spent, whichever comes first. -# A permanently dead candidate (NIM lists gemma-3-12b/4b but answers 404 on -# every run) then costs one probe instead of a served slot, and a healthy hour -# stops early instead of always probing every candidate. MAX_TOTAL_ROUTES is -# the two-stage total (auto pool: 16 free, up to 8 priced; the production -# ``free`` pool lists all 24). A silent candidate's probe costs up to one -# transport timeout (19 probes took 805 s in one artifact), so MAX_PROBES -# bounds preflight wall time as well as request count. Candidates past the -# probe cap are reached only when the account rule below sets earlier ones -# aside, and the report separates ``skipped_count`` (set aside, never probed) -# from the unreached tail so the evidence stays readable. -REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24 -REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT = 16 -REVIEW_PREFLIGHT_TARGET_READY = 8 -REVIEW_PREFLIGHT_MAX_PROBES = 16 -# Once one credential account has answered 429 to this many probes in a row, -# its remaining candidates are set aside so the walk reaches the other -# accounts' next candidates first: under the real 2026-09-06 candidate order -# (jan's table on #1949) the round-robin would otherwise spend five of sixteen -# probes on an account whose every free route answered 429, and the readiness -# target was unreachable; setting them aside lets the same sixteen probes -# reach both keys' llama routes (catalog position 17, ready in eight of the -# fourteen merged-rule artifacts of 2026-09-06 and reached only this way). -# -# But the rule must not END the walk. When every account is set aside the walk -# stops with most of its probe budget unspent and the stage fails closed -- -# and because deferral needs one ready route (#1947), nothing is served -# either. Measured that day: `.github` run 34016207820 sent six probes across -# all three accounts between 07:49:35.111Z and 07:49:35.767Z, every one -# refused 429, and gave up with ten probes unspent; five runs between 07:24Z -# and 08:05Z read probed 6 / skipped 18 / ready 0. Because the walk is a -# round-robin, "two consecutive 429s" on one account is two requests about -# 310 ms apart (nvidia_nim at .111 and .422). -# -# A refusal is not a verdict on the account. Run 34016093772 was inside its -# own preflight during that burst, and its llama probes on the same two NVIDIA -# keys answered ready at 07:50:58.7 and 07:50:59.0 -- 84 s after those keys -# refused 429. Whether the unspent probes would find a ready route *inside* a -# burst is still unmeasured; that is what `retry_after_s` is for. What is -# certain is that failing closed with two thirds of the budget in hand is -# indefensible, and the cost of spending it is bounded by the probe count, not -# a clock: a refused probe costs about 120 ms, a silent one up to the 90 s -# receive timeout, and the postponed tail contains both (google/gemma-4-31b-it -# answered TimeoutError in 15 of the 19 probes that reached it). See ADR-0029's -# amendment for the full cost table. -# -# So a set-aside candidate is postponed, not banned: once the first pass ends -# with the target unmet and probes left, the postponed candidates are probed -# in catalog order until the budget is spent. Probed 429 routes are still -# deferred (#1947); a candidate the budget never reaches is neither probed nor -# served. -REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429 = 2 -# ADR-0005: a single fixed max_tokens cannot fit every model in a heterogeneous -# pool -- some spend internal reasoning tokens before visible content and need -# more, others have a real completion ceiling a large budget would exceed. The -# base probe is deliberately cheap (16 -- the value this codebase ran with -# before #1436, and independently the floor OpenRouter's own schema documents -# for the deprecated max_tokens field: "some providers enforce a minimum of -# 16"): being wrong is fine here because it is diagnosed and escalated below, -# unlike a single guess that fails outright. -REVIEW_PREFLIGHT_BASE_TOKENS = 16 -# Escalated budget used only when the base probe's response was empty because -# choices[0].finish_reason == "length" (OpenAI's documented signature of -# "budget too small", not "candidate unreachable"). Reuses the existing, -# already-proven-working REVIEW_MAX_OUTPUT_TOKENS rather than inventing a new -# number. -REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS -# Shared cap on how many candidates in one preflight run may use the -# escalation retry above. It bounds request count, never model response time. -REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4 -# Probe outcomes the serving gateway itself treats as transient -- it retries -# the same route and then fails over across exactly these statuses -# (contextual_orchestrator.orchestrator.TRANSIENT_HTTP_STATUS at the vendored -# pin; provider_errors.PROVIDER_STATUS_SURFACES marks 429 retryable). A route -# that answered one of them to the 16-token probe is not known to be dead; it -# was rate-limited or unlucky in the second the probe ran, very often because -# the probe itself spent the per-key budget. Discarding it left the serving -# set with nothing to fail over to: on 2026-09-05 a noema-review preflight -# rejected 11 of 12 routes -- six of them with 429 -- served the one ready -# route for 542 s and returned 502. Such routes are kept as *deferred*, ranked -# after every ready route, so failover has somewhere to go. Only a route that -# *answered* with one of these statuses qualifies (a probe that timed out -# records no http_status and stays rejected), so deferral never admits, on the -# strength of a probe that already showed it, the silent route whose serving -# request would spend the gateway's full retry budget in 90 s timeouts. Keep -# this set in sync with the vendored orchestrator's; a status the gateway -# would not retry must not be deferred. -REVIEW_PREFLIGHT_DEFERRABLE_HTTP_STATUS = frozenset({408, 409, 425, 429, 500, 502, 503, 504, 529}) -# Subtracted from a deferred route's catalog priority so the orchestrator's -# ranking (higher priority first; catalog priorities are 0..-11) never places a -# deferred route ahead of a ready one. -REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY = 1000 class ReviewPreflightError(RuntimeError): @@ -169,22 +52,9 @@ def _has_text_output(model: object) -> bool: def _log_discovery_errors(errors: list[object]) -> None: """Print one bounded, secret-free diagnostic per provider discovery failure. - ``discover_all_models()`` isolates one provider's failure from the - others by design, but a caller that discards the returned errors cannot - tell "this provider has zero free models" from "this provider's - discovery silently failed" -- exactly the ambiguity that made a real - incident impossible to diagnose from CI logs alone. Each error's - ``error_code`` is a bounded classification (``http_status_NNN`` / - ``timeout`` / ``transport_error`` / ``invalid_response``) that never - carries raw provider response text, so this is safe to print to stderr. - - Always emits a trailing sentinel line, even with zero errors: the sidecar - shell script's async stream sanitizer processes stderr lines strictly in - order, so once the sanitizer has passed the sentinel through, every - discovery-error line printed here is guaranteed to have already reached - the sanitized file too -- letting the shell script wait for a - deterministic marker instead of racing a fixed-size or fixed-timeout - guess at whether the sanitizer has caught up yet. + The trailing sentinel lets the shell-side sanitizer prove that every + discovery diagnostic preceding it has been drained without depending on a + wall-clock delay or byte-count heuristic. """ for error in errors: print( @@ -197,21 +67,12 @@ def _log_discovery_errors(errors: list[object]) -> None: def _routable_discovered_models(discovered: list[object] | None) -> list[object]: - """Drop evidence-only discovery rows before any live-serving selection. - - Evidence-only rows (e.g. the OpenRouter catalog) exist solely to supply - ZDR evidence for other providers' models; contextual_orchestrator's own - ``agent_from_discovered()`` refuses to turn one into a serving agent. - Filtering here keeps that same invariant in this sidecar's selection path, - which builds its catalog independently rather than calling - ``agent_from_discovered()`` directly. - """ + """Drop evidence-only discovery rows before any live-serving selection.""" return [model for model in (discovered or []) if not getattr(model, "evidence_only", False)] def _route_identity(model: object) -> tuple[str, str]: """Return the provider/model identity used to bind price evidence.""" - return ( str(getattr(model, "provider_name", None) or ""), str(getattr(model, "model_id", None) or ""), @@ -221,22 +82,7 @@ def _route_identity(model: object) -> tuple[str, str]: def _report_rows( discovered: list[object], free_route_identities: frozenset[tuple[str, str]] ) -> list[dict[str, object]]: - """Convert in-process discovered models into price-evidenced report rows. - - Only routes the orchestrator itself marks zero-priced (whole-prompt and - whole-completion published price equal to zero; never name-implied) are - admitted to the ``orchestrator/free`` pool. Provider routing metadata is - read from the discovered model when present and otherwise falls back to the - org ZDR policy table (``scripts/ci/zdr_policy.py``). - - Args: - discovered: Selected ``discover_all_models()`` result. - free_route_identities: Routes the orchestrator attested as zero-priced. - - Returns: - Price-evidenced rows shaped for - ``contextual_orchestrator_review_policy.parse_discovery_report``. - """ + """Convert discovered models into price-evidenced policy report rows.""" from scripts.ci import zdr_policy rows: list[dict[str, object]] = [] @@ -245,9 +91,12 @@ def _report_rows( model_id = str(getattr(model, "model_id", None) or "") if not provider or not model_id: continue - base_url = str(getattr(model, "chat_base_url", None) or zdr_policy.PROVIDER_BASE_URLS[provider]) + base_url = str( + getattr(model, "chat_base_url", None) or zdr_policy.PROVIDER_BASE_URLS[provider] + ) credential_key = str( - getattr(model, "credential_name", None) or zdr_policy.PROVIDER_CREDENTIAL_NAMES[provider] + getattr(model, "credential_name", None) + or zdr_policy.PROVIDER_CREDENTIAL_NAMES[provider] ) auth_scheme = str( getattr(model, "auth_scheme", None) or zdr_policy.PROVIDER_AUTH_SCHEMES[provider] @@ -256,7 +105,9 @@ def _report_rows( { "provider": provider, "model": model_id, - "agent_id": str(getattr(model, "agent_id", None) or f"{provider}_{model_id}"), + "agent_id": str( + getattr(model, "agent_id", None) or f"{provider}_{model_id}" + ), "is_free": (provider, model_id) in free_route_identities, "prompt_price_per_1k": getattr(model, "prompt_price_per_1k", None), "completion_price_per_1k": getattr(model, "completion_price_per_1k", None), @@ -294,55 +145,8 @@ def _safe_http_status(exc: Exception) -> int | None: return None -def _safe_retry_after_seconds(exc: Exception) -> int | None: - """Return the response's ``Retry-After`` delay in whole seconds, if it sent one. - - Recorded so the evidence can answer a question this codebase cannot - answer today: when a preflight probe is refused with 429, do the - providers say how long the refusal lasts? The 2026-09-06 artifacts show - every probe of a burst refused inside a second (`.github` 34016207820 and - four sibling boots), with nothing in the evidence about how long the - refusal window actually was. Only the delta-seconds - form is read; the HTTP-date form and anything out of range record - nothing, because a wrong number here would be worse than no number. - This is evidence only -- no code waits on it (ADR-0003). - - Args: - exc: The exception a probe attempt raised. - - Returns: - The delay in seconds, or ``None`` when the response carried no - usable ``Retry-After`` header. - """ - headers = getattr(exc, "headers", None) - get_header = getattr(headers, "get", None) - if not callable(get_header): - return None - try: - raw = get_header("Retry-After") - except Exception: # noqa: BLE001 - a hostile header mapping is not evidence - return None - # ``isdecimal`` rather than ``isdigit``: a provider controls this header, - # and ``"²".isdigit()`` is True while ``int("²")`` raises. This - # runs inside the probe walk's exception handler, so a ValueError here - # would escape ``_preflight_review_agents`` -- whose callers catch only - # ``ReviewPreflightError`` -- and kill the boot before any evidence file - # is written. Every ``isdecimal`` string is accepted by ``int``. - if not isinstance(raw, str) or not raw.strip().isdecimal(): - return None - seconds = int(raw.strip()) - return seconds if 0 <= seconds <= 86400 else None - - def _response_finish_reason(response: object) -> str | None: - """Return a bounded ``finish_reason`` string from an OpenAI-compatible response. - - Returns ``None`` when no usable ``finish_reason`` is present. A value is - "unknown" rather than the raw provider string whenever it does not look - like a real, short, stable enum token (real values are e.g. ``stop``, - ``length``, ``tool_calls``, ``content_filter``) -- this evidence must - never become an unbounded copy of arbitrary provider text. - """ + """Return a bounded ``finish_reason`` from an OpenAI-compatible response.""" if not isinstance(response, dict): return None choices = response.get("choices") @@ -362,86 +166,23 @@ def _response_finish_reason(response: object) -> str | None: def _record_provider_exception(row: dict[str, object], exc: Exception) -> None: - """Record one sanitized, bounded classification of a provider exception. - - Never overclaims a specific root cause from an HTTP status alone: an - auth failure (401), rate limit (429), or server error (5xx) is not - evidence of a token-budget problem, and this codebase has no validated - signal today (evidence deliberately never carries raw provider error - text) that distinguishes a genuinely budget-specific rejection from any - other non-2xx response -- so this records the exception's own sanitized - type name (or a bounded placeholder when that name is unsafe to log) - plus an optional numeric HTTP status, identically regardless of which - probe attempt (base or escalated) raised it. Mutates ``row`` in place. - - Also clears any ``finish_reason``/``reasoning_without_content`` already - on ``row`` from an EARLIER attempt on the same candidate (a no-op for - the base probe, which never set them yet, but essential for the - escalated probe: an exception here means there is no response object at - all for THIS attempt, so the base attempt's stale diagnostic fields must - not silently linger and look like they describe the outcome being - recorded now -- the same mixed-attempt-telemetry problem already fixed - for the escalated-empty and escalated-success outcomes, closed here too). - - Args: - row: The in-progress per-route evidence row to update. - exc: The exception a probe attempt raised. - """ + """Record a bounded provider-exception classification without raw text.""" row["status"] = "rejected" error_type = type(exc).__name__ row["error_type"] = ( - error_type if error_type.isidentifier() and len(error_type) <= 64 else "provider_error" + error_type + if error_type.isidentifier() and len(error_type) <= 64 + else "provider_error" ) http_status = _safe_http_status(exc) if http_status is not None: row["http_status"] = http_status - retry_after = _safe_retry_after_seconds(exc) - if retry_after is not None: - row["retry_after_s"] = retry_after row.pop("finish_reason", None) row.pop("reasoning_without_content", None) -def _demote_agent(agent: object, penalty: int) -> object: - """Return a copy of ``agent`` whose ``priority`` is lowered by ``penalty``. - - Serving agents are frozen ``ModelAgent`` dataclasses, so the copy goes - through :func:`dataclasses.replace`; the plain objects tests use are - shallow-copied and assigned. A missing ``priority`` counts as 0, matching - the dataclass default. - """ - priority = int(getattr(agent, "priority", 0)) - penalty - if dataclasses.is_dataclass(agent) and not isinstance(agent, type): - return dataclasses.replace(agent, priority=priority) - demoted = copy.copy(agent) - demoted.priority = priority - return demoted - - def _response_has_reasoning_without_content(response: object) -> bool: - """Return whether a response matches the vendored "reasoning, no content" signature. - - Mirrors ``contextual_orchestrator.orchestrator.ModelClient._response_content``'s - own check exactly (a populated ``message.reasoning`` field with no string - ``content``) rather than inferring it from ``finish_reason`` -- a reasoning - model can exhaust its budget mid-reasoning under a ``finish_reason`` other - than ``"length"`` (provider ``finish_reason`` semantics for this case are - not verified as uniform across the pool), so ``finish_reason == "length"`` - alone would miss the exact original failure mode this preflight exists to - diagnose (PR #1436). - - True only when BOTH conditions hold: a populated ``message.reasoning`` - field, AND ``_chat_response_has_text`` is false for this SAME response. - A normal, complete answer that happens to also disclose a reasoning - trace alongside real, non-empty content is never "starved" -- checking - ``reasoning`` alone, with no check that content is actually - absent/empty, would wrongly flag a genuinely healthy response and - pollute this preflight's own evidence. Reusing - ``_chat_response_has_text``'s existing "empty or missing" definition, - rather than duplicating similar-but-subtly-different logic, keeps the - two predicates provably consistent: this one can never be true for a - response the other already accepts as having usable text. - """ + """Return whether reasoning exists while usable visible content does not.""" if not isinstance(response, dict): return False choices = response.get("choices") @@ -456,354 +197,123 @@ def _response_has_reasoning_without_content(response: object) -> bool: return not _chat_response_has_text(response) +def _send_preflight_request( + client: Any, agent: object, payload: dict[str, object] +) -> object: + """Send one exact provider-default compatibility request exactly once.""" + return client.proxy_send_once(agent, "chat/completions", payload) + + +def _preflight_review_agent( + agent: object, *, client: Any +) -> tuple[object | None, dict[str, object]]: + """Observe one route once without allocating repository-side TTC policy.""" + row: dict[str, object] = { + "agent_id": str(getattr(agent, "id", "")), + "provider": str(getattr(agent, "provider_name", "") or "unknown"), + "model": str(getattr(agent, "model", "")), + "attempts": 1, + } + payload: dict[str, object] = { + "model": getattr(agent, "model", ""), + "messages": [ + {"role": "system", "content": "You are a helpful assistant."}, + {"role": "user", "content": "Reply with just 'OK'."}, + ], + "stream": False, + } + try: + response = _send_preflight_request(client, agent, payload) + except Exception as exc: # noqa: BLE001 - sanitize at provider boundary + _record_provider_exception(row, exc) + return None, row + + finish_reason = _response_finish_reason(response) + reasoning_without_content = _response_has_reasoning_without_content(response) + row["finish_reason"] = finish_reason or "unknown" + row["reasoning_without_content"] = reasoning_without_content + if _chat_response_has_text(response): + row["status"] = "ready" + return agent, row + + row["status"] = "rejected" + if finish_reason == "length" or reasoning_without_content: + row["error_type"] = "insufficient_preflight_evidence" + else: + row["error_type"] = "invalid_chat_response" + return None, row + + def _preflight_review_agents( - agents: list[object], *, client: Any, escalations_used: int = 0 + agents: list[object], *, client: Any ) -> tuple[list[object], dict[str, object]]: - """Probe each route with the runtime request contract and keep ready routes. - - ADR-0005: a single fixed ``max_tokens`` cannot fit every model in a - heterogeneous pool. Each candidate gets one cheap base-budget probe - (``REVIEW_PREFLIGHT_BASE_TOKENS``); when that specific candidate's - response is empty for a "budget too small" reason -- either - ``choices[0].finish_reason == "length"`` (OpenAI's documented signature), - or the vendored ``ModelClient._response_content``'s own broader signature - (a populated ``message.reasoning`` with no string ``content``, which a - reasoning model can hit under a different ``finish_reason`` -- provider - ``finish_reason`` semantics for this case are not verified as uniform - across the pool, and this is the exact original failure mode PR #1436 - responded to) -- that *same* candidate is retried once at a larger, - escalated budget (``REVIEW_PREFLIGHT_ESCALATED_TOKENS``) before being - marked rejected -- bounded by a shared ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` - counter, 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. - - Candidates are probed lazily in catalog order (ADR-0029): probing stops - once ``REVIEW_PREFLIGHT_TARGET_READY`` routes are ready or - ``REVIEW_PREFLIGHT_MAX_PROBES`` probes have been spent, so a dead - candidate costs one probe rather than a served slot and a healthy pool is - not probed to exhaustion. An account that has answered 429 to - ``REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429`` consecutive probes has its - remaining candidates postponed behind the other accounts' candidates; - once the first pass ends with the target unmet and budget left, the - postponed candidates are probed in catalog order (a 429 is an answer - about the instant, not the account). Unprobed candidates get no - ``routes`` row; ``skipped_count`` counts the postponed candidates the - budget never reached, ``postponed_probed_count`` the ones it did, and - ``candidate_count - probed_count - skipped_count`` the unreached tail. - - Args: - agents: Selected zero-cost model agents. - client: Vendored ``ModelClient``-compatible transport. - escalations_used: Escalations already spent earlier in this same - preflight run (e.g. by a prior stage), so the shared budget is - honored across calls rather than restarted at zero. - - Returns: - A pair of viable agents and a sanitized preflight report. 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. + """Probe admitted routes once with provider-account bounded concurrency. + + Independent provider accounts progress concurrently. Routes sharing one + provider-account identity remain serial, and outcomes are restored to + catalog order before publication so latency cannot become routing authority. """ - viable: list[object] = [] - routes: list[dict[str, object]] = [] - consecutive_429: dict[str, int] = {} - # Candidates the account rule set aside in the first pass, in catalog - # order. They are probed in a second pass while budget is left and the - # target is unmet; the ones that pass never reaches are the skipped ones. - postponed: list[object] = [] - postponed_probed = 0 - # One entry per probe, in probe order: ``routes[i]`` describes - # ``probed[i]``. A postponed candidate joins both only when its probe - # runs, so the deferral pass below must pair rows with this list, not - # with ``agents``. - probed: list[object] = [] - walk = iter(agents) - second_pass = False - # A dedicated sentinel, not ``None``: ``None`` is a legal element of a - # candidate list and would silently truncate the walk. - exhausted = object() - while True: - if len(viable) >= REVIEW_PREFLIGHT_TARGET_READY or len(routes) >= REVIEW_PREFLIGHT_MAX_PROBES: - break - agent = next(walk, exhausted) - if agent is exhausted: - if second_pass or not postponed: - break - walk = iter(postponed) - second_pass = True - continue - account = provider_account(str(getattr(agent, "provider_name", "") or "unknown")) - if second_pass: - postponed_probed += 1 - elif consecutive_429.get(account, 0) >= REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429: - postponed.append(agent) - continue - # Cleared here; only a 429 answer below restores it, incremented. - streak_429 = consecutive_429.pop(account, 0) - probed.append(agent) - 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, + if not agents: + report: dict[str, object] = { + "contract": "strix-plain-chat-preflight-v2", + "probed_count": 0, + "ready_count": 0, + "rejected_count": 0, + "routes": [], } - 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) - if row.get("http_status") == 429: - consecutive_429[account] = streak_429 + 1 - 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. - # The second pass never draws on the shared escalation budget. That - # budget is one counter for the whole run, spent in catalog order and - # carried into the priced fallback stage (#1458). Candidates in the - # second pass are ones the account rule had set aside and the previous - # design never probed at all, so letting them claim escalations would - # take them from stages that had them before: measured on a two-stage - # run where every primary candidate on one account answered 429, the - # priced fallback candidate that needs its escalation is denied one and - # the run stops serving a route it used to serve. - if ( - not budget_signature - or second_pass - or escalations_used >= REVIEW_PREFLIGHT_MAX_ESCALATIONS - ): - row["status"] = "rejected" - if not budget_signature: - row["error_type"] = "invalid_chat_response" - elif second_pass: - row["error_type"] = "escalation_reserved_for_first_pass" - else: - row["error_type"] = "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["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. - row["finish_reason"] = _response_finish_reason(escalated_response) or "unknown" - row["reasoning_without_content"] = _response_has_reasoning_without_content( - escalated_response + 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]]]]: + """Probe one provider account serially while other accounts progress.""" + 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]] = [] + for ready_agent, row in outcomes: routes.append(row) + if ready_agent is not None: + viable.append(ready_agent) - # Deferral pass: a route rejected with a status the serving gateway would - # retry and fail over across is kept behind the ready routes instead of - # being discarded -- but only once at least one route is ready. With no - # ready route the run still fails this stage exactly as before, so - # _preflight_with_fallback's "priced catalog only after every primary - # route rejects" contract (ADR-0005) is unchanged. ``routes`` holds one - # row per *probed* agent in probe order (every branch above appends once), - # and ``probed`` the matching agents -- a postponed candidate is in both - # once its second-pass probe has run, and in neither otherwise. - deferred: list[object] = [] - if viable: - for agent, row in zip(probed, routes): - if ( - row.get("status") == "rejected" - and row.get("http_status") in REVIEW_PREFLIGHT_DEFERRABLE_HTTP_STATUS - ): - row["status"] = "deferred" - deferred.append(_demote_agent(agent, REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY)) - report: dict[str, object] = { + report = { "contract": "strix-plain-chat-preflight-v2", - "candidate_count": len(agents), - "probed_count": len(routes), + "probed_count": len(agents), "ready_count": len(viable), - "deferred_count": len(deferred), - "rejected_count": len(routes) - len(viable) - len(deferred), - "skipped_count": len(postponed) - postponed_probed, - "postponed_probed_count": postponed_probed, - "target_ready": REVIEW_PREFLIGHT_TARGET_READY, - "probe_budget": REVIEW_PREFLIGHT_MAX_PROBES, - "account_skip_after_429": REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429, - "escalations_used": escalations_used, - "escalation_budget": REVIEW_PREFLIGHT_MAX_ESCALATIONS, + "rejected_count": len(agents) - len(viable), "routes": routes, } if not viable: raise ReviewPreflightError( "no provider route passed the Strix plain-chat preflight", report ) - return [*viable, *deferred], report - - -def _preflight_with_fallback( - primary_agents: list[object], fallback_agents: list[object], *, client: Any -) -> tuple[list[object], dict[str, object], bool]: - """Use the priced catalog only after every primary route rejects. - - 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 16 primary candidates and then - probes 8 fallback candidates still spends at most 4 escalations total (at - most ``REVIEW_PREFLIGHT_MAX_PROBES`` base attempts per stage + 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. - """ - try: - viable, report = _preflight_review_agents(primary_agents, client=client) - return viable, report, False - except ReviewPreflightError as primary_error: - if not fallback_agents: - raise - escalations_used = int(primary_error.report.get("escalations_used", 0)) - try: - viable, report = _preflight_review_agents( - fallback_agents, client=client, escalations_used=escalations_used - ) - except ReviewPreflightError as fallback_error: - fallback_error.report["primary_attempt"] = primary_error.report - raise - report["primary_attempt"] = primary_error.report - report["fallback_reason"] = "primary_routes_unavailable" - return viable, report, True + return viable, report def _log_preflight_rejections(report: dict[str, object]) -> None: - """Print one bounded diagnostic line per rejected preflight route to stderr. - - ``report["routes"]`` rows are already sanitized by ``_preflight_review_agents`` - (stable route identity, a bounded exception class name, an optional numeric - HTTP status -- never provider response bodies, exception messages, URLs, - prompts, or credentials). Before this, that bounded evidence reached only - the ``--preflight-out`` artifact file, invisible in the job log an operator - reads first, so a real "every free route rejected" failure was - indistinguishable from any other cause of ``review sidecar preflight - failed`` in normal CI output. This is printed to stderr (not stdout) so it - reaches the sidecar's sanitized stderr stream the same way discovery and - gateway diagnostics already do. - """ + """Print bounded rejected-route diagnostics to stderr.""" primary_attempt = report.get("primary_attempt") if isinstance(primary_attempt, dict): _log_preflight_rejections(primary_attempt) @@ -811,35 +321,37 @@ def _log_preflight_rejections(report: dict[str, object]) -> None: if not isinstance(routes, list): return for row in routes: - if not isinstance(row, dict) or row.get("status") not in ("rejected", "deferred"): + if not isinstance(row, dict) or row.get("status") != "rejected": continue - event = f"preflight_route_{row['status']}" - # Re-validate rather than trust the caller's own sanitization: this - # print reaches the sidecar's sanitized stderr stream unchanged, so an - # out-of-contract value here (not a plain identifier) must degrade to - # a safe placeholder instead of ever being formatted into the line. provider_value = row.get("provider") provider = ( provider_value - if isinstance(provider_value, str) and re.fullmatch(r"[a-z][a-z0-9_]{0,63}", provider_value) + if isinstance(provider_value, str) + and re.fullmatch(r"[a-z][a-z0-9_]{0,63}", provider_value) else "unknown" ) error_type_value = row.get("error_type") error_type = ( error_type_value - if isinstance(error_type_value, str) and error_type_value.isidentifier() and len(error_type_value) <= 64 + if isinstance(error_type_value, str) + and error_type_value.isidentifier() + and len(error_type_value) <= 64 else "UnknownError" ) http_status = row.get("http_status") - if isinstance(http_status, int) and not isinstance(http_status, bool) and 100 <= http_status <= 599: + if ( + isinstance(http_status, int) + and not isinstance(http_status, bool) + and 100 <= http_status <= 599 + ): print( - f"{event} provider={provider} " + f"preflight_route_rejected provider={provider} " f"error_type={error_type} http_status={http_status}", file=sys.stderr, ) else: print( - f"{event} provider={provider} error_type={error_type}", + f"preflight_route_rejected provider={provider} error_type={error_type}", file=sys.stderr, ) @@ -851,138 +363,13 @@ 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) - # ADR-0029: the production pool is ``free`` (no fallback stage) and lists - # the full two-stage budget. Candidates past REVIEW_PREFLIGHT_MAX_PROBES - # are reached only when the account-skip rule frees probes; the report's - # ``skipped_count`` keeps that tail distinguishable from an early stop. - 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))) - - -DEFAULT_SIDECAR_LOG_LEVEL = "DEBUG" -SIDECAR_LOG_FORMAT = "%(asctime)s %(levelname)s %(name)s %(message)s" - - -def _sidecar_log_level() -> str: - """Return the log level the review sidecar configures for its orchestrator process. - - Defaults to ``DEBUG`` because that is where ``contextual_orchestrator`` - records the per-request trace a failed review needs afterwards: every - provider attempt (``provider_attempt``), its classified failure - (``provider_attempt_failed`` with error type and transient flag), backoff, - and circuit events are ``_LOGGER.debug`` calls, while the default - ``WARNING`` level keeps only ``provider_exhausted``/``circuit_opened``. At - the vendored pin none of those DEBUG sites logs a prompt, payload, or - response body; the one free-text field is ``provider_attempt_failed``'s - ``error_message`` (the exception text, which can quote an upstream error - body), and the sidecar pipes this process's stderr through the allow-list - sanitizer before it reaches disk, so only lines the sanitizer recognises - -- and only their structured fields -- become CI evidence. On - 2026-09-05 a 3122 s ``noema-review`` failure could not be attributed to - "six ready routes, two retry layers, 548 s per hop" from the job log alone - because this trace was never emitted. Override with - ``ORCHESTRATOR_SIDECAR_LOG_LEVEL``. - """ - return os.environ.get("ORCHESTRATOR_SIDECAR_LOG_LEVEL", DEFAULT_SIDECAR_LOG_LEVEL) - - -def _configure_sidecar_logging(configure_logging: Callable[[str], None]) -> str: - """Configure the orchestrator process's logging for CI evidence. - - ``configure_logging`` is ``contextual_orchestrator.debug_logging.configure_logging`` - (injected so this module stays importable without the vendored package): - it installs the root level with ``basicConfig(force=True)``. Its default - formatter carries no timestamp, and a per-attempt trace without - timestamps cannot yield per-hop durations, so every root handler is then - given :data:`SIDECAR_LOG_FORMAT`. - - Returns: - The level name that was applied. - - Raises: - SystemExit: If ``ORCHESTRATOR_SIDECAR_LOG_LEVEL`` is not a level name - the orchestrator accepts; a misspelt level must not silently leave - the process at ``WARNING``. - """ - level = _sidecar_log_level() - try: - configure_logging(level) - except ValueError as exc: - raise SystemExit(f"ORCHESTRATOR_SIDECAR_LOG_LEVEL is invalid: {exc}") from None - formatter = logging.Formatter(SIDECAR_LOG_FORMAT) - for handler in logging.getLogger().handlers: - handler.setFormatter(formatter) - return level - - def _with_discovery_counts( report: dict[str, object], rows: list[dict[str, Any]], *, provider_account: Any, ) -> dict[str, object]: - """Copy a stage report while restoring full discovery-tier counts. - - ``free_account_diversity`` is recomputed here from the full discovery-wide - ``rows``, not trusted from the stage report: the primary ``auto``-pool - stage may have selected only ZDR-admitted free rows (undercounting - diversity whenever ``--require-zdr`` excludes some free routes) and the - priced-fallback stage selects only priced rows (so its own internally - computed diversity is always zero) -- either stage report's - ``free_account_diversity``, as returned by ``build_zdr_prioritized_catalog`` - from whatever narrower row set it was given, would otherwise contradict - that field's documented "among *all* discovered free routes" contract. - """ + """Copy a stage report while restoring discovery-wide route counts.""" free_rows = [row for row in rows if row.get("cost_evidence") == "free"] free_pool_rows = [ row @@ -995,8 +382,12 @@ def _with_discovery_counts( { "total_routes": len(rows), "total_free_routes": len(free_rows), - "total_priced_routes": sum(row.get("cost_evidence") == "priced" for row in rows), - "total_unknown_routes": sum(row.get("cost_evidence") == "unknown" for row in rows), + "total_priced_routes": sum( + row.get("cost_evidence") == "priced" for row in rows + ), + "total_unknown_routes": sum( + row.get("cost_evidence") == "unknown" for row in rows + ), "free_account_diversity": len( {provider_account(str(row["provider"])) for row in free_rows} ), @@ -1047,38 +438,48 @@ def _load_temporary_agents( def main(argv: list[str] | None = None) -> int: - """Bootstrap the KV, discover and preflight free models, then serve. - - Args: - argv: CLI arguments (``--host``, ``--port``, ``--auth-token``, - ``--catalog-out``, ``--report-out``, ``--preflight-out``, - ``--discovery-out``, ``--zdr-endpoints``). - - Returns: - 0 when the server exits cleanly; 1 on any configuration error. - - Raises: - SystemExit: If the vendored library is missing, no provider credential - is in the KV, no free model was discovered, no route passes runtime - preflight, or no auth token is available — the sidecar must fail - closed rather than boot a mock or unaudited pool. - """ - parser = argparse.ArgumentParser(description="Serve the contextual-orchestrator review sidecar.") + """Bootstrap credentials, discover/preflight free models, and serve.""" + parser = argparse.ArgumentParser( + description="Serve the contextual-orchestrator review sidecar." + ) parser.add_argument("--host", default="127.0.0.1") parser.add_argument("--port", type=int, default=18080) - parser.add_argument("--auth-token", default="", help="Explicit bearer token; else resolve from the KV") - parser.add_argument("--discovery-out", required=True, help="Path to write the free-only discovery report JSON") - parser.add_argument("--catalog-out", required=True, help="Path to write the agents catalog JSON") - parser.add_argument("--report-out", required=True, help="Path to write the policy evidence JSON") - 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( + "--auth-token", + default="", + help="Explicit bearer token; else resolve from the KV", + ) + parser.add_argument( + "--discovery-out", + required=True, + help="Path to write the free-only discovery report JSON", + ) + parser.add_argument( + "--catalog-out", required=True, help="Path to write the agents catalog JSON" + ) + parser.add_argument( + "--report-out", required=True, help="Path to write the policy evidence JSON" + ) + 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 from contextual_orchestrator.chat_capability import is_general_chat_agent_model_id - from contextual_orchestrator.model_discovery import discover_all_models, free_discovered_models + from contextual_orchestrator.credentials import get_credential + from contextual_orchestrator.model_discovery import ( + discover_all_models, + free_discovered_models, + ) from contextual_orchestrator.orchestrator import ModelClient, TaskOrchestrator, load_agents from contextual_orchestrator.review_gateway import ( REVIEW_AUTH_CREDENTIAL_NAME, @@ -1086,17 +487,12 @@ 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, parse_discovery_report, provider_account, ) - from contextual_orchestrator.debug_logging import configure_logging - _configure_sidecar_logging(configure_logging) registered = register_review_credentials(os.environ) auth_token = args.auth_token or get_credential(REVIEW_AUTH_CREDENTIAL_NAME) if not auth_token: @@ -1104,8 +500,13 @@ def main(argv: list[str] | None = None) -> int: "review sidecar requires an explicit --auth-token or the " f"KV credential {REVIEW_AUTH_CREDENTIAL_NAME!r}" ) - if not any(name.startswith(("BYTEZ_", "NVIDIA_", "OPENROUTER_", "OPENAI_")) for name in registered): - raise SystemExit("review sidecar requires at least one provider credential in the KV") + if not any( + name.startswith(("BYTEZ_", "NVIDIA_", "OPENROUTER_", "OPENAI_")) + for name in registered + ): + raise SystemExit( + "review sidecar requires at least one provider credential in the KV" + ) try: discovered, discovery_errors = discover_all_models() @@ -1113,7 +514,11 @@ def main(argv: list[str] | None = None) -> int: raise SystemExit(f"review sidecar discovery failed: {exc}") from exc _log_discovery_errors(discovery_errors) routable_discovered = _routable_discovered_models(discovered) - free_models = list(free_discovered_models(routable_discovered)) if routable_discovered else [] + free_models = ( + list(free_discovered_models(routable_discovered)) + if routable_discovered + else [] + ) free_route_identities = frozenset(_route_identity(model) for model in free_models) selected_models = [] for model in routable_discovered: @@ -1132,37 +537,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", "24")) - 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, @@ -1177,70 +553,16 @@ 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( - max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS, - max_retries=0, - temperature=REVIEW_TEMPERATURE, - ) + client = ModelClient(timeout=None, max_retries=0) 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( - max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS, - temperature=REVIEW_TEMPERATURE, - ) + client = ModelClient(timeout=None, max_retries=0) orchestrator = TaskOrchestrator(agents, client=client) serve( orchestrator, diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 241f84bef7..464536edfb 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -8,17 +8,22 @@ 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 import argparse -import itertools import json import math import re import sys -from collections import Counter from pathlib import Path from typing import Any, Iterable, Mapping @@ -32,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 @@ -52,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]+)*$") @@ -76,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: @@ -258,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(). @@ -274,41 +281,28 @@ def _free_pool_source_admitted(row: Mapping[str, Any]) -> bool: ) -def _route_tier(row: Mapping[str, Any], zdr_endpoints: frozenset[str]) -> tuple[int, int]: - """Return the ``(cost rank, ZDR rank)`` tier a route is selected within. - - Free routes rank before priced ones and ZDR-attested routes before - unattested ones; the tier is what the catalog fill must never reorder, - while accounts inside one tier may be interleaved freely. - """ - attested = is_zdr_model( - str(row["provider"]), - model=str(row["model"]), - zdr_endpoints=zdr_endpoints, - ) - return (_COST_EVIDENCE_RANK[_cost_evidence(row)], 0 if attested else 1) - - 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}") @@ -319,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 @@ -331,40 +331,6 @@ def build_zdr_prioritized_catalog( zdr_endpoints=zdr_endpoints, ) ] - eligible_rows.sort( - key=lambda row: ( - *_route_tier(row, zdr_endpoints), - str(row["provider"]), - str(row["model"]), - ) - ) - - # Fill each (cost, ZDR) tier round-robin across independently credentialed - # accounts. A plain sorted fill let the alphabetically first account take - # its whole cap before the next account saw a slot: on 2026-09-05 the review - # sidecar admitted 62 free routes across three accounts and served - # 8 nvidia_nim + 4 nvidia_nim_sub + 0 openrouter (limit 12, cap 8), so a - # stalled NVIDIA endpoint had no other account to fail over to - # (ContextualWisdomLab/.github#1476, contextual-orchestrator#1045). - per_account: Counter[str] = Counter() - picked: list[Mapping[str, Any]] = [] - for _tier, tier_rows in itertools.groupby( - eligible_rows, key=lambda row: _route_tier(row, zdr_endpoints) - ): - queues: dict[str, list[Mapping[str, Any]]] = {} - for row in tier_rows: - queues.setdefault(provider_account(str(row["provider"])), []).append(row) - while queues and len(picked) < limit: - for account in list(queues): - if per_account[account] >= account_cap or not queues[account]: - del queues[account] - continue - picked.append(queues[account].pop(0)) - per_account[account] += 1 - if len(picked) >= limit: - break - if len(picked) >= limit: - break if not picked: route_kind = "attested ZDR" if require_zdr else pool @@ -374,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": "", @@ -396,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": [], @@ -414,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, @@ -434,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 @@ -466,23 +436,14 @@ def build_zdr_prioritized_catalog( def _load_zdr_endpoints(path: str | None) -> frozenset[str]: - """Load exact provider/model keys from an OpenRouter ZDR feed file. - - Each feed row's ``model_id`` is the discovery slug contextual-orchestrator - reports as ``model`` (e.g. ``"inclusionai/ling-3.0-flash-vl:free"``); - ``model_name`` is a human display string (e.g. "DeepSeek: DeepSeek V4.1 - Flash") and is never used to build a route key. ``provider_name`` is the - feed's serving-provider label (e.g. "Novita"). Rows missing either - ``model_id`` or ``provider_name`` are skipped; there is no fallback to - the display name. - """ + """Load exact provider/model keys from an OpenRouter ZDR feed file.""" if not path: return frozenset() payload = json.loads(Path(path).read_text(encoding="utf-8")) keys: set[str] = set() for endpoint in payload.get("data", []): provider = endpoint.get("provider_name") - model = endpoint.get("model_id") + model = endpoint.get("model_name") if provider and model: keys.add(_route_key(str(provider), str(model))) keys.add(_route_key("openrouter", str(model))) @@ -494,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", @@ -521,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( @@ -531,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") @@ -541,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, @@ -560,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 3c2a1b51b9..9757b074f6 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -9,12 +9,12 @@ # 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 -ORCHESTRATOR_PIN_SHA="${ORCHESTRATOR_PIN_SHA:-767e67fbc6b881a452761f32abb69b9971b9b03b}" +ORCHESTRATOR_PIN_SHA="${ORCHESTRATOR_PIN_SHA:-2e414d15ba58f28597751b625a8a2f00fc9fadcf}" ORCHESTRATOR_GIT_URL="${ORCHESTRATOR_GIT_URL:-https://github.com/ContextualWisdomLab/contextual-orchestrator.git}" # The Strix gate and Noema SSRF guard accept this one process-local origin. # Keep it fixed so an environment override cannot create an unvalidated sidecar. @@ -35,13 +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:-24}" -# 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 preflight candidate list (24 candidates, probed lazily -# to a readiness target -- ADR-0029) 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)" @@ -232,15 +225,11 @@ policy_report="$ORCHESTRATOR_WORK/policy-report.json" preflight_report="$STRIX_EVIDENCE_DIR/contextual-orchestrator-preflight.json" sidecar_stdout="$STRIX_EVIDENCE_DIR/contextual-orchestrator-sidecar.stdout.log" sidecar_stderr="$STRIX_EVIDENCE_DIR/contextual-orchestrator-sidecar.stderr.log" -gateway_preflight_request="$ORCHESTRATOR_WORK/gateway-preflight-request.json" -gateway_preflight_response="$ORCHESTRATOR_WORK/gateway-preflight.json" ( umask 077 : > "$preflight_report" : > "$sidecar_stdout" : > "$sidecar_stderr" - : > "$gateway_preflight_request" - : > "$gateway_preflight_response" ) publish_sidecar_evidence() { @@ -296,8 +285,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 @@ -406,274 +393,12 @@ if [ -n "$sidecar_startup_warnings" ]; then log "sidecar startup warnings (non-fatal): $sidecar_startup_warnings" fi -# Exercise the exact OpenAI-compatible endpoint and model name Strix uses. A -# process can be healthy while the coordinator/model-group path still raises an -# internal error, which is the failure this contract prevents from reaching the -# scanner step. -gateway_virtual_model="orchestrator/${orchestrator_pool}" -# max_tokens must match REVIEW_MAX_OUTPUT_TOKENS (the launcher's own escalated -# per-agent routing-probe budget, ADR-0005): observed behavior was an agent the -# routing probe already proved "ready" at that budget failing this separate -# end-to-end check with a spurious 502 invalid_structured_output at a much -# smaller budget, even though the model itself is healthy. The exact -# field-level cause was never captured (the sidecar's log sanitizer strips raw -# provider payloads by design), so treat any specific mechanism as a -# hypothesis, not fact. See "2026-08-30 sidecar preflight max_tokens -# desynchronized from the routing probe" (and its 2026-08-30 correction) in -# ContextualWisdomLab/contextual-orchestrator's own -# docs/product-technical-gap-baseline.md for the evidence that is actually -# captured (downloaded strix-reports artifact, -# ContextualWisdomLab/contextual-orchestrator#912 run 33304076516). -printf '{"model":"%s","messages":[{"role":"system","content":"You are a helpful assistant."},{"role":"user","content":"Reply with just '\''OK'\''."}],"temperature":1.0,"max_tokens":4096,"stream":false}\n' \ - "$gateway_virtual_model" > "$gateway_preflight_request" -# This completion is model inference, so ADR-0003 forbids a wall-clock timeout. -# A slow reasoning model may legitimately take hours after routing proves it -# healthy; transport failures still fail closed through curl's exit status. -# -# ADR-0005 Trigger A: this request goes to the virtual pool, not one pinned -# candidate, so a transport failure or non-2xx status here (unreachable -# process, upstream error) is retried with a fresh attempt at the -# SAME budget, up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS total attempts -- -# a same-budget retry may or may not land on a different underlying candidate -# (route diversity here is a best-effort hope, not a verified guarantee: the -# gateway's internal routing behavior on a failed attempt is not confirmed), -# but it is strictly better than one unconditional attempt with no recovery -# path, which is what let a single transient hang block every required review -# org-wide (live reproduction: ContextualWisdomLab/.github#1449, job -# 99253418179, curl timing out at exactly 120002ms with zero bytes received). -# Trigger B (a response IS received, empty content, finish_reason=="length" or -# a populated reasoning field) is deliberately NOT retried here: that response -# is still HTTP 200, so the gateway's own routing already recorded that -# attempt as "successful" before this script inspects content -- a same-budget -# retry is more likely to repeat the same candidate than diversify away from -# it, so retrying would not help (Devin Review's 4th-round finding on -# ADR-0005; verified directly against contextual-orchestrator's server.py, -# which exposes no parameter to exclude or deprioritize a specific candidate -# on a retry). -REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS="${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS:-3}" -# A malformed override (non-numeric, empty, or zero) must fail closed instead -# of silently disabling the bound: `[ "$gateway_attempt" -ge "$X" ]` with a -# non-integer `$X` is itself a bash integer-comparison error, not a false -# result, so the retry loop below would keep looping (never satisfying its -# own exit test) until the surrounding CI job's own timeout kills it instead -# of this check ever rejecting bad configuration on its own. An all-digit -# value is not automatically safe either: `[ -ge ]` still errors the exact -# same way once the value overflows the shell's integer range (reproduced -# directly: a 55-digit all-digit string fails with "integer expression -# expected", identical to a non-numeric one) -- so the bound below also caps -# digit COUNT, not just digit-ness. Four digits (up to 9999) is already far -# beyond any realistic attempt count and stays safely representable on every -# platform this runs on. -case "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" in - ''|*[!0-9]*|0) - fail "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be a positive integer" ;; - ?????*) - fail "REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS must be at most 9999" ;; -esac -gateway_attempt=1 -gateway_http_status="" -while :; do - 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 - break - fi - if [ "$gateway_attempt" -ge "$REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS" ]; then - if [ -z "$gateway_http_status" ]; then - # Every configured attempt exhausted with no usable HTTP response at - # all (Trigger A never resolved) -- record that before failing closed, - # using the same sanitize-then-atomic-replace pattern as the non-2xx - # and invalid-content paths below, so this exact failure case (the one - # telemetry matters most for) does not leave zero evidence trail. - "$sidecar_python" - "$preflight_report" "$gateway_attempt" <<'PY' -import json -from pathlib import Path -import sys - -report_path = Path(sys.argv[1]) -attempts = int(sys.argv[2]) if sys.argv[2].isdecimal() else 0 -try: - report = json.loads(report_path.read_text(encoding="utf-8")) -except (OSError, json.JSONDecodeError): - report = {} -report["gateway"] = { - "endpoint": "chat/completions", - "error_type": "gateway_transport_exhausted", - "attempts": attempts, - "status": "rejected", -} -temporary = report_path.with_suffix(".tmp") -temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") -temporary.replace(report_path) -PY - fail "gateway preflight request could not reach the local sidecar after ${gateway_attempt} attempts" - fi - "$sidecar_python" - "$preflight_report" "$gateway_preflight_response" "$gateway_http_status" "$gateway_attempt" <<'PY' -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] -attempts = int(sys.argv[4]) if sys.argv[4].isdecimal() else 0 -try: - report = json.loads(report_path.read_text(encoding="utf-8")) -except (OSError, json.JSONDecodeError): - 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", - # ADR-0005: a non-2xx on a retry (attempts > 1) is not honestly - # attributable to any one candidate's ceiling -- the virtual pool's - # routing is not pinned across separate HTTP calls -- so it is - # recorded distinctly from a first-attempt rejection instead of - # implying candidate-ceiling evidence it cannot support. - "error_type": "gateway_retry_rejected" if attempts > 1 else "gateway_rejected", - "error_code": code, - "http_status": status, - "attempts": attempts, - "status": "rejected", -} -temporary = report_path.with_suffix(".tmp") -temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") -temporary.replace(report_path) -# error_code is already regex-validated above ([A-Za-z0-9_.-]{1,64}) and status -# is a plain int, so this is safe to print directly to the job's own log -- -# unlike the sidecar server subprocess's stdout/stderr, this synchronous -# one-shot snippet's output is not routed through the sanitizer, and was -# previously visible only in the CONTEXTUAL_ORCHESTRATOR_PREFLIGHT_EVIDENCE -# artifact file, not the job log a CI operator actually reads first. -print(f"[contextual-orchestrator-sidecar] gateway preflight rejected: error_code={code} http_status={status}") -PY - fail "gateway preflight returned HTTP ${gateway_http_status} after ${gateway_attempt} attempts" - fi - log "gateway preflight attempt ${gateway_attempt} did not reach the sidecar cleanly (status=${gateway_http_status:-unreachable}); retrying (up to ${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS} attempts)" - gateway_attempt=$((gateway_attempt + 1)) -done -if ! "$sidecar_python" - "$gateway_preflight_response" "$preflight_report" "$gateway_attempt" <<'PY' -import json -from pathlib import Path -import sys - -response_path = Path(sys.argv[1]) -report_path = Path(sys.argv[2]) -attempts = int(sys.argv[3]) if sys.argv[3].isdecimal() else 0 -try: - response = json.loads(response_path.read_text(encoding="utf-8")) - if not isinstance(response, dict): - # Valid JSON, wrong top-level shape (e.g. `[]`, `null`, a bare - # string/number instead of an object) -- .get("choices") below - # assumes a dict and would otherwise raise AttributeError, which is - # not in the caught tuple below, losing evidence exactly like the - # unparseable-body case this except block exists to cover. Reuses - # the already-caught TypeError rather than widening the tuple to - # AttributeError broadly, which could mask unrelated bugs elsewhere - # in this block. - raise TypeError(f"gateway response was not a JSON object: {type(response).__name__}") - choices = response.get("choices") - first = choices[0] if isinstance(choices, list) and choices else None - message = first.get("message") if isinstance(first, dict) else None - content = message.get("content") if isinstance(message, dict) else None - # Bounded to a short, stable enum token (never raw provider text), and - # computed once so both the success and rejected outcomes below record - # the SAME evidence shape -- populated on success too (not just - # failure), so future tuning has a real "normal" baseline to compare - # against, not just evidence of what went wrong. - finish_reason = first.get("finish_reason") if isinstance(first, dict) else None - if not isinstance(finish_reason, str) or not finish_reason: - finish_reason = None - elif len(finish_reason) > 32 or not all( - character.isalnum() or character == "_" for character in finish_reason - ): - finish_reason = "unknown" - has_text = isinstance(content, str) and bool(content.strip()) - # Requires BOTH a populated reasoning field AND no usable content -- - # never true for a normal, complete answer that also discloses a - # reasoning trace alongside real content. Checking `reasoning` alone - # (with no check that content is actually absent) would wrongly flag a - # genuinely healthy response and pollute this evidence. - reasoning_without_content = ( - isinstance(message, dict) and bool(message.get("reasoning")) and not has_text - ) - if has_text: - report = json.loads(report_path.read_text(encoding="utf-8")) - report["gateway"] = { - "endpoint": "chat/completions", - "status": "ready", - "attempts": attempts, - "finish_reason": finish_reason or "unknown", - "reasoning_without_content": reasoning_without_content, - } - temporary = report_path.with_suffix(".tmp") - temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") - temporary.replace(report_path) - raise SystemExit(0) - # ADR-0005 Trigger B, deliberately not retried at this layer (see the - # comment above the curl loop): record which budget-too-small signature, - # if any, matched -- for diagnosis only, since this response is a - # terminal outcome here regardless of which one it is. - report = json.loads(report_path.read_text(encoding="utf-8")) - report["gateway"] = { - "endpoint": "chat/completions", - "status": "rejected", - "error_type": "invalid_chat_response", - "finish_reason": finish_reason or "unknown", - "reasoning_without_content": reasoning_without_content, - "attempts": attempts, - } - temporary = report_path.with_suffix(".tmp") - temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") - temporary.replace(report_path) -except (OSError, json.JSONDecodeError, IndexError, TypeError): - # The response file was missing/unreadable, or its body was HTTP 200 - # but not the parseable JSON structure expected (malformed/truncated) -- - # a different failure than "valid JSON, empty content" above. Record a - # bounded classification before failing closed, using the same - # sanitize-then-atomic-replace pattern as every other gateway outcome, - # so this exact case does not leave zero evidence trail either. Never - # attempts to read or copy the unparseable body itself. - try: - report = json.loads(report_path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): - report = {} - report["gateway"] = { - "endpoint": "chat/completions", - "status": "rejected", - "error_type": "gateway_invalid_response", - "attempts": attempts, - } - temporary = report_path.with_suffix(".tmp") - temporary.write_text(json.dumps(report, indent=2, sort_keys=True) + "\n", encoding="utf-8") - temporary.replace(report_path) -raise SystemExit(1) -PY -then - fail "gateway preflight returned unusable chat content" -fi -log "gateway chat/completions preflight confirmed (attempt ${gateway_attempt}/${REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS})" - +# Launcher runtime preflight is the sole live provider observation during +# provisioning. Replaying a chat/completions request here would manufacture a +# second inference, its own token/sampling policy, and a shell-local retry +# budget after the route has already been admitted. Healthz plus the persisted +# per-route preflight evidence are the sidecar readiness boundary; consumers +# exercise the OpenAI-compatible endpoint with their real workload. if [ -n "$ORCHESTRATOR_GITHUB_ENV" ]; then { printf 'CONTEXTUAL_ORCHESTRATOR_BASE_URL=http://%s:%s\n' "$ORCHESTRATOR_HOST" "$ORCHESTRATOR_PORT" @@ -689,7 +414,4 @@ fi log "policy evidence summary:" sed -n '1,80p' "$policy_report" || true log "runtime preflight summary:" -# 16 probed routes at 8-10 lines each plus the header run past the old -# 160-line cap exactly in the dead hour the summary matters most (ADR-0029); -# the artifact copy was always complete, only the job-log echo was cut. -sed -n '1,400p' "$preflight_report" || true +sed -n '1,160p' "$preflight_report" || true 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..68a11cf9b3 --- /dev/null +++ b/tests/_contextual_orchestrator_review_runtime_preflight_cases.py @@ -0,0 +1,425 @@ +"""Current regressions for the contextual-orchestrator review runtime boundary. + +Historical incidents remain documented in ADRs and doctoring records. This +module contains only executable expectations that still match the current +one-shot, provider-default preflight contract; retired token escalation, +priced-fallback, and shell-level inference/retry policies do not remain as +hidden test oracles. +""" + +from __future__ import annotations + +from contextlib import redirect_stdout +import io +from pathlib import Path +import re +import runpy +import sys +from types import SimpleNamespace + +import pytest + + +_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 + + +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 test_routable_discovered_models_excludes_evidence_only_rows() -> None: + """Evidence-only discovery rows must never enter live route selection.""" + namespace = _load_launcher() + routable = namespace.get("_routable_discovered_models") + assert callable(routable) + + 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: + """Discarded discovery failures remain visible without raw provider data.""" + namespace = _load_launcher() + log_discovery_errors = namespace.get("_log_discovery_errors") + assert callable(log_discovery_errors) + + log_discovery_errors( + [ + SimpleNamespace(provider_name="bytez", error_code="http_status_401"), + SimpleNamespace(provider_name="openai", error_code="timeout"), + ] + ) + + 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: + """A clean discovery emits only the completion sentinel.""" + 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 poll target and launcher sentinel must remain identical.""" + 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: + """Reasoning metadata alone never makes a usable response look starved.""" + namespace = _load_launcher() + has_reasoning_without_content = namespace["_response_has_reasoning_without_content"] + + assert not has_reasoning_without_content( + { + "choices": [ + { + "message": { + "reasoning": "the user asked X, so the answer is Y", + "content": "Y", + } + } + ] + } + ) + assert has_reasoning_without_content( + {"choices": [{"message": {"reasoning": "still thinking", "content": ""}}]} + ) + assert has_reasoning_without_content( + {"choices": [{"message": {"reasoning": "still thinking"}}]} + ) + assert not has_reasoning_without_content( + {"choices": [{"message": {"content": "a normal reply"}}]} + ) + + +def test_log_preflight_rejections_prints_bounded_summary_to_stderr( + capsys: pytest.CaptureFixture[str], +) -> None: + """Rejected route evidence reaches the job log without leaking messages.""" + 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 + 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_ignores_malformed_report( + capsys: pytest.CaptureFixture[str], +) -> None: + """Malformed diagnostic data must not raise or manufacture output.""" + 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_sidecar_discovery_and_health_have_no_wall_clock_timeout() -> None: + """Non-inference startup waits remain process-liveness based, not guessed deadlines.""" + 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_base_probe_success_with_reasoning_and_content_is_never_flagged_as_starved() -> None: + """A complete one-shot response may carry reasoning metadata and remain ready.""" + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + agent = SimpleNamespace( + id="openai_transparent_reasoner", + provider_name="openai", + model="reasoner/free", + ) + client = _ProbeClient( + { + agent.id: { + "choices": [ + { + "finish_reason": "stop", + "message": { + "reasoning": "the user asked for a greeting", + "content": "Hello!", + }, + } + ] + } + } + ) + + viable, report = preflight([agent], client=client) + + assert viable == [agent] + assert len(client.calls) == 1 + 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_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) + assert isinstance(error_type, type) + + 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) + assert len(client.calls) == 1 + + +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_policy.py b/tests/test_contextual_orchestrator_review_policy.py index e13e94107e..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 @@ -116,12 +122,8 @@ def test_load_zdr_endpoints_skips_rows_without_provider_or_model(tmp_path) -> No json.dumps( { "data": [ - { - "model_id": "deepseek/deepseek-r1:free", - "model_name": "DeepSeek: R1 (free)", - "provider_name": "DeepSeek", - }, - {"model_id": "no-provider"}, + {"model_name": "deepseek/deepseek-r1:free", "provider_name": "DeepSeek"}, + {"model_name": "no-provider"}, {"provider_name": "NoModel"}, ] } @@ -144,91 +146,6 @@ def test_load_zdr_endpoints_respects_none_feed_path(tmp_path) -> None: assert policy._load_zdr_endpoints(str(empty_feed)) == frozenset() -def test_load_zdr_endpoints_keys_by_model_id_not_display_name(tmp_path) -> None: - """The live OpenRouter ZDR feed keys routes by ``model_id``, not ``model_name``. - - Confirmed by offline reproduction against the real - ``https://openrouter.ai/api/v1/endpoints/zdr`` feed: OpenRouter's - ``model_name`` is a human display string (e.g. "DeepSeek: DeepSeek V4.1 - Flash") while ``model_id`` is the slug contextual-orchestrator discovery - reports as ``model`` (e.g. "inclusionai/ling-3.0-flash-vl:free"). Keying - on ``model_name`` (introduced in 17052a7ca, #1360) meant no live-feed - route ever matched ``is_zdr_model(...)``, so with ``--require-zdr`` - (every private/internal consumer, per ADR-0003) the catalog was always - empty and the sidecar failed closed with "no attested ZDR model route is - available with the ZDR policy; orchestrator/free would fail closed". - This killed noema-review and strix on - ContextualWisdomLab/late-life-anxiety-reanalysis#10 (head - a1cd5bc6783c6510dfcf937f523c733366e82213, runs 34700409452/103571267389 - and 34700409446/103571829483) against central - fb17ef556f94f673234aa557254ae52779e9a7b0. See - ContextualWisdomLab/.github#2122. - """ - feed = tmp_path / "zdr.json" - feed.write_text( - json.dumps( - { - "data": [ - { - "name": "Novita | inclusionai/ling-3.0-flash-vl-20260910:free", - "model_id": "inclusionai/ling-3.0-flash-vl:free", - "model_name": "inclusionAI: Ling 3.0 Flash VL (free)", - "provider_name": "Novita", - }, - { - "name": "x", - "model_name": "Display Only", - "provider_name": "Novita", - }, - ] - } - ), - encoding="utf-8", - ) - - keys = policy._load_zdr_endpoints(str(feed)) - - assert keys == frozenset( - { - policy._route_key("Novita", "inclusionai/ling-3.0-flash-vl:free"), - policy._route_key("openrouter", "inclusionai/ling-3.0-flash-vl:free"), - } - ) - assert not any("Display Only" in key for key in keys) - assert not any("inclusionAI: Ling 3.0 Flash VL" in key for key in keys) - - report = { - "models": [ - { - "provider": "openrouter", - "model": "inclusionai/ling-3.0-flash-vl:free", - "agent_id": "or_ling_vl", - "is_free": True, - **FREE_PRICE, - }, - { - "provider": "openrouter", - "model": "other-vendor/not-covered:free", - "agent_id": "or_not_covered", - "is_free": True, - **FREE_PRICE, - }, - ] - } - result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(report), - limit=12, - account_cap=4, - zdr_endpoints=keys, - require_zdr=True, - pool="free", - ) - assert [agent["model"] for agent in result["agents"]] == [ - "inclusionai/ling-3.0-flash-vl:free" - ] - assert result["report"]["zdr_selected_count"] == 1 - - def test_route_key_prefixes_provider() -> None: """ZDR feed keys are matched with the provider prefix.""" assert policy._route_key("openrouter", "deepseek/deepseek-r1:free") == ( @@ -270,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", ) @@ -316,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( @@ -347,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", @@ -367,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: @@ -395,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 @@ -410,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: @@ -489,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"] @@ -519,8 +457,7 @@ def test_load_zdr_endpoints_parses_feed(tmp_path) -> None: "data": [ { "name": "deepseek/deepseek-r1:free", - "model_id": "deepseek/deepseek-r1:free", - "model_name": "DeepSeek: R1 (free)", + "model_name": "deepseek/deepseek-r1:free", "provider_name": "DeepSeek", } ] @@ -543,15 +480,7 @@ def test_build_catalog_from_paths_writes_both_files(tmp_path) -> None: feed = tmp_path / "zdr.json" feed.write_text( json.dumps( - { - "data": [ - { - "model_id": "deepseek/deepseek-r1:free", - "model_name": "DeepSeek: R1 (free)", - "provider_name": "DeepSeek", - } - ] - } + {"data": [{"model_name": "deepseek/deepseek-r1:free", "provider_name": "DeepSeek"}]} ), encoding="utf-8", ) @@ -562,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() @@ -587,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: @@ -635,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, ) @@ -657,96 +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, - ) - - -def _free_rows(provider: str, count: int, prefix: str) -> list[dict[str, object]]: - """Return ``count`` free discovery rows for one credential account.""" - return [ - { - "provider": provider, - "model": f"{prefix}{i}", - "agent_id": f"{prefix}_{i}", - "is_free": True, - **FREE_PRICE, - } - for i in range(count) - ] - - -def test_build_catalog_interleaves_accounts_within_a_tier() -> None: - """A bounded catalog spreads across admitted accounts instead of filling alphabetically. - - Measured on 2026-09-05 (``noema-review`` run 33969842312): 62 admitted free - routes across three accounts, limit 12, account cap 8, served as - 8 ``nvidia_nim`` + 4 ``nvidia_nim_sub`` + 0 ``openrouter`` because the - sorted fill reached the limit before the alphabetically last account got a - slot -- so a stalled NVIDIA endpoint had no other account to fail over to. - """ - report = { - "models": _free_rows("nvidia_nim", 8, "a") - + _free_rows("nvidia_nim_sub", 8, "b") - + _free_rows("openrouter", 8, "o") - } - result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(report), limit=12, account_cap=8 - ) - providers = [agent["provider_name"] for agent in result["agents"]] - assert providers[:3] == ["nvidia_nim", "nvidia_nim_sub", "openrouter"] - assert providers.count("nvidia_nim") == 4 - assert providers.count("nvidia_nim_sub") == 4 - assert providers.count("openrouter") == 4 - - -def test_build_catalog_interleaving_keeps_zdr_tier_first() -> None: - """Account interleaving never lifts a non-ZDR route above an attested one.""" - report = { - "models": _free_rows("nvidia_nim", 3, "a") - + [ - { - "provider": "openrouter", - "model": "deepseek/deepseek-r1:free", - "agent_id": "or_zdr", - "is_free": True, - **FREE_PRICE, - } - ] - + _free_rows("openrouter", 3, "o") - } - result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(report), - limit=4, - account_cap=8, - zdr_endpoints=ZDR_FEED, - ) - assert result["agents"][0]["model"] == "deepseek/deepseek-r1:free" - assert [agent["provider_name"] for agent in result["agents"]][1:] == [ - "nvidia_nim", - "openrouter", - "nvidia_nim", - ] - - -def test_build_catalog_interleaving_skips_exhausted_accounts() -> None: - """An account with fewer routes than its share hands its turns to the others.""" - report = { - "models": _free_rows("nvidia_nim", 5, "a") - + _free_rows("nvidia_nim_sub", 1, "b") - + _free_rows("openrouter", 2, "o") - } - result = policy.build_zdr_prioritized_catalog( - policy.parse_discovery_report(report), limit=12, account_cap=8 - ) - assert [agent["provider_name"] for agent in result["agents"]] == [ - "nvidia_nim", - "nvidia_nim_sub", - "openrouter", - "nvidia_nim", - "openrouter", - "nvidia_nim", - "nvidia_nim", - "nvidia_nim", - ] + ) \ No newline at end of file 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..4d316b28b6 --- /dev/null +++ b/tests/test_contextual_orchestrator_review_preflight_concurrency.py @@ -0,0 +1,97 @@ +"""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 cross-provider progress without asserting a compute allocation.""" + assert endpoint == "chat/completions" + assert "max_tokens" not in payload + assert "temperature" not in payload + 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. The concurrency + oracle deliberately leaves sampling and token allocation to provider defaults. + """ + 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_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index b279d33a98..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. """ @@ -40,7 +40,7 @@ ) GATEWAY_MODEL = "contextual-orchestrator/orchestrator/free" -ORCH_PIN_SHA = "767e67fbc6b881a452761f32abb69b9971b9b03b" +ORCH_PIN_SHA = "2e414d15ba58f28597751b625a8a2f00fc9fadcf" def _read(path: Path) -> str: @@ -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