feat: cross-platform desktop E2E testing - phase 11.4#223
Merged
Conversation
Phase 11.4: Cross-Platform E2E Testing - Native Postgres + IPFS per CI runner, fresh backend per run - Smart trigger (path filter PRs, full matrix on main/release) - FUSE mount file ops with platform drivers on all 3 runners - Tauri app in test mode, dev-key auth bypass - Full API round-trip interop + static test vectors - Both AES-GCM and AES-CTR coverage - Shared test fixtures checked into git Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ae0dbc39cbd4
Phase 11.4: Cross-Platform E2E Testing - CI stack: native Postgres/IPFS/Redis/FUSE per platform runner - Debug build required for --dev-key headless auth - Redis is hidden API dependency (BullMQ) - Separate e2e-desktop.yml workflow with workflow_run trigger - All building blocks exist in codebase Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 99f19ffff1d3
Phase 11.4: Cross-Platform E2E Testing - 3 plans in 2 waves - Wave 1: Plan 01 (CI debug binary + fixtures) and Plan 02 (test scripts) in parallel - Wave 2: Plan 03 (e2e-desktop.yml workflow) depends on both - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e954974c80c7
- wait-for-mount.sh/ps1: poll mount point every 2s with 90s timeout - run-all.sh/ps1: orchestrate mount wait, FUSE tests, and API round-trip - Bash scripts have +x permission for CI execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6d8ee45a39f1
- aes-gcm-vectors.json: AES-256-GCM vector from tests.rs constants - aes-ctr-vectors.json: AES-256-CTR (Ctr64BE) vector matching Web Crypto length:64 - metadata-vectors.json: v2 folder + v1 file metadata structures with field rules - All values verified against apps/desktop/src-tauri/src/crypto/tests.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 3c172739e08f
Tasks completed: 2/2 - Add debug binary build + artifact upload to CI build jobs - Create cross-platform crypto test vector fixtures SUMMARY: .planning/phases/11.4-cross-platform-e2e-testing/11.4-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8eb68823d38a
- 3-platform matrix (macOS, Windows, Linux) with native service installs - workflow_run trigger after CI + manual workflow_dispatch - Downloads debug binary from CI artifacts (not rebuilt) - Installs Postgres, Kubo, Redis, FUSE drivers per platform - Includes Plan 02 and 03 summaries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 1f273f6f66e6
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ca18ff7ad9d9
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds cross-platform desktop E2E testing: CI now builds and uploads per-OS debug desktop binaries; a new e2e-desktop workflow downloads those artifacts, provisions backend services per platform, launches the desktop app (FUSE), and runs cross-platform test scripts and crypto fixtures to validate FUSE I/O and API round-trips. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Workflow
participant Registry as Artifact Registry
participant E2E as E2E Workflow
participant Setup as Backend Setup
participant API as API Server
participant Desktop as Desktop App
participant Tests as Test Scripts
participant Logs as Log Artifacts
CI->>Registry: Upload desktop-debug-{platform}
E2E->>Registry: Download desktop-debug-{platform}
E2E->>Setup: Install & start Postgres / IPFS / Redis / FUSE
Setup->>API: Services ready
E2E->>API: Launch API server and poll health
E2E->>Desktop: Launch desktop with --dev-key
Desktop->>Setup: Mount FUSE
Desktop->>API: Connect to backend
E2E->>Tests: Run platform-specific test scripts
Tests->>Desktop: Perform FUSE read/write/rename/delete
Tests->>API: Authenticate & query vault/IPNS
alt Success
Tests->>E2E: Exit 0
else Failure
E2E->>Logs: Upload logs/artifacts
Tests->>E2E: Exit non-zero
end
E2E->>Desktop: Cleanup (terminate/unmount)
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. Comment |
FSM1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
tests/e2e-desktop/scripts/wait-for-mount.ps1 (1)
19-19: Apply Write-Output/Write-Information consistently across all desktop test scripts in this directory.These test scripts use
Write-Hostthroughout (wait-for-mount.ps1, test-fuse-operations.ps1, test-round-trip.ps1, run-all.ps1), which limits output capturability if needed for logging or automation contexts. Replace withWrite-OutputorWrite-Informationfor better compatibility with output redirection and pipeline-based tooling.Also applies to: 23-23, 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/wait-for-mount.ps1` at line 19, Replace all uses of Write-Host with a pipeline-friendly cmdlet (Write-Output or Write-Information) in the desktop test scripts; specifically change the Write-Host invocation that prints "Waiting for mount at $MountPoint (timeout: ${Timeout}s)..." in wait-for-mount.ps1 and the analogous Write-Host calls in test-fuse-operations.ps1, test-round-trip.ps1 and run-all.ps1 so output can be captured or redirected. Use Write-Output for simple stdout text or Write-Information if you want the message to be an informational stream, and ensure the same choice is applied consistently across those scripts.tests/e2e-desktop/scripts/test-round-trip.sh (1)
63-63: Replace fixed sleeps with readiness polling.Line 63 and Line 96 make the test timing-sensitive and flaky across runners. Poll the expected API condition with a bounded timeout instead.
Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/test-round-trip.sh` at line 63, Replace the fixed "sleep 5" calls with a bounded readiness poll: add a helper (e.g., wait_for_service or wait_for_api_ready) that repeatedly queries the expected endpoint (curl to the API's health/ready URL used elsewhere in the script) at short intervals, checks for the expected HTTP status or response body, and returns success; loop until either the check passes or a configurable timeout (e.g., DEFAULT_TIMEOUT) is reached and then fail the script. Then replace the two literal "sleep" invocations with calls to this helper so the test waits deterministically for the API condition instead of sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 518-523: The upload-artifact steps named "Upload debug desktop
binary" (and the two other debug artifact upload steps using
actions/upload-artifact@v4) currently rely on the default if-no-files-found
behavior (warn); update each of these three upload steps to include
if-no-files-found: error so the job fails immediately when the expected binary
path (e.g., apps/desktop/src-tauri/target/debug/cipherbox-desktop.exe) is
missing—locate the steps by their step name and the use of
actions/upload-artifact@v4 and add the if-no-files-found: error option to each
block.
In @.github/workflows/e2e-desktop.yml:
- Around line 99-105: The readiness polling loops (the for i in $(seq 1 30) ...
curl -s http://localhost:5001/api/v0/id ... echo "IPFS daemon ready") currently
only break on success and never fail on timeout; update each loop so that after
the loop completes you test whether the service became available and, if not,
emit a clear error message and exit non‑zero (e.g., echo "IPFS daemon did not
become ready after Xs" and exit 1). Apply the same pattern to the other
readiness loops referenced (around the blocks that echo readiness messages at
lines like 120-126, 247-253, 294-300) so the workflow fails fast and with a
clear error when a service never becomes ready.
- Around line 314-318: The step currently runs "bash
tests/e2e-desktop/scripts/run-all.sh" under an -e shell so a non-zero test exit
aborts the step before cleanup; modify the job to disable immediate exit (e.g.,
run set +e or run the test script in a subshell with error capture) immediately
before invoking run-all.sh, capture its exit into TEST_EXIT, then always perform
kill $DESKTOP_PID and diskutil unmount force $HOME/CipherBox (and equivalents in
the later block) and finally exit $TEST_EXIT; update both the block containing
run-all.sh/TEST_EXIT/DESKTOP_PID/diskutil and the similar 332-336 block to
follow this pattern so cleanup always runs.
- Around line 52-57: The "Find latest CI run ID" step (id: find-run) may produce
an empty RUN_ID causing later artifact download to fail obscurely; modify the
run block that sets RUN_ID to check if RUN_ID is empty (test RUN_ID) and if so
emit a clear error message (e.g., "No successful CI run found for workflow
ci.yml on main") to stderr and exit non-zero so the job fails fast; if RUN_ID is
present, continue and echo "run_id=$RUN_ID" to GITHUB_OUTPUT as before.
In `@tests/e2e-desktop/fixtures/crypto/metadata-vectors.json`:
- Around line 44-47: The fixture has an invalid IV length for AES-CTR: update
the "fileIv" value in metadata-vectors.json (the entry with "encryptionMode":
"CTR") to a proper nonce size (e.g., a 16-byte hex string — 32 hex characters)
that matches your AES/CTR implementation expectations so tests don't use a
4-byte IV; ensure the hex string contains only 0-9a-f characters and the length
conforms to the crypto library's required nonce size for AES-CTR.
In `@tests/e2e-desktop/scripts/run-all.ps1`:
- Line 59: The current invocation in run-all.ps1 passes the secret as a
command-line argument (& "$PSScriptRoot\test-round-trip.ps1" -MountPoint
$MountPoint -ApiUrl $ApiUrl -TestSecret $TestSecret); remove the -TestSecret
parameter and instead export the secret into an environment variable (e.g.,
$env:TEST_SECRET = $TestSecret) before calling test-round-trip.ps1, and update
test-round-trip.ps1 to read the secret from the environment (e.g.,
$env:TEST_SECRET) rather than a parameter; ensure any references to the
TestSecret parameter (in test-round-trip.ps1 and callers) are removed or
replaced with environment lookups.
In `@tests/e2e-desktop/scripts/run-all.sh`:
- Line 59: The child script is being invoked with TEST_SECRET as a positional
argument (bash "test-round-trip.sh" "$MOUNT_POINT" "$API_URL" "$TEST_SECRET"),
which exposes secrets in process argv; instead pass the secret via the
environment and update the child to read from the TEST_SECRET env var. Change
the caller to not include "$TEST_SECRET" in the argument list and either export
TEST_SECRET or prefix the invocation with TEST_SECRET="$TEST_SECRET" bash
"test-round-trip.sh" "$MOUNT_POINT" "$API_URL"; then update the child script
(test-round-trip.sh) to stop reading the secret from the positional parameter
(e.g., $3 or $4) and read it from the TEST_SECRET environment variable.
In `@tests/e2e-desktop/scripts/test-fuse-operations.ps1`:
- Around line 132-137: The cleanup always calls Test-Pass regardless of deletion
outcome; change the Remove-Item calls for $MountBinary and $TempBinary to verify
success (either use -ErrorAction Stop with try/catch or run Test-Path after
removal) and only call Test-Pass when both artifacts are gone; otherwise call
Test-Fail and include which path(s) failed; reference the existing Remove-Item
invocations and the Test-Pass/Test-Fail functions when updating the cleanup
block.
In `@tests/e2e-desktop/scripts/test-fuse-operations.sh`:
- Line 2: The script currently enables immediate exit with "set -euo pipefail"
which aborts the script on the first failing command and prevents the fail() and
summary aggregation from running; remove or disable the -e flag so failures are
counted instead (e.g., change "set -euo pipefail" to "set -uo pipefail" or
explicitly run "set +e" before test commands), ensuring commands in the test
flow (those that call fail() and the summary logic) are allowed to fail and
increment the failure counter rather than causing an immediate exit.
In `@tests/e2e-desktop/scripts/test-round-trip.ps1`:
- Line 19: The test sets $ErrorActionPreference = "Continue" which allows Test 2
to pass without verifying the FUSE write; change the test to fail on write
errors by setting $ErrorActionPreference = "Stop" (or explicitly check the
result of the write operation) and add a post-write assertion that verifies the
file was actually written (e.g., check the file exists and its contents/size)
before using rootIpnsName in subsequent assertions so any FUSE write failure
causes the test to fail immediately.
In `@tests/e2e-desktop/scripts/test-round-trip.sh`:
- Around line 42-44: The curl calls (e.g., the AUTH_RESPONSE assignment) can
hang or treat HTTP errors as success; update every curl invocation in the script
(including the other occurrences flagged) to use fail-fast and timeouts (add -f
--connect-timeout 5 --max-time 30) and ensure the script stops on failure (check
curl exit status and exit 1 or use set -e). For example, modify
AUTH_RESPONSE=$(curl ...) to include -f --connect-timeout 5 --max-time 30 and
add an immediate check of the command's exit code to abort the job on failure.
- Line 50: Replace direct dumping of full API responses into failure messages
(e.g., the fail invocation that interpolates AUTH_RESPONSE) with a sanitized log
that extracts only non-sensitive fields such as HTTP status and an error
message; update the fail calls referencing AUTH_RESPONSE and the similar
occurrences at the other locations to parse AUTH_RESPONSE (or the equivalent
response variable) and include only status and a stripped/sanitized error field
(remove tokens/headers/payloads) before calling fail so CI logs never contain
full responses or secrets.
In `@tests/e2e-desktop/scripts/wait-for-mount.ps1`:
- Around line 21-25: The current loop uses Test-Path($MountPoint) which returns
true for an existing directory even if WinFsp isn't mounted; change the
readiness check in the while loop to require the mount point to be a reparse
point (i.e., verify the directory exists and its Attributes contains the
ReparsePoint flag) before writing "PASS" and exiting. Locate the loop that
references $Elapsed, $Timeout, $MountPoint and replace the single
Test-Path-based condition with a check that gets the item (Get-Item/$MountPoint)
and ensures its Attributes -band [System.IO.FileAttributes]::ReparsePoint (or
equivalent PowerShell attribute test) is true.
In `@tests/e2e-desktop/scripts/wait-for-mount.sh`:
- Line 23: The if condition that checks the mount uses regex grep which can
mis-match paths; update the check that references MOUNT_POINT in the if
statement to use fixed-string matching (e.g. use grep -F -q -- "$MOUNT_POINT" or
grep -xF -q -- "$MOUNT_POINT" depending on whether you need whole-line match) so
the mount path is matched literally; modify the conditional containing [ -d
"$MOUNT_POINT" ] && mount | grep -q "$MOUNT_POINT" to use the fixed-string grep
variant.
---
Nitpick comments:
In `@tests/e2e-desktop/scripts/test-round-trip.sh`:
- Line 63: Replace the fixed "sleep 5" calls with a bounded readiness poll: add
a helper (e.g., wait_for_service or wait_for_api_ready) that repeatedly queries
the expected endpoint (curl to the API's health/ready URL used elsewhere in the
script) at short intervals, checks for the expected HTTP status or response
body, and returns success; loop until either the check passes or a configurable
timeout (e.g., DEFAULT_TIMEOUT) is reached and then fail the script. Then
replace the two literal "sleep" invocations with calls to this helper so the
test waits deterministically for the API condition instead of sleeping.
In `@tests/e2e-desktop/scripts/wait-for-mount.ps1`:
- Line 19: Replace all uses of Write-Host with a pipeline-friendly cmdlet
(Write-Output or Write-Information) in the desktop test scripts; specifically
change the Write-Host invocation that prints "Waiting for mount at $MountPoint
(timeout: ${Timeout}s)..." in wait-for-mount.ps1 and the analogous Write-Host
calls in test-fuse-operations.ps1, test-round-trip.ps1 and run-all.ps1 so output
can be captured or redirected. Use Write-Output for simple stdout text or
Write-Information if you want the message to be an informational stream, and
ensure the same choice is applied consistently across those scripts.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.github/workflows/ci.yml.github/workflows/e2e-desktop.yml.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-01-PLAN.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-01-SUMMARY.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-02-PLAN.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-02-SUMMARY.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-03-PLAN.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-03-SUMMARY.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-CONTEXT.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-RESEARCH.md.planning/phases/11.4-cross-platform-e2e-testing/11.4-VERIFICATION.mdtests/e2e-desktop/fixtures/crypto/aes-ctr-vectors.jsontests/e2e-desktop/fixtures/crypto/aes-gcm-vectors.jsontests/e2e-desktop/fixtures/crypto/metadata-vectors.jsontests/e2e-desktop/scripts/run-all.ps1tests/e2e-desktop/scripts/run-all.shtests/e2e-desktop/scripts/test-fuse-operations.ps1tests/e2e-desktop/scripts/test-fuse-operations.shtests/e2e-desktop/scripts/test-round-trip.ps1tests/e2e-desktop/scripts/test-round-trip.shtests/e2e-desktop/scripts/wait-for-mount.ps1tests/e2e-desktop/scripts/wait-for-mount.sh
- Add if-no-files-found: error to CI upload-artifact steps - Fail fast when no CI run ID found in e2e-desktop workflow - Add timeout failure to all readiness polling loops - Wrap run-all.sh in set +e so cleanup always runs after test failure - Fix fileIv to 16 bytes (32 hex chars) for CTR mode in test vectors - Pass test secrets via environment instead of command-line arguments - Validate cleanup post-conditions in FUSE operations tests - Remove set -e from test-fuse-operations.sh for failure aggregation - Add error handling around FUSE write in round-trip test - Add curl fail-fast and timeout flags to prevent hung E2E jobs - Sanitize API responses in failure log messages - Strengthen mount-readiness check with reparse-point on Windows - Use fixed-string matching for mount path checks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ce7a8528457e
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/e2e-desktop.yml (2)
353-354: Quote$DEV_KEYhere as well for consistency.♻️ Proposed fix
- ./desktop-binary/cipherbox-desktop --dev-key $DEV_KEY & + ./desktop-binary/cipherbox-desktop --dev-key "$DEV_KEY" &🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-desktop.yml around lines 353 - 354, The workflow currently passes the DEV_KEY unquoted to the command; update the invocation of cipherbox-desktop to quote the variable (use "$DEV_KEY") so the DEV_KEY value is expanded safely and consistently with the assignment (refer to the DEV_KEY variable and the command ./desktop-binary/cipherbox-desktop --dev-key $DEV_KEY in the diff).
333-334: Quote$DEV_KEYto prevent word splitting.While the hex string from
openssl randwon't contain spaces, quoting prevents potential issues and satisfies shellcheck.♻️ Proposed fix
- ./desktop-binary/cipherbox-desktop --dev-key $DEV_KEY & + ./desktop-binary/cipherbox-desktop --dev-key "$DEV_KEY" &🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-desktop.yml around lines 333 - 334, The DEV_KEY environment variable expansion is unquoted when passed to the desktop binary which can cause word-splitting; update the command invocation that runs ./desktop-binary/cipherbox-desktop --dev-key $DEV_KEY & to quote the variable as ./desktop-binary/cipherbox-desktop --dev-key "$DEV_KEY" & so that the DEV_KEY value is passed intact to the process.tests/e2e-desktop/scripts/test-fuse-operations.sh (1)
137-143: Consider using glob pattern instead ofls | grepfor robustness.The
ls | greppattern can misbehave with filenames containing special characters or newlines. Since this is a controlled E2E test environment, the risk is low, but a safer approach exists.♻️ Proposed alternative using glob/find
-REMAINING=$(ls "$MP" 2>/dev/null | grep -v '\.DS_Store' | grep -v '^$' || true) +REMAINING=$(find "$MP" -mindepth 1 -maxdepth 1 ! -name '.DS_Store' -printf '%f\n' 2>/dev/null || true)Or for macOS compatibility:
-REMAINING=$(ls "$MP" 2>/dev/null | grep -v '\.DS_Store' | grep -v '^$' || true) +REMAINING="" +for f in "$MP"/*; do + [ -e "$f" ] || continue + name=$(basename "$f") + [ "$name" = ".DS_Store" ] && continue + REMAINING="${REMAINING:+$REMAINING }$name" +done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/test-fuse-operations.sh` around lines 137 - 143, Replace the fragile ls|grep pipeline that sets REMAINING with a robust find-based check: instead of using REMAINING=$(ls "$MP" ... | grep ...), run a find search like find "$MP" -mindepth 1 -maxdepth 1 ! -name '.DS_Store' -print -quit and test its output (or use the exit status) to decide cleanup success; update the code that references REMAINING and keep the same pass/else logic so scripts using the REMAINING variable/branching (the block around REMAINING and MP) behave identically but handle filenames with newlines/special chars safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e-desktop/scripts/test-round-trip.ps1`:
- Around line 86-108: The FUSE write failure path can still allow the vault API
call to run and overwrite $RootIpns, so add an explicit boolean flag (e.g.,
$WriteSucceeded) initialized to $false; set it to $true only after the
Set-Content/Test-Path block succeeds, and only execute the
Invoke-RestMethod/$VaultResponse block when $WriteSucceeded is $true; ensure
when the write fails you skip the vault verification (and keep $RootIpns =
$null) so the test cannot pass due to stale state.
---
Nitpick comments:
In @.github/workflows/e2e-desktop.yml:
- Around line 353-354: The workflow currently passes the DEV_KEY unquoted to the
command; update the invocation of cipherbox-desktop to quote the variable (use
"$DEV_KEY") so the DEV_KEY value is expanded safely and consistently with the
assignment (refer to the DEV_KEY variable and the command
./desktop-binary/cipherbox-desktop --dev-key $DEV_KEY in the diff).
- Around line 333-334: The DEV_KEY environment variable expansion is unquoted
when passed to the desktop binary which can cause word-splitting; update the
command invocation that runs ./desktop-binary/cipherbox-desktop --dev-key
$DEV_KEY & to quote the variable as ./desktop-binary/cipherbox-desktop --dev-key
"$DEV_KEY" & so that the DEV_KEY value is passed intact to the process.
In `@tests/e2e-desktop/scripts/test-fuse-operations.sh`:
- Around line 137-143: Replace the fragile ls|grep pipeline that sets REMAINING
with a robust find-based check: instead of using REMAINING=$(ls "$MP" ... | grep
...), run a find search like find "$MP" -mindepth 1 -maxdepth 1 ! -name
'.DS_Store' -print -quit and test its output (or use the exit status) to decide
cleanup success; update the code that references REMAINING and keep the same
pass/else logic so scripts using the REMAINING variable/branching (the block
around REMAINING and MP) behave identically but handle filenames with
newlines/special chars safely.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/e2e-desktop.ymltests/e2e-desktop/fixtures/crypto/metadata-vectors.jsontests/e2e-desktop/scripts/run-all.ps1tests/e2e-desktop/scripts/run-all.shtests/e2e-desktop/scripts/test-fuse-operations.ps1tests/e2e-desktop/scripts/test-fuse-operations.shtests/e2e-desktop/scripts/test-round-trip.ps1tests/e2e-desktop/scripts/test-round-trip.shtests/e2e-desktop/scripts/wait-for-mount.ps1tests/e2e-desktop/scripts/wait-for-mount.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e-desktop/scripts/run-all.sh
- tests/e2e-desktop/scripts/wait-for-mount.sh
- tests/e2e-desktop/scripts/test-round-trip.sh
- .github/workflows/ci.yml
Skip vault API call when FUSE write fails to prevent stale state from masking test failures in test-round-trip.ps1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 197f689208c8
FUSE-T installs fuse.pc to /usr/local/lib/pkgconfig which isn't in the default pkg-config search path on macOS ARM64 GitHub runners. The tauri-action release build succeeds because it uses cached target artifacts, but the debug build step runs the fuser build script fresh and fails to find the fuse system library. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 068ff803da83
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e-desktop/scripts/test-round-trip.ps1`:
- Around line 13-19: The param block is missing the -TestSecret parameter
causing argument binding failures; add a [string]$TestSecret parameter with a
sensible default to the existing param(...) declaration (so the script accepts
-TestSecret), and then use that parameter (or fallback to $env:TEST_SECRET or
"e2e-test-secret-ci-only") instead of redefining $TestSecret later; update the
existing $TestSecret usage to reference the param variable and remove the
conflicting assignment.
- Around line 94-108: The single-shot Start-Sleep + Invoke-RestMethod check is
brittle; replace it with a bounded retry/poll loop that, when
$FuseWriteSucceeded is true, repeatedly calls Invoke-RestMethod against
"$ApiUrl/vault" (reading $VaultResponse.rootIpnsName) with short sleeps between
attempts until rootIpnsName is non-empty or a configurable timeout/retry count
is reached, catching transient exceptions and only calling Test-Fail after
exhausting retries; ensure the same retry logic is applied to the other
vault/IPNS check block referenced (lines ~117-133) and keep using
Test-Pass/Test-Fail for success/failure reporting.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/e2e-desktop/scripts/test-round-trip.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
Desktop build jobs in ci.yml were running on both PRs and pushes, with redundant tauri-action release builds (deploy-staging.yml already handles production releases). This slowed PR CI by ~10min per platform. Changes: - Remove tauri-action and its pnpm/node/crypto prerequisites from all 3 desktop build jobs (Windows, macOS, Linux) - Gate desktop build jobs to only run on push to main (not PRs) - PRs still get fast cargo-check validation for all 3 platforms Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b084d20ef948
Replace single sleep+check with bounded retry loops (12 attempts) for both vault rootIpnsName and IPNS resolve checks. The async pipeline (FUSE write → encrypt → IPFS upload → IPNS publish) may take variable time, making single-shot checks brittle in CI. Also update header comments to document TEST_SECRET as env var instead of the removed -TestSecret parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 31443524ddf8
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 532-536: The CI step "Build debug binary for E2E" sets
PKG_CONFIG_PATH to /usr/local/lib/pkgconfig which fails on Apple Silicon; update
that environment setting so PKG_CONFIG_PATH includes the Apple Silicon Homebrew
path (/opt/homebrew/lib/pkgconfig) in addition to /usr/local/lib/pkgconfig (or
set it to a combination like
"/opt/homebrew/lib/pkgconfig:/usr/local/lib/pkgconfig") so cargo can find FUSE-T
headers/libraries on macos-latest runners.
macos-latest runs on Apple Silicon where Homebrew installs to /opt/homebrew/. Include both paths so pkg-config finds dependencies regardless of install location. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ef494964ce27
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e-desktop/scripts/test-round-trip.ps1 (1)
96-106: Extract duplicated polling logic into a helper to prevent drift.The vault/IPNS retry loops are functionally good, but they duplicate control flow and logging. A shared retry helper will reduce maintenance risk as this suite evolves.
♻️ Suggested refactor
+function Invoke-WithRetry { + param( + [scriptblock]$Action, + [int]$MaxAttempts, + [int]$DelaySeconds, + [string]$Label + ) + + $result = $null + for ($attempt = 1; $attempt -le $MaxAttempts -and -not $result; $attempt++) { + if ($attempt -gt 1) { Start-Sleep -Seconds $DelaySeconds } + try { + $result = & $Action + } catch { + Write-Host " $Label attempt $attempt failed: $_" + } + } + return $result +} @@ - $RootIpns = $null - for ($attempt = 1; $attempt -le 12 -and -not $RootIpns; $attempt++) { - Start-Sleep -Seconds 5 - try { - $VaultResponse = Invoke-RestMethod -Uri "$ApiUrl/vault" -Headers $Headers - $RootIpns = $VaultResponse.rootIpnsName - } catch { - Write-Host " Vault poll attempt $attempt failed: $_" - } - } + $RootIpns = Invoke-WithRetry -MaxAttempts 12 -DelaySeconds 5 -Label "Vault poll" -Action { + (Invoke-RestMethod -Uri "$ApiUrl/vault" -Headers $Headers).rootIpnsName + } @@ - $ResolvedCid = $null - for ($attempt = 1; $attempt -le 12 -and -not $ResolvedCid; $attempt++) { - Start-Sleep -Seconds 2 - try { - $IpnsResponse = Invoke-RestMethod -Uri "$ApiUrl/ipns/$RootIpns/resolve" -Headers $Headers - $ResolvedCid = if ($IpnsResponse.cid) { $IpnsResponse.cid } else { $IpnsResponse.value } - } catch { - Write-Host " IPNS resolve attempt $attempt failed: $_" - } - } + $ResolvedCid = Invoke-WithRetry -MaxAttempts 12 -DelaySeconds 2 -Label "IPNS resolve" -Action { + $IpnsResponse = Invoke-RestMethod -Uri "$ApiUrl/ipns/$RootIpns/resolve" -Headers $Headers + if ($IpnsResponse.cid) { $IpnsResponse.cid } else { $IpnsResponse.value } + }Also applies to: 121-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/test-round-trip.ps1` around lines 96 - 106, Extract the duplicated retry/poll loop into a reusable helper function (e.g., Invoke-WithRetry or Retry-InvokeRestMethod) and replace both vault/IPNS loops with calls to it; the helper should accept a scriptblock to execute (the Invoke-RestMethod call), retry count, delay, and a success predicate, perform Start-Sleep between attempts, log each attempt (use the same message format currently emitted in the catch), and return the successful result (so callers like the block that sets $RootIpns from $VaultResponse.rootIpnsName and the similar loop at lines 121-129 can simply call the helper with $ApiUrl, $Headers and extract the property).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e-desktop/scripts/test-round-trip.sh`:
- Line 72: The assignment that extracts ROOT_IPNS from VAULT_RESPONSE using jq
can fail if VAULT_RESPONSE is not valid JSON; update the ROOT_IPNS extraction
(the command that reads VAULT_RESPONSE into ROOT_IPNS) to mirror the resilience
used elsewhere (line 95) by silencing jq errors and defaulting to an empty
string on failure (i.e., pipe VAULT_RESPONSE to jq -r '.rootIpnsName // empty'
but redirect jq stderr to /dev/null and fallback to echo "" if jq fails) so the
script won't abort under set -e when the vault returns non-JSON.
---
Nitpick comments:
In `@tests/e2e-desktop/scripts/test-round-trip.ps1`:
- Around line 96-106: Extract the duplicated retry/poll loop into a reusable
helper function (e.g., Invoke-WithRetry or Retry-InvokeRestMethod) and replace
both vault/IPNS loops with calls to it; the helper should accept a scriptblock
to execute (the Invoke-RestMethod call), retry count, delay, and a success
predicate, perform Start-Sleep between attempts, log each attempt (use the same
message format currently emitted in the catch), and return the successful result
(so callers like the block that sets $RootIpns from $VaultResponse.rootIpnsName
and the similar loop at lines 121-129 can simply call the helper with $ApiUrl,
$Headers and extract the property).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.ymltests/e2e-desktop/scripts/test-round-trip.ps1tests/e2e-desktop/scripts/test-round-trip.sh
E2E tests spin up 3 service containers and the full stack, adding 5-10 min to PR CI. Unit tests, typecheck, and API spec validation already catch most regressions on PRs. E2E now runs only on push to main (same as desktop E2E) and manual dispatch. Also removes the now-unnecessary path-filter change detection job. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8da6e1925d0c
…nses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 463f0f5b2872
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
desktop-debug-{windows,macos,linux})e2e-desktop.ymlworkflow with 3-platform matrix (macOS, Windows, Linux) using native service installs (Postgres, Kubo IPFS, Redis, FUSE drivers) — no Docker on macOS/WindowsTest plan
ci.ymlYAML is valid and existing CI jobs still passe2e-desktop.ymlviaworkflow_dispatchafter a successful CI runapps/desktop/src-tauri/src/crypto/tests.rs🤖 Generated with Claude Code
Summary by CodeRabbit