feat: add AzureCliCredential fallback auth with preflight check#37
Merged
Conversation
- Add Azure.Identity package; remove unused FluentValidation and FluentResults - Implement credential resolution: key sources (flag/env/secrets) fall back to AzureCliCredential - Add preflight connectivity check: ReadWrite (archive) uploads/deletes probe blob, ReadOnly (restore/ls) calls ExistsAsync - Translate preflight failures (403, 404, CredentialUnavailable) into PreflightException with actionable messages - Make BuildProductionServices async; update factory delegate to Func<..., PreflightMode, Task<IServiceProvider>> - Add global try/catch/finally in Program.cs for unhandled exceptions via AnsiConsole.WriteException - Update --key option description to mention az login fallback - Update CLI tests for new async factory signature
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements Azure CLI-based authentication fallback, introduces preflight connectivity validation, and extracts blob service construction to a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/CLI
participant CliBuilder
participant BlobServiceFactory
participant Azure as Azure SDK
participant Storage as BlobStorageService
Client->>CliBuilder: BuildProductionServices(accountName, key?, passphrase, containerName, preflightMode)
activate CliBuilder
alt key provided
CliBuilder->>BlobServiceFactory: CreateAsync(StorageSharedKeyCredential, ...)
else no key
CliBuilder->>BlobServiceFactory: CreateAsync(AzureCliCredential, ...)
end
activate BlobServiceFactory
BlobServiceFactory->>Azure: new BlobServiceClient(credential)
BlobServiceFactory->>Azure: container = GetBlobContainerClient()
alt preflightMode == ReadWrite
BlobServiceFactory->>Azure: Upload probe blob
BlobServiceFactory->>Azure: Delete probe blob
else preflightMode == ReadOnly
BlobServiceFactory->>Azure: container.ExistsAsync()
end
alt Preflight succeeds
BlobServiceFactory-->>Storage: new AzureBlobStorageService(containerClient)
else Credential unavailable
BlobServiceFactory-->>BlobServiceFactory: throw PreflightException(CredentialUnavailable)
else 403 Forbidden
BlobServiceFactory-->>BlobServiceFactory: throw PreflightException(AccessDenied)
else 404 Not Found
BlobServiceFactory-->>BlobServiceFactory: throw PreflightException(ContainerNotFound)
end
deactivate BlobServiceFactory
alt Preflight exception caught
CliBuilder-->>Client: Return exit code 1 with formatted error
else Success
CliBuilder->>CliBuilder: Build DI container with IBlobStorageService
CliBuilder-->>Client: Return Task<IServiceProvider>
end
deactivate CliBuilder
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #37 +/- ##
==========================================
- Coverage 82.61% 79.24% -3.37%
==========================================
Files 33 35 +2
Lines 3595 3763 +168
Branches 432 443 +11
==========================================
+ Hits 2970 2982 +12
- Misses 526 684 +158
+ Partials 99 97 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Arius.Cli.Tests/CliTests.cs (1)
359-367: Consider adding an integration test forPreflightExceptionhandling.The original
Archive_MissingKeyFromAllSources_ReturnsExitCode1test exercised the full CLI path. The replacement unit test only verifies thatResolveKeyreturnsnull—it no longer tests thePreflightExceptioncatch block in the verb handlers.While the unit test is valuable, consider adding a test that exercises the preflight failure path by configuring the harness factory to throw
PreflightException:[Test] public async Task Archive_PreflightFailure_ReturnsExitCode1() { var rootCommand = CliBuilder.BuildRootCommand( serviceProviderFactory: (_, _, _, _, _) => throw new PreflightException("Simulated preflight failure")); var exitCode = await rootCommand.Parse("archive /data -a acct -c ctr").InvokeAsync(); exitCode.ShouldBe(1); }As per coding guidelines,
**/*.Tests/**: Focus on test coverage gaps.🤖 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 359 - 367, Add an integration test that exercises the CLI preflight failure path by constructing the root command with a serviceProviderFactory that throws PreflightException (similar intent to the removed Archive_MissingKeyFromAllSources_ReturnsExitCode1 test); call CliBuilder.BuildRootCommand with a serviceProviderFactory delegate that throws new PreflightException("Simulated preflight failure"), then parse and InvokeAsync the archive command (e.g., rootCommand.Parse("archive /data -a acct -c ctr").InvokeAsync()) and assert the returned exit code equals 1 so the PreflightException catch block in the verb handlers is exercised.openspec/changes/azure-cli-auth/design.md (1)
29-34: Add language specifier to fenced code block.The code block documenting credential resolution precedence is missing a language identifier. Adding one improves rendering and addresses the markdownlint warning.
📝 Suggested fix
-``` +```text 1. --key / -k CLI flag → StorageSharedKeyCredential 2. ARIUS_KEY env var → StorageSharedKeyCredential 3. User secrets → StorageSharedKeyCredential 4. AzureCliCredential → TokenCredential (from az login)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@openspec/changes/azure-cli-auth/design.mdaround lines 29 - 34, The fenced
code block listing credential resolution precedence is missing a language
identifier; update the code fence that contains the four lines starting with
"--key / -k CLI flag → StorageSharedKeyCredential" to include a language
specifier (use "text") so the block becomestext ...which will satisfy
markdownlint and improve rendering.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@src/Arius.Cli/Program.cs:
- Around line 9-14: Exceptions thrown during command construction/parsing are
logged via AnsiConsole.WriteException but Log.Fatal is a no-op because
Log.Logger isn't initialized until ConfigureAuditLogging runs inside command
handlers; to fix, initialize a minimal fallback Serilog logger (e.g., set
Log.Logger via LoggerConfiguration) before entering the try block in
Program.Main (so Log.Fatal will record pre-handler exceptions), or call a
lightweight initialization method that sets a default sink and level and is
later replaced by ConfigureAuditLogging; reference Log.Fatal, Log.Logger,
ConfigureAuditLogging, BuildRootCommand and Parse to locate where to add the
fallback initialization.In
@src/Directory.Packages.props:
- Line 7: Update the PackageVersion entry for Azure.Identity in
Directory.Packages.props by changing the PackageVersion element with
Include="Azure.Identity" from Version="1.13.2" to Version="1.20.0"; after
updating, run a package restore/build and run the test suite to catch any API
changes, and review Azure.Identity 1.20.0 release notes for breaking changes or
migration steps before merging.
Nitpick comments:
In@openspec/changes/azure-cli-auth/design.md:
- Around line 29-34: The fenced code block listing credential resolution
precedence is missing a language identifier; update the code fence that contains
the four lines starting with "--key / -k CLI flag → StorageSharedKeyCredential"
to include a language specifier (use "text") so the block becomes ```text ...In `@src/Arius.Cli.Tests/CliTests.cs`: - Around line 359-367: Add an integration test that exercises the CLI preflight failure path by constructing the root command with a serviceProviderFactory that throws PreflightException (similar intent to the removed Archive_MissingKeyFromAllSources_ReturnsExitCode1 test); call CliBuilder.BuildRootCommand with a serviceProviderFactory delegate that throws new PreflightException("Simulated preflight failure"), then parse and InvokeAsync the archive command (e.g., rootCommand.Parse("archive /data -a acct -c ctr").InvokeAsync()) and assert the returned exit code equals 1 so the PreflightException catch block in the verb handlers is exercised.🪄 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:
1e1877b6-87c5-4501-865d-d358016e4fc9📒 Files selected for processing (15)
openspec/changes/azure-cli-auth/.openspec.yamlopenspec/changes/azure-cli-auth/design.mdopenspec/changes/azure-cli-auth/proposal.mdopenspec/changes/azure-cli-auth/specs/azure-cli-auth/spec.mdopenspec/changes/azure-cli-auth/specs/cli/spec.mdopenspec/changes/azure-cli-auth/tasks.mdsrc/Arius.Cli.Tests/CliTests.cssrc/Arius.Cli/Arius.Cli.csprojsrc/Arius.Cli/CliBuilder.cssrc/Arius.Cli/Commands/Archive/ArchiveVerb.cssrc/Arius.Cli/Commands/Ls/LsVerb.cssrc/Arius.Cli/Commands/Restore/RestoreVerb.cssrc/Arius.Cli/Program.cssrc/Arius.Core/Arius.Core.csprojsrc/Directory.Packages.props💤 Files with no reviewable changes (1)
- src/Arius.Core/Arius.Core.csproj
Comment on lines
+9
to
+14
| catch (Exception ex) | ||
| { | ||
| AnsiConsole.WriteException(ex, ExceptionFormats.ShortenEverything | ExceptionFormats.ShowLinks); | ||
| Log.Fatal(ex, "Unhandled exception"); | ||
| return 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Log.Fatal may be a no-op for exceptions occurring before command handler execution.
Based on the code flow in CliBuilder.cs and the verb handlers, ConfigureAuditLogging (which initializes Log.Logger) is called inside command handlers—after BuildRootCommand() and Parse() complete. If an exception occurs during command construction or parsing, Log.Logger remains in its default no-op state, and this Log.Fatal call will silently discard the log entry.
The user still sees the exception via AnsiConsole.WriteException, so this is a minor observability gap rather than a functional bug.
💡 Potential fix: Initialize a minimal fallback logger before the try block
using Arius.Cli;
using Serilog;
using Spectre.Console;
+// Minimal bootstrap logger for exceptions before command handlers configure audit logging
+Log.Logger = new LoggerConfiguration()
+ .WriteTo.Console(restrictedToMinimumLevel: Serilog.Events.LogEventLevel.Error)
+ .CreateBootstrapLogger();
+
try
{
return await CliBuilder.BuildRootCommand().Parse(args).InvokeAsync();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Cli/Program.cs` around lines 9 - 14, Exceptions thrown during
command construction/parsing are logged via AnsiConsole.WriteException but
Log.Fatal is a no-op because Log.Logger isn't initialized until
ConfigureAuditLogging runs inside command handlers; to fix, initialize a minimal
fallback Serilog logger (e.g., set Log.Logger via LoggerConfiguration) before
entering the try block in Program.Main (so Log.Fatal will record pre-handler
exceptions), or call a lightweight initialization method that sets a default
sink and level and is later replaced by ConfigureAuditLogging; reference
Log.Fatal, Log.Logger, ConfigureAuditLogging, BuildRootCommand and Parse to
locate where to add the fallback initialization.
| <CentralPackageTransitivePinningEnabled>false</CentralPackageTransitivePinningEnabled> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageVersion Include="Azure.Identity" Version="1.13.2" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of Azure.Identity NuGet package and are there any known vulnerabilities in version 1.13.2?
💡 Result:
The latest version of the Azure.Identity NuGet package is 1.20.0, released on 2026-03-30. There are no known vulnerabilities in version 1.13.2. Known CVEs affecting Azure.Identity for .NET (CVE-2023-36414 fixed in 1.10.2+, CVE-2024-29992 fixed in 1.11.0+) predate 1.13.2 (released 2025-01-15), and CVE-2024-35255 affects versions before 1.11.4 but scanners report false positives for later versions like 1.13.2; scans of 1.14.0 show no vulnerabilities, and NuGet does not flag 1.13.2.
Citations:
- 1: https://www.nuget.org/packages/azure.identity
- 2: https://github.com/Azure/azure-sdk-for-net/releases/tag/Azure.Identity_1.20.0
- 3: https://nuget.org/packages/Azure.Identity
- 4: https://secure.software/nuget/packages/azure.identity/vulnerabilities/1.14.0
- 5: https://vulert.com/vuln-db/CVE-2023-36414
- 6: https://advisories.gitlab.com/pkg/nuget/azure.identity/CVE-2024-29992/
- 7: https://vulert.com/vuln-db/CVE-2024-29992
- 8: https://advisories.gitlab.com/pkg/nuget/azure.identity/CVE-2024-29992
- 9: https://advisories.ecosyste.ms/advisories/GSA_kwCzR0hTQS01bWZ4LTR3Y3gtcnYyN84AA2Vz
- 10: https://www.cvedetails.com/cve/CVE-2024-35255/
- 11: https://nuget.health.go.ke/packages/azure.identity/1.13.2
Consider upgrading Azure.Identity to version 1.20.0.
Version 1.13.2 is secure with no known vulnerabilities, but version 1.20.0 (released 2026-03-30) is the latest available release. Evaluate whether upgrading would benefit the project, as it may include performance improvements, new features, or additional security enhancements beyond the baseline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Directory.Packages.props` at line 7, Update the PackageVersion entry for
Azure.Identity in Directory.Packages.props by changing the PackageVersion
element with Include="Azure.Identity" from Version="1.13.2" to Version="1.20.0";
after updating, run a package restore/build and run the test suite to catch any
API changes, and review Azure.Identity 1.20.0 release notes for breaking changes
or migration steps before merging.
Move PreflightMode, PreflightException (now with structured fields), and preflight probe logic from Arius.Cli into a static BlobServiceFactory in Arius.AzureBlob. Add Azure.Identity to AzureBlob; remove it from Cli. Slim CliBuilder.BuildProductionServices to credential resolution + DI wiring. Update verb catch blocks to switch on PreflightErrorKind for per-host formatting. Widen Core architecture test to all Azure namespaces; add Cli_Should_Not_Reference_Azure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/Arius.Architecture.Tests/DependencyTests.cs (1)
28-30: Consider adding Tests exclusion for consistency.Lines 42, 52, 53, 65, and 66 exclude test assemblies with
&& !a.FullName.Contains("Tests"), but line 29 does not. For consistency and to guard against future test projects, consider adding the same exclusion:IArchRule rule = Classes().That().ResideInAssembly( - Architecture.Assemblies.First(a => a.FullName.Contains("Arius.Core"))) + Architecture.Assemblies.First(a => a.FullName.Contains("Arius.Core") && !a.FullName.Contains("Tests"))) .Should().NotDependOnAnyTypesThat().ResideInNamespace("Azure");🤖 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 28 - 30, The rule creation using IArchRule and Classes().That().ResideInAssembly(Architecture.Assemblies.First(a => a.FullName.Contains("Arius.Core"))) should mirror other exclusions by ignoring test assemblies; update the predicate inside ResideInAssembly (the First lambda) to also exclude assemblies whose FullName contains "Tests" (e.g., change a.FullName.Contains("Arius.Core") to a.FullName.Contains("Arius.Core") && !a.FullName.Contains("Tests")) so the rule consistently skips test projects.src/Arius.AzureBlob/BlobServiceFactory.cs (1)
83-89: Probe blob cleanup is best-effort; consider documenting or handling edge case.If the upload succeeds but delete fails (e.g., network interruption), the
.arius-preflight-probeblob remains in the container. This is mitigated by:
- The blob is 0 bytes (negligible storage cost)
overwrite: trueensures the next preflight will succeedThis is acceptable, but consider adding a comment documenting this behavior, or catching delete failures separately to avoid confusing users with a preflight error when write access was actually confirmed.
Optional: Document or handle delete failure gracefully
if (preflightMode == PreflightMode.ReadWrite) { var probeBlob = containerClient.GetBlobClient(PreflightProbeBlobName); using var emptyStream = new MemoryStream(); await probeBlob.UploadAsync(emptyStream, overwrite: true).ConfigureAwait(false); - await probeBlob.DeleteAsync().ConfigureAwait(false); + // Best-effort cleanup; if this fails, the 0-byte probe blob remains + // and will be overwritten on the next invocation. + try + { + await probeBlob.DeleteAsync().ConfigureAwait(false); + } + catch (Azure.RequestFailedException) + { + // Ignore delete failures — write access was verified + } }🤖 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 83 - 89, The preflight probe cleanup in the PreflightMode.ReadWrite branch (using PreflightProbeBlobName and the probeBlob UploadAsync/DeleteAsync calls in BlobServiceFactory) can leave a zero-byte blob if DeleteAsync fails; update the block to handle delete failures gracefully by catching exceptions from probeBlob.DeleteAsync() and logging a non-fatal warning (or documenting the best-effort cleanup with an explanatory comment) so a delete error doesn't surface as a preflight write failure while the upload succeeded.openspec/changes/blob-service-factory/proposal.md (1)
31-31: Consider direct unit tests for BlobServiceFactory instead of relying solely on transitive testing.While CLI tests will exercise the factory, direct unit tests would provide:
- Faster feedback when factory logic breaks
- Better isolation of failure causes
- Easier testing of edge cases (network errors, malformed credentials, etc.)
- Documentation of factory behavior through test names
- Protection against future CLI refactoring that might reduce coverage
Consider adding a small test suite for
BlobServiceFactory.CreateAsyncthat mocks Azure SDK interactions and verifies:
- Credential type validation
- Error classification (403 → AccessDenied, 404 → ContainerNotFound, etc.)
- Preflight probe behavior for both ReadOnly and ReadWrite modes
- Structured exception field population
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/blob-service-factory/proposal.md` at line 31, Add a focused unit test suite for BlobServiceFactory.CreateAsync rather than relying only on transitive CLI tests: create tests that mock the Azure SDK interactions (e.g., mock container client/credential behavior) to assert credential type validation, map HTTP errors to the correct classifications (403 → AccessDenied, 404 → ContainerNotFound, etc.), exercise preflight probe behavior for ReadOnly and ReadWrite modes, and verify PreflightException structured fields are populated; update existing Arius.Cli.Tests catch-block assertions if they assume different exception shapes so they match the new structured PreflightException representation.openspec/changes/blob-service-factory/specs/cli/spec.md (1)
23-23: DI registration ordering requirement may be fragile.The requirement "Handler registrations MUST be placed after
AddMediator()to override the source generator's auto-registrations" relies on call order, which is error-prone and may break silently if someone refactors the registration sequence.Consider adding:
- A runtime assertion or startup validation check that verifies handler registrations are correct
- An architecture test that validates the registration order
- Code comments at both call sites warning against reordering
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/blob-service-factory/specs/cli/spec.md` at line 23, Add a startup-time validation to fail fast when handler registrations are not overriding the source-generator defaults: in the AddArius() extension (where ICommandHandler<TCommand,TResult> handlers are registered) add a post-registration check that queries the IServiceCollection (or built IServiceProvider) to assert that each expected ICommandHandler<TCommand,TResult> is registered with the factory delegate signature taking accountName/containerName and that AddMediator() registrations have been replaced; if the assertion fails throw an InvalidOperationException with a clear message explaining the required ordering and how to fix it. Also add a short comment in AddMediator() and AddArius() noting the ordering requirement and add the same runtime assertion in CliBuilder.BuildProductionServices before calling BlobServiceFactory.CreateAsync to ensure the DI state is correct at startup; keep messages referencing AddArius, AddMediator, ICommandHandler<,>, CliBuilder.BuildProductionServices, BlobServiceFactory.CreateAsync and IBlobStorageService so maintainers can find the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/blob-service-factory/specs/cli/spec.md`:
- Around line 38-56: Add spec scenarios for the missing PreflightErrorKind.Other
and the ls verb: include a scenario for "Other" where WHEN a verb catches
PreflightException with ErrorKind = Other and THEN the CLI SHALL display a
generic error message that includes the structured fields and an instruction to
open an issue; and add AccessDenied token-auth scenarios for the ls verb
mirroring archive/restore (WHEN the ls verb catches PreflightException with
ErrorKind = AccessDenied and AuthMode = "token" THEN suggest the appropriate
RBAC role or minimal permission guidance consistent with archive (Storage Blob
Data Contributor) or restore (Storage Blob Data Reader) as applicable).
- Line 4: Add a new test scenario to the CLI spec that verifies the resolved key
value is never logged or printed: describe running Arius.Cli with a sensitive
key provided via --key/-k, via ARIUS_KEY, and via user secrets and assert no
process output, error logs, or telemetry contains the raw key; mention that
credential resolution remains in Arius.Cli and the resolved credential
(StorageSharedKeyCredential or TokenCredential) is passed to
BlobServiceFactory.CreateAsync, and explicitly state that test must check
console/stdout, logger outputs, and any serialized diagnostic/telemetry for
absence of the key.
---
Nitpick comments:
In `@openspec/changes/blob-service-factory/proposal.md`:
- Line 31: Add a focused unit test suite for BlobServiceFactory.CreateAsync
rather than relying only on transitive CLI tests: create tests that mock the
Azure SDK interactions (e.g., mock container client/credential behavior) to
assert credential type validation, map HTTP errors to the correct
classifications (403 → AccessDenied, 404 → ContainerNotFound, etc.), exercise
preflight probe behavior for ReadOnly and ReadWrite modes, and verify
PreflightException structured fields are populated; update existing
Arius.Cli.Tests catch-block assertions if they assume different exception shapes
so they match the new structured PreflightException representation.
In `@openspec/changes/blob-service-factory/specs/cli/spec.md`:
- Line 23: Add a startup-time validation to fail fast when handler registrations
are not overriding the source-generator defaults: in the AddArius() extension
(where ICommandHandler<TCommand,TResult> handlers are registered) add a
post-registration check that queries the IServiceCollection (or built
IServiceProvider) to assert that each expected ICommandHandler<TCommand,TResult>
is registered with the factory delegate signature taking
accountName/containerName and that AddMediator() registrations have been
replaced; if the assertion fails throw an InvalidOperationException with a clear
message explaining the required ordering and how to fix it. Also add a short
comment in AddMediator() and AddArius() noting the ordering requirement and add
the same runtime assertion in CliBuilder.BuildProductionServices before calling
BlobServiceFactory.CreateAsync to ensure the DI state is correct at startup;
keep messages referencing AddArius, AddMediator, ICommandHandler<,>,
CliBuilder.BuildProductionServices, BlobServiceFactory.CreateAsync and
IBlobStorageService so maintainers can find the checks.
In `@src/Arius.Architecture.Tests/DependencyTests.cs`:
- Around line 28-30: The rule creation using IArchRule and
Classes().That().ResideInAssembly(Architecture.Assemblies.First(a =>
a.FullName.Contains("Arius.Core"))) should mirror other exclusions by ignoring
test assemblies; update the predicate inside ResideInAssembly (the First lambda)
to also exclude assemblies whose FullName contains "Tests" (e.g., change
a.FullName.Contains("Arius.Core") to a.FullName.Contains("Arius.Core") &&
!a.FullName.Contains("Tests")) so the rule consistently skips test projects.
In `@src/Arius.AzureBlob/BlobServiceFactory.cs`:
- Around line 83-89: The preflight probe cleanup in the PreflightMode.ReadWrite
branch (using PreflightProbeBlobName and the probeBlob UploadAsync/DeleteAsync
calls in BlobServiceFactory) can leave a zero-byte blob if DeleteAsync fails;
update the block to handle delete failures gracefully by catching exceptions
from probeBlob.DeleteAsync() and logging a non-fatal warning (or documenting the
best-effort cleanup with an explanatory comment) so a delete error doesn't
surface as a preflight write failure while the upload succeeded.
🪄 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: 7a56926f-7a45-475b-acb8-918e2ba73894
📒 Files selected for processing (15)
openspec/changes/blob-service-factory/.openspec.yamlopenspec/changes/blob-service-factory/design.mdopenspec/changes/blob-service-factory/proposal.mdopenspec/changes/blob-service-factory/specs/architecture-tests/spec.mdopenspec/changes/blob-service-factory/specs/blob-service-factory/spec.mdopenspec/changes/blob-service-factory/specs/cli/spec.mdopenspec/changes/blob-service-factory/tasks.mdsrc/Arius.Architecture.Tests/DependencyTests.cssrc/Arius.AzureBlob/Arius.AzureBlob.csprojsrc/Arius.AzureBlob/BlobServiceFactory.cssrc/Arius.AzureBlob/Preflight.cssrc/Arius.Cli/CliBuilder.cssrc/Arius.Cli/Commands/Archive/ArchiveVerb.cssrc/Arius.Cli/Commands/Ls/LsVerb.cssrc/Arius.Cli/Commands/Restore/RestoreVerb.cs
✅ Files skipped from review due to trivial changes (4)
- openspec/changes/blob-service-factory/.openspec.yaml
- src/Arius.AzureBlob/Arius.AzureBlob.csproj
- openspec/changes/blob-service-factory/design.md
- openspec/changes/blob-service-factory/specs/blob-service-factory/spec.md
| ## MODIFIED Requirements | ||
|
|
||
| ### Requirement: Account key resolution | ||
| The CLI SHALL resolve the account key in the following order: (1) `--key` / `-k` CLI parameter, (2) `ARIUS_KEY` environment variable, (3) `Microsoft.Extensions.Configuration.UserSecrets` (hidden, developer use only). The key SHALL NEVER be logged or displayed in output. The `--key` option description SHALL NOT mention user secrets. Credential resolution (key lookup and fallback to `AzureCliCredential`) SHALL remain in `Arius.Cli`. The resolved credential SHALL be passed to `BlobServiceFactory.CreateAsync` as either a `StorageSharedKeyCredential` or a `TokenCredential`. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a test scenario to verify keys are never logged.
The requirement states "The key SHALL NEVER be logged or displayed in output," which is critical for security. However, there is no corresponding test scenario to verify this constraint.
💡 Suggested additional scenario
Add after line 21:
+#### Scenario: Account key is never logged
+- **WHEN** any command is executed with a key from any source
+- **THEN** the key SHALL NOT appear in console output, log files, or error messages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/blob-service-factory/specs/cli/spec.md` at line 4, Add a new
test scenario to the CLI spec that verifies the resolved key value is never
logged or printed: describe running Arius.Cli with a sensitive key provided via
--key/-k, via ARIUS_KEY, and via user secrets and assert no process output,
error logs, or telemetry contains the raw key; mention that credential
resolution remains in Arius.Cli and the resolved credential
(StorageSharedKeyCredential or TokenCredential) is passed to
BlobServiceFactory.CreateAsync, and explicitly state that test must check
console/stdout, logger outputs, and any serialized diagnostic/telemetry for
absence of the key.
Comment on lines
+38
to
+56
| #### Scenario: ContainerNotFound error formatted | ||
| - **WHEN** a verb catches `PreflightException` with `ErrorKind = ContainerNotFound` | ||
| - **THEN** the CLI SHALL display a message including the container name and account name from the structured fields | ||
|
|
||
| #### Scenario: AccessDenied with key auth formatted | ||
| - **WHEN** a verb catches `PreflightException` with `ErrorKind = AccessDenied` and `AuthMode = "key"` | ||
| - **THEN** the CLI SHALL display a message suggesting the account key may be incorrect | ||
|
|
||
| #### Scenario: AccessDenied with token auth formatted for archive | ||
| - **WHEN** the archive verb catches `PreflightException` with `ErrorKind = AccessDenied` and `AuthMode = "token"` | ||
| - **THEN** the CLI SHALL display a message suggesting the "Storage Blob Data Contributor" RBAC role | ||
|
|
||
| #### Scenario: AccessDenied with token auth formatted for restore | ||
| - **WHEN** the restore verb catches `PreflightException` with `ErrorKind = AccessDenied` and `AuthMode = "token"` | ||
| - **THEN** the CLI SHALL display a message suggesting the "Storage Blob Data Reader" RBAC role | ||
|
|
||
| #### Scenario: CredentialUnavailable error formatted | ||
| - **WHEN** a verb catches `PreflightException` with `ErrorKind = CredentialUnavailable` | ||
| - **THEN** the CLI SHALL display a message listing the credential resolution options (--key, ARIUS_KEY, user secrets, az login) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing scenarios for PreflightErrorKind.Other and ls verb.
The specification defines scenarios for ContainerNotFound, AccessDenied, and CredentialUnavailable, but PreflightErrorKind.Other (mentioned in tasks.md) has no formatting scenario. Additionally, the ls verb is mentioned in PR objectives but only archive and restore have AccessDenied token auth scenarios.
📝 Suggested additional scenarios
Add after line 56:
+#### Scenario: Other error formatted
+- **WHEN** a verb catches `PreflightException` with `ErrorKind = Other`
+- **THEN** the CLI SHALL display a message including the status code, account name, container name, and inner exception details
+
+#### Scenario: AccessDenied with token auth formatted for ls
+- **WHEN** the ls verb catches `PreflightException` with `ErrorKind = AccessDenied` and `AuthMode = "token"`
+- **THEN** the CLI SHALL display a message suggesting the "Storage Blob Data Reader" RBAC role🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/blob-service-factory/specs/cli/spec.md` around lines 38 -
56, Add spec scenarios for the missing PreflightErrorKind.Other and the ls verb:
include a scenario for "Other" where WHEN a verb catches PreflightException with
ErrorKind = Other and THEN the CLI SHALL display a generic error message that
includes the structured fields and an instruction to open an issue; and add
AccessDenied token-auth scenarios for the ls verb mirroring archive/restore
(WHEN the ls verb catches PreflightException with ErrorKind = AccessDenied and
AuthMode = "token" THEN suggest the appropriate RBAC role or minimal permission
guidance consistent with archive (Storage Blob Data Contributor) or restore
(Storage Blob Data Reader) as applicable).
Sync delta specs to main: create openspec/specs/blob-service-factory/spec.md, create openspec/specs/architecture-tests/spec.md, update openspec/specs/cli/spec.md with account key resolution, DI handler delegation, and PreflightException formatting. Archive change to openspec/changes/archive/2026-04-01-blob-service-factory/.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
openspec/changes/archive/2026-04-01-blob-service-factory/proposal.md (1)
31-31: Consider adding direct factory-focused tests.On Line 31, relying only on transitive CLI coverage may miss classification/probe edge cases in
BlobServiceFactory. A small direct test set for 403/404/credential-unavailable mapping would make this boundary safer to evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-04-01-blob-service-factory/proposal.md` at line 31, Tests currently rely only on transitive coverage through Arius.Cli.Tests which can miss edge cases in BlobServiceFactory; add a small direct unit test suite that exercises BlobServiceFactory to validate mapping of 403/404/credential-unavailable scenarios and ensure PreflightException classification/probe behavior remains correct. Create tests that instantiate BlobServiceFactory (or its test-friendly constructor) and simulate Azure client responses for 403 and 404 and a credential-unavailable error, then assert the factory returns the expected mapped result or throws the structured PreflightException as expected; add focused cases for each mapping and update Arius.Cli.Tests catch-block assertions if the factory test reveals any behavioral differences.openspec/changes/archive/2026-04-01-blob-service-factory/design.md (3)
7-19: Goals section uses future tense for completed work.The Goals list uses imperative/future phrasing ("Extract a
BlobServiceFactory", "MovePreflightMode", "MakePreflightExceptioncarry") when describing changes that are already implemented in the codebase (per context snippets 1-3). This reinforces the timeline confusion noted in the Context section.Consider either:
- Rewriting in past tense ("Extracted...", "Moved...", "Made...") to document what was accomplished, or
- Reframing this as an "Architecture Decision Record" (ADR) with a header like
## Decisionand## Status: Implementedto clarify this captures decisions that have been made and executed.♻️ Example reframe as past-tense achievements
-## Goals / Non-Goals +## Changes Implemented -**Goals:** -- Extract a `BlobServiceFactory` into `Arius.AzureBlob` that any host can call to get a preflight-validated `IBlobStorageService` -- Move `PreflightMode` and `PreflightException` to `Arius.AzureBlob` so both hosts share the same types -- Make `PreflightException` carry structured data (status code, auth mode, account, container) instead of pre-formatted strings, so each host can format its own user-facing messages -- Keep `CliBuilder.BuildProductionServices` as a thin wrapper: resolve credential, call factory, wire DI +**What was done:** +- Extracted `BlobServiceFactory` into `Arius.AzureBlob` so any host can call it to get a preflight-validated `IBlobStorageService` +- Moved `PreflightMode` and `PreflightException` to `Arius.AzureBlob` so all hosts share the same types +- Made `PreflightException` carry structured data (status code, auth mode, account, container) instead of pre-formatted strings, so each host formats its own messages +- Kept `CliBuilder.BuildProductionServices` as a thin wrapper: resolve credential, call factory, wire DI -**Non-Goals:** +**Out of scope for this change:** - Changing the credential resolution logic (CLI flag > env var > user secrets > `AzureCliCredential`) -- that stays in CLI - Adding a second host in this change -- that is a separate future change🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-04-01-blob-service-factory/design.md` around lines 7 - 19, The Goals section in design.md uses future/imperative tense for work that's already done; update that section to reflect completed work by changing phrases like "Extract a `BlobServiceFactory`", "Move `PreflightMode`", and "Make `PreflightException` carry" to past tense (e.g., "Extracted `BlobServiceFactory`", "Moved `PreflightMode`", "Made `PreflightException` carry") or convert the document into an ADR-style entry with headers such as "## Decision" and "## Status: Implemented" so the intent and status of symbols like BlobServiceFactory, PreflightMode, and PreflightException are clearly recorded as completed.
78-84: Risks section describes past trade-offs as future concerns.The Risks section warns about "breaking changes" and proposes "mitigations" as if they're future concerns. However, since the implementation is complete and the PR objectives confirm "All tests pass — 137 CLI tests and 122 Core tests," these trade-offs have already been accepted and addressed.
For instance:
- "Breaking change for test helpers: Mitigation: Straightforward find-and-replace" → This implies a future action, but the tests already pass, so the change has been made.
- "
Azure.Identitytransitive dependency size" → The dependency is already added (the factory in snippet 1 catchesCredentialUnavailableExceptionfromAzure.Identity).Consider reframing this section to document the trade-offs that were made and how they were handled, rather than warning about future risks.
♻️ Example reframe as completed trade-offs
-## Risks / Trade-offs +## Trade-offs Accepted -- **Breaking change for test helpers**: `Arius.Cli.Tests` uses a factory delegate typed `Func<string, string?, string?, string, PreflightMode, Task<IServiceProvider>>`. The `PreflightMode` namespace changes from `Arius.Cli` to `Arius.AzureBlob`, requiring a `using` update. No behavioral change. **Mitigation**: Straightforward find-and-replace. +- **Breaking change for test helpers**: `Arius.Cli.Tests` factory delegates required a `using` update when `PreflightMode` moved from `Arius.Cli` to `Arius.AzureBlob`. This was a straightforward namespace change with no behavioral impact (test suite remains at 137 passing tests). -- **`Azure.Identity` transitive dependency size**: Adding `Azure.Identity` to `AzureBlob` pulls in MSAL and related packages at the library level. However, `Arius.Cli` already depends on `Azure.Identity`, and AzureBlob is only consumed by CLI (and the future second host, which will also need it). **Mitigation**: No actual binary size increase since the dependency is already transitively present. +- **`Azure.Identity` transitive dependency size**: Adding `Azure.Identity` to `Arius.AzureBlob` pulled in MSAL and related packages. This was acceptable because `Arius.Cli` already had this dependency, so there was no binary size increase for the current host. -- **Enum evolution**: Adding a new `PreflightErrorKind` value in the future requires updating all host switch blocks. **Mitigation**: There are only two hosts planned, and the enum is small (4 values). Exhaustive switch with a default throw will catch missing cases at compile time. +- **Enum evolution**: Adding a new `PreflightErrorKind` value will require updating all host switch blocks. This was acceptable because only two hosts are planned, the enum is small (4 values), and exhaustive switch statements with a default throw will catch missing cases at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-04-01-blob-service-factory/design.md` around lines 78 - 84, Reword the "Risks / Trade-offs" section under the "## Risks / Trade-offs" heading to present the items as completed trade-offs rather than future risks: change wording around "Breaking change for test helpers" to state that the PreflightMode namespace move (affecting Arius.Cli.Tests and the factory delegate typed Func<string, string?, string?, string, PreflightMode, Task<IServiceProvider>>) was updated (mention the required using change) and that tests were updated/passed; change the Azure.Identity item to note the dependency was already present and caught (e.g., CredentialUnavailableException handled by the factory) and therefore caused no binary size regression; and update the PreflightErrorKind item to state that enum evolution is handled via exhaustive switches with a default throw and that hosts were updated as needed. Ensure you reference the symbols PreflightMode, Arius.Cli.Tests, Azure.Identity, CredentialUnavailableException, and PreflightErrorKind in the revised text.
21-77: Decisions section framed as proposals rather than completed choices.The Decisions section accurately describes the implemented architecture (confirmed by context snippets), but uses present/imperative tense ("lives", "is", "Add") as if proposing design choices to be made. This compounds the timeline ambiguity throughout the document.
For example:
- Decision 1: "The factory is a static
CreateAsyncmethod" → ✅ matches snippet 1- Decision 3: Lists
PreflightExceptionfields → ✅ matches snippet 2 exactly- Decision 4: "Add
Azure.IdentitytoArius.AzureBlob" → imperative suggests future workIf this document is meant to record the design rationale for the implemented refactoring (which is a valuable practice), consider reframing each decision in past tense to reflect that these choices have been made and executed.
♻️ Example reframe for Decision 1
-### 1. Factory lives in `Arius.AzureBlob` as a static async method +### 1. Factory was implemented in `Arius.AzureBlob` as a static async method -The factory is a static `CreateAsync` method on a new `BlobServiceFactory` class. It takes a `TokenCredential` or `StorageSharedKeyCredential` (via the common base -- both are passed as typed overloads or a discriminated parameter), account name, container name, and `PreflightMode`. It returns an `IBlobStorageService`. +The factory was implemented as a static `CreateAsync` method on `BlobServiceFactory`. It accepts a `TokenCredential` or `StorageSharedKeyCredential` (via an `object` parameter with runtime type check), account name, container name, and `PreflightMode`. It returns an `IBlobStorageService`. -**Why static**: The factory has no instance state. It builds the client chain, runs the preflight probe, and returns the service. No reason to instantiate it. +**Rationale for static**: The factory has no instance state. Making it static avoids unnecessary instantiation for what is purely a construction pipeline.Apply similar changes to decisions 2-6.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-04-01-blob-service-factory/design.md` around lines 21 - 77, Reword the Decisions section so each item records completed choices in past tense rather than proposing actions: e.g., change Decision 1 to state "The factory was implemented as a static CreateAsync method on BlobServiceFactory" and similarly update Decision 2 (credential parameter was accepted as object with runtime type checks), Decision 3 (PreflightException carries structured fields), Decision 4 (Azure.Identity was added to Arius.AzureBlob), Decision 5 (added ArchUnit test Cli_Should_Not_Reference_Azure and widened Core_Should_Not_Reference_Azure), and Decision 6 (CLI error formatting was moved to verb catch blocks); preserve the same facts, symbols (BlobServiceFactory.CreateAsync, PreflightException, PreflightErrorKind, Cli_Should_Not_Reference_Azure, Core_Should_Not_Reference_Azure, Azure.Identity), and rationale but convert imperative/present wording into past-tense statements summarizing what was done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/archive/2026-04-01-blob-service-factory/design.md`:
- Around line 1-6: Update the "Context" paragraph to describe the refactor as
completed rather than planned: state that PreflightMode, PreflightErrorKind, and
PreflightException are defined in src/Arius.AzureBlob/Preflight.cs, and that
BlobServiceFactory.CreateAsync now encapsulates the client construction,
preflight probe and error classification; also note that
CliBuilder.BuildProductionServices delegates to BlobServiceFactory.CreateAsync
(lines referenced for reviewers only) so no duplication is required for the
non-CLI host. Ensure the wording clearly communicates the current architecture
and where each responsibility now lives.
In `@openspec/changes/archive/2026-04-01-blob-service-factory/proposal.md`:
- Around line 12-13: Reword the sentence about PreflightException.Message to
clarify that PreflightException still sets a base, preformatted Message in
src/Arius.AzureBlob/Preflight.cs, but the important change is that hosts no
longer rely on that Message for user-facing UX and instead use the structured
fields (status code, account, container, auth mode) to format errors; update the
line to explicitly state "exception retains a base Message but hosts must ignore
it for UX and format errors from structured fields" and keep the note about
adding Azure.Identity separate.
In
`@openspec/changes/archive/2026-04-01-blob-service-factory/specs/architecture-tests/spec.md`:
- Line 4: Update the spec text to reflect current tests: state that the
architecture tests enforce only the Arius.AzureBlob assembly may reference Azure
SDK namespaces and that both Core and Cli boundaries are already enforced by
tests in DependencyTests.cs using ResideInNamespace("Azure") (i.e., the existing
Core_Should_Not_Reference_Azure check was broadened and a
Cli_Should_Not_Reference_Azure test exists), replacing the stale wording about
planned expansion with this current enforcement description and referencing the
DependencyTests.cs test names (Core_Should_Not_Reference_Azure,
Cli_Should_Not_Reference_Azure) and the ResideInNamespace("Azure") assertion.
---
Nitpick comments:
In `@openspec/changes/archive/2026-04-01-blob-service-factory/design.md`:
- Around line 7-19: The Goals section in design.md uses future/imperative tense
for work that's already done; update that section to reflect completed work by
changing phrases like "Extract a `BlobServiceFactory`", "Move `PreflightMode`",
and "Make `PreflightException` carry" to past tense (e.g., "Extracted
`BlobServiceFactory`", "Moved `PreflightMode`", "Made `PreflightException`
carry") or convert the document into an ADR-style entry with headers such as "##
Decision" and "## Status: Implemented" so the intent and status of symbols like
BlobServiceFactory, PreflightMode, and PreflightException are clearly recorded
as completed.
- Around line 78-84: Reword the "Risks / Trade-offs" section under the "## Risks
/ Trade-offs" heading to present the items as completed trade-offs rather than
future risks: change wording around "Breaking change for test helpers" to state
that the PreflightMode namespace move (affecting Arius.Cli.Tests and the factory
delegate typed Func<string, string?, string?, string, PreflightMode,
Task<IServiceProvider>>) was updated (mention the required using change) and
that tests were updated/passed; change the Azure.Identity item to note the
dependency was already present and caught (e.g., CredentialUnavailableException
handled by the factory) and therefore caused no binary size regression; and
update the PreflightErrorKind item to state that enum evolution is handled via
exhaustive switches with a default throw and that hosts were updated as needed.
Ensure you reference the symbols PreflightMode, Arius.Cli.Tests, Azure.Identity,
CredentialUnavailableException, and PreflightErrorKind in the revised text.
- Around line 21-77: Reword the Decisions section so each item records completed
choices in past tense rather than proposing actions: e.g., change Decision 1 to
state "The factory was implemented as a static CreateAsync method on
BlobServiceFactory" and similarly update Decision 2 (credential parameter was
accepted as object with runtime type checks), Decision 3 (PreflightException
carries structured fields), Decision 4 (Azure.Identity was added to
Arius.AzureBlob), Decision 5 (added ArchUnit test Cli_Should_Not_Reference_Azure
and widened Core_Should_Not_Reference_Azure), and Decision 6 (CLI error
formatting was moved to verb catch blocks); preserve the same facts, symbols
(BlobServiceFactory.CreateAsync, PreflightException, PreflightErrorKind,
Cli_Should_Not_Reference_Azure, Core_Should_Not_Reference_Azure,
Azure.Identity), and rationale but convert imperative/present wording into
past-tense statements summarizing what was done.
In `@openspec/changes/archive/2026-04-01-blob-service-factory/proposal.md`:
- Line 31: Tests currently rely only on transitive coverage through
Arius.Cli.Tests which can miss edge cases in BlobServiceFactory; add a small
direct unit test suite that exercises BlobServiceFactory to validate mapping of
403/404/credential-unavailable scenarios and ensure PreflightException
classification/probe behavior remains correct. Create tests that instantiate
BlobServiceFactory (or its test-friendly constructor) and simulate Azure client
responses for 403 and 404 and a credential-unavailable error, then assert the
factory returns the expected mapped result or throws the structured
PreflightException as expected; add focused cases for each mapping and update
Arius.Cli.Tests catch-block assertions if the factory test reveals any
behavioral differences.
🪄 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: df159d7a-930e-46b2-b591-dd773ccd193c
📒 Files selected for processing (10)
openspec/changes/archive/2026-04-01-blob-service-factory/.openspec.yamlopenspec/changes/archive/2026-04-01-blob-service-factory/design.mdopenspec/changes/archive/2026-04-01-blob-service-factory/proposal.mdopenspec/changes/archive/2026-04-01-blob-service-factory/specs/architecture-tests/spec.mdopenspec/changes/archive/2026-04-01-blob-service-factory/specs/blob-service-factory/spec.mdopenspec/changes/archive/2026-04-01-blob-service-factory/specs/cli/spec.mdopenspec/changes/archive/2026-04-01-blob-service-factory/tasks.mdopenspec/specs/architecture-tests/spec.mdopenspec/specs/blob-service-factory/spec.mdopenspec/specs/cli/spec.md
✅ Files skipped from review due to trivial changes (7)
- openspec/changes/archive/2026-04-01-blob-service-factory/.openspec.yaml
- openspec/specs/architecture-tests/spec.md
- openspec/changes/archive/2026-04-01-blob-service-factory/tasks.md
- openspec/specs/blob-service-factory/spec.md
- openspec/changes/archive/2026-04-01-blob-service-factory/specs/blob-service-factory/spec.md
- openspec/specs/cli/spec.md
- openspec/changes/archive/2026-04-01-blob-service-factory/specs/cli/spec.md
Comment on lines
+1
to
+6
| ## Context | ||
|
|
||
| `CliBuilder.BuildProductionServices` (lines 147-264 of `CliBuilder.cs`) currently owns the entire chain from credential to validated `IBlobStorageService`: it builds a `BlobServiceClient`, obtains a `BlobContainerClient`, runs a preflight connectivity probe, classifies errors into `PreflightException`, and constructs `AzureBlobStorageService`. The `PreflightMode` enum and `PreflightException` class also live in `Arius.Cli`. | ||
|
|
||
| A second (non-CLI) host is planned. Without extraction, that host must duplicate all of this logic. The `Arius.AzureBlob` project is the natural home: it already owns `AzureBlobStorageService` and has the `Azure.Storage.Blobs` dependency. `Arius.Core` must remain Azure-free (enforced by ArchUnit tests). | ||
|
|
Contributor
There was a problem hiding this comment.
Documentation describes completed refactoring as if it's pending.
The "Context" section states that PreflightMode and PreflightException "also live in Arius.Cli" and that BuildProductionServices "currently owns the entire chain" of preflight logic. However, the provided context snippets show the refactoring is already complete:
- Preflight.cs (context snippet 2):
PreflightMode,PreflightErrorKind, andPreflightExceptionalready exist insrc/Arius.AzureBlob/Preflight.cs, not inArius.Cli. - BlobServiceFactory.cs (context snippet 1):
BlobServiceFactory.CreateAsyncalready contains the full chain (client construction, preflight probe, error classification). - CliBuilder.cs (context snippet 3):
BuildProductionServicesalready delegates toBlobServiceFactory.CreateAsyncat lines 125-129.
This creates a timeline mismatch: the document reads like a pre-implementation design plan, but it's being added in the same PR that already implemented the changes. Future readers will be confused about whether this describes planned, in-progress, or completed work.
📝 Suggested fix: rewrite Context to describe existing architecture
## Context
-`CliBuilder.BuildProductionServices` (lines 147-264 of `CliBuilder.cs`) currently owns the entire chain from credential to validated `IBlobStorageService`: it builds a `BlobServiceClient`, obtains a `BlobContainerClient`, runs a preflight connectivity probe, classifies errors into `PreflightException`, and constructs `AzureBlobStorageService`. The `PreflightMode` enum and `PreflightException` class also live in `Arius.Cli`.
+As of PR `#37`, `BlobServiceFactory.CreateAsync` in `Arius.AzureBlob` encapsulates the full chain from credential to validated `IBlobStorageService`: it builds a `BlobServiceClient`, obtains a `BlobContainerClient`, runs a preflight connectivity probe based on `PreflightMode`, classifies errors into `PreflightException`, and constructs `AzureBlobStorageService`. The `PreflightMode` enum, `PreflightErrorKind` enum, and `PreflightException` class are defined in `Arius.AzureBlob/Preflight.cs`.
-A second (non-CLI) host is planned. Without extraction, that host must duplicate all of this logic. The `Arius.AzureBlob` project is the natural home: it already owns `AzureBlobStorageService` and has the `Azure.Storage.Blobs` dependency. `Arius.Core` must remain Azure-free (enforced by ArchUnit tests).
+This extraction enables planned second (non-CLI) hosts to reuse the factory without duplicating credential and preflight logic. The `Arius.AzureBlob` project is the natural home: it already owns `AzureBlobStorageService` and has the `Azure.Storage.Blobs` dependency. `Arius.Core` remains Azure-free (enforced by ArchUnit tests).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/archive/2026-04-01-blob-service-factory/design.md` around
lines 1 - 6, Update the "Context" paragraph to describe the refactor as
completed rather than planned: state that PreflightMode, PreflightErrorKind, and
PreflightException are defined in src/Arius.AzureBlob/Preflight.cs, and that
BlobServiceFactory.CreateAsync now encapsulates the client construction,
preflight probe and error classification; also note that
CliBuilder.BuildProductionServices delegates to BlobServiceFactory.CreateAsync
(lines referenced for reviewers only) so no duplication is required for the
non-CLI host. Ensure the wording clearly communicates the current architecture
and where each responsibility now lives.
Comment on lines
+12
to
+13
| - `PreflightException` changes from carrying a pre-formatted user-facing message to carrying structured fields (status code, account, container, auth mode) — each host formats its own error messages | ||
| - Add `Azure.Identity` package reference to `Arius.AzureBlob` (needed for `CredentialUnavailableException` catch) |
Contributor
There was a problem hiding this comment.
Tighten wording about PreflightException.Message.
On Line 12, the text says the exception changed from carrying a pre-formatted user-facing message to structured fields. In current code (src/Arius.AzureBlob/Preflight.cs), the exception still sets a preformatted base Message; the key change is that hosts no longer rely on it for UX formatting. Consider rewording to avoid ambiguity.
Suggested doc tweak
-- `PreflightException` changes from carrying a pre-formatted user-facing message to carrying structured fields (status code, account, container, auth mode) — each host formats its own error messages
+- `PreflightException` now exposes structured fields (status code, account, container, auth mode) as the primary contract, and hosts format their own user-facing messages instead of relying on exception text📝 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
| - `PreflightException` changes from carrying a pre-formatted user-facing message to carrying structured fields (status code, account, container, auth mode) — each host formats its own error messages | |
| - Add `Azure.Identity` package reference to `Arius.AzureBlob` (needed for `CredentialUnavailableException` catch) | |
| - `PreflightException` now exposes structured fields (status code, account, container, auth mode) as the primary contract, and hosts format their own user-facing messages instead of relying on exception text | |
| - Add `Azure.Identity` package reference to `Arius.AzureBlob` (needed for `CredentialUnavailableException` catch) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/archive/2026-04-01-blob-service-factory/proposal.md` around
lines 12 - 13, Reword the sentence about PreflightException.Message to clarify
that PreflightException still sets a base, preformatted Message in
src/Arius.AzureBlob/Preflight.cs, but the important change is that hosts no
longer rely on that Message for user-facing UX and instead use the structured
fields (status code, account, container, auth mode) to format errors; update the
line to explicitly state "exception retains a base Message but hosts must ignore
it for UX and format errors from structured fields" and keep the note about
adding Azure.Identity separate.
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Only Arius.AzureBlob may depend on Azure SDK namespaces | ||
| The architecture tests SHALL enforce that only the `Arius.AzureBlob` assembly may depend on types in Azure SDK namespaces. Specifically, `Arius.Core` and `Arius.Cli` SHALL NOT depend on types in `Azure.Storage`, `Azure.Identity`, `Azure.Core`, or any other `Azure.*` namespace. The existing `Core_Should_Not_Reference_Azure` test SHALL be widened from checking only `Azure.Storage` to checking all `Azure` namespaces. A new `Cli_Should_Not_Reference_Azure` test SHALL be added to enforce the same boundary for the CLI project. |
Contributor
There was a problem hiding this comment.
Spec text is stale about what tests are being introduced/expanded.
This line says Core is being widened from Azure.Storage and CLI test is new, but both are already implemented as ResideInNamespace("Azure") in src/Arius.Architecture.Tests/DependencyTests.cs (Lines 24–47). Please update this requirement to describe current enforcement rather than planned changes.
Suggested wording update
-The architecture tests SHALL enforce that only the `Arius.AzureBlob` assembly may depend on types in Azure SDK namespaces. Specifically, `Arius.Core` and `Arius.Cli` SHALL NOT depend on types in `Azure.Storage`, `Azure.Identity`, `Azure.Core`, or any other `Azure.*` namespace. The existing `Core_Should_Not_Reference_Azure` test SHALL be widened from checking only `Azure.Storage` to checking all `Azure` namespaces. A new `Cli_Should_Not_Reference_Azure` test SHALL be added to enforce the same boundary for the CLI project.
+The architecture tests SHALL enforce that only the `Arius.AzureBlob` assembly may depend on types in Azure SDK namespaces. Specifically, `Arius.Core` and `Arius.Cli` SHALL NOT depend on types in `Azure.Storage`, `Azure.Identity`, `Azure.Core`, or any other `Azure.*` namespace. This boundary is enforced by the existing `Core_Should_Not_Reference_Azure` and `Cli_Should_Not_Reference_Azure` tests, both checking `Azure` namespaces broadly.📝 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
| The architecture tests SHALL enforce that only the `Arius.AzureBlob` assembly may depend on types in Azure SDK namespaces. Specifically, `Arius.Core` and `Arius.Cli` SHALL NOT depend on types in `Azure.Storage`, `Azure.Identity`, `Azure.Core`, or any other `Azure.*` namespace. The existing `Core_Should_Not_Reference_Azure` test SHALL be widened from checking only `Azure.Storage` to checking all `Azure` namespaces. A new `Cli_Should_Not_Reference_Azure` test SHALL be added to enforce the same boundary for the CLI project. | |
| The architecture tests SHALL enforce that only the `Arius.AzureBlob` assembly may depend on types in Azure SDK namespaces. Specifically, `Arius.Core` and `Arius.Cli` SHALL NOT depend on types in `Azure.Storage`, `Azure.Identity`, `Azure.Core`, or any other `Azure.*` namespace. This boundary is enforced by the existing `Core_Should_Not_Reference_Azure` and `Cli_Should_Not_Reference_Azure` tests, both checking `Azure` namespaces broadly. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/archive/2026-04-01-blob-service-factory/specs/architecture-tests/spec.md`
at line 4, Update the spec text to reflect current tests: state that the
architecture tests enforce only the Arius.AzureBlob assembly may reference Azure
SDK namespaces and that both Core and Cli boundaries are already enforced by
tests in DependencyTests.cs using ResideInNamespace("Azure") (i.e., the existing
Core_Should_Not_Reference_Azure check was broadened and a
Cli_Should_Not_Reference_Azure test exists), replacing the stale wording about
planned expansion with this current enforcement description and referencing the
DependencyTests.cs test names (Core_Should_Not_Reference_Azure,
Cli_Should_Not_Reference_Azure) and the ResideInNamespace("Azure") assertion.
This was referenced Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AzureCliCredentialas a fallback authentication mechanism — users can now runarius archive/restore/lswithout an account key as long as they've runaz loginwith the appropriate RBAC roleExistsAsyncfor restore/ls) that translates auth/connectivity failures into clear, actionable error messages before the pipeline startstry/catch/finallyinProgram.csfor unhandled exceptions usingAnsiConsole.WriteException(formatted, no raw stack dumps) + guaranteedLog.CloseAndFlush()Credential resolution order
--key/-kCLI flag →StorageSharedKeyCredentialARIUS_KEYenv var →StorageSharedKeyCredentialdotnet user-secrets→StorageSharedKeyCredentialAzureCliCredentialviaaz loginChanges
CliBuilder.cs—PreflightModeenum,PreflightException, asyncBuildProductionServices, credential branching, preflight check with error translation, updated--keydescriptionArchiveVerb.cs/RestoreVerb.cs/LsVerb.cs— updated to async factory delegate, removed key-null guard, addedPreflightExceptioncatchProgram.cs— globaltry/catch/finallyArius.Cli.csproj/Directory.Packages.props— addAzure.Identity, remove deadFluentValidation+FluentResultsdependenciesCliTests.cs— updated test harness to new async factory signatureTest results
All tests pass: 137 CLI tests + 122 Core tests.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
--keyoption help text to indicate Azure CLI login as an alternativeChores