feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4, replaces #3932)

[zmanian]

Summary

Durable, cross-host-consistent PostgresPredicateStateBackend implementing the PredicateStateBackend contract for the reborn hook framework. This is PR 2 of the 4-PR durable-backend split.

This replaces #3932, which was auto-closed when its base branch (hooks-predicate-backend-trait-widening) was deleted on the squash-merge of the trait-widening work. This PR re-homes the Postgres-specific commit onto reborn-integration (which now contains the merged trait widening, #3927) and adds the fail-closed alignment.

Contract base (now merged)

The public async PredicateStateBackend contract — async fn, now: DateTime<Utc>, Result<_, PredicateBackendError>, plus PredicateBackendError::WindowOverflow — landed in reborn-integration via #3927. This PR's Postgres impl satisfies that contract exactly and is exercised against the shared contract harness (ironclaw_hooks::predicate_state::contract, contract-tests feature).

Fail-closed cap alignment

The per-key sample cap is now fail-closed, matching the in-memory backend and the uniform trait contract (#3929 / PR #3635 followup):

• When a key's in-window sample count reaches MAX_SAMPLES_PER_KEY and a new distinct id arrives, record_invocation / record_value return PredicateBackendError::WindowOverflow without inserting — instead of the previous drop-oldest eviction.
• Replay-dedup short-circuits before the cap check: an already-recorded in-window id is a no-op (returns the unchanged aggregate), so replay refusal survives the cap boundary.
• The evaluator maps WindowOverflow to a restrictive DENY/PauseApproval, so overflow surfaces as a refusal, never a silent Allow.

Implementation: the shared transaction body now does trim → atomic dedup-check + pre-insert count (BOOL_OR(id = $id) under the per-key advisory lock) → fail-closed cap check → insert → aggregate → per-scope LRU quota, all inside one READ COMMITTED transaction guarded by a transaction-scoped advisory lock.

Schema

hook_predicate_counters (scope_hash BYTEA, key_hash BYTEA, kind CHAR(1), id TEXT, ts TIMESTAMPTZ, value NUMERIC, PRIMARY KEY (key_hash, id)) plus supporting indexes. Idempotent (CREATE … IF NOT EXISTS), applied via run_migrations(). The PRIMARY KEY (key_hash, id) + ON CONFLICT DO NOTHING is the clock-independent cross-host replay-dedup constraint.

Test plan

• Contract harness (predicate_state_postgres_contract.rs): all 9 contract functions wired, including the new record_invocation_overflow_is_fail_closed. Same suite the in-memory backend runs — proven by construction.
• Adversarial (predicate_state_postgres_adversarial.rs): two-host write storm, cross-host replay (count + value), per-scope LRU eviction, and per_key_sample_cap_fails_closed_under_flood (rewritten from the old drop-oldest assertion to assert WindowOverflow + replay-dedup-at-cap).
• Integration tests are env-gated on IRONCLAW_HOOKS_POSTGRES_URL / DATABASE_URL; they skip-pass without a DB (no testcontainers dep), matching the ironclaw_filesystem / ironclaw_reborn_event_store pattern.

Verification

• cargo fmt --all — clean
• cargo clippy --all --benches --tests --examples --all-features -- -D warnings — zero warnings
• cargo check -p ironclaw_hooks_postgres --features postgres — clean
• cargo test -p ironclaw_hooks_postgres --features postgres — 9 contract + 5 adversarial tests compile and skip-pass (no DB in this env); a real Postgres run is required to exercise the SQL paths.
• In-memory ironclaw_hooks predicate_state suite (27 tests) — green.

Crate placement: Postgres is its own crate (ironclaw_hooks_postgres); the libSQL backend moves to its own crate in a sibling task.

Dual-backend compliance (libSQL counterpart + parity)

Per the repo rule "new persistence must support both PostgreSQL and libSQL," the requirement is satisfied across this staged durable-backend series, not in this PR alone (addresses serrrfirat C-1, Medium):

• PostgreSQL half (this PR, 2/4): ironclaw_hooks_postgres.
• libSQL counterpart: ironclaw_hooks_libsql — PR feat(hooks): LibSqlPredicateStateBackend in own crate (durable backend PR 3/4, replaces #3930) #3936 (hooks-predicate-backend-libsql).
• Cross-backend parity suite: ironclaw_hooks_parity — PR test(hooks): cross-backend adversarial parity suite + A3 closeout (durable backend PR 4/4) #3937, which runs the identical contract assertions against both durable backends and proves they are drop-in interchangeable.

Merge ordering: this PR (#3933) lands first, then the libSQL crate (#3936), then the parity suite (#3937) gates that the two backends are behaviorally equivalent. Both backends share ONE logical two-table typed schema (migrations/V1__predicate_state.sql); only the storage column types differ (Postgres TIMESTAMPTZ + NUMERIC vs. libSQL epoch-ms INTEGER + TEXT). The dependency is also recorded in the crate-level doc comment in lib.rs.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces the ironclaw_hooks_postgres crate, which implements a durable PostgreSQL backend for the hook framework's predicate state. The implementation ensures cross-host consistency and atomicity through the use of Postgres advisory locks and READ COMMITTED transactions. Key feedback includes optimizing the scope-level index to support index-only scans for quota enforcement, removing a redundant database query in the replay detection path, and standardizing binary encoding by using 64-bit length prefixes for canonical hashing.

[gemini-code-assist]

The hook_predicate_counters_scope_idx index currently only covers (scope_hash, kind). To optimize the enforce_scope_quota operation, which performs a COUNT(DISTINCT key_hash) and an LRU victim selection using MAX(ts), the index should be expanded to include key_hash and ts. This would enable index-only scans for these operations, significantly improving performance for tenants with a large number of recorded events.

Suggested change

	CREATE INDEX IF NOT EXISTS hook_predicate_counters_scope_idx
	ON hook_predicate_counters (scope_hash, kind);
	CREATE INDEX IF NOT EXISTS hook_predicate_counters_scope_idx
	ON hook_predicate_counters (scope_hash, kind, key_hash, ts);

[gemini-code-assist]

The record function performs a redundant database query when a replay is detected. The pre_row query already computes the in-window count and sum. Since a replay is a no-op that doesn't modify the table, these values can be returned directly, saving a round-trip to the database.

        let pre_count: i64 = pre_row.get("cnt");
        let pre_total: Decimal = pre_row.get("total");
        // BOOL_OR over an empty set is NULL; treat NULL as "no duplicate".
        let is_replay: bool = pre_row.get::<_, Option<bool>>("dup").unwrap_or(false);

        if is_replay {
            // Replay refusal: the id is already in-window for this key, so
            // this is a no-op against the count/sum. Short-circuit BEFORE
            // the cap check so a replay at the cap dedups rather than
            // overflowing — matching the in-memory contract.
            tx.commit().await.map_err(map_pg)?;
            return Ok(if value.is_some() {
                pre_total
            } else {
                Decimal::from(pre_count.max(0) as u64)
            });
        }

[zmanian]

The replay short-circuit still issues one aggregate(..) call on the no-op path. Kept deliberately after the typed-table rewrite: the pre-row cnt is a COUNT(*) valid only for the invocation table, whereas the value path must return SUM(value). Returning cnt directly would be wrong for values, so the replay path runs the same typed aggregate() the insert path uses (one extra in-window read under the advisory lock already held). It keeps the count/sum return correct per table rather than smuggling a count back through the value path.

[gemini-code-assist]

While pre_count is expected to be non-negative, using .max(0) before casting to usize is a safer practice in this codebase to prevent potential wrapping issues if the database were to return a negative value.

Suggested change

	if pre_count as usize >= MAX_SAMPLES_PER_KEY {
	if pre_count.max(0) as usize >= MAX_SAMPLES_PER_KEY {

[zmanian]

Done in 7547ebf — the fail-closed cap check now clamps pre_count.max(0) as usize, matching the codebase convention against negative-row wrap.

[gemini-code-assist]

Update the documentation to reflect the change to an 8-byte length prefix for binary encoding consistency.

Suggested change

	//! Length-prefixing every field (4-byte big-endian length ++ bytes)
	//! Length-prefixing every field (8-byte big-endian length ++ bytes)

References

1. Prefer using u64 for length prefixes in binary encodings to ensure that conversions from usize are infallible on all supported platforms, avoiding panics and potential DoS vectors caused by .expect() on overflow.

[zmanian]

Done in 7547ebf — doc updated to "8-byte big-endian length ++ bytes".

[gemini-code-assist]

Prefer using u64 for length prefixes in binary encodings to ensure that conversions from usize are infallible and to maintain injectivity on all supported platforms. While the current implementation uses u32 with saturation, u64 is the standard for the project's binary encodings to avoid potential collisions if string lengths were to exceed u32::MAX on 64-bit platforms.

Suggested change

	// 4-byte length prefix makes the field boundary unambiguous; u32 is
	// ample for any tenant/capability/field string we will ever see.
	let len = u32::try_from(field.len()).unwrap_or(u32::MAX);
	hasher.update(&len.to_be_bytes());
	// 8-byte length prefix makes the field boundary unambiguous; u64 is
	// ample for any tenant/capability/field string we will ever see.
	let len = field.len() as u64;
	hasher.update(&len.to_be_bytes());

References

1. Prefer using u64 for length prefixes in binary encodings to ensure that conversions from usize are infallible on all supported platforms, avoiding panics and potential DoS vectors caused by .expect() on overflow.

[zmanian]

Done in 7547ebf. The length prefix is now field.len() as u64 (8-byte big-endian), making the usize->len conversion infallible on all platforms and removing the prior u32::try_from(..).unwrap_or(u32::MAX) saturation corner that could (in principle) alias two fields differing only beyond u32::MAX. Serialization stays strictly injective.

[gemini-code-assist]

The hook_predicate_counters_scope_idx index currently only covers (scope_hash, kind). To optimize the enforce_scope_quota operation, which performs a COUNT(DISTINCT key_hash) and an LRU victim selection using MAX(ts), the index should be expanded to include key_hash and ts. This would enable index-only scans for these operations, significantly improving performance for tenants with a large number of recorded events.

Suggested change

	CREATE INDEX IF NOT EXISTS hook_predicate_counters_scope_idx
	ON hook_predicate_counters (scope_hash, kind);
	CREATE INDEX IF NOT EXISTS hook_predicate_counters_scope_idx
	ON hook_predicate_counters (scope_hash, kind, key_hash, ts);

[zmanian]

Applied in 7547ebf, adapted to the post-rewrite two-table typed schema (the old single hook_predicate_counters table is gone). The per-scope index on BOTH typed tables is now (scope_hash, key_hash, occurred_at) instead of (scope_hash), so enforce_scope_quota's COUNT(DISTINCT key_hash) and MIN(occurred_at)-per-key victim ranking run as index-only scans for tenants with many recorded keys.

[zmanian]

@henrypark133 @serrrfirat durable backend PR 2/4 — PostgresPredicateStateBackend (own crate ironclaw_hooks_postgres). Implements the public async DateTime<Utc> contract from #3927 with fail-closed WindowOverflow cap semantics matching #3929. Atomicity via per-key advisory lock; replay dedup via PRIMARY KEY + ON CONFLICT DO NOTHING (clock-independent). id column TEXT (64-char blake3 hex doesn't fit uuid). Merge caveat: my env had no Postgres, so the SQL fail-closed/dedup paths compiled + skip-passed — needs a real-Postgres CI run before merge. Replaces closed #3932.

[zmanian]

Codex review (advisory) — REQUEST CHANGES

The per-key trim/dedup/cap/insert/aggregate flow is mostly right (parameterized SQL, WindowOverflow after replay detection, correct BOOL_OR(id=$3) dedup, (key_hash, ts) index supports the hot path). One correctness blocker + a CI gap.

Critical

1. Scope-LRU deletes victim buckets without the victim key's advisory lock — crates/ironclaw_hooks_postgres/src/backend.rs:316 / :387. The per-key lock only protects current_key; enforce_scope_quota can delete rows for another key while a concurrent transaction is actively recording that victim key under its own advisory lock. Breaks the "all writers to the same bucket are serialized" claim and can return aggregates that include concurrently-deleted rows. Real deadlock shape: victim-key txn trims rows then waits for the scope lock; LRU txn holds the scope lock then waits to delete those victim rows. Fix: make victim eviction participate in the same lock discipline (acquire/try-acquire victim key locks before deleting, skip in-flight victims), or impose one global scope/key/row lock order.

2. No CI validates this crate against real Postgres — root cargo test doesn't run this workspace member, and the PG tests skip-pass without IRONCLAW_HOOKS_POSTGRES_URL/DATABASE_URL. Highest-risk unexercised: advisory-lock semantics, READ COMMITTED visibility, ON CONFLICT dedup with BYTEA/TEXT, NUMERIC/TIMESTAMPTZ codecs, and the LRU concurrency path above. Add a PG service job running cargo test -p ironclaw_hooks_postgres --features postgres.

Recommendations

• Clock basis: trim-before-dedup means a fast host can free an id early, so replay dedup isn't fully clock-independent under arbitrary skew — tighten the docs (bounded-skew/NTP assumption) or add skew tests.
• LRU quota query scans/groups all scope rows — consider a covering (scope_hash, kind, key_hash, ts) index or per-key summary for high-cardinality tenants.
• Add a record_value overflow/Decimal-sum adversarial case (flood test only covers invocation overflow).

[serrrfirat]

Thermo-nuclear maintainability pass: I think this needs restructuring before merge. The backend compiles, and the contract harness is wired, but there are a few structural issues that make the implementation harder to reason about than it needs to be, especially around quota locking and the hash-only schema shape.

[serrrfirat]

This lock model is not as clean as the comment claims. enforce_scope_quota takes the scope advisory lock after this transaction already holds the current key lock, then deletes arbitrary victim keys without taking their per-key advisory locks. A concurrent transaction for a victim key can hold row locks from trim/insert while waiting for the same scope lock, while this transaction holds the scope lock and waits on those rows. This is a structural problem, not just a race edge. Can we make quota enforcement own the scope mutation boundary explicitly, e.g. scope-first for new-key materialization or deterministic victim-key locking before delete?

[zmanian]

Fixed in 806eacb by making victim eviction obey the same per-bucket serialization a recorder does — your "deterministic victim-key locking before delete" suggestion.

enforce_scope_quota no longer issues a blind set-based DELETE under the scope lock alone. It now:

1. Takes the scope advisory lock and computes the over-quota count (unchanged — this keeps the distinct-key count exact across concurrent new-key materializations).
2. Selects victim candidates ordered by MAX(ts) ASC, over-fetching beyond the evict count.
3. For each candidate, acquires that victim key's per-key advisory lock with the non-blocking pg_try_advisory_xact_lock(int4,int4) — the same lock key the recording path derives for that bucket. If the try-lock succeeds it deletes exactly that key's rows; if it fails (a recorder is mid-flight on that bucket under its own key lock) it skips to the next-staleest candidate.

This closes both halves of what you flagged:

• Deadlock cycle eliminated. The cycle required the LRU txn to block on a victim's row locks while a victim recorder blocks on the scope lock. With a try-lock the LRU txn never waits on an in-flight victim — it observes the held key lock, returns immediately, and skips. No edge where both wait on each other.
• Torn aggregate eliminated. Eviction now holds the victim's per-key lock before deleting its rows, so it can never delete rows out from under a recorder that is aggregating that same bucket. Victim deletes are serialized with recorders of that bucket exactly like two recorders are.

Over-fetching candidates means skips don't leave the scope above quota; if every candidate happened to be in-flight the scope simply stays at-or-near quota until the next newly-material insert reruns the pass (bounded, self-correcting). I chose deterministic victim-key try-locking over a global scope-first ordering because it doesn't block the hot recording path and needs no reordering of the existing key-then-scope acquisition.

Fail-closed WindowOverflow and the dedup/atomicity invariants are untouched.

Tests added (crates/ironclaw_hooks_postgres/):

• eviction_and_record_derive_identical_per_key_lock (lib unit, no DB) — pins the eviction try-lock key derivation equal to the recorder's lock key, so the two lock spaces can never silently diverge.
• eviction_try_lock_does_not_block_on_in_flight_victim (adversarial, real PG) — deterministic raw-SQL reproduction: while txn V holds victim key B's lock, txn L's try-lock on B returns false promptly (under a hard timeout) instead of blocking. This is the load-bearing invariant; pre-fix the blocking DELETE would stall.
• scope_lru_eviction_serializes_against_victim_writes_no_deadlock (adversarial, real PG) — concurrency stress: fresh-key flood (eviction passes) concurrent with a flood against a hot eviction-candidate key; asserts no deadlock/hang under timeout and that the hot key's reported aggregate matches its surviving in-window row count.

Verified against a local Postgres 14: full crate suite green (7 lib unit, 7 adversarial, 9 contract). The integration tests gate on IRONCLAW_HOOKS_POSTGRES_URL/DATABASE_URL and skip (passing) without a DB.

One CI note: this crate currently has no real-Postgres job, so these gated tests are skipped in CI today. Recommend adding a Postgres service job that sets IRONCLAW_HOOKS_POSTGRES_URL so the adversarial/contract suites actually execute on PRs — happy to do that as a follow-up.

[serrrfirat]

This schema hides a simple two-shape domain model behind a generic hash table. The successor spec described explicit invocation/value key columns; here all operator/debug/migration semantics are reconstructed through scope_hash, key_hash, and kind. That makes the durable state harder to inspect and reason about. Unless secrecy of tenant/capability names is an explicit requirement, can we use typed columns/tables and keep hashes as indexes/lock keys rather than making them the only persisted identity?

[zmanian]

Treating the typed-columns-vs-hash-table question itself as a design call for the humans (not changing it here). But the cross-PR maintainability angle surfaced a real divergence worth flagging: the libSQL sibling (#3936) did NOT make the same shape choice.

• Postgres (this PR): one generic table hook_predicate_counters(scope_hash, key_hash, kind CHAR(1), id, ts, value NUMERIC) — both predicate kinds folded behind kind + a nullable value, keyed PRIMARY KEY (key_hash, id).
• libSQL (feat(hooks): LibSqlPredicateStateBackend in own crate (durable backend PR 3/4, replaces #3930) #3936): two typed tables — hooks_predicate_invocations(scope_hash, event_id, occurred_at, tenant_id) and hooks_predicate_values(scope_hash, event_id, occurred_at, value TEXT, tenant_id), each keyed PRIMARY KEY (scope_hash, event_id).

So the two durable backends disagree on (a) one-generic-table vs two-typed-tables, (b) column names (key_hash/id/ts vs scope_hash/event_id/occurred_at), (c) timestamp encoding (TIMESTAMPTZ vs epoch-ms INTEGER), (d) value encoding (NUMERIC vs exact-decimal TEXT), and (e) the dedup primary key (key_hash vs scope_hash). That's exactly the shape critique you raised, now confirmed to be inconsistent across siblings — surfacing for a reconciliation decision rather than fixing unilaterally.

Separately, comparing the two also caught a genuine correctness divergence in the LRU victim selection, which I did fix on this branch (0c102a6): Postgres ranked eviction victims by MAX(ts) while in-memory + libSQL evict oldest-front (MIN(ts) per key). See the dedicated thread.

[zmanian]

Resolved in ef93722 on this branch — Zaki signed off on the typed-columns shape as the canonical decision, coordinated across all three durable-backend branches so both backends end with the same logical schema.

The generic hook_predicate_counters(kind CHAR(1), …) table is gone. It's now two explicit typed tables matching the successor spec and the libSQL sibling's two-table model:

• hooks_predicate_invocations(scope_hash, key_hash, event_id, occurred_at) — in-window COUNT(*) is the invocation count.
• hooks_predicate_values(scope_hash, key_hash, event_id, occurred_at, value NUMERIC NOT NULL) — in-window SUM(value) is the running sum.

The kind discriminator column is eliminated (table identity carries it), and id/ts are renamed to the canonical event_id/occurred_at. Operator/debug/migration semantics are now directly inspectable per table rather than reconstructed through a kind filter. Postgres keeps its native types (BYTEA / TIMESTAMPTZ / NUMERIC); the libSQL sibling (#3936, hooks-predicate-backend-libsql) was reconciled to the IDENTICAL column set + PK in 0b174b1 (native libSQL types: BLOB / epoch-ms INTEGER / TEXT). The cross-backend parity suite (#3937) re-merges both and proves they stay behaviorally interchangeable.

[serrrfirat]

This shared record path is doing too much through value: Option<Decimal>, returning Decimal for both count and sum, then having the invocation path cast it back to u32. That feels like a thin abstraction that saves a few lines by making the main control flow less typed. Can we keep the common lock/trim/dedup/cap pieces as helpers and let record_invocation / record_value remain typed flows with their own aggregation?

[zmanian]

Resolved in ef93722 on this branch. The value: Option<Decimal> double-duty is gone now that the two typed tables make the count-vs-sum distinction structural rather than smuggled through a nullable NUMERIC.

The shared record body still serializes the genuinely common steps (advisory lock, window trim, replay-dedup check, fail-closed cap, ON CONFLICT insert, per-tenant LRU quota) — those are identical across both tables — but the kind-specific parts are now explicitly typed:

• A RecordKind { Invocation, Value } enum selects the table and folds a per-table tag into the scope advisory lock.
• A dedicated aggregate(kind, …) helper runs COUNT(*) for the invocation table and SUM(value) for the value table, so the invocation table — which no longer has a value column at all — is never asked to SUM(value).
• The INSERT column list is matched per table (no value column for invocations; value NOT NULL for values).

record_invocation still narrows the count to u32, but it now does so off an explicit COUNT(*) aggregate rather than a Decimal that was standing in for a count. The control flow is typed end-to-end; nothing is reconstructed by casting a sum back into a count.

[serrrfirat]

The migration file is documented as canonical, but the code executes this separate embedded string. That creates drift risk in the durable schema path. Can we make one source executable, or add a small test that proves migrations/V1__predicate_counters.sql and this constant stay equivalent after normalization?

[zmanian]

Fixed in 0c102a6 (this branch). Removed the hand-copied embedded const entirely — POSTGRES_PREDICATE_SCHEMA is now include_str!("../migrations/V1__predicate_counters.sql"), so the .sql file is the only copy and there is nothing left to drift against. batch_execute tolerates the file's leading -- comment block, so no statement-splitting was needed, and run_migrations() is exercised by the contract + adversarial suites.

Reusable pattern note for the libSQL sibling (#3936): the same embedded-vs-file shape exists there (LIBSQL_PREDICATE_STATE_SCHEMA is a hand-maintained const). Recommend the same include_str! collapse so both durable crates have a single schema source of truth.

[zmanian]

@serrrfirat @henrypark133 re-review please — major redesign per the agreed direction (SHA ef93722): the generic hash table is gone, replaced by two typed tables (hooks_predicate_invocations + hooks_predicate_values) shared canonically with the libSQL backend (#3936). This resolves three of your findings at once: (1) typed columns instead of the (scope_hash, key_hash, kind) hash shape — operator/migration-inspectable; (2) the value: Option<Decimal> double-duty is eliminated via a RecordKind enum + per-table aggregate() (no SUM on the value-less invocation table); (3) SQL drift killed via include_str! of the migration. All invariants preserved (MIN oldest-front victim, fail-closed WindowOverflow, dedup, per-tenant quota, advisory locks). Replied on both threads. Caveat: needs the real-PG CI leg (from #3937) before merge — no PG in the dev env.

[zmanian]

@serrrfirat @henrypark133 friendly ping — every item from your review is addressed as of ef937229b (details in the comment above) and CI is green. Ready for another pass to clear the CHANGES_REQUESTED when you have a moment. Thanks!

[serrrfirat]

Thermo-nuclear maintainability review: I found two blocker-level issues and one structural cleanup that should be addressed before this lands.

[serrrfirat]

This test says Unavailable/deadlock-style failures must not happen, but the spawned task discards the record_invocation result. That means a deadlock-detected DB error, serialization error, or other backend failure still passes as long as the task returns before the timeout. Please have each task return the backend result and assert the allowed outcomes explicitly, so this regression test actually fails on the production failure mode it documents.

[zmanian]

Fixed in 741e0ec. Each spawned task now RETURNS its record_invocation result, and the join collects them into a Vec that is asserted explicitly after the timeout.

Per result the allowed outcomes are: Ok(_) (recorded or replayed), or Err(WindowOverflow {..}) (a fresh/hot key legitimately hitting the per-key sample cap), or a non-deadlock Err(Unavailable(..)) (the new explicit fail-closed quota outcome when every stale victim is momentarily locked). Crucially, an Unavailable whose message contains deadlock or serialize now fails the test — that is exactly the production failure mode (deadlock-detected / serialization-failure DB error) this regression test documents, and previously a discarded let _ = would let it pass as long as the task returned before the 60s timeout.

[zmanian]

Confirmed addressed at the current tip (7547ebf; the fix landed in 741e0ec). Each spawned task now RETURNS its record_invocation result; the join collects them into a Vec asserted explicitly after the timeout. A Unavailable(..) whose message contains deadlock or serialize now FAILS the test — the exact production failure mode this regression documents — instead of passing silently via a discarded let _ =. Allowed outcomes per result: Ok(_), Err(WindowOverflow), or a non-deadlock Err(Unavailable) (the explicit fail-closed quota outcome). Verified: scope_lru_eviction_serializes_against_victim_writes_no_deadlock green against local PG and skip-pass without a DB.

[serrrfirat]

This makes quota enforcement silently best-effort: we over-fetch a fixed 64 candidates, skip locked victims, and then commit Ok(evicted) even if the scope is still above MAX_KEYS_PER_TENANT. That is a structural problem because the caller never rechecks, so the strong per-scope bound becomes an implementation accident under enough in-flight victims. Can we reframe this as an explicit quota outcome, or loop/requery until the cap is actually met or enforcement fails closed?

[zmanian]

Fixed in 741e0ec. Quota enforcement is now an explicit outcome that either meets the cap or fails closed — never a silent best-effort Ok.

enforce_scope_quota no longer over-fetches a fixed 64 and commits whatever it managed to evict. Under the scope advisory lock it now runs a bounded loop:

1. Requery the live COUNT(DISTINCT key_hash) for the scope. If <= MAX_KEYS_PER_TENANT, return Ok(evicted) — the cap is genuinely met (the recount reflects rows this loop already deleted).
2. Otherwise compute the exact deficit to_evict = distinct - MAX_KEYS_PER_TENANT, fetch a fresh oldest-front (MIN(occurred_at)) victim batch, and evict victims one at a time under each victim's per-key pg_try_advisory_xact_lock (deterministic lock order, deadlock-free) — stopping at exactly to_evict so it can never over-evict below the cap.
3. If a whole pass makes zero progress (every stale candidate is locked by an in-flight recorder) while still over cap, it fails closed with PredicateBackendError::Unavailable rather than committing an over-quota scope. The caller's transaction rolls back (the record is not applied) and the evaluator maps the error restrictively — the same posture the per-key cap uses for WindowOverflow. A retry or a concurrent recorder finishing clears the contention.

So the strong per-scope bound is no longer an accident of how many victims happened to be in-flight: a committed record always leaves the scope at-or-under the cap, by construction.

Stateful coverage: the new scope_quota_is_enforced_exactly_not_best_effort test drives a single uncontended writer 200 keys past the cap and asserts the scope holds at EXACTLY MAX_KEYS_PER_TENANT, with the oldest-front keys evicted and the newest surviving. (This test earned its keep — against a live Postgres it caught an over-eviction bug in my first cut, where the inner loop drained the whole 64-candidate batch instead of just the per-pass deficit; the to_evict stop fixes it.)

[zmanian]

Confirmed addressed at the current tip (7547ebf; fix landed in 741e0ec). enforce_scope_quota is now an explicit quota OUTCOME, never silent best-effort. Under the scope advisory lock it loops: requery live COUNT(DISTINCT key_hash); if <= MAX_KEYS_PER_TENANT return Ok(evicted); otherwise compute the exact to_evict deficit and evict precisely that many oldest-front victims (each under its own pg_try_advisory_xact_lock, deadlock-free), stopping at the deficit so it never over-evicts below the cap. If a whole pass makes zero progress while still over cap (every stale candidate locked by an in-flight recorder) it FAILS CLOSED with Unavailable — the caller transaction rolls back and the evaluator maps it restrictively, same posture as the per-key WindowOverflow. A committed record always leaves the scope at-or-under cap by construction. scope_quota_is_enforced_exactly_not_best_effort drives a single uncontended writer 200 keys past the cap and asserts the scope holds at EXACTLY the cap.

[serrrfirat]

The schema moved to typed tables to delete the old nullable value abstraction, but this shared record entry point reintroduces the same hidden mode through value: Option<Decimal> plus RecordKind, with the invariant guarded only by debug_assert_eq. This is a maintainability regression: the public shape says typed paths, while the core transaction still has a nullable mode flag. I think there is a cleaner code-judo move here: use a typed RecordPlan/enum that carries the value only for the value path, or split the two record methods and share smaller helpers for lock/trim/dedup/quota instead of rebuilding a generic record path.

[zmanian]

Fixed in 741e0ec. The shared record no longer carries a nullable mode flag.

The value: Option<Decimal> + RecordKind + debug_assert_eq triad is gone. The core transaction now dispatches on a typed RecordPlan enum:

• RecordPlan::Invocation(PlanCommon) — the count path; it structurally cannot carry a value (the invocation table has no value column).
• RecordPlan::Value { common: PlanCommon, value: Decimal } — the sum path; the recorded numeric lives inside the variant, not in a side-channel.

The genuinely common steps (advisory lock, window trim, replay-dedup, fail-closed cap, ON CONFLICT insert, per-tenant LRU quota) run off the shared PlanCommon via RecordPlan::common(), while the only variant-specific bits — the INSERT column list and the final COUNT(*) vs SUM(value) aggregate — match on the enum. There is no nullable NUMERIC double-duty and no debug-only invariant tying a separate value arg to a separate kind; the type makes "value present iff value table" non-representable otherwise. record_invocation / record_value build the typed plan and map the typed return.

[zmanian]

Confirmed addressed at the current tip (7547ebf; fix landed in 741e0ec). The value: Option<Decimal> + RecordKind + debug_assert_eq triad is gone. The shared record now dispatches on a typed RecordPlan enum: RecordPlan::Invocation(PlanCommon) (count path, structurally cannot carry a value) and RecordPlan::Value { common, value: Decimal } (sum path, the numeric lives in the variant). Common steps run off RecordPlan::common(); the only variant-specific bits — INSERT column list and the COUNT(*) vs SUM(value) aggregate — match on the enum. No nullable NUMERIC double-duty, no debug-only invariant; the type makes "value present iff value table" non-representable otherwise.

[zmanian]

Addressed the post-typed-schema maintainability review (741e0ec)

All three follow-up items from the latest review are fixed; the two earlier BLOCKER items (typed columns, migration single-source) had already landed earlier on the branch and the new line refs pointed at the post-refactor file.

1. Quota enforcement — explicit outcome, never silently best-effort (backend.rs:525). enforce_scope_quota now loops: requery distinct count → evict EXACTLY the per-pass deficit via deterministic oldest-front + per-key try-lock → repeat until the cap is met, or fail closed (Unavailable) if a pass makes zero progress while still over cap. A committed record always leaves the scope at-or-under MAX_KEYS_PER_TENANT, by construction. No more Ok(evicted) over a still-over-quota scope.

2. Nullable-mode anti-pattern removed (backend.rs:185). The shared record path no longer takes value: Option<Decimal> + RecordKind guarded by debug_assert_eq. It dispatches on a typed RecordPlan enum (Invocation cannot carry a value; Value { value } carries it in the variant). Common steps run off shared PlanCommon; only the INSERT columns + final aggregate are variant-specific.

3. Adversarial LRU test asserts outcomes (adversarial.rs:407). Each spawned task RETURNS its result; the join asserts the allowed-outcome set and fails the test on any deadlock/serialization Unavailable — the production failure mode it documents.

Plus a new stateful regression test scope_quota_is_enforced_exactly_not_best_effort proving the cap holds at EXACTLY the bound after a flood. It caught an over-eviction bug in my first cut against a live Postgres (inner loop drained the whole candidate batch instead of the per-pass deficit) — now fixed.

Verification

Against a local Postgres 14: 7 lib unit + 8 adversarial + 9 contract tests green. cargo fmt --all --check, cargo clippy -p ironclaw_hooks_postgres --all-features --tests -- -D warnings, and cargo clippy --all --all-features --tests -- -D warnings all clean. No .unwrap()/.expect() added to non-test code; the fail-closed path uses thiserror's existing PredicateBackendError::Unavailable.

(CI note from the earlier thread still stands: this crate has no real-Postgres CI job, so the DB-gated legs skip-pass on PRs until a Postgres service job sets IRONCLAW_HOOKS_POSTGRES_URL.)

[zmanian]

@serrrfirat all six items from the thermo-nuclear review are addressed in 741e0ec (replies posted inline). Highlights: quota enforcement now loops-until-cap-met-or-fails-closed with deterministic oldest-front per-key locking (no more silent best-effort Ok); the nullable value: Option<Decimal> core replaced with a typed RecordPlan enum; adversarial test now returns + asserts task outcomes so deadlock/serialization failures fail the test; new scope_quota_is_enforced_exactly_not_best_effort stateful test (it caught a real over-eviction bug in my first cut). Typed-columns + migration include_str! were already on-branch (your line refs pointed post-refactor). Verified against a live Postgres: 7 unit + 8 adversarial + 9 contract green; workspace clippy clean. Ready for a re-pass.

[henrypark133]

Code Review (multi-agent)

Intent: Add PostgresPredicateStateBackend for durable cross-host predicate state

Stats: 4 findings (from 4 raw, 4 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability. Reviewers failed: none. Body-only: 0

Performance

1. High Redundant COUNT queries on invocation hot path add 2 extra round trips per record() (backend.rs:361-389, confidence 95) — three COUNT queries where one suffices.
2. Low format! per SQL statement allocates 5-6 Strings per hot-path record() call (backend.rs:258-621, confidence 70)

Tests

1. Medium Postgres evict_older_than has no integration test (backend.rs:709-727, confidence 75)

Security

1. Low Raw Postgres error messages exposed through PredicateBackendError::Unavailable (backend.rs:773-778, confidence 75)

Bugs, Conventions, Local Patterns, Maintainability

No findings.

[henrypark133]

Low — Raw Postgres error messages exposed through PredicateBackendError::Unavailable.

map_pg() and map_pool() convert errors via e.to_string(), passing raw DB error messages to callers without redaction.

Fix: Log the full error with tracing::warn! and return a sanitized message.

[zmanian]

Fixed in 2ee6f0f. map_pg/map_pool now tracing::warn!(error = %e, ...) the full raw error for operators and return a fixed sanitized message (predicate state backend unavailable (database error)) through PredicateBackendError::Unavailable, so raw Postgres error text (which can embed connection/schema details) no longer reaches callers. The evaluator only needs the unavailable signal to fail closed.

[henrypark133]

High — Redundant COUNT queries on invocation hot path add 2 extra round trips per record().

Every non-replay invocation issues three COUNT queries where one suffices. (1) pre_count needed for dedup+cap check. (2) in_window_count: under per-key advisory lock, in_window_count = pre_count + 1 provably. (3) aggregate() issues another COUNT, redundant since in_window_count already holds the value.

Fix: Derive in_window_count from pre_count + 1. Use if pre_count == 0 for scope-quota gate. Use derived count as return aggregate for invocations.

[zmanian]

Fixed in 2ee6f0f. The post-insert in-window count is now derived as pre_count + 1 instead of a second COUNT, and for the invocation table that derived value is returned directly as the aggregate (no third COUNT). This is provable under the per-key advisory lock: the dedup check proved the id absent, the cap gate passed, and INSERT ... ON CONFLICT DO NOTHING therefore added exactly one in-window row, and no concurrent writer can touch this key while we hold its lock. The quota gate now keys on pre_count == 0 (equivalent to the old in_window_count == 1). The value aggregate stays a SUM(value) query since a sum is not derivable from the sample count. Net: 3 COUNTs -> 1 on the invocation hot path.

[henrypark133]

Low — format! per SQL statement allocates 5-6 Strings per hot-path record() call.

Every SQL statement in record() uses format! to interpolate the table name, allocating a new String on every call.

Fix: Pre-build statement strings at construction time.

[zmanian]

Fixed in 2ee6f0f. Added a TableStatements struct that pre-formats every record()/reaper SQL statement once per typed table at backend construction; record() selects the matching set via the plan variant. No more per-call format! allocations on the hot path.

[henrypark133]

Medium — Postgres evict_older_than has no integration test.

The Postgres override performs two DELETE statements but no test exercises the Postgres implementation.

Fix: Add evict_older_than_removes_stale_rows_from_both_tables adversarial test.

[zmanian]

Fixed in 2ee6f0f. Added evict_older_than_removes_stale_rows_from_both_tables to the adversarial suite: it records stale + fresh rows on BOTH typed tables, calls evict_older_than(cutoff), asserts the returned count is exactly the 3 stale rows, and verifies via a direct row count (invocation table) and a replay-read (value table) that only the fresh rows survive. Same env-gate pattern as the rest of the suite (skips passing without a live DB).

[zmanian]

Addressed @henrypark133's latest CHANGES_REQUESTED review (all 4 findings) in 2ee6f0f:

• High (perf) — redundant COUNT queries: derive post-insert in-window count as pre_count + 1 and return it directly as the invocation aggregate. 3 COUNTs → 1 on the invocation record() hot path. Correctness rests on the per-key advisory lock (dedup proved absent + cap passed + ON CONFLICT inserted exactly one row, no concurrent same-key writer). Value path keeps its SUM(value) query.
• Low (perf) — format! per statement: new TableStatements struct pre-formats all SQL once per typed table at construction.
• Low (security) — raw PG errors exposed: map_pg/map_pool log the raw error at warn and return a sanitized "backend unavailable" message.
• Medium (tests) — evict_older_than untested: added evict_older_than_removes_stale_rows_from_both_tables to the adversarial suite.

Preserved: atomic record-and-read transaction + advisory-lock serialization, tenant-scoped keying, fail-closed cap/quota posture, shared identity hashing, durable-backend contract suite, libSQL-sibling parity. Removed the now-unused RecordPlan::table(). fmt/clippy clean; cargo test -p ironclaw_hooks_postgres --all-features green (PG-gated tests skip without a live DB).

[henrypark133]

Review of #3933: PostgresPredicateStateBackend

No critical bugs found. Two notes:

Low — pg_try_advisory_xact_lock (non-blocking) for victim eviction: silent skip on lock contention

crates/ironclaw_hooks_postgres/src/backend.rs — victim eviction path:

The victim bucket's per-key advisory lock is taken with pg_try_advisory_xact_lock (non-blocking). If a concurrent writer holds that victim's lock, try_advisory returns false and the victim is skipped. The doc comment says "keeps evicting until the per-scope cap is met, or FAILS CLOSED" — but if all victim candidates are locked, the scope quota may remain over-cap and the call returns WindowOverflow rather than succeeding. This is the correct fail-closed outcome (documented), but worth verifying the test suite covers the case where ALL candidate victims are concurrently locked and the call correctly surfaces WindowOverflow rather than silently accepting over-quota.

Nit — advisory lock key is a 64-bit hash split into two i32s

Hash collision between two different bucket keys maps them to the same advisory lock key → unnecessary serialization of unrelated writers. Not a correctness issue (just throughput), and 64-bit collision space makes it rare. No action needed, just worth knowing.

[serrrfirat]

Code Review — nearai/ironclaw #3933

PostgresPredicateStateBackend (durable backend PR 2/4)

Category	Findings	Severity
Security	0	—
Bugs	0	—
Performance	2	Low
Tests	2	Low
Conventions	2	Medium / Low

Reviewers: security · bugs · performance · tests · conventions
Head: 2ee6f0f5 | Base: 16bb43d5

---

The implementation is solid — the advisory-lock discipline, typed RecordPlan enum, fail-closed cap, and replay-dedup logic are correct. The adversarial test suite (scope_quota_is_enforced_exactly_not_best_effort, scope_lru_eviction_serializes_against_victim_writes_no_deadlock, per_key_sample_cap_fails_closed_under_flood) is comprehensive. No Critical or High findings.

🟡 Conventions — Medium

C-1 · tier-1-#1 · libSQL backend absent: AGENTS.md must support both PostgreSQL and libSQL

AGENTS.md: "New persistence behavior must support both PostgreSQL and libSQL. Add new DB operations to the shared DB trait first, then implement both backends."

This PR adds PostgresPredicateStateBackend + schema + migrations with no libSQL counterpart. The rule is a hard MUST. The PR body names the staged 4-PR series, which partially justifies the approach, but the repo rule requires both backends before the new persistence surface is considered complete.

Recommendation: Before merging, either (a) link the libSQL PR (PR 3 or 4 in the series) and block merge on it landing first, or (b) open a tracking issue and add a TODO comment in lib.rs that the libSQL backend is required. The PR series approach is reasonable — just make the dependency explicit.

---

🔵 Conventions — Low

C-2 · tier-1-#1 · enforce_scope_quota Unavailable messages bypass DB_UNAVAILABLE_MSG sanitization

map_pg / map_pool use DB_UNAVAILABLE_MSG ("predicate state backend unavailable (database error)") precisely because "the raw error... can reach the evaluator/caller" (backend.rs:843). The quota enforcement path constructs rich Unavailable(format!(...)) messages that expose MAX_KEYS_PER_TENANT and internal lock-contention state. These messages are not credentials, but they break the established sanitization invariant. Inline comment attached.

---

🔵 Performance — Low (×2)

Inline comments attached at the relevant lines.

---

🔵 Tests — Low (×2)

Inline comments attached at the relevant lines.

[serrrfirat]

[C-2 · Conventions/Low] enforce_scope_quota constructs detailed Unavailable(format!(...)) messages that expose MAX_KEYS_PER_TENANT and lock-contention state, bypassing the DB_UNAVAILABLE_MSG sanitization pattern established at line 843 ("whose payload can reach the evaluator/caller"). The quota constants are not sensitive, but the inconsistency means the evaluator/caller could observe different message shapes from the same error type depending on the failure path.

Fix: Extract quota-enforcement failure messages into constants (like DB_UNAVAILABLE_MSG) or reuse a single sanitized message. The evaluator should only need the error type, not the message payload, to map to a restrictive outcome.

const QUOTA_CONTENDED_MSG: &str = "predicate state backend unavailable (quota enforcement contended)";
const QUOTA_BUDGET_MSG: &str = "predicate state backend unavailable (quota enforcement budget exhausted)";

[zmanian]

Correct — enforce_scope_quota builds Unavailable(format!(...)) payloads at backend.rs:670 and :684 that leak MAX_KEYS_PER_TENANT and contention state, while map_pg/map_pool route every DB failure through the sanitized DB_UNAVAILABLE_MSG (backend.rs:805, henrypark finding). Will fix: hoist both quota-failure messages into const QUOTA_CONTENDED_MSG/QUOTA_BUDGET_MSG so the evaluator only sees the error type, matching the sanitization contract.

[zmanian]

Fixed in 34d1d60: hoisted both quota fail-closed messages into sanitized constants QUOTA_CONTENDED_MSG and QUOTA_BUDGET_MSG (mirroring DB_UNAVAILABLE_MSG). The operational detail (MAX_KEYS_PER_TENANT, MAX_PASSES, contention state) now goes to tracing::debug! and the Unavailable payload returned to the evaluator/caller is the sanitized constant only.

[serrrfirat]

[P-1 · Performance/Low] scope_advisory_lock_key(scope_ref, plan.lock_tag()) recomputes a blake3 hash on every enforce_scope_quota call (every count == 1 new-key write). The scope bytes are already an artifact of hashing, and the kind tag is one byte — this is a second blake3 invocation per new-key path.

Fix: Precompute and cache the scope lock key in PlanCommon (alongside scope and key) during record_invocation/record_value setup, so it's derived once not twice:

struct PlanCommon {
    scope: Digest,
    key: Digest,
    scope_lock_key: i64,  // precomputed
    label: String,
}

Not blocking — blake3 is fast and this path only runs on new-key inserts.

[zmanian]

Accurate — scope_advisory_lock_key(scope_ref, plan.lock_tag()) (backend.rs:562) recomputes a blake3 over the scope digest on each new-key write, and PlanCommon already carries scope/key so it could be precomputed once. Deferring: this only runs on the in_window_count == 1 new-key path (backend.rs:437), not steady-state same-key writes, and adding a field to PlanCommon touches both record_invocation/record_value constructors — a follow-up since you flagged it non-blocking and blake3 is cheap here.

[zmanian]

Agreed-deferred per the posted reply; not touched in this pass.

[serrrfirat]

[P-2 · Performance/Low] The drop(tx) rollback here is documented as intentional (trim not committed independently of a rejected record). However, there is a subtle repeated-work case: if a key has exactly MAX_SAMPLES_PER_KEY in-window rows and additional stale (out-of-window) rows, each overflow write attempt will: (1) trim the stale rows, (2) read pre_count = MAX_SAMPLES_PER_KEY, (3) fail the cap check, (4) rollback — restoring the stale rows. The next overflow attempt repeats the same cycle.

In practice this requires the key to be simultaneously at full in-window capacity and have unreaped stale rows (an unusual but possible state). The stale rows are eventually cleaned by evict_older_than, but until then, each overflow write generates a trim-and-rollback cycle unnecessarily.

Fix (optional): Commit the trim unconditionally before the cap check, then start a fresh transaction for the cap check + insert. This avoids repeated stale-row churn. Or add a comment noting the wasted-trim scenario as a known minor cost of the fail-closed rollback design.

[zmanian]

The trim-and-rollback churn is real for the simultaneous full-in-window + unreaped-stale state, and drop(tx) at backend.rs:383 does discard the trim. Deferring to a comment rather than the two-transaction restructure: splitting the trim commit from the cap check would break the fail-closed atomicity guarantee the rollback exists to provide (caller observes a clean no-write overflow), and the stale rows are bounded and reaped by evict_older_than. Will add a comment at backend.rs:383 noting the wasted-trim cost as a known minor cost of the fail-closed design.

[zmanian]

Agreed-deferred per the posted reply; not touched in this pass.

[serrrfirat]

[T-1 · Tests/Low] The MAX_PASSES budget exhaustion path (the Err(...) at line 684) and — separately — a scenario where every candidate key in a pass is locked (!progressed at line 662 → Err) are the two critical fail-closed branches of enforce_scope_quota. While scope_lru_eviction_serializes_against_victim_writes_no_deadlock permits Unavailable as a pass-through outcome, no test asserts that either of these specific code paths fires.

Suggested test: In the adversarial suite, set up a scope at MAX_KEYS_PER_TENANT + 1 with all candidate keys mid-write (holding their per-key advisory locks), then verify that enforce_scope_quota returns Err(PredicateBackendError::Unavailable) — confirming the !progressed fail-closed path is exercised, not just silently skipped.

tests::adversarial::scope_quota_fails_closed_when_all_candidates_locked covering !progressed path

[zmanian]

Confirmed — scope_lru_eviction_serializes_against_victim_writes_no_deadlock (predicate_state_postgres_adversarial.rs:370) only permits Unavailable as a pass-through (asserts it isn't a deadlock) and never forces the !progressed branch at backend.rs:662. Deferring: the adversarial suite is gated on a live Postgres (db_url()), and deterministically pinning every candidate key mid-write to drive !progressed is a non-trivial harness addition. Will add scope_quota_fails_closed_when_all_candidates_locked in the durable-backend test follow-up.

[zmanian]

Agreed-deferred per the posted reply; not touched in this pass.

[serrrfirat]

[T-2 · Tests/Low] The Err(_) => now branch (lines 274–275) fires when window exceeds chrono's maximum Duration — e.g., Duration::MAX. In that case cutoff saturates to now, meaning the entire window is treated as in-scope (nothing trimmed). This is the conservative/correct behavior, but it's never exercised by any unit test.

Suggested test:

#[test]
fn cutoff_with_overflow_window_saturates_to_now() {
    let now = DateTime::from_timestamp(1_700_000_000, 0).unwrap();
    // Duration::MAX exceeds chrono's max i64 nanoseconds
    assert_eq!(PostgresPredicateStateBackend::cutoff(now, Duration::MAX), now);
}

This belongs in tests inside backend.rs (where eviction_and_record_derive_identical_per_key_lock lives) and requires making cutoff accessible from tests (it's already a fn on the struct, accessible via use super::*).

[zmanian]

Correct — the Err(_) => now saturation arm of cutoff (backend.rs:274) is unexercised. It's a pure fn cutoff(now, window) on the struct, testable via use super::* with no Postgres. Will fix: add cutoff_with_overflow_window_saturates_to_now asserting cutoff(now, Duration::MAX) == now to the in-module tests block alongside eviction_and_record_derive_identical_per_key_lock.

[zmanian]

Fixed in 34d1d60: added cutoff_with_overflow_window_saturates_to_now to the in-module tests block, asserting cutoff(now, Duration::MAX) == now (the Err(_) => now saturation arm trims nothing). Pure fn, runs without live Postgres; passes under --features postgres.

[zmanian]

@serrrfirat C-1 is addressed: the dual-backend rule is now explicit in the PR body and crate docs (da1aaaff5) — libSQL counterpart is #3936, parity suite is #3937. C-2/T-2 were already fixed on head (34d1d6081); P-1/P-2/T-1 deferrals are reasoned in the inline replies. No Critical/High in your June 3 pass — could you approve or dismiss the May 23 CHANGES_REQUESTED?

[zmanian]

@henrypark133 This one is ready for your re-review as well — all findings from the June 3 review pass are addressed (see the summary comment above for commits), and your review request is still pending on it.

[henrypark133]

Code Review (multi-agent, re-review)

Intent: PostgresPredicateStateBackend — durable backend PR 2/4.

Stats: 8 new findings (1 High, 3 Medium, 4 Low) after deduping against 52 prior comments. 8 reviewers run.

Previously raised — author has sound reply, no re-block

• backend.rs evict_older_than untested in Postgres — fixed in 2ee6f0f55 (test added).
• backend.rs:775 map_pg/map_pool raw error exposure — fixed in 2ee6f0f55 (sanitized return + raw at warn for operators).
• backend.rs value: Option shared record path — refactored to typed tables in ef937229b / 2ee6f0f55 (TableStatements).
• backend.rs:552 lock-ordering / deadlock concern (serrrfirat) — addressed in 741e0ecac (explicit quota outcome, try-lock semantics).
• backend.rs:27 COUNT-on-hot-path round-trip — addressed in 741e0ecac.
• backend.rs:562 scope_advisory_lock_key recomputes blake3 — acknowledged by author as small per-new-key cost.
• backend.rs:383 drop(tx) trim-rollback churn — acknowledged as intentional trade-off.
• schema.rs POSTGRES_PREDICATE_SCHEMA hand-copied const — replaced with include_str! in 0c102a631.
• predicate_state_postgres_adversarial.rs:405 discarded JoinHandle results — fixed in 741e0ecac.

Tests

1. High record_value WindowOverflow fail-closed has no Postgres test (tests/predicate_state_postgres_adversarial.rs:245, conf 90) — value table cap untested; only invocation is. See inline.
2. Medium scope_advisory_lock_key invocation-vs-value distinctness has no unit test (backend.rs:798, conf 75) — pin the b"i"/b"v" disjointness invariant. See inline.
3. Medium evictions counter must not advance on rolled-back WindowOverflow — no test (backend.rs:583, conf 75). See inline.

Bugs

4. Medium evict_older_than runs two DELETEs without a transaction; partial delete on second failure (backend.rs:751, conf 75) — atomicity gap separate from the test added in 2ee6f0f55. See inline.

Local patterns

5. Low New crate missing AGENTS.md required by every workspace crate (Cargo.toml, conf 100). See inline.

Maintainability

6. Low advisory_lock_key is a trivial pass-through wrapper over advisory_lock_key_from_bytes (backend.rs:771, conf 85). See inline.
7. Low TableStatements::new(table, with_value_column: bool) uses a mode flag bool (backend.rs:165, conf 75) — prefer two named constructors. See inline.

Conventions

8. Low Crate accepts raw deadpool_postgres::Pool rather than routing through RootFilesystem per .claude/rules/database.md (backend.rs:222, conf 55) — may have valid rationale; record it. See inline.

[henrypark133]

High — per_key_sample_cap_fails_closed_under_flood exercises the invocation path only. record_value runs through the same record() body but the cap-reject path on hooks_predicate_values is untested. A regression in the value-table INSERT or aggregate_sum would silently allow over-cap values rather than failing closed.

Fix: Add per_value_key_sample_cap_fails_closed_under_flood mirroring this test against record_value; assert WindowOverflow at MAX_SAMPLES_PER_KEY+1; assert in-window replay still dedups.

New finding — not raised in prior reviews.

[henrypark133]

Medium — evict_older_than runs two DELETE statements on a non-transactional client. If the invocation DELETE commits and the value DELETE fails, invocation rows are permanently gone while value rows remain, and the function returns Err — the caller has no indication of partial work. A retry would delete 0 additional invocation rows but delete the remaining value rows.

Fix: Wrap both DELETEs in a transaction (begin → execute both → commit), rolling back on any error so either both tables are reaped or neither is.

New finding — separate from the test added in 2ee6f0f55; not raised in prior reviews.

[henrypark133]

Medium — lock_tag (b"i" vs b"v") is what keeps invocation and value scope-quota passes from serializing against each other. A collision would make every scope-quota pass on hooks_predicate_values serialize behind passes on hooks_predicate_invocations for the same tenant. There is no unit test pinning the disjointness invariant, and it is a pure function testable without Postgres.

Fix: Add invocation_and_value_scope_lock_keys_are_distinct asserting scope_advisory_lock_key(scope, b"i") != scope_advisory_lock_key(scope, b"v") for the same scope bytes.

New finding — not raised in prior reviews.

[henrypark133]

Medium — evictions_observed() increments only after tx.commit(). When a per-key cap overflow rolls back via drop(tx), the counter must not advance. No test drives a key to overflow then asserts the counter is unchanged. A bug crediting evictions on rollback would produce spurious telemetry without corrupting data — still a correctness gap.

Fix: Add evictions_counter_unchanged_on_window_overflow driving a key to MAX_SAMPLES_PER_KEY+1, then asserting evictions_observed() is unchanged from the pre-overflow snapshot.

New finding — not raised in prior reviews.

[henrypark133]

Low — crates/AGENTS.md states "Every crate with Cargo.toml has a crate-local AGENTS.md" and directs agents to treat it as the first file to load. Sibling durable-backend crates ironclaw_reborn_event_store and ironclaw_filesystem ship one; ironclaw_hooks_postgres does not.

Fix: Add crates/ironclaw_hooks_postgres/AGENTS.md following the shape used by ironclaw_reborn_event_store/AGENTS.md (What This Crate Owns / Do Not Move In Here / Validation / Agent Notes).

New finding — not raised in prior reviews.

[henrypark133]

Low — advisory_lock_key(&Digest) exists only to call advisory_lock_key_from_bytes(&[u8]). Digest is [u8; 32]; slice coercion makes &digest[..] (or just &digest) a zero-cost deref. The wrapper introduces a named concept without adding any safety or clarity.

Fix: Delete advisory_lock_key. Update the two call sites (here and in enforce_scope_quota) plus the per-key test to call advisory_lock_key_from_bytes directly.

New finding — not raised in prior reviews.

[henrypark133]

Low — TableStatements::new(table, with_value_column: bool) selects between two insert-SQL shapes via a boolean. Always called with literals (false/true) paired with RecordPlan::Invocation/Value. Repo types convention prefers enums or named constructors over boolean mode flags.

Fix: Replace with TableStatements::for_invocations(table) and TableStatements::for_values(table); each builds its insert SQL directly with no branch.

New finding — not raised in prior reviews.

[henrypark133]

Low (confidence 55) — .claude/rules/database.md directs new crates to mount through RootFilesystem and let the wiring layer pick the backend. This crate accepts a raw deadpool_postgres::Pool. The sibling crate cited as the structural reference (ironclaw_reborn_event_store) routes through RootFilesystem/ScopedFilesystem. The advisory-lock and sliding-window aggregate requirements may genuinely justify the bypass, but no rationale is recorded.

Fix: Either route through Arc<dyn RootFilesystem> matching the event-store pattern, or add an inline rationale comment at pub fn new(pool: Pool) and a sentence in the PR body explaining why the abstraction cannot serve here.

New finding — not raised in prior reviews.