The PR added four test functions for one fix. The host-side short-write and
multibyte cases are the same invariant as the sandbox size probe (an archive
that is not byte-exact is never referenced to the model), so they become two
parametrized rows of test_size_probe_decides_lossless, which now also uses
multibyte content so every row pins the byte-vs-char comparison. Net new
tests for the fix: 2 functions.
Also names in _write_to_sandbox why the +1 tolerance is keyed on heredoc
mode only: the payload backend delivers stdin verbatim and is expected to be
byte-exact. The three bare write_text() calls the footgun scanner flags in
this file gain encoding= while it is being touched.
Five near-identical MagicMock scripts pinned the same contract (byte-exact
or discard, heredoc gets exactly one extra byte); one parametrized test
plus the unprobeable-backend case cover it. Drop the host-side happy-path
test that duplicated test_multibyte_content_verified_by_byte_count.
Docstring now names the real heredoc wrapper
(BaseEnvironment._embed_stdin_heredoc) and notes the extra exec RTT per
oversized result (review point).
Oversized tool results are archived to disk and replaced in-context with a
'Full output saved to: <path>' reference. Until now the write was trusted
blind: a partially-flushed host file (ENOSPC/quota races) or a lossy sandbox
write (API-body truncation on payload backends) still produced the archive
reference, so the model was told the full result was recoverable when bytes
had silently vanished.
Both persistence paths now round-trip-verify size before building the
reference and fail closed to the bounded inline truncation otherwise:
- _write_to_spillover: byte-count check via os.stat after write; mismatched
archives are deleted and the caller falls through to inline truncation.
- _write_to_sandbox: wc -c probe after the cat; heredoc-mode backends get a
+1 byte tolerance (wrap_modal_stdin_heredoc appends one newline by
construction), unprobeable backends stay best-effort success.
Regression tests fail without the fix (verified by stashing the source
change: 4 failed). E2E-verified against a temp HERMES_HOME with real file
I/O including multibyte content and a simulated short write.
The dashboard endpoint GET /api/skills/hub/search passes its user-supplied
`source` straight into parallel_search_sources and never applied the merged
provider cut, so ?source=nvidia returned a mixed set. That was the fourth
caller of the walker; the cut was copy-pasted at three of them and missing
at the fourth.
parallel_search_sources already computes the normalized provider filter, so
the cut now lives there — applied per source before results are counted and
merged. Every caller (CLI search via unified_search, CLI browse, TUI-gateway
browse, dashboard router) sees the same rule with no provider logic of its
own, source_counts stop reporting rows that are then dropped, and the three
duplicated call-site cuts are deleted. do_browse keeps its provider-specific
"No skills found for provider" message.
Also:
- HermesIndexSource.search now treats a whitespace-only provider_filter as
"no filter", matching GitHubSource.search (the two adapters previously
disagreed on the same keyword argument; unreachable through the walker,
which pre-normalizes).
- The regression-test fixture seeds tap caches by github_provider_for label
instead of case-sensitive repo literals, and serializes metas through
_skill_meta_to_dict, so a DEFAULT_TAPS casing change can no longer silently
unseed the fixture.
Validation: 121 targeted tests green; disabling the walker cut fails the
pre-existing test_unified_search_provider_filter_keeps_index_source with the
expected clawhub leak; 4/4 regression cases still red on unpatched main.
Follow-up polish on the provider-filter-before-limit fix:
- GitHubSource.search now skips taps whose repo maps to a different provider
instead of enumerating every tap and filtering afterwards. A tap's repo fixes
the provider of every result it yields (github_provider_for is the only source
of extra.provider in this adapter), so the skip is lossless and avoids up to 23
useless tap enumerations per provider-filtered search — real GitHub API calls
against the 60/hr unauthenticated budget and the 30s overall timeout when the
index is unavailable. The now-redundant post-loop filter is dropped.
- _provider_filter_of() is the single owner of "does --source name a provider";
it replaces the four inline copies of the strip/lower/membership idiom in
_select_active_sources, parallel_search_sources, unified_search and do_browse.
- _tap_cache_key() is shared by _list_skills_in_repo and the regression test so
the seeded tap cache can never drift from the production key format.
- _entry_provider() dedupes the raw-index provider extraction used by both the
pre-ranking filter and the scoring loop in HermesIndexSource.search.
- browse_skills (the TUI-gateway browse path) now applies the same merged
provider cut as do_browse; it accepted a provider value but returned
unfiltered results.
Validation: 121 targeted tests green; the regression tests go red on both
adapters when either the tap skip or the index pre-filter is neutralized, and
4/4 red on unpatched main; live CLI repro returns 0 results on main and 3/3
provider matches on this stack.
The gateway loopback handler re-inlined tools.mcp_oauth._parse_redirect_query;
that copy is exactly how the gateway relay lost `iss` while the CLI path
kept it. Use the helper so the four callback keys have one owner, and
point the docstring at it instead of repeating the RFC 9207 rationale.
mcp 2.x rejects an authorization response that omits the RFC 9207 `iss`
parameter when the authorization server advertised
`authorization_response_iss_parameter_supported`. Cloudflare advertises it
AND sends it; the CLI loopback handler has always forwarded it, but every
other callback producer parsed only code/state/error, so the SDK raised:
OAuthFlowError: Authorization response missing iss parameter
advertised by the authorization server
and the server parked. Same machine, same config, `hermes mcp login <name>`
from a terminal succeeded — the failure is specific to the non-CLI relays.
Forward `iss` on every producer, matching `_make_callback_handler()`:
- tools/mcp_dashboard_oauth.py: `deliver_callback()` accepts `iss`;
`wait_for_callback()` returns `(code, state, iss)`. The bridge in
tools/mcp_oauth.py already splats that tuple into
`_authorization_code_result(code, state, iss)`, so it needs no change.
- tui_gateway/mcp_oauth_sessions.py: the gateway-hosted loopback listener
parses `iss`, and `deliver_callback_flow()` forwards it.
- tui_gateway/methods_tools.py: the `oauth.callback` RPC passes `iss`.
- hermes_cli/web_routers/mcp.py: the dashboard callback route accepts it.
- apps/desktop/electron/mcp-oauth-callback-ipc.ts: the one-shot listener
reads `iss` off the redirect (the renderer already spreads the whole
callback object into the RPC, so it flows through unchanged).
Providers that omit `iss` round-trip as `None`/`null` rather than being
dropped, so servers that do not advertise RFC 9207 keep working.
Verified live on Windows against mcp.cloudflare.com, whose metadata sets
`authorization_response_iss_parameter_supported: true`: the server that
previously parked on the missing-iss error now reports
`Authenticated — 3452 tool(s) available` and `hermes mcp test cloudflare`
connects. State-mismatch and replay rejection are unchanged.
Tests (each fails on base, passes with the fix):
- test_dashboard_flow_preserves_rfc9207_iss
- test_deliver_callback_forwards_iss (client-redirect relay)
- test_loopback_listener_forwards_iss (real HTTP redirect)
- two vitest cases on the Electron listener, incl. the iss-absent case
Refs #92758, #99984. PR #92765 fixes the dashboard route and the loopback
listener but not the client-redirect relay
(`deliver_callback_flow` / `oauth.callback` / the Electron listener), which
is the path Desktop drives against a remote backend.
Widening secret_source_names() to include skipped_existing names silently changed
tools/mcp_tool_config.py::_build_safe_env, an untouched consumer that forwards every
returned name into MCP stdio child envs. That consumer wants only names a source
actually APPLIED (pre-stack semantics), so secret_source_names() goes back to
tuple(_SECRET_SOURCES).
The routed-child scrub in strip_launch_profile_env is the one site that must also see
names a source supplied but lost to a pre-existing process value, so it reads the new
source_supplied_names() accessor instead. tools/mcp_tool_config.py is byte-identical to
origin/main.
_run_job_script popped the launch profile's secret-source names in an
inline loop right above strip_launch_profile_env, so only the no_agent
child got that protection; the four other callers of the same helper
(the external cron worker, scheduler_delivery, kanban dispatch and the
byterover plugin) still handed a served profile the launch vault or
1Password names. Fold the names into the helper's residue set, which is
already gated on multiplex and on the target not being the launch
profile, and keeps administrator-managed keys.
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 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.
flock is bound to an inode, not a path. Unlinking `<srv>.json.refresh.lock`
from `remove()` while a peer still holds the fence lets the next acquirer
open and lock a brand-new inode, so two processes hold "the" fence at once
and the single-use refresh token can be consumed twice. On Windows the unlink
of a locked file raises PermissionError straight out of `remove()`/`restore()`.
A 0-byte 0600 sidecar in a 0700 directory is harmless, so leave it in place.
It stays out of `_state_paths()` so `restore(only_if_absent=True)` still keys
off real token state only.
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 fence is entered from the SDK's coroutine-driven auth flow, so its
acquire loop spun on time.sleep(0.05) and blocked the whole event loop
for up to 60 s while a peer finished its network round trip. The fence
is now an async context manager that polls a non-blocking flock with
asyncio.sleep, and it creates the token directory itself (the parent may
not exist yet on a first refresh; secure_parent_dir only chmods).
The in-process RLock layer is dropped: an advisory lock on a fresh
descriptor already excludes sibling tasks and threads of the same
process, and a thread RLock is reentrant across asyncio tasks on one
thread, so it excluded nothing there anyway.
After acquiring the fence the provider re-reads the store; when a peer
already rotated the pair and the adopted access token is valid, it
raises _RefreshCompletedByPeer instead of building the refresh request.
The auth-flow wrapper restarts the SDK flow so the original request goes
out with the winner's access token. Previously the loser adopted the
new pair but still presented its stale refresh token, burning a
generation on every single-use provider.
Design lifted from #71715.
Co-authored-by: Kevin Yin <182213728+yinkev@users.noreply.github.com>
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)
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.
Hook callbacks are gated per (hook, callback, call identity). Two hooks
on the tool-loop path fired without any identity, so concurrent terminal
calls in one turn, or overlapping turns, still collapsed onto a single
gate key and the second invocation was skipped as if a callback had hung.
transform_terminal_output now forwards the tool_call_id bound in the
approval context around dispatch (only when set); transform_llm_output
forwards the turn_id already in scope. Payloads are additive: the
dispatcher withholds unknown fields from narrow-signature callbacks.
`\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.
`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.
On Windows the Hermes venv interpreter cannot CreateProcess a binary under
C:\Program Files\WindowsApps (WinError 5) even though the shell resolves it,
so `hermes computer-use doctor` died with a raw PermissionError traceback
from _open_mcp. Catch the spawn OSError and print what failed, why the tool
may still work (PATH resolves another copy), and the fix (reinstall outside
WindowsApps or HERMES_CUA_DRIVER_CMD), exit 2.
cua-driver 0.21 refuses a bare element_index:
click: bare element_index is not accepted; pass element_token,
or snapshot_id together with element_index
_maybe_attach_element_token gated solely on the trycua/cua#1961 capability
vocabulary. 0.21 stopped publishing per-tool capability sets — every tool
reports an empty set — while still accepting element_token in its input
schema. The gate therefore fails closed on 0.21.x and we send the bare
index, so the driver rejects the call.
The effect is total: every element-targeted click is refused, leaving
agents with only blind pixel coordinates. Observed against cua-driver
0.21.0 on X11, where a capture returned 762 elements with 762 tokens
cached and every subsequent click still failed with snapshot_id_required.
Check the live input schema first — supports_input_property() already
exists for exactly this, and its docstring notes it "deliberately inspects
tools/list rather than ... requiring a capability token the driver never
shipped". The capability check is retained as a fallback so drivers that
did ship the vocabulary are unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep never-started output behind unsupported owners and drain in admission
order after release. Persist claims before execution and never replay uncertain
started turns. Existing supported-owner receipts keep their authority.
Credits 686f6c61's residual queue proposal in #100319. This is a scoped
implementation, not general retry of failed CLI subprocesses.
Native Electron before/after: CLI-owned target previously returned
SESSION_NOT_OWNED and remained empty after release/tick; now its queued
output and reply appear once in the target Bot Chat. Nested quiet CLI
message_agent delivery to a named Desktop owner also passes on base.
Bot-to-bot message_agent delivery builds both transport argvs (local
teammate chat and peer dm) with a bare "hermes" as argv[0]. Since #96631
the delivery runner spawns under terminal_tool's isolated host-local
environment, which does not inherit the gateway's PATH — so on
docker/service installs (venv at /opt/hermes/.venv) every delivery exits
with FileNotFoundError: 'hermes'.
Resolve the CLI with bot_relay._hermes_cli() (#93590) — the venv sibling
of this interpreter, then shutil.which, then the bare name — at both
argv construction sites. The turn-lock matcher in _delivery_lock()
already matches argv[0] by basename, so absolute paths lock exactly as
before.
Fixes#100662
Masking every balanced [x](dest) in a .md file also blanked
`cp [k](../../../.ssh/id_rsa) /tmp` inside a ```sh fence, which main scored
caution and the branch let through as safe. A link inside a code fence is a
command argument, not a hyperlink: toggle masking off between fence markers.
Review of the write-verb gate found sed -i, chmod, truncate, curl -o, wget -O,
git clone and a scripted open(...) against ~/.ssh all slipping to no finding,
where the bare path regex on main caught them. Add those verbs and the open(
shape to the gate; read-only mentions stay clear.
Salvage trim of #89249:
- keep pattern id `ssh_access` (test_memory_tool and callers key on it; renaming buys nothing)
- word-bound the verb alternation: the unanchored form matched `add` inside "address" and
`dd` inside "middle", so read-only prose still fired; `\b` closes that leak
- fold the second "bare leading redirect" regex into the same alternation (`>>?` branch)
- tests: one parametrized "write shapes still fire" (echo, cat, cp, tee, mv, install,
printf, dd, scp, rsync, ln, leading redirect, option clusters) and one "read-only
mention does not fire"; the trade-off/change-detector tests are dropped
- test_memory_tool: the persistence fixture used a read-only mention; use a write shape
Extend the write-verb alternation with mv/install/printf/dd/scp/rsync/ln,
allow short option clusters between verb and path, and add a bare-leading-
redirect branch (a > ~/.ssh/... line carries no verb word at all). Prose
that names a write primitive before an SSH path still fails closed — a
false positive costs a review, a false negative is a backdoor.
(cherry picked from commit 4865b96ca3c8661c0ea6f5c479c198ccf68f86c4)
The bare \$HOME/.ssh|~/.ssh regex fired on ANY mention of SSH paths in
scanned content, so operational documentation (VPS recovery notes, SSH
configuration write-ups stored in memory or skills) was blocked as a
persistence threat. Require an echo/cat/cp/tee/append/add/write/>>
verb before the path so only backdoor-insertion shapes match.
(cherry picked from commit 8c9b4c8972ac849e4c82fa20d355733ca3e5a231)
`sudo.request` and `sudo.respond` are gateway wire events: the masked
sudo-password prompt the terminal tool raises, which every client surface
(desktop, TUI, any plugin that relays secure prompts to another device)
has to name to forward it. The `sudo_usage` rule matched the bare word,
so any plugin listing those events scored `high` and every install of it
landed on `caution`, which Hermes Desktop cannot confirm past.
A dotted event name is never a shell `sudo` invocation. Exclude exactly
those two names with a negative lookahead; a real `sudo cmd` still fires.
(cherry picked from commit a7a3a31126de8057c2dbb9b7048724aa9d55a7fd)
The shell-startup-file pattern is
`\.(bashrc|zshrc|profile|bash_profile|bash_login|zprofile|zlogin)\b`.
Six of those names are distinctive enough that seeing them after a dot
means the file. `profile` is not: it is also how every language spells
attribute access, so `self.profile`, `user.profile`, and
`request.profile` each score a medium persistence finding.
The cost is signal, not blocking -- `_determine_verdict` treats
medium/low alone as informational -- but a plugin that happens to name a
field `profile` buries the findings a reviewer has to read. A model-
provider plugin whose tests exercise a `profile` object contributed 36
of 38 findings in its scan report, all of them this pattern.
Split `profile` into its own entry anchored on a non-identifier
character before the dot. Real references keep matching in the forms they
actually take (`~/.profile`, `"$HOME/.profile"`, `./.profile`,
bare `.profile`); attribute reads no longer do. The other six names are
untouched. Both entries keep the `shell_rc_mod` id, and scan_file
deduplicates on (pattern_id, line), so a line holding both still yields
one finding.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 705d0bddc61a1c55be82956d343b229544350eae)
The file-safety salvage carried five tests and the execute_code salvage four;
fold them into the two behaviour contracts per fix: (a) the named-profile scope
exempts the root's direct files and the write lands with no prompt; a child env
sees the ACTIVE profile's home per turn; (b) negatives hold: a checkout's
.hermes/config.yaml stays gated fail-closed, a lookalike profiles/ tree exempts
nothing; a dedicated process with no override is untouched. Both red on base.
Also compress the _hermes_exempt_homes docstring (WHY only; cite #110630).
Under `gateway.multiplex_profiles: true` every gateway start logged a WARNING +
traceback from `tools.registry` for `_check_vault_available`. The browser vault
gate is a `no_cache_check_fn`, so `_check_fn_cached` short-circuited into
`_run_check_fn_uncached(fn)` without consulting the cache scope and the default
`unresolved_scope=False` hint reported an EXPECTED boot-time fail-closed read as
"scope was resolved" — the #100697 fix only reached the cached branch.
Derive the verdict from `current_secret_scope()` at the catch site instead of a
branch-derived hint: no scope installed → DEBUG without traceback (expected,
re-probes on the first scoped turn); scope installed but the read still failed
closed → WARNING + traceback (the probe dropped the scope on a bare thread/hop,
a spawn-site bug that must stay loud). Every uncached probe that reads a
credential is covered, vault included.
Fixes#110635. Credit @KoNit-K (#110638) for the report-side diagnosis; that PR
disabled the legacy-cloud probe for the vault gate only, which would have hidden
the vault from legacy Browser Use cloud users and left the class open for every
other uncached probe.
Under a multiplexed Desktop/Dashboard connection, one server process serves
multiple profiles, binding a context-local HERMES_HOME override per turn.
_build_child_env scrubbed the server process's os.environ - which carries the
machine-default HERMES_HOME - so skill scripts run via execute_code silently
read/wrote the wrong profile's directory.
Rewrite HERMES_HOME in the child env from the active override when one is
bound (the same per-turn rewrite apply_subprocess_home_env already does for
HOME). With no override (dedicated per-profile process) the inherited value
is left untouched.
Fixes#110303.
Under a named profile (``hermes -p <name>``, i.e. ``HERMES_HOME=<root>/profiles/<name>``)
the protected-instruction gate exempted only the profile dir, so the ROOT's DIRECT
files (LEDGER.md / MEMORY.md / USER.md / SOUL.md / AGENTS.md / DECISIONS.md ...) fell
through to the ``.hermes`` component rule and were treated as project-local
``<repo>/.hermes/config.yaml``. That gate always-asks and fails closed without a human
channel, so every write there was refused headless — it blocked #54 (LEDGER.md edit
could not land). Subdirectories (scripts/, cron/, data/) were unaffected, which is why
#55 sailed through and #54 did not.
``_hermes_exempt_homes()`` now returns the active profile home plus the Hermes ROOT
when that home really is a named profile (``named_profile_home`` validates: ``.hermes``
name, home markers, tombstone, or the resolved default root), so a coincidental
``profiles/`` directory never exempts its parent. ``_get_real_hermes_home()`` keeps its
per-profile semantics — the #107327 multiplex regression test pins that — and the
exemption consumes the new helper instead.
Refs: https://gitlab.yx.netease.com/agent-projects/platforms/agent-mentor/-/issues/60
Fold the cherry-picked mechanism into the main file's compact style: one
denylist-owner regex replaces the assignment-target + name regex pair and the
_in_denylist_construct helper; the comment-prefix table loses its dead .css
entry; the denylist demotion lands on high (a confirmable caution) instead of
medium so the finding still gates the install and shell_rc_mod, already
medium, leaves the set. The verb guard widens to stem-prefix matching and
copyfile/copy2/sendfile per the review thread on #92632, closing the
Path.read_text()/shutil.copyfile shape. Tests trimmed to two invariants: a
skill's own denylist is caution (force-overridable), a real write to
authorized_keys stays dangerous.
`_scan_file` matches every threat pattern against every line with no notion
of what the line is. The path-token patterns — `authorized_keys`, `~/.aws`,
`~/.ssh` — therefore cannot tell `cat ~/.ssh/authorized_keys` from a skill
that spells the path in order to REFUSE to read it. One `critical` becomes
`dangerous` in `_determine_verdict`, and on a community source that blocks the
install with no `--force`, so the skill that bothered to skip credential
files is the one that gets quarantined (#92478).
Demote, do not drop, following the precedent `allowed_tools_field` already
sets in this file: the finding keeps its file, line and matched text so an
auditor still sees the token; it just stops deciding the verdict alone.
Two contexts qualify, and only for the eight path-reference pattern ids:
- a whole-line comment, in a language that HAS comments, drops to `low`.
Markdown is deliberately excluded: `#` opens a heading there, and Markdown
prose is the prompt-injection surface itself.
- a line inside a construct NAMED as a denylist (`SKIP_PATTERNS`, `DENY_*`,
`EXCLUDE_*`), carrying no verb that could touch the path, drops to `medium`.
The issue also suggested demoting any quoted token on a verb-free line. That
is wider than it looks — a fragment in quotes can be interpolated into a
command a line later — so the name test is the primary rule and the verb test
only guards it, because the construct's name is attacker-chosen.
The denylist check is statement-aware rather than per-line: the reported
match sat on a continuation line of a multi-line regex whose name is four
lines up, which a per-line test reads as anonymous.
8 tests. Reverting the demotion fails the two behaviour tests and leaves the
six contract tests green; dropping the verb guard, the Markdown exclusion, or
the statement-awareness each fails exactly its own test.
(cherry picked from commit f50acd34f62375926bd5843125e106f7e7eb6ee8)
Five scanner rule changes land together (prose/comment demotion, own-denylist
demotion, shell_rc/sudo token fixes, Markdown link masking, ssh write-verb
gate). Cached verdicts keyed on the old versions would keep previously
blocked skills and plugins blocked; one bump re-scans them.
Fold the three PRs' overlapping tests into two invariants per behaviour:
- comment/changelog prose demoted to caution and confirmable; trailing comments,
runtime code and agent-facing docs still dangerous (#111193)
- Markdown plan/design prose demoted to caution; the same content in runtime
code still dangerous (#103364)
- skills_guard: context_exfil needs a transfer directive; rm -rf under temp
roots is not destructive_root_rm
The #111199 regression test is kept (behaviour is the same); its mechanism
(re-read the file per finding, cap every .md) was not carried since the
match-based cap already covers it without touching agent-facing docs.
'//' is not a CSS comment marker, so .css is dropped from the prefix table.
A whole-line code comment or a changelog entry describing the threat a defense
rejects ("# a symlink could point at /etc/passwd, so ...") scored full severity,
driving the verdict to dangerous and making security-hardened community plugins
un-installable: --force does not override dangerous, so the only way through was
deleting the documentation of what was defended against. Whole-line comments and
CHANGELOG entries now cap one severity step lower (critical->high), keeping the
finding visible and the verdict at caution: blocked by default, --force
overridable. Trailing comments (executable code on the line), runtime code and
agent-facing docs keep full severity.
Fixes#111193
(cherry picked from commit 2a79f0a67e70eddcd387e759b1570d4feaee1e97)
A signed-in, paid Nous account that the portal had not enabled for
connectors got `manage_connections` in its schema and a raw "tool gateway
request failed with status 404" back from every call. The gateway answers
404 for any such account by design, and Hermes gated the tool on paid access
or a free tool pool, which says nothing about that.
The gate now reads the portal's own answer: a `managed_tools` token claim,
plus the existing free-tier leg. The gate is also the tool's check_fn, so a
session without the claim never sees the tool and the model has no 404 to
narrate. A token without the claim reads as not enabled.
