Skip to content

fix(approval): enforce explicit timeout on smart-approval guardian call and log its outcome - #84125

Closed
yflmq001 wants to merge 1 commit into
NousResearch:mainfrom
yflmq001:fix/82846-smart-approve-timeout-log
Closed

fix(approval): enforce explicit timeout on smart-approval guardian call and log its outcome#84125
yflmq001 wants to merge 1 commit into
NousResearch:mainfrom
yflmq001:fix/82846-smart-approve-timeout-log

Conversation

@yflmq001

Copy link
Copy Markdown
Contributor

Complements #72500 (watchdog hard ceiling) — root-cause hardening + observability for the smart-approval guardian call, both gaps the watchdog fix leaves open.

Bug Description

_smart_approve (tools/approval.py:3054) gates every flagged terminal command with a synchronous auxiliary LLM call that (a) never passes an explicit timeout= and (b) logs nothing on the success path and only logger.debug on failure. When a provider response stalls, the agent turn freezes silently — our production hit froze for 62 minutes with zero log output; the gateway kill-switch eventually fired and only an unrelated lifecycle-guard error surfaced afterwards (2026-08-11, Feishu gateway, approvals smart mode, DeepSeek latency spike). See #82846 for the full symptom.

Root Cause

call_llm resolves its own timeout from config (auxiliary.approval.timeout, default 30s), but _smart_approve relies on that default silently — no explicit deadline at the call site, and nothing visible in the logs. #72500 addresses the hang with an outer watchdog thread (45s ceiling), but:

  1. the call site still doesn't pass timeout= — the root cause (internal deadline not enforced at the call boundary) stays,
  2. a normal-path failure remains logged at DEBUG, so the "invisible hang" observability hole from Smart-approval auxiliary LLM call has no enforced timeout — a stalled provider response wedges the entire agent session indefinitely #82846 persists for any non-timeout failure.

Fix

  • tools/approval.py — resolve the same configured value the client would use (_get_task_timeout("approval"), honoring auxiliary.approval.timeout) and pass it explicitly to call_llm; log the assessment call + duration (DEBUG); promote failure logging from DEBUG to WARNING with elapsed time and exception class.
  • Behavior unchanged on success/failure paths: timeout/failure still returns "escalate" (fail open to the human/pattern gate).

How to Verify

PYTHONPATH=. python -m pytest tests/tools/test_smart_approval_policy.py -q -k "explicit_timeout or logs_warning"
  • test_smart_approve_passes_explicit_timeout — asserts call_llm receives timeout=42 when auxiliary.approval.timeout resolves to 42. Verified as a genuine regression: removing the timeout= kwarg makes the test fail.
  • test_smart_approve_failure_logs_warning_and_escalates — asserts a TimeoutError from call_llm yields "escalate" and a WARNING containing the failure prefix + exception class.

Test Plan

  • tests/tools/test_smart_approval_policy.py — 7 passed
  • test_smart_approval_injection.py, test_approval_interrupt.py, test_execute_code_approval_cluster.py, test_denial_circuit_breaker.py — 42 more passed (49 total)
  • One unrelated failure in test_approval.py::test_nonrecursive_verification_artifact_cleanup_is_not_dangerous is pre-existing (fails on clean main, unrelated to this change)

Risk Assessment

Low. No new dependencies, no thread/context changes (unlike the watchdog approach — this stays on the calling thread). The timeout value is the same one call_llm would resolve internally; passing it explicitly only removes the reliance on internal defaulting. Logging level change (DEBUG→WARNING) affects only observability.

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists needs-repro Bug needs reproduction steps labels Aug 11, 2026
…ll and log its outcome

The smart-approval guardian (`_smart_approve`) gates every flagged
terminal command with a synchronous auxiliary LLM call, but it never
passes `timeout=` and logs nothing on the normal path. In production a
stalled provider response silently froze the agent turn for 62 minutes
with zero log output; the gateway kill-switch eventually fired, and only
an unrelated error surfaced afterwards (NousResearch#82846; watchdog-style fix in
NousResearch#72500). The call was invisible by design — nothing logs at the hang
point.

Changes in tools/approval.py:
- Resolve the same configured timeout the client would use internally
  (`auxiliary.approval.timeout` via `_get_task_timeout("approval")`) and
  pass it explicitly to `call_llm`, so the deadline cannot be lost if the
  internal default resolution changes or is misconfigured.
- Log the assessment call and its duration (DEBUG), and promote the
  failure branch from DEBUG to WARNING with elapsed time + exception
  class, so a wedged guardian call is visible in the logs instead of
  silent.
- Failure still returns "escalate" (fail open to the human/pattern
  gate) — behavior unchanged, observability only.

Complements NousResearch#72500 (watchdog hard ceiling) rather than duplicating it:
explicit timeout is the root-cause hardening, logging closes the
silence gap; the watchdog remains the safety net if the SDK-level
timeout itself is defeated.

Tests: explicit timeout forwarded to call_llm (revert-fails), failure
logs WARNING + escalates. 49 approval-adjacent tests pass; one unrelated
test_approval.py failure is pre-existing (fails on clean main too).
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(approval): enforce explicit timeout on smart-approval guardian call and log its outcome

  1. tools/approval.py::_smart_approve imports the private agent.auxiliary_client._get_task_timeout. Depending on an underscore-prefixed internal API couples the approval gate to auxiliary-client internals that may change without notice (the test mocks _get_task_timeout directly, confirming the coupling). Consider exposing a public accessor, or reading the resolved auxiliary.approval.timeout from the same config path, so the contract is explicit.
  2. If _get_task_timeout("approval") resolves to None when auxiliary.approval.timeout is unset, the explicit timeout=None passed to call_llm is no stronger than the default resolution this PR is fixing — the whole point is a guaranteed deadline. Suggest asserting smart_timeout is non-None (or logging a warning when it resolves to None) so an unconfigured timeout cannot silently reintroduce the unbounded-wait bug (Smart-approval auxiliary LLM call has no enforced timeout — a stalled provider response wedges the entire agent session indefinitely #82846).
  3. Good: WARNING-level failure logging with elapsed time and exception class makes a hang visible, and the DEBUG success-path duration is reasonable. The time.monotonic() baseline placed before the imports is harmless.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Salvaged into #93809 with your commit cherry-picked, authorship preserved (#85125 Phase 2c). Your belt-and-suspenders shape was exactly right — explicit timeout from the auxiliary resolution + WARNING-with-elapsed on failure. Verified the enforced value flows through _effective_aux_timeout's explicit-wins path so config and bound stay in lockstep. Closing in favor of #93809 — thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles comp/tools Tool registry, model_tools, toolsets needs-repro Bug needs reproduction steps P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants