Add size-capped module output to run reports - #3842
Conversation
Retain masked, UTF-8-capped stdout and stderr tails so historical reports can diagnose failures without scraping CI logs.
|
@coderabbitai review |
There was a problem hiding this comment.
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, andTrimToLimitre-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:
CaptureOutputExcerptruns inside the same_lockblock as_outputs.AddinAddOutput, andGetOutputExcerpttakes 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 inPipelineRunReportFactory.CreateOutputExcerptat report-creation time — covering secrets registered later in the run. This matches the PR description and is backed byRunReportIncludesMaskedModuleOutputWhenEnabledandRunReportMasksOutputAgainAtCreation. stdout/stderrrouting:_isErroris correctly wired only on the error writer (isError: trueon_coordinatedError), andBufferedLogEvent.GetStreamreuses the existingCommandErrorproperty-key convention to classify structured log events.- Schema/JSON:
ModuleOutputExcerptis public and reachable fromPipelineRunReportviaModuleRunReport.Output, so theSystem.Text.Jsonsource generator picks it up transitively forRunReportJsonContext— 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.
|
Addressed the review note in fdbf0bb: MaxOutputBytesPerModule is now validated only when IncludeModuleOutput is enabled. Added enabled/disabled contract tests; both focused tests pass. |
Code reviewRe-reviewed after the latest commit ( Verified the fix:
No new issues found. This closes out the previous review note cleanly. |
|
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 |
Code reviewRe-reviewed after the latest commit ( Verified:
No new issues found. This closes out cleanly — nice, focused fix. |
…48-module-output-excerpts
Code reviewRe-reviewed at the latest state ( 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: 🔴
|
Derive the byte cap from the expected tail so the assertion accounts for Windows CRLF and Unix LF consistently.
|
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. |
|
Independent exact-head validation: ModuleOutputExcerptBufferTests 3/3, ModuleOutputBufferTests 32/32, RunReportTests 60/60, core Release build 0 warnings/errors. @claude review |
Code reviewRe-reviewed at the latest state ( Verified the fix (
Everything else from prior rounds still holds and is unchanged by this commit (confirmed the diff from
Docs ( 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
|
Fixed the three-platform compile failure at |
|
@claude review |
Code reviewRe-reviewed at the latest state ( Verified independently (not just by reading the diff):
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 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
|
Refreshed onto current Validation:
|
|
@claude review |
Code reviewRe-reviewed at the latest state ( Verified this is a merge-only update with no functional change to this PR's feature:
Everything from the prior five review rounds still holds and is unaffected by this merge: locking ( 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.
|
Fixed the branch-specific Ubuntu failure at Validation:
@claude review |
Code reviewRe-reviewed at the latest state ( Verified this is a minimal, test-only change:
Everything from the prior six review rounds still holds (locking, double-masking, stdout/stderr routing convention, UTF-8 tail-trim boundary math, No new issues found. This closes out cleanly — good fix for a legitimate test-isolation race rather than a product bug. |
|
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. |
Code reviewRe-reviewed at the latest state ( Verified this is a merge + mechanical-fix update with no functional change to this PR's own feature:
Everything from the prior eight review rounds still holds and is unaffected by this update: locking ( No new issues found. This closes out cleanly — the merge is clean and the compile fix is correct and complete. |
|
CI triage at |
|
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. |
Code reviewRe-reviewed at the latest state ( Confirmed this is a merge-only update with no functional change to this PR's own feature:
Everything from the prior nine review rounds still holds and is unaffected by this update: locking ( No new issues found. This is another clean merge — no architectural concerns to raise beyond what's already been resolved in earlier rounds. |
Closes #3748
Summary
ModuleRunReport.Outputexcerpts with separate stdout/stderr tailsValidation
ModuleOutputExcerptBufferTests: 3/3ModuleOutputBufferTests: 32/32RunReportTests: 50/50, plus focused JSON round-trip 1/1ModularPipelines.slnxRelease build: 0 warnings, 0 errors