feat(mcp): single-source the tool surface, add read-only mode, paid-operation policy, and concurrency tests - #768
Merged
manoahLinks merged 4 commits intoAug 31, 2026
Conversation
The mcp package does not build or test on main. Three separate bad merges left duplicated declarations and a stale snapshot file behind: - auditLog.ts had two `emit` implementations — one writing to the file sink, the other stamping the correlation ID — so `tsc` rejected the module and every suite importing it collected zero tests. Merged into one exit point that does both, and imported the missing `currentCorrelationId`. - index.ts came out of "Merge branch 'main' into feat/mcp-545-catalog-resources" with both sides of eight conflicting hunks concatenated: two `Mutex` imports (one from `async-mutex`, which is not a dependency), two `safeErrorMessage` imports, two `browse` definitions, two `toolMetrics`, and a `ToolOutcome`/`string` return-type clash in the dispatcher. Redoing that three-way merge yields the pre-merge main file plus exactly the four additions mind-vault-1#545 contributed (the resources capability, the two ListResources/ReadResource handlers, and their imports), which is what this commit applies. - preview() lost its mind-vault-1#556 offline-snapshot fallback when mind-vault-1#582's preview limits landed on top of it, leaving `index.test.ts` asserting a cache path the function no longer had. Restored as `previewData`, composed with `applyPreviewLimits` so both behaviours hold. - toolSchemaSnapshots snapshots were written before mind-vault-1#553 added output schemas to TOOL_DEFINITIONS and never regenerated, so 25 cases expected `null` where a schema is now declared. Regenerated. mcp: 67 files / 2112 tests pass, `tsc --noEmit` is clean, and `smoke:install` drives the built server over stdio.
…aid-operation policy Closes mind-vault-1#596, mind-vault-1#593, mind-vault-1#594. ## mind-vault-1#596 — list_tools contract drift check tools.ts calls itself "the single source of truth for the tool surface advertised to agent clients", and scripts/generate-tool-docs.ts builds docs/mcp-tool-reference.md on that promise. It was not true: the ListTools handler in index.ts carried its own ~490-line literal copy, and the two had drifted apart in every direction at once. - Six implemented, validated, documented tools were missing from the copy and so undiscoverable: mindvault_update_metadata, _set_price, _transfer_ownership, _set_listed, _export_receipts, _recover_catalog_cache. - mindvault_publish_status and mindvault_purchase_history existed only in the copy, so the generated reference never listed them and the schema snapshots never covered them. - mindvault_reset advertised a `confirm` argument that resetGuard reads and TOOL_ARGUMENT_SPECS did not declare, so the validator rejected it as an unknown argument. The tool could never be confirmed — it was permanently stuck returning its preview. - mindvault_publish and mindvault_buy had stopped advertising `dryRun` and `maxAutoPayUsdc`, and several schemas had lost their field descriptions and examples. toolSurface.ts now derives the ListTools payload from TOOL_DEFINITIONS, which removes the cause; listToolsContract.test.ts covers the category. A tool is four declarations — definition, argument spec, dispatch case, output schema — and any one can be added without the others. The test checks all four agree against the response a real client receives over a real transport, and was verified to fail on three injected regressions (a definition with no handler, an argument the validator rejects, a tool dropped from the surface). mindvault_set_tags is defined and validated but has no dispatch case, as docs/mcp-structured-output.md records. It is withheld rather than advertised — an agent that calls an advertised tool and gets `Unknown tool` learns nothing it can act on — and TOOLS_WITHOUT_HANDLERS is asserted to name exactly the tools in that state, in both directions. EXTRA_TOOL_ANNOTATIONS and EXTRA_OUTPUT_SCHEMAS existed only to patch over the two tools missing from TOOL_DEFINITIONS. Both are gone. ## mind-vault-1#593 — read-only mode for catalog browsing MINDVAULT_READ_ONLY=1 restricts the server to catalog browsing. Unlike the mainnet guardrail it is operator-scoped rather than network-scoped, and cannot be lifted by a tool argument — confirmMainnet is supplied from inside the very call you wanted to prevent. ListTools advertises only tools declaring readOnlyHint, and dispatch refuses the rest. Both are needed: the listing keeps an agent from planning around a tool it cannot use, but a client with a cached list still calls it, so the gate that enforces the mode is the one in the dispatcher. The refusal names what is still available. Classification reads each tool's own readOnlyHint, so a tool added later is covered by what it declares about itself. ## mind-vault-1#594 — paid-operation confirmation policy MINDVAULT_CONFIRM_PAID_OPERATIONS=off|usdc|all requires confirmPaid: true before a tool spends. It is a third axis, not a replacement: the mainnet guardrail asks *where* a spend happens and never fires on testnet; the auto-pay ceiling asks *how much* and permits a hundred cheap purchases. This asks whether the caller meant to spend at all. Default is off, so an upgrade changes no existing deployment. Dry runs are exempt — gating one would mean confirming a spend to discover what the spend would be. mindvault_setup_wallet is not gated: the sponsored-account service funds it, not the agent's wallet. An unrecognized value raises rather than falling back to off, because a typo that silently disables a safety setting leaves the operator believing they are protected. Tests pin that confirmPaid and confirmMainnet cannot substitute for each other. ## Verification mcp: 71 files / 2258 tests pass, including 31 consecutive runs and 6 with --sequence.shuffle. Workspace `pnpm test`, eslint (0 errors), prettier, the mcp build, and smoke:install all pass. index.ts drops 586 lines.
Closes mind-vault-1#591. Several MCP tools do a read-modify-write against the module-level `profiles` map and then persist the whole map to ~/.mindvault/state.json. The window between the read and the write is not theoretical — mindvault_import_wallet awaits a dynamic import("@stellar/stellar-sdk") in the middle of it: activeProfileName = target; // read/modify …await… // another call runs here activeProfile().wallet = { … }; // modify, against whatever saveState(); // activeProfileName now says STATE_MUTATING_TOOLS + stateMutex.runExclusive close that window. These tests exist so the closing stays closed. They drive real concurrent dispatchTool calls and assert on the bytes that land on disk; mutex.test.ts already covers the primitive in isolation, which is not the same thing. The failure being guarded against is not a lost write but a misdirected one. Interleaved calls do not drop a profile — they write one call's wallet into another call's profile, which persists cleanly, reads back as valid JSON, and hands the wrong agent a wallet. "Every profile is present" passes while that happens, so the assertions check each profile holds the key meant for it. Twelve overlapping imports are checked for correct per-profile assignment, a parseable state file throughout the run, 0600 preserved across concurrent rewrites, lock release after a failed mutation (a leaked lock would deadlock every later mutating tool and look like a hang rather than a fault), reads interleaved with writes, and a state file left read-only by an earlier run. A final pair models importWallet's exact shape against a local mirror to show which interleaving does the damage: both bodies run to their await before either resumes, so both resume reading the *last* writer's activeProfileName. Two successful calls, no error raised, one profile silently gone. The interleaving is forced with microtask yields rather than timers, so it is deterministic and cannot flake. Keypairs are generated rather than inlined — they only need to be valid and distinct, and a committed S… literal reads like a leaked credential to every scanner that meets it. Nothing here is funded or submitted.
`newCorrelationId > produces distinct ids in a tight loop` minted 500 ids from the real clock and Math.random and asserted all 500 were distinct. It failed roughly one run in fourteen, which was enough to make CI red on unrelated PRs. The failure was correct and the assertion was wrong. A 500-iteration loop completes inside one millisecond, so the time component is constant and all 500 ids are drawn from the suffix alone — 36**4 = 1,679,616 values. By the birthday bound that collides about 7% of the time, which matches the observed rate. The obvious fix — a per-process uniqueness counter — is not available. The test directly above it pins that `newCorrelationId` is deterministic in (clock, random), which exists so other tests can assert exact ids, and a counter would break it. The suffix is sized for real tool calls, each of which does network I/O; a handful per millisecond is already an extreme burst. So the loop is replaced by the two properties the design does guarantee: ids across advancing milliseconds are all distinct, and distinct randomness within one millisecond maps to distinct ids (the mapping is injective — collisions come from Math.random repeating, never from the suffix losing information). A third case pins the suffix at its full four digits, since narrowing it would raise the collision rate, and a collision merges two concurrent calls' audit trails — the failure this module exists to prevent. The module docstring claimed a same-millisecond collision was "effectively impossible". Corrected to state the actual size, what it is sized for, and why a counter is not an option. 20 consecutive full-suite runs pass.
|
@kaluuba-org Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
3 participants
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 #591, Closes #593, Closes #594, Closes #596
All four issues are scoped to
mcp/.mcp/does not build onmainBefore any of the four issues could be worked on — or even tested — the package had to compile. It does not.
pnpm --filter @mindvault/mcp buildfails onmaintoday, andpnpm testcollects zero tests from 15 suites because the modules they import will not parse.Three separate bad merges left duplicated declarations behind:
auditLog.tsemitimplementations — one writing to the file sink, the other stamping the correlation ID.tscrejects the module, so every suite importing it collects nothing.index.tsMerge branch 'main' into feat/mcp-545-catalog-resources(in #757) concatenated both sides of eight conflicting hunks: twoMuteximports (one fromasync-mutex, which is not a dependency), twosafeErrorMessageimports, twobrowsedefinitions, twotoolMetrics, and aToolOutcome/stringreturn-type clash in the dispatcher.preview()index.test.tsasserting a cache path the function no longer had.toolSchemaSnapshotsTOOL_DEFINITIONSand never regenerated — 25 cases expectingnullwhere a schema is now declared.The first commit repairs this. For
index.tsI re-ran the three-way merge properly rather than patching symptoms; the result is the pre-mergemainfile plus exactly the four additions #545 contributed (theresourcescapability, the twoListResources/ReadResourcehandlers, and their imports). Nothing from either branch is dropped.This commit is separable. If you would rather land it on its own first, drop the other commits and I will rebase.
#596 — list_tools contract drift check
tools.tssays of itself that it is "the single source of truth for the tool surface advertised to agent clients", andscripts/generate-tool-docs.tsbuildsdocs/mcp-tool-reference.mdon that promise.It was not true. The
ListToolshandler inindex.tscarried its own hand-maintained ~490-line copy of the list, and the two had drifted apart in every direction at once:mindvault_update_metadata,mindvault_set_price,mindvault_transfer_ownership,mindvault_set_listed,mindvault_export_receipts,mindvault_recover_catalog_cache.mindvault_publish_statusandmindvault_purchase_historyexisted only in the copy, so the generated reference never listed them and the schema snapshots never covered them.mindvault_resetcould never be confirmed. It advertised aconfirmargument,resetGuard.isResetConfirmedreadsconfirm, andTOOL_ARGUMENT_SPECSdid not declare it — so the validator rejected every confirmed reset as an unknown argument and the tool was permanently stuck returning its preview:mindvault_publishandmindvault_buyhad stopped advertisingdryRunandmaxAutoPayUsdc, and several schemas had lost their per-field descriptions and examples.The cause is removed:
toolSurface.tsderives the ListTools payload fromTOOL_DEFINITIONS, andindex.tssheds 586 lines. The category is covered bylistToolsContract.test.ts.A tool is really four declarations — definition, argument spec, dispatch
case, output schema — and any one can be added without the others with nothing failing. The test asserts all four agree, against the response a real client receives over a real transport rather than against the arrays that produce it. I verified it fails on three injected regressions before trusting it:mindvault_set_tagsis defined and validated but has no dispatch case — a known gap thatdocs/mcp-structured-output.mdalready records. I did not implement it (out of scope). It is withheld from ListTools rather than advertised, since an agent that calls an advertised tool and getsUnknown toollearns nothing it can act on.TOOLS_WITHOUT_HANDLERSis asserted to name exactly the tools in that state, in both directions — so implementing it fails the suite until the entry is removed.EXTRA_TOOL_ANNOTATIONSandEXTRA_OUTPUT_SCHEMASexisted only to patch over the two tools missing fromTOOL_DEFINITIONS. Both are now dead and removed.Net effect for agents: 30 → 36 advertised tools, with fuller schemas, and
mindvault_resetworks.#593 — read-only mode for catalog browsing
MINDVAULT_READ_ONLY=1turns the server into a catalog browser.It is deliberately unlike the mainnet guardrail: that one is network-scoped (testnet is wide open) and per-call —
confirmMainnet: truelifts it from inside the very call you wanted to prevent. This is operator-scoped, set once on the process, and no tool argument can override it.Two things change together, and both are load-bearing:
readOnlyHint— so an agent never plans around a tool it cannot use.Classification reads each tool's own
readOnlyHintannotation, so a tool added later is gated by whatever it declares about itself and this cannot fall out of sync with the surface.#594 — paid-operation confirmation policy
MINDVAULT_CONFIRM_PAID_OPERATIONS=off|usdc|allrequiresconfirmPaid: truebefore a tool spends.This is a third axis, not a replacement. The mainnet guardrail asks where a spend happens and never fires on testnet; the auto-pay ceiling asks how much and happily permits a hundred cheap purchases. This asks whether the caller meant to spend at all.
off(default)usdcmindvault_publish,mindvault_buyallregister_onchain,update_metadata,set_price,transfer_ownership,set_listedDesign notes:
off, so an upgrade changes no existing deployment. A guardrail that breaks working setups gets switched off wholesale, which is worse than not shipping it.mindvault_setup_walletis not gated — the sponsored-account service funds it, so the agent's own wallet pays nothing and claiming otherwise would be a lie about what the call costs.off. A typo in a safety setting that silently disables it is worse than one that fails loudly, because the operator believes they are protected.confirmPaidandconfirmMainnetcannot substitute for each other, and that a call subject to both must satisfy both.#591 — concurrent state write regression test
Several tools do a read-modify-write against the module-level
profilesmap and then persist it. The window between read and write is not theoretical —mindvault_import_walletawaits a dynamicimport("@stellar/stellar-sdk")in the middle of it:STATE_MUTATING_TOOLS+stateMutex.runExclusiveclose that window; these tests exist so the closing stays closed. They drive real concurrentdispatchToolcalls and assert on the bytes that land on disk —mutex.test.tsalready covers the primitive in isolation, which is not the same thing.The failure being guarded against is not a lost write but a misdirected one. Interleaved calls do not drop a profile; they write one call's wallet into another call's profile, which persists cleanly, reads back as valid JSON, and hands the wrong agent a wallet. "Every profile is present" passes while that happens — so the assertions check each profile holds the key actually meant for it.
Covered: twelve overlapping imports checked for correct per-profile assignment; the state file parseable throughout the run;
0600preserved across concurrent rewrites; lock release after a failed mutation (a leaked lock would deadlock every later mutating tool and look like a hang rather than a fault); reads interleaved with writes; and a state file left read-only by an earlier run.A final pair models
importWallet's exact shape against a local mirror to show precisely which interleaving does the damage — both bodies reach theirawaitbefore either resumes, so both resume reading the last writer'sactiveProfileName. Two successful calls, no error raised, one profile silently gone. The interleaving is forced with microtask yields rather than timers, so it is deterministic and cannot flake.Keypairs are generated rather than inlined: they only need to be valid and distinct, and a committed
S…literal reads like a leaked credential to every scanner that meets it. Nothing here is funded or submitted.Also included: a pre-existing flaky test
correlation.test.ts > produces distinct ids in a tight loopminted 500 ids from the real clock andMath.randomand asserted all were distinct. It failed roughly one run in fourteen — enough to make CI red on unrelated PRs.The failure was correct; the assertion was wrong. A 500-iteration loop finishes inside one millisecond, so the time component is constant and all 500 ids come from the suffix alone —
36**4= 1,679,616 values, which by the birthday bound collides ~7% of the time.The obvious fix (a uniqueness counter) is unavailable: the test directly above pins that
newCorrelationIdis deterministic in(clock, random), which exists so other tests can assert exact ids. So the loop is replaced by the properties the design does guarantee, and the docstring's claim that a same-millisecond collision is "effectively impossible" is corrected to state the actual size and why a counter is not an option.Also separable if you would prefer it split out.
Verification
make validatepnpm test(workspace)tsc --noEmit(mcp)pnpm --filter @mindvault/mcp buildpnpm --filter @mindvault/mcp smoke:installdist/index.jsmain)--sequence.shuffle, all greenFor reference,
mcp/onmaintoday: 15 suites collecting 0 tests, 25 snapshot failures, build broken.Commits
Each is self-contained and passes the suite on its own:
fix(mcp)— repair the merge damage (prerequisite)feat(mcp)— single-source the tool surface, read-only mode, paid-operation policy ([MCP] Add list_tools contract drift check #596, [MCP] Add read-only mode for catalog browsing #593, [MCP] Add paid-operation confirmation policy #594 — these three sharetoolSurface.ts,tools.tsand the dispatcher, so they land together)test(mcp)— concurrent state write regression tests ([MCP] Add concurrent state write regression test #591)test(mcp)— fix the flaky correlation-id assertionDocs
docs/environment-variables.mdgains both new variables plus a section each;mcp/.env.examplegains commented entries;docs/mcp-tool-reference.mdis regenerated (the staleness guard intoolDescriptions.test.tsenforces this).