fix(db): model the managed engine's non-superuser provider locally, on PostgreSQL 17 by choraria · Pull Request #734 · webhook-co/webhook

choraria · 2026-07-21T19:01:29Z

Closes #728.

Stacked on #733. This branch is built on lane/nightly-rls-safety-and-reporting, so until
#733 merges this PR's diff shows both commits. Base is main rather than that branch because
on.pull_request.branches: [main] means a non-main base runs no CI at all — and the whole point
of #728 is a CI-verifiable claim. Merge #733 first; this PR's diff then narrows to its own commit
automatically. The two commits are separable in git log.

Every nightly-rls failure we have filed for a real bug has had one shape: something that
needs privileges the nightly's provider role does not have — #383 (TRUNCATE), #637/#688 (CREATE
TRIGGER), #717 (EXECUTE on a function revoked from PUBLIC). Each was invisible locally and in PR
CI, and each cost a red nightly plus a debugging round-trip.

The cause was never "Neon is different". The two lanes did not model the same database: locally the
provider was the postgres SUPERUSER, which bypasses every ACL check, so the local lane
structurally could not fail any of them. R1/R5 papered over the gap by pattern-matching shapes we
had already been bitten by — which is why R1 had to be generalised twice and still could not see
#717 (a plain select fn(), not DDL at all).

The change

packages/db/test/pg.ts provisions test_provider — NOSUPERUSER, BYPASSRLS, CREATEDB, CREATEROLE
plus pg_read_all_data/pg_write_all_data — and points providerUrl at it, on both lanes it
controls. It asks whether the role it was handed is a superuser rather than assuming: the
trust-auth CI service hands us postgres (downgrade), the Neon branch hands us a non-superuser
(leave entirely alone, zero change to the nightly's path).

Three corrections to the shape proposed in the issue — all measured, not reasoned:

CREATEROLE is required and was missing. bootstrapOwner's very first statement is
create role webhook_owner, which fails permission denied to create role without it. Verified
on a real PG17 cluster.
WITH INHERIT FALSE, SET FALSE is unnecessary. On PG16+ a CREATEROLE non-superuser that
creates a role is auto-granted it WITH ADMIN OPTION but without inherit and without
set — exactly Neon's measured inherit_option = f, set_option = f, for free. Granting it
explicitly with INHERIT would hand ownership rights straight back and defeat the exercise.
The provider must CREATE the per-file database. public is owned by pg_database_owner from
PG15, so grant all on schema public is a silent no-op — a WARNING, which postgres.js does
not reject — unless the grantor owns the schema, after which webhook_owner cannot create a
single table.

The role is named test_provider, deliberately outside the webhook_ namespace:
migrations.test.ts enumerates every webhook\_% role and asserts exact equality with
DB_ROLES-minus-owner. webhook_provider would break that assertion, and adding an exclusion to
keep it quiet would blunt the very guard it exists to be.

PostgreSQL 17

ci.yml (both service containers) and scripts/dev-db.sh move off 14. PG16 is the real technical
floor — measured on PG14:

PG14 attempt	result
`grant webhook_owner … with inherit false, set false`	syntax error (PG16+ only)
member of `webhook_owner`, INHERIT	`ALTER TABLE` succeeds, TRUNCATE passes the privilege check — bug class papered right back over
member of `webhook_owner`, NOINHERIT	`select from orgs` → permission denied — kills `pg_read_all_data` too

We require 17, not 16, so local matches CI and both Neon projects exactly.

Bumping CI does not bump local: the harness spawned initdb by name, and Homebrew links only one
major, so a machine with PG17 installed still resolved PG14. It now discovers a PG17 install
(TEST_PG_BINDIR overrides) and fails with instructions rather than silently downgrading.

Measured parity

Neon neondb_owner vs local test_provider, both PG17 — identical, down to the error wording:

operation	Neon	local
SELECT / INSERT / UPDATE / DELETE	ALLOWED	ALLOWED
TRUNCATE	`permission denied for table`	same
CREATE INDEX / ALTER TABLE / CREATE POLICY	`must be owner of table`	same
CREATE TRIGGER	`permission denied for table`	same
EXECUTE fn revoked from PUBLIC	`permission denied for function`	same
`pg_has_role USAGE` / `MEMBER`	false / true	same
DROP TABLE (owner-owned)	ALLOWED	ALLOWED

That last row is not a gap — it is faithful, and newly documented: the provider owns the database,
hence public, and a schema owner may drop objects within it. Both engines agree.

packages/db/test/provider-fidelity.test.ts asserts that table executably, on every PR — the
entire historical bug class, caught in under a second, with no shared resources, no credentials and
no nightly round-trip. It was RED on 10 of 12 assertions before this change.

R1 now tolerates an owner-only statement a test asserts is rejected (the same expected-rejection
spans R5 already used), because those denials are the proof and the rule must not flag its own proof.

Verification

full packages/db suite on the new provider — 102 files / 1568 tests green
apps' Neon suites — api 23, web 2 green
CI service-container lane simulated on a real PG17 trust-auth cluster: the harness notices the
superuser and provisions test_provider (rolsuper=f, bypassrls=t, createdb=t, createrole=t);
provider-fidelity + migrations.test.ts pass, and the webhook\_% role-drift guard is unbothered
pnpm lint exit 0 · typecheck 32/32 · prettier clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01YW2CU7RDhfaqsi2aZeNpEw

…d report nightly failures honestly Closes #727, #729, #730. ## #727 — the harness could not tell WHICH CLUSTER it was mutating R1–R5 in the remote-db guard constrain which ROLE may run a statement; none of them constrained which cluster it lands on. On the password path the harness unconditionally rotates CLUSTER-GLOBAL role passwords, `DROP DATABASE … WITH (FORCE)`s every `webhook_test_*` it can see, and (migrations.test.ts) rolls migrations down far enough to DROP those roles. Every environment shares those role names and every role create is `if not exists`-guarded, so a mis-pasted URL never failed early — it proceeded. `packages/db/test/expected-host.ts` refuses unless `TEST_DATABASE_EXPECTED_HOST` names the exact host. Unset ⇒ refuse, never default to allow. The message carries only the parsed host, never the URL. It is called from BOTH entry points, because the vitest globalSetup — not startEphemeralPostgres — is the first thing that can do damage: it opens its own client straight from TEST_DATABASE_URL and drops databases before any suite runs. Guard rule R6 pins the assertion's POSITION ahead of each file's first destructive statement (not merely its presence), and pins that turbo forwards the variable — Turbo 2.x runs in strict env mode, so an unforwarded variable would make every remote run fail closed. Local and trust-auth PR CI are untouched. Verified against the real CI Neon branch: a mismatched host and an unset expectation both refuse inside globalSetup before any drop; the matching host runs the suite green. ## #729 — stale claims, a false recovery path, and no runtime caps * Four comments said the nightly breaks "at 04:00". The cron has been `17 7 * * *` since the workflow was introduced. Replaced with the real cron AND a warning that GitHub queue-delays dispatch (observed runs reach ~4h), so no clock-derived assumption about branch availability is sound. The four legitimate ADR-0055 auth-sweeper 04:00 references are untouched. * "Disposable Neon branch" was false. Verified via the Neon API: the CI project has exactly ONE branch — primary, default, `parent_id: null` — so there is no `reset_from_parent` recovery path, which matters precisely because this harness wedges cluster-global role state. Corrected in both places and explained. * "webhook_owner/app/ingest" understated the blast radius: passwords are minted for all 18 DB_ROLES. Corrected in three places. * `timeout-minutes` was absent from all 40 jobs, so everything ran under the 360-minute default. Every job now declares a cap sized from measured durations (ci.yml jobs max ~5 min observed; nightly's suite ~115 min/attempt). `scripts/workflow-timeout-guard.mjs` PARSES the YAML (not a text scan) and enforces both halves, because the second is not obvious: a JOB timeout CANCELS the job, and `failure()` is FALSE for cancelled — so capping `rls-neon` without widening `open-issue-on-failure`'s gate would have converted a runaway into SILENCE. The guard was proved RED on that exact shape before the gate was widened. ## #730 — the failure report misled its own reader The classifier moved out of inline shell into `scripts/nightly-rls-classify.mjs` so it can be tested against real logs, with a committed excerpt of run 29819273539 as a fixture. * The signature is now scoped to vitest's "Failed Tests" section. It used to grep the whole log, so #717's auto-filed body led with two `permission denied for table …` lines emitted by the tolerated `auth.sweep.skipped` log that GREEN runs print too. * A third bucket: a `28P01` on a `webhook_*` role or a `3D000` on a `webhook_test_*` database is CONTENTION — another consumer touched the branch — not a certified real bug. Both markers are matched only inside the FAILING region, so noise from a passing negative-path test cannot excuse a genuine failure. * Attempt 2 is re-classified on its own evidence instead of being hardcoded "transient-looking". Running it over the real #717 log turns a 5-line signature (3 of them noise) into the two true lines, and flips the verdict from TRANSIENT — which would have re-run a 4-hour suite, because `CONNECT_TIMEOUT` appears in passing output — to NON-TRANSIENT. The transient check is scoped for the same reason, and the whole-log fallback now requires actual failure evidence: without that gate a green log re-published the tolerated noise as a "failure signature". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YW2CU7RDhfaqsi2aZeNpEw

…n PostgreSQL 17 Closes #728. Every nightly-rls failure we have filed for a REAL bug has had one shape: something that needs privileges the nightly's provider role does not have — #383 (TRUNCATE), #637/#688 (CREATE TRIGGER), #717 (EXECUTE on a function revoked from PUBLIC). Each was invisible locally and in PR CI, and each cost a red nightly plus a debugging round-trip. The cause was never "Neon is different". The two lanes did not model the same database: locally the provider was the `postgres` SUPERUSER, which bypasses every ACL check, so the local lane structurally COULD NOT fail any of them. R1/R5 papered over the gap by pattern-matching shapes we had already been bitten by — which is why R1 had to be generalised twice and still could not see #717 (a plain `select fn()`, not DDL at all). ## The change `packages/db/test/pg.ts` now provisions `test_provider` — NOSUPERUSER, BYPASSRLS, CREATEDB, CREATEROLE, plus pg_read_all_data/pg_write_all_data — and points `providerUrl` at it, on BOTH lanes it controls. It asks whether the role it was handed is a superuser rather than assuming: the trust-auth CI service hands us `postgres` (downgrade), the Neon branch hands us a non-superuser (leave entirely alone). Three corrections to the shape proposed in the issue, all measured rather than reasoned: * **CREATEROLE is required** and was missing. `bootstrapOwner`'s very first statement is `create role webhook_owner`, which fails "permission denied to create role" without it. * **`WITH INHERIT FALSE, SET FALSE` is unnecessary.** On PG16+ a CREATEROLE non-superuser that CREATES a role is auto-granted it WITH ADMIN OPTION but WITHOUT inherit and WITHOUT set — exactly Neon's measured `inherit_option = f, set_option = f`, for free. Granting it explicitly with INHERIT would hand ownership rights back and defeat the exercise. * **The provider must CREATE the per-file database.** `public` is owned by `pg_database_owner` from PG15, so `grant all on schema public` is a SILENT NO-OP — a WARNING, which postgres.js does not reject — unless the grantor owns the schema, after which webhook_owner cannot create a single table. The role is named `test_provider`, deliberately outside the `webhook_` namespace: `migrations.test.ts` enumerates every `webhook\_%` role and asserts exact equality with DB_ROLES-minus-owner. `webhook_provider` would break that, and excluding it would blunt the very guard it exists to be. ## PostgreSQL 17 `ci.yml` (both services) and `scripts/dev-db.sh` move off 14. PG16 is the real technical floor — on 14/15 there is no way to hold both `pg_read_all_data` and a non-inheriting `webhook_owner` membership: inheriting makes TRUNCATE and ALTER TABLE SUCCEED (papering the bug class straight back over), and NOINHERIT kills the predefined data roles so the provider cannot read at all. We require 17 so local matches CI and both Neon projects exactly. Bumping CI does not bump local: the harness spawned `initdb` by name, and Homebrew links only one major, so a machine with PG17 installed still resolved PG14. It now discovers a PG17 install (`TEST_PG_BINDIR` overrides) and fails with instructions rather than silently downgrading. ## Measured parity Neon `neondb_owner` vs local `test_provider`, both PG17 — identical, down to error wording: DML on owner-owned tables ........... ALLOWED / ALLOWED TRUNCATE ............................ permission denied for table (both) CREATE INDEX/ALTER TABLE/POLICY ..... must be owner of table (both) CREATE TRIGGER ...................... permission denied for table (both) EXECUTE fn revoked from PUBLIC ...... permission denied for function (both) pg_has_role USAGE / MEMBER .......... false / true (both) DROP TABLE (owner-owned) ............ ALLOWED / ALLOWED `provider-fidelity.test.ts` asserts that table executably on every PR — the entire historical bug class, caught in under a second, with no shared resources and no nightly round-trip. It was RED on 10 of 12 assertions before this change. R1 now tolerates an owner-only statement a test ASSERTS is rejected (the same expected-rejection spans R5 already used), because those denials are the proof and the rule must not flag it. ## Verification * full `packages/db` suite on the new provider: 102 files / 1568 tests green * apps' Neon suites: api 23, web 2 green * CI service-container lane simulated on a real PG17 trust-auth cluster: the harness downgrades itself and provisions `test_provider` (rolsuper=f, bypassrls=t) * `pnpm lint` exit 0, `typecheck` 32/32, prettier clean Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YW2CU7RDhfaqsi2aZeNpEw

…ts found in review Follow-up to the same PR. An adversarial review of this branch confirmed 26 findings; these are the substantive ones, each reproduced before being fixed and mutation-checked after. ## The guard exempted far more than it meant to (#727) The exemption keyed on AUTH MODE, so ANY trust-auth URL skipped the host check — not just loopback. Reproduced end to end on a LAN address: with `TEST_DATABASE_URL=postgres://postgres@192.168.1.70:54399/postgres` and `TEST_DATABASE_EXPECTED_HOST` naming a completely different cluster, a full migrations.test.ts run created and then DROPPED 19 cluster-global roles on the unnamed host — and passed, because its assertion is that the roles are gone. The exemption is now LOOPBACK ONLY: 127.0.0.0/8, ::1, localhost. That still covers both fast lanes (the local ephemeral cluster and the CI service container are both on loopback) and nothing else. Re-running the same attack now refuses in globalSetup with 0 roles created; the control (same host, correctly named) still runs green. Two related bypasses closed at the same time, because the parsed host must be the host the DRIVER dials: * postgres.js splits userinfo at the FIRST `@`, WHATWG at the LAST — an unencoded `@` in a password makes the assertion talk about one host while the client connects to another. * a libpq comma-separated host list parses as a single hostname here, so which cluster gets mutated is not determined by the check. Both now refuse. Hosts are also compared case-insensitively and modulo a trailing dot, so an equivalent spelling neither false-refuses nor slips past. ## The classifier fell back to unscoped more often than it scoped (#730) * **`Failed Suites`.** vitest prints that heading — not `Failed Tests` — when a suite-level hook throws, and every provisioning hook in this repo lives in `beforeAll`. Matching only `Failed Tests` sent all of those runs down the whole-log path, re-publishing the tolerated `auth.sweep.skipped` noise as the signature. The banner now matches either. * **turbo's line prefix.** `pnpm test:db` is two turbo invocations; the apps' one prefixes every line with `<pkg>:<task>: `, which defeats every `^`-anchored pattern. Stripped. * **`gh run view --log` prefix.** Stripped too, so the documented triage path — download the log, run the classifier over it — actually works instead of silently returning the fallback. Verified against the raw 1.5 MB download of run 29819273539. * **Signature ordering.** `FAIL <file>` sorts before `PostgresError:`, so once 8 distinct files failed the cap was spent entirely on filenames and the issue body carried no error at all — the one thing not already recoverable from the run. Error shapes now take their slots first. ## The runtime caps (#729) * `always()` in the widened gate filed an issue whenever a run was cancelled — including a supersede by the concurrency group or a human hitting stop. `!cancelled()` skips those while still reporting a job killed by its own cap. * A STEP timeout kills the shell, so the classifier never runs and the job published an empty classification that read as an infra fault. A new step names it TIMED-OUT instead. The comment claiming the step cap "reports through the normal failure path with a real classification" was simply false and is gone. * `${RUN_ID}` was interpolated into the CANCELLED text where a duration belonged. * The guard's `cancelled` check was a substring test, so `!= 'cancelled'` and `!cancelled()` — which EXCLUDE the case — read as compliance. It is now an affirmative, per-dependency `needs.<dep>.result == 'cancelled'`. * A job that calls a reusable workflow cannot legally carry `timeout-minutes`, so demanding one would make the guard unsatisfiable. In-repo callees are exempt (their own jobs are capped by this same guard); out-of-repo ones are still flagged. Every fix is pinned by a test, and each test was mutation-checked: reverting the fix turns it red. Also corrects the header's claim that a missing expected-host variable "skips cleanly" — it fails the job, deliberately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YW2CU7RDhfaqsi2aZeNpEw

…y-rls-pg17-provider-fidelity

…ed cluster start Follow-up to the same PR, from adversarial review — and confirmed independently by CI's own `ai-review`, which BLOCKed on the first of these. ## The password-auth lane was broken, and nothing covered it The harness has three lanes; CI exercises two. When it is handed a SUPERUSER over password/SCRAM auth it provisions `test_provider` and connects as it — but per-run passwords were minted only for MANAGED_ROLES (= DB_ROLES), and `test_provider` is deliberately outside that set. So its connection URL carried no password and the run died `28P01 password authentication failed for user "test_provider"` before the first migration. That lane is not hypothetical: it is exactly what packages/db/README.md tells you to use to validate password mode locally. The trust-auth CI service and the Neon non-superuser path both skip the provisioning branch entirely, so neither could ever catch it. `provisionProviderSql()` now takes a password and sets it UNCONDITIONALLY — the role is cluster-global and outlives a single test file, so one left behind by a previous run still carries that run's password, the same reason bootstrapOwner resets the owner's every time. It stays out of MANAGED_ROLES: managed-roles.test.ts pins MANAGED_ROLES === DB_ROLES, and applyRolePasswords connects AS this role, so it cannot be in that loop. `packages/db/test/password-mode.test.ts` covers the lane for the first time — it spins a real SCRAM cluster rather than mocking, because the thing under test IS the authentication handshake. Mutation-checked: remove the minted password and it fails with that exact 28P01. ## A failed local cluster start was unreadable `pg_ctl` exiting non-zero told you nothing, and a version- or permission-blocked start is precisely the case the new version floor exists to make legible. The postmaster's log is now read BEFORE stop() deletes the data dir, and its last 20 lines are attached to the error. ## Smaller * `TEST_PG_BINDIR` added to turbo's passThroughEnv on BOTH test tasks — Turbo runs in strict env mode, so without it the override silently did nothing for the apps' suites. * `scripts/dev-db.sh` single-sources `PG_MAJOR`, so the path and the error message cannot drift apart. * Two comments described the local provider as a superuser in the PRESENT tense. Since this PR that is history, and the mistakes they warn about now fail locally too — corrected, with the history kept because it explains why the warnings exist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YW2CU7RDhfaqsi2aZeNpEw

…st where a cluster can be spawned CI caught a real gap in the previous commit's test. `password-mode.test.ts` spins its own SCRAM cluster, which needs local Postgres BINARIES — and the `test-db` job has a postgres SERVICE CONTAINER and no local install, so `pgBinDirForTests()` threw and the file failed (103/104 files passed; only this one, and only for that reason). Split by what each half actually needs: * `provisionProviderSql()` is now exported and asserted directly — that it emits `ALTER ROLE "test_provider" LOGIN PASSWORD` when given one, omits it under trust auth, escapes a quote rather than breaking out of the literal, and sets it OUTSIDE the `if not exists` guard so a cluster-global role left by a previous run is re-credentialed. These run in EVERY lane and pin the actual defect. Mutation-checked with no binaries reachable: dropping the clause turns them red there too. * Only the handshake half — which genuinely cannot run without binaries — is gated, on a capability probe (`hasLocalPostgresBinaries()`), not on failure. It runs on every developer machine, and it is where the `28P01` was caught in the first place. Worth stating plainly since the repo forbids weakening tests to make CI pass: nothing here is a failing test being hidden. The defect is covered everywhere; one lane cannot perform the authentication handshake because it has no server to spawn. Noted in passing: the failure surfaced under vitest's `Failed Suites` heading — the exact section the classifier in #733 could not previously see. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YW2CU7RDhfaqsi2aZeNpEw

#733 landed as a SQUASH, so its commits are not ancestors of main and every file both PRs touched conflicted as 'same change applied twice'. Resolved by keeping this branch's side throughout — it already contained #733's final state (merged in at e921600) plus this PR's own changes on top, so main had nothing it lacked. Verified: the resolved tree is byte-identical to the pre-merge branch, and the diff against main is now exactly #728's 13 files. Full gate re-run on the merged tree: lint clean, 464 script tests, 104 db files / 1589 tests.

choraria and others added 2 commits July 21, 2026 19:43

github-actions Bot assigned choraria Jul 21, 2026

choraria changed the base branch from lane/nightly-rls-safety-and-reporting to main July 21, 2026 19:03

choraria mentioned this pull request Jul 21, 2026

harness fidelity: the local provider role is a superuser, which is why the whole provider-vs-owner class only ever surfaces at night #728

Closed

choraria closed this Jul 21, 2026

choraria reopened this Jul 21, 2026

choraria and others added 5 commits July 21, 2026 21:04

Merge branch 'lane/nightly-rls-safety-and-reporting' into lane/nightl…

e921600

…y-rls-pg17-provider-fidelity

choraria merged commit 2b07e2e into main Jul 21, 2026
51 of 52 checks passed

choraria deleted the lane/nightly-rls-pg17-provider-fidelity branch July 21, 2026 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(db): model the managed engine's non-superuser provider locally, on PostgreSQL 17#734

fix(db): model the managed engine's non-superuser provider locally, on PostgreSQL 17#734
choraria merged 7 commits into
mainfrom
lane/nightly-rls-pg17-provider-fidelity

choraria commented Jul 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

choraria commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The change

PostgreSQL 17

Measured parity

Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

choraria commented Jul 21, 2026 •

edited

Loading