From 4abc94bceccf254ea237e2f8171350863b162481 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 6 Aug 2026 10:55:47 +1000 Subject: [PATCH 1/4] feat(workspace): link protect-ffi and its six platform packages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@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. --- packages/stack-drizzle/package.json | 2 +- packages/stack-supabase/package.json | 2 +- packages/stack/package.json | 2 +- pnpm-lock.yaml | 74 +++------------------------- pnpm-workspace.yaml | 5 -- 5 files changed, 9 insertions(+), 76 deletions(-) diff --git a/packages/stack-drizzle/package.json b/packages/stack-drizzle/package.json index dc9713602..b67ee7bf7 100644 --- a/packages/stack-drizzle/package.json +++ b/packages/stack-drizzle/package.json @@ -62,7 +62,7 @@ "drizzle-orm": ">=0.33" }, "devDependencies": { - "@cipherstash/protect-ffi": "0.31.0", + "@cipherstash/protect-ffi": "workspace:*", "@cipherstash/test-kit": "workspace:*", "fta-cli": "3.0.0", "dotenv": "17.4.2", diff --git a/packages/stack-supabase/package.json b/packages/stack-supabase/package.json index 0efbfe0b6..5520ba9d7 100644 --- a/packages/stack-supabase/package.json +++ b/packages/stack-supabase/package.json @@ -69,7 +69,7 @@ } }, "devDependencies": { - "@cipherstash/protect-ffi": "0.31.0", + "@cipherstash/protect-ffi": "workspace:*", "@cipherstash/test-kit": "workspace:*", "fta-cli": "3.0.0", "@supabase/postgrest-js": "2.110.2", diff --git a/packages/stack/package.json b/packages/stack/package.json index d538e6f79..17de66bc0 100644 --- a/packages/stack/package.json +++ b/packages/stack/package.json @@ -216,7 +216,7 @@ "dependencies": { "@byteslice/result": "0.2.0", "@cipherstash/auth": "catalog:repo", - "@cipherstash/protect-ffi": "0.31.0", + "@cipherstash/protect-ffi": "workspace:*", "evlog": "1.11.0", "uuid": "14.0.1", "zod": "3.25.76" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0a036469f..5fd1b004d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -439,8 +439,8 @@ importers: specifier: catalog:repo version: 0.42.0(@cipherstash/auth-darwin-arm64@0.42.0)(@cipherstash/auth-darwin-x64@0.42.0)(@cipherstash/auth-linux-arm64-gnu@0.42.0)(@cipherstash/auth-linux-x64-gnu@0.42.0)(@cipherstash/auth-linux-x64-musl@0.42.0)(@cipherstash/auth-win32-x64-msvc@0.42.0) '@cipherstash/protect-ffi': - specifier: 0.31.0 - version: 0.31.0 + specifier: workspace:* + version: link:../protect-ffi evlog: specifier: 1.11.0 version: 1.11.0(next@15.5.21(react-dom@19.2.3(react@19.2.3))(react@19.2.3))(react@19.2.3) @@ -538,8 +538,8 @@ importers: version: link:../stack devDependencies: '@cipherstash/protect-ffi': - specifier: 0.31.0 - version: 0.31.0 + specifier: workspace:* + version: link:../protect-ffi '@cipherstash/test-kit': specifier: workspace:* version: link:../test-kit @@ -666,8 +666,8 @@ importers: version: link:../stack devDependencies: '@cipherstash/protect-ffi': - specifier: 0.31.0 - version: 0.31.0 + specifier: workspace:* + version: link:../protect-ffi '@cipherstash/test-kit': specifier: workspace:* version: link:../test-kit @@ -1094,39 +1094,6 @@ packages: '@cipherstash/eql@3.0.4': resolution: {integrity: sha512-h+/1bMCuglE9pTCZLpVfPTQY2nic5JRoNl+rMQX2B/5cHCWaN5p6Q3OjEOa4Wqf/a7XWn43S/vcJLvWYYvGJuQ==} - '@cipherstash/protect-ffi-darwin-arm64@0.31.0': - resolution: {integrity: sha512-ZXMh+hsgddyKxbDSPN3JbpMOoUJK8LXgX3xL+wqb2IZo8RVFuHKfV3D0WSMFb1Avl/vDVjtzXLzCAM811gy14g==} - cpu: [arm64] - os: [darwin] - - '@cipherstash/protect-ffi-darwin-x64@0.31.0': - resolution: {integrity: sha512-O4KsoIgQchylk6ha3/gnetgjpBOO8catgV9WnNNHJ9gfqWW813ytBrCyKHv8zLx79pl7EkaA8UnE/U6YiOF17A==} - cpu: [x64] - os: [darwin] - - '@cipherstash/protect-ffi-linux-arm64-gnu@0.31.0': - resolution: {integrity: sha512-gYlkx5Ol7iv/cEwS/jy6wFuTI5NaO+BNJnXZnwuu4HrPsXSdcFRd+eY3Z7px9mnb+s6lED72pv6hMcNiwUFZJQ==} - cpu: [arm64] - os: [linux] - - '@cipherstash/protect-ffi-linux-x64-gnu@0.31.0': - resolution: {integrity: sha512-2zhNkN+T1+FN2hYAMOiUwk4r/oun+w0pwfeV2xKJhNtLzDvAfHO30ta/qyPdNtnGm/9+LBljfl4KGvz/0eVIuw==} - cpu: [x64] - os: [linux] - - '@cipherstash/protect-ffi-linux-x64-musl@0.31.0': - resolution: {integrity: sha512-Ll8T6feLmChCmRXAWnsXNH+GzwWAICD+it824BhJgEma8DF8aNSar+HsazkrHNfDvVWGVcsmBARfzxoP+6Wy4A==} - cpu: [x64] - os: [linux] - - '@cipherstash/protect-ffi-win32-x64-msvc@0.31.0': - resolution: {integrity: sha512-fx+61P4z0o+nyPkw7bc8QxC7+wV13I5m1llpvR/dO4juYOeCkrs4q7DdxnUyTIi1GOlQNGwGAV91PMKu70j9RQ==} - cpu: [x64] - os: [win32] - - '@cipherstash/protect-ffi@0.31.0': - resolution: {integrity: sha512-qzTJZE0agyWxlukQbA5CG3//gvmohJhM8a1ehYr8tH6awbrLs6p5mgUs2ClgImO8Mv7O5XcYKdnMIdGni5vC4Q==} - '@clack/core@1.4.3': resolution: {integrity: sha512-/kr3UWNtdJfxZtPgDqUOmG2pvwlmcLGheex5yiZKdwbzZJxhV+HMNR9QNmyY5cGwTNV6LrR7Jtp+KjhUAP1qBQ==} engines: {node: '>= 20.12.0'} @@ -4222,35 +4189,6 @@ snapshots: '@cipherstash/eql@3.0.4': {} - '@cipherstash/protect-ffi-darwin-arm64@0.31.0': - optional: true - - '@cipherstash/protect-ffi-darwin-x64@0.31.0': - optional: true - - '@cipherstash/protect-ffi-linux-arm64-gnu@0.31.0': - optional: true - - '@cipherstash/protect-ffi-linux-x64-gnu@0.31.0': - optional: true - - '@cipherstash/protect-ffi-linux-x64-musl@0.31.0': - optional: true - - '@cipherstash/protect-ffi-win32-x64-msvc@0.31.0': - optional: true - - '@cipherstash/protect-ffi@0.31.0': - dependencies: - '@neon-rs/load': 0.1.82 - optionalDependencies: - '@cipherstash/protect-ffi-darwin-arm64': 0.31.0 - '@cipherstash/protect-ffi-darwin-x64': 0.31.0 - '@cipherstash/protect-ffi-linux-arm64-gnu': 0.31.0 - '@cipherstash/protect-ffi-linux-x64-gnu': 0.31.0 - '@cipherstash/protect-ffi-linux-x64-musl': 0.31.0 - '@cipherstash/protect-ffi-win32-x64-msvc': 0.31.0 - '@clack/core@1.4.3': dependencies: fast-wrap-ansi: 0.2.0 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 06277a8c6..c3f592698 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -94,9 +94,6 @@ blockExoticSubdeps: true # integrate against directly. These ship on their own release cadence and # are not exotic deps. # - @prisma-next/* first-party Prisma packages the integration is built against -# - @cipherstash/protect-ffi* CipherStash-published native FFI and its -# per-platform binary packages (darwin-arm64, -# linux-x64-gnu, etc.); bumped in lockstep with stack # - @cipherstash/auth* CipherStash-published auth strategies (NAPI + # WASM-inline variant); also tracked in lockstep # with protect-ffi for the WASM path. @@ -105,8 +102,6 @@ blockExoticSubdeps: true # payload format protect-ffi emits; bumped in lockstep. minimumReleaseAgeExclude: - '@prisma-next/*' - - '@cipherstash/protect-ffi' - - '@cipherstash/protect-ffi-*' - '@cipherstash/auth' - '@cipherstash/auth-*' - '@cipherstash/eql' From e10bf8006dbc1f1dd72aece5aad35b4d7b5f4e49 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 6 Aug 2026 10:55:47 +1000 Subject: [PATCH 2/4] ci: build the protect-ffi binding in the jobs that need it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/integration-drizzle.yml | 45 ++++- .github/workflows/integration-prisma-next.yml | 42 ++++- .github/workflows/integration-supabase.yml | 43 ++++- .../workflows/prisma-example-readme-e2e.yml | 32 ++++ .github/workflows/prisma-next-e2e.yml | 32 ++++ .github/workflows/tests.yml | 48 ++++- packages/protect-ffi/package.json | 1 + packages/protect-ffi/src/lintWiring.test.ts | 10 +- .../__tests__/ffi-binding-step-order.test.mjs | 165 ++++++++++++++++++ .../uses-build-ffi-binding.yml | 17 ++ .../lint-no-workflow-caching.test.mjs | 31 +++- scripts/lint-no-workflow-caching.mjs | 4 +- 12 files changed, 447 insertions(+), 23 deletions(-) create mode 100644 scripts/__tests__/ffi-binding-step-order.test.mjs create mode 100644 scripts/__tests__/fixtures/lint-no-workflow-caching/uses-build-ffi-binding.yml diff --git a/.github/workflows/integration-drizzle.yml b/.github/workflows/integration-drizzle.yml index f4967a09e..54c1b0897 100644 --- a/.github/workflows/integration-drizzle.yml +++ b/.github/workflows/integration-drizzle.yml @@ -59,6 +59,22 @@ on: - '.github/workflows/integration-drizzle.yml' - '.github/actions/integration-setup/**' - '.github/actions/integration-db/**' + - '.github/actions/build-ffi-binding/**' + # The Rust that produces every EQL payload these suites round-trip. + # Absorbing protect-ffi put it in-tree, so a crate change can now + # break them in a PR that touches no TypeScript at all. + # + # The manifests are here for the same reason as the sources: a + # dependency bump in Cargo.lock changes the compiled encryption core + # without touching a .rs file, and package.json / mise.toml carry the + # build scripts and the toolchain pin. Matching the filter in + # tests-rust.yml, which already covers all four. + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' pull_request: branches: ['**'] # Repeated verbatim: GitHub Actions does not support YAML anchors/aliases. @@ -106,6 +122,22 @@ on: - '.github/workflows/integration-drizzle.yml' - '.github/actions/integration-setup/**' - '.github/actions/integration-db/**' + - '.github/actions/build-ffi-binding/**' + # The Rust that produces every EQL payload these suites round-trip. + # Absorbing protect-ffi put it in-tree, so a crate change can now + # break them in a PR that touches no TypeScript at all. + # + # The manifests are here for the same reason as the sources: a + # dependency bump in Cargo.lock changes the compiled encryption core + # without touching a .rs file, and package.json / mise.toml carry the + # build scripts and the toolchain pin. Matching the filter in + # tests-rust.yml, which already covers all four. + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' jobs: integration: @@ -181,8 +213,9 @@ jobs: - uses: ./.github/actions/integration-setup # Fast pre-flight: fail in seconds if a secret was rotated or cleared, - # before the docker pull. The in-test `requireIntegrationEnv` is the - # correctness guarantee; this is the cheap one. + # before the binding build and the docker pull. The in-test + # `requireIntegrationEnv` is the correctness guarantee; this is the cheap + # one, so nothing expensive may be ordered ahead of it. - name: Require CipherStash secrets uses: ./.github/actions/require-cs-secrets with: @@ -191,6 +224,14 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} + # `wasm: true` because CS_IT_SUITE below includes `integration/wasm/**` + # — the `@cipherstash/stack/wasm-inline` adapter over the shared v3 + # matrix — which loads the real WASM, unlike the mocked unit tests. + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + with: + wasm: 'true' + # No pre-`up` cleanup step any more: the project name is unique per job, so # a container leaked by a hard-killed prior run cannot hold this job's # name or its (ephemeral) port. Blanket-pruning would now be actively diff --git a/.github/workflows/integration-prisma-next.yml b/.github/workflows/integration-prisma-next.yml index ddace2c34..4af283142 100644 --- a/.github/workflows/integration-prisma-next.yml +++ b/.github/workflows/integration-prisma-next.yml @@ -33,6 +33,22 @@ on: - '.github/workflows/integration-prisma-next.yml' - '.github/actions/integration-setup/**' - '.github/actions/integration-db/**' + - '.github/actions/build-ffi-binding/**' + # The Rust that produces every EQL payload these suites round-trip. + # Absorbing protect-ffi put it in-tree, so a crate change can now + # break them in a PR that touches no TypeScript at all. + # + # The manifests are here for the same reason as the sources: a + # dependency bump in Cargo.lock changes the compiled encryption core + # without touching a .rs file, and package.json / mise.toml carry the + # build scripts and the toolchain pin. Matching the filter in + # tests-rust.yml, which already covers all four. + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' pull_request: branches: ['**'] # Repeated verbatim: GitHub Actions does not support YAML anchors/aliases. @@ -52,6 +68,22 @@ on: - '.github/workflows/integration-prisma-next.yml' - '.github/actions/integration-setup/**' - '.github/actions/integration-db/**' + - '.github/actions/build-ffi-binding/**' + # The Rust that produces every EQL payload these suites round-trip. + # Absorbing protect-ffi put it in-tree, so a crate change can now + # break them in a PR that touches no TypeScript at all. + # + # The manifests are here for the same reason as the sources: a + # dependency bump in Cargo.lock changes the compiled encryption core + # without touching a .rs file, and package.json / mise.toml carry the + # build scripts and the toolchain pin. Matching the filter in + # tests-rust.yml, which already covers all four. + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' jobs: integration: @@ -94,8 +126,9 @@ jobs: - uses: ./.github/actions/integration-setup # Fast pre-flight: fail in seconds if a secret was rotated or cleared, - # before the docker pull. The in-test `requireIntegrationEnv` is the - # correctness guarantee; this is the cheap one. + # before the binding build and the docker pull. The in-test + # `requireIntegrationEnv` is the correctness guarantee; this is the cheap + # one, so nothing expensive may be ordered ahead of it. - name: Require CipherStash secrets uses: ./.github/actions/require-cs-secrets with: @@ -104,6 +137,11 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} + # No `wasm: true` — the prisma-next family suites go through the native + # binding only. + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + # No pre-`up` cleanup step any more: the project name is unique per job, so # a container leaked by a hard-killed prior run cannot hold this job's # name or its (ephemeral) port. Blanket-pruning would now be actively diff --git a/.github/workflows/integration-supabase.yml b/.github/workflows/integration-supabase.yml index 34b1c3349..9084dcbf8 100644 --- a/.github/workflows/integration-supabase.yml +++ b/.github/workflows/integration-supabase.yml @@ -51,6 +51,22 @@ on: - '.github/workflows/integration-supabase.yml' - '.github/actions/integration-setup/**' - '.github/actions/integration-db/**' + - '.github/actions/build-ffi-binding/**' + # The Rust that produces every EQL payload these suites round-trip. + # Absorbing protect-ffi put it in-tree, so a crate change can now + # break them in a PR that touches no TypeScript at all. + # + # The manifests are here for the same reason as the sources: a + # dependency bump in Cargo.lock changes the compiled encryption core + # without touching a .rs file, and package.json / mise.toml carry the + # build scripts and the toolchain pin. Matching the filter in + # tests-rust.yml, which already covers all four. + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' pull_request: branches: ['**'] paths: @@ -93,6 +109,22 @@ on: - '.github/workflows/integration-supabase.yml' - '.github/actions/integration-setup/**' - '.github/actions/integration-db/**' + - '.github/actions/build-ffi-binding/**' + # The Rust that produces every EQL payload these suites round-trip. + # Absorbing protect-ffi put it in-tree, so a crate change can now + # break them in a PR that touches no TypeScript at all. + # + # The manifests are here for the same reason as the sources: a + # dependency bump in Cargo.lock changes the compiled encryption core + # without touching a .rs file, and package.json / mise.toml carry the + # build scripts and the toolchain pin. Matching the filter in + # tests-rust.yml, which already covers all four. + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' jobs: integration: @@ -154,8 +186,10 @@ jobs: - uses: ./.github/actions/integration-setup # Fast pre-flight: fail in seconds if a secret was rotated or cleared, - # before paying for the ~2 GB supabase/postgres pull. The in-test - # `requireIntegrationEnv` is the correctness guarantee; this is the cheap one. + # before the binding build and before paying for the ~2 GB + # supabase/postgres pull. The in-test `requireIntegrationEnv` is the + # correctness guarantee; this is the cheap one, so nothing expensive may + # be ordered ahead of it. - name: Require CipherStash secrets uses: ./.github/actions/require-cs-secrets with: @@ -164,6 +198,11 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} + # No `wasm: true` — this job's CS_IT_SUITE is `integration/shared/**` + # only, which goes through the native binding. + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + # No pre-`up` cleanup step any more: the project name is unique per job, so # a container leaked by a hard-killed prior run cannot hold this job's # name or its (ephemeral) port. Blanket-pruning would now be actively diff --git a/.github/workflows/prisma-example-readme-e2e.yml b/.github/workflows/prisma-example-readme-e2e.yml index f0fef5db6..dc5f645c3 100644 --- a/.github/workflows/prisma-example-readme-e2e.yml +++ b/.github/workflows/prisma-example-readme-e2e.yml @@ -18,12 +18,36 @@ on: paths: - 'examples/prisma/**' - '.github/workflows/prisma-example-readme-e2e.yml' + # The walkthrough encrypts against the live service, so the native + # binding and the action that builds it are inputs to this suite. Added + # with that build step: without them a protect-ffi change reaches main + # having never run the walkthrough, and the step itself is only ever + # exercised by the push-to-main run. + - '.github/actions/build-ffi-binding/**' + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' pull_request: branches: - '**' paths: - 'examples/prisma/**' - '.github/workflows/prisma-example-readme-e2e.yml' + # The walkthrough encrypts against the live service, so the native + # binding and the action that builds it are inputs to this suite. Added + # with that build step: without them a protect-ffi change reaches main + # having never run the walkthrough, and the step itself is only ever + # exercised by the push-to-main run. + - '.github/actions/build-ffi-binding/**' + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' jobs: walkthrough: @@ -72,6 +96,8 @@ jobs: # A missing / rotated / fork-PR-absent secret makes the walkthrough skip # its live steps silently, hiding regressions behind a green job. Fail loud. + # First, ahead of the binding build: this costs seconds and that costs + # minutes on a cold cache. - name: Require CipherStash secrets uses: ./.github/actions/require-cs-secrets with: @@ -80,6 +106,12 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} + # The walkthrough encrypts against the live service, so it needs the + # native binding. Path-filtered away from the absorption PR that + # introduced the need, but it runs on push to main. + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + # Build via turbo so `^build` on `@cipherstash/stack-prisma` and # its `@cipherstash/stack` peer is honoured. The test's # `pnpm install` subprocess inside `examples/prisma/` is a no-op diff --git a/.github/workflows/prisma-next-e2e.yml b/.github/workflows/prisma-next-e2e.yml index bfc8fee58..ca0072f93 100644 --- a/.github/workflows/prisma-next-e2e.yml +++ b/.github/workflows/prisma-next-e2e.yml @@ -18,6 +18,18 @@ on: - 'packages/stack-prisma/**' - 'examples/prisma/**' - '.github/workflows/prisma-next-e2e.yml' + # This suite encrypts against the live service, so the native binding and + # the action that builds it are inputs to it. Added with that build step: + # without them a protect-ffi change reaches main having never run this + # suite, and the step itself is only ever exercised by the push-to-main + # run. + - '.github/actions/build-ffi-binding/**' + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' pull_request: branches: - '**' @@ -25,6 +37,18 @@ on: - 'packages/stack-prisma/**' - 'examples/prisma/**' - '.github/workflows/prisma-next-e2e.yml' + # This suite encrypts against the live service, so the native binding and + # the action that builds it are inputs to it. Added with that build step: + # without them a protect-ffi change reaches main having never run this + # suite, and the step itself is only ever exercised by the push-to-main + # run. + - '.github/actions/build-ffi-binding/**' + - 'packages/protect-ffi/crates/**' + - 'packages/protect-ffi/src/**' + - 'packages/protect-ffi/Cargo.toml' + - 'packages/protect-ffi/Cargo.lock' + - 'packages/protect-ffi/package.json' + - 'packages/protect-ffi/mise.toml' jobs: e2e: @@ -75,6 +99,8 @@ jobs: # The global-setup hook hard-errors without CS_WORKSPACE_CRN, but a # missing sibling secret could still degrade coverage silently — assert # all four up front so a rotated / fork-PR-absent secret fails loudly. + # First, ahead of the binding build: this costs seconds and that costs + # minutes on a cold cache. - name: Require CipherStash secrets uses: ./.github/actions/require-cs-secrets with: @@ -83,6 +109,12 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} + # This suite encrypts against the live service, so it needs the native + # binding. Path-filtered away from the absorption PR that introduced the + # need, but it runs on push to main and on any PR touching its paths. + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + # Write the CS_* credentials and the harness DATABASE_URL into the # example app's .env so the runtime + the `prisma-next migration # apply` invocation in global-setup both pick them up. The harness diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index f394b73b3..17852d995 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -238,6 +238,14 @@ jobs: - name: Lint — no references to deleted package directories run: pnpm run lint:package-paths + # TEMPORARY — delete with the script in the phase-4 cutover PR. + # The seven protect-ffi packages live here but still publish from + # cipherstash/protectjs-ffi. A changeset naming any of them bumps all + # seven through their fixed group and the next release tries a publish + # npm has not been repointed to trust. + - name: Lint — no protect-ffi changeset before the publishing cutover + run: pnpm run lint:ffi-changeset + - name: Test — lint script self-tests run: pnpm run test:scripts @@ -371,15 +379,16 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile - # The Deno smoke test imports the locally-built dist/wasm-inline.js - # via a file URL in e2e/wasm/deno.json — it needs a fresh build. - - name: Build stack - run: pnpm exec turbo run build --filter @cipherstash/stack - # The e2e/wasm suites FAIL when any of the four CS_* env vars is # missing (requireEnv throws — no skip gating), so a rotated / cleared # secret can't hide a real WASM regression behind a green job. This - # preflight just fails faster, before the Deno module downloads. + # preflight just fails faster, before the two builds below and the Deno + # module downloads. + # + # It is the preflight that moves above the binding build here, not the + # binding build that moves below it: `Build stack` consumes + # protect-ffi's dist/wasm output, so the two build steps have to stay in + # this order. - name: Require CipherStash secrets uses: ./.github/actions/require-cs-secrets with: @@ -388,6 +397,33 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} + # `wasm: true` — this is the job the WASM output exists for. 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. + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + with: + wasm: 'true' + + # The only job that can run this: the type tests read the GENERATED + # dist/wasm/*.d.ts (wasm-bindgen emits them from the `typescript_type` + # attributes in crates/protect-ffi/src/wasm.rs) and those declarations + # import ../../lib/types.js, so it needs both halves of the step above. + # That is why it is exempt from protect-ffi's `test` entry point, which + # must pass in a fresh clone with no dist/ — and the exemption in + # src/lintWiring.test.ts asserts this step exists, because between the + # absorption and this line the check ran nowhere at all: the jobs its + # exemption named were the upstream copies under + # packages/protect-ffi/.github/, which GitHub never executes. + - name: Typecheck the generated WASM declarations + run: pnpm --filter @cipherstash/protect-ffi run test:typecheck:wasm + + # The Deno smoke test imports the locally-built dist/wasm-inline.js + # via a file URL in e2e/wasm/deno.json — it needs a fresh build. + - name: Build stack + run: pnpm exec turbo run build --filter @cipherstash/stack + - name: Run Deno WASM smoke test working-directory: e2e/wasm run: deno task test diff --git a/packages/protect-ffi/package.json b/packages/protect-ffi/package.json index 906f0c1c6..a56fb8713 100644 --- a/packages/protect-ffi/package.json +++ b/packages/protect-ffi/package.json @@ -15,6 +15,7 @@ "test": "pnpm run test:typecheck && pnpm run test:unit && pnpm run test:lint && pnpm run test:format:ts", "test:cargo": "pnpm run test:rust && pnpm run test:format:rust", "test:typecheck": "tsc && tsc -p tsconfig.test.json", + "test:typecheck:wasm": "tsc -p type-tests/tsconfig.json", "test:unit": "vitest run", "test:rust": "cargo test", "test:lint": "pnpm run test:lint:ts", diff --git a/packages/protect-ffi/src/lintWiring.test.ts b/packages/protect-ffi/src/lintWiring.test.ts index 1b3e28b84..34c7d0ba2 100644 --- a/packages/protect-ffi/src/lintWiring.test.ts +++ b/packages/protect-ffi/src/lintWiring.test.ts @@ -187,12 +187,10 @@ function reachableFromAnyEntryPoint(): Set { * repository root alone, never executed. */ const ENTRY_POINT_EXEMPT: Record = { - // Empty, and that is the correct state rather than an oversight: every - // `test:*` script this package declares is reachable from `test` or - // `test:cargo`. `test:typecheck:wasm` is the one carve-out this list was - // written for, and it arrives with the root tests.yml job that runs it — - // adding the script here before that job exists is precisely the laundering - // the paragraph above describes. + // Runs against the generated wasm .d.ts, so it needs `pnpm run build:wasm` + // first. The default test must still pass in a clone with no dist/, so this + // one belongs to the wasm job in the root tests.yml. + 'test:typecheck:wasm': 'needs dist/wasm, run by the root wasm-e2e job', } describe('lint and format wiring', () => { diff --git a/scripts/__tests__/ffi-binding-step-order.test.mjs b/scripts/__tests__/ffi-binding-step-order.test.mjs new file mode 100644 index 000000000..76873d39e --- /dev/null +++ b/scripts/__tests__/ffi-binding-step-order.test.mjs @@ -0,0 +1,165 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { fileURLToPath } from 'node:url' +import yaml from 'js-yaml' +import { describe, expect, it } from 'vitest' + +/** + * `./.github/actions/require-cs-secrets` must run BEFORE + * `./.github/actions/build-ffi-binding` in any job that uses both. + * + * The secrets action is a pre-flight, and its whole value is being cheap: it + * reads four inputs and fails in seconds when a CS_* secret was rotated, + * cleared, or is absent because the PR came from a fork. Every workflow that + * carries it says so in a comment ("Fast pre-flight: fail in seconds if a + * secret was rotated or cleared, before the docker pull"). + * + * `build-ffi-binding` is the opposite kind of step. On a cache miss it compiles + * the Rust core from cold — minutes of runner time, and more again with + * `wasm: 'true'`. Put it first and a job with no usable credentials pays the + * full compile before it learns it was never going to be able to encrypt + * anything. The pre-flight still fails, just several minutes later and after + * the expensive half of the job has already been billed — which is the same as + * not having a pre-flight at all. + * + * So the order is load-bearing, and it does not look load-bearing: both steps + * are self-contained `uses:` blocks, and swapping them changes nothing about + * whether the job passes. That is exactly the shape of edit that gets made + * while "grouping the build steps together". Hence this test — the two steps + * are checked by position, across every workflow, discovered rather than + * listed, so a new workflow that adds the binding build is covered the day it + * lands. + * + * NOTE the direction of the fix when this fails. Moving the binding build down + * is only correct when nothing between the two steps needs the binding. In + * `tests.yml`'s `wasm-e2e-tests` job a `Build stack` step sits between them and + * consumes `dist/wasm/**`, so there the pre-flight moves UP instead. Same + * resulting order; the other edit would have broken the job. + */ + +const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +const WORKFLOW_DIR = '.github/workflows' + +const BUILD_FFI = './.github/actions/build-ffi-binding' +const REQUIRE_SECRETS = './.github/actions/require-cs-secrets' + +/** + * The JOBS that pair the two actions today. This is NOT the list the checks + * iterate — those scan the directory — it is the guard on the scan itself. A + * discovery test that matches nothing passes and proves nothing, and this repo + * has been bitten by that shape before (see the exit-2 "the linter could not + * run" contract in `scripts/lint-no-hardcoded-runners.mjs`, and + * `lintWiring.test.ts`'s "a check nothing invokes reads exactly like a check + * that passes"). + * + * Jobs, not files, and that distinction is the whole guard. The ordering check + * below is generated per paired job, so deleting a job's pre-flight does not + * fail it — it deletes it. A file-granular list cannot see that: drop the + * `Require CipherStash secrets` step from `tests.yml`'s `wasm-e2e-tests` job + * and `tests.yml` is still paired via `run-tests`, so the file is still found, + * the count still clears its floor, and the suite goes green having stopped + * checking the single most expensive job in the repo — the one that builds the + * binding with `wasm: 'true'`, i.e. the cold compile this pre-flight exists to + * stay ahead of. Mutation-tested: that deletion took the suite from 9 tests to + * 8 passing, with nothing red. + * + * Held as a minimum, not an equality: adding a job that builds the binding + * must not fail this. If one is renamed or genuinely stops needing the + * binding, update the list deliberately. + */ +const EXPECTED_PAIRED_JOBS = [ + '.github/workflows/integration-drizzle.yml / integration', + '.github/workflows/integration-prisma-next.yml / integration', + '.github/workflows/integration-protect-ffi.yml / integration', + '.github/workflows/integration-supabase.yml / integration', + '.github/workflows/prisma-example-readme-e2e.yml / walkthrough', + '.github/workflows/prisma-next-e2e.yml / e2e', + '.github/workflows/tests.yml / run-tests', + '.github/workflows/tests.yml / wasm-e2e-tests', +] + +function workflowFiles() { + return readdirSync(join(REPO_ROOT, WORKFLOW_DIR)) + .filter((name) => /\.ya?ml$/.test(name)) + .map((name) => `${WORKFLOW_DIR}/${name}`) + .sort() +} + +function readWorkflow(relPath) { + return yaml.load(readFileSync(join(REPO_ROOT, relPath), 'utf8')) +} + +/** The `uses:` of a step, normalised — `uses` may carry trailing whitespace. */ +function stepUses(step) { + return typeof step?.uses === 'string' ? step.uses.trim() : null +} + +/** + * Every job that uses BOTH actions, with the step index of each. Indexed by + * position in the job's own `steps` list, which is the order GitHub runs them. + */ +function pairedJobs(relPath) { + const wf = readWorkflow(relPath) + const found = [] + for (const [jobName, job] of Object.entries(wf?.jobs ?? {})) { + const steps = Array.isArray(job?.steps) ? job.steps : [] + const buildAt = [] + const secretsAt = [] + steps.forEach((step, index) => { + const uses = stepUses(step) + if (uses === BUILD_FFI) buildAt.push(index) + if (uses === REQUIRE_SECRETS) secretsAt.push(index) + }) + if (buildAt.length === 0 || secretsAt.length === 0) continue + found.push({ relPath, jobName, steps, buildAt, secretsAt }) + } + return found +} + +const PAIRED = workflowFiles().flatMap(pairedJobs) +const PAIRED_JOB_IDS = PAIRED.map( + (entry) => `${entry.relPath} / ${entry.jobName}`, +) + +describe('protect-ffi binding builds after the secrets pre-flight', () => { + it('finds the jobs that pair the two actions', () => { + // The guard on the scan. Without it, a rename of either action path (or a + // js-yaml parse that quietly returned undefined) would empty `PAIRED` and + // every check below would pass by vacuum. + // + // No separate count assertion: job ids are unique, so an empty `missing` + // already means every expected job was found. A `PAIRED.length >= N` + // floor is what let the `wasm-e2e-tests` deletion through — it had slack + // in it, and slack in a scan guard is where the un-run check hides. + const missing = EXPECTED_PAIRED_JOBS.filter( + (id) => !PAIRED_JOB_IDS.includes(id), + ) + expect( + missing, + `These jobs used both ${BUILD_FFI} and ${REQUIRE_SECRETS}, and the scan no longer sees them. Either an action path changed (update the constants in this file), or a job's pre-flight was dropped — in which case the ordering check for it did not fail, it stopped existing. Restore the step, or update EXPECTED_PAIRED_JOBS deliberately.`, + ).toEqual([]) + }) + + for (const file of workflowFiles()) { + const jobs = pairedJobs(file) + if (jobs.length === 0) continue + + for (const { jobName, steps, buildAt, secretsAt } of jobs) { + it(`${file} / ${jobName} requires secrets before building the binding`, () => { + const firstSecrets = Math.min(...secretsAt) + const firstBuild = Math.min(...buildAt) + const order = steps + .map( + (step, index) => + ` ${index}: ${step?.name ?? stepUses(step) ?? '(unnamed)'}`, + ) + .join('\n') + + expect( + firstSecrets, + `"Require CipherStash secrets" must run before "Build the protect-ffi binding" in ${file} job "${jobName}".\nThe secrets check costs seconds; a cold Rust build costs minutes. Running the build first means a job with a rotated or missing credential pays the whole compile before failing — which is the same as having no pre-flight.\nSteps as ordered:\n${order}`, + ).toBeLessThan(firstBuild) + }) + } + } +}) diff --git a/scripts/__tests__/fixtures/lint-no-workflow-caching/uses-build-ffi-binding.yml b/scripts/__tests__/fixtures/lint-no-workflow-caching/uses-build-ffi-binding.yml new file mode 100644 index 000000000..5062ca112 --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-workflow-caching/uses-build-ffi-binding.yml @@ -0,0 +1,17 @@ +# The exact edit `.github/actions/build-ffi-binding/action.yml` warns against in +# its "DO NOT USE FROM A PUBLISHING WORKFLOW" header: a credential-bearing +# publishing job pulling the binding in through the composite. `uses:` here +# names the real action in this repo, so the test that lints this file proves +# the header's claim rather than restating it. +name: Uses Build Ffi Binding +on: + push: + tags: ['v*'] +jobs: + release: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + - run: pnpm changeset:publish diff --git a/scripts/__tests__/lint-no-workflow-caching.test.mjs b/scripts/__tests__/lint-no-workflow-caching.test.mjs index 193bf3190..8101a81a8 100644 --- a/scripts/__tests__/lint-no-workflow-caching.test.mjs +++ b/scripts/__tests__/lint-no-workflow-caching.test.mjs @@ -94,9 +94,9 @@ describe('lint-no-workflow-caching', () => { // The gate read a step's own `uses:` and stopped there, so a workflow could // reach `actions/cache` through one indirection — `uses: ./.github/actions/x` - // — and stay green. Verified against a copy of release.yml with a - // cache-restoring composite spliced in: exit 0, no output, while the - // composite it never opened restores two caches. + // — and stay green. Verified against a copy of release.yml with + // `.github/actions/build-ffi-binding` spliced in: exit 0, no output, while + // the composite it never opened restores two caches. describe('local composite actions', () => { const cfx = (name) => resolve( @@ -239,6 +239,31 @@ describe('lint-no-workflow-caching', () => { expect(r.output).toMatch(/dir-action-yml/) expect(r.output).toMatch(/no action\.yml or action\.yaml there/) }) + + // A live citation, not a fixture: `.github/actions/build-ffi-binding` is + // the composite whose header says publishing workflows must not use it. + // The first assertion is what keeps the second honest — drop the caching + // from that action and this fails, which is the prompt to fix the header + // too, rather than leaving a test that passes because it now proves + // nothing. + it('flags a publishing workflow that uses .github/actions/build-ffi-binding', () => { + const action = yaml.load( + readFileSync( + resolve(REPO_ROOT, '.github/actions/build-ffi-binding/action.yml'), + 'utf8', + ), + ) + expect( + action?.runs?.steps?.filter((s) => + /^actions\/cache(\/(restore|save))?@/.test(s?.uses ?? ''), + ), + ).not.toHaveLength(0) + + const r = run(fx('uses-build-ffi-binding.yml')) + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/build-ffi-binding\/action\.yml/) + expect(r.output).toMatch(/actions\/cache@/) + }) }) // The step-level twin of the false positive `reusable-input-named-cache` diff --git a/scripts/lint-no-workflow-caching.mjs b/scripts/lint-no-workflow-caching.mjs index d5a109b20..d5907ca66 100644 --- a/scripts/lint-no-workflow-caching.mjs +++ b/scripts/lint-no-workflow-caching.mjs @@ -55,8 +55,8 @@ const PARENT_USES = /^\.\.\// // `useblacksmith/cache@v5` and `Swatinem/rust-cache@v2`: `OK`, exit 0. Both are // live-relevant here — eleven jobs in this repo run on `blacksmith-*` runners, // where `useblacksmith/cache` is the documented drop-in for `actions/cache`, -// and an in-tree Cargo workspace is exactly where someone reaches for -// `Swatinem/rust-cache`. +// and the absorbed Cargo workspace at `packages/protect-ffi` is exactly where +// someone reaches for `Swatinem/rust-cache`. // // The obvious repair is to enumerate the cache actions — by name // (`useblacksmith/cache`, `buildjet/cache`, `runs-on/cache`, `tespkg/actions- From 8898ef294165ce99a6cac23399af19250e7ca821 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 6 Aug 2026 12:24:09 +1000 Subject: [PATCH 3/4] ci: extract the workflow-guard helpers and widen what they cover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../workflows/prisma-example-readme-e2e.yml | 29 +- .github/workflows/prisma-next-e2e.yml | 28 +- .github/workflows/tests.yml | 30 ++ AGENTS.md | 9 +- .../bench-index-expressions.test.mjs | 4 +- scripts/__tests__/ffi-binding-action.test.mjs | 4 +- .../__tests__/ffi-binding-step-order.test.mjs | 291 +++++++++++++++--- .../integration-workflow-paths.test.mjs | 53 +--- scripts/__tests__/lib/repo-root.mjs | 19 ++ scripts/__tests__/lib/workflows.mjs | 55 ++++ .../lint-no-dead-package-paths.test.mjs | 3 +- .../__tests__/lint-no-ffi-changeset.test.mjs | 3 +- .../lint-no-workflow-caching.test.mjs | 3 +- .../no-removed-drizzle-surface.test.mjs | 4 +- .../no-removed-eql-version-flag.test.mjs | 4 +- .../rewriter-copies-in-sync.test.mjs | 4 +- .../__tests__/shared-test-helpers.test.mjs | 154 +++++++++ .../__tests__/turbo-skills-inputs.test.mjs | 6 +- .../workflow-dispatch-job-conditions.test.mjs | 19 +- scripts/__tests__/workflow-node-gyp.test.mjs | 26 +- .../workflow-paths-filter-parity.test.mjs | 159 ++++++++++ .../workflow-trigger-comments.test.mjs | 278 +++++++++++++++++ .../workflow-turbo-build-deps.test.mjs | 15 +- 23 files changed, 1023 insertions(+), 177 deletions(-) create mode 100644 scripts/__tests__/lib/repo-root.mjs create mode 100644 scripts/__tests__/lib/workflows.mjs create mode 100644 scripts/__tests__/shared-test-helpers.test.mjs create mode 100644 scripts/__tests__/workflow-paths-filter-parity.test.mjs create mode 100644 scripts/__tests__/workflow-trigger-comments.test.mjs diff --git a/.github/workflows/prisma-example-readme-e2e.yml b/.github/workflows/prisma-example-readme-e2e.yml index dc5f645c3..a22ffac9b 100644 --- a/.github/workflows/prisma-example-readme-e2e.yml +++ b/.github/workflows/prisma-example-readme-e2e.yml @@ -20,9 +20,11 @@ on: - '.github/workflows/prisma-example-readme-e2e.yml' # The walkthrough encrypts against the live service, so the native # binding and the action that builds it are inputs to this suite. Added - # with that build step: without them a protect-ffi change reaches main - # having never run the walkthrough, and the step itself is only ever - # exercised by the push-to-main run. + # with that build step: without them a protect-ffi change would reach main + # having never run the walkthrough. They are repeated verbatim under + # `pull_request` below, which is what makes that a gate rather than a + # report — a PR touching any of these runs the walkthrough before the + # change lands, not after. - '.github/actions/build-ffi-binding/**' - 'packages/protect-ffi/crates/**' - 'packages/protect-ffi/src/**' @@ -36,11 +38,13 @@ on: paths: - 'examples/prisma/**' - '.github/workflows/prisma-example-readme-e2e.yml' - # The walkthrough encrypts against the live service, so the native - # binding and the action that builds it are inputs to this suite. Added - # with that build step: without them a protect-ffi change reaches main - # having never run the walkthrough, and the step itself is only ever - # exercised by the push-to-main run. + # The same entries as under `push` above, and the copy that does the + # work: a PR touching the native binding or the action that builds it + # runs the walkthrough on the PR, so a protect-ffi change is gated by + # this suite rather than reaching main unexercised. (GitHub Actions has + # no YAML anchors, so the list is written twice; + # scripts/__tests__/workflow-paths-filter-parity.test.mjs compares the + # two copies.) - '.github/actions/build-ffi-binding/**' - 'packages/protect-ffi/crates/**' - 'packages/protect-ffi/src/**' @@ -107,8 +111,13 @@ jobs: client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} # The walkthrough encrypts against the live service, so it needs the - # native binding. Path-filtered away from the absorption PR that - # introduced the need, but it runs on push to main. + # native binding — `packages/protect-ffi` is a workspace package now, so + # `index.node` is a build output rather than tarball contents. This step + # runs on push to main and on any PR whose diff matches the paths filter + # above, the absorption PR that introduced the need included: that PR + # edits this workflow, the build action, and the package itself, and all + # three are in the filter. The exception is a fork PR — it triggers the + # workflow, but the job's `if` skips it for want of CS_* credentials. - name: Build the protect-ffi binding uses: ./.github/actions/build-ffi-binding diff --git a/.github/workflows/prisma-next-e2e.yml b/.github/workflows/prisma-next-e2e.yml index ca0072f93..fd759f289 100644 --- a/.github/workflows/prisma-next-e2e.yml +++ b/.github/workflows/prisma-next-e2e.yml @@ -20,9 +20,10 @@ on: - '.github/workflows/prisma-next-e2e.yml' # This suite encrypts against the live service, so the native binding and # the action that builds it are inputs to it. Added with that build step: - # without them a protect-ffi change reaches main having never run this - # suite, and the step itself is only ever exercised by the push-to-main - # run. + # without them a protect-ffi change would reach main having never run this + # suite. They are repeated verbatim under `pull_request` below, which is + # what makes that a gate rather than a report — a PR touching any of these + # runs the suite before the change lands, not after. - '.github/actions/build-ffi-binding/**' - 'packages/protect-ffi/crates/**' - 'packages/protect-ffi/src/**' @@ -37,11 +38,13 @@ on: - 'packages/stack-prisma/**' - 'examples/prisma/**' - '.github/workflows/prisma-next-e2e.yml' - # This suite encrypts against the live service, so the native binding and - # the action that builds it are inputs to it. Added with that build step: - # without them a protect-ffi change reaches main having never run this - # suite, and the step itself is only ever exercised by the push-to-main - # run. + # The same entries as under `push` above, and the copy that does the + # work: a PR touching the native binding or the action that builds it + # runs this suite on the PR, so a protect-ffi change is gated by it + # rather than reaching main unexercised. (GitHub Actions has no YAML + # anchors, so the list is written twice; + # scripts/__tests__/workflow-paths-filter-parity.test.mjs compares the + # two copies.) - '.github/actions/build-ffi-binding/**' - 'packages/protect-ffi/crates/**' - 'packages/protect-ffi/src/**' @@ -110,8 +113,13 @@ jobs: client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} # This suite encrypts against the live service, so it needs the native - # binding. Path-filtered away from the absorption PR that introduced the - # need, but it runs on push to main and on any PR touching its paths. + # binding — `packages/protect-ffi` is a workspace package now, so + # `index.node` is a build output rather than tarball contents. This step + # runs on push to main and on any PR whose diff matches the paths filter + # above, the absorption PR that introduced the need included: that PR + # edits this workflow, the build action, and the package itself, and all + # three are in the filter. The exception is a fork PR — it triggers the + # workflow, but the job's `if` skips it for want of CS_* credentials. - name: Build the protect-ffi binding uses: ./.github/actions/build-ffi-binding diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 17852d995..c72225649 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -317,6 +317,23 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} + # `e2e/vitest.config.ts` includes `tests/**/*.e2e.test.ts` and the step + # below applies no file filter, so this job runs + # `tests/prisma-example-readme.e2e.test.ts` — whose + # `describe.skipIf(!authConfigured)` un-skips the moment CS_CLIENT_ID and + # CS_CLIENT_KEY are set, which the job env above does. That walkthrough's + # `pnpm start` step encrypts against the live service through + # `@cipherstash/stack`, so it needs `index.node`; turbo's `^build` only + # gets as far as protect-ffi's `build` (tsc → `lib/`), which is the half + # that does not encrypt anything. + # + # After the pre-flight above, per + # scripts/__tests__/ffi-binding-step-order.test.mjs. No `wasm: true` — + # nothing in `e2e/tests/**` loads the WASM build (the Deno smoke test that + # does lives in `e2e/wasm/` and runs in `wasm-e2e-tests`). + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + # Run the standalone `e2e/` workspace via turbo so the `^build` # dep on the `test:e2e` task builds cli + wizard first. CLI's own # E2E (`packages/cli/tests/e2e/**`) is covered by the `run-tests` @@ -471,6 +488,19 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # The `packages/stack` suites below are the live ones — 120 files, of + # which only 8 mock `@cipherstash/protect-ffi` — so they load + # `index.node`, and the `Build packages` step below does not produce it + # (protect-ffi's `build` is `tsc`, deliberately cargo-free). No + # `wasm: true`: every `wasm-inline` unit test resolves the stubs in + # `vitest.shared.ts` or asserts on the bundle graph, same as `run-tests`. + # + # This job has no `require-cs-secrets` pre-flight to sit behind. It shares + # the native cache key with `run-tests`, so in the normal case this is a + # restore rather than a compile. + - name: Build the protect-ffi binding + uses: ./.github/actions/build-ffi-binding + - name: Create .env file in ./packages/stack/ run: | touch ./packages/stack/.env diff --git a/AGENTS.md b/AGENTS.md index a989e5158..47bffb3e7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,12 +102,17 @@ so that stays true for everyone else. Absorbing protect-ffi turned `lib/`, `index.node` and `dist/wasm/**` from tarball contents into build outputs, so every job that encrypts, decrypts, or typechecks against the package builds them first via - `.github/actions/build-ffi-binding` (pass `wasm: 'true'` for the two that - load the real WASM). The action caches `index.node` on a content hash of the + `.github/actions/build-ffi-binding` — passing `wasm: 'true'` only where the + job loads the real WASM build, which is the minority and costs a second cargo + build against wasm32. 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. **A new job that runs live encryption needs this step** — without it the failure is `Cannot find module '.../protect-ffi-linux-x64-gnu/index.node'`, reported once per test rather than once per job. + `scripts/__tests__/ffi-binding-step-order.test.mjs` holds both halves of this: + every job that receives a `CS_*` credential must build the binding, and the + `require-cs-secrets` pre-flight must come first. Both scan the workflow + directory rather than a list, so a new job is covered the day it lands. - **Rust checks live behind `test:cargo`** (`cargo test` + `cargo fmt --check`) and `mise run lint:rust` (clippy, host and wasm32). `build:native` carries `cargo build --release`. diff --git a/scripts/__tests__/bench-index-expressions.test.mjs b/scripts/__tests__/bench-index-expressions.test.mjs index 0ea8c9d10..898bc3c47 100644 --- a/scripts/__tests__/bench-index-expressions.test.mjs +++ b/scripts/__tests__/bench-index-expressions.test.mjs @@ -1,10 +1,8 @@ import { readFileSync } from 'node:fs' import { createRequire } from 'node:module' import { dirname, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' - -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +import { REPO_ROOT } from './lib/repo-root.mjs' // The bench exists to prove the functional-index path engages. An index whose // expression is merely "a term extractor for the right column" satisfies diff --git a/scripts/__tests__/ffi-binding-action.test.mjs b/scripts/__tests__/ffi-binding-action.test.mjs index c6c90bc5d..a50b4b574 100644 --- a/scripts/__tests__/ffi-binding-action.test.mjs +++ b/scripts/__tests__/ffi-binding-action.test.mjs @@ -39,12 +39,10 @@ import { execFileSync } from 'node:child_process' import { existsSync, readFileSync, statSync } from 'node:fs' import { dirname, join, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' import yaml from 'js-yaml' import { describe, expect, it } from 'vitest' import { readJsonc } from './lib/read-jsonc.mjs' - -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +import { REPO_ROOT } from './lib/repo-root.mjs' const ACTION = '.github/actions/build-ffi-binding/action.yml' const RUST_WORKFLOW = '.github/workflows/tests-rust.yml' diff --git a/scripts/__tests__/ffi-binding-step-order.test.mjs b/scripts/__tests__/ffi-binding-step-order.test.mjs index 76873d39e..3e2fb2d24 100644 --- a/scripts/__tests__/ffi-binding-step-order.test.mjs +++ b/scripts/__tests__/ffi-binding-step-order.test.mjs @@ -1,8 +1,5 @@ -import { readdirSync, readFileSync } from 'node:fs' -import { join, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' -import yaml from 'js-yaml' import { describe, expect, it } from 'vitest' +import { readWorkflow, workflowFiles } from './lib/workflows.mjs' /** * `./.github/actions/require-cs-secrets` must run BEFORE @@ -37,9 +34,6 @@ import { describe, expect, it } from 'vitest' * resulting order; the other edit would have broken the job. */ -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') -const WORKFLOW_DIR = '.github/workflows' - const BUILD_FFI = './.github/actions/build-ffi-binding' const REQUIRE_SECRETS = './.github/actions/require-cs-secrets' @@ -65,7 +59,9 @@ const REQUIRE_SECRETS = './.github/actions/require-cs-secrets' * * Held as a minimum, not an equality: adding a job that builds the binding * must not fail this. If one is renamed or genuinely stops needing the - * binding, update the list deliberately. + * binding, update the list deliberately — the failure prints the ids the scan + * DID find (see `scanGuard`), so a rename is a copy from the message into the + * list rather than a re-derivation from the workflows. */ const EXPECTED_PAIRED_JOBS = [ '.github/workflows/integration-drizzle.yml / integration', @@ -74,44 +70,39 @@ const EXPECTED_PAIRED_JOBS = [ '.github/workflows/integration-supabase.yml / integration', '.github/workflows/prisma-example-readme-e2e.yml / walkthrough', '.github/workflows/prisma-next-e2e.yml / e2e', + '.github/workflows/tests.yml / e2e-tests', '.github/workflows/tests.yml / run-tests', '.github/workflows/tests.yml / wasm-e2e-tests', ] -function workflowFiles() { - return readdirSync(join(REPO_ROOT, WORKFLOW_DIR)) - .filter((name) => /\.ya?ml$/.test(name)) - .map((name) => `${WORKFLOW_DIR}/${name}`) - .sort() -} - -function readWorkflow(relPath) { - return yaml.load(readFileSync(join(REPO_ROOT, relPath), 'utf8')) -} - /** The `uses:` of a step, normalised — `uses` may carry trailing whitespace. */ function stepUses(step) { return typeof step?.uses === 'string' ? step.uses.trim() : null } /** - * Every job that uses BOTH actions, with the step index of each. Indexed by - * position in the job's own `steps` list, which is the order GitHub runs them. + * Every job that uses BOTH actions, with the EARLIEST step index of each. + * Indexed by position in the job's own `steps` list, which is the order GitHub + * runs them. + * + * First occurrence, not all of them, and that is the property rather than a + * simplification: the claim is "no credentialed work starts before the + * pre-flight", so what matters is whether the earliest pre-flight precedes the + * earliest build. A job that repeats either step still has to clear that, and + * comparing later pairs would let an early unguarded build through on the + * strength of a later guarded one. */ function pairedJobs(relPath) { const wf = readWorkflow(relPath) const found = [] for (const [jobName, job] of Object.entries(wf?.jobs ?? {})) { const steps = Array.isArray(job?.steps) ? job.steps : [] - const buildAt = [] - const secretsAt = [] - steps.forEach((step, index) => { - const uses = stepUses(step) - if (uses === BUILD_FFI) buildAt.push(index) - if (uses === REQUIRE_SECRETS) secretsAt.push(index) - }) - if (buildAt.length === 0 || secretsAt.length === 0) continue - found.push({ relPath, jobName, steps, buildAt, secretsAt }) + const firstBuild = steps.findIndex((step) => stepUses(step) === BUILD_FFI) + const firstSecrets = steps.findIndex( + (step) => stepUses(step) === REQUIRE_SECRETS, + ) + if (firstBuild === -1 || firstSecrets === -1) continue + found.push({ relPath, jobName, steps, firstBuild, firstSecrets }) } return found } @@ -121,33 +112,149 @@ const PAIRED_JOB_IDS = PAIRED.map( (entry) => `${entry.relPath} / ${entry.jobName}`, ) +/** + * The four CipherStash credentials, as they appear in a workflow: `${{ vars.X }}` + * for the two public ones, `${{ secrets.X }}` for the two that aren't. A job + * carrying any of them is a job that intends to talk to the live service. + * + * Matched against the whole job — `env:` at job level, `env:` on a step, a + * `with:` input to the pre-flight action, or a `run:` body that writes a `.env` + * file. `run-tests-bun` is the last of those and the reason a job-level-`env` + * scan would not have been enough. + */ +const CREDENTIAL_EXPRESSION = + /\$\{\{\s*(?:vars|secrets)\.CS_(?:WORKSPACE_CRN|CLIENT_ID|CLIENT_KEY|CLIENT_ACCESS_KEY)\s*\}\}/ + +/** Every job in a workflow that receives at least one CS_* credential. */ +function credentialedJobs(relPath) { + const wf = readWorkflow(relPath) + const found = [] + for (const [jobName, job] of Object.entries(wf?.jobs ?? {})) { + if (!CREDENTIAL_EXPRESSION.test(JSON.stringify(job))) continue + const steps = Array.isArray(job?.steps) ? job.steps : [] + found.push({ + relPath, + jobName, + buildsBinding: steps.some((step) => stepUses(step) === BUILD_FFI), + }) + } + return found +} + +const CREDENTIALED = workflowFiles().flatMap(credentialedJobs) +const CREDENTIALED_JOB_IDS = CREDENTIALED.map( + (entry) => `${entry.relPath} / ${entry.jobName}`, +) + +/** + * The credentialed JOBS, as the guard on the coverage scan below — same role + * as `EXPECTED_PAIRED_JOBS` plays for the ordering scan, and held as a minimum + * for the same reason. If the credential names or expression syntax ever + * change, this empties the scan, and a coverage check that iterates nothing + * passes while proving nothing. + */ +const EXPECTED_CREDENTIALED_JOBS = [ + '.github/workflows/integration-drizzle.yml / integration', + '.github/workflows/integration-prisma-next.yml / integration', + '.github/workflows/integration-protect-ffi.yml / integration', + '.github/workflows/integration-supabase.yml / integration', + '.github/workflows/prisma-example-readme-e2e.yml / walkthrough', + '.github/workflows/prisma-next-e2e.yml / e2e', + '.github/workflows/tests.yml / e2e-tests', + '.github/workflows/tests.yml / run-tests', + '.github/workflows/tests.yml / run-tests-bun', + '.github/workflows/tests.yml / wasm-e2e-tests', +] + +/** + * Credentialed jobs that legitimately do NOT build the binding, each with the + * reason it doesn't need one. Empty today, and that is the intended steady + * state: every job we hand CS_* to currently runs code that encrypts. + * + * It exists because a job can hold credentials without touching the binding — + * one that only validates that the secrets are present, or one that passes them + * to a service container and never runs a suite. When that job arrives, name it + * here WITH its reason rather than loosening the scan; the second test below + * fails on an entry that no longer matches a credentialed job, so a stale + * exemption cannot outlive the job it was written for. + */ +const BINDING_EXEMPT_JOBS = new Map([ + // [' / ', 'why this job never loads index.node or dist/wasm'], +]) + +/** + * The three hand-maintained lists in this file are checked the same way: every + * id in the list must still match something the scan found. Same shape, and — + * more to the point — the same failure. A job gets renamed, the list still + * names the old id, and the fix is to swap in the new one. + * + * That fix costs two seconds if the message says what the scan DID find, and a + * source dive if it only says what went missing: the developer has the old id + * (it is in the diff) and needs the new one, which appears nowhere in the + * output. Mutation-tested — renaming `integration-drizzle.yml`'s `integration` + * job to `integration-suite` failed both guards, and only the credentialed one + * printed the new name. + * + * So the listing is built HERE rather than at each call site: it is part of + * what makes a guard fixable, and a guard that grew its own terser message + * would quietly lose that. + * + * `message` is returned as a value, not passed straight into `expect`, because + * vitest only evaluates an assertion message when the assertion FAILS. A + * diagnostic that is only read on failure is a diagnostic nothing tests — which + * is how it gets stripped back out. `names every job its scan found` below + * asserts on these strings directly. + */ +function scanGuard({ entries, found, hint }) { + return { + found, + unmatched: entries.filter((id) => !found.includes(id)), + message: `${hint}\nThe scan currently sees:\n${ + found.length === 0 + ? ' (nothing — the scan matched no jobs)' + : found.map((id) => ` ${id}`).join('\n') + }`, + } +} + +const SCAN_GUARDS = { + paired: scanGuard({ + entries: EXPECTED_PAIRED_JOBS, + found: PAIRED_JOB_IDS, + hint: `These jobs used both ${BUILD_FFI} and ${REQUIRE_SECRETS}, and the scan no longer sees them. Either an action path changed (update the constants in this file), or a job's pre-flight was dropped — in which case the ordering check for it did not fail, it stopped existing. Restore the step, or update EXPECTED_PAIRED_JOBS deliberately.`, + }), + credentialed: scanGuard({ + entries: EXPECTED_CREDENTIALED_JOBS, + found: CREDENTIALED_JOB_IDS, + hint: `These jobs pass a CS_* credential and the scan no longer sees them. Either the credential names or the \`\${{ }}\` syntax changed (update CREDENTIAL_EXPRESSION), or the job was renamed or deleted — update EXPECTED_CREDENTIALED_JOBS deliberately.`, + }), + exemptions: scanGuard({ + entries: [...BINDING_EXEMPT_JOBS.keys()], + found: CREDENTIALED_JOB_IDS, + hint: 'These BINDING_EXEMPT_JOBS entries do not match any credentialed job. Remove them, or fix the id — an exemption for a job that no longer exists exempts nothing and hides the next one.', + }), +} + describe('protect-ffi binding builds after the secrets pre-flight', () => { it('finds the jobs that pair the two actions', () => { // The guard on the scan. Without it, a rename of either action path (or a // js-yaml parse that quietly returned undefined) would empty `PAIRED` and // every check below would pass by vacuum. // - // No separate count assertion: job ids are unique, so an empty `missing` + // No separate count assertion: job ids are unique, so an empty `unmatched` // already means every expected job was found. A `PAIRED.length >= N` // floor is what let the `wasm-e2e-tests` deletion through — it had slack // in it, and slack in a scan guard is where the un-run check hides. - const missing = EXPECTED_PAIRED_JOBS.filter( - (id) => !PAIRED_JOB_IDS.includes(id), - ) - expect( - missing, - `These jobs used both ${BUILD_FFI} and ${REQUIRE_SECRETS}, and the scan no longer sees them. Either an action path changed (update the constants in this file), or a job's pre-flight was dropped — in which case the ordering check for it did not fail, it stopped existing. Restore the step, or update EXPECTED_PAIRED_JOBS deliberately.`, - ).toEqual([]) + const guard = SCAN_GUARDS.paired + expect(guard.unmatched, guard.message).toEqual([]) }) for (const file of workflowFiles()) { const jobs = pairedJobs(file) if (jobs.length === 0) continue - for (const { jobName, steps, buildAt, secretsAt } of jobs) { + for (const { jobName, steps, firstBuild, firstSecrets } of jobs) { it(`${file} / ${jobName} requires secrets before building the binding`, () => { - const firstSecrets = Math.min(...secretsAt) - const firstBuild = Math.min(...buildAt) const order = steps .map( (step, index) => @@ -163,3 +270,105 @@ describe('protect-ffi binding builds after the secrets pre-flight', () => { } } }) + +/** + * COVERAGE, where the checks above are ORDER. Those only look at jobs that + * already pair the two actions, so a job that never builds the binding at all + * is invisible to them — it is not failing them, it is not in them. + * + * That gap is not theoretical. `packages/protect-ffi` is a workspace package + * now, so `lib/`, `index.node` and `dist/wasm/**` are BUILD OUTPUTS rather than + * tarball contents, and every job that encrypts has to produce them itself. Two + * credentialed jobs in `tests.yml` were missed when the `workspace:*` links + * landed: `e2e-tests` (its unfiltered `test:e2e` run picks up + * `tests/prisma-example-readme.e2e.test.ts`, whose `describe.skipIf` un-skips + * the moment CS_CLIENT_ID and CS_CLIENT_KEY are set, and whose `pnpm start` + * step encrypts for real) and `run-tests-bun` (writes the four CS_* into + * `packages/stack/.env`, then runs 120 `packages/stack` suites of which only 8 + * mock protect-ffi). + * + * The failure mode is `Cannot find module '.../index.node'`, reported once per + * test rather than once per job — and in `run-tests-bun` not reported at all, + * since it carries `continue-on-error: true` AND a `|| true` around the vitest + * invocation. So the omission is silent in exactly the job least likely to be + * looked at. + * + * Discovered, not listed: a new credentialed job is covered the day it lands, + * which is the only way this stays true. Exemptions are explicit and carry + * their reason (see BINDING_EXEMPT_JOBS). + */ +describe('every credentialed job builds the protect-ffi binding', () => { + it('finds the jobs that receive CipherStash credentials', () => { + const guard = SCAN_GUARDS.credentialed + expect(guard.unmatched, guard.message).toEqual([]) + }) + + it('exempts only jobs that are still credentialed', () => { + // A stale exemption is the silent-skip failure this whole file exists to + // avoid: the named job gets renamed or gains a real need for the binding, + // and the entry sits there exempting nothing while reading as deliberate. + const guard = SCAN_GUARDS.exemptions + expect(guard.unmatched, guard.message).toEqual([]) + }) + + it('builds the binding in every credentialed job', () => { + const offenders = CREDENTIALED.filter( + (entry) => + !entry.buildsBinding && + !BINDING_EXEMPT_JOBS.has(`${entry.relPath} / ${entry.jobName}`), + ).map((entry) => `${entry.relPath} / ${entry.jobName}`) + + expect( + offenders, + `These jobs receive CipherStash credentials but never run ${BUILD_FFI}.\n\`packages/protect-ffi\` is a workspace package: \`lib/\`, \`index.node\` and \`dist/wasm/**\` are build outputs, not tarball contents, so a job that encrypts or decrypts must build them itself. Without the step the suite fails with \`Cannot find module '.../protect-ffi-linux-x64-gnu/index.node'\`, once per test.\nAdd the step AFTER any \`${REQUIRE_SECRETS}\` pre-flight and BEFORE anything that consumes the binding. Pass \`wasm: 'true'\` only if the job loads the real WASM build.\nIf a job genuinely does not need it, add it to BINDING_EXEMPT_JOBS with the reason.`, + ).toEqual([]) + }) +}) + +/** + * The guards above are only ever read when they FAIL, which is the one moment + * their text has to be right and the one moment nothing is watching it. So the + * text itself is asserted here, on the real strings the guards carry. + * + * The property is "the message names every job the scan found", and it is the + * difference between a two-second fix and a source dive on the most likely + * failure these guards will ever see: a job rename. The developer has the OLD + * id — it is in the list, and in the diff — and needs the NEW one. Without the + * listing the new id appears nowhere in the output, and the only way to get it + * is to open the workflow and re-derive what the scan would have matched. + */ +describe('the scan guards name what they found', () => { + for (const [label, guard] of Object.entries(SCAN_GUARDS)) { + it(`${label}: the failure message lists every job its scan found`, () => { + // Not vacuous: an empty scan would satisfy a bare forEach over `found`. + // These guards exist precisely because a scan can silently empty out. + expect( + guard.found.length, + `The ${label} scan matched no jobs at all, so this test would pass without checking anything.`, + ).toBeGreaterThan(0) + + const unlisted = guard.found.filter((id) => !guard.message.includes(id)) + expect( + unlisted, + `The ${label} guard's failure message does not name these jobs, and they are exactly what a reader needs.\nWhen a job is renamed the guard reports the OLD id as missing; the NEW id is only visible if the message lists what the scan found. Keep the listing (see \`scanGuard\`).\nMessage as it stands:\n${guard.message}`, + ).toEqual([]) + }) + } + + it('says so explicitly when the scan matched nothing', () => { + // The empty scan is the catastrophic case, not a corner case: rename either + // action path and `pairedJobs` matches nothing at all (mutation-tested — + // pointing BUILD_FFI at a path no workflow uses took the file from 17 tests + // to 8, with the nine ordering checks not failing but ceasing to exist). + // Rendered as a bare empty list the message + // ends on a dangling "The scan currently sees:" with vitest's own + // `expected [...] to deeply equal []` running on from the same line, which + // reads as a broken template rather than as the finding. + const guard = scanGuard({ + entries: ['workflow.yml / job'], + found: [], + hint: 'hint', + }) + expect(guard.message).toContain('(nothing — the scan matched no jobs)') + }) +}) diff --git a/scripts/__tests__/integration-workflow-paths.test.mjs b/scripts/__tests__/integration-workflow-paths.test.mjs index 76fd71ef7..eb5fd0374 100644 --- a/scripts/__tests__/integration-workflow-paths.test.mjs +++ b/scripts/__tests__/integration-workflow-paths.test.mjs @@ -1,8 +1,8 @@ import { existsSync, readdirSync, readFileSync, statSync } from 'node:fs' -import { join, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' -import yaml from 'js-yaml' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' +import { REPO_ROOT } from './lib/repo-root.mjs' +import { readWorkflow, workflowFiles } from './lib/workflows.mjs' /** * An integration workflow only runs when its `paths:` filter matches the diff. @@ -28,13 +28,14 @@ import { describe, expect, it } from 'vitest' * derives which manifest that is (package manifest for exact pins, * `pnpm-workspace.yaml` for `catalog:` ones). * - * Finally, GitHub Actions has no YAML anchors, so every filter is written twice. - * A one-sided edit disables the job on pull requests while leaving it green on - * `main` — the exact inversion of what you want — so the two copies are - * compared directly. + * The third property this file used to hold — that the `push` and + * `pull_request` copies of each filter are identical, GitHub Actions having no + * YAML anchors — now lives in `workflow-paths-filter-parity.test.mjs`. It was + * never integration-specific, and holding it here scoped it to the discovery + * below: two workflows, out of the nine that write the list twice. The two it + * did cover are covered there. */ -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') const STACK_SRC = 'packages/stack/src' const STACK_MANIFEST = 'packages/stack/package.json' const CATALOG_MANIFEST = 'pnpm-workspace.yaml' @@ -42,10 +43,6 @@ const CATALOG_MANIFEST = 'pnpm-workspace.yaml' /** Both trigger events, in the order GitHub evaluates them. */ const TRIGGER_EVENTS = ['push', 'pull_request'] -function readWorkflow(relPath) { - return yaml.load(readFileSync(join(REPO_ROOT, relPath), 'utf8')) -} - /** * The integration workflows, discovered rather than listed: any workflow whose * `CS_IT_SUITE` globs select suites out of `packages/stack/` is in scope. A @@ -53,27 +50,15 @@ function readWorkflow(relPath) { * anyone remembering to add it here. */ function discoverWorkflows() { - const dir = join(REPO_ROOT, '.github/workflows') - return readdirSync(dir) - .filter((name) => name.endsWith('.yml') || name.endsWith('.yaml')) - .map((name) => `.github/workflows/${name}`) - .filter( - (relPath) => suiteFiles(suiteGlobs(readWorkflow(relPath))).length > 0, - ) - .sort() + return workflowFiles().filter( + (relPath) => suiteFiles(suiteGlobs(readWorkflow(relPath))).length > 0, + ) } /** * `on:` parses as the boolean `true` under YAML 1.1 (the "Norway problem"), * which is why this reads both keys rather than `wf.on`. */ -function triggerFilters(wf) { - const on = wf.on ?? wf[true] - return Object.fromEntries( - TRIGGER_EVENTS.map((event) => [event, on?.[event]?.paths]), - ) -} - function triggerBlocks(wf) { const on = wf.on ?? wf[true] return TRIGGER_EVENTS.map((event) => on?.[event]).filter( @@ -203,20 +188,6 @@ describe('integration workflow paths filters', () => { }) for (const relPath of WORKFLOWS) { - /** - * GitHub Actions has no YAML anchors, so each filter is written out twice. - * A one-sided edit is silent: the workflow keeps running on `push` to main - * and stops running on the PR that introduced the break, which is the only - * time it matters. Compare the two lists rather than trusting the comment. - */ - it(`${relPath} repeats an identical paths filter under push and pull_request`, () => { - const filters = triggerFilters(readWorkflow(relPath)) - for (const event of TRIGGER_EVENTS) { - expect(Array.isArray(filters[event])).toBe(true) - } - expect(filters.pull_request).toEqual(filters.push) - }) - it(`${relPath} triggers on the manifests pinning its suites' dependencies`, () => { const wf = readWorkflow(relPath) const blocks = triggerBlocks(wf) diff --git a/scripts/__tests__/lib/repo-root.mjs b/scripts/__tests__/lib/repo-root.mjs new file mode 100644 index 000000000..f1b1db383 --- /dev/null +++ b/scripts/__tests__/lib/repo-root.mjs @@ -0,0 +1,19 @@ +import { resolve } from 'node:path' +import { fileURLToPath } from 'node:url' + +/** + * The repository root, resolved from this file's own location. + * + * Every guard in `scripts/__tests__/` asserts against real repo files — + * workflows, manifests, source trees, `turbo.json` — so every one of them needs + * this, and thirteen of them declared it byte-for-byte identically before it + * was extracted. One copy is one place to fix if the directory ever moves; the + * other twelve would have been found one failing suite at a time, which is not + * a plan. + * + * Note the depth. This lives one directory deeper than the test files that used + * to declare it, so it climbs `../../../..` where they climbed `../../..`. + * `resolve` treats the file path as the starting segment, hence the extra level + * that looks off by one and is not. + */ +export const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../../..') diff --git a/scripts/__tests__/lib/workflows.mjs b/scripts/__tests__/lib/workflows.mjs new file mode 100644 index 000000000..9f25387f6 --- /dev/null +++ b/scripts/__tests__/lib/workflows.mjs @@ -0,0 +1,55 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import yaml from 'js-yaml' +import { REPO_ROOT } from './repo-root.mjs' + +/** + * Reading `.github/workflows`, for the guards that assert things about CI. + * + * Several of those guards are DISCOVERY tests — they scan the directory rather + * than iterate a list, so a workflow added tomorrow is held to the same bar + * without anyone remembering to register it. That design only works if every + * guard discovers the same set, and before this module they each rolled their + * own: three byte-for-byte copies of `workflowFiles` and `readWorkflow`, plus + * two more spelled differently (`discoverWorkflows`, a bare `WORKFLOWS` const) + * and a `readYaml` that was `readWorkflow` under another name. 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. + * + * Extracted alongside `read-jsonc.mjs`, for the same reason it was. + */ + +/** + * Where GitHub reads workflows from — and the only place it reads them from. A + * workflow file deposited under a package's own `.github/workflows` is inert, + * which is the failure `packages/protect-ffi/src/integrationSuiteCi.test.ts` + * exists to catch. + */ +export const WORKFLOW_DIR = '.github/workflows' + +/** + * Every workflow, as a repo-relative path, sorted. + * + * Both extensions: GitHub accepts `.yml` and `.yaml`, so a guard that filtered + * on one of them would silently stop covering a workflow the day someone spelt + * it the other way. + */ +export function workflowFiles() { + return readdirSync(join(REPO_ROOT, WORKFLOW_DIR)) + .filter((name) => /\.ya?ml$/.test(name)) + .map((name) => `${WORKFLOW_DIR}/${name}`) + .sort() +} + +/** + * Parse one repo-relative YAML file from the workflow graph — a workflow, or a + * composite action manifest that a workflow reaches through `uses: ./...`. + * `workflow-node-gyp.test.mjs` follows those `uses:` edges and needs to parse + * both kinds with the same reader. + * + * Note for callers: `on:` parses as the boolean `true` under YAML 1.1 (the + * "Norway problem"), so read triggers as `wf.on ?? wf[true]`. + */ +export function readWorkflow(relPath) { + return yaml.load(readFileSync(join(REPO_ROOT, relPath), 'utf8')) +} diff --git a/scripts/__tests__/lint-no-dead-package-paths.test.mjs b/scripts/__tests__/lint-no-dead-package-paths.test.mjs index 92f9d7aff..b95315932 100644 --- a/scripts/__tests__/lint-no-dead-package-paths.test.mjs +++ b/scripts/__tests__/lint-no-dead-package-paths.test.mjs @@ -4,14 +4,13 @@ import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' +import { REPO_ROOT } from './lib/repo-root.mjs' const SCRIPT = resolve( fileURLToPath(import.meta.url), '../../lint-no-dead-package-paths.mjs', ) -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') - function run(...targets) { return runWith({}, ...targets) } diff --git a/scripts/__tests__/lint-no-ffi-changeset.test.mjs b/scripts/__tests__/lint-no-ffi-changeset.test.mjs index 6e7852f53..149d0d89b 100644 --- a/scripts/__tests__/lint-no-ffi-changeset.test.mjs +++ b/scripts/__tests__/lint-no-ffi-changeset.test.mjs @@ -10,13 +10,12 @@ import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { afterAll, describe, expect, it } from 'vitest' +import { REPO_ROOT } from './lib/repo-root.mjs' const SCRIPT = resolve( fileURLToPath(import.meta.url), '../../lint-no-ffi-changeset.mjs', ) -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') - function run(dir) { try { const stdout = execFileSync('node', dir ? [SCRIPT, dir] : [SCRIPT], { diff --git a/scripts/__tests__/lint-no-workflow-caching.test.mjs b/scripts/__tests__/lint-no-workflow-caching.test.mjs index 8101a81a8..a9cf8f4d8 100644 --- a/scripts/__tests__/lint-no-workflow-caching.test.mjs +++ b/scripts/__tests__/lint-no-workflow-caching.test.mjs @@ -4,6 +4,7 @@ import { resolve } from 'node:path' import { fileURLToPath } from 'node:url' import yaml from 'js-yaml' import { describe, expect, it } from 'vitest' +import { REPO_ROOT } from './lib/repo-root.mjs' // Workflows the supply-chain gate is responsible for. const TARGET_WORKFLOWS = [ @@ -15,8 +16,6 @@ const SCRIPT = resolve( fileURLToPath(import.meta.url), '../../lint-no-workflow-caching.mjs', ) -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') - function run(...targets) { try { execFileSync('node', [SCRIPT, ...targets], { encoding: 'utf8' }) diff --git a/scripts/__tests__/no-removed-drizzle-surface.test.mjs b/scripts/__tests__/no-removed-drizzle-surface.test.mjs index 64334bce4..1ee0f063c 100644 --- a/scripts/__tests__/no-removed-drizzle-surface.test.mjs +++ b/scripts/__tests__/no-removed-drizzle-surface.test.mjs @@ -1,10 +1,8 @@ import { execFileSync } from 'node:child_process' import { readFileSync } from 'node:fs' import { resolve } from 'node:path' -import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' - -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +import { REPO_ROOT } from './lib/repo-root.mjs' /** * The `@cipherstash/stack-drizzle` names removed when EQL v2 went away and diff --git a/scripts/__tests__/no-removed-eql-version-flag.test.mjs b/scripts/__tests__/no-removed-eql-version-flag.test.mjs index cbc8323af..d2aea7e6a 100644 --- a/scripts/__tests__/no-removed-eql-version-flag.test.mjs +++ b/scripts/__tests__/no-removed-eql-version-flag.test.mjs @@ -1,10 +1,8 @@ import { execFileSync } from 'node:child_process' import { readFileSync } from 'node:fs' import { resolve } from 'node:path' -import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' - -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +import { REPO_ROOT } from './lib/repo-root.mjs' /** Tracked executable examples that describe the current public surface. */ function publicCommandDocs() { diff --git a/scripts/__tests__/rewriter-copies-in-sync.test.mjs b/scripts/__tests__/rewriter-copies-in-sync.test.mjs index 96aae7b51..649a5ee6e 100644 --- a/scripts/__tests__/rewriter-copies-in-sync.test.mjs +++ b/scripts/__tests__/rewriter-copies-in-sync.test.mjs @@ -1,9 +1,7 @@ import { readFileSync } from 'node:fs' import { resolve } from 'node:path' -import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' - -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +import { REPO_ROOT } from './lib/repo-root.mjs' /** * The ALTER-COLUMN rewriter exists twice: `@cipherstash/wizard` diff --git a/scripts/__tests__/shared-test-helpers.test.mjs b/scripts/__tests__/shared-test-helpers.test.mjs new file mode 100644 index 000000000..42c4ac66f --- /dev/null +++ b/scripts/__tests__/shared-test-helpers.test.mjs @@ -0,0 +1,154 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { pathToFileURL } from 'node:url' +import { describe, expect, it } from 'vitest' +import { REPO_ROOT } from './lib/repo-root.mjs' +import { WORKFLOW_DIR } from './lib/workflows.mjs' + +/** + * `scripts/__tests__/lib/` is where a helper shared by more than one guard + * lives. This checks the guards actually use it. + * + * WHAT HAPPENED. Four declarations — `REPO_ROOT`, `WORKFLOW_DIR`, + * `workflowFiles()` and `readWorkflow()` — were copied byte-for-byte between + * test files: `REPO_ROOT` into thirteen of them, the workflow trio into three, + * with two further copies wearing different names (`discoverWorkflows`, a bare + * `WORKFLOWS` const) and one whole function renamed (`readYaml`). Nothing + * failed, because copies of a working helper work. + * + * WHY IT MATTERS MORE HERE THAN IN NORMAL CODE. Most of these guards are + * DISCOVERY tests: they scan the workflow directory rather than iterate a list, so + * a workflow added tomorrow is held to the same bar without anyone registering + * it. A discovery helper is the one kind of duplicate whose drift is silent in + * the dangerous direction — a copy that finds fewer files does not fail, it + * passes, having checked less. `ffi-binding-step-order.test.mjs` records a + * mutation test where a deleted step took its suite from 9 tests to 8 with + * nothing red; a copy that quietly stops matching `.yaml` files does the same + * thing to every guard that holds it. + * + * WHAT THIS CAN AND CANNOT SEE. It reads source text, so it catches a + * redeclaration under the SAME name as a shared export, and a hand-rolled scan + * of the workflow directory under any name. It cannot see a one-line copy given + * a fresh name and a fresh directory literal — no source-level check can, short + * of comparing semantics. The two checks below are named for exactly what they + * do rather than for the ambition. + */ + +const TESTS_DIR = 'scripts/__tests__' +const LIB_DIR = `${TESTS_DIR}/lib` + +/** Repo-relative paths of the shared helper modules. */ +function libModules() { + return readdirSync(join(REPO_ROOT, LIB_DIR)) + .filter((name) => name.endsWith('.mjs') && !name.endsWith('.test.mjs')) + .map((name) => `${LIB_DIR}/${name}`) + .sort() +} + +/** Repo-relative paths of the guards themselves — `lib/` is not among them. */ +function testFiles() { + return readdirSync(join(REPO_ROOT, TESTS_DIR)) + .filter((name) => name.endsWith('.test.mjs')) + .map((name) => `${TESTS_DIR}/${name}`) + .sort() +} + +const read = (relPath) => readFileSync(join(REPO_ROOT, relPath), 'utf8') + +/** + * Every name `lib/` exports, mapped to the module exporting it. Taken by + * importing the modules rather than by reading their source: an export added + * later is then protected the day it lands, which is the same discovered-not- + * listed property the guards themselves rely on. + */ +const SHARED = new Map() +for (const relPath of libModules()) { + const mod = await import(pathToFileURL(join(REPO_ROOT, relPath)).href) + for (const name of Object.keys(mod)) SHARED.set(name, relPath) +} + +/** + * The guard on the scan, in the shape this directory uses everywhere else. If + * a rename emptied `SHARED`, every check below would iterate nothing and pass — + * the failure mode the whole directory is written to avoid. + * + * A minimum, not an equality: extracting a fifth helper must not fail this. + */ +const EXPECTED_SHARED = [ + 'REPO_ROOT', + 'WORKFLOW_DIR', + 'readJsonc', + 'readWorkflow', + 'workflowFiles', +] + +/** + * A top-level declaration of `name`. Anchored to the start of a line, which is + * what makes it a DECLARATION test rather than a mention test: a local inside a + * function body is indented, and a reference in prose sits behind ` * ` or + * `// `. Destructured forms (`const { readWorkflow } = ...`) are not matched, + * and do not need to be — that is a re-export of the shared one, not a copy. + */ +function declaresLocally(source, name) { + return new RegExp( + `^(?:export\\s+)?(?:async\\s+)?(?:function\\s+${name}\\b|(?:const|let|var)\\s+${name}\\b)`, + 'm', + ).test(source) +} + +/** + * The workflow directory written as a bare string literal — the directory + * itself, rather than the prefix of a specific file. The closing quote is the + * whole discrimination: `'.github/workflows/tests.yml'` in an expected-jobs + * list is a fact being asserted, while the bare directory is the opening line + * of a hand-rolled `readdirSync` scan. It also keeps the fixture trees under + * `fixtures/lint-no-workflow-caching/` out of scope, since those are always + * spelled with a trailing file segment. + * + * This file quotes the directory nowhere itself — it interpolates + * `WORKFLOW_DIR` below — which is the rule being enforced, applied here first. + * The alternative was to exempt this file from its own check, and an exemption + * is a hole whether or not the file that opens it is the one describing the + * rule. + */ +const BARE_WORKFLOW_DIR = /['"`]\.github\/workflows['"`]/ + +describe('guards import shared helpers instead of copying them', () => { + it('lib/ still exports the helpers these checks are about', () => { + const missing = EXPECTED_SHARED.filter((name) => !SHARED.has(name)) + expect( + missing, + `These helpers are no longer exported from ${LIB_DIR}/. If one was renamed, update EXPECTED_SHARED deliberately; if the directory moved, the checks below are scanning nothing and passing. Found: ${[...SHARED.keys()].sort().join(', ') || '(nothing)'}`, + ).toEqual([]) + }) + + it('no test file redefines a helper that lib/ already exports', () => { + const offenders = [] + for (const relPath of testFiles()) { + const source = read(relPath) + for (const [name, module] of SHARED) { + if (declaresLocally(source, name)) { + offenders.push( + `${relPath} declares \`${name}\` (exported by ${module})`, + ) + } + } + } + + expect( + offenders, + `A helper that ${LIB_DIR}/ exports was declared locally again.\nImport it instead — \`import { } from './lib/.mjs'\`.\nThese guards discover what they check by scanning the repo, so a private copy that drifts does not fail: it finds less and passes.\nIf the local version genuinely needs to differ, give it a name that says how, and say why in a comment.`, + ).toEqual([]) + }) + + it('no test file scans the workflow directory itself', () => { + const offenders = testFiles().filter((relPath) => + BARE_WORKFLOW_DIR.test(read(relPath)), + ) + + expect( + offenders, + `These files spell out \`${WORKFLOW_DIR}\` as a directory. Reading it is \`workflowFiles()\` from './lib/workflows.mjs', and the constant is \`WORKFLOW_DIR\` from the same module.\nThis check exists because the previous copies were not all called the same thing — one was \`discoverWorkflows()\`, one a bare \`WORKFLOWS\` const — so matching on the helper NAME would have missed them. Matching on the directory catches a copy whatever it is called.\nNaming a specific workflow FILE (\`${WORKFLOW_DIR}/tests.yml\`) is fine and is not matched.`, + ).toEqual([]) + }) +}) diff --git a/scripts/__tests__/turbo-skills-inputs.test.mjs b/scripts/__tests__/turbo-skills-inputs.test.mjs index a2413a121..a69d2edea 100644 --- a/scripts/__tests__/turbo-skills-inputs.test.mjs +++ b/scripts/__tests__/turbo-skills-inputs.test.mjs @@ -18,12 +18,10 @@ */ import { existsSync, readdirSync, readFileSync } from 'node:fs' -import { join, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' import { readJsonc } from './lib/read-jsonc.mjs' - -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +import { REPO_ROOT } from './lib/repo-root.mjs' /** Packages whose tsup config copies the repo-root `skills/` into `dist/`. */ function packagesCopyingSkills() { diff --git a/scripts/__tests__/workflow-dispatch-job-conditions.test.mjs b/scripts/__tests__/workflow-dispatch-job-conditions.test.mjs index 11f0f27c1..72d807d24 100644 --- a/scripts/__tests__/workflow-dispatch-job-conditions.test.mjs +++ b/scripts/__tests__/workflow-dispatch-job-conditions.test.mjs @@ -1,8 +1,5 @@ -import { readdirSync, readFileSync } from 'node:fs' -import { join, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' -import yaml from 'js-yaml' import { describe, expect, it } from 'vitest' +import { readWorkflow, workflowFiles } from './lib/workflows.mjs' /** * A workflow that declares `workflow_dispatch:` must actually run when someone @@ -40,9 +37,6 @@ import { describe, expect, it } from 'vitest' * job runs" must not produce the same green. */ -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') -const WORKFLOW_DIR = '.github/workflows' - /** * Synthetic, and only ever compared against itself: what matters is that the * fork context disagrees with `github.repository` and the same-repo one agrees. @@ -348,17 +342,6 @@ const CONTEXTS = { // Discovery // --------------------------------------------------------------------------- -function workflowFiles() { - return readdirSync(join(REPO_ROOT, WORKFLOW_DIR)) - .filter((name) => /\.ya?ml$/.test(name)) - .map((name) => `${WORKFLOW_DIR}/${name}`) - .sort() -} - -function readWorkflow(relPath) { - return yaml.load(readFileSync(join(REPO_ROOT, relPath), 'utf8')) -} - /** * `on:` parses as the boolean `true` under YAML 1.1 (the "Norway problem"), * hence the two keys. `workflow_dispatch:` with no value parses as null, so diff --git a/scripts/__tests__/workflow-node-gyp.test.mjs b/scripts/__tests__/workflow-node-gyp.test.mjs index 08a8220cb..3cb65bb2b 100644 --- a/scripts/__tests__/workflow-node-gyp.test.mjs +++ b/scripts/__tests__/workflow-node-gyp.test.mjs @@ -1,8 +1,8 @@ -import { existsSync, readdirSync, readFileSync } from 'node:fs' -import { join, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' -import yaml from 'js-yaml' +import { existsSync } from 'node:fs' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' +import { REPO_ROOT } from './lib/repo-root.mjs' +import { readWorkflow, workflowFiles } from './lib/workflows.mjs' /** * Every job that runs `pnpm install` on a Linux runner must install `node-gyp` @@ -48,9 +48,6 @@ import { describe, expect, it } from 'vitest' * "looked and found nothing" must not produce the same green. */ -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') -const WORKFLOW_DIR = '.github/workflows' - /** * The JOBS that run a workspace install today. This is NOT the list the checks * iterate — those scan the directory — it is the guard on the scan. @@ -273,17 +270,6 @@ export function auditFlattened(flat) { // The repo scan // --------------------------------------------------------------------------- -function workflowFiles() { - return readdirSync(join(REPO_ROOT, WORKFLOW_DIR)) - .filter((name) => /\.ya?ml$/.test(name)) - .map((name) => `${WORKFLOW_DIR}/${name}`) - .sort() -} - -function readYaml(relPath) { - return yaml.load(readFileSync(join(REPO_ROOT, relPath), 'utf8')) -} - /** * GitHub resolves `uses: ./x` against the checkout root, not the calling file. * Both manifest spellings are valid, so probing only `action.yml` would stop @@ -294,7 +280,7 @@ function resolveRepoComposite(usesPath) { for (const name of ['action.yml', 'action.yaml']) { const rel = `${dir}/${name}` if (existsSync(join(REPO_ROOT, rel))) { - const doc = readYaml(rel) + const doc = readWorkflow(rel) return { id: rel, steps: doc?.runs?.steps ?? [] } } } @@ -304,7 +290,7 @@ function resolveRepoComposite(usesPath) { function scan() { const jobs = [] for (const relPath of workflowFiles()) { - const wf = readYaml(relPath) + const wf = readWorkflow(relPath) for (const [jobName, job] of Object.entries(wf?.jobs ?? {})) { const { flat, unresolved } = flattenSteps( job?.steps, diff --git a/scripts/__tests__/workflow-paths-filter-parity.test.mjs b/scripts/__tests__/workflow-paths-filter-parity.test.mjs new file mode 100644 index 000000000..1da305263 --- /dev/null +++ b/scripts/__tests__/workflow-paths-filter-parity.test.mjs @@ -0,0 +1,159 @@ +import { describe, expect, it } from 'vitest' +import { readWorkflow, workflowFiles } from './lib/workflows.mjs' + +/** + * GitHub Actions has no YAML anchors, so a workflow that wants the same + * `paths:` filter on `push` and `pull_request` has to write the list twice. + * Nothing makes the copies agree, and a one-sided edit is silent in the worst + * direction: drop an entry from the `pull_request` copy and the workflow keeps + * running on `main`, keeps reporting green, and stops running on the pull + * request — which is the only run that could have caught the change before it + * landed. The reverse edit is loud by comparison; you notice a job that never + * runs on main. + * + * This invariant is NOT integration-specific, and for a long time the check was. + * It lived in `integration-workflow-paths.test.mjs`, whose discovery selects + * workflows by their `CS_IT_SUITE` globs — two files today. The other seven + * workflows that write the list twice were covered by nothing, including the + * two prisma e2e workflows whose own comments (until this file existed) claimed + * otherwise. Generalising it costs nothing: all nine were already identical, so + * the widened check went green on the first run and the coverage roughly + * quadrupled. + * + * The comparison is on the PARSED lists, not the text. Comments between entries, + * ordering of unrelated keys and quoting style are all free to differ; only the + * sequence of path globs has to match, because only that is what GitHub reads. + * + * THE ASYMMETRY CASE is the interesting half. A workflow declaring `paths:` on + * one event and not the other has no second copy to compare, so it drops out of + * the parity check entirely — and "drops out of the check" is exactly what a + * one-sided DELETION looks like. `tests-rust.yml` is a legitimate instance and + * therefore the reason the expected-asymmetry map below is not just an + * exemption but a fixed list: a tenth workflow arriving in that state has to be + * justified in writing rather than quietly stop being checked. + */ + +/** The two events whose filters are written out separately. */ +const FILTERED_EVENTS = ['push', 'pull_request'] + +/** + * A workflow's `paths:` filters, per event, `null` where the event declares + * none. + * + * `on:` parses as the boolean `true` under YAML 1.1 — the "Norway problem" — + * hence `wf.on ?? wf[true]`. An event key present with no `paths:` (a bare + * `pull_request:`, or `push: { branches: [main] }`) is `null` here and means + * "unfiltered", which is a different thing from an empty list and a very + * different thing from a filter that lost an entry. + */ +function pathsFilters(relPath) { + const wf = readWorkflow(relPath) + const on = wf?.on ?? wf?.[true] + return Object.fromEntries( + FILTERED_EVENTS.map((event) => [ + event, + Array.isArray(on?.[event]?.paths) ? on[event].paths : null, + ]), + ) +} + +const CLASSIFIED = workflowFiles().map((relPath) => { + const filters = pathsFilters(relPath) + const declared = FILTERED_EVENTS.filter((event) => filters[event] !== null) + return { relPath, filters, declared } +}) + +/** Workflows writing the list twice — the ones the parity check applies to. */ +const PARITY = CLASSIFIED.filter((entry) => entry.declared.length === 2) + +/** Workflows filtering one event and not the other. */ +const ASYMMETRIC = CLASSIFIED.filter((entry) => entry.declared.length === 1) + +/** + * The guard on the scan, in the shape the rest of this directory uses. Every + * assertion below is "these two lists agree", and a scan that finds no + * workflows satisfies that for free. + * + * Held as a minimum: a new workflow that filters both events must not fail + * this. A workflow LEAVING the list is the event worth interrupting for — + * either it stopped filtering both events (see the asymmetry check, which will + * also fire) or the discovery broke. + */ +const EXPECTED_PARITY_WORKFLOWS = [ + '.github/workflows/fta-v3.yml', + '.github/workflows/integration-drizzle.yml', + '.github/workflows/integration-prisma-next.yml', + '.github/workflows/integration-protect-ffi.yml', + '.github/workflows/integration-supabase.yml', + '.github/workflows/prisma-example-readme-e2e.yml', + '.github/workflows/prisma-next-e2e.yml', + '.github/workflows/tests-bench.yml', + '.github/workflows/tests-supply-chain.yml', +] + +/** + * Workflows that deliberately filter one event and not the other, each with the + * reason. An entry here is a workflow the parity check cannot see, so the + * reason has to explain why the drift it guards against is not possible. + */ +const EXPECTED_ASYMMETRIES = new Map([ + [ + '.github/workflows/tests-rust.yml', + // `push:` is `branches: [main]` with no `paths:`, so every push to main runs + // the Rust checks and only pull requests are filtered. There is no second + // list to drift from, and the direction that would hurt — a `push` filter + // narrower than the `pull_request` one, letting a change land on main + // unchecked — is unreachable when `push` is unfiltered. + 'push is deliberately unfiltered (branches: [main]), so main runs a superset of what PRs run', + ], +]) + +describe('paths filters are written twice, identically', () => { + it('finds the workflows that filter both events', () => { + const found = PARITY.map((entry) => entry.relPath) + const missing = EXPECTED_PARITY_WORKFLOWS.filter( + (relPath) => !found.includes(relPath), + ) + expect( + missing, + `These workflows declared \`paths:\` under both \`push\` and \`pull_request\`, and the scan no longer sees it. A workflow that stops filtering both events stops being parity-checked — which is indistinguishable from a one-sided deletion, the exact edit this file exists to catch. Either justify the change in EXPECTED_ASYMMETRIES, or restore the filter. Found:\n${found.map((relPath) => ` ${relPath}`).join('\n')}`, + ).toEqual([]) + }) + + it('accounts for every workflow that filters only one event', () => { + const unexplained = ASYMMETRIC.filter( + (entry) => !EXPECTED_ASYMMETRIES.has(entry.relPath), + ).map( + (entry) => + ` ${entry.relPath} filters \`${entry.declared[0]}\` only (${entry.filters[entry.declared[0]].length} entries)`, + ) + expect( + unexplained, + `These workflows filter one event and not the other, so nothing compares their filters — and a workflow arrives in that state either deliberately or because someone deleted one copy.\nIf it is deliberate, add it to EXPECTED_ASYMMETRIES with the reason the drift this file guards against cannot happen there.\n${unexplained.join('\n')}`, + ).toEqual([]) + }) + + it('keeps no stale asymmetry exemptions', () => { + // The mirror of the check above, and the one that matters when a workflow + // is FIXED: an entry that no longer describes an asymmetry sits there + // reading as deliberate while exempting nothing, and hides the next one. + const asymmetric = ASYMMETRIC.map((entry) => entry.relPath) + const stale = [...EXPECTED_ASYMMETRIES.keys()].filter( + (relPath) => !asymmetric.includes(relPath), + ) + expect( + stale, + 'These EXPECTED_ASYMMETRIES entries no longer match a workflow that filters exactly one event. If the workflow gained the second filter, delete the entry — it now belongs to the parity check above. If it was renamed or removed, fix the key.', + ).toEqual([]) + }) + + for (const { relPath } of PARITY) { + it(`${relPath} filters push and pull_request identically`, () => { + const filters = pathsFilters(relPath) + expect( + filters.pull_request, + `The \`push\` and \`pull_request\` \`paths:\` filters in ${relPath} have diverged. GitHub Actions has no YAML anchors, so the list is written twice and nothing but this check keeps the copies together.\nA \`pull_request\` filter narrower than the \`push\` one is the silent direction: the workflow still runs on main and still reports green, while the PR that introduces the change no longer runs it at all.`, + ).toEqual(filters.push) + }) + } +}) diff --git a/scripts/__tests__/workflow-trigger-comments.test.mjs b/scripts/__tests__/workflow-trigger-comments.test.mjs new file mode 100644 index 000000000..8ac3d00e3 --- /dev/null +++ b/scripts/__tests__/workflow-trigger-comments.test.mjs @@ -0,0 +1,278 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import yaml from 'js-yaml' +import { describe, expect, it } from 'vitest' +import { REPO_ROOT } from './lib/repo-root.mjs' +import { readWorkflow, workflowFiles } from './lib/workflows.mjs' + +/** + * A CI comment that describes WHEN a step runs is load-bearing documentation, + * and it is the one kind of documentation nothing in this repo checks. The YAML + * around it is validated six ways — `integration-workflow-paths.test.mjs` + * compares the two copies of every `paths:` filter, + * `ffi-binding-step-order.test.mjs` checks step order and coverage, + * `workflow-dispatch-job-conditions.test.mjs` checks the fork gate — while the + * prose explaining the filter to the next reader is free to say the opposite of + * what the filter does. + * + * It did. Both prisma workflows carried "the step itself is only ever exercised + * by the push-to-main run", written when the protect-ffi inputs were about to be + * added to the `push` filter, and left in place after they were added to the + * `pull_request` filter as well. A PR touching `packages/protect-ffi/src/**` has + * run both workflows since the day those entries landed. The comment told you it + * could not — which is worse than no comment, because the next person reasoning + * about protect-ffi coverage on PRs reads it and concludes there is none, and + * either adds a redundant job or drops a filter entry to "make it consistent". + * + * So: the invariant here is agreement between the filter and the prose, DERIVED + * from the parsed `on:` block rather than asserted against a fixed sentence. A + * workflow that a protect-ffi change can trigger on `pull_request` may not + * contain a comment that names `push to main` as the trigger and stops there. If + * the filter loses its protect-ffi entries tomorrow, the workflow drops out of + * scope and the push-only sentence becomes legal again — which is the property + * you want, and the one a "these files must contain this string" check cannot + * express. + * + * WHAT THIS DOES NOT COVER, deliberately: + * + * - The fork-PR caveat. Both jobs are gated on + * `github.event.pull_request.head.repo.full_name == github.repository`, so a + * fork PR triggers the workflow and then skips the job for want of CS_* + * credentials. The comments say so in prose; requiring the word "fork" in + * every comment that mentions a PR trigger would fail comments that had no + * business discussing forks. + * - "Path-filtered away from the absorption PR", the other false half of the + * same two comments — false because each workflow lists its own path, the + * build action, and the protect-ffi package in its own filter, and the + * absorption branch edits all three. Checking it mechanically would mean + * knowing what a particular PR touches, which a test in the tree cannot. + * - Trailing inline comments (`key: value # note`). Only full-line comment + * blocks are scanned, because a `#` inside a quoted YAML scalar is not a + * comment and telling the two apart needs a real parser. Every long-form + * explanation in the workflow directory is a full-line block today; if one of + * these claims ever shows up inline, this will not see it. + */ + +const BUILD_FFI_ACTION = '.github/actions/build-ffi-binding' + +/** + * The files that, when edited, change what the binding build produces — + * read out of the action's own cache keys rather than listed here. + * + * `build-ffi-binding` caches `index.node` and `dist/wasm` on `hashFiles(...)` + * of its inputs, so those globs ARE the definition of "a change to the + * binding": anything in them must miss the cache and recompile. Reusing them + * means this scope follows the action. Add a build input to the key and the set + * of workflows held to the invariant below grows with it; the alternative — a + * hardcoded `packages/protect-ffi/**` — would keep passing while quietly + * describing a different package than the one CI builds. + * + * The action's own directory is in the set for the same reason it is in both + * workflows' `paths:` filters: editing the action changes the build. + */ +function bindingBuildInputs() { + const action = yaml.load( + readFileSync(join(REPO_ROOT, `${BUILD_FFI_ACTION}/action.yml`), 'utf8'), + ) + const inputs = new Set([`${BUILD_FFI_ACTION}/**`]) + for (const step of action?.runs?.steps ?? []) { + for (const call of String(step?.with?.key ?? '').matchAll( + /hashFiles\(([^)]*)\)/g, + )) { + for (const arg of call[1].matchAll(/'([^']*)'/g)) inputs.add(arg[1]) + } + } + return [...inputs] +} + +const BINDING_INPUTS = bindingBuildInputs() + +/** + * Do a `paths:` entry and a build input describe overlapping trees? + * + * Overlap in EITHER direction, because both sides are globs and either can be + * the broader one: the filter may say `packages/protect-ffi/src/**` against an + * input file, or `packages/protect-ffi/crates/protect-ffi/**` against the + * input glob `packages/protect-ffi/crates/**`. Both mean a change to the + * binding can trigger the workflow, which is the only question being asked. + */ +function overlaps(entry, input) { + const literal = entry.replace(/\*\*$/, '').replace(/\/$/, '') + const target = input.replace(/\*\*$/, '').replace(/\/$/, '') + return ( + literal === target || + target.startsWith(`${literal}/`) || + literal.startsWith(`${target}/`) + ) +} + +/** + * How a protect-ffi change reaches this workflow through `pull_request`: + * `'unfiltered'` (no `paths:`, so every PR runs it), `'filtered'` (a `paths:` + * entry overlaps a binding input), or `false` (it cannot). + * + * `on:` parses as the boolean `true` under YAML 1.1 — the "Norway problem" — + * hence `wf.on ?? wf[true]`. And `Object.hasOwn` rather than a truthiness test: + * a bare `pull_request:` with no body parses to `null` and triggers on + * everything, which is the opposite of what `!pr` would conclude. + */ +function pullRequestReach(wf) { + const on = wf.on ?? wf[true] + if (!on || !Object.hasOwn(on, 'pull_request')) return false + const paths = on.pull_request?.paths + if (!Array.isArray(paths) || paths.length === 0) return 'unfiltered' + const reached = paths.some((entry) => + BINDING_INPUTS.some((input) => overlaps(entry, input)), + ) + return reached ? 'filtered' : false +} + +/** + * Full-line comment blocks, as `{ startLine, text }`. Consecutive `#` lines are + * one block — a bare `#` separator line included, so a paragraph stays whole and + * a sentence split across two lines is matched as one string rather than two + * halves that each match nothing. + */ +function commentBlocks(relPath) { + const lines = readFileSync(join(REPO_ROOT, relPath), 'utf8').split('\n') + const blocks = [] + let current = null + for (const [index, line] of lines.entries()) { + if (line.trim().startsWith('#')) { + if (!current) { + current = { startLine: index + 1, parts: [] } + blocks.push(current) + } + current.parts.push(line.trim().replace(/^#+\s?/, '')) + } else { + current = null + } + } + return blocks.map(({ startLine, parts }) => ({ + startLine, + text: parts.join(' ').replace(/\s+/g, ' ').trim(), + })) +} + +/** Names the push-to-main trigger: "push to main", "push-to-main run". */ +const NAMES_PUSH_TRIGGER = /push[\s-]+to[\s-]+main/i + +/** + * Names the pull_request trigger too. Several forms, because the point is to + * recognise the acknowledgement rather than to mandate a phrasing — and one + * form it must NOT accept is a bare mention of some PR as a noun. "the + * absorption PR that introduced the need" is not a statement about when the + * step runs, and the comment that contained it was exactly the one claiming + * push-to-main exclusivity in the same breath. + */ +const NAMES_PR_TRIGGER = [ + /\bpull_request\b/, + /\bon (?:any|every|each|all|a) [^.]{0,60}?\bPRs?\b/i, + /\bon (?:any|every|each|all) [^.]{0,60}?pull requests?\b/i, + /\bPRs?\b[^.]{0,40}?(?:touching|touches|matching|match(?:es)? the|whose diff)/i, +] + +const IN_SCOPE = workflowFiles().filter( + (relPath) => pullRequestReach(readWorkflow(relPath)) !== false, +) + +/** Per in-scope workflow, the comment blocks that name the push trigger. */ +const CLAIMS = new Map( + IN_SCOPE.map((relPath) => [ + relPath, + commentBlocks(relPath).filter((block) => + NAMES_PUSH_TRIGGER.test(block.text), + ), + ]), +) + +/** + * The guard on the scan, in the shape the rest of this directory uses (see + * `EXPECTED_PAIRED_JOBS` in `ffi-binding-step-order.test.mjs`, and the + * scan-found-something assertions in `ffi-binding-action.test.mjs`). Every + * check below is "no comment in this set violates X", and an empty set + * satisfies that for free. + * + * Held as a minimum: a new workflow that a protect-ffi change can trigger on a + * PR must not fail this. If one genuinely stops being reachable that way, + * update the list deliberately — the entry disappearing is the same event as + * the coverage disappearing. + */ +const EXPECTED_IN_SCOPE = [ + '.github/workflows/integration-drizzle.yml', + '.github/workflows/integration-prisma-next.yml', + '.github/workflows/integration-protect-ffi.yml', + '.github/workflows/integration-supabase.yml', + '.github/workflows/prisma-example-readme-e2e.yml', + '.github/workflows/prisma-next-e2e.yml', + '.github/workflows/tests-rust.yml', + '.github/workflows/tests.yml', +] + +/** + * The workflows that today explain their trigger in prose naming push-to-main. + * The second half of the guard, and the one that catches a broken scan rather + * than a broken scope: `commentBlocks` returning nothing, or + * `NAMES_PUSH_TRIGGER` stopping matching, empties `CLAIMS` for every file and + * the per-file checks below stop being generated — silently, since a `describe` + * that produces no `it` reports success. + * + * If a comment here is deleted or reworded off the push trigger, that is fine + * and this needs a deliberate edit. It is not fine for all of them to vanish at + * once, which is what a regex or parser regression looks like. + */ +const EXPECTED_CLAIM_FILES = [ + '.github/workflows/prisma-example-readme-e2e.yml', + '.github/workflows/prisma-next-e2e.yml', +] + +describe('workflow trigger comments match the trigger', () => { + it('derives the binding build inputs from the action manifest', () => { + expect( + BINDING_INPUTS, + `No \`hashFiles(...)\` globs were parsed out of ${BUILD_FFI_ACTION}/action.yml, so "a change to the protect-ffi binding" resolves to nothing and every workflow falls out of scope. Either the cache steps moved (follow them), or the key syntax changed (update bindingBuildInputs).`, + ).toContain('packages/protect-ffi/crates/**') + expect(BINDING_INPUTS).toContain(`${BUILD_FFI_ACTION}/**`) + }) + + it('finds the workflows a protect-ffi change can trigger on a PR', () => { + const missing = EXPECTED_IN_SCOPE.filter((file) => !IN_SCOPE.includes(file)) + expect( + missing, + `A protect-ffi change used to reach these workflows through \`on.pull_request\`, and the scan no longer sees it. Either a \`paths:\` filter dropped its protect-ffi entries — in which case the coverage went with them — or \`pullRequestReach\` needs teaching about a new filter shape. Found:\n${IN_SCOPE.map((file) => ` ${file}`).join('\n')}`, + ).toEqual([]) + }) + + it('finds the trigger-claim comments it means to inspect', () => { + const missing = EXPECTED_CLAIM_FILES.filter( + (file) => (CLAIMS.get(file) ?? []).length === 0, + ) + expect( + missing, + `These workflows explained their trigger in a comment naming push-to-main, and the scan no longer finds one. If the comments were deliberately reworded, update EXPECTED_CLAIM_FILES; if not, \`commentBlocks\` or NAMES_PUSH_TRIGGER has stopped working and the per-file checks below are no longer being generated at all.`, + ).toEqual([]) + }) + + for (const relPath of IN_SCOPE) { + const blocks = CLAIMS.get(relPath) ?? [] + if (blocks.length === 0) continue + + it(`${relPath} does not describe the trigger as push-to-main only`, () => { + const reach = pullRequestReach(readWorkflow(relPath)) + const offenders = blocks + .filter( + (block) => !NAMES_PR_TRIGGER.some((form) => form.test(block.text)), + ) + .map((block) => ` line ${block.startLine}: ${block.text}`) + + expect( + offenders, + `These comments in ${relPath} name \`push\` to main as the trigger and never mention \`pull_request\`, but a change to the protect-ffi binding reaches this workflow on a pull request too — ${ + reach === 'unfiltered' + ? 'it has no `paths:` filter on `pull_request`, so every PR runs it' + : 'its `pull_request` `paths:` filter carries the binding inputs' + }. Reword them to name both events; a comment that under-states when a step runs sends the next reader looking for coverage that is already there.\n${offenders.join('\n')}`, + ).toEqual([]) + }) + } +}) diff --git a/scripts/__tests__/workflow-turbo-build-deps.test.mjs b/scripts/__tests__/workflow-turbo-build-deps.test.mjs index 50261d4be..5f3cfad28 100644 --- a/scripts/__tests__/workflow-turbo-build-deps.test.mjs +++ b/scripts/__tests__/workflow-turbo-build-deps.test.mjs @@ -1,9 +1,9 @@ -import { readdirSync, readFileSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { resolve } from 'node:path' -import { fileURLToPath } from 'node:url' -import yaml from 'js-yaml' import { describe, expect, it } from 'vitest' import { readJsonc } from './lib/read-jsonc.mjs' +import { REPO_ROOT } from './lib/repo-root.mjs' +import { readWorkflow, workflowFiles } from './lib/workflows.mjs' /** * A turbo task declaring `dependsOn: ["^build"]` gets its workspace @@ -27,8 +27,6 @@ import { readJsonc } from './lib/read-jsonc.mjs' * This test pins that routing so it cannot be quietly "simplified" back. */ -const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') - /** * The workflow that must carry the `typecheck:scaffold` step specifically. * Scoped, because "that step exists" is a claim about this file, not about @@ -44,10 +42,7 @@ const SCAFFOLD_WORKFLOW = '.github/workflows/tests.yml' * Narrowing the guard to one file left five real bare invocations unchecked * (#787 review follow-up). */ -const WORKFLOWS = readdirSync(resolve(REPO_ROOT, '.github/workflows')) - .filter((file) => /\.ya?ml$/.test(file)) - .map((file) => `.github/workflows/${file}`) - .sort() +const WORKFLOWS = workflowFiles() /** * Bare invocations that predate this guard. Each is the same latent trap: it @@ -118,7 +113,7 @@ const rootScriptDelegatesToTurbo = (task) => typeof rootScripts[task] === 'string' && /\bturbo\b/.test(rootScripts[task]) function workflowRunLines(path) { - const doc = yaml.load(readFileSync(resolve(REPO_ROOT, path), 'utf8')) + const doc = readWorkflow(path) const lines = [] for (const [jobName, job] of Object.entries(doc?.jobs ?? {})) { for (const step of job?.steps ?? []) { From a8e1948ba5271ab250c3444fa728c5556027c779 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 6 Aug 2026 13:29:49 +1000 Subject: [PATCH 4/4] fix(protect-ffi): keep the default test path free of the cargo artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .github/workflows/tests.yml | 24 ++- packages/protect-ffi/src/lintWiring.test.ts | 195 +++++++++++++++++- .../protect-ffi/src/nativeLoading.test.ts | 188 +++++++++++++++-- 3 files changed, 382 insertions(+), 25 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index c72225649..77ad95222 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -108,14 +108,22 @@ jobs: client-key: ${{ secrets.CS_CLIENT_KEY }} client-access-key: ${{ secrets.CS_CLIENT_ACCESS_KEY }} - # Vendoring protect-ffi put it in `turbo test --filter './packages/*'`, - # so root `pnpm test` now runs ITS suite too — and - # `src/nativeLoading.test.ts` asserts the platform binary loads. Nothing - # else in this job produces `index.node`. The type steps below need - # `lib/` for the same reason: they call package scripts directly, so - # turbo's `^build` never runs and protect-ffi's declarations resolve to - # nothing. No `wasm: true` — every `wasm-inline` unit test either mocks - # the module or asserts on the bundle graph. + # `pnpm run test` below runs the `packages/stack` suites, and only 8 of + # those 120 files mock `@cipherstash/protect-ffi` — the rest load + # `index.node`, which nothing else in this job produces. + # + # NOT for protect-ffi's own suite, though vendoring it did add + # `@cipherstash/protect-ffi#test` to `turbo test --filter './packages/*'`. + # That suite must pass with no binding at all (its `build` is `tsc` and + # its `test` is deliberately cargo-free), and `src/lintWiring.test.ts` + # re-runs it here with the artifact hidden to keep that true — this job + # having a binding is exactly why that nested run is the only place the + # artifact-free path gets exercised in CI. + # + # The type steps below need `lib/`: they call package scripts directly, + # so turbo's `^build` never runs and protect-ffi's declarations resolve + # to nothing. No `wasm: true` — every `wasm-inline` unit test either + # mocks the module or asserts on the bundle graph. - name: Build the protect-ffi binding uses: ./.github/actions/build-ffi-binding diff --git a/packages/protect-ffi/src/lintWiring.test.ts b/packages/protect-ffi/src/lintWiring.test.ts index 34c7d0ba2..38cceb68b 100644 --- a/packages/protect-ffi/src/lintWiring.test.ts +++ b/packages/protect-ffi/src/lintWiring.test.ts @@ -9,9 +9,26 @@ * So the property under test is reachability: a check that nothing invokes is * the failure, and it is invisible by construction. Every exemption below has * to name why. + * + * One test here is not static: `keeps cargo OUTPUT off the default test path + * too` re-runs this suite in a child process with the compiled binding made + * unresolvable. It sits with the wiring checks because it guards the same rule + * as `keeps cargo off the default test path` — the entry-point split — and + * because the half it covers cannot be read off the manifest: a test that + * requires `index.node` makes cargo a prerequisite of root `pnpm test` without + * a `cargo` token appearing anywhere in package.json. */ -import { existsSync, readdirSync, readFileSync } from 'node:fs' -import { join } from 'node:path' +import { spawnSync } from 'node:child_process' +import { + existsSync, + mkdtempSync, + readdirSync, + readFileSync, + writeFileSync, +} from 'node:fs' +import { createRequire } from 'node:module' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' import { describe, expect, it } from 'vitest' // Vitest resolves cwd to the directory holding vitest.config.ts. `import.meta` @@ -193,6 +210,81 @@ const ENTRY_POINT_EXEMPT: Record = { 'test:typecheck:wasm': 'needs dist/wasm, run by the root wasm-e2e job', } +/** + * Set on the nested `vitest run` the artifact guard below spawns, so the guard + * does not spawn itself forever. + * + * It is the ONE place in this file a test is allowed to disappear, and it is a + * bounded one: the only process that ever carries this variable is one the + * guard itself started, and the guard then asserts on that process's exit + * status. The skip is reported by the assertion that caused it. + */ +const ARTIFACT_FREE_RUN = 'PROTECT_FFI_ARTIFACT_FREE_RUN' + +/** + * A `--require` preload that removes every cargo-built binding from the child's + * world: unresolvable through `require`, and absent from `fs`. + * + * BOTH halves are needed and they are not redundant. `Module._load` is what + * `@neon-rs/load`'s proxy goes through — for the six platform packages and for + * the `../index.node` debug fallback alike — so patching it simulates the + * missing binary. But `nativeLoading.test.ts` decides which case it is in by + * looking at the DISK, and it has to: an env var the test consults would be a + * back door that silently disables the check for anyone who exports it. So the + * filesystem has to agree with the loader, or the child fails the wrong way — + * loudly, but for the wrong reason, on precisely the machines this guard exists + * to serve. + * + * `syncBuiltinESMExports` is what makes the fs patch visible to + * `import { existsSync } from 'node:fs'`. Without it the named ESM export stays + * bound to the original function and only `require('node:fs').existsSync` sees + * the patch — a half-applied hook, which reads as a working one. + * + * The pid log is the proof that the preload reached the test WORKERS and not + * just the vitest process that spawns them. `--require` is per-process: forks + * inherit it through the environment, worker_threads would not. Vitest's + * default pool is forks today, and if that ever changes this guard would run + * against unhooked workers and pass by proving nothing — so the parent counts + * the pids rather than trusting the pool. + */ +function preloadSource(marker: string): string { + return ` +const fs = require('node:fs') +const Module = require('node:module') + +// The package root's \`index.node\`, a \`platforms//index.node\`, and the +// bare specifier of a platform package — every shape \`src/load.cts\` reaches for. +const BINDING = + /(?:^|[\\\\/])index\\.node$|@cipherstash[\\\\/]protect-ffi-(?:darwin|linux|win32)-/ + +fs.appendFileSync(${JSON.stringify(marker)}, process.pid + '\\n') + +const load = Module._load +Module._load = function (request, parent, isMain) { + if (BINDING.test(request)) { + // Shaped like the real thing: \`code\` is what packages/cli keys on. + const error = new Error("Cannot find module '" + request + "'") + error.code = 'MODULE_NOT_FOUND' + throw error + } + return load.call(this, request, parent, isMain) +} + +const existsSync = fs.existsSync +fs.existsSync = (path) => (BINDING.test(String(path)) ? false : existsSync(path)) + +const statSync = fs.statSync +fs.statSync = (path, ...rest) => { + if (!BINDING.test(String(path))) return statSync(path, ...rest) + const error = new Error('ENOENT: no such file or directory, stat ' + path) + error.code = 'ENOENT' + throw error +} + +Module.syncBuiltinESMExports() +` +} + describe('lint and format wiring', () => { it('reads the files it means to read', () => { // Everything below asserts on file contents resolved from cwd. A wrong cwd @@ -335,6 +427,105 @@ describe('lint and format wiring', () => { expect([...cargoScripts, ...viaMise]).toEqual([]) }) + it.skipIf(process.env[ARTIFACT_FREE_RUN] === '1')( + 'keeps cargo OUTPUT off the default test path too', + () => { + // The other half of the rule the test above states, and the half nothing + // was checking. Keeping cargo out of the SCRIPTS is worth nothing if a + // test then requires what cargo produces: `src/nativeLoading.test.ts` + // asserted `assertNativeBindingAvailable()` does not throw, which needs + // an `index.node` that only `build:native` writes. Root `pnpm test` + // reaches this package through `turbo test --filter './packages/*'`, so + // that made a Rust build a prerequisite of the whole repo's default test + // — the exact thing the entry-point split exists to prevent, arriving + // through the tests instead of through the scripts. + // + // Static analysis cannot see this. A test does not name `index.node`; it + // calls an export that happens to reach the addon, four hops down. So + // the suite is RE-RUN with every binding artifact made unresolvable, and + // the property is the child's exit status. + // + // Which is also why it is not enough that this checkout currently has no + // binary. The failure is invisible exactly where it is introduced: an + // author who ran `build:native` sees green, and so does CI, which builds + // the binding before running this suite. Only the artifact-free + // contributor sees it — and by then it is on main. In CI this nested run + // is the ONLY execution of the artifact-free path. + const workspace = mkdtempSync(join(tmpdir(), 'protect-ffi-no-artifact-')) + const marker = join(workspace, 'preloaded-pids') + const preload = join(workspace, 'hide-binding.cjs') + writeFileSync(marker, '') + writeFileSync(preload, preloadSource(marker)) + + // Resolved through the manifest rather than assumed: `vitest/vitest.mjs` + // is the `bin` entry, and pnpm's store path is not guessable. + const resolve = createRequire(join(repoRoot, 'package.json')) + const manifestPath = resolve.resolve('vitest/package.json') + const vitestBin = join( + dirname(manifestPath), + JSON.parse(readFileSync(manifestPath, 'utf8')).bin.vitest, + ) + + const env: Record = { + ...process.env, + [ARTIFACT_FREE_RUN]: '1', + // The summary line is asserted on below, and vitest wraps every field + // of it in colour escapes when it thinks it has a TTY-ish consumer. + NO_COLOR: '1', + NODE_OPTIONS: [process.env.NODE_OPTIONS, `--require ${preload}`] + .filter(Boolean) + .join(' '), + } + // This process IS a vitest worker, and its VITEST_* variables describe + // that worker. Inherited, they make the child think it is one too. + for (const key of Object.keys(env)) { + if (key.startsWith('VITEST')) delete env[key] + } + + const child = spawnSync(process.execPath, [vitestBin, 'run'], { + cwd: repoRoot, + env, + encoding: 'utf8', + timeout: 300_000, + }) + const output = `${child.stdout ?? ''}\n${child.stderr ?? ''}` + + expect( + child.error, + `Could not start the nested vitest run at ${vitestBin}.`, + ).toBeUndefined() + + expect( + child.status, + `The default test suite does not survive a checkout with no cargo build.\n\`packages/protect-ffi\`'s \`test\` is what root \`pnpm test\` runs through turbo, and it must pass with no \`index.node\` anywhere — the six \`platforms/*\` packages are empty until someone compiles one, and \`build\` is \`tsc\`, not cargo.\nGate the assertion on the artifact being present (see \`builtArtifacts\` in nativeLoading.test.ts) and give the artifact-free case its own contract, rather than making a Rust toolchain a prerequisite of the repo's default test.\nThe nested run said:\n${output}`, + ).toBe(0) + + // Non-vacuity, in two parts. A child that ran nothing exits 0 on some + // configurations, and a preload that never reached the workers leaves + // the artifact visible to them — either one turns this green while + // testing nothing. + // Recursive, matching `vitest.config.ts`'s `src/**/*.test.ts`. A flat + // readdir agrees with it only for as long as nobody nests a test file, + // and then this fails on a correct suite. + const files = readdirSync(join(repoRoot, 'src'), { + recursive: true, + }).filter((name) => String(name).endsWith('.test.ts')) + expect( + output, + `The nested run did not report ${files.length} passing test files, so it did not run this suite.\n${output}`, + ).toContain(`Test Files ${files.length} passed (${files.length})`) + + const pids = new Set( + readFileSync(marker, 'utf8').split('\n').filter(Boolean), + ) + expect( + pids.size, + `The preload logged ${pids.size} process(es). It has to reach the vitest process AND the workers that run the test files — \`--require\` travels through the environment to forked children, but not into worker_threads. If vitest's pool is no longer fork-based, this guard ran against workers that could still see the binding.\nPids: ${[...pids].join(', ')}`, + ).toBeGreaterThan(1) + }, + 300_000, + ) + it('reaches every cargo check from the cargo entry point', () => { // The mirror of the check above: cargo scripts are allowed to exist, but // not to exist unreachable. Without this, moving a check off `test` and diff --git a/packages/protect-ffi/src/nativeLoading.test.ts b/packages/protect-ffi/src/nativeLoading.test.ts index 9bc4a7d45..1cf770047 100644 --- a/packages/protect-ffi/src/nativeLoading.test.ts +++ b/packages/protect-ffi/src/nativeLoading.test.ts @@ -11,24 +11,35 @@ * `@cipherstash/stack-prisma` reaches this package through one entry out of * fifteen. * - * A regression here is silent for anyone with a binary installed, which is - * everyone who would notice — so load timing is asserted against the EMITTED - * JavaScript rather than behaviour. `lib/` exists by the time this runs: - * `test:typecheck` emits it before `test:unit`. + * A regression here is silent for anyone with a binary installed — every + * consumer who installs from npm, and every CI job that has run + * `.github/actions/build-ffi-binding` — so the property is asserted against the + * EMITTED JavaScript rather than behaviour. `lib/` exists by the time this + * runs: `test:typecheck` emits it before `test:unit`. * * The same blind spot is why the last block goes the other way and asserts * behaviour: what `assertNativeBindingAvailable` does with a MISSING binary * cannot be read off the emit, so that block substitutes a failing loader * under the emitted entry instead of substituting an installation. + * + * Nothing in this file may REQUIRE a binary, though, and that is a separate + * rule from the ones above. `packages/protect-ffi`'s `test` is the default task + * root `pnpm test` reaches through `turbo test --filter './packages/*'`, and it + * is deliberately Rust-free: `index.node` stopped being tarball content when + * this package was absorbed, so on a fresh checkout there is no binary + * anywhere and the six `platforms/*` links are empty. `lintWiring.test.ts` + * guards that rule for the whole suite by re-running it with the artifact made + * unresolvable. */ -import { existsSync, readFileSync, statSync } from 'node:fs' +import { existsSync, readdirSync, readFileSync, statSync } from 'node:fs' import Module, { createRequire } from 'node:module' import { join } from 'node:path' import { describe, expect, it } from 'vitest' // Vitest resolves cwd to the directory holding vitest.config.ts. -const entryPath = join(process.cwd(), 'lib/index.cjs') +const packageRoot = process.cwd() +const entryPath = join(packageRoot, 'lib/index.cjs') // `lib/` is generated, so this file carries a prerequisite the package's `test` // chain satisfies and a bare `test:unit` does not. Without this guard the whole @@ -97,27 +108,168 @@ describe('native binding load timing', () => { }) }) +/** + * Every path a build of this package leaves an `index.node` at. + * + * This is the SPLIT the two branches below turn on, and it is deliberately a + * filesystem fact rather than `process.env.CI`. `CI` is a claim about which + * machine this is; the question is whether a cargo build has happened, which is + * observable directly. `CI` also lies in both directions — it is set by `act`, + * by pre-commit wrappers, and by anyone who exports it, none of which builds a + * binding; and it says nothing about a developer who ran `build:native` and + * whose positive case would then never be checked. + */ +function binariesUnder(dir: string): string[] { + if (!existsSync(dir)) return [] + return readdirSync(dir).map((name) => join(dir, name, 'index.node')) +} + +const BINDING_ARTIFACTS = [ + // `build:native` → the `postcargo-build` hook → a bare `neon dist`, which + // writes it at the package ROOT. That is the `debug:` fallback registered in + // `src/load.cts`, and it is what `.github/actions/build-ffi-binding` + // produces — CI never populates a platform package. + join(packageRoot, 'index.node'), + // `neon dist -o platforms/` — release packaging, and the shape an + // `npm install` of `@cipherstash/protect-ffi-` leaves behind. In + // this workspace `node_modules/@cipherstash/protect-ffi-*` symlinks to + // `platforms/*` so the two overlap; for a consumer only the first exists. + ...binariesUnder(join(packageRoot, 'platforms')), + ...binariesUnder(join(packageRoot, 'node_modules/@cipherstash')), +] + +// Zero-byte counts as absent, matching the `lib/index.cjs` guard above: an +// interrupted `neon dist` leaves one, and treating it as present would send +// this into the branch that demands a successful load. +const builtArtifacts = BINDING_ARTIFACTS.filter( + (path) => existsSync(path) && statSync(path).size > 0, +) + +/** + * The platform-package name in a loader failure, as `packages/cli` matches it + * (`PLATFORM_PKG` in `packages/cli/src/native.ts`). + * + * Copied rather than imported — this package does not depend on the CLI, and + * should not. The coupling is the point of the assertion: `index.cts`'s doc + * comment promises the loader's error propagates UNWRAPPED so that "existing + * classification of a missing binding keeps working unchanged", and the CLI is + * the code doing that classification. Nothing checked the promise against a + * real error until now: the CLI's own suite builds its inputs by hand + * (`moduleError("Cannot find module '@cipherstash/protect-ffi-darwin-arm64'")` + * in `packages/cli/src/__tests__/native.test.ts`), so it proves the matcher + * matches a string, not that the string is what the loader raises. + */ +const PLATFORM_PACKAGE = + /@cipherstash\/[a-z0-9-]+-(?:darwin|linux|win32)-[a-z0-9-]+/i + describe('assertNativeBindingAvailable', () => { const mod = requireFromEntry(entryPath) + // Called ONCE, here, rather than inside a test: the loader memoises, so a + // second call after a first success is not a second load, and the branches + // below are two views of the same outcome rather than two attempts. + const outcome: { error: unknown } = (() => { + try { + mod.assertNativeBindingAvailable() + return { error: undefined } + } catch (error) { + return { error } + } + })() + + const errorDetail = + outcome.error instanceof Error + ? `${outcome.error.name} [${(outcome.error as NodeJS.ErrnoException).code}]: ${outcome.error.message}` + : String(outcome.error) + it('is exported from the package entry', () => { // `stash doctor` will consume this by name across a package boundary, so // its presence is the contract — see the doc comment on the function. expect(typeof mod.assertNativeBindingAvailable).toBe('function') }) - it('succeeds when the binding is present', () => { - // The positive half only — this suite runs where a binary is installed. - // The negative half (a `MODULE_NOT_FOUND` propagating unwrapped) is the - // last describe block in this file, which reproduces a missing binary by - // swapping the loader under this same emitted entry rather than by - // needing an installation without one. - expect(() => mod.assertNativeBindingAvailable()).not.toThrow() + it('loads a built binding, and fails classifiably when none is built', () => { + // This used to be a bare `expect(...).not.toThrow()` justified by "this + // suite runs where a binary is installed". That premise died with the + // absorption. `index.node` arrived prebuilt inside the npm tarball; as a + // workspace package it is a cargo output, the six `platforms/*` links are + // empty until someone builds one, and `packages/protect-ffi`'s `test` is + // deliberately Rust-free. So on every fresh checkout the assertion failed + // — and it failed under root `pnpm test`, which reaches this package via + // `turbo test --filter './packages/*'`. + // + // Both branches assert. A `skipIf` here would be the trade this repo keeps + // refusing: it goes quiet on every contributor machine, and quiet is + // indistinguishable from passing. What replaces it is that the artifact- + // free case has a contract of its OWN worth checking, and it is the one + // `stash doctor` and `reportNativeBinaryMissing` depend on. + if (builtArtifacts.length > 0) { + expect( + outcome.error, + `A binding is built at ${builtArtifacts.join(', ')}, so the loader must resolve it. It threw instead:\n ${errorDetail}\nEither the artifact is broken (wrong architecture, truncated write) or \`assertNativeBindingAvailable\` no longer calls a native export that loads cleanly — \`native.isEncrypted(null)\` is chosen because it is pure, synchronous, and validates nothing before reaching the addon.`, + ).toBeUndefined() + return + } + + expect( + outcome.error, + `No index.node exists under packages/protect-ffi, so the loader cannot succeed and this call must throw. Looked at:\n${BINDING_ARTIFACTS.map((path) => ` ${path}`).join('\n')}\nA success here means the binding came from somewhere none of those paths covers, and the check above is then gated on a list that no longer describes reality.`, + ).toBeInstanceOf(Error) + + const error = outcome.error as NodeJS.ErrnoException & { + requireStack?: string[] + } + // The contract `index.cts` documents, verified against the real thing. + expect( + error.code, + `The loader failure must reach callers unwrapped as MODULE_NOT_FOUND. \`packages/cli\`'s \`isNativeBinaryMissing\` tests \`code\` first and returns false for anything else, so a wrapped or re-thrown error turns \`stash doctor\`'s actionable "native binary missing" note back into a raw stack trace. Got: ${errorDetail}`, + ).toBe('MODULE_NOT_FOUND') + expect( + `${error.message}\n${(error.requireStack ?? []).join('\n')}`, + `The failure must name the platform package so \`packages/cli\` can tell a missing NATIVE binary from any other missing module. Got: ${errorDetail}`, + ).toMatch(PLATFORM_PACKAGE) + }) + + it('has its success path exercised by the action that builds a binding', () => { + // The other half of the split above, and what stops the artifact-free + // branch from being a silent skip in disguise. The positive case only runs + // where a binding exists, which is no contributor machine by default — so + // the claim "it is checked where the artifact is guaranteed" has to be + // mechanically checkable rather than a sentence in a comment. + // + // `.github/actions/build-ffi-binding` is that place: it ends in a `Verify + // the binding loads` step calling this function through the emitted entry, + // immediately after producing `index.node`, and + // `scripts/__tests__/ffi-binding-step-order.test.mjs` requires every + // credentialed job (`tests.yml / run-tests` among them) to run it. + // + // Cut to `runs:` before searching, the way `integrationSuiteCi.test.ts` + // cuts to `jobs:`: the action's `description:` is prose, and a description + // that mentions proving the binding loads is not a step that proves it. + // Comments inside `runs:` go for the same reason. + const action = readFileSync( + join(packageRoot, '../../.github/actions/build-ffi-binding/action.yml'), + 'utf8', + ) + const runsAt = action.search(/^runs:/m) + expect( + runsAt, + 'build-ffi-binding/action.yml has no `runs:` key, so it defines no steps and the search below would scan prose alone.', + ).toBeGreaterThan(-1) + + expect( + action.slice(runsAt).replace(/^[ \t]*#.*$/gm, ''), + 'No step in .github/actions/build-ffi-binding calls `assertNativeBindingAvailable`. That step is the only place the success path runs against a real binding: the test above takes its artifact-free branch on any checkout without one, which is every fresh checkout and every contributor who has not run `build:native`.', + ).toContain('assertNativeBindingAvailable') }) it('reaches the loader rather than short-circuiting', () => { // The whole point is that it forces resolution, and a body that returned - // early would pass both tests above while proving nothing. Asserting on + // early would pass the load test above on any machine with a binding + // while proving nothing. (Without one it is caught there — an empty body + // does not throw, and the artifact-free branch requires a throw. Which is + // the wrong way round: the machines that have a binding are the ones this + // property is invisible on.) Asserting on // the emitted body — the technique the first describe block uses, and for // the same reason: on a machine with a binary installed, the difference // between forcing the load and not is invisible at runtime. @@ -137,10 +289,16 @@ describe('assertNativeBindingAvailable', () => { * Its doc comment promises that when the platform binary is missing the * loader's error propagates **unwrapped** — same `code`, same `message` — so * that anything already classifying a missing binding keeps working. Nothing - * asserted that. The test above deferred it to a CLI missing-binary fixture + * asserted that: the test above deferred it to a CLI missing-binary fixture * that does not exist, which left a promise about error handling verified by * prose alone. * + * That test now checks the same contract on its artifact-free branch, which is + * a complement rather than a duplicate — it only runs where nothing has been + * built, so on the machines that DO have a binding (every CI job that ran + * `.github/actions/build-ffi-binding`, every developer who ran `build:native`) + * this block is the only place the missing-binary path executes at all. + * * Reproduced by swapping the loader, not the environment: the REAL emitted * `lib/index.cjs` is re-required with a stand-in for `./load.cjs` whose * platform entry requires a package Node genuinely cannot resolve. So the code