feat(cas): M3 Launchpad — native Bun/Deno adapters, async crypto, and OIDC/JSR release workflow #3
Conversation
… OIDC/JSR release workflow
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRuntime-conditional crypto adapters and async crypto operations were added; CasService lazy-initialization implemented; Bun and WebCrypto adapters introduced; CI and release workflows switched to pnpm and a new release pipeline; package metadata and version bumped to 1.3.0; tests and CLI adjusted for async flows. Changes
Sequence DiagramsequenceDiagram
actor Client
participant CAS as ContentAddressableStore
participant Detector as getDefaultCryptoAdapter
participant Service as CasService
participant Adapter as CryptoAdapter
Client->>CAS: encrypt(data, key)
activate CAS
CAS->>CAS: `#getService`()
CAS->>Detector: detect runtime
activate Detector
Detector-->>CAS: return Adapter (Bun/Web/Node)
deactivate Detector
CAS->>Service: initialize with Adapter
activate Service
Service-->>CAS: ready
CAS->>Service: encrypt(buffer, key)
activate Service
Service->>Adapter: encryptBuffer(buffer, key)
activate Adapter
Adapter->>Adapter: validate key, gen nonce, AES-GCM
Adapter-->>Service: { buf, meta }
deactivate Adapter
Service-->>CAS: encrypted result
deactivate Service
CAS-->>Client: encrypted data
deactivate CAS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 77-81: Update the "jsr" dependency entry in package.json from
"^0.13.3" to "^0.14.2" (the "jsr" key in the dependencies/devDependencies block)
and then run your package manager to refresh the lockfile (npm install / yarn
install / pnpm install) so the lockfile reflects the new baseline version.
In `@src/infrastructure/adapters/WebCryptoAdapter.js`:
- Around line 76-105: The encrypt async generator and finalize function
currently share mutable state via this._finalTag, causing a race when multiple
createEncryptionStream/store() calls run concurrently; change to use a
closure-scoped variable (e.g., let finalTag = null) declared in the
createEncryptionStream scope, assign finalTag = fullBuffer.slice(-tagLength)
inside encrypt instead of setting this._finalTag, and have finalize() return
this.#buildMeta(nonce, finalTag); also remove or avoid relying on .bind(this)
for encrypt if it was only used to access _finalTag so the functions close over
the local variable instead of the instance property.
🧹 Nitpick comments (7)
.github/workflows/release.yml (2)
29-34: Docker integration tests missing--buildflag.The
package.jsonscripts (test:integration:node, etc.) usedocker compose run --build --rm, but this workflow omits--build. On a fresh CI runner this may work if images are built from scratch, but if the runner has a layer cache from a previous run, stale images could be used, leading to flaky test results against old code.Proposed fix
# Integration tests in Docker - name: Integration tests (Node) - run: docker compose run --rm test-node npx vitest run test/integration + run: docker compose run --build --rm test-node npx vitest run test/integration - name: Integration tests (Bun) - run: docker compose run --rm test-bun bunx vitest run test/integration + run: docker compose run --build --rm test-bun bunx vitest run test/integration - name: Integration tests (Deno) - run: docker compose run --rm test-deno deno run -A npm:vitest run test/integration + run: docker compose run --build --rm test-deno deno run -A npm:vitest run test/integration
36-44: No version consistency check between the Git tag andpackage.json.A tag like
v1.4.0can be pushed whilepackage.jsonstill reads1.3.0, causing npm/JSR to publish a version that doesn't match the Git tag. Consider adding a step before publishing to assert the tag matches the package version, e.g.:- name: Verify tag matches package version run: | TAG_VERSION="${GITHUB_REF_NAME#v}" PKG_VERSION=$(node -p "require('./package.json').version") if [ "$TAG_VERSION" != "$PKG_VERSION" ]; then echo "::error::Tag $GITHUB_REF_NAME does not match package.json version $PKG_VERSION" exit 1 fitest/unit/domain/services/CasService.crypto.test.js (1)
81-94: Double decrypt call in integrity tests — minor inefficiency.Each integrity-failure test calls
service.decrypt(...)twice: once forrejects.toThrow(CasError)and again insidetry/catchto asserterr.code. This doubles the crypto work and could mask issues if the two calls behave differently.A single-call pattern is cleaner:
Example consolidation
- await expect(service.decrypt({ buffer: buf, key: keyB, meta })).rejects.toThrow(CasError); - try { - await service.decrypt({ buffer: buf, key: keyB, meta }); - } catch (err) { - expect(err.code).toBe('INTEGRITY_ERROR'); - } + await expect(service.decrypt({ buffer: buf, key: keyB, meta })).rejects.toThrow( + expect.objectContaining({ code: 'INTEGRITY_ERROR' }), + );This same pattern recurs in sections 2b, 2c, and the fuzz tamper tests — a single fix can be applied throughout.
src/infrastructure/adapters/BunCryptoAdapter.js (2)
13-14:sha256is synchronous while the port contract andCasService._sha256nowawaitthe result.This works because
await-ing a non-Promise value simply resolves it. However, it means the Bun adapter'ssha256method signature diverges from the asyncWebCryptoAdapter.sha256. Consider addingasyncfor consistency with the port contract, especially if the port's JSDoc is ever updated to returnPromise<string>.
33-39:decryptBufferskips#validateKey— inconsistent withencryptBuffer.
encryptBuffer(line 22) andcreateEncryptionStream(line 42) both call#validateKey(key), butdecryptBufferdoes not. WhileCasServicevalidates upstream inrestore(), the adapter-level inconsistency could bite ifdecryptBufferis ever called directly.Proposed fix
decryptBuffer(buffer, key, meta) { + this.#validateKey(key); const nonce = Buffer.from(meta.nonce, 'base64');src/infrastructure/adapters/WebCryptoAdapter.js (1)
117-125:#validateKeyonly checks length, missing type check.Unlike
BunCryptoAdapter.#validateKeyandCasService._validateKey, this doesn't reject non-Buffer/Uint8Arrayinputs (e.g., a 32-char string). The upstreamCasServiceguards this, but for adapter-level consistency and defense-in-depth:Proposed fix
`#validateKey`(key) { + if (!ArrayBuffer.isView(key) && !(key instanceof ArrayBuffer)) { + throw new CasError( + 'Encryption key must be a Buffer or Uint8Array', + 'INVALID_KEY_TYPE', + ); + } if (key.length !== 32) {index.js (1)
67-82: Lazy init race: concurrent callers can create duplicateCasServiceinstances.If two methods (e.g.,
storeandverifyIntegrity) are called concurrently before the service is initialized, both will pass the!this.servicecheck and each willawait getDefaultCryptoAdapter()independently, creating two separateCasServiceinstances. The second overwrites the first.Currently benign since
CasServiceis stateless, but caching the initialization promise is a small, standard safeguard:Proposed fix
+ `#servicePromise` = null; + async `#getService`() { - if (!this.service) { + if (!this.#servicePromise) { + this.#servicePromise = this.#initService(); + } + return this.#servicePromise; + } + + async `#initService`() { + if (!this.service) { const persistence = new GitPersistenceAdapter({ plumbing: this.plumbing, policy: this.policyConfig }); const crypto = this.cryptoConfig || await getDefaultCryptoAdapter(); this.service = new CasService({ persistence, chunkSize: this.chunkSizeConfig, codec: this.codecConfig || new JsonCodec(), crypto, }); - } + } return this.service; }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 39-47: The release workflow's two steps ("Publish to npm" running
`pnpm publish --provenance --no-git-checks` and "Publish to JSR" running `pnpm
dlx jsr publish`) are not idempotent: if the npm step succeeds but JSR fails,
re-running the job will fail on npm due to an existing version; either make the
npm step tolerant (add continue-on-error or a pre-check for existing package
version before running `pnpm publish`) or split the steps into independent jobs
(e.g., a job that only runs `pnpm publish` and a separate job for `pnpm dlx jsr
publish`) so each can be retried independently. Ensure the chosen fix preserves
provenance/auth env (NODE_AUTH_TOKEN) and that any version-existence check
reliably detects a previously published version before attempting `pnpm
publish`.
In `@src/infrastructure/adapters/BunCryptoAdapter.js`:
- Around line 17-19: The randomBytes method currently returns a Uint8Array from
globalThis.crypto.getRandomValues but the CryptoPort contract expects a Node
Buffer; change randomBytes to convert the Uint8Array into a Buffer before
returning (e.g., call Buffer.from(...) on the result of getRandomValues) so
callers of randomBytes (function randomBytes) receive a Buffer-compatible object
while still using globalThis.crypto.getRandomValues for entropy.
- Around line 33-39: The decryptBuffer method is missing the key validation
call; add a call to the class's private validation helper (`#validateKey`(key)) at
the start of decryptBuffer (before creating the decipher or using key) so
invalid keys produce the same CasError (INVALID_KEY_TYPE / INVALID_KEY_LENGTH)
as encryptBuffer and createEncryptionStream, then proceed to createDecipheriv,
setAuthTag, and return the decrypted Buffer as before.
🧹 Nitpick comments (5)
scripts/install-hooks.sh (2)
15-15: Use the already-computed absolute path forcore.hooksPath.Line 9 computes
HOOKS_DIRas an absolute path, but line 15 hardcodes a relative"scripts/git-hooks". Relativecore.hooksPathvalues are resolved from the working tree root, which works for normal git operations, but using the absolute path is more robust and avoids the inconsistency.Proposed fix
-git config core.hooksPath "scripts/git-hooks" +git config core.hooksPath "${HOOKS_DIR}"
11-12: Consider making all hooks in the directory executable.Currently only
pre-pushis explicitlychmod +x'd. If additional hooks are introduced later, this line would need updating each time.Future-proof suggestion
-# Make hooks executable -chmod +x "${HOOKS_DIR}/pre-push" +# Make all hooks executable +chmod +x "${HOOKS_DIR}"/*src/infrastructure/adapters/BunCryptoAdapter.js (1)
41-65:finalize()will throw if called before theencryptgenerator is fully consumed.
cipher.getAuthTag()(Line 60) is only valid aftercipher.final()(Line 53) has been called inside the generator. If a caller invokesfinalize()before fully draining the async generator, it will get an opaque Node error. Consider adding a guard to produce a clear error message.Proposed defensive guard
createEncryptionStream(key) { this.#validateKey(key); const nonce = this.randomBytes(12); const cipher = createCipheriv('aes-256-gcm', key, nonce); + let finalized = false; const encrypt = async function* (source) { for await (const chunk of source) { const encrypted = cipher.update(chunk); if (encrypted.length > 0) { yield encrypted; } } const final = cipher.final(); if (final.length > 0) { yield final; } + finalized = true; }; - const finalize = () => { + const finalize = () => { + if (!finalized) { + throw new CasError( + 'Cannot finalize before the encrypt stream is fully consumed', + 'STREAM_NOT_CONSUMED', + ); + } const tag = cipher.getAuthTag(); return this.#buildMeta(nonce, tag); };Dockerfile (1)
4-7: Considercorepack enableinstead of global pnpm install.Node 22 ships with Corepack, which can manage pnpm without a separate global install. This is a lighter approach and keeps the pnpm version in sync with
package.json's"packageManager"field (if set).♻️ Optional alternative
-RUN npm install -g pnpm@10 +RUN corepack enable && corepack prepare pnpm@10 --activate.github/workflows/release.yml (1)
8-13: Consider adding aconcurrencygroup to prevent parallel release jobs.If two version tags are pushed in quick succession, two release jobs could run simultaneously, potentially causing race conditions during publishing.
♻️ Suggested addition (after line 7)
+concurrency: + group: release + cancel-in-progress: false + jobs: release:
1 participant
Implements Milestone 3: Launchpad. Adds native support for Bun and Deno, makes crypto operations async, and sets up secure automated publishing to NPM (OIDC) and JSR.
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Improvements
Tests