feat: Phase 21 BYO-IPFS Node

[FSM1]

Summary

• Phase 21 context captured from discuss-phase (three-mode pinning, client-direct architecture, TEE migration, vault-stored credentials)
• Technical research completed (PSA spec, Kubo RPC, CORS handling, vault metadata extension)
• Validation strategy created with per-task verification map
• UI design contract for STORAGE settings tab (6/6 dimensions passed)

Files

• 21-CONTEXT.md — Implementation decisions from discuss-phase
• 21-RESEARCH.md — Technical domain research
• 21-VALIDATION.md — Test infrastructure and sampling strategy
• 21-UI-SPEC.md — UI design contract (spacing, typography, color, copywriting)

Test plan

• Review CONTEXT.md decisions align with project direction
• Verify UI-SPEC token values match existing CSS custom properties
• Confirm research findings are actionable for planning

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • STORAGE settings: choose pinning mode (CipherBox / External / Dual), configure external node, test connection (TEE-backed or client-side), save/discard gated by test success, and initiate migrations.

• Backend / API

  • Register externally pinned CIDs; BYO users get advisory (non-enforced) quota indicators and connection-test proxy via TEE.

• Migration

  • Start/pause/resume/cancel pin migrations with progress UI, polling, cancel confirmation, and migration status reporting.

• SDK

  • Runtime pinning reconfiguration so client updates pinning without re-login.

• Tests & UX

  • New tests, STORAGE styles, advisory badge, connection and migration UX improvements, and BYO performance baselines added.

[coderabbitai]

Warning

Rate limit exceeded

@FSM1 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 59 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5b34868f-42fb-476d-bacd-4d5c23e4cdf7

📥 Commits

Reviewing files that changed from the base of the PR and between b319001 and da24fee.

📒 Files selected for processing (1)

• apps/web/src/hooks/useAuth.ts

Walkthrough

Adds Phase 21 BYO‑IPFS: SDK pinning abstractions and providers, connection testing (browser + TEE), API CID registration and advisory quota, dual‑pin orchestration, web STORAGE UI with migration controls, TEE‑backed pin migration pipeline, load tests, and security/validation artifacts.

Changes

Cohort / File(s)	Summary
Planning & Roadmap \.planning/ROADMAP.md, \.planning/PROJECT.md, \.planning/STATE.md, \.planning/REQUIREMENTS.md, \.planning/phases/21-byo-ipfs-node-support/*, \.planning/security/*, \.planning/baselines/*	Added Phase 21 planning, specs, research, validation, UI spec, baselines, summaries, verification and security review docs; marked Phase 21 complete and updated project/roadmap state and timestamps.
SDK — Pinning API & Providers packages/sdk-core/src/pinning/types.ts, packages/sdk-core/src/pinning/... (kubo-provider.ts, psa-provider.ts, pinata-provider.ts, dual-pin-provider.ts, connection-test.ts, index.ts), packages/sdk-core/src/index.ts	Introduced PinningProvider types and ConnectionTest; implemented Kubo/PSA/Pinata/DualPin providers; added testConnection logic; created barrel and package re-exports.
SDK — IPFS/Upload & Client wiring packages/sdk-core/src/ipfs/index.ts, packages/sdk-core/src/upload/index.ts, packages/sdk-core/src/__tests__/pinning/*, packages/sdk/src/*, packages/sdk-core/src/index.ts	Added registerCid, made uploadFile accept optional pinFn, added extensive pinning tests; updated CipherBoxClient to support pinningConfig, pinWithMode, emit pin:secondaryFailed, and wire external/dual pin flows.
API — BYO endpoints & Migration apps/api/src/ipfs/*, apps/api/src/vault/*, apps/api/src/migrations/*, apps/api/src/migration/*, apps/api/src/tee/*, apps/api/scripts/generate-openapi.ts, apps/api/src/app.module.ts	Added POST /ipfs/register-cid, vault isByoUser flag and service methods, advisory quota DTO, MigrationModule (entity/controller/service/processor), TeeController/TeeService forwarding, and OpenAPI/client wiring.
TEE Worker — Migration & Connection Test tee-worker/src/routes/migrate.ts, tee-worker/src/routes/connection-test.ts, tee-worker/src/services/migration-worker.ts, tee-worker/src/services/ssrf-validation.ts, tee-worker/src/index.ts	Added /migrate and /connection-test routes; migrateBatch with ECIES decrypt, SSRF/DNS validation, fetch/pin/unpin per CID, credential zeroing; SSRF utilities and safe fetch wrapper.
Web UI — Storage & Migration apps/web/src/components/settings/StorageTab.tsx, apps/web/src/components/settings/ConnectionTest.tsx, apps/web/src/components/settings/MigrationProgress.tsx, apps/web/src/components/layout/StorageQuota.tsx, apps/web/src/routes/SettingsPage.tsx, apps/web/src/hooks/useAuth.ts, apps/web/src/lib/api/migration.ts, apps/web/src/lib/sdk-provider.ts, apps/web/src/stores/quota.store.ts, apps/web/src/App.css	Added STORAGE tab, StorageTab, ConnectionTest (TEE‑routed with fallback), MigrationProgress, advisory quota UI, SDK reconfigure hook, persistence/migration save flow, and CSS styles.
Web & Load Tests packages/sdk/src/__tests__/client-pinning.test.ts, tests/load/src/**	Added client pinning unit tests and BYO load-test harness/scenarios (throughput, capacity ceiling, mixed workload) and BYO client-pool helpers.
Crypto & Vault keying packages/crypto/src/vault/derive-ipns.ts, packages/crypto/src/vault/index.ts, packages/core/src/vault/types.ts, packages/core/src/index.ts	Added deriveByoConfigIpnsKeypair, ByoIpfsConfig type and exports to support deterministic BYO IPNS key derivation and vault metadata.
Migration & Security Tests apps/api/src/migration/*, tee-worker/src/__tests__/ssrf-validation.test.ts, apps/api/src/tee/*, apps/api/src/migration/*	Added MigrationService/Processor/Controller tests, TeeService/controller tests, SSRF validation tests, and extensive processor specs.

Sequence Diagram(s)

sequenceDiagram
    participant User as User (Web UI)
    participant Web as Web App (StorageTab)
    participant SDK as SDK / Pinning
    participant API as CipherBox API
    participant Queue as BullMQ (MigrationProcessor)
    participant TEE as TEE Worker
    participant Ext as External IPFS Provider

    rect rgba(135,206,235,0.5)
    User->>Web: open StorageTab, enter endpoint/auth
    Web->>SDK: request connection test (TEE‑routed or browser)
    SDK->>Ext: probe endpoints (/api/v0/id, /pins, /data/testAuthentication)
    Ext-->>SDK: protocol result / latency / CORS
    SDK-->>Web: return test result
    end

    rect rgba(144,238,144,0.5)
    Web->>API: save BYO config (encrypted)
    API-->>Web: ack
    Web->>API: start migration (if provider changed)
    API->>Queue: enqueue migration job
    Queue->>TEE: POST /migrate (batch + encrypted configs)
    TEE->>Ext: fetch blobs, pin to dest (SSRF/DNS checks)
    TEE-->>Queue: return succeeded/failed
    Queue->>API: update migration status
    API-->>Web: migration status (polled)
    end

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• docs: capture todo - Add bring-your-own IPFS node support #124 — Prior BYO‑IPFS planning doc; strongly related to the implemented BYO architecture and accompanying docs/tests.
• perf: optimize IPFS upload with concurrent pins and pebbleds datastore #342 — Modifies upload/pinning flow; likely overlaps with uploadFile pinFn and CipherBoxClient pinning behavior introduced here.
• feat: extract core crypto SDK as shared packages #296 — Extends SDK/core exports and may overlap with the new pinning provider re-exports and SDK reconfiguration wiring.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch docs/phase-21-context

[codecov]

Codecov Report

❌ Patch coverage is 84.73945% with 123 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.06%. Comparing base (6aa9d11) to head (da24fee).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/sdk-core/src/pinning/connection-test.ts	77.51%	46 Missing and 1 partial ⚠️
packages/crypto/src/vault/derive-ipns.ts	5.00%	19 Missing ⚠️
packages/sdk/src/client.ts	77.38%	17 Missing and 2 partials ⚠️
apps/api/src/migration/migration.controller.ts	69.69%	0 Missing and 10 partials ⚠️
packages/sdk-core/src/pinning/psa-provider.ts	89.36%	10 Missing ⚠️
packages/sdk-core/src/pinning/pinata-provider.ts	93.33%	6 Missing ⚠️
apps/api/src/migration/migration.processor.ts	94.64%	0 Missing and 3 partials ⚠️
apps/api/src/ipfs/ipfs.controller.ts	80.00%	0 Missing and 2 partials ⚠️
apps/api/src/migration/migration.entity.ts	85.71%	0 Missing and 2 partials ⚠️
apps/api/src/migration/migration.service.ts	96.42%	0 Missing and 2 partials ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #346       +/-   ##
===========================================
+ Coverage   56.55%   70.06%   +13.50%     
===========================================
  Files         115      103       -12     
  Lines        8342     6103     -2239     
  Branches      748      889      +141     
===========================================
- Hits         4718     4276      -442     
+ Misses       3445     1623     -1822     
- Partials      179      204       +25     

Flag	Coverage Δ
api	83.44% <84.73%> (+0.55%)	⬆️
api-client	83.44% <84.73%> (+0.55%)	⬆️
core	83.44% <84.73%> (+0.55%)	⬆️
crypto	83.44% <84.73%> (+0.55%)	⬆️
desktop	15.88% <ø> (-15.34%)	⬇️
sdk	83.44% <84.73%> (+0.55%)	⬆️
sdk-core	83.44% <84.73%> (+0.55%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
apps/api/src/tee/tee.service.ts	98.41% <100.00%> (+0.13%)	⬆️
packages/sdk-core/src/pinning/dual-pin-provider.ts	100.00% <100.00%> (ø)
packages/sdk-core/src/pinning/kubo-provider.ts	100.00% <100.00%> (ø)
packages/sdk/src/events.ts	100.00% <ø> (ø)
apps/api/src/vault/vault.service.ts	89.70% <88.88%> (-0.46%)	⬇️
apps/api/src/ipfs/ipfs.controller.ts	84.48% <80.00%> (-1.24%)	⬇️
apps/api/src/migration/migration.entity.ts	85.71% <85.71%> (ø)
apps/api/src/migration/migration.service.ts	96.42% <96.42%> (ø)
apps/api/src/tee/tee.controller.ts	80.00% <80.00%> (ø)
apps/api/src/migration/migration.processor.ts	94.64% <94.64%> (ø)
... and 6 more

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Copilot reviewed 123 out of 126 changed files in this pull request and generated 8 comments.

Files not reviewed (1)

• tee-worker/package-lock.json: Language not supported

[coderabbitai]

Actionable comments posted: 18

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (11)

tee-worker/src/services/ssrf-validation.ts-34-36 (1)

34-36: ⚠️ Potential issue | 🟡 Minor

IPv6 prefix checks may cause false positives on legitimate hostnames.

The startsWith('fd'), startsWith('fc'), and startsWith('fe80') checks work correctly for IPv6 addresses but will incorrectly reject legitimate hostnames like fd.example.com, fc-api.cloud, or fe80live.com. Since isPrivateAddress receives both hostnames (from validateEndpointUrl) and resolved IPs (from validateResolvedIp), IPv6-specific prefix checks should only match actual IPv6 address syntax.

🛡️ Proposed fix to scope IPv6 checks

   return (
     normalized === 'localhost' ||
     normalized === '0.0.0.0' ||
     normalized === '127.0.0.1' ||
     normalized === '::1' ||
     normalized === '::' ||
     normalized.startsWith('10.') ||
     normalized.startsWith('192.168.') ||
     normalized.startsWith('127.') ||
     normalized.startsWith('169.254.') ||
-    normalized.startsWith('fd') ||
-    normalized.startsWith('fc') ||
-    normalized.startsWith('fe80') ||
+    normalized.startsWith('fd') && normalized.includes(':') ||
+    normalized.startsWith('fc') && normalized.includes(':') ||
+    normalized.startsWith('fe80:') ||
     normalized.endsWith('.internal') ||
     normalized.endsWith('.local') ||
     is172Private(normalized) ||
     isCgnRange(normalized)
   );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tee-worker/src/services/ssrf-validation.ts` around lines 34 - 36, The IPv6
prefix checks in isPrivateAddress are currently applied to raw hostnames and
cause false positives (e.g., fd.example.com); update isPrivateAddress so that
the fd|fc|fe80 prefix checks are only evaluated when the input is a
syntactically valid IPv6 address (use validateResolvedIp or an IPv6 regex/parse
before applying startsWith checks), while hostname inputs continue to be checked
only with hostname-specific logic used by validateEndpointUrl; ensure functions
named isPrivateAddress, validateEndpointUrl, and validateResolvedIp are adjusted
so IPv6-prefix logic is gated by an IPv6 detection step.

packages/sdk-core/src/ipfs/index.ts-74-89 (1)

74-89: ⚠️ Potential issue | 🟡 Minor

Move the fetch JSDoc to the actual fetch function.

Line 74 introduces registerCid, but the preceding fetch-specific JSDoc now reads like it documents this new function. This makes API docs misleading.

✏️ Proposed docblock fix

-/**
- * Fetch encrypted data from IPFS via backend relay.
- * Uses native fetch for streaming download progress support.
- * Auth token is obtained from ctx (axiosInstance interceptor isn't used
- * here because fetch provides ReadableStream for progress tracking).
- */
 /**
  * Register an externally-pinned CID with the CipherBox API for advisory quota tracking.
  * Used by BYO users who pin to their own node and need to report CID + size.
  */
 export async function registerCid(ctx: SdkContext, cid: string, sizeBytes: number): Promise<void> {
   ...
 }
 
+/**
+ * Fetch encrypted data from IPFS via backend relay.
+ * Uses native fetch for streaming download progress support.
+ * Auth token is obtained from ctx (axiosInstance interceptor isn't used
+ * here because fetch provides ReadableStream for progress tracking).
+ */
 export async function fetchFromIpfs(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sdk-core/src/ipfs/index.ts` around lines 74 - 89, The JSDoc for the
fetch function was accidentally left above registerCid, making docs misleading;
locate the misplaced JSDoc block immediately before the registerCid function and
move it to the actual fetch function declaration (symbol: fetch) so it documents
fetch behavior, then replace the block above registerCid (symbol: registerCid)
with a short, accurate JSDoc describing that it registers an externally-pinned
CID and its parameters/return type; verify wording matches each function's
implementation and remove any fetch-specific references from registerCid's doc.

.planning/ROADMAP.md-184-184 (1)

184-184: ⚠️ Potential issue | 🟡 Minor

Phase 23 plan count is inconsistent.

Line 184 now says 11/11 complete, but this section still only lists 23-01 through 23-08, and the progress table still shows 8/8. Please reconcile the counts before treating this roadmap update as authoritative.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.planning/ROADMAP.md at line 184, Phase 23 shows inconsistent plan counts:
the header line reads "11/11" while the listed items are "23-01" through "23-08"
and the progress table shows "8/8"; fix by reconciling them either by adding the
missing plan entries (23-09, 23-10, 23-11) and updating the progress table and
list to reflect 11/11, or by changing the header "11/11" to "8/8" and ensuring
the progress table and list consistently show 8/8; update the occurrences of the
"11/11" string, the Phase 23 list entries ("23-01"… "23-08"), and the progress
table entry so all three match.

apps/web/src/lib/api/migration.ts-35-44 (1)

35-44: ⚠️ Potential issue | 🟡 Minor

Consider distinguishing "no migration" from request failures.

The current implementation returns null for any error, which makes it impossible for callers to distinguish between:

• 404: No migration exists (legitimate null)
• 500: Server error (should surface to user)
• Network timeout: Connectivity issue (should retry or alert)

This could lead to confusing UX where migration status appears as "none" during server outages.

Suggested improvement

   async getStatus(): Promise<MigrationStatus | null> {
     try {
       return await customInstance<MigrationStatus>({
         url: '/migration/status',
         method: 'GET',
       });
-    } catch {
-      return null; // No migration exists
+    } catch (err) {
+      // 404 means no migration exists
+      if (err && typeof err === 'object' && 'status' in err && err.status === 404) {
+        return null;
+      }
+      // Re-throw other errors (500, network issues) so callers can handle appropriately
+      throw err;
     }
   },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/lib/api/migration.ts` around lines 35 - 44, The getStatus method
currently swallows all errors and returns null; change getStatus (and its call
to customInstance) to distinguish a 404 "no migration" case from other failures
by inspecting the thrown error (e.g., check error.response?.status === 404) and
returning null only for 404, while rethrowing or returning a rejected error for
5xx or network errors so callers can surface/retry them; reference getStatus,
customInstance, and MigrationStatus when making this change.

apps/api/src/vault/vault.service.ts-154-156 (1)

154-156: ⚠️ Potential issue | 🟡 Minor

Don't let setByoStatus() silently succeed on a missing vault.

Repository.update() no-ops when no row matches ownerId. Right now callers can get a successful response even though BYO mode was never persisted.

🛠️ Proposed fix

   async setByoStatus(userId: string, isByo: boolean): Promise<void> {
-    await this.vaultRepository.update({ ownerId: userId }, { isByoUser: isByo });
+    const result = await this.vaultRepository.update(
+      { ownerId: userId },
+      { isByoUser: isByo }
+    );
+    if (result.affected === 0) {
+      throw new NotFoundException('Vault not found');
+    }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/vault/vault.service.ts` around lines 154 - 156, setByoStatus
currently calls this.vaultRepository.update(...) which no-ops when no vault
matches ownerId; change setByoStatus to inspect the update result from
this.vaultRepository.update({ ownerId: userId }, { isByoUser: isByo }) and if
the result indicates zero rows affected (e.g., affected === 0) throw a clear
error (e.g., NotFound or a descriptive Error) so callers do not receive a silent
success when no vault exists.

tests/load/src/scenarios/byo-upload-throughput.test.ts-32-33 (1)

32-33: ⚠️ Potential issue | 🟡 Minor

Guard LOAD_TEST_CLIENTS before using it.

parseInt can produce NaN, 0, or a negative value here, and that flows straight into the pool config and test title. Falling back to a sane positive default makes this scenario less brittle in CI.

🛠️ Proposed fix

-const NUM_CLIENTS = parseInt(process.env.LOAD_TEST_CLIENTS ?? '10', 10);
+const parsedNumClients = Number.parseInt(process.env.LOAD_TEST_CLIENTS ?? '10', 10);
+const NUM_CLIENTS =
+  Number.isFinite(parsedNumClients) && parsedNumClients > 0 ? parsedNumClients : 10;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/load/src/scenarios/byo-upload-throughput.test.ts` around lines 32 - 33,
Guard the parsed LOAD_TEST_CLIENTS value before using it: validate the result of
parseInt(process.env.LOAD_TEST_CLIENTS ?? '10', 10) assigned to NUM_CLIENTS and
if it is NaN, <= 0, or otherwise invalid, replace it with a sane positive
default (e.g., 10) so the pool config and test title never receive
0/negative/NaN; update the NUM_CLIENTS initialization logic (and any uses of
NUM_CLIENTS) to rely on this validated fallback.

apps/web/src/components/settings/MigrationProgress.tsx-101-109 (1)

101-109: ⚠️ Potential issue | 🟡 Minor

Render failed as a terminal state.

failed currently falls through to the generic migrating: x/y pins copy, so the UI still reads as active after polling has already stopped.

🧭 Proposed fix

-      {isComplete ? (
+      {migration.status === 'failed' ? (
+        <p className="migration-progress-text migration-failed">
+          {`> migration failed. ${migration.failedCids} pins could not be transferred.`}
+        </p>
+      ) : isComplete ? (
         <p className="migration-progress-text migration-complete">
           {`> migration complete. ${migration.totalCids} pins transferred.`}
         </p>
       ) : (

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/settings/MigrationProgress.tsx` around lines 101 -
109, The UI treats a failed migration as still "migrating"; update the render
logic in MigrationProgress.tsx to explicitly handle the failed terminal state by
checking migration.status (or a derived isFailed) before the isComplete branch
and rendering a clear failure message (e.g., "migration failed: X/Y pins
transferred" or include migration.error if available) instead of the generic
"migrating" copy; modify the conditional that currently uses isComplete and
migration.migratedCids/migration.totalCids to first test for migration.status
=== 'failed' (or add an isFailed constant) and return the failure paragraph when
true so the UI reflects the terminal failed state.

.planning/phases/21-byo-ipfs-node-support/21-VERIFICATION.md-1-5 (1)

1-5: ⚠️ Potential issue | 🟡 Minor

This report currently overstates Phase 21 completion.

Truth #1 is still marked ? HUMAN, but the frontmatter and score say 5/5 success criteria verified, and Truth #2 is worded as if every BYO upload is dual-pin. That contradicts the actual Phase 21 criterion here, which is mode-specific behavior (cipherbox / external / dual). Please reword the truth and downgrade the score until the human-only item is actually signed off.

As per coding guidelines, .planning/ROADMAP.md: "upload honors selected pinning mode (cipherbox/external/dual-pin)".

Also applies to: 42-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.planning/phases/21-byo-ipfs-node-support/21-VERIFICATION.md around lines 1
- 5, The verification report's frontmatter and score falsely claim full
completion while "Truth `#1`" remains `? HUMAN` and "Truth `#2`" inaccurately states
that every BYO upload is dual-pin; update the frontmatter fields (`verified`,
`status`, `score`) to reflect that human sign-off is still required (e.g., set
`status: human_needed`, lower `score`), and reword "Truth `#2`" to describe
mode-specific behavior (cipherbox/external/dual) rather than asserting universal
dual-pin; apply the same corrections to the corresponding sections for phases
42–50 referenced in the comment.

tests/load/src/scenarios/byo-mixed-workload.test.ts-62-79 (1)

62-79: ⚠️ Potential issue | 🟡 Minor

Clean up any fulfilled pool before rethrowing setup failures.

If createClientPool() rejects but createByoClientPool() succeeds, Line 79 throws before byoPool is assigned. afterAll() then sees two empty arrays, so the successful BYO clients never get destroyed and can leave open handles behind for later scenarios.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/load/src/scenarios/byo-mixed-workload.test.ts` around lines 62 - 79, If
one Promise in the Promise.allSettled pair (createClientPool /
createByoClientPool) fails while the other fulfills, ensure you clean up the
fulfilled pool before rethrowing: check cbResult and byoResult statuses, assign
their resolved values to cbPool and byoPool (or temp variables) when fulfilled,
and if you need to throw due to a rejection (e.g., cbResult.status ===
'rejected'), call the corresponding pool teardown/destroy method on the other
fulfilled result (the BYO pool returned by createByoClientPool) before
rethrowing the error; reference createClientPool, createByoClientPool, cbResult,
byoResult, cbPool and byoPool to locate and implement this cleanup logic.

packages/sdk/src/__tests__/client-pinning.test.ts-106-373 (1)

106-373: ⚠️ Potential issue | 🟡 Minor

Add a client-level Pinata routing case.

This suite exercises the kubo and psa branches, but not the new protocol: 'pinata' path in CipherBoxClient. A small happy-path test here would catch regressions where the constructor or pinWithMode() stops selecting PinataProvider even though the provider unit tests still pass.

As per coding guidelines, **/*.test.ts: "Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sdk/src/__tests__/client-pinning.test.ts` around lines 106 - 373,
Add a new happy-path test in this file that exercises the client-level "pinata"
routing: create a config with pinningConfig.externalProvider.protocol set to
'pinata', instantiate CipherBoxClient, call uploadFile (use existing
setupFolder, mocks for sdkCore.addFilePointerToFolder and
sdkCore.updateFolderMetadataAndPublish), stub global fetch to return a
successful Pinata-style response, then assert that sdkCore.addToIpfs was called
(CipherBox relay used for CID), that the stubbed fetch was called with a /pins
POST containing the CID, that sdkCore.unpinFromIpfs was called to clean up, and
that sdkCore.registerCid was invoked; reference CipherBoxClient, pinWithMode,
PinataProvider, uploadFile, setupFolder and sdkCore.* in the test to locate
where to add this case.

apps/web/src/components/settings/StorageTab.tsx-177-208 (1)

177-208: ⚠️ Potential issue | 🟡 Minor

Derived IPNS private key should be zeroed after use.

The byoKeypair.privateKey is used for ECIES wrapping (line 196) and IPNS publishing (line 202) but is never zeroed. Per the security guidelines, credentials should be cleared from memory after use.

🛡️ Proposed fix to zero the derived keypair

       await createAndPublishIpnsRecord({
         ipnsPrivateKey: byoKeypair.privateKey,
         ipnsName: byoKeypair.ipnsName,
         metadataCid: cid,
         sequenceNumber,
         encryptedIpnsPrivateKey,
         keyEpoch,
       });
+
+      // Zero derived IPNS private key after use
+      byoKeypair.privateKey.fill(0);

       localStorage.setItem(BYO_IPNS_NAME_KEY, byoKeypair.ipnsName);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/settings/StorageTab.tsx` around lines 177 - 208, The
derived private key returned by deriveByoConfigIpnsKeypair
(byoKeypair.privateKey) remains in memory after use; overwrite/zero it
immediately after its last use to avoid lingering credentials: after computing
wrappedKey (wrapKey) and again after the createAndPublishIpnsRecord call,
securely wipe the byte array backing byoKeypair.privateKey (e.g., fill with
zeros) and then clear or unset the reference (e.g., set to undefined) so the
private key is not retained in memory; ensure any intermediate variables that
contain its bytes (if copied, e.g., wrappedKey/teePublicKey copies) are also
cleared where appropriate.

🧹 Nitpick comments (10)

tee-worker/src/services/ssrf-validation.ts (1)

80-85: Consider handling multiple DNS records for additional robustness.

dns.lookup returns only the first resolved address. An attacker could configure DNS to return a public IP first (passing validation) and a private IP second. While this is a lower-risk edge case (the actual fetch will use OS resolver which may pick differently), using dns.resolve4/dns.resolve6 to check all A/AAAA records would provide stronger protection.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tee-worker/src/services/ssrf-validation.ts` around lines 80 - 85, The
validateResolvedIp function currently uses dns.lookup and only checks the first
address; update it to resolve all A and AAAA records (using dns.resolve4 and
dns.resolve6) and verify every returned IP with isPrivateAddress, throwing an
error if any record is private; handle the case where resolve4/resolve6 may
throw or return empty by falling back to lookup(hostname) so validateResolvedIp
still works with single-record hosts, and ensure both IPv4 and IPv6 results are
checked before returning.

tee-worker/src/__tests__/ssrf-validation.test.ts (1)

78-82: Consider adding test for legitimate hostnames with IPv6-like prefixes.

To validate the fix for the false positive issue, consider adding a test case for hostnames that start with fd, fc, or fe80 but are not IPv6 addresses:

it('accepts legitimate hostnames starting with fd/fc/fe80', () => {
  expect(() => validateEndpointUrl('https://fd.example.com')).not.toThrow();
  expect(() => validateEndpointUrl('https://fc-api.cloud')).not.toThrow();
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tee-worker/src/__tests__/ssrf-validation.test.ts` around lines 78 - 82, Add
tests to ensure validateEndpointUrl does not falsely treat hostnames beginning
with IPv6-like prefixes as IPv6 addresses: add an it block (e.g., "accepts
legitimate hostnames starting with fd/fc/fe80") that calls validateEndpointUrl
with hostnames like "https://fd.example.com", "https://fc-api.cloud" and
"https://fe80-host.local" and asserts they do not throw; place this alongside
the existing IPv6 tests to verify the function distinguishes bracketed IPv6
literals (e.g., "[fd00::1]") from normal hostnames.

apps/api/src/migration/migration.entity.ts (1)

27-28: Consider adding an index on status for operational queries.

If you anticipate queries like "find all running migrations" or background jobs that poll by status, an index on the status column would improve query performance. This is optional given the likely low volume of migration records.

+ `@Index`()
  `@Column`({ type: 'varchar', length: 20 })
  status!: MigrationStatus;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/migration/migration.entity.ts` around lines 27 - 28, Add a
database index on the status column to speed up queries that filter by status:
import and use TypeORM's `@Index`() decorator above the status property (e.g., add
"import { Index } from 'typeorm'" and place "@Index()" directly above "@Column({
type: 'varchar', length: 20 })" on the status field typed as MigrationStatus) so
the generated schema includes an index for Migration.status.

packages/sdk-core/src/pinning/dual-pin-provider.ts (1)

42-50: Expose secondary unpin failures to the caller.

pin() reports secondary-provider failures, but unpin() drops them entirely. That makes retries impossible and can leave deleted content pinned on the secondary provider after a transient outage.

♻️ Possible hardening

-  async unpin(cid: string): Promise<void> {
+  async unpin(
+    cid: string
+  ): Promise<{ secondarySuccess: boolean; secondaryError?: string }> {
     // Unpin from both, primary must succeed
     await this.primary.unpin(cid);
-    // Secondary unpin is best-effort
+    let secondarySuccess = true;
+    let secondaryError: string | undefined;
     try {
       await this.secondary.unpin(cid);
-    } catch {
-      // Ignore secondary unpin failure
+    } catch (err) {
+      secondarySuccess = false;
+      secondaryError = err instanceof Error ? err.message : String(err);
     }
+    return { secondarySuccess, secondaryError };
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sdk-core/src/pinning/dual-pin-provider.ts` around lines 42 - 50, The
unpin method on DualPinProvider currently swallows secondary unpin errors,
preventing callers from retrying or noticing secondary failures; modify async
unpin(cid: string) so it still awaits this.primary.unpin(cid) (primary must
succeed) but when this.secondary.unpin(cid) throws capture that error and
surface it to the caller—either by rethrowing the secondary error after primary
succeeds or by throwing an aggregated/annotated error (e.g., SecondaryUnpinError
or including the original error message) so callers can detect and handle/retry
secondary failures; reference the unpin method and the
this.primary/this.secondary members and ensure the thrown error preserves the
original error details.

packages/sdk-core/src/upload/index.ts (1)

92-96: Validate pinFn output before trusting it.

Since pinFn is a pluggable override, add a small guard for cid and size to fail fast on malformed implementations.

🛡️ Proposed hardening

     const pinResult = params.pinFn
       ? await params.pinFn(params.ctx, ciphertext, params.onProgress)
       : await addToIpfs(params.ctx, ciphertext, params.onProgress);
+    if (!pinResult.cid || !Number.isFinite(pinResult.size) || pinResult.size < 0) {
+      throw new Error('Invalid pin result from pinFn');
+    }
     const { cid, size: encryptedSize } = pinResult;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sdk-core/src/upload/index.ts` around lines 92 - 96, The code trusts
the pluggable pinFn/addToIpfs result without validation; after calling pinFn or
addToIpfs (the variable pinResult returned by pinFn or addToIpfs in the upload
flow), validate that pinResult is an object containing a non-empty cid and a
numeric size (or size-like field) before using them — if validation fails, throw
a clear error indicating the malformed pinFn output and include context from
params (e.g., params.ctx or params.pinFn) so callers can debug; update the
destructuring/usage of cid and encryptedSize to occur only after this guard.

packages/sdk-core/src/__tests__/pinning/connection-test.test.ts (1)

15-162: Add coverage for the timeout and normalization contract.

This suite exercises protocol detection well, but it never proves the 10s abort path or trailing-slash normalization. Regressions in either behavior would still leave CI green.

As per coding guidelines, "Connection test must sequentially probe Kubo then PSA with a 10s timeout per probe and return protocol-specific CORS remediation instructions" and requires "endpoint normalization (strip trailing slashes)."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sdk-core/src/__tests__/pinning/connection-test.test.ts` around lines
15 - 162, The tests currently miss verification of the 10s abort timeout per
probe and of endpoint trailing-slash normalization; add two tests to cover
these: (1) "times out after 10s per probe" — use jest.useFakeTimers(), have
mockFetch for the Kubo probe return a Promise that never resolves (or resolves
after >10_000ms) and advance timers by 10_001ms to assert
testConnection(endpoint) returns a timeout/abort-style error (or that mockFetch
call was aborted) and that the PSA probe is then attempted and also respects the
10s timeout; (2) "normalizes endpoint by stripping trailing slash" — call
testConnection(endpoint + '/') and assert the mocked fetch was invoked with
`${endpoint}/api/v0/id` and `${endpoint}/pins` (no double slash) and that
Authorization header behavior remains unchanged; reference testConnection and
the probe endpoints (/api/v0/id and /pins) and use
mockFetch.mockImplementationOnce / mockResolvedValueOnce / mockRejectedValueOnce
plus jest.advanceTimersByTime to simulate the timeout and verify behavior.

tee-worker/src/services/migration-worker.ts (1)

104-106: Token strings decoded early — consider late decoding per-request.

The sourceToken and destToken strings are decoded before the loop and persist for the entire batch duration. Per the security review, JS strings are immutable and cannot be zeroed. Decoding the token inside each fetch call (late decoding) would reduce the token's lifetime in memory, though the practical impact is limited given the batch context.

♻️ Optional: Late token decoding pattern

-  // Decode auth tokens once for the entire batch instead of per-CID
-  const sourceToken = authTokenString(sourceConfig);
-  const destToken = authTokenString(destConfig);
-
   try {
     for (const cid of cids) {
       try {
         // 3. Fetch encrypted blob from source
-        const data = await fetchFromProvider(cid, sourceConfig, sourceToken);
+        const data = await fetchFromProvider(cid, sourceConfig);

         // 4. Pin to destination
-        const destCid = await pinToProvider(data, cid, destConfig, destToken);
+        const destCid = await pinToProvider(data, cid, destConfig);

Then modify fetchFromProvider/pinToProvider to decode the token inline:

async function fetchFromProvider(cid: string, config: ProviderConfig): Promise<Uint8Array> {
  const token = new TextDecoder().decode(config.authTokenBytes);
  // ... use token ...
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tee-worker/src/services/migration-worker.ts` around lines 104 - 106, The
current code decodes auth tokens once into sourceToken/destToken before the
batch loop, keeping immutable JS strings in memory; instead, remove the
pre-decoded sourceToken and destToken and perform late decoding inside each
network call: decode config.authTokenBytes into a local token string at the
start of fetchFromProvider and pinToProvider (e.g., with a
TextDecoder().decode(config.authTokenBytes)) so the decoded string is scoped to
the request and not stored for the batch duration; update calls that previously
relied on sourceToken/destToken to read the token from the passed ProviderConfig
within those functions.

tee-worker/src/routes/connection-test.ts (1)

68-81: Token bytes created but string used for probing.

Line 69 creates tokenBytes for zeroing, but lines 81 and 87 pass authToken (the string) to probe functions. The tokenBytes is zeroed in finally but the original string persists. For consistency, consider either:

1. Not creating tokenBytes (since string cannot be zeroed anyway), or
2. Passing tokenBytes to probe functions and decoding inline

This is a minor inconsistency; the practical security impact is limited given the short-lived request context.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tee-worker/src/routes/connection-test.ts` around lines 68 - 81, The code
creates a Uint8Array tokenBytes from authToken for zeroing but then passes the
original authToken string into probeKubo (and probePsa elsewhere), leaving the
string live; either remove tokenBytes if you won't zero the string, or
consistently use tokenBytes when calling the probes by decoding it where
probeKubo/probePsa expect a string (e.g., pass a decoded string derived from
tokenBytes or change the probe APIs to accept Uint8Array), and ensure tokenBytes
is zeroed in the finally block; update calls to probeKubo(normalizedEndpoint,
...) and probePsa(...) to use the consistent token representation (tokenBytes or
decoded-from-tokenBytes) and keep zeroing logic intact.

packages/sdk-core/src/pinning/pinata-provider.ts (1)

92-117: Consider continuing on partial unpin failures.

If multiple files share the same CID and one delete fails, the loop throws immediately, leaving remaining files still pinned. For robustness, consider collecting errors and throwing after all attempts.

♻️ Optional: Collect errors and throw at end

   async unpin(cid: string): Promise<void> {
     const listResponse = await fetch(`${this.endpoint}/v3/files?cid=${encodeURIComponent(cid)}`, {
       method: 'GET',
       headers: { Authorization: `Bearer ${this.authToken}` },
       signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS),
     });

     if (!listResponse.ok) {
       throw new Error(`Pinata list failed: ${listResponse.status}`);
     }

     const listResult = (await listResponse.json()) as {
       data: { files: Array<{ id: string }> };
     };

+    const errors: string[] = [];
     for (const file of listResult.data.files) {
       const deleteResponse = await fetch(`${this.endpoint}/v3/files/${file.id}`, {
         method: 'DELETE',
         headers: { Authorization: `Bearer ${this.authToken}` },
         signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS),
       });
       if (!deleteResponse.ok) {
-        throw new Error(`Pinata delete failed: ${deleteResponse.status}`);
+        errors.push(`Delete ${file.id} failed: ${deleteResponse.status}`);
       }
     }
+    if (errors.length > 0) {
+      throw new Error(`Pinata unpin partial failure: ${errors.join('; ')}`);
+    }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sdk-core/src/pinning/pinata-provider.ts` around lines 92 - 117, In
unpin, don't abort on the first failed delete for cid: iterate over
listResult.data.files and attempt each DELETE, collecting failures (e.g.,
file.id and status/error) into an array instead of immediately throwing inside
the loop; after the loop, if any failures were collected, throw a single
aggregated Error that includes which file ids failed and their statuses/messages
so callers know which deletions succeeded vs. failed while still attempting
every delete. Ensure the initial listResponse error still throws immediately,
but change the DELETE handling in unpin to continue on error and report
aggregated results at the end.

apps/api/src/migration/migration.service.ts (1)

158-178: Potential race condition in updateProgress without transaction.

The read-modify-write pattern (lines 164-177) could have a race condition if two batch completions call updateProgress simultaneously. While BullMQ typically serializes job processing, consider using a transaction or increment() for atomic counter updates.

♻️ Suggested: Use atomic increment

   async updateProgress(
     migrationId: string,
     migratedDelta: number,
     failedDelta: number,
     failedCids?: string[]
   ): Promise<void> {
-    const migration = await this.migrationRepo.findOneOrFail({
-      where: { id: migrationId },
-    });
-
-    migration.migratedCids += migratedDelta;
-    migration.failedCids += failedDelta;
-
-    if (failedCids && failedCids.length > 0) {
-      const existingList = migration.failedCidList ? migration.failedCidList.split(',') : [];
-      existingList.push(...failedCids);
-      migration.failedCidList = existingList.join(',');
-    }
-
-    await this.migrationRepo.save(migration);
+    await this.migrationRepo.increment(
+      { id: migrationId },
+      'migratedCids',
+      migratedDelta
+    );
+    await this.migrationRepo.increment(
+      { id: migrationId },
+      'failedCids',
+      failedDelta
+    );
+
+    if (failedCids && failedCids.length > 0) {
+      // For failed CID list, still need read-modify-write but less critical
+      const migration = await this.migrationRepo.findOneOrFail({
+        where: { id: migrationId },
+      });
+      const existingList = migration.failedCidList ? migration.failedCidList.split(',') : [];
+      existingList.push(...failedCids);
+      migration.failedCidList = existingList.join(',');
+      await this.migrationRepo.save(migration);
+    }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/migration/migration.service.ts` around lines 158 - 178, The
updateProgress method has a read-modify-write race on migration.migratedCids,
migration.failedCids and failedCidList when concurrent callers invoke it; change
it to perform atomic updates instead of loading and saving the entity directly:
use the repository/query builder increment() (or an explicit transaction with
SELECT FOR UPDATE) to atomically increment migratedCids and failedCids, and for
failedCidList either update it inside the same transaction (fetch with FOR
UPDATE, append and save) or perform a single UPDATE that concatenates the new
CSV fragment to failedCidList; update references to updateProgress,
migrationRepo, migratedCids, failedCids and failedCidList accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/api/src/migration/migration.controller.ts`:
- Around line 18-21: Add route- or controller-level rate limiting to protect the
migration endpoints that mutate state: apply a throttling guard or decorator
(e.g., NestJS Throttle or your org's RateLimitGuard) to the MigrationController
class or specifically to the start, pause, resume, and cancel handlers so a
single account cannot flood the queue; locate the class decorated with
`@Controller`('migration') and the methods start, pause, resume, cancel and attach
the chosen rate-limiting mechanism with sensible limits (requests per minute and
a burst limit) and ensure the guard is registered in the controller providers or
globally if required.
- Line 11: The GET /migration/status OpenAPI response is documented as non-null
while the method returns MigrationStatusDto | null; import ApiExtraModels and
getSchemaPath from '@nestjs/swagger', add `@ApiExtraModels`(MigrationStatusDto) to
the controller class, and update the `@ApiResponse` on the status endpoint to use
schema: { allOf: [{ $ref: getSchemaPath(MigrationStatusDto) }], nullable: true }
so the response is marked nullable; after updating the decorator, regenerate the
client with pnpm api:generate.

In `@apps/api/src/migration/migration.processor.ts`:
- Around line 72-83: The fetch to `${TEE_URL}/migrate` must be bounded by a
timeout using an AbortController so the TEE call cannot hang forever; wrap the
fetch in an AbortController with a configurable timeout, pass controller.signal
to fetch, and clear the timeout on success. Add error handling for
abort/timeouts and other network errors: catch the AbortError / timeout and
convert that path into an explicit migration transition (e.g., call the existing
migration update routine to mark the migration as paused/failed and release the
BullMQ worker slot), and on other failures mark the migration as failed and log
the response/error (references: TEE_URL, TEE_SECRET, cids, migration, and the
function handling this fetch in migration.processor.ts). Ensure the code still
reads response status/body when not aborted and only treats timeouts as the
paused transition.

In `@apps/api/src/migration/migration.service.ts`:
- Around line 140-152: The migration controller lacks rate limiting guards;
import BypassableThrottlerGuard (or ThrottlerGuard if bypassable is named
differently) and apply `@UseGuards`(JwtAuthGuard, ThrottlerGuard) at the
MigrationController class level (ensure you also import JwtAuthGuard there),
replacing the current `@UseGuards`(JwtAuthGuard) usage; update the controller file
to import BypassableThrottlerGuard/ThrottlerGuard from the throttler module and
add it to the class decorator so all migration endpoints are protected by the
throttler guard.

In `@apps/web/src/components/settings/ConnectionTest.tsx`:
- Around line 3-4: The plaintext credential buffers created around the TEE call
(the Uint8Array `configBytes` and any intermediate arrays produced while
building `{ endpoint, authToken }` and converting to hex via
`bytesToHex`/`hexToBytes`) must be explicitly zeroed in a `finally` block after
`teeControllerConnectionTest()` completes; update the component (ConnectionTest/
function that builds `configBytes`) to move decoding as late as possible, wrap
the TEE call in try/catch/finally, and overwrite/clear `configBytes` and any
temporary byte arrays (and avoid keeping hex/plain JS string copies alive longer
than necessary) in the `finally` block so plaintext buffers are erased
regardless of success or error.

In `@apps/web/src/components/settings/MigrationProgress.tsx`:
- Around line 43-45: The current useEffect uses setInterval which can overlap
async fetchStatus calls; replace it with a single-flight recursive setTimeout
loop: start by calling fetchStatus(), then in its .finally/after-await schedule
pollRef.current = setTimeout(loop, 5000) so a new poll is only scheduled after
the previous fetchStatus completes; ensure you still call
clearTimeout(pollRef.current) in the useEffect cleanup and handle component
unmount/cancel to avoid scheduling further polls; update references in
MigrationProgress.tsx to use this loop instead of setInterval while keeping
fetchStatus and pollRef names.
- Around line 34-40: The current logic in MigrationProgress.tsx clears the
polling interval whenever status is falsy, which stops polling permanently if
the status endpoint returns null; update the logic so the interval is only
cleared when status is in TERMINAL_STATUSES (keep polling when status === null)
or, alternatively, ensure the parent restarts polling after
migrationApi.start(...) — specifically modify the polling teardown check around
pollRef and TERMINAL_STATUSES (referenced symbols: pollRef, TERMINAL_STATUSES,
status, MigrationProgress component) so that clearing the interval happens only
for terminal states or add a restart hook in the parent after invoking
migrationApi.start to resume polling.

In `@apps/web/src/hooks/useAuth.ts`:
- Around line 229-250: The cached key name 'cipherbox-byo-ipns-name' is global
and causes cross-account reuse and permanent stale-cache behavior; modify the
cache handling in useAuth.ts (around deriveByoConfigIpnsKeypair,
BYO_IPNS_NAME_KEY, and the fetch/unwrap logic) to namespace the localStorage key
per-account (e.g., append a stable user identifier or a fingerprint of
userPrivateKey) and, on decryption/JSON parse failure, remove the stored value
so it can be re-resolved (clear localStorage entry when unwrapKey/JSON.parse
throws); also ensure you still set the namespaced key when retrieval succeeds.
- Around line 256-259: The current catch block in useAuth.ts swallows any
resolve/decrypt/parse failures and returns undefined, which causes the code path
at the SDK reinitialization (around the code that reinitializes the SDK at line
~287) to silently downgrade BYO users to cipherbox-only; instead, change the
error handling so that genuine "no persisted BYO config" still returns undefined
but any failure during load/decrypt/parse throws or returns an explicit error
state so the caller (useAuth or the SDK initialization flow) can surface a
broken BYO configuration and avoid reinitializing to the default pinning mode;
locate the try/catch near the BYO load logic in useAuth.ts, distinguish "not
found" vs "load failed" cases, and propagate the error (throw or return a named
error object) so the SDK init code at the reinitialization site can prevent
silent downgrade and surface the failure to the user/admin.

In `@packages/sdk-core/src/pinning/connection-test.ts`:
- Around line 249-257: The current isCorsError function misclassifies generic
fetch TypeErrors as CORS failures; stop using the TypeError message heuristic.
Replace or change isCorsError to avoid returning true for generic "Failed to
fetch" TypeErrors and instead either (a) return false (treat as unknown/network
error) and let callers surface the original error, or (b) rename to
isLikelyCorsError and only return true on a reliable indicator (e.g., explicit
"cors" text or an explicit probe result). Update callers (e.g.,
runConnectionTest and any connectionProbe logic) to: 1) not terminate fallback
based solely on isCorsError, 2) surface the original error when it’s a
network/unknown failure, and 3) optionally perform a dedicated CORS probe or
accept an explicit flag from the caller to detect CORS before showing
CORS-specific instructions.
- Around line 259-261: The isTimeoutError helper only detects AbortError names;
update it to also detect timeouts from AbortSignal.timeout by returning true
when the error's name is "TimeoutError" (in addition to "AbortError"), e.g.,
inside function isTimeoutError(err: unknown) check for err being a DOMException
and err.name === 'AbortError' || err.name === 'TimeoutError' (or otherwise
inspect err?.name === 'TimeoutError') so probe timeouts are correctly
classified.
- Around line 81-88: The code currently treats HTTP 401/403/422 as an
authentication failure; update the logic in
packages/sdk-core/src/pinning/connection-test.ts so that 401 and 422 map to an
authentication error while 403 returns a distinct "forbidden / CORS/origin
issue" message (or inspects the response body for finer-grained diagnosis) —
change the conditional that checks response.status to separate 403 from 401/422
and return protocol: 'kubo' with latencyMs but error: 'forbidden: request
blocked (possible CORS/origin issue); ensure Origin header is allowed' for 403,
keeping the existing 'authentication failed. check your auth token.' message for
401/422.

In `@packages/sdk-core/src/pinning/psa-provider.ts`:
- Around line 129-152: The psa-provider.status method currently allows fetch
errors/timeouts to throw; wrap the network call and parsing in a try/catch
inside status (the same method name in
packages/sdk-core/src/pinning/psa-provider.ts) so any thrown error, non-OK
response, or parse failure returns { cid, status: 'failed' } instead of
propagating; keep using this.buildHeaders() and
AbortSignal.timeout(REQUEST_TIMEOUT_MS) as before but catch exceptions around
fetch/response.json() and return the failed PinStatus to match
KuboProvider.status() behavior.

In `@packages/sdk/src/client.ts`:
- Around line 80-90: The constructor currently silently falls back to CipherBox
when config.pinningConfig.mode !== 'cipherbox' but
config.pinningConfig.externalProvider is missing; update the initialization in
packages/sdk/src/client.ts to validate pinningConfig: if pinningConfig.mode is
'external' or 'dual' (i.e., any non-'cipherbox' mode) and
pinningConfig.externalProvider is null/undefined, throw an explicit error
instead of defaulting to relay; keep the existing branch that instantiates
this.externalProvider using sdkCore.KuboProvider, sdkCore.PinataProvider, or
sdkCore.PsaProvider when externalProvider is present, and ensure the validation
runs before any fallback logic that would use CipherBox so upload honors the
selected mode.
- Around line 1056-1071: The current flow uploads via sdkCore.addToIpfs, then
calls (this.externalProvider as sdkCore.PsaProvider).pinByCid(relayResult.cid)
and immediately calls sdkCore.unpinFromIpfs(relayResult.cid); change this so you
do not unpin the relay copy until the PSA reports a durable pin state (e.g.,
poll PSA pin status via a status/check method on sdkCore.PsaProvider or pin
status endpoint until it returns 'pinned' or a terminal failure) for
relayResult.cid, and only then call sdkCore.unpinFromIpfs; also do not silently
swallow unpin errors—surface them (log and/or throw) or implement a
retry/backoff and record failures so cleanup issues are visible; keep
sdkCore.registerCid only after successful durable pin confirmation or handle
rollback on failures accordingly.

In `@packages/sdk/src/types.ts`:
- Around line 17-20: PinningConfig currently allows selecting a non-default
PinningMode without supplying ExternalProviderConfig, creating invalid SDK
configs; change PinningConfig into a discriminated union so that the variant
with the default mode (e.g., the CipherBox/default mode) has no
externalProvider, and the other variant(s) require externalProvider:
ExternalProviderConfig (e.g., type PinningConfig = { mode: DefaultMode } | {
mode: Exclude<PinningMode, DefaultMode>; externalProvider:
ExternalProviderConfig }). Update any other similar declarations referenced
around the file (the related types at the other occurrence) to follow the same
discriminated-union pattern and ensure consumers must provide externalProvider
when choosing a non-default mode.

In `@tee-worker/src/routes/migrate.ts`:
- Around line 25-46: The /migrate handler currently only checks cids.length but
still allows empty arrays, non-string CID entries, very long CID strings, and
oversized encrypted config payloads; update the router.post('/migrate') handler
to reject empty batches, verify every entry in cids is a non-empty string and <=
a new MAX_CID_LENGTH (e.g., 255), and enforce maximum lengths for
sourceConfigEncrypted and destConfigEncrypted using a new MAX_CONFIG_LENGTH
(e.g., 64KB); return 400 with clear error messages when validations fail and
perform these checks before any heavy work or calling migrateBatch() so
oversized or malformed inputs are rejected early.

In `@tee-worker/src/services/migration-worker.ts`:
- Around line 88-99: The hostname DNS is resolved during validation
(validateResolvedIp) but ssrfSafeFetch does a fresh resolution at fetch time,
creating a TOCTOU DNS rebinding gap; modify the migration-worker flow to pin the
validated IP returned by validateResolvedIp and use that IP for the actual
request (e.g., construct the fetch URL with the validated IP and set the
original hostname in the Host header) or alternatively resolve again immediately
before ssrfSafeFetch and assert it equals the earlier validated IP; update the
code paths that call validateResolvedIp and ssrfSafeFetch so the validated IP is
passed through (or rechecked) to ensure the fetch uses the same IP that was
validated.

---

Minor comments:
In @.planning/phases/21-byo-ipfs-node-support/21-VERIFICATION.md:
- Around line 1-5: The verification report's frontmatter and score falsely claim
full completion while "Truth `#1`" remains `? HUMAN` and "Truth `#2`" inaccurately
states that every BYO upload is dual-pin; update the frontmatter fields
(`verified`, `status`, `score`) to reflect that human sign-off is still required
(e.g., set `status: human_needed`, lower `score`), and reword "Truth `#2`" to
describe mode-specific behavior (cipherbox/external/dual) rather than asserting
universal dual-pin; apply the same corrections to the corresponding sections for
phases 42–50 referenced in the comment.

In @.planning/ROADMAP.md:
- Line 184: Phase 23 shows inconsistent plan counts: the header line reads
"11/11" while the listed items are "23-01" through "23-08" and the progress
table shows "8/8"; fix by reconciling them either by adding the missing plan
entries (23-09, 23-10, 23-11) and updating the progress table and list to
reflect 11/11, or by changing the header "11/11" to "8/8" and ensuring the
progress table and list consistently show 8/8; update the occurrences of the
"11/11" string, the Phase 23 list entries ("23-01"… "23-08"), and the progress
table entry so all three match.

In `@apps/api/src/vault/vault.service.ts`:
- Around line 154-156: setByoStatus currently calls
this.vaultRepository.update(...) which no-ops when no vault matches ownerId;
change setByoStatus to inspect the update result from
this.vaultRepository.update({ ownerId: userId }, { isByoUser: isByo }) and if
the result indicates zero rows affected (e.g., affected === 0) throw a clear
error (e.g., NotFound or a descriptive Error) so callers do not receive a silent
success when no vault exists.

In `@apps/web/src/components/settings/MigrationProgress.tsx`:
- Around line 101-109: The UI treats a failed migration as still "migrating";
update the render logic in MigrationProgress.tsx to explicitly handle the failed
terminal state by checking migration.status (or a derived isFailed) before the
isComplete branch and rendering a clear failure message (e.g., "migration
failed: X/Y pins transferred" or include migration.error if available) instead
of the generic "migrating" copy; modify the conditional that currently uses
isComplete and migration.migratedCids/migration.totalCids to first test for
migration.status === 'failed' (or add an isFailed constant) and return the
failure paragraph when true so the UI reflects the terminal failed state.

In `@apps/web/src/components/settings/StorageTab.tsx`:
- Around line 177-208: The derived private key returned by
deriveByoConfigIpnsKeypair (byoKeypair.privateKey) remains in memory after use;
overwrite/zero it immediately after its last use to avoid lingering credentials:
after computing wrappedKey (wrapKey) and again after the
createAndPublishIpnsRecord call, securely wipe the byte array backing
byoKeypair.privateKey (e.g., fill with zeros) and then clear or unset the
reference (e.g., set to undefined) so the private key is not retained in memory;
ensure any intermediate variables that contain its bytes (if copied, e.g.,
wrappedKey/teePublicKey copies) are also cleared where appropriate.

In `@apps/web/src/lib/api/migration.ts`:
- Around line 35-44: The getStatus method currently swallows all errors and
returns null; change getStatus (and its call to customInstance) to distinguish a
404 "no migration" case from other failures by inspecting the thrown error
(e.g., check error.response?.status === 404) and returning null only for 404,
while rethrowing or returning a rejected error for 5xx or network errors so
callers can surface/retry them; reference getStatus, customInstance, and
MigrationStatus when making this change.

In `@packages/sdk-core/src/ipfs/index.ts`:
- Around line 74-89: The JSDoc for the fetch function was accidentally left
above registerCid, making docs misleading; locate the misplaced JSDoc block
immediately before the registerCid function and move it to the actual fetch
function declaration (symbol: fetch) so it documents fetch behavior, then
replace the block above registerCid (symbol: registerCid) with a short, accurate
JSDoc describing that it registers an externally-pinned CID and its
parameters/return type; verify wording matches each function's implementation
and remove any fetch-specific references from registerCid's doc.

In `@packages/sdk/src/__tests__/client-pinning.test.ts`:
- Around line 106-373: Add a new happy-path test in this file that exercises the
client-level "pinata" routing: create a config with
pinningConfig.externalProvider.protocol set to 'pinata', instantiate
CipherBoxClient, call uploadFile (use existing setupFolder, mocks for
sdkCore.addFilePointerToFolder and sdkCore.updateFolderMetadataAndPublish), stub
global fetch to return a successful Pinata-style response, then assert that
sdkCore.addToIpfs was called (CipherBox relay used for CID), that the stubbed
fetch was called with a /pins POST containing the CID, that
sdkCore.unpinFromIpfs was called to clean up, and that sdkCore.registerCid was
invoked; reference CipherBoxClient, pinWithMode, PinataProvider, uploadFile,
setupFolder and sdkCore.* in the test to locate where to add this case.

In `@tee-worker/src/services/ssrf-validation.ts`:
- Around line 34-36: The IPv6 prefix checks in isPrivateAddress are currently
applied to raw hostnames and cause false positives (e.g., fd.example.com);
update isPrivateAddress so that the fd|fc|fe80 prefix checks are only evaluated
when the input is a syntactically valid IPv6 address (use validateResolvedIp or
an IPv6 regex/parse before applying startsWith checks), while hostname inputs
continue to be checked only with hostname-specific logic used by
validateEndpointUrl; ensure functions named isPrivateAddress,
validateEndpointUrl, and validateResolvedIp are adjusted so IPv6-prefix logic is
gated by an IPv6 detection step.

In `@tests/load/src/scenarios/byo-mixed-workload.test.ts`:
- Around line 62-79: If one Promise in the Promise.allSettled pair
(createClientPool / createByoClientPool) fails while the other fulfills, ensure
you clean up the fulfilled pool before rethrowing: check cbResult and byoResult
statuses, assign their resolved values to cbPool and byoPool (or temp variables)
when fulfilled, and if you need to throw due to a rejection (e.g.,
cbResult.status === 'rejected'), call the corresponding pool teardown/destroy
method on the other fulfilled result (the BYO pool returned by
createByoClientPool) before rethrowing the error; reference createClientPool,
createByoClientPool, cbResult, byoResult, cbPool and byoPool to locate and
implement this cleanup logic.

In `@tests/load/src/scenarios/byo-upload-throughput.test.ts`:
- Around line 32-33: Guard the parsed LOAD_TEST_CLIENTS value before using it:
validate the result of parseInt(process.env.LOAD_TEST_CLIENTS ?? '10', 10)
assigned to NUM_CLIENTS and if it is NaN, <= 0, or otherwise invalid, replace it
with a sane positive default (e.g., 10) so the pool config and test title never
receive 0/negative/NaN; update the NUM_CLIENTS initialization logic (and any
uses of NUM_CLIENTS) to rely on this validated fallback.

---

Nitpick comments:
In `@apps/api/src/migration/migration.entity.ts`:
- Around line 27-28: Add a database index on the status column to speed up
queries that filter by status: import and use TypeORM's `@Index`() decorator above
the status property (e.g., add "import { Index } from 'typeorm'" and place
"@Index()" directly above "@Column({ type: 'varchar', length: 20 })" on the
status field typed as MigrationStatus) so the generated schema includes an index
for Migration.status.

In `@apps/api/src/migration/migration.service.ts`:
- Around line 158-178: The updateProgress method has a read-modify-write race on
migration.migratedCids, migration.failedCids and failedCidList when concurrent
callers invoke it; change it to perform atomic updates instead of loading and
saving the entity directly: use the repository/query builder increment() (or an
explicit transaction with SELECT FOR UPDATE) to atomically increment
migratedCids and failedCids, and for failedCidList either update it inside the
same transaction (fetch with FOR UPDATE, append and save) or perform a single
UPDATE that concatenates the new CSV fragment to failedCidList; update
references to updateProgress, migrationRepo, migratedCids, failedCids and
failedCidList accordingly.

In `@packages/sdk-core/src/__tests__/pinning/connection-test.test.ts`:
- Around line 15-162: The tests currently miss verification of the 10s abort
timeout per probe and of endpoint trailing-slash normalization; add two tests to
cover these: (1) "times out after 10s per probe" — use jest.useFakeTimers(),
have mockFetch for the Kubo probe return a Promise that never resolves (or
resolves after >10_000ms) and advance timers by 10_001ms to assert
testConnection(endpoint) returns a timeout/abort-style error (or that mockFetch
call was aborted) and that the PSA probe is then attempted and also respects the
10s timeout; (2) "normalizes endpoint by stripping trailing slash" — call
testConnection(endpoint + '/') and assert the mocked fetch was invoked with
`${endpoint}/api/v0/id` and `${endpoint}/pins` (no double slash) and that
Authorization header behavior remains unchanged; reference testConnection and
the probe endpoints (/api/v0/id and /pins) and use
mockFetch.mockImplementationOnce / mockResolvedValueOnce / mockRejectedValueOnce
plus jest.advanceTimersByTime to simulate the timeout and verify behavior.

In `@packages/sdk-core/src/pinning/dual-pin-provider.ts`:
- Around line 42-50: The unpin method on DualPinProvider currently swallows
secondary unpin errors, preventing callers from retrying or noticing secondary
failures; modify async unpin(cid: string) so it still awaits
this.primary.unpin(cid) (primary must succeed) but when
this.secondary.unpin(cid) throws capture that error and surface it to the
caller—either by rethrowing the secondary error after primary succeeds or by
throwing an aggregated/annotated error (e.g., SecondaryUnpinError or including
the original error message) so callers can detect and handle/retry secondary
failures; reference the unpin method and the this.primary/this.secondary members
and ensure the thrown error preserves the original error details.

In `@packages/sdk-core/src/pinning/pinata-provider.ts`:
- Around line 92-117: In unpin, don't abort on the first failed delete for cid:
iterate over listResult.data.files and attempt each DELETE, collecting failures
(e.g., file.id and status/error) into an array instead of immediately throwing
inside the loop; after the loop, if any failures were collected, throw a single
aggregated Error that includes which file ids failed and their statuses/messages
so callers know which deletions succeeded vs. failed while still attempting
every delete. Ensure the initial listResponse error still throws immediately,
but change the DELETE handling in unpin to continue on error and report
aggregated results at the end.

In `@packages/sdk-core/src/upload/index.ts`:
- Around line 92-96: The code trusts the pluggable pinFn/addToIpfs result
without validation; after calling pinFn or addToIpfs (the variable pinResult
returned by pinFn or addToIpfs in the upload flow), validate that pinResult is
an object containing a non-empty cid and a numeric size (or size-like field)
before using them — if validation fails, throw a clear error indicating the
malformed pinFn output and include context from params (e.g., params.ctx or
params.pinFn) so callers can debug; update the destructuring/usage of cid and
encryptedSize to occur only after this guard.

In `@tee-worker/src/__tests__/ssrf-validation.test.ts`:
- Around line 78-82: Add tests to ensure validateEndpointUrl does not falsely
treat hostnames beginning with IPv6-like prefixes as IPv6 addresses: add an it
block (e.g., "accepts legitimate hostnames starting with fd/fc/fe80") that calls
validateEndpointUrl with hostnames like "https://fd.example.com",
"https://fc-api.cloud" and "https://fe80-host.local" and asserts they do not
throw; place this alongside the existing IPv6 tests to verify the function
distinguishes bracketed IPv6 literals (e.g., "[fd00::1]") from normal hostnames.

In `@tee-worker/src/routes/connection-test.ts`:
- Around line 68-81: The code creates a Uint8Array tokenBytes from authToken for
zeroing but then passes the original authToken string into probeKubo (and
probePsa elsewhere), leaving the string live; either remove tokenBytes if you
won't zero the string, or consistently use tokenBytes when calling the probes by
decoding it where probeKubo/probePsa expect a string (e.g., pass a decoded
string derived from tokenBytes or change the probe APIs to accept Uint8Array),
and ensure tokenBytes is zeroed in the finally block; update calls to
probeKubo(normalizedEndpoint, ...) and probePsa(...) to use the consistent token
representation (tokenBytes or decoded-from-tokenBytes) and keep zeroing logic
intact.

In `@tee-worker/src/services/migration-worker.ts`:
- Around line 104-106: The current code decodes auth tokens once into
sourceToken/destToken before the batch loop, keeping immutable JS strings in
memory; instead, remove the pre-decoded sourceToken and destToken and perform
late decoding inside each network call: decode config.authTokenBytes into a
local token string at the start of fetchFromProvider and pinToProvider (e.g.,
with a TextDecoder().decode(config.authTokenBytes)) so the decoded string is
scoped to the request and not stored for the batch duration; update calls that
previously relied on sourceToken/destToken to read the token from the passed
ProviderConfig within those functions.

In `@tee-worker/src/services/ssrf-validation.ts`:
- Around line 80-85: The validateResolvedIp function currently uses dns.lookup
and only checks the first address; update it to resolve all A and AAAA records
(using dns.resolve4 and dns.resolve6) and verify every returned IP with
isPrivateAddress, throwing an error if any record is private; handle the case
where resolve4/resolve6 may throw or return empty by falling back to
lookup(hostname) so validateResolvedIp still works with single-record hosts, and
ensure both IPv4 and IPv6 results are checked before returning.

[coderabbitai]

🧹 Nitpick comments (1)

apps/api/src/auth/guards/jwt-auth.guard.spec.ts (1)

125-126: Use a single canActivate invocation for both rejection assertions.

Line 125 and Line 126 each call guard.canActivate(context). Use one promise to avoid duplicate execution in the same test path.

Suggested test tweak

-      await expect(guard.canActivate(context)).rejects.toThrow(ForbiddenException);
-      await expect(guard.canActivate(context)).rejects.toThrow('Insufficient token scope');
+      const result = guard.canActivate(context);
+      await expect(result).rejects.toThrow(ForbiddenException);
+      await expect(result).rejects.toThrow('Insufficient token scope');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/auth/guards/jwt-auth.guard.spec.ts` around lines 125 - 126,
Replace the two separate calls to guard.canActivate(context) with a single
promise variable and reuse it for both assertions: call
guard.canActivate(context) once, assign the returned promise to a variable
(e.g., resultPromise), then await
expect(resultPromise).rejects.toThrow(ForbiddenException) and await
expect(resultPromise).rejects.toThrow('Insufficient token scope'); this ensures
guard.canActivate(context) is executed only once in the test that references
guard.canActivate and ForbiddenException.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/api/src/auth/guards/jwt-auth.guard.spec.ts`:
- Around line 125-126: Replace the two separate calls to
guard.canActivate(context) with a single promise variable and reuse it for both
assertions: call guard.canActivate(context) once, assign the returned promise to
a variable (e.g., resultPromise), then await
expect(resultPromise).rejects.toThrow(ForbiddenException) and await
expect(resultPromise).rejects.toThrow('Insufficient token scope'); this ensures
guard.canActivate(context) is executed only once in the test that references
guard.canActivate and ForbiddenException.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 66b72605-bdfb-4750-86ca-3ca016b1aedc

📥 Commits

Reviewing files that changed from the base of the PR and between 15e5a80 and f9b8136.

📒 Files selected for processing (7)

• apps/api/jest.config.js
• apps/api/src/auth/guards/jwt-auth.guard.spec.ts
• apps/api/src/ipfs/ipfs.controller.spec.ts
• apps/api/src/migration/migration.controller.spec.ts
• apps/api/src/migration/migration.processor.spec.ts
• apps/api/src/tee/tee.controller.spec.ts
• apps/api/src/tee/tee.service.spec.ts

✅ Files skipped from review due to trivial changes (2)

• apps/api/jest.config.js
• apps/api/src/ipfs/ipfs.controller.spec.ts

[FSM1]

@coderabbitai please review the latest changes

[coderabbitai]

@FSM1: Sure! I'll kick off a full review of the Phase 21 BYO-IPFS Node Support changes right away.

---

🧠 Learnings used

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 249
File: apps/api/src/auth/services/auth-method.service.ts:159-194
Timestamp: 2026-03-02T22:32:04.963Z
Learning: In PR `#249` (Tier 2 structural refactoring), pre-existing patterns from moved code (e.g., auth.service.ts → auth-method.service.ts) are intentionally preserved as-is. Behavior changes—such as adding pessimistic locking or changing error handling—are out of scope for structural split PRs and should be tracked as separate follow-up improvements.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 342
File: docker/docker-compose.staging.yml:66-69
Timestamp: 2026-03-23T23:48:42.352Z
Learning: In FSM1/cipher-box, the `ipfs_staging` volume migration from flatfs to pebbleds (introduced in PR `#342`, `IPFS_PROFILE=server,pebbleds`) is intentionally a manual operation — NOT automated in the deploy workflow. The cutover plan is: spin up a new pebbleds Kubo container alongside the existing flatfs one, validate pinned CIDs against the database, re-pin only active CIDs to the new node, then swap. The inline comment in `docker/docker-compose.staging.yml` (around the `IPFS_PROFILE` env var) documents the volume reset requirement for operators. Do not flag the missing `down -v` step in the deploy workflow as a regression.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 344
File: .planning/phases/20-vault-migration/20-02-PLAN.md:21-25
Timestamp: 2026-03-24T03:38:06.629Z
Learning: In FSM1/cipher-box `apps/api` and `.planning/`: As of Phase 20 (PR `#344`), the server stores ZERO crypto material by design. Both `encryptedRootFolderKey` and `encryptedRootIpnsPrivateKey` are NULLed on migration via `POST /vault/migrate`. The previously documented permanent DB fallback for `encryptedRootFolderKey` was intentionally removed in gap-closure plans 20-05 and 20-06 after verification confirmed zero non-migrated vaults exist in any environment (staging DB was nuked during Phase 19.2). `REQUIREMENTS.md` and all login/recovery flows have been updated to reflect the zero-crypto-material server state. Do NOT flag nulling of `encryptedRootFolderKey` in migration logic as a missing fallback.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 253
File: tests/e2e-desktop/scripts/test-conflict-detection.ps1:156-157
Timestamp: 2026-03-03T14:55:05.884Z
Learning: In `tests/e2e-desktop/scripts/*.{sh,ps1}` (FUSE E2E tests): Fixed sleep-based waits after file mutations are an inherent limitation of FUSE-mount testing. The FUSE daemon provides no IPC signal or status endpoint to indicate when a debounced IPNS metadata publish completes. Tests use conservative fixed waits (e.g., 8s for initial publish accounting for 1.5s debounce + 10s safety valve, 15s for conflict resolution) and can be increased if CI proves flaky. Eliminating sleep-based waits would require exposing an IPC readiness signal from the FUSE daemon.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

apps/api/src/migration/migration.controller.ts (2)

40-43: ⚠️ Potential issue | 🟠 Major

Mark GET /migration/status as nullable in Swagger.

Line 43 returns MigrationStatusDto | null, and the controller spec already covers the null path, but Line 42 still documents a non-null DTO. Generated clients will treat “no migration yet” as impossible. Use a nullable schema here and regenerate the client.

🩹 OpenAPI fix sketch

-import { ApiTags, ApiBearerAuth, ApiOperation, ApiResponse } from '@nestjs/swagger';
+import {
+  ApiTags,
+  ApiBearerAuth,
+  ApiOperation,
+  ApiResponse,
+  ApiExtraModels,
+  getSchemaPath,
+} from '@nestjs/swagger';
@@
+@ApiExtraModels(MigrationStatusDto)
 `@ApiTags`('migration')
 `@ApiBearerAuth`()
 `@UseGuards`(JwtAuthGuard, ThrottlerGuard)
 `@Controller`('migration')
 export class MigrationController {
@@
-  `@ApiResponse`({ status: 200, type: MigrationStatusDto })
+  `@ApiResponse`({
+    status: 200,
+    schema: {
+      nullable: true,
+      allOf: [{ $ref: getSchemaPath(MigrationStatusDto) }],
+    },
+  })
   async getStatus(`@Request`() req: RequestWithUser): Promise<MigrationStatusDto | null> {

As per coding guidelines, apps/api/** should keep API contracts consistent with the OpenAPI spec, and apps/api/src/**/*.controller.ts changes must be followed by pnpm api:generate.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/migration/migration.controller.ts` around lines 40 - 43, The
Swagger decorator for the GET /migration/status endpoint documents a non-null
MigrationStatusDto although the method getStatus returns MigrationStatusDto |
null; update the `@ApiResponse` on the getStatus controller to mark the response
as nullable for the MigrationStatusDto (e.g., set nullable: true on the response
schema or use the decorator option that marks the type nullable) so the OpenAPI
contract matches the actual return type, then regenerate the client with pnpm
api:generate; target symbols: getStatus method and MigrationStatusDto.

---

27-37: ⚠️ Potential issue | 🟠 Major

Document response body schemas in Swagger decorators.

Lines 35, 53, 64, and 75 return JSON objects ({ migrationId: string } and { message: string }), but their @ApiResponse decorators lack type or schema properties. @nestjs/swagger does not infer inline response schemas from function signatures, so the generated OpenAPI spec and client will miss these response contracts. Add explicit response DTOs or inline schema blocks to each decorator, then rerun pnpm api:generate.

Also, line 42 declares @ApiResponse({ status: 200, type: MigrationStatusDto }) but line 43 returns Promise<MigrationStatusDto | null>. Update the decorator to reflect the nullable response.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/migration/migration.controller.ts` around lines 27 - 37, The
Swagger decorators are missing explicit response schemas: update the
`@ApiResponse` on startMigration (method startMigration) to include a type or
schema describing { migrationId: string } (e.g., a StartMigrationResponseDto or
inline schema) and do the same for the other endpoints that return { message:
string } (create a MessageResponseDto or inline schema) so the OpenAPI spec
includes these contracts; also update the `@ApiResponse` that targets
MigrationStatusDto to reflect nullable responses (set schema.nullable: true or
use { type: MigrationStatusDto, isArray: false, description: ..., nullable: true
} / an explicit nullable DTO) for the method returning
Promise<MigrationStatusDto | null>, then regenerate the spec with pnpm
api:generate.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/api/src/migration/migration.processor.ts`:
- Around line 40-43: Current code loads the live pinned CIDs via
pinnedCidRepo.find({ where: { userId: migration.userId } }) and batches from
index 0 each run, which causes duplicates on resume; change to use a
migration-owned, persisted snapshot/cursor: create and persist a per-migration
CID list or cursor (e.g., a new MigrationCidSnapshot or cursor field on
Migration) when the migration starts, read from that snapshot instead of
pinnedCidRepo in process (replace uses of pinnedCidRepo.find in
migration.processor.ts), and advance/persist the snapshot cursor after each
batch using MigrationService.updateProgress so resumed runs continue from the
saved cursor index and do not re-process or double-count CIDs.

In `@apps/web/src/hooks/useAuth.ts`:
- Around line 239-250: The decrypted BYO blob is only type-cast but not
validated, so a stale/corrupt JSON can produce a config missing required fields
(e.g., pinningMode or missing externalProvider for external mode); update the
block that parses into config (the local variable config in useAuth.ts where
JSON.parse assigns to ByoIpfsConfig) to perform a runtime shape check/guard:
verify config.pinningMode exists and is one of the allowed modes and if
config.pinningMode === 'external' ensure config.externalProvider is present and
valid; if validation fails, discard the config and return undefined/fallback for
the PinningConfig (the object with mode and externalProvider) so the SDK is not
initialized with an impossible pinning state, while still calling
clearBytes(plaintext) in the finally as before.
- Around line 234-238: Wrap the BYO config lookup (the logic that calls
resolveIpnsRecord, fetchFromIpfs and unwrapKey — i.e., the loadByoConfig flow)
in a time-bound promise so it cannot hang indefinitely; implement a 10–15s
timeout using Promise.race (or an AbortController if the underlying
resolveIpnsRecord/fetchFromIpfs support aborting) and have the timeout path
return undefined or throw a sentinel that triggers the existing cipherbox-only
fallback. Update the caller (where loadByoConfig is awaited) to handle the
timeout result the same as a missing BYO config so the login spinner falls back
promptly.

---

Duplicate comments:
In `@apps/api/src/migration/migration.controller.ts`:
- Around line 40-43: The Swagger decorator for the GET /migration/status
endpoint documents a non-null MigrationStatusDto although the method getStatus
returns MigrationStatusDto | null; update the `@ApiResponse` on the getStatus
controller to mark the response as nullable for the MigrationStatusDto (e.g.,
set nullable: true on the response schema or use the decorator option that marks
the type nullable) so the OpenAPI contract matches the actual return type, then
regenerate the client with pnpm api:generate; target symbols: getStatus method
and MigrationStatusDto.
- Around line 27-37: The Swagger decorators are missing explicit response
schemas: update the `@ApiResponse` on startMigration (method startMigration) to
include a type or schema describing { migrationId: string } (e.g., a
StartMigrationResponseDto or inline schema) and do the same for the other
endpoints that return { message: string } (create a MessageResponseDto or inline
schema) so the OpenAPI spec includes these contracts; also update the
`@ApiResponse` that targets MigrationStatusDto to reflect nullable responses (set
schema.nullable: true or use { type: MigrationStatusDto, isArray: false,
description: ..., nullable: true } / an explicit nullable DTO) for the method
returning Promise<MigrationStatusDto | null>, then regenerate the spec with pnpm
api:generate.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7fef54b1-9b7f-44a7-89a5-ad41b2f065c2

📥 Commits

Reviewing files that changed from the base of the PR and between 15e5a80 and b319001.

📒 Files selected for processing (14)

• apps/api/jest.config.js
• apps/api/src/auth/guards/jwt-auth.guard.spec.ts
• apps/api/src/ipfs/ipfs.controller.spec.ts
• apps/api/src/migration/migration.controller.spec.ts
• apps/api/src/migration/migration.controller.ts
• apps/api/src/migration/migration.processor.spec.ts
• apps/api/src/migration/migration.processor.ts
• apps/api/src/tee/tee.controller.spec.ts
• apps/api/src/tee/tee.service.spec.ts
• apps/web/src/components/settings/ConnectionTest.tsx
• apps/web/src/components/settings/MigrationProgress.tsx
• apps/web/src/hooks/useAuth.ts
• packages/sdk-core/src/__tests__/pinning/connection-test.test.ts
• tee-worker/src/routes/migrate.ts

✅ Files skipped from review due to trivial changes (2)

• apps/api/jest.config.js
• apps/api/src/tee/tee.controller.spec.ts

🚧 Files skipped from review as they are similar to previous changes (4)

• apps/api/src/auth/guards/jwt-auth.guard.spec.ts
• apps/web/src/components/settings/MigrationProgress.tsx
• apps/api/src/ipfs/ipfs.controller.spec.ts
• apps/api/src/tee/tee.service.spec.ts