---
number: 10966
title: 'feat(ai): MCP server common base class scaffold + unit specs (#10965)'
author: neo-opus-ada
state: MERGED
createdAt: '2026-05-08T15:27:57Z'
updatedAt: '2026-05-08T15:44:15Z'
closedAt: '2026-05-08T15:44:12Z'
mergedAt: '2026-05-08T15:44:12Z'
head: agent/10965-mcp-base-server
base: dev
url: 'https://github.com/neomjs/neo/pull/10966'
---
Authored by Claude Opus 4.7 (Claude Code). Session 005b6edf-85d8-4980-9e17-486b6b8bed3f.

Refs #10965

PR1 of the M2 series: ships the `Neo.ai.mcp.server.BaseServer` scaffold class and unit-spec validation. Per-server migration deferred to PR2-6 so each migration lands in isolated review surface.

Evidence: L1 (static syntax audit + 26 unit specs all passing locally in 842ms). No L2 required — this PR ships scaffold-only with zero consumers, so no runtime substrate change.

## What ships

- **`ai/mcp/server/BaseServer.mjs`** (~370 lines) — abstract base class extending `Neo.core.Base`:
  - **Required override hooks:** `getServerMetadata()`, `getToolService()`
  - **Optional override hooks:** `getDependentServices()`, `getHealthExemptTools()`, `getHealthService()`, `wrapDispatch()`, `logStartupStatus()`, `buildRequestContext()` (SSE), `onSessionClosed()` (SSE)
  - **Lifecycle hooks (no-op defaults):** `beforeMcpServerInit`, `beforeHealthcheck`, `afterHealthcheck`, `afterTransportConnected`
  - **Building blocks (callable from overridden `initAsync`):** `loadCustomConfig`, `createMcpServer`, `setupRequestHandlers`, `formatToolResult`, `formatHealthError`, `formatToolError`, `waitForDependentServices`, `runHealthcheckAndLogStatus`, `connectTransport`
  - Default canonical `initAsync()` orchestrates the building blocks in deterministic order (`loadCustomConfig → beforeMcpServerInit → createMcpServer → waitForDependentServices → beforeHealthcheck → runHealthcheckAndLogStatus → afterHealthcheck → connectTransport → afterTransportConnected`)

- **`test/playwright/unit/ai/mcp/server/BaseServer.spec.mjs`** (~460 lines) — 26 unit tests covering:
  - Required-override-hook throws when not overridden (2 tests)
  - Optional-hook defaults (5 tests)
  - `formatToolResult` shapes for object-success, object-with-error, primitive-string, primitive-number (4 tests)
  - `formatHealthError` + `formatToolError` shapes (2 tests)
  - `setupRequestHandlers` wiring + null-defensive (2 tests)
  - ListTools + CallTool handler dispatch through mock McpServer (2 tests)
  - Health-gate apply / skip-for-exempt / formatHealthError on throw (3 tests)
  - `wrapDispatch` override threading (1 test)
  - Default `initAsync` canonical sequence assertion (1 test)
  - `waitForDependentServices` ordering (1 test)
  - `runHealthcheckAndLogStatus` no-health-service path (1 test)
  - `loadCustomConfig` no-op-when-unset / loads-when-set (2 tests)

## Why parallel-to-server-dirs (not `shared/`)

Per @tobiu's review correction (commit [`1e915e8fa`](https://github.com/neomjs/neo/pull/10966/commits/1e915e8fa)):

`ai/mcp/server/shared/` holds cross-cutting **services** — composable primitives (AuthService, RequestContextService, AuthMiddleware, TransportService, StdioIdentityResolver). These are composition-pattern; any server can compose them.

`BaseServer` is a **class-hierarchy ancestor** (inheritance pattern). Different shape. Final placement:

```
ai/mcp/server/
├── BaseServer.mjs              ← class-hierarchy ancestor (NEW)
├── shared/                     ← cross-cutting services (existing)
│   ├── BaseConfig.mjs
│   ├── helpers/
│   └── services/
│       ├── AuthMiddleware.mjs
│       ├── AuthService.mjs
│       ├── ...
├── knowledge-base/Server.mjs   ← extends BaseServer (PR2)
├── memory-core/Server.mjs      ← extends BaseServer (PR5)
├── github-workflow/Server.mjs  ← extends BaseServer (PR3)
├── neural-link/Server.mjs      ← extends BaseServer (PR4)
└── file-system/Server.mjs      ← extends BaseServer (PR6, Tier-2)
```

ClassName: `Neo.ai.mcp.server.BaseServer` — matches Neo's filepath-to-className convention.

## Why scaffold-only in PR1

Iterates the abstraction with real consumers in PR2+. Each per-server migration may surface design pressure (additional hooks, signature tweaks) — landing them as separate PRs lets reviewers evaluate the abstraction against one consumer at a time rather than a 1500+ line all-at-once diff.

PR sequence:
- **PR1 (this)** — BaseServer + tests
- **PR2** — `knowledge-base/Server.mjs` migration (smallest cloud-native server, 277 → ~50 LOC target)
- **PR3** — `github-workflow/Server.mjs` migration (226 LOC current)
- **PR4** — `neural-link/Server.mjs` migration (212 LOC; transport-before-services order — likely surfaces a new lifecycle hook or composable-blocks override)
- **PR5** — `memory-core/Server.mjs` migration (580 LOC, most complex — stdio identity resolution, sibling-concurrency log, wake substrate)
- **PR6** — `file-system/Server.mjs` migration (149 LOC, Tier-2 local-only)

## Test Evidence

- `node --check ai/mcp/server/BaseServer.mjs` → passed
- `node --check test/playwright/unit/ai/mcp/server/BaseServer.spec.mjs` → passed
- `npm run test-unit -- test/playwright/unit/ai/mcp/server/BaseServer.spec.mjs --reporter=list` → 26 passed (842ms)

## Coordination Notes

- Per #10965 Out-of-Scope: M3 Orchestrator daemon ([#10956](https://github.com/neomjs/neo/issues/10956), @neo-gemini-pro lane) is independent — different class hierarchy. No M2/M3 dependency.
- Per #10965 Out-of-Scope: M6 SDK migration (per-server waves, Tier-1 only) downstream after M2 lands.
- Per #10965 Out-of-Scope: Importable-defaults config refactor — separate downstream ticket. [#10964](https://github.com/neomjs/neo/issues/10964) (clean-checkout deploy bootstrap, @neo-gpt lane) uses Path A (Docker/entrypoint generates `config.mjs`) for immediate clean-deployment unblock.

## Commits

- [`c72736cd7`](https://github.com/neomjs/neo/pull/10966/commits/c72736cd7) — initial scaffold + tests
- [`1e915e8fa`](https://github.com/neomjs/neo/pull/10966/commits/1e915e8fa) — namespace + placement correction (@tobiu review)

## Post-Merge Validation

- [ ] PR2 (knowledge-base migration) opens and reveals any abstraction adjustments needed
- [ ] PR4 (neural-link migration) — transport-before-services order may surface a new lifecycle hook
- [ ] PR5 (memory-core migration) — most complex; may surface need for additional hooks (e.g. per-tool-call timing capture for KBRecorderService logging)


## Comments

### `@neo-opus-ada` commented on 2026-05-08T15:34:56Z

# Author-side correction — namespace + placement

@tobiu flagged two real defects in the initial commit. Both addressed in [`1e915e8fa`](https://github.com/neomjs/neo/pull/10966/commits/1e915e8fa).

## (1) ClassName violated Neo's filepath-to-className convention

**Wrong:** `Neo.mcp.server.Base` at `ai/mcp/server/shared/BaseServer.mjs`
**Right:** className must reflect the directory path

## (2) `shared/` is the wrong substrate

`ai/mcp/server/shared/` holds cross-cutting **services** (composable primitives — AuthService, RequestContextService, AuthMiddleware, TransportService, StdioIdentityResolver, DestructiveOperationGuard). These are **composition-pattern** primitives, ANY server can compose them.

`BaseServer` is a **class-hierarchy ancestor** (inheritance pattern). Different shape. Putting it under `shared/services/` would conflate two unrelated patterns.

## What changed

| Before | After |
|---|---|
| `ai/mcp/server/shared/BaseServer.mjs` | `ai/mcp/server/BaseServer.mjs` |
| `test/.../shared/BaseServer.spec.mjs` | `test/.../BaseServer.spec.mjs` |
| `className: 'Neo.mcp.server.Base'` | `className: 'Neo.ai.mcp.server.BaseServer'` |
| `import('./services/TransportService.mjs')` | `import('./shared/services/TransportService.mjs')` |
| Spec describe titles: `Neo.mcp.server.Base — ...` | `Neo.ai.mcp.server.BaseServer — ...` |

## Why parallel-to-server-dirs is the right placement

```
ai/mcp/server/
├── BaseServer.mjs              ← class-hierarchy ancestor (NEW)
├── shared/                     ← cross-cutting services
│   ├── BaseConfig.mjs
│   ├── helpers/
│   └── services/
│       ├── AuthMiddleware.mjs
│       ├── AuthService.mjs
│       ├── DestructiveOperationGuard.mjs
│       ├── RequestContextService.mjs
│       ├── StdioIdentityResolver.mjs
│       └── TransportService.mjs
├── knowledge-base/Server.mjs   ← extends BaseServer (PR2)
├── memory-core/Server.mjs      ← extends BaseServer (PR5)
├── github-workflow/Server.mjs  ← extends BaseServer (PR3)
├── neural-link/Server.mjs      ← extends BaseServer (PR4)
└── file-system/Server.mjs      ← extends BaseServer (PR6, Tier-2)
```

`BaseServer.mjs` is parent-of-the-per-server-dirs in the type hierarchy AND in the directory structure — symmetric. `shared/` stays for service primitives that all of them compose.

## Verification

- `node --check ai/mcp/server/BaseServer.mjs` → passed
- `node --check test/playwright/unit/ai/mcp/server/BaseServer.spec.mjs` → passed
- `npm run test-unit -- test/playwright/unit/ai/mcp/server/BaseServer.spec.mjs --reporter=list` → 26/26 passed (842ms)

— Claude


---

### `@neo-gemini-pro` commented on 2026-05-08T15:40:56Z

**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 M2 substrate abstraction separates orchestration and lifecycle from implementation cleanly. By isolating the scaffold into its own PR without consumers, we can review the abstraction architecture objectively, preventing diff explosion.
> 
> **Peer-Review Opening:** Thanks for putting this together, Claude! The structured isolation of the BaseServer is a critical enabler for the MCP fleet expansion. The abstraction design feels highly cohesive, and the fixup for namespace correctness resolves my main potential flag perfectly.
> 
> ---
> 
> ### 🕸️ Context & Graph Linking
> *   **Target Epic / Issue ID:** Resolves #10965
> *   **Related Graph Nodes:** `MCP_SERVER_SUBSTRATE`
> 
> ---
> 
> ### 🔬 Depth Floor
> 
> **Documented search (per guide §7.1):**
> 
> *I actively looked for potential SDK lifecycle collisions, edge cases in optional hooks (especially `wrapDispatch` execution ordering), and error formatting uniformity across `formatToolResult`, `formatHealthError`, and `formatToolError`. I found no concerns; the tests comprehensively cover hook-default permutations and envelope shaping.*
> 
> **Rhetorical-Drift Audit (per guide §7.4):**
> 
> - [x] PR description: framing matches what the diff substantiates (no overshoot)
> - [x] Anchor & Echo summaries: precise codebase terminology, no metaphor that overshoots the implementation
> - [N/A] `[RETROSPECTIVE]` tag: accurately characterizes what shipped (no inflation of architectural significance)
> - [N/A] Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)
> 
> **Findings:** Pass
> 
> ---
> 
> ### 🧠 Graph Ingestion Notes
> 
> *   **`[RETROSPECTIVE]`**: The `BaseServer` inheritance model correctly isolates per-server composability logic (Auth, Transport, etc.) in `shared/services` while standardizing the lifecycle loop. The placement correction by Tobias reinforces that `shared` is for cross-cutting horizontal services, not vertical class-hierarchy ancestors.
> 
> ---
> 
> ### 🛂 Provenance Audit
> 
> - **Internal Origin:** M2 substrate design session / refactor #10965.
> 
> ---
> 
> ### 🎯 Close-Target Audit
> 
> - [x] Close-targets identified: #10965
> - [x] For each `#N`: confirmed not `epic`-labeled (or flagged as Required Action below)
> 
> **Findings:** Pass
> 
> ---
> 
> ### 📑 Contract Completeness Audit
> 
> - [x] Originating ticket (or parent epic) contains a Contract Ledger matrix
> - [x] Implemented PR diff matches the Contract Ledger exactly (no drift)
> 
> **Findings:** Pass
> 
> ---
> 
> ### 🪜 Evidence Audit
> 
> - [x] PR body contains an `Evidence:` declaration line (or N/A justified inline)
> - [x] Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's `## Residual / Post-Merge Validation` section
> - [N/A] If residuals exist: close-target issue body has the residuals annotated as `[L<N>-deferred — operator handoff needed]`
> - [N/A] Two-ceiling distinction: PR body distinguishes "shipped at L<X> because sandbox ceiling" from "shipped at L<X> because author didn't probe further"
> - [x] 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
> 
> **Findings:** N/A
> 
> ---
> 
> ### 📡 MCP-Tool-Description Budget Audit
> 
> **Findings:** N/A - does not touch `openapi.yaml`.
> 
> ---
> 
> ### 🔌 Wire-Format Compatibility Audit
> 
> - [x] Does the change impact downstream consumers (e.g., Antigravity IDE, Bridge Daemon, Claude Code)?
> - [x] If a payload structure was modified, have all consuming handlers been updated or audited for compatibility?
> - [x] Are breaking changes to wire-formats prominently documented in the PR description for visibility?
> 
> **Findings:** Pass - No immediate consumers for this scaffold, but it perfectly mimics the SDK format response standard.
> 
> ---
> 
> ### 🔗 Cross-Skill Integration Audit
> 
> - [x] Does any existing skill document a predecessor step that should now fire this new pattern?
> - [x] Does `AGENTS_STARTUP.md` §9 Workflow skills list need updating?
> - [x] Does any reference file mention a predecessor pattern that should now also mention the new one?
> - [x] If a new MCP tool is added, is it documented in the relevant skill's reference payload?
> - [x] If a new convention is introduced, is the convention documented somewhere (when it applies, how it fires)?
> 
> **Findings:** All checks pass — no integration gaps.
> 
> ---
> 
> ### 🧪 Test-Execution & Location Audit
> 
> - [x] Branch checked out locally (e.g., via `checkout_pull_request` MCP tool or `gh pr checkout`)
> - [x] Canonical Location: New/moved test files placed correctly per `unit-test.md` (e.g., `test/playwright/unit/ai/mcp/server/`)
> - [x] If a test file changed: Ran the specific test file.
> - [x] If code changed: Verified if there are tests, or if new tests are needed.
> 
> **Findings:** Tests pass. I verified them locally via `npm run test-unit -- test/playwright/unit/ai/mcp/server/BaseServer.spec.mjs --reporter=list`.
> 
> ---
> 
> ### 🛡️ CI / Security Checks Audit
> 
> - [x] Ran `gh pr checks <N>` to empirically verify CI status.
> - [x] Confirmed no checks are pending/in-progress (Hold review if unfinished).
> - [x] Confirmed no "deep red" critical failures (e.g., CodeQL, Security, core build).
> - [x] If checks are failing, flagged them in Required Actions to block approval.
> 
> **Findings:** Pass - all checks green. Verified manually with `gh pr checks 10966`.
> 
> ---
> 
> ### 📋 Required Actions
> 
> No required actions — eligible for human merge.
> 
> ---
> 
> ### 📊 Evaluation Metrics
> *   **`[ARCH_ALIGNMENT]`**: 100 - Substrate abstraction perfectly maps to class hierarchy intentions.
> *   **`[CONTENT_COMPLETENESS]`**: 100 - Exceptionally well-tested across optional and required hooks permutations.
> *   **`[EXECUTION_QUALITY]`**: 100 - Zero lint issues, solid namespace placement, all specs pass fast.
> *   **`[PRODUCTIVITY]`**: 100 - Delivered scaffold quickly and isolated it logically from consumers.
> *   **`[IMPACT]`**: 90 - A significant architectural multiplier for the rest of the M2 phase.
> *   **`[COMPLEXITY]`**: 40 - Abstraction surface is tight and cleanly bounded.
> *   **`[EFFORT_PROFILE]`**: Maintenance - Foundational debt reduction enabling feature scale.
> 
> LGTM!

---



## Reviews

### `@neo-gemini-pro` (APPROVED) reviewed on 2026-05-08T15:43:37Z

Approving on substantive merits per [comment](https://github.com/neomjs/neo/pull/10966#issuecomment-4407719222). Strict-on-merits eligibility achieved; merge act remains @tobiu's per §0 invariant 1.

---

