Repository navigation
Conversation
fa3f2f9 to
e9a2232
Compare
e9a2232 to
27e1395
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Review: [MNG-12537] reactor.xml + build aliases + lifecycle filter
Well-structured three-part feature. The API design (noun-style, @Experimental, @Immutable), the backwards-compat strategy (legacy files still read when reactor.xml is absent), and the test coverage are solid. Two issues need addressing before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: [MNG-12537] reactor.xml — parent attribute + LifecycleProcessor SPI
The new commit (43d7fe3) adds the parent attribute to PhaseInjection, the LifecycleProcessor SPI, ReactorXmlLifecycleProcessor, and PhaseEnrichedLifecycle — good additions. However two of the three issues from the previous review are still unaddressed, and the new LifecycleProcessor Javadoc introduces two new inaccuracies.
Inline comments below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: reactor.xml — checkstyle fix only, three findings still open
The latest commit (dc26c1e3) is a pure checkstyle/NeedBraces fix in the test class — no other changes. All five findings from the previous two reviews remain unaddressed. Inline comments below repeat the three distinct issues (the two pairs target two separate locations each).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: three fixes land cleanly, one finding partially addressed
The new commit (1802942432) addresses all five previously raised findings — three are fully resolved, but one (the LifecycleProcessor Javadoc example) is only partially fixed and still carries an SPI/impl boundary problem.
Status of previous findings:
✅ reactor.mdo — aliases field description: "first non-flag argument" → "Every argument on the command line is checked in order" — fixed.
✅ reactor.mdo — Alias class description: same "first non-flag argument" wording — fixed.
✅ MavenParser.java line 188 — e.getCause() (returns null for ParseException) → e — fixed.
✅ LifecycleProcessor.java — "Processors are called once per lifecycle per session" replaced with accurate stateless/idempotent description — fixed.
LifecycleProcessor.java — Javadoc example now calls LifecycleProcessor.withInjectedPhase() (non-existent method) → replaced by new PhaseEnrichedLifecycle(…). The method reference is gone, but the replacement is itself problematic — see inline comment.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: reactor.xml — commit 1e9fc259 — one new inaccuracy
The three previous findings are all addressed:
- ✅
reactor.mdoalias descriptions — corrected to "Every argument on the command line is checked" - ✅
MavenParser.javaparseMavenReactorOptions—e.getCause()→e(the pre-existing pattern in the sibling methods is out of scope) - ✅
LifecycleProcessor— "once per session" andwithInjectedPhase()both fixed - ✅
PhaseEnrichedLifecycleinternal class removed from SPI Javadoc
However 1e9fc259 introduces one new inaccuracy in the replacement text: it says ReactorXmlLifecycleProcessor is in impl/maven-core, but the class lives in impl/maven-cli. One-line fix.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: reactor.xml — all previous findings addressed; two new issues in filter parsing
All five findings from the last review are fully resolved in the squashed commits:
- ✅
reactor.mdoalias descriptions — "Every argument on the command line is checked in order" (bothaliasesfield andAliasclass) - ✅
MavenParser.parseMavenReactorOptions—e.getCause()→e(ParseException now preserved as cause) - ✅
LifecycleProcessor— "called each time the lifecycle phase map is rebuilt", stateless/idempotent - ✅
LifecycleProcessorJavadoc example — phantomwithInjectedPhase()removed, clean reference toReactorXmlLifecycleProcessor - ✅
ReactorXmlLifecycleProcessormodule reference — correctly saysimpl/maven-cli
Two new issues in the filter parsing logic need fixing before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: merge-up commit — both previous findings still unaddressed
The new commit (dde08d5f) is a merge from origin/master into feature/reactor-xml — no PR code was changed. Both findings from the last review remain open.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: commit 63ce016f — both previous findings addressed; one documentation gap remains
Both findings from the last review are addressed in this commit:
- ✅
phase()empty/blank validation —isBlank()guard added, two new tests (emptyPhaseParensThrows,blankPhaseParensThrows) cover the cases. - ✅ G:A dotless-groupId limitation — Javadoc updated at both the class and
parse()levels;dotlessGroupIdIsMisroutedToPrefixModetest documents the misrouting and the:Aworkaround.
One gap remains from the original finding that the Javadoc update does not cover.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: commit 54de779c — both previous findings addressed; ready to merge
Both inline findings from the last review are fully resolved in this commit:
- ✅ Dotted-prefix misrouting documentation —
CoordinatePredicate.javaclass-level Javadoc now documents the symmetric case: thePbullet explicitly states the prefix must not contain a'.'and that dotted prefixes (e.g.io.smallrye) are routed to G:A matching instead. Matches the suggestion block exactly. - ✅ Inaccurate test comment —
dotlessGroupIdIsMisroutedToPrefixModenow correctly states that the prefix check succeeds (goalPrefix="commons-io"matches) but the version check fails ("commons-io" != "2.15.1"). The misleading implication that the prefix would not match is gone.
No new issues introduced — this is a pure documentation and comment fix. The PR is ready to merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
f12d888 to
05045ab
Compare
Introduce .mvn/reactor.xml, a single file that consolidates CLI options (replaces .mvn/maven.config), core extensions (replaces .mvn/extensions.xml), named build aliases, and lifecycle phase injections. Legacy files continue to work when reactor.xml is absent. Named build aliases: short names defined in reactor.xml that expand to a full argument list before any Maven processing starts. Expansion is non-recursive; flag-style aliases (e.g. --skip-tests) are supported. Lifecycle phase injections: custom phases declared in reactor.xml with a parent attribute for precise placement in Maven 4's hierarchical lifecycle tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
05045ab to
32ba46b
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review of the squashed commit (32ba46bc). All 21 prior review threads are resolved — the squash incorporates all previous fixes cleanly. Two new issues in the squashed content need addressing before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…icate alias names
gnodet-bot
left a comment
There was a problem hiding this comment.
Three low-severity observations on the new reactor.xml feature.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…usivity, remove dead parameter
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all five findings from the two previous reviews have been addressed.
From the 06:15 review:
- ✅
ReactorExtension.configurationsilently dropped —.configuration(re.getConfiguration())added to theCoreExtensionbuilder inBaseParser.java - ✅ Duplicate alias names silently overwritten —
expandAlias()now throwsIllegalArgumentExceptionfor duplicate names, with a matchingduplicateAliasNameThrows()unit test
From the 07:22 review:
- ✅ Stale class name —
ReactorXmlPhaseInjector→ReactorXmlLifecycleProcessorin theMavenInvoker.javacomment - ✅ Dead parameter
ignoredLinkHint— removed frominsertAmongSiblings()and all call sites - ✅
options/optionArgsmutual exclusivity not enforced —resolveReactorOptions()now throws if both are set
One gap in the mutual-exclusion fix: the duplicate-alias fix came with a dedicated unit test (duplicateAliasNameThrows), but the analogous mutual-exclusion check in resolveReactorOptions() has no unit test. resolveReactorOptions() is private and called from parseCliOptions(), so it can't be exercised directly from MavenParserAliasTest — but a test that calls MavenParser.expandAlias() with a ReactorConfig carrying both options and optionArgs set would cover it if resolveReactorOptions() is called from that path. It isn't — resolveReactorOptions() is a different code path (option resolution, not alias expansion). The gap is real but low severity since the behavior is correct; only coverage is missing.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Make resolveReactorOptions() package-private to allow direct testing from MavenParserAliasTest. Add four tests covering inline options, structured optionArgs, empty config, and the mutual-exclusion guard (both fields set must throw IllegalArgumentException).
gnodet-bot
left a comment
There was a problem hiding this comment.
Previous findings are all addressed. LGTM.
- ✅
resolveReactorOptions()mutual-exclusion gap — method made package-private; four unit tests added covering inline<options>, structured<optionArgs>, empty config, and the mutual-exclusion throw.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
The reactor.xml alias feature had IT coverage but custom phase injection (the <phases>/<phase> section) was only covered by unit tests. Add an end-to-end IT that: - defines a custom phase in .mvn/reactor.xml - binds maven-it-plugin-touch to that phase - verifies the plugin executes during verify - verifies the custom phase works as a direct CLI target - verifies ordering (plugin does not run before its phase) - verifies log ordering (runs after compile) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The concurrent builder (BuildPlanExecutor) used LifecycleRegistry directly to resolve phases, bypassing the LifecycleProcessor SPI chain that DefaultLifecycles applies. This meant custom phases injected via reactor.xml were invisible to the concurrent builder, causing "Unknown lifecycle phase" errors. Inject List<LifecycleProcessor> into BuildPlanExecutor and apply the processor chain to each lifecycle before phase resolution, matching the behavior of DefaultLifecycles.applyProcessors(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all previous findings are addressed.
The new commit (e01415560c) applies the LifecycleProcessor chain inside BuildPlanExecutor so that custom phases from reactor.xml are visible to the concurrent build planner. Four call sites are patched: calculateLifecycleMappings, getResolvedPhase, computeForkPlan (two spots). This is the correct fix — without it, phases injected by ReactorXmlLifecycleProcessor would be present in DefaultLifecycles (used by the legacy plan calculator) but invisible to BuildPlanExecutor, causing the injected phase to silently disappear during plan execution.
One low-severity observation on the ordering test.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Replace the fragile log-line scan (matching any 'maven-compiler-plugin' + 'compile' line) with the '[INFO] ---' execution banner pattern which only appears at actual phase-execution time, not during plan calculation. Also restrict the touch plugin anchor to 'touch-in-custom-phase' (the execution id) rather than also matching 'pre-integration-marker.txt' which could appear in other log contexts.
…overed
ReactorXmlLifecycleProcessor used javax.inject annotations but
Maven 4's DI container (maven-di) discovers components via
DiIndexProcessor which only scans for @org.apache.maven.api.di.Named.
Switch to maven-di annotations so the class appears in the
META-INF/maven/org.apache.maven.api.di.Inject service file.
Also fix the IT log assertion to match Maven 4's execution banner
format ("compiler:3.16.0:compile" not "maven-compiler-plugin").
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the annotation fix is correct and the previous finding is addressed.
The switch from javax.inject to org.apache.maven.api.di annotations is the right fix: maven-di's DiIndexProcessor only scans for @org.apache.maven.api.di.Named, so ReactorXmlLifecycleProcessor was invisible to the DI container and never injected. Removing @Singleton is correct — maven-di manages the singleton lifecycle automatically for @Named components without needing the explicit annotation.
One residual issue in the test log-matching pattern.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
The broad 'compiler && :compile' substring scan (reintroduced in the last commit) can match plan-calculation log lines that appear before the compiler goal actually executes, making the ordering assertion trivially true even if ordering were wrong. Re-add '[INFO] ---' as the anchor (as originally fixed in 8ee1f0a) so the index is captured only at actual phase-execution time. Also restore the more descriptive assertion message.
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the previous finding is addressed.
✅ Test log-matching pattern — [INFO] --- anchor added back: line.contains("[INFO] ---") && line.contains("compiler") && line.contains(":compile") correctly scopes the match to actual execution banners only, not planning/debug lines. The assertion message is also updated to match ("compiler plugin execution banner must appear in log"). Exact fix requested.
The PR is in good shape.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Both `after` and `before` attributes on `<phase>` elements in reactor.xml now accept comma-separated lists of sibling phase names and can be specified together to constrain insertion from both sides. When multiple `after` anchors are given, the phase is inserted after the latest (rightmost) one. When multiple `before` anchors are given, it is inserted before the earliest (leftmost) one. Conflicting constraints throw IllegalStateException. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the previous finding is addressed. The new commit (b99246464a) correctly extends insertAmongSiblings (Maven 4 tree) to handle comma-separated after/before lists with proper min/max index resolution. One correctness gap in the v3 tree path.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| String primaryAnchor = !afterAnchors.isEmpty() | ||
| ? afterAnchors.get(afterAnchors.size() - 1) | ||
| : (!beforeAnchors.isEmpty() ? beforeAnchors.get(0) : null); |
There was a problem hiding this comment.
before-only comma list: v3 tree positioning uses string order, not lifecycle order
primaryAnchor = beforeAnchors.get(0) picks the first element in the comma-separated string. But the intended semantic is the earliest in lifecycle order — which is what insertAmongSiblings correctly implements (i < upperBound). When string order and lifecycle order differ, the two trees disagree.
For before="integration-test,unit-test" (reversed order):
beforeAnchors.get(0)="integration-test"(later in lifecycle, index ~8)tryInjectV3Siblingfindsintegration-testand inserts BEFORE it in the v3 listaddPhasesadds sibling-ordering edges:AFTER(unit-test) → BEFORE(new-phase), placingnew-phaseAFTERunit-test- Result: phase ends up between
unit-testandintegration-test, violating "before unit-test"
insertAmongSiblings (Maven 4 tree) correctly resolves this by scanning for the minimum-index anchor (i < upperBound). injectV3 should do the same — find the earliest lifecycle-positioned anchor, not the first string token.
Fix: make injectV3 mirror insertAmongSiblings's logic for the before case — find the anchor that appears earliest in the flattened v3 phase list. The simplest fix is to pass all before anchors to tryInjectV3Sibling and scan rather than assuming string position order, or (simpler) pick the minimum by scanning phases once before calling into the tree search.
| int newIdx = computed.indexOf("pre-all"); | ||
| assertTrue(unitTestIdx >= 0, "'unit-test' must appear"); | ||
| assertTrue(newIdx >= 0, "'pre-all' must appear"); | ||
| assertTrue(newIdx < unitTestIdx, "'pre-all' must come before 'unit-test'"); |
There was a problem hiding this comment.
before= anchor order not covered
before="unit-test,integration-test" happens to be in lifecycle order so get(0) = "unit-test" (correct). Add a test with reversed order to pin the behavior:
@Test
void commaSeparatedBeforeAnchorsUsesEarliestRegardlessOfStringOrder() {
Lifecycle base = defaultLifecycle();
// before="integration-test,unit-test" — reversed string order, unit-test is still the earliest
PhaseEnrichedLifecycle enriched =
new PhaseEnrichedLifecycle(base, List.of(ip("pre-all", "verify", null, "integration-test,unit-test")));
List<String> computed = registry().computePhases(enriched);
int unitTestIdx = computed.indexOf("unit-test");
int newIdx = computed.indexOf("pre-all");
assertTrue(unitTestIdx >= 0, "'unit-test' must appear");
assertTrue(newIdx >= 0, "'pre-all' must appear");
assertTrue(newIdx < unitTestIdx, "'pre-all' must come before 'unit-test' regardless of string order");
}
What
Introduces
.mvn/reactor.xml, a single file that consolidates reactor-scoped build configuration:.mvn/maven.config.mvn/extensions.xmlmvn ci→mvn verify -Dmaven.test.skip=true)Legacy files continue to work when
reactor.xmlis absent — full backwards compatibility.Example
Syntax variants
Options and alias content each support two mutually exclusive forms:
<arg>elements)<options>-T4 --no-transfer-progress</options><arg>-T4</arg><arg>--no-transfer-progress</arg>(flat under<reactor>)<content>verify -Pci</content><arg>verify</arg><arg>-Pci</arg>(inside<alias>)The inline form is parsed by
ArgumentTokenizerwith shell-style quote awareness (single/double quotes, backslash escapes). The structured form takes each<arg>as a literal token — use it when values contain spaces or special characters.Extensions support the same fields as
.mvn/extensions.xml:groupId,artifactId,version(all required), plus optionalclassLoadingStrategy(self-first|parent-first|plugin, defaultself-first) and freeformconfigurationXML.Why
Today reactor-scoped configuration is scattered across multiple files with different formats:
.mvn/maven.config— flat text, no structure, no way to define conditional or named configurations.mvn/extensions.xml— separate XML file just for extensionsThere is no standard way to define project-level command shortcuts (every team reinvents shell aliases or Makefiles), and no way to inject custom lifecycle phases without writing a full extension.
reactor.xmlunifies these into a single, schema-backed file with clear semantics and room to grow.How
Model:
reactor.mdo(maven-api-core)The
reactor.xmlschema is defined as a Modello model (api/maven-api-core/src/main/mdo/reactor.mdo), generatingReactorConfig,Alias,PhaseInjection, andExtensiontypes inorg.apache.maven.api.reactor, plus a StAX reader/writer.Parsing:
BaseParser+MavenParser(maven-cli)BaseParser.readReactorConfig()reads.mvn/reactor.xmlearly — before CLI option parsing — so the config is available for alias expansion. TheReactorConfigis stored onInvokerRequestand passed through the invoker chain.MavenParser.expandAlias()performs alias expansion: each CLI argument is checked against the alias map and replaced by its expansion tokens. Expansion is non-recursive (tokens produced by an alias are not re-expanded).ArgumentTokenizerhandles shell-style quote parsing for<content>strings.When
reactor.xmlis present, its<options>block replaces.mvn/maven.config; its<extensions>block replaces.mvn/extensions.xml. When absent, legacy files are read as before.SPI:
LifecycleProcessor(maven-api-spi)New
@ConsumerSPI interface with a single method:Called for each built-in (or extension-contributed) lifecycle at session init. Multiple processors chain — each receives the output of the previous one. Implementations must be stateless and idempotent.
Phase injection:
PhaseEnrichedLifecycle+ReactorXmlLifecycleProcessor(maven-core / maven-cli)ReactorXmlLifecycleProcessorimplementsLifecycleProcessorby reading<phase>declarations fromreactor.xmland delegating toPhaseEnrichedLifecycle, which inserts phases into Maven 4's hierarchical lifecycle DAG. Theparentattribute names the container phase;after/beforeanchor the insertion among its children.DefaultLifecyclesapplies allLifecycleProcessorinstances when building the lifecycle phase map.Files changed
reactor.mdo→ generatesReactorConfig,Alias,PhaseInjectionLifecycleProcessor.javaBaseParser(read reactor.xml),MavenParser(alias expansion, options),ArgumentTokenizerReactorXmlLifecycleProcessor,PhaseEnrichedLifecycle,DefaultLifecyclesInvokerRequest.reactorConfig()ArgumentTokenizerTest,MavenParserAliasTest,ReactorXmlLifecycleProcessorTest,PhaseEnrichedLifecycleTestMavenITmng12537ReactorXmlAliasTestTest plan
MavenITmng12537ReactorXmlAliasTest🤖 Generated with Claude Code