fix: harden E2E affected discovery, benchmark modes and last-failed writes - #107
Conversation
…rites 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe E2E runner now detects benchmark modes from CLI and environment settings, fails closed when affected-path Git discovery fails, and atomically persists last-failed state. Tests cover Git failures, benchmark isolation, persistence failures, and runner diagnostics. ChangesE2E policy safeguards
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)Affected-test discoverysequenceDiagram
participant E2ERunner
participant E2EPolicy
participant Git
E2ERunner->>E2EPolicy: request affected paths
E2EPolicy->>Git: run validated discovery commands
Git-->>E2EPolicy: return paths or failure
E2EPolicy-->>E2ERunner: return paths or ChangeDiscoveryError
E2ERunner-->>E2ERunner: exit nonzero on discovery failure
Benchmark history handlingsequenceDiagram
participant E2ERunner
participant E2EPolicy
participant HistoryFiles
E2ERunner->>E2EPolicy: resolve active benchmark modes
E2EPolicy-->>E2ERunner: return persistence policy
E2ERunner->>HistoryFiles: skip or save timing and last-failed history
HistoryFiles-->>E2ERunner: return persistence status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 51.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 64 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoHarden E2E discovery, benchmark detection, and history writes
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
CI statusGreen: Ruff bug checks, Pyright data-flow checks, ESLint bug checks, CodeQL (python / javascript-typescript / actions), SonarCloud. Greptile is still running. One red check, and it is not this PR's: That is a backend/model-configuration failure in the scanning agent itself, not a finding against this diff — and it is currently failing the same way on every open PR in this repository (the equivalent runs on #105 and #106 both failed at the same step). There is no fix to port into this branch, and a re-run would hit the same unsupported-model response, so I am standing down on it rather than pushing anything. It should clear on its own once the scanning agent's model configuration is fixed upstream. One thing worth flagging for reviewers: browser E2E was not run for this change. Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba83662078
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses two review findings on the previous commit. save_last_failed() gave every writer the same `<destination>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00c0924a13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 <base>^{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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i
Fooftilly
left a comment
Fooftilly
left a comment
There was a problem hiding this comment.
Reviewed current head 611cc30 after the latest fixes. I do not see an additional code-level blocker in the diff: the earlier fail-open --base cases (option/path/range), env-mode leakage/reporting issues, stale last-failed mutation, and shared temp-file race have been addressed with regression coverage.
One pre-merge validation gap remains: this PR changes tests/e2e/run.py, tests/e2e/harness.py, and tests/e2e/policy.py, and the repository's e2e-framework affected rule maps those paths to the smoke suite. The PR still states that browser E2E was not run, and there is no smoke result/check on current head. Please run the smoke tier against 611cc30 (for example python tests/e2e/run.py --smoke --jobs 2 or scripts/e2e smoke) and post the result before merge.
CI note: the current red github-advanced-security job is not a PR finding. Its log aborts in Copilot autofind with CAPIError: 400 The requested model is not supported; Ruff, Pyright, ESLint, CodeQL, SonarCloud, and CodeRabbit are green.
Browser E2E results on
|
| Run | Result | Duration | Failures |
|---|---|---|---|
611cc30 gate #1 |
709/712 | 1043.9s | test_work_source_offline…servers_own_video_cancels, WorkCreateWorkflowTests…typed_unselected_folder…, OfflineFoundationTests…reconnect_probe_race… |
611cc30 gate #2 |
710/712 | — | a subset of the above two; the work-source one passed |
master (9e75411) gate, same container, same --jobs 4 |
710/712 | 1065.4s | OfflineFoundationTests…failed_then_successful_annotations_snapshot_becomes_editable, OfflineArgumentCoherenceTests…independent_durable_units_survive_one_transport_failure |
Master fails at the same rate, with a disjoint set of failures. Narrowing on this branch:
--last-failed --jobs 1→ all 3 pass, 12.2s total, versus ~95s for the same three under 4-way load- the five owning feature groups (
work-detail,sync,work-create,offline,pdf-annotations, 177 tests) → PASS at--jobs 2in 448s
Every failure across all three gate runs lands in the slow, timing-sensitive offline/race family, the set shifts run to run, and the gate takes ~1050s here against the documented ~7–10 min expectation. That is this container being slow under 4 workers, not a defect in the diff — and it is not attributable to these changes on the merits either: the discovery code runs only under --affected, the persistence code runs after tests complete, and the only changed code in the test path (harness._env_enabled delegating to policy.env_flag_enabled) is logically identical to what it replaced.
I am not proposing to fix those tests here — they are pre-existing timing sensitivity under parallel load, outside this PR's scope, and master shows the same behavior. Worth a separate look if they also flake on your machine or in CI.
Incidental confirmation of the fixes
The gate → --last-failed → clear cycle exercised exactly the machinery this PR hardens, on real runs: gate #1 wrote 3 unresolved failures atomically (Wrote last-failed (3)), the --jobs 1 rerun consumed them, and the state cleared once they passed (Cleared last-failed (all previously failed tests resolved)).
CI note
Agreed on github-advanced-security — same reading as yours, and it reproduces identically on #105/#106, so there is nothing in this branch to fix for it.
No code changes since 611cc30; the diff is untouched by this validation work.
Generated by Claude Code
Fooftilly
left a comment
Fooftilly
left a comment
There was a problem hiding this comment.
Re-reviewed current head 611cc30 after the browser validation was added.
The previous pre-merge gap is closed: the required e2e-framework smoke tier passed 9/9 on real pinned Chromium, and the broader owning feature groups also passed. The full --jobs 4 gate failures do not look attributable to this diff: the same container reproduces comparable failures on master, with a disjoint timing-sensitive offline/race failure set, while the branch failures pass when narrowed/serialized.
I also checked current master, which has advanced six commits since this PR's merge base. Those commits are README/wiki/documentation work plus documentation-enforcement unit-test changes; they do not touch tests/e2e/run.py, tests/e2e/harness.py, tests/e2e/policy.py, or production runtime. AGENTS.md is the only overlapping file and GitHub reports the PR mergeable, so I do not see a substantive need to rebase solely for those commits.
No new code-level findings on this pass. The existing github-advanced-security red check remains the unrelated Copilot autofind 400 The requested model is not supported infrastructure failure. From review/validation evidence, I consider #107 merge-ready.
2 participants
Summary
Three focused E2E runner reliability fixes (EF-006, EF-007, EF-008). All three findings were re-verified against current
master(9e75411) before implementing — each reproduces exactly as described, and none was already fixed or materially changed.--affectednow fails closed when Git change discovery fails or when--basewould make git answer a different question, instead of reporting "zero affected tests" with exit 0No E2E selection semantics changed beyond the fail-closed behavior the first finding requires: a genuine empty diff is still a successful no-op, and every mapping rule is untouched.
Failure modes fixed
#72 — silent fail-open in change discovery.
policy.list_changed_paths()swallowedCalledProcessError/FileNotFoundErrorfrom all three Git queries (git diff … <base>, the secondary local diff,git ls-files --others) and substituted an empty result. An invalid--baseref, a missing or failinggit, a damaged checkout, or a failing untracked query therefore produced an empty changed-path list, whichselect_affected()classifies asnoop_ok— so the runner printed "success no-op" and exited 0 while skipping all regression coverage.The three queries now go through one
_git_lines()helper that raisesChangeDiscoveryErrorcarrying the failing command, git's exit code, and git's first stderr line.run.pycatches it before any Chromium install and exits 2:Review surfaced three further fail-open variants of the same bug, each reproduced against a real repository before fixing.
--affecteddocuments one comparison — working tree vs one base commit — and git will quietly do something else for all of these:--basevalue--relative=nosuchbackend(an existing path)fatal: bad revision, exit 2release..mainfatal: Needed a single revision, exit 2The range case is the sharpest: with a staged
frontend/app.js,--base otherlists it and--base other..HEADdoes not. An explicit base is now verified withgit rev-parse --verify <base>^{commit}before any diff, which rejects all three classes with git's own wording, and the diffs terminate revision parsing with--. The check is skipped when no--baseis given, so the default path costs nothing extra.Untracked discovery failure is an error only when untracked discovery is enabled. A genuinely empty diff still returns
[]and stays a successful no-op.#73 — env-only benchmark runs contaminated normal history.
_is_benchmark_mode(args)inspected only the parser flags, but the same runtime behavior is reachable throughPRKS_E2E_PROFILEandPRKS_E2E_SEED_CACHE=0, which the harness consumes directly. Setting either without the matching flag leftpersist_historyenabled, so an instrumented or cache-off run trained.tests/e2e-timings.json(poisoning later full-gate LPT shard balancing) and mutated last-failed state.main()now exports its flags into the environment first and then askspolicy.benchmark_modes()once — a single decision over the effective configuration covering both entry paths. The harness reads the same env names through the sharedpolicy.env_flag_enabled(), so one definition decides both how a switch is applied and how it is judged; the duplicated truthiness set inharness.pyis gone. Three consequences of making that decision canonical, all from review:main()restores both variables on every exit path), so a benchmark run cannot leave a later in-process run in benchmark mode;PRKS_E2E_PROFILE=1no longer pays the instrumentation cost and prints nothing;last-failed-stalebranch honors suppression too — unlinking the state file is a history mutation like writing it.#74 — non-atomic last-failed write.
save_last_failed()wrote JSON straight to the destination withPath.write_text(). An interrupted or failing write left a truncated file, and becauseload_last_failed()maps read/JSON errors toNone, corruption was indistinguishable from "no saved failure state" — silently discarding the--last-failedfast loop.It now writes to a uniquely named temp file (
tempfile.mkstemp()) in the destination directory and commits withos.replace()— the atomic-replace patterntests/e2e/sharding.save_timings()already uses, atomic on Windows and POSIX alike, with the unique name so two runners sharing a checkout cannot write into or clean up each other's uncommitted state. A failure before the commit removes that temp file, leaves the previous valid state intact, and returnsFalseso the runner warns on stderr rather than printing a write it did not make.Tests added
All in
tests/test_e2e_policy.py(no Chromium):gitexecutable, failing secondary local diff, failing untracked query, option-like base rejected without git being invoked at all, revision-range base rejected before any diff runs, both diff commands ending in--— each assertsChangeDiscoveryErrorwith a useful message; plus genuine empty diff, normal changes, untracked changes, and untracked failure being irrelevant when discovery is disabled. AListChangedPathsRealGitTestsclass exercises the contract against a realgitprocess (unusableHEADin a repo with no commits, an invalid base ref, a base that names an existing path, a range that demonstrably drops a staged file, and a clean checkout that correctly reports no changes then one changed file), skipped whengitis unavailable.--affectedwith a failing query exits 2, prints the diagnostic, and never installs Chromium or starts a run.benchmark_modes()over CLI-equivalent and env-only configurations; a check that the harness and the runner share one truthiness definition; runner regression tests thatPRKS_E2E_PROFILE=1,PRKS_E2E_SEED_CACHE=0and both together leave timing and last-failed files byte-identical whilePRKS_E2E_SEED_CACHE=1(the representative default) still persists, that a--profilerun does not suppress the next in-process run's persistence, that env-only profiling prints the infrastructure report, and that a benchmark--last-failedrun leaves a fully stale state file byte-identical.The two existing
ListChangedPathsTestscases were updated fromsubprocess.check_outputtosubprocess.runmocks; their assertions are unchanged.Each new regression test was mutation-checked against the old implementation to confirm it actually fails without the fix.
Validation
Run with a project-local
.venvon Python 3.12:python run_tests.py— 2153 tests, OK (includes the 77test_e2e_policytests andtest_e2e_sharding, which drives real runner worker subprocesses)ruff check --config ruff.toml prks_app.py backend— passed (the CI static-analysis scope);ruffover the changed files also passes--affected --list-testsselects smoke as expected and exits 0,--base HEAD~1works;--base origin/nope,--base=--relative=nosuch,--base backendand--base master..HEADeach exit 2 with a specific diagnosticprks_app.py,backend,frontend,tests/browser— none of the changed files fall in scopeBrowser E2E on
611cc30(full results and diagnosis in this comment):--jobs 2 --no-pointer-capture. This is the tier thee2e-frameworkaffected rule maps the three changed paths to.work-detail,sync,work-create,offline,pdf-annotations, 177 tests): PASS at--jobs 2in 448s.--jobs 4: red on this container for both this branch and master — 709/712 then 710/712 here, 710/712 onmaster(9e75411) with a disjoint failure set, every failure in the slow timing-sensitive offline/race family, ~1050s per run against the documented ~7–10 min. All of this branch's gate failures pass individually (--last-failed --jobs 1, 12.2s for the three). Pre-existing parallel-load timing sensitivity, not this diff, and not fixed here.Scope
Limited to E2E runner reliability. No unrelated test infrastructure was refactored; the only shared helper introduced is the small env-truthiness function the benchmark-mode fix requires. Documentation updated in
AGENTS.md(--affecteddefaults) anddocs/e2e-performance.md(env equivalence), plus the--profile/--no-seed-cacheCLI help text.One review suggestion was deliberately not taken: a cross-process lock serializing last-failed
load → merge → save. Rationale is in that thread — concurrent runners in one checkout already contend over the timing file, seed cache and port window, so it belongs in its own change rather than this one.Closes #72
Closes #73
Closes #74
🤖 Generated with Claude Code
https://claude.ai/code/session_019ptPms6cxXvRV4hRFLKp7i