Console modes: --console=plain/rich/verbose/machine

[gnodet]

Summary

Part 3 of the logging feature chain. Depends on #12695 (build report).

Adds the --console CLI flag with four output modes:

• plain — compact one-line-per-module output, ideal for CI (auto-selected in CI environments)
• rich — JLine status bar with live reactor progress (auto-selected on interactive TTYs)
• verbose — full mojo-level output, current Maven 4.0 default behavior
• machine — JSON lines: one typed JSON object per lifecycle event, designed for piping to external tools

Auto-detection (--console=auto, the default): CI → plain, interactive TTY → rich, otherwise → verbose.

Files changed (16 files, ~4000 insertions)

Area	Files
API	Options.java (+console())
Event loggers	PlainExecutionEventLogger, RichBuildEventListener, RichExecutionEventLogger, MachineBuildEventListener, MachineExecutionEventLogger
CLI wiring	CommonsCliOptions, LayeredOptions, LookupInvoker (preliminary interactive detection), MavenInvoker (console mode switch + transfer listener)
Existing logger	ExecutionEventLogger (version info on failure)
Tests	5 test classes with full coverage

PR chain

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

Test plan

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

🤖 Generated with Claude Code

[gnodet]

Well-structured PR adding four console modes with comprehensive test coverage (1317 lines of tests). Two issues found, one concurrency bug.

Also noted:

• The Options.console() API addition follows the established pattern (Optional<String>, same as color()). The @Experimental annotation means downstream breakage is acceptable.
• JSON escaping in MachineBuildEventListener correctly handles all RFC 8259 required escapes, with test coverage for edge cases.
• Test coverage is strong: 5 new test classes covering all four console modes.

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: completedProjects++ is not atomic. The field is volatile int (line 103) but ++ is a non-atomic read-increment-write compound operation. In parallel builds (-T N), concurrent calls to projectFinished() from worker threads can lose increments, causing the progress counter to under-report.

The same class already uses AtomicInteger for warningCount and errorCount (lines 133-136).

Suggested change

	completedProjects++;
	completedProjects.incrementAndGet();

(with completedProjects changed to AtomicInteger at line 103)

[gnodet]

Unrecognized --console values (including typos like --console=plian) silently fall through to auto-detection. This is inconsistent with the existing --color option in LookupInvoker.java (lines 274-276) which throws IllegalArgumentException for invalid values. Adding validation for consistency would help users catch configuration mistakes.

[gnodet]

Well-structured PR adding four console modes with comprehensive test coverage. Two concurrency issues found in RichBuildEventListener that affect parallel builds (-T flag):

Confirmed findings (verified independently):

1. [High — Concurrency bug] RichBuildEventListener.java:308 — completedProjects++ on a volatile int is not atomic. In parallel builds, multiple threads call projectFinished() concurrently, causing lost increments. The same class already uses AtomicInteger for warningCount (line 133) and errorCount (line 136), and the companion MachineExecutionEventLogger protects its equivalent counter with synchronized. completedProjects should use AtomicInteger as well.

2. [Medium — Logic issue in parallel builds] RichBuildEventListener.java:635 — projectOrder.indexOf(pid) < completedProjects assumes modules complete in reactor order, which does not hold in parallel builds. Example: reactor order [A(0), B(1), C(2)] with B and C independent. If C completes before B starts, completedProjects=2, and B (index 1 < 2) would incorrectly show a green checkmark before it runs. Consider tracking completed project IDs in a Set<String> instead of relying on positional count.

3. [Low — Style] MachineExecutionEventLogger.java:96 — session.getRequest().getGoals().stream().collect(Collectors.joining(" ")) can be simplified to String.join(" ", session.getRequest().getGoals()), consistent with line 104 of the same file which already uses String.join for profiles.

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]

[High — Concurrency bug] completedProjects++ on a volatile int is not atomic. In parallel builds (-T flag), multiple threads call projectFinished() concurrently, causing lost increments via the classic read-modify-write race.

This class already uses AtomicInteger for warningCount and errorCount. Suggest:

Suggested change

	completedProjects++;
	completedProjects.incrementAndGet();

(with the field declaration changed to private final AtomicInteger completedProjects = new AtomicInteger();)

[gnodet]

[Medium — Logic issue in parallel builds] This assumes modules complete in reactor order, which is not guaranteed with -T. A module not yet started can have a lower reactor index than the count of completed modules, causing an incorrect green checkmark.

Consider tracking completed project IDs in a Set<String> and checking completedProjects.contains(pid) instead.

[gnodet]

[Low — Style] This can be simplified to String.join(" ", session.getRequest().getGoals()), consistent with line 104 which already uses String.join for profiles.

Suggested change

	String goals = session.getRequest().getGoals().stream().collect(Collectors.joining(" "));
	String goals = String.join(" ", session.getRequest().getGoals());

[gnodet]

Well-structured PR introducing four console output modes with good test coverage (5 test classes). The JLine Display approach for rich mode mirrors mvnd's proven pattern. A few items to consider:

Medium severity:

1. completedProjects++ race condition (RichBuildEventListener.java, projectFinished method): volatile int increment is not atomic. In parallel builds (-T), concurrent projectFinished calls can race and lose increments, causing the progress counter to permanently undercount. Inconsistent with warningCount/errorCount in the same class which correctly use AtomicInteger. Fix: change completedProjects to AtomicInteger.

2. Invalid --console=<value> silently accepted (MavenInvoker.java, determineConsoleMode): Unrecognized values like --console=foo fall through to auto-detection with no warning. A user who misspells a mode name (e.g., --console=machin instead of --console=machine) gets silently different behavior.

Low severity:

3. Unused constructor parameter (RichBuildEventListener.java): The output parameter (Consumer<String>) is accepted but never stored or referenced. The Javadoc says "kept for API compat" but this is a brand-new class with no existing callers.

4. Hardcoded report path printed unconditionally: Both PlainExecutionEventLogger.logStats() and RichExecutionEventLogger.logStats() print "Full report: target/build-reports/build-report-latest.json" on every build. This file may not exist if the build-report feature (PR #12695) is not active or the output directory is customized. Consider making it conditional on the file actually existing.

Overall the architecture (splitting BuildEventListener and ExecutionListener per mode, with shared JsonLine builder for machine output) is clean. The preliminaryInteractiveDetection approach is reasonable.

---

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-designed console mode system with clean separation of BuildEventListener and ExecutionListener per mode. The JsonLine builder handles RFC 8259 escapes correctly, and the test suite is comprehensive (~1300 lines across 5 test classes covering all four modes).

Five items to address:

1. Non-atomic completedProjects++ race condition (high)

In RichBuildEventListener.java line ~308, completedProjects++ on a volatile int is a non-atomic read-modify-write. In parallel builds (-T N), concurrent projectFinished() calls can lose increments. The same class already uses AtomicInteger for warningCount and errorCount, and both PlainExecutionEventLogger and MachineExecutionEventLogger protect their counters with synchronized(this). Replace with AtomicInteger and incrementAndGet().

2. Reactor-order assumption for checkmark indicators (medium)

RichBuildEventListener.java line ~635: projectOrder.indexOf(pid) < completedProjects assumes modules complete in reactor order, which doesn't hold in parallel builds. If projects A and C finish (count=2) but B hasn't started, B (index 1) would show a green checkmark because 1 < 2. Track completed project IDs in a Set<String> (e.g. ConcurrentHashMap.newKeySet()) instead.

3. Silent acceptance of invalid --console values (medium)

In MavenInvoker.determineConsoleMode() line ~415, unrecognized values like --console=plian silently fall through to auto-detection. This is inconsistent with the existing --color option which throws IllegalArgumentException for unrecognized values. A user who misspells the mode gets silently different behavior with no feedback.

4. Unused constructor parameter on new class (low)

RichBuildEventListener.java line ~146: The output constructor parameter (Consumer<String>) is accepted but never stored. The Javadoc says "kept for API compat" but this is a brand-new @since 4.1.0 class with no existing callers — there is no backward compatibility concern.

5. Inconsistent String.join usage (low)

MachineExecutionEventLogger.java line ~96 uses stream().collect(Collectors.joining(" ")) while line ~106 uses String.join(",", ...) for the same kind of operation. Use String.join consistently.

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

[gnodet]

One format worth considering for the machine-readable console mode: logfmt.

level=INFO  build=a1b2c3 module=my-app execution=maven-compiler-plugin:compile:default phase=compile ts=1234567890123 msg="Compiling 42 source files"
level=ERROR build=a1b2c3 module=my-app execution=maven-compiler-plugin:compile:default phase=compile ts=1234567890456 msg="Compilation failure"
level=INFO  build=a1b2c3 module=my-app execution=maven-compiler-plugin:compile:default result=FAILURE duration_ms=1823 msg="Mojo finished"

It sits nicely between plain text (human-readable) and JSON lines (fully structured): parseable by Loki, Vector, Grafana out of the box, but still legible with a plain cat.

The interesting property: with build + module + execution as grouping keys, the full hierarchy (modules → executions → log events + timing + results) can be reconstructed from the flat stream by any consumer. Which means a single logfmt stream could serve both the live observability use case and replace the JSON build report from #12695 as the source of truth — tools like mvnlog could consume it directly.

Could be offered as a --console=logfmt option, or considered as the canonical machine format if we want to converge on a single representation.