Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The heartbeat validation message is inconsistent with the updated schema, and the regression test needs the requested contract and dispatch coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Fixes terminal heartbeat schema handling by making heartbeat=0 a valid disabled default, preventing foreground-call validation loops.
Changes:
- Sets heartbeat minimum and default to
0. - Adds regression coverage for fully materialized foreground calls.
| File | Summary | Final review comments |
|---|---|---|
tools/terminal_tool.py |
Updates heartbeat schema semantics. | Moderate (3 votes): update the invalid-value message from “min 60” to describe a non-negative value. |
tests/tools/test_process_heartbeat.py |
Tests disabled heartbeat foreground dispatch. | Nits: assert default: 0 (1 vote) and dispatch through the registry (2 votes). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1389
to
+1391
| "minimum": 0, | ||
| "default": 0, | ||
| "description": "0 disables. With background=true: also notify every N seconds with the output since the last notice (positive values are clamped to min 60). For long jobs you must react to mid-run (merge trains, full suites); implies notify=true." |
| "heartbeat": heartbeat_schema["minimum"], | ||
| } | ||
|
|
||
| result = json.loads(tt._handle_terminal(generated)) |
alt-glitch
added
type/bug
9 of 13 tasks
OutThisLife
pushed a commit
that referenced
this pull request
Sep 26, 2026
… as a user bubble
A `terminal(background=true, heartbeat=N)` tick queued a notification every N seconds
whether or not the process had printed anything, and every queued event costs the owning
session a full model turn. On Desktop and the TUI that turn painted the wake as a user
bubble ("[Background process ... heartbeat #9 ... (no new output since the last
heartbeat)]") followed by the model's "Still running normally." — over and over, for a
process whose row on the status stack already said it was running — and while the wake
held the session's turn, the user's own prompt sat queued behind it.
- `ProcessRegistry._emit_heartbeat` skips a tick with no new output. The sequence counts
delivered beats only; the "(no new output)" placeholder in the formatter is gone.
- TUI/Desktop type heartbeat rows `display_kind: hidden` (the kind both clients and the
transcript preview already honour); the CLI paints a one-line receipt and persists the
row hidden, so reopening the session in Desktop shows only the agent's reply.
- Desktop hydration drops heartbeat rows persisted by older backends the same way.
- `display.background_process_notifications: off` is honored by the TUI/Desktop poller and
the CLI drain, not just the messaging gateway. `off` mutes process-driven wakes only:
a finished `delegate_task(background=true)` still lands.
Supersedes #123123 (cherry-picked; scoped so `off` keeps subagent results) and #119202
(cherry-picked; `heartbeat: 0` is schema-valid so models that materialize every field
stop tripping the foreground guard).
OutThisLife
added a commit
that referenced
this pull request
Sep 27, 2026
… as a user bubble
A `terminal(background=true, heartbeat=N)` tick queued a notification every N seconds
whether or not the process had printed anything, and every queued event costs the owning
session a full model turn. On Desktop and the TUI that turn painted the wake as a user
bubble ("[Background process ... heartbeat #9 ... (no new output since the last
heartbeat)]") followed by the model's "Still running normally." — over and over, for a
process whose row on the status stack already said it was running — and while the wake
held the session's turn, the user's own prompt sat queued behind it.
- `ProcessRegistry._emit_heartbeat` skips a tick with no new output. The sequence counts
delivered beats only; the "(no new output)" placeholder in the formatter is gone.
- TUI/Desktop type heartbeat rows `display_kind: hidden` (the kind both clients and the
transcript preview already honour); the CLI paints a one-line receipt and persists the
row hidden, so reopening the session in Desktop shows only the agent's reply.
- Desktop hydration drops heartbeat rows persisted by older backends the same way.
- `display.background_process_notifications: off` is honored by the TUI/Desktop poller and
the CLI drain, not just the messaging gateway. `off` mutes process-driven wakes only:
a finished `delegate_task(background=true)` still lands.
Supersedes #123123 (cherry-picked; scoped so `off` keeps subagent results) and #119202
(cherry-picked; `heartbeat: 0` is schema-valid so models that materialize every field
stop tripping the foreground guard).
OutThisLife
added a commit
that referenced
this pull request
Sep 27, 2026
… as a user bubble
A `terminal(background=true, heartbeat=N)` tick queued a notification every N seconds
whether or not the process had printed anything, and every queued event costs the owning
session a full model turn. On Desktop and the TUI that turn painted the wake as a user
bubble ("[Background process ... heartbeat #9 ... (no new output since the last
heartbeat)]") followed by the model's "Still running normally." — over and over, for a
process whose row on the status stack already said it was running — and while the wake
held the session's turn, the user's own prompt sat queued behind it.
- `ProcessRegistry._emit_heartbeat` skips a tick with no new output. The sequence counts
delivered beats only; the "(no new output)" placeholder in the formatter is gone.
- TUI/Desktop type heartbeat rows `display_kind: hidden` (the kind both clients and the
transcript preview already honour); the CLI paints a one-line receipt and persists the
row hidden, so reopening the session in Desktop shows only the agent's reply.
- Desktop hydration drops heartbeat rows persisted by older backends the same way.
- `display.background_process_notifications: off` is honored by the TUI/Desktop poller and
the CLI drain, not just the messaging gateway. `off` mutes process-driven wakes only:
a finished `delegate_task(background=true)` still lands.
Supersedes #123123 (cherry-picked; scoped so `off` keeps subagent results) and #119202
(cherry-picked; `heartbeat: 0` is schema-valid so models that materialize every field
stop tripping the foreground guard).
|
Thanks @tmaarcxs. This landed on Copilot's inline note still applies: the refusal text in |
auroracapital
added a commit
to auroracapital/hermes-agent-nous-fork
that referenced
this pull request
Sep 27, 2026
* fix(terminal): heartbeat error text matches the 0-disables schema
Since 4317ed0e71 the heartbeat schema allows 0 (disabled) and clamps
positive values to 60, but the validation error still said "min 60",
steering models away from the valid 0. Flagged on #119202.
* fix(agent): allow async delegation handoff to end turns
(cherry picked from commit 1378fa1b289ead3b2f8db582353eb4b1574c3d51)
* fix(agent): scope async yield guidance to delegation
(cherry picked from commit 99f82de0f99b055bdf5a5ec15be5e662934f2172)
* test(agent): pin background delegation yield guidance
(cherry picked from commit 2034413f35c91c462ed20b9e22b74ec5d9a4ef95)
* test(agent): trim async handoff tests to two invariants
Keep the stack at <=2 invariant tests: the guidance is injected only when
delegate_task is in the toolset (and after the generic keep-working
blocks), and the stable prompt tier stays byte-identical across rebuilds
so the prompt-cache prefix does not drift.
* test(agent): fold async handoff test and dedupe rationale comment
The byte-stability test for the handoff block only re-asserted determinism of
a constant gated on tool-name membership; stable-tier rebuild stability is
already covered by test_system_prompt_restore and test_skills_auto_load. Its
one unique check (block appears exactly once) moves into the positive branch
of the parametrized injection test, and the _prompt helper now takes only the
tool names since every caller used the same model/gates.
The rationale comment lived twice (prompt_builder constant and the
system_prompt call site); keep only the call-site ordering note.
* chore: map happy5318 for salvage of #124086
Attribute the cherry-picked #124086 commit (author email
5318happy@users.noreply.github.com) to happy5318 in release notes.
* fix(compression): classify a Codex stream-guard stall as a timeout, not a terminal network failure (#124077)
## Thinking Path
When the Codex auxiliary stream guard aborts a compaction summary mid-stream
it raises `TimeoutError("Codex auxiliary Responses stream stalled: no new
output for 60.0s ...")`. The message contains neither "timeout" nor
"timed out", so `_classify_summary_failure` returned `timeout=False` while
`_is_connection_error` (which matches the type name "Timeout") returned
`streaming_closed=True`. The terminal network-failure flag then armed an
unconditional abort (`_TERMINAL_SUMMARY_FAILURES`), bypassing the retry
ladder and the deterministic fallback summary — on turn-start preflight
compression that ends in "Auto-resetting session after compression
exhaustion", wiping the session.
### What Changed
`agent/context_compressor.py` `_classify_summary_failure`:
- `timeout` is now computed first, and additionally matches `isinstance(e,
TimeoutError)` and the "stalled" message shape (the actual text the Codex
guard emits).
- `streaming_closed` is `_is_connection_error(e) and not timeout` — a
timeout keeps its retry-ladder semantics and can never arm the terminal
network-failure abort.
### Tests
New `TestSummaryFailureClassification124077` in
`tests/agent/test_context_compressor.py`:
- classify: Codex stall → `timeout=True, streaming_closed=False`.
- classify: plain `ConnectionError` stays `streaming_closed=True` (no
regression on the premature-close class).
- classify: a "timed out" message on a non-TimeoutError type stays a
timeout and is excluded from `streaming_closed`.
- integration: the stalled-summary path in `_generate_summary` does NOT arm
`_last_summary_network_failure`.
### Verification
- RED/GREEN double proof via git stash: pre-fix 3 failed, post-fix 4/4 pass.
- Regression: 15 existing failure-classification tests pass
(network_failure / premature_stream / empty_content / auth / truncation).
- ruff clean on changed files.
### Notes
Local test env: this checkout's venv python is a symlink into
`<home>/hermes-agent/.hermes-runtime/...`, so stdlib `zoneinfo` first-import
and pydantic's plugin `distributions()` scan under the real-home IO guard
needed the collection-time warmups at the top of the test file. CI
interpreters are not symlinked into the home — those two warm-up blocks are
no-ops there.
## Related
#124077 (issue). Family: #124078 (the same stall's template trigger),
#108104 (`auxiliary.compression.no_progress_timeout`).
(cherry picked from commit c5399fbdee44f2db1172f22632cbf348a2c7cab5)
(cherry picked from commit bce26a99791c09911d8a008a1922c512f5c1fcea)
* fix(compression): keep transport timeouts terminal; reclassify only the Codex stall
The cherry-picked fix set streaming_closed=False for every timeout, which
also stripped the terminal abort-and-preserve-session behaviour from real
network timeouts (openai APITimeoutError, httpx Read/ConnectTimeout), the
deliberate #29559/#25585/#94448 design. Narrow it: only a TimeoutError whose
message says "stalled" (the Codex aux stream guard) becomes a timeout that
takes the 60/300/900s ladder and does not arm _last_summary_network_failure.
Other timeouts classify exactly as on main.
Co-authored-by: happy5318 <5318happy@users.noreply.github.com>
(cherry picked from commit e013c38a017b4709d4598a0a07c71ea26519312c)
* test(compression): trim #124077 tests to two invariants, drop local-env warm-ups
The zoneinfo/pydantic plugin warm-up imports were author-machine workarounds
that ran at collection time in CI; remove them. Collapse the class to one
helper and two tests: the Codex stall takes the timeout ladder without the
terminal network-failure flag, and APITimeoutError still sets it.
Co-authored-by: happy5318 <5318happy@users.noreply.github.com>
(cherry picked from commit 1cda73e0ac2d073066741a964b5166b16e9caf34)
* refactor(compressor): key the Codex stall on the stream guard's shared marker
Co-authored-by: happy5318 <5318happy@users.noreply.github.com>
(cherry picked from commit 83c0a8bb9c7d0e611bafe436d2e4da77144c26ec)
* fix(compression): match the Codex stall marker on the raw error text
The stall check relied on the lowercased error string, which only works
while CODEX_STREAM_STALL_MARKER happens to be all-lowercase. Match against
str(e) so the shared marker stays authoritative regardless of case, and
fold the two duplicate #124077 comments into one explaining the split
(stall -> retry-ladder timeout; transport timeouts stay terminal).
* test(compression): pin the TimeoutError guard on the Codex stall reclassification
The isinstance(e, TimeoutError) guard was untested: an APIConnectionError
whose text contains the stall marker must stay a terminal network failure
(#29559/#94448). Parametrize the existing api-timeout test with that input
(red when the guard is removed), and move both #124077 tests into
TestStreamingClosedFailure reusing _fail_on_main instead of a duplicate helper.
* test(compression): parametrize the transport-error case over exception instances
Why: the test parametrized a label string that a ternary in the body mapped
back to an exception, and its name still said "api_timeout" although it also
covers an APIConnectionError carrying the stall marker. Parametrize the two
exception instances directly (with ids) and rename the test to
test_transport_errors_stay_terminal_network_failure. No behaviour change; still
one parametrized test.
* fix(backup): drop failed partial zip members
(cherry picked from commit e19cb0d5880008098066c987e8edc6ad67b0ab66)
* test(backup): cover failed zip member cleanup
(cherry picked from commit 857b969c57cd1c7f50b4d5c3d41800e2bc24e4d0)
* style(backup): keep test class spacing valid
(cherry picked from commit 6c6d11411299cafb5c18b020416388b9a52ddfd5)
* test(backup): capture duplicate zip warning
(cherry picked from commit 82fe378fcb26df9a1ee2ae4d9190b912db6e273f)
* test(backup): trim failed-zip-member regressions to two invariants
Keep the automatic-backup partial-member test and the pre-update rotation
test; drop the duplicate-name central-directory test, which exercises the
same _discard_failed_zip_members boundary and pushes the stack past the
two-invariant-test budget for this salvage.
* fix(backup): keep incomplete salvage zips out of retention and the file
Incomplete automatic backups kept the normal <prefix><ts>.zip name, so they
counted toward retention: the next complete run pruned by count and deleted
the last complete backups, and repeated failing runs piled up. Rename them to
<stem>.incomplete.zip, exclude that suffix from _prune_prefixed_zips, and cap
salvage archives at one.
The failed member's bytes also stayed in the file behind a valid local header,
visible to streaming readers as a ghost entry (#124564). Truncate at the first
dropped header and rewind start_dir so later members overwrite it.
Also: name skipped paths in one merged warning, fix a stale comment, drop a
redundant str(), update None-return docstrings, remove an empty duplicate
section heading, and warn in claw migrate when no pre-migration backup was made.
* fix(backup): never publish an incomplete full-zip over the previous backup
_write_full_zip_backup_locked published the partial archive to out_path via
_atomic_output_path and only then renamed it to the .incomplete.zip salvage
name, so an incomplete run destroyed a pre-existing good backup at out_path
(test_zip_captures_live_wal_and_cleans_failed_staging[True] regressed vs base).
_atomic_output_path now takes an optional publish_path callable evaluated at
publish time: the full-zip writer publishes the hidden partial straight to
out_path when clean, to the salvage path when some entries failed, and
discards it (returns None) when every entry failed, since an empty salvage
archive restores nothing. out_path is never touched on an incomplete run.
* fix(backup): cap logged zip-backup errors and clarify salvage messages
Review cleanups on the incomplete-backup salvage path:
- The incomplete / nothing-salvaged warnings joined every per-entry
error into one log line; a broken tree can fail thousands of entries,
so log the first 10 plus "(+N more)".
- Drop the _entry_error helper: its per-entry logger.debug duplicated
the summary warning, so errors are now collected by a plain lambda.
- claw migrate: a None pre-migration backup can mean an incomplete run
whose salvage was kept, so point the user at the possible
pre-migration-*.incomplete.zip instead of claiming there is no
restore point at all.
- Wrap an overlong create_pre_update_backup docstring line.
No partial can land under the complete out_path name: publish is a
single os.replace from the hidden partial, and any failure (including
the replace itself) unlinks the partial in _atomic_output_path.
* fix(backup): decide full-zip publish target once and pin all-failed keep
_write_full_zip_backup_locked chose clean/salvage/discard in _publish_path
and then re-derived the same choice with an inverse test after the with
block. If only one copy changed later, .stat() could hit a path that was
never published and raise out of a "never raises" helper. _publish_path now
records the destination and the stat/return reuse it.
The `destination is None` discard branch in _atomic_output_path had no
teeth: publishing the empty all-failed archive over out_path kept every
test green. The serialization test now asserts an all-failed automatic run
leaves the previous good archive's members unchanged. Also refresh a stale
comment that still described a renamed salvage archive.
* fix(compression): let the next summary build on a fallback handoff
A deterministic fallback summary replaced the older handoff in the transcript but never updated _previous_summary. The next compaction kept the stale in-memory summary and dropped the fallback row from its window, so the fallback's user asks, files and last dropped turns never reached the summarizer. Store the fallback body in _previous_summary the same way a normal summary is stored.
(cherry picked from commit 35417d2e1ffbb775c3eaff17b26623896afa56c1)
* chore: map charan-rathore for salvage of #124116
* fix(compression): avoid long verbatim task quotes in summaries
(cherry picked from commit 006e1d286126835124c3d837c5bd094291c89716)
* test(compression): assert every removed verbatim directive stays gone
The prior check only guarded one of the three quote-forcing directives the
fix removed; a partial revert of '<exact latest user request>' or 'write the
reverse signal verbatim' would reintroduce long-quote stalls unnoticed.
Also call the classmethod directly and hoist the patch import to module level.
* fix(compression): drop duplicated snapshot sentence from task prompt
The historical_task instructions already explain that the compressor inserts
a bounded, redacted snapshot after generation; repeating it in the reverse
signal paragraph only adds prompt tokens.
* fix(gateway): spare a still-booting gateway from the Desktop orphan reap (#122533)
(cherry picked from commit b616766e30c830cf07096f02cedb1ca5098d3433)
* fix(gateway): reuse the shared process-age probe in the orphan reap grace (#122533)
(cherry picked from commit 1661c66c0a9015ad085edb739b580e445725f804)
* test(gateway): pin the orphan-reap startup grace and its no-grace default (#122533)
(cherry picked from commit a00e61075d21e5f473c8f5f550b651a1241677f5)
* test(desktop): pin the boot orphan reap's startup grace (#122533)
(cherry picked from commit 06ac7f42b47aa6324eae6d635b31bbcfd4c548e6)
* refactor(gateway): fold the reap-grace age probe into the filter
The standalone _gateway_process_age_s wrapper only re-wrapped
dashboard_procs._process_age_seconds in a try/except. Inline it as a local
fail-closed predicate (same shape as dashboard_procs._is_stale_orphan) so
the grace lives entirely inside the one reaper that uses it; an
undeterminable age still never widens the reap.
Co-authored-by: Halldrix <halldrix@users.noreply.github.com>
* test(gateway): trim the reap-grace tests to the one invariant
Collapse TestReaperStartupGrace into a single test pinning the grace
invariant: a booting gateway and an undeterminable age are spared under a
positive grace while a stale orphan is still reaped. The no-grace default
is already covered by the existing reaper tests.
* fix(gateway): trim reap-grace docs and dead test state (#122533)
Review cleanups on the orphan-reap startup grace:
- Drop the docstring claim that the Desktop boot sweep is "the one caller
that races a launch": web_server._spawn_gateway_restart also reaps
(grace-less) before its coalesce check, so the claim was wrong.
- Cut the 7-line lifespan comment to one line; the full reasoning lives in
the _reap_unsupervised_gateway_orphans docstring, so the two can't drift.
- Drop the never-asserted seen["extra_exclude"] and the constant-only
`_REAP_MIN_AGE_SECONDS > 0` assert; a grace-less revert already fails the
`seen["min_age_s"] == _REAP_MIN_AGE_SECONDS` check.
Co-authored-by: Halldrix <halldrix@users.noreply.github.com>
* chore: map kylecap9 for salvage of #124604
* fix(kanban): require lexical containment before scratch rmtree
The scratch-cleanup containment guard (#28818) resolved both the task's
workspace_path and the managed workspaces roots before comparing them.
When a root is itself a symlink to a broad directory (storage relocated
to another disk, or a planted link), every path inside the link target
resolves "under" the root. A legacy explicit-path scratch task naming such
a path directly then passed the guard, and task completion, deferred parent
cleanup and artifact persistence treated user data as scratch; completion
rmtree'd it.
Require the path to be strictly below the root lexically (absolute,
normalised, NFC, symlinks not followed) as well as after resolution. Tasks
created through the root are spelled through it, so relocated roots and
symlinked HERMES_HOMEs keep working. The root's lexical form is also
accepted with its anchor (kanban home, or the override's parent) resolved,
so a process that spells a symlinked home by its real path still matches;
the managed kanban/.../workspaces components are never resolved for this.
`hermes kanban gc` calls the same predicate once its own root-deletion
fix lands, so it inherits this check.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 36b1d0453d153307e8d8d10e481e22392332fd2d)
* fix(kanban): gc must never delete the scratch workspaces root itself
`hermes kanban gc` checked archived scratch paths with resolve() +
relative_to(scratch_root), which also accepts the root itself. A scratch
task whose workspace_path is the managed workspaces root (the kanban_create
tool accepts an explicit workspace_path) therefore made gc rmtree every
task's scratch directory once it was archived. Apply the same strict
containment predicate completion cleanup already uses (#28818).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit b61e1fb22e74f6d2aed0d2dfb5122f263b18e78d)
* test(kanban): trim symlink scratch-containment tests to the invariant
Keep test_symlinked_workspaces_root_does_not_widen_scratch_cleanup, which
is red on base and pins the lexical-containment invariant. The board,
override, relocated-root and symlinked-HERMES_HOME cases exercise the same
predicate branch and exceed the stack's two-invariant-test budget.
* refactor(kanban): drop gc's redundant resolved relative_to check
_is_managed_scratch_path now requires a scratch path to be strictly below a
managed workspaces root both lexically and after resolving symlinks, which
subsumes the old resolve()+relative_to(scratch_root) guard. That leftover
check only narrowed gc to the current board's root and rmtree'd the
resolved spelling; gc now deletes the same path, with the same predicate,
as completion cleanup.
Co-authored-by: Kyle Caponi <94931731+kylecap9@users.noreply.github.com>
* fix(kanban): count only real gc removals and resolve kanban home once
gc ran the full managed-root predicate before checking the dir exists, and
rmtree on a symlinked scratch path silently did nothing (ignore_errors) yet
still bumped the removed count. Check is_dir()/is_symlink() first (cheap,
and most archived rows were already cleaned at completion) and count a
removal only when the path is actually gone.
_managed_scratch_path_info re-resolved the same kanban home once per board
root; resolve it once and pass the real anchor into _add_root.
Co-authored-by: Kyle Caponi <94931731+kylecap9@users.noreply.github.com>
* test(kanban): use require_symlinks marker and fix new encoding footguns
Replace the hand-rolled win32 skip with the repo's require_symlinks marker
(skips only when symlinks really can't be created), inline the single-use
helpers, assert the observable outcome instead of the private predicate,
and add encoding= to the new write_text so check-windows-footguns stays at
0 new hits (the read became an is_file() check).
Co-authored-by: Kyle Caponi <94931731+kylecap9@users.noreply.github.com>
* fix(gateway): resolve handoff/loop-watch scopes off the event loop
The 15s _loop_wakeup_watcher and the handoff watcher resolved watch scopes (_handoff_watch_scopes -> profiles_to_serve -> get_active_profile_name -> Path.resolve/realpath + profile-dir scans) synchronously ON the loop every pass. On a memory-thrashing host those syscalls stall past the loop-liveness watchdog 10s probe; 3 strikes -> exit 75 -> every in-flight session/cron is killed (mini wedges 24/9 20:58, 26/9 22:56, 27/9 00:21+00:33; [hermes] stack caught in posixpath.realpath). Resolve the scopes through the runner executor hop with the defensive getattr idiom from run_idle_gates.off_loop_gate (bare test stand-ins keep the historical on-loop resolve). 31 targeted tests green.
(cherry picked from commit 52952abc7f033d352fed69ac88fc1efb46110c55)
* fix(gateway): resolve startup reclaim scopes off-loop; drop dead goals fallback
The handoff watcher's one-shot startup stale-reclaim still resolved
_handoff_watch_scopes on the loop thread; route it through the same
executor hop as the per-tick resolve (shared local helper). The loop
wakeup watcher's getattr fallback was dead — its idle gate already calls
self._run_in_executor_with_context unguarded — so call the hop directly.
Trim the comments (drop host-specific incident notes).
Co-authored-by: Emir Saffar <emir.saffar@uropenn.se>
* test(gateway): pin handoff watcher scope resolve off the event loop
Invariant: both the startup reclaim and the tick resolve watch scopes on
a worker thread, never the loop thread (a stalled profiles_to_serve walk
trips the loop-liveness watchdog). Red on base and on the tick-only fix.
* fix(gateway): skip the executor hop for single-profile watcher scope resolves
The off-loop scope resolve hopped to the executor on every handoff (2s)
and loop-wakeup (15s) tick, even in the default single-profile mode where
_handoff_watch_scopes does no I/O and returns [(None, None)]. The executor
is unbounded (one thread per work item), so that spawned ~34 OS threads a
minute for no work. One helper next to _handoff_watch_scopes now returns
the root poll directly when multiplex is off and only hops for the
multiplex filesystem walk; both watchers use it, replacing the local
_resolve_scopes closure. Config-less test stand-ins still resolve via the
patched resolver.
Also give the run_goals half teeth: the loop watcher's profile-gate test
patched the resolver with a lambda that recorded nothing, so reverting
run_goals stayed green. It now runs with multiplex on (required by the
short-circuit), records the calling thread and asserts off-loop; red on
the pre-fix run_goals.py.
Co-authored-by: Emir Saffar <emir.saffar@uropenn.se>
* fix(cron): refuse to overwrite a corrupt jobs store on save
A merging save_jobs() over an unreadable jobs.json treated the store as
empty (the non-repairing peek returns None and the shrink-merge skips it),
so any save that landed after corruption — e.g. behind a degraded-lock
sibling's non-atomic copy fallback — silently replaced every job on disk.
Fail closed instead: raise and leave the bytes untouched. replace=True
remains the explicit disaster-recovery rewrite, and load_jobs' own repair
(a locked full-store read, so its repaired list is authoritative, incl.
id-keyed maps the peek deliberately refuses to flatten) now uses it.
Also fsync the parent directory after the atomic rename so the published
store survives power loss, via the existing utils.fsync_directory.
Lock-timeout and EXDEV fallback policy are unchanged.
Co-authored-by: Ayushman Padhi <208280836+ayushmanpadhi@users.noreply.github.com>
* fix(cron): fail closed inside the merge peek; keep merge on mergeable repairs
Review fold for the corrupt-store refusal:
- Move the refusal into _unmerged_disk_jobs (after the #80703 stat-stamp
fast path) instead of a separate pre-check in _save_jobs_unlocked. The
pre-check parsed jobs.json on every save, doubling the parse and
defeating the stamp fast path on the scheduler's per-fire saves. A stamp
match already proves disk is the file load_jobs parsed cleanly, and the
in-merge raise also covers the verify-after-stage re-peek (TOCTOU the
pre-check left open). replace=True never reaches it.
- load_jobs' auto-repair uses replace=True only for the two shapes the
peek cannot read (id-keyed map, non-list "jobs" field). Every other
repair keeps the shrink-merge, so a sibling's create that lands during
a repair under the degraded flock-timeout lock is preserved again
(#80624), as it was before the refusal.
- fsync the directory atomic_replace actually renamed into (it resolves a
symlinked jobs.json), matching utils._atomic_write; refresh the stale
comments/docstrings and pin the refusal message in the test.
Co-authored-by: Ayushman Padhi <208280836+ayushmanpadhi@users.noreply.github.com>
* fix(cron): re-check disk before an unmergeable repair replaces the store
A degraded-lock sibling may rewrite jobs.json between load_jobs' read of an
id-keyed or invalid-jobs store and its repair save; only force the replace
while disk is still a shape the merge cannot read.
Co-authored-by: Ayushman Padhi <208280836+ayushmanpadhi@users.noreply.github.com>
* fix(agent): canonicalize todo history pairing
(cherry picked from commit e8900249ad4bfad39e7944bd62825b10930a29c0)
* fix(agent): pair bridged tool_call todo results via one shared predicate
todo_list is in the default tool_search defer list, so with tool search
active the model calls it through the tool_call bridge and the transcript
keeps function.name == "tool_call". The canonical-name pairing check never
matched those, so todos were still dropped across turns (#124960) in every
tool-search-active session, and the TUI resume snapshot had the same gap.
Add agent.tool_executor.is_todo_tool_call: canonicalizes legacy aliases and
peels the bridge from the recorded arguments with normalize_tool_call_entries
(exactly one entry required). It deliberately does not use
resolve_underlying_call, which reads live config and could disagree with the
defer list in force when the history was written. run_agent and
tui_gateway's _todo_state_from_history now share it; the canonicalizer is
public (canonical_tool_name) since it is now used across modules.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
* test(agent): fold todo_list hydration regression into TestHydrateTodoStore
The standalone test file carried an issue number in its name (AGENTS.md
forbids that) and duplicated TestHydrateTodoStore's fixture and assistant
helper. Give _assistant_todo_call name/arguments params and cover the
direct todo_list name plus the tool_call-bridged form in one parametrized
test. The legacy "todo" case is already covered by the existing class tests.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
* fix(agent): keep the todo predicate off the model_tools/executor import path
is_todo_tool_call lived in agent/tool_executor.py and went through
canonical_tool_name, which imports model_tools. TUI resume calls it from
_todo_state_from_history on the RPC path, so the first resume in a
gateway loaded ~405 modules (2-3s) synchronously. tui_gateway/server.py
and run_agent.py also imported agent.tool_executor at module level,
adding ~142 modules to every TUI/desktop launch and breaking run_agent's
lazy-forward rule.
The predicate now lives in tools/todo_tool.py, which both startup paths
already load. It matches TODO_TOOL_NAMES ({TODO_SCHEMA name} + the legacy
aliases) and imports the bridge parser only when a tool_call entry's
args mention "todo". model_tools._LEGACY_TOOL_ALIASES derives its todo
entry from TODO_LEGACY_ALIASES, so there is one source of truth ("todo"
is the only alias mapping to todo_list). The live tool.complete path in
tool_progress uses is_todo_tool_name and the hand-kept _TODO_TOOL_NAMES
tuple is gone. The server.py noqa import is replaced by a function-local
import next to MAX_TODO_RESULT_CHARS, so a pruned name can't be swallowed
by the broad except. run_agent imports lazily. The dead TypeError arm is
dropped, and field reads use message_sanitization._tc_field.
agent/tool_executor.py is back to its pre-stack state.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
* test(agent): cover the TUI resume half of todo_list pairing
The parametrized todo_list hydrate test only exercised
AIAgent._hydrate_todo_store. Reverting the TUI files left every test
green. It now also asserts that tui_gateway.server._todo_state_from_history
returns the same todos for the direct and bridged cases.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
* fix(agent): harden the todo predicate and import the TUI server once in the test
is_todo_tool_name returns False for non-string names (a malformed list/dict
name used to raise TypeError where the old check returned False), and the
kept regression test imports tui_gateway.server at module level so it no
longer depends on another test importing it first. Docstrings updated.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
* fix(auth): treat nous as a single-use refresh pool provider
Nous portal refresh tokens rotate on redemption, so a nous pool row
cloned into a profile forks one grant into two owners and the first to
refresh strands the other. Adding nous to
SINGLE_USE_REFRESH_POOL_PROVIDERS makes profile clone strip, fork heal
and borrowed-row persistence cover it.
Salvaged (auth_oauth_grants.py hunk only) from PR #121781, commit
dc44df5c741; the dashboard_procs.py half is out of scope here.
Fixes #121649
* fix(auth): strip and heal cloned nous providers refresh grant
With nous in SINGLE_USE_REFRESH_POOL_PROVIDERS the pool row is stripped,
but providers.nous still carried the same single-use refresh token, and
nous load_pool/refresh re-seed from that block, so the profile still
forked the grant. Add nous to _DEVICE_CODE_BLOCK_PROVIDERS, read the
flat Nous token shape as well as the nested tokens shape, and only
strip/heal a block that carries a refresh token so an agent_key-only
nous block survives.
Refs #121649
Co-authored-by: salch-cred <salch-cred@users.noreply.github.com>
* fix(auth): keep agent_key-only nous pool rows when stripping/healing forks
Adding nous to SINGLE_USE_REFRESH_POOL_PROVIDERS made the clone strip drop
every nous oauth pool row, including agent_key-only ones, while the
refresh_token-gated block strip kept the matching agent_key-only
providers.nous block. The profile then borrowed root's pool rows and its next
load_pool('nous') seeded its own block over root's shared row, writing the
profile's agent key into root (and every borrowing sibling).
Strip (and heal) a nous pool row only when it carries a refresh_token, the
same predicate the providers-block strip uses. The heal test now also covers
a fork that lives only in providers.nous (flat tokens), which previously had
no test coverage.
* fix(auth): skip auth.json re-read for pool ownership in classic mode
nous now takes the single-use path, so every load_pool('nous') (once per
message plus aux calls) re-parsed auth.json in _profile_owns_pool_provider.
In classic mode (_global_auth_file_path() is None) read_credential_pool has
no root fallback and persist_pool_entries cannot route to root, so the
answer is effectively always "owns": return early.
Also point the persist_pool_entries docstring at
SINGLE_USE_REFRESH_POOL_PROVIDERS and document why nous is deliberately
absent from _SINGLE_USE_REFRESH_PROVIDERS (own auth-store locking).
* fix(auth): don't reseed root's nous grant into a profile that owns nous rows
A profile left with only an agent_key nous row after the fork strip/heal still
"owns" nous but has no local providers.nous block. The next load_pool('nous')
fell back to the global root block in _seed_nous_singleton and upserted root's
single-use refresh token into the profile pool, recreating the fork one load
later (both device_code and manual:* ak-row shapes). Skip seeding from the
global-root fallback when the profile owns local nous rows; borrowing profiles
(no local rows) are unaffected.
Also drop the redundant try/except around _global_auth_file_path() in
_profile_owns_pool_provider; that function already handles its own failures.
The existing nous strip test now reloads the pool after strip and asserts no
profile row carries root's refresh token.
* fix(auth): read flat refresh_token for forkable pool rows; reuse pool ownership in load_pool
_is_forkable_pool_row only ever receives flat credential_pool rows (the
strip loop over pool entries and heal_pool_rows over _pool_rows), so the
tokens-nesting fallback of _block_tokens was dead weight; read
refresh_token the same way _is_oauth_pool_payload does.
load_pool asked _profile_owns_pool_provider (an uncached auth.json read)
twice. Compute it once after the fork heal and reuse it for the
_borrowed_root_ids check unless _persist() rewrote the store in between
(that write can give the profile its own rows). _seed_nous_singleton keeps
its own call: threading the value through _seed_from_singletons would
change a signature that tests monkeypatch with fixed-arity fakes.
* test(auth): pin heal keeping agent_key-only nous rows with a copied root id
heal_pool_rows gates on _is_forkable_pool_row (refresh_token present for
nous) since 364a29d40e, but no test covered it: reverting to the plain
OAuth-payload check left the suite green while the heal deleted the
profile's agent_key-only row that shares root's id. Extend the existing
nous strip/heal test with that shape; it fails with the gate removed.
* fix(auth): drop redundant ownership auth.json reads in nous seeding and load_pool
_seed_nous_singleton re-read auth.json via _profile_owns_pool_provider even
though its only caller (_seed_from_singletons) just loaded the active store
and passed it in; check the passed auth_store through a shared
_store_owns_pool_provider predicate instead (same non-empty-list semantics,
also used by _profile_owns_pool_provider).
In load_pool, borrowing_root_grant repeated the guard that sets
owns_provider (non-None exactly when that guard holds), so test
`owns_provider is False` directly. The tail ownership re-read ran even
with no disk rows, where it could only assign set() -- the constructor
default -- so gate it on disk_ids and re-read only when _persist() ran.
Fix the stale "Computed once" comment.
* refactor(auth): drop impossible auth_store dict guard
* fix(context): keep compaction history lossless
(cherry picked from commit d51c8f4f5096badfd0beddd78646617643f6028f)
* fix(context): assemble compaction head/tail from the pruned copy (#61932)
The salvaged lossless-history change rebuilt the carried head/tail from
canonical history, which undid _pressure_demote_tail's tool-result
shrinking and re-broke #61932 (an all-oversized tail could no longer
compress). Pruning no longer rewrites tool_calls, so the pruned copy's
arguments are already byte-identical to canonical history: assemble the
head and tail from the pruned copy, keeping tool-result demotions and
exact tool-call arguments at once. Docs updated to match.
* test(context): pin byte-exact old tool-call args with one red-on-base test
The PR's two tests passed on the base code (the prune boundary never
reached their calls). Replace them with one invariant test that is red on
base: six old 3000-char write_file arguments must survive the prune
unchanged. Drop the PR's canonical-state test file (tail-canonical shape
contradicts #61932's pressure demotion).
* fix(context): restore marker import in tests, drop unused compressor imports
The arg-truncation removal deleted the only uses of the marker constants in
agent/context_compressor.py (ruff F401), and the test module had been
importing _COMPRESSION_MARKER_PREFIX through it, so test_context_compressor
line 137 raised NameError. Import it from its home, agent.compression_marker.
* fix(context): measure pruned regions, bound arg redaction, drop dead counters
Follow-ups to making tool-call args byte-exact:
- _record_compression_regions measured canonical_messages slices while
compress_start/compress_end are indices into the pruned copy that head/tail
are assembled from; measure the pruned rows actually sent, as before. This
also removes the only canonical slicing, so blank-echo classification drift
between the two copies can no longer misalign anything.
- _render_tool_call_for_summary redacted the full (now unbounded) args before
cutting to 1200 chars; cut to head+4096 first. Output unchanged for args
within that window.
- pressure_hits always equalled demoted once arg truncation left; fold it.
- Drop the fixture-only tautological assert in the guardrail test helper.
- Reword stale compress()/compression_marker docstrings that still described
canonical head/tail and compressor-written arg markers.
* fix(context): redact full tool-call args before the summarizer cut
62ceddd342 cut raw args to HEAD+4096 before redaction to save time. The
PEM redaction pattern only matches a complete BEGIN...END block. A long key
whose END fell past the cut stayed unredacted, and once an earlier key was
redacted and the text shrank, its body landed in the 1200-char head that
goes into the persisted summary. Go back to the BASE order: redact the full
args, then apply the MAX/HEAD cut. This is a cold path (once per summarized
call per compaction), and _SUMMARY_INPUT_MAX_CHARS still bounds the prompt.
Extend the kept canonical-args test with a two-PEM input that leaks on
62ceddd342 and passes now.
* fix(context): drop stale _shrink reference in marker comment
_shrink was deleted along with _truncate_tool_call_args_json, so the
comment pointed at code that no longer exists.
* refactor(context): drop redundant or-empty before redaction
* test(desktop): match the desktop.mjs compile step with native path separators
The --icons case found the compile command with endsWith('/scripts/build/desktop.mjs'),
which never matches the path.join-built argument on Windows, so the case failed with
TypeError instead of asserting. Match both separator styles.
Fixes https://github.com/NousResearch/hermes-agent/issues/125139
* fix(matrix): answer a voice message whose @mention was typed while recording
Element X ships an MSC3245 voice event with an empty m.mentions block and
sends the mention the user typed while recording as a separate m.text
event right after. With require_mention on, the gate dropped the voice and
the follow-up bare @bot was dispatched as an empty text, so the bot never
answered the voice.
Park an unmentioned voice keyed by (room_id, sender) instead of forgetting
it; a bare mention from the same sender in the SAME room within 120s claims
it and the voice is processed in place of the empty text. The claimed event
id passes the mention gate exactly once, so it is not re-parked; expired
entries are pruned on every access. Parked voices are never downloaded or
transcribed (no wake-word/STT of unmentioned audio), and a mention in
another room never pulls a voice across rooms.
The state lives in a small sibling module (voice_mention.py) because
adapter.py is already far past the file-size budget.
Co-authored-by: miregal89 <142085869+miregal89@users.noreply.github.com>
* fix(matrix): pass the voice-mention claim explicitly instead of a claimed-id set
Review cleanups on the parked-voice fix:
- The claim bypass is now an explicit mention_claimed parameter threaded
_handle_media_message -> _resolve_message_context. The old _claimed set was
only drained inside the require_mention branch, so a voice whose thread became
a bot thread while parked leaked its id for the adapter's lifetime.
- One MSC3245 predicate (has_voice_marker, `.get(...) is not None`) shared by
is_voice_event and _classify_inbound_media; the two used to disagree on a
null marker (parked as voice, classified as AUDIO).
- One mention helper (_content_mentions_bot) for both the gate and the
bare-mention claim, instead of two copies of the m.mentions extraction.
- ParkedVoices.has() dict lookup gates the claim, so ordinary text messages
no longer pay _strip_mention/regex work when nothing is parked.
- Test: a same-room mention WITH text stays a text (gives the bare-only guard
teeth) and the parked voice is asserted never downloaded.
Co-authored-by: miregal89 <142085869+miregal89@users.noreply.github.com>
* fix(matrix): let a same-sync-batch bare mention wait for its voice to park
mautrix dispatches every event of one /sync batch as its own task
(wait_sync=True). The voice only parks after awaiting the room identity,
which is a homeserver round-trip whenever the 60s identity cache is stale,
i.e. in any idle room. The bare-mention text checked the park without
awaiting, found nothing, dispatched an empty text, and the voice then
parked and expired unclaimed.
A parkable voice is now marked in-flight per (room, sender) right before
its first await and released in a finally once it is gated. A bare
mention that sees an in-flight voice waits for it (bounded, 5s) before
claiming; ordinary text only pays two dict lookups and never waits.
After a claim the bare-mention event also gets its read receipt, so the
read marker is not left one event short of the pre-claim behaviour.
Cleanups: ParkedVoices drops its unused window param, stores
(parked_at, voice) instead of a flat tuple, the voice_mention import
moves to the top import block, and the redundant require_mention check
on the claim path is dropped (parking/in-flight only happen under it).
Co-authored-by: miregal89 <142085869+miregal89@users.noreply.github.com>
* fix(matrix): track every in-flight voice per sender and release the mark at the park decision
The same-sync-batch in-flight mark was one slot per (room, sender), so a
second voice's begin overwrote the first. When the older voice finished
gating after the newer one, the bare mention settled on (and claimed) the
newer voice only, and the older one parked afterwards as an orphan: the
sender's next unrelated bare mention within 120s re-dispatched it, so a
stale voice got downloaded, transcribed and answered.
The in-flight mark is now a list of gates per key. release removes only
its own gate (idempotent) and drops the key once it is empty; settle
waits on all current gates under the same 5s cap. Each gate carries an
arrival sequence and park refuses an older voice once a newer voice from
the same sender has parked, so a late older voice can neither replace
nor outlive the claim of the newer one (still one parked voice per
sender, newest wins).
The mark was also held across the whole _resolve_message_context,
including the display-name fetch and thread mark, and taken for voices
that can never park (the voice itself @mentions the bot, free rooms, bot
threads, non-allowlisted rooms). A bare mention racing such a voice
waited for the voice's display-name fetch (0.61s vs 0.31s on main with a
0.3s fetch). begin now runs only when a synchronous pre-check says the
voice can park, and the mark is released as soon as the park decision
is made (before the display-name fetch; DM voices release there too),
with the finally still covering exceptions and cancellation.
Co-authored-by: miregal89 <142085869+miregal89@users.noreply.github.com>
* fix(matrix): claim the newest voice sent before the bare mention, not after it
The r3 fold made park refuse an older voice once a newer one from the same
sender had parked. With one /sync batch holding [v1 (slow gate), m1 bare
mention, v2 (fast)], v2 parked first, v1 was then refused, and m1 claimed
v2 -- a voice sent after the mention. v1 was lost and v2's own mention was
answered as empty text.
The bare mention now takes an arrival limit (ParkedVoices.mark) before it
settles, and claim pops the newest parked voice that began before that
limit, dropping older ones. park no longer refuses by arrival; parked
voices are kept per sender ordered by seq (bounded to 4). A claim made
while gates are still in flight records a floor, so a late older voice
(seq <= claimed) cannot park and outlive the claim; the floor clears when
the sender's in-flight list empties. One answer per bare mention, newest
before the mention wins, and the r3 orphan case still leaves nothing
parked.
Also: pending() ignores entries past CLAIM_WINDOW_SECONDS so an expired
voice no longer sends every later text through the mention regexes; the
m.thread root lookup is one _thread_root helper used by both the park
decision and its pre-check; voice_gate is annotated.
The kept test gains a [voice slow, mention, voice2 fast] + mention2 row:
red on a932dc031c (['$voice2', '$text2']) and on f1ea71de7c
(['$voice', '$text2']).
Co-authored-by: miregal89 <142085869+miregal89@users.noreply.github.com>
* docs(matrix): note the parked-voice cap limit
Co-authored-by: miregal89 <142085869+miregal89@users.noreply.github.com>
* docs(matrix): fix the parked-voice cap limit count
* fix(desktop): keep model rows clickable before pointer movement
* fix(desktop): resolve default Kanban board before event subscription
* fix(desktop): preserve alias task cache invalidation
* style(kanban): satisfy the curly and blank-line lint rules in the alias-resolution change
Mechanical eslint --fix over the two files this branch touched; no behavior
change. 35 kanban notify tests still pass.
* fix(desktop): don't announce a reconnect when the app is quitting
Quitting the Desktop app wrote "[boot] Restarting desktop connection" to
desktop.log and pushed the same message to the renderer over
hermes:boot-progress. Nothing was restarting: the quit coordinator called
teardownPrimaryBackendAndWait() with the default soft=false, and soft=false
is what makes resetHermesConnectionState() call
resetBootProgressForReconnect().
Name the two teardown intents in quit-teardown.ts and use them at every
deliberate primary teardown: a teardown that re-homes ('reconnect', update
hand-off and bundle swap) keeps the announcement; one that brings nothing
back ('quit', the quit coordinator and the uninstall teardown) stays silent.
Fixes #123437.
* fmt(js): `npm run fix` on merge (#125310)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fmt(js): `npm run fix` on merge (#125322)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* test(gateway): regression for busy re-queue hot loop in adapter drain
A busy-demoted event that the runner puts back into the adapter pending slot
is re-dispatched by the in-band drain with zero delay (~400/s). Salvaged from
PR #123259 (test only; the fix is rebuilt separately).
Refs #123229
(cherry picked from commit f83f4c958514c0be6e3f37af9d6ac5afc62fdf16, test file only)
* fix(gateway): back off busy re-queued events inside the drain task
The runner busy-demotion puts the event back into the adapter pending slot and
returns None; the in-band drain re-dispatched it at once, ~400 times/s for the
whole busy window (typing churn, log flood, platform connection storm).
Count consecutive unanswered re-queues per session on the adapter (not by event
identity, so a pre_gateway_dispatch rewrite via dataclasses.replace is still
caught), reset when a handler returns a response. First re-queue stays
immediate (restart auto-resume self-bounce); later ones back off 0.25s..5s.
The delay is slept inside the new drain task before its processing
try/finally, so cancelling during the back-off cannot reach
_finish_session_task late-arrival respawn (no concurrent handler, no
untracked task on shutdown).
Fixes #123229
Co-authored-by: ahisblessed <ahisblessed@users.noreply.github.com>
* fix(gateway): back off only runner-demoted events, not every None turn
The drain back-off keyed on `response is None`, but None is the normal
return for every streamed turn and for queue/steer busy modes, so
ordinary chained follow-ups were delayed 0.25s -> 5s forever. The busy
fast-path in run_inbound now tags the adapter's pending head
(`_busy_requeued`) where it demotes the same inbound event (or its
rewrite-hook copy) back into the queue (interrupt demotion, queue mode,
steer fallback); the drain backs off only for a tagged event and
otherwise resets the counter and dispatches at once — single reset owner.
Also: clear _requeue_counts on cancel_session_processing, stale-lock
heal, session end and shutdown; restore the pending event if cancelled
during the back-off sleep; reuse agent.retry_utils.jittered_backoff.
The regression test now chains 3 genuine follow-ups after a streamed
(None) turn and requires each to dispatch in <0.1s (red on pre-fold).
* fix(gateway): back off only when the dispatched event bounces straight back
The _busy_requeued tag was reset only by an untagged drain, so chained queue/steer
follow-ups reaching the runner fast path (split-brain adapter, multiplex routes) backed off
exponentially again (0.05->2s gaps vs ~0.01 on main). Key the back-off on identity instead:
the drained event must be the one this task just dispatched (same object or same message_id
for rewrite-hook copies). That covers every demotion site (interrupt demotion, queue, steer
fallback, Telegram grace queue, agent-starting merge) with no per-site tagging, so the tag
and _hm_tag_busy_requeue are dropped.
A backed-off event now stays in _pending_messages during the sleep and is popped after it,
so a cancel needs no put-back and can no longer drop the older message when a newer one
took the slot. jittered_backoff import moved to module top (agent.retry_utils is stdlib-only).
* test(gateway): harden busy re-queue hot-loop regression
Measure the 1s window after the first dispatch so a cold first handler call under load
cannot false-green the no-fix mutation; parametrize over interrupt demotion and steer
fallback; cover cancel during back-off keeping the event queued and the /stop tail
replaying it. Reuse RestartTestAdapter instead of a duplicate adapter.
* fix(gateway): keep swapped command guards and id-less rewrites bounded in drain back-off
The back-off drain's slot-empty exit released whatever guard was current, so a
/stop, /new or /reset guard swapped in during the sleep was deleted, defeating
the #48300 guard-swap protection. Capture the guard owned by the drain at spawn
and release only that; also flush the text debounce buffer before popping the
slot, like every other task exit, so a debounced text isn't orphaned.
A pre_gateway_dispatch rewrite copy of an event with no message_id was never
matched to the dispatched event, so it still hot-looped (#123229). Fall back to
the copied timestamp when message_id is empty (a genuine new message gets a
fresh one); a plain message_id==/timestamp== form breaks the runner's re-queue
of a new object with the same id.
Cap the back-off at 1s: nothing wakes the sleep, so a genuine message merged
into the slot meanwhile waited up to 5s; 1 dispatch/s is still ~250x below the
unbounded loop and needs no new wake plumbing.
* test(gateway): deflake busy re-queue drain test and pin follow-up identity
Test 2 waited a fixed 0.6s before cancelling, which under load landed mid
first-handler and raised KeyError; poll for a real back-off instead. Route
chained follow-ups through the runner mid-turn and assert they dispatch
immediately, which fails (0.25s/0.5s gaps) with the identity check disabled.
Add an id-less rewrite-copy case to the hot-loop parametrization and drop the
typing assertion that could never fire in this harness.
* fix(gateway): tighten busy re-queue back-off identity check and test matrix
Review cleanups on the drain back-off:
- `_drain_after` now requires `guard`: a None default silently meant "release
whatever guard is current", which is the guard-swap bug the parameter fixes.
- The identity rule is stated once (docstring, wrapped), and the redundant
`pending_event is dispatched_event` disjunct is dropped: the same object
always has an equal message_id, and when that id is empty its timestamp
equals itself, so the remaining comparison already covers it.
- Tests drop the `_Adapter` alias and cut the hot-loop matrix from 6 to 4
explicit cases (plain, rewrite with id, rewrite without id, steer). The
demotion route does not interact with the identity axis, and each case
spends a fixed 1s measuring window.
* chore: map Nagisa-3000 for salvage of #123849
* fix(persistence): repair inactive transcript rows in place
(cherry picked from commit 1d33010a27fbd7f053e1037269b6059edaef77ef)
* fix(persistence): repair transcript rows for all roles
(cherry picked from commit 7cebc297b145c0e3db007c1f05ea348a626fd38d)
* fix(persistence): preserve transcript row identity safely
(cherry picked from commit a1f28d54482eab32bd119fbc856c6e910c595ba9)
* fix(persistence): import Optional where transcript_repair uses it
transcript_row_snapshot annotates Optional, which was only reachable via the
PLUGIN-COMPAT re-export block at the bottom of the module. Internal code must
not depend on that revert-scheduled block, so import it with the other typing names.
* test(persistence): trim sanitized-row dedupe tests to two invariants
Keep one test per invariant: active user/tool rows whose _db_persisted marker
was popped by the outbound sanitizer keep their _row_id and are not re-inserted
(the #123462 desktop/serve path), and archived user/tool rows are repaired in
place instead of appended. Both fail on b4410b4bad; the other four PR tests
covered edge branches and are dropped per the <=2 invariant-test budget.
* fix(persistence): version transcript rows by digest, not a row copy
The CAS row snapshot was a full copy of each message's durable payload
riding on the live dict. The rough token estimator priced it (about 2x
estimates -> premature compaction) and it doubled transcript memory.
Replace it with a 16-byte blake2b digest of the repair columns. The
compare now runs in Python against the target row already read inside
the BEGIN IMMEDIATE transaction, followed by a plain UPDATE. Also:
- add _db_row_snapshot to PERSISTENCE_ONLY_MESSAGE_FIELDS so the
estimator and the outbound request builder both drop it
- derive _REPAIR_COLUMNS/_SYNC_FIELDS from _MESSAGE_WRITE_COLUMNS
- use hermes_state_common._placeholders
- drop the dead resume-path stamp (the SELECT has no token_count, so it
was always None) and the dead tool name assignment in
_decoded_repair_row
- keep the digest out of divert JSONL
The kept active-row test now pins estimate stability across a flush and
the survival of a concurrent writer's row. It goes red on the old
prod files and red when the digest compare is removed.
* fix(persistence): keep live multimodal content and hash stored rows
The row-addressed repair stamped the decoded durable row on every
resolved message, so after our own rewrite the sync copied the lossy
durable projection (image parts -> "text\n[screenshot]") back onto the
live dict: multimodal user/tool messages lost their images and the
prompt-cache prefix changed. Adopt the DB row only when another writer
won (digest mismatch) or on the legacy assistant path, as BASE did.
The insert-time digest hashed Python bind values, but SQLite affinity
rewrites them on storage (int message_id -> TEXT, float token_count ->
INTEGER), so live and DB digests never matched and in-place edits were
silently dropped. Hash the stored rows instead, only on the
append_messages_batch flush path that reads the digest (one SELECT per
batch), incrementally (type tag + length prefix) instead of via JSON.
Also skip the no-op UPDATE, fix the _write_columns comment/spacing and
drop the duplicate top-level Optional import (F811).
* fix(persistence): strip the adopted canonical row from provider payloads
Gateway/TUI/CLI callers pass their live dicts straight to
append_messages_batch, so a concurrent-winner adoption leaves the
decoded durable row (_canonical_row) on a dict that may later be sent
to the model. Treat it as persistence-only like _row_id and the digest
so the outbound builder and token estimator both drop it.
* fix(persistence): keep live content on metadata-only row changes
Same-process writers (set_message_reaction, display-kind stamping, api_content
backfills, codex reasoning update) change stored columns after a flush without
refreshing the live row digest. The next sanitize + re-flush treated that as a
concurrent winner and copied the lossy durable projection over live multimodal
content, dropping image parts and shifting the prompt-cache prefix. On adoption
we now keep live content when the stored content is just the durable form of
it and sync metadata only; a real concurrent content winner is still adopted.
The legacy blank-assistant path (dict with _row_id but no digest, blank DB row)
again only fills the row from live content, as on main, instead of running the
full canonical sync that wiped live reasoning_content/finish_reason/tool_calls.
Adoption on the legacy path is limited to a non-blank row.
Also: transcript_row_snapshot returns str (the partial-row branch had no
caller), serialization only runs on the digest-match branch that reads it,
stamping reuses hermes_state_common._id_chunks, and _MESSAGE_WRITE_COLUMNS is a
plain top-level import (hermes_state_messages imports this module lazily, so
there is no cycle).
* test(persistence): pin metadata-only adoption and stored-row digests
Extend the kept active-row test: a reaction between flushes must not replace
the live multimodal user content with its text projection (red on the previous
tip at the image assertion), and the reaction metadata is synced. The user row
carries an int message_id so the stored-row (TEXT affinity) digest path is
exercised.
* fix(persistence): version only owned columns so metadata writes keep row ownership
The row digest hashed every repair column, so a same-process metadata write
(reaction, display-kind stamp, api_content / codex reasoning backfill,
platform message id) made our own row look like a foreign winner. The
re-flush then adopted the stale DB row: a later live edit (the non-ASCII
strip recovery) was reverted, and unsanitized tool_calls/reasoning were
copied back onto the live dict.
The digest now covers only the owned (non-metadata) columns: it means "the
row is still what we last committed". Match -> write the live owned values
and hand over only presentation metadata the live dict lacks; mismatch ->
genuine other writer, adopt as before. The r3 "stored content equals the
durable form of live" special case is subsumed and removed.
Also:
- _insert_message_rows drops a carried digest when it assigns a new row id
(compaction/replace/import clones carried the parent's version).
- append_messages_batch restores each message's _row_id / digest /
timestamp and pops the adopted row at the top of every _execute_write
attempt, so a rolled-back attempt cannot resolve to a foreign row.
- message_id is no longer synced onto live (int -> str flip, spurious
platform_message_id).
- The JSONL divert strips both bookkeeping keys via one frozenset.
* fix(persistence): adopt content only on legacy rows and stamp every inserted row
A legacy (no-digest) dict over a non-blank assistant row adopted the whole
decoded DB row: tool_calls / reasoning* / codex_* were overwritten with the
stored JSON (which still holds the escaped lone surrogate the sanitizer just
fixed, re-injecting it into the provider payload) and live-only fields were
popped. Resumed dicts (_rows_to_conversation stamps _row_id without a
digest) and compaction clones hit this path. Adopt content only, as before
this stack, via a content-only canonical handled like the metadata-only one.
_insert_message_rows dropped a clone's parent digest but only the flush
path restamped it, so clones made by archive_and_compact / replace /
rotation handoff / import reached the legacy path and the first live edit
after a clone was not persisted. Stamp the stored-row digest inside
_insert_message_rows (one batched SELECT, cold paths only; the flush path
statement count is unchanged) and drop the duplicate call in
append_messages_batch.
Define the _db_row_snapshot / _canonical_row keys once in
agent/message_metadata.py and import them everywhere instead of repeating
the literals.
* fix(persistence): restore caller row state when any transcript insert rolls back
_insert_message_rows stamps _row_id and the stored-row digest onto the
caller's dicts inside the write transaction. Only append_messages_batch
restored that state on rollback. archive_and_compact, replace_messages
and the rotation handoff left the rolled-back id + digest on the dicts;
SQLite reuses the id, so a later flush found a digest mismatch on the
foreign row, adopted it and silently dropped the user's message.
Move the capture/restore into _execute_transcript_write, used by every
caller that inserts caller-owned dicts: each attempt starts from the
caller's state and a final failure restores it before re-raising.
(Rewind replacement and import insert dicts built inside the txn.)
Also: bind _message_row_params directly on insert instead of the
serialized-dict round-trip, import the public DB_ROW_SNAPSHOT /
CANONICAL_ROW names, set adopt=False once, and reuse target_row
instead of re-SELECTing when nothing was written.
* test(compression): expect the rotation handoff digest on compressed dicts
The rotation handoff now stamps each child row's stored-row digest, so
the exact-dict comparison must ignore DB_ROW_SNAPSHOT. Assert every
compressed dict carries one: this pins the non-flush restamp, which
otherwise only probes covered.
* refactor(persistence): drop a dead digest pop and scope the transcript-write docstring
* fix(files): preserve SQLite locks during previews
Co-Authored-By: Hermes Agent / OpenAI Codex / gpt-6-sol <noreply@agents.invalid>
(cherry picked from commit 60d879c125177cd4cacdbc840f16c2e048b4199c)
* test(files): prove cross-process SQLite writes survive preview
Co-Authored-By: Hermes Agent / OpenAI Codex / gpt-6-sol <noreply@agents.invalid>
(cherry picked from commit 915447ca9d4de7e9d4f9f30c52fd2f1814619569)
* test(files): trim live-DB preview lock matrix to four cases
Keep one case per guarded route (@file, @folder, desktop fs_read_text)
plus one WAL-sidecar refusal (-shm); the remaining alias/wal variants
exercise the same offline_file_access keying path and only add runtime.
* test(files): gate live-DB lock test with platforms("linux")
The PR predates the removal of the linux_only marker; the collection hook
now rejects it outright, so the whole module errored at collection.
The test reads /proc/locks and is genuinely Linux-only.
* fix(files): refuse live-DB downloads and share one sidecar-aware liveness check
FileResponse opens and closes the file in the dashboard process, so
downloading a live state.db (or its -shm/-wal) via /api/fs/download or
the managed-file read/download/media routes still cancelled the
connection's POSIX locks. Both now return 409 via is_live_database_file;
the registry lock is not held across the streamed response.
The main-or-WAL-sidecar rule now lives in one _live_main_key helper used
by offline_file_access, has_live_connection and read_header_bytes_preopen,
and the sidecar refusal names the main database the connection is open on.
@file previews hold _live_lock only for the raw read; token counting and
formatting run after release. The Linux lock test gains requires_wal
(Hermes uses DELETE mode on WAL-reset-vulnerable SQLite), covers the
download refusal, and drops an ambiguous conditional assert.
Co-authored-by: Benjamin PERRY <benjaminperry6@yahoo.fr>
* fix(files): one live-DB refusal source; hold the lock through /api/files/read
Gate r2 Low cleanups (house rule: no aliases/shims):
- Drop the is_live_database_file alias; its point-in-time caveat now lives on
has_live_connection.
- _refuse_live_database reuses offline_file_access's message (via _serve_offline),
so a download 409 on state.db-shm names the main database like the read path;
the verb is "serve" so it fits read/download/stream.
- /api/files/read reads whole files in-process, so _read_base64_file now holds
offline_file_access through close (409 on a live DB; OSError stays 500). Only
the streamed FileResponse routes keep the point-in-time check.
- That check takes the global _live_lock, which other threads hold across
whole-file reads, so fs_download and the managed stream routes run it via
asyncio.to_thread instead of stalling the event loop.
- _managed_readable_file docstring no longer claims a size cap;
_read_file_reference returns (early, text) instead of a str|Expansion union
sniffed with isinstance.
Co-authored-by: Benjamin PERRY <benjaminperry6@yahoo.fr>
* refactor(files): share the 409 mapping with the preview read; encode base64 after release
Co-authored-by: Benjamin PERRY <benjaminperry6@yahoo.fr>
* fix(security): remove mutable display-name from SimpleX allowlist check
The SimpleX sender allowlist (SIMPLEX_ALLOWED_USERS) previously matched
against both the stable numeric contactId (user_id) and the mutable
display name (user_name). Since any SimpleX contact can change their
localDisplayName / profile.displayName to match another user's, this
allowed an unauthorized contact to bypass the allowlist by setting a
colliding display name.
Remove the user_name check so that SIMPLEX_ALLOWED_USERS only matches
on the immutable contactId. Operators must use numeric contact IDs in
the allowlist.
Fixes #44729
(cherry picked from commit b4aa29da1567d45920f79aabdb36b44c5f87bde5)
* test(simplex): trim salvaged allowlist tests to the two invariants
Drop test_simplex_allowlist_rejects_colliding_display_name: it passes on
the unfixed base (the allowlist held the contactId, not the colliding
name), so it never guarded #44729. Drop the setup-prompt string check as a
change-detector. Keep rejects_display_name_only (red on base) and
accepts_numeric_contact_id (contactId path still works).
* fix(simplex): document contactId-only allowlist and warn on name entries
After #44729 SIMPLEX_ALLOWED_USERS matches only the numeric contactId, but
the docs still told operators display names work, and existing name
entries would silently stop matching. Update the docs and log a one-time
warning at first connect listing non-numeric entries that are now ignored.
* fix(simplex): read allowlist name-warning profile-scoped, before the probe
The name-entry warning in connect() read SIMPLEX_ALLOWED_USERS via raw
os.getenv, while authz reads it profile-scoped. Under m…
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
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.
An optional
heartbeatproperty hadminimum: 60and no disabled value. Providers that materialize every tool-schema property therefore turned an ordinary foreground call into:{"command":"pwd","background":false,"notify":false,"heartbeat":60}The terminal handler rejected that shape before execution. The returned correction could then produce either repeated identical failures or unnecessary background calls and completion notifications.
This change makes
0the explicit schema minimum and default for a disabled heartbeat. Positive values keep their existing behavior: foreground calls still reject them, while background calls clamp intervals below 60 seconds to the existing floor. The regression test derives the heartbeat value from the schema minimum and proves a fully materialized foreground call reaches terminal dispatch without starting a background process.Fixes #119196.
Validation:
mainand green with this change.scripts/run_tests.sh tests/tools/test_process_heartbeat.py tests/tools/test_notify_on_complete.py tests/tools/test_terminal_tool.py tests/tools/test_schema_sanitizer.py -qgit diff --check