feat(desktop): OAuth sign-in for registry connections; keep profile picks on the browsed source - #92194
Conversation
Main concern, and it's a repo-policy one: several of the new tests are source-reading tests, which this codebase explicitly bans (they test the shape of the source, not behavior, and break on harmless refactors):
The first two tests in that file do it right-ish by extracting the real helper via Minor, non-blocking:
|
|
Acted on all of it — thanks, the shape-test point was fair. Shape tests → behavior tests. The two open paths shared a duplicated toast block; that duplication was the only reason a proximity-regex test existed. Extracted On full module extraction: considered and deliberately not done. Runtime-fetched plugins load as a single blob-URL Minor notes, both taken: the debounced probe now sets a For scope: the AGENTS.md rule on source-reading tests is scoped to plugin compatibility tests (§ deprecations), so this wasn't a policy violation as such — but the behavioral versions are better tests regardless, which is why the point was worth acting on. |
4b06984 to
e8191cc
Compare
|
Rebased onto current main and rescoped: #90006 (connection-bound Bot Mode actions) merged while this sat in queue and solves the open-remote-bots problem connection-bound, without moving the window — strictly better than this PR's activate-the-owner approach, so that commit and its tests are dropped rather than re-fought. What remains is orthogonal to #90006 and unchanged in substance:
On the rebased branch: plugin suite 406/406 (including #90006's tests), renderer suite 7065 passed / 3 skipped, all three typecheck projects and eslint clean. |
|
Post-#92731 status (verified against main @ e132e11): partially superseded — re-scope before rebasing. Superseded: the plugin.js commit (ACTIVATABLE_SOURCE_KINDS / canOpenOnOwnSource / mentionOnlyNotice). Main's BotRow.open() now routes through requestForBot()/botConnectionRoute() on an immutable captured owner — remote bots open their canonical Bot Chat WITHOUT moving the active gateway, which replaces this PR's ensureGatewayAgent-activation approach entirely; the mention-only toast it removes no longer exists. Still valuable and unduplicated: (1) OAuth sign-in in the connections-registry editor (main's oauth branch is still empty), and (2) the profile.ts fix — selectProfile()/newSessionInProfile() still call bare ensureGatewayProfile(target), so a profile pick while a registry source is active still snaps home. If you drop the plugin.js commit and rebase the other two, this stays a wanted PR. |
e8191cc to
246fc44
Compare
|
Agreed on all points — and the rescope you describe is exactly what this PR became this morning (comment above): the plugin.js commit and its tests were dropped when #90006 landed, for the same reason you give — the connection-bound owner route replaces gateway activation outright, and the toast it gated no longer exists. Now also rebased onto current main (post-#92731): the two surviving commits apply clean, and your read matches what I see there — the registry editor's oauth branch is still empty, and Heads-up on CI: the last few |
246fc44 to
0613950
Compare
A gated remote gateway (OAuth, or username/password) never accepts a session token — it authenticates with a browser sign-in and the desktop keeps whatever the flow mints. The registry editor only rendered a token field for 'token' mode and nothing at all for 'oauth', so a gated connection could be created but never authenticated: selecting OAuth left an empty row, and Test failed with no way to fix it. Render an Authentication row in the oauth branch that calls the existing oauthLoginConnectionConfig IPC — the same one first-run-remote-form and the gateway panel already use. The URL is probed (debounced) so the row can name the provider and use password-specific copy when every advertised provider supports passwords, matching gateway-settings. No new i18n keys; all strings already exist under settings.gateway. Test needed no change: testDesktopConnectionConfig already skips the token for oauth and mints a ws-ticket from the session.
$profiles is the ACTIVE gateway's list, so a profile picked while a registry source is live names one of THAT source's profiles. Both selectProfile and newSessionInProfile sent it through the profile-only path, which resolves the descriptor with a bare name — and getConnection(profile) is answered against the primary. Picking "researcher" while browsing a remote source therefore opened a LOCAL backend of that name and snapped the gateway home, so the pick looked like it never took: the user could reach the agent from Bot Mode but never from the profile switcher. Route both through the live source instead: a non-null activeGatewayConnectionId means a registry source owns the current gateway, so activate the (connection, profile) agent. A null id means the primary is live, which is exactly the legacy path — single-source users keep their existing behavior unchanged.
…gned-in pill on mode flips Review follow-ups on the OAuth sign-in row: the debounced probe sets a cancelled flag in its effect cleanup (probeSeq covers staleness but not unmount), and oauthConnected resets when the auth mode flips as well as on URL changes — a saved row edited token -> oauth no longer reports a stale 'Signed in' from an earlier oauth stint.
0613950 to
efaac4d
Compare
OutThisLife
left a comment
There was a problem hiding this comment.
Approved — the profile-snap fix is the right shape: activateOnCurrentSource routes through ensureGatewayAgent(connectionId, profile) when a registry source is live, and falls back to the legacy path when the primary owns the socket. Bug-class tests cover both paths. The OAuth sign-in row is a clean reuse of the existing oauthLoginConnectionConfig IPC. Thanks for the rescope after #90006/#92731 landed.
OutThisLife
left a comment
There was a problem hiding this comment.
Approved — the profile-snap fix is the right shape: activateOnCurrentSource routes through ensureGatewayAgent(connectionId, profile) when a registry source is live, and falls back to the legacy path when the primary owns the socket. Bug-class tests cover both paths. The OAuth sign-in row is a clean reuse of the existing oauthLoginConnectionConfig IPC. Thanks for the rescope after #90006/#92731 landed.
OutThisLife
left a comment
There was a problem hiding this comment.
Approving. Verified against current main: the registry editor's oauth branch is still empty and selectProfile()/newSessionInProfile() still route through bare ensureGatewayProfile(target), so both surviving commits are unduplicated. The self-rescope after #90006 and the rebase past #92731 match what @teknium1 asked for, and the review follow-ups (probe unmount guard, mode-flip reset) are in.
Required checks are green on the current head; the earlier zero-job ci.yaml runs were the infra window that hit several unrelated PRs that morning.
Unrelated note for anyone routed here from the Discord thread about Desktop reverting to a local backend — that one is v1/v2 connection-registry drift, fixed in #93408, not this PR.
…+ palette local-agent routing Three issues surfaced after merging upstream/main (427 commits) into dev: 1. web-bridge-shim.test.ts: profile-remote-override-dialog.tsx (fork feature, NousResearch#91349) and connections-registry.tsx's OAuth sign-in row (upstream feature, NousResearch#92194) call window.hermesDesktop.applyConnectionConfig / oauthLoginConnectionConfig without a method-level guard. Guard both call sites so the web build shim's partial-shim trap test passes and the browser build doesn't throw "is not a function". 2. profile-select-agent.test.ts / config-settings.test.tsx: stale vi.mock factories for @/store/gateway and @/store/settings-scope were missing activeGatewayConnectionId and $settingsRequestProfile respectively, both added by upstream commits merged in. Updated the mocks to match current module shape. 3. src/store/profile.ts: genuine semantic merge conflict (no textual conflict). Upstream's NousResearch#92194 changed activateOnCurrentSource so a bare selectProfile() call stays on whichever source is currently browsed (activeGatewayConnectionId()), instead of always targeting the local pool. The fork's selectAgent(null, name) — the command palette / profile rail's explicit "switch to this device" action (NousResearch#85731) — delegated to selectProfile() assuming the OLD always-local behavior. Added an explicit forceLocal option so selectAgent's null-connection case still pins to the local pool, while every other selectProfile() caller keeps upstream's new stay-on-browsed-source behavior. Verified: typecheck clean, full vitest suite green (726 files / 7582 tests).
… invariants run
The vi.mock('@/store/gateway') factory in profile-select-agent.test.ts
predates NousResearch#92194's activateOnCurrentSource, which now reads
activeGatewayConnectionId() on every null-connection selectProfile
delegation — three (connection, profile) switching-invariant tests died
in the mock loader instead of asserting. Add the registry double
(vi.fn -> null, reset per test, steerable like
profile-select-source.test.ts).
Likewise config-settings.test.tsx's vi.mock('@/store/settings-scope')
lacked $settingsRequestProfile, which config-settings.tsx:61 now reads
via useStore; both settings-retry regression tests failed before
rendering. Mirror the real module: a computed over the override atom
mapping null -> undefined.
No assertion changed; all 8 tests in the two files pass again.
… invariants run
The vi.mock('@/store/gateway') factory in profile-select-agent.test.ts
predates NousResearch#92194's activateOnCurrentSource, which now reads
activeGatewayConnectionId() on every null-connection selectProfile
delegation — three (connection, profile) switching-invariant tests died
in the mock loader instead of asserting. Add the registry double
(vi.fn -> null, reset per test, steerable like
profile-select-source.test.ts).
Likewise config-settings.test.tsx's vi.mock('@/store/settings-scope')
lacked $settingsRequestProfile, which config-settings.tsx:61 now reads
via useStore; both settings-retry regression tests failed before
rendering. Mirror the real module: a computed over the override atom
mapping null -> undefined.
No assertion changed; all 8 tests in the two files pass again.
…icks on the browsed source (NousResearch#92194) * feat(desktop): add OAuth sign-in to the connections registry editor A gated remote gateway (OAuth, or username/password) never accepts a session token — it authenticates with a browser sign-in and the desktop keeps whatever the flow mints. The registry editor only rendered a token field for 'token' mode and nothing at all for 'oauth', so a gated connection could be created but never authenticated: selecting OAuth left an empty row, and Test failed with no way to fix it. Render an Authentication row in the oauth branch that calls the existing oauthLoginConnectionConfig IPC — the same one first-run-remote-form and the gateway panel already use. The URL is probed (debounced) so the row can name the provider and use password-specific copy when every advertised provider supports passwords, matching gateway-settings. No new i18n keys; all strings already exist under settings.gateway. Test needed no change: testDesktopConnectionConfig already skips the token for oauth and mints a ws-ticket from the session. * fix(desktop): keep profile picks on the source being browsed $profiles is the ACTIVE gateway's list, so a profile picked while a registry source is live names one of THAT source's profiles. Both selectProfile and newSessionInProfile sent it through the profile-only path, which resolves the descriptor with a bare name — and getConnection(profile) is answered against the primary. Picking "researcher" while browsing a remote source therefore opened a LOCAL backend of that name and snapped the gateway home, so the pick looked like it never took: the user could reach the agent from Bot Mode but never from the profile switcher. Route both through the live source instead: a non-null activeGatewayConnectionId means a registry source owns the current gateway, so activate the (connection, profile) agent. A null id means the primary is live, which is exactly the legacy path — single-source users keep their existing behavior unchanged. * fix(desktop): cancel the registry auth probe on unmount; reset the signed-in pill on mode flips Review follow-ups on the OAuth sign-in row: the debounced probe sets a cancelled flag in its effect cleanup (probeSeq covers staleness but not unmount), and oauthConnected resets when the auth mode flips as well as on URL changes — a saved row edited token -> oauth no longer reports a stale 'Signed in' from an earlier oauth stint.
Problem
Two gaps around registered connections:
A gated remote can't be authenticated at all. The connections registry editor renders a token
field for
authMode: 'token'and nothing for'oauth'. A gateway behind a login (OAuth, orusername/password) never accepts
X-Hermes-Session-Token—_require_tokenallows exactly onescheme per bind — so the connection can be created and then never signed in. Selecting OAuth left
an empty row and Test failed with no way to fix it.
Profile picks snap back to the primary.
$profilesis the active gateway's list, so a pickmade while a registry source is live names one of that source's profiles — but
selectProfileandnewSessionInProfilerouted it through the profile-only path, whoseresolveConnectionForProfilecalls
getConnection(profile)with a bare name. The main process answers a bare name against theprimary, so picking
researcheron a remote source opened a local backend of that name andyanked the gateway home. The pick looked like it never took.
Changes
feat(desktop): add OAuth sign-in to the connections registry editor— render an Authenticationrow in the oauth branch that calls the existing
oauthLoginConnectionConfigIPC, the same onefirst-run-remote-formand the gateway panel already use. The URL is probed (debounced) so the row canname the provider and use password-specific copy when every advertised provider supports passwords,
matching
gateway-settings. No new i18n keys — every string already exists undersettings.gateway.testDesktopConnectionConfigneeded no change: it already skips the token for oauth and mints aws-ticket from the session.
fix(desktop): keep profile picks on the source being browsed— routeselectProfileandnewSessionInProfilethrough the live source: a non-nullactiveGatewayConnectionId()means a registrysource owns the current gateway, so activate the
(connection, profile)agent. A null id means theprimary is live, which is exactly the legacy path — single-source users are unaffected.
fix(desktop): cancel the registry auth probe on unmount; reset the signed-in pill on mode flips—review follow-ups on the sign-in row: the debounced probe sets a cancelled flag in its effect cleanup,
and
oauthConnectedresets when the auth mode flips as well as on URL changes.Scope
This does not make two gateways live at once;
$gatewayremains a single socket. With #90006 in,bot-row opens are connection-bound and never move the window — this PR's switcher fix is the same
principle applied to the profile rail: a pick made while browsing a source stays on that source.
Testing
profile-select-source.test.ts(3) covering both routing directionstsc --noEmit(all three projects) and eslint cleanVerified manually against a LAN gateway (
kind: remote, gated by username/password, advertisingauth_flows: ['cookie', 'native_pkce']): sign-in completes via native PKCE, and profile picks stayon that source.
Known gap (follow-up, not in this PR)
Found while dogfooding the same two-machine setup: a registry row with
authMode: 'token'pointingat a gated gateway can never activate —
_require_tokenaccepts only cookie/bearer on a gatedbind — so switching to that connection fails with only the generic
Connection "…" did not become active.(in our case it was the primary row for a gated localdaemon, which made "switch back to local" look broken). With this PR the row can at least be flipped
to
oauthand signed in from the registry editor; before it there was no path at all. Two candidatefollow-ups: name the auth-mode mismatch in the switch error instead of the generic copy, and default
rows for gateways that advertise
auth_requiredtooauth.