From f3facda54a88dbac65009638ecdcebe289d3245d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:06:56 +0000 Subject: [PATCH 01/24] fix(ci): group review catalog admission/diversity by outage domain, not account model-catalog family (they are independent credentials that may expose different models), but in doing so also let the admission cap and free_account_diversity 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, which already said as much). Two independent questions exist for this credential pair: 1. Model-catalog identity (may they expose different models?) -- yes, fixed correctly by #941/#945/#1468. 2. Outage-domain identity (would one physical outage take both down?) -- also yes, and #1468 flattened this axis to match axis 1. Concrete consequences fixed here: - free_account_diversity reported 2 for a discovery report whose only free routes were these two credentials -- falsely reassuring for exactly the decision this evidence exists to support (open PR #1437's Strix orchestrator/free eligibility gate: would a single outage empty the free catalog). - The admission cap (account_cap, sidecar default 8) let the pair jointly consume up to twice its intended per-endpoint budget, crowding out a genuinely independent provider's free routes even when it had capacity. Fix: contextual_orchestrator_review_policy.py gains _outage_domain(row), keyed on each row's own base_url evidence (not a second hand-maintained provider-name table, so it cannot 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; a new, additive free_outage_domain_diversity report field sits alongside the existing free_account_diversity (kept, not renamed, to avoid further naming churn right after #1468's own rename). contextual_orchestrator_review_launcher.py's _with_discovery_counts restores both fields from full discovery rows the same way. account_cap/DEFAULT_ACCOUNT_CAP/--account-cap/ORCHESTRATOR_CATALOG_ACCOUNT_CAP 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 this same sidecar's pin. Tests: two dedicated regressions (semantic-conflation shape: 2 accounts, 1 domain; crowding-out shape: a shared-endpoint pair with many free rows vs. an independent provider with few) plus updated existing tests (test_build_catalog_applies_account_cap and friends, two launcher-facing tests in test_contextual_orchestrator_review_runtime_preflight.py). Full suite: 2095 passed, 1 skipped, 21 subtests passed. 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, for the >= 2 eligibility check. Does not revive closed PR #1470 (a different, now- superseded fix); this is a fresh, narrowly-scoped follow-up found by review against current main after #1468 merged. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 9 ++ ...ntextual-orchestrator-vendored-free-zdr.md | 23 ++++ docs/product-goal-directive.md | 2 +- docs/product-technical-gap-baseline.md | 69 ++++++++++++ ...contextual_orchestrator_review_launcher.py | 48 ++++++--- .../contextual_orchestrator_review_policy.py | 96 ++++++++++++++--- ...t_contextual_orchestrator_review_policy.py | 102 ++++++++++++++++-- ...l_orchestrator_review_runtime_preflight.py | 62 +++++++++-- 8 files changed, 361 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 256b0cdf94..c630ac5cd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,15 @@ 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. - 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 21b118ce90..87316cce2a 100644 --- a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md +++ b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md @@ -190,3 +190,26 @@ 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. 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 758ef2961a..463660b8e7 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,75 @@ 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. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 6dbbe2d5e3..c299f79700 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -670,20 +670,30 @@ 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. """ enriched = dict(report) + free_rows = [row for row in rows if row.get("cost_evidence") == "free"] enriched.update( { "total_routes": len(rows), @@ -691,11 +701,10 @@ def _with_discovery_counts( "total_priced_routes": sum(row.get("cost_evidence") == "priced" for row in rows), "total_unknown_routes": sum(row.get("cost_evidence") == "unknown" for row in rows), "free_account_diversity": len( - { - provider_account(str(row["provider"])) - for row in rows - if row.get("cost_evidence") == "free" - } + {provider_account(str(row["provider"])) for row in free_rows} + ), + "free_outage_domain_diversity": len( + {outage_domain(row) for row in free_rows} ), } ) @@ -777,6 +786,7 @@ def main(argv: list[str] | None = None) -> int: from scripts.ci.contextual_orchestrator_review_policy import ( PolicyError, _load_zdr_endpoints, + _outage_domain, build_zdr_prioritized_catalog, is_zdr_model, parse_discovery_report, @@ -854,7 +864,10 @@ def main(argv: list[str] | None = None) -> int: pool=args.pool, ) 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", @@ -888,7 +901,10 @@ def main(argv: list[str] | None = None) -> int: 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 1c8a170144..721093f302 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -51,6 +51,39 @@ 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. + """ + return str(row["base_url"]) + + 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() @@ -198,20 +231,47 @@ def build_zdr_prioritized_catalog( require_zdr: bool = False, pool: str = "free", ) -> dict[str, Any]: - """Select a free-first, ZDR-aware, credential-account-diverse catalog. - - The returned report's ``free_account_diversity`` counts the distinct - credential accounts among *all* discovered free routes, independent of - ``pool`` or the per-account selection cap. Vendor identity is not model - equivalence; only an explicit contextual-orchestrator ``model_group`` may - share routing evidence across routes. + """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 value of two or - more is evidence that one account failure cannot immediately empty the free - catalog, not proof that either account is presently reachable. A caller - needing readiness, not just discovery-time diversity, must combine this - with the runtime preflight report the sidecar already produces. + 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. """ if pool not in {"free", "auto"}: raise PolicyError(f"unsupported review pool {pool!r}") @@ -248,13 +308,13 @@ def build_zdr_prioritized_catalog( ) ) - 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: + domain = _outage_domain(row) + if per_domain[domain] >= account_cap: continue - per_account[account] += 1 + per_domain[domain] += 1 picked.append(row) if len(picked) >= limit: break @@ -304,6 +364,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} + ) selected_evidence = [_cost_evidence(row) for row in picked] return { @@ -315,6 +378,7 @@ def build_zdr_prioritized_catalog( "total_priced_routes": len(all_priced_rows), "total_unknown_routes": len(all_unknown_rows), "free_account_diversity": free_account_diversity, + "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_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index b10fc4a0b9..95b7f31451 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -75,6 +75,16 @@ 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( ("candidate", "provider", "expected"), [ @@ -275,7 +285,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 +301,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 +343,7 @@ 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_rejects_unknown_pool() -> None: @@ -337,7 +370,19 @@ 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, not per credential. + + ``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). Sort order + (alphabetical among same-cost, same-ZDR rows) picks the two admitted + NVIDIA-domain rows from ``nvidia_nim`` specifically, since + ``"nvidia_nim" < "nvidia_nim_sub"``. + """ report = { "models": [ {"provider": "nvidia_nim", "model": f"m{i}", "agent_id": f"nim_a{i}", "is_free": True, **FREE_PRICE} @@ -365,9 +410,54 @@ 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["openai"] == 2 + assert account_counts == {"nvidia_nim": 2, "openai": 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 (all from nvidia_nim, + # sorted first) -- not 4 from each credential -- leaving bytez and + # openrouter, each an independent domain, fully admitted. + assert counts == {"bytez": 2, "nvidia_nim": 4, "openrouter": 2} + assert result["report"]["free_account_diversity"] == 4 + assert result["report"]["free_outage_domain_diversity"] == 3 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 32f1c22413..64f6731d5c 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1480,19 +1480,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: @@ -1500,24 +1504,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: From 0c13cb8f061bc3b215f12b3cfd3a9510e63a8b2d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:20:25 +0000 Subject: [PATCH 02/24] fix(ci): normalize base_url before using it as the outage-domain key Devin Review finding on #1474: _outage_domain(row) compared raw base_url strings, so two rows for the identical physical endpoint but spelled differently (hostname case, an explicit default port like :443, a trailing slash) would be treated as two different outage domains -- directly undermining the fix free_outage_domain_diversity/the admission cap exist to provide. Verified against this codebase's actual code before acting: every DiscoveredModel.chat_base_url in contextual-orchestrator traces to one of a fixed set of hardcoded string literals (nvidia_nim/nvidia_nim_sub are byte-identical), and this repo's launcher copies that value verbatim, falling back only to zdr_policy.PROVIDER_BASE_URLS (confirmed byte-identical to the same literals). So this repo's one production caller (the sidecar/launcher) cannot produce inconsistent spellings today. It IS reachable through this script's own public --discovery-report CLI, which reads an arbitrary JSON file and isn't restricted to the launcher's exact generation path, and isn't wired into any current production workflow -- latent, not live, but real for that public surface. The fix is cheap and behavior-neutral on every input the sidecar produces today, so it's applied rather than left as an unstated assumption. _outage_domain now compares _normalize_base_url(row["base_url"]): lowercases scheme/host (case-insensitive per RFC 3986), drops an explicit port equal to the scheme's default, strips one trailing slash from the path. A different host, non-default port, path, or scheme still stays genuinely distinct. Falls back to a lowercased/stripped whole-string comparison (never raises) for anything unparseable into a scheme, host, and numeric port -- including a non-numeric port substring, which urlsplit(...).port raises ValueError on. Five new tests: the exact equivalent-spelling cases Devin named (case, default port, trailing slash), genuine distinctions still separate, no-raise on malformed/empty/bad-port input, and one end-to-end test through build_zdr_prioritized_catalog with two differently-spelled rows for the same endpoint (confirms the admission cap and diversity count both honor the normalization, not just the unit-level helper). Full suite: 2106 passed, 1 skipped, 21 subtests passed. 100% coverage (including the new fallback branch) and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 6 +- docs/product-technical-gap-baseline.md | 28 +++++ .../contextual_orchestrator_review_policy.py | 62 ++++++++++- ...t_contextual_orchestrator_review_policy.py | 104 ++++++++++++++++++ 4 files changed, 198 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c630ac5cd3..8533202b54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,11 @@ Semantic Versioning where the repository publishes a release. 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. + 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), so a formatting difference alone cannot split one physical + endpoint into two domains. - 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/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 463660b8e7..b8abb4cdaa 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1784,6 +1784,34 @@ 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/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 721093f302..5067f9d0e1 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -16,6 +16,7 @@ from collections import Counter 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, @@ -30,6 +31,8 @@ DEFAULT_CATALOG_LIMIT = 12 DEFAULT_ACCOUNT_CAP = 4 +_DEFAULT_PORTS: Mapping[str, int] = {"http": 80, "https": 443} + COST_FREE = "free" COST_PRICED = "priced" COST_UNKNOWN = "unknown" @@ -80,8 +83,65 @@ def _outage_domain(row: Mapping[str, Any]) -> str: 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 str(row["base_url"]) + 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 -- is preserved verbatim, including the query and + fragment components (routing evidence has no legitimate reason to carry + either; preserving rather than dropping them means an unexpected one + cannot silently vanish from the computed identity). 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) and a non-numeric port + substring (``urlsplit(...).port`` raises ``ValueError`` for one) -- + 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. + """ + text = base_url.strip() + parsed = urlsplit(text) + 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() + netloc = host if port is None or port == _DEFAULT_PORTS.get(scheme) else f"{host}:{port}" + path = parsed.path.rstrip("/") + return urlunsplit((scheme, netloc, path, parsed.query, parsed.fragment)) def _normalize_agent_id(candidate: str, provider_name: str) -> str: diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 95b7f31451..26acde8b6e 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -85,6 +85,68 @@ def test_outage_domain_groups_by_shared_base_url() -> None: ) != 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"), + ], +) +def test_normalize_base_url_treats_equivalent_spellings_as_one_domain( + base_url: str, equivalent_to: str +) -> None: + """Case, an explicit default port, and a trailing slash do not 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. + """ + 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"), + ], +) +def test_normalize_base_url_preserves_genuine_distinctions( + base_url: str, distinct_from: str +) -> None: + """A different host, non-default port, path, or scheme stays a different domain.""" + 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_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/"}) + + @pytest.mark.parametrize( ("candidate", "provider", "expected"), [ @@ -346,6 +408,48 @@ def test_build_catalog_counts_same_vendor_credentials_independently() -> None: 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_rejects_unknown_pool() -> None: """An unrecognized virtual pool cannot silently widen model admission.""" with pytest.raises(policy.PolicyError, match="unsupported review pool"): From ad3d2ce5d42824c15d132ee6140f86d6a46ff0ee Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:37:31 +0000 Subject: [PATCH 03/24] fix(ci): stop the shared outage-domain cap from starving one credential Two Devin Review findings on this PR, one severe. Severe: shared-cap starvation within a domain. Grouping the admission cap by outage domain (this PR's own earlier fix) correctly stopped nvidia_nim/nvidia_nim_sub from jointly consuming 2x the intended budget across domains -- but the admission loop still walked rows in one strict sorted (cost-tier, ZDR, provider, model) order and admitted greedily until a domain's cap was reached. Since "nvidia_nim" < "nvidia_nim_sub" in every real fixture, nvidia_nim's rows always sort first, so nvidia_nim alone could consume the ENTIRE shared cap before a single nvidia_nim_sub row was ever considered. Verified concretely: 6 free nvidia_nim rows + 6 free nvidia_nim_sub rows, account_cap=4 -> nvidia_nim_sub got zero rows. Not "prevented from taking more than its share" (the bug already fixed), but "the alphabetically-first credential takes the whole shared budget, the other gets nothing" -- the same crowding-out problem, now 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 (each domain's whole block stays at the position of its first row's original appearance, so domain-vs-domain ordering is unaffected), 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. 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: "Invalid IPv6 URL"), before any scheme/host is even available to inspect -- an uncaught exception past this function's own "must never raise" contract. Fixed by wrapping the urlsplit() call itself in the same catch-and-fall-back pattern already used for .port. Noted, not chased further (info-level, optional per this session's coordinator): hostname canonicalization stops at lowercasing -- a trailing root-label dot, IDN Unicode-vs-punycode forms, and differently-compressed IPv6 literals aren't 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. Tests: two existing tests whose assertions had encoded the starvation behavior were corrected to the fair-split expectation (test_build_catalog_applies_account_cap, test_build_catalog_prevents_shared_endpoint_from_crowding_out_independent_providers); added an 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 with domain-block position preserved); added a regression for the IPv6 urlsplit() crash. 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/. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 11 +- docs/product-technical-gap-baseline.md | 47 ++++++ .../contextual_orchestrator_review_policy.py | 104 ++++++++++++- ...t_contextual_orchestrator_review_policy.py | 141 ++++++++++++++++-- 4 files changed, 284 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8533202b54..322b6afcde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,8 +21,15 @@ Semantic Versioning where the repository publishes a release. 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), so a formatting difference alone cannot split one physical - endpoint into two domains. + 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). - 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/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index b8abb4cdaa..ce6699584a 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1812,6 +1812,53 @@ end-to-end test through `build_zdr_prioritized_catalog` itself with two differen 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/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 5067f9d0e1..5d40ae9bfb 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -123,14 +123,33 @@ def _normalize_base_url(base_url: str) -> str: 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) and a non-numeric port - substring (``urlsplit(...).port`` raises ``ValueError`` for one) -- - 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. + 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() - parsed = urlsplit(text) + try: + parsed = urlsplit(text) + except ValueError: + return text.casefold() if not parsed.scheme or not parsed.hostname: return text.casefold() try: @@ -144,6 +163,77 @@ def _normalize_base_url(base_url: str) -> str: return urlunsplit((scheme, netloc, path, parsed.query, parsed.fragment)) +def _fair_admission_order( + rows: list[Mapping[str, Any]], +) -> list[Mapping[str, Any]]: + """Reorder rows so one outage domain's cap fills fairly across accounts. + + ``rows`` must already be in the caller's priority order (cost-tier, ZDR + preference, deterministic ``(provider, model)`` tie-break -- see + ``build_zdr_prioritized_catalog``'s own sort). 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 this exact 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 is 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. + + 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 priority 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; 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. + + Each domain's whole reordered block is emitted at the position of its + first row's original appearance, so which domain is considered before + another is unaffected by this function -- only the order *within* a + multi-account domain changes. + """ + domain_order: list[str] = [] + domain_rows: dict[str, list[Mapping[str, Any]]] = {} + for row in rows: + domain = _outage_domain(row) + if domain not in domain_rows: + domain_order.append(domain) + domain_rows[domain] = [] + domain_rows[domain].append(row) + + ordered: list[Mapping[str, Any]] = [] + for domain in domain_order: + bucket = domain_rows[domain] + account_order: list[str] = [] + queues: dict[str, list[Mapping[str, Any]]] = {} + for row in bucket: + account = provider_account(str(row["provider"])) + if account not in queues: + account_order.append(account) + queues[account] = [] + queues[account].append(row) + if len(account_order) <= 1: + ordered.extend(bucket) + continue + while any(queues[account] for account in account_order): + for account in account_order: + queue = queues[account] + if queue: + ordered.append(queue.pop(0)) + 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() @@ -370,7 +460,7 @@ def build_zdr_prioritized_catalog( per_domain: Counter[str] = Counter() picked: list[Mapping[str, Any]] = [] - for row in eligible_rows: + for row in _fair_admission_order(eligible_rows): domain = _outage_domain(row) if per_domain[domain] >= account_cap: continue diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 26acde8b6e..21ec843036 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -140,6 +140,23 @@ def test_normalize_base_url_falls_back_on_unparseable_input() -> None: ) == 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_outage_domain_uses_normalized_base_url() -> None: """Two rows spelling one endpoint differently share one outage domain.""" assert policy._outage_domain( @@ -147,6 +164,74 @@ def test_outage_domain_uses_normalized_base_url() -> None: ) == policy._outage_domain({"base_url": "https://Integrate.API.Nvidia.com:443/v1/"}) +def _row(provider: str, model: str) -> 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], + } + + +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) == 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) + 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_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) + assert [(row["provider"], row["model"]) for row in ordered] == [ + ("bytez", "b0"), + ("nvidia_nim", "m0"), + ("nvidia_nim_sub", "s0"), + ("nvidia_nim", "m1"), + ("openrouter", "r0"), + ] + + @pytest.mark.parametrize( ("candidate", "provider", "expected"), [ @@ -474,7 +559,7 @@ def test_build_catalog_assigns_unique_priorities() -> None: def test_build_catalog_applies_account_cap() -> None: - """The admission cap is enforced per outage domain, not per credential. + """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 @@ -482,10 +567,12 @@ def test_build_catalog_applies_account_cap() -> None: 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). Sort order - (alphabetical among same-cost, same-ZDR rows) picks the two admitted - NVIDIA-domain rows from ``nvidia_nim`` specifically, since - ``"nvidia_nim" < "nvidia_nim_sub"``. + 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": [ @@ -514,7 +601,7 @@ 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, "openai": 2} + assert account_counts == {"nvidia_nim": 1, "nvidia_nim_sub": 1, "openai": 2} assert len(result["agents"]) == 4 @@ -556,14 +643,48 @@ def test_build_catalog_prevents_shared_endpoint_from_crowding_out_independent_pr 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 (all from nvidia_nim, - # sorted first) -- not 4 from each credential -- leaving bytez and - # openrouter, each an independent domain, fully admitted. - assert counts == {"bytez": 2, "nvidia_nim": 4, "openrouter": 2} + # 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 + + def test_build_catalog_respects_limit() -> None: """The catalog never exceeds the configured agent limit.""" report = { From 0c90bebf5db079b77d4c1e42f67a2dec06e4dfd5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:51:12 +0000 Subject: [PATCH 04/24] fix(ci): keep tier priority strict during fair admission, fix IPv6 collision Two Devin Review findings, one a real correctness regression the previous round's round-robin fix introduced. Real regression: fairness reordering could drop a free route for a paid one. _fair_admission_order grouped every row for one outage domain into a single 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 tiers (e.g. openai contributes both a free and a priced route, one single-account domain). Grouping by domain first, tier-blind, let a domain's lower-tier row get pulled into the same block as its higher-tier row, ahead of a different domain's higher-tier row that only sorted later because of the (provider, model) tie-break. Verified concretely: sorted [free openai, free openrouter, priced openai] reordered to [free openai, priced openai, free openrouter], and with limit=2 the genuinely free openrouter route got dropped for the priced openai one. 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 (sort key and tier-boundary detector can no longer drift apart), the input is split into contiguous same-tier runs (safe -- already tier-sorted), and the existing domain/account round-robin logic (renamed _fair_order_within_tier) applies independently to each run, then the runs concatenate back in original order. Re-verified both the tier-priority scenario and the earlier starvation-fix scenario pass together. 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 [::1]:8443 (host ::1, port 8443) and [::1:8443] (one IPv6 literal, no separate port) both normalized to the identical, syntactically-invalid ::1:8443. Fixed by re-wrapping a colon-bearing host in brackets before conditionally appending a port. Optional, applied since already in this code: round-robin queues switched from list.pop(0) (O(n)) to collections.deque.popleft() (O(1)). Tests: new unit-level and end-to-end regressions for tier-priority preservation; two new regressions for the IPv6 fix (distinct normalization, default-port-drop still works); existing _fair_admission_order tests updated for the now-required zdr_endpoints parameter. Full suite: 2115 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 11 +- docs/product-technical-gap-baseline.md | 42 +++++ .../contextual_orchestrator_review_policy.py | 173 ++++++++++++------ ...t_contextual_orchestrator_review_policy.py | 106 ++++++++++- 4 files changed, 274 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 322b6afcde..98ed6c233e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,16 @@ Semantic Versioning where the repository publishes a release. -- 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). + 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. - 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/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index ce6699584a..ffcea3947e 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1859,6 +1859,48 @@ entirely.** Two more Devin Review findings on this PR, one severe. 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/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 5d40ae9bfb..ce69ade1eb 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -13,7 +13,7 @@ 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 @@ -158,50 +158,128 @@ def _normalize_base_url(base_url: str) -> str: return text.casefold() scheme = parsed.scheme.casefold() host = parsed.hostname.casefold() - netloc = host if port is None or port == _DEFAULT_PORTS.get(scheme) else f"{host}:{port}" + # 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, parsed.fragment)) +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]], + 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 in the caller's priority order (cost-tier, ZDR - preference, deterministic ``(provider, model)`` tie-break -- see - ``build_zdr_prioritized_catalog``'s own sort). 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 this exact 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 is 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. - - 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 priority 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; 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. - - Each domain's whole reordered block is emitted at the position of its - first row's original appearance, so which domain is considered before - another is unaffected by this function -- only the order *within* a - multi-account domain changes. + ``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. + """ + 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. """ domain_order: list[str] = [] domain_rows: dict[str, list[Mapping[str, Any]]] = {} @@ -216,12 +294,12 @@ def _fair_admission_order( for domain in domain_order: bucket = domain_rows[domain] account_order: list[str] = [] - queues: dict[str, list[Mapping[str, Any]]] = {} + 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] = [] + queues[account] = deque() queues[account].append(row) if len(account_order) <= 1: ordered.extend(bucket) @@ -230,7 +308,7 @@ def _fair_admission_order( for account in account_order: queue = queues[account] if queue: - ordered.append(queue.pop(0)) + ordered.append(queue.popleft()) return ordered @@ -444,23 +522,12 @@ 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_domain: Counter[str] = Counter() picked: list[Mapping[str, Any]] = [] - for row in _fair_admission_order(eligible_rows): + for row in _fair_admission_order(eligible_rows, zdr_endpoints=zdr_endpoints): domain = _outage_domain(row) if per_domain[domain] >= account_cap: continue diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 21ec843036..be8d2e5c05 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -157,6 +157,32 @@ def test_normalize_base_url_falls_back_on_malformed_ipv6_bracket() -> None: ) +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( @@ -164,19 +190,22 @@ def test_outage_domain_uses_normalized_base_url() -> None: ) == policy._outage_domain({"base_url": "https://Integrate.API.Nvidia.com:443/v1/"}) -def _row(provider: str, model: str) -> dict[str, object]: +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) == rows + assert policy._fair_admission_order(rows, zdr_endpoints=frozenset()) == rows def test_fair_admission_order_round_robins_a_shared_domain() -> None: @@ -195,7 +224,7 @@ def test_fair_admission_order_round_robins_a_shared_domain() -> None: _row("nvidia_nim_sub", "s0"), _row("nvidia_nim_sub", "s1"), ] - ordered = policy._fair_admission_order(rows) + 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"), @@ -205,6 +234,75 @@ def test_fair_admission_order_round_robins_a_shared_domain() -> None: ] +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. @@ -222,7 +320,7 @@ def test_fair_admission_order_preserves_domain_position_and_multiple_domains() - _row("nvidia_nim_sub", "s0"), _row("openrouter", "r0"), ] - ordered = policy._fair_admission_order(rows) + ordered = policy._fair_admission_order(rows, zdr_endpoints=frozenset()) assert [(row["provider"], row["model"]) for row in ordered] == [ ("bytez", "b0"), ("nvidia_nim", "m0"), From ec6a1e5e37ed4acfd63f4a08fca667e9a4e58503 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:56:03 +0000 Subject: [PATCH 05/24] fix(ci): stop blanket-stripping OpenRouter rows on evidence_only alone scripts/ci/contextual_orchestrator_review_launcher.py's _routable_discovered_models() 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 is fetched and parsed for OpenRouter in that same module. The upstream half of this bug is being fixed separately (a dispatched agent, PR forthcoming) -- not touched here. Consequence for this repo: with 100% of OpenRouter rows carrying evidence_only=True, this filter excluded ALL OpenRouter 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) ever got a chance to evaluate a single one -- making that mechanism dead code for OpenRouter specifically, and leaving OpenRouter contributing zero routes to any pool despite genuinely offering ZDR-attested free models via its own documented feed. OpenRouter rows are now exempt from the evidence_only exclusion. 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()) -- this exemption relies on that existing, independent check, not on trusting evidence_only's current, wrong, blanket value for OpenRouter. Sequencing note, verified before writing this: this fix has real, immediate effect once merged, not only once contextual-orchestrator's own evidence_only fix and a matching ORCHESTRATOR_PIN_SHA bump also land. OpenRouter discovery already runs in this sidecar today, and for the general (non-private, require_zdr=False) pool -- what Noema/OpenCode/ default Strix 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 genuinely chat-capable OpenRouter rows start reaching selection as soon as this merges. What remains gated 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). Documented in the function's own docstring and this PR description. Tests: test_routable_discovered_models_excludes_evidence_only_rows (existing) corrected to use a non-OpenRouter provider for its evidence_only=True fixture; new test_routable_discovered_models_exempts_openrouter_from_evidence_only confirms both an evidence_only-tagged and untagged OpenRouter row pass through while a same-shaped row from a different provider does not; a contract-test assertion pins the exemption's presence in source. Full suite: 2093 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 12 +++++ docs/product-technical-gap-baseline.md | 52 +++++++++++++++++++ ...contextual_orchestrator_review_launcher.py | 51 +++++++++++++++--- ...l_orchestrator_review_runtime_preflight.py | 46 ++++++++++++++-- ...al_orchestrator_review_sidecar_contract.py | 4 ++ 5 files changed, 155 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 256b0cdf94..17b29e80a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ 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. - Fix a dangling reference #1468 left in `docs/product-goal-directive.md` (flagged by Devin Review on that PR): the standing operating directive still named the removed `free_family_diversity` evidence field instead of diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 758ef2961a..ef4c531335 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,58 @@ 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 `.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 is being fixed separately (a dispatched agent, PR forthcoming, not +touched here). + +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/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 6dbbe2d5e3..1578dd1cfc 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -141,14 +141,51 @@ def _log_discovery_errors(errors: list[object]) -> None: def _routable_discovered_models(discovered: list[object] | None) -> list[object]: """Drop evidence-only discovery rows before any live-serving selection. - Evidence-only rows (e.g. the OpenRouter catalog) exist solely to supply - ZDR evidence for other providers' models; contextual_orchestrator's own - ``agent_from_discovered()`` refuses to turn one into a serving agent. - Filtering here keeps that same invariant in this sidecar's selection path, - which builds its catalog independently rather than calling - ``agent_from_discovered()`` directly. + 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 deliberately exempt from this exclusion. + ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` + currently hardcodes ``evidence_only=True`` for every discovered model + unconditionally -- 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 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, and leaving OpenRouter contributing zero routes to any + pool, including the ZDR-attested routes it genuinely offers. 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()``) -- so this exemption relies on + that existing, independent check, not on trusting ``evidence_only``'s + current, wrong, blanket value for OpenRouter. + + 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 then, this + function's remaining protection against those is the same downstream + chat-capability check, not ``evidence_only``. """ - return [model for model in (discovered or []) if not getattr(model, "evidence_only", False)] + return [ + model + for model in (discovered or []) + if not getattr(model, "evidence_only", False) + or getattr(model, "provider_name", None) == "openrouter" + ] def _route_identity(model: object) -> tuple[str, str]: diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 32f1c22413..4a5c2e79e7 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,46 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: assert routable([]) == [] +def test_routable_discovered_models_exempts_openrouter_from_evidence_only() -> None: + """OpenRouter rows are never dropped on evidence_only alone. + + Regression for a confirmed bug: ``contextual-orchestrator``'s OpenRouter + ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for + every discovered model unconditionally (not computed per model from + real evidence), which -- if this filter applied to OpenRouter like + every other provider -- 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 an evidence-only-tagged and an untagged OpenRouter row must pass + through; a same-shaped row from a different provider must not. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + openrouter_evidence_only = SimpleNamespace( + id="openrouter_evidence_only", + provider_name="openrouter", + model_id="some/model", + evidence_only=True, + ) + openrouter_live = SimpleNamespace( + id="openrouter_ready", + provider_name="openrouter", + model_id="ready/free", + evidence_only=False, + ) + nvidia_evidence_only = SimpleNamespace( + id="nvidia_evidence_only", + provider_name="nvidia_nim", + model_id="some/model", + evidence_only=True, + ) + + assert routable( + [openrouter_evidence_only, openrouter_live, nvidia_evidence_only] + ) == [openrouter_evidence_only, openrouter_live] + + def test_log_discovery_errors_prints_one_bounded_line_per_provider_failure( capsys: pytest.CaptureFixture[str], ) -> None: diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index cbcf690d0b..db1005c99e 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -284,6 +284,10 @@ 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. + 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 From 4adb60ee9eb520c0fb2cd0c345c5950e5bb63a3e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 05:11:44 +0000 Subject: [PATCH 06/24] fix(ci): make the OpenRouter evidence_only exemption self-correcting Devin Review on PR #1476 flagged that the blanket OpenRouter exemption in _routable_discovered_models() doesn't distinguish "vendored contextual-orchestrator still has the confirmed blanket evidence_only=True bug" from "vendored copy now computes evidence_only correctly per model" (the fix in contextual-orchestrator#950, open, not yet merged) -- so once #950 merges and ORCHESTRATOR_PIN_SHA bumps past it, this launcher would keep admitting genuinely evidence-only OpenRouter rows the corrected upstream code means to exclude. Considered gating on ORCHESTRATOR_PIN_SHA via git ancestry (merge-base --is-ancestor against #950's eventual merge commit, reachable from the sidecar's already-full vendored clone) but #950 has no merge commit yet, so there is no concrete threshold to gate on, and wiring the plumbing now (new CLI arg/env var, subprocess git call, sidecar/contract changes) would be built against a value that doesn't exist. Implemented instead: _openrouter_reports_per_model_evidence() reads this run's own discovered OpenRouter rows and turns the exemption off the moment any row reports evidence_only=False (real per-model evidence). While every row still reports True (today's exact bug signature), the exemption stays active. Self-corrects with no pin tracking and no manual conversion step once #950 merges. Known, accepted limitation documented in the docstring: a genuinely-fixed vendored copy that happens to report all-True in one run (feed-fetch failure, or zero attested models that run) is indistinguishable from the still-buggy signature by this check alone. Tests: split the previous mixed-fixture regression into test_routable_discovered_models_exempts_openrouter_when_every_row_reports_evidence_only (pre-fix blanket-True shape) and test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence (post-fix mixed shape). Full suite: 2094 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Follow-up recorded in docs/product-technical-gap-baseline.md with an explicit TODO referencing contextual-orchestrator#950 and this repo's #1476. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 64 +++++++++ ...contextual_orchestrator_review_launcher.py | 130 +++++++++++++++--- ...l_orchestrator_review_runtime_preflight.py | 76 +++++++--- 3 files changed, 234 insertions(+), 36 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index ef4c531335..1d8ea00b9a 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1767,6 +1767,70 @@ orchestrator_review_sidecar_contract.py` pinning the exemption's presence in sou 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 proposed in +`ContextualWisdomLab/contextual-orchestrator#950`, **open, not yet merged** as of this entry). Left +unconditional forever, this launcher would keep admitting genuinely evidence-only (non-ZDR-attested) +OpenRouter rows even after `#950` merges and `ORCHESTRATOR_PIN_SHA` is bumped past it -- silently defeating +the very fix `#950` delivers, for exactly the population of rows `evidence_only` exists to gate. + +**Design considered: pin-SHA ancestry.** `#950`'s base SHA (`c107e3e52371993aa9c326fcc245e01c41fc3850`) +is confirmed to equal this repo's current `ORCHESTRATOR_PIN_SHA` default +(`scripts/ci/contextual_orchestrator_review_sidecar.sh`), so once `#950` merges its resulting SHA on +`contextual-orchestrator`'s `main` becomes 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 now: `#950` has not merged, so no concrete +fix-commit SHA exists yet to gate on, and wiring the check in ahead of that would require 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 does not yet have -- over-engineering ahead of the actual need. + +**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 `#950` 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. + +**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 (`#950`'s own documented fail-closed behavior), 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. + +**TODO, tracked here explicitly (`ContextualWisdomLab/contextual-orchestrator#950`, +`ContextualWisdomLab/.github#1476`):** once `#950` merges, re-verify this reasoning holds under its actual +merged test suite (the negative case `test_discover_all_models_openrouter_model_absent_from_zdr_feed_stays_ +evidence_only` and the feed-failure case `test_discover_all_models_openrouter_zdr_feed_failure_keeps_every_ +row_evidence_only`, per `#950`'s own description) and once `ORCHESTRATOR_PIN_SHA` is bumped past it, confirm +in a real CI run that `_openrouter_reports_per_model_evidence()` observes the expected per-model variation +and the exemption turns itself off with no code change required. 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 here, now that a concrete +fix-commit SHA would exist 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/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 1578dd1cfc..e1217e2b80 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -138,6 +138,90 @@ 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`` + currently has a confirmed bug (``ContextualWisdomLab/contextual- + orchestrator#950``, open, not yet merged 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. #950 fixes this by computing + ``evidence_only``/``zdr_capable`` per model instead, from OpenRouter's + own ``/api/v1/endpoints/zdr`` feed match -- so a fixed vendored copy + reports ``evidence_only=False`` for at least the subset of OpenRouter + models that are genuinely ZDR-attested (whenever OpenRouter offers any + such models, which it does today), while unattested models correctly + stay ``True``. + + 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 <950's merge commit> + "$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 #950 has not + merged yet, so no fix commit SHA exists to compare against, and wiring + one in now would need 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 does 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#950`` (the upstream fix) + for the full history; once #950 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 at least one + ZDR-attested free model in a given run. + + KNOWN, ACCEPTED LIMITATION: a genuinely-fixed vendored copy that + happens to report ``evidence_only=True`` for *every* OpenRouter row in + one particular run -- because the ZDR feed fetch failed entirely that + run (#950's own documented fail-closed behavior), or because none of + the OpenRouter models discovered that run happen to be ZDR-attested -- + 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()``) that guards private, ``--require-zdr`` + targets. + + 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 fixed-but-all-unattested run). + """ + 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. @@ -148,25 +232,31 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] in this sidecar's selection path, which builds its catalog independently rather than calling ``agent_from_discovered()`` directly. - OpenRouter rows are deliberately exempt from this exclusion. - ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` - currently hardcodes ``evidence_only=True`` for every discovered model - unconditionally -- 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 would strip every OpenRouter row, including genuinely + 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, ``ContextualWisdomLab/contextual-orchestrator#950``, not + yet merged) -- 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, and leaving OpenRouter contributing zero routes to any - pool, including the ZDR-attested routes it genuinely offers. A + 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()``) -- so this exemption relies on - that existing, independent check, not on trusting ``evidence_only``'s - current, wrong, blanket value for OpenRouter. + ``_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`` @@ -176,15 +266,21 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] ``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 then, this - function's remaining protection against those is the same downstream - chat-capability check, not ``evidence_only``. + real per-model basis (e.g. a non-chat listing); until #950 merges 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``. """ + discovered = list(discovered or []) + openrouter_still_blanket_marked = not _openrouter_reports_per_model_evidence(discovered) return [ model - for model in (discovered or []) + for model in discovered if not getattr(model, "evidence_only", False) - or getattr(model, "provider_name", None) == "openrouter" + or ( + openrouter_still_blanket_marked + and getattr(model, "provider_name", None) == "openrouter" + ) ] diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 4a5c2e79e7..68bf873d46 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -112,33 +112,38 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: assert routable([]) == [] -def test_routable_discovered_models_exempts_openrouter_from_evidence_only() -> None: - """OpenRouter rows are never dropped on evidence_only alone. - - Regression for a confirmed bug: ``contextual-orchestrator``'s OpenRouter - ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for - every discovered model unconditionally (not computed per model from - real evidence), which -- if this filter applied to OpenRouter like - every other provider -- 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 an evidence-only-tagged and an untagged OpenRouter row must pass - through; a same-shaped row from a different provider must not. +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 (``ContextualWisdomLab/contextual- + orchestrator#950``, open, not yet merged): ``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 = SimpleNamespace( - id="openrouter_evidence_only", + openrouter_evidence_only_a = SimpleNamespace( + id="openrouter_evidence_only_a", provider_name="openrouter", model_id="some/model", evidence_only=True, ) - openrouter_live = SimpleNamespace( - id="openrouter_ready", + openrouter_evidence_only_b = SimpleNamespace( + id="openrouter_evidence_only_b", provider_name="openrouter", model_id="ready/free", - evidence_only=False, + evidence_only=True, ) nvidia_evidence_only = SimpleNamespace( id="nvidia_evidence_only", @@ -148,8 +153,41 @@ def test_routable_discovered_models_exempts_openrouter_from_evidence_only() -> N ) assert routable( - [openrouter_evidence_only, openrouter_live, nvidia_evidence_only] - ) == [openrouter_evidence_only, openrouter_live] + [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/contextual-orchestrator#950`` merges and the + vendored pin advances past it, 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_log_discovery_errors_prints_one_bounded_line_per_provider_failure( From e27f7c29825ade8e788dee0bde245097fd8a868f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 05:13:21 +0000 Subject: [PATCH 07/24] fix(ci): preserve cross-domain priority positions, ignore URL fragments in outage-domain key Round-5 Devin Review found two real bugs left over in the outage-domain fairness logic: - _fair_order_within_tier collapsed every domain to one contiguous block positioned at that domain's first appearance, which silently displaced an unrelated domain's row whenever a shared domain's own rows were not already contiguous in priority order (e.g. [A1, B1, A2] became [A1, A2, B1], dropping B1 under a tight limit even though it outranked A2). Fixed by recording each domain's own global positions up front and only reordering which of a domain's own rows fills its own positions, never touching another domain's slot. - _normalize_base_url folded query and fragment into the outage-domain key together, so two identical endpoints differing only by a client-side-only #fragment reported as two domains with separate diversity counts and separate admission-cap budgets. The fragment is now stripped while the query string, which can be a real routing distinction, is still preserved. Both are covered by new regression tests exercising the internal reordering helper directly and the full build_zdr_prioritized_catalog path with a tight limit. 100% coverage/docstring gates re-verified. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- .../contextual_orchestrator_review_policy.py | 76 ++++++--- ...t_contextual_orchestrator_review_policy.py | 147 +++++++++++++++++- 2 files changed, 197 insertions(+), 26 deletions(-) diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index ce69ade1eb..fa100090f0 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -111,15 +111,21 @@ def _normalize_base_url(base_url: str) -> str: 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 -- is preserved verbatim, including the query and - fragment components (routing evidence has no legitimate reason to carry - either; preserving rather than dropping them means an unexpected one - cannot silently vanish from the computed identity). 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). + 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 @@ -173,7 +179,7 @@ def _normalize_base_url(base_url: str) -> str: else f"{bracketed_host}:{port}" ) path = parsed.path.rstrip("/") - return urlunsplit((scheme, netloc, path, parsed.query, parsed.fragment)) + return urlunsplit((scheme, netloc, path, parsed.query, "")) def _admission_priority_key( @@ -252,7 +258,15 @@ def _fair_admission_order( 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. + 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 @@ -280,19 +294,31 @@ def _fair_order_within_tier( 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_order: list[str] = [] - domain_rows: dict[str, list[Mapping[str, Any]]] = {} - for row in rows: - domain = _outage_domain(row) - if domain not in domain_rows: - domain_order.append(domain) - domain_rows[domain] = [] - domain_rows[domain].append(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]] = [] - for domain in domain_order: - bucket = domain_rows[domain] + 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: @@ -302,13 +328,15 @@ def _fair_order_within_tier( queues[account] = deque() queues[account].append(row) if len(account_order) <= 1: - ordered.extend(bucket) 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: - ordered.append(queue.popleft()) + reordered.append(queue.popleft()) + for index, row in zip(positions, reordered): + ordered[index] = row return ordered diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index be8d2e5c05..36955457f0 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -92,12 +92,17 @@ def test_outage_domain_groups_by_shared_base_url() -> None: ("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, and a trailing slash do not split a domain. + """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 @@ -106,6 +111,12 @@ def test_normalize_base_url_treats_equivalent_spellings_as_one_domain( 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) @@ -117,12 +128,18 @@ def test_normalize_base_url_treats_equivalent_spellings_as_one_domain( ("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, or scheme stays a different domain.""" + """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) @@ -330,6 +347,89 @@ def test_fair_admission_order_preserves_domain_position_and_multiple_domains() - ] +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"), [ @@ -633,6 +733,49 @@ def test_build_catalog_collapses_differently_spelled_equivalent_endpoints() -> N 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: """An unrecognized virtual pool cannot silently widen model admission.""" with pytest.raises(policy.PolicyError, match="unsupported review pool"): From a5f2b29057c0de4f6d8492c698e2205debfcde7e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 05:32:31 +0000 Subject: [PATCH 08/24] fix(ci): reconcile #950->#949, gate spend_admitted, close ZDR-bypass finding contextual-orchestrator#950 (cited by this PR as "the upstream half of this bug, not yet merged") was closed as redundant/superseded. The fix actually merged as contextual-orchestrator#949 ("fix(discovery): route OpenRouter by model evidence", 8cd99f139915131ba0239bce12a5d6a5fd85394e); .github#1477 already advances ORCHESTRATOR_PIN_SHA to that commit. Corrects every stale #950 reference in the PR's own docstrings and docs/product-technical-gap- baseline.md, and folds in two things only visible from #949's actual diff: - #949 also added DiscoveredModel.spend_admitted (default True, False for a priced OpenRouter row when openrouter_paid_inference_available() cannot confirm usable credit). orchestrator/free never considers priced rows, so it was never exposed to this, but orchestrator/auto (real, reachable via CONTEXTUAL_ORCHESTRATOR_POOL=auto, no other code change needed) does consider priced rows and had no spend_admitted check anywhere in this repo's pipeline. _routable_discovered_models() now excludes a spend_admitted=False row the same way it excludes evidence_only=True, with regression coverage including an end-to-end auto-pool composition. - A fresh Devin Review red finding on this PR argued the OpenRouter evidence_only exemption could let private/--require-zdr review content reach ZDR-forbidden routes. Traced end to end: build_zdr_prioritized_ catalog() independently re-applies is_zdr_model()'s real OpenRouter ZDR- feed check as its own admission gate whenever require_zdr=True, entirely independent of evidence_only. Confirmed false alarm with a regression test proving a non-ZDR-attested OpenRouter row is excluded from a require_zdr=True catalog even while every discovered OpenRouter row still carries the evidence_only=True bug signature; documented in the gap baseline and replied/resolved on the GitHub review thread. docs/product-technical-gap-baseline.md gets a new 2026-08-31 correction subsection recording all of the above. Full suite: 2098 passed, 1 skipped, 21 subtests passed; 100% coverage on scripts/ci/; 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 186 +++++++++++---- ...contextual_orchestrator_review_launcher.py | 142 +++++++----- ...l_orchestrator_review_runtime_preflight.py | 215 +++++++++++++++++- 3 files changed, 448 insertions(+), 95 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 1d8ea00b9a..5c549fc49d 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1723,8 +1723,9 @@ row with `evidence_only=True`. `contextual-orchestrator`'s OpenRouter `ProviderM `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 is being fixed separately (a dispatched agent, PR forthcoming, not -touched here). +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 @@ -1771,23 +1772,31 @@ skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on `scrip **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 proposed in -`ContextualWisdomLab/contextual-orchestrator#950`, **open, not yet merged** as of this entry). Left -unconditional forever, this launcher would keep admitting genuinely evidence-only (non-ZDR-attested) -OpenRouter rows even after `#950` merges and `ORCHESTRATOR_PIN_SHA` is bumped past it -- silently defeating -the very fix `#950` delivers, for exactly the population of rows `evidence_only` exists to gate. - -**Design considered: pin-SHA ancestry.** `#950`'s base SHA (`c107e3e52371993aa9c326fcc245e01c41fc3850`) -is confirmed to equal this repo's current `ORCHESTRATOR_PIN_SHA` default -(`scripts/ci/contextual_orchestrator_review_sidecar.sh`), so once `#950` merges its resulting SHA on -`contextual-orchestrator`'s `main` becomes 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 now: `#950` has not merged, so no concrete -fix-commit SHA exists yet to gate on, and wiring the check in ahead of that would require 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 does not yet have -- over-engineering ahead of the actual need. +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 @@ -1796,31 +1805,42 @@ per-model evidence, and `_routable_discovered_models()` immediately stops exempt 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 `#950` merges: the check +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. +`_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 (`#950`'s own documented fail-closed behavior), 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. - -**TODO, tracked here explicitly (`ContextualWisdomLab/contextual-orchestrator#950`, -`ContextualWisdomLab/.github#1476`):** once `#950` merges, re-verify this reasoning holds under its actual -merged test suite (the negative case `test_discover_all_models_openrouter_model_absent_from_zdr_feed_stays_ -evidence_only` and the feed-failure case `test_discover_all_models_openrouter_zdr_feed_failure_keeps_every_ -row_evidence_only`, per `#950`'s own description) and once `ORCHESTRATOR_PIN_SHA` is bumped past it, confirm -in a real CI run that `_openrouter_reports_per_model_evidence()` observes the expected per-model variation -and the exemption turns itself off with no code change required. 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 here, now that a concrete -fix-commit SHA would exist to gate on. +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_ @@ -1831,6 +1851,96 @@ Full suite: 2094 passed, 1 skipped, 21 subtests passed. 100% coverage (the launc 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. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index e1217e2b80..90a0419984 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -141,66 +141,75 @@ def _log_discovery_errors(errors: list[object]) -> None: 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`` - currently has a confirmed bug (``ContextualWisdomLab/contextual- - orchestrator#950``, open, not yet merged as of this writing): it + ``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. #950 fixes this by computing - ``evidence_only``/``zdr_capable`` per model instead, from OpenRouter's - own ``/api/v1/endpoints/zdr`` feed match -- so a fixed vendored copy - reports ``evidence_only=False`` for at least the subset of OpenRouter - models that are genuinely ZDR-attested (whenever OpenRouter offers any - such models, which it does today), while unattested models correctly - stay ``True``. + 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 <950's merge commit> + 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 #950 has not - merged yet, so no fix commit SHA exists to compare against, and wiring - one in now would need 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 does 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#950`` (the upstream fix) - for the full history; once #950 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 at least one - ZDR-attested free model in a given run. + 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 -- because the ZDR feed fetch failed entirely that - run (#950's own documented fail-closed behavior), or because none of - the OpenRouter models discovered that run happen to be ZDR-attested -- - 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()``) that guards private, ``--require-zdr`` - targets. + 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 @@ -213,7 +222,8 @@ def _openrouter_reports_per_model_evidence(discovered: list[object]) -> bool: ``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 fixed-but-all-unattested run). + signature, or an indistinguishable run where OpenRouter discovery + itself produced no rows). """ return any( getattr(model, "provider_name", None) == "openrouter" @@ -237,8 +247,11 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] and its documented limitation. In short: ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for every discovered model unconditionally (a - confirmed bug, ``ContextualWisdomLab/contextual-orchestrator#950``, not - yet merged) -- not computed per model from real evidence, even though + 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 @@ -266,21 +279,44 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] ``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 #950 merges and - a run observes real per-model variation, this function's remaining + 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. """ 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" + 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 ] diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 68bf873d46..48dca875a0 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -115,8 +115,10 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: 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 (``ContextualWisdomLab/contextual- - orchestrator#950``, open, not yet merged): ``contextual-orchestrator``'s + 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 @@ -160,8 +162,8 @@ def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_ev 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/contextual-orchestrator#950`` merges and the - vendored pin advances past it, OpenRouter starts reporting real + 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 @@ -190,6 +192,211 @@ def test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_ 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: From 95406d2c740bbc10b5f25721c30e8ad67fe4ef3a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 08:27:05 +0000 Subject: [PATCH 09/24] test(ci): pin the OpenRouter evidence_only exemption's real boolean outcome Devin Review (discussion r3891875665) on tests/test_contextual_orchestrator_ review_sidecar_contract.py:290 correctly noted the contract test's source check only requires the OpenRouter provider_name comparison to appear somewhere in source text -- a reversed (`!=` for `==`) or disconnected exemption would still satisfy that check. Verified by mutation: both mutations were applied locally and confirmed the old assertion alone would not have caught them (a "disconnect" mutation was crafted to leave the literal source fragment byte-for-byte intact while making the exemption a no-op). Add a direct behavioral assertion in the same test, against the already runpy-loaded launcher module's real `_routable_discovered_models`, that exercises one OpenRouter row that must be exempted and one same-shaped non-OpenRouter row that must not, asserting the actual filtered output. This fails under both the reversal and the disconnection mutation, closing the gap Devin identified without weakening the existing source-text checks. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- ...al_orchestrator_review_sidecar_contract.py | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 4602c13659..92a01bf5ee 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -286,7 +286,15 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: 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. + # 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 @@ -300,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", From 9b74577b34192ec34584628a05c0fbaa4d0409c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 09:18:18 +0000 Subject: [PATCH 10/24] fix(ci): bound required-workflow-bootstrap awk extraction to its own job Ports the identical fix from #1506 into this branch. This PR's exact-head-path-policy check runs its own head-branch copy of scripts/ci/test_strix_quick_gate.sh (plain `pull_request` trigger in strix-changed-path-quality-ci.yml, not pull_request_target), so the pre-existing main-branch bug is not fixed here just by #1506 merging into main -- it needs porting into this branch directly. Root cause: assert_opencode_review_uses_codegraph_and_contextual_orchestrator extracted the required-workflow-bootstrap job block from opencode-review.yml with awk '/^ required-workflow-bootstrap:$/,/^[^ ]/'. Every job key in that workflow is indented 2 spaces (never column 0), so the end pattern never matched until EOF, sweeping an unrelated `if:` line from a later job (added by already-merged PR #1497) into the "block" and failing the assertion on unrelated content. Fixed by using an explicit state flag so the end pattern (`^ [A-Za-z0-9_-]+:`) is only tested starting on the line after the start match, correctly bounding the block to just its own lines. See ContextualWisdomLab/.github#1506 for the full root-cause writeup and validation against origin/main. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- scripts/ci/test_strix_quick_gate.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 4053f4fd53..1fc45a34b9 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -522,7 +522,7 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_not_contains "$workflow_file" "Wait for trusted OpenCode approval review" "opencode pull_request bridge was removed to avoid duplicate required-check resource use" assert_file_not_contains "$workflow_file" "Trusted OpenCode requested changes for head" "opencode pull_request bridge no longer reconsumes stale trusted review state" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "opencode review workflow must not hard-code repository-specific PR bypasses" - if awk '/^ required-workflow-bootstrap:$/,/^[^ ]/' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then + if awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then record_failure "opencode required workflow bootstrap must not depend on required-workflow event payload fields" fi assert_file_contains "$workflow_file" 'github.event.client_payload.target_repository || github.repository' "opencode review scopes concurrency by target repository" From 7d693b86776de4e814b096f052c1f564e99505f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 10:53:59 +0000 Subject: [PATCH 11/24] fix(ci): remove grep -q from test_strix_quick_gate.sh pipeline checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit grep -q exits on first match and closes its end of the pipe; if the upstream awk is still writing a large block, it gets SIGPIPE (141). Under `set -o pipefail` that non-zero awk status wins over grep's real 0, so `if pipeline; then` sees the pipeline as failed even though grep found a genuine match — silently missing e.g. a forbidden `if:` key or a fenced-diff marker that should have failed the check. Ports the same-file fix from PR #1506 to this branch's two call sites (required-workflow-bootstrap job-block check; opencode review REQUEST_CHANGES fenced-diff check). This branch already carried #1506's awk job-block-boundary correction, so only the grep -q removal was needed here. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- scripts/ci/test_strix_quick_gate.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 1fc45a34b9..a92871b7ce 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -522,7 +522,7 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_not_contains "$workflow_file" "Wait for trusted OpenCode approval review" "opencode pull_request bridge was removed to avoid duplicate required-check resource use" assert_file_not_contains "$workflow_file" "Trusted OpenCode requested changes for head" "opencode pull_request bridge no longer reconsumes stale trusted review state" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "opencode review workflow must not hard-code repository-specific PR bypasses" - if awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then + if awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | grep '^[[:space:]]*if:' >/dev/null; then record_failure "opencode required workflow bootstrap must not depend on required-workflow event payload fields" fi assert_file_contains "$workflow_file" 'github.event.client_payload.target_repository || github.repository' "opencode review scopes concurrency by target repository" @@ -1501,7 +1501,7 @@ assert_opencode_review_posts_suggested_diffs_inline() { assert_file_contains "$workflow_file" "publish_request_changes_from_control" "opencode review REQUEST_CHANGES path publishes findings from the control JSON" if awk '/format_request_changes_body\(\)/,/build_request_changes_review_payload\(\)/ { print }' "$workflow_file" | - grep -Fq '```diff'; then + grep -F '```diff' >/dev/null; then record_failure "opencode review PR-level REQUEST_CHANGES body must not contain fenced suggested diffs" fi } From fb084b6249043026021b4d8897e8f8054b3c2138 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:08:31 +0000 Subject: [PATCH 12/24] fix(ci): bound required-workflow-bootstrap awk extraction to its own job Ports the identical fix from #1506 into this branch. This PR's exact-head-path-policy check runs its own head-branch copy of scripts/ci/test_strix_quick_gate.sh (plain `pull_request` trigger in strix-changed-path-quality-ci.yml, not pull_request_target), so the pre-existing main-branch bug is not fixed here just by #1506 merging into main -- it needs porting into this branch directly. Root cause: assert_opencode_review_uses_codegraph_and_contextual_orchestrator extracted the required-workflow-bootstrap job block from opencode-review.yml with awk '/^ required-workflow-bootstrap:$/,/^[^ ]/'. Every job key in that workflow is indented 2 spaces (never column 0), so the end pattern never matched until EOF, sweeping an unrelated `if:` line from a later job into the "block" and failing the assertion on unrelated content. Fixed by using an explicit state flag so the end pattern (`^ [A-Za-z0-9_-]+:`) is only tested starting on the line after the start match, correctly bounding the block to just its own lines. Confirmed FAIL before this fix, PASS after (bash scripts/ci/test_strix_quick_gate.sh). See ContextualWisdomLab/.github#1506 for the full root-cause writeup. Co-Authored-By: Claude --- scripts/ci/test_strix_quick_gate.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 4053f4fd53..1fc45a34b9 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -522,7 +522,7 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_not_contains "$workflow_file" "Wait for trusted OpenCode approval review" "opencode pull_request bridge was removed to avoid duplicate required-check resource use" assert_file_not_contains "$workflow_file" "Trusted OpenCode requested changes for head" "opencode pull_request bridge no longer reconsumes stale trusted review state" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "opencode review workflow must not hard-code repository-specific PR bypasses" - if awk '/^ required-workflow-bootstrap:$/,/^[^ ]/' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then + if awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then record_failure "opencode required workflow bootstrap must not depend on required-workflow event payload fields" fi assert_file_contains "$workflow_file" 'github.event.client_payload.target_repository || github.repository' "opencode review scopes concurrency by target repository" From 262a41cfe2f16af5967105f475f25a87d5a748a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:08:31 +0000 Subject: [PATCH 13/24] fix(ci): keep the priced-fallback catalog stage domain-diverse Devin Review finding on this PR's pushed head: with both defaults at 4 (ORCHESTRATOR_CATALOG_LIMIT - primary_count leaves a 4-route fallback budget, and DEFAULT_ACCOUNT_CAP is 4), a single outage domain with at least 4 priced rows exhausted the whole priced-fallback stage before build_zdr_prioritized_catalog's greedy admission loop ever reached a different, genuinely independent domain's row. The per-domain cap this PR added provides no diversity protection for this specific stage precisely because it coincidentally equals the stage's own overall route limit -- a gap this PR's own primary-stage fix does not have, since ORCHESTRATOR_CATALOG_LIMIT (12) comfortably exceeds account_cap (4) there. Adds _fallback_domain_aware_account_cap(): when more than one outage domain is competing for the fallback stage's rows, shrinks the configured cap to fallback_limit // domain_count (floor, minimum 1) so every domain gets at least one turn before any domain can claim a second admission -- cap * domain_count <= fallback_limit means the stage's own overall route limit can never cut admission off before every domain with an eligible row has already contributed one. The single-domain case (the common one) is unchanged: this returns exactly min(configured_cap, fallback_limit), the same value the unmodified cap already produced. Wired into main()'s priced-fallback build_zdr_prioritized_catalog call site; updates the source-level drift-prevention contract test (test_main_sources_the_account_cap_default_from_policy_not_a_magic_number) to match, since the fallback call site no longer spells account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP) directly -- it still sources the same single default, just through the new helper's configured_cap parameter. Three new regression tests, including Devin's own suggested shape (four same-domain priced routes plus one independent priced route). Co-Authored-By: Claude --- ...contextual_orchestrator_review_launcher.py | 71 +++++++++++++- ...l_orchestrator_review_runtime_preflight.py | 93 ++++++++++++++++++- 2 files changed, 162 insertions(+), 2 deletions(-) diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 7618f9dd09..c21360fb22 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -695,6 +695,70 @@ def _catalog_account_cap(default: int) -> int: return int(os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", str(default))) +def _fallback_domain_aware_account_cap( + rows: list[dict[str, Any]], + *, + fallback_limit: int, + configured_cap: int, + outage_domain: Any, +) -> int: + """Return the priced-fallback stage's per-domain admission cap. + + Devin Review finding on `.github#1474` (verified directly, not trusted + from the finding text alone): with both defaults at 4 + (``fallback_limit = ORCHESTRATOR_CATALOG_LIMIT - primary_count``, and + ``account_cap = DEFAULT_ACCOUNT_CAP``), a single outage domain with at + least ``fallback_limit`` priced rows exhausts the whole fallback + catalog before ``build_zdr_prioritized_catalog``'s greedy admission + loop ever reaches a different, genuinely independent domain's row -- + the per-domain cap provides no diversity protection for this + specific stage precisely because it coincidentally equals the stage's + own overall route limit. + + This does not affect the *primary* catalog stage: there, + ``ORCHESTRATOR_CATALOG_LIMIT`` (12 by default) comfortably exceeds + ``account_cap`` (4), so several domains are always structurally able to + contribute before the primary limit is reached. + + Shrinking the configured cap to ``fallback_limit // domain_count`` + (floor, minimum 1) whenever more than one domain is actually competing + for this stage's rows guarantees every domain gets at least one turn + before any domain can claim a second: with ``domain_count`` domains + each capped at ``cap = fallback_limit // domain_count``, the greedy + loop's own ``len(picked) >= limit`` cutoff (``cap * domain_count <= + fallback_limit``) can never trigger before every domain with at least + one admissible row has already contributed one. When only one domain is + present, this returns exactly ``min(configured_cap, fallback_limit)`` -- + the same value the unmodified cap already produced, so the common, + already-tested single-domain-fallback case is unchanged. + + Args: + rows: The priced rows eligible for this fallback stage (before + ``build_zdr_prioritized_catalog``'s own cost/ZDR/limit + filtering -- domain membership does not depend on that). + fallback_limit: The stage's own overall route budget, from + :func:`_bounded_fallback_catalog_limit`. + configured_cap: The operator-configured per-domain cap, from + :func:`_catalog_account_cap`. + outage_domain: ``contextual_orchestrator_review_policy._outage_domain``, + injected so this module never imports the policy module's + private helper at module scope (matching this file's existing + dependency-injection convention for ``outage_domain``/ + ``provider_account``, e.g. in :func:`_with_discovery_counts`). + + Returns: + The per-domain cap to pass to this stage's + ``build_zdr_prioritized_catalog`` call as ``account_cap``. + """ + if fallback_limit < 1: + return configured_cap + domain_count = len({outage_domain(row) for row in rows}) + if domain_count <= 1: + return min(configured_cap, fallback_limit) + fair_share_cap = max(1, fallback_limit // domain_count) + return min(configured_cap, fair_share_cap) + + def _with_discovery_counts( report: dict[str, object], rows: list[dict[str, Any]], @@ -923,7 +987,12 @@ def main(argv: list[str] | None = None) -> int: fallback_result = build_zdr_prioritized_catalog( admitted_priced_rows, limit=fallback_limit, - account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), + account_cap=_fallback_domain_aware_account_cap( + admitted_priced_rows, + fallback_limit=fallback_limit, + configured_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), + outage_domain=_outage_domain, + ), zdr_endpoints=zdr_endpoints, require_zdr=args.require_zdr, pool="auto", diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 753e15018e..f19e9094a7 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1488,6 +1488,90 @@ def test_catalog_account_cap_honors_an_explicit_override( assert namespace["_catalog_account_cap"](policy.DEFAULT_ACCOUNT_CAP) == 6 +def test_fallback_domain_aware_account_cap_leaves_the_single_domain_case_unchanged() -> None: + """One priced domain still gets ``min(configured_cap, fallback_limit)``.""" + namespace = _load_launcher() + fallback_cap = namespace["_fallback_domain_aware_account_cap"] + rows = [{"provider": "openrouter", "base_url": "https://openrouter.ai/api/v1"}] * 6 + assert ( + fallback_cap( + rows, + fallback_limit=4, + configured_cap=4, + outage_domain=policy._outage_domain, + ) + == 4 + ) + + +def test_fallback_domain_aware_account_cap_shrinks_for_competing_domains() -> None: + """Regression for Devin Review's "fallback remains single-domain" finding on `.github#1474`. + + With both defaults at 4 (``fallback_limit == configured_cap``), one + outage domain with at least ``fallback_limit`` priced rows used to + exhaust the whole priced-fallback 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. + """ + namespace = _load_launcher() + fallback_cap = namespace["_fallback_domain_aware_account_cap"] + dominant_domain_rows = [ + {"provider": "nvidia_nim", "base_url": "https://integrate.api.nvidia.com/v1"} + ] * 4 + independent_domain_rows = [ + {"provider": "openrouter", "base_url": "https://openrouter.ai/api/v1"} + ] + cap = fallback_cap( + [*dominant_domain_rows, *independent_domain_rows], + fallback_limit=4, + configured_cap=4, + outage_domain=policy._outage_domain, + ) + assert cap < 4 + assert cap * 2 <= 4 + + +def test_fallback_domain_aware_account_cap_keeps_both_domains_admitted_end_to_end() -> None: + """The computed cap, fed back into ``build_zdr_prioritized_catalog``, admits both domains. + + Exercises the fix at the same boundary the priced-fallback call site in + ``main()`` actually uses: compute the domain-aware cap from the + candidate rows, then build the catalog with it, exactly as + ``main()``'s own ``fallback_result = build_zdr_prioritized_catalog(..., + account_cap=_fallback_domain_aware_account_cap(...), ..., pool="auto")`` + call does. Four same-domain (``nvidia_nim``) priced rows that would, + unmodified, fill the whole 4-route fallback budget must not exclude one + independent (``openrouter``) priced row. + """ + namespace = _load_launcher() + fallback_cap = namespace["_fallback_domain_aware_account_cap"] + priced = {"is_free": False, "prompt_price_per_1k": 0.01, "completion_price_per_1k": 0.01, "currency_code": "USD"} + report = { + "models": [ + {"provider": "nvidia_nim", "model": f"dominant/model-{index}", "agent_id": f"nim_{index}", **priced} + for index in range(4) + ] + + [{"provider": "openrouter", "model": "independent/model", "agent_id": "or_0", **priced}] + } + rows = policy.parse_discovery_report(report) + fallback_limit = 4 + cap = fallback_cap( + rows, + fallback_limit=fallback_limit, + configured_cap=4, + outage_domain=policy._outage_domain, + ) + result = policy.build_zdr_prioritized_catalog( + rows, limit=fallback_limit, account_cap=cap, pool="auto" + ) + providers = {agent["provider_name"] for agent in result["agents"]} + assert "nvidia_nim" in providers + assert "openrouter" in providers + + 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``. @@ -1498,9 +1582,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. + The primary-stage call site passes ``_catalog_account_cap(DEFAULT_ACCOUNT_CAP)`` + directly; the priced-fallback call site passes it as + ``_fallback_domain_aware_account_cap``'s ``configured_cap`` (see that + helper's own regression tests for the domain-diversity fix it adds on + top) -- both still source the same single default, so the substring + check below counts ``_catalog_account_cap(DEFAULT_ACCOUNT_CAP)`` alone, + not the full ``account_cap=`` keyword-argument spelling. """ 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 From 9106f689c07732e719ed130223ba457182a73296 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:13:16 +0000 Subject: [PATCH 14/24] test(ci): refresh review-dispatch blob pin and head-advance assertion after rebase Same fix as ContextualWisdomLab/.github#1444's identical rebase-time finding, applied here since this branch independently merged the same concurrent main PRs (#1532, #1533). Concurrent main PRs legitimately changed .github/workflows/opencode-review-dispatch.yml since this branch's last rebase, and this rebase's merge picked those changes up byte-for-byte (confirmed: `git diff origin/main -- .github/workflows/opencode-review-dispatch.yml` is empty). Two pre-existing contract tests were left pointing at stale expectations by that upstream change -- reproducible on origin/main's own tip, not introduced by this branch's diff: - REVIEW_DISPATCH_BLOB_SHA pinned the workflow's pre-#1532/#1533 blob SHA; updated to the current `git hash-object` value. - test_opencode_privileged_review_security_boundaries_are_fail_closed asserted the pre-#1533 strict `[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ]` equality check. #1533 ("fix(opencode): proceed on head-only advance in review dispatch validation") deliberately removed head_sha from the fail-closed mismatch list -- a head advance between dispatch capture and this job is normal PR activity that every downstream job already re-validates independently (STALE_HEAD guards), so failing closed on it only starved the required review check of a verdict. Updated the assertion to check for the new warn-and-proceed behavior instead of the old fail-closed check it replaced. Co-Authored-By: Claude --- tests/test_opencode_agent_contract.py | 15 ++++++++++++++- .../test_pr_review_autofix_nvidia_nim_contract.py | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 79fdba39aa..ec514386c9 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -2660,7 +2660,20 @@ 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 - assert '[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ]' in metadata_step + # #1533: a head_sha-only mismatch is no longer a fail-closed trust + # violation -- every downstream job re-fetches and re-checks the live + # head itself (STALE_HEAD guards), so rejecting normal PR activity + # between dispatch capture and this job only starved the required + # review check of a verdict. base_ref/base_sha/head_ref stay strict. + assert ( + '[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ] || mismatches+=("head_sha")' + ) not in metadata_step + assert ( + 'if [ -n "$SUPPLIED_HEAD_SHA" ] && ' + '[ "$SUPPLIED_HEAD_SHA" != "$live_head_sha" ]; then' + ) in metadata_step + assert "repository_dispatch head advanced since dispatch" in metadata_step + assert "proceeding with the live head" in metadata_step assert ( 'live_visibility="$(jq -r \'.base.repo.visibility // empty | ascii_downcase\'' ) in metadata_step diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 3dcfe2cdd8..68a0614c01 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -19,7 +19,7 @@ DOCTORING_RECORD = Path("docs/doctoring/hourly-nvidia-nim-autofix.md") CHANGELOG = Path("CHANGELOG.md") REVIEW_DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") -REVIEW_DISPATCH_BLOB_SHA = "2aa245e7f2a053a4c0b7a9cc8bac0d5d44d38092" +REVIEW_DISPATCH_BLOB_SHA = "3762183eb31c2805317362d2b2c2546e4fccdf09" def _workflow_text(path: Path) -> str: From 59dc096cabc50961151b2028cbb714b4e5ddba04 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:17:01 +0000 Subject: [PATCH 15/24] docs(changelog): record the priced-fallback domain-diversity fix Appends to this PR's own still-unmerged CHANGELOG bullet (matching this repo's convention of amending an unmerged PR's own entry in place rather than stacking a separate bullet for the same PR). Co-Authored-By: Claude --- CHANGELOG.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4298a711de..349ab64cbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,7 +52,17 @@ Semantic Versioning where the repository publishes a release. 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. + 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). A new `_fallback_domain_aware_account_cap()` helper + shrinks the cap to `fallback_limit // domain_count` (floor, minimum 1) + whenever more than one domain is competing for that stage's rows, so + every domain gets at least one turn; the common single-domain case 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 From 2154afa41a3c435a3bc6db528514469a5729d0da Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:33:33 +0000 Subject: [PATCH 16/24] fix(ci): stop the fallback domain-coverage fix from wasting probe slots Devin Review follow-up finding on this PR's pushed head: the previous commit's _fallback_domain_aware_account_cap() shrank the per-domain cap to fallback_limit // domain_count (floor) to guarantee every outage domain a seat. That fixes starvation but wastes capacity whenever fallback_limit does not divide evenly by domain_count -- concretely, limit=4 across 3 domains floored every domain to cap=1, admitting only 3 routes even though a 4th eligible row existed in one of those domains ("fallback quota wastes probe slots"). A single scalar per-domain cap cannot solve both problems at once (no uniform cap value simultaneously guarantees every domain a seat *and* leaves no capacity unused on an uneven split), so this replaces the cap-shrinking helper with a new guarantee_domain_coverage flag on build_zdr_prioritized_catalog itself: admission now runs in two passes when set. The first pass admits at most one row per outage domain (in the same priority order, bounded by account_cap and limit as before), guaranteeing representation before any domain claims a second seat. The second pass fills any remaining limit budget from the rows the first pass did not pick, still respecting each domain's account_cap ceiling (inclusive of the first pass's contribution) -- so the full budget is used whenever enough eligible rows exist anywhere. The picked order places every diversity (first-pass) row ahead of every fill (second-pass) row, which is the more useful preflight try-order for a pool whose entire purpose is outage-domain resilience, not merely an implementation artifact. _fallback_domain_aware_account_cap() is removed entirely rather than kept alongside the new mechanism: it computed a value the new two-pass admission no longer needs (both launcher call sites now pass _catalog_account_cap(DEFAULT_ACCOUNT_CAP) directly again, unshrunk; only the fallback call site additionally sets guarantee_domain_coverage=True), and keeping an unused helper around would just be a second, silently-driftable place answering the same question. Six new/replacement regression tests at the policy level, including Devin's own two suggested non-divisible splits (limit=4 with 3 domains, and limit=8 with 3 domains) verifying both domain representation and full use of available capacity, plus the single-domain-unchanged case, the account_cap ceiling still holding, and a domains-outnumber-limit edge case. Full suite green (2163 passed), 100% coverage and 100% docstrings on scripts/ci/. Co-Authored-By: Claude --- ...contextual_orchestrator_review_launcher.py | 72 +------- .../contextual_orchestrator_review_policy.py | 72 +++++++- ...t_contextual_orchestrator_review_policy.py | 157 ++++++++++++++++++ ...l_orchestrator_review_runtime_preflight.py | 102 ++---------- 4 files changed, 236 insertions(+), 167 deletions(-) diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index c21360fb22..7754b637a4 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -695,70 +695,6 @@ def _catalog_account_cap(default: int) -> int: return int(os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", str(default))) -def _fallback_domain_aware_account_cap( - rows: list[dict[str, Any]], - *, - fallback_limit: int, - configured_cap: int, - outage_domain: Any, -) -> int: - """Return the priced-fallback stage's per-domain admission cap. - - Devin Review finding on `.github#1474` (verified directly, not trusted - from the finding text alone): with both defaults at 4 - (``fallback_limit = ORCHESTRATOR_CATALOG_LIMIT - primary_count``, and - ``account_cap = DEFAULT_ACCOUNT_CAP``), a single outage domain with at - least ``fallback_limit`` priced rows exhausts the whole fallback - catalog before ``build_zdr_prioritized_catalog``'s greedy admission - loop ever reaches a different, genuinely independent domain's row -- - the per-domain cap provides no diversity protection for this - specific stage precisely because it coincidentally equals the stage's - own overall route limit. - - This does not affect the *primary* catalog stage: there, - ``ORCHESTRATOR_CATALOG_LIMIT`` (12 by default) comfortably exceeds - ``account_cap`` (4), so several domains are always structurally able to - contribute before the primary limit is reached. - - Shrinking the configured cap to ``fallback_limit // domain_count`` - (floor, minimum 1) whenever more than one domain is actually competing - for this stage's rows guarantees every domain gets at least one turn - before any domain can claim a second: with ``domain_count`` domains - each capped at ``cap = fallback_limit // domain_count``, the greedy - loop's own ``len(picked) >= limit`` cutoff (``cap * domain_count <= - fallback_limit``) can never trigger before every domain with at least - one admissible row has already contributed one. When only one domain is - present, this returns exactly ``min(configured_cap, fallback_limit)`` -- - the same value the unmodified cap already produced, so the common, - already-tested single-domain-fallback case is unchanged. - - Args: - rows: The priced rows eligible for this fallback stage (before - ``build_zdr_prioritized_catalog``'s own cost/ZDR/limit - filtering -- domain membership does not depend on that). - fallback_limit: The stage's own overall route budget, from - :func:`_bounded_fallback_catalog_limit`. - configured_cap: The operator-configured per-domain cap, from - :func:`_catalog_account_cap`. - outage_domain: ``contextual_orchestrator_review_policy._outage_domain``, - injected so this module never imports the policy module's - private helper at module scope (matching this file's existing - dependency-injection convention for ``outage_domain``/ - ``provider_account``, e.g. in :func:`_with_discovery_counts`). - - Returns: - The per-domain cap to pass to this stage's - ``build_zdr_prioritized_catalog`` call as ``account_cap``. - """ - if fallback_limit < 1: - return configured_cap - domain_count = len({outage_domain(row) for row in rows}) - if domain_count <= 1: - return min(configured_cap, fallback_limit) - fair_share_cap = max(1, fallback_limit // domain_count) - return min(configured_cap, fair_share_cap) - - def _with_discovery_counts( report: dict[str, object], rows: list[dict[str, Any]], @@ -987,15 +923,11 @@ def main(argv: list[str] | None = None) -> int: fallback_result = build_zdr_prioritized_catalog( admitted_priced_rows, limit=fallback_limit, - account_cap=_fallback_domain_aware_account_cap( - admitted_priced_rows, - fallback_limit=fallback_limit, - configured_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), - outage_domain=_outage_domain, - ), + account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), zdr_endpoints=zdr_endpoints, require_zdr=args.require_zdr, pool="auto", + guarantee_domain_coverage=True, ) except PolicyError: fallback_result = None diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index fa100090f0..547b5e4b2d 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -486,6 +486,7 @@ 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, outage-domain-diverse catalog. @@ -528,6 +529,37 @@ def build_zdr_prioritized_catalog( 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. """ if pool not in {"free", "auto"}: raise PolicyError(f"unsupported review pool {pool!r}") @@ -555,14 +587,38 @@ def build_zdr_prioritized_catalog( per_domain: Counter[str] = Counter() picked: list[Mapping[str, Any]] = [] - for row in _fair_admission_order(eligible_rows, zdr_endpoints=zdr_endpoints): - domain = _outage_domain(row) - if per_domain[domain] >= account_cap: - continue - per_domain[domain] += 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() + for row in ordered_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 = {id(row) for row in picked} + for row in ordered_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 diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 36955457f0..53f978f059 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -926,6 +926,163 @@ def test_build_catalog_shared_domain_cap_does_not_starve_second_account() -> Non 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_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_respects_limit() -> None: """The catalog never exceeds the configured agent limit.""" report = { diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index f19e9094a7..b5dad05115 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1488,88 +1488,12 @@ def test_catalog_account_cap_honors_an_explicit_override( assert namespace["_catalog_account_cap"](policy.DEFAULT_ACCOUNT_CAP) == 6 -def test_fallback_domain_aware_account_cap_leaves_the_single_domain_case_unchanged() -> None: - """One priced domain still gets ``min(configured_cap, fallback_limit)``.""" - namespace = _load_launcher() - fallback_cap = namespace["_fallback_domain_aware_account_cap"] - rows = [{"provider": "openrouter", "base_url": "https://openrouter.ai/api/v1"}] * 6 - assert ( - fallback_cap( - rows, - fallback_limit=4, - configured_cap=4, - outage_domain=policy._outage_domain, - ) - == 4 - ) - - -def test_fallback_domain_aware_account_cap_shrinks_for_competing_domains() -> None: - """Regression for Devin Review's "fallback remains single-domain" finding on `.github#1474`. - - With both defaults at 4 (``fallback_limit == configured_cap``), one - outage domain with at least ``fallback_limit`` priced rows used to - exhaust the whole priced-fallback 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. - """ - namespace = _load_launcher() - fallback_cap = namespace["_fallback_domain_aware_account_cap"] - dominant_domain_rows = [ - {"provider": "nvidia_nim", "base_url": "https://integrate.api.nvidia.com/v1"} - ] * 4 - independent_domain_rows = [ - {"provider": "openrouter", "base_url": "https://openrouter.ai/api/v1"} - ] - cap = fallback_cap( - [*dominant_domain_rows, *independent_domain_rows], - fallback_limit=4, - configured_cap=4, - outage_domain=policy._outage_domain, - ) - assert cap < 4 - assert cap * 2 <= 4 - - -def test_fallback_domain_aware_account_cap_keeps_both_domains_admitted_end_to_end() -> None: - """The computed cap, fed back into ``build_zdr_prioritized_catalog``, admits both domains. - - Exercises the fix at the same boundary the priced-fallback call site in - ``main()`` actually uses: compute the domain-aware cap from the - candidate rows, then build the catalog with it, exactly as - ``main()``'s own ``fallback_result = build_zdr_prioritized_catalog(..., - account_cap=_fallback_domain_aware_account_cap(...), ..., pool="auto")`` - call does. Four same-domain (``nvidia_nim``) priced rows that would, - unmodified, fill the whole 4-route fallback budget must not exclude one - independent (``openrouter``) priced row. - """ - namespace = _load_launcher() - fallback_cap = namespace["_fallback_domain_aware_account_cap"] - priced = {"is_free": False, "prompt_price_per_1k": 0.01, "completion_price_per_1k": 0.01, "currency_code": "USD"} - report = { - "models": [ - {"provider": "nvidia_nim", "model": f"dominant/model-{index}", "agent_id": f"nim_{index}", **priced} - for index in range(4) - ] - + [{"provider": "openrouter", "model": "independent/model", "agent_id": "or_0", **priced}] - } - rows = policy.parse_discovery_report(report) - fallback_limit = 4 - cap = fallback_cap( - rows, - fallback_limit=fallback_limit, - configured_cap=4, - outage_domain=policy._outage_domain, - ) - result = policy.build_zdr_prioritized_catalog( - rows, limit=fallback_limit, account_cap=cap, pool="auto" - ) - providers = {agent["provider_name"] for agent in result["agents"]} - assert "nvidia_nim" in providers - assert "openrouter" in providers +def test_main_wires_guarantee_domain_coverage_for_the_priced_fallback_stage() -> None: + """``main()``'s priced-fallback ``build_zdr_prioritized_catalog`` call opts into coverage.""" + source = _LAUNCHER.read_text(encoding="utf-8") + 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: @@ -1582,13 +1506,13 @@ 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. - The primary-stage call site passes ``_catalog_account_cap(DEFAULT_ACCOUNT_CAP)`` - directly; the priced-fallback call site passes it as - ``_fallback_domain_aware_account_cap``'s ``configured_cap`` (see that - helper's own regression tests for the domain-diversity fix it adds on - top) -- both still source the same single default, so the substring - check below counts ``_catalog_account_cap(DEFAULT_ACCOUNT_CAP)`` alone, - not the full ``account_cap=`` keyword-argument spelling. + 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("_catalog_account_cap(DEFAULT_ACCOUNT_CAP)") == 2 From f03ecfa7e8b8d82d6285861ee90058c5c9937f45 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:35:41 +0000 Subject: [PATCH 17/24] docs(changelog): correct the fallback fix's mechanism after the revision Updates the CHANGELOG bullet added two commits ago to describe the two-pass guarantee_domain_coverage mechanism instead of the now-removed cap-shrinking helper it originally described. Co-Authored-By: Claude --- CHANGELOG.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 349ab64cbc..8e7bad3f24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,11 +58,18 @@ Semantic Versioning where the repository publishes a release. 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). A new `_fallback_domain_aware_account_cap()` helper - shrinks the cap to `fallback_limit // domain_count` (floor, minimum 1) - whenever more than one domain is competing for that stage's rows, so - every domain gets at least one turn; the common single-domain case is - unchanged. + 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. - 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 From 1715db04a2518b5f88d4b0f1c80100521fc7c2ee Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:48:30 +0000 Subject: [PATCH 18/24] fix(ci): extend domain-coverage guarantee to the primary auto-pool stage Devin Review follow-up finding on this PR's pushed head: the guarantee_domain_coverage fix two commits ago only covered the priced-fallback stage. The identical single-scalar-cap-equals-limit coincidence is independently reachable through the *primary* auto-pool stage too, under the sidecar's real deployed configuration (not the DEFAULT_ACCOUNT_CAP=4 fixture value most of this file's tests use): contextual_orchestrator_review_sidecar.sh exports ORCHESTRATOR_CATALOG_ACCOUNT_CAP=8 by default, and this launcher's own REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT is also 8 for the auto pool's primary stage -- eight free routes from one dominant outage domain could exclude every independent free domain from the primary catalog entirely, at account_cap=limit=8 rather than the fallback stage's account_cap=limit=4. Wires guarantee_domain_coverage=True into main()'s primary build_zdr_prioritized_catalog call site as well. Full local suite (2163 passed before this change) confirmed no regressions from this extension -- guarantee_domain_coverage is a no-op whenever a stage's eligible rows only ever touch one outage domain, which covers every existing primary-stage test fixture. Adds a dedicated regression using the real deployed values (account_cap=8, limit=8, matching the sidecar's actual default rather than this file's usual account_cap=4 fixtures) reproducing Devin's exact scenario, plus extends the source-level wiring contract test to require guarantee_domain_coverage=True at both call sites. Full suite green (2164 passed), 100% coverage and 100% docstrings on scripts/ci/. Co-Authored-By: Claude --- CHANGELOG.md | 12 ++++++- ...contextual_orchestrator_review_launcher.py | 1 + ...t_contextual_orchestrator_review_policy.py | 31 +++++++++++++++++++ ...l_orchestrator_review_runtime_preflight.py | 19 ++++++++++-- 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e7bad3f24..26966117b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,7 +69,17 @@ Semantic Versioning where the repository publishes a release. 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. + 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()`. - 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/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 7754b637a4..8c0989046f 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -893,6 +893,7 @@ 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"], diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 53f978f059..adde3d063b 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -1032,6 +1032,37 @@ def test_build_catalog_guarantee_domain_coverage_uses_full_budget_on_eight_over_ 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. diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index b5dad05115..d29bfd02e0 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1488,9 +1488,24 @@ 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_the_priced_fallback_stage() -> None: - """``main()``'s priced-fallback ``build_zdr_prioritized_catalog`` call opts into coverage.""" +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 From e52923d309a4f562c37f393bbb413a083d64c141 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:49:49 +0000 Subject: [PATCH 19/24] test(ci): revert the head-advance test changes now that main reverted #1533 Main moved again mid-rebase: .github#1540 reverted #1533's warn-and-proceed head_sha check entirely (no rationale recorded beyond the revert itself), restoring the original strict fail-closed equality and, with it, the workflow file's original blob SHA (confirmed: git hash-object on origin/main's copy is exactly 2aa245e7f2a053a4c0b7a9cc8bac0d5d44d38092, byte-identical to what this file's REVIEW_DISPATCH_BLOB_SHA pinned before this whole detour started). Reverts this branch's own two prior commits' changes to these same two spots: REVIEW_DISPATCH_BLOB_SHA back to the original pin, and test_opencode_privileged_review_security_boundaries_are_fail_closed back to asserting the strict equality check instead of the now-reverted warn-and-proceed behavior. Co-Authored-By: Claude --- tests/test_opencode_agent_contract.py | 19 +++++-------------- ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index ec514386c9..746b750c16 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -2660,20 +2660,11 @@ 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: a head_sha-only mismatch is no longer a fail-closed trust - # violation -- every downstream job re-fetches and re-checks the live - # head itself (STALE_HEAD guards), so rejecting normal PR activity - # between dispatch capture and this job only starved the required - # review check of a verdict. base_ref/base_sha/head_ref stay strict. - assert ( - '[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ] || mismatches+=("head_sha")' - ) not in metadata_step - assert ( - 'if [ -n "$SUPPLIED_HEAD_SHA" ] && ' - '[ "$SUPPLIED_HEAD_SHA" != "$live_head_sha" ]; then' - ) in metadata_step - assert "repository_dispatch head advanced since dispatch" in metadata_step - assert "proceeding with the live head" 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 ( 'live_visibility="$(jq -r \'.base.repo.visibility // empty | ascii_downcase\'' ) in metadata_step diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 68a0614c01..3dcfe2cdd8 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -19,7 +19,7 @@ DOCTORING_RECORD = Path("docs/doctoring/hourly-nvidia-nim-autofix.md") CHANGELOG = Path("CHANGELOG.md") REVIEW_DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") -REVIEW_DISPATCH_BLOB_SHA = "3762183eb31c2805317362d2b2c2546e4fccdf09" +REVIEW_DISPATCH_BLOB_SHA = "2aa245e7f2a053a4c0b7a9cc8bac0d5d44d38092" def _workflow_text(path: Path) -> str: From db106d50f2134ece147bc5318e389aeb124d198c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:21:06 +0000 Subject: [PATCH 20/24] test(ci): close main's post-#1546 scheduler coverage regression Protected main regressed to 99% scripts/ci coverage after #1546 added live_head_matches, a no-active/no-stale fall-through in prepare_autofix_slot, and an "already queued or running" wait branch to pr_review_fix_scheduler.py without covering them, while the pre-existing inspect_pr conflicted-draft/conflicted-unauthorized returns and pr_review_merge_scheduler.py's fetch_workflow_names_by_check_suite_rest pagination/filtering/ permission-denied paths stayed untested. Every PR rebasing onto main inherits this via the coverage-evidence required check regardless of its own diff. Test-only change; no production code touched. --- CHANGELOG.md | 9 +++ tests/test_pr_review_fix_scheduler.py | 49 ++++++++++++++ ...ew_fix_scheduler_rest_workflow_identity.py | 67 +++++++++++++++++++ 3 files changed, 125 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5810d5308..1c46c64657 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- 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. - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 9860eeaec7..f6abd64b0f 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -177,6 +177,40 @@ def test_prepare_autofix_slot_preserves_new_head_workers_after_head_advance(monk workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, dry_run=False, ) is None + + +def test_prepare_autofix_slot_returns_directly_with_no_active_or_stale_runs(monkeypatch): + """An empty Actions run list needs no reconciliation and skips cancellation.""" + monkeypatch.setattr(fix, "run_json", lambda _args: {"workflow_runs": []}) + monkeypatch.setattr( + fix, + "force_cancel_workflow_runs", + lambda *_args: pytest.fail("no stale runs must not attempt cancellation"), + ) + + assert fix.prepare_autofix_slot( + "owner/repo", + make_pr(), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=False, + ) is False + + +def test_live_head_matches_compares_case_insensitively_and_fails_closed(monkeypatch): + """Live head lookup normalizes case and rejects malformed or mismatched payloads.""" + head = "a" * 40 + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": head.upper()}}) + assert fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": "b" * 40}}) + assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"nothead": {}}) + assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + def test_terminal_failed_check_triggers_rca_without_prior_opencode_review(): """Exact-head check evidence can start RCA without a circular review prerequisite.""" pr = make_pr( @@ -1329,6 +1363,21 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch): monkeypatch.setattr(fix, "issue_comments", lambda repo, number: [{"body": f"{fix.FIX_MARKER} head_sha={'a' * 40} epoch={int(time.time())} -->"}]) assert fix.inspect_pr("owner/repo", make_pr(), args) == ("wait", ("recent autofix marker exists for this head",)) + assert fix.inspect_pr( + "owner/repo", make_pr(mergeStateStatus="DIRTY", isDraft=True), args + ) == ("skip", ("draft PR",)) + assert fix.inspect_pr("owner/repo", make_pr(mergeStateStatus="DIRTY"), args) == ( + "skip", + ("merge conflict is not authorized for repair",), + ) + + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: True) + assert fix.inspect_pr("owner/repo", make_pr(), args) == ( + "wait", + ("current-head autofix run is already queued or running",), + ) + pr1 = make_pr(number=1) pr2 = make_pr(number=2) monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr1, pr2]) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index c24cfb05f9..f261ce5beb 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -154,3 +154,70 @@ def fake_api(path: str) -> Any: assert merge.is_strix_context(context) assert merge.strix_evidence_state(pr) == expected_state assert fix.current_head_failed_checks(pr) == () + + +def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100( + monkeypatch: Any, +) -> None: + """A first page of exactly 100 runs must fetch a second page and merge both.""" + head_sha = "e" * 40 + page1 = [ + {"check_suite_id": i, "name": f"workflow-{i}"} for i in range(100) + ] + page2 = [{"check_suite_id": 100, "name": "workflow-100"}] + calls: list[str] = [] + + def fake_api(path: str) -> Any: + calls.append(path) + if path.endswith("page=1"): + return {"workflow_runs": page1} + if path.endswith("page=2"): + return {"workflow_runs": page2} + raise AssertionError(f"unexpected path {path}") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) + + assert names == {i: f"workflow-{i}" for i in range(101)} + assert calls == [ + f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=1", + f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=2", + ] + + +def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id_or_name( + monkeypatch: Any, +) -> None: + """A run with no check-suite id or a blank name must not populate the map.""" + head_sha = "f" * 40 + + def fake_api(path: str) -> Any: + return { + "workflow_runs": [ + {"check_suite_id": None, "name": "orphaned run"}, + {"check_suite_id": 900, "name": ""}, + {"check_suite_id": 901, "name": "kept run"}, + ] + } + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) + + assert names == {901: "kept run"} + + +def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors( + monkeypatch: Any, +) -> None: + """A page-fetch failure unrelated to integration access must fail closed.""" + head_sha = "0" * 40 + + def fake_api(path: str) -> Any: + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + with pytest.raises(RuntimeError, match="HTTP 502"): + merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) From 85c2469e1e624f8d4dfbc71c79fe18c927d315ab Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:38:38 +0000 Subject: [PATCH 21/24] docs(gap-baseline): record post-#1546 scheduler coverage regression Adds a dated traceability entry for the coverage gap this PR closes: root cause (#1546's uncovered additions plus the older #1547/#1551/ #1554 gap, neither of which merged or transfers evidence here), the fix and its verification, the resolved Devin false-positive on sub-clause coverage, and the known pre-existing SIGPIPE test flake left unremediated as out of scope. --- docs/product-technical-gap-baseline.md | 48 ++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 76d85b949b..812f068e34 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2344,6 +2344,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. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. From 6f40a0637da94da60f43ca72086d27e1034e8bbc Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 16:46:11 +0900 Subject: [PATCH 22/24] test(ci): document nested REST fixture helpers Raise scoped docstring coverage for the newly added scheduler REST regression helpers to 100% without changing test behavior or production code. --- tests/test_pr_review_fix_scheduler_rest_workflow_identity.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index f261ce5beb..4e36544061 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -168,6 +168,7 @@ def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100( calls: list[str] = [] def fake_api(path: str) -> Any: + """Return deterministic paginated workflow-run fixtures.""" calls.append(path) if path.endswith("page=1"): return {"workflow_runs": page1} @@ -193,6 +194,7 @@ def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id head_sha = "f" * 40 def fake_api(path: str) -> Any: + """Return workflow runs that exercise incomplete-identity filtering.""" return { "workflow_runs": [ {"check_suite_id": None, "name": "orphaned run"}, @@ -215,6 +217,7 @@ def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors( head_sha = "0" * 40 def fake_api(path: str) -> Any: + """Simulate a non-access REST failure that must propagate.""" raise RuntimeError("gh: HTTP 502 (exhausted retries)") monkeypatch.setattr(merge, "gh_api_json", fake_api) From 9e6aa161d83035fef3e8755e326ddfde7b305414 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 08:24:01 +0000 Subject: [PATCH 23/24] fix(ci): scope guarantee_domain_coverage's admission passes to one tier Devin Review found that build_zdr_prioritized_catalog's guarantee_domain_coverage two-pass admission ran across the full tier-sorted row list instead of within one admission-priority tier at a time, letting a worse-tier row win a first-pass "guaranteed representation" seat for its domain ahead of a better-tier row from an already-represented domain. Restructured both passes to iterate contiguous same-tier runs of ordered_rows in priority order, finishing a tier's own first and second pass before ever looking at the next, worse tier -- the same tier-boundary discipline _fair_admission_order already enforces for its own reordering. covered_domains/per_domain still accumulate across tiers so a domain already seated in a better tier does not claim a redundant guaranteed seat in a worse one. Added a regression proving the pre-fix behavior red (two free/ZDR openrouter routes plus one free/non-ZDR bytez route, limit=2: both admitted rows must stay free/ZDR) before confirming it green after. --- CHANGELOG.md | 12 +++ .../contextual_orchestrator_review_policy.py | 78 ++++++++++++++----- ...t_contextual_orchestrator_review_policy.py | 33 ++++++++ 3 files changed, 103 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bae6b7631..8058e11974 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -235,6 +235,18 @@ Semantic Versioning where the repository publishes a release. 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/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 547b5e4b2d..dc17bf156e 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -560,6 +560,25 @@ def build_zdr_prioritized_catalog( 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. """ if pool not in {"free", "auto"}: raise PolicyError(f"unsupported review pool {pool!r}") @@ -590,26 +609,45 @@ def build_zdr_prioritized_catalog( ordered_rows = _fair_admission_order(eligible_rows, zdr_endpoints=zdr_endpoints) if guarantee_domain_coverage: covered_domains: set[str] = set() - for row in ordered_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 = {id(row) for row in picked} - for row in ordered_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) + 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) diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index adde3d063b..5c925c7ef7 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -1114,6 +1114,39 @@ def test_build_catalog_guarantee_domain_coverage_caps_at_limit_when_domains_outn 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: """The catalog never exceeds the configured agent limit.""" report = { From 1eaa3b3a7b9575d5c9fb2285710ad20b73bc7b6a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 12:26:23 +0000 Subject: [PATCH 24/24] fix(ci): merge #1474's tier-scoped domain-cap fix with #1587/#1592/#1564 Merging current main pulled in three overlapping changes to the same review-catalog code this PR's own outage-domain-cap-grouping fix touches: - #1587 added FREE_POOL_CREDENTIAL_NAMES source-authorization filtering (scripts/ci/contextual_orchestrator_review_policy.py and contextual_orchestrator_review_launcher.py's _with_discovery_counts, plus tests/test_contextual_orchestrator_free_pool_enrichment.py) and is independent of this PR's outage-domain grouping -- combined both additions in _with_discovery_counts (free_outage_domain_diversity alongside the free_pool_* fields) and in build_zdr_prioritized_catalog's report dict, dropping one duplicate free_account_diversity key the merge introduced. - #1592 fixed test_build_catalog_applies_account_cap's stale "openai" fixture (pre-#1587) to "openrouter" on protected main; this PR's own branch had already independently rewritten the same test's assertions for the outage-domain-grouped cap semantics (nvidia_nim/nvidia_nim_sub sharing one domain's cap) but still referenced the stale "openai" key in its own assertion -- corrected to "openrouter", matching the fixture's already-current provider name. - test_contextual_orchestrator_free_pool_enrichment.py's own new test called _with_discovery_counts without the outage_domain keyword this PR's fix requires; added a matching lambda and a free_outage_domain_diversity assertion, preserving the test's original free-pool-enrichment intent. - #1564 (merge-base-anchored deleted-file review evidence) left tests/test_noema_review_gate.py and tests/test_noema_removed_file_context.py broken against renamed/removed noema_review_gate.py functions; ported the same fix already opened as its own dedicated PR (#1598). Full suite: 2362 passed, 100% branch coverage, 100% docstrings. --- ...xtual_orchestrator_free_pool_enrichment.py | 2 + tests/test_noema_removed_file_context.py | 132 ++++++++++++++---- tests/test_noema_review_gate.py | 66 ++++----- 3 files changed, 139 insertions(+), 61 deletions(-) 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_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))