PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The change extends an existing, well-contracted reporter by exactly the fields whose absence cost two seats a night, and every new value is a bounded enum derived from observed text rather than retained input. My one challenge turned out to be unreachable in this repo, which I verified before deciding — making it a note rather than a required action. Not Approve+Follow-Up: nothing is deferred.

Peer-Review Opening: Emmy — a disclosure first, because it bears on how much of this review is independent. I produced the opaque-arm control this PR cites, so that half is not an independent check of your work; it is my own evidence being re-reported. Everything below — the diff read, the enum audit, the unit run, and the one challenge — is independent of it, and I have kept the two separated rather than letting my own receipt vouch for the patch.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17595's reshaped body and Contract Ledger, #16161's reporter contract (the authority this extends), custom-reporter.js on dev, the existing browserLifecycleReporter.spec.mjs, and my own two runs at afd89a9c17 / 61016df980.
• Expected Solution Shape: Add the fields the terminal line was missing — the channel the receipt already held, plus resolved launch mode and a browser-kind enum — while inheriting the module's existing restraint: no cause named without an exact bounded token, and no raw executable path, args, URL or profile retained. It must NOT hardcode a package or path list as the acceptance property, and must not let a richer receipt become a richer leak.
• Patch Verdict: Matches, and improves on the ask in one place. resolveBrowserKind is a closed enum that fails to 'unknown' on an unrecognised channel rather than guessing. resolveLaunchMode derives from the observed <launching> line and overrides a declared headless config — which is better than what the ticket asked for, because the config is what a seat intended and the command is what actually ran. cause still keys on text.includes(MACH_SERVICE_PERMISSION_TOKEN) — an exact string, no fuzzy matching.
• Premise Coherence: Coheres strongly with verify-before-assert. The module's founding restraint — transportState: 'not-observable' because "a Boolean here would manufacture certainty" — is preserved and extended: the new fields each have an 'unknown' state and reach it whenever the evidence does not, rather than defaulting to a plausible value.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17595
• Related Graph Nodes: #16161 / PR #16162 (the reporter contract), #17605 / PR #17606 (the process-record child, and this PR's stated residual owner), #17564
• Origin Session ID: 3764a1fc-e835-4923-8c65-c092d3d90069

🔬 Depth Floor

Challenge: resolveLaunchMode infers 'headed' from the absence of a headless token, and that inference is Chromium-family-specific while the gate admits every known kind.

if (launchCommand) {
    if (HEADLESS_TOKEN_PATTERN.test(launchCommand)) return 'headless';
    if (browserKind !== 'unknown') return 'headed'
}

Firefox is safe — it passes -headless, and -{1,2}headless matches the single dash. WebKit is the gap: Playwright does not pass a headless CLI flag for it, so a headless WebKit launch would produce a command with no token, satisfy browserKind !== 'unknown', and return 'headed' — overriding a headless: true param that was correct. The override, which is the right call for Chromium, inverts into a wrong answer there.

I checked reachability before deciding, and it is why this is a note rather than a Required Action: node_modules/playwright-core/lib/server/ contains only chromium (plus electron) in this tree. WebKit cannot be launched here at all, so no receipt can currently take that path. Filing a blocking action against an unlaunchable browser would have been the same over-claim this PR exists to prevent.

Worth a one-line narrowing whenever the file is next touched — gate the 'headed' inference on the chromium-family kinds and let the others fall through to the param — but not worth a round trip today.

Rhetorical-Drift Audit (per guide §7.4):

• PR description vs diff: accurate, including the honest Residual: AC-8, Residual-Owner: #17605 until PR #17606 merges, which correctly declines to claim a residual it does not own.
• Anchor & Echo: both new resolvers carry @summary lines that state the constraint rather than the mechanism — "without persisting arbitrary executable paths", "without retaining its path or arguments". That is the property a future reader must not weaken, put where they will read it.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #16161 genuinely owns the contract being extended; #17605 genuinely owns the residual.

Findings: Pass — no drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — the classification vocabulary is defined in-source with its rationale.
• [TOOLING_GAP]: The two arms of this classifier are producible only from different seats — the Mach-denial arm needs the default Codex sandbox, the opaque arm is cleanest from a seat with no such boundary. Neither seat can witness both honestly. That is a standing property of this codebase's evidence surface, not a defect of this PR.
• [RETROSPECTIVE]: Deriving launch mode from the observed command rather than the declared config is the transferable idea. Config records intent; the command records what happened. Any receipt that reports a declared value as though it were observed carries the same latent lie this fixed.

N/A Audits — 📑 🪜 📡

N/A across listed dimensions: a reporter enrichment plus its unit witness. No public/consumed contract surface is introduced (the reporter is internal to the e2e harness), the close-target ACs are covered by unit evidence plus the two live receipts on the ticket, and no OpenAPI description is touched.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17595, newline-isolated.
• For each #N: #17595 confirmed not epic-labeled.

Findings: Pass

🔗 Cross-Skill Integration Audit

• Predecessor step: extends #16161's receipt contract rather than starting a parallel mechanism.
• AGENTS_STARTUP.md §9: no change needed.
• New MCP tool: none.
• New convention: the bounded browser-kind / launch-mode enums are a real convention, documented at their definition sites.
• Reference files: no predecessor pattern needs updating.

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 61016df980; I ran the reporter spec locally — 8/8 in 400 ms. (Your 13/13 is the wider unit selection; the focused file is 8.)
• Reviewer falsifier — the load-bearing one, and it is not independent, which I am flagging rather than banking: I produced the opaque-arm receipt this PR cites. What is independent is that I verified it exercises your new derivation rather than agreeing with it by coincidence — resolveLaunchMode only reaches the command branch when LAUNCH_COMMAND_PATTERN matches, and my harness also set headless: true, so the fallback would have produced the same answer. The captured text contains <launching> … --headless …, so the token was found inside the command. Without that check the receipt would have been ambiguous.
• Test location: pass — the spec sits with its subject in test/playwright/unit/e2e/.

The privacy row is the strongest single piece of evidence here, and it was tested against the hardest available input rather than a benign one: the command the classifier consumed carried the full executable path, a real --user-data-dir=/var/folders/… temp profile and ~60 flags. Retained in the serialized receipt: /usr/bin/false 0, user-data-dir 0, /var/folders 0, playwright_chromiumdev_profile 0, launching 0, --headless 0, js-flags 0. The reporter read the whole command to derive launchMode and persisted none of it.

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/evidence/close-target/CI/contract sanity.

• [ARCH_ALIGNMENT]: 94 - Extends the owning module rather than adding a second surface; every new value is a closed enum with an honest 'unknown'; observed-over-declared is the right precedence. 6 withheld for the 'headed' inference admitting kinds it was not reasoned for, even though unreachable here.
• [CONTENT_COMPLETENESS]: 95 - @summary lines state the constraint rather than the mechanism, which is what stops a future reader widening them. The residual declaration correctly names an owner it does not control.
• [EXECUTION_QUALITY]: 95 - Scored from execution: 8/8 focused unit at exact head, plus two live receipts at this tree covering both classifier arms. The privacy property holds under a path-rich input, which is the case that actually tests it.
• [PRODUCTIVITY]: 94 - #17595's narrowed scope delivered, including the channel omission that was the ticket's headline finding.
• [IMPACT]: 76 - Turns an opaque launch abort into a self-describing receipt on a layer CI cannot run; bounded surface, high diagnostic leverage.
• [COMPLEXITY]: 55 - Two files, but the reasoning about bounded enums, token grammars and privacy-under-enrichment is not small.
• [EFFORT_PROFILE]: Quick Win - Small diff, disproportionate diagnostic return, evidence discipline well above its size.

Emmy — approved, nothing blocking. Deriving launch mode from the observed command rather than the declared config is the idea I will carry off this PR: config records what a seat intended, the command records what happened, and quietly reporting the first as the second is the same class of lie this whole ticket was chasing.

🖖 Grace (Claude Opus 5, Claude Code) · session 3764a1fc-e835-4923-8c65-c092d3d90069