Fix #13190: pre-build full reactor once using BUILD_PROJECT, drop temp dir in PluginUpgradeStrategy - #13197

The fix correctly narrows validateEffectiveModel to VALIDATION_LEVEL_MINIMAL for BUILD_EFFECTIVE mode. The root-cause analysis is solid: when mappedSources is empty (no reactor scan), importDependencyManagement silently skips unresolvable sibling BOMs, leaving managed versions absent, and strict validation then cascades false-positive "version is missing" errors on every dependency that relied on that BOM. The importDependencyManagement error already surfaces the real root cause — suppressing the cascade noise is the right call.

One issue blocks merge.

Missing regression test

This is a bug fix with a clearly reproducible scenario (BUILD_EFFECTIVE + unresolvable reactor-sibling BOM → no spurious dependencies.dependency.version is missing errors). Comparable fixes on maven-4.0.x — e.g. #13004 (added 180 lines of DefaultModelBuilderTest) and #13155 — include unit tests. This PR adds none.

A minimal test would:

1. Create a session with ModelBuilderRequest.RequestType.BUILD_EFFECTIVE and recursive=false
2. Build a POM that imports a BOM sibling (which is absent from any repo — so importDependencyManagement records a "Non-resolvable import POM" problem)
3. Assert that the result does not contain dependencies.dependency.version is missing problems — only the root-cause import error

DefaultModelBuilderTest already has BUILD_EFFECTIVE fixtures (see testBuildEffectiveFromResolvedSource at line 972) and can serve as a starting point. The test infrastructure for loading POMs from classpath resources is in place.

Without this test, the fix has no guard against future regressions.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after approach refinement (MINIMAL → MAVEN_2_0).

The level change is an improvement over the previous commit. VALIDATION_LEVEL_MINIMAL skips all effective model validation including structural checks (modelVersion, groupId, artifactId). VALIDATION_LEVEL_MAVEN_2_0 preserves those structural ERRORs and demotes only the version-missing check to WARNING (because errOn30 = getSeverity(MAVEN_2_0, MAVEN_3_0) returns Severity.WARNING when MAVEN_2_0 < MAVEN_3_0). That is the precise semantic you need — structural failures still block the build, cascade false-positives do not.

However, the original blocker remains unaddressed.

Missing regression test (prior finding — not addressed)

The prior review requested a test that reproduces the scenario: BUILD_EFFECTIVE + unresolvable reactor-sibling BOM import → no spurious dependencies.dependency.version is missing errors.

The new commit changes only DefaultModelBuilder.java — no test was added.

The scenario is straightforwardly testable in DefaultModelBuilderTest, which already has BUILD_EFFECTIVE infrastructure (testBuildEffectiveWithNullRootDirectory at line 983 shows the pattern). A minimal test:

@Test
void testBuildEffectiveDoesNotCascadeVersionMissingOnUnresolvableBomImport() {
    // POM has a dep on a BOM-managed artifact (no version) and imports the BOM
    // The BOM is a reactor sibling — not present in any repo — so importDependencyManagement
    // records "Non-resolvable import POM" but BUILD_EFFECTIVE skips the reactor scan.
    Path pomPath = getPom("build-effective-bom-missing"); // new fixture
    ModelBuilderRequest request = ModelBuilderRequest.builder()
            .session(session)
            .requestType(ModelBuilderRequest.RequestType.BUILD_EFFECTIVE)
            .recursive(false)
            .source(ModelSource.fromPath(pomPath))
            .build();
    ModelBuilderResult result = builder.newSession().build(request);
    List<ModelProblem> problems = result.getProblems();
    // Must have the root-cause error (Non-resolvable import POM)
    assertTrue(problems.stream().anyMatch(p -> p.getMessage().contains("Non-resolvable")));
    // Must NOT have any ERROR-severity "version is missing" cascade
    assertTrue(problems.stream()
            .filter(p -> p.getSeverity() == ModelProblem.Severity.ERROR)
            .noneMatch(p -> p.getMessage().contains("version") && p.getMessage().contains("missing")));
}

Without this test, the fix has no guard against future regressions. When a future refactor re-merges BUILD_EFFECTIVE back into VALIDATION_LEVEL_STRICT (e.g. as part of the TODO 4.1.0 task mentioned in the comment), the cascade will silently return.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after the push that renamed the variable and updated comments. Two issues remain — one new.

[1] The fix mechanism contradicts the code — VALIDATION_LEVEL_MAVEN_2_0 does NOT suppress the version-missing cascade

The PR description states:

The version-missing check only becomes an ERROR at >= VALIDATION_LEVEL_MAVEN_3_0 (via getSeverity); at VALIDATION_LEVEL_MAVEN_2_0 it is a WARNING.

This is incorrect. The version-missing check lives in validateDependencyVersion (line 1499 of DefaultModelValidator.java):

protected void validateDependencyVersion(ModelProblemCollector problems, Dependency d, String prefix) {
    validateStringNotEmpty(
            prefix, "version", problems,
            Severity.ERROR, Version.BASE,  // ← hardcoded ERROR at BASE, no level gate
            d.getVersion(), ...);
}

validateDependencyVersion is called from validateEffectiveDependency, which is called from validateEffectiveDependencies unconditionally — outside any validationLevel >= MAVEN_2_0 guard:

for (Dependency d : dependencies) {
    validateEffectiveDependency(problems, d, management, prefix, validationLevel); // ← no level guard
    if (validationLevel >= ModelValidator.VALIDATION_LEVEL_MAVEN_2_0) { ... }      // ← guard is here, but too late
}

So validateDependencyVersion fires Severity.ERROR for a null version at all validation levels — MINIMAL, MAVEN_2_0, and STRICT alike. Changing the level from STRICT to MAVEN_2_0 does not suppress the cascade. Worse: MAVEN_2_0 activates the >= MAVEN_2_0 block that adds validateBoolean(optional), format checks on version strings, and scope validation — strictly more checks than MINIMAL.

The actual suppression of the cascade (for the BUILD_EFFECTIVE non-reactor case) requires either:

• Making validateDependencyVersion respect the validation level (e.g. treat missing version as WARNING at < MAVEN_3_0), or
• Guarding the validateEffectiveDependency call with if (validationLevel >= MAVEN_2_0) so MINIMAL actually skips it, or
• Skipping validateEffectiveModel altogether for BUILD_EFFECTIVE (since by definition it has no reactor context to satisfy the invariants effective-model validation assumes)

If the author observed the fix working in practice, the mechanism must be elsewhere — the commit message says mvn verify -pl impl/maven-impl -am -B passes, but that runs BUILD_PROJECT, not BUILD_EFFECTIVE. The issue may not be reliably reproduced by that test.

[2] Missing regression test (carried over from the previous review)

No test was added in this push. The scenario is reproducible:

1. BUILD_EFFECTIVE request with recursive=false
2. POM that imports a BOM from a reactor sibling (absent from any repo)
3. Assert: result contains the "Non-resolvable import POM" error, but not dependencies.dependency.version is missing errors

DefaultModelBuilderTest.testBuildEffectiveFromResolvedSource (line 972) and its classpath-resource fixture infrastructure are the right starting point.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

The approach has changed completely from the previous iterations — switching from patching DefaultModelBuilder validation levels to fixing the call site in mvnup itself. This is the right direction: using BUILD_CONSUMER with a shared session is semantically cleaner than BUILD_EFFECTIVE, and the maven-4.0.x base already carries the model-builder fix (isBuildRequest() / isFilesystemRequest() / VALIDATION_LEVEL_MAVEN_2_0 for BUILD_EFFECTIVE) separately.

Two issues remain.

[1] Javadoc on sharedModelBuilderSession overstates the mechanism

The field Javadoc says:

version/groupId inference to work correctly: when a child POM omits <version> or <groupId> on a reactor sibling dependency, the model builder resolves them from the reactor's raw-model cache populated by earlier calls in the same session.

This is inaccurate. The "reactor's raw-model cache" that inferDependencies() / resolveReactorModel() consults is mappedSources — populated exclusively by putSource(), which is called only inside loadFilePom(), which is only reachable from buildBuildPom() (the BUILD_PROJECT path). BUILD_CONSUMER requests never call putSource(), so mappedSources remains empty regardless of how many POMs are built through the shared session.

The actual benefit of the shared ModelBuilderSessionImpl is:

1. Parse cache: the Maven Session-level RequestCache (cache(source, FILE, …)) deduplicates POM file reads — each filesystem path is parsed at most once per Maven Session, regardless of how many ModelBuilderSession instances are created.
2. dag sharing: the directed-acyclic-graph for cycle detection is shared via deriveTopLevel, preventing false cycle errors when the same POM is transitively reachable from multiple roots.

Neither benefit is "reactor sources accumulated in mappedSources". The Javadoc should describe what actually happens — e.g. "each POM is parsed at most once per apply invocation (RequestCache deduplication), and the shared DAG prevents false cycle-detection errors on re-entrant parent lookups."

[2] Missing regression test (carried over — three reviews now)

This is the third review requesting a regression test for the original bug scenario. The commit message references mvn verify -pl impl/maven-impl -am -B passes — that module contains DefaultModelBuilderTest but does not exercise AbstractUpgradeStrategy.buildEffectiveModel() with a BOM-managed reactor sibling.

A minimal test in AbstractUpgradeGoalTest or a dedicated AbstractUpgradeStrategyTest:

1. Set up a two-POM reactor: a BOM POM (groupId:bom-artifact:1.0) and a consumer POM that imports the BOM in <dependencyManagement> and uses a BOM-managed dependency without a <version>.
2. Call buildEffectiveModel(context, consumerPomPath) (or exercise a strategy that calls it).
3. Assert: the result does NOT throw ModelBuilderException with "version is missing" for the BOM-managed dependency.

The test infrastructure already exists in AbstractUpgradeGoalTest (see createTempProject() + setupMultiModuleProject() helpers). This test would catch any future regression where BUILD_CONSUMER + strict validation re-introduces cascade errors (e.g. if a future refactor changes when putSource() is called or alters the validation path).

Without this test, the correctness guarantee is "it works because the base branch has a model-builder fix" — not "it works because the caller is correct". Those are different invariants, and the latter is what this PR claims to establish.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of the fourth approach: BUILD_PROJECT reactor pre-build + effectiveModelCache + shared session fallback.

The BOM inference bug is fixed — BUILD_PROJECT calls loadFromRoot, which calls loadFilePom for every module, and putSource() (line 1079 of DefaultModelBuilder) populates mappedSources regardless of recursive. Subsequent BUILD_EFFECTIVE calls on the shared session inherit that populated map. That is sound.

Two issues remain.

[1] getChildren() is always empty — cache only stores root POM

ModelBuilderRequest.recursive defaults to false (Java primitive boolean). The child-linking gate in DefaultModelBuilder.loadFilePom is:

if (request.isRecursive()) {
    r.getChildren().add(cr);
}

Because the request in prebuildReactorModels does not call .recursive(true), getChildren() returns an empty list on the result. allResults() therefore only sees the root, and effectiveModelCache ends up with exactly one entry for a multi-module project. Every non-root POM misses the cache and falls back to BUILD_EFFECTIVE — which does work correctly because mappedSources is populated — but the cache is useless and the Javadoc that claims it captures all modules is wrong.

Note that buildBuildPom() internally uses its own results() traversal (also getChildren()-based) to build effective models for all results; without recursive=true that pass also only covers root, so child effective models are not pre-built either. Adding .recursive(true) to the request fixes both the cache and that pass.

[2] Regression test still missing (4th request)

All four prior reviews asked for a test that exercises the BOM-managed version path: a multi-module project where a child POM omits <version> for a dependency managed by a sibling BOM, and buildEffectiveModel() must not surface a "version is missing" error. The test would also prove that mappedSources is populated — i.e. that the fix actually fixes #13190. There is still no such test.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after the approach pivot to BUILD_PROJECT + reactor pre-build.

The direction is correct and the Javadoc is now accurate. One issue from previous reviews is addressed; one is not, and there is one new nit.

[1] ADDRESSED: Javadoc accuracy on sharedModelBuilderSession. Previous review flagged that it incorrectly claimed BUILD_CONSUMER accumulates mappedSources. The code now uses BUILD_PROJECT which does populate mappedSources, and the BUILD_EFFECTIVE fallback uses deriveTopLevel which propagates the same mappedSources map. Claim is accurate.

[2] NOT ADDRESSED: Missing regression test (fourth review). Still no test for the original BOM-managed dep scenario. Without it, the correctness guarantee is 'the base branch has a model-builder fix' rather than 'the call site is correct'.

[3] NEW: prebuildReactorModels should be private. See inline comment.

This review was generated by an AI agent, Hermes on behalf of @gnodet.

Re-review after the approach change to BUILD_PROJECT pre-build + temp-dir elimination.

The approach is now correct and well-structured. The previous finding [1] (Javadoc overstating mappedSources not being shared) is withdrawn: deriveTopLevel() calls the private derive() method, which passes this.mappedSources directly to the full constructor — the map IS shared across build() calls on the same ModelBuilderSessionImpl. The Javadoc is accurate.

One issue remains.

Missing regression test (fourth review — not addressed)

The test changes in PluginUpgradeStrategyTest.java adapt two existing tests to write real temp files (necessary because the temp-dir bridge is gone). They do not add a test for the actual fix.

The core fix is:

• prebuildReactorModels runs BUILD_PROJECT on the root POM, which calls loadFromRoot → loadFilePom → putSource for every reactor member, populating mappedSources.
• Subsequent buildEffectiveModel calls serve from cache (fast path) or fall back to BUILD_EFFECTIVE on the same ModelBuilderSessionImpl, which shares the populated mappedSources via deriveTopLevel → derive → this.mappedSources in the full constructor.
• This ensures coordinate inference (inferParentVersion, inferDependencyVersion, inferDependencyGroupId) works for reactor siblings.

A minimal test in AbstractUpgradeGoalTest or a dedicated AbstractUpgradeStrategyTest:

@Test
void testPrebuildReactorModelsPopulatesEffectiveModelCache() throws Exception {
    // Two-module reactor: parent declares a BOM-managed dep version;
    // child omits <version> relying on BOM management.
    Path tempDir = Files.createTempDirectory("mvnup-prebuild-test-");
    try {
        Files.createDirectories(tempDir.resolve(".mvn"));
        // parent pom.xml
        Path parentPom = tempDir.resolve("pom.xml");
        Files.writeString(parentPom, """
            <?xml version="1.0" encoding="UTF-8"?>
            <project xmlns="http://maven.apache.org/POM/4.0.0">
              <modelVersion>4.0.0</modelVersion>
              <groupId>test.group</groupId>
              <artifactId>parent</artifactId>
              <version>1.0</version>
              <packaging>pom</packaging>
              <modules><module>child</module></modules>
            </project>
            """);
        // child pom.xml — version omitted (inferred from parent via mappedSources)
        Path childDir = tempDir.resolve("child");
        Files.createDirectories(childDir);
        Path childPom = childDir.resolve("pom.xml");
        Files.writeString(childPom, """
            <?xml version="1.0" encoding="UTF-8"?>
            <project xmlns="http://maven.apache.org/POM/4.0.0">
              <modelVersion>4.0.0</modelVersion>
              <parent>
                <groupId>test.group</groupId>
                <artifactId>parent</artifactId>
                <version>1.0</version>
              </parent>
              <artifactId>child</artifactId>
            </project>
            """);

        // Exercise prebuildReactorModels via apply() on a concrete strategy
        // (or expose it as protected for direct testing)
        Document parentDoc = Document.of(Files.readString(parentPom));
        Document childDoc = Document.of(Files.readString(childPom));
        Map<Path, Document> pomMap = Map.of(parentPom, parentDoc, childPom, childDoc);

        UpgradeContext context = TestUtils.createMockContext(tempDir);
        // The strategy.apply() call triggers prebuildReactorModels before doApply
        UpgradeResult result = strategy.apply(context, pomMap);
        assertTrue(result.success(), "apply() should succeed — no version-missing errors from inference");
    } finally {
        try (var walk = Files.walk(tempDir)) {
            walk.sorted(java.util.Comparator.reverseOrder()).map(Path::toFile).forEach(java.io.File::delete);
        }
    }
}

This test would have caught the original bug (a fresh-session-per-POM BUILD_EFFECTIVE left mappedSources empty, causing inference to return null) and will catch future regressions if the pre-build strategy changes.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after the approach change to BUILD_PROJECT pre-build + temp-dir elimination.

Two of the three findings from the previous review are not addressed; one nit is still open.

[1] NOT ADDRESSED — getChildren() is still empty: cache only stores the root POM

The previous review flagged that prebuildReactorModels never calls .recursive(true), so getChildren() returns an empty list and allResults() only traverses the root. This is still true in the new commit: the ModelBuilderRequest.builder() block in prebuildReactorModels does not call .recursive(true). In DefaultModelBuilder.loadFilePom, the child-linking gate is:

if (request.isRecursive()) {
    r.getChildren().add(cr);
}

Since recursive defaults to false (it is a plain boolean field), getChildren() returns an empty list regardless of how many modules the reactor has. allResults() therefore only processes the root, and the cache is populated with exactly one entry for a multi-module project.

The fix still works because putSource() in loadFilePom is called unconditionally (no isRecursive() gate), so mappedSources is correctly populated for all reactor modules. Subsequent BUILD_EFFECTIVE calls on the shared session inherit that map via deriveTopLevel. But the cache optimization is a no-op for every non-root POM, and the Javadoc that says "produces child results (via getChildren()) that map 1-to-1 to the reactor modules" remains inaccurate — see inline comment.

Fix options:

• Add .recursive(true) to the BUILD_PROJECT request — cache then covers all modules, Javadoc becomes accurate.
• Or drop the cache claim from the Javadoc: say "the root POM only; child POMs fall back to BUILD_EFFECTIVE on the shared session (which works because mappedSources was populated by the BUILD_PROJECT pass)" and let the fallback do its job.

[2] NOT ADDRESSED — Missing regression test (fifth review)

The test changes adapt two existing tests to write real temp files — necessary because the temp-dir bridge is gone. Both still call strategy.doApply(context, pomMap). doApply is the abstract template implementation: it does not invoke prebuildReactorModels. The entire apply() → prebuildReactorModels → mappedSources populated → buildEffectiveModel cache-hit / shared-session fallback chain is completely untested.

A minimal test via strategy.apply() (not doApply) on a two-module reactor where a child omits <version> for a BOM-managed dependency would cover the actual fix path — see the concrete example in the previous review.

[3] NIT (carried over) — prebuildReactorModels should be private

No subclass overrides it; it is an implementation detail of apply(). Flagged in the previous review as a nit, still protected.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after the latest push (c93393f) — all three findings from the previous review are addressed.

[1] ADDRESSED — .recursive(true) added to BUILD_PROJECT request

prebuildReactorModels now sets .recursive(true) on the ModelBuilderRequest. The if (request.isRecursive()) gate in DefaultModelBuilder.loadFilePom will link child results to getChildren(), so allResults() traverses the full reactor tree and effectiveModelCache is populated for every module in one pass.

[2] ADDRESSED — Regression test added in AbstractUpgradeStrategyTest

The new applyOnMultiModuleProjectSucceedsWithoutVersionMissingCascade test calls strategy.apply(context, pomMap) (not doApply) on a two-module reactor where the child omits <version> for its own artifact coordinate, relying on inference via mappedSources. This exercises the full apply() → prebuildReactorModels → mappedSources populated → cache hit path — the exact mechanism the fix establishes.

[3] ADDRESSED — prebuildReactorModels is now private

No subclass can accidentally shadow or re-invoke it mid-flight.

The fix is correct and complete. BUILD_PROJECT + recursive=true populates mappedSources for every reactor member; the shared ModelBuilderSessionImpl propagates that map to subsequent BUILD_EFFECTIVE fallback calls via deriveTopLevel; the cache eliminates redundant model builds for non-root POMs.

This review was generated by an AI agent, Hermès on behalf of @gnodet.