diff --git a/crates/llm-worker/src/interceptor.rs b/crates/llm-worker/src/interceptor.rs index 9ab8323a..05afe4ea 100644 --- a/crates/llm-worker/src/interceptor.rs +++ b/crates/llm-worker/src/interceptor.rs @@ -52,6 +52,11 @@ pub enum PreToolAction { Continue, /// Skip this tool call (do not execute). Skip, + /// Do not execute the tool call; commit this synthetic result instead. + /// + /// This preserves provider-visible `tool_use` / `tool_result` pairing + /// without aborting the whole turn. + SyntheticResult(ToolResult), /// Abort the entire run. Abort(String), /// Pause execution (can be resumed later). diff --git a/crates/llm-worker/src/llm_client/scheme/anthropic/request.rs b/crates/llm-worker/src/llm_client/scheme/anthropic/request.rs index 57ceb323..835840f7 100644 --- a/crates/llm-worker/src/llm_client/scheme/anthropic/request.rs +++ b/crates/llm-worker/src/llm_client/scheme/anthropic/request.rs @@ -14,6 +14,10 @@ use crate::llm_client::{ use super::AnthropicScheme; +fn is_false(value: &bool) -> bool { + !*value +} + /// Anthropic API request body #[derive(Debug, Serialize)] pub(crate) struct AnthropicRequest { @@ -104,6 +108,8 @@ pub(crate) enum AnthropicContentPart { ToolResult { tool_use_id: String, content: String, + #[serde(default, skip_serializing_if = "is_false")] + is_error: bool, #[serde(skip_serializing_if = "Option::is_none")] cache_control: Option, }, @@ -141,10 +147,11 @@ impl AnthropicContentPart { } } - fn tool_result(tool_use_id: String, content: String) -> Self { + fn tool_result(tool_use_id: String, content: String, is_error: bool) -> Self { Self::ToolResult { tool_use_id, content, + is_error, cache_control: None, } } @@ -321,6 +328,7 @@ impl AnthropicScheme { call_id, summary, content, + is_error, .. } => { flush_pending( @@ -333,8 +341,10 @@ impl AnthropicScheme { Some(c) => format!("{summary}\n{c}"), None => summary.clone(), }; - pending_user - .push((i, AnthropicContentPart::tool_result(call_id.clone(), text))); + pending_user.push(( + i, + AnthropicContentPart::tool_result(call_id.clone(), text, *is_error), + )); } Item::Reasoning { @@ -355,13 +365,10 @@ impl AnthropicScheme { // 素の reasoning text。Anthropic に投げる意味も // round-trip の根拠も無いので drop。 if let Some(sig) = signature.clone() { - pending_assistant.push(( - i, - AnthropicContentPart::thinking(text.clone(), sig), - )); - } else if let Some(data) = encrypted_content.clone() { pending_assistant - .push((i, AnthropicContentPart::redacted_thinking(data))); + .push((i, AnthropicContentPart::thinking(text.clone(), sig))); + } else if let Some(data) = encrypted_content.clone() { + pending_assistant.push((i, AnthropicContentPart::redacted_thinking(data))); } // どちらも None なら何も pend せず、本 item は無視。 } @@ -828,7 +835,9 @@ mod tests { assert_eq!(thinking_parts.len(), 1); match thinking_parts[0] { AnthropicContentPart::Thinking { - thinking, signature, .. + thinking, + signature, + .. } => { assert_eq!(thinking, "step-by-step"); assert_eq!(signature, "SIG-A"); diff --git a/crates/llm-worker/src/llm_client/types.rs b/crates/llm-worker/src/llm_client/types.rs index 93afafc1..26528078 100644 --- a/crates/llm-worker/src/llm_client/types.rs +++ b/crates/llm-worker/src/llm_client/types.rs @@ -9,6 +9,10 @@ use serde::{Deserialize, Serialize}; +fn is_false(value: &bool) -> bool { + !*value +} + // ============================================================================ // Item - The core unit of conversation // ============================================================================ @@ -79,6 +83,9 @@ pub enum Item { /// Detailed output (removed by pruning when old enough) #[serde(default, skip_serializing_if = "Option::is_none")] content: Option, + /// Whether the tool result represents an execution error. + #[serde(default, skip_serializing_if = "is_false")] + is_error: bool, }, /// Reasoning/thinking item @@ -198,11 +205,27 @@ impl Item { /// Create a tool result item with summary only (no content). pub fn tool_result(call_id: impl Into, summary: impl Into) -> Self { + Self::tool_result_item(call_id, summary, None, false) + } + + /// Create an error tool result item with summary only (no content). + pub fn tool_result_error(call_id: impl Into, summary: impl Into) -> Self { + Self::tool_result_item(call_id, summary, None, true) + } + + /// Create a tool result item with summary, optional content, and error flag. + pub fn tool_result_item( + call_id: impl Into, + summary: impl Into, + content: Option, + is_error: bool, + ) -> Self { Self::ToolResult { id: None, call_id: call_id.into(), summary: summary.into(), - content: None, + content, + is_error, } } @@ -212,12 +235,7 @@ impl Item { summary: impl Into, content: impl Into, ) -> Self { - Self::ToolResult { - id: None, - call_id: call_id.into(), - summary: summary.into(), - content: Some(content.into()), - } + Self::tool_result_item(call_id, summary, Some(content.into()), false) } // ======================================================================== diff --git a/crates/llm-worker/src/tool.rs b/crates/llm-worker/src/tool.rs index df70d43c..0185954f 100644 --- a/crates/llm-worker/src/tool.rs +++ b/crates/llm-worker/src/tool.rs @@ -275,7 +275,7 @@ pub struct ToolCall { /// /// Intermediate representation between tool execution and history. /// Carries `summary` + optional `content` from [`ToolOutput`]. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct ToolResult { /// Corresponding tool call ID pub tool_use_id: String, diff --git a/crates/llm-worker/src/worker.rs b/crates/llm-worker/src/worker.rs index 6431b392..8c98c210 100644 --- a/crates/llm-worker/src/worker.rs +++ b/crates/llm-worker/src/worker.rs @@ -742,8 +742,9 @@ impl Worker { // Map from tool call ID to (ToolCall, Meta, Tool) // Retained because it's needed for PostToolCall hooks let mut call_info_map = HashMap::new(); + let mut synthetic_results = Vec::new(); - // Phase 1: Apply pre_tool_call interceptor (determine skip/abort) + // Phase 1: Apply pre_tool_call interceptor (determine skip/abort/synthetic result) let mut approved_calls = Vec::new(); for mut tool_call in tool_calls { if let Some((meta, tool)) = self.tool_server.get_tool(&tool_call.name) { @@ -758,6 +759,15 @@ impl Worker { PreToolAction::Skip => { continue; } + PreToolAction::SyntheticResult(result) => { + let tool_call = info.call; + call_info_map.insert( + tool_call.id.clone(), + (tool_call, info.meta.clone(), info.tool.clone()), + ); + synthetic_results.push(result); + continue; + } PreToolAction::Abort(reason) => { self.last_run_interrupted = true; return Err(WorkerError::Aborted(reason)); @@ -809,6 +819,7 @@ impl Worker { return Err(WorkerError::Cancelled); } }; + results.extend(synthetic_results); // Phase 3: Apply post_tool_call interceptor for tool_result in &mut results { @@ -1124,16 +1135,12 @@ impl Worker { } Ok(ToolExecutionResult::Completed(results)) => { for result in results { - if let Some(ref content) = result.content { - self.history.push(Item::tool_result_with_content( - &result.tool_use_id, - &result.summary, - content, - )); - } else { - self.history - .push(Item::tool_result(&result.tool_use_id, &result.summary)); - } + self.history.push(Item::tool_result_item( + &result.tool_use_id, + &result.summary, + result.content, + result.is_error, + )); } Ok(None) } diff --git a/crates/llm-worker/tests/parallel_execution_test.rs b/crates/llm-worker/tests/parallel_execution_test.rs index d3ff86a2..2e36c991 100644 --- a/crates/llm-worker/tests/parallel_execution_test.rs +++ b/crates/llm-worker/tests/parallel_execution_test.rs @@ -12,7 +12,7 @@ use llm_worker::interceptor::{ Interceptor, PostToolAction, PreToolAction, ToolCallInfo, ToolResultInfo, }; use llm_worker::llm_client::event::{Event, ResponseStatus, StatusEvent}; -use llm_worker::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; +use llm_worker::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput, ToolResult}; mod common; use common::MockLlmClient; @@ -268,3 +268,59 @@ async fn test_post_tool_call_modification() { "Result should be modified" ); } + +/// Hook: pre_tool_call synthetic result - skipped tool gets an error result in history. +#[tokio::test] +async fn test_before_tool_call_synthetic_result_committed() { + let events = vec![ + Event::tool_use_start(0, "call_1", "blocked_tool"), + Event::tool_input_delta(0, r#"{}"#), + Event::tool_use_stop(0), + Event::Status(StatusEvent { + status: ResponseStatus::Completed, + }), + ]; + + let client = MockLlmClient::with_responses(vec![ + events, + vec![ + Event::text_block_start(0), + Event::text_delta(0, "Denied."), + Event::text_block_stop(0, None), + Event::Status(StatusEvent { + status: ResponseStatus::Completed, + }), + ], + ]); + let mut worker = Worker::new(client); + let blocked_tool = SlowTool::new("blocked_tool", 10); + let blocked_clone = blocked_tool.clone(); + worker.register_tool(blocked_tool.definition()); + + struct SyntheticPolicy; + + #[async_trait] + impl Interceptor for SyntheticPolicy { + async fn pre_tool_call(&self, info: &mut ToolCallInfo) -> PreToolAction { + PreToolAction::SyntheticResult(ToolResult::error( + info.call.id.clone(), + "permission denied", + )) + } + } + + worker.set_interceptor(SyntheticPolicy); + + let result = worker.run("Test synthetic result").await.unwrap(); + + assert_eq!(blocked_clone.call_count(), 0, "Blocked tool should not run"); + assert!(result.worker.history().iter().any(|item| matches!( + item, + llm_worker::Item::ToolResult { + call_id, + summary, + is_error: true, + .. + } if call_id == "call_1" && summary == "permission denied" + ))); +} diff --git a/crates/manifest/src/config.rs b/crates/manifest/src/config.rs index 58809d84..022c82a1 100644 --- a/crates/manifest/src/config.rs +++ b/crates/manifest/src/config.rs @@ -16,7 +16,7 @@ use crate::defaults; use crate::model::{AuthRef, ModelManifest, ReasoningControl}; use crate::{ CompactionConfig, MemoryConfig, PodManifest, PodMeta, ScopeConfig, SkillsConfig, - ToolOutputLimits, WorkerManifest, + ToolOutputLimits, ToolPermissionConfig, ToolPermissionRule, WorkerManifest, }; /// Partial-form Pod manifest. Every field is optional; one or more @@ -36,6 +36,10 @@ pub struct PodManifestConfig { pub worker: WorkerManifestConfig, #[serde(default)] pub scope: ScopeConfig, + /// Optional `[permissions]` section. `None` means the permission layer + /// is disabled; `Some` requires `default_action` during final resolve. + #[serde(default)] + pub permissions: Option, #[serde(default)] pub compaction: Option, /// Memory subsystem opt-in. See [`MemoryConfig`]. @@ -87,6 +91,14 @@ pub struct ToolOutputLimitsPartial { pub per_tool: HashMap, } +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct PermissionConfigPartial { + #[serde(default)] + pub default_action: Option, + #[serde(default, rename = "rule")] + pub rules: Vec, +} + #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct CompactionConfigPartial { #[serde(default)] @@ -204,6 +216,11 @@ impl PodManifestConfig { model: self.model.merge(upper.model), worker: self.worker.merge(upper.worker), scope: merge_scope(self.scope, upper.scope), + permissions: merge_option( + self.permissions, + upper.permissions, + PermissionConfigPartial::merge, + ), compaction: merge_option( self.compaction, upper.compaction, @@ -280,6 +297,16 @@ impl ToolOutputLimitsPartial { } } +impl PermissionConfigPartial { + fn merge(mut self, upper: Self) -> Self { + self.rules.extend(upper.rules); + Self { + default_action: upper.default_action.or(self.default_action), + rules: self.rules, + } + } +} + impl CompactionConfigPartial { fn merge(self, upper: Self) -> Self { Self { @@ -400,6 +427,18 @@ impl TryFrom for PodManifest { ensure_absolute("scope.deny.target", &rule.target)?; } + let permissions = cfg + .permissions + .map(|p| { + Ok(ToolPermissionConfig { + default_action: p + .default_action + .ok_or(ResolveError::MissingField("permissions.default_action"))?, + rules: p.rules, + }) + }) + .transpose()?; + let compaction = cfg .compaction .map(|c| -> Result { @@ -438,6 +477,7 @@ impl TryFrom for PodManifest { model: cfg.model, worker, scope: cfg.scope, + permissions, compaction, memory: cfg.memory, skills: cfg.skills, @@ -482,6 +522,7 @@ mod tests { }], deny: Vec::new(), }, + permissions: None, compaction: None, memory: None, skills: None, @@ -493,6 +534,51 @@ mod tests { let manifest: PodManifest = minimal_valid().try_into().unwrap(); assert_eq!(manifest.pod.name, "test"); assert_eq!(manifest.model.scheme, Some(SchemeKind::Anthropic)); + assert!(manifest.permissions.is_none()); + } + + #[test] + fn resolve_permissions_requires_default_action_when_present() { + let mut cfg = minimal_valid(); + cfg.permissions = Some(PermissionConfigPartial { + default_action: None, + rules: Vec::new(), + }); + + let err = PodManifest::try_from(cfg).unwrap_err(); + + assert!(matches!( + err, + ResolveError::MissingField("permissions.default_action") + )); + } + + #[test] + fn resolve_permissions_preserves_actions_and_rule_order() { + let mut cfg = minimal_valid(); + cfg.permissions = Some(PermissionConfigPartial { + default_action: Some(crate::ToolPermissionAction::Ask), + rules: vec![ + ToolPermissionRule { + tool: "Bash".into(), + pattern: "rm *".into(), + action: crate::ToolPermissionAction::Deny, + }, + ToolPermissionRule { + tool: "Read".into(), + pattern: "*".into(), + action: crate::ToolPermissionAction::Allow, + }, + ], + }); + + let manifest: PodManifest = cfg.try_into().unwrap(); + let permissions = manifest.permissions.unwrap(); + + assert_eq!(permissions.default_action, crate::ToolPermissionAction::Ask); + assert_eq!(permissions.rules.len(), 2); + assert_eq!(permissions.rules[0].tool, "Bash"); + assert_eq!(permissions.rules[1].tool, "Read"); } #[test] @@ -694,6 +780,42 @@ mod tests { assert_eq!(merged.scope.deny.len(), 1); } + #[test] + fn merge_permissions_accumulates_rules_and_upper_default_wins() { + let lower = PodManifestConfig { + permissions: Some(PermissionConfigPartial { + default_action: Some(crate::ToolPermissionAction::Allow), + rules: vec![ToolPermissionRule { + tool: "Bash".into(), + pattern: "git *".into(), + action: crate::ToolPermissionAction::Allow, + }], + }), + ..Default::default() + }; + let upper = PodManifestConfig { + permissions: Some(PermissionConfigPartial { + default_action: Some(crate::ToolPermissionAction::Deny), + rules: vec![ToolPermissionRule { + tool: "Bash".into(), + pattern: "rm *".into(), + action: crate::ToolPermissionAction::Deny, + }], + }), + ..Default::default() + }; + + let merged = lower.merge(upper).permissions.unwrap(); + + assert_eq!( + merged.default_action, + Some(crate::ToolPermissionAction::Deny) + ); + assert_eq!(merged.rules.len(), 2); + assert_eq!(merged.rules[0].pattern, "git *"); + assert_eq!(merged.rules[1].pattern, "rm *"); + } + #[test] fn merge_tool_output_per_tool_keywise() { let lower = PodManifestConfig { diff --git a/crates/manifest/src/lib.rs b/crates/manifest/src/lib.rs index 69f74db9..c400aecb 100644 --- a/crates/manifest/src/lib.rs +++ b/crates/manifest/src/lib.rs @@ -7,8 +7,8 @@ mod scope; pub use cascade::{LayerLoadError, find_project_manifest_from, load_layer}; pub use config::{ - CompactionConfigPartial, PodManifestConfig, PodMetaConfig, ResolveError, - ToolOutputLimitsPartial, WorkerManifestConfig, + CompactionConfigPartial, PermissionConfigPartial, PodManifestConfig, PodMetaConfig, + ResolveError, ToolOutputLimitsPartial, WorkerManifestConfig, }; pub use model::{ AuthRef, ModelCapability, ModelManifest, ReasoningControl, ReasoningEffort, SchemeKind, @@ -35,6 +35,10 @@ pub struct PodManifest { pub model: ModelManifest, pub worker: WorkerManifest, pub scope: ScopeConfig, + /// Optional manifest-level tool permission policy. Absent means the + /// permission layer is disabled and tool calls run as before. + #[serde(default)] + pub permissions: Option, #[serde(default)] pub compaction: Option, /// Memory subsystem opt-in. Presence of `[memory]` in TOML enables @@ -239,6 +243,38 @@ pub struct ScopeConfig { pub deny: Vec, } +/// Manifest-level pattern-based tool permission policy. +/// +/// Presence of `[permissions]` enables this layer. Rules are evaluated +/// in declaration order; if none match, [`Self::default_action`] is used. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ToolPermissionConfig { + pub default_action: ToolPermissionAction, + #[serde(default, rename = "rule")] + pub rules: Vec, +} + +/// One `[[permissions.rule]]` entry. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ToolPermissionRule { + /// Tool registration name. Matching is case-insensitive at runtime so + /// manifests may use either `Bash` or `bash`. + pub tool: String, + /// Glob-like pattern matched against the tool's permission target + /// (for built-in tools, commonly `command`, `file_path`, or `pattern`). + pub pattern: String, + pub action: ToolPermissionAction, +} + +/// Tool permission decision. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ToolPermissionAction { + Allow, + Deny, + Ask, +} + /// Context compaction configuration. /// /// Controls Prune (content removal from old tool results) and Compact diff --git a/crates/pod/src/lib.rs b/crates/pod/src/lib.rs index a6786a34..c9b030a6 100644 --- a/crates/pod/src/lib.rs +++ b/crates/pod/src/lib.rs @@ -11,6 +11,7 @@ pub mod workflow; mod factory; mod interrupt_and_run; +mod permission; mod pod; pub use compact::token_counter::{EstimateSource, SplitPoint, TokenEstimate}; diff --git a/crates/pod/src/permission.rs b/crates/pod/src/permission.rs new file mode 100644 index 00000000..49833e82 --- /dev/null +++ b/crates/pod/src/permission.rs @@ -0,0 +1,189 @@ +use async_trait::async_trait; +use llm_worker::interceptor::PreToolAction; +use llm_worker::llm_client::client::LlmClient; +use llm_worker::tool::ToolResult; +use manifest::{ToolPermissionAction, ToolPermissionConfig}; +use serde_json::Value; +use session_store::Store; + +use crate::Pod; +use crate::hook::{Hook, PreToolCall, ToolCallSummary}; + +/// Built-in manifest permission policy for `PreToolCall`. +/// +/// This hook is registered by Pod before user hooks, so manifest-level deny +/// rules fail closed before user extension code can approve a call. +pub(crate) struct PermissionHook { + config: ToolPermissionConfig, +} + +impl PermissionHook { + pub(crate) fn new(config: ToolPermissionConfig) -> Self { + Self { config } + } + + fn action_for(&self, input: &ToolCallSummary) -> ToolPermissionAction { + let target = permission_target(&input.arguments); + self.config + .rules + .iter() + .find(|rule| { + rule.tool.eq_ignore_ascii_case(&input.tool_name) + && wildcard_match(&rule.pattern, &target) + }) + .map(|rule| rule.action) + .unwrap_or(self.config.default_action) + } +} + +impl Pod { + pub(crate) fn apply_permissions_from_manifest(&mut self) { + let Some(permissions) = self.manifest().permissions.clone() else { + return; + }; + self.add_pre_tool_call_hook(PermissionHook::new(permissions)); + } +} + +#[async_trait] +impl Hook for PermissionHook { + async fn call(&self, input: &ToolCallSummary) -> PreToolAction { + match self.action_for(input) { + ToolPermissionAction::Allow => PreToolAction::Continue, + ToolPermissionAction::Deny => PreToolAction::SyntheticResult(permission_denied(input)), + ToolPermissionAction::Ask => { + PreToolAction::SyntheticResult(permission_ask_unsupported(input)) + } + } + } +} + +fn permission_denied(input: &ToolCallSummary) -> ToolResult { + ToolResult::error( + input.call_id.clone(), + format!( + "permission denied: tool `{}` arguments matched the manifest permission policy", + input.tool_name + ), + ) +} + +fn permission_ask_unsupported(input: &ToolCallSummary) -> ToolResult { + ToolResult::error( + input.call_id.clone(), + format!( + "permission ask unsupported: tool `{}` requires approval, but this runtime has no permission approval protocol; denied fail-closed", + input.tool_name + ), + ) +} + +fn permission_target(arguments: &Value) -> String { + if let Value::Object(map) = arguments { + for key in ["command", "file_path", "path", "pattern"] { + if let Some(value) = map.get(key).and_then(Value::as_str) { + return value.to_string(); + } + } + } + serde_json::to_string(arguments).unwrap_or_else(|_| arguments.to_string()) +} + +fn wildcard_match(pattern: &str, text: &str) -> bool { + let pattern = pattern.as_bytes(); + let text = text.as_bytes(); + let (mut pi, mut ti) = (0usize, 0usize); + let mut star: Option = None; + let mut star_text = 0usize; + + while ti < text.len() { + if pi < pattern.len() && (pattern[pi] == b'?' || pattern[pi] == text[ti]) { + pi += 1; + ti += 1; + } else if pi < pattern.len() && pattern[pi] == b'*' { + star = Some(pi); + pi += 1; + star_text = ti; + } else if let Some(star_pi) = star { + pi = star_pi + 1; + star_text += 1; + ti = star_text; + } else { + return false; + } + } + + while pi < pattern.len() && pattern[pi] == b'*' { + pi += 1; + } + + pi == pattern.len() +} + +#[cfg(test)] +mod tests { + use super::*; + use manifest::ToolPermissionRule; + + fn summary(tool_name: &str, arguments: Value) -> ToolCallSummary { + ToolCallSummary { + call_id: "call_1".into(), + tool_name: tool_name.into(), + arguments, + } + } + + #[test] + fn first_matching_rule_wins_by_declaration_order() { + let hook = PermissionHook::new(ToolPermissionConfig { + default_action: ToolPermissionAction::Deny, + rules: vec![ + ToolPermissionRule { + tool: "bash".into(), + pattern: "git *".into(), + action: ToolPermissionAction::Allow, + }, + ToolPermissionRule { + tool: "Bash".into(), + pattern: "git reset *".into(), + action: ToolPermissionAction::Deny, + }, + ], + }); + + let input = summary("Bash", serde_json::json!({ "command": "git reset --hard" })); + + assert_eq!(hook.action_for(&input), ToolPermissionAction::Allow); + } + + #[test] + fn default_action_applies_when_no_rule_matches() { + let hook = PermissionHook::new(ToolPermissionConfig { + default_action: ToolPermissionAction::Deny, + rules: Vec::new(), + }); + + let input = summary("Read", serde_json::json!({ "file_path": "/tmp/a.txt" })); + + assert_eq!(hook.action_for(&input), ToolPermissionAction::Deny); + } + + #[test] + fn target_prefers_known_builtin_argument_fields() { + assert_eq!( + permission_target(&serde_json::json!({ "command": "rm -rf target" })), + "rm -rf target" + ); + assert_eq!( + permission_target(&serde_json::json!({ "file_path": "/tmp/.env" })), + "/tmp/.env" + ); + } + + #[test] + fn wildcard_supports_star_and_question() { + assert!(wildcard_match("rm *", "rm -rf target")); + assert!(wildcard_match("file?.rs", "file1.rs")); + assert!(!wildcard_match("rm *", "git status")); + } +} diff --git a/crates/pod/src/pod.rs b/crates/pod/src/pod.rs index 47a253fd..3d277bc1 100644 --- a/crates/pod/src/pod.rs +++ b/crates/pod/src/pod.rs @@ -333,6 +333,7 @@ impl Pod { memory_task: None, user_segments: Vec::new(), }; + pod.apply_permissions_from_manifest(); pod.apply_prune_from_manifest(); Ok(pod) } @@ -2291,6 +2292,7 @@ impl Pod, St> { memory_task: None, user_segments: Vec::new(), }; + pod.apply_permissions_from_manifest(); pod.apply_prune_from_manifest(); drain_skill_shadows(&pod, skill_shadows); Ok(pod) @@ -2361,6 +2363,7 @@ impl Pod, St> { memory_task: None, user_segments: Vec::new(), }; + pod.apply_permissions_from_manifest(); pod.apply_prune_from_manifest(); drain_skill_shadows(&pod, skill_shadows); Ok(pod) @@ -2497,6 +2500,7 @@ impl Pod, St> { memory_task: None, user_segments: state.user_segments, }; + pod.apply_permissions_from_manifest(); pod.apply_prune_from_manifest(); drain_skill_shadows(&pod, skill_shadows); Ok(pod) diff --git a/crates/session-store/src/logged_item.rs b/crates/session-store/src/logged_item.rs index 484003d1..9f1170c3 100644 --- a/crates/session-store/src/logged_item.rs +++ b/crates/session-store/src/logged_item.rs @@ -15,6 +15,10 @@ use llm_worker::llm_client::types::{ContentPart, Item, Role}; use serde::{Deserialize, Serialize}; +fn is_false(value: &bool) -> bool { + !*value +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub enum LoggedItem { @@ -32,6 +36,8 @@ pub enum LoggedItem { summary: String, #[serde(default, skip_serializing_if = "Option::is_none")] content: Option, + #[serde(default, skip_serializing_if = "is_false")] + is_error: bool, }, Reasoning { text: String, @@ -86,11 +92,13 @@ impl From<&Item> for LoggedItem { call_id, summary, content, + is_error, .. } => Self::ToolResult { call_id: call_id.clone(), summary: summary.clone(), content: content.clone(), + is_error: *is_error, }, Item::Reasoning { text, @@ -138,11 +146,13 @@ impl From for Item { call_id, summary, content, + is_error, } => Item::ToolResult { id: None, call_id, summary, content, + is_error, }, LoggedItem::Reasoning { text, @@ -347,6 +357,18 @@ mod tests { } } + #[test] + fn round_trip_tool_result_error_flag() { + let original = Item::tool_result_error("call_1", "permission denied"); + let logged: LoggedItem = (&original).into(); + let value = serde_json::to_value(&logged).unwrap(); + assert_eq!(value["is_error"], true); + match Item::from(logged) { + Item::ToolResult { is_error, .. } => assert!(is_error), + other => panic!("unexpected variant: {other:?}"), + } + } + #[test] fn message_serialization_uses_kind_tag() { let logged: LoggedItem = (&Item::assistant_message("hi")).into(); diff --git a/docs/pod-factory.md b/docs/pod-factory.md index 3b91a139..7f23af8a 100644 --- a/docs/pod-factory.md +++ b/docs/pod-factory.md @@ -34,6 +34,7 @@ overlay をマージして、検証済みの `PodManifest` と `PromptLoader` | 配列スカラー（`worker.stop_sequences` 等） | 上層に値があれば配列ごと置換。追記マージはしない | | マップ（`tool_output.per_tool` 等） | キー単位でマージ、同一キーは上層優先 | | `scope.allow` / `scope.deny` | **union**（各層から全部足す）。上位層は `deny` で下位層の `allow` を必ず削れる | +| `permissions.rule` | **union**（下位層の rule → 上位層の rule の順に評価）。`permissions.default_action` は上位層があれば上書き | 各層をマージした結果（`PodManifestConfig`）を `TryFrom for PodManifest` が必須フィールド検証と絶対パス検証をかけて `PodManifest` @@ -161,6 +162,19 @@ recursive = false target = "/abs/path/to/project/secrets" permission = "write" +[permissions] +default_action = "allow" # allow | deny | ask + +[[permissions.rule]] +tool = "Bash" +pattern = "rm *" +action = "deny" + +[[permissions.rule]] +tool = "Write" +pattern = "*.env" +action = "deny" + [compaction] prune_protected_turns = 3 prune_min_savings = 4096 @@ -201,6 +215,24 @@ scheme 側が吸収する。 生成設定は provider 別の値域検証を行わない。型が TOML と合わない場合は manifest parse error になるが、provider が受け付けない値や組み合わせは API 応答で検出する。 +## `[permissions]` 設定 + +`[permissions]` が無い場合、ツール permission 層は無効で従来通り実行する。`[permissions]` を書く場合は `default_action = "allow" | "deny" | "ask"` が必須で、`[[permissions.rule]]` は宣言順に最初に一致した rule が採用される。一致しなければ `default_action` を使う。 + +```toml +[permissions] +default_action = "allow" + +[[permissions.rule]] +tool = "Bash" +pattern = "rm *" +action = "deny" +``` + +`tool` は実行時に登録されているツール名（`Bash`, `Read`, `Write`, `Edit`, `Glob`, `Grep` 等）に対して大小文字を無視して照合する。`pattern` は built-in tool では主に `command` / `file_path` / `path` / `pattern` 引数に対する `*` / `?` ワイルドカードとして評価される。 + +`allow` は通常実行、`deny` はその tool call を実行せず `is_error = true` の synthetic tool result を履歴へ追加してターンを継続する。`ask` は型として受け付けるが、承認 protocol は未実装のため現在は headless に待機せず fail-closed（synthetic error result）になる。 + ## instruction とプロンプト資産 ### `worker.instruction` フィールド diff --git a/tickets/permission-extension-point.md b/tickets/permission-extension-point.md index 40070c0a..94924769 100644 --- a/tickets/permission-extension-point.md +++ b/tickets/permission-extension-point.md @@ -103,3 +103,8 @@ action = "deny" ## 依存チケット - ~~[remove-hook-module.md](remove-hook-module.md)~~ — 完了。PreToolCall は Pod 層の `hook::Hook` として利用可能 + +## Review +- 状態: Approve with follow-up +- レビュー詳細: [./permission-extension-point.review.md](./permission-extension-point.review.md) +- 日付: 2026-05-05 diff --git a/tickets/permission-extension-point.review.md b/tickets/permission-extension-point.review.md new file mode 100644 index 00000000..21dcfca2 --- /dev/null +++ b/tickets/permission-extension-point.review.md @@ -0,0 +1,27 @@ +# Review: パーミッション: パターンベースのツール実行制御 + +## 前提・要件の確認 +- `[permissions]` が無い場合は無効、ある場合は `default_action` 必須: 満たされている。`PodManifestConfig.permissions` は `Option` で、resolve 時に `permissions.default_action` 不在を `MissingField` にしている (`crates/manifest/src/config.rs:39-42`, `crates/manifest/src/config.rs:430-440`)。 +- `[[permissions.rule]]` と first-match / fallback 評価: 満たされている。rule は `rename = "rule"` で TOML 形に対応し、`PermissionHook::action_for` が宣言順 `find` + `default_action` fallback を行う (`crates/manifest/src/lib.rs:246-276`, `crates/pod/src/permission.rs:25-35`)。 +- built-in `PreToolCall` Hook として、ユーザー Hook より先に評価: 満たされている。Pod 構築直後に `apply_permissions_from_manifest` が呼ばれ、permission hook は通常の user hook 登録前に `HookRegistryBuilder` へ追加される (`crates/pod/src/pod.rs:336-337`, `crates/pod/src/pod.rs:2295-2296`, `crates/pod/src/permission.rs:39-45`)。 +- `allow`: 満たされている。permission action `Allow` は `PreToolAction::Continue` に変換される (`crates/pod/src/permission.rs:49-56`)。 +- `deny` は turn Abort ではなく synthetic error tool result: 満たされている。`PreToolAction::SyntheticResult` が追加され、Worker は tool を実行せず result を commit する (`crates/llm-worker/src/interceptor.rs:48-64`, `crates/llm-worker/src/worker.rs:757-769`, `crates/llm-worker/src/worker.rs:1136-1144`)。 +- provider-visible `is_error` の保持: 満たされている。`ToolResult` / `Item::ToolResult` / session replay に `is_error` が通り、Anthropic request へ `tool_result.is_error` が投影される (`crates/llm-worker/src/tool.rs:281-310`, `crates/llm-worker/src/llm_client/types.rs:74-89`, `crates/session-store/src/logged_item.rs:34-40`, `crates/llm-worker/src/llm_client/scheme/anthropic/request.rs:327-347`)。 +- `ask`: 現時点のスコープとしては満たされている。型として受け付け、承認 protocol 未実装環境では待機せず fail-closed synthetic error result にしている (`crates/pod/src/permission.rs:53-56`, `docs/pod-factory.md:218-235`)。 + +## アーキテクチャ・スコープ +- `llm-worker` には汎用的な `SyntheticResult` action だけを追加し、manifest policy 本体は Pod 層の `permission.rs` に置かれているため、層の分離は保たれている。 +- 新しい trait は追加されておらず、既存の Hook / Interceptor 境界上で実装されている。 +- `Scope` の物理境界には手を入れておらず、permission は tool 実行ポリシーとして分離されている。 +- `ask` の protocol/UI 実装に踏み込まず fail-closed として明示している点は、今回の段階的実装として妥当。 + +## 指摘事項 +### Non-blocking / Follow-up +- mixed allow/deny 時の tool result 順序が tool call 順とずれる可能性がある — synthetic result は `synthetic_results` に集めて実行済み result の末尾へ `extend` されるため、先頭 call が deny で後続 call が allow の場合、履歴上の result 順が逆転する (`crates/llm-worker/src/worker.rs:745-769`, `crates/llm-worker/src/worker.rs:821-823`)。provider が call id で対応を取れる限り致命的ではないが、会話の時系列としては call 順を保つ方が安全。 +- manifest から Pod へ permission が実際に効く統合テストが薄い — manifest resolve と `PermissionHook` 単体、および Worker の synthetic result はカバーされているが、`[permissions]` を含む Pod 構築から tool deny までの結合テストがあると回帰検出しやすい。 + +## 判断 +Approve with follow-up — 要件は満たしており設計上の逸脱もないが、result 順序と Pod 統合テストは後続で固める価値がある。 + +## 確認コマンド +- `cargo test -p pod permission -- --nocapture`