diff --git a/md/design/common-issues.md b/md/design/common-issues.md index 285b8f12..383b2817 100644 --- a/md/design/common-issues.md +++ b/md/design/common-issues.md @@ -4,10 +4,6 @@ The following issues were identified by auditing our hook implementations against the agent reference docs (`md/design/agent-details/`). They don't cause crashes (the fallback path handles events without agent-specific handlers) but mean some features are incomplete. -### `toolArgs` not parsed (Copilot) - -Copilot sends `toolArgs` as a JSON *string* (not an object). Our `CopilotPreToolUsePayload` declares it as `serde_json::Value` and passes it through as-is in `to_hook_payload()`. Downstream code expecting structured tool args will get a raw string. Should parse the JSON string into a `Value` during conversion. - ### `permissionDecision` dropped (Copilot) `CopilotPreToolUseOutput::from_hook_output()` never maps `permissionDecision` or `permissionDecisionReason` from the builtin hook output. If a builtin handler wants to deny a tool call, the decision is silently lost in Copilot output. diff --git a/src/hook_schema/copilot.rs b/src/hook_schema/copilot.rs index 547504be..fef3c9f3 100644 --- a/src/hook_schema/copilot.rs +++ b/src/hook_schema/copilot.rs @@ -4,6 +4,28 @@ use crate::hook_schema::{ Agent, AgentHookEvent, AgentHookInput, AgentHookOutput, erase_agent_hook_event, symposium, }; +/// Copilot sends `toolArgs` as a JSON-encoded string (per Copilot's hook protocol), +/// while the canonical `PreToolUseInput.tool_input` is a structured `Value`. Parse +/// the string into a `Value` when crossing into canonical form. If parsing fails +/// or the field is already structured, pass it through unchanged. +fn parse_copilot_tool_args(raw: &serde_json::Value) -> serde_json::Value { + if let Some(s) = raw.as_str() + && let Ok(v) = serde_json::from_str::(s) + { + return v; + } + raw.clone() +} + +/// Inverse of [`parse_copilot_tool_args`]: when writing out to Copilot wire format, +/// re-encode a structured `Value` back into a JSON string. +fn stringify_for_copilot(v: &serde_json::Value) -> serde_json::Value { + match v { + serde_json::Value::String(_) => v.clone(), + _ => serde_json::Value::String(v.to_string()), + } +} + pub struct Copilot; impl Agent for Copilot { fn event(&self, event: super::HookEvent) -> Option> { @@ -117,7 +139,7 @@ impl AgentHookInput for CopilotPreToolUseInput { fn to_symposium(&self) -> symposium::InputEvent { symposium::InputEvent::PreToolUse(symposium::PreToolUseInput { tool_name: self.tool_name.clone(), - tool_input: self.tool_args.clone(), + tool_input: parse_copilot_tool_args(&self.tool_args), session_id: None, cwd: self.cwd.clone(), }) @@ -130,7 +152,7 @@ impl AgentHookInput for CopilotPreToolUseInput { timestamp: None, cwd: p.cwd.clone(), tool_name: p.tool_name.clone(), - tool_args: p.tool_input.clone(), + tool_args: stringify_for_copilot(&p.tool_input), rest: serde_json::Map::new(), } } @@ -183,7 +205,7 @@ impl AgentHookInput for CopilotPostToolUseInput { fn to_symposium(&self) -> symposium::InputEvent { symposium::InputEvent::PostToolUse(symposium::PostToolUseInput { tool_name: self.tool_name.clone(), - tool_input: self.tool_args.clone(), + tool_input: parse_copilot_tool_args(&self.tool_args), tool_response: self.tool_response.clone(), session_id: None, cwd: self.cwd.clone(), @@ -197,7 +219,7 @@ impl AgentHookInput for CopilotPostToolUseInput { timestamp: None, cwd: p.cwd.clone(), tool_name: p.tool_name.clone(), - tool_args: p.tool_input.clone(), + tool_args: stringify_for_copilot(&p.tool_input), tool_response: p.tool_response.clone(), rest: serde_json::Map::new(), } @@ -323,3 +345,63 @@ copilot_output_impl!( SessionStart, SessionStartOutput {} ); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pre_tool_use_parses_string_tool_args() { + let raw = r#"{"toolName":"bash","toolArgs":"{\"command\":\"ls\"}"}"#; + let input: CopilotPreToolUseInput = serde_json::from_str(raw).unwrap(); + let symposium::InputEvent::PreToolUse(canon) = input.to_symposium() else { + panic!("wrong event type") + }; + assert_eq!(canon.tool_input, serde_json::json!({"command": "ls"})); + } + + #[test] + fn pre_tool_use_round_trips_object_to_copilot_string() { + let canon = symposium::InputEvent::PreToolUse(symposium::PreToolUseInput { + tool_name: "bash".into(), + tool_input: serde_json::json!({"command": "ls"}), + session_id: None, + cwd: None, + }); + let out = CopilotPreToolUseInput::from_symposium(&canon); + let s = out + .tool_args + .as_str() + .expect("toolArgs must be a JSON string on the Copilot wire"); + let reparsed: serde_json::Value = serde_json::from_str(s).unwrap(); + assert_eq!(reparsed, serde_json::json!({"command": "ls"})); + } + + #[test] + fn post_tool_use_parses_string_tool_args() { + let raw = r#"{"toolName":"bash","toolArgs":"{\"command\":\"ls\"}","toolResponse":{}}"#; + let input: CopilotPostToolUseInput = serde_json::from_str(raw).unwrap(); + let symposium::InputEvent::PostToolUse(canon) = input.to_symposium() else { + panic!("wrong event type") + }; + assert_eq!(canon.tool_input, serde_json::json!({"command": "ls"})); + } + + #[test] + fn post_tool_use_round_trips_object_to_copilot_string() { + let canon = symposium::InputEvent::PostToolUse(symposium::PostToolUseInput { + tool_name: "bash".into(), + tool_input: serde_json::json!({"command": "ls"}), + tool_response: serde_json::json!({"exit_code": 0}), + session_id: None, + cwd: None, + }); + let out = CopilotPostToolUseInput::from_symposium(&canon); + let s = out + .tool_args + .as_str() + .expect("toolArgs must be a JSON string on the Copilot wire"); + let reparsed: serde_json::Value = serde_json::from_str(s).unwrap(); + assert_eq!(reparsed, serde_json::json!({"command": "ls"})); + } +}