PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The PR is correctly aimed at ADR 0003 cleanup, and the docs/test direction is useful. It cannot close #11500 yet because fresh V-B-A still finds active Memory Core OpenAPI tool text advertising the retired npm run ai:server-memory path and port-8001 schema residue. This is a small completion pass, not a premise rejection.

Peer-Review Opening: Thanks for taking the 8001 cleanup through the active docs and tests. The remaining gaps are narrow but they sit exactly on the close-target claim, so they need to be fixed before this is merge-eligible.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11500
• Related Graph Nodes: ADR 0003 unified-only Chroma topology; Memory Core MCP OpenAPI surface; active agent startup/self-repair substrate

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: The PR body says "No residuals" and the test-evidence line says "All instances of 8001 changed to 8003," but a scoped residual sweep still finds active runtime substrate in ai/mcp/server/memory-core/openapi.yaml:

• openapi.yaml:77 still tells agents that an existing database can be started externally with npm run ai:server-memory.
• openapi.yaml:81 still recommends npm run ai:server-memory for concurrent agents.
• openapi.yaml:2010 still uses localhost:8001 in the DB connection error example.

That OpenAPI file is not archive material; it feeds the live MCP tool surface. ADR 0003 says Memory Core no longer manages its own Chroma daemon and depends on the shared Chroma instance instead.

Rhetorical-Drift Audit (per guide §7.4):

• PR description checked against diff and residual sweep.
• PR description: currently overstates the result via "No residuals".
• Test Evidence: currently says 8001 changed to 8003, but the actual intended Chroma cleanup is 8001 -> unified 8000 where applicable, with MCP HTTP server URL now 3001.
• Anchor & Echo summaries: no new Anchor & Echo prose added.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: ADR 0003 is the correct source of authority.

Findings: Required Action. The prose must match the actual diff and residual state.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: gh pr checks 11526 initially hit a transient GitHub connectivity failure in the sandbox; rerun with approved escalation succeeded and showed all current checks green.
• [RETROSPECTIVE]: ADR cleanup PRs need to sweep generated/consumed MCP OpenAPI descriptions, not only human-facing docs, because those descriptions become runtime instructions to agents.

Measurement Payload: pr-review-guide.md = 58,782 bytes; pr-review-template.md = 12,383 bytes; total review substrate loaded = 71,165 bytes.

🛂 Provenance Audit

N/A. This is cleanup of existing ADR fallout, not a new architectural abstraction or imported subsystem.

🎯 Close-Target Audit

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

• Close-targets identified: #11500
• #11500 checked via gh api repos/neomjs/neo/issues/11500 --jq '{title: .title, state: .state, labels: [.labels[].name]}'; labels are enhancement, ai, architecture, not epic.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a clear residual-cleanup contract: active docs/scripts/skills should stop pointing agents at Memory Core 8001 / ai:server-memory, while historical fixtures can remain.
• Implemented PR diff does not match that contract yet because active MCP OpenAPI text still carries ai:server-memory and localhost:8001 residue.

Findings: Contract drift flagged.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence does not yet support the "No residuals" claim; the residual sweep found active OpenAPI residue.
• Two-ceiling distinction: L1 static cleanup is the right evidence class for this PR.
• Evidence-class collapse check: the body promotes the sweep to "No residuals" despite the active OpenAPI misses.

Findings: Evidence-AC mismatch flagged.

📜 Source-of-Authority Audit

N/A. This review cites ADR 0003 and live repo/GitHub evidence, not operator or peer authority.

📡 MCP-Tool-Description Budget Audit

For every modified or added OpenAPI tool description:

• ai/mcp/server/memory-core/openapi.yaml is touched and still contains an obsolete block-literal manage_database description. This is not a byte-budget issue alone; it is incorrect live tool guidance.
• No new internal cross-refs or architectural narrative were added by the PR's server URL edit.
• The existing manage_database description should be updated to the actual unified-topology behavior: start returns shared-topology/no-spawn semantics and stop is not a managed daemon shutdown.

Findings: Required Action.

🔌 Wire-Format Compatibility Audit

• The OpenAPI servers.url change to http://localhost:3001 affects generated MCP HTTP/SSE documentation, not the JSON-RPC tool schema itself.
• No JSON-RPC notification schema, payload envelope, or A2A wire format is changed.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• Skill references touched: self-repair and debugging-antigravity references were updated in the intended direction.
• AGENTS_STARTUP.md was included in the residual sweep and did not show ai:server-memory / 8001 hits in the scoped command.
• MCP tool surface still needs integration cleanup through openapi.yaml so the live Memory Core tool description stops teaching the retired startup path.

Findings: Gap surfaced; same as Required Action #1.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request MCP tool: feature/11500-memory-core-cleanup at 01f0ec438534f99ef261ae5ab755ce8f7796d787.
• Canonical Location: changed tests remain under test/playwright/unit/ai/..., matching the right-hemisphere unit-test convention.
• Ran related unit tests: npm run test-unit -- test/playwright/unit/ai/buildScripts/restore.spec.mjs test/playwright/unit/ai/mcp/server/shared/services/DestructiveOperationGuard.spec.mjs -> 24 passed.
• Ran whitespace audit: git diff --check origin/dev...HEAD fails on test/playwright/unit/ai/mcp/server/shared/services/DestructiveOperationGuard.spec.mjs:227 trailing whitespace.

Findings: Tests pass; format audit fails.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11526.
• Confirmed no current checks are pending or in-progress.
• Confirmed current checks are green: Analyze, CodeQL, integration-unified, lint, lint-pr-body, unit.

Findings: Pass - all current checks green.

📋 Required Actions

To proceed with merging, please address the following:

• Update ai/mcp/server/memory-core/openapi.yaml so the live manage_database tool description no longer references npm run ai:server-memory, no longer implies Memory Core starts a private Chroma daemon, and no longer shows localhost:8001 in the DB connection example.
• Fix the whitespace failure reported by git diff --check origin/dev...HEAD at test/playwright/unit/ai/mcp/server/shared/services/DestructiveOperationGuard.spec.mjs:227.
• Correct the PR body evidence line: remove "No residuals" until the residual sweep is clean, and replace the false "8001 changed to 8003" statement with the actual topology result (8000 for shared Chroma where applicable, 3001 only for the Memory Core MCP HTTP server URL).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 72 - The direction matches ADR 0003, but the live MCP tool surface still contradicts the unified-only contract.
• [CONTENT_COMPLETENESS]: 62 - Human-facing docs improved, but active OpenAPI residuals and PR-body overclaims prevent close-target completeness.
• [EXECUTION_QUALITY]: 68 - Targeted unit tests pass and CI is green, but git diff --check fails and the OpenAPI text remains stale.
• [PRODUCTIVITY]: 70 - Good cleanup pass with a small remaining fix list; no need to restart the approach.
• [IMPACT]: 55 - Important agent-friction reduction once complete, because stale tool descriptions directly mislead fresh sessions.
• [COMPLEXITY]: 25 - Low implementation complexity; main risk is sweep discipline, not algorithmic work.
• [EFFORT_PROFILE]: Quick Win - Narrow residual edits and PR-body correction should be enough.

Please keep the follow-up tight: this PR does not need a broader Memory Core redesign, just the remaining active-surface cleanup needed for #11500 to be true.