feat(workspace): link protect-ffi and build it in the jobs that need it - #863
Open
tobyhede wants to merge 4 commits into
Open
feat(workspace): link protect-ffi and build it in the jobs that need it#863tobyhede wants to merge 4 commits into
tobyhede wants to merge 4 commits into
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
`@cipherstash/stack` and the two adapters move from the exact pin `0.31.0` to `workspace:*`, so they resolve the in-tree copy rather than the npm tarball. This is the step the previous three set up, and it is deliberately atomic. **Why it cannot be split further.** The tarball ships `lib/`, `index.node` and `dist/wasm/**` as CONTENTS. The workspace copy ships them as BUILD OUTPUTS — `lib/` is the package `main` and is generated, and nothing produces `index.node` without cargo. So the moment this line changes, every CI job that encrypts, decrypts, or typechecks against the package needs the binding built first, and the workflow steps below have to land in the same commit. Split them and the failure is `Cannot find module '.../index.node'`, reported once per test rather than once per job. `minimumReleaseAgeExclude` drops its `@cipherstash/protect-ffi*` entry in the same commit and for the same reason: the cooldown applies to registry dependencies, and as of this line these are not registry dependencies. Leaving it would be a stale exemption for packages nothing fetches. The published surface does not change — the workspace copy is the 0.31.0 release, tree-identical to the tag. The `major` changeset for adopting 0.31.0 is in the first PR of this stack, where the breaking change actually is.
The other half of the workspace link. `.github/actions/build-ffi-binding` compiles the binding and proves it loads, and every job that reaches encryption now runs it before its tests: `tests.yml`'s unit, e2e and bun jobs, the three integration suites, and both Prisma e2e workflows. The action caches `index.node` on a content hash of the Rust inputs, so a PR touching no Rust pays a restore rather than a compile. `wasm: 'true'` is passed by exactly two jobs — the ones that load the real WASM. stack's `dist/wasm-inline.js` imports protect-ffi's `dist/wasm/protect_ffi_inline.js` by relative path, and only the three `.d.ts` under `dist/wasm` are tracked; the `.js` and `.wasm` come from wasm-pack and nothing else in CI runs it. **Ordering is load-bearing in two places, and neither looks it.** `require-cs-secrets` must precede `build-ffi-binding`. The secrets check reads four inputs and fails in seconds; a cold binding build costs minutes, and more with `wasm: 'true'`. Put the build first and a job with a rotated credential pays the whole compile before learning it could never have encrypted anything — which is the same as having no pre-flight. Both steps are self-contained `uses:` blocks and swapping them changes nothing about whether the job passes, so `ffi-binding-step-order.test.mjs` checks it by position across every workflow, discovered rather than listed. Note the direction of the fix when it fails: in `wasm-e2e-tests` a `Build stack` step sits between them and consumes `dist/wasm`, so there the pre-flight moves UP, not the build DOWN. In `wasm-e2e-tests`, `Build stack` must follow the binding build, because it now consumes protect-ffi's `dist/wasm` output from the workspace copy rather than from a tarball. `test:typecheck:wasm` returns here, with the job that runs it. It needs `dist/wasm`, so it cannot hang off the default `test`, and `lintWiring`'s exemption list only accepts a carve-out whose name appears in a root workflow — which is why the script was held back until this commit rather than shipped with the package and a promise. The no-caching gate gains a live citation rather than a fixture: `build-ffi-binding` restores two GitHub Actions caches, and its header says publishing workflows must not use it. The test asserts the caching is really there before asserting the gate flags it, so removing the caching fails the test — which is the prompt to fix the header too, rather than leaving a check that passes because it now proves nothing. No changeset: CI wiring, no published surface. A changeset naming any protect-ffi package would fail `scripts/lint-no-ffi-changeset.mjs` regardless — publishing does not move until the trusted-publishing cutover.
tobyhede
force-pushed
the
feat/protect-ffi-4-workspace-link
branch
from
August 6, 2026 01:06
e0d242e to
98aee99
Compare
freshtonic
approved these changes
Aug 6, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Review: APPROVE
Purely CI-wiring + internal dependency linking (stack 4 of 4). Consumers move @cipherstash/protect-ffi from the 0.31.0 pin to workspace:*, and every job that encrypts/decrypts/typechecks against it gains build-ffi-binding. Reviewed in context against the base branch; ran everything runnable locally.
Verification performed
pnpm install --frozen-lockfile— clean, lockfile in sync.pnpm run test:scripts— 295 passing, incl. newffi-binding-step-order.test.mjs(9) and the updatedlint-no-workflow-cachingsuite.- protect-ffi
lintWiring.test.ts— 12 passing;pnpm run lint:ffi-changeset— passes (no protect-ffi changeset in the PR). pnpm run code:check(biome) — clean for the PR's files.EXPECTED_PAIRED_JOBSmatches ground truth exactly (7 files, 8 paired jobs).wasm:flags cross-checked against each job'sCS_IT_SUITE: drizzlewasm:'true'(includesintegration/wasm/**) ✓, supabase native-only ✓, prisma-next native-only ✓.wasm-e2e-testsreorder verified:require-cs-secrets→build-ffi-binding(wasm)→test:typecheck:wasm→Build stack→ Deno. Preflight correctly moves up above the builds.
Blocking
None.
Nits (non-blocking)
prisma-example-readme-e2e.ymlcomment says the binding build "is only ever exercised by the push-to-main run", but the protect-ffi source paths were added to bothpushandpull_requestfilters, so a PR touchingpackages/protect-ffi/**triggers it too. Behaviour is correct/more coverage;prisma-next-e2e.yml's wording ("on push to main and on any PR touching its paths") is the accurate version — consider aligning.EXPECTED_PAIRED_JOBSis a hand-maintained list. Deliberate and well-documented ("guard on the scan"), just flagging that future job renames must touch it.
Highlights
ffi-binding-step-order.test.mjsis exemplary — enforces an invisible ordering invariant, discovers paired jobs rather than listing them, guards the scan against vacuous-pass with a mutation-tested floor, and its failure messages teach the correct direction of the fix.- The caching-gate test uses a live citation (
build-ffi-binding/action.yml) and asserts the caching exists before asserting the gate flags it, so it can't rot into proving nothing. build-ffi-bindingverifies the binding actually loads (assertNativeBindingAvailable()+ wasm-inline probe), failing at the named artifact instead of as scattered downstream encryption errors.workspace:*resolves to the same0.31.0the consumers were pinned to, so the "no published surface / no changeset" reasoning holds. No skills impact — no public API/CLI/workflow surface changed.
Solid, exceptionally well-documented work. Approving.
Three strands, all in `scripts/__tests__/`, all of the same kind: a CI invariant that nothing enforced, or enforced somewhere too narrow to see the next instance. Shared helpers (`lib/repo-root.mjs`, `lib/workflows.mjs`). Thirteen guards declared `REPO_ROOT` byte-for-byte identically, and five rolled their own workflow discovery under three different names. Copies of a discovery helper are the worst kind to let drift, because a copy that finds nothing does not fail — it passes, having checked nothing. `shared-test-helpers.test.mjs` keeps them the only copies. Credentialed-job coverage (`ffi-binding-step-order.test.mjs`). The ordering checks only ever saw jobs that already paired both actions, so a job that never built the binding was invisible to them — not failing, absent. `tests.yml`'s `e2e-tests` and `run-tests-bun` were exactly that, and `run-tests-bun` carries `continue-on-error` plus `|| true`, so its omission was silent in the job least likely to be looked at. Both now build the binding; the guard is a scan, so the next one is covered the day it lands. Paths-filter parity (`workflow-paths-filter-parity.test.mjs`). GitHub Actions has no YAML anchors, so every filter is written twice, and a one-sided edit is silent in the worst direction: the workflow keeps running on main and stops running on the PR that introduced the change. The check existed but lived inside the integration-workflow suite, whose discovery selects on `CS_IT_SUITE` — two workflows, out of the nine that write the list twice. Generalised and moved; all nine were already identical, so it went green on the first run. `tests-rust.yml` is the one legitimate asymmetry and is named with its reason. Review feedback, on comments rather than code: - Both prisma e2e workflows claimed the binding build was "only ever exercised by the push-to-main run". The protect-ffi paths were added to the `pull_request` filter too, so a PR touching them runs it — and the absorption PR itself touches three entries in that filter, which the "path-filtered away from the absorption PR" half also got wrong. Five comment blocks corrected. `workflow-trigger-comments.test.mjs` derives the claim from the parsed `on:` filters, so it is the YAML that decides, not a spell-check against two known sentences. - The hand-maintained scan guards now print the ids the scan DID find. On the failure they will actually see — a job rename — the developer had the old id and no way to get the new one but to re-derive it from the workflows. The message is returned as a value so a test can assert on it; vitest only evaluates an assertion message on failure, which is how a diagnostic nothing tests gets stripped back out. No changeset: CI configuration and repo tooling, no published surface.
`src/nativeLoading.test.ts` asserted `assertNativeBindingAvailable()` does not throw, justified by "this suite runs where a binary is installed". That premise died with the absorption. `index.node` used to arrive prebuilt inside the npm tarball; as a workspace package it is a cargo output, the six `platforms/*` links are empty until someone compiles one, and this package's `build` is `tsc`. So on a fresh checkout the assertion failed with MODULE_NOT_FOUND — and it failed under root `pnpm test`, which reaches here through `turbo test --filter './packages/*'`, making a Rust toolchain a prerequisite of the whole repo's default test. That is the exact thing the entry-point split exists to prevent, arriving through the tests instead of through the scripts. No skip. The assertion splits on a filesystem fact — every path a build leaves an `index.node` at — and BOTH branches assert. With an artifact present the load must succeed. With none, it must fail as `MODULE_NOT_FOUND` naming the platform package, which is the contract `index.cts` documents and `packages/cli`'s `isNativeBinaryMissing` consumes to turn `stash doctor`'s output into an actionable note rather than a raw stack trace. `process.env.CI` was rejected as the split: it is a claim about which machine this is where the question is whether a build has happened, and it lies both ways — `act` and pre-commit wrappers set it without building, and it says nothing about a developer who ran `build:native` and whose positive case would then never run. That negative case turns out to have been covered by nothing. The CLI's own suite hand-builds its inputs, so it proved the matcher matches a string, never that the string is what the loader raises. It now checks against the real error on every artifact-free run. The regression guard is in `lintWiring.test.ts`, beside the existing "keeps cargo off the default test path", because it is the other half of that rule and the half static analysis cannot see: a test does not name `index.node`, it calls an export that reaches the addon four hops down. It re-runs the suite in a child `vitest run` with every binding artifact made unresolvable through `Module._load` and absent from `fs`, and asserts exit 0. The fs half is not redundant with the loader half — the test above decides which branch it is in by looking at the disk, so the two have to agree or the child fails for the wrong reason. Cost is ~1.1s. It earns it: the failure is invisible where it is introduced. An author who ran `build:native` sees green, and so does CI, which builds the binding before running this suite — so in CI this nested run is the only execution of the artifact-free path. Guarded against passing vacuously by asserting the child ran every test file and that the preload logged more than one pid, which proves it reached the fork workers rather than only the vitest process. `tests.yml`'s `run-tests` justified its binding build by citing this test. It no longer can; the step is still required because only 8 of `packages/stack`'s 120 suites mock protect-ffi, so the comment now says that instead.
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
2 participants
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.
Stack 4 of 4 — splitting #858. Base: #862.
packages/protect-ffiand wire its own CIWhat
@cipherstash/stackand the two adapters move from the exact pin0.31.0toworkspace:*, and the CI jobs that reach encryption gain.github/actions/build-ffi-binding. Review found two that did not — see Known gap below.The same commit also wires
lint:ffi-changesetintotests.yml. That guard has existed since #862 and nothing ran it, which is the failure mode the absorption keeps tripping over: a check nothing invokes reads exactly like a check that passes.Why the two halves cannot be split
The npm tarball ships
lib/,index.nodeanddist/wasm/**as contents. The workspace copy ships them as build outputs —lib/is the packagemainand is generated, and nothing producesindex.nodewithout cargo. The moment the dependency line changes, every job that encrypts, decrypts or typechecks against the package needs the binding built first. Split them and the failure isCannot find module '.../index.node', reported once per test rather than once per job.minimumReleaseAgeExcludedrops its@cipherstash/protect-ffi*entry in the same commit and for the same reason: the cooldown applies to registry dependencies, and as of this line these are not registry dependencies.Ordering is load-bearing in two places, and neither looks it
require-cs-secretsmust precedebuild-ffi-binding. The secrets check costs seconds; a cold binding build costs minutes, more withwasm: 'true'. Build first and a job with a rotated credential pays the whole compile before learning it could never have encrypted anything. Both are self-containeduses:blocks and swapping them changes nothing about whether the job passes — henceffi-binding-step-order.test.mjs, checking by position across every workflow, discovered rather than listed. Note the direction of the fix if it fails: inwasm-e2e-testsaBuild stackstep sits between them and consumesdist/wasm, so there the pre-flight moves up, not the build down.Build stackmust follow the binding build inwasm-e2e-tests, because it now consumes protect-ffi'sdist/wasmfrom the workspace copy rather than from a tarball.test:typecheck:wasmreturns here with the job that runs it. It needsdist/wasmso it cannot hang off the defaulttest, andlintWiring's exemption list only accepts a carve-out whose name appears in a root workflow — so the script was held back rather than shipped with a promise.The no-caching gate gains a live citation rather than a fixture:
build-ffi-bindingrestores two Actions caches and its header says publishing workflows must not use it. The test asserts the caching is really there before asserting the gate flags it, so removing the caching fails the test — the prompt to fix the header too, rather than leaving a check that proves nothing.wasm: 'true'is the minority caseAcross the repo three jobs pass it —
integration-drizzle,integration-protect-ffi(from #862), andtests.yml'swasm-e2e-tests— because those are the ones that load the real WASM build; it costs a second cargo build against wasm32.98aee99a's commit message says "exactly two", counting only the jobs that commit touched.AGENTS.mdnow states the rule rather than a number, since the number drifts and the rule does not.Known gap
Review found two credentialed jobs in
tests.ymlthat reach live encryption without building the binding:e2e-tests— runsturbo run test:e2ewith no file filter, soe2e/vitest.config.ts'stests/**/*.e2e.test.tspicks upprisma-example-readme.e2e.test.ts. That suite isdescribe.skipIf(!authConfigured)and un-skips wheneverCS_CLIENT_IDandCS_CLIENT_KEYare set, which this job sets at job level. It drives the README'spnpm startthroughnewClient, and turbo's^buildreaches protect-ffi'stscbut neverindex.node.run-tests-bun— runspackages/stackvitest against live suites;continue-on-error: trueplus|| truemeans it stays green, so it cannot report the breakage it hits.Fix in progress, not in this push: a coverage assertion beside the ordering one in
ffi-binding-step-order.test.mjs, discovering credentialed jobs by scanning the workflow directory rather than by list —run-tests-bunis only visible that way, since its credentials appear solely inside arun:heredoc.Verification
The head of this stack is byte-identical to #858 across all 311 files the absorption touched (verified per-file by blob hash). Main has moved 8 commits ahead since #858 branched; those 29 files are disjoint from the absorption's 311, so nothing of main's newer work is reverted.
Scripts suite 295 passing; protect-ffi's JS chain passing; supply-chain e2e 21 passing; biome 0 errors; lockfile in sync.
No changeset: CI wiring, no published surface — and a changeset naming any protect-ffi package would fail
lint-no-ffi-changeset.mjsregardless.