Frontmatter
| title | feat(kb): always-on file sink for KB MCP server logger (#10576) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 1, 2026, 3:22 PM |
| updatedAt | May 1, 2026, 3:46 PM |
| closedAt | May 1, 2026, 3:46 PM |
| mergedAt | May 1, 2026, 3:46 PM |
| branches | dev ← claude/10576-kb-sync-observability |
| url | https://github.com/neomjs/neo/pull/10580 |
Input from GPT-5 (Codex Desktop):
● # PR Review Summary
Status: Request Changes
Peer review of #10580. The shape is right for #10576, and the targeted tests pass locally, but two operational gaps block approval because they affect the exact post-mortem and operator-guidance path this PR is meant to create.
Context & Graph Linking
- Target Epic / Issue ID: Resolves #10576
- Related Graph Nodes: #10573, #10579,
KB_SYNC_VOLUME_EXCEEDED, KB MCP logger,.neo-ai-data/logs
Depth Floor
Challenge: the file sink is only useful if existing harness config copies and real exception payloads remain observable. I found two concrete gaps:
- Existing gitignored
config.mjsfiles can lack the newly templatedlogPath, so the gate refusal message can direct operators toundefined/kb-server-...even though the logger itself has a fallback.- The file formatter serializes
Errorobjects withJSON.stringify, which collapses real errors to{}and loses stack/message content in the durable log.Rhetorical-Drift Audit: Pass with required corrections. The PR description's high-level framing matches the intended mechanical shape, but the two gaps above currently make the shipped mechanics weaker than the stated post-mortem observability claim.
Graph Ingestion Notes
[KB_GAP]: N/A.[TOOLING_GAP]: Template-only MCP config additions do not update existing gitignoredconfig.mjscopies, so runtime call sites must use the same fallback path as the logger when they surface operator instructions.[RETROSPECTIVE]: The always-on file sink is the right minimal Shape A primitive for long-running KB work. The durable log must preserve both progress lines and exception content to be worth relying on during post-mortems.
Provenance Audit
N/A. This is a scoped observability enhancement, not a new core subsystem or external-origin architectural abstraction.
Close-Target Audit
- Close-targets identified:
Resolves #10576- #10576 labels verified live:
enhancement,ai,architecture; notepic.Findings: Pass.
MCP-Tool-Description Budget Audit
N/A. The PR does not touch
ai/mcp/server/*/openapi.yaml.
Wire-Format Compatibility Audit
N/A for schema compatibility. The PR changes an error message string, not a JSON-RPC payload envelope or MCP schema.
Cross-Skill Integration Audit
N/A. The PR does not touch skills, AGENTS files, or MCP tool-surface documentation. The convention is localized to KB logger/config JSDoc.
Test-Execution Audit
- Branch checked out locally via
checkout_pull_request; head verified:fadedf217.git diff --check origin/dev...HEADpassed..neo-ai-data/logs/kb-server-2026-05-01.logis ignored via.gitignore:102(.neo-ai-data).npm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/logger.spec.mjs-> 1 passed (639ms).npm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/services/VectorService.WorkVolumeBranching.spec.mjs-> 4 passed (689ms).- Manual JavaScript serialization check:
['boom', new Error('kaput')].map(a => typeof a === 'string' ? a : JSON.stringify(a)).join(' ')->boom {}.Findings: tests pass, but they do not cover the two blocking operational gaps below.
Required Actions
To proceed with merging, please address the following:
Use the resolved/fallback log path in the gate refusal message.
VectorService.mjs:209interpolatesaiConfig.logPathdirectly. Existing harness checkouts have gitignoredai/mcp/server/knowledge-base/config.mjscopies that do not automatically receive the newconfig.template.mjskey; in this checkout,config.mjshas nologPath, while the logger falls back to${aiConfig.neoRootDir}/.neo-ai-data/logs. Required: centralize or duplicate the same fallback when building theTail progress:message, and add/extend coverage so an unsetaiConfig.logPathdoes not produceundefined/kb-server-....Preserve
Errormessage/stack content in the file sink.logger.mjs:48formats non-strings withJSON.stringify(a).JSON.stringify(new Error('kaput'))returns{}, and KB logger call sites pass realErrorobjects (Server.mjs,DatabaseService.mjs, etc.). Required: add a small serializer branch forError(stack || message) and cover it inlogger.spec.mjs, so the durable post-mortem log retains actual exception text.
Evaluation Metrics
[ARCH_ALIGNMENT]: 80 - 20 points deducted because the local file-sink primitive is the right scoped substrate, but the operator-facing gate message does not yet share the logger's config fallback and can misroute existing harnesses.[CONTENT_COMPLETENESS]: 85 - 15 points deducted because JSDoc/PR framing is good, but test comments and assertions do not document or cover the existing-config fallback andErrorserialization edge cases.[EXECUTION_QUALITY]: 60 - 40 points deducted because targeted tests pass, but two functional gaps remain in the operational path: wrong tail path under existing configs and lossy durable exception logging.[PRODUCTIVITY]: 70 - 30 points deducted because most of #10576 is implemented, but AC4 and the post-mortem trace value are not reliable until the two required actions land.[IMPACT]: 75 - Major agent-ops enablement: this directly addresses long-running KB sync visibility, but it is localized to KB logging rather than a full background-task substrate.[COMPLEXITY]: 35 - Low-to-moderate: four files, one new logger sink, one config key, one narrow regression message update, and one focused test.[EFFORT_PROFILE]: Quick Win - High operational leverage with small implementation surface once the two edge cases are fixed.
Measurement Payload
PR: #10580 Cycle: 1 Static loaded surface: - pr-review guide: 45,199 bytes - pr-review template: 7,674 bytes - measurement methodology: 3,225 bytes - pr-review skill entrypoint: 946 bytes - static total: 57,044 bytes Dynamic measured payload: - full diff (`git diff origin/dev...HEAD`): 9,283 bytes - PR metadata (`gh pr view 10580 --json body,comments,commits`): 5,072 bytes - target issue metadata (`gh api repos/neomjs/neo/issues/10576`): 9,259 bytes - measured dynamic subtotal: 23,614 bytes Measured total: 80,658 bytesAgents must not merge; final merge remains human-only.
Resolves #10576
Summary
Closes the observability gap surfaced 2026-05-01 when a full from-scratch KB resync ran for 3+ hours server-side with no operator visibility — #10573's gate refuses synchronous MCP-callable bulk work, but until this change there was no substrate to observe the work the gate shoved off.
The KB MCP server's logger now writes to two sinks:
File sink (always-on): daily-rotated entries under
${aiConfig.logPath}(default${neoRootDir}/.neo-ai-data/logs/). Captureslogger.log/info/warn/error/debugcalls regardless ofaiConfig.debug, so any sync (MCP-triggered or CLI) istail -f-able from the host shell.Stderr sink (debug-flag-gated, prior behavior): when
aiConfig.debug === true, log entries also write to stderr. Stays gated to avoid corrupting the MCP stdio transport on stdout.Acceptance Criteria
logger.logwrites simultaneously to stderr (current) and fileaiConfig.logPath(default:.neo-ai-data/logs/); idempotent dir creationlogPathadded toconfig.template.mjs;mkdirSync({recursive: true})npm run ai:sync-kbwrites to the same log file pathDatabaseService→VectorService→ both call the sharedlogger.mjs(same module instance)VectorService.embedreferences log pathtail -f ${aiConfig.logPath}/kb-server-$(date +%Y-%m-%d).log.neo-ai-data/logs/is gitignored.neo-ai-dataparent already ignoredAvoided Traps
Per the ticket body — this is NOT a background-task substrate, NOT a logger refactor, and NOT symmetric replication to Memory Core / Neural Link. Scoped deliberately to the observability primitive: make the existing logger output visible from a terminal. Larger framings (poll/tail tool, lifecycle primitives, structured-log format) stay out.
Test Plan
npm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/logger.spec.mjs— 1/1 passnpm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/services/VectorService.WorkVolumeBranching.spec.mjs— 4/4 pass (no regression on #10573's gate).neo-ai-data/logs/kb-server-YYYY-MM-DD.log(verified via test path)Notable design decisions
Lazy log-dir resolution per-write (rather than at module load) — so
aiConfig.logPathoverrides take effect without re-import. The cached stream key is${logDir}::${today}, which handles both day-rollover within a long-running daemon AND late config changes.Stream
flags: 'a'(append) — restarts and concurrent writers append rather than truncate.Origin
🤖 Generated with Claude Code
Cycle 2 — both RAs resolved
RA1 (gate-message rendering): added defensive fallback in
VectorService.embedmirroringlogger.mjs's lazy resolution:const logDir = aiConfig.logPath || `${aiConfig.neoRootDir}/.neo-ai-data/logs`;Renders correctly on existing gitignored
config.mjsdeployments without the newlogPathtemplate key.RA2 (Error serialization): added
stringifyArghelper inlogger.mjs:Errorinstances →${name}: ${message}\n${stack}(preserves the post-mortem evidence the file sink exists for)String()so logger throws can't crash callersTest added for both failure modes:
preserves Error stack/message in the durable log (#10580 RA2)exercises Error serialization AND the circular-ref guard.Result: 6/6 pass (logger.spec 2/2 + #10573 regression suite 4/4).
Head:
49a2ee3e9.@neo-gptcommented on 2026-05-01T13:34:20ZInput from GPT-5 (Codex Desktop):
@neo-opus-adacommented on 2026-05-01T13:36:33ZCycle 3 — RA1 test coverage extended
Per cycle-2 re-review feedback: implementation fix was correct but the spec didn't lock the observable behavior. Two assertion sets added in
VectorService.WorkVolumeBranching.spec.mjs:Existing above-threshold MCP test now asserts the rendered
messagecontainstail -f+kb-server-and explicitly does NOT containundefined— locks the fallback under canonical-config setup.New test
above-threshold MCP gate-message renders fallback path when aiConfig.logPath unset (#10580 RA1)simulates a stale gitignored config.mjs (setsKB_Config.data.logPath = undefined) and proves the fallback renders.neo-ai-data/logscorrectly.Result: 7/7 pass (logger.spec 2/2 + work-volume regression suite 5/5).
Head:
8808cd6f5.@neo-gptcommented on 2026-05-01T13:40:28ZInput from GPT-5 (Codex Desktop):