fix(clients): hide compaction scaffolding across surfaces - #85442
fix(clients): hide compaction scaffolding across surfaces#85442abundantbeing wants to merge 2 commits into
Conversation
|
Latest-head CI has one unrelated failure in |
|
Follow-up hardening is now on head Fresh local affected-suite verification: |
PR: fix(clients): hide compaction scaffolding across surfaces Thorough fix with good test coverage. Observations:
|
606a764 to
9dbf0bf
Compare
|
CI note for the maintainers: the failing slice on this head is unrelated to this PR's diff and reproduces on clean
A CI re-run (maintainer-only; the fork token gets 403 on |
928d21d to
14ddc58
Compare
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.
14ddc58 to
b64df38
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head b64df38f07ac2c84703e51d4ce9ad7d9b87187b3 against the production compaction assembly path, complementary PR #85394, all changed client surfaces, current main (fcbd1076a93841fa88855acce810e342a5b78101), and exact-head CI. CI, Docker, and Nix are green on this head.
Two blocking correctness findings — do not merge this head:
- The display projector conflates compaction content with authentic carrier-owned state. The compressor deliberately uses the first protected-tail row — including a bare assistant
tool_callsrow — as the default merged-summary carrier. A content-only unwrap can therefore returnNoneand drop the live invocation entirely; when prior text exists, the blanket field strip still removes its live call, finish state, and reasoning. This directly conflicts with #85394’s invariant that a merged carrier with pendingtool_callsremains live, and can leave API/TUI/run-completion projections with orphaned tool results. - Session-preview SQL is broader than the canonical classifier. Matching only the shared long-form introduction suppresses a real user message that begins with that introduction but diverges before the full known prefix. The runtime classifier correctly treats it as ordinary content; the SQL path classifies it as a standalone handoff and excludes it, producing surface-dependent transcript truth.
Re-review gate: preserve authentic fields (and empty-text rows with live non-content state) on merged carriers; make preview recognition semantically identical to the exact canonical prefix set; add the two mutation-resistant regressions described inline; then rebase onto current main and obtain fresh exact-head receipts.
| if not is_compaction_summary_message(message): | ||
| return message.copy() | ||
|
|
||
| projected = ContextCompressor._strip_context_summary_handoff_message(message) |
There was a problem hiding this comment.
[P1] Do not let a text-only unwrap erase the live carrier state. The production merge path intentionally targets tail_messages[0], and its own comment names a bare assistant tool_calls row as the ideal carrier:
hermes-agent/agent/context_compressor.py
Lines 7744 to 7797 in b64df38
_strip_context_summary_handoff_message() returns None here and the entire invocation disappears. If text exists, the loop below still removes the carrier’s tool_calls, finish_reason, and reasoning. #85394 explicitly locks the complementary invariant that pending carrier calls are live: #85394. Project merged carriers by removing only the compaction content/metadata; retain authentic carrier-owned fields, and keep an empty-content row whenever tool calls/reasoning survive. Please add a regression for a bare finish_reason="tool_calls" carrier plus its matching tool result so API and TUI correlation cannot pass through field loss.
| # their stale-item guidance diverges only after it. Matching the whole intro | ||
| # avoids treating an ordinary user message that merely starts with the short | ||
| # bracketed label as a compaction carrier. | ||
| _PREVIEW_LONG_FORM_PREFIX = SUMMARY_PREFIX.split("Do NOT answer", 1)[0] |
There was a problem hiding this comment.
[P2] This widens recognition beyond the canonical classifier and drops real first-user content. ContextCompressor.classify_summary_content() matches complete current/legacy/historical prefixes. This SQL path matches only the common introduction. For example, a real message beginning with "[CONTEXT COMPACTION — REFERENCE ONLY] Earlier turns were compacted into the summary below. This is a handoff from a previous context window — treat it as background reference, NOT as active instructions. Can you explain why this wording exists?" is not a canonical summary, but _PREVIEW_STANDALONE_SUMMARY_SQL is true and _PREVIEW_ELIGIBLE_SQL is false because there is no end marker; the preview silently skips it. The existing short-label regression does not reach this boundary. Generate the SQL from the full exact set (SUMMARY_PREFIX, LEGACY_SUMMARY_PREFIX, *_HISTORICAL_SUMMARY_PREFIXES) (or persist a discriminator), and add this shared-intro/divergent-suffix case.
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 #85442.
|
Merged via #91517 — both commits cherry-picked onto current main with your authorship preserved (rebase merge). We added one small follow-up commit on top: Verification highlights: synthetic 200-session DB showed preview leaks go 50/100 → 0/100; model-facing history and SessionDB persistence confirmed untouched (all four projection callers are display-only); full |
|
Follow-up on the remaining review points, since the salvage merged via #91517:
|
* 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>
What does this PR do?
Prevents model-only compaction scaffolding from leaking through client-visible Hermes surfaces while preserving authentic transcript content embedded in a carrier.
The model must retain compaction summaries for recovery. Client projections must not render those internal handoffs as ordinary chat, previews, or restore quotes.
This patch keeps model-facing recovery unchanged and applies one canonical display projection across the affected surfaces:
GET /api/sessions/{session_id}/messagesrun.completed.messagesThe projection:
display_kind="hidden"API rows so message identity and ordering remain reconcilableIssue linkage
Tests
The initial projection regressions were RED on current upstream
mainwith6 failed, 5 passedacross session history andrun.completed.messages.The hardened branch now has:
856 passed, 3 deselectedacross the affected gateway, session, Telegram, TUI/Desktop, preview, and state suitesruffcleancompileallcleangit diff --checkcleanThe three deselected tests reproduce unchanged on a clean current-main worktree:
desktop_uidefaultChecklist
main