[antoinetoussaint-byte]

Issue #1293 (reopened), post-merge adversarial review r54 of #1294 (merged ad81f71). No closing keyword: whether the issue is done is the owner's call, not this PR's.

Migration 65 (#1294) renamed the two RLS scope settings onto the platform Postgres library's names in one step. A one-step rename of a scope setting is not deployable: the catalog and the binary that binds it ship separately, and the predicate is col::text = current_setting(<name>, true), so whichever side loses the race reads an unset setting — zero rows on every scoped SELECT, a WITH CHECK failure on every INSERT, and nothing raised anywhere. This delivers the transition that was missing, and the four smaller findings from the post-merge review of that PR.

Classification: fix. Every change sits at the place that owns the behaviour — the ledger owns the predicates, pkg/infra owns the binding and the startup refusal, module/tools owns the gate and the recipe a consumer runs. No stopgaps, nothing labelled temporary except the compatibility window itself, whose closing release is documented and whose text is already committed and proved as 66_scope_settings_transition.down.sql.

Reuse. The rewrite is one artifact used three ways: module/tools/rls-scope-settings-transition.sql is the file a consumer runs, store migration 66 is that file byte for byte, and the down half is the later migration that closes the window. It is catalog-driven rather than generated, so it reaches what no generated file here could name — a policy whose name depends on the month (the audit_events partitions) and a consumer's own policies — and it reuses the ledger's existing idioms rather than adding mechanisms: ALTER POLICY instead of a DROP/CREATE pair, migration 64's replace-as-owner dance for a routine the baseline reassigned, and audit_events' own partition policies as ordinary pg_policy rows. The startup refusal reuses the construction-time refusal seam NewPostgresStoreWithCapabilities already applies to a misprovisioned login; no health endpoint, no second probe. What I deliberately did not reuse: scopedpostgres' own scope-setting default, which is exactly what F3 is about.

Review r54

F1 — major — version skew silently empties every tenant table. Fixed.

• module/services/store/migrations/66_scope_settings_transition.up.sql rewrites every policy predicate and routine body to COALESCE(NULLIF(current_setting('codefly.current_tenant_id', true), ''), current_setting('app.current_org_id', true)), and the same for the user pair. Fail-closed is preserved exactly: both arms empty is a false predicate, not an absent one. It is catalog-driven and idempotent, bounded by SET LOCAL lock_timeout = '15s', and uses ALTER POLICY, which cannot restate a policy's kind, verb or role list.
• pkg/infra.bindRequestScopeSQL binds all four names from the new LegacyTenantScopeSetting / LegacyUserScopeSetting constants, so one image is correct against a catalog from before the rename, after it, or inside the window. Two other sites that bind or check a scope now cover both generations: the effect-receipts tenant scope reuses the one binding instead of its own statement, and the release reset (sessionReset) reads back all four names, because inside the window a session-level binding on the retired half hands the next borrower a scope.
• pkg/infra.requireCatalogReadsScopeSettings refuses to construct the store against a database whose policies read only the retired names. It is a refusal rather than a warning for a reason worth stating: the host can bind both generations on its own request pool, but the two pools service-postgres owns bind the library's two names and take no more, so against such a catalog every read through them is silently empty and no binding here can fix it — a started process would report healthy and answer nothing. It asks the catalog what the policies judge by, not the ledger what version it claims.
• Proof, on real PostgreSQL 16 clusters: TestMigration66OpensTheScopeSettingCompatibilityWindow stands a database at 65, asserts the skew exists as described (a transaction binding the retired name is admitted to nothing), applies 66, and requires that the catalog changed in exactly that one way and no other (fingerprint equality against an independently re-derived substitution over kind, verb, roles, owners and ACLs); that a transaction binding either generation sees its own organisation and no other; that the library's name wins over a stale retired value; that an unbound transaction and one with both halves bound empty see nothing; that the WITH CHECK half admits a write for its own tenant and refuses one for another (a count, because a refused UPDATE under forced RLS is not an error); that the down half restores 65's catalog exactly and replay reproduces the window; and that applying the up half twice is the same catalog.
• Readiness proof: TestScopeSettingRefusalReadsTheCatalogsVersion is the decision over a catalog at each version — 64 (retired names alone) refused, 65 and 66 served, a half-applied rewrite refused and naming the policy left behind, an empty catalog refused. TestCatalogScopeSettingCheckReadsTheLiveCatalog drives the real query against the live catalog: the frontier serves, and a policy staged in a rolled-back transaction with migration 64's own predicate shape refuses with the remedy named.
  Not done as literally specified: I did not boot an accounts process against a full version-64 ledger. The refusal is per policy, so version 64 is seventy-five instances of the one shape the DB test stages and the decision table covers; standing a second cluster and a second ledger to boot a process against it would prove the same rule over more rows. Say the word and I will add it to the store-side replay suite, which is the one place with a container runtime.
• Closing the window is documented as a release of its own, in module/services/store/migrations/README.md § The compatibility window, and how to close it: its migration is the text of the down half, renumbered above the frontier, cut only once no deployable image binds the retired names, and it must not ship beside 66 — together they are the one-step rename again.

F2 — major — consumer modules on app.current_org_id go silently empty; no breaking-change entry, no recipe. Fixed.

• module/module.package.codefly.yaml: a migrations row (scope-settings-transition, from: ">=0.1.0 <0.2.0", breaking) and a breaking-changes entry that states the failure mode in the words it actually appears in (every such table reads as empty, every INSERT fails its WITH CHECK, nothing raised) and names both remedies.
• The recipe is committed as module/tools/rls-scope-settings-transition.sql, under an artifact root that ships. It is one rewrite that reaches the same end state from either side of the rename: a predicate already in the window is left alone, one on the library's name is wrapped, and one still on the retired name is wrapped to the same text — which is also that consumer's rename. module/tools/rls_scope_settings_recipe_test.go holds the migration to the recipe byte for byte (and refuses a migration that appends a body of its own after it), and holds the recipe to the four names, the NULLIF, the lock bound and ALTER POLICY.
• module/tools/rls-migration-gate.mjs: the refusal now names the remedy — rewrite onto the two library names, or run the recipe over a database whose policies are already installed — with two new gate tests, one for the retired-name refusal and one proving the compatibility read is admitted.

F3 — minor — the scoped pools coupled to an unexported library default. Fixed. openScopedBoundaryAs passes scopedpostgres.WithScopeSettings(TenantScopeSetting, UserScopeSetting) again. Behaviour is unchanged today — they are the library's own defaults — and that is the point: the default is unexported, its doc comment in the published library still says app.current_tenant_id, and nothing tied it to the constant the policies, the request binding and the startup check all read. The comment that argued the override would be "a second place to keep the names in step" is replaced by the opposite argument, which is the one the published library's own stale doc comment makes for us.

F4 — minor — migration 65 takes AccessExclusiveLock on 70 relations with no lock_timeout. Fixed forward, not in 65. A shipped migration is immutable here — module/services/store/migrations/README.md says so and migration-reference-gate.mjs enforces it against the base SHA, so an edit to 65 would be refused by CI and would in any case do nothing for a database that already applied it. The bound lives where the same locks are taken again: 66 opens with SET LOCAL lock_timeout = '15s' and rewrites with ALTER POLICY rather than a DROP/CREATE pair, and it calls no partition helper at all (the partitions' policies are pg_policy rows the walk already reaches, which is also why no deadline is needed around audit_events_secure_partition_within). The README now carries the rule for the next bulk migration: an unbounded wait on seventy relations turns a deploy into an outage, a bounded one fails and is retried.

F5 — minor — AGENTS.md names a test that does not exist. Fixed. module/services/accounts/AGENTS.md now names TestNoTenantRebindingOutsideInfra. The CLAIM_INVENTORY.md row for the rename did not name it, so nothing else needed correcting; module/DATABASE_AUTHORITY.md already had it right and is updated for the widened scan.

F6 — minor — the source scan no longer flags the retired names. Fixed. The rebinding rule is (?i)\b(?:app|codefly)\s*\.\s*current_, so a statement outside pkg/infra that binds or reads either generation is refused — which matters more now, not less, since the retired name is readable again for the length of the window. TestTenantRebindingScanSeesEveryForm gains a set_config('app.current_org_id', …) case, and TestNoTenantRebindingOutsideInfra passes over the tree.

The release-reset test no longer waits for a backend

TestRequestConnectionCarriesNoBindingAfterRelease failed once in the first full run of the accounts DB suite and passed in isolation and on the re-run. Its old shape released the poisoned connection to the shared pool and then waited up to five seconds for that backend's PID to reappear among the idle connections, which is two conditions at once: the reset must have cleaned the session, AND nothing else in the package may hold every request connection for the length of the wait. The second is not a property of this code — one background dispatcher is enough to fail it — and a failure could not say which half it was.

Asserted without requiring that backend to come back. The test now drives the pool's own AfterRelease judgement (returnsToTenantRole, exposed through export_test.go) on the connection it poisoned, and asserts both halves there: the judgement returns true, which is the verdict the pool acts on — true puts the connection back, false destroys it — and the session that the next borrower would be handed carries none of the four names and is back to app_tenant. No wait, no PID matching, no shared-pool timing; it runs in 0.00s where the old one could take five seconds. I widened rather than removed: the poison is now set on all four names, because inside the window a predicate falls through to the retired name when the live one is empty, so a session-level binding on either is the previous caller's scope.

Both halves are proved load-bearing by mutation rather than assumed. Replacing RESET ALL with targeted clears of the two library names only — same statement count, so the reset still succeeds — leaves the retired poison behind: with the read-back this PR ships, the verdict assertion fires; with the pre-change two-name read-back, the verdict passes and the four-name assertion fires naming app.current_org_id. Dropping RESET ALL outright fires the verdict assertion too.

Verified

• node module/tools/migration-reference-gate.mjs origin/main --replay — the whole migration suite on real PostgreSQL 16 clusters, green in 1212s: TestMigration66OpensTheScopeSettingCompatibilityWindow passes, TestMigration65RenamesEveryScopeSettingAndNothingElse still passes (it stands a database at 65, where binding the retired name must still admit nothing — 66 does not weaken that assertion, it moves the version it holds at), and TestMigrationUpgrade proves 66 as an upgrade from origin/main's ledger (verified added versions: [66 67], 67 being the probe migration that test appends). The 66 test was re-run alone against the final bytes.
• python3 qualification/managed-database/ci.py — passed, 141 tests. This installs the ledger twice, as a superuser and as the least-privileged NOSUPERUSER NOBYPASSRLS CREATEROLE NOINHERIT principal, and requires one schema and one authority catalog from both. It is not optional for this change: 66 replaces routines the baseline reassigned to an application role, and grants and takes back CREATE ON SCHEMA public around each one — the class of mistake a local superuser install cannot see. It also re-ran every request-scope guard: *_enqueue_empty_context and webhook_sync_empty_org_denied still refuse with 42501, because an empty live binding falls through to an unset retired one and yields NULL, which IS DISTINCT FROM and = both read as "no".
• go test -p 1 ./pkg/infra/ in accounts — the whole DB suite, against the local store at the ledger's frontier: green in 152s. It is the suite that matters most here, because the binding change touches every request transaction, and it caught two things a narrower run would not have:
  • TestOrganizationMemberEmailFunctionHasPinnedAuthority pins the users_select predicate as literal catalog text, which the window moves. The pin is now the compatibility read, built from the two constants, with the fallback arm named as part of it; owner, SECURITY DEFINER, the app_tenant EXECUTE grant and the absence of a PUBLIC grant are unchanged and still pinned. Closing the window moves it back, which is the reason to pin the text rather than assert that it mentions a setting.
  • TestRequestConnectionCarriesNoBindingAfterRelease failed once in the first full run and passed in isolation and in the re-run. Rather than leave that as a known flake, it is rewritten — see below.
  • The rest, specifically: TestEveryScopedPolicyReadsTheLibrarySettings (rewritten to require the window's shape rather than the retired names' absence, with the live-read counts held to the compatibility form), TestCatalogScopeSettingCheckReadsTheLiveCatalog, the decision table, and the rebinding scan.
• golangci-lint run --max-issues-per-linter=0 --max-same-issues=0 with GOWORK=off in module/services/store/code, module/services/accounts/code and module/tools: 0 issues in each, on golangci-lint 2.15.0 (CI pins 2.14.0, so local is the stricter of the two). The Go lint (module/services/store/code) failure on the first push was one errcheck finding on this PR's own defer runner.Close(); it is now defer func() { _, _ = runner.Close() }(), the shape fix(lint): check the three return values errcheck found in accounts and store tests #1301 used for the same call in the migration-65 test. That PR landed while this branch was open, which is also why CI saw only the one finding — this branch is rebased onto it.
• node module/tools/rls-migration-gate.mjs check (55 unit tests), migration-pairing-gate.mjs check, migration-reference-gate.mjs origin/main, naming-gate.mjs check, scripts/check-boundaries.sh, test-targets-gate.mjs check, authz-coverage-gate.mjs check, module-verify.mjs, go test ./... in module/tools, go test ./... in module/services/store/code.
• Behaviour was also exercised by hand against a throwaway PostgreSQL 16 before the tests were written: a policy in the window admits a transaction that binds the retired name only, one that binds the library's name only, and refuses one that binds neither or both-empty; the retired-name-only predicate a consumer has is rewritten to the identical text; a routine body keeps its pg_catalog. qualification and its cast spelling through the rewrite and back.

Not verified: nothing in this change was left unexercised. The one shape I did not stand up is a whole version-64 ledger under a booted accounts process, as noted under F1.

🤖 Generated with Claude Code