diff --git a/CHANGELOG.md b/CHANGELOG.md index 256b0cdf94..39c61c142b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Harden the review sidecar's per-account catalog cap against silent drift: + `contextual_orchestrator_review_launcher.py`'s two + `build_zdr_prioritized_catalog` call sites now source their + `ORCHESTRATOR_CATALOG_ACCOUNT_CAP` fallback from + `contextual_orchestrator_review_policy.DEFAULT_ACCOUNT_CAP` through a new + `_catalog_account_cap()` helper, instead of a hand-typed `"4"` literal. + This closes the exact drift class that produced a real, observed + preflight-budget waste on a separate in-flight branch (a sibling + `_catalog_family_cap()` helper there fell back to the *total* routes + budget instead of the per-account cap, letting two rate-limited NVIDIA + NIM credentials jointly consume all 12 preflight slots, 10 of which were + then rejected via 429/404/timeout). New regression tests pin the default + to the policy module's canonical value and forbid the total-routes + constant from reappearing as the account-cap fallback. - Fix a dangling reference #1468 left in `docs/product-goal-directive.md` (flagged by Devin Review on that PR): the standing operating directive still named the removed `free_family_diversity` evidence field instead of diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 6dbbe2d5e3..f115ef2b88 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -665,6 +665,36 @@ def _bounded_fallback_catalog_limit( return total_limit - primary_count +def _catalog_account_cap(default: int) -> int: + """Return the configured per-account catalog admission cap. + + ``default`` must be ``scripts.ci.contextual_orchestrator_review_policy``'s + own ``DEFAULT_ACCOUNT_CAP`` -- the single source of truth for how many + routes one credential account may contribute to the bounded preflight + budget. A caller must never substitute a total-routes-scale constant + (e.g. ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES``) here: doing so silently + disables per-account diversification and lets one rate-limited account + consume the entire preflight budget. That is not a hypothetical failure + mode -- a sibling in-flight branch's own ``_catalog_family_cap()`` + fell back to exactly ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`` and, in a live + production run, let two NVIDIA NIM credentials sharing one rate-limited + upstream jointly occupy 12/12 preflight slots, of which 10 were then + rejected with 429/404/timeout (see ContextualWisdomLab/.github#1415 and + the "빈 깡통 경로" report it responds to). Routing the default through the + caller-supplied ``policy.DEFAULT_ACCOUNT_CAP`` (rather than hand-typing a + literal here) keeps this module's cap from silently drifting out of sync + with the policy module's own declared intent. + + Args: + default: The cap to use when ``ORCHESTRATOR_CATALOG_ACCOUNT_CAP`` is + unset, always ``policy.DEFAULT_ACCOUNT_CAP``. + + Returns: + The per-account cap to pass to ``build_zdr_prioritized_catalog``. + """ + return int(os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", str(default))) + + def _with_discovery_counts( report: dict[str, object], rows: list[dict[str, Any]], @@ -775,6 +805,7 @@ def main(argv: list[str] | None = None) -> int: ) from contextual_orchestrator.server import SecurityConfig, serve from scripts.ci.contextual_orchestrator_review_policy import ( + DEFAULT_ACCOUNT_CAP, PolicyError, _load_zdr_endpoints, build_zdr_prioritized_catalog, @@ -848,7 +879,7 @@ def main(argv: list[str] | None = None) -> int: result = build_zdr_prioritized_catalog( primary_rows, limit=primary_limit, - account_cap=int(os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", "4")), + account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), zdr_endpoints=zdr_endpoints, require_zdr=args.require_zdr, pool=args.pool, @@ -879,7 +910,7 @@ def main(argv: list[str] | None = None) -> int: fallback_result = build_zdr_prioritized_catalog( admitted_priced_rows, limit=fallback_limit, - account_cap=int(os.environ.get("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", "4")), + account_cap=_catalog_account_cap(DEFAULT_ACCOUNT_CAP), 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 32f1c22413..7093e8a3d0 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1454,6 +1454,57 @@ def test_preflight_stage_limits_share_one_startup_budget() -> None: assert primary + fallback == namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] +def test_catalog_account_cap_defaults_to_the_caller_supplied_policy_default( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The per-account cap falls back to ``policy.DEFAULT_ACCOUNT_CAP``, not the total budget. + + Regression for a real, observed failure mode + (ContextualWisdomLab/.github#1415, reported as "빈 깡통 경로 너무 많다"): a + sibling helper (``_catalog_family_cap()``) fell back to + ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`` -- the *total* preflight budget -- + instead of the intended per-account cap whenever its env var was unset. + That silently disabled per-account diversification: in a live production + run, two NVIDIA NIM credentials sharing one rate-limited upstream jointly + consumed all 12 preflight slots, of which 10 (83%) were then rejected via + 429/404/timeout. This module's own equivalent helper must never resolve + to the same value as the total-routes budget when given the real + ``policy.DEFAULT_ACCOUNT_CAP``, which is strictly smaller. + """ + namespace = _load_launcher() + monkeypatch.delenv("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", raising=False) + cap = namespace["_catalog_account_cap"](policy.DEFAULT_ACCOUNT_CAP) + assert cap == policy.DEFAULT_ACCOUNT_CAP + assert cap != namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] + assert cap < namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] + + +def test_catalog_account_cap_honors_an_explicit_override( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An operator-set ``ORCHESTRATOR_CATALOG_ACCOUNT_CAP`` still takes effect.""" + namespace = _load_launcher() + monkeypatch.setenv("ORCHESTRATOR_CATALOG_ACCOUNT_CAP", "6") + assert namespace["_catalog_account_cap"](policy.DEFAULT_ACCOUNT_CAP) == 6 + + +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``. + + A hand-typed literal (or, worse, a total-routes-scale constant) can + silently drift out of sync with ``policy.DEFAULT_ACCOUNT_CAP`` with no + test catching it -- the exact drift that produced + ContextualWisdomLab/.github#1415's real preflight-budget waste. This + 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. + """ + source = _LAUNCHER.read_text(encoding="utf-8") + assert source.count("account_cap=_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 + + def test_zdr_admission_selects_priced_tier_when_free_routes_are_not_private() -> None: """Privacy admission precedes the free-first tier decision.""" namespace = _load_launcher()