[gnodet]

⚠️ This PR is stacked on #12695. Review only this commit: 14521d5

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 (17 files)

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, linear reactor summary)
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 verify -pl api/maven-api-cli,impl/maven-cli -am — all tests pass
• CI validation

[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.

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

[gnodet-bot]

Console modes review — overall this is solid. Three specific issues worth addressing before merge.

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

[gnodet-bot]

Console modes + build report review — the three findings from the previous review (PR #12697 / old SHA 7b65dd40) are all addressed in this resubmission. One new concurrency issue found, and one pre-existing concern persists.

Addressed from prior review:

• ✅ MachineExecutionEventLogger mojo key now includes groupId — module collision in multi-module builds fixed
• ✅ BUILD FAILURE header correctly at logger.error() in logResult()
• ✅ currentVisitedProjectCount is now AtomicInteger in MachineExecutionEventLogger

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

[gnodet-bot]

Console modes re-review (SHA 14521d59) — prior findings #1 (mojoKey collision) and #2 (BUILD FAILURE level in ExecutionEventLogger) are fixed. Three new issues in the newly introduced classes.

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

[gnodet-bot]

Console modes re-review (SHA 67d50688) — all four findings from the prior review are addressed. One nit remains unresolved.

Addressed from prior review:

• ✅ RichBuildEventListener.completedProjects race — fixed: volatile int → final AtomicInteger with .incrementAndGet() / .get() throughout
• ✅ Per-module FAILURE lines in ExecutionEventLogger logged at wrong level — fixed: if (buildSummary instanceof BuildFailure) logger.error(...) branch added
• ✅ PlainExecutionEventLogger BUILD FAILURE logged at INFO — fixed: session.getResult().hasExceptions() check added, tests updated
• ✅ PlainExecutionEventLogger.volatile int currentVisitedProjectCount — fixed: volatile dropped (lock provides the memory ordering)
• ✅ Unknown --console= value silent fallthrough — fixed: warning now logged via context.logger.warn()

One missed fix:

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

[gnodet-bot]

Re-review after 3487903335 ("fix review: RichBuildEventListener race, logger.error on failures, unknown --console warn, drop volatile")

All four findings from the previous CHANGES_REQUESTED reviews are addressed:

Finding	Status
completedProjects++ race in RichBuildEventListener	✅ Fixed — volatile int → AtomicInteger, incrementAndGet() used throughout
Per-module FAILURE lines logged at INFO in ExecutionEventLogger.logReactorSummary	✅ Fixed — BuildFailure instanceof check now routes to logger.error()
BUILD FAILURE logged at INFO in PlainExecutionEventLogger.logResult	✅ Fixed — session.getResult().hasExceptions() check now routes to logger.error()
Silent fallthrough for unknown --console= values in MavenInvoker.determineConsoleMode	✅ Fixed — unknown values now emit a warn(); "auto" falls through silently as before
Misleading volatile on currentVisitedProjectCount in PlainExecutionEventLogger	✅ Fixed — volatile dropped

One nit from the prior inline comment (already posted on this SHA) remains open:
RichExecutionEventLogger.java:85 — private volatile int currentVisitedProjectCount still has the misleading volatile modifier. The field is only ever mutated inside synchronized (this) blocks, and the lock already provides the required happens-before. The volatile is redundant and signals incorrect intent to future readers — same fix as applied to PlainExecutionEventLogger.

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

[gnodet-bot]

Console modes re-review (SHA a39b4c2e) — the remaining nit from the previous review is addressed.

Addressed:

• ✅ volatile removed from currentVisitedProjectCount in RichExecutionEventLogger — correct. The field is only written inside synchronized (this) (line 258: number = ++currentVisitedProjectCount) and assigned single-threadedly at session start (line 147). The JMM happens-before guarantee from the monitor makes volatile redundant and misleading (it implied an unsynchronized read path that does not exist).

All prior findings from all previous reviews are addressed. The PR is ready.

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

[gnodet-bot]

Console modes re-review (SHA 08559154) — re-reviewing the delta since the prior APPROVE (a39b4c2e).

Delta since last review:

• ✅ Trailing newline added to 7 files (MachineBuildEventListener.java, LayeredOptions.java, LookupInvoker.java, 4 test files) — trivial formatting fix, no logic change.
• ✅ WorkspaceParentProjectBuilderTest.java (new, 141 lines) — solid test for workspace parent basedir handling. Correctly overrides getWorkspaceReader() (the hook exists on master since #13212). Two scenarios well-covered: parent from project-local repository gets no basedir, parent that is a session project keeps its basedir. Test structure is clean — @TempDir, proper POM helpers, correct use of assertNull/assertEquals. No issues.
• ✅ domtripVersion 1.6.0 → 1.7.0, jlineVersion 4.4.3 → 4.4.5 — both are minor bug-fix releases with no breaking API changes. JLine 4.4.5 includes fixes for FFM SIGQUIT handler, ColorPalette OSC 4 fallback, ReDoS-vulnerable regex, and paginated layout defects. domtrip 1.7.0 is from the maveniverse project (same maintainer). Both bumps are safe.
• ✅ .worktrees/** added to RAT exclusion — correct, avoids false license-header hits on git worktree admin files.

All prior findings from the review chain remain addressed. APPROVED.

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

[gnodet-bot]

Console modes re-review (SHA f6bb3306) — single commit since the prior APPROVE (08559154).

Delta: Restores getWorkspaceReader() hook in AbstractCoreMavenComponentTestCase.

• ✅ getWorkspaceReader() returns null by default — DefaultSessionBuilder.setWorkspaceReader(null) is a plain field assignment, no NPE risk.
• ✅ SessionBuilder.setWorkspaceReader(WorkspaceReader) exists in resolver 2.0.23 — the call site is valid.
• ✅ WorkspaceParentProjectBuilderTest.getWorkspaceReader() override (already in-tree) is now wired into initRepoSession(), which is the whole point of this commit.
• ✅ Javadoc on the new method is correct and sufficient.

Clean fix. APPROVE.

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

[gnodet-bot]

Console modes re-review (SHA 48f67cdb) — single commit since the prior APPROVE (f6bb3306).

Delta: Restores isProjectPom() helper and its use in resolveProjectSource() in DefaultProjectBuilder.

• ✅ isProjectPom(Path pomFile) — logic is correct. pomFile == null guard handles Artifact.getPath() returning null before resolution. projects == null guard matches the existing pattern at line 371 (defensive for UT environments). filter(Objects::nonNull) is appropriate even though the list is @Nonnull. The use of normalize() is more correct than the existing line 371 equivalent.
• ✅ localProject = isWorkspaceRepo && isProjectPom(pomArtifact.getPath()) — restores the original invariant: only POMs belonging to actual session projects get a basedir. Fixes WorkspaceParentProjectBuilderTest.parentFromProjectLocalRepositoryHasNoBasedir.
• ✅ No new behavior introduced — pure restoration of code accidentally dropped in the squash commit.

Clean fix, no issues.

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

[gnodet-bot]

Console modes re-review (SHA 905812cd) — single commit since the prior APPROVE (48f67cdb).

Delta: Isolates ITs from the CI environment so CI → plain auto-detection does not suppress output that ITs rely on.

• ✅ AbstractMavenIntegrationTestCase.newVerifier() — removeCIEnvironmentVariables() called in the base method. Verified against all six CI detectors: GenericCIDetector (CI="false"), GithubCIDetector (GITHUB_ACTIONS=""), TravisCIDetector (TRAVIS=""), CircleCIDetector (CIRCLECI=""), JenkinsCIDetector (WORKSPACE=""), TeamcityCIDetector (TEAMCITY_VERSION="") — every detector is correctly neutralized.
• ✅ MavenITmng8655SettingsParserTest.coreRealmParserReadsBootstrapSettings() — adds --console=verbose to the ForkedMavenExecutor call (which bypasses newVerifier() and thus the base-class fix). Correct: verbose mode uses ExecutionEventLogger which emits the Building bootstrap-settings 0.1 line the assertion checks.
• ✅ Existing CI-aware ITs: tests that called removeCIEnvironmentVariables() individually (mng4829, mng6240, mng4461) are now redundant but harmless. No regression.

All prior findings remain resolved. Clean delta.

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

[gnodet-bot]

Console modes re-review (SHA 512629a5) — re-reviewing the delta since the prior APPROVE (905812cd).

Context: The branch was force-pushed. The new HEAD collapses several prior commits into a different squash. Comparing the PR diff vs master (the only merge-relevant delta), the sole net-new code change is the JUL-level clamping in LookupInvoker.

Items that appeared to regress vs 905812cd but are not merge regressions:

• jlineVersion 4.4.5 → 4.4.3, domtripVersion 1.7.0 → 1.6.0, .worktrees/** RAT exclude absent — these were squashed out of the PR branch, but all three are already on master via independent commits (ebb415b5, 77944f80, 18806b1a45). They will survive the merge.
• WorkspaceParentProjectBuilderTest not in PR branch — was added+deleted within the feature branch squash. master has it independently via c3b52889c3. Merge will keep master's version.

New JUL-level clamping in LookupInvoker (configureLogging + activateLogging):

configureLogging (quiet mode): MavenJulHandler.install() sets Level.ALL internally, then setLevel(Level.SEVERE) immediately overrides it. These two calls are synchronous in a single thread — zero event-processing window between them. Level.SEVERE blocks JUL events before they reach publish(), preventing the SLF4J-bootstrap reentrancy flood that Level.ALL would cause while SLF4J is still pre-activated. Correct.

activateLogging: post-activate() JUL root level is set to mirror the SLF4J effective level (quiet→SEVERE, verbose→ALL, default→INFO). The isInstalled() guard prevents double-install. The three-way mapping is correct.

The fix correctly closes the race window described in the comment: between configureLogging and activateLogging, createTerminal() runs and JLine emits JUL events. In quiet mode those events were leaking before this change. Setting Level.SEVERE at source (JUL root) before createTerminal() is the right fix — the isLevelEnabled() check in MavenJulHandler.publish() would not suffice because newly created SLF4J loggers briefly see the default INFO level during the reentrancy window.

IT coverage via MavenITmng4387QuietLoggingTest (on master, already exercises quiet-mode output expectations). The change is narrowly scoped and well-explained. APPROVE.

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

[gnodet-bot]

Console modes re-review (SHA 2c47687a) — re-reviewing the new commit since the prior APPROVE (512629a5).

One nit: the comment block is now stale after the cwd change.

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

[gnodet-bot]

🔶 Nit — stale comment: the comment at lines 447-448 now contradicts the code:

"but we should not use basedir, as it may contain extensions.xml or a project, that Maven will eagerly load"

But the code below now calls .cwd(basedir). If the intent changed (e.g. because the settings.xml path must be resolved relative to basedir, and the new code mitigates the extension-loading risk differently), the comment should be updated to explain the new rationale. If basedir is still problematic here, this change should be reconsidered.

[gnodet-bot]

Three findings in the new API files and test infrastructure.

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

[gnodet-bot]

⚠️ Missing @Nullable annotation on resumeFrom()

The Javadoc says "or null if not specified", but the method has no @Nullable annotation. Every other nullable-returning method in the Maven API carries this annotation to make the null contract explicit for callers and static analysis tools. The file imports @Nonnull but not @Nullable.

Suggested change

	String resumeFrom();
	@Nullable
	String resumeFrom();

And add the import alongside @Nonnull:

import org.apache.maven.api.annotations.Nullable;

[gnodet-bot]

⚠️ Stale comment contradicts the new code

The comment at lines 447–448 says:

// but we should not use basedir, as it may contain extensions.xml or a project, that Maven will eagerly

But this PR changes .cwd(tempBasedir) to .cwd(basedir) on the very next line. Either the comment is stale and should be removed/updated, or the .cwd(basedir) change reintroduces the problem the comment was guarding against. Please clarify the intent and update the comment accordingly.

[gnodet-bot]

💡 goals() and threads() overlap with environment().goals() and environment().threads()

BuildReport already exposes environment() which returns a BuildEnvironment — and BuildEnvironment has both goals() and threads(). Surfacing them again directly on BuildReport creates two sources of truth for the same data. If they are convenience aliases (delegates to environment()), the Javadoc should say so explicitly. If they can diverge from the environment values, that distinction also needs to be documented.