feat: Retrofit Arius.Explorer to Arius7.Core#38
Conversation
…ositoryEntry> - LsCommand now implements IStreamQuery<RepositoryEntry> (was ICommand<LsResult>) - LsHandler rewrites to IStreamQueryHandler, emitting RepositoryFileEntry and RepositoryDirectoryEntry as a discriminated union with cloud/local merge fields - LsVerb updated to consume mediator.CreateStream with await foreach; error handling via exception catch instead of LsResult.Success check - CliTests mock updated to IStreamQueryHandler<LsCommand, RepositoryEntry> - PipelineFixture.LsAsync and LsIntegrationTests adapted to List<RepositoryFileEntry> - ServiceCollectionExtensions registers LsHandler as IStreamQueryHandler
…-3.11) New tests covering: recursive vs non-recursive depth, case-insensitive filename filter (dirs not filtered), directory merge flags (cloud+local/cloud-only/local-only), per-directory batch size lookup with null for unknown hashes, snapshot-not-found exception (no snapshots and specific version), and cancellation stops enumeration at directory boundaries.
Add ContainerNamesQuery and ContainerNamesQueryHandler to stream repository container names by detecting the snapshots/ prefix, plus a focused unit test project covering the handler's filtering behavior.
…e APIs Update Arius.Explorer to restore/build with central package versions, add a per-repository session for rebuilding core services with selected repository credentials, switch container discovery to the new AzureBlob ContainerNamesQuery, and replace the old pointer-file listing flow with streaming LsCommand-based tree loading plus RestoreCommand-based downloads.
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Arius.Core.Tests/FileTree/TreeServiceTests.cs (2)
534-570:⚠️ Potential issue | 🟡 MinorClear the disk cache before the first build in this test.
This case only cleans
TreeBuilder.GetDiskCacheDirectory("acc", "con")infinally. If a previous failed run left entries behind, the first build can hit disk-cache dedup and makeuploadCount1nondeterministically zero.As per coding guidelines, `**/*.Tests/**`: Test projects. Focus on test coverage gaps and assertion quality rather than style.Suggested patch
[Test] public async Task BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk() { // Pre-populate the disk cache so the builder skips the upload var manifestPath = Path.GetTempFileName(); + var cacheDir = TreeBuilder.GetDiskCacheDirectory("acc", "con"); try { + if (Directory.Exists(cacheDir)) + Directory.Delete(cacheDir, recursive: true); + var now = new DateTimeOffset(2024, 1, 1, 0, 0, 0, TimeSpan.Zero); var entry = new ManifestEntry("file.txt", "hash1", now, now); await File.WriteAllTextAsync(manifestPath, entry.Serialize() + "\n"); @@ finally { File.Delete(manifestPath); - // Clean up disk cache - var cacheDir = TreeBuilder.GetDiskCacheDirectory("acc", "con"); if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/FileTree/TreeServiceTests.cs` around lines 534 - 570, The test BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk can flake if a prior run left items in the disk cache; before the first builder.BuildAsync call delete/clear the disk cache directory obtained from TreeBuilder.GetDiskCacheDirectory("acc", "con") so the test starts with an empty cache, then proceed to create the manifest, run the first build (assert uploadCount1 > 0), and continue with the second build assertions; ensure the cache cleanup is done synchronously before creating FakeBlobService/builder to guarantee deterministic behavior.
357-380:⚠️ Potential issue | 🟡 MinorAdd coverage for the remote-hit dedup branch.
FakeBlobService.GetMetadataAsync()always returnsExists = false, so this suite never exercises theEnsureUploadedAsync()path that skips uploads when the tree blob already exists in the container. TheIBlobContainerServicemigration can regress there without a failing test.As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/FileTree/TreeServiceTests.cs` around lines 357 - 380, FakeBlobService.GetMetadataAsync always returns Exists = false so the remote-hit dedup path in EnsureUploadedAsync is never exercised; change GetMetadataAsync to return Exists = true when the requested blobName is already present in the FakeBlobService.Uploaded set (and false otherwise), then add a test that either pre-populates Uploaded or calls EnsureUploadedAsync twice for the same tree blob and asserts that no second upload occurred (check Uploaded content/count and that HeadChecked contains the blobName) to cover the skip-upload branch.src/Arius.Cli.Tests/CliTests.cs (1)
71-97:⚠️ Potential issue | 🟡 MinorNo test currently proves
lswires the hydration stream.
CliHarnessnow registersHydrationHandler, but none of theLsCommandTestsassert thatResolveFileHydrationStatusesCommandis ever sent. If the CLI stops requesting hydration statuses, this file still goes green.As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.Also applies to: 237-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli.Tests/CliTests.cs` around lines 71 - 97, Add an assertion in the LsCommandTests to verify the CLI actually requests hydration statuses: after exercising the CLI (the code that uses CliBuilder.BuildRootCommand and invokes the ls command), assert that the mocked HydrationHandler received a call to Handle with a ResolveFileHydrationStatusesCommand, e.g. verify hydrationHandler.Handle(Arg.Any<ResolveFileHydrationStatusesCommand>(), Arg.Any<CancellationToken>()) was called; this uses the existing HydrationHandler/LsHandler mocks that are registered in the service provider in the test setup so the test will fail if the CLI stops wiring ResolveFileHydrationStatusesCommand.src/Arius.Core/Features/Archive/ArchivePipelineHandler.cs (1)
173-213:⚠️ Potential issue | 🔴 CriticalDon't let hash-stage exceptions turn into partial "success" runs.
hashTaskhere is the continuation returned byContinueWith, not theParallel.ForEachAsynctask itself. If a hash worker faults, the continuation still closeshashedChannel, downstream stages drain normally, and the original failure is never surfaced. The pipeline completes as success despite missing hashes—a silent data loss condition in the archive. Additionally,hashTaskis never awaited in the final synchronization sequence, compounding the risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/Features/Archive/ArchivePipelineHandler.cs` around lines 173 - 213, The continuation assigned to hashTask uses ContinueWith to call hashedChannel.Writer.Complete(), which hides exceptions from the underlying Parallel.ForEachAsync and is never awaited later; change the code to capture and await the actual Parallel.ForEachAsync task (the return of Parallel.ForEachAsync) instead of its ContinueWith, and ensure hashedChannel.Writer.Complete() is called in a finally block so any exception from Parallel.ForEachAsync (hashing workers) propagates to callers: locate the Parallel.ForEachAsync invocation and the hashedChannel.Writer.Complete() call, remove the ContinueWith usage, store the ForEachAsync Task in hashTask, and wrap awaiting it in try/finally where Complete() is invoked in finally so faults are not swallowed.
🟡 Minor comments (14)
src/Arius.Explorer/CLAUDE.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorFix filename casing for the root guidance doc reference.
Line 3 says
CLAUDE.MD; useCLAUDE.mdto avoid ambiguity on case-sensitive environments.Proposed docs fix
-This file contains additional guidance, on top of the CLAUDE.MD in the root of the repository, specifically for the Arius.Explorer project. +This file contains additional guidance, on top of the CLAUDE.md in the root of the repository, specifically for the Arius.Explorer project.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/CLAUDE.md` at line 3, Update the referenced filename string "CLAUDE.MD" to the correct casing "CLAUDE.md" in the Arius.Explorer docs so the root guidance link resolves on case-sensitive filesystems; locate the occurrence of the token "CLAUDE.MD" in the text and replace it with "CLAUDE.md" (preserve surrounding wording and formatting).src/Arius.Explorer/ChooseRepository/PasswordBoxBehavior.cs-25-29 (1)
25-29:⚠️ Potential issue | 🟡 MinorApply the cached password value when the behavior attaches.
OnPasswordPropertyChangedintentionally skips updates whileAssociatedObjectis null (line 48), butOnAttachednever applies the cachedPasswordvalue afterward. If the source property has a value when the behavior attaches, thePasswordBoxremains blank until the source changes again.💡 Proposed fix
protected override void OnAttached() { base.OnAttached(); + AssociatedObject.Password = Password ?? string.Empty; AssociatedObject.PasswordChanged += OnPasswordChanged; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/ChooseRepository/PasswordBoxBehavior.cs` around lines 25 - 29, On attach the behavior subscribes to PasswordChanged but never applies the cached Password value, so update OnAttached to apply the current Password to the AssociatedObject: after subscribing in OnAttached, if AssociatedObject != null and Password != AssociatedObject.Password, assign AssociatedObject.Password = Password (or call the same internal path OnPasswordPropertyChanged/ApplyPassword) while temporarily suppressing the PasswordChanged handler (e.g. set the existing update-guard flag used in OnPasswordPropertyChanged) to avoid re-entrancy; this ensures the PasswordBox shows the source value when the behavior is attached.openspec/changes/archive/2026-04-01-wire-explorer-to-core/proposal.md-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorMinor inconsistency:
LsEntryvsRepositoryEntry.The proposal mentions
IAsyncEnumerable<LsEntry>, but the actual implementation inListModels.csusesRepositoryEntryas the base type. Consider updating for consistency with the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-04-01-wire-explorer-to-core/proposal.md` at line 7, The proposal text is inconsistent with the implementation: it references IAsyncEnumerable<LsEntry> while ListModels.cs uses RepositoryEntry; update the proposal to match the code (or vice versa) by replacing LsEntry with RepositoryEntry (or change ListModels.cs to define/return LsEntry) so the discriminated-union type name is consistent across the proposal and implementation—locate references to LsEntry in the proposal and the RepositoryEntry type and ListModels.cs method signature to align the names.src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerWindow.xaml.cs-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorFix misleading XML documentation comment.
The XML doc references "ChooseRepositoryWindow.xaml" but this file is
RepositoryExplorerWindow. This appears to be a copy-paste error.📝 Proposed fix
/// <summary> -/// Interaction logic for ChooseRepositoryWindow.xaml +/// Interaction logic for RepositoryExplorerWindow.xaml /// </summary>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerWindow.xaml.cs` around lines 5 - 7, The XML documentation summary at the top of RepositoryExplorerWindow.xaml.cs incorrectly names "ChooseRepositoryWindow.xaml"; update the XML comment to refer to the correct view/class by replacing that text with "RepositoryExplorerWindow.xaml" (or a matching description for the RepositoryExplorerWindow class) so the summary accurately reflects the RepositoryExplorerWindow class in this file.src/Arius.Cli/Commands/Ls/LsVerb.cs-121-145 (1)
121-145:⚠️ Potential issue | 🟡 MinorDon't treat cancellation as an
lsfailure.
catch (Exception)also catchesOperationCanceledException, so Ctrl+C currently falls through to the genericLs failed:path instead of behaving like a cancellation.Suggested change
- catch (Exception ex) + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) { AnsiConsole.MarkupLine($"[red]Ls failed:[/] {Markup.Escape(ex.Message)}"); return 1; }As per coding guidelines,
src/Arius.Cli/**: CLI entry point. Validate command argument handling and user-facing error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Ls/LsVerb.cs` around lines 121 - 145, The catch-all in the Ls handling (around awaiting mediator.CreateStream(new ListRepositoryEntriesCommand(opts), ct)) incorrectly treats OperationCanceledException as an error; update the try/catch so cancellation is handled separately: either add a specific catch (OperationCanceledException) { return 0; } or rethrow when ct.IsCancellationRequested before the generic catch, ensuring that OperationCanceledException is not reported via AnsiConsole.MarkupLine("Ls failed:...") and that the current code paths using AnsiConsole.MarkupLine and return 1 remain for other exceptions.src/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cs-120-143 (1)
120-143:⚠️ Potential issue | 🟡 MinorThe positive-path
OpenRepositoryCommandtest is still disabled.
Skip.Test("TODO")means this PR does not actually verify command enablement or repository construction for the valid configuration.I can flesh out the happy-path assertions if you want a follow-up.
As per coding guidelines,**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cs` around lines 120 - 143, The test OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepository is disabled via Skip.Test("TODO"); remove that skip and enable the test so it actually executes the positive path; instantiate ChooseRepositoryViewModel, set LocalDirectoryPath, AccountName, AccountKey, ContainerName, Passphrase as shown, assert OpenRepositoryCommand.CanExecute(null) is true, call OpenRepositoryCommand.Execute(null), then assert viewModel.Repository is not null and its properties (LocalDirectoryPath, AccountName, ContainerName, AccountKey, Passphrase) match the inputs to verify repository construction.src/Arius.Explorer/Settings/RepositoryOptions.cs-29-32 (1)
29-32:⚠️ Potential issue | 🟡 Minor
ToString()may throw ifLocalDirectoryis invalid.Since
ToString()accessesLocalDirectory.FullName, it will propagate any exception from theLocalDirectorygetter. If the defensive change above is applied, this would also need adjustment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/Settings/RepositoryOptions.cs` around lines 29 - 32, The ToString() in RepositoryOptions accesses LocalDirectory.FullName and can throw if LocalDirectory is null/invalid; update RepositoryOptions.ToString() to safely handle missing/invalid LocalDirectory (e.g., use LocalDirectory?.FullName or catch exceptions from accessing FullName) and return a fallback representation like "<invalid-local-dir> on {AccountName}:{ContainerName}" so ToString() never propagates an exception; ensure the change aligns with any earlier defensive getter changes for LocalDirectory.src/Arius.Explorer/App.xaml.cs-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorEnable nullable annotations or remove the
?annotation.The
IServiceProvider?declaration on line 17 is not compiler-enforced because nullable reference types are not enabled in the project. Add<Nullable>enable</Nullable>toArius.Explorer.csproj(or Directory.Build.props) to enforce null-safety checks, or remove the?if nullable is intentionally disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/App.xaml.cs` at line 17, The ServiceProvider property in App.xaml.cs is annotated as nullable (public static IServiceProvider? ServiceProvider) but the project doesn't have nullable reference types enabled; either enable nullable annotations by adding <Nullable>enable</Nullable> to Arius.Explorer.csproj (or a common Directory.Build.props) so the compiler enforces the '?' annotation, or remove the '?' from the ServiceProvider declaration to match the current project settings; update App.xaml.cs (the ServiceProvider property) and the project file accordingly so annotations and compiler settings are consistent.src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs-35-69 (1)
35-69:⚠️ Potential issue | 🟡 MinorAssert the secondary metadata probe in the archive-path cases.
These tests only pin the final enum, so a regression that stops checking
chunks-rehydrated/{hash}would still pass for the pending/needs branches. AddRequestedBlobNamesassertions here too.💡 Minimal fix
var status = await FileHydrationStatusResolver.ResolveAsync(blobs, "abc", CancellationToken.None); status.ShouldBe(FileHydrationStatus.RehydrationPending); + blobs.RequestedBlobNames.ShouldBe(["chunks/abc", "chunks-rehydrated/abc"]);var status = await FileHydrationStatusResolver.ResolveAsync(blobs, "abc", CancellationToken.None); status.ShouldBe(FileHydrationStatus.RehydrationPending); + blobs.RequestedBlobNames.ShouldBe(["chunks/abc", "chunks-rehydrated/abc"]);var status = await FileHydrationStatusResolver.ResolveAsync(blobs, "abc", CancellationToken.None); status.ShouldBe(FileHydrationStatus.NeedsRehydration); + blobs.RequestedBlobNames.ShouldBe(["chunks/abc", "chunks-rehydrated/abc"]);As per coding guidelines, "Test projects. Focus on test coverage gaps and assertion quality rather than style."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cs` around lines 35 - 69, The tests in FileHydrationStatusResolverTests call FileHydrationStatusResolver.ResolveAsync but don't assert that the resolver actually probed the secondary blob name ("chunks-rehydrated/{hash}"), so a regression could skip that check; update the three archive-path tests (methods ResolveAsync_ReturnsPending_WhenArchiveChunkIsRehydrating, ResolveAsync_ReturnsPending_WhenRehydratedCopyExistsButStillArchive, ResolveAsync_ReturnsNeedsRehydration_WhenArchiveChunkHasNoRehydratedCopy) to also assert on the FakeBlobContainerService.RequestedBlobNames contains the expected "chunks-rehydrated/abc" entry after calling FileHydrationStatusResolver.ResolveAsync (use the same blobs instance and "abc" key used in the test), keeping the existing enum assertions intact.src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs-35-35 (1)
35-35:⚠️ Potential issue | 🟡 MinorInitialize the
Unknownhydration UI state explicitly.When
file.Hydratedisnull, the constructor assignsHydrationStatus = Unknown, which matches the backing field's initial value on Line 35. CommunityToolkit.Mvvm's[ObservableProperty]generated setter skips all callbacks (includingOnHydrationStatusChanged) when the assigned value equals the current value, leavingChunkStateColorunset and the tooltip at the stale default for "status still loading" items.💡 Minimal fix
HydrationStatus = file.Hydrated switch { true => FileHydrationStatus.Available, false => FileHydrationStatus.NeedsRehydration, null => FileHydrationStatus.Unknown, }; OriginalLength = file.OriginalSize ?? 0; + OnHydrationStatusChanged(HydrationStatus);Also applies to: 46-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs` at line 35, The backing field hydrationStatus is initialized to FileHydrationStatus.Unknown but the constructor assigns the backing field (or leaves it unchanged) when file.Hydrated is null which prevents the [ObservableProperty]-generated setter for HydrationStatus from firing OnHydrationStatusChanged and updating ChunkStateColor/tooltip; change the constructor(s) in FileItemViewModel to assign the HydrationStatus property (not just the backing field) to FileHydrationStatus.Unknown when file.Hydrated is null so the generated setter runs OnHydrationStatusChanged and initializes ChunkStateColor and the tooltip; also update any other places (lines 46–54 area) that set the backing field directly to instead set the HydrationStatus property.src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs-66-72 (1)
66-72:⚠️ Potential issue | 🟡 MinorDon’t render every
Unknownhydration state as “not loaded yet”.The fallback branch is also used for local-only entries and for resolved-but-unknown cases coming back from core, so this text becomes wrong after loading has already finished. If the loading state matters, it needs its own flag or enum value.
💡 Minimal fix
StateTooltip = value switch { FileHydrationStatus.Available => "Cloud chunk is available for download", FileHydrationStatus.NeedsRehydration => "Cloud chunk is archived and must be rehydrated first", FileHydrationStatus.RehydrationPending => "Cloud chunk rehydration is already pending", - _ => "Cloud chunk status not loaded yet", + _ when !File.ExistsInCloud => "File exists only locally", + _ => "Cloud chunk status unknown", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cs` around lines 66 - 72, The StateTooltip switch on FileHydrationStatus currently uses the catch-all (_) to show "not loaded yet", which incorrectly labels local-only and resolved-but-unknown states; update the switch in FileItemViewModel so you handle specific enum values instead of treating everything else as "not loaded": add explicit cases for local-only (e.g., FileHydrationStatus.LocalOnly) with a tooltip like "File is local only (no cloud chunk)", add an explicit Unknown/ResolvedUnknown case with "Cloud chunk status unknown", and reserve a separate Loading/NotLoaded value if you need to show "not loaded yet" rather than using the default branch; reference the StateTooltip property and the FileHydrationStatus enum when making these changes.src/Arius.Explorer/App.xaml.cs-65-69 (1)
65-69:⚠️ Potential issue | 🟡 MinorDispose the root DI container during shutdown.
The
hostobject created inProgram.csis never disposed afterhost.StopAsync()completes. SinceApp.ServiceProvideris assigned fromhost.Services(the root service provider), disposing it will clean up IDisposable/IAsyncDisposable singletons (e.g.,IRepositorySession) and prevent Serilog log loss. Add disposal inOnExit():Example fix
protected override void OnExit(ExitEventArgs e) { var logger = ServiceProvider?.GetService<ILogger<App>>(); logger?.LogInformation("Application shutting down"); // Dispose the root DI container to clean up singletons if (ServiceProvider is IAsyncDisposable asyncDisposable) asyncDisposable.DisposeAsync().GetAwaiter().GetResult(); else if (ServiceProvider is IDisposable disposable) disposable.Dispose(); base.OnExit(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/App.xaml.cs` around lines 65 - 69, The OnExit method currently logs shutdown but does not dispose the root DI container (ServiceProvider), so singletons implementing IDisposable/IAsyncDisposable are not cleaned up; update OnExit (in App.xaml.cs) to detect if ServiceProvider is IAsyncDisposable and call DisposeAsync().GetAwaiter().GetResult(), otherwise if it is IDisposable call Dispose(), ensuring the root provider created from host.Services in Program.cs is disposed before calling base.OnExit(e) while preserving the existing ILogger<App> shutdown log.openspec/changes/archive/2026-04-01-wire-explorer-to-core/tasks.md-3-14 (1)
3-14:⚠️ Potential issue | 🟡 MinorThe archived task list is still describing the removed API surface.
This archive still refers to
LsCommand,LsHandler,IBlobStorageService,Arius.AzureBlob, andPointerFileEntriesQuery, while the implementation in this PR has already moved toListRepositoryEntriesCommand,ListRepositoryEntriesHandler,IBlobContainerService, Core-sideContainerNamesQuery, and the Explorer list/hydration flow. That makes the archived change misleading for future readers.Also applies to: 18-18, 43-43, 52-53, 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-04-01-wire-explorer-to-core/tasks.md` around lines 3 - 14, The archived task list still references removed APIs (LsCommand, LsHandler, IBlobStorageService, Arius.AzureBlob, PointerFileEntriesQuery) and must be updated to reflect the current implementation (ListRepositoryEntriesCommand, ListRepositoryEntriesHandler, IBlobContainerService, ContainerNamesQuery and the Explorer list/hydration flow); edit the archive text to replace old symbol names with the new ones and remove or rewrite any steps that describe the removed surface so they accurately describe the current streaming/merged listing behavior and flow used by ListRepositoryEntriesHandler and the Explorer hydration path.src/Arius.Core.Tests/Ls/LsHandlerTests.cs-373-416 (1)
373-416:⚠️ Potential issue | 🟡 MinorThe “CalledOncePerDirectory” part of this test name is not asserted.
This only verifies the resulting
OriginalSizevalues. It would still pass if the lookup happened once per file, so the batching behavior is currently uncovered. Either instrument the lookup path or rename the test to match what it actually proves. As per coding guidelines,**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Ls/LsHandlerTests.cs` around lines 373 - 416, The test name promises batching "CalledOncePerDirectory" but never asserts call counts; replace or augment the ChunkIndexService used in this test with a small spy/mock that increments a counter when the batch-size lookup method is invoked (i.e., the index method the handler calls to resolve sizes), use that spy in MakeHandler instead of the real ChunkIndexService (you can still pre-populate it like index.RecordEntry(...)), then add an assertion in Handle_BatchSizeLookup_CalledOncePerDirectory_SizeNullWhenNotInIndex that the spy’s counter equals the expected number of directory lookups (e.g., 2 for root and child). Alternatively, if you don't want to assert behavior, rename the test to remove "CalledOncePerDirectory" to accurately reflect that it only verifies OriginalSize values.
🧹 Nitpick comments (15)
src/Arius.AzureBlob/BlobServiceFactory.cs (1)
17-21: Add input validation foraccountName.The method constructs a URI from
accountNamewithout validation. IfaccountNameis null or empty, this will produce an unclearUriFormatException. Consider validating early for a clearer error message.🛡️ Proposed validation
public Task<IBlobService> CreateAsync( string accountName, string? accountKey, CancellationToken cancellationToken = default) { + ArgumentException.ThrowIfNullOrWhiteSpace(accountName); + var serviceUri = new Uri($"https://{accountName}.blob.core.windows.net");As per coding guidelines: "Azure Blob Storage integration layer. Watch for proper resource disposal, retry policies, and credential handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.AzureBlob/BlobServiceFactory.cs` around lines 17 - 21, CreateAsync in BlobServiceFactory builds a URI from accountName without validating it; add an early guard in the CreateAsync method to check string.IsNullOrWhiteSpace(accountName) and throw an ArgumentException or ArgumentNullException with a clear message (e.g., "accountName cannot be null, empty or whitespace") so callers get a descriptive error instead of a UriFormatException; keep the check at the top of CreateAsync and do not change the rest of the URI-construction logic or disposal/credential handling..github/workflows/ci.yml (1)
62-82: Windows-only Explorer tests job is appropriately isolated.Running Explorer tests on
windows-latestis necessary due to WPF dependencies (net10.0-windows).Note that coverage is not collected for the Explorer tests job. If Explorer test coverage is desired, consider adding
dotnet-coverage collectwrapper similar to the main test job.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 62 - 82, The Explorer tests job (explorer-tests) runs on windows-latest and currently invokes dotnet test directly so coverage isn't collected; to enable coverage wrap the test invocation similar to the main test job by replacing the dotnet test step (the step that runs "dotnet test --project src/Arius.Explorer.Tests/Arius.Explorer.Tests.csproj --no-build -c Release") with the dotnet-coverage collect invocation used elsewhere (e.g., use the same dotnet-coverage collect wrapper and arguments, ensure the coverage tool is installed/available before the test step, and output the coverage file/artifact as the other test job does).src/Arius.Cli/CliBuilder.cs (1)
118-136: Clean refactoring to container-scoped blob service API.The updated flow correctly separates the storage account connection (
CreateAsync) from container resolution (GetContainerServiceAsync). The use ofConfigureAwait(false)is appropriate for this library code path.Consider propagating a
CancellationTokentoGetContainerServiceAsyncif the method signature supports it, enabling cancellation during container preflight checks:♻️ Optional: Add cancellation support
- var blobContainer = await blobService.GetContainerServiceAsync(containerName, preflightMode).ConfigureAwait(false); + var blobContainer = await blobService.GetContainerServiceAsync(containerName, preflightMode, CancellationToken.None).ConfigureAwait(false);Or accept a
CancellationTokenparameter inBuildProductionServicesif the caller can provide one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/CliBuilder.cs` around lines 118 - 136, The container preflight call should accept a CancellationToken so callers can cancel long-running checks: update BuildProductionServices to take an optional CancellationToken parameter (or accept one from its caller) and pass it through to BlobServiceFactory.CreateAsync/GetContainerServiceAsync where supported, specifically propagate the token into the call to blobService.GetContainerServiceAsync (and CreateAsync if its signature supports it); if the target methods do not currently accept a token, add an overload or update their signatures (BlobServiceFactory.CreateAsync, IBlobService.GetContainerServiceAsync) to accept CancellationToken and thread the token through to any async I/O operations.src/Arius.Architecture.Tests/DependencyTests.cs (1)
75-93: Valuable architectural constraint for handler placement.The test effectively enforces that mediator command and stream query handlers reside only in
Arius.Core, preventing architectural drift. The reflection-based approach is appropriate here.One minor robustness note:
i.FullNamecan theoretically benullfor certain runtime types. Consider using null-conditional orStartsWithpattern:🛡️ Optional defensive check
.Where(type => type.GetInterfaces().Any(i => - i.FullName is "Mediator.ICommandHandler`2" - or "Mediator.IStreamQueryHandler`2")) + i.FullName?.StartsWith("Mediator.ICommandHandler`2") == true + || i.FullName?.StartsWith("Mediator.IStreamQueryHandler`2") == true))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Architecture.Tests/DependencyTests.cs` around lines 75 - 93, Update the interface-check in the Mediator_Command_And_Stream_Handlers_Should_Live_In_Core_Only test to defensively handle potential null FullName values: in the LINQ Where that inspects type.GetInterfaces(), change the predicate for i.FullName to guard against null (e.g., use i.FullName != null && (i.FullName == "Mediator.ICommandHandler`2" || i.FullName == "Mediator.IStreamQueryHandler`2") or use i.FullName?.StartsWith("Mediator.") == true with the specific suffix checks) so the reflection filter in nonCoreHandlerTypes won’t throw on unusual runtime types.src/Arius.Explorer/RepositoryExplorer/StateCircle.xaml (1)
1-22: Well-structured state visualization control.The XAML correctly implements a four-segment circular indicator with proper
RelativeSourcebindings to theUserControl's dependency properties. The geometry paths correctly define left/right halves for both outer and inner rings.Consider adding
ToolTipsupport orAutomationProperties.Namefor accessibility, allowing screen readers to convey the state meaning to users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/StateCircle.xaml` around lines 1 - 22, Add accessibility metadata to the StateCircle UserControl so screen readers can convey the state: set AutomationProperties.Name (and optionally AutomationProperties.HelpText) on the root UserControl and/or each Path, and add a ToolTip bound to a new public dependency property (e.g., ToolTipText or AccessibilityLabel) so callers can supply descriptive text; update the bindings for ToolTip and AutomationProperties.Name to use RelativeSource={RelativeSource AncestorType=UserControl} and expose the new dependency property on the StateCircle control alongside the existing LeftOuterColor/RightOuterColor/LeftInnerColor/RightInnerColor properties.src/Arius.Explorer/Shared/Converters/BytesToReadableSizeConverter.cs (1)
10-17: Consider returning a fallback value instead of null for robustness.Returning
nullfrom a WPF value converter can cause silent binding failures or display issues. Consider returning an empty string or a placeholder (e.g.,"—") for non-longvalues, and handlelong?(nullable) inputs that may come from nullable bindings.♻️ Proposed fix for robustness
public object Convert(object value, Type targetType, object parameter, CultureInfo culture) { if (value is long bytes) { return bytes.Bytes().Humanize("#.#"); } - return null; + // Handle nullable long and provide fallback for other types + if (value is null) + return string.Empty; + return string.Empty; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/Shared/Converters/BytesToReadableSizeConverter.cs` around lines 10 - 17, The Convert method in BytesToReadableSizeConverter currently returns null for non-long inputs; change it to handle nullable longs and return a safe fallback string instead of null: accept value as long or long? (nullable), format when HasValue using bytes.Bytes().Humanize("#.#"), and return an empty string or placeholder like "—" for null/invalid inputs; update the Convert method (and any helper logic) to perform this check and return the fallback to avoid binding/display issues.src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerWindow.xaml.cs (1)
10-20: Consider removing the unused logger field or adding future usage.The
loggerfield is stored but only used once in the constructor. If no additional logging is planned, consider simplifying to a local variable or removing the field entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerWindow.xaml.cs` around lines 10 - 20, The private readonly logger field on RepositoryExplorerWindow is only used inside the constructor; either remove the field and accept ILogger<RepositoryExplorerWindow> as a constructor parameter stored in a local variable (or log directly and not keep it), or keep the field and add planned logging calls in methods on RepositoryExplorerWindow (e.g., event handlers or lifecycle methods) so the field is justified; update the RepositoryExplorerWindow constructor and the private readonly ILogger<RepositoryExplorerWindow> logger declaration accordingly to reflect the chosen approach.src/Arius.Cli/ProgressState.cs (1)
533-549: Fail fast on unknownRestoreDispositionvalues.This switch silently ignores any new enum member, which will skew the checked-stage counters without any obvious failure. A default arm throwing
ArgumentOutOfRangeExceptionmakes future additions explicit.Suggested change
public void IncrementDisposition(RestoreDisposition disposition) { switch (disposition) { case RestoreDisposition.New: Interlocked.Increment(ref _dispositionNew); break; case RestoreDisposition.SkipIdentical: Interlocked.Increment(ref _dispositionSkipIdentical); break; case RestoreDisposition.Overwrite: Interlocked.Increment(ref _dispositionOverwrite); break; case RestoreDisposition.KeepLocalDiffers: Interlocked.Increment(ref _dispositionKeepLocalDiffers); break; + default: + throw new ArgumentOutOfRangeException(nameof(disposition), disposition, "Unhandled restore disposition."); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/ProgressState.cs` around lines 533 - 549, The switch in IncrementDisposition(RestoreDisposition disposition) currently ignores unknown enum values; add a default case that throws ArgumentOutOfRangeException including the disposition value and parameter name to fail fast when a new RestoreDisposition member is introduced. Update the method IncrementDisposition to include a default: throw new ArgumentOutOfRangeException(nameof(disposition), disposition, "Unknown RestoreDisposition") so callers and logs clearly surface unexpected values (references: IncrementDisposition, RestoreDisposition, _dispositionNew, _dispositionSkipIdentical, _dispositionOverwrite, _dispositionKeepLocalDiffers).src/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cs (1)
21-36: MakeCopyCallssafe for parallel rehydration.
RestorePipelineHandlercan issue more than oneCopyAsyncconcurrently, but this test double records into a plainList<>. That will get flaky as soon as a multi-chunk scenario exercises concurrent rehydration.Suggested change
internal sealed class RehydrationSimulatingBlobService(IBlobContainerService inner) : IBlobContainerService { + private readonly object _copyCallsLock = new(); + /// <summary> /// Records each (source, destination) pair passed to <see cref="CopyAsync"/>. /// </summary> public List<(string Source, string Destination)> CopyCalls { get; } = new(); @@ public Task CopyAsync(string sourceBlobName, string destinationBlobName, BlobTier destinationTier, RehydratePriority? rehydratePriority = null, CancellationToken ct = default) { - CopyCalls.Add((sourceBlobName, destinationBlobName)); + lock (_copyCallsLock) + { + CopyCalls.Add((sourceBlobName, destinationBlobName)); + } // Do NOT forward to inner — the source is "archived" and a real copy would fail. return Task.CompletedTask; }As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.Also applies to: 93-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cs` around lines 21 - 36, The test double RehydrationSimulatingBlobService uses a plain List for CopyCalls which is not safe for concurrent CopyAsync calls; change CopyCalls to a thread-safe collection (e.g., ConcurrentQueue<(string Source,string Destination)> or ConcurrentBag<(string, string)>) or protect accesses with a lock, update any asserts/reads in the test to enumerate/consume the chosen concurrent collection, and ensure all places that append to CopyCalls inside the CopyAsync implementation use the new thread-safe API; reference the RehydrationSimulatingBlobService type and the CopyAsync call sites when making this change.src/Arius.Integration.Tests/Pipeline/LsIntegrationTests.cs (1)
102-125: Add one invalid-version integration case.This file now covers only successful
Versionlookups. SinceLsAsyncmoved to exception-based streaming, a nonexistent snapshot version should also be exercised here so the new throw/catch path is protected end-to-end.As per coding guidelines,
**/*.Tests/**: Test projects. Focus on test coverage gaps and assertion quality rather than style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Integration.Tests/Pipeline/LsIntegrationTests.cs` around lines 102 - 125, Add a new integration case exercising the error path when a non-existent snapshot version is requested: extend or add a test (e.g., alongside Ls_WithVersion_ReturnsCorrectSnapshot) that calls fix.LsAsync(new ListRepositoryEntriesCommandOptions { Version = "<invalid-version>" }) and asserts that the method throws the expected exception (or that the thrown exception type/message matches the streaming error contract) to exercise the new exception-based streaming path; reference the existing snapshot1 variable and LsAsync/ListRepositoryEntriesCommandOptions to locate where to add this negative test.src/Arius.Cli/Commands/Ls/LsVerb.cs (1)
115-149: This still materializes the full listing in memory.Every streamed entry is added to
tableand only rendered at Line 148, so large repositories still pay O(n) memory in the CLI layer and users don't see any output until the stream completes. If end-to-end streaming is the goal, prefer writing rows as they arrive or using a bounded live view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Ls/LsVerb.cs` around lines 115 - 149, The current implementation builds a full Spectre.Console Table by calling table.AddRow for every item from mediator.CreateStream in LsVerb (so Table and table.AddRow are the culprits), causing O(n) memory and no incremental output; change the loop to render rows as they arrive instead of accumulating them—either write each row immediately to the console (e.g., create a small per-entry Renderable or use Markup/Columns for a single row) or use Spectre.Console's Live/LiveDisplay APIs to maintain a bounded view while streaming; keep the fileCount increment and final summary, but stop appending to a long-lived Table variable so the CLI streams results and avoids materializing the entire listing.src/Arius.Core.Tests/ContainerNames/ContainerNamesQueryHandlerTests.cs (1)
10-37: Add a cancellation/empty-stream case for the streaming contract.This only pins the happy path. The Explorer flow can cancel and restart container discovery as credentials change, so it would be good to assert both an empty stream and
CancellationTokenpropagation toCreateAsync/GetContainerNamesAsync.🧪 Example coverage to add
[Test] public async Task Handle_PassesCancellationTokenThrough() { var blobServiceFactory = Substitute.For<IBlobServiceFactory>(); var blobService = Substitute.For<IBlobService>(); var serviceProvider = Substitute.For<IServiceProvider>(); serviceProvider.GetService(typeof(IBlobServiceFactory)).Returns(blobServiceFactory); using var cts = new CancellationTokenSource(); blobServiceFactory .CreateAsync("account", "key", cts.Token) .Returns(blobService); blobService .GetContainerNamesAsync(cts.Token) .Returns(_ => Array.Empty<string>().ToAsyncEnumerable()); var handler = new ContainerNamesQueryHandler(serviceProvider); await foreach (var _ in handler.Handle(new ContainerNamesQuery("account", "key"), cts.Token)) { } await blobServiceFactory.Received(1).CreateAsync("account", "key", cts.Token); blobService.Received(1).GetContainerNamesAsync(cts.Token); }As per coding guidelines, "Test projects. Focus on test coverage gaps and assertion quality rather than style."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/ContainerNames/ContainerNamesQueryHandlerTests.cs` around lines 10 - 37, Add tests covering cancellation and empty-stream behavior for ContainerNamesQueryHandler.Handle: ensure the CancellationToken passed into the handler is forwarded to IBlobServiceFactory.CreateAsync and IBlobService.GetContainerNamesAsync and that an empty IAsyncEnumerable is handled without yielding items. Specifically, add a test that creates a CancellationTokenSource, configures the substitute IBlobServiceFactory.CreateAsync(account,key,cts.Token) to return the blob service and configures IBlobService.GetContainerNamesAsync(cts.Token) to return Array.Empty<string>().ToAsyncEnumerable(), then enumerate handler.Handle(new ContainerNamesQuery("account","key"), cts.Token) (no items expected) and assert CreateAsync and GetContainerNamesAsync were each called with the same cts.Token. Ensure you reference ContainerNamesQueryHandler.Handle, IBlobServiceFactory.CreateAsync and IBlobService.GetContainerNamesAsync when implementing the assertions.src/Arius.Explorer/ChooseRepository/ChooseRepositoryWindow.xaml (1)
108-110: Consider making password bindings explicit for clarity.
PasswordBoxBehavior.Passwordis registered withBindsTwoWayByDefault, so the bindings at lines 109 and 139 are already two-way. Adding explicitMode=TwoWayis optional but improves code readability.Optional change for clarity
- <local:PasswordBoxBehavior Password="{Binding AccountKey}" /> + <local:PasswordBoxBehavior Password="{Binding AccountKey, Mode=TwoWay}" /> ... - <local:PasswordBoxBehavior Password="{Binding Passphrase}" /> + <local:PasswordBoxBehavior Password="{Binding Passphrase, Mode=TwoWay}" />Also applies to: 138-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/ChooseRepository/ChooseRepositoryWindow.xaml` around lines 108 - 110, Add explicit two-way binding modes to the PasswordBoxBehavior.Password bindings for clarity: update the Password bindings so they include Mode=TwoWay (e.g., Password="{Binding AccountKey, Mode=TwoWay}") and do the same for the other Password binding around lines 138-140; target the PasswordBoxBehavior.Password binding and the bound view-model properties (AccountKey and the other password property) so both bindings are explicitly two-way.src/Arius.Explorer/Program.cs (1)
22-23: Consider using UTC for log filenames to avoid DST ambiguity.Using
DateTime.Nowcan produce duplicate or out-of-order filenames during daylight saving transitions. UsingDateTime.UtcNowensures consistent, monotonically increasing timestamps.Suggested change
- var logPath = Path.Join(logDirectory, $"arius-explorer-{DateTime.Now:yyyyMMdd_HHmmss}.log"); + var logPath = Path.Join(logDirectory, $"arius-explorer-{DateTime.UtcNow:yyyyMMdd_HHmmss}.log");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/Program.cs` around lines 22 - 23, Replace the local time timestamp used in the log filename with UTC to avoid DST-related duplicates: update the logPath construction in Program.cs where logPath is set (currently using DateTime.Now with the format "yyyyMMdd_HHmmss") to use DateTime.UtcNow instead so filenames use a consistent, monotonic UTC timestamp while leaving the rest of the format and logDirectory logic unchanged.src/Arius.Core/ServiceCollectionExtensions.cs (1)
94-96: InjectIBlobServiceFactorydirectly instead of using service locator pattern.
ContainerNamesQueryHandleronly requiresIBlobServiceFactoryfrom the service provider. Replace theIServiceProviderparameter withIBlobServiceFactoryto make dependencies explicit and improve testability:new ContainerNamesQueryHandler(sp.GetRequiredService<IBlobServiceFactory>())Then update the handler:
public sealed class ContainerNamesQueryHandler(IBlobServiceFactory blobServiceFactory) : IStreamQueryHandler<ContainerNamesQuery, string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core/ServiceCollectionExtensions.cs` around lines 94 - 96, Change the singleton registration to inject IBlobServiceFactory directly instead of the service locator: in the services.AddSingleton<IStreamQueryHandler<ContainerNamesQuery, string>> registration, replace the factory lambda that passes IServiceProvider (sp) to ContainerNamesQueryHandler with one that resolves IBlobServiceFactory via sp.GetRequiredService<IBlobServiceFactory>() and passes that into the handler; then update the ContainerNamesQueryHandler constructor/signature to accept IBlobServiceFactory (and implement IStreamQueryHandler<ContainerNamesQuery, string>) so the dependency is explicit and testable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bfeb6e7-3a78-44d4-a0da-f446c416baa8
⛔ Files ignored due to path filters (1)
src/Arius.Explorer/Resources/iceberg.icois excluded by!**/*.ico
📒 Files selected for processing (130)
.github/workflows/ci.yml.github/workflows/release.ymlopenspec/changes/archive/2026-04-01-wire-explorer-to-core/.openspec.yamlopenspec/changes/archive/2026-04-01-wire-explorer-to-core/design.mdopenspec/changes/archive/2026-04-01-wire-explorer-to-core/proposal.mdopenspec/changes/archive/2026-04-01-wire-explorer-to-core/specs/container-names/spec.mdopenspec/changes/archive/2026-04-01-wire-explorer-to-core/specs/ls-command/spec.mdopenspec/changes/archive/2026-04-01-wire-explorer-to-core/tasks.mdopenspec/specs/container-names/spec.mdopenspec/specs/ls-command/spec.mdsrc/Arius.Architecture.Tests/DependencyTests.cssrc/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csprojsrc/Arius.AzureBlob.Tests/AzureBlobServiceTests.cssrc/Arius.AzureBlob/Arius.AzureBlob.csprojsrc/Arius.AzureBlob/AzureBlobContainerService.cssrc/Arius.AzureBlob/AzureBlobService.cssrc/Arius.AzureBlob/BlobServiceFactory.cssrc/Arius.AzureBlob/Preflight.cssrc/Arius.Cli.Tests/CliTests.cssrc/Arius.Cli.Tests/DependencyInjectionTests.cssrc/Arius.Cli.Tests/ProgressTests.cssrc/Arius.Cli/Arius.Cli.csprojsrc/Arius.Cli/CliBuilder.cssrc/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cssrc/Arius.Cli/Commands/Archive/ArchiveVerb.cssrc/Arius.Cli/Commands/Ls/LsVerb.cssrc/Arius.Cli/Commands/Restore/RestoreProgressHandlers.cssrc/Arius.Cli/Commands/Restore/RestoreVerb.cssrc/Arius.Cli/ProgressState.cssrc/Arius.Core.Tests/ChunkIndex/ShardTests.cssrc/Arius.Core.Tests/ContainerNames/ContainerNamesQueryHandlerTests.cssrc/Arius.Core.Tests/Encryption/AesGcmEncryptionTests.cssrc/Arius.Core.Tests/Encryption/GoldenFileDecryptionTests.cssrc/Arius.Core.Tests/Encryption/PassphraseEncryptionServiceTests.cssrc/Arius.Core.Tests/Encryption/PlaintextPassthroughServiceTests.cssrc/Arius.Core.Tests/FileTree/TreeServiceTests.cssrc/Arius.Core.Tests/List/FileHydrationStatusResolverTests.cssrc/Arius.Core.Tests/LocalFile/LocalFileEnumeratorTests.cssrc/Arius.Core.Tests/Ls/LsHandlerTests.cssrc/Arius.Core.Tests/Restore/PricingConfigTests.cssrc/Arius.Core.Tests/Restore/RestoreCostCalculatorTests.cssrc/Arius.Core.Tests/Snapshot/SnapshotSerializerTests.cssrc/Arius.Core.Tests/Streaming/StreamWrapperTests.cssrc/Arius.Core/Arius.Core.csprojsrc/Arius.Core/Features/Archive/ArchiveModels.cssrc/Arius.Core/Features/Archive/ArchivePipelineHandler.cssrc/Arius.Core/Features/ContainerNames/ContainerNamesQuery.cssrc/Arius.Core/Features/Hydration/HydrationModels.cssrc/Arius.Core/Features/Hydration/ResolveFileHydrationStatusesHandler.cssrc/Arius.Core/Features/List/ListModels.cssrc/Arius.Core/Features/List/ListRepositoryEntriesHandler.cssrc/Arius.Core/Features/Restore/PricingConfig.cssrc/Arius.Core/Features/Restore/RestoreCostCalculator.cssrc/Arius.Core/Features/Restore/RestoreModels.cssrc/Arius.Core/Features/Restore/RestorePipelineHandler.cssrc/Arius.Core/Features/Restore/pricing.jsonsrc/Arius.Core/Ls/LsHandler.cssrc/Arius.Core/Ls/LsModels.cssrc/Arius.Core/ServiceCollectionExtensions.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkIndex/Shard.cssrc/Arius.Core/Shared/ChunkIndex/ShardSerializer.cssrc/Arius.Core/Shared/Encryption/IEncryptionService.cssrc/Arius.Core/Shared/Encryption/PassphraseEncryptionService.cssrc/Arius.Core/Shared/Encryption/PlaintextPassthroughService.cssrc/Arius.Core/Shared/FileTree/TreeBlobSerializer.cssrc/Arius.Core/Shared/FileTree/TreeModels.cssrc/Arius.Core/Shared/FileTree/TreeService.cssrc/Arius.Core/Shared/LocalFile/LocalFileEnumerator.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.Core/Shared/Storage/BlobConstants.cssrc/Arius.Core/Shared/Storage/IBlobContainerService.cssrc/Arius.Core/Shared/Storage/IBlobService.cssrc/Arius.Core/Shared/Storage/IBlobServiceFactory.cssrc/Arius.Core/Shared/Streaming/CountingStream.cssrc/Arius.Core/Shared/Streaming/ProgressStream.cssrc/Arius.E2E.Tests/Arius.E2E.Tests.csprojsrc/Arius.E2E.Tests/AzureFixture.cssrc/Arius.E2E.Tests/E2ETests.cssrc/Arius.E2E.Tests/RehydrationE2ETests.cssrc/Arius.Explorer.Tests/Arius.Explorer.Tests.csprojsrc/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cssrc/Arius.Explorer.Tests/Settings/ApplicationSettingsTests.cssrc/Arius.Explorer/App.xamlsrc/Arius.Explorer/App.xaml.cssrc/Arius.Explorer/Arius.Explorer.csprojsrc/Arius.Explorer/Arius.Explorer.snksrc/Arius.Explorer/AssemblyInfo.cssrc/Arius.Explorer/CLAUDE.mdsrc/Arius.Explorer/ChooseRepository/ChooseRepositoryViewModel.cssrc/Arius.Explorer/ChooseRepository/ChooseRepositoryWindow.xamlsrc/Arius.Explorer/ChooseRepository/ChooseRepositoryWindow.xaml.cssrc/Arius.Explorer/ChooseRepository/PasswordBoxBehavior.cssrc/Arius.Explorer/Infrastructure/RepositorySession.cssrc/Arius.Explorer/Program.cssrc/Arius.Explorer/Properties/PublishProfiles/ClickOnceProfile.pubxmlsrc/Arius.Explorer/RepositoryExplorer/FileItemViewModel.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerWindow.xamlsrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerWindow.xaml.cssrc/Arius.Explorer/RepositoryExplorer/StateCircle.xamlsrc/Arius.Explorer/RepositoryExplorer/StateCircle.xaml.cssrc/Arius.Explorer/RepositoryExplorer/TreeNodeViewModel.cssrc/Arius.Explorer/Settings/ApplicationSettings.cssrc/Arius.Explorer/Settings/RepositoryOptions.cssrc/Arius.Explorer/Shared/Converters/BytesToReadableSizeConverter.cssrc/Arius.Explorer/Shared/Extensions/DataProtectionExtensions.cssrc/Arius.Explorer/Shared/Services/DialogService.cssrc/Arius.Explorer/Shared/Services/IDialogService.cssrc/Arius.Explorer/appsettings.jsonsrc/Arius.Integration.Tests/Arius.Integration.Tests.csprojsrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/FileTree/TreeBuilderIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/CbcEncryptionServiceAdapter.cssrc/Arius.Integration.Tests/Pipeline/ContainerCreationTests.cssrc/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cssrc/Arius.Integration.Tests/Pipeline/GcmIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/LsIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Integration.Tests/Pipeline/RecoveryScriptTests.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreCostModelTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreDispositionTests.cssrc/Arius.Integration.Tests/Pipeline/RestorePointerTimestampTests.cssrc/Arius.Integration.Tests/Pipeline/RoundtripTests.cssrc/Arius.Integration.Tests/Snapshot/SnapshotServiceIntegrationTests.cssrc/Arius.Integration.Tests/Storage/AzuriteFixture.cssrc/Arius.Integration.Tests/Storage/BlobStorageServiceTests.cssrc/Arius.slnxsrc/Directory.Packages.props
💤 Files with no reviewable changes (4)
- src/Arius.Integration.Tests/Arius.Integration.Tests.csproj
- src/Arius.Cli/Arius.Cli.csproj
- src/Arius.Core/Ls/LsModels.cs
- src/Arius.Core/Ls/LsHandler.cs
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net10.0</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| <TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Shouldly" /> | ||
| <PackageReference Include="TUnit" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\Arius.AzureBlob\Arius.AzureBlob.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> |
There was a problem hiding this comment.
New test project is not included in the provided test execution list.
Please add this project to the PR verification commands before merge:
dotnet test --project src/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csproj --list-testsdotnet test --project src/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csproj
As per coding guidelines: **/*.csproj: “Run tests using dotnet test --project <path-to-csproj>” and “Use dotnet test --project <path-to-csproj> --list-tests to list available tests”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csproj` around lines 1 - 19,
The new test project Arius.AzureBlob.Tests (Arius.AzureBlob.Tests.csproj) is not
included in the PR verification steps; update the CI/README or PR verification
checklist to run and list tests for this project by adding the two commands for
the project: run `dotnet test --project
src/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csproj --list-tests` and then
`dotnet test --project src/Arius.AzureBlob.Tests/Arius.AzureBlob.Tests.csproj`;
ensure these commands are added alongside other csproj entries so the test
runner follows the repository guideline for `**/*.csproj`.
| <!-- | ||
| Manifest signing disabled: requires Authenticode certificate for bootstrapper. | ||
| Strong name signing is enabled via Arius.Explorer.snk for assembly identity. | ||
| See: https://docs.microsoft.com/en-us/visualstudio/deployment/how-to-sign-application-and-deployment-manifests | ||
| --> | ||
| <SignManifests>False</SignManifests> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For a ClickOnce application distributed from the web, does assembly strong-name signing provide the same publisher verification and update integrity as enabling SignManifests?
💡 Result:
No, assembly strong-name signing does not provide the same publisher verification and update integrity as enabling SignManifests for a ClickOnce application distributed from the web. Strong-name signing assemblies provides unique identity, prevents tampering with individual assemblies, and ensures version binding integrity, but it does not provide verifiable publisher information or Authenticode-based verification displayed to users during installation. SignManifests (the "Sign the ClickOnce manifests" option in Visual Studio) signs the application and deployment manifests using an Authenticode certificate. This enables: - Publisher verification: Displays the publisher name from the certificate Subject field to users, avoiding "Unknown Publisher". - Update integrity: Ensures updates come from the same signer via signature validation; unsigned manifests risk man-in-the-middle attacks and may block seamless updates. Microsoft documentation explicitly distinguishes these: strong-name signing is for assemblies (unique identity, GAC if needed), while manifest signing is separate for ClickOnce security, certificates, and trust prompts. Update checks use file hashes from signed manifests, but without signatures, there's no cryptographic publisher proof or tamper protection for manifests themselves. Sources confirm strong names lack reliable publisher info and revocation, unlike Authenticode.
Citations:
- 1: https://learn.microsoft.com/en-us/previous-versions/aa730868(v=vs.80)?redirectedfrom=MSDN
- 2: https://learn.microsoft.com/en-us/visualstudio/ide/managing-assembly-and-manifest-signing?view=visualstudio
- 3: https://learn.microsoft.com/en-us/visualstudio/ide/how-to-sign-application-and-deployment-manifests?view=vs-2022
- 4: https://learn.microsoft.com/en-us/visualstudio/deployment/clickonce-security-and-deployment?view=visualstudio
- 5: https://learn.microsoft.com/en-us/visualstudio/deployment/securing-clickonce-applications?view=visualstudio
- 6: https://jacobfilipp.com/MSDN/2006_07/CLRInsideOut/chm.htm
- 7: https://learn.microsoft.com/en-us/visualstudio/deployment/clickonce-application-manifest?view=vs-2022
Enable manifest signing for this ClickOnce profile before using for web deployment.
Line 35 disables manifest signing (SignManifests set to False). While the code comment correctly notes that strong-name signing is enabled via the .snk file, assembly strong-name signing does not provide the same security guarantees as Authenticode manifest signing for ClickOnce applications. Manifest signing is essential for web-delivered installers because it provides:
- Publisher verification: Displays verified publisher identity to users during installation
- Update integrity: Cryptographic signature validation prevents man-in-the-middle attacks and ensures updates come from the same signer
- Manifest tamper protection: Protects the manifests themselves from tampering
Strong-name signing alone cannot provide these cryptographic publisher proofs and tamper protections for the application and deployment manifests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Explorer/Properties/PublishProfiles/ClickOnceProfile.pubxml` around
lines 30 - 35, The ClickOnce profile currently disables manifest signing
(SignManifests is False); change SignManifests to True in the
ClickOnceProfile.pubxml and configure a signing certificate for the ClickOnce
manifests by adding either a ManifestCertificateThumbprint (for a certificate
installed in the machine/user store) or a ManifestKeyFile + ManifestKeyPassword
(pointing to a .pfx) in the same .pubxml so the publish step can
Authenticode-sign deployment and application manifests; ensure the referenced
certificate/thumbprint exists on the build machine or CI agent and that MSBuild
has access to the key for signing.
| foreach (var selectedFile in SelectedFiles) | ||
| { | ||
| var command = new RestoreCommand(new RestoreOptions | ||
| { | ||
| RootDirectory = Repository.LocalDirectoryPath, | ||
| TargetPath = selectedFile.File.RelativePath, | ||
| Overwrite = true, | ||
| NoPointers = false, | ||
| }); | ||
|
|
||
| var result = await repositorySession.Mediator.Send(command); | ||
| if (!result.Success) | ||
| { | ||
| throw new InvalidOperationException(result.ErrorMessage ?? $"Restore failed for {selectedFile.File.RelativePath}."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Snapshot the selection before restoring and aggregate per-file failures.
SelectedFiles is live UI state. If the user changes a checkbox or switches folders while this loop is running, the foreach can throw or skip items. Also, the first failed restore currently aborts the rest of the batch even though earlier files may already have been processed.
🛠️ Safer restore loop
+ var filesToRestore = SelectedFiles.ToList();
+ var failures = new List<string>();
+
- foreach (var selectedFile in SelectedFiles)
+ foreach (var selectedFile in filesToRestore)
{
var command = new RestoreCommand(new RestoreOptions
{
RootDirectory = Repository.LocalDirectoryPath,
TargetPath = selectedFile.File.RelativePath,
@@
var result = await repositorySession.Mediator.Send(command);
if (!result.Success)
{
- throw new InvalidOperationException(result.ErrorMessage ?? $"Restore failed for {selectedFile.File.RelativePath}.");
+ failures.Add(result.ErrorMessage ?? $"Restore failed for {selectedFile.File.RelativePath}.");
}
}
+
+ if (failures.Count > 0)
+ {
+ throw new InvalidOperationException(string.Join(Environment.NewLine, failures));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs` around
lines 404 - 419, Snapshot SelectedFiles into a local list first and iterate that
snapshot (e.g., var filesToRestore = SelectedFiles.ToList()) so UI changes
during restore don't modify the collection; for each item create and send the
RestoreCommand (using RestoreOptions.RootDirectory, TargetPath, Overwrite,
NoPointers) and collect failures into a list or dictionary keyed by
selectedFile.File.RelativePath rather than throwing on the first error; after
the loop, if any failures exist, throw a single InvalidOperationException or
surface a summary message that includes per-file error details from
result.ErrorMessage so completed restores are not aborted by a single failure.
| public string Protect() | ||
| { | ||
| if (string.IsNullOrEmpty(plainText)) | ||
| return plainText; | ||
|
|
||
| try | ||
| { | ||
| var data = Encoding.UTF8.GetBytes(plainText); | ||
| var protectedData = ProtectedData.Protect(data, null, DataProtectionScope.CurrentUser); | ||
| return Convert.ToBase64String(protectedData); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| // If protection fails, return the original value | ||
| // This could happen on non-Windows systems or if DPAPI is not available | ||
| return plainText; | ||
| } | ||
| } | ||
|
|
||
| public string Unprotect() | ||
| { | ||
| if (string.IsNullOrEmpty(plainText)) | ||
| return plainText; | ||
|
|
||
| try | ||
| { | ||
| // First check if it's valid Base64 before attempting to unprotect | ||
| var protectedData = Convert.FromBase64String(plainText); | ||
| var data = ProtectedData.Unprotect(protectedData, null, DataProtectionScope.CurrentUser); | ||
| return Encoding.UTF8.GetString(data); | ||
| } | ||
| catch (FormatException) | ||
| { | ||
| // Not valid Base64, likely plain text from before encryption was added | ||
| return plainText; | ||
| } | ||
| catch (Exception) | ||
| { | ||
| // If unprotection fails for other reasons, return the original value | ||
| // This could happen if the data is corrupted or can't be decrypted | ||
| return plainText; | ||
| } |
There was a problem hiding this comment.
Don't silently downgrade protected secrets to plaintext.
Protect() returns the raw secret on any failure, so AccountKeyProtected / PassphraseProtected can be saved unencrypted. Unprotect() also returns undecryptable Base64 as if it were valid plaintext, which hides corruption/profile-migration problems and can re-save bad credentials. Prefer a tagged format for backward-compatible reads, and fail the save path if protection of new secrets fails.
🔐 Safer pattern
public static class DataProtectionExtensions
{
+ private const string ProtectedPrefix = "dpapi:";
+
extension(string plainText)
{
public string Protect()
{
if (string.IsNullOrEmpty(plainText))
return plainText;
-
- try
- {
- var data = Encoding.UTF8.GetBytes(plainText);
- var protectedData = ProtectedData.Protect(data, null, DataProtectionScope.CurrentUser);
- return Convert.ToBase64String(protectedData);
- }
- catch (Exception)
- {
- // If protection fails, return the original value
- // This could happen on non-Windows systems or if DPAPI is not available
- return plainText;
- }
+ var data = Encoding.UTF8.GetBytes(plainText);
+ var protectedData = ProtectedData.Protect(data, null, DataProtectionScope.CurrentUser);
+ return ProtectedPrefix + Convert.ToBase64String(protectedData);
}
public string Unprotect()
{
if (string.IsNullOrEmpty(plainText))
return plainText;
+
+ if (!plainText.StartsWith(ProtectedPrefix, StringComparison.Ordinal))
+ return plainText; // legacy plaintext
try
{
- // First check if it's valid Base64 before attempting to unprotect
- var protectedData = Convert.FromBase64String(plainText);
+ var protectedData = Convert.FromBase64String(plainText[ProtectedPrefix.Length..]);
var data = ProtectedData.Unprotect(protectedData, null, DataProtectionScope.CurrentUser);
return Encoding.UTF8.GetString(data);
}
- catch (FormatException)
- {
- // Not valid Base64, likely plain text from before encryption was added
- return plainText;
- }
- catch (Exception)
+ catch (CryptographicException ex)
{
- // If unprotection fails for other reasons, return the original value
- // This could happen if the data is corrupted or can't be decrypted
- return plainText;
+ throw new InvalidOperationException("Stored credentials could not be decrypted for this user profile.", ex);
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Explorer/Shared/Extensions/DataProtectionExtensions.cs` around
lines 11 - 52, Protect() currently returns the raw secret on any failure and
Unprotect() silently treats undecryptable Base64 as plaintext; change the
behavior to use a tagged format and fail saves on protection errors: in
DataProtectionExtensions.Protect() wrap the protected payload with a tag (e.g.
"DPAPI:<base64>") and throw (or propagate) the exception instead of returning
plainText when ProtectedData.Protect fails so callers (e.g.
AccountKeyProtected/PassphraseProtected save paths) cannot persist unencrypted
secrets; in DataProtectionExtensions.Unprotect() detect the tag first and if
present attempt Base64+ProtectedData.Unprotect and surface errors (throw) on
failure, otherwise treat untagged values as legacy plaintext for backward
compatibility. Ensure all references to Protect/Unprotect are updated to expect
the tagged value.
Use the shared snapshots path with page-sized blob hierarchy probing in AzureBlobService so repository detection exits after the first matching blob. Only mark Explorer repositories as recent after they load successfully, and fix the coverage wrapper command in CI.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38 +/- ##
==========================================
- Coverage 79.30% 71.47% -7.84%
==========================================
Files 35 59 +24
Lines 3760 4634 +874
Branches 442 620 +178
==========================================
+ Hits 2982 3312 +330
- Misses 681 1191 +510
- Partials 97 131 +34
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:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
.github/workflows/ci.yml (2)
42-42: Consider removing the-lflag frombash -lc.The
-lflag creates a login shell, which sources profile scripts (.bash_profile,.profile) that could unexpectedly alter environment or PATH. Since this is a CI environment with controlled setup, a plainbash -cis sufficient and more predictable.Suggested fix
- bash -lc ' + bash -c '🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 42, The workflow step currently invokes a login shell using "bash -lc '...'" which sources user profile scripts; change it to use a non-login shell by removing the "-l" flag (i.e., use "bash -c '...'" wherever "bash -lc" appears) so the CI run uses the controlled environment and avoids unexpected PATH or environment modifications.
63-83: Explorer tests excluded from coverage reporting.The new
explorer-testsjob doesn't collect coverage, so Explorer test coverage won't appear in Codecov reports. If this is intentional (e.g., due to Windows-specific complexity), this is fine. Otherwise, consider addingdotnet-coverage collecthere and uploading a separate coverage artifact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 63 - 83, The explorer-tests job currently runs tests but does not collect coverage; update the job (explorer-tests) to run coverage collection for the Explorer tests by replacing or augmenting the "🧪 Run Arius.Explorer.Tests" step to invoke dotnet-coverage (e.g., using dotnet-coverage collect) for the project src/Arius.Explorer.Tests/Arius.Explorer.Tests.csproj and then add a step to publish the resulting coverage file as an artifact (or upload to Codecov) so Explorer coverage is included in the overall report.src/Arius.AzureBlob.Tests/AzureBlobServiceTests.cs (1)
225-225: Deduplicate the probe blob name literal in tests.The same magic string is repeated twice; keep it in a single constant to reduce drift and simplify updates.
Proposed change
public class AzureBlobServiceTests { + private const string PreflightProbeBlobName = ".arius-preflight-probe"; + @@ - if (blobName == ".arius-preflight-probe") + if (blobName == PreflightProbeBlobName) { container.UploadedProbe = true; } @@ - if (blobName == ".arius-preflight-probe") + if (blobName == PreflightProbeBlobName) { container.DeletedProbe = true; }Also applies to: 237-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.AzureBlob.Tests/AzureBlobServiceTests.cs` at line 225, Replace the repeated magic string ".arius-preflight-probe" with a single constant defined in the test class (e.g., add a private const string PreflightProbeBlobName = ".arius-preflight-probe"; inside AzureBlobServiceTests) and update all occurrences (including the comparison using blobName and any other uses around lines shown) to use PreflightProbeBlobName so the literal is defined once and reused.src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs (3)
397-406: Multiple enumeration of LINQ queries.
itemsToHydrate,itemsPending, anditemsToRestoreareIEnumerable<T>fromWhere()and are enumerated multiple times (Any(),Count(),Sum()). This re-evaluates the predicate for each call. Materialize with.ToList()for clarity and efficiency.♻️ Materialize once
- var itemsToHydrate = SelectedFiles.Where(item => item.HydrationStatus == FileHydrationStatus.NeedsRehydration); + var itemsToHydrate = SelectedFiles.Where(item => item.HydrationStatus == FileHydrationStatus.NeedsRehydration).ToList(); if (itemsToHydrate.Any()) msg.AppendLine($"This will start hydration on {itemsToHydrate.Count()} item(s) ({itemsToHydrate.Sum(item => item.OriginalLength).Bytes().Humanize()}). This may incur a significant cost."); - var itemsPending = SelectedFiles.Where(item => item.HydrationStatus == FileHydrationStatus.RehydrationPending); + var itemsPending = SelectedFiles.Where(item => item.HydrationStatus == FileHydrationStatus.RehydrationPending).ToList(); if (itemsPending.Any()) msg.AppendLine($"{itemsPending.Count()} item(s) ({itemsPending.Sum(item => item.OriginalLength).Bytes().Humanize()}) are already rehydrating in the cloud."); - var itemsToRestore = SelectedFiles.Where(item => item.HydrationStatus == FileHydrationStatus.Available || item.HydrationStatus == FileHydrationStatus.Unknown); + var itemsToRestore = SelectedFiles.Where(item => item.HydrationStatus == FileHydrationStatus.Available || item.HydrationStatus == FileHydrationStatus.Unknown).ToList(); msg.AppendLine($"This will download {itemsToRestore.Count()} item(s) ({itemsToRestore.Sum(item => item.OriginalLength).Bytes().Humanize()}).");After materializing, you can also use
.Countproperty instead of.Count()extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs` around lines 397 - 406, The LINQ queries for itemsToHydrate, itemsPending, and itemsToRestore are being enumerated multiple times; materialize each with .ToList() (e.g., var itemsToHydrate = SelectedFiles.Where(...).ToList()) and then use the List.Count property instead of Count() to avoid re-evaluating the predicate and improve performance; update the references in the string building (SelectedFiles remains untouched) to use the newly materialized lists (itemsToHydrate.Count, itemsPending.Count, itemsToRestore.Count) while keeping the Sum(...) calls on those lists.
251-258: Consider defensive cancellation at method entry.While
CancelHydrationLoad()is called inLoadNodeContentAsyncbefore this method is fire-and-forget invoked, the method doesn't defensively cancel a prior hydration load itself. If this method is ever called from another site, concurrent loads could run. AddingCancelHydrationLoad()at the start (likeLoadNodeContentAsyncdoes withCancelNodeLoad()) would make the method self-contained.♻️ Optional defensive change
private async Task LoadHydrationStatusesAsync(TreeNodeViewModel node) { if (repositorySession.Mediator is null || node.Items.Count == 0) return; + CancelHydrationLoad(); + var cancellationTokenSource = new CancellationTokenSource(); hydrationLoadCancellation = cancellationTokenSource;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs` around lines 251 - 258, LoadHydrationStatusesAsync should defensively cancel any prior hydration load at method entry; add a call to CancelHydrationLoad() as the first action in LoadHydrationStatusesAsync (similar to how LoadNodeContentAsync calls CancelNodeLoad()), then create the new CancellationTokenSource and assign hydrationLoadCancellation as current; also ensure any early-return checks (e.g., repositorySession.Mediator is null or node.Items.Count == 0) occur after the cancel call so previously running loads are stopped before returning.
362-372: Unreachable code in ternary expression.At line 371, the check
totalCount > 0is inside theelseblock wheretotalCount != 0is already guaranteed. The: ""branch is dead code.♻️ Simplify the condition
else { - - return totalCount > 0 ? $"{totalCount} item(s), {totalSize.Bytes().Humanize()}" : ""; + return $"{totalCount} item(s), {totalSize.Bytes().Humanize()}"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs` around lines 362 - 372, The branch contains a redundant ternary: inside the else of if (selectedCount == 0) you've already established totalCount != 0, so the ternary totalCount > 0 ? $"{totalCount} item(s), {totalSize.Bytes().Humanize()}" : "" contains unreachable code; replace the entire ternary with a direct return of the populated string (e.g. return $"{totalCount} item(s), {totalSize.Bytes().Humanize()}") in the RepositoryExplorerViewModel logic that references selectedCount, totalCount and totalSize.Bytes().Humanize().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.AzureBlob.Tests/AzureBlobServiceTests.cs`:
- Line 32: The test currently asserts order-sensitive equality on the container
names via results.ShouldBe([...]) which can flake if enumeration order changes;
replace this with an order-independent set equality assertion (e.g., use
CollectionAssert.AreEquivalent(new[] { "repo-a", "repo-b" }, results) or, if
using FluentAssertions, results.Should().BeEquivalentTo(new[] { "repo-a",
"repo-b" })) in AzureBlobServiceTests so the test verifies the same members
regardless of order.
In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs`:
- Around line 46-52: The fire-and-forget call to LoadRepositoryAsync() can let
exceptions from repositorySession.ConnectAsync() leak; change the startup logic
(where recentRepositoryManager.GetMostRecent() sets Repository and calls
LoadRepositoryAsync()) to call a small wrapper that awaits LoadRepositoryAsync()
and catches/logs any exceptions (or use a centralized fire-and-forget helper) so
exceptions are observed and logged instead of propagating to the synchronization
context; keep assigning Repository (which triggers OnRepositoryChanged) but
replace `_ = LoadRepositoryAsync()` with a safe awaited wrapper that logs
failures (include references to LoadRepositoryAsync,
repositorySession.ConnectAsync, OnRepositoryChanged,
recentRepositoryManager.GetMostRecent, and Repository).
- Around line 91-102: OpenRepositoryAsync currently awaits LoadRepositoryAsync
but still calls recentRepositoryManager.TouchOrAdd even when LoadRepositoryAsync
swallowed errors from LoadNodeContentAsync; modify LoadRepositoryAsync to return
a success boolean (e.g., Task<bool> LoadRepositoryAsync(...)) or re-throw after
showing the MessageBox in LoadNodeContentAsync, then in OpenRepositoryAsync
check the returned success (or catch) and only call
recentRepositoryManager.TouchOrAdd when LoadRepositoryAsync indicates success;
update references to LoadRepositoryAsync and ensure LoadNodeContentAsync either
returns failure or re-throws so failures prevent TouchOrAdd from running.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 42: The workflow step currently invokes a login shell using "bash -lc
'...'" which sources user profile scripts; change it to use a non-login shell by
removing the "-l" flag (i.e., use "bash -c '...'" wherever "bash -lc" appears)
so the CI run uses the controlled environment and avoids unexpected PATH or
environment modifications.
- Around line 63-83: The explorer-tests job currently runs tests but does not
collect coverage; update the job (explorer-tests) to run coverage collection for
the Explorer tests by replacing or augmenting the "🧪 Run Arius.Explorer.Tests"
step to invoke dotnet-coverage (e.g., using dotnet-coverage collect) for the
project src/Arius.Explorer.Tests/Arius.Explorer.Tests.csproj and then add a step
to publish the resulting coverage file as an artifact (or upload to Codecov) so
Explorer coverage is included in the overall report.
In `@src/Arius.AzureBlob.Tests/AzureBlobServiceTests.cs`:
- Line 225: Replace the repeated magic string ".arius-preflight-probe" with a
single constant defined in the test class (e.g., add a private const string
PreflightProbeBlobName = ".arius-preflight-probe"; inside AzureBlobServiceTests)
and update all occurrences (including the comparison using blobName and any
other uses around lines shown) to use PreflightProbeBlobName so the literal is
defined once and reused.
In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs`:
- Around line 397-406: The LINQ queries for itemsToHydrate, itemsPending, and
itemsToRestore are being enumerated multiple times; materialize each with
.ToList() (e.g., var itemsToHydrate = SelectedFiles.Where(...).ToList()) and
then use the List.Count property instead of Count() to avoid re-evaluating the
predicate and improve performance; update the references in the string building
(SelectedFiles remains untouched) to use the newly materialized lists
(itemsToHydrate.Count, itemsPending.Count, itemsToRestore.Count) while keeping
the Sum(...) calls on those lists.
- Around line 251-258: LoadHydrationStatusesAsync should defensively cancel any
prior hydration load at method entry; add a call to CancelHydrationLoad() as the
first action in LoadHydrationStatusesAsync (similar to how LoadNodeContentAsync
calls CancelNodeLoad()), then create the new CancellationTokenSource and assign
hydrationLoadCancellation as current; also ensure any early-return checks (e.g.,
repositorySession.Mediator is null or node.Items.Count == 0) occur after the
cancel call so previously running loads are stopped before returning.
- Around line 362-372: The branch contains a redundant ternary: inside the else
of if (selectedCount == 0) you've already established totalCount != 0, so the
ternary totalCount > 0 ? $"{totalCount} item(s), {totalSize.Bytes().Humanize()}"
: "" contains unreachable code; replace the entire ternary with a direct return
of the populated string (e.g. return $"{totalCount} item(s),
{totalSize.Bytes().Humanize()}") in the RepositoryExplorerViewModel logic that
references selectedCount, totalCount and totalSize.Bytes().Humanize().
🪄 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: bb090d63-55db-492a-919f-00a1098860ad
📒 Files selected for processing (4)
.github/workflows/ci.ymlsrc/Arius.AzureBlob.Tests/AzureBlobServiceTests.cssrc/Arius.AzureBlob/AzureBlobService.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arius.AzureBlob/AzureBlobService.cs
| results.Add(name); | ||
| } | ||
|
|
||
| results.ShouldBe(["repo-a", "repo-b"]); |
There was a problem hiding this comment.
Avoid order-coupled assertion for container names.
This check can fail if container enumeration order changes even when behavior is correct. Assert set equality instead of sequence order.
Proposed change
- results.ShouldBe(["repo-a", "repo-b"]);
+ results.OrderBy(name => name).ShouldBe(["repo-a", "repo-b"]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| results.ShouldBe(["repo-a", "repo-b"]); | |
| results.OrderBy(name => name).ShouldBe(["repo-a", "repo-b"]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.AzureBlob.Tests/AzureBlobServiceTests.cs` at line 32, The test
currently asserts order-sensitive equality on the container names via
results.ShouldBe([...]) which can flake if enumeration order changes; replace
this with an order-independent set equality assertion (e.g., use
CollectionAssert.AreEquivalent(new[] { "repo-a", "repo-b" }, results) or, if
using FluentAssertions, results.Should().BeEquivalentTo(new[] { "repo-a",
"repo-b" })) in AzureBlobServiceTests so the test verifies the same members
regardless of order.
| // Check for most recent repository and auto-open if exists | ||
| var mostRecent = recentRepositoryManager.GetMostRecent(); | ||
| if (mostRecent != null) | ||
| { | ||
| Repository = mostRecent; // this will trigger OnRepositoryChanged for UI updates | ||
| _ = LoadRepositoryAsync(); // fire-and-forget for initial load | ||
| } |
There was a problem hiding this comment.
Fire-and-forget may lose exceptions from repository connection.
LoadRepositoryAsync() calls repositorySession.ConnectAsync() which can throw (e.g., invalid credentials, network failure). With _ = LoadRepositoryAsync(), exceptions propagate to the synchronization context and may crash the app or be silently lost. Consider wrapping with try-catch or using a helper that logs unhandled exceptions.
🛠️ Suggested approach
if (mostRecent != null)
{
Repository = mostRecent;
- _ = LoadRepositoryAsync();
+ _ = LoadRepositoryAsync().ContinueWith(t =>
+ {
+ if (t.IsFaulted && t.Exception != null)
+ logger.LogError(t.Exception, "Failed to auto-open most recent repository");
+ }, TaskScheduler.Default);
}Alternatively, extract a helper method that logs and handles exceptions from fire-and-forget tasks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs` around
lines 46 - 52, The fire-and-forget call to LoadRepositoryAsync() can let
exceptions from repositorySession.ConnectAsync() leak; change the startup logic
(where recentRepositoryManager.GetMostRecent() sets Repository and calls
LoadRepositoryAsync()) to call a small wrapper that awaits LoadRepositoryAsync()
and catches/logs any exceptions (or use a centralized fire-and-forget helper) so
exceptions are observed and logged instead of propagating to the synchronization
context; keep assigning Repository (which triggers OnRepositoryChanged) but
replace `_ = LoadRepositoryAsync()` with a safe awaited wrapper that logs
failures (include references to LoadRepositoryAsync,
repositorySession.ConnectAsync, OnRepositoryChanged,
recentRepositoryManager.GetMostRecent, and Repository).
| [RelayCommand] | ||
| private async Task OpenRepositoryAsync(RepositoryOptions repository, CancellationToken cancellationToken = default) | ||
| { | ||
| Repository = repository; | ||
|
|
||
| // Load repository data asynchronously | ||
| if (repository != null) | ||
| { | ||
| await LoadRepositoryAsync(); | ||
| recentRepositoryManager.TouchOrAdd(repository); | ||
| } | ||
| } |
There was a problem hiding this comment.
TouchOrAdd still runs after failed repository loads.
The previous review requested moving TouchOrAdd to after successful load. While LoadRepositoryAsync is now awaited before TouchOrAdd, the error from LoadNodeContentAsync (lines 240-248) is caught and displayed via MessageBox without propagating. This means TouchOrAdd executes even when the repository fails to load, perpetuating the "sticky startup failure" scenario.
Consider returning a success indicator from LoadRepositoryAsync or re-throwing after showing the message box.
🛠️ Suggested approach
- private async Task LoadRepositoryAsync()
+ private async Task<bool> LoadRepositoryAsync()
{
if (Repository == null)
{
// ... reset state ...
- return;
+ return false;
}
// ... rest of method ...
+ return true; // after successful tree load
}Then in OpenRepositoryAsync:
if (repository != null)
{
- await LoadRepositoryAsync();
- recentRepositoryManager.TouchOrAdd(repository);
+ if (await LoadRepositoryAsync())
+ recentRepositoryManager.TouchOrAdd(repository);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs` around
lines 91 - 102, OpenRepositoryAsync currently awaits LoadRepositoryAsync but
still calls recentRepositoryManager.TouchOrAdd even when LoadRepositoryAsync
swallowed errors from LoadNodeContentAsync; modify LoadRepositoryAsync to return
a success boolean (e.g., Task<bool> LoadRepositoryAsync(...)) or re-throw after
showing the MessageBox in LoadNodeContentAsync, then in OpenRepositoryAsync
check the returned success (or catch) and only call
recentRepositoryManager.TouchOrAdd when LoadRepositoryAsync indicates success;
update references to LoadRepositoryAsync and ensure LoadNodeContentAsync either
returns failure or re-throws so failures prevent TouchOrAdd from running.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/scripts/Get-DotNetProjectMatrix.ps1 (3)
13-32: PSScriptAnalyzer warns about plural noun, but semantically appropriate here.The function returns a collection of TFMs, so the plural noun
Get-ProjectTfmsis reasonable. If you want to suppress the warning or follow strict PowerShell conventions, consider renaming toGet-ProjectTfmListor adding a[Diagnostics.CodeAnalysis.SuppressMessageAttribute].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/Get-DotNetProjectMatrix.ps1 around lines 13 - 32, The PSScriptAnalyzer warning flags the plural function name Get-ProjectTfms; to resolve either rename the function to a non-plural/explicit name like Get-ProjectTfmList (update all call sites) or suppress the analyzer warning by adding an appropriate suppression attribute (e.g., Diagnostics.CodeAnalysis.SuppressMessageAttribute) on the Get-ProjectTfms function; choose one approach, update the function declaration (and any references) if renaming, or add the suppression attribute directly above the Get-ProjectTfms definition to keep the semantic plural name.
64-66: Case-sensitive comparison may miss test projects with non-lowercasetrue.PowerShell's
-eqis case-insensitive for strings by default, but MSBuild boolean values can be authored astrue,True, orTRUE. The current comparison works but relies on this behavior. Consider using-ieqfor explicit case-insensitive matching or normalizing with.ToLowerInvariant()for clarity.🔧 Suggested improvement for explicit case-insensitivity
$isTestProject = ($projectXml.Project.PropertyGroup | Where-Object { - [string]$_.TestingPlatformDotnetTestSupport -eq 'true' + [string]$_.TestingPlatformDotnetTestSupport -ieq 'true' }).Count -gt 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/Get-DotNetProjectMatrix.ps1 around lines 64 - 66, The boolean check for TestingPlatformDotnetTestSupport uses a string comparison that should explicitly handle case-insensitivity; update the expression that sets $isTestProject (the block referencing $projectXml.Project.PropertyGroup and TestingPlatformDotnetTestSupport) to compare case-insensitively — either use the -ieq operator instead of -eq or normalize the value (e.g., .ToLowerInvariant()) before comparing to 'true' — so projects authored with "True"/"TRUE" are correctly detected.
52-54: Consider using named parameters forJoin-Path.Static analysis suggests avoiding positional parameters for clarity. This is a minor style improvement.
🔧 Suggested improvement
-$workspaceRoot = Resolve-Path (Join-Path $PSScriptRoot '..' '..') +$workspaceRoot = Resolve-Path (Join-Path -Path $PSScriptRoot -ChildPath '..' -AdditionalChildPath '..')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/Get-DotNetProjectMatrix.ps1 around lines 52 - 54, Replace positional Join-Path and Resolve-Path calls with named parameters for clarity: when constructing $workspaceRoot use Resolve-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath '..\..'), and when constructing $srcRoot use Join-Path -Path $workspaceRoot -ChildPath 'src'; keep the same variables ($workspaceRoot, $srcRoot, $PSScriptRoot) and logical behavior but switch to -Path and -ChildPath named parameters (and optionally use Resolve-Path -Path) to avoid positional args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/scripts/Get-DotNetProjectMatrix.ps1:
- Around line 13-32: The PSScriptAnalyzer warning flags the plural function name
Get-ProjectTfms; to resolve either rename the function to a non-plural/explicit
name like Get-ProjectTfmList (update all call sites) or suppress the analyzer
warning by adding an appropriate suppression attribute (e.g.,
Diagnostics.CodeAnalysis.SuppressMessageAttribute) on the Get-ProjectTfms
function; choose one approach, update the function declaration (and any
references) if renaming, or add the suppression attribute directly above the
Get-ProjectTfms definition to keep the semantic plural name.
- Around line 64-66: The boolean check for TestingPlatformDotnetTestSupport uses
a string comparison that should explicitly handle case-insensitivity; update the
expression that sets $isTestProject (the block referencing
$projectXml.Project.PropertyGroup and TestingPlatformDotnetTestSupport) to
compare case-insensitively — either use the -ieq operator instead of -eq or
normalize the value (e.g., .ToLowerInvariant()) before comparing to 'true' — so
projects authored with "True"/"TRUE" are correctly detected.
- Around line 52-54: Replace positional Join-Path and Resolve-Path calls with
named parameters for clarity: when constructing $workspaceRoot use Resolve-Path
-Path (Join-Path -Path $PSScriptRoot -ChildPath '..\..'), and when constructing
$srcRoot use Join-Path -Path $workspaceRoot -ChildPath 'src'; keep the same
variables ($workspaceRoot, $srcRoot, $PSScriptRoot) and logical behavior but
switch to -Path and -ChildPath named parameters (and optionally use Resolve-Path
-Path) to avoid positional args.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06b1e8d3-30d2-4f64-8004-adf6d46f3b3b
📒 Files selected for processing (3)
.github/scripts/Get-DotNetProjectMatrix.ps1.github/workflows/ci.yml.github/workflows/codeql.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
Summary
ContainerNamesQuery, and add an architecture test enforcing that boundarywire-explorer-to-coreOpenSpec change after updating the mainls-commandandcontainer-namesspecsTesting
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.csprojdotnet test --project src/Arius.Explorer.Tests/Arius.Explorer.Tests.csproj(environment-blocked on macOS: missingMicrosoft.WindowsDesktop.Appfornet10.0-windows)Summary by CodeRabbit
New Features
Improvements
Documentation
Tests