fix(opencode): do not fail draft PRs when the head moves mid-check - #1697
Merged
seonghobae merged 1 commit intoSep 2, 2026
Merged
Conversation
opencode-review.yml's "Request current-head OpenCode review execution" and "Fail closed without a current-head OpenCode verdict" steps both checked the live head-SHA match before the closed/draft skip checks. A PR that stays a draft while being pushed to repeatedly (push, push, push before marking ready) hit the hard "::error::...head moved..." failure on every dispatch attempt where a newer commit landed between the pull_request_target event snapshot and this step's own live re-fetch -- even though the PR was, and remained, a draft the whole time, so no review was ever actually being requested. Reproduced verbatim on contextual-orchestrator PR #1000 (a draft PR): https://github.com/ContextualWisdomLab/contextual-orchestrator/actions/runs/33548447878/job/100066104033 Separately, even for an open, non-draft PR: a moved head means a newer push already fired its own fresh pull_request_target event and its own fresh run of this workflow, which validates that head correctly. Failing the old, now-superseded dispatch attempt only adds red-X noise for a benign race. Move the closed/draft checks (using the already-fetched live_state/live_draft values) before the head-SHA-match check in both steps, matching the closed-skip pattern already used lower in the same step (and the noema-review.yml closed-vs-stale fix applied earlier this session in #1674). Soften the head-mismatch case -- now only reached for a confirmed open, non-draft PR -- from ::error::+exit 1 to a graceful notice+exit 0; the check itself stays, so a stale-head dispatch still never proceeds for a SHA that is no longer current. Adds regression tests executing the actual step bash for the exact production scenario (open, draft, live head != event snapshot -> exit 0 with the draft-skip message, never the head-moved error) and for the softened open/non-draft/head-moved case, for both steps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 31 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 (2)
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}
Contributor
Author
|
No substantive failing test, security finding, CHANGES_REQUESTED review, unresolved thread, conflict, malformed provenance, or unrelated policy defect is being bypassed. |
seonghobae
deleted the
fix/opencode-review-draft-head-moved-false-positive
branch
September 2, 2026 08:00
This was referenced Sep 2, 2026
seonghobae
pushed a commit
that referenced
this pull request
Sep 2, 2026
While validating the merge with main on this PR, the full suite surfaced tests/test_opencode_live_draft_state_regression.py::test_draft_exemption_fails_closed_when_live_head_moved failing. Reproduced identically on a clean, unmodified checkout of origin/main (5c561a6) alone via a disposable worktree, confirming this is pre-existing on protected main and unrelated to this PR's own diff (scripts/ci/noema_review_gate.py's standalone-import fallback). Root cause: #1697 ("retire stale draft/head dispatches without false failure") deliberately inverted opencode-review.yml's check order -- the draft/closed exemption now runs before the head-moved check, and a still-draft PR whose live head has also moved now exits 0 quietly instead of failing closed with exit 1 -- fixing a real production false-failure (contextual-orchestrator run 33548447878/job 100066104033). #1697 updated its own new tests in test_opencode_required_verdict_regression.py to match, but this file's independent _run_step harness covering the identical production script text was never updated, so it kept asserting the superseded pre-#1697 contract. Same pattern as this repo's own previously-documented "stale test assertions left by a merged PR" class of fix (see CHANGELOG's #1654/#1656/ #1658 entry): no production behavior changed here, only the test's assertion and docstring, which now match the intentional, already-reviewed #1697 contract and cross-reference its sibling coverage. Verified: tests/test_opencode_live_draft_state_regression.py 19/19 passed; interrogate 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
3 tasks
seonghobae
added a commit
that referenced
this pull request
Sep 2, 2026
… (#1710) * fix(tests): match live-head-moved regression to #1697's intentional reorder #1697 (commit 5c561a6) reordered opencode-review.yml's live-state checks so closed/draft admission runs before the head-SHA-match check, and exits 0 instead of 1 for an open, ready PR whose live head has moved. A draft PR whose live head has moved is therefore exempted by the draft check first — the head-moved branch is now unreachable while still draft. test_opencode_live_draft_state_regression.py's test_draft_exemption_fails_closed_when_live_head_moved still asserted the pre-#1697 behavior (returncode 1, "head moved while validating live" in stdout) for exactly that input shape, so it fails on current main. Update it to assert the actual current behavior (returncode 0, exempted via the draft-check message), matching the equivalent direct-production-step coverage #1697 already added in test_opencode_required_verdict_regression.py. Confirmed via a clean origin/main worktree that the regression pre-dates this change and is not introduced by it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 * fix(tests): stub sleep in OpenCode poll regression tests, salvage #1706 Two existing tests extract the real "Fail closed without a current-head OpenCode verdict" step's bash and run it against a fake gh, but never stubbed `sleep` -- driving the transport-failure retry path to its 3-failure threshold performed two genuine 60s sleeps per affected test run (confirmed directly: this exact gap made a 2-test run exceed a 120s timeout). Both now stub `sleep` alongside the existing fake `gh`, matching the pattern already used in test_opencode_poll_self_retirement.py: tests/test_opencode_required_verdict_regression.py::test_fail_closed_step_still_polls_for_a_non_draft_pr tests/test_opencode_live_draft_state_regression.py::test_stale_draft_verdict_event_does_not_exempt_live_ready_pr Also fixes test_opencode_poll_self_retirement.py, which was silently broken on current main: #1707's wall-clock-deadline fix to opencode-review.yml added a `poll_deadline_epoch` reference at the top of the poll loop, but this file's `_run_poll_loop` harness never declared that variable before splicing in the now-changed real loop body, so 7 of its tests failed with an empty gh-calls.log (the script aborted under `set -u` before making any call). Adds the missing `poll_deadline_epoch` line and an injectable fake `date` (extending the existing fake-gh/fake-sleep/fake-timeout harness) to prove the wall-clock deadline logic itself: the loop fails closed with the new diagnostic once the deadline is exceeded even when every gh call keeps succeeding (the exact zombie scenario the fix targets), a fast verdict is unaffected, and the production shape keeps both bounds distinct and additive. No test sleeps for real time. Full affected suite (73 tests) verified green in ~16s; the full project suite (2582 passed, 1 skipped, 21 subtests) runs in ~116s with 100% coverage and 100% docstrings, matching #1706's own claimed 236.65s -> 112.76s improvement. This is a same-file-conflict-driven successor to #1706, which also included this exact test-file delta. #1706 additionally touched .github/workflows/opencode-review.yml with the wall-clock-deadline logic itself -- that exact fix already landed separately as #1707 (bypass-merged during the org-wide capacity incident, before #1706 finished), which is why #1706 is now DIRTY/CONFLICTING against main through no fault of its own test-file changes. This PR carries only the still-valid, non-redundant test-suite-hang fix forward; #1706 is being closed in favor of this PR. Branched from and includes #1705 (fix/live-draft-regression-test-1697, a different in-flight fix to the same tests/test_opencode_live_draft_state_regression.py file, addressing an unrelated draft-head-moved logic question) to avoid a second same-file conflict. If #1705 merges to main independently before this PR, this PR's identical carried-forward hunk should merge as a no-op; if this PR merges first, #1705 should rebase onto main afterward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
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
1 participant
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.
Root cause
.github/workflows/opencode-review.yml's "Request current-head OpenCode review execution" step (and the sibling "Fail closed without a current-head OpenCode verdict" step, which independently re-fetches and re-checks the same live PR state right after) both ran the live head-SHA-match check before the closed/draft skip checks:opennorclosed::error::+exit 1) if the live head SHA doesn't match the event-snapshotHEAD_SHA— "Pull request head moved while validating live review state."live_state == closed→ graceful skip (exit 0)live_draft == true→ graceful skip (exit 0)For a PR that stays a draft while being pushed to repeatedly (push, push, push before marking ready — a very common flow), every dispatch attempt where a newer commit lands between the
pull_request_targetevent firing and this validation step running hits the hard failure at step 3, even though the PR is — and remains — a draft the whole time. No review was ever actually being requested, so the failure is pure noise: a red X on a required check for a completely benign situation.Evidence: contextual-orchestrator PR #1000 (a draft PR, verified live
"draft": true) failed with exactly this error:https://github.com/ContextualWisdomLab/contextual-orchestrator/actions/runs/33548447878/job/100066104033#step:2:1
Separately, even for a PR that is open and non-draft: if its head has moved since the event snapshot, a newer push already fired its own fresh
pull_request_target/synchronizeevent and its own fresh run of this exact workflow, which validates that (now-current) head correctly. Failing the old, superseded dispatch attempt accomplishes nothing.Fix
live_state/live_draft) run before the head-SHA-match check — matching the closed-skip pattern already used lower in the same step, and the noema-review.yml closed-vs-stale fix applied earlier this session (fix(noema): do not fail closed-but-current-head as stale #1674).::error::+exit 1to a graceful notice +exit 0. The check itself is not removed: a stale-head dispatch still never proceeds for a SHA that's no longer current, it just stops quietly instead of erroring.Other occurrences checked
Verified the sibling required workflows (
strix.yml,noema-review.yml,pr-review-autofix.yml,opencode-review-dispatch.yml) for the same conflated ordering:noema-review.yml's "Validate current pull request head" step has an analogous closed-vs-stale conflation, already found and fixed this session via a separate, not-yet-merged PR (fix(noema): do not fail closed-but-current-head as stale #1674) — not duplicated here.pr-review-autofix.yml's head-mismatch checks already checklive_state != openfirst — no bug.strix.yml's cleanup job already treats a changed target as a gracefulnotice+ return; itsrepository_dispatchmetadata-validation gate andopencode-review-dispatch.yml's equivalent gate both requirestate == openunconditionally by design (they run only after a review has already been decided on and dispatched, not on a passive per-push race) — different shape, left untouched.Testing
tests/test_opencode_required_verdict_regression.pythat execute the actual step bash (extended the existing_run_request_review_step/_run_fail_closed_stepharness with an independent live-head parameter) proving:0with the draft-skip message, never the head-moved error, for both steps.0gracefully instead of erroring, for both steps.PYTHONPATH=. python3 -m pytest tests/test_opencode_agent_contract.py tests/test_opencode_required_verdict_regression.py tests/test_opencode_security_boundaries.py tests/test_required_workflow_queue_contract.py -v— 210 passed.actionlint .github/workflows/opencode-review.yml— clean.🤖 Generated with Claude Code