feat(cli): rewrite db validate as eql validate for the EQL v3 domain vocabulary - #857
Open
tobyhede wants to merge 7 commits into
Open
Conversation
The approved plan for moving `db validate` to `eql validate` and rebuilding its rule set around the EQL v3 domain-type vocabulary.
`getEncryptConfig()` returns the protect-ffi view — each column builds to
{ cast_as, indexes } and the concrete EQL v3 domain name is dropped. That
makes cast_as 'number' with an `ope` index ambiguous across
eql_v3_integer_ord, smallint_ord, real_ord, double_ord and numeric_ord, so
tooling that must reason about the DECLARED domain could not recover it
from a client alone. The tables are usually imported into the client file
rather than re-exported from it, so duck-typing the module namespace is
not a reliable substitute.
`getSchemas()` hands back the tuple passed to Encryption({ schemas }), by
reference, from which each column yields getEqlType() / getName() /
getQueryCapabilities() / isQueryable(). `stash eql validate` is the first
consumer.
The exact-member-set gate in v3-only-public-surface.test.ts is updated
deliberately: the accessor returns the same tuple the reconstructor map
and the unknown-table guard were derived from, and can neither replace
nor extend them, so it is not a re-initialization path.
The v2 rule set checked for ore/unique/match/ste_vec indexes and never
learned about `ope`. EQL v3's default ordering domains emit `ope`, so
types.IntegerOrd('age') and types.TimestampOrd('created_at') were both
reported as "Column is encrypted but has no indexes — it will not be
searchable". Two of the most ordinary columns anyone writes, told they
were unsearchable. They are now silent.
The command reads the user's tables through the new
EncryptionClient.getSchemas(), so every rule can key off the concrete
domain rather than the lossy encrypt config. New `loadEncryptSchemas`
sits beside `loadEncryptConfig` and shares its jiti load and placeholder
refusal (both now go through one `loadEncryptionClient`); it degrades to
config-only, with a warning, when the project's @cipherstash/stack
predates getSchemas().
Schema rules: an `_ord_ore` domain (Warning — its ORE operator class
needs superuser), storage-only columns (Info), and three guards for
hand-authored configs (searchable boolean, match on a non-text domain,
ste_vec without json — all Error).
Database rules, when a connection resolves: EQL not installed (reported
once, and the remaining database rules are skipped, so the user is not
told "ORE unavailable" when the answer is `stash eql install`), missing
tables/columns, domain drift against information_schema, a still-plain
column, an `_ord_ore` domain where the opclass is genuinely absent
(upgrading the static Warning), and queryable columns with no functional
index over their term extractor. An unreachable database is a notice, not
a failure.
Every database fact enters through an injected ObservedState, so the
drift rules are unit-tested without a database.
`--exclude-operator-family` is removed: `eql install`/`eql upgrade`
already rejected it because the pinned v3 bundle self-adapts, and
validate was its last consumer. `stash db validate` keeps working as a
deprecated alias, like db install / db upgrade / db status.
Not implemented, deliberately: "ordered domains reject empty strings" is
a value-level CHECK enforced at encrypt time and is not statically
checkable.
🦋 Changeset detectedLatest commit: 098fac7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
📝 WalkthroughWalkthroughAdds ChangesEQL v3 validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant validateCommand
participant EncryptionClient
participant PostgreSQL
CLI->>validateCommand: run eql validate
validateCommand->>EncryptionClient: load client and call getSchemas()
EncryptionClient-->>validateCommand: declared v3 schemas
validateCommand->>PostgreSQL: query installation, columns, domains, and indexes
PostgreSQL-->>validateCommand: observed database state
validateCommand-->>CLI: report issues and exit status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/cli/src/commands/eql/__tests__/validate.test.ts (1)
482-496: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding coverage for
reportIssues.
reportIssuesdecides the exit code (errors > 0) and produces the summary counts. The tests cover the pure rules and the parser but not this function. A small unit test over a mixed issue list would pin the error-exits-1 contract the plan calls out.🤖 Prompt for 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. In `@packages/cli/src/commands/eql/__tests__/validate.test.ts` around lines 482 - 496, Add a focused unit test for reportIssues using a mixed issue list containing errors and non-errors, and assert that it reports the correct summary counts and returns exit code 1 when errors are present. Keep the test independent of the existing validateSchemas rule tests and use the function’s established output or logging hooks.packages/cli/src/config/index.ts (1)
313-333: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden the schema shape guard.
typeof null === 'object', soisV3TableLikeaccepts a table whosecolumnBuildersisnull.collectDeclaredColumnsthen callsObject.values(table.columnBuilders)and throws, which is the failure this guard exists to prevent. A hand-rolled or adapter-builtgetSchemascan also throw; the documented contract is to degrade to config-only rather than fail.♻️ Proposed hardening
- const schemas = getSchemas.call(encryptClient) + let schemas: unknown + try { + schemas = getSchemas.call(encryptClient) + } catch { + // A hand-rolled or adapter-built client can throw here. Degrade to + // config-only, exactly as an absent `getSchemas` does. + return { config, schemas: undefined } + }function isV3TableLike(value: unknown): value is AnyV3Table { return ( !!value && typeof value === 'object' && typeof (value as { tableName?: unknown }).tableName === 'string' && - typeof (value as { columnBuilders?: unknown }).columnBuilders === 'object' + !!(value as { columnBuilders?: unknown }).columnBuilders && + typeof (value as { columnBuilders?: unknown }).columnBuilders === 'object' ) }🤖 Prompt for 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. In `@packages/cli/src/config/index.ts` around lines 313 - 333, Harden isV3TableLike so columnBuilders must be a non-null object, preventing collectDeclaredColumns from receiving null. Also wrap the getSchemas.call(encryptClient) invocation in the surrounding schema-loading flow so adapter or stub errors return { config, schemas: undefined } instead of propagating; preserve the existing valid-schema path.
🤖 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 `@skills/stash-cli/SKILL.md`:
- Around line 434-435: Update the documentation around the getSchemas() accessor
to state that it may be unavailable in older installed `@cipherstash/stack`
versions, causing schemas to be undefined. Document that validation then falls
back to collectDeclaredColumnsFromConfig(), skips concrete-domain checks such as
ORE portability and database-drift validation, and requires upgrading the stack
to enable those checks.
---
Nitpick comments:
In `@packages/cli/src/commands/eql/__tests__/validate.test.ts`:
- Around line 482-496: Add a focused unit test for reportIssues using a mixed
issue list containing errors and non-errors, and assert that it reports the
correct summary counts and returns exit code 1 when errors are present. Keep the
test independent of the existing validateSchemas rule tests and use the
function’s established output or logging hooks.
In `@packages/cli/src/config/index.ts`:
- Around line 313-333: Harden isV3TableLike so columnBuilders must be a non-null
object, preventing collectDeclaredColumns from receiving null. Also wrap the
getSchemas.call(encryptClient) invocation in the surrounding schema-loading flow
so adapter or stub errors return { config, schemas: undefined } instead of
propagating; preserve the existing valid-schema path.
🪄 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 Plus
Run ID: 7cda471e-65bc-42f1-b93d-e70caf84c149
⛔ Files ignored due to path filters (2)
packages/cli/__fixtures__/scaffold/drizzle.generated.tsis excluded by!**/*.generated.*packages/cli/__fixtures__/scaffold/generic.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (24)
.changeset/olive-poems-guess.md.changeset/proud-ravens-repeat.mddocs/plans/cip-3366-eql-validate-v3.mdpackages/cli/README.mdpackages/cli/src/__tests__/v2-retirement.test.tspackages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/db/config-scaffold.tspackages/cli/src/commands/db/validate.tspackages/cli/src/commands/encrypt/context.tspackages/cli/src/commands/eql/__tests__/validate.test.tspackages/cli/src/commands/eql/validate.tspackages/cli/src/commands/init/steps/install-eql.tspackages/cli/src/commands/init/utils.tspackages/cli/src/config/index.tspackages/cli/tests/e2e/command-help.e2e.test.tspackages/cli/tests/e2e/smoke.e2e.test.tspackages/stack/__tests__/client-get-schemas.test.tspackages/stack/__tests__/v3-only-public-surface.test.tspackages/stack/src/encryption/client-v3.tsskills/stash-cli/SKILL.mdskills/stash-encryption/SKILL.mdskills/stash-indexing/SKILL.mdskills/stash-postgres/SKILL.md
💤 Files with no reviewable changes (1)
- packages/cli/src/commands/db/validate.ts
Six defects found reviewing #857, each pinned by a test written to fail first. A table absent from `current_schema()` was an Error, and both catalogue reads are scoped to that one schema. So a project whose tables live anywhere else — Prisma `multiSchema`, a tenant schema, a `schema.table` name that the reader compares whole against a bare `table_name` and never matches — failed validate on a completely healthy database. Validate cannot tell that apart from a migration that never ran, so it is now a Warning naming the schema it searched. The missing-COLUMN rule stays an Error: there the table resolved, so the column really is absent. `isV3TableLike` accepted `columnBuilders: null` (`typeof null === 'object'`) and never checked the builders themselves, so a malformed client reached `Object.values(null)` in `collectDeclaredColumns` and crashed with a stack trace — in the one guard written so that such a client degrades instead. It now rejects null and verifies every builder implements the four methods the rules call. The index read fetched and regex-parsed every `pg_get_indexdef()` in the schema to answer a question about a handful of declared columns; it is now constrained to those tables. `collectDeclaredColumnsFromConfig` guarded `column.indexes` with `?? {}` for the queryable check and then assigned it bare. That config is user code loaded through jiti — the zod types that make `indexes` non-optional never run against it — so a column missing the key threw on `column.indexes.match` partway down the rule list. Guarded on both. The CREATE INDEX suggestion is meant to be pasted, but quoted identifiers without doubling an embedded `"`; `identifiersIn` already un-doubles on the read side. Added `quoteIdent` as the write side of that rule. `EXTRACTOR_HEAD` was a module-level `/g` regex whose `lastIndex` an otherwise-pure function depended on. Now built per call. Also: `getSchemas()` returns a frozen tuple, since the reconstructor map is derived from it once at construction and the CLI reaches the client untyped; and the skill and changeset record the schema-scoping limit and the older-@cipherstash/stack fallback, neither of which was documented.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/cli/src/commands/eql/__tests__/validate.test.ts`:
- Around line 686-702: Remove the duplicate block-scoped queries declaration in
the constrains the index scan to the declared tables test, retaining a single
queries array for capturing client.query calls and leaving the existing
assertions unchanged.
In `@skills/stash-cli/SKILL.md`:
- Line 436: Update the fallback behavior description in the getSchemas()
documentation to remove “plain-column detection” from the checks skipped with
older `@cipherstash/stack` versions; retain the statements about ORE portability
and drift requiring domain information.
🪄 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 Plus
Run ID: 2708e8f3-ea81-4056-a229-70f3e5acac40
📒 Files selected for processing (8)
.changeset/proud-ravens-repeat.mdpackages/cli/src/commands/eql/__tests__/validate.test.tspackages/cli/src/commands/eql/validate.tspackages/cli/src/config/__tests__/load-encrypt-schemas.test.tspackages/cli/src/config/index.tspackages/stack/__tests__/client-get-schemas.test.tspackages/stack/src/encryption/client-v3.tsskills/stash-cli/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/stack/tests/client-get-schemas.test.ts
- packages/cli/src/config/index.ts
- packages/stack/src/encryption/client-v3.ts
Follow-up review of 3bbc7ec, which downgraded the absent-table finding to a Warning because validate could not distinguish the two situations behind it. It can — it just was not asking. `OTHER_SCHEMAS_SQL` looks each declared table name up across every schema, deliberately unscoped where the other reads are scoped to `current_schema()`. Found under another schema, the project is healthy and merely pointed at the wrong `search_path`, so the finding stays a Warning and now names the schema that has the table and the connection option to reach it. Found under none, the migration genuinely has not run, and that is an Error again — the previous commit had traded a false failure for a missed one, and CI stopped catching a forgotten migration. The finding is also reported once per table rather than once per column, which is the rule `validateSchemas` already applies to the not-installed case for the same reason: one fact explains every column, and repeating it per column buries it. A twenty-column table produced twenty identical paragraphs. The columns still run their schema rules; only their database rules are skipped. Two smaller things from the same pass. The `readObservedState` fake routed on `text.includes('current_schema')`, which also matches the index read, so that query was fed schema rows and the test passed only because `RegExp.exec(undefined)` matches nothing; it now routes on the result alias each query selects. And `Object.freeze(schemas) as S` asserted nothing — removing it produces no type error — while its comment implied a depth the freeze does not have, so the assertion is gone and the comment now says shallow, with a test pinning that the tables inside stay mutable.
auxesis
reviewed
Aug 4, 2026
auxesis
left a comment
auxesis
left a comment
Contributor
There was a problem hiding this comment.
Verdict
No correctness, payload-shape, or security issues surfaced. The eql validate rewrite is well tested (validate.test.ts at 820 lines covers the pure rules, extractor parsing, and getSchemas() duck-typing). Both source reviews are test-gap only, and they agree on the one highest-value hole: readObservedState fans its six catalogue queries out but no test asserts the results land in the right ObservedState fields. Kept findings are all coverage gaps; none block merge.
Review stats
| Source | Raw | Survived |
|---|---|---|
| claude (claude-opus-4-8) [test-gap] | 2 headline (write-up enumerated 5 distinct gaps) | 5 |
| codex (gpt-5.5) [test-gap] | 1 | 1 |
- Cross-model overlap: 1 kept finding — the
readObservedStateend-to-end mapping gap — was corroborated by both models. The remaining four are single-source (claude), each verified against the code. - Dropped: none. Every enumerated gap substantiated against
validate.ts/config/index.tsand the test file.
All 5 kept findings fit under the 8-comment cap; posted inline below.
CI has been red since this branch's first push, on every commit, and the PR's own verification section did not catch it: `packages/stack` splits `test` (`vitest run`) from `test:types` (`vitest --run --typecheck.only`), CI gates on the latter, and only the former was ever run. `getSchemas(): S` was the first member to put `S` in an output position, which made a readonly-vs-mutable tuple difference observable for the first time and broke `encryption-v3-only.test-d.ts`. The fix is `getSchemas(): Readonly<S>` — the type the implementation already produces, since it returns `Object.freeze(schemas)`. Note `readonly [...S]` does NOT work: a tuple spread gives `S` a measurable variance, which lets the identity relation short-circuit to comparing type arguments, and `[T]` is not identical to `readonly [T]`. `Readonly<S>` is homomorphic, leaves variance unmeasurable, and falls back to structural comparison. Both files carry a comment saying so, because the failure is invisible from the source. A privilege-invisible table was reported as an unapplied migration. `information_schema.columns` shows only what the connected role holds a privilege on; `pg_catalog.pg_class` is not privilege-filtered. The `pg_class` lookup added two commits ago excluded `current_schema()`, so a table present-but-invisible landed in neither map and hit the hard "does not exist in any schema" Error — telling someone to re-run a migration that had already run. The lookup now covers every schema and the miss resolves four ways: elsewhere, invisible-to-role (carrying the GRANT to run), schema -qualified, or genuinely absent. Only the last exits 1. Verified against a real Postgres cluster, including that the emitted GRANT fixes it. A `schema.table` name gets its own finding rather than a silent false "missing". Splitting the name was rejected deliberately: the only column reader is scoped to `current_schema()`, so `app.users` would have validated against `public.users` and reported an unrelated table's drift as this one's. An explicit "not checked" beats a confident wrong answer. A `getSchemas()` that throws now degrades to config-only like every other malformed-client path, instead of escaping as "Fatal error". The `tableName` guard gained the test two reviews graded low-value: dropping it does not merely put `undefined` in the output, it feeds `table: undefined` into the unreachable-table rule, which raises an error and exits 1 — the same contract violation by another route. Characterization coverage for the two seams that had none: `readObservedState`'s six-query positional mapping (proved non-vacuous by mutation — swapping two entries, and simulating the reader's swallowing catch) and `reportIssues`, whose prefix logic changed in this PR precisely because the rewrite introduced schema-wide and table-level issue shapes; reverting it to v2's form reproduces `undefined.undefined:`. Skill and changeset record the four-way split, the two findings that no longer exit 1, and the widened fallback trigger.
auxesis
approved these changes
Aug 4, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts (1)
134-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch the fixture to the test name.
The test name says the
tableNameis not a string, but the fixture omitstableNameentirely. Both cases failisV3TableLike, so the assertion still holds. Add a present non-string value to cover the case the name describes.Proposed fix
- writeProject(clientReturning(`[{ columnBuilders: {} }]`)) + writeProject(clientReturning(`[{ tableName: 42, columnBuilders: {} }]`))🤖 Prompt for 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. In `@packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts` around lines 134 - 140, Update the fixture in the “rejects a table whose tableName is not a string” test to include a present, non-string tableName value while preserving the existing assertion and test flow.
🤖 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 `@packages/cli/src/commands/eql/__tests__/validate.test.ts`:
- Around line 1000-1007: Remove the duplicate block-scoped queries declaration
in the test case beginning “asks for the schemas of exactly the declared
tables,” retaining a single typed queries variable for the client mock and
subsequent assertions so the test compiles.
---
Nitpick comments:
In `@packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts`:
- Around line 134-140: Update the fixture in the “rejects a table whose
tableName is not a string” test to include a present, non-string tableName value
while preserving the existing assertion and test flow.
🪄 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 Plus
Run ID: d0647f6a-6523-4357-88e6-d8d3c0d3e873
📒 Files selected for processing (9)
.changeset/proud-ravens-repeat.mdpackages/cli/src/commands/eql/__tests__/validate.test.tspackages/cli/src/commands/eql/validate.tspackages/cli/src/config/__tests__/load-encrypt-schemas.test.tspackages/cli/src/config/index.tspackages/stack/__tests__/client-get-schemas.test.tspackages/stack/__tests__/encryption-v3-only.test-d.tspackages/stack/src/encryption/client-v3.tsskills/stash-cli/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/proud-ravens-repeat.md
- packages/cli/src/config/index.ts
- packages/stack/tests/client-get-schemas.test.ts
- packages/cli/src/commands/eql/validate.ts
`TABLE_SCHEMAS_SQL` scanned every schema in the database. `information_schema` publishes views named `columns`, `domains`, `parameters`, `routines`, `sequences`, `tables` and `triggers` — all ordinary application table names — so a project declaring one of them that had not run its migration matched the system view, took the `elsewhere` branch of `unreachableTableIssue`, and was told to point `search_path` at `information_schema`. Being a Warning rather than an Error, the command then exited 0 on a genuinely unapplied migration. Excluded `pg_*` and `information_schema`, pinned by a regression test. The predicates use `!~ '^pg_'`, not `NOT LIKE 'pg\_%'`: the SQL is a JS template literal, which collapses `\_` to a bare `_` and leaves a LIKE wildcard that also swallows `pgbouncer` and `pgsodium`. Report which relation a bare table name resolved to when it is not unique. Both column readers match unqualified names against `current_schema()`, so a `users` in both `public` and Supabase's `auth` left every domain, plain-column and index finding describing whichever one `search_path` happened to pick, with nothing said about the choice. This is the unqualified twin of the `schema.table` collision `unreachableTableIssue` already refuses to guess at. Info, not Warning: it must not move the exit code or report an ordinary Supabase project as unclean, and unlike the not-found cases it did check a table — it is qualifying which, not reporting that nothing happened. `ObservedState.elsewhere`'s doc comment claimed it held only tables absent from the searched schema. It never did — it is populated per catalogue row — and the new rule reads exactly that overlap, so the comment now says so and warns against "tidying" it back. Close the review test gaps: - `validate-command.test.ts` (new, 13 tests) covers `validateCommand` and `tryReadObservedState`: the no-database-URL notice, the connect-error catch, the catalogue-read failure, the degraded-`getSchemas()` warning, count pluralisation, and the exit-code contract in both directions. Separate file because `vi.mock` is hoisted and file-wide — folding the loader, `pg` and `process.exit` stubs into the pure rule suite would apply them to every test there. - The two domain-less database branches now have tests that feed `collectDeclaredColumnsFromConfig` output with an `ObservedState` — the old-`@cipherstash/stack` plus reachable-database combination the degrade was written for. Each asserts the whole message, since the existing `stringContaining` assertions match both renderings of the ternary. - `load-encrypt-schemas.test.ts` splits the `tableName` guard into its two distinct inputs; the existing test was named for a non-string `tableName` but fed a fixture that omitted it. - `validate.test.ts` no longer contributes a `tsc` error: the `ste_vec` fixture was missing its required `prefix`, putting the package one over the budget recorded in `.github/workflows/tests.yml`. Skill, README and changeset carry the new Info rule; `skills/stash-cli` also gains the unqualified-name paragraph alongside the `schema.table` one. Verified: `pnpm --filter stash test` 1070 passed / 10 skipped; `code:check` clean; `tsc` back to the recorded 21 errors, none in changed files.
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.
Closes CIP-3366.
db validatewas still the EQL v2 implementation, untouched since before the v3 work. It validated the v2 vocabulary —ore/unique/match/ste_vecindexes,cast_astypes, operator-family warnings — and on a v3 schema it reports nonsense.The bug
hasAnyIndexcheckedore/unique/match/ste_vecbut never learned aboutope. EQL v3's default_orddomains are OPE-backed and emit exactly that, so:Both are fully searchable ordered columns. Validate was telling users to fix schemas that were already correct. Pinned as a regression test.
getSchemas()on the clientEncryptedV3Column.build()emits only{ cast_as, indexes }— the concrete domain name is dropped. That makescast_as: 'number'+{ope:{}}ambiguous acrosseql_v3_integer_ord,smallint_ord,real_ord,double_ordandnumeric_ord, so validate cannot recover the declared domain from anEncryptConfig. Duck-typing the client module's namespace isn't a substitute either: the scaffold's own pattern imports tables from./db/schemarather than re-exporting them.EncryptionClient.getSchemas()returns the tuple passed toEncryption({ schemas }), from which each column yieldsgetEqlType()/getName()/isQueryable().stash eql validateis the first consumer. The CLI's loader degrades to config-only, with a warning, against a client built on an older@cipherstash/stack.Rules
Static, from the declared domains:
_ord_oredomain declared_ord(OPE) twinboolmatchindex on a non-text domainste_vecwithout a json domainDatabase-backed, when a URL resolves (skipped with a notice otherwise, never a failure):
domain_name≠ declaredeqlTypejsonb/text)_ord_oredeclared while the ORE opclass is absentThe ORE probe mirrors the shipped bundle's own fallback test in
@cipherstash/eql@3.0.4(ore_fallback.sql).to_regtypereturns NULL rather than throwing when EQL isn't installed, so the probe degrades tofalse— not-installed is therefore detected and reported separately, or the user gets "ORE unavailable" when the answer is "runstash eql install".Retired: the operator-family warning,
NON_STRING_CAST_TYPES, and--exclude-operator-family.validateInstallFlagsalready hard-rejected that flag andv2-retirement.test.tsasserted it gone from install/upgrade — validate was its last consumer.Command surface
eql validateis the command;db validatewarns viamessages.db.aliasDeprecatedand forwards, matchingdb install/upgrade/status. Flags are--supabaseand--database-url, and--database-urlis now actually used — the old command accepted it and never connected.Not in this PR
The empty-string ordering rule. The issue lists "ordered domains reject empty strings (CHECK requires non-empty
ob)". That is a value-level CHECK enforced at encrypt time; nothing in the schema or ininformation_schemapredicts it. It belongs in the error message on the encrypt path. Marked "Not checked" inskills/stash-cli.Live-database verification. The DB rules are covered by injecting
ObservedStateintovalidateSchemas, plus a pure test forparseIndexedExtractorsover realpg_get_indexdefshapes.readObservedState's result mapping is now pinned too, by a fake client asserting the wholeObservedStatebytoEqual. What remains unexercised against a real cluster is the six catalogue reads themselves — five SQL constants plusfetchPhysicalColumns(this said "four" while the count was three-plus-one; two reads were added since). Theinformation_schemaexclusion and the privilege case were checked by hand against a live Postgres. Still worth a manual pass: an_ord_orecolumn on a non-superuser role, a drifted domain, an unindexed queryable column.Schema scoping — where this landed
fetchPhysicalColumns(inherited fromencrypt/lib/db-readers.ts) andINDEX_DEFS_SQLboth scope totable_schema = current_schema(). The false "Table … does not exist in the database" Error this originally produced is gone:unreachableTableIssuenow resolves four ways — schema-qualified name, privilege-invisible (emitting theGRANT SELECTthat fixes it), present in another schema, absent everywhere — and only the last is an Error.@cipherstash/migratestill handlesschema.tableviasplitTableName/qualifyTable, so the toolchain disagreement is real but now reported rather than mis-reported.Two consequences a reviewer should weigh:
current_schema(); it exits 0 having skipped every rule for it. Teaching the shared reader aboutschema.tableandcurrent_schemas(false)changesencrypt statustoo, so it stays its own PR.usersresolving topublic.userswhile the application readsapp.usershad every domain, plain-column and index finding computed against the wrong relation and reported as fact.Also flagged
WasmEncryptionClientdid not getgetSchemas(). It already lacksgetEncryptConfig, and the CLI's loader duck-types on that, so validate can't reach it either way — but the two client surfaces are now asymmetric.Verification
pnpm run code:check— clean (0 errors)pnpm --filter stash test— 1070 passed, 10 skipped, 0 failed (validate.test.ts72,validate-command.test.ts13)pnpm --filter stash exec tsc --noEmit— 21 errors, matching the budget recorded at.github/workflows/tests.yml:169-170; none in any file this PR touches.validate.test.tswas one over (aste_vecfixture missing its requiredprefix) and is now clean.pnpm --filter stash test:e2e— 100 passedpnpm --filter @cipherstash/stack test— 908 passed, 157 skipped, 0 failed (10 files fail to collect on missing credentials; pre-existing and unchanged)stash manifest --json→eql validate | --supabase,--database-url; nodb validate; no--exclude-operator-familyanywhere. Everystash <cmd>named inskills/stash-cli/SKILL.mdresolves against it.Changesets:
@cipherstash/stackminor,stashminor.Summary by CodeRabbit
New Features
stash eql validatefor EQL v3 schema and optional database validation.EncryptionClient.getSchemas()to expose configured schema metadata.Breaking Changes
stash db validateis now a deprecated alias forstash eql validate.--exclude-operator-familyoption.Documentation