Fix stop: start turns as real runs so they can be interrupted - #1
Open
sanketkheni01 wants to merge 19 commits into
Open
Fix stop: start turns as real runs so they can be interrupted#1sanketkheni01 wants to merge 19 commits into
sanketkheni01 wants to merge 19 commits into
Conversation
Right-clicking empty canvas now opens a menu offering either a new session or a new screen. A screen window streams a live remote desktop (RFB/VNC) beside the existing session cards, and drags, resizes and persists exactly like one. Screen targets are configured server-side through HERMES_VISUAL_SCREENS (key|label|host|port|password, restricted to the Tailscale mesh or loopback). The browser only ever receives an opaque key and a label. The backend bridges a same-origin WebSocket to the host's RFB port using only the standard library, and terminates authentication itself: it performs the DES challenge (security type 2) against the remote host, then replays a security-type-None handshake to noVNC. No credential reaches the frontend or the SQLite database.
…space
Files can be attached to a session window by drag-and-drop onto the card,
the paperclip in the composer, or paste. Uploads render as chips beneath
the composer -- image thumbnails inline, other kinds as a labelled glyph --
and each chip links to its bytes for download.
How attachments reach the model matters, because the Hermes session chat
API accepts text and image_url/input_image parts but rejects input_file
document parts outright. The split mirrors what gateway/platforms/base.py
cache_media_bytes already does for Telegram and the other adapters:
images -> inlined as a base64 data URL image_url part, so the
model genuinely sees them
everything else -> written to disk and named in the text as
[document 'x.csv' saved at: /path], which the agent
opens with its own tools
Verified end to end against the live gateway: a red PNG and a two-row CSV
uploaded together came back as 'Red (#FF0000), and the CSV totals 146,000'
-- vision on the image, file read on the document.
Notes on the implementation:
- multipart parsing is hand-rolled; the backend is stdlib-only and
cgi.FieldStorage is gone in 3.13.
- stored filenames come from the attachment id, never the upload name,
so a traversal or colliding name cannot escape the node directory.
- delivery is claimed transactionally, so a re-run or concurrent stream
cannot send the same file to two turns.
- 20 MB per file, matching the Telegram adapter's _max_doc_bytes rather
than inventing a new limit.
- storage_path is server-side only and never reaches the browser.
npm run verify passes: 29 new Python tests plus a Playwright acceptance
covering upload, thumbnail render, download links, removal and drag-drop.
A drawing window places a tldraw document on the canvas beside session cards. Drawings belong to the workspace and nothing else: they hold no Hermes session, take no parent node, and are never sent to a model. The snapshot is stored opaquely in a `drawing_windows` row and reloads with the canvas. The editor runs inside a same-origin iframe rather than on the workspace page. tldraw applies inline style attributes at runtime, and CSP cannot whitelist those with a nonce or a hash -- the browser ignores hashes for style attributes unless 'unsafe-hashes' is present. Relaxing style-src on the main document would weaken the policy exactly where model-authored markdown is rendered, so the relaxation is scoped to `drawing.html`, which renders only the editor. `_serve_static` picks the policy per path and that document is the only one served with `frame-ancestors 'self'`. Left on its defaults tldraw fetches fonts, icons and translations from cdn.tldraw.com, which `connect-src 'self'` blocks and which would break the app on a private tailnet. The @tldraw/assets tree is copied into the bundle so every asset is same-origin. The acceptance test asserts the toolbar and style panel have real layout boxes, not merely that the editor mounted. Under a blocked style-src the DOM still builds and the editor API still answers, so an API-only test passes while the window paints blank; measuring geometry is what catches that regression. Snapshots are excluded from the workspace listing and fetched per frame, so a large drawing does not inflate every canvas load.
Deploying to prod surfaced this: the workspace service writes uploads under the host's filesystem, but the agent runs in a container where that path does not exist, so every path note pointed at nothing. Images were fine -- they are inlined as vision parts and never touch the path -- which is what made the failure quiet and easy to miss. HERMES_VISUAL_AGENT_PATH_PREFIX could only prepend, which does not describe a bind mount where one absolute path becomes a different absolute path. Replaced with HERMES_VISUAL_AGENT_PATH_MAP taking 'from:to' (/root/.hermes:/opt/data here); a bare value still prepends.
The asset tree was already copied into the bundle, but the editor was never told to use it: `Tldraw` was mounted without `assetUrls`, so the SDK fell back to cdn.tldraw.com for icons, fonts and translations. On the live server that meant 115 blocked requests and 119 CSP violations behind `connect-src 'self'` -- and a drawing window that could not work offline on the tailnet. @tldraw/assets/selfHosted resolves plain relative paths (unlike the meta-url variant, which resolves against the bundle) so the URLs point at the copies already served next to drawing.html. The acceptance test missed this because it inspected the fixture's own request log, and requests to an external CDN never reach the fixture. It now watches real browser requests and fails on anything leaving the origin; verified by rebuilding without the fix, which reports the CDN URLs. The reload step also waits on the drawing window rather than `networkidle`, which never settles while the app holds an SSE stream.
An attachment stayed pinned above the input box after it had been sent, which read as if it were still queued to send, and it kept a remove button that the backend rightly refused -- clicking it produced 'attachment was already sent to Hermes'. Chips now live in one of two places depending on delivery: pending ones in the composer where they can still be removed, sent ones above the answer under a 'Sent' divider, where they belong to what was actually said. The frontend mirrors the backend's claim on the first node-scoped stream event, keyed off the node id rather than one event name so run, continue, branch and merge all behave the same.
A drawing window now shows a rendered preview until the user clicks Edit, and only one editor is live at a time. Previously every drawing on the canvas booted a full tldraw editor -- a React app with its own canvas and animation loop -- and kept it running whether or not anyone was using it. Two things made that expensive rather than merely wasteful. Several idle editors compete for the main thread with the canvas the user is panning. And the canvas zooms by setting CSS `zoom` on an ancestor of every window, almost always at a fractional factor (0.7549 in the reported session): a live iframe under a fractional scale is resampled by the compositor every frame, and at some scales composites as an empty quad -- the black window. The preview is a downscaled PNG rendered by the editor on save and stored next to the snapshot, so an idle window costs one <img>. It is strictly a cache of the document: a save that cannot rasterise omits the preview rather than failing, and omitting it leaves the stored one untouched, so a failed render can never blank a window or lose a drawing. An active window is promoted to its own compositing layer, which is what keeps the one remaining live editor from flickering under the same fractional zoom. `drawing_windows` predates the preview column, so it is added by migration rather than schema replacement; a test drops the column and reopens the database to prove an existing workspace survives the upgrade.
The idle preview filled its body with near-black (#101014), which is indistinguishable from the compositor bug it was introduced to fix: on a canvas of empty drawings the result looked exactly like the black windows being reported. It now uses the card surface with the same dotted texture as the canvas, and a rendered document sits on a white plate, so an idle window is visibly deliberate. The whole preview activates the editor rather than just the small Edit button, which is the obvious target for a click. That makes the label decorative, so it loses pointer events, and keyboard activation is wired explicitly because the preview wraps an image and cannot be a <button>.
perf1/perf2/perf3/zoomtest/livecheck were one-off probes used to measure canvas frame times and confirm the iframe sat under a fractional CSS zoom. They were committed by an over-broad 'git add -A' and are not part of the app or its suite.
Stopping a node always failed with 'Run not found'. Turns were sent
through /api/sessions/{id}/chat/stream, which mints a run id purely to
correlate SSE events and never registers it in core's active-run table.
POST /v1/runs/{id}/stop looks the id up in exactly that table, so every
stop 404'd and the agent kept running.
Turns now start via POST /v1/runs, which registers the run and makes it
interruptible. The run id is persisted before the first event is read,
so a turn is stoppable from the instant it starts rather than only once
an event happens to carry the id back.
/v1/runs does not reload prior turns, so session continuity is supplied
explicitly from the stored transcript.
Verified against the live service: a running node stops in ~0.3s, emits
no further deltas, keeps its partial output, and the underlying agent
stops writing.
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
1 participant
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.
Problem
Stopping a running node never worked. The UI stop control always failed with:
and the agent kept generating regardless.
Root cause
Turns were sent through
POST /api/sessions/{id}/chat/stream. That endpoint mints arun_idpurely to correlate SSE events — it never registers the run in core's active-run table (_active_run_agents/_active_run_tasks).POST /v1/runs/{run_id}/stopresolves the id in exactly that table, so every stop 404'd and the agent ran to completion.Confirmed against the live API before changing anything:
Client disconnect does not interrupt either, so there was no working stop path at all.
Second, independent bug
The run id was only learned mid-stream, from whichever event happened to carry it. During the model's first (and usually longest) call no id existed yet, so an early stop hit
this run cannot be stopped yet. Even had the id been valid, the earliest part of a turn was unstoppable.Fix
POST /v1/runs, which registers the run and makes it genuinely interruptible./v1/runsdoes not reload prior turns, so session continuity is supplied explicitly from the stored transcript.run.completedon this path carries final text asoutput; the normalizer now reads it.Verification (live service, not just unit tests)
node.failed/ "Stopped at your request."continueturn.tool.started/tool.completed, actual terminal output) still work.Tests
npm run verifygreen: 113 tests + types + purity.Added
test_run_id_is_recorded_before_any_event_so_stop_works_immediately. It is not vacuous — reverting the fix makes it fail:Existing stop tests passed against the broken code because they injected a run id manually, bypassing the path that was actually broken.
Deployment note
Branch is cut from
1db3694, the SHA currently deployed on the Nextbase host, soconfig/hermes-visual-workspace.lockinopenclaw-internalcan be bumped straight to this commit.