#63113 rewrote native drive paths in ShellFileOperations, but init_session
/_wrap_command still embedded C:/... hermes-snap paths from get_temp_dir.
MSYS arg-converts those during bash -l and surfaces Directory \drivers\etc
— including for relative write_file targets, since the wrapper is the fault.
Add _bash_safe_path, override BaseEnvironment._quote_shell_path on
LocalEnvironment (no base→local import), and normalize mixed /c/Users\...
paths in file ops.
Co-authored-by: xxxigm <tuancanhnguyen706@gmail.com>
ShellFileOperations builds bash commands (wc/head/sed/cat/tee ...) with the
target path as an argument. On a Windows/Git-Bash host a native `C:\...` path
has its backslashes eaten by bash (and mangled by the msys runtime even when
single-quoted) — the "Directory \drivers\etc does not exist; exiting — update
your msys package" class of failures. Rewrite a native drive path to forward
slashes in `_escape_shell_arg`, reusing the env layer's `_windows_to_msys_path`.
Both `C:/...` and `/c/...` fix the backslash bug (the MSYS coreutils resolve
either via the POSIX API). We emit `/c/...` purely for consistency: it's the
same form `_windows_to_msys_path` already produces for the terminal `cd`
(LocalEnvironment._quote_cwd_for_cd), so shell file ops and `cd` share one
helper and one path form.
Scoped from #55481, which also patched BaseEnvironment._quote_cwd_for_cd — but
LocalEnvironment already overrides that through `_windows_to_msys_path`, so on a
real Windows host the base branch never ran (the cwd is already `/c/...`).
Co-authored-by: konsisumer <der@konsi.org>
When web_search results are passed directly to web_extract, the URLs
field contains dict objects (e.g., {"url": "...", "title": "..."})
rather than plain URL strings. Two code paths assumed URLs were always
strings and crashed:
- agent/display.py get_cute_tool_message for web_extract: tried to call
url.replace() on a dict, causing AttributeError
- tools/web_tools.py web_extract_tool loop: tried regex search on a dict,
causing TypeError
Both now extract the URL string from dict objects (url or href field) or
fall back to empty string, preserving the cosmetic display and allowing
the tool to process the URLs correctly.
Fixes#61693
Git Bash hands file tools paths like /c/Users/... which Path() on native
Windows treats as relative \\c\\Users\\... under the process cwd. Reuse
local._msys_to_windows_path (extended for /cygdrive and /mnt drive forms)
in _resolve_path_for_task / _resolve_base_dir so read/write/search land on
the real drive. Container/WSL Linux paths are left untouched.
Salvages #50488 (drops unrelated desktop artifact commit); tests adapted
from #46995.
Co-authored-by: Jeff Watts <186512915+lEWFkRAD@users.noreply.github.com>
Co-authored-by: LeonSGP43 <cine.dreamer.one@gmail.com>
`_load_web_config()` is typed `-> dict` but returned `load_config().get("web",
{})`, which is `None` when the config has a present-but-null `web:` section
(YAML `web:` with no body). Every caller then does
`_load_web_config().get(...)` and raises `AttributeError: 'NoneType' object
has no attribute 'get'` — this hits `_get_backend`, `check_web_api_key`, and
the extract-char-limit reader.
Separately, `check_web_api_key()` read the backend as
`.get("backend", "").lower()`; a null `web.backend` value yields `None` (the
`""` default only applies when the key is absent), so `None.lower()` raised.
`check_web_api_key` is the `check_fn` gate for `web_search`/`web_extract`, so
this surfaced as an exception during tool-availability checking.
- Make `_load_web_config()` honor its `-> dict` contract (`... or {}`), fixing
the null-`web:`-section crash at every call site.
- Guard the backend value in `check_web_api_key` with `or ""`, mirroring the
existing guard in `_get_backend`.
Adds regression tests for both the null-backend-value and null-web-section
cases.
* test: deflake CI and dev-machine flaky tests in bulk
Fixes ten distinct flake sources found by mining recent CI failures and
running the full suite on a dev machine with real user state:
CI-observed races:
- tests/conftest.py live-system guard: allow signal 0 (pure liveness
probe) through _guarded_kill/_guarded_killpg. psutil.pid_exists()
probes a just-killed grandchild reparented to init; the subtree check
fails for it and the guard RuntimeError'd
test_entire_tree_is_sigkilled_not_just_parent intermittently on
unrelated PRs.
Hermeticity flakes (fail on dev machines with real state, pass on CI):
- agent/coding_context.py: _marker_root() now skips the shared temp
root (tempfile.gettempdir()) like it skips $HOME — a stray
/tmp/package.json flipped every tmp_path test into the coding
posture (9 failures in test_coding_context.py).
- test_agent_guardrails.py: pin MAX_CONCURRENT_CHILDREN=3 via autouse
monkeypatch instead of freezing the user's real config value at
import time (import-time vs call-time config mismatch).
- test_web_tools_config.py: TestCheckWebApiKey now neutralizes the
ddgs package probe and registry providers — the optional ddgs
package in a dev venv lit up the fallback backend.
- test_credential_pool.py: block claude_code/hermes-oauth credential
autodiscovery in the two pool-merge tests that assert exact id
lists (a real ~/.claude/.credentials.json seeded an extra entry).
- test_modal_sandbox_fixes.py: clear _permanent_approved /
_session_approved — the user's real command_allowlist silently
approved the guard-escalation commands under test.
- test_setup_irc.py: stub prompt_checklist to select only the IRC row;
the non-TTY cancel fallback re-ran the real configured platforms'
interactive setup_fn, which hit input() under captured stdin.
- test_doctor.py: TestGitHubTokenCheck now patches the module-level
HERMES_HOME constant (the file's established pattern) instead of
only setenv — doctor was running PRAGMA integrity_check against the
real multi-GB state.db and blowing the 300s per-file budget.
Latent atexit-duplication (same _enter_buffered_busy class as #34217):
- test_undo_command.py: drop importlib.reload(tui_gateway.server) in
fixture teardown; reload re-registers the module's atexit hooks.
- test_session_platform_resolution.py: drop per-test reload of
tui_gateway.server; every resolver reads env at call time.
* test: sentinel model value in ignore-user-config fallback assertion
With HERMES_IGNORE_USER_CONFIG=1, load_cli_config() falls back to the
repo-root cli-config.yaml (untracked, gitignored). On a dev machine that
file can legitimately set the same popular model the test hardcoded
(anthropic/claude-sonnet-4.6), flipping the != assertion locally while
CI (no cli-config.yaml) stayed green. Use an impossible sentinel model
name instead.
hermes send "MEDIA:/x.png This Caption" now arrives as one native captioned
bubble instead of a separate text message followed by an uncaptioned bubble.
Root cause: the standalone senders (hermes send / cron / send_message tool)
stripped the MEDIA: tag, sent the remaining text as its own message, and
called the media send with no caption -- even though hermes send's help
advertises the captioned form and the bridges/adapters already support a
caption. Signal already captioned correctly.
- tools/send_message_tool.py: new _media_caption_split() chokepoint decides
caption-vs-separate-body (single captionable non-voice file within the
platform's message-length cap). Wired into the Telegram, WhatsApp and
Discord dispatch paths.
- Telegram/WhatsApp/Discord: when the single captioned file is missing, the
caption text is delivered as a plain message so it is never silently lost.
- Telegram caption send gets a MarkdownV2->plain parse fallback.
- Tests: _media_caption_split unit tests + per-platform caption tests
(ride, multi-file fallback, voice exclusion, over-limit fallback,
missing-file text fallback); updated the 3 tests that asserted the old
text-then-media split.
Closes the gap reported against #58911 (the MEDIA_CAPTION directive PR);
credit to @ferreiraesilva for surfacing the caption behavior.
Review finding: callers mutate the returned dicts in place —
hermes_cli/web_server.py annotates s['enabled']/s['usage'] on the skills
list — so handing out the cached objects poisons the cache for every
subsequent caller (and is a cross-thread shared-mutable hazard in the
gateway). Return [dict(s) for s in cached] on both hit and miss paths;
warm-path cost is negligible (241x speedup retained on a 300-skill
fixture). Regression test mutates a returned list/dict and asserts the
next cached call is clean.
Review findings on the cherry-picked cache (follow-up to #58985):
- The cache key was the max mtime of only the TOP-LEVEL scan dirs.
Adding/removing a skill inside a category subdir bumps the category
dir's mtime, NOT the root's, so the cache served a stale list
indefinitely. Replace with a per-dir signature covering roots +
immediate children (one scandir per dir; mirrors
hermes_cli/profiles.py::_count_skills from d5eee133e).
- The disabled-set is config-driven and changes with no filesystem
mtime bump; fold it into the signature so /skills disable takes
effect without a restart.
- Platform is part of the signature (gateway processes serve multiple
platform scopes; scan results are platform-filtered).
- Add a 30s TTL to bound staleness from in-place SKILL.md edits (file
mtime is invisible to any directory signature).
- The original also keyed dirs off the module-level SKILLS_DIR constant;
the scan itself uses _skills_dir() (live profile HERMES_HOME) — use
the same resolution for the signature.
Mutation-verified: nested-add, disabled-set, and TTL tests fail against
the pre-fix cache and pass with it.
Background delegate_task completions only carried session_key. When multiple
active sessions shared a routing peer, get_or_create_session could recover the
latest ended_at IS NULL row and inject the subagent result into the wrong
session.
Capture parent_agent.session_id at dispatch time, include it on async-delegation
completion events, and pin gateway routing via switch_session when the
synthetic completion message is handled.
Fixes#57498
Extends the salvaged session_key filter with the same fail-closed,
compression-chain-aware ownership gate the poller uses (#55578):
- drain_notifications() accepts an owns_event callback; when provided,
an async-delegation event is consumed ONLY on positive proof of
ownership, and a broken callback re-queues (never leaks). Bare key
equality remains for single-session callers (CLI); no filter remains
legacy behavior.
- The TUI post-turn drain passes _session_owns_notification_event, so
it can't adopt another session's (or an orphan's) delegation payload,
while a post-compression session still claims its own pre-compression
dispatches - the gap bare key equality left open.
The completion event already carries the dispatching session's session_key
(captured at dispatch time in delegate_tool.py:2798), but the delivery
router ignored it — results landed in whatever session was active at
completion time instead of the session that dispatched the subagent.
Changes:
- drain_notifications() in process_registry.py: optional session_key
filter. Non-matching async_delegation events are re-queued instead of
consumed, so they remain available for the correct session's drain.
- cli.py process_loop: passes active session_key to drain_notifications()
- tui_gateway/server.py post-turn drain: passes session_key from the
TUI session dict
- gateway/run.py _build_process_event_source: logs warning when routing
metadata is unresolvable (previously silent drop)
- Regression tests verifying session-scoped drain filtering
Fixes#58684
Carry the live TUI session id with async delegation completion events and prefer the commissioning UI session when desktop pollers share the completion queue. Resolve compressed session keys to their continuation before treating events as orphaned, and capture the live parent agent session id for TUI/ACP dispatch.
safe_load() raises ComposerError on multi-document streams (k8s manifests)
and ConstructorError on application-defined tags (CloudFormation !Sub,
Ansible !vault) — both valid YAML syntax. Now that the linter's verdict is
a fail-closed write gate, those false positives would refuse legitimate
writes outright. Switch to yaml.parse() (scanner+parser only), which still
catches real syntax failures.
write_file() previously called _atomic_write() first and only ran the
JSON/YAML/TOML/Python syntax check afterward as an informational lint
delta -- a parse failure never set the top-level `error` key, so a
corrupt structured-data write still landed on disk (and file_tools.py's
files_modified gating, which keys off `error`, silently reported it as
a successful modification).
Move the in-process syntax check for JSON/YAML/TOML ahead of
_atomic_write() and refuse the write outright on a parse failure: no
temp file, no rename, nothing touches disk, and the result carries a
top-level `error` so callers correctly see it as unmodified.
Deliberately scoped to _FAIL_CLOSED_INPROC_EXTS (JSON/YAML/TOML), not
all of LINTERS_INPROC -- .py is excluded because this codebase's own
test fixtures (TestPatchReplacePostWriteVerification et al.) write
arbitrary non-Python text through *.py paths purely to exercise
write-mechanics; a hard block there broke 3 previously-passing tests
during development. Python keeps its pre-existing non-blocking
lint-delta report.
Adds tests/tools/test_write_file_syntax_gate.py: invalid JSON/YAML/YML/
TOML refused with nothing written (new file) and nothing modified
(existing file); valid JSON/YAML still written byte-for-byte; a
non-linted extension with garbage content is unaffected; invalid Python
is confirmed NOT hard-refused (still just reported).
The salvaged test predates the parked-server self-probe
(_PARKED_RETRY_INTERVAL, landed on main after the PR branched): after the
final failed retry, run() parks in a real asyncio.wait that the patched
asyncio.sleep doesn't cover, stalling the test 300s. Signal shutdown once
the retry budget is exhausted so the park exits immediately.
A stdio MCP server (e.g. `npx -y mcp-remote <url>`) is spawned as a direct
child of the Hermes process. Existing teardown (MCPServerTask.shutdown() /
_kill_orphaned_mcp_children()) reaps it correctly on a clean exit, but a
kill -9 / crash / force-quit of the Hermes process skips that path entirely
-- the child (and its own descendants, e.g. mcp-remote's spawned node
process) is orphaned and keeps running. Repeated ungraceful restarts pile up
N orphaned processes racing to hold the same upstream SSE session, producing
errors like 'Invalid request parameters' on legitimate reconnects.
macOS/Linux have no portable equivalent of prctl(PR_SET_PDEATHSIG) at the
Python subprocess level, so this adds a thin supervisor
(tools/mcp_stdio_watchdog.py) that:
- execs the real command as its own child in its own process group
- passes stdin/stdout/stderr through untouched (MCP stdio protocol
talks directly over those streams)
- polls the original spawning PID with the same orphan-detection
algorithm already proven in tui_gateway/slash_worker.py (ppid
comparison + psutil creation-time guard against PID reuse)
- SIGTERM-then-SIGKILL's the child's process group the moment the
original parent is gone
Wired into _run_stdio via a new _wrap_command_with_watchdog() helper,
POSIX-only (matches the existing killpg-based cleanup's platform scope),
fails open (any error resolving pid/create-time falls back to the
unwrapped command) so this can never be the reason a working MCP server
stops starting.
Verified: reproduced the exact orphan scenario standalone (fake parent
process spawns watchdog + fake long-running MCP child, kill -9 the fake
parent, confirm the watchdog reaps the child within its poll window with
zero leaked processes). Updated test_mcp_tool_issue_948.py's resolved-path
assertion to check the watchdog-wrapped command instead of the raw
resolved binary. Full test_mcp_tool.py + test_mcp_stability.py +
test_mcp_tool_issue_948.py suite: 232 passed. Full -k mcp sweep across the
whole test tree: 1003 passed, 2 skipped, 0 failed.
A stdio MCP server that never completes `initialize` (e.g. emits a
non-JSON-RPC frame and then blocks on stdin) leaks a child process plus its
stdio pipes/pidfd on every discovery-retry cycle — unbounded, until the
gateway hits EMFILE and every new open()/spawn fails (#59349).
Root cause (confirmed by instrumenting the live repro, and different from the
issue's own hypothesis): the spawned child IS captured in `new_pids`, so the
report's "new_pids empty at finally" guess is not it. The real cause is that
`session.initialize()` hangs forever on the garbage stream. `connect_timeout`
only bounds the caller's `.result()` wait on the foreground thread — it does
NOT cancel the `_run_stdio` coroutine on the background MCP loop. So the
coroutine is stuck at `await session.initialize()` permanently, its cleanup
`finally` never runs, the child is never reaped, and it stays invisible to the
orphan-reaper (whose `_orphan_stdio_pids` set never gets populated).
Fix: wrap `session.initialize()` in `asyncio.wait_for(..., connect_timeout)`
so a stalled handshake fails instead of hanging. The TimeoutError unwinds
through the SDK context managers (closing the child's stdin -> EOF -> exit)
and lets the existing `finally` reap any straggler. Cross-platform — no
signals/pgid/proc.
Scope: stdio only. The HTTP path has the same `await session.initialize()`
shape but spawns no subprocess (so it can't cause this leak) and already has
httpx transport timeouts.
Verified: the reporter's repro goes from unbounded growth to draining to zero;
added a hermetic regression test (fake transport whose `initialize()` hangs,
asserts the connect is bounded by connect_timeout) that fails on the pre-fix
code and passes on the fix; 566 existing MCP tests pass; ruff clean.
Repro confirmed on macOS (pipe FDs); the Linux-specific pidfd growth in the
report should be equivalent — the reporter offered to validate on Linux.
Closes#59349
When an MCP stdio subprocess fails to connect (token expiry, port
contention, timeout), the run() reconnect loop retries with backoff.
Each retry calls _run_stdio() which spawns a new process pair, but the
previous failed pair was only detected as orphaned (added to
_orphan_stdio_pids) — never actually killed. This caused rapid zombie
accumulation: 5 failed attempts × 2 procs each = 10 orphans competing
for the same port.
Add a _kill_orphaned_mcp_children() call at the top of _run_stdio(),
before the _snapshot_child_pids() baseline, so any orphans from prior
failed attempts are reaped before a new subprocess is spawned.
Fixes#57355
BaseEnvironment writes shell snapshots and cwd metadata through the process
umask. With a common 022 umask, snapshot files containing exported environment
state landed at mode 0644 even though they can include env-carried credentials
from the parent process.
Set umask 077 only around Hermes metadata writes: the initial snapshot
bootstrap and the post-command snapshot/cwd refresh. User commands still run
under the caller's original umask, while Hermes-owned snapshot and cwd files
are created owner-only.
This intentionally does not copy the source PR's global orphan sweep; deleting
all matching /tmp snapshot files could interfere with concurrent Hermes
processes. The security-critical local disclosure fix is the file mode clamp.
This is salvageable because the source report still identifies a concrete
credential-disclosure path, but the safe subset is smaller than the original
proposal: clamp only the Hermes-owned snapshot writes and leave process-wide
cleanup, user command umask, and concurrent sessions alone.
Salvages source PR: https://github.com/NousResearch/hermes-agent/pull/20056
Related issue: https://github.com/NousResearch/hermes-agent/issues/48441
Co-authored-by: Andrew Homeyer <andrew@hndl.app>
Port from anomalyco/opencode#31877: JSON Schema type arrays like
["number","string"] (common in MCP tool schemas) were collapsed to the
first non-null type, silently dropping every other branch. Several
tool-call backends reject the array form outright — llama.cpp's grammar
generator and Gemini via OpenAI-compatible transports (e.g. GitHub
Copilot proxying to Gemini) 400 on it.
_sanitize_node now mirrors @ai-sdk/google: a single non-null type stays
type:X (+nullable if null was present), multiple non-null types become
an anyOf of single-type schemas so no branch is lost, and an all-null
array becomes type:null. Single-null collapse is unchanged.
Verified nested (object props, array items) survive the full sanitize
pipeline — combinator stripping is top-level-only and nullable-union
collapse only fires on single-survivor unions, so multi-type anyOf is
left intact.
