refactor: split CliBuilder into vertical slices under Commands/#34
Merged
Conversation
Move each verb and its progress handlers into dedicated files under
Commands/{Archive,Restore,Ls,Update}/ with sub-namespaces matching the
folder structure. CliBuilder.cs is slimmed down to option factories,
root-command assembly, resolution helpers, DI, and audit logging only.
ProgressHandlers.cs is deleted; update ProgressTests.cs references.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors CLI by extracting archive, restore, ls, and update command builders into dedicated verb classes; splits monolithic progress handlers into Archive- and Restore-specific handlers; adds DisplayHelpers; updates tests to use new BuildDisplay/DisplayHelpers APIs and removes the old consolidated ProgressHandlers file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (Terminal)
participant CLI as CLI (Verb)
participant DI as ServiceProvider (DI)
participant Mediator as IMediator
participant State as ProgressState
participant Console as AnsiConsole / Recorder
User->>CLI: run "archive"/"restore"/"ls"/"update"
CLI->>DI: serviceProviderFactory(...)
CLI->>Mediator: Send ArchiveCommand / RestoreCommand / LsCommand / Update logic
Mediator->>State: publish progress notifications (scan, hash, tar, upload, download, restore)
State-->>CLI: progress state mutated
CLI->>Console: BuildDisplay(ProgressState) / refresh live UI
Console-->>User: live progress UI and prompts
User->>Console: interactive confirmations (restore)
Console->>CLI: signal confirmations
Mediator->>CLI: command completion result
CLI->>User: final status output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #34 +/- ##
==========================================
- Coverage 83.31% 83.15% -0.17%
==========================================
Files 27 33 +6
Lines 3369 3342 -27
Branches 402 401 -1
==========================================
- Hits 2807 2779 -28
- Misses 471 472 +1
Partials 91 91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/Arius.Cli/Commands/Restore/RestoreVerb.cs (1)
142-145: Consider usingasync/awaitinstead ofContinueWithfor clearer exception handling.The
ContinueWithpattern works here since exceptions will propagate whenpipelineTaskis awaited later. However, the pattern silently swallows theresultassignment on failure (t.IsCompletedSuccessfully ? t.Result : null). Consider restructuring for clarity:Alternative approach
RestoreResult? result = null; Exception? pipelineException = null; var pipelineTask = Task.Run(async () => { try { result = await mediator.Send(new RestoreCommand(opts), ct); } catch (Exception ex) { pipelineException = ex; throw; } });This is a minor readability suggestion; the current code functions correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Restore/RestoreVerb.cs` around lines 142 - 145, The ContinueWith usage on mediator.Send(new RestoreCommand(opts), ct) hides result assignment on failure and makes exception flow less clear; replace the ContinueWith with an async/await wrapper: create RestoreResult? result = null and start pipelineTask as an async Task that awaits mediator.Send(...), assigns result on success, catches and stores/rethrows exceptions (or simply rethrows after storing if you need the exception later), then await pipelineTask where it’s consumed. Update references to pipelineTask, result and mediator.Send(new RestoreCommand(opts), ct) so exception propagation and result assignment are explicit and clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs`:
- Around line 86-94: The current TOCTOU uses state.TrackedTars.Count and
Keys.Max() which can throw if another handler (TarBundleUploadedHandler) removes
the last entry; instead locate the active tar by selecting
state.TrackedTars.Values where TarState == TarState.Accumulating, order by
BundleNumber descending and take the first (null-checking) before computing
addedBytes and calling tar.AddEntry; mirror the safe selection pattern used in
TarBundleSealingHandler and keep updating notification.CurrentTarSize -
tar.AccumulatedBytes only when a tar was found.
In `@src/Arius.Cli/Commands/Archive/ArchiveVerb.cs`:
- Around line 232-243: The Uploading header logic incorrectly relies only on
chunksUploaded/tarsUploaded/queueDepth and misses in-flight
TrackedFile/TrackedTar uploads and the SnapshotComplete flag; update the
uploadActive boolean in ArchiveVerb.cs (the expression using chunksUploaded,
tarsUploaded, queueDepth, state.SnapshotComplete, state.ScanComplete) to also
consider any TrackedFile or TrackedTar that is currently uploading (e.g., any
trackedFiles.Any(f => f.IsUploading) || trackedTars.Any(t => t.IsUploading)) and
treat SnapshotComplete appropriately so the header is active while uploads are
in flight and turns green when state.SnapshotComplete indicates success.
- Around line 150-153: The continuation on the mediator.Send( new
ArchiveCommand(opts), ct ) call masks exceptions by using ContinueWith and
setting result = null on failures; change ArchiveVerb to await the original Task
returned by mediator.Send (or await AsTask()) instead of using ContinueWith so
exceptions propagate normally, and if you need to set result capture the awaited
return value into result or use a try/catch around the await to log/handle and
rethrow as appropriate; reference mediator.Send, ArchiveCommand, result and the
ArchiveVerb method to locate where to replace ContinueWith with an awaited call
and proper exception propagation.
In `@src/Arius.Cli/Commands/Restore/RestoreVerb.cs`:
- Around line 292-296: The null/empty error output when result is null should be
replaced with a meaningful message: update the block that checks `if (result is
null || !result.Success)` to determine the error text by first checking
`pipelineTask.IsFaulted` and using
`pipelineTask.Exception?.GetBaseException().Message`, then falling back to
`result?.ErrorMessage` and finally a default string like "Unknown error
occurred"; use that computed message in the `AnsiConsole.MarkupLine` call so
users never see an empty error message.
- Around line 98-108: The ConfirmRehydration and ConfirmCleanup callbacks ignore
their cancellationToken and can hang waiting on answerTcs/task completion;
update each callback (ConfirmRehydration and ConfirmCleanup) to register the
provided cancellationToken so that when cancelled it will unblock the waiting
TaskCompletionSource (questionTcs and cleanupQuestionTcs) and complete/ cancel
the corresponding answer Task (answerTcs and cleanupAnswerTcs) or return a safe
cancelled/false result; ensure you dispose the registration to avoid leaks and
only set the TCS once to avoid race conditions.
In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs`:
- Around line 61-65: The code reads RuntimeInformation.RuntimeIdentifier into
rid and only checks OS substrings, causing mismatches; update the logic in the
UpdateVerb handling (the rid and assetName assignment) to parse the full RID
into OS and architecture components (e.g., split on '-' to get osPart and
archPart), map explicit supported combinations (like "win-x64" ->
"arius-win-x64", "osx-arm64" -> "arius-osx-arm64", "linux-x64" ->
"arius-linux-x64", etc.), and if the RID is not one of the supported combos
return or throw a clear error/exit instead of falling back to a default binary;
ensure assetName is set only for validated mappings so incompatible RIDs are
rejected early.
- Around line 90-125: Download the update into a staging file (use the existing
tempFile and the download logic in the StartAsync block), then verify
integrity/signature of that staging file (validate checksum or digital signature
against a trusted digest) before attempting to replace the running executable;
do not call File.Move(currentExe) while the process is running—instead, spawn or
write an updater helper (or re-exec self) that waits for the current process to
exit and then atomically replaces currentExe with the verified staging file (on
Windows use a separate updater process because OperatingSystem.IsWindows()
prevents in-place replacement, and on Unix you may apply File.SetUnixFileMode to
the staging file before final swap to preserve executable bits). Ensure you
surface verification failures and only proceed to replacement after successful
verification.
- Around line 33-45: The code in UpdateVerb.cs currently slices the raw GitHub
JSON string with IndexOf which is brittle; instead parse the response using
System.Text.Json (e.g., JsonDocument or JsonSerializer) to read the "tag_name"
property safely (use root.GetProperty("tag_name").GetString() with null checks)
and replace the tagStart/tagEnd/tag extraction logic in the method that calls
http.GetStringAsync; do the same fix for the later block (lines ~67-84) where
asset URLs are selected—parse the "assets" array and find the desired asset by
its "name" or "browser_download_url" property rather than string scanning, and
log/return a clear error if properties are missing.
---
Nitpick comments:
In `@src/Arius.Cli/Commands/Restore/RestoreVerb.cs`:
- Around line 142-145: The ContinueWith usage on mediator.Send(new
RestoreCommand(opts), ct) hides result assignment on failure and makes exception
flow less clear; replace the ContinueWith with an async/await wrapper: create
RestoreResult? result = null and start pipelineTask as an async Task that awaits
mediator.Send(...), assigns result on success, catches and stores/rethrows
exceptions (or simply rethrows after storing if you need the exception later),
then await pipelineTask where it’s consumed. Update references to pipelineTask,
result and mediator.Send(new RestoreCommand(opts), ct) so exception propagation
and result assignment are explicit and clearer.
🪄 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: 76bbeb88-047d-463e-9de2-75921f3751f7
📒 Files selected for processing (13)
src/Arius.Cli.Tests/ProgressTests.cssrc/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/Commands/Update/UpdateVerb.cssrc/Arius.Cli/DisplayHelpers.cssrc/Arius.Cli/ProgressHandlers.cssrc/Arius.Core.Tests/Restore/PricingConfigTests.cssrc/Arius.E2E.Tests/RehydrationE2ETests.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cs
💤 Files with no reviewable changes (4)
- src/Arius.Core.Tests/Restore/PricingConfigTests.cs
- src/Arius.E2E.Tests/RehydrationE2ETests.cs
- src/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cs
- src/Arius.Cli/ProgressHandlers.cs
Comment on lines
+232
to
+243
| var uploadActive = chunksUploaded > 0 || tarsUploaded > 0 || queueDepth > 0 || !state.SnapshotComplete && state.ScanComplete; | ||
| if (uploadActive) | ||
| { | ||
| var queuePart = queueDepth > 0 ? $" [dim][[{queueDepth} pending]][/]" : string.Empty; | ||
| var uploadDone = state.SnapshotComplete; | ||
| var uploadSymbol = uploadDone ? "[green]●[/]" : "[yellow]○[/]"; | ||
| lines.Add(new Markup($" {uploadSymbol} Uploading [dim]{chunksUploaded:N0} unique chunks[/]{queuePart}")); | ||
| } | ||
| else | ||
| { | ||
| lines.Add(new Markup(" [grey] Uploading[/]")); | ||
| } |
Contributor
There was a problem hiding this comment.
Derive the Uploading stage from live upload state as well.
This header stays grey when the first upload is already in flight but chunksUploaded == 0 and queueDepth == 0, and it also never turns green for fully deduped/empty successful runs. Include active TrackedFile/TrackedTar upload states and SnapshotComplete in this condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Cli/Commands/Archive/ArchiveVerb.cs` around lines 232 - 243, The
Uploading header logic incorrectly relies only on
chunksUploaded/tarsUploaded/queueDepth and misses in-flight
TrackedFile/TrackedTar uploads and the SnapshotComplete flag; update the
uploadActive boolean in ArchiveVerb.cs (the expression using chunksUploaded,
tarsUploaded, queueDepth, state.SnapshotComplete, state.ScanComplete) to also
consider any TrackedFile or TrackedTar that is currently uploading (e.g., any
trackedFiles.Any(f => f.IsUploading) || trackedTars.Any(t => t.IsUploading)) and
treat SnapshotComplete appropriately so the header is active while uploads are
in flight and turns green when state.SnapshotComplete indicates success.
Comment on lines
+292
to
+296
| if (result is null || !result.Success) | ||
| { | ||
| AnsiConsole.MarkupLine($"[red]Restore failed:[/] {result?.ErrorMessage}"); | ||
| return 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Error message could be empty when result is null.
When the pipeline task faults, result is set to null via the ContinueWith callback, but the exception propagates when awaiting pipelineTask. However, if result is null without an exception (edge case), the error message {result?.ErrorMessage} would display empty, which is unhelpful for users.
Proposed fix
if (result is null || !result.Success)
{
- AnsiConsole.MarkupLine($"[red]Restore failed:[/] {result?.ErrorMessage}");
+ var errorMsg = result?.ErrorMessage ?? "Unknown error occurred";
+ AnsiConsole.MarkupLine($"[red]Restore failed:[/] {errorMsg}");
return 1;
}📝 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.
Suggested change
| if (result is null || !result.Success) | |
| { | |
| AnsiConsole.MarkupLine($"[red]Restore failed:[/] {result?.ErrorMessage}"); | |
| return 1; | |
| } | |
| if (result is null || !result.Success) | |
| { | |
| var errorMsg = result?.ErrorMessage ?? "Unknown error occurred"; | |
| AnsiConsole.MarkupLine($"[red]Restore failed:[/] {errorMsg}"); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Cli/Commands/Restore/RestoreVerb.cs` around lines 292 - 296, The
null/empty error output when result is null should be replaced with a meaningful
message: update the block that checks `if (result is null || !result.Success)`
to determine the error text by first checking `pipelineTask.IsFaulted` and using
`pipelineTask.Exception?.GetBaseException().Message`, then falling back to
`result?.ErrorMessage` and finally a default string like "Unknown error
occurred"; use that computed message in the `AnsiConsole.MarkupLine` call so
users never see an empty error message.
Comment on lines
+90
to
+125
| await AnsiConsole.Progress() | ||
| .AutoClear(false) | ||
| .Columns( | ||
| new TaskDescriptionColumn(), | ||
| new ProgressBarColumn(), | ||
| new PercentageColumn(), | ||
| new SpinnerColumn()) | ||
| .StartAsync(async ctx => | ||
| { | ||
| var task = ctx.AddTask("[green]Downloading update[/]"); | ||
| using var response = await http.GetAsync(downloadUrl, HttpCompletionOption.ResponseHeadersRead, ct); | ||
| response.EnsureSuccessStatusCode(); | ||
| var totalBytes = response.Content.Headers.ContentLength ?? -1; | ||
| if (totalBytes > 0) task.MaxValue = totalBytes; | ||
|
|
||
| await using var stream = await response.Content.ReadAsStreamAsync(ct); | ||
| await using var file = File.Create(tempFile); | ||
| var buffer = new byte[81920]; | ||
| long downloaded = 0; | ||
| int bytesRead; | ||
| while ((bytesRead = await stream.ReadAsync(buffer, ct)) > 0) | ||
| { | ||
| await file.WriteAsync(buffer.AsMemory(0, bytesRead), ct); | ||
| downloaded += bytesRead; | ||
| if (totalBytes > 0) task.Value = downloaded; | ||
| } | ||
| task.Value = task.MaxValue; | ||
| }); | ||
|
|
||
| var currentExe = Environment.ProcessPath!; | ||
| File.Move(tempFile, currentExe, true); | ||
|
|
||
| if (!OperatingSystem.IsWindows()) | ||
| { | ||
| File.SetUnixFileMode(currentExe, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | UnixFileMode.GroupRead | UnixFileMode.GroupExecute | UnixFileMode.OtherRead | UnixFileMode.OtherExecute); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can a running .NET application on Windows replace its own executable with File.Move(..., overwrite: true)? What self-update handoff pattern is typically used instead?
💡 Result:
No, a running .NET application on Windows cannot replace its own executable using File.Move(..., overwrite: true). Windows locks the executable file while the process is running, preventing deletion, overwriting, or moving (if it involves replacing the locked file). File.Move fails with an IOException like "The process cannot access the file because it is being used by another process." The typical self-update handoff pattern is: 1. The main app downloads the new version (e.g., to a temp file). 2. Launches a separate small "updater" executable (passed arguments like process ID, paths). 3. Exits itself. 4. Updater waits for main process to terminate (using Process.WaitForExit or timeout). 5. Updater replaces files (rename old exe if possible, copy new one, clean up). 6. Launches new main app. 7. Updater exits. This is confirmed across StackOverflow, SuperUser, Visual Studio Magazine, and .NET docs. Libraries like AutoUpdater.NET implement this.
Citations:
- 1: https://stackoverflow.com/questions/10552202/how-to-overwrite-my-currently-running-executable
- 2: https://superuser.com/questions/651214/on-windows-why-cant-you-overwrite-the-executable-of-a-running-application
- 3: https://stackoverflow.com/questions/6695496/rename-a-running-executable-exe-file
- 4: https://stackoverflow.com/questions/3781683/can-a-winforms-app-unlock-replace-its-own-exe-file-for-auto-update
- 5: https://stackoverflow.com/questions/2335755/how-to-update-an-assembly-for-a-running-c-process-aka-hot-deploy
- 6: https://stackoverflow.com/questions/459734/is-there-a-way-to-make-a-self-updatable-executable-in-windows
- 7: https://stackoverflow.com/questions/1211948/best-method-for-implementing-self-updating-software
- 8: https://stackoverflow.com/questions/8879405/process-cannot-access-the-file-because-it-is-being-used-by-another-process-on-di
- 9: https://learn.microsoft.com/en-us/dotnet/api/system.io.file.move?view=net-9.0
- 10: https://stackoverflow.com/questions/44995095/file-exists-file-move-erroring-the-process-cannot-access-the-file-because-it
- 11: https://github.com/ravibpatel/AutoUpdater.NET
- 12: https://stackoverflow.com/questions/1609957/how-can-a-net-program-update-itself
🏁 Script executed:
cat -n src/Arius.Cli/Commands/Update/UpdateVerb.csRepository: woutervanranst/Arius7
Length of output: 7408
Stage and verify the replacement binary before swapping it in.
This implementation has two critical issues: (1) On Windows, File.Move will fail because the executable is locked while the process runs—the move happens before the current process exits, making this impossible without a separate updater. (2) There is no checksum, signature, or integrity verification of the downloaded binary from GitHub; the code trusts whatever asset is returned. Download to a staging location, verify the artifact against a trusted digest or signature, then use a separate updater process that waits for the current process to exit before replacing the executable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs` around lines 90 - 125, Download
the update into a staging file (use the existing tempFile and the download logic
in the StartAsync block), then verify integrity/signature of that staging file
(validate checksum or digital signature against a trusted digest) before
attempting to replace the running executable; do not call File.Move(currentExe)
while the process is running—instead, spawn or write an updater helper (or
re-exec self) that waits for the current process to exit and then atomically
replaces currentExe with the verified staging file (on Windows use a separate
updater process because OperatingSystem.IsWindows() prevents in-place
replacement, and on Unix you may apply File.SetUnixFileMode to the staging file
before final swap to preserve executable bits). Ensure you surface verification
failures and only proceed to replacement after successful verification.
…e exceptions in ArchiveVerb
…nd ConfirmCleanup to unblock on cancellation
… RID lookup table in UpdateVerb
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/Arius.Cli/Commands/Update/UpdateVerb.cs (1)
139-145:⚠️ Potential issue | 🔴 CriticalSelf-update will fail on Windows; running executable is locked.
On Windows, the OS locks the executable file while the process runs.
File.Move(tempFile, currentExe, true)will throw anIOExceptionbecause the current process holds a lock oncurrentExe. This issue was previously identified and remains unaddressed.The standard pattern is to:
- Download to a staging location (already done)
- Spawn a separate updater process or script
- Exit the current process
- Have the updater wait for termination, then replace the executable
- Launch the new version
Example approach for Windows
if (OperatingSystem.IsWindows()) { // Write a small batch script that waits and replaces var script = Path.Combine(tempDir, "update.cmd"); File.WriteAllText(script, $""" `@echo` off timeout /t 2 /nobreak >nul move /y "{tempFile}" "{currentExe}" start "" "{currentExe}" del "%~f0" """); Process.Start(new ProcessStartInfo(script) { UseShellExecute = true, CreateNoWindow = true }); Environment.Exit(0); } else { File.Move(tempFile, currentExe, true); // Unix allows replacing running executables }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs` around lines 139 - 145, Self-update on Windows fails because File.Move(tempFile, currentExe, true) attempts to overwrite the running executable (currentExe) which is locked; update the Windows branch inside UpdateVerb (around the code using currentExe and tempFile) to instead create and launch a short updater (e.g., a .cmd or small helper exe) that waits for the current process to exit, moves tempFile to currentExe, restarts the new executable, and cleans up the updater, then call Environment.Exit(0) from the main process; keep the existing File.Move/UnixFileMode path for non-Windows (OperatingSystem.IsWindows() check) so Unix behavior is unchanged.src/Arius.Cli/Commands/Restore/RestoreVerb.cs (1)
296-300:⚠️ Potential issue | 🟡 MinorError message may be empty when
resultis null.When the pipeline task faults,
resultstaysnulland{result?.ErrorMessage}produces an empty string. Provide a fallback message for better user feedback.Proposed fix
if (result is null || !result.Success) { - AnsiConsole.MarkupLine($"[red]Restore failed:[/] {result?.ErrorMessage}"); + var errorMsg = result?.ErrorMessage ?? "An unexpected error occurred"; + AnsiConsole.MarkupLine($"[red]Restore failed:[/] {errorMsg}"); return 1; }Note: The root cause is the
ContinueWithpattern swallowing exceptions (lines 146-149). Fixing that would allow surfacing the actual exception message here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Restore/RestoreVerb.cs` around lines 296 - 300, The failure message can be empty when the pipeline task returns null; update the RestoreVerb error branch to provide a fallback error string by using the local result variable (result) and its ErrorMessage if present, otherwise a clear default like "Unknown error occurred" or the task/exception message; specifically modify the conditional in RestoreVerb where result is checked and the AnsiConsole.MarkupLine is called so it prints result?.ErrorMessage ?? "Unknown error occurred" (or a similarly descriptive default) to ensure users see a meaningful message when result is null.
🧹 Nitpick comments (3)
src/Arius.Cli/Commands/Archive/ArchiveVerb.cs (1)
167-171: Consider logging or exposing more detail on null result.When
result is null, the error message displays{result?.ErrorMessage}which will be empty. This could occur if the task completes unexpectedly. A more informative fallback message would help debugging.Suggested improvement
if (result is null || !result.Success) { - AnsiConsole.MarkupLine($"[red]Archive failed:[/] {result?.ErrorMessage}"); + AnsiConsole.MarkupLine($"[red]Archive failed:[/] {result?.ErrorMessage ?? "Unknown error (no result returned)"}"); return 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Archive/ArchiveVerb.cs` around lines 167 - 171, The current check in ArchiveVerb that logs "Archive failed" uses {result?.ErrorMessage} which is empty when result is null; update the failure logging in the ArchiveVerb command handler (the method that contains the result null check) to include a clearer fallback message and any available context (e.g., "result was null", operation name, input parameters or exception info if available) so you don’t print a blank error; specifically, when result is null print a descriptive message like "Archive failed: no result returned" and, if possible, append result?.ErrorMessage and any caught exception details to aid debugging.src/Arius.Cli/Commands/Update/UpdateVerb.cs (2)
147-153: Consider logging cleanup failures instead of silent swallowing.The empty catch block silently discards any cleanup errors. While a cleanup failure shouldn't fail the overall update, logging at a debug level helps diagnose issues like permission problems or locked files.
try { Directory.Delete(tempDir, true); } -catch +catch (Exception ex) { + AnsiConsole.MarkupLine($"[dim]Note: Could not clean up temp directory: {ex.Message}[/]"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs` around lines 147 - 153, The empty catch around Directory.Delete(tempDir, true) in UpdateVerb silently swallows cleanup errors; change it to catch(Exception ex) and log the exception at debug (or trace) level instead of doing nothing—use the UpdateVerb class's existing logger instance (e.g., _logger or Logger) to call Debug/Trace with a short message like "Failed to delete tempDir" and include ex to capture permission/lock details so cleanup failures are visible without failing the update.
67-79: Consider expanding supported platforms.The explicit RID lookup table is a good improvement. However,
win-arm64andlinux-arm64are missing from the supported platforms. If release assets exist for these architectures, consider adding them:var ridToAsset = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) { ["win-x64"] = "arius-win-x64.exe", + ["win-arm64"] = "arius-win-arm64.exe", ["osx-x64"] = "arius-osx-x64", ["osx-arm64"] = "arius-osx-arm64", ["linux-x64"] = "arius-linux-x64", + ["linux-arm64"] = "arius-linux-arm64", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs` around lines 67 - 79, The ridToAsset lookup in UpdateVerb.cs is missing mappings for arm64 RIDs; update the ridToAsset Dictionary<string,string> (used to resolve assetName) to include "win-arm64" (e.g., "arius-win-arm64.exe") and "linux-arm64" (e.g., "arius-linux-arm64") entries so those platforms are recognized by TryGetValue and no longer hit the unsupported-platform branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.Cli/Commands/Restore/RestoreVerb.cs`:
- Around line 146-149: The continuation using ContinueWith on mediator.Send(new
RestoreCommand(opts), ct) swallows exceptions; replace the ContinueWith pattern
with an awaited call to mediator.Send so exceptions propagate (match the
ArchiveVerb approach): await the Send call (use ConfigureAwait(false) if
applicable), assign its result to the existing result variable (result = await
mediator.Send(new RestoreCommand(opts), ct)...), and remove the ContinueWith
logic; if you need to preserve behavior of capturing and rethrowing, capture any
caught exception into a pipelineException variable and rethrow it after awaiting
the task, but prefer the direct await to ensure exceptions from mediator.Send
and RestoreCommand surface correctly.
In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs`:
- Around line 106-137: The download routine that writes to tempFile after
http.GetAsync(downloadUrl, ...) in the Progress StartAsync block lacks integrity
verification; implement SHA256 checksum verification (or code signature
verification) immediately after download and before replacing the running
binary: fetch the expected checksum (e.g., from a .sha256 URL or release
metadata), compute the SHA256 of tempFile, compare and abort (and log an error)
on mismatch; if you cannot yet obtain a trusted checksum/source, at minimum add
a clear warning log entry where the download completes (after file is written
and before any install/replace step) indicating that integrity verification is
not performed.
- Around line 139-140: Environment.ProcessPath is being force-unwrapped into
currentExe and used in File.Move which can throw a NullReferenceException;
update the UpdateVerb code around the currentExe and File.Move(tempFile,
currentExe, true) usage to first check Environment.ProcessPath for null (assign
to currentExe only if non-null), and if null handle gracefully (e.g., log an
error via the same logger used in UpdateVerb and abort/return with a non-zero
exit or throw a clear exception) so you never call File.Move with a null path.
---
Duplicate comments:
In `@src/Arius.Cli/Commands/Restore/RestoreVerb.cs`:
- Around line 296-300: The failure message can be empty when the pipeline task
returns null; update the RestoreVerb error branch to provide a fallback error
string by using the local result variable (result) and its ErrorMessage if
present, otherwise a clear default like "Unknown error occurred" or the
task/exception message; specifically modify the conditional in RestoreVerb where
result is checked and the AnsiConsole.MarkupLine is called so it prints
result?.ErrorMessage ?? "Unknown error occurred" (or a similarly descriptive
default) to ensure users see a meaningful message when result is null.
In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs`:
- Around line 139-145: Self-update on Windows fails because File.Move(tempFile,
currentExe, true) attempts to overwrite the running executable (currentExe)
which is locked; update the Windows branch inside UpdateVerb (around the code
using currentExe and tempFile) to instead create and launch a short updater
(e.g., a .cmd or small helper exe) that waits for the current process to exit,
moves tempFile to currentExe, restarts the new executable, and cleans up the
updater, then call Environment.Exit(0) from the main process; keep the existing
File.Move/UnixFileMode path for non-Windows (OperatingSystem.IsWindows() check)
so Unix behavior is unchanged.
---
Nitpick comments:
In `@src/Arius.Cli/Commands/Archive/ArchiveVerb.cs`:
- Around line 167-171: The current check in ArchiveVerb that logs "Archive
failed" uses {result?.ErrorMessage} which is empty when result is null; update
the failure logging in the ArchiveVerb command handler (the method that contains
the result null check) to include a clearer fallback message and any available
context (e.g., "result was null", operation name, input parameters or exception
info if available) so you don’t print a blank error; specifically, when result
is null print a descriptive message like "Archive failed: no result returned"
and, if possible, append result?.ErrorMessage and any caught exception details
to aid debugging.
In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs`:
- Around line 147-153: The empty catch around Directory.Delete(tempDir, true) in
UpdateVerb silently swallows cleanup errors; change it to catch(Exception ex)
and log the exception at debug (or trace) level instead of doing nothing—use the
UpdateVerb class's existing logger instance (e.g., _logger or Logger) to call
Debug/Trace with a short message like "Failed to delete tempDir" and include ex
to capture permission/lock details so cleanup failures are visible without
failing the update.
- Around line 67-79: The ridToAsset lookup in UpdateVerb.cs is missing mappings
for arm64 RIDs; update the ridToAsset Dictionary<string,string> (used to resolve
assetName) to include "win-arm64" (e.g., "arius-win-arm64.exe") and
"linux-arm64" (e.g., "arius-linux-arm64") entries so those platforms are
recognized by TryGetValue and no longer hit the unsupported-platform branch.
🪄 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: 6fa88c26-b863-4ff5-b976-22cae41dd59b
📒 Files selected for processing (4)
src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cssrc/Arius.Cli/Commands/Archive/ArchiveVerb.cssrc/Arius.Cli/Commands/Restore/RestoreVerb.cssrc/Arius.Cli/Commands/Update/UpdateVerb.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs
Comment on lines
+106
to
+137
| var tempDir = Path.Combine(Path.GetTempPath(), $"arius-update-{versionStr}"); | ||
| var tempFile = Path.Combine(tempDir, assetName); | ||
| Directory.CreateDirectory(tempDir); | ||
|
|
||
| await AnsiConsole.Progress() | ||
| .AutoClear(false) | ||
| .Columns( | ||
| new TaskDescriptionColumn(), | ||
| new ProgressBarColumn(), | ||
| new PercentageColumn(), | ||
| new SpinnerColumn()) | ||
| .StartAsync(async ctx => | ||
| { | ||
| var task = ctx.AddTask("[green]Downloading update[/]"); | ||
| using var response = await http.GetAsync(downloadUrl, HttpCompletionOption.ResponseHeadersRead, ct); | ||
| response.EnsureSuccessStatusCode(); | ||
| var totalBytes = response.Content.Headers.ContentLength ?? -1; | ||
| if (totalBytes > 0) task.MaxValue = totalBytes; | ||
|
|
||
| await using var stream = await response.Content.ReadAsStreamAsync(ct); | ||
| await using var file = File.Create(tempFile); | ||
| var buffer = new byte[81920]; | ||
| long downloaded = 0; | ||
| int bytesRead; | ||
| while ((bytesRead = await stream.ReadAsync(buffer, ct)) > 0) | ||
| { | ||
| await file.WriteAsync(buffer.AsMemory(0, bytesRead), ct); | ||
| downloaded += bytesRead; | ||
| if (totalBytes > 0) task.Value = downloaded; | ||
| } | ||
| task.Value = task.MaxValue; | ||
| }); |
Contributor
There was a problem hiding this comment.
No integrity verification of downloaded binary.
The download completes without verifying the downloaded file's integrity. A corrupted download, MITM attack, or compromised CDN could result in an invalid or malicious binary being installed. Consider:
- Verifying a SHA256 checksum against a value published in the release (e.g., from a
.sha256file or release body) - Or, verifying a code signature after download
At minimum, log a warning that integrity verification is not performed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs` around lines 106 - 137, The
download routine that writes to tempFile after http.GetAsync(downloadUrl, ...)
in the Progress StartAsync block lacks integrity verification; implement SHA256
checksum verification (or code signature verification) immediately after
download and before replacing the running binary: fetch the expected checksum
(e.g., from a .sha256 URL or release metadata), compute the SHA256 of tempFile,
compare and abort (and log an error) on mismatch; if you cannot yet obtain a
trusted checksum/source, at minimum add a clear warning log entry where the
download completes (after file is written and before any install/replace step)
indicating that integrity verification is not performed.
Comment on lines
+139
to
+140
| var currentExe = Environment.ProcessPath!; | ||
| File.Move(tempFile, currentExe, true); |
Contributor
There was a problem hiding this comment.
Add null check for Environment.ProcessPath.
Environment.ProcessPath can return null in certain edge cases (e.g., some single-file deployment scenarios or when the process info is unavailable). The null-forgiving operator (!) could lead to a NullReferenceException.
-var currentExe = Environment.ProcessPath!;
+var currentExe = Environment.ProcessPath;
+if (string.IsNullOrEmpty(currentExe))
+{
+ AnsiConsole.MarkupLine("[red]Could not determine current executable path.[/]");
+ return 1;
+}
File.Move(tempFile, currentExe, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Cli/Commands/Update/UpdateVerb.cs` around lines 139 - 140,
Environment.ProcessPath is being force-unwrapped into currentExe and used in
File.Move which can throw a NullReferenceException; update the UpdateVerb code
around the currentExe and File.Move(tempFile, currentExe, true) usage to first
check Environment.ProcessPath for null (assign to currentExe only if non-null),
and if null handle gracefully (e.g., log an error via the same logger used in
UpdateVerb and abort/return with a non-zero exit or throw a clear exception) so
you never call File.Move with a null path.
…eVerb to propagate exceptions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Commands/{Archive,Restore,Ls,Update}/with sub-namespaces matching the folder structureCliBuilder.csfrom ~1452 lines down to ~170 lines — only option factories, root-command assembly, resolution helpers, DI wiring, and audit loggingProgressHandlers.cs(replaced byArchiveProgressHandlers.csandRestoreProgressHandlers.cs)RenderProgressBar,TruncateAndLeftJustify,SplitSizePair) toDisplayHelpers.csProgressTests.cswith newusingdirectives and renamed method references (CliBuilder.BuildArchiveDisplay→ArchiveVerb.BuildDisplay, etc.)Summary by CodeRabbit
New Features
Refactor