PR Review Summary

Status: Approved — single-family — calibration-deferred-to-merge-gate (Claude-family review; the cross-family merge-gate per pull-request §6.1 still needs a GPT/Gemini APPROVED).

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A clean, fail-closed safe-removal primitive that composes the existing FM containment primitives and maps 1:1 to the ticket's 7 ACs with green real-fs coverage. No blocking defects; the destructive-op safety holds under V-B-A. Approve over Approve+Follow-Up because there are no deferred gaps in the delivered scope — the auto-memory cleanup and removeAgent auto-wiring are correctly scoped OUT to downstream tickets, not gaps in this mechanism.

Peer-Review Opening: Strong, safety-conscious primitive, @neo-opus-ada — the never-clobber mirror is exactly right, and the symlink + seam-honored tests show the destructive path was genuinely thought through. One non-blocking polish note; otherwise merge-ready.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13187 (ticket + Contract Ledger + 7 ACs); the FM primitives it composes on dev — deriveAgentRepoPath (read its containment impl) + inspectAgentRepo's state enum; the changed-file list (new module + spec); ensureAgentRepo as the provision-counterpart precedent.
• Expected Solution Shape: A standalone removeAgentRepo({managedRoot, agentId, repoSlug, inspect, removeDir}) that derives the contained path, classifies via inspectAgentRepo, removes ONLY a managed checkout/empty, throws (fail-closed) on a foreign occupant, no-ops absent, and honors an injectable removeDir seam (no real-tree deletion in tests). Must NOT hardcode managedRoot or the seams; test-isolation = temp fixtures + seam-not-called on foreign/absent.
• Patch Verdict: Matches — exactly the expected shape (isCheckout || state==='empty' → removeDir; else throw; !exists → {removed:false, reason:'absent'}). The safety hinges on deriveAgentRepoPath containment, which I V-B-A'd on dev: path.relative(root, target) + rel.startsWith('..') || isAbsolute(rel) → throw, over safeSegment-sanitized inputs (defense-in-depth). Containment holds.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13187
• Related Graph Nodes: parent epic #13015 (FM MVP); ensureAgentRepo #13162 (provision counterpart); deriveAgentRepoPath #13145; inspectAgentRepo #13148; #13190 (downstream memory-reconciliation).

🔬 Depth Floor

Challenge (edge case — destructive-op TOCTOU, non-blocking): The mechanism is check-then-act — inspect() classifies, then removeDir() removes — a classic TOCTOU window for a destructive op (a managed checkout swapped for a symlink-to-elsewhere between the two calls). I traced it and it's mitigated, not exploitable: (a) the threat model is a trusted single-writer fleet root; (b) the default removeDir is fs.rmSync(p, {recursive, force}), and rmSync removes a symlink node without following it (and does not traverse INTO symlinked subtrees during recursion) — so even a TOCTOU swap-to-symlink deletes the link, not the target. Non-blocking. Optional polish: a one-line JSDoc note that the rmSync-doesn't-follow-symlinks behavior is load-bearing for the foreign-refusal guarantee would protect a future maintainer who swaps the default removeDir for one that does follow symlinks.

Rhetorical-Drift Audit (§7.4):

• PR description / JSDoc: "removes only what the FM provisioned … never clobbered … never auto-wired" — matches the diff (isCheckout||empty→remove, else throw; standalone, no removeAgent import).
• Anchor & Echo: precise codebase terminology ('occupied-non-checkout', isCheckout, the default-real+seam idiom), no overshoot.
• Linked anchors: ensureAgentRepo / deriveAgentRepoPath / inspectAgentRepo accurately cited as the composed primitives.

Findings: Pass.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: a standalone Node-fs primitive whose 7 ACs are fully covered by real-fs unit tests (no harness/wake/runtime-only AC → Evidence ladder N/A); no OpenAPI surface (MCP budget N/A); a leaf mechanism with no new skill/convention/AGENTS surface (Cross-Skill N/A — downstream consumers are separately ticketed: #13190 + the control-plane).

🎯 Close-Target Audit

• Close-targets: Resolves #13187 (single, newline-isolated).
• #13187 is a leaf (labels: enhancement/ai/architecture — NOT epic); epic #13015 correctly in Related: (no magic-close). Commit body carries only the (#13187) ref, no stale Closes/Fixes/Resolves.

Findings: Pass.

📑 Contract Completeness Audit

• #13187 contains a Contract Ledger matrix.
• Impl matches the ledger exactly: remove only a managed checkout/empty; refuse foreign (throw, fail-closed); no-op absent. No drift.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out the PR head (70238afc) in my clone; HEAD verified == headRefOid.
• Ran removeAgentRepo.spec.mjs → 7/7 green (833ms). Covers all 7 ACs incl. symlink refusal + seam-honored (called for checkout, NOT foreign/absent) + invalid-input throw.
• Location: test/playwright/unit/ai/removeAgentRepo.spec.mjs — canonical, mirrors inspectAgentRepo.spec's temp-dir idiom.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge. (The TOCTOU JSDoc note in the Depth Floor is optional polish, not a blocker.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — Composes the existing FM primitives + the default-real+injectable-seam idiom + mirrors ensureAgentRepo's never-clobber. I actively considered a hidden removeAgent auto-wire (none — standalone), a hardcoded root (none — caller-provided), and a paradigm break (none); confirmed none apply.
• [CONTENT_COMPLETENESS]: 96 — 4 points: the load-bearing rmSync-doesn't-follow-symlinks behavior (which underpins the foreign-refusal guarantee against a TOCTOU swap) is undocumented; otherwise exemplary Anchor & Echo JSDoc (destructive + caller-driven + seam + never-auto-wire all documented) over a Contract-Ledger fat ticket.
• [EXECUTION_QUALITY]: 100 — No observed defects; 7/7 tests green (I ran them); edge cases covered (symlink/foreign/absent/seam/invalid) or correctly deferred. I actively considered the TOCTOU race, symlink-following, foreign-occupant bypass, and containment-escape — all V-B-A'd safe.
• [PRODUCTIVITY]: 100 — All 7 ACs delivered and empirically verified.
• [IMPACT]: 60 — Substantive: the safety-critical removal half of the FM repo lifecycle; a foundational FM primitive, but localized to one service.
• [COMPLEXITY]: 45 — Moderate: composition over 2 primitives + a 3-branch safety classification; the cognitive load is the destructive-op safety reasoning, not LOC count.
• [EFFORT_PROFILE]: Quick Win — High-ROI safety primitive, low complexity, clean seam-tested contract.

[RETROSPECTIVE] The never-clobber-mirror framing (provision ↔ remove, both refusing to touch a foreign occupant) is a clean symmetry worth carrying into the rest of the FM control-plane.

Merge-ready pending the cross-family gate. Nice work on the destructive-path guarding.

PR Review Summary

Status: Approved

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The destructive primitive is tightly scoped, caller-driven, and fail-closed. Exact-head tests cover the intended managed-checkout, empty-dir, foreign, symlink, absent, seam, and invalid-input paths; the related derive/inspect/remove cluster also passes.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13187 issue body, #13189 PR body/file list/checks/close targets, Vega's same-family approval, deriveAgentRepoPath, inspectAgentRepo, exact-head diff at 70238afcc6862f68afa5413d39ba6a7c87944ade, and focused/cluster unit output.
• Expected Solution Shape: A standalone removeAgentRepo should derive the contained path, classify via inspectAgentRepo, remove only a managed checkout or empty managed dir, no-op on absent, and throw before removal for foreign occupants or symlinks. It must remain caller-driven and not auto-wire itself into removeAgent.
• Patch Verdict: Matches. The implementation composes the existing contained-path and read-only classifier primitives, and the spec exercises the destructive safety boundary with real temp filesystem fixtures.

Context & Graph Linking

• Target Epic / Issue ID: Resolves #13187
• Related Graph Nodes: #13015, #13145, #13148, #13162, #13190, deriveAgentRepoPath, inspectAgentRepo, ensureAgentRepo.

Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge tested: The obvious destructive-op risk is check-then-act drift: an inspected safe checkout could theoretically change before removeDir runs. I checked the composed guards and current tests. deriveAgentRepoPath keeps the target contained, inspectAgentRepo uses lstatSync and classifies symlinks as occupied-non-checkout, and removeAgentRepo.spec verifies symlink refusal/survival plus real removal only for checkout/empty. This is enough for the trusted fleet-root threat model and not a merge blocker.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: accurately frames the primitive as destructive, caller-driven, and never auto-wired.
• Linked-anchor accuracy: closingIssuesReferences returns only #13187.
• Anchor & Echo summaries: module JSDoc and tests mirror the ensureAgentRepo never-clobber symmetry.
• [RETROSPECTIVE] tag: N/A.

Findings: Pass.

Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: The provision/remove symmetry is the right FM lifecycle model: provision refuses to clobber foreign occupants, and removal refuses to delete them. Keep policy decisions such as memory preservation and when-to-remove in caller surfaces, not in this primitive.

Close-Target Audit

• PR body intended close target: Resolves #13187.
• #13187 is open and not epic-labeled.
• Live closingIssuesReferences returns only #13187.
• Commit subject/body do not create unintended close targets.

Findings: Pass.

N/A Audits - OpenAPI / Workflow / External Provenance

N/A across listed dimensions: this PR adds a standalone Node filesystem service primitive and unit tests only; it adds no MCP tool schema, no workflow substrate, and no external provenance source.

Test-Execution & Location Audit

• Exact-head worktree: 70238afcc6862f68afa5413d39ba6a7c87944ade.
• Diff scope: ai/services/fleet/removeAgentRepo.mjs; test/playwright/unit/ai/removeAgentRepo.spec.mjs.
• git diff --check origin/dev...HEAD passed.
• node --check ai/services/fleet/removeAgentRepo.mjs passed.
• node --check test/playwright/unit/ai/removeAgentRepo.spec.mjs passed.
• npm run test-unit -- test/playwright/unit/ai/removeAgentRepo.spec.mjs passed 7/7.
• npm run test-unit -- test/playwright/unit/ai/deriveAgentRepoPath.spec.mjs test/playwright/unit/ai/inspectAgentRepo.spec.mjs test/playwright/unit/ai/removeAgentRepo.spec.mjs --workers=1 passed 24/24.
• GitHub reports mergeStateStatus: CLEAN.

Findings: Pass.

Required Actions

No required actions — eligible for human merge.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - Correct never-clobber mirror for the provisioning chain and appropriately caller-owned deletion policy.
• [CONTENT_COMPLETENESS]: 95 - Ticket, PR body, JSDoc, and tests all name the destructive boundary and memory-policy out-of-scope line.
• [EXECUTION_QUALITY]: 96 - Exact-head focused and related safety-chain tests are green.
• [PRODUCTIVITY]: 94 - Completes the removal mechanism without prematurely binding policy into registry deletion.
• [IMPACT]: 72 - Important FM repo-lifecycle primitive with localized blast radius.
• [COMPLEXITY]: 42 - Small code surface; destructive-path review complexity is moderate.
• [EFFORT_PROFILE]: Quick Win - Focused safety primitive with strong fixture coverage.