Conversation
Add Maven multi-module workspace discovery to the Java client. Uses `mvn help:evaluate -Dexpression=project.modules` to list declared modules, with recursive traversal for nested aggregators and wrapper support via `selectMvnRuntime()`. Adds `**/target/**` to default workspace discovery ignore patterns. Maven detection added between Cargo and JavaScript in ecosystem order. Implements TC-4260 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Assisted-by: Claude Code
Contributor
Reviewer's GuideImplements Maven multi-module workspace discovery in ExhortApi by resolving a Maven (or mvnw) binary, invoking Sequence diagram for Maven multi-module workspace discoverysequenceDiagram
participant Caller
participant ExhortApi
participant Operations
participant JavaMavenProvider
Caller->>ExhortApi: discoverWorkspaceManifests(workspaceDir, ignorePatterns)
ExhortApi->>ExhortApi: Files.exists(workspaceDir/Cargo.toml)
ExhortApi-->>Caller: discoverCargoManifests(...) (if Cargo workspace)
ExhortApi->>ExhortApi: Files.isRegularFile(workspaceDir/pom.xml)
ExhortApi->>ExhortApi: discoverMavenModules(workspaceDir, ignorePatterns)
rect rgb(235, 245, 255)
ExhortApi->>ExhortApi: resolveMavenBinary(workspaceDir)
ExhortApi->>Operations: getWrapperPreference(mvn)
Operations-->>ExhortApi: preferWrapper
ExhortApi->>Operations: isWindows()
Operations-->>ExhortApi: isWindowsFlag
ExhortApi->>JavaMavenProvider: traverseForMvnw(wrapperName, pomPath)
JavaMavenProvider-->>ExhortApi: mvnwPath or null
ExhortApi->>Operations: getCustomPathOrElse(mvn)
Operations-->>ExhortApi: mvnBin or null
ExhortApi-->>ExhortApi: mvnBin == null ?
end
ExhortApi-->>Caller: [root pom.xml only] (if mvnBin == null)
Note over ExhortApi: Maven binary available
ExhortApi->>ExhortApi: collectMavenModules(workspaceDir, mvnBin, visited, manifestPaths)
loop for each directory dir
ExhortApi->>ExhortApi: listMavenModules(dir, mvnBin)
ExhortApi->>Operations: runProcessGetFullOutput(dir, [mvn, help:evaluate, ...])
Operations-->>ExhortApi: ProcessExecOutput
ExhortApi-->>ExhortApi: check exitCode, output
ExhortApi->>ExhortApi: parseMavenModuleList(rawOutput)
ExhortApi-->>ExhortApi: List moduleNames
loop for each module
ExhortApi->>ExhortApi: resolve moduleDir and modulePom
ExhortApi-->>ExhortApi: Files.isRegularFile(modulePom)
ExhortApi->>ExhortApi: add modulePom to manifestPaths
ExhortApi->>ExhortApi: collectMavenModules(moduleDir, mvnBin, visited, manifestPaths)
end
end
ExhortApi->>Operations: WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifestPaths, ignorePatterns)
Operations-->>ExhortApi: filteredManifestPaths
ExhortApi-->>Caller: filteredManifestPaths
Updated class diagram for ExhortApi Maven workspace discoveryclassDiagram
class ExhortApi {
- static Set DEFAULT_WORKSPACE_DISCOVERY_IGNORE
+ List discoverWorkspaceManifests(Path workspaceDir, Set ignorePatterns) throws IOException
- List discoverCargoManifests(Path workspaceDir, Set ignorePatterns) throws IOException
- List discoverMavenModules(Path workspaceDir, Set ignorePatterns)
- void collectMavenModules(Path dir, String mvnBin, java.util.HashSet visited, List manifestPaths)
- List listMavenModules(Path dir, String mvnBin)
- static List parseMavenModuleList(String raw)
- String resolveMavenBinary(Path startDir)
}
class Operations {
+ static boolean isWindows()
+ static boolean getWrapperPreference(String tool)
+ static String getCustomPathOrElse(String binary)
+ static Operations.ProcessExecOutput runProcessGetFullOutput(Path dir, String[] command, Object env)
}
class JavaMavenProvider {
+ static String traverseForMvnw(String wrapperName, String pomPath)
}
class WorkspaceUtils {
+ static List filterByIgnorePatterns(Path workspaceDir, List manifestPaths, Set ignorePatterns)
}
class ProcessExecOutput {
+ int getExitCode()
+ String getOutput()
}
ExhortApi ..> Operations : uses
ExhortApi ..> JavaMavenProvider : uses
ExhortApi ..> WorkspaceUtils : uses
Operations ..> ProcessExecOutput : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Test Results467 tests 467 ✅ 1m 50s ⏱️ Results for commit 6ad8c41. ♻️ This comment has been updated with latest results. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Javadoc for discoverMavenModules() says it returns an empty list when Maven is unavailable, but the implementation returns a singleton list with the root pom.xml; update either the Javadoc or the behavior so they are consistent.
- parseMavenModuleList() and listMavenModules() currently split the responsibility for handling the "null" sentinel value from mvn; consider centralizing that handling inside parseMavenModuleList() so it fully reflects the real mvn output format and callers don’t need to special-case it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Javadoc for discoverMavenModules() says it returns an empty list when Maven is unavailable, but the implementation returns a singleton list with the root pom.xml; update either the Javadoc or the behavior so they are consistent.
- parseMavenModuleList() and listMavenModules() currently split the responsibility for handling the "null" sentinel value from mvn; consider centralizing that handling inside parseMavenModuleList() so it fully reflects the real mvn output format and callers don’t need to special-case it.
## Individual Comments
### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/impl/ExhortApi.java" line_range="954-957" />
<code_context>
+ * @param ignorePatterns glob patterns for paths to exclude from results
+ * @return list of discovered pom.xml paths, or empty list if Maven is unavailable
+ */
+ private List<Path> discoverMavenModules(Path workspaceDir, Set<String> ignorePatterns) {
+ Path rootPom = workspaceDir.resolve("pom.xml");
+ String mvnBin = resolveMavenBinary(workspaceDir);
+ if (mvnBin == null) {
+ LOG.warning("Maven binary not available; returning root pom.xml only");
+ return List.of(rootPom);
</code_context>
<issue_to_address>
**issue (bug_risk):** Method behavior disagrees with its JavaDoc when Maven is unavailable.
JavaDoc promises an empty list when Maven is unavailable, but this branch returns `List.of(rootPom)` when `resolveMavenBinary` is `null`. Either update the JavaDoc to document that the root POM is always returned, or change this branch to return an empty list to match the documented contract.
</issue_to_address>
### Comment 2
<location path="src/test/java/io/github/guacsec/trustifyda/impl/MavenWorkspaceDiscoveryTest.java" line_range="292" />
<code_context>
+ }
+ }
+
+ /** Verifies that when mvn command fails, the root pom.xml is still returned. */
+ @Test
+ void discoverWorkspaceManifests_mvnCommandFails() throws IOException {
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for the case where Maven cannot be resolved at all (resolveMavenBinary returns null).
There’s good coverage for mvn command failures, but nothing that hits the `resolveMavenBinary()` path where it returns `null`. Since `discoverMavenModules()` then logs a warning and falls back to only the root `pom.xml`, please add a test that stubs `resolveMavenBinary` to return `null`, calls `discoverWorkspaceManifests` on a directory with a `pom.xml`, and verifies that only the root `pom.xml` is returned. This ensures correct behavior when Maven isn’t installed or resolvable.
Suggested implementation:
```java
assertThat(manifests).noneMatch(p -> p.toString().contains("module-missing"));
}
}
/** Verifies behavior when Maven cannot be resolved at all. */
@Test
void discoverWorkspaceManifests_mavenBinaryNotResolved() throws IOException {
// Given a Maven workspace where Maven cannot be resolved (resolveMavenBinary returns null)
Path workspaceDir = MAVEN_FIXTURES.resolve("maven_multi_module").toAbsolutePath().normalize();
WorkspaceDiscoveryApi api = new MavenWorkspaceDiscovery();
try (MockedStatic<Operations> mockOps = Mockito.mockStatic(Operations.class)) {
// Simulate that no wrapper is preferred and Maven cannot be resolved from PATH/custom location
mockOps.when(() -> Operations.getWrapperPreference("mvn")).thenReturn(false);
mockOps.when(Operations::isWindows).thenReturn(false);
mockOps.when(() -> Operations.getCustomPathOrElse("mvn")).thenReturn(null);
// When discovering workspace manifests
List<Path> manifests = api.discoverWorkspaceManifests(workspaceDir, Set.of());
// Then only the root pom.xml is returned
assertThat(manifests).containsExactly(workspaceDir.resolve("pom.xml"));
}
}
/** Verifies that when mvn command fails, the root pom.xml is still returned. */
```
If the actual resolution logic uses a different method or class than `Operations.getCustomPathOrElse("mvn")` to implement `resolveMavenBinary()`, you should adjust the stubbing in this test to directly mock the method that returns `null` (for example, by mocking a `resolveMavenBinary()` method on `MavenWorkspaceDiscovery` instead of or in addition to `Operations`). Also ensure that `WorkspaceDiscoveryApi`, `MavenWorkspaceDiscovery`, `Operations`, and `MockedStatic` are already imported; if not, add the appropriate imports at the top of the test file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a-oren
reviewed
Apr 28, 2026
a-oren
reviewed
Apr 28, 2026
a-oren
requested changes
Apr 28, 2026
…other ecosystems Two fixes: 1. discoverWorkspaceManifests now checks if Maven module discovery found actual submodules (size > 1) before returning. A single-module Maven project (just root pom.xml) falls through to Gradle/Go/uv/JS detection. 2. resolveMavenBinary now verifies the wrapper is accessible (runs it with -v) before returning it, matching selectMvnRuntime behavior. Also uses Operations.getExecutable instead of getCustomPathOrElse to verify the system Maven binary exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The 2-arg traverseForMvnw is a private instance method on JavaMavenProvider. Call the 3-arg public static overload with null repoRoot instead, which has the same behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use getCustomPathOrElse for Maven binary resolution (matching Gradle), and handle Java PathMatcher quirk where **/X/** doesn't match paths where X is the first component. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Consolidate duplicate Maven binary resolution logic between ExhortApi and JavaMavenProvider into a single public static method that verifies the binary with --version before returning it. - Add JavaMavenProvider.resolveVerifiedMavenBinary(Path) returning Optional<String> with wrapper-first resolution and verification - Refactor selectMvnRuntime to delegate to the new shared method - Delete duplicate resolveMavenBinary from ExhortApi, update discoverMavenModules to call the shared method via .orElse(null) - Fix @return JavaDoc on discoverMavenModules to match actual behavior - Make 2-arg traverseForMvnw static (required by static caller) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The resolveVerifiedMavenBinary refactoring changed the binary resolution path from getCustomPathOrElse (no verification) to getExecutable (runs --version). Update test mocks accordingly so Maven discovery tests pass in environments without Maven on PATH. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
Verification Report — TC-4260Review Feedback — PASS4 comment threads enumerated, all classified:
All code change requests were addressed in follow-up commits. No sub-tasks created. Scope Containment — PASSAll 13 changed files are within the expected scope:
Commit Traceability — WARN6 commits, 4 reference the feature context in their message. 2 commits lack explicit task reference:
Both are legitimate review-feedback fixes but could benefit from a Sensitive Patterns — PASSNo secrets, credentials, or API keys detected in the diff. CI Status — PASSAll CI checks pass (build, unit tests, integration tests across all ecosystems). Acceptance Criteria — PASS (7/7)
Test Quality — PASS14 test methods covering:
Verification Command — PASS
Overall: PASS (minor commit traceability note) This comment was AI-generated by sdlc-workflow/verify-pr v0.5.11. |
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
2 participants
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
discoverMavenModules()to discover all pom.xml manifest paths in Maven multi-module projects usingmvn help:evaluateresolveMavenBinary()reusingJavaMavenProvider.traverseForMvnw()discoverWorkspaceManifests()between Cargo and JavaScript**/target/**to default workspace discovery ignore patternsTest plan
parseMavenModuleList()unit tests (standard, single, null, empty, malformed, whitespace)**/target/**Implements TC-4260
🤖 Generated with Claude Code
Summary by Sourcery
Add Maven multi-module workspace discovery to Exhort workspace scanning and extend default ignore patterns.
New Features:
Enhancements:
Tests: