feat: add MCP server and det-cli command-line interface#773
Conversation
Embed a streamable-HTTP MCP server (rmcp + axum) that exposes backend tasks as tools for AI agents. Feature-gated behind `mcp` feature flag and opt-in via MCP_API_KEY environment variable. Tools: list_wallets, generate_receive_address, list_core_wallets. Auth: Bearer token with constant-time comparison (subtle crate). Supports network switching -- MCP context follows GUI network changes via ArcSwap. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactor MCP module to support two feature-gated transports: - mcp-stdio: standalone binary with lazy AppContext initialization - mcp-http: embedded HTTP server sharing GUI app's context New binary: dash-evo-tool-mcp (requires mcp-stdio feature). Neither transport is enabled by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dynamic CLI client that discovers tools from the MCP server at runtime. Supports HTTP transport (default) and standalone mode (--standalone). Caches tool schemas locally for shell completion. New binary: det-cli (requires `cli` feature). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add docs/MCP.md covering the HTTP and stdio transport modes, available tools, environment variables, and security notes. Add docs/CLI.md covering det-cli connection modes, subcommands, and shell completion setup. Update README.md with a Programmatic Access section linking to both guides. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a feature-gated MCP (Model Context Protocol) server and a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Auth as Bearer Auth MW
participant Server as Axum MCP Server
participant Service as DashMcpService
participant Backend as Backend Task
participant AppCtx as AppContext (ArcSwap)
Client->>Auth: POST /mcp (Authorization: Bearer <key>)
Auth->>Auth: Constant-time compare
alt valid
Auth->>Server: forward request
Server->>Service: parse tool & params
Service->>AppCtx: load snapshot from ArcSwap
Service->>Service: verify network / resolve wallet / ensure SPV
Service->>Backend: dispatch_task(...)
Backend-->>Service: task result
Service-->>Client: 200 + JSON result
else invalid
Auth-->>Client: 401 Unauthorized (JSON)
end
sequenceDiagram
participant CLI as det-cli
participant Stdio as StdIO Transport
participant HTTP as HTTP Transport
participant Service as DashMcpService
participant AppCtx as AppContext
CLI->>CLI: parse args, load .env
alt serve (standalone)
CLI->>Service: start stdio server (new_lazy)
Stdio->>Service: serve; Service->>AppCtx: init once
else connect (remote)
CLI->>HTTP: connect_http(addr, bearer)
end
CLI->>Service: call_tool(name, params)
Service->>AppCtx: acquire context
Service->>Service: tool dispatch -> backend
Service-->>CLI: return result
CLI->>CLI: format & print
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Load app's .env on startup so MCP_API_KEY/MCP_LISTEN are available without separate DET_CLI_BEARER setup - Replace DET_CLI_BEARER with MCP_API_KEY (matches server config) - Derive server URL from MCP_LISTEN env var; fall back to 127.0.0.1:9527 - Auto-select stdio mode when no API key is configured; HTTP when set - Make subcommand optional: bare `det-cli` lists tools live - Inject cached tool list into --help output via print_cached_tools_help() - Update docs/CLI.md to reflect new config and connection behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove explicit `det-cli cache` subcommand; tool list is cached automatically after every `tools` invocation - Add ToolCache struct with version field; cache includes server version from MCP peer info (falls back to PKG_VERSION) - Cache staleness: --help only shows cached tools when version matches the binary; stale cache prompts user to run `det-cli` to refresh - Add `#[command(version)]` so `det-cli --version` works - Fix server info access: use `client.peer().peer_info()` (correct rmcp API) instead of non-existent `peer_server_info()` - Update bash completion jq path to `.tools[].name` for new cache format - Update docs/CLI.md to reflect automatic caching behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add --features cli to all release build commands and include the det-cli binary in release packages (zip for Linux/Windows, .app bundle for macOS). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Simplify from 3 features (mcp-stdio, mcp-http, cli) to 2: - `mcp`: HTTP server embedded in GUI app - `cli`: det-cli binary with in-process MCP and `serve` subcommand Remove standalone `dash-evo-tool-mcp` binary — `det-cli serve` replaces it as the MCP stdio server for Claude Desktop and AI agents. Update docs (MCP.md, CLI.md, CONTRIBUTING.md) to reflect new layout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add binary output mapping, dependency details, CI usage table, and combination examples for all cargo features. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Write bash completion script to ~/.local/share/bash-completion/completions/det-cli whenever the tool cache is rebuilt. bash-completion 2.x auto-loads from this directory — no .bashrc editing or manual setup needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove `call` subcommand — tools are now first-class subcommands (e.g. `det-cli list-wallets` instead of `det-cli call list_wallets`) - Use hyphens in command and parameter names (CLI convention) - Custom help output shows all available tools as primary content, with management commands and options below - Hyphen-to-underscore conversion for MCP tool/param names Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rewrite examples and usage to reflect hyphenated commands (det-cli list-wallets), remove call subcommand references, document auto-installed bash completion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Both `det-cli` (no args) and `det-cli --help` now display the same format. Bare invocation queries the server for fresh tools first; --help uses the disk cache for instant response. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…efault Network selection now follows the same pattern as the GUI: read from the database (last selected network), with --network flag as override. Removes the MCP_NETWORK env var. Also adds short flags: -n (network), -s (standalone), -a (addr), -b (bearer). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Core wallet operations require a local Dash Core node, which conflicts with the MCP server's SPV-only design. Remove the tool and all references from docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add tracing subscriber (stderr, warn level) for non-serve CLI paths so tracing::error!() from AppContext::new() is visible to users - Pre-validate network config exists before attempting AppContext init, giving a clear error like "no configuration found for network 'Dash'. Check your .env file has MAINNET_dapi_addresses set." - Fall through message now says "check logs for details" instead of the opaque "failed to create AppContext" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the --network CLI flag with a proper `network` MCP tool that returns the active network and available configured networks as JSON. This keeps det-cli as a thin client with no local logic — network info comes from the server, same as all other operations. Changes: - Add `network` tool to DashMcpService (active + available networks) - Remove --network flag from det-cli - Remove network_override threading (ContextProvider::Lazy, new_lazy, init_app_context, start_stdio) - Simplify init_app_context to use DB default only - Add available_network_names() and network_display_name() helpers - Make AppContext.network and data_dir pub (needed by MCP tools) - Update docs/CLI.md and docs/MCP.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Missing network configs are normal — not every user configures all four networks. Remove tracing::error! spam and the hard mainnet requirement. Only fail when no networks are configured at all. The actual network availability check happens in init_app_context where it matters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Handle rmcp ServiceError with typed matching instead of string parsing. McpError variant extracts message + code directly from ErrorData fields. - Suppress rmcp service-level tracing warnings (duplicates handled errors) - Revert network tool to use AppContext (correct for embedded mode — needs working config to be useful) - Make AppContext.network and data_dir pub (needed by MCP tools) - Add collect_available() helper for listing configured networks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In CLI/stdio mode, init_app_context now starts the SPV client after creating AppContext so wallet operations work. The init returns immediately — tools like list-wallets and network are not blocked. The generate-receive-address tool polls ConnectionStatus via trigger_refresh() until SPV reaches Synced state (60s timeout), then verifies the specific wallet is loaded into SPV's det_wallets map before dispatching the backend task. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Intercept --help/-h in tool subcommand args and print tool-specific help from the disk cache instead of forwarding to the MCP server. Uses cached tool metadata to show description, usage, and parameters with required/optional markers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add two new MCP tools: - wallet_balances: read total/confirmed/unconfirmed balances in duffs - send_core_funds: send DASH from a wallet to an address via CoreTask Extract SPV wait loop from generate_receive_address into a reusable wait_for_spv_sync() helper shared by both SPV-dependent tools. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The generate_completion() function was emitting a standalone _det_cli_tools function with its own `complete -F` registration, overriding clap's built-in completions. Now uses the same _det_cli_wrapper pattern as install_bash_completion() — calls clap's _det-cli first, then adds dynamic tool/parameter names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
# Conflicts: # Cargo.lock
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/mcp/server.rs (1)
158-163:⚠️ Potential issue | 🟠 MajorDatabase read errors still silently fall back to mainnet.
The
.ok().flatten().unwrap_or(Network::Mainnet)pattern discards database errors, making a settings read failure indistinguishable from "no saved network yet". This undermines the network safety guard since a corrupted or locked database would silently boot on mainnet when the user may have configured testnet/devnet.Suggested fix
- let network = db - .get_settings() - .ok() - .flatten() - .map(|(network, ..)| network) - .unwrap_or(Network::Mainnet); + let network = match db.get_settings() { + Ok(Some((network, ..))) => network, + Ok(None) => Network::Mainnet, + Err(e) => { + return Err(McpError::internal_error( + format!("settings read: {e}"), + None, + )); + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/server.rs` around lines 158 - 163, The current use of db.get_settings().ok().flatten().unwrap_or(Network::Mainnet) in server.rs hides database errors and treats them the same as "no settings"; change the handling of get_settings() so Err is not converted into a default Mainnet: call db.get_settings(), match on its Result and handle Err(e) explicitly (e.g., log and return Err / propagate the error or abort startup) while only using Network::Mainnet for the Ok(None) case; update the code around the network variable assignment (the get_settings / network binding) to distinguish Ok(Some((network, ..))), Ok(None) -> Network::Mainnet, and Err(e) -> fail fast or return the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/mcp/server.rs`:
- Around line 158-163: The current use of
db.get_settings().ok().flatten().unwrap_or(Network::Mainnet) in server.rs hides
database errors and treats them the same as "no settings"; change the handling
of get_settings() so Err is not converted into a default Mainnet: call
db.get_settings(), match on its Result and handle Err(e) explicitly (e.g., log
and return Err / propagate the error or abort startup) while only using
Network::Mainnet for the Ok(None) case; update the code around the network
variable assignment (the get_settings / network binding) to distinguish
Ok(Some((network, ..))), Ok(None) -> Network::Mainnet, and Err(e) -> fail fast
or return the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae6f0d4d-a38b-4bc7-84e0-a9a2d9dfeace
📒 Files selected for processing (1)
src/mcp/server.rs
There was a problem hiding this comment.
🔍 Claudius Code Review — PR #773 (MCP Server & det-cli)
4 agents reviewed 31 files (2553 lines changed). 28 unique findings after deduplication across security, project consistency, Rust code quality, and documentation.
Summary
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 4 |
| LOW | 16 |
| INFO | 8 |
Verdict
Approve with comments. The implementation is solid — clean dual-transport architecture, proper auth with constant-time comparison, good feature gating. The 4 MEDIUM findings posted inline should be addressed before or shortly after merge:
- SEC-001: Release workflow uses
cargodirectly instead ofsafe-cargo.sh(CI secret exposure) - SEC-002: Release builds missing
headlessfeature (documented feature won't work in binaries) - SEC-003: Network safety check optional on destructive tools (wrong-network fund transfer risk)
- DOC-001: curl examples may not work with Streamable HTTP transport
Full report (30 findings with details): available in CI artifacts.
🤖 Co-authored by Claudius the Magnificent AI Agent
📊 View full HTML review report
…send Replace String-based McpToolError with typed thiserror variants (WalletNotFound, InvalidParam, NetworkMismatch, SpvSyncFailed, TaskFailed, Internal) each mapped to a distinct MCP error code. Add input validation at the MCP layer for SendCoreFunds: reject zero amounts, validate Dash address prefix format, and require the 'network' parameter on destructive operations to prevent accidental cross-network transfers. Add INTENTIONAL(PROJ-003) annotation for tool_describe naming. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Enforce minimum 16-character MCP_API_KEY at server startup; refuse
to start and log error if too short.
- Return JSON body {"error": "unauthorized"} on 401 instead of bare
status code.
- Log failed auth attempts at warn level with client address (never
log the attempted key).
- Add ConnectInfo<SocketAddr> extraction for auth logging.
- Document why the dispatch channel receiver is intentionally dropped
in headless/CLI mode.
- Fix collapsible_if clippy lint in server.rs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Change AppContext.data_dir and .network from pub to pub(crate) and add data_dir()/network() accessor methods for external consumers. - Extract the bash completion wrapper script to a shared constant in completion.rs, eliminating duplication between generate and install. - Document TOCTOU-safe pattern in DashMcpService::ctx() docstring (each tool calls ctx() exactly once and passes the Arc snapshot). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tracing::debug! before .ok() when parsing per-network configs from environment variables, so parse errors are visible in logs instead of silently falling through to "no config found". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
15 tests covering: - API key length validation (too short rejected, 16+ accepted, empty) - Amount validation (zero rejected, positive accepted) - Address format validation (empty, valid mainnet/testnet, invalid prefix) - McpToolError Display output for all variants - MCP error code distinctness for custom codes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add note in MCP.md that headless mode requires building from source. - Fix CONTRIBUTING.md feature flag table: clarify headless depends on cli + mcp rather than claiming all features are independent. - Add MCP-001 and MCP-002 user stories for CLI wallet management and AI agent MCP server access. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add docs/EXPOSING_BACKEND_TASKS.md — a concise reference with architecture rules, step-by-step checklist, and don'ts for adding new MCP tools that wrap BackendTask variants. Cross-reference from CLAUDE.md and CONTRIBUTING.md. https://claude.ai/code/session_01D4TGENSgE6fFCrXn5iPMnD
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/mcp/tests.rs (1)
12-46: Consider usingserial_testor similar for environment variable tests.These tests manipulate global environment variables (
MCP_API_KEY,MCP_LISTEN) which can cause flaky behavior when tests run in parallel. Consider using theserial_testcrate with#[serial]attribute, or restructuring to use test-specific isolation.♻️ Example using serial_test
+use serial_test::serial; + #[test] +#[serial] fn api_key_too_short_returns_none() { // ... } #[test] +#[serial] fn api_key_exactly_16_chars_accepted() { // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/tests.rs` around lines 12 - 46, These tests (api_key_too_short_returns_none, api_key_exactly_16_chars_accepted, api_key_empty_returns_none) mutate global env vars and must run serially; add the serial_test crate and annotate each test with the #[serial] attribute (or apply a module-level #[serial] if available), import serial_test::serial in the test module, and keep cleanup (remove_var) as-is to ensure deterministic behavior when calling McpConfig::from_env().src/mcp/resolve.rs (1)
150-160: Address prefix validation may reject valid regtest addresses.The prefix check only allows
X,7,y,8,9. Regtest addresses may use different prefixes depending on configuration. Since this is documented as a "quick sanity check" and full validation happens at the backend, this is acceptable, but consider adding a note about regtest limitations.Optional: Add regtest note to comment
// Dash mainnet: X (P2PKH), 7 (P2SH) // Dash testnet/devnet: y (P2PKH), 8 (P2SH), 9 (P2SH) + // Note: Regtest may use different prefixes; full validation at backend. let first = address.as_bytes()[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/resolve.rs` around lines 150 - 160, The quick prefix sanity check using address.as_bytes()[0] and matches!(first, b'X' | b'7' | b'y' | b'8' | b'9') can incorrectly reject regtest Dash addresses; update the surrounding comment above this validation (and/or the error message produced by McpToolError::InvalidParam) to explicitly state that this is a lightweight sanity check which may not accept regtest/custom-prefix addresses and that full validation occurs on the backend, so regtest users should be aware of this limitation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp/error.rs`:
- Around line 18-19: TaskFailed/TaskError::Config is leaking raw
ConfigError::LoadError strings (including file paths) because LoadError uses
#[error("{0}")] and TaskError::Config is #[error(transparent)]; change the
ConfigError::LoadError display to a sanitized, user-facing message (e.g. replace
#[error("{0}")] with #[error("failed to load configuration")] or similar), keep
the original inner error as #[source] if you need to preserve the cause for
internal logs but do not include it in the formatted display so that
TaskError::Config no longer exposes filesystem paths.
In `@src/mcp/mod.rs`:
- Around line 7-14: The server and tools modules are declared unconditionally
but depend on the optional rmcp crate and feature-gated enum variants, causing
build failures when neither "mcp" nor "cli" is enabled; update the module
declarations for server and tools to be conditional using cfg(any(feature =
"mcp", feature = "cli")) so they are only compiled when at least one feature is
enabled, and ensure any references to the ContextProvider enum (the variants
currently behind feature flags) and rmcp imports inside server.rs and
tools/mod.rs are only compiled under the same cfg to avoid a zero-variant enum
or missing dependency.
---
Nitpick comments:
In `@src/mcp/resolve.rs`:
- Around line 150-160: The quick prefix sanity check using address.as_bytes()[0]
and matches!(first, b'X' | b'7' | b'y' | b'8' | b'9') can incorrectly reject
regtest Dash addresses; update the surrounding comment above this validation
(and/or the error message produced by McpToolError::InvalidParam) to explicitly
state that this is a lightweight sanity check which may not accept
regtest/custom-prefix addresses and that full validation occurs on the backend,
so regtest users should be aware of this limitation.
In `@src/mcp/tests.rs`:
- Around line 12-46: These tests (api_key_too_short_returns_none,
api_key_exactly_16_chars_accepted, api_key_empty_returns_none) mutate global env
vars and must run serially; add the serial_test crate and annotate each test
with the #[serial] attribute (or apply a module-level #[serial] if available),
import serial_test::serial in the test module, and keep cleanup (remove_var)
as-is to ensure deterministic behavior when calling McpConfig::from_env().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bccbe24f-278d-4946-8d26-7e4f678fc6b7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
CLAUDE.mdCONTRIBUTING.mddocs/EXPOSING_BACKEND_TASKS.mddocs/MCP.mddocs/user-stories.mdsrc/bin/det_cli/completion.rssrc/config.rssrc/context/mod.rssrc/mcp/auth.rssrc/mcp/config.rssrc/mcp/dispatch.rssrc/mcp/error.rssrc/mcp/mod.rssrc/mcp/resolve.rssrc/mcp/server.rssrc/mcp/tests.rssrc/mcp/tools/meta.rssrc/mcp/tools/network.rssrc/mcp/tools/platform.rssrc/mcp/tools/wallet.rs
✅ Files skipped from review due to trivial changes (5)
- CONTRIBUTING.md
- src/mcp/config.rs
- docs/EXPOSING_BACKEND_TASKS.md
- src/bin/det_cli/completion.rs
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/context/mod.rs
- src/mcp/dispatch.rs
- src/mcp/auth.rs
- src/mcp/tools/platform.rs
- Fix jq query in bash completion: use `input_schema` (snake_case) instead of `inputSchema` (camelCase) to match rmcp serialization - Validate hex seed hash exists in wallet map before accepting it, preventing misleading errors for non-existent wallets - Version-check cache in print_tool_help() to reject stale entries - Add headless subcommand to custom help output (cfg-gated) - Return non-zero exit for unknown tool help lookups - Add Accept: application/json header to MCP curl examples Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/bin/det_cli/help.rs (1)
60-63: Prefer a single owner for unknown-tool error output.Line 61 prints an error, and the caller path also raises an error for unknown tools; this can produce duplicate user-facing messages. Consider returning only status/result here and printing in one layer.
Possible simplification
None => { - eprintln!( - "Unknown tool '{cli_name}'. Run 'det-cli tools' to refresh the command list." - ); false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/det_cli/help.rs` around lines 60 - 63, The None branch in the match (the unknown-tool handling in src/bin/det_cli/help.rs) is printing an error directly (eprintln!), which duplicates messages because the caller also reports the same error; instead remove the direct eprintln! and have the function return an error/result indicating "unknown tool" so the caller is the single place that prints user-facing messages—modify the match None arm to return a Result::Err (or a custom UnknownTool error) and update the surrounding function signature to propagate that Result rather than performing I/O here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/MCP.md`:
- Line 154: Change the sentence that currently reads "Stdio mode has no
authentication — the caller controls which process connects." to use lowercase
"stdio" for consistency with the rest of the document; update the exact phrase
"Stdio mode has no authentication — the caller controls which process connects."
to "stdio mode has no authentication — the caller controls which process
connects." so all occurrences match the normalized casing.
- Line 3: Update the introductory sentence that currently reads "Two modes are
available:" to reflect the actual number of modes; change it to "Three modes are
available:" so it correctly matches the documented modes (HTTP, stdio, headless)
referenced later in the MCP intro line that starts with "Dash Evo Tool exposes
wallet and core operations via the Model Context Protocol".
- Around line 79-84: The curl example calling the /mcp endpoint (JSON-RPC method
"tools/call" with "core_wallets_list") is missing an Accept header; update the
example to include -H "Accept: application/json" alongside the existing
Authorization and Content-Type headers so the request aligns with other HTTP
examples and clearly indicates the client expects JSON responses.
In `@src/bin/det_cli/help.rs`:
- Around line 175-181: The parameter parsing currently allows an empty key
(e.g., "=value") which gets inserted as an empty map key; after splitting the
param with split_once into (key, value) add a validation that key is not empty
and fail fast with a clear CLI error (using the same error construction style as
ok_or_else) before calling serde_json::from_str and map.insert; reference the
local variables param, key, value, json_value, and map and return an Err or
propagate an error with a message like "Invalid parameter '{param}': key must
not be empty".
---
Nitpick comments:
In `@src/bin/det_cli/help.rs`:
- Around line 60-63: The None branch in the match (the unknown-tool handling in
src/bin/det_cli/help.rs) is printing an error directly (eprintln!), which
duplicates messages because the caller also reports the same error; instead
remove the direct eprintln! and have the function return an error/result
indicating "unknown tool" so the caller is the single place that prints
user-facing messages—modify the match None arm to return a Result::Err (or a
custom UnknownTool error) and update the surrounding function signature to
propagate that Result rather than performing I/O here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cb70356-b3c8-48df-902f-96fa485714ec
📒 Files selected for processing (5)
docs/MCP.mdsrc/bin/det_cli/completion.rssrc/bin/det_cli/help.rssrc/bin/det_cli/main.rssrc/mcp/resolve.rs
✅ Files skipped from review due to trivial changes (1)
- src/mcp/resolve.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bin/det_cli/main.rs
- src/bin/det_cli/completion.rs
There was a problem hiding this comment.
Pull request overview
This PR adds a Model Context Protocol (MCP) server plus a new det-cli binary to expose Dash Evo Tool functionality programmatically (wallet/network/platform operations) over both HTTP (embedded in GUI) and stdio (CLI/server mode), with feature-flagged builds and supporting documentation/CI updates.
Changes:
- Introduces the MCP service (
DashMcpService), tool router, tool implementations, and shared validation/dispatch helpers. - Adds
det-cliwith dynamic tool discovery, cached metadata, help output, and shell completion; includes stdio server + optional headless HTTP daemon. - Updates docs, feature flags, and release workflow to build/package the new CLI.
Reviewed changes
Copilot reviewed 27 out of 34 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mcp/tools/wallet.rs | Adds core wallet tools (address, balances, send funds) and platform address balance tool |
| src/mcp/tools/platform.rs | Adds Platform withdrawals query tool |
| src/mcp/tools/network.rs | Adds network info and loaded-wallet listing tools |
| src/mcp/tools/mod.rs | Defines shared tool parameter structs (wallet/network/empty/tool-name) |
| src/mcp/tools/meta.rs | Adds meta tool to describe tool schemas/metadata |
| src/mcp/tests.rs | Adds unit tests for MCP validation and error formatting/mapping |
| src/mcp/server.rs | Implements MCP service, context providers (ArcSwap/OnceCell), tool router registration, and CLI-mode AppContext init |
| src/mcp/resolve.rs | Adds shared param resolution and guards (network check, wallet resolution, SPV sync wait, basic validation) |
| src/mcp/mod.rs | Adds MCP module entry points for stdio and HTTP server startup |
| src/mcp/error.rs | Defines McpToolError and conversion to rmcp error payloads/codes |
| src/mcp/dispatch.rs | Bridges MCP tool calls to the existing backend task system |
| src/mcp/config.rs | Adds MCP HTTP configuration from environment variables |
| src/mcp/auth.rs | Adds Axum bearer-auth middleware for HTTP MCP endpoint |
| src/lib.rs | Exposes mcp module behind feature flags |
| src/context/mod.rs | Adds getters needed by MCP (data_dir, network) |
| src/config.rs | Makes network config loading tolerant of missing networks (debug logs vs hard errors) |
| src/bin/det_cli/main.rs | Adds CLI entry point, mode selection (stdio vs HTTP), subcommands, and tool invocation path |
| src/bin/det_cli/help.rs | Adds dynamic help/tool help rendering and key=value argument parsing |
| src/bin/det_cli/headless.rs | Adds headless HTTP MCP daemon mode (feature-gated) |
| src/bin/det_cli/connect.rs | Adds in-process MCP connection wiring and HTTP transport client connection |
| src/bin/det_cli/completion.rs | Adds shell completion generation plus bash wrapper for dynamic completion |
| src/bin/det_cli/cache.rs | Adds on-disk versioned tool metadata cache and completion install hook |
| src/app.rs | Starts embedded HTTP MCP server (feature-gated) and updates server context on network switches |
| docs/user-stories.md | Documents new MCP user stories for programmatic access |
| docs/MCP.md | Adds MCP usage/config/tool reference documentation |
| docs/EXPOSING_BACKEND_TASKS.md | Adds contributor guide for exposing backend tasks via MCP/CLI tools |
| docs/CLI.md | Adds det-cli usage and examples documentation |
| README.md | Links to new MCP/CLI docs |
| Cargo.toml | Adds dependencies, features (mcp, cli, headless), and det-cli bin target |
| Cargo.lock | Locks new dependency graph for MCP/CLI additions |
| CONTRIBUTING.md | Documents feature flags and tool-extension workflow |
| CLAUDE.md | Documents MCP/CLI architecture conventions and usage |
| .github/workflows/release.yml | Updates release build to include det-cli artifact packaging |
| .env.example | Adds MCP env vars (MCP_API_KEY, MCP_LISTEN) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review — PR #773 (MCP Server + det-cli)
Overall this is well-architected — clean separation between tools/backend dispatch/transport layers, good use of feature gating, and the security fundamentals (constant-time auth, minimum key length, network verification) are solid. A few issues need attention, one of which is a schema/behavior mismatch on the destructive fund-sending tool.
🚫 Schema/runtime mismatch on SendFundsParams.network (blocking)
SendFundsParams.network is Option<String> without #[serde(default)]. This creates a three-way inconsistency:
- schemars sees
Option<T>→ generates JSON Schema wherenetworkis NOT required - serde without
#[serde(default)]→ rejects absentnetworkwith genericmissing fielderror require_network()→ the carefully written error message (The 'network' parameter is required for fund-sending operations...) is never reached
MCP clients (Claude, Cursor, etc.) rely on the schema to determine mandatory parameters. They'll see network as optional, omit it, and hit a confusing serde error instead of the safety explanation. For a destructive fund-sending operation, this is particularly problematic.
Suggested fix: Make network a required String (not Option) in SendFundsParams. This aligns the schema (schemars will put it in required), serde behavior, and the tool description which already says "The network parameter is required." Then require_network() can be replaced with a simple verify_network() call since the value is always present.
⚠️ SPV startup failure silently swallowed in headless/stdio mode
init_app_context() logs start_spv() failures with tracing::warn! and still returns a live AppContext. The MCP tools then call ensure_spv_synced() which polls until the 600s timeout. A bad SPV config now produces a server that appears healthy but makes every request hang for 10 minutes before failing with a generic sync error.
For a headless daemon, startup should either fail fast or enter an explicit degraded mode that rejects SPV-dependent tools synchronously (rather than timing out).
⚠️ SPV readiness gate blocks RPC-mode users
ensure_spv_synced() is a blanket prerequisite for all wallet/platform tools, but init_app_context() preserves the user's saved core_backend_mode from the database. If a user's DET profile is set to RPC mode, tools like core_balances_get and core_funds_send can still execute through existing task paths — but the MCP layer forces them through full SPV sync first, causing 10-minute stalls or spurious failures.
The readiness gate should be conditional on core_backend_mode == Spv.
💡 Env var tests not thread-safe
The three MCP_API_KEY tests (api_key_too_short_returns_none, api_key_exactly_16_chars_accepted, api_key_empty_returns_none) all manipulate the same environment variable. cargo test runs these in parallel by default → potential race conditions. Consider adding serial_test crate and marking them with #[serial].
💡 Config loading behavioral change affects GUI app
The change to Config::load_from() downgrades individual network config parse failures from error! to debug!. This is needed for MCP/CLI (users may only configure testnet), but it modifies shared code used by the GUI. A GUI user with a subtle mainnet .env typo would now silently fall through to another network instead of getting a startup error. Consider using tracing::warn! instead of debug! so genuine misconfigurations are visible.
💡 Bearer auth middleware has no unit tests
The bearer_auth middleware is the sole access control mechanism for the HTTP MCP server (which can send funds). Recommended: test valid/invalid/missing/wrong-scheme bearer tokens via axum's TestClient.
Nitpick: network_display_name wildcard
The _ => "unknown" catch-all means a new upstream Network variant silently returns "unknown" instead of producing a compile error. Since this is used for network verification, a new variant would cause all network checks to silently fail for it. If the upstream enum is #[non_exhaustive] this is unavoidable, but worth a comment explaining the rationale.
- docs/MCP.md: mark `network` as required for `core_funds_send` - docs/CLI.md: add `network=testnet` to send example, remove misleading "optional" example - src/app.rs: initialize MCP HTTP server with context matching `chosen_network` instead of hardcoded mainnet - src/mcp/tools/wallet.rs: make `SendFundsParams.network` a non-optional `String` to match runtime requirement - docs/EXPOSING_BACKEND_TASKS.md: fix typo `InvalidParams` → `InvalidParam` - CLAUDE.md: fix same `InvalidParams` → `InvalidParam` typo - src/mcp/tests.rs: remove env var tests that race with parallel tests due to unsynchronized `set_var`/`remove_var` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 34 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bot issues fixed: - #35: WalletNotFound stores only the identifier, not full error sentence - #36: MCP log differentiates unset vs invalid MCP_API_KEY - #37: Release workflow splits GUI and CLI builds (GUI without --features cli, CLI with --features headless --bin det-cli) - #38: SendCoreFunds validates empty network string before require_network - #39: Remove unnecessary ensure_spv_synced() from QueryWithdrawals - #40: Remove input_schema() override from NetworkTool Human issues addressed: - #41: Tool table includes det-cli command column - #42: CLI section added to MCP.md - #43: Headless mode performance characteristics documented - #45: Authentication section with setup, behavior, and security guidance - #46: ListWalletsTool moved from network.rs to wallet.rs - #47: TODO added for withdrawal pagination (requires backend changes) - #48: TODO added for structured JSON output (requires new backend variant) - #49: Verified correct BackendTask dispatch for withdrawals Issue #44 (CLAUDE.md duplication): no actual duplication found — the MCP section in CLAUDE.md covers architecture guidance while docs/MCP.md covers usage. Both serve distinct purposes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Add an MCP (Model Context Protocol) server and
det-clicommand-line interface for programmatic access to Dash Evo Tool — enabling AI agents and scripts to manage wallets, query Platform, and interact with the Dash network.Key capabilities
det-cli serve{domain}_{object}_{action}naming convention:network_info— active network and configured networkscore_wallets_list— loaded wallets (alias + seed hash)core_address_create— generate receive addresscore_balances_get— wallet balances in duffscore_funds_send— send DASH to an addressplatform_addresses_list— platform credits and noncesplatform_withdrawals_get— query withdrawal documentstool_describe— full MCP tool definition for a tooldet-cliCLI: direct subcommands with--help, bash completion, versioned tool cachenetworkparam — request fails if it doesn't match the server's active network, preventing cross-network mistakesdet-cli headless): HTTP MCP server without GUIArchitecture
ToolBase+AsyncTool<DashMcpService>) — adding tools requires zero CLI changestools/list, works over both in-process and HTTP transportsContextProviderenum:Shared(ArcSwap)for HTTP mode (follows GUI network switches),Lazy(OnceCell)for stdio modemcp(HTTP/GUI),cli(stdio/CLI),headless(both)Test plan
det-cli— lists all 8 tools with new namesdet-cli serve— responds to MCPinitialize+tools/listover stdiodet-cli network-info— returns active network and available networksdet-cli core-wallets-list— returns loaded wallets (requires SPV sync)det-cli core-balances-get wallet-id=<alias>— returns balances in duffsdet-cli core-funds-send wallet-id=<alias> address=<addr> amount-duffs=1000— sends fundsdet-cli platform-withdrawals-get status=queued— queries Platform withdrawalsdet-cli core-balances-get wallet-id=<alias> network=wrongnet— returns mismatch error/mcpshows DET server connected with 8 tools🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests
CI / Build