feat: streaming list rewrite#103
Merged
Merged
Conversation
…ng-list-and-restore
…ng-list-and-restore
…ng-list-and-restore
Contributor
📝 WalkthroughWalkthroughThe PR refactors the list-query and chunk-index subsystems to support storage-tier awareness and state-based entry modeling. It replaces boolean existence flags with a composable ChangesRepository state flags and tier-aware listing rewrite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #103 +/- ##
==========================================
+ Coverage 80.11% 80.50% +0.39%
==========================================
Files 90 91 +1
Lines 6045 6166 +121
Branches 799 824 +25
==========================================
+ Hits 4843 4964 +121
+ Misses 977 974 -3
- Partials 225 228 +3
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: 6
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.Tests/Shared/ChunkIndex/ShardSerializerTests.cs (1)
46-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd tier assertions for stream-based deserialization paths.
Line 46 and Line 88 only assert size fields; they should also assert
StorageTierHintto fully cover the new shard tier contract in bothDeserialize(Stream, ...)test paths.Suggested test patch
@@ loaded.TryLookup(FakeContentHash('e'), out var e).ShouldBeTrue(); e!.OriginalSize.ShouldBe(512); + e.StorageTierHint.ShouldBe(BlobTier.Archive); @@ loaded.TryLookup(FakeContentHash('8'), out var e).ShouldBeTrue(); e!.CompressedSize.ShouldBe(40); + e.StorageTierHint.ShouldBe(BlobTier.Cool);As per coding guidelines, test-project reviews should prioritize coverage gaps and assertion quality.
Also applies to: 88-90
Source: Coding guidelines
🧹 Nitpick comments (4)
README.md (1)
310-312: 💤 Low valueOptional: specify language for fenced code block.
The shard line format example is in an unlabeled fenced code block. Consider adding
```textto improve markdown hygiene.🤖 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 `@README.md` around lines 310 - 312, Update the unlabeled fenced code block that shows the shard line format example (the block containing "`<content-hash> <chunk-hash> <original-size> <compressed-size> <tier>`") to include an explicit language label by changing the opening fence to "```text" so the example is rendered as plain text and improves markdown hygiene.Source: Linters/SAST tools
openspec/changes/2026-06-10-rewrite/PLAN.MD (1)
64-66: 💤 Low valueOptional: specify language for fenced code block.
The ASCII pipeline diagram is in an unlabeled fenced code block. Consider adding
```textto improve markdown hygiene, though this is purely cosmetic.🤖 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-10-rewrite/PLAN.MD` around lines 64 - 66, Add a language label to the fenced code block that contains the ASCII pipeline diagram (the block starting with ``` and the content `Walk (×1) ─► walkItemChannel (bounded) ─► Resolve (×1) ─► entryChannel (bounded) ─► Handle (yield)`) by changing the opening fence to ```text so the block is explicitly marked as plain text; this is a purely cosmetic markdown hygiene change and requires updating only the fenced code block around that pipeline string.Source: Linters/SAST tools
src/Arius.Cli.Tests/Commands/Ls/LsStateFormatterTests.cs (1)
38-58: ⚡ Quick winAdd explicit
ToMarkupassertions for~and?tier outputs.Current markup tests cover hydrated/archived/absent dots, but not rehydrating and unknown-tier paths, which are emitted by the formatter and surfaced in CLI output.
As per coding guidelines, “Test projects. Focus on test coverage gaps and assertion quality rather than style.”Suggested additions
+ [Test] + public void ToMarkup_Rehydrating_UsesExpectedColor() + { + var state = RepositoryEntryState.Repository | RepositoryEntryState.RepositoryArchived | RepositoryEntryState.RepositoryRehydrating; + LsStateFormatter.ToMarkup(state).ShouldBe("[dim].[/][dim].[/]R[purple]~[/]"); + } + + [Test] + public void ToMarkup_UnknownTier_RendersQuestionMark() + { + var state = RepositoryEntryState.Repository; + LsStateFormatter.ToMarkup(state).ShouldBe("[dim].[/][dim].[/]R?"); + }🤖 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.Cli.Tests/Commands/Ls/LsStateFormatterTests.cs` around lines 38 - 58, Add explicit assertions to LsStateFormatterTests to cover the rehydrating and unknown-tier outputs emitted by LsStateFormatter.ToMarkup: update or add tests (e.g., in ToMarkup_PresentFlags_AreColored or new test methods) that call LsStateFormatter.ToMarkup with state combinations that produce the rehydrating marker (~) and the unknown-tier marker (?) and assert the exact markup strings include "~" and "?" with the expected coloring/formatting; reference LsStateFormatter.ToMarkup and RepositoryEntryState flags to construct the input states that produce those outputs so the tests fail if those markers are omitted or misformatted.Source: Coding guidelines
src/Arius.AzureBlob/AzureBlobContainerService.cs (1)
184-198: Clarify tier behavior forGetBlobsAsyncand guard against null tiers
BlobTraits.Noneonly requests “default information”;AccessTier/AccessTierInferredcome from the List Blobs response contract, not from theBlobTraitsflags—so this mapping (Tier = FromAzureTier(item.Properties.AccessTier)) shouldn’t be prevented just becausetraitsisBlobTraits.None.- Still,
item.Properties.AccessTiercan be absent/null depending on the service/account/blob tiering support; ensureFromAzureTier(and the in-memory fake) handles null consistently and add coverage for that scenario in listing tests.🤖 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.AzureBlob/AzureBlobContainerService.cs` around lines 184 - 198, Listing code currently skips or assumes Tier behavior based on BlobTraits but AccessTier can be present even when traits == BlobTraits.None and can also be null; update the blob listing in GetBlobsAsync so it always calls FromAzureTier(item.Properties.AccessTier) for Tier (do not gate it behind includeMetadata/traits), modify FromAzureTier to accept a nullable AccessTier and return a consistent value for null inputs, and update the in-memory/fake implementation and listing tests to cover the null-AccessTier case and the presence-of-AccessTier-when-traits-none scenario (refer to GetBlobsAsync, BlobListItem, FromAzureTier, and the fake container/listing test helpers).
🤖 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.Cli/Commands/Ls/LsVerb.cs`:
- Around line 145-147: The legend printed by AnsiConsole.MarkupLine in LsVerb.cs
is missing symbols emitted by LsStateFormatter (specifically "~" and "?");
update the legend string (the two AnsiConsole.MarkupLine calls) to include short
human-readable descriptions for "~" and "?" alongside the existing P/B/R/H/A
entries so streamed output users see all possible state symbols; ensure the text
matches the semantics defined in LsStateFormatter so the legend accurately
describes each symbol.
In `@src/Arius.Core/Features/ListQuery/ListQuery.cs`:
- Around line 69-79: The hydration flags in the RepositoryEntryState enum
(RepositoryHydrated, RepositoryArchived, RepositoryRehydrating) must encode
their documented implication of Repository by including the Repository bit in
their values; update those enum member definitions so each is defined as
Repository ORed with its current unique bit (e.g., RepositoryHydrated =
Repository | (1 << 4), etc.) so HasFlag(RepositoryEntryState.Repository) returns
true for those states and downstream checks in RepositoryExplorerViewModel work
correctly.
In `@src/Arius.Core/Features/ListQuery/ListQueryHandler.cs`:
- Around line 216-258: The overlay matching is currently case-sensitive causing
duplicate emits; make local-overlay comparisons case-insensitive by normalizing
or using ordinal-ignore-case collections: build emittedDirectoryNames as a
case-insensitive set, access localSnapshot.Directories with case-insensitive
keys (or normalize DirectoryEntry.Name/PathSegment when populating), and perform
localFiles.Remove using a case-insensitive lookup; update all uses referenced
here (emittedDirectoryNames, localSnapshot.Directories, localSnapshot.Files,
localDirectory.Name, repositoryDirectory.Name, localFiles.Remove) and the same
logic in the other block (lines ~423-459) so repository vs local matching uses
consistent OrdinalIgnoreCase semantics.
In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexLocalStore.cs`:
- Line 560: Create an explicit schema-break handling path in
ChunkIndexLocalStore initialization/CreateOrUpgradeSchema(): detect older
cache.sqlite files missing the new required column (storage_tier_hint) by
checking PRAGMA table_info('chunk_index') or attempting a safe test query and
catching a SQLite missing-column error, and if the column is absent/exception
occurs, delete or recreate the local cache DB and re-run schema creation so the
cache is rebuilt rather than letting a later SELECT/UPSERT fail; update
CreateOrUpgradeSchema() (and any initialization logic in ChunkIndexLocalStore
that opens cache.sqlite) to perform this check and recreate the DB when the
schema mismatch is detected.
In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs`:
- Around line 362-377: The code currently defaults missing parent TAR tiers to
BlobTier.Hot when building tarTiers in the pass over _blobs.ListAsync (using
BlobPaths.ChunksPrefix, BlobMetadataKeys.TypeTar and ChunkHash.TryParse), which
can incorrectly mark archived content as hydrated; change the logic in the
tarTiers construction and any subsequent use (the code around tarTiers lookup
where BlobTier.Hot is used) to treat a missing tarTiers entry as an error
condition: either fail the repair with a clear exception/return or explicitly
skip that thin-chunk entry (do not synthesize BlobTier.Hot), so that unknown
parent TAR tiers are not persisted as hydrated state.
In `@src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs`:
- Around line 45-50: The HydrationStatus setter in FileItemViewModel assigns
ChunkHydrationStatus.Unknown as a no-op so the hydration UI presentation never
gets initialized; fix by ensuring the Unknown presentation is applied: either
call OnHydrationStatusChanged(HydrationStatus) once after the constructor
finishes (so the Unknown case applies the label/color), or initialize the
backing presentation fields to the same values used for
ChunkHydrationStatus.Unknown (so the constructor seeds the UI state); update
FileItemViewModel to use one of these approaches referencing the HydrationStatus
property and OnHydrationStatusChanged method.
---
Nitpick comments:
In `@openspec/changes/2026-06-10-rewrite/PLAN.MD`:
- Around line 64-66: Add a language label to the fenced code block that contains
the ASCII pipeline diagram (the block starting with ``` and the content `Walk
(×1) ─► walkItemChannel (bounded) ─► Resolve (×1) ─► entryChannel (bounded) ─►
Handle (yield)`) by changing the opening fence to ```text so the block is
explicitly marked as plain text; this is a purely cosmetic markdown hygiene
change and requires updating only the fenced code block around that pipeline
string.
In `@README.md`:
- Around line 310-312: Update the unlabeled fenced code block that shows the
shard line format example (the block containing "`<content-hash> <chunk-hash>
<original-size> <compressed-size> <tier>`") to include an explicit language
label by changing the opening fence to "```text" so the example is rendered as
plain text and improves markdown hygiene.
In `@src/Arius.AzureBlob/AzureBlobContainerService.cs`:
- Around line 184-198: Listing code currently skips or assumes Tier behavior
based on BlobTraits but AccessTier can be present even when traits ==
BlobTraits.None and can also be null; update the blob listing in GetBlobsAsync
so it always calls FromAzureTier(item.Properties.AccessTier) for Tier (do not
gate it behind includeMetadata/traits), modify FromAzureTier to accept a
nullable AccessTier and return a consistent value for null inputs, and update
the in-memory/fake implementation and listing tests to cover the null-AccessTier
case and the presence-of-AccessTier-when-traits-none scenario (refer to
GetBlobsAsync, BlobListItem, FromAzureTier, and the fake container/listing test
helpers).
In `@src/Arius.Cli.Tests/Commands/Ls/LsStateFormatterTests.cs`:
- Around line 38-58: Add explicit assertions to LsStateFormatterTests to cover
the rehydrating and unknown-tier outputs emitted by LsStateFormatter.ToMarkup:
update or add tests (e.g., in ToMarkup_PresentFlags_AreColored or new test
methods) that call LsStateFormatter.ToMarkup with state combinations that
produce the rehydrating marker (~) and the unknown-tier marker (?) and assert
the exact markup strings include "~" and "?" with the expected
coloring/formatting; reference LsStateFormatter.ToMarkup and
RepositoryEntryState flags to construct the input states that produce those
outputs so the tests fail if those markers are omitted or misformatted.
🪄 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: 323d16a0-858c-4134-9455-4ecf3e5758ab
📒 Files selected for processing (37)
AGENTS.mdREADME.mdopenspec/changes/2026-06-10-rewrite/2026-06-10-234022-local-command-caveatcaveat-the-messages-below.txtopenspec/changes/2026-06-10-rewrite/2026-06-11-010913-this-session-is-being-continued-from-a-previous-c.txtopenspec/changes/2026-06-10-rewrite/PLAN.MDsrc/Arius.AzureBlob/AzureBlobContainerService.cssrc/Arius.Cli.Tests/Commands/Ls/ListQueryParsingTests.cssrc/Arius.Cli.Tests/Commands/Ls/LsStateFormatterTests.cssrc/Arius.Cli/Commands/Ls/LsStateFormatter.cssrc/Arius.Cli/Commands/Ls/LsVerb.cssrc/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/ChunkIndexLocalStoreTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceArchiveScenarioTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceFlushTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceLookupTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceRepairTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardSerializerTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardTests.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/Shared/ChunkIndex/ChunkIndexLocalStore.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkIndex/IChunkIndexService.cssrc/Arius.Core/Shared/ChunkIndex/Shard.cssrc/Arius.Core/Shared/Storage/IBlobContainerService.cssrc/Arius.Explorer.Tests/RepositoryExplorer/FileItemViewModelTests.cssrc/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cssrc/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceArchiveScenarioTests.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openspec/changes/2026-06-10-rewrite-restore/PLAN.MD (2)
78-79: 💤 Low valueClarify concurrent collection choice for re-route rehydration set.
Lines 78-79 and 174 mention "ConcurrentDictionary/ConcurrentBag" for the ×N-written re-route rehydration set but don't specify which. ConcurrentDictionary supports deduplication (if the same chunk fails multiple times), while ConcurrentBag is append-only. For a rehydration set, deduplication is likely desired.
Consider specifying ConcurrentDictionary<ChunkHash, byte> (or a ConcurrentHashSet-equivalent) in the plan.
🤖 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-10-rewrite-restore/PLAN.MD` around lines 78 - 79, Clarify the concurrent collection choice for the "re-route rehydration set" by replacing the ambiguous "ConcurrentDictionary/ConcurrentBag" references with a deduplicating structure; specify using ConcurrentDictionary<ChunkHash, byte> (or a ConcurrentHashSet-equivalent) for the ×N-written re-route rehydration set so repeated failed-chunk entries are deduplicated rather than appended, and update both the mentions around "re-route rehydration set" and any usage notes to reflect deduplication semantics.
203-206: ⚡ Quick winAvoid coupling the scattered-tar test to implementation details.
Lines 205-206 suggest "counting
DownloadAsynccalls onFakeInMemoryBlobContainerService" to validate that each chunk is downloaded exactly once. This couples the test to the fake's call-count implementation detail rather than testing the behavior (files are restored correctly).Based on learnings, prefer testing behavior over implementation. Instead, the test could:
- Assert that all scattered-tar files are restored with correct content and timestamps.
- Optionally verify bounded memory (e.g., no full-tree materialization) by measuring peak allocations if needed, but avoid asserting on internal method call counts.
Based on learnings: "Avoid coupling tests to implementation details. Test the behavior, not the implementation."
🤖 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-10-rewrite-restore/PLAN.MD` around lines 203 - 206, Update the "scattered-tar" test to avoid asserting on FakeInMemoryBlobContainerService.DownloadAsync call counts; instead validate behavior by asserting all scattered files are restored with correct contents and timestamps (and any expected metadata), and if you need to assert resource bounds do so via external observable metrics (e.g., peak memory/allocations) rather than internal method call counts; remove or replace the DownloadAsync call-count assertion in the scattered-tar test and rely on file-level restoration assertions (referencing the scattered-tar test and FakeInMemoryBlobContainerService.DownloadAsync to locate the change).Source: Learnings
🤖 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 `@openspec/changes/2026-06-10-rewrite-restore/PLAN.MD`:
- Around line 197-214: Update the PLAN.MD verification section to adopt a TDD
workflow: before implementing the rewrite, add failing tests that drive the new
streaming behavior (create new tests such as a scattered-tar test and
cancel-rehydration test in RestoreCommandHandlerTests and add
bounded-memory/streaming tests), then implement the pipeline incrementally
across the Walk → Route → Resolve → Pass1 → Pass2 stages making one stage green
at a time, and commit after each stage passes; also ensure CLI parsing tests
(RestoreCommandTests) remain unchanged and run dotnet test/dotnet build after
each stage to catch analyzer/nullable warnings early.
---
Nitpick comments:
In `@openspec/changes/2026-06-10-rewrite-restore/PLAN.MD`:
- Around line 78-79: Clarify the concurrent collection choice for the "re-route
rehydration set" by replacing the ambiguous "ConcurrentDictionary/ConcurrentBag"
references with a deduplicating structure; specify using
ConcurrentDictionary<ChunkHash, byte> (or a ConcurrentHashSet-equivalent) for
the ×N-written re-route rehydration set so repeated failed-chunk entries are
deduplicated rather than appended, and update both the mentions around "re-route
rehydration set" and any usage notes to reflect deduplication semantics.
- Around line 203-206: Update the "scattered-tar" test to avoid asserting on
FakeInMemoryBlobContainerService.DownloadAsync call counts; instead validate
behavior by asserting all scattered files are restored with correct contents and
timestamps (and any expected metadata), and if you need to assert resource
bounds do so via external observable metrics (e.g., peak memory/allocations)
rather than internal method call counts; remove or replace the DownloadAsync
call-count assertion in the scattered-tar test and rely on file-level
restoration assertions (referencing the scattered-tar test and
FakeInMemoryBlobContainerService.DownloadAsync to locate 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: 10b5e5b7-f975-4cc7-b09b-7eb802f7bf9f
📒 Files selected for processing (13)
README.mdopenspec/changes/2026-06-10-rewrite-list/2026-06-10-234022-local-command-caveatcaveat-the-messages-below.txtopenspec/changes/2026-06-10-rewrite-list/2026-06-11-010913-this-session-is-being-continued-from-a-previous-c.txtopenspec/changes/2026-06-10-rewrite-list/PLAN.MDopenspec/changes/2026-06-10-rewrite-list/PLAN2-Coderabbit fixes.MDopenspec/changes/2026-06-10-rewrite-restore/PLAN.MDsrc/Arius.Cli.Tests/Commands/Ls/LsStateFormatterTests.cssrc/Arius.Cli/Commands/Ls/LsVerb.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceRepairTests.cssrc/Arius.Core/Features/ListQuery/ListQuery.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs
💤 Files with no reviewable changes (1)
- openspec/changes/2026-06-10-rewrite-list/2026-06-11-010913-this-session-is-being-continued-from-a-previous-c.txt
✅ Files skipped from review due to trivial changes (3)
- openspec/changes/2026-06-10-rewrite-list/PLAN2-Coderabbit fixes.MD
- openspec/changes/2026-06-10-rewrite-list/PLAN.MD
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs
- src/Arius.Cli.Tests/Commands/Ls/LsStateFormatterTests.cs
- src/Arius.Cli/Commands/Ls/LsVerb.cs
- src/Arius.Core/Features/ListQuery/ListQuery.cs
- src/Arius.Core/Features/ListQuery/ListQueryHandler.cs
Comment on lines
+197
to
+214
| ## Verification | ||
|
|
||
| - **Unit/handler tests** (`src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs`): | ||
| all existing tests must pass unchanged (behavior preserved) — large files, tar duplicates, zero-byte | ||
| tar entries, invalid-hash failure, `TargetPath` filtering, `NoPointers`, missing-container. Run | ||
| `dotnet test` for `Arius.Core.Tests`. | ||
| - Add a **scattered-tar** test: several distinct small files that land in one tar bundle but live at | ||
| distant tree paths (e.g. `a/…`, `z/…`); assert all are restored correctly. Optionally assert each | ||
| chunk is downloaded exactly once by counting `DownloadAsync` calls on `FakeInMemoryBlobContainerService` | ||
| (== distinct available chunks) — validates the refcount flush avoids re-downloads. | ||
| - Add a **cancel-rehydration** test (returns `FilesRestored = 0`, downloads nothing) using a fake | ||
| that reports an archive-tier chunk — confirms the two-pass barrier. | ||
| - **CLI tests** (`src/Arius.Cli.Tests/Commands/Restore/RestoreCommandTests.cs`): parsing tests unchanged | ||
| (no new CLI options/args). Verify `CliHarness` still builds. | ||
| - **End-to-end manual check:** archive a small tree then restore it via the CLI (`/run` or | ||
| `dotnet run -- archive … && dotnet run -- restore …`) into an empty dir; confirm files + pointers + | ||
| timestamps and the progress UI (now showing download queue depth) render correctly. | ||
| - Build: `dotnet build` the solution; ensure no analyzer/nullable warnings introduced. |
Contributor
There was a problem hiding this comment.
Consider a Test-Driven Development approach for this rewrite.
The plan lists verification steps at the end, after the full pipeline design. Based on learnings, the preferred workflow is: "first write a failing test, then implement, then make a conventional git commit when tests pass."
For a rewrite of this scope—particularly one addressing memory bottlenecks and streaming correctness in a backup tool—discovering issues early via TDD provides higher confidence. Consider restructuring the implementation plan to:
- Write failing tests for the new streaming behavior first (e.g., scattered-tar, cancel-rehydration, bounded-memory constraints).
- Implement stages incrementally (Walk → Route → Resolve → Pass 1 → Pass 2), making tests green one stage at a time.
- Commit after each stage passes its tests.
This reduces the risk of discovering design issues late and aligns with the "work in small steps" learning.
Based on learnings: "Work in small steps using Test-Driven Development: first write a failing test, then implement, then make a conventional git commit when tests pass."
🧰 Tools
🪛 LanguageTool
[grammar] ~199-~199: Ensure spelling is correct
Context: ...Verification - Unit/handler tests (`src/Arius.Core.Tests/Features/RestoreCommand/Rest...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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-10-rewrite-restore/PLAN.MD` around lines 197 - 214,
Update the PLAN.MD verification section to adopt a TDD workflow: before
implementing the rewrite, add failing tests that drive the new streaming
behavior (create new tests such as a scattered-tar test and cancel-rehydration
test in RestoreCommandHandlerTests and add bounded-memory/streaming tests), then
implement the pipeline incrementally across the Walk → Route → Resolve → Pass1 →
Pass2 stages making one stage green at a time, and commit after each stage
passes; also ensure CLI parsing tests (RestoreCommandTests) remain unchanged and
run dotnet test/dotnet build after each stage to catch analyzer/nullable
warnings early.
Source: Learnings
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
Notes
Summary by CodeRabbit
Release Notes
New Features
arius lsoutput with file state indicators (showing hydration, availability, and local presence)[path]argument toarius lsfor validating local file overlay directoriesImprovements
Documentation
lsfiltering and snapshot selection options