Repository navigation
docs(adr): decide the local MCP server, pinned to spec revision 2026-07-28 - #128
Merged
Merged
Conversation
…07-28 Closes #79. Writes ADR 0001: KnowNote is the MCP *server* and MCP is its export surface, not an agent platform (`#38` is the demand signal and prior art only - its fork is not merged or used as a template). The protocol revision is the part that had to be checked against the live spec rather than a summary, and it changes the shape of the work. Verified on 2026-09-25: `2026-07-28` is the newest non-prerelease revision in the specification repository. That revision **removed the `initialize` handshake** - "There is no negotiation handshake" - and moved version, capabilities and identity into per-request `_meta`. So the review comment in #38 that the revision "changed session/handshake behaviour" is right, and anything built from an older tutorial implements an era the protocol no longer has. The ADR therefore commits to modern (`2026-07-28`) only, with `server/discover` (the spec's MUST-implement discovery RPC, usable as a backward-compatibility probe on stdio) and a named revisit trigger instead of speculative dual-era support. It also records the revision's concrete obligations on #80: required `resultType` on every result, `ttlMs`/`cacheScope` on list results, deterministic `tools/list` order, the removed `ping`/`logging/setLevel`, and that Roots/Sampling/Logging are deprecated and must not be added. Decisions recorded: stdio-only transport (no network listener, so no remote attack surface); a small read-only tool surface with `read_document` bounded by page; explicit opt-in visible in the UI; no path exposure (the reader's bytes-by-id rule); and the reuse requirement - the same `Retriever`/`RetrievedEvidence` the chat path and eval harness use, never a second RAG path. One correction to its own inputs: #79 and #80 both say the server should sit on a `CitationService`, which does not exist. #60 recorded that it was deliberately never created - citation parsing and source-anchor mapping are stateless and shared between main and renderer, so they are pure functions in `src/shared/utils/`. The ADR names the real modules, because otherwise #80 sends someone hunting for a class that was never written. Verification: the spec revision, the removed handshake, `server/discover` and the deprecations are quoted from the fetched specification pages, and the revision is confirmed against the specification repository's release list. Documentation only - no code, no behaviour change.
github-actions
Bot
added
the
documentation
…ault Corrects the core decision in ADR 0001. The first version concluded that serving 2025-era clients would mean maintaining two server implementations, and therefore chose modern-only. That premise was wrong, and I asserted it without checking the SDK. With the official TypeScript SDK v2 it is the reverse: `serveStdio(factory)` "is the stdio entry point for serving the 2026-07-28 protocol revision ... with 2025-era serving as the default", and its `legacy?: 'reject' | 'serve'` option defaults to `'serve'`. The entry owns the era decision and constructs ONE server instance from a single era-agnostic factory. Dual-era therefore costs one function choice, not a second implementation - and the same file documents the trap this ADR exists to avoid: hand-constructed servers connected straight to a `StdioServerTransport` "keep serving the 2025-era protocol they were written for", so the obvious first thing to write serves the wrong era. The compatibility cost is equally real and the ADR now records it: Codex keeps the legacy lifecycle for local stdio by default and requires `CODEX_MCP_PROTOCOL_VERSION=2026-07-28` to speak the new revision (openai/codex#35724). Modern-only would have broken a client the ADR itself names, for every user who had not set an environment variable. So: target the 2026-07-28 model, serve both eras via the SDK, keep one factory / one Retriever / one provenance model, hand-write no legacy stack, and revisit dropping legacy (`legacy: 'reject'`) with a support matrix in a small follow-up ADR. Also in this commit, from the same review: - `server/discover` is a protocol RPC, not a tool, and the surface table no longer lists it beside `list_notebooks`. The spec requires it of the server; registering it in `tools/list` would be wrong. - The stdio claim was too strong. It no longer leans on "protocol semantics are identical on every binding" (HTTP and stdio cancellation genuinely differ) but on the narrower, checkable statement that the surface defined here needs no HTTP-only capability. - `resultType` is a `string` discriminator, not a closed set of two: the base protocol only requires the field. Recorded as `complete` for this surface, `input_required` for MRTR-capable operations, Tasks out of scope. - A new constraint on #80, from the SDK source: the factory may be constructed twice per connection (optimistic modern probe, then legacy instance on fallback), so it must be cheap and side-effect-free. #80's body is patched in the same pass: it still said "the same `Retriever` and `CitationService`" (a class #60 recorded as deliberately never created) and pointed at a "citation service" in its dependencies. It now names the real modules and carries the dual-era constraints above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #79.
Writes
docs/adr/0001-knowledge-mcp-server.md: KnowNote is the MCP server and MCP is its export surface, not an agent platform. #38 is the demand signal and prior art only — its fork is not merged and not used as a template.The verified part
Verified against the live specification on 2026-09-25:
2026-07-28is the newest non-prerelease revision (predecessor2025-11-25; nothing newer exists). That revision removed theinitializehandshake — "There is no negotiation handshake. Every request carries its protocol version, and the server accepts or rejects each request independently" — and moved version/capabilities/identity into per-request_meta. The spec's own terminology calls2026-07-28+ modern and2025-11-25- legacy.So the review comment in #38 was right, and a hand-written
initialize/session server built from an old tutorial is building the previous era by hand.Corrected in review: serve both eras, because the SDK makes it the default
My first version concluded from the above that serving 2025-era clients meant maintaining two implementations, and chose modern-only. That premise was wrong — I asserted it without checking the SDK — and it would have broken a client this ADR names.
From the official TypeScript SDK v2 (
packages/server/src/server/serveStdio.ts):legacy?: 'reject' | 'serve'—'serve'is the default.StdioServerTransport"keep serving the 2025-era protocol they were written for" — so the obvious first thing to write (Server.connect(new StdioServerTransport())) serves the wrong era.serveStdio/createMcpHandler) owns the era decision, the factory is era-agnostic."The compatibility cost is equally real. Codex keeps the legacy lifecycle for local stdio by default and needs
CODEX_MCP_PROTOCOL_VERSION=2026-07-28to speak the new revision (openai/codex#35724, Add MCP 2026-07-28 discovery support). Modern-only would have failed against a client the ADR itself names.2026-07-28model (per-request envelope,server/discover, no session)serveStdio(factory)— dual-era,legacy: 'serve'(its default)Retriever, one provenance modellegacy: 'reject', its own small ADR with a support matrixOther review corrections
server/discoveris a protocol RPC, not a tool. The spec requires "servers MUST implement this RPC"; it is what a modern client calls first. The surface table now separates the protocol-required RPC from the KnowNote application tools, so [Feat] Implement the Knowledge MCP server #80 does not register a tool namedserver/discover.resultTypeis not a closed set of two. The base protocol only requires the field, typedstring. Recorded ascompletefor this surface,input_requiredfor MRTR-capable operations, and Tasks out of scope for v1.One thing I did not change
The review asked me to fix "#80 (v1.5)" to v1.6 in the ADR header. #80 is in v1.5, so I left it. Evidence:
gh api repos/mrsibe/KnowNote/issues/80reportsmilestone: v1.5; the v1.5 milestone contains[77, 78, 80, 94, 95, 96, 97, 98, 100]while v1.6 contains only[99]; and #80's own body records "Moved up from v1.6 to v1.5 at the maintainer's request". Changing it would have introduced drift rather than removed it.#80 patched in the same pass
#80 still said "backed by the same
RetrieverandCitationService" and listed "the citation service (#69, #70)" as a dependency. There is noCitationService— #60 recorded that it was deliberately never created, because citation parsing, resolution and source-anchor mapping are stateless and shared by main and renderer. #80 now names the real modules (src/shared/utils/), carries the dual-era constraints, and no longer implies a class that does not exist.Acceptance
docs/adr/Verification
Documentation only — no code, no behaviour change. Prettier clean.
The spec facts are quoted from fetched specification pages (
/specification/2026-07-28/changelog,/basic/versioning,/basic/transports,/basic/index); the revision is confirmed against the specification repository's release list; the SDK behaviour is quoted frompackages/server/src/server/serveStdio.tsand thedual-eraexample; the Codex behaviour is from openai/codex#35724. Nothing here rests on a chat summary.