fix(agent): measure batch-deadline exclusion at the human wait, not authorization-gate residency - #80297
Merged
kshitijk4poor merged 3 commits intoAug 6, 2026
Conversation
…uthorization-gate residency A tool wedged inside _ConcurrentToolAuthorizationGate hung the whole turn forever (NousResearch#79719): excluded_seconds() measured residency in gate.run() — arbitrary code — so an open window grew 1:1 with wall clock and the batch deadline's remaining was constant (remaining = deadline - window_started; now cancels out). A hanging pre_tool_call plugin or an approval round-trip to a dead client defeated the deadline entirely. The serialization lock was also an unbounded acquire, so every other worker needing authorization parked behind the wedged holder forever. Fix, in two halves: - tools/approval.py grows per-session human-wait accounting (human_wait_window / human_wait_seconds). The two places that are verifiably blocked on a HUMAN — the CLI approval prompt and the gateway approval poll loop — mark their own windows. Both are intrinsically bounded by approvals.timeout; the open-window read is additionally clamped to that timeout plus a margin as belt-and-braces. - _ConcurrentToolAuthorizationGate keeps only serialization, with a bounded acquire (approvals.timeout + 60s; on expiry the prompt runs unserialized — the same degradation the start-order gate accepted in NousResearch#79705). excluded_seconds() becomes a baseline-delta read of the session's human-wait total. A wedged plugin now contributes nothing to the exclusion, so the batch times out at the normal deadline with correctly labeled results, while a genuine approval wait — which can legitimately exceed any fixed bound — still extends the deadline in full. E2E (real AIAgent, worktree imports): wedged-plugin batch on main never ends (>30s observed, 3s deadline); with the fix it ends at 3.0s. A 4s simulated approval over a 2s deadline completes without a timeout label. Closes NousResearch#79719
Review-driven follow-up to the NousResearch#79719 fix: - Clamp the CLOSE-side accrual too: a wedged window that eventually closed used to inject its full unclamped overstay into completed_seconds, retroactively extending a running batch's deadline by hours. Both clamps now share one ceiling helper (_human_wait_ceiling = approvals.timeout + HUMAN_WAIT_MARGIN_S), and the gate's lock-timeout uses the same margin constant so the bounds cannot drift apart. - Evict idle sessions until the table is under the cap (was: at most one per insert, so churn could outgrow _HUMAN_WAIT_MAX_SESSIONS). Entries with an open window are still never evicted. - Log (debug) instead of silently swallowing a failed session-key snapshot in the gate constructor. Tests: close-side clamp regression + table-cap assertion added; suite at 17 passed.
- Single source for the approval-derived bound: public human_wait_ceiling() in tools/approval.py; the gate's lock-timeout helper delegates to it instead of re-deriving timeout + margin (was duplicated in two modules and reached for a private _get_approval_timeout). - Shared _clamped_window_seconds() for the close-time accrual and the open-window read, so the two clamps are identical by construction. - Gate __init__ grows session_key kwarg; tests construct via the real constructor instead of mutating privates post-hoc. - Gateway test resolves its pending approval via resolve_gateway_approval() (the production /deny path) instead of hand-rolling queue-entry internals. - Docstring accuracy: human_wait_seconds monotonicity caveat under cap eviction; s/pre_tool_block/pre_tool_call/ hook name.
kshitijk4poor
enabled auto-merge (rebase)
August 6, 2026 11:30
This was referenced Aug 6, 2026
kshitijk4poor
added a commit
to kshitijk4poor/hermes-agent
that referenced
this pull request
Aug 24, 2026
… sudo human-wait exclusion (NousResearch#85125 2e) Closes the NousResearch#81048 residue on current main (the CLI/TUI/ACP classification itself landed in aac74be / PR NousResearch#77234): - _run_approval_gate GATEWAY tail: timeout vs deny wording was already distinct but the returned dict carried NO outcome key — it now sets outcome='timeout'/'denied' + user_consent + deny_reason, matching the check_all_command_guards (:5043) and execute_code (:5588) siblings. - notify-failure returns (gate :3780 + guards :5000) now carry outcome='notify_failed', matching the execute_code sibling. - is_interrupted() arm: deliberately NOT changed — the per-thread interrupt flag carries only free-text reasons and cannot reliably distinguish a deliberate /stop from a gateway inactivity timeout; both intentionally resolve as 'deny' per NousResearch#8697. Documented in-line; changing it needs a dedicated interrupt-cause channel, not string matching. - sudo human-wait exclusion (the unaddressed NousResearch#80297-family item): the interactive sudo-password wait — both the CLI-callback path and the thread.join path — is now wrapped in human_wait_window(), so it stops counting against tool deadlines on BOTH executor paths (exclusion is consumed at agent/tool_executor.py:490-517 concurrent and :884 sequential from the same accrual). Tests: tests/tools/test_approval_outcome_parity.py (sudo wait accrues human-wait seconds on the callback path; join-path wrap structural). tests/run_agent/test_authorization_gate.py + test_approval_interrupt.py: 18 passed. test_approval.py: failure set identical to upstream/main baseline (11 pre-existing environmental, zero new). ruff clean. Out of scope per tracker: approvals.mode semantics, allowlist, dangerous-command parsing — untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A tool wedged inside the concurrent authorization gate no longer hangs the turn forever: the batch deadline now excludes only verifiable human approval waits (measured at the source, in
tools/approval.py), not residency in the gate — so a hangingpre_tool_callplugin or a dead approval client times the batch out normally instead of defeating the deadline entirely.Root cause (#79719):
excluded_seconds()measured wall-clock residency ingate.run()— arbitrary code — and the deadline loop adds it to the deadline on every poll. With a window open,remaining = (deadline + (now − window_started)) − now = deadline − window_started: constant, so the deadline never fired. The serialization lock was also an unbounded acquire, parking every other worker behind the wedged holder.Changes
tools/approval.py: per-session human-wait accounting (human_wait_window/human_wait_seconds). The two places genuinely parked on a human — the CLI approval prompt (prompt_dangerous_approval) and the gateway approval poll loop (_await_gateway_decision) — mark their own windows. Overlapping windows coalesce; per-window contribution is clamped (read-side AND close-side) toapprovals.timeout + 60s, since every legitimate wait self-terminates atapprovals.timeout; table capped at 256 sessions with idle-only eviction.agent/tool_executor.py:_ConcurrentToolAuthorizationGatekeeps only serialization, with a bounded acquire (approvals.timeout + 60s; on expiry the prompt runs unserialized — the same degradation the start-order gate accepted in fix(agent): bound the concurrent start-order gate under the batch deadline (salvages #79571) #79705).excluded_seconds()is a baseline-delta read of the session's human-wait total.tests/run_agent/test_authorization_gate.py: 17 tests — tracker semantics, session isolation, both clamps, eviction cap, lock-timeout degradation, the issue's exact degenerate-arithmetic repro (remaining must converge), and instrumentation checks for both approval paths. The gate previously had zero test coverage.Validation
pre_tool_blockplugin, 3s batch deadline (realAIAgentE2E)Targeted suites: 17/17 new + 3/3 start-order gate + 19/19
Concurrent*intest_run_agent.py+ approval suites green (1 pre-existing failure on clean main, unrelated).tests/tools/full sweep diffed against an upstream/main baseline worktree: no new failures. No new config keys (reusesapprovals.timeout), no env vars, no message/prompt mutation.Credit
@x7peeps (#79755) and @RelaxJonh (#80134) both proposed bounding the gate for this issue — #79755's bounded acquire + approval-timeout-derived ceiling overlaps the serialization half here. This PR goes one step further and removes gate residency from the deadline arithmetic entirely, so the exclusion only ever tracks a pending human prompt.
Closes #79719