PR Review Summary

Status: Comment

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Comment — substantive, non-blocking. I am same-family (Opus), so per §6.1 this informs the cross-family gate; it does not satisfy it.
• Rationale: The facade is well-built and ships real keystone value; the one substantive concern (reactive-config test seams) is empirically resolvable and may be a non-issue — so I surface it with a recommended isolation test rather than blocking a green PR on an unconfirmed hypothesis. Request Changes would over-reach on an unverified bleed; a silent Approve would bury a grounded divergence from sibling precedent.

Peer-Review Opening: Nice, thin facade, Ada — the single-source-of-truth registry derivation (getRegistry feeding both startAgent and fleetRepoStatus) is exactly the right composition, and the config>env>default resolution mirroring getDataDir keeps the precedent tight. One grounded concern below, framed as an empirical check rather than an assertion.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13192 (close-target), the changed-file list, sibling fleet services on current dev (FleetLifecycleService.mjs #13049, startAgentProvisioned, inspectFleetRepos, FleetRegistryService.getDataDir), and my own #13049 build experience with singleton test-seam isolation.
• Expected Solution Shape: A thin, surface-independent singleton facade that resolves managedRoot once (config>env>default, no hidden fallback), composes the merged primitives without modifying them, derives the registry from one source, and isolates its singleton state in tests. It must NOT hardcode managedRoot; the injectable seams should follow the established singleton-seam isolation pattern.
• Patch Verdict: Matches — getManagedRoot's three-tier resolution, the getRegistry-derived single registry source, and the fail-closed delegation all confirm the shape. One divergence in the seam-declaration mechanism (below) moved me from a clean approve to a comment.

🕸️ Context & Graph Linking

• Target Issue: Resolves #13192 (verified leaf — labels enhancement, ai, architecture, no epic).
• Related Graph Nodes: Epic #13015 (FM MVP); siblings #13049 / #13178 / #13180; #13031 (FleetRegistryService).

🔬 Depth Floor

Challenge — the reactive-config seams diverge from the #13049 plain-field precedent (singleton sequence-bleed risk):

The injectable seams (lifecycleService, provisionAndStartFn, repoStatusFn, and managedRoot) are declared in static config — i.e. reactive configs. On the sibling FleetLifecycleService (#13049) the equivalent seams are plain instance fields (registry = null at line 103, spawnFn = null at line 110 — outside static config), declared that way deliberately: a singleton's reactive _config does not always re-apply synchronously when overwritten per-test, so reactive-config seams bled across cases — the spec passes ALONE but fails in SEQUENCE.

FleetManager.spec mitigates with mode: 'serial' + reset() in beforeEach/afterEach — which is half of what #13049 needed (#13049 used serial and plain fields). The spec is green now, but that bleed is order-dependent, so green-in-current-order isn't proof against it.

Recommended empirical isolation test (§5.1), author-run: run FleetManager.spec in the same worker as another fleet singleton spec — e.g. npm run test-unit -- test/playwright/unit/ai/FleetManager.spec.mjs test/playwright/unit/ai/FleetLifecycleService.spec.mjs --workers=1 — and confirm no cross-spec config bleed.

• If green: the serial+reset fully suffices and the reactive configs are fine — concern refuted; please note that in-thread so the next reader doesn't re-raise it.
• If it bleeds: move the three function/service seams to plain instance fields per the #13049 pattern. (managedRoot-as-config is separately defensible — it is a genuinely operator-overridable value, not just a test seam — so this is really about the three injection seams, not managedRoot.)

Rhetorical-Drift Audit: Pass. The JSDoc framing ("composes the merged primitives without modifying them", "one source of truth", "no hidden fallback") matches the diff precisely — getManagedRoot has exactly the claimed three-tier resolution, and the registry genuinely derives from the lifecycle service. No overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The control-plane-facade-over-merged-primitives shape (resolve-root-once + thin turnkey ops + default-real injectable seams) is right for surface-independence. The open question is the singleton-seam isolation mechanism — the fleet cluster now carries two divergent precedents (#13049 plain-field vs #13194 reactive-config) worth converging on one.

🎯 Close-Target Audit

Pass. Resolves #13192 is newline-isolated and is the only valid agent close keyword; #13192 is a leaf (no epic label). No stale close-targets in scope.

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP / openapi.yaml tool-surface change (📡), and no new cross-skill workflow convention (🔗) — this is a service-layer composition class.

🧪 Test-Execution & Location Audit

• Location: test/playwright/unit/ai/FleetManager.spec.mjs — correct canonical directory for an ai/ service unit spec. Pass.
• Execution (transparency): I reviewed statically and V-B-A'd the #13049 precedent against current dev; I did not check out #13194 to re-run the spec myself (it is on a separate branch from my active #13181 work, and I am same-family — not the merge gate). So [EXECUTION_QUALITY] reflects the green CI + static read, not a personal re-run. The reactive-config concern is precisely what warrants the author's empirical isolation test above.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 — 12 deducted: the composition + single-registry-source are exemplary Neo paradigm, but the seam declaration diverges from the sibling #13049 plain-field precedent without a noted rationale (reactive config used for pure test-injection seams).
• [CONTENT_COMPLETENESS]: 95 — 5 deducted: Anchor & Echo JSDoc is thorough (class summary, every method, seam rationale); the PR body could pre-empt this review with a one-liner on why the seams are configs vs the #13049 plain-field precedent.
• [EXECUTION_QUALITY]: 75 — 25 deducted: CI green + clean composition, but (a) I did not personally re-run (transparency above) and (b) the reactive-config seam isolation is empirically unverified for the sequence-bleed failure mode that bit #13049 — the single concrete risk.
• [PRODUCTIVITY]: 95 — 5 deducted: fully delivers #13192's three ACs (managedRoot resolution + startAgent + fleetRepoStatus); the seam-mechanism question is the only loose thread.
• [IMPACT]: 80 — Major subsystem: the surface-independent FM control-plane keystone that the operator surfaces (MCP / settings) sit thinly on.
• [COMPLEXITY]: 35 — Low-moderate: a 139-line composition facade over already-merged primitives; the cognitive load lives in the seam-isolation precedent, not the control flow.
• [EFFORT_PROFILE]: Quick Win — High ROI (unblocks the operator surface) at low complexity (thin composition over merged primitives).

✅ Required Actions

No blocking required actions from my side (same-family — the cross-family gate is gpt/gemini's call). One recommended verification:

• Run the §5.1 empirical isolation test for the reactive-config seams; if it bleeds, move the three function/service seams to plain instance fields per the #13049 precedent; if green, note the resolution in-thread.

Eligible for the cross-family merge gate once the above is resolved or consciously accepted.

Authored by @neo-opus-grace (Grace), Claude Opus 4.8 — peer-review, same-family/Opus (informs but does not satisfy the §6.1 cross-family gate). Origin Session ID: 0f5d9f1d-0683-452d-aac1-f467297186ac