Add size-capped module output to run reports

[thomhurst]

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

Summary by CodeRabbit

• New Features

  • Run reports can include optional, size-limited excerpts of module standard output and error output.
  • Excerpts preserve separate streams, indicate truncation, and mask sensitive values.
  • Added configuration for enabling excerpts and setting per-module output limits.
  • Updated the run report schema to version 4.

• Bug Fixes

  • Improved preservation and categorization of standard output versus error output.

• Documentation

  • Added guidance for configuring module output excerpts in run reports.

[thomhurst]

@coderabbitai review

[claude]

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]

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]

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]

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]

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]

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]

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.

[thomhurst]

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.

[thomhurst]

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

[claude]

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.

[thomhurst]

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.

[thomhurst]

@claude review

[claude]

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.

[thomhurst]

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

[thomhurst]

@claude review

[claude]

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.

[thomhurst]

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]

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]

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]

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 PipelineOptions → PipelineConsoleOptions 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.

[thomhurst]

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]

Addressed both latest review findings in 68472cc: coalesced small output writes before masked suffix analysis, and removed eager enumeration of arbitrary structured logging state. Validation: excerpt 20/20; output buffer 38/38; core Release build 0 warnings/errors.

[thomhurst]

@codex review exact head 68472ccdd95839698fc99d357528af50f78030e5``n@claude review exact head 68472cc`

[thomhurst]

@codex review exact head 68472ccdd95839698fc99d357528af50f78030e5
@claude review exact head 68472ccdd95839698fc99d357528af50f78030e5

[claude]

Code review

Re-reviewed exact head 68472ccdd9 as requested.

What changed since the last review (51c13213be → 68472ccd): purely a performance refactor plus a defensive fix, neither of which touches the two open blocking issues.

• ModuleOutputExcerptBuffer.RebalanceMaskedStreamByteLimits now builds suffix text from a new GetCoalescedChunks() helper instead of re-walking/re-decoding _chunks on every iteration. This avoids the previous O(n²) re-stringification for inputs with many small Append calls; the new CoalescesManySmallWritesBeforeMaskedSuffixAnalysis test covers it and the values produced are unchanged (I traced it — coalescing adjacent same-stream chunks before building suffixes is byte-for-byte equivalent to the prior walk).
• ModuleOutputBuffer.GetStream swapped a LINQ .Any over IEnumerable<KeyValuePair<...>> for an indexed loop over IReadOnlyList<KeyValuePair<...>> wrapped in a catch-and-default-to-stdout guard, since structured logging state is caller-controlled and classification must stay best-effort.

Both are reasonable, low-risk changes. Neither file's CreateExcerpt/GetTruncatedByteCount (ModuleOutputExcerptBuffer.cs:74-117) or RunReportService.CompleteAsync/PrepareHistoryReport (RunReportService.cs:75-108) was touched by this commit.

Still open — both flagged in the prior two review rounds, neither addressed here

🔴 TruncatedBytes still diverges from the actual masked tail split. CreateExcerpt computes truncatedBytes from GetFinalStreamByteLimits(stdoutValue, stderrValue) — a split over the raw, unmasked text — then separately calls TryCreateTails → TryCreateMaskedTails, which throws that split away and recomputes an independent one via RebalanceMaskedStreamByteLimits over masked text. stdoutBytes/stderrBytes are plain method parameters, so the reassignment at line 180 is local to TryCreateMaskedTails and never flows back to the truncatedBytes already computed in CreateExcerpt. The repro from the last round (10-byte budget, a 1-char secret, "aaaa" on stdout then "a" on stderr) still reproduces on this exact head: StdoutTail comes back null (the entire stdout excerpt silently dropped by the masked rebalance) while TruncatedBytes reports 0.

🟠 PrepareHistoryReport still strips excerpts from the primary report file, not just the history store. CompleteAsync reassigns the single shared report variable through PrepareHistoryReport before both WriteReportAsync and the method's return. When historyEnabled is true and a non-FileSystemRunHistoryStore is configured, this strips excerpts from the file written to reportPath and from the report handed back to the caller — not just the copy passed to historyStore.SaveAsync. Confirmed unchanged in this commit; RunReportService.cs isn't among the files touched between 51c13213be and 68472ccd.

Why this is the architectural issue worth fixing now, not the two symptoms

This is the third distinct formula for "what did we actually keep" that ModuleOutputExcerptBuffer has grown (GetFinalStreamByteLimits over raw text, RebalanceMaskedStreamByteLimits over masked text, and GetTruncatedByteCount diffing yet another tail computation against _totalBytes), and each fix in this thread has patched the specific input that broke a given formula rather than removing the duplication that lets them drift. The fact that the fix landed at 0c950ce7/542567c5/475c3244 earlier in this PR and then reappeared under a new repro is the signature of "N independent derivations that must agree" rather than "one bug." Consolidating around a single source of truth — track bytes evicted/retained as explicit state updated exactly once, at the point in Append/TrimToLimit/the masked rebalance where truncation actually happens, and derive both TruncatedBytes and the tails from that same state — would close the whole class instead of the next instance of it. The RunReportService issue has the same shape at a different layer: "which history backend is configured" and "what excerpts does the primary report contain" are orthogonal concerns coupled through one mutable report variable; splitting PrepareHistoryReport's output into a value scoped only to SaveHistoryAsync (rather than reassigning the variable also used for WriteReportAsync/the return) removes the coupling directly rather than requiring careful sequencing to avoid it.

This still needs to be resolved before merge — both issues were flagged in the prior two rounds and remain open in this commit.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 68472ccdd9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Addressed alternating-stream masked suffix rescans in 377616c using one mapped mask pass per stream. Validation: ModuleOutputExcerptBufferTests 21/21; SecretMaskingPatternTests 27/27; ModularPipelines.slnx Release build passed with 0 warnings and 0 errors.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 377616ca6b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review

Re-reviewed at the latest state (377616ca6b), the "map masked suffix offsets" perf commit since the last review round closed at 68472ccdd9.

What changed: RebalanceMaskedStreamByteLimits no longer re-obfuscates a growing suffix string on every chunk boundary. It now calls a new SecretObfuscator.ObfuscatePreservingMasksWithSourceMap once per stream, which masks the full text in a single pass and simultaneously builds a SourceToOutputByteOffsets map (UTF-16 index → output UTF-8 byte offset, snapped to code-point/mask-span boundaries). Suffix byte counts are then O(1) lookups (GetSuffixByteCount) instead of O(n) re-masks, replacing the O(n²) behavior the previous round's rewrite (coalesced chunks) had only partially fixed.

I traced the offset math by hand for the boundary case that matters most here — a truncation point falling inside a secret span: AppendUnchangedWithSourceMap's masked-match branch writes outputByteCount (the offset before the mask is emitted) to every source index in [matchIndex, matchEnd), so any suffix start that lands mid-secret reports the same byte count as one starting at the secret's beginning — i.e. it always retains the whole mask rather than splitting it. That's the same "never cut a mask" invariant TryGetSafeMaskedTail's maximumMatchBytes check exists to protect, and it's actually stricter than the old per-suffix-independent-masking approach, which could fail to recognize a secret straddling a suffix boundary at all (no match = no re-mask = raw bytes retained). Ran the directly relevant suites against this exact head: ModuleOutputExcerptBufferTests 21/21, SecretMaskingPatternTests + SecretObfuscatorTests 27/27 combined, including the existing boundary-specific cases (OmitsExcerptWhenSecretExceedsSafeBoundaryContext, ReallocatesUtf8BoundaryWasteToOlderOtherStream, OmitsExcerptWhenCaseInsensitiveMatchCanExceedBoundaryContext). All pass. This is a correct, low-risk change in isolation.

Both previously-flagged blockers are still open — neither is touched by this commit

🔴 TruncatedBytes still diverges from the actual masked tail split. CreateExcerpt still computes truncatedBytes (line 78) from GetFinalStreamByteLimits — a split over the raw, unmasked text — before TryCreateTails → TryCreateMaskedTails independently recomputes stdoutBytes/stderrBytes over the masked text via RebalanceMaskedStreamByteLimits (line 180). That recomputation reassigns local parameters inside TryCreateMaskedTails; it never flows back to the truncatedBytes already captured in CreateExcerpt. The repro from the last two rounds (10-byte budget, 1-char secret, "aaaa" on stdout then "a" on stderr → StdoutTail comes back null while TruncatedBytes reports 0) still applies — nothing in this commit's diff touches CreateExcerpt, GetFinalStreamByteLimits, or GetTruncatedByteCount.

🟠 PrepareHistoryReport still strips excerpts from the primary report file, not just the history store. CompleteAsync reassigns the single shared report variable through PrepareHistoryReport (line 82) before both WriteReportAsync and the method's return. RunReportService.cs isn't among the files this commit touches, so the coupling is unchanged: when historyEnabled is true and a non-FileSystemRunHistoryStore is configured, this still strips excerpts from the report written to reportPath and handed back to the caller — not just the copy passed to historyStore.SaveAsync.

Same architectural note as the last two rounds, now reinforced by this commit

This commit is the second full rewrite of RebalanceMaskedStreamByteLimits in this PR (first for coalesced chunks at 68472ccd, now for source-mapped offsets). Both rewrites were scoped tightly to the masked-tail computation and correctly left GetFinalStreamByteLimits/GetTruncatedByteCount (the raw-text formula) alone — which is exactly why they couldn't have fixed the TruncatedBytes divergence above. ModuleOutputExcerptBuffer now has three independent formulas for "how many bytes did we keep" (raw split, masked split, truncated-byte diff), and two rounds of otherwise-correct performance work have proven none of the three refactors so far reaches across that boundary. The fix isn't a third rewrite of the masked-split formula — it's collapsing to one source of truth: have TryCreateMaskedTails return the masked (stdoutBytes, stderrBytes) it computes back to CreateExcerpt (or track bytes-retained as explicit state updated once, at the point truncation actually happens), and derive TruncatedBytes from that same value instead of a second, independent raw-text computation. RunReportService has the analogous fix: give PrepareHistoryReport's output a variable scoped only to SaveHistoryAsync, rather than reassigning the variable also used for WriteReportAsync/the return, so "which history backend is configured" can't leak into "what excerpts does the primary report contain."

This still needs to be resolved before merge — both issues were flagged in the prior two rounds and remain open in this commit.

[thomhurst]

Addressed persistence-boundary excerpt validation in 5410a73. Both report and built-in history writes now fail closed after the temp-file sink. Validation: RunReportTests 93/93 passed; ModularPipelines.slnx Release build passed with 0 warnings and 0 errors.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5410a731a4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Addressed the publication-boundary race in c3168ac. Secret registration and the atomic move now share the same lock; a failed version guard rewrites and publishes excerpt-free JSON. Focused tests 94/94; core Release build clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3168acdfa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Preserved Write vs WriteLine semantics end-to-end in 86e18dd, including excerpt byte accounting. Three affected suites pass (22/22, 19/19, 38/38); core Release build clean.

[claude]

Review: PR #3842 — Add size-capped module output to run reports

I traced the actual PR diff (gh pr diff 3842) file-by-file and cross-checked every candidate finding against it plus the existing test suite before writing this up — a first pass surfaced a long list of "regressions" in DependencyWaiter.cs, WorkerCancellationClassifier.cs, ModuleRunner.cs, BuildSystemLogIssueLoggerProvider.cs, and a Kustomize scraper change, but none of those files are touched by this PR (confirmed via gh pr diff --name-only, 30 files total, all under output-excerpt/run-report/secret-masking). Those were discarded as noise from comparing against the wrong ref. I also confirmed CustomHistoryStoreOmitsOutputExcerptsFromAllReports in RunReportTests.cs explicitly asserts that a custom IRunHistoryStore strips Output from the primary run-report.json too — that's intentional, tested behavior, not a leak, despite an early automated pass flagging it as a bug.

Overall this is a well-tested feature (ModuleOutputExcerptBufferTests.cs alone has 22 cases covering UTF-8 boundaries, late-secret masking, and cross-stream rebalancing). The findings below are things I traced through the actual code in ModuleOutputExcerptBuffer.cs/SecretObfuscator.cs.

1. TruncatedBytes is computed from a stream split that doesn't match the returned tails, once masking triggers rebalancing

ModuleOutputExcerptBuffer.cs — CreateExcerpt() calls GetTruncatedByteCount(stdout, stderr, stdoutBytes, stderrBytes) using the pre-masking split from GetFinalStreamByteLimits (line ~84), before TryCreateTails/TryCreateMaskedTails runs. When secrets are registered and masking actually changes text length, TryCreateMaskedTails computes an entirely different split via RebalanceMaskedStreamByteLimits and that's what actually produces StdoutTail/StderrTail — but TruncatedBytes is never recomputed against it.

I traced a concrete case through the code by hand (2 streams, budget=10, a secret spanning a stdout append boundary that masks 7 raw bytes down to 1): the raw split used for TruncatedBytes gives (4, 6) while the actual masked/rebalanced split used for the tails is (2, 8). The resulting TruncatedBytes value doesn't correspond to either "bytes discarded from the retention buffer" or "raw bytes not represented in the final excerpt" — it's answering neither question consistently. ReallocatesFreedMaskedBytesToNewerOtherStreamChunks (line 231) exercises exactly this scenario (masking + rebalancing across two streams) but doesn't assert on TruncatedBytes at all, so this gap isn't caught by the suite. Worth deciding what TruncatedBytes should mean once masking is in play (bytes dropped by the ring buffer vs. raw bytes not visible in the final report) and computing it from the same split that actually produced the tails.

2. Several "omit the whole excerpt" fail-closed guards are completely silent

ModuleOutputExcerptBuffer.TryCreateMaskedTails returns false (→ CreateExcerpt() returns null, so the module simply has no Output in the report) in three separate cases, none of which log anything:

• secretObfuscator is a custom ISecretObfuscator implementation rather than the internal SecretObfuscator type (line ~163: "Custom or incomplete masking dependencies do not expose enough information to prove that a bounded context is safe") — anyone who registers their own ISecretObfuscator via DI gets zero output excerpts, permanently, with no indication why.
• The longest registered secret's max match length exceeds the whole maximumBytes budget (line ~178) — one long secret (a cert, connection string) registered anywhere disables excerpts for every module.
• The masking-boundary safety check fails for either stream, which discards both streams' tails (line ~206: stdoutTail = stderrTail = null), even when only one stream had an unsafe boundary.

These are all deliberate, documented-in-comments fail-closed choices, not bugs — but contrast this with PrepareHistoryReport in RunReportService.cs, which logs a LogDebug explaining exactly why output was omitted for a custom history store. Applying that same pattern here (a debug/trace log on each of these three paths) would save someone real time the first time they wonder why RunReport.IncludeModuleOutput "isn't working."

3. Duplicated UTF-8 boundary-scanning and secret-scan logic

• ModuleOutputExcerptBuffer.cs has three separate hand-rolled implementations of "walk forward while the byte is a UTF-8 continuation byte" (GetUtf8TailByteCount, GetUtf8Tail, and the inline loop in TrimToLimit). GetUtf8Tail even re-derives the byte array and re-implements GetUtf8TailByteCount's scan rather than calling it. Given this file's commit history is mostly UTF-8/masking-boundary bugfixes, three copies of the same boundary logic is exactly the kind of thing that gets fixed in one place and not the others.
• SecretObfuscator.ObfuscateMatches and ObfuscateMatchesWithSourceMap run the same match/lookup/IsContainedInExistingMask loop, differing only in whether they also build a source-offset map. Worth having ObfuscateMatches call the source-map variant and discard the map, or extracting the shared scan into one method.

4. Output gets fully re-masked on every report generation, even though it's already masked once at write time

CoordinatedTextWriter.WriteCompletedLine/FlushPartialPrefix already call _secretObfuscator.Obfuscate(...) before routing each line into the module's buffer. ModuleOutputExcerptBuffer.TryCreateMaskedTails then runs ObfuscatePreservingMasksWithSourceMap (a full SearchValues scan plus the GetMaskRanges/IndexOf pass to detect already-masked spans) over the entire retained tail again, on every CreateExcerpt() call — and RebalanceMaskedStreamByteLimits separately re-decodes every retained chunk via Utf8.GetString(chunk.Bytes).Length just to get a char count, duplicating the decode already done a few lines earlier in CreateExcerpt's own loop. For pipelines with a lot of output/many modules this is a real, avoidable cost on every report write (not just once at pipeline end) — worth caching the decoded strings/lengths or skipping the re-mask pass when nothing changed since the buffer already only contains post-obfuscation text.

Minor

SecretProvider.TryExecuteIfVersionCurrent (used by AtomicFileWriter's publish-retry path to move the finished report file into place) runs the supplied action — a synchronous File.Move — while holding _secretsLock, the same lock AddSecret/AddSecrets take. Probably fine in practice since File.Move is fast, but it does mean report-file publication and secret registration share a lock, which is worth a one-line comment so it isn't accidentally widened later (e.g. if action ever grows to do more I/O).

Nice touches: the GetMaximumMatchByteCount case-insensitive UTF-8 upper-bound calculation and the TryGetSafeMaskedTail cross-boundary-match guard are solid, careful work — this is a genuinely hard "don't let a secret leak across a truncation boundary" problem and the core algorithm handles it correctly in the cases I traced.

[thomhurst]

Addressed the latest review at exact head 77dffa3. TruncatedBytes now derives from source bytes represented by the final rebalanced masked tails; fail-closed excerpt omissions emit debug diagnostics; UTF-8 boundary and secret-match scans are centralized; decoded chunk text is cached; the secret publication lock contract is documented. Validation: ModuleOutputExcerptBufferTests 22/22, SecretObfuscatorTests 3/3, ModuleOutputBufferTests 38/38, ModularPipelines.slnx Release build 0 warnings/errors. @codex review exact head 77dffa3. @claude review exact head 77dffa3.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 77dffa312a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce54ec037e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Allow untrimmed output when a secret exceeds the cap

When any registered secret is larger than MaxOutputBytesPerModule, this unconditional check suppresses every module excerpt, even if that module emitted less than the cap and its complete output is still retained. For example, with an 8-byte cap, a registered 9-byte secret, and output ok\n, there is no discarded boundary and the whole value can be safely scanned, yet CreateExcerpt returns null. Apply this fail-closed condition only to streams whose raw prefix was actually trimmed so unrelated short diagnostics remain available.

Useful? React with 👍 / 👎.

[claude]

Code review

Re-reviewed at the latest state (ce54ec037e0a), which adds one commit since the last reviewed commit (377616ca6b95): 5410a731a4 (validate through persistence), c3168acdfa (guard report publication), 86e18dd5f8 (preserve output line endings), 77dffa312a (align excerpt truncation with masked tails — addresses all four findings + the minor note from the previous review round, confirmed by Codex with no issues), and ce54ec037e (avoid disposed logger factory during output capture).

Verified the new commit (ce54ec037e):

• ConsoleCoordinator previously called _loggerFactory.CreateLogger<ModuleOutputExcerptBuffer>() lazily inside GetModuleBuffer's GetOrAdd factory delegate. LoggerFactory.CreateLogger throws ObjectDisposedException once the factory is disposed, so any call to GetModuleBuffer after the DI-owned ILoggerFactory is torn down (e.g. output still being captured/flushed during host shutdown) would crash.
• The fix hoists the CreateLogger call into the constructor (_outputExcerptLogger), where the factory is guaranteed live, and reuses that cached instance in GetModuleBuffer. Since the logger category (ModuleOutputExcerptBuffer) doesn't vary per module type, caching it once is behaviorally equivalent to the old per-call creation — no regression there.
• GetModuleBuffer_DoesNotUseDisposedLoggerFactory genuinely reproduces the crash: I confirmed against the pre-fix code (77dffa312a) that it still calls _loggerFactory.CreateLogger<ModuleOutputExcerptBuffer>() inside GetModuleBuffer, so this test would have failed (ObjectDisposedException) before this commit and passes after it.
• Scope is tight and consistent with the rest of the PR — other pre-existing lazy _loggerFactory.CreateLogger(...) call sites in the same file (lines ~145, ~385, ~659) are untouched by this PR and out of scope.

No issues found in this delta. All four substantive findings and the minor lock-contract note from the previous full review round were addressed in 77dffa312a and independently confirmed clean by Codex; this round's only change is the well-tested disposed-factory fix above.