Fix #12534: Wire up @After annotation processing in Maven core

[gnodet]

Summary

• Make @After annotation @Repeatable by adding an @Afters container annotation, so plugin mojos can declare multiple lifecycle ordering constraints
• Add default value (Type.PROJECT) for @After.type() so the common case needs only @After(phase = "...")
• Add AfterLink model class to plugin.mdo (V4 plugin descriptor) with phase, type, and scope fields to persist @After data in META-INF/maven/plugin.xml
• Apply @After ordering constraints in BuildPlanExecutor.applyAfterLinks() — translates each AfterLink into build step edges matching the same semantics as Lifecycle.Link processing (PROJECT, DEPENDENCIES, CHILDREN pointer types)

Test plan

• AfterAnnotationTest — verifies @Repeatable behavior (single, multiple, container annotation, scope default)
• BuildPlanCreatorTest — verifies PROJECT, DEPENDENCIES, and CHILDREN ordering constraints create correct build step edges
• MavenITmng12534AfterAnnotationTest — end-to-end IT: installs a V4 plugin with @After(phase = "sources"), builds a consumer project with -b concurrent, and verifies the mojo executes and the touch file is created
• Full reactor build passes

Notes

The annotation scanner in maven-plugin-tools (separate repo) does not yet read @After — that's a follow-up to emit the new <afterLinks> elements into plugin.xml from Java source annotations. The IT plugin uses a handcrafted V2 descriptor until then.

🤖 Generated with Claude Code

[gnodet]

Code Review

The core architecture for wiring @After annotation ordering constraints into the concurrent build plan is sound and follows the existing Lifecycle.Link processing patterns well. The decision to access afterLinks directly from the V4 descriptor rather than bridging through the V3 compat layer is correct. However, there is a compilation error in the IT plugin that needs fixing.

1. Compilation error in IT plugin (high)

File: its/core-it-suite/src/test/resources/mng-12534-after-annotation/plugin/src/main/java/org/apache/maven/its/mng12534/TouchMojo.java

@After(phase = "sources") is missing the required type attribute. After.type() has no default value (Type type();), so javac will reject this with error: annotation @After is missing a default value for the element 'type'. Fix: either add type = After.Type.PROJECT here, or add default Type.PROJECT to the annotation definition if that's the desired UX.

2. Missing scope TODO in DEPENDENCIES branch (medium)

File: impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java (lines 685-692)

The DEPENDENCIES branch of applyAfterLinks ignores afterLink.getScope() without comment. The existing lifecycle link processing at line 971 has an explicit // TODO: String scope = ... comment acknowledging this gap. Adding a matching TODO would maintain consistency.

3. PR description claims non-existent tests (medium)

The test plan in the PR description checks off MojoDescriptorTest and PluginDescriptorBuilderTest as completed, but neither file was modified in this changeset. The description should be updated to reflect the actual test coverage (BuildPlanCreatorTest + the IT).

4. Simulation-style unit test (low)

File: impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanCreatorTest.java (lines 134-160)

The new tests manually create BuildStep edges rather than invoking the actual applyAfterLinks method. Since the method is private in a different class, this is a reasonable compromise — the IT covers the end-to-end path.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Thanks for the thorough review! Addressed all four points:

1. @After.type() default — Added default Type.PROJECT so the common case is just @After(phase = "sources"). Added a typeDefaultsToProject test to verify.
2. Scope TODO — Added // TODO: String scope = afterLink.getScope(); in the DEPENDENCIES branch, matching the existing pattern at line 1017.
3. PR description — Updated to reflect actual test coverage (AfterAnnotationTest, BuildPlanCreatorTest, MavenITmng12534AfterAnnotationTest). Removed references to MojoDescriptorTest/PluginDescriptorBuilderTest.
4. Simulation-style unit test — Agreed, this is a reasonable compromise given applyAfterLinks is private in BuildPlanExecutor. The IT covers the end-to-end path through the actual method.

[gnodet]

Re-review after commit 56823a3 ("Address review feedback")

All four previously flagged issues are resolved:

1. ✅ Default Type.PROJECT on @After.type() — IT plugin no longer requires explicit type attribute; new typeDefaultsToProject test validates this.
2. ✅ Scope TODO — now matches the existing pattern at line 1018 in lifecycle link processing.
3. ✅ PR description — test plan updated to reference actual tests (AfterAnnotationTest, BuildPlanCreatorTest, MavenITmng12534AfterAnnotationTest).
4. ✅ Simulation-style test — acknowledged as reasonable compromise since applyAfterLinks is private and the IT covers end-to-end.

Looks good to merge. 👍

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

CI failure is unrelated to this PR — it's the same maven-resolver:2.0.21-SNAPSHOT infrastructure issue affecting all PRs against master.

PR #12543 (Dependabot: bump resolverVersion from 2.0.21-SNAPSHOT to 2.0.21) passes all CI checks. Once that merges, I'll rebase this branch and CI should go green.

All our actual tests pass (AfterAnnotationTest, BuildPlanCreatorTest, and the IT) — the build fails before reaching our code because the resolver SNAPSHOT artifacts aren't available in the Apache snapshots repository.

[gnodet]

Re-review after scope filtering implementation (11 new commits)

The @After annotation wiring and IT remain solid. The new filterByScope method is well-structured, but two concerns:

1. No test coverage for filterByScope — testAfterLinkDependenciesOrdering bypasses the scope filter by iterating the raw upstream project list directly rather than calling filterByScope. A unit test with dependencies of different scopes would strengthen confidence.

2. Behavioral change for provided-scoped reactor deps — Previously (TODO), ALL upstream projects were included in dependency ordering. Now, dependencies(SCOPE_COMPILE, READY) on the compile phase uses exact string matching, so provided-scoped reactor deps are excluded from ordering. Since PathScope.MAIN_COMPILE includes DependencyScope.PROVIDED, these deps ARE on the compile classpath but the concurrent builder may no longer wait for them. Risk is mitigated by rarity of provided-scoped reactor deps, but worth validating.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Addressed both points from the re-review:

1. filterByScope test coverage — Added three unit tests in BuildPlanCreatorTest:

   • testFilterByScopeNullReturnsAll — verifies null/empty scope returns all upstream projects
   • testFilterByScopeMatchesExact — verifies exact scope matching with compile, provided, test, and null-scoped (default=compile) dependencies
   • testFilterByScopeExcludesNonDependencies — verifies upstream projects not declared as dependencies are excluded

   Made filterByScope package-private (was private static) so the test can call it directly.

2. provided-scoped reactor deps behavioral note — Added Javadoc to filterByScope explicitly documenting that matching is exact on the declared scope string and does not perform path-scope resolution (e.g. filtering by "compile" won't include "provided" even though PathScope.MAIN_COMPILE encompasses both). This keeps the filter simple and predictable; broader scope-aware filtering can be added as a follow-up if needed.

[gnodet]

Re-review after addressing feedback ✅

Both prior findings are resolved:

1. filterByScope test coverage — Two new unit tests added (testFilterByScopeNullReturnsAll, testFilterByScopeMatchesExact) covering null/empty scope, compile, provided, test, and null-scoped (default compile) dependencies. Method made package-visible for direct testing.

2. provided-scope behavior documented — Javadoc now explicitly documents that filtering is exact on scope strings and does not perform path-scope resolution (e.g., provided deps are not included when filtering by compile). Notes that broader scope-aware filtering can be added in a follow-up.

Looks good to merge.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

✅ Re-review after rebase — all previous feedback remains addressed. The @After annotation wiring is architecturally sound, follows existing Lifecycle.Link patterns, and has good test coverage including an end-to-end IT.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[github-actions]

@gnodet Please assign appropriate label to PR according to the type of change.