diff --git a/src/agent/loop_.rs b/src/agent/loop_.rs index 7432b95cb..af3b81c15 100644 --- a/src/agent/loop_.rs +++ b/src/agent/loop_.rs @@ -1167,7 +1167,7 @@ pub(crate) async fn run_tool_call_loop( /// Append the hardware device summary to the system prompt if non-trivial. fn append_hw_summary(system_prompt: &mut String, hw_device_summary: &str) { if !hw_device_summary.is_empty() - && hw_device_summary != "No hardware devices connected." + && hw_device_summary != crate::hardware::NO_HW_DEVICES_SUMMARY { system_prompt.push_str("\n## Connected Hardware Devices\n\n"); system_prompt.push_str(hw_device_summary); @@ -1879,6 +1879,54 @@ pub async fn process_message(config: Config, message: &str) -> Result { mod tests { use super::*; + #[test] + fn parse_shorthand_valid_object_returns_call() { + let result = parse_shorthand_tag_call(r#"shell{"command":"ls -la"}"#); + let call = result.expect("should parse valid shorthand"); + assert_eq!(call.name, "shell"); + assert_eq!( + call.arguments.get("command").and_then(|v| v.as_str()), + Some("ls -la") + ); + } + + #[test] + fn parse_shorthand_valid_with_trailing_junk_still_parses() { + // Trailing content after the closing '}' must not prevent parsing. + let result = parse_shorthand_tag_call(r#"my_tool{"key":true} extra junk"#); + let call = result.expect("should parse despite trailing junk"); + assert_eq!(call.name, "my_tool"); + assert_eq!(call.arguments["key"], serde_json::json!(true)); + } + + #[test] + fn parse_shorthand_identifier_starts_with_digit_returns_none() { + assert!(parse_shorthand_tag_call(r#"1bad{"k":1}"#).is_none()); + } + + #[test] + fn parse_shorthand_identifier_with_disallowed_chars_returns_none() { + // Hyphens are not allowed in the identifier. + assert!(parse_shorthand_tag_call(r#"bad-name{"k":1}"#).is_none()); + } + + #[test] + fn parse_shorthand_array_value_returns_none() { + // JSON after identifier is an array, not an object. + assert!(parse_shorthand_tag_call(r#"tool[1,2,3]"#).is_none()); + } + + #[test] + fn parse_shorthand_literal_value_returns_none() { + // JSON after identifier is a scalar literal, not an object. + assert!(parse_shorthand_tag_call(r#"tool"string""#).is_none()); + } + + #[test] + fn parse_shorthand_empty_identifier_returns_none() { + assert!(parse_shorthand_tag_call(r#"{"k":1}"#).is_none()); + } + #[test] fn test_scrub_credentials() { let input = "API_KEY=sk-1234567890abcdef; token: 1234567890; password=\"secret123456\""; diff --git a/src/channels/mod.rs b/src/channels/mod.rs index 3a418ab83..877224d09 100644 --- a/src/channels/mod.rs +++ b/src/channels/mod.rs @@ -1348,7 +1348,7 @@ pub async fn start_channels(config: Config) -> Result<()> { // Inject hardware device summary if available if !hw_device_summary.is_empty() - && hw_device_summary != "No hardware devices connected." + && hw_device_summary != crate::hardware::NO_HW_DEVICES_SUMMARY { system_prompt.push_str("\n## Connected Hardware Devices\n\n"); system_prompt.push_str(&hw_device_summary); diff --git a/src/firmware/pico/main.py b/src/firmware/pico/main.py index 0ea53362b..7b9b4e8b7 100644 --- a/src/firmware/pico/main.py +++ b/src/firmware/pico/main.py @@ -4,6 +4,13 @@ # Wire protocol: # Host → Device: {"cmd":"gpio_write","params":{"pin":25,"value":1}}\n # Device → Host: {"ok":true,"data":{"pin":25,"value":1,"state":"HIGH"}}\n +# +# Pin direction policy: +# gpio_write always configures the pin as OUTPUT and caches it. +# gpio_read uses the cached Pin object if one already exists, so a pin +# that was set via gpio_write retains its OUTPUT direction — it is NOT +# reconfigured to INPUT. If no cached Pin exists the pin is opened as +# INPUT and the new Pin is cached for subsequent reads. import sys import json @@ -12,6 +19,11 @@ from machine import Pin # Onboard LED — GPIO 25 on Pico 1 led = Pin(25, Pin.OUT) +# Cache of Pin objects keyed by pin number (excludes the onboard LED on 25). +# gpio_write stores pins as OUTPUT; gpio_read reuses the existing Pin if one +# is cached rather than clobbering its direction. +pins_cache = {} + def handle(msg): cmd = msg.get("cmd") params = msg.get("params", {}) @@ -40,7 +52,10 @@ def handle(msg): if pin_num == 25: led.value(value) else: - Pin(pin_num, Pin.OUT).value(value) + # Always (re-)configure as OUT so subsequent reads on this pin + # reflect the driven state rather than clobbering the direction. + pins_cache[pin_num] = Pin(pin_num, Pin.OUT) + pins_cache[pin_num].value(value) state = "HIGH" if value == 1 else "LOW" return {"ok": True, "data": {"pin": pin_num, "value": value, "state": state}} @@ -55,7 +70,10 @@ def handle(msg): return {"ok": False, "error": "invalid pin"} if pin_num < 0: return {"ok": False, "error": "invalid pin"} - value = led.value() if pin_num == 25 else Pin(pin_num, Pin.IN).value() + value = led.value() if pin_num == 25 else ( + pins_cache[pin_num].value() if pin_num in pins_cache + else pins_cache.setdefault(pin_num, Pin(pin_num, Pin.IN)).value() + ) state = "HIGH" if value == 1 else "LOW" return {"ok": True, "data": {"pin": pin_num, "value": value, "state": state}} @@ -70,5 +88,9 @@ while True: msg = json.loads(line) result = handle(msg) print(json.dumps(result)) - except Exception as e: + except (ValueError, KeyError, TypeError) as e: + # ValueError — json.loads() on malformed input + # KeyError — unexpected missing key in a message dict + # TypeError — wrong type in an operation + # Any other exception propagates so bugs surface during development. print(json.dumps({"ok": False, "error": str(e)})) diff --git a/src/hardware/device.rs b/src/hardware/device.rs index d826c7cd7..205a0eb1e 100644 --- a/src/hardware/device.rs +++ b/src/hardware/device.rs @@ -164,6 +164,11 @@ struct RegisteredDevice { capabilities: DeviceCapabilities, } +/// Summary string returned by [`DeviceRegistry::prompt_summary`] when no +/// devices are registered. Exported so callers can compare against it without +/// duplicating the literal. +pub const NO_HW_DEVICES_SUMMARY: &str = "No hardware devices connected."; + /// Registry of discovered devices with stable session aliases. /// /// - Scans at startup (via `hardware::discover`) @@ -230,15 +235,21 @@ impl DeviceRegistry { } /// Attach a transport and capabilities to a previously registered device. + /// + /// Returns `Err` when `alias` is not found in the registry (should not + /// happen in normal usage because callers pass aliases from `register`). pub fn attach_transport( &mut self, alias: &str, transport: Arc, capabilities: DeviceCapabilities, - ) { + ) -> anyhow::Result<()> { if let Some(entry) = self.devices.get_mut(alias) { entry.transport = Some(transport); entry.capabilities = capabilities; + Ok(()) + } else { + Err(anyhow::anyhow!("unknown device alias: {}", alias)) } } @@ -268,7 +279,7 @@ impl DeviceRegistry { /// Return a summary of connected devices for the LLM system prompt. pub fn prompt_summary(&self) -> String { if self.devices.is_empty() { - return "No hardware devices connected.".to_string(); + return NO_HW_DEVICES_SUMMARY.to_string(); } let mut lines = vec!["Connected devices:".to_string()]; @@ -467,7 +478,8 @@ impl DeviceRegistry { gpio: true, // assume GPIO; Phase 3 will populate via capabilities handshake ..DeviceCapabilities::default() }; - registry.attach_transport(&alias, transport, caps); + registry.attach_transport(&alias, transport, caps) + .unwrap_or_else(|e| tracing::warn!(alias = %alias, err = %e, "attach_transport: unexpected unknown alias")); tracing::info!( alias = %alias, @@ -648,7 +660,7 @@ mod tests { #[test] fn registry_prompt_summary_empty() { let reg = DeviceRegistry::new(); - assert_eq!(reg.prompt_summary(), "No hardware devices connected."); + assert_eq!(reg.prompt_summary(), NO_HW_DEVICES_SUMMARY); } #[test] diff --git a/src/hardware/gpio.rs b/src/hardware/gpio.rs index 8d40e9713..4996f6fc9 100644 --- a/src/hardware/gpio.rs +++ b/src/hardware/gpio.rs @@ -339,7 +339,8 @@ mod tests { gpio: true, ..Default::default() }, - ); + ) + .expect("alias was just registered"); Arc::new(RwLock::new(reg)) } diff --git a/src/hardware/mod.rs b/src/hardware/mod.rs index cdf64ceb8..c4d519ac7 100644 --- a/src/hardware/mod.rs +++ b/src/hardware/mod.rs @@ -33,7 +33,7 @@ pub mod manifest; pub mod subprocess; pub mod tool_registry; -pub use device::{Device, DeviceCapabilities, DeviceContext, DeviceKind, DeviceRegistry, DeviceRuntime}; +pub use device::{Device, DeviceCapabilities, DeviceContext, DeviceKind, DeviceRegistry, DeviceRuntime, NO_HW_DEVICES_SUMMARY}; pub use gpio::{gpio_tools, GpioReadTool, GpioWriteTool}; #[cfg(feature = "hardware")] pub use pico_code::{device_code_tools, DeviceExecTool, DeviceReadCodeTool, DeviceWriteCodeTool}; @@ -135,7 +135,8 @@ pub async fn boot(peripherals: &crate::config::PeripheralsConfig) -> anyhow::Res HardwareSerialTransport::new(&path, board.baud), ) as std::sync::Arc; let caps = DeviceCapabilities { gpio: true, ..DeviceCapabilities::default() }; - registry_inner.attach_transport(&alias, transport, caps); + registry_inner.attach_transport(&alias, transport, caps) + .unwrap_or_else(|e| tracing::warn!(alias = %alias, err = %e, "attach_transport: unexpected unknown alias")); // Mark path as registered so duplicate config entries are skipped. discovered_paths.insert(path.clone()); tracing::info!( diff --git a/src/hardware/subprocess.rs b/src/hardware/subprocess.rs index 88b690122..80d4e544b 100644 --- a/src/hardware/subprocess.rs +++ b/src/hardware/subprocess.rs @@ -177,47 +177,57 @@ impl Tool for SubprocessTool { } }; - // Kill child, wait for exit status, and collect stderr — done before - // the match so all error branches can include them. - let _ = child.kill().await; - let child_status = child.wait().await.ok(); - let stderr_msg = collect_stderr(stderr_handle).await; - match read_result { // ── Timeout ──────────────────────────────────────────────────── - Err(_elapsed) => Ok(ToolResult { - success: false, - output: String::new(), - error: Some(format!( - "plugin '{}' timed out after {}s{}", - self.manifest.tool.name, - SUBPROCESS_TIMEOUT_SECS, - if stderr_msg.is_empty() { - String::new() - } else { - format!("; stderr: {}", stderr_msg) - } - )), - }), + // The read deadline elapsed — force-kill the plugin and collect + // any stderr it emitted before dying. + Err(_elapsed) => { + let _ = child.kill().await; + let _ = child.wait().await; + let stderr_msg = collect_stderr(stderr_handle).await; + Ok(ToolResult { + success: false, + output: String::new(), + error: Some(format!( + "plugin '{}' timed out after {}s{}", + self.manifest.tool.name, + SUBPROCESS_TIMEOUT_SECS, + if stderr_msg.is_empty() { + String::new() + } else { + format!("; stderr: {}", stderr_msg) + } + )), + }) + } // ── I/O error reading stdout ─────────────────────────────────── - Ok(Err(io_err)) => Ok(ToolResult { - success: false, - output: String::new(), - error: Some(format!( - "plugin '{}': I/O error reading stdout: {}{}", - self.manifest.tool.name, - io_err, - if stderr_msg.is_empty() { - String::new() - } else { - format!("; stderr: {}", stderr_msg) - } - )), - }), + Ok(Err(io_err)) => { + let _ = child.kill().await; + let _ = child.wait().await; + let stderr_msg = collect_stderr(stderr_handle).await; + Ok(ToolResult { + success: false, + output: String::new(), + error: Some(format!( + "plugin '{}': I/O error reading stdout: {}{}", + self.manifest.tool.name, + io_err, + if stderr_msg.is_empty() { + String::new() + } else { + format!("; stderr: {}", stderr_msg) + } + )), + }) + } // ── Got a line ──────────────────────────────────────────────── + // Let the process finish naturally — plugins that write their + // result and then do cleanup should not be interrupted. Ok(Ok(line)) => { + let child_status = child.wait().await.ok(); + let stderr_msg = collect_stderr(stderr_handle).await; let line = line.trim(); if line.is_empty() { diff --git a/src/peripherals/serial.rs b/src/peripherals/serial.rs index b3b64373d..81fd597ae 100644 --- a/src/peripherals/serial.rs +++ b/src/peripherals/serial.rs @@ -285,10 +285,14 @@ impl Tool for GpioWriteTool { .and_then(|v| v.as_u64()) .ok_or_else(|| anyhow::anyhow!("Missing 'value' parameter"))?; if value != 0 && value != 1 { - return Err(anyhow::anyhow!( - "Invalid 'value' parameter: expected 0 or 1, got {}", - value - )); + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some(format!( + "Invalid 'value' parameter: expected 0 or 1, got {}", + value + )), + }); } self.transport .request("gpio_write", json!({ "pin": pin, "value": value })) diff --git a/src/util.rs b/src/util.rs index ddaa8e1cb..0358ba429 100644 --- a/src/util.rs +++ b/src/util.rs @@ -204,8 +204,9 @@ pub fn is_serial_path_allowed(path: &str) -> bool { } // ── Fallback (other platforms) ──────────────────────────────────────── + // Reject unknown platforms rather than applying a too-permissive prefix + // match. If support is needed for a new platform, add a targeted branch + // above. #[allow(unreachable_code)] - ALLOWED_SERIAL_PATH_PREFIXES - .iter() - .any(|p| path.starts_with(p)) + false }