diff --git a/CHANGELOG.md b/CHANGELOG.md index d7c6d40ae7..9d18bd1cfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,30 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- `scripts/ci/contextual_orchestrator_review_launcher.py`'s `_routable_discovered_models()` + no longer blanket-strips every OpenRouter row on `evidence_only` alone. + `contextual-orchestrator`'s OpenRouter `ProviderModelSource` currently + hardcodes `evidence_only=True` for every discovered model unconditionally + (a confirmed bug, being fixed upstream separately), which was excluding + 100% of OpenRouter discovery rows here before + `zdr_policy.is_zdr_model()`'s purpose-built, per-route OpenRouter ZDR-feed + check ever got a chance to evaluate them -- making that already-correct + mechanism dead code for OpenRouter specifically. OpenRouter rows are now + exempt from this exclusion; a genuinely non-servable OpenRouter row is + still excluded by the existing, provider-agnostic chat-capability check + every other provider's rows already go through. `_routable_discovered_models()` + also now excludes a `spend_admitted=False` row the same way it excludes + `evidence_only=True`, so a credit-exhausted priced OpenRouter row cannot + reach `orchestrator/auto`'s served catalog. +- Close a 99% `scripts/ci` coverage regression on protected main: merged #1546 added an + uncovered `live_head_matches` helper, an uncovered no-active/no-stale-runs fall-through in + `prepare_autofix_slot`, and an uncovered "current-head autofix run is already queued or + running" wait path in `pr_review_fix_scheduler.py::inspect_pr`, while the pre-existing + conflicted-draft and conflicted-unauthorized `inspect_pr` returns and the REST + `fetch_workflow_names_by_check_suite_rest` pagination/name-filtering/permission-denied paths + in `pr_review_merge_scheduler.py` remained untested. Every PR rebasing onto main inherited + this failure via the `coverage-evidence` required check regardless of its own diff; this adds + test-only coverage for all of the above with no production code change. - **Fix `opencode-review.yml` admission gaps around stale/out-of-order events (`#1568`).** Building on the draft-poll exemption's live PR/head validation, Devin Review found two further defects. (1) The concurrency group was keyed only by repository and PR number, so @@ -237,6 +261,74 @@ Semantic Versioning where the repository publishes a release. still named the removed `free_family_diversity` evidence field instead of its `free_account_diversity` replacement, which could send future monitoring work looking for a field that no longer exists. +- `scripts/ci/contextual_orchestrator_review_policy.py`'s catalog admission + cap and diversity evidence no longer conflate "independent credential + account" with "independent outage domain": `nvidia_nim`/`nvidia_nim_sub` + are independent accounts (may expose different models) but share one + physical upstream endpoint (`https://integrate.api.nvidia.com/v1`), so + they now share one admission-cap budget and count as one outage domain. A + new `free_outage_domain_diversity` report field (additive, alongside the + existing `free_account_diversity`) reflects this for callers deciding + whether a single provider outage could empty the free catalog. Outage- + domain grouping normalizes each row's `base_url` first (lowercasing + scheme/host, dropping an explicit default port, stripping a trailing + slash, and never raising even on a malformed IPv6-bracket URL), so a + formatting difference alone cannot split one physical endpoint into two + domains. Within a shared domain, the admission cap's bounded slots are + now split round-robin across the domain's contending accounts instead of + being consumed entirely by whichever account's rows happen to sort first + -- fixing a narrower starvation bug the outage-domain grouping itself + introduced (one credential could otherwise get zero admissions from a + shared domain even with rows available and cap budget nominally unused + by it). That fairness reordering is now strictly scoped to one admission- + priority tier (cost tier + ZDR status) at a time, never across tiers -- + an earlier revision grouped a whole outage domain's rows into one block + regardless of tier, which could drag a lower-priority route (paid, + non-ZDR) ahead of a higher-priority route (free, ZDR) belonging to a + different domain, sometimes dropping a free route for a paid one under a + tight catalog limit. IPv6 host normalization now re-brackets a + colon-bearing host before appending a port, so an explicit-port address + (`[::1]:8443`) and an unrelated literal that merely contains the same + digits (`[::1:8443]`) no longer collapse to one outage domain. The + priced-fallback catalog stage (`orchestrator/auto`'s post-primary-stage + fallback) gets its own domain-diversity fix: with both defaults at 4, + the fallback route budget coincidentally equaled the per-domain cap, so + a single dominant outage domain could exhaust the entire fallback stage + before a genuinely independent domain's row was ever considered (Devin + Review finding). `build_zdr_prioritized_catalog` gains an opt-in + `guarantee_domain_coverage` flag (only the priced-fallback call site + sets it) that admits in two passes instead of one: the first pass + admits at most one row per outage domain, guaranteeing representation; + the second fills any remaining budget from whichever domain's + next-highest-priority row comes first, still bounded by `account_cap`. + A first revision shrank the cap to `fallback_limit // domain_count` + instead, which fixed representation but wasted capacity whenever the + split was uneven (a second Devin Review finding, "fallback quota wastes + probe slots" -- `limit=4` across 3 domains admitted only 3 routes under + a floor of 1); the two-pass approach guarantees both properties at + once. The common single-domain case is unchanged. A third Devin Review + finding caught the identical gap reachable through the *primary* + `auto`-pool stage too, not just the fallback: the review sidecar's real + deployed default is `ORCHESTRATOR_CATALOG_ACCOUNT_CAP=8` (not the + launcher's `DEFAULT_ACCOUNT_CAP=4` fallback, which the sidecar never + leaves the env var unset for), and the primary stage's own route limit + for the `auto` pool is also capped at 8 + (`REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT`) -- the same cap-equals-limit + coincidence, just at 8 instead of 4. `guarantee_domain_coverage=True` + now applies to both `build_zdr_prioritized_catalog` call sites in + `main()`. +- `guarantee_domain_coverage`'s two admission passes now run strictly + within one admission-priority tier at a time, in tier order, instead of + across `ordered_rows` as a whole (Devin Review: "domain coverage defeats + ZDR priority") -- without this, a worse-tier row could win a first-pass + "guaranteed representation" seat for its domain ahead of a better-tier + row from an already-represented domain (e.g. two free/ZDR routes in one + domain plus one free/non-ZDR route in an independent domain, `limit=2`, + wrongly admitted one row from each instead of both free/ZDR routes). + This is the same tier-boundary discipline `_fair_admission_order` + already enforces for its own reordering, now applied one level up; the + cumulative-representation and `account_cap` accounting across tiers is + unchanged. - Noema, Strix, and OpenCode review sidecars now vendor contextual-orchestrator at `c107e3e52371993aa9c326fcc245e01c41fc3850` and treat every KV credential as an independent discovery account. Same-vendor credentials no longer diff --git a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md index 7b9ea7e1ac..4176e9f4fc 100644 --- a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md +++ b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md @@ -190,6 +190,29 @@ all five, and auto-optimize routing by cost. amendment" (above) are closed, without requiring a manual re-audit. `docs/doctoring/contextual-orchestrator-strix-free-diversity-evidence.md` records that PR's own reasoning trail. +- **2026-08-31 correction: account diversity is not outage-domain diversity.** + Review during this session found that #1468 (above), in correctly stopping + `nvidia_nim`/`nvidia_nim_sub` from being treated as one *model-catalog* + family, also let `free_account_diversity` and the catalog's admission cap + treat them as two fully independent *outage domains* — they are not: both + resolve to the identical `https://integrate.api.nvidia.com/v1` upstream + (see `PROVIDER_BASE_URLS` in `scripts/ci/zdr_policy.py`, and that table's + own `nvidia_nim_sub` ZDR-scope note). Conflating the two meant a discovery + report whose only free routes were these two credentials reported + `free_account_diversity == 2` — falsely reassuring for exactly the decision + this evidence exists to support (would a single physical outage empty the + free catalog) — and the admission cap let the pair jointly consume up to + twice its intended per-domain budget, crowding out a genuinely independent + provider even when one had free routes available. + `contextual_orchestrator_review_policy.py` now reports a second, distinct + field, `free_outage_domain_diversity`, grouped by each row's own `base_url` + evidence rather than a second hand-maintained provider-name table, and the + admission cap (`account_cap`; the name predates this fix and is kept for + CLI/environment stability) groups by outage domain, not by credential. A + caller deciding whether Strix can safely rely on a strict `orchestrator/free` + pool without the `orchestrator/auto` paid fallback (open PR #1437) should + read `free_outage_domain_diversity`, not `free_account_diversity`, for that + specific decision. - **2026-08-31 amendment: Noema reviews independently of OpenCode.** Noema no longer waits for an OpenCode approval, review-thread state, or other check conclusions before calling the gateway and submitting its current-head diff --git a/docs/product-goal-directive.md b/docs/product-goal-directive.md index ecb4f3b69c..b5de58a234 100644 --- a/docs/product-goal-directive.md +++ b/docs/product-goal-directive.md @@ -66,7 +66,7 @@ Per this file's own conflict policy above: this note is the resolution, and `doc **Note (flagged by CodeRabbit on this PR, 2026-08-30):** section 8's quoted text describes `contextual-orchestrator`'s general product capability — broad model/modality support and all-five-secret auto model discovery as a *design principle for the orchestrator itself*. It does not specify, and must not be read as overriding, which pool each CI consumer routes through: that is governed exclusively by `docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md` and its doctoring records — `OpenCode` and `Noema` use the fail-closed, ZDR-prioritized `orchestrator/free` pool; only `Strix` security analysis uses the provider-diverse `orchestrator/auto` pool; private/internal review targets require an attested ZDR-only catalog and never fall back to a non-ZDR provider. Do not loosen any CI consumer's pool or credential scope on the strength of this section's general wording alone. -**Note (2026-08-30, superseded by the merged pin flip — see the correction below):** an earlier draft of this note said Strix stayed on `orchestrator/auto` pending `free_family_diversity` reaching `>= 2`. That is no longer true and must not be read as current: `.github/workflows/strix.yml` now hardcodes `STRIX_MODEL`/`CONTEXTUAL_ORCHESTRATOR_POOL` to `orchestrator/free` and fails closed on any other value, and ADR-0003's 2026-08-30 amendment records the owner's decision to accept the residual single-outage-domain risk immediately rather than wait for the evidence-gated threshold this note originally described. `free_account_diversity` (`scripts/ci/contextual_orchestrator_review_policy.py`; renamed from `free_family_diversity` once every KV credential became an independent discovery account rather than being grouped into a vendor "family", see #1468) remains useful as ongoing monitoring evidence for that accepted risk, not as a gate blocking the pin. +**Note (2026-08-30, superseded by the merged pin flip — see the correction below):** an earlier draft of this note said Strix stayed on `orchestrator/auto` pending `free_family_diversity` reaching `>= 2`. That is no longer true and must not be read as current: `.github/workflows/strix.yml` now hardcodes `STRIX_MODEL`/`CONTEXTUAL_ORCHESTRATOR_POOL` to `orchestrator/free` and fails closed on any other value, and ADR-0003's 2026-08-30 amendment records the owner's decision to accept the residual single-outage-domain risk immediately rather than wait for the evidence-gated threshold this note originally described. `free_account_diversity` (`scripts/ci/contextual_orchestrator_review_policy.py`; renamed from `free_family_diversity` once every KV credential became an independent discovery account rather than being grouped into a vendor "family", see #1468) remains useful as ongoing monitoring evidence for that accepted risk, not as a gate blocking the pin. **Correction (2026-08-31):** for *this specific* single-outage-domain risk, read `free_outage_domain_diversity`, not `free_account_diversity` — #1468's rename correctly made every KV credential an independent *account*, but `nvidia_nim`/`nvidia_nim_sub` remain one *outage domain* (both resolve to the identical `https://integrate.api.nvidia.com/v1` upstream), so `free_account_diversity` alone can read `2` for a catalog that is, in fact, still exposed to a single provider outage. `free_outage_domain_diversity` is the field that actually answers this note's question. ## 9. Reference libraries, tool invocations, and ecosystem repositories diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 9367d54f67..54bd9c05d1 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,417 @@ string, a bare number) confirmed to fail against the pre-fix script (`KeyError: signature as the original round-4 bug) before passing after the fix. 1930 tests pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 a second, subtler NVIDIA-independence gap: account diversity conflated with outage-domain diversity + +**Context.** This session investigated why `contextual-orchestrator` PR #941/#945's fix (independent +`nvidia_nim`/`nvidia_nim_sub` credentials must not be assumed to share one model catalog) was not +reflected in production review evidence, and found two bugs: a stale `ORCHESTRATOR_PIN_SHA` vendoring +pin, and this repo's own independent copy of the collapsing assumption in +`scripts/ci/contextual_orchestrator_review_policy.py`'s `PROVIDER_FAMILIES`. Both were superseded +mid-session by `.github#1468` ("fix(ci): keep sidecar credential accounts independent"), which the repo +owner merged directly and which covers both: it bumps the pin to `contextual-orchestrator`'s then-current +`main` tip (`0adca4703df67f8f31d3ea5b04a1e07ed775dd6c`, later advanced again by `.github#1469`) and +removes `PROVIDER_FAMILIES` entirely, renaming the concept from "provider family" to "provider account" +throughout (`free_family_diversity` → `free_account_diversity`, `family_cap` → `account_cap`). + +**What #1468 did not catch.** Review during this session (a Devin Review finding on the now-closed, +superseded PR #1470, checked directly against `main`'s actual merged code before acting) found that +#1468's fix, while correctly removing the wrong model-catalog assumption, introduced a second, more +subtle conflation on a genuinely different axis. Two independent questions exist for +`nvidia_nim`/`nvidia_nim_sub`: + +1. **Model-catalog identity** — may these two credentials be entitled to different models? Yes. This is + what #941/#945/#1468 correctly fixed. +2. **Outage-domain identity** — would one physical infrastructure outage take both credentials down + together? Also yes: both resolve to the identical `https://integrate.api.nvidia.com/v1` upstream (see + `PROVIDER_BASE_URLS` in `scripts/ci/zdr_policy.py`, and that table's own `nvidia_nim_sub` ZDR-scope + note, which already said as much). #1468's fix, in correcting axis 1, also flattened axis 2 to be + identical to axis 1 -- `provider_account()` (identity-only) became the *sole* grouping key for both + the `free_account_diversity` evidence field and the catalog's admission cap. + +This matters concretely: `free_account_diversity` exists specifically so a caller (open PR #1437, +draft, gating Strix's `orchestrator/free` eligibility) can tell whether a single provider outage could +empty the free catalog -- that is fundamentally an outage-domain question, not a credential-count +question. With the two axes conflated, a discovery report whose only free routes are these two NVIDIA +credentials reports `free_account_diversity == 2`, which would falsely read as "safe" for exactly the +decision this evidence exists to support. Separately, the admission cap (`account_cap`, sidecar default +8) let the two credentials jointly consume up to *twice* its intended per-endpoint budget, which +concretely re-creates a milder version of the 2026-08-30 `orchestrator/free` exhaustion incident this +cap exists to prevent (documented earlier in this file): a shared endpoint's rows could crowd out a +smaller, genuinely independent provider's free routes even when that provider had capacity available. + +**Fix (this PR, a small, focused follow-up against current `main`, not a revival of #1470).** +`scripts/ci/contextual_orchestrator_review_policy.py` gains a second, distinct grouping, +`_outage_domain(row)`, keyed on each row's own `base_url` evidence (not a second hand-maintained +provider-name table, so it cannot silently go stale independently of the `base_url` evidence the catalog +already serves from -- the exact failure mode that made the removed `PROVIDER_FAMILIES` mapping wrong). +The admission cap now groups by outage domain (two same-endpoint credentials share one cap budget, they +do not each get their own); a new report field, `free_outage_domain_diversity`, is added *alongside* the +existing `free_account_diversity` (additive, not a rename, to avoid another naming churn on top of +#1468's very recent one) so a caller like #1437 can read the field that actually answers its question. +`scripts/ci/contextual_orchestrator_review_launcher.py`'s `_with_discovery_counts` (which recomputes +diversity from full discovery-wide rows, not the narrower per-stage set) restores both fields the same +way. `account_cap`/`DEFAULT_ACCOUNT_CAP`/the CLI `--account-cap` flag/the sidecar's +`ORCHESTRATOR_CATALOG_ACCOUNT_CAP` env var names are all left unchanged (still meaningful as "the cap +value"; only its grouping was wrong) to minimize collision risk with `.github#1469`, which was +concurrently advancing the same sidecar's pin in the same active window. + +**Tests.** Two dedicated regressions reproduce the exact gaps: one asserting `nvidia_nim` + +`nvidia_nim_sub` alone report `free_account_diversity == 2` but `free_outage_domain_diversity == 1` +(the semantic-conflation bug), and one reproducing the crowding-out scenario concretely (a shared-endpoint +credential pair with far more free rows than an independent provider; before this fix the independent +provider could be admitted zero rows, after it the shared endpoint's admissions are capped to protect +room for independent providers). Existing tests (`test_build_catalog_applies_account_cap`, +`test_build_catalog_reports_free_account_diversity`, `test_build_catalog_counts_same_vendor_credentials_ +independently`, plus two launcher-facing tests in `test_contextual_orchestrator_review_runtime_ +preflight.py`) were updated to the corrected, domain-aware expectations. Full suite green; 100% coverage +and 100% docstring coverage on `scripts/ci/`. + +**Not touched:** open PR #1437's own gating logic. Its reviewer should read `free_outage_domain_ +diversity`, not `free_account_diversity`, when wiring the `>= 2` eligibility check this file documents. + +**Follow-up (same PR, same day): raw-string comparison would have reintroduced the same class of bug.** +A Devin Review finding on this PR pointed out that `_outage_domain(row)` as first written compared raw +`base_url` strings -- so a hostname-case difference, an explicit default port (`:443`), or a trailing +slash between two rows that are actually the *same* physical endpoint would split them into two outage +domains, silently reintroducing the exact diversity-overstating/cap-bypassing bug this PR set out to fix. +Verified against this codebase's actual code before acting, not assumed: every `DiscoveredModel.chat_ +base_url` in `contextual-orchestrator/contextual_orchestrator/model_discovery.py` traces to one of a +fixed set of hardcoded Python string literals (the `nvidia_nim`/`nvidia_nim_sub` entries are byte- +identical), and this repo's launcher (`_report_rows`) copies that value verbatim, falling back only to +`zdr_policy.PROVIDER_BASE_URLS` -- confirmed byte-identical to the same literals for all five tracked +providers. So the risk is **not reachable through this repo's one production caller (the sidecar/ +launcher) today**. It *is* reachable through `contextual_orchestrator_review_policy.py`'s own public, +independently invocable `--discovery-report` CLI, which reads an arbitrary JSON file and is not +restricted to the launcher's exact generation path -- not wired into any current production workflow +(`hourly-nvidia-nim-review-repair.yml` only runs tests/coverage against this file, never the CLI on live +input), so the risk is latent, not live, but real for that public surface. Given the fix is cheap and +behavior-neutral on every input this repo's sidecar produces today, it was applied rather than left as an +unstated assumption: `_outage_domain` now compares `_normalize_base_url(row["base_url"])`, which +lowercases scheme/host, drops an explicit default port, and strips a trailing slash, while preserving a +different host, non-default port, path, or scheme as genuinely distinct domains, and falling back to a +lowercased/stripped whole-string comparison (never raising) for anything it cannot parse into a scheme, +host, and numeric port. Five new tests cover the exact equivalent-spelling cases Devin named (case, +default port, trailing slash), confirm genuine distinctions still separate, confirm no-raise on malformed +input (including a non-numeric port, which `urlsplit(...).port` raises `ValueError` on), and one +end-to-end test through `build_zdr_prioritized_catalog` itself with two differently-spelled rows for the +same endpoint. Full suite green (2106 tests); 100% coverage (including the new fallback branch) and 100% +docstring coverage on `scripts/ci/`. + +**Second follow-up (same PR, same day): the outage-domain cap itself could starve one credential +entirely.** Two more Devin Review findings on this PR, one severe. + +- **Severe: shared-cap starvation within a domain.** Grouping the admission cap by outage domain (above) + fixed cross-domain crowding-out, but the admission loop still walks rows in one strict sorted + (cost-tier, ZDR, provider, model) order and admits greedily until a domain's cap is reached. Since + `"nvidia_nim" < "nvidia_nim_sub"` in every real fixture, `nvidia_nim`'s rows always sort first -- + meaning `nvidia_nim` alone could consume the *entire* shared cap before a single `nvidia_nim_sub` row + was ever considered. Verified concretely before fixing: 6 free `nvidia_nim` rows + 6 free + `nvidia_nim_sub` rows, `account_cap=4` -> `nvidia_nim_sub` was admitted **zero** rows. Not "prevented + from taking more than its fair share" (the bug already fixed), but "the alphabetically-first credential + can take the *entire* shared budget, the other gets nothing" -- a narrower but just-as-real version of + the same crowding-out problem, now happening *within* one domain instead of across domains. Fixed with + a new `_fair_admission_order()` reordering step, applied before the existing (otherwise unchanged) + greedy admission loop: rows are partitioned by outage domain (preserving each domain's original + position relative to other domains), and within any domain contributed to by more than one account, + rows are taken in round-robin turns across those accounts -- one from each account's own + priority-ordered queue per round -- instead of exhausting whichever account sorts first. A domain with + only one contributing account (every provider except the shared NVIDIA pair, as of this writing) is + returned completely untouched. Re-verified the same scenario after the fix: `nvidia_nim: 2, + nvidia_nim_sub: 2` -- both credentials now contribute. Two existing tests whose assertions had encoded + the starvation behavior (`test_build_catalog_applies_account_cap`, + `test_build_catalog_prevents_shared_endpoint_from_crowding_out_independent_providers`) were corrected + to the fair-split expectation; a new end-to-end regression + (`test_build_catalog_shared_domain_cap_does_not_starve_second_account`) and two unit-level tests + directly against `_fair_admission_order()` (untouched-single-account case; visible round-robin + reordering, including that a multi-account domain's block still starts at its original position among + other domains) were added. +- **Real: `urlsplit()` itself can raise, not only `.port`.** `_normalize_base_url`'s existing fallback + wrapped only the `.port` property access; `urlsplit()` itself raises `ValueError` for an unmatched + IPv6-literal bracket (e.g. `https://[::1/v1`, confirmed: `ValueError: Invalid IPv6 URL`), which happens + earlier, before any scheme/host is even available to inspect -- an uncaught exception past this + function's own "must never raise on evidence it merely groups" contract. Fixed by wrapping the + `urlsplit()` call itself in the same catch-and-fall-back-to-a-lowercased-copy pattern already used for + the `.port` case. One new regression test confirms both a malformed IPv6-bracket URL and its + differently-cased twin fall back to the same, non-raising, normalized value. +- **Noted, not chased further (info-level, optional):** hostname canonicalization stops at lowercasing -- + a trailing root-label dot, an IDN's Unicode vs. punycode form, and differently-compressed-but-equivalent + IPv6 literals are not folded together. None of these shapes occur in any `base_url` this codebase + produces today (every value traces to a fixed set of hardcoded, already-canonical HTTPS hostnames), so + this is documented as a deliberate residual gap in `_normalize_base_url`'s own docstring rather than + implemented prophylactically; a future provider whose entitled address genuinely takes one of these + forms should extend the function with evidence of that specific case. + +Full suite: 2111 passed, 1 skipped, 21 subtests passed. 100% coverage (including the new reordering +function and both new fallback branches) and 100% docstring coverage on `scripts/ci/`. + +**Third follow-up (same PR, same day): the round-robin fix itself broke tier priority, plus a real IPv6 +normalization collision.** Two more Devin Review findings, one a real correctness regression the previous +round introduced. + +- **Real regression: fairness reordering could drop a free route for a paid one.** The round-robin fix + above grouped every row belonging to one outage domain into a single contiguous block, emitted at the + position of that domain's *first* appearance in the (already tier-sorted) input -- but a domain's rows + can span multiple admission-priority tiers (e.g. `openai` contributes both a free and a priced route, + same single-account domain). Grouping by domain first, tier-blind, let a domain's lower-tier row (e.g. + priced) get pulled into the same block as its higher-tier row (free), ahead of a *different* domain's + higher-tier row that only sorted later because of the `(provider, model)` tie-break. Verified + concretely before fixing: sorted input `[free openai, free openrouter, priced openai]` reordered to + `[free openai, priced openai, free openrouter]`, and with `limit=2` the genuinely free `openrouter` + route was dropped in favor of the priced `openai` route -- a real correctness regression for a catalog + whose entire purpose is admitting free/ZDR routes preferentially. Fixed by scoping the round-robin + fairness pass strictly *within* one admission-priority tier at a time: `eligible_rows` and + `_fair_admission_order` now share one `_admission_priority_key()` function (the sort key and the + tier-boundary detector can no longer silently drift apart), the input is split into contiguous + same-tier runs (safe, since it is already tier-sorted), and the existing domain/account round-robin + logic (renamed `_fair_order_within_tier`) is applied independently to each run, then the runs are + concatenated back in their original order. Re-verified: the same scenario now correctly keeps + `[free openai, free openrouter]` under `limit=2`; the starvation-fix regression scenario from the + previous round still passes unchanged (both were verified together, programmatically, before + committing). Added a unit-level regression directly against `_fair_admission_order` and an end-to-end + regression through `build_zdr_prioritized_catalog`. +- **Real: IPv6 host normalization could collide two different endpoints.** `urlsplit().hostname` strips + IPv6 literal brackets (`[::1]` -> `::1`); appending a port without re-adding them meant an explicit-port + IPv6 URL (`https://[::1]:8443/v1`, host `::1` port `8443`) and an unrelated literal that merely contains + the same colon-digit sequence (`https://[::1:8443]/v1`, one IPv6 address, no separate port) both + normalized to the identical, syntactically-invalid `::1:8443` -- two genuinely different endpoints + undercounted as one outage domain, in addition to producing malformed reassembled URL syntax either + way. Fixed by re-wrapping a colon-bearing host in brackets before ever conditionally appending a port. + Re-verified: the two example URLs now normalize distinctly, a default IPv6 port is still correctly + dropped, and the malformed-IPv6-bracket fallback from the previous round still works unchanged. Two new + regression tests. +- **Optional perf nit, applied since already in this code:** the round-robin queues switched from + list-`pop(0)` (O(n) per pop) to `collections.deque.popleft()` (O(1)) -- current catalog sizes make this + immaterial, but the change was one import and two identifiers. + +Full suite: 2115 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on +`scripts/ci/`. + +## 2026-08-31 `.github`-side half of OpenRouter's premature evidence_only exclusion + +**Confirmed bug.** `scripts/ci/contextual_orchestrator_review_launcher.py`'s `_routable_discovered_models()` +(called at the top of `main()`, before any live-serving selection) unconditionally dropped every discovery +row with `evidence_only=True`. `contextual-orchestrator`'s OpenRouter `ProviderModelSource` hardcodes +`evidence_only=True` for *every* discovered model unconditionally -- not computed per model from real +evidence, even though genuine per-model ZDR evidence (`_openrouter_zdr_model_ids`/`_apply_discovered_ +model_evidence`, feeding the `zdr_capable` field) is fetched and parsed for OpenRouter in that same +module. The upstream half of this bug was fixed separately, in +`ContextualWisdomLab/contextual-orchestrator#949` ("fix(discovery): route OpenRouter by model evidence"), +merged at `8cd99f139915131ba0239bce12a5d6a5fd85394e`. + +The consequence for this repo specifically: with 100% of OpenRouter rows carrying `evidence_only=True`, +`_routable_discovered_models()` excluded ALL OpenRouter discovery rows before `scripts/ci/zdr_policy.py`'s +own purpose-built, already-correct, already-wired per-route OpenRouter ZDR-feed check +(`is_zdr_model()`'s `openrouter_endpoints_feed` branch, an exact `route_key(provider, model) in +zdr_endpoints` match against OpenRouter's authoritative `/api/v1/endpoints/zdr` feed) ever got a chance to +evaluate a single OpenRouter row -- making that mechanism dead code for OpenRouter specifically, and +leaving OpenRouter contributing zero routes to any pool (free, auto, or ZDR-required private targets), +even though it genuinely offers ZDR-attested free models via its own documented feed. + +**Fix.** OpenRouter rows are now exempt from the `evidence_only` exclusion in `_routable_discovered_ +models()`. A genuinely non-servable OpenRouter row (e.g. a non-chat listing) is still excluded downstream +by the same provider-agnostic chat-capability check every other provider's rows already go through +(`is_general_chat_agent_model_id` + `_has_text_output`, in `main()`) -- so this exemption relies on that +existing, independent check, not on trusting `evidence_only`'s current, wrong, blanket value for +OpenRouter. + +**Sequencing correction, verified before writing this record.** The task as described expected this fix +to be "inert" until both the upstream `contextual-orchestrator` fix and a matching `ORCHESTRATOR_PIN_SHA` +bump land. Traced through the actual code before accepting that: OpenRouter discovery already runs in +this sidecar today (`OPENROUTER_API_KEY` is one of the five KV-registered credentials), and for the +*general* (non-private, `require_zdr=False`) pool -- which is what Noema/OpenCode/the default Strix path +use -- `_zdr_admitted_rows()` returns every row unfiltered regardless of ZDR status; `is_zdr_model()` only +affects sort priority and tagging there, never admission. So this fix has real, immediate effect once +merged: genuinely chat-capable OpenRouter rows, currently blocked here regardless of what +`contextual-orchestrator` reports, start reaching selection as soon as this lands -- not only once the +upstream `evidence_only` fix and pin bump also land. What remains genuinely gated on the upstream fix is +OpenRouter rows being correctly excluded from `evidence_only` on a real per-model basis (a non-chat +listing, say); until then this function's only remaining protection against those is the downstream +chat-capability check, not `evidence_only`. Documented explicitly in `_routable_discovered_models()`'s own +docstring and in the PR description so a reviewer isn't surprised by observable behavior change before the +upstream PR merges. + +**Tests.** `test_routable_discovered_models_excludes_evidence_only_rows` (existing) corrected to use a +non-OpenRouter provider for its `evidence_only=True` fixture, since that scenario no longer applies to +OpenRouter; a new `test_routable_discovered_models_exempts_openrouter_from_evidence_only` regression +confirms both an `evidence_only`-tagged and an untagged OpenRouter row pass through while a same-shaped +row from a different provider does not. A contract-test assertion was added to `test_contextual_ +orchestrator_review_sidecar_contract.py` pinning the exemption's presence in source, matching this +repo's existing pattern of pinning exact prose/structure in trusted scripts. Full suite: 2093 passed, 1 +skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on `scripts/ci/`. + +## 2026-08-31 follow-up: making the OpenRouter `evidence_only` exemption self-correcting + +**Gap raised by Devin Review on `ContextualWisdomLab/.github#1476`.** The blanket exemption above does not +distinguish "the vendored `contextual-orchestrator` still has the confirmed blanket-`evidence_only=True` +bug" from "the vendored copy now computes `evidence_only` correctly per model" (the fix originally tracked +here as `ContextualWisdomLab/contextual-orchestrator#950` — **since corrected: `#950` was closed as +redundant/superseded, and the fix instead merged as `ContextualWisdomLab/contextual-orchestrator#949`**, +"fix(discovery): route OpenRouter by model evidence", at `8cd99f139915131ba0239bce12a5d6a5fd85394e`; see +the 2026-08-31 correction subsection below). Left unconditional forever, this launcher would keep admitting +genuinely evidence-only (non-ZDR-attested) OpenRouter rows even after `#949` merges and +`ORCHESTRATOR_PIN_SHA` is bumped past it -- silently defeating the very fix `#949` delivers, for exactly the +population of rows `evidence_only` exists to gate. + +**Design considered: pin-SHA ancestry.** The base SHA this design work compared against +(`c107e3e52371993aa9c326fcc245e01c41fc3850`) is confirmed to equal this repo's then-current +`ORCHESTRATOR_PIN_SHA` default (`scripts/ci/contextual_orchestrator_review_sidecar.sh`), so once a fix +merged upstream its resulting SHA on `contextual-orchestrator`'s `main` would become the natural gating +threshold. The sidecar's vendored clone at `$ORCHESTRATOR_SOURCE` is a non-shallow (`--filter=blob:none`, +full commit/tree history, blobs only) clone of every ref, so +`git -C "$ORCHESTRATOR_SOURCE" merge-base --is-ancestor "$ORCHESTRATOR_PIN_SHA"` is technically +reachable at review-time. It was not implemented at the time: neither candidate fix (`#950`, later closed; +`#949`, the one that actually merged) had landed yet, so no concrete fix-commit SHA existed to gate on, and +wiring the check in ahead of that would have required new plumbing (passing `$ORCHESTRATOR_SOURCE` or the +pin itself into the launcher via a new CLI argument/env var, a `subprocess` git call, and matching +`contextual_orchestrator_review_sidecar.sh` / contract-test changes) built against a threshold this org did +not yet have -- over-engineering ahead of the actual need. `#949` has since merged and +`ContextualWisdomLab/.github#1477` (open as of this correction) advances `ORCHESTRATOR_PIN_SHA` straight to +its merge commit, so a concrete fix SHA now exists -- but the observed-behavior check below already covers +the need without this plumbing, so pin-ancestry tracking remains unimplemented by choice, not by necessity. + +**Fix implemented instead: an observed-behavior signature check, not a version marker.** +`_openrouter_reports_per_model_evidence()` (`scripts/ci/contextual_orchestrator_review_launcher.py`) reads +this run's own discovered OpenRouter rows: if at least one reports `evidence_only=False`, that is real +per-model evidence, and `_routable_discovered_models()` immediately stops exempting OpenRouter and applies +the same `evidence_only` contract every other provider already gets -- unattested OpenRouter rows are +excluded, attested ones pass on their own merit. While every OpenRouter row still reports +`evidence_only=True` (today's exact, confirmed bug signature), the historical exemption stays active. This +needs no pin tracking, no `subprocess` calls, and no manual conversion step once `#949` merges: the check +self-corrects the moment the vendored pin actually includes the fix and a run observes real per-model +variation, because it is reading the vendored code's actual output rather than trusting a commit SHA to +imply that output. Documented as the more robust, less brittle choice for this reason in +`_openrouter_reports_per_model_evidence()`'s own docstring. `#949`'s actual merged diff confirms this +premise directly: it removes the `evidence_only=True` hardcode from OpenRouter's `ProviderModelSource` +entirely rather than computing a per-model value, so `DiscoveredModel.evidence_only` defaults to `False` +for every OpenRouter row the moment the pin is bumped past it -- exactly the "at least one row reports +`evidence_only=False`" signature this check watches for. + +**Known, accepted limitation, documented in the same docstring.** A genuinely-fixed vendored copy that +reports `evidence_only=True` for every OpenRouter row in one particular run -- a total ZDR-feed-fetch +failure that run (`#949`'s own documented fail-closed behavior, restated in its ADR 0032 update: "Missing +or failed ZDR evidence therefore fails closed only for `zdr_only` selection, not for general inference"), +or simply zero ZDR-attested OpenRouter models discovered that run -- is indistinguishable from the +still-buggy blanket signature by this check alone, and the exemption stays active for that one run. This +only widens which OpenRouter rows reach the same downstream, `evidence_only`-independent chat-capability +check every other provider's rows already pass through; it does not touch the separate ZDR admission gate +(`is_zdr_model()` / `_zdr_admitted_rows()`) that guards `--require-zdr` private targets, which never +depended on `evidence_only` in the first place (see the 2026-08-31 correction subsection below, which +traces this claim end to end against a second, independent Devin Review finding that questioned it). + +**TODO, resolved by the 2026-08-31 correction below.** The original TODO here asked to re-verify this +reasoning once `#950` merged and `ORCHESTRATOR_PIN_SHA` was bumped past it. `#950` never merged (closed as +redundant/superseded); `#949` merged instead. That re-verification against `#949`'s actual merged diff and +test suite is now done (2026-08-31 correction subsection below) -- `#949`'s tests exercise the same +"OpenRouter model absent from the ZDR feed keeps that row correctly gated" and "ZDR-feed-fetch failure fails +closed" shapes this TODO named, under different test names than originally guessed +(`test_discover_all_models_blocks_only_paid_openrouter_without_credit` and the existing ZDR-feed tests in +`tests/test_model_discovery.py`, per `#949`'s diff). The one action still pending is operational, not +analytical: confirm in a real CI run, once `ContextualWisdomLab/.github#1477` merges and +`ORCHESTRATOR_PIN_SHA` actually advances, that `_openrouter_reports_per_model_evidence()` observes the +expected per-model variation and the exemption turns itself off with no further code change. If real-world +experience ever shows the observed-behavior check's known limitation above firing often enough to matter +(e.g. OpenRouter's ZDR feed proves flaky in practice), revisit the pin-ancestry alternative recorded above, +now that a concrete fix-commit SHA (`8cd99f139915131ba0239bce12a5d6a5fd85394e`) exists to gate on. + +**Tests.** `test_routable_discovered_models_exempts_openrouter_from_evidence_only` (previous entry's +regression) split into two: `test_routable_discovered_models_exempts_openrouter_when_every_row_reports_ +evidence_only` (blanket-`True` pre-fix signature -- both OpenRouter rows pass) and +`test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence` (mixed +post-fix signature -- the unattested row is now excluded, matching the same-shaped non-OpenRouter case). +Full suite: 2094 passed, 1 skipped, 21 subtests passed. 100% coverage (the launcher stays coverage-omitted +per `pyproject.toml`'s existing, unchanged rationale -- it imports the vendored library only present inside +the sidecar's own runtime) and 100% docstring coverage on `scripts/ci/`. + +## 2026-08-31 correction: #949 merged (not #950), `spend_admitted` traced, a second Devin finding closed + +**Correction: the upstream PR number.** Both entries above, and `ContextualWisdomLab/.github#1476`'s own +original PR body, named `ContextualWisdomLab/contextual-orchestrator#950` as "the upstream half of this +bug, not yet merged." That was wrong. `#950` was closed as redundant/superseded. The PR that actually +merged is a different one, `ContextualWisdomLab/contextual-orchestrator#949` ("fix(discovery): route +OpenRouter by model evidence"), at `8cd99f139915131ba0239bce12a5d6a5fd85394e`. Separately, +`ContextualWisdomLab/.github#1477` (open as of this correction) already bumped this repo's +`ORCHESTRATOR_PIN_SHA` (`scripts/ci/contextual_orchestrator_review_sidecar.sh`) to that exact commit, +pending `#1477`'s own merge. Every other reference to `#950` above is corrected in place; the reasoning +itself needed no other change -- it was always about the *behavior* the observed-behavior check watches +for, not about which PR number delivered it. + +**New fact from `#949`'s actual diff, not knowable from the original entries: `spend_admitted`.** `#949` did +more than compute `evidence_only`/`zdr_capable` per model. It also added a new field, +`spend_admitted: bool = True`, to `DiscoveredModel` (`contextual_orchestrator/model_discovery.py`), and a +new `apply_openrouter_spend_admission()` helper: for a **priced** (non-`is_free`) OpenRouter row, whenever +`openrouter_paid_inference_available()` does not affirmatively return `True` (i.e. returns `False` or +`None` -- no usable credit, or the check itself failed), that row's `spend_admitted` becomes `False` +(fail-closed). A **free** OpenRouter row's `spend_admitted` is always `True`, unconditionally, regardless of +credit status -- `apply_openrouter_spend_admission` short-circuits on `model.is_free`. `is_routable_ +discovered_model()` (the vendored library's own agent-activation gate) was updated to require +`spend_admitted` in addition to `not evidence_only`, and `agent_from_discovered()`/`serving_tags_for_ +discovered()` now tag a blocked row `spend:blocked`. + +**Investigated: does this repo's own review-catalog pipeline need to respect `spend_admitted`, or is it +already safe without it?** Traced `scripts/ci/contextual_orchestrator_review_launcher.py`'s `main()` in +full. It calls `discover_all_models()` directly and receives real `DiscoveredModel` rows (not some +already-filtered surface), so `spend_admitted` genuinely reaches this repo's code -- but only priced rows +can ever have `spend_admitted=False` (see above), and this launcher's default, and *every* current call +site's actual configured pool (`CONTEXTUAL_ORCHESTRATOR_POOL`, unset almost everywhere and explicitly `free` +in `strix.yml`), is `--pool free`. Under `--pool free`, `main()`'s `selected_models` loop drops every row not +in `free_route_identities` *before* it ever becomes a report row (`if args.pool == "free" and +_route_identity(model) not in free_route_identities: continue`) -- so no priced row, and therefore no +`spend_admitted=False` row, ever reaches this repo's catalog today. `scripts/ci/contextual_orchestrator_ +review_policy.py`'s `build_zdr_prioritized_catalog()` reinforces this independently: for `pool="free"` its +`candidate_rows` is `all_free_rows` only, never `all_priced_rows`. + +**The real, latent gap: `--pool auto`.** `--pool auto` is real, tested, wired code -- selectable today via +the `CONTEXTUAL_ORCHESTRATOR_POOL=auto` environment variable with no further code change, even though no +current workflow sets it. Under `auto`, priced rows are genuine candidates (`primary_rows = admitted_free_ +rows or admitted_priced_rows`, plus an explicit priced-fallback stage in `main()` and `[*all_free_rows, +*all_priced_rows]` in `build_zdr_prioritized_catalog()`). Neither of those priced-row paths, nor +`_report_rows()` (which builds report rows from selected `DiscoveredModel`s), ever read or propagated +`spend_admitted` -- so before this correction, a spend-blocked (credit-exhausted) paid OpenRouter row could +reach `orchestrator/auto`'s served catalog exactly as if it were servable. This matches this org's stated +direction that the review catalog is meant to be free+ZDR-only ("free+ZDR 조합도 해결 못하는데 유료 모델 +포함된 auto 써서 뭐 하려고"), so `auto`'s existence is itself a separate, pre-existing scope question this +correction does not resolve -- but as long as `--pool auto` is live, reachable code, it must not admit a row +the vendored library itself now refuses to activate as an agent. + +**Fix.** `_routable_discovered_models()` now excludes `getattr(model, "spend_admitted", True) is False` rows +the same way it excludes `evidence_only=True` rows -- unconditionally, with no self-correcting exemption +(unlike the OpenRouter `evidence_only` exemption, `spend_admitted` was never wrongly blanket-set for every +OpenRouter row, so there is no equivalent bug shape to work around). The `getattr(..., True)` default keeps +this correct against the currently-pinned vendored copy too, which predates `#949` and has no +`spend_admitted` attribute at all -- exactly the same forward-compatible pattern already used for +`evidence_only`. Regression tests cover: a `spend_admitted=False` row excluded regardless of provider or +pool; a `spend_admitted=True` row and a row with the attribute entirely absent both still pass; and an +end-to-end `--pool auto` composition (`_routable_discovered_models` → `_report_rows` → `parse_discovery_ +report` → `build_zdr_prioritized_catalog`) proving a credit-exhausted priced OpenRouter row no longer +reaches the built catalog. + +**Second Devin Review finding on `ContextualWisdomLab/.github#1476`, investigated and closed as a false +alarm (discussion `r3891875749`, 🟥 "Private code can reach forbidden routes").** The finding: because +`_routable_discovered_models()` converts every OpenRouter row into a candidate while every row still shows +`evidence_only=True`, "private review content \[could\] reach third-party routes the vendored ZDR contract +forbids serving." Traced the full `--require-zdr` path (`CONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDR`, set from +`is_private`/target-visibility in `noema-review.yml`, `strix.yml`, and `opencode-review-dispatch.yml`) end +to end: becoming a *candidate* that survives `_routable_discovered_models()` is not the same as being +*admitted* to a private target's served catalog. The actual, independent admission gate for a +`--require-zdr` build is `is_zdr_model()` (`scripts/ci/zdr_policy.py`), which for OpenRouter requires an +exact `route_key(provider, model)` match against the real, live `/api/v1/endpoints/zdr` feed and fails +closed (`False`) whenever that feed is empty or the model is unset. `build_zdr_prioritized_catalog()` -- +the function that actually produces the served `agents` catalog for both the `free` and `auto` pools -- +re-applies this exact `is_zdr_model()` check as its own `eligible_rows` filter whenever `require_zdr=True`, +independent of whatever `_routable_discovered_models()` already did upstream; `evidence_only` plays no part +in that filter at all. So a row exempted from `evidence_only` still cannot reach a private target's catalog +unless it also genuinely matches OpenRouter's own authoritative ZDR feed -- at which point, by OpenRouter's +own definition, it *is* a zero-data-retention route, satisfying the actual contract the finding is +concerned about. The separate, provider-agnostic chat-capability check (`is_general_chat_agent_model_id` + +`_has_text_output`, in `main()`, applied uniformly before any pool split) additionally guards against a +non-chat metadata stub being admitted regardless of pool or privacy requirement. No code change was made +for this finding; a regression test +(`test_require_zdr_still_excludes_non_zdr_openrouter_route_despite_evidence_only_exemption`) composes the +real pipeline (`_routable_discovered_models` → `_report_rows` → `parse_discovery_report` → +`build_zdr_prioritized_catalog(..., require_zdr=True)`) to prove this holds even while every discovered +OpenRouter row still carries `evidence_only=True`, and the GitHub review thread was replied to and marked +resolved with this reasoning. ## 2026-08-31 noema-review-gate: malformed LLM JSON crashed the required check instead of failing closed The required `noema-review` check on `ContextualWisdomLab/contextual-orchestrator#960` crashed with an @@ -2344,6 +2755,54 @@ contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr "today" reference. Landed in the same PR (`#1463`) as the streaming revert, not split out, since the revert is unsafe without it. +## 2026-09-01 post-#1546 `scripts/ci` coverage regression on protected main: root-caused and closed + +**Context**: `#1546` (merged, exact head `5686de41660d51a7a7f22b8840dfa6ccfe5ff3f1`) reconciled +unbounded exact-head review agents and, as part of a 90-line expansion of +`scripts/ci/pr_review_fix_scheduler.py`, added a `live_head_matches` helper, a no-active/no-stale +fall-through branch in `prepare_autofix_slot`, and an "already queued or running" wait branch in +`inspect_pr` — none of which any test exercised directly. This compounded a narrower, older gap in +the same file (`inspect_pr`'s conflicted-draft and conflicted-unauthorized returns) and in +`scripts/ci/pr_review_merge_scheduler.py::fetch_workflow_names_by_check_suite_rest` (pagination, +missing-suite-id/blank-name filtering, non-access-error propagation), first found and attempted in +now-closed, unmerged `#1547`/`#1551`/`#1554` — none of whose evidence or diffs transferred here; +this pass re-derived the current gap from a clean `origin/main` clone rather than assuming those +predecessors were still accurate against `#1546`'s shifted line numbers and new branches. Verified +directly: `coverage report --show-missing` on unmodified `main` showed +`scripts/ci/pr_review_fix_scheduler.py` at 97% (missing 116-121, 459->466, 495, 503, 546) and +`scripts/ci/pr_review_merge_scheduler.py` at 99% (missing 1003, 1008->1005, 1012) — total repo-wide +99%, below the `pyproject.toml` `fail_under = 100` gate. Because `opencode-review-dispatch.yml`'s +`coverage-evidence` job measures the **merged** PR tree (base + head) and hard-fails below 100%, +every PR rebasing onto main inherited this failure regardless of its own diff — org-wide impact, +not scoped to one PR. + +**Fix**: `#1567` (test-only, no production code) adds direct unit coverage for `live_head_matches` +(case-insensitive match, mismatch, malformed-payload paths), `prepare_autofix_slot`'s empty-run +fall-through, the `inspect_pr` conflicted-draft/conflicted-unauthorized/already-queued cases, and +the `fetch_workflow_names_by_check_suite_rest` pagination/filtering/error-propagation paths. +Verified on the fix commit (`db106d50f2134ece147bc5318e389aeb124d198c`): `coverage run -m pytest +tests -q` (2251 passed, 1 skipped, 21 subtests), `coverage report` (repo-wide 100%, both files +individually 100% statement and 100% branch), `interrogate` (100.0%). + +**Devin Review raised a false positive on the fix itself**, claiming +`test_live_head_matches_compares_case_insensitively_and_fails_closed` left non-object-payload, +non-string-SHA, and wrong-length-SHA branches uncovered. Re-verified against the actual gate rather +than accepted at face value: `live_head_matches` has exactly one `if` statement (two arcs, both +exercised by the committed test), and its final `return (isinstance(...) and len(...) == 40 and +...)` is a single boolean expression with no `if`/`else` of its own — `coverage.py`'s branch mode +(what `fail_under = 100` actually measures here) tracks control-flow arcs between statements, not +sub-clause condition coverage within one expression. The cited cases are additional test +thoroughness, not something the gate is currently failing on; confirmed by a full-suite run on the +exact same head showing both files at 100% branch coverage with zero missing branches. Replied with +this evidence on the review thread and did not widen the PR's diff for a claim that does not hold +against this repo's own tooling. + +**One test in the full suite remains a known, pre-existing flake**, unrelated to this change: +`tests/test_opencode_required_verdict_regression.py::test_scheduler_wake_reuses_trusted_receipt_predicate` +intermittently exits 141 (SIGPIPE) under full-suite parallel load; reproduces identically on +unmodified `origin/main` and passes cleanly in file isolation. Not remediated here — out of scope +for a coverage-gap-only PR, and not itself a coverage regression. + ## 2026-09-01 naruon#1486 transport-crash: root cause, owner, status **Live incident**: the required `noema-review` check on `ContextualWisdomLab/naruon#1486` crashed with an diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 2e56809639..f7186b4cee 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -116,17 +116,186 @@ def _log_discovery_errors(errors: list[object]) -> None: print(_DISCOVERY_DIAGNOSTICS_COMPLETE_SENTINEL, file=sys.stderr, flush=True) +def _openrouter_reports_per_model_evidence(discovered: list[object]) -> bool: + """Return whether this run's OpenRouter rows show real per-model evidence. + + ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` had a + confirmed bug in the vendored copy pinned as of this writing: it + hardcodes ``evidence_only=True`` for *every* discovered OpenRouter + model unconditionally, at the provider-source level, regardless of that + specific model's own evidence -- so today's real, observable signature + is that literally every discovered OpenRouter row carries + ``evidence_only=True``, with zero exceptions, even for genuinely + servable, ZDR-attested models. ``ContextualWisdomLab/contextual- + orchestrator#949`` ("fix(discovery): route OpenRouter by model + evidence", merged at ``8cd99f139915131ba0239bce12a5d6a5fd85394e``) fixes + this by removing the hardcode entirely rather than computing a + per-model value: once a run's vendored pin includes the fix, + ``evidence_only`` falls back to its ``False`` dataclass default for + *every* discovered OpenRouter row, unconditionally -- ZDR attestation + for OpenRouter rows is carried separately, on the ``zdr_capable`` field + (already computed per model from OpenRouter's own + ``/api/v1/endpoints/zdr`` feed match both before and after #949), not on + ``evidence_only``. + + This function is this run's *observed-behavior* signal for which of + those two shapes is currently vendored, used by + ``_routable_discovered_models`` in place of tracking + ``ORCHESTRATOR_PIN_SHA`` (the sidecar's vendored-commit pin, defined in + ``scripts/ci/contextual_orchestrator_review_sidecar.sh``) directly. A + pin-ancestry check (``git merge-base --is-ancestor + "$ORCHESTRATOR_PIN_SHA"``) was considered and is technically reachable + -- the sidecar's vendored clone at ``$ORCHESTRATOR_SOURCE`` keeps full + commit history (only blob content is filtered) -- but at the time this + was written neither candidate upstream fix had merged yet, so no fix + commit SHA existed to compare against, and wiring one in would have + needed a new CLI argument/env var carrying ``$ORCHESTRATOR_SOURCE`` (or + the pin itself) into this launcher, a ``subprocess`` git call, and + matching sidecar-script/contract-test changes -- real plumbing built + against a threshold value that did not exist yet. This observed-behavior + check needs none of that: it reads the same ``discovered`` rows this + function already receives, requires no new plumbing, and -- unlike a + pin comparison -- keeps working even if a fix is ever backported to the + vendored fork without a matching ``ORCHESTRATOR_PIN_SHA`` bump, since it + looks at what the vendored code actually returned this run rather than + which commit is nominally pinned. See ``ContextualWisdomLab/.github#1476`` + (this change) and ``ContextualWisdomLab/contextual-orchestrator#949`` + (the merged upstream fix; pinned by ``ContextualWisdomLab/.github#1477``) + for the full history; once ``#1477`` merges, no further change is + required here -- this check starts reporting ``True`` as soon as the + vendored pin actually includes the fix and OpenRouter has discovered at + least one model that run (``#949`` makes ``evidence_only=False`` + unconditional for OpenRouter, not contingent on that model being + ZDR-attested). + + KNOWN, ACCEPTED LIMITATION: a genuinely-fixed vendored copy that + happens to report ``evidence_only=True`` for *every* OpenRouter row in + one particular run -- e.g. OpenRouter discovery itself failing entirely + that run, so ``discovered`` carries no OpenRouter rows to observe real + evidence from at all -- is indistinguishable from the still-buggy + blanket signature by this check alone, and the historical exemption + stays active for that one run. This mirrors the fail-open-on-ambiguity + reasoning already used elsewhere in this module (e.g. the KNOWN GAP + entries above) rather than inventing a new policy; a false-negative + here only widens which OpenRouter rows reach the same downstream, + provider-agnostic chat-capability check every other provider's rows + already pass through (``is_general_chat_agent_model_id`` + + ``_has_text_output``, in ``main()``) -- it does not bypass the + separate, ``evidence_only``-independent ZDR admission gate + (``is_zdr_model()`` / ``_zdr_admitted_rows()``, and the equivalent + filter re-applied inside ``build_zdr_prioritized_catalog()``) that + guards private, ``--require-zdr`` targets -- traced end to end and + confirmed still intact in the 2026-08-31 correction entry of + ``docs/product-technical-gap-baseline.md`` in response to a second + Devin Review finding that raised exactly this question. + + Args: + discovered: The full discovery-wide row set for this run (already + filtered to nothing upstream -- this must see every row, + including non-OpenRouter ones, though only OpenRouter rows are + inspected). + + Returns: + ``True`` once at least one discovered OpenRouter row reports + ``evidence_only=False`` (real per-model evidence observed this + run); ``False`` when there are no OpenRouter rows at all, or every + OpenRouter row is ``evidence_only=True`` (today's confirmed bug + signature, or an indistinguishable run where OpenRouter discovery + itself produced no rows). + """ + return any( + getattr(model, "provider_name", None) == "openrouter" + and not getattr(model, "evidence_only", False) + for model in discovered + ) + + 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. + Evidence-only rows (e.g. a provider's pure price/policy-scraping stub) + exist solely to supply metadata 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. + + OpenRouter rows are conditionally exempt from this exclusion -- + see ``_openrouter_reports_per_model_evidence`` for the full rationale + and its documented limitation. In short: ``contextual-orchestrator``'s + OpenRouter ``ProviderModelSource`` currently hardcodes + ``evidence_only=True`` for every discovered model unconditionally (a + confirmed bug, fixed upstream at + ``ContextualWisdomLab/contextual-orchestrator#949``, merged at + ``8cd99f139915131ba0239bce12a5d6a5fd85394e`` -- not yet pinned in this + repo as of this writing; see ``ContextualWisdomLab/.github#1477``) -- + not computed per model from real evidence, even though + genuine per-model ZDR evidence is fetched and parsed for OpenRouter in + that same module. Applying this filter to OpenRouter verbatim while + that bug is live would strip every OpenRouter row, including genuinely + servable, chat-capable ones, before ``zdr_policy.is_zdr_model()``'s + purpose-built, per-route OpenRouter ZDR-feed check (``openrouter_ + endpoints_feed``) ever gets a chance to evaluate them -- making that + already-correct, already-wired mechanism dead code for OpenRouter + specifically. So the exemption applies only while this run's own + OpenRouter rows still match that exact blanket-``True`` bug signature; + the moment any OpenRouter row in a run reports real per-model evidence + (``evidence_only=False``), OpenRouter rows go back through the same + ``evidence_only`` contract every other provider already gets -- + automatically, with no pin bump or manual edit needed here. A + genuinely non-servable OpenRouter row is still excluded downstream by + the same provider-agnostic chat-capability check every other provider's + rows already go through (``is_general_chat_agent_model_id`` + + ``_has_text_output``, in ``main()``) regardless of which branch this + function takes. + + This exemption is expected to have real, live effect once merged (not + only once ``contextual-orchestrator``'s own per-model ``evidence_only`` + fix and a matching ``ORCHESTRATOR_PIN_SHA`` bump land): OpenRouter + discovery already runs in this sidecar today, so genuinely chat-capable + OpenRouter rows -- currently blocked here regardless of what + ``contextual-orchestrator`` reports -- start reaching selection + immediately. What remains genuinely blocked on the upstream fix is + OpenRouter rows being correctly excluded from ``evidence_only`` on a + real per-model basis (e.g. a non-chat listing); until + ``ContextualWisdomLab/.github#1477`` lands the ``#949`` pin bump and a + run observes real per-model variation, this function's remaining + protection against those is the same downstream chat-capability check, + not ``evidence_only``. + + A row is also excluded whenever ``getattr(model, "spend_admitted", + True) is False`` -- the same treatment as ``evidence_only=True``, with + no self-correcting exemption (there is no equivalent blanket-bug shape + to work around: ``spend_admitted`` was never wrongly ``False`` for + every OpenRouter row). ``contextual-orchestrator#949`` added this field + to ``DiscoveredModel`` (default ``True``) and sets it ``False`` only for + a *priced* OpenRouter row when ``openrouter_paid_inference_available()`` + does not affirmatively confirm usable credit; a free OpenRouter row is + always ``spend_admitted=True`` regardless of credit status. The + vendored library's own ``is_routable_discovered_model()`` already + refuses to activate such a row as an agent; this mirrors that refusal + here so a spend-blocked row can never reach ``orchestrator/auto``'s + priced-fallback path either (``orchestrator/free`` never considers + priced rows at all, so it was never exposed to this). The + ``getattr(..., True)`` default keeps this correct against a vendored + pin that predates ``#949`` and has no ``spend_admitted`` attribute at + all. See the 2026-08-31 correction entry in + ``docs/product-technical-gap-baseline.md`` for the full investigation. """ - return [model for model in (discovered or []) if not getattr(model, "evidence_only", False)] + discovered = list(discovered or []) + openrouter_still_blanket_marked = not _openrouter_reports_per_model_evidence(discovered) + return [ + model + for model in discovered + if ( + not getattr(model, "evidence_only", False) + or ( + openrouter_still_blanket_marked + and getattr(model, "provider_name", None) == "openrouter" + ) + ) + and getattr(model, "spend_admitted", True) is not False + ] def _route_identity(model: object) -> tuple[str, str]: @@ -678,18 +847,35 @@ def _with_discovery_counts( rows: list[dict[str, Any]], *, provider_account: Any, + outage_domain: 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. + ``free_account_diversity`` and ``free_outage_domain_diversity`` are both + 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 diversity fields, as returned by + ``build_zdr_prioritized_catalog`` from whatever narrower row set it was + given, would otherwise contradict those fields' documented "among *all* + discovered free routes" contract. + + ``provider_account`` and ``outage_domain`` are two deliberately distinct + groupings (see ``contextual_orchestrator_review_policy._outage_domain``'s + docstring): the former treats every credential as independent regardless + of vendor, the latter groups credentials that share one physical + upstream endpoint (e.g. ``nvidia_nim``/``nvidia_nim_sub``, both + ``https://integrate.api.nvidia.com/v1``) into one outage domain. + + ``free_pool_admitted_routes``, ``free_pool_excluded_source_count``, and + ``free_pool_account_diversity`` restore the same discovery-wide-vs-stage + distinction for the ``pool="free"`` admission boundary: a free route + whose ``credential_key`` is not in ``FREE_POOL_CREDENTIAL_NAMES`` (e.g. + ``OPENAI_API_KEY``) is globally discoverable and counted in + ``free_account_diversity``/``free_outage_domain_diversity`` above, but is + never itself admissible to the ``free`` pool's served catalog. """ free_rows = [row for row in rows if row.get("cost_evidence") == "free"] free_pool_rows = [ @@ -708,6 +894,9 @@ def _with_discovery_counts( "free_account_diversity": len( {provider_account(str(row["provider"])) for row in free_rows} ), + "free_outage_domain_diversity": len( + {outage_domain(row) for row in free_rows} + ), "free_pool_admitted_routes": len(free_pool_rows), "free_pool_excluded_source_count": len(free_rows) - len(free_pool_rows), "free_pool_account_diversity": len( @@ -797,6 +986,7 @@ def main(argv: list[str] | None = None) -> int: DEFAULT_ACCOUNT_CAP, PolicyError, _load_zdr_endpoints, + _outage_domain, build_zdr_prioritized_catalog, is_zdr_model, parse_discovery_report, @@ -872,9 +1062,13 @@ def main(argv: list[str] | None = None) -> int: zdr_endpoints=zdr_endpoints, require_zdr=args.require_zdr, pool=args.pool, + guarantee_domain_coverage=True, ) result["report"] = _with_discovery_counts( - result["report"], normalized_rows, provider_account=provider_account + result["report"], + normalized_rows, + provider_account=provider_account, + outage_domain=_outage_domain, ) Path(args.catalog_out).write_text( json.dumps({"agents": result["agents"]}, indent=2, sort_keys=True) + "\n", @@ -903,12 +1097,16 @@ def main(argv: list[str] | None = None) -> int: zdr_endpoints=zdr_endpoints, require_zdr=args.require_zdr, pool="auto", + guarantee_domain_coverage=True, ) 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"], + normalized_rows, + provider_account=provider_account, + outage_domain=_outage_domain, ) fallback_result["report"]["primary_selected_count"] = primary_report[ "selected_count" diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 53e66cfa36..331068ff58 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -15,9 +15,10 @@ import math import re import sys -from collections import Counter +from collections import Counter, deque from pathlib import Path from typing import Any, Iterable, Mapping +from urllib.parse import urlsplit, urlunsplit from scripts.ci.zdr_policy import ( PROVIDER_AUTH_SCHEMES, @@ -32,6 +33,8 @@ DEFAULT_CATALOG_LIMIT = 12 DEFAULT_ACCOUNT_CAP = 4 +_DEFAULT_PORTS: Mapping[str, int] = {"http": 80, "https": 443} + FREE_POOL_CREDENTIAL_NAMES = frozenset( { "BYTEZ_API_KEY", @@ -67,6 +70,292 @@ def provider_account(provider_name: str) -> str: return provider_name +def _outage_domain(row: Mapping[str, Any]) -> str: + """Return the shared-infrastructure outage domain for a normalized row. + + This is a deliberately *different* axis from :func:`provider_account`. + ``provider_account`` answers "is this a distinct credential that may be + entitled to a distinct model catalog" (yes, for ``nvidia_nim`` vs. + ``nvidia_nim_sub`` -- see PR #941/#945 in ``contextual-orchestrator`` and + this repo's own matching fix, both of which correctly stopped assuming + those two independent NVIDIA NIM API keys share a catalog). This + function instead answers "would one physical upstream outage take both + of these routes down together" -- and for those same two credentials the + answer is yes: both resolve to the identical ``base_url``, + ``https://integrate.api.nvidia.com/v1`` (see ``PROVIDER_BASE_URLS`` in + ``scripts/ci/zdr_policy.py``, and that table's own ``nvidia_nim_sub`` + ZDR-scope note: "the same integrate.api.nvidia.com trial API"). + Conflating these two axes -- treating "independent credential" as + "independent outage domain" -- would let two same-endpoint credentials + jointly report full diversity and jointly fill an admission cap meant to + protect against exactly one endpoint's outage, silently recreating the + 2026-08-30 ``orchestrator/free`` exhaustion incident this cap exists to + prevent (see ``docs/product-technical-gap-baseline.md``), just on a + different axis than the one #941/#945/#1468 already fixed. + + Grouped by each row's own ``base_url`` evidence (already present on + every row ``parse_discovery_report``/the sidecar's live discovery + produces) rather than a second hand-maintained provider-name table, so + this cannot silently go stale independently of the ``base_url`` evidence + the catalog itself already serves from -- the same failure mode that + made the removed ``PROVIDER_FAMILIES`` mapping wrong in the first place. + + Compares :func:`_normalize_base_url`'s normalized form, not the raw + string: two spellings of the identical endpoint (a hostname cased + differently, an explicit default port, a trailing slash on one row but + not another) must not be read as two outage domains, or a pure + formatting accident could reintroduce exactly the diversity-overstating, + cap-bypassing bug this function exists to fix. Every KV-credentialed + provider in this codebase today resolves ``base_url`` from one of a + fixed set of hardcoded string literals (never a live, potentially + differently-formatted network response), so this normalization changes + nothing for any input this repository's sidecar currently produces -- + it exists to keep this public, independently invocable function (also + reachable through this script's own ``--discovery-report`` CLI, not only + the sidecar's exact generation path) correct for any future input, not + to compensate for an observed live discrepancy. + """ + return _normalize_base_url(str(row["base_url"])) + + +def _normalize_base_url(base_url: str) -> str: + """Return a case/port/trailing-slash-normalized identity for a base URL. + + Scheme and host are lowercased (both are case-insensitive per RFC 3986 + 3.1/3.2.2); an explicit port equal to the scheme's default (``:443`` for + ``https``, ``:80`` for ``http``) is dropped, since it is equivalent to + omitting it; exactly one trailing slash is stripped from the path, since + a base URL's trailing slash does not change which resource it addresses. + Every other distinction -- a different host, a different non-default + port, a different path, or a different query string -- is preserved + verbatim (routing evidence has no legitimate reason to carry a query + string; preserving rather than dropping it means an unexpected one + cannot silently vanish from the computed identity). The URL fragment is + the one deliberate exception: it is stripped, not preserved, because a + fragment is a client-side-only artifact that is never transmitted to + the server and therefore never identifies a different upstream endpoint + -- two base URLs differing only by fragment must collapse to the same + outage-domain key, sharing one diversity count and one admission-cap + budget, not report inflated diversity or a separately budgeted cap. Any + userinfo component present is dropped rather than preserved: + outage-domain identity is about the physical endpoint, not which + credential reaches it, and this codebase's base URLs never carry + userinfo (see ``configured_gateway_source`` in + ``contextual-orchestrator``, which rejects one outright). + + A string this cannot parse into a scheme, host, and numeric port -- + including an empty string (which would otherwise normalize to a value + indistinct from a real one-character path), a malformed IPv6 host (an + unmatched ``[``/``]`` bracket makes ``urlsplit()`` itself raise + ``ValueError``, before any scheme/host/port is even available to + inspect), and a non-numeric port substring (``urlsplit(...).port`` + raises ``ValueError`` for one, once splitting succeeds) -- falls back to + a simple lowercased, stripped copy of the whole string: grouping only + needs equal inputs to compare equal, not a validated URL, and this + function must never raise on evidence it merely groups. + + Known, deliberate residual gap: hostname canonicalization stops at + lowercasing. A trailing root-label dot (``host.``), an IDN written as + Unicode versus its ASCII/punycode form, or two differently-compressed + but equivalent literal IPv6 addresses (e.g. ``::1`` vs ``0:0:0:0:0:0:0:1``) + are not folded together, so such a pair could still read as two outage + domains. None of these shapes occur in any ``base_url`` this codebase + produces today (every value traces to a fixed set of hardcoded, + already-canonical HTTPS hostnames -- see ``_outage_domain``'s + docstring), so this is intentionally not chased further here; a future + provider whose entitled address genuinely takes one of these forms + should extend this function with evidence of the specific case, not + prophylactically. + """ + text = base_url.strip() + try: + parsed = urlsplit(text) + except ValueError: + return text.casefold() + if not parsed.scheme or not parsed.hostname: + return text.casefold() + try: + port = parsed.port + except ValueError: + return text.casefold() + scheme = parsed.scheme.casefold() + host = parsed.hostname.casefold() + # urlsplit().hostname strips IPv6 literal brackets (``[::1]`` -> ``::1``). + # Re-adding them whenever the host itself contains a colon -- before ever + # conditionally appending a port -- is required for two reasons: without + # it, an explicit-port IPv6 URL (``[::1]:8443``) and a bracketless, + # colon-bearing literal address that merely *looks* like host:port when + # flattened (``[::1:8443]``, port None) collapse to the identical + # ``::1:8443`` string despite being different addresses; and the + # reassembled ``netloc`` must stay valid host:port syntax regardless. + bracketed_host = f"[{host}]" if ":" in host else host + netloc = ( + bracketed_host + if port is None or port == _DEFAULT_PORTS.get(scheme) + else f"{bracketed_host}:{port}" + ) + path = parsed.path.rstrip("/") + return urlunsplit((scheme, netloc, path, parsed.query, "")) + + +def _admission_priority_key( + row: Mapping[str, Any], *, zdr_endpoints: frozenset[str] +) -> tuple[int, int, str, str]: + """Return the deterministic ``(cost tier, ZDR tier, provider, model)`` sort key. + + The single source of truth for admission priority: ``build_zdr_ + prioritized_catalog`` sorts ``eligible_rows`` with this key, and + :func:`_fair_admission_order` re-derives just its first two components + (the tier, excluding the ``(provider, model)`` tie-break) to find tier + boundaries in that same sorted sequence -- sharing one function instead + of two independently written key expressions means the two can never + silently drift out of sync with each other. + """ + return ( + _COST_EVIDENCE_RANK[_cost_evidence(row)], + 0 + if is_zdr_model( + str(row["provider"]), model=str(row["model"]), zdr_endpoints=zdr_endpoints + ) + else 1, + str(row["provider"]), + str(row["model"]), + ) + + +def _fair_admission_order( + rows: list[Mapping[str, Any]], *, zdr_endpoints: frozenset[str] +) -> list[Mapping[str, Any]]: + """Reorder rows so one outage domain's cap fills fairly across accounts. + + ``rows`` must already be sorted by :func:`_admission_priority_key` (see + ``build_zdr_prioritized_catalog``'s own sort, which uses the same key). + Grouping the admission cap by outage domain (:func:`_outage_domain`) + fixed one starvation bug -- two same-endpoint credentials sharing one + budget instead of each getting their own -- but introduced a second, + narrower one: the greedy admission loop consumes rows in sorted order, + so whichever account's rows happen to sort first (``"nvidia_nim"`` + before ``"nvidia_nim_sub"``, alphabetically, in every real fixture in + this file) could exhaust the *entire* shared cap before the domain's + other account was considered at all -- not "prevented from taking more + than its share", but shut out completely, even with rows of its own + available and cap budget nominally unused by it. + + Fairness is reordered strictly *within* one admission-priority tier + (the ``(cost tier, ZDR tier)`` pair -- the first two components of + :func:`_admission_priority_key`), never across tiers: an earlier + revision of this function grouped every row for one outage domain into + a single block at that domain's first appearance in the whole input, + regardless of tier, which could drag a lower-priority route (e.g. paid, + non-ZDR) from one domain ahead of a higher-priority route (e.g. free, + ZDR) belonging to a *different* domain that happened to appear later in + the original order -- a real correctness regression for a catalog whose + entire purpose is admitting free/ZDR routes preferentially. Splitting + ``rows`` into contiguous same-tier runs first (safe because the input + is already tier-sorted, so equal-tier rows are already contiguous) and + reordering fairness independently within each run, then concatenating + the runs back in their original order, makes tier priority strictly + non-negotiable: no row from a worse tier can ever end up ahead of a row + from a better tier, regardless of domain/account composition. + + Within one tier, a domain contributed to by only one account is + returned completely untouched, in its original relative position -- + this function changes nothing for the common case (every provider + except the shared ``nvidia_nim``/``nvidia_nim_sub`` pair, as of this + writing). Within a domain shared by more than one account, rows are + taken in round-robin turns across those accounts -- one row from + account A's own queue (which keeps A's rows in their original relative + order), then one from B's, cycling only over accounts that still have + an unconsumed row -- instead of admission naturally exhausting + whichever account's rows sort first. This guarantees every contending + account gets at least one turn before any account gets a second + admission from that domain, so the domain's cap is filled + proportionally across its accounts rather than by whichever one + happens to rank first within the tier; an account that runs out of rows + before the cap is reached simply stops participating in further + rounds, letting the domain's remaining accounts absorb the leftover + capacity. Crucially, a shared domain's own rows keep the exact global + positions they already occupied among ``rows`` -- round-robin only + decides which of the domain's own rows lands in which of its own + positions, never how far ahead or behind an unrelated domain's row + sits (see :func:`_fair_order_within_tier`'s docstring for the concrete + bug an earlier revision had here: collapsing a shared domain into one + contiguous block at its first appearance silently displaced an + unrelated domain's row that had been priority-ranked between two of + the shared domain's own occurrences). + """ + ordered: list[Mapping[str, Any]] = [] + tier_start = 0 + total = len(rows) + while tier_start < total: + tier = _admission_priority_key(rows[tier_start], zdr_endpoints=zdr_endpoints)[:2] + tier_end = tier_start + 1 + while ( + tier_end < total + and _admission_priority_key(rows[tier_end], zdr_endpoints=zdr_endpoints)[:2] + == tier + ): + tier_end += 1 + ordered.extend(_fair_order_within_tier(rows[tier_start:tier_end])) + tier_start = tier_end + return ordered + + +def _fair_order_within_tier( + rows: list[Mapping[str, Any]], +) -> list[Mapping[str, Any]]: + """Round-robin one already-single-tier run of rows across shared-domain accounts. + + See :func:`_fair_admission_order`'s docstring for why fairness must stay + scoped to one admission-priority tier at a time; this is that per-tier + reordering step, factored out so it never has visibility into rows from + a different tier to (mis)order against. + + This never collapses a domain's rows into one contiguous block. An + earlier revision grouped every row for one domain at that domain's + *first* appearance in ``rows``, which silently moved rows belonging to + *other* domains whenever a shared domain's own rows were not already + contiguous in the input: e.g. ``[A1, B1, A2]`` (domain A shared by two + accounts, with an unrelated domain B's row ranked between A's two + occurrences) became ``[A1, A2, B1]`` under that revision -- B1, an + independent domain's row that had outranked A2, was pushed behind + *both* of A's rows, which could drop B1 entirely under a tight + admission limit even though it was priority-ranked ahead of A2. + Instead, each domain keeps exactly the global positions its own rows + already occupy (recorded in ``domain_positions`` below); round-robining + a shared domain's accounts only decides which of *that domain's own* + rows fills each of its own positions, so a shared domain's Nth admitted + row can only displace what its own Nth-occurrence priority position + would have displaced, never a different domain's row. + """ + domain_positions: dict[str, list[int]] = {} + for index, row in enumerate(rows): + domain_positions.setdefault(_outage_domain(row), []).append(index) + + ordered: list[Mapping[str, Any]] = list(rows) + for positions in domain_positions.values(): + bucket = [rows[index] for index in positions] + account_order: list[str] = [] + queues: dict[str, deque[Mapping[str, Any]]] = {} + for row in bucket: + account = provider_account(str(row["provider"])) + if account not in queues: + account_order.append(account) + queues[account] = deque() + queues[account].append(row) + if len(account_order) <= 1: + continue + reordered: list[Mapping[str, Any]] = [] + while any(queues[account] for account in account_order): + for account in account_order: + queue = queues[account] + if queue: + reordered.append(queue.popleft()) + for index, row in zip(positions, reordered): + ordered[index] = row + return ordered + + def _normalize_agent_id(candidate: str, provider_name: str) -> str: """Return a two-or-more-word snake_case agent identifier.""" slug = re.sub(r"[^a-zA-Z0-9]+", "_", candidate).strip("_").lower() @@ -233,8 +522,99 @@ def build_zdr_prioritized_catalog( zdr_endpoints: frozenset[str] = frozenset(), require_zdr: bool = False, pool: str = "free", + guarantee_domain_coverage: bool = False, ) -> dict[str, Any]: - """Select a free-first, ZDR-aware, credential-account-diverse catalog. + """Select a free-first, ZDR-aware, outage-domain-diverse catalog. + + The returned report carries two distinct diversity/admission signals, + deliberately kept separate (see :func:`_outage_domain`'s docstring for + the full rationale): + + - ``free_account_diversity`` counts the distinct credential accounts + (:func:`provider_account`) among *all* discovered free routes. Vendor + identity is not model equivalence -- ``nvidia_nim`` and + ``nvidia_nim_sub`` are independent here, since either may be entitled + to a different model catalog; only an explicit contextual-orchestrator + ``model_group`` may share routing evidence across routes. + - ``free_outage_domain_diversity`` counts the distinct shared- + infrastructure outage domains (:func:`_outage_domain`, keyed on each + row's own ``base_url``) among the same routes. ``nvidia_nim`` and + ``nvidia_nim_sub`` collapse to *one* domain here, since both resolve to + the identical upstream endpoint -- a caller deciding whether it is + safe to rely on a strict, fail-closed ``orchestrator/free`` pool + without an ``orchestrator/auto`` paid-route safety net (the actual + question ADR-0003 raised) should require at least two here, not on + ``free_account_diversity``: one shared endpoint's outage can empty the + free catalog even when two independent credentials both point at it. + + Both are computed independent of ``pool`` or the per-domain admission + cap below. The admission cap itself (``account_cap`` -- the name + predates this fix and is kept for CLI/environment stability, but its + grouping is by outage domain, matching the cap's original purpose: + preventing one physical endpoint from absorbing the bounded catalog, the + confirmed root cause of a real 2026-08-30 ``orchestrator/free`` + exhaustion incident recorded in ``docs/product-technical-gap- + baseline.md``) admits at most ``account_cap`` rows per outage domain, + not per credential -- two same-endpoint credentials share one cap + budget, they do not each get their own. + + This counts routes discovery reports as free, not routes runtime + preflight has confirmed are actually serving requests: a + ``free_outage_domain_diversity`` of two or more is evidence that one + endpoint's outage cannot immediately empty the free catalog, not proof + that either domain is presently reachable. A caller needing readiness, + not just discovery-time diversity, must combine this with the runtime + preflight report the sidecar already produces. + + ``guarantee_domain_coverage`` (default ``False``, preserving every + existing caller's behavior unchanged) fixes a narrower gap a uniform + ``account_cap`` cannot: when ``limit`` is small relative to the number + of competing outage domains -- the review sidecar's priced-fallback + stage's own real shape, where ``limit`` and ``account_cap`` can both be + 4 -- a single scalar cap forces an uncomfortable choice between two + failure modes. A cap left at ``account_cap`` lets one dominant domain + exhaust ``limit`` before a second domain is ever considered (Devin + Review: "fallback remains single-domain"). Shrinking the cap to + ``limit // domain_count`` fixes that but wastes admittable capacity + whenever ``limit`` does not divide evenly (Devin Review, same PR: + "fallback quota wastes probe slots" -- concretely, ``limit=4`` across 3 + domains admits only 3 routes under a uniform floor of 1, even though a + 4th eligible row exists in one of those domains). When set, admission + runs in two passes instead of one: the first pass admits at most one + row per outage domain (bounded by ``account_cap`` and ``limit``, in the + same priority order the single-pass loop already uses), guaranteeing + every domain with an eligible row is represented before any domain + claims a second seat; the second pass then fills any remaining + ``limit`` budget from the rows the first pass did not pick, still + respecting each domain's ``account_cap`` ceiling (inclusive of what the + first pass already gave it), from whichever domain's next-highest- + priority row comes first -- so the full budget is used whenever enough + eligible rows exist anywhere, not artificially left idle. The picked + order places every first-pass (diversity) row ahead of every + second-pass (fill) row: for a fallback pool whose entire purpose is + outage-domain resilience, trying one candidate from each domain before + a second candidate from an already-represented domain is the more + useful preflight order, not merely a side effect of the two-pass + implementation. + + Both passes run strictly *within* one admission-priority tier at a + time, in tier order (Devin Review: "domain coverage defeats ZDR + priority") -- the same tier-boundary discipline :func:`_fair_admission_order` + already enforces for its own reordering, applied here too, because + ``ordered_rows`` mixes every tier when this flag is used at a catalog's + primary (not fallback-only) stage. Without it, a worse-tier row could + win a first-pass "guaranteed representation" seat for its domain ahead + of a better-tier row from an already-represented domain -- e.g. two + free/ZDR rows in domain A and one free/non-ZDR row in domain B with + ``limit=2`` would wrongly admit one row from each domain instead of + both of A's free/ZDR rows. Processing tier-by-tier (finishing a tier's + own first *and* second pass before ever looking at the next, worse + tier) makes tier priority non-negotiable here exactly as it already is + in :func:`_fair_admission_order`, while ``covered_domains`` and + ``per_domain`` still accumulate across tiers: a domain already given a + seat in a better tier does not claim a second guaranteed seat in a + worse one, and ``account_cap`` bounds a domain's total admissions + across the whole catalog, not per tier. ``orchestrator/free`` first applies a source-identity invariant: only rows whose credential source is in :data:`FREE_POOL_CREDENTIAL_NAMES` are free @@ -268,30 +648,62 @@ def build_zdr_prioritized_catalog( ) ] eligible_rows.sort( - key=lambda row: ( - _COST_EVIDENCE_RANK[_cost_evidence(row)], - 0 - if is_zdr_model( - str(row["provider"]), - model=str(row["model"]), - zdr_endpoints=zdr_endpoints, - ) - else 1, - str(row["provider"]), - str(row["model"]), - ) + key=lambda row: _admission_priority_key(row, zdr_endpoints=zdr_endpoints) ) - per_account: Counter[str] = Counter() + per_domain: Counter[str] = Counter() picked: list[Mapping[str, Any]] = [] - for row in eligible_rows: - account = provider_account(str(row["provider"])) - if per_account[account] >= account_cap: - continue - per_account[account] += 1 - picked.append(row) - if len(picked) >= limit: - break + ordered_rows = _fair_admission_order(eligible_rows, zdr_endpoints=zdr_endpoints) + if guarantee_domain_coverage: + covered_domains: set[str] = set() + tier_start = 0 + total = len(ordered_rows) + while tier_start < total and len(picked) < limit: + tier = _admission_priority_key( + ordered_rows[tier_start], zdr_endpoints=zdr_endpoints + )[:2] + tier_end = tier_start + 1 + while ( + tier_end < total + and _admission_priority_key( + ordered_rows[tier_end], zdr_endpoints=zdr_endpoints + )[:2] + == tier + ): + tier_end += 1 + tier_rows = ordered_rows[tier_start:tier_end] + tier_start = tier_end + + first_pass_ids: set[int] = set() + for row in tier_rows: + if len(picked) >= limit: + break + domain = _outage_domain(row) + if domain in covered_domains or per_domain[domain] >= account_cap: + continue + covered_domains.add(domain) + per_domain[domain] += 1 + picked.append(row) + first_pass_ids.add(id(row)) + for row in tier_rows: + if len(picked) >= limit: + break + if id(row) in first_pass_ids: + continue + domain = _outage_domain(row) + if per_domain[domain] >= account_cap: + continue + per_domain[domain] += 1 + picked.append(row) + else: + for row in ordered_rows: + domain = _outage_domain(row) + if per_domain[domain] >= account_cap: + continue + per_domain[domain] += 1 + picked.append(row) + if len(picked) >= limit: + break if not picked: route_kind = "attested ZDR" if require_zdr else pool @@ -338,6 +750,9 @@ def build_zdr_prioritized_catalog( free_account_diversity = len( {provider_account(str(row["provider"])) for row in all_free_rows} ) + free_outage_domain_diversity = len( + {_outage_domain(row) for row in all_free_rows} + ) free_pool_account_diversity = len( {provider_account(str(row["provider"])) for row in free_pool_rows} ) @@ -355,6 +770,7 @@ def build_zdr_prioritized_catalog( "free_pool_account_diversity": free_pool_account_diversity, "total_priced_routes": len(all_priced_rows), "total_unknown_routes": len(all_unknown_rows), + "free_outage_domain_diversity": free_outage_domain_diversity, "zdr_required": require_zdr, "selected_count": len(catalog_rows), "free_selected_count": selected_evidence.count(COST_FREE), diff --git a/tests/test_contextual_orchestrator_free_pool_enrichment.py b/tests/test_contextual_orchestrator_free_pool_enrichment.py index d28dbbc98d..9066d0e56c 100644 --- a/tests/test_contextual_orchestrator_free_pool_enrichment.py +++ b/tests/test_contextual_orchestrator_free_pool_enrichment.py @@ -32,11 +32,13 @@ def test_discovery_enrichment_recomputes_free_pool_counts_from_full_rows() -> No stage_report, rows, provider_account=lambda provider: provider, + outage_domain=lambda row: row["provider"], ) assert enriched["total_routes"] == 3 assert enriched["total_free_routes"] == 2 assert enriched["free_account_diversity"] == 2 + assert enriched["free_outage_domain_diversity"] == 2 assert enriched["free_pool_admitted_routes"] == 1 assert enriched["free_pool_excluded_source_count"] == 1 assert enriched["free_pool_account_diversity"] == 1 diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 4cda949897..a611f8d201 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -75,6 +75,361 @@ def test_provider_account_keeps_nvidia_keys_independent() -> None: assert policy.provider_account("openai") == "openai" +def test_outage_domain_groups_by_shared_base_url() -> None: + """Outage domain is keyed on a row's own base_url, not its provider name.""" + assert policy._outage_domain( + {"base_url": "https://integrate.api.nvidia.com/v1"} + ) == policy._outage_domain({"base_url": "https://integrate.api.nvidia.com/v1"}) + assert policy._outage_domain( + {"base_url": "https://api.openai.com/v1"} + ) != policy._outage_domain({"base_url": "https://integrate.api.nvidia.com/v1"}) + + +@pytest.mark.parametrize( + ("base_url", "equivalent_to"), + [ + ("HTTPS://Integrate.API.Nvidia.COM/v1", "https://integrate.api.nvidia.com/v1"), + ("https://integrate.api.nvidia.com:443/v1", "https://integrate.api.nvidia.com/v1"), + ("https://integrate.api.nvidia.com/v1/", "https://integrate.api.nvidia.com/v1"), + ("https://integrate.api.nvidia.com/v1//", "https://integrate.api.nvidia.com/v1"), + ("https://integrate.api.nvidia.com/v1#fragment", "https://integrate.api.nvidia.com/v1"), + ( + "https://integrate.api.nvidia.com/v1#fragment-a", + "https://integrate.api.nvidia.com/v1#fragment-b", + ), + ], +) +def test_normalize_base_url_treats_equivalent_spellings_as_one_domain( + base_url: str, equivalent_to: str +) -> None: + """Case, an explicit default port, a trailing slash, or a fragment don't split a domain. + + Regression for a Devin Review finding on this fix: comparing raw + ``base_url`` strings would let a hostname-case difference, an explicit + ``:443``, or a trailing slash split one physical endpoint into two + outage domains by formatting accident alone -- silently reintroducing + the diversity-overstating, cap-bypassing bug this module exists to fix, + for exactly the ``nvidia_nim``/``nvidia_nim_sub`` pair it was written to + protect. + + The two fragment cases are a second, later Devin Review finding: a URL + fragment is client-side only and never reaches the server, so it cannot + identify a different upstream endpoint -- two base URLs differing only + by fragment (including one with no fragment at all against one that has + one) must still normalize to the identical outage-domain key. + """ + assert policy._normalize_base_url(base_url) == policy._normalize_base_url(equivalent_to) + + +@pytest.mark.parametrize( + ("base_url", "distinct_from"), + [ + ("https://integrate.api.nvidia.com/v1", "https://api.openai.com/v1"), + ("https://integrate.api.nvidia.com:8443/v1", "https://integrate.api.nvidia.com/v1"), + ("https://integrate.api.nvidia.com/v2", "https://integrate.api.nvidia.com/v1"), + ("http://integrate.api.nvidia.com/v1", "https://integrate.api.nvidia.com/v1"), + ("https://integrate.api.nvidia.com/v1?tenant=a", "https://integrate.api.nvidia.com/v1"), + ], +) +def test_normalize_base_url_preserves_genuine_distinctions( + base_url: str, distinct_from: str +) -> None: + """A different host, non-default port, path, scheme, or query stays a different domain. + + The query-string case guards the fragment fix's scope: only the + fragment is dropped, the query string stays a real distinguishing + component (see :func:`_normalize_base_url`'s docstring). + """ + assert policy._normalize_base_url(base_url) != policy._normalize_base_url(distinct_from) + + +def test_normalize_base_url_falls_back_on_unparseable_input() -> None: + """A hostless or malformed-port URL groups by a stripped, lowercased copy. + + Never raises: this function only needs equal inputs to compare equal, + not a validated URL, since it groups audit evidence, not user input that + must be rejected. + """ + assert policy._normalize_base_url("") == policy._normalize_base_url("") + assert policy._normalize_base_url(" NOT-A-URL ") == policy._normalize_base_url("not-a-url") + assert policy._normalize_base_url( + "https://host:notaport/v1" + ) == policy._normalize_base_url("HTTPS://HOST:NOTAPORT/v1") + + +def test_normalize_base_url_falls_back_on_malformed_ipv6_bracket() -> None: + """An unmatched IPv6 bracket cannot raise past this function. + + Regression for a Devin Review finding: ``urlsplit()`` itself raises + ``ValueError`` for an unmatched ``[``/``]`` (e.g. ``https://[::1/v1``, + a missing closing bracket) -- before any scheme/host/port is even + available to inspect, so the earlier fallback (which only wrapped the + ``.port`` property access) did not cover it. + """ + # Would raise ValueError: Invalid IPv6 URL if urlsplit() itself were not + # also wrapped. + assert policy._normalize_base_url("https://[::1/v1") == "https://[::1/v1" + assert policy._normalize_base_url("HTTPS://[::1/V1") == policy._normalize_base_url( + "https://[::1/v1" + ) + + +def test_normalize_base_url_distinguishes_ipv6_port_from_literal_colon_digits() -> None: + """An IPv6 host:port pair and a differently-shaped literal stay distinct. + + Regression for a Devin Review finding: ``urlsplit().hostname`` strips + IPv6 literal brackets (``[::1]`` -> ``::1``), so appending a port + without re-adding them collapsed ``https://[::1]:8443/v1`` (host + ``::1``, port ``8443``) and ``https://[::1:8443]/v1`` (one IPv6 + literal, ``::1:8443``, with no separate port at all) to the identical + ``::1:8443`` string -- two different addresses undercounted as one + outage domain. + """ + explicit_port = policy._normalize_base_url("https://[::1]:8443/v1") + literal_colon_digits = policy._normalize_base_url("https://[::1:8443]/v1") + assert explicit_port != literal_colon_digits + # Both stay valid, bracketed netloc syntax, not the pre-fix bare form. + assert explicit_port == "https://[::1]:8443/v1" + assert literal_colon_digits == "https://[::1:8443]/v1" + + +def test_normalize_base_url_drops_default_port_for_ipv6_host() -> None: + """An explicit default port on an IPv6 host is still dropped, brackets intact.""" + assert policy._normalize_base_url( + "https://[::1]:443/v1" + ) == policy._normalize_base_url("https://[::1]/v1") + + +def test_outage_domain_uses_normalized_base_url() -> None: + """Two rows spelling one endpoint differently share one outage domain.""" + assert policy._outage_domain( + {"base_url": "https://integrate.api.nvidia.com/v1"} + ) == policy._outage_domain({"base_url": "https://Integrate.API.Nvidia.com:443/v1/"}) + + +def _row( + provider: str, model: str, *, cost_evidence: str = policy.COST_UNKNOWN +) -> dict[str, object]: + """Return a minimal normalized-shaped row for ``_fair_admission_order`` tests.""" + return { + "provider": provider, + "model": model, + "base_url": policy.PROVIDER_BASE_URLS[provider], + "cost_evidence": cost_evidence, + } + + +def test_fair_admission_order_untouched_for_single_account_domains() -> None: + """A domain with only one contributing account keeps its original order.""" + rows = [_row("openrouter", "a"), _row("openai", "b"), _row("bytez", "c")] + assert policy._fair_admission_order(rows, zdr_endpoints=frozenset()) == rows + + +def test_fair_admission_order_round_robins_a_shared_domain() -> None: + """Two accounts sharing a domain alternate instead of one exhausting first. + + Regression for the same Devin Review finding as + ``test_build_catalog_shared_domain_cap_does_not_starve_second_account``, + exercised directly against the reordering helper: unit-level coverage of + exactly which row is emitted in which position, not just the resulting + admission counts. + """ + rows = [ + _row("nvidia_nim", "m0"), + _row("nvidia_nim", "m1"), + _row("nvidia_nim", "m2"), + _row("nvidia_nim_sub", "s0"), + _row("nvidia_nim_sub", "s1"), + ] + ordered = policy._fair_admission_order(rows, zdr_endpoints=frozenset()) + assert [(row["provider"], row["model"]) for row in ordered] == [ + ("nvidia_nim", "m0"), + ("nvidia_nim_sub", "s0"), + ("nvidia_nim", "m1"), + ("nvidia_nim_sub", "s1"), + ("nvidia_nim", "m2"), + ] + + +def test_fair_admission_order_never_moves_a_row_across_priority_tiers() -> None: + """Fairness reordering never lets a worse-tier row outrank a better-tier one. + + Regression for a real correctness bug a Devin Review finding caught: an + earlier revision of ``_fair_admission_order`` grouped every row for one + outage domain into a single block at that domain's first appearance, + *regardless of tier* -- so a lower-priority row (here, priced OpenAI) + sharing a domain with a higher-priority row (free OpenAI) could get + dragged ahead of a higher-priority row from a *different* domain (free + OpenRouter) that happened to sort later only because of the + ``(provider, model)`` tie-break. Concretely: sorted input + ``[free OpenAI, free OpenRouter, priced OpenAI]`` must stay in that + exact order -- the free OpenRouter row must never be pushed behind the + priced OpenAI row merely because OpenAI's two rows share a domain. + """ + rows = [ + _row("openai", "free-model", cost_evidence=policy.COST_FREE), + _row("openrouter", "free-model", cost_evidence=policy.COST_FREE), + _row("openai", "priced-model", cost_evidence=policy.COST_PRICED), + ] + ordered = policy._fair_admission_order(rows, zdr_endpoints=frozenset()) + assert [(row["provider"], row["model"]) for row in ordered] == [ + ("openai", "free-model"), + ("openrouter", "free-model"), + ("openai", "priced-model"), + ] + + +def test_build_catalog_never_admits_a_priced_route_over_a_free_one_from_another_domain() -> None: + """End-to-end: a tight limit must never drop a free route for a paid one. + + Same Devin Review finding as ``test_fair_admission_order_never_moves_a_ + row_across_priority_tiers``, exercised through the full public API + rather than the internal reordering helper directly. + """ + report = { + "models": [ + { + "provider": "openai", + "model": "free-model", + "agent_id": "oa_free", + "is_free": True, + **FREE_PRICE, + }, + { + "provider": "openai", + "model": "priced-model", + "agent_id": "oa_priced", + "is_free": False, + "prompt_price_per_1k": 0.002, + "completion_price_per_1k": 0.008, + "currency_code": "USD", + }, + { + "provider": "openrouter", + "model": "free-model", + "agent_id": "or_free", + "is_free": True, + **FREE_PRICE, + }, + ] + } + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(report), limit=2, account_cap=4, pool="auto" + ) + assert [agent["model"] for agent in result["agents"]] == ["free-model", "free-model"] + assert [agent["provider_name"] for agent in result["agents"]] == ["openai", "openrouter"] + + +def test_fair_admission_order_preserves_domain_position_and_multiple_domains() -> None: + """Reordering stays local to each multi-account domain, in its original slot. + + A single-account domain on either side of a multi-account domain stays + exactly where it was, untouched; the multi-account domain's block still + starts where its first row originally appeared, with only its internal + order changed (``nvidia_nim``'s two consecutive rows are pulled apart to + give ``nvidia_nim_sub`` a turn between them, rather than staying + adjacent). + """ + rows = [ + _row("bytez", "b0"), + _row("nvidia_nim", "m0"), + _row("nvidia_nim", "m1"), + _row("nvidia_nim_sub", "s0"), + _row("openrouter", "r0"), + ] + ordered = policy._fair_admission_order(rows, zdr_endpoints=frozenset()) + assert [(row["provider"], row["model"]) for row in ordered] == [ + ("bytez", "b0"), + ("nvidia_nim", "m0"), + ("nvidia_nim_sub", "s0"), + ("nvidia_nim", "m1"), + ("openrouter", "r0"), + ] + + +def test_fair_admission_order_preserves_non_contiguous_shared_domain_positions() -> None: + """A shared domain's own rows never displace an interleaved independent row. + + Regression for a Devin Review finding: an earlier revision collapsed a + domain to one contiguous block at that domain's *first* appearance, + which was wrong whenever the domain's own rows were not already + contiguous in priority order. Here domain A (shared by two accounts) + contributes ``A1`` and ``A2``, with an unrelated domain B's ``B1`` + priority-ranked between them: ``[A1, B1, A2]``. The old code produced + ``[A1, A2, B1]`` -- B1, which had outranked A2, got pushed behind + *both* of A's rows. The fix must preserve B1's original slot between + A1 and A2. + """ + rows = [ + _row("nvidia_nim", "m0"), + _row("bytez", "b0"), + _row("nvidia_nim_sub", "s0"), + ] + ordered = policy._fair_admission_order(rows, zdr_endpoints=frozenset()) + assert [(row["provider"], row["model"]) for row in ordered] == [ + ("nvidia_nim", "m0"), + ("bytez", "b0"), + ("nvidia_nim_sub", "s0"), + ] + + +def test_build_catalog_does_not_drop_an_interleaved_independent_route_under_a_tight_limit() -> None: + """A tight global limit must not drop an independent-domain route. + + End-to-end regression for the same Devin Review finding as + ``test_fair_admission_order_preserves_non_contiguous_shared_domain_ + positions``, exercised through the full public API with a real, + sort-derived priority order rather than a hand-fed one. + + ``nvidia_nim`` and ``nvidia_nim_sub`` are this codebase's only shared + outage domain (both resolve to ``https://integrate.api.nvidia.com/v1``), + and no third registered provider name sorts alphabetically between them + -- so to reach a genuinely *sort-derived* interleaved order (not just a + hand-fed one) this reuses the ``nvidia_nim`` credential for a second row + with an explicit ``base_url`` override pointing at an unrelated, + independent endpoint. Account identity and outage-domain identity are + deliberately decoupled by this module's own design (see + ``_outage_domain``'s docstring), so one credential's discovery rows + spanning two different base URLs is a legitimate shape, not a + contrivance. Choosing a model name (``"z-indep"``) that sorts after + ``"m0"`` places the independent row's priority rank between the shared + domain's ``nvidia_nim`` and ``nvidia_nim_sub`` rows once + ``build_zdr_prioritized_catalog`` sorts by ``_admission_priority_key``. + """ + report = { + "models": [ + { + "provider": "nvidia_nim", + "model": "m0", + "agent_id": "nim_m0", + "is_free": True, + **FREE_PRICE, + }, + { + "provider": "nvidia_nim", + "model": "z-indep", + "agent_id": "nim_indep", + "is_free": True, + "base_url": "https://independent.example.com/v1", + **FREE_PRICE, + }, + { + "provider": "nvidia_nim_sub", + "model": "s0", + "agent_id": "nimsub_s0", + "is_free": True, + **FREE_PRICE, + }, + ] + } + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(report), limit=2, account_cap=4 + ) + admitted = {(agent["provider_name"], agent["model"]) for agent in result["agents"]} + assert ("nvidia_nim", "z-indep") in admitted + assert len(result["agents"]) == 2 + + @pytest.mark.parametrize( ("candidate", "provider", "expected"), [ @@ -275,7 +630,15 @@ def test_build_auto_catalog_keeps_private_targets_zdr_only() -> None: def test_build_catalog_reports_free_account_diversity() -> None: - """Diversity counts independently credentialed accounts with free routes.""" + """Diversity counts independently credentialed accounts with free routes. + + ``free_outage_domain_diversity`` is one lower than ``free_account_ + diversity`` here: ``nvidia_nim`` and ``nvidia_nim_sub`` are two + independent accounts (see ``test_build_catalog_counts_same_vendor_ + credentials_independently``) but share one physical upstream endpoint, + so they collapse to a single outage domain while the other three + providers (openrouter, openai, bytez) each keep their own. + """ result = policy.build_zdr_prioritized_catalog( policy.parse_discovery_report(_report()), limit=12, @@ -283,10 +646,24 @@ def test_build_catalog_reports_free_account_diversity() -> None: zdr_endpoints=ZDR_FEED, ) assert result["report"]["free_account_diversity"] == 5 + assert result["report"]["free_outage_domain_diversity"] == 4 def test_build_catalog_counts_same_vendor_credentials_independently() -> None: - """Same-vendor credentials remain distinct discovery accounts.""" + """Same-vendor credentials remain distinct discovery accounts. + + But they are *not* automatically distinct outage domains: + ``free_outage_domain_diversity`` reports 1 here, not 2, because both + rows' ``base_url`` (via ``PROVIDER_BASE_URLS``) resolve to the identical + ``https://integrate.api.nvidia.com/v1`` upstream. Regression for a real, + separate bug found by review during this session: #941/#945/#1468 + correctly stopped assuming these two credentials share a *model + catalog*, but a caller deciding whether a single physical outage could + empty the free catalog (e.g. open PR #1437's Strix ``orchestrator/free`` + eligibility gate) needs the outage-domain count, not the account count + -- conflating the two would let this exact pair report a falsely safe + diversity of 2 for that specific decision. + """ single_family_report = { "models": [ { @@ -311,6 +688,92 @@ def test_build_catalog_counts_same_vendor_credentials_independently() -> None: account_cap=4, ) assert result["report"]["free_account_diversity"] == 2 + assert result["report"]["free_outage_domain_diversity"] == 1 + + +def test_build_catalog_collapses_differently_spelled_equivalent_endpoints() -> None: + """A hostname-case/port/slash spelling difference cannot split one domain. + + End-to-end regression for the same Devin Review finding as + ``test_normalize_base_url_treats_equivalent_spellings_as_one_domain``, + exercised through ``parse_discovery_report``'s ``base_url`` override + (the field a discovery report -- including this script's own + ``--discovery-report`` CLI input, not only the sidecar's exact + generation path -- may supply explicitly) rather than the unit-level + helper directly. + """ + differently_spelled_report = { + "models": [ + { + "provider": "nvidia_nim", + "model": "nvidia/nemotron-3-nano-30b-a3b", + "agent_id": "nim_nano_free", + "is_free": True, + "base_url": "https://integrate.api.nvidia.com/v1", + **FREE_PRICE, + }, + { + "provider": "nvidia_nim_sub", + "model": "meta/llama-3.3-70b-instruct", + "agent_id": "nimsec_70b", + "is_free": True, + "base_url": "HTTPS://Integrate.API.Nvidia.com:443/v1/", + **FREE_PRICE, + }, + ] + } + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(differently_spelled_report), + limit=12, + account_cap=1, + ) + assert result["report"]["free_outage_domain_diversity"] == 1 + # The shared domain's cap of 1 admits only the first-sorted row, not one + # from each differently-spelled row. + assert len(result["agents"]) == 1 + + +def test_build_catalog_collapses_fragment_only_difference() -> None: + """A fragment-only spelling difference cannot split one domain. + + End-to-end regression for a Devin Review finding: a URL fragment is + client-side only and is never sent to the server, so it cannot + legitimately identify a different upstream endpoint. Two base URLs + differing only by fragment must still share one + ``free_outage_domain_diversity`` count and one admission-cap budget, + exercised through ``parse_discovery_report``'s ``base_url`` override + the same way as + ``test_build_catalog_collapses_differently_spelled_equivalent_endpoints``. + """ + fragment_only_report = { + "models": [ + { + "provider": "nvidia_nim", + "model": "nvidia/nemotron-3-nano-30b-a3b", + "agent_id": "nim_nano_free", + "is_free": True, + "base_url": "https://integrate.api.nvidia.com/v1#primary", + **FREE_PRICE, + }, + { + "provider": "nvidia_nim_sub", + "model": "meta/llama-3.3-70b-instruct", + "agent_id": "nimsec_70b", + "is_free": True, + "base_url": "https://integrate.api.nvidia.com/v1#secondary", + **FREE_PRICE, + }, + ] + } + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(fragment_only_report), + limit=12, + account_cap=1, + ) + assert result["report"]["free_outage_domain_diversity"] == 1 + # The shared domain's cap of 1 admits only the first-sorted row, not one + # from each differently-fragmented row. + assert len(result["agents"]) == 1 def test_build_catalog_rejects_unknown_pool() -> None: @@ -337,7 +800,21 @@ def test_build_catalog_assigns_unique_priorities() -> None: def test_build_catalog_applies_account_cap() -> None: - """An account cap keeps one credential from absorbing the pool.""" + """The admission cap is enforced per outage domain, split fairly within it. + + ``nvidia_nim`` and ``nvidia_nim_sub`` share one outage domain (both + ``https://integrate.api.nvidia.com/v1``), so they share one ``2``-slot + cap budget here rather than each getting their own -- with ``account_cap`` + still named for the credential-account concept it started as, but its + grouping fixed to outage domains (see ``test_build_catalog_prevents_ + shared_endpoint_from_crowding_out_independent_providers`` for the + concrete crowding-out scenario this exists to prevent). The shared + budget is split round-robin across the domain's accounts (see + ``test_build_catalog_shared_domain_cap_does_not_starve_second_account``), + not consumed entirely by whichever one sorts first: one slot each for + ``nvidia_nim``/``nvidia_nim_sub`` here, not two for one and zero for the + other. + """ report = { "models": [ {"provider": "nvidia_nim", "model": f"m{i}", "agent_id": f"nim_a{i}", "is_free": True, **FREE_PRICE} @@ -365,9 +842,309 @@ def test_build_catalog_applies_account_cap() -> None: 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": 1, "nvidia_nim_sub": 1, "openrouter": 2} + assert len(result["agents"]) == 4 + + +def test_build_catalog_prevents_shared_endpoint_from_crowding_out_independent_providers() -> None: + """A shared-endpoint credential pair cannot out-compete independent providers. + + Regression for a real, still-open gap this session's own review found in + the already-merged #1468 fix: #1468 correctly stopped treating + ``nvidia_nim``/``nvidia_nim_sub`` as one *model-catalog* family, but in + doing so also let the admission cap treat them as two fully independent + *accounts* -- meaning the two credentials could jointly consume up to + ``2 * account_cap`` catalog slots, all from one physical endpoint, + crowding out a genuinely independent provider (``openrouter`` here) even + though it has its own free routes available. With the cap correctly + grouped by outage domain instead, the two NVIDIA credentials share one + domain's cap budget and cannot jointly exceed it. + """ + report = { + "models": [ + {"provider": "bytez", "model": f"b{i}", "agent_id": f"bytez_{i}", "is_free": True, **FREE_PRICE} + for i in range(2) + ] + + [ + {"provider": "nvidia_nim", "model": f"n{i}", "agent_id": f"nim_{i}", "is_free": True, **FREE_PRICE} + for i in range(10) + ] + + [ + {"provider": "nvidia_nim_sub", "model": f"n{i}", "agent_id": f"nimsub_{i}", "is_free": True, **FREE_PRICE} + for i in range(10) + ] + + [ + {"provider": "openrouter", "model": f"r{i}", "agent_id": f"or_{i}", "is_free": True, **FREE_PRICE} + for i in range(2) + ] + } + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(report), limit=20, account_cap=4 + ) + counts: dict[str, int] = {} + for agent in result["agents"]: + counts[agent["provider_name"]] = counts.get(agent["provider_name"], 0) + 1 + # NVIDIA's shared domain admits at most 4 total, split fairly (2 from + # each credential, not 4 from whichever sorts first and 0 from the + # other) -- leaving bytez and openrouter, each an independent domain, + # fully admitted. + assert counts == {"bytez": 2, "nvidia_nim": 2, "nvidia_nim_sub": 2, "openrouter": 2} + assert result["report"]["free_account_diversity"] == 4 + assert result["report"]["free_outage_domain_diversity"] == 3 + + +def test_build_catalog_shared_domain_cap_does_not_starve_second_account() -> None: + """A shared domain's cap admits from every contending account, not just one. + + Regression for a Devin Review finding on this fix: the admission loop + walks rows in strict sorted (cost-tier, ZDR, provider, model) order, so + grouping the cap by outage domain alone was not enough -- whichever + account's rows happened to sort first (``nvidia_nim`` before + ``nvidia_nim_sub`` in every fixture here) could exhaust the *entire* + shared cap before the domain's other account was considered at all, a + narrower but just-as-real version of the crowding-out bug this file + already fixes across domains. With both credentials offering far more + rows than the shared cap, both must still contribute. + """ + report = { + "models": [ + {"provider": "nvidia_nim", "model": f"n{i}", "agent_id": f"nim_{i}", "is_free": True, **FREE_PRICE} + for i in range(8) + ] + + [ + {"provider": "nvidia_nim_sub", "model": f"n{i}", "agent_id": f"nimsub_{i}", "is_free": True, **FREE_PRICE} + for i in range(8) + ] + } + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(report), limit=20, account_cap=4 + ) + counts: dict[str, int] = {} + for agent in result["agents"]: + counts[agent["provider_name"]] = counts.get(agent["provider_name"], 0) + 1 + assert counts == {"nvidia_nim": 2, "nvidia_nim_sub": 2} + assert sum(counts.values()) == 4 + + +PRICED_PRICE = { + "prompt_price_per_1k": 0.01, + "completion_price_per_1k": 0.01, + "currency_code": "USD", +} + + +def test_build_catalog_guarantee_domain_coverage_leaves_single_domain_unchanged() -> None: + """A single competing domain behaves exactly as the unmodified admission loop did.""" + report = { + "models": [ + {"provider": "openrouter", "model": f"r{i}", "agent_id": f"or_{i}", "is_free": False, **PRICED_PRICE} + for i in range(6) + ] + } + rows = policy.parse_discovery_report(report) + without_flag = policy.build_zdr_prioritized_catalog( + rows, limit=4, account_cap=4, pool="auto" + ) + with_flag = policy.build_zdr_prioritized_catalog( + rows, limit=4, account_cap=4, pool="auto", guarantee_domain_coverage=True + ) + assert len(without_flag["agents"]) == len(with_flag["agents"]) == 4 + + +def test_build_catalog_guarantee_domain_coverage_fixes_single_domain_starvation() -> None: + """Regression for Devin Review's "fallback remains single-domain" finding on `.github#1474`. + + With both ``limit`` and ``account_cap`` at 4 (the review sidecar's real + priced-fallback shape), an outage domain with at least ``limit`` priced + rows used to exhaust the whole stage before a second, genuinely + independent domain's row was ever considered -- the per-domain cap + provided no diversity protection for this specific stage. Four + same-domain priced routes plus one independent priced route (Devin's + own suggested regression shape) must now leave room for the + independent route. + """ + report = { + "models": [ + {"provider": "nvidia_nim", "model": f"n{i}", "agent_id": f"nim_{i}", "is_free": False, **PRICED_PRICE} + for i in range(4) + ] + + [{"provider": "openrouter", "model": "independent", "agent_id": "or_0", "is_free": False, **PRICED_PRICE}] + } + rows = policy.parse_discovery_report(report) + result = policy.build_zdr_prioritized_catalog( + rows, limit=4, account_cap=4, pool="auto", guarantee_domain_coverage=True + ) + providers = {agent["provider_name"] for agent in result["agents"]} + assert providers == {"nvidia_nim", "openrouter"} + + +def test_build_catalog_guarantee_domain_coverage_uses_full_budget_on_uneven_split() -> None: + """Regression for Devin Review's "fallback quota wastes probe slots" finding. + + A uniform ``limit // domain_count`` floor (this fix's own first + revision) correctly guarantees every domain a seat but wastes capacity + whenever ``limit`` does not divide evenly: ``limit=4`` across 3 domains + floors to 1 each, admitting only 3 routes even though a 4th eligible + row exists. Three domains (bytez, openrouter, openai), each with 2 + priced rows, ``limit=4``, ``account_cap=4``: every domain must still be + represented, and the full 4-route budget must be used, not left at 3. + """ + report = { + "models": [ + {"provider": provider, "model": f"{provider}-{i}", "agent_id": f"{provider}_{i}", "is_free": False, **PRICED_PRICE} + for provider in ("bytez", "openrouter", "openai") + for i in range(2) + ] + } + rows = policy.parse_discovery_report(report) + result = policy.build_zdr_prioritized_catalog( + rows, limit=4, account_cap=4, pool="auto", guarantee_domain_coverage=True + ) + providers = {agent["provider_name"] for agent in result["agents"]} + assert providers == {"bytez", "openrouter", "openai"} + assert len(result["agents"]) == 4 + + +def test_build_catalog_guarantee_domain_coverage_uses_full_budget_on_eight_over_three() -> None: + """Devin Review's own second suggested non-divisible split (8 routes, 3 domains). + + Three domains, each with ample priced rows (5 each -- comfortably above + both ``account_cap`` and any per-domain share of ``limit``), ``limit=8``, + ``account_cap=4``: every domain represented, the full 8-route budget + used, and no domain exceeds ``account_cap``. + """ + report = { + "models": [ + {"provider": provider, "model": f"{provider}-{i}", "agent_id": f"{provider}_{i}", "is_free": False, **PRICED_PRICE} + for provider in ("bytez", "openrouter", "openai") + for i in range(5) + ] + } + rows = policy.parse_discovery_report(report) + result = policy.build_zdr_prioritized_catalog( + rows, limit=8, account_cap=4, pool="auto", guarantee_domain_coverage=True + ) + counts: dict[str, int] = {} + for agent in result["agents"]: + counts[agent["provider_name"]] = counts.get(agent["provider_name"], 0) + 1 + assert set(counts) == {"bytez", "openrouter", "openai"} + assert sum(counts.values()) == 8 + assert all(count <= 4 for count in counts.values()) + + +def test_build_catalog_guarantee_domain_coverage_fixes_auto_primary_stage_too() -> None: + """Regression for Devin Review's "auto primary catalog remains single-domain" finding. + + The *primary* ``auto``-pool stage has the identical single-scalar-cap- + equals-limit coincidence the priced-fallback stage already had fixed -- + not the launcher's ``DEFAULT_ACCOUNT_CAP`` (4) it might appear to use + at a glance, but the review sidecar's own real deployed default: the + sidecar script exports ``ORCHESTRATOR_CATALOG_ACCOUNT_CAP=8`` (see + ``contextual_orchestrator_review_sidecar.sh``), and the launcher's + ``REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT`` is also 8 for the ``auto`` + pool's primary stage. Eight free routes from one dominant outage + domain, ``limit=8`` and ``account_cap=8`` (the real deployed values, + not this file's usual ``account_cap=4`` fixtures), used to exclude + every independent free domain entirely. + """ + report = { + "models": [ + {"provider": "nvidia_nim", "model": f"n{i}", "agent_id": f"nim_{i}", "is_free": True, **FREE_PRICE} + for i in range(8) + ] + + [{"provider": "openrouter", "model": "independent", "agent_id": "or_0", "is_free": True, **FREE_PRICE}] + } + rows = policy.parse_discovery_report(report) + result = policy.build_zdr_prioritized_catalog( + rows, limit=8, account_cap=8, pool="auto", guarantee_domain_coverage=True + ) + providers = {agent["provider_name"] for agent in result["agents"]} + assert providers == {"nvidia_nim", "openrouter"} + assert len(result["agents"]) == 8 + + +def test_build_catalog_guarantee_domain_coverage_still_bounded_by_account_cap() -> None: + """The first-pass diversity guarantee never lets a domain skip its own cap. + + A single domain with far more rows than ``account_cap`` must still stop + at ``account_cap``, exactly as the unmodified admission loop already + guarantees -- ``guarantee_domain_coverage`` only changes *when* other + domains get a turn, never the per-domain ceiling itself. + """ + report = { + "models": [ + {"provider": "openrouter", "model": f"r{i}", "agent_id": f"or_{i}", "is_free": False, **PRICED_PRICE} + for i in range(10) + ] + } + rows = policy.parse_discovery_report(report) + result = policy.build_zdr_prioritized_catalog( + rows, limit=8, account_cap=4, pool="auto", guarantee_domain_coverage=True + ) + assert len(result["agents"]) == 4 + + +def test_build_catalog_guarantee_domain_coverage_caps_at_limit_when_domains_outnumber_it() -> None: + """More competing domains than ``limit`` still stops exactly at ``limit``. + + Five independent single-account domains only exist in this fixture set + via distinct providers, but this codebase registers only five providers + total (see ``PROVIDER_BASE_URLS``); ``nvidia_nim``/``nvidia_nim_sub`` + share one domain, so the maximum distinct domains available is four. + With ``limit=3`` and four competing domains, full domain coverage is + structurally impossible -- the first admission pass itself must stop at + ``limit`` before every domain gets a turn, exercising that pass's own + ``len(picked) >= limit`` bound (never reached by the other + ``guarantee_domain_coverage`` tests, which all keep ``limit >= + domain_count``). Exactly ``limit`` routes are admitted, each from a + different domain. + """ + report = { + "models": [ + {"provider": provider, "model": f"{provider}-0", "agent_id": f"{provider}_0", "is_free": False, **PRICED_PRICE} + for provider in ("bytez", "nvidia_nim", "openrouter", "openai") + ] + } + rows = policy.parse_discovery_report(report) + result = policy.build_zdr_prioritized_catalog( + rows, limit=3, account_cap=4, pool="auto", guarantee_domain_coverage=True + ) + assert len(result["agents"]) == 3 + providers = {agent["provider_name"] for agent in result["agents"]} + assert len(providers) == 3 + + +def test_build_catalog_guarantee_domain_coverage_never_crosses_a_tier_boundary() -> None: + """Regression for Devin Review's "domain coverage defeats ZDR priority" finding. + + Two free/ZDR routes in one domain (openrouter) and one free/non-ZDR + route in an independent domain (bytez), ``limit=2``: the unguarded + first pass used to admit one row from *each* domain (one guaranteed + seat apiece) before the domain already represented ever got a second + look, wrongly seating the worse-tier bytez row ahead of openrouter's + second free/ZDR row. Both admitted rows must stay free/ZDR. + """ + zdr_endpoints = frozenset({"openrouter/zdr-a", "openrouter/zdr-b"}) + report = { + "models": [ + {"provider": "openrouter", "model": "zdr-a", "agent_id": "or_zdr_a", "is_free": True, **FREE_PRICE}, + {"provider": "openrouter", "model": "zdr-b", "agent_id": "or_zdr_b", "is_free": True, **FREE_PRICE}, + {"provider": "bytez", "model": "not-zdr", "agent_id": "bytez_not_zdr", "is_free": True, **FREE_PRICE}, + ] + } + rows = policy.parse_discovery_report(report) + result = policy.build_zdr_prioritized_catalog( + rows, + limit=2, + account_cap=4, + pool="auto", + zdr_endpoints=zdr_endpoints, + guarantee_domain_coverage=True, + ) + agents = result["agents"] + assert len(agents) == 2 + assert all(agent["provider_name"] == "openrouter" for agent in agents) + assert all("zdr" in agent["tags"] for agent in agents) def test_build_catalog_respects_limit() -> None: diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 559c2d1e99..4661d5de53 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -83,14 +83,14 @@ def _openai_text(content: str) -> dict[str, object]: def test_routable_discovered_models_excludes_evidence_only_rows() -> None: - """Evidence-only rows (e.g. OpenRouter) must never enter live selection.""" + """Evidence-only rows for a non-OpenRouter provider never enter live selection.""" namespace = _load_launcher() routable = namespace.get("_routable_discovered_models") assert callable(routable), "launcher must expose an evidence-only discovery filter" evidence_only_model = SimpleNamespace( - id="openrouter_evidence_only", - provider_name="openrouter", + id="nvidia_evidence_only", + provider_name="nvidia_nim", model_id="some/model", evidence_only=True, ) @@ -112,6 +112,291 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: assert routable([]) == [] +def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_evidence_only() -> None: + """OpenRouter rows are exempt from evidence_only while every row still shows it. + + Regression for a confirmed bug, fixed upstream at + ``ContextualWisdomLab/contextual-orchestrator#949`` (merged, not yet + pinned in this repo as of this writing -- see + ``ContextualWisdomLab/.github#1477``): ``contextual-orchestrator``'s + OpenRouter ``ProviderModelSource`` currently hardcodes + ``evidence_only=True`` for every discovered model unconditionally (not + computed per model from real evidence) -- so today's real signature is + that *every* discovered OpenRouter row carries ``evidence_only=True``, + with no exceptions, even genuinely servable ones. If this filter + applied to OpenRouter like every other provider while that bug is + live, it would strip every OpenRouter row, including genuinely + servable ones, before ``zdr_policy.is_zdr_model()``'s purpose-built + per-route OpenRouter ZDR-feed check ever runs on them. Both OpenRouter + rows here carry ``evidence_only=True`` (today's real bug shape) and + both must still pass through; a same-shaped row from a different + provider must not. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + openrouter_evidence_only_a = SimpleNamespace( + id="openrouter_evidence_only_a", + provider_name="openrouter", + model_id="some/model", + evidence_only=True, + ) + openrouter_evidence_only_b = SimpleNamespace( + id="openrouter_evidence_only_b", + provider_name="openrouter", + model_id="ready/free", + evidence_only=True, + ) + nvidia_evidence_only = SimpleNamespace( + id="nvidia_evidence_only", + provider_name="nvidia_nim", + model_id="some/model", + evidence_only=True, + ) + + assert routable( + [openrouter_evidence_only_a, openrouter_evidence_only_b, nvidia_evidence_only] + ) == [openrouter_evidence_only_a, openrouter_evidence_only_b] + + +def test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence() -> None: + """The historical exemption turns off the moment per-model evidence appears. + + Once ``ContextualWisdomLab/.github#1477`` lands the + ``ContextualWisdomLab/contextual-orchestrator#949`` pin bump, OpenRouter starts reporting real + per-model ``evidence_only`` (at minimum ``False`` for its genuinely + ZDR-attested free models). This is the post-fix signature: a run whose + OpenRouter rows are no longer uniformly ``True`` must go back through + the same ``evidence_only`` contract every other provider's rows + already get -- an attested row still passes (it always would have, on + its own merit), but an unattested OpenRouter row is now excluded here + exactly like a same-shaped row from any other provider, with no pin + bump or manual code edit required to reach this behavior. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + openrouter_attested = SimpleNamespace( + id="openrouter_attested", + provider_name="openrouter", + model_id="attested/free", + evidence_only=False, + ) + openrouter_unattested = SimpleNamespace( + id="openrouter_unattested", + provider_name="openrouter", + model_id="unattested/model", + evidence_only=True, + ) + + assert routable([openrouter_attested, openrouter_unattested]) == [openrouter_attested] + + +def test_routable_discovered_models_excludes_spend_blocked_rows() -> None: + """A ``spend_admitted=False`` row is excluded the same way as ``evidence_only=True``. + + ``contextual-orchestrator#949`` added ``DiscoveredModel.spend_admitted`` + (default ``True``): a priced OpenRouter row becomes ``False`` when + ``openrouter_paid_inference_available()`` cannot confirm usable credit. + The vendored library's own ``is_routable_discovered_model()`` already + refuses to activate such a row as an agent; this launcher must refuse it + too, with the same ``getattr(..., True)`` default so a vendored pin that + predates ``#949`` (and so has no ``spend_admitted`` attribute at all) + keeps behaving exactly as it did before this filter existed. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + spend_blocked = SimpleNamespace( + id="openrouter_spend_blocked", + provider_name="openrouter", + model_id="provider/paid", + evidence_only=False, + spend_admitted=False, + ) + spend_admitted_row = SimpleNamespace( + id="openrouter_spend_admitted", + provider_name="openrouter", + model_id="provider/paid-ok", + evidence_only=False, + spend_admitted=True, + ) + no_spend_attribute = SimpleNamespace( + id="nvidia_untagged", + provider_name="nvidia_nim", + model_id="untagged/model", + evidence_only=False, + ) + + assert routable([spend_blocked, spend_admitted_row, no_spend_attribute]) == [ + spend_admitted_row, + no_spend_attribute, + ] + + +def test_routable_discovered_models_excludes_spend_blocked_openrouter_row_even_while_evidence_only_exempt() -> None: + """The ``spend_admitted`` exclusion applies independently of the ``evidence_only`` exemption. + + A spend-blocked OpenRouter row that also still carries today's blanket + ``evidence_only=True`` bug signature -- so the OpenRouter ``evidence_ + only`` exemption would otherwise let it through -- must still be + excluded: the two filters are independent conditions, and neither + exemption weakens the other. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + openrouter_blanket_and_spend_blocked = SimpleNamespace( + id="openrouter_blanket_spend_blocked", + provider_name="openrouter", + model_id="provider/paid", + evidence_only=True, + spend_admitted=False, + ) + openrouter_blanket_and_admitted = SimpleNamespace( + id="openrouter_blanket_admitted", + provider_name="openrouter", + model_id="provider/free", + evidence_only=True, + spend_admitted=True, + ) + + assert routable( + [openrouter_blanket_and_spend_blocked, openrouter_blanket_and_admitted] + ) == [openrouter_blanket_and_admitted] + + +def test_pool_auto_never_admits_a_spend_blocked_priced_openrouter_row() -> None: + """A credit-exhausted paid OpenRouter row must never reach ``orchestrator/auto``. + + Regression for the real, latent gap found while investigating whether + this repo's pipeline needs to respect ``spend_admitted``: + ``orchestrator/free`` never considers priced rows at all (its + ``selected_models`` loop in ``main()`` drops anything outside + ``free_route_identities`` before it becomes a report row), so it was + never exposed to a spend-blocked row. ``--pool auto`` is real, tested, + reachable code (``CONTEXTUAL_ORCHESTRATOR_POOL=auto``, no other change + needed) whose candidate rows explicitly include priced ones + (``build_zdr_prioritized_catalog``'s ``[*all_free_rows, + *all_priced_rows]`` for ``pool="auto"``, plus ``main()``'s explicit + priced-fallback stage) -- so without the ``spend_admitted`` filter in + ``_routable_discovered_models``, a spend-blocked row could have reached + a served ``auto`` catalog exactly as if it were servable. This composes + the real pipeline: ``_routable_discovered_models`` -> ``_report_rows`` + -> ``parse_discovery_report`` -> ``build_zdr_prioritized_catalog``. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + report_rows = namespace["_report_rows"] + route_identity = namespace["_route_identity"] + + free_model = SimpleNamespace( + provider_name="nvidia_nim", + model_id="free/model", + agent_id="nvidia_nim_free_model", + evidence_only=False, + spend_admitted=True, + prompt_price_per_1k=0.0, + completion_price_per_1k=0.0, + currency_code="USD", + ) + spend_blocked_model = SimpleNamespace( + provider_name="openrouter", + model_id="provider/paid", + agent_id="openrouter_provider_paid", + evidence_only=False, + spend_admitted=False, + chat_base_url="https://openrouter.ai/api/v1", + credential_name="OPENROUTER_API_KEY", + auth_scheme="Bearer", + prompt_price_per_1k=0.1, + completion_price_per_1k=0.1, + currency_code="USD", + ) + discovered = [free_model, spend_blocked_model] + + routable_discovered = routable(discovered) + assert routable_discovered == [free_model] + + free_route_identities = frozenset({route_identity(free_model)}) + rows = report_rows(routable_discovered, free_route_identities) + normalized_rows = policy.parse_discovery_report({"models": rows}) + + result = policy.build_zdr_prioritized_catalog(normalized_rows, pool="auto") + + assert {entry["model"] for entry in result["agents"]} == {"free/model"} + + +def test_require_zdr_still_excludes_non_zdr_openrouter_route_despite_evidence_only_exemption() -> None: + """The ``evidence_only`` exemption never weakens the real ZDR admission gate. + + Devin Review flagged (``ContextualWisdomLab/.github#1476``, discussion + ``r3891875749``, 🟥) that ``_routable_discovered_models`` converting + every OpenRouter row into a serving candidate while every row still + carries the vendored ``evidence_only=True`` bug signature could let + "private review content reach third-party routes the vendored ZDR + contract forbids serving." Traced end to end, this is a false alarm: + becoming a *candidate* that survives ``_routable_discovered_models`` is + not the same as being *admitted* to a ``--require-zdr`` (private) + target's served catalog. The real, independent admission gate for that + path is ``is_zdr_model()`` (``scripts/ci/zdr_policy.py``), which + ``build_zdr_prioritized_catalog`` -- the function that actually builds + the served ``agents`` catalog -- re-applies as its own ``eligible_rows`` + filter whenever ``require_zdr=True``, completely independent of + ``evidence_only``. This reproduces the real pipeline + (``_routable_discovered_models`` -> ``_report_rows`` -> + ``parse_discovery_report`` -> ``build_zdr_prioritized_catalog(..., + require_zdr=True)``) with two free OpenRouter rows that BOTH still + report ``evidence_only=True`` (today's exact bug signature, so the + exemption is active for both) -- only the one genuinely present in the + live OpenRouter ZDR feed is ever admitted to the catalog. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + report_rows = namespace["_report_rows"] + route_identity = namespace["_route_identity"] + + zdr_model = SimpleNamespace( + provider_name="openrouter", + model_id="zdr/free-model", + agent_id="openrouter_zdr_free_model", + evidence_only=True, + prompt_price_per_1k=0.0, + completion_price_per_1k=0.0, + currency_code="USD", + ) + non_zdr_model = SimpleNamespace( + provider_name="openrouter", + model_id="forbidden/free-model", + agent_id="openrouter_forbidden_free_model", + evidence_only=True, + prompt_price_per_1k=0.0, + completion_price_per_1k=0.0, + currency_code="USD", + ) + discovered = [zdr_model, non_zdr_model] + + routable_discovered = routable(discovered) + # Both rows survive the still-blanket-marked exemption: exactly the + # shape the Devin finding is concerned about. + assert routable_discovered == discovered + + free_route_identities = frozenset(route_identity(model) for model in discovered) + rows = report_rows(discovered, free_route_identities) + normalized_rows = policy.parse_discovery_report({"models": rows}) + + result = policy.build_zdr_prioritized_catalog( + normalized_rows, + zdr_endpoints=frozenset({"openrouter/zdr/free-model"}), + require_zdr=True, + pool="free", + ) + + selected_models = {entry["model"] for entry in result["agents"]} + assert selected_models == {"zdr/free-model"} + assert "forbidden/free-model" not in selected_models + + def test_log_discovery_errors_prints_one_bounded_line_per_provider_failure( capsys: pytest.CaptureFixture[str], ) -> None: @@ -1510,6 +1795,29 @@ def test_catalog_account_cap_honors_an_explicit_override( assert namespace["_catalog_account_cap"](policy.DEFAULT_ACCOUNT_CAP) == 6 +def test_main_wires_guarantee_domain_coverage_for_both_catalog_stages() -> None: + """``main()``'s primary and priced-fallback catalog calls both opt into coverage. + + Devin Review finding on `.github#1474`: the primary stage's own real + deployment shape has the identical single-scalar-cap-equals-limit + coincidence the fallback stage already had fixed -- the sidecar's own + default `ORCHESTRATOR_CATALOG_ACCOUNT_CAP` is 8 (not + `DEFAULT_ACCOUNT_CAP`'s library-level fallback of 4, which the sidecar + never leaves the env var unset for), and `REVIEW_PREFLIGHT_PRIMARY_ + ROUTE_LIMIT` is also 8 for the ``auto`` pool's primary stage. Both + ``build_zdr_prioritized_catalog`` call sites in ``main()`` must pass + ``guarantee_domain_coverage=True``. + """ + source = _LAUNCHER.read_text(encoding="utf-8") + assert source.count("guarantee_domain_coverage=True") == 2 + primary_call_start = source.index("result = build_zdr_prioritized_catalog(") + primary_call = source[primary_call_start : primary_call_start + 400] + assert "guarantee_domain_coverage=True" in primary_call + fallback_call_start = source.index('pool == "auto"\n and admitted_free_rows') + fallback_call = source[fallback_call_start : fallback_call_start + 800] + assert "guarantee_domain_coverage=True" in fallback_call + + def test_main_sources_the_account_cap_default_from_policy_not_a_magic_number() -> None: """``main()`` must wire the cap default from ``policy.DEFAULT_ACCOUNT_CAP``. @@ -1520,9 +1828,16 @@ def test_main_sources_the_account_cap_default_from_policy_not_a_magic_number() - source-level contract test pins both ``build_zdr_prioritized_catalog`` call sites in ``main()`` to the single source of truth and forbids the total-routes constant from ever reappearing as the account-cap fallback. + Both the primary-stage and priced-fallback call sites pass + ``_catalog_account_cap(DEFAULT_ACCOUNT_CAP)`` directly as + ``account_cap=``; the fallback call site additionally sets + ``guarantee_domain_coverage=True`` (see + ``policy.build_zdr_prioritized_catalog``'s own regression tests for the + domain-diversity fix that flag adds), which does not change what value + the cap itself is sourced from. """ source = _LAUNCHER.read_text(encoding="utf-8") - assert source.count("account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP)") == 2 + assert source.count("_catalog_account_cap(DEFAULT_ACCOUNT_CAP)") == 2 assert "ORCHESTRATOR_CATALOG_FAMILY_CAP" not in source assert 'os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", "4")' not in source @@ -1553,19 +1868,23 @@ def test_discovery_counts_survive_stage_specific_policy_reports() -> None: namespace = _load_launcher() base = {"selected_count": 1, "selected": [{"model": "priced/model"}]} rows = [ - {"cost_evidence": "free", "provider": "nvidia_nim"}, - {"cost_evidence": "priced", "provider": "openai"}, - {"cost_evidence": "priced", "provider": "openai"}, - {"cost_evidence": "unknown", "provider": "bytez"}, + {"cost_evidence": "free", "provider": "nvidia_nim", "base_url": "https://integrate.api.nvidia.com/v1"}, + {"cost_evidence": "priced", "provider": "openai", "base_url": "https://api.openai.com/v1"}, + {"cost_evidence": "priced", "provider": "openai", "base_url": "https://api.openai.com/v1"}, + {"cost_evidence": "unknown", "provider": "bytez", "base_url": "https://api.bytez.com/models/v2/openai/v1"}, ] enriched = namespace["_with_discovery_counts"]( - base, rows, provider_account=policy.provider_account + base, + rows, + provider_account=policy.provider_account, + outage_domain=policy._outage_domain, ) assert base == {"selected_count": 1, "selected": [{"model": "priced/model"}]} assert [enriched[key] for key in ( "total_routes", "total_free_routes", "total_priced_routes", "total_unknown_routes" )] == [4, 1, 2, 1] assert enriched["free_account_diversity"] == 1 + assert enriched["free_outage_domain_diversity"] == 1 def test_discovery_counts_recompute_diversity_from_full_discovery_not_the_stage() -> None: @@ -1573,24 +1892,60 @@ def test_discovery_counts_recompute_diversity_from_full_discovery_not_the_stage( Regression for a real bug: the ``auto``-pool primary stage only sees ZDR-admitted free rows, and the priced-fallback stage sees no free rows - at all, so either stage's internally computed ``free_account_diversity`` + at all, so either stage's internally computed diversity fields (whatever ``build_zdr_prioritized_catalog`` returned from its own narrower input) would undercount or read zero even when the full - discovery has multiple credential accounts with free routes. + discovery has multiple credential accounts (and outage domains) with + free routes. """ namespace = _load_launcher() - stage_report_from_priced_only_rows = {"free_account_diversity": 0} + stage_report_from_priced_only_rows = { + "free_account_diversity": 0, + "free_outage_domain_diversity": 0, + } full_discovery_rows = [ - {"cost_evidence": "free", "provider": "nvidia_nim"}, - {"cost_evidence": "free", "provider": "openrouter"}, - {"cost_evidence": "priced", "provider": "openai"}, + {"cost_evidence": "free", "provider": "nvidia_nim", "base_url": "https://integrate.api.nvidia.com/v1"}, + {"cost_evidence": "free", "provider": "openrouter", "base_url": "https://openrouter.ai/api/v1"}, + {"cost_evidence": "priced", "provider": "openai", "base_url": "https://api.openai.com/v1"}, ] enriched = namespace["_with_discovery_counts"]( stage_report_from_priced_only_rows, full_discovery_rows, provider_account=policy.provider_account, + outage_domain=policy._outage_domain, + ) + assert enriched["free_account_diversity"] == 2 + assert enriched["free_outage_domain_diversity"] == 2 + + +def test_discovery_counts_distinguish_account_from_outage_domain_diversity() -> None: + """Two same-endpoint NVIDIA credentials are 2 accounts but 1 outage domain. + + Regression for a real, separate bug found by review during this + session: #1468 correctly stopped treating ``nvidia_nim``/ + ``nvidia_nim_sub`` as one *model-catalog* family (they are independent + credentials that may expose different models), but a naive read of that + fix could also wrongly assume they are two independent *outage domains* + -- they are not: both resolve to the identical + ``https://integrate.api.nvidia.com/v1`` upstream. If one physical + endpoint's outage were mistaken for two independent domains, a caller + gating on diversity (e.g. open PR #1437's Strix ``orchestrator/free`` + eligibility check) could wrongly conclude the free catalog can survive + that single outage. + """ + namespace = _load_launcher() + full_discovery_rows = [ + {"cost_evidence": "free", "provider": "nvidia_nim", "base_url": "https://integrate.api.nvidia.com/v1"}, + {"cost_evidence": "free", "provider": "nvidia_nim_sub", "base_url": "https://integrate.api.nvidia.com/v1"}, + ] + enriched = namespace["_with_discovery_counts"]( + {}, + full_discovery_rows, + provider_account=policy.provider_account, + outage_domain=policy._outage_domain, ) assert enriched["free_account_diversity"] == 2 + assert enriched["free_outage_domain_diversity"] == 1 def test_temporary_fallback_catalog_is_removed_after_loading(tmp_path: Path) -> None: diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 0a63356dad..92a01bf5ee 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -284,6 +284,18 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert "routable_discovered = _routable_discovered_models(discovered)" in text assert "free_discovered_models(routable_discovered)" in text assert 'getattr(model, "evidence_only", False)' in text + # OpenRouter must stay exempt from the evidence_only exclusion, or + # zdr_policy.is_zdr_model()'s purpose-built per-route OpenRouter ZDR-feed + # check goes back to never seeing an OpenRouter row at all. This + # fragment-presence check only pins that the comparison exists + # somewhere in source -- Devin Review (discussion r3891875665) correctly + # noted it would still pass even if the exemption were reversed (e.g. + # ``!=`` for ``==``) or disconnected from the rows it is meant to gate. + # The behavioral assertions below, against the loaded module's real + # ``_routable_discovered_models``, close that gap by pinning the actual + # boolean outcome for both a row that must be exempted and one that + # must not. + assert 'getattr(model, "provider_name", None) == "openrouter"' in text assert 'getattr(model, "output_modalities", None)' in text assert 'isinstance(modalities, str)' in text assert '"text" in {str(modality).casefold() for modality in modalities}' in text @@ -296,6 +308,26 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert has_text_output(SimpleNamespace(output_modalities=("text", "image"))) assert not has_text_output(SimpleNamespace(output_modalities=("video",))) assert not has_text_output(SimpleNamespace()) + + # Pin the exemption's real boolean outcome, not just source-text + # presence: an OpenRouter row carrying today's blanket evidence_only=True + # bug signature must still be routable, while a same-shaped row from any + # other provider must not -- so a reversed comparison (``!=`` instead of + # ``==``) or a disconnected/no-op exemption (e.g. the OpenRouter branch + # never actually reached, or applied unconditionally regardless of + # provider) fails this assertion even though the source fragment above + # would still be present verbatim. + routable_discovered_models = launcher["_routable_discovered_models"] + openrouter_blanket_marked = SimpleNamespace( + provider_name="openrouter", model_id="some/model", evidence_only=True + ) + non_openrouter_evidence_only = SimpleNamespace( + provider_name="nvidia_nim", model_id="some/model", evidence_only=True + ) + assert routable_discovered_models( + [openrouter_blanket_marked, non_openrouter_evidence_only] + ) == [openrouter_blanket_marked] + report_rows = launcher["_report_rows"] free = SimpleNamespace( provider_name="openrouter", diff --git a/tests/test_noema_removed_file_context.py b/tests/test_noema_removed_file_context.py index 8c5d8ca539..500d406f73 100644 --- a/tests/test_noema_removed_file_context.py +++ b/tests/test_noema_removed_file_context.py @@ -3,6 +3,7 @@ from __future__ import annotations import base64 +import json from scripts.ci import noema_review_gate as noema @@ -12,7 +13,14 @@ def test_fetch_changed_files_preserves_path_and_status(monkeypatch): monkeypatch.setattr( noema, "run", - lambda args, stdin=None: "a.py\tmodified\n\nb.py\tremoved\nfuzz/x.py\tadded\n", + lambda args, stdin=None: ( + json.dumps(["a.py", "modified"]) + + "\n\n" + + json.dumps(["b.py", "removed"]) + + "\n" + + json.dumps(["fuzz/x.py", "added"]) + + "\n" + ), ) assert noema.fetch_changed_files("owner/repo", 7) == [ @@ -22,8 +30,11 @@ def test_fetch_changed_files_preserves_path_and_status(monkeypatch): ] -def test_removed_file_context_uses_base_content(monkeypatch): - """A deleted file must be reviewed from immutable pre-deletion evidence.""" +def test_removed_file_context_uses_merge_base_content(monkeypatch): + """A deleted file must be reviewed from immutable merge-base evidence.""" + head_sha = "a" * 40 + base_sha = "b" * 40 + merge_base_sha = "c" * 40 encoded = base64.b64encode(b"def doomed():\n pass\n").decode("ascii") calls: list[str] = [] @@ -31,24 +42,88 @@ def fake_run(args, stdin=None): target = args[2] calls.append(target) if target.endswith("/files"): - return "fuzz/fuzz_opencode_normalize_output.py\tremoved\n" - if "contents/fuzz/fuzz_opencode_normalize_output.py?ref=base-sha" in target: + return json.dumps(["fuzz/fuzz_opencode_normalize_output.py", "removed"]) + "\n" + if target == f"repos/owner/repo/compare/{base_sha}...{head_sha}": + return merge_base_sha + if f"contents/fuzz/fuzz_opencode_normalize_output.py?ref={merge_base_sha}" in target: return encoded raise AssertionError(args) monkeypatch.setattr(noema, "run", fake_run) - context = noema.changed_file_context( - "owner/repo", 1486, "head-sha", "base-sha" - ) + context = noema.changed_file_context("owner/repo", 1486, head_sha, base_sha) - assert "File removed in this PR. Pre-deletion content at base ref" in context + assert f"Pre-deletion content at merge base `{merge_base_sha}`" in context assert "def doomed" in context - assert not any("ref=head-sha" in target for target in calls) + assert not any(f"ref={head_sha}" in target for target in calls) + + +def test_fetch_changed_files_rejects_malformed_json_line(monkeypatch): + """A non-JSON line from the Files API must fail closed, not crash raw.""" + monkeypatch.setattr(noema, "run", lambda args, stdin=None: "not json\n") + + try: + noema.fetch_changed_files("owner/repo", 7) + except RuntimeError as exc: + assert "malformed" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed JSON line") + + +def test_fetch_changed_files_rejects_malformed_record_shape(monkeypatch): + """A well-formed JSON line that is not a two-element string pair must fail closed.""" + monkeypatch.setattr( + noema, "run", lambda args, stdin=None: json.dumps(["only-one-field"]) + "\n" + ) + + try: + noema.fetch_changed_files("owner/repo", 7) + except RuntimeError as exc: + assert "malformed" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed record shape") + + +def test_fetch_merge_base_sha_rejects_malformed_head_sha(): + """An invalid head SHA must be rejected before any network call is attempted.""" + try: + noema.fetch_merge_base_sha("owner/repo", "a" * 40, "not-a-sha") + except RuntimeError as exc: + assert "PR head SHA was unavailable or malformed" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed head SHA") + + +def test_fetch_merge_base_sha_rejects_malformed_compare_response(monkeypatch): + """A compare response lacking a valid merge-base SHA must fail closed.""" + monkeypatch.setattr(noema, "run", lambda args, stdin=None: "") + + try: + noema.fetch_merge_base_sha("owner/repo", "a" * 40, "b" * 40) + except RuntimeError as exc: + assert "did not contain a valid merge-base SHA" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed compare response") + + +def test_removed_file_context_section_without_merge_base_or_error(): + """No merge-base SHA and no recorded error must still be explicit, not silent.""" + context = noema.removed_file_context_section("owner/repo", "gone.py", "", "") + + assert "merge-base SHA unavailable for pre-deletion content" in context + + +def test_removed_file_context_section_empty_merge_base_content(monkeypatch): + """An empty (non-UTF-8-decodable) merge-base blob must be reported, not silently dropped.""" + monkeypatch.setattr(noema, "fetch_file_content_at_ref", lambda repo, path, ref: "") + + context = noema.removed_file_context_section("owner/repo", "gone.py", "c" * 40, "") + + assert "no UTF-8 text content available from merge-base content API" in context def test_removed_file_context_fails_closed_without_base_sha(monkeypatch): - """Missing base identity must be explicit and must not trigger a head fetch.""" + """Missing base identity must be explicit and must not trigger a content fetch.""" monkeypatch.setattr( noema, "fetch_changed_files", @@ -56,44 +131,49 @@ def test_removed_file_context_fails_closed_without_base_sha(monkeypatch): ) monkeypatch.setattr( noema, - "fetch_head_file_content", + "fetch_file_content_at_ref", lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("unexpected fetch")), ) - context = noema.changed_file_context("owner/repo", 7, "head-sha", "") + context = noema.changed_file_context("owner/repo", 7, "a" * 40, "") + + assert "PR base SHA was unavailable or malformed" in context + assert "Merge-base lookup unavailable" in context - assert "base SHA unavailable" in context +def test_removed_file_merge_base_content_failure_is_distinct_from_head_failure(monkeypatch): + """A merge-base content API failure must remain typed as merge-base evidence failure.""" + head_sha = "a" * 40 + base_sha = "b" * 40 + merge_base_sha = "c" * 40 -def test_removed_file_base_fetch_failure_is_distinct_from_head_failure(monkeypatch): - """A base-side API failure must remain typed as base evidence failure.""" monkeypatch.setattr( noema, "fetch_changed_files", lambda repo, number: [("gone.py", "removed")], ) + monkeypatch.setattr( + noema, "fetch_merge_base_sha", lambda repo, base, head: merge_base_sha + ) def fail_fetch(repo, path, ref): raise RuntimeError("HTTP 502: token ***") - monkeypatch.setattr(noema, "fetch_head_file_content", fail_fetch) + monkeypatch.setattr(noema, "fetch_file_content_at_ref", fail_fetch) - context = noema.changed_file_context( - "owner/repo", 7, "head-sha", "base-sha" - ) + context = noema.changed_file_context("owner/repo", 7, head_sha, base_sha) - assert "Unavailable from base content API" in context + assert "Unavailable from merge-base content API" in context assert "Unavailable from head content API" not in context def test_build_review_context_passes_live_base_ref(monkeypatch): """The GraphQL base identity must reach changed-file context construction.""" - observed: list[tuple[str, int, str, str]] = [] + observed: list[tuple[str, int, str, str, object]] = [] monkeypatch.setattr(noema, "review_thread_context", lambda pr: "") - monkeypatch.setattr(noema, "load_codegraph_context", lambda: "") - def fake_context(repo, number, head_sha, base_sha=""): - observed.append((repo, number, head_sha, base_sha)) + def fake_context(repo, number, head_sha, base_sha="", changed_files=None): + observed.append((repo, number, head_sha, base_sha, changed_files)) return "files" monkeypatch.setattr(noema, "changed_file_context", fake_context) @@ -104,5 +184,5 @@ def fake_context(repo, number, head_sha, base_sha=""): {"headRefOid": "head-sha", "baseRefOid": "base-sha"}, ) - assert observed == [("owner/repo", 7, "head-sha", "base-sha")] + assert observed == [("owner/repo", 7, "head-sha", "base-sha", None)] assert "## Changed file context\nfiles" in result diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 43aaf46e81..a86ee3b499 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1250,8 +1250,8 @@ def test_inspect_and_review_reports_stale_before_repair_retry_cleanly(monkeypatc monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") def fake_call_llm(*args, **kwargs): raise noema.StaleHeadDuringRepairRetryError( @@ -1694,15 +1694,15 @@ def test_current_actor_rejects_unbound_action_identity(monkeypatch, actor, insta noema.current_actor() -def test_review_context_builders_include_codegraph_threads_and_files(monkeypatch, tmp_path): +def test_review_context_builders_include_threads_and_files(monkeypatch, tmp_path): assert noema.truncate_text("abc", 10) == "abc" assert "truncated 2 characters" in noema.truncate_text("abcdef", 4) assert "missing PR head SHA" in noema.changed_file_context("owner/repo", 7, "") - original_fetch_paths = noema.fetch_changed_file_paths - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: []) + original_fetch_changed_files = noema.fetch_changed_files + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: []) assert "no changed files" in noema.changed_file_context("owner/repo", 7, "head") - monkeypatch.setattr(noema, "fetch_changed_file_paths", original_fetch_paths) + monkeypatch.setattr(noema, "fetch_changed_files", original_fetch_changed_files) encoded = base64.b64encode(b"print('hello')\n").decode("ascii") calls = [] @@ -1711,7 +1711,10 @@ def fake_run(args, stdin=None): calls.append(args) target = args[2] if target.endswith("/files"): - return "src/a.py\nREADME.md\nempty.txt\n" + return "\n".join( + json.dumps([path, "modified"]) + for path in ("src/a.py", "README.md", "empty.txt") + ) + "\n" if "contents/src/a.py" in target: return encoded if "contents/README.md" in target: @@ -1721,9 +1724,6 @@ def fake_run(args, stdin=None): raise AssertionError(args) monkeypatch.setattr(noema, "run", fake_run) - codegraph_path = tmp_path / "codegraph.md" - codegraph_path.write_text("call graph: src/a.py -> tests", encoding="utf-8") - monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(codegraph_path)) pr = make_pr( headRefOid="head sha", reviewThreads={ @@ -1747,8 +1747,6 @@ def fake_run(args, stdin=None): context = noema.build_review_context("owner/repo", 7, pr) - assert "## CodeGraph context" in context - assert "call graph: src/a.py -> tests" in context assert "Thread open at src/a.py:3" in context assert "reviewer: check call site" in context assert "### src/a.py" in context @@ -1758,16 +1756,14 @@ def fake_run(args, stdin=None): assert any("/files" in call[2] for call in calls) -def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch, tmp_path): - monkeypatch.delenv("NOEMA_CODEGRAPH_CONTEXT_PATH", raising=False) - assert noema.load_codegraph_context() == "" - - monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(tmp_path / "missing.md")) - assert "CodeGraph context unavailable" in noema.load_codegraph_context() - +def test_review_context_reports_omitted_files(monkeypatch, tmp_path): paths = [f"src/file_{index}.py" for index in range(noema.MAX_CONTEXT_FILES + 1)] - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths) - monkeypatch.setattr(noema, "fetch_head_file_content", lambda repo, path, head_sha: "x") + monkeypatch.setattr( + noema, + "fetch_changed_files", + lambda repo, number: [(path, "modified") for path in paths], + ) + monkeypatch.setattr(noema, "fetch_file_content_at_ref", lambda repo, path, ref: "x") context = noema.changed_file_context("owner/repo", 7, "head") @@ -2007,8 +2003,8 @@ def test_inspect_and_review_skip_paths(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) @@ -2048,8 +2044,8 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) @@ -2087,8 +2083,8 @@ def test_head_movement_stops_before_review_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr( noema, "call_llm", @@ -2110,8 +2106,8 @@ def test_closed_during_model_stops_before_review_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve"}) monkeypatch.setattr( noema, @@ -2129,8 +2125,8 @@ def test_uppercase_expected_head_is_not_stale_before_model_work(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) calls = [] monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) @@ -2146,8 +2142,8 @@ def test_uppercase_expected_head_is_not_stale_before_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr( noema, "call_llm", @@ -2168,8 +2164,8 @@ def test_inspect_and_review_rechecks_head_before_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(responses)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve"}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: submitted.append(args)) diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 2854e0671f..a958cd2bc1 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -2585,6 +2585,10 @@ def test_opencode_privileged_review_security_boundaries_are_fail_closed(): '^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]' ) in metadata_step assert '[ "$live_head_repository" != "$TARGET_REPOSITORY" ]' not in metadata_step + # #1533 briefly relaxed this to a warn-and-proceed check, but #1540 + # reverted it back to the original strict fail-closed equality (no + # rationale recorded beyond the revert itself) -- confirmed against + # main's actual current content, not assumed from the PR history. assert '[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ]' in metadata_step assert 'mismatches+=("head_sha")' in metadata_step assert "proceeding with the live head" not in metadata_step