From 1604a1a3a4cb569e084f8296d47b155a8ab7ded0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 03:51:28 +0000 Subject: [PATCH] fix(ci): stop collapsing nvidia_nim/nvidia_nim_sub in review policy PROVIDER_FAMILIES scripts/ci/contextual_orchestrator_review_policy.py's PROVIDER_FAMILIES mapping wrongly assumed the two independent nvidia_nim/nvidia_nim_sub KV credentials share one model catalog/outage domain -- the exact assumption contextual-orchestrator PR #941/#945 already corrected in that repo's own model_discovery.py, and which docs/planning/adrs/0015-durable-provider- catalog.md (accepted 2026-08-22, in contextual-orchestrator) already contradicted. This file has its own, completely independent copy of that bug: #941 never touched .github, since it only fixed contextual-orchestrator itself. This matters beyond internal consistency: provider_family() feeds free_family_diversity (PR #1433's evidence field), which open PR #1437 (draft) gates Strix's orchestrator/free eligibility on (>= 2). The bug undercounted a discovery report whose only free routes are the two NVIDIA credentials as diversity 1 instead of 2, which could keep Strix on the paid orchestrator/auto pool when the underlying catalog was, in fact, diverse enough. PROVIDER_FAMILIES is now empty ({}) rather than deleted outright: provider_family()'s existing fallback already treats an unregistered provider name as its own family, and contextual_orchestrator_review_ launcher.py plus two test files still depend on the provider_family/ free_family_diversity contract, so this keeps that public surface stable while fixing the actual data. No other legitimate entries existed in PROVIDER_FAMILIES to preserve. Tests: renamed/corrected test_provider_family_groups_nvidia_keys -> test_provider_family_treats_nvidia_credentials_as_independent; corrected test_build_catalog_reports_free_family_diversity's expectation 4 -> 5; added test_build_catalog_treats_nvidia_credentials_as_independent_families (reproduces the exact PR #1437 undercount shape, asserts diversity == 2, not 1); rebuilt test_build_catalog_reports_single_family_free_concentration on a genuinely single-family fixture (two openrouter models) instead of relying on the wrong NVIDIA collapsing; updated test_build_catalog_applies_family_cap to expect nvidia_nim and nvidia_nim_sub each capped independently. Full suite: 2093 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/ (interrogate). Does not touch open PR #1437's own gating logic; that PR's reviewer should read this as confirmation the free_family_diversity evidence it depends on was previously undercounted for the two-NVIDIA-credential case. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 11 +++ docs/product-technical-gap-baseline.md | 64 +++++++++++++ .../contextual_orchestrator_review_policy.py | 31 ++++++- ...t_contextual_orchestrator_review_policy.py | 92 ++++++++++++++++--- 4 files changed, 180 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6682241627..2bf0bd0654 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- `scripts/ci/contextual_orchestrator_review_policy.py`'s `PROVIDER_FAMILIES` + no longer collapses the independent `nvidia_nim`/`nvidia_nim_sub` + credentials into one outage-domain family -- a duplicate, independently + introduced copy of the assumption `contextual-orchestrator` PR #941/#945 + already corrected upstream in `model_discovery.py`. This directly + increases `free_family_diversity` (PR #1433) whenever both NVIDIA + credentials are present, which open PR #1437's Strix `orchestrator/free` + gating logic depends on. `PROVIDER_FAMILIES` is now empty rather than + removed, since `contextual_orchestrator_review_launcher.py` and two test + files still depend on the `provider_family()`/`free_family_diversity` + contract. - Web verification now runs backend, frontend, and E2E commands inside an isolated Linux bubblewrap workspace by default (`--isolation required`), mounting a read-only runtime root with a single writable `/workspace` diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 758ef2961a..cdbd8052e0 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,70 @@ 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 `scripts/ci/contextual_orchestrator_review_policy.py`'s own `PROVIDER_FAMILIES` copy of the NVIDIA-independence bug + +**What this is a duplicate of.** `contextual-orchestrator` PR #941 (`fix(discovery): keep credential +accounts independent`, merged) removed a wrong assumption from that repo's `model_discovery.py`: that +the two independent `nvidia_nim`/`nvidia_nim_sub` KV credentials share one model catalog/outage domain. +`docs/planning/adrs/0015-durable-provider-catalog.md` (accepted 2026-08-22, in `contextual-orchestrator`) +already contradicted that assumption. This session's investigation, verifying that #941/#945's fix +actually reaches production (see the sidecar-pin entry above), found a **second, completely independent +copy of the identical wrong assumption** living in this repo: `scripts/ci/contextual_orchestrator_review_ +policy.py`'s hardcoded `PROVIDER_FAMILIES` mapping — + +```python +PROVIDER_FAMILIES: Mapping[str, str] = { + "nvidia_nim": "nvidia_nim", + "nvidia_nim_sub": "nvidia_nim", +} +``` + +— used by `provider_family(provider_name)`, which in turn feeds two things: the `per_family` admission +cap inside `build_zdr_prioritized_catalog` (so the two credentials shared one combined +`DEFAULT_FAMILY_CAP` slot count instead of each getting its own), and — more consequentially — the +`free_family_diversity` evidence field PR #1433 added to that same function's report. `#941` never +touched this file, since #941 only fixed `contextual-orchestrator` itself; this copy was never wired to +that fix and has been undercounting independently the whole time. + +**Why this matters beyond internal consistency.** Open PR #1437 (draft, "gate Strix's `orchestrator/free` +access on live diversity evidence") reads `free_family_diversity` and gates Strix's move off +`orchestrator/auto` onto the free pool on `free_family_diversity >= 2`. With the bug in place, a +discovery report whose only free routes happen to be `nvidia_nim` + `nvidia_nim_sub` reported diversity +of 1 (one shared family) instead of 2 (two independent credential accounts) — undercounting exactly the +scenario that evidence exists to detect, and potentially keeping Strix on the paid `orchestrator/auto` +pool when the underlying catalog was, in fact, diverse enough to qualify for `orchestrator/free`. + +**Fix.** `PROVIDER_FAMILIES` is now empty (`{}`), not deleted outright: `provider_family()`'s existing +fallback (`PROVIDER_FAMILIES.get(provider_name, provider_name)`) already treats any unregistered +provider name as its own independent family, so an empty map means every KV-registered provider +credential defaults to its own family unless a *future* provider is verified to genuinely share +infrastructure and catalog fate across two credential names — the map stays as that registration point, +rather than removing the `provider_family`/`free_family_diversity` concept `contextual_orchestrator_ +review_launcher.py` and two other test files already depend on. No other legitimate entries existed in +`PROVIDER_FAMILIES` to preserve — it held only the two wrong NVIDIA rows. + +**Tests.** `tests/test_contextual_orchestrator_review_policy.py`: renamed/corrected +`test_provider_family_groups_nvidia_keys` (asserted the wrong collapsing) to +`test_provider_family_treats_nvidia_credentials_as_independent`; corrected +`test_build_catalog_reports_free_family_diversity`'s expectation from 4 to 5 (the fixture's five +providers, including both NVIDIA credentials, are now all independent); added a dedicated regression, +`test_build_catalog_treats_nvidia_credentials_as_independent_families`, reproducing the exact PR #1437 +undercount shape (only the two NVIDIA credentials as free routes) and asserting diversity is now +correctly 2, not 1; rebuilt `test_build_catalog_reports_single_family_free_concentration` on a fixture +that is genuinely single-family (two models under one `openrouter` credential) rather than relying on +the wrong NVIDIA collapsing to make its point; and updated `test_build_catalog_applies_family_cap`'s +assertions to expect `nvidia_nim` and `nvidia_nim_sub` each capped independently rather than sharing one +combined cap. Full suite green; 100% coverage and 100% docstring coverage on `scripts/ci/` (see this PR's +own test-plan checklist for the exact run). + +**Sequencing with the sidecar-pin fix above.** This fix and the `ORCHESTRATOR_PIN_SHA` bump above are +deliberately two separate PRs (a hardcoded-assumption bug here vs. a stale-vendoring-pin bug there) but +are causally linked: the pin bump is what lets `contextual-orchestrator`'s own #941/#945 fix reach +production at all, while this fix corrects this repo's own independent copy of the same bug. Both need +to land for `free_family_diversity` to be trustworthy end-to-end. Neither PR modifies PR #1437's own +gating logic; #1437's reviewer should read this entry as confirmation that the evidence it depends on was +previously undercounted for exactly the two-NVIDIA-credential case, now corrected. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_policy.py b/scripts/ci/contextual_orchestrator_review_policy.py index 6a03319f00..20e0ec8686 100644 --- a/scripts/ci/contextual_orchestrator_review_policy.py +++ b/scripts/ci/contextual_orchestrator_review_policy.py @@ -27,10 +27,23 @@ route_key, ) -PROVIDER_FAMILIES: Mapping[str, str] = { - "nvidia_nim": "nvidia_nim", - "nvidia_nim_sub": "nvidia_nim", -} +# Each KV-registered provider credential is its own independent outage domain +# and catalog by default: nothing here collapses two credential names into one +# family unless a provider genuinely shares infrastructure and catalog fate +# across separate credentials. This was NOT true of ``nvidia_nim``/ +# ``nvidia_nim_sub``: they are two independent NVIDIA NIM API key accounts +# that may be entitled to different models and may fail independently of each +# other, per `contextual-orchestrator`'s own +# ``docs/planning/adrs/0015-durable-provider-catalog.md`` (accepted +# 2026-08-22) and its PR #941/#945 fix to ``model_discovery.py``'s matching +# assumption. Collapsing them here silently undercounted +# ``free_family_diversity`` (see ``build_zdr_prioritized_catalog``) and let +# one family's cap in ``build_zdr_prioritized_catalog`` absorb capacity meant +# for two independent accounts. Left empty, not deleted outright, so a +# provider with a *verified* shared outage domain across credential names can +# still be registered here later with evidence, without redesigning every +# caller of ``provider_family``. +PROVIDER_FAMILIES: Mapping[str, str] = {} DEFAULT_CATALOG_LIMIT = 12 DEFAULT_FAMILY_CAP = 4 @@ -52,7 +65,15 @@ class PolicyError(ValueError): def provider_family(provider_name: str) -> str: - """Return the outage-domain family for a provider.""" + """Return the outage-domain family for a provider. + + Defaults to the provider name itself: with ``PROVIDER_FAMILIES`` empty, + every KV-registered provider credential is treated as its own + independent family unless explicitly registered otherwise above. This + is a deliberately conservative default -- sharing a vendor or endpoint + is not evidence that two credential accounts share catalog or outage + fate. + """ return PROVIDER_FAMILIES.get(provider_name, provider_name) diff --git a/tests/test_contextual_orchestrator_review_policy.py b/tests/test_contextual_orchestrator_review_policy.py index 0f0f8cb6b0..3a18abf399 100644 --- a/tests/test_contextual_orchestrator_review_policy.py +++ b/tests/test_contextual_orchestrator_review_policy.py @@ -68,11 +68,22 @@ def _report() -> dict[str, object]: } -def test_provider_family_groups_nvidia_keys() -> None: - """The primary and secondary NVIDIA keys share one outage-domain family.""" +def test_provider_family_treats_nvidia_credentials_as_independent() -> None: + """The primary and secondary NVIDIA keys are independent outage domains. + + Regression coverage for a real bug: an earlier revision of + ``PROVIDER_FAMILIES`` collapsed ``nvidia_nim``/``nvidia_nim_sub`` into + one family, wrongly assuming the two independent NVIDIA NIM API key + accounts share one model catalog/outage domain -- an assumption + ``contextual-orchestrator``'s ``docs/planning/adrs/0015-durable-provider- + catalog.md`` already contradicted, and which that repo's own PR #941/ + #945 corrected in ``model_discovery.py``. This is the matching, until-now + independent copy of that same wrong assumption in this repo. + """ assert policy.provider_family("nvidia_nim") == "nvidia_nim" - assert policy.provider_family("nvidia_nim_sub") == "nvidia_nim" + assert policy.provider_family("nvidia_nim_sub") == "nvidia_nim_sub" assert policy.provider_family("openai") == "openai" + assert policy.PROVIDER_FAMILIES == {} @pytest.mark.parametrize( @@ -282,8 +293,50 @@ def test_build_catalog_reports_free_family_diversity() -> None: family_cap=4, zdr_endpoints=ZDR_FEED, ) - # openrouter, nvidia_nim (+ its nvidia_nim_sub sibling), openai, bytez. - assert result["report"]["free_family_diversity"] == 4 + # openrouter, nvidia_nim, nvidia_nim_sub, openai, bytez -- five + # independent credential accounts, none collapsed. + assert result["report"]["free_family_diversity"] == 5 + + +def test_build_catalog_treats_nvidia_credentials_as_independent_families() -> None: + """The two NVIDIA NIM credentials count as two independent families. + + Regression coverage for a real bug: an earlier revision of + ``PROVIDER_FAMILIES`` collapsed ``nvidia_nim``/``nvidia_nim_sub`` into + one outage-domain family, wrongly assuming the two independent NVIDIA + NIM API key accounts share one model catalog/outage domain -- an + assumption ``contextual-orchestrator``'s ``docs/planning/adrs/0015- + durable-provider-catalog.md`` already contradicted, and which that + repo's own PR #941/#945 corrected in ``model_discovery.py``. Before that + fix, a discovery report whose only free routes were these two NVIDIA + credentials undercounted ``free_family_diversity`` as 1, which could + keep a caller gating on ``free_family_diversity >= 2`` (see PR #1437) on + a paid pool even though the two credentials are, in fact, independent. + """ + two_nvidia_credentials_report = { + "models": [ + { + "provider": "nvidia_nim", + "model": "nvidia/nemotron-3-nano-30b-a3b", + "agent_id": "nim_nano_free", + "is_free": True, + **FREE_PRICE, + }, + { + "provider": "nvidia_nim_sub", + "model": "meta/llama-3.3-70b-instruct", + "agent_id": "nimsec_70b", + "is_free": True, + **FREE_PRICE, + }, + ] + } + result = policy.build_zdr_prioritized_catalog( + policy.parse_discovery_report(two_nvidia_credentials_report), + limit=12, + family_cap=4, + ) + assert result["report"]["free_family_diversity"] == 2 def test_build_catalog_reports_single_family_free_concentration() -> None: @@ -293,21 +346,27 @@ def test_build_catalog_reports_single_family_free_concentration() -> None: ADR-0003: every discovered free route sharing one upstream provider means that provider's outage takes down the whole free catalog, which is why Strix cannot safely run on a strict ``orchestrator/free`` pool - without this evidence showing at least two independent families. + without this evidence showing at least two independent families. Uses + two models under the *same* credential (``openrouter`` twice) so the + single-family premise holds regardless of any per-provider family + mapping -- unlike an earlier revision of this test, which relied on + ``nvidia_nim``/``nvidia_nim_sub`` wrongly collapsing to one family (see + ``test_build_catalog_treats_nvidia_credentials_as_independent_families`` + for that corrected, dedicated regression). """ single_family_report = { "models": [ { - "provider": "nvidia_nim", - "model": "nvidia/nemotron-3-nano-30b-a3b", - "agent_id": "nim_nano_free", + "provider": "openrouter", + "model": "deepseek/deepseek-r1:free", + "agent_id": "or_ds_r1", "is_free": True, **FREE_PRICE, }, { - "provider": "nvidia_nim_sub", - "model": "meta/llama-3.3-70b-instruct", - "agent_id": "nimsec_70b", + "provider": "openrouter", + "model": "meta-llama/llama-3.3-70b-instruct:free", + "agent_id": "or_llama_70b", "is_free": True, **FREE_PRICE, }, @@ -345,7 +404,13 @@ def test_build_catalog_assigns_unique_priorities() -> None: def test_build_catalog_applies_family_cap() -> None: - """A family cap keeps one outage domain from absorbing the pool.""" + """A family cap keeps one outage domain from absorbing the pool. + + ``nvidia_nim`` and ``nvidia_nim_sub`` are independent credential + accounts/families (see ``test_build_catalog_treats_nvidia_credentials_ + as_independent_families``), so each gets its own cap here rather than + sharing one combined cap of ``family_cap`` between them. + """ report = { "models": [ {"provider": "nvidia_nim", "model": f"m{i}", "agent_id": f"nim_a{i}", "is_free": True, **FREE_PRICE} @@ -374,6 +439,7 @@ def test_build_catalog_applies_family_cap() -> None: family = policy.provider_family(agent["provider_name"]) family_counts[family] = family_counts.get(family, 0) + 1 assert family_counts["nvidia_nim"] == 2 + assert family_counts["nvidia_nim_sub"] == 2 assert family_counts["openai"] == 2