Repository navigation
Conversation
…les cargo compiles The protect-ffi binding links eql-bindings by path, so its cache key and the paths: filters of the eight workflows that build it named that crate. They named it as `packages/eql/crates/**`: every EQL crate, with its tests, docs, generated TypeScript bindings and the `v4` / `encryption` modules that only compile with the `stack-encrypt` feature, which protect-ffi never enables. The EQL v4 block ORE pull request (#1280) touched none of what the binding compiles and still started every Drizzle, Prisma Next, Supabase and protect-ffi suite. The key and the filters now list the files cargo reads: the manifest, `src/lib.rs`, `src/sql.rs`, `src/from_v2/**`, `src/v3/**` and the SQL bundle `src/sql.rs` includes. That list is derived, not remembered. A new `scripts/__tests__/lib/rust-compiled-files.mjs` follows a crate's modules from `src/lib.rs` under the features it is built with, plus its include_str! targets; for eql-bindings it agrees file for file with cargo's own dep-info. ffi-binding-action.test.mjs now fails both ways: a compiled file the key misses (a new module, a feature turned on), and a file beside the compiled crates that the key hashes and cargo does not read. The filters are held to the key as before, and wasm-build-inputs-paths-filter.test.mjs now also fails a filter entry reaching into packages/eql/crates beyond it. The reverse gap, found on the way: the Drizzle, Prisma Next, Supabase, Prisma example and bench workflows install EQL from the workspace `@cipherstash/eql` package and did not trigger on it, so an EQL release bump started none of them. They now list `packages/eql/packages/eql/**`.
tests.yml ran its credentialed jobs (Node 22 and 24, Bun, the e2e suite and the WASM / Deno smoke test, each building two native bindings) on every pull request, including ones that changed only Go, the Stack Encrypt crates or EQL's SQL. A new `changes` job now skips them unless the pull request changes a file they can read. The question is asked the safe way round. Instead of an allowlist, which skips a suite on the change that breaks it the day someone forgets a directory, the jobs run unless every changed file sits in an excluded tree. dorny/paths-filter cannot re-include a path inside one list, so the decision is two lists: `rest` (`**` minus the excluded trees) and `direct` (what the jobs do read inside them: the eql-bindings files the binding compiles, the `@cipherstash/eql` package, tests.yml itself). The checks that read the whole tree move to an ungated `repo-checks` job: the lint scripts, the release gate, `test:scripts`, and the suites that read workflow files (protect-ffi's lintWiring and integrationSuiteCi, the e2e supply-chain and skill-map checks). They ran inside `run-tests` once per Node leg; gating that job without moving them would have left a workflow-only change with none of its guards. `tests-required` summarises the run as one check a branch rule can require, since a skipped job required by name never reports. tests-workflow-gate.test.mjs holds the lists to the jobs: every job but the four ungated ones carries the gate; no code under the live roots reads an excluded path (a textual scan that resolves relative paths from the file and its package root, with each exception written down and stale-checked); the crates the auth and profile bindings compile stay selected; and the decision script is run once per event. wasm-build-inputs-paths-filter.test.mjs now reads the gate as tests.yml's effective pull_request filter, so the FFI build inputs are held against the list that actually starts the jobs.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 039a665a5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tests-required: | ||
| name: tests-required |
There was a problem hiding this comment.
When a branch rule requires this new tests-required check, a PR that changes only docs/plans/** cannot merge: the workflow-level pull_request.paths filter on lines 12–14 prevents the entire workflow from starting, so this aggregation job never reports a status. GitHub leaves required checks pending when their workflow is skipped by path filtering, which is exactly the problem this aggregation job is intended to avoid for job-level skips.
Useful? React with 👍 / 👎.
| - name: Test — lint script self-tests | ||
| run: pnpm run test:scripts |
There was a problem hiding this comment.
On a clean repo-checks runner this step now runs immediately after installation, before anything builds packages/eql/packages/eql/dist; consequently scripts/__tests__/bench-index-expressions.test.mjs:175 skips its dist/sql matches sql assertion via it.runIf(existsSync(shipped)). In the old job, preceding Turbo typechecks built the workspace EQL package before test:scripts, so the assertion validated that copy-assets.mjs had not transformed the shipped SQL. Build @cipherstash/eql before this step or make that assertion create the build itself so this packaging guard still executes in CI.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Summary
Pull requests that cannot affect the TypeScript packages were still starting the full TypeScript test matrix. The EQL v4 block ORE pull request (#1280) is the example: it changes Go, the Rust encryption engine and EQL's SQL, and it ran every Drizzle, Prisma Next, Supabase and protect-ffi integration suite plus all of
tests.yml. Each of those jobs builds native bindings, starts PostgreSQL and uses CipherStash credentials.This PR makes two of those triggers precise, and adds guards so they stay precise without becoming unsafe.
Changes
The FFI binding's cache key and the workflows that copy it (first commit)
eql-bindingscrate from this repo. Its cache key and eight workflow filters named that crate aspackages/eql/crates/**. That covered every EQL crate, plus tests, docs, generated files, and the EQL v4 modules that only compile with a feature protect-ffi never turns on.scripts/__tests__/lib/rust-compiled-files.mjs, works this out by following a crate's modules fromsrc/lib.rsunder its enabled features. Foreql-bindingsit matches cargo's own dep-info file for file.ffi-binding-action.test.mjsnow fails in both directions: a compiled file the key misses, and a file the key hashes that cargo doesn't read.wasm-build-inputs-paths-filter.test.mjsalso fails any workflow filter that reaches wider intopackages/eql/crates/than the key does.@cipherstash/eqlpackage, but none of them triggered on it, so an EQL release bump started none of them. They now listpackages/eql/packages/eql/**.tests.yml(second commit)changesjob skips the live jobs (run-tests,e2e-tests,wasm-e2e-tests,run-tests-bun) only when every changed file is in a tree they can't read: the Go module, the Stack Encrypt crates, EQL outside the compiledeql-bindingsfiles and the npm package, docs, other workflows, and Rust tool config. The jobs run unless a change is ruled out, so a forgotten directory costs an extra run, never a missed one.repo-checksjob: the lint scripts, the release gate,test:scripts, protect-ffi's workflow-wiring tests, and the static e2e checks for supply chain and the skill map. They used to run once per Node version insiderun-tests.tests-requiredjob summarises the run as one check that a branch rule could require.tests-workflow-gate.test.mjs, fails if:AGENTS.mddescribes the gate under "Writing tests".Verification
pnpm run test:scripts: 80 files, 1409 passed, 29 skipped (the same skips as onnext). After the first commit alone: 79 files, 1392 passed.packages/eql/crates/**;eql-bindingsentry from the gate;packages/stack-auth/**;.changeset/**(the CLI reads.changeset/config.json);repo-checks.cargo check -p eql-bindings --no-default-featuresdep-info: the same 26 source files.tests.ymlstill starts its live jobs, but only because of rootCargo.tomlandCargo.lock(see review notes).pnpm run code:checkis clean (errors gate; warnings unchanged). The static e2e suites pass (31 tests).actionlint1.7.12 is clean on every changed workflow.tests.yml, so the live jobs should run here.Related
Review notes
changesjob intests.ymlandtests-workflow-gate.test.mjs. The exclusion list is the part that could be wrong in the dangerous direction.Cargo.toml/Cargo.lockstill count as relevant to everything, because the auth binding compiles from that workspace. A change that only adds dependencies to the Stack Encrypt crates (as feat(eql)!: produce the block ORE EQL v4 types through ore-rs 6-bit block ORE #1280 does) still starts the TypeScript jobs, and also Miri, fuzz, udeps and the other Rust workflows. Fixing it means reading the lockfile diff to see which crates it reaches. That is a separate change.