fix(policies): block ASK gates until a human answers, not a short client timeout (#626)
* fix(policies): default ASK approval timeout to 1 day, not 30s
An ASK policy is a human-in-the-loop gate, but DEFAULT_ASK_TIMEOUT was
30s. When a user didn't answer within 30s the server failed closed
(DENY) with no input and the web card flipped to the neutral "Resolved
elsewhere" pill -- looking like a silent auto-resolve. This bit the
session_cost_budget warning-threshold ASK in particular: it re-fires on
every request/tool_call until approved (the approved-checkpoint
state_update lands only on accept), so each one timed out in turn.
Every other wait-for-a-human budget in the native path is already
86400 (1 day): the PermissionRequest / evaluate-policy hook long-polls
and their server-side mirrors. The design intent (see sessions.py and
polly's config) is that everything waits a day and the policy
ask_timeout is the real cap -- so a 30s default was the lone outlier
that capped first. Align the default with the rest of the system.
Headless/unattended agents that want a fast fail-closed still override
per-policy via PolicySpec.ask_timeout or spec-wide via
GuardrailsSpec.ask_timeout (polly already does).
* fix(policies): block ASK gates until a human answers, not a short client timeout
An ASK approval is a human-in-the-loop checkpoint, but several client-side
timeouts on the delivery paths capped the wait far below the deciding
policy's ask_timeout. So the approval card auto-resolved (DENY) — or, on
the sub-agent wake path, retried into duplicate cards — before any human
could answer. The deciding policy's ask_timeout must be the single real
cap; every layer that merely waits for the human is pinned above it.
Source:
- spec: DEFAULT_ASK_TIMEOUT -> INT_MAX (effectively infinite, ~68y).
- native plumbing (claude/codex hooks + server-side mirrors): every
wait-for-a-human budget -> INT_MAX so no layer caps the wait first.
- runner deliverers that PARK behind the gate now wait for the verdict
instead of severing it, extracted to a named _ASK_GATE_DELIVERY_TIMEOUT
(INT_MAX read, fast 30s connect): the policy-eval + sub-agent
wake-notice POSTs (runner/app.py) and the message-send POSTs
(runner/tool_dispatch.py); plus pending_approvals._DEFAULT_WAIT_SECONDS
(was 120s -> auto-refuse) -> INT_MAX.
- SDK round-trip gate (_scaffold): -> INT_MAX and fail CLOSED (DENY) on the
now-unreachable expiry instead of fail-open (ALLOW).
Tests:
- tests/test_ask_timeout_infinite.py: drift-guard pinning every ASK timeout
(policy default, native plumbing + lockstep ordering, SDK, runner
delivery constants) to INT_MAX.
- tests/runner/test_pending_approvals.py: behavioral test that the gate
keeps blocking on the default budget and only a real verdict releases it.
- updated scaffold fail-closed + claude-bridge hook-timeout assertions.
* fix(policies): scope ASK-gate fix to 1 day, not infinite
Per review: 1 day (DEFAULT_ASK_TIMEOUT) is enough; no need for an effectively
infinite budget. The native plumbing was ALREADY 1 day before this work — the
bug was only that several runner→server delivery clients sat BELOW it. So:
- Revert the "infinite" (INT_MAX) churn on the native plumbing, DEFAULT_ASK_TIMEOUT,
and the server-side park mirrors back to main's existing 1-day values (those
files now have no net change).
- Keep only the real fix: bump the sub-1-day delivery budgets up to the 1-day
ASK budget so they wait for the verdict instead of severing the parked gate:
* pending_approvals._DEFAULT_WAIT_SECONDS 120s -> 86400
* runner.app _ASK_GATE_DELIVERY_TIMEOUT (policy-eval + wake POST) 30s -> 86400 read
* runner.tool_dispatch _ASK_GATE_DELIVERY_TIMEOUT (message sends) 30s -> 86400 read
* _scaffold._POLICY_EVAL_TIMEOUT_S 35s -> 86400 (main's phase-aware fail
open/closed fallback kept)
connect stays fast (30s).
Tests: rename drift-guard to tests/test_ask_timeout.py, assert the delivery
budgets == 1 day and never undercut DEFAULT_ASK_TIMEOUT; behavioral test in
test_pending_approvals.py unchanged in intent (gate blocks until verdict).
This commit is contained in:
ckcuslife-source
+35
-2
@@ -108,6 +108,18 @@ _SUBAGENT_DELIVERY_UNTRACKED = "untracked"
|
||||
_SUBAGENT_DELIVERY_MISSING_WORK_ENTRY = "missing_work_entry"
|
||||
_SUBAGENT_DELIVERY_MISSING_PARENT_INBOX = "missing_parent_inbox"
|
||||
_NATIVE_TERMINAL_START_FAILED_CODE = "native_terminal_start_failed"
|
||||
# Read budget for runner→server POSTs that can PARK behind a human-approval
|
||||
# ASK gate: policy evaluation (``_evaluate_policy_via_omnigent``) and sub-agent
|
||||
# wake-notice delivery (``_deliver_subagent_wake_post``). Both are gated at the
|
||||
# recipient's REQUEST/LLM/TOOL phase, which can hold for the deciding policy's
|
||||
# ``ask_timeout`` (default one day). Held at one day (86400s) — matching that
|
||||
# default — so the POST WAITS for the real verdict instead of severing the
|
||||
# parked gate at a short read timeout. A 30s cut previously fail-closed to DENY
|
||||
# (and the wake POST retried into duplicate approval cards). Fast connect (30s)
|
||||
# so an unreachable server still fails out promptly into the caller's
|
||||
# fail-open/retry path. Guarded by tests/test_ask_timeout_infinite.py.
|
||||
_ASK_GATE_DELIVERY_READ_TIMEOUT_S: float = 86400.0
|
||||
_ASK_GATE_DELIVERY_TIMEOUT = httpx.Timeout(_ASK_GATE_DELIVERY_READ_TIMEOUT_S, connect=30.0)
|
||||
# Terminal resource hosting the framework's own TUI (the Omnigent REPL,
|
||||
# ``omnigent attach``) for runner-hosted SDK sessions — the SDK mirror of
|
||||
# the claude-/codex-native embedded terminals. Resource id derives as
|
||||
@@ -2970,7 +2982,18 @@ async def _evaluate_policy_via_omnigent(
|
||||
"data": data,
|
||||
},
|
||||
},
|
||||
timeout=30.0,
|
||||
# A TOOL_CALL/LLM_REQUEST/REQUEST ASK parks server-side in
|
||||
# ``_hold_native_ask_gate`` until a human resolves it (up to the
|
||||
# deciding policy's ``ask_timeout``, default one day). A 30s read
|
||||
# budget here severed that long-poll after 30s — the server saw an
|
||||
# UPSTREAM DISCONNECT and failed the gate closed (DENY), so the
|
||||
# main (claude-sdk) agent's approval card auto-resolved while
|
||||
# native sub-agents (whose hooks already wait the full day) parked
|
||||
# correctly. Hold the read budget at one day to match the native
|
||||
# hooks' ``_EVALUATE_POLICY_TIMEOUT_S``; the server's ``ask_timeout``
|
||||
# remains the single real cap. Fast connect so an unreachable
|
||||
# server still fails out promptly into the fail-open path below.
|
||||
timeout=_ASK_GATE_DELIVERY_TIMEOUT,
|
||||
)
|
||||
if ap_resp.status_code == 200:
|
||||
result = ap_resp.json()
|
||||
@@ -4016,7 +4039,17 @@ async def _deliver_subagent_wake_post(
|
||||
"content": [{"type": "input_text", "text": notice}],
|
||||
},
|
||||
},
|
||||
timeout=30.0,
|
||||
# The server gates this injected wake at the parent's REQUEST
|
||||
# phase, which can PARK on a human ASK (e.g. session_cost_budget)
|
||||
# for up to the deciding policy's ``ask_timeout`` (default one
|
||||
# day). A 30s read budget severed that park after 30s → the
|
||||
# TimeoutError below retried → each retry re-posted the notice
|
||||
# and parked ANOTHER gate → duplicate approval cards, and the
|
||||
# gate never cleanly blocked. Hold the read budget at one day so
|
||||
# this POST waits for the real verdict (one held connection, one
|
||||
# card); fast connect so an unreachable parent runner still
|
||||
# fails out into the bounded retry below.
|
||||
timeout=_ASK_GATE_DELIVERY_TIMEOUT,
|
||||
)
|
||||
# Treat a non-2xx RESPONSE (e.g. a genuine 503 JSONResponse) as a
|
||||
# failure — httpx does not raise on status by itself.
|
||||
|
||||
@@ -31,10 +31,16 @@ import asyncio
|
||||
from collections.abc import Callable
|
||||
from typing import Any
|
||||
|
||||
# Default wait budget for a UI verdict, in seconds. Bounded so a
|
||||
# user who walked away doesn't pin a runner task forever; on
|
||||
# timeout the caller treats the elicitation as refused.
|
||||
_DEFAULT_WAIT_SECONDS: float = 120.0
|
||||
# Default wait budget for a UI verdict, in seconds. Held at one day
|
||||
# (86400s) — matching the deciding policy's default ``ask_timeout``: an ASK
|
||||
# is a human-in-the-loop gate and should outlive a user stepping away rather
|
||||
# than auto-refuse on its own. The old 120s default silently refused (treated
|
||||
# as DENY) any prompt a user didn't answer within two minutes — the
|
||||
# runner-side mirror of the cost-policy auto-resolve bug. Callers that resolve
|
||||
# a per-policy ``ask_timeout`` should still pass ``timeout_seconds`` explicitly;
|
||||
# this is only the fallback when none is provided. Headless/unattended agents
|
||||
# that want a fast fail-closed should pass a finite ``timeout_seconds``.
|
||||
_DEFAULT_WAIT_SECONDS: float = 86400.0
|
||||
|
||||
# Module-global registry: elicitation_id → asyncio.Future[bool].
|
||||
# True = approved, False = declined/timed-out. Future is owned by the
|
||||
|
||||
@@ -98,6 +98,15 @@ _SUBAGENT_POLICY_STATUSES = frozenset({"completed", "failed"})
|
||||
_SUBAGENT_INBOX_TERMINAL_STATUSES = frozenset({"completed", "failed", "cancelled"})
|
||||
_SUBAGENT_POLICY_FAILURE_OUTPUT = "[Result suppressed by policy: policy evaluation failed]"
|
||||
_SESSION_WRAPPER_LABEL_KEY = "omnigent.wrapper"
|
||||
# Read budget for runner→server message-send POSTs that are gated at the
|
||||
# recipient's REQUEST phase, which can PARK behind a human-approval ASK gate
|
||||
# (e.g. session_cost_budget) for the deciding policy's ``ask_timeout``. Held at
|
||||
# one day (86400s) — matching that default — so the send WAITS for the verdict
|
||||
# instead of severing the parked gate at a short read timeout (a 30s cut
|
||||
# previously fail-closed to DENY). Fast connect (30s) so an unreachable server
|
||||
# still fails out promptly. Guarded by tests/test_ask_timeout_infinite.py.
|
||||
_ASK_GATE_DELIVERY_READ_TIMEOUT_S: float = 86400.0
|
||||
_ASK_GATE_DELIVERY_TIMEOUT = httpx.Timeout(_ASK_GATE_DELIVERY_READ_TIMEOUT_S, connect=30.0)
|
||||
|
||||
# Read timeouts for the two MCP-proxy hops that carry a tool call back to the
|
||||
# runner (runner → Omnigent server → runner). ``sys_os_shell`` accepts caller-provided
|
||||
@@ -1186,7 +1195,12 @@ async def _execute_subagent_tool(
|
||||
"content": [{"type": "input_text", "text": str(message)}],
|
||||
},
|
||||
},
|
||||
timeout=30.0,
|
||||
# This message is gated at the recipient's REQUEST phase, which can
|
||||
# PARK on a human ASK (e.g. session_cost_budget) up to the policy's
|
||||
# ``ask_timeout``. A 30s read budget severed that park → fail-closed
|
||||
# /retry → duplicate cards. Wait for the real verdict (one-day read
|
||||
# budget, fast connect); a non-parking eval still returns immediately.
|
||||
timeout=_ASK_GATE_DELIVERY_TIMEOUT,
|
||||
)
|
||||
except httpx.HTTPError as exc:
|
||||
_runner_app.unregister_child_session(child_session_id)
|
||||
@@ -1328,7 +1342,11 @@ async def _send_to_existing_session(
|
||||
"content": [{"type": "input_text", "text": message}],
|
||||
},
|
||||
},
|
||||
timeout=30.0,
|
||||
# Same as the other message-send: gated at the recipient's REQUEST
|
||||
# phase, which can PARK on a human ASK up to the policy's
|
||||
# ``ask_timeout``. Wait for the real verdict (one-day read budget,
|
||||
# fast connect) instead of severing at 30s and retrying into duplicates.
|
||||
timeout=_ASK_GATE_DELIVERY_TIMEOUT,
|
||||
)
|
||||
except httpx.HTTPError as exc:
|
||||
_runner_app.unregister_child_session(target_session_id)
|
||||
|
||||
@@ -89,11 +89,14 @@ _HEARTBEAT_INTERVAL_S = 15.0
|
||||
_SHUTDOWN_GRACE_S = 4.5
|
||||
|
||||
# Timeout for the policy evaluation round-trip (harness → runner →
|
||||
# Omnigent server → runner → harness). Fail-open on expiry so a stalled
|
||||
# round-trip doesn't hang the executor indefinitely. Must be ≥
|
||||
# DEFAULT_POLICY_CLASSIFIER_TIMEOUT (30 s) since PromptPolicy
|
||||
# classifiers make their own LLM call on the Omnigent server side.
|
||||
_POLICY_EVAL_TIMEOUT_S = 35.0
|
||||
# Omnigent server → runner → harness). Held at one day (86400s) — matching
|
||||
# the deciding policy's default ``ask_timeout``: a TOOL_CALL/REQUEST ASK parks
|
||||
# server-side until a human answers, and this gate must block until the
|
||||
# verdict arrives rather than auto-resolve on a short cut (the cost-policy
|
||||
# bug). The server caps the real wait via the policy's ``ask_timeout``. On the
|
||||
# (now rare) expiry the fallback below is phase-aware — TOOL_CALL fails CLOSED
|
||||
# (DENY), advisory LLM/TOOL_RESULT phases fail OPEN (ALLOW).
|
||||
_POLICY_EVAL_TIMEOUT_S = 86400.0
|
||||
|
||||
# Per-turn IDLE watchdog: max gap WITHOUT progress before a wedged
|
||||
# ``run_turn`` becomes ``response.failed`` (vs heartbeating forever).
|
||||
|
||||
@@ -227,6 +227,58 @@ async def test_wait_for_user_approval_publishes_on_cancellation() -> None:
|
||||
assert publishes[0][1]["elicitation_id"] == "elicit_cancel"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_wait_for_user_approval_default_budget_gates_until_verdict() -> None:
|
||||
"""With NO explicit timeout, the ASK gate blocks until a human verdict.
|
||||
|
||||
Behavioral guard for the cost-policy auto-resolve bug. The relay / MCP
|
||||
callers (``proxy_mcp_manager`` / ``mcp_manager``) invoke
|
||||
:func:`wait_for_user_approval` WITHOUT ``timeout_seconds``, so it falls
|
||||
back to :data:`pending_approvals._DEFAULT_WAIT_SECONDS`. That default was
|
||||
once 120s, which silently refused (``False``) any prompt the human didn't
|
||||
answer within two minutes — the card "auto-resolved" and the agent moved
|
||||
on. The default is now one day (matching the policy's ``ask_timeout``), so
|
||||
the gate must KEEP blocking until a real verdict arrives; ONLY the verdict —
|
||||
never the budget elapsing on its own — may release it.
|
||||
|
||||
This exercises the exact default-budget path the real callers use (no
|
||||
``timeout_seconds``) and asserts: (1) the gate is still parked after a
|
||||
real delay, and (2) a human verdict — and only that — releases it with
|
||||
the right value. A regression that shortens the default to anything
|
||||
inside the sleep window flips assertion (1); the companion drift-guard in
|
||||
``tests/test_ask_timeout.py`` pins the exact one-day value so a 120s-style
|
||||
regression (too long to catch by waiting) still fails loudly.
|
||||
"""
|
||||
publishes: list[tuple[str, dict[str, Any]]] = []
|
||||
|
||||
def _publish(conv_id: str, event: dict[str, Any]) -> None:
|
||||
publishes.append((conv_id, event))
|
||||
|
||||
task = asyncio.create_task(
|
||||
pending_approvals.wait_for_user_approval(
|
||||
elicitation_id="elicit_default_budget",
|
||||
conversation_id="conv_default_budget",
|
||||
publish_event=_publish,
|
||||
# NOTE: no timeout_seconds — drive the default budget, the exact
|
||||
# path the relay/MCP approval callers use.
|
||||
)
|
||||
)
|
||||
# The gate must STILL be blocking after a real delay — it must not
|
||||
# auto-resolve on the default budget. (Pre-fix, a short/expired default
|
||||
# would have already returned False here.)
|
||||
await asyncio.sleep(0.2)
|
||||
assert not task.done(), (
|
||||
"ASK gate auto-resolved on the default budget — it must keep gating "
|
||||
"until a human verdict, never refuse on its own."
|
||||
)
|
||||
assert pending_approvals.has_pending("conv_default_budget") is True
|
||||
|
||||
# Only a real human verdict releases the gate, and the value is honored.
|
||||
assert pending_approvals.resolve("elicit_default_budget", approved=True) is True
|
||||
assert await asyncio.wait_for(task, timeout=1.0) is True
|
||||
assert pending_approvals.has_pending("conv_default_budget") is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# has_pending — session is "awaiting human approval"
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -0,0 +1,73 @@
|
||||
"""Drift guard for the policy-ASK delivery timeouts.
|
||||
|
||||
A policy ASK is a human-in-the-loop gate: the verdict is delivered
|
||||
synchronously over a runner→server connection while the server parks the
|
||||
gate for up to the deciding policy's ``ask_timeout`` (default one day,
|
||||
``DEFAULT_ASK_TIMEOUT``). The cost-policy bug was that several of those
|
||||
delivery clients used short read timeouts (30s / 120s / 35s) — far below
|
||||
``ask_timeout`` — so they severed the parked gate before any human answered,
|
||||
which fail-closed to DENY (and the sub-agent wake POST retried into duplicate
|
||||
approval cards).
|
||||
|
||||
This pins every such delivery budget to the one-day ASK budget so no client
|
||||
caps the wait before the policy does. ``connect`` stays fast (30s) so an
|
||||
unreachable server still fails out promptly. If any delivery budget drops
|
||||
below ``DEFAULT_ASK_TIMEOUT`` again, these fail loudly.
|
||||
"""
|
||||
|
||||
import omnigent.runner.app as runner_app
|
||||
import omnigent.runner.pending_approvals as pending_approvals
|
||||
import omnigent.runner.tool_dispatch as tool_dispatch
|
||||
import omnigent.runtime.harnesses._scaffold as scaffold
|
||||
from omnigent.spec.types import DEFAULT_ASK_TIMEOUT
|
||||
|
||||
# The deciding policy's default ASK budget — one day. Every delivery client
|
||||
# that can park behind the gate is pinned to this so none caps the wait first.
|
||||
ONE_DAY = 86400
|
||||
|
||||
|
||||
def test_default_ask_timeout_is_one_day() -> None:
|
||||
"""Anchor: the policy ASK default is one day."""
|
||||
assert DEFAULT_ASK_TIMEOUT == ONE_DAY
|
||||
|
||||
|
||||
def test_ask_gate_delivery_timeouts_hold_the_ask_budget() -> None:
|
||||
"""Every runner→server client that PARKS behind a human-approval gate holds
|
||||
its read budget at the one-day ASK budget (fast connect kept).
|
||||
|
||||
These are the exact paths whose short timeouts produced the auto-resolved
|
||||
card + duplicate cards: the relay/MCP approval default, the policy-eval +
|
||||
sub-agent wake-notice POSTs, the message-send POSTs, and the SDK round-trip.
|
||||
"""
|
||||
# relay / MCP approval park default (was 120s -> auto-refuse).
|
||||
assert pending_approvals._DEFAULT_WAIT_SECONDS == ONE_DAY
|
||||
|
||||
# policy-eval + sub-agent wake-notice delivery POSTs (were 30s; the wake
|
||||
# POST retried on each timeout -> duplicate cards).
|
||||
assert runner_app._ASK_GATE_DELIVERY_READ_TIMEOUT_S == ONE_DAY
|
||||
assert runner_app._ASK_GATE_DELIVERY_TIMEOUT.read == ONE_DAY
|
||||
assert runner_app._ASK_GATE_DELIVERY_TIMEOUT.connect == 30.0
|
||||
|
||||
# message-send POSTs to a child/target session (were 30s).
|
||||
assert tool_dispatch._ASK_GATE_DELIVERY_READ_TIMEOUT_S == ONE_DAY
|
||||
assert tool_dispatch._ASK_GATE_DELIVERY_TIMEOUT.read == ONE_DAY
|
||||
assert tool_dispatch._ASK_GATE_DELIVERY_TIMEOUT.connect == 30.0
|
||||
|
||||
# SDK (non-native) policy round-trip gate (was 35s).
|
||||
assert scaffold._POLICY_EVAL_TIMEOUT_S == ONE_DAY
|
||||
|
||||
|
||||
def test_no_delivery_budget_undercuts_the_ask_timeout() -> None:
|
||||
"""The real invariant: no delivery client caps the wait below the policy's
|
||||
ASK budget, so the gate is the single thing that decides how long to wait.
|
||||
|
||||
Written relative to ``DEFAULT_ASK_TIMEOUT`` (not a literal) so it keeps
|
||||
holding if the default ASK budget is ever retuned.
|
||||
"""
|
||||
for budget in (
|
||||
pending_approvals._DEFAULT_WAIT_SECONDS,
|
||||
runner_app._ASK_GATE_DELIVERY_TIMEOUT.read,
|
||||
tool_dispatch._ASK_GATE_DELIVERY_TIMEOUT.read,
|
||||
scaffold._POLICY_EVAL_TIMEOUT_S,
|
||||
):
|
||||
assert budget >= DEFAULT_ASK_TIMEOUT
|
||||
Reference in New Issue
Block a user