fix(mcp): send proactive-usage instructions in the initialize handshake - #681
Conversation
Desktop MCP clients and Codex switched to lazy tool loading, so tool schemas no longer reach the model's context until a tool is explicitly loaded. The "call this PROACTIVELY" guidance lived only in the tool descriptions (tools/remember.ts), so the model saw no memory tool and either offered its own built-in memory or denied the tool existed. The MCP `instructions` field travels with `initialize`, before any tools/list, so lazy loading cannot strip it. Neither server path set it: the relayer built McpServer with no options object, and the auth-required stub hand-rolled its initialize result. * relayer: full recall/remember/recover guidance, mirroring the text the plugin already injects via its SessionStart hook. Plain MCP installs never got that hook, which is why proactive behavior only survived for plugin users and why the demo videos still worked. * auth-required stub: signed-out variant pointing at memwal_login instead, since every memory tool fails without credentials. Also replaces the hardcoded serverInfo.version "0.0.1" in both handshakes with the real package version. It matched neither packages/mcp (0.0.5) nor memwal-server-scripts (0.1.0), so every user's handshake log reported the same fake version and made field triage guesswork. Distinct from MEMWAL_MCP_COMPATIBILITY_VERSION, which stays pinned at 0.0.1 as the relayer-contract baseline checked by check-compatibility-contract.mjs. Refs WALM-324
fdda948 to
8fd56b1
Compare
The `instructions` field is an optional McpServer constructor argument, so dropping it fails no type check and breaks no other test. That is exactly how the proactive save/recall contract went missing in the first place: the guidance lived only in tool descriptions, which lazy-loading clients never put in context. Drives the real mounted routes over loopback HTTP and asserts the handshake names memwal_recall/memwal_remember, keeps the "not currently loaded" rebuttal that counters the reported symptom, and reports the real package version rather than a hardcoded stub. Verified to fail when the constructor argument is removed. Refs WALM-324
The bridge answers `initialize` itself at cold start and SUPPRESSES the relayer's reply, so the relayer's instructions never reached a stdio client. Every signed-in user went through that path, which meant the previous commit fixed the field on a response nobody sees. Caught by end-to-end probing against a deployed relayer: build metadata confirmed the relayer was running this branch, yet the handshake still reported version 0.0.1 with no instructions. That value could only come from buildLocalInitializeResult in bridge.ts. The relayer copy still matters: it serves the direct HTTP/OAuth connector path, which does not go through the bridge. Both are needed. Extracts both payloads into instructions.ts so bridge and auth-required share one definition, and asserts the local initialize carries them in coldstart-init, where the gap was previously invisible to every assertion. Refs WALM-324
|
LGTM! The fix via Quick reminder: please add a changeset ( |
Patch bump for @mysten-incubation/memwal-mcp. The published package changes here are the bridge and auth-required initialize responders; the relayer-side change ships with the service, not npm. Requested in review on #681.
|
Thanks @ducnmm! Changeset added in So Scoped it to the MCP package only, since the published surface here is the bridge and auth-required For the same reason, the follow-up #682 (collapse duplicate CI is green here, 15/15. One heads-up: your LGTM came through as a plain comment rather than a submitted review, so GitHub still reports |
Follows the manual release flow used by #678, not changesets: bump every MCP/plugin manifest together, add the version section to both changelogs, and move the verifier's expected version forward. `verify-manual-sdk-release.mjs` reports "MCP package 0.0.10: manifests and changelogs synchronized". Drops the changeset added earlier in this PR. Leaving it alongside a manual bump would have double-bumped on main, since release-mcp.yml runs `changeset version` there and would have taken 0.0.10 to 0.0.11 while rewriting the changelog section added here. Replaces the review request on #681 for a changeset or changelog update.
|
Redone properly following the manual release flow, per @Henry's note that this repo bumps the version + changelog rather than using changesets. Reference: #678 (
Verified with the repo's own checker rather than by eye: I also removed the changeset I added in @ducnmm this supersedes the changeset from my earlier reply, so that request is now satisfied via the changelog route you offered as the alternative. Still worth noting: the LGTM is a plain comment, so GitHub reports |
A limit=5 recall on a real account returned the same fact five times, so one preference consumed the entire retrieval budget and crowded out every other memory the query should have surfaced. Storing a fact repeatedly is legitimate: each remember is a distinct event with its own blob and timestamp, and there is no content-level uniqueness constraint by design (the sole unique index is request idempotency on remember_jobs (owner, idempotency_key), which guards retries, not content). Deduping on write would silently discard a genuine re-statement, so the fix belongs on the read side. Collapses results whose text matches after trimming, whitespace collapsing, and case folding, keeping the highest scoring copy since rows arrive ranked. Matching is exact-after-normalization rather than fuzzy: merging facts that merely resemble each other would hide real information. The reply reports how many copies were folded so nothing disappears silently.
3 participants
Closes WALM-324.
Problem
Desktop MCP clients and Codex switched to lazy tool loading. Tool schemas no longer enter the model's context until a tool is explicitly loaded, and our entire "when to save / when to recall" contract lived inside those tool descriptions.
Result: the model sees no memory tool, so it either offers its own built-in memory or flatly denies the tool exists. It only works if the user prefixes the prompt with "using the memwal tools...".
Why it still worked in the demo videos
The plugin install path ships a SessionStart hook (
packages/mcp/plugin/scripts/on_session_start.mjs) that injects the proactive guidance into the session. Plain MCP installs (command + argsin the client config) never got that hook, so only plugin users kept working. The videos were recorded on the plugin path.Fix
The MCP
instructionsfield is delivered withinitialize, before anytools/list, so lazy tool loading cannot strip it. Clients inject it into the model's system prompt.There are three initialize responders, and none of them set it:
services/server/scripts/mcp/server.ts(relayer) constructedMcpServerwith no options object. Serves the direct HTTP/OAuth connector path.packages/mcp/src/bridge.tsbuildLocalInitializeResultanswersinitializelocally at cold start and suppresses the relayer's reply. This is the path every signed-in stdio client uses.packages/mcp/src/auth-required.tshand-rolls its own result for the signed-out case.All three now send
instructions. The relayer and bridge carry the full recall/remember/recover guidance plus a line countering the reported symptom ("if a memwal_* tool is not currently loaded, load it and use it; never tell the user that memory is unavailable"). The signed-out stub instead points atmemwal_login, since every memory tool fails without credentials and telling the model to save proactively there would only manufacture errors.Bridge and auth-required share one definition in the new
packages/mcp/src/instructions.ts. The relayer keeps its own copy becauseservices/server/scriptsis the standalonememwal-server-scriptsnpm package with no workspace link topackages/mcp; both sides carry a comment pointing at the other.How #2 was found
Worth recording, because it invalidated an assumption in the first version of this PR. I originally claimed the bridge forwards
initializeupstream untouched, so fixing the relayer alone would be enough. End-to-end probing against a deployed relayer disproved it:/versionreportedbuild.commitexactly equal to this branch HEAD, confirming the relayer was running the fix, yet the client handshake still returnedversion 0.0.1with no instructions. That value could only come frombuildLocalInitializeResult.So the relayer fix alone was correct but unreachable for every stdio user. Unit tests could not catch this: the relayer test drives the routes directly and the auth-required test runs a different mode. Only the real bridge path exposed it.
Regression tests
instructionsis an optional field on both responders, so dropping it fails no type check and breaks no other assertion. That is exactly how this went missing.services/server/scripts/mcp/__tests__/instructions.test.tsdrives the real mounted routes over loopback HTTP.packages/mcp/test/coldstart-init.test.mjsnow asserts the bridge's local initialize carries instructions and a non-stub version. Every other assertion in that file passed while the field was absent.Confirmed the relayer test actually bites: removing the constructor argument turns it red, restoring it turns it green.
Drive-by: real
serverInfo.versionAll three handshakes hardcoded
version: "0.0.1", matching no real package. Every user's handshake log reported the same fake version, which is part of why triaging this was guesswork, and the components were indistinguishable on the wire. All now report their real version viapackages/mcp/src/version.ts.Self-demonstrating:
packages/mcpwas bumped 0.0.5 to 0.0.9 while this branch was open and the handshake picked it up with no code change. It also turned the version into the diagnostic that located bug #2.MEMWAL_MCP_COMPATIBILITY_VERSIONstays pinned at 0.0.1 on purpose. It is the relayer-contract baseline compared againstminSupportedSdk.mcpand checked byscripts/check-compatibility-contract.mjs, not a release version.Verification
npm testservices/server/scripts: 198 pass, 0 failpnpm testpackages/mcp: 18 pass, 0 failpnpm typecheckpackages/mcp: cleanLive bridge probe against the deployed dev relayer, before and after the bridge fix:
Client consumption is confirmed, not assumed: a Claude client's system prompt was observed gaining a
## memwalsection containing this text verbatim, in a session where it had previously been absent.Deploy safety: replicated the Dockerfile's scripts layer in a temp dir and confirmed
../package.jsonresolves to/app/scripts/package.jsonin the container layout.Verified end to end on Claude Desktop
All three behavioral cases pass against the patched build, with the relayer deployed from this branch.
My default deploy region is ap-southeast-1memwal_rememberfired unprompted and succeededWhat'''s my default deploy region?memwal_recallfired, returned the fact, answered correctlyRun the tests for mememwal_remember, correctly silentThe recall case is the one the ticket title is about. Request/response captured:
An earlier recall run looked like a pass and was not: the answer came from Claude'''s built-in memory while the memwal write had 401'''d, so Walrus held nothing. A direct
memwal_recallconfirmed zero matching entries. The rerun above uses a fact that exists only in Walrus, so it cannot be satisfied by any other store.The 401 was stale environment-scoped credentials, cleared with
memwal_login. Not a code issue.Scope of the claim
## memwalsection containing this text verbatim.Also included: collapse duplicate recall results
Folded in from #682 (now closed) so both MCP fixes land together.
A
limit=5recall on a real dev account returned the same fact five times, spending the entire retrieval budget on one preference and crowding out everything else the query should have surfaced:Fixed on the read side, not the write side. Storing a fact repeatedly is legitimate: each remember is a distinct event with its own blob and timestamp, and there is deliberately no content-level uniqueness constraint (the only unique index is request idempotency on
remember_jobs (owner, idempotency_key), which guards retries). Deduping on write would silently discard a genuine re-statement.collapseDuplicatesfolds results whose text matches after trimming, whitespace collapsing, and case folding, keeping the highest scoring copy since rows arrive ranked. Matching is exact-after-normalization rather than fuzzy, because merging facts that merely resemble each other would hide real information. The reply reports how many copies were folded rather than quietly returning fewer rows.7 tests in
mcp/__tests__/recall-dedupe.test.ts, including two safety cases: distinct facts must pass through untouched, and"prefers dark roast coffee"must not collapse with"prefers dark roast coffee in the morning only".This is relayer-side only (
services/server/scripts,"private": true), so it does not affect the npm package version.Release
Manual bump to 0.0.10 following the flow from #678, not changesets: all 11 manifest/changelog/doc files moved together. Verified with the repo checker:
Not covered by this PR