- Single source for the approval-derived bound: public human_wait_ceiling()
in tools/approval.py; the gate's lock-timeout helper delegates to it
instead of re-deriving timeout + margin (was duplicated in two modules
and reached for a private _get_approval_timeout).
- Shared _clamped_window_seconds() for the close-time accrual and the
open-window read, so the two clamps are identical by construction.
- Gate __init__ grows session_key kwarg; tests construct via the real
constructor instead of mutating privates post-hoc.
- Gateway test resolves its pending approval via resolve_gateway_approval()
(the production /deny path) instead of hand-rolling queue-entry internals.
- Docstring accuracy: human_wait_seconds monotonicity caveat under cap
eviction; s/pre_tool_block/pre_tool_call/ hook name.
Review-driven follow-up to the #79719 fix:
- Clamp the CLOSE-side accrual too: a wedged window that eventually closed
used to inject its full unclamped overstay into completed_seconds,
retroactively extending a running batch's deadline by hours. Both clamps
now share one ceiling helper (_human_wait_ceiling = approvals.timeout +
HUMAN_WAIT_MARGIN_S), and the gate's lock-timeout uses the same margin
constant so the bounds cannot drift apart.
- Evict idle sessions until the table is under the cap (was: at most one
per insert, so churn could outgrow _HUMAN_WAIT_MAX_SESSIONS). Entries
with an open window are still never evicted.
- Log (debug) instead of silently swallowing a failed session-key snapshot
in the gate constructor.
Tests: close-side clamp regression + table-cap assertion added; suite at
17 passed.
A tool wedged inside _ConcurrentToolAuthorizationGate hung the whole turn
forever (#79719): excluded_seconds() measured residency in gate.run() —
arbitrary code — so an open window grew 1:1 with wall clock and the batch
deadline's remaining was constant (remaining = deadline - window_started;
now cancels out). A hanging pre_tool_call plugin or an approval round-trip
to a dead client defeated the deadline entirely. The serialization lock was
also an unbounded acquire, so every other worker needing authorization
parked behind the wedged holder forever.
Fix, in two halves:
- tools/approval.py grows per-session human-wait accounting
(human_wait_window / human_wait_seconds). The two places that are
verifiably blocked on a HUMAN — the CLI approval prompt and the gateway
approval poll loop — mark their own windows. Both are intrinsically
bounded by approvals.timeout; the open-window read is additionally
clamped to that timeout plus a margin as belt-and-braces.
- _ConcurrentToolAuthorizationGate keeps only serialization, with a bounded
acquire (approvals.timeout + 60s; on expiry the prompt runs unserialized —
the same degradation the start-order gate accepted in #79705).
excluded_seconds() becomes a baseline-delta read of the session's
human-wait total.
A wedged plugin now contributes nothing to the exclusion, so the batch
times out at the normal deadline with correctly labeled results, while a
genuine approval wait — which can legitimately exceed any fixed bound —
still extends the deadline in full. E2E (real AIAgent, worktree imports):
wedged-plugin batch on main never ends (>30s observed, 3s deadline); with
the fix it ends at 3.0s. A 4s simulated approval over a 2s deadline
completes without a timeout label.
Closes#79719
Follow-up to c0d974b19 (#79741). Three review findings against that commit,
none of which change the escalation behaviour it shipped.
1. The recovery decision lived inline in `_handle_message_with_agent`, a
~2000-line async method, so the only way to pin it was a test that read
`inspect.getsource(...)` and asserted on substrings. AGENTS.md bans reading
source in tests outright and names this exact situation: "if the logic lives
inline in a god-file (gateway/run.py) and extracting it feels disruptive:
that's the actual signal to do the extraction, not to regex around it."
Those tests were not merely stylistically wrong, they were actively harmful.
One asserted the substring `_new_tokens < _approx_tokens` was PRESENT -- so
it passed while the gate had the bug that substring represents, and had to be
edited when the gate was fixed. It failed on correct code and passed on
broken code, in one assertion.
Extracted `hygiene_compaction_recovered()` as a module-level pure predicate
and replaced the three source-reading tests with eight direct unit tests.
The extraction immediately earned itself: the new tests caught a `NameError`
(the predicate called `compression_made_progress` while the module bound it
under an alias) that a source-text assertion cannot see, because the symbol
is spelled correctly in the source and only fails at runtime.
2. The gate inferred "did the transcript actually get rewritten" from a numeric
side effect -- the degenerate "did not rotate or compact in place" path
(#21301) reuses the pre-compression counts -- when the booleans
`_hyg_rotated` / `_hyg_in_place` were already in scope and explicitly set
False on that path. The predicate now takes them directly, so a future edit
that re-estimates instead of reusing the old counts cannot silently defeat
the escalation.
3. `_record_hygiene_cooldown` passed no `error` to
`record_compression_failure_cooldown`, which writes `compression_failure_error`
unconditionally -- so a hygiene failure clobbered to NULL whatever reason the
in-conversation path had recorded, and readers then show the user "unknown
error" (agent/manual_compression_feedback.py, gateway/slash_commands.py). The
reason was already in hand at both call sites. Pre-existing from #74136 but
amplified by escalation: a blank reason on a 45-minute cooldown is far more
user-visible than on a 5-minute one.
Also: the ladder docstring described the compressor's absolute 60/300/900s
ladder while the constant is multipliers (1, 3, 9); the config docs still
described `hygiene_failure_cooldown_seconds` as a flat interval rather than the
first rung of a capped ladder; and `PersistentState.hygiene_failure_streak` now
documents that it is process-local by design -- keying on `session_key` is what
survives compaction rotation, which the persisted `compression_*_streak`
columns cannot express since they key on the rotating `session_id`. Making it
durable is a schema change, tracked on #79624 rather than smuggled in here.
Also replaces the file's hand-written `_Runner` stub with
`object.__new__(GatewayRunner)` (already the idiom elsewhere in the same file).
The stub reimplemented `_session_state` and `_peek_session_state`, so the tests
exercised copies that could drift from production; using the real class
immediately made one assertion stronger -- on a fresh runner `_sessions` does not
exist at all until something materialises it, so the reset provably did not even
create the map.
A review pass on this follow-up then caught that the CALL SITE was still
unbound: deleting the whole `if not _hyg_aborted: if
hygiene_compaction_recovered(...)` block left every ladder test green, because
the unit tests prove the predicate correct without proving it is wired in. The
merged commit had the same gap and its only cover was the banned source-reading
test. `test_session_hygiene_forces_in_place_compaction_with_bound_session_db`
now spies the reset on a genuine in-place compaction, so deleting the wiring
fails. Two earlier attempts at this test did NOT close the gap -- asserting on
streak VALUES passes either way, since the streak is 0 whether or not the gate
ran; only a positive spy assertion on a recovering run detects the deletion.
Same pass also corrected an overstatement: point (2) is hardening, not a live
bug. The degenerate path also sets `_new_count = _msg_count` and `_new_tokens =
_approx_tokens`, and `compression_made_progress(n, n, t, t)` is always False, so
the merged code already declined to reset there. A 200k-trial fuzz over the
reachable state space found zero behavioural disagreements between the merged
gate and this one. The guard's value is surviving a future edit that stops
reusing those counts.
Tests: 28 in tests/gateway/test_hygiene_failure_cooldown_ladder.py (8 new unit
tests for the predicate, 3 for reason forwarding, 3 source-reading tests
deleted). All 5 mutations caught -- including one that restores the hand-rolled
comparison and one that removes the rotated/in_place guard. Two mutations
initially SURVIVED and exposed vacuous tests of my own: the no-rewrite test used
counts the progress predicate already rejects, so it passed without binding the
guard at all; it now passes counts that read as progress on their own, proving
the guard is what rejects them. gateway hygiene + session-state + agent
compression-progress suites: 54 passed; ruff clean.
Refs #79624
/refine [focus] fires the existing background review fork
(AIAgent._spawn_background_review) immediately instead of waiting for
the automatic 10-turn memory / 10-iteration skill nudge counters.
Optional focus instructions are appended to the review prompt so the
fork prioritizes what the user asked for (e.g. '/refine save the
deploy workflow as a skill').
- New optional focus parameter threaded through
_spawn_background_review -> spawn_background_review_thread.
Automatic post-turn reviews pass None and their prompts are
byte-identical to before.
- CLI handler snapshots conversation_history; gateway handler pulls
the idle session's cached AIAgent from _agent_cache (rejected while
the agent is running).
- Review runs in a daemon thread against the snapshot — live
conversation, message alternation, and prompt cache untouched.
- Slack stays under the 50-slash cap via /hermes refine.
Adapted from the /refine concept in Prime Intellect's Prime-Agent
(Continual Harness); Hermes' equivalent durable state is the
memory + skill stores, so the review fork is the natural target.
/heartbeat every <interval> <prompt> gives the current session one
recurring instruction. When the session is idle and the interval has
elapsed, the prompt is injected as a plain user turn — same
conversation, same context, prompt cache and role alternation
untouched.
- CLI: idle-poll watchdog thread (wake-word watchdog pattern) feeding
_pending_input; gateway: single gateway-wide async poller injecting
through the adapter FIFO. Busy sessions coalesce their tick to the
next idle poll.
- Missed ticks coalesce (anchor resets on fire) — a busy hour yields
ONE heartbeat turn, never a backlog. Real user messages always win.
- 60s interval floor; injected prompt carries a don't-invent-work
guard so idle heartbeats don't generate busywork.
- State persists in SessionDB.state_meta (heartbeat:<session_id>),
survives /resume, migrates across compression session rotations
alongside /goal state.
- Session-scoped and in-process by design — durable cross-process
schedules remain the cron subsystem's job (docs draw the boundary).
- Slack stays under the 50-slash cap via /hermes heartbeat; ghost-text
suggester now prefers the shortest prefix match so /he still
suggests /help.
Adapted from the session-heartbeat concept in Prime Intellect's
Prime-Agent (/heartbeat).
/goal gate add <command> attaches shell commands to the active goal.
Gates run at turn boundary BEFORE the LLM judge: a failing gate skips
the judge entirely and feeds its exit code + bounded output tail back
as the continuation prompt, so the agent iterates against concrete
evidence instead of a prose verdict.
- Unchanged-workspace skip: a gate that failed on an identical
workspace (git HEAD + status fingerprint) is not re-run — the
recorded failure replays and the attempt count advances.
- Bounded retries (default 3) + per-gate timeout (default 300s);
exhaustion auto-pauses the goal like the turn budget does.
- Gates persist in SessionDB.state_meta with the goal (survive
/resume and compression rotation); pre-gate goal rows load
unchanged.
- /goal gate [list|add|remove|clear] on CLI + gateway; 'gate' added
to the mid-run control-verb whitelist (gates only run at turn
boundary, so editing the list mid-run is safe).
Adapted from the quality-gate concept in Prime Intellect's Prime-Agent
(--autonomous-gate).
The read_file description added in #79781 states that PDF, legacy
Office, OpenDocument, RTF, and EPUB convert via the optional anydoc
converter, unconditionally. Conversion actually depends on the lazy
install succeeding, security.allow_lazy_installs, and the file being
readable from the Hermes host, so the schema overpromises and the model
learns to expect conversion in environments that can never provide it.
The description now says these formats convert when the optional anydoc
converter is available, and that the auto-install applies where
installs are permitted.
The anydoc path from #79781 passed every covered file straight to
to_markdown with no pre-check. anydoc loads the whole document through
its Rust core and the read_file char budget only applies after
conversion, so one large PDF or deck could pin a tool turn and spike
RAM.
_extract_anydoc now rejects inputs over MAX_ANYDOC_BYTES (50 MB) with
ExtractionError before calling the converter, which routes them to the
existing read_file fallthrough instead of converting. No timeout is
added: the conversion is a synchronous Rust call that cannot be
cancelled from Python, so a thread-based deadline would bound the wait
but leave the RAM burn running in the background.
The first _anydoc() load cached None on any failure (network blip,
missing wheel, pip race), so one bad first try disabled document
extraction for the rest of the process. Long-lived gateway and desktop
workers never recovered.
Failed loads now cool down for ANYDOC_RETRY_SECONDS and retry instead
of sticking, and a lock serializes first use so parallel readers cannot
double-install or race a failure into the cache. Successful loads are
still cached for the process lifetime.
Port from google-gemini/gemini-cli#28700: when an interrupted/failed turn
leaves history ending on an unanswered tool result and the user sends a new
message, fusing the two into one Gemini user content makes the model read the
trailing text as a continuation of the tool result — it 'finishes your
sentence' instead of answering.
Builds on #68863 (@rille111), which split the mixed functionResponse/text
merge but emitted two consecutive user contents — a shape Gemini's
alternation contract rejects with HTTP 400 on other request paths (#55125).
This follow-up interposes gemini-cli's INTERRUPTED_RESPONSE_PLACEHOLDER model
turn between the split contents so the request stays alternation-valid while
the user's message remains a turn of its own.
Do not fold a human user text turn into a preceding functionResponse
user content. Gemini 3 accepts that fold with HTTP 200 but then returns
an empty model response.
Contract:
- ordinary same-role merges remain (parallel tool results, back-to-back
plain user texts) for Gemini alternation
- only mixed functionResponse/text user turns are split
test_hermes_state.py was the slowest file in the suite (46.7s in CI's
durations cache) and therefore the LPT floor: no test slice can finish
faster than its slowest file, which caps how far slicing the test matrix
wider can cut the merge-gate critical path.
Profiling (cProfile on the slowest tests) found the time was dead, not
work:
1. time.sleep in optimize_fts_storage's inter-chunk throttle — 4.1s of
a 4.6s migration test. The throttle exists so a LIVE gateway/CLI
sharing the DB isn't starved of the write lock; tests run against a
private tmp-path DB with no concurrent process, so the sleep protects
nobody. New autouse fixture zeroes _FTS_REBUILD_MIN_PAUSE /
_FTS_REBUILD_DUTY_FACTOR for this file (~20s saved). No test asserts
on wall-clock pacing, so nothing weakens.
2. TestGetMessagesPagination._seed appending 3000 messages one
append_message (= one commit, and off WAL one fsync) at a time —
~10s of seeding before the query under test even ran. Switched to
append_messages_batch (one write transaction), the API the docstring
of which exists for exactly this shape. The perf contract the seed
feeds still discriminates: measured 11 progress-handler steps on the
indexed path vs 855 on the forced scan path, against the unchanged
300 threshold.
Measured (local, 3 runs + canonical runner):
before: 187 passed in 51.8s
after: 187 passed in 9.1-16.1s (canonical scripts/run_tests.sh: 14.9s)
Zero production code touched; 187 tests before and after.
read_file's auto-extraction covered only the stdlib trio (.ipynb/.docx/
.xlsx). firecrawl-anydoc (MIT, Rust core, imports as `anydoc`) converts
Word, PowerPoint, Excel — including legacy .doc/.ppt/.xls — OpenDocument,
RTF, EPUB, and PDF to clean Markdown through one shared document model.
Wiring follows the footprint ladder: no new tool, no hard dependency.
- tools/read_extract.py gains an ANYDOC_EXTENSIONS set that is active
only when the converter imports; the stdlib extractors remain
authoritative for their three formats so behavior is identical with
or without the package.
- tools/lazy_deps.py adds tool.doc_extract (firecrawl-anydoc==0.1.6),
installed on first read of such a file with prompt=False so read_file
can never block. Lazy-only for now: the package's first release was
2026-08-04, inside uv's 14-day exclude-newer quarantine, so the
mirrored pyproject extra lands after it clears.
- Any anydoc ConvertError maps to ExtractionError, falling back to the
existing path/binary handling instead of erroring the tool.
Tests: real-binding suite skips cleanly when the wheel is absent
(verified: 15 passed/3 skipped without it, 18 passed with it), plus an
absent-dep contract class that pins the fallback regardless of local
install state.
The 8 test slices are the last thing the required all-checks-pass gate
waits for on every python PR: each slice carries ~968s of LPT-balanced
per-file work and runs 150-190s wall, while every other gated job is
done by ~90s. Per-slice fixed overhead is ~10s with the warm uv cache,
so slicing wider is nearly free: at 12 slices per-slice work drops to
646s and stays balanced (makespan == min within 1s in simulation
against the live durations cache).
Verified on this PR's own CI run: slices 141-187s -> 97-140s wall.
Peak run concurrency rises 23 -> 27 jobs; observed queue delay at 23
is 2-10s, well under the org limit.
Residual #76762 class: _read_referenced_script caught OSError from
os.open but not ValueError, so a path token carrying an embedded NUL
(tokenized binary-adjacent command text) crashed the terminal tool's
lifecycle guard with 'ValueError: embedded null byte' instead of being
skipped as nothing-to-scan. Reproduced live against main. Same
treatment the resolve()-time site already has; two sabotage-verified
regressions added.
Cron agents are constructed with skip_memory=True, so the memory
backend is not initialised — exposing the memory tool only gives the
model an unbacked tool that fails at runtime with 'Memory is not
available.' Add 'memory' to _resolve_cron_disabled_toolsets() so the
tool is stripped from the schema before the model can call it.
Fixes#38129.
Co-authored-by: Paolo Shamoon <Paolo@Dylans-Mac-Studio.local>
scripts/run_tests.sh runs the suite under `env -i` with an explicit
allowlist. The runner's own documented environment knobs were never on
that list, so all of them were silent no-ops for anyone invoking the
canonical wrapper:
* HERMES_TEST_WORKERS / PATHS / FILE_TIMEOUT / FILE_RETRIES / SLICE
are read by run_tests_parallel.py at argparse-default time — inside
the stripped environment.
* HERMES_TEST_IMAGE is read by tests/docker/conftest.py to skip its
session-scoped docker build.
The HERMES_TEST_IMAGE strip is the expensive one, and it's been biting
CI since docker.yml switched from bare pytest to run_tests.sh
(f0cb04921): the workflow sets HERMES_TEST_IMAGE to the image the build
step just loaded, the wrapper drops it, and every per-file pytest
subprocess falls back to building hermes-agent-harness:latest itself.
The job log timing shows it plainly — the first 8 files dispatched (the
LPT-heaviest) all report 248-297s, which is them waiting out the
concurrent initial `docker build` (~4 min on a cold local builder);
every file dispatched after that rides the layer cache and finishes in
4-38s (e.g. test_dump_build_sha.py, a single `docker run --entrypoint
cat`, reported 256.6s). ~4 min of pure waste per docker job, on both
arches — and the tests exercised a locally-rebuilt image WITHOUT the
HERMES_GIT_SHA build-arg the workflow bakes in, not the artifact being
shipped.
Fix: forward the six knobs the same way the Windows location vars are
forwarded (66c4c9c0b) — an explicit compute-before-drop allowlist, each
var only when set, so POSIX runs without them are byte-for-byte
unchanged and the 'no credential can leak' property stays auditable.
Verified empirically via a probe test through the wrapper:
before: HERMES_TEST_IMAGE=None inside the subprocess
after: HERMES_TEST_IMAGE='sentinel-image', HERMES_TEST_FILE_TIMEOUT
forwarded, HERMES_TEST_WORKERS=3 yields '(3 workers)' in the
summary, and an unrelated SOME_SECRET stays stripped.
bash -n clean; shellcheck: no new findings (SC2046 on the pre-existing
compileall line predates this change).
A gateway session whose summary model keeps timing out no longer retries
compaction on the same fixed interval forever.
The in-agent compressor already escalates repeat summary timeouts
60 -> 300 -> 900s (ContextCompressor.record_timeout_failure), but that ladder
reads the in-memory _consecutive_timeout_failures counter and
bind_session_state() zeroes it (context_compressor.py:1645). Session hygiene
constructs a FRESH AIAgent for every run (gateway/run.py:16820) and re-binds
state each time, so from the gateway that streak is structurally always 0 --
only the flat hygiene_failure_cooldown_seconds (300s) could ever be recorded.
Issue #79624 reported exactly that steady state: an oversized session
(1053 messages, ~119.5k tokens) whose aux model always timed out, re-attempting
compaction every 300s across five days until the reporter deleted the session
by hand.
Track the streak on PersistentState instead, which outlives the per-run agent
and is not cleared by turn/boundary resets, so consecutive hygiene failures
climb 300 -> 900 -> 2700s and then saturate. Both failure sites (progress
timeout and aborted compression) feed it; a real compression resets it, so a
session that recovers starts from the first rung again. The ladder multiplies
the configured base, so operators who tuned
hygiene_failure_cooldown_seconds keep their first rung. Per-session, so one
wedged chat cannot penalize other conversations.
Deliberately NOT changed, since each is a maintainer policy call rather than a
defect (all three are written up on #79624):
- no durable failure-streak column, so escalation still resets on restart
- the gateway 30s / in-agent 120s / aux-client 300s-floor timeout mismatch
- no `hermes doctor` check or `hermes sessions list` marker for a session
stuck in a compression-failure cooldown
Note the reported exit(1) is NOT a crash: it is the deliberate
_signal_initiated_shutdown path (gateway/run.py:26746-26751, #5646) that lets
systemd Restart=on-failure revive the gateway after a bare SIGTERM, and it
fires on every `systemctl restart` independently of compaction. The compaction
log lines appear after the shutdown line because the gateway-owned executor is
torn down with shutdown(wait=False, cancel_futures=True) (run.py:21164), so an
in-flight turn keeps logging during teardown. Full analysis on the issue.
Post-review hardening (Phase 2c + /simplify-code found five real defects in the
first cut):
- the recovery gate hand-rolled `_new_tokens < _approx_tokens` when a canonical
predicate already existed: `compression_made_progress` (agent/turn_context.py,
#39548). They disagree on 3 of 5 cases -- the hand-rolled form misses a
row-count win when the summary keeps the token estimate flat, misses one
where the summary is slightly MORE verbose (so a genuinely recovered session
would keep escalating forever), and counts a sub-5% wobble as recovery. Now
reuses the shared predicate, promoted from `_compression_made_progress` to a
public name with the old private name kept as a back-compat alias so the
existing importer (tests/agent/test_protected_tail_pressure_61932.py) and any
patcher of that symbol keep working.
- the reset was gated on "not aborted", but the degenerate "did not rotate or
compact in place" branch (#21301) is NOT aborted and yields zero reduction,
so a session wedged there reset its streak every run and could never
escalate -- silently defeating the fix. Now gated on real progress.
- no absolute ceiling: base * 9 reaches 9h at an operator base of 3600s,
indistinguishable from "compaction switched off". Added
_HYGIENE_COOLDOWN_MAX_SECONDS = 3600, mirroring the in-file
_RECONNECT_BACKOFF_CAP precedent.
- the reset used the get-or-create accessor to write a 0 that was already 0,
materialising a _sessions entry (never evicted). Now peeks.
- the abort verdict was probed twice, leaving the reset/record mutual
exclusion implicit; a future await between the probes would have broken it
silently. Computed once into _hyg_aborted.
Tests: 19 new in tests/gateway/test_hygiene_failure_cooldown_ladder.py --
ladder escalation, saturation, the absolute cap, per-session isolation,
reset-on-recovery, custom/zero base, PersistentState scoping (a mutation moving
the field to TurnState fails), degraded runners, the progress gate, the exact
progress-predicate semantics the gate depends on, and end-to-end that the
escalated value is what reaches the state DB. All 12 mutations caught, including
ones that restore the flat cooldown (the original bug), ungate the reset, swap
the canonical predicate back for the hand-rolled comparison, remove the cap, and
share the streak globally; the harness hard-errors when a mutation cannot be
applied, since a silently no-op mutation check is worse than none -- an earlier
version of it WAS silently no-opping after a refactor. The gate's contract test
slices by AST node span rather than a fixed character count, which had already
truncated once as the block grew. gateway hygiene + session-state + the three
touched agent compression suites: 50 passed; ruff clean.
E2E with real imports demonstrates the premise rather than asserting it:
bind_session_state zeroes the in-agent counter, and the recorded deadlines go
300 -> 900 -> 2700 -> 2700 -> 2700s where they were previously a flat 300s.
Reported by @yucezerey (#79624), whose state.db column dump and
"deleting the session fixed it" datapoint made the real mechanism findable.
Post-review fixes on the preserve_mode/create_mode follow-up:
- create_mode is now applied ONLY when the target does not exist, on
both atomic_write_text and atomic_yaml_write. Previously
atomic_write_text(path, s, create_mode=X) without preserve_mode would
silently chmod an EXISTING file to X (docstring/code mismatch, latent
trap -- no caller relied on it), and a stat failure on an existing
file could fall through to create_mode instead of leaving the mode
alone.
- atomic_yaml_write now fchmods the temp fd BEFORE the replace when a
mode is known, matching atomic_write_text: a freshly created
distribution.yaml no longer transits through mkstemp's 0600 (a crash
between replace and chmod could previously leave it 0600 forever).
The post-replace _restore_file_mode stays as the Windows path.
- fchmod moved inside the fdopen context in atomic_write_text, so a
raising fchmod can no longer leak the fd.
Tests: create_mode-never-rewrites-existing guard (mutation-checked) and
a monkeypatch.delattr(os, 'fchmod') test covering the Windows
post-replace branch that the win32 module skip left uncovered.
Follow-up to the salvaged #79323 commits. The three hand-rolled
stat -> atomic_write_text -> chmod blocks (xai migration, uninstaller
shell-rc rewrite, dashboard SOUL.md editor) collapse into an opt-in
preserve_mode=True kwarg on utils.atomic_write_text, plus create_mode=
on both atomic_write_text and atomic_yaml_write for first-create paths
(SOUL.md first save, write_manifest's allowlist create path).
Beyond deduplication this closes two gaps the hand-rolled copies had:
- Owner preservation: the old in-place writes kept the inode, so file
ownership survived root-run rewrites for free. atomic_write_text
swaps in a new inode owned by the writing user, and the hand-rolled
blocks restored only the mode -- a root-run 'hermes migrate xai' or
sudo uninstall on a user-owned Docker/NAS volume would flip
config.yaml / ~/.zshrc ownership to root. preserve_mode now routes
through the same _preserve_file_owner/_restore_file_owner helpers
atomic_yaml_write and atomic_json_write already use.
- chmod-after-replace window: the mode is applied to the temp fd via
fchmod BEFORE the replace (mirroring atomic_json_write's mode= param),
so the target never transits through mkstemp's 0600.
Also removes write_manifest's caller-side existed/chmod block (and its
small TOCTOU) in favor of atomic_yaml_write(create_mode=0o644), and
corrects the SOUL.md mode comment (the default profile's runtime seeder
does run it through _secure_file; named profiles do not).
preserve_mode defaults to False so the existing callers (memory store,
skill manager, cron, agent importer) keep their current semantics.
New tests in tests/test_atomic_write_text_metadata.py cover mode
preservation, owner restore through symlinks, fchmod-before-replace,
create_mode on both writers, and no-behavior-change without opt-in;
all mutation-checked.
Both files were routed through the shared atomic writers earlier in this
branch. tempfile.mkstemp creates the temp file 0600 and the atomic swap
carries that mode onto the target, so the *create* paths silently tightened
two files that previously landed at the umask default:
- web_routers/profiles.py: the dashboard persona editor's first-ever Save has
no prior SOUL.md to copy permissions from, so the existing guard skipped the
chmod entirely -- contradicting the comment directly below it, which states
profile SOUL.md is created 0644 and is not run through _secure_file.
- profile_distribution.py: atomic_yaml_write only restores a mode it captured
from a file that already existed. _materialize() calls write_manifest() with
no manifest on disk whenever a distribution declares an explicit
distribution_owned allowlist that omits distribution.yaml, so the staged
copy is never placed in the profile.
Both are fixed with a local chmod at the two sites this branch regressed;
utils.py's public mode semantics are left alone. profiles.py now also
distinguishes "no file yet" (FileNotFoundError -> 0644) from "stat failed for
some other reason" (-> leave the mode alone rather than guess at it).
uninstall.py and xai_retirement.py have no create path and are unchanged: the
former captures prior_mode unconditionally after a successful read_text(), and
the latter runs require_readable_config_before_write() first.
The four new mode-preservation guards assert POSIX permission bits, which
Windows does not model (os.chmod only toggles the read-only flag there).
Guard them the way the suite already guards POSIX-specific semantics so the
tests stay meaningful on Linux/macOS without failing for Windows contributors.
The symlink cases stay unguarded, matching the existing symlink tests in
tests/hermes_cli/.
`utils.atomic_write_text`'s docstring states the invariant: it exists "so that
every destructive file rewrite in the codebase shares one implementation."
Four full-file rewrites of *existing user-authored files* still bypass it and
use a bare truncating `open(path, "w")` / `Path.write_text()`, which truncates
the target before the new content is produced. A crash, SIGINT, or ENOSPC
mid-write therefore leaves the file empty or half-written.
In all four cases the read half degrades silently to a default rather than
erroring, so the damage is invisible and the next write cements it:
* `xai_retirement.apply_migration()` rewrites the user's config.yaml. Merged
commit beaa1a08e added a readability guard here and noted the writer "lives
outside the atomic_yaml_write path, so the chokepoint didn't cover it"; this
closes the durability half it left open. `--no-backup` is a documented flag,
so on that path the truncated file is the only copy that exists, and the
loader returns early on `doc is None` — the next run reports nothing to
migrate rather than surfacing the damage.
* `uninstall.remove_path_from_shell_configs()` rewrites the user's shell rc
(~/.bashrc, ~/.zshrc, ...). Hermes does not own these files and this function
takes no backup; the enclosing `except Exception` downgrades a partial write
to a warning, so the next login just starts a bare shell.
* `web_routers.profiles.update_profile_soul()` replaces SOUL.md from the
dashboard editor. The paired GET reports an unreadable file as
`{"content": "", "exists": False}`, so an interrupted save presents as "your
persona was never set" and the editor's next Save persists the empty document.
* `profile_distribution.write_manifest()` rewrites distribution.yaml on every
install/update. `read_manifest` treats an unparseable manifest as "not a
distribution", silently dropping update tracking and env_requires.
The xAI migration keeps its ruamel round-trip dumper (comments, key order and
quoting must survive) and now serializes to a string before handing the bytes
to the shared writer. `write_manifest` moves to `atomic_yaml_write`, whose
SafeDumper output the manifest already round-trips through, retiring the local
`_dump_yaml` helper.
`atomic_write_text` recreates the target from a 0600 temp file, so each of its
call sites re-applies the file's previous permission bits: `_secure_file`
deliberately leaves config.yaml alone under managed (NixOS 0640) and container
installs, shell rc files are normally 0644, and profile SOUL.md is created 0644
and never secured. `atomic_yaml_write` already preserves mode and owner itself.
Routing through `atomic_replace` also keeps a symlinked config.yaml or ~/.zshrc
(dotfiles repo, managed deployment) pointing at the real file.
Tests: one regression test per site fails on clean main (the interrupted write
completes there and destroys the file) and passes here; the remaining cases are
behaviour guards covering symlink survival, permission preservation, comment
round-tripping, and the existing happy paths.
Follow-up to #79722, which introduced _escape_like in hermes_state.py for
the prune/archive filter fix. The same three-replace escape chain existed
as five more inline copies in hermes_state.py and two in
hermes_state_search.py (which must not import hermes_state — cycle).
Move the helper to hermes_state_common.escape_like (the module that exists
for exactly this) and route every copy through it:
- hermes_state.py: session-ID prefix resolution, find_session_by_title,
get_next_title_in_lineage, the _like_pattern closure in list projection,
and the kanban cwd retag
- hermes_state_search.py: the two LIKE-fallback token escapes
hermes_state re-imports it as _escape_like for back-compat. No behavior
change: every site produces byte-identical SQL patterns.
The quadruple-backslash pattern arm is the trickiest byte sequence in the
fix and had no direct coverage — 'simplifying' it to a double backslash
would break Windows child matching with every test still green. Mutation
checked: weakening the arm fails this test.
Follow-up on the salvage of #78681 + #78927: the second fix inlined the
exact body of the _escape_like helper the first fix introduced ten lines
above. Call the helper instead so there is one copy of the escaping rule.
_cwd_prefix_clause builds "cwd is this directory or under it" for session
listing, workspace resume and prune/archive. The two LIKE arms bound the
raw prefix, so `_` and `%` acted as wildcards on a value that is a path:
cwd_prefix="/home/me/my_project"
main -> ['sibling', 'target'] # /home/me/myXproject/src matched too
fix -> ['target']
`_` matches any single character, so a same-length sibling directory with
children falls inside the pattern. prune_sessions() deletes the rows it
matches (and their on-disk transcripts), so an unrelated project's history
goes with it.
Escape the needle and pair both arms with ESCAPE, the convention the rest
of this file already uses; the literal separator backslash in the Windows
pattern is escaped for the same reason. The `=` arm is an exact compare and
keeps the raw prefix, so directory-and-children matching is unchanged.
Follow-up to the *_like filters in #78681, kept separate because this helper
is shared by four call sites beyond prune.
_prune_filter_where documents title_like / model_like / branch_like as
"case-insensitive substring matches", and the CLI confirmation renders them
as "title contains 'X'". They were bound straight into a bare LIKE, so `_`
matched any single character and `%` any run.
The builder backs prune_sessions(), which deletes session rows and their
on-disk transcripts, so the over-match is unrecoverable: pruning
title_like="user_auth" also destroys "user-auth", "userXauth" and
"user auth". `_` is not exotic here -- git branch names and session titles
carry it routinely.
Escape the operator's needle and add ESCAPE '\' to the three clauses, the
same convention the rest of this file already uses for LIKE queries. Match
direction is unchanged for needles without wildcards.
Left alone: _cwd_prefix_clause has the same unescaped shape but is shared
by four call sites beyond prune, so it is a separate change.
Follow-up to the salvaged start-order gate bound. Two gaps remained, both
reachable through the same knob.
1. The gate bound ignored the batch deadline it sits under. With
HERMES_CONCURRENT_TOOL_TIMEOUT_S below 120s the deadline fired first, so
the parked tools were still reported as "timed out" without ever running --
the exact bug the bound exists to fix. The gate now clamps to
min(120s, batch_timeout / 2), matching the sibling constant's documented
habit of relating the two timeouts.
2. A gate-parked worker released purely by its own timeout could wake up after
the batch was abandoned and dispatch its tool anyway: wasted work whose
result nobody reads, a duplicate post_tool_call for a tool_call_id the turn
already closed as timeout, and agent._current_tool left pointing at a dead
tool for the rest of the session (the main thread's reset already ran).
Abandonment is now a first-class wakeup: both abandon sites set an event and
notify the condition, and a released worker raises _BatchAbandoned instead
of dispatching. Parked threads are reclaimed in milliseconds rather than one
full gate timeout plus a tool runtime.
Also names the tool in the gate-timeout warning. The closure's function_name
binds the last-parsed tool, so logging it directly would have printed the wrong
name; it is threaded through _begin_in_order instead.
Measured, 3-tool batch with the first tool wedged during dispatch:
main PR as-is with this commit
dispatched in batch 0 0 tool_b, tool_c
dispatched after return 0 2 (ghost) 0
_current_tool leaked no "tool_b" no
Adds tests/run_agent/test_start_order_gate.py (3 tests). Mutation-checked
against the parent commit: the starvation guard passes there (it binds the
salvaged fix), while the deadline-clamp and abandonment guards both fail,
reproducing the ghost dispatch as
"tool(s) dispatched after the batch was abandoned: [tool_a, tool_b]".
_begin_in_order parks each concurrent tool worker on a timeout-less
Condition.wait_for until every earlier-ordered tool has advanced through
its dispatch. If one tool wedges during dispatch (observed in production:
a stuck skill_view; also reproducible via any blocking authorization),
three failures compound: every later-ordered worker is starved and never
starts; the batch deadline then falsely reports those never-started tools
as "timed out" (sub-second read_file/search_files calls get blamed while
having done zero work, and the model reasons against that false failure
info); and after the batch is abandoned the parked workers leak forever —
f.cancel() cannot cancel running threads, the per-thread interrupt flag
is never polled inside wait_for, and nothing notifies the condition
again. Confirmed with a faulthandler all-threads dump taken after batch
abandonment showing workers still parked at the gate.
Bound the wait at 120s; on expiry, log a warning and proceed out of
order (worst case: interleaved approval prompts — strictly better than
permanent starvation). The >= predicate lets one worker's timeout-jump
release every skipped worker immediately, and max() keeps the counter
monotonic for out-of-order advancement.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The AST invariant only matched `env["HERMES_KANBAN_X"] = ...` subscript
assignments, so a future dispatcher var added via `env.update({...})`,
`env.setdefault(...)`, or an annotated subscript would have slipped past
the guard and leaked into cron sessions unnoticed.
None of those shapes exist in _default_spawn today; this is about the
guard staying trustworthy as that function evolves.
Verified by injecting an unregistered var into _default_spawn one shape at
a time and requiring the guard to fail: subscript assign, annotated
assign, update(dict literal), setdefault(literal), and update(kwarg) are
all detected. Source restored byte-identical after probing.
tests/cron/ 410 passed; ruff clean.
A kanban worker that fires a cron job in-process no longer leaks its task
identity into the cron agent.
The worker is a normal `hermes chat -q` CLI agent whose default toolset
includes `cronjob`, running with HERMES_KANBAN_TASK legitimately set in its
own environment. `cronjob(action="run")` calls run_one_job() -> run_job()
in that same process, so the cron AIAgent was misidentified as that worker:
kanban toolset force-added, kanban-worker protocol injected into its system
prompt, and kanban_complete defaulting task_id to $HERMES_KANBAN_TASK --
letting an unrelated cron job close the worker's task and overwrite real
results.
Fixed with a ContextVar (`non_dispatcher_owned_context`), not by clearing
os.environ. The env is process-global and shared with three concurrent
readers that all need the real values:
* the worker's own claim heartbeat -- run_agent._touch_activity ->
heartbeat_current_worker_from_env reads TASK/CLAIM_LOCK/RUN_ID, and the
cron-run heartbeat thread drives it every 10s. Clearing them silently
no-ops the heartbeat, so after DEFAULT_CLAIM_TTL_SECONDS (15 min) the
dispatcher reclaims a task whose worker is still alive and re-dispatches
it -- the same duplicate-work failure from the other direction.
* the gateway's kanban watchers, which do their own HERMES_KANBAN_BOARD
save/restore around a slow decompose_task() LLM call.
* concurrent cron jobs, which take a *shared* read lock
(_terminal_cwd_lock.acquire_read) and so interleave: job A clears, job B
snapshots empty, A restores, B clears and its restore no-ops -- the
worker's identity is destroyed permanently.
`is_dispatcher_owned_worker_context()` is now the single predicate every
HERMES_KANBAN_* identity gate consults before trusting those vars. It also
closes a pre-existing gap in agent/skill_utils.py, which read the vars
without consulting the delegate_task ContextVar at all; the `kanban` verdict
additionally bypasses _ENV_DETECT_CACHE, since a context-dependent answer
must not be memoized process-wide.
HERMES_KANBAN_BOARD/DB/WORKSPACES_ROOT are left untouched, so the #20074
board pin and the dispatcher's path overrides keep working.
Tests: 18 new, including thread-isolation, concurrent-cron-jobs, and an AST
invariant over _default_spawn that fails if the dispatcher gains a var that
is neither identity-gated nor explicitly classified behaviour-only. All six
mutations are caught, including one that reintroduces the os.environ clear.
tests/cron/ + kanban suites 440 passed; model_tools/skill_utils/boards 63
passed; ruff clean.
Reported and diagnosed by Geoff Friesen (#78961), who identified the symptom
and the exact gating mechanism.
Co-authored-by: Geoff Friesen <gfriesen1@users.noreply.github.com>
Follow-up to #79669. That PR routed the three fallback recorder sites through
_record_turn_final_payload so a split turn would record the unsplit ledger
instead of a tail-only payload. For two of them that is right. For
_send_empty_fallback_final it is wrong, and it reintroduces the #78541 swallow
at the one site that was supposed to be fixed.
_send_empty_fallback_final is a *replacement* recovery: it sends the completed
text as a fresh message and deletes every tracked segment preview -- which on an
overflow split includes the sealed head chunks. After it runs, the only thing
on screen is the message it just sent. Recording the ledger there claims
delivery for text the same function just removed, so delivered_final_matches()
returns True, the gateway suppresses its own send, and the user is left with a
fraction of the answer.
Observed with a probe driving the real run() loop (543-char reply, 475-char head
sealed then deleted, 67-char tail committed):
before this fix recorded=543 matches=True -> suppressed, 67/543 on screen
after this fix recorded=67 matches=False -> gateway sends the full answer
Record final_text verbatim here instead. The sibling site in
_send_fallback_final keeps the recorder: its delete is gated on
`continuation == final_text` and targets only the single active partial, never
the sealed heads, so the ledger correctly describes what survives.
The distinction is whether a recovery ADDS to what is on screen or REPLACES it.
Additive paths may record the ledger; replacing paths must record only what they
leave behind. _try_fresh_final is the same shape and #79669 handled it by
refusing the route on split turns.
Test drives the real seal-then-delete sequence and asserts the mismatch, so the
gateway is required to re-send. Mutation-checked: restoring the recorder call
turns it red.
The salvaged fix changed only the gateway's verdict: a payload-less
multi-message split stopped inheriting legacy trust. But six code paths set
_turn_split_delivery, and only one of them was taught to record a payload, so
the remaining five swapped the swallow for the opposite defect.
Fix the producers instead of only distrusting them at the boundary:
- _send_or_edit failed-final-edit branch: record the visible payload on split
turns too. It deliberately skipped recording, which now reads as a mismatch
and re-sends an answer already on screen -- reintroducing the duplicate
#45517 fixed (#36965 / #25349).
- _send_fallback_final (x2) and _send_empty_fallback_final: route through
_record_turn_final_payload instead of assigning _delivered_final_text
directly. On a split turn their final_text is only the trailing chunk, so a
fully delivered heads+tail reply recorded a tail-only payload and was
re-sent in full.
- _try_fresh_final: refuse the fresh-final route once a head chunk is sealed.
It replaces every tracked preview with one message, which only holds the
whole answer on a single-message turn. After a split it deleted the sealed
heads while sending just the tail, so the complete reply was still lost --
on Telegram, the default finalize route and the shape #78541 reports.
- Set _turn_split_delivery at seal time rather than after the tail send, so
the tail's own finalize sees the split state. The sibling overflow path
already did this; the divergence is what let fresh-final delete the heads.
- run.py stale-finalize reconciliation: skip the in-place edit on a split
delivery. message_id is only the LAST chunk there, so editing it with the
complete response repeated every sealed head's text inside the tail
message. Fall through to the normal final send.
Also drop a dead `or "".join(chunks)` fallback (all growth funnels through
_append_accumulated, so the ledger is never empty at that call site, and joined
chunks carry injected fence markers that could never match final_response), and
document that _record_turn_final_payload intentionally ignores its argument on
split turns.
Tests: four end-to-end cases driving the real overflow-split loop instead of
hand-setting private flags -- complete split still suppresses (no duplicate),
split missing a tail does not suppress, fresh-final keeps sealed heads, and a
flood-controlled final edit after a split stays suppressed. Each was
mutation-checked: reverting any individual fix turns its test red.
The pre-existing gateway-boundary test asserted the recovery *route* (the
reconcile edit) rather than the guarantee. Relaxed to the real contract: either
_run_agent puts the complete text on the wire, or it declines to claim delivery
so the caller's normal final send does.
Co-authored-by: HexLab98 <liruixinch@outlook.com>
Record an unsplit stream ledger for multi-message deliveries and refuse
legacy trust when split delivery left no payload, so Telegram group
sessions no longer suppress a complete reply after an early/partial
finalize (#78541).
- optional-skills/devops/actual-setup: field-tested setup skill contributed
by shl0ms, updated for the first-class 'actual' provider (the original
targeted a custom-provider config that now collides with the built-in name)
- docs: providers.md section + tables, environment-variables.md, quickstart.md
- tests/skills: frontmatter + first-class-provider conformance checks
- fetch_models(): accept base_url kwarg (interface grew on main since May)
- runtime_provider: config-driven loopback base_url now reaches the local
no-auth placeholder before the usable-secret gate (added on main in the
interim, would otherwise AuthError on keyless local setups)
- test: fetch is now called with base_url by the generic live-fetch path
kshitij
Teknium
Adolanium
Rickard Robin
Kevin Yin
kshitij
Paolo Shamoon
briandevans
Drexuxux
Sylvain Baeriswyl
ethernet
HexLab98
Justin Bennington