diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b661772cb..552966c233 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,31 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- **Fix stale test assertions and dead-code gaps left by `#1654`, `#1656`, and `#1658`.** + Reproduced all failures on a fresh unmodified `main` clone before attributing blame. + `#1654` (introducing `scripts/ci/current_head_run_coalescer.py` and hardening several + review-workflow polling loops with retry-with-backoff) left 7 stale assertions: one + genuinely dead-code check (`_run_matches_head_identity` already rejects any non-PR-event + candidate before a later, narrower "not a pull-request" check could ever run -- removed + the redundant check and updated the test to the correct, now-authoritative "head moved" + message), two synthetic-sentinel-vs-real-retry-loop mismatches (a fixture's unmocked-call + exit code no longer reaches the script's own exit status once a 3-attempt backoff loop + absorbs it), two literal-text contract drifts ("sleep 30" -> `poll_interval_seconds`; the + reviews endpoint gained `?per_page=100`), and two renamed/relocated message assertions (a + jq field rename `current_head`->`classified_head`; a diagnostic moved from the workflow + YAML into the `scripts/ci/revalidate_queue_cancellation.sh` helper it now delegates to). + While re-verifying `current_head_run_coalescer.py`'s own coverage in isolation, found and + closed two more, unrelated gaps in the same file: a second dead-code instance + (`select_duplicate_queued_run_ids` re-derived `workflow_id` behind a redundant guard + `_run_identity_matches` already guarantees) and six genuinely-reachable but untested + early-return guard clauses in `_run_pr_scope_is_safe` plus one in the sibling-authority + loop, closed with eight new targeted regression tests. `#1656` (removing ten no-op + `cancel-closed-pr-runs` runner jobs) and `#1658` (removing the 300s `LLM_TIMEOUT` cap, in + service of the org's now-unlimited-by-default LLM timeout policy) each left their own + runner-image-count and literal-value contract tests asserting pre-change reality; updated + four more test files to match. Full suite: 2600+ passed, 100% branch coverage, 100% + docstrings; no production behavior change except the two dead-code removals (both + provably unreachable, so behavior-neutral). - **Pin the three central required review workflows (Strix, OpenCode Review, Noema Review) off the observed starved floating `ubuntu-latest` runner image.** Following the same repair already rolled out to security gates (`#1618`) and the merge scheduler (`#1609`), `strix.yml`, `opencode-review.yml`, and `noema-review.yml` now request the explicit `ubuntu-24.04` image on every job. These three workflows are the org's own required-workflow gate for every sibling repository, so a starved floating image here directly contributes to organization-wide required-check queuing. New `tests/test_required_review_runner_image_contract.py` asserts no job in any of the three files still requests the floating image. Also fixed 4 pre-existing, unrelated test failures on `main` left by `#1630`'s organization-sweep rotation cadence change (every 15 minutes to hourly, to reduce control-plane pressure under the same Actions saturation): `tests/test_required_workflow_queue_contract.py`'s rotation-index tests still asserted the old `/ 900` (15-minute) divisor against the new `/ 3600` (hourly) production value. - **Refresh Noema reviewer App authority after long model work (`#1616`).** A real `naruon#1497` review outlived its repository-scoped GitHub App installation token and failed the next exact-head GitHub operation with HTTP 401. The trusted workflow now prepares the validated verdict into a private runner-local envelope, remints the same least-privilege repository-scoped App authority after model work, independently re-fetches exact live head/reviewer identity, and only then publishes. Skipped preparation creates no envelope, predecessor App tokens cannot authorize publication, PAT/OIDC remain explicit fail-closed sources, malformed handoffs are cleaned up, and executable plus step-scoped regressions cover stale-head, identity, alias, workflow wiring, and migration of legacy broader-suite contracts away from the retired single-process reviewer path. - Fix `existing_noema_review()` treating a "legacy" Noema review (one posted before diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 2a8f4c7b54..29acdfeecc 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2590,3 +2590,26 @@ Higgins, S. S., Crepalde, N., & Fernandes, L. (2021). Segmented multiplexity: A **Validation.** Full suite `2407 passed, 1 skipped, 21 subtests`; `coverage` 100% on `scripts/ci`; `interrogate` 100%; all four touched/added workflow files re-parse as valid YAML; `test_opencode_workflow_shell_syntax.py` and related shell-syntax tests pass unchanged. **Residual.** This closes the specific floating-image contribution from these three central workflows; it does not by itself guarantee the organization-wide Actions queue is fully drained, since other repositories' own workflows and any remaining unpinned central workflows may still request the floating image. Worth a follow-up sweep across the rest of `.github/workflows/` and sibling-repo workflows if queuing persists after this lands. + +## 2026-09-02 GitHub Actions review sidecar pool pinned to `orchestrator/free`; `auto` removed as an accepted value + +**Problem.** `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 to `contextual-orchestrator` — read an operator-settable `CONTEXTUAL_ORCHESTRATOR_POOL` environment variable, defaulted it to `free`, and validated it against exactly two accepted values: `free` or `auto` (`case "$orchestrator_pool" in free|auto) ...`). `auto` is a real, load-bearing value one layer down: `scripts/ci/contextual_orchestrator_review_launcher.py --pool auto` admits *priced* discovered routes as a fallback stage once the free pool is exhausted (`build_zdr_prioritized_catalog(..., pool="auto")`), by design, for callers that want that behavior. Nothing in this repository's own review-provisioning code path currently sets `CONTEXTUAL_ORCHESTRATOR_POOL=auto` — the only workflow that sets the variable at all, `strix.yml`, sets it to `free`; every other central review workflow simply relies on the script's own `:-free` default — so this was not a live incident, it was an unaudited, structurally-reachable escape hatch: a future edit to any of the four workflows above, or a manually-triggered `workflow_dispatch` with a custom env override, could set `CONTEXTUAL_ORCHESTRATOR_POOL=auto` and the sidecar would accept it silently, with no cost ceiling, no budget/authorization gate, and no reviewer visibility that priced models were now in scope for a required check. + +**Why this matters now, not hypothetically.** The org's explicit standing operating directive (the perpetual PR review→fix→merge→develop loop this session runs under) states plainly that the free+ZDR routing combination is not yet solved reliably in central CI — this exact gap-baseline document's own accumulated 2026-08-30/08-31 entries above record a real `orchestrator/free` exhaustion incident, a crowding-out bug between shared-endpoint credentials, and multiple rounds of Devin-Review-caught admission-priority defects in `contextual_orchestrator_review_policy.py`, all specifically about getting the *free* pool right. Admitting a priced-inclusive `auto` pool into required review workflows before that work is solid would let one misconfiguration or one well-intentioned "let's widen coverage" workflow edit start spending real provider credit on every PR's required Strix/OpenCode/Noema review, with no operator-visible signal that this had happened — the sidecar's own `log` lines print the resolved pool, but nothing downstream alerts on it, and there is no spend cap in this repository's own review-provisioning path (unlike `contextual-orchestrator`'s own cost-ledger, which this vendored sidecar path does not call into for CI review spend). + +**Alternatives considered.** +1. *Leave `auto` accepted but never set it.* Rejected: this is the status quo, and the status quo is exactly the unaudited escape hatch described above — "nobody currently sets it" is not a control, it is an absence of one. +2. *Remove the `CONTEXTUAL_ORCHESTRATOR_POOL` environment variable entirely, hard-coding `--pool free` with no override mechanism.* Considered and rejected in favor of the fail-closed `case` statement kept below: removing the variable removes the ability to reason about *why* an override was rejected (a caller setting `auto` would instead see an unrelated "unrecognized flag" or `--pool` argparse error further downstream, or silently fall through to whatever the launcher's own default resolves to, depending on how the removal was implemented) and removes a natural place to extend validation later (e.g. if the org ever explicitly re-authorizes `auto` for CI with a budget gate, only this one `case` arm needs to change). A `case` statement that explicitly names and rejects `auto` with a clear diagnostic is this repository's own established idiom (see the sibling `CONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDR` validation two lines above it in the same file) and is more auditable, not less. +3. *Narrow the launcher's own `--pool` argparse choices to just `("free",)`.* Rejected: the launcher (`contextual_orchestrator_review_launcher.py`) is a general-purpose CLI, not GitHub-Actions-specific — it is invoked directly (outside any workflow) for local testing and by other, non-CI-review callers that may have a legitimate reason to exercise the `auto` pool's priced-fallback behavior. Narrowing it there would remove functionality the tool's own design intentionally provides, contradicting the directive's explicit scoping ("GitHub Actions Workflow 이용에 관해" — regarding GitHub Actions Workflow *usage* specifically, not the tool in general). `test_launcher_uses_orchestrator_discovery_and_governed_pools`'s existing pin of `choices=("free", "auto")` on the launcher was therefore left unchanged. + +**Fix.** `scripts/ci/contextual_orchestrator_review_sidecar.sh`'s `case "$orchestrator_pool" in` now accepts only `free`; every other value (`auto` included, and any typo/unexpected value) falls to the `*)` arm and calls `fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free"`, matching this script's own existing fail-closed idiom for `CONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDR`. The variable's default (`${CONTEXTUAL_ORCHESTRATOR_POOL:-free}`) is unchanged, so every existing caller (all of which already resolve to `free`, explicitly or by default) is unaffected — this is a pure narrowing of previously-unused surface, not a behavior change for any current workflow run. + +**Developer experience.** New `test_sidecar_pins_the_pool_to_free_for_github_actions` in `tests/test_contextual_orchestrator_review_sidecar_contract.py` extracts the sidecar's own `case "$orchestrator_pool" in ... esac` block as text and *executes* it (not just string-matches it) in a minimal bash harness against four inputs — `free` (must succeed, `pool_args=--pool free`), `auto` (must fail closed with the new diagnostic), empty string (must resolve to the `:-free` default and succeed, since bash's `:-` operator treats empty and unset identically), and an arbitrary bogus value (must fail closed) — so a future edit that silently re-widens the accepted set back to include `auto` (or any other value) breaks this test rather than passing unnoticed. Static assertions confirm the exact new source text (`case "$orchestrator_pool" in\n free)` and the new fail message) and the absence of the old text (`free|auto`, `must be free or auto`). + +**Verified before touching anything.** Grepped every `.github/workflows/*.yml` for `CONTEXTUAL_ORCHESTRATOR_POOL` and any `--pool auto`/`pool.*auto` pattern: only `strix.yml` sets the variable, and it sets `free`. Grepped `scripts/ci/contextual_orchestrator_review_launcher.py`'s own `--pool` argparse and its one internal `pool="auto"` use (the priced-fallback stage, gated on `args.pool == "auto"` already being true from the CLI flag) to confirm that stage is reachable only when a caller explicitly requests `--pool auto` on the launcher directly — never as a side effect of the sidecar's own resolved value once this fix lands, since the sidecar can no longer produce `--pool auto`. + +**Risk of this fix itself.** Low and one-directional: this can only ever cause a caller that was setting `CONTEXTUAL_ORCHESTRATOR_POOL=auto` to start failing closed with a clear diagnostic instead of silently proceeding with priced routes; grep confirms no current caller does this, so no existing workflow run's behavior changes. The failure mode if this fix is ever wrong (e.g. a legitimate future need for `auto` in CI) is a clear, immediate `fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free"` diagnostic in the workflow log, not a silent behavior change — trivially reversible by widening the one `case` arm back, with the new regression test updated in the same PR to match. + +**Expected effect.** No observable change to any current GitHub Actions review run (every current invocation already resolves to `free`). The effect is structural: it is no longer possible for a future workflow edit or manual dispatch override to admit priced-model spend into a required review check without an explicit, reviewed code change to this one `case` statement (and its now-locked-in regression test) first. + +**Follow-up.** If the organization later solves free+ZDR routing robustly enough to deliberately widen required-review CI to `orchestrator/auto` (e.g. once a spend ceiling and reviewer-visible cost evidence exist for that path), the change is exactly one `case` arm plus the corresponding assertions in `test_sidecar_pins_the_pool_to_free_for_github_actions` — this entry is the record of *why* it was narrowed, not a permanent prohibition. diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index 4205e4c5ab..48bb3934f8 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -279,11 +279,17 @@ esac orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}" case "$orchestrator_pool" in - free|auto) + free) pool_args=(--pool "$orchestrator_pool") ;; *) - fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free or auto" + # GitHub Actions Workflow usage of contextual-orchestrator is pinned to + # orchestrator/free: the org has not solved cost-safe free+ZDR routing + # well enough yet to justify a priced-inclusive "auto" pool in central CI, + # so "auto" is rejected here even though the launcher's own --pool flag + # (a general-purpose CLI also used outside GitHub Actions) still accepts + # it. + fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free" ;; esac diff --git a/scripts/ci/current_head_run_coalescer.py b/scripts/ci/current_head_run_coalescer.py index cb0fadeea6..0c58d32263 100644 --- a/scripts/ci/current_head_run_coalescer.py +++ b/scripts/ci/current_head_run_coalescer.py @@ -140,7 +140,13 @@ def select_duplicate_queued_run_ids( branch: str, head_sha: str, ) -> list[int]: - """Select redundant queued runs while retaining one authoritative sibling.""" + """Select redundant queued runs while retaining one authoritative sibling. + + ``_run_identity_matches`` already requires a positive-int ``workflow_id`` + before a run reaches this loop body, so re-deriving it here is only ever + non-``None`` -- grouping unconditionally, rather than behind a redundant + ``is not None`` guard, avoids a branch no input can ever fail. + """ groups: dict[int, list[dict[str, Any]]] = {} for run_data in runs: if not _run_identity_matches( @@ -148,8 +154,7 @@ def select_duplicate_queued_run_ids( ): continue workflow_id = _positive_int(run_data.get("workflow_id")) - if workflow_id is not None: - groups.setdefault(workflow_id, []).append(run_data) + groups.setdefault(workflow_id, []).append(run_data) redundant: list[int] = [] for group in groups.values(): @@ -222,7 +227,15 @@ def validate_candidate_against_live_state( current_pr_number: int | None = None, associated_prs: Mapping[int, Mapping[str, Any]] | None = None, ) -> None: - """Fail closed unless a queued candidate still has an authoritative sibling.""" + """Fail closed unless a queued candidate still has an authoritative sibling. + + ``_run_matches_head_identity`` already rejects any candidate whose + ``event`` is not in ``PR_EVENTS`` before comparing repository, branch, or + SHA, so a non-pull-request candidate always fails the head-identity check + below rather than reaching a later, narrower event-only check -- there is + no candidate shape that can satisfy head identity while carrying a + disqualifying event. + """ if candidate.get("status") != "queued": raise CoalescingRefused("candidate is no longer queued") if live_pr.get("state") != "open": @@ -241,8 +254,6 @@ def validate_candidate_against_live_state( workflow_id = _positive_int(candidate.get("workflow_id")) if candidate_id is None or workflow_id is None: raise CoalescingRefused("candidate identity is malformed") - if candidate.get("event") not in PR_EVENTS: - raise CoalescingRefused("candidate is not a pull-request workflow run") association_map = associated_prs or {} if current_pr_number is not None and not _run_pr_scope_is_safe( diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 8fee875c6d..39895e2685 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -92,6 +92,49 @@ def test_sidecar_feeds_discovery_and_policy_artifacts_to_the_launcher() -> None: assert "https://openrouter.ai/api/v1/endpoints/zdr" in text +def test_sidecar_pins_the_pool_to_free_for_github_actions() -> None: + """GitHub Actions Workflow usage of contextual-orchestrator is pinned free. + + ``auto`` was removed from the sidecar's own accepted + ``CONTEXTUAL_ORCHESTRATOR_POOL`` values: the org has not solved cost-safe + free+ZDR routing well enough yet to justify a priced-inclusive pool in + central CI. The launcher's own ``--pool`` flag (a general-purpose CLI + also used outside GitHub Actions, asserted separately above) still + accepts ``auto`` -- only this repo's GitHub-Actions-facing sidecar script + is narrowed. + """ + text = _read(SIDECAR) + assert 'orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"' in text + assert 'case "$orchestrator_pool" in\n free)' in text + assert "fail \"CONTEXTUAL_ORCHESTRATOR_POOL must be free\"" in text + assert "free|auto" not in text + assert "must be free or auto" not in text + + pool_case = text.split('orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"', 1)[ + 1 + ].split("esac", 1)[0] + for candidate, should_fail in (("free", False), ("auto", True), ("", False), ("bogus", True)): + result = subprocess.run( + [ + "bash", + "-c", + 'fail() { echo "FAIL: $*"; exit 7; }\n' + f'CONTEXTUAL_ORCHESTRATOR_POOL="{candidate}"\n' + 'orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"\n' + + pool_case + + "esac\necho \"pool_args=${pool_args[*]}\"", + ], + capture_output=True, + text=True, + ) + if should_fail: + assert result.returncode == 7, (candidate, result.stdout, result.stderr) + assert "must be free" in result.stdout + else: + assert result.returncode == 0, (candidate, result.stdout, result.stderr) + assert "pool_args=--pool free" in result.stdout + + def test_sidecar_exports_gateway_env_for_review_steps() -> None: """Only a private token-file path crosses the GitHub step boundary.""" text = _read(SIDECAR) diff --git a/tests/test_current_head_run_coalescer.py b/tests/test_current_head_run_coalescer.py index 33eee98d6c..38cc635ce8 100644 --- a/tests/test_current_head_run_coalescer.py +++ b/tests/test_current_head_run_coalescer.py @@ -126,6 +126,24 @@ def test_in_progress_run_is_never_selected_and_makes_queued_siblings_redundant() ) == [101, 102] +def test_group_with_no_queued_runs_has_nothing_to_coalesce() -> None: + """A workflow group whose only active runs are in-progress selects nothing. + + ``_run_identity_matches`` only admits runs whose ``status`` is queued or + in-progress, so a group can legitimately contain zero queued entries when + every admitted run for that workflow happens to already be running -- + the ``if not queued: continue`` guard exists precisely for that shape. + """ + module = load_module() + runs = [run_record(100, 10, status="in_progress"), run_record(101, 10, status="in_progress")] + assert module.select_duplicate_queued_run_ids( + runs, + repository="ContextualWisdomLab/.github", + branch="feature/current", + head_sha="a" * 40, + ) == [] + + def test_pull_request_target_uses_associated_pr_head_not_execution_head() -> None: """Trusted-base pull_request_target runs coalesce by their associated PR head.""" module = load_module() @@ -199,7 +217,7 @@ def test_revalidation_fails_closed_for_status_state_identity_and_event_changes() module.validate_candidate_against_live_state(candidate, live_pr=live_pr(head_sha="b" * 40), active_same_head_runs=[sibling]) with pytest.raises(module.CoalescingRefused, match="identity is malformed"): module.validate_candidate_against_live_state(run_record(0, 10), live_pr=live_pr(), active_same_head_runs=[sibling]) - with pytest.raises(module.CoalescingRefused, match="not a pull-request"): + with pytest.raises(module.CoalescingRefused, match="head moved"): module.validate_candidate_against_live_state(run_record(100, 10, event="push"), live_pr=live_pr(), active_same_head_runs=[sibling]) @@ -238,6 +256,123 @@ def test_pr_scope_rejects_other_open_pr_and_accepts_closed_matching_predecessor( ) +def test_pr_scope_unsafe_sibling_cannot_supply_authoritative_evidence() -> None: + """A sibling belonging to an independent open PR is skipped, not authoritative. + + Regression for the ``validate_candidate_against_live_state`` sibling loop + specifically (not the standalone ``_run_pr_scope_is_safe`` calls above): + a sibling that passes id/workflow/head-identity but belongs to a + different, still-open PR must be excluded from the authoritative-sibling + search entirely, not merely fail some other unrelated check. The bad + sibling's id (150) exceeds the candidate's (100), so if it were wrongly + treated as authoritative this would pass instead of failing closed. + """ + module = load_module() + candidate = run_record(100, 10, pr_number=1) + other_open = live_pr(number=2) + bad_sibling = run_record(150, 10, pr_number=2, associations=[pr_association(2)]) + with pytest.raises(module.CoalescingRefused, match="authoritative sibling"): + module.validate_candidate_against_live_state( + candidate, + live_pr=live_pr(), + active_same_head_runs=[candidate, bad_sibling], + current_pr_number=1, + associated_prs={2: other_open}, + ) + + +def test_pr_scope_rejects_a_run_with_no_pull_request_associations() -> None: + """An orphaned run with zero PR associations cannot claim any PR's scope.""" + module = load_module() + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_a_malformed_live_pr() -> None: + """A live PR missing head/base identity cannot authorize any scope decision.""" + module = load_module() + malformed_live_pr = { + "number": 1, + "state": "open", + "head": {"sha": "", "ref": "feature/current", "repo": {"full_name": "ContextualWisdomLab/.github"}}, + "base": {"sha": "c" * 40, "ref": "main", "repo": {"full_name": "ContextualWisdomLab/.github"}}, + } + assert not module._run_pr_scope_is_safe( + run_record(100, 10), + live_pr=malformed_live_pr, + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_an_association_with_a_malformed_number() -> None: + """An association carrying no positive-integer PR number is untrusted.""" + module = load_module() + malformed_association = { + **pr_association(1), + "number": None, + } + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[malformed_association]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_an_association_whose_head_does_not_match_live_head() -> None: + """An association reporting a different head than the live PR is untrusted.""" + module = load_module() + mismatched_association = pr_association(1, head_sha="b" * 40) + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[mismatched_association]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_an_association_whose_base_does_not_match_live_base() -> None: + """An association reporting a different base branch than the live PR is untrusted.""" + module = load_module() + mismatched_association = pr_association(1, base_ref="release") + assert not module._run_pr_scope_is_safe( + run_record(100, 10, associations=[mismatched_association]), + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_a_predecessor_number_missing_from_associated_prs() -> None: + """A closed-predecessor PR number with no fetched live state is untrusted.""" + module = load_module() + candidate = run_record(100, 10, pr_number=2, associations=[pr_association(2)]) + assert not module._run_pr_scope_is_safe( + candidate, + live_pr=live_pr(), + current_pr_number=1, + associated_prs={}, + ) + + +def test_pr_scope_rejects_a_predecessor_whose_live_head_does_not_match() -> None: + """A fetched closed predecessor whose live head has since moved is untrusted.""" + module = load_module() + candidate = run_record(100, 10, pr_number=2, associations=[pr_association(2)]) + moved_predecessor = live_pr(state="closed", number=2, head_sha="b" * 40) + assert not module._run_pr_scope_is_safe( + candidate, + live_pr=live_pr(), + current_pr_number=1, + associated_prs={2: moved_predecessor}, + ) + + def test_revalidation_ignores_non_authoritative_sibling_shapes() -> None: """Different workflow or malformed sibling records cannot authorize cancellation.""" module = load_module() diff --git a/tests/test_merge_scheduler_runner_image_contract.py b/tests/test_merge_scheduler_runner_image_contract.py index caf7456df5..3334ac5b87 100644 --- a/tests/test_merge_scheduler_runner_image_contract.py +++ b/tests/test_merge_scheduler_runner_image_contract.py @@ -27,7 +27,6 @@ def test_queue_draining_jobs_use_explicit_supported_image(self) -> None: """Require the scheduler control plane to use explicit Ubuntu 24.04.""" workflow = WORKFLOW.read_text(encoding='utf-8') for job_name in ( - 'cancel-closed-pr-runs', 'scan-pr-queue', 'org-queue-sweep', ): diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index 18fd482b64..a9d9c518bc 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -134,11 +134,22 @@ def test_stale_draft_request_event_does_not_exempt_live_ready_pr( def test_stale_draft_verdict_event_does_not_exempt_live_ready_pr( tmp_path: Path, ) -> None: - """A stale draft verdict snapshot cannot publish a success for a ready PR.""" + """A stale draft verdict snapshot cannot publish a success for a 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. + """ result = _run_step(tmp_path, fail_closed_script(), live_draft=False) - assert result.returncode == 19 + assert result.returncode == 1 assert "Event draft snapshot is stale" in result.stdout + assert "Reviews API read failed 3 consecutive times" in result.stdout @pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 4b098f9296..48d8808d10 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -268,9 +268,9 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non ) assert "Current-head substantive OpenCode verdict already exists; scheduler wake skipped." in dispatch_step assert "while :; do" in target_job - assert "sleep 30" in target_job + assert 'sleep "$poll_interval_seconds"' in target_job assert "enable_auto_merge:false" in workflow - assert 'gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews"' in workflow + assert 'gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews?per_page=100"' in workflow assert "github.event.pull_request.head.sha" in workflow assert "This required check is not a review and must not succeed" in workflow assert ( @@ -309,7 +309,7 @@ def _run_fail_closed_step( A fake ``gh`` that fails loudly is installed on ``PATH`` so a closed or draft early exit that reaches the Reviews API call at all fails the test immediately, rather than actually looping (the production step's - ``while :; do ... sleep 30; done`` never naturally terminates on a + ``while :; do ... sleep "$poll_interval_seconds"; done`` never naturally terminates on a non-matching review, so a real ``gh`` fixture serving no match would hang a test rather than fail it). """ @@ -355,7 +355,7 @@ def test_fail_closed_step_exempts_a_draft_pr_before_polling(tmp_path: Path) -> N (`scripts/ci/pr_review_merge_scheduler.py`'s `inspect_pr`) skips dispatching a review for an ordinary draft entirely (no `@opencode-agent` mention). With no draft exemption here, this step's - `while :; do ... sleep 30; done` loop would poll for a verdict OpenCode + `while :; do ... sleep "$poll_interval_seconds"; done` loop would poll for a verdict OpenCode will never post, until the job's own ~360-minute runtime ceiling kills it -- reproduced against this exact commit before this fix (`#1443` fixed the same class of bug on a now-superseded design; this restores @@ -523,10 +523,20 @@ def test_fail_closed_step_closed_still_takes_precedence_over_draft(tmp_path: Pat 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).""" + """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) + 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. + """ result = _run_fail_closed_step(tmp_path, pr_action="synchronize", pr_draft="false") - assert result.returncode == 17, result.stderr + assert result.returncode == 1, result.stderr assert "unexpected gh invocation after live-state validation" in result.stderr + assert "Reviews API read failed 3 consecutive times" in result.stdout @pytest.mark.parametrize( diff --git a/tests/test_queue_cancellation_revalidation.py b/tests/test_queue_cancellation_revalidation.py index 23ac824d59..e8e6b57492 100644 --- a/tests/test_queue_cancellation_revalidation.py +++ b/tests/test_queue_cancellation_revalidation.py @@ -322,7 +322,7 @@ def test_unassociated_aged_pr_uses_live_ref_not_stale_listing_sha( run_sha=current, ) assert result.returncode == 0, result.stderr - assert "authoritative current-head evidence" in result.stdout + assert "associated with an open PR at its authoritative current head" in result.stdout assert not cancelled diff --git a/tests/test_queue_cancellation_scheduler_contract.py b/tests/test_queue_cancellation_scheduler_contract.py index 16c291a6f1..70f9d75e8c 100644 --- a/tests/test_queue_cancellation_scheduler_contract.py +++ b/tests/test_queue_cancellation_scheduler_contract.py @@ -45,7 +45,7 @@ def test_reconciled_scheduler_preserves_current_main_control_plane_fixes() -> No assert '- cron: "0 * * * *"' in workflow assert '*/15 * * * *' not in workflow - assert workflow.count("runs-on: ubuntu-24.04") >= 3 + assert workflow.count("runs-on: ubuntu-24.04") >= 2 scan_job = workflow.split(" scan-pr-queue:", 1)[1].split(" org-queue-sweep:", 1)[0] assert "github.event_name == 'pull_request_review'" in scan_job.split( "TRIGGER_REVIEWS:", 1 diff --git a/tests/test_required_security_runner_image_contract.py b/tests/test_required_security_runner_image_contract.py index 2a11d1ca5d..699e4dde3f 100644 --- a/tests/test_required_security_runner_image_contract.py +++ b/tests/test_required_security_runner_image_contract.py @@ -17,13 +17,18 @@ def test_security_scan_uses_explicit_supported_image(self) -> None: """Require every Security Scan job to use explicit Ubuntu 24.04.""" workflow = SECURITY_SCAN.read_text(encoding="utf-8") self.assertNotIn("runs-on: ubuntu-latest", workflow) - self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 5) + self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 4) def test_sast_semgrep_uses_explicit_supported_image(self) -> None: - """Require both SAST Semgrep jobs to use explicit Ubuntu 24.04.""" + """Require the SAST Semgrep job to use explicit Ubuntu 24.04. + + `#1656` removed the sibling `cancel-closed-pr-runs` no-op job (it + only duplicated PR-stable workflow concurrency), leaving one runner + job in this workflow instead of two. + """ workflow = SAST_SEMGREP.read_text(encoding="utf-8") self.assertNotIn("runs-on: ubuntu-latest", workflow) - self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 2) + self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 1) if __name__ == "__main__": diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index b49e968d6a..056e47a678 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1037,11 +1037,14 @@ def test_org_queue_sweep_covers_target_repositories_on_a_heartbeat() -> None: assert '"pull_request" or .event == "pull_request_target"' in workflow assert "$current_pr_head == null or .head_sha != $current_pr_head" in workflow assert ".head_sha != $current_default_sha" in workflow - assert "do not match an open PR or default-branch Current HEAD" in workflow + assert "classified as not matching an open PR or default-branch Current HEAD" in workflow assert '.current_head // "closed-or-no-open-pr"' in workflow assert '.current_head // \\"closed-or-no-open-pr\\"' not in workflow assert "select($current_pr_heads[$head_key] == null)" in workflow - assert "Could not cancel superseded run" in workflow + revalidate_script = ( + REPO_ROOT / "scripts" / "ci" / "revalidate_queue_cancellation.sh" + ).read_text(encoding="utf-8") + assert "Could not cancel ${cancellation_mode} run" in revalidate_script assert "No run will be cancelled from incomplete evidence" in workflow assert "queue_hygiene_ready=false" in workflow # Organization sweep budgets must be consumed across the repository loop; @@ -1103,7 +1106,7 @@ def test_org_queue_sweep_superseded_run_log_filter_executes() -> None: ) assert result.returncode == 0, result.stderr - assert "current_head=closed-or-no-open-pr" in result.stdout + assert "classified_head=closed-or-no-open-pr" in result.stdout def _extract_org_sweep_rotation_snippet(workflow: str) -> str: diff --git a/tests/test_strix_llm_timeout_contract.py b/tests/test_strix_llm_timeout_contract.py index 62b0563bbc..8661486441 100644 --- a/tests/test_strix_llm_timeout_contract.py +++ b/tests/test_strix_llm_timeout_contract.py @@ -44,7 +44,7 @@ def test_strix_timeout_compat_is_installed_after_the_pinned_runtime() -> None: workflow = WORKFLOW.read_text(encoding="utf-8") token_loader = TOKEN_LOADER.read_text(encoding="utf-8") - assert "export LLM_TIMEOUT=300" in workflow + assert "export LLM_TIMEOUT=0" in workflow assert 'if [ -n "${STRIX_EXECUTABLE_PATH:-}" ]; then' in token_loader assert "install_strix_timeout_compat.py" in token_loader assert INSTALLER.is_file()