Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 76dfac9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 12 minutes. View limit details
Comment |
131e6cb to
4ae9071
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Reviewed only this PR's diff against refactor/eql-schema-placeholders. I built both bundles locally (mise run build), installed them together into one Postgres 15 database, and ran the new gate against altered copies of the v4 bundle. The v4 artefacts don't ship through any npm, crate or release path, because every publisher names the v3 files explicitly. The path filters and the nextest artifact already cover the new files. Two things need fixing before the "both versions install side by side" claim holds.
1. Major: verify_no_v3_in_v4.sh passes a large leak and fails only a small one
packages/eql/tasks/test/verify_no_v3_in_v4.sh:37 (and the same pattern at :43):
if grep -n 'eql_v3' "$f" | head -5 | sed "s|^|$f:|" | grep .; thenThe script sets pipefail. When grep -n has more output than fits in the pipe buffer (about 64 KiB, so a few hundred matching lines), head -5 exits early and grep is killed by SIGPIPE (status 141). Under pipefail that makes the whole if condition false. The script prints the first five offending lines, then skips the ERROR, never sets fail=1, and reports v4 isolation gate OK.
I reproduced it on this branch's build:
- I renamed every
eql_v4_internaltoeql_v3_internalin a copy ofrelease/cipherstash-encrypt-v4.sql, which gives 4,482 lines namingeql_v3. The gate printed five matches, thenv4 isolation gate OK (1 files), and exited 0. - A copy with one
eql_v4.eq_termchanged toeql_v3.eq_termfailed correctly, with exit 1.
So the gate catches a single stray reference and passes a wholesale one, such as a renderer bug that maps one placeholder to the wrong prefix. A wholesale leak is the failure an artefact-level check most needs to catch. The PR's verification used single-line edits plus the full v3 bundle, and the v3 bundle is rejected by the separate "never names eql_v4" check, so this case was never exercised.
Suggested fix (checked: it fails the 4,482-line leak and passes the real bundle). Capture the matches first and print from a here-string, so no writer process is left in the pipeline to be killed:
if hits=$(grep -n 'eql_v3' "$f"); then
sed -n "1,5s|^|$f:|p" <<<"$hits"
echo "ERROR: $f names eql_v3 (first 5 shown)" >&2
fail=1
fiApply the same change at line 43. It would also be worth adding a small self-test for the gate, like symbol_order_selftest.sh, that includes a many-line leak, so this can't regress unnoticed.
2. Major: with both bundles installed, each version's lints() reports thousands of errors
With v3 and v4 in one database:
| query | v3 only | v3 + v4 |
|---|---|---|
SELECT count(*) FROM eql_v3.lints() |
0 | 3,840 errors |
SELECT count(*) FROM eql_v4.lints() |
— | 3,840 errors |
Every row is about the other version's objects, in the categories inlinability_language and inlinability_set_clause, for example operator #-(eql_v4_bigint, text[]) -> eql_v4_internal."#-"(eql_v4_bigint,text[]). Uninstalling v4 brings eql_v3.lints() back to 0.
The cause is in the rendered src/v3/lint/lints.sql. encrypted_domain_types (around :86-102) takes every jsonb domain in public. That now includes the other version's public.eql_vN_* domains, so eql_operators pulls in all of their operators. But encrypted_domain_blockers (:164) recognises blockers only in the lint's own owned_schemas(). The other version's blocker operators therefore fail the inlinability rules they are meant to be exempt from.
lints() is the documented installation self-check (docs/reference/eql-functions.md). A customer who runs it after installing both bundles gets a wall of false errors. lint_tests.rs will fail for the same reason as soon as any suite installs both bundles in one database. The lint SQL is unchanged here, but this PR is the one that makes coexistence a supported state, and v3_v4_coexistence_tests.rs doesn't check it.
Suggested fix: limit the public arm of encrypted_domain_types to the lint's own family, for example dn.nspname = 'public' AND dt.typname LIKE '{{prefix}}\_%'. Then add an assertion to v3_and_v4_install_side_by_side that eql_v3.lints() and eql_v4.lints() both return no error rows with both installed. If the fix belongs in a later PR in the stack, a note in the module doc and a pinned test would stop the claim from reading as complete.
3. Minor: the coexistence tests don't pin one of the loud failures the ADR records
The ADR's Consequences section says three things fail loudly across versions: a wrong-version insert, the function form, and OPERATOR(public.=) between a v3 and a v4 column ("not unique"). v3_v4_coexistence_tests.rs covers the first two but not the third. I confirmed it does raise operator is not unique: eql_v3_integer_eq public.= eql_v4_integer_eq today. The ADR also says eql_v3.eq_term(v3col) = eql_v4.eq_term(v4col) compares the two terms as text. That is a second silent path, and it is not pinned either. The module doc says these tests pin what the separate names do and don't enforce, so both belong next to operator_across_versions_returns_no_rows (tests/sqlx/tests/v3_v4_coexistence_tests.rs:175). Otherwise a change to either one goes unnoticed.
4. Nit: stale v4 artefacts aren't cleaned
packages/eql/tasks/build.sh:48-49 removes the v3 installer and uninstaller before rebuilding, but not the v4 pair. The clean task in packages/eql/mise.toml:63-69 also removes only the v3 files. After mise run clean, a stale release/cipherstash-encrypt-v4.sql stays behind, and install_v4 in the coexistence tests reads whatever is on disk. Add the two v4 paths to both places.
4ae9071 to
4794120
Compare
|
Rebased onto the updated #1147, then pushed 4794120, which answers the review above:
Coexistence suite: 7 of 7 pass locally against Postgres 17. That run used a structurally valid local stand-in for the gitignored |
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. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation: automated /code-review medium plus /cross-check-review-findings, run by Claude Code against head 4794120. Scope was the PR's two commits (f0946fe, 4794120) over the placeholder base. I read the diff and cross-checked release/*.sql consumers in packages/eql/tasks, mise.toml and the workflows. I did not run the build, the isolation selftest or the sqlx suite.
Critical / blocking: none.
Non-blocking (2):
tasks/test/verify_no_v3_in_v4.sh:286— the payload-version check only matches the text form'v' = '3'. A leftover jsonb-form check such as(VALUE->'v') = '3'::jsonbwould pass the gate. Today every domain CHECK is written in the text form, so this is hardening only. Widening the regex, or asserting'{{eql_version}}'appears the expected number of times in the v4 render, would close it.src/v3/lint/lints.sql:103—LIKE '{{prefix}}\_%'leaves the_insideeql_v3/eql_v4as an unescaped wildcard. This is harmless for these prefixes, but the pattern reads as more precise than it is.
Cross-checks that came out clean:
- Nothing else reads the v4 installer.
prepare-bindings-assets,_build-eql-sql.ymland the sqlx archive/partition tasks stay v3-only, and the artifact globrelease/*.sqlalready ships the v4 files the coexistence tests read at runtime. - The symbol-order check runs on v3 only, and the comment explains why that covers v4.
- The
.changesetentry for thelints()fix is present.
Generated by Claude Code
tobyhede
left a comment
tobyhede
left a comment
There was a problem hiding this comment.
Code review (medium): no confirmed correctness bugs. Four coverage and fragility findings are posted inline.
Attestation
- Files reviewed:
packages/eql(tasks/build.sh,tasks/test/*gates,crates/eql-codegenrender,lints.sql,mise.toml, coexistence tests) and.changeset. Generatedsrc/v3/**SQL was not read line by line. - Model: claude-sonnet-5-5. Nothing was built or run.
Generated by Claude Code
|
|
||
| # ONE stray reference. | ||
| printf "CREATE SCHEMA eql_v4;\nSELECT eql_v3.eq_term(x);\n" > "$tmp/one.sql" | ||
| if gate "$tmp/one.sql"; then echo "FAIL: one eql_v3 reference accepted"; exit 1; fi |
There was a problem hiding this comment.
The negative cases only check that the gate exits non-zero. The gate exits 2 on bad input (missing or empty file, wrong path) and 1 on a real leak. If the gate crashes, every "rejected" case still passes and only the clean case can fail. Assert exit status 1 specifically.
Generated by Claude Code
| # The domain CHECKs compare the payload version. Any v3 comparison left in | ||
| # the v4 bundle would admit v3 payloads into v4 columns. | ||
| if hits=$(grep -nE "'v'[[:space:]]*=[[:space:]]*'3'" "$f"); then | ||
| report "$f" "checks for payload version 3" "$hits" |
There was a problem hiding this comment.
The version check matches only the literal 'v' = '3'. Other spellings of a v3 check slip through, for example VALUE->>'v' IN ('3'), '3' = VALUE->>'v' or (VALUE->>'v')::int = 3. A v4 domain would then admit v3 payloads and the gate would stay green. A positive check would be safer: every CHECK on 'v' in the v4 bundle must compare against '4'. Also, grep exit status 2 is currently treated the same as no match.
Generated by Claude Code
| @@ -118,14 +126,22 @@ assemble() { | |||
| } | |||
|
|
|||
| assemble 3 release/cipherstash-encrypt.sql release/cipherstash-encrypt-uninstall.sql | |||
There was a problem hiding this comment.
The v4 installer is covered by few of the v3 checks. verify_installer_complete.sh (mise.toml) runs only against cipherstash-encrypt.sql. sync-lockstep-versions.mjs and prepare-bindings-assets.sh only hash and manifest the v3 installer and uninstaller. The symbol-order check is skipped for v4. A truncated or stale v4 bundle could therefore pass every check. Either extend these checks to v4 or state that v4 is internal-only for now.
Generated by Claude Code
| } | ||
|
|
||
| let joined: i64 = sqlx::query_scalar("SELECT count(*) FROM t3 JOIN t4 ON t3.c = t4.c") | ||
| .fetch_one(&pool) |
There was a problem hiding this comment.
plan.contains("(c)::jsonb = ") depends on the exact EXPLAIN deparse text, which can differ between Postgres majors. A catalog check, such as looking up the operator in pg_operator with oprleft = 'jsonb'::regtype, would pin the same behaviour without that dependency.
Generated by Claude Code
ADR-0002 installs one SQL source under two names, so a column's domain says which engine wrote its terms. The build now renders the placeholder source a second time as eql_v4 and writes release/cipherstash-encrypt-v4.sql and its uninstaller. The v3 artefacts are unchanged, byte for byte. verify_no_v3_in_v4.sh fails the build if the v4 bundle names eql_v3 or checks for payload version 3: a stray v3 reference would install cleanly and then compare v4 data with v3 terms. The symbol-order check stays on v3 only, since v4 is the same files in the same order with every name renamed. v3_v4_coexistence_tests installs both bundles into one database and pins what the separate names enforce: a wrong-version payload fails the domain CHECK, and the cross-version function form is ambiguous. It also pins the gap the corrected ADR records: a plain `=` across versions falls back to jsonb = jsonb and returns no rows, silently, until cross-version blocker operators exist (#1160). Refs #1141
…dles verify_no_v3_in_v4.sh piped grep into head under pipefail. When the matches outgrew the pipe buffer, grep died of SIGPIPE, the condition read false, and a wholesale eql_v3 leak passed while a single stray reference failed. It now captures the matches before printing them, and a new self-test (test:v4_isolation, run in CI) feeds it a 20,000-line leak. With v3 and v4 in one database, each lints() reported 3,840 errors about the other version's blocker operators: encrypted_domain_types took every jsonb domain in public, while blockers are recognised only in the lint's own schemas. The public arm is now limited to the lint's own prefix, and the coexistence test asserts both lints are clean with both installed. The coexistence tests also pin OPERATOR(public.=) between versions (ambiguous) and the eq_term text comparison (runs, refuses nothing), and build.sh and mise run clean remove the v4 artefacts too.
4794120 to
76dfac9
Compare
This branch has not been deployed
3 participants
Summary
EQL (Encrypt Query Language) is the SQL we install into a customer's Postgres so it can store and search encrypted values. ADR-0002 (#1139) installs the same SQL under two names.
eql_v3is for payloads written by cipherstash-client, the engine behind the TypeScript SDK.eql_v4is for payloads written by Stack Encrypt, the engine behind the Go SDK. This PR makes the build produce theeql_v4installer next to the existing one, and adds tests showing what having two installs in one database does and doesn't protect against. The v3 installer customers get is unchanged, byte for byte. The v4 installer isn't published yet; shipping it is #1142.Changes
tasks/build.shrenders the placeholder source (from refactor(eql): write the SQL source with schema and version placeholders #1147) a second time, aseql_v4, intorelease/cipherstash-encrypt-v4.sqlandrelease/cipherstash-encrypt-v4-uninstall.sql. The installer-completeness check runs on both. The symbol-order check stays on v3 only: v4 is the same files in the same order with every name renamed, so the result can't differ.tasks/test/verify_no_v3_in_v4.sh: new, and part of every build. It fails if the v4 bundle nameseql_v3or checks for payload version 3. A stray v3 reference would install cleanly and then compare v4 data with v3 terms.tests/sqlx/tests/v3_v4_coexistence_tests.rs: new. It installs both bundles into one database and checks five things:eql_v3.eq(v3col, v4term)) errors as ambiguous;v3col = v4term, or a join between a v3 and a v4 column, falls back to Postgres's built-injsonb = jsonband returns no rows without an error. The corrected ADR records this, and EQL:=between aneql_v3column and aneql_v4query term returns no rows silently — add cross-version blocker operators #1160 tracks the operators that would turn it into an error.packages/eql/AGENTS.md: lists the v4 outputs and the new gate.Verification
cmp).eql_v4.eq_termchanged toeql_v3.eq_term, one with a single'v' = '4'changed to'3', and the v3 bundle itself.eql_v3_integerfixture with dummy ciphertexts, because I don't have CipherStash credentials locally. CI generates the real fixture and runs them against real ciphertexts.codegen:parity,test:crates,test:installer_complete,test:symbol_order_v3,test:self_contained_v3, and rootpnpm run test:scripts(71 files, 1296 tests).packages/eql/release/*.sql, so the test shards get the v4 installer.Related
Refs #1141. Stacked on #1147 (placeholders), which is stacked on #1139 (the ADR). Next in the stack: run the SQLx suites against both installs.
Review notes
Read the module doc at the top of
v3_v4_coexistence_tests.rsfirst. It is the plain-language statement of what EQL v4's separate names enforce. When the blockers from #1160 land,operator_across_versions_returns_no_rowsshould start expecting an error.https://claude.ai/code/session_01CA4pFfaN4DPNSTLnJVNxsu