Skip to content

Commit 69ea799

Browse files
hanwenchengclaude
andauthored
fix(cli): #15 part 3 (fix-15b) — agentkeys scope command (#29)
* fix(cli): #15 part 3 (fix-15b) — agentkeys scope command Closes part 3 of #15. Adds `agentkeys scope <AGENT>` subcommand with `--add`/`--remove`/`--set`/`--list` for editing a child agent's scope. Under the hood: new trait methods `CredentialBackend::get_scope` + `update_scope` backed by a new mock-server `PUT /session/scope` endpoint (owner-checked, updates all active sessions for the target wallet). Tests: 5 new CLI integration tests (list/add/remove/set/conflict). 62 total tests across cli/core/mock-server, all green under --test-threads=1. Minor: CommandContext::load_session made pub so integration tests can access the current session token. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli/mock-server): #15b v2 — address codex P1+P2 Codex P1: update_scope handler allowed a master session to target its own wallet, silently writing a restrictive scope_json onto the master's session row. Subsequent credential/read calls would start failing for services outside the accidental scope. Now rejects self-targeting with a clear bad_request. Codex P2 (URL encoding): cmd_scope's identity resolution built the /identity/resolve query string via raw interpolation, breaking aliases/emails with reserved characters ('+', '&', '=', '%', spaces). Switched to reqwest's .query() builder which percent-encodes per RFC. Codex P3 (--list exclusivity): --list now rejects combination with --add/--remove/--set up front instead of silently dropping the mutation. Deferred (tracked as follow-up): CBOR vs JSON parsing of ScopeChange request_details in mint_scope_change_session — the scope CLI uses the direct /session/scope endpoint, not the auth-request flow, so this finding doesn't affect the CLI path. Will be addressed when AuthRequest::ScopeChange approval is wired end-to-end. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli/mock-server): #29 v3 — claude P1+P2 review fixes after rebase Post-rebase review (claude code-reviewer) flagged 3 P1 issues that map directly to patterns in `.github/REVIEW_GUIDELINES.md`. All fixed in this commit plus one P2 nit that was cheap. **P1 — URL encoding via reqwest `.query()` on scope + list_credentials** `get_scope` and `list_credentials` in `mock_client.rs` built queries with raw `format!()` interpolation. Wallet strings with reserved characters (`&`, `#`, `%`, `+`, spaces) would break the request or smuggle extra params server-side. Switched both to `.get(self.url("/path")).query(&[...])` so reqwest percent-encodes per RFC 3986. Same pattern as the fix on PR #20 `aa0cc95` / PR #22 `ffdc908` / PR #29 `7aa9e70`. **P1 — Case-insensitive self-target check in `update_scope`** The self-target reject `session.wallet_address == target_wallet` was string-equal, but the backend stores wallets in lowercase while EIP-55 checksummed input preserves case. A master passing its own wallet in mixed-case form would bypass the self-target guard and silently restrict its own scope (the exact failure the guard was designed to prevent). Switched to `eq_ignore_ascii_case` matching the pattern established in PR #18's self-revoke detection. **P1 — Missing DENIED audit rows on scope endpoints** `update_scope` and `get_session_scope` returned 403 on ownership failure without inserting an audit row, breaking the cross-agent-probing-visibility invariant established for `list_credentials` / `read_credential` in PR #19. Added `'scope_update' 'DENIED'` and `'scope_read' 'DENIED'` inserts before the 403 return in both handlers. **P2 — `--add foo --remove foo` would no-op silently** `cmd_scope` accepted combinations like `--add X --remove X`. The add loop would push, then `retain(!remove.contains)` would cancel, and the backend PUT would still fire with the original scope + misleading "Scope updated" output. Added an up-front `add ∩ remove` check that lists overlapping services and rejects. **P2 — Narrow UPDATE to most-recent active session** Write-side `update_scope` UPDATEd ALL active sessions for the target wallet; read-side `get_session_scope` always returned the scope from the most-recent session via `ORDER BY created_at DESC LIMIT 1`. Read/write contract mismatched on wallets with multiple active sessions (paired + recovered). Narrowed the UPDATE to use the same ORDER/LIMIT subquery. Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1 core: 22 passed; cli: 35 passed; mock-server: 56 passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli/mock-server): #29 v4 — claude re-review nits (dead sort, pct_encode consistency, test coverage) Follow-up on the claude-code-action re-review posted against commit 57dc9fb. All 3 substantive findings addressed; the 4th (malformed `/` doc-comments) was a false positive from pre-rebase state — grep for `^\s*/ ` returns nothing in the current tree. **Low — Redundant `sort_by` after `update_scope`** `lib.rs:769` was re-sorting `new_scope.services` after the backend call returned, but both the `--set` branch (line 749) and the `--add`/`--remove` branch (line 760) sort before the `update_scope` call. Dead op; removed. Added a comment so a future reader doesn't re-add it. **Low — `InProcessBackend::get_scope` raw URL concat** `test_client.rs:725` built the query string via `format!()` while the `resolve_identity` method right above uses `pct_encode`. Wallet strings are hex so this was safe in practice, but the inconsistency violates the URL-encoding invariant documented in `.github/REVIEW_GUIDELINES.md` pattern #3. Swapped to `pct_encode`. **Low — Missing test for `--list + --add` conflict** Added `cmd_scope_list_and_add_conflict_errors`. Also added `cmd_scope_add_remove_overlap_errors` (covering the P2 overlap guard from v3) that wasn't yet under test. Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1 core: 22 passed; cli: 37 passed (+2 new); mock-server: 56 passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 0b68f94 commit 69ea799

11 files changed

Lines changed: 888 additions & 13 deletions

File tree

crates/agentkeys-cli/src/lib.rs

Lines changed: 150 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@ use std::sync::Arc;
33
use agentkeys_core::backend::{BackendError, CredentialBackend};
44
use agentkeys_core::mock_client::MockHttpClient;
55
pub use agentkeys_core::session_store;
6-
use agentkeys_types::{AuditEvent, AuditFilter, AuthToken, ServiceName, Session, WalletAddress};
6+
use agentkeys_types::{
7+
AuditEvent, AuditFilter, AuthToken, Scope, ServiceName, Session, WalletAddress,
8+
};
79
use anyhow::{anyhow, Context, Result};
810
use serde_json::json;
911

@@ -68,7 +70,7 @@ impl CommandContext {
6870
self
6971
}
7072

71-
fn load_session(&self) -> Result<Session> {
73+
pub fn load_session(&self) -> Result<Session> {
7274
if let Some(ref s) = self.session_override {
7375
return Ok(s.clone());
7476
}
@@ -629,6 +631,152 @@ pub async fn cmd_approve(ctx: &CommandContext, pair_code: &str, auto_yes: bool)
629631
Ok("Approved. Agent paired successfully.".to_string())
630632
}
631633

634+
async fn resolve_agent_to_wallet(
635+
ctx: &CommandContext,
636+
session: &Session,
637+
agent: &str,
638+
) -> Result<String> {
639+
if agent.starts_with("0x") {
640+
return Ok(agent.to_string());
641+
}
642+
// Resolve alias or email via /identity/resolve
643+
let (identity_type, identity_value) = if agent.contains('@') {
644+
("email", agent)
645+
} else {
646+
("alias", agent)
647+
};
648+
// reqwest's .query() builder percent-encodes per RFC 3986 so identities
649+
// containing '+', '&', '=', '%', spaces (e.g. plus-addressed emails like
650+
// "bot+prod@example.com") are sent intact to the server.
651+
let http_client = reqwest::Client::new();
652+
let resp = http_client
653+
.get(format!("{}/identity/resolve", ctx.backend_url))
654+
.query(&[("identity_type", identity_type), ("identity_value", identity_value)])
655+
.header("authorization", format!("Bearer {}", session.token))
656+
.send()
657+
.await
658+
.context("GET /identity/resolve")?;
659+
if !resp.status().is_success() {
660+
let status = resp.status();
661+
let body: serde_json::Value = resp.json().await.unwrap_or(serde_json::Value::Null);
662+
let msg = body["message"].as_str().unwrap_or("not found");
663+
return Err(anyhow!("Error: HTTP {}: {}", status, msg));
664+
}
665+
let body: serde_json::Value = resp.json().await.context("parse identity/resolve response")?;
666+
let wallet = body["wallet_address"]
667+
.as_str()
668+
.ok_or_else(|| anyhow!("identity/resolve returned no wallet_address"))?
669+
.to_string();
670+
Ok(wallet)
671+
}
672+
673+
pub async fn cmd_scope(
674+
ctx: &CommandContext,
675+
agent: &str,
676+
add: &[String],
677+
remove: &[String],
678+
set: Option<&str>,
679+
list: bool,
680+
) -> Result<String> {
681+
if set.is_some() && (!add.is_empty() || !remove.is_empty()) {
682+
return Err(anyhow!(
683+
"Error: --set is mutually exclusive with --add and --remove. Use one or the other."
684+
));
685+
}
686+
687+
// --list is read-only. Combining it with mutating flags would silently
688+
// drop the mutation (the --list early-return happens before the update
689+
// path), so reject the combo up front with a clear error.
690+
if list && (set.is_some() || !add.is_empty() || !remove.is_empty()) {
691+
return Err(anyhow!(
692+
"Error: --list is mutually exclusive with --add, --remove, and --set. Use --list alone to read the current scope."
693+
));
694+
}
695+
696+
// `--add foo --remove foo` would silently no-op after mutation
697+
// (retain after push cancels) yet still issue a backend write with a
698+
// misleading "Scope updated" message. Reject up front (codex PR #29
699+
// v2 P2).
700+
if !add.is_empty() && !remove.is_empty() {
701+
let add_set: std::collections::HashSet<&str> = add.iter().map(|s| s.as_str()).collect();
702+
let overlap: Vec<&str> = remove
703+
.iter()
704+
.map(|s| s.as_str())
705+
.filter(|s| add_set.contains(s))
706+
.collect();
707+
if !overlap.is_empty() {
708+
return Err(anyhow!(
709+
"Error: the following services appear in both --add and --remove: {}. Pass each service to only one flag.",
710+
overlap.join(", ")
711+
));
712+
}
713+
}
714+
715+
if !list && set.is_none() && add.is_empty() && remove.is_empty() {
716+
return Err(anyhow!(
717+
"No action specified. Use --add, --remove, --set, or --list.\nRun `agentkeys scope --help` for usage."
718+
));
719+
}
720+
721+
let session = ctx.load_session().context("load session (run `agentkeys init` first)")?;
722+
let target_wallet = WalletAddress(resolve_agent_to_wallet(ctx, &session, agent).await?);
723+
let backend = ctx.backend();
724+
725+
let current_scope = backend
726+
.get_scope(&session, &target_wallet)
727+
.await
728+
.map_err(wrap_backend_error)?
729+
.unwrap_or(Scope { services: vec![], read_only: false });
730+
731+
if list {
732+
let service_names: Vec<&str> =
733+
current_scope.services.iter().map(|s| s.0.as_str()).collect();
734+
return Ok(format!(
735+
"Scope for agent {}:\n services: [{}]\n read_only: {}",
736+
target_wallet.0,
737+
service_names.join(", "),
738+
current_scope.read_only
739+
));
740+
}
741+
742+
let mut new_scope = if let Some(set_val) = set {
743+
let mut services: Vec<ServiceName> = set_val
744+
.split(',')
745+
.map(|s| s.trim())
746+
.filter(|s| !s.is_empty())
747+
.map(|s| ServiceName(s.to_string()))
748+
.collect();
749+
services.sort_by(|a, b| a.0.cmp(&b.0));
750+
Scope { services, read_only: current_scope.read_only }
751+
} else {
752+
let mut services: Vec<ServiceName> = current_scope.services.clone();
753+
for svc in add {
754+
let name = ServiceName(svc.clone());
755+
if !services.contains(&name) {
756+
services.push(name);
757+
}
758+
}
759+
services.retain(|s| !remove.contains(&s.0));
760+
services.sort_by(|a, b| a.0.cmp(&b.0));
761+
Scope { services, read_only: current_scope.read_only }
762+
};
763+
764+
backend
765+
.update_scope(&session, &target_wallet, &new_scope)
766+
.await
767+
.map_err(wrap_backend_error)?;
768+
769+
// `new_scope.services` is already sorted — both the --set branch
770+
// (line 749) and the --add/--remove branch (line 760) sort before
771+
// the update_scope call.
772+
let service_names: Vec<&str> = new_scope.services.iter().map(|s| s.0.as_str()).collect();
773+
Ok(format!(
774+
"Scope updated for agent {}. New services: [{}]",
775+
target_wallet.0,
776+
service_names.join(", ")
777+
))
778+
}
779+
632780
pub fn cmd_feedback() -> String {
633781
let url = "https://github.com/agentkeys/agentkeys/discussions";
634782
let opened = std::process::Command::new("open").arg(url).status().is_ok()

crates/agentkeys-cli/src/main.rs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use agentkeys_cli::{
22
cmd_approve, cmd_feedback, cmd_init, cmd_link, cmd_read, cmd_recover, cmd_revoke, cmd_run,
3-
cmd_store, cmd_teardown, cmd_usage, CommandContext,
3+
cmd_scope, cmd_store, cmd_teardown, cmd_usage, CommandContext,
44
};
55

66

@@ -139,6 +139,23 @@ enum Commands {
139139
yes: bool,
140140
},
141141

142+
#[command(
143+
about = "Edit or list the scope of a child agent",
144+
long_about = "Add, remove, replace, or list the services in a child agent's scope.\n\nExamples:\n agentkeys scope 0xAGENT --add openrouter\n agentkeys scope 0xAGENT --remove anthropic\n agentkeys scope 0xAGENT --set openrouter,anthropic\n agentkeys scope 0xAGENT --list"
145+
)]
146+
Scope {
147+
#[arg(help = "Agent wallet address, alias, or email")]
148+
agent: String,
149+
#[arg(long, help = "Add a service to the scope (repeatable)")]
150+
add: Vec<String>,
151+
#[arg(long, help = "Remove a service from the scope (repeatable)")]
152+
remove: Vec<String>,
153+
#[arg(long, help = "Replace the entire scope with a comma-separated list of services")]
154+
set: Option<String>,
155+
#[arg(long, help = "List the current scope without making changes")]
156+
list: bool,
157+
},
158+
142159
#[command(
143160
about = "Open the feedback forum in your browser",
144161
long_about = "Open https://github.com/agentkeys/agentkeys/discussions in the default browser.\n\nExamples:\n agentkeys feedback"
@@ -168,6 +185,9 @@ async fn main() {
168185
}
169186
Commands::Recover { identity, method } => cmd_recover(&ctx, identity, method).await,
170187
Commands::Approve { pair_code, yes } => cmd_approve(&ctx, pair_code, *yes).await,
188+
Commands::Scope { agent, add, remove, set, list } => {
189+
cmd_scope(&ctx, agent, add, remove, set.as_deref(), *list).await
190+
}
171191
Commands::Feedback => Ok(cmd_feedback()),
172192
};
173193

0 commit comments

Comments
 (0)