test: validate atomic RLS changes through Supabase

[aparajon]

Why

Row level security (RLS) changes need to preserve what application users can read and write. This adds Supabase API coverage for the atomic executor introduced in #123.

What

Four integration tests verify user isolation, allowed and denied writes, changes to visible rows, default deny after removing all policies, and cancellation rollback. The guides now explain this coverage and its limits.

How

Tests apply complete schema declarations through ExecuteRowSecurity, then query PostgREST as two users and an anonymous caller. Explicit table grants ensure access decisions come from RLS. The tests assert the exact policy replacement statements and their order. The rollback test cancels only after PostgreSQL confirms the fully qualified live policy drop, then verifies the original policies and access still hold.

The fixture waits for Auth's JWT helper to be ready. The existing Supabase compatibility CI job picks up these tests automatically.

Risk

Tests and documentation only; production code and image pins are unchanged. Coverage uses local Supabase and fixture tokens, not hosted projects, signup/login, or Realtime authorization.

Testing

No manual testing; automated coverage runs in the existing Supabase compatibility job.

Bigger picture

This extends the Supabase test harness so future RLS changes are checked against application behavior as well as database definitions. Hosted validation remains a follow-up.

Generated with Codex (GPT-6)

[Copilot]

Copilot review overview

🟢 Approval recommended

The tests exercise the stated RLS guarantees through real Supabase services, and the documentation accurately scopes the evidence.

Review effort: Balanced
Findings: None

What changed in this PR

Adds Supabase integration coverage proving atomic RLS changes preserve application-level authorization through PostgREST.

Changes:

• Tests tenant isolation, writes, visibility changes, default denial, and no-op reapplication.
• Verifies cancellation rolls back policy changes and restores access.
• Documents coverage and limitations.

Fixture maintenance: A separate update PR should evaluate Auth 2.197.0, Supavisor 2.9.13, Realtime 2.138.1, and PostgREST 16.4 (major drift from 14.17; 14.18 is the same-major alternative). Preserve digest pins and validate Auth initialization, pooling, replication, and PostgREST compatibility; the canonical Supabase stack still uses the current fixture versions.

File	Description
integration/​supabase/​row_security_api_test.go	Tests RLS behavior through PostgREST.
integration/​supabase/​row_security_api_rollback_test.go	Verifies cancellation rollback.
integration/​supabase/​row_security_api_helpers_test.go	Adds API, fixture, and readiness helpers.
integration/​supabase/​README.md	Explains the new test evidence and limits.
docs/​supabase.md	Updates the compatibility matrix and RLS guidance.
docs/​declarative-row-security.md	Records application-level validation coverage.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pg-sprite/pull/125, 14b9f09.

Verdict: 1 finding — 1 general suggestion (test assertion strength).

General suggestions

• The first apply's report is only checked for being non-empty (row_security_api_test.go:33). assert.NotEmpty(t, report.Statements) goes against the AGENTS.md rule "Tests assert specific values, not just existence" (AGENTS.md:168). The fixture's own_rows policy (services_test.go:73) behaves like the four new policies, so the API checks cannot tell whether it was dropped, and a create-before-drop order in one transaction also looks the same. Assert the exact list instead: DROP POLICY own_rows ON public.pgsprite_rls_api_boundaries followed by the four rendered CREATE POLICY statements (the re-apply assert.Empty at line 64 only catches a DROP that is missing once).

The one thing that could have broken, verified

The rollback test cancels the apply when it sees a DROP POLICY command tag. If anything else issued DROP POLICY, the cancellation would land at the wrong point. The only DROP POLICY in the run is the executor's live statement: scratch introspection (schemadiff/desired.go) uses CREATE SCHEMA plus savepoint rollback and never drops a policy. After the cancel, the next tx.Exec fails before it sends anything, the pgx conn stays healthy, and the deferred Rollback runs under context.WithoutCancel. So the ACCESS EXCLUSIVE lock is released before the post-cancel API reads.

Verified correct

• rowSecurityError wraps a caller cancellation as row security caller stopped: %w with caller.Err(), so require.ErrorIs(err, context.Canceled) is sound.
• An errored ExecuteRowSecurity returns a zero RowSecurityReport, which matches assert.Empty(report.Statements) in the rollback test.
• The rollback test's nobody policy (USING false) behaves differently from own_rows, so the tenant-1 read of [1] and the insert of id 3 (201) do show the old policy survived.
• The visibility test (body <> 'private' hides row 3) and the default-deny test give API results that differ from the pre-apply state.
• assertRLSInsertDenied maps 42501 to 403 for an authenticated caller and 401 for anon, which fits anon having grants but no policy.
• assertRLSRows builds non-nil empty slices, so assertRLSRead(name, 0) compares [] with [] correctly.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 14b9f09. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[morgo]

🤖 Automated adversarial review, posted on Morgan Tocker's behalf.

This is the right shape of test for RLS: driving PostgREST as real application roles is the only way to check that a policy change preserved what users can actually read and write, and granting SELECT, INSERT, UPDATE, DELETE to authenticated, anon up front so a refusal proves RLS rather than a missing grant is exactly the detail these tests usually get wrong. A few specific things I went looking for and found handled:

• Asserting 200 with an empty array for the cross-tenant UPDATE and DELETE, rather than an error. That is the correct RLS semantic — an invisible row is a no-op, not a denial — and it is the assertion most likely to be written wrong.
• The owner_id reassignment case, which checks WITH CHECK separately from USING. Those are genuinely different policy clauses and a test that only covers USING would pass with a broken WITH CHECK.
• assertRLSRows comparing against append([]int{}, ids...) and building actual with make([]int, 0, len(rows)), so an empty expectation does not fail on nil-versus-empty.
• Counting rows through the pool afterward to prove default deny and visibility filtering hide data rather than delete it.
• The repeat-apply assertion that a second identical declaration produces no statements and leaves access unchanged.
• Waiting for auth.uid() in a rolled-back transaction rather than mutating session state, and waiting separately for PostgREST's schema cache. Those are two distinct readiness conditions and treating PostgreSQL health as a proxy for either is the usual source of flakes here.

Three notes, none blocking.

assert.Empty(t, report.Statements) cannot fail

report, err := executor.ExecuteRowSecurity(ctx, changing, "public", desired, ...)
require.ErrorIs(t, err, context.Canceled)
require.True(t, trace.reached.Load(), ...)
assert.Empty(t, report.Statements)

Every error path in ExecuteRowSecurity returns RowSecurityReport{} — the zero value — so once require.ErrorIs has established err != nil, report.Statements is nil by construction, whatever the executor did before it was cancelled. The assertion passes for a cancellation after the live DROP POLICY, for a cancellation before any DDL, and for a privilege refusal that never took the lock.

It is not wrong, it just carries no information, and it reads as if it were checking that the cancelled apply reported no applied statements. The real evidence that nothing survived is already in the test — assert.Equal(t, before, after) on the introspected catalog, plus the API assertions. I would either drop this line or replace it with something that can fail.

The tracer's stated reason for being safe is not the reason it is safe

// The command tag distinguishes the successful live DROP from scratch work.
if data.Err == nil && data.CommandTag.String() == "DROP POLICY" {

A command tag carries no schema, so DROP POLICY from scratch materialization and DROP POLICY against the live table are byte-identical. The guard works today because the scratch phase (IntrospectDesiredWithRowSecurityTx) never drops a policy — it only builds the desired shape — so the only executed DROP POLICY is the live one in the diff branch. That is a property of the executor, not of the tag.

Failure scenario: scratch materialization gains a reason to drop a policy — re-materializing into a reused scratch schema, or normalizing a policy it just created. The tracer now fires during the scratch phase. trace.reached is still true, so the require.True guard that exists precisely to catch this does not catch it, and the test quietly becomes an assertion that cancelling before any live DDL leaves the live table untouched — which is trivially true. It would keep passing while covering nothing.

Cheap fix: assert on something that is actually live-specific. Checking inside the tracer that the live table currently has fewer policies than it started with, or arming the tracer only after the LOCK TABLE is observed, would make the guard structural rather than incidental. At minimum, correct the comment to say why it holds, so the next person changing the scratch path knows this test depends on it.

Assertion order buries the useful failure message

require.ErrorIs(t, err, context.Canceled) runs before require.True(t, trace.reached.Load(), "cancellation must happen after live DROP POLICY succeeds"). If the executor never reaches the live DROP POLICY — a diff that decides nothing changed, an admission refusal, a privilege failure — then nothing cancels the context, ExecuteRowSecurity returns some other error or succeeds, and the test fails on ErrorIs with "expected error to be context.Canceled, got <nil>". That message points at cancellation being broken, when the actual cause is that the DDL path was never entered. Swapping the two lines makes the failure name its own cause.

The documentation changes are careful and I have no objections to them — in particular, being explicit that local results do not establish hosted support, and that tokens are fixture-signed rather than from real signup/login, is the kind of scope statement that stops a passing suite from being read as more than it is.

[aparajon]

🤖 Addressed the suggestions from @Kiran01bm and @morgo in 2ac7444:

• Assert the exact seven statements in order: drop own_rows, set ENABLE / NO FORCE, then create the four policies. Equivalent API behavior can no longer hide a missing or reordered replacement.
• Match the exact, fully qualified live DROP POLICY query and require its successful command tag before cancelling. A policy drop in a scratch schema cannot trigger cancellation.
• Remove the empty-report assertion. The catalog comparison and API reads/writes remain the rollback evidence.
• Check that the live drop was reached before checking the cancellation error, so a failure identifies the missing execution step.

All four API tests passed locally against the pinned Supabase services with -race; lint passed. CI is running on the new commit. There are no inline review threads to resolve; both reviews were posted as general feedback.

Generated with Codex (GPT-6)

[aparajon]

🤖 The fixture maintenance follow-up stays separate from this assertion fix. Refreshed upstream release checks:

Image	Current pin	Candidate	Review notes
PostgREST	14.17	16.4	Major-version drift; validate JWT role extraction and schema-cache behavior. The latest patch fixes mixed-case root responses and database-loaded JWT cache settings
Auth	2.196.0	2.197.0	Adds authentication schema changes; validate initialization and JWT helpers
Supavisor	2.9.12	2.9.13	Connection-limit throttling and password-rotation observability changes; validate both pool modes
Realtime	2.134.10	2.138.1	Latest patch prevents channel crashes on Connect RPC errors; intervening releases still need replication review
PostgreSQL	17.6.1.136	17.6.1.177	Release body is empty, so initialization/role/replication review is incomplete

Proposed follow-up: review the intervening changes, resolve immutable image digests, run the full Supabase compatibility job, and update the tested-version documentation together. These are upgrade candidates, not validated replacements; no pins changed here.

Generated with Codex (GPT-6)