fix: stop non-convergent plans from hot-looping; preflight unenforceable revokes; make valid_until converge - #206
Merged
Merged
Conversation
Some planned changes execute without error yet change nothing pgroles can observe. The reproducible class: PostgreSQL's REVOKE removes only ACL entries whose grantor the executor can act as (select_best_grantor), and a REVOKE matching none of them succeeds silently — no error, no warning. A privilege granted onward by a delegate (WITH GRANT OPTION) therefore survives the executor's revoke, and the same drift replans forever. Under approval: auto this degenerated: each replan minted a fresh plan (the terminal Applied plan never matched the Pending dedup), every plan status write woke the policy controller via plan_decision_hash, and the same no-op changes re-applied about once a second. create_or_update_plan now recognises — for auto-applying callers only — that a plan with identical change digest was Applied inside a 120s window and still diffs, and returns DeduplicatedNonConvergent instead of minting a new plan. The reconciler surfaces Ready=False reason=NonConvergentPlan naming that plan and defers the retry to the policy interval, mirroring the existing failed-plan back-off. Clock-skew guard and window semantics match retry_deferred_until_window_expires, with unit tests for both. Documented the silent-revoke limitation in limitations.md. Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…classes
Two more classes of "applies cleanly but never converges", found by
auditing for the same shape as the non-convergent-plan hot loop:
- password_valid_until with a timezone offset or fractional seconds named
a valid instant but could never string-compare equal to the inspected
value (always rendered UTC whole-second), so the ALTER ROLE re-planned
on every run. Validation now accepts exactly the convergent form
YYYY-MM-DDTHH:MM:SSZ and says why.
- VALID UNTIL 'infinity' — which pgroles itself renders when an
expiration is removed — inspected as Some("") (to_char renders
non-finite timestamps as an empty string), so removing an expiration
created a perpetual diff. Inspection now treats non-finite
rolvaliduntil as no expiration, verified by a new live test covering
set, remove, and hand-set-infinity round-trips.
Also documented (limitations.md) the pre-16 membership inherit churn:
inspection falls back to the member's rolinherit for the edge inherit
option that PG 15 and older cannot express, so a NOINHERIT member's
declared membership plans an unactionable revoke-and-regrant every run.
Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
PostgreSQL's REVOKE removes only ACL entries whose grantor the executor can act as, and a revoke matching none of them succeeds silently, leaving the entry in place and the same drift re-planning on every run. The authority preflight covered only PUBLIC revokes, via owner membership — a test that is both too strict and too loose for ordinary grantees: an executor can hold revoke authority through its own grant option without owner membership, and owner membership cannot remove an entry a delegate granted onward WITH GRANT OPTION. Role-grantee revokes are now checked per ACL entry: the preflight explodes the live ACLs of the targeted objects (all eight object types, wildcards resolved through the inspected grant keys like the PUBLIC check) and reports each entry for the revoked grantee and privileges whose grantor is out of the executor's reach, naming the object and grantor and pointing at the fix. diff warns, apply blocks, the operator blocks execution while still producing the reviewable plan. Entries whose grantor is reachable can still survive PostgreSQL's grantor selection (the executor's own grant option shadowing the owner's); that residue is what the operator's post-apply NonConvergentPlan detection exists for, and the module docs now spell out the division. Live test drives the full matrix: unreachable owner + delegate grantors both reported, owner membership narrowing the report to the delegate entry alone, and a superuser executor staying clean. Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bff2a0c6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…alid_until Review follow-ups on the foreign-grantor preflight and valid_until fixes: - The grantee revoke grouping unioned object names and privileges per (object_type, schema, grantee), so heterogeneous revokes were checked as their cross-product: revoking SELECT on table A and UPDATE on table B flagged a foreign-grantor UPDATE entry on A that nothing targeted, blocking apply. Targets now carry object -> privileges (wildcard privileges tracked separately), the authority rows carry the entry's privilege_type, and the association is re-applied client-side. Live test pins the untargeted entry staying unflagged. - VALID UNTIL '-infinity' means "already expired" — the opposite of 'infinity' — and mapping both to "no expiration" made a role that cannot authenticate report as converged. Negative infinity now inspects as the literal '-infinity', which no manifest value equals, so one ALTER ROLE repairs it to the declared state and the repair converges. Live test covers the round-trip. Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…grantors; cite upstream docs
Since PostgreSQL 16 each membership edge records its grantor, and a bare
REVOKE role FROM member removes only the edge attributed to the executor:
with ADMIN OPTION but a different grantor it succeeds with just a WARNING
("role ... has not been granted membership ... by role ...") and the edge
survives — the membership sibling of the silent object-privilege revoke,
verified live on PostgreSQL 16. The preflight now flags every targeted
edge whose grantor the executor cannot act as, per edge and naming the
grantor, gated to servers >= 16 (earlier revokes are not
grantor-attributed). Live test covers the ADMIN-but-wrong-grantor case
and a clean superuser executor.
Docs now separate the object-privilege and role-membership revoke
attribution rules, link the upstream REVOKE/GRANT documentation, and the
course's role-hierarchy and security-review chapters explain why a
delegated grant survives someone else's revoke. Condensed the unreleased
CHANGELOG entries.
Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…included Divisible re-verification of every behavioral claim against a live PostgreSQL 16 and the upstream REVOKE notes found the preflight's actability test wrong for superusers. PostgreSQL removes only grants attributed to the revoker, and per the upstream notes a superuser's GRANT/REVOKE "is performed as though it were issued by the owner of the affected object" — for role memberships, by the bootstrap superuser. Verified live: - a superuser's plain object REVOKE removed only the owner-attributed entry; the delegate-granted entry survived silently; - a superuser's bare membership REVOKE of an edge granted by an ordinary role warned and left the edge; edges attributed to the bootstrap superuser (which is how grants by any superuser are recorded) were removed; - object GRANTED BY errors unless it names the current user, even for superusers; the delegate itself removes its entry with a plain REVOKE; - an owner-member executor without its own grant option removes the owner-attributed entry (selection falls to the owner). pg_has_role(executor, grantor) is true for every grantor when the executor is a superuser, so both new checks never flagged anything for superuser executors — a false negative on exactly the entries no one's plain revoke can remove. The predicates now branch: superusers flag entries not attributed to the owner (objects) / not attributed to the bootstrap superuser or themselves (memberships); non-superusers keep the conservative membership test, whose shadowing residue stays with the post-apply non-convergence detection as documented. Live tests now pin the superuser cases both ways (delegate entry flagged, owner/bootstrap-attributed entry not), and the module docs, issue messages, limitations page, and course text quote the upstream wording instead of the imprecise "grantor the executor can act as". Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…emberships converge The durable fix for the residual attribution gaps: instead of predicting which grant PostgreSQL's plain REVOKE will remove (it removes only the grant attributed to the revoker; a superuser acts as the object owner, or the bootstrap superuser for role memberships), pgroles now records each grant's grantor at inspection time and targets it explicitly. - Inspection reads aclexplode's grantor for every managed ACL entry and pg_auth_members' grantor for every membership edge (PG16+). One role/member pair can carry several edges with distinct grantors — a live churn bug before, since duplicate rows produced duplicate model edges; they now aggregate into one effective edge (options OR) with the grantor set carried alongside. - The diff splits revokes per grantor: one Revoke/RemoveMember per recorded grantor, plus a plain fallback for anything without a breakdown (pre-16, wildcard-collapsed keys, desired-side callers). A schema whose owner the same plan transfers falls back to a plain revoke, because ALTER ... OWNER TO rewrites the ACL's attribution and the recorded grantor goes stale (found by the live property suite). - Rendering acts as the grantor inside the plan's single transaction: SET ROLE grantor; REVOKE ...; RESET ROLE; for object privileges (GRANTED BY must name the current user there), and REVOKE ... GRANTED BY grantor for memberships (version-gated to 16+ with the data that feeds it). - The preflight becomes exact and loses its attribution heuristics: per grantor, can the executor become it — pg_has_role(..., 'SET') for object revokes, 'USAGE' for membership GRANTED BY; superusers qualify everywhere. UnsatisfiableRevoke names any grantor the executor lacks, with the remedy. - Change::Revoke/RemoveMember gain an optional grantor field, skipped in serialization when None so digests and JSON output of grantor-less plans are byte-identical (no approval-effect encoding bump). Live proof (grantor_targeted_revokes_live.rs): a plain non-superuser executor — the production posture — converges in ONE apply the three scenarios that previously re-planned forever: an owner-granted entry shadowed by the executor's own grant option, a delegate-granted entry (unremovable even by superusers before), and a membership edge granted by another admin, with an empty replan and live has_privilege/ pg_has_role probes. The DB-backed property suite now runs each change list on one connection like production, which is also what surfaced the owner-transfer staleness case. Docs: limitations, executor-privileges, and both course chapters now describe the attribution model and the executor requirements (become the grantor) instead of documenting the non-convergence. Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…ution, plan-order authority
Three review findings on the grantor-targeted revoke mechanism:
- RESET ROLE after a grantor-targeted object revoke reset to the *login*
role, silently bypassing the operator's connection.params.setRole
boundary (applied via after_connect SET ROLE) for the rest of the plan
and the pooled connection. SqlContext now carries the connection's
detected execution role (current_setting('role')) and revokes restore
it explicitly; RESET ROLE remains only when no execution role is set.
- PUBLIC ACL rows discarded their grantor, so a privilege a grant-option
holder delegated to PUBLIC got a plain owner-attributed revoke that
never converged. PUBLIC queries now read owner and grantor, and
derive_public_privileges feeds entry_grantors like the role path.
- Preflight checked grantor authority against the current graph while
membership removals executed before object revokes and could strip the
executor's SET path mid-plan. Object revokes now run before membership
removals, and the preflight re-checks GRANTED-BY grantors and
default-privilege owners against the post-removal membership graph,
flagging dependency-breaking plans (GrantorAuthorityRemovedByPlan).
Live tests: setRole-restore round trip, delegated-PUBLIC convergence as
a non-superuser executor, and dependency-breaking plan detection.
Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…ccuracy - The PUBLIC owner-authority sweep now covers only plain (grantor-less) PUBLIC revokes: a grantor-targeted revoke runs SET ROLE <grantor> and needs exactly that, already checked by the exact grantor feasibility pass — demanding owner authority as well blocked least-privilege executors whose targeted revoke succeeds. New live test converges a delegated PUBLIC grant with the executor a member of the delegate only. - Mid-plan grantor authority is now checked phase by phase, matching execution order: GRANTED BY membership revokes against the graph with the plan's removals applied; default-privilege revokes against the graph with removals AND the plan's inheriting additions, since they execute after the addition batch — a plan that replaces the executor's path to a defaults owner in one apply is executable and no longer flagged. New live test drives the whole scenario through diff() from a manifest and applies it end to end as a non-superuser executor. - Version-claim accuracy: object-ACL grantors are recorded (aclexplode) and targeted on every supported version; only membership-edge grantor targeting requires PostgreSQL 16. Changelog, limitations page, and code comments narrowed accordingly; stale RESET ROLE restoration wording swept from Rust docs. Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…ns included The mid-plan simulation only reported roles reachable now but broken by removals, and was skipped entirely for plans with no removals — while the separate current-state owner check still rejected any RevokeDefaultPrivilege owner the executor cannot inherit yet. That blocked an executable pure-addition plan: an executor holding the owner WITH ADMIN TRUE, INHERIT FALSE grants the owner to a role it already inherits, then revokes the owner's default privileges. The phase check is now authoritative for RevokeDefaultPrivilege owners whenever the plan changes memberships: reachability is computed over the graph with the plan's removals and inheriting additions applied, and the current-state owner check skips those owners (SetDefaultPrivilege, which executes before membership changes, keeps the current-state check). Unreachable owners the executor can use now report GrantorAuthorityRemovedByPlan; ones it never could report DefaultPrivilegeOwner. Membership GRANTED-BY grantors keep the removals-only view, since they execute in the removal batch. New live regression drives the pure-addition scenario through diff() and applies it end to end as the non-superuser executor. Changelog and limitations updated to say additions count for default-privilege revokes. Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
…mper phase-precision prose An owner carrying both SetDefaultPrivilege and RevokeDefaultPrivilege that the executor could never use received the identical DefaultPrivilegeOwner issue twice — once from the phase check and once from the current-state check. The phase check now defers such owners to the current-state check. The limitations page also overstated the phase model's precision; it now says later statements are judged against a conservative approximation: membership revokes see the whole removal batch at once, and additions referencing plan-created roles are ignored. Claude-Session: https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD
This was referenced Aug 27, 2026
Merged
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
2 participants
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.
Fixes the reported apply-mode incident (24 no-op changes re-applied about once a second), then closes the underlying attribution gaps for good: delegated grants and foreign-admin memberships now converge, including for non-superuser executors.
Root cause
PostgreSQL attributes every grant to a grantor, and a plain
REVOKEremoves only grants attributed to the revoker (REVOKE, GRANT); a superuser's revoke acts as the object owner — for role memberships, as the bootstrap superuser. Verified live on PostgreSQL 16:WARNING, even for superusers withADMIN OPTION.pgroles inspected grants grantor-blind, so authoritative mode re-planned identical revokes forever; under
approval: autoeach replan minted a fresh plan and every plan status write woke the controller — the 1/s loop.Changes
Appliedwithin 120s and still diff is not re-applied:Ready=False, reason=NonConvergentPlan, retry at the policy interval — mirroring the failed-plan back-off. Remains the backstop for anything attribution-shaped on pre-16 servers or wildcard-collapsed keys.PUBLICentries included. The diff splits revokes per grantor and rendering becomes the grantor inside the plan's single transaction:SET ROLE grantor; REVOKE ...;for object privileges — restoring the connection's configured execution role afterwards (connection.params.setRolesurvives;RESET ROLEonly when none is set) — andREVOKE ... GRANTED BY grantorfor memberships. Object revokes run before membership removals so removals can't strip a grantor path mid-plan. A schema whose owner the same plan transfers falls back to a plain revoke (ALTER ... OWNER TOrewrites ACL attribution — found by the live property suite). Thegrantorfield is omitted from serialization when absent, so digests and JSON of grantor-less plans are unchanged.pg_has_role(..., 'SET')for object revokes,'USAGE'for membershipGRANTED BY(superusers qualify everywhere).UnsatisfiableRevokenames any missing grantor and the remedy;diffwarns,applyblocks. Authority is also checked against the plan's own execution order: a plan whose membership removals would strip the executor's path to a grantor a laterGRANTED BYrevoke orALTER DEFAULT PRIVILEGES FOR ROLEstill needs is flagged (GrantorAuthorityRemovedByPlan) instead of failing mid-apply. The PUBLIC-revoke and grantor-less catalog sweeps remain for the plain-revoke fallbacks.valid_untilconvergence. Offsets/fractional seconds andinfinity/-infinityeach re-planned anALTER ROLEforever. Validation now accepts exactlyYYYY-MM-DDTHH:MM:SSZ(breaking, upgrade note in CHANGELOG);infinityinspects as "no expiration";-infinity(already expired) stays distinguishable so one apply repairs it.SEToption / the grantor's privileges), and the course's role-hierarchy and security-review chapters, quoting and linking the upstream PostgreSQL documentation. Pre-16 membership-inheritchurn documented with workaround.Testing
grantor_targeted_revokes_live.rs): a plain non-superuser executor — the production posture — converges in one apply the three scenarios that previously re-planned forever (owner entry shadowed by the executor's own grant option; delegate-granted entry, unremovable even by superusers before; membership edge granted by another admin), with an empty replan and livehas_table_privilege/pg_has_roleprobes. A companion test round-trips a configuredSET ROLEexecution role through a grantor-targeted revoke;public_delegated_grants_live.rsconverges a delegate's grant toPUBLIC.valid_untilround-trips; the DB-backed property suite (25 seeds, full bootstrap→converge→re-inspect→differential-interpreter check) exercises grantor-targeted rendering end-to-end and runs each change list on one connection like production.--include-ignoredgreen; clippy clean.Codex review findings (privilege cross-product,
-infinity) and agent review findings (setRole bypass viaRESET ROLE, unattributedPUBLICdelegated grants, plan-order authority) addressed.https://claude.ai/code/session_01QnnEJ6DmyBUHYqw1zZRoDD