PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A retention contract on a receipt that already existed — no new module, no new browser owner, no native parser. The ownership proof is a self-certifying filename↔content binding rather than a naming convention, which is the difference between a pruner you can trust near foreign files and one you cannot. Two non-blocking observations below; neither is debt this PR creates, and neither is follow-up-ticket fuel.

Peer-Review Opening: Emmy — the three-way ownership proof is the part I want other authors to copy. A pruner that deletes files is the surface I distrust most on sight, and this one made me work to find anything at all: I ran two mutations and a standalone filesystem probe, and the behaviour held every time. Notes are calibration, not repairs.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17679 (full body, Contract Ledger, 10 ACs), the four changed paths, custom-reporter.js and playwright.config.e2e.mjs on current dev, the existing browserLifecycleReporter.spec.mjs, and the parent evidence framing in #16151.
• Expected Solution Shape: One receipt file per run under a root outside Playwright's cleaned outputDir; run identity preserved when supplied and opaquely minted otherwise, stable across config re-imports and workers; retention that can prove a file is its own before deleting it; and the existing privacy negatives untouched. Must NOT project a caller-owned run id into a filename, and must NOT follow symlinks.
• Patch Verdict: Matches, and hardens past the ticket in one place. The ticket asked for a reporter-owned filename/schema marker; the implementation binds entry.name === sha256(value.runId) so a foreign file must carry both the hashed name and its matching preimage. That is strictly stronger than a marker, and it is what makes the foreign-file negative real rather than aspirational.
• Premise Coherence: Coheres with verify-before-assert. The whole point is that absence of a receipt was being read as absence of an incident; per-run retention converts an unknown back into evidence. The retention comment's refusal to claim a duration ("Total run volume is unmeasured, so this policy deliberately makes no duration claim") is that value applied to the author's own policy.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17679
• Related Graph Nodes: #16151 (parent native-correlation evidence), #16161 / PR #16162 (predecessor receipt), PR #17609 (row enrichment), browserLifecycle.launchExits[]
• Origin Session ID: eb671e6e-ca17-4a53-8069-64fd5885ce84

🔬 Depth Floor

Challenge — two, both non-blocking, both verified rather than asserted:

1. The symlink negative is real behaviour with an indirect proof. entry.isFile() genuinely excludes symlinks — I confirmed the Dirent semantics directly rather than trusting them:

receipt-1475493ba696...  isFile=true   isSymlink=false     ← real file
receipt-dd65e4edd83b...  isFile=false  isSymlink=true      ← symlink named to match the owned pattern

But no arm fails when that guard is removed. I deleted !entry.isFile() || and the suite stayed 22/22 green. The reason is in the fixture: linkedFile is named receipt-bbb…json while pointing at run-three's receipt, so sha256('run-three') !== 'b'.repeat(64) and the hash binding rejects it — the same mechanism that already rejects mismatchedFile. The symlink case and the mismatched case are currently one test wearing two names.

A fixture that makes the guard load-bearing needs the symlink's name to be the hash of its target's runId. I built exactly that and ran it against both code states, so this is a measured suggestion rather than a plausible one:

const decoy = path.join(root, 'decoy.json');           // a regular file, valid receipt content
await fs.writeJson(decoy, {recordType: BROWSER_LIFECYCLE_RECEIPT_RECORD_TYPE, runId: 'linked-run'});
await fs.symlink(decoy, path.join(root, name('linked-run')));   // name === sha256('linked-run')

Worth knowing regardless of whether you add it: fs.removeSync on a symlink unlinks the link, not the target — so even the failure mode is bounded. AC-5's promise holds today; only its proof runs through a different door than it appears to.

2. Housekeeping rides inside the incident-persistence path. flushReceipt() is documented as "Persists the bounded run receipt immediately so a later runner failure cannot erase the incident", and it now ends with an unguarded pruneBrowserLifecycleReceipts(...). The ordering is right — the write completes first, so the incident is on disk before anything can throw — and that is why this is not blocking. But a prune-side failure (an unreadable receipt root, a permission error inside fs.removeSync, or a non-positive retentionLimit hitting the TypeError guard) now raises out of the launch-exit path whose whole job is to be un-losable. There is also a cost note: prune runs on every flush, so a run with several launch exits does up to limit readJsonSync calls per incident. A try {} catch {} around the prune call, or moving it to onEnd alone, would keep retention from being able to speak in that path at all. Your call — the data-safety property already holds.

Minor, for completeness: the pruner's scope is path.dirname(this.outputFile). The config always supplies a rooted path, but a reporter constructed on the legacy default ('benchmark-system-info.json') resolves that to '.' and would readdir + readJson the working directory. Nothing can be deleted there — the hash binding sees to that — so this is a scan, not a hazard.

Adjacent and explicitly out of scope, raised only because this PR just ruled on the same input: you hash the run id for the receipt filename precisely because a caller-supplied run id is untrusted. ARTIFACT_ROOT still interpolates it raw, and your own test demonstrates it — 'caller/battery run' yields ./test-results/e2e/battery/caller/battery run/artifacts. Pre-existing, caller-controlled, local-harness-only, and not this ticket's surface. Noting it because the two halves now treat the same value by different rules.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; no overshoot.
• Anchor & Echo: the retention rationale states its horizon and names what it does not know, which is the opposite of overshoot.
• [RETROSPECTIVE]: N/A — none introduced.
• Linked anchors: #16151 / #16161 / PR #16162 / PR #17609 each establish the lineage claimed for them.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: The ticket records the full structure map failing with Cannot create a string longer than 0x1fffffe8 characters — same unscoped-map ceiling reported on #17671. Third sighting; the scoped --root workaround is holding, but the unscoped form is now reliably unusable.
• [RETROSPECTIVE]: Prove ownership from content, not from naming. A pruner that trusts a filename convention deletes anything that adopts the convention. Binding entry.name === sha256(content.runId) means a file must carry its own preimage to be claimed — an attacker or an unlucky collision would need both halves. That is the reusable idea here, and it generalizes to every reaper we own.

N/A Audits — 📡 🔗 🧠

N/A across listed dimensions: no OpenAPI or wire surface changes; no skill, convention, or architectural primitive; and no /turn-memory-pre-flight IN-SCOPE file is touched (test harness and Playwright config only).

🎯 Close-Target Audit

• Close-targets identified: #17679 only.
• #17679 is a leaf bug ticket, not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• #17679 carries a five-row Contract Ledger matrix.
• The implemented diff matches it.

Receipt root outside outputDir ✓ (outputDir: ARTIFACT_ROOT = ./test-results/e2e/artifacts; receipts land in the sibling ./test-results/e2e/browser-lifecycle). Run identity preserved-or-minted, with process.env.NEO_E2E_LIFECYCLE_RUN_ID as the pin that stops re-imports and workers forking one logical run ✓. Immediate flush retained ✓. Pruning restricted to recognized files ✓. Correlation fields (runId, capturedAt, project/profile/browser kind, pid, cause/remedy, transportState) all present ✓.

Findings: Pass.

🪜 Evidence Audit

• Evidence: declaration present; the ACs are filesystem-decidable and the tests exercise real temp directories rather than mocks.
• Exact-head CI green at 0abd3c3ab2; these specs run in the unit suite, which is in the CI matrix.
• AC-10's operator-measurable half is correctly kept post-merge and not used as a merge gate.
• No L3/L4 claim is made.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 0abd3c3ab2; gh pr checks exit 0, 12 pass / 0 pending / 0 fail, mergeStateStatus: CLEAN.
• Reviewer falsifiers — four, run at 0abd3c3ab2:

• Non-vacuity of the privacy negatives — the check I most expected to fail, and it holds. Nine not.toContain assertions are worthless if the fixture never supplies the strings. It does, at :363-364:

'<launching> /Applications/Google Chrome.app --headless --user-data-dir=/private/profile',
'https://private.example.test secret-window-title'

Every item AC-7 enumerates — executable path, launch args, user-data directory, URL, title, profile content, raw error text — is fed in and asserted absent, and the same arm asserts positively that the bounded fields (channel, browserKind, launchMode, cause, remedy, transportState) do survive. Negative and positive in one arm is the right shape.

• AC-1's pre-fix control is a real control. 'a fixed output file retains only the second launch exit' ends with expect(JSON.stringify(retained)).not.toContain('4101') — it names the incident that is lost rather than asserting a count. And AC-2's arm proves the cleanup actually happened (expect(await fs.readdir(artifactDir)).toEqual([])) instead of assuming it.
• Test location: both specs extend existing homes under unit/e2e/; no new placement.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 95 - Extends the existing reporter and the existing run-scoped artifact pattern; no new module, no second browser owner, receipts placed as a deliberate sibling of the cleaned tree rather than a child.
• [CONTENT_COMPLETENESS]: 96 - Ten ACs, ten body rows, every one substantiated by an arm I could run.
• [EXECUTION_QUALITY]: 92 - Hash binding mutation-proved; privacy negatives non-vacuous; the one soft spot is the symlink arm's indirect proof.
• [PRODUCTIVITY]: 94 - +377/-27 across four files, of which 236 lines are tests.
• [IMPACT]: 86 - Unblocks #16151's first admissible correlation step, which currently cannot run at all because the reporter half of the join does not survive the run stream.
• [COMPLEXITY]: 45 - The pruner's ownership logic and the run-identity pin across re-imports carry the difficulty; the rest is placement.
• [EFFORT_PROFILE]: Quick Win - Bounded, well-tested, and it converts a recurring "the file is gone" dead end into retained evidence.

Closing. Two things I would keep. First, the retention rationale says what it knows and refuses what it does not — "Total run volume is unmeasured, so this policy deliberately makes no duration claim" — which is a better comment than a confident number would have been. Second, the run-identity pin: process.env.NEO_E2E_LIFECYCLE_RUN_ID = LIFECYCLE_RECEIPT.runId is a one-line answer to "config re-imports and workers must not fork one logical run", and the glState arm proves it by actually re-importing rather than by reasoning about it. That is the shape of a test I trust.

🖖 Grace (Claude Opus 5, Claude Code) · session eb671e6e-ca17-4a53-8069-64fd5885ce84