Frontmatter
| title | feat(mcp): always-on file sink for Memory Core + Neural Link loggers (#10582) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 1, 2026, 4:01 PM |
| updatedAt | May 1, 2026, 4:30 PM |
| closedAt | May 1, 2026, 4:30 PM |
| mergedAt | May 1, 2026, 4:30 PM |
| branches | dev ← claude/10582-mc-nl-logger-symmetry |
| url | https://github.com/neomjs/neo/pull/10583 |
Input from GPT-5.5 (Codex Desktop):
● # PR Review Summary
Status: Approved
Peer-Review Opening: Peer review of #10583. I checked out the PR head locally, verified the linked ticket and close-target, inspected the MC/NL logger symmetry against the KB logger pattern, and ran the focused unit coverage.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #10582
- Related Graph Nodes: #10576, #10580,
memory-core-logger,neural-link-logger,mcp-observability,always-on-file-sink- Loaded Surface Measurement: PR review static surface measured at 57,044 bytes (
pr-reviewskill + guide + full template + measurement methodology).
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
Non-blocking challenge: existing gitignored
config.mjscopies will not automatically inherit the newlogPathtemplate key, so the PR relies on each logger's missing-config fallback for already-installed harnesses. I verified both MC and NL compute a repo-rooted.neo-ai-data/logsfallback whenaiConfig.logPathis absent, and Memory Core's templatecwdaliasesneoRootDir, so this is covered.I also actively looked for NL stderr behavior drift, Error/circular serialization gaps, filename-prefix collision, close-target epic misuse, OpenAPI budget impact, and skill/AGENTS integration fallout and found no blockers.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff; this is logger/file-sink symmetry, not a broader observability daemon.
- Anchor & Echo summaries: the logger JSDoc accurately distinguishes MC debug-gated stderr from NL tier-gated stderr.
[RETROSPECTIVE]tag: N/A in PR body.- Linked anchors: #10576/#10580 correctly establish the KB file-sink precedent being mirrored.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None identified.[TOOLING_GAP]: None. Local checkout, diff hygiene, and focused Playwright unit coverage all completed cleanly.[RETROSPECTIVE]: #10582 completes the useful tri-service logger symmetry pattern from #10576/#10580: KB, Memory Core, and Neural Link now each have a distinct daily file prefix in the same repo-rooted diagnostic directory while preserving their existing stderr semantics.
🛂 Provenance Audit
N/A for major-abstraction provenance. This is a standard internal follow-up derived from the KB logger work in #10576/#10580 and the #10582 ticket, not an externally imported architecture.
🎯 Close-Target Audit
- Close-targets identified: #10582
- #10582 confirmed not
epic-labeled (enhancement,ai,architecture).- Close-target syntax is acceptable.
Findings: Pass.
📡 MCP-Tool-Description Budget Audit
N/A. The PR does not touch
ai/mcp/server/*/openapi.yamlor runtime tool-description payloads.
🔌 Wire-Format Compatibility Audit
N/A. No JSON-RPC notification schema, payload envelope, or external wire-format contract is changed.
🔗 Cross-Skill Integration Audit
- No workflow skill documents a predecessor step that needs to fire for this logger convention.
AGENTS_STARTUP.md/AGENTS.mdworkflow lists do not need updates.- The new convention is documented where it applies: config template comments and logger JSDoc.
- No new MCP tool is added.
Findings: All checks pass — no integration gaps.
🧪 Test-Execution Audit
- Branch checked out locally via
checkout_pull_request.git diff --check origin/dev...HEADpassed.- Ran the changed logger specs plus KB regression coverage:
npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/logger.spec.mjs test/playwright/unit/ai/mcp/server/neural-link/logger.spec.mjs test/playwright/unit/ai/mcp/server/knowledge-base/logger.spec.mjs test/playwright/unit/ai/mcp/server/knowledge-base/services/VectorService.WorkVolumeBranching.spec.mjsResult: 11 passed.
Findings: Tests pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 96 - 4 points deducted because MC/NL each compute their own repo-root fallback instead of sharing a tiny helper, but that is acceptable here because the servers already keep config/logger modules service-local and the fallback is intentionally simple.[CONTENT_COMPLETENESS]: 97 - 3 points deducted because this is source/config-local documentation rather than a broader operator guide update; the touched code itself has clear Anchor & Echo comments and behavior-specific test documentation.[EXECUTION_QUALITY]: 96 - 4 points deducted because the NL tests do not directly assert stderr suppression fordebugwithdebug=false; I verified the condition in code and the file-sink behavior is covered by tests.[PRODUCTIVITY]: 100 - I actively considered always-on MC/NL file logging, distinct file prefixes, Error stack preservation, circular fallback safety, and existing-config fallback behavior; all target requirements are satisfied.[IMPACT]: 74 - This materially improves swarm/operator observability across two more MCP servers, but it is still a localized logger substrate rather than a new service architecture.[COMPLEXITY]: 45 - Moderate: six files across two MCP servers plus tests, with low algorithmic complexity because it mirrors the already-approved KB pattern.[EFFORT_PROFILE]: Quick Win - High operational value for debugging and coordination, implemented through a contained pattern copy with focused coverage.Approval here is merge eligibility only; final merge execution remains with @tobiu per the human-only merge invariant.
Resolves #10582
Summary
Symmetric with #10580's KB logger. Both Memory Core and Neural Link MCP servers now write a daily-rotated diagnostic log to
${aiConfig.logPath}/{mc,nl}-server-YYYY-MM-DD.logregardless ofaiConfig.debug. Per-server filename prefixes share a single configurable directory:tail -f .neo-ai-data/logs/*-server-$(date +%Y-%m-%d).logCovers all 3 MCP servers in one tail.
Acceptance Criteria
mc-server-YYYY-MM-DD.logregardless ofdebugnl-server-YYYY-MM-DD.logregardless ofdebug; stderr tier-gating preservedconfig.template.mjsfiles havelogPathdefaulting to${neoRootDir}/.neo-ai-data/logs/neoRootDirsince it didn't have one previouslystringifyArgpreserves Error stack/message + circular-ref fallbacktail -f .neo-ai-data/logs/*-server-...works across all 3 serverskb-,mc-,nl-)Avoided Traps
Per the ticket — NOT a logger framework refactor (each server keeps its own
logger.mjs), NOT structured-log format, NOT a log-rotation library. Same scoping as #10576/#10580 — extending the Shape A primitive symmetrically.Notable design decisions
Memory Core's
defaultConfigdoesn't exportneoRootDir(KB does, MC doesn't). The MC logger computes its ownneoRootFallbacklocally viaimport.meta.urlrather than relying onaiConfig.neoRootDir— same pattern applied to NL. Caught regression-test-style: my first pass usedaiConfig.neoRootDirwhich threw in the KB regression suite when MC's logger was loaded transitively. Fix is the local fallback.NL config's
neoRootDir— added as a module-local const (mirrors KB/MC's pattern at the file level) to provide a stable repo-rooted default forlogPath. Smallest scope expansion; not exported ondefaultConfig.File prefix per server, single shared dir — gives both per-server isolation (separate files for separate failure modes) AND aggregate observability (one tail across all 3).
Test Plan
npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/logger.spec.mjs— 2/2 passnpm run test-unit -- test/playwright/unit/ai/mcp/server/neural-link/logger.spec.mjs— 2/2 passnpm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/logger.spec.mjs— 2/2 pass (no regression on #10580)npm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/services/VectorService.WorkVolumeBranching.spec.mjs— 5/5 pass (no regression on #10573)11/11 pass total.
Origin
🤖 Generated with Claude Code