From 922083ab9318466e90f22112928c6e7959c83b27 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:00:49 -0700 Subject: [PATCH 1/3] =?UTF-8?q?refactor(tools):=20trim=20subagent=5Fworktr?= =?UTF-8?q?ee/terminal=5Fscope/terminal=5Fhints/thread=5Fcontext/slash=5Fc?= =?UTF-8?q?onfirm=20=E2=80=94=20drop=20no-op=20HERMES=5FHOME=20override,?= =?UTF-8?q?=20inline=20single-use=20constants,=20unify=20probes,=20compact?= =?UTF-8?q?=20docstrings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tools/slash_confirm.py | 35 +++++--------- tools/subagent_worktree.py | 98 ++++++++++++++++---------------------- tools/terminal_hints.py | 61 +++++++++--------------- tools/terminal_scope.py | 86 +++++++++++---------------------- tools/thread_context.py | 63 +++++++----------------- 5 files changed, 121 insertions(+), 222 deletions(-) diff --git a/tools/slash_confirm.py b/tools/slash_confirm.py index e465cff580..b7bdbe3f3a 100644 --- a/tools/slash_confirm.py +++ b/tools/slash_confirm.py @@ -1,14 +1,12 @@ """Generic slash-command confirmation primitive (gateway-side). -Slash commands with an expensive side effect worth surfacing (currently only -``/reload-mcp``, which invalidates the provider prompt cache) route through -here. Adapters with button UIs render Approve Once / Always Approve / Cancel -and call ``resolve()`` from the callback; text-only adapters get a prompt and -the gateway intercepts ``/approve``, ``/always``, ``/cancel`` replies. - -State is module-level (like ``tools.approval``) so adapters can resolve -callbacks without a ``GatewayRunner`` backreference. The CLI has its own -synchronous variant (``_prompt_slash_confirm`` in ``cli.py``). +Slash commands with an expensive side effect (currently only ``/reload-mcp``, +which invalidates the provider prompt cache) route through here. Button-UI +adapters render Approve Once / Always Approve / Cancel and call ``resolve()``; +text-only adapters get a prompt and the gateway intercepts ``/approve``, +``/always``, ``/cancel``. State is module-level (like ``tools.approval``) so +adapters can resolve without a ``GatewayRunner`` backreference. The CLI has its +own synchronous variant (``_prompt_slash_confirm`` in ``cli.py``). """ from __future__ import annotations @@ -24,9 +22,8 @@ logger = logging.getLogger(__name__) _pending: Dict[str, Dict[str, Any]] = {} _lock = threading.RLock() -# A pending confirm older than this is discarded when the next message -# arrives for the same session (buttons live as long as the adapter keeps -# the callback_data). +# A pending confirm older than this is discarded when the next message arrives +# for the same session (buttons live as long as the adapter keeps callback_data). DEFAULT_TIMEOUT_SECONDS = 300 @@ -38,12 +35,8 @@ def register( ) -> None: """Register a pending confirm, superseding any prior one for the session.""" with _lock: - _pending[session_key] = { - "confirm_id": confirm_id, - "command": command, - "handler": handler, - "created_at": time.time(), - } + _pending[session_key] = {"confirm_id": confirm_id, "command": command, + "handler": handler, "created_at": time.time()} def get_pending(session_key: str) -> Optional[Dict[str, Any]]: @@ -83,7 +76,6 @@ async def resolve( Returns the handler's output string, or None if the confirm was stale, already resolved, or the confirm_id doesn't match (superseded prompt). - Safe from an asyncio button callback or the gateway's message intercept. """ with _lock: entry = _pending.get(session_key) @@ -102,9 +94,6 @@ async def resolve( try: result = await handler(choice) except Exception as exc: - logger.error( - "Slash-confirm handler for /%s raised: %s", - command, exc, exc_info=True, - ) + logger.error("Slash-confirm handler for /%s raised: %s", command, exc, exc_info=True) return f"❌ Error handling confirmation: {exc}" return result if isinstance(result, str) else None diff --git a/tools/subagent_worktree.py b/tools/subagent_worktree.py index ae98759839..edeb76dbe1 100644 --- a/tools/subagent_worktree.py +++ b/tools/subagent_worktree.py @@ -1,18 +1,14 @@ -"""Opt-in git worktree isolation for delegated subagents (clean-room port of -Muse Code's documented ``--subagent-worktree-isolation`` semantics). -Enable with ``delegation.worktree_isolation: true`` (default false). - -Contract: git-only — in a non-git workspace the setting is ignored without -error and children share the parent's cwd. One worktree per child, branched -from the parent's ``HEAD`` under ``/.worktrees/subagent-`` on branch -``hermes-subagent/``. The parent reviews/merges: each result entry reports -path, branch, commit count and dirty state. A worktree is pruned only on -affirmative proof (zero commits AND clean tree, both probes succeeded); if a -probe fails the state is unknown, so it is kept and the entry carries -``inspection_failed`` + ``note``. +"""Opt-in git worktree isolation for delegated subagents +(``delegation.worktree_isolation: true``, default false). +Git-only: in a non-git workspace the setting is ignored and children share +the parent's cwd. One worktree per child, branched from the parent's ``HEAD`` +under ``/.worktrees/subagent-`` on branch ``hermes-subagent/``. +Each result entry reports path, branch, commit count and dirty state; a +worktree is pruned only on affirmative proof (zero commits AND clean tree, both +probes succeeded) — otherwise it is kept with ``inspection_failed`` + ``note``. Local terminal backend only: on docker/ssh/modal the host worktree is invisible -inside the sandbox, so isolation is skipped (debug log) rather than half-applied. +inside the sandbox, so isolation is skipped rather than half-applied. """ from __future__ import annotations @@ -29,18 +25,14 @@ from hermes_cli._subprocess_compat import harden_git_argv, noninteractive_git_en logger = logging.getLogger(__name__) _GIT_TIMEOUT = 30 -_WORKTREES_DIRNAME = ".worktrees" -_BRANCH_NAMESPACE = "hermes-subagent" def _run_git(args, cwd: str, timeout: int = _GIT_TIMEOUT): - """Run a git command, capturing output. Never raises on non-zero exit. + """Run git capturing output; never raises on non-zero exit. - Runs under :func:`noninteractive_git_env` (GHSA-7x36-8jrh-v4pw): worktree - isolation runs automatically for delegated subagents against whatever repo - the parent sits in, and ``worktree add`` runs checkout hooks. Disabling the - fsmonitor/hooks/pager/credential config sinks keeps a malicious ``.git/config`` - from executing on the host. + Runs under :func:`noninteractive_git_env` (GHSA-7x36-8jrh-v4pw): this runs + automatically against whatever repo the parent sits in and ``worktree add`` + runs checkout hooks, so a malicious ``.git/config`` must not execute. """ return subprocess.run(["git", *harden_git_argv(args)], cwd=cwd, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, @@ -77,29 +69,26 @@ def resolve_repo_root(path: Optional[str]) -> Optional[str]: def _ensure_gitignore_entry(repo_root: str) -> None: """Best-effort: keep ``.worktrees/`` out of git status.""" gitignore = Path(repo_root) / ".gitignore" - entry = f"{_WORKTREES_DIRNAME}/" try: existing = gitignore.read_text(encoding="utf-8-sig", errors="replace") if gitignore.exists() else "" - if entry not in existing.splitlines(): + if ".worktrees/" not in existing.splitlines(): with open(gitignore, "a", encoding="utf-8") as f: if existing and not existing.endswith("\n"): f.write("\n") - f.write(f"{entry}\n") + f.write(".worktrees/\n") except Exception as exc: logger.debug("subagent worktree: could not update .gitignore: %s", exc) def create_subagent_worktree(parent_cwd: Optional[str], subagent_id: Optional[str] = None) -> Optional[Dict[str, str]]: - """Create an isolated worktree for one child agent. Returns - ``path``/``branch``/``repo_root``/``base_commit``, or ``None`` when not a git - repo or creation fails — absence of git downgrades silently to shared workspace.""" + """Create an isolated worktree for one child; None (silent downgrade) when not a git repo or on failure.""" repo_root = resolve_repo_root(parent_cwd) if not repo_root: return None wt_name = f"subagent-{(subagent_id or uuid.uuid4().hex[:8]).replace('/', '-')}" - branch = f"{_BRANCH_NAMESPACE}/{wt_name}" - wt_path = Path(repo_root) / _WORKTREES_DIRNAME / wt_name + branch = f"hermes-subagent/{wt_name}" + wt_path = Path(repo_root) / ".worktrees" / wt_name try: wt_path.parent.mkdir(parents=True, exist_ok=True) @@ -136,14 +125,11 @@ def mark_worktree_payload_unproven( ) -> Dict[str, Any]: """Flag a worktree result payload as un-inspected, in place. - A failed probe proves nothing, so the fields it would have filled keep - their defaults. The parent only sees this dict (not logs), so the - uncertainty must travel in the payload or "0 commits, clean" reads as "the - child produced nothing". *unmeasured* names only the fields actually left - unproven: one probe can succeed while the other fails, and calling a - measured value UNKNOWN would be its own misreport. Shared by - ``finalize_subagent_worktree`` and ``delegate_tool``'s finalize-raised - fallback so the two producers of this schema cannot drift. + The parent only sees this dict, so the uncertainty must travel in it or + "0 commits, clean" reads as "the child produced nothing". *unmeasured* + names only the fields actually left unproven (one probe can succeed while + the other fails). Shared by ``finalize_subagent_worktree`` and + ``delegate_tool``'s fallback so the two producers cannot drift. """ path = payload.get("path", "") branch = payload.get("branch", "") @@ -159,19 +145,18 @@ def mark_worktree_payload_unproven( def unproven_worktree_payload(info: Dict[str, str], reason: str) -> Dict[str, Any]: - """Complete un-inspected payload for callers that never got one back - (``delegate_tool``'s fallback when ``finalize_subagent_worktree`` raises). - Emits exactly the parent-facing schema, WITHOUT ``repo_root``/``base_commit``.""" + """Complete un-inspected payload for ``delegate_tool`` when ``finalize_subagent_worktree`` raises.""" return mark_worktree_payload_unproven(_base_payload(info), reason) def finalize_subagent_worktree(info: Dict[str, str], *, prune: bool = True) -> Dict[str, Any]: """Inspect (and possibly prune) a child worktree after the child finishes. - Returns path, branch, ``commits`` ahead of base, ``dirty``, ``pruned``. Prunes - only when *prune*, commits==0, clean tree AND both git probes succeeded. If a - probe exits non-zero or raises, the worktree/branch are kept and the payload - carries ``inspection_failed: True`` + ``note``; ``commits``/``dirty`` are then - defaults, NOT measurements.""" + + Prunes only when *prune*, commits==0, clean tree AND both git probes + succeeded. If a probe exits non-zero or raises the worktree/branch are kept + and the payload carries ``inspection_failed`` + ``note``; ``commits`` / + ``dirty`` are then defaults, NOT measurements. + """ path = info.get("path", "") branch = info.get("branch", "") base_commit = info.get("base_commit", "") @@ -189,19 +174,18 @@ def finalize_subagent_worktree(info: Dict[str, str], *, prune: bool = True) -> D failed: list = [] unmeasured: list = [] + probes = ( + ("commits", "rev-list", ["rev-list", "--count", f"{base_commit}..HEAD"], lambda out: int(out or 0)), + ("dirty", "status", ["status", "--porcelain"], bool), + ) try: - counted = _run_git(["rev-list", "--count", f"{base_commit}..HEAD"], cwd=path) - if counted.returncode == 0: - payload["commits"] = int(counted.stdout.strip() or 0) - else: - failed.append(f"rev-list exit {counted.returncode}: {counted.stderr.strip()[:200]}") - unmeasured.append("commits") - status = _run_git(["status", "--porcelain"], cwd=path) - if status.returncode == 0: - payload["dirty"] = bool(status.stdout.strip()) - else: - failed.append(f"status exit {status.returncode}: {status.stderr.strip()[:200]}") - unmeasured.append("dirty") + for field, label, args, parse in probes: + res = _run_git(args, cwd=path) + if res.returncode == 0: + payload[field] = parse(res.stdout.strip()) + else: + failed.append(f"{label} exit {res.returncode}: {res.stderr.strip()[:200]}") + unmeasured.append(field) except Exception as exc: # Timeout, OSError or non-numeric rev-list stdout: which probe raised is # unknowable, so neither value is trustworthy — keep the worktree. diff --git a/tools/terminal_hints.py b/tools/terminal_hints.py index 213c3d2b9b..071808fad4 100644 --- a/tools/terminal_hints.py +++ b/tools/terminal_hints.py @@ -1,14 +1,11 @@ """Output-pattern failure hints for the terminal tool. -Extends the exit-code semantics table in ``terminal_tool`` with an -output-pattern tier: a bounded scan of failed-command output mapped to one -short, actionable recovery hint. - -Rules (keep when adding patterns): only fires on non-zero exit; at most ONE -hint, first match wins, patterns ordered by observed production frequency; -scans only the first ``_SCAN_CHARS`` so hints key on error headers, not deep -context; hints state the *next action* in 1-2 sentences, not a diagnosis; -pure function, no I/O or config reads. +Extends the exit-code semantics table in ``terminal_tool`` with a bounded scan +of failed-command output mapped to ONE short, actionable recovery hint. +Rules: only fires on non-zero exit; first match wins, patterns ordered by +observed production frequency; scans only the first ``_SCAN_CHARS`` so hints +key on error headers, not deep context; hints state the *next action* in 1-2 +sentences; pure function, no I/O or config reads. """ from __future__ import annotations @@ -16,13 +13,11 @@ from __future__ import annotations import re from typing import Callable, Optional -# Bounded scan window: error headers appear early; deep output is noise. _SCAN_CHARS = 4000 def _regex_hint(pattern: str, message: str | Callable[[str], str], flags: int = 0) -> Callable[[str, str], Optional[str]]: - """Build a hint that fires when ``pattern`` matches; ``{0}`` = first capture group, - or ``message(group1)`` when a callable is given.""" + """Hint firing when ``pattern`` matches; ``{0}`` = first group, or ``message(group1)`` if callable.""" rx = re.compile(pattern, flags) def hint(command: str, output: str) -> Optional[str]: @@ -103,8 +98,8 @@ _OUTPUT_HINTS: list[Callable[[str, str], Optional[str]]] = [ ), ] -# Exit-code-only hints for codes the semantics table in terminal_tool does -# not cover per-command. Checked after output patterns. +# Exit-code-only hints for codes the terminal_tool semantics table does not +# cover per-command. Checked after output patterns. _EXIT_CODE_HINTS: dict[int, str] = { 126: "Exit 126: the file was found but is not executable — `chmod +x` it or invoke it via its interpreter (e.g. `bash script.sh`).", 137: "Exit 137: the process was SIGKILLed — usually out-of-memory or an external kill. Reduce memory use or check `dmesg | tail` before retrying.", @@ -112,14 +107,10 @@ _EXIT_CODE_HINTS: dict[int, str] = { } -# --------------------------------------------------------------------------- -# Masked-success detection (exit 0 that probably isn't a success) -# --------------------------------------------------------------------------- -# `cargo build 2>&1 | tail -20` exits with tail's 0 (no pipefail) and -# `cargo build || echo FAILED` exits with echo's 0, so the model can conclude -# a build passed while the output says it failed. Conservative: BOTH the -# command shape (top-level pipe into a passthrough consumer, or `|| `) AND a strong tool-specific failure shape must hold, and +# Masked-success detection: `cargo build 2>&1 | tail -20` exits with tail's 0 +# (no pipefail) and `cargo build || echo FAILED` with echo's 0, so the model can +# conclude a build passed while the output says it failed. Conservative: BOTH a +# masking command shape AND a strong tool-specific failure shape must hold, and # read-only heads (`grep ... | head`) are excluded because their output # legitimately contains error text. Advisory only — exit_code is never changed. @@ -174,8 +165,8 @@ _FAILURE_SHAPES = re.compile( def _first_token(command: str) -> str: + """Basename of the command head, skipping leading env-var assignments.""" for tok in (command or "").strip().split(): - # Skip env-var assignments and common wrappers. if "=" in tok and not tok.startswith(("=", "./", "/")): continue return tok.rsplit("/", 1)[-1] @@ -183,15 +174,10 @@ def _first_token(command: str) -> str: def annotate_masked_success(command: str, output: str) -> Optional[str]: - """Return a warning note when an exit-0 result likely masks a failure. - - Caller gates on exit_code == 0. Advisory only; returns None otherwise. - """ + """Warning note when an exit-0 result likely masks a failure (caller gates on exit 0).""" cmd = command or "" window = (output or "")[:_SCAN_CHARS] - if not cmd or not window or _first_token(cmd) in _READONLY_HEADS: - return None - if not _FAILURE_SHAPES.search(window): + if not cmd or not window or _first_token(cmd) in _READONLY_HEADS or not _FAILURE_SHAPES.search(window): return None return next((note for rx, note in _MASKING_SHAPES if rx.search(cmd)), None) @@ -201,12 +187,11 @@ def annotate_failure(command: str, exit_code: int, output: str) -> Optional[str] if exit_code == 0: return None window = (output or "")[:_SCAN_CHARS] - if window: - for fn in _OUTPUT_HINTS: - try: - hint = fn(command or "", window) - except Exception: - continue - if hint: - return hint + for fn in _OUTPUT_HINTS if window else (): + try: + hint = fn(command or "", window) + except Exception: + continue + if hint: + return hint return _EXIT_CODE_HINTS.get(exit_code) diff --git a/tools/terminal_scope.py b/tools/terminal_scope.py index d47164a64b..1cd8807b57 100644 --- a/tools/terminal_scope.py +++ b/tools/terminal_scope.py @@ -1,20 +1,13 @@ """Per-turn terminal scope: profile-scoped TERMINAL_* policy. -Multiplexed surfaces (gateway, dashboard/TUI, cron) serve several profiles -from one process; mirroring terminal settings into ``os.environ`` let the -first profile pin its backend onto everyone else (sandbox escape). Like -``agent/secret_scope.py`` for credentials, a ContextVar holds the active -profile's COMPLETE effective ``TERMINAL_*`` policy, installed at each -in-process profile boundary. - -- **Authoritative projection.** While a scope is bound, ``terminal_env`` - resolves ONLY from the policy (defaults + profile ``.env`` + its - ``config.yaml``); omitted keys yield the defined default, never ambient - ``os.environ`` (#68559). -- **Fail closed.** If the policy cannot be resolved, callers install a - *refusal* scope and terminal execution under it raises - :class:`TerminalPolicyUnavailable` instead of falling back to ambient - authority. +Multiplexed surfaces (gateway, dashboard/TUI, cron) serve several profiles from +one process; mirroring terminal settings into ``os.environ`` let the first +profile pin its backend onto everyone else (sandbox escape). Like +``agent/secret_scope.py``, a ContextVar holds the active profile's COMPLETE +effective ``TERMINAL_*`` policy. While bound, ``terminal_env`` resolves ONLY +from it (omitted keys -> defined default, never ambient env). If the policy +cannot be resolved a *refusal* scope is installed and terminal execution raises +:class:`TerminalPolicyUnavailable` instead of falling back to ambient authority. """ from __future__ import annotations @@ -28,12 +21,13 @@ from typing import Any, Dict, Iterator, Optional logger = logging.getLogger(__name__) -# None = no scope bound (historical process-env behavior); dict = the active -# profile's complete policy; TerminalPolicyRefusal = resolution failed. +# None = no scope bound (process-env behavior); dict = active profile's complete +# policy; TerminalPolicyRefusal = resolution failed. _terminal_scope_var: ContextVar = ContextVar("hermes_terminal_scope", default=None) -# Terminal keys whose config default lives in the consuming tool -# (terminal_tool.py) rather than DEFAULT_CONFIG; DEFAULT_CONFIG wins on overlap. +# Terminal keys whose config default lives in terminal_tool.py rather than +# DEFAULT_CONFIG (DEFAULT_CONFIG wins on overlap). Without them the projection +# is not total. _TOOL_LEVEL_DEFAULTS: Dict[str, Any] = { "cwd": ".", "ssh_host": "", @@ -80,23 +74,23 @@ def get_terminal_scope() -> Optional[Dict[str, str]]: return _terminal_scope_var.get() -def _raise_if_refusal(scope: Any) -> None: +def enforce_no_refusal() -> None: + """Raise when the active scope is a refusal scope (fail closed).""" + scope = _terminal_scope_var.get() if isinstance(scope, TerminalPolicyRefusal): - raise TerminalPolicyUnavailable( - f"terminal policy unavailable for this profile: {scope.reason}" - ) + raise TerminalPolicyUnavailable(f"terminal policy unavailable for this profile: {scope.reason}") def terminal_env(name: str, default: str = "") -> str: """Authoritative read of a ``TERMINAL_*`` variable. - No scope: process env, then *default*. Refusal scope: raise. Policy - scope: ONLY the policy; a missing key yields *default*, never os.environ. + No scope: process env, then *default*. Refusal scope: raise. Policy scope: + ONLY the policy; a missing key yields *default*, never os.environ. """ scope = _terminal_scope_var.get() if scope is None: return os.environ.get(name, default) - _raise_if_refusal(scope) + enforce_no_refusal() value = scope.get(name) return default if value is None else str(value) @@ -104,9 +98,9 @@ def terminal_env(name: str, default: str = "") -> str: def build_profile_terminal_scope(hermes_home: "Any") -> Dict[str, str]: """Build the COMPLETE effective ``TERMINAL_*`` policy for a profile home. - Projection: ``DEFAULT_CONFIG['terminal']`` <- profile ``.env`` TERMINAL_* - <- profile ``config.yaml`` ``terminal:`` keys. Total by construction, so a - bound scope never widens back to ambient process authority. Raises + Projection: ``DEFAULT_CONFIG['terminal']`` <- profile ``.env`` TERMINAL_* <- + profile ``config.yaml`` ``terminal:`` keys. Total by construction, so a bound + scope never widens back to ambient authority. Raises :class:`TerminalPolicyUnavailable` when either file exists but cannot be read/parsed. """ @@ -116,7 +110,6 @@ def build_profile_terminal_scope(hermes_home: "Any") -> Dict[str, str]: from hermes_cli.config_defaults import DEFAULT_CONFIG defaults = DEFAULT_CONFIG.get("terminal") if isinstance(DEFAULT_CONFIG, dict) else None - # Without the tool-level defaults the projection is not total. defaults = {**_TOOL_LEVEL_DEFAULTS, **(defaults if isinstance(defaults, dict) else {})} scope: Dict[str, str] = {} @@ -146,22 +139,12 @@ def build_profile_terminal_scope(hermes_home: "Any") -> Dict[str, str]: if key.startswith("TERMINAL_"): scope[key] = str(value) - # Read config.yaml through the HERMES_HOME override so the profile's own - # file is consulted; a present-but-unparseable file fails closed. - from hermes_constants import ( - get_hermes_home_override, - reset_hermes_home_override, - set_hermes_home_override, - ) - - override_token = None - if get_hermes_home_override() != str(home): - override_token = set_hermes_home_override(home) + # The profile's own config.yaml is read directly (not read_raw_config(): it + # collapses "missing" and "unparseable" into {}); present-but-unparseable + # fails closed. try: config_path = home / "config.yaml" if config_path.exists(): - # Not read_raw_config(): it collapses "missing" and "unparseable" - # into {}; the file exists, so a parse failure must fail closed. from hermes_cli.config import fast_safe_load try: @@ -177,18 +160,12 @@ def build_profile_terminal_scope(hermes_home: "Any") -> Dict[str, str]: raise except Exception as exc: raise TerminalPolicyUnavailable(f"cannot resolve terminal config in {home}: {exc}") from exc - finally: - if override_token is not None: - reset_hermes_home_override(override_token) return scope def install_profile_terminal_scope(hermes_home: "Any") -> Token: - """Build AND install a profile's policy; on failure install the refusal scope. - - Never raises. Returns the token for ``reset_terminal_scope``. - """ + """Build AND install a profile's policy; on failure install the refusal scope. Never raises.""" try: return set_terminal_scope(build_profile_terminal_scope(hermes_home)) except TerminalPolicyUnavailable as exc: @@ -196,15 +173,8 @@ def install_profile_terminal_scope(hermes_home: "Any") -> Token: return install_refusal_scope(str(exc)) -def enforce_no_refusal() -> None: - """Raise when the active scope is a refusal scope (fail closed, #68559).""" - _raise_if_refusal(_terminal_scope_var.get()) - - @contextmanager -def install_and_reset_profile_terminal_scope( - hermes_home: "Any", -) -> Iterator[None]: +def install_and_reset_profile_terminal_scope(hermes_home: "Any") -> Iterator[None]: """Install the profile's terminal policy for a bounded turn/fire. Never raises.""" token = install_profile_terminal_scope(hermes_home) try: diff --git a/tools/thread_context.py b/tools/thread_context.py index 2a53589212..b48867fa98 100644 --- a/tools/thread_context.py +++ b/tools/thread_context.py @@ -1,29 +1,13 @@ -#!/usr/bin/env python3 """Propagate agent-turn context into worker threads that dispatch Hermes tools. -A bare ``threading.Thread`` / ``ThreadPoolExecutor`` worker starts with an empty -``contextvars.Context`` and no thread-local approval/sudo callbacks, so tool -dispatch inside it silently loses: - - * the approval session/platform ContextVars (``tools.approval`` / - ``gateway.session_context``) — gateway sessions then fall into - ``check_dangerous_command``'s non-interactive auto-approve branch and - dangerous commands run without prompting; - * the thread-local CLI approval/sudo callbacks (``tools.terminal_tool``) — - ``prompt_dangerous_approval`` then cannot reach the user (GHSA-qg5c-hvr5-hjgr). - -One audited capture/install/clear lifecycle for every place that fans tool -dispatch onto worker threads (``agent.tool_executor``, ``execute_code`` RPC threads). - -Call :func:`propagate_context_to_thread` **on the parent thread** (it snapshots -the parent's ContextVars and callbacks at call time) and use the result as the -worker target:: - - t = threading.Thread(target=propagate_context_to_thread(loop_fn), args=(...)) - executor.submit(propagate_context_to_thread(worker_fn), *args) - -Callbacks are installed for the worker's lifetime and **always cleared on exit**, -so a recycled thread never holds a stale reference to a disposed CLI instance. +A bare ``threading.Thread`` / ``ThreadPoolExecutor`` worker starts with an +empty ``contextvars.Context`` and no thread-local approval/sudo callbacks, so +tool dispatch inside it silently loses the approval ContextVars (gateway +sessions then auto-approve dangerous commands) and the CLI approval/sudo +callbacks (``prompt_dangerous_approval`` cannot reach the user, +GHSA-qg5c-hvr5-hjgr). Call :func:`propagate_context_to_thread` **on the parent +thread** (it snapshots at call time) and use the result as the worker target. +Callbacks are installed for the worker's lifetime and always cleared on exit. """ from __future__ import annotations @@ -38,8 +22,8 @@ logger = logging.getLogger(__name__) def _callback_api(): """Resolve the terminal_tool callback getters/setters. - Imported lazily: ``tools.terminal_tool`` imports ``tools.approval`` at module - load, so a top-level import would risk a cycle for callers in ``tools.approval``. + Lazy: ``tools.terminal_tool`` imports ``tools.approval`` at module load, so a + top-level import would risk a cycle for callers in ``tools.approval``. """ from tools.terminal_tool import ( _get_approval_callback, @@ -47,22 +31,15 @@ def _callback_api(): set_approval_callback, set_sudo_password_callback, ) - return ( - _get_approval_callback, - _get_sudo_password_callback, - set_approval_callback, - set_sudo_password_callback, - ) + return (_get_approval_callback, _get_sudo_password_callback, set_approval_callback, set_sudo_password_callback) def propagate_context_to_thread(target: Callable) -> Callable: - """Wrap *target* so it runs on a worker thread with the *current* thread's - ContextVars and approval/sudo callbacks. The wrapper forwards args/kwargs. + """Wrap *target* to run with the *current* thread's ContextVars and approval/sudo callbacks. Fail-closed: if callback installation raises, the callbacks stay unset - (``None``) — ``prompt_dangerous_approval`` denies dangerous commands with no - callback in an interactive context, and the gateway approval queue blocks - when its notify callback is absent. + (``None``) — ``prompt_dangerous_approval`` then denies dangerous commands + and the gateway approval queue blocks. """ ctx = contextvars.copy_context() parent_approval_cb = parent_sudo_cb = None @@ -86,11 +63,8 @@ def propagate_context_to_thread(target: Callable) -> Callable: if parent_sudo_cb is not None: set_sudo(parent_sudo_cb) except Exception: - logger.debug( - "Failed to install propagated approval/sudo callbacks; " - "dangerous-command approval will fail closed", - exc_info=True, - ) + logger.debug("Failed to install propagated approval/sudo callbacks; " + "dangerous-command approval will fail closed", exc_info=True) try: return target(*args, **kwargs) finally: @@ -98,10 +72,7 @@ def propagate_context_to_thread(target: Callable) -> Callable: set_approval(None) set_sudo(None) except Exception: - logger.debug( - "Failed to clear propagated approval/sudo callbacks", - exc_info=True, - ) + logger.debug("Failed to clear propagated approval/sudo callbacks", exc_info=True) return ctx.run(_inner) From 1b889dbc1e04b171ec6dfe2094ea0eb9d7c2f4b9 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:27:29 -0700 Subject: [PATCH 2/3] refactor(tools): unify worktree probes, fold terminal_scope config-yaml guard, rewrap hint tables, compact docstrings (group I) --- tools/slash_confirm.py | 46 +++++------- tools/subagent_worktree.py | 103 ++++++++++----------------- tools/terminal_hints.py | 142 ++++++++++++++----------------------- tools/terminal_scope.py | 121 ++++++++++++------------------- tools/thread_context.py | 46 +++++------- 5 files changed, 172 insertions(+), 286 deletions(-) diff --git a/tools/slash_confirm.py b/tools/slash_confirm.py index b7bdbe3f3a..a44b9d0dab 100644 --- a/tools/slash_confirm.py +++ b/tools/slash_confirm.py @@ -1,12 +1,11 @@ """Generic slash-command confirmation primitive (gateway-side). -Slash commands with an expensive side effect (currently only ``/reload-mcp``, -which invalidates the provider prompt cache) route through here. Button-UI -adapters render Approve Once / Always Approve / Cancel and call ``resolve()``; -text-only adapters get a prompt and the gateway intercepts ``/approve``, -``/always``, ``/cancel``. State is module-level (like ``tools.approval``) so -adapters can resolve without a ``GatewayRunner`` backreference. The CLI has its -own synchronous variant (``_prompt_slash_confirm`` in ``cli.py``). +Slash commands with an expensive side effect (currently only ``/reload-mcp``, which invalidates +the provider prompt cache) route through here. Button-UI adapters render Approve Once / Always +Approve / Cancel and call ``resolve()``; text-only adapters get a prompt and the gateway +intercepts ``/approve``, ``/always``, ``/cancel``. State is module-level (like ``tools.approval``) +so adapters can resolve without a ``GatewayRunner`` backreference. The CLI has its own +synchronous variant (``_prompt_slash_confirm`` in ``cli.py``). """ from __future__ import annotations @@ -22,17 +21,13 @@ logger = logging.getLogger(__name__) _pending: Dict[str, Dict[str, Any]] = {} _lock = threading.RLock() -# A pending confirm older than this is discarded when the next message arrives -# for the same session (buttons live as long as the adapter keeps callback_data). +# Older pending confirms are discarded when the session's next message arrives (buttons live +# as long as the adapter keeps callback_data). DEFAULT_TIMEOUT_SECONDS = 300 -def register( - session_key: str, - confirm_id: str, - command: str, - handler: Callable[[str], Awaitable[Optional[str]]], -) -> None: +def register(session_key: str, confirm_id: str, command: str, + handler: Callable[[str], Awaitable[Optional[str]]]) -> None: """Register a pending confirm, superseding any prior one for the session.""" with _lock: _pending[session_key] = {"confirm_id": confirm_id, "command": command, @@ -60,29 +55,24 @@ def clear_if_stale(session_key: str, timeout: float = DEFAULT_TIMEOUT_SECONDS) - """Drop the pending confirm if older than ``timeout`` seconds; True if dropped.""" with _lock: entry = _pending.get(session_key) - if entry and _is_stale(entry, timeout): + stale = bool(entry and _is_stale(entry, timeout)) + if stale: _pending.pop(session_key, None) - return True - return False + return stale -async def resolve( - session_key: str, - confirm_id: str, - choice: str, - timeout: float = DEFAULT_TIMEOUT_SECONDS, -) -> Optional[str]: +async def resolve(session_key: str, confirm_id: str, choice: str, + timeout: float = DEFAULT_TIMEOUT_SECONDS) -> Optional[str]: """Run the pending handler with ``choice`` ("once" / "always" / "cancel"). - Returns the handler's output string, or None if the confirm was stale, - already resolved, or the confirm_id doesn't match (superseded prompt). + Returns the handler's output string, or None if the confirm was stale, already resolved, + or the confirm_id doesn't match (superseded prompt). """ with _lock: entry = _pending.get(session_key) if not entry or entry.get("confirm_id") != confirm_id: return None - # Pop before running so duplicate callbacks (button double-click) - # cannot run the handler twice. + # Pop before running so duplicate callbacks (button double-click) cannot run it twice. _pending.pop(session_key, None) if _is_stale(entry, timeout): return None diff --git a/tools/subagent_worktree.py b/tools/subagent_worktree.py index edeb76dbe1..709049441c 100644 --- a/tools/subagent_worktree.py +++ b/tools/subagent_worktree.py @@ -1,14 +1,9 @@ -"""Opt-in git worktree isolation for delegated subagents -(``delegation.worktree_isolation: true``, default false). +"""Opt-in git worktree isolation for delegated subagents (``delegation.worktree_isolation``). -Git-only: in a non-git workspace the setting is ignored and children share -the parent's cwd. One worktree per child, branched from the parent's ``HEAD`` -under ``/.worktrees/subagent-`` on branch ``hermes-subagent/``. -Each result entry reports path, branch, commit count and dirty state; a -worktree is pruned only on affirmative proof (zero commits AND clean tree, both -probes succeeded) — otherwise it is kept with ``inspection_failed`` + ``note``. -Local terminal backend only: on docker/ssh/modal the host worktree is invisible -inside the sandbox, so isolation is skipped rather than half-applied. +Git-only (outside a repo children share the parent's cwd); local terminal backend only (on +docker/ssh/modal the host worktree is invisible in the sandbox, so isolation is skipped). One +worktree per child under ``/.worktrees/subagent-``, branch ``hermes-subagent/``; +pruned only on proof (zero commits AND clean, both probes ok), else kept + ``inspection_failed``. """ from __future__ import annotations @@ -30,9 +25,9 @@ _GIT_TIMEOUT = 30 def _run_git(args, cwd: str, timeout: int = _GIT_TIMEOUT): """Run git capturing output; never raises on non-zero exit. - Runs under :func:`noninteractive_git_env` (GHSA-7x36-8jrh-v4pw): this runs - automatically against whatever repo the parent sits in and ``worktree add`` - runs checkout hooks, so a malicious ``.git/config`` must not execute. + :func:`noninteractive_git_env` (GHSA-7x36-8jrh-v4pw): this runs unattended against whatever + repo the parent sits in and ``worktree add`` runs hooks, so a malicious ``.git/config`` must + not execute. """ return subprocess.run(["git", *harden_git_argv(args)], cwd=cwd, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, @@ -73,15 +68,14 @@ def _ensure_gitignore_entry(repo_root: str) -> None: existing = gitignore.read_text(encoding="utf-8-sig", errors="replace") if gitignore.exists() else "" if ".worktrees/" not in existing.splitlines(): with open(gitignore, "a", encoding="utf-8") as f: - if existing and not existing.endswith("\n"): - f.write("\n") - f.write(".worktrees/\n") + sep = "\n" if existing and not existing.endswith("\n") else "" + f.write(f"{sep}.worktrees/\n") except Exception as exc: logger.debug("subagent worktree: could not update .gitignore: %s", exc) def create_subagent_worktree(parent_cwd: Optional[str], subagent_id: Optional[str] = None) -> Optional[Dict[str, str]]: - """Create an isolated worktree for one child; None (silent downgrade) when not a git repo or on failure.""" + """Create an isolated worktree for one child; None (silent downgrade) if not git / on failure.""" repo_root = resolve_repo_root(parent_cwd) if not repo_root: return None @@ -89,16 +83,9 @@ def create_subagent_worktree(parent_cwd: Optional[str], subagent_id: Optional[st wt_name = f"subagent-{(subagent_id or uuid.uuid4().hex[:8]).replace('/', '-')}" branch = f"hermes-subagent/{wt_name}" wt_path = Path(repo_root) / ".worktrees" / wt_name - try: wt_path.parent.mkdir(parents=True, exist_ok=True) - except Exception as exc: - logger.warning("subagent worktree: cannot create %s: %s", wt_path.parent, exc) - return None - - _ensure_gitignore_entry(repo_root) - - try: + _ensure_gitignore_entry(repo_root) base = _run_git(["rev-parse", "HEAD"], cwd=repo_root) base_commit = base.stdout.strip() if base.returncode == 0 else "" result = _run_git(["worktree", "add", str(wt_path), "-b", branch, "HEAD"], cwd=repo_root) @@ -109,7 +96,6 @@ def create_subagent_worktree(parent_cwd: Optional[str], subagent_id: Optional[st # Common on repos with zero commits (unborn HEAD) — degrade silently. logger.warning("subagent worktree: git worktree add failed: %s", result.stderr.strip()) return None - logger.info("subagent worktree created: %s (branch %s)", wt_path, branch) return {"path": str(wt_path), "branch": branch, "repo_root": repo_root, "base_commit": base_commit} @@ -120,64 +106,51 @@ def _base_payload(info: Dict[str, str]) -> Dict[str, Any]: "commits": 0, "dirty": False, "pruned": False} -def mark_worktree_payload_unproven( - payload: Dict[str, Any], reason: str, *, unmeasured: str = "commits/dirty" -) -> Dict[str, Any]: +def mark_worktree_payload_unproven(payload: Dict[str, Any], reason: str, *, + unmeasured: str = "commits/dirty") -> Dict[str, Any]: """Flag a worktree result payload as un-inspected, in place. - The parent only sees this dict, so the uncertainty must travel in it or - "0 commits, clean" reads as "the child produced nothing". *unmeasured* - names only the fields actually left unproven (one probe can succeed while - the other fails). Shared by ``finalize_subagent_worktree`` and - ``delegate_tool``'s fallback so the two producers cannot drift. + The parent only sees this dict, so the uncertainty must travel in it or "0 commits, clean" + reads as "the child produced nothing". *unmeasured* names only the fields actually left + unproven (one probe can succeed while the other fails). """ - path = payload.get("path", "") - branch = payload.get("branch", "") + path, branch = payload.get("path", ""), payload.get("branch", "") payload["inspection_failed"] = True - payload["note"] = ( - f"git inspection failed ({reason}): {unmeasured} UNKNOWN — not " - "proven zero/clean. The worktree and branch were preserved " - f"— inspect {path} (branch {branch}) before assuming no work." - ) - logger.warning("subagent worktree: git inspection failed (%s) — keeping %s (branch %s) for manual review", - reason, path, branch) + payload["note"] = (f"git inspection failed ({reason}): {unmeasured} UNKNOWN — not proven " + f"zero/clean. The worktree and branch were preserved — inspect {path} " + f"(branch {branch}) before assuming no work.") + logger.warning("subagent worktree: git inspection failed (%s) — keeping %s (branch %s) " + "for manual review", reason, path, branch) return payload def unproven_worktree_payload(info: Dict[str, str], reason: str) -> Dict[str, Any]: - """Complete un-inspected payload for ``delegate_tool`` when ``finalize_subagent_worktree`` raises.""" + """Complete un-inspected payload for ``delegate_tool`` when finalize raises.""" return mark_worktree_payload_unproven(_base_payload(info), reason) def finalize_subagent_worktree(info: Dict[str, str], *, prune: bool = True) -> Dict[str, Any]: """Inspect (and possibly prune) a child worktree after the child finishes. - Prunes only when *prune*, commits==0, clean tree AND both git probes - succeeded. If a probe exits non-zero or raises the worktree/branch are kept - and the payload carries ``inspection_failed`` + ``note``; ``commits`` / - ``dirty`` are then defaults, NOT measurements. + Prunes only when *prune*, commits==0, clean tree AND both git probes succeeded; otherwise + keeps it with ``inspection_failed`` + ``note`` (``commits``/``dirty`` then are defaults). """ - path = info.get("path", "") - branch = info.get("branch", "") + path, branch = info.get("path", ""), info.get("branch", "") base_commit = info.get("base_commit", "") payload = _base_payload(info) if not path or not os.path.isdir(path): payload["pruned"] = True # nothing on disk to review return payload - # Without a base commit the count is an unproven default, and the prune # condition reads payload["commits"] — so it must not prune either. if not base_commit: return mark_worktree_payload_unproven( - payload, "no base_commit recorded — commit count unmeasurable", unmeasured="commits" - ) + payload, "no base_commit recorded — commit count unmeasurable", unmeasured="commits") - failed: list = [] - unmeasured: list = [] - probes = ( - ("commits", "rev-list", ["rev-list", "--count", f"{base_commit}..HEAD"], lambda out: int(out or 0)), - ("dirty", "status", ["status", "--porcelain"], bool), - ) + failed, unmeasured = [], [] + probes = (("commits", "rev-list", ["rev-list", "--count", f"{base_commit}..HEAD"], + lambda s: int(s or 0)), + ("dirty", "status", ["status", "--porcelain"], bool)) try: for field, label, args, parse in probes: res = _run_git(args, cwd=path) @@ -190,7 +163,6 @@ def finalize_subagent_worktree(info: Dict[str, str], *, prune: bool = True) -> D # Timeout, OSError or non-numeric rev-list stdout: which probe raised is # unknowable, so neither value is trustworthy — keep the worktree. return mark_worktree_payload_unproven(payload, f"inspection raised: {exc}") - if failed: # Destructive cleanup requires affirmative proof; defaults prove nothing. return mark_worktree_payload_unproven(payload, "; ".join(failed), unmeasured="/".join(unmeasured)) @@ -207,7 +179,6 @@ def finalize_subagent_worktree(info: Dict[str, str], *, prune: bool = True) -> D logger.debug("subagent worktree: prune failed: %s", removed.stderr.strip()) except Exception as exc: logger.debug("subagent worktree: prune failed: %s", exc) - return payload @@ -217,10 +188,8 @@ def build_worktree_context_note(info: Dict[str, str]) -> str: "\n\n[WORKTREE ISOLATION] You are working in an isolated git worktree " f"at: {info.get('path')}\n" f"Your dedicated branch is: {info.get('branch')}\n" - "All file edits and shell commands must happen inside this worktree " - "directory (your terminal already starts there). Do NOT cd to the " - "main repository checkout. Commit your changes to your branch when " - "done; the parent agent will review and merge your branch. If you " - "make no commits and leave the tree clean, the worktree is discarded " - "automatically." + "All file edits and shell commands must happen inside this worktree directory (your " + "terminal already starts there). Do NOT cd to the main repository checkout. Commit your " + "changes to your branch when done; the parent agent will review and merge your branch. If " + "you make no commits and leave the tree clean, the worktree is discarded automatically." ) diff --git a/tools/terminal_hints.py b/tools/terminal_hints.py index 071808fad4..396838ef2f 100644 --- a/tools/terminal_hints.py +++ b/tools/terminal_hints.py @@ -1,11 +1,10 @@ """Output-pattern failure hints for the terminal tool. -Extends the exit-code semantics table in ``terminal_tool`` with a bounded scan -of failed-command output mapped to ONE short, actionable recovery hint. -Rules: only fires on non-zero exit; first match wins, patterns ordered by -observed production frequency; scans only the first ``_SCAN_CHARS`` so hints -key on error headers, not deep context; hints state the *next action* in 1-2 -sentences; pure function, no I/O or config reads. +Extends the exit-code semantics table in ``terminal_tool`` with a bounded scan of failed-command +output mapped to ONE short, actionable recovery hint. Rules: only fires on non-zero exit; first +match wins, patterns ordered by observed production frequency; scans only the first +``_SCAN_CHARS`` so hints key on error headers, not deep context; hints state the *next action* +in 1-2 sentences; pure function, no I/O or config reads. """ from __future__ import annotations @@ -17,85 +16,59 @@ _SCAN_CHARS = 4000 def _regex_hint(pattern: str, message: str | Callable[[str], str], flags: int = 0) -> Callable[[str, str], Optional[str]]: - """Hint firing when ``pattern`` matches; ``{0}`` = first group, or ``message(group1)`` if callable.""" + """Hint firing when ``pattern`` matches; ``{0}`` = first group, or ``message(group1)``.""" rx = re.compile(pattern, flags) def hint(command: str, output: str) -> Optional[str]: m = rx.search(output) - if not m: - return None - return message(m.group(1)) if callable(message) else message.format(*m.groups()) + if m: + return message(m.group(1)) if callable(message) else message.format(*m.groups()) + return None return hint # Most `command not found` hits are bare `python` on python3-only distros. _MISSING_COMMAND_HINTS = { - "python": ( - "This system has no bare `python` — use `python3`, or the " - "project venv's interpreter (e.g. .venv/bin/python)." - ), - "pip": ( - "This system has no bare `pip` — use `pip3`, `python3 -m pip`, " - "or the project venv's pip (e.g. .venv/bin/pip)." - ), + "python": "This system has no bare `python` — use `python3`, or the project venv's " + "interpreter (e.g. .venv/bin/python).", + "pip": "This system has no bare `pip` — use `pip3`, `python3 -m pip`, or the project venv's " + "pip (e.g. .venv/bin/pip).", } def _missing_command_hint(missing: str) -> str: return _MISSING_COMMAND_HINTS.get(missing) or ( - f"`{missing}` is not installed or not on PATH. Verify with " - f"`which {missing}`; install it or use an absolute path instead of " - "retrying the same command." - ) + f"`{missing}` is not installed or not on PATH. Verify with `which {missing}`; install it " + "or use an absolute path instead of retrying the same command.") # Ordered by production frequency — first match wins. _OUTPUT_HINTS: list[Callable[[str, str], Optional[str]]] = [ # gh version drift; gh already prints the valid field list. - _regex_hint( - r'Unknown JSON field: "?(\w+)', - "The installed gh does not support the JSON field '{0}'. " - "The valid field list is printed in the output above — retry using " - "only fields from that list.", - ), - _regex_hint( - r"^CONFLICT |Automatic merge failed|needs merge", - "Git merge conflict. Do not retry this command. Resolve the " - "conflicted files listed above (edit, then `git add`), then continue " - "(`git rebase --continue` / commit the merge) — or abort with " - "`--abort`.", - re.M, - ), - _regex_hint( - r"(?:bash: line \d+: |bash: |sh: \d*:? ?)?([\w.+-]+): command not found", - _missing_command_hint, - ), + _regex_hint(r'Unknown JSON field: "?(\w+)', + "The installed gh does not support the JSON field '{0}'. The valid field list is " + "printed in the output above — retry using only fields from that list."), + _regex_hint(r"^CONFLICT |Automatic merge failed|needs merge", + "Git merge conflict. Do not retry this command. Resolve the conflicted files " + "listed above (edit, then `git add`), then continue (`git rebase --continue` / " + "commit the merge) — or abort with `--abort`.", re.M), + _regex_hint(r"(?:bash: line \d+: |bash: |sh: \d*:? ?)?([\w.+-]+): command not found", + _missing_command_hint), # Almost always a venv-activation slip, not a missing dependency. - _regex_hint( - r"(?:ModuleNotFoundError|ImportError): No module named '?([\w.]+)", - "Python cannot import '{0}'. Most often the wrong " - "interpreter is running: activate the project venv (e.g. `source " - ".venv/bin/activate`) or invoke its python directly. Only pip " - "install if the package is genuinely absent from that venv.", - ), - _regex_hint( - r"(?:fatal|error):.*?'([^']+)' already exists", - "'{0}' already exists — retrying unchanged will keep " - "failing. Reuse it, choose another name, or delete it first if it is " - "genuinely stale.", - ), - _regex_hint( - r"API rate limit|was submitted too quickly", - "GitHub API rate limit hit — immediate retries will keep failing. " - "Continue with other work and retry this operation later.", - ), - _regex_hint( - r"Permission denied|EACCES", - "Permission denied. Check ownership/mode of the target path " - "(`ls -la`); prefer a user-writable location. Only escalate to sudo " - "if the task genuinely requires it.", - ), + _regex_hint(r"(?:ModuleNotFoundError|ImportError): No module named '?([\w.]+)", + "Python cannot import '{0}'. Most often the wrong interpreter is running: " + "activate the project venv (e.g. `source .venv/bin/activate`) or invoke its python " + "directly. Only pip install if the package is genuinely absent from that venv."), + _regex_hint(r"(?:fatal|error):.*?'([^']+)' already exists", + "'{0}' already exists — retrying unchanged will keep failing. Reuse it, choose " + "another name, or delete it first if it is genuinely stale."), + _regex_hint(r"API rate limit|was submitted too quickly", + "GitHub API rate limit hit — immediate retries will keep failing. Continue with " + "other work and retry this operation later."), + _regex_hint(r"Permission denied|EACCES", + "Permission denied. Check ownership/mode of the target path (`ls -la`); prefer a " + "user-writable location. Only escalate to sudo if the task genuinely requires it."), ] # Exit-code-only hints for codes the terminal_tool semantics table does not @@ -120,30 +93,22 @@ _PASSTHROUGH_CONSUMERS = r"(?:tail|head|cat|tee|less|more|wc|sort|uniq)" # Command shapes that swallow an upstream status -> warning, checked in order. _MASKING_SHAPES: list[tuple[re.Pattern[str], str]] = [ # Top-level `... | tail -20` (not `||`); consumer must be the LAST segment. - ( - re.compile(r"(? str: """Basename of the command head, skipping leading env-var assignments.""" for tok in (command or "").strip().split(): - if "=" in tok and not tok.startswith(("=", "./", "/")): - continue - return tok.rsplit("/", 1)[-1] + if "=" not in tok or tok.startswith(("=", "./", "/")): + return tok.rsplit("/", 1)[-1] return "" @@ -177,7 +140,8 @@ def annotate_masked_success(command: str, output: str) -> Optional[str]: """Warning note when an exit-0 result likely masks a failure (caller gates on exit 0).""" cmd = command or "" window = (output or "")[:_SCAN_CHARS] - if not cmd or not window or _first_token(cmd) in _READONLY_HEADS or not _FAILURE_SHAPES.search(window): + if (not cmd or not window or _first_token(cmd) in _READONLY_HEADS + or not _FAILURE_SHAPES.search(window)): return None return next((note for rx, note in _MASKING_SHAPES if rx.search(cmd)), None) diff --git a/tools/terminal_scope.py b/tools/terminal_scope.py index 1cd8807b57..a5dc6e4f08 100644 --- a/tools/terminal_scope.py +++ b/tools/terminal_scope.py @@ -1,13 +1,11 @@ """Per-turn terminal scope: profile-scoped TERMINAL_* policy. -Multiplexed surfaces (gateway, dashboard/TUI, cron) serve several profiles from -one process; mirroring terminal settings into ``os.environ`` let the first -profile pin its backend onto everyone else (sandbox escape). Like -``agent/secret_scope.py``, a ContextVar holds the active profile's COMPLETE -effective ``TERMINAL_*`` policy. While bound, ``terminal_env`` resolves ONLY -from it (omitted keys -> defined default, never ambient env). If the policy -cannot be resolved a *refusal* scope is installed and terminal execution raises -:class:`TerminalPolicyUnavailable` instead of falling back to ambient authority. +Multiplexed surfaces (gateway, dashboard/TUI, cron) serve several profiles from one process; +mirroring terminal settings into ``os.environ`` let the first profile pin its backend onto +everyone else (sandbox escape). Like ``agent/secret_scope.py``, a ContextVar holds the active +profile's COMPLETE ``TERMINAL_*`` policy; while bound, ``terminal_env`` resolves ONLY from it +(omitted keys -> defined default, never ambient env). If the policy cannot be resolved a +*refusal* scope is installed and terminal execution raises :class:`TerminalPolicyUnavailable`. """ from __future__ import annotations @@ -21,24 +19,15 @@ from typing import Any, Dict, Iterator, Optional logger = logging.getLogger(__name__) -# None = no scope bound (process-env behavior); dict = active profile's complete -# policy; TerminalPolicyRefusal = resolution failed. +# None = no scope bound (process-env behavior); dict = complete policy; Refusal = resolution failed. _terminal_scope_var: ContextVar = ContextVar("hermes_terminal_scope", default=None) -# Terminal keys whose config default lives in terminal_tool.py rather than -# DEFAULT_CONFIG (DEFAULT_CONFIG wins on overlap). Without them the projection -# is not total. +# Keys whose default lives in terminal_tool.py, not DEFAULT_CONFIG (which wins on overlap); +# without them the projection is not total. _TOOL_LEVEL_DEFAULTS: Dict[str, Any] = { - "cwd": ".", - "ssh_host": "", - "ssh_user": "", - "ssh_port": 22, - "ssh_key": "", - "docker_orphan_reaper": True, - "docker_persist_across_processes": True, - "sandbox_dir": "", - "lifetime_seconds": 300, - "docker_shared_container_key": "", + "cwd": ".", "ssh_host": "", "ssh_user": "", "ssh_port": 22, "ssh_key": "", + "docker_orphan_reaper": True, "docker_persist_across_processes": True, + "sandbox_dir": "", "lifetime_seconds": 300, "docker_shared_container_key": "", "home_mode": "auto", } @@ -60,11 +49,6 @@ def set_terminal_scope(mapping: Optional[Dict[str, str]]) -> Token: return _terminal_scope_var.set(mapping) -def install_refusal_scope(reason: str) -> Token: - """Install a refusal scope; terminal execution under it is rejected.""" - return _terminal_scope_var.set(TerminalPolicyRefusal(reason)) - - def reset_terminal_scope(token: Token) -> None: _terminal_scope_var.reset(token) @@ -84,8 +68,8 @@ def enforce_no_refusal() -> None: def terminal_env(name: str, default: str = "") -> str: """Authoritative read of a ``TERMINAL_*`` variable. - No scope: process env, then *default*. Refusal scope: raise. Policy scope: - ONLY the policy; a missing key yields *default*, never os.environ. + No scope: process env, then *default*. Refusal scope: raise. Policy scope: ONLY the + policy; a missing key yields *default*, never os.environ. """ scope = _terminal_scope_var.get() if scope is None: @@ -98,69 +82,56 @@ def terminal_env(name: str, default: str = "") -> str: def build_profile_terminal_scope(hermes_home: "Any") -> Dict[str, str]: """Build the COMPLETE effective ``TERMINAL_*`` policy for a profile home. - Projection: ``DEFAULT_CONFIG['terminal']`` <- profile ``.env`` TERMINAL_* <- - profile ``config.yaml`` ``terminal:`` keys. Total by construction, so a bound - scope never widens back to ambient authority. Raises - :class:`TerminalPolicyUnavailable` when either file exists but cannot be - read/parsed. + Projection: ``DEFAULT_CONFIG['terminal']`` <- profile ``.env`` TERMINAL_* <- profile + ``config.yaml`` ``terminal:``. Total by construction, so a bound scope never widens back to + ambient authority. Raises :class:`TerminalPolicyUnavailable` if a present file is unreadable. """ - home = Path(hermes_home) - from hermes_cli.config import TERMINAL_CONFIG_ENV_MAP from hermes_cli.config_defaults import DEFAULT_CONFIG - defaults = DEFAULT_CONFIG.get("terminal") if isinstance(DEFAULT_CONFIG, dict) else None - defaults = {**_TOOL_LEVEL_DEFAULTS, **(defaults if isinstance(defaults, dict) else {})} - + home = Path(hermes_home) scope: Dict[str, str] = {} - def _apply(cfg_key: str, value: Any) -> None: - # cwd placeholders are resolved per-surface later; not a policy value. - if value is None or (cfg_key == "cwd" and str(value).strip() in {".", "auto", "cwd"}): - return - env_var = TERMINAL_CONFIG_ENV_MAP.get(cfg_key) - if env_var: - scope[env_var] = str(value) - - for cfg_key, value in defaults.items(): - _apply(cfg_key, value) + def _apply(mapping: Dict[str, Any]) -> None: + for cfg_key, value in mapping.items(): + # cwd placeholders are resolved per-surface later; not a policy value. + if value is None or (cfg_key == "cwd" and str(value).strip() in {".", "auto", "cwd"}): + continue + env_var = TERMINAL_CONFIG_ENV_MAP.get(cfg_key) + if env_var: + scope[env_var] = str(value) + _apply({**_TOOL_LEVEL_DEFAULTS, **(DEFAULT_CONFIG.get("terminal") or {})}) env_path = home / ".env" if env_path.exists(): - # load_env_file swallows OSError by design (secret scope fails soft); - # an unreadable profile .env must fail closed here. + # load_env_file swallows OSError by design (secret scope fails soft); an unreadable + # profile .env must fail closed here. try: env_path.read_bytes() except Exception as exc: raise TerminalPolicyUnavailable(f"cannot read {env_path}: {exc}") from exc from agent.secret_scope import load_env_file - for key, value in load_env_file(env_path).items(): - if key.startswith("TERMINAL_"): - scope[key] = str(value) - - # The profile's own config.yaml is read directly (not read_raw_config(): it - # collapses "missing" and "unparseable" into {}); present-but-unparseable - # fails closed. + scope.update((k, str(v)) for k, v in load_env_file(env_path).items() + if k.startswith("TERMINAL_")) + # Read config.yaml directly, not via read_raw_config() (which collapses "missing" and + # "unparseable" into {}): present-but-unparseable must fail closed. + config_path = home / "config.yaml" try: - config_path = home / "config.yaml" - if config_path.exists(): - from hermes_cli.config import fast_safe_load - - try: - with open(config_path, encoding="utf-8") as f: - raw = fast_safe_load(f) - except Exception as exc: - raise TerminalPolicyUnavailable(f"cannot parse {config_path}: {exc}") from exc - raw_terminal = raw.get("terminal") if isinstance(raw, dict) else None - if isinstance(raw_terminal, dict): - for cfg_key, value in raw_terminal.items(): - _apply(cfg_key, value) - except TerminalPolicyUnavailable: - raise + config_exists = config_path.exists() except Exception as exc: raise TerminalPolicyUnavailable(f"cannot resolve terminal config in {home}: {exc}") from exc + if config_exists: + from hermes_cli.config import fast_safe_load + try: + with open(config_path, encoding="utf-8") as f: + raw = fast_safe_load(f) + except Exception as exc: + raise TerminalPolicyUnavailable(f"cannot parse {config_path}: {exc}") from exc + raw_terminal = raw.get("terminal") if isinstance(raw, dict) else None + if isinstance(raw_terminal, dict): + _apply(raw_terminal) return scope @@ -170,7 +141,7 @@ def install_profile_terminal_scope(hermes_home: "Any") -> Token: return set_terminal_scope(build_profile_terminal_scope(hermes_home)) except TerminalPolicyUnavailable as exc: logger.warning("terminal policy unavailable: %s", exc) - return install_refusal_scope(str(exc)) + return _terminal_scope_var.set(TerminalPolicyRefusal(str(exc))) @contextmanager diff --git a/tools/thread_context.py b/tools/thread_context.py index b48867fa98..9e5ae6a7a3 100644 --- a/tools/thread_context.py +++ b/tools/thread_context.py @@ -1,13 +1,12 @@ """Propagate agent-turn context into worker threads that dispatch Hermes tools. -A bare ``threading.Thread`` / ``ThreadPoolExecutor`` worker starts with an -empty ``contextvars.Context`` and no thread-local approval/sudo callbacks, so -tool dispatch inside it silently loses the approval ContextVars (gateway -sessions then auto-approve dangerous commands) and the CLI approval/sudo -callbacks (``prompt_dangerous_approval`` cannot reach the user, -GHSA-qg5c-hvr5-hjgr). Call :func:`propagate_context_to_thread` **on the parent -thread** (it snapshots at call time) and use the result as the worker target. -Callbacks are installed for the worker's lifetime and always cleared on exit. +A bare ``threading.Thread`` / ``ThreadPoolExecutor`` worker starts with an empty +``contextvars.Context`` and no thread-local approval/sudo callbacks, so tool dispatch inside it +silently loses the approval ContextVars (gateway sessions then auto-approve dangerous commands) +and the CLI callbacks (``prompt_dangerous_approval`` cannot reach the user, GHSA-qg5c-hvr5-hjgr). +Call :func:`propagate_context_to_thread` **on the parent thread** (it snapshots at call time) and +use the result as the worker target; callbacks are installed for the worker's lifetime and +always cleared on exit. """ from __future__ import annotations @@ -20,26 +19,19 @@ logger = logging.getLogger(__name__) def _callback_api(): - """Resolve the terminal_tool callback getters/setters. + """Resolve the terminal_tool callback getters/setters (lazy: terminal_tool imports + tools.approval at load, so a top-level import risks a cycle for tools.approval callers).""" + from tools import terminal_tool as tt - Lazy: ``tools.terminal_tool`` imports ``tools.approval`` at module load, so a - top-level import would risk a cycle for callers in ``tools.approval``. - """ - from tools.terminal_tool import ( - _get_approval_callback, - _get_sudo_password_callback, - set_approval_callback, - set_sudo_password_callback, - ) - return (_get_approval_callback, _get_sudo_password_callback, set_approval_callback, set_sudo_password_callback) + return (tt._get_approval_callback, tt._get_sudo_password_callback, + tt.set_approval_callback, tt.set_sudo_password_callback) def propagate_context_to_thread(target: Callable) -> Callable: """Wrap *target* to run with the *current* thread's ContextVars and approval/sudo callbacks. - Fail-closed: if callback installation raises, the callbacks stay unset - (``None``) — ``prompt_dangerous_approval`` then denies dangerous commands - and the gateway approval queue blocks. + Fail-closed: if callback installation raises they stay ``None`` — dangerous commands are then + denied by ``prompt_dangerous_approval`` and the gateway approval queue blocks. """ ctx = contextvars.copy_context() parent_approval_cb = parent_sudo_cb = None @@ -58,10 +50,9 @@ def propagate_context_to_thread(target: Callable) -> Callable: return target(*args, **kwargs) set_approval, set_sudo = setters try: - if parent_approval_cb is not None: - set_approval(parent_approval_cb) - if parent_sudo_cb is not None: - set_sudo(parent_sudo_cb) + for setter, cb in ((set_approval, parent_approval_cb), (set_sudo, parent_sudo_cb)): + if cb is not None: + setter(cb) except Exception: logger.debug("Failed to install propagated approval/sudo callbacks; " "dangerous-command approval will fail closed", exc_info=True) @@ -72,7 +63,8 @@ def propagate_context_to_thread(target: Callable) -> Callable: set_approval(None) set_sudo(None) except Exception: - logger.debug("Failed to clear propagated approval/sudo callbacks", exc_info=True) + logger.debug("Failed to clear propagated approval/sudo callbacks", + exc_info=True) return ctx.run(_inner) From 3149ad5ef7da095b84a0bf21825ac51dbbdb5fd8 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:37:39 -0700 Subject: [PATCH 3/3] refactor(tools): fold thread_context callback pairs, resolve_repo_root guard, final compaction (group I: 885->662 LOC) --- tools/subagent_worktree.py | 11 +++-------- tools/terminal_hints.py | 5 ++--- tools/terminal_scope.py | 3 ++- tools/thread_context.py | 17 +++++++---------- 4 files changed, 14 insertions(+), 22 deletions(-) diff --git a/tools/subagent_worktree.py b/tools/subagent_worktree.py index 709049441c..be9ce84de4 100644 --- a/tools/subagent_worktree.py +++ b/tools/subagent_worktree.py @@ -48,12 +48,10 @@ def local_backend_active() -> bool: def resolve_repo_root(path: Optional[str]) -> Optional[str]: """Return the git toplevel for *path*, or None when not in a work tree.""" - if not path: + candidate = os.path.abspath(os.path.expanduser(str(path))) if path else "" + if not candidate or not os.path.isdir(candidate): return None try: - candidate = os.path.abspath(os.path.expanduser(str(path))) - if not os.path.isdir(candidate): - return None result = _run_git(["rev-parse", "--show-toplevel"], cwd=candidate) except Exception as exc: logger.debug("subagent worktree: rev-parse failed: %s", exc) @@ -75,11 +73,10 @@ def _ensure_gitignore_entry(repo_root: str) -> None: def create_subagent_worktree(parent_cwd: Optional[str], subagent_id: Optional[str] = None) -> Optional[Dict[str, str]]: - """Create an isolated worktree for one child; None (silent downgrade) if not git / on failure.""" + """Create an isolated worktree for one child; None (silent downgrade) outside git/on failure.""" repo_root = resolve_repo_root(parent_cwd) if not repo_root: return None - wt_name = f"subagent-{(subagent_id or uuid.uuid4().hex[:8]).replace('/', '-')}" branch = f"hermes-subagent/{wt_name}" wt_path = Path(repo_root) / ".worktrees" / wt_name @@ -146,7 +143,6 @@ def finalize_subagent_worktree(info: Dict[str, str], *, prune: bool = True) -> D if not base_commit: return mark_worktree_payload_unproven( payload, "no base_commit recorded — commit count unmeasurable", unmeasured="commits") - failed, unmeasured = [], [] probes = (("commits", "rev-list", ["rev-list", "--count", f"{base_commit}..HEAD"], lambda s: int(s or 0)), @@ -166,7 +162,6 @@ def finalize_subagent_worktree(info: Dict[str, str], *, prune: bool = True) -> D if failed: # Destructive cleanup requires affirmative proof; defaults prove nothing. return mark_worktree_payload_unproven(payload, "; ".join(failed), unmeasured="/".join(unmeasured)) - if prune and payload["commits"] == 0 and not payload["dirty"]: cwd = info.get("repo_root", "") or path try: diff --git a/tools/terminal_hints.py b/tools/terminal_hints.py index 396838ef2f..fe155df040 100644 --- a/tools/terminal_hints.py +++ b/tools/terminal_hints.py @@ -153,9 +153,8 @@ def annotate_failure(command: str, exit_code: int, output: str) -> Optional[str] window = (output or "")[:_SCAN_CHARS] for fn in _OUTPUT_HINTS if window else (): try: - hint = fn(command or "", window) + if hint := fn(command or "", window): + return hint except Exception: continue - if hint: - return hint return _EXIT_CODE_HINTS.get(exit_code) diff --git a/tools/terminal_scope.py b/tools/terminal_scope.py index a5dc6e4f08..1d63081893 100644 --- a/tools/terminal_scope.py +++ b/tools/terminal_scope.py @@ -62,7 +62,8 @@ def enforce_no_refusal() -> None: """Raise when the active scope is a refusal scope (fail closed).""" scope = _terminal_scope_var.get() if isinstance(scope, TerminalPolicyRefusal): - raise TerminalPolicyUnavailable(f"terminal policy unavailable for this profile: {scope.reason}") + raise TerminalPolicyUnavailable( + f"terminal policy unavailable for this profile: {scope.reason}") def terminal_env(name: str, default: str = "") -> str: diff --git a/tools/thread_context.py b/tools/thread_context.py index 9e5ae6a7a3..d8c37af8f5 100644 --- a/tools/thread_context.py +++ b/tools/thread_context.py @@ -34,23 +34,20 @@ def propagate_context_to_thread(target: Callable) -> Callable: denied by ``prompt_dangerous_approval`` and the gateway approval queue blocks. """ ctx = contextvars.copy_context() - parent_approval_cb = parent_sudo_cb = None - setters = None + # (setter, parent callback) pairs; None when the callback API could not be captured. + installs = None try: get_approval, get_sudo, set_approval, set_sudo = _callback_api() - parent_approval_cb = get_approval() - parent_sudo_cb = get_sudo() - setters = (set_approval, set_sudo) + installs = ((set_approval, get_approval()), (set_sudo, get_sudo())) except Exception: logger.debug("Could not capture parent approval/sudo callbacks", exc_info=True) def _runner(*args, **kwargs): def _inner(): - if setters is None: + if installs is None: return target(*args, **kwargs) - set_approval, set_sudo = setters try: - for setter, cb in ((set_approval, parent_approval_cb), (set_sudo, parent_sudo_cb)): + for setter, cb in installs: if cb is not None: setter(cb) except Exception: @@ -60,8 +57,8 @@ def propagate_context_to_thread(target: Callable) -> Callable: return target(*args, **kwargs) finally: try: - set_approval(None) - set_sudo(None) + for setter, _cb in installs: + setter(None) except Exception: logger.debug("Failed to clear propagated approval/sudo callbacks", exc_info=True)