test: property-based convergence harness for the diff engine - #137
Conversation
📝 WalkthroughWalkthroughAdds pure and live seeded property tests for ChangesDiff convergence validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa6b4fe517
ℹ️ 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".
| for change in changes { | ||
| match change { | ||
| Change::CreateRole { name, state } => { | ||
| g.roles.insert(name.clone(), state.clone()); |
There was a problem hiding this comment.
When a desired role has config and is created from scratch, this interpreter immediately installs the config by cloning the full RoleState. However render_create_role intentionally does not render state.config (crates/pgroles-core/src/sql.rs:254-257); those settings only take effect through the follow-up AlterRole SetConfig changes. If diff() regresses and stops emitting that follow-up for new roles, convergence_and_idempotence would still pass because the config was already copied here, so the new property test misses the create-time config loss it is meant to catch.
Useful? React with 👍 / 👎.
Adds crates/pgroles-core/tests/diff_property.rs: a dependency-free
seeded-PRNG property harness (same xorshift64* convention as
suggest_property.rs, 200 seeds per property) checking the diff
engine's core contracts against pseudo-random RoleGraph pairs:
- self-diff is empty
- applying diff(a, b) to a model of `a` yields exactly `b`, and the
converged graph re-diffs empty (convergence + idempotence)
- diff output is deterministic
- additive-mode filtering never retains destructive changes, and any
retained AlterRole is a pure-SetConfig follow-up to a CreateRole in
the same plan
The file carries an in-test interpreter giving pure-data semantics for
every Change variant the engine emits (panicking on variants it should
never emit), and documents the engine's deliberate asymmetries the
generators respect: no DropSchema, owner-privilege convergence toward
{CREATE, USAGE}, owner=None meaning ensure-existence-only, and
wildcard shadow filtering. Harness verified to have teeth by mutation
(neutering DropRole semantics makes convergence fail).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TwqLzRSJR8WHpLvvVqkCg
476a89e to
9386d66
Compare
…racle
Adds crates/pgroles-inspect/tests/diff_property_live.rs: for each of 25
fixed seeds (PGROLES_LIVE_PROPERTY_SEEDS overrides), a generated
current-state is bootstrapped into a live database, the diff engine's
plan is rendered and executed, and the re-inspected RoleGraph must
equal the desired graph exactly (plus idempotence: the re-diff must be
empty). The pure interpreter from diff_property.rs is retained as a
differential cross-check: its prediction must match the re-inspected
graph, so any three-way disagreement between model, engine, and real
PostgreSQL fails CI with a reproducible seed. Runs in the PG 16/17/18
integration matrix via --include-ignored (~5s for 25 seeds); per-seed
drop-guards with defensive pre-cleanup; unique dpl{seed}_ prefixes
since roles are cluster-global.
The live oracle immediately earned its keep — it found a real
single-pass convergence bug the model-based harness was structurally
blind to: when a plan contains AlterSchemaOwner(s -> z) and a revoke of
z's explicit schema grant, PostgreSQL's ALTER SCHEMA OWNER merges z's
old ACL entry into the new owner entry, so the later REVOKE strips the
NEW OWNER's USAGE; EnsureSchemaOwnerPrivileges is computed pre-transfer
and not emitted, so the state only self-heals on the next reconcile.
Verified interactively against PostgreSQL 16.13. The generator excludes
that shape with a pointer comment (see strip_owner_schema_grants);
fixing the engine is follow-up work, not a test concern.
Beyond that one finding, the interpreter's prediction matched real
PostgreSQL on every axis across all seeds: role attributes, rolconfig
including GUC-list values, comments, schema owners and owner
privileges, grants, default privileges, and PG16 membership options.
The pure harness's doc header is reframed accordingly: fast every-push
logic check, differentially validated against the live oracle.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TwqLzRSJR8WHpLvvVqkCg
9386d66 to
ec50a69
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/pgroles-core/tests/diff_property.rs`:
- Line 132: Replace the useless format calls in the test data generation
branches with `.to_string()` on the `rng.usize(...)` results, including both the
100 and 9 bounds, while preserving the generated values and surrounding tuple
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06514e78-6115-4648-9ce7-902cae1b58c6
📒 Files selected for processing (2)
crates/pgroles-core/tests/diff_property.rscrates/pgroles-inspect/tests/diff_property_live.rs
| ["10s", "30s", "0"][rng.usize(3)].into(), | ||
| ), | ||
| 3 => ("app.foo".into(), ["on", "off"][rng.usize(2)].into()), | ||
| _ => ("app.bar".into(), format!("{}", rng.usize(100))), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
echo "== relevant lines in crates/pgroles-core/tests/diff_property.rs =="
nl -ba crates/pgroles-core/tests/diff_property.rs | sed -n '120,140p;345,360p'
echo
echo "== minimal clippy probe for format!(\"{}\", ...) =="
tmpdir="$(mktemp -d)"
cat >"$tmpdir/Cargo.toml" <<'TOML'
[package]
name = "clippy_probe"
version = "0.1.0"
edition = "2021"
[dependencies]
TOML
mkdir -p "$tmpdir/src"
cat >"$tmpdir/src/main.rs" <<'RS'
fn main() {
let x = 123;
let _s = format!("{}", x);
let _t = format!("{}", 456usize);
}
RS
cd "$tmpdir"
cargo clippy --quiet -- -D warningsRepository: hardbyte/pgroles
Length of output: 261
🏁 Script executed:
set -euo pipefail
echo "== relevant lines in crates/pgroles-core/tests/diff_property.rs =="
sed -n '120,140p;345,360p' crates/pgroles-core/tests/diff_property.rs | cat -n
echo
echo "== minimal clippy probe for format!(\"{}\", ...) =="
tmpdir="$(mktemp -d)"
cat >"$tmpdir/Cargo.toml" <<'TOML'
[package]
name = "clippy_probe"
version = "0.1.0"
edition = "2021"
[dependencies]
TOML
mkdir -p "$tmpdir/src"
cat >"$tmpdir/src/main.rs" <<'RS'
fn main() {
let x = 123;
let _s = format!("{}", x);
let _t = format!("{}", 456usize);
}
RS
cd "$tmpdir"
cargo clippy --quiet -- -D warningsRepository: hardbyte/pgroles
Length of output: 2182
Replace format!("{}", …) with .to_string()
format!("{}", rng.usize(100)) and format!("{}", rng.usize(9)) trip clippy::useless_format under -D warnings; use .to_string() in both spots.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/pgroles-core/tests/diff_property.rs` at line 132, Replace the useless
format calls in the test data generation branches with `.to_string()` on the
`rng.usize(...)` results, including both the 100 and 9 bounds, while preserving
the generated values and surrounding tuple behavior.
Source: Coding guidelines
2 participants
Second in the stacked draft series (base: #136 — retarget to
mainafter it merges).What
Property-based testing for the diff engine's convergence contract, in two layers — real PostgreSQL is the authoritative oracle (per maintainer direction, replacing the original model-only approach):
Live suite —
crates/pgroles-inspect/tests/diff_property_live.rs(#[ignore], runs in CI's PG 16/17/18 integration matrix via--include-ignored, ~5s for the default 25 fixed seeds;PGROLES_LIVE_PROPERTY_SEEDSoverrides). Per seed: a generated current-state is bootstrapped into the live database (roles, schemas, backing tables/sequences, grants, default privileges, memberships, role config incl. list-GUCsearch_path), the plan fromdiff(inspected, desired)is rendered and executed, and then:inspect()must equal the desired graph exactly.Roles are cluster-global, so every name carries a per-seed
dpl{seed}_prefix; drop-guards clean up per seed (defensively before, and on panic). Coverage boundaries (no wildcards/functions/database privileges; relation grants confined to schemas whose backing objects are bootstrapped) are documented in the file header.Pure harness —
crates/pgroles-core/tests/diff_property.rsstays as the fast every-push logic check (200 seeds, milliseconds, runs in the unit job): self-diff emptiness, convergence-in-model, determinism, additive-mode soundness. Its header now states its demoted role explicitly.Findings — the live oracle earned its keep immediately
It found a real single-pass convergence bug (#140) that the engine and the model shared a blind spot on: a plan containing
ALTER SCHEMA s OWNER TO zplus a revoke of z's pre-existing explicit schema grant strips the new owner's USAGE, because PostgreSQL merges z's old ACL entry into the new owner entry before the revoke lands. The state self-heals only on the next reconcile. Verified interactively against PG 16.13. The generator excludes that shape with a comment pointing at #140 (strip_owner_schema_grants); whoever fixes the engine deletes the exclusion and the suite proves the fix.Beyond that: across all seeds the interpreter matched real PostgreSQL on every axis — role attributes,
rolconfigincluding GUC-list canonicalization, comments, schema owners/owner-privileges, grants, default privileges, PG16 membership options. Two additional round-trip boundaries are encoded as generation constraints grounded in pgroles' own design (owner schema-grants fold intoSchemaState; default-privilege self-grants materialize implicit defaults).Harness teeth verified by mutation both ways: neutering
DropRolein the model fails the pure suite; skipping executedRESETstatements fails the live suite at seed 0 with the offending stray setting named.Testing
Live suite green twice consecutively (determinism), zero leaked
dpl%roles/schemas after runs; full workspace with--include-ignoredgreen; clippy clean.🤖 Generated with Claude Code
https://claude.ai/code/session_019TwqLzRSJR8WHpLvvVqkCg
Summary by CodeRabbit