fix(ci): exempt draft PRs from the opencode-review current-head verdict gate - #1443
fix(ci): exempt draft PRs from the opencode-review current-head verdict gate#1443seonghobae wants to merge 16 commits into
Conversation
…ct gate
opencode-review.yml's required opencode-review-target check ("Fail closed
without a current-head OpenCode verdict") unconditionally demanded an
APPROVED/CHANGES_REQUESTED review from opencode-agent on the current head for
every opened/synchronize/reopened event, with no draft handling. Meanwhile
scripts/ci/pr_review_merge_scheduler.py deliberately never dispatches an
OpenCode review request for a draft PR:
if pr.get("isDraft"):
return Decision(number, "skip", "draft PR")
Net effect: every draft PR showed this required check as a hard exit-1
failure on every push, forever, until marked ready for review -- a permanent
false alarm, not a transient/pending state.
Add a github.event.pull_request.draft early-exit mirroring the existing
closed early-exit exactly in style and placement (right after it, before the
PR_NUMBER/HEAD_SHA check). The job still always runs and always reports a
status -- it just reports success instead of a misleading failure for a
state where a verdict was never going to be requested. Once the PR is marked
ready for review, ready_for_review and subsequent synchronize events carry
draft: false, so the real gate applies unchanged.
Adds shell-level regression coverage in
tests/test_opencode_required_verdict_regression.py that executes the
production step body directly (draft short-circuits before any Reviews API
call, closed still takes precedence over draft, and non-draft PRs still
genuinely require a verdict).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Warning Review limit reachedNext included review available in 58 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 (6)
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 |
A ready PR converted back to draft with no new commit never fired the required-workflow gate again (converted_to_draft wasn't in its trigger list), so a previously failed opencode-review check stayed failed forever even though the existing draft exemption would have passed it. Add converted_to_draft to opencode-review.yml's pull_request_target types, and update the matching contract/regression tests. Found by Devin's automated review on PR #1443.
|
Status on the failing Separately, pushed 644255a to fix the real bug Devin found on this PR (draft-reconversion trigger gap) — see the resolved review thread. That push will itself trigger a fresh required-workflow run and review dispatch for the new head. Generated by Claude Code |
Fresh operational contradiction: explicit draft review-only request is discardedProtected On Draft PR #1450 at unchanged exact head Fresh exact evidence:
Current #1443 source makes the required status green for a Draft, but leaves Please extend the TDD boundary so:
The addressed |
|
Confirmed — this is a distinct defect from the one this PR fixes. Filed and fixed in #1456: New opt-in Generated by Claude Code |
…w-draft-gate-fix # Conflicts: # CHANGELOG.md
|
Resolved a merge conflict against protected Only Every actual code/test/workflow file ( Local full test suite is running to confirm (this repo's suite includes several deliberate 30s+ timeout-testing sleeps in Generated by Claude Code |
opencode-review-target's earlier "Request current-head OpenCode review execution" step still ran unconditionally (only excluding closed events), performing its own OIDC token exchange, app-token exchange, and repository_dispatch call under set -euo pipefail. Any transient failure there could fail the job before the later step's draft exemption ever ran, keeping the required check red on draft PRs during an infrastructure outage unrelated to draft status. Gate this step on !github.event.pull_request.draft as well, and add a regression test asserting the exact if: condition on the raw workflow YAML. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Fresh consumer canary confirming this PR's causal boundary: This is exactly the draft-state contradiction #1443 owns: the required workflow dispatches/waits for a verdict while the scheduler does not review Draft PRs. No BandScope-local workflow/source workaround is appropriate. Acceptance canary after #1443 reaches protected |
|
Thanks — good independent confirmation this is exactly the bug this PR fixes, on a third repo now ( Status: this PR itself is fully validated (full suite, both the original draft-gating fix and the follow-up "Request current-head OpenCode review execution" dispatch-step fix) and waiting on a formal current-head OpenCode verdict — Devin's latest pass shows 0 new issues, no other check is red. Once that verdict lands and this merges to Generated by Claude Code |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Strix가 6시간 이상 동작해서 취약점 잡는 것도 본 일이 있습니다. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
Resolves a real conflict in tests/test_opencode_required_verdict_regression.py (both branches added independent, non-overlapping tests to the same file: this branch's draft-gate regression tests, main's #1532 poll-budget-ceiling guard) by keeping both. Also fixes two issues the merge exposed: - The merged-in poll loop bound increase (180->660 attempts via #1532) turned test_non_draft_pr_without_a_verdict_still_fails_closed's real `sleep 30` calls into a ~5.5-hour test; stub `sleep` as a no-op on PATH alongside the existing fake `gh`. - main is currently red on the review-dispatch blob pin and the security-boundary test (stale since #1533's already-merged head_sha warn-and-proceed change); re-pin both to match, same fix already applied in #1482, pending #1536. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Resolved the merge conflict against
The merge also exposed a real bug: Validated: full suite Generated by Claude Code |
|
뭔가 한 거 같긴 한데 OpenCode Review 안 되는군. |
…e draft-gate fix main's opencode-review.yml has been substantially redesigned since this branch last synced (#1507/#1532): the old 325-minute synchronous poll loop is gone, replaced by a fast check-once-dispatch-and-fail-closed "Resolve current-head formal OpenCode verdict" step plus a separate formal-receipt "wake" callback that reruns the failed job once a verdict actually lands, instead of blocking a runner for hours. #1533's head_sha warn-and-proceed design (which an earlier revision of this fix's blob-pin port matched) was also reverted upstream (#1540, real bugs found by Codex/Devin) -- restored to the original hard-fail assertions and current blob pin. The draft-gate exemption itself is unaffected by any of that and is re-applied cleanly against the new three-step structure: - "Resolve current-head formal OpenCode verdict" now exits early with verdict=DRAFT for a draft PR, mirroring its existing closed exit. - "Request current-head OpenCode review execution"'s own if: also skips drafts, so a transient OIDC/dispatch failure can't turn a draft PR's check red before the exemption runs. - The now-trivial "Fail closed without a current-head OpenCode verdict" step (no gh calls or loop left in it at all) treats VERDICT=DRAFT the same as VERDICT=CLOSED. - converted_to_draft added to the trigger types, so a ready PR converted back to draft with no new commit still gets a fresh run. tests/test_opencode_required_verdict_regression.py's old _run_step helper and its six tests assumed the removed monolithic polling step; replaced with _run_verdict_step/_run_fail_closed_step matching the new split, keeping the same draft/closed/ready-for-review coverage. Full suite: 2216 passed, 1 skipped, 21 subtests. Ruff, interrogate, YAML, and shell-syntax checks clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
업데이트: 제 이전 재실행 판단이 틀렸습니다 — 단순 큐 congestion이 아니라,
새 설계는 빠르게 실패/성공하도록 되어 있어서(더 이상 몇 시간씩 폴링하지 않음), 큐 슬롯만 받으면 몇 분 안에 결과가 나올 것으로 예상합니다. 정상적인 required check 통과를 먼저 지켜보고, 그래도 계속 막히면 말씀하신 chicken-and-egg bypass를 쓰겠습니다. Generated by Claude Code |
…closed state Devin review on #1443: a manual re-run of an old workflow run (e.g. a stale converted_to_draft run) replays that event's stored github.event.* fields verbatim, so a since-ready, unreviewed PR at the same head SHA could pass the required opencode-review check on a stale "still draft" reading. The verdict step now decides closed/draft from the pull request's live state via gh api instead of the triggering event's payload, and fails closed if that lookup itself fails.
Devin review on #1443: the "Request current-head OpenCode review execution" step's if: still combined github.event.action/ github.event.pull_request.draft with steps.verdict.outputs.verdict == ''. Once the verdict step resolves closed/draft from live PR state, those stale-payload conjuncts became a liability: a manual re-run of an old closed/draft-era job could still suppress the dispatch for a since- reopened/ready PR at the same head SHA, leaving the required check red with no review ever requested. Gate on the live verdict signal alone.
Devin review on #1443: even with the dispatch step correctly enabled from the live verdict, it still built its repository_dispatch payload's pr_base_ref/pr_base_sha/pr_head_ref/pr_head_sha from github.event.pull_request.* -- the same stale-payload source the prior two fixes removed from the pass/fail decision. On a manual re-run of an old job whose base branch has since advanced, opencode-review-dispatch.yml's live validate-pr-metadata check hard-rejects that stale base_sha, so the dispatch would fail even though the verdict step correctly decided one was needed. The verdict step now exposes base_ref/base_sha/head_ref/head_sha as step outputs from the same live gh api response it already uses for closed/draft, and the dispatch step builds its payload from those instead.
|
Current-main convergence: protected |
|
Traceability while converging this queue-reduction lane: add one concise append-only dated entry to |
|
Done. Merged current protected Generated by Claude Code |
Append-only dated entry documenting the three-round live-state hardening on .github#1443 (draft-gate exemption, then two Devin-flagged stale-event- payload gaps in the dispatch if: and dispatch payload), tied to #1531's required-workflow queue-pressure tracking.
|
Added the traceability entry to Generated by Claude Code |
|
@opencode-agent Please close the current exact-head live-state gap rather than merely weakening the new baseline prose. Devin is correct: formal-review matching still uses event-derived |
|
@opencode-agent review Fresh substantive review on exact head |
Owner direction, agreeing with and extending a Devin review comment on #1443: the formal-review-matching jq query still keyed off the triggering event's own HEAD_SHA, the one event-derived value the prior three rounds hadn't touched. A stale rerun's event payload could therefore match an approval that was only ever valid for a predecessor head, or miss a real approval already posted against the actual live head. The verdict step now matches reviews against the live head.sha from the same gh api fetch it already uses for closed/draft/dispatch metadata. Adds regression coverage for both directions, corrects the CHANGELOG/gap-baseline prose accordingly.
|
Done at @opencode-agent please review this exact head ( Generated by Claude Code |
|
Flagging a timing/ordering note rather than acting further: this review request (04:19:09) references exact head I'm not reverting Generated by Claude Code |
|
@opencode-agent review exact current head |
|
@opencode-agent review exact head |
|
Superseded by the current protected-main review architecture. On |
The bug
.github/workflows/opencode-review.yml's required check job (opencode-review-target, displayed as theopencode-reviewcheck) triggers onpull_request_target: types: [opened, synchronize, reopened, ready_for_review, closed]. Its one step, "Fail closed without a current-head OpenCode verdict", unconditionally demands an APPROVED/CHANGES_REQUESTED review fromopencode-agenton the current head, with no draft handling:Meanwhile
scripts/ci/pr_review_merge_scheduler.py— the thing that actually issues therepository_dispatchevent that would eventually causeopencode-agentto post a verdict — deliberately never requests a review for a draft PR (line 2401-2402):Net effect: every draft PR in the org showed this required check as a hard
exit 1failure on every single push, forever, until marked ready for review — not a transient/pending state, an active failure with a scary-looking error message every time. This was independently observed recurring identically on two different draft PRs across two different repos (ContextualWisdomLab/.github#1437andContextualWisdomLab/contextual-orchestrator#922).The fix
Add a draft-aware early-exit to the same step, mirroring the existing
closedearly-exit exactly in style and placement (right after it, before thePR_NUMBER/HEAD_SHAcheck):This preserves the org's own stated design principle (
9550c18: "a required-workflow check must never depend on event payload fields to materialize") — that principle governs therequired-workflow-bootstrapjob's materialization (confirmed viascripts/ci/test_strix_quick_gate.sh's job-scopedif:scan), which is unaffected here. Theopencode-review-targetjob itself still always runs unconditionally and always reports a status; only its internal bash logic decides pass/fail, exactly like the pre-existingclosedbranch.Why this is safe / doesn't weaken the real gate:
ready_for_reviewand subsequentsynchronizeevents carrydraft: false, so the check goes back to genuinely requiring a current-head verdict, exactly as today."No APPROVED or CHANGES_REQUESTED from opencode-agent"string) across every workflow in the repo: this is the only occurrence.opencode-review-dispatch.ymlhas an unrelated job also namedopencode-review-target(the privileged reviewer itself,repository_dispatch-triggered) with no equivalent unconditional-demand pattern.noema-review.ymlis a different shape entirely — it performs the review itself rather than demanding a pre-existing verdict — so it isn't affected by this bug.Tests
Added shell-level regression coverage in
tests/test_opencode_required_verdict_regression.pythat extracts the production step's literal bash body from the YAML (mirroring the existing_extract_run_blockpattern already used intests/test_opencode_workflow_shell_syntax.py), substitutes the two inline${{ github.* }}expressions the way GitHub Actions would, and executes it directly against fakeghbinaries:ghproves this), acrossopened/synchronize/reopenedclosedstill takes precedence overdraft(ordering regression guard)ready_for_reviewPR with a matching current-headAPPROVEDreview still passes through the real gate unchangedFull local check suite:
Note:
coverage report --show-missingshows 99% (one line inscripts/ci/pingora_edge_policy.py's changed-file pagination fallback, untouched by this PR) — confirmed byte-for-byte identical and pre-existing on a cleanmaincheckout before any of this PR's changes, viagit stash/re-run. Not introduced or worsened by this PR; flagging for visibility rather than silently masking it.Scope
This is a narrow, self-contained fix to one required-check step. No changes to
docs/pr-review-and-merge-procedure.mdorPR_GOVERNANCE_AUDIT.md— neither currently describes this check's draft behavior, so neither was rendered inaccurate by this change.Opening as draft per repo governance — OpenCode-approval + scheduler review/merge applies here same as any other PR.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Generated by Claude Code