Repository navigation
Conversation
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
TheSentinel454
left a comment
There was a problem hiding this comment.
🤖 Approving at d835556. Splitting the drop out of #8494 means the 0076 step can be rolled out and judged on its own. Every kind:39002 writer still has the application roster check, and the migration test fails if 0076 stops dropping the trigger. The SQL is identical to the 0074 we ran yesterday against a local relay, where a #8611 relay started both with and without the trigger. The one red PostgreSQL test (session_statement_timeouts_bound_writer_and_reader_sessions, PoolTimedOut) does not touch this change, and it passed in #8494, which contains all of this PR. As the PR says, merge only after #8611 runs in every environment.
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude's code review agent — APPROVE at exact head d835556200dce52c0b50c6d1c24816c59fd0ca91 (base 3dbc95c3f668547e501c7d5f306d85ca5dcd055e).
I reviewed this as the integrated A-Team verdict, incorporating the product/operator lane and the systems/integration lane. No unresolved author-actionable defect was found.
What I verified:
- Freshness/review state: GitHub PR head and local review head both stayed at
d835556200dce52c0b50c6d1c24816c59fd0ca91; authenticated reviewer and PR author are different GitHub identities; there are no unresolved review threads; required checks are green, with Mobile/Web skipped by path filter. - Migration scope:
migrations/0076_retire_channel_roster_trigger.sqlonly dropstrg_events_guard_channel_roster_snapshotandguard_channel_roster_snapshot()with boundedlock_timeout/statement_timeout; no data rewrite or backfill is introduced. - Desired-state convergence: the schema no longer installs the retired roster trigger/function, while the remaining events floor/write-fence triggers stay intact.
- Runtime contract: the application-side kind
39002roster validation remains on the production write seams before event insertion/replacement, including the relay-authored member snapshot path. - Regression coverage: the migration/schema tests pin the migration count/statements, desired-state absence, and the post-0076 behavior where raw stale roster inserts are no longer blocked by the retired trigger.
Validation evidence reviewed:
- GitHub required checks at this head: Rust lint/unit, PostgreSQL tests, Desktop E2E Relay, Backend Integration, Relay E2E, Security, DCO, Desktop RC all green; Mobile/Web skipped by path filter.
- Local exact-head lane evidence:
cargo fmt --check,scripts/test-postgres-test-discovery.sh,cargo test -p buzz-db --lib embedded_migrator_contains_consolidated_initial_schema -- --nocapture, andcargo test -p buzz-db --liball passed in the lane checkouts. Local Postgres-gated tests were not rerun outside CI.
Important operational gate, not author rework: the PR's documented required stop is still real. The visible relay-v* tags currently top out at relay-v0.2.1, and none contain #8611's merge commit (646ef8abf9bc5ac3e698826b38811033537a4806). Do not merge/deploy 0076 until a relay-v* release containing #8611 is tagged and deployed to every serving and rollback target, and the release notes name that required stop.
Author action: none from this review.
Verification owner: release/merge operator must enforce the #8611 required-stop gate before merge/deploy.
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 3dbc95c3f668547e501c7d5f306d85ca5dcd055e..d835556200dce52c0b50c6d1c24816c59fd0ca91 (exact head d835556200dce52c0b50c6d1c24816c59fd0ca91)
Risk: medium-high operational/database risk. The code change is narrow, but it retires a database trigger/function that used to enforce the kind:39002 roster snapshot contract at the database boundary.
Behavior/contracts traced: migration 0076_retire_channel_roster_trigger, desired-state schema convergence, event insert/replaceable writer seams, relay-authored roster snapshot path, PostgreSQL/relay CI evidence, and the PR’s documented rollout prerequisite for #8611.
Findings: no unresolved author-actionable code/product defect.
Integrated review evidence:
- Product/operator lane found no user-facing or operator-trust defect. The migration documents the required #8611 stop, lock/statement timeout behavior, and drops only
trg_events_guard_channel_roster_snapshotplusguard_channel_roster_snapshot()(migrations/0076_retire_channel_roster_trigger.sql:1-20). - Systems lane verified desired-state convergence:
schema/schema.sql:65-68no longer includes the roster function,schema/triggers/events.sql:1-12keeps only the remaining events triggers, andscripts/reconcile-schema-after-pgschema.sql:16-21no longer carries roster child-trigger cleanup. - The app-side roster guard remains on the production write seams before event writes: canonical roster validation in
crates/buzz-db/src/store/channel_members.rs:72-152; generic append calls before insert incrates/buzz-db/src/store/event.rs:492and:1880; replaceable writers guard before delete/insert incrates/buzz-db/src/store/replaceable.rs:120-132,:263-264, and:442-492; relay-authored member snapshots revalidate in the locked writer path incrates/buzz-db/src/store/channel_members.rs:398-456and:510-526. - Test coverage pins the retirement: migration count/statements in
crates/buzz-db/src/runtime/migration.rs:779-788, desired-state absence in:1331-1347, andmigration_0076_retires_the_roster_triggerincrates/buzz-db/src/store/channel_members.rs:5049-5113proves pre-0076 trigger presence, post-0076 absence, and raw stale-roster acceptance after retirement.
Author action: none.
Verification owner: release/operator. The PR’s own rollout gate must be enforced before merge/deploy: do not merge/deploy migration 0076 until a relay-v* containing #8611’s merge commit 646ef8abf9bc5ac3e698826b38811033537a4806 is tagged and deployed to every serving and rollback relay target, with release notes naming that required stop. Visible relay-v* tags still topped out at relay-v0.2.1 in the review-room check and did not contain that commit. That is an operational release gate, not author code rework.
Validation: review lanes reported exact-head green required GitHub checks for Rust Lint, Unit Tests, PostgreSQL Tests, Desktop E2E Relay, Backend Integration, Relay E2E, Security, DCO, and Desktop RC, with Mobile/Web skipped by path filter. Local exact-head checks reported by the lanes: cargo fmt --check PASS, scripts/test-postgres-test-discovery.sh PASS, cargo test -p buzz-db --lib embedded_migrator_contains_consolidated_initial_schema -- --nocapture PASS, and cargo test -p buzz-db --lib PASS (182 passed; 642 ignored). Local Postgres-gated tests were not rerun by the lanes; exact-head GitHub PostgreSQL Tests were green.
Manual/native evidence: not applicable; this is a relay/database migration retirement with no Desktop/UI runtime behavior.
Residual risk: rollout ordering is the material remaining risk and is owned by the release/operator gate above.
Agent correction: duplicate exact-head approval submitted after an equivalent approval already existed; dismissing duplicate per review idempotency contract.
d835556 to
33d4fbb
Compare
`bot` has the same permissions as `member`; it only labels a key with a recorded agent owner. Writers store `member` for both ordinary roles, and every role read computes the label: `bot` exactly when users.agent_owner_pubkey is set for the key in that community. - The app-side roster check treats `member` and `bot` as one role. - When first AUTH or a v1/v2 invite claim records an agent's owner, the relay updates its owner caches and republishes the agent's rosters in the background, at most two republishes at a time per process. - A re-add that changes only member/bot reports the member as already present. - The relay refuses to start while the 0032 roster trigger is installed: migration 0079 (#8696) must run first. This restacks the branch onto #8696 after main took migrations 0076-0078. It replaces these earlier commits: - Update the owner caches when an invite claim records an agent owner (47458e4) - Republish an agent's rosters when an invite claim records its owner (35fc447) - Renumber the roster-trigger retirement to migration 0076 (62290cf) - test(relay): bind the member/bot re-add report to the kind:9000 path (c1aabfa) - Treat member and bot as one role when reporting a re-add (d20e11d) - Drop the one-shot member/bot roster reconcile scan (f8ebee0) - Refuse to start while the 0032 roster trigger is installed (a4311bb) - Bound roster republishes and tighten the 0074 rollout (4c2448c) - test(relay): first AUTH is not held by a contended roster lock (143e4c7) - Compute the member/bot label at read time and retire the roster trigger (02ccba9) Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
33d4fbb to
64b5dcb
Compare
`bot` has the same permissions as `member`; it only labels a key with a recorded agent owner. Writers store `member` for both ordinary roles, and every role read computes the label: `bot` exactly when users.agent_owner_pubkey is set for the key in that community. - The app-side roster check treats `member` and `bot` as one role. - When first AUTH or a v1/v2 invite claim records an agent's owner, the relay updates its owner caches and republishes the agent's rosters in the background, at most two republishes at a time per process. - A re-add that changes only member/bot reports the member as already present. - The relay refuses to start while the 0032 roster trigger is installed: migration 0079 (#8696) must run first. This restacks the branch onto #8696 after main took migrations 0076-0078. It replaces these earlier commits: - Update the owner caches when an invite claim records an agent owner (47458e4) - Republish an agent's rosters when an invite claim records its owner (35fc447) - Renumber the roster-trigger retirement to migration 0076 (62290cf) - test(relay): bind the member/bot re-add report to the kind:9000 path (c1aabfa) - Treat member and bot as one role when reporting a re-add (d20e11d) - Drop the one-shot member/bot roster reconcile scan (f8ebee0) - Refuse to start while the 0032 roster trigger is installed (a4311bb) - Bound roster republishes and tighten the 0074 rollout (4c2448c) - test(relay): first AUTH is not held by a contended roster lock (143e4c7) - Compute the member/bot label at read time and retire the roster trigger (02ccba9) Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
Every kind:39002 roster writer already checks the roster against canonical channel membership in the application (#8316), and since #8611 no relay requires the 0032 trigger at startup. Migration 0081 drops the trigger and its function, with the lock and statement timeouts from 0049. The desired-state schema and the pgschema reconcile script no longer install them. Relays from this change still store and publish the requested role, so they serve correctly whether or not 0081 has run. Every relay, and every rollback target, must run #8611 or later before 0081 is applied. Tests that compared the trigger with the app check, or that disabled the trigger first, now test the app check alone. A new Postgres test applies 0081 to a database migrated through 0080 and checks that the trigger and function are gone. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
64b5dcb to
ecce12f
Compare
`bot` has the same permissions as `member`; it only labels a key with a recorded agent owner. Writers store `member` for both ordinary roles, and every role read computes the label: `bot` exactly when users.agent_owner_pubkey is set for the key in that community. - The app-side roster check treats `member` and `bot` as one role. - When first AUTH or a v1/v2 invite claim records an agent's owner, the relay updates its owner caches and republishes the agent's rosters in the background, at most two republishes at a time per process. - A re-add that changes only member/bot reports the member as already present. - The relay refuses to start while the 0032 roster trigger is installed: migration 0081 (#8696) must run first. This restacks the branch onto #8696 after main took migrations 0076-0078. It replaces these earlier commits: - Update the owner caches when an invite claim records an agent owner (47458e4) - Republish an agent's rosters when an invite claim records its owner (35fc447) - Renumber the roster-trigger retirement to migration 0076 (62290cf) - test(relay): bind the member/bot re-add report to the kind:9000 path (c1aabfa) - Treat member and bot as one role when reporting a re-add (d20e11d) - Drop the one-shot member/bot roster reconcile scan (f8ebee0) - Refuse to start while the 0032 roster trigger is installed (a4311bb) - Bound roster republishes and tighten the 0074 rollout (4c2448c) - test(relay): first AUTH is not held by a contended roster lock (143e4c7) - Compute the member/bot label at read time and retire the roster trigger (02ccba9) Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
|
Hi @loganj, I have resolved the merge conflicts with the latest
A clean, zero-conflict branch is available at: You can pull it directly into your branch: git fetch https://github.com/presentyourlove/buzz.git fix/retire-roster-trigger-8696
git merge --ff-only FETCH_HEAD |
Summary
Migration 0081 drops the 0032 channel roster trigger (
trg_events_guard_channel_roster_snapshot) and its function (guard_channel_roster_snapshot()).The trigger repeats a check that the relay already does in Rust. Every kind:39002 roster writer checks the roster against the channel's membership before it saves it (#8316). Since #8611, no relay requires the trigger at startup. This PR is the last step of the trigger retirement sequence in
docs/database.md.This PR was split out of #8494 so the migration can roll out by itself. A relay from this PR does not change how it stores or publishes roles, and it has no new startup check. It serves correctly whether or not 0081 has run, so merging it does not block relay rollouts. #8494 stacks on this PR and must not merge until 0081 has run in every environment.
Details
DROP TRIGGERandDROP FUNCTION, withlock_timeout = '1s'andstatement_timeout = '5s'as in 0049.DROP TRIGGERtakesACCESS EXCLUSIVEon eacheventspartition in turn, so event reads and writes block while it runs. A timeout rolls it back with no change, and it can be retried.schema/functions/guard_channel_roster_snapshot.sqland the trigger inschema/triggers/events.sqlare removed, as are the per-partition trigger drops inscripts/reconcile-schema-after-pgschema.sql.Rollout
Required stop: #8611. A relay older than #8611 refuses to start without the 0032 trigger. Every environment, and every rollback target, must run a release that contains #8611 before 0081 runs. #8611 has merged to main but is not in a tagged relay release yet; the latest is
relay-v0.2.1. The release notes for this PR's release must name that release as the required stop.Ordering with main's artifact migrations. Main added migrations 0076–0078 for incremental artifact reads, then 0079 (NIP-FI verified profiles) and 0080 (channel auto-join), so this migration is now 0081, and it runs only after 0076–0080. On a populated database, 0076 and 0078 refuse to run until their indexes are built out of band (
docs/artifact-sync-deployment.md). Until those steps are done, a relay with auto-migration on stops at 0076, and 0081 does not run.Merge gate. Every push to main publishes relay
:mainimages, and the Helm chart defaults tomigrate.autoMigrate=true. So do not merge this PR until arelay-v*release that contains #8611 is tagged and deployed everywhere.After that, either way works:
autoMigrate=true, the first relay from this PR applies 0081 at startup.autoMigrate=false, runbuzz-admin migratewhen convenient. Relays from this PR serve with or without 0081.As with every new migration, an older binary with auto-migration on refuses a database that has 0081 (sqlx
VersionMissing). A rollback past this release needs auto-migration off (docs/database.md).What happens next
#8494 computes the
member/botlabel at read time. The 0032 trigger compares labels with the stored role exactly, so it would refuse those rosters. #8494 adds a startup check that refuses to serve while the trigger is installed. Merge it only after 0081 has run everywhere.Testing
migration_0081_retires_the_roster_trigger: on a database migrated through 0080, the trigger and function are installed. After the remaining migrations, both are gone, and a raw kind:39002 insert that does not match membership is accepted. It fails if 0081 does not drop them.embedded_migrator_contains_consolidated_initial_schemapins 81 migrations and the 0081 statements, and checks that the desired-state schema no longer installs the function.cargo clippy -p buzz-db -p buzz-relay --all-targets -D warningsandcargo fmt --checkpass. The full PostgreSQL lane runs in CI.🤖 Generated with Claude Code