PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Correct fix, smaller than the ticket prescribed because the author measured rather than implemented, and the shrink is documented with the falsifying evidence. Nothing here creates debt, so there is no Approve+Follow-Up residual to park.

Peer-Review Opening: Iris — the part I want to name first is the Delta, not the diff. The ticket told you to pipe webServer.stdout/stderr in config; you checked playwright/lib/runner/index.js, found 'pipe' is already the default and 'inherit' is silently dropped, and shipped no config change for that half at all. Reducing a prescribed fix on read-the-source evidence is harder than implementing it, and the comment carries the mechanism so the next person does not re-derive it.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16732; origin/dev state of playwright.config.e2e.mjs; the package.json script surface (only test-e2e exists — no battery script); the existing outputDir literal. Not the PR body as premise.
• Expected Solution Shape: Scope the artifact root per run so serial re-runs stop overwriting each other's trace.zip, without changing solo/film behaviour. It must NOT hardcode a battery-only path as the default, and the legacy path must be preserved byte-identically rather than "equivalently".
• Patch Verdict: Matches. ARTIFACT_ROOT falls back to './test-results/e2e/artifacts' — the exact prior literal, verified character-for-character in the diff, so the unset path is unchanged rather than reconstructed. The battery branch is the only new behaviour.
• Premise Coherence: Coheres with verify-before-assert in its strongest form — the Delta exists because the author falsified the ticket's own prescription against the runner's source and two live runs, then wrote down what the experiment removed.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16732
• Related Graph Nodes: #16508 (directory-guard containment, see note below)
• Origin Session ID: da404637-9dd0-4d84-bdfe-1f6f831a6b41

🔬 Depth Floor

• Challenge: NEO_E2E_RUN_ID is interpolated directly into a filesystem path (./test-results/e2e/battery/${RUN_ID}/artifacts) with no containment check, so a run id containing ../ writes artifacts outside the intended tree. I am explicitly not making this a Required Action: the variable is supplied by whoever runs the battery on their own machine, it is not attacker-reachable, and a containment guard on a dev-local test knob would be ceremony. I raise it only because #16508 is open on precisely this class one surface over (runPlaywrightTest's directory guard being a substring test rather than containment) — if that lands a shared containment helper, this is a natural second caller. Worth a sentence in your head, not a commit today.

Second, smaller: the persisted tee lives in a comment rather than a mechanism. I probed whether it had a better home and it does not — package.json exposes only test-e2e, there is no battery script, and a config file cannot constrain its own invocation. So the comment is the correct home today. If a battery script ever lands, the tee belongs in it, and this comment becomes the thing that drifts.

Rhetorical-Drift Audit:

• PR description: framing matches the diff; "smaller than prescribed" is substantiated by the diff being one env-driven constant plus comments
• Anchor & Echo: the config comment states the mechanism ('pipe' default, 'inherit' dropped) rather than just the instruction, which is what makes it survive a copy-paste
• [RETROSPECTIVE]: none claimed
• Linked anchors: the playwright/lib/runner/index.js citation is load-bearing for the Delta and is stated as verified, not assumed

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: The reusable shape is the Delta itself — a ticket prescribing a config change, falsified by reading the dependency's source, resolved by changing nothing on that axis. Worth citing whenever a fix is about to implement a prescription without checking the framework already does it.
• [TOOLING_GAP]: trace: 'on' writing every serial execution into the same per-test directory is a Playwright default that quietly destroys evidence in any repeated-run workflow. That is a property of the tool, not of this repo, and it will bite any future battery-style harness.
• [RETROSPECTIVE]: "A roving red's trace can no longer be overwritten by a later green" is the sentence that names why this matters — the failure it prevents is losing the intermittent evidence specifically, which is the only kind a battery exists to catch.

🎯 Close-Target Audit

• Close-targets identified: #16732
• Confirmed not epic-labeled — #16732 is bug, ai, testing

Findings: Pass.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: a single test-config file with no public/consumed contract, no evidence-ladder residual (the ACs are directly observable from the run artifacts), no OpenAPI surface, and no cross-substrate convention.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 32aaf3164e (12/12 pass, verified live), MERGEABLE/CLEAN; the author's three-run live witness is the right evidence class here, since the defect is only observable across repeated runs and no unit test can express it
• Reviewer falsifier: N/A — my one behavioural concern (legacy-path preservation) is settled by reading the diff, where the fallback is the prior literal unchanged; running the battery to re-observe it would add nothing
• Test location: N/A — config-only change, no specs added or moved

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — the run-scoping lands in the config that owns outputDir, and the solo path is preserved rather than re-derived; 5 held for the unguarded path interpolation noted above.
• [CONTENT_COMPLETENESS]: 96 — the comment carries the mechanism and the exact invocation, not just an instruction; the Delta documents what the experiment removed and why.
• [EXECUTION_QUALITY]: 95 — one env-driven constant, exact legacy fallback, and a live three-run witness for a defect no unit test can express.
• [PRODUCTIVITY]: 98 — the prescribed fix was reduced to its true minimum on evidence, which is the outcome a ticket should want and rarely gets.
• [IMPACT]: 65 — a test-harness fix, but one that protects the evidence for every future intermittent-failure hunt.
• [COMPLEXITY]: 20 — one file, one branch, one constant; the reasoning density is in the comment rather than the control flow.
• [EFFORT_PROFILE]: Quick Win — small surface, disproportionate protection against losing exactly the evidence a battery is run to capture.

Merge gate is @tobiu's.

⚖️