From 0007587ad85d126a3a4871c96204878c336448a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 08:42:14 +0000 Subject: [PATCH 1/2] fix(tests): match live-head-moved regression to #1697's intentional reorder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1697 (commit 5c561a65) 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 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- ...st_opencode_live_draft_state_regression.py | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index a9d9c518bc..dcbabc2428 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -186,15 +186,29 @@ def test_stale_draft_request_reuses_live_ready_approval(tmp_path: Path) -> None: @pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) -def test_draft_exemption_fails_closed_when_live_head_moved( +def test_draft_exemption_applies_even_when_live_head_has_moved( tmp_path: Path, script: str, ) -> None: - """The event cannot exempt a different live head even when it is still draft.""" + """A still-draft PR exempts before the head-match check ever runs. + + #1697 reordered the live-state checks so closed/draft admission is + evaluated before the head-SHA-match check (a draft PR whose live head + moved between the event snapshot and this step's own live re-fetch must + not fail closed with red-X noise -- see + ``ContextualWisdomLab/contextual-orchestrator`` PR #1000). The + head-moved branch is therefore unreachable while still draft: this + exercise now exempts via the draft check, not the head-match check. + Equivalent direct coverage of the production step lives in + ``test_opencode_required_verdict_regression.py``'s + ``test_request_review_step_exempts_a_draft_pr_whose_live_head_has_moved`` + and ``test_fail_closed_step_exempts_a_draft_pr_whose_live_head_has_moved``. + """ result = _run_step(tmp_path, script, live_draft=True, live_head="b" * 40) - assert result.returncode == 1 - assert "head moved while validating live" in result.stdout + assert result.returncode == 0, result.stderr + assert "still a draft on the live exact head" in result.stdout + assert "head moved" not in result.stdout @pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) From 1b957632793fdcdcade1cf6929e5ff448db3fbae Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 2 Sep 2026 18:03:57 +0900 Subject: [PATCH 2/2] 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 --- ...st_opencode_live_draft_state_regression.py | 24 +++- tests/test_opencode_poll_self_retirement.py | 136 +++++++++++++++++- ...st_opencode_required_verdict_regression.py | 21 ++- 3 files changed, 170 insertions(+), 11 deletions(-) diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index dcbabc2428..df45cb0d8b 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -34,6 +34,14 @@ def _write_live_state_gh( for exercising a missing/null/non-string/unexpected ``state`` field that the convenience ``live_draft``/``live_head``/``live_state`` parameters cannot express. + + Also stubs ``sleep`` to return instantly: ``fail_closed_script()``'s + transport-failure retry path really does ``sleep "$poll_interval_seconds"`` + (60s) between attempts, and this fixture's later-call sentinel exit code + drives that path to its 3-failure fail-closed threshold in + ``test_stale_draft_verdict_event_does_not_exempt_live_ready_pr`` -- without + this stub that test performs two genuine 60s sleeps (~120s real + wall-clock time per run) instead of running fast. """ payload = json.dumps( live_payload_override @@ -70,6 +78,9 @@ def evaluate_receipts(reviews, head_sha, *, is_draft): encoding="utf-8", ) fake_gh.chmod(fake_gh.stat().st_mode | 0o111) + fake_sleep = bin_dir / "sleep" + fake_sleep.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8") + fake_sleep.chmod(fake_sleep.stat().st_mode | 0o111) def _run_step( @@ -138,12 +149,13 @@ def test_stale_draft_verdict_event_does_not_exempt_live_ready_pr( Unlike ``request_review_script()``'s single unguarded live-PR fetch, this step's post-draft-check Reviews API poll retries a transport failure up - to ``max_poll_transport_failures`` times (with a real backoff sleep - between attempts) before failing closed with its own exit 1 and - diagnostic -- so the fixture's synthetic unmocked-call sentinel exit code - never reaches this script's own exit status, unlike the sibling test - above. The "stale" continuation message is still emitted first, proving - the step did not silently exempt the live-ready PR from verdict polling. + to ``max_poll_transport_failures`` times (with a stubbed, instant backoff + "sleep" between attempts -- see ``_write_live_state_gh``) before failing + closed with its own exit 1 and diagnostic -- so the fixture's synthetic + unmocked-call sentinel exit code never reaches this script's own exit + status, unlike the sibling test above. The "stale" continuation message + is still emitted first, proving the step did not silently exempt the + live-ready PR from verdict polling. """ result = _run_step(tmp_path, fail_closed_script(), live_draft=False) diff --git a/tests/test_opencode_poll_self_retirement.py b/tests/test_opencode_poll_self_retirement.py index cd12a567d5..17d5e937ae 100644 --- a/tests/test_opencode_poll_self_retirement.py +++ b/tests/test_opencode_poll_self_retirement.py @@ -35,8 +35,16 @@ def _run_poll_loop( reviews: list[dict[str, object]] | None = None, fail_live_pr_attempts: int = 0, fail_review_attempts: int = 0, + date_epochs: list[int] | None = None, ) -> tuple[subprocess.CompletedProcess[str], list[str]]: - """Execute the production poll body against a deterministic fake ``gh``.""" + """Execute the production poll body against a deterministic fake ``gh``. + + ``date_epochs``, when given, stubs ``date`` to return each listed epoch + in turn (clamped to the last entry once exhausted) instead of the real + clock -- letting a test fast-forward past the real + ``poll_deadline_epoch`` wall-clock deadline after a chosen number of + genuinely-executed loop iterations, without ever sleeping for real time. + """ call_log = tmp_path / "gh-calls.log" live_fail_counter = tmp_path / "live-pr-failures" review_fail_counter = tmp_path / "review-failures" @@ -84,6 +92,35 @@ def _run_poll_loop( ) fake_timeout.chmod(0o755) + env_overrides: dict[str, str] = {} + if date_epochs is not None: + date_epochs_file = tmp_path / "date-epochs" + date_epochs_file.write_text( + "\n".join(str(epoch) for epoch in date_epochs) + "\n", encoding="utf-8" + ) + date_counter = tmp_path / "date-calls" + fake_date = tmp_path / "date" + fake_date.write_text( + """#!/bin/sh +set -eu +count=0 +if [ -e "$FAKE_DATE_COUNTER" ]; then + count="$(cat "$FAKE_DATE_COUNTER")" +fi +count=$((count + 1)) +printf '%s\\n' "$count" > "$FAKE_DATE_COUNTER" +line="$(sed -n "${count}p" "$FAKE_DATE_EPOCHS")" +if [ -z "$line" ]; then + line="$(tail -n1 "$FAKE_DATE_EPOCHS")" +fi +printf '%s\\n' "$line" +""", + encoding="utf-8", + ) + fake_date.chmod(0o755) + env_overrides["FAKE_DATE_EPOCHS"] = str(date_epochs_file) + env_overrides["FAKE_DATE_COUNTER"] = str(date_counter) + script = "\n".join( ( "set -euo pipefail", @@ -92,6 +129,7 @@ def _run_poll_loop( 'review_poll_failures=0', 'max_poll_transport_failures=3', 'poll_interval_seconds=60', + 'poll_deadline_epoch=$(( $(date +%s) + 10800 ))', "while :; do", _poll_loop(), "done", @@ -111,6 +149,7 @@ def _run_poll_loop( "GH_REVIEW_FAIL_COUNTER": str(review_fail_counter), "GH_LIVE_PR": json.dumps(live_pr), "GH_REVIEWS": json.dumps(reviews or []), + **env_overrides, } ) result = subprocess.run( @@ -324,3 +363,98 @@ def test_self_retirement_does_not_replace_semantic_review_with_a_short_timeout() assert "while :; do" in target_job assert "poll_interval_seconds=60" in target_job assert 'sleep "$poll_interval_seconds"' in target_job + + +def test_poll_fails_closed_after_wall_clock_deadline_with_every_gh_call_succeeding( + tmp_path: Path, +) -> None: + """The zombie scenario: no transport failure ever occurs, yet no verdict posts. + + `max_poll_transport_failures` cannot catch this -- every `gh` call + below succeeds -- so only a genuinely distinct wall-clock deadline + (`poll_deadline_epoch`, computed once before the loop) can release the + runner. A fake `date` fast-forwards past the real production 10800s + (180-minute) bound only after two full, genuinely-executed fast + iterations (proving the check is a real per-iteration wall-clock + comparison, not a check that fires before any work happens), without + this test ever sleeping for real time. + """ + head_sha = "5" * 40 + result, calls = _run_poll_loop( + tmp_path, + head_sha=head_sha, + live_pr={"head": {"sha": head_sha}, "draft": False, "state": "open"}, + reviews=[], # opencode-agent never posts a review on this head + date_epochs=[1000, 1000, 1000, 999999999999], + ) + + assert result.returncode == 1 + assert ( + "::error::No current-head OpenCode verdict after 180 minutes of " + "polling; failing closed and releasing the runner." in result.stdout + ) + # Distinct diagnostic from the transport-failure path: nothing here failed. + assert "consecutive times" not in result.stdout + assert calls == [ + "api repos/ContextualWisdomLab/example/pulls/42", + "api --paginate repos/ContextualWisdomLab/example/pulls/42/reviews?per_page=100", + "api repos/ContextualWisdomLab/example/pulls/42", + "api --paginate repos/ContextualWisdomLab/example/pulls/42/reviews?per_page=100", + ] + + +def test_poll_wall_clock_deadline_does_not_interfere_with_a_fast_verdict( + tmp_path: Path, +) -> None: + """A verdict arriving on the first poll is unaffected by the new bound.""" + head_sha = "6" * 40 + result, calls = _run_poll_loop( + tmp_path, + head_sha=head_sha, + live_pr={"head": {"sha": head_sha}, "draft": False, "state": "open"}, + reviews=[ + { + "user": {"login": "opencode-agent[bot]"}, + "commit_id": head_sha, + "state": "APPROVED", + "body": "Source-backed current-head semantic review.", + } + ], + date_epochs=[1000, 1000], # baseline call, then one in-bounds iteration check + ) + + assert result.returncode == 0, result.stderr + assert calls == [ + "api repos/ContextualWisdomLab/example/pulls/42", + "api --paginate repos/ContextualWisdomLab/example/pulls/42/reviews?per_page=100", + ] + assert "No current-head OpenCode verdict after" not in result.stdout + + +def test_wall_clock_deadline_is_distinct_from_and_additional_to_transport_counter() -> None: + """The new bound sits alongside, not in place of, the transport-failure counter. + + Pins the production shape so a future edit cannot quietly collapse the + two into one, or drop the wall-clock bound back to unbounded: both + `max_poll_transport_failures` (existing) and `poll_deadline_epoch` + (computed once before the loop) must be present, and the wall-clock + check must live inside the `while :; do` loop body -- not as a + job-level `timeout-minutes:`, which would kill the runner mid-request + instead of failing closed with a clear diagnostic. + """ + target_job = WORKFLOW.read_text(encoding="utf-8").split( + " opencode-review-target:\n", 1 + )[1].split("\n cancel-superseded-opencode-review-runs:\n", 1)[0] + assert "max_poll_transport_failures=3" in target_job + assert "poll_deadline_epoch=$(( $(date -u +%s) + 10800 ))" in target_job + loop = _poll_loop() + assert 'if [ "$(date -u +%s)" -ge "$poll_deadline_epoch" ]; then' in loop + assert ( + "::error::No current-head OpenCode verdict after 180 minutes of " + "polling; failing closed and releasing the runner." in loop + ) + # The deadline check must precede this iteration's gh calls so an + # already-expired deadline never spends another API request. + assert loop.index('-ge "$poll_deadline_epoch"') < loop.index( + 'live_poll_pr="$(timeout 30s gh api' + ) diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index f4f353e6da..05993face8 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -280,7 +280,16 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non def _write_live_pr_then_refusing_gh(bin_dir: Path) -> None: - """Serve the authoritative live PR lookup, then reject downstream GitHub I/O.""" + """Serve the authoritative live PR lookup, then reject downstream GitHub I/O. + + Also stubs ``sleep`` to return instantly. The production poll loop's + transport-failure path really does ``sleep "$poll_interval_seconds"`` + (60s) between retries -- without this stub, a test that drives that path + to its 3-failure fail-closed threshold performs two genuine 60s sleeps + (observed directly: this exact gap made + ``test_fail_closed_step_still_polls_for_a_non_draft_pr`` take ~120s of + real wall-clock time per run instead of running fast). + """ fake_gh = bin_dir / "gh" fake_gh.write_text( "#!/usr/bin/env bash\n" @@ -294,6 +303,9 @@ def _write_live_pr_then_refusing_gh(bin_dir: Path) -> None: encoding="utf-8", ) fake_gh.chmod(fake_gh.stat().st_mode | 0o111) + fake_sleep = bin_dir / "sleep" + fake_sleep.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8") + fake_sleep.chmod(fake_sleep.stat().st_mode | 0o111) def _run_fail_closed_step( @@ -623,9 +635,10 @@ def test_fail_closed_step_still_polls_for_a_non_draft_pr(tmp_path: Path) -> None """A non-draft PR must still reach the Reviews API call (not exempted). Unlike the request-review step's single unguarded call, the Reviews API - fetch here retries a transport failure up to three times (with a real - backoff sleep between attempts) before failing closed with its own exit - 1, so the fixture's synthetic unmocked-call sentinel exit code (17) + fetch here retries a transport failure up to three times (with a + stubbed, instant backoff "sleep" between attempts -- see + ``_write_live_pr_then_refusing_gh``) before failing closed with its own + exit 1, so the fixture's synthetic unmocked-call sentinel exit code (17) never reaches this script's own exit status -- it is absorbed by the retry loop instead, which still logs the sentinel's stderr diagnostic on every attempt.