Skip to content

fix(mcp): anchor REMEMBER to the turn, log tool names, add a live handshake probe - #711

Merged
harrymove-ctrl merged 3 commits into
devfrom
fix/mcp-proactive-descriptions
Aug 20, 2026
Merged

fix(mcp): anchor REMEMBER to the turn, log tool names, add a live handshake probe#711
harrymove-ctrl merged 3 commits into
devfrom
fix/mcp-proactive-descriptions

Conversation

@harrymove-ctrl

@harrymove-ctrl harrymove-ctrl commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Rebased onto dev after #706 landed. This PR is now only what #706 did not cover.

Dropped, because #706 solved it better

This branch originally carried two more fixes. Both are superseded, and I removed them rather than resolve the conflict in my favour:

  1. Signed-out vs signed-in descriptions. fix(mcp): make Claude Code auto-remember/recall actually fire #706 added buildToolDefinitions(proactive) with separate SIGNED_OUT_* / SIGNED_IN_* constants. My version pushed proactive wording into the one shared list, which would have told signed-out clients to save proactively using tools that cannot work without credentials. fix(mcp): make Claude Code auto-remember/recall actually fire #706's design is correct; mine was worse.
  2. memwal_recall annotations. fix(mcp): make Claude Code auto-remember/recall actually fire #706 set readOnlyHint: true, destructiveHint: false, going further than the readOnlyHint: false I had, with the same reasoning. memwal_analyze keeps dev's destructiveHint: true — it writes memories, so that value is defensible.

1. REMEMBER had no turn anchor

With the #706-equivalent fixes live on dev, I ran T1-T3 from WALM-368, scored from the MCP log rather than the tool cards:

Test Result
T2 recall PASSmemwal_recall fired unprompted and answered from it
T1 remember FAIL — no tools/call at all
T3 control PASS

Recall firing is new. Both earlier dogfood passes recorded zero tools/call for an entire session. Since RECALL and REMEMBER ship in the same instructions block, to the same client, in the same session, this is a controlled comparison rather than a guess — and the difference is in our own text:

Trigger Anchored to
RECALL "before answering anything that touches..." a turn event
REMEMBER "...call memwal_remember without waiting to be asked" nothing

REMEMBER asked the model to classify a statement and then self-start, with nothing tying the call to a moment in the turn. In T1 it replied "Got it — <fact>. What do you need done with it?", treating a durable fact as task setup and moving on.

REMEMBER now anchors with "in that same turn, before you finish replying", adds configuration values (hostname, port, region, id) since the failing case was a hostname and port, and closes the observed behaviour: do not ask whether to save, and acknowledging a fact in the reply does not store it.

#706's scoping is preserved verbatim ("skip one-off tasks, the current file or bug, and small talk"). Applied to all three copies that carry this contract; the two that must be byte-identical were verified so.

2. Tool calls were not scorable from the log

The host logs method="tools/call" id=N with no tool name, so "remember never fired" and "remember fired and failed" are indistinguishable — the exact ambiguity under investigation, and why the earlier dogfood runs could not be reconciled.

The bridge now logs {"event":"bridge.tool_call","tool":"memwal_recall","id":7}name only, never arguments, since memory text is the user's private data.

grep -o '"event":"bridge.tool_call","tool":"[^"]*"' ~/Library/Logs/Claude/mcp-server-memwal.log | tail

3. packages/mcp/test/handshake-contract.mjs

node test/handshake-contract.mjs --dev

Drives the real built server against a chosen env and asserts what a client is actually handed: instructions present with both halves anchored, memwal_recall advertised read-only, and cold start agreeing with upstream field by field.

tool-definitions.test.mjs from #706 covers the static list. This covers the deployed path, which is where the earlier runs went wrong:

  1. Tool metadata lives in services/server/scripts, so a published npm package alone changes nothing for a signed-in user. Testing before the relayer redeploys measures the old server.
  2. Cold start and upstream can disagree, so the answer depended on whether you looked before or after the re-list.

Deliberately not named *.test.mjs: it needs a live relayer and credentials, and the npm test glob must not collect it. On failure it names the broken precondition; on success it prints the T1-T3 runbook, since those are model-behaviour cases a script cannot automate.

4. Finishing the cold-start sync #706 started

The probe immediately earned its keep, catching drift on four tools #706 did not touch:

Tool Cold start Upstream
memwal_remember_bulk "Remember Several Facts" "Remember Multiple Facts"
memwal_health "Check Relayer Health" "Check Walrus Memory Health"
memwal_analyze 150 chars 340 chars
memwal_restore 165 chars 427 chars

Less severe than the remember/recall case since these are thinner rather than contradictory — memwal_restore upstream explains when to reach for it ("recall returns nothing even though facts were saved before"), while the cold-start copy described only the mechanics. A client that caches its first tools/list keeps the thin version all session.

Values were copied programmatically from a live upstream tools/list rather than retyped, so they cannot drift again through transcription.

Testing

packages/mcp             28 pass, 0 fail
services/server/scripts  221 pass, 0 fail
handshake-contract --dev 13/13   (dev relayer deployed from this branch)

login-handoff.test.mjs pinned the two stale titles. Its own comment requires pre-login discovery to expose "the same safety metadata clients will receive after the bridge hands off," so the expectations were wrong, not the change. Correction, not relaxation.

Honest scope

Items 2, 3 and 4 are tooling and correctness, and stand on their own. Item 1 is a hypothesis with a controlled comparison behind it, not a proven fix. It needs another T1 run on a real client to confirm. Nothing here should be read as "proactive memory is fixed" until that run happens — the probe exists precisely so that run is attributable.

Found in passing, NOT fixed here

The dev relayer crash-loops on redeploy, which produced the 502s during this work:

thread 'main' panicked at src/main.rs:758:14:
Failed to initialize legacy security-delete database:
  Internal("legacy migration failed: ... canceling statement due to lock timeout")

LEGACY_DB_URL and DATABASE_URL point at the same Neon database, so the legacy security-delete migration contends with the main migration runner; during a redeploy the draining container holds the lock and the new one panics. Two separate problems: a transient lock timeout should not be fatal at main.rs:758, and the legacy path should not compete with the main runner on every boot. Both are in the Rust server, out of scope here, and worth their own ticket.

Refs WALM-368, WALM-324


Measured against dev's shipping artifacts

Added MEMWAL_MCP_SERVER so the probe can run a published package instead of this checkout's build. That lets it answer the question that matters before merging: what does dev ship today?

Published @mysten-incubation/memwal-mcp@dev (0.0.10-dev.4) against the dev relayer at e4bdcc97 (#706), signed in:

11/13 passed
FAIL  REMEMBER is anchored to a turn event
FAIL  cold-start and upstream lists agree
      (drift: remember_bulk, analyze, restore, health)

Both defects this PR fixes reproduce with none of my code in the path — dev's own package, dev's own relayer. The same probe against this branch reports 13/13.

Reproduce:

npm pack @mysten-incubation/memwal-mcp@dev
# extract, npm i --omit=dev, then:
MEMWAL_MCP_SERVER=<extracted>/dist/bin/memwal-mcp.js \
  node packages/mcp/test/handshake-contract.mjs --dev

@harrymove-ctrl harrymove-ctrl changed the title fix(mcp): sync cold-start tool list with the relayer's proactive contract fix(mcp): proactive contract in the cold-start list, and stop flagging recall destructive Aug 20, 2026
@railway-app
railway-app Bot temporarily deployed to Walrus Memory / dev August 20, 2026 04:09 Inactive
@railway-app
railway-app Bot temporarily deployed to Walrus Memory / dev August 20, 2026 04:31 Inactive
@harrymove-ctrl harrymove-ctrl changed the title fix(mcp): proactive contract in the cold-start list, and stop flagging recall destructive fix(mcp): make the proactive memory contract consistent and anchored Aug 20, 2026
Rebased onto dev after #706 landed. Two of the four things this branch
originally carried are now solved there, better than I had them:

  - #706 split SIGNED_OUT_* from SIGNED_IN_* tool descriptions via
    buildToolDefinitions(proactive). My version pushed proactive wording
    into the one shared list, which would have told signed-out clients to
    save proactively with tools that cannot work. Dropped mine.
  - #706 set memwal_recall to readOnlyHint: true, destructiveHint: false,
    going further than the readOnlyHint: false I had. Dropped mine.
    memwal_analyze keeps dev's destructiveHint: true; it writes memories,
    so that value is defensible.

What remains is what #706 did not cover.

1. REMEMBER had no turn anchor.

Measured on dev with the #706-equivalent fixes live, T1-T3 from WALM-368,
scored from the MCP log rather than the tool cards:

  T2 recall   PASS  memwal_recall fired unprompted and answered from it
  T1 remember FAIL  no tools/call at all
  T3 control  PASS

Recall firing is new; both earlier dogfood passes recorded zero tools/call
for a whole session. RECALL and REMEMBER ship in the same instructions
block, to the same client, in the same session, so this is a controlled
comparison rather than a guess. The difference is in our own text: RECALL
is anchored to a turn event ("before answering anything that touches..."),
REMEMBER said "without waiting to be asked", which asks the model to
classify a statement and then self-start with nothing tying the call to a
moment. In T1 it replied "Got it - <fact>. What do you need done with
it?", treating a durable fact as task setup.

REMEMBER now anchors with "in that same turn, before you finish replying",
adds configuration values (hostname, port, region, id) since the failing
case was one, and closes the observed behaviour: do not ask whether to
save, and acknowledging a fact in the reply does not store it. #706's
scoping ("skip one-off tasks, the current file or bug, and small talk") is
preserved verbatim. Applied to all three copies; the two that must be
byte-identical were verified so.

2. Tool calls were not scorable.

The host logs `method="tools/call" id=N` with no tool name, so "remember
never fired" and "remember fired and failed" are indistinguishable - the
exact ambiguity under investigation. Logs bridge.tool_call with the tool
NAME and never `arguments`, since memory text is the user's private data.

3. No live check of what a client is actually handed.

test/handshake-contract.mjs drives the real built server against a chosen
env and asserts the delivered contract: instructions present with both
halves anchored, recall advertised read-only, and cold start agreeing with
upstream field by field. Not named *.test.mjs on purpose - it needs a live
relayer and credentials, and the npm test glob must not collect it.
tool-definitions.test.mjs from #706 covers the static list; this covers
the deployed path, which is where the earlier runs went wrong: metadata
lives in services/server/scripts, so a published package alone changes
nothing for a signed-in user, and testing before the relayer redeploys
measures the old server.

packages/mcp 28 pass, services/server/scripts 221 pass.

Refs WALM-324, WALM-368
@harrymove-ctrl
harrymove-ctrl force-pushed the fix/mcp-proactive-descriptions branch from 2c93d22 to bfd793d Compare August 20, 2026 07:17
@railway-app
railway-app Bot temporarily deployed to Walrus Memory / dev August 20, 2026 07:17 Inactive
@harrymove-ctrl harrymove-ctrl changed the title fix(mcp): make the proactive memory contract consistent and anchored fix(mcp): anchor REMEMBER to the turn, log tool names, add a live handshake probe Aug 20, 2026
hien-p added 2 commits August 20, 2026 14:22
…tools

Running the new probe against dev caught drift #706 did not cover. It
synced memwal_remember and memwal_recall, which were the contradictory
ones, and left four others pointing at pre-rewrite copy:

  memwal_remember_bulk  title "Remember Several Facts"  -> "Remember Multiple Facts"
  memwal_health         title "Check Relayer Health"    -> "Check Walrus Memory Health"
  memwal_analyze        description 150 chars -> 340
  memwal_restore        description 165 chars -> 427

Less severe than the remember/recall case, since these are thinner rather
than contradictory: memwal_restore upstream explains when to reach for it
("recall returns nothing even though facts were saved before"), while the
cold-start copy only described the mechanics. A client that caches its
first tools/list keeps the thin version for the whole session.

Values copied programmatically from a live upstream tools/list rather than
retyped, so they cannot drift again through transcription.

login-handoff pinned the two stale titles. Its own comment requires
pre-login discovery to expose "the same safety metadata clients will
receive after the bridge hands off", so the expectations were wrong, not
the change.

Also makes the probe's instruction assertions whitespace-tolerant. The
payload is newline-wrapped, so a phrase match with literal spaces fails on
correct output whenever the phrase straddles a wrap. That produced a false
FAIL on "Do not ask whether / to save it" and would keep doing so on any
future rewording.

packages/mcp 28 pass, services/server/scripts 221 pass,
handshake-contract --dev 13/13.

Refs WALM-368
…aseline

MEMWAL_MCP_SERVER overrides the server path, which defaults to this
checkout's build. Without it the probe can only describe the branch it
sits in, so it could not answer the question that actually matters before
merging: what does dev ship today?

Measured against the real shipping artifacts, published
@mysten-incubation/memwal-mcp@dev 0.0.10-dev.4 talking to the dev relayer
at e4bdcc9 (#706), signed in:

  11/13 passed
  FAIL  REMEMBER is anchored to a turn event
  FAIL  cold-start and upstream lists agree
        (drift: remember_bulk, analyze, restore, health)

Those are exactly the two defects this PR fixes, and they reproduce with
no code of mine in the path: dev's own package, dev's own relayer. The
same probe against this branch reports 13/13.

Useful beyond this PR as the before/after harness for any change to the
handshake contract, and as the pre-flight before a dogfood run: it
distinguishes "the model chose not to call the tool" from "the client was
never handed the contract", which is the ambiguity that made the earlier
WALM-368 runs unreconcilable.

packages/mcp 28 pass.

Refs WALM-368
@harrymove-ctrl
harrymove-ctrl merged commit 4515f00 into dev Aug 20, 2026
14 checks passed
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.

3 participants