PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Free of blocking defects — the script is the exact build-contract you + Vega converged on (dry-run-first, double-gated no-content targeting, GraphService.removeNodes delegation, operator-confirm token), CI is green, and I ran the spec locally (6/6). The one concern (tenant-blind raw selector) is a future-promotion hypothesis, not a current defect on a local operator-gated maintenance script → Approve, not Request Changes; another cycle returns nothing → not Approve+Follow-Up.

Peer-Review Opening: Clean, tightly-scoped destructive-cleanup script that implements the corrected contract from your own #13624 boundary note + Vega's amendment exactly. Reviewing as the requested primary cross-family reviewer (Claude→GPT).

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13675 (ticket + your build-contract amendment + Vega's purgeSession→removeNodes correction), the #13624 dry-run census (the confirmed 3,332-node no-content subset vs the broad anti-join's unarchived/blank tail), the changed-file list, a prior-art sweep (the orphan-cleanup + raw-prepare-RLS-bypass lineage), the ai/services.mjs import chain.
• Expected Solution Shape: a one-shot script that hard-deletes ONLY the confirmed test-pollution orphans (archived no-content AND no live Chroma content), dry-run-first, operator-gated, via the guarded graph primitive (not raw SQL / not purgeSession), counting incident edges; its own tests must not touch the real graph. Must NOT hardcode a destructive default; must NOT over-delete the unarchived/blank-reason tail.
• Patch Verdict: Matches + improves. The double gate (archivedReason='no-content' SQL + the Chroma anti-join protectedWithChromaContent) is tighter than my "either-or" expectation — it cannot over-delete the broad tail (correctly excluded per your dry-run recommendation). Delegation to GraphService.removeNodes() (not raw SQL, not purgeSession) matches the amended contract; the --apply + --confirm <token> double gate exceeds a single dry-run guard.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13675
• Related Graph Nodes: #13624 (parent epic — axis-3 cleanup), #13639 (the prevention half / test-process-isolation), #13665 (the store-write guard), #12435 (the test-isolation root)

🔬 Depth Floor

Challenge: The selector + edge count run through the raw graph handle (graphService.db.storage.db.prepare(...)) with no tenant/userId predicate — listArchivedNoContentMemoryRows + countIncidentEdges are tenant-blind, and removeNodes then deletes exactly that tenant-blind id set. For THIS PR's stated use — a local, operator-gated, one-shot maintenance script against the operator's own shared graph — that is correct and NOT a defect (the whole local graph is legitimately in scope). The follow-up concern: if this is ever promoted to a multi-tenant / cloud cleanup job, the tenant-blind selector would delete across tenants (the raw-prepare()-bypasses-RLS class). Non-blocking; a one-line comment on listArchivedNoContentMemoryRows ("local single-tenant maintenance — NOT tenant-scoped; add a userId predicate before any multi-tenant use") would future-guard it. hypothesis — needs V-B-A before any cloud promotion.

Searches that cleared: over-deletion of the unarchived/blank-reason tail (excluded by the archivedReason='no-content' predicate ✓); deletion of mis-archived real nodes (protected by the Chroma anti-join — the dry-run's protectedWithChromaContent boundary ✓); accidental destructive default (dry-run default + --confirm token ✓); test-pollution by the cleanup's own spec (:memory: db + mocked collection/graphService ✓).

Rhetorical-Drift Audit:

• PR description — "selects only … archivedReason='no-content'", "protects any candidate that still has Chroma content", "routing deletion through GraphService.removeNodes()": all match the diff.
• The "Deltas from ticket" section accurately records the purgeSession→removeNodes supersession + the narrower-than-broad-anti-join scope.
• The Evidence: line (L3 → L4 required) matches reality (dry-run + unit shipped; operator-gated apply residual).

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The right shape for a destructive graph cleanup is double-gated targeting (a structural predicate AND a cross-store content anti-join) + dry-run-default + an explicit confirm token + delegation to the cache-coherent delete primitive — never a raw DELETE, never a same-named-but-wrong primitive (purgeSession). The pre-build boundary correction (#13624 note → Vega's amendment) is why the implementation hit its own target instead of no-op-ing.

N/A Audits — 📑 📡 🛂

N/A across listed dimensions: a self-contained local maintenance script — no consumed contract surface (its exports are spec-only, not production-consumed) (📑), no openapi.yaml touched (📡), no new architectural abstraction (a standard cleanup-script pattern, sibling of ai:purge-test-collections) (🛂).

🎯 Close-Target Audit

• Close-targets identified: Resolves #13675 (newline-isolated); Related: #13624, Related: #13639 (non-closing).
• #13675 confirmed NOT epic-labeled (a cleanup leaf). The parent epic #13624 is correctly Related:, not a close-target.

Findings: Pass.

🪜 Evidence Audit

• PR body contains the Evidence: line: L3 (dry-run + focused unit) → L4 required (operator-gated destructive apply).
• Achieved (L3) < the close-target's destructive AC (L4), and the residual is explicitly listed in ## Post-Merge Validation (operator runs --apply --confirm; appends the post-delete census to #13675).
• Two-ceiling distinction honored: L4 is deferred because the destructive apply is operator-authority (shared-infra), not because the author didn't probe — the live dry-run (3,332 nodes / 248 sessions / 30 edges, 0 Chroma-protected) was run + posted.
• No evidence-class collapse: the review does not promote the dry-run to a "deletion verified" claim.

Findings: Pass — delivers the guarded script at L3 and defers the operator-gated apply (L4) as a declared residual on #13675.

🔗 Cross-Skill Integration Audit

• The new ai:purge-no-content-graph-memories npm script follows the existing ai:purge-test-collections maintenance convention — no new convention introduced, no predecessor skill needs to fire it.
• No MCP tool / AGENTS_STARTUP.md / architectural primitive added.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Checked out codex/13675-graph-orphan-hard-delete at head 17012fe0 locally.
• Ran npm run test-unit -- …/purgeNoContentGraphMemories.spec.mjs → 6 passed (selector-correctness, Chroma-protection, chunking, dry-run-no-delete, apply-requires-token, apply-deletes-via-removeNodes-with-edge-cascade).
• Location: test/playwright/unit/ai/scripts/maintenance/ mirrors the script path — correct.

Findings: Tests pass; location correct. [TOOLING_GAP]: default npx playwright test <spec> finds nothing here — Neo's custom unit config (npm run test-unit) is required; a reviewer unfamiliar with that would mis-read it as "no tests."

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — delegates to the cache-coherent GraphService.removeNodes() (the amended contract), pure exported helpers (chunk / buildCleanupPlan / assertApplyConfirmed) with DI seams, dry-run-default; no raw-SQL delete, no purgeSession mis-primitive.
• [CONTENT_COMPLETENESS]: 95 — precise @summary JSDoc on every export + a Fat-Ticket body with Deltas / Evidence / Post-Merge sections. 5 off: no inline comment naming the tenant-blind-selector / local-single-tenant assumption (the one future-guard).
• [EXECUTION_QUALITY]: 100 — ran the spec locally (6/6) at head; double-gated targeting + dry-run-default + confirm-token + edge-cascade all covered by tests; a destructive default is impossible by construction.
• [PRODUCTIVITY]: 100 — delivers the guarded script resolving #13675's scope (the operator-gated apply is the declared post-merge residual, not in-PR scope).
• [IMPACT]: 80 — clears a repo-wide graph-pollution class (3,332 orphan nodes / 248 sessions) + restores getPendingSessionSummaryCount honesty; high operational reach, not framework-core.
• [COMPLEXITY]: 60 — a single-file script with a few pure helpers + a cross-store anti-join; moderate reader load, well-decomposed.
• [EFFORT_PROFILE]: Quick Win — bounded single-file script + a focused 6-test spec; high ROI (removes a repo-wide pollution class behind a safe operator gate).

Thanks for the clean, contract-faithful turnaround — the double gate + the removeNodes delegation make the destructive path safe by construction. No required actions; eligible for the human merge gate, @tobiu. One non-blocking future-guard: an inline note on the tenant-blind selector before any multi-tenant promotion.