stash eql verify: assert the installed EQL surface is complete - #906
stash eql verify: assert the installed EQL surface is complete#906coderdan wants to merge 3 commits into
Conversation
A partial EQL install - domains present, some of their comparison functions or operators absent - reported success at install time and failed at query time on a specific predicate (e.g. `weight >= x`). Nothing detected it: isInstalled() is a presence test and `eql validate` checks only the columns an application declared. `stash eql verify` compares what the database actually has against everything the pinned bundle installs - every domain, function overload, operator, cast, and the ORE operator class - via read-only catalog queries. The manifest is parsed out of the bundle itself, so a bundle bump updates the expectation automatically; the bundle's two DO-block conditionals (the ORE opclass and its poison fallback) are modelled explicitly instead. Expected absence reads as such: the ORE opclass skipped on managed Postgres with the loud-failure fallback in place is a supported configuration, not damage. Damage is grouped per-domain, exits 1, and `--json` emits the structured report for agents. A version mismatch with the pinned bundle skips the object diff (wrong manifest to compare against) and suggests `eql upgrade`. `stash eql install` now runs the same check before declaring success. Coverage: unit tests run the parser and differ against the real pinned bundle; a live-Postgres suite (gated on STASH_TEST_DATABASE_URL) installs the bundle, asserts the full surface reads complete with exact counts, then drops an operator and version() and asserts the damage is named and attributed. Claude-Session: https://claude.ai/code/session_01AwM5Cm5ddasXozb6stxPR1
🦋 Changeset detectedLatest commit: 4bd720f 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 |
|
Review performed by GPT-5.6-sol. Five inline findings were posted on the install-time verification and catalog surface checks. |
Code review — 10 findings
The three most severe findings form a pattern: the new guarantee doesn't hold in exactly the places it's most needed. 1. Version-mismatch report returns
|
Semantics: - A version mismatch now reports ok:false and exits 1 — "could not verify" must never read as "verified", or a `stash eql verify || fail` CI gate passes on a damaged older install. Exit 0 now means exactly one thing: checked and complete. One exit predicate serves both output modes. - `eql install` verifies on the already-installed early exit too, so a plain re-run over a damaged database fails instead of printing "Nothing to do." (isInstalled() is only a presence test). Precision: - Function checks compare type-only signatures, not per-name counts, so a stale same-name function cannot mask a genuinely missing overload. Catalogue spelling comes from format_type() under a pinned empty search_path, which qualifies every non-catalogue type deterministically (and spells composite arrays `ore_block_256_term[]`, not `_ore_block_256_term`). - The ORE poison-constraint count is scoped to the expected ORE domains (constraint names are not globally unique). - pgcrypto is checked for a supported schema, not bare presence, matching the install preflight. - The version() probe distinguishes 42883 (missing — damage) from other errors (EXECUTE denied, timeout — verification failure), instead of a bare catch that produced a phantom full diff. - Operator identity deliberately stays schema-agnostic: the reviewer's suggested public-only scope phantom-fails a healthy install on any database with a "$user" schema (unqualified CREATE OPERATOR follows the install-time search_path) — the live suite runs against exactly such a database and caught it. Robustness / coverage: - The live suite now runs in CI: tests.yml's run-tests job already has a Postgres service, so STASH_TEST_DATABASE_URL points the CLI's .live.test.ts suites at it. The parser<->catalogue spelling seam is no longer guarded only on developer machines. - `--database-url` with a missing value is rejected up front on verify/preflight instead of silently resolving a different database. - `eql verify --database-url` is a one-shot like `eql install`'s: it bypasses config loading, so the database named is the database judged. - The preflight/verify URL resolver is one shared function; a TYPE_ALIASES map absorbs non-canonical spellings a future bundle might use. Docs: stash-cli skill updated for the new exit semantics and one-shot flag; stash-indexing now points its hand-run ORE-state SQL walkthrough at `stash eql verify` and lists the command. Claude-Session: https://claude.ai/code/session_01AwM5Cm5ddasXozb6stxPR1
|
All 15 findings from the two review passes are addressed in 286cca7. Summary, keyed to the Fable review's numbering (the five GPT-5.6-sol inline findings overlap with findings 2, 5, and 8, plus two of their own — noted inline): Fixed as reported
One suggested fix amended, with evidence
Full validation: 20 unit + 3 live tests green (the live DB is the |
tobyhede
left a comment
There was a problem hiding this comment.
Review — 3 issues
Verified against 286cca78; each finding was adversarially re-checked before filing.
1. eql install hard-fails on a version mismatch, with contradictory advice
packages/cli/src/commands/db/install.ts:251
Two fixes from the earlier passes combine badly. diffSurface now returns ok: false for status: 'version-mismatch' (verify.ts:583), and verifySurfaceOrExit was added to the already-installed path — but it branches on report.ok alone, so skew and real damage take the same branch.
Against a DB carrying 3.0.2 with a CLI pinning 3.0.4, stash eql install prints three inconsistent things and exits 1:
- headline — "The installed EQL surface is incomplete." (nothing was checked;
countsisnull) - finding — "…Run
stash eql upgrade, then verify again." (severity: warning) - error — "Re-run with
stash eql install --force"
Pre-PR this path was p.outro('Nothing to do.'), exit 0. Two consequences: idempotent provisioning scripts break, and stash init dies mid-run — its direct-install route calls installCommand (init/steps/install-eql.ts), and process.exit(1) escapes the surrounding try/catch.
Suggested: branch on report.status === 'version-mismatch' before the ok check and return, leaving eql verify's strict gate as-is.
Worth deciding which remedy is correct first: stash eql upgrade is installer.install(), the same DDL as --force, and it calls loadStashConfig, which exits 1 with no stash.config.ts. On a one-shot --database-url run, --force is the only remedy that actually works.
Nothing covers this path — there are no unit tests for installCommand.
2. Live suites race on one database in CI
.github/workflows/tests.yml:286
STASH_TEST_DATABASE_URL on the whole pnpm run test step enables four live suites at once: verify.live (new), plus guarded-grants.live, preflight.live, applied.live. packages/cli/vitest.config.ts sets no pool, fileParallelism, or maxWorkers, so vitest 3.2.7 runs them in parallel forks against one database and one eql_v3 / eql_v3_internal pair.
verify.live.test.ts's beforeAll installs the full bundle, which opens with DROP SCHEMA IF EXISTS eql_v3 CASCADE. guarded-grants.live.test.ts grants USAGE, CREATE on those schemas and asserts on pg_default_acl rows scoped to them. Recreated schemas get new OIDs and zero ACLs, so its ALTER DEFAULT PRIVILEGES … IN SCHEMA eql_v3 as MEMBER fails on the USAGE it no longer holds — and the pg_has_role guard in grants.ts gates on membership only, so the error propagates rather than being caught.
These files already carry race-safety comments about each other ("Race-safe against the preflight live suite creating it concurrently"), but only for idempotent creates — not a destructive drop.
packages/migrate/vitest.config.ts already sets fileParallelism: false with this exact justification. Same fix here, or give the live suites their own step.
3. Valueless --database-url still unguarded on the destructive subcommands
packages/cli/src/bin/main.ts:281
rejectMissingDatabaseUrlValue is wired into preflight and verify only. parseArgs booleanises --database-url when the next token starts with -, leaving values['database-url'] undefined — indistinguishable from "not passed" — so resolveDatabaseUrl silently falls through to DATABASE_URL and prints nothing.
Against the built CLI:
$ DATABASE_URL='postgres://…:59999/envdb' stash eql install --database-url --force
■ Fatal error: Failed to connect to database: connect ECONNREFUSED 127.0.0.1:59999
The valid path prints "Using DATABASE_URL from --database-url flag"; that line is the only tell, and it is absent here. --force then skips the isInstalled() early exit and runs DROP SCHEMA … CASCADE with no confirmation, never naming the target database.
Pre-existing rather than introduced here — this PR narrows it for two commands. But one await rejectMissingDatabaseUrlValue(flags) at the top of dispatch() covers every subcommand and replaces both per-case calls. A valueless --database-url is always a typo.
Minor
verify.live.test.ts has no afterAll, so the dropped >= (public.eql_v3_double_ord, …) operator and eql_v3.version() outlive the file. Low impact — the sibling live suites all tolerate a missing install, CI Postgres is a per-job container, and the next run's beforeAll reinstall repairs it. Still a one-line fix worth taking.
…live suites serialised, --database-url guard global - `stash eql install`'s surface check no longer exits 1 on a version mismatch: `ok: false` there means "nothing was checked", and the pre-verification behaviour of a no-op re-run over an older EQL was exit 0 — idempotent provisioning scripts and `stash init`'s direct-install route depend on that. Damage still fails the install; `stash eql verify` keeps its strict gate. The mismatch finding now also names `eql install --force --database-url ...` as the remedy that works without a stash.config.ts (`eql upgrade` requires one). New unit suite covers all four verifySurfaceOrExit outcomes. - The CLI vitest config now splits into `unit` and `live` projects, with `fileParallelism: false` on `live` only: four live suites share one database, and verify.live's bundle install opens with DROP SCHEMA ... CASCADE, which raced destructively under guarded-grants.live in parallel forks. The ~1300 unit tests keep their parallelism. verify.live also gained an afterAll reinstall so its surgical damage does not outlive the file. - The valueless `--database-url` rejection moved from the two diagnostic commands to the top of dispatch(), covering every subcommand — most importantly `eql install --force`, where the silent fallback to DATABASE_URL meant dropping and reinstalling the EQL schemas on a database the command never named. E2E-pinned. Claude-Session: https://claude.ai/code/session_01AwM5Cm5ddasXozb6stxPR1
|
All four findings addressed in 4bd720f. Finding 1 — version mismatch hard-failing the install. Finding 2 — live-suite races. Took the scoped version of the migrate fix: the CLI vitest config now defines two projects, Finding 3 — valueless Minor — no Validation: 1315 unit tests (including the 4 new gate tests), the four serialised live suites green against the local container, 111 e2e, biome clean. |
Fixes #890.
The gap
A partial EQL install — domains present, some of their comparison functions or operators absent — reported success at install time and failed at query time on a specific predicate (
weight >= x). Nothing detected it:isInstalled()is a presence test (do the two schemas exist), andeql validatechecks the columns an application declared, not whether the installed operator surface is internally coherent.stash eql verifyAsserts the installed EQL surface is complete and coherent, independent of any application schema:
parseExpectedSurface()parses the pinned@cipherstash/eqlinstall SQL into everything it creates unconditionally — 95 domains, 2 composite types, 3,266 function/aggregate overloads, 3,025 operators (identity = name + operand types), 1 cast. A bundle bump updates the expectation automatically; there is no hand-maintained list to drift. Dollar-quoted bodies are stripped first, which is also what keeps the bundle's DO-block conditionals out of the unconditional set.pg_catalogqueries;format_typenormalisation makes the catalog's spellings (_text,int8) meet the bundle's (text[],public.eql_v3_bigint). Runs in under a second._oredomains carrying theeql_ore_unavailablepoison CHECK is the supported managed-Postgres configuration and reads as info. The opclass absent with an incomplete fallback — or present with leftover poison — is damage.eql_v3_double_ord: Operator \>= (…)` is missing), exits 1.--jsonemits the structured report with astatusdiscriminator (complete/incomplete/not-installed/version-mismatch`) for agents.eql upgrade.stash eql installnow runs the same check before declaring success, so "install succeeded" means the full query-time surface is present, not just that the SQL committed.Example output (against a deliberately broken install)
Coverage
src/installer/__tests__/verify.test.ts, 18 tests): parser assertions against the real pinned bundle — including the issue's exact predicate operator, the quoted-name (eql_v3_internal."-") handling, and that the conditionalore_domain_unavailableis excluded — plus every differ classification (fallback vs incoherent ORE states, missing overloads, version mismatch, not-installed).verify.live.test.ts, gated onSTASH_TEST_DATABASE_URL): installs the real bundle, asserts the surface reads complete with exact full counts (proving the bundle/catalog spelling normalisation for every one of the 3,025 operators), then drops an operator andversion()and asserts the damage is named and attributed. This is the check no fake can provide.eql verifyadded to the smoke command list and--helpcoverage.Also: registry/manifest entry, help banner,
skills/stash-cli/SKILL.md(neweql verifysection + install self-verify note), and astashminor changeset.Notes for review
eql_v3_double_ordcase was not reproduced (likely the rolled-back install it was fighting), but the class of failure is now detected whatever the cause.eql installtreats a verification error (connection dropped, etc.) as a warning, not an install failure — the install itself committed.https://claude.ai/code/session_01AwM5Cm5ddasXozb6stxPR1