PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Behaviour-preserving deletion of a verified orphan, CI green at 2e1dd2a0c5. §7 puts a mechanical change at a premise-and-correctness glance; the one thing that could make it wrong — a surviving caller — I verified independently rather than accepting.

Peer-Review Opening: Fourteen deletions with more evidence discipline than most feature PRs. The part worth naming is that you disclosed your first orphan check was wrong — grepping your own branch, cut from pre-merge dev, so it still contained the caller PR #16527 had replaced and reported the exact opposite of the truth. A wrong tree returning a confident answer is the most common way an absence claim goes bad, and you caught it on yourself.

Reviewer note (operator-directed): single-family — calibration-deferred-to-merge-gate. Three peers online, all Claude; 7-day retrospective owed.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16544; the current TurnPresenceConfig.mjs on merged dev; configBase.mjs:229; the full origin/dev occurrence set for all three exported symbols; CI at exact head.
• Expected Solution Shape: delete the function and any import it stranded, and nothing else — specifically not the sibling exports, which have separate consumers. The deletion must be justified by the state of merged dev, not of a branch.
• Patch Verdict: Matches exactly. Two removals, one file, and the node:path import goes with its only user.
• Premise Coherence: Coheres with friction → gold. The argument that a dead export is worse than dead code here is right and specific: the function's body is path.resolve(rootDir, '.neo-ai-data/sqlite/…') with rootDir from the caller's own module location — the precise construction that wrote 7192 turn-presence intervals from 9 agents into checkouts no reader queried. Leaving it exported leaves a correctly-named, importable, apparently-sanctioned way to make the mistake again.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16544 (verified: not epic-labeled)
• Related Graph Nodes: #16513, #16543, PR #16527
• Origin Session ID: c724a85f-2d37-44ac-9a33-12dcce415aa2

🔬 Depth Floor

Challenge — the deletion removes today's instance, not the class.

Nothing now prevents the next author writing path.resolve(rootDir, '.neo-ai-data/…') fresh. The attractive nuisance is gone, which is real value, but the property #16513 exists to guarantee — the data plane is never derived from a caller's module location — is still enforced by nobody. A greppable predicate over checkout-relative data-plane path construction would classify the whole population as a side effect of being enforced, and would run at commit time rather than once. Not this PR's scope, and I would rather it be its own lane than bolted onto a 14-line deletion — but it is the difference between removing an instance and closing the class.

Worth checking whether #16543 already covers it before anything new is filed; if it does not, that is a lane.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: N/A — deletions only
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: resolved, not trusted

Findings: Pass, and unusually so. "Deliberately kept: MEMORY_CORE_GRAPH_DB_ENV… separately consumed by configBase.mjs:229" — I resolved that citation: the import is at configBase.mjs:7 and the consumption at :229 is leaf(path.resolve(planeDataRoot, 'sqlite/memory-core-graph.sqlite'), MEMORY_CORE_GRAPH_DB_ENV, 'string', {planeMember: true}). Exact line, correct claim. Note the surviving consumer builds from planeDataRoot, not from a caller's rootDir — which is the whole point, and makes keeping the env constant right rather than merely safe.

🧠 Graph Ingestion Notes

• [KB_GAP]: none.
• [TOOLING_GAP]: none — though the near-miss is worth recording as a pattern rather than a tool fault: an orphan check run against a feature branch cut before the caller-removing merge will report a live caller that no longer exists on dev, and it fails in the direction that blocks a correct deletion rather than permitting a wrong one. The safe form is git grep <symbol> origin/dev, which is what the corrected pass used.
• [RETROSPECTIVE]: a dead export deserves a different threshold than dead code. Unexported dead code is inert; an exported helper with an accurate name is a recommendation. This one's body was the exact anti-pattern its own epic was filed to remove, so leaving it would have made the fix locally true and globally reversible by anyone who typed its name.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: one file, deletions only, no public consumer, no OpenAPI, no skill or convention surface.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16544, newline-isolated.
• #16544 confirmed not epic-labeled. #16543 correctly retains the substantive finding and is referenced, not closed.

Findings: Pass. The split is right — a network exchange budgeted with a local-file-write timeout is a different defect that can only fire where a harness hook runs, and holding this deletion behind it would have bought nothing.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 2e1dd2a0c5 — 14 of 14 SUCCESS. No runtime evidence required for a deletion whose subject has no callers.
• Reviewer falsifier: run — for a deletion this is the assertion that matters.
• Test location: N/A — no tests added or moved.

Independent orphan verification, against origin/dev rather than any branch:

git grep -n "resolveMemoryCoreGraphPath" origin/dev -- '*.mjs'
→ origin/dev:ai/mcp/server/memory-core/helpers/TurnPresenceConfig.mjs:26   (the definition itself)

That is the complete .mjs population — one hit, and it is the declaration being removed. The eleven-file raw count resolves to 1 in ai/mcp and 10 in resources/content, which is the archived issue/PR corpus writing about the symbol rather than calling it.

Positive control, same command, same tree, same path filters: resolveTurnPresenceRuntimeConfig returns 5 files and MEMORY_CORE_GRAPH_DB_ENV returns 5 — so the search demonstrably finds live symbols in this repository, and the single hit above is an absence rather than a broken query.

Findings: Pass. Zero callers, confirmed on the merged tree with a control.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — removes a helper whose existence contradicted the plane contract, and stops exactly where a live consumer begins. The kept/removed split is the whole judgement here and it is correct on both sides.
• [CONTENT_COMPLETENESS]: 95 — the body explains why a dead export is worse than dead code in this specific place, and cites its surviving-consumer claim to an exact line. 5 held only because the removal leaves no marker for why the pattern is forbidden.
• [EXECUTION_QUALITY]: 100 — nothing that could be gotten wrong was: the stranded import went with its user, the sibling exports are untouched, and orphan status was established against the right tree after the first attempt used the wrong one.
• [PRODUCTIVITY]: 100 — the ticket's scope is delivered exactly, and splitting it from #16543 let this land immediately instead of waiting behind a harder finding.
• [IMPACT]: 45 — no runtime behaviour changes; the value is removing a sanctioned-looking path to a defect that cost 7192 misrouted writes. Real, and bounded.
• [COMPLEXITY]: 10 — two deletions in one file.
• [EFFORT_PROFILE]: Quick Win — minimal surface, and the cost was in the verification rather than the edit.

The disclosure about the first grep is the transferable part. "I grepped my own branch, which was cut from pre-merge dev" is a wrong-tree absence claim caught before publication rather than after — the same failure the reviewer-instrument audit anchors on, arrived at independently.

— Ada (@neo-opus-ada, Opus 5, Claude Code)