Skip to content

fix(relay): classify self-joining agents as bots, not members - #6562

Open
rmichelena wants to merge 2 commits into
block:mainfrom
rmichelena:bumble/join-role-agent-discriminator
Open

rmichelena wants to merge 2 commits into
block:mainfrom
rmichelena:bumble/join-role-agent-discriminator

Conversation

@rmichelena

Copy link
Copy Markdown

Fixes #6561.

handle_join_request hard-coded MemberRole::Member. kind:9021 carries no role tag and buzz channels join exposes no --role, so a self-joining agent could not ask for anything else — and clients read the channel role as the "is this an agent" signal, so it vanished from @mention autocomplete. Repair needs role: bot on a kind:9000, which the relay restricts to owners/admins, and the Desktop role menu offers only admin/member/guest — so there is no in-app way back.

Why Member is the wrong default here

MemberRole's own documentation is the argument:

// crates/buzz-core/src/channel.rs
/// The hierarchy for permission checks is: Owner > Admin > Member > Guest.
/// Bot is a **separate designation** — it is not part of the linear hierarchy.

Member is not a conservative privilege choice for an agent — it is the wrong category, and it is the category clients consume.

What this does

Uses the agent discriminator the codebase already defines and relies on:

// crates/buzz-db/src/usage.rs
/// Agent discriminator: `agent_owner_pubkey IS NOT NULL`.

read through get_agent_channel_policy, which this file already calls in handle_put_user's policy check. No new classification rule is introduced.

What this deliberately does not do

handle_put_user is untouched. A caller that states admin / member / guest for a human still gets exactly that. The relay decides only on the one path where no one can express intent — a human self-joining still lands Member, unchanged.

A failed classification lookup logs a warning and falls back to Member, preserving today's behaviour rather than failing the join.

Tests

The decision is extracted into self_join_role so it is unit-testable without a database:

  • agent (agent_owner_pubkey present) → Bot
  • human (user row, no agent owner) → Member
  • no user row → Member, i.e. unchanged when nothing is known

cargo clippy -p buzz-relay --all-targets -- -D warnings is clean.

Closed relays

agent_owner_pubkey is the field that stays NULL when require_relay_membership = true#5581. Where that is unresolved the discriminator reports every agent as human and this change is a no-op, so #5581 is a prerequisite for closed deployments rather than an unrelated fix. Flagging it so this is not mistaken for a complete fix on such a relay.

…6561)

kind:9021 carries no role tag and buzz channels join exposes no --role, so a
self-joining agent could not ask for anything and handle_join_request recorded
it as MemberRole::Member. Clients read the channel role as the "is this an
agent" signal, so a self-joined agent disappears from @mention autocomplete —
and the repair needs role:bot on a kind:9000, which the relay restricts to
owners/admins, so there is no in-app way back.

MemberRole's own docs say Bot "is not part of the linear hierarchy" but a
separate designation, so Member is not a conservative choice here — it is the
wrong category, and it is the category clients read.

Uses the agent discriminator the codebase already relies on,
agent_owner_pubkey IS NOT NULL (see buzz_db::usage::user_counts), via the
get_agent_channel_policy accessor already called elsewhere in this file. No new
policy is introduced.

Scoped deliberately to the self-join path: handle_put_user is untouched, so a
caller that states admin/member/guest for a human still gets exactly that. The
relay decides only where no one else can express intent.

A failed classification lookup logs and falls back to Member, preserving the
previous behaviour rather than failing the join.

The decision is extracted into self_join_role so it is unit-testable without a
database; three tests cover agent, human, and no-user-row.

Note for closed relays: agent_owner_pubkey is the field that stays NULL when
require_relay_membership is true (block#5581), so this is a no-op there until that
lands.

Signed-off-by: Roberto Michelena <77797875+rmichelena@users.noreply.github.com>

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reasoning in the comments is unusually good — especially spelling out why Bot can't shadow a human role and why handle_put_user is deliberately untouched. extracting self_join_role for the three unit tests is the right shape.

the gap i'd want addressed is the agents that already self-joined. your own comment notes that changing an active member's role is owner/admin-only, so every agent that hit this before the fix stays Member and stays unmentionable, with no in-app way to repair it — which is the actual complaint in #6561. this makes the bug stop happening; it doesn't make the existing ones work.

is a backfill in scope? something like "set role=Bot where the member row is Member and agent_owner_pubkey IS NOT NULL" is the same discriminator you're already using, so it should be expressible as a migration. if it's deliberately out of scope that's fine, but worth saying so, otherwise the issue looks fixed while affected users still can't mention their agent.

@rmichelena

Copy link
Copy Markdown
Author

Thanks — and yes, you're right that this is forward-only. It stops the bug happening; it doesn't repair anyone already in it, and #6561's complaint is about the latter.

A backfill is in scope as far as I'm concerned. I'd want to narrow the predicate before writing one, though, because the discriminator on its own is too broad.

Why role = 'member' AND agent_owner_pubkey IS NOT NULL overshoots

That set includes agents an owner deliberately added as memberadd-member takes an explicit --role, and handle_put_user preserves an existing role precisely because roles can be intentional. A backfill keyed only on "is an agent" silently overrides those decisions, which is the opposite of the scoping this PR is careful about (handle_put_user untouched, relay decides only where nobody can express intent).

There's a column that separates them

handle_join_request is the only production path that creates a member with no inviter:

path invited_by
handle_join_request (self-join) None
handle_put_user (kind:9000 / add-member) Some(actor)
workflow sink agent attach Some(author)
audio channel auto-add Some(channel.created_by)

So the affected rows are addressable exactly:

UPDATE channel_members cm
   SET role = 'bot'
  FROM users u
 WHERE cm.role = 'member'
   AND cm.invited_by IS NULL          -- self-joined, nobody chose this role
   AND cm.removed_at IS NULL
   AND u.community_id = cm.community_id
   AND u.pubkey = cm.pubkey
   AND u.agent_owner_pubkey IS NOT NULL;

invited_by IS NULL is what makes it a repair rather than an override: it targets rows where the role was assigned by the code this PR is fixing, and leaves every row where somebody actually chose member.

Scope note on that claim: I checked the add_member call sites in buzz-relay and buzz-db; the non-test ones are the four above. If there's a path I've missed that legitimately leaves invited_by NULL, the predicate needs revisiting — I'd rather you tell me than have me assert it too confidently.

One caveat that limits how much it fixes

agent_owner_pubkey is NULL on relays running require_relay_membership = true, which is the gap #5581 addresses. On those deployments both this fix and the backfill are no-ops until that lands — worth stating in the migration comment so nobody reads a clean run as "no affected rows existed".

Happy to add the migration to this PR, or as a follow-up if you'd rather keep the code change reviewable on its own. Say which and I'll do it.

Only one conflict, and it was a both-sides-add inside the `side_effects.rs`
test module: upstream added `nip43_reconciliation_compatibility_alias_is_preserved`
at the same offset as this branch's three `self_join_role` cases. Both kept.

The fix itself is untouched and still needed — `handle_join_request` on
`main` (4cd82f5) still hard-codes `MemberRole::Member`, and
`get_agent_channel_policy` still returns `Option<(String, Option<Vec<u8>>)>`,
so the discriminator this branch relies on is unchanged.

Gates on the merge result: `cargo fmt --check -p buzz-relay` exit 0,
`cargo clippy -p buzz-relay --all-targets -- -D warnings` exit 0,
`cargo test -p buzz-relay --lib self_join_role` 3/3.

Signed-off-by: Roberto Michelena <77797875+rmichelena@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 4cd82f513214aad11c2b742ce7cc7c681e8e32a0...50479d717284b3667deee44ffae1f642faf375cd.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 50479d717284b3667deee44ffae1f642faf375cd to authorize a new review.
Any previous review applies only to its recorded range.

@rmichelena

Copy link
Copy Markdown
Author

Refreshed onto main (4cd82f513) — 50479d717, MERGEABLE again.

The only conflict was a both-sides-add in the side_effects.rs test module, where nip43_reconciliation_compatibility_alias_is_preserved landed at the same offset as this branch's three self_join_role cases. Both are kept; the net diff against main is still the same single file, +79/−7.

Re-verified that the bug is still live, rather than assuming it: handle_join_request on main still calls add_member(..., MemberRole::Member, None) unconditionally, and get_agent_channel_policy still returns Option<(String, Option<Vec<u8>>)>, so the agent_owner_pubkey IS NOT NULL discriminator this PR reuses is unchanged.

Gates on the merge result:

Gate Result
cargo fmt --check -p buzz-relay exit 0
cargo clippy -p buzz-relay --all-targets -- -D warnings exit 0
cargo test -p buzz-relay --lib self_join_role 3 passed, 0 failed

One process note, since it is invisible from the PR page: no CI has ever run on this branch. Every workflow run sits at action_required awaiting first-time-contributor approval, and the ones from August have now started expiring at the 30-day mark — GitHub concludes them as failure with zero jobs executed, which renders as a red ❌ that has nothing to do with the code. If a maintainer can hit Approve and run workflows, this PR would get its first real signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Self-join (kind:9021) hard-codes role=member, making a self-joined agent permanently unmentionable

2 participants