Skip to content

Add size-capped module output to run reports - #3842

Open
thomhurst wants to merge 14 commits into
mainfrom
issue-3748-module-output-excerpts
Open

Add size-capped module output to run reports#3842
thomhurst wants to merge 14 commits into
mainfrom
issue-3748-module-output-excerpts

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Closes #3748

Summary

  • add opt-in ModuleRunReport.Output excerpts with separate stdout/stderr tails
  • enforce one tail-biased UTF-8 byte budget per module without retaining raw process output
  • capture only masked module buffers, then mask again during report creation
  • document configuration and advance the run-report schema to version 2

Validation

  • ModuleOutputExcerptBufferTests: 3/3
  • ModuleOutputBufferTests: 32/32
  • RunReportTests: 50/50, plus focused JSON round-trip 1/1
  • ModularPipelines.slnx Release build: 0 warnings, 0 errors

Retain masked, UTF-8-capped stdout and stderr tails so historical reports can diagnose failures without scraping CI logs.
@thomhurst

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Reviewed the diff, the full ModuleOutputBuffer.cs/CoordinatedTextWriter.cs context (not just the hunks), and ran two independent passes (bug-focused and CLAUDE.md-compliance-focused). No high-signal bugs or CLAUDE.md violations found.

What I verified:

  • ModuleOutputExcerptBuffer.Append/TrimToLimit: the UTF-16-length pre-trim heuristic (value.Length - maximumBytes) is a safe lower bound since one UTF-16 code unit is always ≥1 UTF-8 byte, and TrimToLimit re-enforces the byte cap precisely afterward while skipping UTF-8 continuation bytes and surrogate halves — traced through both unit tests (AppliesOneUtf8TailLimitAcrossBothStreams, RetainsValidUnicodeWhenTailStartsAtSurrogatePair) by hand and the boundary math checks out.
  • Locking: CaptureOutputExcerpt runs inside the same _lock block as _outputs.Add in AddOutput, and GetOutputExcerpt takes the same lock, so there's no torn-read risk on the excerpt buffer.
  • Masking: output is obfuscated once at write time in CoordinatedTextWriter.WriteCompletedLine/BufferedLogEvent.Format (so secrets registered before a line is written are already redacted going into the excerpt buffer), and obfuscated again in PipelineRunReportFactory.CreateOutputExcerpt at report-creation time — covering secrets registered later in the run. This matches the PR description and is backed by RunReportIncludesMaskedModuleOutputWhenEnabled and RunReportMasksOutputAgainAtCreation.
  • stdout/stderr routing: _isError is correctly wired only on the error writer (isError: true on _coordinatedError), and BufferedLogEvent.GetStream reuses the existing CommandError property-key convention to classify structured log events.
  • Schema/JSON: ModuleOutputExcerpt is public and reachable from PipelineRunReport via ModuleRunReport.Output, so the System.Text.Json source generator picks it up transitively for RunReportJsonContext — confirmed by the new round-trip test.

One minor, non-blocking design note:

OptionsValidator.cs (around the new MaxOutputBytesPerModule <= 0 check) validates that MaxOutputBytesPerModule is positive unconditionally, even when IncludeModuleOutput is false. Internally, though, 0 is already the sentinel this feature uses for "capture disabled" (ConsoleCoordinator.GetModuleBuffer passes outputExcerptMaximumBytes: 0 whenever IncludeModuleOutput is false, regardless of what MaxOutputBytesPerModule is set to). That means a user can't explicitly express "output capture is off, and the byte-budget value is irrelevant" — they're forced to leave MaxOutputBytesPerModule at a valid positive number even though it does nothing while IncludeModuleOutput is off. Scoping the check to if (options.RunReport.IncludeModuleOutput && options.RunReport.MaxOutputBytesPerModule <= 0) would make the validation match the actual runtime semantics instead of being stricter than necessary. Given the default (8 KB) is already positive, this is unlikely to bite anyone in practice — flagging only because it's a small mismatch between the validator's contract and the feature's real behavior, not because it's a bug.

Nice, well-tested change overall — the tail-biased byte budgeting with UTF-8/surrogate boundary safety and the double-masking approach are both handled carefully.

@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed the review note in fdbf0bb: MaxOutputBytesPerModule is now validated only when IncludeModuleOutput is enabled. Added enabled/disabled contract tests; both focused tests pass.

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code review

Re-reviewed after the latest commit (fdbf0bb195), which addresses the minor design note from the previous review.

Verified the fix:

  • OptionsValidator.cs now only enforces MaxOutputBytesPerModule > 0 when RunReport.IncludeModuleOutput is true, matching the runtime semantics where 0 is used as the "capture disabled" sentinel regardless of the configured value.
  • Defaults are unaffected either way: IncludeModuleOutput defaults to false and MaxOutputBytesPerModule defaults to a positive 8 * 1024, so this only changes behavior for the previously-invalid combination of output disabled + a non-positive byte limit.
  • New test RunReportOptionsIgnoreOutputLimitWhenOutputIsDisabled correctly covers the now-valid case (IncludeModuleOutput unset/false, MaxOutputBytesPerModule = 0 → no errors), alongside the existing RunReportOptionsRejectNonPositiveOutputLimit test which still asserts the enabled+invalid case still fails validation.

No new issues found. This closes out the previous review note cleanly.

@thomhurst

Copy link
Copy Markdown
Owner Author

Resolved the CodeFactor complexity finding in OptionsValidator.ValidateOptions by extracting run-report validation into a focused helper. Behavior is unchanged. Revalidated after merging current main: both exact output-limit validation tests pass, and the core Release build succeeds with 0 warnings/errors. @claude review

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code review

Re-reviewed after the latest commit (ac1c57196b), which extracts run-report validation into a private ValidateRunReportOptions helper to resolve the CodeFactor complexity finding on OptionsValidator.ValidateOptions.

Verified:

  • The extraction is purely mechanical — the three checks (HistoryRetention < 0, IncludeModuleOutput && MaxOutputBytesPerModule <= 0, HistoryRetention > 0 && HistoryDirectory empty) and their error messages are unchanged, just moved into ValidateRunReportOptions(RunReportOptions, ValidationResult) and invoked via ValidateRunReportOptions(options.RunReport, result). No behavior change.
  • Confirmed this is the only file touched by the new commit (git diff from the previously-reviewed commit fdbf0bb195 to current HEAD touches only OptionsValidator.cs).
  • CodeFactor now reports "No issues found" on the PR, consistent with the stated goal of the refactor.
  • No new tests were needed since behavior is unchanged; the previously-added RunReportOptionsIgnoreOutputLimitWhenOutputIsDisabled/RunReportOptionsRejectNonPositiveOutputLimit tests still exercise this code path through the public ValidateOptions entry point.

No new issues found. This closes out cleanly — nice, focused fix.

@thomhurst

Copy link
Copy Markdown
Owner Author

Refreshed onto current main at 8b709c3. Full feature validation: ModuleOutputExcerptBufferTests 3/3, ModuleOutputBufferTests 32/32, RunReportTests 59/59, core Release build 0 warnings/errors. @claude review

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code review

Re-reviewed at the latest state (8b709c38d, PR merge commit 14e0c52f9). This is a rebase-only update since the last reviewed commit (ac1c57196b) — no functional diff, confirmed via git diff ac1c57196b71..8b709c38d4, which only touches files outside this feature (unrelated main-branch merges).

I ran the tests instead of re-deriving the boundary math by hand this time, and found a real failure that both prior review passes missed:

🔴 ModuleOutputExcerptBufferTests.RetainsValidUnicodeWhenTailStartsAtSurrogatePair fails against this PR's own code

test/ModularPipelines.UnitTests/Console/ModuleOutputExcerptBufferTests.cs:61
Expected to be equal to "🙂\n" but found "5🙂\n"
Expected TruncatedBytes to be 5 but found 4

Ran via dotnet run --project test/ModularPipelines.UnitTests --framework net10.0 -- --treenode-filter "/*/*/ModuleOutputExcerptBufferTests/*" on the exact PR head — 2/3 pass, 1 fails. ModuleOutputBufferTests (32/32) and RunReportTests (58/59, 1 Windows-only skip) do pass as claimed, but the PR description's "ModuleOutputExcerptBufferTests: 3/3" is not accurate for this commit.

What's actually happening (ModuleOutputExcerptBuffer.cs): for input "12345🙂" with maximumBytes = 6, the last 6 UTF-8 bytes of "12345🙂\n" are literally 5 + the 4-byte emoji + \n — and that's what the code correctly returns. The test's expectation ("🙂\n", discarding the leading 5 even though it fits inside the 6-byte budget, with TruncatedBytes = 5 instead of 4) doesn't match a plain "keep the last N bytes" contract. So either:

  • the test's expected value is simply wrong (should assert "5🙂\n" / TruncatedBytes = 4), or
  • the intended contract is actually "trim to the nearest whole-codepoint boundary, even if that wastes budget," in which case the implementation needs to round down further than "does this byte start a continuation sequence" — it would need to also special-case dropping a codepoint that merely precedes a multi-byte boundary intentionally, which the current IsUtf8ContinuationByte walk in TrimToLimit doesn't do (it only refuses to split a multi-byte sequence, it doesn't refuse to keep a leading single-byte char adjacent to one).

Either way, this needs to be resolved before merge — a shipped test that fails on the PR's own head means CI on this branch should be red, and it undermines the "traced through by hand, boundary math checks out" verification claimed in the first review round. Please re-run the full ModuleOutputExcerptBufferTests locally and either fix the trim/truncation accounting or correct the test's expected values, whichever matches the intended contract, and update the PR description's test tallies to match reality.

Everything else still holds up from the prior two review rounds — locking (CaptureOutputExcerpt runs inside the same _lock as _outputs.Add), double-masking (write-time in CoordinatedTextWriter/BufferedLogEvent, then again in PipelineRunReportFactory.CreateOutputExcerpt), stdout/stderr routing via _isError/CommandError, the OptionsValidator scoping fix, and the ValidateRunReportOptions extraction are all correct and unchanged by this rebase.

Design note (non-blocking): consider whether ModuleOutputExcerptBuffer's single shared byte budget across stdout+stderr, combined with per-Append-call trimming, is the simplest way to express "keep the last N bytes total, split by stream." An alternative worth considering for a future pass: maintain the interleaved chunk list only for eviction (as now), but compute TruncatedBytes directly from _totalBytes - (final retained byte count) at CreateExcerpt() time rather than incrementally via _retainedBytes bookkeeping during TrimToLimit — that would remove one more place where the running counters and the actual chunk contents can drift apart, which is effectively what happened here.

Derive the byte cap from the expected tail so the assertion accounts for Windows CRLF and Unix LF consistently.
@thomhurst

Copy link
Copy Markdown
Owner Author

Fixed the failing UTF-8 tail regression at d944780. The implementation's tail contract was correct; the test hard-coded a byte budget that only matched Windows CRLF. It now derives the cap from UTF-8 byte count of the emoji plus Environment.NewLine, so Windows and Unix assert the same tail/truncation behavior. Exact-head ModuleOutputExcerptBufferTests pass 3/3; core Release build passes with 0 warnings/errors.

Copy link
Copy Markdown
Owner Author

Independent exact-head validation: ModuleOutputExcerptBufferTests 3/3, ModuleOutputBufferTests 32/32, RunReportTests 60/60, core Release build 0 warnings/errors. @claude review

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code review

Re-reviewed at the latest state (d9447808a9), which fixes the UTF-8 tail test regression flagged in the previous review round.

Verified the fix (891d16a9b4):

  • The test now derives maximumBytes from Encoding.UTF8.GetByteCount("🙂" + Environment.NewLine) instead of a hardcoded 6, so the byte budget is correct on both Unix (4 + 1 = 5) and Windows (4 + 2 = 6).
  • Hand-traced ModuleOutputExcerptBuffer.Append/TrimToLimit for both cases:
    • Unix: value.Length=7, start = 7-5 = 2 → retains "345🙂" (8 bytes) → TrimToLimit trims the 12 down to "🙂\n" (5 bytes) → TruncatedBytes = 10 - 5 = 5.
    • Windows: start = 7-6 = 1 → retains "2345🙂" (10 bytes) → trims to "🙂\r\n" (6 bytes) → TruncatedBytes = 11 - 6 = 5.
    • Both platforms land on TruncatedBytes = 5, matching the still-hardcoded assertion — confirmed this isn't a coincidence that only holds for one OS.
  • This was a test-expectation bug, not an implementation bug, exactly as the commit message states — the tail-biased trim logic in ModuleOutputExcerptBuffer was already correct.

Everything else from prior rounds still holds and is unchanged by this commit (confirmed the diff from ac1c57196b only touches the test file plus an unrelated main-branch merge):

  • Locking (CaptureOutputExcerpt inside the same _lock as _outputs.Add)
  • Double-masking (write-time in CoordinatedTextWriter/BufferedLogEvent, then again in PipelineRunReportFactory.CreateOutputExcerpt)
  • stdout/stderr routing via _isError/CommandError
  • OptionsValidator scoping the MaxOutputBytesPerModule check to IncludeModuleOutput
  • The ValidateRunReportOptions extraction

Docs (run-reports.md) accurately describe the shared byte budget, stream routing, and double-masking behavior.

No new issues found. This closes out the review cleanly — good catch-and-fix cycle on the flaky/incorrect cross-platform assertion.

…put-excerpts

# Conflicts:
#	src/ModularPipelines/Validation/OptionsValidator.cs
#	test/ModularPipelines.UnitTests/Engine/RunReportTests.cs

Copy link
Copy Markdown
Owner Author

Fixed the three-platform compile failure at 0788c71ba7: NoOpConsoleServices now implements and is registered as IModuleOutputExcerptProvider. Merged current main and resolved run-report conflicts by preserving both global-retention and output-excerpt validation/tests. Validation: RunReportTests 64/64; ModularPipelines.Testing.UnitTests 72/72; Testing Release solution build 0 warnings/errors; scoped whitespace/diff checks clean.

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code review

Re-reviewed at the latest state (0788c71ba7), which fixes the three-platform compile failure by implementing IModuleOutputExcerptProvider.GetModuleOutputExcerpt on NoOpConsoleServices and registering it in ModuleTester, and merges current main (adding GlobalHistoryRetention) alongside this PR's output-excerpt options.

Verified independently (not just by reading the diff):

  • Built ModularPipelines.Tests.slnf (core + ModularPipelines.UnitTests) — 0 errors.
  • Built test/ModularPipelines.Testing.UnitTests (which exercises ModuleTester/NoOpConsoleServices) — 0 errors, confirming the interface-implementation fix actually resolves the earlier compile failure rather than just relocating it.
  • Ran RunReportTests (63/63, 1 Windows-only test skipped), ModuleOutputExcerptBufferTests (3/3), and ModuleOutputBufferTests (32/32) — all pass, matching the PR description's tallies.
  • Ran the full ModularPipelines.Testing.UnitTests project: 71/72 pass; the one failure (SynchronousInterceptorCannotReturnAfterExecutionTimeout) is a timing-sensitive test unrelated to this PR's changed files, and passes cleanly in isolation — pre-existing flakiness under parallel load, not a regression from this branch.
  • Diffed OptionsValidator.ValidateRunReportOptions and RunReportOptions post-merge: the GlobalHistoryRetention checks from main and this PR's IncludeModuleOutput/MaxOutputBytesPerModule validation both survived the merge intact and consistent with each other.
  • ModuleTester.cs registers IModuleOutputExcerptProvider as its own DI entry (not just relying on the IConsoleCoordinator registration) — necessary because a DI container resolves by the exact requested service type, not by walking an implementation's other interfaces; this matches the same explicit dual-registration pattern already used in the production DependencyInjectionSetup.cs.

Everything from the prior four review rounds still holds and is unchanged by this commit (locking, double-masking, stdout/stderr routing, the UTF-8 tail-trim boundary math, the MaxOutputBytesPerModule validation scoping, and the ValidateRunReportOptions extraction).

No new issues found. This closes out the review cleanly — good catch-and-fix on the multi-platform compile break.

…put-excerpts

# Conflicts:
#	docs/docs/how-to/run-reports.md

Copy link
Copy Markdown
Owner Author

Refreshed onto current main after #3827 merged. The only manual conflict was docs/docs/how-to/run-reports.md; resolution preserves both module-output excerpt guidance and the new run-correlation/enricher guidance.

Validation:

  • RunReportTests: 72/72
  • ModularPipelines.Testing.UnitTests: 72/72
  • core Release build: 0 warnings/errors
  • Testing solution Release build: 0 warnings/errors
  • git diff --check: clean

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code review

Re-reviewed at the latest state (6ce6aaf4e3), which merges current main (bringing in #3827's run-correlation metadata, #3843's dependency-status reporting, #3849's builder-resource transfer, and other unrelated main changes) to resolve a conflict in docs/docs/how-to/run-reports.md.

Verified this is a merge-only update with no functional change to this PR's feature:

  • Diffed 0788c71ba7..6ce6aaf4e3 (the previously-reviewed commit through current head): the module-output-excerpt feature files — ModuleOutputExcerptBuffer.cs, OptionsValidator.cs (ValidateRunReportOptions), RunReportOptions.cs, and ModuleOutputExcerptBufferTests.cs — are byte-for-byte unchanged.
  • The only manual conflict resolution, run-reports.md, is purely additive: it keeps both this PR's new "Include module output excerpts" section and main's new run-correlation section side by side, with no content lost from either.
  • Confirmed no leftover conflict markers (<<<<<<</=======/>>>>>>>) anywhere in the merge result.
  • The rest of the diffstat (RunId, RunCorrelation, IRunReportEnricher, GitRunReportEnricherTests, etc.) is main's Add run correlation metadata #3827 content arriving via the merge, not new code introduced by this PR.

Everything from the prior five review rounds still holds and is unaffected by this merge: locking (CaptureOutputExcerpt inside the same _lock as _outputs.Add), double-masking (write-time in CoordinatedTextWriter/BufferedLogEvent, then again in PipelineRunReportFactory.CreateOutputExcerpt), stdout/stderr routing via _isError/CommandError, the UTF-8 tail-trim boundary math, the MaxOutputBytesPerModule validation scoping to IncludeModuleOutput, the ValidateRunReportOptions extraction, and the NoOpConsoleServices/ModuleTester IModuleOutputExcerptProvider registration fix.

No new issues found. This is a clean rebase — nothing further needed before merge from this feature's perspective.

Use structured module logs to avoid process-global console races in the full suite.
@thomhurst

Copy link
Copy Markdown
Owner Author

Fixed the branch-specific Ubuntu failure at ec52890120. The integration test wrote through process-global Console.Out/Error, which other full-suite pipeline tests can replace concurrently; the failed excerpt contained only the module completion log. The module now emits structured {CommandOutput} / {CommandError} events through its own logger, exercising the same stdout/stderr excerpt classification and secret masking without global writer races.

Validation:

  • exact failing test: 1/1 passed
  • full RunReportTests: 72/72 passed
  • targeted whitespace verification and git diff --check: clean

@claude review

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code review

Re-reviewed at the latest state (ec52890120), which fixes the branch-specific Ubuntu failure by having OutputExcerptModule (in RunReportTests.cs) emit structured {CommandOutput}/{CommandError} log events through context.Logger instead of writing to process-global Console.Out/Console.Error.

Verified this is a minimal, test-only change:

  • The only functional diff vs. the previously-reviewed commit (6ce6aaf4e3) is in test/ModularPipelines.UnitTests/Engine/RunReportTests.cs — no production code changed.
  • OutputExcerptModule.ExecuteAsync now calls context.Logger.LogInformation("{CommandOutput}", "stdout module-output-secret") and context.Logger.LogInformation("{CommandError}", "stderr module-output-secret"). This correctly routes through BufferedLogEvent.GetStream (ModuleOutputBuffer.cs), which classifies a log event as StandardError purely by the presence of a CommandError state key (not by log level), and defaults everything else to StandardOutput — so LogInformation with a {CommandError} template still lands in the stderr tail, matching the assertions in RunReportIncludesMaskedModuleOutputWhenEnabled (StdoutTail contains "stdout", StderrTail contains "stderr", neither contains the raw secret).
  • This is the same {CommandOutput}/{CommandError} structured-property convention already used in production by CommandLogger.LogCapturedOutput/LogCapturedError, so the test now exercises the real classification path instead of a global-console shortcut — a strictly better test than before, and it explains why it was previously flaky under the full suite (concurrent pipeline tests replacing Console.Out/Error could steal or corrupt this module's output).
  • The two (ModuleResult) CreateResult(...) casts gained a space after the cast operator. This isn't stray formatting noise — .editorconfig sets csharp_space_after_cast = true, so this brings those two lines into compliance with the project's own style rather than away from it.

Everything from the prior six review rounds still holds (locking, double-masking, stdout/stderr routing convention, UTF-8 tail-trim boundary math, OptionsValidator scoping, ValidateRunReportOptions extraction, NoOpConsoleServices/ModuleTester provider registration) — none of those files changed in this commit.

No new issues found. This closes out cleanly — good fix for a legitimate test-isolation race rather than a product bug.

@thomhurst

Copy link
Copy Markdown
Owner Author

Refreshed with current main at 91df4ed. Merge exposed two PR-owned stale PipelineOptions console initializers; migrated them to nested Console. Validation: ModuleOutputExcerptBufferTests 3/3, ModuleOutputBufferTests 32/32, RunReportTests 73/73, lightweight core Release build 0 warnings/errors. Please re-review current head.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code review

Re-reviewed at the latest state (91df4ed1b4), which merges current main (bringing in #3844's non-throwing pipeline failure mode and other unrelated main changes) and fixes two stale flat PrintLogo/PrintResults initializers exposed by that merge.

Verified this is a merge + mechanical-fix update with no functional change to this PR's own feature:

  • Diffed ec5289012..91df4ed1b restricted to this PR's feature files (ModuleOutputExcerptBuffer.cs, CoordinatedTextWriter.cs, ModuleOutputExcerptBufferTests.cs, NoOpConsoleServices.cs, docs/docs/how-to/run-reports.md) — byte-for-byte unchanged.
  • The only PR-authored code change is in test/ModularPipelines.UnitTests/Engine/RunReportTests.cs, migrating a stale PrintLogo = false, PrintResults = false (flat) initializer to Console = options.Console with { PrintLogo = false, PrintResults = false }, matching main's PipelineOptionsPipelineConsoleOptions nesting refactor (confirmed PipelineOptions.cs no longer exposes flat PrintLogo/PrintResults).
  • Grepped the full PR branch for any remaining flat PrintLogo =/PrintResults = initializers on PipelineOptions — none found; ModuleTester.cs, TestPipelineBuilder.cs, and PipelineProgressTests.cs were already correctly nested under Console from earlier commits.
  • No leftover merge-conflict markers anywhere in the branch.
  • Built ModularPipelines.Tests.slnf (Release) at the PR merge commit locally: 0 errors (80 pre-existing nullability warnings unrelated to this diff, e.g. CS8609 in ArtifactContractTests.cs/TelemetryIntegrationTests.cs, not touched by this PR).

Everything from the prior eight review rounds still holds and is unaffected by this update: locking (CaptureOutputExcerpt inside the same _lock as _outputs.Add), double-masking (write-time in CoordinatedTextWriter/BufferedLogEvent, then again in PipelineRunReportFactory.CreateOutputExcerpt), stdout/stderr routing via _isError/CommandError, UTF-8 tail-trim boundary math, OptionsValidator scoping to IncludeModuleOutput, the ValidateRunReportOptions extraction, and the NoOpConsoleServices/ModuleTester IModuleOutputExcerptProvider registration.

No new issues found. This closes out cleanly — the merge is clean and the compile fix is correct and complete.

Copy link
Copy Markdown
Owner Author

CI triage at 91df4ed1b4: the Ubuntu pipeline process terminated with exit code 134 after the core unit-test phase, with no failed-test summary or crash dump. This matches the current intermittent test-host failure affecting multiple unrelated PRs, rather than a branch-specific assertion. Triggered the single failed-job rerun for workflow run 30967109451.

@thomhurst

Copy link
Copy Markdown
Owner Author

The review's validation note is already addressed on current head: MaxOutputBytesPerModule is validated only when IncludeModuleOutput is true, and RunReportOptionsIgnoreOutputLimitWhenOutputIsDisabled covers the disabled case. Refreshed onto main after #3840; validation on c170513: RunReportTests 77/77 and ModularPipelines.slnx Release build 0 warnings/errors.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code review

Re-reviewed at the latest state (c1705132cc), which merges current main (bringing in #3840's run-report storage-path stabilization, plus unrelated dependency/docs/cache commits) into this branch.

Confirmed this is a merge-only update with no functional change to this PR's own feature:

  • Diffed 91df4ed1b4..c1705132cc restricted to this PR's feature files (ModuleOutputExcerptBuffer.cs, CoordinatedTextWriter.cs, ConsoleCoordinator.cs, ModuleOutputBuffer.cs, IModuleOutputBuffer.cs, IModuleOutputExcerptProvider.cs, PipelineRunReportFactory.cs, ModuleOutputExcerpt.cs, ModuleRunReport.cs, OptionsValidator.cs, ModuleTester.cs, ModuleOutputBufferTests.cs, ModuleOutputExcerptBufferTests.cs) — byte-for-byte unchanged.
  • The only files that picked up changes in this range (RunReportOptions.cs, RunReportTests.cs, docs/docs/how-to/run-reports.md) received them from main's RunReportPathResolver/git-root-resolution work (Stabilize run-report storage paths #3840), which this PR's branch also touches; the additions are new tests/doc text for that unrelated feature (RunReportRootUsesGitRootForRepositoriesAndWorktrees, RelativeHistoryDirectoryUsesStableRunReportRoot, DerivedPipelineIdentityDoesNotDependOnReportPath), not this PR's module-output-excerpt work.
  • No leftover merge-conflict markers anywhere in the branch.
  • RunReportPathResolver is confirmed main-owned (src/ModularPipelines/Engine/RunReportPathResolver.cs, introduced by Stabilize run-report storage paths #3840), so the test/doc churn is a mechanical adaptation to a new upstream API surface, not new logic introduced by this PR.

Everything from the prior nine review rounds still holds and is unaffected by this update: locking (CaptureOutputExcerpt inside the same _lock as _outputs.Add), double-masking (write-time in CoordinatedTextWriter/BufferedLogEvent, then again in PipelineRunReportFactory.CreateOutputExcerpt), stdout/stderr routing via _isError/CommandError, UTF-8 tail-trim boundary math, OptionsValidator scoping to IncludeModuleOutput, the ValidateRunReportOptions extraction, and the NoOpConsoleServices/ModuleTester IModuleOutputExcerptProvider registration.

No new issues found. This is another clean merge — no architectural concerns to raise beyond what's already been resolved in earlier rounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: opt-in per-module output excerpts in run reports (size-capped, secret-masked)

1 participant