feat(desktop): route remote bot actions by connection - #90006
Conversation
Carry immutable connection and profile ownership through Bot Mode actions, session hydration, transcript loading, new chats, and profile deletion. Keep same-named local and remote profiles isolated, preserve the full context menu, and migrate Bot metadata to connection-qualified storage. Add adversarial routing, alias, migration, cache, and compatibility coverage.
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 9b7ab9d65aeda7c535e0a91e5a1c0d951cb48180 against current main 13ce0c5c675e843af70d19c9e5144249cd51c8d1. There were no prior review submissions or PR discussion on this head.
The architectural direction is right: Bot Mode should carry an immutable (connectionId, profile, targetProfile) owner through reads and mutations instead of borrowing whichever gateway happens to be foregrounded. The source-qualified selection/cache keys, captured session owner route, route-aware transcript backfill, fail-closed requestForBot, and source-bound profile/session/routine operations are all aligned with the multi-source architecture that landed in #89719 and the active-profile correction in #89902.
I would not merge this head yet. I found two class-level blockers and one narrower argument-rewrite bug.
1. targetProfile is preserved on the route, but several Bot metadata/canonical-chat paths still interpret backend responses with the Desktop-facing profile name
connection-registry.ts explicitly defines targetProfile as the backend profile when a registry route maps the Desktop profile name, and this PR correctly proves scalar request routing with shapes like:
{ connectionId: 'remote-a', profile: 'worker', targetProfile: 'backend-worker' }
But the Bot consumer does not consistently cross that alias boundary:
prepareBotSource()callsrequestForBot(bot, 'profiles.list', {}), then selectsprofile.name === bot.name.ensureBotMetadata()does the same.openBotCanonicalChat()sendspreferred_session_ids: { [name]: pinned };scopedBotParams()rewrites top-levelprofile/name/clone_from, but not profile-keyed nested maps.
For worker -> backend-worker, the request is delivered to the correct connection, but the returned profile row is named backend-worker, so both metadata readers miss it. The exact pin lookup is likewise keyed as worker instead of backend-worker. That can drop the durable Bot ui_meta/canonical-chat pointer, make Edit/Groups hydrate an empty view, and drive canonical-chat recovery toward a new session even though the existing backend-owned Bot Chat is healthy.
The remote-DM code already uses route.targetProfile || profile correctly, so this is not a product-model ambiguity; it is an incomplete translation boundary.
Required fix: establish one logical-profile ↔ backend-profile translation seam and use the backend identity when matching returned profile rows and building profile-keyed payloads. Please add executable coverage for a non-identity alias through (a) Bot metadata hydration/Edit, (b) canonical pin reuse via preferred_session_ids, and (c) context-menu mutation—not only the scalar remote-DM/request cases.
2. The v2 Bot metadata transaction can delete the last known-good committed v2 state after migration
commitBotMetaV2() currently performs:
remove(commit marker)
set(bot-meta-v2, next snapshot)
set(commit marker, true)
and on any error removes both the marker and bot-meta-v2.
That is safe enough during the initial v1 -> v2 migration, because v1 still represents the current rollback state. It is not failure-atomic for later normal edits after v2 has become authoritative. v1 is intentionally retained as one-version rollback data and is no longer advanced with every source-scoped Bot edit.
Concrete failure shape:
- Migration succeeds; committed v2 contains state B while v1 is older state A.
- More remote/local scoped edits land in v2 (state C).
- A later
saveBotMeta()attempts state D; the new v2 write or final marker write fails (disk-full/storage failure). - The catch path deletes the v2 snapshot and marker.
- On restart,
migrateBotMeta()correctly refuses markerless v2 and falls back to stale v1 state A.
So one failed successor write can erase the previous valid v2 generation and roll back unrelated Bot metadata changes across the whole plugin, rather than merely rejecting the failed edit. The existing failure tests prove initial-migration rollback, but not preservation of a previously committed v2 generation.
Required fix: make v2 updates copy-on-write/generation-based (or otherwise restore the previous committed v2 snapshot) so the last known-good generation remains authoritative until a complete successor is committed. Add a regression that starts from marker=true + committed v2 B, fails the next commit, reloads, and proves B survives unchanged.
3. cli.exec alias rewriting is value-global instead of position/semantic-aware
scopedBotParams() maps every argv element exactly equal to the logical profile name to targetProfile:
argv: params.argv.map(arg => arg === logical ? target : arg)That rewrites user payload as well as the profile operand. Example:
profile describe worker --text worker
becomes:
profile describe backend-worker --text backend-worker
so a description whose literal text equals the logical Bot name is silently changed. Please rewrite only the command positions that actually carry profile identity (or avoid the CLI compatibility path for profile mutations where a typed RPC exists), and pin that with a regression.
Interlocks / provenance
- #89719, salvaging #88325 by @dokterdok, is the foundation: explicit
(connection, profile)Desktop routing and retained background gateways. This PR is a complementary Bot Mode consumer/closure slice and should preserve that provenance rather than being described as re-originating multi-source routing. - #89902 by @teknium1 fixed the active-profile session-resolution half of blank Bot threads. This PR's captured owner route is the stronger generalization for cross-connection opens; retain the #89902 regression while composing it.
- #89916 by @frizikk remains the adjacent “other side of the shape”: filesystem/Git artifact facades can still discard
connectionIdand route a remote-owned resource to local. #90006 does not duplicate or close that bug; together they establish the same principle: once a resource/session/Bot has a qualified owner, downstream facades must not reduce it to foreground profile state. - #89252 / #89860 are the gateway-side analogue: transport owner, runtime profile, and persistence owner are separate dimensions.
profileandtargetProfileshould likewise remain distinct here instead of becoming incidentally interchangeable.
CI / re-review gate
The author reports substantial focused validation (312 Bot Mode Node tests, 280 renderer tests, 58 registry tests, typechecks), but the connector returns no PR-triggered workflow runs for this exact head yet, so there is no hosted exact-head matrix to treat as a receipt.
Re-review gate for me: close the alias-response/nested-map hole, preserve the previous committed v2 generation across a failed successor write, make cli.exec remapping semantic, then run the exact-head Desktop/renderer/electron matrix. I did not find a reason to reject the core immutable-owner-route architecture; the blockers are where that route is translated or persisted.
Preserve current upstream Desktop and Bot Mode behavior while carrying the connection-bound remote Bot routing implementation forward without rewriting the signed feature commit.
|
Thanks for the detailed review. I preserved the immutable-owner architecture and addressed each requested blocker on signed merge head
The merge also closes adjacent connection-bound edge cases found during fail-closed review: focused-owner ambiguity, local-recipient DMs while remote-active, same-name post-delete navigation, source-scoped legacy-delete rejection, and registered local/SSH deletion teardown. Exact-head verification against current main
The branch was updated by a signed merge commit; the original signed feature commit was not rewritten. The PR is now conflict-free. |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact current head 723dbda0414ce2d8fdc86283b76302537d91b724 against current main 13ce0c5c675e843af70d19c9e5144249cd51c8d1.
Conflict status: verified clear
GitHub reports this head as mergeable: true. The current base is the merge base, and the branch is 2 commits ahead / 0 behind. The merge commit is signature-verified and has the prior reviewed feature head plus current main as its two parents. There are no unresolved inline review threads.
Original code blockers: closed
- Alias translation: one
backendTargetProfileseam now drives backend response matching and nested profile-keyed requests.prepareBotSource(),ensureBotMetadata(), andopenBotCanonicalChat()use the backend identity, includingpreferred_session_ids. Executable regressions cover non-identity Edit hydration/save, context-menu/group mutation routing, and canonical-pin reuse. - v2 metadata failure atomicity:
commitBotMetaV2()captures the previously committed snapshot/marker and restores them when a successor commit fails. The regression starts from committed B, fails D, reloads, and proves B remains authoritative. - Semantic CLI rewriting:
rewriteCliProfileOperands()rewrites only--profilevalues and theprofile <command> <operand>identity slot. The regression preserves literal payload text inprofile describe worker --text worker.
I do not see a remaining implementation blocker from my prior review.
Completion blockers still open
- The required hosted exact-head matrix has not run. The three PR-triggered workflows for this SHA — CI run
32261267598, Docker run32261265397, and Nix run32261264672— all completed asaction_required, and each currently has zero jobs. That is an approval-gated non-run, not a green receipt. GitHub correspondingly reportsmergeable_state: blocked. - The PR's own final-build manual plan still leaves two items unchecked: remote Edit Profile display and rejection of canonical
defaultdeletion.
So the branch is conflict-free and the three code blockers are repaired, but all completion blockers are not yet closed. I am not approving this head until the workflows are approved/run and green and those two final-build checks are recorded.
|
Updated the branch through current Upstream split the former monolithic
A fail-closed review caught two initial porting regressions (lost ambient/explicit-local tags and profile-only deletion); both received red-green tests and passed re-review. Exact latest-head verification:
The PR is conflict-free. No prior signed commit was rewritten. |
Adopt upstream's one-hidden-forever-chat model and remove the per-bot hidden session browser while preserving connection-bound remote Bot opening, the full remote action menu, same-name isolation, and global Sessions navigation.
|
Reworked the branch to align with current Bot Mode's one-forever-chat contract while preserving the missing multi-connection affordances. The per-bot hidden-session browser is removed. Ordinary visible history stays in global Sessions; remote Bot rows now directly open their source-owned canonical chat and retain the full source-bound action menu. The reconciliation also includes current upstream canonical adoption/no-fork behavior, group clarify/approval handling, fresh room-session identity, and fail-closed routing for aliases, deletion teardown, group plumbing, DMs, metadata, and cancellation. Current signed head: Verification: 394 Bot, 333 renderer, 88 Electron tests; all Desktop typechecks; clean diff/security scan; successful build-only package. |
|
Verified live on the packaged Windows Desktop v0.20.4 with a token-authenticated WSL remote gateway while the app-managed local gateway remains Primary. Current behavior after registering the remote source:
This matches the manual-test gaps listed in this PR. In particular, acceptance should verify:
Related but separate: #89304 tracks presenting a remote agent as a normal linked Profile in the profile switcher. The behavior above is specifically about the already-rendered remote Bot row and its advertised Bot Mode operations. |
Make explicit registry qualification authoritative: only a current exact ID is accepted, while blank, malformed, unknown, or retired claims fail closed without endpoint inference. Restrict genuinely unqualified legacy descriptors to the shared full-envelope URL/Cloud/SSH matcher, reject zero or multiple matches, normalize SSH host/user identity, and prove remote-primary restoration keeps the exact (connectionId, profile) tuple. Closes NousResearch#90048. Prior work by @teknium1 in NousResearch#89719 and NousResearch#88922, @andrexibiza in NousResearch#90913, and @AndreasG78 in NousResearch#90048 (comment) shaped this implementation. @saralilyb's NousResearch#90006 remains downstream consumer context; the production stopgap is credited but excluded because registry primary does not prove route ownership.
|
Collision/interlock note: #90198 is the surviving owner of registry route identity and is now published at exact head Merge order: land #90198 first after its hosted gates, then rebase #90006 and consume the exact-ID/current-registry contract. Please do not add a second resolver or primary-as-owner heuristic here. Prior work by @saralilyb in #90006 is explicitly credited in #90048 and #90198. |
|
Topology / composition decision after review of #90198: #90198 is the surviving authority for Desktop connection identity. This branch may own connection-scoped profile deletion, teardown ordering, and the deletion gate, but it must not infer or validate source identity independently. Before landing, compose on #90198 and make every
The deletion gate then authorizes mutation of the already-resolved object; it is not an identity resolver. Keep #90913 closed and import the #90198 implementation rather than copying its matcher into this PR. |
|
Seen and agreed. I’ll keep #90006 downstream of #90198 and wait for the resolver owner to merge before updating this branch. After it lands, I’ll rebase #90006 and consume I’ll run the exact-head Desktop, renderer, Electron, typecheck, and packaging matrix after composition and report the resulting head and receipts here. |
Make explicit registry qualification authoritative: only a current exact ID is accepted, while blank, malformed, unknown, or retired claims fail closed without endpoint inference. Restrict genuinely unqualified legacy descriptors to the shared full-envelope URL/Cloud/SSH matcher, reject zero or multiple matches, normalize SSH host/user identity, and prove remote-primary restoration keeps the exact (connectionId, profile) tuple. Closes #90048. Prior work by @teknium1 in #89719 and #88922, @andrexibiza in #90913, and @AndreasG78 in #90048 (comment) shaped this implementation. @saralilyb's #90006 remains downstream consumer context; the production stopgap is credited but excluded because registry primary does not prove route ownership.
e132e11
|
Landed via #92731 (merge commit e132e11) — thank you @saralilyb, this was substantial work and your commits are preserved in the history. What survived intact: the immutable (connectionId, profile) owner capture, requestForBot routing, backend profile aliasing, group session owners, connection-qualified deletion, focused-owner atoms, remote roster merge, and the Electron/sdk/store changes. What we reconciled onto current main rather than restoring: canonical Bot Chat resolution stayed name-identity (the 'Bot Chat' registry row — pointer pins were removed on main after repeated fork incidents) and fail-closed on lookup errors (#92692), now consulted on the bot's own source via your owner routes, so remote bots get the same no-fork guarantees. The renderer-side remote DM delivery was superseded by the message_agent tool architecture (#91802/#91915: the composer middleware identifies mentions, the agent owns delivery) and was not ported. #90198 landed first as the connection-identity authority, per the agreement in this thread. Closing this as landed. |
…ciled with name-identity + fail-closed canonical resolution Salvage of saralilyb's remote-bot routing work onto current main: - kept: immutable (connectionId, profile) owner capture, requestForBot routing, backendTargetProfile aliasing, group session owners, connection-qualified deletion, focused-owner atoms, remote roster merge, Electron profile-delete routing, sdk/store/transcript changes - reconciled: canonical Bot Chat resolution stays NAME-identity (the 'Bot Chat' registry row) and FAIL-CLOSED on lookup errors — now consulted on the bot's own source via the captured owner route, so remote bots get the same no-fork guarantees - dropped: pointer-pin plumbing (preferredSessionIds, saveBotMeta chat writes, pin verification) — superseded by name-identity on main; renderer-side remote DM delivery (deliverRemoteRosterMentions / pollRemoteDmReply / ensureRemoteCanonicalChat) — superseded by the message_agent tool architecture (NousResearch#91802/NousResearch#91915: middleware identifies, never delivers); pointer-era test files deleted on main - openStoredBotChat/createCanonicalChat: remote opens keep Desktop's chrome home (keepAllProfilesScope: true on routed opens); local bots keep the measured workspace re-home - prepareBotSource: capability gate only — routed RPCs never require activation authority
Make explicit registry qualification authoritative: only a current exact ID is accepted, while blank, malformed, unknown, or retired claims fail closed without endpoint inference. Restrict genuinely unqualified legacy descriptors to the shared full-envelope URL/Cloud/SSH matcher, reject zero or multiple matches, normalize SSH host/user identity, and prove remote-primary restoration keeps the exact (connectionId, profile) tuple. Closes NousResearch#90048. Prior work by @teknium1 in NousResearch#89719 and NousResearch#88922, @andrexibiza in NousResearch#90913, and @AndreasG78 in NousResearch#90048 (comment) shaped this implementation. @saralilyb's NousResearch#90006 remains downstream consumer context; the production stopgap is credited but excluded because registry primary does not prove route ownership.
…ciled with name-identity + fail-closed canonical resolution Salvage of saralilyb's remote-bot routing work onto current main: - kept: immutable (connectionId, profile) owner capture, requestForBot routing, backendTargetProfile aliasing, group session owners, connection-qualified deletion, focused-owner atoms, remote roster merge, Electron profile-delete routing, sdk/store/transcript changes - reconciled: canonical Bot Chat resolution stays NAME-identity (the 'Bot Chat' registry row) and FAIL-CLOSED on lookup errors — now consulted on the bot's own source via the captured owner route, so remote bots get the same no-fork guarantees - dropped: pointer-pin plumbing (preferredSessionIds, saveBotMeta chat writes, pin verification) — superseded by name-identity on main; renderer-side remote DM delivery (deliverRemoteRosterMentions / pollRemoteDmReply / ensureRemoteCanonicalChat) — superseded by the message_agent tool architecture (NousResearch#91802/NousResearch#91915: middleware identifies, never delivers); pointer-era test files deleted on main - openStoredBotChat/createCanonicalChat: remote opens keep Desktop's chrome home (keepAllProfilesScope: true on routed opens); local bots keep the measured workspace re-home - prepareBotSource: capability gate only — routed RPCs never require activation authority
…mote-bots feat(desktop): remote bots open their own Bot Chat without re-homing Desktop (salvage NousResearch#90006)
Summary
gateway
switching Desktop's chrome/API home
duplication, new chats, routines, and deletion to the captured owner
aggregates connected gateways
Safety and compatibility
(connectionId, profile, targetProfile)ownership beforereads and mutations
nested profile maps
Bot Chatbefore minting; transient lookup/resumefailures never fork the forever-chat
and Bot metadata by connection and profile
writes
Verification
Exact code from signed merge head
ddc78217fdc1a34a4b91c6f78362b20d8415aa43was reconciled with current
main603d5651b31f0c46d18138946d8c4009fdf2c3d4and verified locally:
git diff --checkhermes desktop --build-onlyManual test plan
the launch connection remains Desktop's home
and permitted Delete actions
defaultcannot be deleted