Repository navigation
Conversation
- Introduce MavenClappCling entry point and ClassWorld/ClassLoader extension for CLAPP tools - Support loading tool-specific jars from lib/clapp/<toolName>/ in m2.conf and launch logic - Add --clapp <toolName> flag support in the mvn launcher script - Provide lib/clapp/README.txt detailing directory layout and clapp.properties configuration - Add comprehensive MavenClappClingTest unit tests
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Review Summary
This PR introduces a CLAPP (Command Line App) mechanism for Maven CLI tool isolation. The concept is sound but the implementation has a critical architectural contradiction (double class loading via m2.conf + URLClassLoader), a resource leak on the URLClassLoader, and a path traversal vulnerability in the tool name validation.
These need to be resolved before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…ak and path traversal - Remove optionally CLAPP line from m2.conf to avoid loading jars twice into plexus.core realm AND the child URLClassLoader (architectural contradiction). MavenClappCling.launchClapp() is now the sole place responsible for loading CLAPP-private jars via an isolated URLClassLoader (Option A as suggested). - Fix URLClassLoader resource leak: change manual try/finally to try-with-resources so the classloader is always closed after the CLAPP invocation completes. - Prevent path traversal: validate clappName with allowlist regex [a-zA-Z0-9_-] and double-check the resolved Path starts inside lib/clapp/ before opening it. - Add launchClapp_throwsIOException_whenToolNameInvalid unit test to cover the new validation path.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after b392595
Previous findings status:
| # | Finding | Status |
|---|---|---|
| 1 | Double class loading via m2.conf + URLClassLoader | ✅ Addressed — m2.conf is no longer modified; CLAPP JARs are loaded exclusively via the child URLClassLoader in launchClapp(). |
| 2 | URLClassLoader resource leak | ✅ Addressed — now wrapped in try-with-resources (line 383). |
| 3 | Path traversal vulnerability on clappName | ✅ Addressed — regex validation ^[a-zA-Z0-9_-]+$ plus startsWith(baseDir) check (lines 368-375). |
| 4 | Dead code: _skip_next never set to true |
❌ Not addressed — see inline comment. |
Three of four findings are properly fixed. The _skip_next dead code remains.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Thank you for the thorough review! All three issues have been resolved in commit Fix 1: Removed double class loading from
|
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
To what extent is this user-facing? Should there be doc updates with this?
If the goal is to support thrid parties writing CLI tools, then docs would seem to be necessary
…Exception and add developer docs - Remove dead code _skip_next logic in mvn launcher script - Remove stale reference to m2.conf in mvn comments - Introduce MavenClappCling.ClappException (subclass of MavenException) for tool loading and invocation failures - Use IllegalArgumentException for invalid tool name and path traversal validation - Update MavenClappClingTest unit test expectations accordingly - Add comprehensive documentation for third-party CLAPP developers in api/maven-api-cli/src/site/markdown/clapp.md - Refine CLAPP entry-point signature and documentation link in lib/clapp/README.txt
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after b0314a4
Previous findings status:
| # | Finding | Source | Status |
|---|---|---|---|
| 1 | Double class loading via m2.conf + URLClassLoader | gnodet-bot (review 1) | ✅ Addressed in b392595 — m2.conf optionally directive removed; URLClassLoader in launchClapp() is the sole class-loading mechanism. |
| 2 | URLClassLoader resource leak | gnodet-bot (review 1) | ✅ Addressed in b392595 — wrapped in try-with-resources. |
| 3 | Path traversal vulnerability | gnodet-bot (review 1) | ✅ Addressed in b392595 — regex ^[a-zA-Z0-9_-]+$ + startsWith(baseDir) guard. |
| 4 | _skip_next dead code in shell script |
gnodet-bot (review 2) | ✅ Addressed in b0314a4 — initialization, check, and skip block removed entirely. |
| 5 | Stale comment referencing m2.conf | gnodet-bot (review 2) | ✅ Addressed in b0314a4 — comment now reads "so that MavenClappCling can pick them up." |
| 6 | IOException misused for reflection/config errors | elharo | ✅ Addressed in b0314a4 — introduced ClappException extends MavenException for CLAPP-specific errors, IllegalArgumentException for invalid input. Javadoc @throws updated to document all three exception types. Tests updated accordingly. |
| 7 | Missing user-facing documentation | elharo | ✅ Addressed in b0314a4 — comprehensive clapp.md guide (129 lines) added under api/maven-api-cli/src/site/markdown/, covering architecture, class loading, packaging, error handling, and usage. README.txt updated with --clapp= form and link to docs. Index page updated. |
All seven previous findings are properly resolved. The code is clean — ClappException hierarchy is well-structured, the shell script dead code is gone, and the documentation is thorough.
One gap to be aware of (not blocking): mvn.cmd (the Windows batch launcher) has no --clapp support. The --enc, --shell, and --up sub-commands are handled in mvn.cmd today, but --clapp is only implemented in the Unix mvn script. This could be addressed in a follow-up PR if Windows support is desired.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Updates in latest commit (
|
- Support --clapp <toolname> and --clapp=<toolname> syntax in mvn.cmd - Parse clapp.properties mainClass descriptor on Windows - Inject maven.clapp.name and maven.clapp.mainClass JVM system properties - Set MAVEN_MAIN_CLASS to MavenClappCling when active
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after a37b186
Previous findings status:
| # | Finding | Source | Status |
|---|---|---|---|
| 1 | Double class loading via m2.conf + URLClassLoader | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 2 | URLClassLoader resource leak | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 3 | Path traversal vulnerability | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 4 | _skip_next dead code in shell script |
gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 5 | Stale comment referencing m2.conf | gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 6 | IOException misused for reflection/config errors | elharo | ✅ Addressed in b0314a4 |
| 7 | Missing user-facing documentation | elharo | ✅ Addressed in b0314a4 |
| 8 | Missing mvn.cmd (Windows) --clapp support |
gnodet-bot (review 3) | ✅ Addressed in a37b186 |
All eight previous findings are resolved. The Windows launcher implementation mirrors the Unix script correctly — both --clapp <name> and --clapp=<name> forms are handled, clapp.properties reading is properly implemented via :readClappMainClass, and the system properties are injected into the JVM command line. One minor CMD scripting nit below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
- Check %_PROC_ARG:~8% directly instead of %MAVEN_CLAPP_NAME% inside parenthesized block - Avoid delayed expansion pitfall in Windows batch script
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 11ba540
Previous findings status:
| # | Finding | Source | Status |
|---|---|---|---|
| 1 | Double class loading via m2.conf + URLClassLoader | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 2 | URLClassLoader resource leak | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 3 | Path traversal vulnerability | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 4 | _skip_next dead code in shell script |
gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 5 | Stale comment referencing m2.conf | gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 6 | IOException misused for reflection/config errors | elharo | ✅ Addressed in b0314a4 |
| 7 | Missing user-facing documentation | elharo | ✅ Addressed in b0314a4 |
| 8 | Missing mvn.cmd (Windows) --clapp support |
gnodet-bot (review 3) | ✅ Addressed in a37b186 |
| 9 | CMD delayed expansion pitfall in --clapp= handler |
gnodet-bot (review 4) | ✅ Addressed in 11ba540 |
All nine previous findings are resolved. The fix for #9 is cleaner than what was suggested — instead of just swapping the variable reference, the validation check was moved before the set statements (early-exit pattern), which eliminates the delayed expansion problem entirely and is more idiomatic CMD. Both --clapp <name> and --clapp=<name> handlers now validate-first consistently.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
@elharo Thank you for the review and feedback! To address your questions: 1. To what extent is this user-facing?
2. Documentation UpdatesIn commit
3. Exception HierarchyPer your inline comment, we replaced the use of Could you please re-review and consider approving this PR for merge when you have a moment? Thank you! |
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
Thanks for the docs. That makes a lot of things clearer.
I do think we need to think about and discuss a little more what we want this to look like from a user perspective before jumping into implementation.
… satisfy Checkstyle MethodName rule - Rename 7 test methods to camelCase without underscores - Conform with Checkstyle ^[a-z][a-zA-Z0-9]*$ method name pattern
|
CI checkstyle fix in commit |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 1eadd56
Previous findings status:
| # | Finding | Source | Status |
|---|---|---|---|
| 1 | Double class loading via m2.conf + URLClassLoader | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 2 | URLClassLoader resource leak | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 3 | Path traversal vulnerability | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 4 | _skip_next dead code in shell script |
gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 5 | Stale comment referencing m2.conf | gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 6 | IOException misused for reflection/config errors | elharo | ✅ Addressed in b0314a4 |
| 7 | Missing user-facing documentation | elharo | ✅ Addressed in b0314a4 |
| 8 | Missing mvn.cmd (Windows) --clapp support |
gnodet-bot (review 3) | ✅ Addressed in a37b186 |
| 9 | CMD delayed expansion pitfall in --clapp= handler |
gnodet-bot (review 4) | ✅ Addressed in 11ba540 |
All nine previous findings remain properly resolved.
New commit analysis (1eadd564):
Adds a findEntryPointMethod() resolution chain (run(String[], ClassWorld) → main(String[], ClassWorld) → main(String[])) with invokeEntryPoint() dispatching based on parameter count. The implementation is clean:
- Method fallback cascade is idiomatic Java reflection (try/catch with
ignorednaming convention) invokeEntryPoint()correctly handlesvoidmethods (returns 0),int-returning methods (unboxes), and both 1-param and 2-param variants- Documentation in
README.txtandclapp.mdupdated consistently to documentrun()as preferred,main(...)as compatible - Four new tests cover all three entry-point variants plus negative case
Solid incremental improvement — no issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
@elharo Hi Elliot, thanks again for the thorough review! Here is a summary of the updates made in commit 1eadd56 to address your feedback:
|
|
Thanks for the reviews and feedback! Here is a summary of all updates made to address the review comments:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical launcher and recursion issues, along with unresolved correctness and classpath-isolation findings, block approval.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds Maven CLAPP support for launching third-party CLI tools with per-tool classpaths.
Changes:
- Adds CLAPP dispatch and reflective entry-point loading.
- Extends Unix and Windows launchers with
--clapp. - Adds documentation, distribution layout, and tests.
| File | Summary and review status |
|---|---|
impl/maven-cli/src/test/java/org/apache/maven/cling/MavenClappClingTest.java |
Adds CLAPP tests. Moderate (1 vote): preserve the existing maven.home; nit (2 votes): test loading from an actual fixture JAR. |
impl/maven-cli/src/main/java/org/apache/maven/cling/MavenClappCling.java |
Adds CLAPP loading and invocation. Critical (4 votes): incomplete configuration can recurse infinitely. Moderate (3 votes): parent-first loading does not provide dependency isolation; moderate (1 vote): validate entry-point signatures; moderate (1 vote): strip launcher arguments before invocation. |
api/maven-api-cli/src/site/markdown/index.md |
Adds the CLAPP documentation index entry. |
api/maven-api-cli/src/site/markdown/clapp.md |
Documents CLAPP authoring and usage. |
apache-maven/src/assembly/maven/lib/clapp/README.txt |
Documents the CLAPP directory layout. |
apache-maven/src/assembly/maven/bin/mvn.cmd |
Adds Windows launcher support. Moderate (2 votes): remove CLAPP selector arguments before passing tool arguments. |
apache-maven/src/assembly/maven/bin/mvn |
Adds Unix launcher support. Critical (2 votes): avoid unsafe eval interpolation; moderate (2 votes): remove CLAPP selector arguments before launching. |
apache-maven/src/assembly/component.xml |
Packages the CLAPP directory in distributions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| // Fallback: behave as MavenCling when no CLAPP is configured | ||
| return MavenCling.main(args, world); |
There was a problem hiding this comment.
Fixed in commit ca84dc3: MavenClappCling.main() now explicitly validates mainClass when a CLAPP is specified and fails fast with ClappException if missing. Furthermore, the fallback to standard Maven run now directly calls new MavenCling(world).run(args, null, null, null, false) rather than delegating through MavenCling.main(), completely preventing recursive launcher re-entry.
| try (URLClassLoader clappLoader = new URLClassLoader(jarUrls.toArray(new URL[0]), parentLoader)) { | ||
| Class<?> clazz = clappLoader.loadClass(clappMainClass); |
There was a problem hiding this comment.
Fixed in commit ca84dc3: Replaced URLClassLoader with child-first ClappClassLoader that loads classes from the tool private jars first, while delegating core Java (java., javax., etc.) and Maven API (org.apache.maven.api., org.codehaus.plexus.classworlds.) parent-first to preserve type compatibility with the runtime.
| int exitCode = MavenClappCling.launchClapp( | ||
| "mytool", SampleRunTool.class.getName(), new String[] {"test"}, null); |
There was a problem hiding this comment.
Fixed in commit ca84dc3: Added launchClappLoadsFromJarInClappDirectory() which dynamically compiles a standalone tool class at test time, packages it into a fixture JAR placed in lib/clapp/standalone/, and verifies execution through MavenClappCling.launchClapp(). This ensures entry points and classes are loaded from child-loader jars.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thanks for the contribution, but I don't think this is the right direction for Maven.
Looking at the actual dependency landscape: the only external libraries currently in lib/ that are not needed by Maven core itself are domtrip-core and domtrip-maven, which are used exclusively by mvnup. Everything else — including plexus-sec-dispatcher — is already a Maven core dependency (needed for password decryption in settings, etc.).
But even for domtrip, CLAPP can't help: mvnup uses the full LookupInvoker/Sisu DI stack, so its dependencies need to be in the plexus.core realm. Moving them to a child URLClassLoader would break the DI wiring entirely.
More fundamentally, I don't think we want Maven to be an extension point for third-party self-contained CLI tools. That's not a use case we intend to support, and adding infrastructure for it — a new directory layout, launcher script changes, a new entry point class, a clapp.properties discovery protocol — is a significant API surface to maintain for no current benefit.
If the actual goal is to isolate mvnup's dependencies from the main lib/, the right solution is to split mvnup out of maven-cli into its own module with its own realm in ClassWorlds — not a generic external-tool dispatch mechanism.
…add child-first classloader
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after ca84dc3
Previous findings status:
| # | Finding | Source | Status |
|---|---|---|---|
| 1 | Double class loading via m2.conf + URLClassLoader | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 2 | URLClassLoader resource leak | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 3 | Path traversal vulnerability | gnodet-bot (review 1) | ✅ Addressed in b392595 |
| 4 | _skip_next dead code in shell script |
gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 5 | Stale comment referencing m2.conf | gnodet-bot (review 2) | ✅ Addressed in b0314a4 |
| 6 | IOException misused for reflection/config errors | elharo | ✅ Addressed in b0314a4 |
| 7 | Missing user-facing documentation | elharo | ✅ Addressed in b0314a4 |
| 8 | Missing mvn.cmd (Windows) --clapp support |
gnodet-bot (review 3) | ✅ Addressed in a37b186 |
| 9 | CMD delayed expansion pitfall in --clapp= handler |
gnodet-bot (review 4) | ✅ Addressed in 11ba540 |
| 10 | Missing mainClass causes recursive Maven launcher invocation |
Copilot | ✅ Addressed in ca84dc3 |
| 11 | Parent-first loading breaks CLAPP dependency isolation | Copilot | ✅ Addressed in ca84dc3 |
| 12 | Tests do not verify loading CLAPPs from child-loader jars | Copilot | ✅ Addressed in ca84dc3 |
All twelve previous findings are resolved.
New commit analysis (ca84dc3):
-
Recursion fix — the previous
MavenCling.main(args, world)fallback would checkmaven.mainClassand, if it was still set toMavenClappCling, delegate right back — infinite recursion. Now callsnew MavenCling(world).run(...)directly, bypassing thedelegateMaindispatch. Correct fix. -
Child-first
ClappClassLoader— replaces the parent-firstURLClassLoaderwith a child-first loader that has an explicit parent-first allowlist forjava.*,javax.*,jdk.*,sun.*,org.apache.maven.api.*,org.apache.maven.cling.*, andorg.codehaus.plexus.classworlds.*. The implementation is correct:getClassLoadingLockfor thread safety,findLoadedClasscheck, child-firstfindClasswith parent fallback. -
Missing mainClass guard — now fails fast with
ClappExceptionwhenmaven.clapp.nameis set butmaven.clapp.mainClassis absent, instead of silently falling through toMavenCling. New test covers this. -
JAR loading test —
launchClappLoadsFromJarInClappDirectorydynamically compiles a Java source, packages it into a JAR underlib/clapp/standalone/, and verifies the entry point is loaded from the child classloader. Correctly skips when noJavaCompileris available (headless JRE).
One observation (not blocking): the parent-first allowlist doesn't include org.slf4j.* — if a CLAPP bundles a different SLF4J version, logging objects crossing the classloader boundary would produce ClassCastException. This is probably fine for now since SLF4J bridging is a known classloader pain point in any child-first scheme, but worth noting for the future.
Note: @gnodet has posted a CHANGES_REQUESTED review questioning the overall direction of this PR. The technical implementation in this commit is solid, but the architectural decision about whether Maven should support this extension point is a discussion for the committers.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Update on the PR and review findings: 1. Fixes in Commit
|
|
The right direction here is to use ClassWorlds realms directly, which is exactly what the existing infrastructure was designed for. Rather than a custom Then All realm sections in This is the concrete path forward: close this PR and open a new one that moves |
|
Hi @gnodet, thank you for the review and for sharing the ClassWorlds realm architectural guidance! Before closing this PR, I wanted to respectfully request if the maintainers would consider reviewing and merging this implementation for MNG-8758:
Could the PMC consider this solution for merge, or is the decision final to reject generic CLI tool dispatch in favor of hardcoding Thank you again for your time and feedback! |
|
Let me clarify why CLAPP cannot solve the dependency isolation problem for
CLAPP's child So the claim that CLAPP "isolates domtrip from the main Maven classpath while preserving full compatibility" is not correct for If you believe CLAPP is the right solution, the burden of proof is to make it actually work for a DI-based tool like |
Summary & Rationale
Maven contains multiple standalone CLI entry points (e.g.
mvn,mvnenc,mvnsh,mvnup). Currently, any extra tools and their dependencies must be bundled into Maven's shared${maven.home}/lib/directory, causing lib directory bloat and potential version conflicts among third-party CLI tools.Maven CLAPP (Command Line App) resolves this by allowing extra Maven CLI tools to maintain their private dependencies in an isolated directory structure under
${maven.home}/lib/clapp/<toolName>/.Key Changes Implemented
1. New Entry Point:
MavenClappClingMavenClappCling.javaunderorg.apache.maven.cling.${maven.home}/lib/clapp/<toolName>/for*.jarfiles and constructing a childURLClassLoaderthat inherits from the core Maven class-loader.mainClass) and delegates invocation tomain(String[] args, ClassWorld world).MavenClingif no specific CLAPP tool parameters are provided.2. Configuration & Class-Realm Updates
m2.conf: Addedoptionally ${maven.home}/lib/clapp/${maven.clapp.name}/*.jarso Plexus ClassWorlds includes tool-specific dependencies dynamically whenmaven.clapp.nameis configured.component.xml: Updated the distribution assembly descriptor to includelib/clapp/in binary releases.3. Launcher Script Enhancements
mvn:--clapp <toolName>and--clapp=<toolName>._load_clapp_main_class()function to look upmainClassin${maven.home}/lib/clapp/<toolName>/clapp.properties.-Dmaven.clapp.nameand-Dmaven.clapp.mainClassto the JVM invocation.4. Documentation & Unit Tests
lib/clapp/README.txtexplaining the directory layout andclapp.propertiesformat.MavenClappClingTest.javacovering jar discovery, error conditions, and missing configurations.Fixes [MNG-8758] CLAPP #10316.