refactor: replace strings with strong Hash types, subclass FileTree into File/Directory#69
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces raw hex string hashes with three typed value objects (ContentHash, ChunkHash, FileTreeHash) across models, events, services, serializers, CLI progress state, and tests; splits file-tree entries into FileEntry and DirectoryEntry; adds HashCodec and test helpers; updates many public/internal signatures and test fixtures. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69 +/- ##
==========================================
- Coverage 79.20% 78.81% -0.39%
==========================================
Files 69 74 +5
Lines 4986 5084 +98
Branches 671 695 +24
==========================================
+ Hits 3949 4007 +58
- Misses 881 909 +28
- Partials 156 168 +12
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:
|
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs (1)
405-420:⚠️ Potential issue | 🟡 MinorUse chunk-level compressed bytes here, not the first file's share.
For tar chunks,
ShardEntry.CompressedSizeis a proportional per-file share. This loop adds only the first file's share, sototalRehydrateBytesis underreported for multi-file tar chunks.Suggested fix
foreach (var chunkHash in chunksToRequest) { try { await _chunkStorage.StartRehydrationAsync(chunkHash, rehydratePriority, cancellationToken); - if (TryGetIndexEntry(filesByChunkHash[chunkHash][0], indexEntries, out var ie)) - totalRehydrateBytes += ie.CompressedSize; + totalRehydrateBytes += SumCompressedBytes(chunkHash); } catch (Exception ex) { _logger.LogWarning(ex, "Failed to start rehydration for chunk {ChunkHash}", chunkHash); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs` around lines 405 - 420, The loop currently adds only the first file's ShardEntry.CompressedSize (via TryGetIndexEntry(filesByChunkHash[chunkHash][0], ...)), underreporting multi-file tar chunk sizes; change the logic in the foreach over chunksToRequest so totalRehydrateBytes uses the actual chunk-level compressed bytes—either by looking up the chunk's index entry by chunkHash (rather than the first file) or by summing the ShardEntry.CompressedSize for all files in filesByChunkHash[chunkHash]—before publishing RehydrationStartedEvent; update references to TryGetIndexEntry, filesByChunkHash, indexEntries, totalRehydrateBytes and chunksToRehydrate accordingly.
🧹 Nitpick comments (10)
src/Arius.Core.Tests/Shared/Hashes/FileTreeHashTests.cs (1)
8-21: Consider adding invalid-input parse tests forFileTreeHash.This suite currently verifies normalization and
Short8, but not rejection paths (non-hex or wrong length). Adding those would lock parser invariants the same way as other hash types.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.Core.Tests/Shared/Hashes/FileTreeHashTests.cs` around lines 8 - 21, Add negative tests for FileTreeHash.Parse to assert it throws on invalid input: one test (e.g., Parse_Throws_OnNonHex) should call FileTreeHash.Parse with a string containing non-hex characters and assert an exception (ArgumentException/FormatException as used elsewhere) is thrown; another test (e.g., Parse_Throws_OnWrongLength) should pass a hex string of incorrect length (too short and/or too long) and assert the same exception. Place tests alongside the existing Parse_NormalizesUppercaseHexToCanonicalLowercase and Short8 tests and reference FileTreeHash.Parse and the expected exception type used by other hash parse tests to ensure consistency.src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs (1)
12-13: MoveTree(char c)into the test method as a local helper.
Tree(char c)is only used bySnapshotResolvedHandler_SetsTimestampAndRootHash, so keeping it class-level adds unnecessary surface. Per coding guidelines, 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.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs` around lines 12 - 13, Move the private static helper FileTreeHash Tree(char c) into the SnapshotResolvedHandler_SetsTimestampAndRootHash test as a local function and remove the class-level Tree method; update the test to call the local Tree(char) helper (ensuring it uses FileTreeHash.Parse(new string(c, 64))) so the helper scope is limited to the test and the class no longer contains the unused static method.src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs (1)
229-238: MoveCompressAsyncto a local method inside the single test that uses it.On Line 229, this helper is single-use; keeping it local reduces surface area and keeps the test self-contained.
As per coding guidelines "`**/*.cs`: Prefer local methods over private static methods for helper functionality that is only used within a single method."♻️ Proposed refactor
public async Task Handle_InvalidSnapshotContentHash_SkipsFileInsteadOfThrowing() { + static async Task<byte[]> CompressAsync(byte[] plaintext) + { + using var output = new MemoryStream(); + await using (var gzip = new System.IO.Compression.GZipStream( + output, + System.IO.Compression.CompressionLevel.Optimal, + leaveOpen: true)) + { + await gzip.WriteAsync(plaintext); + } + return output.ToArray(); + } + // existing test body... } - -private static async Task<byte[]> CompressAsync(byte[] plaintext) -{ - using var output = new MemoryStream(); - await using (var gzip = new System.IO.Compression.GZipStream(output, System.IO.Compression.CompressionLevel.Optimal, leaveOpen: true)) - { - await gzip.WriteAsync(plaintext); - } - - return output.ToArray(); -}🤖 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` around lines 229 - 238, The private static helper CompressAsync is only used by one test in RestoreCommandHandlerTests; move it into that test as a local async method (remove the private static declaration), update the test to call the local CompressAsync, and delete the now-unused private static CompressAsync from the class so the helper is scoped to the single test method.src/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cs (1)
42-43: StrengthenNormalizeHashto avoid lossy hash collapsing and empty-input crashes.On Line 43, multiple different seeds can normalize to the same value, and empty input will throw at
hash[0]. That weakens hash-specific test coverage.As per coding guidelines "`**/*.Tests/**`: Test projects. Focus on test coverage gaps and assertion quality rather than style."♻️ Proposed fix
private static string NormalizeHash(string hash) - => hash.Length == 64 ? hash : hash[0].ToString().PadRight(64, char.ToLowerInvariant(hash[0])); +{ + ArgumentException.ThrowIfNullOrWhiteSpace(hash); + return hash.Length == 64 + ? hash.ToLowerInvariant() + : s_enc.ComputeHash(System.Text.Encoding.UTF8.GetBytes(hash)).ToString(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cs` around lines 42 - 43, NormalizeHash currently crashes on empty input and collapses many different short hashes to the same value by padding with the first char; update NormalizeHash to validate non-empty input (throw ArgumentException for empty/null) and to pad deterministically without losing uniqueness by repeating the entire input string (not just hash[0]) until reaching length 64 (e.g., append successive characters from the original hash cyclically). Keep the method name NormalizeHash and ensure it returns a 64-char string for valid inputs.src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs (1)
69-83: Inline the one-off tar hash helper.
ComputeTarHashis only used byArchive_TarBlobAlreadyExistsWithMetadata_Rerun_Continues. Making it a local function keeps the tar-shape assumption scoped to that scenario and matches the repo preference for single-use helpers.As per coding guidelines, "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/ArchiveRecoveryTests.cs` around lines 69 - 83, Move the private static helper ComputeTarHash into Archive_TarBlobAlreadyExistsWithMetadata_Rerun_Continues as a local function: cut the ComputeTarHash method and paste its body inside the test method as a local function (e.g., ComputeTarHash or computeTarHash) so the tar-shape assumption is scoped to that single test; ensure the local function uses the same parameters (ArchiveTestEnvironment env, ContentHash contentHash, byte[] content), preserves the using blocks for TarWriter/MemoryStream, and remove the original private static ComputeTarHash declaration.src/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cs (1)
164-166: Stream hashing instead of buffering full files in memory.On Line 164,
File.ReadAllBytesAsyncruns inside the candidate scan loop. For larger representative datasets, this can create avoidable memory pressure. Prefer stream-based hashing.Based on learnings: Prefer streaming, batching, and bounded-memory or bounded-disk pipelines over whole-repository in-memory materialization when file count can grow.♻️ Proposed change
- var bytes = await File.ReadAllBytesAsync(filePath, cancellationToken); // todo use streaming - var contentHash = fixture.Encryption.ComputeHash(bytes); + await using var fileStream = File.OpenRead(filePath); + var contentHash = await fixture.Encryption.ComputeHashAsync(fileStream);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cs` around lines 164 - 166, Replace the in-memory read of each file with a streaming hash: instead of File.ReadAllBytesAsync(filePath, ...), open a FileStream (preferably with FileOptions.SequentialScan) and pass that stream to the hashing API (e.g., use fixture.Encryption.ComputeHash(Stream) or compute via a streaming HashAlgorithm) to produce contentHash, then call fixture.Index.LookupAsync(contentHash, cancellationToken); ensure the stream is disposed and that no full-file byte[] is allocated so hashing inside ArchiveTierLifecycleStep avoids buffering entire files in memory.src/Arius.Core/Shared/Snapshot/SnapshotService.cs (1)
12-19: Keep the new converter out ofSnapshotService.cs.This works, but adding another top-level type here makes the file even harder to navigate. I'd either nest it inside
SnapshotSerializeror move it to its ownFileTreeHashJsonConverter.cs.As per coding guidelines,
**/*.cs: Prefer one top-level class per file, with the filename matching the class name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Shared/Snapshot/SnapshotService.cs` around lines 12 - 19, The FileTreeHashJsonConverter type is defined as a separate top-level type inside SnapshotService.cs which violates the one-top-level-class-per-file guideline; move the converter into its own file named FileTreeHashJsonConverter.cs (or alternatively nest it inside SnapshotSerializer) and keep the implementation identical: declare it as internal sealed class FileTreeHashJsonConverter : JsonConverter<FileTreeHash> with the same Read and Write overrides, ensure it lives in the same namespace and retains any required using directives, and remove the converter declaration from SnapshotService.cs so the file contains only its primary type.src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs (1)
203-206: Use the new file-path hashing overload here.This still duplicates file-open and
ProgressStreamwiring even thoughIEncryptionService.ComputeHashAsync(string, IProgress<long>?, CancellationToken)now owns that path.Proposed simplification
- await using var fs = File.OpenRead(fullBinaryPath!); - var hashProgress = opts.CreateHashProgress?.Invoke(pair.RelativePath, fileSize) ?? new Progress<long>(); - await using var ps = new ProgressStream(fs, hashProgress); - contentHash = await _encryption.ComputeHashAsync(ps, ct); + var hashProgress = opts.CreateHashProgress?.Invoke(pair.RelativePath, fileSize) ?? new Progress<long>(); + contentHash = await _encryption.ComputeHashAsync(fullBinaryPath!, hashProgress, ct);Based on learnings: Write minimum code that solves the problem with nothing speculative. If a simpler approach exists, say so.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs` around lines 203 - 206, The current code manually opens the file and wraps it in ProgressStream before calling _encryption.ComputeHashAsync, but IEncryptionService now exposes ComputeHashAsync(string, IProgress<long>?, CancellationToken) that handles file opening and progress wiring; replace the fs/ps block by calling _encryption.ComputeHashAsync(pair.RelativePath, opts.CreateHashProgress?.Invoke(pair.RelativePath, fileSize), ct) and assign its result to contentHash (remove File.OpenRead, ProgressStream and the extra using statements).src/Arius.Core/Features/ListQuery/ListQueryHandler.cs (1)
156-166: Avoid theContentHash -> string -> ContentHashround-trip during listing.
FileEntry.ContentHashis already validated. Reparsing it here just adds allocations and an effectively dead warning path on a file-count-sensitive query.Proposed simplification
var hashes = visibleCloudFiles .Where(candidate => MatchesFilter(candidate.Entry.Name, filter)) - .Select(candidate => candidate.Entry.ContentHash.ToString()) - .Distinct(StringComparer.Ordinal) - .Select(TryParseContentHash) - .Where(hash => hash is not null) - .Select(hash => hash!.Value) + .Select(candidate => candidate.Entry.ContentHash) + .Distinct() .ToList(); var sizeLookup = hashes.Count == 0 ? new Dictionary<ContentHash, ShardEntry>() : new Dictionary<ContentHash, ShardEntry>(await _index.LookupAsync(hashes, cancellationToken));- private ContentHash? TryParseContentHash(string value) - { - if (ContentHash.TryParse(value, out var hash)) - return hash; - - _logger.LogWarning("Invalid content hash encountered while listing repository data: {ContentHash}", value); - return null; - }Based on learnings: Arius is a backup tool. Consider file-count scale when designing list behavior for large repositories with many thousands of files.
Also applies to: 380-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/ListQuery/ListQueryHandler.cs` around lines 156 - 166, The code currently converts Entry.ContentHash to string then reparses it via TryParseContentHash; instead build the hash list directly from visibleCloudFiles by selecting candidate.Entry.ContentHash (type ContentHash), apply Distinct with an appropriate comparer, and pass that IEnumerable<ContentHash> into _index.LookupAsync instead of the string round-trip; update any signature or call-site of _index.LookupAsync if necessary to accept ContentHash, and remove TryParseContentHash usage in this path (also adjust the sizeLookup construction that depends on hashes being ContentHash).src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs (1)
20-62: Consider deprecating the legacy string overloads to keep the typed API strict.The typed overloads are in place, but Lines [20-62] still expose public string entry points on the concrete service. If compatibility is not required, remove them; if it is, mark them
[Obsolete]to push callers toward typed hashes.♻️ Optional migration aid
+ [Obsolete("Use UploadLargeAsync(ChunkHash, ...) instead.")] public Task<ChunkUploadResult> UploadLargeAsync( string chunkHash, Stream content, long sourceSize, BlobTier tier, IProgress<long>? progress = null, CancellationToken cancellationToken = default) => UploadLargeAsync(ChunkHash.Parse(chunkHash), content, sourceSize, tier, progress, cancellationToken);Based on learnings: “Write minimum code that solves the problem with nothing speculative.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs` around lines 20 - 62, These public string-based overloads (UploadLargeAsync(string...), UploadTarAsync(string...), UploadThinAsync(string...), DownloadAsync(string...), GetHydrationStatusAsync(string...), StartRehydrationAsync(string...)) should be deprecated to steer callers to the strongly typed APIs: add the [Obsolete("Use overload accepting ChunkHash/ContentHash instead", false)] attribute to each of these methods (or remove them entirely if backwards compatibility is not required) so callers receive a compile-time warning and are guided to the typed overloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-27-typed-hashes.md`:
- Line 13: The markdown heading "### Task 1: Add Core Hash Value Objects And
Shared Utility" jumps from the document's H1 to H3 and triggers markdownlint
MD001; change the heading level to H2 by replacing "### Task 1: Add Core Hash
Value Objects And Shared Utility" with "## Task 1: Add Core Hash Value Objects
And Shared Utility" (or alternatively insert an intervening H2 section above it)
so the heading levels are hierarchical and MD001 is resolved.
In `@src/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceReadTests.cs`:
- Around line 12-13: The fixture hash
"abc12300112233445566778899aabbccddeeff00112233445566778899aabb" is 62 hex chars
and causes ChunkHash.Parse (called by the helper Chunk) to throw; replace each
occurrence of that invalid literal with valid 64-character hex strings (e.g. 64
'a' chars, 64 'b' chars, 64 'c' chars, 64 'd' chars, 64 'e' chars, 64 'f' chars
and 64 '0' chars) so Chunk(string) -> ChunkHash.Parse(...) receives exact
64-char SHA-256 hex strings and tests proceed past validation.
In `@src/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceUploadTests.cs`:
- Around line 51-55: Tests are failing because the literal arguments passed to
Chunk(...) (used in blobs.SeedTarBlobAsync, blobs.ThrowAlreadyExistsOnOpenWrite
and the UploadTarAsync call) contain non-hex characters ("tar...") and
ChunkHash.Parse/HashCodec.NormalizeHex enforces strict hex; replace those
"tar..." chunk literals with valid hexadecimal strings of the correct length
(same length as other chunk literals in the file) so Chunk(...) and
ChunkHash.Parse succeed and the UploadTarAsync path is exercised; update all
occurrences in the test (the SeedTarBlobAsync call,
ThrowAlreadyExistsOnOpenWrite call, and UploadTarAsync chunkHash parameter) to
the new valid hex literal.
In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs`:
- Around line 90-92: The assertion uses a null-forgiving dereference on root
when checking ToString().Length; instead make non-null explicit after the
ShouldNotBeNull() check: keep root.ShouldNotBeNull(); then capture the string
safely (e.g. var s = root?.ToString() ?? throw new
InvalidOperationException("root is null")); and assert s.Length.ShouldBe(64);
reference symbols: root, ToString(), and the test method in
FileTreeBuilderTests.cs.
In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs`:
- Around line 123-124: The string overload LookupAsync(string contentHash,
CancellationToken) currently calls ContentHash.Parse which throws on malformed
input; change it to non-throwing: attempt to parse using ContentHash.TryParse
(or wrap Parse in a try/catch if no TryParse exists), and if parsing fails
immediately return Task.FromResult<ShardEntry?>(null); otherwise call the
existing LookupAsync(ContentHash, CancellationToken) with the parsed value.
Update the LookupAsync(string...) method only (referencing LookupAsync and
ContentHash.Parse/TryParse) so invalid hash text degrades to null instead of
throwing.
In `@src/Arius.Core/Shared/FileTree/FileTreeBlobSerializer.cs`:
- Around line 164-189: The FileTreeBlobSerializer parsing for typeMarker 'F'
currently allows empty file names; update the FileEntry creation path in the 'F'
branch (inside the FileTreeBlobSerializer method) to validate the extracted name
variable and skip malformed entries by doing a check like if
(string.IsNullOrWhiteSpace(name)) continue before
ContentHash.TryParse/entries.Add, so empty or whitespace names are dropped just
like malformed directory lines; ensure you reference the existing name,
contentHash, entries and FileEntry usage when making the change.
In `@src/Arius.Core/Shared/Hashes/ChunkHash.cs`:
- Line 13: The Value property on struct ChunkHash currently returns string.Empty
for uninitialized instances which hides invalid state; change the getter for
Value (backed by _value) to throw InvalidOperationException when _value is null
or empty so that ToString(), Prefix4, Short8 and other consumers fail fast;
update the Value getter in ChunkHash to check _value and throw new
InvalidOperationException with a clear message (similar to ContentHash),
ensuring TryParse/creation paths still set _value when valid.
In `@src/Arius.Core/Shared/Hashes/FileTreeHash.cs`:
- Around line 13-39: The Value getter currently returns string.Empty when _value
is null which lets default(FileTreeHash) silently appear valid; change Value
(and therefore ToString()) to fail fast by throwing an InvalidOperationException
(or similar) when _value is null/uninitialized so consumers like
Prefix4/Short8/path builders crash early; update the Value property getter to
check _value and throw with a clear message referencing FileTreeHash being
uninitialized, leaving Parse, TryParse, FromDigest and other construction
methods unchanged.
---
Outside diff comments:
In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs`:
- Around line 405-420: The loop currently adds only the first file's
ShardEntry.CompressedSize (via TryGetIndexEntry(filesByChunkHash[chunkHash][0],
...)), underreporting multi-file tar chunk sizes; change the logic in the
foreach over chunksToRequest so totalRehydrateBytes uses the actual chunk-level
compressed bytes—either by looking up the chunk's index entry by chunkHash
(rather than the first file) or by summing the ShardEntry.CompressedSize for all
files in filesByChunkHash[chunkHash]—before publishing RehydrationStartedEvent;
update references to TryGetIndexEntry, filesByChunkHash, indexEntries,
totalRehydrateBytes and chunksToRehydrate accordingly.
---
Nitpick comments:
In `@src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs`:
- Around line 12-13: Move the private static helper FileTreeHash Tree(char c)
into the SnapshotResolvedHandler_SetsTimestampAndRootHash test as a local
function and remove the class-level Tree method; update the test to call the
local Tree(char) helper (ensuring it uses FileTreeHash.Parse(new string(c, 64)))
so the helper scope is limited to the test and the class no longer contains the
unused static method.
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs`:
- Around line 69-83: Move the private static helper ComputeTarHash into
Archive_TarBlobAlreadyExistsWithMetadata_Rerun_Continues as a local function:
cut the ComputeTarHash method and paste its body inside the test method as a
local function (e.g., ComputeTarHash or computeTarHash) so the tar-shape
assumption is scoped to that single test; ensure the local function uses the
same parameters (ArchiveTestEnvironment env, ContentHash contentHash, byte[]
content), preserves the using blocks for TarWriter/MemoryStream, and remove the
original private static ComputeTarHash declaration.
In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`:
- Around line 229-238: The private static helper CompressAsync is only used by
one test in RestoreCommandHandlerTests; move it into that test as a local async
method (remove the private static declaration), update the test to call the
local CompressAsync, and delete the now-unused private static CompressAsync from
the class so the helper is scoped to the single test method.
In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cs`:
- Around line 42-43: NormalizeHash currently crashes on empty input and
collapses many different short hashes to the same value by padding with the
first char; update NormalizeHash to validate non-empty input (throw
ArgumentException for empty/null) and to pad deterministically without losing
uniqueness by repeating the entire input string (not just hash[0]) until
reaching length 64 (e.g., append successive characters from the original hash
cyclically). Keep the method name NormalizeHash and ensure it returns a 64-char
string for valid inputs.
In `@src/Arius.Core.Tests/Shared/Hashes/FileTreeHashTests.cs`:
- Around line 8-21: Add negative tests for FileTreeHash.Parse to assert it
throws on invalid input: one test (e.g., Parse_Throws_OnNonHex) should call
FileTreeHash.Parse with a string containing non-hex characters and assert an
exception (ArgumentException/FormatException as used elsewhere) is thrown;
another test (e.g., Parse_Throws_OnWrongLength) should pass a hex string of
incorrect length (too short and/or too long) and assert the same exception.
Place tests alongside the existing
Parse_NormalizesUppercaseHexToCanonicalLowercase and Short8 tests and reference
FileTreeHash.Parse and the expected exception type used by other hash parse
tests to ensure consistency.
In `@src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs`:
- Around line 203-206: The current code manually opens the file and wraps it in
ProgressStream before calling _encryption.ComputeHashAsync, but
IEncryptionService now exposes ComputeHashAsync(string, IProgress<long>?,
CancellationToken) that handles file opening and progress wiring; replace the
fs/ps block by calling _encryption.ComputeHashAsync(pair.RelativePath,
opts.CreateHashProgress?.Invoke(pair.RelativePath, fileSize), ct) and assign its
result to contentHash (remove File.OpenRead, ProgressStream and the extra using
statements).
In `@src/Arius.Core/Features/ListQuery/ListQueryHandler.cs`:
- Around line 156-166: The code currently converts Entry.ContentHash to string
then reparses it via TryParseContentHash; instead build the hash list directly
from visibleCloudFiles by selecting candidate.Entry.ContentHash (type
ContentHash), apply Distinct with an appropriate comparer, and pass that
IEnumerable<ContentHash> into _index.LookupAsync instead of the string
round-trip; update any signature or call-site of _index.LookupAsync if necessary
to accept ContentHash, and remove TryParseContentHash usage in this path (also
adjust the sizeLookup construction that depends on hashes being ContentHash).
In `@src/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cs`:
- Around line 20-62: These public string-based overloads
(UploadLargeAsync(string...), UploadTarAsync(string...),
UploadThinAsync(string...), DownloadAsync(string...),
GetHydrationStatusAsync(string...), StartRehydrationAsync(string...)) should be
deprecated to steer callers to the strongly typed APIs: add the [Obsolete("Use
overload accepting ChunkHash/ContentHash instead", false)] attribute to each of
these methods (or remove them entirely if backwards compatibility is not
required) so callers receive a compile-time warning and are guided to the typed
overloads.
In `@src/Arius.Core/Shared/Snapshot/SnapshotService.cs`:
- Around line 12-19: The FileTreeHashJsonConverter type is defined as a separate
top-level type inside SnapshotService.cs which violates the
one-top-level-class-per-file guideline; move the converter into its own file
named FileTreeHashJsonConverter.cs (or alternatively nest it inside
SnapshotSerializer) and keep the implementation identical: declare it as
internal sealed class FileTreeHashJsonConverter : JsonConverter<FileTreeHash>
with the same Read and Write overrides, ensure it lives in the same namespace
and retains any required using directives, and remove the converter declaration
from SnapshotService.cs so the file contains only its primary type.
In `@src/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cs`:
- Around line 164-166: Replace the in-memory read of each file with a streaming
hash: instead of File.ReadAllBytesAsync(filePath, ...), open a FileStream
(preferably with FileOptions.SequentialScan) and pass that stream to the hashing
API (e.g., use fixture.Encryption.ComputeHash(Stream) or compute via a streaming
HashAlgorithm) to produce contentHash, then call
fixture.Index.LookupAsync(contentHash, cancellationToken); ensure the stream is
disposed and that no full-file byte[] is allocated so hashing inside
ArchiveTierLifecycleStep avoids buffering entire files in memory.
🪄 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: 5730ea3f-5d29-49ae-ab15-15613a9c0bc1
📒 Files selected for processing (78)
AGENTS.mddocs/superpowers/plans/2026-04-27-typed-hashes.mdsrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cssrc/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cssrc/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cssrc/Arius.Cli.Tests/Commands/Restore/ChunkDownloadCompletedHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/TrackedDownloadLifecycleTests.cssrc/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cssrc/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cssrc/Arius.Cli/Commands/Restore/RestoreProgressHandlers.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveModelTypingTests.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/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/Features/RestoreCommand/RestoreModelTypingTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerLocalTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardTests.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/GoldenFileDecryptionTests.cssrc/Arius.Core.Tests/Shared/Encryption/PassphraseEncryptionServiceTests.cssrc/Arius.Core.Tests/Shared/Encryption/PlaintextPassthroughServiceTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerStorageTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cssrc/Arius.Core.Tests/Shared/Hashes/ChunkHashTests.cssrc/Arius.Core.Tests/Shared/Hashes/ContentHashTests.cssrc/Arius.Core.Tests/Shared/Hashes/FileTreeHashTests.cssrc/Arius.Core.Tests/Shared/LocalFile/LocalFileEnumeratorTests.cssrc/Arius.Core.Tests/Shared/Snapshot/SnapshotSerializerTests.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ArchiveCommand/Events.cssrc/Arius.Core/Features/ArchiveCommand/Models.cssrc/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Features/RestoreCommand/Events.cssrc/Arius.Core/Features/RestoreCommand/Models.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkIndex/Shard.cssrc/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cssrc/Arius.Core/Shared/ChunkStorage/ChunkUploadResult.cssrc/Arius.Core/Shared/ChunkStorage/IChunkStorageService.cssrc/Arius.Core/Shared/Encryption/IEncryptionService.cssrc/Arius.Core/Shared/Encryption/PassphraseEncryptionService.cssrc/Arius.Core/Shared/Encryption/PlaintextPassthroughService.cssrc/Arius.Core/Shared/FileTree/FileTreeBlobSerializer.cssrc/Arius.Core/Shared/FileTree/FileTreeBuilder.cssrc/Arius.Core/Shared/FileTree/FileTreeModels.cssrc/Arius.Core/Shared/FileTree/FileTreeService.cssrc/Arius.Core/Shared/Hashes/ChunkHash.cssrc/Arius.Core/Shared/Hashes/ContentHash.cssrc/Arius.Core/Shared/Hashes/FileTreeHash.cssrc/Arius.Core/Shared/Hashes/HashCodec.cssrc/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.Core/Shared/Storage/BlobConstants.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryMaterializer.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryState.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryStateAssertions.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cssrc/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cssrc/Arius.E2E.Tests/Workflows/Steps/Helpers.cssrc/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cssrc/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cssrc/Arius.Explorer.Tests/Usings.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/Fakes/CbcEncryptionServiceAdapter.cssrc/Arius.Integration.Tests/Pipeline/RecoveryScriptTests.cssrc/Arius.Integration.Tests/Shared/FileTree/FileTreeBuilderIntegrationTests.cssrc/Arius.Integration.Tests/Snapshot/SnapshotServiceIntegrationTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerLocalTests.cs (1)
16-35: Strengthen round-trip assertions by validatingChunkHashtoo.Current checks on Line 32 and Line 35 only validate sizes. A serialization bug that preserves sizes but alters mapped
ChunkHashcould still pass. Add explicit hash assertions to close that gap.✅ Suggested test assertion additions
loaded.TryLookup(Content('a'), out var e1).ShouldBeTrue(); e1!.OriginalSize.ShouldBe(512); + e1.ChunkHash.ShouldBe(Chunk('b')); loaded.TryLookup(Content('c'), out var e2).ShouldBeTrue(); e2!.CompressedSize.ShouldBe(40); + e2.ChunkHash.ShouldBe(Chunk('d'));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.Core.Tests/Shared/ChunkIndex/ShardSerializerLocalTests.cs` around lines 16 - 35, The test currently only asserts OriginalSize and CompressedSize after round-tripping via ShardSerializer.SerializeLocal/DeserializeLocal; add assertions that the mapped ChunkHash values are preserved as well by checking e1.ChunkHash equals the original Chunk('b') hash and e2.ChunkHash equals the original Chunk('d') hash (locate the originals by the Chunk(...) calls used to build the ShardEntry instances or by capturing their hash before serialization), so use TryLookup results (e1 and e2) to assert equality of ChunkHash in addition to the existing size assertions.src/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cs (1)
23-29: AssertSetFileUploading(...)return values to tighten behavior checks.Both tests validate state, but not the method contract (
truewhen promoted,falsewhen unknown hash). Adding these assertions will prevent false positives in future regressions.As per coding guidelines `**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.`Suggested test assertion updates
- state.SetFileUploading(Content('a')); + state.SetFileUploading(Content('a')).ShouldBeTrue(); state.TrackedFiles["video.mp4"].State.ShouldBe(FileState.Uploading); @@ - state.SetFileUploading(Content('b')); + state.SetFileUploading(Content('b')).ShouldBeFalse(); state.TrackedFiles["pending.txt"].State.ShouldBe(FileState.Hashing);Also applies to: 46-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cs` around lines 23 - 29, The test currently calls state.SetFileUploading(Content('a')) and only asserts the TrackedFiles state; update the test to also assert the method's boolean return (expect true when a known/hashed file is promoted) by adding Assert/ShouldBe checks on the SetFileUploading result, and likewise add a complementary assertion that SetFileUploading returns false when called with an unknown hash (the other test block that promotes unknown hashes should assert false). Locate and update the calls to SetFileUploading in TrackedFileLargeFilePathTests (the lines that call state.SetFileUploading(Content('a')) and the similar call later) to verify the method contract in addition to the TrackedFiles state.src/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cs (1)
13-15: Move single-use hash helpers into the test as local methods.Line 13 and Line 14 helpers are only used by one test method; prefer local functions there to follow repo convention.
Suggested refactor
public class TrackedTarLifecycleTests { - private static ContentHash Content(char c) => ContentHash.Parse(new string(c, 64)); - private static ChunkHash Chunk(char c) => ChunkHash.Parse(new string(c, 64)); - [Test] public async Task TrackedTar_FullLifecycle_StateTransitions() { + static ContentHash Content(char c) => ContentHash.Parse(new string(c, 64)); + static ChunkHash Chunk(char c) => ChunkHash.Parse(new string(c, 64)); + var state = new ProgressState(); var startedH = new TarBundleStartedHandler(state);As per coding guidelines: "In C# code, prefer local methods over private static methods for helper functionality used only within a single method."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cs` around lines 13 - 15, The private static helpers Content and Chunk in TrackedTarLifecycleTests are only used by one test—move them into that test as local functions: locate the test method that calls Content(...) and Chunk(...), define local methods Content(char c) => ContentHash.Parse(new string(c,64)) and Chunk(char c) => ChunkHash.Parse(new string(c,64)) at the top of that method, update uses to call the local versions, and remove the two private static methods from the class.
🤖 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/Commands/Restore/RestoreVerb.cs`:
- Around line 152-153: The code currently uses identifier[..8] as a fallback
label when ChunkHash.TryParse fails, which will throw for identifiers shorter
than 8 characters; update the logic around ChunkHash.TryParse(identifier, out
var typedChunkHash) and the subsequent restoreProgress.TarBundleMetadata
fallback to produce a safe slice (e.g., take up to Math.Min(8,
identifier.Length) or conditional use of the whole identifier when Length < 8)
so you never call identifier[..8]; apply the same guarded-slicing fix to the
other occurrence referenced (lines 158-161) where the fallback label is
constructed.
---
Nitpick comments:
In `@src/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cs`:
- Around line 23-29: The test currently calls
state.SetFileUploading(Content('a')) and only asserts the TrackedFiles state;
update the test to also assert the method's boolean return (expect true when a
known/hashed file is promoted) by adding Assert/ShouldBe checks on the
SetFileUploading result, and likewise add a complementary assertion that
SetFileUploading returns false when called with an unknown hash (the other test
block that promotes unknown hashes should assert false). Locate and update the
calls to SetFileUploading in TrackedFileLargeFilePathTests (the lines that call
state.SetFileUploading(Content('a')) and the similar call later) to verify the
method contract in addition to the TrackedFiles state.
In `@src/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cs`:
- Around line 13-15: The private static helpers Content and Chunk in
TrackedTarLifecycleTests are only used by one test—move them into that test as
local functions: locate the test method that calls Content(...) and Chunk(...),
define local methods Content(char c) => ContentHash.Parse(new string(c,64)) and
Chunk(char c) => ChunkHash.Parse(new string(c,64)) at the top of that method,
update uses to call the local versions, and remove the two private static
methods from the class.
In `@src/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerLocalTests.cs`:
- Around line 16-35: The test currently only asserts OriginalSize and
CompressedSize after round-tripping via
ShardSerializer.SerializeLocal/DeserializeLocal; add assertions that the mapped
ChunkHash values are preserved as well by checking e1.ChunkHash equals the
original Chunk('b') hash and e2.ChunkHash equals the original Chunk('d') hash
(locate the originals by the Chunk(...) calls used to build the ShardEntry
instances or by capturing their hash before serialization), so use TryLookup
results (e1 and e2) to assert equality of ChunkHash in addition to the existing
size assertions.
🪄 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: 9f7e79ac-c4af-4eae-bcd0-e5b29d8cd3bd
📒 Files selected for processing (22)
src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayDoneTests.cssrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cssrc/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cssrc/Arius.Cli.Tests/Commands/Archive/ContentHashToPathTests.cssrc/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Archive/ProgressCallbackIntegrationTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedFileSmallFilePathTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cssrc/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cssrc/Arius.Cli.Tests/ProgressStateThreadSafetyTests.cssrc/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cssrc/Arius.Cli/Commands/Archive/ArchiveVerb.cssrc/Arius.Cli/Commands/Restore/RestoreProgressHandlers.cssrc/Arius.Cli/Commands/Restore/RestoreVerb.cssrc/Arius.Cli/ProgressState.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerLocalTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.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/PlaintextPassthroughServiceTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs
- src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs
- src/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.cs
- src/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cs
- src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs
- src/Arius.Core.Tests/Shared/Encryption/PlaintextPassthroughServiceTests.cs
- src/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceUploadTests.cs
- src/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageHydrationStatusTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Shared/FileTree/FileTreeBlobSerializerTests.cs`:
- Around line 19-43: The NormalizeHash helper currently pads using only the
first character, collapsing distinct short fixtures; update NormalizeHash to
expand the whole input string by repeating the full input sequence until length
>= 64 and then truncate to 64 (e.g., string repeated and substring(0,64)),
preserving the original nibble sequence for calls from BuildEntries which feed
into ContentHash.Parse and FileTreeHash.Parse; keep the same signature and
ensure it handles inputs already length 64 by returning them unchanged.
In `@src/Arius.Core.Tests/Shared/Hashes/HashValueObjectTests.cs`:
- Around line 5-137: Add tests exercising TryParse for ContentHash, ChunkHash,
and FileTreeHash: create a MatrixDataSource test that verifies TryParse returns
true for a valid 64-hex string and that the out value's ToString() equals the
canonical lowercase (use the same example strings as Prefix4/Short8 tests),
another test that verifies TryParse returns false for invalid inputs (empty
string, wrong length, non-hex) and that the out parameter equals
default(ContentHash)/default(ChunkHash)/default(FileTreeHash) respectively;
locate and call the static TryParse methods (ContentHash.TryParse,
ChunkHash.TryParse, FileTreeHash.TryParse) and reuse the Parse helper pattern to
map HashKind to the appropriate TryParse overloads.
🪄 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: 52936823-4ddb-497e-9836-7e10b47da4ac
📒 Files selected for processing (6)
src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cssrc/Arius.Core.Tests/Shared/Hashes/HashValueObjectTests.cssrc/Arius.Core/Shared/FileTree/FileTreeBlobSerializer.cssrc/Arius.Core/Shared/Hashes/ChunkHash.cssrc/Arius.Core/Shared/Hashes/ContentHash.cssrc/Arius.Core/Shared/Hashes/FileTreeHash.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Arius.Core/Shared/Hashes/FileTreeHash.cs
- src/Arius.Core/Shared/FileTree/FileTreeBlobSerializer.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-27-shared-hash-test-helper.md`:
- Around line 13-38: The markdown headings under "Task 1"–"Task 4" currently use
H3 (###) which causes an H1→H3 jump and violates markdownlint MD001; update
those task headings in
docs/superpowers/plans/2026-04-27-shared-hash-test-helper.md to use H2 (change
"### Task X: ..." to "## Task X: ...") or alternatively add a parent H2 section
above them so the heading levels progress correctly, then run markdownlint to
confirm MD001 is resolved.
🪄 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: dfb5e900-4fa5-481e-a7db-7df7c1217a2b
📒 Files selected for processing (24)
docs/superpowers/plans/2026-04-27-shared-hash-test-helper.mdsrc/Arius.Cli.Tests/Arius.Cli.Tests.csprojsrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cssrc/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cssrc/Arius.Cli.Tests/Commands/Archive/ContentHashToPathTests.cssrc/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Archive/ProgressCallbackIntegrationTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedFileSmallFilePathTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cssrc/Arius.Cli.Tests/Commands/Restore/ChunkDownloadCompletedHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/TrackedDownloadLifecycleTests.cssrc/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cssrc/Arius.Cli.Tests/Usings.cssrc/Arius.Core.Tests/Arius.Core.Tests.csprojsrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerLocalTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.cssrc/Arius.Core.Tests/Usings.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/Snapshot/SnapshotServiceIntegrationTests.cssrc/Arius.Integration.Tests/Usings.cssrc/Arius.Tests.Shared/AssemblyMarker.cssrc/Arius.Tests.Shared/Hashes/HashTestData.cs
✅ Files skipped from review due to trivial changes (10)
- src/Arius.Cli.Tests/Arius.Cli.Tests.csproj
- src/Arius.Tests.Shared/AssemblyMarker.cs
- src/Arius.Core.Tests/Usings.cs
- src/Arius.Core.Tests/Arius.Core.Tests.csproj
- src/Arius.Cli.Tests/Usings.cs
- src/Arius.Integration.Tests/Usings.cs
- src/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cs
- src/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.cs
- src/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cs
- src/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs
- src/Arius.Integration.Tests/Snapshot/SnapshotServiceIntegrationTests.cs
- src/Arius.Cli.Tests/Commands/Restore/TrackedDownloadLifecycleTests.cs
- src/Arius.Cli.Tests/Commands/Archive/ContentHashToPathTests.cs
- src/Arius.Cli.Tests/Commands/Restore/ChunkDownloadCompletedHandlerTests.cs
- src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs
…vanranst/Arius7 into feat/cleanup-compute-hash
…vanranst/Arius7 into feat/cleanup-compute-hash
…vanranst/Arius7 into feat/cleanup-compute-hash
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md (1)
13-16:⚠️ Potential issue | 🟡 MinorSurround the markdown table with blank lines.
MD058is triggered here; add a blank line before and after the table block.Proposed diff
Method Mean Error StdDev Completed Work Items Lock Contentions Gen0 Gen1 Gen2 Allocated 'Canonical_Representative_Workflow_Runs_On_Supported_Backends (Azurite)' 38.54 s 6.373 s 0.349 s 70331.0000 21.0000 49000.0000 17000.0000 14000.0000 829.57 MB
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md
around lines 13 - 16, The markdown table starting with the header row "| Method
| Mean..." is missing surrounding blank lines which triggers MD058; add exactly
one blank line before the table header line and one blank line after the final
table row (the row containing the Azurite result) so the table is separated from
surrounding text/blocks.</details> </blockquote></details> <details> <summary>src/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md (1)</summary><blockquote> `13-16`: _⚠️ Potential issue_ | _🟡 Minor_ **Add blank lines around the markdown table.** The table block is not surrounded by blank lines (MD058), so it will keep failing markdownlint if this path is linted. <details> <summary>Suggested patch</summary> ```diff + | Method | Mean | Error | StdDev | Completed Work Items | Lock Contentions | Gen0 | Gen1 | Gen2 | Allocated | |------------------------------------------------------------------------- |--------:|--------:|--------:|---------------------:|-----------------:|-----------:|-----------:|-----------:|----------:| | 'Canonical_Representative_Workflow_Runs_On_Supported_Backends (Azurite)' | 38.88 s | 5.162 s | 0.283 s | 70650.0000 | 3.0000 | 50000.0000 | 18000.0000 | 15000.0000 | 822.99 MB | +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md` around lines 13 - 16, Add blank lines before and after the markdown table that begins with the header "| Method | Mean | Error | StdDev | Completed Work Items | Lock Contentions | Gen0 | Gen1 | Gen2 | Allocated |" (the table row containing 'Canonical_Representative_Workflow_Runs_On_Supported_Backends (Azurite)') so the table block is separated by empty lines above and below to satisfy MD058; edit the report content to insert a single empty line immediately before the table header line and a single empty line immediately after the final table row.
♻️ Duplicate comments (3)
src/Arius.Core.Tests/Shared/Hashes/HashValueObjectTests.cs (1)
30-164:⚠️ Potential issue | 🟡 MinorAdd
TryParsecontract tests to complete the suite.This file validates the throwing
ParseAPI well, but it still missesTryParsesuccess/failure behavior andoutdefault-value assertions for all hash kinds. Please add matrix tests covering valid input (true+ canonical value) and invalid inputs (false+ default hash value).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.Core.Tests/Shared/Hashes/HashValueObjectTests.cs` around lines 30 - 164, Add matrix tests for the TryParse contract for all hash kinds: implement a test (e.g., TryParse_SucceedsForCanonicalValue) that calls ContentHash.TryParse/ChunkHash.TryParse/FileTreeHash.TryParse with a valid canonical hex string and asserts it returns true and the out value equals the parsed canonical value (use ContentHash.Parse/ChunkHash.Parse/FileTreeHash.Parse or FromDigest for comparison), and another test (e.g., TryParse_FailsForInvalidInputs) that calls TryParse with invalid inputs (empty, wrong length, non-hex) and asserts it returns false and the out parameter equals default(ContentHash)/default(ChunkHash)/default(FileTreeHash); use the same MatrixDataSource pattern and the existing Parse helper switch as guidance to locate the correct types and methods.src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerStorageTests.cs (1)
21-22:⚠️ Potential issue | 🟡 MinorNormalize short test hashes without collapsing them.
NormalizeHash("a1b2c3d4")currently becomesaaaa…, so these round-trip assertions stop checking preservation of the full canonical hash value. Repeat/truncate the full input instead of padding with only the first nibble.💡 Minimal fix
private static string NormalizeHash(string hash) - => hash.Length == 64 ? hash : hash[0].ToString().PadRight(64, char.ToLowerInvariant(hash[0])); +{ + var normalized = hash.ToLowerInvariant(); + if (normalized.Length == 64) + return normalized; + + return string.Concat(Enumerable.Repeat(normalized, 64 / normalized.Length + 1))[..64]; +}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.Core.Tests/Shared/FileTree/FileTreeBlobSerializerStorageTests.cs` around lines 21 - 22, The NormalizeHash helper incorrectly pads short test hashes by repeating only the first character, collapsing the original value; update the NormalizeHash method so that when hash.Length != 64 it constructs a 64-char string by repeating the full input sequence and then taking the first 64 characters (i.e., string.Concat(Enumerable.Repeat(hash, ...)) or a loop) so short inputs like "a1b2c3d4" are expanded/truncated to preserve their full nibble sequence rather than turning into "aaaa...". Ensure the method still returns the original when length == 64.src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs (1)
19-20:⚠️ Potential issue | 🟡 MinorNormalize short test hashes without collapsing them.
This still turns inputs like
a1b2c3d4intoaaaa…, which weakens the typed-hash round-trip assertions and can hide collisions between distinct fixtures.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.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs` around lines 19 - 20, NormalizeHash currently pads short inputs by repeating only the first character, collapsing distinct short hashes into identical values; change NormalizeHash to pad by repeating the whole input string (e.g., repeat the short hash cyclically and then take the first 64 chars) so inputs like "a1b2c3d4" remain distinguishable, keeping the method deterministic and still returning exactly 64 characters.
🧹 Nitpick comments (9)
src/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md (1)
1-12: Add a language to the fenced code block for markdownlint compliance.This block is missing a fence language (
MD040). Usetextsince this is benchmark output.Proposed diff
-``` +```text ... -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md` around lines 1 - 12, The fenced code block containing the BenchmarkDotNet output (starting with "BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS") is missing a language tag; update the opening backticks to include "text" (i.e., change ``` to ```text) so the block complies with markdownlint MD040 while preserving the existing content and formatting.src/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md (1)
1-12: Specify a language on the fenced code block.The opening fence at Line 1 is missing a language token, which triggers MD040 in markdownlint.
Suggested patch
-``` +```text BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) AMD EPYC 7763 2.45GHz, 1 CPU, 4 logical and 2 physical cores .NET SDK 10.0.203 [Host] : .NET 10.0.7 (10.0.7, 10.0.726.21808), X64 RyuJIT x86-64-v3 Job-HILDPN : .NET 10.0.7 (10.0.7, 10.0.726.21808), X64 RyuJIT x86-64-v3 InvocationCount=1 IterationCount=3 LaunchCount=1 UnrollFactor=1 WarmupCount=0</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md
around lines 1 - 12, The fenced code block at the top of the report is missing a
language token (triggers markdownlint MD040); fix by changing the opening fence
fromtotext (or another appropriate language) so the block becomesthe report file (the first triple-backtick block) accordingly.src/Arius.Benchmarks/raw/20260428T044504.151Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md (1)
1-1: Static analysis warnings in generated content.The Markdown linter flags two formatting issues:
- Line 1: Fenced code block should specify a language (e.g.,
```text)- Line 13: Table should be surrounded by blank lines
These are valid warnings, but they appear in auto-generated BenchmarkDotNet output. If you decide to commit these reports, you could post-process them to fix the formatting:
📝 Optional formatting fixes
-``` +```text BenchmarkDotNet v0.15.8, macOS Tahoe 26.4 (25E246) [Darwin 25.4.0]UnrollFactor=1 WarmupCount=0
| Method | Mean | Error | StdDev | Gen0 | Completed Work Items | Lock Contentions | Gen1 | Gen2 | Allocated |
</details> Also applies to: 13-13 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/Arius.Benchmarks/raw/20260428T044504.151Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md
at line 1, The generated BenchmarkDotNet markdown lacks a language on the
opening fenced code block and misses blank lines around the table header,
causing linter warnings; update the post-processing step that emits the report
so it modifies the top fence from "" to "text" and ensures a blank line
before and after the table header row that begins with "| Method", or adjust the
report generation routine that produces the BenchmarkDotNet output to insert
those formatting fixes automatically.</details> </blockquote></details> <details> <summary>src/Arius.Integration.Tests/Storage/BlobStorageServiceTests.cs (1)</summary><blockquote> `69-78`: **Test name currently overstates what is asserted.** `Upload_WithCoolTier_BlobHasCoolTier` only checks existence, not tier. Consider renaming the test (or moving tier verification to an Azure-capability-gated test) to avoid false confidence. <details> <summary>Suggested rename</summary> ```diff -[Test] -public async Task Upload_WithCoolTier_BlobHasCoolTier() +[Test] +public async Task Upload_WithCoolTier_BlobExists() ``` </details> Based on learnings: "Keep real archive-tier and rehydration semantics in Azure-capability-gated tests." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/Arius.Integration.Tests/Storage/BlobStorageServiceTests.cs` around lines 69 - 78, The test Upload_WithCoolTier_BlobHasCoolTier is misnamed because it only asserts existence; rename the test to reflect its actual assertion (e.g., Upload_WithCoolTier_BlobExists or Upload_UploadsBlob_WhenCoolTierRequested) or split the behavior: keep this test to check existence using svc.UploadAsync(..., BlobTier.Cool) and move any real-tier verification into an Azure-capability-gated test that inspects effective tier/rehydration; update the test method name and any references to Upload_WithCoolTier_BlobHasCoolTier accordingly. ``` </details> </blockquote></details> <details> <summary>src/Arius.Core.Tests/Shared/Encryption/PassphraseEncryptionServiceTests.cs (1)</summary><blockquote> `123-129`: **Avoid overlapping file handles in the parity test for better robustness.** Open the stream after computing the path variant (or run them sequentially without overlap). It keeps the test focused on hash parity rather than file-share behavior. <details> <summary>♻️ Proposed refactor</summary> ```diff try { - await using var stream = File.OpenRead(path); - var fromPath = await svc.ComputeHashAsync(path); + + await using var stream = File.OpenRead(path); var fromStream = await svc.ComputeHashAsync(stream); fromPath.ShouldBe(fromStream); } ``` </details> As per coding guidelines, "Test projects. Focus on test coverage gaps and assertion quality rather than style." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/Encryption/PassphraseEncryptionServiceTests.cs` around lines 123 - 129, The test currently opens a FileStream before calling svc.ComputeHashAsync(path) causing overlapping file handles; change the order so you first call await svc.ComputeHashAsync(path) and only then open the stream (await using var stream = File.OpenRead(path)) before calling await svc.ComputeHashAsync(stream), or alternatively compute the two hashes fully sequentially by disposing the stream between calls; update the test in PassphraseEncryptionServiceTests to ensure ComputeHashAsync(path) and ComputeHashAsync(stream) do not run with the same file handle open concurrently. ``` </details> </blockquote></details> <details> <summary>src/Arius.Core.Tests/Features/ChunkHydrationStatusQuery/ResolveFileHydrationStatusesHandlerTests.cs (1)</summary><blockquote> `28-46`: **The `TypeThin` matrix branch no longer matches the real thin-file shape.** This branch still models the thin case as `BlobPaths.Chunk(chunkHash)` plus `AriusType=thin`, while the real persisted thin-pointer path is `BlobPaths.ThinChunk(contentHash)` with hydration derived from the backing tar chunk. That means this case can stay green even if the real thin-file resolution path regresses. I’d either drop `TypeThin` from this matrix or set up the actual thin-pointer + tar-chunk relationship here. As per coding guidelines, `**/*.Tests/**`: Test projects. Focus on test coverage gaps and assertion quality rather than style. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/ChunkHydrationStatusQuery/ResolveFileHydrationStatusesHandlerTests.cs` around lines 28 - 46, The TypeThin branch is modelling thin files incorrectly; update the test matrix so TypeThin uses the real thin-pointer shape by setting resolvedChunkHash to BlobPaths.ThinChunk(contentHash) (not Chunk(chunkHash)) and make the ShardEntry for TypeThin point at the backing tarChunkHash (i.e., new ShardEntry(contentHash, tarChunkHash, ...)); also ensure FakeMetadataOnlyBlobContainerService.ConfigureChunk is invoked to create both the thin-pointer blob and the backing tar chunk (so the thin->tar relationship exists), or alternatively remove the TypeThin branch from the matrix if you prefer to avoid testing thin resolution here. ``` </details> </blockquote></details> <details> <summary>src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs (1)</summary><blockquote> `68-82`: **Prefer a local helper for tar-hash computation.** Line 68 introduces a private static helper used by only one test method; keep it local to reduce file-level surface. <details> <summary>♻️ Proposed refactor</summary> ```diff - var tarHash = ComputeTarHash(env, contentHash, content); + var tarHash = ComputeTarHash(contentHash, content); + + ChunkHash ComputeTarHash(ContentHash hash, byte[] bytes) + { + using var tarStream = new MemoryStream(); + using (var writer = new TarWriter(tarStream, leaveOpen: true)) + { + var entry = new PaxTarEntry(TarEntryType.RegularFile, hash.ToString()) + { + DataStream = new MemoryStream(bytes, writable: false) + }; + writer.WriteEntry(entry); + } + + return ChunkHash.Parse(env.Encryption.ComputeHash(tarStream.ToArray())); + } @@ - private static ChunkHash ComputeTarHash(ArchiveTestEnvironment env, ContentHash contentHash, byte[] content) - { - using var tarStream = new MemoryStream(); - using (var writer = new TarWriter(tarStream, leaveOpen: true)) - { - var entry = new PaxTarEntry(TarEntryType.RegularFile, contentHash.ToString()) - { - DataStream = new MemoryStream(content, writable: false) - }; - - writer.WriteEntry(entry); - } - - return ChunkHash.Parse(env.Encryption.ComputeHash(tarStream.ToArray())); - } ``` </details> As per coding guidelines: `**/*.cs`: Prefer local methods over private static methods for helper functionality that is only used within a single method. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs` around lines 68 - 82, The private static helper ComputeTarHash is only used by a single test and should be converted to a local function inside that test method in ArchiveRecoveryTests; move the body of ComputeTarHash into the test as a local method (remove private/static modifiers), keep the same parameters (ArchiveTestEnvironment env, ContentHash contentHash, byte[] content) and return type ChunkHash, and keep the internals (creating TarWriter, PaxTarEntry, and calling env.Encryption.ComputeHash followed by ChunkHash.Parse) unchanged; update the test call sites to use the new local function name and ensure streams are disposed the same way. ``` </details> </blockquote></details> <details> <summary>src/Arius.Core/Shared/Snapshot/SnapshotService.cs (1)</summary><blockquote> `34-41`: **Move `FileTreeHashJsonConverter` out of `SnapshotService.cs`.** This adds another top-level class to the file. Please give the converter its own `FileTreeHashJsonConverter.cs` file, or nest it if you want it to stay local to snapshot serialization. As per coding guidelines, `**/*.cs`: Prefer one top-level class per file, with the filename matching the class name. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Shared/Snapshot/SnapshotService.cs` around lines 34 - 41, The FileTreeHashJsonConverter class should be moved out of SnapshotService.cs into its own file named FileTreeHashJsonConverter.cs: create a new internal sealed class FileTreeHashJsonConverter in the same namespace as SnapshotService, copy the Read and Write overrides exactly, preserve usings and accessibility, and remove the converter definition from SnapshotService.cs so only one top-level class remains per file; ensure the project builds (no references need changing if the namespace is unchanged) or alternatively nest the converter inside a Snapshot serialization helper type if you prefer to keep it local. ``` </details> </blockquote></details> <details> <summary>src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs (1)</summary><blockquote> `32-83`: **Assert the surviving entry types and typed hashes.** These new skip-tests only check count and names. They would still pass if `healthy.txt` lost its `ContentHash` or `photos/` was deserialized as the wrong entry type. Please assert the surviving `FileEntry`/`DirectoryEntry` payloads too. As per coding guidelines, `**/*.Tests/**`: Test projects. Focus on test coverage gaps and assertion quality rather than style. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs` around lines 32 - 83, The tests Deserialize_SkipsEntriesWithInvalidHashes_AndPreservesValidEntries, Deserialize_SkipsMalformedEntries_AndPreservesValidSiblings and Deserialize_SkipsFileEntriesWithEmptyOrWhitespaceNames_AndPreservesValidSiblings only assert count and names; update each test to also assert the surviving entries' concrete types and their hashes: after calling FileTreeBlobSerializer.Deserialize, find the entry for "healthy.txt" and assert it is a FileEntry and that its ContentHash equals the expected 64-char string (e.g., new string('a',64) or 'c' as used), and find "photos/" and assert it is a DirectoryEntry and its Hash equals the expected 64-char value (e.g., new string('b',64) or 'd'); use the blob.Entries collection and the entry's type/property names (FileEntry, DirectoryEntry, ContentHash/Hash) to locate and assert these values. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs:
- Around line 214-220: The test currently treats a snapshot with an invalid
content hash for "broken.txt" as a silent success; update the assertions in
RestoreCommandHandlerTests to surface the corruption instead: assert that
result.Success is false (or otherwise indicates an incomplete restore), assert
result.FilesRestored == 1 and result.FilesSkipped == 1, and assert
result.ErrorMessage is non-null and contains "broken.txt" (or other identifier
of the corrupt entry) so callers can detect the skipped/corrupt file.In
@src/Arius.Core.Tests/Shared/ChunkIndex/ShardTests.cs:
- Around line 181-185: The test named Shard_PrefixOf_ShortHash_Throws no longer
calls Shard.PrefixOf and only asserts ContentHash.Parse("abc") throws; either
rename the test to reflect parse validation (e.g.,
ContentHash_Parse_ShortHash_Throws) or restore coverage by adding an explicit
PrefixOf assertion: call Shard.PrefixOf with appropriately constructed inputs
that exercise the prefix validation (for example, create a valid ContentHash and
a deliberately short/invalid hash input or use the API overload that accepts raw
bytes) and assert Should.Throw(() => Shard.PrefixOf(...));
update the test method name if you choose the rename option.In
@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs:
- Around line 632-633: The code currently swallows malformed tar entry names by
continuing when ContentHash.TryParse(tarEntry.Name, out var contentHash) fails;
instead treat this as corrupt repository data and fail fast: in the method on
RestoreCommandHandler that iterates tar entries, replace the silent continue
with logic that surfaces an error (throw a descriptive exception or return a
failed result) including the offending tarEntry.Name so the restore command
fails rather than silently omitting data; ensure you use the same
ContentHash.TryParse check and include the file name in the error message to aid
debugging while preserving surrounding deduplication/encryption logic.
Outside diff comments:
In
@src/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md:
- Around line 13-16: The markdown table starting with the header row "| Method
| Mean..." is missing surrounding blank lines which triggers MD058; add exactly
one blank line before the table header line and one blank line after the final
table row (the row containing the Azurite result) so the table is separated from
surrounding text/blocks.In
@src/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md:
- Around line 13-16: Add blank lines before and after the markdown table that
begins with the header "| Method | Mean | Error | StdDev | Completed Work Items
| Lock Contentions | Gen0 | Gen1 | Gen2 | Allocated |" (the table row containing
'Canonical_Representative_Workflow_Runs_On_Supported_Backends (Azurite)') so the
table block is separated by empty lines above and below to satisfy MD058; edit
the report content to insert a single empty line immediately before the table
header line and a single empty line immediately after the final table row.
Duplicate comments:
In@src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerStorageTests.cs:
- Around line 21-22: The NormalizeHash helper incorrectly pads short test hashes
by repeating only the first character, collapsing the original value; update the
NormalizeHash method so that when hash.Length != 64 it constructs a 64-char
string by repeating the full input sequence and then taking the first 64
characters (i.e., string.Concat(Enumerable.Repeat(hash, ...)) or a loop) so
short inputs like "a1b2c3d4" are expanded/truncated to preserve their full
nibble sequence rather than turning into "aaaa...". Ensure the method still
returns the original when length == 64.In
@src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs:
- Around line 19-20: NormalizeHash currently pads short inputs by repeating only
the first character, collapsing distinct short hashes into identical values;
change NormalizeHash to pad by repeating the whole input string (e.g., repeat
the short hash cyclically and then take the first 64 chars) so inputs like
"a1b2c3d4" remain distinguishable, keeping the method deterministic and still
returning exactly 64 characters.In
@src/Arius.Core.Tests/Shared/Hashes/HashValueObjectTests.cs:
- Around line 30-164: Add matrix tests for the TryParse contract for all hash
kinds: implement a test (e.g., TryParse_SucceedsForCanonicalValue) that calls
ContentHash.TryParse/ChunkHash.TryParse/FileTreeHash.TryParse with a valid
canonical hex string and asserts it returns true and the out value equals the
parsed canonical value (use ContentHash.Parse/ChunkHash.Parse/FileTreeHash.Parse
or FromDigest for comparison), and another test (e.g.,
TryParse_FailsForInvalidInputs) that calls TryParse with invalid inputs (empty,
wrong length, non-hex) and asserts it returns false and the out parameter equals
default(ContentHash)/default(ChunkHash)/default(FileTreeHash); use the same
MatrixDataSource pattern and the existing Parse helper switch as guidance to
locate the correct types and methods.
Nitpick comments:
In
@src/Arius.Benchmarks/raw/20260428T044504.151Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md:
- Line 1: The generated BenchmarkDotNet markdown lacks a language on the opening
fenced code block and misses blank lines around the table header, causing linter
warnings; update the post-processing step that emits the report so it modifies
the top fence from "" to "text" and ensures a blank line before and after
the table header row that begins with "| Method", or adjust the report
generation routine that produces the BenchmarkDotNet output to insert those
formatting fixes automatically.In
@src/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md:
- Around line 1-12: The fenced code block containing the BenchmarkDotNet output
(starting with "BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS") is missing a
language tag; update the opening backticks to include "text" (i.e., changetotext) so the block complies with markdownlint MD040 while preserving the
existing content and formatting.In
@src/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md:
- Around line 1-12: The fenced code block at the top of the report is missing a
language token (triggers markdownlint MD040); fix by changing the opening fence
fromtotext (or another appropriate language) so the block becomesthe report file (the first triple-backtick block) accordingly. In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs`: - Around line 68-82: The private static helper ComputeTarHash is only used by a single test and should be converted to a local function inside that test method in ArchiveRecoveryTests; move the body of ComputeTarHash into the test as a local method (remove private/static modifiers), keep the same parameters (ArchiveTestEnvironment env, ContentHash contentHash, byte[] content) and return type ChunkHash, and keep the internals (creating TarWriter, PaxTarEntry, and calling env.Encryption.ComputeHash followed by ChunkHash.Parse) unchanged; update the test call sites to use the new local function name and ensure streams are disposed the same way. In `@src/Arius.Core.Tests/Features/ChunkHydrationStatusQuery/ResolveFileHydrationStatusesHandlerTests.cs`: - Around line 28-46: The TypeThin branch is modelling thin files incorrectly; update the test matrix so TypeThin uses the real thin-pointer shape by setting resolvedChunkHash to BlobPaths.ThinChunk(contentHash) (not Chunk(chunkHash)) and make the ShardEntry for TypeThin point at the backing tarChunkHash (i.e., new ShardEntry(contentHash, tarChunkHash, ...)); also ensure FakeMetadataOnlyBlobContainerService.ConfigureChunk is invoked to create both the thin-pointer blob and the backing tar chunk (so the thin->tar relationship exists), or alternatively remove the TypeThin branch from the matrix if you prefer to avoid testing thin resolution here. In `@src/Arius.Core.Tests/Shared/Encryption/PassphraseEncryptionServiceTests.cs`: - Around line 123-129: The test currently opens a FileStream before calling svc.ComputeHashAsync(path) causing overlapping file handles; change the order so you first call await svc.ComputeHashAsync(path) and only then open the stream (await using var stream = File.OpenRead(path)) before calling await svc.ComputeHashAsync(stream), or alternatively compute the two hashes fully sequentially by disposing the stream between calls; update the test in PassphraseEncryptionServiceTests to ensure ComputeHashAsync(path) and ComputeHashAsync(stream) do not run with the same file handle open concurrently. In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs`: - Around line 32-83: The tests Deserialize_SkipsEntriesWithInvalidHashes_AndPreservesValidEntries, Deserialize_SkipsMalformedEntries_AndPreservesValidSiblings and Deserialize_SkipsFileEntriesWithEmptyOrWhitespaceNames_AndPreservesValidSiblings only assert count and names; update each test to also assert the surviving entries' concrete types and their hashes: after calling FileTreeBlobSerializer.Deserialize, find the entry for "healthy.txt" and assert it is a FileEntry and that its ContentHash equals the expected 64-char string (e.g., new string('a',64) or 'c' as used), and find "photos/" and assert it is a DirectoryEntry and its Hash equals the expected 64-char value (e.g., new string('b',64) or 'd'); use the blob.Entries collection and the entry's type/property names (FileEntry, DirectoryEntry, ContentHash/Hash) to locate and assert these values. In `@src/Arius.Core/Shared/Snapshot/SnapshotService.cs`: - Around line 34-41: The FileTreeHashJsonConverter class should be moved out of SnapshotService.cs into its own file named FileTreeHashJsonConverter.cs: create a new internal sealed class FileTreeHashJsonConverter in the same namespace as SnapshotService, copy the Read and Write overrides exactly, preserve usings and accessibility, and remove the converter definition from SnapshotService.cs so only one top-level class remains per file; ensure the project builds (no references need changing if the namespace is unchanged) or alternatively nest the converter inside a Snapshot serialization helper type if you prefer to keep it local. In `@src/Arius.Integration.Tests/Storage/BlobStorageServiceTests.cs`: - Around line 69-78: The test Upload_WithCoolTier_BlobHasCoolTier is misnamed because it only asserts existence; rename the test to reflect its actual assertion (e.g., Upload_WithCoolTier_BlobExists or Upload_UploadsBlob_WhenCoolTierRequested) or split the behavior: keep this test to check existence using svc.UploadAsync(..., BlobTier.Cool) and move any real-tier verification into an Azure-capability-gated test that inspects effective tier/rehydration; update the test method name and any references to Upload_WithCoolTier_BlobHasCoolTier accordingly.🪄 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:
3ae7556f-5c26-48bc-af5c-c0aa9be4818d⛔ Files ignored due to path filters (16)
src/Arius.Benchmarks/benchmark-tail.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T044504.151Z/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-20260428-064504.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T044504.151Z/benchmark-output.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T044504.151Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.csvis excluded by!**/*.csvsrc/Arius.Benchmarks/raw/20260428T045152.291Z/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-20260428-045152.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T045152.291Z/benchmark-output.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.csvis excluded by!**/*.csvsrc/Arius.Benchmarks/raw/20260428T091515.993Z/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-20260428-091516.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T091515.993Z/benchmark-output.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T091515.993Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.csvis excluded by!**/*.csvsrc/Arius.Benchmarks/raw/20260428T093552.360Z/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-20260428-093552.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T093552.360Z/benchmark-output.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T093552.360Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.csvis excluded by!**/*.csvsrc/Arius.Benchmarks/raw/20260428T101947.880Z/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-20260428-101948.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T101947.880Z/benchmark-output.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.csvis excluded by!**/*.csv📒 Files selected for processing (91)
.agents/skills/csharp-api-design/SKILL.mddocs/decisions/adr-0003-use-distinct-typed-hashes.mddocs/decisions/adr-0004-split-filetree-entry-hash-identities.mdsrc/Arius.Architecture.Tests/Arius.Architecture.Tests.csprojsrc/Arius.Architecture.Tests/Usings.cssrc/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csprojsrc/Arius.AzureBlob.Tests/Usings.cssrc/Arius.Benchmarks/raw/20260428T044504.151Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.mdsrc/Arius.Benchmarks/raw/20260428T044504.151Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.htmlsrc/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.mdsrc/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.htmlsrc/Arius.Benchmarks/raw/20260428T091515.993Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.mdsrc/Arius.Benchmarks/raw/20260428T091515.993Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.htmlsrc/Arius.Benchmarks/raw/20260428T093552.360Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.mdsrc/Arius.Benchmarks/raw/20260428T093552.360Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.htmlsrc/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.mdsrc/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.htmlsrc/Arius.Cli.Tests/Arius.Cli.Tests.csprojsrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayDoneTests.cssrc/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cssrc/Arius.Cli.Tests/Commands/Archive/ChunkUploadingHandlerDualLookupTests.cssrc/Arius.Cli.Tests/Commands/Archive/ContentHashToPathTests.cssrc/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Archive/ProgressCallbackIntegrationTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedFileSmallFilePathTests.cssrc/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cssrc/Arius.Cli.Tests/Commands/Restore/ChunkDownloadCompletedHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cssrc/Arius.Cli.Tests/Commands/Restore/TrackedDownloadLifecycleTests.cssrc/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cssrc/Arius.Cli.Tests/Usings.cssrc/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cssrc/Arius.Cli/Commands/Archive/ArchiveVerb.cssrc/Arius.Cli/ProgressState.cssrc/Arius.Core.Tests/Arius.Core.Tests.csprojsrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.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/ChunkIndex/ShardSerializerLocalTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceReadTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceUploadTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/Fakes/BlockingDeleteBlobContainerService.cssrc/Arius.Core.Tests/Shared/Encryption/PassphraseEncryptionServiceTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerStorageTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cssrc/Arius.Core.Tests/Shared/Hashes/HashValueObjectTests.cssrc/Arius.Core.Tests/Shared/Snapshot/SnapshotSerializerTests.cssrc/Arius.Core.Tests/Shared/Storage/BlobPathsTests.cssrc/Arius.Core.Tests/Usings.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommand.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cssrc/Arius.Core/Features/ListQuery/ListQuery.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkIndex/Shard.cssrc/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cssrc/Arius.Core/Shared/FileTree/FileTreeBlobSerializer.cssrc/Arius.Core/Shared/FileTree/FileTreeBuilder.cssrc/Arius.Core/Shared/FileTree/FileTreeModels.cssrc/Arius.Core/Shared/FileTree/FileTreeService.cssrc/Arius.Core/Shared/FileTree/ManifestEntry.cssrc/Arius.Core/Shared/Hashes/ChunkHash.cssrc/Arius.Core/Shared/Hashes/ContentHash.cssrc/Arius.Core/Shared/Hashes/FileTreeHash.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.Core/Shared/Storage/BlobConstants.cssrc/Arius.E2E.Tests/Usings.cssrc/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cssrc/Arius.E2E.Tests/Workflows/Steps/Helpers.cssrc/Arius.Explorer.Tests/Arius.Explorer.Tests.csprojsrc/Arius.Explorer.Tests/RepositoryExplorer/FileItemViewModelTests.cssrc/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cssrc/Arius.Explorer.Tests/Usings.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cssrc/Arius.Integration.Tests/Shared/FileTree/FileTreeBuilderIntegrationTests.cssrc/Arius.Integration.Tests/Snapshot/SnapshotServiceIntegrationTests.cssrc/Arius.Integration.Tests/Storage/BlobStorageServiceTests.cssrc/Arius.Integration.Tests/Usings.cssrc/Arius.Tests.Shared/AssemblyMarker.cssrc/Arius.Tests.Shared/Hashes/HashTestData.cssrc/Arius.Tests.Shared/IO/FileSystemHelper.cs💤 Files with no reviewable changes (6)
- .agents/skills/csharp-api-design/SKILL.md
- src/Arius.Cli.Tests/Commands/Archive/TrackedFileLargeFilePathTests.cs
- src/Arius.Cli.Tests/Commands/Archive/TrackedFileSmallFilePathTests.cs
- src/Arius.Cli.Tests/Commands/Restore/RestoreNotificationHandlerTests.cs
- src/Arius.Cli.Tests/Commands/Archive/ContentHashToPathTests.cs
- src/Arius.Tests.Shared/AssemblyMarker.cs
✅ Files skipped from review due to trivial changes (21)
- src/Arius.Architecture.Tests/Arius.Architecture.Tests.csproj
- src/Arius.Benchmarks/raw/20260428T093552.360Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.html
- src/Arius.Core.Tests/Usings.cs
- src/Arius.Tests.Shared/IO/FileSystemHelper.cs
- src/Arius.E2E.Tests/Usings.cs
- src/Arius.AzureBlob.Tests/Usings.cs
- src/Arius.Architecture.Tests/Usings.cs
- src/Arius.Benchmarks/raw/20260428T093552.360Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md
- src/Arius.Cli.Tests/Usings.cs
- src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayDoneTests.cs
- src/Arius.Benchmarks/raw/20260428T091515.993Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.html
- src/Arius.Benchmarks/raw/20260428T045152.291Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.html
- src/Arius.Integration.Tests/Usings.cs
- src/Arius.Cli.Tests/Arius.Cli.Tests.csproj
- src/Arius.Core.Tests/Arius.Core.Tests.csproj
- src/Arius.Benchmarks/raw/20260428T101947.880Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.html
- src/Arius.Explorer.Tests/RepositoryExplorer/FileItemViewModelTests.cs
- src/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.cs
- src/Arius.Benchmarks/raw/20260428T044504.151Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.html
- src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs
- src/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cs
🚧 Files skipped from review as they are similar to previous changes (16)
- src/Arius.Explorer.Tests/Usings.cs
- src/Arius.Tests.Shared/Hashes/HashTestData.cs
- src/Arius.Cli/Commands/Archive/ArchiveVerb.cs
- src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs
- src/Arius.Core/Shared/Hashes/ContentHash.cs
- src/Arius.Core/Shared/Hashes/ChunkHash.cs
- src/Arius.Core.Tests/Shared/Snapshot/SnapshotSerializerTests.cs
- src/Arius.Cli.Tests/Commands/Archive/ProgressCallbackIntegrationTests.cs
- src/Arius.Cli.Tests/Commands/Archive/TrackedTarLifecycleTests.cs
- src/Arius.Core/Features/ChunkHydrationStatusQuery/ChunkHydrationStatusQuery.cs
- src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs
- src/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerLocalTests.cs
- src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs
- src/Arius.Core/Shared/FileTree/FileTreeService.cs
- src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs
- src/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cs
Comment on lines
181
to
185
| [Test] | ||
| public void Shard_PrefixOf_ShortHash_Throws() | ||
| { | ||
| Should.Throw<ArgumentException>(() => Shard.PrefixOf("abc")); | ||
| Should.Throw<FormatException>(() => ContentHash.Parse("abc")); | ||
| } |
Contributor
There was a problem hiding this comment.
This test no longer exercises Shard.PrefixOf.
Line 184 only checks that ContentHash.Parse("abc") throws, so the test now passes even if Shard.PrefixOf(...) changes. Please either rename it to reflect parse validation or keep a separate PrefixOf assertion so the test still covers the behavior its name promises.
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.Core.Tests/Shared/ChunkIndex/ShardTests.cs` around lines 181 - 185,
The test named Shard_PrefixOf_ShortHash_Throws no longer calls Shard.PrefixOf
and only asserts ContentHash.Parse("abc") throws; either rename the test to
reflect parse validation (e.g., ContentHash_Parse_ShortHash_Throws) or restore
coverage by adding an explicit PrefixOf assertion: call Shard.PrefixOf with
appropriately constructed inputs that exercise the prefix validation (for
example, create a valid ContentHash and a deliberately short/invalid hash input
or use the API overload that accepts raw bytes) and assert
Should.Throw<FormatException>(() => Shard.PrefixOf(...)); update the test method
name if you choose the rename option.
…vanranst/Arius7 into feat/cleanup-compute-hash
Stop silently skipping malformed filetree and tar entries so restore surfaces repository corruption instead of omitting files.
woutervanranst
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs (2)
189-198:⚠️ Potential issue | 🟠 MajorFail restore when a file's content hash has no chunk-index entry.
This path currently logs and continues, so a requested file can disappear from the restore while the handler still returns
Success = true. For restore, a missing chunk-index mapping is repository corruption/inconsistency and should be surfaced as a failure instead of being silently omitted.🛠️ Minimal fix
- var unresolved = new List<FileToRestore>(); - foreach (var file in toRestore) { if (!indexEntries.TryGetValue(file.ContentHash, out var entry)) - { - _logger.LogWarning("Content hash not found in index, skipping: {Hash} ({Path})", file.ContentHash, file.RelativePath); - unresolved.Add(file); - continue; - } + throw new InvalidOperationException( + $"Content hash '{file.ContentHash}' for '{file.RelativePath}' was not found in the chunk index."); if (!filesByChunkHash.TryGetValue(entry.ChunkHash, out var list)) filesByChunkHash[entry.ChunkHash] = list = new List<FileToRestore>();Based on learnings, Arius is a backup/archive tool that prioritizes recoverability and correctness over throughput.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs` around lines 189 - 198, The loop that handles toRestore currently treats a missing indexEntries mapping as a warning and continues, which allows a restore to succeed despite repository inconsistency; in the TryGetValue branch inside the foreach over toRestore (where filesByChunkHash and unresolved are managed and _logger.LogWarning is called), replace the silent-continue behavior with a hard failure: log an error (use _logger.LogError with the hash and path), add the FileToRestore to unresolved if you still want to collect failures, and then surface the failure immediately by either throwing a descriptive exception (e.g., InvalidOperationException) or returning a failed result (ensure the handler's result Success=false) so the restore does not report success when a chunk-index mapping is missing.
245-266:⚠️ Potential issue | 🟠 MajorDon't treat
ChunkHydrationStatus.Unknownas warning-only.If storage says the chunk blob is unknown, every file behind that chunk is skipped from the restore but the command can still complete successfully with
ChunksPendingRehydration = 0. That makes missing repository data look like a clean restore.🛠️ Minimal fix
switch (hydrationStatus) { case ChunkHydrationStatus.Unknown: - _logger.LogWarning("Chunk blob not found: {ChunkHash}", chunkHash); - break; + throw new InvalidOperationException( + $"Chunk '{chunkHash}' referenced by restore metadata was not found in storage."); case ChunkHydrationStatus.RehydrationPending: rehydrationPending.Add(chunkHash); break;Based on learnings, Arius is a backup/archive tool that prioritizes recoverability and correctness over throughput.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs` around lines 245 - 266, The handler currently treats ChunkHydrationStatus.Unknown as a mere LogWarning, which lets restores succeed silently with missing data; update RestoreCommandHandler to treat Unknown as a fatal/missing-chunk condition: when hydrationStatus == ChunkHydrationStatus.Unknown, log an error via _logger.LogError with the chunkHash, add the chunkHash to a new missingChunks collection (or an existing failure indicator), and ensure the final restore result uses that collection to set ChunksPendingRehydration/overall status to failed (or non-zero) so the command does not report success when repository data is missing; modify any downstream result construction to include missingChunks.
♻️ Duplicate comments (1)
src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs (1)
19-20:⚠️ Potential issue | 🟡 MinorNormalizeHash still collapses distinct short fixtures
This helper still maps inputs like
a1b2andaaaato the same 64-char value, which can hide collisions in typed-hash test coverage.Suggested fix
private static string NormalizeHash(string hash) - => hash.Length == 64 ? hash : hash[0].ToString().PadRight(64, char.ToLowerInvariant(hash[0])); +{ + ArgumentException.ThrowIfNullOrEmpty(hash); + var normalized = hash.ToLowerInvariant(); + if (normalized.Length == 64) + return normalized; + + return string.Concat(Enumerable.Repeat(normalized, 64 / normalized.Length + 1))[..64]; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs` around lines 19 - 20, NormalizeHash currently pads short inputs using only the first character (char.ToLowerInvariant(hash[0])), causing distinct short strings like "a1b2" and "aaaa" to map to the same 64-char value; update NormalizeHash to produce deterministic, distinct 64-char outputs by padding based on the full input (e.g., cycle the input characters to fill to length 64 or append a deterministic digest of the input such as SHA256 hex until length 64) while preserving existing behavior for already-64-char inputs; change the implementation in the NormalizeHash helper to use the full input for padding rather than only hash[0].
🧹 Nitpick comments (2)
src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs (1)
161-162: Rename this test to reference filetree corruption, not snapshot corruption.The malformed hash is in the serialized filetree payload, so the current name points future readers at the wrong layer when this fails.
Based on learnings, use precise domain terminology consistently in code, tests, docs, and reviews: binary file, pointer file, FilePair, hash (content hash vs chunk hash), chunk, chunk index, shard, filetree, and snapshot.
🤖 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` around lines 161 - 162, Rename the test Handle_InvalidSnapshotContentHash_FailsRestore to reflect filetree corruption (e.g., Handle_FiletreeInvalidContentHash_FailsRestore or Handle_FiletreeCorruption_InvalidContentHash_FailsRestore) because the malformed hash lives in the serialized filetree payload; update the test method name and any references to it in RestoreCommandHandlerTests so readers see the correct domain layer (filetree) rather than snapshot.src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs (1)
88-91: Strengthen timestamp round-trip assertions
ShouldNotBe(default)is too weak for this test; assert exact timestamp equality to validate serializer/deserializer fidelity.Suggested fix
- file.Created.ShouldNotBe(default); - file.Modified.ShouldNotBe(default); + file.Created.ShouldBe(s_created); + file.Modified.ShouldBe(s_modified);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.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs` around lines 88 - 91, The timestamps assertions in FileTreeBlobSerializerTests are too weak; replace the two ShouldNotBe(default) checks on file.Created and file.Modified with exact equality checks against the original values that were serialized (e.g., assert file.Created.ShouldBe(expectedCreated) and file.Modified.ShouldBe(expectedModified)), obtaining expectedCreated/expectedModified from the original entry used before round-trip (the source entry in the test that produced back via the serializer), so the test verifies exact round-trip fidelity for FileEntry.Created and FileEntry.Modified rather than just non-default values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cs`:
- Around line 189-198: The loop that handles toRestore currently treats a
missing indexEntries mapping as a warning and continues, which allows a restore
to succeed despite repository inconsistency; in the TryGetValue branch inside
the foreach over toRestore (where filesByChunkHash and unresolved are managed
and _logger.LogWarning is called), replace the silent-continue behavior with a
hard failure: log an error (use _logger.LogError with the hash and path), add
the FileToRestore to unresolved if you still want to collect failures, and then
surface the failure immediately by either throwing a descriptive exception
(e.g., InvalidOperationException) or returning a failed result (ensure the
handler's result Success=false) so the restore does not report success when a
chunk-index mapping is missing.
- Around line 245-266: The handler currently treats ChunkHydrationStatus.Unknown
as a mere LogWarning, which lets restores succeed silently with missing data;
update RestoreCommandHandler to treat Unknown as a fatal/missing-chunk
condition: when hydrationStatus == ChunkHydrationStatus.Unknown, log an error
via _logger.LogError with the chunkHash, add the chunkHash to a new
missingChunks collection (or an existing failure indicator), and ensure the
final restore result uses that collection to set
ChunksPendingRehydration/overall status to failed (or non-zero) so the command
does not report success when repository data is missing; modify any downstream
result construction to include missingChunks.
---
Duplicate comments:
In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs`:
- Around line 19-20: NormalizeHash currently pads short inputs using only the
first character (char.ToLowerInvariant(hash[0])), causing distinct short strings
like "a1b2" and "aaaa" to map to the same 64-char value; update NormalizeHash to
produce deterministic, distinct 64-char outputs by padding based on the full
input (e.g., cycle the input characters to fill to length 64 or append a
deterministic digest of the input such as SHA256 hex until length 64) while
preserving existing behavior for already-64-char inputs; change the
implementation in the NormalizeHash helper to use the full input for padding
rather than only hash[0].
---
Nitpick comments:
In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`:
- Around line 161-162: Rename the test
Handle_InvalidSnapshotContentHash_FailsRestore to reflect filetree corruption
(e.g., Handle_FiletreeInvalidContentHash_FailsRestore or
Handle_FiletreeCorruption_InvalidContentHash_FailsRestore) because the malformed
hash lives in the serialized filetree payload; update the test method name and
any references to it in RestoreCommandHandlerTests so readers see the correct
domain layer (filetree) rather than snapshot.
In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cs`:
- Around line 88-91: The timestamps assertions in FileTreeBlobSerializerTests
are too weak; replace the two ShouldNotBe(default) checks on file.Created and
file.Modified with exact equality checks against the original values that were
serialized (e.g., assert file.Created.ShouldBe(expectedCreated) and
file.Modified.ShouldBe(expectedModified)), obtaining
expectedCreated/expectedModified from the original entry used before round-trip
(the source entry in the test that produced back via the serializer), so the
test verifies exact round-trip fidelity for FileEntry.Created and
FileEntry.Modified rather than just non-default values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 279fcf60-b9bd-4390-bbed-6e250eaf8b45
📒 Files selected for processing (5)
src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBlobSerializerTests.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/Shared/FileTree/FileTreeBlobSerializer.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arius.Core/Shared/FileTree/FileTreeBlobSerializer.cs
This was referenced Apr 28, 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
ContentHash,ChunkHash, andFileTreeHashvalue objects across encryption, chunk index/storage, file tree, snapshot, archive, and restore flowsFileTreeentries into explicit file and directory entry types, keep persisted hash formats as canonical lowercase hex strings, and add file-path hashing with optional progress supportTest Plan
dotnet test --project C:\Users\WouterVanRanst\repos\Arius7\src\Arius.Core.Tests\Arius.Core.Tests.csproj --treenode-filter /*/*/ArchiveRecoveryTests/*|/*/*/GoldenFileDecryptionTests/*|/*/*/ResolveFileHydrationStatusesHandlerTests/*|/*/*/ListQueryHandlerTests/*dotnet test --project C:\Users\WouterVanRanst\repos\Arius7\src\Arius.Integration.Tests\Arius.Integration.Tests.csproj --treenode-filter /*/*/ChunkIndexServiceIntegrationTests/*dotnet test --project C:\Users\WouterVanRanst\repos\Arius7\src\Arius.Integration.Tests\Arius.Integration.Tests.csproj --treenode-filter /*/*/SnapshotServiceIntegrationTests/*dotnet test --project C:\Users\WouterVanRanst\repos\Arius7\src\Arius.Integration.Tests\Arius.Integration.Tests.csproj --treenode-filter /*/*/FileTreeBuilderIntegrationTests/*|/*/*/RecoveryScriptTests/*dotnet test --project C:\Users\WouterVanRanst\repos\Arius7\src\Arius.E2E.Tests\Arius.E2E.Tests.csprojslopwatch analyzeSummary by CodeRabbit
Refactor
Documentation