fix(mcp): improve auth error messages and add wallet-locked guards#723
Open
fix(mcp): improve auth error messages and add wallet-locked guards#723
Conversation
- Add early wallet-locked checks in MCP request_capability and generate_jwt tools so users get clear "wallet is locked" errors instead of cryptic "main key not found" when the agent has not been unlocked yet - Improve error messages in token.rs generate_jwt/decode_jwt to distinguish between locked wallet and missing key (uninitialized agent) - The old "main key not found. call createMainKey() first" was misleading since the keys exist on disk but are inaccessible until agentUnlock
Four tests covering the previous bug where generate_jwt/decode_jwt returned "main key not found. call createMainKey() first" when the wallet was locked: - generate_jwt_on_locked_wallet_gives_clear_error: verifies locked wallet now returns "Wallet is locked" (not the old misleading message) - decode_jwt_on_locked_wallet_gives_clear_error: same for decode path - generate_jwt_succeeds_when_unlocked: round-trip JWT generate + decode - generate_jwt_without_main_key_gives_not_found_error: verifies unlocked wallet without "main" key says "not found" + "agentGenerate" (not locked) Run with: cargo test --lib agent::capabilities::token::tests -- --test-threads=1
…ck, and tool inventory - Rewrite references/mcp.md: document Streamable HTTP transport (not old SSE), required notifications/initialized handshake, session lifecycle, wallet-locked guards, complete tool inventory, error table, and full curl session example - Update SKILL.md: add wallet unlock prerequisite to quick start and auth section, note session handshake requirement, fix GraphQL default port (12100 not 12000), add restart unlock reminder to executor setup
…, revert docs overhaul Address review from @data-bot-coasys: - token.rs: extract require_unlocked_wallet() helper to deduplicate the locked-wallet guard in generate_jwt and decode_jwt - auth.rs: extract check_wallet_unlocked() helper to deduplicate the guard in request_capability and generate_jwt MCP tools - tests: add comment explaining --test-threads=1 requirement for global Wallet singleton; restore main key in last test cleanup - Revert mcp.md overhaul (will submit as separate docs PR) ⬡
- Restore Bearer header requirement (JWT not auto-stored in MCP session) - Revert GraphQL endpoint port back to 12000
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughEnforces that the Wallet singleton must be unlocked before JWT operations. Added unlock-check helpers used by generate_jwt, decode_jwt, and MCP auth endpoints; strengthened error messages; added tests and documentation updates to require agentUnlock and a two-step MCP handshake. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP / Auth Endpoint
participant JWT as JWT Operation
participant Wallet as Wallet Singleton
rect rgba(100,150,200,0.5)
Note over Client,Wallet: Wallet unlock check added before JWT ops
Client->>MCP: request_capability / generate_jwt call
MCP->>JWT: invoke generate_jwt() / decode_jwt()
JWT->>Wallet: require_unlocked_wallet() / check_wallet_unlocked()
alt Wallet is Locked
Wallet-->>JWT: Error ("Wallet is locked")
JWT-->>MCP: Return structured error JSON
MCP-->>Client: Error response (suggest agentUnlock)
else Wallet is Unlocked
Wallet-->>JWT: OK
JWT->>JWT: Perform signing / decoding (main key / DID checks)
JWT-->>MCP: Return JWT / Claims
MCP-->>Client: Success response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
# Conflicts: # skills/ad4m/SKILL.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/ad4m/SKILL.md (1)
28-36:⚠️ Potential issue | 🟡 MinorDuplicate step number in Quick Start.
After adding the unlock step, the numbering shows two "4." entries:
4. Join neighbourhood → neighbourhood_join_from_url(url: "neighbourhood://Qm...") 32 4. List perspectives → list_perspectives() → find the joined perspective UUIDLines 31-36 should be renumbered 5-8.
📝 Proposed fix
-4. Join neighbourhood → neighbourhood_join_from_url(url: "neighbourhood://Qm...") -4. List perspectives → list_perspectives() → find the joined perspective UUID -5. Read messages → message_query(perspective_id: "...", source: "channel-id") -6. Post a message → add_child(perspective_id: "...", parent: "channel-id", child: "msg-id") +5. Join neighbourhood → neighbourhood_join_from_url(url: "neighbourhood://Qm...") +6. List perspectives → list_perspectives() → find the joined perspective UUID +7. Read messages → message_query(perspective_id: "...", source: "channel-id") +8. Post a message → add_child(perspective_id: "...", parent: "channel-id", child: "msg-id") + message_set_body(perspective_id: "...", uri: "msg-id", value: "Hello!") -7. Set up waker → generate_waker_query() → configure ad4m-waker.js → auto-respond +9. Set up waker → generate_waker_query() → configure ad4m-waker.js → auto-respond🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ad4m/SKILL.md` around lines 28 - 36, The Quick Start numbering has duplicate "4." entries; update the ordered list items starting at "Join neighbourhood → neighbourhood_join_from_url(url: \"neighbourhood://Qm...\")" and the following "List perspectives → list_perspectives()" and subsequent steps so their numeric prefixes increment sequentially (rename the two "4." entries to "5." and "6." and shift the remaining steps accordingly through "Set up waker → generate_waker_query() → configure ad4m-waker.js → auto-respond"), ensuring all step numbers are unique and in order.
🧹 Nitpick comments (2)
skills/ad4m/SKILL.md (1)
193-199: Add language specifier to fenced code block.Per static analysis (MD040): fenced code blocks should have a language specified. This block shows shell-like request/response examples.
📝 Proposed fix
-``` +```text → request_capability(app_name: "MyBot", app_desc: "AI agent", app_url: "http://localhost") ← { request_id: "...", code: "189217" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ad4m/SKILL.md` around lines 193 - 199, The fenced code block in SKILL.md containing the request/response examples for request_capability(...) and generate_jwt(...) lacks a language specifier; update that fenced block to include a language tag (e.g., ```text) so the example is correctly recognized by linters (MD040) and rendered as plain text; locate the block showing the lines starting with "→ request_capability(app_name: "MyBot", ..." and "→ generate_jwt(request_id: "...", ..." and prepend the opening fence with the chosen language.rust-executor/src/agent/capabilities/token.rs (1)
24-37: Consider combining lock acquisition to avoid redundant mutex operations.
require_unlocked_wallet()acquires and releases the wallet lock, then lines 26-28 immediately re-acquire it. While functionally correct, this is slightly inefficient.♻️ Alternative: inline the check or pass wallet ref
One option is to inline the check and reuse the lock:
pub fn generate_jwt( audience: String, expiration_time: u64, capabilities: AuthInfo, ) -> Result<String, AnyError> { - require_unlocked_wallet()?; - let wallet = Wallet::instance(); let wallet_lock = wallet.lock().expect("wallet lock"); let wallet_ref = wallet_lock.as_ref().expect("wallet instance"); + if !wallet_ref.is_unlocked() { + return Err(anyhow!( + "Wallet is locked. The agent must be unlocked (agentUnlock) before generating JWTs." + )); + } let name = "main".to_string();However, the current approach with a separate guard function is cleaner for readability and consistency with the MCP layer's
check_wallet_unlocked(). The minor overhead is negligible in practice—this is a low-frequency auth operation, not a hot path. Feel free to keep as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/agent/capabilities/token.rs` around lines 24 - 37, The code calls require_unlocked_wallet() which acquires/releases the wallet mutex and then immediately re-locks the same mutex via Wallet::instance() / wallet.lock() when fetching the "main" secret key and DID document; to remove the redundant lock, change require_unlocked_wallet() to return the held lock guard (or return a reference/handle to the unlocked wallet) so the caller can reuse that guard instead of re-locking, then use the returned guard to call get_secret_key(&name) and get_did_document(&name) (functions referenced) while holding the same mutex guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@skills/ad4m/SKILL.md`:
- Around line 28-36: The Quick Start numbering has duplicate "4." entries;
update the ordered list items starting at "Join neighbourhood →
neighbourhood_join_from_url(url: \"neighbourhood://Qm...\")" and the following
"List perspectives → list_perspectives()" and subsequent steps so their numeric
prefixes increment sequentially (rename the two "4." entries to "5." and "6."
and shift the remaining steps accordingly through "Set up waker →
generate_waker_query() → configure ad4m-waker.js → auto-respond"), ensuring all
step numbers are unique and in order.
---
Nitpick comments:
In `@rust-executor/src/agent/capabilities/token.rs`:
- Around line 24-37: The code calls require_unlocked_wallet() which
acquires/releases the wallet mutex and then immediately re-locks the same mutex
via Wallet::instance() / wallet.lock() when fetching the "main" secret key and
DID document; to remove the redundant lock, change require_unlocked_wallet() to
return the held lock guard (or return a reference/handle to the unlocked wallet)
so the caller can reuse that guard instead of re-locking, then use the returned
guard to call get_secret_key(&name) and get_did_document(&name) (functions
referenced) while holding the same mutex guard.
In `@skills/ad4m/SKILL.md`:
- Around line 193-199: The fenced code block in SKILL.md containing the
request/response examples for request_capability(...) and generate_jwt(...)
lacks a language specifier; update that fenced block to include a language tag
(e.g., ```text) so the example is correctly recognized by linters (MD040) and
rendered as plain text; locate the block showing the lines starting with "→
request_capability(app_name: "MyBot", ..." and "→ generate_jwt(request_id:
"...", ..." and prepend the opening fence with the chosen language.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 502a26c8-36d7-4b77-b9b3-92b5b6d6b411
📒 Files selected for processing (3)
rust-executor/src/agent/capabilities/token.rsrust-executor/src/mcp/tools/auth.rsskills/ad4m/SKILL.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/ad4m/SKILL.md (1)
240-246:⚠️ Potential issue | 🟡 MinorAdd language specifier to fenced code block.
The fenced code block lacks a language specifier, which makes syntax highlighting unavailable. Since this appears to show a conceptual flow rather than executable code, consider using
textor a more specific identifier.📝 Proposed fix
-``` +```text → request_capability(app_name: "MyBot", app_desc: "AI agent", app_url: "http://localhost") ← { request_id: "...", code: "189217" }As per coding guidelines: markdownlint rule MD040 requires fenced code blocks to have a language specified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/ad4m/SKILL.md` around lines 240 - 246, The fenced example blocks showing the capability/jwt exchange (lines containing request_capability and generate_jwt) lack a language specifier; edit the SKILL.md markdown to add a language tag (e.g., text) after the opening ``` for those fenced code blocks so markdownlint MD040 is satisfied and highlighting is applied, ensuring both blocks that include request_capability(...) and generate_jwt(...) are updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/ad4m/SKILL.md`:
- Line 34: Rename the misspelled function name `set_agetn_profile_picture` to
`set_agent_profile_picture` in the SKILL.md step (and any other occurrences) so
documentation and any references use the correct symbol
`set_agent_profile_picture`; update the example invocation
`set_agetn_profile_picture(image_base64: "34Aff...")` to
`set_agent_profile_picture(image_base64: "34Aff...")`.
- Line 241: The example call to request_capability is missing the required
app_domain argument; update the example to pass app_domain (a String) along with
optional app_url so it matches the function signature (request_capability with
required app_domain and optional app_url) and the Rust client implementation;
locate the example using the request_capability invocation and add an
appropriate app_domain value (e.g., "mybot.example.com") before app_url.
---
Outside diff comments:
In `@skills/ad4m/SKILL.md`:
- Around line 240-246: The fenced example blocks showing the capability/jwt
exchange (lines containing request_capability and generate_jwt) lack a language
specifier; edit the SKILL.md markdown to add a language tag (e.g., text) after
the opening ``` for those fenced code blocks so markdownlint MD040 is satisfied
and highlighting is applied, ensuring both blocks that include
request_capability(...) and generate_jwt(...) are updated.
…ode block language specs
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
1 participant
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.
Summary
Improves MCP authentication error messages and adds early wallet-locked guards.
Replaces #718 (recreated from upstream branch to fix CI stalling on fork PRs).
Problem
When an MCP client calls
request_capabilityorgenerate_jwtbefore the agent has been unlocked (agentUnlock), the error was:This is misleading — the keys exist on disk (encrypted in
agent.json), they just aren't accessible until the wallet is unlocked. MCP clients (especially AI agents) interpret this as needing to call somecreateMainKeyfunction that doesn't exist in the MCP tool surface.Changes
rust-executor/src/agent/capabilities/token.rswallet.is_unlocked()check before key access in bothgenerate_jwtanddecode_jwt"Wallet is locked. The agent must be unlocked (agentUnlock) before generating JWTs."rust-executor/src/mcp/tools/auth.rsrequest_capabilityandgenerate_jwtMCP toolsTesting
Tested manually via curl against MCP server:
request_capabilityreturns"Agent wallet is locked. Call agentUnlock first"request_capability→generate_jwt→ JWT issued successfullynotifications/initializedis required by MCP spec)⬡
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests