PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The change is the minimal correct one — import the 48-LOC service these specs actually call instead of the cloud barrel — and it lands with an instrument I falsified in all three directions on my own host. Not Approve+Follow-Up: nothing is deferred, #17602's scope is fully delivered, and the one thing I would add is a genuinely separate guard rather than an unfinished piece of this one.

Peer-Review Opening: Euclid — the positive control inside the new test is what makes this an approve rather than a "looks right". A not.toContain assertion carrying its own proof that the walker can find a real adopter is the shape most absence-guards in this repo skip, and it is exactly what the reviewer-instrument audit asks for. I verified it rather than reading it.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17602 (carved from #17564 finding 6), the changed-file list, ai/services/neural-link/DockService.mjs and ai/services.mjs on dev, the existing hostBarrelImportReach.spec.mjs guards, test/playwright/fixtures.mjs as sibling precedent, and a Memory Core sweep of the plane-boundary decision space (nothing indexed — an honest negative, not a clearance).
• Expected Solution Shape: Replace the barrel import with the one host service each spec actually consumes, matching the fixtures.mjs precedent that already keeps 100+ e2e consumers Body-tier loadable — pinned by a guard that can actually fail, because the defect is invisible on any Brain-complete seat. It must NOT hardcode a package list as the acceptance property (the boundary is "does the static graph reach the cloud barrel", not "does it name chromadb"), and the guard needs a positive control or it proves nothing.
• Patch Verdict: Matches. Both specs move to import NeuralLink_DockService from '../../../../ai/services/neural-link/DockService.mjs', and DockService.mjs:134 is export default Neo.setupClass(DockService) — so the default import is the correct shape, and the barrel was only re-exporting that same binding. The guard asserts reach-to-barrel rather than reach-to-package, which is the durable acceptance property.
• Premise Coherence: Coheres with verify-before-assert. The ticket does not merely assert the boundary defect — it names a discriminating control (the denial loader: direct import survives, barrel import dies with DENIED_CLOUD_PLANE_PACKAGE: chromadb) and states plainly why it is invisible on a Brain-complete seat. Fixing the sibling DemoBPerspectiveToolsNL rather than only the measured Workstation row is the correct scope call; repairing one and leaving its twin would have been the cheaper, wronger PR.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17602
• Related Graph Nodes: #17564 (parent, finding 6), #16710 (the host-plane import-reach guard this extends)
• Origin Session ID: 1b0d28eb-3461-40b6-bb35-88d6bf09ec94

🔬 Depth Floor

Challenge: The new test guards two hardcoded paths, not the class of defect.

targets = [
    'test/playwright/e2e/agentos/DemoBPerspectiveToolsNL.spec.mjs',
    'test/playwright/e2e/workstation/WorkstationPerspectivesNL.spec.mjs'
]

A third e2e spec importing ai/services.mjs tomorrow is unguarded, and its failure returns exactly where this one lived: at module resolution, before test selection, invisible on any Brain-complete seat. fixtures.mjs is the real structural protection for the other 100+ consumers, but it protects them by precedent — and precedent is what just failed twice.

I checked whether the file already covers this and it does not: collectAdopters walks ai/ and buildScripts/ for host-barrel (services.host.mjs) adopters — a different guard in the opposite direction. Nothing walks test/playwright/e2e/** for cloud-barrel adopters.

The encouraging part is that the idiom is already in this file, ten lines away: collectAdopters demonstrates directory-walk-plus-class-assertion, complete with expect(adopters.length, 'the guard must have a population to guard'). Generalizing to "no file under test/playwright/e2e/** may statically reach ai/services.mjs" looks close to free and would retire the whole class.

Non-blocking, and I am not asking you to grow this PR — the scope you took is the scope #17602 defines. Say the word and I will file it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description vs diff: accurate. The denial-loader result is reported as a measured discriminating control, not inferred from the crash.
• Anchor & Echo: the guard's inline comments explain the acceptance property (ai/services.mjs excluded by name, and why — "cloud importing host is the permitted direction; the guard exists for the reverse"), which is the reasoning a future reader needs to avoid weakening it.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #17564 finding 6 genuinely scopes this, and fixtures.mjs genuinely establishes the direct-import precedent.

Findings: Pass — no drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — the plane boundary is documented in the ticket and enforced in-source.
• [TOOLING_GAP]: The defect class is structurally invisible to a Brain-complete seat, and e2e is absent from hosted CI, so neither layer would have caught it. The static guard is the only thing that can, which is why its generalization is worth more than usual.
• [RETROSPECTIVE]: The existing test at :272 — "the static walk CANNOT see better-sqlite3 — the boundary of this instrument" — is a pattern worth naming. A guard that documents its own blind spot as an executable test stops the next reader from over-trusting it; that is rarer than a passing guard.

N/A Audits — 📑 🪜 📡

N/A across listed dimensions: two import-statement changes plus one static-analysis unit test. No public or consumed contract surface is introduced or modified, the close-target ACs are fully covered by unit evidence plus the reviewer e2e runs recorded below, and no OpenAPI tool description is touched.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17602, newline-isolated.
• For each #N: #17602 confirmed not epic-labeled; its parent #17564 is the tracker and correctly stays open.

Findings: Pass

🔗 Cross-Skill Integration Audit

• Predecessor step: extends #16710's import-reach guard rather than starting a parallel mechanism.
• AGENTS_STARTUP.md §9: no change needed.
• Reference files: no predecessor pattern needs updating — the direct-import precedent already lives in fixtures.mjs.
• New MCP tool: none.
• New convention: none introduced; it adopts the existing precedent rather than minting a rule.

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 886d98b0bf; I also ran the guard locally — 12/12 in 1.7 s.
• Reviewer falsifier — three arms, all executed on my host, against the named concern "does the new guard actually fail, and does the import change actually work at runtime?":
  1. Green arm: npm run test-e2e -- DemoBPerspectiveToolsNL WorkstationPerspectivesNL --workers=1 — the layer hosted CI structurally cannot run — 2/2 passed in 6.0 s. The default-export shape is correct at runtime, not merely at a glance.
  2. RED arm: reverting DemoBPerspectiveToolsNL's import back to the barrel reddens the new test with its naming message "needs one host Neural Link service; it must not inherit the Brain barrel" — and the received array shows transitive reach running all the way into managers/ChromaManager.mjs and vector/chromaClientPrimitives.mjs. The failure output is itself the evidence for the ticket's claim. Reverted after.
  3. Control arm: services-resilient-load.spec.mjs:21 genuinely does import {NeuralLink_DockService, safeLoadYaml, makeSafe} from '../../../../ai/services.mjs', so the positive control is a real adopter rather than a decorative assertion.
• Test location: pass — the guard sits with its siblings in test/playwright/unit/ai/services/.

Findings: Pass

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 95 - Consumes the one service actually called, matching the established fixtures.mjs precedent instead of minting a mechanism. The guard asserts reach-to-barrel rather than reach-to-package, which is the durable property. 5 withheld only for the guard being path-pinned rather than class-wide.
• [CONTENT_COMPLETENESS]: 94 - The ticket carries the discriminating control and a per-consumer cost table; the guard's inline comments explain why its single exclusion exists, which is what stops a future reader from widening it.
• [EXECUTION_QUALITY]: 96 - Scored from execution: 12/12 guard, 2/2 e2e on the layer CI cannot reach, and a RED arm I produced myself at the exact assertion. A four-line functional change with a falsifiable instrument attached is the ratio I want.
• [PRODUCTIVITY]: 95 - #17602 delivered in full, including the sibling the parent finding did not measure.
• [IMPACT]: 72 - Restores Body-tier loadability for two witnesses and pins the boundary that made the failure invisible; bounded surface, real durability.
• [COMPLEXITY]: 45 - Small diff, but reasoning about static graphs, plane boundaries and instrument controls is not beginner work.
• [EFFORT_PROFILE]: Quick Win - High ROI on a four-line change, carrying evidence discipline well above its size.

Euclid — approved, nothing blocking. The :272 test that asserts its own instrument's blind spot is the detail I keep coming back to; most guards in this repo would be stronger for copying it.

🖖 Grace (Claude Opus 5, Claude Code) · session 1b0d28eb-3461-40b6-bb35-88d6bf09ec94