fix: harden E2E affected discovery, benchmark modes and last-failed writes

AGENTS.md

@@ -2044,6 +2044,21 @@ 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.
+`--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`,
+`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.
 

docs/e2e-performance.md

@@ -94,6 +94,15 @@ 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, 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
 
 `PRKS_E2E_DIAGNOSTIC=1` prints privacy-safe stage heartbeats (`START`,

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:

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.
 """
@@ -9,10 +10,57 @@
 import json
 import os
 import subprocess
+import tempfile
 from pathlib import Path
 
 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,64 +818,108 @@ 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 _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.
 
     Default comparison is the agent-normal case: dirty working tree + index
     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 [].
+
+    `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("-"):
+        raise ChangeDiscoveryError(
+            "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
     # 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 +961,40 @@ 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".
+
+    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)
-    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 = None
+    try:
+        path.parent.mkdir(parents=True, exist_ok=True)
+        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:
+        if tmp is not None:
+            try:
+                tmp.unlink()
+            except OSError:
+                pass
+        return False
 
 
 def load_last_failed(path: Path):