Fix #863: add ProcessRunner utility to capabilities-common#89
Merged
orpiske merged 1 commit intowanaku-ai:mainfrom Mar 3, 2026
Merged
Fix #863: add ProcessRunner utility to capabilities-common#89orpiske merged 1 commit intowanaku-ai:mainfrom
orpiske merged 1 commit intowanaku-ai:mainfrom
Conversation
Reviewer's GuideAdds a reusable ProcessRunner utility to capabilities-common for running external processes with SLF4J logging and introduces a basic unit test for capturing command output. Sequence diagram for ProcessRunner.runWithOutput execution flowsequenceDiagram
participant Client
participant ProcessRunner
participant ProcessBuilder
participant Process
participant Runtime
Client->>ProcessRunner: runWithOutput(command)
activate ProcessRunner
ProcessRunner->>ProcessBuilder: new ProcessBuilder(command)
ProcessRunner->>ProcessBuilder: redirectOutput(PIPE), redirectError(PIPE)
ProcessRunner->>ProcessBuilder: start()
ProcessBuilder-->>ProcessRunner: Process
activate Process
ProcessRunner->>ProcessRunner: readOutput(Process)
ProcessRunner->>ProcessRunner: waitForExit(Process)
ProcessRunner->>Runtime: addShutdownHook(Thread)
Runtime-->>ProcessRunner: hook added
ProcessRunner->>Process: waitFor()
Process-->>ProcessRunner: exitCode
alt exitCode != 0
ProcessRunner-->>ProcessRunner: log warn
end
ProcessRunner->>Runtime: removeShutdownHook(Thread)
Runtime-->>ProcessRunner: hook removed
deactivate Process
ProcessRunner-->>Client: output
deactivate ProcessRunner
Class diagram for new ProcessRunner utilityclassDiagram
class ProcessRunner {
-Logger LOG
-ProcessRunner()
+static String runWithOutput(String command)
+static void run(File directory, String command)
+static void run(File directory, Map~String,String~ environmentVariables, String command)
-static String readOutput(Process process)
-static void waitForExit(Process process)
}
class WanakuException {
}
class Logger {
}
class ProcessBuilder {
}
class Process {
}
ProcessRunner --> WanakuException : throws
ProcessRunner --> Logger : uses
ProcessRunner --> ProcessBuilder : creates
ProcessBuilder --> Process : starts
ProcessRunner --> Process : manages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
runWithOutput, you redirectstderrto a separate PIPE but never consume it, which risks deadlocking on large error output; consider eitherredirectErrorStream(true)or reading from both streams. readOutputcreates aBufferedReaderwithout closing it; use try-with-resources to ensure the process streams are properly closed after reading.- When catching
InterruptedExceptionyou wrap and rethrow but never restore the interrupt status; consider callingThread.currentThread().interrupt()before throwing theWanakuException.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `runWithOutput`, you redirect `stderr` to a separate PIPE but never consume it, which risks deadlocking on large error output; consider either `redirectErrorStream(true)` or reading from both streams.
- `readOutput` creates a `BufferedReader` without closing it; use try-with-resources to ensure the process streams are properly closed after reading.
- When catching `InterruptedException` you wrap and rethrow but never restore the interrupt status; consider calling `Thread.currentThread().interrupt()` before throwing the `WanakuException`.
## Individual Comments
### Comment 1
<location path="capabilities-common/src/main/java/ai/wanaku/capabilities/sdk/common/ProcessRunner.java" line_range="44-49" />
<code_context>
+ }
+ }
+
+ private static String readOutput(Process process) throws IOException {
+ // Read the output from the process
+ BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
+ StringBuilder output = new StringBuilder();
+ String line;
+ while ((line = reader.readLine()) != null) {
+ output.append(line).append("\n");
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Close the BufferedReader/InputStream via try-with-resources to avoid leaking OS resources.
The `BufferedReader` (and underlying `InputStream`) remains open for the lifetime of the process, which can cause resource leaks in long-running or repeated executions. Wrap the reader in a try-with-resources (or close it in a finally block) so it’s always released:
```java
try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
StringBuilder output = new StringBuilder();
String line;
while ((line = reader.readLine()) != null) {
output.append(line).append("\n");
}
return output.toString();
}
```
</issue_to_address>
### Comment 2
<location path="capabilities-common/src/main/java/ai/wanaku/capabilities/sdk/common/ProcessRunner.java" line_range="24-26" />
<code_context>
+ public static String runWithOutput(String... command) {
+ try {
+ ProcessBuilder processBuilder = new ProcessBuilder(command);
+ // Redirect output and error streams to a pipe
+ processBuilder.redirectOutput(ProcessBuilder.Redirect.PIPE);
+ processBuilder.redirectError(ProcessBuilder.Redirect.PIPE);
+
+ final Process process = processBuilder.start();
</code_context>
<issue_to_address>
**issue (bug_risk):** Reading only stdout while piping stderr risks deadlock for verbose error output.
In `runWithOutput`, both stdout and stderr are piped but only stdout is read. If the subprocess writes heavily to stderr, its buffer can fill and block the process so `waitFor()` never returns. Consider either using `redirectErrorStream(true)` and reading a single stream, or consuming stderr concurrently (e.g., separate thread) / redirecting it to inheritIO or logging instead of a pipe.
</issue_to_address>
### Comment 3
<location path="capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java" line_range="11" />
<code_context>
+public class ProcessRunnerTest {
+
+ @Test
+ void runWithOutput_returnsCommandOutput() {
+ String output = ProcessRunner.runWithOutput("echo", "hello");
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for `runWithOutput` when the process exits with a non-zero status
Because `waitForExit` only logs a warning on non-zero exits and doesn’t throw, please add a test that runs a command exiting with status 1 and asserts that `runWithOutput` does not throw and returns the expected output. This will lock in the intended behavior for failure cases and prevent regressions.
</issue_to_address>
### Comment 4
<location path="capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java" line_range="10-17" />
<code_context>
+
+ @Test
+ void runWithOutput_returnsCommandOutput() {
+ String output = ProcessRunner.runWithOutput("echo", "hello");
+
+ assertNotNull(output, "Output should not be null");
</code_context>
<issue_to_address>
**suggestion (testing):** The test command may not be fully portable across platforms
Using `echo` as the command makes this test non-portable (e.g., on Windows `echo` is usually a shell built-in, not an executable). To keep the test reliable across CI environments, consider using a known binary available in the test environment (such as `java`) or choosing the command based on the detected OS.
```suggestion
@Test
void runWithOutput_returnsCommandOutput() {
String osName = System.getProperty("os.name").toLowerCase();
String output;
if (osName.contains("win")) {
// On Windows, echo is a cmd.exe built-in
output = ProcessRunner.runWithOutput("cmd.exe", "/c", "echo hello");
} else {
// On Unix-like systems, echo is typically a shell built-in
output = ProcessRunner.runWithOutput("sh", "-c", "echo hello");
}
assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("hello"), "Output should contain 'hello'");
}
```
</issue_to_address>
### Comment 5
<location path="capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java" line_range="16" />
<code_context>
+
+ assertNotNull(output, "Output should not be null");
+ assertFalse(output.isEmpty(), "Output should not be empty");
+ assertTrue(output.contains("hello"), "Output should contain 'hello'");
+ }
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that covers stderr handling in `runWithOutput`
Currently we only cover a stdout-only command. Since `runWithOutput` pipes stderr but only reads stdout, please add a test where the child writes to both streams, and assert that it completes successfully and the returned value includes only the expected stdout content. This will make the intended stderr behavior explicit and protected by tests.
Suggested implementation:
```java
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertFalse;
import org.junit.jupiter.api.Test;
public class ProcessRunnerTest {
@Test
void runWithOutput_returnsCommandOutput() {
String output = ProcessRunner.runWithOutput("echo", "hello");
assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("hello"), "Output should contain 'hello'");
}
@Test
void runWithOutput_ignoresStderrAndReturnsOnlyStdout() {
String output = ProcessRunner.runWithOutput(
"sh",
"-c",
"echo stdout && echo stderr 1>&2"
);
assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("stdout"), "Output should contain only stdout content");
assertFalse(output.contains("stderr"), "Output should not contain stderr content");
}
}
```
If your test environment does not guarantee a POSIX-compatible `sh` shell (e.g., pure Windows without WSL or Git Bash), you will need to adapt the command invocation in `runWithOutput_ignoresStderrAndReturnsOnlyStdout` to a platform-appropriate shell or helper script that can write to both stdout and stderr.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+44
to
+49
| private static String readOutput(Process process) throws IOException { | ||
| // Read the output from the process | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); | ||
| StringBuilder output = new StringBuilder(); | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { |
There was a problem hiding this comment.
suggestion (bug_risk): Close the BufferedReader/InputStream via try-with-resources to avoid leaking OS resources.
The BufferedReader (and underlying InputStream) remains open for the lifetime of the process, which can cause resource leaks in long-running or repeated executions. Wrap the reader in a try-with-resources (or close it in a finally block) so it’s always released:
try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
StringBuilder output = new StringBuilder();
String line;
while ((line = reader.readLine()) != null) {
output.append(line).append("\n");
}
return output.toString();
}
Comment on lines
+24
to
+26
| // Redirect output and error streams to a pipe | ||
| processBuilder.redirectOutput(ProcessBuilder.Redirect.PIPE); | ||
| processBuilder.redirectError(ProcessBuilder.Redirect.PIPE); |
There was a problem hiding this comment.
issue (bug_risk): Reading only stdout while piping stderr risks deadlock for verbose error output.
In runWithOutput, both stdout and stderr are piped but only stdout is read. If the subprocess writes heavily to stderr, its buffer can fill and block the process so waitFor() never returns. Consider either using redirectErrorStream(true) and reading a single stream, or consuming stderr concurrently (e.g., separate thread) / redirecting it to inheritIO or logging instead of a pipe.
| public class ProcessRunnerTest { | ||
|
|
||
| @Test | ||
| void runWithOutput_returnsCommandOutput() { |
There was a problem hiding this comment.
suggestion (testing): Add a test for runWithOutput when the process exits with a non-zero status
Because waitForExit only logs a warning on non-zero exits and doesn’t throw, please add a test that runs a command exiting with status 1 and asserts that runWithOutput does not throw and returns the expected output. This will lock in the intended behavior for failure cases and prevent regressions.
Comment on lines
+10
to
+17
| @Test | ||
| void runWithOutput_returnsCommandOutput() { | ||
| String output = ProcessRunner.runWithOutput("echo", "hello"); | ||
|
|
||
| assertNotNull(output, "Output should not be null"); | ||
| assertFalse(output.isEmpty(), "Output should not be empty"); | ||
| assertTrue(output.contains("hello"), "Output should contain 'hello'"); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): The test command may not be fully portable across platforms
Using echo as the command makes this test non-portable (e.g., on Windows echo is usually a shell built-in, not an executable). To keep the test reliable across CI environments, consider using a known binary available in the test environment (such as java) or choosing the command based on the detected OS.
Suggested change
| @Test | |
| void runWithOutput_returnsCommandOutput() { | |
| String output = ProcessRunner.runWithOutput("echo", "hello"); | |
| assertNotNull(output, "Output should not be null"); | |
| assertFalse(output.isEmpty(), "Output should not be empty"); | |
| assertTrue(output.contains("hello"), "Output should contain 'hello'"); | |
| } | |
| @Test | |
| void runWithOutput_returnsCommandOutput() { | |
| String osName = System.getProperty("os.name").toLowerCase(); | |
| String output; | |
| if (osName.contains("win")) { | |
| // On Windows, echo is a cmd.exe built-in | |
| output = ProcessRunner.runWithOutput("cmd.exe", "/c", "echo hello"); | |
| } else { | |
| // On Unix-like systems, echo is typically a shell built-in | |
| output = ProcessRunner.runWithOutput("sh", "-c", "echo hello"); | |
| } | |
| assertNotNull(output, "Output should not be null"); | |
| assertFalse(output.isEmpty(), "Output should not be empty"); | |
| assertTrue(output.contains("hello"), "Output should contain 'hello'"); | |
| } |
|
|
||
| assertNotNull(output, "Output should not be null"); | ||
| assertFalse(output.isEmpty(), "Output should not be empty"); | ||
| assertTrue(output.contains("hello"), "Output should contain 'hello'"); |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test that covers stderr handling in runWithOutput
Currently we only cover a stdout-only command. Since runWithOutput pipes stderr but only reads stdout, please add a test where the child writes to both streams, and assert that it completes successfully and the returned value includes only the expected stdout content. This will make the intended stderr behavior explicit and protected by tests.
Suggested implementation:
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertFalse;
import org.junit.jupiter.api.Test;
public class ProcessRunnerTest {
@Test
void runWithOutput_returnsCommandOutput() {
String output = ProcessRunner.runWithOutput("echo", "hello");
assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("hello"), "Output should contain 'hello'");
}
@Test
void runWithOutput_ignoresStderrAndReturnsOnlyStdout() {
String output = ProcessRunner.runWithOutput(
"sh",
"-c",
"echo stdout && echo stderr 1>&2"
);
assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("stdout"), "Output should contain only stdout content");
assertFalse(output.contains("stderr"), "Output should not contain stderr content");
}
}If your test environment does not guarantee a POSIX-compatible sh shell (e.g., pure Windows without WSL or Git Bash), you will need to adapt the command invocation in runWithOutput_ignoresStderrAndReturnsOnlyStdout to a platform-appropriate shell or helper script that can write to both stdout and stderr.
orpiske
added a commit
to orpiske/wanaku-capabilities-java-sdk
that referenced
this pull request
Mar 3, 2026
- Use redirectErrorStream(true) to prevent stderr deadlock - Use try-with-resources for BufferedReader in readOutput - Restore interrupt status before rethrowing WanakuException - Make tests cross-platform with OS detection - Add test for non-zero exit status behavior - Add test for merged stderr/stdout stream
orpiske
added a commit
that referenced
this pull request
Mar 3, 2026
- Use redirectErrorStream(true) to prevent stderr deadlock - Use try-with-resources for BufferedReader in readOutput - Restore interrupt status before rethrowing WanakuException - Make tests cross-platform with OS detection - Add test for non-zero exit status behavior - Add test for merged stderr/stdout stream
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ProcessRunnerutility class tocapabilities-commonmodule (moved from wanakucore-util)runWithOutput()Test plan
mvn verifypassesrunWithOutput()passesSummary by Sourcery
Introduce a reusable ProcessRunner utility in capabilities-common for executing external processes and capturing their output.
New Features:
Tests: