From 4a37db2269dc092a3ca2ad35d965ea22339b2b33 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:51:25 -0700 Subject: [PATCH] refactor(approval): compact leaf docstrings/comments by hand, keep every invariant (1375->1310) --- tools/approval_context.py | 57 ++++++++------------ tools/approval_floors.py | 64 ++++++++++------------ tools/approval_gateway_wait.py | 45 +++++++--------- tools/approval_human_wait.py | 97 +++++++++++++++------------------ tools/approval_prompt.py | 98 +++++++++++++++------------------- tools/approval_smart.py | 26 ++++----- 6 files changed, 161 insertions(+), 226 deletions(-) diff --git a/tools/approval_context.py b/tools/approval_context.py index 52febefabc..5f778bbee4 100644 --- a/tools/approval_context.py +++ b/tools/approval_context.py @@ -2,8 +2,7 @@ Session identity and observability contextvars, the interactive/gateway/cron/ unattended predicates, and the ``approvals.*`` config readers used by every -gate in :mod:`tools.approval` (which re-exports all of them). No approval -state or prompting lives here. +gate in :mod:`tools.approval` (which re-exports all of them). """ import contextvars @@ -19,23 +18,22 @@ def _ctx(name: str, default: "str | None" = "") -> contextvars.ContextVar: return contextvars.ContextVar(name, default=default) -# Per-thread/per-task gateway session identity. Gateway runs agent turns -# concurrently in executor threads, so a process-global env var is racy; the -# env fallback stays for legacy single-threaded callers. +# Per-thread/per-task gateway session identity: gateway runs agent turns +# concurrently in executor threads, so a process-global env var is racy (the +# env fallback stays for legacy single-threaded callers). _approval_session_key: contextvars.ContextVar[str] = _ctx("approval_session_key") _approval_turn_id: contextvars.ContextVar[str] = _ctx("approval_turn_id") _approval_tool_call_id: contextvars.ContextVar[str] = _ctx("approval_tool_call_id") # Hermes session id (observability identity, distinct from the gateway routing -# session_key). Forwarded to approval hooks so observer plugins attach marks to -# the REAL session scope — without it they fall back to a synthetic "default" +# session_key), forwarded to approval hooks so observer plugins attach marks to +# the REAL session scope — otherwise they fall back to a synthetic "default" # session whose scope never closes, so close-time exporters never ship them. _approval_session_id: contextvars.ContextVar[str] = _ctx("approval_session_id") # Interactive-CLI flag. Concurrent ACP sessions share a ThreadPoolExecutor, so # mutating os.environ["HERMES_INTERACTIVE"] races: one session's `finally` # restore can clobber another's set mid-run, dropping it onto the # non-interactive auto-approve path so a dangerous command runs without the -# approval callback firing (GHSA-96vc-wcxf-jjff). None = unset → fall back to -# the env var for legacy single-threaded CLI callers. +# approval callback firing (GHSA-96vc-wcxf-jjff). None = unset → env fallback. _hermes_interactive_ctx: contextvars.ContextVar[str | None] = _ctx("hermes_interactive", None) @@ -75,8 +73,7 @@ def _fire_approval_hook(hook_name: str, **kwargs) -> None: kwargs.setdefault("session_id", _approval_session_id.get()) invoke_hook(hook_name, **kwargs) except Exception as exc: - # invoke_hook() swallows per-callback errors; reaching here means the - # dispatch layer itself failed. + # invoke_hook() swallows per-callback errors; this is the dispatch layer itself failing. logger.debug("Approval hook %s dispatch failed: %s", hook_name, exc) @@ -168,8 +165,7 @@ def _is_gateway_approval_context() -> bool: HERMES_SESSION_PLATFORM via contextvars. Cron is NEVER a gateway approval context even when it originated from a platform (cron binds the platform for delivery routing): falling through would submit a pending approval with no - listener and block the job indefinitely. Unattended platforms are excluded - for the same reason. + listener and block the job indefinitely; unattended platforms likewise. """ from tools import approval as _a if _a._is_cron_approval_context() or _is_unattended_platform_approval_context(): @@ -189,13 +185,11 @@ def _resolve_cli_approval_callback(approval_callback=None): def _should_fall_through_to_cli_approval(*, is_cli: bool, approval_callback, notify_cb) -> bool: - """Prefer the CLI Dangerous Command panel over a silent pending approval. - + """Prefer the CLI Dangerous Command panel over a silent pending approval: ``HERMES_EXEC_ASK`` (or a platform marker) can leak into an interactive CLI - process — historically via ``import gateway.run``. Without a gateway notify + process (historically via ``import gateway.run``), and without a gateway notify listener the ask branch used to return ``pending_approval`` immediately and - skip the panel the user can actually answer. - """ + skip the panel the user can actually answer.""" return bool(is_cli and approval_callback is not None and notify_cb is None) @@ -203,12 +197,10 @@ _VALID_MODES = ("manual", "smart", "off") def _normalize_approval_mode(mode) -> str: - """Normalize approval mode values loaded from YAML/config. - - YAML 1.1 parses a bare ``off`` as False, so ``mode: off`` arrives as a bool; - treat it as the intended string mode. Unknown strings (e.g. 'auto') warn and - fall back to 'manual' instead of silently failing every mode check. - """ + """Normalize approval mode values loaded from YAML/config. YAML 1.1 parses a + bare ``off`` as False, so ``mode: off`` arrives as a bool; treat it as the + intended string mode. Unknown strings (e.g. 'auto') warn and fall back to + 'manual' instead of silently failing every mode check.""" if isinstance(mode, bool): return "off" if mode is False else "manual" if isinstance(mode, str): @@ -222,11 +214,8 @@ def _normalize_approval_mode(mode) -> str: def _get_approval_config() -> dict: - """Read the approvals config block. - - Returns the LIVE config-cache sub-dict (load_config_readonly contract) — - callers must not mutate it or any nested structure. - """ + """Read the approvals config block: the LIVE config-cache sub-dict + (load_config_readonly contract) — callers must not mutate it or any nested structure.""" try: from hermes_cli.config import load_config_readonly return load_config_readonly().get("approvals", {}) or {} @@ -251,12 +240,10 @@ def _get_approval_mode() -> str: def _get_approval_timeout() -> int: """Read ``approvals.timeout`` (default 300s: gateway push notifications may not be seen for minutes; 60s failed closed before Telegram taps landed). - Clamped to ``agent.deadline.MAX_SAFE_TIMEOUT_S`` (~1 year): a larger value overflows ``time_t`` inside ``Thread.join`` / ``Lock.acquire`` on macOS and - crashed every parallel tool batch. Clamping at the single config-read site - keeps every consumer platform-safe at once. - """ + crashed every parallel tool batch; clamping at the single config-read site + keeps every consumer platform-safe at once.""" from tools import approval as _a try: raw = int(_a._get_approval_config().get("timeout", 300)) @@ -304,10 +291,8 @@ def _get_unattended_approval_mode() -> str: def _tirith_fail_open() -> bool: """``security.tirith_fail_open`` (default True; True when config is unreadable). - False means the operator opted into fail-closed: an un-importable scanner - must not silently grant access. - """ + must not silently grant access.""" try: from hermes_cli.config import load_config_readonly as _load_cfg _sec = (_load_cfg() or {}).get("security", {}) or {} diff --git a/tools/approval_floors.py b/tools/approval_floors.py index a25b0295ab..a22605ea7c 100644 --- a/tools/approval_floors.py +++ b/tools/approval_floors.py @@ -18,14 +18,12 @@ logger = logging.getLogger("tools.approval") def _match_user_deny_rule(command: str) -> str | None: - """Return the matching ``approvals.deny`` glob, or None. - - User-defined fnmatch globs that block unconditionally — like the hardline - floor, a match fires BEFORE the yolo / mode=off bypass ("never let the - agent run this, even under yolo"). Case-insensitive, run over the same - normalized/deobfuscated variants the dangerous-pattern detector uses so - quoting tricks (``r\\m``, ``git st""atus``) can't sidestep a rule. - """ + """Return the matching ``approvals.deny`` glob, or None. User-defined fnmatch + globs that block unconditionally — like the hardline floor, a match fires + BEFORE the yolo / mode=off bypass ("never let the agent run this, even under + yolo"). Case-insensitive, run over the same normalized/deobfuscated variants + the dangerous-pattern detector uses so quoting tricks (``r\\m``, + ``git st""atus``) can't sidestep a rule.""" from tools import approval as _a try: deny_patterns = _a._get_approval_config().get("deny") or [] @@ -58,15 +56,13 @@ def _user_deny_block_result(pattern: str) -> dict: def _save_blocked_payload(command: str) -> str | None: - """Persist a parser-limit-blocked command as a runnable script. - - The parser-limit block fires on payload SIZE/shape, not the operation — - usually a legitimate script the model inlined. Saving it makes recovery one - turn (`bash `) instead of two, and is strictly safer than the - hint-only path: the file goes through the normal execution pipeline - (including the referenced-script content guard) and nothing runs here. - Returns the path, or None on any failure (hint falls back to write_file). - """ + """Persist a parser-limit-blocked command as a runnable script. That block + fires on payload SIZE/shape, not the operation — usually a legitimate script + the model inlined. Saving it makes recovery one turn (`bash `) instead + of two, and is strictly safer than the hint-only path: the file goes through + the normal execution pipeline (including the referenced-script content guard) + and nothing runs here. Returns the path, or None on any failure (hint falls + back to write_file).""" try: from hermes_constants import get_hermes_home script_dir = get_hermes_home() / "cache" / "blocked-scripts" @@ -113,8 +109,8 @@ def _hardline_block_result(description: str, command: str = "") -> dict: "agent." ) # The parser-limit block is almost always a giant inline payload, not a - # forbidden operation, and is typically followed by blind rephrase - # retries — point at the saved script (or the write_file recipe). + # forbidden operation, and is typically followed by blind rephrase retries — + # point at the saved script (or the write_file recipe). if description in (_PARSER_LIMIT_DESCRIPTION, _MALFORMED_EXEC_DESCRIPTION): saved = _a._save_blocked_payload(command) if command else None if saved: @@ -146,28 +142,24 @@ def _sudo_stdin_block_result(description: str) -> dict: } -# Shell control characters that make a command compound when they appear -# OUTSIDE quotes. Inside quotes they are literal to the outer shell — but they -# become executable again if an option like `-c`/`-e`/`--eval` (or a git -# `-c alias.x=!...`) hands the quoted argument to another interpreter, so quoted -# control chars only disqualify a command when such an option is present. +# Shell control characters that make a command compound when they appear OUTSIDE +# quotes. Inside quotes they are literal to the outer shell — but they become +# executable again if an option like `-c`/`-e`/`--eval` (or a git `-c alias.x=!...`) +# hands the quoted argument to another interpreter, so quoted control chars only +# disqualify a command when such an option is present. _SHELL_CONTROL_CHARS = frozenset("\n\r;&|<>`$()") -_REINTERPRETED_ARGUMENT_RE = re.compile( - r"(?:^|[ \t])(?:-[^-\s]*[ce]|--(?:command|eval))(?:[= \t]|$)" -) +_REINTERPRETED_ARGUMENT_RE = re.compile(r"(?:^|[ \t])(?:-[^-\s]*[ce]|--(?:command|eval))(?:[= \t]|$)") def _has_allowlist_shell_operator(command: str) -> bool: """Return True when a command is too compound for the allowlist shortcut. - Quote-aware: metacharacters inside quotes or behind a backslash are literal arguments (``cargo bench -- '^a(b|c)$'``), not shell syntax. Still disqualifying: ``$`` or backtick inside DOUBLE quotes (expansion stays active), and any quoted/escaped control character when the command also carries a ``-c``/``-e``/``--command``/``--eval``-style option that would - hand the quoted text to another interpreter. - """ + hand the quoted text to another interpreter.""" command = command or "" quote = None # None | "'" | '"' has_reinterpretable = False @@ -191,8 +183,8 @@ def _has_allowlist_shell_operator(command: str) -> bool: elif ch in ("'", '"'): quote = ch elif ch == "$": - # Unquoted $ is only compound when it opens a substitution - # ("$HOME" stays simple, matching the historical `\$\(` behavior). + # Unquoted $ is only compound when it opens a substitution ("$HOME" + # stays simple, matching the historical `\$\(` behavior). if i + 1 < n and command[i + 1] == "(": return True elif ch in _SHELL_CONTROL_CHARS and ch not in "()": @@ -205,12 +197,10 @@ def _has_allowlist_shell_operator(command: str) -> bool: def _command_matches_permanent_allowlist(command: str) -> bool: - """True when command_allowlist holds this exact command text or a matching glob. - - Permanent approvals historically store dangerous-pattern keys such as + """True when command_allowlist holds this exact command text or a matching + glob. Permanent approvals historically store dangerous-pattern keys such as ``recursive delete``; manual entries are command text, possibly with - shell-style wildcards like ``podman *``. - """ + shell-style wildcards like ``podman *``.""" from tools import approval as _a command = (command or "").strip() if not command or _a._has_allowlist_shell_operator(command): diff --git a/tools/approval_gateway_wait.py b/tools/approval_gateway_wait.py index 8ebea355fe..43fb352ff6 100644 --- a/tools/approval_gateway_wait.py +++ b/tools/approval_gateway_wait.py @@ -5,10 +5,9 @@ pending approval, the gateway notifies the user, and the thread blocks until ``/approve`` / ``/deny`` resolves it or the approval timeout elapses. Multiple threads (parallel subagents, execute_code RPC handlers) can block concurrently — each gets its own ``threading.Event``; ``/approve`` resolves the oldest, -``/approve all`` every pending entry. - -Queue state (``_gateway_queues``, ``_lock``) is owned by ``tools.approval`` and -reached through that module at call time so tests patching it keep working. +``/approve all`` every pending entry. Queue state (``_gateway_queues``, +``_lock``) is owned by ``tools.approval`` and reached through that module at +call time so tests patching it keep working. """ import logging @@ -31,27 +30,23 @@ class _ApprovalEntry: self.data.setdefault("request_id", uuid.uuid4().hex) self.acknowledged = False self.result: str | None = None # "once"|"session"|"always"|"deny" - # Free-text reason from ``/deny `` so the agent can adapt - # instead of only hearing "denied". + # Free-text reason from ``/deny `` so the agent can adapt, not just hear "denied". self.reason: str | None = None def _poll_event(event: threading.Event, session_key: str, *, interrupt_log: str) -> str: - """Wait on *event* until it fires, the turn is interrupted, or approvals.timeout elapses. - - Returns ``"set"`` | ``"interrupted"`` | ``"timeout"``. Polls in ~1s slices - so activity heartbeats reach the agent's inactivity tracker every ~10s — + """Wait on *event* until it fires, the turn is interrupted, or approvals.timeout + elapses; returns ``"set"`` | ``"interrupted"`` | ``"timeout"``. Polls in ~1s + slices so activity heartbeats reach the agent's inactivity tracker every ~10s — otherwise the gateway watchdog kills the agent while the user is still - responding (mirrors ``_wait_for_process()`` cadence). The loop is recorded - as human-wait time so the concurrent batch deadline excludes it. + responding (mirrors ``_wait_for_process()`` cadence). The loop is recorded as + human-wait time so the concurrent batch deadline excludes it. - ``is_interrupted()`` deliberately does NOT distinguish a deliberate /stop - from a gateway inactivity timeout — both resolve as 'deny' (not - outcome='timeout'). The per-thread interrupt flag carries no stable - machine-checkable cause, so a fail-closed deny preserves the historical - semantics; changing this needs a dedicated interrupt-cause channel, not - string matching. - """ + ``is_interrupted()`` deliberately does NOT distinguish a deliberate /stop from + a gateway inactivity timeout — both resolve as 'deny' (not outcome='timeout'). + The per-thread interrupt flag carries no stable machine-checkable cause, so a + fail-closed deny preserves the historical semantics; changing this needs a + dedicated interrupt-cause channel, not string matching.""" from tools.approval import _get_approval_timeout, human_wait_window timeout = _get_approval_timeout() @@ -87,15 +82,13 @@ def _finish(payload: dict, resolved: bool, choice: str | None, reason, **extra) def _await_coalesced_leader(session_key: str, leader, payload: dict): """Wait on an already-pending identical approval instead of re-prompting. - Adopts the leader's decision: ``session``/``always`` → approval (same dict shape as a direct resolution; persistence stays the caller's and is idempotent across leader and followers); ``deny`` → denial carrying the leader's reason; leader timeout / our own deadline → unresolved. ``once`` returns ``None``: single-use consent covers only the leader's execution, so the caller must issue a fresh prompt. Hooks fire with ``coalesced=True`` - so observers see the follower's lifecycle without a duplicate prompt. - """ + so observers see the follower's lifecycle without a duplicate prompt.""" from tools.approval import _fire_approval_hook _fire_approval_hook("pre_approval_request", **payload, coalesced=True) state = _poll_event(leader.event, session_key, @@ -117,9 +110,8 @@ def _await_coalesced_leader(session_key: str, leader, payload: dict): def _await_gateway_decision(session_key: str, notify_cb, approval_data: dict, *, surface: str = "gateway") -> dict: - """Enqueue *approval_data*, notify the user, and block until resolved or timed out. - - Shared by the terminal command guard, the execute_code guard, the plugin + """Enqueue *approval_data*, notify the user, and block until resolved or timed + out. Shared by the terminal command guard, the execute_code guard, the plugin escalation gate, and MCP elicitation. Returns ``{"resolved", "choice", "reason"}`` or ``{"resolved": False, "choice": None, "notify_failed": True}`` when the notify callback raised. Persisting the choice and building the @@ -129,8 +121,7 @@ def _await_gateway_decision(session_key: str, notify_cb, approval_data: dict, coalesced: parallel tool calls would otherwise fire N identical prompts the user must /approve N times while the agent sits wedged. Followers adopt the leader's ``session``/``always``/``deny``/timeout; a ``once`` covers only - the leader, so the follower falls through to a fresh prompt. - """ + the leader, so the follower falls through to a fresh prompt.""" from tools import approval as _approval primary_key = approval_data.get("pattern_key", "") diff --git a/tools/approval_human_wait.py b/tools/approval_human_wait.py index 24d2ca53e2..68b3178efe 100644 --- a/tools/approval_human_wait.py +++ b/tools/approval_human_wait.py @@ -6,11 +6,10 @@ deadline in agent/tool_executor.py excludes this time so a slow human answer never times a batch out — but ONLY this time. Measuring at the source (rather than residency in the authorization gate, which is arbitrary code) is what keeps a wedged pre_tool_call plugin or a dead approval client from growing the -exclusion 1:1 with wall clock and defeating the deadline entirely. - -Keyed by session so one gateway session's pending approval cannot extend a -different session's batch deadline. State is process-global like the rest of -the approval state; entries are bounded by _HUMAN_WAIT_MAX_SESSIONS. +exclusion 1:1 with wall clock and defeating the deadline entirely. Keyed by +session so one gateway session's pending approval cannot extend a different +session's batch deadline; state is process-global like the rest of the approval +state, bounded by _HUMAN_WAIT_MAX_SESSIONS. """ import contextlib @@ -30,26 +29,23 @@ class _HumanWaitState: _human_wait_lock = threading.Lock() _human_wait_states: dict[str, _HumanWaitState] = {} _HUMAN_WAIT_MAX_SESSIONS = 256 -# Margin added on top of approvals.timeout when clamping a window's -# contribution (read-side AND close-side) and when bounding the authorization -# gate's serialization-lock acquire in agent/tool_executor.py. One constant so -# the clamps can't drift apart. +# Margin added on top of approvals.timeout when clamping a window's contribution +# (read-side AND close-side) and when bounding the authorization gate's +# serialization-lock acquire in agent/tool_executor.py. One constant so the +# clamps can't drift apart. HUMAN_WAIT_MARGIN_S = 60.0 def human_wait_ceiling() -> float: """Max seconds a single window may contribute: approvals.timeout + margin. - - Every legitimate human wait self-terminates at ``approvals.timeout`` (the - CLI prompt join and the gateway poll loop both enforce it), so a window - that overstays this ceiling is itself wedged and must not keep extending - a batch deadline. Also the bound on the authorization gate's - serialization-lock acquire in agent/tool_executor.py, so the two cannot - drift. Never call while holding ``_human_wait_lock`` — it reads the - config cache. ``_get_approval_timeout`` caps at - ``agent.deadline.MAX_SAFE_TIMEOUT_S`` so the value is always safe for - ``Lock.acquire(timeout=...)`` / ``Thread.join(timeout=...)``. - """ + Every legitimate human wait self-terminates at ``approvals.timeout`` (the CLI + prompt join and the gateway poll loop both enforce it), so a window that + overstays this ceiling is itself wedged and must not keep extending a batch + deadline. Also the bound on the authorization gate's serialization-lock + acquire in agent/tool_executor.py, so the two cannot drift. Never call while + holding ``_human_wait_lock`` — it reads the config cache. + ``_get_approval_timeout`` caps at ``agent.deadline.MAX_SAFE_TIMEOUT_S`` so the + value is always safe for ``Lock.acquire(timeout=...)`` / ``Thread.join(timeout=...)``.""" from tools.approval import _get_approval_timeout return float(_get_approval_timeout()) + HUMAN_WAIT_MARGIN_S @@ -62,14 +58,12 @@ def _clamped_window_seconds(started: float, now: float, ceiling: float) -> float def _human_wait_state(session_key: str) -> _HumanWaitState: - """Return (creating if needed) the wait state for *session_key*. - - Caller must hold ``_human_wait_lock``. Evicts idle entries (no pending - waiter) insertion-order-first until the table is under the cap so an army - of short-lived session keys cannot grow it without bound. Entries with an - open window are never evicted (that would corrupt live accounting), so - the cap is best-effort under 256+ concurrently-pending sessions. - """ + """Return (creating if needed) the wait state for *session_key*. Caller must + hold ``_human_wait_lock``. Evicts idle entries (no pending waiter) + insertion-order-first until the table is under the cap so an army of + short-lived session keys cannot grow it without bound. Entries with an open + window are never evicted (that would corrupt live accounting), so the cap is + best-effort under 256+ concurrently-pending sessions.""" state = _human_wait_states.get(session_key) if state is None: for key in list(_human_wait_states): @@ -90,16 +84,13 @@ def _resolve_key(session_key: str | None) -> str: @contextlib.contextmanager def human_wait_window(session_key: str | None = None): - """Mark the enclosed block as time spent blocked on a human prompt. - - Wrap ONLY code that is genuinely parked waiting for a user's answer (the - CLI approval prompt, the gateway approval poll loop). The concurrent tool - batch deadline excludes this time; wrapping anything else re-creates the - hang where arbitrary wedged code pushes the deadline out forever. - - Overlapping windows for the same session coalesce (pending counter), so - two serialized approval prompts don't double-count the same wall clock. - """ + """Mark the enclosed block as time spent blocked on a human prompt. Wrap ONLY + code that is genuinely parked waiting for a user's answer (the CLI approval + prompt, the gateway approval poll loop). The concurrent tool batch deadline + excludes this time; wrapping anything else re-creates the hang where + arbitrary wedged code pushes the deadline out forever. Overlapping windows + for the same session coalesce (pending counter), so two serialized approval + prompts don't double-count the same wall clock.""" key = _resolve_key(session_key) now = time.monotonic() with _human_wait_lock: @@ -111,8 +102,8 @@ def human_wait_window(session_key: str | None = None): yield finally: now = time.monotonic() - # Clamp the accrual too: a window that overstayed the ceiling was - # wedged — record at most the ceiling, not the whole overstay. + # Clamp the accrual too: a window that overstayed the ceiling was wedged — + # record at most the ceiling, not the whole overstay. ceiling = human_wait_ceiling() with _human_wait_lock: state = _human_wait_states.get(key) @@ -120,27 +111,23 @@ def human_wait_window(session_key: str | None = None): state.pending -= 1 if state.pending == 0: if state.window_started is not None: - state.completed_seconds += _clamped_window_seconds( - state.window_started, now, ceiling - ) + state.completed_seconds += _clamped_window_seconds(state.window_started, now, ceiling) state.window_started = None def human_wait_seconds(session_key: str | None = None) -> float: - """Return total human-wait seconds recorded for the session. - - Completed windows plus the currently open one (if any). Monotonically - non-decreasing for the life of the process — except when an idle session's - entry is evicted under cap pressure, which can only shrink a consumer's - baseline delta to zero (the safe direction: the deadline fires sooner). - Deadline consumers snapshot a baseline at batch start and use the delta. - Each window's contribution is clamped to :func:`human_wait_ceiling` - (belt-and-braces against the wedged-window hang). - """ + """Return total human-wait seconds recorded for the session: completed windows + plus the currently open one (if any). Monotonically non-decreasing for the + life of the process — except when an idle session's entry is evicted under + cap pressure, which can only shrink a consumer's baseline delta to zero (the + safe direction: the deadline fires sooner). Deadline consumers snapshot a + baseline at batch start and use the delta. Each window's contribution is + clamped to :func:`human_wait_ceiling` (belt-and-braces against the + wedged-window hang).""" key = _resolve_key(session_key) now = time.monotonic() - # Resolve the clamp outside the lock: it reads the config cache, which - # must never nest under _human_wait_lock. + # Resolve the clamp outside the lock: it reads the config cache, which must + # never nest under _human_wait_lock. ceiling = human_wait_ceiling() with _human_wait_lock: state = _human_wait_states.get(key) diff --git a/tools/approval_prompt.py b/tools/approval_prompt.py index cf33c57855..58009e23c1 100644 --- a/tools/approval_prompt.py +++ b/tools/approval_prompt.py @@ -22,29 +22,26 @@ def prompt_dangerous_approval(command: str, description: str, timeout_seconds: i *, allow_session: bool = True, smart_denied: bool = False) -> str: """Prompt the user to approve a dangerous command (CLI only). - Args: - allow_permanent: When False, hide [a]lways (tirith warnings present: - broad permanent allowlisting is wrong for content-level findings). - allow_session: When False, hide [s]ession too — the caller grants one - operation and re-asks next time (the protected agent-instruction - gate in ``tools/file_tools.py``). Offering a scope the caller - discards makes every later write re-prompt and reads as broken. - smart_denied: Owner override of a Smart DENY: offer only once/deny. - approval_callback: CLI prompt_toolkit callback, - ``(command, description, *, allow_permanent=True, - allow_session=True, smart_denied=False) -> str``. Legacy - signatures keep working while both keywords hold their defaults. + allow_permanent=False hides [a]lways (tirith warnings present: broad permanent + allowlisting is wrong for content-level findings). allow_session=False hides + [s]ession too — the caller grants one operation and re-asks next time (the + protected agent-instruction gate in ``tools/file_tools.py``); offering a scope + the caller discards makes every later write re-prompt and reads as broken. + smart_denied: owner override of a Smart DENY, offer only once/deny. + approval_callback: CLI prompt_toolkit callback ``(command, description, *, + allow_permanent=True, allow_session=True, smart_denied=False) -> str``; legacy + signatures keep working while both keywords hold their defaults. - Returns: 'once', 'session', 'always', 'deny', or 'timeout'. 'timeout' - means no user response — still blocked (fail-closed), but callers - report "no response" rather than an explicit denial. + Returns 'once', 'session', 'always', 'deny', or 'timeout'. 'timeout' means no + user response — still blocked (fail-closed), but callers report "no response" + rather than an explicit denial. """ from tools import approval as _a if timeout_seconds is None: timeout_seconds = _a._get_approval_timeout() - # Everything below is a human prompt (callback panel or input() fallback, - # both bounded by the approval deadline): record it as human-wait time so - # the concurrent batch deadline excludes it. + # Everything below is a human prompt (callback panel or input() fallback, both + # bounded by the approval deadline): record it as human-wait time so the + # concurrent batch deadline excludes it. with human_wait_window(): return _prompt_dangerous_approval_inner( command, description, timeout_seconds, allow_permanent, @@ -85,9 +82,9 @@ def _read_choice(prompt: str, timeout_seconds: int) -> str | None: def _prompt_dangerous_approval_inner(command: str, description: str, timeout_seconds: int, allow_permanent: bool = True, approval_callback=None, *, allow_session: bool = True, smart_denied: bool = False) -> str: - # Redact before any user-visible rendering; the original `command` is - # still what executes after approval. Same redactor as memory/log - # sanitization so tokens mask consistently across surfaces. + # Redact before any user-visible rendering; the original `command` still + # executes after approval. Same redactor as memory/log sanitization so + # tokens mask consistently across surfaces. from agent.redact import redact_sensitive_text display_command = redact_sensitive_text(command) display_description = redact_sensitive_text(description) @@ -106,12 +103,11 @@ def _prompt_dangerous_approval_inner(command: str, description: str, timeout_sec logger.error("Approval callback failed: %s", e, exc_info=True) return "deny" - # Fail-closed guard: when prompt_toolkit owns the terminal and no callback - # is registered on this thread, the input() fallback would spawn a daemon - # thread whose read never sees Enter (keystrokes go to prompt_toolkit) — - # an invisible deadlock. Deny loudly instead; threads needing interactive - # approval must install a callback via - # tools.terminal_tool.set_approval_callback() first. + # Fail-closed guard: when prompt_toolkit owns the terminal and no callback is + # registered on this thread, the input() fallback would spawn a daemon thread + # whose read never sees Enter (keystrokes go to prompt_toolkit) — an invisible + # deadlock. Deny loudly instead; threads needing interactive approval must + # install a callback via tools.terminal_tool.set_approval_callback() first. try: from prompt_toolkit.application.current import get_app_or_none if get_app_or_none() is not None: @@ -163,9 +159,9 @@ def _prompt_dangerous_approval_inner(command: str, description: str, timeout_sec def get_plugin_manager(): """Lazy plugin-manager seam used by tests and early tool-only imports.""" from hermes_cli.plugins import discover_plugins, get_plugin_manager as _get_manager - # Approval can be imported before model_tools (which triggers discovery); - # make an explicitly selected transport available on the first approval - # instead of treating the undiscovered registry as unavailable. + # Approval can be imported before model_tools (which triggers discovery); make + # an explicitly selected transport available on the first approval instead of + # treating the undiscovered registry as unavailable. discover_plugins() return _get_manager() @@ -178,13 +174,11 @@ def _attempt(name: str, choice, failure, fallback) -> dict: def _present_with_selected_transport(*, command: str, description: str, pattern_key: str, pattern_keys: list[str], session_key: str, surface: str, allow_session: bool, allow_permanent: bool) -> dict: - """Present through an explicitly selected plugin transport, if any. - - A selected transport replaces every built-in prompt surface; detection, - allowed scopes, persistence, timeout, and final authorization stay - host-owned. A failed transport reaches a built-in surface only under the - explicit ``transport_fallback: builtin`` opt-in. - """ + """Present through an explicitly selected plugin transport, if any. A selected + transport replaces every built-in prompt surface; detection, allowed scopes, + persistence, timeout, and final authorization stay host-owned. A failed + transport reaches a built-in surface only under the explicit + ``transport_fallback: builtin`` opt-in.""" from tools import approval as _a name, fallback = _a._get_approval_transport_config() if name == "builtin": @@ -213,9 +207,8 @@ def _present_with_selected_transport(*, command: str, description: str, pattern_ timeout_seconds=timeout_seconds, ) except Exception: - # Never fall back to raw text if redaction or request construction - # fails: fail closed without calling the plugin or leaking the - # unredacted payload to logs/hooks. + # Never fall back to raw text if redaction or request construction fails: + # fail closed without calling the plugin or leaking the unredacted payload. logger.warning("Could not build redacted plugin approval request") return _attempt(name, "deny", "error", None) hook_kwargs = dict( @@ -246,12 +239,10 @@ def _present_with_selected_transport(*, command: str, description: str, pattern_ def _transport_choice(attempt: dict, *, pattern_key: str, description: str): - """Interpret a ``_present_with_selected_transport`` attempt. - - Returns ``(choice, denied_result)``: both None when the built-in surfaces - should run (no transport selected, or a failure with the explicit builtin - fallback); a denied result for any other failure; else the user's choice. - """ + """Interpret a ``_present_with_selected_transport`` attempt into + ``(choice, denied_result)``: both None when the built-in surfaces should run + (no transport selected, or a failure with the explicit builtin fallback); a + denied result for any other failure; else the user's choice.""" if not attempt.get("selected"): return None, None failure = attempt.get("failure") @@ -281,14 +272,12 @@ def _consent(choice, unresolved: str) -> str: def request_elicitation_consent(message: str, description: str, *, timeout_seconds: int | None = None, surface: str = "mcp-elicitation") -> str: - """Route an MCP elicitation request to the surface owning the active session. - - Gateway sessions go through ``_await_gateway_decision``; CLI/TUI through - ``prompt_dangerous_approval``. Always fails closed: a missing notify_cb in - a gateway session, timeouts, and exceptions map to ``"decline"`` so a server + """Route an MCP elicitation request to the surface owning the active session: + gateway sessions through ``_await_gateway_decision``, CLI/TUI through + ``prompt_dangerous_approval``. Always fails closed: a missing notify_cb in a + gateway session, timeouts, and exceptions map to ``"decline"`` so a server treats them as "user did not approve" rather than retrying or hanging. - Returns ``"accept" | "decline" | "cancel"``. - """ + Returns ``"accept" | "decline" | "cancel"``.""" from tools import approval as _a try: session_key = _a.get_current_session_key() @@ -315,8 +304,7 @@ def request_elicitation_consent(message: str, description: str, *, return "cancel" return _consent(decision.get("choice"), "decline") - # allow_permanent=False: elicitation is a per-call confirmation — there - # is no pattern to remember. + # allow_permanent=False: elicitation is a per-call confirmation — no pattern to remember. try: choice = _a.prompt_dangerous_approval( message, description, timeout_seconds=timeout_seconds, allow_permanent=False, diff --git a/tools/approval_smart.py b/tools/approval_smart.py index b1b60ce939..dc27e6c816 100644 --- a/tools/approval_smart.py +++ b/tools/approval_smart.py @@ -55,12 +55,9 @@ def _strip_line_comment(line: str) -> str: def _strip_shell_comments(command: str) -> str: - """Strip unquoted ``# ...`` comments before LLM assessment. - - Not a POSIX parser — quoted ``#`` and heredoc bodies are preserved by a - simple state machine. The goal is removing the low-hanging injection - surface, not full shell parsing. - """ + """Strip unquoted ``# ...`` comments before LLM assessment. Not a POSIX parser + — quoted ``#`` and heredoc bodies are preserved by a simple state machine; the + goal is removing the low-hanging injection surface, not full shell parsing.""" cleaned: list[str] = [] for line in command.split("\n"): stripped = _strip_line_comment(line) @@ -82,16 +79,15 @@ def _smart_approve(command: str, description: str) -> str: try: from agent.auxiliary_client import _get_task_timeout, call_llm - # Pass the timeout explicitly AND log call + duration: this synchronous - # call gates EVERY flagged command, and a stalled provider once froze - # turns for tens of minutes with zero log output. + # Pass the timeout explicitly AND log call + duration: this synchronous call + # gates EVERY flagged command, and a stalled provider once froze turns for + # tens of minutes with zero log output. smart_timeout = _get_task_timeout("approval") logger.debug("Smart approvals: assessing risk for command (timeout=%ss)", smart_timeout) system_prompt = _SYSTEM_PROMPT - # Operator policy goes in the SYSTEM prompt only — the trusted channel. - # Never next to the block: that would dilute the trust - # boundary and teach the guard to accept policy-looking text adjacent - # to (untrusted) commands. + # Operator policy goes in the SYSTEM prompt only — the trusted channel. Never + # next to the block: that would dilute the trust boundary and teach + # the guard to accept policy-looking text adjacent to (untrusted) commands. operator_policy = _get_smart_policy() if operator_policy: system_prompt += ( @@ -127,10 +123,8 @@ def _smart_approve(command: str, description: str) -> str: def _smart_verdict(command: str, description: str, pattern_key: str, pattern_keys: list[str], session_key: str) -> str: """Run the guardian LLM with observer hooks; 'approve' | 'deny' | 'escalate'. - Redaction is observer-payload preparation, not approval policy: if it fails, - skip observability rather than leak raw data or block the LLM decision. - """ + skip observability rather than leak raw data or block the LLM decision.""" from tools import approval as _a try: from agent.redact import redact_sensitive_text