feat(executor): bind row security execution to reviewed SQL

[aparajon]

Why

An orchestrator can review a row security plan, then wait for a table lock while another actor changes its policies. Execution needs to verify the reviewed SQL after acquiring that lock.

What

Add PreviewRowSecurity and ExecuteReviewedRowSecurity. Preview returns ordered generated SQL without target changes. Reviewed execution refuses a different sequence before target DDL with a distinct row-security-plan-changed outcome that calls for a fresh review. Add an engine-owned parser for persisted RLS operation scripts, retaining their original statement sequence and a canonical comparison form. Namespace comparison requires the expected physical schema and table before omitting the target schema; qualified helpers and expressions remain intact.

How

Reuse the existing atomic executor and its admission, budgets, readback, and commit handling. Compare the complete generated sequence under the target lock, including order and duplicates. Never execute caller-supplied SQL directly.

preview → review SQL → acquire target lock → regenerate SQL
                                               │
                         same sequence ─────────┤── different sequence
                               ↓                         ↓
                      apply + verify + commit      refuse + rollback

Risk

This touches the atomic RLS executor. Existing CLI and unreviewed Go behavior stay unchanged. Preview also takes a bounded exclusive lock. The contract binds SQL, not an earlier catalog snapshot: a predicate edit can leave replacement SQL unchanged.

Testing

No manual testing

Bigger picture

Provides the execution contract for SchemaBot's existing replan and consent workflow, without new CLI flags, approval tokens, or fingerprints. PostgreSQL AST handling belongs in pg-sprite. SchemaBot will consume the engine API for review and orchestration; the stored-plan and apply integration is in SchemaBot #1522.

Generated with Codex (GPT-6)

[Copilot]

Copilot review overview

🟡 Changes recommended

The new AST mutation path conflicts with the repository’s canonical parsing safety guidance and needs redesign or an explicit documented exception.

Review effort: Balanced
Findings: 1

Open (1)

• Second AST path bypasses canonical Qualify-only guidance · New

What changed in this PR

Adds review-bound RLS execution for SchemaBot-compatible orchestration while retaining atomic executor safeguards.

Changes:

• Adds locked preview and reviewed execution APIs.
• Adds ordered RLS script parsing and canonical comparison.
• Documents and tests the RS-5 invariant.

File	Description
SAFETY.md	Updates the RLS execution boundary.
README.md	Describes reviewed SQL binding.
pkg/​statement/​row_security_change.go	Parses and canonicalizes RLS operations.
pkg/​statement/​row_security_change_test.go	Tests parsing and comparison behavior.
pkg/​executor/​row_security.go	Implements preview and reviewed execution.
pkg/​executor/​row_security_review_integration_test.go	Tests atomic review enforcement and concurrency.
pkg/​capabilities/​capabilities.yaml	Updates capability metadata.
docs/​limitations.md	Clarifies review binding limitations.
docs/​invariants.md	Registers invariant RS-5.
docs/​capabilities.md	Documents the expanded Go API capability.
docs/​atomic-row-security.md	Documents the reviewed execution workflow.

---

💡 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/130, 3c45d34.

Verdict: 6 findings — 2 non-blocking (reviewer-checklist drift, reused refusal code), 4 suggestions.

Non-blocking

1. The review-agent checklist was not updated with the other rule docs. .agents/checks/review.md#L25-L26 still says executor.ExecuteRowSecurity is the only live consumer of DesiredWithRowSecurity and stops at RS-1..RS-4. Line 43 still says the relation retarget is the only permitted AST edit, and SAFETY.md#L133 still ends at RS-1..RS-4. AGENTS.md points review agents at this checklist, so a later review will flag ExecuteReviewedRowSecurity and the CanonicalSQLForNamespace edit as violations, and will never check RS-5.

2. A stale reviewed plan reuses CodeRowSecurityRefused. row_security.go#L190-L191 wraps the mismatch in ErrRowSecurityRefused, and the test at L87 pins that. The code's contract is "requires a declaration, target, or privilege change", and docs/execution-model.md#L292 tells users to change the declaration, but here the fix is to re-preview and re-review. That also goes against code.go's "new outcomes add new codes". No caller in this repo is affected, so only an external orchestrator that branches on the code would misroute.

General suggestions

3. The README suggests the CLI can bind execution to a review. README.md#L69 says the executor "also available through migrate --desired, can bind execution to an ordered SQL review". The CLI only calls ExecuteRowSecurity (migrate_row_security.go#L29), so this is Go-API only, as capabilities.md in this PR already says.

4. SAFETY.md leaves PreviewRowSecurity off the consumer list. SAFETY.md#L77 names only the two execute functions. Preview also takes the ACCESS EXCLUSIVE lock and builds a scratch schema on the live database, and it does not run the convergence check that the same sentence attributes to "that executor".

5. No test round-trips real renderer output through ParseRowSecurityChange. row_security_change.go#L42 says it accepts the SQL a complete atomic replacement emits, but its tests only pass hand-written SQL. Today's output parses, but feeding real PreviewRowSecurity output through it in a test would catch a future renderer change before persisted plans start failing.

6. Tests index Policies[0] with no length check. row_security_review_integration_test.go#L46 and L195 would panic, not fail cleanly, if a regression dropped the policy. Add require.Len(t, after.RowSecurity.Policies, 1), as the lock-wait test already does.

The one thing that could have broken, verified

The riskiest mechanism is RS-5's ordering, where the reviewed-plan comparison must run after the lock and before any DDL. The comparison runs after LOCK TABLE ... ACCESS EXCLUSIVE and live introspection, and before any target DDL. The lock-wait test would catch plan generation moving ahead of the lock, because the blocked DROP POLICY readers would fail instead of returning ErrRowSecurityPlanChanged.

Verified correct

• Execute mode is unchanged: report.Statements is empty with no diff, search_path is still set only on the change path, and CI is green on PG 14–18.
• slices.Equal(nil, []string{}) holds, so an empty reviewed plan allows only a no-op.
• ExecuteReviewedRowSecurity clones reviewed, so a caller that mutates the slice cannot change the comparison.
• Preview rolls back explicitly, which undoes the savepoint and SET LOCALs, and the deferred Rollback's ErrTxClosed is safely discarded.
• Preview and execute produce the same SQL: policies are read ORDER BY p.polname, rendering is pure, and the scratch schema name never appears in the SQL.
• The RowSecurityPlan(report) conversion is valid, because the field types match and tags are ignored.
• A mismatch error satisfies errors.Is for both sentinels, and rowSecurityError passes it through unchanged.

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

[aparajon]

🤖 Addressed all six findings in 2f4d7d9:

• Updated the review checklist and SAFETY.md for preview, reviewed execution, RS-5, and the narrow comparison-only AST exception
• Added permanent row-security-plan-changed for ErrRowSecurityPlanChanged, separate from declaration/target/privilege refusal. It no longer wraps ErrRowSecurityRefused; the documented remedy is a fresh preview and SQL review
• Clarified that review binding is Go-API-only; migrate --desired retains its existing unreviewed behavior
• Distinguished preview's lock/scratch/rollback path from execution's convergence check
• Round-tripped actual PreviewRowSecurity output through ParseRowSecurityChange, asserted the ordered SQL is unchanged, and executed the parsed sequence
• Added both missing policy-length assertions

The focused reviewed-RLS and outcome-contract tests passed against real PostgreSQL with the race detector (14.887s), and the pre-push executor unit tests passed. CI is rerunning. SchemaBot #1522 also consumes the new outcome with guidance to re-plan and review, with its focused adapter/integration tests passing.

— Codex

[morgo]

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

Binding execution to the regenerated SQL rather than to a fingerprint or an approval token is the right call, and the Risk section stating the residual honestly ("the contract binds SQL, not an earlier catalog snapshot: a predicate edit can leave replacement SQL unchanged") is worth more than a longer design doc. Comparing under the lock, before any target DDL, and refusing rather than reconciling, is the correct failure direction throughout.

Verified rather than assumed:

• The comparison is self-binding to the target, so a reviewed plan cannot be replayed against another table. ExecuteReviewedRowSecurity never checks that its schema/desired.Table() match the ones the plan was previewed for — which looked like the obvious hole — but every generated statement carries the fully qualified target (DROP POLICY … ON "s"."t", and RenderRowSecurity likewise), so a plan reviewed for one table cannot equal the sequence regenerated for another. Exact string comparison is what buys this; a canonicalisation that dropped qualifiers would lose it.
• The order-sensitive comparison rests on a real ordering guarantee. Comparing "including order and duplicates" makes statement order load-bearing, and an unordered catalog read would make reviewed execution refuse at random. The policy query ends ORDER BY p.polname, and the render side derives from the declaration, so both sides are stable across the preview/execute gap.
• An intervening policy add or drop is caught. The DROP list is built from live.RowSecurity.Policies under the lock, so a policy created or removed between preview and execution changes the sequence length and the comparison refuses. The admitted residual is narrower than it first reads: it is specifically an edit to a policy that is being dropped and recreated by name, where neither the drop nor the create text depends on the old definition.
• Hoisting the exec loop out of the if is behaviour-preserving. The old loop ranged over the whole accumulated report.Statements from inside the diff block, which read like a quadratic re-execution; the block is an if, not a for, so it ran once and the hoist changes nothing. Worth the double-take.
• slices.Clone(reviewed) at the entry point is load-bearing. The comparison happens after a lock wait that can last the whole LockTimeout, so a caller that reuses its slice would otherwise be able to mutate the thing being compared while the executor blocks.
• ErrRowSecurityPlanChanged's "no target RLS statements have executed" holds. The refusal precedes the exec loop; everything before it is privilege checks, the lock, introspection and a SET LOCAL, all inside the rolled-back transaction.

One finding, two smaller ones.

CanonicalSQLForNamespace makes two different schemas' operations compare equal, and only a doc sentence prevents it

// CanonicalSQLForNamespace omits only the operation's target schema qualifier.
// Use it when the comparison key already contains a canonical namespace and
// table. …
func (c RowSecurityChange) CanonicalSQLForNamespace() (string, error) {

The method strips Schemaname from every CreatePolicyStmt and AlterTableStmt, and drops the leading element of every DropStmt/CommentStmt object list. So two RowSecurityChange values parsed from operations on tenant_a.orders and tenant_b.orders — identical policies, different schemas — produce byte-identical output. The type knows which schema it came from (c.schema is right there, and the method already reads it to reject a zero value) and does not surface it in the result or let the caller assert it.

The stated precondition is that the caller's comparison key "already contains a canonical namespace and table". That is not a property the type can rely on: the caller is a stored-plan consumer, and the namespace in a stored plan record comes from the same place the SQL does. If the key is derived from the plan rather than from the target the operator is actually executing against, the precondition is self-referential and the guard is a comment.

Failure scenario. The consumer stores an approved RLS operation as {namespace, canonical_sql} and, at apply time, re-derives the canonical form and compares to decide whether the approval still covers the plan. An operation approved for tenant_a.orders is re-pointed at tenant_b.orders — by an edit to the plan's namespace field, a namespace-mapping change, or a copied plan record. The canonical SQL is unchanged, so the approval check passes and the operation presents as already-reviewed. Whether it then executes against B is the consumer's problem, not this package's — but this package is what told it the two were the same operation, and it is the only place that knew they were not.

This is the one place the API can answer "these are the same" about two things that are not, and it is in the security-critical comparison path. Everything else in the PR fails closed.

Fix. Take the expected namespace as an argument and validate it, so the precondition is checked where it is knowable rather than asserted in prose:

func (c RowSecurityChange) CanonicalSQLForNamespace(schema, table string) (string, error) {
	if c.schema != schema || c.table != table {
		return "", fmt.Errorf("%w: operation targets %s.%s, not %s.%s", ErrRowSecurityChange, c.schema, c.table, schema, table)
	}
	…
}

The caller already has both values — it must, for the precondition to be satisfiable at all — so this costs nothing and turns a comment into a compile-time obligation. Returning (sql, schema, table, error) would also work but leaves the caller free to ignore two of them.

The test file does not cover this: row_security_change_test.go exercises the stripping, not the collision. TestCanonicalSQLForNamespaceCollidesAcrossSchemas asserting the two are equal today, or that the validated form refuses, is a two-line pin either way.

Nothing checks that Statements() and the canonical form describe the same statements

for i, raw := range tree.Stmts {          // canonical: built from pgquery.Parse
	…
	result.canonical = append(result.canonical, canonical)
}
source, err := Split(sql)                 // statements: built from the repo's splitter
…
for _, stmt := range source {
	result.statements = append(result.statements, stmt.SQL)
}

Two independent parsers walk the same input and their outputs are stored as parallel lists, with no assertion that they agree on how many statements there are. CanonicalSQL() is documented as "a single operation's comparison key" and Statements() as "the original statements in their original order" — a consumer will reasonably validate with the first and display or persist the second, and any disagreement silently decouples what was checked from what is shown.

I could not construct an input where Split and pg_query disagree without reading Split, so treat the reachability as unverified — the structural point stands regardless: the invariant "these two lists are the same operation" is relied on and never stated. One line after the loops:

if len(source) != len(result.canonical) {
	return RowSecurityChange{}, fmt.Errorf("%w: %d statements parsed, %d split", ErrRowSecurityChange, len(result.canonical), len(source))
}

Notes

RS-4's guarantee became positional. SET LOCAL search_path = pg_catalog is set inside the if block, with the comment explaining that no target-schema function may shadow a built-in while policy expressions are replayed. The statements it protects are now executed outside that block. The coupling survives only because report.Statements is non-empty exclusively inside the if — an invariant nothing states and nothing tests. Anyone who later appends a statement outside that block replays an expression under the caller's search_path, which is the exact shadowing RS-4 exists to prevent, and no test would notice. Moving the SET LOCAL to immediately before the hoisted loop restores the local guarantee at no cost.

The preview's explicit rollback can discard a plan it already computed. tx.Rollback(ctx) runs on the budgeted attempt context and its error is returned in place of the report, while the deferred cleanup already rolls back on a detached context with its own 5 s window. Nothing has been written, so a rollback failure here is benign; returning report, nil and letting the defer do the work removes a way for a successful preview to surface as a failure.

Preview takes ACCESS EXCLUSIVE. PreviewRowSecurity's doc says "under a bounded exclusive table lock", which undersells it: this blocks readers as well as writers for up to LockTimeout, and preview is the operation most likely to be called speculatively, repeatedly, or from a UI. Saying "blocks all reads and writes on the target for the lock's duration; it is not a dry run you can run freely" in the doc comment puts the cost where the caller reads it. The Risk section has it; the API surface does not.

Two raw field writes in CanonicalSQLForNamespace are panics rather than errors if their shape assumption is ever wrong. node.GetCreatePolicyStmt().Table.Schemaname = "" dereferences Table without a nil check, and object.Items = object.Items[1:] dereferences the result of GetList() and assumes at least one element. Both are guaranteed by the validation ParseRowSecurityChange already did — but the method re-parses deparsed text rather than reusing the validated nodes, so the guarantee travels through a parse → deparse → parse round trip. A library that panics on malformed input is worse than one that errors; a if list == nil || len(list.Items) < 2 guard is cheaper than reasoning about round-trip stability.

CanonicalSQLForNamespace re-parses what ParseRowSecurityChange already parsed. Each statement goes parse → deparse (stored in canonical) → parse → deparse. Retaining tree.Stmts on the struct, or a cloned copy, removes two full parser round trips per statement. Not hot today; worth noting before a consumer calls it per policy per apply.

The empty-input rejection is the only unwrapped error. return RowSecurityChange{}, ErrRowSecurityChange gives the caller no reason, while every other failure path explains itself. fmt.Errorf("%w: no statements", …) matches the rest.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pg-sprite/pull/130, 2f4d7d9.

Verdict: clean — approve.

The one thing that could have broken, verified

The reviewed-SQL gate could be skipped on some path. The RS-5 check mode == rowSecurityReviewed && !slices.Equal(reviewed, report.Statements) runs after the ACCESS EXCLUSIVE lock and the re-introspection under that lock. It also runs before any target DDL. It covers both the no-op path and the non-empty path, and no early return gets past it. slices.Equal(nil, []string{}) is true, so an empty review authorizes only a no-op. A non-empty review against a plan that is now empty is refused, and TestReviewedRowSecurityPreviewAndApply covers that case.

Verified correct

• Moving the tx.Exec loop out of the diff-error block keeps ExecuteRowSecurity's behaviour: on a no-op, Statements is []string{}, so the loop does nothing.
• slices.Clone(reviewed) means the caller cannot mutate the input mid-run. The supplied SQL is only compared, and only generated statements are executed.
• Preview rolls back before any target DDL. The deferred second rollback is ignored, and scratch objects are discarded.
• The RowSecurityPlan(report) conversion is valid: both types have the same fields and field types, and Go ignores struct tags when converting.
• The SQL is deterministic between preview and execute: policies are introspected ORDER BY p.polname and roles with ORDER BY 1, so ordering drift cannot cause a spurious refusal.
• ErrRowSecurityPlanChanged maps to CodeRowSecurityPlanChanged. The code is listed in Codes() and marked Permanent.
• rowSecurityError returns the sentinel unchanged. The execution-model.md row says 'yes', and the docs guard tests pass locally.

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 2f4d7d9. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 Addressed in 903cafc and 13cce84:

• CanonicalSQLForNamespace now requires the expected physical schema and table and rejects a mismatch. Docs require those values to come from target configuration, not from the SQL being checked. Tests cover wrong schema and wrong table
• Revalidate each freshly parsed canonical node before editing it, covering the relation/list shape assumptions and returning an error rather than indexing unchecked shapes
• Check split/parsed statement counts, add a quoted-semicolon/dollar-quote round-trip regression, and explain empty-input rejection
• Move SET LOCAL search_path immediately before target execution, and explicitly document that preview's ACCESS EXCLUSIVE lock blocks reads and writes while held

The consumer needed attention too: SchemaBot #1522 now preserves physical schema qualifiers in RLS drift comparison. A dispatch does not prove its source deployment's mapping, so it cannot use a qualifier-free comparison as review authority. A differently named physical target needs its own plan and review. Local logical-to-physical mapping remains covered by the stored-plan integration test.

Two suggestions intentionally remain unchanged:

• Preview keeps explicit rollback. It has created scratch objects and holds a lock even though it has not changed target policies. Returning success only after rollback acknowledges cleanup; a rollback failure should remain visible. The detached deferred rollback remains the cleanup fallback
• Keep fresh-tree reparsing rather than storing mutable AST nodes. This preserves the immutable comparison boundary; the new shape revalidation checks the parse/deparse round trip. No measured performance issue justifies caching here

Focused race tests passed: statement/parser (7.466s), executor RLS integration (44.679s), SchemaBot drift (7.843s), and stored-plan/local RLS integration (16.576s). Consumer-module tests and build passed. CI reruns on the published heads.

— Codex