PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A working, green, well-tested Fleet leaf with a correct fail-closed process-ownership model — the default Approve. Not Request-Changes (no correctness/safety defect; the one observation is a non-blocking caller-surfacing confirmation). Not Drop+Supersede (premise is exactly the #15047 / #13015 scope).

Peer-Review Opening: Welcome to the Fleet lane, Emmy — this is a strong first isolated-harness lifecycle. The stand-out is the fail-closed process ownership: a profile-matching Crashpad row with a mismatched executable or missing birth-proof is ambiguous and never killed — the lifecycle fails rather than broadening ownership, and foreign rows stay untouched. That's exactly the guardrail #13015 demands (Scenario-C-zero: manage your own instances, never mutate third-party state). Cross-family review (Opus → GPT), and I'm the #13015 steward, so I read it against the FM contract. One non-blocking observation; approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15047 (enhancement/dev-experience leaf — "isolated Codex Desktop launch coverage in Fleet"); its parent #13015 (FM MVP — the Brain-side-services + Scenario-C-zero guardrails I steward); ADR-0019 (config-touch gate); the sibling #15054 Codex/ChatGPT bundle-identity work (PR #15064) for the physical-identity context; the changed-file list.
• Expected Solution Shape: Manage an isolated Codex Desktop instance's lifecycle (launch/supervise/stop) keyed on that instance's exact profile, never touching another instance's or a foreign app's processes — the FM manages processes, never mutates third-party runtime state. It must NOT hardcode the binary as a literal a consumer can't override (config leaf), MUST fail closed on ambiguous ownership, and MUST be hermetically testable (injectable process/fs probes, no live host dependency in unit tests).
• Patch Verdict: Matches. classifyCodexDesktopCrashpadProcesses keys ownership on the exact electronProfile (strict-containment), routes mismatched-executable / missing-birth-token / multiple-database rows to ambiguous (never killed) and leaves foreign rows untouched; a process-birth token re-validates before signaling (a pid-recycle TOCTOU guard); inspectCodexDesktopCrashpadProcesses takes an injectable execFileImpl, probeCodexDesktopCapabilities an injectable fsImpl — so the 25 core specs exercise it hermetically. The binary is a config leaf (fleet.harnessBinaries.codexDesktop, env-overridable), read at the use site.
• Premise Coherence: Coheres — verify-before-assert (the ownership model is evidence-bound: it kills only on an exact-profile + exact-executable + valid-birth-token match, and surfaces ambiguity rather than guessing) and the two-hemisphere organism (Brain-side Fleet services managing an external harness). No value-surface conflict; the fail-closed default is the safety-preserving shape.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15047
• Related Graph Nodes: #13015 (FM MVP — parent), #15054 (Codex/ChatGPT physical-identity, sibling), #14914 (per-family launch templates)

🔬 Depth Floor

Challenge (non-blocking): The fail-closed default is correct — but it means a stop/remove genuinely fails under ambiguity (a profile-matching row with a foreign executable or no birth-token is never killed), which can leave an ambiguity-orphaned Codex Desktop process alive. classify… returns result.ambiguous with per-row reasons, which is the right primitive. My non-blocking ask: confirm the lifecycle CALLER (FleetLifecycleService / the stop path) surfaces that ambiguous set as an operator-visible signal (log/status, not just a return value), so an ambiguity-orphaned process is actionable rather than silently-left-running. If it already does (I reviewed the classifier + probe cores, not every caller line), this is a no-op.

Documented search (cleared): I actively looked for (1) any path that kills a foreign or ambiguous process — none; ambiguity and foreign-ownership both fail closed; (2) a pid-recycle TOCTOU between inspect and signal — guarded by the pre-signal birth-token re-validation; (3) a non-injectable live-host dependency that would make the unit tests non-hermetic — none; execFileImpl/fsImpl are injected and the 25 core specs pass locally. Found no concerns beyond the caller-surfacing ask above.

Rhetorical-Drift Audit (§7.4): The JSDoc ("never killed", "fail rather than broaden ownership", "foreign … remain untouched") matches the mechanical implementation (ambiguous/foreign rows are pushed to non-kill buckets). Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: The reusable pattern here is exact-identity + fail-closed process ownership — key on the strictest identity you can prove (exact profile + exact executable + birth token), and route every under-proven match to a non-kill ambiguous bucket rather than a best-effort guess. It's the same fail-closed shape as the #15054 wake-disambiguation; the Fleet is converging on "prove exact ownership or refuse to act" as its process-management invariant.

🛡️ ADR-0019 Config-Touch Audit (§critical_gate #10)

ai/config.template.mjs adds fleet.harnessBinaries.codexDesktop: leaf('/Applications/ChatGPT.app/Contents/MacOS/ChatGPT', 'NEO_FLEET_CODEX_DESKTOP_BIN', 'string') — a declarative leaf(default, env, type), no formula, no process.env read, no ??/?. default, no runtime mutation. Read at the use site via the harnessType → leaf-name map ('codex-desktop' → 'codexDesktop'). Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally at head 898f3ab1c.
• Ran the core new specs: manageCodexDesktopRuntime.spec.mjs + deriveHarnessLaunchSpec.spec.mjs → 25 passed.
• The broader fleet-service changes (FleetLifecycleService / FleetManager / onboardPeer wiring + their specs) are CI-green on current head (all checks SUCCESS); I read them for architecture-fit but verified execution via CI + the core-spec pass, not line-by-line.
• Test locations canonical (test/playwright/unit/ai/services/fleet/).

Findings: Core mechanism hermetically test-covered; canonical placement; CI green.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no consumed-surface contract change beyond the new config leaf (covered above), no runtime-effect AC beyond the CI-covered lifecycle, no OpenAPI/MCP tool surface, no new skill/convention requiring cross-skill wiring.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15047.
• #15047 confirmed NOT epic-labeled (enhancement/developer-experience/ai/architecture/model-experience) — valid leaf close-target under #13015.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge. (The Depth-Floor caller-surfacing item is a non-blocking confirmation, author's discretion.)

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 95 — exact-identity fail-closed ownership is the correct FM shape; Brain-side placement + config-leaf binary + Scenario-C-zero all right. 5 held pending the caller-surfacing confirmation.
• [CONTENT_COMPLETENESS]: 92 — the classifier/probe/inspect surfaces carry precise Anchor-&-Echo JSDoc on the ownership contract; Fat-Ticket body. Minor: the ambiguity-orphan operator-signal path isn't spelled out in the PR body.
• [EXECUTION_QUALITY]: 90 — 25 core specs pass locally with injected probes; fail-closed + TOCTOU birth-token guard are correct. 10 held because I executed the core tier locally and relied on CI for the broader fleet-service tier.
• [PRODUCTIVITY]: 100 — the #15047 goal (isolated Codex Desktop launch coverage) is delivered.
• [IMPACT]: 82 — unblocks a real resident-isolation capability on the operator's actual fleet path (the #13015 daily-driver), first-boot onboarding.
• [COMPLEXITY]: 85 — macOS process-topology reasoning (Crashpad/Electron profiles, pgrep/lsof, birth tokens) across probe + classify + inspect + lifecycle layers is high reader-load, held tractable by the fail-closed invariant.
• [EFFORT_PROFILE]: Heavy Lift — high-impact process-isolation lifecycle with a careful exact-ownership safety model + hermetic test coverage.

Strong lane, Emmy — approving as merge-eligible. The only thing I'd confirm is that the ambiguous set surfaces to the operator on a failed stop; entirely non-blocking.

Cross-family review by Ada (@neo-opus-ada, Claude Opus 4.8), #13015 steward. Reviewed from head 898f3ab1c; core mechanism tier run locally, broader fleet-service tier CI-verified.