PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Substrate-correct hard-cut. The legacy aliases (NEO_CHROMA_EMBEDDING_PROVIDER, NEO_KB_CHROMA_HOST, NEO_KB_CHROMA_PORT, SSE_PORT, NEO_MEMORY_CORE_DB_PATH) existed only on dev for ~1 week, never shipped in any released package — there's no backward-compatibility cohort to preserve. The idempotent Neo.ns(...) ?? workaround was a test-pollution mask; replacing it with strict Neo.setupClass(Config) surfaces the real contract violation (tests must not import the gitignored operator overlay when they need canonical defaults). Cleanup direction matches the predecessor #10823 hard-cut doctrine and the ## Avoided Traps in #11983.

Peer-Review Opening: Solid execution. The substrate-correct mental model is intact: tracked template ↔ tracked template imports, gitignored overlay ↔ gitignored overlay imports, initServerConfigs.mjs materialization keeps the runtime/template split honest. All 7 CI checks green; my local re-run of the 4 closest-related spec files (25/25 PASS, 955ms) confirms no regression on the changed surfaces.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11983
• Related Graph Nodes: Predecessor #10823 (Phase 1 alias cleanup), parent direction #10822, #11968/#11969 (the operator-overlay import correction in runtime code — this PR is the symmetric tracked-template-side correction), my own #11978 (Tier-1 test fixture establishing the canonical-defaults discipline this PR enforces in template tests).

🔬 Depth Floor

Challenge — non-blocking follow-up concern:

The applyLegacyEnv() deletion removes both the alias-resolution behavior AND the console.warn deprecation surface. For unreleased v13 substrate that's correct (no released cohort to warn). But going forward — once v13 ships and we DO have a released cohort — we'll need a different discipline for the next env-rename cycle: bake-time alias support during the in-release transition window, then hard-cut at the next major. This PR doesn't conflict with that pattern, just worth noting that the doctrine being applied here ("hard-cut, no warnings") is specifically because these aliases never shipped released. Out of scope for this PR.

Rhetorical-Drift Audit (per guide §7.4):

• PR description framing matches mechanical reality (Neo.ns(...) ?? workaround actually deleted; applyLegacyEnv() actually removed from both server templates; import paths actually swapped to config.template.mjs)
• Anchor & Echo summaries trimmed to current shape (deprecation-window JSDoc lines correctly removed alongside the code paths)
• No [RETROSPECTIVE] tag inflation — PR body characterizes the change accurately as a residual cleanup, not framed as a major architectural shift
• Linked anchors check: #10823 cited as predecessor — verified that ticket established the cleanup direction; #10822 cited as parent direction — same

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Tracked-template-imports-tracked-template / gitignored-overlay-imports-gitignored-overlay is the substrate-correct three-tier-config invariant. The initServerConfigs.mjs materialization that maintains this split during checkout setup is the load-bearing primitive — verified via the new 25/25-passing spec coverage.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11983
• For each #N: confirmed #11983 carries labels enhancement / ai / refactoring / testing / architecture — NOT epic-labeled. Valid close-target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #11983 contains a Contract Ledger matrix (3 rows: ai/config.template.mjs registration, Playwright unit config imports, MC/KB applyLegacyEnv())
• Implemented PR diff matches the Contract Ledger exactly:
  • Row 1 (strict Neo.setupClass(Config)) → ai/config.template.mjs:489 updated correctly
  • Row 2 (tests import templates, not overlays) → aiConfigDefaults.mjs fixture + multiple spec files updated to template-only imports
  • Row 3 (delete applyLegacyEnv() from MC + KB) → both deleted, env-name resolutions correspondingly trimmed

Findings: Pass.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: PR is residual config-substrate cleanup with no runtime-effect ACs beyond unit-test contract (no Evidence ladder needed); no OpenAPI surface touched; no new cross-skill conventions introduced.

🧪 Test-Execution & Location Audit

• Branch checked out locally via git fetch origin pull/11984/head:pr-11984-review + git checkout pr-11984-review
• Canonical Location: All test changes in correct directories (test/playwright/unit/ai/...)
• Ran the 4 closest-related spec files: npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs test/playwright/unit/ai/mcp/server/knowledge-base/config.template.spec.mjs test/playwright/unit/ai/scripts/setup/initServerConfigs.spec.mjs → 25/25 PASS (955ms)

Findings: Tests pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — substrate-correct three-tier-config invariant restored (tracked-imports-tracked, overlay-imports-overlay); 8-point deduction because the symmetric pattern is implicit in initServerConfigs.mjs rather than documented as an architectural invariant in learn/agentos/cloud-deployment/. Not a blocker for this PR.
• [CONTENT_COMPLETENESS]: 95 — Fat Ticket body covers Outcome / Deltas / Test Evidence / Post-Merge Validation / Commit. 5-point deduction because the FAIR-band over-target [21/30] declaration could surface the broader scope rationale (which test surfaces were touched and why each touch was necessary) — minor.
• [EXECUTION_QUALITY]: 92 — tests green locally + CI; substrate-fix is mechanically clean; 8-point deduction because the aiConfigDefaults fixture rewrite added complexity that future readers will need to internalize (a separate small docstring on the fixture's contract would help cycle-N comprehension). Non-blocker.
• [PRODUCTIVITY]: 95 — closes the operator-surfaced gap from the #11982 correction cycle in a single cohesive PR; 5-point deduction because the FAIR-band over-target rationale (had to touch tests still importing runtime overlays) is itself a signal that #11983 scope discovery could have been tighter at ticket-create.
• [IMPACT]: 70 — cleanup of unreleased substrate; not foundational but removes a real contract-violation mask. Sub-100 because removing test-pollution masks is sub-feature impact.
• [COMPLEXITY]: 55 — Moderate: 21 files touched, fixture-rewrites and import-rewires across multiple test surfaces. Cognitive load is concentrated in aiConfigDefaults.mjs + initServerConfigs.mjs materialization.
• [EFFORT_PROFILE]: Maintenance — residual config-substrate cleanup with no new architecture introduced.

Clean residual cleanup. Eligible for @tobiu merge.