fix(desktop): fresh managed-SSH spawns no longer wedge on dash or double-quoted paths (salvage #96061) - #96084
Merged
Conversation
…80921) Deterministic, LLM-free conformance cells against the real SessionDB with real SIGKILL mid-write, per the tracking issue's spot-probe method: - cell 1: acknowledged-append durability + recovery determinism (adapted from the issue's 29.5K probe, scaled kill window, identical assertions) - cell 2: consume-once under 8-process concurrent claim_handoff - cell 3 (new): compression-rotation atomicity — never a compression-ended parent without a continuation (#80337 contract; #80487 recovery context) - cells 4-5: documented stubs interlocked with #82956-#82959 and #83197/#83557 Journal-mode matrix (resolver default / DELETE / WAL-with-skip-gate) per cell; every wait deadline-bounded; writers asserted alive at kill time.
…ld resolver, audit the effective mode Salvage round on the Phase 1 skeleton (three findings from review): 1. The DELETE leg was silently upgraded to WAL on healthy SQLite: pre-seeding the file via PRAGMA was undone by SessionDB.__init__'s apply_wal_with_fallback(), which upgrades any non-WAL file whenever the configured mode (default wal) says so — only WAL-reset-vulnerable interpreters preserved DELETE, i.e. the leg tested the advertised mode only where CI wasn't running. Each matrix leg now pins database.journal_mode in an isolated HERMES_HOME for the child and audits the ON-DISK mode after the run (effective_mode_or_skip): a leg that ran in a different mode skips instead of double-counting. 2. Cell 2 exit-code conflation: a claimant crashing with an unhandled exception exits 1 — indistinguishable from the clean "lost the claim" exit(1), so one winner + seven crashes passed as consume-once proof. Codes are now disjoint (0=won, 10=lost, anything else=crash). 3. Crashed-writer diagnostics: wait_for now fails immediately with the child's stderr when the writer dies before reaching the kill window (was: 60s opaque deadline, stderr discarded). spawn_child prepends to an inherited PYTHONPATH instead of clobbering it. Plus: dead `if False` scaffolding removed from cell 1's writer; README matrix section corrected (cell 2 is default-mode-only by design — the consume-once property rests on a single predicated UPDATE).
…Error on macOS The _ConcurrentToolAuthorizationGate uses threading.Lock.acquire(timeout=...) where timeout comes from human_wait_ceiling() (approvals.timeout + 60s). When approvals.timeout is set very large (e.g. 999999999999 to effectively disable timeouts), this overflows macOS timespec and raises: OverflowError: timestamp out of range for platform time_t The gate only needs to serialize parallel dispatch — it should never wait longer than the wedged-holder bound (_AUTHORIZATION_GATE_LOCK_TIMEOUT_S = 360s). Clamp the return value with min() so unbounded approvals.timeout cannot overflow the lock acquire. Fixes #83220
…83220) Widen the salvaged gate-site clamp to the bug class. The gate min() from the contributor PR capped the gate bound at 360s, which would break the #79719 contract (gate must extend while a legitimate >360s approval prompt is answerable) and left the sibling overflow sites live: the CLI prompt thread.join, the gateway poll deadline, and human_wait_ceiling all consume the same config value. Clamp once in _get_approval_timeout() via agent.deadline.MAX_SAFE_TIMEOUT_S (1 year - semantically unbounded, platform-safe). The gate keeps its approvals.timeout-tracking behavior above 360s; 7 regression tests pin lock-acquire/thread-join safety and the gate-extension contract. Bilateral E2E: with approvals.timeout=1e20 in a real config.yaml, main crashes every consumer with OverflowError; this branch survives all 5 probes.
…og clamp engagement Review round (#86412): - the except-import fallback returned the RAW oversized value, re-opening the exact macOS time_t overflow this fix prevents; it now fails closed to a finite ~1-year cap matching agent.deadline.MAX_SAFE_TIMEOUT_S - clamp engagement logs a WARNING so operators see the semantic change - new tests: float-form oversized value (YAML 1e18), warning emission, and the import-failure fail-closed path (blocked-import probe proving the result stays Lock.acquire-safe)
…top boot path The desktop app spawns `hermes serve --ws-only` instead of `hermes serve`. The slim server (tui_gateway/entry_ws.py) uses the bare websockets library to call handle_ws directly — same dispatch surface, same 158 RPCs, same events, zero HTTP framework. tui_gateway.server has zero FastAPI imports; the slim server never touches web_server.py (19.8K lines) at all. Full backward compat: older runtimes strip --ws-only and fall back to regular serve (still headless, just through uvicorn).
…he whole function
…dout to stderr at module level
…close, real v15 handshake tests Review F1: --ws-only is now OPT-IN (HERMES_DESKTOP_WS_ONLY=1). The slim server has no HTTP routes while the desktop's hermes:api REST plane only speaks http — shipping it on-by-default made every REST call fail with 'Unsupported Hermes backend URL protocol: ws:'. The descriptor keeps baseUrl http-only and flags the transport via wsOnlyTransport instead of overloading baseUrl. Default flips after the REST consumers migrate to JSON-RPC (#94484 phase 3). Review F3: entry_ws.run() fails closed on non-loopback hosts — the static query-token auth is the desktop loopback shortcut, not a posture fit for network exposure (no gated one-time-ticket auth). Review F2 (test half): tests/test_tui_gateway_entry_ws.py drives a REAL websockets-15.0.1 server + sync client through the actual handshake — valid token accepted (path read from the v15 ServerConnection), missing/ wrong token rejected with 4401, plus the loopback guard both ways.
…eq-namespace epoch Chat/event-plane quality work for the amended Phase 1 scope of #94484 (maintainer restructure: lean chat/event plane, no control-plane changes). Three fixes came out of a source-level comparison against OpenHands, Chainlit, VS Code, Zed, LangGraph, and Goose. 1. Per-turn trace_id + active-turn telemetry: _start_inflight_turn mints a 12-hex trace_id; _event_frame stamps it on every event frame in the turn, so a client can correlate the full lifecycle (dispatch -> first token -> tool calls -> complete) from one identifier — none of the six surveyed projects has frame-level turn correlation. session.events.stats now reports active_turns (session_id, trace_id, elapsed_s, streaming). 2. Transient vs durable events (OpenHands StreamingDeltaEvent pattern): message.delta / thinking.delta are stamped with seqs (live ordering holds) but never buffered — one streaming turn emitted hundreds of delta frames and evicted every durable control event from the 512-slot ring, defeating replay for the exact reconnect window it exists to cover. The ring now evicts manually and records the highest DURABLE seq dropped, so truncated means real data loss and delta-only gaps no longer false-positive. 3. Seq-namespace epoch (Goose stale-cursor recovery): event_replay.EPOCH (8-hex per boot) is announced in gateway.ready and echoed by session.events.since; the client drops its seq watermarks when the epoch changes, so a stale HIGH watermark from a previous gateway process can no longer suppress replay/gap-detection forever. Legacy backends without an epoch are unaffected (client keeps watermarks). Validation: Python 85/85 across replay/entry_ws/keepalive/protocol (12 replay tests, 3 new); vitest 9/9 shared (2 new epoch tests), 66/66 desktop; full tui_gateway sweep 614/615 (1 known ordering flake, passes in isolation). Live e2e on this tree: 12-event turn -> seqs contiguous 1..12, 8 durable frames buffered + 4 deltas live-only, single trace_id on all frames, active_turns elapsed_s matches the real turn duration. Research provenance: #94484 (comparative-scan comment); techniques credited to OpenHands (transient split), Goose (epoch/stale-cursor), per maintainer-restructured plan.
Contributor
૮ >ﻌ< ა ci reviewran on 542736a — desktop: fix two managed-SSH-spawn bugs that break every fre
|
alt-glitch
added
type/bug
… "voice", and a User-Agent for CDN downloads (#95274) * fix(relay): map wire media[] → event.media_types; accept message_type voice A relayed voice note arrived as MessageType.AUDIO with media_types=[] — the STT gate (_event_media_is_stt_input) excludes AUDIO unconditionally and its per-attachment MIME rescue was unreachable, so STT never fired and the agent fell back to the "user sent an audio file attachment" context note (live-verified on staging 2026-08-26, Discord + Telegram). Two wire-boundary fixes, both additive within contract_version 1: - "voice" parses to MessageType.VOICE: the enum already had it — pinned by test so a future refactor can't collapse the two. - media[] is now mapped into event.media_types (positional alignment with media_urls; mime-less entries keep their slot as ""). This is what run.py's per-attachment classifiers key off, so EVERY relayed attachment — image vs document, audio vs voice — now routes like its native-adapter equivalent, not just voice notes. Behaviour pinned: new-connector voice → STT-eligible; legacy audio-typed events unchanged (no STT); music uploads never STT-eligible (direct _event_media_is_stt_input assertions on real wire-parsed events, not mocks). Pairs with the gateway-gateway PR that puts "voice" on the wire. * review: pin the STT gate by test; fail safe on media/media_urls mismatch Addresses independent review of #95274. 1. The PR's acceptance criterion is STT ROUTING, but no committed test called _event_media_is_stt_input — it was only asserted ad-hoc. Adds TestSttGate: voice→eligible, voice-without-media_types→eligible (the new-connector/old-gateway shape), legacy audio-typed voice note→not eligible, music→not eligible. Mutation-verified: removing the VOICE branch from the gate turns these RED. 2. media_urls and media[] are INDEPENDENT wire fields that consumers index by the same i. Mapping MIMEs positionally without checking agreement means a disagreeing producer misassociates a MIME with the wrong URL and mis-routes that attachment — strictly worse than no MIME, which degrades safely to message-level classification. _media_types_from_wire() now maps only when the lengths agree, warns and returns [] otherwise. Note for the record: MessageType.VOICE predates this PR and the gate's VOICE branch ignores media_types, so a NEW connector against an OLD gateway ALREADY fires STT. That is desirable, but it is not "unchanged" — the PR body's rollout matrix said otherwise and is corrected. * fix(relay): send a User-Agent on relay media requests (Discord CDN 403) Discord's CDN rejects urllib's default "Python-urllib/x.y" User-Agent with HTTP 403, and RelayMediaClient never set one. Every Discord CDN pass-through download therefore failed; _localize_inbound_media then kept the raw URL (its "a public URL still has value" branch), and the consumer tried to open a URL as a FILE PATH: WARNING gateway.relay.media: relay media download failed for https://cdn.discordapp.com/...voice-message.ogg: HTTP Error 403 INFO gateway.run: Voice transcription failed for https://cdn.discord... : Audio file not found: https://cdn.discordapp.com/... This killed ALL Discord relay media inbound — voice notes, images and documents alike — not just the voice lane. Telegram/WhatsApp were unaffected because their media is connector-re-hosted (/relay/media/{id}, fetched from our own host) and localizes to real /tmp paths. Reproduced from a clean shell against a live CDN URL: curl (own UA) -> 200 urllib, no UA -> 403 Forbidden urllib + descriptive UA -> 200, 14583 bytes, OggS magic Fix: a module-level _MEDIA_USER_AGENT sent on both download() and upload(). upload() only ever targets our own connector so it was not broken, but a single client should identify itself consistently. Validated on staging: hot-patched hermes-agent-stg-test-6698, restarted the gateway service, and Ben's Discord voice note transcribed successfully — zero new 403s and zero new transcription failures after the patch (last 403 predates it). Test is mutation-verified: removing the UA from download() turns it RED while the other five media tests stay green. * fix(relay): keep url↔mime pairing through media localization Addresses a blocking review finding on my own change: mapping media[] into media_types created a POSITIONAL contract that the rest of the inbound path then broke. 1. _localize_inbound_media (adapter.py) filtered media_urls without filtering media_types. Dropping a dead connector re-host is a NORMAL best-effort path, so every surviving attachment inherited its neighbour's mime. Reproduced through the real functions: before urls [.../relay/media/dead, .../kept.png] types [application/pdf, image/png] after urls [.../kept.png] types [application/pdf, image/png] <-- PNG reads as PDF _event_media_is_image(ev, 0) -> False The loop now carries (url, mime) as PAIRS, so a dropped URL drops its mime with it. 2. _media_types_from_wire compared LENGTHS only, which is not alignment: equal-length-but-reordered wire fields were accepted and paired wrongly, and an absent media_urls skipped the check entirely while still emitting types. Resolution is now BY URL (url -> mime lookup over media_urls); an unmatched URL degrades to "" and falls back to message-level classification. Tests: 4 new cases driving the real chain (wire parse -> localization -> run.py classifier), incl. the dropped-first-attachment case the existing localization test could not catch (it builds events without media_types). The obsolete length-mismatch test now asserts the stronger by-url guarantee. Both fixes mutation-verified: reinstating the URL-only filter fails 1 test, reverting to positional resolution fails 3. Relay suite 258 passed; media/voice/stt selection 685 passed; ruff clean; cross-repo integration payload re-verified. * fix(relay): media_types is always one slot per media_url Self-review after two review rounds flagged this bug class in adjacent seams: I checked the function I edited, not every consumer of the parallel arrays I created. Grepping ALL writers found a third instance. merge_pending_message_event (gateway/platforms/base.py:2725-2735) EXTENDS media_urls and media_types together when a second media message merges into a pending one. My mapping could emit a POPULATED media_urls with an EMPTY media_types (an older connector sends media_urls but no media[]), so extend() concatenated lists of different lengths: A urls [old1.png, old2.png] types [] B urls [new.pdf] types [application/pdf] merged urls [old1.png, old2.png, new.pdf] types [application/pdf] -> old1.png reads as application/pdf; the real PDF gets '' Fix: media_types is now ALWAYS len(media_urls), padded with '' — the url-keyed lookup runs even when media[] is absent, and the localizer rewrites the list unconditionally (no short-circuit that could leave a stale/short list behind). Tests: 4 new cases — padding with no media[], the merge shift above driven through the real merge_pending_message_event, localization preserving the invariant while dropping an entry, and normalization of a short/empty media_types arriving from a non-wire source. All mutation-verified: removing the padding fails 4; restoring the guard fails 1. Relay 262 passed; media/voice/stt selection 689 passed; ruff clean; cross-repo integration payload re-verified.
6 tasks
…e metadata (gateway-directed) Relay-fronted Slack reads platforms.relay.extra.slack.unfurl_links/unfurl_media and stamps explicit booleans onto the frame metadata; the connector forwards them to chat.postMessage with no config of its own (mirrors reply_in_thread). Covers send, send_for_platform (cron/scheduled), and send_media lanes.
Live staging (Coatue Slack): - hermes config set / Railway knobs persist "true" as a string; bots that omit unfurl_links do NOT inherit the human default, so dropping the string looked like suppression. - chat.startStream cannot carry unfurl_*. Native SlackAdapter already falls back to chat.postMessage; the relay now matches.
Relay-plane parity: hermes config set / Railway persist YAML booleans as strings, and _slack_unfurl_kwargs silently dropped them — so 'unfurl_links: "false"' was a no-op on native while working on relay. Coerce recognized string booleans exactly as _slack_unfurl_hints does; unrecognized values still drop so junk config keeps Slack's default instead of accidentally suppressing previews. Replaces test_send_ignores_non_boolean_unfurl_options (which froze the dropped-string behavior) with coercion + junk-drop tests.
The send and send_media lanes resolved the platform only from _platform_by_chat, which is empty until an inbound frame arrives (e.g. after a gateway restart). A proactive send to a Slack chat then missed the unfurl stamp. Mirror the streaming gate and delivery resolver: fall back to the negotiated descriptor's platform.
…knobs When either unfurl key is set, media captions post as a separate message before the file (the upload API cannot carry unfurl controls) and native draft streaming falls back to edit-based delivery. Surface both side effects in the config reference table.
The send_media lane (08b95c3) had no committed regression test: cover explicit-bool stamping on media frames, the descriptor-platform fallback when _platform_by_chat is empty (post-restart proactive sends), and the omitted-key absence case.
|
This PR fixes two of the three double-quoting sites enumerated in #96188, but // remote-lifecycle.ts:913 (main) — unchanged by this PR
return `python3 -c ${shq(script)} ${shq(mutexPath)} ${shq(command)}`Both call sites pass Confirmed on a Linux install by shimming Python's Worth separating from the boot livelock: on this machine the SSH backend boots fine ( One-line fix, consistent with the rest of this PR ( - return `python3 -c ${shq(script)} ${shq(mutexPath)} ${shq(command)}`
+ return `python3 -c ${shq(script)} ${mutexPath} ${shq(command)}`All 89 |
|
Confirming the - return `python3 -c ${shq(script)} ${shq(mutexPath)} ${shq(command)}`
+ return `python3 -c ${shq(script)} ${mutexPath} ${shq(command)}`The silent-failure framing matches what I observed: the stuck process and queued retries in #96188's report were a flock on a path under One supporting data point for the 'no regression coverage' note: the existing 89 remote-lifecycle tests pass both with and without the mutexPath change, so whichever PR lands this should add an argv-level assertion (e.g. render |
Contributor
|
Spent the last 2 hours struggling to get the remote gateway to work - this PR alongside #96189 helped resolve. |
alt-glitch
added
P3
22 tasks
|
@DanBennettUK Glad it unblocked you! For anyone else landing here: #96189's branch |
3 tasks
Contributor
|
@teknium1 Thanks for merging this. One POSIX spawn-publication follow-up from #96103 appears not to be included in the merged head. The merged command still has an outer So Would you like me to retarget/rebase #96103 onto current |
SZWzz
added a commit
to SZWzz/hermes-agent
that referenced
this pull request
Aug 28, 2026
…eger lockfile pid Follow-up to beb212d (NousResearch#96084), which fixed the reservation-site double quoting and the dash ${var//} bashism. Two defects in the same spawn machinery remain on main: 1. withRemoteUpdateMutex still wraps mutexPath in shq() even though both call sites pass expandRemotePath() output — an already-quoted shell word. The remote python then makedirs/flocks a bogus "$HOME/' tree (mirrors the absolute path, quote characters included) instead of the real sidecar: a mutex that silently serializes nothing. No boot symptom — boot succeeds while the stray tree grows on every spawn. Independently confirmed by shim-capturing argv from the real buildSpawnCommand output (see NousResearch#96084 discussion). 2. The POSIX sed placeholder swap introduced in NousResearch#96084 replaces the bare __PID__ inside the JSON-quoted "pid":"__PID__" template, so the initial lockfile carries a quoted-string pid. readLockfile requires Number.isInteger(pid) — a crash between the payload write and readiness leaves a malformed-pid record that fails closed (remote-lockfile-skew) on the next connect, and the in-payload reuse regex only matches "pid":<digits>. Replace the quoted placeholder instead. Adds regression coverage: an argv-level assertion that the mutex path is passed as exactly one shell word (the existing '.hermes-update-in-progress.mutex' substring assertion matches both the broken and fixed forms), and updates the POSIX-sh test to pin the quoted-placeholder sed form. All 93 remote-lifecycle unit tests pass; the rendered payload passes dash -n and the mutex argv verifies quote-free on a live fnOS (dash) remote.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fresh managed-SSH backend spawns work on stock Ubuntu again (salvage of #96061 by @SmelterLabs, commit cherry-picked with authorship preserved). Both bugs shipped with #95942's extraction of the #93042 spawn machinery and were masked by the lockfile-REUSE path — only a fresh spawn against a real remote exposed them, which is exactly the live-round-trip gap flagged at merge time. @SmelterLabs ran that test on a real Ubuntu 24.04 remote within hours and delivered the fix.
expandRemotePath()already returns a shell-quoted fragment ("$HOME"'/…'); wrapping it inshq()again stored the quote characters in the payload'sreservation/lock/owner_filevariables, so everymkdir/catagainst them targeted a literal"$HOME"path — the reservation loop spun forever holding the update mutex. Same defect inbuildOwnedStaleTerminationCommand's identity match (every comparison REFUSEd).${var//__PID__/$child}; the payload runs under plainsh(dash on Ubuntu), which aborts on it AFTER the serve child spawned — the client saw an unknown failure, deleted the token file, and orphaned one backend per attempt. Now POSIXsed.Validation
buildSpawnCommandpayload executed under dash: no "Bad substitution" abort, no literal-"$HOME"filesystem artifacts, fresh-spawn logic runs through reservation/mutex/publication (pre-fix payload aborts)backend.lock.json, kill-recovery round-trip 12sLive repro: both E2Es above — dash-level and full-remote.
Regression introduced by #95942 (extraction of #93042's spawn block); caught by the community live-run before any release shipped it. Credit: @SmelterLabs — second landed fix of theirs today.
Infographic