Repository navigation
feat(relay): learn system channel membership changes live - #1290
Conversation
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility/user-flow) reviews.
No actionable findings. Source assessment: 9/10 for minimalness, elegance and correctness. The added notice kinds reuse the existing self-scoped subscription and verified membership-confirmation owner. Grants still require strong exact discovery; removal uses the complete roster refresh, including system channels. Existing cancellation and retry paths remain in place, and discovery keeps system channels out of conversation lists.
Reviewed head: 2ab3c5588f950bd135ffa938b6310628c041e778
Base: a5b3391
PR merge-base: 326fedb
Scope: all five files in the feature diff, relevant callers and lifecycle paths, and the public description/commit material (no attached images). The base has diverged; this is not validation of a merged tree.
Validation limits: source-only; no tests, builds, app or relay execution. One hosted-check snapshot showed 30 successful checks, including CI required, and two skipped checks (Windows native validation and browser flake check). The container test is opt-in and not exercised by ordinary CI; the reported end-to-end results depend on relay block/buzz#8406 and app #1288 and were not independently reproduced. Native/live acceptance, focus/accessibility behavior and performance remain unverified. This COMMENT review is not merge approval.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/user-flow) reviews.
No actionable findings in this follow-up. Source assessment: 9/10 for minimalness, elegance and correctness. Compared with previously reviewed 2ab3c5588f950bd135ffa938b6310628c041e778, production changes are comments only. The parameterized named-channel test now exercises both 44100 and 24100 through the existing signed helper, retaining its exact-read, roster-state, interests and cleanup assertions. Confirmation, removal, cancellation and retry behavior remain unchanged; no new focus/keyboard surface is introduced.
Reviewed head: 5260e116debd96fb8729c29561e26dd21810949f
Base: a5b3391
PR merge-base: 326fedb
Scope: the three-file follow-up and relevant confirmation paths, under the repository’s later-review rule; public description and commit material inspected, with no attachments or actionable disclosure findings. This does not validate a merged tree.
Validation limits: source-only; no tests, builds, app or relay execution. One current-head hosted snapshot showed 3 successful checks and 26 queued checks, so application CI was not established. Container-test results, relay emission behavior, native/live acceptance and performance were not independently reproduced. The integration check still depends on relay block/buzz#8406 and app #1288 and is opt-in. This COMMENT review is not merge approval.
5260e11 to
0389905
Compare
loganj
changed the title
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility/user-flow) reviews.
No actionable findings. Source assessment: 9/10 for minimalness, elegance and correctness. The revised membership subscription requests system-tagged 44100/44101 notices on its existing wire, retaining the self-scoped filter, replay bounds, admission and teardown. Relay-authored notices remain hints: additions use exact confirmation and removals use the complete strong roster refresh, including system rosters. Discovery keeps these channels readable without listing them. Existing failure, retry and cancellation ownership is preserved; no new focus surface is introduced.
Reviewed head: 03899051d234f6e519d1e9a76ef9467b342836a7
Base: 2655a01
PR merge-base: 2488d60
Scope: the four-file feature diff, comparison with previously reviewed 5260e116debd96fb8729c29561e26dd21810949f, relevant callers and lifecycle paths, and public description/commit material (no attached images). The base has diverged; this does not validate a merged tree.
Validation limits: source-only; no tests, builds, app or relay execution. One hosted-check snapshot showed 28 successful checks, one browser shard still running, and two skipped checks; overall CI completion was not established. The opt-in container test requires relay block/buzz#8406 and was not independently run. Its removal leg opens the channel before removal, so it does not isolate membership-notice delivery from channel revocation traffic. The paired relay’s documented filter contract was checked, but live interoperability, native acceptance, performance and rendered accessibility remain unverified. This COMMENT review is not merge approval.
🤖
## Summary
- When someone is added to or removed from a system channel, the relay
now sends them the usual "you were added / removed" notice. Before, it
sent nothing, so a client could learn about a new system channel only by
rereading every channel.
- **A client sees a notice only if it knows the channel's type.** Every
membership notice now carries its channel type. Clients that do not send
`#t` (every client today) see only the types that existed before system
channels, so they see no change. A type that Buzz adds later is hidden
from them by default.
- The notices are stored like every other membership notice, so a client
that was offline can catch up with `since` instead of rereading all its
channels.
## Details
- Every kind:44100 (added) and kind:44101 (removed) notice now carries
`["t", <channel_type>]`, for example `["t","stream"]` or
`["t","system"]`.
- **One place reads the channel type:** `emit_membership_notification`
in `side_effects.rs`, where the relay signs the notice. A channel's type
is set when it is created and no code path changes it, so the tag stays
correct. If the channel row is missing, the relay sends no notice rather
than an untagged one.
- **The rule (NIP-CL):**
- a filter with `#t` matches a notice only when the notice's type is one
of its `#t` values;
- a filter without `#t` matches only the **legacy types**: `stream`,
`forum`, `dm`, `workflow`. This list is frozen
(`LEGACY_NOTICE_CHANNEL_TYPES`), so a new type never joins it;
- a notice without a `t` tag was stored before this change. It is always
a legacy type, so the relay treats it as one, and replay across the
upgrade stays complete.
- **Fails closed.** The rule lives in two shared places, and both
default to the legacy types. A read path that is not updated behaves as
it does today:
- `filters_match` / `notice_visible` (buzz-core). Live fan-out, the REQ
history post-filter, COUNT, the HTTP bridge and push all match through
it.
- `EventQuery::notice_channel_types` (buzz-db). Its SQL keeps only
visible notices before `LIMIT`, so page limits and COUNT stay exact.
`None` means legacy types. Only `filter_to_query_params` sets it, from
the filter's `#t`.
- A client that knows every current type sends one filter:
`{"kinds":[44100,44101],"#p":[<me>],"#t":["stream","forum","dm","workflow","system"]}`.
NIP-CL and NOSTR.md describe this.
- **Only the target receives a notice.** A notice names the target, the
channel and who made the change, and the relay signs it, so knowing its
id is not permission to read it. 44100/44101 are now in
`RESULT_GATED_KINDS`, the one list used by the `ids` exemption check,
the per-result reader check (WS and HTTP history, push), live fan-out
(same pod and Redis) and COUNT. This also closes the same id lookup for
ordinary channels' notices.
- No migration: 44100/44101 are already excluded from search indexing.
## Testing
- `buzz-core`: a table test covers untagged, `stream`, `system` and a
future type against filters without `#t`, with `#t` for one type, and
with `#t` for every type. Other kinds keep the plain `#t` match.
- Postgres tests (`system_channel_postgres_tests.rs`): live fan-out, REQ
history, COUNT and the HTTP bridge, for a filter without `#t`, a
`#t:["system"]` filter and an all-types filter; an untagged legacy
notice; a non-target with an `ids` filter on each path; and a raw
`EventQuery` that does not set the field.
- I broke each part on purpose and the tests failed: no type tag on the
notice, no rule in `filters_match`, no SQL rule, and no untagged clause
in the SQL.
- End to end: I built a relay container from f24f8e7 and ran
buzz-app's `container-relay.e2e.test.ts` (block/buzz-app#1290) against
it. The live system channel test passed 5 of 5 runs.
Paired client change: block/buzz-app#1290. It works unchanged against
this relay.
---------
Signed-off-by: Logan Johnson <[email protected]>
Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
The relay now sends a system channel's member-added and member-removed notices (kind:44100/44101) with ["t","system"], and returns them only to a filter that asks with #t:["system"] (NIP-CL, block/buzz#8406). The membership route now sends that filter next to the usual one, so a running session learns at once that it joined or left a system channel. The existing hint handling confirms the channel by reading it, as it does for every notice. Against a relay without #8406 the extra filter matches nothing. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <[email protected]>
0389905 to
baf2b61
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility/user-flow) reviews.
No actionable findings in this follow-up. Source assessment: 9/10 for minimalness, elegance and correctness. Compared with previously reviewed 03899051d234f6e519d1e9a76ef9467b342836a7, the feature patch is rebased with a narrow adaptation preserving the current base’s observer filter. The system-membership filter remains self-scoped on the existing wire; strong exact confirmation, full-roster removal reconciliation, retry and cleanup remain with their existing owners. No new UI or focus path is introduced.
Reviewed head: baf2b61
Base: 4ad00a8
Scope: four-file feature diff, rebase comparison and relevant integration paths under the repository’s later-review rule. Public description and commit material inspected; no attached images or actionable disclosure findings.
Validation limits: source-only; no tests, builds, app/relay execution or CI verification. The extra filter retains its own 500-event replay limit; live performance was not measured. The opt-in container test requires relay block/buzz#8406 and was not run. Its removal leg opens the channel first, so it does not isolate membership notices from channel revocation traffic. Live interoperability, native acceptance and rendered accessibility remain unverified. This COMMENT review is not merge approval.
🤖
Summary
Details
["t","system"]. It returns them only to a filter that asks for them with#t:["system"], so older apps never see them.{"kinds":[44100,44101],"#p":[<me>],"#t":["system"]}.container-relay.e2e.test.tsadds a check against a real relay: a running session gains and loses a system channel without a refresh. It needs a relay with #8406.