fix(ci): pin GitHub Actions review sidecar pool to orchestrator/free - #1663
Closed
seonghobae wants to merge 2 commits into
Closed
fix(ci): pin GitHub Actions review sidecar pool to orchestrator/free#1663seonghobae wants to merge 2 commits into
seonghobae wants to merge 2 commits into
Conversation
#1656/#1658 #1654 (introducing scripts/ci/current_head_run_coalescer.py and hardening several review-workflow polling loops with retry-with-backoff), #1656 (removing ten no-op cancel-closed-pr-runs runner jobs), and #1658 (removing the 300s LLM_TIMEOUT cap) each landed with their own new content correct, but left pre-existing tests referencing removed/changed behavior broken. Reproduced every failure on a fresh unmodified main clone before attributing blame. #1654's fallout (tests/test_current_head_run_coalescer.py, tests/test_opencode_live_draft_state_regression.py, tests/test_opencode_required_verdict_regression.py, tests/test_queue_cancellation_revalidation.py, tests/test_required_workflow_queue_contract.py): - A stale assertion expected "candidate is not a pull-request workflow run", but _run_matches_head_identity already rejects any non-PR-event candidate before that later, narrower check is ever reached -- the check was dead code. Removed it; updated the test to the correct, now-authoritative "head moved" message. - The "Fail closed without a current-head OpenCode verdict" step's Reviews API poll gained a 3-attempt retry-with-backoff around transport failures, absorbing the fixtures' synthetic unmocked-call sentinel exit codes (17, 19) into the loop's own exit 1 after real backoff sleeps; updated the return-code and message assertions to match. - Two literal-text contract drifts: "sleep 30" -> the current poll_interval_seconds variable; the reviews endpoint gained "?per_page=100". - Two renamed/relocated message assertions: a jq output field renamed current_head->classified_head; a diagnostic moved from the workflow YAML into the scripts/ci/revalidate_queue_cancellation.sh helper it now calls. While re-verifying current_head_run_coalescer.py's own coverage in isolation, found and closed two more gaps in the same file, neither touched by the above: - select_duplicate_queued_run_ids re-derived workflow_id behind a redundant `is not None` guard that _run_identity_matches (called immediately above it) already guarantees -- a second dead-code instance. Removed the guard. - _run_pr_scope_is_safe's six early-return-False guard clauses (no associations, malformed live PR, malformed association number, head/base mismatch, missing/mismatched closed-predecessor evidence) and validate_candidate_against_live_state's own sibling-scope rejection were genuinely reachable but had no test exercising them. Added eight targeted regression tests. #1656's fallout (tests/test_merge_scheduler_runner_image_contract.py, tests/test_required_security_runner_image_contract.py, tests/test_queue_cancellation_scheduler_contract.py): three runner-image occurrence-count/job-presence assertions still counted the ten removed no-op jobs. Updated counts and the job-name list to match the new job set. #1658's fallout (tests/test_strix_llm_timeout_contract.py): a stale literal assertion still expected "export LLM_TIMEOUT=300" in strix.yml; the workflow now exports 0 (unlimited) directly instead of relying on a separate normalization step to correct 300 after the fact. Full suite: 100% branch coverage, 100% docstrings. No production behavior change except the two dead-code removals, both provably unreachable and therefore behavior-neutral.
scripts/ci/contextual_orchestrator_review_sidecar.sh previously accepted CONTEXTUAL_ORCHESTRATOR_POOL=free or =auto and defaulted to free, but every current caller already resolves to free (strix.yml sets it explicitly; every other central review workflow relies on the script's own default) -- so this closes an unused, unaudited escape hatch rather than fixing a live incident. The org has not yet solved cost-safe free+ZDR routing well enough in central CI to justify admitting priced routes there; a future workflow edit or manual dispatch override setting CONTEXTUAL_ORCHESTRATOR_POOL=auto would have silently opened every required review (OpenCode, Noema, Strix) to priced model spend with no budget/authorization gate. The launcher's own --pool CLI flag (a general-purpose tool also invoked outside GitHub Actions, e.g. local testing) is unchanged and still accepts auto; only this repo's GitHub-Actions-facing sidecar script is narrowed. New test_sidecar_pins_the_pool_to_free_for_github_actions extracts and executes the sidecar's own case statement against free, auto, empty, and an arbitrary value, asserting auto now fails closed with a "CONTEXTUAL_ORCHESTRATOR_POOL must be free" diagnostic instead of being silently accepted. Full decision record (alternatives considered, verification, risk assessment) in docs/product-technical-gap-baseline.md's new 2026-09-02 entry.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
seonghobae
changed the base branch from
fix/stale-test-assertions-post-1654
to
main
September 2, 2026 01:55
seonghobae
pushed a commit
that referenced
this pull request
Sep 2, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Retargeted to
maindirectly (see comment below) — this PR now carries both commits: the stale-test-assertion fixes originally proposed in#1662(closed without explanation) plus the pool-pinning feature below. Both were independently validated together at 100% coverage before this PR existed.scripts/ci/contextual_orchestrator_review_sidecar.sh— the script every central required review workflow (Strix, OpenCode Review, Noema Review, the PR-review autofix sidecar) provisions to talk tocontextual-orchestrator— previously acceptedCONTEXTUAL_ORCHESTRATOR_POOL=freeor=autoand defaulted tofree.autois a real, load-bearing value one layer down: the launcher's--pool autoadmits priced discovered routes as a fallback once the free pool is exhausted.Why (pool pin)
Nothing in this repository's own review-provisioning path currently sets
CONTEXTUAL_ORCHESTRATOR_POOL=auto— the only workflow that sets the variable at all (strix.yml) sets it tofree; every other central review workflow relies on the script's own default. So this was not a live incident, it was an unaudited, structurally-reachable escape hatch: a future workflow edit or a manually-triggeredworkflow_dispatchwith a custom env override could set it toauto, and the sidecar would accept it silently — with no cost ceiling, no budget/authorization gate, and no reviewer visibility that priced models had entered scope for a required check.The org has not yet solved cost-safe free+ZDR routing reliably enough in central CI to justify admitting priced routes there (this repo's own
docs/product-technical-gap-baseline.mdrecords a realorchestrator/freeexhaustion incident and multiple admission-priority defects, all specifically about getting the free pool right first).Alternatives considered
autoaccepted but never set it. Rejected — "nobody currently sets it" is not a control, it's an absence of one.CONTEXTUAL_ORCHESTRATOR_POOLenv var entirely, hard-coding--pool free. Rejected in favor of keeping the fail-closedcasestatement (this repo's own established idiom, matching the siblingCONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDRvalidation two lines above it) — more auditable, and leaves one obvious place to extend validation later if the org ever re-authorizesautofor CI with a budget gate.--poolargparse choices. Rejected — the launcher is a general-purpose CLI also invoked outside GitHub Actions (local testing, other non-CI callers). Narrowing it there would remove functionality the tool intentionally provides.test_launcher_uses_orchestrator_discovery_and_governed_pools's existing pin ofchoices=("free", "auto")on the launcher is left unchanged.Full reasoning, verification steps, and risk assessment in
docs/product-technical-gap-baseline.md's 2026-09-02 entries.Why (stale-test fixes, absorbed from #1662)
#1654(introducingscripts/ci/current_head_run_coalescer.py),#1656(removing ten no-opcancel-closed-pr-runsrunner jobs), and#1658(removing the 300sLLM_TIMEOUTcap) each landed with their own new content correct, but left pre-existing tests broken. Fixes reproduced on a fresh unmodifiedmainclone before attributing blame, including two dead-code removals incurrent_head_run_coalescer.py(both provably unreachable given their guarding conditions) and eight new regression tests for previously-untested-but-reachable branches in the same file. Full details in#1662's original description (still visible on the closed PR) and this PR's commit messages.Developer experience
coverage report→ 100% onscripts/ci/.interrogate→ 100%.bash -nclean.User experience
No observable change to any current GitHub Actions review run — every current invocation already resolves to
free, and the stale-test fixes are test-only except for two provably-unreachable dead-code removals. This restores CI signal onmainand closes the pool-pinning escape hatch structurally.