feat(exec): support JSON-RPC 2.0 communication with extension plugins#19
feat(exec): support JSON-RPC 2.0 communication with extension plugins#19
Conversation
…ication 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 <[email protected]>
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 <[email protected]>
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.
Summary of ChangesHello @fohte, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's extensibility by integrating a mechanism for external processes to act as validation plugins. By leveraging JSON-RPC 2.0 over standard I/O, it allows for custom, language-agnostic validation logic to be executed outside the main application, thereby increasing flexibility and modularity without tightly coupling the validation rules to the core codebase. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism for communicating with external extension plugins using JSON-RPC 2.0. However, a critical command injection vulnerability has been identified where the executor_cmd string is parsed insecurely, potentially allowing arbitrary command execution. Additionally, there are areas for improvement in error handling and the robustness of command parsing, particularly concerning serialization, I/O errors, and handling complex command lines. Addressing this vulnerability is crucial before merging.
| fn parse_command(executor_cmd: &str) -> (String, Vec<String>) { | ||
| let mut parts = executor_cmd.split_whitespace(); | ||
| let program = parts.next().unwrap_or("").to_string(); | ||
| let args: Vec<String> = parts.map(String::from).collect(); | ||
| (program, args) | ||
| } |
There was a problem hiding this comment.
The parse_command function's use of split_whitespace is insecure and leads to a command injection vulnerability. It fails to correctly handle arguments with spaces or shell metacharacters, enabling arbitrary command execution if executor_cmd is user-controlled (e.g., sh -c 'reboot'). This also makes the parsing non-robust for legitimate commands with quoted arguments. Using a crate like shlex is recommended for secure and reliable shell-style word splitting.
| fn parse_command(executor_cmd: &str) -> (String, Vec<String>) { | |
| let mut parts = executor_cmd.split_whitespace(); | |
| let program = parts.next().unwrap_or("").to_string(); | |
| let args: Vec<String> = parts.map(String::from).collect(); | |
| (program, args) | |
| } | |
| fn parse_command(executor_cmd: &str) -> (String, Vec<String>) { | |
| let parts: Vec<String> = shlex::split(executor_cmd).unwrap_or_default(); | |
| let program = parts.get(0).cloned().unwrap_or_default(); | |
| let args = parts.into_iter().skip(1).collect(); | |
| (program, args) | |
| } |
There was a problem hiding this comment.
executor_cmd comes from the config file authored by the user who runs the process, so command injection is not a threat here. shlex support for quoted arguments could be added later as a feature enhancement if needed.
src/exec/extension_runner.rs
Outdated
| 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() | ||
| } |
There was a problem hiding this comment.
The use of unwrap_or_default() can hide serialization errors. If serde_json::to_string fails, an empty string will be sent to the extension, which will likely cause it to fail or hang without a clear error message in runok. It's better to propagate the error by returning a Result.
You'll also need to update the call site in validate to handle the Result, for example:
let json_request = self.build_jsonrpc_request(request)
.map_err(|e| ExtensionError::InvalidResponse(format!("Request serialization failed: {e}")))?;| 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() | |
| } | |
| pub fn build_jsonrpc_request(&self, request: &ExtensionRequest) -> Result<String, serde_json::Error> { | |
| let envelope = serde_json::json!({ | |
| "jsonrpc": "2.0", | |
| "id": 1, | |
| "method": "validateCommand", | |
| "params": request, | |
| }); | |
| serde_json::to_string(&envelope) | |
| } |
src/exec/extension_runner.rs
Outdated
| // 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()); |
There was a problem hiding this comment.
The result of write_all is ignored. If writing to the child's stdin fails, the error is silently dropped. This can lead to the child process hanging and eventually timing out, masking the real issue. The io::Error should be propagated.
| let _ = stdin.write_all(json_request.as_bytes()); | |
| stdin.write_all(json_request.as_bytes())?; |
| let _ = child.kill(); | ||
| let _ = child.wait(); |
There was a problem hiding this comment.
The results of child.kill() and child.wait() are ignored. These operations can fail, and ignoring the errors can hide underlying issues. It's better to handle or propagate these errors.
| let _ = child.kill(); | |
| let _ = child.wait(); | |
| child.kill()?; | |
| child.wait()?; |
There was a problem hiding this comment.
kill()/wait() here are best-effort cleanup after a timeout. Propagating their errors would replace the Timeout error with a Spawn(io::Error), which is less informative for the caller.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 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)), | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Potential deadlock: stdout pipe buffer fills before process exit
The polling loop on lines 127-144 waits for the child process to exit (via try_wait) before reading any stdout (via wait_with_output on line 130). If the extension process writes a response larger than the OS pipe buffer (typically 64KB on Linux), the child will block on its write() to stdout, unable to exit. Meanwhile, the parent is stuck in the polling loop waiting for exit, never reading stdout.
Root Cause
The code uses a poll-then-read pattern:
- Parent writes to stdin and closes it (lines 119-123)
- Parent enters
try_wait()polling loop (line 128) - Only after
try_wait()returnsOk(Some(_))does it callwait_with_output()(line 130) to read stdout
If the child's output exceeds the pipe buffer capacity, the child blocks on writing to stdout. Since the parent never reads stdout until after the child exits, this is a classic deadlock. The timeout on line 135 acts as a safety net, but the user gets a misleading "timeout" error instead of receiving the valid response.
The correct approach is to read stdout concurrently with waiting — e.g., by using wait_with_output() directly (it internally handles concurrent reading) with a separate timeout mechanism, or by spawning a reader thread for stdout before entering the wait loop.
Impact: Extensions that produce large responses (>64KB) will always hit the timeout and be killed, even though their output is valid.
Prompt for agents
Refactor the validate method in ProcessExtensionRunner (src/exec/extension_runner.rs lines 108-145) to avoid the deadlock. Instead of polling with try_wait() and then calling wait_with_output(), you should read stdout concurrently with waiting for the child. One approach: spawn a thread to read stdout to completion, then use the try_wait polling loop (or wait()) with the timeout for the exit status. Another approach: call wait_with_output() directly on a separate thread and use the main thread to enforce the timeout. The key constraint is that stdout must be drained while the child is still running, not after it exits.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The extension response is a small JSON (status, message, fix_suggestion), so it will not reach the 64KB pipe buffer limit in practice. Async stdout reading would add significant complexity for no real benefit here.
`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.
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.
1 participant
Why
What
ExtensionRunnertrait andProcessExtensionRunnerimplementation for communicating with external processes via JSON-RPC 2.0 over stdio