feat(test): add SDK-driven E2E and load test suites#318
Merged
Conversation
Replace browser-dependent testing with SDK-level tests that drive CipherBoxClient directly against the API, eliminating Playwright overhead and UI flakiness. SDK E2E (@cipherbox/sdk-e2e): - Test harness extracting the proven integration.test.ts pattern - Multi-account fixture with api-client singleton switching - 10 test suites: vault-lifecycle, folder-crud, file-operations, data-integrity, bin-operations, error-cases, share-operations, invite-link, ipns-consistency, concurrent-operations Load tests (@cipherbox/load-tests): - Client pool managing N CipherBoxClient instances with own accounts - Metrics collector (p50/p95/p99 latency, throughput, error rates) - Console table + JSON reporter - 5 scenarios: upload-throughput, ipns-publish-storm, mixed-workload, sustained-load, spike-test - 3 parameterized workloads: file, folder, mixed CI workflows: - sdk-e2e.yml: PR-triggered, path-filtered, no browser/web build - load-test.yml: manual dispatch with environment/clients/scenario inputs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e1698bd3af95
WalkthroughThis pull request introduces comprehensive load testing infrastructure for the CipherBox application, adds a dedicated SDK end-to-end test suite, refactors the API client from a module-level singleton to per-instance axios architecture, implements a throttle-bypass guard for rate limiting control during testing, and reorganizes E2E test directories from Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant API as CipherBox API
participant IPFS as IPFS Kubo
participant IPNS as IPNS DHT
participant CipherBox as CipherBoxClient
Test->>API: POST /auth/test-login (secret)
API-->>Test: accessToken, encrypted keys
Test->>Test: Decrypt key material
Test->>API: POST /vault/init (encryptedKeys, rootIpnsName)
API->>IPNS: Publish root folder metadata
API-->>Test: Vault initialized
Test->>CipherBox: new CipherBoxClient(config + axiosInstance)
CipherBox->>CipherBox: registerFolder(rootIpnsName)
CipherBox-->>Test: Client ready
par Load Test Scenarios
Test->>Test: createClientPool(clientCount)
Test->>Test: Start metrics for each PoolClient
loop Per Client Workload
Test->>CipherBox: createFolder/uploadFile/renameItem
CipherBox->>IPFS: Add data
CipherBox->>IPNS: Publish metadata
IPNS-->>CipherBox: sequenceNumber
CipherBox-->>Test: operation complete
Test->>Test: Record latency sample
end
end
Test->>Test: aggregateAndReport()
Test->>Test: Compute percentiles, throughput
Test-->>Test: JSON metrics report
sequenceDiagram
participant Controller as API Controller
participant Guard as BypassableThrottlerGuard
participant Throttler as ThrottlerGuard
participant Request as Request Handler
Controller->>Guard: canActivate(context)
Guard->>Guard: Read THROTTLE_BYPASS_SECRET
alt BYPASS_SECRET set & NODE_ENV !== 'production'
Guard->>Guard: Extract x-throttle-bypass header
alt Header matches secret (timing-safe)
Guard-->>Controller: return true (bypass)
else Header missing or mismatches
Guard->>Throttler: Delegate to parent
Throttler-->>Guard: return parent result
Guard-->>Controller: return result
end
else Secret empty or production
Guard->>Throttler: Delegate to parent
Throttler-->>Guard: return canActivate result
Guard-->>Controller: return result
end
alt Guard allows
Controller->>Request: Execute handler
Request-->>Controller: Response
else Guard blocks
Controller-->>Request: 429 Too Many Requests
end
sequenceDiagram
participant SDK as CipherBoxClient
participant SDKCore as sdk-core module
participant AxiosInstance as Axios Instance
participant API as API Server
SDK->>SDK: constructor(config + axiosInstance?)
SDK->>SDK: Create/assign axios instance
SDK->>SDK: Store in SdkContext.axiosInstance
loop SDK Operations
SDK->>SDKCore: createFolder(params: {..., ctx})
SDKCore->>SDKCore: Create metadata
SDKCore->>SDKCore: createAndPublishIpnsRecord(params, ctx)
alt ctx.axiosInstance provided
SDKCore->>AxiosInstance: POST /ipns/publish with instance
AxiosInstance-->>SDKCore: Success (uses client's token)
else No instance (fallback)
SDKCore->>SDK: Get access token
SDKCore->>API: POST /ipns/publish with manual auth
API-->>SDKCore: Success
end
SDKCore-->>SDK: Updated metadata
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
- Extract shared createTestAccount() in test-harness.ts, reused by load test client-pool.ts (eliminates 60-line duplication) - Extract aggregateAndReport() helper for load test scenarios (eliminates ~60 lines of repeated aggregation boilerplate) - Add getReadonlySamples() to MetricsCollector (avoids unnecessary array copies during aggregation) - Replace (client as any).folderTree with client.getFolderTree() (uses public @internal API instead of unsafe cast) - Share single test context across 4 BinNotLoadedError tests (was creating 4 separate accounts, now creates 1) - Remove redundant double-call in BinNotLoadedError deleteToBin test - Remove unused refreshToken from TestContext interface Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ff776297d125
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (5)
tests/load/src/workloads/folder-workload.ts (1)
1-6: Minor doc inconsistency: comment mentions "moves" but implementation doesn't include move operations.The doc comment states "Creates, renames, moves, and deletes folders" but the implementation only performs create, rename, and delete. Consider updating the comment for accuracy.
📝 Proposed doc fix
/** * Folder CRUD Workload * - * Creates, renames, moves, and deletes folders. + * Creates, renames, and deletes folders. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/workloads/folder-workload.ts` around lines 1 - 6, Update the top-of-file JSDoc comment that currently reads "Folder CRUD Workload: Creates, renames, moves, and deletes folders" to accurately reflect the implemented operations (create, rename, delete) by removing "moves" or, alternatively, implement move functionality; locate and edit the file-level comment beginning with "Folder CRUD Workload" (the top-of-file JSDoc) to either remove the word "moves" or expand the implementation to include move operations in the workload functions.tests/load/src/scenarios/spike-test.test.ts (1)
93-104: Guard against potential division by zero in degradation calculation.If
baseOp.latency.p95is 0 (possible with extremely fast operations or timing edge cases), the division will produceInfinity. Consider adding a guard.🛡️ Proposed guard
for (const baseOp of baselineOps) { const burstOp = burstOps.find((b) => b.operation === baseOp.operation); if (burstOp) { + const basep95 = baseOp.latency.p95 || 1; // Avoid division by zero const degradation = ( - ((burstOp.latency.p95 - baseOp.latency.p95) / baseOp.latency.p95) * + ((burstOp.latency.p95 - baseOp.latency.p95) / basep95) * 100 ).toFixed(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/scenarios/spike-test.test.ts` around lines 93 - 104, The degradation computation can divide by zero when baseOp.latency.p95 is 0; update the loop that iterates baselineOps and computes degradation (referencing baselineOps, burstOps, baseOp.latency.p95 and burstOp.latency.p95) to guard against zero by checking if baseOp.latency.p95 is falsy or === 0 and handling that case (e.g., set degradation to "N/A" or compute an absolute delta) before calling toFixed/console.log so you never perform the division when base p95 is zero.tests/load/src/scenarios/mixed-workload.test.ts (1)
46-54: Throughput calculation may be inaccurate for aggregated metrics.The aggregated
MetricsCollectoris started and stopped after all work completes, sogetMetrics()will computethroughputOpsPerSecusing the brief aggregation window rather than actual test duration. Consider using the pre-computedtotalDurationto override or recalculate throughput in the report, or set the collector's timing window to match the actual workload span.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/scenarios/mixed-workload.test.ts` around lines 46 - 54, The aggregated MetricsCollector (allMetrics) is started/stopped only around the aggregation loop so getMetrics() computes throughputOpsPerSec over that short window; change this by either (A) matching the collector timing to the actual workload span (call allMetrics.start() before the pool work begins and allMetrics.stop() after pool completes) or (B) recomputing/overriding throughputOpsPerSec after aggregation using the pre-computed totalDuration (use totalDuration to calculate throughput = totalOps / totalDuration and set that value on the aggregated metrics returned by getMetrics()); locate MetricsCollector, allMetrics, pc.metrics.getRawSamples(), getMetrics(), totalDuration and update accordingly.tests/sdk-e2e/src/suites/folder-crud.test.ts (1)
42-71: Avoid registering folders with synthetic key material in this suite.
getChild()only gives you child metadata, soparent.folderKey ?? new Uint8Array(32)and the empty public keys here are placeholders. Feeding those intoregisterFolder()seeds SDK state that cannot exist in real usage, which makes the nested/move coverage less representative. Keeping the originalcreateFolder()result around, or creating/registering the parent within the same test, would exercise the real path end to end.As per coding guidelines, "Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics".
Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/suites/folder-crud.test.ts` around lines 42 - 71, The test must not register folders with synthetic key material: replace the use of getChild() + placeholder keys and the registerFolder(...) call that passes parent.folderKey ?? new Uint8Array(32) and empty public keys with creating and registering a real parent via ctx.client.createFolder(...) and then using that createFolder result's ipnsName, folderKey, and ipnsPrivateKey when calling ctx.client.registerFolder(...); apply the same change to the later similar registerFolder usage (the block around the subfolder registration at lines 95-100) so both parent and subfolder are registered with real key material returned by createFolder instead of placeholders.tests/sdk-e2e/src/fixtures/multi-account.ts (1)
51-57: Switch the singleton before per-account teardown.This fixture documents
switchTo()as the guard around singleton-backed operations, butcleanupAll()skips it and implicitly relies on whichever account was last configured. Calling it inside the loop keeps teardown aligned with the fixture contract.💡 Suggested fix
const cleanupAll = async (): Promise<void> => { - for (const [, ctx] of accounts) { + for (const [label, ctx] of accounts) { + switchTo(label); ctx.cleanup(); await deleteTestAccount(ctx); } accounts.clear(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/fixtures/multi-account.ts` around lines 51 - 57, cleanupAll currently calls ctx.cleanup() and deleteTestAccount(ctx) without calling switchTo, so singleton-backed resources may operate under the wrong account; update cleanupAll to call switchTo(ctx.accountId) (or switchTo(ctx)) before invoking ctx.cleanup() and deleteTestAccount(ctx) for each entry in accounts, ensuring per-account teardown runs with the correct singleton context, then clear accounts as before. Ensure you reference the existing functions/variables: cleanupAll, switchTo, accounts, ctx.cleanup, and deleteTestAccount.
🤖 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/load-test.yml:
- Around line 133-143: The Start API server step's health-check loop (pnpm
--filter `@cipherbox/api` start:prod &, for i in $(seq 1 30); do ... curl
http://localhost:3000/health ...) never fails if the service never becomes
ready; add an explicit failure path after the loop so the step exits non-zero
when the health check times out (e.g., set a success flag or check the final
curl/loop exit status and run exit 1 with a clear message if the API never
became ready) so the workflow won't proceed to the load test when `@cipherbox/api`
didn't start.
- Around line 172-175: The workflow step "Run load test" uses the Jest-only flag
--testPathPattern; replace that option by passing the scenario pattern/path as a
positional argument to the pnpm vitest invocation. Update the pnpm command in
the "Run load test" step (the line invoking pnpm --filter `@cipherbox/load-tests`
test) to remove --testPathPattern and append "${{ inputs.scenario }}" as a
positional argument so Vitest receives the file/pattern directly.
In `@tests/load/src/harness/client-pool.ts`:
- Around line 150-166: The destroyClientPool cleanup loop currently fires DELETE
/auth/account calls without inspecting responses and will silently drop 429s;
update destroyClientPool to await the fetch response, check res.ok, and
implement an exponential backoff + retry loop (e.g., up to N attempts with
delays doubling each try) when receiving a 429/temporarily-failing status before
giving up; keep calling pc.client.destroy() as now, perform retries sequentially
per PoolClient, and when ultimately failing log a warning that includes the
final status or error alongside the pc.accessToken or client id for diagnostics.
- Around line 46-64: The current batching loop that calls createPoolClient in
Promise.allSettled silently accepts failures and returns fewer clients; update
the batch handling (the for loop that uses batchSize, Promise.allSettled,
clients, createPoolClient, and clientCount) to detect rejections and either
retry failed createPoolClient calls with exponential backoff (e.g., up to 3
attempts with increasing delays) or fail the pool creation when failures exceed
a safe threshold; alternatively throttle batches by adding a pause between
batches to respect the /auth/test-login limit so you reliably produce
clientCount clients, and ensure any final shortfall throws or returns an
explicit error rather than just console.warn.
- Around line 71-74: The test sets a global api-client getAccessToken to always
return clients[0].accessToken, which causes all SDK-core calls (e.g.,
ipnsControllerPublishRecord used by updateFolderMetadataAndPublish) to
authenticate as client 0; change this so each CipherBoxClient uses its own
token: either call setApiClientConfig per client (inside CipherBoxClient
construction) with a closure that returns that client's accessToken, or modify
the shared getAccessToken to look up the token by client id/index from the
clients array (e.g., tokenMap[clientId]) so ipnsControllerPublishRecord and
other api-client singletons obtain the correct per-client token instead of
always using clients[0].accessToken.
In `@tests/load/src/scenarios/sustained-load.test.ts`:
- Around line 38-45: The throughput is being measured over the short copy window
because allMetrics.start()/stop() are called only while copying raw samples;
update MetricsCollector usage so its elapsed time covers the actual replay/test
window (not the copy). Specifically, move allMetrics.start() to the beginning of
the replay/test loop (or the test window start) and call allMetrics.stop() at
the end of that window (or derive ops/sec from sample timestamps instead of
MetricsCollector elapsed time). Ensure references: MetricsCollector,
getMetrics(), start(), stop(), allMetrics, pc.metrics.getRawSamples(), and
record() are adjusted so getMetrics() uses the correct elapsed period.
- Around line 33-49: The test currently uses Promise.allSettled to run
runSustainedClient across pool and only logs a completion count, which masks
failed client loops; change the post-run handling to detect any rejected results
from results (the Promise.allSettled return) and fail the test by throwing or
asserting: after obtaining results, filter for r.status === 'rejected', collect
their reasons (from r.reason) and throw a new Error (or use the test framework's
fail) with a concise message including the number of failed clients and the
aggregated reasons; keep the aggregation of metrics as-is but ensure the thrown
error happens before the test finishes so a single failed runSustainedClient
causes the scenario to fail.
In `@tests/load/src/scenarios/upload-throughput.test.ts`:
- Around line 8-12: The test currently uses Promise.allSettled and only logs a
summary, so failures don't fail the Vitest case; update the workload
orchestration in upload-throughput.test.ts to fail the test when any client
workload rejects: either switch from Promise.allSettled to Promise.all for the
array of runFileWorkload promises (so Vitest will fail on first rejection), or
keep Promise.allSettled but scan its results and throw an aggregated Error if
any result.status === 'rejected' (include rejection reasons). Ensure cleanup
(destroyClientPool) still runs in a finally block and reference
functions/objects like createClientPool, destroyClientPool, runFileWorkload,
MetricsCollector, printSummary, and toJsonReport when making the change.
In `@tests/load/src/workloads/file-workload.ts`:
- Around line 41-55: When verifyDownloads is true and the uploaded file isn't
found in the folder state, fail the test instead of silently skipping
verification: in the block that reads (client as
any).folderTree.get(rootIpnsName) and finds child via children.find(c => c.name
=== fileName), detect the case where child is undefined or
child.fileMetaIpnsName is missing and throw an explicit Error indicating the
uploaded file never appeared in folder state (include rootIpnsName and fileName
for context). Otherwise proceed to call metrics.measure('downloadFile', () =>
client.downloadFromIpns(child.fileMetaIpnsName, rootFolderKey), size) and keep
the existing size check.
- Around line 27-30: The use of crypto.getRandomValues(data) in the
file-workload generator will throw QuotaExceededError for arrays larger than
65,536 bytes; update the loop that creates the Uint8Array (in
tests/load/src/workloads/file-workload.ts, inside the for (let i = 0; i <
fileCount; i++) block) to fill the buffer in chunks no larger than 65536 bytes
by repeatedly calling crypto.getRandomValues on subarrays (e.g.,
data.subarray(offset, offset+chunkSize)) until the entire Uint8Array is
populated; ensure chunkSize is 65536 and iterate offsets accordingly so larger
files are generated safely.
In `@tests/sdk-e2e/package.json`:
- Line 9: The package.json script "test:single" uses the unsupported Vitest flag
"--testPathPattern"; remove that flag from the "test:single" script in
package.json and update invocation/docs to use positional file arguments or
Vitest's test name filter (e.g., pass a file path or use
"-t"/"--testNamePattern" after a "--" when running the "test:single" script),
ensuring references to the "test:single" script are adjusted accordingly.
In `@tests/sdk-e2e/src/fixtures/test-harness.ts`:
- Around line 82-86: The test-harness currently calls setApiClientConfig which
mutates a module-level singleton and causes different createTestContext
instances to share the same getAccessToken; fix by removing reliance on the
global singleton and making API client config per-context: modify
createTestContext to stop calling setApiClientConfig and instead construct or
inject a per-context API client config (getAccessToken/ baseUrl) into
CipherBoxClient (add an optional constructor/initializer parameter in
CipherBoxClient to accept this config), update places that instantiate
CipherBoxClient to pass the context-specific config, and keep switchTo() as a
compatibility helper; alternatively, if you prefer the quick docs-only route,
add a prominent note at the top of test-harness.ts and require tests to call
switchTo() when creating multiple contexts.
- Around line 43-55: createTestContext is calling /auth/test-login for every
invocation and will exceed the endpoint's throttle; modify createTestContext to
reuse or cache test accounts instead of always creating new ones: introduce a
module-level map (e.g., testAccountCache) keyed by label or a small fixed pool
key, check the cache at the top of createTestContext and return the cached
TestContext (accessToken, refreshToken, publicKeyHex, privateKeyHex) when
present, otherwise perform the POST to /auth/test-login, store the returned
tokens/keys in the cache, and return them; ensure the cache key aligns with
existing callers (including createMultiAccountFixture) so suites can share
accounts and avoid hitting /auth/test-login repeatedly.
In `@tests/sdk-e2e/src/suites/concurrent-operations.test.ts`:
- Around line 1-8: The suite's tests claim to exercise concurrent behavior but
run all operations sequentially on a single ctx.client; change tests in
concurrent-operations.test.ts to actually run concurrent operations (e.g., use
two distinct client instances instead of a single ctx.client and launch
operations without awaiting sequentially, using Promise.all or similar) for
scenarios involving createFolder, renameItem, moveItem, deleteItem, and
deleteToBin; ensure at least one test spawns overlapping mutations from
different clients against the same folder and asserts final consistency/expected
conflict behavior rather than relying on serial awaits.
In `@tests/sdk-e2e/src/suites/error-cases.test.ts`:
- Around line 59-63: The test is too broad and accepts any rejection from
ctx.client.moveItem; update the assertion to assert the specific "Destination
folder not loaded" error so it fails for other causes. Replace the generic
rejects.toThrow() with an assertion that checks the error message (e.g., use
await expect(ctx.client.moveItem(ctx.rootIpnsName, fakeIpnsName,
'child-id')).rejects.toThrow('Destination folder not loaded')) so the test
explicitly validates the unloaded-destination branch in moveItem.
In `@tests/sdk-e2e/src/suites/invite-link.test.ts`:
- Around line 24-25: The test uses dummyReWrappedKey which only proves the
endpoint accepts a blob; replace it with a real wrapped folder key produced by
the same re-wrapping/wrapping routine used in the app (use the project helper
that creates a rewrapped/wrapped folder key for Bob instead of dummyReWrappedKey
so the invite contains a valid encrypted share), then after claiming the invite
(the code that sets inviteToken/folderIpnsName) add an assertion where Bob
unwraps/decrypts the received share with his private key (call the project’s
unwrap/decrypt function) and verify the resulting key matches the original
folder key or that Bob can open/access the folder via the SDK; update tests that
reference dummyReWrappedKey across the blocks around variables inviteToken and
folderIpnsName to ensure the full handoff is validated.
In `@tests/sdk-e2e/src/suites/share-operations.test.ts`:
- Line 10: The test currently passes share.encryptedKey (text from the
/shares/received JSON) directly into unwrapKey; instead decode that string into
a Uint8Array before calling unwrapKey so the binary API receives real bytes.
Locate the call that uses unwrapKey (and the related wrapKey/bytesToHex usage)
and replace the direct string with a decoded Uint8Array (e.g.,
Buffer.from(share.encryptedKey, 'base64') or the appropriate hex-to-bytes path)
so unwrapKey receives a Uint8Array rather than a string.
---
Nitpick comments:
In `@tests/load/src/scenarios/mixed-workload.test.ts`:
- Around line 46-54: The aggregated MetricsCollector (allMetrics) is
started/stopped only around the aggregation loop so getMetrics() computes
throughputOpsPerSec over that short window; change this by either (A) matching
the collector timing to the actual workload span (call allMetrics.start() before
the pool work begins and allMetrics.stop() after pool completes) or (B)
recomputing/overriding throughputOpsPerSec after aggregation using the
pre-computed totalDuration (use totalDuration to calculate throughput = totalOps
/ totalDuration and set that value on the aggregated metrics returned by
getMetrics()); locate MetricsCollector, allMetrics, pc.metrics.getRawSamples(),
getMetrics(), totalDuration and update accordingly.
In `@tests/load/src/scenarios/spike-test.test.ts`:
- Around line 93-104: The degradation computation can divide by zero when
baseOp.latency.p95 is 0; update the loop that iterates baselineOps and computes
degradation (referencing baselineOps, burstOps, baseOp.latency.p95 and
burstOp.latency.p95) to guard against zero by checking if baseOp.latency.p95 is
falsy or === 0 and handling that case (e.g., set degradation to "N/A" or compute
an absolute delta) before calling toFixed/console.log so you never perform the
division when base p95 is zero.
In `@tests/load/src/workloads/folder-workload.ts`:
- Around line 1-6: Update the top-of-file JSDoc comment that currently reads
"Folder CRUD Workload: Creates, renames, moves, and deletes folders" to
accurately reflect the implemented operations (create, rename, delete) by
removing "moves" or, alternatively, implement move functionality; locate and
edit the file-level comment beginning with "Folder CRUD Workload" (the
top-of-file JSDoc) to either remove the word "moves" or expand the
implementation to include move operations in the workload functions.
In `@tests/sdk-e2e/src/fixtures/multi-account.ts`:
- Around line 51-57: cleanupAll currently calls ctx.cleanup() and
deleteTestAccount(ctx) without calling switchTo, so singleton-backed resources
may operate under the wrong account; update cleanupAll to call
switchTo(ctx.accountId) (or switchTo(ctx)) before invoking ctx.cleanup() and
deleteTestAccount(ctx) for each entry in accounts, ensuring per-account teardown
runs with the correct singleton context, then clear accounts as before. Ensure
you reference the existing functions/variables: cleanupAll, switchTo, accounts,
ctx.cleanup, and deleteTestAccount.
In `@tests/sdk-e2e/src/suites/folder-crud.test.ts`:
- Around line 42-71: The test must not register folders with synthetic key
material: replace the use of getChild() + placeholder keys and the
registerFolder(...) call that passes parent.folderKey ?? new Uint8Array(32) and
empty public keys with creating and registering a real parent via
ctx.client.createFolder(...) and then using that createFolder result's ipnsName,
folderKey, and ipnsPrivateKey when calling ctx.client.registerFolder(...); apply
the same change to the later similar registerFolder usage (the block around the
subfolder registration at lines 95-100) so both parent and subfolder are
registered with real key material returned by createFolder instead of
placeholders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0549305a-e3ea-4e0e-91e8-c135bf2ed387
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.github/workflows/load-test.yml.github/workflows/sdk-e2e.ymlrelease-please-config.jsontests/load/.env.exampletests/load/package.jsontests/load/src/harness/client-pool.tstests/load/src/harness/metrics.tstests/load/src/harness/reporter.tstests/load/src/scenarios/ipns-publish-storm.test.tstests/load/src/scenarios/mixed-workload.test.tstests/load/src/scenarios/spike-test.test.tstests/load/src/scenarios/sustained-load.test.tstests/load/src/scenarios/upload-throughput.test.tstests/load/src/workloads/file-workload.tstests/load/src/workloads/folder-workload.tstests/load/src/workloads/mixed-workload.tstests/load/tsconfig.jsontests/load/vitest.config.tstests/sdk-e2e/.env.exampletests/sdk-e2e/package.jsontests/sdk-e2e/src/fixtures/multi-account.tstests/sdk-e2e/src/fixtures/test-harness.tstests/sdk-e2e/src/helpers/assertions.tstests/sdk-e2e/src/helpers/data-generators.tstests/sdk-e2e/src/suites/bin-operations.test.tstests/sdk-e2e/src/suites/concurrent-operations.test.tstests/sdk-e2e/src/suites/data-integrity.test.tstests/sdk-e2e/src/suites/error-cases.test.tstests/sdk-e2e/src/suites/file-operations.test.tstests/sdk-e2e/src/suites/folder-crud.test.tstests/sdk-e2e/src/suites/invite-link.test.tstests/sdk-e2e/src/suites/ipns-consistency.test.tstests/sdk-e2e/src/suites/share-operations.test.tstests/sdk-e2e/src/suites/vault-lifecycle.test.tstests/sdk-e2e/tsconfig.jsontests/sdk-e2e/vitest.config.ts
- Relax API rate limits in NODE_ENV=test (200 req/s, 10k req/min) to prevent 429s during rapid SDK E2E and load test operations - Use real ECIES-wrapped keys in invite-link tests (API validates MinLength(258) on encryptedKey, dummy 128-char hex was too short) - Convert hex to Uint8Array before unwrapKey in share-operations test - Re-set api-client singleton token before event emission test (other suites' multi-account fixtures overwrite the singleton) All 83 tests pass in 11.3s against local API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 9319e2d02cab
There was a problem hiding this comment.
♻️ Duplicate comments (6)
tests/sdk-e2e/src/suites/error-cases.test.ts (1)
59-63:⚠️ Potential issue | 🟡 MinorAssert the specific unloaded-destination error message.
This test accepts any rejection, so it won't detect if
moveItem()starts failing for a different reason. The source folder is loaded, so the expected error is"Destination folder not loaded".💡 Suggested fix
await expect( ctx.client.moveItem(ctx.rootIpnsName, fakeIpnsName, 'child-id') - ).rejects.toThrow(); + ).rejects.toThrow('Destination folder not loaded');As per coding guidelines
**/*.test.ts: 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 `@tests/sdk-e2e/src/suites/error-cases.test.ts` around lines 59 - 63, The test currently asserts any rejection from ctx.client.moveItem; change it to assert the specific error message "Destination folder not loaded" so it fails only for the expected unloaded-destination case. Locate the test in error-cases.test.ts that calls ctx.client.moveItem(ctx.rootIpnsName, fakeIpnsName, 'child-id') and update the expectation to check the rejection message (e.g., use rejects.toThrow with "Destination folder not loaded" or an equivalent matcher) so the test verifies the precise error from moveItem.tests/load/src/harness/client-pool.ts (2)
74-81:⚠️ Potential issue | 🔴 CriticalAll pool clients authenticate as client 0, defeating per-account load isolation.
The
getAccessTokenclosure at line 79 always returnsclients[0].accessToken. Since the api-client singleton invokesgetAccessToken()on every request (seepackages/api-client/src/instance.ts:31-37), all API calls from all pool clients authenticate as the first client, regardless of whichCipherBoxClientinitiated the request.This collapses load test isolation — N clients actually share one account's auth context.
💡 Potential fix approaches
- Thread-local token via AsyncLocalStorage (recommended for Node.js):
import { AsyncLocalStorage } from 'async_hooks'; const tokenStorage = new AsyncLocalStorage<string>(); setApiClientConfig({ baseUrl: API_URL, getAccessToken: async () => tokenStorage.getStore() ?? clients[0].accessToken, }); // Wrap each client's workload: tokenStorage.run(pc.accessToken, () => runWorkload(pc));
Per-client api-client instances (requires SDK refactor to accept injected client)
Document limitation if load tests are only meant to stress the server, not validate per-user isolation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/harness/client-pool.ts` around lines 74 - 81, The current getAccessToken closure passed to setApiClientConfig always returns clients[0].accessToken causing every request to authenticate as client 0; fix by using AsyncLocalStorage to provide a per-request/token context: create a tokenStorage via AsyncLocalStorage<string>, change getAccessToken to return tokenStorage.getStore() ?? clients[0].accessToken, and wrap each client workload invocation (e.g., when starting runWorkload or similar per-CipherBoxClient tasks) in tokenStorage.run(pc.accessToken, () => runWorkload(pc)) so each client’s requests use its own token while preserving the shared api-client singleton.
45-72:⚠️ Potential issue | 🟠 MajorBatch creation silently downgrades pool size on failures.
With rate-limited endpoints,
Promise.allSettled+ warning logs allow the pool to be created with fewer clients than requested. Load tests may run with an under-provisioned pool without failing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/harness/client-pool.ts` around lines 45 - 72, The current batch creation uses Promise.allSettled and only logs failures so the resulting clients array can be smaller than the requested clientCount; update the logic around createTestAccount / the batching loop so the pool never silently shrinks: either retry failed creations (e.g., re-enqueue failed indices and retry until they succeed or a retry limit) or throw an error when clients.length < clientCount after the batching completes; refer to the symbols clientCount, clients, batchSize, createTestAccount, PoolClient and the Promise.allSettled block and implement the chosen strategy (retry with a limit or fail-fast) to ensure the final clients.length equals clientCount or the creation fails loudly.tests/sdk-e2e/src/fixtures/test-harness.ts (2)
90-104:⚠️ Potential issue | 🟠 MajorThe global api-client singleton creates token isolation issues in multi-context scenarios.
setApiClientConfig()at line 91-94 mutates a module-level singleton. When multipleTestContextinstances exist (e.g.,ctxandfreshCtxin error-cases.test.ts), the later call overwrites the singleton'sgetAccessToken, causing bothCipherBoxClientinstances to potentially share the wrong user's token for API calls routed through the singleton.This is documented as a known caveat but contributes to the 401 errors seen in the pipeline (
Request failed with status code 401at line 106 in error-cases.test.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/fixtures/test-harness.ts` around lines 90 - 104, The test harness currently calls the module-level singleton setter setApiClientConfig(...) which mutates global getAccessToken and causes token leakage between TestContext instances; remove or avoid the global call and instead make each test context pass its own token provider directly to clients (e.g., provide getAccessToken to CipherBoxClient constructor and ensure any code that would use the singleton uses the per-instance client), or if the singleton must be used, reset it per-context immediately after use; update the code around setApiClientConfig, CipherBoxClient instantiation, and registerFolder usage so each TestContext has its own token provider and no cross-test global mutation occurs.
48-65:⚠️ Potential issue | 🟠 Major
createTestAccount()will hit the /auth/test-login throttle during the E2E suite run.The pipeline failure (
429: Request failed with status code 429) confirms this. The/auth/test-loginendpoint is rate-limited, and creating 22+ test accounts across suites within the throttle window exceeds the limit.Consider:
- Caching/reusing test accounts across suites
- Exempting
/auth/test-loginfrom throttling in test environments- Adding retry with exponential backoff for 429 responses
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/fixtures/test-harness.ts` around lines 48 - 65, createTestAccount currently calls the rate-limited /auth/test-login directly and triggers 429s; modify createTestAccount to (1) first check a shared in-memory cache keyed by opts.emailPrefix/opts.label (or another stable key) and return the cached TestAccount if present to reuse accounts across suites, and (2) wrap the fetch to "/auth/test-login" in a retry loop that detects 429 responses and retries with exponential backoff (e.g., initial delay, multiply on each attempt, cap attempts) before throwing; update the logic that consumes accessToken/publicKeyHex/privateKeyHex to work the same when returning a cached account.tests/load/src/scenarios/sustained-load.test.ts (1)
36-44:⚠️ Potential issue | 🟠 MajorConsider failing the test when client loops reject.
Promise.allSettled()with log-only success count lets the test pass even when multiple clients abort early. The reported load may be significantly lower than requested.💡 Suggested fix
const results = await Promise.allSettled( pool.map((pc) => runSustainedClient(pc, endTime, OPS_PER_SEC_PER_CLIENT)) ); - const succeeded = results.filter((r) => r.status === 'fulfilled').length; - console.log(`\nClients completed: ${succeeded}/${pool.length}`); + const failed = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected'); + if (failed.length > 0) { + console.error(`${failed.length}/${pool.length} clients failed`); + // Optionally throw if too many failures + } + console.log(`\nClients completed: ${pool.length - failed.length}/${pool.length}`);As per coding guidelines
**/*.test.ts: 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 `@tests/load/src/scenarios/sustained-load.test.ts` around lines 36 - 44, The test currently uses Promise.allSettled so failures are only logged and the test can pass silently; update it to fail when any client loop rejects by either replacing Promise.allSettled(...) with Promise.all(...) or, if you want to preserve per-client result reporting, keep Promise.allSettled but inspect results and throw when any r.status === 'rejected' (include count and at least one error message) before calling aggregateAndReport; reference runSustainedClient, pool and aggregateAndReport when making the change so the test fails on client aborts rather than only logging them.
🧹 Nitpick comments (4)
tests/load/src/scenarios/sustained-load.test.ts (1)
85-87: Silent error swallowing is intentional here but reduces observability.The empty catch keeps the sustained loop running, which is correct for load testing. Consider at least incrementing an error counter or logging at debug level to aid troubleshooting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/scenarios/sustained-load.test.ts` around lines 85 - 87, The empty catch in the sustained load test silently swallows errors and reduces observability; update the catch block inside the sustained load loop in tests/load/src/scenarios/sustained-load.test.ts to record failures (e.g., increment a local error counter) and/or emit a debug-level log via the test logger so the loop continues but incidents are visible for troubleshooting; ensure the counter and/or logger are declared in the test scope and used inside the catch so metrics or debug output are available after the run.tests/sdk-e2e/src/helpers/assertions.ts (1)
53-68: Consider adding explicit return types for better type safety.
getChildandgetChildrenreturnany/any[]. If the folder tree's child type is available from@cipherbox/sdk, using it would improve IDE support and catch type mismatches in tests.💡 Suggested improvement
-export function getChild(client: CipherBoxClient, folderIpnsName: string, name: string): any { +export function getChild(client: CipherBoxClient, folderIpnsName: string, name: string): FolderChild {Import
FolderChildfrom@cipherbox/sdkif exported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/helpers/assertions.ts` around lines 53 - 68, The functions getChild and getChildren use any/any[] which weakens type safety; import the proper child type (e.g., FolderChild or whatever the SDK exports for folder nodes) from `@cipherbox/sdk` and update the signatures to return that type and an array of that type (change getChild(...): any to getChild(...): FolderChild and getChildren(...): any[] to getChildren(...): FolderChild[]), and update any local usages if needed to satisfy the stronger types.tests/sdk-e2e/src/suites/error-cases.test.ts (1)
139-157: Lifecycle test creates additional test account without cleanup in afterAll.
tempCtxis created inline at line 141 and deleted at line 155, but if the test fails before line 155, the account is orphaned. Consider using a try/finally or moving cleanup to afterAll.💡 Suggested fix
describe('Client lifecycle', () => { + let tempCtx: TestContext | null = null; + + afterAll(async () => { + if (tempCtx) { + tempCtx.cleanup(); + await deleteTestAccount(tempCtx); + } + }); + it('should not work after destroy()', async () => { - const tempCtx = await createTestContext('destroy-test'); + tempCtx = await createTestContext('destroy-test'); // ... rest of test - await deleteTestAccount(tempCtx); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/suites/error-cases.test.ts` around lines 139 - 157, The test creates a temporary account via createTestContext('destroy-test') assigned to tempCtx but only calls deleteTestAccount(tempCtx) at the end, which leaves the account orphaned if the test fails; wrap the test body so tempCtx is cleaned up in all outcomes (use try/finally) or move creation to a beforeEach and cleanup to afterEach/afterAll; specifically, ensure tempCtx is declared in the test scope, then call deleteTestAccount(tempCtx) inside a finally block after the destroy/assertions (or register cleanup in afterEach) so deleteTestAccount always runs even if an assertion throws.tests/load/src/scenarios/spike-test.test.ts (1)
64-77: Guard against division by zero in degradation calculation.If
baseOp.latency.p95is 0 (e.g., no successful operations in baseline), the percentage calculation will produceInfinityorNaN.💡 Suggested fix
if (burstOp) { - const degradation = ( - ((burstOp.latency.p95 - baseOp.latency.p95) / baseOp.latency.p95) * - 100 - ).toFixed(0); + const degradation = baseOp.latency.p95 > 0 + ? (((burstOp.latency.p95 - baseOp.latency.p95) / baseOp.latency.p95) * 100).toFixed(0) + : 'N/A'; console.log(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/scenarios/spike-test.test.ts` around lines 64 - 77, The degradation percentage calculation can divide by zero when baseOp.latency.p95 === 0; update the loop that compares baselineMetrics to burstMetrics (the for ... of baselineMetrics block and the degradation computation using baseOp.latency.p95 and burstOp.latency.p95) to guard against zero or falsy baseline p95—e.g., if baseOp.latency.p95 is 0 or NaN, skip the percentage calc and log a meaningful placeholder like "N/A" or "0%" (or compute absolute difference instead) so you never produce Infinity/NaN in the console output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/load/src/harness/client-pool.ts`:
- Around line 74-81: The current getAccessToken closure passed to
setApiClientConfig always returns clients[0].accessToken causing every request
to authenticate as client 0; fix by using AsyncLocalStorage to provide a
per-request/token context: create a tokenStorage via AsyncLocalStorage<string>,
change getAccessToken to return tokenStorage.getStore() ??
clients[0].accessToken, and wrap each client workload invocation (e.g., when
starting runWorkload or similar per-CipherBoxClient tasks) in
tokenStorage.run(pc.accessToken, () => runWorkload(pc)) so each client’s
requests use its own token while preserving the shared api-client singleton.
- Around line 45-72: The current batch creation uses Promise.allSettled and only
logs failures so the resulting clients array can be smaller than the requested
clientCount; update the logic around createTestAccount / the batching loop so
the pool never silently shrinks: either retry failed creations (e.g., re-enqueue
failed indices and retry until they succeed or a retry limit) or throw an error
when clients.length < clientCount after the batching completes; refer to the
symbols clientCount, clients, batchSize, createTestAccount, PoolClient and the
Promise.allSettled block and implement the chosen strategy (retry with a limit
or fail-fast) to ensure the final clients.length equals clientCount or the
creation fails loudly.
In `@tests/load/src/scenarios/sustained-load.test.ts`:
- Around line 36-44: The test currently uses Promise.allSettled so failures are
only logged and the test can pass silently; update it to fail when any client
loop rejects by either replacing Promise.allSettled(...) with Promise.all(...)
or, if you want to preserve per-client result reporting, keep Promise.allSettled
but inspect results and throw when any r.status === 'rejected' (include count
and at least one error message) before calling aggregateAndReport; reference
runSustainedClient, pool and aggregateAndReport when making the change so the
test fails on client aborts rather than only logging them.
In `@tests/sdk-e2e/src/fixtures/test-harness.ts`:
- Around line 90-104: The test harness currently calls the module-level
singleton setter setApiClientConfig(...) which mutates global getAccessToken and
causes token leakage between TestContext instances; remove or avoid the global
call and instead make each test context pass its own token provider directly to
clients (e.g., provide getAccessToken to CipherBoxClient constructor and ensure
any code that would use the singleton uses the per-instance client), or if the
singleton must be used, reset it per-context immediately after use; update the
code around setApiClientConfig, CipherBoxClient instantiation, and
registerFolder usage so each TestContext has its own token provider and no
cross-test global mutation occurs.
- Around line 48-65: createTestAccount currently calls the rate-limited
/auth/test-login directly and triggers 429s; modify createTestAccount to (1)
first check a shared in-memory cache keyed by opts.emailPrefix/opts.label (or
another stable key) and return the cached TestAccount if present to reuse
accounts across suites, and (2) wrap the fetch to "/auth/test-login" in a retry
loop that detects 429 responses and retries with exponential backoff (e.g.,
initial delay, multiply on each attempt, cap attempts) before throwing; update
the logic that consumes accessToken/publicKeyHex/privateKeyHex to work the same
when returning a cached account.
In `@tests/sdk-e2e/src/suites/error-cases.test.ts`:
- Around line 59-63: The test currently asserts any rejection from
ctx.client.moveItem; change it to assert the specific error message "Destination
folder not loaded" so it fails only for the expected unloaded-destination case.
Locate the test in error-cases.test.ts that calls
ctx.client.moveItem(ctx.rootIpnsName, fakeIpnsName, 'child-id') and update the
expectation to check the rejection message (e.g., use rejects.toThrow with
"Destination folder not loaded" or an equivalent matcher) so the test verifies
the precise error from moveItem.
---
Nitpick comments:
In `@tests/load/src/scenarios/spike-test.test.ts`:
- Around line 64-77: The degradation percentage calculation can divide by zero
when baseOp.latency.p95 === 0; update the loop that compares baselineMetrics to
burstMetrics (the for ... of baselineMetrics block and the degradation
computation using baseOp.latency.p95 and burstOp.latency.p95) to guard against
zero or falsy baseline p95—e.g., if baseOp.latency.p95 is 0 or NaN, skip the
percentage calc and log a meaningful placeholder like "N/A" or "0%" (or compute
absolute difference instead) so you never produce Infinity/NaN in the console
output.
In `@tests/load/src/scenarios/sustained-load.test.ts`:
- Around line 85-87: The empty catch in the sustained load test silently
swallows errors and reduces observability; update the catch block inside the
sustained load loop in tests/load/src/scenarios/sustained-load.test.ts to record
failures (e.g., increment a local error counter) and/or emit a debug-level log
via the test logger so the loop continues but incidents are visible for
troubleshooting; ensure the counter and/or logger are declared in the test scope
and used inside the catch so metrics or debug output are available after the
run.
In `@tests/sdk-e2e/src/helpers/assertions.ts`:
- Around line 53-68: The functions getChild and getChildren use any/any[] which
weakens type safety; import the proper child type (e.g., FolderChild or whatever
the SDK exports for folder nodes) from `@cipherbox/sdk` and update the signatures
to return that type and an array of that type (change getChild(...): any to
getChild(...): FolderChild and getChildren(...): any[] to getChildren(...):
FolderChild[]), and update any local usages if needed to satisfy the stronger
types.
In `@tests/sdk-e2e/src/suites/error-cases.test.ts`:
- Around line 139-157: The test creates a temporary account via
createTestContext('destroy-test') assigned to tempCtx but only calls
deleteTestAccount(tempCtx) at the end, which leaves the account orphaned if the
test fails; wrap the test body so tempCtx is cleaned up in all outcomes (use
try/finally) or move creation to a beforeEach and cleanup to afterEach/afterAll;
specifically, ensure tempCtx is declared in the test scope, then call
deleteTestAccount(tempCtx) inside a finally block after the destroy/assertions
(or register cleanup in afterEach) so deleteTestAccount always runs even if an
assertion throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e738c095-6b6e-45ca-99e9-79505d884d8d
📒 Files selected for processing (13)
tests/load/src/harness/client-pool.tstests/load/src/harness/metrics.tstests/load/src/scenarios/ipns-publish-storm.test.tstests/load/src/scenarios/mixed-workload.test.tstests/load/src/scenarios/spike-test.test.tstests/load/src/scenarios/sustained-load.test.tstests/load/src/scenarios/upload-throughput.test.tstests/load/src/workloads/file-workload.tstests/load/src/workloads/mixed-workload.tstests/load/tsconfig.jsontests/sdk-e2e/src/fixtures/test-harness.tstests/sdk-e2e/src/helpers/assertions.tstests/sdk-e2e/src/suites/error-cases.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/load/tsconfig.json
- tests/load/src/scenarios/ipns-publish-storm.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/load/src/scenarios/mixed-workload.test.ts
- tests/load/src/workloads/mixed-workload.ts
- tests/load/src/workloads/file-workload.ts
- tests/load/src/scenarios/upload-throughput.test.ts
…coped axios Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 3fa3e9df5c9b
Add BypassableThrottlerGuard that extends NestJS ThrottlerGuard. When THROTTLE_BYPASS_SECRET is configured and a request includes a matching X-Throttle-Bypass header, rate limiting is skipped. This replaces the NODE_ENV=test conditional throttle relaxation with a proper secret-based bypass that works across all environments: - Production: secret not set, bypass impossible - Staging: secret in environment, load tests can bypass - CI: secret hardcoded in workflow .env creation - Local dev: secret in apps/api/.env Implementation: - BypassableThrottlerGuard with constant-time secret comparison - All 7 controllers updated to use the new guard - defaultHeaders added to ApiClientConfig for axios instance - testFetch() helper in test harness for raw fetch calls - CI workflows inject THROTTLE_BYPASS_SECRET in .env and env vars All 83 SDK E2E tests pass in 11.5s with production throttle limits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 801431440395
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: d81a6d44fd4a
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/sdk-e2e/src/suites/share-operations.test.ts (2)
118-120: Assert the unwrapped bytes, not just the key length.
length === 32still passes if the API stores the wrong 32-byte ciphertext. Persist the original folder key from the create step and compare the actual bytes returned byunwrapKey().🧪 Suggested change
- const folderKey = await unwrapKey(hexToBytes(share.encryptedKey), bob.privateKey); - expect(folderKey.length).toBe(32); // AES-256 key + const folderKey = await unwrapKey(hexToBytes(share.encryptedKey), bob.privateKey); + expect(folderKey).toEqual(sharedFolderKey);Add this near the suite state:
let sharedFolderKey: Uint8Array;Then assign it when Alice creates the shared folder:
sharedFolderKey = new Uint8Array(folder.folderKey);As per coding guidelines, "
**/*.test.ts: 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 `@tests/sdk-e2e/src/suites/share-operations.test.ts` around lines 118 - 120, The test currently only asserts the length of the unwrapped key; instead persist the original folder key from the create step (assign new Uint8Array(folder.folderKey) to a suite-scoped variable like sharedFolderKey when Alice creates the folder) and then in the assertion replace the length check with a byte-wise comparison between the returned folderKey (from unwrapKey(hexToBytes(share.encryptedKey), bob.privateKey)) and sharedFolderKey to ensure the unwrapped bytes match exactly; keep using unwrapKey, hexToBytes and share.encryptedKey to locate the code to change.
133-139: Make the self-share request valid apart from the conflict.Using an all-zero
encryptedKeymeans this can fail for payload-validation reasons instead of the self-share check. Reuse a real ECIES-wrapped key for the same item so the409is tied to the behavior this test is supposed to prove.🧪 Suggested change
- ipnsName: alice.rootIpnsName, - itemName: 'SelfShare', - encryptedKey: bytesToHex(new Uint8Array(64)), + ipnsName: sharedFolderIpnsName, + itemName: 'SharedFolder', + encryptedKey: bytesToHex(await wrapKey(sharedFolderKey, alice.publicKey)),Support code needed near the suite state/create step:
let sharedFolderIpnsName: string; sharedFolderIpnsName = folder.ipnsName;As per coding guidelines, "
**/*.test.ts: 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 `@tests/sdk-e2e/src/suites/share-operations.test.ts` around lines 133 - 139, The test currently sends an all-zero encryptedKey which can trigger payload validation errors instead of the intended self-share conflict; instead reuse a real ECIES-wrapped key for the same item by replacing the zeroed encryptedKey with the actual wrapped key used when sharing the folder (e.g., use the ECIES-wrapped key bytes for folder.ipnsName / sharedFolderIpnsName from the suite setup), so that the POST body (recipientPublicKey: '0x'+bytesToHex(alice.publicKey), itemType: 'folder', ipnsName: alice.rootIpnsName, itemName: 'SelfShare', encryptedKey: bytesToHex(...realWrappedKey...)) is valid and the test exercises the 409 self-share behavior.
🤖 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/common/guards/throttler-bypass.guard.ts`:
- Around line 19-27: The guard currently trusts the presence of
THROTTLE_BYPASS_SECRET alone; change the logic in throttler-bypass.guard.ts to
require an explicit opt-in before using the secret by adding a second gate
(e.g., process.env.THROTTLE_BYPASS_ENABLED === 'true' or an explicit allowlist
env like THROTTLE_BYPASS_ALLOWLIST) checked before reading secret/header; only
if the opt-in flag/allowlist permits should you read
request.headers['x-throttle-bypass'] and call timingSafeEqual(secret, header) to
return true, otherwise always fail-closed and do not consult the secret.
- Around line 33-40: Replace the custom timingSafeEqual function with Node's
native implementation: remove the hand-rolled timingSafeEqual function and
instead import timingSafeEqual from 'crypto' at the top of the file, and when
comparing secrets convert the strings to Buffers and call crypto's
timingSafeEqual (e.g., timingSafeEqual(Buffer.from(a), Buffer.from(b))); ensure
you no longer return early on length mismatch and handle unequal lengths by
comparing equal-length buffers or explicitly returning false after a safe
compare.
---
Nitpick comments:
In `@tests/sdk-e2e/src/suites/share-operations.test.ts`:
- Around line 118-120: The test currently only asserts the length of the
unwrapped key; instead persist the original folder key from the create step
(assign new Uint8Array(folder.folderKey) to a suite-scoped variable like
sharedFolderKey when Alice creates the folder) and then in the assertion replace
the length check with a byte-wise comparison between the returned folderKey
(from unwrapKey(hexToBytes(share.encryptedKey), bob.privateKey)) and
sharedFolderKey to ensure the unwrapped bytes match exactly; keep using
unwrapKey, hexToBytes and share.encryptedKey to locate the code to change.
- Around line 133-139: The test currently sends an all-zero encryptedKey which
can trigger payload validation errors instead of the intended self-share
conflict; instead reuse a real ECIES-wrapped key for the same item by replacing
the zeroed encryptedKey with the actual wrapped key used when sharing the folder
(e.g., use the ECIES-wrapped key bytes for folder.ipnsName /
sharedFolderIpnsName from the suite setup), so that the POST body
(recipientPublicKey: '0x'+bytesToHex(alice.publicKey), itemType: 'folder',
ipnsName: alice.rootIpnsName, itemName: 'SelfShare', encryptedKey:
bytesToHex(...realWrappedKey...)) is valid and the test exercises the 409
self-share behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 981354a4-83a1-4bea-98c0-dc0e74a2c41f
⛔ Files ignored due to path filters (1)
packages/api-client/src/instance.tsis excluded by!packages/api-client/**
📒 Files selected for processing (21)
.github/workflows/load-test.yml.github/workflows/sdk-e2e.yml.planning/STATE.md.planning/todos/pending/2026-03-22-refactor-api-client-from-singleton-to-instance-scoped-axios.mdapps/api/src/app.module.tsapps/api/src/auth/auth.controller.tsapps/api/src/auth/controllers/identity.controller.tsapps/api/src/common/guards/throttler-bypass.guard.tsapps/api/src/device-approval/device-approval.controller.tsapps/api/src/ipns/ipns.controller.tsapps/api/src/shares/invites.controller.tsapps/api/src/shares/share-invites.controller.tsapps/api/src/shares/shares.controller.tstests/TESTING_STRATEGY.mdtests/load/.env.exampletests/sdk-e2e/.env.exampletests/sdk-e2e/src/fixtures/test-harness.tstests/sdk-e2e/src/suites/error-cases.test.tstests/sdk-e2e/src/suites/invite-link.test.tstests/sdk-e2e/src/suites/share-operations.test.tstests/sdk-e2e/src/suites/vault-lifecycle.test.ts
✅ Files skipped from review due to trivial changes (6)
- .planning/STATE.md
- apps/api/src/app.module.ts
- tests/load/.env.example
- .planning/todos/pending/2026-03-22-refactor-api-client-from-singleton-to-instance-scoped-axios.md
- .github/workflows/sdk-e2e.yml
- tests/sdk-e2e/src/suites/error-cases.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/sdk-e2e/.env.example
- .github/workflows/load-test.yml
- tests/sdk-e2e/src/suites/vault-lifecycle.test.ts
- tests/sdk-e2e/src/fixtures/test-harness.ts
- Add try/catch to folder and file workloads so individual operation failures are recorded as errors but don't abort the entire workload (matching the mixed-workload pattern) - Fix crypto.getRandomValues() 65536-byte limit in file-workload by chunking large file generation - Add error logging to file-workload for debugging upload failures Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: cec3086cee82
…Throttle decorators Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 0be7844f5d05
Contributor
There was a problem hiding this comment.
Pull request overview
Adds SDK-driven end-to-end correctness tests and Node-native load test scenarios to exercise CipherBox without a browser, plus CI workflows and an API throttling bypass mechanism to support high-volume test runs.
Changes:
- Introduces
@cipherbox/sdk-e2eVitest suites + fixtures/helpers to provision test accounts and driveCipherBoxClientagainst the live API. - Introduces
@cipherbox/load-testsscenarios + harness/metrics reporter for configurable multi-client workload generation. - Adds CI workflows for SDK E2E (PR/push, path-filtered) and manual load tests, and adds an API throttling bypass header/guard path for tests.
Reviewed changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sdk-e2e/vitest.config.ts | Vitest config for SDK E2E runtime/timeouts/serialization. |
| tests/sdk-e2e/tsconfig.json | TS config for SDK E2E package compilation/typechecking. |
| tests/sdk-e2e/src/suites/vault-lifecycle.test.ts | Vault init/GET/export/config/quota + auth negative tests via raw fetch. |
| tests/sdk-e2e/src/suites/share-operations.test.ts | Multi-account share creation/received/sent/revoke validation. |
| tests/sdk-e2e/src/suites/ipns-consistency.test.ts | IPNS sequence monotonicity + persistence across reloads. |
| tests/sdk-e2e/src/suites/invite-link.test.ts | Invite link lifecycle (create/status/data/claim/double-claim/revoke). |
| tests/sdk-e2e/src/suites/folder-crud.test.ts | Folder create/rename/move/delete + stress + negative cases. |
| tests/sdk-e2e/src/suites/file-operations.test.ts | File upload/download/rename/delete + round-trip verification. |
| tests/sdk-e2e/src/suites/error-cases.test.ts | Negative-path coverage (unloaded folders, bin not loaded, events, destroy). |
| tests/sdk-e2e/src/suites/data-integrity.test.ts | Byte-for-byte integrity tests across sizes + unicode + nested. |
| tests/sdk-e2e/src/suites/concurrent-operations.test.ts | “Rapid” mixed operation sequences to shake out ordering/state issues. |
| tests/sdk-e2e/src/suites/bin-operations.test.ts | Recycle-bin lifecycle coverage and persistence checks. |
| tests/sdk-e2e/src/helpers/data-generators.ts | Deterministic in-memory payload generators for tests. |
| tests/sdk-e2e/src/helpers/assertions.ts | Domain-specific assertions for folder tree + byte equality. |
| tests/sdk-e2e/src/fixtures/test-harness.ts | Shared account provisioning harness + throttle-bypass header injection. |
| tests/sdk-e2e/src/fixtures/multi-account.ts | Multi-account fixture that swaps api-client singleton between users. |
| tests/sdk-e2e/package.json | New workspace package definition + scripts/deps for SDK E2E. |
| tests/sdk-e2e/.env.example | Example env vars for SDK E2E auth + throttle bypass. |
| tests/load/vitest.config.ts | Vitest config for long-running load scenarios. |
| tests/load/tsconfig.json | TS config for load test package (including shared harness import). |
| tests/load/src/workloads/mixed-workload.ts | Weighted mixed-operation workload runner. |
| tests/load/src/workloads/folder-workload.ts | Folder CRUD cycle workload runner. |
| tests/load/src/workloads/file-workload.ts | Upload + optional download/verify workload runner. |
| tests/load/src/scenarios/upload-throughput.test.ts | Scenario: upload throughput measurement. |
| tests/load/src/scenarios/sustained-load.test.ts | Scenario: sustained load over time. |
| tests/load/src/scenarios/spike-test.test.ts | Scenario: baseline → burst comparison. |
| tests/load/src/scenarios/mixed-workload.test.ts | Scenario: mixed workload across clients. |
| tests/load/src/scenarios/ipns-publish-storm.test.ts | Scenario: IPNS publish contention stress. |
| tests/load/src/harness/reporter.ts | Console table + JSON report formatting. |
| tests/load/src/harness/metrics.ts | Per-operation timing/error/bytes metrics collection and aggregation. |
| tests/load/src/harness/client-pool.ts | Pool creation/cleanup + aggregation wrapper around scenario runs. |
| tests/load/package.json | New workspace package definition + scripts/deps for load tests. |
| tests/load/.env.example | Example env vars for load tests and throttle bypass. |
| tests/TESTING_STRATEGY.md | Documentation of test suite roles, triggers, and throttle strategy. |
| release-please-config.json | Includes new test packages in release-please extraFiles list. |
| pnpm-lock.yaml | Lockfile updates for the two new workspace test packages. |
| packages/api-client/src/instance.ts | Adds defaultHeaders support to singleton axios instance creation. |
| apps/api/src/shares/shares.controller.ts | Switches rate-limit guard import to bypassable throttler guard. |
| apps/api/src/shares/share-invites.controller.ts | Switches rate-limit guard import to bypassable throttler guard. |
| apps/api/src/shares/invites.controller.ts | Switches rate-limit guard import to bypassable throttler guard. |
| apps/api/src/ipns/ipns.controller.ts | Switches rate-limit guard import to bypassable throttler guard. |
| apps/api/src/device-approval/device-approval.controller.ts | Switches rate-limit guard import to bypassable throttler guard. |
| apps/api/src/common/guards/throttler-bypass.guard.ts | New guard enabling X-Throttle-Bypass header-based throttling bypass. |
| apps/api/src/auth/controllers/identity.controller.ts | Switches rate-limit guard import to bypassable throttler guard. |
| apps/api/src/auth/auth.controller.ts | Switches rate-limit guard import to bypassable throttler guard. |
| apps/api/src/app.module.ts | Documents test bypass behavior alongside throttler module config. |
| .planning/todos/pending/2026-03-22-refactor-api-client-from-singleton-to-instance-scoped-axios.md | Planning doc capturing singleton limitations and proposed refactor. |
| .planning/todos/pending/2026-03-22-investigate-throttle-bypass-gap-for-per-endpoint-throttle-decorators.md | Planning doc about observed 429s and investigation steps. |
| .planning/STATE.md | Updates planning state to reference pending todos. |
| .github/workflows/sdk-e2e.yml | New CI workflow to run SDK E2E tests on PR/push with service deps. |
| .github/workflows/load-test.yml | New manual workflow to run load test scenarios and upload artifacts. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
- Add NODE_ENV !== 'production' guard to BypassableThrottlerGuard - Use crypto.timingSafeEqual instead of custom implementation - Handle string[] header values (Express duplicate headers) - Add API health check failure path in CI workflows - Fix aggregateAndReport throughput calculation (use sample timestamps) - Write metrics JSON file for CI artifact upload - Preserve defaultHeaders in multi-account switchTo() - Assert specific error on unloaded destination moveItem - Fail file-workload verification when child not found in folder state - Fix concurrent-operations test description (serial, not concurrent) - Rename misleading nested folder test in folder-crud Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 415d2ea03a0a
FSM1
commented
Mar 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/load/src/harness/client-pool.ts (1)
64-71:⚠️ Potential issue | 🟠 MajorFail fast when pool provisioning under-delivers requested clients.
At Line 64-71, failed account creations are only warned. The function then returns a smaller pool (Line 88), which skews scenario semantics and throughput expectations.
Suggested patch
const elapsed = performance.now() - start; + if (clients.length !== clientCount) { + throw new Error( + `Pool provisioning incomplete: requested ${clientCount}, created ${clients.length}` + ); + } console.log( `Created ${clients.length}/${clientCount} clients in ${(elapsed / 1000).toFixed(1)}s` ); return clients;Also applies to: 83-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/harness/client-pool.ts` around lines 64 - 71, The current pooling logic collects Promise.allSettled results and only warns on rejections, returning a smaller clients array; change this to fail fast: after awaiting Promise.allSettled(promises) (the results variable), if any result.status === 'rejected' throw an Error (include count and reasons) instead of pushing successes and warning, so the function does not return an under-provisioned pool; apply the same change to the second similar block (the other Promise.allSettled handling around the clients array) so both code paths enforce exact client provisioning.apps/api/src/common/guards/throttler-bypass.guard.ts (1)
23-30:⚠️ Potential issue | 🟠 MajorAdd an explicit enable flag for throttle bypass.
At Line 23, bypass is enabled whenever
THROTTLE_BYPASS_SECRETexists andNODE_ENV !== 'production'. That is still too permissive; require a second explicit opt-in so bypass stays fail-closed unless intentionally enabled.Suggested patch
async canActivate(context: ExecutionContext): Promise<boolean> { const secret = process.env.THROTTLE_BYPASS_SECRET; + const bypassEnabled = process.env.THROTTLE_BYPASS_ENABLED === 'true'; - if (secret && process.env.NODE_ENV !== 'production') { + if (bypassEnabled && secret && process.env.NODE_ENV !== 'production') { const request = context.switchToHttp().getRequest(); const rawHeader = request.headers['x-throttle-bypass']; const header = Array.isArray(rawHeader) ? rawHeader[0] : rawHeader; if (header && safeEqual(header, secret)) { return true; } }As per coding guidelines, "
apps/api/**: Focus on security vulnerabilities and input validation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/common/guards/throttler-bypass.guard.ts` around lines 23 - 30, The current bypass logic in the ThrottlerBypass guard enables bypass whenever THROTTLE_BYPASS_SECRET exists and NODE_ENV !== 'production'; change this to require an explicit enable flag so bypass is fail-closed: read a second environment variable (e.g. THROTTLE_BYPASS_ENABLED or THROTTLE_BYPASS === 'true') and only proceed to check the secret when that flag is truthy and NODE_ENV !== 'production'; update the code path around the secret variable, the request header extraction (context.switchToHttp().getRequest() / rawHeader / header) and the safeEqual(secret, header) check so bypass returns true only when both the enable flag is set and the header matches the secret..github/workflows/load-test.yml (1)
179-183:⚠️ Potential issue | 🟠 MajorRun the scenario via
test:singleso filtering is actually applied.At Line 181-182, the workflow invokes
testand passes--testPathPatterndirectly. In this repo, scenario filtering is expected through thetest:singlescript, not the plaintestscript.Suggested patch
- name: Run load test run: | - pnpm --filter `@cipherbox/load-tests` test -- \ - --testPathPattern="${{ inputs.scenario }}" + pnpm --filter `@cipherbox/load-tests` test:single "${{ inputs.scenario }}"#!/bin/bash # Verify workflow command vs package scripts set -euo pipefail echo "=== load workflow command ===" rg -n --type yml -C2 "Run load test|@cipherbox/load-tests|testPathPattern|test:single" .github/workflows/load-test.yml echo "=== load package scripts ===" rg -n --type json -C2 "\"test\"|\"test:single\"|testPathPattern" tests/load/package.jsonBased on learnings: "In
.github/workflows/load-test.yml, ensure the CI step runstest:single(nottest) so scenario filtering via--testPathPatternis applied correctly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/load-test.yml around lines 179 - 183, The workflow step "Run load test" currently invokes the package script `test` with `--testPathPattern`, but scenario filtering is implemented in the `test:single` script; change the command to invoke `pnpm --filter `@cipherbox/load-tests` run test:single` (preserving the existing `--testPathPattern="${{ inputs.scenario }}"` argument) so the `testPathPattern` filtering is actually applied by the `test:single` script.
🧹 Nitpick comments (1)
tests/sdk-e2e/src/suites/concurrent-operations.test.ts (1)
1-22: Update describe block name to match the clarified purpose.The header comments (lines 1-15) now clearly describe these as "Rapid Sequential Operations Tests," which addresses the earlier concern. However, the
describeblock on line 22 still says'Concurrent Operations'. Rename it to match the header for consistency.✏️ Suggested rename
-describe('Concurrent Operations', () => { +describe('Rapid Sequential Operations', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/suites/concurrent-operations.test.ts` around lines 1 - 22, Rename the top-level test suite string in the describe call (describe('Concurrent Operations', ...)) to match the file header and intent — e.g., change it to "Rapid Sequential Operations Tests" — so the describe block string and the header comment are consistent; update the describe invocation text accordingly in the tests/sdk-e2e/src/suites/concurrent-operations.test.ts file.
🤖 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/load/src/harness/client-pool.ts`:
- Around line 77-80: setApiClientConfig is currently configured with baseUrl and
getAccessToken but omits propagating the X-Throttle-Bypass header; update the
setApiClientConfig call (used with API_URL and clients[0].accessToken) to
include a headers or getDefaultHeaders option that sets "X-Throttle-Bypass" to
the THROTTLE_BYPASS_SECRET (or reads it from env/config) so api-client-backed
calls include the bypass header and avoid unnecessary throttling during load
runs.
In `@tests/load/src/workloads/file-workload.ts`:
- Around line 58-62: The size mismatch check in file-workload.ts that currently
does console.warn when downloaded.length !== data.length should instead mark the
verification as failed: replace the warn with code that logs an error including
pc.id and both lengths, increment the verification failure metric (e.g.,
verificationFailures or similar metrics collector used by this workload), and
set/return a failure status for the current verification operation (or throw an
Error) so callers know the verification failed; update the block referencing
downloaded, data and pc.id accordingly and ensure any calling code that expects
a boolean/result handles the failure path.
---
Duplicate comments:
In @.github/workflows/load-test.yml:
- Around line 179-183: The workflow step "Run load test" currently invokes the
package script `test` with `--testPathPattern`, but scenario filtering is
implemented in the `test:single` script; change the command to invoke `pnpm
--filter `@cipherbox/load-tests` run test:single` (preserving the existing
`--testPathPattern="${{ inputs.scenario }}"` argument) so the `testPathPattern`
filtering is actually applied by the `test:single` script.
In `@apps/api/src/common/guards/throttler-bypass.guard.ts`:
- Around line 23-30: The current bypass logic in the ThrottlerBypass guard
enables bypass whenever THROTTLE_BYPASS_SECRET exists and NODE_ENV !==
'production'; change this to require an explicit enable flag so bypass is
fail-closed: read a second environment variable (e.g. THROTTLE_BYPASS_ENABLED or
THROTTLE_BYPASS === 'true') and only proceed to check the secret when that flag
is truthy and NODE_ENV !== 'production'; update the code path around the secret
variable, the request header extraction (context.switchToHttp().getRequest() /
rawHeader / header) and the safeEqual(secret, header) check so bypass returns
true only when both the enable flag is set and the header matches the secret.
In `@tests/load/src/harness/client-pool.ts`:
- Around line 64-71: The current pooling logic collects Promise.allSettled
results and only warns on rejections, returning a smaller clients array; change
this to fail fast: after awaiting Promise.allSettled(promises) (the results
variable), if any result.status === 'rejected' throw an Error (include count and
reasons) instead of pushing successes and warning, so the function does not
return an under-provisioned pool; apply the same change to the second similar
block (the other Promise.allSettled handling around the clients array) so both
code paths enforce exact client provisioning.
---
Nitpick comments:
In `@tests/sdk-e2e/src/suites/concurrent-operations.test.ts`:
- Around line 1-22: Rename the top-level test suite string in the describe call
(describe('Concurrent Operations', ...)) to match the file header and intent —
e.g., change it to "Rapid Sequential Operations Tests" — so the describe block
string and the header comment are consistent; update the describe invocation
text accordingly in the tests/sdk-e2e/src/suites/concurrent-operations.test.ts
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1abb79fb-5cd8-4e9e-b8d9-9fef654dbcea
📒 Files selected for processing (17)
.github/workflows/load-test.yml.github/workflows/sdk-e2e.yml.planning/todos/pending/2026-03-22-investigate-throttle-bypass-gap-for-per-endpoint-throttle-decorators.mdapps/api/src/common/guards/throttler-bypass.guard.tstests/load/src/harness/client-pool.tstests/load/src/harness/metrics.tstests/load/src/scenarios/ipns-publish-storm.test.tstests/load/src/scenarios/mixed-workload.test.tstests/load/src/scenarios/spike-test.test.tstests/load/src/scenarios/sustained-load.test.tstests/load/src/scenarios/upload-throughput.test.tstests/load/src/workloads/file-workload.tstests/load/src/workloads/folder-workload.tstests/sdk-e2e/src/fixtures/multi-account.tstests/sdk-e2e/src/suites/concurrent-operations.test.tstests/sdk-e2e/src/suites/error-cases.test.tstests/sdk-e2e/src/suites/folder-crud.test.ts
✅ Files skipped from review due to trivial changes (6)
- tests/load/src/scenarios/ipns-publish-storm.test.ts
- tests/load/src/scenarios/spike-test.test.ts
- tests/load/src/scenarios/sustained-load.test.ts
- tests/sdk-e2e/src/suites/error-cases.test.ts
- tests/sdk-e2e/src/suites/folder-crud.test.ts
- .github/workflows/sdk-e2e.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/load/src/scenarios/mixed-workload.test.ts
- tests/load/src/scenarios/upload-throughput.test.ts
The controller specs were overriding ThrottlerGuard from @nestjs/throttler, but controllers now use BypassableThrottlerGuard. NestJS couldn't resolve the guard's THROTTLER:MODULE_OPTIONS dependency in the test module context. Fix: override BypassableThrottlerGuard instead of ThrottlerGuard in all 8 affected spec files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 644af0f89425
…smatch - Add defaultHeaders with X-Throttle-Bypass to setApiClientConfig in createClientPool (was missing, causing 429s in load tests) - Throw on download size mismatch in file-workload instead of just logging a warning (properly records verification failure as error) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1441655f35a9
There was a problem hiding this comment.
♻️ Duplicate comments (4)
.github/workflows/load-test.yml (2)
179-182:⚠️ Potential issue | 🟠 MajorUse the scenario-filtered npm script in CI (
test:single).The workflow currently invokes
testand passes--testPathPatterndirectly, so scenario filtering can be skipped/misapplied in CI.Suggested patch
- name: Run load test run: | - pnpm --filter `@cipherbox/load-tests` test -- \ - --testPathPattern="${{ inputs.scenario }}" + pnpm --filter `@cipherbox/load-tests` test:single -- \ + "${{ inputs.scenario }}"#!/bin/bash set -euo pipefail echo "Workflow run step:" sed -n '176,184p' .github/workflows/load-test.yml echo echo "Load test scripts:" cat tests/load/package.json | jq '.scripts'Expected verification result: workflow should call
test:single, and scenario value should be passed as positional argument.Based on learnings, the load-test workflow should call
test:single(nottest) so scenario filtering is applied correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/load-test.yml around lines 179 - 182, The workflow's "Run load test" step calls the npm script "test" and passes --testPathPattern, which bypasses the intended scenario filtering; change the step to run the scenario-filtered script "test:single" instead and pass the scenario as a positional argument (using inputs.scenario) so the script can apply filtering correctly; update the run command referenced in the step (the block that currently invokes pnpm --filter `@cipherbox/load-tests` test -- --testPathPattern="${{ inputs.scenario }}") to call pnpm --filter `@cipherbox/load-tests` run test:single and forward the scenario value as an argument to that script.
139-146:⚠️ Potential issue | 🟡 MinorRequire HTTP success in readiness checks (
curl -f).
curl -streats HTTP 500 as a successful command, so the API can be marked “ready” while unhealthy.Suggested patch
for i in $(seq 1 30); do - if curl -s http://localhost:3000/health > /dev/null 2>&1; then + if curl -fsS http://localhost:3000/health > /dev/null; then echo "API is ready" break fi sleep 1 done - if ! curl -s http://localhost:3000/health > /dev/null 2>&1; then + if ! curl -fsS http://localhost:3000/health > /dev/null; then echo "API failed to start within 30s" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/load-test.yml around lines 139 - 146, The readiness check uses plain `curl -s` which treats HTTP 5xx as success; update both curl invocations in the readiness loop and the final check to use `curl -sf` (or `curl -s --fail`) so non-2xx responses make the command fail, ensuring the health endpoint must return HTTP success before marking the API ready; modify the `if curl -s http://localhost:3000/health` in the loop and the `if ! curl -s http://localhost:3000/health` final check accordingly.tests/load/src/workloads/file-workload.ts (1)
58-62:⚠️ Potential issue | 🟡 MinorTreat byte-length mismatch as a verification failure.
This branch only warns, so integrity regressions are underreported in workload results.
Suggested patch
if (downloaded.length !== data.length) { - console.warn( - `[Client ${pc.id}] Size mismatch: uploaded ${data.length}, downloaded ${downloaded.length}` - ); + throw new Error( + `[Client ${pc.id}] Size mismatch: uploaded ${data.length}, downloaded ${downloaded.length}` + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/workloads/file-workload.ts` around lines 58 - 62, The size-mismatch branch currently only logs a warning which hides verification failures; replace the console.warn call inside the block that compares downloaded.length and data.length with a proper failure signal (e.g., throw a descriptive Error or call the test harness failure API) so the workload records a verification failure. Use the existing variables pc.id, data.length and downloaded.length in the message so the thrown Error (or failure call) clearly identifies the client and sizes.tests/load/src/harness/client-pool.ts (1)
77-80:⚠️ Potential issue | 🟠 MajorPropagate throttle-bypass headers into api-client config.
The singleton config currently omits
defaultHeaders, so api-client-backed calls may missX-Throttle-Bypassand get throttled during load runs.Suggested patch
if (clients.length > 0) { + const defaultHeaders = process.env.THROTTLE_BYPASS_SECRET + ? { 'X-Throttle-Bypass': process.env.THROTTLE_BYPASS_SECRET } + : undefined; + setApiClientConfig({ baseUrl: API_URL, getAccessToken: async () => clients[0].accessToken, + defaultHeaders, }); }As per coding guidelines, “verify
BypassableThrottlerGuard.canActivate()actually seesX-Throttle-Bypasson IPNS publish requests (header propagation via axiosdefaultHeaders).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/harness/client-pool.ts` around lines 77 - 80, The api-client config needs to propagate the X-Throttle-Bypass header so BypassableThrottlerGuard.canActivate() sees it: update the setApiClientConfig call to include defaultHeaders that expose the throttle-bypass value from the test client (e.g. add defaultHeaders: { 'X-Throttle-Bypass': clients[0].throttleBypass } or the appropriate field where the test stores that header) alongside baseUrl and getAccessToken so axios-backed calls include the header during load tests.
🧹 Nitpick comments (1)
tests/load/src/scenarios/spike-test.test.ts (1)
18-18: HardenLOAD_TEST_CLIENTSparsing to avoid accidental empty burst runs.If
LOAD_TEST_CLIENTSis non-numeric,parseIntyieldsNaNand the burst phase can silently run with zero clients.Suggested patch
-const BURST_CLIENTS = parseInt(process.env.LOAD_TEST_CLIENTS ?? '20', 10); +const parsedBurstClients = Number.parseInt(process.env.LOAD_TEST_CLIENTS ?? '20', 10); +const BURST_CLIENTS = Number.isFinite(parsedBurstClients) && parsedBurstClients > 0 + ? parsedBurstClients + : 20;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/scenarios/spike-test.test.ts` at line 18, The BURST_CLIENTS constant in spike-test.test.ts is using parseInt directly which can produce NaN for non-numeric env values and lead to zero-client bursts; change the assignment so you parse the env value into a variable (e.g., let parsed = parseInt(process.env.LOAD_TEST_CLIENTS ?? '', 10)), then validate it (if Number.isNaN(parsed) or parsed <= 0) and fallback to a safe default (e.g., 20) before assigning to const BURST_CLIENTS; update only the variable initialization logic around BURST_CLIENTS to ensure it never becomes NaN or a non-positive number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/load-test.yml:
- Around line 179-182: The workflow's "Run load test" step calls the npm script
"test" and passes --testPathPattern, which bypasses the intended scenario
filtering; change the step to run the scenario-filtered script "test:single"
instead and pass the scenario as a positional argument (using inputs.scenario)
so the script can apply filtering correctly; update the run command referenced
in the step (the block that currently invokes pnpm --filter
`@cipherbox/load-tests` test -- --testPathPattern="${{ inputs.scenario }}") to
call pnpm --filter `@cipherbox/load-tests` run test:single and forward the
scenario value as an argument to that script.
- Around line 139-146: The readiness check uses plain `curl -s` which treats
HTTP 5xx as success; update both curl invocations in the readiness loop and the
final check to use `curl -sf` (or `curl -s --fail`) so non-2xx responses make
the command fail, ensuring the health endpoint must return HTTP success before
marking the API ready; modify the `if curl -s http://localhost:3000/health` in
the loop and the `if ! curl -s http://localhost:3000/health` final check
accordingly.
In `@tests/load/src/harness/client-pool.ts`:
- Around line 77-80: The api-client config needs to propagate the
X-Throttle-Bypass header so BypassableThrottlerGuard.canActivate() sees it:
update the setApiClientConfig call to include defaultHeaders that expose the
throttle-bypass value from the test client (e.g. add defaultHeaders: {
'X-Throttle-Bypass': clients[0].throttleBypass } or the appropriate field where
the test stores that header) alongside baseUrl and getAccessToken so
axios-backed calls include the header during load tests.
In `@tests/load/src/workloads/file-workload.ts`:
- Around line 58-62: The size-mismatch branch currently only logs a warning
which hides verification failures; replace the console.warn call inside the
block that compares downloaded.length and data.length with a proper failure
signal (e.g., throw a descriptive Error or call the test harness failure API) so
the workload records a verification failure. Use the existing variables pc.id,
data.length and downloaded.length in the message so the thrown Error (or failure
call) clearly identifies the client and sizes.
---
Nitpick comments:
In `@tests/load/src/scenarios/spike-test.test.ts`:
- Line 18: The BURST_CLIENTS constant in spike-test.test.ts is using parseInt
directly which can produce NaN for non-numeric env values and lead to
zero-client bursts; change the assignment so you parse the env value into a
variable (e.g., let parsed = parseInt(process.env.LOAD_TEST_CLIENTS ?? '', 10)),
then validate it (if Number.isNaN(parsed) or parsed <= 0) and fallback to a safe
default (e.g., 20) before assigning to const BURST_CLIENTS; update only the
variable initialization logic around BURST_CLIENTS to ensure it never becomes
NaN or a non-positive number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c529729-e6a5-4ae7-85d6-538e239e304c
📒 Files selected for processing (25)
.github/workflows/load-test.yml.github/workflows/sdk-e2e.yml.planning/todos/pending/2026-03-22-investigate-throttle-bypass-gap-for-per-endpoint-throttle-decorators.mdapps/api/src/auth/auth.controller.spec.tsapps/api/src/auth/controllers/identity.controller.spec.tsapps/api/src/common/guards/throttler-bypass.guard.tsapps/api/src/device-approval/device-approval.controller.spec.tsapps/api/src/ipns/__tests__/ipns.integration.spec.tsapps/api/src/ipns/__tests__/ipns.security.spec.tsapps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/share-invites.controller.spec.tsapps/api/src/shares/shares.controller.spec.tstests/load/src/harness/client-pool.tstests/load/src/harness/metrics.tstests/load/src/scenarios/ipns-publish-storm.test.tstests/load/src/scenarios/mixed-workload.test.tstests/load/src/scenarios/spike-test.test.tstests/load/src/scenarios/sustained-load.test.tstests/load/src/scenarios/upload-throughput.test.tstests/load/src/workloads/file-workload.tstests/load/src/workloads/folder-workload.tstests/sdk-e2e/src/fixtures/multi-account.tstests/sdk-e2e/src/suites/concurrent-operations.test.tstests/sdk-e2e/src/suites/error-cases.test.tstests/sdk-e2e/src/suites/folder-crud.test.ts
✅ Files skipped from review due to trivial changes (11)
- apps/api/src/auth/controllers/identity.controller.spec.ts
- tests/load/src/scenarios/ipns-publish-storm.test.ts
- apps/api/src/common/guards/throttler-bypass.guard.ts
- tests/load/src/scenarios/mixed-workload.test.ts
- tests/sdk-e2e/src/suites/concurrent-operations.test.ts
- tests/load/src/scenarios/upload-throughput.test.ts
- tests/load/src/scenarios/sustained-load.test.ts
- .github/workflows/sdk-e2e.yml
- tests/sdk-e2e/src/suites/error-cases.test.ts
- tests/load/src/harness/metrics.ts
- tests/sdk-e2e/src/suites/folder-crud.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/sdk-e2e/src/fixtures/multi-account.ts
- tests/load/src/workloads/folder-workload.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
==========================================
+ Coverage 50.36% 50.49% +0.13%
==========================================
Files 114 116 +2
Lines 9162 9888 +726
Branches 716 720 +4
==========================================
+ Hits 4614 4993 +379
- Misses 4371 4718 +347
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Apply defaultHeaders in createAxiosInstance() (was only in singleton) - Fix safeEqual to compare Buffer byte lengths, not string lengths - Use grouped regex /^(k51|bafz)/ for clarity - Guard against division by zero in reporter throughput calculation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: f413a30234b4
Vitest v3 doesn't support --testPathPattern as a CLI flag (that's Jest). The test script silently ignored it, causing all scenarios to run. Use test:single which has --testPathPattern embedded in the script def. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4ee44666a27d
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/load-test.yml (1)
138-146:⚠️ Potential issue | 🟠 MajorUse HTTP-failing curl flags in readiness checks.
At Line 139 and Line 145,
curl -sreturns success even for HTTP 500, so the API can be treated as “ready” while unhealthy. Use-fsSso only successful HTTP responses pass.Suggested patch
- name: Start API server if: inputs.environment == 'local' run: | pnpm --filter `@cipherbox/api` start:prod & for i in $(seq 1 30); do - if curl -s http://localhost:3000/health > /dev/null 2>&1; then + if curl -fsS http://localhost:3000/health > /dev/null; then echo "API is ready" break fi sleep 1 done - if ! curl -s http://localhost:3000/health > /dev/null 2>&1; then + if ! curl -fsS http://localhost:3000/health > /dev/null; then echo "API failed to start within 30s" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/load-test.yml around lines 138 - 146, The readiness check uses curl -s which treats HTTP error responses as success; update both curl invocations in the readiness loop and the final check to use curl -fsS (or equivalent failing flags) so only 2xx/3xx responses mark the API as ready; locate the two curl calls in the loop and the subsequent if (the loop that echoes "API is ready" and the final if that echoes "API failed to start within 30s") and replace the -s flag with -fsS.
🤖 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/load-test.yml:
- Around line 14-17: The workflow accepts a free-form input "client_count" but
exports it directly as LOAD_TEST_CLIENTS, which can be empty, non-numeric, or
non-positive; update the workflow to validate and normalize "client_count"
before exporting by coalescing to a safe positive integer (e.g., parse to int,
ensure >=1) and either set a fallback default or fail fast with a clear error
message; locate the input name "client_count" and the export of
"LOAD_TEST_CLIENTS" in the steps that set environment variables and add a
validation step that checks numeric/positive-ness and transforms the value into
a valid integer for downstream scenario parsing.
In `@tests/load/src/workloads/file-workload.ts`:
- Around line 47-67: The verification errors (file not found and size mismatch)
are thrown outside metrics.measure('downloadFile', ...) so they aren't captured
as measured errors; wrap the download plus the subsequent verification logic
inside the same metrics.measure callback (i.e., move the file existence check
and the size comparison into the async function passed to metrics.measure used
around client.downloadFromIpns), or alternatively call metrics.measure again for
the verification step so any thrown Error is recorded; update the block that
references verifyDownloads, fileIpnsName, client.downloadFromIpns, downloaded
and metrics.measure to ensure throws occur inside the measured callback so
aggregateAndReport will count them.
---
Duplicate comments:
In @.github/workflows/load-test.yml:
- Around line 138-146: The readiness check uses curl -s which treats HTTP error
responses as success; update both curl invocations in the readiness loop and the
final check to use curl -fsS (or equivalent failing flags) so only 2xx/3xx
responses mark the API as ready; locate the two curl calls in the loop and the
subsequent if (the loop that echoes "API is ready" and the final if that echoes
"API failed to start within 30s") and replace the -s flag with -fsS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8986f2b5-a239-4d52-886f-a6fd0b01f1f5
⛔ Files ignored due to path filters (1)
packages/api-client/src/instance.tsis excluded by!packages/api-client/**
📒 Files selected for processing (7)
.github/workflows/load-test.ymlapps/api/src/common/guards/throttler-bypass.guard.tstests/load/src/harness/client-pool.tstests/load/src/harness/reporter.tstests/load/src/workloads/file-workload.tstests/sdk-e2e/src/suites/folder-crud.test.tstests/sdk-e2e/src/suites/vault-lifecycle.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/load/src/harness/reporter.ts
- tests/sdk-e2e/src/suites/folder-crud.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/common/guards/throttler-bypass.guard.ts
- tests/load/src/harness/client-pool.ts
- Validate client_count input is a positive integer before exporting - Move verification errors inside metrics.measure callback so they are captured in error metrics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 3e950d8ba73e
Eliminates the dual-path architecture where the orval singleton and CipherBoxClient had separate axios instances: - createAxiosInstance now supports 401 refresh (moved from getCachedInstance) - setApiClientConfig accepts optional pre-built instance - api-config.ts creates one shared instance, registers it as the singleton - CipherBoxClientConfig gains optional axiosInstance field - sdk-provider.ts passes the shared instance to CipherBoxClient - Both orval-generated function calls and SDK operations now use the exact same axios instance Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ecba7ad248b8
Entire-Checkpoint: 475a18fd0ade
addToIpfs and unpinFromIpfs now use ctx.axiosInstance (with auth interceptors and default headers) instead of bare axios with manual token injection. This ensures IPFS operations get the same 401 refresh and default headers as IPNS operations. fetchFromIpfs still uses native fetch for streaming download progress support. Closes the last remaining direct-axios path in sdk-core. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 40cf1e51e1ea
Owner
Author
|
@coderabbitai please review the latest changes since the last review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
tests/sdk-e2e/src/fixtures/test-harness.ts (1)
141-159:⚠️ Potential issue | 🟠 MajorRetry transient account-deletion failures in the shared helper.
Every SDK E2E suite and the load harness funnel teardown through this function. Right now any 429/5xx/network blip is logged and dropped, which leaves orphaned test users behind and contaminates later runs.
🛠️ Suggested retry loop
export async function deleteTestAccount( ctx: { accessToken: string }, apiUrl = API_URL ): Promise<void> { - try { - const res = await fetch(`${apiUrl}/auth/account`, { - method: 'DELETE', - headers: fetchHeaders({ - Authorization: `Bearer ${ctx.accessToken}`, - 'Content-Type': 'application/json', - }), - body: JSON.stringify({ confirmation: 'DELETE' }), - }); - if (!res.ok) { - console.warn(`Account deletion failed (${res.status}): ${await res.text()}`); - } - } catch (err) { - console.warn(`Account deletion error: ${(err as Error).message}`); - } + let delayMs = 500; + for (let attempt = 1; attempt <= 5; attempt++) { + try { + const res = await fetch(`${apiUrl}/auth/account`, { + method: 'DELETE', + headers: fetchHeaders({ + Authorization: `Bearer ${ctx.accessToken}`, + 'Content-Type': 'application/json', + }), + body: JSON.stringify({ confirmation: 'DELETE' }), + }); + + if (res.ok) return; + if (![429, 502, 503, 504].includes(res.status) || attempt === 5) { + console.warn(`Account deletion failed (${res.status}): ${await res.text()}`); + return; + } + } catch (err) { + if (attempt === 5) { + console.warn(`Account deletion error: ${(err as Error).message}`); + return; + } + } + + await new Promise((resolve) => setTimeout(resolve, delayMs)); + delayMs *= 2; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/fixtures/test-harness.ts` around lines 141 - 159, The deleteTestAccount helper silently drops transient failures; change deleteTestAccount to retry transient errors (HTTP 429 and 5xx) and network exceptions with an exponential/backoff loop: wrap the fetch call in a retry loop (e.g., maxAttempts 3–5) that checks response.status (retry when status === 429 or status >= 500) and catches network errors to retry, awaiting an increasing delay between attempts; if a non-retryable error (2xx/4xx other than 429) occurs log and return, and if attempts are exhausted log an error including final status/text or exception—refer to deleteTestAccount, ctx.accessToken, API_URL, and fetchHeaders to locate and modify the code.
🧹 Nitpick comments (11)
tests/sdk-e2e/src/helpers/data-generators.ts (1)
10-15: ValidatesizeBytesto fail fast with a clearer error.
Uint8Array(sizeBytes)will throw for invalid sizes, but adding an explicit guard makes failures easier to diagnose in tests.♻️ Suggested patch
export function generateBytes(sizeBytes: number, seed = 42): Uint8Array { + if (!Number.isInteger(sizeBytes) || sizeBytes < 0) { + throw new Error(`sizeBytes must be a non-negative integer, got: ${sizeBytes}`); + } const data = new Uint8Array(sizeBytes); for (let i = 0; i < sizeBytes; i++) { data[i] = (seed + i * 7 + (i >> 8) * 13) & 0xff; } return data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/helpers/data-generators.ts` around lines 10 - 15, The generateBytes function should validate the sizeBytes argument to fail fast with a clear error: check that sizeBytes is a finite integer >= 0 (and of type number) before constructing the Uint8Array and throw a descriptive RangeError or TypeError if invalid; update the input guard in generateBytes to validate and throw something like "generateBytes: sizeBytes must be a non-negative integer" so tests get a clear diagnostic instead of an internal Uint8Array error.apps/api/src/common/guards/throttler-bypass.guard.ts (1)
25-27: Fail closed on multi-value bypass headers.For a security-sensitive bypass header, accepting
string[]and using the first value is ambiguous. Prefer only a single string value and reject arrays outright.As per coding guidelines, "`apps/api/**`: Focus on security vulnerabilities and input validation".🔐 Suggested hardening
- const rawHeader = request.headers['x-throttle-bypass']; - const header = Array.isArray(rawHeader) ? rawHeader[0] : rawHeader; + const rawHeader = request.headers['x-throttle-bypass']; + const header = typeof rawHeader === 'string' ? rawHeader : undefined; if (header && safeEqual(header, secret)) { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/common/guards/throttler-bypass.guard.ts` around lines 25 - 27, The guard currently accepts multi-value headers by taking the first element (rawHeader -> header) which is ambiguous and unsafe; update the check in the throttler bypass logic (the rawHeader/header/secret comparison that uses safeEqual) to reject arrays outright by returning false or treating the header as absent when Array.isArray(rawHeader) is true, and only proceed if typeof rawHeader === 'string' (assign header = rawHeader as string) before calling safeEqual(header, secret), ensuring a fail-closed behavior for multi-value headers.tests/sdk-e2e/src/helpers/assertions.ts (1)
53-68: Consider adding type annotations forgetChildandgetChildren.The
anyreturn types lose type safety. Since the folder tree containsFolderChildentries from@cipherbox/core, typed returns would improve IDE support and catch type errors in tests.💡 Optional type improvement
+import type { FolderChild } from '@cipherbox/core'; + /** * Get a child entry from the folder tree by name. * Throws if not found. */ -export function getChild(client: CipherBoxClient, folderIpnsName: string, name: string): any { +export function getChild(client: CipherBoxClient, folderIpnsName: string, name: string): FolderChild { const folder = client.getFolderTree().get(folderIpnsName); if (!folder) throw new Error(`Folder ${folderIpnsName} not in tree`); const match = folder.children.find((c) => c.name === name); if (!match) throw new Error(`Child "${name}" not found in folder`); return match; } /** * Get all children from the folder tree. */ -export function getChildren(client: CipherBoxClient, folderIpnsName: string): any[] { +export function getChildren(client: CipherBoxClient, folderIpnsName: string): FolderChild[] { const folder = client.getFolderTree().get(folderIpnsName); if (!folder) throw new Error(`Folder ${folderIpnsName} not in tree`); return folder.children; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/helpers/assertions.ts` around lines 53 - 68, The functions getChild and getChildren currently return any; import the FolderChild type from `@cipherbox/core` and update signatures so getChild(client: CipherBoxClient, folderIpnsName: string, name: string): FolderChild and getChildren(client: CipherBoxClient, folderIpnsName: string): FolderChild[]; also ensure the local variables (the folder lookup and match result in getChild) are typed/used as FolderChild to avoid any casting surprises.tests/sdk-e2e/.env.example (1)
8-8: Prefer non-usable placeholders for secret defaults.Using obvious placeholders reduces accidental reuse of weak/shared secrets outside local dev.
🔧 Suggested update
-SDK_E2E_SECRET=e2e-test-secret-do-not-use-in-production +SDK_E2E_SECRET=CHANGE_ME_LOCAL_ONLY -THROTTLE_BYPASS_SECRET=test-throttle-bypass-secret +THROTTLE_BYPASS_SECRET=CHANGE_ME_LOCAL_ONLYAlso applies to: 11-11
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/.env.example` at line 8, Replace the real-looking default secret value for SDK_E2E_SECRET with a non-usable placeholder to avoid accidental reuse; edit the SDK_E2E_SECRET entry (and the other occurrence mentioned) so the value is a clearly-non-functional token such as REPLACE_ME_SDK_E2E_SECRET or UNSET_SDK_E2E_SECRET and add a short comment if supported indicating it must be set in CI/local env before running tests.tests/load/.env.example (1)
8-8: Prefer non-functional placeholders for secret defaults.Using concrete secret-like defaults in example files increases the chance they get copied unchanged into shared environments. Consider using explicit
CHANGE_ME_*placeholders instead.Suggested update
-LOAD_TEST_SECRET=e2e-test-secret-do-not-use-in-production +LOAD_TEST_SECRET=CHANGE_ME_LOAD_TEST_SECRET @@ -THROTTLE_BYPASS_SECRET=test-throttle-bypass-secret +THROTTLE_BYPASS_SECRET=CHANGE_ME_THROTTLE_BYPASS_SECRETAlso applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/.env.example` at line 8, Replace the concrete secret default for LOAD_TEST_SECRET with a non-functional placeholder to avoid accidental reuse (e.g., use a CHANGE_ME_* token); update the value for LOAD_TEST_SECRET in the tests/load/.env.example and likewise replace the other occurrence noted (line 17) so both use an explicit placeholder such as CHANGE_ME_LOAD_TEST_SECRET or <CHANGE_ME_LOAD_TEST_SECRET> instead of an actual-looking secret.tests/load/src/scenarios/spike-test.test.ts (1)
66-77: Consider guarding against zero baseline latency in degradation calculation.If
baseOp.latency.p95is zero (edge case if no samples collected), the division will produceInfinityorNaN.♻️ Suggested defensive check
if (burstOp) { - const degradation = ( - ((burstOp.latency.p95 - baseOp.latency.p95) / baseOp.latency.p95) * - 100 - ).toFixed(0); + const degradation = baseOp.latency.p95 > 0 + ? (((burstOp.latency.p95 - baseOp.latency.p95) / baseOp.latency.p95) * 100).toFixed(0) + : 'N/A'; console.log(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/load/src/scenarios/spike-test.test.ts` around lines 66 - 77, The degradation calculation can divide by zero when baseOp.latency.p95 is 0; in the loop that iterates baselineMetrics and finds burstOp from burstMetrics (using baseOp and burstOp variables), add a defensive check on baseOp.latency.p95 before computing degradation — if it is 0 or falsy, skip the percentage calculation and log a safe placeholder like "N/A" (or treat degradation as 0) instead of performing the division, then print the same console message using that placeholder; ensure you reference baseOp.latency.p95 and burstOp.latency.p95 when performing the guarded computation.tests/sdk-e2e/src/suites/concurrent-operations.test.ts (1)
22-22: Consider renaming the describe block to match the updated header.The file header comments were updated to "Rapid Sequential Operations" (per the previous review fix), but the
describeblock still uses'Concurrent Operations'. This mismatch could cause confusion when viewing test results.Suggested fix
-describe('Concurrent Operations', () => { +describe('Rapid Sequential Operations', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/suites/concurrent-operations.test.ts` at line 22, Rename the describe block string to match the updated file header by changing the existing describe('Concurrent Operations', ...) to describe('Rapid Sequential Operations', ...); update any matching test suite title usages in the same file (the describe block and any snapshot/test reporting strings) so the suite name and header comment are consistent.tests/sdk-e2e/src/suites/bin-operations.test.ts (1)
50-100: Prefer publicloadBin()assertions over internalbinStateaccess.Relying on
(ctx.client as any).binStateties tests to internals and makes them brittle. For E2E quality, assert state transitions through public APIs (loadBin) after each mutation.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".Also applies to: 116-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/suites/bin-operations.test.ts` around lines 50 - 100, Replace direct internal access to (ctx.client as any).binState with assertions using the public API ctx.client.loadBin(): after operations like deleteToBin, restoreFromBin, and permanentDelete call await ctx.client.loadBin() and assert on the returned entries array (e.g., check length, find by name, itemType) instead of reading binState; update tests that currently reference binState (including the case that checks BinFolder and bin-file.txt) to use loadBin() for both pre- and post-mutation verification so tests rely only on public methods (also update any other occurrences that access binState directly, e.g., the other test block that inspects BinFolder).tests/sdk-e2e/src/suites/error-cases.test.ts (1)
151-153: Make the post-destroy()failure assertion specific.Using a generic
.toThrow()allows unrelated errors to pass this test. Prefer asserting a stable destroyed-client error substring (or specific error type) to keep this check semantically tight.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 `@tests/sdk-e2e/src/suites/error-cases.test.ts` around lines 151 - 153, The test currently uses a generic await expect(tempCtx.client.createFolder(...)).rejects.toThrow(); change this to assert a specific destroyed-client error substring or type so unrelated errors don't pass: update the expectation to await expect(tempCtx.client.createFolder(tempCtx.rootIpnsName, 'PostDestroy')).rejects.toThrowError(/destroy(ed)? client|client has been destroyed/i) or match the exact error message/type your client throws after calling destroy(), referencing tempCtx.client.createFolder and the client's destroy() behavior so the assertion is tightly scoped.tests/sdk-e2e/src/suites/file-operations.test.ts (1)
32-151: Reduce cross-test state coupling in this suite.A few cases depend on side effects from earlier tests (e.g.,
hello.txt→ rename → delete). Consider making each case self-contained (or seed state in hooks) to avoid cascade failures and improve single-test reruns.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 `@tests/sdk-e2e/src/suites/file-operations.test.ts` around lines 32 - 151, Tests in this suite are tightly coupled (e.g., hello.txt created in one test is renamed and deleted in later tests) causing cascade failures; make each test self-contained by having each test create (via ctx.client.uploadFile or ctx.client.createFolder + registerFolder) the specific files/folders it needs using unique names (e.g., include Date.now() or a UUID), use beforeEach to seed required items and afterEach to clean up via deleteItem, and update references that currently rely on shared state (getChild, expectChildNamed, renameItem, deleteItem) to operate on the locally created item's id/ipnsName so tests can run independently and be rerun individually.tests/sdk-e2e/src/suites/folder-crud.test.ts (1)
17-29: Avoid splitting one stateful workflow across multipleitblocks.Most cases here assume folders created or renamed by earlier tests. That makes targeted reruns brittle and turns one failure into a cascade. If the single shared account is important for speed, I’d collapse the dependent chain into one flow test instead of spreading it across separate cases.
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."Also applies to: 31-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/suites/folder-crud.test.ts` around lines 17 - 29, The tests under describe('Folder CRUD') split a single stateful workflow across multiple it blocks using the shared ctx from createTestContext in beforeAll, which makes reruns brittle; either collapse the dependent steps into one end-to-end test (e.g., a single it('full folder CRUD flow') that performs create → rename → delete in sequence) or make each test independent by moving setup/teardown into beforeEach/afterEach and creating/cleaning its own context (use createTestContext and ctx.cleanup/deleteTestAccount within per-test setup/teardown) so no test relies on side effects from others.
🤖 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/app.module.ts`:
- Around line 50-51: Update the module comment explaining
BypassableThrottlerGuard to accurately reflect runtime behavior: state that the
X-Throttle-Bypass header can be used to skip rate limits only when
THROTTLE_BYPASS_SECRET is set and NODE_ENV !== 'production' (bypass is blocked
in production), so modify the comment above BypassableThrottlerGuard in
app.module.ts to include the NODE_ENV === 'production' restriction for
operators.
In `@tests/load/src/harness/client-pool.ts`:
- Around line 32-38: Update the docblock to remove references to a removed
shared api-client singleton and clarify the current behavior: state that the
helper creates N authenticated CipherBoxClient instances by delegating to
createTestAccount(), each client gets its own test account, vault, and metrics
collector, and each client manages its own auth tokens (no shared api-client or
global token handling). Mention createTestAccount and CipherBoxClient by name so
readers can locate the implementation.
In `@tests/load/src/workloads/file-workload.ts`:
- Around line 29-30: Check and guard the minSize/maxSize bounds before computing
size in file-workload.ts: validate that maxSize >= minSize (or that both are
non-negative) at the start of the workload setup (the code that defines minSize,
maxSize and computes size), and if the check fails either throw a clear error or
normalize the values (e.g., swap or set maxSize = minSize) before the size
calculation and new Uint8Array(size) allocation; reference the variables
minSize, maxSize and the allocation of new Uint8Array(size) so the validation is
added immediately prior to computing size.
- Around line 60-65: The current check in the download verification (inside the
code that calls client.downloadFromIpns and compares downloaded.length to
data.length) can miss corruption when sizes match; update the verification in
the verify/download logic to compare the actual payload contents rather than
just lengths—e.g., compute and compare a digest (hash) or perform a
byte-for-byte equality between downloaded and data (referencing variables
downloaded and data, and the client.downloadFromIpns call) and, on mismatch,
throw a descriptive error that includes context such as pc.id and rootFolderKey.
In `@tests/load/src/workloads/mixed-workload.ts`:
- Around line 36-37: The merged weights (weights = { ...DEFAULT_WEIGHTS,
...opts.weights }) must be validated before using totalWeight: ensure every key
in weights is a finite number (not undefined, NaN, or Infinity) and >= 0, and
that totalWeight (computed via Object.values(weights).reduce...) is > 0; if any
check fails, throw a clear error (or reject the config) referencing the
offending key(s) so the caller can fix opts.weights rather than silently
producing wrong workload behavior—apply this validation just after computing
weights/totalWeight in mixed-workload.ts.
In `@tests/sdk-e2e/src/fixtures/test-harness.ts`:
- Around line 80-127: After successfully obtaining accessToken from loginRes,
the setup steps (initializeVault, encryptVaultKeys, deriveIpnsName, the
/vault/init fetch, CipherBoxClient construction, and client.registerFolder) can
fail and currently leak the partly-provisioned test account; wrap steps that
begin with initializeVault and end after client.registerFolder in a try/catch,
and in the catch call await deleteTestAccount({ accessToken }, apiUrl) to
rollback the created account before rethrowing the original error so failures
don’t leave test accounts behind.
- Around line 107-116: CipherBoxClientConfig should not include rootIpnsName,
rootFolderKey, or teeKeys because these are discovered at runtime; remove those
fields from the CipherBoxClientConfig type, delete the constructor copy that
sets this.internalRootFolderKey from config (the assignment where
this.internalRootFolderKey = new Uint8Array(config.rootFolderKey)), and update
all code paths that read this.config.teeKeys (calls inside CipherBoxClient
methods such as where teeKeys are referenced) to obtain teeKeys via an internal
fetch/initialization path or an injectable test helper instead of reading from
config; finally, update the test harness to stop passing
rootIpnsName/rootFolderKey/teeKeys into new CipherBoxClient(...) and instead let
the client discover them or call registerFolder(...) only where appropriate.
---
Duplicate comments:
In `@tests/sdk-e2e/src/fixtures/test-harness.ts`:
- Around line 141-159: The deleteTestAccount helper silently drops transient
failures; change deleteTestAccount to retry transient errors (HTTP 429 and 5xx)
and network exceptions with an exponential/backoff loop: wrap the fetch call in
a retry loop (e.g., maxAttempts 3–5) that checks response.status (retry when
status === 429 or status >= 500) and catches network errors to retry, awaiting
an increasing delay between attempts; if a non-retryable error (2xx/4xx other
than 429) occurs log and return, and if attempts are exhausted log an error
including final status/text or exception—refer to deleteTestAccount,
ctx.accessToken, API_URL, and fetchHeaders to locate and modify the code.
---
Nitpick comments:
In `@apps/api/src/common/guards/throttler-bypass.guard.ts`:
- Around line 25-27: The guard currently accepts multi-value headers by taking
the first element (rawHeader -> header) which is ambiguous and unsafe; update
the check in the throttler bypass logic (the rawHeader/header/secret comparison
that uses safeEqual) to reject arrays outright by returning false or treating
the header as absent when Array.isArray(rawHeader) is true, and only proceed if
typeof rawHeader === 'string' (assign header = rawHeader as string) before
calling safeEqual(header, secret), ensuring a fail-closed behavior for
multi-value headers.
In `@tests/load/.env.example`:
- Line 8: Replace the concrete secret default for LOAD_TEST_SECRET with a
non-functional placeholder to avoid accidental reuse (e.g., use a CHANGE_ME_*
token); update the value for LOAD_TEST_SECRET in the tests/load/.env.example and
likewise replace the other occurrence noted (line 17) so both use an explicit
placeholder such as CHANGE_ME_LOAD_TEST_SECRET or <CHANGE_ME_LOAD_TEST_SECRET>
instead of an actual-looking secret.
In `@tests/load/src/scenarios/spike-test.test.ts`:
- Around line 66-77: The degradation calculation can divide by zero when
baseOp.latency.p95 is 0; in the loop that iterates baselineMetrics and finds
burstOp from burstMetrics (using baseOp and burstOp variables), add a defensive
check on baseOp.latency.p95 before computing degradation — if it is 0 or falsy,
skip the percentage calculation and log a safe placeholder like "N/A" (or treat
degradation as 0) instead of performing the division, then print the same
console message using that placeholder; ensure you reference baseOp.latency.p95
and burstOp.latency.p95 when performing the guarded computation.
In `@tests/sdk-e2e/.env.example`:
- Line 8: Replace the real-looking default secret value for SDK_E2E_SECRET with
a non-usable placeholder to avoid accidental reuse; edit the SDK_E2E_SECRET
entry (and the other occurrence mentioned) so the value is a
clearly-non-functional token such as REPLACE_ME_SDK_E2E_SECRET or
UNSET_SDK_E2E_SECRET and add a short comment if supported indicating it must be
set in CI/local env before running tests.
In `@tests/sdk-e2e/src/helpers/assertions.ts`:
- Around line 53-68: The functions getChild and getChildren currently return
any; import the FolderChild type from `@cipherbox/core` and update signatures so
getChild(client: CipherBoxClient, folderIpnsName: string, name: string):
FolderChild and getChildren(client: CipherBoxClient, folderIpnsName: string):
FolderChild[]; also ensure the local variables (the folder lookup and match
result in getChild) are typed/used as FolderChild to avoid any casting
surprises.
In `@tests/sdk-e2e/src/helpers/data-generators.ts`:
- Around line 10-15: The generateBytes function should validate the sizeBytes
argument to fail fast with a clear error: check that sizeBytes is a finite
integer >= 0 (and of type number) before constructing the Uint8Array and throw a
descriptive RangeError or TypeError if invalid; update the input guard in
generateBytes to validate and throw something like "generateBytes: sizeBytes
must be a non-negative integer" so tests get a clear diagnostic instead of an
internal Uint8Array error.
In `@tests/sdk-e2e/src/suites/bin-operations.test.ts`:
- Around line 50-100: Replace direct internal access to (ctx.client as
any).binState with assertions using the public API ctx.client.loadBin(): after
operations like deleteToBin, restoreFromBin, and permanentDelete call await
ctx.client.loadBin() and assert on the returned entries array (e.g., check
length, find by name, itemType) instead of reading binState; update tests that
currently reference binState (including the case that checks BinFolder and
bin-file.txt) to use loadBin() for both pre- and post-mutation verification so
tests rely only on public methods (also update any other occurrences that access
binState directly, e.g., the other test block that inspects BinFolder).
In `@tests/sdk-e2e/src/suites/concurrent-operations.test.ts`:
- Line 22: Rename the describe block string to match the updated file header by
changing the existing describe('Concurrent Operations', ...) to describe('Rapid
Sequential Operations', ...); update any matching test suite title usages in the
same file (the describe block and any snapshot/test reporting strings) so the
suite name and header comment are consistent.
In `@tests/sdk-e2e/src/suites/error-cases.test.ts`:
- Around line 151-153: The test currently uses a generic await
expect(tempCtx.client.createFolder(...)).rejects.toThrow(); change this to
assert a specific destroyed-client error substring or type so unrelated errors
don't pass: update the expectation to await
expect(tempCtx.client.createFolder(tempCtx.rootIpnsName,
'PostDestroy')).rejects.toThrowError(/destroy(ed)? client|client has been
destroyed/i) or match the exact error message/type your client throws after
calling destroy(), referencing tempCtx.client.createFolder and the client's
destroy() behavior so the assertion is tightly scoped.
In `@tests/sdk-e2e/src/suites/file-operations.test.ts`:
- Around line 32-151: Tests in this suite are tightly coupled (e.g., hello.txt
created in one test is renamed and deleted in later tests) causing cascade
failures; make each test self-contained by having each test create (via
ctx.client.uploadFile or ctx.client.createFolder + registerFolder) the specific
files/folders it needs using unique names (e.g., include Date.now() or a UUID),
use beforeEach to seed required items and afterEach to clean up via deleteItem,
and update references that currently rely on shared state (getChild,
expectChildNamed, renameItem, deleteItem) to operate on the locally created
item's id/ipnsName so tests can run independently and be rerun individually.
In `@tests/sdk-e2e/src/suites/folder-crud.test.ts`:
- Around line 17-29: The tests under describe('Folder CRUD') split a single
stateful workflow across multiple it blocks using the shared ctx from
createTestContext in beforeAll, which makes reruns brittle; either collapse the
dependent steps into one end-to-end test (e.g., a single it('full folder CRUD
flow') that performs create → rename → delete in sequence) or make each test
independent by moving setup/teardown into beforeEach/afterEach and
creating/cleaning its own context (use createTestContext and
ctx.cleanup/deleteTestAccount within per-test setup/teardown) so no test relies
on side effects from others.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8ecc225-c8da-4945-8a43-c862b9b86636
⛔ Files ignored due to path filters (2)
packages/api-client/src/instance.tsis excluded by!packages/api-client/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (71)
.github/workflows/load-test.yml.github/workflows/sdk-e2e.yml.planning/STATE.md.planning/todos/done/2026-03-22-investigate-throttle-bypass-gap-for-per-endpoint-throttle-decorators.md.planning/todos/done/2026-03-22-refactor-api-client-from-singleton-to-instance-scoped-axios.md.planning/todos/pending/2026-03-22-investigate-throttle-bypass-gap-for-per-endpoint-throttle-decorators.md.planning/todos/pending/2026-03-22-refactor-api-client-from-singleton-to-instance-scoped-axios.mdapps/api/src/app.module.tsapps/api/src/auth/auth.controller.spec.tsapps/api/src/auth/auth.controller.tsapps/api/src/auth/controllers/identity.controller.spec.tsapps/api/src/auth/controllers/identity.controller.tsapps/api/src/common/guards/throttler-bypass.guard.tsapps/api/src/device-approval/device-approval.controller.spec.tsapps/api/src/device-approval/device-approval.controller.tsapps/api/src/ipns/__tests__/ipns.integration.spec.tsapps/api/src/ipns/__tests__/ipns.security.spec.tsapps/api/src/ipns/ipns.controller.tsapps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/invites.controller.tsapps/api/src/shares/share-invites.controller.spec.tsapps/api/src/shares/share-invites.controller.tsapps/api/src/shares/shares.controller.spec.tsapps/api/src/shares/shares.controller.tsapps/web/src/lib/api-config.tsapps/web/src/lib/sdk-provider.tspackages/sdk-core/src/__tests__/ipns.test.tspackages/sdk-core/src/file/index.tspackages/sdk-core/src/folder/index.tspackages/sdk-core/src/ipfs/index.tspackages/sdk-core/src/ipns/index.tspackages/sdk-core/src/types.tspackages/sdk/src/__tests__/integration.test.tspackages/sdk/src/bin/index.tspackages/sdk/src/client.tspackages/sdk/src/types.tsrelease-please-config.jsontests/TESTING_STRATEGY.mdtests/load/.env.exampletests/load/package.jsontests/load/src/harness/client-pool.tstests/load/src/harness/metrics.tstests/load/src/harness/reporter.tstests/load/src/scenarios/ipns-publish-storm.test.tstests/load/src/scenarios/mixed-workload.test.tstests/load/src/scenarios/spike-test.test.tstests/load/src/scenarios/sustained-load.test.tstests/load/src/scenarios/upload-throughput.test.tstests/load/src/workloads/file-workload.tstests/load/src/workloads/folder-workload.tstests/load/src/workloads/mixed-workload.tstests/load/tsconfig.jsontests/load/vitest.config.tstests/sdk-e2e/.env.exampletests/sdk-e2e/package.jsontests/sdk-e2e/src/fixtures/multi-account.tstests/sdk-e2e/src/fixtures/test-harness.tstests/sdk-e2e/src/helpers/assertions.tstests/sdk-e2e/src/helpers/data-generators.tstests/sdk-e2e/src/suites/bin-operations.test.tstests/sdk-e2e/src/suites/concurrent-operations.test.tstests/sdk-e2e/src/suites/data-integrity.test.tstests/sdk-e2e/src/suites/error-cases.test.tstests/sdk-e2e/src/suites/file-operations.test.tstests/sdk-e2e/src/suites/folder-crud.test.tstests/sdk-e2e/src/suites/invite-link.test.tstests/sdk-e2e/src/suites/ipns-consistency.test.tstests/sdk-e2e/src/suites/share-operations.test.tstests/sdk-e2e/src/suites/vault-lifecycle.test.tstests/sdk-e2e/tsconfig.jsontests/sdk-e2e/vitest.config.ts
💤 Files with no reviewable changes (1)
- packages/sdk/src/tests/integration.test.ts
- app.module.ts: note bypass is blocked in production - client-pool.ts: update docblock to reflect instance-scoped architecture - file-workload.ts: validate minSize/maxSize bounds, add byte-level content verification for downloads - mixed-workload.ts: validate weight values are non-negative finite numbers - test-harness.ts: wrap setup in try/catch to clean up partially provisioned accounts on failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4548f0405d9e
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/load/src/harness/client-pool.ts`:
- Around line 124-130: The catch currently swallows writeFile errors; change it
to catch the thrown error and log a non-fatal diagnostic (including the
scenarioName/slug and the error message/stack) instead of doing nothing so CI
shows why the artifact was not produced—modify the try around the
import('node:fs/promises') and await fs.writeFile(...) to use catch(error) and
emit a clear error via your logging facility (or console.error) while keeping
the failure non-fatal.
- Around line 83-88: The teardown in destroyClientPool currently only calls
pc.client.destroy() and deleteTestAccount, leaving PoolClient fields with secret
material (privateKey, rootFolderKey, rootIpnsKeypair.privateKey) alive; update
destroyClientPool to explicitly wipe/zero-out or nullify these fields on each pc
(e.g., set pc.privateKey = undefined/null, pc.rootFolderKey = undefined/null,
and pc.rootIpnsKeypair.privateKey = undefined/null) after destroying the SDK
client and before/after calling deleteTestAccount so no raw key material remains
in memory after cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: efef595e-10e5-4534-a3cb-392e620466c6
📒 Files selected for processing (5)
apps/api/src/app.module.tstests/load/src/harness/client-pool.tstests/load/src/workloads/file-workload.tstests/load/src/workloads/mixed-workload.tstests/sdk-e2e/src/fixtures/test-harness.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/app.module.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/load/src/workloads/file-workload.ts
- tests/sdk-e2e/src/fixtures/test-harness.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: bd43ec70520f
7 test cases covering all branches: - Bypass with valid header and non-production env - Fall-through on wrong secret, missing header, production env, unset secret, and length-mismatch (timing-safe rejection) - Array header handling (first element) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e3fadb8ee0cf
The Playwright-based browser tests are now at tests/ui-e2e/ to distinguish them from the SDK E2E tests (tests/sdk-e2e/) and load tests (tests/load/). Updated everywhere: - Package name: @cipherbox/e2e → @cipherbox/ui-e2e - Root scripts: test:e2e → test:ui-e2e - GitHub workflows: e2e.yml, release-gate.yml, tag-staging.yml - release-please-config.json - Documentation, .learnings, .claude commands - Internal comments and README Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FSM1
force-pushed
the
feat/sdk-e2e-and-load-tests
branch
from
841ff73 to
bbc9725
Compare
The job name is referenced by branch protection — renaming it causes the required check to show as "expected — waiting for status" on all PRs. Reverted to "Verify E2E Passed". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4d5de64d2942
The SDK E2E job now runs inside ci.yml after unit tests pass, instead of being a separate workflow. This gives a single CI check on PRs and ensures SDK E2E only runs when unit tests succeed (fail-fast). Job dependency chain: changes → lint → typecheck → test → sdk-e2e → build The standalone sdk-e2e.yml is removed. Branch protection does not reference it, so no rule updates needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8842514ec58b
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/src/common/guards/throttler-bypass.guard.spec.ts (2)
24-28: Consider using NestJS testing utilities for more robust instantiation.
Object.create(BypassableThrottlerGuard.prototype)bypasses the constructor, which works here sincecanActivatedoesn't rely on instance state. However, ifThrottlerGuardever initializes required instance properties in its constructor, these tests will fail unexpectedly.A more robust (but heavier) approach would use
Test.createTestingModulewith mocked throttler providers. Given the current implementation's simplicity, this is a pragmatic trade-off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/common/guards/throttler-bypass.guard.spec.ts` around lines 24 - 28, The test uses Object.create(BypassableThrottlerGuard.prototype) in beforeEach which bypasses the constructor and could break if ThrottlerGuard later adds instance state; replace this manual instantiation with a proper NestJS test module: use Test.createTestingModule to provide and compile BypassableThrottlerGuard (mocking any ThrottlerGuard/ThrottlerService dependencies), retrieve the guard via module.get(BypassableThrottlerGuard) in beforeEach, keep clearing superCanActivate and restoring process.env as before, and ensure tests still call canActivate on the retrieved instance so constructor initialization (if added later) runs.
45-53: Consider asserting return value propagation.These tests verify
superCanActivateis called but don't assert that the return value from the parent is properly returned. Since the mock returnstrue, addingexpect(result).toBe(true)would ensure thereturn super.canActivate(context)path correctly propagates the parent's result.Example for one test case
it('should fall through to parent when secret does not match', async () => { process.env.THROTTLE_BYPASS_SECRET = 'test-secret'; process.env.NODE_ENV = 'test'; const ctx = createMockContext({ 'x-throttle-bypass': 'wrong-secret' }); - await guard.canActivate(ctx); + const result = await guard.canActivate(ctx); + expect(result).toBe(true); expect(superCanActivate).toHaveBeenCalledWith(ctx); });Also applies to: 55-63, 75-83, 96-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/common/guards/throttler-bypass.guard.spec.ts` around lines 45 - 53, The tests for ThrottlerBypassGuard currently assert that superCanActivate (the mocked parent canActivate) was called but don't verify its return value is propagated; update the test cases that call guard.canActivate(ctx) (the ones using createMockContext and asserting superCanActivate) to capture the result (e.g., const result = await guard.canActivate(ctx)) and add an assertion that result is true to confirm return value propagation from the parent canActivate; apply this change to the test blocks referencing superCanActivate at the indicated locations (including the other similar tests around lines 55-63, 75-83, 96-104).
🤖 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/common/guards/throttler-bypass.guard.spec.ts`:
- Around line 24-28: The test uses
Object.create(BypassableThrottlerGuard.prototype) in beforeEach which bypasses
the constructor and could break if ThrottlerGuard later adds instance state;
replace this manual instantiation with a proper NestJS test module: use
Test.createTestingModule to provide and compile BypassableThrottlerGuard
(mocking any ThrottlerGuard/ThrottlerService dependencies), retrieve the guard
via module.get(BypassableThrottlerGuard) in beforeEach, keep clearing
superCanActivate and restoring process.env as before, and ensure tests still
call canActivate on the retrieved instance so constructor initialization (if
added later) runs.
- Around line 45-53: The tests for ThrottlerBypassGuard currently assert that
superCanActivate (the mocked parent canActivate) was called but don't verify its
return value is propagated; update the test cases that call
guard.canActivate(ctx) (the ones using createMockContext and asserting
superCanActivate) to capture the result (e.g., const result = await
guard.canActivate(ctx)) and add an assertion that result is true to confirm
return value propagation from the parent canActivate; apply this change to the
test blocks referencing superCanActivate at the indicated locations (including
the other similar tests around lines 55-63, 75-83, 96-104).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20140c6d-5411-4854-bf73-e649d60ef2f5
⛔ Files ignored due to path filters (3)
.claude/commands/resolve-pr-reviews.mdis excluded by!.claude/commands/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yamltests/web-e2e/fixtures/files/test-image.pngis excluded by!**/*.png
📒 Files selected for processing (86)
.claude/gsd-local-patches/get-shit-done/workflows/verify-work.md.github/workflows/ci.yml.github/workflows/desktop-e2e.yml.github/workflows/e2e.yml.github/workflows/release-gate.yml.github/workflows/tag-staging.yml.learnings/2026-02-07-empty-state-upload-regression.md.learnings/2026-02-08-desktop-testing-strategy.md.learnings/2026-02-24-run-e2e-locally-before-push.md.learnings/2026-02-25-ipns-stale-resolution-staging.md.learnings/2026-03-20-playwright-mcp-login-and-upload.md.planning/codebase/STACK.md.planning/codebase/TESTING.mdREADME.mdapps/api/src/app.module.tsapps/api/src/common/guards/throttler-bypass.guard.spec.tsdocs/DEVELOPMENT.mdpackage.jsonrelease-please-config.jsonscripts/baseline-benchmark.shtests/TESTING_STRATEGY.mdtests/desktop-e2e/fixtures/crypto/aes-ctr-vectors.jsontests/desktop-e2e/fixtures/crypto/aes-gcm-vectors.jsontests/desktop-e2e/fixtures/crypto/metadata-vectors.jsontests/desktop-e2e/scripts/run-all.ps1tests/desktop-e2e/scripts/run-all.shtests/desktop-e2e/scripts/test-conflict-detection.ps1tests/desktop-e2e/scripts/test-conflict-detection.shtests/desktop-e2e/scripts/test-fuse-operations.ps1tests/desktop-e2e/scripts/test-fuse-operations.shtests/desktop-e2e/scripts/test-recycle-bin.ps1tests/desktop-e2e/scripts/test-recycle-bin.shtests/desktop-e2e/scripts/test-round-trip.ps1tests/desktop-e2e/scripts/test-round-trip.shtests/desktop-e2e/scripts/wait-for-mount.ps1tests/desktop-e2e/scripts/wait-for-mount.shtests/load/src/harness/client-pool.tstests/load/src/workloads/file-workload.tstests/load/src/workloads/mixed-workload.tstests/sdk-e2e/src/fixtures/test-harness.tstests/web-e2e/.env.exampletests/web-e2e/.gitignoretests/web-e2e/README.mdtests/web-e2e/fixtures/files/.gitkeeptests/web-e2e/package.jsontests/web-e2e/page-objects/base.page.tstests/web-e2e/page-objects/dialogs/confirm-dialog.page.tstests/web-e2e/page-objects/dialogs/create-folder-dialog.page.tstests/web-e2e/page-objects/dialogs/details-dialog.page.tstests/web-e2e/page-objects/dialogs/index.tstests/web-e2e/page-objects/dialogs/invite-link-tab.page.tstests/web-e2e/page-objects/dialogs/move-dialog.page.tstests/web-e2e/page-objects/dialogs/rename-dialog.page.tstests/web-e2e/page-objects/dialogs/search-palette.page.tstests/web-e2e/page-objects/dialogs/share-dialog.page.tstests/web-e2e/page-objects/dialogs/text-editor-dialog.page.tstests/web-e2e/page-objects/file-browser/breadcrumbs.page.tstests/web-e2e/page-objects/file-browser/context-menu.page.tstests/web-e2e/page-objects/file-browser/file-list.page.tstests/web-e2e/page-objects/file-browser/index.tstests/web-e2e/page-objects/file-browser/parent-dir.page.tstests/web-e2e/page-objects/file-browser/selection-action-bar.page.tstests/web-e2e/page-objects/file-browser/shared-file-browser.page.tstests/web-e2e/page-objects/file-browser/upload-zone.page.tstests/web-e2e/page-objects/index.tstests/web-e2e/page-objects/login.page.tstests/web-e2e/page-objects/pages/bin.page.tstests/web-e2e/page-objects/pages/invite.page.tstests/web-e2e/playwright.config.tstests/web-e2e/playwright.load.config.tstests/web-e2e/tests/conflict-detection.spec.tstests/web-e2e/tests/full-workflow.spec.tstests/web-e2e/tests/invite-link-workflow.spec.tstests/web-e2e/tests/load-test.spec.tstests/web-e2e/tests/mfa-flows.spec.tstests/web-e2e/tests/recycle-bin.spec.tstests/web-e2e/tests/search-workflow.spec.tstests/web-e2e/tests/sharing-workflow.spec.tstests/web-e2e/tests/wallet-login.spec.tstests/web-e2e/tsconfig.jsontests/web-e2e/utils/api-helpers.tstests/web-e2e/utils/conflict-helpers.tstests/web-e2e/utils/mfa-helpers.tstests/web-e2e/utils/multi-account-wallet.tstests/web-e2e/utils/test-files.tstests/web-e2e/utils/wallet-login-helpers.ts
✅ Files skipped from review due to trivial changes (21)
- docs/DEVELOPMENT.md
- README.md
- .learnings/2026-02-07-empty-state-upload-regression.md
- .planning/codebase/STACK.md
- .claude/gsd-local-patches/get-shit-done/workflows/verify-work.md
- tests/web-e2e/playwright.load.config.ts
- tests/web-e2e/tests/load-test.spec.ts
- .learnings/2026-03-20-playwright-mcp-login-and-upload.md
- .planning/codebase/TESTING.md
- tests/web-e2e/playwright.config.ts
- tests/web-e2e/README.md
- scripts/baseline-benchmark.sh
- release-please-config.json
- .learnings/2026-02-08-desktop-testing-strategy.md
- tests/web-e2e/utils/test-files.ts
- .learnings/2026-02-24-run-e2e-locally-before-push.md
- tests/web-e2e/package.json
- .github/workflows/tag-staging.yml
- .learnings/2026-02-25-ipns-stale-resolution-staging.md
- apps/api/src/app.module.ts
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/load/src/workloads/file-workload.ts
- tests/sdk-e2e/src/fixtures/test-harness.ts
This was referenced Mar 23, 2026
5 tasks
Merged
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
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
@cipherbox/sdk-e2epackage with 10 test suites that drive CipherBoxClient directly against the API (no browser, no Playwright, no UI selectors)@cipherbox/load-testspackage with 5 configurable load test scenarios using SDK client poolssdk-e2e.ymlCI workflow (PR-triggered, path-filtered) andload-test.yml(manual dispatch)SDK E2E Tests (
tests/sdk-e2e/)Extracts the proven pattern from
packages/sdk/src/__tests__/integration.test.tsinto a reusable test harness, then builds 10 comprehensive test suites:Load Tests (
tests/load/)Node.js-native load testing (not k6 — SDK needs full Node.js crypto):
CI Workflows
sdk-e2e.yml: Triggers on PR/push topackages/**,apps/api/**,tests/sdk-e2e/**. ~50% faster than Playwright E2E (no browser download, no web build, no UI rendering)load-test.yml: Manual dispatch with inputs for environment (local/staging), client count, and scenarioTest plan
pnpm --filter @cipherbox/sdk-e2e testwith local API + IPFS runningpnpm --filter @cipherbox/load-tests test -- --testPathPattern=mixed-workloadagainst localsdk-e2e.ymltriggers on this PR and passesload-test.ymlafter merge🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor
Chores