fix(ci): fail closed on an unexpected test cluster, cap every job, and report nightly failures honestly#733
Merged
Conversation
…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
This was referenced Jul 21, 2026
Closed
…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
choraria
added a commit
that referenced
this pull request
Jul 21, 2026
…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
choraria
added a commit
that referenced
this pull request
Jul 21, 2026
#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
added a commit
that referenced
this pull request
Jul 21, 2026
…n PostgreSQL 17 (#734) * fix(ci): fail closed on an unexpected test cluster, cap every job, and 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 * fix(db): model the managed engine's non-superuser provider locally, on 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 * fix(ci): close the target-cluster hole and the classifier's blind spots 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 * fix(db): give the provisioned provider a password, and surface a failed 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 * fix(db): pin the provider-credential regression in every lane, not just 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 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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 #727, closes #729, closes #730.
Three of the four
nightly-rlsissues. They all land together because they sharenightly-rls.yml, and because #729's runtime cap is only safe alongside #730's honestreporting. #728 (PG17 + a non-superuser local provider) follows in its own PR.
#727 — the harness never checked which cluster it was mutating
R1–R5 constrain which role may run a statement. Nothing constrained which cluster
it lands on. On the password path the harness unconditionally:
DB_ROLES— and roles are cluster-global;DROP DATABASE … WITH (FORCE)s everywebhook_test_*it can see;migrations.test.ts, rolls migrations down far enough to DROP those roles.Every environment uses the same role names and every role create is
if not exists-guarded,so a mis-pasted URL never failed early — it proceeded and landed its damage.
packages/db/test/expected-host.tsrefuses unlessTEST_DATABASE_EXPECTED_HOSTequalsthe URL's host exactly. Unset ⇒ refuse, never default to allow. Errors carry only the parsed
host, never the URL.
Called from both entry points — the vitest
globalSetup, notstartEphemeralPostgres,is the first thing that can do damage: it opens its own client straight from
TEST_DATABASE_URLand 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 + trust-auth PR CI are untouched and cost nothing.
Verified against the real CI Neon branch:
globalSetup, before any dropThe ordering test is mutation-checked: remove the guard and it fails with
getaddrinfo ENOTFOUND, proving the assertion really does short-circuit before the firstconnection rather than merely throwing eventually.
#729 — stale claims, a false recovery path, no runtime caps
17 7 * * *sincethe workflow was introduced —
git log -Sshows only one cron line was ever added. Replacedwith the real cron and a warning that GitHub queue-delays dispatch (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.
exactly one branch —
primary,default,parent_id: null— so there is noreset_from_parentrecovery path, which matters precisely because this harness wedgescluster-global role state. Corrected in both places, with the reason.
18
DB_ROLES. Corrected in three places.timeout-minuteswas absent from all 40 jobs. Every job now declares a cap, sized frommeasured durations (ci.yml jobs max ~5 min observed; the nightly's suite ~115 min/attempt →
step 300, job 320).
scripts/workflow-timeout-guard.mjsparses the YAML (not a text scan) and enforces bothhalves — because the second is not obvious:
So capping
rls-neonwithout wideningopen-issue-on-failure's gate would have converted arunaway into silence — strictly worse than the runaway. The guard was proved RED on
exactly that 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.mjsso it can betested against real logs, with a committed excerpt of run 29819273539 as a fixture.
log, so nightly-rls failed on 2026-07-21 (run 29819273539) #717's auto-filed body led with two
permission denied for table …lines emittedby the tolerated
auth.sweep.skippedlog that green runs print too.28P01on awebhook_*role, or a3D000on awebhook_test_*database, is CONTENTION — another consumer touched the branch — not a certified real
bug. Both markers match only inside the failing region, so noise from a passing
negative-path test cannot excuse a genuine failure.
"transient-looking".
Run over the real #717 log, before → after:
Two defects found only because the fixture is real, not hand-written:
scrape silently fell back to the unscoped path.
gh run view --logrenders the escape asthe literal two chars
^[, which is a second form worth handling.CONNECT_TIMEOUTappears in this log's passingoutput, so a deterministic failure classified as TRANSIENT and asked to re-run a 4-hour
suite. Now scoped.
Also: the whole-log fallback now requires actual failure evidence — without that gate a green
log re-published the tolerated noise as a "failure signature" (caught by its own test).
Verification
pnpm lintclean (0 errors) — includes the newworkflow-timeout-guardand R6.pnpm run test:scripts— 450 passed.pnpm run typecheck— 32/32.prettier --check .clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01YW2CU7RDhfaqsi2aZeNpEw