[MNG-5359] Do not activate pluginManagement executions through lifecycle bindings

[goutamadwant]

Fixes #6918.

Problem

Lifecycle-binding injection can activate a managed execution from a different lifecycle even when the plugin is not declared in build/plugins. For example, a managed clean-plugin execution bound to package runs during mvn package.

Change

• Filter cross-lifecycle managed executions when a plugin is introduced only by lifecycle bindings.
• Preserve managed executions in the same lifecycle, executions without an explicit phase, and normal plugin-management merging for explicitly declared plugins.
• Preserve managed version, configuration, dependencies, and input locations.
• Apply the same rules to the legacy mutable model and the Maven 4 immutable model.
• Resolve phases and aliases from the current lifecycle registry. Do not cache the map, since extensions can register lifecycles later. Unknown phases are retained only on an exact phase match.

Tests

• Ten unit tests cover both model implementations, including cross-lifecycle exclusion, same-lifecycle retention, unknown phases, aliases, and phases registered after injector construction.
• Two independent Core IT cases check management-only and explicitly declared plugins.
• mvn -Prun-its -Dits.test=MavenITmng5359PluginManagementExecutionTest verify: passed.
• Full mvn -Prun-its verify: 1,075 Core IT tests, zero assertion failures, two errors, and 43 skipped. The two DI fixtures (MavenITgh11055DIServiceInjectionTest and MavenITmng8525MavenDIPlugin) fail to load PathTranslator; the same errors reproduce on the earlier PR revision before this follow-up. The new Core IT cases pass.

Scope

This changes lifecycle-binding injection only and adds no public API. Explicit plugin declarations retain normal plugin-management behavior.

Following this checklist to help us incorporate the contribution quickly and easily:

• This pull request addresses one issue without unrelated changes.

• The description explains what the pull request does, how, and why.

• Each commit has a meaningful subject line and body.

• Unit tests cover the behavioral changes.

• mvn verify passes as part of the targeted Core IT reactor run.

• The complete Core IT suite passed. See the two fixture errors above.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, I have filed an Apache Individual Contributor License Agreement.

[gnodet]

Please provide an IT that reproduces the problem, it's way easier to understand the exact problem.

[goutamadwant]

Thanks @gnodet. The PR includes MavenITmng5359PluginManagementExecutionTest, which reproduces the pluginManagement-only case and includes an explicit plugin declaration as a positive control.

While validating the fix, I found that filtering all managed executions regressed MNG-4344. The follow-up now filters only executions bound to a different lifecycle while retaining same-lifecycle and phase-less executions.

Validated with the focused lifecycle injector tests and both MavenITmng5359PluginManagementExecutionTest and MavenITmng4344ManagedPluginExecutionOrderTest. Let me know. thanks!

[gnodet]

Thanks for tackling this long-standing issue. The approach of filtering cross-lifecycle managed executions while preserving same-lifecycle ones is sound and addresses MNG-5359 without regressing MNG-4344. Tests are solid with good positive/negative controls.

A few observations below.

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

[gnodet]

💡 Performance: getPhaseToLifecycleMap() is called on every injectLifecycleBindings invocation, creating a new HashMap and a new LifecycleBindingsMerger each time. The previous code cached a single LifecycleBindingsMerger as a field.

Since the phase-to-lifecycle map doesn't change after startup, this could be computed once in the constructor:

Suggested change

	new LifecycleBindingsMerger(getPhaseToLifecycleMap()).merge(model, lifecycleModel);
	new LifecycleBindingsMerger(phaseToLifecycleMap).merge(model, lifecycleModel);

…with phaseToLifecycleMap as a final field initialized in the constructor. Not critical — model building isn't in a tight loop — but it's a free optimization.

[goutamadwant]

I kept the lookup dynamic because extensions can register lifecycle phases after injector construction. Added a comment and tests in both model implementations that register another phase between injections.

[gnodet]

📝 Edge case to consider: When managedPhase is a custom/unknown phase not in phaseToLifecycleMap, managedLifecycle is null and the logic falls back to exact phase matching only. This means a managed execution bound to a custom phase (e.g. from a lifecycle extension) will be filtered out unless a lifecycle execution is bound to the exact same phase.

Is this the intended behavior? It seems reasonable (err on the side of not activating unknown phases), but worth documenting as a conscious design decision, especially since custom lifecycle extensions exist in the wild.

[goutamadwant]

Yes, exact-phase matching is intentional for unregistered phases. Added a comment and tests for matching and nonmatching unknown phases, plus different phases belonging to the same registered lifecycle.

[gnodet]

📝 Consistency with legacy model: The legacy model's getPhaseToLifecycleMap() delegates to DefaultLifecycles.getPhaseToLifecycleMap(), while the Maven 4 model implementation computes the map directly from LifecycleRegistry including aliases. This means the two implementations might produce different maps if alias handling differs.

This is likely fine (the legacy model doesn't have Maven 4 lifecycle aliases), but a comment explaining the difference would help future maintainers.

[goutamadwant]

The legacy path already includes aliases through DefaultLifecycles' LifecycleRegistry adapter. Added a comment and an alias regression test for each model implementation.

[gnodet]

💡 Nit: The test verifies the result has 3 executions and checks the expected set, but doesn't verify that the filtered execution (managed-initialize) is absent. While the Set.of(...) assertion implicitly covers this (3 elements, none is managed-initialize), an explicit assertFalse would make the intent clearer:

assertFalse(result.getExecutions().stream()
        .anyMatch(e -> "managed-initialize".equals(e.getId())),
        "Cross-lifecycle managed execution should be filtered out");

[goutamadwant]

Added the explicit assertion that managed-initialize is absent in both model tests.

[gnodet]

💡 Suggestion: Both test phases could use separate test methods (e.g., testManagedExecutionNotActivatedWithoutDeclaration and testManagedExecutionActivatedWithExplicitDeclaration) for clearer test isolation and failure reporting. If one phase fails, you immediately know which scenario broke.

Also, the test uses package as the target phase, but the managed execution is also bound to package — so the test verifies that a managed clean plugin execution bound to package (a default lifecycle phase) is NOT activated when the clean plugin is introduced only via lifecycle bindings (clean lifecycle). This is a good cross-lifecycle test. A brief comment explaining this would help readability.

[goutamadwant]

Split the scenarios into separate test methods with independent project directories, and added the cross-lifecycle explanation. Both Core IT cases pass.

[gnodet]

Re-review after the follow-up commits. All five findings from the previous review have been addressed — confirming each below.

Previous findings status:

1. ✅ getPhaseToLifecycleMap() performance note — comment added explaining dynamic lookup is intentional (extensions can register lifecycle phases later). Tests for dynamic registration added in both model implementations.
2. ✅ Unknown-phase fallback behavior — documented with inline comment (An unregistered phase has no known lifecycle; retain it only for an exact phase match). New tests in both models cover matching and non-matching unknown phases, and cross-lifecycle retention for registered lifecycles.
3. ✅ Legacy/impl alias consistency — comment added in the impl getPhaseToLifecycleMap() explaining it mirrors DefaultLifecycles alias handling. Alias regression test added for both models.
4. ✅ assertFalse(managed-initialize absent) — added to both DefaultLifecycleBindingsInjectorTest implementations with the message "Cross-lifecycle managed execution should be filtered out".
5. ✅ IT test split — MavenITmng5359PluginManagementExecutionTest now has two separate @Test methods (testManagedExecutionNotActivatedWithoutDeclaration and testManagedExecutionActivatedWithExplicitDeclaration) with independent prepareProject calls. Cross-lifecycle explanation added inline.

New observations on the follow-up commits:

One nit on the IT test, see inline. The core logic and the new tests are correct — the dynamic phase map, alias handling, null-phase pass-through, and the cross-lifecycle filtering all behave as intended. The code is ready from a correctness standpoint.

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

[gnodet]

🔍 Nit: extractResources("mng-5359") returns the same path (tmpdir/mng-5359) for both test methods. Both tests call Files.createDirectories(project) on distinct subdirectories (management-only/ and explicit-plugin/) and copy the same pom.xml there, so there is no inter-test collision. However, if both test methods run in parallel (JUnit 5 parallel execution) they race on creating subdirectories inside the shared parent — Files.createDirectories is idempotent so there is no crash, but Files.copy(... REPLACE_EXISTING) on the same destination concurrently is not atomic on all OS/filesystems.

For robustness, consider using a unique working directory per test invocation, which is the pattern used by MavenITmng8750NewScopesTest (testDir.resolve("compile-only-test") after extracting a pre-existing subdirectory). Alternatively, annotate the class with @TestMethodOrder and @Execution(ExecutionMode.SAME_THREAD) if parallel execution is a concern in this test suite.

Not blocking — this is a test-isolation nit, not a correctness bug in the production code.

[goutamadwant]

I checked this path: the tests copy to different destinations, management-only/pom.xml and explicit-plugin/pom.xml. extractResources() only resolves the shared parent path; it does not rewrite the fixture. There is no same-destination copy between these tests, so I have left the isolation unchanged.