Build report: structured JSON report with per-mojo log capture

[gnodet]

Summary

Part 2 of the logging feature chain (depends on #12694 — logging foundation).

⚠️ This PR is stacked on #12694. Review only this commit: 1dae6bb

Adds a structured build report that captures per-module and per-mojo execution results, timing, log events, and failures as a JSON file at the end of every build. Also adds BuildEnvironment to the Maven API, exposed via Session.buildEnvironment() and frozen in BuildReport.environment().

What's in this PR

Layer	Files	Description
API	BuildReport, BuildStatus, ModuleReport, MojoReport, FailureReport	Immutable interfaces for the report data model
API	BuildEnvironment	New interface in o.a.m.api capturing the invocation context
API	Session.buildEnvironment()	Live access to build environment for plugins
Impl	DefaultBuildReport, DefaultModuleReport, DefaultMojoReport, DefaultFailureReport, DefaultBuildEnvironment	Record implementations
Collector	BuildReportCollector	EventSpy that tracks lifecycle events and captures log output via LogEventSink, routing to mojo/module/build-level buffers
Writer	BuildReportJsonWriter	Zero-dependency JSON serializer with stable field order
Tests	3 test classes, 17+ tests	Unit tests for collector, JSON writer, and integration

Key design decisions

• EventSpy pattern: BuildReportCollector is a @Named @Singleton that extends AbstractEventSpy, discovered automatically — no wiring changes needed
• Thread-based log routing: Uses ConcurrentHashMap<Long, String> (thread ID → mojo/project key) to associate log events with the correct scope in parallel builds
• Dual sink architecture: Uses LogEventSink (4-arg) independently from the existing LogSink (5-arg) used by ProjectBuildLogAppender — no interference with console output
• Atomic writes: Reports are written to a temp file, then atomic-moved into place with a timestamped filename and a build-report-latest.json symlink
• Defensive: onSessionEnded wraps report generation in try-catch so report failures never crash the build

BuildEnvironment

BuildEnvironment (in o.a.m.api) captures the full invocation context at SessionStarted:

• Goals, user properties (sensitive keys redacted: password, token, secret, passphrase, apikey), curated system info (OS, JVM, Maven home, available processors)
• Local repository path, active profiles (explicit -P only), selected projects (-pl), resume-from (-rf)
• Reactor failure behavior, offline, update-snapshots, no-transfer-progress, batch mode, thread count

Session.buildEnvironment() gives plugins live access to the same data. BuildReport.environment() carries a frozen snapshot in the JSON output.

Also fixes a gap in MavenExecutionRequest: noTransferProgress was consumed by MavenInvoker to pick a TransferListener but never stored on the request. Added isNoTransferProgress() / setNoTransferProgress().

What's NOT in this PR (deferred to later PRs)

• BuilderProblem enrichments (key, suggestion, documentationUrl) — Warning mode, diagnostic collector, BuilderProblem enrichments #12698
• Diagnostic collection and warning deduplication — Warning mode, diagnostic collector, BuilderProblem enrichments #12698
• --warning-mode CLI flag — Warning mode, diagnostic collector, BuilderProblem enrichments #12698
• Console modes (--console=plain/rich/machine) — Console modes: --console=plain/rich/verbose/machine #13180
• mvnlog viewer tool — mvnlog: build log viewer, integration tests, script routing #12699
• Raw command line (args[]), --also-make/--also-make-dependents in BuildEnvironment — pending further API work
• Implicitly activated profiles (activation by property/OS/file) — IDs only for now

PR chain

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

Test plan

• 17+ new unit/integration tests pass
• Sensitive user property keys are redacted in BuildEnvironment
• Full Maven test suite passes (same results as base branch)
• CI validation

[gnodet]

Well-designed build report feature with clean API/impl separation and comprehensive tests. A few issues noted below.

Also noted:

• The architecture is solid: clean API interfaces in maven-api-core, record-based implementations in maven-core, EventSpy pattern for automatic discovery, thread-based log routing for parallel-build safety, atomic file writes with symlink swap.
• The PR correctly depends on PR #12694 (logging foundation) — should not be merged until #12694 lands.
• No test for the captureLogEvent routing logic (mojo-level vs module-level vs build-level buffers). This is the core routing mechanism and warrants at least one test exercising the dispatch.

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]

Well-designed build report feature with clean API/impl separation, solid thread safety, and comprehensive tests. A few issues worth addressing:

Confirmed findings (verified independently):

1. [Medium] BuildReportCollector.java — The Javadoc on MAX_LOG_EVENTS_PER_SCOPE states "events are dropped and a truncation notice is appended," but captureLogEvent silently drops events without ever appending a truncation notice. Either implement the truncation notice (e.g., append a synthetic LogEvent like "... N events truncated") or correct the Javadoc to say events are silently dropped.

2. [Low] BuildReportJsonWriter.java — The writeNullableField method has an unused boolean hasMore parameter annotated with @SuppressWarnings("unused"). The parameter is never read and the method always emits a trailing comma regardless. Remove it to avoid confusion.

3. [Low] BuildReportJsonWriter.java — writeProblem uses sb.lastIndexOf(",\n") to remove trailing commas (searches entire buffer backwards), while writeLogEvent uses the dedicated removeTrailingComma helper (checks only last two characters). Use removeTrailingComma consistently — it's safer since lastIndexOf could theoretically match an earlier ,\n if future refactors change field order.

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 addition of a JSON build report feature with clean API design, good test coverage (17 tests), and defensive error handling. The zero-dependency JSON writer is appropriate for Maven's philosophy. A few design items:

Medium severity:

1. No opt-out mechanism (BuildReportCollector.java): The collector is unconditionally active for every build — every mvn invocation writes a JSON file to disk with no system property to disable it. Consider adding -Dmaven.build.report.skip=true for environments where this is undesirable (read-only filesystems, embedded invocations, CI runners).

2. MojoSkipped events not handled (BuildReportCollector.java): ExecutionEvent.Type.MojoSkipped (fired e.g. when a mojo requires online mode but Maven is offline) silently vanishes from the report. Inconsistent with ProjectSkipped which IS handled. Skipped mojos should be tracked with BuildStatus.SKIPPED.

Low severity:

3. Failure timestamp inaccuracy (BuildReportCollector.java line 1110): failureTimestamp is set to MonotonicClock.now() at report-assembly time, not at actual failure time. The mojo's timing data does capture the real timing — worth documenting in the FailureReport.timestamp() Javadoc.

4. Inconsistent trailing comma removal (BuildReportJsonWriter.java): writeProblem uses sb.lastIndexOf(",\n") which searches backwards through the entire buffer, while writeLogEvent uses the more robust removeTrailingComma(sb) which checks only the end. Consider using removeTrailingComma consistently.

5. Unused hasMore parameter (BuildReportJsonWriter.java line 1553): Annotated @SuppressWarnings("unused") and never referenced. Either use it to control comma behavior or remove it.

6. ~70 lines of duplicated test helpers: BuildReportCollectorTest and BuildReportIntegrationTest share identical createProject, createSession, createMojoExecution, and createEvent methods. Consider extracting to a shared test utility.

The API design (immutable interfaces in maven-api-core, record implementations in maven-core, @Experimental markers) follows Maven's established patterns. Thread safety approach is sound. The atomic file write + symlink pattern is well-implemented with proper fallbacks.

---

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-bot]

Re-review of squash commit 1dae6bb2 — full build report feature (API + impl + collector + JSON writer + tests).

Resolved since prior reviews:

• MAX_LOG_EVENTS_PER_SCOPE Javadoc now says "silently dropped" ✅
• writeNullableField hasMore parameter removed ✅
• writeProblem uses removeTrailingComma(sb) consistently ✅
• BuildEnvironment "not yet captured" section now only lists args[] and implicit profiles ✅
• Session.buildEnvironment() Javadoc no longer has "same object" claim ✅
• MavenSimpleLoggerTest — LogSink tests added ✅

Two items remain.

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

[gnodet-bot]

Re-review of squash commit 9be8b118 — single formatting fixup: writeNullableField signature reflowed to one line.

Two prior findings remain unaddressed.

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

[gnodet-bot]

Re-review of commit cbc76239 — SessionStub.buildEnvironment() stub added.

The stub impl is consistent with the rest of SessionStub (e.g. getMavenVersion() also returns null despite @Nonnull). No new bugs introduced by this commit.

Two findings from the prior review remain unaddressed. See inline comments.

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

[gnodet-bot]

Re-review of squash commit f656ea9 — full build-report feature including BuildEnvironment API, BuildReportCollector, BuildReportJsonWriter, and wiring into DefaultSession, ApiRunner, SessionStub.

Resolved since prior reviews:

• MAX_LOG_EVENTS_PER_SCOPE Javadoc fixed — now says "silently dropped" ✅
• hasMore parameter removed from writeNullableField ✅
• writeProblem now uses removeTrailingComma(sb) consistently ✅
• Session.buildEnvironment() Javadoc — "same object" claim removed, now says "immutable snapshot" ✅
• BuildEnvironment.java "not yet captured" section updated — batchMode and noTransferProgress removed from the list ✅

Two findings remain:

1. [Medium] SessionStub.buildEnvironment() returns null, violating the @Nonnull contract declared on Session.buildEnvironment(). Any test that calls session.buildEnvironment() through a SessionStub will get an NPE when it dereferences the result. See inline comment.

2. [Low] ApiRunner.buildEnvironment() constructs a new anonymous BuildEnvironment object on every call with no caching. Since the values are all static defaults, this should be a private static final constant. See inline comment.

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

[gnodet-bot]

(Note: ignore the preceding test review — posted in error.)

[gnodet-bot]

Re-review of squash commit f656ea9567ce — full build report feature (API + impl + BuildReportCollector + BuildReportJsonWriter + MavenJulHandler + JUL quiet-mode fix + tests).

All prior findings resolved since ee2e6c44:

• MAX_LOG_EVENTS_PER_SCOPE Javadoc now says "silently dropped" — truncation claim removed ✅
• Dead hasMore parameter removed from writeNullableField ✅
• mojoSkipped() now calls setMojoId(null) ✅
• Stale BuildEnvironment Javadoc (batchMode/noTransferProgress listed as "not yet captured") removed ✅
• LogEvent.sequenceNumber() Javadoc corrected to "or -1 if unavailable" ✅
• Session.buildEnvironment() no longer claims it returns "the same object" ✅
• writeProblem now uses removeTrailingComma consistently ✅

Two new issues.

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

[gnodet-bot]

Re-review of commit 5a244435 — addresses the two findings from the prior f656ea9 review.

Resolved since prior review:

• SessionStub.buildEnvironment() now returns a proper no-op implementation instead of null — @Nonnull contract honored ✅
• ApiRunner.DefaultSession.buildEnvironment() extracted to private static final EMPTY_BUILD_ENVIRONMENT — no longer allocates a new anonymous object on every call ✅
• DefaultSession.buildEnvironment() uses full double-checked locking (volatile field + synchronized(this) inner guard) — correct for a @ThreadSafe class ✅

Minor nit (non-blocking): SessionStub’s anonymous BuildEnvironment uses fully-qualified java.util.List.of() / java.util.Map.of() even though the top-level class already imports java.util.List and java.util.Map (which inner anonymous classes inherit). ApiRunner’s EMPTY_BUILD_ENVIRONMENT correctly uses the unqualified form. Not worth a change request — just worth aligning if the class is touched again.

All prior findings resolved. The build-report feature (API, impl, collector, JSON writer, tests) is well-structured. Approved.

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

Posted in error during testing — please disregard.

[gnodet-bot]

Re-review of commit 1b48e9c1 — synchronize buildLogBuffer iteration in buildReport().

All prior findings resolved. This commit addresses the one remaining correctness gap:

• buildLogBuffer is a Collections.synchronizedList. Its Javadoc explicitly requires external synchronization during traversal. List.copyOf() iterates internally, so without a synchronized (buildLogBuffer) guard it was susceptible to ConcurrentModificationException on concurrent log appends during onSessionEnded(). Fixed correctly — same pattern as moduleLogBuffer at line 519. ✅

Notes (non-blocking, pre-existing):

• The size() < MAX_LOG_EVENTS_PER_SCOPE check-then-add() in captureLogEvent (line ~306) is still an unsynchronized TOCTOU on buildLogBuffer. For a synchronizedList, individual size() and add() calls are atomic but the compound check-then-act can allow slightly more than MAX_LOG_EVENTS_PER_SCOPE events under heavy concurrency. The cap is a soft limit, so the impact is bounded and benign — not worth a change request. Same pattern exists for mojoLogBuffers and moduleLogBuffers.
• mojoLogBuffers.remove(mKey) → List.copyOf(logBuffer) correctly needs no synchronization: once removed from the map, logBuffer is a private reference with no concurrent writers.

The fix is correct, minimal, and well-explained in the commit message.

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

[gnodet-bot]

Re-review of commits 5a244435 + 1b48e9c1 — all four findings from the previous review are resolved. One new low-severity finding.

Resolved since last review:

• buildLogBuffer iteration in buildReport() is now synchronized ✅
• DefaultSession.buildEnvironment() now uses double-checked locking with volatile — caches correctly ✅
• ApiRunner.DefaultSession anonymous BuildEnvironment extracted to static constant EMPTY_BUILD_ENVIRONMENT ✅
• SessionStub.buildEnvironment() no longer returns null ✅

One new finding (inline below).

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

[gnodet-bot]

Re-review of commit 32901219 — MAX_THROWABLE_DEPTH depth guard in MavenSimpleLogger + SessionStub.buildEnvironment() static constant.

Resolved since prior review:

• SessionStub.buildEnvironment() now returns EMPTY_BUILD_ENVIRONMENT static constant instead of per-call anonymous class ✅

One new low-severity finding (inline below).

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

[gnodet-bot]

Re-review of commit 3abb55be — three fix commits + one test commit since prior review (ee2e6c44).

All previous findings resolved ✅

Every finding raised in prior reviews has been addressed:

• ✅ Truncation Javadoc: now says "silently dropped to prevent unbounded memory growth"
• ✅ Dead hasMore parameter removed; writeNullableField simplified to 4-arg signature
• ✅ writeProblem now uses removeTrailingComma() consistently
• ✅ mojoSkipped() now calls setMojoId(null) — mojo ID no longer leaked on skip
• ✅ Session.buildEnvironment() Javadoc: "same object" claim removed
• ✅ DefaultSession.buildEnvironment() cached via DCL + volatile field
• ✅ Log.java trace methods remain default — binary compat preserved
• ✅ hasReportCapture() guard restored in DefaultLog.withMetadata()
• ✅ BuildEnvironment stale Javadoc removed (batchMode/noTransferProgress)
• ✅ MavenSimpleLogger stack overflow guarded (MAX_THROWABLE_DEPTH=20 with truncation notice)
• ✅ LogEvent.sequenceNumber() Javadoc corrected to "or -1 if unavailable"
• ✅ MavenJulHandlerTest restored with setInPublishForTest hook

Two low-severity issues in the new commits:

1. buildLogBuffer TOCTOU (low) — see inline
2. isLoggable filter path untested (low) — see inline

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

[gnodet-bot]

Re-review of new commits since ee2e6c44 — BuildReportCollector TOCTOU fix, DefaultSession.buildEnvironment() double-checked locking, MavenSimpleLogger depth guard, SessionStub stub, and test additions.

Resolved since prior reviews:

• Log.java trace methods restored to default — binary-compat issue fixed ✅
• LoggingExecutionListener.mojoSkipped() now calls setMojoId(null) ✅
• LogEvent.sequenceNumber() Javadoc correctly says or -1 if unavailable ✅
• Session.java Javadoc no longer claims "same object" ✅
• DefaultSession.buildEnvironment() caches with double-checked locking (volatile field + synchronized block) ✅
• BuildReportCollector.MAX_LOG_EVENTS_PER_SCOPE Javadoc now says "silently dropped" ✅
• BuildReportJsonWriter.writeNullableField dead hasMore parameter removed ✅
• writeProblem now uses removeTrailingComma consistently ✅
• MavenSimpleLogger.writeThrowable depth guard (MAX_THROWABLE_DEPTH=20) ✅
• FORKING_MOJO_ID wired in forkStarted/forkSucceeded/forkFailed ✅
• buildLogBuffer TOCTOU fixed with external synchronized block ✅
• buildLogBuffer iteration in buildReport() correctly synchronized ✅
• SessionStub.buildEnvironment() returns a non-null constant ✅
• MavenJulHandlerTest now tests the filter path ✅

Two remaining items.

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

[gnodet-bot]

test - ignore

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

[gnodet-bot]

test inline

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

[gnodet-bot]

Re-review of commit 6d646283 — seven fix commits addressing prior review findings.

Resolved since prior reviews (ee2e6c4 and earlier):

• Dead hasMore parameter removed from writeNullableField ✅
• MAX_LOG_EVENTS_PER_SCOPE Javadoc updated: now correctly says "silently dropped" ✅
• Session.buildEnvironment() Javadoc: "same object" claim removed, says "immutable snapshot" ✅
• DefaultSession.buildEnvironment(): DCL caching with volatile field added ✅
• BuildEnvironment.java: batchMode/noTransferProgress removed from "What is not yet captured" ✅
• LogEvent.sequenceNumber() Javadoc: fixed to "or -1 if unavailable" ✅
• buildLogBuffer synchronization: Collections.synchronizedList + explicit synchronized block ✅
• MavenJulHandler: null logger-name guard added ✅

Still unresolved (1 finding): see inline comment.

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

[gnodet-bot]

Re-review of commit 11205e68 — MojoSkipped handling added to BuildReportCollector.

All prior findings resolved:

• MAX_LOG_EVENTS_PER_SCOPE Javadoc corrected to "silently dropped" ✅
• Dead hasMore parameter removed from writeNullableField ✅
• BuildEnvironment Javadoc updated: batchMode/noTransferProgress removed from "not yet captured" ✅
• Session.buildEnvironment() Javadoc no longer claims "same object" ✅
• writeProblem switched to removeTrailingComma ✅
• DefaultSession.buildEnvironment() now caches via DCL/volatile ✅
• mojoSkipped() in LoggingExecutionListener now calls setMojoId(null) ✅
• MojoSkipped events now recorded in BuildReportCollector via onMojoSkipped ✅ (this commit)

Review of this commit:

The onMojoSkipped implementation is correct. A skipped mojo has no preceding MojoStarted event, so:

• No currentMojoByThread entry to remove (correct — nothing was registered)
• No log buffer to drain (correct — no MojoStarted means no buffer was initialized)
• now as start timestamp and Duration.ZERO is accurate and honest
• computeIfAbsent on the synchronized list is consistent with onMojoFinished

The test covers the critical path: MojoSkipped fires without a preceding MojoStarted, status is SKIPPED, duration is ZERO, output is empty. Static analysis (ast-grep, semgrep) clean.

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

[gnodet-bot]

Re-review of commit 11205e680 — MojoSkipped handling in BuildReportCollector.

Resolved since prior review (6d646283):

• MojoSkipped events now routed to a new onMojoSkipped() handler — skipped mojos recorded with BuildStatus.SKIPPED, Duration.ZERO, and empty output ✅
• Consistent with ProjectSkipped handling (both route to a "finished" handler) ✅
• testMojoSkippedIsRecorded() regression guard added in BuildReportCollectorTest: fires MojoSkipped without a prior MojoStarted and asserts the report contains one entry with goal "test", status SKIPPED, duration ZERO, and empty output ✅

All prior findings resolved. The build report feature is complete and correct: clean API/impl separation, EventSpy-based collector with thread-safe log routing (ConcurrentHashMap + synchronizedList with proper external synchronization), atomic JSON file writes, and comprehensive coverage of all mojo lifecycle events (started, succeeded, failed, skipped).

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