chore(protect-ffi): add integration suite to pnpm workspace - #948
Open
tobyhede wants to merge 1 commit into
Open
chore(protect-ffi): add integration suite to pnpm workspace#948tobyhede wants to merge 1 commit into
tobyhede wants to merge 1 commit into
Conversation
`packages/protect-ffi/integration-tests` sat outside the pnpm workspace with its own `package-lock.json`, installed by `npm ci`. That made it the last place in the tree where `@cipherstash/eql` was pinned to a published version — and it is the suite that owns the repo's only EQL v2 *and* v3 SQL coverage. So its `postgres-v3.test.ts` queried SQL installed from the npm bundle while the payloads under test were emitted by the in-tree `eql-bindings`: the two halves of EQL could disagree, and would have disagreed in a database rather than in CI. The directory is now a workspace member, `@cipherstash/eql` resolves `workspace:^`, and the registry-pin guard's exemption list is empty. Moving the pins was not neutral, which is why this was always meant to be its own change. `@cipherstash/auth` `^0.39.0 -> 0.42.0` is a breaking API move: `AccessKeyStrategy.create`, `OidcFederationStrategy.create` and `getToken` all return `@byteslice/result` Results now, and `OidcFederationStrategy.create` takes a whole CRN instead of `(region, workspaceId, getJwt)`. Four call sites needed unwrapping, and nothing would have caught them — the suite's `tsc` rode on its own `npm test` while CI invoked vitest directly, so it compiled nowhere. It has a `typecheck` script run from `tests.yml` now, with `checkJs` on so the two `.cjs` fixtures (one of them a real strategy call site) are covered too. Workspace membership has a cost worth naming: root `pnpm test` is `turbo test --filter './packages/**'`, which now reaches this package. The suite is kept out by naming no live script after a turbo task — its runners are `vitest:live` and `vitest:live:coverage`. `test` is the obvious trap and `test:integration` the less obvious one, being a real turbo task that four integration workflows invoke. `integrationSuiteCi.test.ts` derives the forbidden set from `turbo.json` rather than listing it, and also asserts `tasks.toml` still installs with pnpm, reading past comments so a sentence about `npm ci` cannot stand in for the command. No changeset: nothing in a published package's behaviour or surface changes. Verified with a credentialed local run against Docker and live ZeroKMS -- `mise run test:integration:all`, 20 files / 232 tests, zero skips, EQL v2 and v3 both installed. Also `pnpm test` (16/16 tasks, suite correctly not among them), `pnpm test:scripts` (818, stable over three runs), the e2e supply-chain suite, all four lint scripts, the release gate, and `pnpm install --frozen-lockfile` clean with one shared `@cipherstash/auth` instance across stack, cli, wizard and the suite. Refs #885 Claude-Session: https://claude.ai/code/session_01FGhktoHnwEuibNQF1MYexw
|
tobyhede
marked this pull request as ready for review
August 26, 2026 05:57
tobyhede
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CipherStash Stack encrypts data in the database. Two pieces have to agree for that to work: the Rust that produces an encrypted value, and the SQL (called EQL — Encrypt Query Language) that stores and searches it. They ship as a matched pair, at one version, because a mismatch is not a compile error — it is a query that returns wrong results, or nothing, at runtime in a customer's database.
Both pieces now live in this repository, so they can be built from the same commit and cannot disagree. One directory was still opting out:
packages/protect-ffi/integration-tests, the test suite that runs real encryption against a real Postgres. It sat outside the repo's package manager with a lockfile of its own, downloading a published copy of the EQL SQL from the npm registry while testing Rust built from this repo. That directory is also the only place the EQL SQL is exercised at all.So the one suite whose job is to catch that mismatch was the one place it could still happen. This PR brings the directory into the workspace, and the mismatch is now unrepresentable.
It was not a free move, which is why it was always planned as its own PR. Joining the workspace also means taking the repository's shared dependency versions, and one of those — the authentication library — had made breaking API changes across the versions being skipped. Those broke the suite, and are fixed here.
Changes
Joining the workspace
packages/protect-ffi/integration-testsis listed inpnpm-workspace.yaml; itspackage-lock.jsonis deleted and thenpm cistep is gone.@cipherstash/auth,vitest,typescript) come from the repo's catalog.Fixing what the version bump broke
@cipherstash/auth^0.39.0 → 0.42.0changed three functions to return a success/failure wrapper object instead of the value directly, and changed one function's arguments. Four call sites updated across three test files and a test fixture.Making the same class of break visible next time
typecheckscript, run on every pull request. Its type-check previously ran nowhere in CI, which is why a breaking API change could land silently..cjsfixtures. One is a real call site of the changed API, and the default file pattern would have skipped it.Keeping the ordinary test command usable
pnpm test, which every contributor runs. If one of its scripts were named after a task the build tool knows,pnpm testwould start demanding Docker, a database and live credentials for everyone. Its runners are namedvitest:live/vitest:live:coverageinstead, and a test derives the forbidden names fromturbo.jsonrather than listing them.Guards and docs
vitestand its coverage plugin stay on the same version — they refuse to run against each other otherwise.AGENTS.md, the package README, and both absorption planning documents updated.No changeset: no published package changes behaviour or public API here.
Verification
Run against Docker and live CipherStash credentials, with both EQL versions installed:
mise run test:integration:all— 20 files, 232 tests, 0 skipped. Includes the identity-bound encryption tests, both EQL v1-era and current SQL suites, and the WebAssembly round trip. Both shapes of the changed auth API are covered by a live test each.pnpm test— 16/16 tasks. The live suite is correctly not among them.pnpm test:scripts— 818 passing, stable across five runs.pnpm install --frozen-lockfileclean, with a single shared@cipherstash/authinstance acrossstack,cli,wizardand this suite — the arrangement whose absence caused the "Failed to load native binding" failures in 1.0.0-rc.2.Honest gaps:
npm ciappears nowhere in its log. It has not run on the default branch yet; that only happens after merge.pnpm test:scriptsfailed once (817/818) mid-work and passed on every run since. That run overlapped a concurrent build rewriting a file one of the tests reads, which I believe was the cause — but I did not capture the test name before it went green, so I cannot state that as fact.pnpm run code:checkcannot run repo-wide on my machine: stale agent worktrees under.claude/carry their own Biome config and abort it. Biome was run scoped to the changed paths instead — clean, warnings only.Related
Refs #885 — the EQL absorption this completes. That PR moved the SQL in-repo; this removes the last place a downloaded copy was still used.
Review notes
Start with
packages/protect-ffi/integration-tests/package.jsonandpnpm-workspace.yaml— everything else follows from those two.The auth API fixes are the part that could be wrong in a way tests would not catch, since they are test code themselves.
tests/js-strategy.test.tsis worth a close look: the API accepts two return shapes and the suite deliberately covers one per test. Before this change that split was accidental — the old library version returned the plain shape and the test forwarded it. Preserving the split took an explicit unwrap, and without it both tests would have exercised the same shape while the other, which is what hand-written integrations return, went untested.Deliberately kept:
@vitest/coverage-v8brings roughly fifteen transitive dev dependencies into the shared lockfile. It was already installed in the old isolated lockfile, where nothing monitored it — the absorption plan lists that as a problem this change resolves. Bringing it under the repo's dependency monitoring is the point, not a side effect.Not done, and I would rather it were raised than assumed: the
.cjsfixtures needed one type annotation each to pass the new type-check. That is a real constraint of checking plain JavaScript, not a workaround, and there is a comment saying so.https://claude.ai/code/session_01FGhktoHnwEuibNQF1MYexw