Repository navigation
ci: run JS and FFI suites only on changes that can reach them - #1290
Merged
Merged
Conversation
Contributor
|
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".
freshtonic
reviewed
Oct 23, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Comment only. The gate design is sound. One coverage gap should be fixed before merge.
What I checked
pnpm run test:scriptspasses in a worktree: 1409 passed, 29 skipped. The new and related guards also pass: tests-workflow-gate, rust-compiled-files, ffi-binding-action, wasm-build-inputs-paths-filter, ffi-binding-step-order, eql-workflow-filters and workflow-paths-filter-parity.- The gate is job-level (
if: needs.changes.outputs.js == 'true'), so a gated job reports "skipped".tests-requiredaccepts onlysuccessorskipped. A failedchangesjob makes it fail. Push events run everything. - The exclusions are safe.
eql-domainsis only a dev-dependency ofeql-bindings. Theeql-bindingsdefault features and itsinclude_str!targets stay inside the included paths. protect-ffi takesstack-authfrom the registry. RootCargo.*,mise.toml, the pnpm files,turbo.json,.github/actions/**andvitest.shared.tsall still start the live jobs. - Adding
packages/eql/packages/eql/**to the Drizzle, Prisma Next, Supabase, Prisma example and bench filters closes a gap that existed before this PR.
Non-blocking (not part of this diff)
- The workflow-level
paths: ['**', '!docs/plans/**']filter (onnextalready) means that a PR that changes only plan documents starts no run. Thentests-requirednever reports. No ruleset requires that check today, so nothing blocks now. If someone makes it a required check, plans-only PRs will stay pending. Remove the workflow-level filter, or add a note that the check must not be required while the filter exists. Codex raised this too.
…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.
…heck runs repo-checks now runs test:scripts straight after install. Nothing before it builds @cipherstash/eql, so bench-index-expressions' check that dist/sql is a verbatim copy of sql skipped without a word. On next the run-tests turbo builds came first and the check ran. The job builds the package first, and sets EQL_DIST_REQUIRED, which makes the check fail rather than skip when dist/ is missing. Locally it still runs only after a build. tests-required and the workflow's paths filter now say that the check must not be required while plans-only pull requests skip the workflow. Claude-Session: https://claude.ai/code/session_01XQN1QpaWVDGjHEg39KEJF3
coderdan
force-pushed
the
ci/path-filter-precision
branch
from
October 23, 2026 12:39
039a665 to
02e49dc
Compare
|
Contributor
Author
|
@freshtonic thanks for the review. Both points are addressed in 02e49dc.
I also rebased the branch onto the current |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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.