Rewrite MCP server and add ask_fpf wrapper#11
Conversation
📝 WalkthroughWalkthroughThe PR migrates the project to a Bun-first toolchain and Rspress docs, replaces the Mastra-integrated MCP surface with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Dev CLI
participant MCP as fpfMcpServer (stdio)
participant Tools as MCP Tools
participant Runtime as FpfRuntime
participant QE as QueryEngine
participant LLM as LM Studio
Dev->>MCP: JSON-RPC request (tools/call)
MCP->>Tools: dispatch tool handler
Tools->>Runtime: invoke runtime method (query/inspect/trace/expand)
Runtime->>QE: run query/inspect/trace/expand
QE-->>Runtime: deterministic results / IDs / anchors
alt requires LLM
Runtime->>LLM: synthesizer.request (apiStyle: responses|lmstudio_chat)
LLM-->>Runtime: generated content / preview
end
Runtime-->>Tools: structured result (validated)
Tools-->>MCP: tool result
MCP-->>Dev: JSON-RPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex[agent] review |
|
@claude[agent] review |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
There was a problem hiding this comment.
Pull request overview
This PR replaces the Mastra-based integration boundary with repo-owned runtime surfaces: a raw stdio JSON-RPC MCP server (snake_case tool IDs) plus a Bun-first CLI/tooling stack, and adds new “ask_fpf” + citation/anchor expansion capabilities with Effect-owned contracts and repo-owned logging/observability.
Changes:
- Replaces Mastra MCP/tools/observability with a repo-owned JSON-RPC stdio MCP server, Effect Schema tool contracts, and runtime logging/observability.
- Adds new MCP/runtime capabilities:
ask_fpf,inspect_fpf_anchor,expand_fpf_citations, plus LM Studio API-style support and health checks (lm-check). - Migrates project tooling to Bun + Rstest/Rslint/Rspress and replaces the old wiki build with generated docs + GitHub Pages deployment.
Reviewed changes
Copilot reviewed 36 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Expands TS include set and removes Vitest globals typing. |
| package.json | Switches to Bun-first scripts/tooling; removes Mastra/Vitest/Zod deps; adds Effect/Rstest/Rslint/Rspress. |
| README.md | Updates repo architecture docs, env vars, commands, MCP tool catalog, and verification steps. |
| .env.example | Updates defaults (query mode, LM Studio model/api style) and clarifies opt-in behavior. |
| .gitignore | Ignores Rspress build output and generated docs content. |
| .github/workflows/deploy-docs.yml | Replaces Node/npm-based wiki deploy with Bun + Rspress docs build to doc_build. |
| rstest.config.ts | Adds Rstest configuration for Node environment and timeouts. |
| rslint.config.ts | Adds Rslint configuration. |
| rspress.config.ts | Adds Rspress site configuration for generated docs + MCP interface docs. |
| docs/index.md | Adds docs landing page describing the new runtime/docs surfaces. |
| docs/mcp-interface.md | Adds MCP wire protocol + tool contract documentation for fpf_memory. |
| scripts/verify-runtime.sh | Updates verification script to Bun commands and renamed log messaging. |
| scripts/generate-docs.ts | Adds compiler-backed doc generation into docs/generated/ for Rspress. |
| src/cli.ts | Migrates CLI to runtime logger and adds lm-check (LM Studio health checks). |
| src/mcp-stdio.ts | Replaces Mastra MCP entrypoint with repo-owned stdio server bootstrap. |
| src/mcp/server.ts | Introduces raw newline-delimited JSON-RPC MCP server (initialize/ping/tools/list/tools/call) and error handling. |
| src/mcp/tools.ts | Defines canonical MCP tools (snake_case) and implements ask_fpf markdown wrapper + new anchor/citation tools. |
| src/mcp/tool-contracts.ts | Adds Effect Schema tool contracts + draft-07-ish JSON Schema emission for MCP listing and validation. |
| src/logging/runtime-logger.ts | Adds repo-owned JSONL structured logger (compatible env names retained). |
| src/observability/runtime-observability.ts | Adds repo-owned observability sink with withRuntimeSpan and status summary reporting. |
| src/runtime/types.ts | Adds types for AskFpfResult, anchor inspection, citation expansion, and LM Studio API-style in status. |
| src/runtime/runtime.ts | Adds runtime APIs inspectAnchor + expandCitations; migrates status to runtime observability summary. |
| src/runtime/query-engine.ts | Implements anchor inspection + citation batch expansion and anchor owner resolution. |
| src/runtime/lm-studio-synthesizer.ts | Adds LM Studio API-style support (responses vs lmstudio_chat), defaults, health checks, and runtime-span logging. |
| src/runtime/ai-trace-log.ts | Extends AI trace log records to include LM Studio API style. |
| src/runtime/compiler.ts | Removes an unused type import (cleanup). |
| tests/mcp-server.test.ts | Adds tests for MCP initialization, tool listing, tool calls, and parse-error behavior. |
| tests/lm-studio-synthesizer.test.ts | Migrates to Rstest; updates LM Studio model defaults; adds chat-style + health-check coverage. |
| tests/fpf-spec-runtime.test.ts | Migrates to Rstest; adds coverage for new tools/contracts, default query mode behavior, anchor/citation expansion. |
| tests/wiki/render.test.ts | Removes tests for deleted wiki HTML render/autolink implementation. |
| src/wiki/build.ts | Removes wiki build pipeline (replaced by Rspress docs generation). |
| src/wiki/render.ts | Removes markdown rendering + pattern ID autolinking (wiki removed). |
| src/wiki/template.ts | Removes embedded wiki HTML/CSS/JS app shell (wiki removed). |
| src/mastra/index.ts | Removes Mastra entrypoint wiring (Mastra removed). |
| src/mastra/logger.ts | Removes Mastra logger wrapper (replaced by runtime logger). |
| src/mastra/observability.ts | Removes Mastra observability integration (replaced by runtime observability). |
| src/mastra/mcp/fpf-spec-server.ts | Removes Mastra MCP server wrapper (replaced by raw MCP server). |
| src/mastra/tools/fpf-spec-tools.ts | Removes Mastra tool definitions (replaced by Effect-contract tools). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!this.initialized) { | ||
| return protocolError(message.id ?? null, 'Server has not been initialized yet.'); | ||
| } | ||
|
|
||
| switch (message.method) { | ||
| case 'ping': | ||
| return { jsonrpc: '2.0', id: message.id ?? null, result: {} }; | ||
| case 'tools/list': |
There was a problem hiding this comment.
JSON-RPC notifications (requests without an id) must not receive a response, but the server currently responds with id: null for methods like ping, tools/list, tools/call, and unknown methods when message.id is undefined. This can violate JSON-RPC/MCP expectations and confuse clients. Consider treating id === undefined as a notification and returning null (or rejecting as invalid request) for all non-notification methods.
| if (message.method === 'notifications/initialized') { | ||
| this.ready = true; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
notifications/initialized sets this.ready = true even if initialize was never received, and ready is not used anywhere else in the server. This creates an odd state transition and dead state. Consider either gating this on this.initialized, or removing ready entirely if it is not needed.
| - [tests/mcp-server.test.ts](</Users/stas-studio/Developer/fpf-memory/tests/mcp-server.test.ts>) | ||
| - [src/mcp/server.ts](</Users/stas-studio/Developer/fpf-memory/src/mcp/server.ts>) | ||
| - [src/mcp/tools.ts](</Users/stas-studio/Developer/fpf-memory/src/mcp/tools.ts>) | ||
| - [src/mcp/tool-contracts.ts](</Users/stas-studio/Developer/fpf-memory/src/mcp/tool-contracts.ts>) |
There was a problem hiding this comment.
The "Verification" section links to absolute local filesystem paths (e.g. /Users/.../tests/mcp-server.test.ts), which will be broken in the published docs site and for other contributors. Replace these with repo-relative links (e.g. ../tests/mcp-server.test.ts) or plain code references without links.
| - [tests/mcp-server.test.ts](</Users/stas-studio/Developer/fpf-memory/tests/mcp-server.test.ts>) | |
| - [src/mcp/server.ts](</Users/stas-studio/Developer/fpf-memory/src/mcp/server.ts>) | |
| - [src/mcp/tools.ts](</Users/stas-studio/Developer/fpf-memory/src/mcp/tools.ts>) | |
| - [src/mcp/tool-contracts.ts](</Users/stas-studio/Developer/fpf-memory/src/mcp/tool-contracts.ts>) | |
| - [tests/mcp-server.test.ts](../tests/mcp-server.test.ts) | |
| - [src/mcp/server.ts](../src/mcp/server.ts) | |
| - [src/mcp/tools.ts](../src/mcp/tools.ts) | |
| - [src/mcp/tool-contracts.ts](../src/mcp/tool-contracts.ts) |
| private findOwnerNodeForAnchor(anchor: AnchorRef): CompiledNode | undefined { | ||
| if (anchor.nodeId && this.snapshot.compiledNodes[anchor.nodeId]) { | ||
| return this.snapshot.compiledNodes[anchor.nodeId]; | ||
| } | ||
|
|
||
| return Object.values(this.snapshot.compiledNodes).find((node) => node.anchorIds.includes(anchor.id)); |
There was a problem hiding this comment.
findOwnerNodeForAnchor falls back to Object.values(this.snapshot.compiledNodes).find(...) when anchor.nodeId is missing. For batch operations like expandCitations, this can become O(nodes × citations) and may be noticeable on large snapshots. Consider precomputing an anchorId -> ownerNode map once per QueryEngine (or deriving ownership from existing index structures) to keep citation expansion linear in the number of citations.
| private findOwnerNodeForAnchor(anchor: AnchorRef): CompiledNode | undefined { | |
| if (anchor.nodeId && this.snapshot.compiledNodes[anchor.nodeId]) { | |
| return this.snapshot.compiledNodes[anchor.nodeId]; | |
| } | |
| return Object.values(this.snapshot.compiledNodes).find((node) => node.anchorIds.includes(anchor.id)); | |
| private anchorOwnerNodeMap?: Map<string, CompiledNode>; | |
| private getAnchorOwnerNodeMap(): Map<string, CompiledNode> { | |
| if (!this.anchorOwnerNodeMap) { | |
| const anchorOwnerNodeMap = new Map<string, CompiledNode>(); | |
| for (const node of Object.values(this.snapshot.compiledNodes)) { | |
| for (const anchorId of node.anchorIds) { | |
| anchorOwnerNodeMap.set(anchorId, node); | |
| } | |
| } | |
| this.anchorOwnerNodeMap = anchorOwnerNodeMap; | |
| } | |
| return this.anchorOwnerNodeMap; | |
| } | |
| private findOwnerNodeForAnchor(anchor: AnchorRef): CompiledNode | undefined { | |
| if (anchor.nodeId && this.snapshot.compiledNodes[anchor.nodeId]) { | |
| return this.snapshot.compiledNodes[anchor.nodeId]; | |
| } | |
| return this.getAnchorOwnerNodeMap().get(anchor.id); |
| async function appendObservabilityEvent( | ||
| filePath: string, | ||
| format: ObservabilityFileFormat, | ||
| event: Record<string, unknown>, | ||
| ): Promise<void> { | ||
| const current = writeChains.get(filePath) ?? Promise.resolve(); | ||
| const next = current.then(async () => { | ||
| const document = await loadPersistedObservability(filePath, format); | ||
| document.updatedAt = new Date().toISOString(); | ||
| document.events.push(event); | ||
| await writeFile(filePath, JSON.stringify(document, null, 2)); | ||
| }); | ||
| writeChains.set(filePath, next.catch(() => undefined)); | ||
| await next; | ||
| } |
There was a problem hiding this comment.
appendObservabilityEvent reads the full JSON file, mutates it, and rewrites it for every event. This is vulnerable to cross-process races (e.g. CLI + MCP both writing to the same observability path) causing lost events, and can become expensive as events grows. Consider switching to an append-only format (e.g. JSONL), adding file locking, or segmenting/rotating the log to avoid unbounded read/modify/write cycles.
| const responses = ( | ||
| await Promise.all(message.map((item) => this.handleSingleMessage(item))) | ||
| ).filter((value): value is JsonRpcResponse => value !== null); |
There was a problem hiding this comment.
Batch JSON-RPC requests are currently processed concurrently via Promise.all(...). Several tool implementations perform filesystem writes (e.g. refresh() writing artifacts/audit), so concurrent execution in a single batch can race and produce inconsistent artifacts or lost writes. Consider processing batch items sequentially (or adding a runtime-level mutex) to keep tool execution deterministic and avoid file write races.
| const responses = ( | |
| await Promise.all(message.map((item) => this.handleSingleMessage(item))) | |
| ).filter((value): value is JsonRpcResponse => value !== null); | |
| const responses: JsonRpcResponse[] = []; | |
| for (const item of message) { | |
| const response = await this.handleSingleMessage(item); | |
| if (response !== null) { | |
| responses.push(response); | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Found two blockers: (1) STDIO server reads/writes raw newline-delimited JSON without MCP Content-Length framing, so standard MCP clients will emit parse errors on the header line and can’t parse responses (src/mcp/server.ts:185-285). (2) Batch requests are processed concurrently; if a client sends initialize plus tools/list in one batch the tools call can race before initialized and return a protocol error (src/mcp/server.ts:35-44). Tests not run here because bun is unavailable in the environment ( |
|
To use Codex here, create a Codex account and connect to github. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6071ee0480
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case 'ping': | ||
| return { jsonrpc: '2.0', id: message.id ?? null, result: {} }; | ||
| case 'tools/list': |
There was a problem hiding this comment.
This branch responds to methods like ping even when the incoming message has no id, by emitting id: null. In MCP/JSON-RPC, notifications are one-way and must not receive any response; replying to client notifications (for example cancellation or future notification methods) can trigger protocol errors or desynchronize strict clients. Add an id check before dispatch so non-initialize notifications are ignored instead of answered.
Useful? React with 👍 / 👎.
| if (url.pathname === '/responses') { | ||
| return `${url.origin}/v1/models`; | ||
| } | ||
| if (url.pathname === '/' || url.pathname === '') { | ||
| return `${url.origin}/v1/models`; |
There was a problem hiding this comment.
/v1/responses to the correct models endpoint
The models endpoint mapper only special-cases '/responses', so a base URL like http://localhost:1234/v1/responses falls through to the default path and becomes .../v1/responses/models. That breaks lm-check model discovery for a valid OpenAI-style base URL and reports unhealthy status even when generation would work. Normalize '/v1/responses' (and equivalent prefixed responses paths) back to '/v1/models' before returning.
Useful? React with 👍 / 👎.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b3a7ba11d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| ok: false, | ||
| durationMs: Date.now() - startedAt, | ||
| error: error instanceof Error ? error.message : 'Unknown LM Studio health-check error', | ||
| } as T & { durationMs: number; error?: string }; |
There was a problem hiding this comment.
When runHealthRequest catches an exception (for example, LM Studio is down or times out), it returns only { ok, durationMs, error }, but runLmStudioHealthCheck exposes this as modelDiscovery, whose contract requires listed and modelCount. In those failure scenarios these fields become undefined, which breaks consumers that rely on the declared response shape (including schema validation or strict JSON parsing).
Useful? React with 👍 / 👎.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 190: Update the tool-role description for ask_fpf to use proper noun
casing: change "markdown" to "Markdown" so the line reads that ask_fpf returns
the grounded answer as Markdown with IDs, citations, constraints, gaps, and
snapshot metadata; search for the string "ask_fpf" in the README and edit its
description accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67a1acad-b22c-4a99-9ed7-65356b923109
📒 Files selected for processing (4)
README.mdsrc/server-config.tssrc/server.tstests/server-config.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server.ts
| - `refresh_fpf_index`: rebuild the local artifact set | ||
| - `get_fpf_index_status`: inspect runtime freshness, artifact presence, and runtime configuration | ||
| - `query_fpf_spec`: return the answer envelope with IDs, citations, constraints, and freshness metadata | ||
| - `ask_fpf`: return the grounded answer as markdown with IDs, citations, constraints, gaps, and snapshot metadata |
There was a problem hiding this comment.
Capitalize “Markdown” in tool-role description.
Use proper noun casing for consistency with the rest of the docs.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~190-~190: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ask_fpf`: return the grounded answer as markdown with IDs, citations, constraints, gaps,...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 190, Update the tool-role description for ask_fpf to use
proper noun casing: change "markdown" to "Markdown" so the line reads that
ask_fpf returns the grounded answer as Markdown with IDs, citations,
constraints, gaps, and snapshot metadata; search for the string "ask_fpf" in the
README and edit its description accordingly.
|
@codex[agent] review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Validation
Notes
Summary by CodeRabbit
New Features
Changes