refactor test suite structure to mirror production code#57
Merged
woutervanranst merged 59 commits intomasterfrom Apr 12, 2026
Merged
refactor test suite structure to mirror production code#57woutervanranst merged 59 commits intomasterfrom
woutervanranst merged 59 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #57 +/- ##
==========================================
+ Coverage 72.72% 72.80% +0.07%
==========================================
Files 66 66
Lines 4807 4831 +24
Branches 651 654 +3
==========================================
+ Hits 3496 3517 +21
Misses 1169 1169
- Partials 142 145 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
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:
📝 WalkthroughWalkthroughAdded C#-focused ast-grep documentation and a skills lock; reorganized and replaced large monolithic test files with many focused test suites and extracted test helpers/fakes/fixtures; implemented atomic disk-cache read/write for FileTreeService; introduced multiple test-only blob-service wrappers and streaming helpers; and applied widespread using-directive and namespace cleanups. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (20)
src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs (1)
55-55: Replace NullLogger with FakeLogger.The coding guideline requires using
FakeLoggerinstead ofNullLoggerin tests. FakeLogger provides better test observability and allows verification of logged messages. This pattern appears in 5 locations throughout the file.As per coding guidelines: Use FakeLogger instead of NullLogger in tests.
📝 Proposed pattern for using FakeLogger
For the helper method at line 525 (which would fix multiple tests):
-private ListQueryHandler MakeHandler(FakeSeededBlobContainerService blobs, ChunkIndexService index, string account = "account", string container = "container") => - new(index, new FileTreeService(blobs, s_encryption, index, account, container), new SnapshotService(blobs, s_encryption, account, container), NullLogger<ListQueryHandler>.Instance, account, container); +private ListQueryHandler MakeHandler(FakeSeededBlobContainerService blobs, ChunkIndexService index, string account = "account", string container = "container") => + new(index, new FileTreeService(blobs, s_encryption, index, account, container), new SnapshotService(blobs, s_encryption, account, container), new FakeLogger<ListQueryHandler>(), account, container);Apply the same pattern to the standalone handler instantiations at lines 55, 131, 191, and 243.
Also applies to: 131-131, 191-191, 243-243, 525-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs` at line 55, Tests use NullLogger<ListQueryHandler>.Instance; replace it with FakeLogger<ListQueryHandler> to enable log verification. Update the helper that constructs handlers (the factory method referenced in the comment) to create and return a FakeLogger<T> for the handler, and change the standalone ListQueryHandler instantiations that currently pass NullLogger<ListQueryHandler>.Instance to instead pass a FakeLogger<ListQueryHandler> instance so all five sites (the four direct constructor calls and the helper-based construction) use FakeLogger.src/Arius.Core.Tests/FileTree/FileTreeBlobSerializerStorageTests.cs (1)
57-58: Add a length guard before prefix slicing.At Line 57, assert
bytes.Length >= 6beforebytes[..6]to produce a cleaner failure signal if serialization header generation regresses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/FileTree/FileTreeBlobSerializerStorageTests.cs` around lines 57 - 58, Add a length guard before slicing the header bytes: in the test FileTreeBlobSerializerStorageTests where `bytes` is sliced into `prefix` (`var prefix = System.Text.Encoding.ASCII.GetString(bytes[..6]);`), first assert that `bytes.Length >= 6` (e.g., using the test framework's assertion helper) so the test fails clearly if the serialized header is too short, then perform the slice and `prefix.ShouldBe("ArGCM1")`.src/Arius.Cli.Tests/CrashLoggingTests.cs (1)
20-47: Harden test cleanup to avoid leftover log files on assertion failures.
File.Delete(logFile)on Line 46 and Line 81 is not protected by a final cleanup guard. If an assertion fails first, artifacts can leak and affect later runs. Also consider unique command names per test to avoid filename collisions in same-second execution.Suggested refactor
[Test] public void ConfigureAuditLogging_WritesFullExceptionDetailsToLogFile() { - var logFile = CliBuilder.ConfigureAuditLogging("acct", "ctr", "test"); + var logFile = CliBuilder.ConfigureAuditLogging("acct", "ctr", $"test-{Guid.NewGuid():N}"); try { try { throw new InvalidOperationException("top level failure", new ArgumentException("inner failure")); } catch (InvalidOperationException exception) { Log.Fatal(exception, "Unhandled exception"); } - } - finally - { - Log.CloseAndFlush(); - } - - var logContents = File.ReadAllText(logFile); - - logContents.ShouldContain("Unhandled exception"); - logContents.ShouldContain("System.InvalidOperationException: top level failure"); - logContents.ShouldContain("System.ArgumentException: inner failure"); - logContents.ShouldContain(" at "); - - File.Delete(logFile); + var logContents = File.ReadAllText(logFile); + logContents.ShouldContain("Unhandled exception"); + logContents.ShouldContain("System.InvalidOperationException: top level failure"); + logContents.ShouldContain("System.ArgumentException: inner failure"); + logContents.ShouldContain(" at "); + } + finally + { + Log.CloseAndFlush(); + if (File.Exists(logFile)) File.Delete(logFile); + } }Also applies to: 61-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/CrashLoggingTests.cs` around lines 20 - 47, The test currently deletes the generated log file with File.Delete(logFile) outside a guaranteed cleanup block, so if an assertion fails the artifact can leak; update the test to ensure deletion happens in a finally cleanup (wrap the assertions and reading of the file in a try block and move File.Delete(logFile) into the finally) and make the log filename unique per test invocation by passing a unique test name to CliBuilder.ConfigureAuditLogging (for example append a Guid or timestamp to the "test" command name) so ConfigureAuditLogging(...) and File.Delete(...) refer to a non-colliding file even when tests run in the same second; keep Log.CloseAndFlush() behavior intact.src/Arius.Cli.Tests/TruncateAndLeftJustifyTests.cs (1)
10-48: Add tests for the remaining branch boundaries (width <= 0andwidth <= 3).Current cases are strong, but two explicit branches in
DisplayHelpers.TruncateAndLeftJustifyremain untested. Add focused cases for:
width = 0(expects empty string),width = 3with long input (expects"..."),- optionally
width = 2to confirm dot-only truncation behavior.As per coding guidelines: "Test projects. Focus on test coverage gaps and assertion quality rather than style."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/TruncateAndLeftJustifyTests.cs` around lines 10 - 48, Add unit tests to cover the remaining branch boundaries of DisplayHelpers.TruncateAndLeftJustify: create tests (in TruncateAndLeftJustifyTests) for width = 0 expecting an empty string, for width = 3 with a long input expecting "..." (length 3), and optionally for width = 2 with a long input expecting ".." or single-dot behavior per the implementation; ensure each test asserts both the exact returned string and its Length property and uses DisplayHelpers.TruncateAndLeftJustify to exercise those branches.src/Arius.Cli.Tests/TrackedDownloadLifecycleTests.cs (1)
69-78:InterlockedUpdateis a misleading name for a sequential test.This test validates ordinary sequential assignment behavior, not inter-thread safety semantics. Consider renaming (or adding a true concurrent-writer case).
Minimal rename option
-[Test] -public void TrackedDownload_BytesDownloaded_InterlockedUpdate() +[Test] +public void TrackedDownload_BytesDownloaded_SequentialUpdates()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/TrackedDownloadLifecycleTests.cs` around lines 69 - 78, The test TrackedDownload_BytesDownloaded_InterlockedUpdate is misnamed because it only exercises sequential assignments; rename the test to something like TrackedDownload_BytesDownloaded_SequentialUpdate (update the test method name) or alternatively add a new concurrent test that exercises SetBytesDownloaded from multiple threads (e.g., use Parallel.For or Tasks to call TrackedDownload.SetBytesDownloaded concurrently and then assert the final BytesDownloaded) so the intent matches the behavior; locate the test method and the TrackedDownload, SetBytesDownloaded, and BytesDownloaded symbols to implement the rename or add the additional concurrent-writer test.src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs (1)
180-190: Duplicate scenario coverage exists in another file.This exact removed-file scenario is already covered in
BuildArchiveDisplayDoneTests. Keep one canonical test to avoid drift and reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs` around lines 180 - 190, Remove the duplicate test BuildArchiveDisplay_DoesNotShowRemovedFiles from class BuildArchiveDisplayTests; the same removed-file scenario is already covered in BuildArchiveDisplayDoneTests, so delete the test method (which uses ProgressState.AddFile, SetFileHashed, RemoveFile and asserts via RenderToString(ArchiveVerb.BuildDisplay(state))). Ensure no other references to this test remain in that test class.src/Arius.Cli.Tests/AddRestoreEventTests.cs (1)
20-24: Cap test should assert ordering, not only membership.Since this validates a queue-backed “most recent” window, assert the exact expected sequence (
file6.txt…file15.txt) to catch ordering regressions.Suggested assertion upgrade
var paths = state.RecentRestoreEvents.Select(e => e.RelativePath).ToList(); - paths.ShouldContain("file15.txt"); - paths.ShouldContain("file6.txt"); - paths.ShouldNotContain("file5.txt"); - paths.ShouldNotContain("file1.txt"); + paths.ShouldBe(Enumerable.Range(6, 10).Select(i => $"file{i}.txt").ToList());As per coding guidelines,
**/*.Tests/**: "Test projects. Focus on test coverage gaps and assertion quality rather than style."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/AddRestoreEventTests.cs` around lines 20 - 24, The test currently only checks membership of paths but must assert ordering for the queue-backed "most recent" window: replace the loose ShouldContain/ShouldNotContain assertions on the paths list derived from state.RecentRestoreEvents.Select(e => e.RelativePath).ToList() with a strict sequence assertion that the list equals the expected ordered sequence (e.g., ["file6.txt", ..., "file15.txt"]); update the assertion in the AddRestoreEventTests test method to compare the entire paths list (use your test framework's sequence-equality assertion or Assert.Equal on the expected list) so ordering regressions are caught.src/Arius.Cli.Tests/Commands/Archive/ArchiveCommandTests.cs (1)
52-61: Use an exact path assertion here.
ShouldEndWith("tmp")is pretty loose for a parsing test: a wrong directory can still satisfy it. Comparing againstPath.GetFullPath("/tmp")would verify the actual contract instead of just the last segment.As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Archive/ArchiveCommandTests.cs` around lines 52 - 61, The test Archive_MockHandlerCapturesPath uses a loose assertion ShouldEndWith("tmp"); change it to assert the exact normalized path by comparing ArchiveCommand.CommandOptions.RootDirectory to Path.GetFullPath("/tmp") (i.e., replace the ShouldEndWith assertion with a ShouldBe against Path.GetFullPath("/tmp") so the test verifies the full resolved path produced by harness.InvokeAsync and received via harness.ArchiveHandler).src/Arius.Cli.Tests/TrackedDownloadThreadSafetyTests.cs (1)
23-33: Assert the add/remove operations succeed.Right now this can still pass without proving the tracked-download lifecycle actually happened, because
AddRestoreBytesDownloadedis unconditional andCount == 0is also true if inserts never stick. Checking theTryAdd/TryRemovereturn values would make the test cover the intended behavior directly.Proposed strengthening
- state.TrackedDownloads.TryAdd(key, td); + state.TrackedDownloads.TryAdd(key, td).ShouldBeTrue(); // Simulate byte-level progress td.SetBytesDownloaded(500_000); td.SetBytesDownloaded(1_000_000); // Remove on completion - state.TrackedDownloads.TryRemove(key, out _); + state.TrackedDownloads.TryRemove(key, out _).ShouldBeTrue(); state.AddRestoreBytesDownloaded(1_000_000);As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/TrackedDownloadThreadSafetyTests.cs` around lines 23 - 33, The test does not assert that the tracked-download was actually added/removed because TryAdd/TryRemove return values aren't checked; update the test to assert the boolean results of state.TrackedDownloads.TryAdd(key, td) and state.TrackedDownloads.TryRemove(key, out _) are true, and optionally assert state.TrackedDownloads.ContainsKey(key) is false after removal, while keeping the existing calls to td.SetBytesDownloaded(...) and state.AddRestoreBytesDownloaded(1_000_000) to preserve the lifecycle verification for TrackedDownload and bytes accounting.src/Arius.Cli.Tests/Commands/Restore/RestoreHandlerNewSignatureTests.cs (1)
18-23: Assert the queued event payload too.These tests prove an event was enqueued, but not that the handler forwarded the right
RelativePathandFileSizeintoRecentRestoreEvents. Adding assertions for the single event's path and size would close that gap.As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.Also applies to: 32-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Restore/RestoreHandlerNewSignatureTests.cs` around lines 18 - 23, The test currently asserts an event was enqueued but not that the payload is correct; update the assertions after calling handler.Handle(new FileRestoredEvent("a/b.txt", 4096L), ...) to also assert state.RecentRestoreEvents.First().RelativePath equals "a/b.txt" and state.RecentRestoreEvents.First().FileSize (or the actual property name representing size) equals 4096L; apply the same additional assertions to the other similar assertions around lines 32-37 to ensure the handler forwarded the correct RelativePath and FileSize into RecentRestoreEvents.src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs (1)
5-5: Namespace doesn't match folder structure.Similar to
ListQueryParsingTests.cs, this file is inCommands/Restore/but uses namespaceArius.Cli.Tests. ConsiderArius.Cli.Tests.Commands.Restorefor consistency.♻️ Proposed fix
-namespace Arius.Cli.Tests; +namespace Arius.Cli.Tests.Commands.Restore;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs` at line 5, The test file's namespace is incorrect for its folder—update the namespace declaration in RestoreNotificationHandlerTests (class RestoreNotificationHandlerTests) to match the folder structure by changing from "Arius.Cli.Tests" to "Arius.Cli.Tests.Commands.Restore" so it aligns with other tests (e.g., ListQueryParsingTests) and the file path.src/Arius.Cli.Tests/ProgressCallbackIntegrationTests.cs (3)
60-75: TAR test also bypasses the callback.
foundTar.SetBytesUploaded(150L)is called directly instead of creating and invoking anIProgress<long>that wrapsSetBytesUploaded, which would mirror the actualCreateUploadProgressTAR branch behavior fromArchiveVerb.cs:153-157.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/ProgressCallbackIntegrationTests.cs` around lines 60 - 75, The test bypasses the real callback behavior by calling foundTar.SetBytesUploaded(150L) directly; change it to create an IProgress<long> that wraps SetBytesUploaded and invoke Report(150) to mirror the TAR branch in CreateUploadProgress (see ArchiveVerb.CreateUploadProgress logic) so the test exercises the progress callback path using ProgressState and TrackedTar.
12-29: Test doesn't exercise theIProgress<long>callback it creates.The test creates
hashProgressas anIProgress<long>(line 22) but never invokes it. Instead, it directly callsfile.SetBytesProcessed(2_500_000)on line 27. This bypasses the callback wiring entirely and only tests thatSetBytesProcessedupdatesBytesProcessed.To actually verify the callback wiring, invoke the progress reporter:
♻️ Proposed fix to actually test the callback
hashProgress.ShouldNotBeNull(); file.ShouldNotBeNull(); - file.SetBytesProcessed(2_500_000); + ((IProgress<long>)hashProgress).Report(2_500_000); file.BytesProcessed.ShouldBe(2_500_000L);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/ProgressCallbackIntegrationTests.cs` around lines 12 - 29, The test CreateHashProgress_UpdatesBytesProcessed currently bypasses the progress callback by calling file.SetBytesProcessed directly; change the assertion to invoke the IProgress<long> reporter created (hashProgress.Report(2_500_000)) so the Progress<long> delegates to TrackedFile.SetBytesProcessed and then assert file.BytesProcessed == 2_500_000; locate the test method and replace the direct call to file.SetBytesProcessed with a call to hashProgress.Report(...) (ensure hashProgress is not null before calling) to validate the callback wiring between ProgressState/TrackedFiles and TrackedFile.SetBytesProcessed.
31-58: Same issue:uploadProgresscallback is created but never invoked.Line 56 directly calls
SetBytesProcessedinstead of invoking theuploadProgresscallback. The reset-to-zero behavior (line 48) is exercised, but the actual progress reporting callback is not tested.♻️ Proposed fix
uploadProgress.ShouldNotBeNull(); state.TrackedFiles["chunk.bin"].BytesProcessed.ShouldBe(0L); - state.TrackedFiles["chunk.bin"].SetBytesProcessed(450_000); + ((IProgress<long>)uploadProgress).Report(450_000); state.TrackedFiles["chunk.bin"].BytesProcessed.ShouldBe(450_000L);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/ProgressCallbackIntegrationTests.cs` around lines 31 - 58, The test creates an uploadProgress Progress<long> callback but never invokes it; replace the direct call to state.TrackedFiles["chunk.bin"].SetBytesProcessed(450_000) with invoking the progress callback (uploadProgress.Report(450_000) or uploadProgress!.Report(450_000) after the null check) so the Progress<long> delegate created in the block is exercised; keep the preceding assertions (uploadProgress.ShouldNotBeNull(), BytesProcessed == 0) and then assert state.TrackedFiles["chunk.bin"].BytesProcessed equals 450_000 after calling uploadProgress.Report to verify the callback updated the TrackedFiles via SetBytesProcessed.src/Arius.Cli.Tests/AccountResolutionTests.cs (2)
39-48: Consider wrapping in try/finally for env var cleanup.Unlike the other env-var tests in this file, this test sets
ARIUS_ACCOUNTtonullat line 42 but doesn't wrap the test in try/finally. If the test fails or throws, subsequent tests in this[NotInParallel]group could see unexpected state.♻️ Consistent cleanup pattern
[Test] public async Task Archive_MissingAccountFromAllSources_ReturnsExitCode1() { - Environment.SetEnvironmentVariable("ARIUS_ACCOUNT", null); - var harness = new CliHarness(); - var exitCode = await harness.InvokeAsync("archive /data -k key -c ctr"); - - exitCode.ShouldBe(1); - harness.ArchiveHandler.ReceivedCalls().ShouldBeEmpty(); + var original = Environment.GetEnvironmentVariable("ARIUS_ACCOUNT"); + Environment.SetEnvironmentVariable("ARIUS_ACCOUNT", null); + try + { + var harness = new CliHarness(); + var exitCode = await harness.InvokeAsync("archive /data -k key -c ctr"); + + exitCode.ShouldBe(1); + harness.ArchiveHandler.ReceivedCalls().ShouldBeEmpty(); + } + finally + { + Environment.SetEnvironmentVariable("ARIUS_ACCOUNT", original); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/AccountResolutionTests.cs` around lines 39 - 48, The test Archive_MissingAccountFromAllSources_ReturnsExitCode1 sets the ARIUS_ACCOUNT env var directly and needs deterministic cleanup: capture the original value via Environment.GetEnvironmentVariable("ARIUS_ACCOUNT"), then set ARIUS_ACCOUNT to null, run the test logic (creating CliHarness and calling InvokeAsync), and finally restore the original value in a finally block (using try/finally around the test body) so Environment.SetEnvironmentVariable is always called to reset ARIUS_ACCOUNT even if the test throws; update the method to use this pattern to match other env-var tests.
1-1: Unused import.
NSubstituteis imported but not used in this file. The harness internally uses NSubstitute, but no direct usage here.♻️ Remove unused import
-using NSubstitute; using Shouldly;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/AccountResolutionTests.cs` at line 1, Remove the unused NSubstitute import from AccountResolutionTests.cs by deleting the "using NSubstitute;" directive; there are no references to NSubstitute in this file (e.g., no calls to Substitute.For or related APIs), so simply remove the unused using to clean up imports and avoid compilation/style warnings.src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs (1)
23-23: UseFakeLoggerinstead ofNullLoggerProvider.As per coding guidelines, test projects should use
FakeLoggerinstead ofNullLogger. This enables capturing and asserting on log output if needed.♻️ Proposed fix
- services.AddLogging(b => b.AddProvider(Microsoft.Extensions.Logging.Abstractions.NullLoggerProvider.Instance)); + services.AddLogging(b => b.AddFakeLogging());Note: Ensure
Microsoft.Extensions.Logging.Testingpackage is referenced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs` at line 23, Replace the use of NullLoggerProvider in the test DI setup so the test can capture logs: update the services.AddLogging call that currently uses Microsoft.Extensions.Logging.Abstractions.NullLoggerProvider.Instance to use Microsoft.Extensions.Logging.Testing.FakeLoggerProvider (or the FakeLogger extension) instead and ensure the Microsoft.Extensions.Logging.Testing package is referenced; specifically modify the registration around services.AddLogging in MediatorEventRoutingIntegrationTests so it uses FakeLogger/FakeLoggerProvider to enable asserting log output.src/Arius.Cli.Tests/Commands/Ls/ListQueryParsingTests.cs (1)
6-6: Namespace doesn't match folder structure.The file is located at
Commands/Ls/ListQueryParsingTests.csbut uses namespaceArius.Cli.Tests. Consider usingArius.Cli.Tests.Commands.Lsto match the folder structure, which helps with discoverability and aligns with the mirrored test-structure rules.♻️ Proposed fix
-namespace Arius.Cli.Tests; +namespace Arius.Cli.Tests.Commands.Ls;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Ls/ListQueryParsingTests.cs` at line 6, The test file ListQueryParsingTests.cs uses the top-level namespace Arius.Cli.Tests which doesn't mirror its folder path; update the namespace declaration to Arius.Cli.Tests.Commands.Ls to match the Commands/Ls folder structure and improve discoverability, i.e. change the namespace line in the ListQueryParsingTests class file (ListQueryParsingTests) to the new namespace and adjust any internal using references or partial types if necessary.src/Arius.Cli.Tests/TestSupport/CliHarness.cs (1)
5-5: Remove unused importArius.Core.Shared.ChunkIndex.The import on line 5 is not used anywhere in the file. All chunk hydration types (
ChunkHydrationStatusQuery,ChunkHydrationStatusResult) come fromArius.Core.Features.ChunkHydrationStatusQuery(line 2).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/TestSupport/CliHarness.cs` at line 5, Remove the unused using directive for Arius.Core.Shared.ChunkIndex from CliHarness.cs; locate the using statement "using Arius.Core.Shared.ChunkIndex;" and delete it so only the required imports (e.g., Arius.Core.Features.ChunkHydrationStatusQuery) remain.src/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayAggregateProgressTests.cs (1)
15-26: Consider centralizingRenderToStringtest utility.This helper is repeated in multiple restore display test files; extracting it into a shared test utility would reduce duplication and keep render config consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayAggregateProgressTests.cs` around lines 15 - 26, The RenderToString(IRenderable) test helper is duplicated across restore display tests; extract it into a single shared test utility (e.g., a static method RenderToString in a TestHelpers or RenderTestHelper class) and update tests to call that helper. The extracted method should recreate the same logic and settings (AnsiConsole.Create with AnsiConsoleSettings: Ansi = AnsiSupport.No, ColorSystem = ColorSystemSupport.NoColors, Out = new AnsiConsoleOutput(writer)) and return writer.ToString(), so references to IRenderable, AnsiConsoleSettings and AnsiConsoleOutput remain identical and behavior stays consistent across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/ast-grep/references/rule_reference.md:
- Line 3: Change the unhyphenated compound modifiers to hyphenated forms:
replace "rule building blocks" with "rule-building blocks" (the phrase on Line
3) and replace "log level call" with "log-level call" (the phrase around Line
246); update both occurrences in the document so the compound adjectives are
hyphenated consistently.
- Around line 417-430: The example "Find methods annotated with [Test]" is too
broad: the rule with id "test-methods" currently matches any method_declaration
that has an attribute_list; update the rule to constrain the attribute_list to
the Test attribute by nesting a more specific `has`/`pattern` check inside the
attribute_list (e.g., require an attribute node or attribute name pattern
matching "Test"), or alternatively rename the section to "Find methods with any
attribute" if you intend to keep the generic rule; reference the rule id
"test-methods", node kinds "method_declaration" and "attribute_list", and add a
nested constraint that targets the attribute name "Test".
In @.agents/skills/ast-grep/SKILL.md:
- Line 6: The heading "# ast-grep for C#" triggers markdownlint MD020 because
the trailing "#" is parsed as a closing hash; update the heading in SKILL.md by
escaping the hash (e.g., "ast-grep for C\#") or rewording it (e.g., "ast-grep
for C Sharp") so the ATX heading no longer contains an unescaped trailing hash
and the MD020 warning is resolved.
In `@src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs`:
- Around line 45-138: Tests like
BuildArchiveDisplay_ScanningHeader_ShowsLiveCount,
BuildArchiveDisplay_HashingHeader_*, and BuildArchiveDisplay_UploadingHeader_*
use very narrow assertions (single characters/digits) which can pass due to
other stages' output; instead, locate the specific header line in the
RenderToString(ArchiveVerb.BuildDisplay(state)) output (e.g., find the line that
contains "Scanning", "Hashing" or "Uploading") and assert against that line only
so you verify the intended stage. Update each test (referencing
ArchiveVerb.BuildDisplay, RenderToString, ProgressState and the named tests) to
split or regex the rendered output to extract the relevant header line and then
assert on a precise token or pattern (e.g., "2 files scanned" or "Scanning .*
○/●" or "Hashing .* unique: 3 pending: 12") rather than single
characters/digits.
In
`@src/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayActiveDownloadsTests.cs`:
- Around line 59-74: The test
BuildRestoreDisplay_NoActiveDownloads_NoDownloadTable is only asserting that the
"TAR bundle" label is absent but should instead assert that no download rows
render; update the test to follow the glyph-based extraction pattern used
elsewhere by rendering the output via
RenderToString(RestoreVerb.BuildDisplay(state)) and then assert that none of the
download glyph markers/row identifiers (the same glyph strings used in the other
tests around lines 95–100) appear in the output, ensuring no download progress
rows are present rather than just the "TAR bundle" label.
In `@src/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayTests.cs`:
- Around line 58-66: The assertions in BuildRestoreDisplayTests are too loose
(e.g., output.ShouldContain("10") and single-bullet checks); update them to
assert the full, specific substrings or patterns so they can’t match unrelated
text — for example replace output.ShouldContain("10") with a more precise check
such as output.ShouldContain("10 files") or a regex/assert that the "Resolved"
line contains both the 2026 timestamp and "10 files" together, and similarly
tighten the "Checked"/"2 new"/"0 identical" assertions to match the complete
line or pattern; apply the same tightening to the analogous assertions around
lines 91-93 so each stage’s completion and counts are validated as a single,
specific assertion (use the test class/method BuildRestoreDisplayTests and the
existing output variable to locate and change these assertions).
In `@src/Arius.Cli.Tests/Commands/Restore/TreeTraversalProgressHandlerTests.cs`:
- Line 5: The test file's namespace is incorrect: change the namespace
declaration from "Arius.Cli.Tests" to "Arius.Cli.Tests.Commands.Restore" so it
mirrors the file path and production structure; update the namespace line in
TreeTraversalProgressHandlerTests (and any other tests in the same folder) to
"Arius.Cli.Tests.Commands.Restore" to align with the folder hierarchy and
project conventions.
In `@src/Arius.Cli.Tests/KeyResolutionTests.cs`:
- Around line 9-29: The tests ResolveKey_EnvVarUsedWhenCliFlagOmitted and
ResolveKey_ReturnsNullWhenNoSourceAvailable mutate the ARIUS_KEY process
environment without restoring prior state; capture the previous value via
Environment.GetEnvironmentVariable("ARIUS_KEY") at the start of each test, then
in the finally block restore it with
Environment.SetEnvironmentVariable("ARIUS_KEY", previousValue) (or use null if
previousValue is null) so the global environment is preserved; update both tests
around the calls to CliBuilder.ResolveKey to use this save-and-restore pattern.
In `@src/Arius.Cli.Tests/TrackedFileBytesProcessedTests.cs`:
- Around line 12-30: The test BytesProcessed_UpdatesCorrectlyUnderContention
currently only asserts a range and doesn't validate last-writer semantics;
change it to assert deterministically that the TrackedFile.BytesProcessed equals
the final write value captured in finalValue (or alternatively rename the test
to indicate it only asserts a value within range). Locate the test method
BytesProcessed_UpdatesCorrectlyUnderContention and modify the final assertion to
compare file.BytesProcessed to finalValue (the value set via
Interlocked.Exchange) after the Parallel.ForEachAsync completes, ensuring
SetBytesProcessed is the writer being validated; if you prefer the weaker
behavior, update the test name to reflect non-deterministic outcome instead of
fixing the assertion.
In `@src/Arius.Core.Tests/ChunkIndex/ShardSerializerLocalTests.cs`:
- Line 5: The test file ShardSerializerLocalTests has the wrong namespace;
update the namespace declaration from "Arius.Core.Tests.ChunkIndex" to
"Arius.Core.Tests.Shared.ChunkIndex" (and ensure the test class
ShardSerializerLocalTests uses that namespace) so the test folder mirrors the
production folder Shared/ChunkIndex; if necessary adjust any using directives or
project file entries so the test assembly picks up the renamed namespace.
- Around line 49-51: The test currently only checks the first gzip magic byte
(localBytes[0]) — change the assertion to verify both gzip magic bytes by
replacing the single-byte check with a two-byte comparison, e.g. use
localBytes.Take(2).SequenceEqual(new byte[] { (byte)0x1f, (byte)0x8b
}).ShouldBeFalse(); so the test asserts the data is not gzip-compressed while
keeping the existing salted check (localBytes.Take(8).ShouldNotBe(salted)).
In `@src/Arius.Core.Tests/ChunkIndex/ShardSerializerTests.cs`:
- Line 5: Update the test namespace to mirror the production path: change the
namespace declaration in the ShardSerializerTests file from
"Arius.Core.Tests.ChunkIndex" to "Arius.Core.Tests.Shared.ChunkIndex" (and move
the file into a Shared/ChunkIndex test folder if not already) so the test suite
for ShardSerializer (ShardSerializerTests) mirrors
src/Arius.Core/Shared/ChunkIndex/ShardSerializer.cs; ensure any using/imports or
project references still compile after the namespace change.
In `@src/Arius.Core.Tests/FileTree/FileTreeBlobSerializerStorageTests.cs`:
- Around line 30-33: The tests in FileTreeBlobSerializerStorageTests currently
only assert Name/Hash/Type; extend the round-trip assertions to validate full
entry shape by checking the Created and Modified timestamps on the file entry
and asserting that the dir entry has null timestamps. Locate the assertion block
that inspects back.Entries (the Single(e => e.Name == "photo.jpg") and Single(e
=> e.Name == "subdir/")) and add assertions for e.Created and e.Modified on the
photo.jpg entry (matching the original values used to create the entry) and
assert that the subdir/ entry has Created == null and Modified == null to catch
storage-path regressions.
In `@src/Arius.Core.Tests/FileTree/FileTreeBuilderTests.cs`:
- Line 8: Move the test file for FileTreeBuilder so its folder and namespace
mirror the production path under Shared/FileTree: relocate the
FileTreeBuilderTests.cs into the test tree under a Shared/FileTree folder and
change the namespace from "Arius.Core.Tests.FileTree" to
"Arius.Core.Tests.Shared.FileTree" (and update any surrounding project/ csproj
include if needed); keep the class name FileTreeBuilderTests and its
using/imports intact so tests compile against the FileTreeBuilder under the
production Shared.FileTree namespace.
In `@src/Arius.Core.Tests/Streaming/CountingStreamTests.cs`:
- Around line 4-6: Move the test class to mirror the production layout by
placing the CountingStreamTests under the Shared.Streaming namespace and
corresponding test folder; update the namespace declaration in the file from
Arius.Core.Tests.Streaming to Arius.Core.Tests.Shared.Streaming and relocate the
file so its directory structure matches the production class CountingStream
(Shared.Streaming) to ensure discoverability and conformity with the test
placement guidelines.
- Around line 35-44: Add a new unit test in CountingStreamTests that explicitly
calls the CountingStream.WriteAsync(byte[] buffer, int offset, int count,
CancellationToken) overload to verify partial writes are counted; instantiate a
MemoryStream and a CountingStream, call WriteAsync with a buffer larger than the
count but pass a non-zero offset and a specific count (e.g., buffer length 1024,
offset 100, count 512) using CancellationToken.None, await the call, and assert
CountingStream.BytesWritten equals the count passed. Reference the
CountingStream class, its WriteAsync(byte[], int, int, CancellationToken)
overload, and the BytesWritten property when adding the new test (name it
something like WriteAsync_WithOffsetCount_TracksBytesWritten).
In `@src/Arius.Core.Tests/Streaming/ProgressStreamTests.cs`:
- Around line 33-46: Add two new unit tests in ProgressStreamTests that exercise
ProgressStream.ReadAsync(byte[], int, int, CancellationToken) and
ProgressStream.ReadAsync(Memory<byte>, CancellationToken) directly: create a
source MemoryStream of known length (like 3000 bytes), wrap it in
ProgressStream, use a SyncProgress<long> that appends or updates a captured
list/lastReport on each callback, then perform repeated ReadAsync calls (or a
single ReadAsync into a buffer until zero returned) using CancellationToken.None
to consume the stream and assert that reported progress is non-decreasing and
that the final reported value equals the source length; reference
ProgressStream, ReadAsync(byte[],int,int,CancellationToken),
ReadAsync(Memory<byte>,CancellationToken), and SyncProgress<long> to locate
where to add these assertions.
---
Nitpick comments:
In `@src/Arius.Cli.Tests/AccountResolutionTests.cs`:
- Around line 39-48: The test
Archive_MissingAccountFromAllSources_ReturnsExitCode1 sets the ARIUS_ACCOUNT env
var directly and needs deterministic cleanup: capture the original value via
Environment.GetEnvironmentVariable("ARIUS_ACCOUNT"), then set ARIUS_ACCOUNT to
null, run the test logic (creating CliHarness and calling InvokeAsync), and
finally restore the original value in a finally block (using try/finally around
the test body) so Environment.SetEnvironmentVariable is always called to reset
ARIUS_ACCOUNT even if the test throws; update the method to use this pattern to
match other env-var tests.
- Line 1: Remove the unused NSubstitute import from AccountResolutionTests.cs by
deleting the "using NSubstitute;" directive; there are no references to
NSubstitute in this file (e.g., no calls to Substitute.For or related APIs), so
simply remove the unused using to clean up imports and avoid compilation/style
warnings.
In `@src/Arius.Cli.Tests/AddRestoreEventTests.cs`:
- Around line 20-24: The test currently only checks membership of paths but must
assert ordering for the queue-backed "most recent" window: replace the loose
ShouldContain/ShouldNotContain assertions on the paths list derived from
state.RecentRestoreEvents.Select(e => e.RelativePath).ToList() with a strict
sequence assertion that the list equals the expected ordered sequence (e.g.,
["file6.txt", ..., "file15.txt"]); update the assertion in the
AddRestoreEventTests test method to compare the entire paths list (use your test
framework's sequence-equality assertion or Assert.Equal on the expected list) so
ordering regressions are caught.
In `@src/Arius.Cli.Tests/Commands/Archive/ArchiveCommandTests.cs`:
- Around line 52-61: The test Archive_MockHandlerCapturesPath uses a loose
assertion ShouldEndWith("tmp"); change it to assert the exact normalized path by
comparing ArchiveCommand.CommandOptions.RootDirectory to
Path.GetFullPath("/tmp") (i.e., replace the ShouldEndWith assertion with a
ShouldBe against Path.GetFullPath("/tmp") so the test verifies the full resolved
path produced by harness.InvokeAsync and received via harness.ArchiveHandler).
In `@src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs`:
- Around line 180-190: Remove the duplicate test
BuildArchiveDisplay_DoesNotShowRemovedFiles from class BuildArchiveDisplayTests;
the same removed-file scenario is already covered in
BuildArchiveDisplayDoneTests, so delete the test method (which uses
ProgressState.AddFile, SetFileHashed, RemoveFile and asserts via
RenderToString(ArchiveVerb.BuildDisplay(state))). Ensure no other references to
this test remain in that test class.
In `@src/Arius.Cli.Tests/Commands/Ls/ListQueryParsingTests.cs`:
- Line 6: The test file ListQueryParsingTests.cs uses the top-level namespace
Arius.Cli.Tests which doesn't mirror its folder path; update the namespace
declaration to Arius.Cli.Tests.Commands.Ls to match the Commands/Ls folder
structure and improve discoverability, i.e. change the namespace line in the
ListQueryParsingTests class file (ListQueryParsingTests) to the new namespace
and adjust any internal using references or partial types if necessary.
In
`@src/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayAggregateProgressTests.cs`:
- Around line 15-26: The RenderToString(IRenderable) test helper is duplicated
across restore display tests; extract it into a single shared test utility
(e.g., a static method RenderToString in a TestHelpers or RenderTestHelper
class) and update tests to call that helper. The extracted method should
recreate the same logic and settings (AnsiConsole.Create with
AnsiConsoleSettings: Ansi = AnsiSupport.No, ColorSystem =
ColorSystemSupport.NoColors, Out = new AnsiConsoleOutput(writer)) and return
writer.ToString(), so references to IRenderable, AnsiConsoleSettings and
AnsiConsoleOutput remain identical and behavior stays consistent across tests.
In `@src/Arius.Cli.Tests/Commands/Restore/RestoreHandlerNewSignatureTests.cs`:
- Around line 18-23: The test currently asserts an event was enqueued but not
that the payload is correct; update the assertions after calling
handler.Handle(new FileRestoredEvent("a/b.txt", 4096L), ...) to also assert
state.RecentRestoreEvents.First().RelativePath equals "a/b.txt" and
state.RecentRestoreEvents.First().FileSize (or the actual property name
representing size) equals 4096L; apply the same additional assertions to the
other similar assertions around lines 32-37 to ensure the handler forwarded the
correct RelativePath and FileSize into RecentRestoreEvents.
In `@src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs`:
- Line 5: The test file's namespace is incorrect for its folder—update the
namespace declaration in RestoreNotificationHandlerTests (class
RestoreNotificationHandlerTests) to match the folder structure by changing from
"Arius.Cli.Tests" to "Arius.Cli.Tests.Commands.Restore" so it aligns with other
tests (e.g., ListQueryParsingTests) and the file path.
In `@src/Arius.Cli.Tests/CrashLoggingTests.cs`:
- Around line 20-47: The test currently deletes the generated log file with
File.Delete(logFile) outside a guaranteed cleanup block, so if an assertion
fails the artifact can leak; update the test to ensure deletion happens in a
finally cleanup (wrap the assertions and reading of the file in a try block and
move File.Delete(logFile) into the finally) and make the log filename unique per
test invocation by passing a unique test name to
CliBuilder.ConfigureAuditLogging (for example append a Guid or timestamp to the
"test" command name) so ConfigureAuditLogging(...) and File.Delete(...) refer to
a non-colliding file even when tests run in the same second; keep
Log.CloseAndFlush() behavior intact.
In `@src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs`:
- Line 23: Replace the use of NullLoggerProvider in the test DI setup so the
test can capture logs: update the services.AddLogging call that currently uses
Microsoft.Extensions.Logging.Abstractions.NullLoggerProvider.Instance to use
Microsoft.Extensions.Logging.Testing.FakeLoggerProvider (or the FakeLogger
extension) instead and ensure the Microsoft.Extensions.Logging.Testing package
is referenced; specifically modify the registration around services.AddLogging
in MediatorEventRoutingIntegrationTests so it uses FakeLogger/FakeLoggerProvider
to enable asserting log output.
In `@src/Arius.Cli.Tests/ProgressCallbackIntegrationTests.cs`:
- Around line 60-75: The test bypasses the real callback behavior by calling
foundTar.SetBytesUploaded(150L) directly; change it to create an IProgress<long>
that wraps SetBytesUploaded and invoke Report(150) to mirror the TAR branch in
CreateUploadProgress (see ArchiveVerb.CreateUploadProgress logic) so the test
exercises the progress callback path using ProgressState and TrackedTar.
- Around line 12-29: The test CreateHashProgress_UpdatesBytesProcessed currently
bypasses the progress callback by calling file.SetBytesProcessed directly;
change the assertion to invoke the IProgress<long> reporter created
(hashProgress.Report(2_500_000)) so the Progress<long> delegates to
TrackedFile.SetBytesProcessed and then assert file.BytesProcessed == 2_500_000;
locate the test method and replace the direct call to file.SetBytesProcessed
with a call to hashProgress.Report(...) (ensure hashProgress is not null before
calling) to validate the callback wiring between ProgressState/TrackedFiles and
TrackedFile.SetBytesProcessed.
- Around line 31-58: The test creates an uploadProgress Progress<long> callback
but never invokes it; replace the direct call to
state.TrackedFiles["chunk.bin"].SetBytesProcessed(450_000) with invoking the
progress callback (uploadProgress.Report(450_000) or
uploadProgress!.Report(450_000) after the null check) so the Progress<long>
delegate created in the block is exercised; keep the preceding assertions
(uploadProgress.ShouldNotBeNull(), BytesProcessed == 0) and then assert
state.TrackedFiles["chunk.bin"].BytesProcessed equals 450_000 after calling
uploadProgress.Report to verify the callback updated the TrackedFiles via
SetBytesProcessed.
In `@src/Arius.Cli.Tests/TestSupport/CliHarness.cs`:
- Line 5: Remove the unused using directive for Arius.Core.Shared.ChunkIndex
from CliHarness.cs; locate the using statement "using
Arius.Core.Shared.ChunkIndex;" and delete it so only the required imports (e.g.,
Arius.Core.Features.ChunkHydrationStatusQuery) remain.
In `@src/Arius.Cli.Tests/TrackedDownloadLifecycleTests.cs`:
- Around line 69-78: The test TrackedDownload_BytesDownloaded_InterlockedUpdate
is misnamed because it only exercises sequential assignments; rename the test to
something like TrackedDownload_BytesDownloaded_SequentialUpdate (update the test
method name) or alternatively add a new concurrent test that exercises
SetBytesDownloaded from multiple threads (e.g., use Parallel.For or Tasks to
call TrackedDownload.SetBytesDownloaded concurrently and then assert the final
BytesDownloaded) so the intent matches the behavior; locate the test method and
the TrackedDownload, SetBytesDownloaded, and BytesDownloaded symbols to
implement the rename or add the additional concurrent-writer test.
In `@src/Arius.Cli.Tests/TrackedDownloadThreadSafetyTests.cs`:
- Around line 23-33: The test does not assert that the tracked-download was
actually added/removed because TryAdd/TryRemove return values aren't checked;
update the test to assert the boolean results of
state.TrackedDownloads.TryAdd(key, td) and state.TrackedDownloads.TryRemove(key,
out _) are true, and optionally assert state.TrackedDownloads.ContainsKey(key)
is false after removal, while keeping the existing calls to
td.SetBytesDownloaded(...) and state.AddRestoreBytesDownloaded(1_000_000) to
preserve the lifecycle verification for TrackedDownload and bytes accounting.
In `@src/Arius.Cli.Tests/TruncateAndLeftJustifyTests.cs`:
- Around line 10-48: Add unit tests to cover the remaining branch boundaries of
DisplayHelpers.TruncateAndLeftJustify: create tests (in
TruncateAndLeftJustifyTests) for width = 0 expecting an empty string, for width
= 3 with a long input expecting "..." (length 3), and optionally for width = 2
with a long input expecting ".." or single-dot behavior per the implementation;
ensure each test asserts both the exact returned string and its Length property
and uses DisplayHelpers.TruncateAndLeftJustify to exercise those branches.
In `@src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs`:
- Line 55: Tests use NullLogger<ListQueryHandler>.Instance; replace it with
FakeLogger<ListQueryHandler> to enable log verification. Update the helper that
constructs handlers (the factory method referenced in the comment) to create and
return a FakeLogger<T> for the handler, and change the standalone
ListQueryHandler instantiations that currently pass
NullLogger<ListQueryHandler>.Instance to instead pass a
FakeLogger<ListQueryHandler> instance so all five sites (the four direct
constructor calls and the helper-based construction) use FakeLogger.
In `@src/Arius.Core.Tests/FileTree/FileTreeBlobSerializerStorageTests.cs`:
- Around line 57-58: Add a length guard before slicing the header bytes: in the
test FileTreeBlobSerializerStorageTests where `bytes` is sliced into `prefix`
(`var prefix = System.Text.Encoding.ASCII.GetString(bytes[..6]);`), first assert
that `bytes.Length >= 6` (e.g., using the test framework's assertion helper) so
the test fails clearly if the serialized header is too short, then perform the
slice and `prefix.ShouldBe("ArGCM1")`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c1f46ba-2a78-4d7a-8336-a58e53e6a0c4
📒 Files selected for processing (76)
.agents/skills/ast-grep/SKILL.md.agents/skills/ast-grep/references/rule_reference.mdAGENTS.mdREADME.mdskills-lock.jsonsrc/Arius.Cli.Tests/AccountResolutionTests.cssrc/Arius.Cli.Tests/AddAriusRegistrationTests.cssrc/Arius.Cli.Tests/AddRestoreEventTests.cssrc/Arius.Cli.Tests/ChunkUploadingHandlerDualLookupTests.cssrc/Arius.Cli.Tests/CliTests.cssrc/Arius.Cli.Tests/Commands/Archive/ArchiveCommandTests.cssrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayDoneTests.cssrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayRound2Tests.cssrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cssrc/Arius.Cli.Tests/Commands/Ls/ListQueryParsingTests.cssrc/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayActiveDownloadsTests.cssrc/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayAggregateProgressTests.cssrc/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayDispositionTests.cssrc/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayResolvingTests.cssrc/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayTests.cssrc/Arius.Cli.Tests/Commands/Restore/ChunkDownloadCompletedHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/ChunkResolutionCompleteHandlerByteTotalsTests.cssrc/Arius.Cli.Tests/Commands/Restore/FileRestoredHandlerTrackedDownloadTests.cssrc/Arius.Cli.Tests/Commands/Restore/RestoreCommandTests.cssrc/Arius.Cli.Tests/Commands/Restore/RestoreHandlerNewSignatureTests.cssrc/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/TreeTraversalProgressHandlerTests.cssrc/Arius.Cli.Tests/ContentHashToPathTests.cssrc/Arius.Cli.Tests/CrashLoggingTests.cssrc/Arius.Cli.Tests/KeyResolutionTests.cssrc/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cssrc/Arius.Cli.Tests/NotificationHandlerTests.cssrc/Arius.Cli.Tests/ProgressCallbackIntegrationTests.cssrc/Arius.Cli.Tests/ProgressStateThreadSafetyTests.cssrc/Arius.Cli.Tests/ProgressTests.cssrc/Arius.Cli.Tests/RenderProgressBarTests.cssrc/Arius.Cli.Tests/RestoreByteAccumulatorTests.cssrc/Arius.Cli.Tests/RestoreTcsCoordinationTests.cssrc/Arius.Cli.Tests/SetRehydrationTests.cssrc/Arius.Cli.Tests/TestSupport/CliHarness.cssrc/Arius.Cli.Tests/TrackedDownloadLifecycleTests.cssrc/Arius.Cli.Tests/TrackedDownloadThreadSafetyTests.cssrc/Arius.Cli.Tests/TrackedFileBytesProcessedTests.cssrc/Arius.Cli.Tests/TrackedFileLargeFilePathTests.cssrc/Arius.Cli.Tests/TrackedFileSmallFilePathTests.cssrc/Arius.Cli.Tests/TrackedTarLifecycleTests.cssrc/Arius.Cli.Tests/TruncateAndLeftJustifyTests.cssrc/Arius.Core.Tests/ChunkIndex/ShardSerializerLocalTests.cssrc/Arius.Core.Tests/ChunkIndex/ShardSerializerTests.cssrc/Arius.Core.Tests/ChunkIndex/ShardTests.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/ChunkHydrationStatusQuery/ResolveFileHydrationStatusesHandlerTests.cssrc/Arius.Core.Tests/Features/ContainerNamesQuery/ContainerNamesQueryHandlerTests.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/PricingConfigTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCostCalculatorTests.cssrc/Arius.Core.Tests/FileTree/FileTreeBlobSerializerStorageTests.cssrc/Arius.Core.Tests/FileTree/FileTreeBlobSerializerTests.cssrc/Arius.Core.Tests/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/FileTree/FileTreeTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageHydrationStatusTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceReadTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceUploadTests.cssrc/Arius.Core.Tests/Shared/Encryption/AesGcmEncryptionTests.cssrc/Arius.Core.Tests/Shared/Encryption/GoldenFileDecryptionTests.cssrc/Arius.Core.Tests/Shared/Encryption/PassphraseEncryptionServiceTests.cssrc/Arius.Core.Tests/Shared/Encryption/PlaintextPassthroughServiceTests.cssrc/Arius.Core.Tests/Shared/LocalFile/LocalFileEnumeratorTests.cssrc/Arius.Core.Tests/Shared/RepositoryPathsCompatibilityTests.cssrc/Arius.Core.Tests/Shared/RepositoryPathsTests.cssrc/Arius.Core.Tests/Shared/Snapshot/SnapshotSerializerTests.cssrc/Arius.Core.Tests/Streaming/CountingStreamTests.cssrc/Arius.Core.Tests/Streaming/ProgressStreamTests.cssrc/Arius.Core.Tests/Streaming/StreamWrapperTests.cssrc/Arius.Integration.Tests/Shared/FileTree/FileTreeBuilderIntegrationTests.cs
💤 Files with no reviewable changes (4)
- src/Arius.Core.Tests/Streaming/StreamWrapperTests.cs
- src/Arius.Cli.Tests/ProgressTests.cs
- src/Arius.Cli.Tests/CliTests.cs
- src/Arius.Core.Tests/FileTree/FileTreeTests.cs
| @@ -0,0 +1,469 @@ | |||
| # ast-grep Rule Reference for C# | |||
|
|
|||
| This reference describes the main `ast-grep` rule building blocks using C# examples. | |||
Contributor
There was a problem hiding this comment.
Hyphenate these compound modifiers.
Use “rule-building blocks” on Line 3 and “log-level call” on Line 246.
Also applies to: 246-246
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...rence describes the main ast-grep rule building blocks using C# examples. The ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/ast-grep/references/rule_reference.md at line 3, Change the
unhyphenated compound modifiers to hyphenated forms: replace "rule building
blocks" with "rule-building blocks" (the phrase on Line 3) and replace "log
level call" with "log-level call" (the phrase around Line 246); update both
occurrences in the document so the compound adjectives are hyphenated
consistently.
Comment on lines
+417
to
+430
| ### Find methods annotated with `[Test]` | ||
|
|
||
| ```yaml | ||
| id: test-methods | ||
| language: csharp | ||
| rule: | ||
| all: | ||
| - kind: method_declaration | ||
| - has: | ||
| kind: attribute_list | ||
| stopBy: end | ||
| ``` | ||
|
|
||
| If you need the specific attribute name, test the concrete pattern with real code and add a narrower `has` sub-rule from the inspected AST. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ast-grep for C#, what rule pattern matches methods specifically annotated with [Test], and does has: { kind: attribute_list } match any attributed method rather than only [Test]?
💡 Result:
To match methods specifically annotated with the [Test] attribute in ast-grep for C#, use a rule that targets method declarations having an attribute_list child with the specific "Test" identifier. Example YAML rule: language: csharp rule: kind: method_declaration has: kind: attribute_list has: pattern: Test This matches methods like: [Test] public void MyTest { } A simpler but less precise version using regex on the attribute text: rule: kind: method_declaration has: kind: attribute regex: ^[Test]$ The pattern has: { kind: attribute_list } matches any method_declaration that has an attribute_list child (i.e., any method with any attributes), not specifically [Test]. To target only [Test], nest additional constraints inside the has rule as shown, or use regex/ pattern on the attribute name. Node kinds like method_declaration and attribute_list are standard from tree-sitter-c-sharp grammar, verifiable in ast-grep playground by pasting C# code with [Test] methods and inspecting the AST.
Citations:
- 1: https://ast-grep.github.io/reference/rule.html
- 2: https://github.com/tree-sitter/tree-sitter-c-sharp
- 3: https://ast-grep.github.io/reference/playground
- 4: https://ast-grep.github.io/guide/rule-config.html
- 5: https://ast-grep.github.io/guide/rule-config/relational-rule.html
- 6: https://github.com/emacs-csharp/csharp-mode/blob/master/csharp-tree-sitter.el
The example matches any attributed method, not specifically [Test] methods.
The rule has: { kind: attribute_list } matches any method_declaration with any attributes. To match only [Test], nest an additional constraint—e.g., has: pattern: Test inside the has rule—or use regex to target the attribute name. Either update the example to include this constraint or rename the section to reflect that it demonstrates matching any attributed method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/ast-grep/references/rule_reference.md around lines 417 - 430,
The example "Find methods annotated with [Test]" is too broad: the rule with id
"test-methods" currently matches any method_declaration that has an
attribute_list; update the rule to constrain the attribute_list to the Test
attribute by nesting a more specific `has`/`pattern` check inside the
attribute_list (e.g., require an attribute node or attribute name pattern
matching "Test"), or alternatively rename the section to "Find methods with any
attribute" if you intend to keep the generic rule; reference the rule id
"test-methods", node kinds "method_declaration" and "attribute_list", and add a
nested constraint that targets the attribute name "Test".
| description: Guide for writing ast-grep rules for C# structural code search and analysis. Use when users need to search C# codebases using AST patterns, find declarations or invocations by syntax shape, or perform structural queries that go beyond text search. | ||
| --- | ||
|
|
||
| # ast-grep for C# |
Contributor
There was a problem hiding this comment.
Fix markdownlint MD020 on the heading.
Line 6 triggers MD020 because C# is parsed like a closing hash in an ATX heading. Escape the hash (or reword) to keep lint clean.
Suggested fix
-# ast-grep for C#
+# ast-grep for C\#📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # ast-grep for C# | |
| # ast-grep for C\# |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 6-6: No space inside hashes on closed atx style heading
(MD020, no-missing-space-closed-atx)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/ast-grep/SKILL.md at line 6, The heading "# ast-grep for C#"
triggers markdownlint MD020 because the trailing "#" is parsed as a closing
hash; update the heading in SKILL.md by escaping the hash (e.g., "ast-grep for
C\#") or rewording it (e.g., "ast-grep for C Sharp") so the ATX heading no
longer contains an unescaped trailing hash and the MD020 warning is resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs (1)
14-14: Preferinternaltest classes unless external visibility is required.Line 14 uses
public class BuildArchiveDisplayTests; repository rule prefersinternal.Proposed fix
-public class BuildArchiveDisplayTests +internal class BuildArchiveDisplayTestsDoes TUnit require test classes to be public for discovery, or are internal test classes fully supported?As per coding guidelines,
**/*.cs: "Make classesinternal. Only make thempublicwhen they need to be visible outside of the assembly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs` at line 14, The test class BuildArchiveDisplayTests is declared public but should be internal per repository rules; change the class declaration for BuildArchiveDisplayTests from public to internal (keeping the same name and members) so it is not exported from the assembly unless external visibility is needed, then run the test discovery to verify the test framework (TUnit) still finds the tests.src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs (2)
10-10: Test class should beinternal.As per coding guidelines, classes should be
internalunless they need to be visible outside the assembly.♻️ Suggested fix
-public class FileTreeBuilderTests +internal class FileTreeBuilderTests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs` at line 10, The test class declaration public class FileTreeBuilderTests should be made internal to follow the guideline that test classes are internal by default; locate the FileTreeBuilderTests class declaration and change its accessibility from public to internal (i.e., replace "public class FileTreeBuilderTests" with "internal class FileTreeBuilderTests").
150-181: Good deduplication test, but cleanup could miss cache directory on test failure.The
finallyblock (lines 174-180) cleans up after retrievingcacheDiragain. If the test fails beforebuilder.BuildAsynccompletes the first time, the cache directory from the first builder might not be cleaned. Consider extractingcacheDirbefore the try block.♻️ Suggested improvement
[Test] public async Task BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk() { var manifestPath = Path.GetTempFileName(); + var cacheDir = FileTreeService.GetDiskCacheDirectory("acc", "con"); try { // ... test body unchanged ... } finally { File.Delete(manifestPath); - var cacheDir = FileTreeService.GetDiskCacheDirectory("acc", "con"); if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs` around lines 150 - 181, The finally block may miss deleting the disk cache if the test fails early; in the BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk test (class FileTreeBuilderTests, test method BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk) compute and store the cacheDir (via FileTreeService.GetDiskCacheDirectory("acc","con")) before the try block (or otherwise capture both potential cache dirs) and then use that stored value in the finally to remove the directory, ensuring the cache is always cleaned even if the test fails before builder.BuildAsync runs.src/Arius.Core.Tests/FileTree/FileTreeServiceTests.cs (1)
17-17: Test class should beinternal.As per coding guidelines, classes should be
internalunless they need to be visible outside the assembly.♻️ Suggested fix
-public class FileTreeServiceTests +internal class FileTreeServiceTests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/FileTree/FileTreeServiceTests.cs` at line 17, The test class FileTreeServiceTests is declared public but should be internal per project guidelines; change the class declaration from "public class FileTreeServiceTests" to "internal class FileTreeServiceTests" so its visibility is limited to the assembly while keeping NUnit/xUnit test discovery intact.src/Arius.Core/Shared/FileTree/FileTreeService.cs (2)
219-223: Consider using atomic writes inWriteAsyncfor consistency.
WriteAsync(line 222) uses non-atomicFile.WriteAllBytesAsync, whileDownloadAndCacheAsync(line 154) usesWriteCacheAtomicallyAsync. AlthoughWriteAsyncis typically called in single-threaded archive flows, using atomic writes would provide consistent safety guarantees.♻️ Suggested refactor
// Write plaintext to disk cache regardless of whether upload was new or existing. var diskPath = Path.Combine(_diskCacheDir, hash); var plaintext = FileTreeBlobSerializer.Serialize(tree); - await File.WriteAllBytesAsync(diskPath, plaintext, cancellationToken); + await WriteCacheAtomicallyAsync(diskPath, plaintext, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Shared/FileTree/FileTreeService.cs` around lines 219 - 223, WriteAsync currently writes cache with File.WriteAllBytesAsync which is non-atomic; change it to use the same atomic write approach as DownloadAndCacheAsync by invoking WriteCacheAtomicallyAsync (or the existing helper used there) to write the serialized bytes from FileTreeBlobSerializer.Serialize to the disk cache path (constructed from _diskCacheDir and hash), passing the plaintext bytes and cancellationToken so the cache write is atomic and consistent with DownloadAndCacheAsync.
170-173: Minor TOCTOU race betweenFile.ExistsandFile.Replace.If the file is deleted between the
Existscheck andReplacecall,File.Replacewill throw. Consider wrapping in a try-catch to fall back toFile.Move:🛡️ Defensive fix
- if (OperatingSystem.IsWindows() && File.Exists(diskPath)) - File.Replace(tempPath, diskPath, destinationBackupFileName: null, ignoreMetadataErrors: true); - else - File.Move(tempPath, diskPath, overwrite: true); + try + { + if (OperatingSystem.IsWindows() && File.Exists(diskPath)) + File.Replace(tempPath, diskPath, destinationBackupFileName: null, ignoreMetadataErrors: true); + else + File.Move(tempPath, diskPath, overwrite: true); + } + catch (FileNotFoundException) + { + // Destination was deleted between Exists check and Replace call. + // Fall back to Move which works when destination doesn't exist. + File.Move(tempPath, diskPath, overwrite: true); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Shared/FileTree/FileTreeService.cs` around lines 170 - 173, There is a TOCTOU race where File.Exists(diskPath) may be true but the file deleted before File.Replace is called; update the Windows branch inside FileTreeService (the block using OperatingSystem.IsWindows()) to catch exceptions from File.Replace (e.g., FileNotFoundException/IOException) and on failure fall back to calling File.Move(tempPath, diskPath, overwrite: true); ensure you only swallow the specific replace-related exceptions and rethrow unexpected exceptions so other errors are not hidden.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs`:
- Around line 272-281: The test
BuildArchiveDisplay_TruncatesLongRelativePath_WithEllipsisPrefix uses
output.ShouldContain("...") which can match the header "files..." and produce a
false positive; change the assertion to verify the ellipsis appears in the
truncated path itself by asserting output contains a unique suffix of the long
path with an ellipsis prefix (for example ".../file.bin" or ".../with/file.bin")
and keep the output.ShouldNotContain(longPath) check; locate this in the test
method BuildArchiveDisplay_TruncatesLongRelativePath_WithEllipsisPrefix where
longPath, ProgressState.AddFile, RenderToString(ArchiveVerb.BuildDisplay(state))
and output are used and replace the generic "..."/ShouldContain assertion with a
check for the ellipsis plus a path tail (e.g.,
output.ShouldContain(".../file.bin")).
- Around line 242-257: The test BuildArchiveDisplay_ShowsTarLine_WhenUploading
currently asserts output.ShouldContain("Uploading") which can match the stage
header instead of the TAR row; update the assertion to locate the TAR row (use
the rendered output from RenderToString(ArchiveVerb.BuildDisplay(state)) and
find the line containing "TAR `#1`") and assert that that specific line/string
segment contains "Uploading" (i.e. replace the global
output.ShouldContain("Uploading") with an assertion that the "TAR `#1`" row text
includes "Uploading" so the TAR row state is verified).
In `@src/Arius.Core.Tests/Shared/Streaming/CountingStreamTests.cs`:
- Around line 6-9: Change the test class visibility for CountingStreamTests from
public to internal so it follows project guidelines; update the class
declaration for CountingStreamTests (the class containing the
Write_TracksBytesWritten test) to use the internal modifier instead of public.
In `@src/Arius.Core/Shared/FileTree/FileTreeService.cs`:
- Around line 113-145: The method TryReadCachedTree is synchronously reading the
cache but uses File.ReadAllBytesAsync(...).GetAwaiter().GetResult(), which can
deadlock; change the read to the synchronous File.ReadAllBytes(diskPath) call,
preserve the existing cancellation behavior by removing the CancellationToken
usage for this sync path, keep the same null/empty checks and exception
handling, and ensure references to FileTreeBlobSerializer.Deserialize(cached)
and the File.Delete(diskPath) cleanup remain unchanged.
---
Nitpick comments:
In `@src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs`:
- Line 14: The test class BuildArchiveDisplayTests is declared public but should
be internal per repository rules; change the class declaration for
BuildArchiveDisplayTests from public to internal (keeping the same name and
members) so it is not exported from the assembly unless external visibility is
needed, then run the test discovery to verify the test framework (TUnit) still
finds the tests.
In `@src/Arius.Core.Tests/FileTree/FileTreeServiceTests.cs`:
- Line 17: The test class FileTreeServiceTests is declared public but should be
internal per project guidelines; change the class declaration from "public class
FileTreeServiceTests" to "internal class FileTreeServiceTests" so its visibility
is limited to the assembly while keeping NUnit/xUnit test discovery intact.
In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs`:
- Line 10: The test class declaration public class FileTreeBuilderTests should
be made internal to follow the guideline that test classes are internal by
default; locate the FileTreeBuilderTests class declaration and change its
accessibility from public to internal (i.e., replace "public class
FileTreeBuilderTests" with "internal class FileTreeBuilderTests").
- Around line 150-181: The finally block may miss deleting the disk cache if the
test fails early; in the BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk test
(class FileTreeBuilderTests, test method
BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk) compute and store the cacheDir
(via FileTreeService.GetDiskCacheDirectory("acc","con")) before the try block
(or otherwise capture both potential cache dirs) and then use that stored value
in the finally to remove the directory, ensuring the cache is always cleaned
even if the test fails before builder.BuildAsync runs.
In `@src/Arius.Core/Shared/FileTree/FileTreeService.cs`:
- Around line 219-223: WriteAsync currently writes cache with
File.WriteAllBytesAsync which is non-atomic; change it to use the same atomic
write approach as DownloadAndCacheAsync by invoking WriteCacheAtomicallyAsync
(or the existing helper used there) to write the serialized bytes from
FileTreeBlobSerializer.Serialize to the disk cache path (constructed from
_diskCacheDir and hash), passing the plaintext bytes and cancellationToken so
the cache write is atomic and consistent with DownloadAndCacheAsync.
- Around line 170-173: There is a TOCTOU race where File.Exists(diskPath) may be
true but the file deleted before File.Replace is called; update the Windows
branch inside FileTreeService (the block using OperatingSystem.IsWindows()) to
catch exceptions from File.Replace (e.g., FileNotFoundException/IOException) and
on failure fall back to calling File.Move(tempPath, diskPath, overwrite: true);
ensure you only swallow the specific replace-related exceptions and rethrow
unexpected exceptions so other errors are not hidden.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d4cbaa6-6daa-4446-91af-83976d628f15
📒 Files selected for processing (9)
src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cssrc/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayActiveDownloadsTests.cssrc/Arius.Core.Tests/Fakes/FakeInMemoryBlobContainerService.cssrc/Arius.Core.Tests/FileTree/FileTreeBlobSerializerStorageTests.cssrc/Arius.Core.Tests/FileTree/FileTreeServiceTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/Shared/Streaming/CountingStreamTests.cssrc/Arius.Core.Tests/Streaming/ProgressStreamTests.cssrc/Arius.Core/Shared/FileTree/FileTreeService.cs
✅ Files skipped from review due to trivial changes (1)
- src/Arius.Cli.Tests/Commands/Restore/BuildRestoreDisplayActiveDownloadsTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Arius.Core.Tests/Streaming/ProgressStreamTests.cs
- src/Arius.Core.Tests/FileTree/FileTreeBlobSerializerStorageTests.cs
Comment on lines
+6
to
+9
| public class CountingStreamTests | ||
| { | ||
| [Test] | ||
| public void Write_TracksBytesWritten() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Framework package/usings check =="
rg -n -C2 --type=xml 'PackageReference Include="(NUnit|xunit|TUnit)' src/Arius.Core.Tests/Arius.Core.Tests.csproj Directory.Packages.props Directory.Build.props 2>/dev/null || true
rg -n -C2 --type=cs 'global using (NUnit|TUnit)|using (NUnit|TUnit)' src/Arius.Core.Tests 2>/dev/null || true
echo
echo "== Attribute and class visibility check in tests =="
rg -n -C1 --type=cs '\[Test\]|\[Fact\]|public class .*Tests|internal class .*Tests' src/Arius.Core.TestsRepository: woutervanranst/Arius7
Length of output: 50377
Change test class visibility to internal.
Line 6 declares public class CountingStreamTests, but coding guidelines require test classes to be internal unless they need visibility outside the assembly. The test framework (TUnit with [Test] attributes) is correctly configured; update the class visibility modifier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Core.Tests/Shared/Streaming/CountingStreamTests.cs` around lines 6
- 9, Change the test class visibility for CountingStreamTests from public to
internal so it follows project guidelines; update the class declaration for
CountingStreamTests (the class containing the Write_TracksBytesWritten test) to
use the internal modifier instead of public.
… increments the upload counter. I’m changing just that test to throwAfterN: 1, then I’ll run the focused FaultingBlobServiceTests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs (1)
40-40: Use an isolated temporary directory instead of the shared temp root.Using
Path.GetTempPath()directly can couple this test to shared machine state if restore behavior expands later. Prefer a unique per-test path.♻️ Suggested hardening
- var result = await handler.Handle( - new RestoreCommandMessage(new RestoreOptions { RootDirectory = Path.GetTempPath() }), - CancellationToken.None); + var uniqueRoot = Path.Combine(Path.GetTempPath(), $"arius-restore-test-{Guid.NewGuid():N}"); + var result = await handler.Handle( + new RestoreCommandMessage(new RestoreOptions { RootDirectory = uniqueRoot }), + CancellationToken.None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs` at line 40, The test currently constructs RestoreCommandMessage with RestoreOptions.RootDirectory = Path.GetTempPath(), which uses a shared temp root; change the test to create a unique per-test temporary directory (e.g., combine Path.GetTempPath() with a GUID or use the testing framework's temp folder helper and call Directory.CreateDirectory) and pass that path into new RestoreCommandMessage(new RestoreOptions { RootDirectory = uniqueTempDir }), and ensure the test deletes or disposes that directory after the test completes.src/Arius.Core.Tests/Shared/ChunkStorage/BlockingDeleteBlobContainerService.cs (2)
46-46: Add cancellation-aware waiting to avoid potentially hanging tests.
WaitForConcurrentDeletesAsync()has no bounded wait surface. Expose aCancellationTokenoverload so callers can fail fast when concurrency is not observed.As per coding guidelines: "Test projects. Focus on test coverage gaps and assertion quality rather than style."Proposed diff
- public Task WaitForConcurrentDeletesAsync() => _sawConcurrentDeletes.Task; + public Task WaitForConcurrentDeletesAsync(CancellationToken cancellationToken = default) + => _sawConcurrentDeletes.Task.WaitAsync(cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/ChunkStorage/BlockingDeleteBlobContainerService.cs` at line 46, WaitForConcurrentDeletesAsync currently returns an unbounded Task and can hang tests; add an overload Task WaitForConcurrentDeletesAsync(CancellationToken cancellationToken) that respects cancellation by wiring the token to the existing TaskCompletionSource (_sawConcurrentDeletes) via cancellation registration or by awaiting Task.WhenAny(_sawConcurrentDeletes.Task, cancellation task) and throwing OperationCanceledException when the token fires; keep the original parameterless method for backward compatibility and implement it to call the new overload with CancellationToken.None.
16-22: UseBlobPaths.ChunkRehydrated(...)consistently in metadata lookup.
GetMetadataAsynchard-codes blob keys whileListAsyncbuilds them viaBlobPaths. Keeping both paths centralized avoids brittle test drift if naming changes.Proposed diff
internal sealed class BlockingDeleteBlobContainerService : IBlobContainerService { + private static readonly string RehydratedA = BlobPaths.ChunkRehydrated("a"); + private static readonly string RehydratedB = BlobPaths.ChunkRehydrated("b"); + private readonly TaskCompletionSource _sawConcurrentDeletes = new(TaskCreationOptions.RunContinuationsAsynchronously); private readonly TaskCompletionSource _releaseDeletes = new(TaskCreationOptions.RunContinuationsAsynchronously); private int _activeDeletes; @@ public Task<BlobMetadata> GetMetadataAsync(string blobName, CancellationToken cancellationToken = default) => Task.FromResult(blobName switch { - "chunks-rehydrated/a" => new BlobMetadata { Exists = true, ContentLength = 3 }, - "chunks-rehydrated/b" => new BlobMetadata { Exists = true, ContentLength = 4 }, + var name when name == RehydratedA => new BlobMetadata { Exists = true, ContentLength = 3 }, + var name when name == RehydratedB => new BlobMetadata { Exists = true, ContentLength = 4 }, _ => new BlobMetadata { Exists = false } }); @@ private async IAsyncEnumerable<string> AsyncEnumerable() { - yield return BlobPaths.ChunkRehydrated("a"); - yield return BlobPaths.ChunkRehydrated("b"); + yield return RehydratedA; + yield return RehydratedB; await Task.CompletedTask; } }Also applies to: 50-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/ChunkStorage/BlockingDeleteBlobContainerService.cs` around lines 16 - 22, GetMetadataAsync currently hard-codes blob keys ("chunks-rehydrated/a", "chunks-rehydrated/b") causing divergence from ListAsync which uses BlobPaths.ChunkRehydrated(...); update GetMetadataAsync to call BlobPaths.ChunkRehydrated("a") and BlobPaths.ChunkRehydrated("b") (or the same identifiers used in ListAsync) when building the switch keys so both metadata lookup and listing use the centralized BlobPaths.ChunkRehydrated method; ensure the BlobMetadata results remain identical and apply the same change for the other similar block mentioned (lines 50-53).src/Arius.E2E.Tests/E2EFixturePathTests.cs (1)
17-34: Add a Windows-style traversal test case ("..\\escape.bin").Current tests validate
../traversal, but an explicit backslash traversal case will protect cross-platform behavior.💡 Suggested test addition
+ [Test] + public void CombineValidatedRelativePath_RejectsBackslashDotDotTraversal() + { + var root = Path.Combine(Path.GetTempPath(), $"arius-e2e-path-{Guid.NewGuid():N}"); + + Should.Throw<ArgumentException>(() => E2EFixture.CombineValidatedRelativePath(root, "..\\escape.bin")); + }As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/E2EFixturePathTests.cs` around lines 17 - 34, Add a Windows-style traversal test to ensure CombineValidatedRelativePath rejects backslash traversal: create a new test method (e.g., CombineValidatedRelativePath_RejectsBackslashTraversal) in E2EFixturePathTests.cs that builds a temp root like the other tests and asserts Should.Throw<ArgumentException>(() => E2EFixture.CombineValidatedRelativePath(root, "..\\escape.bin")) (or use a verbatim string @"..\escape.bin") to verify backslash-based "..\.." escape is rejected by E2EFixture.CombineValidatedRelativePath.src/Arius.E2E.Tests/E2EFixture.cs (1)
176-188: Reject empty/whitespace relative paths explicitly.
relativePathvalues like""or" "currently pass validation and can fail later with less clear IO exceptions.💡 Suggested guard
internal static string CombineValidatedRelativePath(string rootPath, string relativePath) { + if (string.IsNullOrWhiteSpace(relativePath)) + throw new ArgumentException("Path must be a non-empty relative path.", nameof(relativePath)); + // These helpers should only touch files under the fixture roots; rejecting rooted // and parent-traversal inputs keeps accidental path escapes out of test code. if (Path.IsPathRooted(relativePath)) throw new ArgumentException($"Path '{relativePath}' must be relative.", nameof(relativePath));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/E2EFixture.cs` around lines 176 - 188, The CombineValidatedRelativePath method should explicitly reject null/empty/whitespace relativePath inputs to avoid opaque IO errors later; add a guard at the start of CombineValidatedRelativePath that checks string.IsNullOrWhiteSpace(relativePath) and throws an ArgumentException (using nameof(relativePath)) with a clear message like "Path '{relativePath}' must be a non-empty relative path." Place this check before the existing Path.IsPathRooted and subsequent validations so blank inputs are rejected immediately.src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs (2)
102-111: Prefer a local function forTryDeleteDirectory.
TryDeleteDirectoryis only used insideDispose, so keeping it local reduces class-level surface.♻️ Proposed refactor
public void Dispose() { + static void TryDeleteDirectory(string path) + { + try + { + Directory.Delete(path, recursive: true); + } + catch (DirectoryNotFoundException) + { + } + } + if (Directory.Exists(_rootDirectory)) Directory.Delete(_rootDirectory, recursive: true); var chunkIndexDir = RepositoryPaths.GetChunkIndexCacheDirectory(AccountName, _containerName); if (Directory.Exists(chunkIndexDir)) TryDeleteDirectory(chunkIndexDir); @@ var snapshotCacheDir = SnapshotService.GetDiskCacheDirectory(AccountName, _containerName); if (Directory.Exists(snapshotCacheDir)) TryDeleteDirectory(snapshotCacheDir); } - -private static void TryDeleteDirectory(string path) -{ - try - { - Directory.Delete(path, recursive: true); - } - catch (DirectoryNotFoundException) - { - } -}As per coding guidelines: "
**/*.cs: Prefer local methods over private static methods for helper functionality that is only used within a single method".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs` around lines 102 - 111, Move the private static helper TryDeleteDirectory into Dispose as a local function since it's only used there: open the Dispose method, create a local method named TryDeleteDirectory inside it that calls Directory.Delete(path, recursive: true) and catches DirectoryNotFoundException, then remove the private static TryDeleteDirectory member from the class; reference the existing TryDeleteDirectory identifier and the Dispose method to locate and perform the change.
81-82: Consider makingLookupasynchronous.Line 82 uses sync-over-async, which can be problematic. Since both call sites in
ArchiveRecoveryTests.csare already in async test methods, exposing an async method would be more straightforward.♻️ Proposed refactor
-public ShardEntry? Lookup(string contentHash) - => _index.LookupAsync(contentHash).GetAwaiter().GetResult(); +public Task<ShardEntry?> LookupAsync(string contentHash) + => _index.LookupAsync(contentHash);Update call sites at ArchiveRecoveryTests.cs:25 and :44 to use
await env.LookupAsync(contentHash).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs` around lines 81 - 82, The Lookup method is doing sync-over-async; change its signature to be asynchronous (e.g., Task<ShardEntry?> LookupAsync(string contentHash)) and forward to _index.LookupAsync(contentHash) with await, then update callers in ArchiveRecoveryTests.cs (the two places mentioned) to call await env.LookupAsync(contentHash) instead of the synchronous Lookup; ensure to remove GetAwaiter().GetResult() and update using statements if needed for Task/async.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs`:
- Around line 40-47: The WriteRandomFile method accepts a relativePath that can
contain "../" and escape _rootDirectory; ensure you canonicalize the target with
Path.GetFullPath after combining _rootDirectory and relativePath and verify the
resulting fullPath starts with the canonical _rootDirectory (or throw/normalize)
before creating directories or writing bytes; update WriteRandomFile to compute
rootFull = Path.GetFullPath(_rootDirectory) and fullPath =
Path.GetFullPath(Path.Combine(_rootDirectory, relativePath)), check
fullPath.StartsWith(rootFull, StringComparison.OrdinalIgnoreCase) (or use a safe
path containment helper) and reject or normalize paths that escape the test
root, then proceed to create the directory and write the file.
---
Nitpick comments:
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs`:
- Around line 102-111: Move the private static helper TryDeleteDirectory into
Dispose as a local function since it's only used there: open the Dispose method,
create a local method named TryDeleteDirectory inside it that calls
Directory.Delete(path, recursive: true) and catches DirectoryNotFoundException,
then remove the private static TryDeleteDirectory member from the class;
reference the existing TryDeleteDirectory identifier and the Dispose method to
locate and perform the change.
- Around line 81-82: The Lookup method is doing sync-over-async; change its
signature to be asynchronous (e.g., Task<ShardEntry?> LookupAsync(string
contentHash)) and forward to _index.LookupAsync(contentHash) with await, then
update callers in ArchiveRecoveryTests.cs (the two places mentioned) to call
await env.LookupAsync(contentHash) instead of the synchronous Lookup; ensure to
remove GetAwaiter().GetResult() and update using statements if needed for
Task/async.
In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`:
- Line 40: The test currently constructs RestoreCommandMessage with
RestoreOptions.RootDirectory = Path.GetTempPath(), which uses a shared temp
root; change the test to create a unique per-test temporary directory (e.g.,
combine Path.GetTempPath() with a GUID or use the testing framework's temp
folder helper and call Directory.CreateDirectory) and pass that path into new
RestoreCommandMessage(new RestoreOptions { RootDirectory = uniqueTempDir }), and
ensure the test deletes or disposes that directory after the test completes.
In
`@src/Arius.Core.Tests/Shared/ChunkStorage/BlockingDeleteBlobContainerService.cs`:
- Line 46: WaitForConcurrentDeletesAsync currently returns an unbounded Task and
can hang tests; add an overload Task
WaitForConcurrentDeletesAsync(CancellationToken cancellationToken) that respects
cancellation by wiring the token to the existing TaskCompletionSource
(_sawConcurrentDeletes) via cancellation registration or by awaiting
Task.WhenAny(_sawConcurrentDeletes.Task, cancellation task) and throwing
OperationCanceledException when the token fires; keep the original parameterless
method for backward compatibility and implement it to call the new overload with
CancellationToken.None.
- Around line 16-22: GetMetadataAsync currently hard-codes blob keys
("chunks-rehydrated/a", "chunks-rehydrated/b") causing divergence from ListAsync
which uses BlobPaths.ChunkRehydrated(...); update GetMetadataAsync to call
BlobPaths.ChunkRehydrated("a") and BlobPaths.ChunkRehydrated("b") (or the same
identifiers used in ListAsync) when building the switch keys so both metadata
lookup and listing use the centralized BlobPaths.ChunkRehydrated method; ensure
the BlobMetadata results remain identical and apply the same change for the
other similar block mentioned (lines 50-53).
In `@src/Arius.E2E.Tests/E2EFixture.cs`:
- Around line 176-188: The CombineValidatedRelativePath method should explicitly
reject null/empty/whitespace relativePath inputs to avoid opaque IO errors
later; add a guard at the start of CombineValidatedRelativePath that checks
string.IsNullOrWhiteSpace(relativePath) and throws an ArgumentException (using
nameof(relativePath)) with a clear message like "Path '{relativePath}' must be a
non-empty relative path." Place this check before the existing Path.IsPathRooted
and subsequent validations so blank inputs are rejected immediately.
In `@src/Arius.E2E.Tests/E2EFixturePathTests.cs`:
- Around line 17-34: Add a Windows-style traversal test to ensure
CombineValidatedRelativePath rejects backslash traversal: create a new test
method (e.g., CombineValidatedRelativePath_RejectsBackslashTraversal) in
E2EFixturePathTests.cs that builds a temp root like the other tests and asserts
Should.Throw<ArgumentException>(() =>
E2EFixture.CombineValidatedRelativePath(root, "..\\escape.bin")) (or use a
verbatim string @"..\escape.bin") to verify backslash-based "..\.." escape is
rejected by E2EFixture.CombineValidatedRelativePath.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c07011ac-74a8-42e2-9d70-64063ae6a0c8
📒 Files selected for processing (23)
AGENTS.mdREADME.mdsrc/Arius.Cli.Tests/AccountResolutionTests.cssrc/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cssrc/Arius.Cli.Tests/Commands/Archive/ProgressCallbackIntegrationTests.cssrc/Arius.Core.Tests/Arius.Core.Tests.csprojsrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/ChunkHydrationStatusQuery/ResolveFileHydrationStatusesHandlerTests.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/BlockingDeleteBlobContainerService.cssrc/Arius.Core.Tests/Shared/ChunkStorage/NonSeekableReadStream.cssrc/Arius.E2E.Tests/Arius.E2E.Tests.csprojsrc/Arius.E2E.Tests/E2EFixture.cssrc/Arius.E2E.Tests/E2EFixturePathTests.cssrc/Arius.E2E.Tests/RehydrationE2ETests.cssrc/Arius.Integration.Tests/Arius.Integration.Tests.csprojsrc/Arius.Integration.Tests/Pipeline/ContainerCreationTests.cssrc/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cssrc/Arius.Integration.Tests/Pipeline/FaultingBlobServiceTests.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cssrc/Directory.Packages.props
✅ Files skipped from review due to trivial changes (5)
- src/Arius.E2E.Tests/Arius.E2E.Tests.csproj
- src/Directory.Packages.props
- src/Arius.Integration.Tests/Arius.Integration.Tests.csproj
- src/Arius.Core.Tests/Arius.Core.Tests.csproj
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Arius.Core.Tests/Features/ChunkHydrationStatusQuery/ResolveFileHydrationStatusesHandlerTests.cs
- src/Arius.Core.Tests/Shared/ChunkStorage/NonSeekableReadStream.cs
- src/Arius.Cli.Tests/Commands/Archive/ProgressCallbackIntegrationTests.cs
- src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs
- src/Arius.Integration.Tests/Pipeline/FaultingBlobServiceTests.cs
- src/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cs
Comment on lines
+40
to
+47
| public byte[] WriteRandomFile(string relativePath, int sizeBytes) | ||
| { | ||
| var content = new byte[sizeBytes]; | ||
| Random.Shared.NextBytes(content); | ||
| var fullPath = Path.Combine(_rootDirectory, relativePath); | ||
| Directory.CreateDirectory(Path.GetDirectoryName(fullPath)!); | ||
| File.WriteAllBytes(fullPath, content); | ||
| return content; |
Contributor
There was a problem hiding this comment.
Constrain relativePath to stay inside the test root.
Line 44 can be escaped with .. segments, which may write outside _rootDirectory and leave artifacts behind after cleanup.
🛠️ Proposed fix
public byte[] WriteRandomFile(string relativePath, int sizeBytes)
{
+ var rootFullPath = Path.GetFullPath(_rootDirectory);
+ var fullPath = Path.GetFullPath(Path.Combine(rootFullPath, relativePath));
+ if (!fullPath.StartsWith(rootFullPath + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase))
+ throw new ArgumentException("Path must stay within the test root.", nameof(relativePath));
+
var content = new byte[sizeBytes];
Random.Shared.NextBytes(content);
- var fullPath = Path.Combine(_rootDirectory, relativePath);
Directory.CreateDirectory(Path.GetDirectoryName(fullPath)!);
File.WriteAllBytes(fullPath, content);
return content;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs`
around lines 40 - 47, The WriteRandomFile method accepts a relativePath that can
contain "../" and escape _rootDirectory; ensure you canonicalize the target with
Path.GetFullPath after combining _rootDirectory and relativePath and verify the
resulting fullPath starts with the canonical _rootDirectory (or throw/normalize)
before creating directories or writing bytes; update WriteRandomFile to compute
rootFull = Path.GetFullPath(_rootDirectory) and fullPath =
Path.GetFullPath(Path.Combine(_rootDirectory, relativePath)), check
fullPath.StartsWith(rootFull, StringComparison.OrdinalIgnoreCase) (or use a safe
path containment helper) and reject or normalize paths that escape the test
root, then proceed to create the directory and write the file.
This was referenced Apr 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Features/andShared/and splitting the large CLI progress test monolith into per-class filesPipeline/ast-grepworkflow and the mirrored test-structure rules in agent and project docsValidation
dotnet test --project src/Arius.Architecture.Tests/Arius.Architecture.Tests.csprojdotnet test --project src/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csprojdotnet test --project src/Arius.Core.Tests/Arius.Core.Tests.csprojdotnet test --project src/Arius.Integration.Tests/Arius.Integration.Tests.csprojsource ~/.zshrc && dotnet test --project src/Arius.E2E.Tests/Arius.E2E.Tests.csprojNotes
Arius.Explorer.Testswas intentionally skipped because it targetsnet10.0-windowsArius.Core.Tests.FileTree.FileTreeServiceTests.ReadAsync_ConcurrentReads_NoDiskCorruption_AtMostOneAzureDownloadremained intermittently flaky during intermediate runs but the final sequential runnable suite passed greenSummary by CodeRabbit
Documentation
Bug Fixes
Tests
Chores