From ecd3e4ceb96549743b146a6a16cd0b9e83a3d3ca Mon Sep 17 00:00:00 2001 From: bellman Date: Thu, 4 Jun 2026 12:01:58 +0900 Subject: [PATCH] fix: type allowed tools validation --- ROADMAP.md | 2 +- USAGE.md | 2 + rust/README.md | 2 +- rust/crates/rusty-claude-cli/src/main.rs | 189 +++++++++++++++- .../tests/output_format_contract.rs | 53 ++++- rust/crates/tools/src/lib.rs | 209 ++++++++++++++---- 6 files changed, 400 insertions(+), 57 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index ae9e0021..c21a25a1 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -6371,7 +6371,7 @@ Original filing (2026-04-18): the session emitted `SessionStart hook (completed) 431. **DONE — `skills uninstall ` resolves locally instead of requiring Anthropic credentials** — fixed 2026-06-03 in `fix: keep skills lifecycle local`. `claw skills uninstall nonexistent-skill-xyz --output-format json` now stays on the local skills lifecycle surface and emits `kind:"skills"`, `action:"uninstall"`, `error_kind:"skill_not_found"`, `skills_dir`, `available_names`, and a hint without provider credentials. `claw skills install` no-arg emits typed `missing_argument` with `argument:"install_source"`; `claw skills install ` emits typed `invalid_install_source` with `source`, `source_kind`, `reason`, and a recovery hint. Installed skill roundtrips remove the installed files through the shared local lifecycle helper. `claw agents create ` now scaffolds `.claw/agents/.toml` and lists through the existing TOML agent discovery surface. Regression coverage: `skills_lifecycle_errors_have_typed_local_json_795_431`, `skills_install_uninstall_roundtrip_stays_local_431`, `agents_create_scaffolds_toml_and_lists_locally_431`, local command routing tests, parser discriminant tests, and command help/docs assertions. -432. **`--allowedTools` validator inconsistency: tool name list is half snake_case (`bash`, `read_file`, `write_file`, `edit_file`, `glob_search`, `grep_search`) and half PascalCase (`WebFetch`, `WebSearch`, `TodoWrite`, `Skill`, `Agent`, `Sleep`) with three UPPERCASE entries (`REPL`, `LSP`, `MCP`); accepts undocumented CamelCase aliases (`Read`, `Write`, `Edit`) and silently translates them to snake_case; argument parsing consumes the next positional when value is missing** — dogfooded 2026-05-11 by Jobdori on `fad53e2d` in response to Clawhip pinpoint nudge at `1503283046856655029`. Reproduction: `claw --allowedTools status --output-format json` → `{"error":"unsupported tool in --allowedTools: status (expected one of: bash, read_file, write_file, edit_file, glob_search, grep_search, WebFetch, WebSearch, TodoWrite, Skill, Agent, ToolSearch, NotebookEdit, Sleep, SendUserMessage, Config, EnterPlanMode, ExitPlanMode, StructuredOutput, REPL, PowerShell, AskUserQuestion, TaskCreate, RunTaskPacket, TaskGet, TaskList, TaskStop, TaskUpdate, TaskOutput, WorkerCreate, WorkerGet, WorkerObserve, WorkerResolveTrust, WorkerAwaitReady, WorkerSendPrompt, WorkerRestart, WorkerTerminate, WorkerObserveCompletion, TeamCreate, TeamDelete, CronCreate, CronDelete, CronList, LSP, ListMcpResources, ReadMcpResource, McpAuth, RemoteTrigger, MCP, TestingPermission)","kind":"unknown"}`. The `status` subcommand was consumed as the `--allowedTools` value because the flag parser doesn't distinguish missing-value from end-of-flag-args. The error reveals **the supported tool list mixes naming conventions inconsistently within a single error message**: snake_case (`bash`, `read_file`, `write_file`, `edit_file`, `glob_search`, `grep_search`), PascalCase (`WebFetch`, `WebSearch`, `TodoWrite`, `Skill`, `Agent`, `Sleep`, `Config`, `PowerShell`, `AskUserQuestion`, `TaskCreate`, `WorkerCreate`, `TeamCreate`, `CronCreate`), UPPERCASE (`REPL`, `LSP`, `MCP`), and CamelCase compounds (`McpAuth`, `RemoteTrigger`). **Hidden alias mapping**: `claw --allowedTools Read,Write,Edit status --output-format json` is accepted and returns `allowed_tools.entries:["edit_file","read_file","write_file"]` — proving the validator has an undocumented CamelCase→snake_case alias map (`Read`→`read_file`, `Write`→`write_file`, `Edit`→`edit_file`) that is not surfaced in the error message. Users who copy-paste tool names from Claude Code documentation work, users who copy from the validator error don't. **Sibling missing-value bug:** `claw --allowedTools status` with `status` as a positional subcommand is interpreted as `--allowedTools=status`, swallowing the subcommand. The flag parser must require a value for `--allowedTools` and emit `kind:"missing_argument"` when followed by a recognized subcommand or `--`-prefixed flag instead of silently treating the next arg as a tool name. **Sibling typed-kind bug:** both errors use `kind:"unknown"` instead of typed `kind:"invalid_tool_name"` / `kind:"missing_argument"` — the catch-all keeps appearing (#422/#423/#424/#428/#430/#431/#432). **Required fix shape:** (a) standardize the canonical tool-name registry on one casing convention (snake_case is most CLI-ergonomic) and update both the registry and all CamelCase aliases; (b) document and expose the alias map (`tool_aliases:{Read:"read_file",...}`) in `claw doctor`/`status` and in the validator error; (c) flag parser must require a value for `--allowedTools` and refuse to consume a recognized subcommand or `-`/`--`-prefixed token as the value, emit `kind:"missing_argument"` with `argument:"--allowedTools"`; (d) emit `kind:"invalid_tool_name"` with `tool_name:` and `available:[]` fields instead of `kind:"unknown"`; (e) regression test that `claw --allowedTools ` rejects with `missing_argument`, and that the canonical name list in errors uses the same casing as the alias map. **Why this matters:** `--allowedTools` is the primary surface for restricting claw's tool surface area (security-relevant). Inconsistent naming between the validator error and the alias map means users following the error message guidance pick names that work in some places and fail in others. The missing-value bug silently swallows a subcommand, leading to confusing "unsupported tool: status" errors when the user actually wanted to run `claw status`. Cross-references #94/#97/#101/#106/#115/#123 (permission-rule audit), #428 (default permission_mode), #422/#423/#424/#428/#430/#431 (`kind:"unknown"` catch-all). Source: Jobdori live dogfood, `fad53e2d`, 2026-05-11. +432. **DONE — `--allowedTools` uses a canonical snake_case registry with typed diagnostics and documented aliases** — fixed 2026-06-04 in `fix: type allowed tools validation`. `GlobalToolRegistry::normalize_allowed_tools` now normalizes built-in, plugin, runtime, and MCP wrapper tool names to canonical snake_case allow-list entries while still accepting documented aliases such as `read`, `Read`, and legacy provider-facing names like `WebFetch`/`MCPTool`. Provider tool definitions and CLI/subagent executors compare against canonical names, so aliases do not break internal dispatch. `claw --allowedTools status --output-format json` now refuses to consume `status` as a value and emits typed `missing_argument` JSON with `argument:"--allowedTools"`; unsupported names emit typed `invalid_tool_name` JSON with `tool_name`, `available`, and `tool_aliases`. `status --output-format json` exposes `allowed_tools.available` and `allowed_tools.aliases`, and help/usage docs describe canonical names plus aliases. Regression coverage: `parses_allowed_tools_flags_with_aliases_and_lists`, `rejects_allowed_tools_followed_by_subcommand_or_flag_432`, `rejects_unknown_allowed_tools`, `allowed_tools_errors_have_typed_json_and_alias_map_432`, `allowed_tools_normalize_to_canonical_snake_case_and_aliases_432`, status JSON alias assertions, MCP wrapper normalization coverage, and classifier coverage for `invalid_tool_name`. 433. **Repeated `--output-format` flag silently takes the last value without warning — `claw --output-format json --output-format text status` produces text output, no signal that the prior `json` was overridden; sibling: `--output-format` value is case-sensitive (`JSON` rejected as `kind:"unknown"`); sibling: no `CLAW_OUTPUT_FORMAT` env var for default format override** — dogfooded 2026-05-11 by Jobdori on `ce39d5c5` in response to Clawhip pinpoint nudge at `1503290592556220488`. Reproduction: `claw --output-format json --output-format text status` returns the text-format `Status\n Model claude-opus-4-6...` table — the first `--output-format json` was silently overridden. No warning, no `format_overridden:true` field, no stderr message. Scripts that compose flag arrays from multiple sources (`flags=("${BASE_FLAGS[@]}" --output-format json)` while `BASE_FLAGS` already contains `--output-format text`) silently get the wrong format. **Three sibling findings in same probe:** (a) **case-sensitivity drift**: `claw --output-format JSON status` returns `{"error":"unsupported value for --output-format: JSON (expected text or json)","kind":"unknown"}` — error message tells user to use lowercase `json` but doesn't accept the uppercase form that users often type from muscle memory. Most CLI flag-value validators (cargo, kubectl, gh) are case-insensitive for enum values or accept both forms with normalization. (b) **`kind:"unknown"` for invalid format value**: same catch-all bucket bug as #422/#423/#424/#428/#430/#431/#432 — should be `kind:"invalid_output_format"` with `value:` and `expected:["text","json"]` fields. (c) **no env-var default for output format**: `CLAW_OUTPUT_FORMAT=json claw status` silently ignored — no env override for the global default, forcing scripts to repeat `--output-format json` on every invocation. Other major CLIs honor `KUBECTL_OUTPUT=`, `AWS_DEFAULT_OUTPUT=`, `GH_NO_PROMPT=` etc. (d) **silently-ignored env vars `CLAW_LOG`/`RUST_LOG`**: no env-based log level control surfaced in `claw doctor` — debug logging requires undocumented `RUST_LOG=` (Rust convention) but `claw --help` doesn't mention either. **Required fix shape:** (a) repeated `--output-format` (or any flag that takes a value, not a count flag) emits a warning to stderr (`warning: --output-format specified multiple times; using last value 'text'`) and adds a `format_source:"flag", format_overridden:[]` field to the JSON envelope; (b) accept case-insensitive enum values for `--output-format` (`JSON`, `Json`, `json` all work), document the canonical lowercase form in `--help`; (c) emit `kind:"invalid_output_format"` (not `kind:"unknown"`) when value is invalid; (d) accept `CLAW_OUTPUT_FORMAT` env var as the default for `--output-format`, with flag-overrides-env precedence documented; (e) document `RUST_LOG` / `CLAW_LOG` in `--help` or doctor output as the log-level env vars; (f) regression test: repeated flag emits stderr warning + JSON metadata field; case-insensitive enum accepts all three casings; env-var default is honored when flag is absent. **Why this matters:** scripts that compose flag arrays from multiple sources (CI envs + per-invocation flags) silently get the wrong output format. Case-sensitive enum values trip up users typing from muscle memory. Missing env-var defaults force per-invocation flag repetition. Cross-references #422/#423/#424/#428/#430/#431/#432 (`kind:"unknown"` catch-all cluster). Source: Jobdori live dogfood, `ce39d5c5`, 2026-05-11. diff --git a/USAGE.md b/USAGE.md index 2a9a827f..3385f394 100644 --- a/USAGE.md +++ b/USAGE.md @@ -198,6 +198,8 @@ cd rust Global workspace override flags: `--cwd PATH`, `-C PATH`, and `--directory PATH` are accepted before any subcommand. They are validated before command dispatch and take precedence over the process `$PWD`; invalid paths return typed `invalid_cwd` JSON errors in JSON mode. +`--allowedTools` accepts canonical snake_case tool names (for example `read_file`, `glob_search`, `web_fetch`) plus documented aliases such as `read`, `glob`, `Read`, and `WebFetch`. `claw status --output-format json` exposes `allowed_tools.available` and `allowed_tools.aliases`, and invalid values return typed `invalid_tool_name` JSON with `tool_name`, `available`, and `tool_aliases`. A missing value before a subcommand or another flag returns `missing_argument` with `argument:"--allowedTools"`. + Supported permission modes (default: `workspace-write`): - `read-only` allows inspection-only local tools such as file reads, glob/grep searches, local skills, and status-style reporting. It does not allow workspace mutation, network-fetch/search tools, or arbitrary command execution. diff --git a/rust/README.md b/rust/README.md index 80d0033c..58c76105 100644 --- a/rust/README.md +++ b/rust/README.md @@ -126,7 +126,7 @@ Flags: --permission-mode MODE --cwd PATH, -C PATH, --directory PATH --dangerously-skip-permissions, --skip-permissions - --allowedTools TOOLS + --allowedTools TOOLS canonical snake_case names or aliases; status JSON exposes allowed_tools.available/aliases --resume [SESSION.jsonl|session-id|latest] --version, -V diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index 594547e6..9bb8e2a6 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -63,7 +63,8 @@ use runtime::{ use serde::Deserialize; use serde_json::{json, Map, Value}; use tools::{ - execute_tool, mvp_tool_specs, GlobalToolRegistry, RuntimeToolDefinition, ToolSearchOutput, + canonical_allowed_tool_name, execute_tool, mvp_tool_specs, GlobalToolRegistry, + RuntimeToolDefinition, ToolSearchOutput, }; const DEFAULT_MODEL: &str = "anthropic/claude-opus-4-7"; @@ -356,6 +357,19 @@ fn main() { ); } } + } else if kind == "invalid_tool_name" { + let (tool_name, available, aliases) = invalid_tool_name_details(&message); + if let Some(object) = error_json.as_object_mut() { + if let Some(tool_name) = tool_name { + object.insert("tool_name".to_string(), serde_json::json!(tool_name)); + } + object.insert("available".to_string(), serde_json::json!(available)); + object.insert("tool_aliases".to_string(), aliases); + } + } else if kind == "missing_argument" && message.contains("--allowedTools") { + if let Some(object) = error_json.as_object_mut() { + object.insert("argument".to_string(), serde_json::json!("--allowedTools")); + } } // #819/#820/#823: JSON mode error envelopes must go to stdout so machine // consumers can parse failures from stdout byte 0 (parity with all @@ -428,6 +442,8 @@ fn classify_error_kind(message: &str) -> &'static str { "invalid_cwd" } else if message.starts_with("invalid_output_path:") { "invalid_output_path" + } else if message.starts_with("invalid_tool_name:") { + "invalid_tool_name" } else if message.contains("unrecognized argument") || message.contains("unknown option") { "cli_parse" } else if message.starts_with("missing_flag_value:") { @@ -534,6 +550,42 @@ fn split_error_hint(message: &str) -> (String, Option) { } } +fn invalid_tool_name_details(message: &str) -> (Option, Vec, Value) { + let tool_name = message + .strip_prefix("invalid_tool_name: unsupported tool in --allowedTools:") + .and_then(|rest| rest.lines().next()) + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(ToOwned::to_owned); + let available = message + .lines() + .find_map(|line| line.strip_prefix("Available:")) + .map(|line| { + line.split(',') + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(ToOwned::to_owned) + .collect::>() + }) + .unwrap_or_default(); + let aliases = message + .lines() + .find_map(|line| line.strip_prefix("Aliases:")) + .map(|line| { + line.split(',') + .filter_map(|entry| entry.trim().split_once('=')) + .map(|(alias, canonical)| { + ( + alias.trim().to_string(), + Value::String(canonical.trim().to_string()), + ) + }) + .collect::>() + }) + .unwrap_or_default(); + (tool_name, available, Value::Object(aliases)) +} + /// #781: derive a stable fallback hint from a classified error kind when the error /// message itself has no `\n`-delimited hint. Returns `None` for kinds where the /// message is self-explanatory or no canonical remediation exists. @@ -576,6 +628,9 @@ fn fallback_hint_for_error_kind(kind: &str) -> Option<&'static str> { "invalid_install_source" => Some( "Pass a local skill directory containing SKILL.md or a standalone markdown file.", ), + "invalid_tool_name" => Some( + "Use canonical snake_case tool names from `available` or documented aliases from `tool_aliases`.", + ), _ => None, } } @@ -1404,16 +1459,27 @@ fn parse_args(args: &[String]) -> Result { "--allowedTools" | "--allowed-tools" => { let value = args .get(index + 1) - .ok_or_else(|| "missing_flag_value: missing value for --allowedTools.\nUsage: --allowedTools e.g. --allowedTools Bash".to_string())?; + .ok_or_else(allowed_tools_missing_error)?; + if value.starts_with('-') || is_known_top_level_subcommand(value) { + return Err(allowed_tools_missing_error()); + } allowed_tool_values.push(value.clone()); index += 2; } flag if flag.starts_with("--allowedTools=") => { - allowed_tool_values.push(flag[15..].to_string()); + let value = flag[15..].to_string(); + if value.trim().is_empty() { + return Err(allowed_tools_missing_error()); + } + allowed_tool_values.push(value); index += 1; } flag if flag.starts_with("--allowed-tools=") => { - allowed_tool_values.push(flag[16..].to_string()); + let value = flag[16..].to_string(); + if value.trim().is_empty() { + return Err(allowed_tools_missing_error()); + } + allowed_tool_values.push(value); index += 1; } other if rest.is_empty() && other.starts_with('-') => { @@ -2391,6 +2457,41 @@ fn suggest_similar_subcommand(input: &str) -> Option> { (!suggestions.is_empty()).then_some(suggestions) } +fn is_known_top_level_subcommand(value: &str) -> bool { + matches!( + value, + "help" + | "version" + | "status" + | "sandbox" + | "doctor" + | "state" + | "dump-manifests" + | "bootstrap-plan" + | "agents" + | "agent" + | "mcp" + | "skills" + | "skill" + | "plugins" + | "plugin" + | "marketplace" + | "system-prompt" + | "acp" + | "init" + | "export" + | "prompt" + | "resume" + | "session" + | "compact" + | "config" + | "model" + | "models" + | "settings" + | "diff" + ) +} + fn common_prefix_len(left: &str, right: &str) -> usize { left.chars() .zip(right.chars()) @@ -2549,6 +2650,20 @@ fn normalize_allowed_tools(values: &[String]) -> Result, current_tool_registry()?.normalize_allowed_tools(values) } +fn allowed_tools_missing_error() -> String { + "missing_argument: --allowedTools requires a tool list before subcommands or flags.\nUsage: --allowedTools [,...] e.g. --allowedTools read,glob".to_string() +} + +fn allowed_tool_aliases_json(registry: &GlobalToolRegistry) -> Value { + Value::Object( + registry + .allowed_tool_aliases() + .into_iter() + .map(|(alias, canonical)| (alias, Value::String(canonical))) + .collect(), + ) +} + fn current_tool_registry() -> Result { let cwd = env::current_dir().map_err(|error| error.to_string())?; let loader = ConfigLoader::default_for(&cwd); @@ -3089,6 +3204,7 @@ impl DoctorReport { fn json_value(&self) -> Value { let report = self.render(); let (ok_count, warn_count, fail_count) = self.counts(); + let tool_registry = GlobalToolRegistry::builtin(); json!({ "kind": "doctor", "action": "doctor", @@ -3107,6 +3223,10 @@ impl DoctorReport { .iter() .map(DiagnosticCheck::json_value) .collect::>(), + "allowed_tools": { + "available": tool_registry.canonical_allowed_tool_names(), + "aliases": allowed_tool_aliases_json(&tool_registry), + }, }) } } @@ -8194,6 +8314,9 @@ fn status_json_value( let model_env_var = provenance.and_then(|p| p.env_var.clone()); let permission_mode_source = permission_provenance.map(|p| p.source.as_str()); let permission_mode_env_var = permission_provenance.and_then(|p| p.env_var); + let tool_registry = GlobalToolRegistry::builtin(); + let available_tool_names = tool_registry.canonical_allowed_tool_names(); + let tool_aliases = allowed_tool_aliases_json(&tool_registry); // #732: always emit an array (empty when unrestricted) so callers can do // `.allowed_tools.entries | length > 0` without a null-check first. let allowed_tool_entries = allowed_tools @@ -8217,6 +8340,8 @@ fn status_json_value( "source": if allowed_tools.is_some() { "flag" } else { "default" }, "restricted": allowed_tools.is_some(), "entries": allowed_tool_entries, + "available": available_tool_names, + "aliases": tool_aliases, }, "binary_provenance": context.binary_provenance.json_value(), "usage": { @@ -8919,7 +9044,7 @@ fn render_doctor_help_json() -> serde_json::Value { "requires_provider_request": false, "requires_session_resume": false, "mutates_workspace": false, - "output_fields": ["kind", "action", "status", "message", "report", "has_failures", "summary", "checks"], + "output_fields": ["kind", "action", "status", "message", "report", "has_failures", "summary", "checks", "allowed_tools"], "check_names": ["auth", "config", "install source", "workspace", "boot preflight", "sandbox", "permissions", "system"], "status_values": ["ok", "warn", "fail"], "options": [ @@ -12217,7 +12342,7 @@ impl ToolExecutor for CliToolExecutor { if self .allowed_tools .as_ref() - .is_some_and(|allowed| !allowed.contains(tool_name)) + .is_some_and(|allowed| !allowed.contains(&canonical_allowed_tool_name(tool_name))) { return Err(ToolError::new(format!( "tool `{tool_name}` is not enabled by the current --allowedTools setting" @@ -12422,7 +12547,11 @@ fn print_help_to(out: &mut impl Write) -> io::Result<()> { out, " --dangerously-skip-permissions, --skip-permissions Skip all permission checks" )?; - writeln!(out, " --allowedTools TOOLS Restrict enabled tools (repeatable; comma-separated aliases supported)")?; + writeln!( + out, + " --allowedTools TOOLS Restrict enabled tools by canonical snake_case name or alias" + )?; + writeln!(out, " Examples: read, glob, web_fetch, WebFetch; status JSON exposes aliases")?; writeln!( out, " --version, -V Print version and build information locally" @@ -13346,13 +13475,41 @@ mod tests { ); } + #[test] + fn rejects_allowed_tools_followed_by_subcommand_or_flag_432() { + let _env_guard = env_lock(); + let _cwd_guard = cwd_guard(); + for args in [ + vec!["--allowedTools".to_string(), "status".to_string()], + vec![ + "--allowedTools".to_string(), + "status".to_string(), + "--output-format".to_string(), + "json".to_string(), + ], + vec!["--allowedTools".to_string(), "--output-format".to_string()], + vec!["--allowedTools=".to_string()], + ] { + let error = parse_args(&args).expect_err("allowedTools missing value should reject"); + assert!( + error.starts_with("missing_argument: --allowedTools requires a tool list"), + "unexpected error for {args:?}: {error}" + ); + } + } + #[test] fn rejects_unknown_allowed_tools() { let _env_guard = env_lock(); let _cwd_guard = cwd_guard(); let error = parse_args(&["--allowedTools".to_string(), "teleport".to_string()]) .expect_err("tool should be rejected"); + assert!(error.starts_with("invalid_tool_name:")); assert!(error.contains("unsupported tool in --allowedTools: teleport")); + assert!(error.contains("Available: ")); + assert!(error.contains("web_fetch")); + assert!(error.contains("Aliases: ")); + assert!(error.contains("WebFetch=web_fetch")); } #[test] @@ -14097,6 +14254,18 @@ mod tests { Some(false), "default status should expose unrestricted tool state: {json}" ); + assert_eq!( + json.pointer("/allowed_tools/available/0") + .and_then(|v| v.as_str()), + Some("agent"), + "status JSON should expose canonical snake_case available tools: {json}" + ); + assert_eq!( + json.pointer("/allowed_tools/aliases/WebFetch") + .and_then(|v| v.as_str()), + Some("web_fetch"), + "status JSON should expose allowed-tool aliases: {json}" + ); let allowed: super::AllowedToolSet = ["read_file", "grep_search"] .into_iter() @@ -14417,6 +14586,10 @@ mod tests { classify_error_kind("invalid_install_source: bogus"), "invalid_install_source" ); + assert_eq!( + classify_error_kind("invalid_tool_name: unsupported tool in --allowedTools: teleport"), + "invalid_tool_name" + ); assert_eq!( classify_error_kind( "missing_flag_value: missing value for --model.\nUsage: --model " @@ -17206,7 +17379,7 @@ UU conflicted.rs", .expect("mcp tools should be allow-listable") .expect("allow-list should exist"); assert!(allowed.contains("mcp__alpha__echo")); - assert!(allowed.contains("MCPTool")); + assert!(allowed.contains("mcp_tool")); let mut executor = CliToolExecutor::new( None, diff --git a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs index 35d98c19..647534cc 100644 --- a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs +++ b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs @@ -1252,9 +1252,13 @@ fn doctor_and_resume_status_emit_json_when_requested() { assert!(summary["ok"].as_u64().is_some()); assert!(summary["warnings"].as_u64().is_some()); assert!(summary["failures"].as_u64().is_some()); + assert_eq!(doctor["allowed_tools"]["aliases"]["WebFetch"], "web_fetch"); + assert!(doctor["allowed_tools"]["available"] + .as_array() + .is_some_and(|available| available.iter().any(|name| name == "web_fetch"))); let checks = doctor["checks"].as_array().expect("doctor checks"); - assert_eq!(checks.len(), 7); + assert_eq!(checks.len(), 8); let check_names = checks .iter() .map(|check| { @@ -1279,6 +1283,7 @@ fn doctor_and_resume_status_emit_json_when_requested() { "workspace", "boot preflight", "sandbox", + "permissions", "system" ] ); @@ -2718,6 +2723,52 @@ fn flag_value_errors_have_error_kind_and_hint_756() { ); } +#[test] +fn allowed_tools_errors_have_typed_json_and_alias_map_432() { + let root = unique_temp_dir("allowed-tools-432"); + fs::create_dir_all(&root).expect("temp dir"); + + let missing = run_claw( + &root, + &["--allowedTools", "status", "--output-format", "json"], + &[], + ); + assert_eq!(missing.status.code(), Some(1)); + assert!( + missing.stderr.is_empty(), + "JSON missing allowedTools value must keep stderr empty: {}", + String::from_utf8_lossy(&missing.stderr) + ); + let missing_json = parse_json_stdout(&missing, "allowedTools subcommand missing value"); + assert_eq!(missing_json["error_kind"], "missing_argument"); + assert_eq!(missing_json["argument"], "--allowedTools"); + assert!(missing_json["hint"] + .as_str() + .is_some_and(|hint| { hint.contains("--allowedTools") && hint.contains("read,glob") })); + + let invalid = run_claw( + &root, + &["--output-format", "json", "--allowedTools", "teleport"], + &[], + ); + assert_eq!(invalid.status.code(), Some(1)); + assert!( + invalid.stderr.is_empty(), + "JSON invalid allowedTools value must keep stderr empty: {}", + String::from_utf8_lossy(&invalid.stderr) + ); + let invalid_json = parse_json_stdout(&invalid, "allowedTools invalid tool"); + assert_eq!(invalid_json["error_kind"], "invalid_tool_name"); + assert_eq!(invalid_json["tool_name"], "teleport"); + assert!(invalid_json["available"] + .as_array() + .is_some_and(|available| available.iter().any(|name| name == "web_fetch"))); + assert_eq!(invalid_json["tool_aliases"]["WebFetch"], "web_fetch"); + assert!(invalid_json["hint"] + .as_str() + .is_some_and(|hint| { hint.contains("canonical snake_case") && hint.contains("aliases") })); +} + #[test] fn short_p_flag_swallows_no_flags_755() { // #755: `claw -p hello --output-format json` must parse --output-format json diff --git a/rust/crates/tools/src/lib.rs b/rust/crates/tools/src/lib.rs index 5dd1727a..15dff721 100644 --- a/rust/crates/tools/src/lib.rs +++ b/rust/crates/tools/src/lib.rs @@ -201,30 +201,20 @@ impl GlobalToolRegistry { return Ok(None); } - let builtin_specs = mvp_tool_specs(); - let canonical_names = builtin_specs - .iter() - .map(|spec| spec.name.to_string()) - .chain( - self.plugin_tools - .iter() - .map(|tool| tool.definition().name.clone()), - ) - .chain(self.runtime_tools.iter().map(|tool| tool.name.clone())) - .collect::>(); - let mut name_map = canonical_names - .iter() - .map(|name| (normalize_tool_name(name), name.clone())) - .collect::>(); + let actual_names = self.actual_tool_names(); + let canonical_names = self.canonical_allowed_tool_names(); + let canonical_name_set = canonical_names.iter().cloned().collect::>(); + let mut name_map = BTreeMap::new(); + for actual in &actual_names { + let canonical = canonical_allowed_tool_name(actual); + name_map.insert(allowed_tool_lookup_key(actual), canonical.clone()); + name_map.insert(allowed_tool_lookup_key(&canonical), canonical); + } - for (alias, canonical) in [ - ("read", "read_file"), - ("write", "write_file"), - ("edit", "edit_file"), - ("glob", "glob_search"), - ("grep", "grep_search"), - ] { - name_map.insert(alias.to_string(), canonical.to_string()); + for (alias, canonical) in self.allowed_tool_aliases() { + if canonical_name_set.contains(&canonical) { + name_map.insert(allowed_tool_lookup_key(&alias), canonical); + } } let mut allowed = BTreeSet::new(); @@ -233,11 +223,11 @@ impl GlobalToolRegistry { .split(|ch: char| ch == ',' || ch.is_whitespace()) .filter(|token| !token.is_empty()) { - let normalized = normalize_tool_name(token); - let canonical = name_map.get(&normalized).ok_or_else(|| { + let canonical = name_map.get(&allowed_tool_lookup_key(token)).ok_or_else(|| { format!( - "unsupported tool in --allowedTools: {token} (expected one of: {})", - canonical_names.join(", ") + "invalid_tool_name: unsupported tool in --allowedTools: {token}\nAvailable: {}\nAliases: {}\nHint: Use canonical snake_case tool names from Available or aliases from Aliases.", + canonical_names.join(", "), + format_allowed_tool_aliases(&self.allowed_tool_aliases()) ) })?; allowed.insert(canonical.clone()); @@ -258,7 +248,10 @@ impl GlobalToolRegistry { pub fn definitions(&self, allowed_tools: Option<&BTreeSet>) -> Vec { let builtin = mvp_tool_specs() .into_iter() - .filter(|spec| allowed_tools.is_none_or(|allowed| allowed.contains(spec.name))) + .filter(|spec| { + allowed_tools + .is_none_or(|allowed| allowed.contains(&canonical_allowed_tool_name(spec.name))) + }) .map(|spec| ToolDefinition { name: spec.name.to_string(), description: Some(spec.description.to_string()), @@ -267,7 +260,11 @@ impl GlobalToolRegistry { let runtime = self .runtime_tools .iter() - .filter(|tool| allowed_tools.is_none_or(|allowed| allowed.contains(tool.name.as_str()))) + .filter(|tool| { + allowed_tools.is_none_or(|allowed| { + allowed.contains(&canonical_allowed_tool_name(&tool.name)) + }) + }) .map(|tool| ToolDefinition { name: tool.name.clone(), description: tool.description.clone(), @@ -277,8 +274,11 @@ impl GlobalToolRegistry { .plugin_tools .iter() .filter(|tool| { - allowed_tools - .is_none_or(|allowed| allowed.contains(tool.definition().name.as_str())) + allowed_tools.is_none_or(|allowed| { + allowed.contains(&canonical_allowed_tool_name( + tool.definition().name.as_str(), + )) + }) }) .map(|tool| ToolDefinition { name: tool.definition().name.clone(), @@ -294,19 +294,29 @@ impl GlobalToolRegistry { ) -> Result, String> { let builtin = mvp_tool_specs() .into_iter() - .filter(|spec| allowed_tools.is_none_or(|allowed| allowed.contains(spec.name))) + .filter(|spec| { + allowed_tools + .is_none_or(|allowed| allowed.contains(&canonical_allowed_tool_name(spec.name))) + }) .map(|spec| (spec.name.to_string(), spec.required_permission)); let runtime = self .runtime_tools .iter() - .filter(|tool| allowed_tools.is_none_or(|allowed| allowed.contains(tool.name.as_str()))) + .filter(|tool| { + allowed_tools.is_none_or(|allowed| { + allowed.contains(&canonical_allowed_tool_name(&tool.name)) + }) + }) .map(|tool| (tool.name.clone(), tool.required_permission)); let plugin = self .plugin_tools .iter() .filter(|tool| { - allowed_tools - .is_none_or(|allowed| allowed.contains(tool.definition().name.as_str())) + allowed_tools.is_none_or(|allowed| { + allowed.contains(&canonical_allowed_tool_name( + tool.definition().name.as_str(), + )) + }) }) .map(|tool| { permission_mode_from_plugin(tool.required_permission()) @@ -316,6 +326,52 @@ impl GlobalToolRegistry { Ok(builtin.chain(runtime).chain(plugin).collect()) } + #[must_use] + pub fn actual_tool_names(&self) -> Vec { + mvp_tool_specs() + .iter() + .map(|spec| spec.name.to_string()) + .chain( + self.plugin_tools + .iter() + .map(|tool| tool.definition().name.clone()), + ) + .chain(self.runtime_tools.iter().map(|tool| tool.name.clone())) + .collect() + } + + #[must_use] + pub fn canonical_allowed_tool_names(&self) -> Vec { + self.actual_tool_names() + .into_iter() + .map(|name| canonical_allowed_tool_name(&name)) + .collect::>() + .into_iter() + .collect() + } + + #[must_use] + pub fn allowed_tool_aliases(&self) -> BTreeMap { + let mut aliases = BTreeMap::from([ + ("read".to_string(), "read_file".to_string()), + ("Read".to_string(), "read_file".to_string()), + ("write".to_string(), "write_file".to_string()), + ("Write".to_string(), "write_file".to_string()), + ("edit".to_string(), "edit_file".to_string()), + ("Edit".to_string(), "edit_file".to_string()), + ("glob".to_string(), "glob_search".to_string()), + ("Glob".to_string(), "glob_search".to_string()), + ("grep".to_string(), "grep_search".to_string()), + ("Grep".to_string(), "grep_search".to_string()), + ]); + for actual in self.actual_tool_names() { + let canonical = canonical_allowed_tool_name(&actual); + if actual != canonical { + aliases.insert(actual, canonical); + } + } + aliases + } #[must_use] pub fn has_runtime_tool(&self, name: &str) -> bool { self.runtime_tools.iter().any(|tool| tool.name == name) @@ -378,8 +434,40 @@ impl GlobalToolRegistry { } } -fn normalize_tool_name(value: &str) -> String { - value.trim().replace('-', "_").to_ascii_lowercase() +pub fn canonical_allowed_tool_name(value: &str) -> String { + let trimmed = value.trim().replace('-', "_"); + let mut output = String::new(); + let chars = trimmed.chars().collect::>(); + for (index, ch) in chars.iter().copied().enumerate() { + if ch == '_' || ch.is_whitespace() { + output.push('_'); + continue; + } + let previous = index.checked_sub(1).and_then(|i| chars.get(i)).copied(); + let next = chars.get(index + 1).copied(); + if ch.is_ascii_uppercase() + && index > 0 + && !output.ends_with('_') + && (previous.is_some_and(|p| p.is_ascii_lowercase() || p.is_ascii_digit()) + || next.is_some_and(|n| n.is_ascii_lowercase())) + { + output.push('_'); + } + output.push(ch.to_ascii_lowercase()); + } + output.trim_matches('_').to_string() +} + +fn allowed_tool_lookup_key(value: &str) -> String { + canonical_allowed_tool_name(value).replace('_', "") +} + +fn format_allowed_tool_aliases(aliases: &BTreeMap) -> String { + aliases + .iter() + .map(|(alias, canonical)| format!("{alias}={canonical}")) + .collect::>() + .join(", ") } fn permission_mode_from_plugin(value: &str) -> Result { @@ -4210,7 +4298,7 @@ fn allowed_tools_for_subagent(subagent_type: &str) -> BTreeSet { "PowerShell", ], }; - tools.into_iter().map(str::to_string).collect() + tools.into_iter().map(canonical_allowed_tool_name).collect() } fn agent_permission_policy() -> PermissionPolicy { @@ -5238,7 +5326,10 @@ impl SubagentToolExecutor { impl ToolExecutor for SubagentToolExecutor { fn execute(&mut self, tool_name: &str, input: &str) -> Result { - if !self.allowed_tools.contains(tool_name) { + if !self + .allowed_tools + .contains(&canonical_allowed_tool_name(tool_name)) + { return Err(ToolError::new(format!( "tool `{tool_name}` is not enabled for this sub-agent" ))); @@ -5253,7 +5344,10 @@ impl ToolExecutor for SubagentToolExecutor { fn tool_specs_for_allowed_tools(allowed_tools: Option<&BTreeSet>) -> Vec { mvp_tool_specs() .into_iter() - .filter(|spec| allowed_tools.is_none_or(|allowed| allowed.contains(spec.name))) + .filter(|spec| { + allowed_tools + .is_none_or(|allowed| allowed.contains(&canonical_allowed_tool_name(spec.name))) + }) .collect() } @@ -7603,6 +7697,29 @@ mod tests { } } + #[test] + fn allowed_tools_normalize_to_canonical_snake_case_and_aliases_432() { + let registry = GlobalToolRegistry::builtin(); + let allowed = registry + .normalize_allowed_tools(&["Read,WebFetch,MCP".to_string()]) + .expect("aliases and legacy names should normalize") + .expect("allow-list should be populated"); + assert!(allowed.contains("read_file")); + assert!(allowed.contains("web_fetch")); + assert!(allowed.contains("mcp")); + assert!(!allowed.contains("Read")); + assert!(!allowed.contains("WebFetch")); + + let canonical = registry.canonical_allowed_tool_names(); + assert!(canonical.contains(&"web_fetch".to_string())); + assert!(canonical.contains(&"todo_write".to_string())); + assert!(!canonical.contains(&"WebFetch".to_string())); + assert_eq!( + registry.allowed_tool_aliases().get("WebFetch"), + Some(&"web_fetch".to_string()) + ); + } + #[test] fn runtime_tools_extend_registry_definitions_permissions_and_search() { let registry = GlobalToolRegistry::builtin() @@ -8584,7 +8701,7 @@ mod tests { .expect("spawn job should be captured"); assert_eq!(captured_job.prompt, "Check tests and outstanding work."); assert!(captured_job.allowed_tools.contains("read_file")); - assert!(!captured_job.allowed_tools.contains("Agent")); + assert!(!captured_job.allowed_tools.contains("agent")); let normalized = execute_tool( "Agent", @@ -9184,7 +9301,7 @@ mod tests { let general = allowed_tools_for_subagent("general-purpose"); assert!(general.contains("bash")); assert!(general.contains("write_file")); - assert!(!general.contains("Agent")); + assert!(!general.contains("agent")); let explore = allowed_tools_for_subagent("Explore"); assert!(explore.contains("read_file")); @@ -9192,13 +9309,13 @@ mod tests { assert!(!explore.contains("bash")); let plan = allowed_tools_for_subagent("Plan"); - assert!(plan.contains("TodoWrite")); - assert!(plan.contains("StructuredOutput")); - assert!(!plan.contains("Agent")); + assert!(plan.contains("todo_write")); + assert!(plan.contains("structured_output")); + assert!(!plan.contains("agent")); let verification = allowed_tools_for_subagent("Verification"); assert!(verification.contains("bash")); - assert!(verification.contains("PowerShell")); + assert!(verification.contains("power_shell")); assert!(!verification.contains("write_file")); }