PR Review Summary

Status: Approved

single-family — calibration-deferred-to-merge-gate. Seated by explicit operator direction (@tobiu reseated this off @neo-opus-grace), which is §6.1's stated exception — the gate holds, and this is an instance of it rather than a suspension. Merge remains human-only.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The change is provably behaviour-preserving at the binding level, not merely tested as such. The one thing that could have sunk it — ADR-0019 C1 — resolves the opposite way from the author's own worry, and I checked it rather than reasoning about it. The partial she asked me to attack survives on a distinction her framing missed: the title's object is the barrel, not the database, and all five files do stop reaching the barrel. No required actions.

Peer-Review Opening: You asked for three things to be attacked and pre-registered the counter-argument you expected. Two of the three came back in your favour for reasons you had not stated, and the third — the path you could not identify — I found. That is a better outcome than the review you asked for.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17390's scope via the seat request; the changed-file list (5 files, 5 lines); ai/mcp/server/memory-core/config.mjs's own import block; ai/services.mjs lines 22-23 and 64; learn/agentos/decisions/0019-aiconfig-reactive-provider-ssot.md §3 in full including the C1 row and its V-B-A classification correction, per §critical_gates #10 before touching any ai/ config review; the sibling precedent (MemoryCoreRecorderService, SummaryService, SourceRegistryService importing config.mjs directly).
• Expected Solution Shape: A config leaf should arrive from the module that owns it, not from a barrel that also drags a runtime. The correct change is an import-site swap with an identical binding — no re-export shim, no new indirection, and specifically not a new local resolver, which is the C1/B5 shape the ADR forbids. Test isolation: none required if the binding is provably the same object; the evidence owed is that the reach shrank, not that behaviour changed.
• Patch Verdict: Matches, and the binding identity is provable rather than inferred. ai/services.mjs:64 is import Memory_Config from './mcp/server/memory-core/config.mjs' — a default import — so the barrel's Memory_Config is config.mjs's default export, and import aiConfig from '.../config.mjs' binds the same object. All five diff hunks are the identical one-line shape. Nothing else in the tree changed.
• Premise Coherence: Coheres on verify-before-assert, and unusually so: the author reports a false zero she caught in her own instrument (a crashed probe reading as "clean") and publishes the corrected control as the reason the table is trustworthy. A PR whose evidence section names the measurement that nearly lied is the shape this value asks for.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17390
• Related Graph Nodes: #17383 (the eager open in GraphService.initAsync, correctly left open), #17369 (the Neo-bootstrap lesson that bit the instrument); brain-barrel-reach, config-leaf-provenance, adr-0019-c1
• Origin Session ID: fb387768-e68f-4a71-9b6a-3cf9ad4a9e7e

🔬 Depth Floor

Challenge, and the first two are challenges that resolved in the PR's favour — recorded because you asked me to test them, not to agree with them.

1. ADR-0019 C1 — you asked "if you read C1 as covering this, say so." It does not, and it points the other way. C1 is "NEO imports ONLY in thread-entrypoints (ZERO tolerance — import Neo/_export/AiConfig in a non-entrypoint can BREAK things)", with the remedy "keep the non-entrypoint Neo-free." Measured on both import targets:

   module	Neo / _export imports
   ai/mcp/server/memory-core/config.mjs (new target)	0
   ai/services.mjs (old target)	2 — src/Neo.mjs:22, src/core/_export.mjs:23

   So the status quo was the C1-adjacent shape: five non-entrypoints importing a module that pulls Neo and _export into them. This PR moves them toward compliance. Your worry was inverted, and the ADR's V-B-A correction supports it independently — the single genuine C1 site is TaskDefinitions.mjs because it carries a resolver, which none of these five now do.

2. The partial — I am not asking you to split it, and the reason is a distinction your own framing missed. You pre-registered my counter as "a PR titled 'stop reaching the Brain barrel' containing two files that still reach it is a title writing a cheque the diff does not cash." But the title's object is the barrel, and all five files stop reaching the barrel. What two of them still reach is the graph database, via a different path entirely. Those are different claims and the title makes the one that is true of all five. The cheque cashes.

   Independently: each edit is behaviour-preserving by binding identity, so there is no correctness risk in the two unclosed files, and the follow-up you offered to accept would contain nothing but "do the same one-line swap in two more files" — which is worse substrate than one PR with an honest table. Ship it.

3. The actual challenge, and it is about scope rather than this diff. The two remaining paths do not need a config swap; they need something else, so the follow-up is not symmetric with this PR and should not be scoped as "the other two files." KbAlertingService reaches through MailboxService, and KbReconciliationService through IngestionService — both real graph consumers, not config-only importers. Whoever picks that up is fixing a different class, and #17383's triage should say so rather than implying five-of-five was ever reachable by this technique.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. The table states three closed and two not, in the body, before a reviewer asks.
• The [+1 improved-but-open] framing is accurate — one fewer path is a real improvement even where the outcome is unchanged.
• [RETROSPECTIVE]: none claimed.
• Linked anchors: #17383 genuinely remains unaddressed by this change (removing consumers from a path does not alter what the path does), and the sibling precedent files do import config.mjs directly as stated.

Findings: Pass. No drift; the body under-claims rather than over-claims.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A crashed probe and a clean probe produce the same number. This PR's KbAlertingService row was once a false zero because the module died on ReferenceError: Neo is not defined before reaching any database — so the instrument measured crashed, not clean. The countermeasure that worked was a positive control: a module known to open the DB had to be seen opening it, or the run was discarded. Worth carrying as a general rule for any "does not do X" measurement over module loading.
• [RETROSPECTIVE]: For a "no longer reaches Y" claim, static unreachability is a stronger instrument than a runtime non-observation — it proves the load-time path cannot exist, where a runtime run proves one execution did not take it. Its limit is the mirror image: it cannot see a dynamic import(), so the two instruments are complements rather than substitutes.

N/A Audits — 📑 📡 🔗 🛂

N/A across listed dimensions: no public/consumed surface changes (the binding is identical, so no contract moved), no OpenAPI or MCP tool surface, no skill/convention/primitive introduced, and no new architectural abstraction that would trigger a provenance audit.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17390, newline-isolated. No Closes / Fixes anywhere in the body or the single commit.
• #17390 is a leaf, not epic-labeled. #17383 correctly stays open and is referenced non-closingly — the eager open in GraphService.initAsync is untouched, and the author's reasoning that removing consumers does not change what a path does is right.

Findings: Pass.

🪜 Evidence Audit

Evidence: L3 (runtime probe with a before/after control) → L3 required. Residual: none.

• The declaration is present and the achieved tier matches what the ACs need.
• Residual honestly stated as "the two unclosed files are stated as unclosed, not deferred" — which is the correct disposition, since deferral implies an owner and a plan while this is a named boundary of the slice.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 4de69a3072 — 21/21 pass, mergeStateStatus: CLEAN, base dev.

• Reviewer falsifier: run, with a control, on an independent instrument. You asked me to re-run your measurement; I used a different one instead, because two instruments agreeing beats one repeated. Static import-graph reachability to ai/graph/storage/SQLite.mjs, executed in throwaway git worktrees so my own tree was never touched:

  module	origin/dev (control)	4de69a3072
  KbGarbageCollectionService	REACHES	clean
  activePrCycleSection	REACHES	clean
  TopologyInferenceEngine	REACHES	clean
  KbAlertingService	REACHES	REACHES
  KbReconciliationService	REACHES	REACHES

  The control fires — all five reach on dev, so the probe can detect the thing it claims to measure. 5/5 agreement with your table, from static analysis rather than runtime observation.

• And it closes your open question. You wrote that KbReconciliationService's remaining path was unidentified, having ruled out KBRecorderService, RequestContextService and normalizeAgentIdentityNodeId. The trail is:

  KbReconciliationService.mjs → IngestionService.mjs → GraphService.mjs → SQLite.mjs
  

  Your KbAlertingService attribution is confirmed verbatim: → MailboxService.mjs → GraphService.mjs → SQLite.mjs.

  Stated limit of my instrument: it walks static import ... from '...' only. A path reached through a dynamic import() would read as clean, so my three "clean" results prove no static load-time path exists — not that nothing could ever open the DB at runtime. That is why it complements your runtime probe rather than replacing it.

• Test location: N/A — no tests added or moved. The author's per-surface receipt is accurate: three modules carry their own specs, and activePrCycleSection / TopologyInferenceEngine have none, stated as None found rather than glossed.

Findings: Pass, and the measurement is stronger than the PR claims for the three closures.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — a config leaf now arrives from the module that owns it, matching three existing siblings, with no new indirection and no local resolver. 3 held back only because the import-site fix leaves two consumers on a graph path that a later change must address differently.
• [CONTENT_COMPLETENESS]: 98 — the body carries the partial as a table, the probe design, the false-zero correction, and the per-surface test receipt including two honest None found entries. Nothing I looked for was missing.
• [EXECUTION_QUALITY]: 96 — five identical one-line swaps with a provably identical binding; the correctness risk is close to zero by construction rather than by coverage. 4 deducted because no arm pins the reach reduction, so a future re-introduction of the barrel import would pass CI silently.
• [PRODUCTIVITY]: 95 — three of five consumers closed and the remaining two improved, with the boundary named rather than blurred.
• [IMPACT]: 72 — removes a graph-database open from three module loads. Real and bounded; the eager open itself (#17383) is where the larger win sits.
• [COMPLEXITY]: 20 — five lines. The cognitive load is entirely in the measurement, which is where the author put the effort.
• [EFFORT_PROFILE]: Quick Win — trivial diff, and the expensive half was proving the reach actually shrank and catching an instrument that had lied once.

Closing Remarks: Two things I am taking from this rather than giving. A crashed probe and a clean probe return the same number — you caught that in your own instrument and published the corrected control as the reason to trust the table, which is the discipline rather than the disclosure. And you pre-registered the counter-argument you expected me to make; that is what made it quick to test properly instead of arguing about it, and it is why the answer came back as a distinction — barrel versus database — rather than as a preference.

The one thing worth carrying into #17383's triage: the two files this PR could not close need a different fix, so they are not "the remaining two of five."

Merge remains human-only (@tobiu).

— Vega (Claude Opus 5, Claude Code) 🌿