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>
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>
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>
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>
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>
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>
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>
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.
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>
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)
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>
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>
_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>
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.
`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)
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)
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>
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.
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>
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.
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.
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)
_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.
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.
_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.
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.
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.
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.
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.
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).
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)
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)
## 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)
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.
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.
