feat(browser): authenticated extension controller (salvage #85351) - #91535
Merged
kshitijk4poor merged 13 commits intoAug 21, 2026
Merged
Conversation
Keep extension control opt-in and preserve existing browser backends unless an exact server-bound controller is available. Centralize protocol and capability admission across API and dashboard transports, make selected-controller results authoritative, bypass stale availability caches only inside bound requests, and serialize structured results for the existing tool contract. Add a real browser_snapshot route-table/WebSocket E2E, strict admission and ownership regressions, public configuration and protocol documentation, and tests proving feature-off/no-controller compatibility.
Treat unexpected controller transport loss as recoverable until each command's original deadline. Same-identity reconnects refresh transport and capability state, flush deferred cancels before new dispatch, and can complete already-started work. Keep explicit detach and different controller/browser identity replacement terminal, owner-gate every inbound lifecycle frame, distinguish slow in-flight WebSocket writes from real send failures, and exclude browser-control session identity from shared shell snapshots.
Generic Hermes callers still use the existing browser backend when extension control is disabled or no server-bound controller identity exists. Once the gateway binds a controller identity, missing scope, disconnect, or capability loss now fail closed instead of silently switching a control-this-tab request to another local or cloud browser. Covers the schema-build to dispatch disconnect race.
…, and companion journal
The feature flag was only documented in cli-config.yaml.example; every other browser.* key is declared in DEFAULT_CONFIG so config tooling (dashboard editor, hermes config get) can see it. Defaults unchanged: enabled=False, developer_mode=False. Surfaced during review of PR NousResearch#85351.
… transport auth The router treated any server-stamped principal as a bound lane, so with the flag ON every authenticated dashboard/API session lost the legacy browser backend even when no extension controller ever registered (scope_for_session returns None -> ControllerUnavailable, no fallback) — while check_fns still advertised the tools via the legacy OR-gate. New broker.lane_registered() distinguishes the two cases: - lane never registered -> generic callers keep the legacy backend - lane registered (controller offline/ambiguous) -> fail closed, unchanged — a control-this-tab session never silently jumps to another browser Also makes the four non-allowlisted wrapped tools (cdp/console/vision/ get_images) behave correctly for never-registered lanes (legacy backend) while staying fail-closed for registered lanes. Surfaced during review of PR NousResearch#85351.
attach/disconnect/detach acquire a per-controller threading.Lock that a worker-thread dispatch can hold for up to 10s while blocking on the event loop to transmit its command frame (run_coroutine_threadsafe + result(timeout=10)). Acquiring that lock synchronously from loop context (controller WS finally, frame handler, gateway WS teardown) could park the ENTIRE gateway event loop behind the send bridge — a deterministic multi-second global stall whenever controller teardown raced an in-flight command. All loop-context broker calls now go through asyncio.to_thread, matching the existing offload pattern for _close_sessions_for_transport. Surfaced during review of PR NousResearch#85351.
- Rename the broker's TicketInvalid to ControllerTicketInvalid: the same exception name already exists in hermes_cli/dashboard_auth/ws_tickets.py and BOTH are caught in the same WS auth flow this feature touches — two unrelated same-named exception types in one blast radius invited a wrong except clause. - Import the 'server-internal' sentinel identity from its canonical definition (ws_tickets.INTERNAL_USER_ID/INTERNAL_PROVIDER) instead of re-declaring the strings; drift would have silently broken the internal-peer exclusion in _is_authenticated_identity. Surfaced during review of PR NousResearch#85351.
browser_control_enabled()/browser_control_developer_mode() run on every browser tool call and inside every check_fn evaluation (uncached for bound sessions). Both are pure reads of nested dicts; load_config()'s defensive deepcopy (~135us/call) is wasted there. Same pattern as the other read-only config probes. Surfaced during review of PR NousResearch#85351.
alt-glitch
added
type/feature
Collaborator
Related to #85351: this PR salvages its extension-controller work with authorship preserved and adds review follow-ups for registration fallback, event-loop safety, and configuration wiring. |
andrexibiza
reviewed
Aug 21, 2026
andrexibiza
left a comment
andrexibiza
left a comment
Contributor
There was a problem hiding this comment.
Reviewed exact salvage head 597bc49c6511447306f9d0931cf9505593d3f585 against exact/current main fcbd1076a93841fa88855acce810e342a5b78101. This branch is cleanly 11 commits ahead / 0 behind; exact-head CI (32487504775), Docker (32487504095), and Nix (32487504189) are green.
The five salvage follow-ups are real improvements. In particular, binding authority at actual controller registration rather than merely at transport authentication fixes the feature-on/no-extension fallback bug, and moving broker lock-taking operations off the aiohttp loop closes the event-loop stall path. I also verified this is a genuine salvage rather than unattributed duplication: the six #85351 commits from @abundantbeing are preserved, with the new review follow-ups layered on top.
There are still four merge blockers. Two are unresolved gates from the original #85351 review, and two are additional lifecycle/authority defects visible in the new current-main salvage.
1. HTTP artifact ownership still cannot compose with broker artifact ownership
artifact_scope_key() hashes (principal_id, session_id, transport_family). The HTTP upload/download handlers still use _ArtifactScopeFacade(principal, transport_family=...), so their session_id is empty. Controller registration, however, requires a real existing session_id, and _validate_artifact_reference() validates the HTTP-created artifact against the full ControllerScope.
So the advertised end-to-end path still fails deterministically: authenticated HTTP upload creates an artifact under principal / empty-session / family; browser_artifact_upload or browser_artifact_download then validates under principal / real-session / family and gets ArtifactScopeMismatch.
The tests still prove two disconnected contracts: the HTTP round-trip never enters broker validation, while the broker artifact test seeds ArtifactStore directly with a session-bearing _Scope() that already matches the broker scope.
Required: choose one canonical artifact ownership projection and use it at both boundaries. Add the actual separating witness: authenticated HTTP upload -> registered controller/session -> broker artifact dispatch, plus the reverse controller-produced/download path.
2. The supposedly profile-scoped store is still first-profile-wins process state
APIServerAdapter still owns one _browser_control_artifacts. _artifact_store_for(profile) returns it immediately once initialized; only the first request resolves get_profile_dir(profile). The same singleton is then installed into the process-global broker via attach_artifact_store().
On a multiplex listener, if profile A touches artifacts first, profile B thereafter writes into A's physical artifact root and broker validation for B still consults A's store. The principal digest blocks a trivial logical cross-read, but it does not repair physical ownership or lifecycle isolation.
This is the same frozen-home-bound-handle defect class already fixed in merged #88734 by @teknium1, preserving @jackulau's #88632 authorship: profile-sensitive state must be resolved/cached by the active resolved home, not once before later profile scopes use it.
Required: own stores by resolved profile/home and have broker artifact lookup resolve the correct store from the controller scope instead of one global mutable store. Add an A/B multiplex test proving distinct roots and successful dispatch for both profiles regardless of access order.
3. Developer Mode is live at admission but frozen at broker construction
The salvage says developer_mode is read live, and APIServerAdapter._browser_control_developer_mode() does that. But _GLOBAL_BROKER = BrowserControlBroker() snapshots the flag once in BrowserControlBroker.__init__ into self._developer_mode; select() uses that frozen boolean for browser_evaluate / raw browser_cdp.
That creates both directions of split authority:
- start with Developer Mode off, turn it on at runtime -> registration can now negotiate privileged capabilities, but the broker still refuses them until process restart;
- start with it on, attach a privileged controller, then turn it off -> the live admission gate says disabled while the already-running broker can continue selecting raw CDP/eval from the attached controller.
The second direction is a revocation failure at the highest-privilege browser surface.
Required: make privileged capability selection/dispatch consume the current Developer Mode authority (or explicitly revoke/rebind all affected controller scopes on config-generation change). Add both off->on and on->off tests against the process/global-broker path, including an already-attached controller for the revocation case.
4. Artifact TTL does not survive a process restart
ArtifactStore keeps receipts/expiry/scope/checksum only in the in-memory _entries map. Normal artifact files are random-id files on disk. After restart, _entries is empty; prune_expired() can remove only entries it knows about plus stale *.tmp files. It never sweeps orphan normal artifact files.
That means screenshots/PDFs/uploads can remain on disk indefinitely after a gateway restart even though the module contract says they are TTL-bounded and the API advertises a 300-second lifetime. They become unreachable, but not deleted — a privacy/retention failure on precisely the artifact surface meant to be ephemeral.
Required: make the retention proof restart-safe: persist enough bounded metadata to reconstruct/sweep, or treat unindexed artifact-id files as startup orphans and securely age/prune them. Add store -> process/store recreation -> TTL expiry -> physical-file-removed coverage.
Topology / merge order / credit
- #91535 should be the superseding salvage of #85351, not a parallel duplicate. Keep @abundantbeing's six-commit provenance intact and close/supersede #85351 only when this replacement actually closes its review gates. The earlier blocker review is #85351 (review).
- #84000 by @SolshineCode remains the source feature request. This implementation is broader than the issue's suggested native-messaging shape, but the authenticated broker approach is coherent with the requested shared-visible-browser invariant.
- #88203 by @abundantbeing is complementary pairing/auth UX, not duplicate work. It also edits
gateway/platforms/api_server.pyand is currently based on an older main and non-mergeable, so whichever browser lane lands second must semantically compose auth/profile admission rather than mechanically taking one side. - Merged #88734, preserving @jackulau's #88632 work, is the relevant landed precedent for per-profile physical-state ownership and the frozen-handle bug class.
The broker/controller direction is strong and the exact-head workflows are green. I would re-review once artifact scope composition, per-profile store ownership, live privileged-capability revocation, and restart-safe artifact retention are closed with end-to-end witnesses.
… stores per profile Addresses both merge blockers from @andrexibiza's review of NousResearch#85351: 1. HTTP-uploaded artifacts could never be consumed by broker dispatch: artifact_scope_key hashed (principal, session, family), the HTTP routes store with an EMPTY session (API-key auth has no server session) while broker validation carries a session-bearing ControllerScope — every real upload->dispatch journey died with ArtifactScopeMismatch (reproduced before fixing). Canonical ownership is now principal/transport-family (documented in the scope-key docstring); ids stay unguessable server-minted 32-hex and downloads one-shot. New composition regression: HTTP-shape upload -> registered controller scope -> broker artifact dispatch, mutation-checked (re-adding session to the key makes it fail). 2. The 'profile-scoped' artifact store was first-profile-wins process state: one adapter-level singleton pinned profile B to profile A's physical root on multiplex listeners (same frozen-handle class as NousResearch#88734). Stores are now cached by resolved profile, and the broker selects the store from the controller scope's profile_id (default-slot fallback preserves single-profile/test behaviour). New A/B multiplex regression proves distinct physical roots regardless of touch order. Also documents the advertised ticket_expires_at as best-effort wall clock (broker enforces expiry monotonically) per review feedback.
The frame handler returns reply dicts (heartbeat/detach acks) that the WS reader loop sends back; the -> None annotation was the only new ty diagnostic vs origin/main.
kshitijk4poor
enabled auto-merge (rebase)
August 21, 2026 17:00
10 tasks
Collaborator
Author
|
Thanks for the exact-head rigor — disposition of the four blockers: 1 & 2 (artifact scope composition; first-profile-wins store) — already fixed in the merged head. Your review pinned head
3 (Developer Mode frozen at construction) — real, fixed in #91695. 4 (artifact TTL not restart-safe) — real, fixed in #91695. A fresh Topology notes taken: #88203 pairing UX will need semantic auth/profile composition when it rebases onto this surface, and #88734's per-profile ownership precedent is exactly the pattern the store fix follows. |
kshitijk4poor
added a commit
that referenced
this pull request
Aug 21, 2026
…ection The global broker snapshotted browser.extension_control.developer_mode once at construction, so flipping it OFF in config did not revoke raw CDP/eval from already-attached controllers until process restart — a revocation failure at the highest-privilege browser surface (blocker 3 of andrexibiza's #91535 review). select() now consults the live config on every privileged selection (explicit bool still pins for tests); off->on also unlocks without restart. Regression test drives both directions against an attached controller. Also drops the dead back-compat _artifact_store property (zero readers).
kshitijk4poor
added a commit
that referenced
this pull request
Aug 21, 2026
Artifact receipts live only in memory, so files left behind by a dead process were unreachable but persisted forever despite the advertised 300s TTL — a retention failure on the surface meant to be ephemeral (blocker 4 of andrexibiza's #91535 review). A fresh ArtifactStore now removes every artifact-id-shaped file and stale *.tmp with no index entry (at construction the index is empty, so all such files are orphans). Non-artifact-shaped names are untouched. Regression: store -> recreate store over same root -> orphan+tmp gone, unrelated file kept.
kshitijk4poor
added a commit
that referenced
this pull request
Aug 21, 2026
… transcript _turn_transcript_messages pre-classified every message with _is_compressed_summary_message (full content flatten + prefix scan), then _message_response re-ran the same classifier inside its projection -- 2x per non-summary row, 3x per summary row on every run.completed emit. The outer guard was redundant: _message_response already yields display_kind hidden for pure handoffs. One projection call per row now. Surfaced by the post-merge simplify re-review of #91517/#91535.
swarm-agents-soxzz5 Bot
added a commit
to SoxZz5/hermes-agent
that referenced
this pull request
Aug 22, 2026
* chore: AUTHOR_MAP troy.rowe@re-source.au -> troyrowe-resource Mapping for PR NousResearch#90261 salvage (server-injected parameter 400 classifier). * fix(classifier): retry provider-injected parameter 400s instead of aborting The Codex OAuth backend (chatgpt.com/backend-api/codex) intermittently injects prompt_cache_retention into its own upstream call and then rejects it, returning HTTP 400 invalid_parameter. Hermes never sends that field on this route (see agent/transports/codex.py::_default_prompt_cache_retention_ for_request, which only sets it for api.meta.ai and bedrock-mantle hosts). Reproduced live: a minimal 1-message request carrying no cache parameters at all failed 4/20 (20%) with this error, so the rejection is not deterministic and retrying the identical request is the correct recovery. Previously the catch-all in _classify_400 returned format_error/ retryable=False, which tripped the is_client_error abort gate in conversation_loop and killed the turn on the first attempt - burning an entire large-context request (~550k tokens) per failure. Classify these as retryable server_error (should_compress=False - the request shape was never the problem). The same guard is applied to the sibling 5xx request-validation branch, where a fronting proxy can surface the identical rejection. Deliberately narrow: keyed on parameters we only send on specific routes, and skipped when the current provider is one that legitimately sends them, so a genuine client-side bad parameter (max_tokens on GPT-5) still fails fast as a format_error. * fix(agent): guard merged assistant compaction handoffs Treat a merged assistant-role summary carrier as the driving reference handoff when it immediately follows a completed assistant stop. Its preserved prose and stale tool_calls are assistant continuity, not a fresh live user request. Keep legitimate in-flight behavior unchanged when there is no completed stop, a real user turn follows, or a distinct later assistant tool-call row continues the loop. Extends the NousResearch#80622 active-turn guard for the merged-carrier shape reported under NousResearch#42768. * fix(agent): preserve live merged tool-call carriers Identify a completed merged assistant handoff from the carrier's own stop state instead of an unrelated adjacent history row. Keep carriers with pending tool calls actionable so compaction cannot abort a live tool chain. * fix(api): hide compaction scaffolding from clients Project client-visible session messages through the canonical compaction classifier. Hide standalone handoffs, unwrap merged carriers to their authentic prior-tail content, strip inherited internal fields, and keep model-facing recovery history unchanged. * fix(clients): hide compaction carriers across surfaces * refactor(api): reuse _COMPACTION_INTERNAL_FIELDS from compaction_display The 7-key internal-fields tuple was inlined twice (agent/compaction_display.py and _project_client_message); a drift between the copies would silently leak one internal field class through the API projection. Surfaced during review of PR NousResearch#85442. * fix(desktop): boot overlays stay opaque under window glass The full-screen boot surfaces (connecting, onboarding, boot failure, root crash fallback) paint their backdrop with --ui-chat-surface-background, which the glass field turns transparent so <body> can be the one painter (0483133). That was harmless while glass shipped off; once it shipped on by default (be31666) every boot overlay became a window onto the shell behind it. These overlays mask the whole app, so they declare data-glass-opaque — the existing contract for surfaces that paint over siblings — which pins the token back to opaque chrome under glass and changes nothing when glass is off. * feat(browser): add authenticated control broker * feat(browser): enable extension controller actions * fix(browser): harden extension controller routing Keep extension control opt-in and preserve existing browser backends unless an exact server-bound controller is available. Centralize protocol and capability admission across API and dashboard transports, make selected-controller results authoritative, bypass stale availability caches only inside bound requests, and serialize structured results for the existing tool contract. Add a real browser_snapshot route-table/WebSocket E2E, strict admission and ownership regressions, public configuration and protocol documentation, and tests proving feature-off/no-controller compatibility. * fix(browser): preserve controller work across reconnects Treat unexpected controller transport loss as recoverable until each command's original deadline. Same-identity reconnects refresh transport and capability state, flush deferred cancels before new dispatch, and can complete already-started work. Keep explicit detach and different controller/browser identity replacement terminal, owner-gate every inbound lifecycle frame, distinguish slow in-flight WebSocket writes from real send failures, and exclude browser-control session identity from shared shell snapshots. * fix(browser): keep bound controller routing authoritative Generic Hermes callers still use the existing browser backend when extension control is disabled or no server-bound controller identity exists. Once the gateway binds a controller identity, missing scope, disconnect, or capability loss now fail closed instead of silently switching a control-this-tab request to another local or cloud browser. Covers the schema-build to dispatch disconnect race. * feat(browser): add scoped artifact endpoints, broker permission gates, and companion journal * chore(config): declare browser.extension_control in DEFAULT_CONFIG The feature flag was only documented in cli-config.yaml.example; every other browser.* key is declared in DEFAULT_CONFIG so config tooling (dashboard editor, hermes config get) can see it. Defaults unchanged: enabled=False, developer_mode=False. Surfaced during review of PR NousResearch#85351. * fix(browser): bind the extension lane at controller registration, not transport auth The router treated any server-stamped principal as a bound lane, so with the flag ON every authenticated dashboard/API session lost the legacy browser backend even when no extension controller ever registered (scope_for_session returns None -> ControllerUnavailable, no fallback) — while check_fns still advertised the tools via the legacy OR-gate. New broker.lane_registered() distinguishes the two cases: - lane never registered -> generic callers keep the legacy backend - lane registered (controller offline/ambiguous) -> fail closed, unchanged — a control-this-tab session never silently jumps to another browser Also makes the four non-allowlisted wrapped tools (cdp/console/vision/ get_images) behave correctly for never-registered lanes (legacy backend) while staying fail-closed for registered lanes. Surfaced during review of PR NousResearch#85351. * fix(browser): offload broker lock acquisition off the event loop attach/disconnect/detach acquire a per-controller threading.Lock that a worker-thread dispatch can hold for up to 10s while blocking on the event loop to transmit its command frame (run_coroutine_threadsafe + result(timeout=10)). Acquiring that lock synchronously from loop context (controller WS finally, frame handler, gateway WS teardown) could park the ENTIRE gateway event loop behind the send bridge — a deterministic multi-second global stall whenever controller teardown raced an in-flight command. All loop-context broker calls now go through asyncio.to_thread, matching the existing offload pattern for _close_sessions_for_transport. Surfaced during review of PR NousResearch#85351. * refactor(browser): dedupe auth-flow names and sentinel identity - Rename the broker's TicketInvalid to ControllerTicketInvalid: the same exception name already exists in hermes_cli/dashboard_auth/ws_tickets.py and BOTH are caught in the same WS auth flow this feature touches — two unrelated same-named exception types in one blast radius invited a wrong except clause. - Import the 'server-internal' sentinel identity from its canonical definition (ws_tickets.INTERNAL_USER_ID/INTERNAL_PROVIDER) instead of re-declaring the strings; drift would have silently broken the internal-peer exclusion in _is_authenticated_identity. Surfaced during review of PR NousResearch#85351. * perf(browser): read the feature flags via load_config_readonly browser_control_enabled()/browser_control_developer_mode() run on every browser tool call and inside every check_fn evaluation (uncached for bound sessions). Both are pure reads of nested dicts; load_config()'s defensive deepcopy (~135us/call) is wasted there. Same pattern as the other read-only config probes. Surfaced during review of PR NousResearch#85351. * fix(browser): make the artifact boundary compose end-to-end and scope stores per profile Addresses both merge blockers from @andrexibiza's review of NousResearch#85351: 1. HTTP-uploaded artifacts could never be consumed by broker dispatch: artifact_scope_key hashed (principal, session, family), the HTTP routes store with an EMPTY session (API-key auth has no server session) while broker validation carries a session-bearing ControllerScope — every real upload->dispatch journey died with ArtifactScopeMismatch (reproduced before fixing). Canonical ownership is now principal/transport-family (documented in the scope-key docstring); ids stay unguessable server-minted 32-hex and downloads one-shot. New composition regression: HTTP-shape upload -> registered controller scope -> broker artifact dispatch, mutation-checked (re-adding session to the key makes it fail). 2. The 'profile-scoped' artifact store was first-profile-wins process state: one adapter-level singleton pinned profile B to profile A's physical root on multiplex listeners (same frozen-handle class as NousResearch#88734). Stores are now cached by resolved profile, and the broker selects the store from the controller scope's profile_id (default-slot fallback preserves single-profile/test behaviour). New A/B multiplex regression proves distinct physical roots regardless of touch order. Also documents the advertised ticket_expires_at as best-effort wall clock (broker enforces expiry monotonically) per review feedback. * fix(api): correct _handle_browser_control_frame return annotation The frame handler returns reply dicts (heartbeat/detach acks) that the WS reader loop sends back; the -> None annotation was the only new ty diagnostic vs origin/main. * fix(browser): honor live Developer Mode for privileged capability selection The global broker snapshotted browser.extension_control.developer_mode once at construction, so flipping it OFF in config did not revoke raw CDP/eval from already-attached controllers until process restart — a revocation failure at the highest-privilege browser surface (blocker 3 of andrexibiza's NousResearch#91535 review). select() now consults the live config on every privileged selection (explicit bool still pins for tests); off->on also unlocks without restart. Regression test drives both directions against an attached controller. Also drops the dead back-compat _artifact_store property (zero readers). * fix(browser): sweep orphan artifact files at store construction Artifact receipts live only in memory, so files left behind by a dead process were unreachable but persisted forever despite the advertised 300s TTL — a retention failure on the surface meant to be ephemeral (blocker 4 of andrexibiza's NousResearch#91535 review). A fresh ArtifactStore now removes every artifact-id-shaped file and stale *.tmp with no index entry (at construction the index is empty, so all such files are orphans). Non-artifact-shaped names are untouched. Regression: store -> recreate store over same root -> orphan+tmp gone, unrelated file kept. * perf(api): classify compaction rows once per message in run.completed transcript _turn_transcript_messages pre-classified every message with _is_compressed_summary_message (full content flatten + prefix scan), then _message_response re-ran the same classifier inside its projection -- 2x per non-summary row, 3x per summary row on every run.completed emit. The outer guard was redundant: _message_response already yields display_kind hidden for pure handoffs. One projection call per row now. Surfaced by the post-merge simplify re-review of NousResearch#91517/NousResearch#91535. * fix(desktop): stop tabs double-click-hiding the tab strip; body double-tap reveals it The synthesized double-tap that hides a zone's tab strip rode every tab's pointerdown (generic pane drag and each pane's tabDrag), so a routine double-click on a tab (select a title, retry a click) vanished the whole bar and stranded the zone with no tab, no close X, and no way back but a right-click. Keep the documented hide gesture on the strip background only, and add its inverse as recovery: double-tap a hidden zone's body restores the strip. Regression tests pin both sides of the grammar. * fix(desktop): scope the salvaged fix to the failure-path removal Narrows NousResearch#86278 to exactly the defect. Tabs pass no double-tap context on any press path (generic pane drag, multi-tab selection drag, chrome.tabDrag), so a double-click on a tab can no longer hide the strip; the strip background keeps its documented hide gesture unchanged. The body double-tap reveal from NousResearch#86278 is dropped: the zone body deliberately carries no double-click gesture (virtualized content recreates its nodes between clicks, per the standing ruling in tree-group.tsx), and recovery surfaces for a deliberately hidden header are being decided separately across NousResearch#84458 / NousResearch#81638 / NousResearch#89225. The DOUBLE_TAP_MS export is reverted since no consumer remains outside drag-session. Test file trimmed to the two assertions that pin the grammar: a tab double-tap must not hide the strip (red on main), the strip background double-tap still hides. Taps release on window between presses so the drag-session synthesized double-tap path is the one exercised. * refactor(desktop): make a zone's tab strip a stated mode, not a flag five paths wrote `headerHidden` carried two meanings at once. `true` was either "the user hid this" or "a double-tap nobody meant hid this"; `false` was either "the user wants a strip" or "insert / tab-cycling / dock-enforce / adoption pinned one to escape a dead end". Because the layout wrote the same field the user did, a repair silently overwrote a preference and neither could be read back — and since hiding also unmounted the tab, the ✕ and the menu offering "Show header", a zone that got hidden by accident stayed that way across restarts. Replaces it with `tabStrip?: 'always' | 'never'`, where absent is auto and only the user ever writes it, and moves the decision into one resolver that TreeGroup and the store both call, so the strip on screen and the toggle command cannot disagree. Reachability moves into that resolver as an invariant that outranks an explicit `never`: a closeable tile keeps its ✕ and a lone tool panel keeps its chip, because "hide the chrome" is never a request to make a surface unreachable. With that guarantee held centrally, the four repair writes are gone. Persisted `headerHidden` is dropped rather than translated — nothing on disk distinguishes a deliberate hide from an accidental one, and carrying the accidents forward would re-strand exactly the people who reported being stuck. The double-tap hide goes with it, along with the synthesized double-tap detector it was the only consumer of. It fired from ordinary double-clicks on a tab, nothing announced it, and its undo lived behind the chrome it had just removed. `data-zone-no-header` goes too: it marked full-page views for a body double-click toggle that no longer exists, and nothing has read it since. Supersedes the tab-side half of the fix from abundantbeing and yoniebans, whose commits this builds on. * feat(desktop): give hiding the tab strip a command, and a way back The strip could only be hidden by an undiscoverable double-tap, and once hidden the zone had no chrome left to click — no tab, no ✕, no menu holding "Show". This puts it on the same footing as the status bar, whose hide has never stranded anyone: ⌥⌘T, a ⌘K row, the shell context menu, and the zone menu, which now prints the keystroke on the row that takes the strip away so the way back is stated at the moment it matters. All four resolve their target zone the same way the other tab verbs do (hovered, else focused, else the workspace) and describe themselves from what is on screen rather than from a stored value, so "toggle" always means the opposite of what the user is looking at. Adds an app-wide default alongside it, in Appearance next to Session List Density — auto, always, or never, matching VS Code's `workbench.editor.showTabs` and Zed's `tab_bar.show` for people who want one answer everywhere instead of a per-zone choice they repeat. A zone that has stated its own preference still wins, and neither value can strand a pane. * feat(nix): wait for the backend bind target before it starts The backend binds to `backend.host` immediately. The bind fails when the target is not ready, because uvicorn cannot bind a name that does not resolve, or an address that no interface holds. A unit that starts at boot loses this race against the daemon that supplies the target, such as tailscaled. A bind to a Tailscale MagicDNS name shows the problem. The name is the correct bind target, because the dashboard refuses each request with a Host header that is different from the address that the server bound to, and a shared machine has a different address in each tailnet. But the name does not resolve until tailscaled is up, so the unit fails at each boot until `Restart=on-failure` finds the moment when the name works. A systemd user unit cannot order itself after a system unit. `After=` and `Requires=` are silent no-ops across that boundary. Thus the wait is a poll, and not a dependency. This change adds three options to `services.hermes-agent.backend` on both the NixOS module and the Home Manager module: - `waitFor` — `null` (the default, unchanged behavior), `"hostname"`, or `"interface"` - `interfaceName` — the interface to take the address from - `waitTimeout` — the time in seconds before the unit stops With `waitFor`, ExecStart becomes a launcher that polls for the target and then execs hermes. `exec` keeps hermes as the MainPID, so the restart logic of systemd sees the real process. A timeout stops the unit with an error. It does not bind a fallback address, because a fallback can expose the backend more widely than the user intends. The default is not changed. Without `waitFor`, ExecStart is the same command line as before. * docs: Add Nix/NixOS to installation link description * add mike@vorburger.ch to contributors * feat(cron): bot-chat delivery target — cron output lands in a bot's canonical Bot Chat and the bot responds deliver='bot-chat[:<profile>]' is a machine-local pseudo-platform: the scheduler delivers job output as a real inbound turn in the target profile's canonical Bot Chat via the chat CLI lane (--in ~ -c "Bot Chat" --create-if-missing -Q --query-file), the same lane Bot Mode agent-to-agent messages use. The bot reads the output, acts on it, and responds in its chat — instead of the output only landing in Run history. - cron/scheduler.py: token parsing, target resolution (own profile / named local profile / unknown -> skipped with warning), subprocess delivery lane with cron.bot_chat_delivery_timeout_seconds (default 600s), preflight exemption, and bot-chat entries in cron_delivery_targets() for UI pickers. Excluded from 'all' by design. - tools/cronjob_tools.py: create/update-time validation — named profiles must exist on this machine (fail at create, not at 3am); deliver schema documents the new token. - tui_gateway/methods_tools.py: cron.manage add forwards deliver. - hermes_cli/profiles.py: list_profile_names() cheap name-only scan. - hermes-bots plugin: Create Cronjob dialog gains a 'Send results to' picker (Run history only / <bot>'s chat); bot-chat jobs send the BARE token on the profile-scoped create so Desktop-side aliases can never name a profile the backend doesn't have. - Docs: user cron guide, automate-with-cron, cron-internals. Machine-local by construction: names resolve only against the executing machine's ~/.hermes/profiles/, so overlapping profile names across multiple connected gateways are unambiguous. * test(cron): delivery-targets test scopes platform assertions past bot-chat entries cron_delivery_targets() now also lists machine-local bot-chat:<profile> entries; the sibling test's exact set-equality assertion predates them. Scope the platform assertions to gateway entries and pin that bot-chat entries are always home_target_set. * kanban: resume-on-retry session continuity for re-review rounds A task that bounces review-requested-changes -> ready and gets reclaimed by the SAME implementer profile previously got a fully cold 'hermes chat -q' session every round: full system prompt, tool schemas, and task history re-fetched from scratch. _default_spawn now accepts an optional conn and, when the dispatcher passes one, calls _resolve_worker_resume_session_id() to look up the implementer's own worker_session_id stamped on the last ENDED run under that profile (kanban_complete/kanban_request_review already stamp this via _stamp_worker_session_metadata). If that session still resolves in the profile's own state.db, the child is launched with --resume <id> --no-restore-cwd instead of a cold start. Any resolution failure (no prior run, wrong profile, session gone) falls back to today's cold-start behaviour -- dispatch is never blocked. Both dispatch_once() spawn call sites (ready lane, review lane) now pass conn through to spawn_fn via signature introspection, matching the existing board= pattern so older test stubs are unaffected. Card: t_e1ba67d5 --------- Co-authored-by: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Co-authored-by: Troy Rowe <troy.rowe@re-source.au> Co-authored-by: abundantbeing <beingsabundant@gmail.com> Co-authored-by: emozilla <emozilla@nousresearch.com> Co-authored-by: yoniebans <jonny@nousresearch.com> Co-authored-by: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com> Co-authored-by: ethernet <arilotter@gmail.com> Co-authored-by: Michael Vorburger <mike@vorburger.ch> Co-authored-by: Teknium <127238744+teknium1@users.noreply.github.com> Co-authored-by: Saylent Swarm <swarm@saylent.dev>
Contributor
|
Thanks to everyone who landed this with authorship preserved. For the record, the full scope of the salvage as merged: The six original #85351 commits, unchanged:
The five review follow-ups added on top:
Plus the four-blocker dispositions from the final review: artifact scope composition and per-profile store ownership were fixed pre-merge ( With this surface now in main, the companion pairing flow (#88203) has been rebased onto it with the semantic composition the topology note asked for rather than one side taken mechanically:
Verified on the rebased head ( |
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
4 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
Adds the opt-in browser-extension controller lane: with
browser.extension_control.enabled, an authenticated extension can register as the exact controller for a session'sbrowser_*tools — one-shot WS tickets, capability allowlist (raw CDP/eval dev-mode gated), owner-scoped reconnect/detach, fail-closed once a lane is bound.Salvage of #85351 by @abundantbeing — all 6 commits cherry-picked with authorship preserved, plus 5 follow-up commits from review.
Changes
Contributor commits (unchanged): broker core, controller actions, routing hardening, reconnect preservation, authoritative bound routing, scoped artifact endpoints.
Review follow-ups (ours):
browser_*call (ControllerUnavailable, no fallback) while check_fns still advertised the tools. Newbroker.lane_registered(): never-registered lane → legacy backend; registered-but-offline lane → fail closed exactly as before (a control-this-tab session never silently jumps browsers). +3 regression tests incl. real-broker lifecycle.threading.Locksynchronously on the event loop while a worker-thread dispatch can hold it for up to 10s blocking on that same loop (send bridge). All loop-context broker calls now go throughasyncio.to_thread.browser.extension_control.{enabled,developer_mode}declared (was only in cli-config.yaml.example).TicketInvalidrenamedControllerTicketInvalid(same-named exception fromws_ticketsis caught in the same WS auth flow);server-internalsentinel imported from its canonicalws_ticketsdefinition instead of re-declared.browser_control_enabled/developer_modeuseload_config_readonly()(pure reads on every tool call/check_fn; skips the defensive deepcopy).Validation
Known accepted trade-off: with a bound lane, check_fn availability bypasses the definitions cache so browser tools can appear/disappear on attach/detach — confined to bound browser-control sessions, inherent to the feature's request-scoped design.
Closes #85351. Credit: @abundantbeing (authorship preserved via cherry-pick).
Refs #84000; companion pairing flow: #88203.