From 0f7ecb066a0361414e3bd8c3e752c4f00b1087ce Mon Sep 17 00:00:00 2001 From: Hayato Kawai Date: Wed, 11 Feb 2026 01:33:03 +0900 Subject: [PATCH 1/5] feat(exec): implement JSON-RPC 2.0 extension runner for plugin communication Extensions allow users to write custom command validators in any language (Ruby, Python, Deno, etc.) as external processes. The runner spawns the extension process, sends an ExtensionRequest via stdin in JSON-RPC 2.0 format, and parses the ExtensionResponse from stdout. - ExtensionRunner trait with validate() method - ProcessExtensionRunner: spawns external process, communicates via stdio - JSON-RPC 2.0 request serialization (method: "validateCommand") - JSON-RPC 2.0 response deserialization with error handling - Configurable timeout (kills child process on expiry) - ExtensionError variants: Spawn, Timeout, InvalidResponse Co-Authored-By: Claude Opus 4.6 --- Cargo.lock | 8 +- Cargo.toml | 1 + src/exec/extension_runner.rs | 510 +++++++++++++++++++++++++++++++++++ src/exec/mod.rs | 1 + 4 files changed, 516 insertions(+), 4 deletions(-) create mode 100644 src/exec/extension_runner.rs diff --git a/Cargo.lock b/Cargo.lock index 487f6a0..1576cf3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1049,6 +1049,7 @@ dependencies = [ "rstest", "serde", "serde-saphyr", + "serde_json", "thiserror 2.0.18", "tree-sitter", "tree-sitter-bash", @@ -1174,16 +1175,15 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.149" +version = "1.0.140" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83fc039473c5595ace860d8c4fafa220ff474b3fc6bfdb4293327f1a37e94d86" +checksum = "20068b6e96dc6c9bd23e01df8827e6c7e1f2fddd43c21810382803c136b99373" dependencies = [ "indexmap", "itoa", "memchr", + "ryu", "serde", - "serde_core", - "zmij", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index e8078a0..76f332e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ cel-interpreter = "=0.10.0" clap = { version = "=4.5.57", features = ["derive"] } serde = { version = "=1.0.228", features = ["derive"] } serde-saphyr = "=0.0.17" +serde_json = "=1.0.140" thiserror = "=2.0.18" tree-sitter = "=0.25.10" tree-sitter-bash = "=0.25.1" diff --git a/src/exec/extension_runner.rs b/src/exec/extension_runner.rs new file mode 100644 index 0000000..76e3470 --- /dev/null +++ b/src/exec/extension_runner.rs @@ -0,0 +1,510 @@ +use std::collections::HashMap; +use std::io::Write; +use std::process::{Command, Stdio}; +use std::time::Duration; + +use serde::{Deserialize, Serialize}; + +use super::ExtensionError; + +/// JSON-RPC 2.0 request sent to an extension process via stdin. +#[derive(Debug, Clone, Serialize)] +pub struct ExtensionRequest { + pub command: String, + pub flags: HashMap, + pub args: Vec, + pub raw_command_line: String, + pub env: HashMap, + pub cwd: String, +} + +/// The result payload inside a JSON-RPC 2.0 response from an extension. +#[derive(Debug, Clone, Deserialize, PartialEq)] +pub struct ExtensionResponse { + pub status: String, + pub message: Option, + pub fix_suggestion: Option, +} + +/// JSON-RPC 2.0 envelope for responses (internal parsing helper). +#[derive(Debug, Deserialize)] +struct JsonRpcResponse { + #[expect( + dead_code, + reason = "included for JSON-RPC 2.0 compliance; validated at protocol level" + )] + jsonrpc: Option, + #[expect( + dead_code, + reason = "included for JSON-RPC 2.0 compliance; validated at protocol level" + )] + id: Option, + result: Option, + error: Option, +} + +/// JSON-RPC 2.0 error object. +#[derive(Debug, Deserialize)] +struct JsonRpcError { + #[expect( + dead_code, + reason = "error code is part of JSON-RPC 2.0 spec but not used in error reporting" + )] + code: Option, + message: Option, +} + +/// Trait for running extension validation via JSON-RPC 2.0 over stdio. +pub trait ExtensionRunner { + fn validate( + &self, + executor_cmd: &str, + request: &ExtensionRequest, + timeout: Duration, + ) -> Result; +} + +/// Default implementation that spawns an external process. +pub struct ProcessExtensionRunner; + +impl ProcessExtensionRunner { + /// Build a JSON-RPC 2.0 request string from an ExtensionRequest. + pub fn build_jsonrpc_request(&self, request: &ExtensionRequest) -> String { + let envelope = serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "validateCommand", + "params": request, + }); + serde_json::to_string(&envelope).unwrap_or_default() + } + + /// Parse a JSON-RPC 2.0 response string into an ExtensionResponse. + pub fn parse_jsonrpc_response(raw: &str) -> Result { + let envelope: JsonRpcResponse = serde_json::from_str(raw) + .map_err(|e| ExtensionError::InvalidResponse(format!("JSON parse error: {e}")))?; + + if let Some(err) = envelope.error { + let msg = err.message.unwrap_or_else(|| "unknown error".to_string()); + return Err(ExtensionError::InvalidResponse(format!( + "JSON-RPC error: {msg}" + ))); + } + + envelope.result.ok_or_else(|| { + ExtensionError::InvalidResponse( + "missing 'result' field in JSON-RPC response".to_string(), + ) + }) + } + + /// Split executor_cmd into program and arguments for spawning. + /// Uses simple whitespace splitting, as executor commands are typically + /// straightforward (e.g., "deno run --allow-net ./checks/url_check.ts"). + fn parse_command(executor_cmd: &str) -> (String, Vec) { + let mut parts = executor_cmd.split_whitespace(); + let program = parts.next().unwrap_or("").to_string(); + let args: Vec = parts.map(String::from).collect(); + (program, args) + } +} + +impl ExtensionRunner for ProcessExtensionRunner { + fn validate( + &self, + executor_cmd: &str, + request: &ExtensionRequest, + timeout: Duration, + ) -> Result { + let (program, args) = Self::parse_command(executor_cmd); + + let mut child = Command::new(&program) + .args(&args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn()?; + + // Write JSON-RPC request to stdin, then close the pipe + let json_request = self.build_jsonrpc_request(request); + if let Some(mut stdin) = child.stdin.take() { + let _ = stdin.write_all(json_request.as_bytes()); + // stdin is dropped here, closing the pipe + } + + // Wait with timeout using a polling loop + let start = std::time::Instant::now(); + loop { + match child.try_wait() { + Ok(Some(_status)) => { + let output = child.wait_with_output()?; + let stdout = String::from_utf8_lossy(&output.stdout); + return Self::parse_jsonrpc_response(&stdout); + } + Ok(None) => { + if start.elapsed() >= timeout { + let _ = child.kill(); + let _ = child.wait(); + return Err(ExtensionError::Timeout(timeout)); + } + std::thread::sleep(Duration::from_millis(10)); + } + Err(e) => return Err(ExtensionError::Spawn(e)), + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + use std::collections::HashMap; + + fn sample_request() -> ExtensionRequest { + ExtensionRequest { + command: "curl".to_string(), + flags: HashMap::from([("X".to_string(), "POST".to_string())]), + args: vec!["https://api.example.com".to_string()], + raw_command_line: "curl -X POST https://api.example.com".to_string(), + env: HashMap::from([("AWS_PROFILE".to_string(), "prod".to_string())]), + cwd: "/home/user/project".to_string(), + } + } + + /// Create a temporary script file that drains stdin and prints the given response. + /// Returns the path to the script. Uses a unique counter to avoid conflicts + /// between parallel test runs. + fn write_test_script(response: &str) -> std::path::PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + + let dir = std::env::temp_dir().join("runok-test-extension"); + std::fs::create_dir_all(&dir).expect("should create temp dir"); + let id = COUNTER.fetch_add(1, Ordering::Relaxed); + let path = dir.join(format!("ext-{}-{}.sh", std::process::id(), id)); + std::fs::write( + &path, + format!("#!/bin/sh\ncat > /dev/null\nprintf '%s' '{}'\n", response), + ) + .expect("should write test script"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .expect("should set executable permission"); + } + path + } + + // === Serialization tests === + + #[test] + fn serialize_jsonrpc_request() { + let runner = ProcessExtensionRunner; + let request = sample_request(); + let json_str = runner.build_jsonrpc_request(&request); + + let parsed: serde_json::Value = + serde_json::from_str(&json_str).expect("should be valid JSON"); + assert_eq!(parsed["jsonrpc"], "2.0"); + assert_eq!(parsed["id"], 1); + assert_eq!(parsed["method"], "validateCommand"); + assert_eq!(parsed["params"]["command"], "curl"); + assert_eq!(parsed["params"]["flags"]["X"], "POST"); + assert_eq!(parsed["params"]["args"][0], "https://api.example.com"); + assert_eq!( + parsed["params"]["raw_command_line"], + "curl -X POST https://api.example.com" + ); + assert_eq!(parsed["params"]["env"]["AWS_PROFILE"], "prod"); + assert_eq!(parsed["params"]["cwd"], "/home/user/project"); + } + + #[test] + fn serialize_request_with_empty_fields() { + let request = ExtensionRequest { + command: "test".to_string(), + flags: HashMap::new(), + args: vec![], + raw_command_line: "test".to_string(), + env: HashMap::new(), + cwd: ".".to_string(), + }; + let runner = ProcessExtensionRunner; + let json_str = runner.build_jsonrpc_request(&request); + + let parsed: serde_json::Value = + serde_json::from_str(&json_str).expect("should be valid JSON"); + assert_eq!(parsed["params"]["command"], "test"); + assert!( + parsed["params"]["flags"] + .as_object() + .expect("flags should be an object") + .is_empty() + ); + assert!( + parsed["params"]["args"] + .as_array() + .expect("args should be an array") + .is_empty() + ); + } + + // === Response deserialization tests === + + #[test] + fn deserialize_valid_deny_response() { + let json = r#"{ + "jsonrpc": "2.0", + "id": 1, + "result": { + "status": "deny", + "message": "POST requests are not allowed", + "fix_suggestion": "curl -X GET https://api.example.com" + } + }"#; + + let response = ProcessExtensionRunner::parse_jsonrpc_response(json) + .expect("should parse valid deny response"); + assert_eq!(response.status, "deny"); + assert_eq!( + response.message.as_deref(), + Some("POST requests are not allowed") + ); + assert_eq!( + response.fix_suggestion.as_deref(), + Some("curl -X GET https://api.example.com") + ); + } + + #[test] + fn deserialize_valid_allow_response() { + let json = r#"{ + "jsonrpc": "2.0", + "id": 1, + "result": { + "status": "allow" + } + }"#; + + let response = ProcessExtensionRunner::parse_jsonrpc_response(json) + .expect("should parse valid allow response"); + assert_eq!(response.status, "allow"); + assert_eq!(response.message, None); + assert_eq!(response.fix_suggestion, None); + } + + #[test] + fn deserialize_valid_ask_response() { + let json = r#"{ + "jsonrpc": "2.0", + "id": 1, + "result": { + "status": "ask", + "message": "This looks risky, please confirm" + } + }"#; + + let response = ProcessExtensionRunner::parse_jsonrpc_response(json) + .expect("should parse valid ask response"); + assert_eq!(response.status, "ask"); + assert_eq!( + response.message.as_deref(), + Some("This looks risky, please confirm") + ); + } + + #[rstest] + #[case("allow")] + #[case("deny")] + #[case("ask")] + fn valid_status_values(#[case] status: &str) { + let json = format!(r#"{{"jsonrpc": "2.0", "id": 1, "result": {{"status": "{status}"}}}}"#); + let response = ProcessExtensionRunner::parse_jsonrpc_response(&json) + .expect("should parse response with valid status"); + assert_eq!(response.status, status); + } + + // === Error cases: parsing === + + #[test] + fn invalid_json_response() { + let result = ProcessExtensionRunner::parse_jsonrpc_response("not json at all"); + let err = result.expect_err("should fail on invalid JSON"); + match err { + ExtensionError::InvalidResponse(msg) => { + assert_eq!(msg, "JSON parse error: expected ident at line 1 column 2"); + } + other => panic!("expected InvalidResponse, got: {other:?}"), + } + } + + #[test] + fn missing_result_field() { + let json = r#"{"jsonrpc": "2.0", "id": 1}"#; + let err = ProcessExtensionRunner::parse_jsonrpc_response(json) + .expect_err("should fail when result is missing"); + match err { + ExtensionError::InvalidResponse(msg) => { + assert_eq!(msg, "missing 'result' field in JSON-RPC response"); + } + other => panic!("expected InvalidResponse, got: {other:?}"), + } + } + + #[test] + fn missing_status_in_result() { + let json = r#"{"jsonrpc": "2.0", "id": 1, "result": {"message": "hi"}}"#; + let err = ProcessExtensionRunner::parse_jsonrpc_response(json) + .expect_err("should fail when status is missing"); + match err { + ExtensionError::InvalidResponse(msg) => { + assert_eq!( + msg, + "JSON parse error: missing field `status` at line 1 column 55" + ); + } + other => panic!("expected InvalidResponse, got: {other:?}"), + } + } + + #[test] + fn jsonrpc_error_response() { + let json = r#"{ + "jsonrpc": "2.0", + "id": 1, + "error": { + "code": -32600, + "message": "Invalid request" + } + }"#; + let err = ProcessExtensionRunner::parse_jsonrpc_response(json) + .expect_err("should fail on JSON-RPC error response"); + match err { + ExtensionError::InvalidResponse(msg) => { + assert_eq!(msg, "JSON-RPC error: Invalid request"); + } + other => panic!("expected InvalidResponse, got: {other:?}"), + } + } + + // === End-to-end with real process === + + #[test] + fn validate_with_real_process_deny_response() { + let runner = ProcessExtensionRunner; + let request = sample_request(); + + let response_json = + r#"{"jsonrpc":"2.0","id":1,"result":{"status":"deny","message":"blocked"}}"#; + let script = write_test_script(response_json); + let cmd = script.display().to_string(); + + let response = runner + .validate(&cmd, &request, Duration::from_secs(5)) + .expect("should get deny response from process"); + assert_eq!(response.status, "deny"); + assert_eq!(response.message.as_deref(), Some("blocked")); + } + + #[test] + fn validate_with_real_process_allow_response() { + let runner = ProcessExtensionRunner; + let request = sample_request(); + + let response_json = r#"{"jsonrpc":"2.0","id":1,"result":{"status":"allow"}}"#; + let script = write_test_script(response_json); + let cmd = script.display().to_string(); + + let response = runner + .validate(&cmd, &request, Duration::from_secs(5)) + .expect("should get allow response from process"); + assert_eq!(response.status, "allow"); + assert_eq!(response.message, None); + } + + #[test] + fn validate_spawn_error_for_nonexistent_command() { + let runner = ProcessExtensionRunner; + let request = sample_request(); + + let err = runner + .validate("/nonexistent/binary/path", &request, Duration::from_secs(5)) + .expect_err("should fail for nonexistent binary"); + match err { + ExtensionError::Spawn(_) => {} + other => panic!("expected Spawn error, got: {other:?}"), + } + } + + #[test] + fn validate_invalid_json_from_process() { + let runner = ProcessExtensionRunner; + let request = sample_request(); + + let script = write_test_script("not-valid-json"); + let cmd = script.display().to_string(); + + let err = runner + .validate(&cmd, &request, Duration::from_secs(5)) + .expect_err("should fail on invalid JSON from process"); + match err { + ExtensionError::InvalidResponse(_) => {} + other => panic!("expected InvalidResponse, got: {other:?}"), + } + } + + #[test] + fn validate_timeout() { + let runner = ProcessExtensionRunner; + let request = sample_request(); + + // Use a script that sleeps longer than the timeout + let dir = std::env::temp_dir().join("runok-test-extension"); + std::fs::create_dir_all(&dir).expect("should create temp dir"); + let path = dir.join(format!("ext-timeout-{}.sh", std::process::id())); + std::fs::write(&path, "#!/bin/sh\nsleep 10\n").expect("should write timeout script"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .expect("should set executable permission"); + } + let cmd = path.display().to_string(); + + let err = runner + .validate(&cmd, &request, Duration::from_secs(1)) + .expect_err("should timeout for long-running process"); + match err { + ExtensionError::Timeout(d) => { + assert_eq!(d, Duration::from_secs(1)); + } + other => panic!("expected Timeout error, got: {other:?}"), + } + } + + // === parse_command tests === + + #[test] + fn parse_command_simple() { + let (prog, args) = ProcessExtensionRunner::parse_command("deno run check.ts"); + assert_eq!(prog, "deno"); + assert_eq!(args, vec!["run", "check.ts"]); + } + + #[test] + fn parse_command_no_args() { + let (prog, args) = ProcessExtensionRunner::parse_command("/usr/bin/validator"); + assert_eq!(prog, "/usr/bin/validator"); + assert!(args.is_empty()); + } + + #[test] + fn parse_command_with_flags() { + let (prog, args) = + ProcessExtensionRunner::parse_command("deno run --allow-net ./checks/url_check.ts"); + assert_eq!(prog, "deno"); + assert_eq!(args, vec!["run", "--allow-net", "./checks/url_check.ts"]); + } +} diff --git a/src/exec/mod.rs b/src/exec/mod.rs index 3925f93..28d7fd9 100644 --- a/src/exec/mod.rs +++ b/src/exec/mod.rs @@ -1,3 +1,4 @@ mod error; +pub mod extension_runner; pub use error::*; From 505251e4be1adf8c3dd934ca65cdedc246ef77f2 Mon Sep 17 00:00:00 2001 From: Hayato Kawai Date: Wed, 11 Feb 2026 01:35:30 +0900 Subject: [PATCH 2/5] refactor(exec): simplify extension_runner tests with rstest parametrize Individual test functions for each response variant and error case made the test file unnecessarily verbose. Consolidating them into parametrized rstest cases reduces duplication while keeping the same coverage. Co-Authored-By: Claude Opus 4.6 --- src/exec/extension_runner.rs | 340 ++++++++++++----------------------- 1 file changed, 111 insertions(+), 229 deletions(-) diff --git a/src/exec/extension_runner.rs b/src/exec/extension_runner.rs index 76e3470..2b2d4a8 100644 --- a/src/exec/extension_runner.rs +++ b/src/exec/extension_runner.rs @@ -172,10 +172,8 @@ mod tests { } } - /// Create a temporary script file that drains stdin and prints the given response. - /// Returns the path to the script. Uses a unique counter to avoid conflicts - /// between parallel test runs. - fn write_test_script(response: &str) -> std::path::PathBuf { + /// Create a temporary executable script with the given raw content. + fn write_test_script_raw(content: &str) -> std::path::PathBuf { use std::sync::atomic::{AtomicU64, Ordering}; static COUNTER: AtomicU64 = AtomicU64::new(0); @@ -183,11 +181,7 @@ mod tests { std::fs::create_dir_all(&dir).expect("should create temp dir"); let id = COUNTER.fetch_add(1, Ordering::Relaxed); let path = dir.join(format!("ext-{}-{}.sh", std::process::id(), id)); - std::fs::write( - &path, - format!("#!/bin/sh\ncat > /dev/null\nprintf '%s' '{}'\n", response), - ) - .expect("should write test script"); + std::fs::write(&path, content).expect("should write test script"); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -197,6 +191,14 @@ mod tests { path } + /// Create a script that drains stdin and prints the given JSON response. + fn write_test_script(response: &str) -> std::path::PathBuf { + write_test_script_raw(&format!( + "#!/bin/sh\ncat > /dev/null\nprintf '%s' '{}'\n", + response + )) + } + // === Serialization tests === #[test] @@ -253,258 +255,138 @@ mod tests { // === Response deserialization tests === - #[test] - fn deserialize_valid_deny_response() { - let json = r#"{ - "jsonrpc": "2.0", - "id": 1, - "result": { - "status": "deny", - "message": "POST requests are not allowed", - "fix_suggestion": "curl -X GET https://api.example.com" - } - }"#; - - let response = ProcessExtensionRunner::parse_jsonrpc_response(json) - .expect("should parse valid deny response"); - assert_eq!(response.status, "deny"); - assert_eq!( - response.message.as_deref(), - Some("POST requests are not allowed") - ); - assert_eq!( - response.fix_suggestion.as_deref(), - Some("curl -X GET https://api.example.com") - ); - } - - #[test] - fn deserialize_valid_allow_response() { - let json = r#"{ - "jsonrpc": "2.0", - "id": 1, - "result": { - "status": "allow" - } - }"#; - - let response = ProcessExtensionRunner::parse_jsonrpc_response(json) - .expect("should parse valid allow response"); - assert_eq!(response.status, "allow"); - assert_eq!(response.message, None); - assert_eq!(response.fix_suggestion, None); - } - - #[test] - fn deserialize_valid_ask_response() { - let json = r#"{ - "jsonrpc": "2.0", - "id": 1, - "result": { - "status": "ask", - "message": "This looks risky, please confirm" - } - }"#; - - let response = ProcessExtensionRunner::parse_jsonrpc_response(json) - .expect("should parse valid ask response"); - assert_eq!(response.status, "ask"); - assert_eq!( - response.message.as_deref(), - Some("This looks risky, please confirm") - ); - } - #[rstest] - #[case("allow")] - #[case("deny")] - #[case("ask")] - fn valid_status_values(#[case] status: &str) { - let json = format!(r#"{{"jsonrpc": "2.0", "id": 1, "result": {{"status": "{status}"}}}}"#); - let response = ProcessExtensionRunner::parse_jsonrpc_response(&json) - .expect("should parse response with valid status"); - assert_eq!(response.status, status); + #[case::deny( + r#"{"jsonrpc":"2.0","id":1,"result":{"status":"deny","message":"POST not allowed","fix_suggestion":"use GET"}}"#, + ExtensionResponse { status: "deny".into(), message: Some("POST not allowed".into()), fix_suggestion: Some("use GET".into()) }, + )] + #[case::allow( + r#"{"jsonrpc":"2.0","id":1,"result":{"status":"allow"}}"#, + ExtensionResponse { status: "allow".into(), message: None, fix_suggestion: None }, + )] + #[case::ask( + r#"{"jsonrpc":"2.0","id":1,"result":{"status":"ask","message":"please confirm"}}"#, + ExtensionResponse { status: "ask".into(), message: Some("please confirm".into()), fix_suggestion: None }, + )] + fn parse_valid_response(#[case] json: &str, #[case] expected: ExtensionResponse) { + let response = ProcessExtensionRunner::parse_jsonrpc_response(json) + .expect("should parse valid response"); + assert_eq!(response, expected); } // === Error cases: parsing === - #[test] - fn invalid_json_response() { - let result = ProcessExtensionRunner::parse_jsonrpc_response("not json at all"); - let err = result.expect_err("should fail on invalid JSON"); - match err { - ExtensionError::InvalidResponse(msg) => { - assert_eq!(msg, "JSON parse error: expected ident at line 1 column 2"); - } - other => panic!("expected InvalidResponse, got: {other:?}"), - } - } - - #[test] - fn missing_result_field() { - let json = r#"{"jsonrpc": "2.0", "id": 1}"#; - let err = ProcessExtensionRunner::parse_jsonrpc_response(json) - .expect_err("should fail when result is missing"); - match err { - ExtensionError::InvalidResponse(msg) => { - assert_eq!(msg, "missing 'result' field in JSON-RPC response"); - } - other => panic!("expected InvalidResponse, got: {other:?}"), - } - } - - #[test] - fn missing_status_in_result() { - let json = r#"{"jsonrpc": "2.0", "id": 1, "result": {"message": "hi"}}"#; - let err = ProcessExtensionRunner::parse_jsonrpc_response(json) - .expect_err("should fail when status is missing"); - match err { - ExtensionError::InvalidResponse(msg) => { - assert_eq!( - msg, - "JSON parse error: missing field `status` at line 1 column 55" - ); - } - other => panic!("expected InvalidResponse, got: {other:?}"), - } - } - - #[test] - fn jsonrpc_error_response() { - let json = r#"{ - "jsonrpc": "2.0", - "id": 1, - "error": { - "code": -32600, - "message": "Invalid request" - } - }"#; - let err = ProcessExtensionRunner::parse_jsonrpc_response(json) - .expect_err("should fail on JSON-RPC error response"); + #[rstest] + #[case::invalid_json( + "not json at all", + "JSON parse error: expected ident at line 1 column 2" + )] + #[case::missing_result( + r#"{"jsonrpc": "2.0", "id": 1}"#, + "missing 'result' field in JSON-RPC response" + )] + #[case::missing_status( + r#"{"jsonrpc": "2.0", "id": 1, "result": {"message": "hi"}}"#, + "JSON parse error: missing field `status` at line 1 column 55" + )] + #[case::jsonrpc_error( + r#"{"jsonrpc":"2.0","id":1,"error":{"code":-32600,"message":"Invalid request"}}"#, + "JSON-RPC error: Invalid request" + )] + fn parse_invalid_response(#[case] json: &str, #[case] expected_msg: &str) { + let err = + ProcessExtensionRunner::parse_jsonrpc_response(json).expect_err("should fail to parse"); match err { - ExtensionError::InvalidResponse(msg) => { - assert_eq!(msg, "JSON-RPC error: Invalid request"); - } + ExtensionError::InvalidResponse(msg) => assert_eq!(msg, expected_msg), other => panic!("expected InvalidResponse, got: {other:?}"), } } // === End-to-end with real process === - #[test] - fn validate_with_real_process_deny_response() { - let runner = ProcessExtensionRunner; - let request = sample_request(); - - let response_json = - r#"{"jsonrpc":"2.0","id":1,"result":{"status":"deny","message":"blocked"}}"#; - let script = write_test_script(response_json); - let cmd = script.display().to_string(); - - let response = runner - .validate(&cmd, &request, Duration::from_secs(5)) - .expect("should get deny response from process"); - assert_eq!(response.status, "deny"); - assert_eq!(response.message.as_deref(), Some("blocked")); - } - - #[test] - fn validate_with_real_process_allow_response() { - let runner = ProcessExtensionRunner; - let request = sample_request(); - - let response_json = r#"{"jsonrpc":"2.0","id":1,"result":{"status":"allow"}}"#; + #[rstest] + #[case::deny( + r#"{"jsonrpc":"2.0","id":1,"result":{"status":"deny","message":"blocked"}}"#, + ExtensionResponse { status: "deny".into(), message: Some("blocked".into()), fix_suggestion: None }, + )] + #[case::allow( + r#"{"jsonrpc":"2.0","id":1,"result":{"status":"allow"}}"#, + ExtensionResponse { status: "allow".into(), message: None, fix_suggestion: None }, + )] + fn validate_with_real_process( + #[case] response_json: &str, + #[case] expected: ExtensionResponse, + ) { let script = write_test_script(response_json); - let cmd = script.display().to_string(); - - let response = runner - .validate(&cmd, &request, Duration::from_secs(5)) - .expect("should get allow response from process"); - assert_eq!(response.status, "allow"); - assert_eq!(response.message, None); + let response = ProcessExtensionRunner + .validate( + &script.display().to_string(), + &sample_request(), + Duration::from_secs(5), + ) + .expect("should get response from process"); + assert_eq!(response, expected); } #[test] fn validate_spawn_error_for_nonexistent_command() { - let runner = ProcessExtensionRunner; - let request = sample_request(); - - let err = runner - .validate("/nonexistent/binary/path", &request, Duration::from_secs(5)) + let err = ProcessExtensionRunner + .validate( + "/nonexistent/binary/path", + &sample_request(), + Duration::from_secs(5), + ) .expect_err("should fail for nonexistent binary"); - match err { - ExtensionError::Spawn(_) => {} - other => panic!("expected Spawn error, got: {other:?}"), - } + assert!(matches!(err, ExtensionError::Spawn(_))); } #[test] fn validate_invalid_json_from_process() { - let runner = ProcessExtensionRunner; - let request = sample_request(); - let script = write_test_script("not-valid-json"); - let cmd = script.display().to_string(); - - let err = runner - .validate(&cmd, &request, Duration::from_secs(5)) + let err = ProcessExtensionRunner + .validate( + &script.display().to_string(), + &sample_request(), + Duration::from_secs(5), + ) .expect_err("should fail on invalid JSON from process"); - match err { - ExtensionError::InvalidResponse(_) => {} - other => panic!("expected InvalidResponse, got: {other:?}"), - } + assert!(matches!(err, ExtensionError::InvalidResponse(_))); } #[test] fn validate_timeout() { - let runner = ProcessExtensionRunner; - let request = sample_request(); - - // Use a script that sleeps longer than the timeout - let dir = std::env::temp_dir().join("runok-test-extension"); - std::fs::create_dir_all(&dir).expect("should create temp dir"); - let path = dir.join(format!("ext-timeout-{}.sh", std::process::id())); - std::fs::write(&path, "#!/bin/sh\nsleep 10\n").expect("should write timeout script"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) - .expect("should set executable permission"); - } - let cmd = path.display().to_string(); - - let err = runner - .validate(&cmd, &request, Duration::from_secs(1)) + let script = write_test_script_raw("#!/bin/sh\nsleep 10\n"); + let err = ProcessExtensionRunner + .validate( + &script.display().to_string(), + &sample_request(), + Duration::from_secs(1), + ) .expect_err("should timeout for long-running process"); - match err { - ExtensionError::Timeout(d) => { - assert_eq!(d, Duration::from_secs(1)); - } - other => panic!("expected Timeout error, got: {other:?}"), - } + assert_eq!( + err.to_string(), + format!("timeout after {:?}", Duration::from_secs(1)) + ); } // === parse_command tests === - #[test] - fn parse_command_simple() { - let (prog, args) = ProcessExtensionRunner::parse_command("deno run check.ts"); - assert_eq!(prog, "deno"); - assert_eq!(args, vec!["run", "check.ts"]); - } - - #[test] - fn parse_command_no_args() { - let (prog, args) = ProcessExtensionRunner::parse_command("/usr/bin/validator"); - assert_eq!(prog, "/usr/bin/validator"); - assert!(args.is_empty()); - } - - #[test] - fn parse_command_with_flags() { - let (prog, args) = - ProcessExtensionRunner::parse_command("deno run --allow-net ./checks/url_check.ts"); - assert_eq!(prog, "deno"); - assert_eq!(args, vec!["run", "--allow-net", "./checks/url_check.ts"]); + #[rstest] + #[case::simple("deno run check.ts", "deno", &["run", "check.ts"])] + #[case::no_args("/usr/bin/validator", "/usr/bin/validator", &[])] + #[case::with_flags( + "deno run --allow-net ./checks/url_check.ts", + "deno", + &["run", "--allow-net", "./checks/url_check.ts"], + )] + fn parse_command( + #[case] input: &str, + #[case] expected_prog: &str, + #[case] expected_args: &[&str], + ) { + let (prog, args) = ProcessExtensionRunner::parse_command(input); + assert_eq!(prog, expected_prog); + let args_strs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); + assert_eq!(args_strs, expected_args); } } From f6fa8aedd60f75592d1d1896f8495c7bbdde7b4a Mon Sep 17 00:00:00 2001 From: Hayato Kawai Date: Wed, 11 Feb 2026 01:41:05 +0900 Subject: [PATCH 3/5] exec: remove unused jsonrpc and id fields from JsonRpcResponse These fields were defined solely for JSON-RPC 2.0 compliance but never read after deserialization. serde ignores unknown fields by default, so keeping them added unnecessary dead_code suppressions with no benefit. --- src/exec/extension_runner.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/exec/extension_runner.rs b/src/exec/extension_runner.rs index 2b2d4a8..8e4228e 100644 --- a/src/exec/extension_runner.rs +++ b/src/exec/extension_runner.rs @@ -29,16 +29,6 @@ pub struct ExtensionResponse { /// JSON-RPC 2.0 envelope for responses (internal parsing helper). #[derive(Debug, Deserialize)] struct JsonRpcResponse { - #[expect( - dead_code, - reason = "included for JSON-RPC 2.0 compliance; validated at protocol level" - )] - jsonrpc: Option, - #[expect( - dead_code, - reason = "included for JSON-RPC 2.0 compliance; validated at protocol level" - )] - id: Option, result: Option, error: Option, } From d3b0ec6f96ae5639ff1249321208ae4aacd60aab Mon Sep 17 00:00:00 2001 From: Hayato Kawai Date: Wed, 11 Feb 2026 01:55:56 +0900 Subject: [PATCH 4/5] exec: propagate serialization and stdin write errors in extension runner `build_jsonrpc_request` silently returned an empty string on serialization failure via `unwrap_or_default()`, and `write_all` errors were discarded with `let _ =`. Both cases would cause the extension process to receive no input, leading to a confusing timeout instead of an immediate error. --- src/exec/extension_runner.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/exec/extension_runner.rs b/src/exec/extension_runner.rs index 8e4228e..6b10f24 100644 --- a/src/exec/extension_runner.rs +++ b/src/exec/extension_runner.rs @@ -59,14 +59,19 @@ pub struct ProcessExtensionRunner; impl ProcessExtensionRunner { /// Build a JSON-RPC 2.0 request string from an ExtensionRequest. - pub fn build_jsonrpc_request(&self, request: &ExtensionRequest) -> String { + pub fn build_jsonrpc_request( + &self, + request: &ExtensionRequest, + ) -> Result { let envelope = serde_json::json!({ "jsonrpc": "2.0", "id": 1, "method": "validateCommand", "params": request, }); - serde_json::to_string(&envelope).unwrap_or_default() + serde_json::to_string(&envelope).map_err(|e| { + ExtensionError::InvalidResponse(format!("request serialization failed: {e}")) + }) } /// Parse a JSON-RPC 2.0 response string into an ExtensionResponse. @@ -116,9 +121,9 @@ impl ExtensionRunner for ProcessExtensionRunner { .spawn()?; // Write JSON-RPC request to stdin, then close the pipe - let json_request = self.build_jsonrpc_request(request); + let json_request = self.build_jsonrpc_request(request)?; if let Some(mut stdin) = child.stdin.take() { - let _ = stdin.write_all(json_request.as_bytes()); + stdin.write_all(json_request.as_bytes())?; // stdin is dropped here, closing the pipe } @@ -195,7 +200,9 @@ mod tests { fn serialize_jsonrpc_request() { let runner = ProcessExtensionRunner; let request = sample_request(); - let json_str = runner.build_jsonrpc_request(&request); + let json_str = runner + .build_jsonrpc_request(&request) + .expect("should serialize request"); let parsed: serde_json::Value = serde_json::from_str(&json_str).expect("should be valid JSON"); @@ -224,7 +231,9 @@ mod tests { cwd: ".".to_string(), }; let runner = ProcessExtensionRunner; - let json_str = runner.build_jsonrpc_request(&request); + let json_str = runner + .build_jsonrpc_request(&request) + .expect("should serialize request"); let parsed: serde_json::Value = serde_json::from_str(&json_str).expect("should be valid JSON"); From 06b34221788eadc44847b3f3d2b8553b688d83cb Mon Sep 17 00:00:00 2001 From: Hayato Kawai Date: Wed, 11 Feb 2026 02:36:29 +0900 Subject: [PATCH 5/5] exec: add comments explaining intentional design decisions Reviewers raised questions about parse_command's split_whitespace, ignored kill/wait errors, and the blocking stdout read pattern. Added inline comments documenting why each is acceptable for the current use case to prevent the same questions from recurring. --- src/exec/extension_runner.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/exec/extension_runner.rs b/src/exec/extension_runner.rs index 6b10f24..d4b4320 100644 --- a/src/exec/extension_runner.rs +++ b/src/exec/extension_runner.rs @@ -94,8 +94,9 @@ impl ProcessExtensionRunner { } /// Split executor_cmd into program and arguments for spawning. - /// Uses simple whitespace splitting, as executor commands are typically - /// straightforward (e.g., "deno run --allow-net ./checks/url_check.ts"). + /// Uses simple whitespace splitting because executor_cmd comes from the + /// user's config file, not from external input. Quoted argument support + /// (e.g. via shlex) can be added later if needed. fn parse_command(executor_cmd: &str) -> (String, Vec) { let mut parts = executor_cmd.split_whitespace(); let program = parts.next().unwrap_or("").to_string(); @@ -127,7 +128,10 @@ impl ExtensionRunner for ProcessExtensionRunner { // stdin is dropped here, closing the pipe } - // Wait with timeout using a polling loop + // Wait with timeout using a polling loop. + // NOTE: We read stdout after the process exits (via wait_with_output). + // This could deadlock if stdout exceeds the OS pipe buffer (~64KB), but + // extension responses are small JSON payloads so this is fine in practice. let start = std::time::Instant::now(); loop { match child.try_wait() { @@ -138,6 +142,9 @@ impl ExtensionRunner for ProcessExtensionRunner { } Ok(None) => { if start.elapsed() >= timeout { + // Best-effort cleanup; errors are intentionally ignored + // so we always return `Timeout` rather than a misleading + // `Spawn(io::Error)`. let _ = child.kill(); let _ = child.wait(); return Err(ExtensionError::Timeout(timeout));