Introduce core filesystem domain types#81
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces typed repository-relative path primitives ( ChangesCore Path Typing Sweep
Editor / Docs & Auxiliary (separate DAG)
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Core as ArchiveCommandHandler
participant FS as RelativeFileSystem
participant Blob as IBlobContainerService
participant Tree as FileTreeService
CLI->>Core: ArchiveCommand(opts with RelativePath)
Core->>FS: Enumerate(LocalDirectory) -> yields FilePair (FileScannedEvent)
Core->>Core: Hash & route, build sealed in-memory tar (SealedTar)
Core->>Blob: Upload chunk blobs (RelativePath)
Core->>Tree: WriteFileTreeEntry(RelativePath)
Core-->>CLI: Publish progress events (FileHashing/FileHashed with RelativePath)
sequenceDiagram
autonumber
participant CLI as CLI
participant Core as RestoreCommandHandler
participant Snap as SnapshotService
participant Tree as FileTreeService
participant Blob as IBlobContainerService
participant FS as RelativeFileSystem
CLI->>Core: RestoreCommand(TargetPath: RelativePath?)
Core->>Snap: ResolveAsync() -> snapshot blob (RelativePath)
Core->>Tree: ReadAsync(snapshot) -> stream entries
Core->>Core: WalkTree(RelativePath) -> filtered file list
Core->>Blob: Download chunk/tar (RelativePath)
Core->>FS: CreateFile / SetTimestamps / (ToPointerPath + WriteAllText)
Core-->>CLI: Publish FileRestoredEvent(RelativePath)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #81 +/- ##
==========================================
+ Coverage 78.17% 78.77% +0.59%
==========================================
Files 71 78 +7
Lines 5091 5277 +186
Branches 690 722 +32
==========================================
+ Hits 3980 4157 +177
+ Misses 934 929 -5
- Partials 177 191 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
src/Arius.Core.Tests/Shared/Storage/BlobPathsTests.cs (1)
9-39: ⚡ Quick winAdd coverage for thin and rehydrated blob helpers.
This suite validates
ChunkPathandFileTreePath, but the changed storage flow also depends onThinChunkPathandChunkRehydratedPath. Those are persisted blob keys too, so a naming regression there would break uploads or rehydration lookups without a failing unit test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core.Tests/Shared/Storage/BlobPathsTests.cs` around lines 9 - 39, Add unit tests to the BlobPathsTests suite that cover the thin and rehydrated chunk helpers: call BlobPaths.ThinChunkPath(chunkHash) and BlobPaths.ChunkRehydratedPath(chunkHash) (and the corresponding string-returning helpers if present) and assert they produce canonical RelativePath.Parse(...) values and matching string forms, mirroring the existing ChunkPath/FileTreePath tests so regressions in ThinChunkPath and ChunkRehydratedPath naming are detected.src/Arius.Core/Shared/LocalFile/ArchivePathCollisionValidator.cs (1)
15-38: ⚡ Quick winKeep collision observation local to preserve comparer invariants.
Line 28 exposes
ObservewithIDictionary<string, RelativePath>, which lets callers pass a case-sensitive map and accidentally bypass the validator’s case-insensitive guarantee. Make it a local function insideValidate(or private andDictionary-typed).Suggested refactor
public static void Validate(IEnumerable<FilePair> pairs) { + ArgumentNullException.ThrowIfNull(pairs); var firstByKey = new Dictionary<string, RelativePath>(StringComparer.OrdinalIgnoreCase); foreach (var pair in pairs) { - Observe(firstByKey, pair.Path); + Observe(pair.Path); } -} - -public static void Observe(IDictionary<string, RelativePath> firstByKey, RelativePath path) -{ - var key = path.ToString(); - if (firstByKey.TryGetValue(key, out var existing) && existing != path) - { - throw new InvalidOperationException( - $"Archive input contains case-insensitive path collisions: '{existing}' and '{path}'."); - } - - firstByKey[key] = path; + + void Observe(RelativePath path) + { + var key = path.ToString(); + if (firstByKey.TryGetValue(key, out var existing) && existing != path) + { + throw new InvalidOperationException( + $"Archive input contains case-insensitive path collisions: '{existing}' and '{path}'."); + } + + firstByKey[key] = path; + } }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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/LocalFile/ArchivePathCollisionValidator.cs` around lines 15 - 38, The Observe helper leaks the comparer contract by accepting IDictionary<string, RelativePath>, allowing callers to pass a case-sensitive map; make Observe local to Validate (or change it to a private method that takes Dictionary<string, RelativePath>) so it must use the firstByKey created with StringComparer.OrdinalIgnoreCase; update references to call the new local function Observe (or the private Dictionary-typed method) and keep the existing behavior of comparing path.ToString() and throwing on collisions between different RelativePath instances.src/Arius.Core/Shared/FileTree/FileTreeService.cs (1)
361-367: 💤 Low valueConsider using
RelativeFileSystemforExistsInRemoteconsistency.
ValidateAsyncuses_diskCacheFileSystem.FileExists(relativePath)for marker checks (line 330), butExistsInRemoteuses rawFile.Exists(FileTreePaths.GetCachePath(...)). Both are functionally correct since they resolve to the same path, but using the filesystem boundary consistently would align better with the IO quarantine spec.Proposed change
public bool ExistsInRemote(FileTreeHash hash) { if (!_validated) throw new InvalidOperationException($"{nameof(ExistsInRemote)} must not be called before {nameof(ValidateAsync)}."); - return File.Exists(FileTreePaths.GetCachePath(_diskCacheDir, hash)); + return _diskCacheFileSystem.FileExists(RelativePath.Parse(hash.ToString())); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/FileTree/FileTreeService.cs` around lines 361 - 367, ExistsInRemote currently calls File.Exists(FileTreePaths.GetCachePath(_diskCacheDir, hash)) which bypasses the IO boundary used elsewhere; replace that raw File.Exists call with the disk-file-system check used in ValidateAsync by computing the same relative path and calling _diskCacheFileSystem.FileExists(relativePath) (use the same helper/logic ValidateAsync uses to derive relativePath from FileTreeHash/FileTreePaths) while keeping the existing _validated check and exception.src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs (1)
275-280: ⚡ Quick winSync-over-async pattern may block thread pool threads.
WriteAllBytesAsync(...).GetAwaiter().GetResult()blocks the calling thread. SinceSaveToL2is only called from async methods (FlushAsyncandLoadShardAsync), consider makingSaveToL2async to avoid thread pool starvation under load.♻️ Proposed async refactor
- private void SaveToL2(string prefix, Shard shard) + private async Task SaveToL2Async(string prefix, Shard shard, CancellationToken cancellationToken) { var path = RelativePath.Parse(prefix); var bytes = ShardSerializer.SerializeLocal(shard); - _l2FileSystem.WriteAllBytesAsync(path, bytes, CancellationToken.None).GetAwaiter().GetResult(); + await _l2FileSystem.WriteAllBytesAsync(path, bytes, cancellationToken); }Then update callers in
FlushAsyncandLoadShardAsynctoawait SaveToL2Async(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs` around lines 275 - 280, The SaveToL2 method synchronously blocks on an async call; change it to an async Task method (e.g., SaveToL2Async) that awaits _l2FileSystem.WriteAllBytesAsync(path, bytes, CancellationToken.None) after computing path via RelativePath.Parse and bytes via ShardSerializer.SerializeLocal(shard), then update callers (FlushAsync and LoadShardAsync) to await SaveToL2Async(...) instead of calling the sync helper to avoid sync-over-async blocking; keep the same logic and exception behavior and propagate the CancellationToken if available.src/Arius.Core/Shared/RepositoryPaths.cs (1)
33-43: 💤 Low valueConsider extracting the path conversion pattern.
The
relativePath.ToString().Replace('/', Path.DirectorySeparatorChar)pattern is repeated acrossGetChunkIndexCacheRoot,GetFileTreeCacheRoot,GetSnapshotCacheRoot, andGetLogsRoot. SinceLocalDirectoryalready handles platform paths, consider adding a helper method toLocalDirectoryor creating a local helper to reduce duplication.♻️ Example refactor
+ private static LocalDirectory GetCacheRoot(string accountName, string containerName, RelativePath relativePath) => + LocalDirectory.Parse(Path.Combine( + GetRepositoryRoot(accountName, containerName).ToString(), + relativePath.ToString().Replace('/', Path.DirectorySeparatorChar))); + internal static LocalDirectory GetChunkIndexCacheRoot(string accountName, string containerName) => - LocalDirectory.Parse(Path.Combine(GetRepositoryRoot(accountName, containerName).ToString(), ChunkIndexCacheRelativePath.ToString().Replace('/', Path.DirectorySeparatorChar))); + GetCacheRoot(accountName, containerName, ChunkIndexCacheRelativePath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/RepositoryPaths.cs` around lines 33 - 43, The four methods GetChunkIndexCacheRoot, GetFileTreeCacheRoot, GetSnapshotCacheRoot, and GetLogsRoot duplicate the same relativePath.ToString().Replace('/', Path.DirectorySeparatorChar) logic; extract that conversion into a single helper (either a new LocalDirectory.FromRelativePath/NormalizeRelativePath on LocalDirectory or a private static helper in RepositoryPaths) and update each method to call the helper and then LocalDirectory.Parse(Path.Combine(GetRepositoryRoot(...).ToString(), helperResult)); reference ChunkIndexCacheRelativePath, FileTreeCacheRelativePath, SnapshotCacheRelativePath, LogsRelativePath and LocalDirectory.Parse/GetRepositoryRoot when making the change.openspec/specs/list-query/spec.md (1)
95-102: ⚡ Quick winSpecify invalid
LocalPathbehavior in the public contract.
LocalPathis now a user-facing input, but this requirement never says whether it must be an absolute local directory or what error is returned when parsing fails. Since the implementation now routes through the typed local-root boundary, this omission leaves a user-visible failure mode outside the spec. Please add an invalid-LocalPathscenario so callers get deterministic behavior instead of an implementation-defined parse failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/specs/list-query/spec.md` around lines 95 - 102, The spec omits deterministic behavior for invalid LocalPath inputs to ListQuery; add a public contract that LocalPath, when present, MUST be a canonical absolute directory path (or explicitly stated form) and that the handler SHALL validate it via the local-root/filesystem domain boundary and return a defined error (e.g., an InvalidLocalPath / 400-style response with a clear message) rather than an implementation-defined parse failure; also document that absent LocalPath implies cloud-only merge, and describe that on validation failure the merge is aborted and no partial results are yielded (affecting the two-phase algorithm and FilePair handling).src/Arius.Core/Shared/FileTree/FileTreeStagingWriter.cs (1)
112-125: 💤 Low valuePotential duplicate directory entries for nested paths.
When multiple files share parent directories (e.g.,
a/b/c.txtanda/b/d.txt), each call toAppendDirectoryEntriesAsyncwill append directory entries fora/anda/b/even if they were already written by a previous file. While this may be acceptable if downstream processing handles duplicates, it could cause unnecessary disk I/O and larger staging files.Consider whether the staging file format tolerates duplicates or if deduplication is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/FileTree/FileTreeStagingWriter.cs` around lines 112 - 125, AppendDirectoryEntriesAsync currently writes directory lines for every ancestor segment even if already emitted, causing duplicate entries and extra I/O; add deduplication by tracking emitted directory ids (use FileTreePaths.GetStagingDirectoryId(currentPath) as the unique key) in a per-writer HashSet (e.g., _emittedDirectoryIds) and check that set before calling AppendLineAsync(FileTreeSerializer.SerializePersistedDirectoryEntryLine(...)); update AppendDirectoryEntriesAsync to skip appending when the directoryId is already in the set and add it when writing.src/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cs (1)
132-177: 💤 Low valueRemove dead code:
EnumerateFilesDepthFirstmethod is unreachable.The private method
EnumerateFilesDepthFirst(lines 132–177) is not called from anywhere in the codebase. After refactoring to useRelativeFileSystem.EnumerateFiles(), this method became unreachable. Remove it to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cs` around lines 132 - 177, The private method EnumerateFilesDepthFirst is dead/unreferenced and should be removed; delete the entire EnumerateFilesDepthFirst method from LocalFileEnumerator.cs, ensure there are no remaining callers or private references to EnumerateFilesDepthFirst (search for its identifier), rebuild/run tests to confirm nothing breaks, and commit the removal to reduce maintenance overhead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/csharp-coding-standards/performance-and-api-design.md:
- Around line 194-205: The second overloaded Sum is non-compilable because C#
doesn't allow overload resolution to differ only by the params modifier; replace
the params ReadOnlySpan<int> overload with a params int[] convenience overload
named Sum(params int[] values) and forward it to the ReadOnlySpan<int>
implementation by creating a ReadOnlySpan<int> from the incoming array (e.g.,
call the existing Sum(ReadOnlySpan<int>) with a new ReadOnlySpan<int>(values));
update references to Sum accordingly.
In @.agents/skills/csharp-coding-standards/SKILL.md:
- Line 2: The AGENTS.md routing still references the old skill name
"modern-csharp-coding-standards"; update the routing entry (the reference at
AGENTS.md:77) to use the new skill name "csharp-coding-standards" so agents can
find the skill; locate the routing table or mapping that mentions
"modern-csharp-coding-standards" and replace it with "csharp-coding-standards",
keeping any surrounding keys or metadata intact.
In `@docs/decisions/adr-0008-introduce-internal-filesystem-domain-types.md`:
- Line 179: Update the stale OpenSpec path reference string
"openspec/changes/introduce-core-filesystem-domain-types/" to the archived
location
"openspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/" in
the ADR text (search for that exact substring in the document, e.g., the
sentence containing that path on line 179), and scan the rest of the file for
any other occurrences of the old path and replace them similarly so all
references point to the archived OpenSpec change.
- Around line 109-110: ADR line claiming "RelativeFileSystem ... is the only
place that should use direct System.IO APIs" is inaccurate because other types
still call System.IO directly (notably FileTreeService, FileTreePaths,
RepositoryPaths); update the ADR to either narrow the claim and list those
exceptions (reference RelativeFileSystem, LocalDirectory, FileTreeService,
FileTreePaths, RepositoryPaths) or state the migration is incomplete and remove
the exclusivity language until all direct System.IO usages are migrated to
RelativeFileSystem, then re-run the ADR finalization.
In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`:
- Around line 358-359: Add a null guard before calling ShouldContain on
restoreResult.ErrorMessage: assert that restoreResult.ErrorMessage is not null
(for example using ShouldNotBeNull on restoreResult.ErrorMessage) immediately
before the ShouldContain call so the test cannot throw a null reference; update
the test around the assertions that reference restoreResult and
restoreResult.ErrorMessage (the existing restoreResult.Success.ShouldBeFalse()
and the failing ShouldContain) to include this null check.
In `@src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs`:
- Around line 97-98: The cast `_logger as ILogger<LocalFileEnumerator>` in
ArchiveCommandHandler.EnumerateFilePairs can produce null and suppress logs;
modify ArchiveCommandHandler to accept an ILoggerFactory (or
ILogger<LocalFileEnumerator>) via constructor injection and replace the cast
with a correctly-typed logger when creating LocalFileEnumerator (e.g., use
loggerFactory.CreateLogger<LocalFileEnumerator>() or pass the injected
ILogger<LocalFileEnumerator>), so enumeration warnings from LocalFileEnumerator
are actually logged.
In `@src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs`:
- Around line 13-14: BuildFiles() is incorrectly using
StringComparer.OrdinalIgnoreCase which collapses sibling names that differ only
by case; change the dictionaries (the one currently constructed as new
Dictionary<string, LocalFileState>(StringComparer.OrdinalIgnoreCase) and the
similar dictionaries at the other spots) to use a case-sensitive comparer (e.g.,
StringComparer.Ordinal or the default) so keys preserve exact filename identity,
and add explicit case-insensitive collision detection: when inserting a new key
into LocalFileState maps, check for any existing key that equals the new key
under OrdinalIgnoreCase but not under Ordinal and surface/report that collision
(throw or record an error) instead of silently merging. Ensure this logic is
applied inside BuildFiles() and the other analogous blocks that build
per-directory snapshots so case-only conflicts are flagged consistently.
In `@src/Arius.Core/Shared/FileSystem/LocalDirectory.cs`:
- Around line 78-85: IsContained currently fails for repository roots that are
filesystem roots and rejects Windows paths that differ only by case; update
IsContained to (1) ensure you don't append a second separator when Value already
ends with Path.DirectorySeparatorChar or is a path root (use Value as-is if it
already ends with the separator or use Path.GetPathRoot(Value) to detect roots)
so descendant paths like "C:\foo" are matched, and (2) use a case-insensitive
comparison on Windows (StringComparison.OrdinalIgnoreCase when
Path.DirectorySeparatorChar == '\\', otherwise StringComparison.Ordinal) so
TryGetRelativePath() and Resolve() don't reject valid paths that differ only by
case. Ensure the logic still returns true when fullPath equals Value and still
checks StartsWith against the normalized root-with-single-separator variant.
In `@src/Arius.Core/Shared/FileSystem/PathSegment.cs`:
- Around line 26-41: PathSegment.TryParse must reject Windows-invalid filenames:
add checks after the existing control/sep checks to (1) reject names that end
with a space or dot (value.EndsWith(' ') || value.EndsWith('.')), (2) reject any
characters invalid on Windows like <>:"|?* (check for any of "<>:\\"\"|?*"), and
(3) reject reserved device names (case-insensitive) by splitting value at the
first '.' and comparing the base token against the set {CON, PRN, AUX, NUL,
COM1..COM9, LPT1..LPT9}; if any of these checks fail set segment = default and
return false. Apply these checks inside PathSegment.TryParse (referencing the
TryParse method and the PathSegment constructor) so Linux-archived names that
would fail on Windows are rejected at the domain boundary.
In `@src/Arius.Core/Shared/FileSystem/RelativePathPointerExtensions.cs`:
- Around line 24-44: Make pointer conversion stricter: in ToPointerPath ensure
the input string does not already end with PointerSuffix (throw if it does) and
also forbid converting a root-equivalent by throwing when value.Length ==
PointerSuffix.Length; in ToBinaryPath after trimming PointerSuffix verify the
resulting binaryPath is non-empty (throw if empty) and does not itself end with
PointerSuffix (throw to reject double-suffix pointer paths) before calling
RelativePath.Parse; reference ToPointerPath, ToBinaryPath, PointerSuffix and
RelativePath.Parse when applying these checks.
---
Nitpick comments:
In `@openspec/specs/list-query/spec.md`:
- Around line 95-102: The spec omits deterministic behavior for invalid
LocalPath inputs to ListQuery; add a public contract that LocalPath, when
present, MUST be a canonical absolute directory path (or explicitly stated form)
and that the handler SHALL validate it via the local-root/filesystem domain
boundary and return a defined error (e.g., an InvalidLocalPath / 400-style
response with a clear message) rather than an implementation-defined parse
failure; also document that absent LocalPath implies cloud-only merge, and
describe that on validation failure the merge is aborted and no partial results
are yielded (affecting the two-phase algorithm and FilePair handling).
In `@src/Arius.Core.Tests/Shared/Storage/BlobPathsTests.cs`:
- Around line 9-39: Add unit tests to the BlobPathsTests suite that cover the
thin and rehydrated chunk helpers: call BlobPaths.ThinChunkPath(chunkHash) and
BlobPaths.ChunkRehydratedPath(chunkHash) (and the corresponding string-returning
helpers if present) and assert they produce canonical RelativePath.Parse(...)
values and matching string forms, mirroring the existing ChunkPath/FileTreePath
tests so regressions in ThinChunkPath and ChunkRehydratedPath naming are
detected.
In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs`:
- Around line 275-280: The SaveToL2 method synchronously blocks on an async
call; change it to an async Task method (e.g., SaveToL2Async) that awaits
_l2FileSystem.WriteAllBytesAsync(path, bytes, CancellationToken.None) after
computing path via RelativePath.Parse and bytes via
ShardSerializer.SerializeLocal(shard), then update callers (FlushAsync and
LoadShardAsync) to await SaveToL2Async(...) instead of calling the sync helper
to avoid sync-over-async blocking; keep the same logic and exception behavior
and propagate the CancellationToken if available.
In `@src/Arius.Core/Shared/FileTree/FileTreeService.cs`:
- Around line 361-367: ExistsInRemote currently calls
File.Exists(FileTreePaths.GetCachePath(_diskCacheDir, hash)) which bypasses the
IO boundary used elsewhere; replace that raw File.Exists call with the
disk-file-system check used in ValidateAsync by computing the same relative path
and calling _diskCacheFileSystem.FileExists(relativePath) (use the same
helper/logic ValidateAsync uses to derive relativePath from
FileTreeHash/FileTreePaths) while keeping the existing _validated check and
exception.
In `@src/Arius.Core/Shared/FileTree/FileTreeStagingWriter.cs`:
- Around line 112-125: AppendDirectoryEntriesAsync currently writes directory
lines for every ancestor segment even if already emitted, causing duplicate
entries and extra I/O; add deduplication by tracking emitted directory ids (use
FileTreePaths.GetStagingDirectoryId(currentPath) as the unique key) in a
per-writer HashSet (e.g., _emittedDirectoryIds) and check that set before
calling
AppendLineAsync(FileTreeSerializer.SerializePersistedDirectoryEntryLine(...));
update AppendDirectoryEntriesAsync to skip appending when the directoryId is
already in the set and add it when writing.
In `@src/Arius.Core/Shared/LocalFile/ArchivePathCollisionValidator.cs`:
- Around line 15-38: The Observe helper leaks the comparer contract by accepting
IDictionary<string, RelativePath>, allowing callers to pass a case-sensitive
map; make Observe local to Validate (or change it to a private method that takes
Dictionary<string, RelativePath>) so it must use the firstByKey created with
StringComparer.OrdinalIgnoreCase; update references to call the new local
function Observe (or the private Dictionary-typed method) and keep the existing
behavior of comparing path.ToString() and throwing on collisions between
different RelativePath instances.
In `@src/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cs`:
- Around line 132-177: The private method EnumerateFilesDepthFirst is
dead/unreferenced and should be removed; delete the entire
EnumerateFilesDepthFirst method from LocalFileEnumerator.cs, ensure there are no
remaining callers or private references to EnumerateFilesDepthFirst (search for
its identifier), rebuild/run tests to confirm nothing breaks, and commit the
removal to reduce maintenance overhead.
In `@src/Arius.Core/Shared/RepositoryPaths.cs`:
- Around line 33-43: The four methods GetChunkIndexCacheRoot,
GetFileTreeCacheRoot, GetSnapshotCacheRoot, and GetLogsRoot duplicate the same
relativePath.ToString().Replace('/', Path.DirectorySeparatorChar) logic; extract
that conversion into a single helper (either a new
LocalDirectory.FromRelativePath/NormalizeRelativePath on LocalDirectory or a
private static helper in RepositoryPaths) and update each method to call the
helper and then
LocalDirectory.Parse(Path.Combine(GetRepositoryRoot(...).ToString(),
helperResult)); reference ChunkIndexCacheRelativePath,
FileTreeCacheRelativePath, SnapshotCacheRelativePath, LogsRelativePath and
LocalDirectory.Parse/GetRepositoryRoot when making the change.
🪄 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: 278f5951-eb6d-4ccc-a6b0-20a81f21d0b9
📒 Files selected for processing (75)
.agents/skills/csharp-coding-standards/SKILL.md.agents/skills/csharp-coding-standards/performance-and-api-design.md.agents/skills/grill-me/SKILL.md.agents/skills/simplify/SKILL.md.github/prompts/opsx-apply.prompt.md.github/prompts/opsx-explore.prompt.md.github/skills/openspec-apply-change/SKILL.md.github/skills/openspec-archive-change/SKILL.md.github/skills/openspec-explore/SKILL.md.github/skills/openspec-propose/SKILL.md.opencode/command/opsx-apply.md.opencode/command/opsx-archive.md.opencode/command/opsx-explore.md.opencode/commands/opsx-apply.md.opencode/commands/opsx-propose.md.opencode/skills/openspec-apply-change/SKILL.md.opencode/skills/openspec-archive-change/SKILL.md.opencode/skills/openspec-explore/SKILL.md.opencode/skills/openspec-propose/SKILL.md.superset/config.jsondocs/decisions/adr-0008-introduce-internal-filesystem-domain-types.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/.openspec.yamlopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/design.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/proposal.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/specs/archive-pipeline/spec.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/specs/file-tree-service/spec.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/specs/filesystem-domain-types/spec.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/specs/list-query/spec.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/specs/restore-pipeline/spec.mdopenspec/changes/archive/2026-05-06-introduce-core-filesystem-domain-types/tasks.mdopenspec/specs/archive-pipeline/spec.mdopenspec/specs/file-tree-service/spec.mdopenspec/specs/filesystem-domain-types/spec.mdopenspec/specs/list-query/spec.mdopenspec/specs/restore-pipeline/spec.mdsrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/FileSystem/LocalDirectoryTests.cssrc/Arius.Core.Tests/Shared/FileSystem/PathSegmentTests.cssrc/Arius.Core.Tests/Shared/FileSystem/RelativeFileSystemTests.cssrc/Arius.Core.Tests/Shared/FileSystem/RelativePathTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeStagingWriterTests.cssrc/Arius.Core.Tests/Shared/LocalFile/ArchivePathCollisionValidatorTests.cssrc/Arius.Core.Tests/Shared/LocalFile/LocalFileEnumeratorTests.cssrc/Arius.Core.Tests/Shared/RepositoryPathsTests.cssrc/Arius.Core.Tests/Shared/Storage/BlobPathsTests.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ArchiveCommand/Models.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cssrc/Arius.Core/Features/RestoreCommand/Models.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cssrc/Arius.Core/Shared/FileSystem/LocalDirectory.cssrc/Arius.Core/Shared/FileSystem/LocalDirectoryEntry.cssrc/Arius.Core/Shared/FileSystem/LocalFileEntry.cssrc/Arius.Core/Shared/FileSystem/PathSegment.cssrc/Arius.Core/Shared/FileSystem/RelativeFileSystem.cssrc/Arius.Core/Shared/FileSystem/RelativePath.cssrc/Arius.Core/Shared/FileSystem/RelativePathPointerExtensions.cssrc/Arius.Core/Shared/FileTree/FileTreePaths.cssrc/Arius.Core/Shared/FileTree/FileTreeService.cssrc/Arius.Core/Shared/FileTree/FileTreeStagingWriter.cssrc/Arius.Core/Shared/LocalFile/ArchivePathCollisionValidator.cssrc/Arius.Core/Shared/LocalFile/BinaryFile.cssrc/Arius.Core/Shared/LocalFile/FilePair.cssrc/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cssrc/Arius.Core/Shared/LocalFile/PointerFile.cssrc/Arius.Core/Shared/RepositoryPaths.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.Core/Shared/Storage/BlobConstants.cs
💤 Files with no reviewable changes (4)
- .opencode/command/opsx-archive.md
- .superset/config.json
- .opencode/command/opsx-explore.md
- .opencode/command/opsx-apply.md
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.Integration.Tests/Pipeline/PipelineFixture.cs (1)
167-174:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrop the pre-disposal cache delete.
_repository.DisposeAsync()already disposes the shared repository services and clears the repo cache. DeletingcacheDirfirst is redundant and can turn teardown into a handle-in-use failure on platforms where the index still has files open.♻️ Suggested change
public async ValueTask DisposeAsync() { - // Clean up any cache dirs created by this test's container (unique name) - var cacheDir = RepositoryCachePaths.GetRepositoryDirectory(Account, Container.Name); - if (Directory.Exists(cacheDir)) - Directory.Delete(cacheDir, recursive: true); - await _repository.DisposeAsync(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Integration.Tests/Pipeline/PipelineFixture.cs` around lines 167 - 174, In DisposeAsync the code preemptively deletes the repo cache directory (cacheDir from RepositoryCachePaths.GetRepositoryDirectory(Account, Container.Name)), which is redundant and can cause "handle-in-use" errors; remove the Directory.Exists/Directory.Delete block so that _repository.DisposeAsync() runs first and performs repository shutdown and cache cleanup; simply call await _repository.DisposeAsync() (and any remaining cleanup after dispose if needed) and delete the manual cache deletion lines referencing cacheDir.src/Arius.E2E.Tests/Fixtures/E2EFixture.cs (1)
91-115:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe lease-aware cache policy is bypassed during disposal.
DisposeAsync()calls_repository.DisposeAsync()first, andRepositoryTestFixture.DisposeAsync()unconditionally resets the local repo cache. That makesPreserveLocalCacheAsync()ineffective and also sidesteps the active-lease guard inResetLocalCacheAsync(), so warm-cache E2E steps can lose state or have their shared cache deleted out from under another live fixture.A clean fix is to split repository-fixture teardown into “dispose services/temp roots” and “reset local cache”, then let
E2EFixtureown the lease-aware cache deletion decision. As per coding guidelines, "Treat dataset versions (V1 vs V2) and cache transitions (Warm vs Cold) as explicit workflow steps in one evolving repository history."Also applies to: 176-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs` around lines 91 - 115, The teardown currently disposes services and then unconditionally calls cache reset (RepositoryTestFixture.DisposeAsync() calls _repository.DisposeAsync() then resets cache), which bypasses PreserveLocalCacheAsync() and the active-lease guard in ResetLocalCacheAsync(); refactor by splitting repository fixture teardown into two steps: (1) a method that only disposes services/temp roots (e.g., keep or add DisposeServicesAsync() or StopServices() inside RepositoryTestFixture and ensure _repository.DisposeAsync() is invoked there) and (2) remove the unconditional cache deletion from RepositoryTestFixture.DisposeAsync(); then have E2EFixture own the cache-deletion decision—call ResetLocalCacheAsync(accountName, containerName) from E2EFixture.DisposeAsync() only when PreserveLocalCacheAsync() has not been requested and when HasActiveLease(...) is false so the existing lease-aware logic in ResetLocalCacheAsync() is respected.
♻️ Duplicate comments (3)
src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs (1)
324-325:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNull-check
ErrorMessagebeforeShouldContain.These three failure-path assertions can still throw on a null
ErrorMessagebefore they verify the expected message.Proposed fix
restoreResult.Success.ShouldBeFalse(); + restoreResult.ErrorMessage.ShouldNotBeNull(); restoreResult.ErrorMessage.ShouldContain("Invalid relative path");Also applies to: 358-359, 392-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs` around lines 324 - 325, The assertions in RestoreCommandHandlerTests reference restoreResult.ErrorMessage directly with ShouldContain, which can throw if ErrorMessage is null; update each failure-path assertion (instances around the checks for "Invalid relative path" and the other two occurrences at lines noted) to first assert restoreResult.ErrorMessage is not null (e.g., ShouldNotBeNull) before calling ShouldContain, ensuring you check restoreResult.Success is false, then validate ErrorMessage is not null, and only then assert ErrorMessage contains the expected substring.src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs (1)
13-27:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
BuildFilesstill collapses distinct local files that differ only by case.
filesis keyed by basename withOrdinalIgnoreCase, so on a case-sensitive filesystemdocs/shared.txtanddocs/Shared.txtoverwrite each other. The same comparer also lets a pointer likeshared.txt.POINTER.ARIUSattach toShared.txt, which reports the wrong local state instead of surfacing a collision.As per coding guidelines,
src/Arius.Core/**: "Core library. Pay close attention to deduplication logic, encryption correctness, and thread safety. Flag any changes that could cause data loss or corruption."Also applies to: 37-42, 51-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs` around lines 13 - 27, The BuildFiles logic uses the variable files (a Dictionary keyed by basename with StringComparer.OrdinalIgnoreCase) which collapses distinct local files that differ only by case and misattaches pointer entries; change the keying to preserve case-sensitivity (use an Ordinal comparer or include the full relative path as the key) so that entries like docs/shared.txt and docs/Shared.txt are stored separately and pointer files resolved by TryGetPointerBinaryPath map to the exact LocalFileState instance; update all places that construct and access files (including where files.TryGetValue(binaryName, out var existingBinary) and the code paths at the referenced blocks) to use the new keying strategy and ensure PointerExists/PointerHash assignments update the correct per-file LocalFileState rather than collapsing different-case files.src/Arius.Core/Features/ListQuery/ListQueryHandler.cs (1)
104-105:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe list merge still hides case-only directory and file conflicts.
yieldedDirectoryNames,yieldedFileNames, and the local directory map all useOrdinalIgnoreCase. On Linux, clouddocs/plus localDocs/, or cloudReadme.txtplus localreadme.txt, gets reported as a single entry because the later local pass treats the exact-case variant as already yielded.As per coding guidelines,
src/Arius.Core/**: "Core library. Pay close attention to deduplication logic, encryption correctness, and thread safety. Flag any changes that could cause data loss or corruption."Also applies to: 126-128, 189-191, 268-275
🧹 Nitpick comments (2)
src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs (1)
79-87: ⚡ Quick winMove
LocalFileStateintoLocalFileState.cs.
LocalFileSnapshotBuilder.csnow contains two top-level types. Splitting the record out keeps lookup/discovery simpler and matches the repository’s file-per-type convention.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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs` around lines 79 - 87, Extract the LocalFileState record into its own file named LocalFileState.cs: create a new internal sealed record LocalFileState(...) with the exact same signature and namespace as LocalFileSnapshotBuilder, add any needed using directives (e.g., for RelativePath and ContentHash), and remove the LocalFileState declaration from LocalFileSnapshotBuilder.cs so each top-level type lives in its own file and the repository convention (filename matches type name) is preserved.src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs (1)
309-388: ⚡ Quick winAdd a case-only sibling regression for
BuildFiles.The new tests cover pointer-suffix normalization, but they will not catch the Linux case where
shared.txtandShared.txtcollapse into one entry, or where a differently-cased pointer attaches to the wrong binary. A focused fixture with both variants would lock down the intended behavior.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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs` around lines 309 - 388, Add a focused regression test for case-only sibling handling in LocalFileSnapshotBuilder.BuildFiles: create a new test (similar to BuildFiles_UppercasePointerSuffix_UpdatesExistingBinaryEntry) that supplies entries with the same path differing only by case (e.g., RelativePath.Parse("docs/Shared.txt") and RelativePath.Parse("docs/shared.txt.POINTER.ARIUS")), call LocalFileSnapshotBuilder.BuildFiles with the same existence/content probes, and assert that the resulting map collapses to a single key ("shared.txt" as other tests expect) and that the pointer attaches to the intended binary (check BinaryExists/PointerExists/PointerHash on the merged entry); reference the existing test names and LocalFileEntry/RelativePath.Parse to locate where to add this fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`:
- Around line 315-327: The test shows the handler accepts an absolute/invalid
TargetPath and performs writes before returning an error; fix this by validating
the restore target path at the start of the restore flow and returning a failed
RestoreResult without touching disk. Specifically, add an early check in the
RestoreCommand handler (the Handle method on
Core.Features.RestoreCommand.RestoreCommandHandler or the method created by
CreateRestoreHandler) that validates RestoreOptions.TargetPath is a safe
relative path (reject leading slashes, .. segments, or absolute URIs) and
immediately returns Success=false with "Invalid relative path" if invalid,
before any filesystem operations or calls that write to fixture.RestoreRoot.
Ensure the error text matches existing expectations and no restore-side effects
occur when the path is invalid.
In `@src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs`:
- Around line 308-316: In the dedup-hit branch (the if handling isKnown ||
inFlightHashes.ContainsKey(hashed.ContentHash)) also honor the --remove-local
option by enqueueing the local file for deletion when opts.RemoveLocal is true:
after WriteFileTreeEntry(...) and Interlocked.Increment(ref filesDeduped), add
logic to record hashed.FilePair.Path (or hashed.FilePair) into the same deletion
queue your command uses for removals (e.g., pendingDeletes/pendingRemovals or
the existing deletion mechanism) so deduped files are removed locally; keep the
existing pendingPointers behavior when !opts.NoPointers. Ensure you reference
the same deletion collection/process used elsewhere in ArchiveCommandHandler so
removal happens later in the run.
In `@src/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cs`:
- Around line 55-65: The current branch creates a FilePair with PointerFile
containing only Path, BinaryPath and Hash, dropping available file metadata
(file.Size, file.Created, file.Modified) which causes snapshot/timestamp drift;
update the creation of the FilePair/PointerFile in LocalFileEnumerator (the code
that calls ReadPointerHash and yields new FilePair) to copy the original file's
metadata into the PointerFile (e.g., set PointerFile.Size, PointerFile.Created,
PointerFile.Modified from the local 'file' variable) so pointer-only files
preserve size and timestamps downstream (ensure those property names match the
PointerFile/FilePair model and keep Binary = null).
---
Outside diff comments:
In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs`:
- Around line 91-115: The teardown currently disposes services and then
unconditionally calls cache reset (RepositoryTestFixture.DisposeAsync() calls
_repository.DisposeAsync() then resets cache), which bypasses
PreserveLocalCacheAsync() and the active-lease guard in ResetLocalCacheAsync();
refactor by splitting repository fixture teardown into two steps: (1) a method
that only disposes services/temp roots (e.g., keep or add DisposeServicesAsync()
or StopServices() inside RepositoryTestFixture and ensure
_repository.DisposeAsync() is invoked there) and (2) remove the unconditional
cache deletion from RepositoryTestFixture.DisposeAsync(); then have E2EFixture
own the cache-deletion decision—call ResetLocalCacheAsync(accountName,
containerName) from E2EFixture.DisposeAsync() only when
PreserveLocalCacheAsync() has not been requested and when HasActiveLease(...) is
false so the existing lease-aware logic in ResetLocalCacheAsync() is respected.
In `@src/Arius.Integration.Tests/Pipeline/PipelineFixture.cs`:
- Around line 167-174: In DisposeAsync the code preemptively deletes the repo
cache directory (cacheDir from
RepositoryCachePaths.GetRepositoryDirectory(Account, Container.Name)), which is
redundant and can cause "handle-in-use" errors; remove the
Directory.Exists/Directory.Delete block so that _repository.DisposeAsync() runs
first and performs repository shutdown and cache cleanup; simply call await
_repository.DisposeAsync() (and any remaining cleanup after dispose if needed)
and delete the manual cache deletion lines referencing cacheDir.
---
Duplicate comments:
In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`:
- Around line 324-325: The assertions in RestoreCommandHandlerTests reference
restoreResult.ErrorMessage directly with ShouldContain, which can throw if
ErrorMessage is null; update each failure-path assertion (instances around the
checks for "Invalid relative path" and the other two occurrences at lines noted)
to first assert restoreResult.ErrorMessage is not null (e.g., ShouldNotBeNull)
before calling ShouldContain, ensuring you check restoreResult.Success is false,
then validate ErrorMessage is not null, and only then assert ErrorMessage
contains the expected substring.
In `@src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs`:
- Around line 13-27: The BuildFiles logic uses the variable files (a Dictionary
keyed by basename with StringComparer.OrdinalIgnoreCase) which collapses
distinct local files that differ only by case and misattaches pointer entries;
change the keying to preserve case-sensitivity (use an Ordinal comparer or
include the full relative path as the key) so that entries like docs/shared.txt
and docs/Shared.txt are stored separately and pointer files resolved by
TryGetPointerBinaryPath map to the exact LocalFileState instance; update all
places that construct and access files (including where
files.TryGetValue(binaryName, out var existingBinary) and the code paths at the
referenced blocks) to use the new keying strategy and ensure
PointerExists/PointerHash assignments update the correct per-file LocalFileState
rather than collapsing different-case files.
---
Nitpick comments:
In `@src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs`:
- Around line 309-388: Add a focused regression test for case-only sibling
handling in LocalFileSnapshotBuilder.BuildFiles: create a new test (similar to
BuildFiles_UppercasePointerSuffix_UpdatesExistingBinaryEntry) that supplies
entries with the same path differing only by case (e.g.,
RelativePath.Parse("docs/Shared.txt") and
RelativePath.Parse("docs/shared.txt.POINTER.ARIUS")), call
LocalFileSnapshotBuilder.BuildFiles with the same existence/content probes, and
assert that the resulting map collapses to a single key ("shared.txt" as other
tests expect) and that the pointer attaches to the intended binary (check
BinaryExists/PointerExists/PointerHash on the merged entry); reference the
existing test names and LocalFileEntry/RelativePath.Parse to locate where to add
this fixture.
In `@src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs`:
- Around line 79-87: Extract the LocalFileState record into its own file named
LocalFileState.cs: create a new internal sealed record LocalFileState(...) with
the exact same signature and namespace as LocalFileSnapshotBuilder, add any
needed using directives (e.g., for RelativePath and ContentHash), and remove the
LocalFileState declaration from LocalFileSnapshotBuilder.cs so each top-level
type lives in its own file and the repository convention (filename matches type
name) is preserved.
🪄 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: 3d1d2097-b021-4584-bb76-2f1a65603d0d
📒 Files selected for processing (22)
src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cssrc/Arius.Core.Tests/Shared/RepositoryPathsTests.cssrc/Arius.Core/AssemblyMarker.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/Shared/FileSystem/RelativePath.cssrc/Arius.Core/Shared/FileSystem/RelativePathPointerExtensions.cssrc/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cssrc/Arius.Core/Shared/RepositoryPaths.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Integration.Tests/Shared/FileTree/FileTreeBuilderIntegrationTests.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cssrc/Arius.Tests.Shared/RepositoryCachePaths.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arius.Core/Shared/FileSystem/RelativePathPointerExtensions.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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`:
- Around line 392-393: The test currently asserts restoreResult.Success and then
calls restoreResult.ErrorMessage.ShouldContain(...); add a null-guard by
asserting restoreResult.ErrorMessage.ShouldNotBeNull() before calling
ShouldContain to avoid a potential null reference; update the
RestoreCommandHandlerTests assertion sequence around the
restoreResult.ErrorMessage check (use ShouldNotBeNull() then
ShouldContain("Invalid relative path")).
🪄 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: 2eb1ae7c-a5dd-4863-9f52-6570f5e87839
📒 Files selected for processing (4)
README.mdsrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Arius.Core/Shared/Extensions.cs (1)
17-40: ⚡ Quick winResolve inconsistency in buffer length handling.
The two methods use different sources for the segment length when
TryGetBuffersucceeds:
- Line 20 (
ToImmutableArray) usessegment.Count- Line 37 (
ToArraySegment) useschecked((int)stream.Length)While both should yield the same value, the inconsistency is confusing and suggests uncertainty about which is correct. For consistency and clarity, both methods should use the same approach — preferably
segment.Count, since it's the data already returned byTryGetBuffer.♻️ Proposed fix for consistency
public static ArraySegment<byte> ToArraySegment(this MemoryStream stream) { if (stream.TryGetBuffer(out var sealedBuffer)) - return new ArraySegment<byte>(sealedBuffer.Array!, sealedBuffer.Offset, checked((int)stream.Length)); + return sealedBuffer; return new ArraySegment<byte>(stream.ToArray()); }Alternatively, if you prefer to use
stream.Lengthconsistently, updateToImmutableArrayto match:public static ImmutableArray<byte> ToImmutableArray(this MemoryStream stream) { if (stream.TryGetBuffer(out ArraySegment<byte> segment)) - return ImmutableArray.Create(segment.Array!, segment.Offset, segment.Count); + return ImmutableArray.Create(segment.Array!, segment.Offset, checked((int)stream.Length));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/Extensions.cs` around lines 17 - 40, The two extension methods ToImmutableArray and ToArraySegment are inconsistent when TryGetBuffer succeeds; change ToArraySegment to use the buffer-provided length (sealedBuffer.Count from TryGetBuffer) instead of checked((int)stream.Length) so both methods rely on the ArraySegment length returned by TryGetBuffer; update the return construction in ToArraySegment to use sealedBuffer.Offset and sealedBuffer.Count and ensure ToImmutableArray still uses segment.Count for the same semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Core/Shared/Extensions.cs`:
- Around line 5-8: The class IEnumerableExtensions is public and the extension
method Empty<T>(this IEnumerable<T> str) uses a misleading parameter name;
change the class visibility to internal and rename the extension parameter from
str to a clearer name (e.g., source or enumerable) in the Empty<T> method
signature and its body so callers and tests use InternalsVisibleTo for test
access and the parameter name reflects an IEnumerable<T> input.
- Line 10: Change the StreamExtensions class from public to internal to follow
the coding guideline: locate the public class declaration "StreamExtensions" and
replace its accessibility modifier with internal; if tests need access, add an
InternalsVisibleTo entry in the assembly info (or equivalent) rather than making
the class public.
In `@src/Arius.Core/Shared/FileSystem/LocalDirectory.cs`:
- Around line 56-69: TryGetRelativePath fails when hostPath equals the directory
root because Path.GetRelativePath returns "." which RelativePath.TryParse
rejects; modify TryGetRelativePath so after computing var relative =
Path.GetRelativePath(Value, fullPath) you detect the special-case value "." (and
possibly empty) and convert it to the canonical empty/root representation before
normalizing separators and calling RelativePath.TryParse; keep the IsContained
check and then pass the normalized (with "." mapped to "") string into
RelativePath.TryParse so a root-contained path returns true and an appropriate
RelativePath result.
---
Nitpick comments:
In `@src/Arius.Core/Shared/Extensions.cs`:
- Around line 17-40: The two extension methods ToImmutableArray and
ToArraySegment are inconsistent when TryGetBuffer succeeds; change
ToArraySegment to use the buffer-provided length (sealedBuffer.Count from
TryGetBuffer) instead of checked((int)stream.Length) so both methods rely on the
ArraySegment length returned by TryGetBuffer; update the return construction in
ToArraySegment to use sealedBuffer.Offset and sealedBuffer.Count and ensure
ToImmutableArray still uses segment.Count for the same semantics.
🪄 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: 58bbd693-fbef-4e8b-a0f8-3d3126dcbed8
📒 Files selected for processing (9)
src/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceUploadTests.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ArchiveCommand/Models.cssrc/Arius.Core/Shared/ChunkStorage/ChunkStorageService.cssrc/Arius.Core/Shared/Extensions.cssrc/Arius.Core/Shared/FileSystem/LocalDirectory.cssrc/Arius.Core/Shared/FileSystem/PathSegment.cssrc/Arius.Core/Shared/FileSystem/RelativePath.cssrc/Arius.Core/Shared/IEnumerableExtensions.cs
💤 Files with no reviewable changes (1)
- src/Arius.Core/Shared/IEnumerableExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Arius.Core/Features/ArchiveCommand/Models.cs
- src/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceUploadTests.cs
- src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Arius.Core/Shared/Extensions.cs (1)
5-41: ⚡ Quick winSplit extension classes into separate files to match project layout rules.
This file currently holds two top-level classes (
IEnumerableExtensionsandStreamExtensions). Please split them into separate files named after each class for consistency with repo conventions.As per coding guidelines: "Prefer one top-level class per file, with the filename matching the class name."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/Extensions.cs` around lines 5 - 41, This file contains two top-level extension classes (IEnumerableExtensions and StreamExtensions); split them into two separate files named IEnumerableExtensions.cs and StreamExtensions.cs, each containing the corresponding internal static class (preserve the existing namespace, usings, XML docs and method implementations such as Empty<T>(), ToImmutableArray(MemoryStream) and ToArraySegment(MemoryStream)); remove the original combined file and ensure both new files compile and keep the same accessibility and method signatures (including TryGetBuffer usage and fallbacks).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Arius.Core/Shared/Extensions.cs`:
- Around line 5-41: This file contains two top-level extension classes
(IEnumerableExtensions and StreamExtensions); split them into two separate files
named IEnumerableExtensions.cs and StreamExtensions.cs, each containing the
corresponding internal static class (preserve the existing namespace, usings,
XML docs and method implementations such as Empty<T>(),
ToImmutableArray(MemoryStream) and ToArraySegment(MemoryStream)); remove the
original combined file and ensure both new files compile and keep the same
accessibility and method signatures (including TryGetBuffer usage and
fallbacks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44f31da5-0929-468b-afeb-d9c7fef88fa6
📒 Files selected for processing (4)
src/Arius.Core.Tests/Fakes/FakeSeededBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileSystem/LocalDirectoryTests.cssrc/Arius.Core/Shared/Extensions.cssrc/Arius.Core/Shared/FileSystem/LocalDirectory.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Arius.Core.Tests/Shared/FileSystem/LocalDirectoryTests.cs
- src/Arius.Core/Shared/FileSystem/LocalDirectory.cs
- src/Arius.Core.Tests/Fakes/FakeSeededBlobContainerService.cs
This was referenced Jun 12, 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
Arius.Core, including relative path, rooted filesystem, archive-time file models, and typed path handling across archive, list, restore, filetree, cache, and blob-path flowsTest Plan
dotnet test --project src/Arius.Core.Tests/Arius.Core.Tests.csprojdotnet test --project src/Arius.Cli.Tests/Arius.Cli.Tests.csprojdotnet test --project src/Arius.Architecture.Tests/Arius.Architecture.Tests.csprojslopwatch analyzeSummary by CodeRabbit
New Features
Bug Fixes
Documentation