PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Free of blocking defects. The implementation is correct — I empirically verified the one real risk (default-instance regression) is handled — and the spec is green (15/15). The single finding is a non-blocking test-coverage gap on the highest-risk path, better added as a quick polish than gated. The #12571 freshness-gate interaction is a pre-existing dependency shared with #12570, not introduced here.

Peer-Review Opening: Clean, well-isolated fallback. Reusing the bridge daemon's resolveInstancePid as the maps-to-a-real-pid proof is exactly the right composition — it makes the path fail-closed by construction — and the pure-resolver / side-effect split keeps it testable. One coverage gap on the default-instance path worth closing; details below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12419 (Ticket C — leaf; enhancement/ai/architecture, no epic), the changed files, the current dev BootEnvelopeResolver, the sibling instanceResolver.mjs (resolveInstancePid / resolveDefaultInstancePid) I went deep on for #12570, and my own (default-instance) live process tree via ps axww.
• Expected Solution Shape: A pure, testable fallback that, when the explicit env envelope is absent, discovers the userDataDir from the MCP server's parent chain — without hardcoding any per-operator path, without changing the durable template / identity-root shape, preserving explicit-envelope-wins + partial-fail-closed, and proving the discovered dir maps to a real main pid. The default (arg-less) instance MUST keep routing by absence (null).
• Patch Verdict: Matches. resolveParentChainOverrideMetadata is gated on darwin + enableParentChainFallback; the pure resolveParentChainUserDataDir walk is cycle-guarded + depth-capped and feeds resolveInstancePid as the final proof; it fails closed on every ambiguous / non-Electron / cloud shape; no template / identity-root shape changed; the fallback sits only inside the both-absent branch, so explicit-envelope precedence + partial-fail-closed are untouched.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12419
• Related Graph Nodes: #12418 (BootEnvelopeResolver origin), #12422 (boot-envelope mechanism), #12570 (vega subscriptionTemplate — complementary), #12571 (HARNESS_PRESENCE refresh — interaction below)

🔬 Depth Floor

Challenge (edge case — empirically verified, non-blocking): The risk I chased hardest — does this fallback hand the default (arg-less) instance a userDataDir address? Electron passes --user-data-dir to helper subprocesses even for the default instance. I confirmed on my own (default) process tree that the arg-less default Claude main has renderer / GPU / network helpers all carrying --user-data-dir=<default Application-Support location>. If those helpers were ancestors of the MCP server, the fallback would flip the default onto the targeted (userDataDir) path — which is subject to the HARNESS_PRESENCE freshness gate that #12571 shows is bootstrap-only (stale after 5 min) → a default-instance wake regression.

I traced it against reality: the memory-core MCP server is a direct child of the Claude main (live ppid is the Contents/MacOS/Claude main, not a renderer helper). For the default, that main is arg-less (no --user-data-dir, ppid 1), so the parent-only walk terminates at null — the --user-data-dir-carrying helpers are siblings, never visited. The code is correct: the default stays absence-routed.

But the test doesn't prove it. The "default instance returns null" test uses a trivial non-Electron chain (90001 1 node /tmp/server.mjs); it does NOT model the realistic default shape — an arg-less Contents/MacOS/Claude ancestor (ppid 1) with sibling helpers carrying the default-location --user-data-dir. That is exactly the scenario where a future walk-logic change (inspecting children, or a looser isElectronAppProcess) would silently break the default instance — the coordinating harness. Recommend adding that default-case test. Non-blocking (code is correct today), but it locks in the highest-value safety invariant.

Second, smaller robustness note: isElectronAppProcess matches any .app/Contents/... Electron process, not Claude specifically (I saw an unrelated WebStorm process carrying --user-data-dir on this box). The resolveInstancePid proof gate makes a misroute unlikely (a non-Claude dir won't map back through the Claude chain), and the MCP's parent chain is always Claude in practice — so this is a robustness nit, not a defect.

Rhetorical-Drift Audit: Pass. The docstring update ("Omitting both first attempts a narrow macOS Electron parent-chain fallback… Otherwise omission remains the normal default instance routed by absence") matches mechanical reality — the fallback returns null for the real default, so it does remain absence-routed. No overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Reusing the bridge daemon's resolveInstancePid as the discovered-address proof step is the right call — an address that can't map back to a concrete main pid is never emitted, so the fallback is fail-closed by construction rather than via a parallel heuristic.
• [KB_GAP]: The sibling-vs-ancestor invariant for the default instance (Electron helpers carry --user-data-dir even when the arg-less main does not, but the parent-only walk never visits siblings) is a subtle, load-bearing property that currently lives only in the implementer's head + this review. The recommended realistic-default test would encode it durably.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: the emitted {instanceAddress, addressType} output contract is unchanged (the added options arg is injectable-test-only with behavior-preserving defaults), so no Contract-Ledger surface; no OpenAPI / MCP-tool-description surface touched; no skill / convention / startup surface touched (internal resolver only).

🪜 Evidence Audit

The PR body carries an honest Evidence: L2 (...) line, and #12419 AC4 is itself unit-coverage-scoped (helper→main fallback + fail-closed edges), so L2 is the correct ceiling — the live multi-Claude auto-discovery behavior is a separate post-merge L3 validation, not gated here. The one L2 completeness gap is the realistic-default case flagged in the Depth Floor.

Findings: Pass (L2 declared = L2 required; one recommended L2 coverage addition).

🎯 Close-Target Audit

• Close-targets identified: #12419
• #12419 confirmed not epic-labeled (labels: enhancement, ai, architecture); Resolves #12419 present in the PR body, newline-isolated.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out (bb2ba9825, detached) + ran the related spec: 15 passed.
• Canonical location correct: test/playwright/unit/ai/mcp/server/shared/services/.
• Coverage gap (realistic-default case) noted in the Depth Floor.

Findings: Tests pass; one recommended (non-blocking) coverage addition.

📋 Required Actions

No required actions — eligible for human merge.

Recommended (non-blocking, ideally a quick pre-merge polish per guide §2.5):

• Add a default-instance test whose psOutput mirrors reality — MCP → arg-less Contents/MacOS/Claude (ppid 1) with sibling helpers carrying --user-data-dir=<default> — asserting null. Locks in the sibling-not-walked + arg-less-main-yields-null invariant.
• Merge-order awareness: this fallback expands targeted (userDataDir) delivery to auto-discovered named instances, which are subject to the bootstrap-only HARNESS_PRESENCE freshness gate. Landing #12571 makes that delivery robust past 5 min idle (shared dependency with #12570, not a blocker for this PR).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — 8 deducted: strong alignment with the existing instanceResolver pattern (pure/side-effect split, cycle-guard, depth cap, fail-closed) and the reuse-as-proof composition; minor deduction for isElectronAppProcess being Electron-generic rather than Claude-specific.
• [CONTENT_COMPLETENESS]: 95 — 5 deducted: full Anchor & Echo JSDoc on every new method + updated class docstring + Fat PR body; the load-bearing sibling-vs-ancestor default-instance invariant isn't captured in a code comment or test.
• [EXECUTION_QUALITY]: 85 — 15 deducted: 15/15 spec green and code verified correct on the real default instance; deducted because the highest-risk path (default-instance null) is covered only by a trivial non-Electron snapshot, so a future walk-logic regression there wouldn't be caught, plus the Electron-generic heuristic nit.
• [PRODUCTIVITY]: 95 — 5 deducted: fully achieves #12419's parent-chain fallback AC; only the recommended polish test outstanding.
• [IMPACT]: 70 — substantive wake-substrate ergonomics: removes the manual per-instance NEO_HARNESS_INSTANCE_ADDRESS step for named instances and composes with #12570 to make vega wake-able with zero operator env config. Subsystem-level, not foundational.
• [COMPLEXITY]: 60 — descriptive: moderate; a depth-capped, cycle-guarded parent-chain walk plus Electron process heuristics plus the reuse-as-proof composition. The sibling-vs-ancestor subtlety adds real reasoning load for a maintainer.
• [EFFORT_PROFILE]: Quick Win — one focused resolver file + comprehensive unit tests; high ROI (removes a manual rollout step) at contained complexity.

Nice work — this is the piece that makes the vega path (and any future named instance) wake-able without hand-set env vars. Approving; the realistic-default test is the one thing I'd add before merge to lock in the default-instance safety property I had to verify by hand.