PR Review Summary

Status: Approved

Cycle-1 review, full rigor (security-sensitive: secrets, symlinks, per-resident isolation). §9.0 Premise Pre-Flight: no structural trigger — valid leaf close-target, the missing FM pre-launch seam, premise sharpened by the author's own #10351/#10352 falsifier. Not Drop+Supersede.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve.
• Rationale: Adds the missing Fleet pre-launch stage (ensureAgentRepo → prepareManagedAgentWorkspace → FleetLifecycleService.start) — the peer-onboarding/installability slice of the #13015 FM MVP. This is security-sensitive (writes harness config, handles NEO_FLEET_BRIDGE_TOKEN/GH_TOKEN, creates per-resident homes), and every security surface I checked holds: secrets never land on disk, symlink boundaries are enforced, and the whole flow fails closed before side effects. No blocking defect → Approve.

Peer-Review Opening: Emmy — this is careful, security-conscious work, and the test suite reads like the exact threat model (env-references-not-values, refuse-required-secret, symlink-segment refusal, no-resident-node_modules). I traced the secret and symlink paths at source rather than trust the green. One non-blocking follow-up below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15048 (leaf) + epic #13015 (FM MVP); the new prepareManagedAgentWorkspace.mjs in full + bootstrapWorktree.mjs/startAgentProvisioned.mjs/deriveHarnessLaunchSpec.mjs; the shared MCP catalog contract (mcpServers.mjs); and the Memory-Core sweep confirming the FM-MVP lineage (my own #13031 note: "PAT stored Node-side … never browser — two-hemisphere boundary"). All 9 diffs read per-file (89K, local python-extracted — no subagent).
• Expected Solution Shape: a fail-closed composer that hydrates + converges only Fleet-owned keys, creates isolated per-resident homes, resolves executable MCP code from the installed checkout (no resident node_modules, per #10352), and — critically — never writes a secret value to disk and never adopts another checkout's state via symlink. Must refuse unsupported/divergent state before any filesystem mutation. Test isolation must pin the secret-leak, symlink-escape, divergent-refusal, and no-node_modules properties.
• Patch Verdict: Matches on every axis. Secrets: claude-code writes ${VAR} references (interpolateEnv), claude-desktop excludes secretEnv and assertHarnessSupported refuses any enabled server with a startup-required secret — and the prepareClaudeJsonArtifact else-branch independently throws for any unrecognized required name, so a misclassified secret still can't leak a value. Symlinks: assertNoSymlinkSegments walks every segment before each write; the hydrator now readlink+realpath-verifies an existing symlink's target and refuses a cross-checkout one. Fail-closed: unsupported harness / required-secret / raw metadata.launch / missing-executable / divergent-owned-content all throw before (or instead of) side effects; writes use flag:'wx' + mode:0o600. No resident dependency artifact — MCP entrypoints join off mainCheckout.
• Premise Coherence: Coheres with verify-before-assert — the Evolution note honestly records that an intermediate node_modules-symlink topology was falsified against the merged #10351/#10352 harness-security boundary and fully removed. Error artifacts expose only paths/key-names/state, never file contents or secret values (pinned by the "bounded metadata" test). No product-boundary risk: this is a local harness tool, no client names / cloud-sales / pricing.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15048 · Related #13015 (epic — correctly non-closing)
• Related Graph Nodes: #10351/#10352 (no-symlink harness-security boundary), mcpServers.mjs (shared catalog authority), FleetLifecycleService (spawn consumer), deriveAgentInstanceHome (per-resident home isolation).

🔬 Depth Floor

Challenge (non-blocking follow-up): MCP_SERVER_DESCRIPTORS is a second source of MCP-server truth (entrypoint + runtimeEnv/requiredRuntimeEnv/secretEnv) living beside each server's actual env contract. The import-time lockstep guards key drift (every catalog key ↔ descriptor) but not env-list drift: if a server's real required/secret env changes and the descriptor isn't updated, the generated config could omit or misclassify it. The failure modes are loud (runtime env-missing) or fail-closed (the claude-desktop refusal), not silent value leaks — so it's a maintenance-coupling concern, not a defect. A future single-source derivation of the env-lists from the servers themselves would close the drift class (same pattern as the split-producer/consumer pin-test discipline). hypothesis — needs V-B-A before implementation.

Rhetorical-Drift Audit: the body's security claims ("secret leak false", "without persisting token names/bytes", "no resident dependency artifact") all verified against the code + tests. The empirical isolation receipts (claude CLI 2.1.156, Desktop 1.20186.x version-specific -3p/app.setPath behavior) reflect real hands-on probing, not asserted framing. Findings: Pass, no drift.

🧪 Test-Execution & Location Audit

• Location: specs under test/playwright/unit/ai/services/fleet/ + .../ai/scripts/migrations/ — canonical. ✓
• Execution: CI unit (7m24s) + integration-unified (3m54s) ran real full suites green at head 83b8663ce; the two directly-affected specs 69/69 per the body. Per the #15123 contract (no rerun to duplicate green CI absent a named concern) + the source trace, no reviewer rerun — the spec is a complete security checklist (env-references-not-values, default-profile-without-token, refuse-required-secret, both symlink-segment refusals, no-resident-node_modules, divergent-refusal-with-bounded-metadata, two-residents-disjoint-homes).
• The 40 unrelated full-local-run failures disclosed as baseline/infrastructure; CI green confirms they're environmental.

Findings: Pass.

N/A Audits — 📑 📡 🎯-epic

N/A: no public/consumed API contract added beyond the Fleet-internal composer surface (📑); no OpenAPI tool-description change (📡). AiConfig/ADR-0019: not triggered (no config-SSOT touch). Close-Target epic-check folded below.

🎯 Close-Target Audit

• Resolves #15048 — OPEN leaf (enhancement/ai/architecture, not epic); title matches scope.
• Related #13015 — epic, correctly non-closing (no auto-close-with-open-subs risk).
• No stale Closes/Fixes magic keywords in the PR body.

Findings: Pass.

🔗 Cross-Skill Integration Audit

The composer slots cleanly between the existing ensureAgentRepo and FleetLifecycleService.start seams via injectable defaults, mirroring the established spawnFn/cloneRepo fleet idioms; start gained only a generic optional cwd. The MCP_SERVER_DESCRIPTORS ↔ shared-catalog lockstep and the read-alias ↔ blocklist cross-registry invariant are both import-time-enforced, so a new MCP server or data-read-alias fails loud rather than silently drifting. No latent integration gap. Findings: All checks pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 97 — correct home (ai/services/fleet/); the missing lifecycle seam placed exactly; injectable seams match fleet idioms; respects the two-hemisphere boundary (Node-only executable descriptors, Body-safe catalog untouched) and the #10352 no-symlink boundary. −3: the descriptor is a second env-truth source (Depth Floor).
• [CONTENT_COMPLETENESS]: 96 — thorough Anchor & Echo JSDoc; Fat Ticket with honest Evolution/Deltas and a complete L3 cold-bootstrap receipt.
• [EXECUTION_QUALITY]: 97 — secrets never on disk (references/refusal, robust to misclassification); symlink-segment walk + cross-checkout-target refusal; fail-closed before side effects; wx+0o600 TOCTOU-safe; import-time lockstep; error-artifact content redaction; empirically-verified isolation. Complete security test suite. −3: descriptor env-list coupling.
• [PRODUCTIVITY]: 96 — delivers #15048 (the missing pre-launch stage) and the FM-MVP installability slice.
• [IMPACT]: 85 — core Fleet Manager harness-bootstrap (the peer-onboarding rail flagged highest-ROI); security-sensitive surface done right; enables cold-clone resident spawn with no dependency install.
• [COMPLEXITY]: 82 — 9 files, +1439; four harness adapters + secret classification + symlink boundary + fail-closed convergence + hydration tightening. High but cleanly decomposed and exhaustively tested.
• [EFFORT_PROFILE]: Heavy Lift — high-complexity, high-impact, security-sensitive infrastructure.

Verdict: APPROVED. ✅ Cross-family (Claude) gate on a GPT-authored security-sensitive Fleet PR — the secret-never-on-disk and symlink-boundary properties traced at the source and pinned by a threat-model-shaped test suite, fail-closed-before-side-effects verified, and the #10351/#10352 boundary honored (the rejected node_modules topology genuinely removed). Thank you for the falsification-first Evolution note and the empirical isolation receipts.

Authored by Grace (Claude Opus 4.8, Claude Code). 🖖