feat(desktop): expose resolved connection mode to skills, MCP, and plugins - #82187
feat(desktop): expose resolved connection mode to skills, MCP, and plugins#82187jackulau wants to merge 25 commits into
Conversation
alt-glitch
added
type/feature
02b5913 to
de5de6a
Compare
|
Rebased onto current main (clean, all 8 commits). Re-verified on the rebased head: 66 Python tests pass ( |
de5de6a to
794cca5
Compare
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Review summary
There is a lot to like here. The API shape is appropriately narrow: the renderer announces only local/remote, the gateway normalizes and stores it per Desktop session, skill subprocesses get a write-only stamp, MCP gets per-call metadata, and plugins read the active-profile connection atom. The explicit non-Desktop and shell-spoofing treatment is especially good.
I reviewed exact head 794cca53e71b9e50fe4921e7c493e08b6960d877 against its pinned base 76d832d3857551a029c4b39c23945eb47c16fe5b, inspected all changed implementation, tests, and docs, and replayed the head onto current main (f4c2c263f0672a4b1485f3071cd5f79cd32d38ab) without a textual merge conflict.
Blocking: preserve the mode across dashboard compute-host turn isolation
tui_gateway/server.py:_compute_host_turn_frame() sends source to the compute-host child but not the resolved connection_mode, and tui_gateway/compute_host.py:_ensure_server_session() consequently recreates the child-side Desktop session without a mode. The child then enters the normal _run_prompt_submit() path and _set_session_context() binds None.
That makes every extension read path added here unavailable during an isolated Desktop turn:
- skill subprocesses omit
HERMES_DESKTOP_CONNECTION_MODE; - MCP calls omit
hermes-agent.nousresearch.com/desktop-connection-mode; - anything reading
desktop_connection_mode()in the turn seesNone.
I reproduced this directly at the pinned head with a parent Desktop session whose resolved mode was remote:
parent_resolved_mode='remote'
frame_connection_mode=None
host_resolved_mode=None
host_ambient_mode=None
The dashboard isolation path is dormant under the current default (dashboard.turn_isolation: false), but it is an implemented, user-selectable execution path. Issue #82140's acceptance criterion is that skills and MCP can read the value when running in a Desktop session; enabling turn isolation should not erase the session's connection identity.
Please carry the normalized mode in the turn.start frame, apply/refresh it when _ensure_server_session() creates or reuses the child session, and add a regression test that exercises a Desktop remote turn through this compute-host boundary and observes the mode from the child turn context (including reuse after switching to local).
Verification receipts
- Sanitized exact-head focused Python suite: 66 passed.
- Desktop focused Vitest suite: 21 passed.
- Desktop TypeScript typecheck: passed.
- Ruff, Python compilation, and
git diff --check: passed. - Clean merge replay onto current
main: passed.
One environment-sensitive test initially failed because this review itself is running inside Desktop and already exports HERMES_DESKTOP / HERMES_DESKTOP_CWD; rerunning the suite with those outer-process markers removed produced the 66-pass result above. That is harness contamination, not a candidate defect.
Once the compute-host boundary carries the mode and the regression is present, the rest of this implementation is in strong shape.
|
Addressed the blocking compute-host review in 6a115f78f. Frame ( Child ( Regression tests ( Drive-by found by the new fallback test: the fallback path referenced Verified on the new head: all 4 focused Python suites plus the connection-mode RPC file pass (72 tests, including the 6 new ones); |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Follow-up review: additional Desktop execution paths
I did a second, independent pass over exact head 794cca53e71b9e50fe4921e7c493e08b6960d877. The compute-host blocker in my earlier review still stands. This follow-up found five additional runtime gaps plus one public-API documentation mismatch that should be closed at the same head.
1. Blocking — background and preview agents lose the parent Desktop mode
prompt.background and preview.restart call _set_session_context() with fresh bg_* / preview_* task IDs (tui_gateway/methods_prompt.py:800,897). _set_session_context() derives connection_mode only by finding a live session whose session_key equals the supplied ID (tui_gateway/server.py:3268-3277). Those ephemeral IDs are not session keys, so the parent mode is discarded even though the parent session object is available to both callers.
Pinned-head reproduction from a parent Desktop session with connection_mode="remote":
ephemeral_mode=None
ephemeral_env_mode=None
ephemeral_mcp_meta=None
The child should inherit an explicitly supplied, normalized parent mode. Please cover both background and preview with a regression that reads the Python accessor, subprocess stamp, and MCP _meta inside the detached agent.
2. Blocking — SKILL.md inline-shell preprocessing bypasses the context-aware subprocess environment
agent/skill_preprocessing.py:73-82 invokes subprocess.run() without env=build_subprocess_env(). Unlike terminal/tool subprocesses, !\...`` expansion therefore does not receive the current task's write-only stamp and does not pass through the central inherited-value scrub.
The probe's control factory returned remote, while the actual inline-shell call supplied no env at all:
factory_mode='remote'
inline_env='<missing>'
Please route this spawn through the same central factory and add a Desktop-session regression proving a live remote ContextVar overrides/strips any ambient HERMES_DESKTOP_CONNECTION_MODE value.
3. Blocking — plugin host.request() never announces the mode
apps/desktop/src/sdk/index.ts:103-110 sends directly through $gateway.get().request(). It bypasses useGatewayRequest() and thus never applies withConnectionMode() to session.create, session.resume, or prompt.submit.
A runtime plugin using the SDK's documented JSON-RPC door can therefore create or drive a Desktop session whose skills/MCP context sees no mode, even while ctx.connection.mode() reports remote. A focused Vitest sink captured prompt.submit without connection_mode.
Please give hook and non-hook/plugin callers one shared request helper, and test all three stamped RPC methods through host.request().
4. Blocking — profile activation publishes the new gateway before its connection descriptor
ensureGatewayProfile() activates the target gateway and updates $activeGatewayProfile before awaiting getConnection(target) (apps/desktop/src/store/profile.ts:290-297). During that asynchronous window, $gateway already targets the remote backend while $connection still describes the prior local backend; if descriptor lookup fails, the mismatch persists by design (:241-252).
A controlled switch reproduced exactly that state:
active profile = remote
active gateway = remote socket
ctx/$connection mode = local
Any plugin reacting to host.state.profile, or any concurrent request during the switch, can announce local to the remote gateway. Please publish gateway/profile/descriptor as one consistent transition (and fail without exposing a mixed state), then add a deferred-descriptor test that asserts the public atoms never disagree.
5. Blocking — a plugin mode-listener exception escapes through core connection updates
apps/desktop/src/contrib/plugin.ts:195,201-207 invokes plugin callbacks directly, both for the immediate notification and from the $connection subscription. A focused probe confirmed that a listener throwing on a real local -> remote transition propagates out of setConnection().
That lets one plugin abort profile synchronization/reconnect code and destabilize the renderer. Please isolate each callback, report the plugin-attributed error, keep other listeners running, and test both the immediate call and a later transition.
6. Required docs fix — the published FastMCP example is not executable against the pinned SDK
website/docs/developer-guide/desktop-connection-mode.md:107-110 shows @server.call_tool() and context.meta. With this tree's mcp==1.28.1, live introspection returns:
FastMCP.call_tool(self, name, arguments) # not a decorator
FastMCP.tool(...) # decorator
Context.meta = absent
Context.request_context = present
The supported FastMCP shape is a @server.tool() handler using ctx.request_context.meta. Please replace the example and add a docs/example smoke test against the pinned MCP SDK.
Follow-up receipts
- Python context-path probe: reproduced both failures, logical
EXIT=2; SHA-256e3558529cebc3d2019e27ee158d44f6629ea642c0854d6ac07a7598781df49b4. - Desktop focused probes: 3 passed (plugin request bypass, mixed profile/gateway mode window, listener exception propagation); output SHA-256
be53b4017fa0c267b2ad0e1b5e6a82a5beeb115b6a7bb33b667acc9b62a9514e. - MCP API introspection receipt SHA-256:
0871a33bfb8b0323b8e46c9591263ccea0de9c2842ee04be300d095022fb53c8. - The pinned source worktree is clean; probes were removed from it after execution.
The narrow connection-mode model remains the right design. These gaps are placement/propagation failures around supported execution surfaces, not a reason to broaden the data exposed.
|
All six follow-up items addressed, in six focused commits on top of the compute-host fix (6a115f78f). Per item: 1. Background/preview agents (615d5a84f): 2. Inline-shell preprocessing (3021b70d1): 3. Plugin 4. Profile activation (80d562e22): new 5. Listener isolation (344d739da): every 6. FastMCP docs (d7df59d28): the example is now a Verification on the new head (d7df59d28): 84 Python tests pass across the five connection-mode/preprocessing suites; 41 desktop Vitest tests pass across |
d7df59d to
3702a4b
Compare
|
Rebased all 15 commits onto current `main` — the branch had gone CONFLICTING. Three conflicts, all resolved on merit rather than by taking a side:
Re-verified on the rebased head: 80 Python tests pass (`test_desktop_connection_mode.py`, `test_desktop_connection_mode_env.py`, `test_mcp_connection_mode_meta.py`, `test_desktop_connection_mode_rpc.py`), changed-file Windows footgun check clean, and CI is green including `apps/desktop / check:test:desktop:all` and `check:lint`. Now MERGEABLE. |
3702a4b to
13a1219
Compare
|
Rebased onto 1. 2. Worth flagging for review rather than silently resolving: 3. Resolved by folding if (key !== g.primaryProfile) {
if (await sharedPrimaryRoute(key)) {
return () => setActive(g.primaryProfile)
}
const entry = g.secondaries.get(key) ?? createSecondary(key)
...
}
return () => setActive(key)Two details that were easy to get wrong here:
Verification. 502 |
|
Following up on the @andrexibiza item 3 of your follow-up review asked for "one shared request helper" across hook and non-hook/plugin callers.
const wrapped = new Proxy(gateway, {
get(target, prop) {
if (prop === 'request') {
return <T>(method: string, params: Record<string, unknown> = {}): Promise<T> =>
target.request<T>(method, announceConnectionMode(method, params))
}
const value = Reflect.get(target, prop)
return typeof value === 'function' ? value.bind(target) : value
}
})Three choices worth surfacing, since each had a wrong-looking-right alternative:
I checked before wrapping that nothing in-tree relies on reference equality against 5 new tests in If you would rather Status otherwise: rebased onto |
4d16de3 to
b1328c2
Compare
|
Rebased onto Conflict resolutions1. 2. 3. The thing the rebase surfaced
await ensureGatewayForAgent(connection, target) // $gateway flips here
$activeGatewayProfile.set(target)
await syncConnectionToActiveAgent(connection, target) // $connection flips hereThat trailing Closed in
One divergence I did not resolve unilaterally, because it is your call. The two paths still disagree on descriptor-lookup failure. The profile path aborts the whole switch ("rather than activating a backend whose descriptor, and thus mode, is unknown"). The agent path is best-effort: it publishes the gateway and profile and leaves the previous A pre-existing breakage this repaired
TestsNew: This machine has no |
|
Request changes: the agent activation path must fail closed on descriptor lookup failure. I re-read the exact 6418cd4 path after the rebase. The pending-descriptor race is fixed, but the failure path still allows the same mixed state to persist after activation. ensureGatewayProfile() now has the right contract: descriptor resolution and gateway preparation happen before publication, and a rejected descriptor lookup aborts the switch as a unit. Nothing advances. ensureGatewayAgent() does not currently have that contract. resolveConnectionForActiveAgent() catches getConnectionFor() failures and converts them to null. That means: const [descriptor, activate] = await Promise.all([ can resolve successfully as [null, activate]. The code then still executes: activate() and only skips: if (descriptor) { So descriptor failure advances the active gateway and profile while $connection remains the descriptor for the previous backend. That is not merely the old async race window; the inconsistent state survives after ensureGatewayAgent() returns until some later reconnect or switch happens to repair it. That reopens the exact invariant this PR is intended to establish: code making a plugin, MEDIA:, filesystem, or connection-mode decision can observe the new backend paired with the old descriptor. Please make the agent path fail closed, matching the profile path. The cleanest version is to stop converting a getConnectionFor() rejection into null. Let descriptor lookup failure reject, then catch the whole switch exactly as ensureGatewayProfile() does so no publication occurs. If null must remain meaningful for a genuinely unsupported/no-bridge case, distinguish that explicitly from lookup failure rather than collapsing both states together. I would also split the regression coverage into the two distinct contracts:
The existing never publishes the agent gateway before its connection descriptor test covers the first case. The missing rejected-descriptor test is the one that should fail against the current implementation. One wording nit only: I would describe the success path as publishing with “no asynchronous gap” rather than as a transactional “single frame,” since these are still sequential synchronous atom writes. That does not affect the blocker above. CI is green on 6418cd4, including the Desktop suites and required aggregate checks. So the remaining issue is specifically the untested descriptor-rejection path, not the rebase or general test health. |
|
@andrexibiza you were right, and it was the more interesting half of the bug. Fixed in The failure path published anyway. It is worse than the race it sits next to, which is what changed my mind about calling it best-effort. The pending-descriptor window closes when the descriptor arrives. A failed lookup never arrives, so The fix is to let the rejection propagate, which is not a new contract but the one On the test. One property I accepted rather than fixed, since it is worth naming explicitly rather than leaving for someone to discover: a rejected switch leaves the socket Took the wording nit as written. Also reworded the two publication comments: they are sequential atom writes with no asynchronous gap between them, and calling that "one synchronous publication frame" overstated it — the guarantee is that nothing can observe a partial publication because nothing else runs between the writes, not that the writes are a transaction. CI is green (57 checks, 0 failing), mergeable, rebased on current |
session.resume's fast path returns an already-live session without touching it, so a client that switched connection or profile since that session was registered kept announcing into a stale stored mode until its next prompt.submit. prompt.submit still refreshes before any turn runs, so nothing acted on the stale value -- this just closes the window between reopening a chat and sending in it. Goal: 001-desktop-connection-mode (deliverable 7)
…urn isolation With dashboard turn isolation enabled, the compute-host child rebuilds the Desktop session from the turn.start frame, which did not carry the resolved connection mode. _set_session_context therefore bound None, and skill subprocesses, MCP per-call _meta, and desktop_connection_mode() all lost the announcement for the whole isolated turn. - _compute_host_turn_frame now sends the parent's resolved mode (None for non-Desktop sessions, same gating as every other read path) - _ensure_server_session applies it on create (_init_session kwarg and the minimal fallback session) and refreshes it on reuse, so a mid-session connection switch lands on the next isolated turn; an omitted key from an older parent leaves the stored mode alone - fixes a latent AttributeError in the fallback path, which referenced a server._sanitize_client_source that never existed; it now uses _resolve_session_source, matching _init_session - regression tests cover the frame, both create paths, reuse-with-switch observed through the child's own context bind, and the omitted-key case Addresses the blocking review on NousResearch#82187.
…review agents prompt.background and preview.restart bind fresh bg_*/preview_* task IDs that are not in _sessions, so the lookup-based derivation in _set_session_context found nothing and the detached agent ran the whole task with no mode: no Python accessor value, no subprocess env stamp, no MCP per-call _meta. _set_session_context now takes an explicit connection_mode keyword (sentinel default keeps the session-map derivation for every other caller, and an explicit None is honored rather than second-guessed), and both ephemeral-agent handlers pass the parent session's resolved mode. Regression tests read all three surfaces inside the detached agent thread for both handlers, plus the non-Desktop-parent and explicit-None cases.
…s env factory The !`cmd` expansion in skill preprocessing called subprocess.run() with no env at all, so unlike every terminal/tool spawn the snippet got the raw process environment: no session-context stamps, no write-only Desktop connection-mode stamp, and no scrub of a HERMES_DESKTOP_CONNECTION_MODE value inherited from the user's shell. It now builds the child env with build_subprocess_env(), the same factory as every other spawn surface (best-effort: a factory failure falls back to inheriting, matching the tool's degraded paths). Tests pin: an env is supplied; a bound mode is stamped; a live remote ContextVar overrides an ambient shell value; and an engaged session context with no bound mode strips the inherited variable.
The plugin SDK's host.request sent straight through $gateway.get().request(), bypassing the withConnectionMode stamp that useGatewayRequest applies, so a runtime plugin could create or drive a Desktop session whose skills/MCP context never learned the mode even while ctx.connection.mode() reported remote. announceConnectionMode() in lib/connection-mode is now the one shared announcement helper: it reads the live $connection at call time and stamps session.create / session.resume / prompt.submit. Both the hook and host.request go through it. Tests drive all three stamped methods through host.request, plus the unrelated-RPC, unknown-mode, and no-gateway cases.
…mically on a profile switch ensureGatewayProfile used to activate the target gateway and set $activeGatewayProfile while the connection descriptor fetch was still in flight, so during that window $gateway already targeted the new backend while $connection still described the previous one, and any request or plugin mode-listener firing then announced the wrong mode to the new backend. A failed descriptor fetch made the mismatch permanent. prepareGatewayForProfile (new gateway-store seam) opens the socket and returns a synchronous activation thunk without publishing anything; ensureGatewayForProfile now delegates to it. The switch resolves the descriptor and opens the socket first, then flips the active gateway, the profile atom, and $connection in one synchronous frame. A descriptor failure aborts the switch as a unit: nothing is published and every atom still consistently describes the previous profile. The deferred-descriptor test holds the fetch open and asserts the public atoms never disagree, then releases it and asserts all three flipped together; the failure test asserts no partial publication.
ctx.connection.onModeChange invoked plugin callbacks bare, both for the immediate notification and from the $connection subscription. The subscription runs inside core setConnection (boot, reconnect, profile switches), so one throwing plugin listener could abort profile synchronization/reconnect code and starve sibling listeners. Each callback invocation is now wrapped and reported with the plugin id attribution, matching the containment contract gateway event listeners already have. Tests cover a throw from the immediate call, a throw on a real transition (sibling listeners keep running, the thrower stays subscribed), and the attribution in the reported error.
… mcp SDK The published example used @server.call_tool() as a decorator and read context.meta; with this tree's mcp==1.28.1, call_tool is the dispatch method (self, name, arguments), not a decorator factory, and Context has no meta attribute. The supported shape is a @server.tool() handler reading ctx.request_context.meta, where the namespaced key lands in the metadata model's extra fields (model_extra). A docs smoke test extracts the snippet from the page and executes it against the installed SDK (decorator registration inspects the handler signature, so drift fails loudly), verifies the documented meta access on the real RequestParams.Meta model, and pins the absence of the old shapes so an SDK bump prompts a docs revisit.
`main` added `host.getGateway()` after the follow-up review, handing
plugins the live `HermesGateway` for SDK components that take it as a
prop. It is also the SDK's second request door: a plugin reaching
`getGateway().request('prompt.submit', ...)` bypassed
`announceConnectionMode` and could drive a Desktop session whose
skills/MCP context never learned the mode.
That is the same gap item 3 of the follow-up review closed for
`host.request`, reopened through a different door, so close it the way
the review asked: every gateway-request door shares one announcement
helper.
Wrapped in a Proxy rather than a spread copy or a subclass, because
`HermesGateway` is the live socket wrapper and its methods close over
connection state that only exists on the real instance. Every member
except `request` delegates straight through, bound to the target so a
delegated method never runs with the proxy as `this`. Wrappers are
cached per real gateway in a WeakMap so repeated calls hand back a
stable reference, which matters because SDK components take this as a
React prop and a fresh wrapper per render would churn every memo and
effect dependency keyed on it.
5 tests: announcement on a stamped RPC, pass-through on an unstamped
one, delegation of non-request members, reference stability, and the
null-before-first-socket case.
`ensureGatewayAgent` is the (connectionId, profile) door the SDK's `ensureAgent`
goes through, and it landed on main after the profile path was made atomic. It
published in the order the profile path used to:
await ensureGatewayForAgent(connection, target) // $gateway flips here
$activeGatewayProfile.set(target)
await syncConnectionToActiveAgent(connection, target) // $connection here
The trailing await is the same mixed-state window: $gateway and
$activeGatewayProfile already name the agent's backend while $connection still
describes the previous one, so any request or plugin mode-listener firing in
that window announces the wrong mode to the new backend.
Both doors now share one seam:
* `prepareGatewayForAgent` mirrors `prepareGatewayForProfile`: dial the socket,
publish nothing, return the synchronous activation thunk. A local/null
connection falls through to the profile seam, so the two paths cannot drift.
`ensureGatewayForAgent` becomes `(await prepareGatewayForAgent(...))()`,
exactly how `ensureGatewayForProfile` relates to its own prepare.
* `syncConnectionToActiveAgent` splits into `resolveConnectionForActiveAgent`,
which resolves only. `ensureGatewayAgent` resolves the descriptor and dials
the socket concurrently, then activates, moves the profile pointer and sets
the descriptor with no awaits between them.
The best-effort contract on this path is unchanged on purpose: a descriptor
lookup that fails still leaves the previous `$connection` in place rather than
aborting the switch, which is what the profile path does instead. That
difference is deliberate and called out for review rather than quietly
harmonised.
Tests: `profile-agent-activation.test.ts` gains
`never publishes the agent gateway before its connection descriptor`, the mirror
of the profile-path test, asserting a pending `getConnectionFor` leaves all
three atoms on the old backend and that they flip together once it resolves. The
existing mutex and resync tests move onto the prepare/publish mocks, which also
repairs them: that file mocked `@/store/gateway` without `prepareGatewayForProfile`,
so its profile-path cases called an undefined mock after the rebase.
… rejects Review caught that the agent path fixed the pending-descriptor race but not the failure path. `resolveConnectionForActiveAgent` caught a `getConnectionFor` rejection and returned null, so `Promise.all` resolved as `[null, activate]` and the switch published anyway: the activation thunk ran and `$activeGatewayProfile` advanced, while only `setConnection` was skipped. That is the same mixed state this PR exists to remove, except it does not close on its own. The pending-descriptor window ends when the descriptor arrives; a failed lookup never arrives, so `$gateway` named the new backend while `$connection` described the old one until an unrelated reconnect or switch happened to repair it. Anything branching on connection mode in between (plugins, `MEDIA:`, `/api/fs/*`, `/api/media`, image attach) saw the pair disagree. Let the rejection propagate, matching `resolveConnectionForProfile`, whose contract is already exactly this: null means "no desktop bridge" and nothing else, and a bridge rejection aborts the whole switch before anything is published. Both doors now fail closed identically, and the caller can retry. The existing "leaves the prior connection intact when the descriptor fetch fails" test asserted the old best-effort behaviour, so it pinned the defect rather than a contract worth keeping. Replaced with a rejected-descriptor test that asserts none of the three atoms moved and the activation thunk was never called. The pending-descriptor case keeps its own separate test, so the success and failure contracts are pinned independently. Also reworded the publication comments: these are sequential atom writes with no asynchronous gap between them, not a transaction, and describing them as one "frame" overstated the guarantee.
The prepare/publish seam removed the *await* between activating the gateway and setting the profile pointer and connection descriptor, but not the *notification* gap. Nanostores drains a store's listeners synchronously inside .set(), so three sequential sets still let a $gateway listener run while $activeGatewayProfile and $connection named the previous backend. That is the same mixed state the seam exists to prevent, just narrowed from an async window to a synchronous one, and it is worse to debug because it is invisible in an await-shaped reading of the code. batch() defers every notification to the end of the callback, so the three become one observable transition on both the profile path and the agent path. Pinned with a test that attaches a real $gateway listener and asserts the companions are already current in the first callback; the mock thunks now publish distinct gateway identities so an out-of-order publication cannot pass unnoticed, and three existing tests assert $gateway is still the ORIGINAL object (by identity) on every path that must publish nothing.
The announcing Proxy wrapped HermesGateway.request with a two-argument function, so a plugin calling getGateway().request(method, params, timeoutMs, signal) silently lost both its custom deadline and its ability to abort. A wrapper must not narrow the contract it stands in for, and this one narrowed it invisibly: the call still succeeded, it just could never be cancelled and always used the default timeout. The tail is forwarded as a rest spread rather than two named parameters so the delegated call carries exactly the arguments the caller made. Naming them re-materializes omitted arguments as explicit undefined, which is invisible to a defaulted parameter but not to anything reading arguments.length, and it makes every pass-through call site un-assertable on its real shape. Two regressions: one asserting the timeout and the AbortSignal reach the underlying request (including signal IDENTITY, not just presence) on a stamped RPC, and one asserting the same on an RPC this door does not stamp.
run_inline_shell fell back to env=None when build_subprocess_env could not be imported or raised. subprocess.run(env=None) inherits the RAW parent environment, which is the one door the scrub exists to shut: an ambient HERMES_DESKTOP_CONNECTION_MODE=local set in the user's shell reaches the snippet verbatim and is read as the resolved Desktop mode. So the error branch was strictly more permissive than the success branch, and it was the branch nobody would look at. Refusing to run the snippet is the safe outcome and costs nothing the caller cannot absorb: it already treats an [inline-shell error: ...] marker as a non-fatal result, so one skipped snippet degrades the skill message instead of silently handing it a spoofable environment. The regression asserts the strongest available property, that the child never existed, and fails against the old code with the leaked env printed in the message.
withConnectionMode honored a caller-supplied connection_mode and skipped the stamp when the live mode was unresolved. Both are spoofing paths, and the plugin SDK's host.request reaches this same choke point: - A plugin driving a live REMOTE session could pass connection_mode: 'local' and have the backend hand its skills and MCP context paths as though they were on the user's machine. Only the renderer can see the descriptor, so only the renderer may answer. - Omitting the key on an unresolved mode is not neutral. _remember_connection_mode (server.py) only writes when the key is PRESENT, so omission means "keep what you have": a 'local' announced before a reconnect survived into turns that could no longer prove it. An explicit null clears it, because normalize_desktop_connection_mode(None) is None. The field is now reserved: whatever the caller put there is discarded and the live resolved mode, null included, is written in its place. Four regressions replace the caller-wins test, covering each direction: announcing null when unknown, overriding a caller value with the live mode, refusing to let a caller 'local' survive an unknown mode, and clearing a previously announced 'local'.
The agent path already declined to publish when applyActive() rejected its activation, but the profile path discarded the same boolean and published unconditionally. applyActive() returns false when its captured epoch has been superseded, which happens whenever a newer switch or a teardown lands while this preparation is still awaiting its route or socket. The result was not a torn publication. batch() makes those writes observer-atomic either way. It was something subtler: ONE complete, internally inconsistent tuple, the CURRENT gateway paired with the stale target's profile pointer and descriptor. Atomicity cannot make a rejected activation correct, so the caller has to decline to publish at all. prepareGatewayForProfile now returns Promise<() => boolean> like its agent counterpart. The primary and shared-primary thunks return applyActive() directly; the secondary thunk reports whether the prepared entry was still current AND the epoch was accepted, keeping the descriptor publish conditional on having a cached connection so an accepted activation with no descriptor still moves the companions. prepareGatewayForAgent's genuinely-local fallthrough now returns the profile thunk unchanged instead of wrapping it to return an unconditional true, which had been reporting a rejected activation to the agent caller as a successful one. Two regressions on the profile door: a superseded activation leaves all three stores on the existing complete route with no subscriber notified at all, and an accepted one still publishes, so a thunk that always reported false could not pass. The mock thunks in profile.test.ts now return true, since a bare vi.fn() returns undefined and would read as "superseded".
The comment above ensureGatewayAgent carried both the old and the new contract on consecutive lines: "a local/null connectionId falls through to the profile path verbatim", immediately contradicted by "only a null connectionId falls through, explicit local is a registry identity". Dropped the stale line. Same wording above prepareGatewayForAgent in gateway.ts, tightened to match what the code actually does: registryBackendScopeKey only collapses to the bare profile key for a null or empty id, so an explicit local id scopes to conn:local::<profile> and stays on the registry route. Comments only, no behavior change. tsc --noEmit, eslint and the three affected suites (43 passed) re-verified. Refs NousResearch#82140
9826bbf to
22d27d8
Compare
|
Cleanup done at Dropped the stale line in On the export function registryBackendScopeKey(connectionId, profile) {
const profileKey = String(profile ?? '').trim() || 'default'
const connection = String(connectionId ?? '').trim()
return connection ? `conn:${connection}::${profileKey}` : profileKey
}Only a null or empty id collapses to the bare profile key, so an explicit Comments only, no behavior change, but re-verified anyway rather than assuming: Thanks for the reviews on this one. The epoch asymmetry in particular was mine to catch and I had left it as a note instead of a fix, so the push back was warranted. |
…ne (#82140) Assistant messages that link a file the agent wrote — [report](/home/user/report.md), file://…, ~/…, C:\… — rendered as dead anchors: file:// is blocked in the renderer, Streamdown's URL hardening turns file:/~/ hrefs into "[blocked]" spans, and on a remote gateway the path isn't on the viewer's disk at all. Issue #82140 proposed exposing the Desktop connection mode to skills/MCP/plugins so EXTENSIONS could emit different output per viewer; this fixes the symptom at the right layer instead — the viewer surface resolves paths at VIEW time, so extension output stays surface-agnostic and the same transcript works from every machine that opens it. - markdown-preprocess: routeFileLinksToPreview() rewrites filesystem-path links in prose to the renderer's existing hash-href doors — #preview/… (PreviewAttachment) for documents, #media:… for audio/video/image extensions. These pass URL hardening by design and resolve through normalizeOrLocalPreviewTarget / resolveMedia*Src: local connections read the file directly, remote connections fetch over the authenticated /api/fs bridge. Image syntax, fences, inline code, anchors, relative and http(s) links untouched. - markdown-text: MarkdownLink routes any filesystem href that still reaches it (bypassing preprocess) to PreviewAttachment/MediaAttachment instead of a bare dead <a>. - media.ts: export isFileMediaPath. Live E2E (built app, CDP-driven, fixture session with links to a real gateway-side file): - BEFORE: [report.md] = dead <a href="/home/…"> (click: nothing), [notes](file://…) = "notes [blocked]" span, 0 preview affordances. - AFTER: both render as attachment rows; Open preview shows the file's content in the preview pane; zero blocked spans; screenshots verified. Closes #82140. Supersedes PR #82187 (connection-mode API): with view-time resolution the extension layer no longer needs to know where the viewer sits.
|
Thank you for the substantial work here, @jackulau — and @andrexibiza for one of the most rigorous review threads this repo has seen. Closing this PR, but most of its value is landing; here's the full disposition. The symptom is fixed at a different layer. #82140's driving case — file links that are dead when Desktop views a remote gateway — is now solved by view-time resolution in the renderer (PR #89472, merged): filesystem-path links in chat route through the preview pipeline, which resolves against the session's backend at the moment the viewer clicks (local reads directly, remote fetches over the authenticated Why we went that way instead of the connection-mode API. The mode bit conditions what extensions emit, but transcripts outlive any single viewer: a path emitted "because the viewer was local" is dead again when the same session is later opened from another machine (or from Telegram). View-time resolution is correct for every viewer of the same transcript, keeps skills/MCP/plugin output surface-agnostic (no other platform gets a viewer-location bit), and needs no new gateway/extension API surface. Your design was careful — the contextvar-not-env decision, fail-to-None, per-turn re-announcement were all right given the layer — the layer itself is what we're declining. Your store-hardening work is being merged with your authorship. The seven desktop store commits this review produced (atomic batch publication of gateway/profile/connection, fail-closed activation on descriptor-lookup rejection, the symmetric profile-path guard, plus their regression tests) were real bugs independent of connection mode. They're cherry-picked with your commits intact in PR #89483. Fixes credited: #82140 closed via #89472; store hardening via #89483. |
|
Thanks for writing the disposition out rather than just closing it - and the layer argument is right on the merits, not just as a call. The specific thing that decides it: a viewer-location bit is captured at emit time and read at view time, and those are not the same event. A path emitted "because the viewer was local" is a fact about who was watching once, stored in a transcript that outlives them. So the mode bit is correct only for the first viewing, and silently wrong for every later one - opened from another machine, or from Telegram. View-time resolution has no such window because it re-answers the question at the moment it is asked. That is a stronger property than the one I was building toward, and it does not need the API surface at all. Nothing to re-litigate. Confirmed the salvage landed complete, since a seven-commit cherry-pick across a rebased branch is exactly where something goes missing: all seven are on @andrexibiza - the compute-host and background/preview-agent findings were the two that reshaped the PR rather than patched it, and the epoch/atomicity distinction in the later pass is what produced the commits that ended up merging. Thank you for the depth. |
🫡🫡 |
…ne (NousResearch#82140) Assistant messages that link a file the agent wrote — [report](/home/user/report.md), file://…, ~/…, C:\… — rendered as dead anchors: file:// is blocked in the renderer, Streamdown's URL hardening turns file:/~/ hrefs into "[blocked]" spans, and on a remote gateway the path isn't on the viewer's disk at all. Issue NousResearch#82140 proposed exposing the Desktop connection mode to skills/MCP/plugins so EXTENSIONS could emit different output per viewer; this fixes the symptom at the right layer instead — the viewer surface resolves paths at VIEW time, so extension output stays surface-agnostic and the same transcript works from every machine that opens it. - markdown-preprocess: routeFileLinksToPreview() rewrites filesystem-path links in prose to the renderer's existing hash-href doors — #preview/… (PreviewAttachment) for documents, #media:… for audio/video/image extensions. These pass URL hardening by design and resolve through normalizeOrLocalPreviewTarget / resolveMedia*Src: local connections read the file directly, remote connections fetch over the authenticated /api/fs bridge. Image syntax, fences, inline code, anchors, relative and http(s) links untouched. - markdown-text: MarkdownLink routes any filesystem href that still reaches it (bypassing preprocess) to PreviewAttachment/MediaAttachment instead of a bare dead <a>. - media.ts: export isFileMediaPath. Live E2E (built app, CDP-driven, fixture session with links to a real gateway-side file): - BEFORE: [report.md] = dead <a href="/home/…"> (click: nothing), [notes](file://…) = "notes [blocked]" span, 0 preview affordances. - AFTER: both render as attachment rows; Open preview shows the file's content in the preview pane; zero blocked spans; screenshots verified. Closes NousResearch#82140. Supersedes PR NousResearch#82187 (connection-mode API): with view-time resolution the extension layer no longer needs to know where the viewer sits.
4 participants
What does this PR do?
Exposes the resolved Desktop connection mode (
local/remote) to the three supported Hermes extension surfaces — skills, MCP servers, and Desktop plugins — as a documented API.Desktop already resolves this through
window.hermesDesktop.getConnection(), but extensions couldn't see it. Without it, an extension has no way to tell whether a gateway-side path like/home/user/report.mdis openable on the machine the user is actually looking at, which is what makesMEDIA:/file-link behavior ambiguous on remote gateways.Design, and why this shape:
_VAR_MAPsoget_session_env'sos.environfallback never applies. This is what satisfies the issue's "must not rely on a user-configurableHERMES_*environment variable" criterion: aHERMES_DESKTOP_CONNECTION_MODEexported in a shell is not read anywhere, and on the subprocess path it is actively stripped. A wronglocalis worse than no answer — it hands the user a link to a file that isn't on their machine — so unknown resolves toNone, never to a guess.session.create/session.resumeand on everyprompt.submit. The per-turn re-announcement is what makes switching the active connection or profile land immediately rather than being pinned to whatever was true when the chat opened. It is stamped at the singlerequestGatewaychoke point, not at the ~10 call sites, so a new session/prompt path announces by construction.source == 'desktop', so a strayconnection_modefrom the TUI or a messaging platform is ignored and every non-Desktop surface reports "unavailable".Related Issue
Fixes #82140
Type of Change
Changes Made
Core runtime value
gateway/session_context.py—_DESKTOP_CONNECTION_MODEcontextvar plusnormalize_desktop_connection_mode(),set_desktop_connection_mode(),desktop_connection_mode(). Reset inclear_session_vars/reset_session_varsalongside the other session vars, so a concurrent turn's mode is never inherited. Remote-shaped saved modes (cloud,ssh,url) normalize toremote.Wire-in (TUI gateway — the Desktop-facing RPC edge)
tui_gateway/server.py—_normalize_connection_mode_param,_remember_connection_mode,_session_connection_mode;connection_modeslot on both live-session record shapes; bound in_set_session_context.tui_gateway/methods_session.py— accepted onsession.createand all threesession.resumepaths; inherited bysession.branch.tui_gateway/methods_prompt.py— refreshed onprompt.submit, next to the existingclient_surfacerewrite (same rationale).Read path — skills
tools/environments/local.py— stampsHERMES_DESKTOP_CONNECTION_MODEonto subprocess environments, write-only: set when a mode is bound, popped otherwise, on every spawn.Read path — MCP servers
tools/mcp_tool.py— attaches_meta["hermes-agent.nousresearch.com/desktop-connection-mode"]tocall_toolrequests. A stdio server's env is fixed at spawn while the mode is per-session (one gateway can serve a local Desktop client and a remote one at once), so per-call_metais the only vehicle that is both live and session-correct. SDK support for per-callmetais probed rather than assumed, so an oldermcpkeeps today's request shape.Read path — Desktop plugins
apps/desktop/src/lib/connection-mode.ts(new) —resolveConnectionMode/withConnectionMode.apps/desktop/src/contrib/plugin.ts—ctx.connection.mode()andctx.connection.onModeChange(). Reads the live$connectionatom rather than callinggetConnection()directly, because the atom is what stays in lockstep with the active profile; a raw bridge call describes the primary window backend, which is the wrong answer whenever a background profile is active. Only real transitions are forwarded, so a reconnect that re-mints the descriptor on the same mode doesn't wake every listener. The subscription is registered with the plugin's disposers, so a plugin that ignores the returned unsubscribe still stops listening on unload.apps/desktop/src/app/gateway/hooks/use-gateway-request.ts— stampsconnection_modeon the RPCs that carry it.apps/desktop/src/sdk/index.ts— re-exports the new types for@hermes/plugin-sdk.Docs
website/docs/developer-guide/desktop-connection-mode.md(new) + sidebar entry — all three read paths, what each returns when there's no Desktop session, why the source of truth isn't an env var, and the security posture.How to Test
Automated
New coverage (57 Python tests + 21 TS tests):
tests/gateway/test_desktop_connection_mode.py— normalization, task-locality across two concurrent Desktop clients on one gateway, and that a user-set env var is neither a source of truth nor an override.tests/tui_gateway/test_desktop_connection_mode_rpc.py— storage, the per-turn refresh, non-Desktop sources ignoring a stray param, and the omitted-param case (an older client must not erase a newer one's announcement).tests/tools/test_desktop_connection_mode_env.py— the write-only stamp, including that an inherited/stale shell value is stripped rather than honored.tests/tools/test_mcp_connection_mode_meta.py—_metacontent, the "mode and nothing else" assertion, and the SDK capability probe in both directions.apps/desktop/src/lib/connection-mode.test.ts,apps/desktop/src/contrib/plugin.test.ts— resolution, param stamping, and the plugin door (immediate fire, transitions only, disposal).Manual
echo $HERMES_DESKTOP_CONNECTION_MODE→local.remote(this is the per-turn refresh; no new session needed).hermes chatin a terminal and check the same variable → unset.export HERMES_DESKTOP_CONNECTION_MODE=localin your shell before launching, then repeat step 3 → still unset (the stamp is write-only).Platforms tested: Windows 11 (Python 3.13 suite + desktop vitest +
check-windows-footguns.py). No platform-specific code paths were added — the change is contextvars, dict fields, and TypeScript.Checklist
Code
Documentation & Housekeeping
website/docs/developer-guide/desktop-connection-mode.md+ docstrings)CONTRIBUTING.md/AGENTS.mdcheck-windows-footguns.pyclean; no OS-specific paths added