From f0d58304e24c3fc85b5d0deb88754f45e63690c2 Mon Sep 17 00:00:00 2001 From: ll7 <32880741+ll7@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:07:24 +0200 Subject: [PATCH 1/3] chore(model): restore registry-only scope --- tests/unit/test_registry.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/unit/test_registry.py b/tests/unit/test_registry.py index d522734213..9a2686bd81 100644 --- a/tests/unit/test_registry.py +++ b/tests/unit/test_registry.py @@ -1000,15 +1000,3 @@ def test_upsert_registry_entry_requires_model_id(tmp_path: Path) -> None: {"local_path": "output/model_cache/demo/model.zip"}, registry_path=tmp_path / "registry.yaml", ) - - -def test_predictive_proxy_registry_paths_match_release_assets() -> None: - """Predictive proxy cache pointers should name their release assets exactly.""" - registry_path = Path(__file__).resolve().parents[2] / "model" / "registry.yaml" - entries = registry.load_registry(registry_path) - - for model_id in ("predictive_proxy_selected_v1", "predictive_proxy_selected_v2_full"): - entry = entries[model_id] - release = entry["github_release"] - assert Path(entry["local_path"]).parts[:3] == ("output", "model_cache", model_id) - assert Path(entry["local_path"]).name == release["asset_name"] From a2bf33373e5775eacbeed5049c15b987ad5af9df Mon Sep 17 00:00:00 2001 From: ll7 <32880741+ll7@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:55:45 +0200 Subject: [PATCH 2/3] fix(ci): close remaining uv sync diagnostic gaps (#8249) (#8446) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This successor closes the remaining boundedness gap in the CI `ci_uv_sync_diag` probe. It removes the unbounded `uv cache size` traversal and makes both advisory `du` probes deadline-aware and diagnostically explicit. ## Linked Issues - Relates to #8249 (successor follow-up to merged PR #8365; the issue is reopened for this work). ## Stack / Dependency - Base dependency: none. - Required prior PRs and stack follow-up issues: none. - Safe to review independently: yes; the branch is based on current `main`. ## What Changed - Removed the redundant `uv cache size` cache-root traversal. - Require a positive `ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS` value, defaulting invalid values (including zero) to 10 seconds. - Detect GNU `timeout(1)` explicitly and use `--kill-after=2s` after the configured deadline. - Emit `ok`, `timed-out`, or `error` sizing status markers and exit codes for cache and virtualenv probes. - Added deterministic tests for the unbounded-`uv` regression, zero timeout, hard timeout timing, non-timeout errors, no-GNU fallback behavior, and the script contract. ## Why It Matters The diagnostic remains advisory and exits zero, while its expensive cache and virtual-environment walks no longer hide failures or consume the full readiness test budget. Hosts without GNU `timeout(1)` retain the prior direct-`du` fallback and ordinary output keys. ## Research / Evidence Notes Not applicable — this is a CI/tooling reliability change with no research, benchmark, metric, or paper-facing claim. ## Validation / Proof - `tests/dev/test_ci_uv_sync_diag.py`: 12 passed. - `tests/test_ci_script_contract.py`: 153 passed. - `bash -n scripts/dev/ci_uv_sync_diag.sh`: passed. - Ruff check and format checks for both changed Python test files: passed. - `BASE_REF=origin/main PR_READY_MODE=final ... scripts/dev/pr_ready_check.sh`: passed on committed head `8e7015c578279c1d4e1dd31279b0143bf9161eec` against base `5f476625eba9c4ae2fa5770781852b78c865fd8b`. ## Risks / Rollback The no-GNU fallback remains intentionally unbounded for compatibility with stock macOS-style hosts. Revert commit `8e7015c57` to restore the preceding diagnostic implementation if needed. ## Docs / Provenance No durable benchmark or model artifacts were produced. Readiness receipts remain worktree-local ignored output; the source contract and reproducible test fixtures are tracked in this PR. ## Downstream Propagation Not applicable — support/tooling change; no benchmark, metric, model, claim-map, registry, or durable evidence update is required. ## Follow-Up / Residual Scope No deferred work remains for this implementation slice. Issue #8249 is reopened for this PR; this is the immediate successor requested for the remaining `uv sync` diagnostic gaps. ## Reviewer Notes - Verify that `uv cache size` is absent from the executable path and that the later bounded `du` remains the sole cache-size traversal. - Verify that timeout expirations are not attributed to host contention without evidence and that non-timeout tool failures remain visible. --- scripts/dev/ci_uv_sync_diag.sh | 79 +++++++++---- tests/dev/test_ci_uv_sync_diag.py | 190 ++++++++++++++++++++++++++++-- tests/test_ci_script_contract.py | 23 ++++ 3 files changed, 259 insertions(+), 33 deletions(-) diff --git a/scripts/dev/ci_uv_sync_diag.sh b/scripts/dev/ci_uv_sync_diag.sh index 25890b1163..bd899c9379 100755 --- a/scripts/dev/ci_uv_sync_diag.sh +++ b/scripts/dev/ci_uv_sync_diag.sh @@ -13,31 +13,51 @@ set -uo pipefail label="${1:-uv-sync-diag}" # Bounded tree sizing (issue #8249): `du` walks scale with cache/venv size and -# shared-host I/O contention (measured 28s for a 60GB cache against the 30s -# test budget, so loaded hosts exceed it). Each probe below is capped so this -# advisory diagnostic stays fast; on timeout it reports +# shared-host I/O contention. With GNU timeout(1), each tree-size probe below +# is capped so this advisory diagnostic stays fast; on timeout it reports # `unavailable-timed-out` instead of hanging the caller. Override the cap with # ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS (default 10). Hosts without GNU timeout(1) -# (e.g. macOS) keep the previous unbounded behavior. +# (e.g. macOS) keep the previous direct-`du` fallback behavior. diag_du_timeout="${ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS:-10}" -if ! [[ "$diag_du_timeout" =~ ^[0-9]+$ ]]; then +if ! [[ "$diag_du_timeout" =~ ^[1-9][0-9]*$ ]]; then diag_du_timeout=10 fi # GNU timeout(1) is absent on some hosts (e.g. stock macOS bash 3.2 runners); -# without it keep the previous unbounded behavior. The function form (rather -# than an arg array) stays safe under `set -u` on old bash versions. -du_timeout_secs="" -if command -v timeout >/dev/null 2>&1; then - du_timeout_secs="$diag_du_timeout" +# without it keep the previous direct-`du` behavior. Detect GNU explicitly so +# a non-GNU command with the same name does not receive incompatible options. +# The function form (rather than an arg array) stays safe under `set -u` on old +# bash versions. +du_timeout_bin="" +if command -v timeout >/dev/null 2>&1 && + timeout --version 2>/dev/null | grep -q "GNU coreutils"; then + du_timeout_bin="$(command -v timeout)" fi +du_timeout_kill_after_secs=2 bounded_du() { - if [[ -n "${du_timeout_secs:-}" ]]; then - timeout "$du_timeout_secs" "$@" + if [[ -n "${du_timeout_bin:-}" ]]; then + "$du_timeout_bin" --kill-after="${du_timeout_kill_after_secs}s" "$diag_du_timeout" "$@" else "$@" fi } +du_timed_out() { + local rc="$1" + [[ -n "${du_timeout_bin:-}" && ( "$rc" -eq 124 || "$rc" -eq 137 ) ]] +} + +report_du_failure() { + local prefix="$1" + local rc="$2" + if du_timed_out "$rc"; then + echo " ${prefix}_sizing_status=timed-out" + echo " ${prefix}_sizing_timeout_seconds=${diag_du_timeout}" + else + echo " ${prefix}_sizing_status=error" + fi + echo " ${prefix}_sizing_exit_code=${rc}" +} + echo "::group::${label}" echo "uv_sync_diag runner_info" @@ -52,11 +72,6 @@ echo "uv_sync_diag uv_info" if command -v uv >/dev/null 2>&1; then echo " uv_version=$(uv --version 2>/dev/null || echo unknown)" echo " uv_cache_dir=$(uv cache dir 2>/dev/null || echo unknown)" - # Best-effort cache size report; older uv versions may not have 'uv cache size'. - uv_cache_size="$(uv cache size 2>/dev/null || true)" - if [[ -n "$uv_cache_size" ]]; then - echo " uv_cache_size=${uv_cache_size}" - fi else echo " uv_version=not_installed" fi @@ -100,15 +115,14 @@ if [[ -d "$cache_dir" ]]; then # tree up to a dozen times and risking preflight timeouts on large caches. # The captured output is then parsed in a single awk pass (pure in-memory, # no further disk I/O), preserving the curated key names and ordering. - # The walk itself is capped by bounded_du (issue #8249); exit 124 means the - # probe timed out under contention, which is reported, not retried. + # The walk itself is capped by bounded_du (issue #8249). It is the sole + # cache-size traversal; `uv cache size` is intentionally not called because + # that full-cache operation cannot be deadline controlled here. cache_du="" cache_du_rc=0 cache_du="$(bounded_du du -h -d 1 "$cache_dir" 2>/dev/null)" || cache_du_rc=$? - if [[ "$cache_du_rc" -eq 124 ]]; then - echo " cache_total_size=unavailable-timed-out" - echo " cache_sizing_note=du exceeded ${diag_du_timeout}s under host contention" - else + if [[ "$cache_du_rc" -eq 0 ]]; then + echo " cache_sizing_status=ok" printf '%s\n' "$cache_du" | awk -F'\t' -v dir="$cache_dir" ' { size[$2] = $1 } END { @@ -120,6 +134,13 @@ if [[ -d "$cache_dir" ]]; then } } ' + else + report_du_failure "cache" "$cache_du_rc" + if du_timed_out "$cache_du_rc"; then + echo " cache_total_size=unavailable-timed-out" + else + echo " cache_total_size=unavailable-error" + fi fi else echo " cache_dir=${cache_dir} (does not exist)" @@ -129,10 +150,16 @@ echo "uv_sync_diag venv_info" if [[ -d .venv ]]; then venv_du_rc=0 venv_du="$(bounded_du du -sh .venv 2>/dev/null)" || venv_du_rc=$? - if [[ "$venv_du_rc" -eq 124 ]]; then - echo " venv_size=unavailable-timed-out" - else + if [[ "$venv_du_rc" -eq 0 ]]; then + echo " venv_sizing_status=ok" printf '%s\n' "$venv_du" | awk '{print " venv_size="$1}' || true + else + report_du_failure "venv" "$venv_du_rc" + if du_timed_out "$venv_du_rc"; then + echo " venv_size=unavailable-timed-out" + else + echo " venv_size=unavailable-error" + fi fi if [[ -x .venv/bin/python ]]; then echo " python_version=$(.venv/bin/python --version 2>&1 || true)" diff --git a/tests/dev/test_ci_uv_sync_diag.py b/tests/dev/test_ci_uv_sync_diag.py index c2a1a8c74a..398df4f300 100644 --- a/tests/dev/test_ci_uv_sync_diag.py +++ b/tests/dev/test_ci_uv_sync_diag.py @@ -5,6 +5,7 @@ import os import shutil import subprocess +import time from pathlib import Path import pytest @@ -118,6 +119,7 @@ def test_ci_uv_sync_diag_reports_runner_and_uv_state() -> None: assert "uv_sync_diag cache_size" in output assert "uv_sync_diag venv_info" in output assert "::endgroup::" in output + assert "uv_cache_size=" not in output if shutil.which("uv"): assert "uv_version=uv " in output @@ -186,26 +188,50 @@ def test_ci_uv_sync_diag_cache_sizing_is_single_pass(tmp_path: Path) -> None: def test_ci_uv_sync_diag_du_timeout_reports_unavailable(tmp_path: Path) -> None: - """Slow `du` walks under host contention must not hang the probe (issue #8249). + """Slow `du` walks must not hang the probe (issue #8249). - A 60GB cache measured 28s for one `du` pass against the 30s test budget, so - loaded hosts exceed it. With a slow-`du` shim and a 1s probe budget, the - script must still exit 0 quickly with timed-out markers. Completion inside - the outer 30s budget is itself the boundedness proof. + With a TERM-resistant slow-`du` shim and a 1s probe budget, both probes must + be killed promptly after the configured deadline and the script must still + exit 0 with timed-out markers. The fake uv also proves that the diagnostic + no longer performs the unbounded ``uv cache size`` traversal. """ timeout_bin = shutil.which("timeout") if timeout_bin is None: pytest.skip("GNU timeout(1) is required for the du-budget probe") + timeout_version = subprocess.run( + [timeout_bin, "--version"], capture_output=True, text=True, check=False, timeout=5 + ) + if "GNU coreutils" not in timeout_version.stdout: + pytest.skip("GNU timeout(1) is required for the du-budget probe") script = _script_path() bash_path = shutil.which("bash") assert bash_path, "bash is required for this test" fake_bin = tmp_path / "bin" fake_bin.mkdir() + du_calls = tmp_path / "du_calls.log" slow_du = fake_bin / "du" - slow_du.write_text("#!/usr/bin/env bash\nsleep 30\n", encoding="utf-8") + slow_du.write_text( + f'#!/usr/bin/env bash\nprintf "%s\\n" "$*" >> "{du_calls}"\ntrap \'\' TERM\nsleep 30\n', + encoding="utf-8", + ) slow_du.chmod(0o755) + uv_calls = tmp_path / "uv_calls.log" + uv_shim = fake_bin / "uv" + uv_shim.write_text( + "#!/usr/bin/env bash\n" + f'printf "%s\\n" "$*" >> "{uv_calls}"\n' + 'case "$*" in\n' + ' "--version") printf "uv 0.11.21\\n" ;;\n' + ' "cache dir") printf "%s\\n" "$UV_CACHE_DIR" ;;\n' + ' "cache size") sleep 30 ;;\n' + " *) exit 99 ;;\n" + "esac\n", + encoding="utf-8", + ) + uv_shim.chmod(0o755) + cache_dir = tmp_path / "uv-cache" cache_dir.mkdir() (tmp_path / ".venv" / "bin").mkdir(parents=True) @@ -215,6 +241,7 @@ def test_ci_uv_sync_diag_du_timeout_reports_unavailable(tmp_path: Path) -> None: env["UV_CACHE_DIR"] = str(cache_dir) env["ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS"] = "1" + started = time.monotonic() result = subprocess.run( [bash_path, str(script), "du-timeout-test"], capture_output=True, @@ -222,14 +249,163 @@ def test_ci_uv_sync_diag_du_timeout_reports_unavailable(tmp_path: Path) -> None: check=False, env=env, cwd=tmp_path, # .venv dir present here, so both du probes fire - timeout=30, + timeout=10, ) + elapsed = time.monotonic() - started assert result.returncode == 0, f"stderr: {result.stderr}" + assert elapsed < 8, f"diagnostic took {elapsed:.2f}s instead of stopping near the 1s probes" assert "cache_total_size=unavailable-timed-out" in result.stdout + assert "cache_sizing_status=timed-out" in result.stdout + assert "cache_sizing_timeout_seconds=1" in result.stdout assert "venv_size=unavailable-timed-out" in result.stdout + assert "venv_sizing_status=timed-out" in result.stdout + assert "venv_sizing_timeout_seconds=1" in result.stdout + assert "under host contention" not in result.stdout + uv_invocations = uv_calls.read_text(encoding="utf-8").splitlines() + assert "cache size" not in uv_invocations + assert len(du_calls.read_text(encoding="utf-8").splitlines()) == 2 assert "::endgroup::" in result.stdout +def test_ci_uv_sync_diag_zero_timeout_uses_default(tmp_path: Path) -> None: + """A zero timeout must not disable the GNU timeout boundary.""" + script = _script_path() + bash_path = shutil.which("bash") + assert bash_path, "bash is required for this test" + + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + timeout_calls = tmp_path / "timeout_calls.log" + fake_timeout = fake_bin / "timeout" + fake_timeout.write_text( + "#!/bin/bash\n" + 'if [[ "${1:-}" == "--version" ]]; then\n' + ' printf "timeout (GNU coreutils) 9.1\\n"\n' + " exit 0\n" + "fi\n" + f'printf "%s\\n" "$*" >> "{timeout_calls}"\n' + "exit 124\n", + encoding="utf-8", + ) + fake_timeout.chmod(0o755) + + cache_dir = tmp_path / "uv-cache" + cache_dir.mkdir() + env = os.environ.copy() + env["PATH"] = f"{fake_bin}{os.pathsep}{env['PATH']}" + env["UV_CACHE_DIR"] = str(cache_dir) + env["ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS"] = "0" + + result = subprocess.run( + [bash_path, str(script), "zero-timeout-test"], + capture_output=True, + text=True, + check=False, + env=env, + cwd=tmp_path, + timeout=5, + ) + assert result.returncode == 0, f"stderr: {result.stderr}" + assert "cache_sizing_status=timed-out" in result.stdout + assert "cache_sizing_timeout_seconds=10" in result.stdout + assert "cache_total_size=unavailable-timed-out" in result.stdout + calls = timeout_calls.read_text(encoding="utf-8").splitlines() + assert len(calls) == 1 + assert "--kill-after=2s 10 du -h -d 1" in calls[0] + assert "--kill-after=2s 0 " not in calls[0] + + +def test_ci_uv_sync_diag_reports_du_errors(tmp_path: Path) -> None: + """Non-timeout `du` failures must remain visible in advisory output.""" + script = _script_path() + bash_path = shutil.which("bash") + assert bash_path, "bash is required for this test" + + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + failing_du = fake_bin / "du" + failing_du.write_text("#!/bin/bash\nexit 125\n", encoding="utf-8") + failing_du.chmod(0o755) + + cache_dir = tmp_path / "uv-cache" + cache_dir.mkdir() + (tmp_path / ".venv" / "bin").mkdir(parents=True) + env = os.environ.copy() + env["PATH"] = f"{fake_bin}{os.pathsep}{env['PATH']}" + env["UV_CACHE_DIR"] = str(cache_dir) + + result = subprocess.run( + [bash_path, str(script), "du-error-test"], + capture_output=True, + text=True, + check=False, + env=env, + cwd=tmp_path, + timeout=10, + ) + assert result.returncode == 0, f"stderr: {result.stderr}" + assert "cache_sizing_status=error" in result.stdout + assert "cache_sizing_exit_code=125" in result.stdout + assert "cache_total_size=unavailable-error" in result.stdout + assert "venv_sizing_status=error" in result.stdout + assert "venv_sizing_exit_code=125" in result.stdout + assert "venv_size=unavailable-error" in result.stdout + + +def test_ci_uv_sync_diag_without_gnu_timeout_preserves_du_output(tmp_path: Path) -> None: + """The direct-`du` fallback must preserve ordinary size markers.""" + script = _script_path() + bash_path = shutil.which("bash") + assert bash_path, "bash is required for this test" + + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + for command_name in ("awk", "date", "df", "nproc", "uptime"): + command_path = shutil.which(command_name) + if command_path: + (fake_bin / command_name).symlink_to(command_path) + assert (fake_bin / "awk").exists(), "awk is required for this fallback test" + + du_calls = tmp_path / "du_calls.log" + du_shim = fake_bin / "du" + du_shim.write_text( + "#!/bin/bash\n" + f'printf "%s\\n" "$*" >> "{du_calls}"\n' + 'target="${@: -1}"\n' + 'if [[ "$target" == ".venv" ]]; then\n' + ' printf "4K\\t%s\\n" "$target"\n' + "else\n" + ' printf "8K\\t%s\\n" "$target"\n' + "fi\n", + encoding="utf-8", + ) + du_shim.chmod(0o755) + + cache_dir = tmp_path / "uv-cache" + cache_dir.mkdir() + (tmp_path / ".venv" / "bin").mkdir(parents=True) + env = _clean_diag_env(tmp_path) + env["PATH"] = str(fake_bin) + + assert shutil.which("timeout", path=env["PATH"]) is None + result = subprocess.run( + [bash_path, str(script), "no-timeout-test"], + capture_output=True, + text=True, + check=False, + env=env, + cwd=tmp_path, + timeout=10, + ) + assert result.returncode == 0, f"stderr: {result.stderr}" + assert "cache_sizing_status=ok" in result.stdout + assert "cache_total_size=8K" in result.stdout + assert "venv_sizing_status=ok" in result.stdout + assert "venv_size=4K" in result.stdout + assert "unavailable-timed-out" not in result.stdout + assert len(du_calls.read_text(encoding="utf-8").splitlines()) == 2 + + def test_workflow_uv_cache_is_pruned_by_setup_uv() -> None: """CI must use setup-uv's pruned cache without a second unbounded payload cache. diff --git a/tests/test_ci_script_contract.py b/tests/test_ci_script_contract.py index 70b82cfd78..ec8924a289 100644 --- a/tests/test_ci_script_contract.py +++ b/tests/test_ci_script_contract.py @@ -58,6 +58,7 @@ CHECK_CARLA_RUNTIME = ROOT / "scripts" / "dev" / "check_carla_runtime.sh" CI_INSTALL_HEADLESS_PACKAGES = ROOT / "scripts" / "dev" / "ci_install_headless_packages.sh" EVIDENCE_REGISTRY_RATCHET = ROOT / "scripts" / "dev" / "evidence_registry_ratchet.py" +CI_UV_SYNC_DIAG = ROOT / "scripts" / "dev" / "ci_uv_sync_diag.sh" COVERAGE_GUIDE = ROOT / "docs" / "coverage_guide.md" DEV_GUIDE = ROOT / "docs" / "dev_guide.md" CI_WORKFLOW = ROOT / ".github" / "workflows" / "ci.yml" @@ -76,6 +77,28 @@ def test_ci_driver_smoke_uses_runtime_schema_and_output_matrix_path() -> None: assert "cat > matrix.yaml" not in script_text +def test_ci_uv_sync_diag_preserves_bounded_sizing_contract() -> None: + """Keep diagnostic sizing bounded and make advisory failures observable.""" + + script_text = CI_UV_SYNC_DIAG.read_text(encoding="utf-8") + + assert 'diag_du_timeout" =~ ^[1-9][0-9]*$' in script_text + assert 'timeout --version 2>/dev/null | grep -q "GNU coreutils"' in script_text + assert "du_timeout_kill_after_secs=2" in script_text + assert '"$du_timeout_bin" --kill-after="${du_timeout_kill_after_secs}s"' in script_text + assert 'cache_du="$(bounded_du du -h -d 1 "$cache_dir" 2>/dev/null)"' in script_text + assert 'venv_du="$(bounded_du du -sh .venv 2>/dev/null)"' in script_text + assert "${prefix}_sizing_status=timed-out" in script_text + assert "${prefix}_sizing_status=error" in script_text + assert "${prefix}_sizing_exit_code=" in script_text + assert "cache_total_size=unavailable-timed-out" in script_text + assert "cache_total_size=unavailable-error" in script_text + assert "venv_size=unavailable-timed-out" in script_text + assert "venv_size=unavailable-error" in script_text + assert "uv_cache_size=" not in script_text + assert "uv cache size 2>/dev/null" not in script_text + + def test_ci_driver_test_phase_uses_shared_parallel_test_wrapper() -> None: """Preserve the shared pytest wrapper and default testpaths in the CI driver.""" From e7b6f9bfde9f80ca65d62bb1d85c681946e2a3dd Mon Sep 17 00:00:00 2001 From: ll7 <32880741+ll7@users.noreply.github.com> Date: Sat, 5 Sep 2026 00:29:34 +0200 Subject: [PATCH 3/3] fix(dev): paginate main CI incident evidence (#8445) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Repair the scheduled main-CI incident reconciler's evidence acquisition so cancellation-heavy workflow history cannot hide the decisive runs required by the two-green closure rule. ## Linked Issues - Refs #8414 (historical cancellation-saturated reconciliation window; the incident is already closed and this PR does not change its state). ## Stack / Dependency - Base dependency: none. - Required prior PRs and stack follow-up issues: none. - Safe to review independently: yes; this is a self-contained maintenance-tooling change. ## What Changed - Replaced the default raw `gh run list` evidence cutoff with bounded GitHub REST pagination. - Resolve the configured workflow display name, read full 100-run pages, and stop only after two decisive completed green/red runs are visible. - Keep cancellations, pending runs, malformed payloads, and page-budget exhaustion fail-closed. - Add focused tests for cancellation-saturated pages, page-budget exhaustion, and page-budget routing; document the new `--max-run-pages` option and legacy `--run-limit` behavior. ## Why It Matters The prior ten-raw-run window could contain only cancelled runs under the repository's latest-main-wins concurrency policy, producing a pending report despite decisive greens just outside the window. The new default searches a bounded evidence window without treating cancellations as failures or making a current-head-green claim. ## Research / Evidence Notes Not applicable — no research claim. This changes only incident-reconciliation tooling and its operational evidence-window contract. ## Validation / Proof - `python -m pytest -q tests/dev/test_reconcile_main_ci_incidents.py` — 19 passed. - Ruff check and format checks passed for the changed Python files. - `python scripts/dev/check_docs_evidence_integrity.py --full` — 2052 Markdown files passed. - `bash scripts/dev/check_context_notes.sh` — passed. - Read-only live Actions REST probe from the linked worktree returned 100 current `CI` runs and identified the current decisive successes/failures. - Full final PR readiness is run against the committed exact head before publication. ## Risks / Rollback The default may make additional read-only Actions requests when cancellations fill a page; the ten-page ceiling bounds that work. If workflow inventory or run pagination cannot be verified, the reconciler returns an error and performs no issue mutation. Rollback is a single revert of this tooling commit. ## Docs / Provenance Updated `docs/dev_guide_reference.md` with the paginated evidence-window contract. No benchmark, model, checkpoint, raw log, or generated evidence artifact is changed. ## Downstream Propagation Not applicable — support change. ## Follow-Up / Residual Scope None; current-head CI failures remain separate incidents keyed to their own deciding runs. ## Reviewer Notes - Verify that only completed `success`/`failure` runs count as decisive evidence. - Verify that the page budget fails closed before any issue comment or close mutation. - This PR intentionally does not close or reopen #8414 and does not assert that any current head is green. --- docs/dev_guide_reference.md | 8 + scripts/dev/reconcile_main_ci_incidents.py | 179 +++++++++++++++++- tests/dev/test_reconcile_main_ci_incidents.py | 155 +++++++++++++++ 3 files changed, 339 insertions(+), 3 deletions(-) diff --git a/docs/dev_guide_reference.md b/docs/dev_guide_reference.md index 7aaa33c911..7e31ee558c 100644 --- a/docs/dev_guide_reference.md +++ b/docs/dev_guide_reference.md @@ -2595,6 +2595,14 @@ two newer consecutive decisive green runs before it posts an evidence comment and closes an incident as completed. Active, pending, malformed, or concurrent-change cases remain open. +The Actions run evidence window is paginated. The reconciler reads full +workflow-run pages and stops only after two decisive completed green/red runs +are visible, so a cancellation-saturated newest page cannot hide the decisive +history. The default page budget is ten; `--max-run-pages N` changes it, and +the legacy `--run-limit N` option is retained as an alias for that page budget. +If the budget is exhausted before two decisive runs are found, the helper +fails closed instead of classifying an incomplete window. + The helper is report-only unless `--apply` is supplied, so an offline or local inspection can use: diff --git a/scripts/dev/reconcile_main_ci_incidents.py b/scripts/dev/reconcile_main_ci_incidents.py index d5916b411f..40da304770 100644 --- a/scripts/dev/reconcile_main_ci_incidents.py +++ b/scripts/dev/reconcile_main_ci_incidents.py @@ -13,6 +13,11 @@ changed body, label set, or state causes the item to be skipped or reported as failed. Malformed incidents, active incidents, incomplete green evidence, and API failures never close an issue. + +The Actions run reader paginates past cancellation-heavy raw pages and stops +only after two decisive completed runs are visible, subject to a bounded page +budget. It fails closed when that budget is exhausted before the evidence +window is complete. """ from __future__ import annotations @@ -31,13 +36,16 @@ from scripts.dev._gh_rest import parse_json, run_gh_api from scripts.dev.main_ci_incident_reconcile import ( build_incident_signal, - fetch_runs, incident_reconcile_status, ) from scripts.dev.main_ci_is_green import classify DEFAULT_REPO = "ll7/robot_sf_ll7" DEFAULT_WORKFLOW = "CI" +# ``run_limit`` is retained in the public Python/CLI contract for callers that +# inject the legacy ``gh run list`` fetcher. The default REST path interprets +# it as a page budget, not as a raw-run cutoff, so cancellation-heavy windows +# cannot hide the decisive records behind the first page. DEFAULT_RUN_LIMIT = 10 DEFAULT_MAX_PAGES = 10 DEFAULT_MAX_COMMENT_PAGES = 10 @@ -137,6 +145,144 @@ def _paginate_collection( ) +def _resolve_workflow_selector( + *, + repo: str, + workflow: str, + max_pages: int, + runner: Runner, +) -> str: + """Resolve a workflow display name to a stable REST workflow selector.""" + selector = workflow.strip() + if not selector: + raise ReconciliationError("workflow must not be empty") + if selector.isdecimal() or selector.lower().endswith((".yml", ".yaml")): + return selector + + rows: list[Mapping[str, Any]] = [] + for page in range(1, max_pages + 1): + endpoint = ( + f"repos/{quote(repo, safe='/')}/actions/workflows?per_page={PER_PAGE}&page={page}" + ) + payload = _api_json( + endpoint, + runner=runner, + operation="Actions workflow inventory", + ) + if not isinstance(payload, Mapping): + raise ReconciliationError("Actions workflow inventory returned a non-object payload") + page_rows = payload.get("workflows") + if not isinstance(page_rows, list) or any( + not isinstance(row, Mapping) for row in page_rows + ): + raise ReconciliationError("Actions workflow inventory returned malformed rows") + rows.extend(row for row in page_rows if isinstance(row, Mapping)) + if len(page_rows) < PER_PAGE: + break + else: + raise ReconciliationError( + f"Actions workflow inventory exceeded the {max_pages}-page budget; " + "refusing an ambiguous workflow selector" + ) + + matches = [ + row + for row in rows + if row.get("name") == selector + or row.get("path") == selector + or str(row.get("path") or "").rsplit("/", 1)[-1] == selector + ] + if not matches: + raise ReconciliationError(f"workflow {workflow!r} was not found") + if len(matches) > 1: + raise ReconciliationError(f"workflow {workflow!r} resolved to multiple workflows") + workflow_id = _positive_int(matches[0].get("id"), field="workflow id") + return str(workflow_id) + + +def _normalize_actions_run(row: Mapping[str, Any], *, index: int) -> dict[str, Any]: + """Normalize one Actions REST run to the existing classifier schema.""" + run_id = _positive_int(row.get("id"), field=f"Actions run row {index} id") + status = row.get("status") + if not isinstance(status, str) or not status: + raise ReconciliationError(f"Actions run row {index} has no usable status") + conclusion = row.get("conclusion") + if conclusion is not None and not isinstance(conclusion, str): + raise ReconciliationError(f"Actions run row {index} has a malformed conclusion") + created_at = row.get("created_at") + if not isinstance(created_at, str) or not created_at: + raise ReconciliationError(f"Actions run row {index} has no usable created_at") + head_sha = row.get("head_sha") + if head_sha is not None and not isinstance(head_sha, str): + raise ReconciliationError(f"Actions run row {index} has a malformed head_sha") + return { + "databaseId": run_id, + "status": status, + "conclusion": conclusion, + "headSha": head_sha, + "createdAt": created_at, + } + + +def _fetch_main_ci_runs( + repo: str, + workflow: str, + *, + max_pages: int, + runner: Runner, +) -> list[dict[str, Any]]: + """Fetch a bounded Actions window until two decisive runs are visible. + + GitHub's latest-main-wins concurrency can make the newest raw pages almost + entirely cancelled. A raw ``--limit`` therefore does not identify a + sufficient evidence window. Read full REST pages and stop only after the + window contains two completed green/red runs; a complete short final page + is also a valid stopping point. Hitting the page budget before finding + two decisive runs fails closed instead of silently classifying a partial + history. + """ + if max_pages <= 0: + raise ValueError("max_run_pages must be positive") + selector = _resolve_workflow_selector( + repo=repo, + workflow=workflow, + max_pages=max_pages, + runner=runner, + ) + endpoint_base = ( + f"repos/{quote(repo, safe='/')}/actions/workflows/{quote(selector, safe='')}/runs" + f"?{urlencode({'branch': 'main'})}" + ) + runs: list[dict[str, Any]] = [] + for page in range(1, max_pages + 1): + endpoint = f"{endpoint_base}&per_page={PER_PAGE}&page={page}" + payload = _api_json( + endpoint, + runner=runner, + operation=f"main-CI runs page {page}", + ) + if not isinstance(payload, Mapping): + raise ReconciliationError(f"main-CI runs page {page} returned a non-object payload") + page_rows = payload.get("workflow_runs") + if not isinstance(page_rows, list) or any( + not isinstance(row, Mapping) for row in page_rows + ): + raise ReconciliationError(f"main-CI runs page {page} returned malformed rows") + runs.extend( + _normalize_actions_run(row, index=index) + for index, row in enumerate(page_rows, start=len(runs)) + if isinstance(row, Mapping) + ) + if len(_ordered_decisive_runs(runs)) >= 2: + return runs + if len(page_rows) < PER_PAGE: + return runs + raise ReconciliationError( + f"main-CI run search exceeded the {max_pages}-page budget before " + "finding two decisive runs; refusing a partial evidence window" + ) + + def _label_names(row: Mapping[str, Any], *, issue: int) -> set[str]: """Validate and normalize an issue's REST label objects.""" raw_labels = row.get("labels") @@ -591,6 +737,7 @@ def reconcile_batch( workflow: str = DEFAULT_WORKFLOW, apply: bool = False, run_limit: int = DEFAULT_RUN_LIMIT, + max_run_pages: int | None = None, max_pages: int = DEFAULT_MAX_PAGES, max_comment_pages: int = DEFAULT_MAX_COMMENT_PAGES, max_issues: int = DEFAULT_MAX_ISSUES, @@ -601,6 +748,9 @@ def reconcile_batch( """Inventory, classify, and optionally reconcile all open incidents.""" if run_limit <= 0 or max_issues <= 0 or max_mutations <= 0: raise ValueError("run_limit, max_issues, and max_mutations must be positive") + resolved_run_pages = run_limit if max_run_pages is None else max_run_pages + if resolved_run_pages <= 0: + raise ValueError("max_run_pages must be positive") rest_runner = runner or _default_runner incidents = list_open_incidents(repo=repo, max_pages=max_pages, runner=rest_runner) if len(incidents) > max_issues: @@ -619,6 +769,7 @@ def reconcile_batch( "open_incident_count": 0, "pagination_complete": True, "run_limit": run_limit, + "run_page_limit": resolved_run_pages, "run_count": 0, }, "run_window": [], @@ -628,7 +779,16 @@ def reconcile_batch( "status": "ok", } try: - runs = (run_fetcher or fetch_runs)(repo, workflow, run_limit) + if run_fetcher is not None: + # Preserve the injectable legacy contract used by offline callers. + runs = run_fetcher(repo, workflow, run_limit) + else: + runs = _fetch_main_ci_runs( + repo, + workflow, + max_pages=resolved_run_pages, + runner=rest_runner, + ) except (RuntimeError, json.JSONDecodeError) as exc: raise ReconciliationError(f"main-CI run fetch failed: {exc}") from exc if not isinstance(runs, list): @@ -684,6 +844,7 @@ def reconcile_batch( "open_incident_count": len(incidents), "pagination_complete": True, "run_limit": run_limit, + "run_page_limit": resolved_run_pages, "run_count": len(runs), }, "run_window": [ @@ -726,7 +887,18 @@ def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", default=os.environ.get("GITHUB_REPOSITORY", DEFAULT_REPO)) parser.add_argument("--workflow", default=DEFAULT_WORKFLOW) - parser.add_argument("--run-limit", type=int, default=DEFAULT_RUN_LIMIT) + parser.add_argument( + "--run-limit", + type=int, + default=DEFAULT_RUN_LIMIT, + help=("maximum Actions run-search pages (legacy option name; raw runs are not the bound)"), + ) + parser.add_argument( + "--max-run-pages", + type=int, + default=None, + help="maximum paginated Actions run pages; overrides --run-limit when supplied", + ) parser.add_argument("--max-pages", type=int, default=DEFAULT_MAX_PAGES) parser.add_argument("--max-comment-pages", type=int, default=DEFAULT_MAX_COMMENT_PAGES) parser.add_argument("--max-issues", type=int, default=DEFAULT_MAX_ISSUES) @@ -751,6 +923,7 @@ def main(argv: Sequence[str] | None = None) -> int: workflow=args.workflow, apply=args.apply, run_limit=args.run_limit, + max_run_pages=args.max_run_pages, max_pages=args.max_pages, max_comment_pages=args.max_comment_pages, max_issues=args.max_issues, diff --git a/tests/dev/test_reconcile_main_ci_incidents.py b/tests/dev/test_reconcile_main_ci_incidents.py index 9af2cfb8ff..00b3ee6c4b 100644 --- a/tests/dev/test_reconcile_main_ci_incidents.py +++ b/tests/dev/test_reconcile_main_ci_incidents.py @@ -105,11 +105,113 @@ def __call__( raise AssertionError(f"unexpected REST path: {path}") +def _actions_run(run: dict[str, Any]) -> dict[str, Any]: + """Convert a classifier-shaped run into an Actions REST row.""" + return { + "id": run["databaseId"], + "status": run["status"], + "conclusion": run["conclusion"], + "head_sha": run["headSha"], + "created_at": run["createdAt"], + } + + +class FakeActionsRunREST: + """REST fake for workflow resolution and paginated Actions runs.""" + + def __init__(self, pages: list[list[dict[str, Any]]]) -> None: + """Initialize paginated raw workflow-run rows.""" + self.pages = pages + self.calls: list[str] = [] + + def __call__( + self, + path: str, + payload: object | None = None, + *, + method: str | None = None, + extra_args: list[str] | None = None, + ) -> subprocess.CompletedProcess[str]: + """Return the requested workflow inventory or run page.""" + assert payload is None + assert method is None + assert extra_args is None + self.calls.append(path) + if path == "repos/owner/repo/actions/workflows?per_page=100&page=1": + return _proc( + { + "total_count": 1, + "workflows": [ + { + "id": 77, + "name": "CI", + "path": ".github/workflows/ci.yml", + } + ], + } + ) + prefix = "repos/owner/repo/actions/workflows/77/runs?branch=main&per_page=100&page=" + if path.startswith(prefix): + page = int(path.removeprefix(prefix)) + return _proc({"total_count": 0, "workflow_runs": self.pages[page - 1]}) + raise AssertionError(f"unexpected Actions REST path: {path}") + + def _fetcher(runs: list[dict[str, Any]]): """Return an injectable run-window fetcher.""" return lambda _repo, _workflow, limit: runs[:limit] +def test_paginated_actions_fetch_reaches_decisive_runs_past_cancellations() -> None: + """A full cancelled page must not be mistaken for the complete evidence window.""" + cancelled_page = [ + _actions_run(_run(1000 + index, "cancelled", f"2026-09-04T00:{index:02d}:00Z")) + for index in range(100) + ] + decisive_page = [ + _actions_run(_run(900, "success", "2026-09-03T23:00:00Z")), + _actions_run(_run(899, "success", "2026-09-03T22:00:00Z")), + ] + fake = FakeActionsRunREST([cancelled_page, decisive_page]) + + runs = reconciler._fetch_main_ci_runs( + REPO, + "CI", + max_pages=2, + runner=fake, + ) + + assert len(runs) == 102 + assert [run["databaseId"] for run in runs[-2:]] == [900, 899] + assert any(path.endswith("page=2") for path in fake.calls) + + +def test_paginated_actions_fetch_fails_closed_at_page_budget() -> None: + """A page ceiling without two decisive runs cannot produce a guessed verdict.""" + cancelled_pages = [ + [ + _actions_run( + _run( + 2000 + page * 100 + index, "cancelled", f"2026-09-{page:02d}T00:{index:02d}:00Z" + ) + ) + for index in range(100) + ] + for page in range(1, 3) + ] + fake = FakeActionsRunREST(cancelled_pages) + + with pytest.raises(reconciler.ReconciliationError, match="two decisive runs"): + reconciler._fetch_main_ci_runs( + REPO, + "CI", + max_pages=2, + runner=fake, + ) + + assert any(path.endswith("page=2") for path in fake.calls) + + def test_parse_deciding_run_requires_one_canonical_field() -> None: """Malformed or duplicated incident fields are pending, never eligible.""" run_id, error = reconciler.parse_deciding_run_id(_body(300), repo=REPO) @@ -330,6 +432,59 @@ def unexpected_fetch(*_args: object) -> list[dict[str, Any]]: assert report["source"]["open_incident_count"] == 0 +def test_explicit_run_page_limit_overrides_legacy_run_limit() -> None: + """The new page budget remains independently controllable for the REST path.""" + fake = FakeREST(_issue(run_id=300)) + fake.issue["state"] = "closed" + + report = reconciler.reconcile_batch( + repo=REPO, + runner=fake, + run_limit=3, + max_run_pages=2, + ) + + assert report["source"]["run_limit"] == 3 + assert report["source"]["run_page_limit"] == 2 + + +def test_default_run_reader_receives_resolved_page_budget( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The batch path routes the new page budget to the default REST reader.""" + fake = FakeREST(_issue(run_id=300)) + observed: dict[str, Any] = {} + + def fake_run_reader( + repo: str, + workflow: str, + *, + max_pages: int, + runner: object, + ) -> list[dict[str, Any]]: + observed.update(repo=repo, workflow=workflow, max_pages=max_pages, runner=runner) + return [ + _run(500, "success", "2026-09-01T02:00:00Z"), + _run(400, "success", "2026-09-01T01:00:00Z"), + _run(300, "failure", "2026-09-01T00:00:00Z"), + ] + + monkeypatch.setattr(reconciler, "_fetch_main_ci_runs", fake_run_reader) + report = reconciler.reconcile_batch( + repo=REPO, + workflow="CI", + runner=fake, + run_limit=7, + max_run_pages=4, + ) + + assert observed["repo"] == REPO + assert observed["workflow"] == "CI" + assert observed["max_pages"] == 4 + assert observed["runner"] is fake + assert report["source"]["run_page_limit"] == 4 + + def test_malformed_run_window_fails_closed_before_any_issue_write() -> None: """A malformed fetched run cannot become evidence or trigger a close.""" fake = FakeREST(_issue(run_id=300))