Reduce CoordinatedTextWriter secret-scan contention - #3856
Conversation
GitHub Actions
Failure
Code reviewReviewed Verified correct (worth calling out since this touches secret redaction):
Architectural suggestion (not a bug, but worth considering as a follow-up):
Since this PR is already reworking No CLAUDE.md compliance issues — the changes stay within the core library ( |
|
CI diagnosis: the sole Ubuntu failure was an unrelated timing timeout in |
GitHub Actions
Failure
|
Refreshed onto current main at 1127d84. Current-head validation: SecretMaskingPatternTests 29/29; OutputCoordinatorTests 19/19; core Release build 0 warnings / 0 errors. Please re-review current head. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Re-reviewed current head (1127d849f, after the rebase onto main) — thanks for the ping to re-review. Confirmed the concurrency rework itself (per-buffer SyncRoot, _outputLock serializing real-console writes, lock ordering SyncRoot → _lineBufferLock/_outputLock never reversed) is sound and deadlock-free, and the new SearchValues<string>-based single-pass matching preserves the original leftmost-longest-match semantics.
Found one high-severity issue (posted inline): removing the _secretObfuscator.Obfuscate(...) calls from FlushDirectPrefix/WriteCompletedLine/FlushPartialPrefix drops a safety net that mattered. ObfuscateCompletePatterns's early break (when the longest match at the leftmost position overlaps the retained-prefix boundary) can flush a buffer region verbatim even though it contains a different, shorter, fully-contained registered secret earlier in that same region. I verified this with a concrete repro and a standalone reimplementation of the exact algorithm — confirmed the leak occurs post-PR and that the removed second Obfuscate() pass would have caught it pre-PR. Details and a suggested fix direction are in the inline comment.
My previous review on this PR (before the rebase) concluded the removed calls were "genuinely redundant" — that conclusion was wrong; this scenario slipped through because it depends on interaction between two separately-registered secrets near the retained-prefix boundary, which the earlier pass didn't construct a counterexample for.
Everything else from the previous review still stands: the architectural suggestion to extract a shared SecretPatternCache-style type (mirroring SecretObfuscator.GetRegisteredSecretCache) remains a reasonable non-blocking follow-up, not required for this PR.
|
@claude review |
|
@codex review |
Code reviewRe-reviewed at current head ( Verified correct — the nested-secret fix ( The old behavior: once a match overlapping the retained-prefix boundary was found, the loop appended everything from I also verified the invariant the fix depends on: when Also checked (unchanged from previous review, still holds at this head):
Still-open, non-blocking suggestion from the previous review: No CLAUDE.md compliance issues — both changed files stay within the core library ( |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef575ed76f
ℹ️ 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".
GitHub Actions
Failure
ef575ed to
07f7f70
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesCoordinated output processing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConsoleWrite
participant CoordinatedTextWriter
participant SecretProvider
participant SecretObfuscator
participant RealConsole
ConsoleWrite->>CoordinatedTextWriter: Write output
CoordinatedTextWriter->>SecretProvider: ExecuteWithStableSecrets
CoordinatedTextWriter->>SecretObfuscator: ObfuscateWithConsumption
SecretObfuscator-->>CoordinatedTextWriter: Return masked output and consumed length
CoordinatedTextWriter->>RealConsole: Flush synchronized output
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
When a longer same-start secret overlaps a retained suffix, scan for the longest same-start match that ends in the safe output before skipping the position. This prevents shorter registered secrets from reaching the console unmasked.\n\nRefs #3856
When a longer same-start secret overlaps a retained suffix, scan for the longest same-start match that ends in the safe output before skipping the position. This prevents shorter registered secrets from reaching the console unmasked. Refs #3856
3ff7944 to
b44c0c5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b44c0c57f0
ℹ️ 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".
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b93b2fd3c
ℹ️ 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".
|
Follow-up P1 fixed in f64f281: partial candidate masking now preserves the unchanged suffix and resumes overlap scanning instead of consuming the full case-insensitive candidate. Validation: SecretMaskingPatternTests 33/33; OutputCoordinatorTests 19/19; core Release build 0 warnings/errors; scoped formatting clean. |
There was a problem hiding this comment.
Code review
Re-reviewed at current head (f64f281d09, after the "rescan partial secret matches" fix — the fourth P1 fix on top of what I reviewed previously). I traced ObfuscateCompletePatterns end-to-end against all four adversarial repros raised across this thread (nested retained-prefix secret, shorter same-start secret, case-sensitivity mismatch, partially-obfuscated candidate) plus tried to construct new counterexamples of my own.
Correctness at current head: I could not find a new leak. The key invariant that makes this hold together, even with the GetUnchangedSuffixLength/consumedLength bookkeeping added in the last fix, is that everything appended to output is one of exactly two things: (a) raw pending text that a FindFirstPattern scan has already proven contains no match start, or (b) bytes taken directly from _secretObfuscator.Obfuscate(secret, null)'s own return value, which by definition has already had every registered secret inside it masked. Any content that gets "deferred" back onto searchIndex/outputIndex is always raw, unprocessed pending text, which the loop is guaranteed to re-scan with the same pattern set before it can reach output unmasked. So even where the suffix-trim heuristic in GetUnchangedSuffixLength under- or over-estimates the truly "untouched" span (it's a character-equality heuristic, not a real record of which spans Obfuscate touched), the failure mode is redundant re-scanning of already-safe text, not a leak — I could not construct an input where it causes unmasked secret bytes to reach output directly. consumedLength also can't go negative or reach 0 while looping the same position (which would hang), since unchangedSuffixLength is bounded by min(secret.Length, obfuscated.Length) and obfuscated != secret is already guaranteed at that point, so at least one byte of the match is always consumed. Lock ordering, the new SecretPatterns/SearchValues cache, and FindFirstPattern's leftmost/longest tie-break all still hold as verified in earlier passes.
Process/architecture concern (this is the part worth acting on):
This file has now needed four independent P1 fixes in six days, each one patching the exact adversarial input the previous version failed on (nested secret → same-start shorter secret → case-sensitivity → partial-obfuscation resume). That pattern — react to a counterexample, add a regression test for precisely that input — is a strong signal the invariant ObfuscateCompletePatterns is trying to maintain (single forward pass, mutate the buffer in place, retain a variable-length unsafe suffix, resume mid-candidate on partial matches) is fundamentally harder to prove correct than the incremental patches suggest. My read above is informal reasoning, not a proof, and this is the exact code path standing between a leaked cloud credential and a CI log. Two concrete suggestions, either of which would meaningfully raise confidence beyond "four humans/bots didn't find a fifth counterexample":
- Add a randomized/property-based test that generates many registered-secret sets (including overlapping, nested, and case-variant secrets) and many arbitrary
Writesplit points, then asserts the resulting_realConsoleoutput is byte-identical to a naive reference — buffer the entire stream and call_secretObfuscator.Obfuscateonce on the whole thing. That's the only way to get coverage beyond the specific examples reviewers happened to think of, and it directly targets the class of bug this file keeps producing. - Restore a cheap safety-net obfuscation pass, but scoped to just the small chunk being flushed (not the whole retained buffer, which is what made the original blanket
Obfuscate()calls expensive).FlushDirectPrefix/WriteCompletedLine/FlushPartialPrefixcurrently trustObfuscateCompletePatterns's bookkeeping unconditionally. A final_secretObfuscator.Obfuscate()call on just the bytes about to hit_realConsole(typically a single line or a short prefix, not the whole buffer) would cost little and turns any future bug in the streaming/retained-prefix logic into a redundant no-op instead of a live credential leak — i.e. defense in depth on the one path where "we optimized this and got it wrong" has already happened four times.
Neither is a blocker for this specific PR — the current head does appear correct against every repro raised so far — but given the security stakes and the repeated-P1 track record, I'd treat this as a near-term follow-up rather than a someday-maybe.
Still-open, non-blocking suggestion from earlier reviews: CoordinatedTextWriter.GetSecretPatterns() still duplicates the pattern-cache construction logic in SecretObfuscator.GetRegisteredSecretCache() (same version-check, same distinct/sort/SearchValues.Create construction, same ISecretProvider source, differing only in hardcoded OrdinalIgnoreCase vs. honoring SecretMaskingOptions.CaseInsensitive). Worth extracting into a shared SecretPatternCache-style type parameterized by case-sensitivity so the two obfuscation passes can't silently diverge on matching semantics again.
Minor, non-blocking: FlushAvailableAsync/FlushAsync now call the synchronous _realConsole.Flush() under _outputLock instead of the previous _realConsole.FlushAsync(). That's a reasonable trade-off given a plain lock can't be held across an await, but it does mean a _realConsole implementation with genuinely async I/O (e.g. network-backed) would now block a thread-pool thread during flush. If that matters here, a SemaphoreSlim instead of the object lock would let flush stay truly async.
No CLAUDE.md compliance issues — both changed files stay within the core library (ModularPipelines.slnx/ModularPipelines.Tests.slnf scope) and don't touch build/test tooling or generated options classes.
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f64f281d09
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1befd589af
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbbc227729
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a12ad75fa5
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/ModularPipelines/Engine/ISecretObfuscator.cs (1)
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the consumption contract.
ConsumedInputLengthdrives buffer advancement inCoordinatedTextWriter.ObfuscateCompletePatterns. Add XML documentation that states the invariant:ConsumedInputLengthcounts characters ofinputthatOutputreplaces, and the remaininginput.Length - ConsumedInputLengthcharacters appear unchanged at the end ofOutput. This invariant is currently only implied by the caller arithmetic at lines 308-311 ofsrc/ModularPipelines/Console/CoordinatedTextWriter.cs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ModularPipelines/Engine/ISecretObfuscator.cs` around lines 19 - 22, Document SecretObfuscationResult and its ConsumedInputLength member with XML comments describing the consumption invariant: it counts input characters replaced by Output, while the remaining input.Length - ConsumedInputLength characters appear unchanged at the end of Output.test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs (1)
211-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the first module output as well.
The test verifies only
secondBuffer. It does not confirm that the blocked write produced the masked line. Add a verification forfirstBufferafterawait firstWrite. This confirms that per-state locking preserves correctness, not only progress.💚 Proposed addition
secondBuffer.Verify(x => x.WriteLine("ordinary output"), Times.Once); + firstBuffer.Verify(x => x.WriteLine("**********"), Times.Once);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs` around lines 211 - 278, Update DifferentModuleBuffers_ProcessConcurrently to verify firstBuffer after await firstWrite, asserting it received the masked "**********" line exactly once, while retaining the existing secondBuffer verification.src/ModularPipelines/Console/CoordinatedTextWriter.cs (2)
209-236: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a dedicated lock for the secret-pattern cache.
GetSecretPatternsacquires_lineBufferLock, which also guards the_lineBuffersdictionary.ProcessPendingOutputcallsGetSecretPatternson every processed write (Line 176). Every write from every module therefore serializes on one global lock, and each write also blocks concurrentGetLineBufferStatecalls. This works against the stated goal of removing the global writer lock.Two lock orders also exist in this class.
ProcessPendingOutputtakesstate.SyncRootand then_lineBufferLock.Flushtakes_lineBufferLock, releases it, and then takesstate.SyncRoot. The current code does not deadlock because the second order is not nested. A future change that holds_lineBufferLockacross astate.SyncRootacquisition would deadlock.Separate the two concerns and add a volatile fast path, as
SecretObfuscator.GetRegisteredSecretCachealready does.♻️ Proposed refactor
private readonly object _lineBufferLock = new(); + private readonly object _secretPatternsLock = new(); private readonly object _outputLock = new(); private SecretPatterns _secretPatterns = new([], null); private long? _secretPatternsVersion;private SecretPatterns GetSecretPatterns() { - lock (_lineBufferLock) - { - var version = _secretProvider.Version; - if (_secretPatternsVersion is not null - && (version & 1) == 0 - && _secretPatternsVersion == version - && _secretProvider.Version == version) - { - return _secretPatterns; - } + var version = _secretProvider.Version; + var cachedVersion = Volatile.Read(ref _secretPatternsVersion); + if (cachedVersion is not null + && (version & 1) == 0 + && cachedVersion == version + && _secretProvider.Version == version) + { + return Volatile.Read(ref _secretPatterns); + } + lock (_secretPatternsLock) + { var snapshot = _secretProvider.GetSnapshot(); + if (_secretPatternsVersion == snapshot.Version) + { + return _secretPatterns; + } + var values = (snapshot.Secrets ?? []) .Where(pattern => !string.IsNullOrEmpty(pattern)) .Distinct(StringComparer.OrdinalIgnoreCase) .OrderByDescending(pattern => pattern.Length) .ToArray(); - _secretPatterns = new SecretPatterns( + Volatile.Write(ref _secretPatterns, new SecretPatterns( values, values.Length == 0 ? null - : SearchValues.Create(values, StringComparison.OrdinalIgnoreCase)); - _secretPatternsVersion = snapshot.Version; + : SearchValues.Create(values, StringComparison.OrdinalIgnoreCase))); + Volatile.Write(ref _secretPatternsVersion, snapshot.Version); return _secretPatterns; } }
_secretPatternsis arecord struct, soVolatile.Read/Volatile.Writedo not apply to it directly. If you adopt the fast path, changeSecretPatternsto a sealed record class, or keep the field access inside the lock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ModularPipelines/Console/CoordinatedTextWriter.cs` around lines 209 - 236, Update GetSecretPatterns to use a dedicated secret-pattern cache lock instead of _lineBufferLock, keeping _lineBufferLock exclusively for line-buffer state. Add a volatile fast path for a stable, even _secretProvider.Version, and publish the rebuilt SecretPatterns and version safely; since SecretPatterns is currently a record struct, convert it to a sealed record class or otherwise retain all accesses under the dedicated lock.
254-258: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the buffer copy after the candidate gate.
state.Buffer.ToString()allocates a full copy of the pending buffer. The code allocates it before theIndexOfAnygate on Line 255. Output that contains no secret candidate therefore pays a full string allocation on every processed write.ProcessPendingOutputruns on each newline, so this is a hot path.Scan the
StringBuilderchunks first and allocate the string only when a candidate exists.♻️ Proposed refactor
- var pending = state.Buffer.ToString(); - if (pending.AsSpan().IndexOfAny(patterns.SearchValues) < 0) + if (!ContainsPatternCandidate(state.Buffer, patterns)) { return retainedPrefixLength; } + + var pending = state.Buffer.ToString();Add the helper. A candidate can straddle a chunk boundary, so overlap the scan window by
patterns.Values[0].Length - 1characters, or fall back toToString()when the buffer holds more than one chunk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ModularPipelines/Console/CoordinatedTextWriter.cs` around lines 254 - 258, Update ProcessPendingOutput to avoid calling state.Buffer.ToString() before the candidate gate. Scan StringBuilder chunks first, preserving detection across chunk boundaries by overlapping adjacent scan windows by patterns.Values[0].Length - 1 characters, or fall back to ToString() for multi-chunk buffers; only create the pending string after a candidate is found, then retain the existing IndexOfAny and retainedPrefixLength behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ModularPipelines/Console/CoordinatedTextWriter.cs`:
- Around line 536-572: Update the shutdown lifecycle around
CoordinatedTextWriter.Flush and ConsoleCoordinator.DisposeAsync so active
writers are stopped or awaited before Uninstall restores the console streams.
Ensure no concurrent write can create or append to a line-buffer state after
Flush snapshots and drains _lineBuffers; otherwise, explicitly enforce and
document a lifecycle contract requiring all Console.Out users to finish before
disposal.
---
Nitpick comments:
In `@src/ModularPipelines/Console/CoordinatedTextWriter.cs`:
- Around line 209-236: Update GetSecretPatterns to use a dedicated
secret-pattern cache lock instead of _lineBufferLock, keeping _lineBufferLock
exclusively for line-buffer state. Add a volatile fast path for a stable, even
_secretProvider.Version, and publish the rebuilt SecretPatterns and version
safely; since SecretPatterns is currently a record struct, convert it to a
sealed record class or otherwise retain all accesses under the dedicated lock.
- Around line 254-258: Update ProcessPendingOutput to avoid calling
state.Buffer.ToString() before the candidate gate. Scan StringBuilder chunks
first, preserving detection across chunk boundaries by overlapping adjacent scan
windows by patterns.Values[0].Length - 1 characters, or fall back to ToString()
for multi-chunk buffers; only create the pending string after a candidate is
found, then retain the existing IndexOfAny and retainedPrefixLength behavior.
In `@src/ModularPipelines/Engine/ISecretObfuscator.cs`:
- Around line 19-22: Document SecretObfuscationResult and its
ConsumedInputLength member with XML comments describing the consumption
invariant: it counts input characters replaced by Output, while the remaining
input.Length - ConsumedInputLength characters appear unchanged at the end of
Output.
In `@test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs`:
- Around line 211-278: Update DifferentModuleBuffers_ProcessConcurrently to
verify firstBuffer after await firstWrite, asserting it received the masked
"**********" line exactly once, while retaining the existing secondBuffer
verification.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd8eaf9d-8272-47bf-96c9-b53694603136
📒 Files selected for processing (4)
src/ModularPipelines/Console/CoordinatedTextWriter.cssrc/ModularPipelines/Engine/ISecretObfuscator.cssrc/ModularPipelines/Engine/SecretObfuscator.cstest/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs
|
Scoped custom-obfuscator reentrancy to each CoordinatedTextWriter in 0097a7c. Cross-stream sync/async flush regressions added; 76/76 focused tests and clean core Release build. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0097a7cc43
ℹ️ 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".
|
Fixed in c0b26cf. Reentrant sink writes now enter their own active-writer scope, and line-buffer keys include the per-writer reentrancy depth. Added a two-level nested sink regression; SecretMaskingPatternTests pass 77/77 and the core Release build is clean. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/ModularPipelines/Engine/SecretProvider.cs (2)
789-798: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the lock around the read of the readonly
_snapshotfield.
_snapshotisreadonlyand is assigned only in the constructor. Thelock (_syncRoot)in theSnapshotgetter adds contention without any benefit, and this getter runs on everyVersionandGetSnapshotcall.♻️ Proposed simplification
- public SecretSnapshot Snapshot - { - get - { - lock (_syncRoot) - { - return _snapshot; - } - } - } + public SecretSnapshot Snapshot => _snapshot;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ModularPipelines/Engine/SecretProvider.cs` around lines 789 - 798, Update the SecretSnapshot property getter to return the readonly _snapshot field directly without acquiring _syncRoot; preserve the existing property and return value while removing only the unnecessary lock.
288-310: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe
CurrentUnscopedRegistrationContextchain grows without bound.
TryDeferUnscopedRegistrationpushes a newUnscopedRegistrationContextonto the async-local chain on every deferred registration, and no code removes entries. A long-lived worker that registers many secrets while emissions are active keeps a chain that grows linearly.GetUnscopedRegistrationBatchthen walks that chain on everyExecuteWithStableSecretscall, so cost grows per registration.Only the newest batch for this provider is needed. Replace the entry for the current provider instead of pushing a new one, or store the batch in a single mutable holder per provider.
Also applies to: 341-354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ModularPipelines/Engine/SecretProvider.cs` around lines 288 - 310, Update TryDeferUnscopedRegistration and the related context handling used by GetUnscopedRegistrationBatch so repeated deferred registrations for this provider replace the existing CurrentUnscopedRegistrationContext entry rather than append another UnscopedRegistrationContext. Preserve the parent context for other providers and ensure lookups continue returning only the newest deferred batch for this provider.test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs (2)
2335-2354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this test method above the private helper classes.
DirectConsoleWrite_ReentrantFlushDrainsPartialPrefixsits between the helper writer declarationsAsyncFlushTrackingWriterandBlockingAsyncFlushStringWriter. Every other test in the file appears before the helper section. Move this method next toFlushAsync_ReentrantFlushDrainsPartialPrefixto keep tests and helpers separated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs` around lines 2335 - 2354, Move the test method DirectConsoleWrite_ReentrantFlushDrainsPartialPrefix above the private helper class declarations, placing it next to FlushAsync_ReentrantFlushDrainsPartialPrefix. Keep the test implementation unchanged and leave the helper classes together below the test methods.
1210-1227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated release-and-await block.
The
tryblock already callsreleaseObfuscation.Set()and awaitsTask.WhenAll(write, registration)at Lines 1220-1221. Thefinallyblock repeats both statements. The repetition is only needed for the failure path, so keep it infinallyand drop it fromtry. The same pattern appears inDirectConsoleWrite_DefersRegistrationUntilWriteCompletes(Lines 1266-1272) andStableSecretEmission_HoldsLeaseForInheritedWorker(Lines 1853-1859).♻️ Proposed simplification
await Assert.That(provider.Secrets).DoesNotContain("dynamic-secret"); - - releaseObfuscation.Set(); - await Task.WhenAll(write, registration).WaitAsync(TimeSpan.FromSeconds(5)); } finally { releaseObfuscation.Set(); await Task.WhenAll(write, registration).WaitAsync(TimeSpan.FromSeconds(5)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs` around lines 1210 - 1227, Remove the redundant releaseObfuscation.Set() and Task.WhenAll(write, registration).WaitAsync(...) calls from the try block, leaving them only in finally for failure cleanup. Apply the same cleanup to DirectConsoleWrite_DefersRegistrationUntilWriteCompletes and StableSecretEmission_HoldsLeaseForInheritedWorker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ModularPipelines/Engine/SecretProvider.cs`:
- Around line 227-286: Restructure EnterStableEmission and ExitStableEmission so
_secretEmissionLock.EnterReadLock and ExitReadLock occur outside the
_emissionStateLock critical sections, while preserving the existing state
updates and wait conditions. Move PublishPatterns out of the _emissionStateLock
region as well, coordinating deferred-pattern extraction and active-emission
bookkeeping before publishing, then clear _deferredMaskingSnapshot at the
appropriate post-publication point and verify published-snapshot visibility
behavior.
- Around line 219-224: Update the deferred snapshot creation in the masking
snapshot flow, including UpdateDeferredMaskingSnapshot and
GetNestedMaskingSnapshot, so its versioning cannot produce odd values
interpreted as in-progress publication states. Use a separate deferred-state
indicator or ensure generated deferred versions remain even, while preserving
existing snapshot reuse when secrets.Count equals existingCount.
---
Nitpick comments:
In `@src/ModularPipelines/Engine/SecretProvider.cs`:
- Around line 789-798: Update the SecretSnapshot property getter to return the
readonly _snapshot field directly without acquiring _syncRoot; preserve the
existing property and return value while removing only the unnecessary lock.
- Around line 288-310: Update TryDeferUnscopedRegistration and the related
context handling used by GetUnscopedRegistrationBatch so repeated deferred
registrations for this provider replace the existing
CurrentUnscopedRegistrationContext entry rather than append another
UnscopedRegistrationContext. Preserve the parent context for other providers and
ensure lookups continue returning only the newest deferred batch for this
provider.
In `@test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs`:
- Around line 2335-2354: Move the test method
DirectConsoleWrite_ReentrantFlushDrainsPartialPrefix above the private helper
class declarations, placing it next to
FlushAsync_ReentrantFlushDrainsPartialPrefix. Keep the test implementation
unchanged and leave the helper classes together below the test methods.
- Around line 1210-1227: Remove the redundant releaseObfuscation.Set() and
Task.WhenAll(write, registration).WaitAsync(...) calls from the try block,
leaving them only in finally for failure cleanup. Apply the same cleanup to
DirectConsoleWrite_DefersRegistrationUntilWriteCompletes and
StableSecretEmission_HoldsLeaseForInheritedWorker.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 891ce30a-bab9-4633-8019-a589e39b3400
📒 Files selected for processing (3)
src/ModularPipelines/Console/CoordinatedTextWriter.cssrc/ModularPipelines/Engine/SecretProvider.cstest/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ModularPipelines/Console/CoordinatedTextWriter.cs
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b26cfe23
ℹ️ 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".
|
Fixed all three new review findings in 928d458: even deferred versions, deadlock-free emission/publication lock ordering, and immediate scoped-registration visibility to direct redactors. Validation: SecretMaskingPatternTests 80/80, SecretObfuscatorCachingTests 14/14, FormattedLogValuesObfuscatorTests 11/11; core Release build 0 warnings/errors. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 928d458473
ℹ️ 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".
|
Fixed in 8bd802e. Unscoped registration state now immutably upserts the current provider entry and updates its batch instead of prepending a node per secret; execution-context isolation is preserved and chain depth is bounded by distinct providers. A 100-registration regression verifies depth remains one. SecretMaskingPatternTests pass 81/81; core Release build is clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bd802e24a
ℹ️ 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".
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48f36f9435
ℹ️ 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".
|
Addressed both latest P2s and integrated concurrent lease work at exact head b374449. Output and custom-obfuscation contexts now use expiring ownership scopes: active leases alone permit reentrant behavior, while weakly held scope-keyed buffers preserve exact causal identity across expiry without unbounded dictionary retention. Validation: SecretMaskingPatternTests 83/83; ModularPipelines.slnx Release build 0 warnings/errors. @codex review exact head b374449. @claude review exact head b374449. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3744497a0
ℹ️ 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".
A late CaseInsensitive change could bypass final obfuscation and emit a differently cased secret. Carry the scanned pattern snapshot through emission and reobfuscate when its comparison no longer matches.
ConditionalWeakTable keys may expire after fire-and-forget work while their line buffers still contain accepted output. Retain non-empty scoped states until flush drains them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e6af1fe65
ℹ️ 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".
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c2f357f50
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| _outputLock.Wait(); |
There was a problem hiding this comment.
When _realConsole.Write or WriteLine suppresses execution-context flow, starts a worker that writes back through this coordinated writer, and synchronously waits for it, the worker cannot see ActiveOutputWriterScope; because _isAsyncFlushActive is only enabled for FlushRealConsoleAsync, TryDeferOutputWrite also declines it and the worker blocks here on the semaphore still owned by the outer sink callback. The sink then waits for that worker permanently, so the unflowed-reentrancy mechanism needs to cover ordinary sink writes as well as asynchronous flush callbacks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fccadd4. Sink-active deferral now begins for ordinary Write/WriteLine calls as well as flushes, before a callback can block on the shared line-buffer lock. Raw callback writes retain their line-buffer state and are drained only after the outer public writer operation releases that buffer, so they still go through normal secret masking without recursively mutating the active buffer.
Added DirectConsoleWrite_AllowsReentrantSinkWriteWithoutExecutionContextFlow, which suppresses ExecutionContext, synchronously waits for the callback, and verifies its diagnostic is masked. Validation: SecretMaskingPatternTests 87/87; Release ModularPipelines.csproj build 0 warnings / 0 errors.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
GitHub Actions
Inactive
Code review (head
|
1 participant
Summary
SearchValues<string>with each secret-pattern snapshot and use it for one-pass match discoveryValidation
SecretMaskingPatternTests: 25 passedOutputCoordinatorTests: 19 passedModularPipelines.slnxRelease build: 0 warnings, 0 errorsCloses #3755
Summary by CodeRabbit