Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 9c27a0f157
ℹ️ 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".
| target.role, | ||
| &members, | ||
| &target.pubkey, | ||
| &actor_bytes, | ||
| ) |
There was a problem hiding this comment.
When a sole owner submits an ordered batch that first grants ownership to another member and then demotes themselves, every decide_put_user call receives the same pre-batch members snapshot. The self-demotion is therefore reported as LastOwnerDemotion, even though put_members processes targets sequentially and would count the preceding ownership grant. The accepted event only performs the grant and unexpectedly leaves the sender as an owner; validation should evolve membership state in tag order or defer this state-dependent decision to the locked database path.
Useful? React with 👍 / 👎.
| Err(e) => { | ||
| error!(channel = %channel_id, error = %e, "NIP-29 PUT_USER write failed"); | ||
| for target in &allowed { | ||
| report.reject(&target.pubkey, "error: membership write failed".into()); | ||
| } |
There was a problem hiding this comment.
When put_members returns an outer error after the kind:9000 event has already been stored, this catch converts the failure into a report and returns normally, so ingest sends an accepted OK. Replaying the signed event then returns duplicate: without retrying the membership write; for a single-target request the response is even empty because non-batches discard the report. Propagate the failure or record it for durable retry instead of permanently accepting an unapplied membership event.
AGENTS.md reference: AGENTS.md:L219-L226
Useful? React with 👍 / 👎.
RustSec published RUSTSEC-2026-0387 against crypto-bigint: some operations meant to be constant-time could branch on secret values. cargo-deny now fails the Security job on main and every PR. crypto-bigint 0.5.5 came in only through p256 0.13 in buzz-push-gateway. p256 0.14 uses crypto-bigint 0.7.9, which is patched. The only API change the gateway hits is the SEC1 encoder, renamed from ToEncodedPoint::to_encoded_point to ToSec1Point::to_sec1_point. Co-authored-by: ART <ea39c1c8acf57180161ef4ba4ed68abf8589f5291956b909a5aa991fc66664ca@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
A NIP-29 put-user may now name up to 100 people, one `p` tag each.
Each `["p", pubkey, role]` may carry its own role, falling back to the
event's `role` tag. Every person gets the same checks a single add does
(decide_put_user, the agent's channel_add_policy, then put_member under
the channel's membership lock), so one refused person no longer refuses
the others.
An event naming several people reports each result in its OK message:
`members: {"added", "updated", "already_member", "rejected": [{reason,
pubkeys}]}`, listing only non-empty results. It is refused only when it
is malformed or nobody it names may be added. One-person events keep
their empty OK message and their error text.
Re-adding someone at their current role no longer posts a second
member_joined notice; the member-added notification still goes out.
The group-state events are published once per event, not once per
person.
Co-authored-by: claude00 <fb8b76a01257932b8ca35060862c29bd09b2137638573731c2d5074fc02c5385@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
A put-user naming many people made one transaction, one membership lock and five or six round trips per person, plus a policy query and a channel-type read per person. buzz-db gains put_members: one transaction and lock, one read of every involved role, the owner count only when a demotion needs it, and one UNNEST upsert with RETURNING. People are decided in tag order, each seeing the roles earlier ones were given (including the actor's own) and the owners they add or remove, so the answers match one write per person. put_member is put_members with one person, so the admission and last-owner rules keep one implementation; it replaces put_member_with_prior_role. Validation reads every third-party target's channel_add_policy in one query, and apply reads the channel type once for all member-added notifications. Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Signed-off-by: Bradley Axen <baxen@squareup.com>
9c27a0f to
83867ba
Compare
## Summary RustSec published [RUSTSEC-2026-0387](https://rustsec.org/advisories/RUSTSEC-2026-0387) against `crypto-bigint`. Some of its operations meant to be constant-time could compile to code that branches on secret values. Since the advisory came out, `cargo-deny check` reports advisories as failed. That breaks `Security Domain / Security` on `main` and on every PR that runs the Rust security job (for example #8575 and #8579). `crypto-bigint` 0.5.5 came in only through `p256` 0.13 → `elliptic-curve` 0.13 in `buzz-push-gateway`. The advisory has no 0.5.x fix; the patched line is 0.7.6+. This PR moves `buzz-push-gateway` to `p256` 0.14, which uses `crypto-bigint` 0.7.9. | Advisory | Crate | Change | |---|---|---| | [RUSTSEC-2026-0387](https://rustsec.org/advisories/RUSTSEC-2026-0387) | `crypto-bigint` | 0.5.5 → 0.7.9 (via `p256` 0.13.2 → 0.14.0) | **Code changes:** `p256` 0.14 renamed the SEC1 encoder from `ToEncodedPoint::to_encoded_point` to `ToSec1Point::to_sec1_point`. The gateway calls it in two places, both to re-encode an App Attest public key as uncompressed SEC1 bytes. The tests call it in five more. Nothing else in the gateway's API use changed. **Lockfile:** only the `p256` dependency chain changes: `ecdsa`, `elliptic-curve`, `ff`, `group`, `primeorder`, `rfc6979` and `sec1` move up a minor version. `primefield` and `wnaf` are added. The old `der`/`spki`/`pkcs8`/`pem-rfc7468`/`const-oid` 0.x versions that only this chain used are removed. `desktop/src-tauri/Cargo.lock` and the `buzz-feature-flags` fixture lockfile don't contain `crypto-bigint`, so they're unchanged. **Exposure, for context:** in production the gateway uses `p256` only to verify App Attest signatures against public keys. Signing with a private key happens only in tests. So the timing issue didn't realistically apply here, but the bump clears the advisory instead of ignoring it. ### Related issue N/A. I searched open and closed PRs for `RUSTSEC-2026-0387` and `crypto-bigint` and found none. ### Testing - `cargo-deny check`: `advisories ok, bans ok, licenses ok, sources ok`. On `origin/main` the Security job fails on this advisory. - `cargo test -p buzz-push-gateway --all-features`: 82 passed, 18 ignored (the same tests are `#[ignore]` on `main`). - `cargo clippy -p buzz-push-gateway --all-targets --all-features -- -D warnings` is clean. - `cargo check --workspace --all-targets` passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Bradley Axen <baxen@squareup.com> Co-authored-by: ART <ea39c1c8acf57180161ef4ba4ed68abf8589f5291956b909a5aa991fc66664ca@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Reviewed
The five existing batch ingest tests passed; both temporary regression probes reproduced these failures. |
…eaves Validation checked every person against the roster as it was before the event, so a sole owner who named an heir and then stepped down was refused as the last owner, though `put_members` would apply both in order. Apply each allowed person to the validator's roster before deciding the next, and read every third party's add policy up front so a policy-refused heir is never counted. No extra database reads. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Luke Tornquist <tornquist@squareup.com>
GitHub Actions
Error
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Luke Tornquist <tornquist@squareup.com>
github-actions
Bot
added
the
codex-security-review-current
🔐 Codex Security Review
|
nostr's EventBuilder drops a p tag for the signing key unless allow_self_tagging is set, so the ordering test's step-down p never reached the relay. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Luke Tornquist <tornquist@squareup.com>
github-actions
Bot
removed
the
codex-security-review-current
GitHub Actions
In progress
What
A NIP-29 kind:9000 (put-user) can now name up to 100 people, one
ptag each. A client sharing a channel with many people can do it in one signed write instead of one event per person.["p", pubkey, role]. When the third element is absent or empty, the event's["role", r]tag applies.decide_put_user, then the agent'schannel_add_policy, then the admission rules again under the channel's membership lock. One refused person no longer refuses the others.phex, unknown role, more than 100 people, missingh, archived channel), or if nobody it names may be added. With one person the message keeps today'sinvalid: <reason>. With several it isinvalid: members: {…}.member_joinedtimeline notice. The member-added notification (kind:44100) still goes out, so an agent that missed its first one is still re-signaled.Cost of a batch
The database work for one event does not grow with the number of people:
buzz-dbgainsput_members: one transaction, one membership lock, one channel read, one read of every involved role, the owner count only if a demotion needs it, and oneUNNESTupsert withRETURNING.put_memberis nowput_memberswith one person, so there is one implementation of the admission and last-owner rules, and a single add makes two fewer round trips than before.channel_add_policywith one= ANY($2)query (get_agent_channel_policies) instead of one per person.What still scales with people is what each person must receive: their
member_joinedtimeline notice and their kind:44100 notification, one stored event each.Why
This is for Solo chat sharing in buzz-app. block/buzz-app#1605 routes every share through one helper,
shareChatsWithChannel, which today sends one kind:9000 per missing member. A small follow-up there switches it to one batch, then re-reads the roster and adds anyone still missing one at a time. That keeps it correct on relays without this change, which accept the event but ignore everypafter the first.The batch stays a signed NIP-29 event rather than a new HTTP endpoint, per VISION.md and AGENTS.md ("Prefer Nostr events over new HTTP endpoints").
Wire changes for existing senders
ptag now refuses the event. Before, malformedptags were skipped and the first valid one was used.ptags are now targets. Before, they were ignored.pelement that isn't a role, such as a NIP-01 relay hint, refuses the event withinvalid role: …. No sender in this repo does that:build_add_membersends one barep.Tests
buzz-db (
channel_membersPostgres tests): newput_members_decides_each_person_in_order: the sole owner can't step down before an heir is named in the same call, but the heir is still added; with the heir counted the owner can step down, and the admin grant after it is then refused; a no-op re-add reports its prior role; a repeated pubkey refuses the call. The otherchannel_membersPostgres tests pass, including the announce-admission and concurrentjoin_roletests that exerciseput_member.buzz-relay (
put_user_postgres_tests.rs), 5 tests:nobodypolicy rejected, a per-pbotrole, a duplicatep, a single roster publishupdatedfor a role change, with the eventroletag as fallbackpptagRelated relay Postgres groups (
group_state,create_with_members,edit_metadata,membership) pass.Live relay:
e2e_relayagainst a local relay at 9c27a0f: 58 of 61 passed, includingtest_nip29_batch_put_user_wire(added to the CI Relay E2E list) and every NIP-29 put-user, announce, guest and departure test in that list. The 3 failures don't touch this change: the two invite tests assume the relay's seeded host, and this relay ran on:3030.test_unarchive_emits_member_added_notificationtimed out once and passed when re-run, as it did on the first commit.Local caveats: the local
channel_membersroster-fence test and the deletion tests fail on a shared local database because other tests leave its fence trigger disabled, so the PostgreSQL Tests job in CI is the authority for the full suite. The pre-pushrust-testshook passed.desktop-tauri-checksfailed onchunks_exactlints in desktop files this PR doesn't touch, so the push used--no-verify.cargo clippy --all-targets(buzz-db, buzz-relay) andcargo fmt --checkare clean.