Review-pass follow-ups (three parallel reviewers, findings verified):
- hermes_state_search.py list_recent_user_messages now drops legacy
standalone compaction handoffs in the decode loop (SQL can't see them:
durable role=user, no display_kind). Closes the /undo N pairing skew
where the in-memory count (new predicate) and the DB soft-delete pick
(old predicate) targeted different turns on legacy sessions. Fetches
with headroom so the requested limit is still honored. 3 new tests,
mutation-checked (no-op'ing the skip fails 2/3).
- _should_skip_model_call_for_reference_handoff: single drive-check scan
(was two — once inside the restore helper, once after); the restore
helper no longer re-scans and its return value now decides the verdict.
- _final_response_from_messages replaced by the _HANDOFF_SKIP_FINAL_RESPONSE
constant it always returned (parameter was unused).
- _handoff_carries_live_user_content delegates to the canonical
_strip_context_summary_handoff_message — also fixes the edge where a
merged-shaped row with an EMPTY preserved prior tail was wrongly
treated as carrying live content.
- Site-level guard test for rollback.restore with a legacy handoff row
(predicate-in-context, complements the unit tests).
Follow-ups on top of the salvaged #80696 fix (review findings):
- Sibling sites: rollback.restore, gateway /retry, CLI /retry and /undo N,
and both CLI resume turn counters now use is_user_originated_turn so
legacy-persisted standalone handoffs (durable role=user, no display_kind)
can never be truncation targets or counted as user turns (#80622
suggested regression 4, dispatcher-wide).
- Site-1 guard: hoist the api_call_count decrement + iteration-budget
refund above the break so a skipped turn no longer leaks a budget unit
and finalize_turn logs the true call count (matches the ollama early-exit
and the site-2 sibling).
- Site-2 guard: run the handoff guard BEFORE reanchoring so a restored
user ask is what the anchor lands on, not a stale pre-restore index.
- SUMMARY_PREFIX: add the mid-tool-loop carve-out the code-side guard
already implements, so a literal-minded model doesn't halt an in-flight
exchange after in-place compaction.
- Skip path returns a short compaction status instead of replaying the
previous turn's answer (finalize_turn would append it as a fresh
assistant row — duplicate prose in transcript and delivery).
Pin #80622 invariants: handoff alone must not drive a model call after
stop, pending real users are restored, and synthetic compaction rows are
never treated as user-originated turns. Also give micro-compaction
enough passes to pay back the longer SUMMARY_PREFIX marker overhead.
After a completed assistant stop, a standalone CONTEXT COMPACTION handoff
could occupy the sole user slot and resume stale Historical Task Snapshot
work with no new human ask. Guard post-compaction continues, hide
standalone handoffs from session dispatch, and harden SUMMARY_PREFIX for
the empty-after-handoff case (#80622).
Two docstring corrections on top of the salvaged #80997 fix — behavior
unchanged, both verified against the code:
- The 'rough growth over-counts every content class' claim is false for
Cyrillic/Greek/Thai/Arabic (chars/4 vs ~2-3 chars/token on o200k):
growth there can under-count up to ~2x (#62605's direction). Document
the real backstops instead: an at/over-threshold real reading clears
the baseline (post-response gate fires on real usage within one call)
and the provider overflow handler compacts reactively.
- Document the two measurement bases (turn-prologue raw messages vs the
loop's fully assembled request that seeds the baseline) and why the
prologue's smaller basis can only OVER-defer — the loop's pre-API
pressure check re-runs the projection with the aligned basis before
every provider call, so a prologue over-defer never skips a needed
compaction.
The rough preflight estimate intentionally overestimates, but not by a
fixed margin: CJK text is counted at ~1.7x its o200k cost and
Responses-mode reasoning replay blobs at several times their billed
cost. Heavy sessions show rough estimates 2-3x real usage and compact
at 35-55% of the real window, stalling turns for minutes and discarding
detail (churn), because the defer guard only tolerated 5% rough growth
and sessions that never compressed had no baseline at all.
Pair every request's rough estimate (note_request_rough_estimate,
recorded in the conversation loop right after the pressure estimate)
with the provider's real prompt_tokens in update_from_response(), then
defer preflight while projected real usage — last real + rough growth
since that reading — stays under the threshold. Rough growth is itself
an overestimate of real growth, so the projection is an upper bound and
deferring below the threshold is safe; the provider's context-overflow
handler remains the backstop.
The baseline no longer ratchets on defer: it is refreshed by the
response pairing, and advancing it without a matching real reading
would shrink apparent growth and defer on stale data.
The docs promised 'frees ~370MB RAM' — measured behavior on macOS/CPU
is that ctranslate2's allocator keeps the freed pages (RSS doesn't
visibly shrink); the concrete win is VRAM release on CUDA hosts and
process-internal reuse on CPU. Say exactly that instead.
Review pass on the idle-unload feature found two material concurrency
bugs; both fixed here with a regression guard:
1. Unload-vs-use null deref (HIGH): _transcribe_local re-read the
module global _local_model at the transcribe call site. An idle
unload firing between the model load and transcribe() evaluated
None.transcribe → AttributeError → user-visible 'Local
transcription failed'. The window was real: the idle timer was only
touched AFTER a successful transcription, so a voice note arriving
exactly as the timeout expired raced the watcher directly.
Fix: bind a strong local reference under the model lock and use it
for the whole transcription (the watcher can null the global at any
time; this in-flight call keeps its instance — the generator holds
self, so no use-after-free). Also touch the idle timer at the START
of transcription so a long in-flight transcribe can't be counted as
idle time. The CUDA-fallback retry path gets the same treatment
(locked global write, local ref use).
2. Watcher replacement race + response-path join (MEDIUM/HIGH): the
old design stopped and re-started the watcher after EVERY
transcription with an unlocked set/join(5)/clear/start sequence on
shared globals. Two concurrent voice messages could interleave to
leave TWO live watchers (one with a stale, shorter timeout — a
raised unload_after_idle_seconds could still unload on the old
value), and the join(timeout=5) sat on the user-visible response
path (a watcher blocked on _local_model_lock during a concurrent
multi-second model load stalls the reply up to 5s).
Fix: single long-lived watcher under a management lock — started
only when none is alive (per-transcription cost: one lock + one
is_alive check), re-reads the configured timeout from config every
cycle (config edits now apply within one 30s interval, without
waiting for the next voice message — previously undocumented), and
stands down without unloading when the timeout is set to 0
mid-idle.
Tests: 17 now — idempotent start (same thread, no churn), config
re-read + stand-down-when-disabled, and the race guard
(unload firing mid-transcription must not fail the in-flight call).
The race guard is mutation-verified: reverting the fix (re-reading
the global at the call site) makes it fail with the exact NoneType
error; the fixed code passes.
The local faster-whisper model singleton (_local_model) is loaded once
and never released — the 'base' model holds ~370 MB of RAM/VRAM for
the entire lifetime of the process, even when no voice messages arrive
for hours or days. On long-running gateway processes (especially with
local LLMs competing for the same GPU) this is wasteful.
Add a config-driven idle unload: after stt.local.unload_after_idle_seconds
(default 0 = never) of no transcription activity, a lightweight daemon
thread sets _local_model = None so the Python GC can reclaim the
ctranslate2 objects. The next voice message reloads the model
transparently (the existing lazy-load path handles it).
The watcher:
- Checks every 30s whether idle time exceeds the configured threshold
- Acquires _local_model_lock before unloading (prevents races with
concurrent transcriptions that are mid-load)
- Exits immediately if the model is already None (unloaded by another
path, e.g. the CUDA fallback eviction)
- Is restarted by each transcription with the current config value,
so changing stt.local.unload_after_idle_seconds in config.yaml takes
effect on the next voice message without a process restart
Default is 0 (never unload) — zero behavior change for existing users.
Recommended value for gateway processes: 300 (5 minutes).
15 tests: config resolution (garbage/negative/None fallbacks), unload
safety (already-None, lock acquisition), touch timestamp, watcher
lifecycle (unload after timeout, no unload within timeout, exits when
model already None, stopped on new start). Existing STT test suite
unchanged.
The review-fold commit added the input-duration gate but the user-facing
docs and config example still implied every cloud clip gets trimmed.
One-line additions to both.
Three-reviewer pass (reuse / quality / efficiency) on the trim diff;
four findings folded:
1. Short-clip input gate (efficiency, HIGH): the trim previously paid
the full ffmpeg encode before the <10%-saving discard check — every
dense conversational voice note burned 3 subprocess spawns + a
complete re-encode on the synchronous response path for nothing.
New _CLOUD_TRIM_MIN_INPUT_SECONDS=12 gate: below it, savings can't
matter (a >=10% saving is ~1s of audio, and several providers bill
a per-request minimum anyway — Groq bills 10s minimum), so the
whole pipeline is skipped using the duration we already probed.
Typical 5-10s voice notes now pay 1 ffprobe (~50ms), not 3 spawns +
encode (~0.3-1s; multi-second on small-VPS gateway hosts).
2. Shared encode profile (reuse, HIGH): the trim's ffmpeg command
duplicated _transcode_audio_for_stt's encode byte-for-byte (same
16kHz/mono/AAC-32k/faststart args, same subprocess.run kwargs).
Extracted _STT_M4A_ENCODE_ARGS + _run_ffmpeg_stt_encode(ffmpeg,
in, out, audio_filter=None); both call sites now share one owner,
so codec/bitrate/timeout changes can't drift between the paths.
3. is_truthy_value for the enable flag (quality, MEDIUM): raw
bool(cfg.get(...)) treated a YAML string "false" as enabled — the
exact bug class utils.is_truthy_value (already imported, already
used by is_stt_enabled and the xai/elevenlabs flags) exists for.
4. All-silence guard scales with keep_ms (quality, LOW): the fixed
0.3s floor equals the default keep window, so an output consisting
solely of one kept pause could pass as "speech"; now
max(0.3, 2*keep_seconds).
Also: _probe_audio_duration docstring documents it as the canonical
sync seconds-probe (gateway/run.py and the Telegram adapter carry
local variants of the same ffprobe invocation).
Tests: 24 now — YAML-string-false disables; short clips skip the
encode entirely (encoder mock asserted not-called); E2E fixtures
moved past the input gate. E2E re-verified: 13.2s note -> 6.2s
(-53%), 8s clip skipped with 1 probe.
Local faster-whisper gets Silero VAD (bf8004e3a) so silence never
reaches the model. Cloud providers got no such protection: the raw
file uploads untouched, so every second of silence in a voice note is
paid for twice — upload time and per-audio-minute billing — and cloud
Whisper hallucinates junk tokens on silent stretches exactly like
local Whisper did before the VAD hardening. A 13s voice note with two
long pauses is billed as 13s of audio to transcribe ~6s of speech.
Close the gap client-side: before uploading to a built-in cloud
provider (groq/openai/mistral/xai/elevenlabs/deepinfra), collapse long
pauses with ffmpeg's silenceremove filter, keeping
stt.cloud_trim_keep_ms (default 300) of every pause so word boundaries
and natural pacing survive. Uses ffmpeg, already a dependency of this
exact path via _transcode_audio_for_stt — no new dependency.
The trim is strictly best-effort — ALL of these upload the original
untouched, transcription never fails because of the trim:
- stt.cloud_trim_silence: false
- ffmpeg/ffprobe missing, trim failure, or timeout
- trimmed result ~empty (mostly-silence clip: the provider, not a
client-side dB heuristic, decides whether it contains speech)
- trim saves <10% (re-encoding for nothing)
Command-type and plugin providers are deliberately NOT trimmed: they
may wrap local CLIs that want the original bytes or run their own VAD.
E2E (real ffmpeg + faster-whisper): 13.2s voice note with 7s pause ->
6.2s upload (-53%); transcript of trimmed audio matches the original
on both utterances. Dense-speech and all-silence WAVs correctly fall
back to the original. 22 unit+E2E tests; STT/voice suite failures
identical to upstream/main baseline (all pre-existing).
* fix(desktop): hide the terminal overlay when its pane is inactive
One xterm is CSS-overlayed onto whichever `<TerminalSlot />` is active,
positioned with `position: fixed` from the slot's bounding rect. Keep-alive
tab layers stay MOUNTED when inactive — hidden with `visibility: hidden` +
`data-pane-hidden`, deliberately preserving their layout box so scroll state
and xterm survive a tab round-trip.
So an inactive terminal slot still reports a full-size rect identical to the
front tab's, and `rect.width > 0 && rect.height > 0` cannot tell the two
apart. The overlay stayed painted at z-4 over whatever tab the user switched
to, swallowing its clicks.
Sample the hidden state alongside the geometry: `Rect` carries `hidden` from
`isElementInHiddenPane(slot)`, `sameRect` compares it so a tab switch wakes
the tracker, the ancestor MutationObserver watches `PANE_HIDDEN_ATTR`, and
the overlay gates on `!rect.hidden`. `TerminalWorkspace` stays mounted
throughout — PTYs are never torn down, only the surface stops painting.
`opacity: 0` rides alongside `visibility: hidden` because Electron can keep
xterm's WebGL canvas composited after an ancestor goes hidden.
Refs #71407
* fix(desktop): collapse an active tool pane onto the workspace, not a neighbour
`setPaneCollapsed` on the ACTIVE pane of a shared zone that holds the
uncloseable workspace handed the active slot to `group.panes[at - 1]` — the
tab to its left, whichever that happened to be.
The workspace can't minimize (it would strand the app), so tab-switching to a
sibling is the right shape; picking a positional neighbour is not. In the
Focus preset the terminal is a tab in the workspace's own group:
[workspace, files, review, terminal]
Collapsing the active terminal therefore selected `review`. The user asked for
the terminal to go away and landed on a diff pane they never opened — and with
the overlay still painting (before the previous commit), it read as "the
terminal came back".
Hand the slot to the uncloseable pane itself. That pane is the zone's anchor:
it's the one member guaranteed to be a real destination rather than another
tool the user was not asking for. The positional fallback stays for the
defensive case of collapsing the uncloseable pane itself.
This is deliberately broader than one entry point — every route into
`setPaneCollapsed` for a shared zone gets it: the rail, the tab toggle, and
⌃`. Pure tool-only zones are untouched and still fold as a unit.
* fix(desktop): front the workspace when a fresh chat starts
`startFreshSessionDraft` resets the whole view — messages, usage, timers,
route intent, cwd — but left `$terminalTakeover` set. That atom is not a
cosmetic flag: `controller.tsx` binds it as the terminal's toggle store via
`bindToolPaneCollapse`, so while it stays true the terminal keeps the pane
fronted and ⌘N appeared to bounce straight back into the shell.
Clear it, then `revealTreePane('workspace')`. The reveal is not redundant
with the clear: takeover can already be false while the terminal is simply
the active tab (the flag stays true behind a stacked sibling, and tile flows
never touch it), so the state the user sees and the state the flag describes
drift apart. Clearing homes the common case; revealing states the intent
outright — a new chat shows the chat.
The terminal is not torn down. Tool panels collapse to a rail and keep their
PTYs; re-opening finds the same shell.
The `+` / ⌘T tile path needs no takeover clear — it fronts its new tile
through `revealTreePane` and relies on the hidden-pane-aware overlay.
* fix(desktop): reveal the workspace without closing the terminal
The fresh-session commit cleared `$terminalTakeover` on the way to fronting
the workspace. That atom is not a Focus-only fronting flag — it is the
terminal's open/closed state in every layout, and clearing it is wrong twice
over.
Only the Focus preset stacks the terminal with the workspace. Default,
Terminal deck, and Quad each give it a zone of its own, where it sits beside
the chat and obscures nothing — and there ⌘N minimized a terminal the user
had deliberately open.
The flag is also persisted, so the damage outlived the session. On the next
boot the Focus terminal tab is still in the strip and its zone is not
minimized, so clicking it only calls `activateTreePane`; `PersistentTerminal`
mounts its workspace solely while takeover is true, so the tab fronted empty.
`revealTreePane('workspace')` already carries the whole intent. Behind another
tab the terminal is HIDDEN, not closed: it keeps its PTYs, and the overlay
stops painting on the pane-hidden marker from the first commit in this branch
— which is what was actually covering the chat. Removing the clear costs
nothing and keeps the toggle store truthful.
Two regression tests, both verified to fail when the clear is reinstated: a
terminal in its own zone stays open and visible across a fresh chat, and a
Focus terminal tab still mounts after a restart.
Reported by Copilot review on #81019.
---------
Co-authored-by: izumi0uu <izumi0uu@gmail.com>
Co-authored-by: Ritesh Patel <60716910+DECRUX9812@users.noreply.github.com>
_download_image() and _download_video() both used client.get() +
response.content, buffering the entire media body into memory before
checking the size cap. A server that omits Content-Length could send
an arbitrarily large payload, causing OOM.
Extract _stream_download_to_file() shared helper: streams via
client.stream() + aiter_bytes(), writes chunks to a temp file, enforces
the running byte count against the cap after each chunk, and atomically
replaces onto the destination on success. Cleans up the temp file on
failure. Uses utils.atomic_replace() for cross-device/symlink safety.
Malformed Content-Length values are now caught and ignored instead of
crashing with ValueError; the streaming cap is the authoritative guard.
Approach adapted from PR #10440 by @WuKongAI-CMU (closed as stale —
14923 commits behind, reverted 32 commits of vision_tools.py evolution
including SSRF-safe client, retry classification, and lazy imports).
Closes#10440
_fts_teardown_trash_step deleted rows via 'WHERE key IN (SELECT key
LIMIT N)' — each chunk's subquery re-scanned from the start of the
table, so chunk k skipped past (k-1)xN already-deleted rows: O(n²)
total row visits. On a v22 shadow table with ~230K rows that is on the
order of 10^8 row visits, turning optimize-storage teardown into a
multi-hour grind on slow disks, with a write lock held per chunk.
Single-column INTEGER-PK trash tables now drain via a fts_teardown_<tbl>_progress
high-water marker mirroring fts_rebuild_step: each chunk claims rows
past the marker (SELECT ... WHERE key > ? ORDER BY key LIMIT N), deletes
the claimed range, and publishes the new marker in the same transaction.
Per-chunk work is bounded → O(n) total.
TEXT-PK tables (the FTS config shadow table, pk like 'version') and
compound-key tables fall back to the legacy chunked delete — those are
small by construction.
Fixes#79324
The dedup fix in #80679 reduced ~1.6k sequential API calls to ~30,
but those ~30 calls were still sequential. Extract the per-base-ID
resolution into an async helper and run all lookups concurrently
via asyncio.gather. Turns a 6-15s serial block into <1s.
Follow-up to #80700:
1. _await_disconnect_step was missing the try/except CancelledError around
asyncio.wait() that _await_adapter_cleanup_with_timeout already has.
When the outer fatal-handler timeout cancels disconnect() mid-step,
asyncio.wait does NOT cancel its inner task — the task was orphaned
with no observer. Add the same cancel+detach+re-raise pattern.
2. _queue_retryable_fatal_platform omitted credential_claim/listener_claim
keys that all 3 startup-path queue sites include. These are consumed by
the multiplex reservation logic to prevent secondary profiles from
taking the endpoint while a primary is queued. Pre-existing latent bug
— now fixed since the extraction makes it trivial.
After a network outage the Telegram fatal handler could hang inside
disconnect() and never populate _failed_platforms, so the reconnect
watcher had nothing to retry and the process stayed permanently deaf.
Queue retryable platforms before any disconnect await, bound the fatal
handler with an outer detach deadline, and release the Telegram token
lock / PTB close steps with detach-on-timeout so recovery cannot stall.
Follow-up to the #80376 salvage:
- TurnLeaseToken.degraded is dead code since acquire() started raising
TurnLeaseTimeoutError: the only constructor site passes the default and
repo-wide there are zero external readers. Remove the field, its ctor
param, the repr segment, and the always-False guards in rebind()/release();
the class docstring's 'retained for compatibility' claim described
consumers that do not exist.
- Revert pure black-rewrap churn in test_config_env_bridge_authority.py and
test_turn_lease.py (hunks on functions this change does not touch), keeping
the functional Windows-env/encoding additions.
- Turn the default-value test into an invariant: config default must equal
gateway.turn_lease.DEFAULT_LEASE_WAIT instead of pinning the 1800 literal.
- Rewrap the dangling 'Released' comment line in gateway/run.py.
Verified: 20/20 targeted gateway tests, ruff clean; mutation check — with
gateway/turn_lease.py reverted to pre-fix main the module fails, restored it
passes.
External hosts speak this protocol directly, so the parameter that
rewrites a session's stored transcript should not be folklore. Document
what each truncation field means, that an ordinal without
confirm_truncate is refused, and that a client must never hold the
ordinal in state across ordinary submits.
prompt.submit honored truncate_before_user_ordinal on every request. A
client that carried a leftover ordinal into an ordinary send therefore
issued something the gateway could not tell apart from a real rewind —
same method, same shape, an in-range target — and the cut was applied
with replace_messages(), which DELETEs the durable rows. One report lost
244 messages (296 -> 52) with no prompt and nothing to restore from.
The existing guard only covered ordinal 0, where the cut empties the
transcript; a mid-session ordinal sailed straight through. Only the
client knows whether a submit is a rewind, an edit, or a regenerate, so
require it to say so: an ordinal without confirm_truncate is refused on
4029 and neither memory nor the DB is touched. Desktop sends the flag
from the one place that builds these params, so every rewind path is
covered and a stale build fails closed with an actionable error instead
of quietly deleting a conversation.
- Share one HERMES_CRON_TIMEOUT parser (_cron_inactivity_seconds) between
run_job's inactivity monitor and the cwd-lock bound so the two sites
cannot drift - the bound must stay >= the inactivity limit or waiters
would fail while a healthy holder runs.
- Bind the timeout once per run_job; the raise reports the value that was
actually used for the wait.
- Timeout message covers the writer-blocked-by-readers path instead of
always blaming a workdir job.
- The finally's TERMINAL_CWD restore is now gated on _cwd_lock_acquired:
a fail-closed timeout raised before the env-set, so restoring there
replayed a pre-wait snapshot over the ACTIVE holder's live override
(pre-existing microsecond race from the #80912 snapshot placement).
- Comment accuracy: the bound is measured from the waiter's arrival; a
late-wedging or pre-agent-hung holder can outlive it.
The 120s bound added in #80912 proceeded WITHOUT the lock on timeout
(fail-open). That degraded mode fires on every overlap with a HEALTHY
long-running workdir job - the write lock is legitimately held for the
holder's entire agent run - and a workdir-less job that proceeds unlocked
executes its shell/file/code commands with the holder's process-global
TERMINAL_CWD override visible: silent wrong-directory execution, the
exact corruption _ReadWriteLock exists to prevent (see
test_reader_never_observes_writer_override). A degraded WRITER was worse:
it clobbered the active holder's override mid-run and later restored a
pre-wait snapshot over the holder's value.
Fail closed instead: on timeout the job errors loudly with an actionable
message (stagger the holder's schedule / drop its workdir) and is retried
on its next tick. A failed job is visible and recoverable; a job that ran
in the wrong directory is neither.
The bound is now derived from the cron inactivity limit
(HERMES_CRON_TIMEOUT, default 600s) + 60s margin instead of a flat 120s:
a wedged holder stops touching its activity clock, so the inactivity
monitor reaps it and releases the lock within that limit - waiters only
fail when even the monitor could not clear the holder. Healthy workdir
jobs shorter than the inactivity limit can no longer fail their waiters.
Design follows @necoweb3's #63959 (fail-closed semantic); its 30s flat
bound would have failed every waiter overlapping a healthy >30s workdir
run, which is why the bound is derived instead.
Bare-noreply author email (no NNN+ prefix) on the PR #80376 salvage is not
auto-skipped by check-attribution; add the mapping file ahead of the salvage
PR.
Sibling of the chat_completions zero-byte-args fix (previous commits):
a clean SSE close after content_block_start(tool_use) but before any
input_json_delta / message_delta yields an SDK final-message snapshot
whose content is NON-empty (the tool_use block is present, input={})
and whose stop_reason is None. That shape sailed past both
empty-stream guards (they only fire on empty content) and executed the
tool with empty input — no retry, no error: the same silent-data-loss
class as #80498, one provider transport over.
A legitimate completion always carries a stop_reason, so a
tool_use-bearing message without one is a mid-tool-call stream drop.
Raise EmptyStreamError for it, riding the same bounded stream-retry
(HERMES_STREAM_RETRIES) the eventless-stream case already uses.
Gate checked on both return paths (raw SDK snapshot and
accumulator-modified message). Regression tests cover the dropped
shape (mutation-verified: disabling the gate fails exactly that test),
the legitimate tool_use completion, and the text-only no-stop_reason
shape (pre-existing behavior preserved).
Locks in two gaps left by 015a114a2 (#80623): a mixed response where one
tool call completes validly while a sibling has zero argument bytes still
gets discarded whole via the shared partial-stream-stub path, and the
zero-byte trigger now has an end-to-end test through run_conversation's
retry loop, not just at the chat_completion_helpers unit level.
When the stream closes right after a tool call's name arrives but
before any argument bytes are delivered, has_truncated_tool_args
was never set (the existing check required a non-empty, whitespace-
stripped arguments buffer). The call fell through to a normal "stop"
finish_reason, later coerced to "{}" at dispatch and executed
silently with no arguments and no retry.
Route this case through the same dropped-mid-tool-call stub/retry
path already used for partially-truncated JSON.
Review follow-ups on the guard: state the accepted \n-residual in the
comment, reuse the span local in the next condition instead of
re-slicing, and short-circuit the substring checks before the regex.
Post-merge review of aecb9ca89 found the join guard over-broad: skipping
the join whenever ANY fragment self-matches _PREFIX_RE reopened a leak
for non-newline splits — sk-<15 chars>ESC<25 chars> masked only the
self-matching head and left the 25-char tail in cleartext (fully masked
before the guard; main never masked this shape at all, so the merged
state was still >= main, but the salvage's own coverage regressed).
Skip the join only when the span crosses a line boundary (\n / \r) —
that is the shape where adjacent legitimate text gets swallowed
(ghp_<token>-then-'button [ref=e3]' annotation bug). ESC/zero-width
controls never legitimately separate a token from prose, so joining
there is safe and restores full-tail masking.
Both legs mutation-checked: reverting to the unconditional skip fails
the new tail-mask test; removing the guard fails the annotation test.
Follow-up to the salvaged registration contract:
- share one _raise_if_cron_registration_error() helper for the two
byte-identical dashboard 424 except-blocks (web_server + cron router,
via the existing late() seam)
- add endpoint-level 424 coverage for /api/cron/blueprints/instantiate
(previously only the sync worker was tested)
- give chat/CLI surfaces a human-facing user_message() (job name, no
exception class name) and add a recovery hint (pause/resume or update
re-registers via provider reconcile) to the model/REST message
- consolidate five inline provider test doubles into one ABC-subclassing
make_cron_provider conftest factory; the web_server test double now
subclasses CronScheduler so an ABC rename fails loudly
- narrow the wrapper facade to keyword-only (**kwargs) and route the
tool's partial-failure return through tool_error()
Follow-up hardening on the salvaged #80687 shrink-merge guard, folding in
the best part of the competing #80703 (credit: @JoaoMarcos44):
- Stat-stamp fast path: load_jobs() inside a _jobs_lock() section records
jobs.json's (mtime_ns, size, ino) BEFORE reading; the save-path merge
and the post-stage verify skip their full read+parse when the stamp
still matches. The healthy no-race save (every mark_job_run /
claim_dispatch / heartbeat / advance_next_runs tick persist) now costs
one stat() instead of up to two full JSON parses.
- Fail-safe stamp discipline: the stamp is captured pre-read (a sibling
racing the load leaves it older than disk -> mismatch -> merge runs),
includes st_ino (mkstemp+rename always allocates a new inode, so
coarse-mtime filesystems cannot false-match), resets on section
entry/exit, and is INVALIDATED - never refreshed - after any write in
the section (a refresh would let a nested create_job be clobbered by
an outer caller's stale payload; probe-verified both directions).
- _merge_unexpected_disk_jobs no longer mutates the caller's list in
place - it returns a new list when anything was recovered, and logs the
recovered ids.
- The tolerant read cascade (utf-8-sig + strict=False fallback) is
factored into one shared _parse_jobs_file used by both load_jobs and
_peek_jobs_unlocked, so future encoding/shape fixes land once. The
peek's repair-free re-entrancy contract is now documented - a repairing
read on the save path would recurse through _save_jobs_unlocked (the
exact defect the stamp-reconcile approach in #80703 had).
4 new regression tests (fast path, no-mutation, corrupt-file save,
nested-create-vs-stale-outer-save), each verified to fail against the
implementation it guards.
Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com>
A stale or smaller in-memory snapshot could overwrite jobs.json and drop
CLI/tool-created jobs (including no_agent watchdogs) while the gateway
ticker was running. Merge unexpected on-disk ids back on save unless the
caller marks them in removed_ids.
CI slice 1/12 caught a regression in _mask_control_split_tokens: a
COMPLETE prefix token at end-of-line followed by ordinary text (browser
accessibility annotations: 'ghp_<tok>\nbutton [ref=e3]: Copy') was
joined across the newline into one stripped-copy match, and the mask
swallowed the adjacent line ('button' disappeared).
Join only when no fragment inside the span matches _PREFIX_RE on its
own — a self-matching fragment is already handled by the ordinary
prefix pass, so joining can only cause damage. All smuggling shapes
(ESC/ZWSP/newline splits with under-length fragments) still mask;
regression test added and mutation-checked (fails without the guard).
_write_checkpoint persisted s.command verbatim to ~/.hermes/processes.json.
Recovery only uses command for display/logging (the process is already
running; adoption re-validates PID + start time, never re-runs the
command), so masking is lossless.
A masked secret's visible head/tail could carry control bytes (newline,
NUL, DEL, C1 0x80-0x9F, zero-width) into config/status/dump output.
Strip every control incl. \n/\t (display differs from redact_sensitive_text,
which preserves \n/\t as line structure) before slicing; all-control values
return the configured empty fallback.
Consolidates the previously-closed #58079 approach (strip controls before
masking) - supersedes it.
Yishova
kshitij
HexLab98
Alan Hsu
kshitij
Austin Pickett
Chen Jin
kyssta-exe
poisdahl
texasich
dsad
joaomarcos
Gille
Soheil Fakour