feat(desktop): multi-connection registry — named agent sources (schema v2 + IPC + Settings UI) - #86679
Conversation
…a v2 + IPC + Settings UI) First slice of multi-source agent support: the desktop can now persist ANY number of named backends (local runtime, remote gateways, Hermes Cloud instances, SSH hosts) side by side instead of one global connection plus per-profile overrides. - electron/connection-registry.ts: pure v2 registry module — required case-insensitively-unique labels (device names), @name-device handle rule for duplicate profile names across sources (agentHandle), defensive normalizeRegistry for corrupt files, one-time v1→v2 migration that imports the global block + per-profile overrides (deduped by URL/host) and leaves connection.json untouched for older builds. - main.ts: connections.json storage beside connection.json (same secret posture: safeStorage-encrypted tokens, 0600, tighten-before-parse, mtime cache) + hermes:connections:* IPC (list/save/remove/set-primary/test). Test maps registry entries onto the existing testDesktopConnectionConfig probe stack — no new probe code. - Settings → Connections: manage the registry (add/edit/remove/test/make primary) with forced naming; local entry is non-removable; removing the primary retargets to local. en + zh locales. Storage-level only by design: routing/pool generalization to composite (connection, profile) keys, the multi-source roster, plugin SDK surface, and fan-out updates land as follow-up PRs.
૮ >ﻌ< ა ci reviewran on 08abe2e — fix(desktop): address connections-registry review findings
|
trevorgordon981
left a comment
There was a problem hiding this comment.
Solid structure overall — the pure connection-registry.ts module with centralized agentHandle/label-slug rules, defensive normalizeRegistry that degrades corrupt files to local-only, and migration dedup by fingerprint is genuinely well done. Token bytes correctly never cross the IPC boundary. A couple of things need attention before merge.
1. The new Settings UI can't actually save a fresh token (blocking)
saveRegistryConnection (main.ts) calls resolvePersistedRemoteToken({ ..., persistToken: true, allowPlainText: input.allowPlainTextToken, ... }) and then unconditionally rejects with "Remote gateway session token is required." when !decryptDesktopSecret(entry.token).
But the new ConnectionsSettings.tsx save handler builds its payload with only {kind, label, id, url, authMode, token, host, user, port, keyPath} — it never sends allowPlainTextToken. So creating a brand-new token-authed remote depends entirely on encryptDesktopSecret(incomingToken, { allowPlainText: false }) succeeding and round-tripping. That path has zero test coverage in this PR — all connection-registry.test.ts cases are pure-module and the React test mocks the bridge entirely, so hermes:connections:save is untested. If encryptDesktopSecret throws when safeStorage is unavailable (headless/CI/local without keychain), creating a token remote silently fails at the IPC layer. The v1 flow explicitly threads allowPlainTextToken from input and has a focused regression test on that seam; this new handler ships the same secret-handling responsibility bare. At minimum add an IPC-level test for the required-token branch and confirm the UI can persist a fresh token.
2. Editing remote token→oauth leaves a dangling token
When editing (id present), existingToken is inherited unconditionally. Switching a remote from token to oauth keeps the stale token envelope on the entry, which decryptDesktopSecret still reads. Not a leak (still encrypted, never crosses IPC), but it's dead secret material on a connection that no longer needs it. Clear token when authMode is non-token, mirroring the v1 leavingCloud reset.
3. First-run migration is a read-then-write race
readDesktopConnectionsRegistry() does migrate-then-write on first boot. Two racing processes (updater relaunch, second window) both migrate and both write; the later write wins but labels were derived independently in each. Stable in practice under single-instance, but the write isn't guarded against a concurrent first-run.
Tests
connection-registry.test.ts is excellent (21 cases covering labels/handles/validation/normalize/migration/ops) and connections-settings.test.tsx gives good component smoke coverage. The gap is the main-process IPC layer — save/remove/set-primary/test have no coverage, and save is the one that handles secrets, which is exactly the finding in #1.
|
The registry foundation looks strong: keeping normalization and migration pure, centralizing label and handle rules, leaving The fresh Hermes Cloud path currently looks unusable. “Add connection” allows “Make primary” is also exposed before anything consumes the v2 primary for routing. It updates The token-storage concern from the earlier review is valid on keyring-less Linux. The existing Gateway flow can ask for explicit plaintext-storage consent and resend with Could we also add focused coverage around the actual |
|
Ran a full review pass (targeted suites 21/21 + 4/4, whole 1. HIGH — editing an entry wipes fields the editor doesn't carry
2. HIGH — hidden stale ssh The ssh editor shows a single host field ( 3. MED-HIGH — registry "Test" inherits v1 state
Fix: build the probe directly from the registry entry (or pass an empty 4. MED —
5. LOW polish
Verified clean: token bytes never cross IPC (preview + set flag, matching v1 sanitize); safeStorage/0600/tighten-before-parse/atomic-write all correctly reused from hardening.ts; en/zh key parity exact (35/35); migration URL/SSH fingerprint dedup works as described; settings nav/type/icon wiring complete. |
Review fixes from #86679 comments (trevorgordon981, helix4u, kshitijk4poor): - Edit inheritance: mergeConnectionInput preserves fields the editor does not carry (cloud org, ssh remoteHermesPath/remoteProfile) so a rename no longer wipes them. When the payload carries an ssh host string, stored user/port are NOT inherited — the composite host field is authoritative, fixing the stale user/port resurrection on edit. - Token hygiene: tokens only persist on token-auth remotes; switching an entry to oauth (or cloud) clears the stale envelope. - Plain-text opt-in: the panel now surfaces the same consent dialog as Settings -> Gateway on keyring-less machines (registry list exposes secureTokenStorage; save retries with allowPlainTextToken after consent). - Registry test isolation: hermes:connections:test builds the probe directly from the registry entry instead of coercing against v1 connection.json — no more inheriting the v1 global token for a different host, and the local entry now probes the app-managed backend (never v1 remote/ssh state, so the test button can no longer trigger a v1 file write). - 'local' id reserved at the validation boundary: a crafted IPC payload can no longer replace the local entry via upsert. - Cloud creation hidden in the editor (a dialable cloud entry comes from the Cloud sign-in/discovery flow); migrated cloud entries stay editable. - First-run migration write is guarded: a failed write keeps the migrated registry in memory instead of hard-failing every connections IPC call. - uniqueLabel(): single label-dedup helper — counts up instead of "X 2 2", clamps 253-char migrated URL-host labels under LABEL_MAX; used by normalizeRegistry and both migration paths. - UI copy: staged-rollout note replaces the "side by side" claim; test failure toast leads with the failure wording; dropped unused i18n keys. Tests: +9 pure-module cases (reserved id, token-drop rules, merge inheritance, ssh host precedence, uniqueLabel); electron+settings suites 1355 passed.
…stry) Covers the named-source registry from #86679: forced unique device names, @profile-device disambiguation, add/edit/remove/test, automatic v1 import, cloud-via-discovery, encrypted token storage, and the staged rollout note.
…ped pool keys Phase 2 of the multi-connection campaign (#86679 shipped the registry). The Electron backend pool can now serve agents from ANY registered connection concurrently, keyed by composite (connection, profile) scopes. - connection-registry.ts: backendScopeKey(connectionId, profile) — the single home of the composite-key rule. Local/empty connection ids keep the BARE profile key, so every legacy pool entry, reaper log line, and touch call is byte-identical for single-source users; non-local connections get `conn:<id>::<profile>`, which cannot collide with a plain profile name. backendScopePrefix() matches the keys a connection owns (teardown on remove). - main.ts ensureRegistryBackend(connectionId, profile): resolves a backend against the v2 registry. local kind delegates to ensureBackend() untouched; remote/cloud dial the entry's own URL/auth (descriptor carries profile + connectionId + sharedRemote for per-request ?profile= scoping); ssh bootstraps a tunnel scoped to the composite key, with the served dashboard token persisted back onto the REGISTRY entry (not v1 connection.json). Pool entries reuse the existing LRU/idle-reaper/touch lifecycle. - hermes:connections:remove now stops every pooled backend + ssh scope the removed connection owns. - New IPC hermes:connection:for + preload getConnectionFor + renderer types (connectionId/sharedRemote on HermesConnection). No renderer behavior change yet — the multi-source roster/socket switchover is PR 3. Tests: +2 backendScopeKey contract cases (28 total in the registry suite); electron + settings projects 1357 passed; both tsc configs and eslint clean.
… fan-out updates Phases 3-5 of the multi-connection campaign in one PR (per Teknium), on top of the registry (#86679) and composite-key backend routing (#86839). Agents from every registered connection are now usable side by side. Renderer socket registry (phase 3): - backendScopeKey moves to apps/shared (@hermes/shared) so main-process pool keys and renderer socket keys derive from ONE rule; the electron module keeps a byte-identical twin (tsconfig project boundaries) pinned by a cross-copy contract test. - store/gateway secondaries are scope-keyed: entries carry (connectionId, profile); registry-scoped entries dial through getConnectionFor + getGatewayWsUrlFor (fresh per-connect OAuth tickets against the right host); events keep the bare profile plus a connectionId tag; touch/idle keepalive uses the scope key; pruning keeps entries whose PROFILE has live work. New ensureGatewayForAgent/openGatewayForAgent fall through to the profile path for local/null sources — single-source behavior byte-identical. Union roster + plugin SDK (phases 3+4, the Bot Mode door): - hermes:agents:roster enumerates every connection's /api/profiles concurrently (eager REST, lazy sockets; unreachable sources report per-row; undialed ssh boxes stay connect-on-demand) and flattens through buildAgentRoster — the @name-device duplicate-handle rule applied once across all sources, pure + tested. - SDK: host.connections(), host.agents(), host.warmAgent(), host.ensureAgent() — feature-detected so plugins degrade cleanly on older Desktop builds. Fan-out updates (phase 5): - hermes:connections:update-all dispatches hermes update to every eligible source in parallel: local via the app's own applyUpdates pipeline, remote/ssh via the backend's own POST /api/hermes/update; cloud skipped as platform-managed (updateEligibility, pure + tested); per-connection result rows so one dead box can't wedge the batch. Settings → Connections gains the "Update all instances" button (shown with 2+ connections). Also: getJsonForBackend/postJsonForBackend helpers with the token/OAuth-cookie auth split; docs section updated from "staged rollout" to live behavior. Tests: +4 pure cases (cross-copy contract, roster handles, unreachable sources, update eligibility); FULL desktop suite 5115 passed; tsc renderer + electron + shared clean; eslint clean.
… fan-out updates Phases 3-5 of the multi-connection campaign in one PR (per Teknium), on top of the registry (#86679) and composite-key backend routing (#86839). Agents from every registered connection are now usable side by side. Renderer socket registry (phase 3): - backendScopeKey moves to apps/shared (@hermes/shared) so main-process pool keys and renderer socket keys derive from ONE rule; the electron module keeps a byte-identical twin (tsconfig project boundaries) pinned by a cross-copy contract test. - store/gateway secondaries are scope-keyed: entries carry (connectionId, profile); registry-scoped entries dial through getConnectionFor + getGatewayWsUrlFor (fresh per-connect OAuth tickets against the right host); events keep the bare profile plus a connectionId tag; touch/idle keepalive uses the scope key; pruning keeps entries whose PROFILE has live work. New ensureGatewayForAgent/openGatewayForAgent fall through to the profile path for local/null sources — single-source behavior byte-identical. Union roster + plugin SDK (phases 3+4, the Bot Mode door): - hermes:agents:roster enumerates every connection's /api/profiles concurrently (eager REST, lazy sockets; unreachable sources report per-row; undialed ssh boxes stay connect-on-demand) and flattens through buildAgentRoster — the @name-device duplicate-handle rule applied once across all sources, pure + tested. - SDK: host.connections(), host.agents(), host.warmAgent(), host.ensureAgent() — feature-detected so plugins degrade cleanly on older Desktop builds. Fan-out updates (phase 5): - hermes:connections:update-all dispatches hermes update to every eligible source in parallel: local via the app's own applyUpdates pipeline, remote/ssh via the backend's own POST /api/hermes/update; cloud skipped as platform-managed (updateEligibility, pure + tested); per-connection result rows so one dead box can't wedge the batch. Settings → Connections gains the "Update all instances" button (shown with 2+ connections). Also: getJsonForBackend/postJsonForBackend helpers with the token/OAuth-cookie auth split; docs section updated from "staged rollout" to live behavior. Tests: +4 pure cases (cross-copy contract, roster handles, unreachable sources, update eligibility); FULL desktop suite 5115 passed; tsc renderer + electron + shared clean; eslint clean.
Summary
The desktop can now register ANY number of named agent sources — the local runtime, remote gateways (LAN/Tailscale/internet), Hermes Cloud instances, and SSH hosts — persisted together in one registry, instead of a single global connection with per-profile overrides bolted on.
This is PR 1 of the multi-source campaign: schema + storage + IPC + Settings UI. Routing/pool generalization to composite
(connection, profile)keys, the multi-source agent roster, the plugin-SDK surface (Bot Mode), and fan-out updates follow as separate PRs.Changes
electron/connection-registry.ts(new, pure): v2 registry model. Required, case-insensitively-unique labels (device names) enforced at save;agentHandle()is the single home of the@name-devicedisambiguation rule for duplicate profile names across sources;normalizeRegistrydegrades corrupt files to local-only;migrateV1ToRegistryimports the v1 global block + per-profile overrides (deduped by URL/SSH fingerprint) exactly once.electron/main.ts:connections.jsonbesideconnection.json(v1 file untouched — older builds keep working) with the same secret posture: safeStorage-encrypted tokens, 0600, tighten-before-parse, mtime cache. New IPC:hermes:connections:list/save/remove/set-primary/test;testmaps entries onto the existingtestDesktopConnectionConfigprobe stack (zero new probe code).electron/preload.ts+src/global.d.ts: typedhermesDesktop.connectionsbridge; token bytes never cross IPC (preview + set flag only).defineLocale).Validation
connection-registry.test.ts(labels, validation, migration, ops)connections-settings.test.tsx(list, required-label save, set-primary, test)electron/suitetscrenderer + electron configsInfographic