Structured BuilderProblem pipeline for DiagnosticCollector

[gnodet]

Summary

Follow-up to the build report / warning mode chain. This PR implements the structured BuilderProblem pipeline for the 4 pathways identified in the foundation work, plus a full migration of the PluginValidationManager interface.

Commit 1: Pipe structured BuilderProblems into DiagnosticCollector (Pathways 1–4)

Pathway 2 — Plugin parameter validation (AbstractMavenPluginParametersValidator): 3 validators now create BuilderProblem with structured key, severity, and suggestion, and pipe through PluginValidationManager → DiagnosticCollector.

Pathway 3 — Plugin dependency validation (AbstractMavenPluginDependenciesValidator): 4 validators now create BuilderProblem with structured key and pipe through PluginValidationManager → DiagnosticCollector.

Pathway 4 — Plugin manager Contextualizable check (DefaultMavenPluginManager): creates BuilderProblem with key plugin-validation:contextualizable and pipes through PluginValidationManager → DiagnosticCollector.

Commit 2: Migrate PluginValidationManager to native BuilderProblem API

• Changed PluginValidationManager interface: 3 abstract methods now accept BuilderProblem instead of String
• Added @Deprecated(since = "4.1.0", forRemoval = true) String-based default methods as backward-compat adapters
• Updated all 9 call sites across 8 files to create BuilderProblem natively

PR chain

#	PR	Feature
1	#12694	Logging foundation
2	#12695	Build report
3	#12697	Console modes
4	#12698	Warning mode + diagnostics
5	#12699	mvnlog viewer
6	This PR	Structured problems pipeline
7	#12714	TRACE level migration

Test plan

• mvn test -pl impl/maven-core — tests pass
• CI

🤖 Generated with Claude Code

[gnodet]

Well-structured PR that cleanly pipes structured BuilderProblems into the DiagnosticCollector. Two observations after verification (a finding about source-incompatible API changes was a false positive — old String-based methods are preserved as deprecated defaults).

Also noted:

• The mvnlog tool addition is complete with comprehensive unit and integration tests.
• The EXCLUDED_LOGGERS set correctly prevents double-counting for classes that now pipe BuilderProblems directly.
• The shell script changes correctly handle --log routing.

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]

The diagnostic key is generated as "model:" + problem.getMessage().hashCode(). Using String.hashCode() for deduplication keys is fragile — hash collisions would cause unrelated problems to be silently deduplicated. The same pattern appears in DefaultProjectsSelector and the deprecated adapters in PluginValidationManager.

Elsewhere in this PR, human-readable keys are used (e.g. "plugin-validation:contextualizable", "plugin-validation:maven2-plugin"). Consider using a more collision-resistant approach here too — e.g., incorporating the problem source and a truncated/normalized message.

[gnodet]

This toBuilderProblem(ModelProblem) method is an exact duplicate of the one in DefaultMaven.java. If the conversion logic changes, both copies must be updated in lockstep. Consider extracting to a shared utility method.

[gnodet]

Well-structured PR that correctly implements the structured BuilderProblem pipeline. The API migration preserves backward compatibility via deprecated default methods. A few items to address:

Confirmed findings (verified independently):

1. [Medium — Windows parity gap] mvn.cmd:278 — Windows batch script sets MAVEN_MAIN_CLASS for --log but does not strip the routing flag from %* before passing it to the Java process. The Unix mvn script (lines 316-327) explicitly strips --debug, --yjp, --enc, --shell, --up, and --log to prevent Commons CLI prefix-matching collision (e.g. --log matching --log-file). On Windows, running mvn --log validate will pass --log through to the Java process, causing the documented collision.

2. [Low — Code duplication] DefaultMaven.java:673 — The static method toBuilderProblem(ModelProblem) is duplicated character-for-character in DefaultMaven.java and DefaultProjectsSelector.java. Consider extracting to a shared utility class.

3. [Low — Fragile dedup key] DefaultMaven.java:691 — Using message.hashCode() as part of the dedup key ("model:" + problem.getMessage().hashCode()) is fragile since String.hashCode() is a 32-bit hash that can produce collisions. Two different model problems with the same hashCode would be silently deduplicated. Using the full message or a stronger hash would be more robust.

4. [Low — Dead code] BuildReportRenderer.java:45 — MAX_PADDED_BUILD_TIME_DURATION_LENGTH is declared but never referenced. Leftover copy from ExecutionEventLogger.

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

Claude Code on behalf of Guillaume Nodet

[gnodet]

[Medium — Windows parity gap] The Unix mvn script strips routing flags (--debug, --yjp, --enc, --shell, --up, --log) from $@ before exec (lines 316-327), but the Windows .cmd passes %* unmodified. Running mvn --log validate on Windows will pass --log through to Commons CLI, where it can collide with --log-file via prefix matching.

Windows batch's %* cannot be modified by shift, so equivalent stripping logic would need a for loop to rebuild the argument list.

[gnodet]

[Low — Fragile dedup key] String.hashCode() is a 32-bit hash that can produce collisions (e.g., "Aa" and "BB" both hash to 2112). If two different model problems collide, one gets silently deduplicated. Consider using the full message string or a stronger hash for the key.

[gnodet]

Well-structured PR that cleanly implements the structured BuilderProblem pipeline and migrates PluginValidationManager. Backward compatibility is preserved via deprecated default methods. A few design items to consider:

Windows script parity gap (mvn.cmd): The Unix script strips routing flags (--debug, --yjp, --enc, --shell, --up, --log) before passing to the JVM, but the Windows batch script does not. While the stripping was not present before for any flag on Windows, this PR widens the gap by adding a new routing flag (--log) only on Unix.

PluginValidationManager interface break: The 3 abstract methods changed from String to BuilderProblem. External implementors (not extending DefaultPluginValidationManager) will get compilation errors. This is intentional for 4.1.0 and correctly excluded from japicmp — worth noting in the PR description for downstream consumers.

Code duplication: toBuilderProblem(ModelProblem) is duplicated character-for-character in DefaultMaven.java and DefaultProjectsSelector.java. Consider extracting to a shared utility.

hashCode-based dedup keys: The pattern "model:" + problem.getMessage().hashCode() appears in 5 places across 3 files. String.hashCode() is 32-bit with known collision rates — different messages could be silently deduplicated. Low practical risk given typical Maven build volumes, but worth considering a stronger key (full message or source+line+severity).

Overall the architecture is sound — structured BuilderProblems flow cleanly through the pipeline, and the EXCLUDED_LOGGERS set correctly prevents double-counting.

---

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

Claude Code on behalf of Guillaume Nodet

[gnodet]

Well-structured PR that introduces two major features: a mvnlog build log viewer and a structured BuilderProblem pipeline with PluginValidationManager API migration. The overall architecture is clean — the dual-path approach (structured problems piped directly + EXCLUDED_LOGGERS to prevent double-counting) is sound, and the BuilderProblem.builder() API is a nice improvement over the previous 9-parameter constructor.

Three items worth addressing before merge to master:

1. Duplicated toBuilderProblem(ModelProblem) method

DefaultMaven.java and DefaultProjectsSelector.java both contain an identical private static toBuilderProblem(ModelProblem) method with the same severity mapping, field assignment, and key generation. Both are in the impl/maven-core module, so extracting to a shared utility (e.g. a static method on a ModelProblemUtils or BuilderProblemUtils class) would prevent divergence over time.

2. Javadoc / implementation mismatch in LogOptions

The Javadoc for module() and mojo() in LogOptions.java references "glob pattern" matching, but BuildReportFilter only performs case-insensitive substring contains() matching — no glob support. Either the Javadoc should remove the "glob pattern" wording, or glob matching should be implemented.

3. Inconsistent EXCLUDED_LOGGERS constant style

In BuildReportCollector.java, three of the four EXCLUDED_LOGGERS entries use hardcoded class name strings while the first uses .class.getName(). All four classes are in the same module (impl/maven-core), so using .class.getName() consistently would provide compile-time safety against silent breakage on class rename.

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

Claude Code on behalf of Guillaume Nodet