Skip to content

fix: instructions on the result, one Implementation type for both ends (#1560) - #1561

Merged
panyam merged 2 commits into
mainfrom
fix/implementation-type-1560
Oct 18, 2026
Merged

panyam merged 2 commits into
mainfrom
fix/implementation-type-1560

Conversation

@panyam

@panyam panyam commented Oct 18, 2026 •

Copy link
Copy Markdown
Owner

What changes

  • Problem: mcpkit sends instructions inside serverInfo. Every schema since 2025-03-26 puts it on InitializeResult, beside serverInfo, and Implementation has never had the field. Hosts that follow the spec drop it, which is how Claude Code ended up showing the model nothing from declaire (declaire#308).
  • The stateless wire was wrong in two more ways. The 2026-07-28 DiscoverResult has a top-level instructions, and mcpkit never sent it there. Meanwhile the _meta serverInfo stamped onto every stateless result repeated the full text.
  • Change: instructions moves off the identity struct onto server.WithInstructions(s). Initialize and server/discover both send it at the top level of the result, and the client reads it into Client.Instructions on either wire.
  • Change: core.Implementation becomes the one identity type, as in the spec. ServerInfo and ClientInfo are now aliases of it. Client identity was stuck at the 2024-11-05 shape, so it gains title, description, websiteUrl and icons, and both sides gain icons (new core.Icon).
  • Change: the stateless _meta clientInfo was a hand-built {name, version} map. It now marshals the whole Implementation.

Breaking for anyone setting ServerInfo.Instructions. That field is gone, and WithInstructions replaces it.

Minimal example (a server configured with instructions, then initialize):

server.NewServer(core.Implementation{Name: "demo", Version: "1.0"},
    server.WithInstructions("Use search before fetch."))

On main the equivalent is core.ServerInfo{..., Instructions: "Use search before fetch."}.

Before / after

Wire output, same server on both branches:

# main
initialize:      {..."serverInfo":{"name":"demo","version":"1.0","instructions":"Use search before fetch."}}
server/discover: {"_meta":{"io.modelcontextprotocol/serverInfo":{"instructions":"Use search before fetch.","name":"demo","version":"1.0"}},"capabilities":{...},"supportedVersions":["2026-07-28"],...}
tools/list:      {"_meta":{"io.modelcontextprotocol/serverInfo":{"instructions":"Use search before fetch.",...}},"tools":[...]}

# this branch
initialize:      {..."serverInfo":{"name":"demo","version":"1.0"},"instructions":"Use search before fetch."}
server/discover: {"_meta":{"io.modelcontextprotocol/serverInfo":{"name":"demo","version":"1.0"}},"capabilities":{...},"instructions":"Use search before fetch.","supportedVersions":["2026-07-28"],...}
tools/list:      {"_meta":{"io.modelcontextprotocol/serverInfo":{"name":"demo","version":"1.0"}},"tools":[...]}

Interop with the TypeScript reference SDK (1.29.0, Client.getInstructions()), connected to each build:

main:        {"serverVersion":{"name":"demo","version":"1.0"},"instructions":null}
this branch: {"serverVersion":{"name":"demo","version":"1.0"},"instructions":"Use search before fetch."}

That null is the symptom from the issue, reproduced against the reference client.

How we missed it

  • The field went onto ServerInfo in a417cda (2026-04-02), the commit that added the 2025-11-25 Implementation fields (title, description, websiteUrl). instructions sits a few lines below serverInfo in InitializeResult, and it got swept in with them. That was the first week of the repo, so every later version inherited it.
  • Nothing tested it. No test in core, server or client asserted on instructions at all.
  • Conformance doesn't grade it. Upstream's harness plumbs getInstructions() through src/connection/ and no scenario checks it, so Server 31/31 stayed green the whole time.
  • The mcpkit client read it back from the same wrong place, so mcpkit-to-mcpkit round trips looked fine. It took a third-party host to notice.
  • ClientInfo fell behind for the same structural reason. Two Go types were named after the wire fields instead of the spec type, and only the server one got updated.

Folded in: the TestTaskElicitE2E flake (#390)

This PR's first CI run failed on TestTaskElicitE2E, which has failed on CI three times now (2026-09-07 on main, 2026-10-15 on #1545, and here). #874 only capped the hang at 20s. The cause turns out to be a lost wakeup in the v1 tasks/result long-poll, and it's a real server bug, not a test problem.

  • The handler checked the side channel, then the task status, then called WaitForUpdate, which only wakes on store updates made after it registers.
  • TaskElicit sets input_required and then queues its request on the side channel. If both happened in that gap, the handler slept on the store. A channel send isn't a store update, so it never woke, and the tool blocked forever waiting for the client's answer.
  • A task that finished in the gap between the status check and the wait was missed the same way.
  • Fix: the handler now selects on the side channel and on WaitForResult together. WaitForResult already checks terminal state and registers its waiter under one lock (task_store.go), so nothing falls in a gap, and there's no polling.

Two new tests force each interleaving deterministically, using a store wrapper that holds the handler's first wait until the task has moved on:

# main (3 runs each)
--- FAIL: TestTaskResultSeesElicitationQueuedBeforeItWaits (5.00s)
    GetTaskPayloadV1(task-…) did not return within 5s — task side-channel push likely never arrived (issue 390)
--- FAIL: TestTaskResultSeesCompletionBeforeItWaits (5.00s)

# this branch
ok  github.com/panyam/mcpkit/server  (20 runs each; plus TestTaskElicitE2E/TestTaskSampleE2E 600 runs at -cpu 1,2,8, and the Task tests under -race)

The first failure prints the same message CI did.

Diff groups

Prerequisite knowledge

Reviewer's guide

  • Think of a business card and the cover note clipped to it. The card says who you are, and the note says how to work with you.
  • Implementation is the card and instructions is the note. The spec keeps them as siblings on the result. mcpkit printed the note on the card, so a reader looking for a note found none.
  • On the stateless wire the card is photocopied onto every reply (_meta serverInfo). So the note went out on every reply too, and the one place it belongs (server/discover) had nothing.
  • So the PR takes the note off the card for good (it goes in a server option) and puts it where each wire expects it. Since the card is one spec type, the server and client copies become one struct.

Read in this order:

  1. core/protocol.go is the card itself: Implementation, Icon, the two aliases, and InitializeResult.Instructions.
  2. server/instructions_wire_test.go checks raw wire keys on both wires, including the photocopied _meta card.
  3. server/server.go, server/dispatch.go add the option and the initialize result.
  4. server/stateless/discover.go, server/stateless/backend.go, server/stateless_backend.go cover discover and the new Backend.Instructions().
  5. client/client.go, client/client_reconnect.go, client/stateless.go read it back, and send the full clientInfo in _meta.
  6. client/instructions_test.go runs a real server and client on both wires.

Then the folded fix, which has nothing to do with the card analogy:

  1. server/tasks_v1.go has the tasks/result loop and the new waitForTaskEvent.
  2. server/tasks_v1_wakeup_test.go holds the handler's first wait until the race window has passed (stallingStore).

Skim or skip: core/stateless.go (type swaps), server/stateless/dispatch_test.go (fake gains the method), CHANGELOG.md, the README lines.

Decision log

  • Chose WithInstructions over a WithImplementation option. NewServer's first argument already carries the identity, and instructions aren't identity; in the draft they sit on DiscoverResult, which has no identity in its body at all.
  • Kept ServerInfo and ClientInfo as aliases rather than renaming about 730 literals across about 255 files. There's one struct underneath, so the two can't drift again, and the diff stays reviewable.
  • No decode fallback for servers that still nest instructions in serverInfo (mcpkit up to v0.8.1). That placement was off-spec, and the known affected server (declaire) will bump. Easy to add later if one turns up.
  • Icon.Theme is a plain string with IconThemeLight and IconThemeDark constants, matching how core handles other small spec enums like CacheScope.

Risk / blast radius

  • Affects anyone setting ServerInfo.Instructions (compile error, fix is WithInstructions), and any out-of-tree stateless.Backend implementation (one new method). chakra uses neither.
  • Tests covering this: 8 new tests, 28 new assertions, none removed or weakened. Each instructions fix was mutation-checked: reverting the initialize line, the discover line, or the _meta clientInfo marshal each turns the matching tests red. The two wakeup tests fail on main as shown above.
  • Verified: make test, all 59 example modules build, go vet clean in every sub-module, testconf 40/0, testconf-stateless 30/0, testconf-client 581/0, check-stateless-middleware ok.
  • Be paranoid about the tasks/result loop. It used to proxy a pending side-channel request before checking for terminal state. Now, if both are ready at once, select picks either. That only happens after a cancel (a request still queued for a task that's already terminal), and returning the result then is fine. TaskStore.WaitForUpdate has no callers left, and it stays because the interface is public.
  • Be paranoid about the _meta clientInfo now carrying optional fields. Servers must treat it as display-only per spec, but a strict decoder somewhere could object to unknown keys.
  • Related tickets: ttlMs is omitted by default where the 2026-07-28 draft makes it required #1546 (ttlMs on discover) touches the same DiscoverResult. exercise-wire: a wire-parity check that fails on #1355, #1515 and #1546 today #1555 (exercise-wire) is the wire-parity check that should grow an instructions row.

Out of scope (deliberately deferred)

  • icons on Tool, Resource, ResourceTemplate and Prompt (also 2025-11-25) → follow-up issue.
  • An upstream conformance check that graded instructions placement would have caught this → same follow-up issue.
  • chakra's host never puts a connected server's instructions into the system prompt → noted in the same follow-up, since it's a separate repo.

Serves mission #1552 (a 2026-07-28 client gets spec-correct results on both wires).

Closes #1560
Fixes the root cause of #390 (closed by #874, which only bounded the hang).

…on type (#1560)

Every schema from 2025-03-26 on puts instructions on InitializeResult,
beside serverInfo, and the 2026-07-28 draft puts it on DiscoverResult too.
mcpkit carried it as a field of core.ServerInfo, so initialize nested it
inside serverInfo, server/discover never sent it, and every stateless
result repeated it in _meta serverInfo. Hosts that follow the spec,
Claude Code and the TypeScript SDK among them, saw no instructions.

The field had sat on ServerInfo since a417cda, which added the
2025-11-25 Implementation fields and took instructions along with them.
No test asserted on it and no upstream conformance scenario grades it.

- core.Implementation is the spec's single identity type, with Icons and
  core.Icon added. ServerInfo and ClientInfo become aliases of it, so
  client identity gains title, description, websiteUrl and icons.
- ServerInfo.Instructions is gone; server.WithInstructions sets it.
  InitializeResult and the stateless DiscoverResult carry it top-level,
  and stateless.Backend gains Instructions().
- The client reads it into Client.Instructions on both wires.
- The stateless _meta clientInfo marshals the whole Implementation
  instead of a name/version map.
… before it waits (#390)

The long-poll checked the side channel, then the task status, then called
WaitForUpdate, which only wakes on store updates made after it registers.
Two things could land in the gap and strand the handler:

- TaskElicit sets input_required, then queues its request on the buffered
  side channel. A channel send is not a store update, so a handler that
  registered after both waited forever, and the tool blocked on the reply.
  This is the intermittent TestTaskElicitE2E failure (CI 2026-09-07,
  2026-10-15, and on #1561); #874 only bounded the hang at 20s.
- A task finishing between the status check and the wait was never seen.

The handler now selects on the side channel and on WaitForResult together.
WaitForResult checks terminal state and registers its waiter under one
lock, so neither event can fall in a gap.

Two tests force each interleaving with a store wrapper that holds the
handler's first wait until the task has moved; both hang to their 5s bound
on the old handler and pass on the new one.
@panyam
panyam merged commit 0845457 into main Oct 18, 2026
14 checks passed
@panyam
panyam deleted the fix/implementation-type-1560 branch October 18, 2026 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

initialize sends instructions inside serverInfo; the spec puts it on InitializeResult

1 participant