fix: make Noema review independent - #1477
Conversation
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Warning Review limit reachedNext included review available in 6 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: Pro Plus Run ID: 📒 Files selected for processing (9)
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}
| set -euo pipefail | ||
|
|
||
| ORCHESTRATOR_PIN_SHA="${ORCHESTRATOR_PIN_SHA:-c107e3e52371993aa9c326fcc245e01c41fc3850}" | ||
| ORCHESTRATOR_PIN_SHA="${ORCHESTRATOR_PIN_SHA:-8cd99f139915131ba0239bce12a5d6a5fd85394e}" |
There was a problem hiding this comment.
🔍 External pin compatibility unverified
The runtime, ADR, and test pins agree. The vendored commit is absent locally, so its API and lock compatibility still depend on external integration evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified: the pinned commit is the merged contextual-orchestrator #949 SHA 8cd99f139915131ba0239bce12a5d6a5fd85394e. Its exact-head full suite (2830 passed, 1 skipped), Atheris, supply-chain, CodeQL, Semgrep, Trivy and OSV checks passed before merge; this PR's sidecar contracts and full central suite also pass.
Signed-off-by: Seongho Bae <me@seonghobae.me>
There was a problem hiding this comment.
🔍 Obsolete check data remains queried
PR_QUERY still fetches reviewDecision and the status-check rollup after their consumers were removed. Drop them to match the independent-review boundary and reduce GraphQL cost.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| github.event.pull_request.base.repo.full_name || | ||
| github.event.client_payload.target_repository || github.repository }}-${{ | ||
| github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || | ||
| github.event.client_payload.pr_number || | ||
| github.run_id }} |
There was a problem hiding this comment.
🔴 Follow-ups cancel required Noema checks
When OpenCode or Strix finishes during a pull-request Noema run, the shared concurrency.group cancels that required run. Its workflow-run replacement cannot restore the cancelled pull-request check.
Prompt for agents
Keep pull_request_target Noema runs in a concurrency group that workflow_run and repository_dispatch follow-ups cannot cancel, because only the pull-request-triggered run supplies the required PR check. Follow-up triggers can still be deduplicated separately, and the close-event run must retain a way to cancel the active pull-request run. Update the queue contract tests to assert both required-check isolation and close-event cancellation.
Was this helpful? React with 👍 or 👎 to provide feedback.
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Current-head follow-up
Noema remains independent of OpenCode approval and other check state. |
… cap no-op bug Rebases this branch onto main's provider_account/account_cap rename (#1468) and Noema-independence work (#1477/#1480), then fixes a real bug this branch's own batched-preflight merge introduced: _catalog_family_cap() defaulted to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES (24) whenever ORCHESTRATOR_CATALOG_FAMILY_CAP was unset, silently disabling per-account diversification. Live production evidence: probed_count 12, ready_count 2, rejected_count 10 (83% rejected via 429/404/timeout), with the admitted free-pool catalog 100% nvidia_nim/nvidia_nim_sub -- two credentials sharing one rate-limited upstream jointly occupying the entire 12-slot preflight batch. Reported at #1415 (comment) Fix (mirrors the pattern in open main PR #1487's diff, not yet merged): renamed the helper to _catalog_account_cap(default), which now requires the caller to supply contextual_orchestrator_review_policy.DEFAULT_ACCOUNT_CAP (4) as the default instead of hand-typing/defaulting to a total-routes-scale constant. An explicit ORCHESTRATOR_CATALOG_ACCOUNT_CAP override remains honored. The sidecar shell script's equivalent CATALOG_FAMILY_CAP default (also baked to the total 24-route budget) is renamed to CATALOG_ACCOUNT_CAP and restored to a real, smaller cap (8, main's existing operational value). Also corrects CHANGELOG.md and docs/adr/0003 prose that had described the no-op 24-route cap as an intentional fix, and updates two contract tests (test_contextual_orchestrator_review_runtime_preflight.py, test_contextual_orchestrator_review_sidecar_contract.py) that pinned the old family_cap naming and the buggy default. Verification: coverage run -m pytest tests -> 2122 passed, 1 skipped, 21 subtests; coverage report -> 100% on scripts/ci; interrogate -> 100% docstrings; bash -n on the touched shell script; git diff --check clean. Red-then-green: reverting _catalog_account_cap's default back to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES makes the new regression tests fail; restoring the fix makes them pass again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
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
1 participant
Summary
8cd99f139915131ba0239bce12a5d6a5fd85394eEvidence
uv run python -m pytest -q: 2094 passed, 1 skipped, 21 subtests passedReview semantics
Provider families are not introduced. Provider accounts remain independent credentials; model family semantics belong only to
model_group.No research PDF is added: this is a CI correctness and governance repair grounded in repository runtime evidence, not a substantive routing algorithm change.