Conversation
|
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:
📝 WalkthroughWalkthroughSmartPipe 2.1.2 adds ChangesJSON package and compatibility
Core runtime and reliability
Release automation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/getting-started.md (1)
93-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBullet list doesn't indicate which items belong to which package.
The new lead-in text explicitly splits ownership between
SmartPipe.Extensions.JsonandSmartPipe.Extensions, but the following selector/transform/sink bullets mix JSON and non-JSON components (JsonFileSource<T>next toCsvFileSource<T>, etc.) without labeling. This undercuts the point of the split for readers trying to figure out which package to reference.🤖 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 `@docs/getting-started.md` around lines 93 - 104, The component lists in the documentation do not identify package ownership despite the split lead-in. Update the selector, transform, and sink bullets to clearly label or group each item under SmartPipe.Extensions.Json versus SmartPipe.Extensions, preserving the existing component names and classifications.
🧹 Nitpick comments (15)
src/SmartPipe.Extensions.Json/JsonDocumentLimitStream.cs (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Flush()throws instead of no-op.Overriding
Flush()to throw on a read-only stream is a bit surprising (baseStream.Flush()is a no-op by default); if any consumer defensively callsFlush()on a read stream, this would blow up unnecessarily. Low risk given current call sites, but worth a defensive no-op instead.🤖 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/SmartPipe.Extensions.Json/JsonDocumentLimitStream.cs` at line 58, Update the Flush method in JsonDocumentLimitStream to be a no-op instead of throwing NotSupportedException, while leaving the stream’s other read-only behavior unchanged.src/SmartPipe.Extensions.Json/Sinks/JsonFileSink.cs (2)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
_leaveOpenreadonly.It's assigned only once, in the internal constructor.
🔧 Suggested fix
- private bool _leaveOpen; + private readonly bool _leaveOpen;Also applies to: 116-126
🤖 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/SmartPipe.Extensions.Json/Sinks/JsonFileSink.cs` at line 30, Update the _leaveOpen field in JsonFileSink to be readonly, preserving its single assignment in the internal constructor and any existing constructor behavior.Source: Linters/SAST tools
240-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNested
ifcan be merged with the enclosing one.Minor readability nit flagged by SonarCloud; doesn't affect behavior (traced the comma-insertion logic across batches and it is correct as written).
🤖 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/SmartPipe.Extensions.Json/Sinks/JsonFileSink.cs` around lines 240 - 252, In the batch-writing loop, merge the nested _options.Format == JsonFileFormat.Array condition with the enclosing conditional so comma insertion remains guarded by both the array format and (_arrayHasItems || output.Length > 1). Preserve the existing serialization and NDJSON newline behavior.Source: Linters/SAST tools
src/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cs (3)
67-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
"document"literal flagged by static analysis.SonarCloud suggests extracting the
"document"scope literal (used 4 times) into a constant.Also applies to: 120-127
🤖 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/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cs` around lines 67 - 79, Extract the repeated "document" scope value into a class-level constant in JsonFileSource, then replace all four usages in _deserializeItems and _deserializeBatches, including the corresponding code around _deserializeItemRecord and _deserializeBatchRecord, with that constant.Source: Linters/SAST tools
323-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
ReadFirstJsonByteAsyncbetween this file andDeadLetterSource.cs.Same helper is duplicated in
DeadLetterSource.cs(lines 270-295) with a small drift already (extrastream.Position = 0step here that the other copy omits). Extracting to a shared internal helper would prevent further divergence.🤖 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/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cs` around lines 323 - 348, Extract the duplicated ReadFirstJsonByteAsync logic from JsonFileSource and DeadLetterSource into a shared internal helper, then update both callers to use it. Preserve the existing BOM detection, whitespace skipping, cancellation behavior, and stream-position semantics consistently, including the initial reset before applying the BOM offset.
245-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCognitive complexity flagged by static analysis.
ReadFramedRecordsAsyncexceeds the configured complexity threshold (16 vs. 15). Worth a look when convenient, not blocking.🤖 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/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cs` around lines 245 - 289, Reduce the cognitive complexity of ReadFramedRecordsAsync below the configured threshold by extracting the oversized-record handling and JSON validation/deserialization error handling into focused helper methods. Preserve the existing record index tracking, HandleInvalidRecord behavior, null handling, and yielded-record behavior.Source: Linters/SAST tools
src/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cs (2)
111-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCognitive complexity flagged by static analysis.
SonarCloud flags
ReadEnvelopesAsyncfor exceeding the cognitive-complexity threshold (19 vs. 15 allowed) and recommends splitting parameter validation from the iterator body. Reasonable given it now branches across streaming/legacy and array/framed paths.🤖 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/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cs` around lines 111 - 176, Reduce the cognitive complexity of ReadEnvelopesAsync by extracting serializer/format validation and the separate streaming or legacy iteration paths into focused helper methods. Keep ReadEnvelopesAsync responsible for dispatching between those paths, and preserve the existing cancellation, empty-file, array/framed, and null OriginalPayload behaviors.Source: Linters/SAST tools
270-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
ReadFirstJsonByteAsyncbetween this file andJsonFileSource.cs.This ~25-line helper is duplicated nearly verbatim in
JsonFileSource.cs(lines 323-348), with a subtle difference (this version skips the extrastream.Position = 0reset before seeking tooffset, which is harmless but shows the copies have already started drifting). Consider extracting to a shared internal static helper in the project to avoid future divergence.🤖 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/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cs` around lines 270 - 295, Extract the duplicated ReadFirstJsonByteAsync logic from DeadLetterSource and JsonFileSource into a shared internal static helper, then update both callers to use it. Preserve the existing BOM handling, whitespace skipping, stream positioning, cancellation, and nullable-byte behavior while removing the duplicate implementations.src/SmartPipe.Extensions.Json/JsonRecordValidator.cs (1)
9-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a comment to the intentionally empty read loop to satisfy the SonarCloud "empty block" finding.
The empty
while (reader.Read()) { }loop is intentional — it fully exhausts the reader to surface any structural/trailing-content errors — but SonarCloud flags it as an empty block to remove or fill.♻️ Proposed fix
- while (reader.Read()) { } + while (reader.Read()) + { + // Intentionally empty: exhausting the reader validates the entire record, + // including detecting trailing content past the first top-level value. + }🤖 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/SmartPipe.Extensions.Json/JsonRecordValidator.cs` around lines 9 - 25, Add an explanatory comment to the intentionally empty body of the while loop in JsonRecordValidator, clarifying that exhausting Utf8JsonReader validates structural and trailing-content errors. Keep the existing reader logic and exception handling unchanged.Source: Linters/SAST tools
src/SmartPipe.Extensions.Json/DeadLetterRecordReader.cs (1)
10-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract per-record parsing to reduce cognitive complexity.
SonarCloud flags
ReadFramedAsyncat complexity 21 (limit 15). Extracting the size-check + validate/deserialize try/catch block (lines 25-48) into a private helper (e.g.TryReadRecordAsync) returning a result/invalid-exception tuple would flatten the method and satisfy the analyzer without changing behavior.🤖 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/SmartPipe.Extensions.Json/DeadLetterRecordReader.cs` around lines 10 - 61, Reduce the cognitive complexity of ReadFramedAsync by extracting the per-record size check and validation/deserialization logic into a private TryReadRecordAsync helper. Have the helper return the parsed DeadLetterEnvelope<T> together with any JsonException, preserving existing record-size checks, serializer validation, and invalid-record behavior while leaving logging and iteration in ReadFramedAsync.Source: Linters/SAST tools
src/SmartPipe.Core/CircuitBreaker.cs (1)
706-721: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
GetWindowStatisticslocks for a full linear scan on the failure/success hot path.
ConcurrentQueue<T>enumeration is already safe without external locking. Holding_windowGatefor the wholeforeachinGetWindowStatistics(invoked on everyRecordFailure/RecordSuccessviaEvaluateSlidingWindow) adds lock contention proportional to window size without a clear correctness need, since nothing else requires stronger consistency than the queue's own snapshot semantics.♻️ Suggested change
private WindowStatistics GetWindowStatistics() { - lock (_windowGate) - { - var total = 0; - var failures = 0; - foreach (var (_, isSuccess) in _window) - { - total++; - if (!isSuccess) - failures++; - } - - return new WindowStatistics(total, failures); - } + var total = 0; + var failures = 0; + foreach (var (_, isSuccess) in _window) + { + total++; + if (!isSuccess) + failures++; + } + + return new WindowStatistics(total, failures); }🤖 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/SmartPipe.Core/CircuitBreaker.cs` around lines 706 - 721, Remove the _windowGate lock from GetWindowStatistics and perform the existing ConcurrentQueue enumeration directly, preserving the current total and failure counting logic and WindowStatistics result.src/SmartPipe.Extensions.Json/JsonOptions.cs (1)
37-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating
JsonFileSourceOptionsandDeadLetterSourceOptions.These two records have identical properties with identical defaults. If they're expected to evolve independently, keeping them separate is fine. If not, consider having
DeadLetterSourceOptionsreuseJsonFileSourceOptionsor extracting a shared base to reduce maintenance burden.Also applies to: 62-75
🤖 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/SmartPipe.Extensions.Json/JsonOptions.cs` around lines 37 - 49, Deduplicate the identical option definitions represented by JsonFileSourceOptions and DeadLetterSourceOptions. Prefer reusing JsonFileSourceOptions or extracting a shared options base, while preserving the existing properties and defaults and allowing the two source configurations to retain any necessary source-specific behavior..github/workflows/publish-nuget.yml (2)
42-45: 🩺 Stability & Availability | 🔵 TrivialManual
recoverable-rerundispatches must target the release tag.The tag-ref check (
$GITHUB_REF != refs/tags/v*) still applies underworkflow_dispatch. If an operator triggers the recovery run from the default branch selector without explicitly choosing the release tag, the job fails immediately with the generic message, which doesn't hint at "pick the tag when dispatching." Consider clarifying this in the error message or in run-book docs.🤖 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 @.github/workflows/publish-nuget.yml around lines 42 - 45, Update the validation message in the workflow’s tag-ref check to explicitly tell operators using manual recoverable-rerun dispatches to select the release tag, while preserving the existing rejection behavior for non-release refs.
76-89: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftConsider migrating to NuGet Trusted Publishing (OIDC) instead of a long-lived
NUGET_API_KEY.NuGet.org's Trusted Publishing exchanges a short-lived GitHub OIDC token for a temporary (~1 hour) API key, removing the need to store/rotate a long-lived secret. This directly addresses the zizmor hints on lines 86-88.
As per static analysis hints, zizmor recommends: "prefer trusted publishing for authentication (use-trusted-publishing)".
I verified this is a currently valid, GA capability on nuget.org (not deprecated/experimental) as of mid-2026 via web search.
🤖 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 @.github/workflows/publish-nuget.yml around lines 76 - 89, Replace the long-lived NUGET_API_KEY authentication in the publish step with NuGet Trusted Publishing via GitHub OIDC. Configure the job permissions and NuGet publishing commands according to the supported trusted-publishing flow, remove the secret-based environment variable and references, and preserve the existing package versioning and RECOVERABLE_RERUN skip-duplicate behavior.Source: Linters/SAST tools
.github/workflows/reusable-release-validation.yml (1)
25-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout.Later steps generate throwaway consumer projects and run
dotnet run/dotnet publish, which restore and execute arbitrary transitive NuGet packages. Leaving theGITHUB_TOKENpersisted in.git/configgives that code an exfiltration target. Disabling credential persistence closes it (the token isn't used for git operations in this job).🔒 Proposed hardening
- - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false🤖 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 @.github/workflows/reusable-release-validation.yml at line 25, Update the actions/checkout step in the release validation workflow to set persist-credentials to false. Leave the existing checkout action version and surrounding workflow behavior unchanged.Source: Linters/SAST tools
🤖 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 @.github/workflows/ci.yml:
- Line 24: Update the actions/checkout step to set persist-credentials to false,
while preserving the existing pinned action reference and version comment.
- Around line 34-64: Update the package workflow around the “Package split
direct, forwarding, and legacy consumers” step so SmartPipe.Extensions is built
in Release before its dotnet pack --no-build command runs. Add the build for
src/SmartPipe.Extensions/SmartPipe.Extensions.csproj alongside the existing
build flow, preserving the current no-build packaging behavior.
In @.github/workflows/publish-nuget.yml:
- Around line 29-31: Update the actions/checkout step in the version job to set
persist-credentials to false alongside fetch-depth, keeping the existing
checkout action and shallow/full-history configuration unchanged.
In `@docs/getting-started.md`:
- Around line 5-19: The “Choose the integration package” guidance should show
SmartPipe.Core alone as sufficient for the Delegate Pipeline and Component
Pipeline examples. Split the package instructions into a Core-only setup for
those examples and an additional integration-package setup for JSON, HTTP,
database, and other extensions, without presenting all three packages as
required upfront.
In `@eng/validate-json-package-split.ps1`:
- Line 224: Remove the --configfile argument from the dotnet run/build
invocations in the validation flow, including the command around Invoke-DotNet.
Rely on the parent NuGet.Config hierarchy, or explicitly restore with
--configfile first and then run/build with --no-restore.
In `@src/SmartPipe.Core/PipelineObserverDispatcher.cs`:
- Around line 216-244: The best-effort WriteAsync path currently lets
ChannelClosedException escape when the channel closes with a recorded pipeline
fault. Update the ChannelClosedException handling around WriteAsync to detect
closure while _completed is still zero, retrieve and rethrow the fault supplied
to _events.Writer.TryComplete, and preserve the existing teardown handling for
completed dispatchers.
In `@src/SmartPipe.Extensions.Json/README.md`:
- Around line 6-10: Update the repository’s lychee configuration to exclude
SmartPipe NuGet package URLs from link validation, or accept their expected
404/429 responses, so the unpublished SmartPipe.Extensions.Json package does not
block CI before release.
In `@src/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cs`:
- Around line 151-176: Change the legacy top-level-values deserialization flow
around JsonDocumentLimitStream so MaxDocumentSizeBytes is applied independently
to each JSON record rather than accumulating across the entire stream. Preserve
the existing whole-document behavior for array-rooted input, and ensure each
top-level value is processed through a fresh record-scoped limit before
ProcessElement is called.
In `@src/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cs`:
- Around line 188-213: The auto-detected deserialization path currently reuses
one JsonDocumentLimitStream across all top-level values, causing
MaxDocumentSizeBytes to accumulate globally. Update the flow around
_deserializeBatches and JsonDocumentLimitStream so each top-level JSON value is
processed with a fresh limit stream whose byte count resets, while preserving
batch iteration, invalid-null handling, and envelope creation.
In `@src/SmartPipe.Extensions.Json/Sinks/AppendFraming.cs`:
- Around line 11-38: Update RequiresLineSeparatorAsync’s three-byte BOM check to
use ReadExactlyAsync instead of ReadAsync, ensuring the full BOM is read before
comparing bom to the UTF-8 BOM. Preserve the existing cancellation and
stream-position restoration behavior.
In
`@tests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceRecoveryTests.cs`:
- Line 373: Update both fake stream implementations in
DeadLetterSourceRecoveryTests, including the methods containing the line 373 and
line 463 copies, to use bytes.AsMemory().CopyTo(buffer) instead of
bytes.CopyTo(buffer).
---
Outside diff comments:
In `@docs/getting-started.md`:
- Around line 93-104: The component lists in the documentation do not identify
package ownership despite the split lead-in. Update the selector, transform, and
sink bullets to clearly label or group each item under SmartPipe.Extensions.Json
versus SmartPipe.Extensions, preserving the existing component names and
classifications.
---
Nitpick comments:
In @.github/workflows/publish-nuget.yml:
- Around line 42-45: Update the validation message in the workflow’s tag-ref
check to explicitly tell operators using manual recoverable-rerun dispatches to
select the release tag, while preserving the existing rejection behavior for
non-release refs.
- Around line 76-89: Replace the long-lived NUGET_API_KEY authentication in the
publish step with NuGet Trusted Publishing via GitHub OIDC. Configure the job
permissions and NuGet publishing commands according to the supported
trusted-publishing flow, remove the secret-based environment variable and
references, and preserve the existing package versioning and RECOVERABLE_RERUN
skip-duplicate behavior.
In @.github/workflows/reusable-release-validation.yml:
- Line 25: Update the actions/checkout step in the release validation workflow
to set persist-credentials to false. Leave the existing checkout action version
and surrounding workflow behavior unchanged.
In `@src/SmartPipe.Core/CircuitBreaker.cs`:
- Around line 706-721: Remove the _windowGate lock from GetWindowStatistics and
perform the existing ConcurrentQueue enumeration directly, preserving the
current total and failure counting logic and WindowStatistics result.
In `@src/SmartPipe.Extensions.Json/DeadLetterRecordReader.cs`:
- Around line 10-61: Reduce the cognitive complexity of ReadFramedAsync by
extracting the per-record size check and validation/deserialization logic into a
private TryReadRecordAsync helper. Have the helper return the parsed
DeadLetterEnvelope<T> together with any JsonException, preserving existing
record-size checks, serializer validation, and invalid-record behavior while
leaving logging and iteration in ReadFramedAsync.
In `@src/SmartPipe.Extensions.Json/JsonDocumentLimitStream.cs`:
- Line 58: Update the Flush method in JsonDocumentLimitStream to be a no-op
instead of throwing NotSupportedException, while leaving the stream’s other
read-only behavior unchanged.
In `@src/SmartPipe.Extensions.Json/JsonOptions.cs`:
- Around line 37-49: Deduplicate the identical option definitions represented by
JsonFileSourceOptions and DeadLetterSourceOptions. Prefer reusing
JsonFileSourceOptions or extracting a shared options base, while preserving the
existing properties and defaults and allowing the two source configurations to
retain any necessary source-specific behavior.
In `@src/SmartPipe.Extensions.Json/JsonRecordValidator.cs`:
- Around line 9-25: Add an explanatory comment to the intentionally empty body
of the while loop in JsonRecordValidator, clarifying that exhausting
Utf8JsonReader validates structural and trailing-content errors. Keep the
existing reader logic and exception handling unchanged.
In `@src/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cs`:
- Around line 111-176: Reduce the cognitive complexity of ReadEnvelopesAsync by
extracting serializer/format validation and the separate streaming or legacy
iteration paths into focused helper methods. Keep ReadEnvelopesAsync responsible
for dispatching between those paths, and preserve the existing cancellation,
empty-file, array/framed, and null OriginalPayload behaviors.
- Around line 270-295: Extract the duplicated ReadFirstJsonByteAsync logic from
DeadLetterSource and JsonFileSource into a shared internal static helper, then
update both callers to use it. Preserve the existing BOM handling, whitespace
skipping, stream positioning, cancellation, and nullable-byte behavior while
removing the duplicate implementations.
In `@src/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cs`:
- Around line 67-79: Extract the repeated "document" scope value into a
class-level constant in JsonFileSource, then replace all four usages in
_deserializeItems and _deserializeBatches, including the corresponding code
around _deserializeItemRecord and _deserializeBatchRecord, with that constant.
- Around line 323-348: Extract the duplicated ReadFirstJsonByteAsync logic from
JsonFileSource and DeadLetterSource into a shared internal helper, then update
both callers to use it. Preserve the existing BOM detection, whitespace
skipping, cancellation behavior, and stream-position semantics consistently,
including the initial reset before applying the BOM offset.
- Around line 245-289: Reduce the cognitive complexity of ReadFramedRecordsAsync
below the configured threshold by extracting the oversized-record handling and
JSON validation/deserialization error handling into focused helper methods.
Preserve the existing record index tracking, HandleInvalidRecord behavior, null
handling, and yielded-record behavior.
In `@src/SmartPipe.Extensions.Json/Sinks/JsonFileSink.cs`:
- Line 30: Update the _leaveOpen field in JsonFileSink to be readonly,
preserving its single assignment in the internal constructor and any existing
constructor behavior.
- Around line 240-252: In the batch-writing loop, merge the nested
_options.Format == JsonFileFormat.Array condition with the enclosing conditional
so comma insertion remains guarded by both the array format and (_arrayHasItems
|| output.Length > 1). Preserve the existing serialization and NDJSON newline
behavior.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df57002f-90d5-4ed2-9949-ffe8731315af
📒 Files selected for processing (84)
.github/workflows/ci.yml.github/workflows/publish-nuget.yml.github/workflows/reusable-release-validation.ymlCHANGELOG.mdDirectory.Build.propsREADME.mdSmartPipe.Core.slnxdocs/aot-compatibility.mddocs/api-reference.mddocs/architecture.mddocs/getting-started.mddocs/migration/2.1.2-json-package-split.mddocs/resilience.mdeng/tests/validate-json-package-split.Tests.ps1eng/tests/validate-release-version.Tests.sheng/tests/workflow-contract.Tests.ps1eng/tests/workflow_contract_tests.pyeng/validate-json-package-split.ps1eng/validate-release-version.shsrc/SmartPipe.Core/CircuitBreaker.cssrc/SmartPipe.Core/DeadLetterSerialization.cssrc/SmartPipe.Core/PipelineObserverDispatcher.cssrc/SmartPipe.Core/PipelineRuntimeOptions.cssrc/SmartPipe.Core/PublicAPI.Shipped.txtsrc/SmartPipe.Core/PublicAPI.Unshipped.txtsrc/SmartPipe.Core/Runtime/Execution/PipelineTime.cssrc/SmartPipe.Core/SmartPipe.Core.csprojsrc/SmartPipe.Core/TypedPipelineRuntime.cssrc/SmartPipe.Extensions.Json/DeadLetterRecordReader.cssrc/SmartPipe.Extensions.Json/JsonDocumentLimitStream.cssrc/SmartPipe.Extensions.Json/JsonDocumentValidator.cssrc/SmartPipe.Extensions.Json/JsonInfrastructureContext.cssrc/SmartPipe.Extensions.Json/JsonInputOptionsValidator.cssrc/SmartPipe.Extensions.Json/JsonOptions.cssrc/SmartPipe.Extensions.Json/JsonRecordValidator.cssrc/SmartPipe.Extensions.Json/PublicAPI.Shipped.txtsrc/SmartPipe.Extensions.Json/PublicAPI.Unshipped.txtsrc/SmartPipe.Extensions.Json/README.mdsrc/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cssrc/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cssrc/SmartPipe.Extensions.Json/SharedAsyncDisposeState.cssrc/SmartPipe.Extensions.Json/Sinks/AppendFraming.cssrc/SmartPipe.Extensions.Json/Sinks/DeadLetterSink.cssrc/SmartPipe.Extensions.Json/Sinks/JsonFileSink.cssrc/SmartPipe.Extensions.Json/SmartPipe.Extensions.Json.csprojsrc/SmartPipe.Extensions.Json/Transforms/JsonTransform.cssrc/SmartPipe.Extensions.Json/Utf8LineRecordReader.cssrc/SmartPipe.Extensions.Json/packages.lock.jsonsrc/SmartPipe.Extensions/JsonTypeForwarders.cssrc/SmartPipe.Extensions/PublicAPI.Shipped.txtsrc/SmartPipe.Extensions/PublicAPI.Unshipped.txtsrc/SmartPipe.Extensions/README.mdsrc/SmartPipe.Extensions/Selectors/DeadLetterSource.cssrc/SmartPipe.Extensions/Selectors/JsonFileSource.cssrc/SmartPipe.Extensions/Sinks/DeadLetterSink.cssrc/SmartPipe.Extensions/Sinks/JsonFileSink.cssrc/SmartPipe.Extensions/SmartPipe.Extensions.csprojsrc/SmartPipe.Extensions/packages.lock.jsontests/SmartPipe.Core.Tests/Engine/ObserverDispatcherTests.cstests/SmartPipe.Core.Tests/Engine/TypedPipelineLifecycleTests.cstests/SmartPipe.Core.Tests/Resilience/CircuitBreakerTests.cstests/SmartPipe.Core.Tests/Types/DeadLetterSerializationTests.cstests/SmartPipe.Extensions.Json.Tests/DeadLetterSinkAppendTests.cstests/SmartPipe.Extensions.Json.Tests/DeadLetterSinkLifecycleTests.cstests/SmartPipe.Extensions.Json.Tests/DeadLetterSinkTests.cstests/SmartPipe.Extensions.Json.Tests/Fixtures/JsonGoldenFixtureTests.cstests/SmartPipe.Extensions.Json.Tests/JsonDocumentLimitStreamTests.cstests/SmartPipe.Extensions.Json.Tests/JsonFileRoundTripTests.cstests/SmartPipe.Extensions.Json.Tests/JsonOptionsTests.cstests/SmartPipe.Extensions.Json.Tests/JsonTransformTests.cstests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkAppendTests.cstests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cstests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkTests.cstests/SmartPipe.Extensions.Json.Tests/SmartPipe.Extensions.Json.Tests.csprojtests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceRecoveryTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/JsonFileSourceLimitTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/JsonFileSourceMetadataTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/JsonFileSourceRecoveryTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/JsonFileSourceTests.cstests/SmartPipe.Extensions.Json.Tests/Utf8LineRecordReaderTests.cstests/SmartPipe.Extensions.Json.Tests/packages.lock.jsontests/SmartPipe.Extensions.Tests/PackageOwnershipTests.cstests/SmartPipe.Extensions.Tests/packages.lock.json
💤 Files with no reviewable changes (5)
- src/SmartPipe.Extensions/Selectors/JsonFileSource.cs
- src/SmartPipe.Extensions/Sinks/JsonFileSink.cs
- src/SmartPipe.Extensions/Selectors/DeadLetterSource.cs
- src/SmartPipe.Extensions/Sinks/DeadLetterSink.cs
- src/SmartPipe.Extensions/PublicAPI.Unshipped.txt
| ## Installation | ||
|
|
||
| ```bash | ||
| dotnet add package SmartPipe.Extensions.Json --version 2.1.2 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
CI link check fails: NuGet package page is not published yet.
The markdown link check (lychee) fails with a 404 for https://www.nuget.org/packages/SmartPipe.Extensions.Json because the 2.1.2 package hasn't been published. Since this URL won't resolve until after release, exclude it from the link checker (lychee --exclude / .lycheeignore, or accept 404/429 for nuget.org/packages/SmartPipe.*) so the release PR isn't blocked pre-publish.
🤖 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/SmartPipe.Extensions.Json/README.md` around lines 6 - 10, Update the
repository’s lychee configuration to exclude SmartPipe NuGet package URLs from
link validation, or accept their expected 404/429 responses, so the unpublished
SmartPipe.Extensions.Json package does not block CI before release.
Source: Pipeline failures
| if (!_sent) | ||
| { | ||
| _sent = true; | ||
| bytes.CopyTo(buffer); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Enumerate the CopyTo call sites in the file to confirm the divergent patterns.
fd -a 'DeadLetterSourceRecoveryTests.cs' | while read -r f; do
rg -nP 'bytes\.(CopyTo|AsMemory)\b' "$f"
doneRepository: MrFr3di/SmartPipe-Core
Length of output: 305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceRecoveryTests.cs"
echo "== usings =="
sed -n '1,80p' "$file"
echo
echo "== relevant regions =="
sed -n '350,480p' "$file"
echo
echo "== repo search for byte[] CopyTo extensions =="
rg -n "CopyTo\\s*\\(\\s*this\\s+byte\\s*\\[\\]" .
rg -n "static\\s+.*CopyTo\\s*\\(" tests src . | head -n 100Repository: MrFr3di/SmartPipe-Core
Length of output: 10407
Replace bytes.CopyTo(buffer) with bytes.AsMemory().CopyTo(buffer)
bytes is a byte[], so the direct CopyTo(Memory<byte>) call doesn’t bind. This appears in both fake streams at lines 373 and 463.
🤖 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
`@tests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceRecoveryTests.cs`
at line 373, Update both fake stream implementations in
DeadLetterSourceRecoveryTests, including the methods containing the line 373 and
line 463 copies, to use bytes.AsMemory().CopyTo(buffer) instead of
bytes.CopyTo(buffer).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eng/tests/workflow_contract_tests.py (1)
207-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a constant for
--minimum-expected-tests 1.This literal appears at line 207 (validation check), line 268 (
_strip_minimum_expected_from_windows), and line 314 (mainmutation assertion). Duplicating it risks silent drift if the flag name changes.♻️ Proposed refactor
Add a module-level constant near the other constants (e.g., after line 21):
+MINIMUM_EXPECTED_TESTS = "--minimum-expected-tests 1"Then replace all three occurrences:
# Line 207 - require("--minimum-expected-tests 1" in command, + require(MINIMUM_EXPECTED_TESTS in command, f"Every filtered test command must set --minimum-expected-tests 1: {command}") # Line 268 - step["run"] = str(step["run"]).replace("--minimum-expected-tests 1", "") + step["run"] = str(step["run"]).replace(MINIMUM_EXPECTED_TESTS, "") # Line 314 - "--minimum-expected-tests 1", + MINIMUM_EXPECTED_TESTS,Also applies to: 268-268, 314-314
🤖 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 `@eng/tests/workflow_contract_tests.py` at line 207, Extract the repeated “--minimum-expected-tests 1” value into a module-level constant near the existing constants, then reuse it in the validation check, _strip_minimum_expected_from_windows, and main mutation assertion. Remove the duplicated literals while preserving the current command-validation and mutation behavior.Source: Linters/SAST tools
🤖 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 `@eng/tests/workflow_contract_tests.py`:
- Line 204: Update the require call around filtered to pass an explicit boolean
by converting the list with bool(filtered) or checking len(filtered) > 0, while
preserving the existing assertion behavior.
---
Nitpick comments:
In `@eng/tests/workflow_contract_tests.py`:
- Line 207: Extract the repeated “--minimum-expected-tests 1” value into a
module-level constant near the existing constants, then reuse it in the
validation check, _strip_minimum_expected_from_windows, and main mutation
assertion. Remove the duplicated literals while preserving the current
command-validation and mutation behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c609b52-4ce1-456f-bd47-709c61e5b5c6
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/publish-nuget.yml.github/workflows/reusable-release-validation.ymleng/tests/workflow_contract_tests.pylychee.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- .github/workflows/publish-nuget.yml
- .github/workflows/reusable-release-validation.yml
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
- Rename MaxDocumentSizeBytes to MaxUnframedInputSizeBytes (default 256 MiB). - Rename JsonDocumentLimitStream to JsonUnframedInputLimitStream with an explicit unframed-input message. - Apply MaxDepth uniformly to the legacy unframed dead-letter stream. - Add DeadLetterSource(path, JsonTypeInfo<T>, DeadLetterSourceOptions) so the legacy unframed path is configurable. - Update README, migration guide, and changelog; add contract tests and boundary cases.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/SmartPipe.Core.Tests/Engine/ObserverDispatcherTests.cs (1)
83-87: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the concurrency coordination waits.
A regression preventing the callback or observer entry would currently hang the suite indefinitely rather than produce a useful failure.
Proposed fix
- await timeProvider.TimerCallbackEntered.Task; + await timeProvider.TimerCallbackEntered.Task.WaitAsync(TimeSpan.FromSeconds(5)); var dispose = fixture.Dispatcher.DisposeAsync().AsTask(); timeProvider.ReleaseTimerCallback(); - await Task.WhenAll(advance, emit, dispose); + await Task.WhenAll(advance, emit, dispose).WaitAsync(TimeSpan.FromSeconds(5));await dispatcher.EmitAsync(NewStartedEvent(), CancellationToken.None); - await observer.Entered.Task; + await observer.Entered.Task.WaitAsync(TimeSpan.FromSeconds(5)); await dispatcher.EmitAsync(NewStartedEvent(), CancellationToken.None);Also applies to: 292-295
🤖 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 `@tests/SmartPipe.Core.Tests/Engine/ObserverDispatcherTests.cs` around lines 83 - 87, Bound the coordination awaits in the test around timeProvider.TimerCallbackEntered.Task and Task.WhenAll(advance, emit, dispose) so callback or observer-entry regressions fail within a finite timeout instead of hanging indefinitely. Apply the same bounded-wait behavior to the corresponding coordination block noted in the comment, while preserving the existing synchronization order and assertions.
🧹 Nitpick comments (1)
src/SmartPipe.Extensions.Json/JsonStreamProbe.cs (1)
35-40: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScan leading whitespace in buffered chunks.
The current loop performs one asynchronous stream read per byte, making large whitespace prefixes unnecessarily expensive.
Proposed fix
- var buffer = new byte[1]; - while (await stream.ReadAsync(buffer, ct).ConfigureAwait(false) == 1) + var buffer = new byte[4096]; + int bytesRead; + while ((bytesRead = await stream.ReadAsync(buffer, ct).ConfigureAwait(false)) > 0) { - if (buffer[0] is not ((byte)' ' or (byte)'\t' or (byte)'\r' or (byte)'\n')) - return new JsonStreamProbeResult(buffer[0], contentStartOffset); + foreach (var value in buffer.AsSpan(0, bytesRead)) + { + if (value is not ((byte)' ' or (byte)'\t' or (byte)'\r' or (byte)'\n')) + return new JsonStreamProbeResult(value, contentStartOffset); + } }🤖 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/SmartPipe.Extensions.Json/JsonStreamProbe.cs` around lines 35 - 40, Update the leading-whitespace scan in the JsonStreamProbe logic to read buffered chunks instead of allocating a one-byte buffer and awaiting once per byte. Inspect each byte in the returned chunk, return the first non-whitespace byte with the correct contentStartOffset, and preserve cancellation and end-of-stream 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 `@tests/SmartPipe.Core.Tests/Engine/ObserverDispatcherTests.cs`:
- Around line 167-177: Update the disposal test around observer.Entered and
blockedEmit to saturate the channel first: emit and await an additional queued
event before creating the second emission that should remain blocked. Then
dispose and release the observer as currently arranged, preserving the assertion
that the genuinely blocked emission completes without throwing.
In `@tests/SmartPipe.Extensions.Json.Tests/Sources/JsonFileSourceLimitTests.cs`:
- Around line 158-174: Update the test method
LegacyTopLevelSequence_TotalAboveLimit_Throws to write an unwrapped payload such
as “1 2” instead of the root array “[1,2]”, so JsonFileFormat.Auto exercises
legacy top-level sequence detection while preserving the existing limit and
exception assertions.
---
Outside diff comments:
In `@tests/SmartPipe.Core.Tests/Engine/ObserverDispatcherTests.cs`:
- Around line 83-87: Bound the coordination awaits in the test around
timeProvider.TimerCallbackEntered.Task and Task.WhenAll(advance, emit, dispose)
so callback or observer-entry regressions fail within a finite timeout instead
of hanging indefinitely. Apply the same bounded-wait behavior to the
corresponding coordination block noted in the comment, while preserving the
existing synchronization order and assertions.
---
Nitpick comments:
In `@src/SmartPipe.Extensions.Json/JsonStreamProbe.cs`:
- Around line 35-40: Update the leading-whitespace scan in the JsonStreamProbe
logic to read buffered chunks instead of allocating a one-byte buffer and
awaiting once per byte. Inspect each byte in the returned chunk, return the
first non-whitespace byte with the correct contentStartOffset, and preserve
cancellation and end-of-stream behavior.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c43bc08d-ed8a-4d92-8a46-bf1efbc801cc
📒 Files selected for processing (25)
CHANGELOG.mddocs/migration/2.1.2-json-package-split.mdeng/tests/validate-json-package-split.Tests.ps1eng/tests/workflow_contract_tests.pyeng/validate-json-package-split.ps1src/SmartPipe.Core/PipelineObserverDispatcher.cssrc/SmartPipe.Extensions.Json/JsonInputOptionsValidator.cssrc/SmartPipe.Extensions.Json/JsonOptions.cssrc/SmartPipe.Extensions.Json/JsonStreamProbe.cssrc/SmartPipe.Extensions.Json/JsonUnframedInputLimitStream.cssrc/SmartPipe.Extensions.Json/PublicAPI.Unshipped.txtsrc/SmartPipe.Extensions.Json/README.mdsrc/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cssrc/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cssrc/SmartPipe.Extensions.Json/Sinks/AppendFraming.cssrc/SmartPipe.Extensions/PublicAPI.Unshipped.txttests/SmartPipe.Core.Tests/Engine/ObserverDispatcherTests.cstests/SmartPipe.Extensions.Json.Tests/JsonOptionsTests.cstests/SmartPipe.Extensions.Json.Tests/JsonStreamProbeTests.cstests/SmartPipe.Extensions.Json.Tests/JsonUnframedInputLimitStreamTests.cstests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkAppendTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceRecoveryTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/JsonFileSourceLimitTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/JsonFileSourceTests.cs
🚧 Files skipped from review as they are similar to previous changes (11)
- CHANGELOG.md
- src/SmartPipe.Extensions.Json/Sinks/AppendFraming.cs
- src/SmartPipe.Extensions/PublicAPI.Unshipped.txt
- src/SmartPipe.Extensions.Json/README.md
- docs/migration/2.1.2-json-package-split.md
- tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkAppendTests.cs
- src/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cs
- src/SmartPipe.Extensions.Json/PublicAPI.Unshipped.txt
- tests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceTests.cs
- eng/tests/workflow_contract_tests.py
- src/SmartPipe.Core/PipelineObserverDispatcher.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cs (1)
184-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fault-injecting
MemoryStreamtest doubles across two lifecycle test files.ReentrantDisposeStreamandFaultingDisposeStreamboth reimplement Flush/Dispose failure injection on aMemoryStreamfor near-identical dispose-lifecycle scenarios within the same test project.
tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cs#L184-L238: promoteReentrantDisposeStream(already the superset — supports WriteFailure, blocking, reentrancy) into a shared internal test-utility class for the project.tests/SmartPipe.Extensions.Json.Tests/DeadLetterSinkLifecycleTests.cs#L173-L194: replaceFaultingDisposeStreamwith the shared fault-injecting stream double instead of maintaining a second, narrower copy.🤖 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 `@tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cs` around lines 184 - 238, Promote ReentrantDisposeStream from tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cs:184-238 into a shared internal test utility, preserving its existing write-failure, flush-blocking, reentrancy, and dispose-failure behavior. In tests/SmartPipe.Extensions.Json.Tests/DeadLetterSinkLifecycleTests.cs:173-194, remove FaultingDisposeStream and update its usages to the shared ReentrantDisposeStream; no other lifecycle behavior should change.
🤖 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.
Nitpick comments:
In `@tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cs`:
- Around line 184-238: Promote ReentrantDisposeStream from
tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cs:184-238
into a shared internal test utility, preserving its existing write-failure,
flush-blocking, reentrancy, and dispose-failure behavior. In
tests/SmartPipe.Extensions.Json.Tests/DeadLetterSinkLifecycleTests.cs:173-194,
remove FaultingDisposeStream and update its usages to the shared
ReentrantDisposeStream; no other lifecycle behavior should change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c880a2e-89f3-48e9-b6e7-1e0f94822a87
📒 Files selected for processing (21)
.github/workflows/publish-nuget.yml.github/workflows/reusable-release-validation.yml.work/reviews/pull27/plan 2.1.3.mddocs/getting-started.mdeng/tests/validate-json-package-split.Tests.ps1eng/validate-json-package-split.ps1src/SmartPipe.Extensions.Json/DeadLetterRecordReader.cssrc/SmartPipe.Extensions.Json/JsonRecordValidator.cssrc/SmartPipe.Extensions.Json/PublicAPI.Unshipped.txtsrc/SmartPipe.Extensions.Json/Selectors/DeadLetterSource.cssrc/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cssrc/SmartPipe.Extensions.Json/SharedAsyncDisposeState.cssrc/SmartPipe.Extensions.Json/Sinks/DeadLetterSink.cssrc/SmartPipe.Extensions.Json/Sinks/JsonFileSink.cssrc/SmartPipe.Extensions/PublicAPI.Shipped.txttests/SmartPipe.Extensions.Json.Tests/DeadLetterSinkLifecycleTests.cstests/SmartPipe.Extensions.Json.Tests/JsonFileRoundTripTests.cstests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkAppendTests.cstests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkLifecycleTests.cstests/SmartPipe.Extensions.Json.Tests/Sources/DeadLetterSourceRecoveryTests.cstests/SmartPipe.Extensions.Tests/PackageOwnershipTests.cs
💤 Files with no reviewable changes (2)
- src/SmartPipe.Extensions.Json/PublicAPI.Unshipped.txt
- src/SmartPipe.Extensions/PublicAPI.Shipped.txt
🚧 Files skipped from review as they are similar to previous changes (11)
- src/SmartPipe.Extensions.Json/JsonRecordValidator.cs
- src/SmartPipe.Extensions.Json/SharedAsyncDisposeState.cs
- tests/SmartPipe.Extensions.Json.Tests/JsonFileRoundTripTests.cs
- docs/getting-started.md
- eng/tests/validate-json-package-split.Tests.ps1
- .github/workflows/publish-nuget.yml
- .github/workflows/reusable-release-validation.yml
- src/SmartPipe.Extensions.Json/Sinks/JsonFileSink.cs
- src/SmartPipe.Extensions.Json/Selectors/JsonFileSource.cs
- tests/SmartPipe.Extensions.Json.Tests/Sinks/JsonFileSinkAppendTests.cs
- src/SmartPipe.Extensions.Json/Sinks/DeadLetterSink.cs
|
GitHub Actions
Failure
1 participant
Summary by CodeRabbit
SmartPipe.Extensions.Jsonfor JSON file sources, sinks, transforms, and dead-letter processing.SmartPipe.Extensions.