Review findings on #110914 (@ehz0ah): the psutil leg compared watched
abspath against the kernel-resolved path psutil reports, so a symlinked
HERMES_HOME on macOS returned no holders and let the fallback probe mint
replacement sidecars under a live writer. Both sides now realpath.
doctor's `file:{path}?mode=ro` truncated at '?'/'#' in a home name;
build the URI with as_uri().
`hermes doctor --fix`'s WAL checkpoint and `repair_state_db_schema`'s
preflight documented themselves as fail-OPEN: `live_writer_holds_db` only
refused on unknown/deleted/uninspectable holders and then trusted a
`BEGIN IMMEDIATE` probe, which is blind to a `journal_mode=DELETE` reader
(SHARED only) and cannot run on a malformed file — exactly the states repair
and checkpoint get invoked in. A repair in a second process then REINDEXed /
VACUUMed a file the gateway still held (#103339 item 2).
- `hermes_state_holders.live_writer_holds_db`: any foreign holder of the DB or
a sidecar is a live holder; the probe is only an additional positive signal.
- doctor `--fix`: the checkpoint runs on `_exclusive_repair_db_guard`'s
connection instead of a bare writable `sqlite3.connect`, so an opener
arriving after the scan is refused, not joined; `_session_count` is a
`mode=ro` reader.
- Normal SessionDB writers are untouched: gateway + dashboard in two processes
both keep writing (a process-wide flock on the write path — PR #109270's
shape — would break that).
Tests: the two-process repair race test releases the test process's own
header-probe fd (it is a genuine holder now); the mid-repair writer fixture
opens its connection after staging starts (a pre-existing holder is refused up
front, which is the point).
Refs #103339#100896
Requests accumulate history, so the mock re-counted the start receipt on
every follow-up request (observed 2 == 1). Dedupe seen receipts and pin the
new contract: the owned notify_on_complete completion resumes in-process as
one follow-up user turn carrying the child's output and exit code.
The budget test's frozen clock bound nothing (a fresh per-round deadline
passed it too): it now drives two rounds with an advancing clock and
asserts round 2 waits only the remaining budget (600 then 300, not 600
and 600). New: the timeout-with-drained-texts stop (one wait, texts still
run), and the finalize skip (_wait_for_oneshot_background_completions
must not re-wait once _quiet_notify_linger_done is set).
Durable async_delegation rows carry a delivery ledger that every other
drain consumer honors (cli_process_notifications, tui session_notifications,
gateway run_notifications). Without the claim/complete handshake the row
stays delivery_state='pending' and restore_undelivered_completions re-queues
it on the next process start, so the next chat -Q on the resumed session
injects the same delegation result twice.
The HERMES_SESSION_* prefix match also stripped non-identity knobs
(HERMES_SESSION_STALL_TIMEOUT tunes gateway stall watchers). The strip now
uses gateway.session_context's session env names (_VAR_MAP) — synced with
the session binding surface as vars are added — imported unguarded like
tools/approval_context already does on a hotter path; the except fallback
was an unreachable branch whose 3-name tuple silently narrowed the strip.
Test pins STALL_TIMEOUT as the kept negative case.
_quiet_notify_linger_done was set BEFORE continue_quiet_notify_completions,
so an exception between the flag and the loop's first wait made the finalize
pass skip a linger that never ran (children SIGPIPE — the #90879 class).
The flag now lands in a finally around the loop call: once the loop's first
wait has started, budget is consumed and the finalize re-wait is correctly
skipped; before that, an exception still lets finalize linger.
_follow_up now calls _sync_cli_session_id_from_agent like the main turn,
so a mid-run compression rotation during a follow-up turn cannot leave a
stale session id on the stderr exit line (read by automation wrappers)
or on the notify-drain ownership key.
bind_quiet_session_key returned a (token, reset_fn) pair guarded by a
try/except around ContextVar.set (which cannot raise for a token produced
in the same frame) and an except-pass in the finally — the banned
dead-defense shape. It is now a @contextlib.contextmanager and
_run_quiet_single_query wraps the turn in a plain `with`, flattening the
double try nesting; the sys.exit(130) interrupt path still passes through
the reset.
drain_notifications pops every owned event off the shared queue; the
salvaged loop kept only type=completion texts, so an owned
async_delegation result was consumed and silently dropped — neither
injected as a follow-up turn nor requeued for another consumer.
Every drained event type renders formatted text, so the loop now injects
all of them. Regression: test_quiet_notify_loop_injects_owned_async_delegation_events.
The salvaged loop called wait_for_pending_completions(None) with a fresh
default 600s deadline on every round, and _finalize_single_query then
re-waited the full timeout on the same stuck notify_on_complete child:
a hung child blocked a quiet one-shot 2x-9x longer than before.
One deadline now covers the whole run — the loop passes the remaining
budget each round, stops after draining once a wait times out (a timed-out
process never fires this run), and the finalize pass skips its re-wait
when the loop already consumed the budget.
Regression: test_quiet_notify_loop_shares_one_linger_budget (one wait
call, full budget, on a stuck child).
Quiet chat -Q inherited the dispatcher's HERMES_SESSION_KEY, so a nested
message_agent notify was addressed to the grandparent and B never woke.
Bind this session's key, strip inherited session identity from the delivery
child env, and continue owned notify completions in-process before stdout.
`hermes_state_dbfile._canonical_sqlite_path` was a byte-identical copy of
`hermes_state_holders.canonical_sqlite_path`; keep the public one and repoint
the two hermes_state call sites. No import cycle: holders is stdlib+psutil.
Review follow-up: a sentinel written before the create_time stamp has only
the claim time, but that is epoch seconds too, and the owner was born before
it claimed while a reuser was born after the owner died. One-sided compare
instead of trusting any live PID. Dead monkeypatch line removed from the test.
`detect_unclean_exit` decided "live owner mid-handover" by comparing the
sentinel's `start_time` (the ledger claim, `time.time()` seconds) with
`gateway.status.get_process_start_time(pid)` (proc clock ticks on Linux,
centiseconds elsewhere — its own docstring says it is only comparable with
itself). The two never matched, so every `--replace` takeover whose old owner
was still tearing down read as a crash and was logged/persisted as one.
The sentinel now carries the psutil `create_time` (stamped at claim since
8c3a35b69d), so the guard compares that with the live PID's create time —
same producer, same unit. A pre-stamp sentinel cannot disambiguate PID
reuse; a live PID is taken as the owner, as the psutil-silent case already
was. Test drives all three cases; it fails on the previous ledger.
Found while reviewing the start-attestation follow-ups (#110958).
Review follow-ups on the identity binding:
- The sentinel's `start_time` is `time.time()` at `record_startup`, seconds
after the process was born once imports finish, so comparing it with
psutil's create_time within 2 s would have read every real gateway as
undecidable and silently stopped the #109538 cold-start. `record_startup`
now stamps `create_time` (psutil birth via the existing
`process_identity._process_create_time`), `mark_exited` carries it, and
the attestation compares birth to birth. A sentinel from a gateway older
than the stamp falls back to the PID-only rule.
- A resume token written by pre-generation code and resumed by this code
probes the marker again instead of skipping the spawn.
- Horizon allows a 60 s backwards clock step; the unused `now` parameter is
gone; the create-time tolerance is a named constant; the read-then-unlink
in `_consume_start_attestation` is documented as best-effort.
`_attested_pid_exited_cleanly` matched the lifecycle sentinel by numeric PID
only, so a stale marker for PID 111 flipped from "clean exit" to "crash" once
an unrelated PID 222 lifecycle overwrote the sentinel, and a reused PID's clean
exit could vouch for a different life (#110020 review, gateway_windows.py:937).
`_write_start_attestation` now records `create_times: {pid: create_time}` via
the existing `process_identity._process_create_time`; `mark_exited` carries the
running sentinel's `start_time` onto the exited sentinel; the attested probes
fail closed for a bound PID whenever the sentinel cannot be shown to describe
that incarnation (other PID, start time off by > 2s, or no start time) —
"unknown" never reads as "dead". A missing sentinel still reads as dead, and
markers without `create_times` keep the PID-only rule.
Tests: two attestation tests (stale marker vs. moved-on sentinel → no
authority; own incarnation keeps authority / clean exit / legacy marker) and a
ledger test for the carried `start_time`. Mutation: with HEAD's prod files the
no-authority test and the ledger test fail.
`attested_death_generation` matched the clean-exit sentinel by PID only and never looked at
the marker's `ts`, so a historical marker (e.g. one left behind before a Desktop-owned era)
could later override Desktop ownership and authorize a duplicate gateway (#76129).
The read-only probe now treats a marker whose `ts` is missing, unparsable, in the future or
older than `START_ATTESTATION_MAX_AGE_S` (24h — the marker only bridges the seconds between a
✓ and the next CLI invocation) as no authority: `None`, fail closed, marker left unconsumed.
Partial follow-up to #110020 review thread (d); binding to per-PID process create_time is left
for a follow-up (needs `mark_exited` to carry `start_time` into the exited sentinel).
The resume token only recorded `cold_start_if_installed: bool` and execution re-read the
mutable one-shot start-attestation marker to decide whether a Desktop-owned install still owed
a cold-start. A concurrent `hermes gateway status`/`start` (`check_start_attestation`) consumes
that marker between plan and execution, so the spawn was skipped and the token cleared with no
gateway running.
The marker now carries a `generation` nonce. The plan records the generation whose unclean
death authorized the cold-start on the token (`attested_generation`); execution authorizes the
spawn from the token, still re-checks live gateway PIDs, and consumes the marker only while it
is still that generation — a newer marker written by a concurrent start keeps its own report.
`attested_gateway_died` becomes `attested_death_generation` (`None` = undecidable, fail closed).
Follow-up to #110020 review thread (a).
_cold_start_windows_gateway_after_update cleared the dead attestation as soon
as _spawn_detached() returned a PID, before _wait_for_gateway_ready() proved
the gateway survived. When readiness failed, the RuntimeError registered the
retry, but the retry then saw Desktop lifecycle ownership with no marker and
returned success without spawning anything — the silent outage of #109538
came back through the retry path (#110020 review). The marker is now
consumed after readiness is confirmed, so a failed spawn leaves the retry
its recovery obligation.
_attested_pids_from kept any `isinstance(p, int)` item, so `[true]`, `[0]`
and `[-1]` each survived as a "PID" and made attested_gateway_died([]) read
True — a malformed marker could override Desktop gateway-lifecycle ownership
and spawn a duplicate gateway (#110020 review). PIDs are now `type(p) is int
and p > 0`, and one malformed item fails the whole list closed: the writer
never emits such values, so a partially-bad list is not trustworthy either.
Gateway, dashboard, ACP server and the CLI already hold one registry-shared
SessionDB per state.db path, yet several in-process call paths still opened
a bare SessionDB() beside it. Each one is a full writer: schema init, write
lock, token-writer thread and a close-time WAL checkpoint. On a dashboard
serving overlapping requests that stacked up to the "5 live SessionDB
handles" precursor within seconds; per #110544 they are now harmless to each
other's WAL generation, but the leak itself remained.
Pure readers attach read_only=True (no writer connection, no write lock):
- plugins/hermes-achievements/dashboard/plugin_api.py::scan_sessions
(dashboard, per background scan and per /rescan; highest-frequency site)
- hermes_cli/console_engine.py::_session_db (dashboard console; list,
stats and export are reads; rename/optimize opt in to a writer)
- tools/process_registry_results.py::_owns_result (gateway, per retained
result load)
- hermes_cli/main.py::_session_db (last-session / title / cwd lookups)
- hermes_cli/terminal_breadcrumbs.py, hermes_cli/status.py,
hermes_cli/main_tui_launch.py (lookup one-shots)
Writers share the process's registry handle (hermes_state_registry.acquire;
close()/release_or_close release one refcount):
- acp_adapter/session.py::SessionManager._get_db — the AIAgent it builds
acquires the same path, so the ACP server held two writers per process
- hermes_cli/kanban_db_dispatch.py::_retag_legacy_worker_sessions
(gateway dispatcher tick)
- hermes_cli/main.py::_create_titled_session, hermes_cli/oneshot.py,
hermes_cli/foreign_sessions.py — the CLI acquires the same handle a
moment later
The "N live SessionDB handles" warning now counts only writable members:
read-only attaches are the sanctioned per-request shape for dashboard
routers and CLI lookups, and counting them turned a healthy topology into
an operator alarm (#100896 field reports of restart loops keyed on it).
Live repro (one registry writer + 4 overlapping dashboard/gateway paths in
one process): before 5 writable opens, 5 live handles, warning fired;
after 1 writable open (the registry handle), 0 from the request paths,
no warning.
Refs #100896#103339
`_open_session_db_for_profile(None, read_only=True)` is the path every
dashboard router already takes for this process's state.db; the inline
`Path(_default_db_path())` re-derived it. Docstrings now state the verified
rationale (no second writable owner; the read path heals through one
writable open when its probe fails) instead of asserting the FTS corruption
mechanism the reviewer showed is already fenced by rebuild admission.
Review follow-ups on the refresh transaction:
- `_provider_state_transaction` takes a `timeout_seconds` applied to BOTH the
active and the root lock. The refresh passes max(default, refresh timeout
+ 5 s); before, only the profile lock used that budget and root's lock kept
the 15 s default, so the waiting profile raised TimeoutError instead of
adopting whenever the peer's POST ran long. The regression test now holds
the endpoint past the lock floor and fails without the passthrough.
- Peer adoption requires a stored access token as well as a rotated refresh
token; an incomplete stored pair falls through to the refresh.
- `_save_codex_tokens` keeps its body: the per-path lock is reentrant, so the
refresh calls it inside the open transaction (as the CLI-recovery path
already did) instead of a split-out helper.
Follow-up to #110024 (ehz0ah's review thread). `_refresh_codex_auth_tokens` POSTed the
single-use refresh token to OpenAI first and only then entered
`_provider_state_transaction("openai-codex")` for the write-back, so root's lock covered the
save alone. `resolve_codex_runtime_credentials` holds only the caller's own profile lock, so
two profiles borrowing the same ROOT grant could both submit `old-rt`; last root save won and
OpenAI answered `refresh_token_reused` / revoked the family — the failure #87503 exists to
prevent.
The transaction now spans re-read -> endpoint refresh -> write-back:
- enter `_provider_state_transaction` first; the yielded state is root's, re-read under root's
lock. If its refresh token already differs from the one we were about to submit, a peer
rotated it: adopt the stored pair and return without touching the endpoint.
- otherwise POST and write back through `_store_codex_tokens_in`, the body of
`_save_codex_tokens` split out so it can run inside an already-open transaction.
`_save_codex_tokens` keeps its signature for the login/import/CLI-recovery callers.
Holding the advisory flock across the network call is safe here and already the established
shape: `resolve_codex_runtime_credentials` holds the active-store lock across the same POST,
and every waiter's timeout is `max(AUTH_LOCK_TIMEOUT_SECONDS, refresh_timeout + 5)`, i.e. it
outlives one full endpoint timeout. `_load_auth_store` readers never take the lock, so
readers are not blocked; `_file_lock` is reentrant per thread per path, so the nested
transaction inside the caller's lock and the CLI-recovery save inside the transaction both
re-enter cleanly. A release-POST-retake variant would reopen the window it is meant to close.
Test: two refreshers with the same stale pre-read pair against a rotate-once endpoint that
rejects any replay — the endpoint sees `old-rt` exactly once, both callers end with the
rotated pair, root holds it, the profile store stays unshadowed. Red on origin/main
(`refresh_token_reused` surfaces for the second caller).
The summary path had grown a verbatim copy of turn_api_request's 3-line
surrogate/ASCII chokepoint — the same drift class this PR removes for the
hand-rolled kwargs builder. Move the two lines and the #50959 rationale into
`message_sanitization.sanitize_outbound_kwargs` and call it from both sites,
so the next sanitizer step added to the main loop cannot miss the summary.
Tighten two comments: "same kwargs builder" (cache_control redecoration is
not re-applied here) and a `_summary_text` note that is true for all three
summary branches, not just the chat one.
`_summary_text_with_scrub` was the old helper plus a warning — `.content`
never carried tool calls, so nothing was being discarded that was not
already ignored. Restore the original name, keep the warning with the WHY
(the request now carries tools, this path never executes a call), and make
the tool-only test assert the log line so it actually binds the new code
path instead of passing on the pre-fix retry behaviour. Trim the builder
comment to the WHY.
Building the summary through `_build_api_kwargs` means it now carries
`tools`, but it still skipped the `_sanitize_structure_surrogates` /
`_force_ascii_payload` chokepoint the main loop applies after building
(turn_api_request). On cache-planned routes the main loop scrubs a deep
copy of the tools, so `agent.tools` can still hold a lone surrogate that
providers reject with a non-retryable 400 (#50959 class) — a request the
tool-less summary never used to make. Apply the same two passes here.
`_chat_summary_attempt` now builds through `_build_api_kwargs`, leaving
`_iteration_summary_chat_kwargs` (56 lines mirroring the transport by hand)
and its only consumer `AIAgent._resolve_lmstudio_summary_reasoning_effort`
without a caller. The transport already owns every quirk they re-derived
(fixed temperature, LM Studio `reasoning_effort`, portal tags, provider
preferences, pareto router plugin), so there is nothing to keep in sync.
Every caller tears the SSH connection down right after `await
cancelAndWait(scope)`, so resolving on this call's own barrier alone let a
connection-apply teardown overlap the pool stop's still-running afterStop
teardown for the same key. Wait for the composed barrier instead; chain the
barriers (drain promises never reject, so this equals allSettled). The test
now waits a macrotask and asserts that neither the apply nor the new
bootstrap runs before the first teardown completes.
A pool stop blocked in teardownSshConnection() holds drains[scope]; a
concurrent connection apply calling cancelAndWait() for the same scope
replaced that barrier with its own and, having nothing to drain, cleared the
map entry as soon as it finished. start() then saw no drain and began a new
bootstrap while the first SSH teardown was still running.
cancelAndWait() now composes with the drain already in flight and the entry
is cleared only when the composed barrier settles, so start() waits for
every active teardown. Regression test reproduces the pool-stop / apply
race; it fails on the previous coordinator.
Reported by ehz0ah on #110025 (follow-up to #106935).
The destination preflight / refusal path suppressed the whole progress lane
for a flat DM regardless of mode, so an operator who WROTE `tool_progress:
all` got nothing there (before #108668 they got text bubbles via the
fallback). Silence is right only for Slack's tier default, where no text
lane was asked for; explicit new/all now routes through the editable text
fallback instead. Also hoists resolve_tool_progress into the existing
display_config import in _run_agent_display_settings.
Test proven red on the salvaged head (adapter.sent == [] with `all`).
Review findings (Salt, adversarial pass on the two preceding commits):
- BLOCKING: a `tool_progress: null` (global, platform, or legacy overrides)
counted as an explicit mode because the gate tested key presence, while
the display resolver skips None and inherits. Null resolved to Slack's
tier default `off` and disabled cards, which is the default-off trap the
change exists to avoid. Explicit intent is now a non-None value (or the
env bridge). Tests cover null at each level plus null-over-global-all;
mutation to key-presence turns the three null cases red.
- TASTE: `_TaskCardState.egress_declined` now also latched on unsupported
destinations, so the name no longer described the field. Renamed to
`publication_suppressed` with both causes documented; readers unchanged.
- SHOULD-FIX: slack.md still promised an unconditional text fallback and
described the opt-in as independent of tool_progress. Rewritten: cards
follow an operator-written off (including /verbose), null inherits, an
un-threaded chat with the card lane active shows no tool progress, other
native failures keep the editable fallback.
In flat Slack DMs (reply_in_thread false) the connector refuses task cards
("slack task_card requires a thread anchor"; native Slack: "No Slack thread
target"). The card lane treated that like a transient native failure and
fell back to an editable text message, so every tool event re-rendered
"Hermes is working / - tool - running" in the DM: text tool progress on a
platform whose default is off, for an operator who never enabled it.
Treat unsupported-destination refusals as terminal for the turn (same
latch as an egress decline) and log at info; transient native failures
keep the text fallback.
Slack task cards are tool progress rendered natively, but the card lane
ignored the operator's tool_progress mode. Slack's built-in display tier
sets tool_progress off, so the lane was decoupled on purpose (#29483) to
keep cards on for unconfigured installs. The side effect: an operator who
wrote `display.platforms.slack.tool_progress: off` to silence tool updates
still got cards, and on relay-fronted Slack (where the connector always
advertises task_card) there was no setting that could turn them off.
Gate the card lane on operator intent, not the tier default: cards stay on
when nothing is configured, and go off only when tool_progress was written
as `off` (global, platform override, legacy overrides, or the env bridge).
`new`/`all` keep cards.
Tests assert the wire contract: no native card send, no stop, no fallback
text for an explicit off; card lane engaged for `new` and for the
unconfigured tier default (regression guard for #29483). The duplicate-tools
fixture now mirrors production's _safe_callback null-guard.
Three inline context-manager classes shared the same __enter__/__exit__
boilerplate and differed only in the events yielded before the raise.
Also drop the unreachable 'or agent.base_url' fallback: every
anthropic_messages init path sets _anthropic_base_url, and the two
sibling call sites read it bare.
The SDK's malformed-frame ValueError substrings were hand-copied into the
retry classifier and the error summary; a third SDK message would need
both remembered. The mid-tool retry branch no longer clears
partial_tool_names itself — _start_stream_attempt resets it for every
attempt. buffer_anthropic_tool_input's docstring states the trade: the
knob stays on the turn's kwargs and the changed tools block costs one
prompt-cache miss.
A tool_use block name recorded by a stream attempt that died before any
visible text survived into the next attempt: only the deltas_were_sent
mid-tool branch cleared result["partial_tool_names"]. When the retry then
streamed plain text and dropped, the partial stub blamed the stale tool
("Stream stalled mid tool-call (old_tool)") and the stale name could make
a later attempt look mid-tool-call when deciding whether the drop is
retryable. Reset it in _start_stream_attempt alongside
provider_tool_in_flight, which already has attempt-local semantics.
Regression: two-attempt stream (tool_use start + parse error, then text +
drop) — stub content and emitted deltas carry no stale tool name.
