Gate finding (2c + quality, convergent): the settle-before-trim split moved
the thinking-sound stop and the TTS end-of-text sentinel after
session["running"] = False. Both are non-blocking (a lock + Event.set; an
unbounded queue put), so deferring them gained nothing, and it opened a
window where a prompt admitted on the same session during the trim had its
thinking sound stopped by the old thread and the old stream's final chunk
cut as a barge-in. They run in _release_turn_scopes again, before
settlement; _post_turn_housekeeping keeps only the trim and the
HERMES_HOME override reset. (The original commit subject says "trim/TTS";
only the trim is deferred now.)
The settle-before-trim test now also asserts both audio teardown steps ran
while the session was still running.
The prompt-turn finally ran trim_memory (gc.collect + malloc_trim, 20-50 s
on multi-GB heaps), the thinking-sound stop and the TTS sentinel BEFORE
marking the session idle and logging the "tui turn finished" bookend. A
slow or blocked trim therefore kept the session busy and the bookend
missing for its whole duration.
Split _finish_turn into _release_turn_scopes (snapshots, one-turn model
restore, approval/secret/terminal/session scopes — unchanged order and
logging) and _post_turn_housekeeping (trim, thinking sound, TTS sentinel,
then the HERMES_HOME override reset so the trim still reads the session
profile's config, #58576). The settle block runs between them verbatim.
Refs #131740 (the reported 600 s watchdog hang is inside
agent.run_conversation, so this hardening does not close it).
Salvaged from #131773.
Gate finding (BN07 quality #3): the comment above _IMPORT_FLAG_COLS restated the
test module docstring (and called the sweep's auto_archived a user decision); cut
it to one WHY line. The donor-content and retry-adoption assertions exercise
adopt_session_lineage_from behaviour that already holds on main (verified: they
pass against the merge-base prod file), so they add no teeth for this fix.
Gate finding (BN07 quality #2): auto_archived was imported independently of
archived, so a payload with archived=0, auto_archived=1 stored a live row marked
as swept, a state the idle sweep never produces (it only sets the flag on rows
it archives). Clear it unless the row is archived.
Gate finding (BN07 quality #1): the session flags were read by truthiness, so a
hand-edited export carrying the string "0" imported as pinned/hidden/archived = 1.
The same function already coerces message flags with _coerce_or(..., int, 0);
use it for the session flags too.
A legacy export without flag columns must still import as an ordinary
visible session; the adopted canonical Bot Chat must keep its rename guard;
the donor must stay untouched; and a retried adoption must skip the
already-imported row instead of restoring the donor retirement archive
over the local copy. Extends the two existing tests instead of adding a
duplicate file.
Co-authored-by: itsflownium <157689911+itsflownium@users.noreply.github.com>
import_sessions copied archived but not auto_archived, so a chat the
idle sweep had archived came back as a deliberate archive. Resume and
compression only un-hide sweep archives (_unarchive_auto_archived_lineage),
so after a restore the chat stayed hidden for good.
(cherry picked from commit 130aa6f3cddee06238dd46f9dbc50eb8acef552f)
Stranded-session adoption copies a profile bot's chat from the default store
through import_sessions, which dropped hidden. Bot Mode chats are hidden, and
hidden is what makes the canonical Bot Chat canonical: the adopted copy showed
in every session listing, lost the rename guard on its registry title and
became a stale-archive candidate. hidden joins archived and pinned as a
restored flag.
(cherry picked from commit 1deb08a9cfd08845bc1de25cc502c3bcb51e8569)
import_sessions restored archived but not pinned, so a session restored from
a backup (dashboard import of `hermes sessions export`) or adopted from the
default store came back unpinned. Pinned is what the startup prune and the
stale-archive sweep exempt: an old pinned session was deleted by the next
startup after the restore.
Ported onto the split import helpers from #76206.
(cherry picked from commit 59ea00ea713f90a1aa0487e05de79e6690be0bdf)
The legacy AUTHOR_MAP already maps this email, but the per-email file the
release tooling now reads was missing; add it so the cherry-picked commit
is credited.
Quality-gate Lows (BS03 r1): resolved_count always equalled
len(self._username_resolved_ids) (each name is discarded from to_resolve on
its first match, so it adds exactly one ID), and resolved_allowlist_user_ids
filtered isdigit()/"*" out of a set that only ever holds str(member.id) and is
always initialised in __init__ and reset by the resolver. Drop the counter, the
filter, the getattr fallback (no test builds the adapter without the resolver
and then calls the accessor) and the stale "Numeric only" docstring line.
Behaviour unchanged (probe-BS03-1 output identical before and after).
Quality-gate Low (BS03 r1): test_filters_usernames_and_wildcard seeded
_username_resolved_ids with "teknium" and "*", values the resolver can never
write there, so it only pinned a defensive filter against impossible state.
Fold it into the kept accessor test, which now drives the real
_resolve_allowed_usernames (wildcard, unresolved username and numeric entry
in the allowlist) and asserts only the username-resolved ID comes back. It
fails on the pre-salvage accessor, which leaked the numeric entry.
Also drop the redundant _username_resolved_ids reset in the helper (the
resolver resets it first), hoist `import asyncio` to the module imports and
restore PEP 8 two-blank-line spacing between top-level definitions.
Keep the nickname/global-name impostor case (parametrized) and the
revoked-numeric-entry-after-env-reload case; both go red on the pre-fix
adapter. The display-name hint and numeric-only snapshot tests cover
the same code paths as the kept ones.
_resolve_allowed_usernames reset _username_resolved_ids only after the
early return for an empty allowlist / missing client, so a reconnect
after the allowlist was emptied kept the previous connect's resolved IDs
in the set the gateway authz union reads. Reset first so the set always
reflects the current connect.
resolved_allowlist_user_ids() returned every numeric ID in the adapter's
connect-time _allowed_user_ids, and the gateway authz layer unions that set
into the reloaded DISCORD_ALLOWED_USERS. So an ID removed from the allowlist
after connect (hermes pairing revoke from the CLI or dashboard, or a hand
edit) kept authorizing until the gateway restarted. #130920's in-place purge
only reaches adapters in the same process, and those commands run outside it.
Track the IDs _resolve_allowed_usernames actually resolved from username
entries and return only those: they are what the per-turn .env reload loses
(a0a63a1bc2); numeric entries are already read live from the env.
(cherry picked from commit 90451505545745dbb27ecad28f2c431ad5afd301)
_resolve_allowed_usernames resolved a DISCORD_ALLOWED_USERS name through member.name,
member.display_name or member.global_name, and the first match took the slot. Any member of a
shared server could set a nickname or display name to an allowlisted username, get the
resolved ID (DMs, tools, approval buttons) and lock the real user out.
Match the unique account username only, as #125034 did for SimpleX display names. An entry
that matches only a display name stays unresolved and gets a connect-time hint. Tests cover
a copied nickname and a copied display name seen before the real user, and the hint; the
Discord docs now say display names and nicknames are never matched.
(cherry picked from commit 252f0bccab39afe94f0f71254e47a8f785ec1fb0)
The channel gate ran inside _handle_slash_command, after the bolt listener had
already acked with an ephemeral "Running /x". In an ignored or non-allowed
channel the invoker was promised a run that never came, and an ignored channel
("never touched") still got a bot reply. Move both checks into
_slash_channel_gated() and consult it before the ack: gated -> bare ack().
_handle_slash_command keeps calling the same predicate for direct callers.
The listener becomes the _handle_hermes_command method so the existing
parametrized gate test drives the real ack path (BN11 gate r1 2c Low finding).
The allowed_channels guard recomputed str(channel_id).startswith("D") inline and
the next line computed it again as is_dm. Hoist is_dm above the gates and reuse it.
Same behaviour, one DM classification (BN11 gate r1 quality/reuse Low finding).
The message path drops a message from an ignored channel (_prefilter_inbound)
and, when allowed_channels is set, from any non-1:1 conversation outside it
(_channel_gate_allows). _handle_slash_command applied neither, so a slash
command ran in a channel the operator had excluded. Apply both gates by
channel id alone, before any lookup, so a failed conversations.info can never
drop a control command.
Rebuilt from 2b7cde506c (#128767). Fixes#130358.
Gate finding (BN06 r1 quality, Low): the --session-id path never reads the
export filters, so this block passes on the merge base too (probe-BN06-3) and
pins behaviour the stack does not change.
Gate finding (BN06 r1 quality, Low): the subset check rebuilt candidates with
the same list_prune_candidates kwargs _cmd_export hard-codes, so it would still
pass if prod and test both dropped include_pinned. Assert the exported ids
directly; keep the pinned/archived flag asserts.
Gate finding (BN06 r1 quality, Low): the missing-id check lived in three
places (_collect_sessions, _export_trace, _export_markdown_single), and
_collect_sessions had grown a list|int|None return with an isinstance
branch in _export_flat just to carry the exit code. Resolve the id once at
the top of _cmd_export and return _not_found's non-zero code there; the
per-format copies and the int return are now dead and reverted.
The salvaged #131075 fix makes filtered `hermes sessions export` include
pinned rows (archived were already included), so a backup taken before
`prune --include-pinned` holds every row prune can delete. Surface that in
the export --older-than help so users do not assume prune's keep rules
also narrow the backup.
Co-authored-by: Alisson Galindo <alissongalindo@users.noreply.github.com>
Co-authored-by: Yuan Li <dskwelmcy@163.com>
The stranger[0] membership assert was circular (it re-found an element
taken from the same list) and raised IndexError instead of a useful
message when the Timer did nothing. Assert the list is non-empty and
shutdown-only in one line. The trailing time.sleep(0.2) did no work:
every stranger event is recorded before woken.set(), and the owner's
close events happen before pytest.raises exits.
_attempt_stream_socket re-implemented a narrow slice of the existing
_socket_from_stream helper (get_extra_info only) behind a blanket
try/except. Delegating to the shared helper keeps one socket-discovery
path (it already suppresses get_extra_info failures and covers the
_sock / anyio backends), and the remaining lookup cannot raise.
Also trims the close_attempt_stream docstring to the essential why.
_shutdown_socket already swallows OSError itself (agent/agent_runtime_helpers.py:
"except OSError: pass"), so the extra try/except in close_attempt_stream only hid
programming errors behind a debug log.
Closes#130115.
Co-authored-by: Yuan Li <dskwelmcy@163.com>
_CodexStreamGuard._close_client_on_timeout keeps the FD-ownership contract for the client's pool sockets
(shutdown() from the Timer, close() only on the owning thread), but then called close_attempt_stream(),
and an SDK Stream.close() on an unfinished response ends in sock.close() (httpx Response.close ->
httpcore HTTP11Connection.close -> SyncStream.close). That releases the descriptor while the owner is
still inside SSL_read: the kernel hands the number to the next open() and OpenSSL writes a TLS alert
record into that file. Two SQLite files in a gateway process lost bytes 5..28 of their header this way
(b"SQLit" + 17 03 03 00 13 ...), 15 ms after "Codex auxiliary client aborted (timeout,
tcp_force_closed=1, deferred_close=stranger_thread)".
close_attempt_stream() now does a real close() only on the owning thread. From any other thread it
shuts down the socket under the stream (stream.response.extensions["network_stream"]), which wakes the
reader and keeps the FD; the owner closes the stream in its finally (release_stream). A stream with no
reachable socket (a test double, a non-httpx transport) is closed as before.
Fixes#130115. Follow-up to #70773.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 33cfb5ea6550f4a56ea0faa072ddf7f91412af49)
The header said the duplicate paints "on current main" (stale once the
fix lands) and pointed at boundary "specs below" when only the
message.start spec remains. Also apply prettier to the delta helper,
which --check flagged.
collapseDuplicateFinalOntoIdenticalInterim has one caller and it sits
inside `if (streamIndex >= 0)`, so the `streamIndex < 0` check could
never fire. Note the precondition in the comment instead.
The query side reads identity through transcriptRowIds, which takes
sourceRowId from every part, while the index side still filtered on
`part.type === 'text'`. Only text parts are ever stamped, so the filter
did nothing but make the two sides use different rules; drop it.
Replace the comment that restated the rowId-vs-last-part ternary with
the reason for it: a fold with a rowId is addressed by its first source
row (hydration's key); without one, its last part row is the final
reply the settled live bubble holds, so pending rows land before it.
No behaviour change.
identicalInterimSiblingIndex now returns -1 for a failed completion, so
the stream path and the fallback settle path share one failure gate
(previously the fallback path settled a failed completion onto the
sealed interim). collapseDuplicateFinalOntoIdenticalInterim drops its
own hasFailure option and the dead guards a non-negative interim index
already implies, and shares the merge-and-drop tail with the tool-interim
collapse via a private helper. The no-fallback branch's
`identicalInterimIndex >= 0` arm was unreachable (any such index is a
visible assistant past lastUserIndex, which makes fallbackIndex >= 0) and
is removed.
partRowAnchor reuses transcriptRowIds instead of re-deriving the row
list; the anchor choice (message row first, else the last part row) is
unchanged.
The extra `existingPartRowIndices.get(message.rowId)` clause matched a
fetched fold as soon as its FIRST row was held as a part of another
bubble, short-circuiting partRowAnchor's every-row-held rule. Any unheld
row in that fold (e.g. the final reply) was then dropped and never
rendered. For single-row messages partRowAnchor already covers the
part-row match, so the clause only added the bug.
Add an it.each row for that shape and make the table assert explicit
merged ids instead of branching on a `kept` flag.
Keep the headline spec (red without the collapse) and the message.start
occurrence-bound spec (red without the guard); the distinct-segment and
tool-row boundary specs already pass on main, so they pinned nothing new.
The part-row anchor dropped a fetched fold as soon as ANY of its text rows
was already held, so a fold carrying a tool call and narration the store
did not hold lost both (Enough1122's review on #124472). Anchor through
the part rows only when all of the fold's source rows are held; otherwise
append it as before. Also drops the 'ponytail:' note this replaces and
folds the case-B spec into the existing test as a parametrized pair.
Refs #123801
Co-authored-by: finn763 <165816600+finn763@users.noreply.github.com>
Backfill merge matched only message-level row ids. A hydrated fold is
addressed by its first source row while the final reply rides inside as
a text part sourceRowId, so an overlapping older page appended the fold
next to the settled live bubble and the reply rendered twice. Closes#123801
(cherry picked from commit 50021e8c0a668797d54db0874c6cc4662d702369)
message.start starts a new occurrence while keeping the prior messages (a
chained or prompt-less turn) and resets interimBoundaryPending. Scanning back
only to the prompt row let interim('X') -> message.start -> delta('X') ->
complete('X') match the PREVIOUS turn's interim, settle it and delete this
turn's live bubble.
Gate the scan on the same flag collapseDuplicateFinalAfterToolInterim already
requires — it is exactly 'an interim was sealed since the last message.start' —
and add the same-text restart regression (fails without the guard). Copilot
review on #126023.
(cherry picked from commit 45ced2713efa4cd92acae3783205cdf94c6f5e72)
A reply the client sealed as an interim before it had the text on screen
(the interim materialized its own bubble because a superseded attempt's
frames had cleared the live stream) is painted twice when the turn then
streams the same text and completes: the sealed interim above, the settled
bubble below, while the store holds one row (#123801).
An interim whose text IS the final text is this turn's reply, not a second
segment, so completion now settles onto it and drops the live twin. Interims
carrying tool rows are never collapsed, and a distinct segment keeps its own
bubble.
Reproduced deterministically on main; the regression test fails without the
fix and the 222-file use-message-stream / chat-messages / chat suite passes
with it.
(cherry picked from commit f2980a9c9868a41e4ef3bf216c161acbc260e61b)
publish_runtime_status enqueues writes on a worker thread, so reading the
status file right after the reconnect raced the writer on CI (KeyError 'slack').
Startup only queues an adapterless platform when it can heal on its own (an
unregistered plugin with a bot credential), but the reconnect watcher's
`if not adapter:` branch kept every queued platform retrying forever. A
platform that became builtin-probe-failing or registered-but-None after being
queued (e.g. queued from a runtime fatal or startup connect failure) would
re-warn at the backoff cap indefinitely (#5196 fleet nodes).
Move the predicate into GatewayAdapterLifecycleMixin._adapter_may_heal and use
it at both sites: the watcher now marks such a platform fatal/needs_attention
with the non-retrying message and drops it from the queue. The late imports
of _BUILTIN_ADAPTERS / platform_registry / _platform_has_bot_credential keep
the same seams tests patch.
Tests: fold the builtin-missing-creds "fatal, not queued" case into the
missing-adapter test, run the heal test straight-line, and pin the watcher
dropping a registered plugin that still returns None. Still 2 tests.
The previous commit queued every enabled platform whose adapter came back
None. Some of those cases cannot heal without a config change: a builtin
whose requirements probe fails on missing creds (BlueBubbles, QQBot), a
Signal config that fails validation, a registered plugin that returns None,
or an empty bot credential. The watcher then re-warned about each of them
forever at the 5-minute backoff cap. That is the #5196 fleet case, where one
shared config.yaml runs on nodes holding only some of the credentials.
Now startup queues a platform only when it is not a builtin, no plugin has
registered it, and it has a bot credential. Every other adapterless platform
is still flagged (adapter_unavailable, needs_attention) but is marked fatal,
without the "Retrying" claim, and is not queued.
Also:
- One shared _adapter_unavailable_message() so the reconnect watcher keeps
the actionable plugin/deps/credentials hint instead of overwriting it with
a short message after the first retry.
- The "queued for retry" log counts startup_retryable_errors, the list it
names, rather than _failed_platforms (which includes adapterless entries).
- The all-adapterless warning now mentions background retry.
- Tests share one _runner() helper. The heal test has a builtin-with-missing-
creds row that must not be queued.
An enabled platform whose plugin failed to load (no adapter) was only
flagged at startup and never retried, so a transient plugin-load failure
left it down until a manual restart (#126356). Queue it in
_failed_platforms as retrying/adapter_unavailable (still needs_attention)
and keep it queued with backoff while _create_adapter returns None, so the
existing reconnect watcher connects it once the plugin registers. It is no
longer parked as fatal, so the gateway no longer stays degraded after heal.
Fixes#126356
Co-authored-by: Zheqing Zeng <26277217+zengzheqing@users.noreply.github.com>
prune_never_active_keyed_sessions now returns (deleted, entries_dropped,
skipped) from its own guarded-row list. `sessions prune --never-active`
printed candidates-minus-deleted, which lumped rows that vanished
concurrently in with live-turn skips and needed an "or already gone"
hedge; it now reports only rows spared by a live turn or compression lock.
The docstring claimed a failed sweep rolls back whole; the row sweep is one
transaction but routing entries are dropped in a second write after it
commits, so say that.
The kept guard test now also covers a candidate that vanishes between
listing and the sweep: its routing entry must still be dropped (the r1
per-row loop left it behind, so the gateway would resume a nonexistent id).
Its contract is a boolean "deleted or not" (callers like the CLI /new and
/clear hygiene path treat False as "keep it"), and every other non-candidate
(messages, title, children) returns False. Raising SessionActiveWriteGuardError
here forced every caller to grow a try/except for what is just another reason
the row is not empty-and-idle.
The per-row delete_session loop ran N write transactions and N routing scans,
and a row that vanished between list and delete (concurrent delete) returned
False and kept its routing entry alive, pointing the gateway at a nonexistent
id. delete_sessions already has the exact delete_session semantics (delegate
cascade incl. guard scope over delegate children, branch orphaning, messages,
system-prompt GC, transcript files) in ONE transaction and reports guarded rows
via skipped_ids. Routing entries are now dropped once, after the commit, for
every candidate except the guarded ones; an aborted sweep rolls back whole, so
no route is stranded and the mid-loop abort test is obsolete.
`hermes sessions prune --never-active` now tells the user how many candidates
were skipped (mid-turn or already gone) instead of silently reporting less.
Keep three tests: guarded row survives with its routing entry, a mid-loop
abort strands no live row, and delete_session_if_empty refuses under a
lease. The compression-lock case exercises the same _guarded_ids path as
the turn-lease case, so it adds no coverage. Also correct the abort
test's docstring (it described the ordering backwards) and drop an
unused import.
