Warning mode, diagnostic collector, BuilderProblem enrichments

[gnodet]

Summary

Part 4 of the logging feature chain. Depends on #12697 (console modes).

Adds --warning-mode CLI flag and structured diagnostic infrastructure:

• --warning-mode=summary (default) — deduplicated warning summary at end of build
• --warning-mode=all — inline warnings + summary
• --warning-mode=none — suppress diagnostic summary
• --warning-mode=fail — treat warnings as build errors

BuilderProblem API enrichments

• getKey() — stable deduplication key for cross-module dedup
• getSuggestion() — actionable fix suggestion
• getDocumentationUrl() — link to relevant docs
• INFO severity level added to Severity enum
• BuilderProblem.builder() — fluent builder API with DefaultProblem record

New service API

• DiagnosticReporter — @Inject-able service for plugins to report structured problems

Internal infrastructure

• DefaultDiagnosticCollector — thread-safe, deduplicating problem store with suppression support
• DefaultDiagnosticReporter / DefaultDiagnosticSummary — wiring
• BuildReportCollector upgraded to use DefaultDiagnosticCollector, auto-collect WARN log events, print summary

Files changed (16 files, ~1600 insertions)

Area	Files
API	BuilderProblem enrichments, DiagnosticReporter, Options (+warningMode())
CLI wiring	CommonsCliOptions, LayeredOptions, MavenInvoker
Collector	BuildReportCollector (full diagnostic integration), BuildReportJsonWriter (problem enrichments)
Internal	DefaultDiagnosticCollector, DefaultDiagnosticReporter, DefaultDiagnosticSummary
Impl	DefaultBuilderProblem (key/suggestion/url fields)
Tests	4 test classes updated + 1 new (DefaultDiagnosticCollectorTest)

PR chain

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

Test plan

• mvn test -pl impl/maven-core — all tests pass
• mvn test -pl impl/maven-cli — all 692+ tests pass
• CI validation

🤖 Generated with Claude Code

[gnodet]

Well-structured diagnostic infrastructure with good test coverage. Two issues found after verification (a third finding about removed try-catch was a false positive — EventSpyDispatcher.onEvent() already provides the safety net).

Also noted:

• Two independent BuilderProblem implementations exist (private DefaultProblem record in Builder and public DefaultBuilderProblem in impl). Future field additions need to update both.
• The warningMode propagation via user properties (maven.build.warningMode) is pragmatic for an EventSpy but worth noting as a side-channel.

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]

Race condition: noKeyCounter.increment() and noKeyCounter.longValue() are not atomic together. Two concurrent threads reporting null-key problems can both get the same counter value, producing the same synthetic key "__no_key__N", and the second problem is silently dropped by putIfAbsent.

Fix: use AtomicLong instead of LongAdder with incrementAndGet() for an atomic unique counter:

Suggested change

	if (key == null) {
	key = "__no_key__" + noKeyCounter.incrementAndGet();

(also change noKeyCounter field type from LongAdder to AtomicLong)

[gnodet]

No validation of --warning-mode values. An invalid value like --warning-mode=quiet is silently accepted and defaults to summary behavior. BaseParser.validate() already validates --color and --fail-on-severity against allowed values — --warning-mode should follow the same pattern for consistency.

[gnodet]

Well-designed diagnostic infrastructure with good test coverage for the new functionality. A few issues worth addressing, including a concurrency bug and a logic mismatch:

Confirmed findings (verified independently):

1. [Medium — Concurrency bug] DefaultDiagnosticCollector.java:113 — Race condition in synthetic key generation for null-key problems. noKeyCounter.increment() and noKeyCounter.longValue() are two separate operations on a LongAdder. Between them, another thread can call increment(), causing two distinct null-key problems to receive the same synthetic key and one will be silently dropped by putIfAbsent. Fix: replace LongAdder with AtomicLong and use incrementAndGet() in a single call.

2. [Medium — Logic mismatch] BuildReportCollector.java:251 — hasWarnings() returns true for severity WARNING or higher (FATAL, ERROR) via ordinal comparison, but the count loop only counts strict BuilderProblem.Severity.WARNING entries. If only ERROR-severity diagnostics exist, hasWarnings() returns true but warningCount stays 0, producing the misleading message "Build has 0 warning(s) and --warning-mode=fail is set".

3. [Medium — Test coverage regression] Three existing tests deleted without replacement: testStackTraceIsTruncated(), testShortStackTraceIsNotTruncated(), and testTimestampedReportFile(). The production code they covered (truncateStackTrace() method and timestamped report file write/symlink logic) is still present and used.

4. [Low] MavenInvoker.java:268 — No input validation for --warning-mode value. Invalid values like --warning-mode=typo are silently accepted and treated as "summary" behavior. Consider validating against the known set (summary, all, none, fail).

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 — Concurrency bug] noKeyCounter.increment() + noKeyCounter.longValue() is not atomic. In parallel builds, two threads can call increment() between each other's longValue(), getting the same synthetic key. One problem gets silently dropped by putIfAbsent.

Fix: replace LongAdder with AtomicLong and use incrementAndGet():

Suggested change

	noKeyCounter.increment();
	key = "__no_key__" + noKeyCounter.incrementAndGet();

(with field declaration changed to private final AtomicLong noKeyCounter = new AtomicLong();)

[gnodet]

[Medium — Logic mismatch] hasWarnings() returns true for severity WARNING or higher (FATAL, ERROR) via ordinal comparison, but this count loop uses strict == BuilderProblem.Severity.WARNING. If only ERROR-severity diagnostics exist, hasWarnings() triggers but warningCount stays 0, producing "Build has 0 warning(s)".

Consider either: (a) changing hasWarnings() to check for exactly WARNING severity, or (b) counting all severities that hasWarnings() considers (i.e., ordinal <= WARNING).