Conversation
- make ACP approvals inherit approvals.timeout when no explicit timeout is provided - allow non-positive ACP timeout values to wait indefinitely - make terminal CLI approval UI honor approvals.timeout instead of a hardcoded 60s limit - add focused regression tests for ACP and terminal CLI approval flows Fixes NousResearch#3765.
5793a3b to
9c99e62
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the configuration gap across CLI and ACP. The CLI portion is already present on current main in 8ae65d5c8 (cli.py:11695), but the ACP work is still useful after a focused port.
Problems
- The change covers only ACP command approvals. Current main also creates an ACP edit approval requester at
acp_adapter/server.py:1425; that requester still defaults to 60 seconds inacp_adapter/edit_approval.py:290and waits with it at:327. - The proposed
timeout <= 0behavior blocks ACP command approvals indefinitely, while the CLI deadline expires immediately for that value (cli.py:11705,:11733). This also differs from the documented fail-closed timeout contract (website/docs/user-guide/security.md:141-147). - Current ACP bridging now schedules through
safe_schedule_threadsafe(acp_adapter/permissions.py:134-150), so the PR's direct scheduler call and scheduler patch in its tests need porting.
Suggested changes
- Salvage the ACP configuration resolution against the current helper, cover both command and edit approval bridges, and define one shared non-positive timeout policy.
Automated hermes-sweeper review.
| loop: asyncio.AbstractEventLoop, | ||
| session_id: str, | ||
| timeout: float = 60.0, | ||
| timeout: float | None = None, |
There was a problem hiding this comment.
Please extend the configured timeout to the ACP edit approval requester as well. Current main constructs that independent bridge in acp_adapter/server.py:1425; it still defaults to 60 seconds in acp_adapter/edit_approval.py:290, so edits would retain the reported bug.
| try: | ||
| future = asyncio.run_coroutine_threadsafe(coro, loop) | ||
| response = future.result(timeout=timeout) | ||
| if timeout is not None and timeout <= 0: |
There was a problem hiding this comment.
An indefinite wait for timeout <= 0 conflicts with the current CLI, which immediately expires a non-positive deadline (cli.py:11705, :11733), and with the documented fail-closed timeout behavior. Define one shared policy before making ACP special-case this value.
Summary
approvals.timeoutfrom config when no explicit timeout is providedapprovals.timeoutinstead of a hardcoded 60s limitProblem
Hermes had two separate 60-second approval timeouts:
This meant
approvals.timeoutinconfig.yamlwas not fully honored, and users could still hit 1-minute expiry in regular terminal sessions even after fixing the ACP path.Test Plan
./venv/bin/python -m pytest tests/acp/test_permissions.py tests/cli/test_cli_approval_ui.py -q -o 'addopts='./venv/bin/python -m py_compile cli.py acp_adapter/permissions.py tests/cli/test_cli_approval_ui.py tests/acp/test_permissions.pyapprovals.timeoutis set higherFixes #3765.