eeymoo/claw-code
1
0
Fork 0
mirror of https://github.com/instructkr/claw-code.git synced 2026-06-04 13:37:09 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: type allowed tools validation

Browse Source
This commit is contained in:
bellman
2026-06-04 12:01:58 +09:00
parent 22fdaeae2c
commit ecd3e4ceb9
6 changed files with 400 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
ROADMAP.md
View File

@@ -6371,7 +6371,7 @@ Original filing (2026-04-18): the session emitted `SessionStart hook (completed)
431. **DONE — `skills uninstall <name>` 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 <bogus-name>` 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 <name>` now scaffolds `.claw/agents/<name>.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 <subcommand>` 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.

2
USAGE.md
View File

@@ -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.

2
rust/README.md
View File

@@ -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

189
rust/crates/rusty-claude-cli/src/main.rs
View File

@@ -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<String>) {
}
}
fn invalid_tool_name_details(message: &str) -> (Option<String>, Vec<String>, 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::<Vec<_>>()
})
.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::<Map<_, _>>()
})
.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<CliAction, String> {
"--allowedTools" | "--allowed-tools" => {
let value = args
.get(index + 1)
.ok_or_else(|| "missing_flag_value: missing value for --allowedTools.\nUsage: --allowedTools <tool-name> 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<Vec<String>> {
(!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<Option<AllowedToolSet>,
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 <tool-name>[,<tool-name>...] 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<GlobalToolRegistry, String> {
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::<Vec<_>>(),
"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 <provider/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,

53
rust/crates/rusty-claude-cli/tests/output_format_contract.rs
View File

@@ -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

209
rust/crates/tools/src/lib.rs
View File

@@ -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::<Vec<_>>();
let mut name_map = canonical_names
.iter()
.map(|name| (normalize_tool_name(name), name.clone()))
.collect::<BTreeMap<_, _>>();
let actual_names = self.actual_tool_names();
let canonical_names = self.canonical_allowed_tool_names();
let canonical_name_set = canonical_names.iter().cloned().collect::<BTreeSet<_>>();
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<String>>) -> Vec<ToolDefinition> {
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<Vec<(String, PermissionMode)>, 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<String> {
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<String> {
self.actual_tool_names()
.into_iter()
.map(|name| canonical_allowed_tool_name(&name))
.collect::<BTreeSet<_>>()
.into_iter()
.collect()
}
#[must_use]
pub fn allowed_tool_aliases(&self) -> BTreeMap<String, String> {
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::<Vec<_>>();
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, String>) -> String {
aliases
.iter()
.map(|(alias, canonical)| format!("{alias}={canonical}"))
.collect::<Vec<_>>()
.join(", ")
}
fn permission_mode_from_plugin(value: &str) -> Result<PermissionMode, String> {
@@ -4210,7 +4298,7 @@ fn allowed_tools_for_subagent(subagent_type: &str) -> BTreeSet<String> {
"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<String, ToolError> {
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<String>>) -> Vec<ToolSpec> {
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"));
}