refactor(tools): replace panic paths with proper error handling

- Convert permission_mode_from_plugin panic to Result-based error
- Add input validation for tool dispatch edge cases
- Propagate signature changes to main.rs caller
- 29 tools tests pass, clippy clean
This commit is contained in:
YeonGyu-Kim
2026-04-02 18:04:55 +09:00
parent 6e4b0123a6
commit f5fa3e26c8
2 changed files with 32 additions and 19 deletions

View File

@@ -3732,7 +3732,8 @@ fn build_runtime(
progress_reporter,
)?,
CliToolExecutor::new(allowed_tools.clone(), emit_output, tool_registry.clone()),
permission_policy(permission_mode, &feature_config, &tool_registry),
permission_policy(permission_mode, &feature_config, &tool_registry)
.map_err(std::io::Error::other)?,
system_prompt,
&feature_config,
);
@@ -4731,13 +4732,13 @@ fn permission_policy(
mode: PermissionMode,
feature_config: &runtime::RuntimeFeatureConfig,
tool_registry: &GlobalToolRegistry,
) -> PermissionPolicy {
tool_registry.permission_specs(None).into_iter().fold(
) -> Result<PermissionPolicy, String> {
Ok(tool_registry.permission_specs(None)?.into_iter().fold(
PermissionPolicy::new(mode).with_permission_rules(feature_config.permission_rules()),
|policy, (name, required_permission)| {
policy.with_tool_requirement(name, required_permission)
},
)
))
}
fn convert_messages(messages: &[ConversationMessage]) -> Vec<InputMessage> {
@@ -5391,7 +5392,8 @@ mod tests {
PermissionMode::ReadOnly,
&feature_config,
&registry_with_plugin_tool(),
);
)
.expect("permission policy should build");
let required = policy.required_mode_for("plugin_echo");
assert_eq!(required, PermissionMode::WorkspaceWrite);
}

View File

@@ -173,7 +173,7 @@ impl GlobalToolRegistry {
pub fn permission_specs(
&self,
allowed_tools: Option<&BTreeSet<String>>,
) -> Vec<(String, PermissionMode)> {
) -> Result<Vec<(String, PermissionMode)>, String> {
let builtin = mvp_tool_specs()
.into_iter()
.filter(|spec| allowed_tools.is_none_or(|allowed| allowed.contains(spec.name)))
@@ -186,12 +186,11 @@ impl GlobalToolRegistry {
.is_none_or(|allowed| allowed.contains(tool.definition().name.as_str()))
})
.map(|tool| {
(
tool.definition().name.clone(),
permission_mode_from_plugin(tool.required_permission()),
)
});
builtin.chain(plugin).collect()
permission_mode_from_plugin(tool.required_permission())
.map(|permission| (tool.definition().name.clone(), permission))
})
.collect::<Result<Vec<_>, _>>()?;
Ok(builtin.chain(plugin).collect())
}
pub fn execute(&self, name: &str, input: &Value) -> Result<String, String> {
@@ -211,12 +210,12 @@ fn normalize_tool_name(value: &str) -> String {
value.trim().replace('-', "_").to_ascii_lowercase()
}
fn permission_mode_from_plugin(value: &str) -> PermissionMode {
fn permission_mode_from_plugin(value: &str) -> Result<PermissionMode, String> {
match value {
"read-only" => PermissionMode::ReadOnly,
"workspace-write" => PermissionMode::WorkspaceWrite,
"danger-full-access" => PermissionMode::DangerFullAccess,
other => panic!("unsupported plugin permission: {other}"),
"read-only" => Ok(PermissionMode::ReadOnly),
"workspace-write" => Ok(PermissionMode::WorkspaceWrite),
"danger-full-access" => Ok(PermissionMode::DangerFullAccess),
other => Err(format!("unsupported plugin permission: {other}")),
}
}
@@ -3102,8 +3101,9 @@ mod tests {
use super::{
agent_permission_policy, allowed_tools_for_subagent, execute_agent_with_spawn,
execute_tool, final_assistant_text, mvp_tool_specs, persist_agent_terminal_state,
push_output_block, AgentInput, AgentJob, SubagentToolExecutor,
execute_tool, final_assistant_text, mvp_tool_specs, permission_mode_from_plugin,
persist_agent_terminal_state, push_output_block, AgentInput, AgentJob,
SubagentToolExecutor,
};
use api::OutputContentBlock;
use runtime::{ApiRequest, AssistantEvent, ConversationRuntime, RuntimeError, Session};
@@ -3151,6 +3151,17 @@ mod tests {
assert!(error.contains("unsupported tool"));
}
#[test]
fn permission_mode_from_plugin_rejects_invalid_inputs() {
let unknown_permission = permission_mode_from_plugin("admin")
.expect_err("unknown plugin permission should fail");
assert!(unknown_permission.contains("unsupported plugin permission: admin"));
let empty_permission = permission_mode_from_plugin("")
.expect_err("empty plugin permission should fail");
assert!(empty_permission.contains("unsupported plugin permission: "));
}
#[test]
fn web_fetch_returns_prompt_aware_summary() {
let server = TestServer::spawn(Arc::new(|request_line: &str| {