From f3facda54a88dbac65009638ecdcebe289d3245d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:06:56 +0000 Subject: [PATCH 01/13] 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 256b0cdf9..c630ac5cd 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 21b118ce9..87316cce2 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 ecb4f3b69..b5de58a23 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 758ef2961..463660b8e 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 6dbbe2d5e..c299f7970 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 1c8a17014..721093f30 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 b10fc4a0b..95b7f3145 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 32f1c2241..64f6731d5 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/13] 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 c630ac5cd..8533202b5 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 463660b8e..b8abb4cda 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 721093f30..5067f9d0e 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 95b7f3145..26acde8b6 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/13] 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 8533202b5..322b6afcd 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 b8abb4cda..ce6699584 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 5067f9d0e..5d40ae9bf 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 26acde8b6..21ec84303 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/13] 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 322b6afcd..98ed6c233 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 ce6699584..ffcea3947 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 5d40ae9bf..ce69ade1e 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 21ec84303..be8d2e5c0 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 e27f7c29825ade8e788dee0bde245097fd8a868f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 05:13:21 +0000 Subject: [PATCH 05/13] 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 ce69ade1e..fa100090f 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 be8d2e5c0..36955457f 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 fb084b6249043026021b4d8897e8f8054b3c2138 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:08:31 +0000 Subject: [PATCH 06/13] 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 4053f4fd5..1fc45a34b 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 07/13] 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 7618f9dd0..c21360fb2 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 753e15018..f19e9094a 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 08/13] 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 79fdba39a..ec514386c 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 3dcfe2cdd..68a0614c0 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 09/13] 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 4298a711d..349ab64cb 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 10/13] 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 c21360fb2..7754b637a 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 fa100090f..547b5e4b2 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 36955457f..53f978f05 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 f19e9094a..b5dad0511 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 11/13] 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 349ab64cb..8e7bad3f2 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 12/13] 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 8e7bad3f2..26966117b 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 7754b637a..8c0989046 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 53f978f05..adde3d063 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 b5dad0511..d29bfd02e 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 13/13] 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 ec514386c..746b750c1 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 68a0614c0..3dcfe2cdd 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: