executor: apply row security changes atomically

[aparajon]

Why

pg-sprite can compare row-level security (RLS) definitions, but cannot yet apply them declaratively. Policy replacement needs one transaction so applications never see an intermediate access rule.

What

Add executor.ExecuteRowSecurity for complete RLS declarations on existing supported tables. This is a Go API; it adds no CLI flags or fingerprint convention.

How

Desired SQL
    ↓
Begin bounded transaction → lock target
    ↓
Read live state + inspect desired state in a rolled-back savepoint
    ↓
Refuse table changes → replace policies and settings
    ↓
Verify convergence → commit together
Any failure before commit → roll back together

The executor supports ENABLE/DISABLE, FORCE/NO FORCE, policies, and policy comments. A converged retry does no live DDL. Scratch inspection uses the same connection. Live policy SQL comes from the inspected catalog through the shared renderer.

Risk

This changes access rules and briefly blocks readers and writers with an exclusive table lock. Both lock waits and the whole attempt are bounded. Missing tables, unsupported table shapes, and mixed table/policy changes are refused. Unsupported declarations, table shapes, and insufficient privileges return a permanent refusal. Owner and scratch-schema privileges are checked before and after locking. An uncertain commit response requires catalog inspection before retrying.

Grants, role membership, helper bodies, and authentication remain outside this operation. A changed definition replaces the complete policy set, including unchanged policies.

Testing

No manual testing. Regression coverage includes real DDL fault injection, deadline rollback, lock contention, concurrent policy changes, non-owner default deny, quoted names, helper resolution despite shadowed built-ins, stable error codes, unsupported-table refusals, ownership changes during lock waits, scratch privileges, missing policy dependencies, and single-connection execution.

Bigger picture

Follows #119 and replaces the approach explored in closed #122. Orchestrators retain their existing replan and consent workflow. CLI execution and Supabase application-level validation are follow-ups.

Generated with Codex (GPT-6)

[Copilot]

Copilot review overview

🟡 Changes recommended

Budget and missing-table failures bypass the stable error taxonomy, and the live SQL generation boundary needs correction.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4 · 1

Open (5)

• Use native_safer_sequence for synthesized policy replay · New
• Normalize derived deadline expiry as statement budget exceeded · New
• Map PostgreSQL lock and missing-table errors to stable outcomes · New
• Generate policy SQL from admitted catalog model · New
• Add end-to-end coverage for qualified external helper resolution · New

What changed in this PR

Adds an atomic Go executor for applying complete row-level security definitions to existing tables.

Changes:

• Adds bounded, transactional RLS replacement and convergence verification.
• Adds statement qualification, scratch introspection, reports, and outcome codes.
• Updates tests, capability metadata, safety contracts, and documentation.

File	Description
SAFETY.md	Extends the trusted-core model for RLS execution.
README.md	Documents Go API support.
.agents/​checks/​review.md	Updates review guidance for RLS execution.
pkg/​statement/​row_security_execution.go	Generates qualified RLS statements.
pkg/​statement/​row_security_execution_test.go	Tests RLS statement qualification.
pkg/​statement/​desired_rls.go	Expands the declaration type’s execution scope.
pkg/​schemadiff/​row_security_roundtrip.go	Adds transactional desired-state inspection.
pkg/​schemadiff/​desired.go	Refactors scratch inspection around caller transactions.
pkg/​executor/​row_security.go	Implements atomic RLS execution.
pkg/​executor/​row_security_test.go	Tests input validation.
pkg/​executor/​row_security_integration_test.go	Covers execution, rollback, locking, and convergence.
pkg/​executor/​code.go	Adds an uncertain RLS commit outcome code.
pkg/​executor/​code_test.go	Tests outcome-code classification.
pkg/​capabilities/​capabilities.yaml	Records Go API RLS support.
pkg/​capabilities/​capabilities_test.go	Updates capability validation expectations.
docs/​tcb-model.md	Documents the RLS declaration proof boundary.
docs/​limitations.md	Narrows remaining RLS limitations.
docs/​invariants.md	Defines atomic RLS invariants.
docs/​execution-model.md	Documents the new outcome code.
docs/​declarative-row-security.md	Updates the RLS workflow and roadmap.
docs/​capabilities.md	Regenerates the support matrix.
docs/​atomic-row-security.md	Documents the new API and execution contract.

---

💡 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/123, 5761822.

Verdict: 4 findings — 1 blocking (refusals mis-typed as retryable), 2 non-blocking, 1 suggestion.

Blocking

Every RLS refusal escapes as an unmapped sibling-package sentinel, so OutcomeCode returns CodeExecutionFailed, which Permanent() reports as false. row_security.go:163 wraps schemadiff.ErrUnsupportedChange, but sentinelCode has no schemadiff/statement case and falls through to default: return CodeExecutionFailed, which Code.Permanent() does not list. An orchestrator therefore reads a permanent, author-must-fix refusal as a retryable operational error and can retry the identical call, each attempt taking ACCESS EXCLUSIVE on the live table. Same hole covers :49 (statement.ErrRowSecurityDeclaration), the ErrUnrenderable* family from Render at :154, and statement.ErrPolicyRelationDependency; TestExecuteRowSecurityRefusesMixedChanges asserts only ErrorIs, never OutcomeCode, so nothing pins it.

Non-blocking

The Render(live) admission gate has no test — deleting it leaves the whole new suite green. row_security.go:154 is the only refusal for partitioned parents, inheritance participants, FK-referenced tables and unlogged tables, yet every fixture builds the plain documents table and the mixed-changes test trips the Diff branch instead. The surviving schemadiff.Diff check never looks at ReferencedBy or InheritsParents, so removing lines 151-156 silently admits RLS on FK-referenced and inheritance tables. That leaves step 3 of docs/atomic-row-security.md unproven against AGENTS.md's "no behavior lands without a test that would fail without it".

ExecuteRowSecurity never re-verifies the ownership precondition its own doc states. row_security.go:44 takes a raw schema string, so a non-owner holding UPDATE can take ACCESS EXCLUSIVE at :94, replay the whole desired declaration, and only then fail at owner-only DROP POLICY with SQLSTATE 42501 — mapped to the fallback execution-failed, not a typed refusal. Ownership is decidable before the lock, and pkg/preflight already mints PrivilegedRole/PreflightedTable that ExecuteNative and ExecuteCreate both require. Rated PLAUSIBLE rather than confirmed: no in-repo caller exists and docs/atomic-row-security.md scopes grants and role membership out of this operation.

General suggestions

Five new error returns propagate bare errors against the repo's mandated wrapping style. row_security.go:103, :107, :117, :124 and :136 all return RowSecurityReport{}, err unwrapped, so a failure in the RS-4 SET LOCAL search_path statement surfaces as a bare pgx SQLSTATE with no mention of row security, schema or table. Lines 117 and 124 are the real gaps — LocalSearchPath produces no error of its own to inherit context from, and the sibling RenderWithRowSecurity wraps as fmt.Errorf("render row security for %q: %w", ...). The same function wraps correctly at 81, 90, 97, 99 and 129, so this is an internal inconsistency as well as an AGENTS.md deviation.

The one thing that could have broken, verified

The convergence gate could have been outside the lock or the transaction, letting a concurrent writer's policy change land unnoticed. It is not: admitRowSecurityTable plus DiffWithRowSecurity run against a fresh readback at row_security.go:134-143 before tx.Commit, wrapped in ErrInvariantViolation, which sentinelCode maps first to the permanent CodeInvariantViolation — so RS-2 is fail-closed.

Verified correct

• ReadCommitted at :79 is right: the post-lock baseline sees the blocker's committed policy; RepeatableRead would have snapshotted pre-lock and broken RS-1.
• RLS deparse is apples-to-apples: introspectRowSecurity at :63 sets pg_catalog itself and runs last, so all three models deparse under one path.
• Savepoint nesting is sound: ROLLBACK TO SAVEPOINT discards the scratch schema and its SET LOCAL, while the parent's lock and timeouts survive.
• Identifier quoting is complete: schema/table, DROP POLICY names and renderPolicy names/roles all go through pgx.Identifier.Sanitize(), with E-string comment quoting exercised end to end.
• Budget.validate() rejects sub-millisecond bounds, so the .Milliseconds() truncation at :88 cannot emit an unbounded lock_timeout = 0.
• Model mutation is copy-local: admitRowSecurityTable takes values and zeroes only its own fields, so the caller's policies survive for RenderRowSecurity.
• Multi-statement Exec at :88 is safe: pgx v5 forces the simple protocol with no arguments, as accepted_blocking.go:108 already relies on.

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

[aparajon]

🤖 Addressed the review findings in 1803dba.

• Permanent refusals: Added row-security-refused, classified as permanent. Invalid declarations, table-shape/mixed-change admission failures, and insufficient privileges retain their underlying causes. Tests now assert outcome codes and retry classification, not just the underlying error.
• Admission coverage: Added separate PostgreSQL tests for FK-referenced tables, inheritance parents and children, unlogged tables, and partitioned parents. Each verifies the specific refusal and unchanged catalog state.
• Ownership: Check effective owner privileges before taking the exclusive lock, then recheck under the lock. A non-owner with UPDATE is refused even while a conflicting lock is held. A separate test verifies that a non-superuser with inherited ownership can apply successfully.
• Error context: Added operation and target context to the inspection, search-path, rendering, admission, and verification errors.

Focused PostgreSQL tests, unit/safety checks, lint, and the pre-push race tests pass. CI is running on the new commit. The PR remains ready for review.

Generated with Codex (GPT-6)

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pg-sprite/pull/123, 1803dba.
Re-review of the delta only: 57618227..1803dba0, since the last delivered review at 57618227.

Verdict: 3 findings — 1 blocking (untested under-lock recheck), 2 non-blocking.

Blocking

The new under-lock ownership recheck has no test — delete it and the suite still passes. pkg/executor/row_security.go:113 — removing lines 112-115 leaves make test green, because TestExecuteRowSecurityRefusesNonOwnerBeforeLock refuses at the pre-lock check and TestExecuteRowSecurityAcceptsInheritedOwnership passes both, so neither distinguishes one check from two. No test transfers ownership during the lock wait, so RS-1 in docs/invariants.md is unenforced by the merge gate and a later refactor can silently drop the recheck. The existing TestExecuteRowSecurityReadsStateAfterWaitingForLock harness is the fix: swap its CREATE POLICY for ALTER TABLE ... OWNER TO <other>.

Non-blocking

The doc promises permanent row-security-refused for every invalid declaration, but a nonexistent role or helper yields non-permanent execution-failed. docs/atomic-row-security.md:57 — CREATE POLICY readers ON documents FOR SELECT TO no_such_role ... parses (roles are not resolved client-side), the scratch replay fails with 42704, and rowSecurityError maps only ErrPolicyRelationDependency, ErrRowSecurityDeclaration and 42501, so sentinelCode falls through to CodeExecutionFailed with Permanent() false. An orchestrator trusting the claim retries a permanently-broken declaration forever — the loop Code.Permanent() exists to prevent. Same for a missing qualified helper (42883).

The pre-lock gate covers table ownership only; the scratch-schema CREATE privilege is still discovered after ACCESS EXCLUSIVE is held. pkg/executor/row_security.go:120 — a role that owns the table but lacks CREATE on the database passes the gate, takes the table lock, then fails 42501 inside CREATE SCHEMA pgsprite_scratch_… (pkg/schemadiff/desired.go:54), so application traffic is frozen to discover a privilege a pre-lock has_database_privilege(...,'CREATE') probe could have caught. The outcome code is still correct, and docs/engine-role.md:58 records the unchecked privilege as a pre-existing repo-wide follow-up, so this is a widening of a known gap rather than a new bug.

The one thing that could have broken, verified

Whether the under-lock recheck is real protection or dead code: ALTER TABLE ... OWNER TO itself takes ACCESS EXCLUSIVE, so any owner change must commit before the executor's lock is granted, and the ReadCommitted per-statement snapshot makes the post-lock read observe it. The mechanism is sound — only its test coverage is missing (blocking finding above).

Verified correct

• pg_has_role(current_user, c.relowner, 'USAGE') matches PostgreSQL's own has_privs_of_role DDL check, so the gate neither over- nor under-refuses.
• TestExecuteRowSecurityAcceptsInheritedOwnership discriminates the predicate from a naive relowner = current_user::regrole equality.
• No double-wrap of ErrRowSecurityRefused: the declaration guard at row_security.go:49 returns directly, never via rowSecurityError.
• sentinelCode ordering is safe: ErrInvariantViolation still matches first, keeping RS-2 shape/convergence failures at invariant-violation.
• The ErrNoRows branch wraps only ErrTableNotFound, so TestExecuteRowSecurityRefusesMissingTable keeps table-not-found.
• Commit ambiguity preserved: *RowSecurityOutcomeUnknownError is checked before the new declaration and 42501 branches in both rowSecurityError and OutcomeCode.
• Budget classification unshadowed: both new branches sit after cancellation, deadline and asBudgetError, so the 55P03 and 57014 tests keep their budget codes.
• Two %w verbs at :125 and :141 wrap both sentinels, which is what makes requireRLSShapeRefusal's two assertions hold together.
• The docs guard covers the new code: TestDocNamesEveryOutcomeCode, TestDocPermanentColumnMatchesCodePermanent and TestCodesEnumerateEveryDeclaredCode pin the constant, the Codes() entry and the docs row against each other.

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

[aparajon]

🤖 Addressed all three follow-up findings in 64d16a9.

• Under-lock ownership recheck: Added a real lock-queue test that transfers ownership while the executor waits. It uses an already-converged declaration and preserves SELECT/UPDATE privileges so a later PostgreSQL DDL refusal cannot mask a missing recheck. I temporarily removed the recheck: the test failed because the operation incorrectly succeeded. Restored the check afterward.
• Unresolved declarations: Scratch SQLSTATE class 42 errors now return permanent row-security-refused, preserving the underlying PostgreSQL error. Separate tests cover a missing policy role (42704) and qualified helper (42883), including unchanged live catalog state. Operational failures remain unclassified by this admission mapping.
• Scratch privileges: The pre-lock gate now checks database CREATE as well as effective ownership, and both are rechecked under lock. A table-owner test holds a conflicting lock and confirms that missing CREATE is refused before waiting for ACCESS EXCLUSIVE.

The focused PostgreSQL suite, unit/safety checks, lint, and pre-push race tests pass. CI is running on the new commit; the PR remains ready for review.

Generated with Codex (GPT-6)