fix(desktop): reuse welcome-team agents across communities instead of minting new keypairs - #6037
fix(desktop): reuse welcome-team agents across communities instead of minting new keypairs#6037mfethe1 wants to merge 5 commits into
Conversation
themiguelamador
left a comment
There was a problem hiding this comment.
Two changes are required before merge:
-
P2 — Canonicalize relay identity before ranking existing agents. The new
normalizeRelayUrlonly trimmed whitespace and trailing slashes. It therefore ranked backend-equivalent pins such asws://localhost:3000vsws://127.0.0.1:3000, orWSS://RELAY.EXAMPLE:443/vswss://relay.example, as different communities. On an install that already has duplicate Welcome records, that can select the wrong identity even though the backend deliberately treats those relay URLs as the same pair key. The fix reuses the existing backend-compatiblecanonicalRelayUrl, retains a stable fallback for malformed legacy pins, and adds regressions for loopback aliases, host case, and default ports. -
Repository attribution policy. The PR commit contains a
Co-Authored-Bytrailer crediting an AI and the PR description contains a “Generated with” footer.AGENTS.mdexplicitly prohibits both on commits and PRs. I could sanitize the commit on the review branch, but GitHub does not grant my account permission to edit this PR's description, so the author must remove that footer when adopting the fix.
The complete fix is available at Complear/buzz:review/pr-6037-fix (7a322e8d1; its rewritten parent also removes the prohibited commit trailer).
Verification on that branch:
- Desktop test suite: 4,960 passed
tsc --noEmit: passedpnpm check: passed (2 pre-existing warnings and 2 pre-existing infos outside this diff)git diff --check: passed
Review follow-up on PR block#6037. relayPinRank compared pins normalized only by trimming whitespace and trailing slashes, so backend-equivalent spellings ranked as different communities: ws://localhost:3000 vs ws://127.0.0.1:3000 wss://RELAY.EXAMPLE:443/ vs wss://relay.example ws://relay.example:80 vs ws://relay.example The backend's pair key already folds host case, loopback aliases and default ports, so a record pinned to one spelling of the active community was ranked "pinned elsewhere". On an install that already carries duplicate Welcome records -- the exact population this PR exists to stop growing -- that can reuse the wrong identity. normalizeRelayUrl now delegates to the existing canonicalRelayUrl (features/agents/managedAgentRuntimeStatus.ts) rather than hand-rolling a second normalization. canonicalRelayUrl returns null for anything that is not a ws/wss URL, so a legacy or malformed pin falls back to a stable lowercased form: two records carrying the same bad pin still rank as one community instead of each becoming its own. An unbound pin ("") stays null and keeps its eligible-everywhere rank. Tests cover loopback aliases, host case, both default ports and a trailing slash, plus a malformed pin, and a guard that canonicalization does not fold two genuinely distinct hosts together. A note on how those tests are written: pickAgentForRelay walks every rank and returns the last candidate standing rather than reporting a starter missing, so a single-agent assertion passes with or without canonicalization. My first draft did exactly that and was vacuous -- it passed against the unfixed source. Each case now lists a decoy pinned elsewhere FIRST, so the assertion only holds if the equivalent spelling is actually recognised as rank 0. Verified: reverting welcomeGuide.ts alone takes the file to 25 pass / 2 fail. Full desktop unit suite 4961/4962; the one failure is the pre-existing timing flake in shared/lib/useDocumentVisible.test.mjs, which this branch does not touch. tsc --noEmit and biome clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
ca4275f to
eb2656a
Compare
…-minting Joining a second community minted a brand new keypair for every Welcome starter. `isAgentScopedToRelay` in welcomeGuide.ts filtered candidates by exact equality against the record's stored `relay_url` pin, so a starter already provisioned for community A matched nothing when community B provisioned, and `provisionWelcomeTeam` fell through to `createManagedAgent` -> `Keys::generate()`. Unbound records were worse: the backend stores `""` when no pin was supplied, and `""` never equals a target relay, so those records matched no community at all. The pin is not supposed to gate anything. `effective_agent_relay_url` (src-tauri/src/relay.rs) deliberately ignores the per-record pin (agents-everywhere, block#2122) — every agent is eligible on every community. The TS filter was drift from that decision. Rank instead of filter: a record pinned to the target community wins, then an unbound record, then a record another community pinned. The lookup now always reuses an existing starter when one exists, so it never falls through to the minting path. welcomeKickoff.ts carried a duplicate pin filter that pre-filtered the agent list. Left as-is it would have started resolving to `null` on the second community once provisioning began reusing pinned records, silently disabling the kickoff closer backstop. It now delegates relay preference to the shared helper. Forward-looking only: no existing record is deleted, merged, or renamed. kind:5 tombstones propagate cross-device while the nsec does not, so merging duplicates would strand those agents on other machines. Already duplicated installs keep every record; they just stop accumulating more. Not fully closed: a starter whose record carries a different `team_id` (e.g. the retired `builtin-team:fizz` built-in in managed_agents/teams.rs) still misses the `WELCOME_TEAM_ID` prefilter and re-mints. Relaxing that prefilter would let provisioning hijack a user's own agent that merely shares a built-in persona, which the existing "starter matching ignores user agents with a Welcome persona" test pins deliberately, so it is left alone. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
Review follow-up on PR block#6037. relayPinRank compared pins normalized only by trimming whitespace and trailing slashes, so backend-equivalent spellings ranked as different communities: ws://localhost:3000 vs ws://127.0.0.1:3000 wss://RELAY.EXAMPLE:443/ vs wss://relay.example ws://relay.example:80 vs ws://relay.example The backend's pair key already folds host case, loopback aliases and default ports, so a record pinned to one spelling of the active community was ranked "pinned elsewhere". On an install that already carries duplicate Welcome records -- the exact population this PR exists to stop growing -- that can reuse the wrong identity. normalizeRelayUrl now delegates to the existing canonicalRelayUrl (features/agents/managedAgentRuntimeStatus.ts) rather than hand-rolling a second normalization. canonicalRelayUrl returns null for anything that is not a ws/wss URL, so a legacy or malformed pin falls back to a stable lowercased form: two records carrying the same bad pin still rank as one community instead of each becoming its own. An unbound pin ("") stays null and keeps its eligible-everywhere rank. Tests cover loopback aliases, host case, both default ports and a trailing slash, plus a malformed pin, and a guard that canonicalization does not fold two genuinely distinct hosts together. A note on how those tests are written: pickAgentForRelay walks every rank and returns the last candidate standing rather than reporting a starter missing, so a single-agent assertion passes with or without canonicalization. My first draft did exactly that and was vacuous -- it passed against the unfixed source. Each case now lists a decoy pinned elsewhere FIRST, so the assertion only holds if the equivalent spelling is actually recognised as rank 0. Verified: reverting welcomeGuide.ts alone takes the file to 25 pass / 2 fail. Full desktop unit suite 4961/4962; the one failure is the pre-existing timing flake in shared/lib/useDocumentVisible.test.mjs, which this branch does not touch. tsc --noEmit and biome clean. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
eb2656a to
8e0666c
Compare
…lay pins Joining a second community on an install that predates the Welcome Team fell through to createManagedAgent and minted a duplicate Fizz keypair: records provisioned under the retired single-member built-in team (block#1718) carry teamId "builtin-team:fizz", which the Welcome starter filter never matched. Pin the exact retired identity (team id + persona + stock name) so it is reused without absorbing renamed or user-created lookalikes, and share the team-scope predicate between the starter pick and the Welcome pubkey listing. Also bring canonicalRelayUrl into rejection parity with buzz-core's normalize_relay_url: credentialed or fragmented relay URLs are rejected outright instead of being silently stripped into a false exact match. E2e coverage joins a second community through the switcher and asserts no additional create_managed_agent calls, and seeds a retired-team Fizz to prove onboarding mints only the two missing teammates. The mock bridge now round-trips team_id so the team-scoped reuse path is exercisable in e2e. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
themiguelamador
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 8e0666c. The follow-up correctly reuses the existing backend-compatible canonicalRelayUrl for relay-pin ranking, with non-vacuous tests covering loopback aliases, host case, default ports, trailing slashes, malformed legacy pins, and genuinely distinct hosts. It preserves the Welcome Team/team-persona eligibility boundary and only turns the relay pin from a filter into an advisory preference, matching the backend agents-everywhere contract. The prohibited attribution metadata from the previously reviewed head is no longer present. Verification: Desktop TypeScript typecheck passed; focused Biome check passed; full Desktop unit suite passed 4,962/4,962; git diff check passed.
…name The retired-team identity check case-folded and trimmed the agent name, so a user-customized record in a demoted copy of the retired team named "FIZZ" or " Fizz " would be absorbed and silently reused as the Welcome lead. Require the exact stock spelling, and add mutation-sensitive case and surrounding-whitespace regressions beside the existing lookalikes. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
|
Hi @themiguelamador — could you approve the pending CI workflow runs on this PR at head a03bea4? Two review rounds have landed since the APPROVED at 8e0666c (exact-name predicate fix + full-diff scope correction), so a fresh look is welcome too. The CI run matters beyond the usual signal: both dev hosts available to us are Windows, where Local exact-head receipts (Windows): pnpm typecheck PASS, pnpm check PASS, full desktop suite 4968/4968 (71 suites), build:e2e PASS + both new onboarding specs PASS isolated, desktop build + web build PASS, git diff --check PASS. Red-before-green receipts for the predicate fix are in the review thread. Thanks! |
Resolves a semantic conflict git could not see. Main renamed the `welcomeKickoff` test fixture `bumble` -> `pollen` (block#5874's neighbourhood), and this branch had added three assertions that still referenced `bumble`. Neither side touched the other's lines, so the merge was textually clean and GitHub reported MERGEABLE -- but the merged file defines `pollen` and reads `bumble`, and the suite dies with `ReferenceError: bumble is not defined` at welcomeKickoff.test.mjs:49. Nothing had ever built this combination: as a fork PR it runs DCO and nothing else, and the branch alone is self-consistent, so the break existed only in the merge result. The three added references now use `pollen`. The `"builtin:bumble"` runtime id on line 34 is unchanged -- that is the harness identifier, not the fixture. Verified in this worktree on x86_64-pc-windows-msvc: - `pnpm test`: the welcomeKickoff ReferenceError is gone. The one remaining failure is `focused polling pauses on blur`, which passes 5/5 in isolation and lives in a file this branch does not touch. - `pnpm check` (biome + file-size ratchet + px-text + pubkey guards): exit 0. Signed-off-by: Michael Feth <michael@jira-flow.com>
The bug
Joining a second community mints a new keypair for a Welcome Team agent that already exists, instead of reusing it.
welcomeGuide.ts'sisAgentScopedToRelaycomparednormalizeRelayUrl(agent.relayUrl)against the target relay by exact string equality. A miss falls through tocreateManagedAgent, which isKeys::generate()incommands/agents.rs. So the same "Bumble" becomes a second Bumble with a different pubkey in community B.Three details make it worse than it looks:
normalizeRelayUrl("")returned"", notnull—"".trim().replace(...)is"", and?? nulldoesn't fire on an empty string. So an unbound record compared"" === "wss://…"→ false and matched no community at all.normalize_relay_url,crates/buzz-core/src/relay.rs). A trim-only compare rankedws://localhost:3000andws://127.0.0.1:3000as two different communities while the backend treated them as one.desktop/src-tauri/src/relay.rs:57-71states it verbatim: "The legacy per-recordrelay_urlpin is deliberately IGNORED (agents-everywhere, feat(desktop+acp): spawn a harness per (agent, community) pair at GUI startup — warm sockets, lazy LLM pool #2122): every agent is eligible on every community." The Rust runtime ignores the pin; the TS onboarding path still enforced it.Observed result on one owner's machine: 27 managed-agent records for 11 agent names.
The fix
1. Pin becomes a preference, not a filter.
isAgentScopedToRelayis replaced byrelayPinRank(welcomeGuide.ts:100) +pickAgentForRelay(:175):The picker walks the ranks and returns the first hit, so it never returns
nullwhile any candidate exists, and it still prefers a correctly-pinned instance.2. Ranking uses the backend's relay identity, not a trim.
normalizeRelayUrl(welcomeGuide.ts:79) now delegates to the existingcanonicalRelayUrl, so loopback aliases, host case, default ports and trailing slashes all collapse the way the backend collapses them. A malformed legacy pin canonicalizes tonulland falls back to a stable lowercased form, so two records carrying the same bad pin still match each other instead of each becoming its own community.3.
canonicalRelayUrlis brought to full parity with buzz-core.managedAgentRuntimeStatus.ts:86-87now rejects URLs carrying credentials or a fragment outright, exactly asnormalize_relay_urldoes (crates/buzz-core/src/relay.rs,rejects_non_relay_and_ambiguous_urls). Silently stripping userinfo would have canonicalizedwss://user@relay.exampleinto a false exact match onwss://relay.example. The raw#check keeps parity for an empty fragment (/#), which the URL API normalizes away but the backend still rejects.4. Upgraded installs adopt the retired built-in Fizz instead of duplicating it. Installs predating the Welcome Team carry a Fizz record pinned to the retired single-member
builtin-team:fizzteam (#1718,desktop/src-tauri/src/managed_agents/teams.rs). That record is the Welcome lead identity, but it fails ateamId === WELCOME_TEAM_IDcheck, so onboarding minted a second Fizz.isWelcomeTeamStarterAgent(welcomeGuide.ts:136) now accepts it.The adoption predicate is deliberately narrow — team id and persona and the exact stock name
"Fizz"(welcomeGuide.ts:123). A pristine retired team is purged on load while a customized one is demoted to user-owned, so a user's own renamed agent can still be sitting in a team with that id; the name pin is what keeps it from being absorbed and silently reconfigured. The match is case- and whitespace-sensitive on purpose, and there is a test for each mutation.5.
welcomeKickoff.tsis load-bearing here, not cleanup. It carried a second, private copy of the pin filter (resolveWelcomeAgentSetForRelay) that pre-filtered the list. Once provisioning starts reusing an A-pinned record, that filter would have returnednullin community B, andagentSet == nullis a hard early return in the closer effect — the kickoff backstop would have silently stopped firing. It now delegates to the shared helper.6. The e2e mock bridge was missing
team_identirely.shared/api/tauri.ts:676mapsteam_id → teamId, bute2eBridge'sRawManagedAgentnever emitted the key, soagent.teamIdwas alwaysnullunder the mock and every team-scoped path was untestable in e2e. Seed, clone, and thecreate_managed_agenthandler now carry it.Scope: forward-looking only
This stops the next duplicate. It does not remove records that already exist, deliberately:
delete_managed_agentpublishes a kind:5 tombstone that propagates cross-device, while the nsec does not — so merging duplicates would delete a working agent on the owner's other machines and leave the surviving name runnable on exactly one. Cleanup of existing duplicates is a separate problem and is not attempted here.Two further exclusions worth naming:
builtin-team:welcome.UpdateManagedAgentInputhas noteamIdfield, so the frontend cannot re-parent without a backend change. Nothing is lost at runtime —BUILT_IN_TEAMSseeds the Welcome Team withinstructions: Noneandeffective_team_instructionsdegrades a missing team toNone— but a team-grouped view will not list the adopted lead under "Welcome Team".welcomeGuide.tshave no production callers and are updated only for consistency:pickWelcomeGuideAgentForRelay,getWelcomeTeamAgentPubkeys,getWelcomeGuideAgentPubkeys(git grepover the whole tree returns only their definitions). The one live call site of the fixed predicate ispickWelcomeTeamStarterAgentForRelay, reached fromprovisionWelcomeTeamandresolveWelcomeAgentSet. Removing the dead exports is out of scope for a fix PR.Testing
+14 unit tests, +2 e2e specs, all red before the corresponding source change:
welcomeGuide.test.mjswelcomeKickoff.test.mjsmanagedAgentRuntimeStatus.test.mjstests/e2e/onboarding.spec.tsCoverage of the new behavior:
"Fizz"in a demoted retired team is not absorbed.resolveWelcomeAgentSethonors relay preference without dropping tonull.create_managed_agentcalls and leaves the pubkey set unchanged; an install seeded with abuiltin-team:fizzFizz mints exactly["Bumble", "Honey"]and keeps the original Fizz pubkey.Two existing tests asserted the old behaviour as correct and were rewritten (
…returns null when Fizz only exists in another community→…reuses Fizz from another community).A note on test rigour: the first draft of the kickoff test passed pre-fix, because the old
resolveWelcomeAgentSetignored a second argument and the assertion was vacuous. It was rewritten to assert that a relay-pinned Fizz beats an other-community Fizz listed first in the array, which does fail on the old code.Local gates at head
a03bea4b8:pnpm typecheckclean,pnpm checkclean (2 pre-existing warnings + 2 pre-existing infos, in files this PR does not touch), full desktop unit suite 4,968/4,968 across 71 suites,pnpm build:e2eplus both new onboarding specs green in isolation, desktop and web builds pass,git diff --checkclean.No Rust in the diff — all 8 changed files are under
desktop/srcanddesktop/tests.welcomeGuide.tsis 494 lines against the 1000-line desktop ratchet;e2eBridge.tsandtests/e2e/are outside the ratchet's roots.CI on this PR has not been able to run — both workflow runs at head sit at
action_requiredpending maintainer approval, so DCO is the only executed check. A maintainer approving the runs would give thedesktop-corejob on ubuntu-latest, which is the supported-environment run of these gates.