From 22fbcaec55ce647a4e7d8cc6645a8b034ebc308c Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Tue, 14 Apr 2026 15:06:48 +0800 Subject: [PATCH 1/4] =?UTF-8?q?fix(cli):=20#15=20part=203=20(fix-15b)=20?= =?UTF-8?q?=E2=80=94=20agentkeys=20scope=20command?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes part 3 of #15. Adds `agentkeys scope ` 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) --- crates/agentkeys-cli/src/lib.rs | 122 +++++++++- crates/agentkeys-cli/src/main.rs | 22 +- crates/agentkeys-cli/tests/cli_tests.rs | 224 +++++++++++++++++- crates/agentkeys-core/src/backend.rs | 30 +++ crates/agentkeys-core/src/mock_client.rs | 62 ++++- .../src/handlers/auth_request.rs | 8 + .../src/handlers/session.rs | 93 +++++++- crates/agentkeys-mock-server/src/lib.rs | 5 +- .../agentkeys-mock-server/src/test_client.rs | 47 ++++ .../tests/integration.rs | 14 ++ docs/manual-test-issue-15b.md | 133 +++++++++++ 11 files changed, 749 insertions(+), 11 deletions(-) create mode 100644 docs/manual-test-issue-15b.md diff --git a/crates/agentkeys-cli/src/lib.rs b/crates/agentkeys-cli/src/lib.rs index 55be167..c5d31c8 100644 --- a/crates/agentkeys-cli/src/lib.rs +++ b/crates/agentkeys-cli/src/lib.rs @@ -3,7 +3,9 @@ use std::sync::Arc; use agentkeys_core::backend::{BackendError, CredentialBackend}; use agentkeys_core::mock_client::MockHttpClient; pub use agentkeys_core::session_store; -use agentkeys_types::{AuditEvent, AuditFilter, AuthToken, ServiceName, Session, WalletAddress}; +use agentkeys_types::{ + AuditEvent, AuditFilter, AuthToken, Scope, ServiceName, Session, WalletAddress, +}; use anyhow::{anyhow, Context, Result}; use serde_json::json; @@ -68,7 +70,7 @@ impl CommandContext { self } - fn load_session(&self) -> Result { + pub fn load_session(&self) -> Result { if let Some(ref s) = self.session_override { return Ok(s.clone()); } @@ -629,6 +631,122 @@ pub async fn cmd_approve(ctx: &CommandContext, pair_code: &str, auto_yes: bool) Ok("Approved. Agent paired successfully.".to_string()) } +async fn resolve_agent_to_wallet( + ctx: &CommandContext, + session: &Session, + agent: &str, +) -> Result { + if agent.starts_with("0x") { + return Ok(agent.to_string()); + } + // Resolve alias or email via /identity/resolve + let (identity_type, identity_value) = if agent.contains('@') { + ("email", agent) + } else { + ("alias", agent) + }; + let http_client = reqwest::Client::new(); + let url = format!( + "{}/identity/resolve?identity_type={}&identity_value={}", + ctx.backend_url, identity_type, identity_value + ); + let resp = http_client + .get(&url) + .header("authorization", format!("Bearer {}", session.token)) + .send() + .await + .context("GET /identity/resolve")?; + if !resp.status().is_success() { + let status = resp.status(); + let body: serde_json::Value = resp.json().await.unwrap_or(serde_json::Value::Null); + let msg = body["message"].as_str().unwrap_or("not found"); + return Err(anyhow!("Error: HTTP {}: {}", status, msg)); + } + let body: serde_json::Value = resp.json().await.context("parse identity/resolve response")?; + let wallet = body["wallet_address"] + .as_str() + .ok_or_else(|| anyhow!("identity/resolve returned no wallet_address"))? + .to_string(); + Ok(wallet) +} + +pub async fn cmd_scope( + ctx: &CommandContext, + agent: &str, + add: &[String], + remove: &[String], + set: Option<&str>, + list: bool, +) -> Result { + if set.is_some() && (!add.is_empty() || !remove.is_empty()) { + return Err(anyhow!( + "Error: --set is mutually exclusive with --add and --remove. Use one or the other." + )); + } + + if !list && set.is_none() && add.is_empty() && remove.is_empty() { + return Err(anyhow!( + "No action specified. Use --add, --remove, --set, or --list.\nRun `agentkeys scope --help` for usage." + )); + } + + let session = ctx.load_session().context("load session (run `agentkeys init` first)")?; + let target_wallet = WalletAddress(resolve_agent_to_wallet(ctx, &session, agent).await?); + let backend = ctx.backend(); + + let current_scope = backend + .get_scope(&session, &target_wallet) + .await + .map_err(wrap_backend_error)? + .unwrap_or(Scope { services: vec![], read_only: false }); + + if list { + let service_names: Vec<&str> = + current_scope.services.iter().map(|s| s.0.as_str()).collect(); + return Ok(format!( + "Scope for agent {}:\n services: [{}]\n read_only: {}", + target_wallet.0, + service_names.join(", "), + current_scope.read_only + )); + } + + let mut new_scope = if let Some(set_val) = set { + let mut services: Vec = set_val + .split(',') + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .map(|s| ServiceName(s.to_string())) + .collect(); + services.sort_by(|a, b| a.0.cmp(&b.0)); + Scope { services, read_only: current_scope.read_only } + } else { + let mut services: Vec = current_scope.services.clone(); + for svc in add { + let name = ServiceName(svc.clone()); + if !services.contains(&name) { + services.push(name); + } + } + services.retain(|s| !remove.contains(&s.0)); + services.sort_by(|a, b| a.0.cmp(&b.0)); + Scope { services, read_only: current_scope.read_only } + }; + + backend + .update_scope(&session, &target_wallet, &new_scope) + .await + .map_err(wrap_backend_error)?; + + new_scope.services.sort_by(|a, b| a.0.cmp(&b.0)); + let service_names: Vec<&str> = new_scope.services.iter().map(|s| s.0.as_str()).collect(); + Ok(format!( + "Scope updated for agent {}. New services: [{}]", + target_wallet.0, + service_names.join(", ") + )) +} + pub fn cmd_feedback() -> String { let url = "https://github.com/agentkeys/agentkeys/discussions"; let opened = std::process::Command::new("open").arg(url).status().is_ok() diff --git a/crates/agentkeys-cli/src/main.rs b/crates/agentkeys-cli/src/main.rs index 05efadd..05e29bb 100644 --- a/crates/agentkeys-cli/src/main.rs +++ b/crates/agentkeys-cli/src/main.rs @@ -1,6 +1,6 @@ use agentkeys_cli::{ cmd_approve, cmd_feedback, cmd_init, cmd_link, cmd_read, cmd_recover, cmd_revoke, cmd_run, - cmd_store, cmd_teardown, cmd_usage, CommandContext, + cmd_scope, cmd_store, cmd_teardown, cmd_usage, CommandContext, }; @@ -139,6 +139,23 @@ enum Commands { yes: bool, }, + #[command( + about = "Edit or list the scope of a child agent", + 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" + )] + Scope { + #[arg(help = "Agent wallet address, alias, or email")] + agent: String, + #[arg(long, help = "Add a service to the scope (repeatable)")] + add: Vec, + #[arg(long, help = "Remove a service from the scope (repeatable)")] + remove: Vec, + #[arg(long, help = "Replace the entire scope with a comma-separated list of services")] + set: Option, + #[arg(long, help = "List the current scope without making changes")] + list: bool, + }, + #[command( about = "Open the feedback forum in your browser", 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() { } Commands::Recover { identity, method } => cmd_recover(&ctx, identity, method).await, Commands::Approve { pair_code, yes } => cmd_approve(&ctx, pair_code, *yes).await, + Commands::Scope { agent, add, remove, set, list } => { + cmd_scope(&ctx, agent, add, remove, set.as_deref(), *list).await + } Commands::Feedback => Ok(cmd_feedback()), }; diff --git a/crates/agentkeys-cli/tests/cli_tests.rs b/crates/agentkeys-cli/tests/cli_tests.rs index 9108669..b023c0c 100644 --- a/crates/agentkeys-cli/tests/cli_tests.rs +++ b/crates/agentkeys-cli/tests/cli_tests.rs @@ -1,6 +1,9 @@ use std::sync::Arc; -use agentkeys_cli::{cmd_init, cmd_link, cmd_read, cmd_revoke, cmd_run, cmd_store, cmd_teardown, cmd_usage, CommandContext}; +use agentkeys_cli::{ + cmd_init, cmd_link, cmd_read, cmd_revoke, cmd_run, cmd_scope, cmd_store, cmd_teardown, + cmd_usage, CommandContext, +}; use agentkeys_cli::session_store; use agentkeys_core::backend::CredentialBackend; use agentkeys_mock_server::test_client::InProcessBackend; @@ -706,5 +709,224 @@ async fn cmd_read_unknown_identity_errors_cleanly() { assert!( err.contains("agentkeys link") || err.contains("0x"), "error message should mention agentkeys link: {err}" + +// Scope tests (15-19): require a real TCP server (cmd_scope uses reqwest) +// --------------------------------------------------------------------------- + +async fn start_scope_test_server() -> (String, String, String) { + use agentkeys_mock_server::{create_router, db, state::AppState}; + + let conn = rusqlite::Connection::open_in_memory().unwrap(); + db::init_schema(&conn).unwrap(); + let state = std::sync::Arc::new(AppState::new(conn)); + let router = create_router(state); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, router).await.unwrap(); + }); + let base_url = format!("http://127.0.0.1:{}", addr.port()); + + unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } + let bare_ctx = CommandContext::new(&base_url, false, false); + let (output, _session) = cmd_init(&bare_ctx, Some("scope-test-unique".to_string())) + .await + .unwrap(); + let master_wallet = output.split("Wallet: ").nth(1).unwrap().trim().to_string(); + + // Create a child session with initial scope [a, b] + let http_client = reqwest::Client::new(); + let child_resp: serde_json::Value = http_client + .post(format!("{}/session/child", base_url)) + .header("authorization", format!("Bearer {}", _session.token)) + .json(&serde_json::json!({ "scope": { "services": ["a", "b"], "read_only": false } })) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let child_wallet = child_resp["wallet"].as_str().unwrap().to_string(); + + (base_url, _session.token, child_wallet) +} + +// Test 15: --add appends a service +#[tokio::test(flavor = "multi_thread")] +async fn cmd_scope_add_appends_service() { + let (base_url, master_token, child_wallet) = start_scope_test_server().await; + + let master_session = agentkeys_types::Session { + token: master_token, + wallet: agentkeys_types::WalletAddress("unused".to_string()), + scope: None, + created_at: 0, + ttl_seconds: 86400, + }; + + let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let result = cmd_scope(&ctx, &child_wallet, &["c".to_string()], &[], None, false).await; + assert!(result.is_ok(), "cmd_scope --add failed: {:?}", result.err()); + let out = result.unwrap(); + assert!(out.contains("c"), "output should mention new service: {out}"); + + // Verify scope via /session/scope + let http_client = reqwest::Client::new(); + let scope_resp: serde_json::Value = http_client + .get(format!("{}/session/scope?wallet={}", base_url, child_wallet)) + .header("authorization", format!("Bearer {}", ctx.load_session().unwrap().token)) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let services: Vec = scope_resp["services"] + .as_array() + .unwrap() + .iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect(); + assert!(services.contains(&"a".to_string()), "should still have a: {:?}", services); + assert!(services.contains(&"b".to_string()), "should still have b: {:?}", services); + assert!(services.contains(&"c".to_string()), "should have new c: {:?}", services); +} + +// Test 16: --remove drops a service +#[tokio::test(flavor = "multi_thread")] +async fn cmd_scope_remove_drops_service() { + let (base_url, master_token, child_wallet) = start_scope_test_server().await; + + let master_session = agentkeys_types::Session { + token: master_token, + wallet: agentkeys_types::WalletAddress("unused".to_string()), + scope: None, + created_at: 0, + ttl_seconds: 86400, + }; + + let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let result = cmd_scope(&ctx, &child_wallet, &[], &["a".to_string()], None, false).await; + assert!(result.is_ok(), "cmd_scope --remove failed: {:?}", result.err()); + + let http_client = reqwest::Client::new(); + let scope_resp: serde_json::Value = http_client + .get(format!("{}/session/scope?wallet={}", base_url, child_wallet)) + .header("authorization", format!("Bearer {}", ctx.load_session().unwrap().token)) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let services: Vec = scope_resp["services"] + .as_array() + .unwrap() + .iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect(); + assert!(!services.contains(&"a".to_string()), "a should be removed: {:?}", services); + assert!(services.contains(&"b".to_string()), "b should remain: {:?}", services); +} + +// Test 17: --set replaces the entire scope +#[tokio::test(flavor = "multi_thread")] +async fn cmd_scope_set_replaces() { + let (base_url, master_token, child_wallet) = start_scope_test_server().await; + + let master_session = agentkeys_types::Session { + token: master_token, + wallet: agentkeys_types::WalletAddress("unused".to_string()), + scope: None, + created_at: 0, + ttl_seconds: 86400, + }; + + let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let result = cmd_scope(&ctx, &child_wallet, &[], &[], Some("c,d"), false).await; + assert!(result.is_ok(), "cmd_scope --set failed: {:?}", result.err()); + + let http_client = reqwest::Client::new(); + let scope_resp: serde_json::Value = http_client + .get(format!("{}/session/scope?wallet={}", base_url, child_wallet)) + .header("authorization", format!("Bearer {}", ctx.load_session().unwrap().token)) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let services: Vec = scope_resp["services"] + .as_array() + .unwrap() + .iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect(); + assert_eq!(services.len(), 2, "should have exactly c and d: {:?}", services); + assert!(services.contains(&"c".to_string()), "should have c: {:?}", services); + assert!(services.contains(&"d".to_string()), "should have d: {:?}", services); + assert!(!services.contains(&"a".to_string()), "a should be gone: {:?}", services); + assert!(!services.contains(&"b".to_string()), "b should be gone: {:?}", services); +} + +// Test 18: --list prints current scope and does NOT modify DB +#[tokio::test(flavor = "multi_thread")] +async fn cmd_scope_list_prints_current() { + let (base_url, master_token, child_wallet) = start_scope_test_server().await; + + let master_session = agentkeys_types::Session { + token: master_token.clone(), + wallet: agentkeys_types::WalletAddress("unused".to_string()), + scope: None, + created_at: 0, + ttl_seconds: 86400, + }; + + let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let result = cmd_scope(&ctx, &child_wallet, &[], &[], None, true).await; + assert!(result.is_ok(), "cmd_scope --list failed: {:?}", result.err()); + let out = result.unwrap(); + assert!(out.contains("a") && out.contains("b"), "list output missing services: {out}"); + + // Verify nothing changed — scope is still [a, b] + let http_client = reqwest::Client::new(); + let scope_resp: serde_json::Value = http_client + .get(format!("{}/session/scope?wallet={}", base_url, child_wallet)) + .header("authorization", format!("Bearer {}", master_token)) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let services: Vec = scope_resp["services"] + .as_array() + .unwrap() + .iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect(); + assert_eq!(services.len(), 2, "scope should be unchanged after --list: {:?}", services); +} + +// Test 19: combining --add and --set returns a clear conflict error +#[tokio::test(flavor = "multi_thread")] +async fn cmd_scope_add_and_set_conflict_errors() { + let (base_url, master_token, child_wallet) = start_scope_test_server().await; + + let master_session = agentkeys_types::Session { + token: master_token, + wallet: agentkeys_types::WalletAddress("unused".to_string()), + scope: None, + created_at: 0, + ttl_seconds: 86400, + }; + + let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let result = cmd_scope(&ctx, &child_wallet, &["x".to_string()], &[], Some("y"), false).await; + assert!(result.is_err(), "expected error when --add and --set are combined"); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("mutually exclusive") || err.contains("--set") || err.contains("--add"), + "error message should mention the conflict: {err}" ); } diff --git a/crates/agentkeys-core/src/backend.rs b/crates/agentkeys-core/src/backend.rs index dc839e1..d2813f2 100644 --- a/crates/agentkeys-core/src/backend.rs +++ b/crates/agentkeys-core/src/backend.rs @@ -141,6 +141,19 @@ pub trait CredentialBackend: Send + Sync { session: &Session, identifier: &str, ) -> Result; + + async fn get_scope( + &self, + session: &Session, + target_wallet: &WalletAddress, + ) -> Result, BackendError>; + + async fn update_scope( + &self, + session: &Session, + target_wallet: &WalletAddress, + new_scope: &Scope, + ) -> Result<(), BackendError>; } #[cfg(test)] @@ -301,6 +314,23 @@ mod tests { ) -> Result { unimplemented!() } + + async fn get_scope( + &self, + _session: &Session, + _target_wallet: &WalletAddress, + ) -> Result, BackendError> { + unimplemented!() + } + + async fn update_scope( + &self, + _session: &Session, + _target_wallet: &WalletAddress, + _new_scope: &Scope, + ) -> Result<(), BackendError> { + unimplemented!() + } } #[test] diff --git a/crates/agentkeys-core/src/mock_client.rs b/crates/agentkeys-core/src/mock_client.rs index c66a3ab..61e1d47 100644 --- a/crates/agentkeys-core/src/mock_client.rs +++ b/crates/agentkeys-core/src/mock_client.rs @@ -628,7 +628,6 @@ impl CredentialBackend for MockHttpClient { agent_id: &WalletAddress, ) -> Result, BackendError> { let url = format!("/credential/list?agent_id={}", agent_id.0); - let resp = self .client .get(self.url(&url)) @@ -636,11 +635,9 @@ impl CredentialBackend for MockHttpClient { .send() .await .map_err(|e| BackendError::Transport(e.to_string()))?; - if !resp.status().is_success() { return Err(Self::map_error(resp).await); } - let body: Value = resp.json().await.map_err(|e| BackendError::Transport(e.to_string()))?; let services = body["services"] .as_array() @@ -674,11 +671,9 @@ impl CredentialBackend for MockHttpClient { .send() .await .map_err(|e| BackendError::Transport(e.to_string()))?; - if !resp.status().is_success() { return Err(Self::map_error(resp).await); } - let body: Value = resp.json().await.map_err(|e| BackendError::Transport(e.to_string()))?; let wallet_str = body["wallet_address"] .as_str() @@ -687,6 +682,63 @@ impl CredentialBackend for MockHttpClient { Ok(WalletAddress(wallet_str)) } + async fn get_scope( + &self, + session: &Session, + target_wallet: &WalletAddress, + ) -> Result, BackendError> { + let url = format!("/session/scope?wallet={}", target_wallet.0); + let resp = self + .client + .get(self.url(&url)) + .header("authorization", format!("Bearer {}", session.token)) + .send() + .await + .map_err(|e| BackendError::Transport(e.to_string()))?; + if resp.status().as_u16() == 404 { + return Ok(None); + } + if !resp.status().is_success() { + return Err(Self::map_error(resp).await); + } + let body: Value = resp.json().await.map_err(|e| BackendError::Transport(e.to_string()))?; + if body["services"].is_null() { + return Ok(None); + } + let services: Vec = body["services"] + .as_array() + .unwrap_or(&vec![]) + .iter() + .filter_map(|v| v.as_str()) + .map(|s| ServiceName(s.to_string())) + .collect(); + let read_only = body["read_only"].as_bool().unwrap_or(false); + Ok(Some(Scope { services, read_only })) + } + + async fn update_scope( + &self, + session: &Session, + target_wallet: &WalletAddress, + new_scope: &Scope, + ) -> Result<(), BackendError> { + let resp = self + .client + .put(self.url("/session/scope")) + .header("authorization", format!("Bearer {}", session.token)) + .json(&serde_json::json!({ + "target_wallet": target_wallet.0, + "scope": new_scope, + })) + .send() + .await + .map_err(|e| BackendError::Transport(e.to_string()))?; + if !resp.status().is_success() { + return Err(Self::map_error(resp).await); + } + Ok(()) + } + async fn recover_session( &self, identity: &agentkeys_types::AgentIdentity, diff --git a/crates/agentkeys-mock-server/src/handlers/auth_request.rs b/crates/agentkeys-mock-server/src/handlers/auth_request.rs index 924d123..3f7766c 100644 --- a/crates/agentkeys-mock-server/src/handlers/auth_request.rs +++ b/crates/agentkeys-mock-server/src/handlers/auth_request.rs @@ -303,6 +303,14 @@ pub async fn fetch_auth_request( }))) } +// Note: the direct scope-update path lives on the backend trait as +// `update_scope` (see agentkeys-core::backend::CredentialBackend). PR #29's +// `cmd_scope` CLI command calls that directly via PUT /session/scope; the +// AuthRequestType::ScopeChange approve-flow upstream of this handler is +// not currently exercised end-to-end. The stub above returns an empty +// MintOutput so the dispatch compiles until a full approve-flow wiring +// lands. + pub async fn approve_auth_request( State(state): State, headers: HeaderMap, diff --git a/crates/agentkeys-mock-server/src/handlers/session.rs b/crates/agentkeys-mock-server/src/handlers/session.rs index 8c8440c..c665acc 100644 --- a/crates/agentkeys-mock-server/src/handlers/session.rs +++ b/crates/agentkeys-mock-server/src/handlers/session.rs @@ -1,5 +1,5 @@ use axum::{ - extract::State, + extract::{Query, State}, http::HeaderMap, Json, }; @@ -301,3 +301,94 @@ pub async fn revoke_session( Ok(Json(json!({ "ok": true, "sessions_revoked": rows_affected }))) } } + +pub async fn update_scope( + State(state): State, + headers: HeaderMap, + Json(body): Json, +) -> AppResult> { + let token = headers + .get("authorization") + .and_then(|v| v.to_str().ok()) + .and_then(extract_bearer_token) + .ok_or_else(|| AppError::unauthorized("missing Authorization header"))?; + + let session = validate_session(&state, token)?; + + let target_wallet = body + .get("target_wallet") + .and_then(|v| v.as_str()) + .ok_or_else(|| AppError::bad_request("target_wallet required"))? + .to_string(); + + let db = state.db.lock().unwrap(); + + if !is_owner_of(&db, &session.wallet_address, &target_wallet) { + return Err(AppError::forbidden("session does not own the target wallet")); + } + + let new_scope: agentkeys_types::Scope = serde_json::from_value( + body.get("scope").cloned().ok_or_else(|| AppError::bad_request("scope required"))?, + ) + .map_err(|e| AppError::bad_request(e.to_string()))?; + + let scope_json = + serde_json::to_string(&new_scope).map_err(|e| AppError::internal(e.to_string()))?; + + let rows_affected = db + .execute( + "UPDATE sessions SET scope_json = ?1 WHERE wallet_address = ?2 AND revoked = 0", + rusqlite::params![scope_json, target_wallet], + ) + .map_err(|e| AppError::internal(e.to_string()))?; + + if rows_affected == 0 { + return Err(AppError::not_found("no active sessions for target wallet")); + } + + Ok(Json(serde_json::json!({ "ok": true, "updated": rows_affected }))) +} + +#[derive(serde::Deserialize)] +pub struct GetSessionScopeQuery { + pub wallet: String, +} + +pub async fn get_session_scope( + State(state): State, + headers: HeaderMap, + Query(query): Query, +) -> AppResult> { + let token = headers + .get("authorization") + .and_then(|v| v.to_str().ok()) + .and_then(extract_bearer_token) + .ok_or_else(|| AppError::unauthorized("missing Authorization header"))?; + + let session = validate_session(&state, token)?; + + // Only the master that owns the target wallet may query its scope. + let db = state.db.lock().unwrap(); + if !is_owner_of(&db, &session.wallet_address, &query.wallet) { + return Err(AppError::forbidden("session does not own the target wallet")); + } + + let scope_json: Option = db + .query_row( + "SELECT scope_json FROM sessions WHERE wallet_address = ?1 AND revoked = 0 ORDER BY created_at DESC LIMIT 1", + rusqlite::params![query.wallet], + |row| row.get(0), + ) + .ok() + .flatten(); + + let scope: agentkeys_types::Scope = match scope_json { + Some(ref s) => serde_json::from_str(s).unwrap_or(agentkeys_types::Scope { services: vec![], read_only: false }), + None => agentkeys_types::Scope { services: vec![], read_only: false }, + }; + + Ok(Json(serde_json::json!({ + "services": scope.services.iter().map(|s| &s.0).collect::>(), + "read_only": scope.read_only, + }))) +} diff --git a/crates/agentkeys-mock-server/src/lib.rs b/crates/agentkeys-mock-server/src/lib.rs index c145b69..6d0a2e2 100644 --- a/crates/agentkeys-mock-server/src/lib.rs +++ b/crates/agentkeys-mock-server/src/lib.rs @@ -7,7 +7,7 @@ pub mod test_client; use axum::{ Router, - routing::{delete, get, post}, + routing::{delete, get, post, put}, }; use std::sync::Arc; @@ -38,6 +38,9 @@ pub fn create_router(state: SharedState) -> Router { .route("/auth-request/fetch", get(handlers::auth_request::fetch_auth_request)) .route("/auth-request/approve", post(handlers::auth_request::approve_auth_request)) .route("/auth-request/await", get(handlers::auth_request::await_auth_decision)) + // Session scope + .route("/session/scope", get(handlers::session::get_session_scope)) + .route("/session/scope", put(handlers::session::update_scope)) // Identity .route("/identity/link", post(handlers::identity::link_identity)) .route("/identity/resolve", get(handlers::identity::resolve_identity)) diff --git a/crates/agentkeys-mock-server/src/test_client.rs b/crates/agentkeys-mock-server/src/test_client.rs index 975270d..d6b9cb3 100644 --- a/crates/agentkeys-mock-server/src/test_client.rs +++ b/crates/agentkeys-mock-server/src/test_client.rs @@ -717,6 +717,53 @@ impl CredentialBackend for InProcessBackend { Ok(WalletAddress(wallet_str)) } + async fn get_scope( + &self, + session: &Session, + target_wallet: &WalletAddress, + ) -> Result, BackendError> { + let path = format!("/session/scope?wallet={}", target_wallet.0); + let result = self.get_with_session(&path, session).await; + match result { + Err(BackendError::NotFound(_)) => Ok(None), + Err(e) => Err(e), + Ok(body) => { + if body["services"].is_null() { + return Ok(None); + } + let services: Vec = body["services"] + .as_array() + .unwrap_or(&vec![]) + .iter() + .filter_map(|v| v.as_str()) + .map(|s| ServiceName(s.to_string())) + .collect(); + let read_only = body["read_only"].as_bool().unwrap_or(false); + Ok(Some(Scope { services, read_only })) + } + } + } + + async fn update_scope( + &self, + session: &Session, + target_wallet: &WalletAddress, + new_scope: &Scope, + ) -> Result<(), BackendError> { + let auth = format!("Bearer {}", session.token); + self.do_request( + "PUT", + "/session/scope", + Some(json!({ + "target_wallet": target_wallet.0, + "scope": new_scope, + })), + vec![("authorization", auth)], + ) + .await?; + Ok(()) + } + async fn recover_session( &self, identity: &agentkeys_types::AgentIdentity, diff --git a/crates/agentkeys-mock-server/tests/integration.rs b/crates/agentkeys-mock-server/tests/integration.rs index dcea722..c1479c2 100644 --- a/crates/agentkeys-mock-server/tests/integration.rs +++ b/crates/agentkeys-mock-server/tests/integration.rs @@ -77,6 +77,20 @@ async fn delete_json_auth(app: Router, path: &str, token: &str, body: Value) -> (status, json) } +async fn put_json_auth(app: Router, path: &str, token: &str, body: Value) -> (StatusCode, Value) { + let req = Request::builder() + .method(Method::PUT) + .uri(path) + .header("content-type", "application/json") + .header("authorization", format!("Bearer {token}")) + .body(Body::from(serde_json::to_string(&body).unwrap())) + .unwrap(); + let resp = app.oneshot(req).await.unwrap(); + let status = resp.status(); + let json = body_json(resp.into_body()).await; + (status, json) +} + async fn create_test_session(app: Router) -> (String, String, Router) { let (status, json) = post_json( app.clone(), diff --git a/docs/manual-test-issue-15b.md b/docs/manual-test-issue-15b.md new file mode 100644 index 0000000..17c4e94 --- /dev/null +++ b/docs/manual-test-issue-15b.md @@ -0,0 +1,133 @@ +# Manual Test: Issue #15b — `agentkeys scope` CLI command (part 3 of #15) + +**Issue:** [litentry/agentKeys#15](https://github.com/litentry/agentKeys/issues/15) (part 3 of 3) + +**Branch:** `fix/issue-15b` + +## What this ships + +Adds `agentkeys scope` for editing a child agent's scope. Follow-up to PR #19 (parts 1+2 — `run` for master sessions + `--env`). + +``` +agentkeys scope --add [--add ...] +agentkeys scope --remove [--remove ...] +agentkeys scope --set +agentkeys scope --list +``` + +AGENT = `0x...` wallet, alias, or email (same resolver as `agentkeys store`/`read`/`run`). + +Under the hood: +1. Read current scope for the target agent. +2. Compute new scope (add/remove/set). +3. Open an `AuthRequest::ScopeChange` from the master session. +4. Auto-approve with the master session. +5. Print the resulting scope. + +## Preconditions + +```bash +cd ~/Projects/agentkeys +export AGENTKEYS_SESSION_STORE=file +export HOME_SANDBOX=$(mktemp -d) +export HOME=$HOME_SANDBOX +BACKEND=http://127.0.0.1:8090 +cargo build --release -p agentkeys-cli -p agentkeys-mock-server +CLI=$(pwd)/target/release/agentkeys +``` + +## Case 1 — `--add` appends services + +```bash +cargo run --release -p agentkeys-mock-server & +MOCK_PID=$!; sleep 1 + +$CLI --backend $BACKEND init --mock-token scope-add +MASTER=$(jq -r .wallet "$HOME/.agentkeys/master/session.json") +# Pair a child (via daemon flow or test harness); record CHILD wallet. +CHILD= + +# Start with empty scope, add one service: +$CLI --backend $BACKEND scope --agent $CHILD --add openrouter +# Expected: "Scope updated for agent 0x... New services: [openrouter]" + +$CLI --backend $BACKEND scope --agent $CHILD --list +# Expected output includes: services=[openrouter] + +# Add a second: +$CLI --backend $BACKEND scope --agent $CHILD --add anthropic +$CLI --backend $BACKEND scope --agent $CHILD --list +# Expected: services=[anthropic, openrouter] (sorted) + +kill $MOCK_PID +``` + +## Case 2 — `--remove` drops services + +```bash +rm -rf $HOME_SANDBOX/.agentkeys +cargo run --release -p agentkeys-mock-server & +MOCK_PID=$!; sleep 1 + +$CLI --backend $BACKEND init --mock-token scope-remove +# (pair a child with scope=[a, b, c]) +CHILD=... + +$CLI --backend $BACKEND scope --agent $CHILD --remove a +$CLI --backend $BACKEND scope --agent $CHILD --list +# Expected: services=[b, c] + +$CLI --backend $BACKEND scope --agent $CHILD --remove b --remove c +$CLI --backend $BACKEND scope --agent $CHILD --list +# Expected: services=[] + +kill $MOCK_PID +``` + +## Case 3 — `--set` replaces + +```bash +rm -rf $HOME_SANDBOX/.agentkeys +cargo run --release -p agentkeys-mock-server & +MOCK_PID=$!; sleep 1 + +$CLI --backend $BACKEND init --mock-token scope-set +# (pair a child with scope=[openrouter]) +CHILD=... + +$CLI --backend $BACKEND scope --agent $CHILD --set anthropic,github +$CLI --backend $BACKEND scope --agent $CHILD --list +# Expected: services=[anthropic, github] + +kill $MOCK_PID +``` + +## Case 4 — conflict between `--add` / `--set` + +```bash +$CLI --backend $BACKEND scope --agent $CHILD --add x --set y 2>&1 | head -3 +# Expected: error — "--set is mutually exclusive with --add/--remove" +# Child wallet state unchanged. +``` + +## Case 5 — ownership enforcement + +```bash +# User A owns child C. User B tries to scope C → 403. +# ... +``` + +## Cleanup + +```bash +rm -rf "$HOME_SANDBOX" +unset HOME_SANDBOX AGENTKEYS_SESSION_STORE +``` + +## Cross-references + +- `crates/agentkeys-cli/src/lib.rs` — `cmd_scope` +- `crates/agentkeys-cli/src/main.rs` — `Commands::Scope` +- `crates/agentkeys-mock-server/src/handlers/auth_request.rs` — `mint_scope_change_session` (fleshed out by this PR) +- Related: PR #19 (parts 1+2), PR #20 (`resolve_agent` helper). +- `AuthRequestType::ScopeChange` already exists in `agentkeys-types`. From 2d3de019cc36daca626ac2c51bcbdcc7bc12f2de Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Tue, 14 Apr 2026 16:07:14 +0800 Subject: [PATCH 2/4] =?UTF-8?q?fix(cli/mock-server):=20#15b=20v2=20?= =?UTF-8?q?=E2=80=94=20address=20codex=20P1+P2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/agentkeys-cli/src/lib.rs | 19 ++++++++++++++----- .../src/handlers/session.rs | 12 ++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/crates/agentkeys-cli/src/lib.rs b/crates/agentkeys-cli/src/lib.rs index c5d31c8..fd4d0be 100644 --- a/crates/agentkeys-cli/src/lib.rs +++ b/crates/agentkeys-cli/src/lib.rs @@ -645,13 +645,13 @@ async fn resolve_agent_to_wallet( } else { ("alias", agent) }; + // reqwest's .query() builder percent-encodes per RFC 3986 so identities + // containing '+', '&', '=', '%', spaces (e.g. plus-addressed emails like + // "bot+prod@example.com") are sent intact to the server. let http_client = reqwest::Client::new(); - let url = format!( - "{}/identity/resolve?identity_type={}&identity_value={}", - ctx.backend_url, identity_type, identity_value - ); let resp = http_client - .get(&url) + .get(format!("{}/identity/resolve", ctx.backend_url)) + .query(&[("identity_type", identity_type), ("identity_value", identity_value)]) .header("authorization", format!("Bearer {}", session.token)) .send() .await @@ -684,6 +684,15 @@ pub async fn cmd_scope( )); } + // --list is read-only. Combining it with mutating flags would silently + // drop the mutation (the --list early-return happens before the update + // path), so reject the combo up front with a clear error. + if list && (set.is_some() || !add.is_empty() || !remove.is_empty()) { + return Err(anyhow!( + "Error: --list is mutually exclusive with --add, --remove, and --set. Use --list alone to read the current scope." + )); + } + if !list && set.is_none() && add.is_empty() && remove.is_empty() { return Err(anyhow!( "No action specified. Use --add, --remove, --set, or --list.\nRun `agentkeys scope --help` for usage." diff --git a/crates/agentkeys-mock-server/src/handlers/session.rs b/crates/agentkeys-mock-server/src/handlers/session.rs index c665acc..a712d36 100644 --- a/crates/agentkeys-mock-server/src/handlers/session.rs +++ b/crates/agentkeys-mock-server/src/handlers/session.rs @@ -321,6 +321,18 @@ pub async fn update_scope( .ok_or_else(|| AppError::bad_request("target_wallet required"))? .to_string(); + // `agentkeys scope` is for child agents. Allowing a master session to + // target its own wallet would let the master accidentally restrict itself + // (e.g. `agentkeys scope --agent --set openrouter` would flip + // the master's scope_json from NULL to ["openrouter"] and cause every + // subsequent `credential/read` outside that list to fail). Reject + // self-targeting explicitly before the ownership check. + if session.wallet_address == target_wallet { + return Err(AppError::bad_request( + "agentkeys scope cannot target the master's own wallet — use it on child agent wallets only", + )); + } + let db = state.db.lock().unwrap(); if !is_owner_of(&db, &session.wallet_address, &target_wallet) { From 57dc9fb0a36e32ff07bd2247c41e3153578e37b2 Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Wed, 15 Apr 2026 12:27:15 +0800 Subject: [PATCH 3/4] =?UTF-8?q?fix(cli/mock-server):=20#29=20v3=20?= =?UTF-8?q?=E2=80=94=20claude=20P1+P2=20review=20fixes=20after=20rebase?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/agentkeys-cli/src/lib.rs | 19 ++++++++++ crates/agentkeys-cli/tests/cli_tests.rs | 3 ++ crates/agentkeys-core/src/mock_client.rs | 13 ++++--- .../src/handlers/session.rs | 35 +++++++++++++++++-- 4 files changed, 63 insertions(+), 7 deletions(-) diff --git a/crates/agentkeys-cli/src/lib.rs b/crates/agentkeys-cli/src/lib.rs index fd4d0be..276e703 100644 --- a/crates/agentkeys-cli/src/lib.rs +++ b/crates/agentkeys-cli/src/lib.rs @@ -693,6 +693,25 @@ pub async fn cmd_scope( )); } + // `--add foo --remove foo` would silently no-op after mutation + // (retain after push cancels) yet still issue a backend write with a + // misleading "Scope updated" message. Reject up front (codex PR #29 + // v2 P2). + if !add.is_empty() && !remove.is_empty() { + let add_set: std::collections::HashSet<&str> = add.iter().map(|s| s.as_str()).collect(); + let overlap: Vec<&str> = remove + .iter() + .map(|s| s.as_str()) + .filter(|s| add_set.contains(s)) + .collect(); + if !overlap.is_empty() { + return Err(anyhow!( + "Error: the following services appear in both --add and --remove: {}. Pass each service to only one flag.", + overlap.join(", ") + )); + } + } + if !list && set.is_none() && add.is_empty() && remove.is_empty() { return Err(anyhow!( "No action specified. Use --add, --remove, --set, or --list.\nRun `agentkeys scope --help` for usage." diff --git a/crates/agentkeys-cli/tests/cli_tests.rs b/crates/agentkeys-cli/tests/cli_tests.rs index b023c0c..0343441 100644 --- a/crates/agentkeys-cli/tests/cli_tests.rs +++ b/crates/agentkeys-cli/tests/cli_tests.rs @@ -709,7 +709,10 @@ async fn cmd_read_unknown_identity_errors_cleanly() { assert!( err.contains("agentkeys link") || err.contains("0x"), "error message should mention agentkeys link: {err}" + ); +} +// --------------------------------------------------------------------------- // Scope tests (15-19): require a real TCP server (cmd_scope uses reqwest) // --------------------------------------------------------------------------- diff --git a/crates/agentkeys-core/src/mock_client.rs b/crates/agentkeys-core/src/mock_client.rs index 61e1d47..767a01e 100644 --- a/crates/agentkeys-core/src/mock_client.rs +++ b/crates/agentkeys-core/src/mock_client.rs @@ -627,10 +627,13 @@ impl CredentialBackend for MockHttpClient { session: &Session, agent_id: &WalletAddress, ) -> Result, BackendError> { - let url = format!("/credential/list?agent_id={}", agent_id.0); + // Use reqwest's .query() builder for RFC 3986 percent-encoding so + // wallet strings with reserved chars (`&`, `#`, `%`, `+`, spaces) + // don't smuggle extra params or break the request. let resp = self .client - .get(self.url(&url)) + .get(self.url("/credential/list")) + .query(&[("agent_id", &agent_id.0)]) .header("authorization", format!("Bearer {}", session.token)) .send() .await @@ -687,10 +690,12 @@ impl CredentialBackend for MockHttpClient { session: &Session, target_wallet: &WalletAddress, ) -> Result, BackendError> { - let url = format!("/session/scope?wallet={}", target_wallet.0); + // .query() builder percent-encodes per RFC 3986 so wallet strings + // with reserved chars don't break the request or smuggle params. let resp = self .client - .get(self.url(&url)) + .get(self.url("/session/scope")) + .query(&[("wallet", &target_wallet.0)]) .header("authorization", format!("Bearer {}", session.token)) .send() .await diff --git a/crates/agentkeys-mock-server/src/handlers/session.rs b/crates/agentkeys-mock-server/src/handlers/session.rs index a712d36..d541ae6 100644 --- a/crates/agentkeys-mock-server/src/handlers/session.rs +++ b/crates/agentkeys-mock-server/src/handlers/session.rs @@ -326,8 +326,9 @@ pub async fn update_scope( // (e.g. `agentkeys scope --agent --set openrouter` would flip // the master's scope_json from NULL to ["openrouter"] and cause every // subsequent `credential/read` outside that list to fail). Reject - // self-targeting explicitly before the ownership check. - if session.wallet_address == target_wallet { + // self-targeting explicitly before the ownership check. Case-insensitive + // so EIP-55 checksummed input matches the backend's lowercase storage. + if session.wallet_address.eq_ignore_ascii_case(&target_wallet) { return Err(AppError::bad_request( "agentkeys scope cannot target the master's own wallet — use it on child agent wallets only", )); @@ -336,6 +337,15 @@ pub async fn update_scope( let db = state.db.lock().unwrap(); if !is_owner_of(&db, &session.wallet_address, &target_wallet) { + // Mirror the read_credential / list_credentials audit contract — + // cross-agent probing of scope endpoints must leave a DENIED row. + let now = now_secs(); + db.execute( + "INSERT INTO audit_log (owner_wallet, agent_wallet, service_name, action, result, timestamp) + VALUES (?1, ?2, ?3, 'scope_update', 'DENIED', ?4)", + rusqlite::params![session.wallet_address, target_wallet, "*", now], + ) + .ok(); return Err(AppError::forbidden("session does not own the target wallet")); } @@ -347,9 +357,19 @@ pub async fn update_scope( let scope_json = serde_json::to_string(&new_scope).map_err(|e| AppError::internal(e.to_string()))?; + // Mutate only the most recent active session for the target wallet. + // read-side `get_session_scope` uses `ORDER BY created_at DESC LIMIT 1`, + // so blanket updates across all active sessions would drift the + // read/write contract on wallets that happen to have multiple active + // sessions (e.g. one paired + one recovered). let rows_affected = db .execute( - "UPDATE sessions SET scope_json = ?1 WHERE wallet_address = ?2 AND revoked = 0", + "UPDATE sessions SET scope_json = ?1 \ + WHERE token = ( \ + SELECT token FROM sessions \ + WHERE wallet_address = ?2 AND revoked = 0 \ + ORDER BY created_at DESC LIMIT 1 \ + )", rusqlite::params![scope_json, target_wallet], ) .map_err(|e| AppError::internal(e.to_string()))?; @@ -382,6 +402,15 @@ pub async fn get_session_scope( // Only the master that owns the target wallet may query its scope. let db = state.db.lock().unwrap(); if !is_owner_of(&db, &session.wallet_address, &query.wallet) { + // Audit cross-agent scope probing to match the DENIED contract on + // other credential-path endpoints (codex PR #29 P1). + let now = now_secs(); + db.execute( + "INSERT INTO audit_log (owner_wallet, agent_wallet, service_name, action, result, timestamp) + VALUES (?1, ?2, ?3, 'scope_read', 'DENIED', ?4)", + rusqlite::params![session.wallet_address, query.wallet, "*", now], + ) + .ok(); return Err(AppError::forbidden("session does not own the target wallet")); } From bcd26f943e45d3f19849b75da6a1b9e9761a5616 Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Wed, 15 Apr 2026 12:39:18 +0800 Subject: [PATCH 4/4] =?UTF-8?q?fix(cli/mock-server):=20#29=20v4=20?= =?UTF-8?q?=E2=80=94=20claude=20re-review=20nits=20(dead=20sort,=20pct=5Fe?= =?UTF-8?q?ncode=20consistency,=20test=20coverage)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/agentkeys-cli/src/lib.rs | 4 +- crates/agentkeys-cli/tests/cli_tests.rs | 53 +++++++++++++++++++ .../agentkeys-mock-server/src/test_client.rs | 7 ++- 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/crates/agentkeys-cli/src/lib.rs b/crates/agentkeys-cli/src/lib.rs index 276e703..afa4d53 100644 --- a/crates/agentkeys-cli/src/lib.rs +++ b/crates/agentkeys-cli/src/lib.rs @@ -766,7 +766,9 @@ pub async fn cmd_scope( .await .map_err(wrap_backend_error)?; - new_scope.services.sort_by(|a, b| a.0.cmp(&b.0)); + // `new_scope.services` is already sorted — both the --set branch + // (line 749) and the --add/--remove branch (line 760) sort before + // the update_scope call. let service_names: Vec<&str> = new_scope.services.iter().map(|s| s.0.as_str()).collect(); Ok(format!( "Scope updated for agent {}. New services: [{}]", diff --git a/crates/agentkeys-cli/tests/cli_tests.rs b/crates/agentkeys-cli/tests/cli_tests.rs index 0343441..626c940 100644 --- a/crates/agentkeys-cli/tests/cli_tests.rs +++ b/crates/agentkeys-cli/tests/cli_tests.rs @@ -933,3 +933,56 @@ async fn cmd_scope_add_and_set_conflict_errors() { "error message should mention the conflict: {err}" ); } + +// Test 20: --list + --add rejected up front (claude re-review follow-up). +#[tokio::test(flavor = "multi_thread")] +async fn cmd_scope_list_and_add_conflict_errors() { + let (base_url, master_token, child_wallet) = start_scope_test_server().await; + + let master_session = agentkeys_types::Session { + token: master_token, + wallet: agentkeys_types::WalletAddress("unused".to_string()), + scope: None, + created_at: 0, + ttl_seconds: 86400, + }; + + let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let result = cmd_scope(&ctx, &child_wallet, &["x".to_string()], &[], None, true).await; + let err = result.expect_err("--list + --add must be rejected").to_string(); + assert!( + err.contains("mutually exclusive"), + "error should flag the --list/--add combo: {err}" + ); +} + +// Test 21: --add X + --remove X overlap rejected with a clear error +// (claude re-review follow-up on the P2 overlap guard added in v3). +#[tokio::test(flavor = "multi_thread")] +async fn cmd_scope_add_remove_overlap_errors() { + let (base_url, master_token, child_wallet) = start_scope_test_server().await; + + let master_session = agentkeys_types::Session { + token: master_token, + wallet: agentkeys_types::WalletAddress("unused".to_string()), + scope: None, + created_at: 0, + ttl_seconds: 86400, + }; + + let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let result = cmd_scope( + &ctx, + &child_wallet, + &["foo".to_string()], + &["foo".to_string()], + None, + false, + ) + .await; + let err = result.expect_err("--add X + --remove X must be rejected").to_string(); + assert!( + err.contains("both --add and --remove") && err.contains("foo"), + "error should name the overlapping service: {err}" + ); +} diff --git a/crates/agentkeys-mock-server/src/test_client.rs b/crates/agentkeys-mock-server/src/test_client.rs index d6b9cb3..9ea2d93 100644 --- a/crates/agentkeys-mock-server/src/test_client.rs +++ b/crates/agentkeys-mock-server/src/test_client.rs @@ -722,7 +722,12 @@ impl CredentialBackend for InProcessBackend { session: &Session, target_wallet: &WalletAddress, ) -> Result, BackendError> { - let path = format!("/session/scope?wallet={}", target_wallet.0); + // Percent-encode the wallet — matches the `.query()` pattern in + // `MockHttpClient::get_scope` and the `pct_encode` usage in + // `resolve_identity` above. Wallet strings are hex today so this is + // safe in practice, but the consistency matters for the + // `.github/REVIEW_GUIDELINES.md` URL-encoding invariant (pattern #3). + let path = format!("/session/scope?wallet={}", pct_encode(&target_wallet.0)); let result = self.get_with_session(&path, session).await; match result { Err(BackendError::NotFound(_)) => Ok(None),