fix(desktop): connect managed agents with the configured relay URL, not the canonical identity URL - #4859
fix(desktop): connect managed agents with the configured relay URL, not the canonical identity URL#4859anilkishan wants to merge 6 commits into
Conversation
Chessing234
left a comment
There was a problem hiding this comment.
connecting with the configured relay url while identity/snapshot stay on canonical key.relay_url matches the tenancy vs identity split. any leftover call site that still builds the connect URL from the identity spelling?
|
Good question — swept the desktop crate for
Identity and bookkeeping uses of |
|
🤖 The identity/connection split is the right fix, and I’d like to use this as the canonical PR for the overlapping reports. One blocker remains: the current diff loses the configured connection URL during global-config and ACP-install restarts, causing an initially healthy localhost agent to restart against 127.0.0.1 and become idle again. Please retain the original connection URL in the pair runtime and persisted receipt, use it for every restart path, and add backward-compatible receipt plus restart-target tests. The canonical URL should remain the runtime key and spawn-snapshot input. Please also rebase onto current main and run the full desktop/Tauri checks. Once that is addressed, I’m comfortable approving this and closing the duplicate implementations. it could be worth looking at:
to pick out some of the pieces from those PRs which seem to be a duplicate |
…ot the canonical identity URL Managed agents were spawned with BUZZ_RELAY_URL set to the canonical (pubkey, relay_url) identity URL from normalize_relay_url, which folds loopback spellings to 127.0.0.1. Relay tenancy is host-derived and does NOT fold spellings, so an agent configured against ws://localhost:PORT connected to the ws://127.0.0.1:PORT community instead - discovering 0 channels and sitting idle (block#2444, block#3283, block#3505, block#4147). Pass the configured relay URL through to the child connection at all three spawn call sites, keeping the canonical key URL for runtime identity and for the spawn-config snapshot (the restart-drift check recomputes with key.relay_url, so the stamp must agree or agents restart-loop when the spellings differ). Co-authored-by: anilkishan <10408515+anilkishan@users.noreply.github.com> Signed-off-by: anilkishan <10408515+anilkishan@users.noreply.github.com>
Review follow-up on the same tenancy-vs-identity split: the relay-access probe built its HTTP base from the canonical key spelling (probing a different or unmapped community than the one being configured), and the post-probe start passed the canonical spelling as the child's connection URL. Both now use the requested URL; start_pair re-derives the same canonical key, so identity is unchanged. Co-authored-by: anilkishan <10408515+anilkishan@users.noreply.github.com> Signed-off-by: anilkishan <10408515+anilkishan@users.noreply.github.com>
…start path Config-change and post-install restarts rebuilt their dial targets from the canonical runtime key, so an initially healthy localhost agent restarted against 127.0.0.1 - a different (empty, fail-closed) community on a host-scoped relay - and went idle again. The spawn-path fix earlier in this branch corrected first starts; this closes the restart paths. - Stamp the exact dial URL (the trimmed value written to the child's BUZZ_RELAY_URL) onto ManagedAgentProcess at spawn; every receipt write persists it as `connectRelayUrl`. Old receipts remain valid when the field is absent; every live runtime carries the stamped URL. - Validate that a stored connection URL canonicalizes back to the receipt's own pair key; a foreign or unparseable URL fails validation. - Add managed_agent_restart_targets(): collects each live pair's stamped connection URL before the stop drops the pair from the runtimes map; both backend restart orchestrations (set_global_agent_config, install_acp_runtime) now dial those targets instead of key.relay_url. - status_for echoes the live pair's connection spelling through requestedRelayUrl, so status rows correlate across spellings. Interactive community actions already submit the configured community URL on current main. - Launch restore carries the URL inside the process struct from the Phase-B spawn to the Phase-C receipt write - stamped once, never recomputed between phases. - ManagedAgentProcess Debug masks the stored URL (relay URLs may carry query tokens), pinned by the owning-process Debug sentinel test. The canonical URL remains the runtime key and spawn-snapshot input, unchanged. Co-authored-by: Ahmet Karapinar <ahmet.karapinar@maneva.ai> Co-authored-by: Michael Kennedy <mike.kennedy12@gmail.com> Co-authored-by: Lewis Wang <lewis.wang@plusalsostudios.com.au> Co-authored-by: anilkishan <10408515+anilkishan@users.noreply.github.com> Signed-off-by: anilkishan <10408515+anilkishan@users.noreply.github.com>
5e028af to
8f1ffc0
Compare
|
Done. Thank you for the review and for consolidating the reports here. The follow-up commit implements the blocker as scoped:
Pieces folded in from the overlapping PRs are credited in the commit trailers and the prior-art section below, from #2446, #2944, and #5200 in particular. |
wolfyy970
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 8f1ffc0c65. The restart blocker is closed: canonical URLs remain pair identity and spawn-snapshot input, while the actual dial URL is stamped on the live process and backward-compatible receipt and reused by both automatic restart paths. Foreign receipt URLs fail validation, and old receipts still load.
I reran the focused receipt, multi-pair restart-target, and loopback connection tests at this head: 14 passed. Tauri formatting and git diff --check are clean. Approved.
There was a problem hiding this comment.
[P1] Canonical-key collisions can reuse an agent connected to the wrong community.
start_pair returns any live runtime matching the canonical key without checking its new connect_relay_url. If an agent is running against ws://127.0.0.1:3000 and reconciliation requests ws://localhost:3000, the probe reaches the new tenant, but start_pair returns the old process because both spellings share a key. The status is then overwritten to claim the localhost request succeeded. The agent remains connected to the previous tenant and reconciliation stops retrying.
It'd be good to fail with an explicit connection-target conflict when the canonical key is already occupied by a runtime connected to a different tenant.
|
|
Review round 2 on this PR (P1 + desktop-check ratchet). Fix - connection-target conflict (P1): all three canonical-key reuse sites (start_pair, start_managed_agent_process, and launch restore's Phase-B already-live check) reused any live runtime matching the canonical pair key without comparing its stamped connection URL, so a start request for one loopback spelling could reuse a child connected to the other spelling's tenant and falsely report the requested tenant as started, stopping reconciliation retries. All three sites now call ensure_pair_connection_matches(); the command paths return an explicit connection-target-conflict error and restore records the conflict as a failed outcome in Phase C. The error deliberately omits both URLs (they may carry query tokens and the string lands in last_error and the UI). Covered by pair_reuse_with_matching_spelling_is_allowed, pair_reuse_across_spellings_is_a_connection_target_conflict, and restore_reuses_live_pair_only_for_matching_spelling. Restructure - file-size ratchet (no behavior change): - runtime/connect_url_tests.rs: new home for the connection-URL tests; shared fixtures (receipt, minimal record, pair-runtime placeholder) move to runtime/test_fixtures.rs. - ManagedAgentProcess Debug impl moves from types.rs to runtime/process.rs (cross-platform, beside the receipt validation); connection_relay_url() moves there too, next to its only coupling. - child_rust_log_filter() moves to agent_env.rs; persona_drift_state() moves to runtime/metadata.rs. - Comment tightening in agent_discovery.rs and shared/api/types.ts. All five flagged files are back at or under their ratchet limits; no limit was raised. Co-authored-by: anilkishan <10408515+anilkishan@users.noreply.github.com> Signed-off-by: anilkishan <10408515+anilkishan@users.noreply.github.com>
|
Both items addressed:
Full desktop/Tauri workspace suite (2,538 passed, 0 failed), clippy with warnings denied, fmt, and the full |
wesbillman
left a comment
There was a problem hiding this comment.
P1 — live status still aliases distinct loopback tenants, so the UI can stop the wrong agent pair.
This branch correctly exposes the actual dial spelling as requestedRelayUrl, but the frontend lookup still accepts the canonical identity URL first/finally (desktop/src/features/agents/managedAgentRuntimeStatus.ts:95-112). For a live row dialed to ws://localhost:3000, the row is now:
relayUrl = ws://127.0.0.1:3000(canonical identity)requestedRelayUrl = ws://localhost:3000(actual tenant)
When rendering the distinct configured community ws://127.0.0.1:3000, findManagedAgentRuntime matches that same row because runtime.relayUrl === relayUrl (line 109), even though requestedRelayUrl proves the child is connected to localhost. The card therefore reports the agent as running in the wrong community; its Stop/Restart action uses the shared canonical key and can terminate the localhost child. That defeats the PR's new conflict guard at the user-facing boundary and is especially dangerous because the two hosts are deliberately distinct tenants.
Smallest safe remedy: when requestedRelayUrl is present, make it authoritative for community matching and do not fall back to canonical identity matching; reserve the canonical fallback for legacy/status rows where the requested URL is absent. Add a regression with simultaneous configured localhost and 127.0.0.1 communities proving only the actually dialed card resolves a runtime and an action on the other card cannot target it. If tenant-equivalent spelling variants (case/default port/FQDN dot) must match, use tenancy-authority equivalence—not normalize_relay_url, which intentionally collapses these two loopback tenants.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes at exact head 4f759f915414781d8bcafb857c457f3c31bf532a for two correctness defects in the identity/connection split:
-
P1: live status aliases distinct loopback tenants in the frontend.
findManagedAgentRuntimeacceptsruntime.relayUrl === relayUrland canonical fallback even whenrequestedRelayUrlis present (desktop/src/features/agents/managedAgentRuntimeStatus.ts:95-112). A child dialed tows://localhost:3000therefore appears live on the distinctws://127.0.0.1:3000community because both share the canonical runtime key. The wrong card can issue Stop/Restart against that key and terminate the localhost child. MakerequestedRelayUrlauthoritative when present; use canonical fallback only for legacy rows without it. Add a two-spelling regression at this UI/action boundary. -
P1: the new reuse guard rejects harmless spellings of the same connection target.
ensure_pair_connection_matchescompares trimmed strings byte-for-byte (desktop/src-tauri/src/managed_agents/runtime/process.rs:487-501). DNS host case, an explicit default port, a root slash, and an FQDN root dot are connection-equivalent and resolve to the same relay tenant, but this guard reports aconnection-target conflict. The tenancy contract explicitly folds host case, default ports, and the root dot (crates/buzz-core/src/tenant.rs:100-137); URL parsing also folds the root slash/default port. This can strand reconcile/start after harmless configuration formatting drift. Compare normalized connection targets while preserving tenancy-significant host spelling such aslocalhostversus127.0.0.1; add equality regressions for DNS case/default port/root slash/FQDN dot and retain the loopback-inequality regression.
The core fix is directionally correct: actual dial URLs are now persisted and reused across restart paths while canonical URLs remain identity keys. Current checks are green, but neither semantic boundary above is covered.
…ns in UI Review round 3 (two P1s from automated review). - ensure_pair_connection_matches compared trimmed strings byte-for-byte, so connection-equivalent spellings (host case, explicit default port, root slash, FQDN root dot) read as a connection-target conflict and could strand reconcile/start after harmless config formatting drift. Equivalence is now by comparable connection target, parsed with url::Url: lowercased scheme, parsed-host normalization (ASCII lowercase, single FQDN root dot stripped), a port kept only when it differs from the scheme's own default, a root-slash-folded path, and the query preserved verbatim as its own field. Tenancy-significant differences stay conflicts: localhost, 127.0.0.1, and [::1] are three distinct hosts, and ws vs wss, non-default ports (including the other scheme's default), paths, and query strings stay distinct. Unparsable URLs fail closed to exact comparison. - findManagedAgentRuntime accepted canonical identity matches even when a row carried the actual dial spelling, so with both loopback communities configured the wrong community's card could show the child as running there and its stop/restart could target it through the shared canonical key. requestedRelayUrl is now authoritative when present, matched by a TS mirror of the same connection-target rule; the canonical fallback remains only for legacy rows without it. - Regressions: Rust equivalence set (case, default port, root slash, FQDN dot, query with/without root slash, IPv6 textual forms) plus retained loopback/scheme/port/path conflicts and new wrong-scheme default-port, query-difference, and [::1]-vs-loopback conflicts; TS two-spelling test proving only the actually dialed community resolves the runtime while the other loopback card resolves nothing, plus spelling-equivalence and target-rule tests. Co-authored-by: anilkishan <10408515+anilkishan@users.noreply.github.com> Signed-off-by: anilkishan <10408515+anilkishan@users.noreply.github.com>
|
Both P1s addressed:
Gates on the new head: full desktop/Tauri workspace suite, clippy with warnings denied, fmt, |
connection_target and its TS mirror connectionTargetUrl silently dropped userinfo and fragments while folding, so invalid relay spellings like wss://alice@relay.example and wss://bob@relay.example compared as the same target instead of taking the documented exact-string fallback. The Rust helper also accepted non-ws(s) schemes the TS mirror rejects. Reject credential-bearing, fragment-bearing, and non-ws(s) URLs in both helpers (all rejected by normalize_relay_url), routing them to the exact-comparison fallback, and add regressions on both sides. Reported-by: Miguel Amador (review on block#5993) Signed-off-by: anilkishan <10408515+anilkishan@users.noreply.github.com>
|
Pushed 796c78a: Credit to @themiguelamador, who spotted this gap while reviewing the same helper carried into #5993. Verification: full Tauri suite green, 4,780 desktop JS tests green, typecheck, Biome, fmt, clippy |
Summary
Desktop-managed agents can silently connect to a different relay community than the owner's desktop, discover 0 channels, and sit idle forever - never replying to mentions.
Root cause: two normalizers deliberately disagree about host equivalence, and the spawn path mixes them up.
buzz_core::relay::normalize_relay_url(the identity normalizer) folds all loopback spellings to127.0.0.1to build the stable(pubkey, relay_url)runtime key. Its doc comment explicitly says connection code should retain the configured URL.buzz_core::tenant::normalize_host(the tenancy normalizer) does not collapselocalhostvs127.0.0.1- distinct spellings are distinct communities, and the relay fails closed on unmapped hosts.spawn_agent_childviolates the first normalizer's contract: it sets the child'sBUZZ_RELAY_URLfrom the canonicalruntime_key.relay_urlinstead of the configured URL. So an agent configured againstws://localhost:3000is spawned againstws://127.0.0.1:3000, binds to the wrong (often empty) community, and goes deaf.The fix keeps the two roles separate, per
normalize_relay_url's documented intent:spawn_agent_childnow uses the configured relay URL (trimmed) for the child'sBUZZ_RELAY_URL. All three call sites (start_managed_agent_process,start_pair, launch restore) already have the configured URL in scope and now pass it through.key.relay_url. The restart-drift check recomputes the prospective snapshot withkey.relay_url, so the spawn-time stamp must agree - otherwise agents restart-loop whenever the configured spelling differs from the canonical one.No identity migration: existing
(pubkey, relay_url)runtime keys are unchanged.Follow-up: restart-path persistence
Restart paths that only had the canonical runtime key were still dialing
127.0.0.1after a healthylocalhostspawn. The exact connection URL each child was dialed with is now stamped on the pair runtime at spawn and persisted on the receipt (connectRelayUrl; old receipts remain valid when the field is absent). The two backend restart orchestrations (global-config and ACP-install) now reuse each live pair's stamped connection URL. Interactive community actions already submit the configured community URL on current main, and live status rows now echo the pair's actual connection spelling throughrequestedRelayUrl. Identity, receipts-as-keys, and the spawn-config snapshot stay on the canonical form. Debug output masks the stored connection URL, matching the spawn-snapshot redaction policy, since relay URLs may carry query tokens.Related issues
Fixes #2444, #3033, #3505, #4147.
Does not close:
normalize_relay_urlto fold loopback tolocalhost, which rewrites runtime keys. Leaving it for maintainers; this PR does not compete with it.Testing
agent_connection_preserves_loopback_authoritycovering the connection-URL helper.just desktop-tauri-test(fullcargo test --workspaceindesktop/src-tauri, 2,200+ tests) passes.just desktop-tauri-clippyandcargo fmt --checkclean.Prior art
Several independent PRs diagnosed the same spawn-vs-tenancy split. salman1993 asked to consolidate on this PR and fold useful pieces in. What landed, and what we left with its author:
requestedRelayUrlstatus-row complement so rows correlate across spellings.child_connect_relay_urlseam. Not lifted - currentmainalready restructured that area. Credit for the write-up; no code taken.localhost). Not superseded; not folded in.spawn_relay_rolesframing). The restart-target helper in this follow-up is that idea.If I missed a piece that should be in the diff, say so on this PR - happy to take it.