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