The earlier trim dropped the only test of the `routed_profile_fire() and
not is_multiplex_active()` branch in the external-worker handoff: the
process flag is OFF (a desktop tick is not a multiplexer) yet a fire routed
to a sibling profile must serialize `multiplex_active=True` and hand the
worker an env without the launch profile's residue, and the context must
not outlive the handoff span. Without it that branch could regress to the
pre-fix behaviour unnoticed.
The PR shipped nineteen tests across six files, most of them variations of
one boundary. Keep the three that pin distinct behaviour:
- a routed desktop-ticker fire runs under multiplex semantics for exactly
its scope: a scope miss returns None instead of the launch credential and
the parent os.environ is byte-identical afterwards;
- a routed no_agent child never sees a launch-only name, whether the launch
.env defined it or a launch external source supplied it (applied or lost
to a pre-existing process value), while its own values come through;
- administrator-managed keys keep policy precedence over the routed
profile's own value.
Everything else was either a positive control of the same seam, a
set-membership check on a module-level constant, or a re-statement through
a different entry point.
Review findings on f5f88d5058. Three are defects the previous round introduced.
Managed keys were stripped as launch residue. Recording every dotenv load as
residue swept in the administrator-managed `.env`, which `_apply_managed_env`
applies LAST with override precisely so it beats the user's own `.env`. A
routed child then lost `ORG_POLICY_FLAG=managed-value` to the routed user's
`user-value`. Managed keys are now recorded separately, never enter the
residue set, and are re-applied over the routed scope in both child builders
(`scheduler_script`, the restart-safe handoff) so the child sees the same
precedence the launch process does. `kanban_db_dispatch` and
`scheduler_delivery` strip without any overlay, so for them the exclusion
alone is the guarantee; the test pins the case that exercises it — the same
key defined in both the user and the managed file.
Private hydration did not record supplied names. `_hydrate_profile_secret_sources`
now feeds `provenance` plus `skipped_existing` into the same ownership set the
process-global path uses; the provenance label map stays applied-only.
Removal cleanup cleared its marker before the fallible work. A raising
reload left the removed plugin's credential active with no retry, because the
next no-source discovery saw the flag already false. The marker is cleared
only after reset, reload and installed-scope refresh succeed.
Routed fire not multiplexed at the handoff. `run_one_job` enables the
context in `_install_fire_secret_scope`, which runs AFTER
`_launch_external_cron_worker`, so a routed desktop fire on the managed path
serialized `multiplex_active=False` and built the worker env with launch
residue and no scrub. The handoff now treats `routed_profile_fire()` as
multiplexed for exactly its own span; the worker re-establishes the state from
the payload as before.
Each fix was checked by reverting it and confirming its regression fails,
including the overlay half and the exclusion half of the managed fix
separately.
(cherry picked from commit 329cbd8963d68c45b425e95a5b11ade59f513960)
Review findings on d8c467f223, each reproduced through its production path.
Stale launch key. `strip_launch_profile_env` built its residue set from a
re-parse of the launch `.env`. A key removed or renamed in that file after
boot is still in `os.environ` with the old value (dotenv never unsets), and
the current file no longer names it, so it survived into the routed child.
`_load_dotenv_with_fallback` — the one chokepoint every dotenv load goes
through — now records the KEY names it put into the process env, additive for
the process lifetime (`launch_dotenv_keys()`), and the strip unions that record
with the current file.
Source name that lost to the process env. `_apply_external_secret_sources`
snapshots every name a source SUPPLIED (`provenance` + `skipped_existing`),
but `secret_source_names()` only exposed `_SECRET_SOURCES`, which is
provenance metadata and names applied values alone. A launch-profile source
that supplied `CUSTOM_VAULT_SECRET` while the process already had it was
therefore invisible to the scrub, and a routed child with an empty scope got
the launch value. Supplied names are tracked separately
(`_SOURCE_SUPPLIED_NAMES`) so the provenance labels stay honest, and
`secret_source_names()` returns the union.
Last plugin source removed. `_refresh_secret_sources_after_discovery`
returned before the cache reset and the installed-scope refresh whenever no
plugin source was enabled — and `discover_and_load(force=True)` unloads the
old registration first, so removing the final plugin source hit exactly that
return with the removed plugin's names still in the per-home snapshot and the
current scope. The manager now remembers that a discovery re-applied plugin
sources and, on the next discovery that finds none, reconciles once. A home
that never had a plugin source is still a no-op (pinned by the existing tests).
Regressions: the stale-key lifecycle and the skipped-existing case through
`_run_job_script` against a real routed child, and the removal case through
the manager. Each checked by reverting its fix and confirming the test fails.
(cherry picked from commit d464f5f6126a394cfb47937f683d3a5e2f141840)
The source-name strip added alongside the external-source fix ran
unconditionally. Outside multiplexing there is no other profile to leak from --
os.environ IS this profile's environment -- so popping those names relied on the
routed scope overlay putting each one back, which in turn relies on the
per-home snapshot recorded at boot. Correct today, but it made a single-profile
child's credentials depend on bookkeeping that has nothing to do with isolation.
Guard it the way strip_launch_profile_env guards itself: no multiplexing, no
strip. A single-profile no_agent child now keeps a byte-identical env even if a
source's snapshot were ever missing. Pinned by a regression that runs a real
child with a source-owned name in os.environ and no multiplex context; making
the strip unconditional fails it.
(cherry picked from commit afa429b30a1c9c9b4c011f7d2426a9f099911596)
Two credential-isolation gaps found in review of the previous head.
1. strip_launch_profile_env() only knows dotenv- and terminal-config-owned names,
but external secret sources (vault, 1Password, ...) also write their names into
the shared os.environ and are tracked in secret_source_names(). A name the LAUNCH
profile's source supplied therefore still reached a routed no_agent child. Drop
every non-global source-owned name from the base; the routed scope overlay that
follows puts back exactly the ones that profile's OWN sources supply, since
build_profile_secret_scope folds get_secret_source_values(home) in.
2. refresh_installed_secret_scope() merged the rebuild with dict.update(), so a
name a source had stopped supplying -- rotated, revoked, source removed -- kept
its old value for the rest of the fire. Replace the mapping contents instead: the
rebuild is the profile's current truth.
Regressions: a routed child sees <unset> for a launch-source name while its own
source value comes through, and a refresh whose rebuild omits a name drops it.
Both fail if the corresponding change is reverted.
(cherry picked from commit ecd51517c4828a75acb5f458ed99aea0bc3e5e9f)
The routed no_agent child env started from all of os.environ and only overwrote the
names present in the installed scope. A name defined only by the LAUNCH profile's .env
and absent from the routed profile therefore reached the routed child with the launch
value instead of unset -- the secret scrub only knows classified names, so a custom or
unclassified secret crossed the profile boundary (review finding on the first head).
strip_launch_profile_env (main, 284d220ba4) is the primitive the external-worker path
already uses for exactly this: it drops the launch profile's dotenv-owned keys and the
bridged TERMINAL_* settings, and is a no-op outside multiplex or when the target IS the
launch profile. Apply it to the base BEFORE the scope overlay (so a shared name keeps its
routed value) and BEFORE the sanitizer (so routed values still pass the scrub and
passthrough rules). Pinned by a child-process negative control: the launch-only name
arrives <unset>, the shared name arrives routed, and the parent process is unchanged.
(cherry picked from commit 69349b527b5144c1f1540a4c10035d5ec798c1db)
The desktop backend ticks EVERY local profile's cron store from one process — its own docstring
says "like a multiplex gateway" (hermes_cli/web_server.py) — but never sets the process-global
multiplex flag, and cannot: its own chat turns are unscoped and would fail closed. Every
isolation in the tree keys on that flag — the guard that keeps a routed `.env` out of the shared
`os.environ`, `get_secret`'s fail-closed miss, passthrough resolution, the MCP and kanban
subprocess scrubs — so all of it was inert for a sibling profile's fire. Verified: a secondary
profile's API keys replaced the launch profile's in `os.environ` with `override=True` and stayed
there after the tick, and a scope miss read the launch profile's tokens (#107692).
Give multiplex mode a context-local counterpart. `set_multiplex_context` (agent/secret_scope.py)
is OR'd into `is_multiplex_active()`. `_profile_cron_scope` only MARKS a fire whose home is not
the process's own (`routed_profile_fire`, decided against `get_process_hermes_home()`, the
override-immune resolver); `_install_fire_secret_scope` in cron/scheduler.py installs the
profile's hydrated secret scope and, for a marked fire, the multiplex context — for exactly that
span, dropped again before the scope by `_reset_fire_secret_scope`. Multiplex semantics are
therefore never active in cron without a scope to read: `run_one_job`'s restart-safe handoff runs
before the body's scope and keeps today's semantics (its own scope is #107413 / #106050's seam,
left untouched so this composes with whichever lands). Every existing multiplex-keyed isolation
applies inside the routed fire with no per-site patching; the launch profile's own fires and the
backend's turns keep single-profile semantics; marker and override both reach the pool worker via
`copy_context()`. `get_secret` read the raw global in its miss branch; it now goes through
`is_multiplex_active()`. The dotenv guard keeps its pinned flag-only form (#77970).
Two consequences of suppressing the write are handled rather than left as regressions:
- a `no_agent` script's env is `os.environ.copy()`, which no longer carries the routed `.env`;
the runner overlays the installed scope onto the base BEFORE sanitizing, so the same scrub /
passthrough rules apply to those values and the parent process is never mutated;
- plugin secret sources are discovered on the fire's first agent build, after the scope froze,
and the post-discovery reload is hydrate-only under multiplex semantics; the refresh now folds
the values into the installed scope in place (`refresh_installed_secret_scope`, the pattern
`_publish_env_value` already uses for `.env` writes under multiplex).
And the profile's external secret sources are hydrated before the scope is frozen, the order
gateway/run.py and the external cron worker already use.
Tests pin each direction: the marker without the semantics before the scope, the semantics on and
off exactly with it, the marker reaching a copy_context worker; the process's own profile staying
single-profile; the restart-safe handoff's child env building without raising under a routed tick
with a passthrough key registered; a real child process receiving the routed values while
`os.environ` keeps the launch value; a source registered after the freeze reaching the fire
through the real PluginManager refresh. Reverting any one direction fails a distinct test.
(cherry picked from commit 2f87677425d2cca19286ac83bc45cab23e546669)
`/*.db-wal` / `/*.db-shm` only match at the checkout root, so on a flat
install `git stash push --include-untracked` still swept
`cron/executions.db-wal` / `-shm` while the scheduler held the WAL-mode
database open (cron.executions._connect opens it via open_db in WAL mode).
The base file stayed put but its WAL vanished under a live writer, so the
next `_connect()` failed with `disk I/O error` (review finding on #111175).
Use `/cron/executions.db*` like the gateway recovery db rule already does,
covering -wal/-shm/-journal and retired-WAL dirs. Every other non-root db
rule in the block already uses the glob. The regression tuple gains the two
sidecars, and the stash test gets the reviewer's repro against the real
`_stash_local_changes_if_needed`: an open WAL connection with one committed
row must still be readable from a fresh connection afterwards.
The flat-install block listed state.db and kanban.db sidecars one by one,
so any other root-level SQLite store (response_store.db, a future ledger)
and its -wal/-shm/-journal sidecars would still be swept by the updater's
`git stash push --include-untracked` and unlinked under the running
gateway (#110648). Replace the per-file lines with root-anchored globs
(`/*.db`, `/*.db-wal`, ...). No tracked root-level *.db exists, and
`git ls-files -ci --exclude-standard` is unchanged before/after, so the
globs newly ignore nothing that is committed.
Also add the rest of the flat-install runtime roots the previous fold
missed: the credential siblings from
gateway/platforms/base.py::_ROOT_CREDENTIAL_PATHS (.anthropic_oauth.json,
google_token.json, google_oauth_pending.json, auth/,
webhook_subscriptions.json), the active pairing location platforms/
(gateway/pairing.py), kanban/, gateway_state.json, processes.json,
cron.pid, the channel directory/alias and feishu pairing stores,
pending_messages/, checkpoints/, plugin-data/, hooks/, and the Discord
message-recovery db under gateway/ (gateway/ itself is tracked, so only
that file pattern is ignored). `/.credentials/` had no producer -- the
real dir is `credentials/` (web_routers/files.py, _ROOT_CREDENTIAL_PATHS)
-- so it is replaced. `/state-snapshots/` is dropped: the existing
unanchored `*-snapshots/` rule already matches it.
The test tuple now carries one representative per ignored class and its
comment no longer claims _ROOT_CREDENTIAL_PATHS enumerates the sidecar
set (that is `_sqlite_files`).
The same `git stash push --include-untracked` sweep that took state.db
on a flat install (#110648) also takes every other untracked file at the
$HERMES_HOME root: config.yaml, auth.json/auth.lock, memories/,
profiles/, .credentials/, mcp-tokens/ and pairing/. Losing those on a
declined or failed restore strands the user's credentials and profile
config just as badly as losing the session store.
Extend the root-anchored block with those paths (none are tracked or
already ignored on main) and append them to the test's
FLAT_INSTALL_RUNTIME_STATE list so the existing stash invariant covers
them without a new test.
The per-profile job store lives at HERMES_HOME/cron/jobs.json, so a
flat install keeps it beside executions.db inside the checkout-root
stash domain. Without an ignore rule the untracked autostash of
hermes update sweeps it away with the rest of the runtime state.
Absorb the path (noted in #110670) and its regression assertion into
the runtime-state carrier.
(cherry picked from commit 66282e6dc3ac5f276cdee5b92a22856f146c9a45)
On a flat install (checkout root == $HERMES_HOME) the untracked autostash of
`hermes update` sweeps the live state.db/-wal, snapshots, cron ledger and
lock/pid files into the stash and unlinks them under the running gateway; the
restart recreates an empty store at the same path and the declined restore
leaves the profile with no transcripts (#110648).
Root-anchor the runtime state set in .gitignore, mirroring the
.hermes-bootstrap-complete (#38529) and /.install_method (#66189) precedent
and the $HERMES_HOME-root enumeration in the platforms base module, so the
stash step is never entered for runtime state alone. Regression test runs the
exact stash command against a real repo carrying the tracked .gitignore.
(cherry picked from commit 6c74c24e6131af189801e54f1e11b260a529b74a)
Fold the transient/permanent sync-loop tests into one parametrized table
and add the two classifier branches that had no loop-level coverage: a
401 whose body was rewritten to HTML by a reverse proxy (errcode dropped,
so only http_status can stop the loop) and a 429 M_LIMIT_EXCEEDED that
must be retried because neither errcode nor status is an auth signal.
Deleting the http_status fallback in _is_permanent_matrix_auth_error now
fails the 401-html case.
Hoist _sync_error to module level so parametrize can call it directly
instead of the staticmethod.__func__ workaround. Trim the
test_ws_auth_retry docstring, which still described a Matrix test class
that moved to test_matrix.py.
The inherited fixture coordinate "40.4302" does not contain the substring
"403" (the dot splits it), so the old substring classifier also passed on
it and the case proved nothing. Use a coordinate that genuinely embeds the
digits so the test is red on the pre-fix classifier.
The pinned mautrix 0.21.1 raises MatrixRequestError (carrying errcode and
http_status) from HTTPAPI._send on every non-2xx and sync() returns only
the parsed JSON dict, so the result-object auth branch in _sync_loop was
unreachable; it dated from the nio client whose SyncError objects were
real. Drop it together with the nio-mock test that pinned it.
With structured attributes guaranteed, the leading-status regex and the
bounded keyword scan over the message text were the only remaining ways
for body digits or HTML words to leak into the verdict, so drop them too:
no errcode/http_status auth signal means retry. Trim the contributor's
17 tests to the two loop-level invariants: both production repros (502
HTML body embedding "403" via an SVG coordinate; timeout echoing a since
token embedding "401") keep looping, and a 401/M_UNKNOWN_TOKEN stops.
The comment above the result-object branch in _sync_loop claimed mautrix's
Client.sync() returns an object carrying a message string for auth failures.
That is wrong. In the pinned mautrix 0.21.0, HTTPAPI._send raises
make_request_error() for any non-2xx and otherwise returns parsed JSON, so a
real M_FORBIDDEN arrives as an exception and is handled by the except branch.
The claim was introduced by this PR, which rewrote an accurate comment about
the earlier matrix-nio client (whose SyncError result objects were genuine).
The branch itself is kept as defense in depth against a future client swap,
but it now classifies with the same errcode/http_status logic as the
exception path instead of a lone "unknown_token" substring test, which
silently missed M_MISSING_TOKEN and M_FORBIDDEN and resynced forever
against a credential that can never succeed.
A structured errcode/http_status is authoritative; the message text is only
consulted when the object exposes neither, since str(object) is an opaque
repr. The text scan deliberately cannot override a structured verdict, so a
transient 502 whose HTML body contains "Forbidden" is still retried.
Adds four tests. Three are discriminating RED/GREEN cases that fail against
the old substring branch (M_MISSING_TOKEN errcode, http_status=401 with no
keyword in the message, and an unstructured object whose only signal is
.message). The fourth pins the precedence rule and passes either way.
Verified: 136 passed / 1 failed in tests/gateway/test_matrix.py; the single
failure (test_password_login_uses_device_id) fails identically at the
pristine PR head and is unrelated.
(cherry picked from commit bc9e6a8dafcf349a4e6b20a261fb2449603c0239)
The independent-verifier caught that my first loop-level test did not
actually prove anything. The 502/SVG coordinate fixture I reused from
gmoranxyz's unit-level test does not contain the substring 403 once
case-folded, so the old naive substring classifier already treated it
as transient. A test that passes under both the buggy code and the
fix proves nothing about the fix.
I replaced the fixture with a plain connection timeout whose message
wraps the real Matrix sync pagination token, an arbitrary digit
string that happens to contain 401. I verified this directly: with
the pre-fix classifier restored, the retry test now fails (the old
code stops the loop on this fixture), and with the fix in place it
passes (the loop retries as it should). That is the RED/GREEN proof
the maintainer originally asked for.
I also documented in the stop test's docstring that it does not
discriminate old from new, since the word forbidden in its message
trips the old naive check too. It is still worth keeping as a
regression test proving genuine auth errors stop the loop, just not
as proof of this specific fix.
While I was in there I also fixed a stale comment above the
M_UNKNOWN_TOKEN sync-object pre-check. It said nio returns SyncError
objects, but the dependency here is mautrix, not matrix-nio, and
importing nio raises ModuleNotFoundError in this codebase. The
pre-check logic itself was already correct and untouched.
Co-authored-by: gmoranxyz <gmoranxyz@users.noreply.github.com>
(cherry picked from commit ad3aad579a675a5aae544a50f88a82717c0ac3b6)
I added two more classifier unit tests for the attribute narrowing:
a bare .code attribute that happens to be 401, and a bare .status
attribute that happens to be 403, both must stay classified as
transient since only .http_status is trustworthy. I also added a
parametrized test for the five transient exception types the sync
loop now short-circuits on.
On top of that I added two tests that exercise _sync_loop directly
instead of just the classifier function in isolation. One replays the
real 502 Umbrel repro string through a mocked client.sync and confirms
the loop retries with the 5s backoff. The other raises a genuine
M_FORBIDDEN error and confirms the loop stops on the first call with
no retry sleep. These catch a regression in how the loop wires the
classifier in, not just a regression in the classifier itself.
(cherry picked from commit f747bb4b5a6e6da1bb9136168f08d6e7af5ea64b)
I hit a bug where the Matrix sync loop treated a passing 502 from
Umbrel's app proxy as a permanent auth failure and stopped syncing for
good. The old check did a naive "403" in str(exc) substring match, and
the 502 HTML error body embedded an SVG path with the coordinate
40.4302, which contains the digit sequence 403.
I replaced the substring check with a layered classifier. Transport
exceptions like TimeoutError, ConnectionError, and OSError are always
treated as transient regardless of their message text. Structured
signals take priority next: the errcode attribute against a known set
of permanent Matrix error codes, then the http_status attribute
against 401/403 specifically (not status, status_code, or code, which
belong to unrelated exception shapes and risk coincidental integer
matches). Only when none of those are present does it fall back to a
bounded, word-boundary-safe text scan on the first 200 characters.
Added tests covering the attribute narrowing, the transient exception
types, and two loop-level tests exercising _sync_loop directly to
confirm it retries on a transient error and stops on a genuine 401/403.
(cherry picked from commit 96d3363e45a63e08d9f07518ed949df334a33b3c)
The 400-recovery reload installs a disk pair and only then re-runs issuer
binding on it. When that pair was minted by a different issuer the enforcer
strips its refresh token, and the reload must report "no recovery" so the
session is cleared like any other dead grant. No test pinned that verdict:
a reload that ignored the install result would keep a stripped pair in the
context and return True. The new invariant drives a real 400 through
_handle_refresh_response against a foreign-bound disk pair and asserts the
result is False, the context is cleared, and the foreign refresh token does
not survive on disk.
_hermes_live_ttl read expires_in off current_tokens without checking for
None; getattr(None, ...) happened to yield the default and report "live".
Both current callers install a pair first, but the helper is now explicit
that an empty context is never live, so a future call site cannot adopt
nothing.
Two defects in the peer-adoption path of the refresh fence:
1. `_hermes_install_disk_pair` raised `_RefreshCompletedByPeer` when issuer
binding stripped the candidate's refresh token. That is the right outcome
for `_refresh_token` (restart the flow so the SDK lands in 401 -> full
auth), but `_hermes_reload_tokens_after_refresh_failure` shares the helper
and must instead treat the candidate as rejected: restore the previous
pair and return False so the caller clears state and prompts. The helper
now returns whether a refresh token survived binding and each caller
decides; the one-shot adopt wrapper is inlined into `_refresh_token`.
2. `_hermes_live_ttl` treated `expires_in is None` as expired. RFC 6749 makes
`expires_in` optional, the SDK's `is_token_valid()` is True with no expiry,
and `_rebase_expires_in` preserves None on read, so a peer's rotated pair
without an expiry was never adopted and we POSTed its refresh token
anyway, burning a generation on single-use providers. None now counts as
live; the try/except around a pydantic `int | None` field is dropped.
One new test drives the real auth flow against a peer pair with no
`expires_in` and asserts the pair is adopted with zero POSTs.
The SDK drives a refresh as a generator (request yielded from
_refresh_token, response consumed in _handle_refresh_response), so the
fence was a hand-driven @asynccontextmanager: __aenter__ in one method,
__aexit__ in another, generator object stashed on the provider. A plain
`acquire_refresh_fence(path, timeout) -> fd` / `release_refresh_fence(fd)`
pair says what actually happens and leaves nothing half-entered to leak.
The descriptor is opened with os.open at 0600 and closed on every
acquisition failure.
The `_refresh_token` release-on-exception stays: `_refresh_token` is also
reachable outside `async_auth_flow` (tests call it directly), and the
wrapper's finally only covers the generator-driven path.
HermesTokenStorage.remove() now unlinks the `.refresh.lock` sibling so
logout leaves no stray file. It is deliberately NOT added to
_state_paths(): snapshot()/restore(only_if_absent=True) treat any
existing state path as "newer state exists", and a lingering lock file
would silently veto a rollback.
tests/tools/test_mcp_oauth.py: drop the unused `Path` import (`time` is
still used by the socket poll helper).
The poll loop swallowed every OSError from the lock syscall as contention,
so a filesystem that cannot take advisory locks at all (ENOLCK on some
network mounts, EMFILE, ...) stalled for the full 60 s deadline and then
blamed a peer. Only EWOULDBLOCK/EAGAIN/EACCES/EDEADLK mean "held by
someone else"; anything else now raises RefreshFenceTimeout immediately
with the real errno. Still fails closed -- the refresh is never POSTed
without ownership -- but the failure is diagnosable and instant.
The errno set mirrors cron.scheduler._is_lock_contention_errno; it is
duplicated rather than imported because importing the scheduler pulls in
the whole cron module graph for a four-value tuple.
Both fence paths that pull a peer's pair off disk now go through
_hermes_rotated_candidate (different, non-empty refresh token + non-empty
access token) and _hermes_install_disk_pair, which runs
enforce_refresh_token_issuer on the installed pair. Before, the adopt path
skipped the issuer check entirely, so a pair minted by a different issuer
could be POSTed straight to the new one.
The adopt path installs the candidate even when its access token has
already expired: the POST we are about to build needs the new refresh
token, and skipping the POST (_RefreshCompletedByPeer) is only correct
when the peer's access token is live with a positive TTL, mirroring the
reload path's clamp-to-zero guard. When the issuer enforcer strips the
refresh token there is nothing to refresh with, so the flow restarts into
401 -> full authorization instead of failing with OAuthTokenError.
The reload helper drops its outer except-Exception: get_tokens already
returns None for absent or corrupt files, so the blanket catch only hid
programming errors. Comment updated: this path exists for writers outside
the fence (interactive login, pre-fence Hermes), not for a fenced peer.
`_token_store_lock` serialized a single get_tokens()/set_tokens() call and
then released. Its two justifications no longer hold:
- torn reads: `_write_json` goes through `atomic_json_write` (write to a
sibling, rename), so a reader can never observe a half-written token
file, locked or not;
- the read-modify-write of a single-use refresh token: a lock released
between the read and the POST cannot close that race. `_refresh_fence`
now spans read -> POST -> persist, and every store access on the refresh
path (adopt-from-disk read, post-failure reload, `_store_tokens` write)
runs inside it.
The remaining unfenced accesses are the cold `_initialize` read and the
authorization-code exchange's full overwrite -- neither is a
read-modify-write, so neither needs mutual exclusion. Keeping a second,
fail-open lock layer only adds a 10 s stall on a stale lock file with no
correctness gain. Tests that exercised the removed lock go with it.
The 433-line harness spawned three interpreters and an HTTPServer to show
that one refresh generation is consumed once. The same invariant holds
in-process: flock is per open file description, so two real provider
instances sharing one token store contend for the fence exactly like two
processes do, and the SDK auth flow can be pumped with asend() the way
httpx does. Two tests now bind the fix deterministically:
- two providers, one store, a single-use token endpoint: exactly one POST
carries R1 and both providers end holding the rotated pair (the loser
adopts from disk and never presents the burned grant);
- fence held by another holder past the deadline: the refresh fails
closed, no POST is sent and neither memory nor disk loses the tokens.
The token-store lock is per-operation: get_tokens() and set_tokens() each
take it and release it. With a provider that issues single-use refresh
tokens, two processes can therefore both read R1, both POST it, and the
loser gets invalid_grant on a session that was healthy:
A: get_tokens() -> R1 (lock taken and RELEASED)
B: get_tokens() -> R1 (lock taken and RELEASED)
A: POST R1 -> 200, receives R2
B: POST R1 -> 400, credential already burned
Add _refresh_fence(), held across read -> POST -> persist so exactly one
process consumes a refresh generation. It fails CLOSED: unlike the
token-store lock it raises RefreshFenceTimeout instead of degrading to
unlocked, because proceeding without ownership is the race itself. It
locks a .refresh.lock sibling rather than the token file, since
flock/msvcrt locks are per-descriptor and nesting one path would
self-deadlock on Windows and silently no-op on POSIX.
The provider takes the fence before its final read, re-reads under it so
a peer rotation is adopted instead of overwritten, and releases in
_handle_refresh_response. async_auth_flow also releases on abandonment:
a cancelled generator never reaches the handler, which would strand the
fence and turn the race into a deadlock. That wrapper delegates send and
throw manually -- async generators have no yield from, and `async for`
would feed the SDK response to the inner generator as None.
tests/tools/test_mcp_oauth_refresh_fence.py is an acceptance test, not a
unit test: two real OS processes refresh against a single-use-token
authorization server that audits every redemption. It asserts R1 is
presented exactly once, neither process clears the session, and disk
converges on the newest token. Verified to FAIL without the fence
(audit=[rt-1, rt-1]); a threading-only lock cannot catch this.
(cherry picked from commit c1508a1d47e2cbd94e05fa507684f3716a1e988c)
The desktop app spawns 'serve' while the scheduled task runs 'gateway run',
so two backends routinely share one HERMES_HOME. With a provider that issues
single-use refresh tokens, both could POST the same token and the loser's
refresh was rejected, clearing an otherwise healthy session.
Guard the token file's read-modify-write with a bounded advisory file lock
(fcntl on Unix, msvcrt on Windows, in-process only where neither exists),
mirroring how cron/jobs.py guards jobs.json. Acquisition is non-blocking with
a 10s ceiling: a briefly-contended refresh beats a permanently stuck client.
(cherry picked from commit 16f0d811de446a66ed5fd061fd7594fca3d230da)
`_classify_responses_issuer` reimplemented endpoint canonicalisation with
its own urlsplit/urlunsplit pass. The repo already owns that logic in
`hermes_cli/route_identity.py::normalize_route_base_url` (stdlib-only,
used by agent/backend_identity.py), so delegate to it.
Reasoning items persisted before canonicalisation were stamped with the
raw `other:<agent.base_url>` (trailing slash, host case). Comparing them
verbatim against the now-canonical `current_issuer_kind` marked them
foreign and dropped them on the very endpoint that minted them. Run the
persisted stamp through the same canonicaliser (`_canonical_issuer_kind`,
non-`other:` kinds untouched) before comparing.
Also fixes the `_chat_messages_to_responses_input` docstring, which still
stated the pre-stack rule that legacy endpoint-stamped items drop when the
current model is known; the stack replays them on a matching issuer.
The transport wiring (build_kwargs model -> _last_issuer_model -> normalize_response
stamp) and the new error_classifier string had no test that fails when either is
removed; these two do.
The openai SDK appends a trailing slash to `client.base_url`, so the aux
adapter stamped `other:https://h/v1/` while the main transport stamped
`other:https://h/v1`. On custom Responses endpoints every aux call
(compression, flush_memories) therefore dropped all main-minted reasoning
items as "foreign".
`_classify_responses_issuer` now strips whitespace and trailing slashes and
lowercases scheme+netloc before stamping. The aux adapter also derives its
route flags from `classify_responses_route` — the single owner of the
codex/xai/github predicates — instead of an inline chatgpt.com host check,
and reuses the same flags for the effort clamp.
Native compaction checkpoints and reasoning items persisted before model
stamping existed carry only `_issuer_kind`. Treating a missing `_issuer_model`
as foreign dropped every such item once the current model was known, which
wiped existing sessions' native-compaction context on upgrade (four consumer
tests in test_native_compaction / test_native_preflight_estimate /
test_413_compression went red on the stack).
Trust the endpoint stamp when no model stamp is present, as main does today.
Items minted after this change carry the model stamp and still drop on a
same-endpoint model switch; a wrong guess on a legacy item is caught by the
invalid_encrypted_content 400 classifier and the replay kill switch.
Encrypted reasoning blobs are sealed to the model that minted them, not
only to the endpoint. Switching models on the same custom Responses
endpoint therefore replayed blobs the new model cannot decrypt and the
turn failed with HTTP 400.
Stamp captured reasoning items with `_issuer_model` (the canonical wire
model) alongside `_issuer_kind`, and replay an item only when both the
issuer kind and the model match the current request. Endpoint-stamped
legacy items without model provenance are dropped once the current
model is known (fail closed); ordinary assistant text stays replayable.
The transport threads the effective wire model (request_overrides win)
into conversion and normalization; the auxiliary Codex adapter stamps
and filters against its own model rather than the main agent's. The
400 classifier also recognises the custom-endpoint wording
"encrypted content could not be decrypted or parsed" so recovery strips
the replay state instead of aborting.
Hand-grafted from #95849 (final head d9cf6bcc08) onto current main; the
middleware-model-rewrite half is intentionally left out.
Closes#95834
The zero-knob test only asserted that ack_stale still trips with the knob
at 0. Because _read_websocket_health evaluates ack age before the
event-silence dimension, the test stayed green even with the
`_event_max_silence_seconds > 0` guard deleted: it never observed the
stale stamp being ignored. Assert (True, "healthy") with knob=0, a stale
stamp and a green transport first, then make the ACK stale and keep the
ack_stale assertion. Dropping the guard now fails this test.
Also correct the on_socket_event_type comment: discord.py dispatches
socket_event_type before the op-code switch but only for a non-null `t`;
heartbeat ACK frames carry `t: null`, they do not "return before" it.
The event-silence tests duplicated `_make_adapter`/`_connect` from
test_discord_liveness.py and carried a bespoke handler-wait loop. Reuse
the sibling helpers instead: `_make_adapter` grows an optional
`max_event_silence` that is only written into `extra` when given, so the
sibling's own tests keep the adapter default and stay unchanged.
Two invariants now bind the fix:
- deaf socket: a `None` stamp (no DISPATCH yet) reads healthy across
several probe intervals, and once armed, transport-green + event
silence trips `event_silence` through `_liveness_loop`.
- `websocket_event_max_silence_seconds: 0` disables only that
dimension: with a stale stamp and a stale heartbeat ACK the probe must
still run and trip on `ack_stale`. This goes red if the knob is moved
into `_start_liveness_probe`'s all-or-nothing guard (#109782).
The YAML->extra passthrough test also asserts the new key, so dropping
its `_YAML_WEBSOCKET_LIVENESS_KEYS` entry fails.
Trim the new event-silence file from 8 tests to the 2 that bind the fix:
the deaf-socket e2e through `_liveness_loop` (transport green, no
DISPATCH → probe trips with the `event_silence` reason) and the
None-stamp window reading healthy (no false trip on quiet reconnects).
The removed cases re-checked knob parsing, defaults and the YAML seed
loop already covered by the sibling liveness knobs' tests, or restated
the two kept invariants from a different angle.
Incident 2 of #109521: a Gateway socket can stay ESTABLISHED and keep
ACKing heartbeats while zero DISPATCH events are parsed, so every
transport-side liveness sample (ready/open/ack-age/latency) reads
healthy for hours. The merged #109963 deliberately dropped the
event_silence dimension: a raw-frame stamp is debug-gated
(on_socket_raw_receive needs enable_debug_events) and, since heartbeat
ACKs are frames, ack_stale always fires first by construction.
This adds the dispatch-side signal that was requested instead:
- stamp on on_socket_event_type, which discord.py 2.7.1 dispatches for
every parsed DISPATCH frame with no debug gate (verified live against
the real received_message path: 4/4 frames fired with
enable_debug_events=False, on_socket_raw_receive 0/4)
- new knob websocket_event_max_silence_seconds (default 4h, the
incident report's field-proven operator bound); 0 opts out of this
dimension ONLY — the #109782 review failure put the knob in
_start_liveness_probe's all-or-nothing guard, killing the whole
watchdog; it is gated strictly inside _read_websocket_health here
- the stamp resets per connection (connect() clears it), and a None
stamp (no event parsed yet on this connection) is not silence
- docs (en + zh-Hans) cover the new knob and the per-dimension opt-out
Fixes#109521
(cherry picked from commit b4baa97fc45794209711a45e052111d7d44d5f90)
The timeout branch of _run_hook_callback_bounded unconditionally added
gate_key to _hook_abandoned. A worker that finishes between done.wait()
returning False and the caller taking the lock has already popped its
token via _release_token, so nothing would ever clear that entry: the
callback stayed blocked for every later call id until reload with no
thread behind it. Guard the insert on the worker still being registered.
The new test makes the race deterministic by swapping the module's
threading.Event for one whose wait() lets the worker finish and then
reports a timeout, and asserts a fresh call id still runs.
Also pass tool_call_id inline from terminal_tool_result instead of the
conditional dict plumbing: an empty id is already treated as "no
identity" by _hook_call_identity and unknown fields are withheld from
narrow-signature callbacks (same shape as _fire_approval_hook). Update
the stale "(hook_name, id(cb))" comment above _hook_running_callbacks.
The same-call negative control fired two sequential calls with a 0.1 s
timeout, so the first call timed out and the second was dropped by the
60 s suppression window; the identity gate itself was never exercised.
Mirror the positive test instead: a 5 s timeout, two threads with the
same tool_call_id while the callback is held on an Event.
Add one test for the abandoned-worker gate: a hung callback followed by
a call with a fresh tool_call_id, with suppression zeroed, must start
exactly one worker. Reverting the gate change makes it fail.
Concurrent invocations of the same tool in one session collapsed into a single
busy key (hook_name, id(cb)): the second invocation was reported as 'still
running' and dropped. For pre_tool_call a drop is a fail-closed block, so the
gate silenced itself on an ordinary, healthy callback.
Measured on a busy profile: 3574 skip lines and 0 timeout lines in one hour —
every skip was the 'while still running' branch, i.e. pure key collision, not
slowness.
The gate now keys on the call identity that is already in the payload
(tool_call_id, else turn_id, else none — the last case behaves exactly as
before). Suppression stays keyed coarsely on (hook_name, id(cb)): a hung
callback is a fact about the callback, so its back-off must not be diluted
per call.
Refs #98382. Independent of #107894 (that one releases the slot on timeout;
this one stops healthy concurrency from colliding).
(cherry picked from commit 53b3dacd008418fcdf5fa6dfcadde575a35a776e)
`_fd_is_truly_unlinked` stats `/proc/<pid>/fd/<n>` after the scan has
already read the readlink target. Between those two steps the descriptor
can be closed (ENOENT, handled by the previous commit) or the whole
process can exit (ESRCH). Both mean the descriptor can no longer keep a
retired WAL/SHM generation alive, so neither is evidence of a live holder
and neither should make the guard refuse to open the database.
Match the sibling scan in hermes_state_holders, which already skips both
errnos, by branching on `exc.errno in (ENOENT, ESRCH)`; any other OSError
still fails closed. The existing closed-descriptor test is parametrized
over both errnos, injecting the failure at `os.stat` so the ESRCH path is
exercised on every platform.
`\bsocat\b` under IGNORECASE matched "SOCAT", the Surface Ocean CO2 Atlas,
in every oceanography skill of a 2,110-file research bundle (17 critical
findings in one file), burying the bundle's real issues under noise. A real
socat relay always names an address type (TCP:/UDP:/OPENSSL:/EXEC:/SYSTEM:/
PTY:/UNIX-…:), so the pattern now requires one on the same line. `nc -l` /
`ncat -l` are unchanged. Scanner version bumped to v5 so cached verdicts
re-scan.
Catalog entries sort official → stars desc → name, both in browse shelves and
filtered grids, with a ★ pill on each card linking to the repo's stargazers.
Rate-limit discipline is the design constraint: the docs site deploys many
times a day and shares one GitHub App API budget with every other workflow
(tonight's merge train got rate-limited on unrelated uploads). So
website/scripts/fetch-plugin-stars.py first fetches the live site's own
plugin-stars.json (a CDN GET, not the API); if that cache is under 24h old it
is reused verbatim and GitHub is never called. Only a stale cache triggers one
GET /repos/{owner}/{repo} per unique catalog repo, and a 403/429 mid-run keeps
the previous counts instead of zeroing them. extract-plugins.py merges the
cache into plugins.json (`stars`) and plugins-meta.json (`starsFetchedAt`), and
the page footnote says when the ranking was last refreshed.
Teknium's call: most community submissions are Desktop panes, so an entry
without a category lands on the Desktop shelf; "other" becomes "general" for
plugins that genuinely span areas. Shelf order puts Desktop first. The six
entries merged today (pets-all, newswire, auto-titler, live-voice,
metamask-wallet, web-octen) get explicit categories.
The catalog page was one undifferentiated grid filtered only by tier, so a
memory provider sat between two Desktop panes. Entries now carry an optional
``category`` (memory | desktop | platform | web | tools | voice | automation |
models | other, default other) that the loader, the admission validator and
the site extractor all understand.
/docs/plugins renders one shelf per category in browse mode, a category pill
row under the tier pills, a clickable category chip on every card, and a
results bar (active category, count, clear) when a filter or search flattens
the view. ``hermes plugins catalog`` gains a Category column and groups by it.
All 18 shipped entries are categorised. Unknown categories fail admission
(same contract as tier) so a typo cannot create a phantom shelf.
`ProcessRegistry._move_to_finished` pops the session out of `_running`, then
saves the receipt, releases handles and writes the checkpoint, and only THEN
enqueues the completion and sets `_completion_event`. A quiet one-shot parent
whose turn ends inside that window called `wait_for_pending_completions`,
found nothing in `_running`, drained an empty queue and exited without the
follow-up turn. That is the CI flake in
tests/tools/test_completed_process_results.py::test_headless_terminal_result_survives_cli_exit
(`follow_ups == []`), which also hit unrelated branches.
Consider `_finished` sessions whose event is not yet set as pending too.
Repro: a 6s sleep before the enqueue plus a 2s delay before the parent's first
wait fails the E2E 2/2 on main and passes 2/2 with this change.
