Simplify list query walk and local filesystem pairing#106
Merged
Conversation
chore: add specs of recent fable implementations
…eSystem.EnumerateFiles and EnumerateDirectories now return IEnumerable<RelativePath> directly, and every caller lost its .Path hop (archive's LocalFileEnumerator, ListQuery's LocalDirectoryReader.PairFiles, the three E2E sites, and the filesystem tests). The architecture test's internal-type list dropped the two deleted entries. LocalFile stays untouched in ListQuery as the paired logical-file type.
Contributor
|
Warning Review limit reached
More reviews will be available in 32 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR executes the list-query handler simplification design plan by refactoring the handler from a multi-stage channel-based pipeline into a breadth-first async iterator, introducing symmetric local/remote directory listing models, and simplifying filesystem enumeration APIs to return paths directly instead of wrapper objects. ChangesList-query handler simplification and filesystem refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
==========================================
- Coverage 80.47% 80.39% -0.08%
==========================================
Files 91 93 +2
Lines 6166 6081 -85
Branches 824 815 -9
==========================================
- Hits 4962 4889 -73
+ Misses 976 964 -12
Partials 228 228
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
openspec/changes/2026-06-12-simplify-list/PLAN.MD (1)
22-47: 💤 Low valueAdd language specifiers to code blocks for syntax highlighting.
The code blocks containing C# code should declare the language to enable syntax highlighting and improve readability on documentation platforms.
Proposed fix for code block language specifiers
-```csharp +```csharp /// <summary>What the repository knows about one directory: the persisted filetree node,(Apply the same fix to all code blocks: add
csharpafter the opening```.)Also applies to: 59-96
🤖 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/changes/2026-06-12-simplify-list/PLAN.MD` around lines 22 - 47, Add the C# language specifier to the fenced code blocks in PLAN.MD so syntax highlighting is enabled for the shown types (e.g., the blocks containing RemoteDirectoryListing, LocalDirectoryListing and LocalFile). Edit the opening fence for each affected block from ``` to ```csharp (apply to the block starting around the snippet that defines RemoteDirectoryListing, the block with LocalDirectoryListing.Empty, and the block with LocalFile) and leave the contents unchanged.src/Arius.Core/Features/ListQuery/LocalDirectoryReader.cs (1)
80-86: 💤 Low valueEdge case: pointer enumerated but binary not enumerated (yet exists on disk) results in no entry.
When
fileExists(binaryPath)returns true but the binary wasn't enumerated (a race where binary was created mid-enumeration), line 83 skips the pointer assuming the binary pass will handle it. However, the binary won't be iterated since it's not infilePaths, resulting in noLocalFileentry for this pair.This is a narrow race window and the degradation is acceptable (the file will appear on next list), but consider whether the intent is to create a
BinaryExists: trueentry here.🤖 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/LocalDirectoryReader.cs` around lines 80 - 86, The pointer-only branch currently skips creating an entry when fileExists(binaryPath) is true, which loses the file if the binary wasn't enumerated; in LocalDirectoryReader.cs adjust the logic so the pointer entry is created with BinaryExists set based on fileExists(binaryPath) rather than skipping — e.g., only skip when enumeratedPaths.Contains(binaryPath) but when not enumerated create files[binaryName] = new LocalFile(binaryName, BinaryExists: fileExists(binaryPath), PointerExists: true, ...); ensure you reference variables/methods binaryPath, binaryName, fileExists, enumeratedPaths, files and the LocalFile constructor when making the change.src/Arius.Core/Shared/FileSystem/PathSegmentComparers.cs (1)
3-16: ⚡ Quick winSplit these comparers into separate files.
Lines 3-16 introduce two top-level types in
PathSegmentComparers.cs. This breaks the repo’s C# file-layout rule and makes symbol-to-file lookup less predictable. Please movePathSegmentOrdinalIgnoreCaseComparerandPathSegmentOrdinalComparerinto files that match their type names. 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/FileSystem/PathSegmentComparers.cs` around lines 3 - 16, The file contains two top-level comparer types; split them so each top-level type lives in its own file: create PathSegmentOrdinalIgnoreCaseComparer.cs containing the internal sealed class PathSegmentOrdinalIgnoreCaseComparer with its static Instance and Compare implementation, and create PathSegmentOrdinalComparer.cs containing the internal sealed class PathSegmentOrdinalComparer with its static Instance and Compare implementation; remove the duplicate type declarations from PathSegmentComparers.cs and ensure both new files use the same namespace as the original file and keep the existing signatures (Compare(PathSegment x, PathSegment y) and Instance property) unchanged.Source: Coding guidelines
🤖 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/Features/ListQuery/LocalDirectoryReader.cs`:
- Around line 40-47: The stat lambda passed to PairFiles in
LocalDirectoryReader.cs can throw if files are removed or inaccessible; wrap the
logic that calls fileSystem.GetTimestamps(path) and fileSystem.GetFileSize(path)
in a try/catch that returns null (or a nullable metadata tuple) on failure and
logs a warning (mirroring the graceful degradation used for enumeration), or
alternatively update PairFiles to accept a nullable-returning stat delegate so
stat failures do not abort the entire PairFiles operation.
---
Nitpick comments:
In `@openspec/changes/2026-06-12-simplify-list/PLAN.MD`:
- Around line 22-47: Add the C# language specifier to the fenced code blocks in
PLAN.MD so syntax highlighting is enabled for the shown types (e.g., the blocks
containing RemoteDirectoryListing, LocalDirectoryListing and LocalFile). Edit
the opening fence for each affected block from ``` to ```csharp (apply to the
block starting around the snippet that defines RemoteDirectoryListing, the block
with LocalDirectoryListing.Empty, and the block with LocalFile) and leave the
contents unchanged.
In `@src/Arius.Core/Features/ListQuery/LocalDirectoryReader.cs`:
- Around line 80-86: The pointer-only branch currently skips creating an entry
when fileExists(binaryPath) is true, which loses the file if the binary wasn't
enumerated; in LocalDirectoryReader.cs adjust the logic so the pointer entry is
created with BinaryExists set based on fileExists(binaryPath) rather than
skipping — e.g., only skip when enumeratedPaths.Contains(binaryPath) but when
not enumerated create files[binaryName] = new LocalFile(binaryName,
BinaryExists: fileExists(binaryPath), PointerExists: true, ...); ensure you
reference variables/methods binaryPath, binaryName, fileExists, enumeratedPaths,
files and the LocalFile constructor when making the change.
In `@src/Arius.Core/Shared/FileSystem/PathSegmentComparers.cs`:
- Around line 3-16: The file contains two top-level comparer types; split them
so each top-level type lives in its own file: create
PathSegmentOrdinalIgnoreCaseComparer.cs containing the internal sealed class
PathSegmentOrdinalIgnoreCaseComparer with its static Instance and Compare
implementation, and create PathSegmentOrdinalComparer.cs containing the internal
sealed class PathSegmentOrdinalComparer with its static Instance and Compare
implementation; remove the duplicate type declarations from
PathSegmentComparers.cs and ensure both new files use the same namespace as the
original file and keep the existing signatures (Compare(PathSegment x,
PathSegment y) and Instance property) unchanged.
🪄 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: 50882d21-ca39-4657-81e0-089ea4875f28
📒 Files selected for processing (26)
openspec/changes/2026-06-10-1-rewrite-list/2026-06-10-234022-local-command-caveatcaveat-the-messages-below.txtopenspec/changes/2026-06-10-1-rewrite-list/2026-06-11-010913-this-session-is-being-continued-from-a-previous-c.txtopenspec/changes/2026-06-10-1-rewrite-list/PLAN.MDopenspec/changes/2026-06-10-1-rewrite-list/PLAN2-Coderabbit fixes.MDopenspec/changes/2026-06-10-2-rewrite-restore/PLAN.MDopenspec/changes/2026-06-10-3-dynamic-shard-length/PLAN.MDopenspec/changes/2026-06-12-simplify-list/PLAN.MDopenspec/specs/list-query/spec.mdsrc/Arius.Architecture.Tests/DependencyTests.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.Core.Tests/Shared/FileSystem/RelativeFileSystemTests.cssrc/Arius.Core/Features/ArchiveCommand/LocalFileEnumerator.cssrc/Arius.Core/Features/ListQuery/ListQuery.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Features/ListQuery/LocalDirectoryReader.cssrc/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cssrc/Arius.Core/Features/ListQuery/Models.cssrc/Arius.Core/Shared/FileSystem/LocalDirectoryEntry.cssrc/Arius.Core/Shared/FileSystem/LocalFileEntry.cssrc/Arius.Core/Shared/FileSystem/PathSegmentComparers.cssrc/Arius.Core/Shared/FileSystem/RelativeFileSystem.cssrc/Arius.Core/Shared/FileTree/FileTreeSerializer.cssrc/Arius.Core/Shared/FileTree/FileTreeService.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryMaterializer.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryStateAssertions.cssrc/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cs
💤 Files with no reviewable changes (6)
- src/Arius.Core/Shared/FileSystem/LocalFileEntry.cs
- src/Arius.Core/Features/ListQuery/LocalFileSnapshotBuilder.cs
- src/Arius.Core/Shared/FileSystem/LocalDirectoryEntry.cs
- src/Arius.Architecture.Tests/DependencyTests.cs
- src/Arius.Core/Shared/FileTree/FileTreeSerializer.cs
- src/Arius.Core/Shared/FileTree/FileTreeService.cs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Jun 15, 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
ListQueryHandleras a single-stage breadth-first remote/local directory walkLocalDirectoryReaderand symmetric list-query modelsRelativePathdirectly and update affected callers/testsTesting
Summary by CodeRabbit
Release Notes
Documentation
Specification Updates
Tests