From ec6a1e5e37ed4acfd63f4a08fca667e9a4e58503 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:56:03 +0000 Subject: [PATCH 01/15] fix(ci): stop blanket-stripping OpenRouter rows on evidence_only alone scripts/ci/contextual_orchestrator_review_launcher.py's _routable_discovered_models() unconditionally dropped every discovery row with evidence_only=True. contextual-orchestrator's OpenRouter ProviderModelSource hardcodes evidence_only=True for every discovered model unconditionally -- not computed per model from real evidence, even though genuine per-model ZDR evidence is fetched and parsed for OpenRouter in that same module. The upstream half of this bug is being fixed separately (a dispatched agent, PR forthcoming) -- not touched here. Consequence for this repo: with 100% of OpenRouter rows carrying evidence_only=True, this filter excluded ALL OpenRouter rows before scripts/ci/zdr_policy.py's own purpose-built, already-correct, already- wired per-route OpenRouter ZDR-feed check (is_zdr_model()'s openrouter_endpoints_feed branch) ever got a chance to evaluate a single one -- making that mechanism dead code for OpenRouter specifically, and leaving OpenRouter contributing zero routes to any pool despite genuinely offering ZDR-attested free models via its own documented feed. OpenRouter rows are now exempt from the evidence_only exclusion. A genuinely non-servable OpenRouter row is still excluded downstream by the same provider-agnostic chat-capability check every other provider's rows already go through (is_general_chat_agent_model_id + _has_text_output, in main()) -- this exemption relies on that existing, independent check, not on trusting evidence_only's current, wrong, blanket value for OpenRouter. Sequencing note, verified before writing this: this fix has real, immediate effect once merged, not only once contextual-orchestrator's own evidence_only fix and a matching ORCHESTRATOR_PIN_SHA bump also land. OpenRouter discovery already runs in this sidecar today, and for the general (non-private, require_zdr=False) pool -- what Noema/OpenCode/ default Strix use -- _zdr_admitted_rows() returns every row unfiltered regardless of ZDR status; is_zdr_model() only affects sort priority and tagging there, never admission. So genuinely chat-capable OpenRouter rows start reaching selection as soon as this merges. What remains gated on the upstream fix is OpenRouter rows being correctly excluded from evidence_only on a real per-model basis (e.g. a non-chat listing). Documented in the function's own docstring and this PR description. Tests: test_routable_discovered_models_excludes_evidence_only_rows (existing) corrected to use a non-OpenRouter provider for its evidence_only=True fixture; new test_routable_discovered_models_exempts_openrouter_from_evidence_only confirms both an evidence_only-tagged and untagged OpenRouter row pass through while a same-shaped row from a different provider does not; a contract-test assertion pins the exemption's presence in source. Full suite: 2093 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 12 +++++ docs/product-technical-gap-baseline.md | 52 +++++++++++++++++++ ...contextual_orchestrator_review_launcher.py | 51 +++++++++++++++--- ...l_orchestrator_review_runtime_preflight.py | 46 ++++++++++++++-- ...al_orchestrator_review_sidecar_contract.py | 4 ++ 5 files changed, 155 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 256b0cdf94..17b29e80a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- `scripts/ci/contextual_orchestrator_review_launcher.py`'s `_routable_discovered_models()` + no longer blanket-strips every OpenRouter row on `evidence_only` alone. + `contextual-orchestrator`'s OpenRouter `ProviderModelSource` currently + hardcodes `evidence_only=True` for every discovered model unconditionally + (a confirmed bug, being fixed upstream separately), which was excluding + 100% of OpenRouter discovery rows here before + `zdr_policy.is_zdr_model()`'s purpose-built, per-route OpenRouter ZDR-feed + check ever got a chance to evaluate them -- making that already-correct + mechanism dead code for OpenRouter specifically. OpenRouter rows are now + exempt from this exclusion; a genuinely non-servable OpenRouter row is + still excluded by the existing, provider-agnostic chat-capability check + every other provider's rows already go through. - Fix a dangling reference #1468 left in `docs/product-goal-directive.md` (flagged by Devin Review on that PR): the standing operating directive still named the removed `free_family_diversity` evidence field instead of diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 758ef2961a..ef4c531335 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,58 @@ string, a bare number) confirmed to fail against the pre-fix script (`KeyError: signature as the original round-4 bug) before passing after the fix. 1930 tests pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 `.github`-side half of OpenRouter's premature evidence_only exclusion + +**Confirmed bug.** `scripts/ci/contextual_orchestrator_review_launcher.py`'s `_routable_discovered_models()` +(called at the top of `main()`, before any live-serving selection) unconditionally dropped every discovery +row with `evidence_only=True`. `contextual-orchestrator`'s OpenRouter `ProviderModelSource` hardcodes +`evidence_only=True` for *every* discovered model unconditionally -- not computed per model from real +evidence, even though genuine per-model ZDR evidence (`_openrouter_zdr_model_ids`/`_apply_discovered_ +model_evidence`, feeding the `zdr_capable` field) is fetched and parsed for OpenRouter in that same +module. The upstream half of this bug is being fixed separately (a dispatched agent, PR forthcoming, not +touched here). + +The consequence for this repo specifically: with 100% of OpenRouter rows carrying `evidence_only=True`, +`_routable_discovered_models()` excluded ALL OpenRouter discovery rows before `scripts/ci/zdr_policy.py`'s +own purpose-built, already-correct, already-wired per-route OpenRouter ZDR-feed check +(`is_zdr_model()`'s `openrouter_endpoints_feed` branch, an exact `route_key(provider, model) in +zdr_endpoints` match against OpenRouter's authoritative `/api/v1/endpoints/zdr` feed) ever got a chance to +evaluate a single OpenRouter row -- making that mechanism dead code for OpenRouter specifically, and +leaving OpenRouter contributing zero routes to any pool (free, auto, or ZDR-required private targets), +even though it genuinely offers ZDR-attested free models via its own documented feed. + +**Fix.** OpenRouter rows are now exempt from the `evidence_only` exclusion in `_routable_discovered_ +models()`. A genuinely non-servable OpenRouter row (e.g. a non-chat listing) is still excluded downstream +by the same provider-agnostic chat-capability check every other provider's rows already go through +(`is_general_chat_agent_model_id` + `_has_text_output`, in `main()`) -- so this exemption relies on that +existing, independent check, not on trusting `evidence_only`'s current, wrong, blanket value for +OpenRouter. + +**Sequencing correction, verified before writing this record.** The task as described expected this fix +to be "inert" until both the upstream `contextual-orchestrator` fix and a matching `ORCHESTRATOR_PIN_SHA` +bump land. Traced through the actual code before accepting that: OpenRouter discovery already runs in +this sidecar today (`OPENROUTER_API_KEY` is one of the five KV-registered credentials), and for the +*general* (non-private, `require_zdr=False`) pool -- which is what Noema/OpenCode/the default Strix path +use -- `_zdr_admitted_rows()` returns every row unfiltered regardless of ZDR status; `is_zdr_model()` only +affects sort priority and tagging there, never admission. So this fix has real, immediate effect once +merged: genuinely chat-capable OpenRouter rows, currently blocked here regardless of what +`contextual-orchestrator` reports, start reaching selection as soon as this lands -- not only once the +upstream `evidence_only` fix and pin bump also land. What remains genuinely gated on the upstream fix is +OpenRouter rows being correctly excluded from `evidence_only` on a real per-model basis (a non-chat +listing, say); until then this function's only remaining protection against those is the downstream +chat-capability check, not `evidence_only`. Documented explicitly in `_routable_discovered_models()`'s own +docstring and in the PR description so a reviewer isn't surprised by observable behavior change before the +upstream PR merges. + +**Tests.** `test_routable_discovered_models_excludes_evidence_only_rows` (existing) corrected to use a +non-OpenRouter provider for its `evidence_only=True` fixture, since that scenario no longer applies to +OpenRouter; a new `test_routable_discovered_models_exempts_openrouter_from_evidence_only` regression +confirms both an `evidence_only`-tagged and an untagged OpenRouter row pass through while a same-shaped +row from a different provider does not. A contract-test assertion was added to `test_contextual_ +orchestrator_review_sidecar_contract.py` pinning the exemption's presence in source, matching this +repo's existing pattern of pinning exact prose/structure in trusted scripts. Full suite: 2093 passed, 1 +skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on `scripts/ci/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 6dbbe2d5e3..1578dd1cfc 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -141,14 +141,51 @@ def _log_discovery_errors(errors: list[object]) -> None: def _routable_discovered_models(discovered: list[object] | None) -> list[object]: """Drop evidence-only discovery rows before any live-serving selection. - Evidence-only rows (e.g. the OpenRouter catalog) exist solely to supply - ZDR evidence for other providers' models; contextual_orchestrator's own - ``agent_from_discovered()`` refuses to turn one into a serving agent. - Filtering here keeps that same invariant in this sidecar's selection path, - which builds its catalog independently rather than calling - ``agent_from_discovered()`` directly. + Evidence-only rows (e.g. a provider's pure price/policy-scraping stub) + exist solely to supply metadata for other providers' models; + contextual_orchestrator's own ``agent_from_discovered()`` refuses to + turn one into a serving agent. Filtering here keeps that same invariant + in this sidecar's selection path, which builds its catalog independently + rather than calling ``agent_from_discovered()`` directly. + + OpenRouter rows are deliberately exempt from this exclusion. + ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` + currently hardcodes ``evidence_only=True`` for every discovered model + unconditionally -- not computed per model from real evidence, even + though genuine per-model ZDR evidence is fetched and parsed for + OpenRouter in that same module. Applying this filter to OpenRouter + verbatim would strip every OpenRouter row, including genuinely + servable, chat-capable ones, before ``zdr_policy.is_zdr_model()``'s + purpose-built, per-route OpenRouter ZDR-feed check (``openrouter_ + endpoints_feed``) ever gets a chance to evaluate them -- making that + already-correct, already-wired mechanism dead code for OpenRouter + specifically, and leaving OpenRouter contributing zero routes to any + pool, including the ZDR-attested routes it genuinely offers. A + genuinely non-servable OpenRouter row is still excluded downstream by + the same provider-agnostic chat-capability check every other provider's + rows already go through (``is_general_chat_agent_model_id`` + + ``_has_text_output``, in ``main()``) -- so this exemption relies on + that existing, independent check, not on trusting ``evidence_only``'s + current, wrong, blanket value for OpenRouter. + + This exemption is expected to have real, live effect once merged (not + only once ``contextual-orchestrator``'s own per-model ``evidence_only`` + fix and a matching ``ORCHESTRATOR_PIN_SHA`` bump land): OpenRouter + discovery already runs in this sidecar today, so genuinely chat-capable + OpenRouter rows -- currently blocked here regardless of what + ``contextual-orchestrator`` reports -- start reaching selection + immediately. What remains genuinely blocked on the upstream fix is + OpenRouter rows being correctly excluded from ``evidence_only`` on a + real per-model basis (e.g. a non-chat listing); until then, this + function's remaining protection against those is the same downstream + chat-capability check, not ``evidence_only``. """ - return [model for model in (discovered or []) if not getattr(model, "evidence_only", False)] + return [ + model + for model in (discovered or []) + if not getattr(model, "evidence_only", False) + or getattr(model, "provider_name", None) == "openrouter" + ] def _route_identity(model: object) -> tuple[str, str]: diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 32f1c22413..4a5c2e79e7 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -83,14 +83,14 @@ def _openai_text(content: str) -> dict[str, object]: def test_routable_discovered_models_excludes_evidence_only_rows() -> None: - """Evidence-only rows (e.g. OpenRouter) must never enter live selection.""" + """Evidence-only rows for a non-OpenRouter provider never enter live selection.""" namespace = _load_launcher() routable = namespace.get("_routable_discovered_models") assert callable(routable), "launcher must expose an evidence-only discovery filter" evidence_only_model = SimpleNamespace( - id="openrouter_evidence_only", - provider_name="openrouter", + id="nvidia_evidence_only", + provider_name="nvidia_nim", model_id="some/model", evidence_only=True, ) @@ -112,6 +112,46 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: assert routable([]) == [] +def test_routable_discovered_models_exempts_openrouter_from_evidence_only() -> None: + """OpenRouter rows are never dropped on evidence_only alone. + + Regression for a confirmed bug: ``contextual-orchestrator``'s OpenRouter + ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for + every discovered model unconditionally (not computed per model from + real evidence), which -- if this filter applied to OpenRouter like + every other provider -- would strip every OpenRouter row, including + genuinely servable ones, before ``zdr_policy.is_zdr_model()``'s + purpose-built per-route OpenRouter ZDR-feed check ever runs on them. + Both an evidence-only-tagged and an untagged OpenRouter row must pass + through; a same-shaped row from a different provider must not. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + openrouter_evidence_only = SimpleNamespace( + id="openrouter_evidence_only", + provider_name="openrouter", + model_id="some/model", + evidence_only=True, + ) + openrouter_live = SimpleNamespace( + id="openrouter_ready", + provider_name="openrouter", + model_id="ready/free", + evidence_only=False, + ) + nvidia_evidence_only = SimpleNamespace( + id="nvidia_evidence_only", + provider_name="nvidia_nim", + model_id="some/model", + evidence_only=True, + ) + + assert routable( + [openrouter_evidence_only, openrouter_live, nvidia_evidence_only] + ) == [openrouter_evidence_only, openrouter_live] + + def test_log_discovery_errors_prints_one_bounded_line_per_provider_failure( capsys: pytest.CaptureFixture[str], ) -> None: diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index cbcf690d0b..db1005c99e 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -284,6 +284,10 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert "routable_discovered = _routable_discovered_models(discovered)" in text assert "free_discovered_models(routable_discovered)" in text assert 'getattr(model, "evidence_only", False)' in text + # OpenRouter must stay exempt from the evidence_only exclusion, or + # zdr_policy.is_zdr_model()'s purpose-built per-route OpenRouter ZDR-feed + # check goes back to never seeing an OpenRouter row at all. + assert 'getattr(model, "provider_name", None) == "openrouter"' in text assert 'getattr(model, "output_modalities", None)' in text assert 'isinstance(modalities, str)' in text assert '"text" in {str(modality).casefold() for modality in modalities}' in text From 4adb60ee9eb520c0fb2cd0c345c5950e5bb63a3e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 05:11:44 +0000 Subject: [PATCH 02/15] fix(ci): make the OpenRouter evidence_only exemption self-correcting Devin Review on PR #1476 flagged that the blanket OpenRouter exemption in _routable_discovered_models() doesn't distinguish "vendored contextual-orchestrator still has the confirmed blanket evidence_only=True bug" from "vendored copy now computes evidence_only correctly per model" (the fix in contextual-orchestrator#950, open, not yet merged) -- so once #950 merges and ORCHESTRATOR_PIN_SHA bumps past it, this launcher would keep admitting genuinely evidence-only OpenRouter rows the corrected upstream code means to exclude. Considered gating on ORCHESTRATOR_PIN_SHA via git ancestry (merge-base --is-ancestor against #950's eventual merge commit, reachable from the sidecar's already-full vendored clone) but #950 has no merge commit yet, so there is no concrete threshold to gate on, and wiring the plumbing now (new CLI arg/env var, subprocess git call, sidecar/contract changes) would be built against a value that doesn't exist. Implemented instead: _openrouter_reports_per_model_evidence() reads this run's own discovered OpenRouter rows and turns the exemption off the moment any row reports evidence_only=False (real per-model evidence). While every row still reports True (today's exact bug signature), the exemption stays active. Self-corrects with no pin tracking and no manual conversion step once #950 merges. Known, accepted limitation documented in the docstring: a genuinely-fixed vendored copy that happens to report all-True in one run (feed-fetch failure, or zero attested models that run) is indistinguishable from the still-buggy signature by this check alone. Tests: split the previous mixed-fixture regression into test_routable_discovered_models_exempts_openrouter_when_every_row_reports_evidence_only (pre-fix blanket-True shape) and test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence (post-fix mixed shape). Full suite: 2094 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on scripts/ci/. Follow-up recorded in docs/product-technical-gap-baseline.md with an explicit TODO referencing contextual-orchestrator#950 and this repo's #1476. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 64 +++++++++ ...contextual_orchestrator_review_launcher.py | 130 +++++++++++++++--- ...l_orchestrator_review_runtime_preflight.py | 76 +++++++--- 3 files changed, 234 insertions(+), 36 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index ef4c531335..1d8ea00b9a 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1767,6 +1767,70 @@ orchestrator_review_sidecar_contract.py` pinning the exemption's presence in sou repo's existing pattern of pinning exact prose/structure in trusted scripts. Full suite: 2093 passed, 1 skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 follow-up: making the OpenRouter `evidence_only` exemption self-correcting + +**Gap raised by Devin Review on `ContextualWisdomLab/.github#1476`.** The blanket exemption above does not +distinguish "the vendored `contextual-orchestrator` still has the confirmed blanket-`evidence_only=True` +bug" from "the vendored copy now computes `evidence_only` correctly per model" (the fix proposed in +`ContextualWisdomLab/contextual-orchestrator#950`, **open, not yet merged** as of this entry). Left +unconditional forever, this launcher would keep admitting genuinely evidence-only (non-ZDR-attested) +OpenRouter rows even after `#950` merges and `ORCHESTRATOR_PIN_SHA` is bumped past it -- silently defeating +the very fix `#950` delivers, for exactly the population of rows `evidence_only` exists to gate. + +**Design considered: pin-SHA ancestry.** `#950`'s base SHA (`c107e3e52371993aa9c326fcc245e01c41fc3850`) +is confirmed to equal this repo's current `ORCHESTRATOR_PIN_SHA` default +(`scripts/ci/contextual_orchestrator_review_sidecar.sh`), so once `#950` merges its resulting SHA on +`contextual-orchestrator`'s `main` becomes the natural gating threshold. The sidecar's vendored clone at +`$ORCHESTRATOR_SOURCE` is a non-shallow (`--filter=blob:none`, full commit/tree history, blobs only) clone +of every ref, so `git -C "$ORCHESTRATOR_SOURCE" merge-base --is-ancestor "$ORCHESTRATOR_PIN_SHA"` +is technically reachable at review-time. It was not implemented now: `#950` has not merged, so no concrete +fix-commit SHA exists yet to gate on, and wiring the check in ahead of that would require new plumbing +(passing `$ORCHESTRATOR_SOURCE` or the pin itself into the launcher via a new CLI argument/env var, a +`subprocess` git call, and matching `contextual_orchestrator_review_sidecar.sh` / contract-test changes) +built against a threshold this org does not yet have -- over-engineering ahead of the actual need. + +**Fix implemented instead: an observed-behavior signature check, not a version marker.** +`_openrouter_reports_per_model_evidence()` (`scripts/ci/contextual_orchestrator_review_launcher.py`) reads +this run's own discovered OpenRouter rows: if at least one reports `evidence_only=False`, that is real +per-model evidence, and `_routable_discovered_models()` immediately stops exempting OpenRouter and applies +the same `evidence_only` contract every other provider already gets -- unattested OpenRouter rows are +excluded, attested ones pass on their own merit. While every OpenRouter row still reports +`evidence_only=True` (today's exact, confirmed bug signature), the historical exemption stays active. This +needs no pin tracking, no `subprocess` calls, and no manual conversion step once `#950` merges: the check +self-corrects the moment the vendored pin actually includes the fix and a run observes real per-model +variation, because it is reading the vendored code's actual output rather than trusting a commit SHA to +imply that output. Documented as the more robust, less brittle choice for this reason in +`_openrouter_reports_per_model_evidence()`'s own docstring. + +**Known, accepted limitation, documented in the same docstring.** A genuinely-fixed vendored copy that +reports `evidence_only=True` for every OpenRouter row in one particular run -- a total ZDR-feed-fetch +failure that run (`#950`'s own documented fail-closed behavior), or simply zero ZDR-attested OpenRouter +models discovered that run -- is indistinguishable from the still-buggy blanket signature by this check +alone, and the exemption stays active for that one run. This only widens which OpenRouter rows reach the +same downstream, `evidence_only`-independent chat-capability check every other provider's rows already +pass through; it does not touch the separate ZDR admission gate (`is_zdr_model()` / `_zdr_admitted_rows()`) +that guards `--require-zdr` private targets, which never depended on `evidence_only` in the first place. + +**TODO, tracked here explicitly (`ContextualWisdomLab/contextual-orchestrator#950`, +`ContextualWisdomLab/.github#1476`):** once `#950` merges, re-verify this reasoning holds under its actual +merged test suite (the negative case `test_discover_all_models_openrouter_model_absent_from_zdr_feed_stays_ +evidence_only` and the feed-failure case `test_discover_all_models_openrouter_zdr_feed_failure_keeps_every_ +row_evidence_only`, per `#950`'s own description) and once `ORCHESTRATOR_PIN_SHA` is bumped past it, confirm +in a real CI run that `_openrouter_reports_per_model_evidence()` observes the expected per-model variation +and the exemption turns itself off with no code change required. If real-world experience ever shows the +observed-behavior check's known limitation above firing often enough to matter (e.g. OpenRouter's ZDR feed +proves flaky in practice), revisit the pin-ancestry alternative recorded here, now that a concrete +fix-commit SHA would exist to gate on. + +**Tests.** `test_routable_discovered_models_exempts_openrouter_from_evidence_only` (previous entry's +regression) split into two: `test_routable_discovered_models_exempts_openrouter_when_every_row_reports_ +evidence_only` (blanket-`True` pre-fix signature -- both OpenRouter rows pass) and +`test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence` (mixed +post-fix signature -- the unattested row is now excluded, matching the same-shaped non-OpenRouter case). +Full suite: 2094 passed, 1 skipped, 21 subtests passed. 100% coverage (the launcher stays coverage-omitted +per `pyproject.toml`'s existing, unchanged rationale -- it imports the vendored library only present inside +the sidecar's own runtime) and 100% docstring coverage on `scripts/ci/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 1578dd1cfc..e1217e2b80 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -138,6 +138,90 @@ def _log_discovery_errors(errors: list[object]) -> None: print(_DISCOVERY_DIAGNOSTICS_COMPLETE_SENTINEL, file=sys.stderr, flush=True) +def _openrouter_reports_per_model_evidence(discovered: list[object]) -> bool: + """Return whether this run's OpenRouter rows show real per-model evidence. + + ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` + currently has a confirmed bug (``ContextualWisdomLab/contextual- + orchestrator#950``, open, not yet merged as of this writing): it + hardcodes ``evidence_only=True`` for *every* discovered OpenRouter + model unconditionally, at the provider-source level, regardless of that + specific model's own evidence -- so today's real, observable signature + is that literally every discovered OpenRouter row carries + ``evidence_only=True``, with zero exceptions, even for genuinely + servable, ZDR-attested models. #950 fixes this by computing + ``evidence_only``/``zdr_capable`` per model instead, from OpenRouter's + own ``/api/v1/endpoints/zdr`` feed match -- so a fixed vendored copy + reports ``evidence_only=False`` for at least the subset of OpenRouter + models that are genuinely ZDR-attested (whenever OpenRouter offers any + such models, which it does today), while unattested models correctly + stay ``True``. + + This function is this run's *observed-behavior* signal for which of + those two shapes is currently vendored, used by + ``_routable_discovered_models`` in place of tracking + ``ORCHESTRATOR_PIN_SHA`` (the sidecar's vendored-commit pin, defined in + ``scripts/ci/contextual_orchestrator_review_sidecar.sh``) directly. A + pin-ancestry check (``git merge-base --is-ancestor <950's merge commit> + "$ORCHESTRATOR_PIN_SHA"``) was considered and is technically reachable + -- the sidecar's vendored clone at ``$ORCHESTRATOR_SOURCE`` keeps full + commit history (only blob content is filtered) -- but #950 has not + merged yet, so no fix commit SHA exists to compare against, and wiring + one in now would need a new CLI argument/env var carrying + ``$ORCHESTRATOR_SOURCE`` (or the pin itself) into this launcher, a + ``subprocess`` git call, and matching sidecar-script/contract-test + changes -- real plumbing built against a threshold value that does not + exist yet. This observed-behavior check needs none of that: it reads + the same ``discovered`` rows this function already receives, requires + no new plumbing, and -- unlike a pin comparison -- keeps working even + if a fix is ever backported to the vendored fork without a matching + ``ORCHESTRATOR_PIN_SHA`` bump, since it looks at what the vendored code + actually returned this run rather than which commit is nominally + pinned. See ``ContextualWisdomLab/.github#1476`` (this change) and + ``ContextualWisdomLab/contextual-orchestrator#950`` (the upstream fix) + for the full history; once #950 merges, no further change is required + here -- this check starts reporting ``True`` as soon as the vendored + pin actually includes the fix and OpenRouter has at least one + ZDR-attested free model in a given run. + + KNOWN, ACCEPTED LIMITATION: a genuinely-fixed vendored copy that + happens to report ``evidence_only=True`` for *every* OpenRouter row in + one particular run -- because the ZDR feed fetch failed entirely that + run (#950's own documented fail-closed behavior), or because none of + the OpenRouter models discovered that run happen to be ZDR-attested -- + is indistinguishable from the still-buggy blanket signature by this + check alone, and the historical exemption stays active for that one + run. This mirrors the fail-open-on-ambiguity reasoning already used + elsewhere in this module (e.g. the KNOWN GAP entries above) rather than + inventing a new policy; a false-negative here only widens which + OpenRouter rows reach the same downstream, provider-agnostic + chat-capability check every other provider's rows already pass through + (``is_general_chat_agent_model_id`` + ``_has_text_output``, in + ``main()``) -- it does not bypass the separate, ``evidence_only``- + independent ZDR admission gate (``is_zdr_model()`` / + ``_zdr_admitted_rows()``) that guards private, ``--require-zdr`` + targets. + + Args: + discovered: The full discovery-wide row set for this run (already + filtered to nothing upstream -- this must see every row, + including non-OpenRouter ones, though only OpenRouter rows are + inspected). + + Returns: + ``True`` once at least one discovered OpenRouter row reports + ``evidence_only=False`` (real per-model evidence observed this + run); ``False`` when there are no OpenRouter rows at all, or every + OpenRouter row is ``evidence_only=True`` (today's confirmed bug + signature, or an indistinguishable fixed-but-all-unattested run). + """ + return any( + getattr(model, "provider_name", None) == "openrouter" + and not getattr(model, "evidence_only", False) + for model in discovered + ) + + def _routable_discovered_models(discovered: list[object] | None) -> list[object]: """Drop evidence-only discovery rows before any live-serving selection. @@ -148,25 +232,31 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] in this sidecar's selection path, which builds its catalog independently rather than calling ``agent_from_discovered()`` directly. - OpenRouter rows are deliberately exempt from this exclusion. - ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` - currently hardcodes ``evidence_only=True`` for every discovered model - unconditionally -- not computed per model from real evidence, even - though genuine per-model ZDR evidence is fetched and parsed for - OpenRouter in that same module. Applying this filter to OpenRouter - verbatim would strip every OpenRouter row, including genuinely + OpenRouter rows are conditionally exempt from this exclusion -- + see ``_openrouter_reports_per_model_evidence`` for the full rationale + and its documented limitation. In short: ``contextual-orchestrator``'s + OpenRouter ``ProviderModelSource`` currently hardcodes + ``evidence_only=True`` for every discovered model unconditionally (a + confirmed bug, ``ContextualWisdomLab/contextual-orchestrator#950``, not + yet merged) -- not computed per model from real evidence, even though + genuine per-model ZDR evidence is fetched and parsed for OpenRouter in + that same module. Applying this filter to OpenRouter verbatim while + that bug is live would strip every OpenRouter row, including genuinely servable, chat-capable ones, before ``zdr_policy.is_zdr_model()``'s purpose-built, per-route OpenRouter ZDR-feed check (``openrouter_ endpoints_feed``) ever gets a chance to evaluate them -- making that already-correct, already-wired mechanism dead code for OpenRouter - specifically, and leaving OpenRouter contributing zero routes to any - pool, including the ZDR-attested routes it genuinely offers. A + specifically. So the exemption applies only while this run's own + OpenRouter rows still match that exact blanket-``True`` bug signature; + the moment any OpenRouter row in a run reports real per-model evidence + (``evidence_only=False``), OpenRouter rows go back through the same + ``evidence_only`` contract every other provider already gets -- + automatically, with no pin bump or manual edit needed here. A genuinely non-servable OpenRouter row is still excluded downstream by the same provider-agnostic chat-capability check every other provider's rows already go through (``is_general_chat_agent_model_id`` + - ``_has_text_output``, in ``main()``) -- so this exemption relies on - that existing, independent check, not on trusting ``evidence_only``'s - current, wrong, blanket value for OpenRouter. + ``_has_text_output``, in ``main()``) regardless of which branch this + function takes. This exemption is expected to have real, live effect once merged (not only once ``contextual-orchestrator``'s own per-model ``evidence_only`` @@ -176,15 +266,21 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] ``contextual-orchestrator`` reports -- start reaching selection immediately. What remains genuinely blocked on the upstream fix is OpenRouter rows being correctly excluded from ``evidence_only`` on a - real per-model basis (e.g. a non-chat listing); until then, this - function's remaining protection against those is the same downstream - chat-capability check, not ``evidence_only``. + real per-model basis (e.g. a non-chat listing); until #950 merges and + a run observes real per-model variation, this function's remaining + protection against those is the same downstream chat-capability check, + not ``evidence_only``. """ + discovered = list(discovered or []) + openrouter_still_blanket_marked = not _openrouter_reports_per_model_evidence(discovered) return [ model - for model in (discovered or []) + for model in discovered if not getattr(model, "evidence_only", False) - or getattr(model, "provider_name", None) == "openrouter" + or ( + openrouter_still_blanket_marked + and getattr(model, "provider_name", None) == "openrouter" + ) ] diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 4a5c2e79e7..68bf873d46 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -112,33 +112,38 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: assert routable([]) == [] -def test_routable_discovered_models_exempts_openrouter_from_evidence_only() -> None: - """OpenRouter rows are never dropped on evidence_only alone. - - Regression for a confirmed bug: ``contextual-orchestrator``'s OpenRouter - ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for - every discovered model unconditionally (not computed per model from - real evidence), which -- if this filter applied to OpenRouter like - every other provider -- would strip every OpenRouter row, including - genuinely servable ones, before ``zdr_policy.is_zdr_model()``'s - purpose-built per-route OpenRouter ZDR-feed check ever runs on them. - Both an evidence-only-tagged and an untagged OpenRouter row must pass - through; a same-shaped row from a different provider must not. +def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_evidence_only() -> None: + """OpenRouter rows are exempt from evidence_only while every row still shows it. + + Regression for a confirmed bug (``ContextualWisdomLab/contextual- + orchestrator#950``, open, not yet merged): ``contextual-orchestrator``'s + OpenRouter ``ProviderModelSource`` currently hardcodes + ``evidence_only=True`` for every discovered model unconditionally (not + computed per model from real evidence) -- so today's real signature is + that *every* discovered OpenRouter row carries ``evidence_only=True``, + with no exceptions, even genuinely servable ones. If this filter + applied to OpenRouter like every other provider while that bug is + live, it would strip every OpenRouter row, including genuinely + servable ones, before ``zdr_policy.is_zdr_model()``'s purpose-built + per-route OpenRouter ZDR-feed check ever runs on them. Both OpenRouter + rows here carry ``evidence_only=True`` (today's real bug shape) and + both must still pass through; a same-shaped row from a different + provider must not. """ namespace = _load_launcher() routable = namespace["_routable_discovered_models"] - openrouter_evidence_only = SimpleNamespace( - id="openrouter_evidence_only", + openrouter_evidence_only_a = SimpleNamespace( + id="openrouter_evidence_only_a", provider_name="openrouter", model_id="some/model", evidence_only=True, ) - openrouter_live = SimpleNamespace( - id="openrouter_ready", + openrouter_evidence_only_b = SimpleNamespace( + id="openrouter_evidence_only_b", provider_name="openrouter", model_id="ready/free", - evidence_only=False, + evidence_only=True, ) nvidia_evidence_only = SimpleNamespace( id="nvidia_evidence_only", @@ -148,8 +153,41 @@ def test_routable_discovered_models_exempts_openrouter_from_evidence_only() -> N ) assert routable( - [openrouter_evidence_only, openrouter_live, nvidia_evidence_only] - ) == [openrouter_evidence_only, openrouter_live] + [openrouter_evidence_only_a, openrouter_evidence_only_b, nvidia_evidence_only] + ) == [openrouter_evidence_only_a, openrouter_evidence_only_b] + + +def test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence() -> None: + """The historical exemption turns off the moment per-model evidence appears. + + Once ``ContextualWisdomLab/contextual-orchestrator#950`` merges and the + vendored pin advances past it, OpenRouter starts reporting real + per-model ``evidence_only`` (at minimum ``False`` for its genuinely + ZDR-attested free models). This is the post-fix signature: a run whose + OpenRouter rows are no longer uniformly ``True`` must go back through + the same ``evidence_only`` contract every other provider's rows + already get -- an attested row still passes (it always would have, on + its own merit), but an unattested OpenRouter row is now excluded here + exactly like a same-shaped row from any other provider, with no pin + bump or manual code edit required to reach this behavior. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + openrouter_attested = SimpleNamespace( + id="openrouter_attested", + provider_name="openrouter", + model_id="attested/free", + evidence_only=False, + ) + openrouter_unattested = SimpleNamespace( + id="openrouter_unattested", + provider_name="openrouter", + model_id="unattested/model", + evidence_only=True, + ) + + assert routable([openrouter_attested, openrouter_unattested]) == [openrouter_attested] def test_log_discovery_errors_prints_one_bounded_line_per_provider_failure( From a5f2b29057c0de4f6d8492c698e2205debfcde7e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 05:32:31 +0000 Subject: [PATCH 03/15] fix(ci): reconcile #950->#949, gate spend_admitted, close ZDR-bypass finding contextual-orchestrator#950 (cited by this PR as "the upstream half of this bug, not yet merged") was closed as redundant/superseded. The fix actually merged as contextual-orchestrator#949 ("fix(discovery): route OpenRouter by model evidence", 8cd99f139915131ba0239bce12a5d6a5fd85394e); .github#1477 already advances ORCHESTRATOR_PIN_SHA to that commit. Corrects every stale #950 reference in the PR's own docstrings and docs/product-technical-gap- baseline.md, and folds in two things only visible from #949's actual diff: - #949 also added DiscoveredModel.spend_admitted (default True, False for a priced OpenRouter row when openrouter_paid_inference_available() cannot confirm usable credit). orchestrator/free never considers priced rows, so it was never exposed to this, but orchestrator/auto (real, reachable via CONTEXTUAL_ORCHESTRATOR_POOL=auto, no other code change needed) does consider priced rows and had no spend_admitted check anywhere in this repo's pipeline. _routable_discovered_models() now excludes a spend_admitted=False row the same way it excludes evidence_only=True, with regression coverage including an end-to-end auto-pool composition. - A fresh Devin Review red finding on this PR argued the OpenRouter evidence_only exemption could let private/--require-zdr review content reach ZDR-forbidden routes. Traced end to end: build_zdr_prioritized_ catalog() independently re-applies is_zdr_model()'s real OpenRouter ZDR- feed check as its own admission gate whenever require_zdr=True, entirely independent of evidence_only. Confirmed false alarm with a regression test proving a non-ZDR-attested OpenRouter row is excluded from a require_zdr=True catalog even while every discovered OpenRouter row still carries the evidence_only=True bug signature; documented in the gap baseline and replied/resolved on the GitHub review thread. docs/product-technical-gap-baseline.md gets a new 2026-08-31 correction subsection recording all of the above. Full suite: 2098 passed, 1 skipped, 21 subtests passed; 100% coverage on scripts/ci/; 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 186 +++++++++++---- ...contextual_orchestrator_review_launcher.py | 142 +++++++----- ...l_orchestrator_review_runtime_preflight.py | 215 +++++++++++++++++- 3 files changed, 448 insertions(+), 95 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 1d8ea00b9a..5c549fc49d 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1723,8 +1723,9 @@ row with `evidence_only=True`. `contextual-orchestrator`'s OpenRouter `ProviderM `evidence_only=True` for *every* discovered model unconditionally -- not computed per model from real evidence, even though genuine per-model ZDR evidence (`_openrouter_zdr_model_ids`/`_apply_discovered_ model_evidence`, feeding the `zdr_capable` field) is fetched and parsed for OpenRouter in that same -module. The upstream half of this bug is being fixed separately (a dispatched agent, PR forthcoming, not -touched here). +module. The upstream half of this bug was fixed separately, in +`ContextualWisdomLab/contextual-orchestrator#949` ("fix(discovery): route OpenRouter by model evidence"), +merged at `8cd99f139915131ba0239bce12a5d6a5fd85394e`. The consequence for this repo specifically: with 100% of OpenRouter rows carrying `evidence_only=True`, `_routable_discovered_models()` excluded ALL OpenRouter discovery rows before `scripts/ci/zdr_policy.py`'s @@ -1771,23 +1772,31 @@ skipped, 21 subtests passed. 100% coverage and 100% docstring coverage on `scrip **Gap raised by Devin Review on `ContextualWisdomLab/.github#1476`.** The blanket exemption above does not distinguish "the vendored `contextual-orchestrator` still has the confirmed blanket-`evidence_only=True` -bug" from "the vendored copy now computes `evidence_only` correctly per model" (the fix proposed in -`ContextualWisdomLab/contextual-orchestrator#950`, **open, not yet merged** as of this entry). Left -unconditional forever, this launcher would keep admitting genuinely evidence-only (non-ZDR-attested) -OpenRouter rows even after `#950` merges and `ORCHESTRATOR_PIN_SHA` is bumped past it -- silently defeating -the very fix `#950` delivers, for exactly the population of rows `evidence_only` exists to gate. - -**Design considered: pin-SHA ancestry.** `#950`'s base SHA (`c107e3e52371993aa9c326fcc245e01c41fc3850`) -is confirmed to equal this repo's current `ORCHESTRATOR_PIN_SHA` default -(`scripts/ci/contextual_orchestrator_review_sidecar.sh`), so once `#950` merges its resulting SHA on -`contextual-orchestrator`'s `main` becomes the natural gating threshold. The sidecar's vendored clone at -`$ORCHESTRATOR_SOURCE` is a non-shallow (`--filter=blob:none`, full commit/tree history, blobs only) clone -of every ref, so `git -C "$ORCHESTRATOR_SOURCE" merge-base --is-ancestor "$ORCHESTRATOR_PIN_SHA"` -is technically reachable at review-time. It was not implemented now: `#950` has not merged, so no concrete -fix-commit SHA exists yet to gate on, and wiring the check in ahead of that would require new plumbing -(passing `$ORCHESTRATOR_SOURCE` or the pin itself into the launcher via a new CLI argument/env var, a -`subprocess` git call, and matching `contextual_orchestrator_review_sidecar.sh` / contract-test changes) -built against a threshold this org does not yet have -- over-engineering ahead of the actual need. +bug" from "the vendored copy now computes `evidence_only` correctly per model" (the fix originally tracked +here as `ContextualWisdomLab/contextual-orchestrator#950` — **since corrected: `#950` was closed as +redundant/superseded, and the fix instead merged as `ContextualWisdomLab/contextual-orchestrator#949`**, +"fix(discovery): route OpenRouter by model evidence", at `8cd99f139915131ba0239bce12a5d6a5fd85394e`; see +the 2026-08-31 correction subsection below). Left unconditional forever, this launcher would keep admitting +genuinely evidence-only (non-ZDR-attested) OpenRouter rows even after `#949` merges and +`ORCHESTRATOR_PIN_SHA` is bumped past it -- silently defeating the very fix `#949` delivers, for exactly the +population of rows `evidence_only` exists to gate. + +**Design considered: pin-SHA ancestry.** The base SHA this design work compared against +(`c107e3e52371993aa9c326fcc245e01c41fc3850`) is confirmed to equal this repo's then-current +`ORCHESTRATOR_PIN_SHA` default (`scripts/ci/contextual_orchestrator_review_sidecar.sh`), so once a fix +merged upstream its resulting SHA on `contextual-orchestrator`'s `main` would become the natural gating +threshold. The sidecar's vendored clone at `$ORCHESTRATOR_SOURCE` is a non-shallow (`--filter=blob:none`, +full commit/tree history, blobs only) clone of every ref, so +`git -C "$ORCHESTRATOR_SOURCE" merge-base --is-ancestor "$ORCHESTRATOR_PIN_SHA"` is technically +reachable at review-time. It was not implemented at the time: neither candidate fix (`#950`, later closed; +`#949`, the one that actually merged) had landed yet, so no concrete fix-commit SHA existed to gate on, and +wiring the check in ahead of that would have required new plumbing (passing `$ORCHESTRATOR_SOURCE` or the +pin itself into the launcher via a new CLI argument/env var, a `subprocess` git call, and matching +`contextual_orchestrator_review_sidecar.sh` / contract-test changes) built against a threshold this org did +not yet have -- over-engineering ahead of the actual need. `#949` has since merged and +`ContextualWisdomLab/.github#1477` (open as of this correction) advances `ORCHESTRATOR_PIN_SHA` straight to +its merge commit, so a concrete fix SHA now exists -- but the observed-behavior check below already covers +the need without this plumbing, so pin-ancestry tracking remains unimplemented by choice, not by necessity. **Fix implemented instead: an observed-behavior signature check, not a version marker.** `_openrouter_reports_per_model_evidence()` (`scripts/ci/contextual_orchestrator_review_launcher.py`) reads @@ -1796,31 +1805,42 @@ per-model evidence, and `_routable_discovered_models()` immediately stops exempt the same `evidence_only` contract every other provider already gets -- unattested OpenRouter rows are excluded, attested ones pass on their own merit. While every OpenRouter row still reports `evidence_only=True` (today's exact, confirmed bug signature), the historical exemption stays active. This -needs no pin tracking, no `subprocess` calls, and no manual conversion step once `#950` merges: the check +needs no pin tracking, no `subprocess` calls, and no manual conversion step once `#949` merges: the check self-corrects the moment the vendored pin actually includes the fix and a run observes real per-model variation, because it is reading the vendored code's actual output rather than trusting a commit SHA to imply that output. Documented as the more robust, less brittle choice for this reason in -`_openrouter_reports_per_model_evidence()`'s own docstring. +`_openrouter_reports_per_model_evidence()`'s own docstring. `#949`'s actual merged diff confirms this +premise directly: it removes the `evidence_only=True` hardcode from OpenRouter's `ProviderModelSource` +entirely rather than computing a per-model value, so `DiscoveredModel.evidence_only` defaults to `False` +for every OpenRouter row the moment the pin is bumped past it -- exactly the "at least one row reports +`evidence_only=False`" signature this check watches for. **Known, accepted limitation, documented in the same docstring.** A genuinely-fixed vendored copy that reports `evidence_only=True` for every OpenRouter row in one particular run -- a total ZDR-feed-fetch -failure that run (`#950`'s own documented fail-closed behavior), or simply zero ZDR-attested OpenRouter -models discovered that run -- is indistinguishable from the still-buggy blanket signature by this check -alone, and the exemption stays active for that one run. This only widens which OpenRouter rows reach the -same downstream, `evidence_only`-independent chat-capability check every other provider's rows already -pass through; it does not touch the separate ZDR admission gate (`is_zdr_model()` / `_zdr_admitted_rows()`) -that guards `--require-zdr` private targets, which never depended on `evidence_only` in the first place. - -**TODO, tracked here explicitly (`ContextualWisdomLab/contextual-orchestrator#950`, -`ContextualWisdomLab/.github#1476`):** once `#950` merges, re-verify this reasoning holds under its actual -merged test suite (the negative case `test_discover_all_models_openrouter_model_absent_from_zdr_feed_stays_ -evidence_only` and the feed-failure case `test_discover_all_models_openrouter_zdr_feed_failure_keeps_every_ -row_evidence_only`, per `#950`'s own description) and once `ORCHESTRATOR_PIN_SHA` is bumped past it, confirm -in a real CI run that `_openrouter_reports_per_model_evidence()` observes the expected per-model variation -and the exemption turns itself off with no code change required. If real-world experience ever shows the -observed-behavior check's known limitation above firing often enough to matter (e.g. OpenRouter's ZDR feed -proves flaky in practice), revisit the pin-ancestry alternative recorded here, now that a concrete -fix-commit SHA would exist to gate on. +failure that run (`#949`'s own documented fail-closed behavior, restated in its ADR 0032 update: "Missing +or failed ZDR evidence therefore fails closed only for `zdr_only` selection, not for general inference"), +or simply zero ZDR-attested OpenRouter models discovered that run -- is indistinguishable from the +still-buggy blanket signature by this check alone, and the exemption stays active for that one run. This +only widens which OpenRouter rows reach the same downstream, `evidence_only`-independent chat-capability +check every other provider's rows already pass through; it does not touch the separate ZDR admission gate +(`is_zdr_model()` / `_zdr_admitted_rows()`) that guards `--require-zdr` private targets, which never +depended on `evidence_only` in the first place (see the 2026-08-31 correction subsection below, which +traces this claim end to end against a second, independent Devin Review finding that questioned it). + +**TODO, resolved by the 2026-08-31 correction below.** The original TODO here asked to re-verify this +reasoning once `#950` merged and `ORCHESTRATOR_PIN_SHA` was bumped past it. `#950` never merged (closed as +redundant/superseded); `#949` merged instead. That re-verification against `#949`'s actual merged diff and +test suite is now done (2026-08-31 correction subsection below) -- `#949`'s tests exercise the same +"OpenRouter model absent from the ZDR feed keeps that row correctly gated" and "ZDR-feed-fetch failure fails +closed" shapes this TODO named, under different test names than originally guessed +(`test_discover_all_models_blocks_only_paid_openrouter_without_credit` and the existing ZDR-feed tests in +`tests/test_model_discovery.py`, per `#949`'s diff). The one action still pending is operational, not +analytical: confirm in a real CI run, once `ContextualWisdomLab/.github#1477` merges and +`ORCHESTRATOR_PIN_SHA` actually advances, that `_openrouter_reports_per_model_evidence()` observes the +expected per-model variation and the exemption turns itself off with no further code change. If real-world +experience ever shows the observed-behavior check's known limitation above firing often enough to matter +(e.g. OpenRouter's ZDR feed proves flaky in practice), revisit the pin-ancestry alternative recorded above, +now that a concrete fix-commit SHA (`8cd99f139915131ba0239bce12a5d6a5fd85394e`) exists to gate on. **Tests.** `test_routable_discovered_models_exempts_openrouter_from_evidence_only` (previous entry's regression) split into two: `test_routable_discovered_models_exempts_openrouter_when_every_row_reports_ @@ -1831,6 +1851,96 @@ Full suite: 2094 passed, 1 skipped, 21 subtests passed. 100% coverage (the launc per `pyproject.toml`'s existing, unchanged rationale -- it imports the vendored library only present inside the sidecar's own runtime) and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 correction: #949 merged (not #950), `spend_admitted` traced, a second Devin finding closed + +**Correction: the upstream PR number.** Both entries above, and `ContextualWisdomLab/.github#1476`'s own +original PR body, named `ContextualWisdomLab/contextual-orchestrator#950` as "the upstream half of this +bug, not yet merged." That was wrong. `#950` was closed as redundant/superseded. The PR that actually +merged is a different one, `ContextualWisdomLab/contextual-orchestrator#949` ("fix(discovery): route +OpenRouter by model evidence"), at `8cd99f139915131ba0239bce12a5d6a5fd85394e`. Separately, +`ContextualWisdomLab/.github#1477` (open as of this correction) already bumped this repo's +`ORCHESTRATOR_PIN_SHA` (`scripts/ci/contextual_orchestrator_review_sidecar.sh`) to that exact commit, +pending `#1477`'s own merge. Every other reference to `#950` above is corrected in place; the reasoning +itself needed no other change -- it was always about the *behavior* the observed-behavior check watches +for, not about which PR number delivered it. + +**New fact from `#949`'s actual diff, not knowable from the original entries: `spend_admitted`.** `#949` did +more than compute `evidence_only`/`zdr_capable` per model. It also added a new field, +`spend_admitted: bool = True`, to `DiscoveredModel` (`contextual_orchestrator/model_discovery.py`), and a +new `apply_openrouter_spend_admission()` helper: for a **priced** (non-`is_free`) OpenRouter row, whenever +`openrouter_paid_inference_available()` does not affirmatively return `True` (i.e. returns `False` or +`None` -- no usable credit, or the check itself failed), that row's `spend_admitted` becomes `False` +(fail-closed). A **free** OpenRouter row's `spend_admitted` is always `True`, unconditionally, regardless of +credit status -- `apply_openrouter_spend_admission` short-circuits on `model.is_free`. `is_routable_ +discovered_model()` (the vendored library's own agent-activation gate) was updated to require +`spend_admitted` in addition to `not evidence_only`, and `agent_from_discovered()`/`serving_tags_for_ +discovered()` now tag a blocked row `spend:blocked`. + +**Investigated: does this repo's own review-catalog pipeline need to respect `spend_admitted`, or is it +already safe without it?** Traced `scripts/ci/contextual_orchestrator_review_launcher.py`'s `main()` in +full. It calls `discover_all_models()` directly and receives real `DiscoveredModel` rows (not some +already-filtered surface), so `spend_admitted` genuinely reaches this repo's code -- but only priced rows +can ever have `spend_admitted=False` (see above), and this launcher's default, and *every* current call +site's actual configured pool (`CONTEXTUAL_ORCHESTRATOR_POOL`, unset almost everywhere and explicitly `free` +in `strix.yml`), is `--pool free`. Under `--pool free`, `main()`'s `selected_models` loop drops every row not +in `free_route_identities` *before* it ever becomes a report row (`if args.pool == "free" and +_route_identity(model) not in free_route_identities: continue`) -- so no priced row, and therefore no +`spend_admitted=False` row, ever reaches this repo's catalog today. `scripts/ci/contextual_orchestrator_ +review_policy.py`'s `build_zdr_prioritized_catalog()` reinforces this independently: for `pool="free"` its +`candidate_rows` is `all_free_rows` only, never `all_priced_rows`. + +**The real, latent gap: `--pool auto`.** `--pool auto` is real, tested, wired code -- selectable today via +the `CONTEXTUAL_ORCHESTRATOR_POOL=auto` environment variable with no further code change, even though no +current workflow sets it. Under `auto`, priced rows are genuine candidates (`primary_rows = admitted_free_ +rows or admitted_priced_rows`, plus an explicit priced-fallback stage in `main()` and `[*all_free_rows, +*all_priced_rows]` in `build_zdr_prioritized_catalog()`). Neither of those priced-row paths, nor +`_report_rows()` (which builds report rows from selected `DiscoveredModel`s), ever read or propagated +`spend_admitted` -- so before this correction, a spend-blocked (credit-exhausted) paid OpenRouter row could +reach `orchestrator/auto`'s served catalog exactly as if it were servable. This matches this org's stated +direction that the review catalog is meant to be free+ZDR-only ("free+ZDR 조합도 해결 못하는데 유료 모델 +포함된 auto 써서 뭐 하려고"), so `auto`'s existence is itself a separate, pre-existing scope question this +correction does not resolve -- but as long as `--pool auto` is live, reachable code, it must not admit a row +the vendored library itself now refuses to activate as an agent. + +**Fix.** `_routable_discovered_models()` now excludes `getattr(model, "spend_admitted", True) is False` rows +the same way it excludes `evidence_only=True` rows -- unconditionally, with no self-correcting exemption +(unlike the OpenRouter `evidence_only` exemption, `spend_admitted` was never wrongly blanket-set for every +OpenRouter row, so there is no equivalent bug shape to work around). The `getattr(..., True)` default keeps +this correct against the currently-pinned vendored copy too, which predates `#949` and has no +`spend_admitted` attribute at all -- exactly the same forward-compatible pattern already used for +`evidence_only`. Regression tests cover: a `spend_admitted=False` row excluded regardless of provider or +pool; a `spend_admitted=True` row and a row with the attribute entirely absent both still pass; and an +end-to-end `--pool auto` composition (`_routable_discovered_models` → `_report_rows` → `parse_discovery_ +report` → `build_zdr_prioritized_catalog`) proving a credit-exhausted priced OpenRouter row no longer +reaches the built catalog. + +**Second Devin Review finding on `ContextualWisdomLab/.github#1476`, investigated and closed as a false +alarm (discussion `r3891875749`, 🟥 "Private code can reach forbidden routes").** The finding: because +`_routable_discovered_models()` converts every OpenRouter row into a candidate while every row still shows +`evidence_only=True`, "private review content \[could\] reach third-party routes the vendored ZDR contract +forbids serving." Traced the full `--require-zdr` path (`CONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDR`, set from +`is_private`/target-visibility in `noema-review.yml`, `strix.yml`, and `opencode-review-dispatch.yml`) end +to end: becoming a *candidate* that survives `_routable_discovered_models()` is not the same as being +*admitted* to a private target's served catalog. The actual, independent admission gate for a +`--require-zdr` build is `is_zdr_model()` (`scripts/ci/zdr_policy.py`), which for OpenRouter requires an +exact `route_key(provider, model)` match against the real, live `/api/v1/endpoints/zdr` feed and fails +closed (`False`) whenever that feed is empty or the model is unset. `build_zdr_prioritized_catalog()` -- +the function that actually produces the served `agents` catalog for both the `free` and `auto` pools -- +re-applies this exact `is_zdr_model()` check as its own `eligible_rows` filter whenever `require_zdr=True`, +independent of whatever `_routable_discovered_models()` already did upstream; `evidence_only` plays no part +in that filter at all. So a row exempted from `evidence_only` still cannot reach a private target's catalog +unless it also genuinely matches OpenRouter's own authoritative ZDR feed -- at which point, by OpenRouter's +own definition, it *is* a zero-data-retention route, satisfying the actual contract the finding is +concerned about. The separate, provider-agnostic chat-capability check (`is_general_chat_agent_model_id` + +`_has_text_output`, in `main()`, applied uniformly before any pool split) additionally guards against a +non-chat metadata stub being admitted regardless of pool or privacy requirement. No code change was made +for this finding; a regression test +(`test_require_zdr_still_excludes_non_zdr_openrouter_route_despite_evidence_only_exemption`) composes the +real pipeline (`_routable_discovered_models` → `_report_rows` → `parse_discovery_report` → +`build_zdr_prioritized_catalog(..., require_zdr=True)`) to prove this holds even while every discovered +OpenRouter row still carries `evidence_only=True`, and the GitHub review thread was replied to and marked +resolved with this reasoning. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index e1217e2b80..90a0419984 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -141,66 +141,75 @@ def _log_discovery_errors(errors: list[object]) -> None: def _openrouter_reports_per_model_evidence(discovered: list[object]) -> bool: """Return whether this run's OpenRouter rows show real per-model evidence. - ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` - currently has a confirmed bug (``ContextualWisdomLab/contextual- - orchestrator#950``, open, not yet merged as of this writing): it + ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` had a + confirmed bug in the vendored copy pinned as of this writing: it hardcodes ``evidence_only=True`` for *every* discovered OpenRouter model unconditionally, at the provider-source level, regardless of that specific model's own evidence -- so today's real, observable signature is that literally every discovered OpenRouter row carries ``evidence_only=True``, with zero exceptions, even for genuinely - servable, ZDR-attested models. #950 fixes this by computing - ``evidence_only``/``zdr_capable`` per model instead, from OpenRouter's - own ``/api/v1/endpoints/zdr`` feed match -- so a fixed vendored copy - reports ``evidence_only=False`` for at least the subset of OpenRouter - models that are genuinely ZDR-attested (whenever OpenRouter offers any - such models, which it does today), while unattested models correctly - stay ``True``. + servable, ZDR-attested models. ``ContextualWisdomLab/contextual- + orchestrator#949`` ("fix(discovery): route OpenRouter by model + evidence", merged at ``8cd99f139915131ba0239bce12a5d6a5fd85394e``) fixes + this by removing the hardcode entirely rather than computing a + per-model value: once a run's vendored pin includes the fix, + ``evidence_only`` falls back to its ``False`` dataclass default for + *every* discovered OpenRouter row, unconditionally -- ZDR attestation + for OpenRouter rows is carried separately, on the ``zdr_capable`` field + (already computed per model from OpenRouter's own + ``/api/v1/endpoints/zdr`` feed match both before and after #949), not on + ``evidence_only``. This function is this run's *observed-behavior* signal for which of those two shapes is currently vendored, used by ``_routable_discovered_models`` in place of tracking ``ORCHESTRATOR_PIN_SHA`` (the sidecar's vendored-commit pin, defined in ``scripts/ci/contextual_orchestrator_review_sidecar.sh``) directly. A - pin-ancestry check (``git merge-base --is-ancestor <950's merge commit> + pin-ancestry check (``git merge-base --is-ancestor "$ORCHESTRATOR_PIN_SHA"``) was considered and is technically reachable -- the sidecar's vendored clone at ``$ORCHESTRATOR_SOURCE`` keeps full - commit history (only blob content is filtered) -- but #950 has not - merged yet, so no fix commit SHA exists to compare against, and wiring - one in now would need a new CLI argument/env var carrying - ``$ORCHESTRATOR_SOURCE`` (or the pin itself) into this launcher, a - ``subprocess`` git call, and matching sidecar-script/contract-test - changes -- real plumbing built against a threshold value that does not - exist yet. This observed-behavior check needs none of that: it reads - the same ``discovered`` rows this function already receives, requires - no new plumbing, and -- unlike a pin comparison -- keeps working even - if a fix is ever backported to the vendored fork without a matching - ``ORCHESTRATOR_PIN_SHA`` bump, since it looks at what the vendored code - actually returned this run rather than which commit is nominally - pinned. See ``ContextualWisdomLab/.github#1476`` (this change) and - ``ContextualWisdomLab/contextual-orchestrator#950`` (the upstream fix) - for the full history; once #950 merges, no further change is required - here -- this check starts reporting ``True`` as soon as the vendored - pin actually includes the fix and OpenRouter has at least one - ZDR-attested free model in a given run. + commit history (only blob content is filtered) -- but at the time this + was written neither candidate upstream fix had merged yet, so no fix + commit SHA existed to compare against, and wiring one in would have + needed a new CLI argument/env var carrying ``$ORCHESTRATOR_SOURCE`` (or + the pin itself) into this launcher, a ``subprocess`` git call, and + matching sidecar-script/contract-test changes -- real plumbing built + against a threshold value that did not exist yet. This observed-behavior + check needs none of that: it reads the same ``discovered`` rows this + function already receives, requires no new plumbing, and -- unlike a + pin comparison -- keeps working even if a fix is ever backported to the + vendored fork without a matching ``ORCHESTRATOR_PIN_SHA`` bump, since it + looks at what the vendored code actually returned this run rather than + which commit is nominally pinned. See ``ContextualWisdomLab/.github#1476`` + (this change) and ``ContextualWisdomLab/contextual-orchestrator#949`` + (the merged upstream fix; pinned by ``ContextualWisdomLab/.github#1477``) + for the full history; once ``#1477`` merges, no further change is + required here -- this check starts reporting ``True`` as soon as the + vendored pin actually includes the fix and OpenRouter has discovered at + least one model that run (``#949`` makes ``evidence_only=False`` + unconditional for OpenRouter, not contingent on that model being + ZDR-attested). KNOWN, ACCEPTED LIMITATION: a genuinely-fixed vendored copy that happens to report ``evidence_only=True`` for *every* OpenRouter row in - one particular run -- because the ZDR feed fetch failed entirely that - run (#950's own documented fail-closed behavior), or because none of - the OpenRouter models discovered that run happen to be ZDR-attested -- - is indistinguishable from the still-buggy blanket signature by this - check alone, and the historical exemption stays active for that one - run. This mirrors the fail-open-on-ambiguity reasoning already used - elsewhere in this module (e.g. the KNOWN GAP entries above) rather than - inventing a new policy; a false-negative here only widens which - OpenRouter rows reach the same downstream, provider-agnostic - chat-capability check every other provider's rows already pass through - (``is_general_chat_agent_model_id`` + ``_has_text_output``, in - ``main()``) -- it does not bypass the separate, ``evidence_only``- - independent ZDR admission gate (``is_zdr_model()`` / - ``_zdr_admitted_rows()``) that guards private, ``--require-zdr`` - targets. + one particular run -- e.g. OpenRouter discovery itself failing entirely + that run, so ``discovered`` carries no OpenRouter rows to observe real + evidence from at all -- is indistinguishable from the still-buggy + blanket signature by this check alone, and the historical exemption + stays active for that one run. This mirrors the fail-open-on-ambiguity + reasoning already used elsewhere in this module (e.g. the KNOWN GAP + entries above) rather than inventing a new policy; a false-negative + here only widens which OpenRouter rows reach the same downstream, + provider-agnostic chat-capability check every other provider's rows + already pass through (``is_general_chat_agent_model_id`` + + ``_has_text_output``, in ``main()``) -- it does not bypass the + separate, ``evidence_only``-independent ZDR admission gate + (``is_zdr_model()`` / ``_zdr_admitted_rows()``, and the equivalent + filter re-applied inside ``build_zdr_prioritized_catalog()``) that + guards private, ``--require-zdr`` targets -- traced end to end and + confirmed still intact in the 2026-08-31 correction entry of + ``docs/product-technical-gap-baseline.md`` in response to a second + Devin Review finding that raised exactly this question. Args: discovered: The full discovery-wide row set for this run (already @@ -213,7 +222,8 @@ def _openrouter_reports_per_model_evidence(discovered: list[object]) -> bool: ``evidence_only=False`` (real per-model evidence observed this run); ``False`` when there are no OpenRouter rows at all, or every OpenRouter row is ``evidence_only=True`` (today's confirmed bug - signature, or an indistinguishable fixed-but-all-unattested run). + signature, or an indistinguishable run where OpenRouter discovery + itself produced no rows). """ return any( getattr(model, "provider_name", None) == "openrouter" @@ -237,8 +247,11 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] and its documented limitation. In short: ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for every discovered model unconditionally (a - confirmed bug, ``ContextualWisdomLab/contextual-orchestrator#950``, not - yet merged) -- not computed per model from real evidence, even though + confirmed bug, fixed upstream at + ``ContextualWisdomLab/contextual-orchestrator#949``, merged at + ``8cd99f139915131ba0239bce12a5d6a5fd85394e`` -- not yet pinned in this + repo as of this writing; see ``ContextualWisdomLab/.github#1477``) -- + not computed per model from real evidence, even though genuine per-model ZDR evidence is fetched and parsed for OpenRouter in that same module. Applying this filter to OpenRouter verbatim while that bug is live would strip every OpenRouter row, including genuinely @@ -266,21 +279,44 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] ``contextual-orchestrator`` reports -- start reaching selection immediately. What remains genuinely blocked on the upstream fix is OpenRouter rows being correctly excluded from ``evidence_only`` on a - real per-model basis (e.g. a non-chat listing); until #950 merges and - a run observes real per-model variation, this function's remaining + real per-model basis (e.g. a non-chat listing); until + ``ContextualWisdomLab/.github#1477`` lands the ``#949`` pin bump and a + run observes real per-model variation, this function's remaining protection against those is the same downstream chat-capability check, not ``evidence_only``. + + A row is also excluded whenever ``getattr(model, "spend_admitted", + True) is False`` -- the same treatment as ``evidence_only=True``, with + no self-correcting exemption (there is no equivalent blanket-bug shape + to work around: ``spend_admitted`` was never wrongly ``False`` for + every OpenRouter row). ``contextual-orchestrator#949`` added this field + to ``DiscoveredModel`` (default ``True``) and sets it ``False`` only for + a *priced* OpenRouter row when ``openrouter_paid_inference_available()`` + does not affirmatively confirm usable credit; a free OpenRouter row is + always ``spend_admitted=True`` regardless of credit status. The + vendored library's own ``is_routable_discovered_model()`` already + refuses to activate such a row as an agent; this mirrors that refusal + here so a spend-blocked row can never reach ``orchestrator/auto``'s + priced-fallback path either (``orchestrator/free`` never considers + priced rows at all, so it was never exposed to this). The + ``getattr(..., True)`` default keeps this correct against a vendored + pin that predates ``#949`` and has no ``spend_admitted`` attribute at + all. See the 2026-08-31 correction entry in + ``docs/product-technical-gap-baseline.md`` for the full investigation. """ discovered = list(discovered or []) openrouter_still_blanket_marked = not _openrouter_reports_per_model_evidence(discovered) return [ model for model in discovered - if not getattr(model, "evidence_only", False) - or ( - openrouter_still_blanket_marked - and getattr(model, "provider_name", None) == "openrouter" + if ( + not getattr(model, "evidence_only", False) + or ( + openrouter_still_blanket_marked + and getattr(model, "provider_name", None) == "openrouter" + ) ) + and getattr(model, "spend_admitted", True) is not False ] diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 68bf873d46..48dca875a0 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -115,8 +115,10 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_evidence_only() -> None: """OpenRouter rows are exempt from evidence_only while every row still shows it. - Regression for a confirmed bug (``ContextualWisdomLab/contextual- - orchestrator#950``, open, not yet merged): ``contextual-orchestrator``'s + Regression for a confirmed bug, fixed upstream at + ``ContextualWisdomLab/contextual-orchestrator#949`` (merged, not yet + pinned in this repo as of this writing -- see + ``ContextualWisdomLab/.github#1477``): ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for every discovered model unconditionally (not computed per model from real evidence) -- so today's real signature is @@ -160,8 +162,8 @@ def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_ev def test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence() -> None: """The historical exemption turns off the moment per-model evidence appears. - Once ``ContextualWisdomLab/contextual-orchestrator#950`` merges and the - vendored pin advances past it, OpenRouter starts reporting real + Once ``ContextualWisdomLab/.github#1477`` lands the + ``ContextualWisdomLab/contextual-orchestrator#949`` pin bump, OpenRouter starts reporting real per-model ``evidence_only`` (at minimum ``False`` for its genuinely ZDR-attested free models). This is the post-fix signature: a run whose OpenRouter rows are no longer uniformly ``True`` must go back through @@ -190,6 +192,211 @@ def test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_ assert routable([openrouter_attested, openrouter_unattested]) == [openrouter_attested] +def test_routable_discovered_models_excludes_spend_blocked_rows() -> None: + """A ``spend_admitted=False`` row is excluded the same way as ``evidence_only=True``. + + ``contextual-orchestrator#949`` added ``DiscoveredModel.spend_admitted`` + (default ``True``): a priced OpenRouter row becomes ``False`` when + ``openrouter_paid_inference_available()`` cannot confirm usable credit. + The vendored library's own ``is_routable_discovered_model()`` already + refuses to activate such a row as an agent; this launcher must refuse it + too, with the same ``getattr(..., True)`` default so a vendored pin that + predates ``#949`` (and so has no ``spend_admitted`` attribute at all) + keeps behaving exactly as it did before this filter existed. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + spend_blocked = SimpleNamespace( + id="openrouter_spend_blocked", + provider_name="openrouter", + model_id="provider/paid", + evidence_only=False, + spend_admitted=False, + ) + spend_admitted_row = SimpleNamespace( + id="openrouter_spend_admitted", + provider_name="openrouter", + model_id="provider/paid-ok", + evidence_only=False, + spend_admitted=True, + ) + no_spend_attribute = SimpleNamespace( + id="nvidia_untagged", + provider_name="nvidia_nim", + model_id="untagged/model", + evidence_only=False, + ) + + assert routable([spend_blocked, spend_admitted_row, no_spend_attribute]) == [ + spend_admitted_row, + no_spend_attribute, + ] + + +def test_routable_discovered_models_excludes_spend_blocked_openrouter_row_even_while_evidence_only_exempt() -> None: + """The ``spend_admitted`` exclusion applies independently of the ``evidence_only`` exemption. + + A spend-blocked OpenRouter row that also still carries today's blanket + ``evidence_only=True`` bug signature -- so the OpenRouter ``evidence_ + only`` exemption would otherwise let it through -- must still be + excluded: the two filters are independent conditions, and neither + exemption weakens the other. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + + openrouter_blanket_and_spend_blocked = SimpleNamespace( + id="openrouter_blanket_spend_blocked", + provider_name="openrouter", + model_id="provider/paid", + evidence_only=True, + spend_admitted=False, + ) + openrouter_blanket_and_admitted = SimpleNamespace( + id="openrouter_blanket_admitted", + provider_name="openrouter", + model_id="provider/free", + evidence_only=True, + spend_admitted=True, + ) + + assert routable( + [openrouter_blanket_and_spend_blocked, openrouter_blanket_and_admitted] + ) == [openrouter_blanket_and_admitted] + + +def test_pool_auto_never_admits_a_spend_blocked_priced_openrouter_row() -> None: + """A credit-exhausted paid OpenRouter row must never reach ``orchestrator/auto``. + + Regression for the real, latent gap found while investigating whether + this repo's pipeline needs to respect ``spend_admitted``: + ``orchestrator/free`` never considers priced rows at all (its + ``selected_models`` loop in ``main()`` drops anything outside + ``free_route_identities`` before it becomes a report row), so it was + never exposed to a spend-blocked row. ``--pool auto`` is real, tested, + reachable code (``CONTEXTUAL_ORCHESTRATOR_POOL=auto``, no other change + needed) whose candidate rows explicitly include priced ones + (``build_zdr_prioritized_catalog``'s ``[*all_free_rows, + *all_priced_rows]`` for ``pool="auto"``, plus ``main()``'s explicit + priced-fallback stage) -- so without the ``spend_admitted`` filter in + ``_routable_discovered_models``, a spend-blocked row could have reached + a served ``auto`` catalog exactly as if it were servable. This composes + the real pipeline: ``_routable_discovered_models`` -> ``_report_rows`` + -> ``parse_discovery_report`` -> ``build_zdr_prioritized_catalog``. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + report_rows = namespace["_report_rows"] + route_identity = namespace["_route_identity"] + + free_model = SimpleNamespace( + provider_name="nvidia_nim", + model_id="free/model", + agent_id="nvidia_nim_free_model", + evidence_only=False, + spend_admitted=True, + prompt_price_per_1k=0.0, + completion_price_per_1k=0.0, + currency_code="USD", + ) + spend_blocked_model = SimpleNamespace( + provider_name="openrouter", + model_id="provider/paid", + agent_id="openrouter_provider_paid", + evidence_only=False, + spend_admitted=False, + chat_base_url="https://openrouter.ai/api/v1", + credential_name="OPENROUTER_API_KEY", + auth_scheme="Bearer", + prompt_price_per_1k=0.1, + completion_price_per_1k=0.1, + currency_code="USD", + ) + discovered = [free_model, spend_blocked_model] + + routable_discovered = routable(discovered) + assert routable_discovered == [free_model] + + free_route_identities = frozenset({route_identity(free_model)}) + rows = report_rows(routable_discovered, free_route_identities) + normalized_rows = policy.parse_discovery_report({"models": rows}) + + result = policy.build_zdr_prioritized_catalog(normalized_rows, pool="auto") + + assert {entry["model"] for entry in result["agents"]} == {"free/model"} + + +def test_require_zdr_still_excludes_non_zdr_openrouter_route_despite_evidence_only_exemption() -> None: + """The ``evidence_only`` exemption never weakens the real ZDR admission gate. + + Devin Review flagged (``ContextualWisdomLab/.github#1476``, discussion + ``r3891875749``, 🟥) that ``_routable_discovered_models`` converting + every OpenRouter row into a serving candidate while every row still + carries the vendored ``evidence_only=True`` bug signature could let + "private review content reach third-party routes the vendored ZDR + contract forbids serving." Traced end to end, this is a false alarm: + becoming a *candidate* that survives ``_routable_discovered_models`` is + not the same as being *admitted* to a ``--require-zdr`` (private) + target's served catalog. The real, independent admission gate for that + path is ``is_zdr_model()`` (``scripts/ci/zdr_policy.py``), which + ``build_zdr_prioritized_catalog`` -- the function that actually builds + the served ``agents`` catalog -- re-applies as its own ``eligible_rows`` + filter whenever ``require_zdr=True``, completely independent of + ``evidence_only``. This reproduces the real pipeline + (``_routable_discovered_models`` -> ``_report_rows`` -> + ``parse_discovery_report`` -> ``build_zdr_prioritized_catalog(..., + require_zdr=True)``) with two free OpenRouter rows that BOTH still + report ``evidence_only=True`` (today's exact bug signature, so the + exemption is active for both) -- only the one genuinely present in the + live OpenRouter ZDR feed is ever admitted to the catalog. + """ + namespace = _load_launcher() + routable = namespace["_routable_discovered_models"] + report_rows = namespace["_report_rows"] + route_identity = namespace["_route_identity"] + + zdr_model = SimpleNamespace( + provider_name="openrouter", + model_id="zdr/free-model", + agent_id="openrouter_zdr_free_model", + evidence_only=True, + prompt_price_per_1k=0.0, + completion_price_per_1k=0.0, + currency_code="USD", + ) + non_zdr_model = SimpleNamespace( + provider_name="openrouter", + model_id="forbidden/free-model", + agent_id="openrouter_forbidden_free_model", + evidence_only=True, + prompt_price_per_1k=0.0, + completion_price_per_1k=0.0, + currency_code="USD", + ) + discovered = [zdr_model, non_zdr_model] + + routable_discovered = routable(discovered) + # Both rows survive the still-blanket-marked exemption: exactly the + # shape the Devin finding is concerned about. + assert routable_discovered == discovered + + free_route_identities = frozenset(route_identity(model) for model in discovered) + rows = report_rows(discovered, free_route_identities) + normalized_rows = policy.parse_discovery_report({"models": rows}) + + result = policy.build_zdr_prioritized_catalog( + normalized_rows, + zdr_endpoints=frozenset({"openrouter/zdr/free-model"}), + require_zdr=True, + pool="free", + ) + + selected_models = {entry["model"] for entry in result["agents"]} + assert selected_models == {"zdr/free-model"} + assert "forbidden/free-model" not in selected_models + + def test_log_discovery_errors_prints_one_bounded_line_per_provider_failure( capsys: pytest.CaptureFixture[str], ) -> None: From 95406d2c740bbc10b5f25721c30e8ad67fe4ef3a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 08:27:05 +0000 Subject: [PATCH 04/15] test(ci): pin the OpenRouter evidence_only exemption's real boolean outcome Devin Review (discussion r3891875665) on tests/test_contextual_orchestrator_ review_sidecar_contract.py:290 correctly noted the contract test's source check only requires the OpenRouter provider_name comparison to appear somewhere in source text -- a reversed (`!=` for `==`) or disconnected exemption would still satisfy that check. Verified by mutation: both mutations were applied locally and confirmed the old assertion alone would not have caught them (a "disconnect" mutation was crafted to leave the literal source fragment byte-for-byte intact while making the exemption a no-op). Add a direct behavioral assertion in the same test, against the already runpy-loaded launcher module's real `_routable_discovered_models`, that exercises one OpenRouter row that must be exempted and one same-shaped non-OpenRouter row that must not, asserting the actual filtered output. This fails under both the reversal and the disconnection mutation, closing the gap Devin identified without weakening the existing source-text checks. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- ...al_orchestrator_review_sidecar_contract.py | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 4602c13659..92a01bf5ee 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -286,7 +286,15 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert 'getattr(model, "evidence_only", False)' in text # OpenRouter must stay exempt from the evidence_only exclusion, or # zdr_policy.is_zdr_model()'s purpose-built per-route OpenRouter ZDR-feed - # check goes back to never seeing an OpenRouter row at all. + # check goes back to never seeing an OpenRouter row at all. This + # fragment-presence check only pins that the comparison exists + # somewhere in source -- Devin Review (discussion r3891875665) correctly + # noted it would still pass even if the exemption were reversed (e.g. + # ``!=`` for ``==``) or disconnected from the rows it is meant to gate. + # The behavioral assertions below, against the loaded module's real + # ``_routable_discovered_models``, close that gap by pinning the actual + # boolean outcome for both a row that must be exempted and one that + # must not. assert 'getattr(model, "provider_name", None) == "openrouter"' in text assert 'getattr(model, "output_modalities", None)' in text assert 'isinstance(modalities, str)' in text @@ -300,6 +308,26 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert has_text_output(SimpleNamespace(output_modalities=("text", "image"))) assert not has_text_output(SimpleNamespace(output_modalities=("video",))) assert not has_text_output(SimpleNamespace()) + + # Pin the exemption's real boolean outcome, not just source-text + # presence: an OpenRouter row carrying today's blanket evidence_only=True + # bug signature must still be routable, while a same-shaped row from any + # other provider must not -- so a reversed comparison (``!=`` instead of + # ``==``) or a disconnected/no-op exemption (e.g. the OpenRouter branch + # never actually reached, or applied unconditionally regardless of + # provider) fails this assertion even though the source fragment above + # would still be present verbatim. + routable_discovered_models = launcher["_routable_discovered_models"] + openrouter_blanket_marked = SimpleNamespace( + provider_name="openrouter", model_id="some/model", evidence_only=True + ) + non_openrouter_evidence_only = SimpleNamespace( + provider_name="nvidia_nim", model_id="some/model", evidence_only=True + ) + assert routable_discovered_models( + [openrouter_blanket_marked, non_openrouter_evidence_only] + ) == [openrouter_blanket_marked] + report_rows = launcher["_report_rows"] free = SimpleNamespace( provider_name="openrouter", From 9b74577b34192ec34584628a05c0fbaa4d0409c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 09:18:18 +0000 Subject: [PATCH 05/15] fix(ci): bound required-workflow-bootstrap awk extraction to its own job Ports the identical fix from #1506 into this branch. This PR's exact-head-path-policy check runs its own head-branch copy of scripts/ci/test_strix_quick_gate.sh (plain `pull_request` trigger in strix-changed-path-quality-ci.yml, not pull_request_target), so the pre-existing main-branch bug is not fixed here just by #1506 merging into main -- it needs porting into this branch directly. Root cause: assert_opencode_review_uses_codegraph_and_contextual_orchestrator extracted the required-workflow-bootstrap job block from opencode-review.yml with awk '/^ required-workflow-bootstrap:$/,/^[^ ]/'. Every job key in that workflow is indented 2 spaces (never column 0), so the end pattern never matched until EOF, sweeping an unrelated `if:` line from a later job (added by already-merged PR #1497) into the "block" and failing the assertion on unrelated content. Fixed by using an explicit state flag so the end pattern (`^ [A-Za-z0-9_-]+:`) is only tested starting on the line after the start match, correctly bounding the block to just its own lines. See ContextualWisdomLab/.github#1506 for the full root-cause writeup and validation against origin/main. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- scripts/ci/test_strix_quick_gate.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 4053f4fd53..1fc45a34b9 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -522,7 +522,7 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_not_contains "$workflow_file" "Wait for trusted OpenCode approval review" "opencode pull_request bridge was removed to avoid duplicate required-check resource use" assert_file_not_contains "$workflow_file" "Trusted OpenCode requested changes for head" "opencode pull_request bridge no longer reconsumes stale trusted review state" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "opencode review workflow must not hard-code repository-specific PR bypasses" - if awk '/^ required-workflow-bootstrap:$/,/^[^ ]/' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then + if awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then record_failure "opencode required workflow bootstrap must not depend on required-workflow event payload fields" fi assert_file_contains "$workflow_file" 'github.event.client_payload.target_repository || github.repository' "opencode review scopes concurrency by target repository" From 7d693b86776de4e814b096f052c1f564e99505f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 10:53:59 +0000 Subject: [PATCH 06/15] fix(ci): remove grep -q from test_strix_quick_gate.sh pipeline checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit grep -q exits on first match and closes its end of the pipe; if the upstream awk is still writing a large block, it gets SIGPIPE (141). Under `set -o pipefail` that non-zero awk status wins over grep's real 0, so `if pipeline; then` sees the pipeline as failed even though grep found a genuine match — silently missing e.g. a forbidden `if:` key or a fenced-diff marker that should have failed the check. Ports the same-file fix from PR #1506 to this branch's two call sites (required-workflow-bootstrap job-block check; opencode review REQUEST_CHANGES fenced-diff check). This branch already carried #1506's awk job-block-boundary correction, so only the grep -q removal was needed here. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- scripts/ci/test_strix_quick_gate.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 1fc45a34b9..a92871b7ce 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -522,7 +522,7 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_not_contains "$workflow_file" "Wait for trusted OpenCode approval review" "opencode pull_request bridge was removed to avoid duplicate required-check resource use" assert_file_not_contains "$workflow_file" "Trusted OpenCode requested changes for head" "opencode pull_request bridge no longer reconsumes stale trusted review state" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "opencode review workflow must not hard-code repository-specific PR bypasses" - if awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then + if awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | grep '^[[:space:]]*if:' >/dev/null; then record_failure "opencode required workflow bootstrap must not depend on required-workflow event payload fields" fi assert_file_contains "$workflow_file" 'github.event.client_payload.target_repository || github.repository' "opencode review scopes concurrency by target repository" @@ -1501,7 +1501,7 @@ assert_opencode_review_posts_suggested_diffs_inline() { assert_file_contains "$workflow_file" "publish_request_changes_from_control" "opencode review REQUEST_CHANGES path publishes findings from the control JSON" if awk '/format_request_changes_body\(\)/,/build_request_changes_review_payload\(\)/ { print }' "$workflow_file" | - grep -Fq '```diff'; then + grep -F '```diff' >/dev/null; then record_failure "opencode review PR-level REQUEST_CHANGES body must not contain fenced suggested diffs" fi } From db106d50f2134ece147bc5318e389aeb124d198c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:21:06 +0000 Subject: [PATCH 07/15] test(ci): close main's post-#1546 scheduler coverage regression Protected main regressed to 99% scripts/ci coverage after #1546 added live_head_matches, a no-active/no-stale fall-through in prepare_autofix_slot, and an "already queued or running" wait branch to pr_review_fix_scheduler.py without covering them, while the pre-existing inspect_pr conflicted-draft/conflicted-unauthorized returns and pr_review_merge_scheduler.py's fetch_workflow_names_by_check_suite_rest pagination/filtering/ permission-denied paths stayed untested. Every PR rebasing onto main inherits this via the coverage-evidence required check regardless of its own diff. Test-only change; no production code touched. --- CHANGELOG.md | 9 +++ tests/test_pr_review_fix_scheduler.py | 49 ++++++++++++++ ...ew_fix_scheduler_rest_workflow_identity.py | 67 +++++++++++++++++++ 3 files changed, 125 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5810d5308..1c46c64657 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Close a 99% `scripts/ci` coverage regression on protected main: merged #1546 added an + uncovered `live_head_matches` helper, an uncovered no-active/no-stale-runs fall-through in + `prepare_autofix_slot`, and an uncovered "current-head autofix run is already queued or + running" wait path in `pr_review_fix_scheduler.py::inspect_pr`, while the pre-existing + conflicted-draft and conflicted-unauthorized `inspect_pr` returns and the REST + `fetch_workflow_names_by_check_suite_rest` pagination/name-filtering/permission-denied paths + in `pr_review_merge_scheduler.py` remained untested. Every PR rebasing onto main inherited + this failure via the `coverage-evidence` required check regardless of its own diff; this adds + test-only coverage for all of the above with no production code change. - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 9860eeaec7..f6abd64b0f 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -177,6 +177,40 @@ def test_prepare_autofix_slot_preserves_new_head_workers_after_head_advance(monk workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, dry_run=False, ) is None + + +def test_prepare_autofix_slot_returns_directly_with_no_active_or_stale_runs(monkeypatch): + """An empty Actions run list needs no reconciliation and skips cancellation.""" + monkeypatch.setattr(fix, "run_json", lambda _args: {"workflow_runs": []}) + monkeypatch.setattr( + fix, + "force_cancel_workflow_runs", + lambda *_args: pytest.fail("no stale runs must not attempt cancellation"), + ) + + assert fix.prepare_autofix_slot( + "owner/repo", + make_pr(), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=False, + ) is False + + +def test_live_head_matches_compares_case_insensitively_and_fails_closed(monkeypatch): + """Live head lookup normalizes case and rejects malformed or mismatched payloads.""" + head = "a" * 40 + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": head.upper()}}) + assert fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": "b" * 40}}) + assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"nothead": {}}) + assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + def test_terminal_failed_check_triggers_rca_without_prior_opencode_review(): """Exact-head check evidence can start RCA without a circular review prerequisite.""" pr = make_pr( @@ -1329,6 +1363,21 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch): monkeypatch.setattr(fix, "issue_comments", lambda repo, number: [{"body": f"{fix.FIX_MARKER} head_sha={'a' * 40} epoch={int(time.time())} -->"}]) assert fix.inspect_pr("owner/repo", make_pr(), args) == ("wait", ("recent autofix marker exists for this head",)) + assert fix.inspect_pr( + "owner/repo", make_pr(mergeStateStatus="DIRTY", isDraft=True), args + ) == ("skip", ("draft PR",)) + assert fix.inspect_pr("owner/repo", make_pr(mergeStateStatus="DIRTY"), args) == ( + "skip", + ("merge conflict is not authorized for repair",), + ) + + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: True) + assert fix.inspect_pr("owner/repo", make_pr(), args) == ( + "wait", + ("current-head autofix run is already queued or running",), + ) + pr1 = make_pr(number=1) pr2 = make_pr(number=2) monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr1, pr2]) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index c24cfb05f9..f261ce5beb 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -154,3 +154,70 @@ def fake_api(path: str) -> Any: assert merge.is_strix_context(context) assert merge.strix_evidence_state(pr) == expected_state assert fix.current_head_failed_checks(pr) == () + + +def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100( + monkeypatch: Any, +) -> None: + """A first page of exactly 100 runs must fetch a second page and merge both.""" + head_sha = "e" * 40 + page1 = [ + {"check_suite_id": i, "name": f"workflow-{i}"} for i in range(100) + ] + page2 = [{"check_suite_id": 100, "name": "workflow-100"}] + calls: list[str] = [] + + def fake_api(path: str) -> Any: + calls.append(path) + if path.endswith("page=1"): + return {"workflow_runs": page1} + if path.endswith("page=2"): + return {"workflow_runs": page2} + raise AssertionError(f"unexpected path {path}") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) + + assert names == {i: f"workflow-{i}" for i in range(101)} + assert calls == [ + f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=1", + f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=2", + ] + + +def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id_or_name( + monkeypatch: Any, +) -> None: + """A run with no check-suite id or a blank name must not populate the map.""" + head_sha = "f" * 40 + + def fake_api(path: str) -> Any: + return { + "workflow_runs": [ + {"check_suite_id": None, "name": "orphaned run"}, + {"check_suite_id": 900, "name": ""}, + {"check_suite_id": 901, "name": "kept run"}, + ] + } + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) + + assert names == {901: "kept run"} + + +def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors( + monkeypatch: Any, +) -> None: + """A page-fetch failure unrelated to integration access must fail closed.""" + head_sha = "0" * 40 + + def fake_api(path: str) -> Any: + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + with pytest.raises(RuntimeError, match="HTTP 502"): + merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) From 85c2469e1e624f8d4dfbc71c79fe18c927d315ab Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:38:38 +0000 Subject: [PATCH 08/15] docs(gap-baseline): record post-#1546 scheduler coverage regression Adds a dated traceability entry for the coverage gap this PR closes: root cause (#1546's uncovered additions plus the older #1547/#1551/ #1554 gap, neither of which merged or transfers evidence here), the fix and its verification, the resolved Devin false-positive on sub-clause coverage, and the known pre-existing SIGPIPE test flake left unremediated as out of scope. --- docs/product-technical-gap-baseline.md | 48 ++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 76d85b949b..812f068e34 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2344,6 +2344,54 @@ contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr "today" reference. Landed in the same PR (`#1463`) as the streaming revert, not split out, since the revert is unsafe without it. +## 2026-09-01 post-#1546 `scripts/ci` coverage regression on protected main: root-caused and closed + +**Context**: `#1546` (merged, exact head `5686de41660d51a7a7f22b8840dfa6ccfe5ff3f1`) reconciled +unbounded exact-head review agents and, as part of a 90-line expansion of +`scripts/ci/pr_review_fix_scheduler.py`, added a `live_head_matches` helper, a no-active/no-stale +fall-through branch in `prepare_autofix_slot`, and an "already queued or running" wait branch in +`inspect_pr` — none of which any test exercised directly. This compounded a narrower, older gap in +the same file (`inspect_pr`'s conflicted-draft and conflicted-unauthorized returns) and in +`scripts/ci/pr_review_merge_scheduler.py::fetch_workflow_names_by_check_suite_rest` (pagination, +missing-suite-id/blank-name filtering, non-access-error propagation), first found and attempted in +now-closed, unmerged `#1547`/`#1551`/`#1554` — none of whose evidence or diffs transferred here; +this pass re-derived the current gap from a clean `origin/main` clone rather than assuming those +predecessors were still accurate against `#1546`'s shifted line numbers and new branches. Verified +directly: `coverage report --show-missing` on unmodified `main` showed +`scripts/ci/pr_review_fix_scheduler.py` at 97% (missing 116-121, 459->466, 495, 503, 546) and +`scripts/ci/pr_review_merge_scheduler.py` at 99% (missing 1003, 1008->1005, 1012) — total repo-wide +99%, below the `pyproject.toml` `fail_under = 100` gate. Because `opencode-review-dispatch.yml`'s +`coverage-evidence` job measures the **merged** PR tree (base + head) and hard-fails below 100%, +every PR rebasing onto main inherited this failure regardless of its own diff — org-wide impact, +not scoped to one PR. + +**Fix**: `#1567` (test-only, no production code) adds direct unit coverage for `live_head_matches` +(case-insensitive match, mismatch, malformed-payload paths), `prepare_autofix_slot`'s empty-run +fall-through, the `inspect_pr` conflicted-draft/conflicted-unauthorized/already-queued cases, and +the `fetch_workflow_names_by_check_suite_rest` pagination/filtering/error-propagation paths. +Verified on the fix commit (`db106d50f2134ece147bc5318e389aeb124d198c`): `coverage run -m pytest +tests -q` (2251 passed, 1 skipped, 21 subtests), `coverage report` (repo-wide 100%, both files +individually 100% statement and 100% branch), `interrogate` (100.0%). + +**Devin Review raised a false positive on the fix itself**, claiming +`test_live_head_matches_compares_case_insensitively_and_fails_closed` left non-object-payload, +non-string-SHA, and wrong-length-SHA branches uncovered. Re-verified against the actual gate rather +than accepted at face value: `live_head_matches` has exactly one `if` statement (two arcs, both +exercised by the committed test), and its final `return (isinstance(...) and len(...) == 40 and +...)` is a single boolean expression with no `if`/`else` of its own — `coverage.py`'s branch mode +(what `fail_under = 100` actually measures here) tracks control-flow arcs between statements, not +sub-clause condition coverage within one expression. The cited cases are additional test +thoroughness, not something the gate is currently failing on; confirmed by a full-suite run on the +exact same head showing both files at 100% branch coverage with zero missing branches. Replied with +this evidence on the review thread and did not widen the PR's diff for a claim that does not hold +against this repo's own tooling. + +**One test in the full suite remains a known, pre-existing flake**, unrelated to this change: +`tests/test_opencode_required_verdict_regression.py::test_scheduler_wake_reuses_trusted_receipt_predicate` +intermittently exits 141 (SIGPIPE) under full-suite parallel load; reproduces identically on +unmodified `origin/main` and passes cleanly in file isolation. Not remediated here — out of scope +for a coverage-gap-only PR, and not itself a coverage regression. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. From 6f40a0637da94da60f43ca72086d27e1034e8bbc Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 16:46:11 +0900 Subject: [PATCH 09/15] test(ci): document nested REST fixture helpers Raise scoped docstring coverage for the newly added scheduler REST regression helpers to 100% without changing test behavior or production code. --- tests/test_pr_review_fix_scheduler_rest_workflow_identity.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index f261ce5beb..4e36544061 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -168,6 +168,7 @@ def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100( calls: list[str] = [] def fake_api(path: str) -> Any: + """Return deterministic paginated workflow-run fixtures.""" calls.append(path) if path.endswith("page=1"): return {"workflow_runs": page1} @@ -193,6 +194,7 @@ def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id head_sha = "f" * 40 def fake_api(path: str) -> Any: + """Return workflow runs that exercise incomplete-identity filtering.""" return { "workflow_runs": [ {"check_suite_id": None, "name": "orphaned run"}, @@ -215,6 +217,7 @@ def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors( head_sha = "0" * 40 def fake_api(path: str) -> Any: + """Simulate a non-access REST failure that must propagate.""" raise RuntimeError("gh: HTTP 502 (exhausted retries)") monkeypatch.setattr(merge, "gh_api_json", fake_api) From fa0758acc13e41a16728d8d9cdcfddb3dcc84472 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 12:15:49 +0000 Subject: [PATCH 10/15] test(noema): port stale-test fixes from #1598 (post-#1564) Syncing with current main pulled in #1564 (merge-base-anchored deleted-file review evidence), which left the pre-existing tests/test_noema_review_gate.py and its own new tests/test_noema_removed_file_context.py broken against the final merged implementation. Port the same fix already opened as its own dedicated PR (#1598) rather than widening this PR's own scope: - tests/test_noema_review_gate.py: rename fetch_changed_file_paths call sites to fetch_changed_files with (path, status) tuples; accept the new changed_files parameter in build_review_context mocks; drop the two CodeGraph-only assertions/tests for the removed function. - tests/test_noema_removed_file_context.py: rewrite against the real run() JSON-per-line contract, fetch_merge_base_sha's SHA validation, and fetch_file_content_at_ref; add direct coverage for the malformed-input and empty-content branches #1564 introduced. Full suite: 2324 passed, 100% branch coverage, 100% docstrings. --- tests/test_noema_removed_file_context.py | 132 ++++++++++++++++++----- tests/test_noema_review_gate.py | 66 ++++++------ 2 files changed, 137 insertions(+), 61 deletions(-) diff --git a/tests/test_noema_removed_file_context.py b/tests/test_noema_removed_file_context.py index 8c5d8ca539..500d406f73 100644 --- a/tests/test_noema_removed_file_context.py +++ b/tests/test_noema_removed_file_context.py @@ -3,6 +3,7 @@ from __future__ import annotations import base64 +import json from scripts.ci import noema_review_gate as noema @@ -12,7 +13,14 @@ def test_fetch_changed_files_preserves_path_and_status(monkeypatch): monkeypatch.setattr( noema, "run", - lambda args, stdin=None: "a.py\tmodified\n\nb.py\tremoved\nfuzz/x.py\tadded\n", + lambda args, stdin=None: ( + json.dumps(["a.py", "modified"]) + + "\n\n" + + json.dumps(["b.py", "removed"]) + + "\n" + + json.dumps(["fuzz/x.py", "added"]) + + "\n" + ), ) assert noema.fetch_changed_files("owner/repo", 7) == [ @@ -22,8 +30,11 @@ def test_fetch_changed_files_preserves_path_and_status(monkeypatch): ] -def test_removed_file_context_uses_base_content(monkeypatch): - """A deleted file must be reviewed from immutable pre-deletion evidence.""" +def test_removed_file_context_uses_merge_base_content(monkeypatch): + """A deleted file must be reviewed from immutable merge-base evidence.""" + head_sha = "a" * 40 + base_sha = "b" * 40 + merge_base_sha = "c" * 40 encoded = base64.b64encode(b"def doomed():\n pass\n").decode("ascii") calls: list[str] = [] @@ -31,24 +42,88 @@ def fake_run(args, stdin=None): target = args[2] calls.append(target) if target.endswith("/files"): - return "fuzz/fuzz_opencode_normalize_output.py\tremoved\n" - if "contents/fuzz/fuzz_opencode_normalize_output.py?ref=base-sha" in target: + return json.dumps(["fuzz/fuzz_opencode_normalize_output.py", "removed"]) + "\n" + if target == f"repos/owner/repo/compare/{base_sha}...{head_sha}": + return merge_base_sha + if f"contents/fuzz/fuzz_opencode_normalize_output.py?ref={merge_base_sha}" in target: return encoded raise AssertionError(args) monkeypatch.setattr(noema, "run", fake_run) - context = noema.changed_file_context( - "owner/repo", 1486, "head-sha", "base-sha" - ) + context = noema.changed_file_context("owner/repo", 1486, head_sha, base_sha) - assert "File removed in this PR. Pre-deletion content at base ref" in context + assert f"Pre-deletion content at merge base `{merge_base_sha}`" in context assert "def doomed" in context - assert not any("ref=head-sha" in target for target in calls) + assert not any(f"ref={head_sha}" in target for target in calls) + + +def test_fetch_changed_files_rejects_malformed_json_line(monkeypatch): + """A non-JSON line from the Files API must fail closed, not crash raw.""" + monkeypatch.setattr(noema, "run", lambda args, stdin=None: "not json\n") + + try: + noema.fetch_changed_files("owner/repo", 7) + except RuntimeError as exc: + assert "malformed" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed JSON line") + + +def test_fetch_changed_files_rejects_malformed_record_shape(monkeypatch): + """A well-formed JSON line that is not a two-element string pair must fail closed.""" + monkeypatch.setattr( + noema, "run", lambda args, stdin=None: json.dumps(["only-one-field"]) + "\n" + ) + + try: + noema.fetch_changed_files("owner/repo", 7) + except RuntimeError as exc: + assert "malformed" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed record shape") + + +def test_fetch_merge_base_sha_rejects_malformed_head_sha(): + """An invalid head SHA must be rejected before any network call is attempted.""" + try: + noema.fetch_merge_base_sha("owner/repo", "a" * 40, "not-a-sha") + except RuntimeError as exc: + assert "PR head SHA was unavailable or malformed" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed head SHA") + + +def test_fetch_merge_base_sha_rejects_malformed_compare_response(monkeypatch): + """A compare response lacking a valid merge-base SHA must fail closed.""" + monkeypatch.setattr(noema, "run", lambda args, stdin=None: "") + + try: + noema.fetch_merge_base_sha("owner/repo", "a" * 40, "b" * 40) + except RuntimeError as exc: + assert "did not contain a valid merge-base SHA" in str(exc) + else: + raise AssertionError("expected RuntimeError for malformed compare response") + + +def test_removed_file_context_section_without_merge_base_or_error(): + """No merge-base SHA and no recorded error must still be explicit, not silent.""" + context = noema.removed_file_context_section("owner/repo", "gone.py", "", "") + + assert "merge-base SHA unavailable for pre-deletion content" in context + + +def test_removed_file_context_section_empty_merge_base_content(monkeypatch): + """An empty (non-UTF-8-decodable) merge-base blob must be reported, not silently dropped.""" + monkeypatch.setattr(noema, "fetch_file_content_at_ref", lambda repo, path, ref: "") + + context = noema.removed_file_context_section("owner/repo", "gone.py", "c" * 40, "") + + assert "no UTF-8 text content available from merge-base content API" in context def test_removed_file_context_fails_closed_without_base_sha(monkeypatch): - """Missing base identity must be explicit and must not trigger a head fetch.""" + """Missing base identity must be explicit and must not trigger a content fetch.""" monkeypatch.setattr( noema, "fetch_changed_files", @@ -56,44 +131,49 @@ def test_removed_file_context_fails_closed_without_base_sha(monkeypatch): ) monkeypatch.setattr( noema, - "fetch_head_file_content", + "fetch_file_content_at_ref", lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("unexpected fetch")), ) - context = noema.changed_file_context("owner/repo", 7, "head-sha", "") + context = noema.changed_file_context("owner/repo", 7, "a" * 40, "") + + assert "PR base SHA was unavailable or malformed" in context + assert "Merge-base lookup unavailable" in context - assert "base SHA unavailable" in context +def test_removed_file_merge_base_content_failure_is_distinct_from_head_failure(monkeypatch): + """A merge-base content API failure must remain typed as merge-base evidence failure.""" + head_sha = "a" * 40 + base_sha = "b" * 40 + merge_base_sha = "c" * 40 -def test_removed_file_base_fetch_failure_is_distinct_from_head_failure(monkeypatch): - """A base-side API failure must remain typed as base evidence failure.""" monkeypatch.setattr( noema, "fetch_changed_files", lambda repo, number: [("gone.py", "removed")], ) + monkeypatch.setattr( + noema, "fetch_merge_base_sha", lambda repo, base, head: merge_base_sha + ) def fail_fetch(repo, path, ref): raise RuntimeError("HTTP 502: token ***") - monkeypatch.setattr(noema, "fetch_head_file_content", fail_fetch) + monkeypatch.setattr(noema, "fetch_file_content_at_ref", fail_fetch) - context = noema.changed_file_context( - "owner/repo", 7, "head-sha", "base-sha" - ) + context = noema.changed_file_context("owner/repo", 7, head_sha, base_sha) - assert "Unavailable from base content API" in context + assert "Unavailable from merge-base content API" in context assert "Unavailable from head content API" not in context def test_build_review_context_passes_live_base_ref(monkeypatch): """The GraphQL base identity must reach changed-file context construction.""" - observed: list[tuple[str, int, str, str]] = [] + observed: list[tuple[str, int, str, str, object]] = [] monkeypatch.setattr(noema, "review_thread_context", lambda pr: "") - monkeypatch.setattr(noema, "load_codegraph_context", lambda: "") - def fake_context(repo, number, head_sha, base_sha=""): - observed.append((repo, number, head_sha, base_sha)) + def fake_context(repo, number, head_sha, base_sha="", changed_files=None): + observed.append((repo, number, head_sha, base_sha, changed_files)) return "files" monkeypatch.setattr(noema, "changed_file_context", fake_context) @@ -104,5 +184,5 @@ def fake_context(repo, number, head_sha, base_sha=""): {"headRefOid": "head-sha", "baseRefOid": "base-sha"}, ) - assert observed == [("owner/repo", 7, "head-sha", "base-sha")] + assert observed == [("owner/repo", 7, "head-sha", "base-sha", None)] assert "## Changed file context\nfiles" in result diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 43aaf46e81..a86ee3b499 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1250,8 +1250,8 @@ def test_inspect_and_review_reports_stale_before_repair_retry_cleanly(monkeypatc monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") def fake_call_llm(*args, **kwargs): raise noema.StaleHeadDuringRepairRetryError( @@ -1694,15 +1694,15 @@ def test_current_actor_rejects_unbound_action_identity(monkeypatch, actor, insta noema.current_actor() -def test_review_context_builders_include_codegraph_threads_and_files(monkeypatch, tmp_path): +def test_review_context_builders_include_threads_and_files(monkeypatch, tmp_path): assert noema.truncate_text("abc", 10) == "abc" assert "truncated 2 characters" in noema.truncate_text("abcdef", 4) assert "missing PR head SHA" in noema.changed_file_context("owner/repo", 7, "") - original_fetch_paths = noema.fetch_changed_file_paths - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: []) + original_fetch_changed_files = noema.fetch_changed_files + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: []) assert "no changed files" in noema.changed_file_context("owner/repo", 7, "head") - monkeypatch.setattr(noema, "fetch_changed_file_paths", original_fetch_paths) + monkeypatch.setattr(noema, "fetch_changed_files", original_fetch_changed_files) encoded = base64.b64encode(b"print('hello')\n").decode("ascii") calls = [] @@ -1711,7 +1711,10 @@ def fake_run(args, stdin=None): calls.append(args) target = args[2] if target.endswith("/files"): - return "src/a.py\nREADME.md\nempty.txt\n" + return "\n".join( + json.dumps([path, "modified"]) + for path in ("src/a.py", "README.md", "empty.txt") + ) + "\n" if "contents/src/a.py" in target: return encoded if "contents/README.md" in target: @@ -1721,9 +1724,6 @@ def fake_run(args, stdin=None): raise AssertionError(args) monkeypatch.setattr(noema, "run", fake_run) - codegraph_path = tmp_path / "codegraph.md" - codegraph_path.write_text("call graph: src/a.py -> tests", encoding="utf-8") - monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(codegraph_path)) pr = make_pr( headRefOid="head sha", reviewThreads={ @@ -1747,8 +1747,6 @@ def fake_run(args, stdin=None): context = noema.build_review_context("owner/repo", 7, pr) - assert "## CodeGraph context" in context - assert "call graph: src/a.py -> tests" in context assert "Thread open at src/a.py:3" in context assert "reviewer: check call site" in context assert "### src/a.py" in context @@ -1758,16 +1756,14 @@ def fake_run(args, stdin=None): assert any("/files" in call[2] for call in calls) -def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch, tmp_path): - monkeypatch.delenv("NOEMA_CODEGRAPH_CONTEXT_PATH", raising=False) - assert noema.load_codegraph_context() == "" - - monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(tmp_path / "missing.md")) - assert "CodeGraph context unavailable" in noema.load_codegraph_context() - +def test_review_context_reports_omitted_files(monkeypatch, tmp_path): paths = [f"src/file_{index}.py" for index in range(noema.MAX_CONTEXT_FILES + 1)] - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths) - monkeypatch.setattr(noema, "fetch_head_file_content", lambda repo, path, head_sha: "x") + monkeypatch.setattr( + noema, + "fetch_changed_files", + lambda repo, number: [(path, "modified") for path in paths], + ) + monkeypatch.setattr(noema, "fetch_file_content_at_ref", lambda repo, path, ref: "x") context = noema.changed_file_context("owner/repo", 7, "head") @@ -2007,8 +2003,8 @@ def test_inspect_and_review_skip_paths(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) @@ -2048,8 +2044,8 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) @@ -2087,8 +2083,8 @@ def test_head_movement_stops_before_review_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr( noema, "call_llm", @@ -2110,8 +2106,8 @@ def test_closed_during_model_stops_before_review_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve"}) monkeypatch.setattr( noema, @@ -2129,8 +2125,8 @@ def test_uppercase_expected_head_is_not_stale_before_model_work(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) calls = [] monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) @@ -2146,8 +2142,8 @@ def test_uppercase_expected_head_is_not_stale_before_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr( noema, "call_llm", @@ -2168,8 +2164,8 @@ def test_inspect_and_review_rechecks_head_before_publication(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(responses)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve"}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: submitted.append(args)) From 19f48f4d6effa8e5227bc98694a6b1d057bfd792 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 01:56:47 +0000 Subject: [PATCH 11/15] test(ci): port stale-test fixes from #1663 (post-#1654/#1656/#1658) Syncing with current main pulled in #1651, #1656, and #1658 on top of #1654, all of which left pre-existing tests broken (see #1663's commit history for full root-cause detail on each). Ported the same fix already validated and opened as its own PR (#1663) rather than re-deriving it here: - scripts/ci/current_head_run_coalescer.py + 5 test files: removed two provably-unreachable dead-code checks (_run_matches_head_identity already subsumes the later event-type check; select_duplicate_queued_run_ids re-derived workflow_id behind a guard _run_identity_matches already enforces), added eight regression tests for genuinely-reachable but previously-untested branches in _run_pr_scope_is_safe and the sibling- authority loop, and fixed seven stale assertions (a retry-with-backoff loop now absorbs two test fixtures' synthetic sentinel exit codes; literal-text and renamed-field contract drift). - Three runner-image occurrence-count/job-presence assertions updated for #1656's removal of ten no-op cancel-closed-pr-runs jobs. - One literal LLM_TIMEOUT=300 assertion updated for #1658's removal of the 300s cap (now exports 0/unlimited directly). Full suite: 2605 passed, 100% branch coverage, 100% docstrings. --- scripts/ci/current_head_run_coalescer.py | 23 ++- tests/test_current_head_run_coalescer.py | 137 +++++++++++++++++- ...t_merge_scheduler_runner_image_contract.py | 1 - ...st_opencode_live_draft_state_regression.py | 15 +- ...st_opencode_required_verdict_regression.py | 22 ++- tests/test_queue_cancellation_revalidation.py | 2 +- ...t_queue_cancellation_scheduler_contract.py | 2 +- ...required_security_runner_image_contract.py | 11 +- .../test_required_workflow_queue_contract.py | 9 +- tests/test_strix_llm_timeout_contract.py | 2 +- 10 files changed, 199 insertions(+), 25 deletions(-) diff --git a/scripts/ci/current_head_run_coalescer.py b/scripts/ci/current_head_run_coalescer.py index cb0fadeea6..0c58d32263 100644 --- a/scripts/ci/current_head_run_coalescer.py +++ b/scripts/ci/current_head_run_coalescer.py @@ -140,7 +140,13 @@ def select_duplicate_queued_run_ids( branch: str, head_sha: str, ) -> list[int]: - """Select redundant queued runs while retaining one authoritative sibling.""" + """Select redundant queued runs while retaining one authoritative sibling. + + ``_run_identity_matches`` already requires a positive-int ``workflow_id`` + before a run reaches this loop body, so re-deriving it here is only ever + non-``None`` -- grouping unconditionally, rather than behind a redundant + ``is not None`` guard, avoids a branch no input can ever fail. + """ groups: dict[int, list[dict[str, Any]]] = {} for run_data in runs: if not _run_identity_matches( @@ -148,8 +154,7 @@ def select_duplicate_queued_run_ids( ): continue workflow_id = _positive_int(run_data.get("workflow_id")) - if workflow_id is not None: - groups.setdefault(workflow_id, []).append(run_data) + groups.setdefault(workflow_id, []).append(run_data) redundant: list[int] = [] for group in groups.values(): @@ -222,7 +227,15 @@ def validate_candidate_against_live_state( current_pr_number: int | None = None, associated_prs: Mapping[int, Mapping[str, Any]] | None = None, ) -> None: - """Fail closed unless a queued candidate still has an authoritative sibling.""" + """Fail closed unless a queued candidate still has an authoritative sibling. + + ``_run_matches_head_identity`` already rejects any candidate whose + ``event`` is not in ``PR_EVENTS`` before comparing repository, branch, or + SHA, so a non-pull-request candidate always fails the head-identity check + below rather than reaching a later, narrower event-only check -- there is + no candidate shape that can satisfy head identity while carrying a + disqualifying event. + """ if candidate.get("status") != "queued": raise CoalescingRefused("candidate is no longer queued") if live_pr.get("state") != "open": @@ -241,8 +254,6 @@ def validate_candidate_against_live_state( workflow_id = _positive_int(candidate.get("workflow_id")) if candidate_id is None or workflow_id is None: raise CoalescingRefused("candidate identity is malformed") - if candidate.get("event") not in PR_EVENTS: - raise CoalescingRefused("candidate is not a pull-request workflow run") association_map = associated_prs or {} if current_pr_number is not None and not _run_pr_scope_is_safe( diff --git a/tests/test_current_head_run_coalescer.py b/tests/test_current_head_run_coalescer.py index 33eee98d6c..38cc635ce8 100644 --- a/tests/test_current_head_run_coalescer.py +++ b/tests/test_current_head_run_coalescer.py @@ -126,6 +126,24 @@ def test_in_progress_run_is_never_selected_and_makes_queued_siblings_redundant() ) == [101, 102] +def test_group_with_no_queued_runs_has_nothing_to_coalesce() -> None: + """A workflow group whose only active runs are in-progress selects nothing. + + ``_run_identity_matches`` only admits runs whose ``status`` is queued or + in-progress, so a group can legitimately contain zero queued entries when + every admitted run for that workflow happens to already be running -- + the ``if not queued: continue`` guard exists precisely for that shape. + """ + module = load_module() + runs = [run_record(100, 10, status="in_progress"), run_record(101, 10, status="in_progress")] + assert module.select_duplicate_queued_run_ids( + runs, + repository="ContextualWisdomLab/.github", + branch="feature/current", + head_sha="a" * 40, + ) == [] + + def test_pull_request_target_uses_associated_pr_head_not_execution_head() -> None: """Trusted-base pull_request_target runs coalesce by their associated PR head.""" module = load_module() @@ -199,7 +217,7 @@ def test_revalidation_fails_closed_for_status_state_identity_and_event_changes() module.validate_candidate_against_live_state(candidate, live_pr=live_pr(head_sha="b" * 40), active_same_head_runs=[sibling]) with pytest.raises(module.CoalescingRefused, match="identity is malformed"): module.validate_candidate_against_live_state(run_record(0, 10), live_pr=live_pr(), active_same_head_runs=[sibling]) - with pytest.raises(module.CoalescingRefused, match="not a pull-request"): + with pytest.raises(module.CoalescingRefused, match="head moved"): module.validate_candidate_against_live_state(run_record(100, 10, event="push"), live_pr=live_pr(), active_same_head_runs=[sibling]) @@ -238,6 +256,123 @@ def test_pr_scope_rejects_other_open_pr_and_accepts_closed_matching_predecessor( ) +def test_pr_scope_unsafe_sibling_cannot_supply_authoritative_evidence() -> None: + """A sibling belonging to an independent open PR is skipped, not authoritative. + + Regression for the ``validate_candidate_against_live_state`` sibling loop + specifically (not the standalone ``_run_pr_scope_is_safe`` calls above): + a sibling that passes id/workflow/head-identity but belongs to a + different, still-open PR must be excluded from the authoritative-sibling + search entirely, not merely fail some other unrelated check. The bad + sibling's id (150) exceeds the candidate's (100), so if it were wrongly + treated as authoritative this would pass instead of failing closed. + """ + module = load_module() + candidate = run_record(100, 10, pr_number=1) + other_open = live_pr(number=2) + bad_sibling = run_record(150, 10, pr_number=2, associations=[pr_association(2)]) + with pytest.raises(module.CoalescingRefused, match="authoritative sibling"): + module.validate_candidate_against_live_state( + candidate, + live_pr=live_pr(), + active_same_head_runs=[candidate, bad_sibling], + current_pr_number=1, + associated_prs={2: other_open}, + ) + + +def test_pr_scope_rejects_a_run_with_no_pull_request_associations() -> None: + """An orphaned run with zero PR associations cannot claim any PR's scope.""" + module = load_module() + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_a_malformed_live_pr() -> None: + """A live PR missing head/base identity cannot authorize any scope decision.""" + module = load_module() + malformed_live_pr = { + "number": 1, + "state": "open", + "head": {"sha": "", "ref": "feature/current", "repo": {"full_name": "ContextualWisdomLab/.github"}}, + "base": {"sha": "c" * 40, "ref": "main", "repo": {"full_name": "ContextualWisdomLab/.github"}}, + } + assert not module._run_pr_scope_is_safe( + run_record(100, 10), + live_pr=malformed_live_pr, + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_an_association_with_a_malformed_number() -> None: + """An association carrying no positive-integer PR number is untrusted.""" + module = load_module() + malformed_association = { + **pr_association(1), + "number": None, + } + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[malformed_association]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_an_association_whose_head_does_not_match_live_head() -> None: + """An association reporting a different head than the live PR is untrusted.""" + module = load_module() + mismatched_association = pr_association(1, head_sha="b" * 40) + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[mismatched_association]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_an_association_whose_base_does_not_match_live_base() -> None: + """An association reporting a different base branch than the live PR is untrusted.""" + module = load_module() + mismatched_association = pr_association(1, base_ref="release") + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[mismatched_association]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_a_predecessor_number_missing_from_associated_prs() -> None: + """A closed-predecessor PR number with no fetched live state is untrusted.""" + module = load_module() + candidate = run_record(100, 10, pr_number=2, associations=[pr_association(2)]) + assert not module._run_pr_scope_is_safe( + candidate, + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_a_predecessor_whose_live_head_does_not_match() -> None: + """A fetched closed predecessor whose live head has since moved is untrusted.""" + module = load_module() + candidate = run_record(100, 10, pr_number=2, associations=[pr_association(2)]) + moved_predecessor = live_pr(state="closed", number=2, head_sha="b" * 40) + assert not module._run_pr_scope_is_safe( + candidate, + live_pr=live_pr(), + current_pr_number=1, + associated_prs={2: moved_predecessor}, + ) + + def test_revalidation_ignores_non_authoritative_sibling_shapes() -> None: """Different workflow or malformed sibling records cannot authorize cancellation.""" module = load_module() diff --git a/tests/test_merge_scheduler_runner_image_contract.py b/tests/test_merge_scheduler_runner_image_contract.py index caf7456df5..3334ac5b87 100644 --- a/tests/test_merge_scheduler_runner_image_contract.py +++ b/tests/test_merge_scheduler_runner_image_contract.py @@ -27,7 +27,6 @@ def test_queue_draining_jobs_use_explicit_supported_image(self) -> None: """Require the scheduler control plane to use explicit Ubuntu 24.04.""" workflow = WORKFLOW.read_text(encoding='utf-8') for job_name in ( - 'cancel-closed-pr-runs', 'scan-pr-queue', 'org-queue-sweep', ): diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index 18fd482b64..a9d9c518bc 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -134,11 +134,22 @@ def test_stale_draft_request_event_does_not_exempt_live_ready_pr( def test_stale_draft_verdict_event_does_not_exempt_live_ready_pr( tmp_path: Path, ) -> None: - """A stale draft verdict snapshot cannot publish a success for a ready PR.""" + """A stale draft verdict snapshot cannot publish a success for a ready PR. + + Unlike ``request_review_script()``'s single unguarded live-PR fetch, this + step's post-draft-check Reviews API poll retries a transport failure up + to ``max_poll_transport_failures`` times (with a real backoff sleep + between attempts) before failing closed with its own exit 1 and + diagnostic -- so the fixture's synthetic unmocked-call sentinel exit code + never reaches this script's own exit status, unlike the sibling test + above. The "stale" continuation message is still emitted first, proving + the step did not silently exempt the live-ready PR from verdict polling. + """ result = _run_step(tmp_path, fail_closed_script(), live_draft=False) - assert result.returncode == 19 + assert result.returncode == 1 assert "Event draft snapshot is stale" in result.stdout + assert "Reviews API read failed 3 consecutive times" in result.stdout @pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 4b098f9296..48d8808d10 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -268,9 +268,9 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non ) assert "Current-head substantive OpenCode verdict already exists; scheduler wake skipped." in dispatch_step assert "while :; do" in target_job - assert "sleep 30" in target_job + assert 'sleep "$poll_interval_seconds"' in target_job assert "enable_auto_merge:false" in workflow - assert 'gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews"' in workflow + assert 'gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews?per_page=100"' in workflow assert "github.event.pull_request.head.sha" in workflow assert "This required check is not a review and must not succeed" in workflow assert ( @@ -309,7 +309,7 @@ def _run_fail_closed_step( A fake ``gh`` that fails loudly is installed on ``PATH`` so a closed or draft early exit that reaches the Reviews API call at all fails the test immediately, rather than actually looping (the production step's - ``while :; do ... sleep 30; done`` never naturally terminates on a + ``while :; do ... sleep "$poll_interval_seconds"; done`` never naturally terminates on a non-matching review, so a real ``gh`` fixture serving no match would hang a test rather than fail it). """ @@ -355,7 +355,7 @@ def test_fail_closed_step_exempts_a_draft_pr_before_polling(tmp_path: Path) -> N (`scripts/ci/pr_review_merge_scheduler.py`'s `inspect_pr`) skips dispatching a review for an ordinary draft entirely (no `@opencode-agent` mention). With no draft exemption here, this step's - `while :; do ... sleep 30; done` loop would poll for a verdict OpenCode + `while :; do ... sleep "$poll_interval_seconds"; done` loop would poll for a verdict OpenCode will never post, until the job's own ~360-minute runtime ceiling kills it -- reproduced against this exact commit before this fix (`#1443` fixed the same class of bug on a now-superseded design; this restores @@ -523,10 +523,20 @@ def test_fail_closed_step_closed_still_takes_precedence_over_draft(tmp_path: Pat def test_fail_closed_step_still_polls_for_a_non_draft_pr(tmp_path: Path) -> None: - """A non-draft PR must still reach the Reviews API call (not exempted).""" + """A non-draft PR must still reach the Reviews API call (not exempted). + + Unlike the request-review step's single unguarded call, the Reviews API + fetch here retries a transport failure up to three times (with a real + backoff sleep between attempts) before failing closed with its own exit + 1, so the fixture's synthetic unmocked-call sentinel exit code (17) + never reaches this script's own exit status -- it is absorbed by the + retry loop instead, which still logs the sentinel's stderr diagnostic on + every attempt. + """ result = _run_fail_closed_step(tmp_path, pr_action="synchronize", pr_draft="false") - assert result.returncode == 17, result.stderr + assert result.returncode == 1, result.stderr assert "unexpected gh invocation after live-state validation" in result.stderr + assert "Reviews API read failed 3 consecutive times" in result.stdout @pytest.mark.parametrize( diff --git a/tests/test_queue_cancellation_revalidation.py b/tests/test_queue_cancellation_revalidation.py index 23ac824d59..e8e6b57492 100644 --- a/tests/test_queue_cancellation_revalidation.py +++ b/tests/test_queue_cancellation_revalidation.py @@ -322,7 +322,7 @@ def test_unassociated_aged_pr_uses_live_ref_not_stale_listing_sha( run_sha=current, ) assert result.returncode == 0, result.stderr - assert "authoritative current-head evidence" in result.stdout + assert "associated with an open PR at its authoritative current head" in result.stdout assert not cancelled diff --git a/tests/test_queue_cancellation_scheduler_contract.py b/tests/test_queue_cancellation_scheduler_contract.py index 16c291a6f1..70f9d75e8c 100644 --- a/tests/test_queue_cancellation_scheduler_contract.py +++ b/tests/test_queue_cancellation_scheduler_contract.py @@ -45,7 +45,7 @@ def test_reconciled_scheduler_preserves_current_main_control_plane_fixes() -> No assert '- cron: "0 * * * *"' in workflow assert '*/15 * * * *' not in workflow - assert workflow.count("runs-on: ubuntu-24.04") >= 3 + assert workflow.count("runs-on: ubuntu-24.04") >= 2 scan_job = workflow.split(" scan-pr-queue:", 1)[1].split(" org-queue-sweep:", 1)[0] assert "github.event_name == 'pull_request_review'" in scan_job.split( "TRIGGER_REVIEWS:", 1 diff --git a/tests/test_required_security_runner_image_contract.py b/tests/test_required_security_runner_image_contract.py index 2a11d1ca5d..699e4dde3f 100644 --- a/tests/test_required_security_runner_image_contract.py +++ b/tests/test_required_security_runner_image_contract.py @@ -17,13 +17,18 @@ def test_security_scan_uses_explicit_supported_image(self) -> None: """Require every Security Scan job to use explicit Ubuntu 24.04.""" workflow = SECURITY_SCAN.read_text(encoding="utf-8") self.assertNotIn("runs-on: ubuntu-latest", workflow) - self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 5) + self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 4) def test_sast_semgrep_uses_explicit_supported_image(self) -> None: - """Require both SAST Semgrep jobs to use explicit Ubuntu 24.04.""" + """Require the SAST Semgrep job to use explicit Ubuntu 24.04. + + `#1656` removed the sibling `cancel-closed-pr-runs` no-op job (it + only duplicated PR-stable workflow concurrency), leaving one runner + job in this workflow instead of two. + """ workflow = SAST_SEMGREP.read_text(encoding="utf-8") self.assertNotIn("runs-on: ubuntu-latest", workflow) - self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 2) + self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 1) if __name__ == "__main__": diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index b49e968d6a..056e47a678 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1037,11 +1037,14 @@ def test_org_queue_sweep_covers_target_repositories_on_a_heartbeat() -> None: assert '"pull_request" or .event == "pull_request_target"' in workflow assert "$current_pr_head == null or .head_sha != $current_pr_head" in workflow assert ".head_sha != $current_default_sha" in workflow - assert "do not match an open PR or default-branch Current HEAD" in workflow + assert "classified as not matching an open PR or default-branch Current HEAD" in workflow assert '.current_head // "closed-or-no-open-pr"' in workflow assert '.current_head // \\"closed-or-no-open-pr\\"' not in workflow assert "select($current_pr_heads[$head_key] == null)" in workflow - assert "Could not cancel superseded run" in workflow + revalidate_script = ( + REPO_ROOT / "scripts" / "ci" / "revalidate_queue_cancellation.sh" + ).read_text(encoding="utf-8") + assert "Could not cancel ${cancellation_mode} run" in revalidate_script assert "No run will be cancelled from incomplete evidence" in workflow assert "queue_hygiene_ready=false" in workflow # Organization sweep budgets must be consumed across the repository loop; @@ -1103,7 +1106,7 @@ def test_org_queue_sweep_superseded_run_log_filter_executes() -> None: ) assert result.returncode == 0, result.stderr - assert "current_head=closed-or-no-open-pr" in result.stdout + assert "classified_head=closed-or-no-open-pr" in result.stdout def _extract_org_sweep_rotation_snippet(workflow: str) -> str: diff --git a/tests/test_strix_llm_timeout_contract.py b/tests/test_strix_llm_timeout_contract.py index 62b0563bbc..8661486441 100644 --- a/tests/test_strix_llm_timeout_contract.py +++ b/tests/test_strix_llm_timeout_contract.py @@ -44,7 +44,7 @@ def test_strix_timeout_compat_is_installed_after_the_pinned_runtime() -> None: workflow = WORKFLOW.read_text(encoding="utf-8") token_loader = TOKEN_LOADER.read_text(encoding="utf-8") - assert "export LLM_TIMEOUT=300" in workflow + assert "export LLM_TIMEOUT=0" in workflow assert 'if [ -n "${STRIX_EXECUTABLE_PATH:-}" ]; then' in token_loader assert "install_strix_timeout_compat.py" in token_loader assert INSTALLER.is_file() From bdcb3208185fa4cb62b23fe65a065d375910e501 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 02:11:37 +0000 Subject: [PATCH 12/15] docs: correct stale "#1477 pending" pin-history claims Devin Review (this PR, scripts/ci/contextual_orchestrator_review_launcher.py lines 129-169) flagged that the docstrings describe ContextualWisdomLab/.github#1477 (the ORCHESTRATOR_PIN_SHA bump to contextual-orchestrator#949's merge commit) as still pending. Verified: #1477 merged on 2026-08-31 -- before this PR's original evidence_only/spend_admitted fix even landed -- and `git merge-base --is-ancestor 8cd99f139915131ba0239bce12a5d6a5fd85394e 045d17da5e2aea56a97e241ee158ab1628d78660` against contextual-orchestrator confirms the current pin already descends from #949's fix commit. The "not yet pinned" / "once #1477 merges" framing was therefore stale runtime history that would mislead future maintenance into thinking the pin bump was still outstanding. Updates both affected docstrings in the launcher module and the two docstrings in tests/test_contextual_orchestrator_review_runtime_preflight.py that repeated the same stale framing. Docs-only: no behavior change, no assertion changed. docs/product-technical-gap-baseline.md's own #1477 references are left untouched -- they are dated, explicitly time-qualified ("open as of this correction") historical entries in this repo's append-only decision log, not living claims about current state, so rewriting them would falsify the audit trail rather than correct it. Verified: `python -m pytest tests/test_contextual_orchestrator_review_runtime_preflight.py tests/test_contextual_orchestrator_review_sidecar_contract.py -q` -- 95 passed; `interrogate scripts/ci/contextual_orchestrator_review_launcher.py` -- 100%. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- ...contextual_orchestrator_review_launcher.py | 52 +++++++++++-------- ...l_orchestrator_review_runtime_preflight.py | 10 ++-- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index a55be68400..cc237415d0 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -161,12 +161,21 @@ def _openrouter_reports_per_model_evidence(discovered: list[object]) -> bool: which commit is nominally pinned. See ``ContextualWisdomLab/.github#1476`` (this change) and ``ContextualWisdomLab/contextual-orchestrator#949`` (the merged upstream fix; pinned by ``ContextualWisdomLab/.github#1477``) - for the full history; once ``#1477`` merges, no further change is - required here -- this check starts reporting ``True`` as soon as the - vendored pin actually includes the fix and OpenRouter has discovered at - least one model that run (``#949`` makes ``evidence_only=False`` - unconditional for OpenRouter, not contingent on that model being - ZDR-attested). + for the full history. ``#1477`` merged on 2026-08-31, bumping + ``ORCHESTRATOR_PIN_SHA`` (in + ``scripts/ci/contextual_orchestrator_review_sidecar.sh``) to + ``045d17da5e2aea56a97e241ee158ab1628d78660`` -- confirmed by + ``git merge-base --is-ancestor 8cd99f139915131ba0239bce12a5d6a5fd85394e + 045d17da5e2aea56a97e241ee158ab1628d78660`` to already include ``#949``'s + merge commit -- so this check now reports ``True`` as soon as + OpenRouter has discovered at least one model that run (``#949`` makes + ``evidence_only=False`` unconditional for OpenRouter, not contingent on + that model being ZDR-attested). The observed-behavior approach + documented above is kept rather than switched to a direct pin-ancestry + comparison: it needs no new plumbing, and it keeps working even if a + future pin ever regresses behind ``#949`` (a downgrade, a re-vendor from + a stale fork) without a matching ``ORCHESTRATOR_PIN_SHA`` edit being + noticed first. KNOWN, ACCEPTED LIMITATION: a genuinely-fixed vendored copy that happens to report ``evidence_only=True`` for *every* OpenRouter row in @@ -227,8 +236,10 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] ``evidence_only=True`` for every discovered model unconditionally (a confirmed bug, fixed upstream at ``ContextualWisdomLab/contextual-orchestrator#949``, merged at - ``8cd99f139915131ba0239bce12a5d6a5fd85394e`` -- not yet pinned in this - repo as of this writing; see ``ContextualWisdomLab/.github#1477``) -- + ``8cd99f139915131ba0239bce12a5d6a5fd85394e`` -- pinned in this repo by + ``ContextualWisdomLab/.github#1477``, merged 2026-08-31, whose current + ``ORCHESTRATOR_PIN_SHA`` (``045d17da5e2aea56a97e241ee158ab1628d78660``) + is confirmed to descend from that fix commit) -- not computed per model from real evidence, even though genuine per-model ZDR evidence is fetched and parsed for OpenRouter in that same module. Applying this filter to OpenRouter verbatim while @@ -249,19 +260,18 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] ``_has_text_output``, in ``main()``) regardless of which branch this function takes. - This exemption is expected to have real, live effect once merged (not - only once ``contextual-orchestrator``'s own per-model ``evidence_only`` - fix and a matching ``ORCHESTRATOR_PIN_SHA`` bump land): OpenRouter - discovery already runs in this sidecar today, so genuinely chat-capable - OpenRouter rows -- currently blocked here regardless of what - ``contextual-orchestrator`` reports -- start reaching selection - immediately. What remains genuinely blocked on the upstream fix is - OpenRouter rows being correctly excluded from ``evidence_only`` on a - real per-model basis (e.g. a non-chat listing); until - ``ContextualWisdomLab/.github#1477`` lands the ``#949`` pin bump and a - run observes real per-model variation, this function's remaining - protection against those is the same downstream chat-capability check, - not ``evidence_only``. + This exemption has real, live effect as of this merge, on two + timelines that were originally expected to be sequential but have both + already landed by the time this docstring was last corrected: + OpenRouter discovery already runs in this sidecar, so genuinely + chat-capable OpenRouter rows -- previously blocked here regardless of + what ``contextual-orchestrator`` reported -- reach selection + immediately upon this PR merging; and, separately, + ``ContextualWisdomLab/.github#1477`` already landed the ``#949`` pin + bump (see above), so a run that observes real per-model variation gets + OpenRouter rows correctly excluded from ``evidence_only`` on a genuine + per-model basis (e.g. a non-chat listing) rather than relying solely on + the downstream chat-capability check for that protection. A row is also excluded whenever ``getattr(model, "spend_admitted", True) is False`` -- the same treatment as ``evidence_only=True``, with diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 78c876ae23..33991b7d5a 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -116,9 +116,9 @@ def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_ev """OpenRouter rows are exempt from evidence_only while every row still shows it. Regression for a confirmed bug, fixed upstream at - ``ContextualWisdomLab/contextual-orchestrator#949`` (merged, not yet - pinned in this repo as of this writing -- see - ``ContextualWisdomLab/.github#1477``): ``contextual-orchestrator``'s + ``ContextualWisdomLab/contextual-orchestrator#949`` and pinned in this + repo by ``ContextualWisdomLab/.github#1477`` (merged 2026-08-31): + ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` currently hardcodes ``evidence_only=True`` for every discovered model unconditionally (not computed per model from real evidence) -- so today's real signature is @@ -162,8 +162,8 @@ def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_ev def test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence() -> None: """The historical exemption turns off the moment per-model evidence appears. - Once ``ContextualWisdomLab/.github#1477`` lands the - ``ContextualWisdomLab/contextual-orchestrator#949`` pin bump, OpenRouter starts reporting real + Now that ``ContextualWisdomLab/.github#1477`` has landed the + ``ContextualWisdomLab/contextual-orchestrator#949`` pin bump, OpenRouter reports real per-model ``evidence_only`` (at minimum ``False`` for its genuinely ZDR-attested free models). This is the post-fix signature: a run whose OpenRouter rows are no longer uniformly ``True`` must go back through From 1d2d103f604f8b120199debe38125898611651fe Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 09:22:53 +0000 Subject: [PATCH 13/15] docs: remove stale duplicate SIGPIPE-flake paragraph Devin Review (this PR, docs/product-technical-gap-baseline.md lines 2651-2655) flagged that the SIGPIPE-flake note appeared twice back to back with contradictory status: the first copy said the flake "remains" unremediated ("Not remediated here"), immediately followed by a second copy that says the same thing in past tense and then adds "Since remediated (9e0c0224, fix(test): eliminate scheduler-wake SIGPIPE flake)". A recent merge from origin/main (5952e5b5, not authored in this turn) brought this branch's own original present-tense paragraph back alongside main's already-corrected past-tense-plus-remediation version. The second paragraph fully supersedes the first (same test, same root cause, plus the remediation commit and PR the first paragraph predates), so this removes the stale first copy rather than keeping both -- unlike this repo's usual append-only convention for genuinely independent entries, a paragraph a later paragraph explicitly says is "since remediated" is not independent information worth preserving twice. No test pins the removed prose (grepped tests/ for "One test in the full suite rema" -- no hits). Verified: full suite already re-run on this head before this fix (2600 passed, 100% coverage, 100% docstrings); this is a docs-only follow-up with no code change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 3ef867c180..7c0c557366 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2648,11 +2648,6 @@ exact same head showing both files at 100% branch coverage with zero missing bra this evidence on the review thread and did not widen the PR's diff for a claim that does not hold against this repo's own tooling. -**One test in the full suite remains a known, pre-existing flake**, unrelated to this change: -`tests/test_opencode_required_verdict_regression.py::test_scheduler_wake_reuses_trusted_receipt_predicate` -intermittently exits 141 (SIGPIPE) under full-suite parallel load; reproduces identically on -unmodified `origin/main` and passes cleanly in file isolation. Not remediated here — out of scope -for a coverage-gap-only PR, and not itself a coverage regression. **One test in the full suite remained a known, pre-existing flake**, unrelated to this change: `tests/test_opencode_required_verdict_regression.py::test_scheduler_wake_reuses_trusted_receipt_predicate` intermittently exited 141 (SIGPIPE) under full-suite parallel load; reproduced identically on From dc35b1da63cfd509e5f973f1956466187912071d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 19:08:03 +0000 Subject: [PATCH 14/15] fix(ci): update test_strix_quick_gate.sh's stale cron assertion from #1630 pr-review-merge-scheduler.yml's repository-local heartbeat was lengthened from cron: "*/30 * * * *" to cron: "30 * * * *" by #1630 to reduce Actions-capacity pressure during organization-wide queue saturation. tests/test_actions_queue_saturation_scheduler_cadence.py was updated to match at the time, but the parallel bash contract in scripts/ci/test_strix_quick_gate.sh was not, and kept asserting the literal old string -- a genuine, reproducible defect on protected main itself (confirmed failing on a fresh unmodified main clone before this change), not a symptom of any one PR being stale. Since exact-head-path-policy runs this trusted base-branch script against every PR's own exact head, this silently blocked an unbounded number of unrelated PRs across the whole .github queue until fixed at the root. Updates the one stale assertion to the current cron string and corrects an adjacent stale "15-minute organization sweep / 30-minute scheduled scan" description to the current hourly/hourly cadence. Verified: bash scripts/ci/test_strix_quick_gate.sh -- FAIL before this change on unmodified main, PASS after. Full suite: coverage run -m pytest tests -q -- all passed; coverage report --fail-under=100 -- 100% on scripts/ci/; interrogate -- 100%. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 71 +++++++++++++++++++++++ docs/product-technical-gap-baseline.md | 79 ++++++++++++++++++++++++++ scripts/ci/test_strix_quick_gate.sh | 4 +- 3 files changed, 152 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70532ec0f1..9e01a01c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,77 @@ Semantic Versioning where the repository publishes a release. also now excludes a `spend_admitted=False` row the same way it excludes `evidence_only=True`, so a credit-exhausted priced OpenRouter row cannot reach `orchestrator/auto`'s served catalog. +- **Fix a stale `test_strix_quick_gate.sh` assertion left broken by the `#1630` + scheduler-cadence lengthening.** `pr-review-merge-scheduler.yml`'s repository-local + heartbeat was changed from a quarter-hourly `cron: "*/30 * * * *"` to an hourly + `cron: "30 * * * *"` (see `docs/doctoring/actions-queue-saturation-hourly-sweep.md`), + and the Python regression `tests/test_actions_queue_saturation_scheduler_cadence.py` + was updated to match at the time — but the parallel bash contract in + `scripts/ci/test_strix_quick_gate.sh` still asserted the literal old string, so + every PR whose required `exact-head-path-policy` check ran this script against a + current `main` checkout failed on an assertion the workflow file itself could no + longer satisfy, regardless of the PR's own diff. Updated the assertion to the + current cron string and corrected an adjacent stale "15-minute organization sweep + / 30-minute scheduled scan" description to the current hourly/hourly cadence. + Verified: `bash scripts/ci/test_strix_quick_gate.sh` now passes against unmodified + `main` (confirmed failing before this fix, on the same clean clone); full suite + unaffected (2600+ passed, 100% coverage, 100% docstrings) since this is a + bash-only assertion string with no Python-side counterpart to update. +- **Consolidate the two genuinely duplicate quality-CI callers behind one reusable + `workflow_call` gate; leave the other six alone.** An audit of the 8 + `.github/workflows/*-quality-ci.yml` bootstrap-templated files found only one pair — + `javascript-coverage-quality-ci.yml` and + `organization-commercial-readiness-loop-quality-ci.yml` — where the shared skeleton + (checkout at the exact PR head, an identical pinned six-package mini-requirements + heredoc, `coverage run --branch -m pytest --import-mode=importlib`, `coverage report + --fail-under=100`, `compileall`, `git diff --exit-code`) was byte-for-byte the same + logic with only the timeout, pytest target, and coverage `--include` path varying per + subsystem. Extracted that shared shape into a new + `.github/workflows/exact-head-coverage-quality-gate.yml` reusable workflow + (`workflow_call`-only, four required inputs: `timeout_minutes`, `pytest_target`, + `coverage_include`, `compileall_targets`) and turned both callers into thin + `uses:`/`with:` wrappers. Verified first that no branch-protection required status + check or the org's required-workflow ruleset references either caller's job name + (`exact-head-coverage-contract` / `exact-head-policy`) before restructuring, so nothing + downstream depends on their exact shape. Updated the three contract tests that pinned + the old inline text + (`test_organization_commercial_readiness_loop_policy.py`, + `test_organization_commercial_readiness_loop_import_contract.py`) to check the + coverage/exact-head mechanics against the shared gate file and the subsystem wiring + against each caller, and added + `tests/test_exact_head_coverage_quality_gate_contract.py` to pin the gate's own + `workflow_call` contract and both callers' input wiring. The other 6 files + (`agent-mention-router-quality-ci.yml`, `exact-artifact-sbom-attestation-quality.yml`, + `noema-token-lifetime-quality-ci.yml`, + `opencode-rust-coverage-toolchain-quality-ci.yml`, `strix-changed-path-quality-ci.yml`, + `trusted-uv-materializer-quality-ci.yml`) look superficially similar but each encodes a + genuinely different policy -- harden-runner presence, a docstring/interrogate gate, + exact-head-verification mechanics (or, for noema, no `ref:` pin at all), multi-Python- + version matrices with non-shared extra logic (a tomli-fallback exercise, a Python 3.10 + compile-only contract), or no `coverage --fail-under` step at all (strix delegates to a + bash gate script instead) -- so templatizing them would either weaken what they + individually enforce or need enough per-caller toggles to defeat the point of sharing. + Left untouched, matching the precedent already set for ruling out the agent-mention + dispatch pair and the noema/opencode/strix "cancel superseded runs" jobs. Full suite: + 2603 passed, 1 skipped, 100% branch coverage, 100% docstrings, `actionlint` clean. +- **Fail closed before cancelling stale PR workflow runs.** Validate snapshot `headRefOid` and re-read live PR/run identity immediately before destructive cancellation, including OpenCode/Strix dispatch cleanup, so a missing head or concurrent push cannot cancel the sole current-head evidence or trigger a duplicate review. Also ensures every cancellation path (`cancel_stale_pr_runs`, `cancel_stale_opencode_runs`, `_cancel_revalidated_review_run_refs`) treats a run as cancelled only when `force_cancel_workflow_runs` actually reports success, not merely when live revalidation proved it stale -- superseding PR #1712's simpler `force_cancel_workflow_run_refs` wrapper (removed as dead code; its safety guarantee is preserved inline at every call site by this more thorough revalidate-then-cancel design). +- **Cache `active_workflow_runs` for the life of one `pr_review_merge_scheduler.py` + invocation.** `inspect_pr()` calls `cancel_stale_pr_runs()` unconditionally for + every non-draft PR before any eligibility gate, and several other call sites + (`active_review_run_refs`, `dispatch_strix_evidence`'s busy check) ask the + identical unfiltered `(repo, ("queued", "in_progress"))` question again -- + all against the one repository a scheduler invocation ever targets, with zero + caching anywhere in the file. At the default `MAX_PRS=100` this reissued the + same repository-wide, paginated `gh api .../actions/runs` fetch well over a + hundred times per run. `active_workflow_runs` now memoizes its result keyed on + the full `(repo, statuses, event, created, head_sha)` call shape for one + `main()` invocation, with explicit cache invalidation immediately after the + four places that mutate GitHub Actions run state + (`force_cancel_workflow_runs`, `rerun_actions_job`, `dispatch_opencode_review`, + `dispatch_strix_evidence`) so a later read in the same run can never replay a + pre-mutation snapshot. The four pre-existing `ThreadPoolExecutor` sites and the + correctly-sequential per-PR mutation-budget loop are untouched. See + ADR-0022. - **Consolidate the 18 per-repository hourly review-repair caller workflows into one file.** At the repository owner's request ("이런 Workflow는 단일 파일로 통합하라"), replaced `accounting-information-platform-`, `afipc-`, `bandscope-`, `clearfolio-`, diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 7c0c557366..0aa00b8e9d 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2838,3 +2838,82 @@ Higgins, S. S., Crepalde, N., & Fernandes, L. (2021). Segmented multiplexity: A **Expected effect.** No observable change to any current GitHub Actions review run (every current invocation already resolves to `free`). The effect is structural: it is no longer possible for a future workflow edit or manual dispatch override to admit priced-model spend into a required review check without an explicit, reviewed code change to this one `case` statement (and its now-locked-in regression test) first. **Follow-up.** If the organization later solves free+ZDR routing robustly enough to deliberately widen required-review CI to `orchestrator/auto` (e.g. once a spend ceiling and reviewer-visible cost evidence exist for that path), the change is exactly one `case` arm plus the corresponding assertions in `test_sidecar_pins_the_pool_to_free_for_github_actions` — this entry is the record of *why* it was narrowed, not a permanent prohibition. + +## Noema single-request model-control ownership — PR #1672 (2026-09-02) + +**Status:** Merged into protected `main` as `a28fc2f4e185df7847e2f2f5f6ec561d1e84805d`; fresh exact-head hosted evidence remains an operational acceptance item. + +**Root cause.** Noema duplicated contextual-orchestrator structured-output repair by making a second model request and wrapped that request in an unmeasured 900-second repository wall-clock deadline. This created a self-hosting admission failure: valid long inference could be terminated by a policy that the gateway already owns. + +**Context Map / responsibility boundary.** `.github` owns CI review orchestration, exact-revision evidence, deterministic verdict validation, and publication. `contextual-orchestrator` owns provider discovery, capability routing, `orchestrator/free`, structured-output repair/failover, and provider completion. No provider/model-specific fallback or caller wall-clock timeout crosses that boundary. + +**Action delivered.** The recursive caller repair and fixed deadline/signal machinery were removed. Noema now sends one structured-output request, keeps exact-head checks before and after model work, sanitizes serving-model telemetry, restores exact changed-line diagnostics, and retains bounded non-heuristic evidence cardinality with strict local JSON parsing. + +**900-second clarification.** The historical `NoemaRepairDeadlineExceeded` from the html4tree incident came from the retired caller repair path. The three literal `timeout --kill-after=20 900` invocations still present in `opencode-review-dispatch.yml` are separate containment limits for untrusted test-measurement commands; they are not model or Noema inference timeouts. Telemetry and runbooks must report the command class and phase separately. + +**Evidence / acceptance.** Permanent tests forbid retry/deadline/sampling symbols in the caller and prove one gateway request, one attempt annotation, control-character-safe telemetry, missing-value rejection, valid trailing-comma normalization, and exact changed-line guidance. Fresh exact-head repository checks and reviews remain the admission authority; predecessor-head evidence is not transferable. The remaining runtime work is to preserve distinct `request_too_large`, discovery, rate-limit, provider transport, malformed-output, stale-head, and sandbox-command-timeout categories in hosted logs. + +## 2026-09-02 `test_strix_quick_gate.sh` stale cron assertion left broken by the `#1630` cadence lengthening + +**Problem.** The required `exact-head-path-policy` check (which runs `bash +scripts/ci/test_strix_quick_gate.sh` against the exact PR head) was failing on +multiple, unrelated open PRs (observed directly on `.github#1476`, a PR whose own +diff never touches this script or the scheduler workflow) with: + +``` +FAIL: scheduler wakes frequently enough to clear auto-merge PRs that become stale +after their initial PR events (missing 'cron: "*/30 * * * *"') +``` + +**Root cause.** `#1630` (referenced in `docs/doctoring/actions-queue-saturation-hourly-sweep.md`) +deliberately lengthened `pr-review-merge-scheduler.yml`'s repository-local heartbeat +from a quarter-hourly `cron: "*/30 * * * *"` to an hourly `cron: "30 * * * *"` to +reduce Actions-capacity pressure during the sustained organization-wide queue +saturation this session repeatedly documented. The Python regression +`tests/test_actions_queue_saturation_scheduler_cadence.py` was correctly updated at +the time (it now asserts `'- cron: "30 * * * *"' in workflow` and explicitly +`'*/30 * * * *' not in workflow`) — but the parallel bash contract test, +`scripts/ci/test_strix_quick_gate.sh`, was not, and kept asserting the literal old +string. This is a genuine, reproducible defect on protected `main` itself, not a +symptom of any one PR being stale: I confirmed it by running the script directly +against an unmodified, freshly cloned `main` (commit `8c085835`) before making any +change, and it failed with the identical message. + +**Why this matters at organization scale.** `exact-head-path-policy` is a required +check for every PR touching Strix-quick-gate-covered paths, checked out against +each PR's own exact head but running this trusted base-branch script. Since the +assertion can never pass against the current, correctly-updated workflow file, this +was a standing, silent block on an unbounded number of unrelated PRs across the +whole `.github` PR queue until fixed at the root -- exactly the class of "root +cause outside any one PR's diff" issue this session's operating directive requires +be fixed at the canonical location rather than worked around per-PR. + +**Fix.** Updated the one stale assertion (`scripts/ci/test_strix_quick_gate.sh`) +from `'cron: "*/30 * * * *"'` to `'cron: "30 * * * *"'`, matching the workflow's +actual current value and the already-correct Python-side assertion. Also corrected +an adjacent stale human-readable description ("scheduler isolates the 15-minute +organization sweep from the separate 30-minute scheduled scan") to the current +hourly/hourly cadence -- both `org-queue-sweep` and this repository-local scan are +now hourly, so the old minute figures described a schedule that no longer exists. + +**Verification.** `bash scripts/ci/test_strix_quick_gate.sh` — confirmed FAIL on +unmodified `main` before the change, confirmed PASS after. Full suite: +`coverage run -m pytest tests -q` — all passed; `coverage report --fail-under=100` +— 100% on `scripts/ci/`; `interrogate` — 100%. This is a bash-string-only fix with +no Python production code touched, so the full-suite pass is a non-regression +check, not evidence the fix itself works — the direct before/after script run is +that evidence. + +**Risk of this fix itself.** Essentially none: a one-line literal-string update in +a test assertion, verified to both fail before and pass after against the exact +same unmodified `main` checkout. No workflow, script, or other test file changed. + +**Expected effect.** `exact-head-path-policy` stops failing organization-wide PRs +on this assertion once this fix reaches protected `main`; any PR whose branch has +already synced past this point (or syncs after) picks it up automatically. + +**Follow-up.** None identified — this closes the specific gap. If a future cadence +change lands again, the durable fix is process, not code: update every test that +asserts the literal cron string (currently exactly these two files) in the same PR +that changes the cron value, per this repo's own "contract tests pin workflows AND +prose" convention already stated in `CLAUDE.md`. diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index d5db849145..c248b4a7ac 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1559,11 +1559,11 @@ assert_pr_review_merge_scheduler_uses_github_actions_bot_token() { assert_file_contains "$workflow_file" 'pull_request_target:' "scheduler can run as an organization required workflow without repository-local copies" assert_file_contains "$workflow_file" 'auto_merge_enabled' "scheduler rechecks already stale PRs as soon as native auto-merge is enabled" assert_file_contains "$workflow_file" 'workflows: ["Required OpenCode Review", "Strix Security Scan"]' "scheduler reruns after review or security evidence completion so approvals can trigger merge/update actions" - assert_file_contains "$workflow_file" 'cron: "*/30 * * * *"' "scheduler wakes frequently enough to clear auto-merge PRs that become stale after their initial PR events" + assert_file_contains "$workflow_file" 'cron: "30 * * * *"' "scheduler wakes frequently enough to clear auto-merge PRs that become stale after their initial PR events" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "scheduler must not hard-code repository-specific PR bypasses" assert_file_contains "$workflow_file" "github.event_name == 'pull_request_target' && format('pr-{0}', github.event.pull_request.number)" "scheduler scopes pull_request_target concurrency to the active PR" assert_file_contains "$workflow_file" "github.event_name == 'workflow_run' && github.event.workflow_run.pull_requests[0].number && format('pr-{0}', github.event.workflow_run.pull_requests[0].number)" "scheduler scopes workflow_run concurrency to the completed review PR" - assert_file_contains "$workflow_file" "github.event_name == 'schedule' && format('schedule-{0}', github.event.schedule)" "scheduler isolates the 15-minute organization sweep from the separate 30-minute scheduled scan" + assert_file_contains "$workflow_file" "github.event_name == 'schedule' && format('schedule-{0}', github.event.schedule)" "scheduler isolates the hourly organization sweep from the separate hourly repository-local scan" assert_file_contains "$workflow_file" "github.event_name == 'repository_dispatch' && github.event.client_payload.target_repository != '' && github.event.client_payload.pr_number != ''" "scheduler scopes targeted manual queue scans to the requested PR" assert_file_contains "$workflow_file" "cancel-in-progress: \${{ github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review' || github.event_name == 'repository_dispatch' || (github.event_name == 'workflow_run' && !github.event.workflow_run.pull_requests[0].number) }}" "scheduler cancels stale PR/review/manual queue scans instead of accumulating merge/update attempts" assert_file_contains "$workflow_file" "timeout-minutes: 60" "organization sweep has enough headroom to finish the complete repository walk" From 92f3a432da4ab6e3c528653093513338b956f685 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 19:26:02 +0000 Subject: [PATCH 15/15] docs: fix CodeRabbit-flagged prose issues on #1476 - CHANGELOG.md / gap-baseline.md: cron: "*/30 * * * *" is a 30-minute (half-hourly) cadence, not "quarter-hourly" as previously described. - gap-baseline.md: add a `text` language identifier to the fenced FAIL block (markdownlint MD040) and fix a self-contradictory scope statement that said "no ... script ... changed" immediately after describing a change to scripts/ci/test_strix_quick_gate.sh. - contextual_orchestrator_review_launcher.py and its tests: reword the vendored-OpenRouter-bug prose from present tense ("currently hardcodes", "today's bug") to the correct conditional framing -- the blanket evidence_only=True bug only applies under a pin predating contextual-orchestrator#949; this repo's current pin already descends from that fix, so the exemption in _routable_discovered_models exists as regression protection against a future pin rollback, not a workaround for a live bug. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- CHANGELOG.md | 2 +- docs/product-technical-gap-baseline.md | 8 ++- ...contextual_orchestrator_review_launcher.py | 68 ++++++++++--------- ...l_orchestrator_review_runtime_preflight.py | 37 +++++----- ...al_orchestrator_review_sidecar_contract.py | 15 ++-- 5 files changed, 70 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e01a01c37..f8166e7a8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ Semantic Versioning where the repository publishes a release. reach `orchestrator/auto`'s served catalog. - **Fix a stale `test_strix_quick_gate.sh` assertion left broken by the `#1630` scheduler-cadence lengthening.** `pr-review-merge-scheduler.yml`'s repository-local - heartbeat was changed from a quarter-hourly `cron: "*/30 * * * *"` to an hourly + heartbeat was changed from a half-hourly `cron: "*/30 * * * *"` to an hourly `cron: "30 * * * *"` (see `docs/doctoring/actions-queue-saturation-hourly-sweep.md`), and the Python regression `tests/test_actions_queue_saturation_scheduler_cadence.py` was updated to match at the time — but the parallel bash contract in diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 0aa00b8e9d..e1471412be 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2860,14 +2860,14 @@ scripts/ci/test_strix_quick_gate.sh` against the exact PR head) was failing on multiple, unrelated open PRs (observed directly on `.github#1476`, a PR whose own diff never touches this script or the scheduler workflow) with: -``` +```text FAIL: scheduler wakes frequently enough to clear auto-merge PRs that become stale after their initial PR events (missing 'cron: "*/30 * * * *"') ``` **Root cause.** `#1630` (referenced in `docs/doctoring/actions-queue-saturation-hourly-sweep.md`) deliberately lengthened `pr-review-merge-scheduler.yml`'s repository-local heartbeat -from a quarter-hourly `cron: "*/30 * * * *"` to an hourly `cron: "30 * * * *"` to +from a half-hourly `cron: "*/30 * * * *"` to an hourly `cron: "30 * * * *"` to reduce Actions-capacity pressure during the sustained organization-wide queue saturation this session repeatedly documented. The Python regression `tests/test_actions_queue_saturation_scheduler_cadence.py` was correctly updated at @@ -2906,7 +2906,9 @@ that evidence. **Risk of this fix itself.** Essentially none: a one-line literal-string update in a test assertion, verified to both fail before and pass after against the exact -same unmodified `main` checkout. No workflow, script, or other test file changed. +same unmodified `main` checkout. Only `scripts/ci/test_strix_quick_gate.sh` (a +bash test script) changed -- no workflow YAML and no Python production or test +code changed. **Expected effect.** `exact-head-path-policy` stops failing organization-wide PRs on this assertion once this fix reaches protected `main`; any PR whose branch has diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index cc237415d0..98ad8beac8 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -208,9 +208,11 @@ def _openrouter_reports_per_model_evidence(discovered: list[object]) -> bool: ``True`` once at least one discovered OpenRouter row reports ``evidence_only=False`` (real per-model evidence observed this run); ``False`` when there are no OpenRouter rows at all, or every - OpenRouter row is ``evidence_only=True`` (today's confirmed bug - signature, or an indistinguishable run where OpenRouter discovery - itself produced no rows). + OpenRouter row is ``evidence_only=True`` (the blanket bug + signature from a vendored pin predating + ``ContextualWisdomLab/contextual-orchestrator#949``, or an + indistinguishable run where OpenRouter discovery itself produced + no rows). """ return any( getattr(model, "provider_name", None) == "openrouter" @@ -231,34 +233,38 @@ def _routable_discovered_models(discovered: list[object] | None) -> list[object] OpenRouter rows are conditionally exempt from this exclusion -- see ``_openrouter_reports_per_model_evidence`` for the full rationale - and its documented limitation. In short: ``contextual-orchestrator``'s - OpenRouter ``ProviderModelSource`` currently hardcodes - ``evidence_only=True`` for every discovered model unconditionally (a - confirmed bug, fixed upstream at - ``ContextualWisdomLab/contextual-orchestrator#949``, merged at - ``8cd99f139915131ba0239bce12a5d6a5fd85394e`` -- pinned in this repo by - ``ContextualWisdomLab/.github#1477``, merged 2026-08-31, whose current - ``ORCHESTRATOR_PIN_SHA`` (``045d17da5e2aea56a97e241ee158ab1628d78660``) - is confirmed to descend from that fix commit) -- - not computed per model from real evidence, even though - genuine per-model ZDR evidence is fetched and parsed for OpenRouter in - that same module. Applying this filter to OpenRouter verbatim while - that bug is live would strip every OpenRouter row, including genuinely - servable, chat-capable ones, before ``zdr_policy.is_zdr_model()``'s - purpose-built, per-route OpenRouter ZDR-feed check (``openrouter_ - endpoints_feed``) ever gets a chance to evaluate them -- making that - already-correct, already-wired mechanism dead code for OpenRouter - specifically. So the exemption applies only while this run's own - OpenRouter rows still match that exact blanket-``True`` bug signature; - the moment any OpenRouter row in a run reports real per-model evidence - (``evidence_only=False``), OpenRouter rows go back through the same - ``evidence_only`` contract every other provider already gets -- - automatically, with no pin bump or manual edit needed here. A - genuinely non-servable OpenRouter row is still excluded downstream by - the same provider-agnostic chat-capability check every other provider's - rows already go through (``is_general_chat_agent_model_id`` + - ``_has_text_output``, in ``main()``) regardless of which branch this - function takes. + and its documented limitation. In short: a vendored pin predating + ``ContextualWisdomLab/contextual-orchestrator#949`` (merged at + ``8cd99f139915131ba0239bce12a5d6a5fd85394e``) has + ``contextual-orchestrator``'s OpenRouter ``ProviderModelSource`` + hardcode ``evidence_only=True`` for every discovered model + unconditionally -- not computed per model from real evidence, even + though genuine per-model ZDR evidence is fetched and parsed for + OpenRouter in that same module. This repo's current pin + (``ContextualWisdomLab/.github#1477``, merged 2026-08-31, + ``ORCHESTRATOR_PIN_SHA`` ``045d17da5e2aea56a97e241ee158ab1628d78660``) + is confirmed to descend from the ``#949`` fix commit, so the blanket + bug is not live in this run's actual pin -- the exemption below exists + as regression protection against a future pin that reverts to a + pre-``#949`` state, not a workaround for a currently-live bug. + Applying this filter to OpenRouter verbatim while that pre-``#949`` + shape is in effect would strip every OpenRouter row, including + genuinely servable, chat-capable ones, before + ``zdr_policy.is_zdr_model()``'s purpose-built, per-route OpenRouter + ZDR-feed check (``openrouter_endpoints_feed``) ever gets a chance to + evaluate them -- making that already-correct, already-wired mechanism + dead code for OpenRouter specifically. So the exemption applies only + while this run's own OpenRouter rows still match that exact + blanket-``True`` bug signature (i.e. only under a reverted, + pre-``#949`` pin); the moment any OpenRouter row in a run reports real + per-model evidence (``evidence_only=False``), OpenRouter rows go back + through the same ``evidence_only`` contract every other provider + already gets -- automatically, with no pin bump or manual edit needed + here. A genuinely non-servable OpenRouter row is still excluded + downstream by the same provider-agnostic chat-capability check every + other provider's rows already go through + (``is_general_chat_agent_model_id`` + ``_has_text_output``, in + ``main()``) regardless of which branch this function takes. This exemption has real, live effect as of this merge, on two timelines that were originally expected to be sequential but have both diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 33991b7d5a..a5eb2f4fdd 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -115,22 +115,23 @@ def test_routable_discovered_models_excludes_evidence_only_rows() -> None: def test_routable_discovered_models_exempts_openrouter_when_every_row_reports_evidence_only() -> None: """OpenRouter rows are exempt from evidence_only while every row still shows it. - Regression for a confirmed bug, fixed upstream at + Regression for a bug fixed upstream at ``ContextualWisdomLab/contextual-orchestrator#949`` and pinned in this repo by ``ContextualWisdomLab/.github#1477`` (merged 2026-08-31): ``contextual-orchestrator``'s - OpenRouter ``ProviderModelSource`` currently hardcodes - ``evidence_only=True`` for every discovered model unconditionally (not - computed per model from real evidence) -- so today's real signature is - that *every* discovered OpenRouter row carries ``evidence_only=True``, - with no exceptions, even genuinely servable ones. If this filter - applied to OpenRouter like every other provider while that bug is - live, it would strip every OpenRouter row, including genuinely - servable ones, before ``zdr_policy.is_zdr_model()``'s purpose-built - per-route OpenRouter ZDR-feed check ever runs on them. Both OpenRouter - rows here carry ``evidence_only=True`` (today's real bug shape) and - both must still pass through; a same-shaped row from a different - provider must not. + OpenRouter ``ProviderModelSource`` hardcoded + ``evidence_only=True`` for every discovered model unconditionally in + any pin predating ``#949`` (not computed per model from real + evidence) -- so that pin's real signature was that *every* discovered + OpenRouter row carried ``evidence_only=True``, with no exceptions, + even genuinely servable ones. If this filter applied to OpenRouter + like every other provider under such a pin, it would strip every + OpenRouter row, including genuinely servable ones, before + ``zdr_policy.is_zdr_model()``'s purpose-built per-route OpenRouter + ZDR-feed check ever runs on them. Both OpenRouter rows here carry + ``evidence_only=True`` (the pre-``#949`` bug shape) and both must + still pass through; a same-shaped row from a different provider must + not. """ namespace = _load_launcher() routable = namespace["_routable_discovered_models"] @@ -237,11 +238,11 @@ def test_routable_discovered_models_excludes_spend_blocked_rows() -> None: def test_routable_discovered_models_excludes_spend_blocked_openrouter_row_even_while_evidence_only_exempt() -> None: """The ``spend_admitted`` exclusion applies independently of the ``evidence_only`` exemption. - A spend-blocked OpenRouter row that also still carries today's blanket - ``evidence_only=True`` bug signature -- so the OpenRouter ``evidence_ - only`` exemption would otherwise let it through -- must still be - excluded: the two filters are independent conditions, and neither - exemption weakens the other. + A spend-blocked OpenRouter row that also still carries the pre-``#949`` + blanket ``evidence_only=True`` bug signature -- so the OpenRouter + ``evidence_only`` exemption would otherwise let it through -- must + still be excluded: the two filters are independent conditions, and + neither exemption weakens the other. """ namespace = _load_launcher() routable = namespace["_routable_discovered_models"] diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 00d86d9e8f..3a40ac5741 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -353,13 +353,14 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert not has_text_output(SimpleNamespace()) # Pin the exemption's real boolean outcome, not just source-text - # presence: an OpenRouter row carrying today's blanket evidence_only=True - # bug signature must still be routable, while a same-shaped row from any - # other provider must not -- so a reversed comparison (``!=`` instead of - # ``==``) or a disconnected/no-op exemption (e.g. the OpenRouter branch - # never actually reached, or applied unconditionally regardless of - # provider) fails this assertion even though the source fragment above - # would still be present verbatim. + # presence: an OpenRouter row carrying the pre-#949 blanket + # evidence_only=True bug signature must still be routable, while a + # same-shaped row from any other provider must not -- so a reversed + # comparison (``!=`` instead of ``==``) or a disconnected/no-op + # exemption (e.g. the OpenRouter branch never actually reached, or + # applied unconditionally regardless of provider) fails this + # assertion even though the source fragment above would still be + # present verbatim. routable_discovered_models = launcher["_routable_discovered_models"] openrouter_blanket_marked = SimpleNamespace( provider_name="openrouter", model_id="some/model", evidence_only=True