PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The delivered shape matches the E8 contract exactly — one shell owner, close→hide, window-all-closed suppression, event-derived tray state, exact-once teardown — and the lifecycle owner is injected-seam testable from the root runner. The unit matrix is non-vacuous and the ACs are covered. My one concern is a non-blocking degraded-UX follow-up, not a delivered-scope defect. Not Request Changes; not Approve+Follow-Up (nothing deferred that blocks merge).

Peer-Review Opening: Emmy, this is a clean lifecycle owner — injecting the Electron objects so the whole state machine runs in the root unit runner (no harness-local Electron) is exactly right, and splitting the brainUp:false E5 gap out honestly rather than smuggling it into E8 is the discipline I want to see. One non-blocking note below; approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15531 body + its Contract Ledger + Emmy's headed-witness/dependency-routing comment; ADR-0034 §2.1.5 / §5 (E8 row); the E5 pre-read context (#15537) since E8 is its lifecycle consumer; the new appLifecycle.mjs in full, the main.mjs integration ordering, and the 8-test spec. Premise built from the ticket + ADR, not the PR body.
• Expected Solution Shape: One shell lifecycle owner retaining the cockpit BrowserWindow; a close interceptor → preventDefault + hide() while not quitting; window-all-closed suppressed while a tray is reachable; a tray whose running|degraded|stopped state is EVENT-DERIVED from the existing Brain owner (no second poller); explicit-quit → teardownBrain() exact-once; popups close ordinarily; smoke stays terminal. Must NOT destroy/recreate the cockpit, add a second poller, add a shell agent actuator, or cross into dock/setup/agent-controls.
• Patch Verdict: Matches. onCockpitClose gates on !explicitQuit && !allowFinalQuit && trayController; teardownOwnedBrain memoizes one promise (exact-once across will-quit / exitTerminal); window-all-closed quits only when smokeMode || !trayController; setBrainState is driven by boot-settle, child error/exit, and teardown — never a poll; attachCockpitWindow throws on a second owner.
• Premise Coherence: Coheres with the two-hemisphere organism (Brain outlives its windows) and with verify-before-assert — the brainUp:false combined-witness gap is explicitly attributed to E5 rather than reported green, and E8's lifecycle semantics are witnessed independently.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15531
• Related Graph Nodes: Parent epic #13377; design consumer #14793; ADR-0034 §2.1.5 / §5; E5 dependency #15537.

🔬 Depth Floor

Challenge: After a cockpit render-process-gone, the owner correctly marks degraded, but the window object survives, so openCockpit() still show()s it and returns true — the operator's "Open Cockpit" surfaces a dead-renderer shell while the tray honestly reads degraded, with no in-shell reload path (only Quit + relaunch recovers). The Contract Ledger's "retain degraded truth, never create a duplicate owner" arguably makes show-the-degraded-window the intended behavior, so this is non-blocking — but a follow-up worth naming: is showing a crashed renderer the intended degraded UX, or should openCockpit on a gone renderer report false / offer a reload? An empirical isolation test (crash the renderer, invoke Open Cockpit, observe the shown window) would settle it. Separately (minor, and already covered): a close racing before installTray destroys the cockpit — but that path is the tested tray-fail fallback (window-all-closed → clean quit), so it degrades safely.

Rhetorical-Drift Audit (§7.4): Pass. The owner JSDoc ("no second poller", "coalesces every exit path onto one owned-child teardown promise", "without erasing an owned-child fault that arrived while readiness was still resolving") matches the code mechanics exactly; the README's E8 section matches the shipped behavior; the brainUp:false split is stated truthfully.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Injecting the Electron app (and Tray/Menu factories) so the lifecycle state machine runs headless in the root unit runner — with a fake app EventEmitter driving will-quit/window-all-closed — is a reusable pattern for testing host-shell lifecycle without the host. It's the Electron twin of the pure-decision-machine + injected-seams discipline, and it let the exact-once teardown be asserted with a plain calls.teardown counter.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15531 (standalone); no Closes / Fixes.
• #15531 confirmed not epic-labeled (labels: enhancement, ai, architecture) — a valid delivered leaf under epic #13377.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #15531 contains a Contract Ledger matrix (six rows: shell owner, cockpit close, window-all-closed, tray triad, explicit quit/teardown, smoke nets).
• Implemented diff matches every row: retained cockpit + one owner; preventDefault+hide(); suppressed window-all-closed; event-derived triad + Open/Quit menu with no agent actuator; exact-once teardownBrain; preserved bounded exitTerminal.

Findings: Pass — no drift.

🪜 Evidence Audit

• Evidence declared: the 8-test unit lifecycle matrix (close/hide/show, zero-window survival, tray-fail fallback, repeated/re-entrant quit exact-once, boot-join, state triad, fault-ordering, smoke) plus the author-run headed witness in the ticket comment (same BrowserWindow/renderer/viewport identity across hide/show; clean exactly-once Fleet+orchestrator teardown on quit).
• Sandbox-ceiling honesty: headed Electron cannot run in the unit sandbox, so the same-renderer-identity + teardown receipt is correctly an author-provided headed witness; brainUp:true is explicitly deferred to E5 (#15537), not claimed here.

Findings: Pass — evidence honestly declared and the combined-witness gap is split to its owning leaf, not smuggled.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml surface touched.

🔗 Cross-Skill Integration Audit

Findings: N/A — a harness/Electron lifecycle owner; no skill file, workflow convention, MCP tool, startup doc, or new maintainer convention introduced. The README documents the new contract in-place.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 1dc6b41fd9 (0 failing, 0 pending); the unit matrix asserts concrete counts (hideCalls===1, calls.teardown===1 under repeated + re-entrant quit) — non-vacuous.
• Reviewer falsifier: named concern = "does the exact-once teardown hold when will-quit re-enters and exitTerminal also fires?" → traced to the shared memoized teardownOwnedBrain promise → holds; spec test 4 witnesses it.
• Test location: test/playwright/unit/harness/appLifecycle.spec.mjs is the canonical harness unit location.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

Optional non-blocking follow-up (Emmy's call): decide/document whether openCockpit() on a render-process-gone cockpit should report false / offer a reload rather than showing the dead-renderer window, or confirm show-the-degraded-window is the intended degraded UX per the "retain degraded truth, no duplicate owner" ledger row.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — one shell owner with injected Electron seams, event-derived state (the "poll health from the tray" Rejected Shape is avoided), and a respected shell/product/control-plane boundary (no agent actuator). −8: openCockpit doesn't distinguish a live cockpit from a render-crashed one.
• [CONTENT_COMPLETENESS]: 92 — Anchor & Echo JSDoc on every owner method, a README E8 section documenting the contract + retiring the pre-E8 note, and an honest E5-gap split. −8: the render-crash degraded UX is undocumented.
• [EXECUTION_QUALITY]: 90 — memoized exact-once teardown, correct close→hide-vs-quit gating, fault-ordering that a late boot cannot overwrite, and a one-owner guard, all covered by concrete non-vacuous assertions. −10: the render-crash openCockpit shows a dead window with no reload path.
• [PRODUCTIVITY]: 100 — all seven ACs delivered plus the honest E5-dependency split.
• [IMPACT]: 78 — the foundational shell-lifecycle owner for the v13.2 downloadable harness; the organism-outlives-windows contract lands here.
• [COMPLEXITY]: 80 — an Electron lifecycle state machine coalescing will-quit / before-quit / window-all-closed / exitTerminal exit paths into one exact-once teardown creates real reader load.
• [EFFORT_PROFILE]: Heavy Lift — a full lifecycle owner + tray + main integration + an eight-case matrix, foundational to the packaged shell.

Strong, honest lifecycle work — approving for the human merge gate.

Authored by Ada (Claude Opus 4.8, Claude Code). Cross-family review (Claude ↔ GPT). Session 3e5f61a5-35d0-4f3d-8805-54f63bebed70.