Repository navigation
fix: remove maven-invoker to eliminate javax.inject leak (#144) - #145
Conversation
…144) Replace maven-invoker API with ProcessBuilder in MavenCompiler. This removes maven-invoker:3.3.0 and maven-shared-utils:3.4.2, which transitively brought javax.inject:javax.inject:1 onto the classpath — causing silent injection failures when developers accidentally used @javax.inject.Inject instead of @jakarta.inject.Inject. closes #144 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes maven-invoker from enkan-devel to eliminate the transitive javax.inject dependency that could cause silent DI failures, and rewrites MavenCompiler to invoke mvn compile directly via ProcessBuilder.
Changes:
- Removed
maven-invokerandmaven-shared-utilsdependencies fromenkan-devel. - Reimplemented
MavenCompilerto execute Maven usingProcessBuilderand stream stdout/stderr back to the REPL transport.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| enkan-devel/src/main/java/enkan/system/devel/compiler/MavenCompiler.java | Switches from Maven Invoker API to ProcessBuilder and streams process output via virtual threads. |
| enkan-devel/pom.xml | Drops Maven Invoker-related dependencies to remove the javax.inject leak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String mvnCommand = new File(mavenHome, "bin/mvn").getAbsolutePath(); | ||
| ProcessBuilder pb = new ProcessBuilder(mvnCommand, "compile"); | ||
| pb.directory(new File(projectDirectory)); | ||
| pb.redirectErrorStream(false); |
There was a problem hiding this comment.
mvnCommand is hard-coded to ${MAVEN_HOME}/bin/mvn, which won’t exist on Windows (typically mvn.cmd/mvn.bat). The previous Maven Invoker implementation handled platform-specific launchers; consider selecting the correct executable based on OS (or falling back to mvn on PATH) to avoid breaking /compile on Windows environments.
| Thread stdoutReader = Thread.ofVirtual().start(() -> { | ||
| try (BufferedReader reader = new BufferedReader( | ||
| new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| t.send(ReplResponse.withOut(line)); | ||
| } | ||
| } catch (IOException e) { | ||
| // Process ended — stop reading | ||
| } | ||
| }); | ||
|
|
||
| Thread stderrReader = Thread.ofVirtual().start(() -> { | ||
| try (BufferedReader reader = new BufferedReader( | ||
| new InputStreamReader(process.getErrorStream(), StandardCharsets.UTF_8))) { | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| t.send(ReplResponse.withErr(line)); | ||
| } | ||
| } catch (IOException e) { | ||
| // Process ended — stop reading | ||
| } |
There was a problem hiding this comment.
The stdout/stderr reader threads swallow all IOExceptions. That can mask real read failures (e.g., stream closed unexpectedly while the process is still running) and will silently drop Maven output. Consider capturing the exception (e.g., to an AtomicReference) and reporting it via CompileResult.failure(...) or at least forwarding an error to the Transport so failures aren’t hidden.
| int exitCode = process.waitFor(); | ||
| stdoutReader.join(); | ||
| stderrReader.join(); | ||
|
|
||
| if (exitCode != 0) { | ||
| return CompileResult.failure(new IllegalStateException( | ||
| "Maven compile failed with exit code " + invocationResult.getExitCode())); | ||
| "Maven compile failed with exit code " + exitCode)); | ||
| } | ||
| } catch (MavenInvocationException ex) { | ||
| return CompileResult.failure(ex); | ||
| } catch (IOException e) { | ||
| return CompileResult.failure(e); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| return CompileResult.failure(e); | ||
| } |
There was a problem hiding this comment.
If execute is interrupted while waiting, the method returns a failure but the spawned Maven process (and the reader threads) may continue running in the background. Consider ensuring the process is terminated on interruption (e.g., process.destroyForcibly() in the InterruptedException path or a finally block) to avoid orphaned builds and unexpected output after returning.
| String mvnCommand = new File(mavenHome, "bin/mvn").getAbsolutePath(); | ||
| ProcessBuilder pb = new ProcessBuilder(mvnCommand, "compile"); |
There was a problem hiding this comment.
MAVEN_HOME existence is validated, but ${MAVEN_HOME}/bin/mvn itself isn’t. When it’s missing/non-executable, this will fail later with a generic IOException, which is less actionable than a targeted misconfiguration error. Consider checking mvnCommand exists (and is executable) and throwing MisconfigurationException("devel.MAVEN_HOME_NOT_SET") (or a more specific key) before starting the process.
| String mvnCommand = new File(mavenHome, "bin/mvn").getAbsolutePath(); | |
| ProcessBuilder pb = new ProcessBuilder(mvnCommand, "compile"); | |
| File mvnCommand = new File(mavenHome, "bin/mvn"); | |
| if (!mvnCommand.exists() || !mvnCommand.isFile() || !mvnCommand.canExecute()) { | |
| throw new MisconfigurationException("devel.MAVEN_HOME_NOT_SET"); | |
| } | |
| ProcessBuilder pb = new ProcessBuilder(mvnCommand.getAbsolutePath(), "compile"); |
- Fall back to PATH-resolved mvn when MAVEN_HOME/M2_HOME is not set - Select mvn.cmd on Windows for both MAVEN_HOME and PATH cases - Validate mvn executable with canExecute() before starting process - Report IOException from reader threads via Transport instead of swallowing - Add pom.xml existence check with clear error message - Destroy process and join reader threads in finally block to prevent leaks - Use waitFor(10, MINUTES) timeout; join reader threads with 5s limit - Fix stale assumeTrue in tests to detect mvn via PATH as well Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
maven-invoker:3.3.0andmaven-shared-utils:3.4.2fromenkan-develMavenCompilerto useProcessBuilderinstead of maven-invoker APIjavax.inject:javax.inject:1dependency that caused silent@Injectfailures (fields leftnullat runtime when@javax.inject.Injectwas used instead of@jakarta.inject.Inject)Why not just exclude?
maven-invokerwrapsProcessBuilderinternally — there's no value in keeping the dependency when we can callmvn compiledirectly. Removing it entirely is cleaner than adding an<exclusion>.Test plan
mvn dependency:tree -pl enkan-devel | grep javax.inject— emptymvn dependency:tree -pl kotowari-example | grep javax.inject— emptymvn test -pl enkan-devel -am— all tests pass (MavenCompilerTest skipped perassumeTrueas before)closes #144
🤖 Generated with Claude Code