EQL install: defer owner-scoped Supabase grants for non-postgres roles; add stash eql preflight - #902
Conversation
… add `stash eql preflight` Fixes the Lovable-class install failure (#887) and adds the read-only role preflight it called for (#888). - Split the Supabase grants (installer/grants.ts) into plain-GRANT and owner-scoped ALTER DEFAULT PRIVILEGES halves. SUPABASE_PERMISSIONS_SQL_V3 stays byte-identical (locked by a unit test) so the live proof in stack-supabase's grants integration test still covers the shipped SQL. - EQLInstaller.install(): the bundle commits in its own transaction; grants run after it, so one refused grant no longer rolls back ~194 functions. When the connecting role is not a member of postgres, the owner-scoped statements are skipped and returned (InstallResult.deferredGrantsSql) with a header explaining what to do; eql install / eql upgrade print them under 'Deferred SQL — run as postgres'. Failure messages now state recoverability in both directions. - EQLInstaller.preflight() replaces checkPermissions(): one guarded catalogue query also reporting current_user, membership of postgres (null when no postgres role exists — pg_has_role raises otherwise, proven by a live test), and EQL v3 schema presence. - New 'stash eql preflight' command (--json for agents, pure-JSON stdout via a new quiet resolver option), also run at the head of eql install. - Skills: stash-cli (new command, deferral semantics, API surface) and stash-supabase (non-postgres-role note) updated; changeset added.
🦋 Changeset detectedLatest commit: 1c5bea3 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 |
freshtonic
left a comment
There was a problem hiding this comment.
Request changes — the implementation is solid and I'd approve on the strength of it, but one shipped-artifact defect needs a fix first.
Blocking
skills/stash-cli/SKILL.md — the eql install flags table is broken. The new Non-postgres roles paragraph was inserted between the --supabase and --database-url rows, so the --database-url <url> | One-shot install (see below) row is orphaned below a paragraph and no longer renders as a table row — it comes out as a stray literal pipe-delimited line. Verified on the branch:
| `--supabase` | Supabase-compatible install; … |
**Non-`postgres` roles.** The Supabase grants include three owner-scoped …
| `--database-url <url>` | One-shot install (see below) |
Move the paragraph below the complete table (after the --database-url row). Per AGENTS.md these files ship in the stash tarball and get copied into customer repos, so a rendering defect here is a code defect. One-line fix.
What I verified (all good)
- Byte-identity lock: the unit test pins
SUPABASE_PERMISSIONS_SQL_V3to the exact pre-split block, and the partition test proves every statement lands in exactly one half with the right shape (GRANTvsALTER DEFAULT PRIVILEGES FOR ROLE postgres). The live Supabase grants integration test therefore still proves the shipped SQL. - Commit-then-grants ordering: the bundle commits in its own transaction before any grant runs; the grant-failure test asserts
COMMIThappened andROLLBACKdidn't, and both failure messages state recoverability in the right direction. pg_has_roleCASE guard: genuinely load-bearing (it raises on a missing role rather than returning false), and the live test covers all three membership arms — including the compose image that bootstraps with nopostgresrole.- Preflight UX: superuser suppresses privilege annotations (tested),
memberOfPostgresnever blocks,--jsonstdout stays pure via the newquiettier on the URL resolver, and the resolver falls back cleanly when nostash.config.tsexists. - Meta: registry entry, help/smoke e2e coverage, changeset (
stashminor), and both skill files updated. CI fully green including the Supabase integration suites.
Fix the table placement and this is ready.
… deferred work Review feedback on the deferral design: every path that creates new EQL objects through stash tooling re-grants them itself — install --force and eql upgrade re-run the blanket grants, and the generated Supabase migration embeds them alongside the bundle. The ALTER DEFAULT PRIVILEGES FOR ROLE postgres statements therefore only cover objects postgres might create in the EQL schemas outside stash tooling, and on platforms where no operator can act as postgres (Lovable) they can simply never be applied without anything being lost. Reword accordingly: the install reports itself complete, the skipped SQL prints under 'Optional SQL — requires postgres' with the header explaining when it would matter, the pre-install notice drops from warn to info, and the preflight row annotation says 'skips optional' rather than 'blocks'. Skills and changeset updated to match. No behavioural change to what runs against the database.
|
Review findings Analysis performed by UGPT-5.6 Solv.
|
…ht, JSON contract, deprecated alias
Review findings from James, the Codex review, and /code-review, all fixed:
- Generated Supabase migrations no longer die for non-postgres roles: the
owner-scoped ALTER DEFAULT PRIVILEGES statements ship inside a
pg_has_role-guarded DO block (SUPABASE_MIGRATION_GRANTS_SQL_V3), so a
migration applied by Lovable's sandbox_exec skips them instead of rolling
back the whole file. Live-proven in both membership arms.
- PREFLIGHT_SQL guards has_schema_privilege against a dropped public schema
(it raises 3F000, and the combined query took superusers down with it),
reports pgcrypto's schema (outside extensions/public the bundle aborts —
blocks even superusers), and reports drop-ownership of existing EQL
schemas (reinstall opens with DROP SCHEMA ... CASCADE). Query-phase
failures are now labelled as preflight failures, not connect failures.
- eql install on an already-installed Supabase database re-applies the role
grants (new applySupabaseGrants(), idempotent) instead of early-exiting,
so a grants failure after the committed bundle heals on a plain re-run.
- eql preflight --json keeps stdout pure JSON in every outcome: blockers are
status 'blocked' (not 'ok'), and URL-resolution failures use the shared
{ status: 'error', code, message } envelope via a new jsonErrors resolver
option. Human mode resolves the URL before any spinner exists (the
interactive prompt was being redrawn over), and both modes warn when a
config literal databaseUrl overrides --database-url, matching install.
- No breaking change at 1.0: checkPermissions() stays as a deprecated
adapter over preflight() and PermissionCheckResult is still exported, so
the changeset remains minor.
- Skills: the Non-postgres-roles paragraph no longer splits the eql install
flags table (James's blocker); API/JSON-contract docs updated. README's
EQLInstaller section rewritten for the current surface.
Both preflight.live and guarded-grants.live create the postgres role when the compose database lacks one, and vitest runs the files in parallel workers against the same database — the check-then-CREATE raced (pg_authid_rolname_index duplicate key, observed locally). Creation is now a duplicate_object-tolerant DO block in both files, neither drops the role afterwards (the compose database is ephemeral, and a drop from one file races the other's queries), and the missing-role assertion samples role existence on both sides of the probe so a mid-probe appearance accepts either membership arm. Three consecutive full live runs green.
|
All review feedback addressed in 54832b5 + 1c5bea3. @freshtonic's blocker — fixed: the Non- Codex review (posted by @coderdan):
Further /code-review findings, also fixed:
Verification: 1296 unit+live tests (including two new live suites proving the guarded DO block and the hardened preflight SQL against real Postgres), 109 e2e, |
freshtonic
left a comment
There was a problem hiding this comment.
Approve. The blocking issue from my previous review is fixed, and the follow-up commits go well beyond it — I re-reviewed the full delta (e68879b4..1c5bea39), not just the fix.
The blocker, resolved
The eql install flags table in skills/stash-cli/SKILL.md is intact again — --database-url is back inside the table and the Non-postgres roles paragraph sits below it. Verified on the branch.
The rest of the delta, reviewed
- Guarded migration grants (
SUPABASE_GUARDED_DEFAULT_PRIVILEGES_SQL_V3/SUPABASE_MIGRATION_GRANTS_SQL_V3): the right fix for the gap the first revision left open — a generated--supabasemigration previously shipped the unguarded owner-scoped statements and would have aborted (and rolled back the whole file) under exactly the Lovable-style role this PR exists to serve. TheDO $$ … pg_has_role … $$guard is live-proven in both membership arms viapg_default_aclcounts, and the unit test asserts no bareALTER DEFAULT PRIVILEGESline survives in the migration block. - "Optional", not "deferred": the reframing is honest and materially better UX — stash re-grants every object on each install/upgrade, so the owner-scoped statements genuinely aren't owed work, and telling a Lovable operator to "run this as postgres" when nobody can act as postgres was a dead end.
- Heal-on-rerun: the already-installed path now re-applies the grants via
applySupabaseGrants(), closing the hole where a committed-bundle-but-failed-grants database would previously early-exit past its missing grants forever. checkPermissions()restored as a deprecated adapter with its shape unchanged and a test pinning it — the accidental break for existingstash@1.xconsumers is gone, and the changeset now says so accurately.- Preflight hardening: the
has_schema_privilegeguard for databases withoutpublic(3F000), pgcrypto placement (blocks even superusers, correctly ungated), and theDROP SCHEMA ... CASCADEownership probes are each guarded in SQL so a missing object reads as an answer rather than a query failure. Verifiedpg_has_role(name, oid, text)is a real overload for thenspownerprobes, and the live suite asserts probes answer rather than raise. - JSON contract:
status: 'ok' | 'blocked'as the agent-facing discriminator (blockers no longer masquerade asok), the sharedemitJsonErrorenvelope on every failure path including unresolvable DATABASE_URL, and the--database-url-vs-config-literal precedence warning routed to stderr in json mode.loadStashConfig'sknownConfigPathsecond parameter and thejsonErrorsresolver option are wired correctly. - De-raced live suites: leaving the shared
postgresrole behind and sampling existence on both sides of the probe is the pragmatic call for an ephemeral compose database.
CI is fully green on 1c5bea39. Two non-blocking observations: the live suites are STASH_TEST_DATABASE_URL-gated and not wired into any CI workflow (consistent with applied.live.test.ts, but the guarded-grants proof only runs when someone remembers to); and renderPreflightReport restates the supported pgcrypto schemas as a literal rather than sharing SUPPORTED_PGCRYPTO_SCHEMAS. Neither needs to hold this up.
Fixes #887, fixes #888.
Reported by a Lovable agent whose database role is
sandbox_exec— notpostgres, and not a member of it. One owner-scoped grant rolled back the entire EQL install, and nothing could predict it beforehand.What changed
The install no longer dies on the owner-scoped grants (#887)
packages/cli/src/installer/grants.tssplits each Supabase grants block into a plain-GRANThalf and an owner-scopedALTER DEFAULT PRIVILEGES FOR ROLE postgreshalf.SUPABASE_PERMISSIONS_SQL_V3remains byte-identical — locked by a new unit test — sopackages/stack-supabase/integration/grants.integration.test.tsstill proves the exact shipped SQL.EQLInstaller.install()now commits the bundle in its own transaction and runs the grants after it. A grants failure can no longer roll back a working install.postgresis checked (guarded —pg_has_roleraises when the role doesn't exist) before the owner-scoped statements. Non-member: the plainGRANTs run, covering all existing objects, and the install reports itself complete. The skipped statements print under "Optional SQL — requires postgres" — optional because they only cover EQL objectspostgresmight later create outside stash tooling: everystash eql install/eql upgradere-runs the blanket grants, and the generated Supabase migration embeds them alongside the bundle. On platforms where nobody can act aspostgres(Lovable), nothing is lost by never applying them.stash eql preflight(#888)Read-only role-capability report, exposed as a command and run at the head of
eql install:postgresnever blocks — anorow is annotated as skipping the optional statements, not as a blocker.--jsonfor agents — stdout stays pure JSON via a newquietoption on the DATABASE_URL resolver (suppresses info chrome and the interactive prompt tier).stash.config.ts(falls back to the plain resolution chain), since preflight runs before anything is set up.EQLInstaller.preflight()replacescheckPermissions();eql statususes it too.Verification
postgres-eql:17): the preflight SQL in both membership arms (member_of_postgresnull / false / true), a fulleql install, and a Supabase-mode--forceinstall exercising the deferral end-to-end (deferred SQL printed, install kept).eql preflight --helpcovered.stash-cli(new command, deferral semantics,EQLInstallersurface) andstash-supabase(non-postgres-role note) updated; command names verified againststash manifest --json.Groundwork: the immediate/deferred split is what #886 (
--print-sql) needs to label the tail.