fix(ci): stop blanket-stripping OpenRouter rows on evidence_only alone - #1476
fix(ci): stop blanket-stripping OpenRouter rows on evidence_only alone#1476seonghobae wants to merge 26 commits into
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Warning Review limit reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughOpenRouter 라우팅 필터와 preflight 테스트를 갱신했습니다. quality-CI 호출을 재사용 게이트로 통합했습니다. scheduler 계약을 시간당 주기에 맞게 수정했습니다. 실행 취소 전 신원 검증과 ChangesOpenRouter 라우팅
CI 워크플로우 계약
실행 취소와 조회 캐시
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change broadens eligible OpenRouter routing candidates while retaining independent spend and privacy admission checks; the remaining issues are localized documentation fixes, so no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant OpenRouterDiscovery
participant RoutableDiscoveredModels
participant AutoCatalog
participant ZDRGate
OpenRouterDiscovery->>RoutableDiscoveredModels: evidence 및 spend_admitted 전달
RoutableDiscoveredModels->>AutoCatalog: 라우팅 후보 전달
AutoCatalog->>ZDRGate: require_zdr 검증 요청
ZDRGate-->>AutoCatalog: 승인 또는 제외 결과 반환
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files. (3 skipped: 2 unsupported, 1 too large.) ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
# Conflicts: # CHANGELOG.md
…utcome 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
| 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 |
There was a problem hiding this comment.
📝 Info: Private routing remains independently gated
The exemption only widens discovery candidates. Private catalogs still require an exact OpenRouter feed match in build_zdr_prioritized_catalog.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return any( | ||
| getattr(model, "provider_name", None) == "openrouter" | ||
| and not getattr(model, "evidence_only", False) | ||
| for model in discovered | ||
| ) |
There was a problem hiding this comment.
📝 Info: Signature matches the active vendor pin
The current vendor pin gives OpenRouter rows evidence_only=False. Successful discovery disables the exemption, while spend_admitted=False still rejects credit-blocked paid routes.
Was this helpful? React with 👍 or 👎 to provide feedback.
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 #1506 for the full root-cause writeup and validation against origin/main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Hour-22: merged main without force-push. Current head @opencode-agent review |
|
Hour-23 exact-head review request. Current head @opencode-agent review |
|
Contextual-Orchestrator를 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. |
|
On the NIM/Contextual-Orchestrator/timeout note above: this PR's own diff doesn't touch timeouts or introduce any direct NIM call — it's entirely about Generated by Claude Code |
Ports the identical fix already applied to this session's #1476 and #1488 (commit 4323e27 pattern; root-caused in .github#1506, which targets main but has not merged there yet). 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, not pull_request_target), and the merge commit onto this branch carried forward the pre-existing bug from both sides (neither main nor this branch's own prior head had the fix), so it needs porting here directly -- confirmed live: run 33455130248/job 99693376191 failed with exactly this assertion. Root cause: assert_opencode_review_uses_codegraph_and_contextual_orchestrator extracted the required-workflow-bootstrap job block from opencode-review.yml with awk '/^ required-workflow-bootstrap:$/,/^[^ ]/'. Every job key in that workflow is indented 2 spaces (never column 0), so the end pattern never matched until EOF, sweeping an unrelated if: line from a later job into the "block" and failing the assertion on unrelated content. Fixed by using an explicit state flag so the end pattern (^ [A-Za-z0-9_-]+:) is only tested starting on the line after the start match, correctly bounding the block to just its own lines. Verified directly: the old awk swept in line 219's `if: github.event.action != 'closed'` from a later job; the new awk captures zero if: lines. Full local run: bash scripts/ci/test_strix_quick_gate.sh -> PASS, exit 0 (was FAIL/exit 1 before this commit). coverage run -m pytest tests -q -> 2126 passed, 1 skipped, 21 subtests. coverage report -> 100%. interrogate -> 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
…ure-evidence-only-filter
| and getattr(model, "provider_name", None) == "openrouter" | ||
| ) | ||
| ) | ||
| and getattr(model, "spend_admitted", True) is not False |
There was a problem hiding this comment.
📝 Info: Spend rejection covers every catalog path
Filtering spend_admitted=False before catalog construction blocks paid routes from both auto-pool selection stages. The True fallback preserves older dependency compatibility.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR resolves a critical discovery bug where OpenRouter models were blanket-stripped due to an upstream bug in contextual-orchestrator hardcoding evidence_only=True. The solution is highly robust, employing an observed-behavior signature check (_openrouter_reports_per_model_evidence) to create a self-correcting exemption that automatically disables itself once the upstream fix is pinned and observed. It also closes a latent gap in --pool auto by respecting the spend_admitted field. Security concerns regarding private routing were falsified through a dedicated regression test proving the ZDR admission gate remains the final authority.
Reviewed changed lines
scripts/ci/contextual_orchestrator_review_launcher.py:143 (RIGHT): Implements the observed-behavior signal. By checking if any OpenRouter row isevidence_only=False, it distinguishes between the buggy blanket-True state and the corrected per-model state without requiring fragile pin-SHA ancestry tracking.scripts/ci/contextual_orchestrator_review_launcher.py:245 (RIGHT): Core filtering logic. Correctly implements the conditional exemption for OpenRouter and the unconditional exclusion forspend_admitted=Falseusing a safegetattrdefault for backward compatibility with older pins.tests/test_contextual_orchestrator_review_runtime_preflight.py:385 (RIGHT): Crucial security regression test. Proves that even when theevidence_onlyexemption is active, thebuild_zdr_prioritized_catalogfunction still correctly filters out non-ZDR OpenRouter routes whenrequire_zdr=True.
Adversarial validation
scripts/ci/contextual_orchestrator_review_launcher.py:245 (RIGHT)falsified: The OpenRouter exemption might allow non-chat metadata stubs to be routed to private targets. — Verified bytest_require_zdr_still_excludes_non_zdr_openrouter_route_despite_evidence_only_exemptionwhich confirms the ZDR gate inzdr_policy.pyis the final authority.scripts/ci/contextual_orchestrator_review_launcher.py:245 (RIGHT)falsified: Thespend_admittedfilter might crash on older vendored pins that lack the attribute. — The use ofgetattr(model, "spend_admitted", True)ensures a default ofTrue, preventingAttributeError.- Residual risk: Low. A total ZDR-feed-fetch failure might mimic the blanket-bug signature, but this only widens candidates for the general pool and remains gated by downstream chat-capability checks.
Findings
-
No blocking findings.
-
Result: APPROVE
-
Head SHA:
54fd95d231572e622b0ed35979a55705607e89ac -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
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.
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.
|
@opencode-agent make this the current-main parent for review-launcher routing fixes. Protected main is now |
Raise scoped docstring coverage for the newly added scheduler REST regression helpers to 100% without changing test behavior or production code.
…ure-evidence-only-filter # Conflicts: # CHANGELOG.md
…7) into fix/openrouter-premature-evidence-only-filter Interim stack: #1567 is not yet merged to main and closes the pre-existing post-#1546 scheduler coverage regression this PR would otherwise inherit via the merged-tree coverage-evidence gate. This merge is a no-op once #1567 lands on main and a future main-merge picks it up normally.
|
Merged current protected main ( Since Validated on the new head ( @opencode-agent @cwl-noema-review please review the new exact head Generated by Claude Code Generated by Claude Code |
# Conflicts: # CHANGELOG.md # docs/product-technical-gap-baseline.md
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.
# Conflicts: # CHANGELOG.md # docs/product-technical-gap-baseline.md
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.
# Conflicts: # CHANGELOG.md
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review (this PR, scripts/ci/contextual_orchestrator_review_launcher.py lines 129-169) flagged that the docstrings describe #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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
seonghobae
added
bug
ChatGPT Codex Connector
…ure-evidence-only-filter # Conflicts: # CHANGELOG.md
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Merged current Before merging, confirmed in isolation on a clean New head
No predecessor evidence should transfer to this new head. Also corrected the PR description's stale " @opencode-agent @cwl-noema-review please review the new exact head Generated by Claude Code Generated by Claude Code |
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 (9e0c022, fix(test): eliminate scheduler-wake SIGPIPE
flake)". A recent merge from origin/main (5952e5b, 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
There was a problem hiding this comment.
Noema LLM review
The PR fixes a discovery bug where OpenRouter models were blanket-marked as evidence_only=True by a vendored library were being stripped from the serving catalog. It introduces a self-correcting exemption mechanism that disables itself once the vendored library begins reporting per-model evidence. It also closes a latent gap in the --pool auto path by respecting the spend_admitted flag to block credit-exhausted priced models. The changes are backed by comprehensive regression tests, including a specific check ensuring that the ZDR security gate for private targets remains independent and effective despite the exemption.
Reviewed changed lines
scripts/ci/contextual_orchestrator_review_launcher.py:123 (RIGHT): Implements_openrouter_reports_per_model_evidenceto detect the transition from blanket-True to per-model evidence reporting.scripts/ci/contextual_orchestrator_review_launcher.py:225 (RIGHT): Implements the conditional OpenRouter exemption and the unconditionalspend_admittedfilter.tests/test_contextual_orchestrator_review_runtime_preflight.py:145 (RIGHT): Verifies that the exemption is correctly disabled when real per-model evidence is observed.tests/test_contextual_orchestrator_review_runtime_preflight.py:265 (RIGHT): Confirms that the ZDR admission gate for private targets is not bypassed by theevidence_onlyexemption.
Adversarial validation
scripts/ci/contextual_orchestrator_review_launcher.py:225 (RIGHT)falsified: The OpenRouter exemption might bypass the ZDR security gate for private targets. — test_require_zdr_still_excludes_non_zdr_openrouter_route_despite_evidence_only_exemption proves that build_zdr_prioritized_catalog filters out non-ZDR routes regardless of the launcher's candidate list.scripts/ci/contextual_orchestrator_review_launcher.py:225 (RIGHT)falsified: The exemption remains active forever, defeating the upstream fix once pinned. — test_routable_discovered_models_stops_exempting_openrouter_once_a_row_shows_real_evidence confirms the exemption is disabled when per-model evidence is detected.- Residual risk: Low. A fail-open scenario exists if a fixed vendored copy reports 100%
evidence_only=Truein a specific run, but this is mitigated by downstream chat-capability checks and the independent ZDR gate.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
1d2d103f604f8b120199debe38125898611651fe - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
…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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
The required Not this PR's diff — Fix ported, not re-derived: opened Generated by Claude Code Generated by Claude Code |
| 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" |
There was a problem hiding this comment.
📝 Info: Cron assertion targets local heartbeat
The updated literal matches the repository-local hourly scan at minute 30. The separate organization sweep remains hourly at minute zero.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
…ure-evidence-only-filter # Conflicts: # CHANGELOG.md # docs/product-technical-gap-baseline.md
| and getattr(model, "provider_name", None) == "openrouter" | ||
| ) | ||
| ) | ||
| and getattr(model, "spend_admitted", True) is not False |
There was a problem hiding this comment.
📝 Info: Spend filtering preserves old pins
Only explicit False blocks a row. Older pins lack this field, while current producers define it as a boolean with a True default.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
scripts/ci/contextual_orchestrator_review_launcher.py's_routable_discovered_models()unconditionally dropped every discovery row withevidence_only=True.contextual-orchestrator's OpenRouterProviderModelSourcehardcodesevidence_only=Truefor 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 was fixed inContextualWisdomLab/contextual-orchestrator#949("fix(discovery): route OpenRouter by model evidence"), merged at8cd99f139915131ba0239bce12a5d6a5fd85394e— not touched here. (Correction: this PR originally citedcontextual-orchestrator#950as the pending upstream fix;#950was closed as redundant/superseded, and#949is the PR that actually merged.ContextualWisdomLab/.github#1477bumped this repo'sORCHESTRATOR_PIN_SHAto that exact commit and merged on 2026-08-31 — confirmed viagit merge-base --is-ancestor 8cd99f139915131ba0239bce12a5d6a5fd85394e 045d17da5e2aea56a97e241ee158ab1628d78660against contextual-orchestrator, i.e. the current pin already descends from #949's fix commit.)Fix
OpenRouter rows are now exempt from the
evidence_onlyexclusion in_routable_discovered_models(). 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, inmain()) — this exemption relies on that existing, independent check, not on trustingevidence_only's current, wrong, blanket value for OpenRouter._routable_discovered_models()also now excludes aspend_admitted=Falserow the same way it excludesevidence_only=True.#949's actual merged diff added a new field toDiscoveredModel,spend_admitted: bool = True, setFalsefor a priced OpenRouter row wheneveropenrouter_paid_inference_available()cannot confirm usable credit (a free OpenRouter row is alwaysspend_admitted=True). Investigated whether this repo's pipeline needs to respect it:orchestrator/free(the pool every current workflow actually configures) never considers priced rows at all, so it was never exposed to this — butorchestrator/autois real, reachable code (CONTEXTUAL_ORCHESTRATOR_POOL=auto, no other change needed) whose candidate rows do include priced ones, and nothing in this repo's pipeline checkedspend_admittedanywhere. Fixed with the samegetattr(model, "spend_admitted", True) is not Falsetreatmentevidence_onlyalready gets, so it degrades safely against a vendored pin that predates#949too. Full reasoning and theorchestrator/free-is-safe /orchestrator/auto-was-not analysis are in the 2026-08-31 correction entry ofdocs/product-technical-gap-baseline.md.Sequencing note (verified, not assumed)
I traced through the actual code before writing this rather than assuming: this fix has real, immediate effect once merged, not only once
contextual-orchestrator#949and a matchingORCHESTRATOR_PIN_SHAbump also land. OpenRouter discovery already runs in this sidecar today (OPENROUTER_API_KEYis one of the five KV-registered credentials), and for the general (non-private,require_zdr=False) pool — 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 genuinely chat-capable OpenRouter rows, currently blocked here regardless of whatcontextual-orchestratorreports, start reaching selection as soon as this merges.Update (2026-09-02, post-merge into main): both
#949and#1477have since landed (see the "Related" section below), so the gating described above no longer applies — the exemption's own self-correcting logic (see the updated docstring on_routable_discovered_models/_openrouter_reports_per_model_evidence) now takes over automatically once a run observes real per-modelevidence_onlyvariation from the pinned commit, rather than relying solely on the downstream chat-capability check.Developer experience
test_routable_discovered_models_excludes_evidence_only_rows(existing) corrected to use a non-OpenRouter provider for itsevidence_only=Truefixture, since that scenario no longer applies to OpenRouter.test_routable_discovered_models_exempts_openrouter_from_evidence_onlyregression: confirms both anevidence_only-tagged and an untagged OpenRouter row pass through while a same-shaped row from a different provider does not.test_contextual_orchestrator_review_sidecar_contract.py), matching this repo's existing pattern of pinning exact prose/structure in trusted scripts.spend_admittedregression tests:test_routable_discovered_models_excludes_spend_blocked_rows,test_routable_discovered_models_excludes_spend_blocked_openrouter_row_even_while_evidence_only_exempt, and an end-to-endtest_pool_auto_never_admits_a_spend_blocked_priced_openrouter_rowcomposing_routable_discovered_models→_report_rows→parse_discovery_report→build_zdr_prioritized_catalog(pool="auto").r3891875749) raised "Private code can reach forbidden routes" against theevidence_onlyexemption. Investigated end to end and confirmed a false alarm:build_zdr_prioritized_catalog()independently re-appliesis_zdr_model()'s real OpenRouter ZDR-feed check as its own admission gate wheneverrequire_zdr=True, regardless ofevidence_only. New regressiontest_require_zdr_still_excludes_non_zdr_openrouter_route_despite_evidence_only_exemptionproves a non-ZDR-attested OpenRouter row is excluded from arequire_zdr=Truecatalog even while every discovered OpenRouter row still showsevidence_only=True. Replied and resolved on the GitHub thread with this reasoning.docs/product-technical-gap-baseline.md's 2026-08-31 entries (including a same-dated correction subsection covering all of the above).User experience
Once merged, genuinely chat-capable, free/ZDR-attested OpenRouter models start becoming eligible catalog candidates for Noema/OpenCode/Strix review, correctly tagged by
is_zdr_model()'s live feed check — a real, previously-nonexistent contribution to pool diversity, not just an evidence/reporting nicety. A credit-exhausted paid OpenRouter row can no longer reachorchestrator/auto's served catalog, and a private/--require-zdrreview target's ZDR boundary is unchanged and independently re-verified.Test plan
coverage run -m pytest tests -q(full suite) — 2098 passed, 1 skipped, 21 subtests passedcoverage report --show-missing— 100% onscripts/ci/interrogate— 100%Related
ContextualWisdomLab/contextual-orchestrator#949— the upstream half of this bug ("fix(discovery): route OpenRouter by model evidence"), merged at8cd99f139915131ba0239bce12a5d6a5fd85394e. Not touched here. (contextual-orchestrator#950, originally cited here, was closed as redundant/superseded.)ContextualWisdomLab/.github#1477— bumped this repo'sORCHESTRATOR_PIN_SHAto#949's merge commit. Merged 2026-08-31.scripts/ci/zdr_policy.py'sis_zdr_model()/ProviderZdrScope.openrouter_endpoints_feed— the already-correct mechanism this fix lets actually run for OpenRouter.Generated by Claude Code
Summary by CodeRabbit
개선 사항
문서 및 품질