test: build representative end-to-end workflow coverage#68
Open
woutervanranst wants to merge 140 commits intomasterfrom
Open
test: build representative end-to-end workflow coverage#68woutervanranst wants to merge 140 commits intomasterfrom
woutervanranst wants to merge 140 commits intomasterfrom
Conversation
…/Arius7 into improve-e2e-tests
Rewrite ArchiveTierLifecycleStep around one representative tar-backed file instead of rebuilding all tar chunks under a broad subtree. Preserve the real readable chunk blob, move that one chunk to archive tier, verify the pending rehydration prompt, then restore successfully from the staged ready blob and validate cleanup.
Contributor
📝 WalkthroughWalkthroughCreates a shared test-support library and refactors integration/E2E fixtures into a representative workflow framework; introduces deterministic synthetic repository modeling and workflow steps; moves test dependencies into a shared project; removes Testcontainers-based Linux-runner detection; updates docs and minor core formatting/logic (hash/encryption, multi-file restore). Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Runner as RepresentativeWorkflowRunner
participant Backend as IE2EStorageBackend
participant Context as E2EStorageBackendContext
participant Fixture as E2EFixture
participant Step as Workflow Step
Test->>Runner: RunAsync(backend, workflow)
Runner->>Backend: InitializeAsync()
Runner->>Backend: CreateContextAsync()
Backend-->>Runner: Context
Runner->>Runner: create workflowRoot, state
Runner->>Fixture: CreateFixtureAsync(Context, workflowRoot)
Fixture-->>Runner: E2EFixture
loop for each Step in workflow
Runner->>Step: ExecuteAsync(state)
alt Materialize
Step->>Fixture: MaterializeV1/V2(...)
else Archive
Step->>Fixture: CreateArchiveHandler() / Archive(...)
else Restore
Step->>Fixture: CreateRestoreHandler() / Restore(...)
else Assert
Step->>Fixture: AssertRestoreOutcome(...)
end
Step-->>Runner: completed
end
Runner->>Fixture: DisposeAsync()
Runner->>Context: CleanupAsync()
Runner-->>Test: RepresentativeWorkflowRunResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
woutervanranst
changed the title
Owner
Author
|
@CodeRabbit start the review |
Contributor
|
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68 +/- ##
==========================================
+ Coverage 78.65% 78.76% +0.10%
==========================================
Files 66 69 +3
Lines 4840 4972 +132
Branches 655 669 +14
==========================================
+ Hits 3807 3916 +109
- Misses 877 902 +25
+ Partials 156 154 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Arius.E2E.Tests/E2ETests.cs (1)
23-35:⚠️ Potential issue | 🟠 MajorSkip this live sanity test when Azure credentials are absent.
Unlike the other two live tests, this one still hard-fails on runners without
ARIUS_E2E_*configured. That makes the suite non-self-skipping in the exact environment the file comment says should be skipped.Suggested fix
public async Task E2E_Configuration_IsAvailable_WhenAzureBackendIsEnabled() { + if (!AzureFixture.IsAvailable) + { + Skip.Unless(false, "Azure credentials not available — skipping live Azure configuration sanity test"); + return; + } + AzureFixture.AccountName.ShouldNotBeNullOrWhiteSpace(); AzureFixture.AccountKey.ShouldNotBeNullOrWhiteSpace();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/E2ETests.cs` around lines 23 - 35, The test E2E_Configuration_IsAvailable_WhenAzureBackendIsEnabled currently hard-fails when ARIUS_E2E_* credentials are missing; change it to skip early instead of asserting. Replace the direct ShouldNotBeNullOrWhiteSpace checks on AzureFixture.AccountName and AzureFixture.AccountKey in the test with a guard that returns (or marks the test skipped) when either value is null/empty/whitespace before calling azure.CreateTestContainerAsync; keep the rest of the test (container creation, ExistsAsync check, cleanup) unchanged and reference the AzureFixture and azure.CreateTestContainerAsync symbols to locate where to add the guard.
🧹 Nitpick comments (11)
src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs (1)
36-36: Optional cleanup: reusecontentHashfortarHashin this single-content test.Line 36 recomputes the same hash value already calculated on Line 34.
♻️ Minimal diff
- var tarHash = Convert.ToHexString(env.Encryption.ComputeHash(content)).ToLowerInvariant(); + var tarHash = contentHash;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs` at line 36, The test recomputes the same hash for the tar content; replace the redundant ComputeHash call by reusing the existing variable contentHash for tarHash (i.e., remove the Convert.ToHexString(env.Encryption.ComputeHash(content)).ToLowerInvariant() expression and assign tarHash = contentHash) so the single-content test uses the precomputed contentHash variable instead; locate the variables contentHash and tarHash in ArchiveRecoveryTests.cs to make the change.opencode.json (1)
1-4: Remember to restart OpenCode after this config change.This ensures the plugin is actually reloaded in local dev environments/CI agents where applicable.
As per coding guidelines, "Project-level OpenCode configuration lives in
opencode.json. Restart OpenCode after config changes so the plugin is reloaded."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@opencode.json` around lines 1 - 4, Update the project-level OpenCode configuration in opencode.json (the "plugin" entry referencing "superpowers@git+https://github.com/obra/superpowers.git") and then restart the OpenCode process/agent so the plugin is reloaded in local dev and CI environments; ensure you mention this restart in the PR description or checklist so reviewers/CI operators know to restart OpenCode after the config change.src/Arius.E2E.Tests/Datasets/SyntheticFileDefinition.cs (2)
9-10: Consider supporting zero-byte files.The validation rejects
SizeBytes <= 0, but real repositories can contain empty files (0 bytes). If the synthetic dataset needs to model such files for edge-case coverage, this constraint would need adjustment.If empty files are intentionally excluded from the synthetic dataset scope, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Datasets/SyntheticFileDefinition.cs` around lines 9 - 10, The validation in SyntheticFileDefinition rejects zero-byte files by checking SizeBytes <= 0; to support empty files change the check to SizeBytes < 0 so only negative sizes are invalid, and update the ArgumentOutOfRangeException message (and any related tests) to say "File size must be non-negative" or similar; locate the SizeBytes validation inside the SyntheticFileDefinition constructor/initializer to make this change.
12-13: Redundant null check beforeThrowIfNullOrWhiteSpace.The conditional
if (ContentId is not null)followed byThrowIfNullOrWhiteSpace(ContentId)is slightly redundant—ifContentIdis not null, the only failure case is whitespace-only strings. This works correctly but could be simplified.🧹 Simplified validation
- if (ContentId is not null) - ArgumentException.ThrowIfNullOrWhiteSpace(ContentId); + if (ContentId is not null && string.IsNullOrWhiteSpace(ContentId)) + throw new ArgumentException("ContentId cannot be whitespace.", nameof(ContentId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Datasets/SyntheticFileDefinition.cs` around lines 12 - 13, Remove the redundant null check and call ArgumentException.ThrowIfNullOrWhiteSpace directly on ContentId in SyntheticFileDefinition: replace the "if (ContentId is not null) ArgumentException.ThrowIfNullOrWhiteSpace(ContentId);" pattern with a single ArgumentException.ThrowIfNullOrWhiteSpace(ContentId) so validation is simplified while still catching null or whitespace; update the method/container where ContentId is validated accordingly.src/Arius.Integration.Tests/Arius.Integration.Tests.csproj (1)
15-19: Minor formatting inconsistency on Line 15.There's an extra leading space before the
<ItemGroup>tag that doesn't match the indentation style of other ItemGroup elements in this file.🧹 Suggested fix
- <ItemGroup> + <ItemGroup> <ProjectReference Include="..\Arius.Tests.Shared\Arius.Tests.Shared.csproj" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Integration.Tests/Arius.Integration.Tests.csproj` around lines 15 - 19, The opening `<ItemGroup>` tag on line 15 has an extra leading space that doesn't match the indentation pattern of other ItemGroup elements in the file. Remove the extra space before the `<ItemGroup>` tag to align it with the consistent indentation style used throughout the project file.src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs (1)
70-87: UnusedcancellationTokenparameters inCreateAsyncmethods.Both
CreateAsyncoverloads accept aCancellationTokenparameter that is never used. Either remove the parameter or pass it to any async operations if they support cancellation.Option: Remove unused parameter
public static Task<RepositoryTestFixture> CreateAsync( IBlobContainerService blobContainer, string accountName, string containerName, string? passphrase = null, string? tempRoot = null, - Action<string>? deleteTempRoot = null, - CancellationToken cancellationToken = default) + Action<string>? deleteTempRoot = null)Also applies to: 89-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs` around lines 70 - 87, The CreateAsync overloads in RepositoryTestFixture accept a CancellationToken that is never used; remove the unused cancellationToken parameter from both CreateAsync method signatures (the one creating PassphraseEncryptionService/ChunkIndexService/etc. and the other overload at lines ~89-105) and update all callers/tests to stop passing a CancellationToken, or alternatively if you intend to support cancellation change the method to perform true async work and pass the token into awaited calls—specifically edit the RepositoryTestFixture.CreateAsync methods to either drop CancellationToken or thread it into any async factory calls and Task.FromResult should be replaced with proper async/await so the token can be honored.src/Arius.E2E.Tests/RepresentativeArchiveRestoreTests.cs (1)
27-33: Strengthen the non-archive-tier assertion path.When
SupportsArchiveTieris false, add an explicit assertion thatArchiveTierOutcomeis absent to tighten backend-capability validation.Suggested assertion addition
if (backend.Capabilities.SupportsArchiveTier) { result.ArchiveTierOutcome.ShouldNotBeNull(); result.ArchiveTierOutcome.PendingRehydratedBlobCount.ShouldBeGreaterThan(0); result.ArchiveTierOutcome.WasCostEstimateCaptured.ShouldBeTrue(); result.ArchiveTierOutcome.RerunCopyCalls.ShouldBe(0); } + else + { + result.ArchiveTierOutcome.ShouldBeNull(); + }As per coding guidelines:
**/*.Tests/**: “Focus on test coverage gaps and assertion quality rather than style.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/RepresentativeArchiveRestoreTests.cs` around lines 27 - 33, When backend.Capabilities.SupportsArchiveTier is false, explicitly assert that result.ArchiveTierOutcome is null to ensure the test validates the absence of archive-tier data; update the test around the SupportsArchiveTier branch (check the backend.Capabilities.SupportsArchiveTier conditional and the result.ArchiveTierOutcome checks) to add a failing assertion like Assert.Null(result.ArchiveTierOutcome) / result.ArchiveTierOutcome.ShouldBeNull() in the else path so the test fails if an unexpected ArchiveTierOutcome is present.docs/superpowers/plans/2026-04-20-shared-test-infrastructure-refactor.md (1)
13-13: Minor: Markdown heading level jumps from h1 to h3.The markdownlint hint is valid—headings should increment by one level at a time. Task headings use
###(h3) but there's no h2 section header above them.This is a documentation style nit and doesn't affect the plan's clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-20-shared-test-infrastructure-refactor.md` at line 13, The heading "### Task 1: Create Shared Test Library" jumps from h1 to h3; change it so heading levels increment by one (either make this line "## Task 1: Create Shared Test Library" or insert an appropriate h2 section header immediately above the "### Task 1: Create Shared Test Library" heading) to satisfy markdownlint; locate the exact string "### Task 1: Create Shared Test Library" in the document and apply the level fix.src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs (1)
7-13: SplitRepresentativeWorkflowRunnerDependenciesinto its own file.Keeping the dependency wrapper as a second top-level type in
RepresentativeWorkflowRunner.csmakes the runner surface harder to navigate.As per coding guidelines "
**/*.cs: Prefer one top-level class per file, with the filename matching the class name."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs` around lines 7 - 13, Move the internal sealed type RepresentativeWorkflowRunnerDependencies out of RepresentativeWorkflowRunner.cs into its own file named RepresentativeWorkflowRunnerDependencies.cs: create the new file with the same namespace, declare internal sealed class RepresentativeWorkflowRunnerDependencies, preserve the public Func<E2EStorageBackendContext, string, CancellationToken, Task<E2EFixture>> CreateFixtureAsync property and its initializer that references RepresentativeWorkflowRunner.CreateFixtureAsync, add any required using directives (for E2EStorageBackendContext, E2EFixture, CancellationToken, Task), and remove the duplicate top-level type from RepresentativeWorkflowRunner.cs so the runner file contains only RepresentativeWorkflowRunner; ensure the project compiles after the move.src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunResult.cs (1)
8-17: MoveArchiveTierWorkflowOutcomeinto its own file.This file now contains two top-level types, so the outcome record is harder to find and no longer matches the filename.
As per coding guidelines "
**/*.cs: Prefer one top-level class per file, with the filename matching the class name."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunResult.cs` around lines 8 - 17, Move the ArchiveTierWorkflowOutcome record out of RepresentativeWorkflowRunResult.cs into its own file named ArchiveTierWorkflowOutcome.cs and leave only the RepresentativeWorkflowRunResult type in the original file; in the new file declare the same internal sealed record ArchiveTierWorkflowOutcome(...) with the identical parameter list and place it in the same namespace as the original file so accessibility and references remain unchanged, then remove the record declaration from RepresentativeWorkflowRunResult.cs so each top-level type lives in a file matching its name.src/Arius.E2E.Tests/E2ETests.cs (1)
38-105: Move these live restore/streaming probes out ofE2ETests.cs.These are behavior-coverage tests now, not credential/configuration sanity checks. Keeping them here drifts this file back into owning representative coverage instead of leaving that to the representative suites.
As per coding guidelines,
src/Arius.E2E.Tests/E2ETests.csis now only the live Azure credential/configuration sanity check. Representative suites own archive and restore behavior coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/E2ETests.cs` around lines 38 - 105, The two live behavior tests E2E_HotTier_Restore_CreatesPointerFiles_ByDefault and E2E_LargeFile_Streaming_RemainsCovered should be removed from E2ETests.cs and relocated into the representative archive/restore test suite: create a new test class (e.g., ArchiveRestoreBehaviorTests) or add them to the existing representative suite, preserve their signatures, attributes (e.g., [Test], [Timeout], CancellationToken parameter) and any setup/teardown usage (azure.CreateTestContainerAsync, E2EFixture.CreateAsync, DisposeAsync/cleanup), and update namespaces/usings so they compile; leave E2ETests.cs containing only live Azure credential/configuration sanity 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 `@opencode.json`:
- Line 3: The plugin entry
"superpowers@git+https://github.com/obra/superpowers.git" is unpinned and should
be fixed by replacing it with a URL pinned to a commit SHA (append
#<commit-sha>), e.g. change
"superpowers@git+https://github.com/obra/superpowers.git" to
"superpowers@git+https://github.com/obra/superpowers.git#<commit-sha>" using the
exact commit SHA you want to lock to; after updating the plugin string restart
OpenCode to apply the deterministic dependency resolution.
In `@src/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinition.cs`:
- Around line 66-84: For SyntheticFileMutationKind.Rename handling, validate and
normalize mutation.TargetPath before any root-prefix or containment checks:
ensure mutation.TargetPath is non-null (avoid the null-forgiving !) then call
SyntheticRepositoryPath.NormalizeRelativePath on it and use that
normalizedTarget for subsequent checks (IsUnderDeclaredRoot,
rootDirectorySet.Contains, v1Paths.Contains, finalPaths.Add and duplicate
detection) so inputs like "src/../escape.bin" are resolved correctly and errors
remain clear and consistent with other validations for V2Mutations.
- Around line 130-155: The NormalizeRelativePath method allows Windows-style
absolute paths like "C:\..." to slip through on non-Windows platforms after
normalization; after the existing normalization into the variable normalized
(i.e., after path.Replace('\\','/')), add a check that rejects strings that
start with a drive-letter pattern (e.g., a single ASCII letter followed by ":"
and "/"), and throw the same ArgumentException with paramName when matched;
implement this using a simple pattern match (Regex.IsMatch(normalized,
@"^[A-Za-z]:\/") or equivalent char checks) to detect and reject Windows
absolute paths before continuing the other validations.
In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs`:
- Around line 87-100: ResetLocalCacheAsync currently deletes the on-disk cache
unconditionally, which breaks coordination with the RepositoryCacheLeases;
change it to first consult the lease table (e.g., RepositoryCacheLeases) for the
given accountName/containerName and skip (or defer) deletion if there are any
active leases for that pair. Locate ResetLocalCacheAsync and before
Directory.Delete, call the lease-check API (e.g.,
RepositoryCacheLeases.HasActiveLease or equivalent) and only perform
Directory.Delete when no active leases exist; ensure the check is done
atomically with respect to any lease state transitions your lease API provides
(or retry/wait) so you don't remove a cache while a live fixture still holds a
lease.
In `@src/Arius.E2E.Tests/RepresentativeArchiveRestoreTests.cs`:
- Around line 14-23: The Azure cold-cache skip was removed; update the Azure
guard in RepresentativeArchiveRestoreTests so it actually skips the canonical
Azure run until `#65` is fixed: in the existing if block around
AzureE2EBackendFixture/AzureFixture, change the logic to detect when Azure
credentials are available (backend is AzureE2EBackendFixture &&
AzureFixture.IsAvailable) and invoke the test skip with the issue reference
(e.g., Skip.If(true, "Temporarily skip Azure cold-cache canonical restore — see
`#65`")) before returning so the call to RepresentativeWorkflowRunner.RunAsync is
not executed on live Azure.
In `@src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs`:
- Around line 42-46: workflowRoot is created before calling
dependencies.CreateFixtureAsync and if that call throws the temp directory is
leaked; wrap the fixture creation in a try/finally so the directory is removed
on error: declare RepresentativeWorkflowState? state = null and the fixture
variable before the try, create the workflowRoot and
Directory.CreateDirectory(workflowRoot) then call CreateFixtureAsync inside a
try block, and in the finally check if fixture (or state) is still null (meaning
creation failed) and delete Directory.Delete(workflowRoot, true) to ensure
cleanup; keep the rest of the existing disposal/cleanup logic for the successful
path.
In `@src/Arius.E2E.Tests/Workflows/Steps/AssertRemoteStateStep.cs`:
- Around line 23-24: The current use of the index operator on the result of
Path.GetFileName((await
state.Fixture.Snapshot.ListBlobNamesAsync(cancellationToken))[^1]) can throw
IndexOutOfRangeException when the returned list is empty; change the logic to
safely get the last blob name (e.g., use LastOrDefault() or check Count/Any on
the IList/array returned by state.Fixture.Snapshot.ListBlobNamesAsync) and then
guard against null/empty before calling Path.GetFileName and asserting against
state.LatestSnapshotVersion, including a clear assertion message that uses Name
when the list is empty.
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Around line 143-149: Dispose the test fixture's Index (ChunkIndexService)
before removing the temp directory: in DisposeAsync, if the fixture has a
field/property named Index of type ChunkIndexService (which implements
IDisposable), call Index?.Dispose() (or DisposeAsync if it implements
IAsyncDisposable) prior to invoking _deleteTempRoot(_tempRoot); keep the null
check and then return ValueTask.CompletedTask so resources held by
ChunkIndexService are released before the directory is removed.
---
Outside diff comments:
In `@src/Arius.E2E.Tests/E2ETests.cs`:
- Around line 23-35: The test
E2E_Configuration_IsAvailable_WhenAzureBackendIsEnabled currently hard-fails
when ARIUS_E2E_* credentials are missing; change it to skip early instead of
asserting. Replace the direct ShouldNotBeNullOrWhiteSpace checks on
AzureFixture.AccountName and AzureFixture.AccountKey in the test with a guard
that returns (or marks the test skipped) when either value is
null/empty/whitespace before calling azure.CreateTestContainerAsync; keep the
rest of the test (container creation, ExistsAsync check, cleanup) unchanged and
reference the AzureFixture and azure.CreateTestContainerAsync symbols to locate
where to add the guard.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-20-shared-test-infrastructure-refactor.md`:
- Line 13: The heading "### Task 1: Create Shared Test Library" jumps from h1 to
h3; change it so heading levels increment by one (either make this line "## Task
1: Create Shared Test Library" or insert an appropriate h2 section header
immediately above the "### Task 1: Create Shared Test Library" heading) to
satisfy markdownlint; locate the exact string "### Task 1: Create Shared Test
Library" in the document and apply the level fix.
In `@opencode.json`:
- Around line 1-4: Update the project-level OpenCode configuration in
opencode.json (the "plugin" entry referencing
"superpowers@git+https://github.com/obra/superpowers.git") and then restart the
OpenCode process/agent so the plugin is reloaded in local dev and CI
environments; ensure you mention this restart in the PR description or checklist
so reviewers/CI operators know to restart OpenCode after the config change.
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs`:
- Line 36: The test recomputes the same hash for the tar content; replace the
redundant ComputeHash call by reusing the existing variable contentHash for
tarHash (i.e., remove the
Convert.ToHexString(env.Encryption.ComputeHash(content)).ToLowerInvariant()
expression and assign tarHash = contentHash) so the single-content test uses the
precomputed contentHash variable instead; locate the variables contentHash and
tarHash in ArchiveRecoveryTests.cs to make the change.
In `@src/Arius.E2E.Tests/Datasets/SyntheticFileDefinition.cs`:
- Around line 9-10: The validation in SyntheticFileDefinition rejects zero-byte
files by checking SizeBytes <= 0; to support empty files change the check to
SizeBytes < 0 so only negative sizes are invalid, and update the
ArgumentOutOfRangeException message (and any related tests) to say "File size
must be non-negative" or similar; locate the SizeBytes validation inside the
SyntheticFileDefinition constructor/initializer to make this change.
- Around line 12-13: Remove the redundant null check and call
ArgumentException.ThrowIfNullOrWhiteSpace directly on ContentId in
SyntheticFileDefinition: replace the "if (ContentId is not null)
ArgumentException.ThrowIfNullOrWhiteSpace(ContentId);" pattern with a single
ArgumentException.ThrowIfNullOrWhiteSpace(ContentId) so validation is simplified
while still catching null or whitespace; update the method/container where
ContentId is validated accordingly.
In `@src/Arius.E2E.Tests/E2ETests.cs`:
- Around line 38-105: The two live behavior tests
E2E_HotTier_Restore_CreatesPointerFiles_ByDefault and
E2E_LargeFile_Streaming_RemainsCovered should be removed from E2ETests.cs and
relocated into the representative archive/restore test suite: create a new test
class (e.g., ArchiveRestoreBehaviorTests) or add them to the existing
representative suite, preserve their signatures, attributes (e.g., [Test],
[Timeout], CancellationToken parameter) and any setup/teardown usage
(azure.CreateTestContainerAsync, E2EFixture.CreateAsync, DisposeAsync/cleanup),
and update namespaces/usings so they compile; leave E2ETests.cs containing only
live Azure credential/configuration sanity checks.
In `@src/Arius.E2E.Tests/RepresentativeArchiveRestoreTests.cs`:
- Around line 27-33: When backend.Capabilities.SupportsArchiveTier is false,
explicitly assert that result.ArchiveTierOutcome is null to ensure the test
validates the absence of archive-tier data; update the test around the
SupportsArchiveTier branch (check the backend.Capabilities.SupportsArchiveTier
conditional and the result.ArchiveTierOutcome checks) to add a failing assertion
like Assert.Null(result.ArchiveTierOutcome) /
result.ArchiveTierOutcome.ShouldBeNull() in the else path so the test fails if
an unexpected ArchiveTierOutcome is present.
In `@src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs`:
- Around line 7-13: Move the internal sealed type
RepresentativeWorkflowRunnerDependencies out of RepresentativeWorkflowRunner.cs
into its own file named RepresentativeWorkflowRunnerDependencies.cs: create the
new file with the same namespace, declare internal sealed class
RepresentativeWorkflowRunnerDependencies, preserve the public
Func<E2EStorageBackendContext, string, CancellationToken, Task<E2EFixture>>
CreateFixtureAsync property and its initializer that references
RepresentativeWorkflowRunner.CreateFixtureAsync, add any required using
directives (for E2EStorageBackendContext, E2EFixture, CancellationToken, Task),
and remove the duplicate top-level type from RepresentativeWorkflowRunner.cs so
the runner file contains only RepresentativeWorkflowRunner; ensure the project
compiles after the move.
In `@src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunResult.cs`:
- Around line 8-17: Move the ArchiveTierWorkflowOutcome record out of
RepresentativeWorkflowRunResult.cs into its own file named
ArchiveTierWorkflowOutcome.cs and leave only the RepresentativeWorkflowRunResult
type in the original file; in the new file declare the same internal sealed
record ArchiveTierWorkflowOutcome(...) with the identical parameter list and
place it in the same namespace as the original file so accessibility and
references remain unchanged, then remove the record declaration from
RepresentativeWorkflowRunResult.cs so each top-level type lives in a file
matching its name.
In `@src/Arius.Integration.Tests/Arius.Integration.Tests.csproj`:
- Around line 15-19: The opening `<ItemGroup>` tag on line 15 has an extra
leading space that doesn't match the indentation pattern of other ItemGroup
elements in the file. Remove the extra space before the `<ItemGroup>` tag to
align it with the consistent indentation style used throughout the project file.
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Around line 70-87: The CreateAsync overloads in RepositoryTestFixture accept a
CancellationToken that is never used; remove the unused cancellationToken
parameter from both CreateAsync method signatures (the one creating
PassphraseEncryptionService/ChunkIndexService/etc. and the other overload at
lines ~89-105) and update all callers/tests to stop passing a CancellationToken,
or alternatively if you intend to support cancellation change the method to
perform true async work and pass the token into awaited calls—specifically edit
the RepositoryTestFixture.CreateAsync methods to either drop CancellationToken
or thread it into any async factory calls and Task.FromResult should be replaced
with proper async/await so the token can be honored.
🪄 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: 54fc6df9-4770-4035-b7cb-1ef2a5aa6ab3
📒 Files selected for processing (83)
.github/scripts/Get-DotNetProjectMatrix.ps1AGENTS.mdREADME.mddocs/superpowers/plans/2026-04-19-representative-e2e-suite.mddocs/superpowers/plans/2026-04-20-shared-test-infrastructure-refactor.mddocs/superpowers/plans/2026-04-23-representative-workflow-refactor.mddocs/superpowers/plans/2026-04-24-workflow-step-boundaries.mddocs/superpowers/plans/2026-04-25-archive-tier-step-simplification.mddocs/superpowers/specs/2026-04-20-shared-test-infrastructure-design.mddocs/superpowers/specs/2026-04-23-representative-workflow-design.mdopencode.jsonsrc/Arius.AzureBlob/AzureBlobContainerService.cssrc/Arius.Cli/Arius.Cli.csprojsrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core/Arius.Core.csprojsrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/RestoreCommand/RestoreCommandHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.E2E.Tests/Arius.E2E.Tests.csprojsrc/Arius.E2E.Tests/Datasets/SyntheticFileDefinition.cssrc/Arius.E2E.Tests/Datasets/SyntheticFileMutation.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinition.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinitionFactory.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryMaterializer.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryState.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryStateAssertions.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryVersion.cssrc/Arius.E2E.Tests/E2EFixturePathTests.cssrc/Arius.E2E.Tests/E2ETests.cssrc/Arius.E2E.Tests/Fixtures/AzureFixture.cssrc/Arius.E2E.Tests/Fixtures/AzuriteE2EBackendFixture.cssrc/Arius.E2E.Tests/Fixtures/E2EBackendCapabilities.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.E2E.Tests/Fixtures/E2EStorageBackendContext.cssrc/Arius.E2E.Tests/Fixtures/IE2EStorageBackend.cssrc/Arius.E2E.Tests/RehydrationE2ETests.cssrc/Arius.E2E.Tests/RepresentativeArchiveRestoreTests.cssrc/Arius.E2E.Tests/Services/CopyTrackingBlobService.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowCatalog.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowDefinition.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunResult.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowState.cssrc/Arius.E2E.Tests/Workflows/Steps/ArchiveStep.cssrc/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cssrc/Arius.E2E.Tests/Workflows/Steps/AssertConflictBehaviorStep.cssrc/Arius.E2E.Tests/Workflows/Steps/AssertRemoteStateStep.cssrc/Arius.E2E.Tests/Workflows/Steps/Helpers.cssrc/Arius.E2E.Tests/Workflows/Steps/IRepresentativeWorkflowStep.cssrc/Arius.E2E.Tests/Workflows/Steps/MaterializeVersionStep.cssrc/Arius.E2E.Tests/Workflows/Steps/ResetCacheStep.cssrc/Arius.E2E.Tests/Workflows/Steps/RestoreStep.cssrc/Arius.Explorer.Tests/Arius.Explorer.Tests.csprojsrc/Arius.Explorer/Arius.Explorer.csprojsrc/Arius.Integration.Tests/Arius.Integration.Tests.csprojsrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/ContainerCreationTests.cssrc/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cssrc/Arius.Integration.Tests/Pipeline/GcmIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/ListQueryIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Integration.Tests/Pipeline/RecoveryScriptTests.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreCostModelTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreDispositionTests.cssrc/Arius.Integration.Tests/Pipeline/RestorePointerTimestampTests.cssrc/Arius.Integration.Tests/Pipeline/RoundtripTests.cssrc/Arius.Integration.Tests/Shared/FileTree/FileTreeBuilderIntegrationTests.cssrc/Arius.Integration.Tests/Snapshot/SnapshotServiceIntegrationTests.cssrc/Arius.Integration.Tests/Storage/AzuriteFixture.cssrc/Arius.Integration.Tests/Storage/AzuriteFixtureTests.cssrc/Arius.Integration.Tests/Storage/BlobStorageServiceTests.cssrc/Arius.Tests.Shared/Arius.Tests.Shared.csprojsrc/Arius.Tests.Shared/AssemblyMarker.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cssrc/Arius.Tests.Shared/IO/FileSystemHelper.cssrc/Arius.Tests.Shared/Storage/AzuriteFixture.cssrc/Arius.slnxsrc/Directory.Packages.props
💤 Files with no reviewable changes (8)
- src/Arius.Core/Arius.Core.csproj
- src/Arius.Explorer/Arius.Explorer.csproj
- .github/scripts/Get-DotNetProjectMatrix.ps1
- src/Arius.E2E.Tests/Services/CopyTrackingBlobService.cs
- src/Arius.E2E.Tests/RehydrationE2ETests.cs
- src/Arius.Integration.Tests/Storage/AzuriteFixture.cs
- src/Arius.E2E.Tests/E2EFixturePathTests.cs
- src/Arius.Cli/Arius.Cli.csproj
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (7)
README.md (4)
31-33: Minor: Clarify the OpenCode “restart” requirement.“Restart OpenCode after pulling the workspace” is directionally fine, but it’s a bit vague. Consider tightening it to exactly what triggers the need to restart (e.g., “after
opencode.jsonchanges” / “after plugin config changes”) so users don’t assume every git pull requires a restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 31 - 33, The README's instruction to "Restart OpenCode after pulling the workspace" is ambiguous; update the sentence to specify that a restart is only required when plugin configuration files change (e.g., opencode.json) so users don't restart on every git pull—reference the opencode.json file and the superpowers plugin and change the text to something like "Restart OpenCode after pulling the workspace if opencode.json (plugin configuration) has changed so the superpowers plugin and other plugin skills will load."
103-106: Doc clarity: differentiate CLIARIUS_KEYvs E2EARIUS_E2E_KEY.The README has an “Account key” section using
ARIUS_KEYand a specific user-secrets key pattern, while the “Setup” section for tests usesARIUS_E2E_ACCOUNT/ARIUS_E2E_KEYwith asrc/Arius.E2E.Testsuser-secrets scope. If these are intentionally separate, consider adding a one-liner that they’re for different flows (CLI vs E2E harness) to prevent misconfiguration by copy/paste.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 103 - 106, Clarify that ARIUS_KEY is for interactive/CLI usage while ARIUS_E2E_ACCOUNT and ARIUS_E2E_KEY are for the end-to-end test harness: add a one-line note under "Account key" stating that ARIUS_KEY (or the dotnet user-secrets key arius:<account>:key) is used by the CLI/runtime, whereas ARIUS_E2E_ACCOUNT/ARIUS_E2E_KEY (and the src/Arius.E2E.Tests user-secrets scope) are only for E2E tests to avoid copy/paste misconfiguration.
137-145: Optional: Consider adding the “capability-gated” phrasing where it matters most.You already mention live Azure being credential-gated and representative behavior being shared. If the archive-tier steps are capability-gated (as per the ADR), consider a short explicit callout in this bullet list for faster mapping between README and the workflow behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 137 - 145, Add an explicit capability-gated callout to the README bullet list: state that archive-tier steps in the canonical representative workflow (referenced by RepresentativeArchiveRestoreTests.cs) are gated by backend capabilities per the ADR, so archive-tier pending-versus-ready behavior only runs when the backend advertises support; keep the rest of the bullets as-is and ensure SyntheticRepositoryDefinitionFactory and E2ETests.cs are still mentioned for repository sizing and live-Azure probes respectively.
121-124: Minor: Strengthen wording of the Azurite “skipped when Docker is unavailable” explanation.LanguageTool suggests changing the verb around the sentence that includes “so the test report shows …”. For example, “indicates”/“highlights” tends to read more precise for CI output semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 121 - 124, Update the README sentence that currently reads "Azurite-backed integration and E2E tests report as skipped when Docker is unavailable, so the test report shows that the local emulator coverage was intentionally not run." to use a stronger, more precise verb (e.g., "indicates" or "highlights") and tighten the phrasing; locate the sentence containing "Azurite-backed integration and E2E tests report as skipped when Docker is unavailable" and replace the trailing clause with something like "indicating that local emulator coverage was intentionally not run" or "highlighting that the local emulator coverage was intentionally skipped" to improve CI-output clarity.docs/decisions/adr-0001-structure-representative-e2e-coverage.md (3)
39-41: Optional: Ensure the “two-pass pending vs ready archive-tier lifecycle” claim matches the real step model.If the archive-tier lifecycle is implemented as “pending restore with staged rehydration blobs” then “ready restore with cleanup”, it may be worth double-checking this maps 1:1 to the actual representative workflow step sequence/names (so future readers don’t have to infer it).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/decisions/adr-0001-structure-representative-e2e-coverage.md` around lines 39 - 41, Update the ADR text so the "two-pass pending vs ready archive-tier lifecycle" claim is precisely aligned with the real workflow step names and sequencing: verify the implementation's exact step identifiers and replace or augment the phrases "pending restore with staged rehydration blobs" and "ready restore with cleanup" with the canonical step names (or add parenthetical mapping) so there is a 1:1 mapping between the ADR wording and the representative workflow step sequence/names; ensure the sentence in the list that currently contains those phrases clearly states the exact step names used in the codebase or references them explicitly.
84-90: Optional: Keep “removed as part of this cleanup” verifiable.The ADR states intermediate plan docs under
docs/superpowers/plans/were removed. If that was literal deletion, consider leaving a pointer to the superseded plans (or their former filenames) to make the decision history auditable later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/decisions/adr-0001-structure-representative-e2e-coverage.md` around lines 84 - 90, Update the ADR text to make the removal of intermediate implementation plans verifiable by adding a brief pointer to the removed filenames or a note where their prior content can be retrieved; specifically modify the paragraph that references "docs/superpowers/plans/" in adr-0001-structure-representative-e2e-coverage.md to list the former filenames (or state they are archived in git history with commit hashes or a pointer to the branch/tag) so the decision history is auditable.
35-40: Minor: Reduce “Good, because …” repetition for readability/tooling noise.The “Consequences” list repeats the same sentence opening (“Good, because …”) multiple times in a row. Consider varying the phrasing (or merging bullets) to improve scanability and reduce LanguageTool-style warnings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/decisions/adr-0001-structure-representative-e2e-coverage.md` around lines 35 - 40, Edit the "Consequences" bullet list in adr-0001-structure-representative-e2e-coverage.md to remove repetitive "Good, because …" openings: either merge related bullets or rephrase each line to vary the lead (e.g., "Reusable wiring: …", "Models evolving V1→V2 history: …", "Deterministic dataset: …", "Includes remote-state checks: …", "Simplified archive-tier lifecycle: …"), keeping the same content and references to Arius.Tests.Shared, Arius.Integration.Tests, Arius.E2E.Tests and SyntheticRepositoryDefinitionFactory, so the meaning is unchanged but scanability and tooling warnings are reduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/decisions/adr-0001-structure-representative-e2e-coverage.md`:
- Around line 39-41: Update the ADR text so the "two-pass pending vs ready
archive-tier lifecycle" claim is precisely aligned with the real workflow step
names and sequencing: verify the implementation's exact step identifiers and
replace or augment the phrases "pending restore with staged rehydration blobs"
and "ready restore with cleanup" with the canonical step names (or add
parenthetical mapping) so there is a 1:1 mapping between the ADR wording and the
representative workflow step sequence/names; ensure the sentence in the list
that currently contains those phrases clearly states the exact step names used
in the codebase or references them explicitly.
- Around line 84-90: Update the ADR text to make the removal of intermediate
implementation plans verifiable by adding a brief pointer to the removed
filenames or a note where their prior content can be retrieved; specifically
modify the paragraph that references "docs/superpowers/plans/" in
adr-0001-structure-representative-e2e-coverage.md to list the former filenames
(or state they are archived in git history with commit hashes or a pointer to
the branch/tag) so the decision history is auditable.
- Around line 35-40: Edit the "Consequences" bullet list in
adr-0001-structure-representative-e2e-coverage.md to remove repetitive "Good,
because …" openings: either merge related bullets or rephrase each line to vary
the lead (e.g., "Reusable wiring: …", "Models evolving V1→V2 history: …",
"Deterministic dataset: …", "Includes remote-state checks: …", "Simplified
archive-tier lifecycle: …"), keeping the same content and references to
Arius.Tests.Shared, Arius.Integration.Tests, Arius.E2E.Tests and
SyntheticRepositoryDefinitionFactory, so the meaning is unchanged but
scanability and tooling warnings are reduced.
In `@README.md`:
- Around line 31-33: The README's instruction to "Restart OpenCode after pulling
the workspace" is ambiguous; update the sentence to specify that a restart is
only required when plugin configuration files change (e.g., opencode.json) so
users don't restart on every git pull—reference the opencode.json file and the
superpowers plugin and change the text to something like "Restart OpenCode after
pulling the workspace if opencode.json (plugin configuration) has changed so the
superpowers plugin and other plugin skills will load."
- Around line 103-106: Clarify that ARIUS_KEY is for interactive/CLI usage while
ARIUS_E2E_ACCOUNT and ARIUS_E2E_KEY are for the end-to-end test harness: add a
one-line note under "Account key" stating that ARIUS_KEY (or the dotnet
user-secrets key arius:<account>:key) is used by the CLI/runtime, whereas
ARIUS_E2E_ACCOUNT/ARIUS_E2E_KEY (and the src/Arius.E2E.Tests user-secrets scope)
are only for E2E tests to avoid copy/paste misconfiguration.
- Around line 137-145: Add an explicit capability-gated callout to the README
bullet list: state that archive-tier steps in the canonical representative
workflow (referenced by RepresentativeArchiveRestoreTests.cs) are gated by
backend capabilities per the ADR, so archive-tier pending-versus-ready behavior
only runs when the backend advertises support; keep the rest of the bullets
as-is and ensure SyntheticRepositoryDefinitionFactory and E2ETests.cs are still
mentioned for repository sizing and live-Azure probes respectively.
- Around line 121-124: Update the README sentence that currently reads
"Azurite-backed integration and E2E tests report as skipped when Docker is
unavailable, so the test report shows that the local emulator coverage was
intentionally not run." to use a stronger, more precise verb (e.g., "indicates"
or "highlights") and tighten the phrasing; locate the sentence containing
"Azurite-backed integration and E2E tests report as skipped when Docker is
unavailable" and replace the trailing clause with something like "indicating
that local emulator coverage was intentionally not run" or "highlighting that
the local emulator coverage was intentionally skipped" to improve CI-output
clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47cb6ca1-dde1-4a8f-8ee6-2972fa893847
📒 Files selected for processing (38)
.agents/skills/ilspy-decompile/SKILL.md.github/prompts/opsx-bulk-archive.prompt.md.github/prompts/opsx-explore.prompt.md.github/prompts/opsx-onboard.prompt.md.github/skills/openspec-apply-change/SKILL.md.github/skills/openspec-archive-change/SKILL.md.github/skills/openspec-bulk-archive-change/SKILL.md.github/skills/openspec-continue-change/SKILL.md.github/skills/openspec-explore/SKILL.md.github/skills/openspec-ff-change/SKILL.md.github/skills/openspec-new-change/SKILL.md.github/skills/openspec-onboard/SKILL.md.github/skills/openspec-sync-specs/SKILL.md.github/skills/openspec-verify-change/SKILL.md.opencode/commands/opsx-apply.md.opencode/commands/opsx-archive.md.opencode/commands/opsx-bulk-archive.md.opencode/commands/opsx-continue.md.opencode/commands/opsx-explore.md.opencode/commands/opsx-ff.md.opencode/commands/opsx-new.md.opencode/commands/opsx-onboard.md.opencode/commands/opsx-sync.md.opencode/commands/opsx-verify.md.opencode/skills/openspec-apply-change/SKILL.md.opencode/skills/openspec-archive-change/SKILL.md.opencode/skills/openspec-bulk-archive-change/SKILL.md.opencode/skills/openspec-continue-change/SKILL.md.opencode/skills/openspec-explore/SKILL.md.opencode/skills/openspec-ff-change/SKILL.md.opencode/skills/openspec-new-change/SKILL.md.opencode/skills/openspec-onboard/SKILL.md.opencode/skills/openspec-sync-specs/SKILL.md.opencode/skills/openspec-verify-change/SKILL.mdAGENTS.mdREADME.mddocs/decisions/adr-0001-structure-representative-e2e-coverage.mddocs/decisions/adr-template.md
💤 Files with no reviewable changes (1)
- .agents/skills/ilspy-decompile/SKILL.md
✅ Files skipped from review due to trivial changes (27)
- .opencode/commands/opsx-bulk-archive.md
- .github/skills/openspec-apply-change/SKILL.md
- .github/skills/openspec-ff-change/SKILL.md
- .opencode/skills/openspec-ff-change/SKILL.md
- .github/prompts/opsx-bulk-archive.prompt.md
- .opencode/skills/openspec-continue-change/SKILL.md
- .github/skills/openspec-new-change/SKILL.md
- .opencode/skills/openspec-verify-change/SKILL.md
- .opencode/skills/openspec-new-change/SKILL.md
- .opencode/skills/openspec-apply-change/SKILL.md
- .opencode/skills/openspec-sync-specs/SKILL.md
- .github/skills/openspec-archive-change/SKILL.md
- .opencode/skills/openspec-archive-change/SKILL.md
- .github/skills/openspec-continue-change/SKILL.md
- docs/decisions/adr-template.md
- .opencode/commands/opsx-explore.md
- .opencode/skills/openspec-bulk-archive-change/SKILL.md
- .github/prompts/opsx-explore.prompt.md
- .github/skills/openspec-bulk-archive-change/SKILL.md
- .opencode/commands/opsx-onboard.md
- .github/skills/openspec-sync-specs/SKILL.md
- .github/skills/openspec-explore/SKILL.md
- .github/skills/openspec-onboard/SKILL.md
- .opencode/skills/openspec-explore/SKILL.md
- .opencode/skills/openspec-onboard/SKILL.md
- .github/skills/openspec-verify-change/SKILL.md
- .github/prompts/opsx-onboard.prompt.md
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Arius.E2E.Tests/Workflows/Models.cs (1)
6-26: Split these workflow records into file-per-type definitions.
Models.csnow holds three top-level record types, so symbol-to-file lookup gets harder and the filename no longer points at a concrete type. Please move them intoRepresentativeWorkflowDefinition.cs,RepresentativeWorkflowRunResult.cs, andArchiveTierWorkflowOutcome.cs.As per coding guidelines,
**/*.cs: Prefer one top-level class per file, with the filename matching the class name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Workflows/Models.cs` around lines 6 - 26, Models.cs currently defines three top-level records which should each live in their own file; split RepresentativeWorkflowDefinition, RepresentativeWorkflowRunResult, and ArchiveTierWorkflowOutcome into separate files named after each record, move the respective record declaration into RepresentativeWorkflowDefinition.cs, RepresentativeWorkflowRunResult.cs, and ArchiveTierWorkflowOutcome.cs, ensure each new file has the same namespace and accessibility (internal sealed record ...) and update any usings or references if needed so compilation and symbol lookup work unchanged.src/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinition.cs (1)
123-170: MoveSyntheticRepositoryPathinto its own file.This helper is a second top-level type inside
SyntheticRepositoryDefinition.cs. Giving it a dedicatedSyntheticRepositoryPath.cskeeps the file/type mapping consistent and makes future path-validation changes easier to track.As per coding guidelines,
**/*.cs: Prefer one top-level class per file, with the filename matching the class name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinition.cs` around lines 123 - 170, SyntheticRepositoryPath is a second top-level type in the same file; extract it into its own file by creating a new top-level class file that declares SyntheticRepositoryPath and moves both NormalizeRootDirectory and NormalizeRelativePath (and any related using directives) into it, keeping the implementation unchanged and preserving accessibility; update any references/imports as needed so callers of SyntheticRepositoryPath continue to compile.src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs (1)
7-11: GiveRepresentativeWorkflowRunnerDependenciesits own file.This adds a second top-level type to
RepresentativeWorkflowRunner.cs. Moving it toRepresentativeWorkflowRunnerDependencies.cskeeps the repo’s file/type mapping predictable.As per coding guidelines,
**/*.cs: Prefer one top-level class per file, with the filename matching the class name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs` around lines 7 - 11, Extract the internal sealed class RepresentativeWorkflowRunnerDependencies (which declares the CreateFixtureAsync Func that delegates to RepresentativeWorkflowRunner.CreateFixtureAsync) into its own file named RepresentativeWorkflowRunnerDependencies.cs; remove the type from RepresentativeWorkflowRunner.cs so each top-level type lives in its own file, preserve the same namespace and accessibility, and ensure using directives and compilation/namespace declarations are updated so RepresentativeWorkflowRunnerDependencies compiles and references RepresentativeWorkflowRunner.CreateFixtureAsync correctly.
🤖 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.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs`:
- Around line 71-84: The finally block in RepresentativeWorkflowRunner currently
awaits state.Fixture.DisposeAsync() or fixture.DisposeAsync() directly which can
throw and prevent the subsequent Directory.Delete(workflowRoot) from running;
modify the finally so that disposal calls for state.Fixture.DisposeAsync() and
fixture.DisposeAsync() are wrapped in their own try/catch (or use Task.Run with
swallow/log on exception) and do not rethrow, then always run
Directory.Delete(workflowRoot, recursive: true) unconditionally (guarded by
Directory.Exists) after disposal attempts so the workflowRoot is removed even if
disposal fails.
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Around line 167-179: CreateTempRoots currently reuses existing directories
(TempRootFolderName/resolvedTempRoot) which leaves stale files in the "source"
and "restore" trees when RepresentativeWorkflowRunner recreates a fixture for
the same workflowRoot; change CreateTempRoots to ensure a clean directory state
by deleting any existing resolvedTempRoot (or at minimum the localRoot and
restoreRoot subdirectories) before calling Directory.CreateDirectory, so
localRoot and restoreRoot are recreated empty; reference the CreateTempRoots
method and TempRootFolderName to locate where to perform the deletion/cleanup.
---
Nitpick comments:
In `@src/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinition.cs`:
- Around line 123-170: SyntheticRepositoryPath is a second top-level type in the
same file; extract it into its own file by creating a new top-level class file
that declares SyntheticRepositoryPath and moves both NormalizeRootDirectory and
NormalizeRelativePath (and any related using directives) into it, keeping the
implementation unchanged and preserving accessibility; update any
references/imports as needed so callers of SyntheticRepositoryPath continue to
compile.
In `@src/Arius.E2E.Tests/Workflows/Models.cs`:
- Around line 6-26: Models.cs currently defines three top-level records which
should each live in their own file; split RepresentativeWorkflowDefinition,
RepresentativeWorkflowRunResult, and ArchiveTierWorkflowOutcome into separate
files named after each record, move the respective record declaration into
RepresentativeWorkflowDefinition.cs, RepresentativeWorkflowRunResult.cs, and
ArchiveTierWorkflowOutcome.cs, ensure each new file has the same namespace and
accessibility (internal sealed record ...) and update any usings or references
if needed so compilation and symbol lookup work unchanged.
In `@src/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cs`:
- Around line 7-11: Extract the internal sealed class
RepresentativeWorkflowRunnerDependencies (which declares the CreateFixtureAsync
Func that delegates to RepresentativeWorkflowRunner.CreateFixtureAsync) into its
own file named RepresentativeWorkflowRunnerDependencies.cs; remove the type from
RepresentativeWorkflowRunner.cs so each top-level type lives in its own file,
preserve the same namespace and accessibility, and ensure using directives and
compilation/namespace declarations are updated so
RepresentativeWorkflowRunnerDependencies compiles and references
RepresentativeWorkflowRunner.CreateFixtureAsync correctly.
🪄 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: 9f5d2d3e-c4c6-448e-bcfd-8865e5157025
📒 Files selected for processing (9)
AGENTS.mddocs/decisions/adr-0001-structure-representative-e2e-coverage.mdsrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinition.cssrc/Arius.E2E.Tests/Fixtures/AzuriteE2EBackendFixture.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.E2E.Tests/Workflows/Models.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowState.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs
✅ Files skipped from review due to trivial changes (1)
- src/Arius.E2E.Tests/Fixtures/AzuriteE2EBackendFixture.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 113-122: Update the README table so the src/Arius.E2E.Tests row
does not assert real Azure credentials are required: change the "Requires real
Azure credentials" cell for src/Arius.E2E.Tests from Y to N (or annotate it as
"Optional/Opt-in") to align with the lower section that says live Azure coverage
is opt-in and Azurite-backed runs without credentials; ensure the table header
"Requires real Azure credentials" and the row for src/Arius.E2E.Tests are
consistent with the explanatory text below.
In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs`:
- Around line 91-99: ResetLocalCacheAsync currently returns Task.CompletedTask
when HasActiveLease(accountName, containerName) is true, silently skipping the
requested reset; change this to fail fast by throwing an explicit exception
(e.g., InvalidOperationException with a clear message) so callers cannot assume
the cache was reset when a lease is active. Locate the early-return in
ResetLocalCacheAsync (the lock on RepositoryCacheLeaseLock and the call to
HasActiveLease) and replace the silent return with a thrown exception that
includes accountName/containerName and guidance that a lease is active, so
workflow steps (V1/V2 and Warm/Cold transitions) remain explicit and visible.
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Line 16: Change the RepositoryTestFixture class from public to internal to
avoid unnecessarily widening the API surface; locate the declaration "public
sealed class RepositoryTestFixture : IAsyncDisposable" and update its
accessibility to "internal sealed class RepositoryTestFixture :
IAsyncDisposable". If other test assemblies must access it, add an
InternalsVisibleTo attribute in the test-support assembly's AssemblyInfo (or
project file) for the consuming test assembly instead of making the class
public.
🪄 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: 1a7a010d-c555-443b-be83-814e0c3dbc41
📒 Files selected for processing (7)
AGENTS.mdREADME.mdsrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.E2E.Tests/Fixtures/AzuriteE2EBackendFixture.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cs
Comment on lines
+91
to
+99
| public static Task ResetLocalCacheAsync(string accountName, string containerName) | ||
| { | ||
| var cacheDir = RepositoryPaths.GetRepositoryDirectory(accountName, containerName); | ||
|
|
||
| lock (RepositoryCacheLeaseLock) | ||
| { | ||
| if (HasActiveLease(accountName, containerName)) | ||
| return Task.CompletedTask; | ||
|
|
Contributor
There was a problem hiding this comment.
Don’t silently skip a requested cache reset.
This returns success when a live lease exists, so a caller asking for a cold-cache step can quietly keep running warm. That makes the workflow state implicit again and can hide sequencing bugs in representative coverage.
Minimal fail-fast fix
lock (RepositoryCacheLeaseLock)
{
if (HasActiveLease(accountName, containerName))
- return Task.CompletedTask;
+ throw new InvalidOperationException(
+ $"Cannot reset local cache for '{accountName}/{containerName}' while fixtures are still active.");
try
{Based on learnings, treat dataset versions (V1 vs V2) and cache transitions (Warm vs Cold) as explicit workflow steps in one evolving repository history, not incidental fixture behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs` around lines 91 - 99,
ResetLocalCacheAsync currently returns Task.CompletedTask when
HasActiveLease(accountName, containerName) is true, silently skipping the
requested reset; change this to fail fast by throwing an explicit exception
(e.g., InvalidOperationException with a clear message) so callers cannot assume
the cache was reset when a lease is active. Locate the early-return in
ResetLocalCacheAsync (the lock on RepositoryCacheLeaseLock and the call to
HasActiveLease) and replace the silent return with a thrown exception that
includes accountName/containerName and guidance that a lease is active, so
workflow steps (V1/V2 and Warm/Cold transitions) remain explicit and visible.
|
|
||
| namespace Arius.Tests.Shared.Fixtures; | ||
|
|
||
| public sealed class RepositoryTestFixture : IAsyncDisposable |
Contributor
There was a problem hiding this comment.
Make the shared fixture internal.
RepositoryTestFixture lives in a test-support assembly, so exposing it as public widens the surface area for no production benefit. If other test assemblies need it, prefer InternalsVisibleTo over a public type. As per coding guidelines, src/**/*.cs: Make non-test classes internal. Only make them public when they must be consumed by another non-test assembly; for test access, prefer InternalsVisibleTo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs` at line 16, Change
the RepositoryTestFixture class from public to internal to avoid unnecessarily
widening the API surface; locate the declaration "public sealed class
RepositoryTestFixture : IAsyncDisposable" and update its accessibility to
"internal sealed class RepositoryTestFixture : IAsyncDisposable". If other test
assemblies must access it, add an InternalsVisibleTo attribute in the
test-support assembly's AssemblyInfo (or project file) for the consuming test
assembly instead of making the class public.
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
Test Plan
dotnet build "src/Arius.E2E.Tests/Arius.E2E.Tests.csproj"dotnet test --project "src/Arius.E2E.Tests/Arius.E2E.Tests.csproj" --treenode-filter "/*/*/RepresentativeArchiveRestoreTests/*"/Users/wouter/.dotnet/tools/slopwatch analyzeNotes
Summary by CodeRabbit