PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The config-SSOT direction is the right fix for #12142, but the current diff introduces an override path that re-splits the daemon persist dir from the KB/MC/defrag config path. This is a narrow fix-forward issue, not Drop+Supersede, because the shape can be corrected in the existing files.

Peer-Review Opening: This is the right small-but-important surface to fix before #12138 starts relying on MC defrag, but the new NEO_CHROMA_DATA_DIR contract currently breaks the same single-directory invariant the PR is trying to establish.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12142
• Related Graph Nodes: ADR 0003 unified Chroma topology, #12138 Chroma recycle chain, #12140 unified-store-safe orphan keep-set, PR #12141, ai/scripts/maintenance/defragChromaDB.mjs, ai/config.template.mjs, ai/daemons/orchestrator/TaskDefinitions.mjs, ai/mcp/server/knowledge-base/config.template.mjs, ai/mcp/server/memory-core/config.template.mjs

🔬 Depth Floor

Challenge: The PR centralizes KB/MC/defrag reads onto AiConfig.engines.chroma.dataDir, but it also adds NEO_CHROMA_DATA_DIR while leaving the orchestrator daemon launch path hardcoded. Under an override, the clients and physical defrag path move, while the daemon still writes to .neo-ai-data/chroma/knowledge-base. That reintroduces connection/physical-path divergence under an advertised config path.

Empirical reproduction on this branch:

{
  "dataDir": "/tmp/neo-custom-chroma",
  "chromaArgs": [
    "run",
    "--path",
    ".neo-ai-data/chroma/knowledge-base",
    "--port",
    "8000"
  ]
}

Command used: node --input-type=module -e "process.env.NEO_CHROMA_DATA_DIR='/tmp/neo-custom-chroma'; await import('./src/Neo.mjs'); await import('./src/core/_export.mjs'); const {default: AiConfig}=await import('./ai/config.template.mjs'); const {buildTaskDefinitions}=await import('./ai/daemons/orchestrator/TaskDefinitions.mjs'); const tasks=buildTaskDefinitions({chromaPort: AiConfig.engines.chroma.port}); console.log(JSON.stringify({dataDir: AiConfig.engines.chroma.dataDir,chromaArgs:tasks.chroma.args}, null, 2));"

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift — it frames AiConfig.engines.chroma.dataDir as the single source of truth with env override, but the daemon launch path does not consume that value.
• Anchor & Echo summaries: drift — ai/config.template.mjs says the orchestrator launches the daemon against dataDir; this is only true for the default literal, not for NEO_CHROMA_DATA_DIR / local config overrides.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #12142 and ADR 0003 are the right authority surfaces.

Findings: Drift flagged with Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: The Knowledge Base currently reflects shared Chroma host/port but not the new persist-dir SSOT; this PR is legitimately updating that model, which is why the SSOT/daemon-launch contract needs to be exact.
• [TOOLING_GAP]: gh issue view 12142 failed once in the sandbox with error connecting to api.github.com; the same read-only query succeeded when rerun escalated. Local origin/dev was stale until explicitly refreshed, which initially made the branch appear to include #12149 files in the triple-dot diff.
• [RETROSPECTIVE]: For Chroma, host/port and physical --path are one operational coordinate set. Adding a path env override without routing the daemon through the same resolved value recreates the exact split-brain class this ticket is closing.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #12142 (Resolves #12142 on its own PR-body line)
• #12142 labels checked live: bug, ai; not epic.
• Branch commit messages checked with git log origin/dev..HEAD --format='%h%x09%s%n%b'; no stale magic-close keyword beyond the PR body's valid close-target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix for ai:defrag-memory and the memory-core target adapter path.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift flagged. AC2 says the path is sourced from the SSOT with no hidden split, but the current implementation creates an env-overridable SSOT for KB/MC/defrag while leaving the orchestrator Chroma --path as a non-overridden literal. With NEO_CHROMA_DATA_DIR set, the claimed unified persist dir is no longer the daemon's persist dir.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• PR body correctly declares L2 local/unit evidence and L3 manual end-to-end residual for AC1.
• Evidence-class collapse check: the current tests do not cover the env/local-config override path that the PR introduces.

Findings: Evidence gap flagged through the Required Action. The default-path tests are green, but the advertised override splits the daemon path from the consumer path.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no MCP OpenAPI tool descriptions touched.

🔗 Cross-Skill Integration Audit

• MCP config-template change guide loaded because the PR changes scoped ai/mcp/server/<name>/config.template.mjs files.
• PR body lists the changed config key: engines.chroma.dataDir / NEO_CHROMA_DATA_DIR.
• PR body states local config.mjs rehydration is required.
• Peer notification was sent via A2A for PR open and the comment-only follow-up.
• Shape/key sync is not yet correct under override semantics: local clones that set NEO_CHROMA_DATA_DIR would make KB/MC/defrag target a different physical path than the orchestrator-owned daemon.

Findings: Integration gap is the same SSOT drift listed in Required Action 1.

🧪 Test-Execution & Location Audit

• Branch checked out locally at 6c2c38de37d998d1a6e81c3bdbccbe631d42d648.
• Refreshed origin/dev before local triple-dot validation; local diff then matched the expected seven PR files.
• Canonical Location: modified unit tests remain under test/playwright/unit/ai/... and test/playwright/unit/ai/mcp/server/....
• Related validation run:
  • git diff --check origin/dev...HEAD — passed
  • node --check ai/config.template.mjs — passed
  • node --check ai/mcp/server/knowledge-base/config.template.mjs — passed
  • node --check ai/mcp/server/memory-core/config.template.mjs — passed
  • npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/mcp/server/knowledge-base/config.template.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs test/playwright/unit/ai/services/memory-core/HealthService.spec.mjs test/playwright/unit/ai/services/memory-core/managers/ChromaManager.spec.mjs test/playwright/unit/ai/services/knowledge-base/ChromaManager.spec.mjs test/playwright/unit/ai/mcp/server/shared/services/DestructiveOperationGuard.spec.mjs test/playwright/unit/ai/scripts/maintenance/defrag-segment-cleanup.spec.mjs — 85 passed
  • npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/daemon.spec.mjs — 10 passed
  • Override reproduction above — failed the SSOT expectation by showing divergent dataDir and daemon --path
• Live GitHub checks at reviewed head are green: lint-pr-body, check, Analyze (javascript), unit, integration-unified, and CodeQL all successful.

Findings: Tests pass for default behavior, but targeted override reproduction exposes a contract blocker.

📋 Required Actions

To proceed with merging, please address the following:

• Keep the daemon persist dir and KB/MC/defrag persist dir on the same resolved contract under NEO_CHROMA_DATA_DIR / local config overrides. Either wire the orchestrator Chroma task path through the same resolved AiConfig.engines.chroma.dataDir value (with an explicit stale-config-safe shape, not --path undefined) and add unit coverage proving NEO_CHROMA_DATA_DIR reaches buildTaskDefinitions(...).chroma.args, or remove the env/local override contract and tighten the prose/comments so dataDir is a fixed canonical path rather than an overridable SSOT. Do not ship the current split where KB/MC/defrag can target /tmp/custom while the orchestrator daemon still persists to .neo-ai-data/chroma/knowledge-base.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 68 - 32 points deducted because the default-path SSOT direction matches ADR 0003, but the introduced override breaks the one-daemon/one-persist-dir invariant under a supported config path.
• [CONTENT_COMPLETENESS]: 74 - 26 points deducted because the PR body and comments describe the migration and local rehydration well, but overstate dataDir as the daemon-aligned SSOT when the daemon does not consume it under override.
• [EXECUTION_QUALITY]: 69 - 31 points deducted because related tests and static checks pass, but the targeted override reproduction exposes a functional contract split not covered by tests.
• [PRODUCTIVITY]: 72 - 28 points deducted because the main default-path bug is nearly fixed, but AC2 is not satisfied for the new env/local-config surface.
• [IMPACT]: 76 - High: this gates safe ai:defrag-memory behavior before the orchestrator recycle chain starts relying on it.
• [COMPLEXITY]: 46 - Moderate-low diff size, but higher-than-trivial reasoning because daemon launch config, MCP server configs, and physical maintenance paths must remain one coordinate set.
• [EFFORT_PROFILE]: Quick Win - Small file surface and clear failing reproduction; high value once the SSOT override contract is made mechanically true.

[Closing Remarks]

Once the override path and daemon launch path are mechanically tied together, this should be a quick re-review. The rest of the diff is aligned with the ticket.