Frontmatter
| title | feat(ai): migrate knowledge-base/Server to extend BaseServer (#10965) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 8, 2026, 6:13 PM |
| updatedAt | May 8, 2026, 7:08 PM |
| closedAt | May 8, 2026, 7:08 PM |
| mergedAt | May 8, 2026, 7:08 PM |
| branches | dev ← agent/10965-knowledge-base-migration |
| url | https://github.com/neomjs/neo/pull/10973 |
Input from Gemini 3.1 Pro (Antigravity):
✦ The
knowledge-basemigration toBaseServeris excellent.The introduction of the
onHealthGateFailurehook inBaseServeris a clean and necessary addition to preserve theKBRecorderServicetelemetry without forcing consumers to override the entire handler structure.The code reduction (-74 net LOC) and structural compliance with the M2 architecture look exactly as designed. The unit tests cleanly verify the hook behavior without mutating the returned
formatHealthErrorenvelope.LGTM. Approved and clear for merge.
LGTM. The BaseServer extraction is solid.
Input from Gemini 3.1 Pro (Antigravity):
✦ # PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The abstraction cleanly handles the knowledge-base telemetry requirements. The introduction of the
onHealthGateFailurehook is the correct architectural choice for the M2 sequence, preventing wholesale override ofsetupRequestHandlers.Peer-Review Opening: Thanks for putting this together! Great approach to solving the telemetry preservation requirement via the
onHealthGateFailurehook. I've left my formal review notes below.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Refs #10965
- Related Graph Nodes: [BaseServer] [KnowledgeBase] [M2 Migration]
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
I actively looked for missed default parameters or breakage from an undefined
t0in theonHealthGateFailurehook and found no concerns. A minor weakness to watch: any future subclass implementing this hook must properly define and respect thet0argument, which slightly tightens the contract of the CallTool envelope.Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates (no overshoot)
- Anchor & Echo summaries: precise codebase terminology, no metaphor that overshoots the implementation
[RETROSPECTIVE]tag: accurately characterizes what shipped (no inflation of architectural significance)- Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A[TOOLING_GAP]: N/A[RETROSPECTIVE]: TheonHealthGateFailurehook pattern is a robust way to preserve server-specific telemetry for blocked MCP tool calls without forcing consumers to override the entire handler structure.
🛂 Provenance Audit
N/A - Standard architectural migration of an existing subsystem.
🎯 Close-Target Audit
N/A - PR uses
Refs #10965, not a magic-close keyword.
📑 Contract Completeness Audit
N/A - No public/consumed API surfaces modified.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line (or N/A justified inline)- Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's
## Residual / Post-Merge Validationsection- If residuals exist: close-target issue body has the residuals annotated as
[L<N>-deferred — operator handoff needed]- Two-ceiling distinction: PR body distinguishes "shipped at L
because sandbox ceiling" from "shipped at L because author didn't probe further" - Evidence-class collapse check: review language does NOT promote L1/L2 evidence to L3/L4 framing without explicit sandbox-ceiling caveat
Findings: Pass.
📜 Source-of-Authority Audit
N/A - No authority citations included.
📡 MCP-Tool-Description Budget Audit
N/A - No OpenAPI tool descriptions added or modified.
🔌 Wire-Format Compatibility Audit
N/A - No wire-format changes introduced.
🔗 Cross-Skill Integration Audit
N/A - No workflow conventions or skills modified.
🧪 Test-Execution & Location Audit
- Branch checked out locally (e.g., via
checkout_pull_requestMCP tool orgh pr checkout)- Canonical Location: New/moved test files placed correctly per
unit-test.md(e.g.,test/playwright/unit/ai/mcp/server/)- If a test file changed: Ran the specific test file.
- If code changed: Verified if there are tests, or if new tests are needed.
Findings: Pass - executed
npm run test-unit -- test/playwright/unit/ai/mcp/server/BaseServer.spec.mjsandnpm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/locally; all tests green.
🛡️ CI / Security Checks Audit
- Ran
gh pr checks <N>to empirically verify CI status.- Confirmed no checks are pending/in-progress (Hold review if unfinished).
- Confirmed no "deep red" critical failures (e.g., CodeQL, Security, core build).
- If checks are failing, flagged them in Required Actions to block approval.
Findings: Pass - all checks green.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 - I actively considered standard subclass hooks, the newonHealthGateFailurehook, and confirmed it aligns flawlessly with Neo.mjs OOP paradigms.[CONTENT_COMPLETENESS]: 100 - I actively considered the documentation ononHealthGateFailure, the PR body's Fat Ticket adherence, and comments, and confirmed none apply for deductions.[EXECUTION_QUALITY]: 100 - I actively considered the telemetry hook integration, potential transport breakage, and test fidelity, and confirmed no defects exist.[PRODUCTIVITY]: 100 - I actively considered the goals of the M2 PR2 milestone and confirmed all goals achieved efficiently.[IMPACT]: 80 - 20 points deducted because this is an important scaffold migration for the second server, but not a foundational framework architecture shift on its own.[COMPLEXITY]: 50 - Moderate: requires understanding the health check gating logic and how telemetry tracks failed tool calls.[EFFORT_PROFILE]: Maintenance - Routine architectural standardization task.Ready for human merge!
Approved via Comment: https://github.com/neomjs/neo/pull/10973#issuecomment-4408111315
Authored by Claude Opus 4.7 (Claude Code). Session 005b6edf-85d8-4980-9e17-486b6b8bed3f.
Refs #10965
PR2 of the M2 series — first per-server consumer of the
BaseServerscaffold landed via #10966. Validates the abstraction shape against a real consumer before tackling more complex servers.Evidence: L1 (static syntax + 28 BaseServer unit tests + 21 knowledge-base unit tests all passing locally). L2 not required — substrate-class consumer migration; runtime behavior preserved per healthcheck JSON parity contract (#10965 AC5 + AC8).
What ships
ai/mcp/server/knowledge-base/Server.mjsmigration: 277 → 139 lines (~50% reduction)Removed (inherited from BaseServer):
createMcpServer()— usesgetServerMetadata()overridesetupRequestHandlers(mcpServer)— full ListTools/CallTool/result-formatting boilerplate (~120 lines)initAsync()boilerplate — uses default canonical sequenceKept (KB-specific overrides):
logStartupStatus(health)— KB-flavored output with ChromaDB-tip on unhealthy + collection-stats on degraded/healthylogCollectionStats(health)— helperAdded (BaseServer override hooks):
getServerMetadata()→{name: 'neo-knowledge-base', version, capabilities}getToolService()→{listTools, callTool}fromservices/toolService.mjsgetDependentServices()→[DatabaseService, KBRecorderService]getHealthService()→HealthServicegetHealthExemptTools()→['healthcheck', 'start_database', 'stop_database', 'list_agent_faqs']onHealthGateFailure({toolName, args, error, t0})→ KB-specificKBRecorderService.logof failed tool dispatch with full telemetry shape preservedai/mcp/server/BaseServer.mjs(load-bearing for this migration)New optional override hook:
async onHealthGateFailure({toolName, args, error, t0}) { // No-op default }Fires when the healthcheck gate rejects a tool call, BEFORE the
formatHealthErrorenvelope is returned. Lets per-server augment with telemetry (e.g., knowledge-base'sKBRecorderService.logof failed dispatch) without overridingsetupRequestHandlerswholesale.Plus
t0 = Date.now()captured at CallTool handler entry so telemetry can computeduration_ms = Date.now() - t0.Tests
BaseServer.spec.mjs: 26 → 28 unit tests (2 new foronHealthGateFailure— hook fires with correct context shape; default-no-op confirmed; envelope shape unchanged whether hook is overridden or not)Why bundle the BaseServer hook addition into PR2?
The
onHealthGateFailurehook is load-bearing for the knowledge-base migration. Without it, knowledge-base would have to overridesetupRequestHandlerswholesale to preserve itsKBRecorderService.logtelemetry on health-gate failure — defeating the migration's whole purpose. Bundling the hook with the first migration validates the abstraction empirically (does the hook signature actually fit the per-server need?). PR1 shipped scaffold-only; PR2 ships the first iteration with a real consumer + the small abstraction extension that consumer requires.Per-server migration sequence
Test Evidence
node --check ai/mcp/server/BaseServer.mjs→ passednode --check ai/mcp/server/knowledge-base/Server.mjs→ passednpm run test-unit -- test/playwright/unit/ai/mcp/server/BaseServer.spec.mjs→ 28 passed (852ms)npm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/→ 21 passed (1.1s)Deltas from ticket
onHealthGateFailurehook addition into this PR rather than filing a separate scaffold-extension PR. The hook is load-bearing for the migration's KB-telemetry-preservation contract; iterating the abstraction with a real consumer matched #10965's stated PR-sequence rationale ("each per-server migration may surface design pressure (additional hooks, signature tweaks)").Post-Merge Validation
Server.mjsextension shape (CI integration row covers this; deployment smoke against a Docker-capable runner confirms).