From ba836620780725e3c01ea84d1784f995e2458a98 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 21:18:00 +0000 Subject: [PATCH 1/4] fix: harden E2E affected discovery, benchmark modes and last-failed writes Three E2E runner reliability fixes (EF-006, EF-007, EF-008). --affected fails closed (#72). list_changed_paths() turned every Git failure into an empty changed-path list, so an invalid --base ref, a missing/failing git, a damaged checkout or a failing untracked query became "zero affected tests" with exit 0. All three queries now run through _git_lines(), which raises ChangeDiscoveryError with the failing command and git's first stderr line; the runner exits 2 with that diagnostic. A genuine empty diff still returns [] and stays a successful no-op. Benchmark detection follows the effective configuration (#73). _is_benchmark_mode(args) only looked at --profile / --no-seed-cache, so a run configured through PRKS_E2E_PROFILE or PRKS_E2E_SEED_CACHE=0 alone still trained .tests/e2e-timings.json and mutated last-failed state. main() now exports its flags into the environment and asks policy.benchmark_modes() once; the harness reads the same env names through the shared policy.env_flag_enabled(), so one definition decides how a switch is read and how it is judged. Active benchmark modes are printed with the suppression notice. last-failed state is written atomically (#74). save_last_failed() wrote straight to the destination, so an interrupted or failing write left a truncated file that load_last_failed() reports as "no saved failures". It now uses the temp-file + os.replace commit already used for timing history (cross-platform), removes the uncommitted temp file, and returns False so the runner warns instead of silently claiming a write. Tests: fail-closed cases for invalid base ref, missing git, failing local and untracked queries (mock-based plus a real-git class covering an unusable HEAD, a clean checkout and a modified file), env-driven and CLI-driven benchmark configurations, atomic round trip, and preservation of the previous valid file when the temp write or the replace fails. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i --- AGENTS.md | 10 + docs/e2e-performance.md | 7 + tests/e2e/harness.py | 16 +- tests/e2e/policy.py | 165 ++++++++++----- tests/e2e/run.py | 62 ++++-- tests/test_e2e_policy.py | 420 ++++++++++++++++++++++++++++++++++----- 6 files changed, 565 insertions(+), 115 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1522b313..7ecf5f54 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2044,6 +2044,16 @@ paths select nothing. Add new production areas by editing `AFFECTED_RULES` and `FEATURES` in `tests/e2e/policy.py`. Classify a new E2E module by adding its module/class prefix to the right feature's `selectors`. +`--affected` fails closed: when Git change discovery itself fails (invalid +`--base` ref, unusable checkout, missing/failing `git`, or untracked-file +discovery failure) the runner exits nonzero with a diagnostic instead of +reporting zero affected tests. A genuine empty diff remains a successful no-op. + +Benchmark/profile runs never train history. `--profile` / `--no-seed-cache` +**and** their environment equivalents (`PRKS_E2E_PROFILE`, +`PRKS_E2E_SEED_CACHE=0`) suppress `.tests/e2e-timings.json` and last-failed +persistence; `policy.benchmark_modes()` is the single decision. + Optional stress: `tests/e2e/stress_cache_offline.py` — never part of normal iteration or the full gate. diff --git a/docs/e2e-performance.md b/docs/e2e-performance.md index 48d5c923..9401c9bf 100644 --- a/docs/e2e-performance.md +++ b/docs/e2e-performance.md @@ -94,6 +94,13 @@ parallel run to a serial run when judging an individual-test optimization. instrumented or cache-off durations into them would poison later normal runs. Profile/slowest output for the current run still prints as usual. +The same applies when the run is configured through the environment instead of +the flags: `PRKS_E2E_PROFILE=1` or `PRKS_E2E_SEED_CACHE=0` make the run +non-representative on their own, so the runner suppresses history persistence +for them too and prints which benchmark mode is active. `tests/e2e/policy.py` +(`benchmark_modes()`) makes that decision once, from the effective +configuration after CLI flags are exported into the environment. + ### Opt-in hang diagnostics and Chromium recycle `PRKS_E2E_DIAGNOSTIC=1` prints privacy-safe stage heartbeats (`START`, diff --git a/tests/e2e/harness.py b/tests/e2e/harness.py index 77783806..fe36f4ce 100644 --- a/tests/e2e/harness.py +++ b/tests/e2e/harness.py @@ -29,29 +29,31 @@ installed_chromium_revisions, playwright_chromium_revision, ) +from tests.e2e.policy import ( + PROFILE_ENV, + SEED_CACHE_ENV, + env_flag_enabled, +) REPO = Path(__file__).resolve().parents[2] HOST = "127.0.0.1" READY_TIMEOUT_S = 25.0 STOP_TIMEOUT_S = 8.0 -_FALSE_ENV_VALUES = {"0", "false", "no", "off"} _ACTIVE_PROFILE = None _SEED_CACHE_TMP = None _SEED_SNAPSHOTS = {} def _env_enabled(name: str, default: bool = False) -> bool: - raw = os.environ.get(name) - if raw is None: - return default - return raw.strip().lower() not in _FALSE_ENV_VALUES + """Shared with the runner's benchmark-mode decision (tests.e2e.policy).""" + return env_flag_enabled(name, default) def start_test_profile(test_id: str) -> None: """Begin opt-in per-test infrastructure profiling for the runner.""" global _ACTIVE_PROFILE - if not _env_enabled("PRKS_E2E_PROFILE"): + if not _env_enabled(PROFILE_ENV): _ACTIVE_PROFILE = None return _ACTIVE_PROFILE = {"test_id": test_id, "phases": {}} @@ -81,7 +83,7 @@ def finish_test_profile(test_id: str) -> dict: def seed_cache_enabled() -> bool: """Worker-local immutable seed snapshots are on unless explicitly disabled.""" - return _env_enabled("PRKS_E2E_SEED_CACHE", default=True) + return _env_enabled(SEED_CACHE_ENV, default=True) def diagnostic_enabled() -> bool: diff --git a/tests/e2e/policy.py b/tests/e2e/policy.py index 45b7457d..3ecde089 100644 --- a/tests/e2e/policy.py +++ b/tests/e2e/policy.py @@ -1,6 +1,7 @@ """Declarative E2E selection: tiers, feature groups, affected-file mapping. -Pure logic — no Playwright, no subprocesses. Covered by +Pure selection logic — no Playwright, no browser; the only subprocesses are +read-only Git queries for ``--affected`` discovery. Covered by `tests/test_e2e_policy.py`. The runner (`tests/e2e/run.py`) is the only entry point that applies these selections to a real Chromium run. """ @@ -13,6 +14,52 @@ LAST_FAILED_PATH = Path(".tests") / "e2e-last-failed.json" +# --------------------------------------------------------------------------- +# Effective runtime configuration (CLI flags and the equivalent environment) +# --------------------------------------------------------------------------- + +# Runner/harness env switches. The harness reads them to configure a run; the +# runner reads the same names to decide whether a run may train history. +PROFILE_ENV = "PRKS_E2E_PROFILE" +SEED_CACHE_ENV = "PRKS_E2E_SEED_CACHE" + +FALSE_ENV_VALUES = frozenset({"0", "false", "no", "off"}) + + +def env_flag_enabled(name: str, default: bool = False, environ=None) -> bool: + """Canonical truthiness for PRKS E2E env switches (unset → default).""" + env = os.environ if environ is None else environ + raw = env.get(name) + if raw is None: + return default + return raw.strip().lower() not in FALSE_ENV_VALUES + + +def benchmark_modes(environ=None) -> tuple: + """Active non-representative modes for the *effective* configuration. + + ``--profile`` adds instrumentation overhead and ``--no-seed-cache`` measures + a slower non-default configuration; both are also reachable through + ``PRKS_E2E_PROFILE`` / ``PRKS_E2E_SEED_CACHE`` without the matching flag. + The runner exports its flags into the environment before asking, so this is + the single decision covering CLI- and environment-driven benchmark runs. + """ + modes = [] + if env_flag_enabled(PROFILE_ENV, environ=environ): + modes.append("profile") + if not env_flag_enabled(SEED_CACHE_ENV, default=True, environ=environ): + modes.append("no-seed-cache") + return tuple(modes) + + +class ChangeDiscoveryError(RuntimeError): + """Git could not report the working-tree changes ``--affected`` needs. + + Distinct from "Git reported no changes": the first must fail closed, the + second is an ordinary successful no-op. + """ + + # --------------------------------------------------------------------------- # Tiers # --------------------------------------------------------------------------- @@ -770,6 +817,35 @@ def select_affected(all_ids, changed_paths): ) +def _git_lines(repo: Path, args, what: str) -> list: + """Run one read-only git command; raise ChangeDiscoveryError on any failure. + + Git failures (missing executable, damaged checkout, invalid base ref) must + never be indistinguishable from "no changes" — see fail-closed note on + ``list_changed_paths``. + """ + cmd = ["git", "-C", str(repo), *args] + try: + proc = subprocess.run(cmd, capture_output=True, text=True, check=False) + except OSError as exc: + raise ChangeDiscoveryError( + "%s failed: could not run `git %s` (%s)" + % (what, " ".join(args), exc.__class__.__name__) + ) from exc + if proc.returncode != 0: + detail = [line.strip() for line in (proc.stderr or "").splitlines() if line.strip()] + raise ChangeDiscoveryError( + "%s failed: `git %s` exited %d%s" + % ( + what, + " ".join(args), + proc.returncode, + (" — " + detail[0]) if detail else "", + ) + ) + return [line.strip() for line in proc.stdout.splitlines() if line.strip()] + + def list_changed_paths(repo: Path, base: str | None = None, include_untracked=True): """Working-tree changes vs base (default: HEAD). Explicit --base overrides. @@ -777,57 +853,38 @@ def list_changed_paths(repo: Path, base: str | None = None, include_untracked=Tr against HEAD (or against `base` when provided). Includes Added/Copied/ Modified/Renamed/Deleted (D). Also includes untracked files under frontend/, backend/, tests/e2e/, tools/, scripts/ when include_untracked. + + Fails closed: any Git/change-discovery failure raises ChangeDiscoveryError + instead of degrading to an empty (and therefore "nothing affected") list. + A genuinely empty diff still returns []. """ repo = Path(repo) ref = base or "HEAD" paths = [] # Staged + unstaged vs ref — include deletes so removed production/E2E # files still drive feature selection. - cmd = ["git", "-C", str(repo), "diff", "--name-only", "--diff-filter=ACMRD", ref] - try: - out = subprocess.check_output(cmd, text=True, stderr=subprocess.DEVNULL) - except (subprocess.CalledProcessError, FileNotFoundError): - out = "" - for line in out.splitlines(): - line = line.strip() - if line: - paths.append(line) + for line in _git_lines( + repo, + ["diff", "--name-only", "--diff-filter=ACMRD", ref], + "change discovery vs %s" % ref, + ): + paths.append(line) # Also include staged-only relative to HEAD when base is HEAD — already covered # by diff HEAD. When base is another ref, also include uncommitted local work: if base and base != "HEAD": - try: - local = subprocess.check_output( - [ - "git", - "-C", - str(repo), - "diff", - "--name-only", - "--diff-filter=ACMRD", - "HEAD", - ], - text=True, - stderr=subprocess.DEVNULL, - ) - for line in local.splitlines(): - line = line.strip() - if line and line not in paths: - paths.append(line) - except (subprocess.CalledProcessError, FileNotFoundError): - pass + for line in _git_lines( + repo, + ["diff", "--name-only", "--diff-filter=ACMRD", "HEAD"], + "local change discovery vs HEAD", + ): + if line not in paths: + paths.append(line) if include_untracked: - try: - untracked = subprocess.check_output( - ["git", "-C", str(repo), "ls-files", "--others", "--exclude-standard"], - text=True, - stderr=subprocess.DEVNULL, - ) - except (subprocess.CalledProcessError, FileNotFoundError): - untracked = "" - for line in untracked.splitlines(): - line = line.strip() - if not line: - continue + for line in _git_lines( + repo, + ["ls-files", "--others", "--exclude-standard"], + "untracked change discovery", + ): if any( line.startswith(p) or line == p.rstrip("/") for p in UNTRACKED_AFFECTED_PREFIXES @@ -869,14 +926,32 @@ def merge_last_failed(previous_ids, executed_ids, current_failed_ids, known_ids= return merged -def save_last_failed(path: Path, test_ids, meta=None): +def save_last_failed(path: Path, test_ids, meta=None) -> bool: + """Persist unresolved failures atomically; returns False when nothing was written. + + Same temp-file + os.replace commit used for timing history + (tests/e2e/sharding.save_timings): a crash or disk error before the replace + leaves the previous valid file untouched rather than a truncated one that + load_last_failed would report as "no saved failures". + """ path = Path(path) - path.parent.mkdir(parents=True, exist_ok=True) payload = { "test_ids": list(test_ids), "meta": meta or {}, } - path.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n", encoding="utf-8") + tmp = path.with_suffix(path.suffix + ".tmp") + try: + path.parent.mkdir(parents=True, exist_ok=True) + with open(tmp, "w", encoding="utf-8") as handle: + handle.write(json.dumps(payload, indent=2, sort_keys=True) + "\n") + os.replace(tmp, path) + return True + except OSError: + try: + tmp.unlink() + except OSError: + pass + return False def load_last_failed(path: Path): diff --git a/tests/e2e/run.py b/tests/e2e/run.py index 2b6b7485..f59672b2 100644 --- a/tests/e2e/run.py +++ b/tests/e2e/run.py @@ -42,6 +42,10 @@ from tests.e2e.install_browser import ensure_chromium_installed from tests.e2e.policy import ( LAST_FAILED_PATH, + PROFILE_ENV, + SEED_CACHE_ENV, + ChangeDiscoveryError, + benchmark_modes, format_feature_catalog, full_gate_timeout_s, list_changed_paths, @@ -364,14 +368,21 @@ def _persist_timings(observed, known_ids): save_timings(path, merged) -def _is_benchmark_mode(args) -> bool: - """True for non-representative runs that must not train LPT / last-failed history. +def _apply_runtime_modes(args) -> tuple: + """Export CLI benchmark flags, then decide the effective benchmark modes once. - ``--profile`` adds instrumentation overhead; ``--no-seed-cache`` measures a - slower non-default configuration. Persisting either into - ``.tests/e2e-timings.json`` poisons later normal-gate shard balancing. + ``--profile`` / ``--no-seed-cache`` configure the harness through + PRKS_E2E_PROFILE / PRKS_E2E_SEED_CACHE, and those variables are equally + supported on their own. Exporting first and asking the environment after + keeps one canonical decision for both entry paths, so an env-only benchmark + run cannot train ``.tests/e2e-timings.json`` (poisoning later shard + balancing) or mutate last-failed state that ordinary runs rely on. """ - return bool(getattr(args, "profile", False) or getattr(args, "no_seed_cache", False)) + if args.profile: + os.environ[PROFILE_ENV] = "1" + if args.no_seed_cache: + os.environ[SEED_CACHE_ENV] = "0" + return benchmark_modes(os.environ) # --- worker mode --------------------------------------------------------- @@ -894,7 +905,8 @@ def build_parser(): help=( "Measure per-test E2E infrastructure phases (seed build/clone, server startup, " "browser context, app readiness, async waits, request routing, shutdown). " - "Does not update .tests/e2e-timings.json or last-failed history." + "Does not update .tests/e2e-timings.json or last-failed history " + "(same for PRKS_E2E_PROFILE=1 without this flag)." ), ) parser.add_argument( @@ -902,7 +914,8 @@ def build_parser(): action="store_true", help=( "Disable worker-local immutable fixture seed snapshots for A/B benchmarking. " - "Does not update .tests/e2e-timings.json or last-failed history." + "Does not update .tests/e2e-timings.json or last-failed history " + "(same for PRKS_E2E_SEED_CACHE=0 without this flag)." ), ) # Internal: how the parent invokes one shard. @@ -1028,10 +1041,7 @@ def _resolve_selection(args, all_ids): def main(argv=None) -> int: args = build_parser().parse_args(sys.argv[1:] if argv is None else argv) - if args.profile: - os.environ["PRKS_E2E_PROFILE"] = "1" - if args.no_seed_cache: - os.environ["PRKS_E2E_SEED_CACHE"] = "0" + active_benchmark_modes = _apply_runtime_modes(args) if args.worker_index is not None: return run_worker( @@ -1048,6 +1058,16 @@ def main(argv=None) -> int: all_ids = discover_test_ids() try: tier, test_ids, note = _resolve_selection(args, all_ids) + except ChangeDiscoveryError as exc: + # Fail closed: an unusable Git comparison is not "nothing changed". + print("affected: %s" % exc, file=sys.stderr) + print( + "refusing to report zero affected tests from failed change discovery; " + "check --base/the checkout, or select tests explicitly with " + "--smoke / --feature.", + file=sys.stderr, + ) + return 2 except ValueError as exc: print(str(exc), file=sys.stderr) return 2 @@ -1176,7 +1196,12 @@ def main(argv=None) -> int: targeted = tier != "full" # Benchmark modes still print profile / slowest for this run, but must not # train LPT history or mutate last-failed — those files drive ordinary gates. - persist_history = not _is_benchmark_mode(args) + persist_history = not active_benchmark_modes + if active_benchmark_modes: + print( + "benchmark mode (%s): not persisting timing or last-failed history" + % ",".join(active_benchmark_modes) + ) if persist_history: _persist_timings(observed, test_ids if not targeted else None) _print_slowest({**timings, **observed} if targeted else observed) @@ -1192,7 +1217,7 @@ def main(argv=None) -> int: ) last_failed_path = REPO / LAST_FAILED_PATH if unresolved: - save_last_failed( + written = save_last_failed( last_failed_path, unresolved, meta={ @@ -1203,7 +1228,14 @@ def main(argv=None) -> int: "failed_this_run": len(failed_ids), }, ) - print("Wrote last-failed (%d) → %s" % (len(unresolved), LAST_FAILED_PATH)) + if written: + print("Wrote last-failed (%d) → %s" % (len(unresolved), LAST_FAILED_PATH)) + else: + # Atomic write never committed: any previous state is still valid. + print( + "warning: could not write last-failed state → %s" % LAST_FAILED_PATH, + file=sys.stderr, + ) elif last_failed_path.is_file(): try: last_failed_path.unlink() diff --git a/tests/test_e2e_policy.py b/tests/test_e2e_policy.py index a8d964c9..bf8bbf9f 100644 --- a/tests/test_e2e_policy.py +++ b/tests/test_e2e_policy.py @@ -1,8 +1,10 @@ """Unit coverage for tests.e2e.policy — no Chromium.""" from __future__ import annotations +import contextlib import json import os +import shutil import signal import subprocess import sys @@ -246,6 +248,48 @@ def test_round_trip(self): self.assertEqual(data["test_ids"], ["a.b.C.test_x"]) self.assertEqual(data["meta"]["tier"], "feature") + def test_save_commits_atomically_and_leaves_no_temp_file(self): + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "nested" / "e2e-last-failed.json" + self.assertTrue(policy.save_last_failed(path, ["a.b.C.test_x"])) + self.assertEqual(policy.load_last_failed(path)["test_ids"], ["a.b.C.test_x"]) + self.assertEqual( + sorted(q.name for q in path.parent.iterdir()), + ["e2e-last-failed.json"], + ) + + def test_failed_commit_preserves_previous_valid_state(self): + """An interrupted write must never destroy usable last-failed state.""" + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + policy.save_last_failed(path, ["a.b.C.test_keep"], meta={"tier": "full"}) + before = path.read_bytes() + + with mock.patch("os.replace", side_effect=OSError("boom")): + self.assertFalse(policy.save_last_failed(path, ["a.b.C.test_new"])) + self.assertEqual(path.read_bytes(), before) + self.assertEqual( + policy.load_last_failed(path)["test_ids"], ["a.b.C.test_keep"] + ) + # The uncommitted temp file must not linger next to the real state. + self.assertEqual( + sorted(q.name for q in path.parent.iterdir()), + ["e2e-last-failed.json"], + ) + + def test_failed_temp_write_preserves_previous_valid_state(self): + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + policy.save_last_failed(path, ["a.b.C.test_keep"]) + before = path.read_bytes() + + with mock.patch("builtins.open", side_effect=OSError("no space left")): + self.assertFalse(policy.save_last_failed(path, ["a.b.C.test_new"])) + self.assertEqual(path.read_bytes(), before) + self.assertEqual( + policy.load_last_failed(path)["test_ids"], ["a.b.C.test_keep"] + ) + def test_corrupt_or_missing_returns_none(self): self.assertIsNone(policy.load_last_failed(Path("/no/such/file.json"))) with tempfile.TemporaryDirectory() as raw: @@ -312,13 +356,21 @@ def test_glob_double_star(self): self.assertFalse(policy._path_matches("backend/server.py", "backend/work_tag_sync.py")) +def _git_ok(stdout: str = "") -> subprocess.CompletedProcess: + return subprocess.CompletedProcess(args=["git"], returncode=0, stdout=stdout, stderr="") + + +def _git_fail(stderr: str, code: int = 128) -> subprocess.CompletedProcess: + return subprocess.CompletedProcess(args=["git"], returncode=code, stdout="", stderr=stderr) + + class ListChangedPathsTests(unittest.TestCase): def test_invokes_git_diff_against_base_including_deletes(self): - with mock.patch("subprocess.check_output") as check: - check.side_effect = [ - "frontend/js/app.js\nfrontend/js/gone.js\n", # diff vs base (incl D) - "frontend/js/app.js\nbackend/x.py\n", # local vs HEAD when base != HEAD - "scripts/e2e\nfrontend/js/new.js\n", # untracked + with mock.patch("subprocess.run") as run: + run.side_effect = [ + _git_ok("frontend/js/app.js\nfrontend/js/gone.js\n"), # diff vs base (incl D) + _git_ok("frontend/js/app.js\nbackend/x.py\n"), # local vs HEAD when base != HEAD + _git_ok("scripts/e2e\nfrontend/js/new.js\n"), # untracked ] paths = policy.list_changed_paths( Path("/tmp/repo"), base="origin/master", include_untracked=True @@ -329,14 +381,14 @@ def test_invokes_git_diff_against_base_including_deletes(self): self.assertIn("frontend/js/new.js", paths) self.assertIn("scripts/e2e", paths) # Diff filter must include Deleted (D). - first_cmd = check.call_args_list[0][0][0] + first_cmd = run.call_args_list[0][0][0] self.assertIn("--diff-filter=ACMRD", first_cmd) def test_untracked_scripts_and_e2e_policy_are_discoverable(self): - with mock.patch("subprocess.check_output") as check: - check.side_effect = [ - "", # diff vs HEAD - "scripts/e2e\ntests/e2e/policy.py\ntmp/scratch.txt\n", + with mock.patch("subprocess.run") as run: + run.side_effect = [ + _git_ok(""), # diff vs HEAD + _git_ok("scripts/e2e\ntests/e2e/policy.py\ntmp/scratch.txt\n"), ] paths = policy.list_changed_paths( Path("/tmp/repo"), base=None, include_untracked=True @@ -345,6 +397,140 @@ def test_untracked_scripts_and_e2e_policy_are_discoverable(self): self.assertIn("tests/e2e/policy.py", paths) self.assertNotIn("tmp/scratch.txt", paths) + def test_genuine_empty_diff_is_not_an_error(self): + """No changes must stay an ordinary empty result — not a discovery failure.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok(""), _git_ok("")] + self.assertEqual( + policy.list_changed_paths(Path("/tmp/repo"), include_untracked=True), [] + ) + + def test_invalid_base_ref_fails_closed(self): + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_fail("fatal: bad revision 'origin/nope'")] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), base="origin/nope") + message = str(ctx.exception) + self.assertIn("origin/nope", message) + self.assertIn("bad revision", message) + + def test_missing_git_executable_fails_closed(self): + with mock.patch("subprocess.run", side_effect=FileNotFoundError("git")): + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo")) + self.assertIn("FileNotFoundError", str(ctx.exception)) + + def test_local_diff_failure_against_explicit_base_fails_closed(self): + """The secondary working-tree diff is required too — never silently skipped.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [ + _git_ok("frontend/js/app.js\n"), + _git_fail("fatal: not a git repository"), + ] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), base="origin/master") + self.assertIn("local change discovery", str(ctx.exception)) + + def test_untracked_discovery_failure_fails_closed(self): + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok(""), _git_fail("fatal: unable to read index")] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), include_untracked=True) + self.assertIn("untracked change discovery", str(ctx.exception)) + + def test_untracked_failure_is_irrelevant_when_discovery_is_disabled(self): + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok("backend/server.py\n")] + self.assertEqual( + policy.list_changed_paths(Path("/tmp/repo"), include_untracked=False), + ["backend/server.py"], + ) + + +@unittest.skipUnless(shutil.which("git"), "git executable not available") +class ListChangedPathsRealGitTests(unittest.TestCase): + """The fail-closed contract against a real git process, not just mocks.""" + + def _git(self, repo, *args): + subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ) + + def test_unusable_head_fails_closed(self): + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + self._git(repo, "init", "--quiet") + # No commit yet: HEAD cannot be resolved, so discovery must not + # report "nothing changed". + with self.assertRaises(policy.ChangeDiscoveryError): + policy.list_changed_paths(repo) + + def test_clean_checkout_reports_no_changes(self): + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + self._git(repo, "init", "--quiet") + (repo / "backend").mkdir() + (repo / "backend" / "server.py").write_text("x = 1\n", encoding="utf-8") + self._git(repo, "add", "backend/server.py") + self._git( + repo, + "-c", + "user.email=e2e@example.invalid", + "-c", + "user.name=E2E", + "commit", + "--quiet", + "--no-gpg-sign", + "-m", + "seed", + ) + self.assertEqual(policy.list_changed_paths(repo), []) + (repo / "backend" / "server.py").write_text("x = 2\n", encoding="utf-8") + self.assertEqual(policy.list_changed_paths(repo), ["backend/server.py"]) + + def test_invalid_base_ref_fails_closed(self): + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + self._git(repo, "init", "--quiet") + with self.assertRaises(policy.ChangeDiscoveryError): + policy.list_changed_paths(repo, base="origin/definitely-missing") + + +class BenchmarkModeTests(unittest.TestCase): + """Effective benchmark configuration — CLI flags are exported into env first.""" + + def test_default_environment_is_representative(self): + self.assertEqual(policy.benchmark_modes({}), ()) + self.assertEqual(policy.benchmark_modes({"PRKS_E2E_SEED_CACHE": "1"}), ()) + self.assertEqual(policy.benchmark_modes({"PRKS_E2E_PROFILE": "0"}), ()) + + def test_profile_and_disabled_seed_cache_are_benchmark_modes(self): + self.assertEqual(policy.benchmark_modes({"PRKS_E2E_PROFILE": "1"}), ("profile",)) + self.assertEqual( + policy.benchmark_modes({"PRKS_E2E_SEED_CACHE": "off"}), ("no-seed-cache",) + ) + self.assertEqual( + policy.benchmark_modes( + {"PRKS_E2E_PROFILE": "yes", "PRKS_E2E_SEED_CACHE": "false"} + ), + ("profile", "no-seed-cache"), + ) + + def test_env_truthiness_is_shared_with_the_harness(self): + """One definition decides how the harness reads a switch and how the + runner judges the same switch.""" + from tests.e2e import harness + + for raw, enabled in (("1", True), ("true", True), ("0", False), ("off", False)): + self.assertEqual(policy.env_flag_enabled("X", environ={"X": raw}), enabled) + with mock.patch.dict(os.environ, {"X": raw}, clear=False): + self.assertEqual(harness._env_enabled("X"), enabled) + self.assertTrue(policy.env_flag_enabled("X", default=True, environ={})) + self.assertFalse(policy.env_flag_enabled("X", environ={})) + class ReportBannerTests(unittest.TestCase): def test_full_gate_labelled(self): @@ -440,6 +626,43 @@ def test_affected_docs_only_is_success_noop(self): else: os.environ["PRKS_E2E"] = previous + def test_affected_discovery_failure_fails_closed(self): + """A failed Git query must never look like a clean "nothing affected" run.""" + previous = os.environ.get("PRKS_E2E") + os.environ["PRKS_E2E"] = "1" + try: + from tests.e2e import run as runner + import io + from contextlib import redirect_stderr, redirect_stdout + + out = io.StringIO() + err = io.StringIO() + failure = policy.ChangeDiscoveryError( + "change discovery vs origin/nope failed: " + "`git diff --name-only --diff-filter=ACMRD origin/nope` exited 128 " + "— fatal: bad revision 'origin/nope'" + ) + with mock.patch.object(runner, "ensure_chromium_installed") as ensure: + with mock.patch.object(runner, "run_serial") as serial: + with mock.patch.object( + runner, "list_changed_paths", side_effect=failure + ): + with redirect_stdout(out), redirect_stderr(err): + code = runner.main(["--affected", "--base", "origin/nope"]) + self.assertEqual(code, 2) + diagnostic = err.getvalue() + self.assertIn("origin/nope", diagnostic) + self.assertIn("bad revision", diagnostic) + self.assertIn("refusing to report zero affected tests", diagnostic) + self.assertNotIn("success no-op", out.getvalue()) + ensure.assert_not_called() + serial.assert_not_called() + finally: + if previous is None: + os.environ.pop("PRKS_E2E", None) + else: + os.environ["PRKS_E2E"] = previous + def test_fail_fast_last_failed_persists_unexecuted_priors(self): """Runner must merge on observed completions, not the pre-run selection.""" previous_env = os.environ.get("PRKS_E2E") @@ -877,6 +1100,137 @@ def test_last_failed_stale_unlink_failure_skips_cleared_message(self): else: os.environ["PRKS_E2E"] = previous_env + def _invoke_runner_history_case( + self, + repo, + last_path, + test_id, + known_extra=(), + observed=None, + argv_extra=(), + serial_result=None, + ): + """runner.main() over a temp repo with Chromium and the browser run stubbed. + + Returns (exit_code, _print_slowest mock). + """ + from tests.e2e import run as runner + + if observed is None: + observed = {test_id: 99.0} + if serial_result is None: + serial_result = (True, observed, [], {test_id: {"seed_build": 0.01}}) + with contextlib.ExitStack() as stack: + enter = stack.enter_context + enter(mock.patch.object(runner, "REPO", repo)) + enter(mock.patch.object(runner, "LAST_FAILED_PATH", last_path)) + enter(mock.patch.object(runner, "ensure_chromium_installed")) + enter( + mock.patch.object( + runner, "discover_test_ids", return_value=[test_id, *known_extra] + ) + ) + enter(mock.patch.object(runner, "run_serial", return_value=serial_result)) + print_slow = enter(mock.patch.object(runner, "_print_slowest")) + enter(mock.patch.object(runner, "_run_pointer_capture", return_value=0)) + code = runner.main( + [test_id, "--jobs", "1", "--no-pointer-capture", *argv_extra] + ) + return code, print_slow + + def test_env_only_benchmark_modes_do_not_persist_history(self): + """PRKS_E2E_PROFILE / PRKS_E2E_SEED_CACHE=0 are benchmark runs without a flag.""" + from tests.e2e.sharding import load_timings, save_timings + + test_id = "tests.e2e.fake.BenchTests.test_x" + prior_failed = ["tests.e2e.fake.Other.test_keep"] + prior_timings = {test_id: 1.25, prior_failed[0]: 2.0} + observed = {test_id: 99.0} + + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + timings_path = repo / ".tests" / "e2e-timings.json" + last_path = repo / ".tests" / "e2e-last-failed.json" + save_timings(timings_path, prior_timings) + policy.save_last_failed(last_path, prior_failed, meta={"tier": "full"}) + prior_timings_bytes = timings_path.read_bytes() + prior_last_bytes = last_path.read_bytes() + + def _run(env): + timings_path.write_bytes(prior_timings_bytes) + last_path.write_bytes(prior_last_bytes) + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + os.environ.update(env) + return self._invoke_runner_history_case( + repo, + last_path, + test_id, + known_extra=prior_failed, + observed=observed, + ) + + for env in ( + {"PRKS_E2E_PROFILE": "1"}, + {"PRKS_E2E_SEED_CACHE": "0"}, + {"PRKS_E2E_PROFILE": "1", "PRKS_E2E_SEED_CACHE": "0"}, + ): + code, print_slow = _run(env) + self.assertEqual(code, 0) + print_slow.assert_called_once() + self.assertEqual( + timings_path.read_bytes(), + prior_timings_bytes, + "env=%s must leave timing history untouched" % env, + ) + self.assertEqual( + last_path.read_bytes(), + prior_last_bytes, + "env=%s must leave last-failed untouched" % env, + ) + + # An explicitly-default seed cache is a representative run: it persists. + code, _ = _run({"PRKS_E2E_SEED_CACHE": "1"}) + self.assertEqual(code, 0) + self.assertEqual(load_timings(timings_path)[test_id], 99.0) + + def test_failed_last_failed_write_warns_and_keeps_previous_state(self): + """A last-failed write that never commits is reported, not silently lost.""" + import io + from contextlib import redirect_stderr, redirect_stdout + from tests.e2e import run as runner + + test_id = "tests.e2e.fake.BenchTests.test_x" + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + last_path = repo / ".tests" / "e2e-last-failed.json" + policy.save_last_failed(last_path, ["tests.e2e.fake.Other.test_keep"]) + prior_last_bytes = last_path.read_bytes() + + out = io.StringIO() + err = io.StringIO() + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + with mock.patch.object( + runner, "save_last_failed", return_value=False + ) as save: + with redirect_stdout(out), redirect_stderr(err): + code, _ = self._invoke_runner_history_case( + repo, + last_path, + test_id, + serial_result=(False, {test_id: 12.0}, [test_id], {}), + ) + self.assertNotEqual(code, 0) + save.assert_called_once() + self.assertIn("could not write last-failed state", err.getvalue()) + self.assertNotIn("Wrote last-failed", out.getvalue()) + self.assertEqual(last_path.read_bytes(), prior_last_bytes) + def test_benchmark_modes_do_not_persist_timing_or_last_failed_history(self): """--profile / --no-seed-cache must not train LPT timings or last-failed.""" from tests.e2e import run as runner @@ -909,44 +1263,14 @@ def test_benchmark_modes_do_not_persist_timing_or_last_failed_history(self): prior_last_bytes = last_path.read_bytes() def _invoke(extra_flags): - with mock.patch.object(runner, "REPO", repo): - with mock.patch.object(runner, "LAST_FAILED_PATH", last_path): - with mock.patch.object( - runner, "ensure_chromium_installed" - ): - with mock.patch.object( - runner, - "discover_test_ids", - return_value=[test_id] + prior_failed, - ): - with mock.patch.object( - runner, - "run_serial", - return_value=( - True, - observed, - [], - {test_id: {"seed_build": 0.01}}, - ), - ): - with mock.patch.object( - runner, "_print_slowest" - ) as print_slow: - with mock.patch.object( - runner, - "_run_pointer_capture", - return_value=0, - ): - code = runner.main( - [ - test_id, - "--jobs", - "1", - "--no-pointer-capture", - *extra_flags, - ] - ) - return code, print_slow + return self._invoke_runner_history_case( + repo, + last_path, + test_id, + known_extra=prior_failed, + observed=observed, + argv_extra=extra_flags, + ) for flags in ( ["--profile"], From 8e548b64e1b0c36d86f275a78738957324b9e601 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 21:24:51 +0000 Subject: [PATCH 2/4] fix: scope E2E runtime-mode env and give last-failed a unique temp file Addresses two review findings on the previous commit. save_last_failed() gave every writer the same `.tmp` name, so two runners sharing one checkout could write into, commit, or clean up each other's uncommitted state. The temp file now comes from tempfile.mkstemp() in the destination directory, so each writer owns its own; the os.replace() commit and the "previous valid state survives a failed write" contract are unchanged. main() exported the CLI benchmark flags into os.environ and left them there. Workers and the harness need them for the duration of the run, but a later in-process main() without those flags then saw benchmark mode and silently suppressed that run's timing and last-failed persistence. The exports are now restored on every exit path: main() saves and restores the two variables around the run, which lives in _main(). Tests: a failed temp write and an unavailable temp file each preserve the previous valid state, two saves never share a temp name, and a --profile run followed by an ordinary in-process run persists history again (the last one fails without the restore). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i --- tests/e2e/policy.py | 21 ++++++++---- tests/e2e/run.py | 18 ++++++++++ tests/test_e2e_policy.py | 71 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 7 deletions(-) diff --git a/tests/e2e/policy.py b/tests/e2e/policy.py index 3ecde089..14d42af8 100644 --- a/tests/e2e/policy.py +++ b/tests/e2e/policy.py @@ -10,6 +10,7 @@ import json import os import subprocess +import tempfile from pathlib import Path LAST_FAILED_PATH = Path(".tests") / "e2e-last-failed.json" @@ -933,24 +934,32 @@ def save_last_failed(path: Path, test_ids, meta=None) -> bool: (tests/e2e/sharding.save_timings): a crash or disk error before the replace leaves the previous valid file untouched rather than a truncated one that load_last_failed would report as "no saved failures". + + The temp file gets a unique name so two runners sharing one checkout cannot + write into, commit, or clean up each other's uncommitted state. """ path = Path(path) payload = { "test_ids": list(test_ids), "meta": meta or {}, } - tmp = path.with_suffix(path.suffix + ".tmp") + tmp = None try: path.parent.mkdir(parents=True, exist_ok=True) - with open(tmp, "w", encoding="utf-8") as handle: + handle_fd, raw_tmp = tempfile.mkstemp( + dir=str(path.parent), prefix=path.name + ".", suffix=".tmp" + ) + tmp = Path(raw_tmp) + with os.fdopen(handle_fd, "w", encoding="utf-8") as handle: handle.write(json.dumps(payload, indent=2, sort_keys=True) + "\n") os.replace(tmp, path) return True except OSError: - try: - tmp.unlink() - except OSError: - pass + if tmp is not None: + try: + tmp.unlink() + except OSError: + pass return False diff --git a/tests/e2e/run.py b/tests/e2e/run.py index f59672b2..ad2a08b0 100644 --- a/tests/e2e/run.py +++ b/tests/e2e/run.py @@ -1039,6 +1039,24 @@ def _resolve_selection(args, all_ids): def main(argv=None) -> int: + """Entry point; runtime-mode env exports stay scoped to this invocation. + + _apply_runtime_modes() publishes the CLI flags through the environment so + workers and the harness see them, which would otherwise leak into a later + in-process main() and silently suppress that run's history persistence. + """ + saved_modes = {name: os.environ.get(name) for name in (PROFILE_ENV, SEED_CACHE_ENV)} + try: + return _main(argv) + finally: + for name, value in saved_modes.items(): + if value is None: + os.environ.pop(name, None) + else: + os.environ[name] = value + + +def _main(argv=None) -> int: args = build_parser().parse_args(sys.argv[1:] if argv is None else argv) active_benchmark_modes = _apply_runtime_modes(args) diff --git a/tests/test_e2e_policy.py b/tests/test_e2e_policy.py index bf8bbf9f..9699443c 100644 --- a/tests/test_e2e_policy.py +++ b/tests/test_e2e_policy.py @@ -278,17 +278,58 @@ def test_failed_commit_preserves_previous_valid_state(self): ) def test_failed_temp_write_preserves_previous_valid_state(self): + def _fail_after_opening(fd, *args, **kwargs): + os.close(fd) + raise OSError("no space left on device") + with tempfile.TemporaryDirectory() as raw: path = Path(raw) / "e2e-last-failed.json" policy.save_last_failed(path, ["a.b.C.test_keep"]) before = path.read_bytes() - with mock.patch("builtins.open", side_effect=OSError("no space left")): + with mock.patch("os.fdopen", side_effect=_fail_after_opening): self.assertFalse(policy.save_last_failed(path, ["a.b.C.test_new"])) self.assertEqual(path.read_bytes(), before) self.assertEqual( policy.load_last_failed(path)["test_ids"], ["a.b.C.test_keep"] ) + self.assertEqual( + sorted(q.name for q in path.parent.iterdir()), + ["e2e-last-failed.json"], + ) + + def test_unavailable_temp_file_preserves_previous_valid_state(self): + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + policy.save_last_failed(path, ["a.b.C.test_keep"]) + before = path.read_bytes() + + with mock.patch("tempfile.mkstemp", side_effect=OSError("read-only")): + self.assertFalse(policy.save_last_failed(path, ["a.b.C.test_new"])) + self.assertEqual(path.read_bytes(), before) + + def test_concurrent_writers_do_not_share_a_temp_file(self): + """Two runners in one checkout must never write the same uncommitted file.""" + seen = [] + real_mkstemp = tempfile.mkstemp + + def _record(*args, **kwargs): + fd, name = real_mkstemp(*args, **kwargs) + seen.append(name) + return fd, name + + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + with mock.patch("tempfile.mkstemp", side_effect=_record): + self.assertTrue(policy.save_last_failed(path, ["a.b.C.test_one"])) + self.assertTrue(policy.save_last_failed(path, ["a.b.C.test_two"])) + self.assertEqual(len(seen), 2) + self.assertNotEqual(seen[0], seen[1]) + for name in seen: + self.assertEqual(Path(name).parent, path.parent) + self.assertEqual( + policy.load_last_failed(path)["test_ids"], ["a.b.C.test_two"] + ) def test_corrupt_or_missing_returns_none(self): self.assertIsNone(policy.load_last_failed(Path("/no/such/file.json"))) @@ -1196,6 +1237,34 @@ def _run(env): self.assertEqual(code, 0) self.assertEqual(load_timings(timings_path)[test_id], 99.0) + def test_benchmark_flags_do_not_leak_into_a_later_in_process_run(self): + """CLI mode exports are scoped to one main(); the next run persists again.""" + from tests.e2e.sharding import load_timings, save_timings + + test_id = "tests.e2e.fake.BenchTests.test_x" + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + timings_path = repo / ".tests" / "e2e-timings.json" + last_path = repo / ".tests" / "e2e-last-failed.json" + save_timings(timings_path, {test_id: 1.25}) + + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + + code, _ = self._invoke_runner_history_case( + repo, last_path, test_id, argv_extra=["--profile"] + ) + self.assertEqual(code, 0) + self.assertEqual(load_timings(timings_path)[test_id], 1.25) + # The benchmark run must not leave the process in benchmark mode. + self.assertEqual(policy.benchmark_modes(os.environ), ()) + + code, _ = self._invoke_runner_history_case(repo, last_path, test_id) + self.assertEqual(code, 0) + self.assertEqual(load_timings(timings_path)[test_id], 99.0) + def test_failed_last_failed_write_warns_and_keeps_previous_state(self): """A last-failed write that never commits is reported, not silently lost.""" import io From 00c0924a13b3a8a027f52836910191cffe8fdcfb Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 21:29:31 +0000 Subject: [PATCH 3/4] fix: reject non-revision --base and honor benchmark mode consistently Addresses three more review findings. An option-like or path-like --base was a remaining fail-open path. `--base=--relative=nope` is parsed by git as an option and `--base backend` as a pathspec; both exit 0 with no paths, so --affected reported a successful no-op while the requested comparison had changes. A base starting with "-" is now rejected before git runs, and both diff commands terminate revision parsing with "--" so a base that happens to be an existing path fails closed with git's own diagnostic. The infrastructure-profile report was printed only for --profile, so PRKS_E2E_PROFILE=1 paid the profiling overhead and printed nothing. It now keys off the effective benchmark mode, like the persistence decision. The last-failed-stale branch unlinked the state file before reaching the persistence guard, so --last-failed with a benchmark flag mutated last-failed state after all. Benchmark runs now leave the stale file alone and say so. Tests: option-like base rejected without running git, both diff commands ending in "--", a real-git base that is a path failing closed, env-only profiling printing the report, and a benchmark --last-failed run leaving a fully stale state file byte-identical. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i --- AGENTS.md | 3 ++ docs/e2e-performance.md | 4 +- tests/e2e/policy.py | 14 +++++- tests/e2e/run.py | 9 +++- tests/test_e2e_policy.py | 101 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7ecf5f54..757bd928 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2048,6 +2048,9 @@ its module/class prefix to the right feature's `selectors`. `--base` ref, unusable checkout, missing/failing `git`, or untracked-file discovery failure) the runner exits nonzero with a diagnostic instead of reporting zero affected tests. A genuine empty diff remains a successful no-op. +`--base` must name a revision: a leading `-` is rejected and the diff +terminates revision parsing with `--`, so an option-like or path-like base +cannot quietly produce an empty selection. Benchmark/profile runs never train history. `--profile` / `--no-seed-cache` **and** their environment equivalents (`PRKS_E2E_PROFILE`, diff --git a/docs/e2e-performance.md b/docs/e2e-performance.md index 9401c9bf..f22f3287 100644 --- a/docs/e2e-performance.md +++ b/docs/e2e-performance.md @@ -99,7 +99,9 @@ the flags: `PRKS_E2E_PROFILE=1` or `PRKS_E2E_SEED_CACHE=0` make the run non-representative on their own, so the runner suppresses history persistence for them too and prints which benchmark mode is active. `tests/e2e/policy.py` (`benchmark_modes()`) makes that decision once, from the effective -configuration after CLI flags are exported into the environment. +configuration after CLI flags are exported into the environment, and the +infrastructure-profile report prints for `PRKS_E2E_PROFILE=1` exactly as it +does for `--profile`. Those exports last for the one runner invocation. ### Opt-in hang diagnostics and Chromium recycle diff --git a/tests/e2e/policy.py b/tests/e2e/policy.py index 14d42af8..333ca958 100644 --- a/tests/e2e/policy.py +++ b/tests/e2e/policy.py @@ -858,15 +858,25 @@ def list_changed_paths(repo: Path, base: str | None = None, include_untracked=Tr Fails closed: any Git/change-discovery failure raises ChangeDiscoveryError instead of degrading to an empty (and therefore "nothing affected") list. A genuinely empty diff still returns []. + + `base` must name a revision. A leading "-" is rejected (git would parse it + as an option), and the "--" terminator stops git from silently reading a + base that happens to be an existing path as a pathspec; both otherwise exit + 0 with no paths, which is the fail-open this guards against. """ repo = Path(repo) + if base is not None and base.startswith("-"): + raise ChangeDiscoveryError( + "invalid --base %r: a revision cannot start with '-' " + "(git would read it as an option and report no changes)" % base + ) ref = base or "HEAD" paths = [] # Staged + unstaged vs ref — include deletes so removed production/E2E # files still drive feature selection. for line in _git_lines( repo, - ["diff", "--name-only", "--diff-filter=ACMRD", ref], + ["diff", "--name-only", "--diff-filter=ACMRD", ref, "--"], "change discovery vs %s" % ref, ): paths.append(line) @@ -875,7 +885,7 @@ def list_changed_paths(repo: Path, base: str | None = None, include_untracked=Tr if base and base != "HEAD": for line in _git_lines( repo, - ["diff", "--name-only", "--diff-filter=ACMRD", "HEAD"], + ["diff", "--name-only", "--diff-filter=ACMRD", "HEAD", "--"], "local change discovery vs HEAD", ): if line not in paths: diff --git a/tests/e2e/run.py b/tests/e2e/run.py index ad2a08b0..a4bcbf95 100644 --- a/tests/e2e/run.py +++ b/tests/e2e/run.py @@ -1116,6 +1116,13 @@ def _main(argv=None) -> int: return 0 if tier == "last-failed-stale": + if active_benchmark_modes: + # Clearing the file is a history mutation like any other. + print( + "last-failed: every persisted failure is stale, but benchmark mode " + "(%s) leaves the state untouched" % ",".join(active_benchmark_modes) + ) + return 0 last_failed_path = REPO / LAST_FAILED_PATH cleared = True if last_failed_path.is_file(): @@ -1184,7 +1191,7 @@ def _main(argv=None) -> int: test_ids, jobs, timings, args.fail_fast ) - if args.profile and phase_timings: + if "profile" in active_benchmark_modes and phase_timings: totals = {} for phases in phase_timings.values(): for name, seconds in phases.items(): diff --git a/tests/test_e2e_policy.py b/tests/test_e2e_policy.py index 9699443c..08e66b81 100644 --- a/tests/test_e2e_policy.py +++ b/tests/test_e2e_policy.py @@ -455,6 +455,24 @@ def test_invalid_base_ref_fails_closed(self): self.assertIn("origin/nope", message) self.assertIn("bad revision", message) + def test_option_like_base_fails_closed_without_running_git(self): + """A leading '-' would be parsed as a git option and report no changes.""" + with mock.patch("subprocess.run") as run: + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths( + Path("/tmp/repo"), base="--relative=definitely-no-such-prefix" + ) + self.assertIn("cannot start with", str(ctx.exception)) + run.assert_not_called() + + def test_diff_commands_terminate_revision_parsing(self): + """`--` stops git reading a base that is also a path as a pathspec.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok(""), _git_ok(""), _git_ok("")] + policy.list_changed_paths(Path("/tmp/repo"), base="origin/master") + for call in run.call_args_list[:2]: + self.assertEqual(call[0][0][-1], "--") + def test_missing_git_executable_fails_closed(self): with mock.patch("subprocess.run", side_effect=FileNotFoundError("git")): with self.assertRaises(policy.ChangeDiscoveryError) as ctx: @@ -539,6 +557,29 @@ def test_invalid_base_ref_fails_closed(self): with self.assertRaises(policy.ChangeDiscoveryError): policy.list_changed_paths(repo, base="origin/definitely-missing") + def test_base_that_is_a_path_fails_closed(self): + """`--base backend` is a pathspec to git, and would diff nothing at all.""" + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + self._git(repo, "init", "--quiet") + (repo / "backend").mkdir() + (repo / "backend" / "server.py").write_text("x = 1\n", encoding="utf-8") + self._git(repo, "add", "backend/server.py") + self._git( + repo, + "-c", + "user.email=e2e@example.invalid", + "-c", + "user.name=E2E", + "commit", + "--quiet", + "--no-gpg-sign", + "-m", + "seed", + ) + with self.assertRaises(policy.ChangeDiscoveryError): + policy.list_changed_paths(repo, base="backend") + class BenchmarkModeTests(unittest.TestCase): """Effective benchmark configuration — CLI flags are exported into env first.""" @@ -1265,6 +1306,66 @@ def test_benchmark_flags_do_not_leak_into_a_later_in_process_run(self): self.assertEqual(code, 0) self.assertEqual(load_timings(timings_path)[test_id], 99.0) + def test_env_only_profiling_still_prints_the_infrastructure_profile(self): + """PRKS_E2E_PROFILE=1 pays the profiling cost, so it must show the report.""" + import io + from contextlib import redirect_stdout + + test_id = "tests.e2e.fake.BenchTests.test_x" + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + last_path = repo / ".tests" / "e2e-last-failed.json" + out = io.StringIO() + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + os.environ["PRKS_E2E_PROFILE"] = "1" + with redirect_stdout(out): + code, _ = self._invoke_runner_history_case(repo, last_path, test_id) + self.assertEqual(code, 0) + printed = out.getvalue() + self.assertIn("E2E infrastructure profile", printed) + self.assertIn("seed_build", printed) + self.assertIn("benchmark mode (profile)", printed) + + def test_benchmark_mode_does_not_clear_stale_last_failed_state(self): + """Pruning the stale file is a history mutation; benchmark runs skip it.""" + import io + from contextlib import redirect_stdout + from tests.e2e import run as runner + + known = ["tests.e2e.live.T.test_ok"] + stale = ["tests.e2e.gone.Old.test_a"] + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + last_path = repo / "e2e-last-failed.json" + policy.save_last_failed(last_path, stale, meta={"tier": "full"}) + before = last_path.read_bytes() + + out = io.StringIO() + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + with contextlib.ExitStack() as stack: + enter = stack.enter_context + enter(mock.patch.object(runner, "REPO", repo)) + enter(mock.patch.object(runner, "LAST_FAILED_PATH", last_path)) + enter( + mock.patch.object( + runner, "discover_test_ids", return_value=known + ) + ) + ensure = enter( + mock.patch.object(runner, "ensure_chromium_installed") + ) + with redirect_stdout(out): + code = runner.main(["--last-failed", "--profile"]) + self.assertEqual(code, 0) + ensure.assert_not_called() + self.assertTrue(last_path.is_file()) + self.assertEqual(last_path.read_bytes(), before) + self.assertIn("leaves the state untouched", out.getvalue()) + def test_failed_last_failed_write_warns_and_keeps_previous_state(self): """A last-failed write that never commits is reported, not silently lost.""" import io From 611cc308143bdda99ea24dd2c3c88d6576e75d24 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 21:36:12 +0000 Subject: [PATCH 4/4] fix: require --affected --base to resolve to a single revision A revision range is the last way a --base could answer a different question than --affected documents. `git diff a..b` is accepted and switches to commit-vs-commit, so the working tree is left out entirely: with a staged frontend/app.js, `--base other` lists it and `--base other..HEAD` does not. The secondary HEAD diff only covers uncommitted work, so committed changes can be dropped and the run can report a no-op. list_changed_paths() now verifies an explicit base with `git rev-parse --verify ^{commit}` before diffing, which rejects ranges, paths and unknown refs alike and carries git's own diagnostic into the ChangeDiscoveryError. Tests: a mocked range base is rejected before any diff runs, and a real git repository confirms the same range that silently drops a staged file now fails closed. The real-git cases share a seeded-repo helper instead of repeating the init/commit block. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i --- AGENTS.md | 8 ++-- tests/e2e/policy.py | 32 +++++++++++-- tests/test_e2e_policy.py | 100 ++++++++++++++++++++++++--------------- 3 files changed, 94 insertions(+), 46 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 757bd928..7033401a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2048,9 +2048,11 @@ its module/class prefix to the right feature's `selectors`. `--base` ref, unusable checkout, missing/failing `git`, or untracked-file discovery failure) the runner exits nonzero with a diagnostic instead of reporting zero affected tests. A genuine empty diff remains a successful no-op. -`--base` must name a revision: a leading `-` is rejected and the diff -terminates revision parsing with `--`, so an option-like or path-like base -cannot quietly produce an empty selection. +`--base` must name a single revision: a leading `-` is rejected, the base is +verified to resolve to one commit (a range like `a..b` compares commit to +commit and would drop the working tree), and the diff terminates revision +parsing with `--`. An option-like, range or path-like base therefore fails +closed instead of quietly answering a different question. Benchmark/profile runs never train history. `--profile` / `--no-seed-cache` **and** their environment equivalents (`PRKS_E2E_PROFILE`, diff --git a/tests/e2e/policy.py b/tests/e2e/policy.py index 333ca958..1c84c2a0 100644 --- a/tests/e2e/policy.py +++ b/tests/e2e/policy.py @@ -847,6 +847,27 @@ def _git_lines(repo: Path, args, what: str) -> list: return [line.strip() for line in proc.stdout.splitlines() if line.strip()] +def _verify_base_revision(repo: Path, base: str) -> None: + """--affected compares the working tree to ONE base commit. + + Git accepts a range (`a..b`) and silently switches `git diff` to + commit-vs-commit, dropping the working tree from the comparison, so a base + that is not a single resolvable revision has to fail closed rather than + answer a different question. + """ + try: + _git_lines( + repo, + ["rev-parse", "--verify", "%s^{commit}" % base], + "base revision check vs %s" % base, + ) + except ChangeDiscoveryError as exc: + raise ChangeDiscoveryError( + "invalid --base %r: not a single revision this repository resolves " + "(a range like 'a..b', a path, or an unknown ref) — %s" % (base, exc) + ) from exc + + def list_changed_paths(repo: Path, base: str | None = None, include_untracked=True): """Working-tree changes vs base (default: HEAD). Explicit --base overrides. @@ -859,10 +880,11 @@ def list_changed_paths(repo: Path, base: str | None = None, include_untracked=Tr instead of degrading to an empty (and therefore "nothing affected") list. A genuinely empty diff still returns []. - `base` must name a revision. A leading "-" is rejected (git would parse it - as an option), and the "--" terminator stops git from silently reading a - base that happens to be an existing path as a pathspec; both otherwise exit - 0 with no paths, which is the fail-open this guards against. + `base` must name a single revision. A leading "-" is rejected (git would + parse it as an option), the base is then verified to resolve to one commit + (a range or a path does not), and the diff terminates revision parsing with + "--". Each of those otherwise answers a different question, or exits 0 with + no paths at all, which is the fail-open this guards against. """ repo = Path(repo) if base is not None and base.startswith("-"): @@ -870,6 +892,8 @@ def list_changed_paths(repo: Path, base: str | None = None, include_untracked=Tr "invalid --base %r: a revision cannot start with '-' " "(git would read it as an option and report no changes)" % base ) + if base is not None and base != "HEAD": + _verify_base_revision(repo, base) ref = base or "HEAD" paths = [] # Staged + unstaged vs ref — include deletes so removed production/E2E diff --git a/tests/test_e2e_policy.py b/tests/test_e2e_policy.py index 08e66b81..da533d01 100644 --- a/tests/test_e2e_policy.py +++ b/tests/test_e2e_policy.py @@ -409,6 +409,7 @@ class ListChangedPathsTests(unittest.TestCase): def test_invokes_git_diff_against_base_including_deletes(self): with mock.patch("subprocess.run") as run: run.side_effect = [ + _git_ok("deadbeef\n"), # rev-parse: base resolves to one commit _git_ok("frontend/js/app.js\nfrontend/js/gone.js\n"), # diff vs base (incl D) _git_ok("frontend/js/app.js\nbackend/x.py\n"), # local vs HEAD when base != HEAD _git_ok("scripts/e2e\nfrontend/js/new.js\n"), # untracked @@ -422,8 +423,9 @@ def test_invokes_git_diff_against_base_including_deletes(self): self.assertIn("frontend/js/new.js", paths) self.assertIn("scripts/e2e", paths) # Diff filter must include Deleted (D). - first_cmd = run.call_args_list[0][0][0] - self.assertIn("--diff-filter=ACMRD", first_cmd) + diff_cmd = run.call_args_list[1][0][0] + self.assertIn("--diff-filter=ACMRD", diff_cmd) + self.assertIn("rev-parse", run.call_args_list[0][0][0]) def test_untracked_scripts_and_e2e_policy_are_discoverable(self): with mock.patch("subprocess.run") as run: @@ -468,10 +470,14 @@ def test_option_like_base_fails_closed_without_running_git(self): def test_diff_commands_terminate_revision_parsing(self): """`--` stops git reading a base that is also a path as a pathspec.""" with mock.patch("subprocess.run") as run: - run.side_effect = [_git_ok(""), _git_ok(""), _git_ok("")] + run.side_effect = [_git_ok("deadbeef\n"), _git_ok(""), _git_ok(""), _git_ok("")] policy.list_changed_paths(Path("/tmp/repo"), base="origin/master") - for call in run.call_args_list[:2]: - self.assertEqual(call[0][0][-1], "--") + diff_calls = [ + call[0][0] for call in run.call_args_list if "diff" in call[0][0] + ] + self.assertEqual(len(diff_calls), 2) + for cmd in diff_calls: + self.assertEqual(cmd[-1], "--") def test_missing_git_executable_fails_closed(self): with mock.patch("subprocess.run", side_effect=FileNotFoundError("git")): @@ -483,6 +489,7 @@ def test_local_diff_failure_against_explicit_base_fails_closed(self): """The secondary working-tree diff is required too — never silently skipped.""" with mock.patch("subprocess.run") as run: run.side_effect = [ + _git_ok("deadbeef\n"), _git_ok("frontend/js/app.js\n"), _git_fail("fatal: not a git repository"), ] @@ -497,6 +504,19 @@ def test_untracked_discovery_failure_fails_closed(self): policy.list_changed_paths(Path("/tmp/repo"), include_untracked=True) self.assertIn("untracked change discovery", str(ctx.exception)) + def test_revision_range_base_fails_closed(self): + """A range switches git diff to commit-vs-commit and drops the working tree.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_fail("fatal: Needed a single revision")] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), base="release..main") + message = str(ctx.exception) + self.assertIn("release..main", message) + self.assertIn("not a single revision", message) + # Rejected before any diff runs. + self.assertEqual(run.call_count, 1) + self.assertIn("rev-parse", run.call_args_list[0][0][0]) + def test_untracked_failure_is_irrelevant_when_discovery_is_disabled(self): with mock.patch("subprocess.run") as run: run.side_effect = [_git_ok("backend/server.py\n")] @@ -518,6 +538,26 @@ def _git(self, repo, *args): text=True, ) + def _seeded_repo(self, repo: Path) -> Path: + """A repo with one commit touching backend/server.py.""" + self._git(repo, "init", "--quiet") + (repo / "backend").mkdir() + (repo / "backend" / "server.py").write_text("x = 1\n", encoding="utf-8") + self._git(repo, "add", "backend/server.py") + self._git( + repo, + "-c", + "user.email=e2e@example.invalid", + "-c", + "user.name=E2E", + "commit", + "--quiet", + "--no-gpg-sign", + "-m", + "seed", + ) + return repo + def test_unusable_head_fails_closed(self): with tempfile.TemporaryDirectory() as raw: repo = Path(raw) @@ -529,23 +569,7 @@ def test_unusable_head_fails_closed(self): def test_clean_checkout_reports_no_changes(self): with tempfile.TemporaryDirectory() as raw: - repo = Path(raw) - self._git(repo, "init", "--quiet") - (repo / "backend").mkdir() - (repo / "backend" / "server.py").write_text("x = 1\n", encoding="utf-8") - self._git(repo, "add", "backend/server.py") - self._git( - repo, - "-c", - "user.email=e2e@example.invalid", - "-c", - "user.name=E2E", - "commit", - "--quiet", - "--no-gpg-sign", - "-m", - "seed", - ) + repo = self._seeded_repo(Path(raw)) self.assertEqual(policy.list_changed_paths(repo), []) (repo / "backend" / "server.py").write_text("x = 2\n", encoding="utf-8") self.assertEqual(policy.list_changed_paths(repo), ["backend/server.py"]) @@ -557,26 +581,24 @@ def test_invalid_base_ref_fails_closed(self): with self.assertRaises(policy.ChangeDiscoveryError): policy.list_changed_paths(repo, base="origin/definitely-missing") + def test_revision_range_base_fails_closed(self): + with tempfile.TemporaryDirectory() as raw: + repo = self._seeded_repo(Path(raw)) + self._git(repo, "branch", "other", "HEAD") + # A range resolves for git, but answers a different question: the + # working tree is left out entirely. + (repo / "frontend").mkdir() + (repo / "frontend" / "app.js").write_text("// x\n", encoding="utf-8") + self._git(repo, "add", "frontend/app.js") + self.assertIn("frontend/app.js", policy.list_changed_paths(repo, base="other")) + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(repo, base="other..HEAD") + self.assertIn("not a single revision", str(ctx.exception)) + def test_base_that_is_a_path_fails_closed(self): """`--base backend` is a pathspec to git, and would diff nothing at all.""" with tempfile.TemporaryDirectory() as raw: - repo = Path(raw) - self._git(repo, "init", "--quiet") - (repo / "backend").mkdir() - (repo / "backend" / "server.py").write_text("x = 1\n", encoding="utf-8") - self._git(repo, "add", "backend/server.py") - self._git( - repo, - "-c", - "user.email=e2e@example.invalid", - "-c", - "user.name=E2E", - "commit", - "--quiet", - "--no-gpg-sign", - "-m", - "seed", - ) + repo = self._seeded_repo(Path(raw)) with self.assertRaises(policy.ChangeDiscoveryError): policy.list_changed_paths(repo, base="backend")