Skip to content

fix: honor approval timeout config in CLI prompt#17933

Open
hanzckernel wants to merge 3 commits into
NousResearch:mainfrom
hanzckernel:fix/cli-approval-timeout-config
Open

fix: honor approval timeout config in CLI prompt#17933
hanzckernel wants to merge 3 commits into
NousResearch:mainfrom
hanzckernel:fix/cli-approval-timeout-config

Conversation

@hanzckernel

Copy link
Copy Markdown
Contributor

Classic CLI dangerous-command approvals now use the existing approvals.timeout config instead of a hardcoded 60-second deadline.

Changes

  • Read approvals.timeout when building the prompt_toolkit approval deadline.
  • Add a shared timeout coercion helper for approval paths.
  • Fall back to defaults for malformed approvals blocks and unsafe timeout values.
  • Cover CLI callback, legacy prompt, and combined guard paths with regression tests.

Verification

Passed:

scripts/run_tests.sh tests/cli/test_cli_approval_timeout.py tests/tools/test_approval.py -q
/Users/hanzhicheng/.hermes/hermes-agent/venv/bin/python -m py_compile cli.py tools/approval.py
scripts/run_tests.sh tests/cli tests/tools/test_approval.py -q
git diff --check origin/main...HEAD

Results:

  • 139 approval-focused tests passed.
  • 715 CLI + approval tests passed.
  • py_compile passed for cli.py and tools/approval.py.
  • git diff --check passed.

Known:

  • A full local scripts/run_tests.sh tests -q run did not complete on my macOS checkout because pytest hit OSError: [Errno 24] Too many open files; targeted approval/CLI coverage above is green.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles labels Apr 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #13088 — both fix approvals.timeout being ignored in CLI (hardcoded 60s). Check which has broader scope.

@hanzckernel

Copy link
Copy Markdown
Contributor Author

Checked against #13088. There is overlap on the classic CLI hardcoded 60s approval timeout, but this PR is the cleaner narrow fix: current/mergeable, less stale cli.py churn, and safer approvals.timeout parsing with targeted tests.

#13088 also covers ACP, which this PR does not. If maintainers want that in scope, I can add a small ACP follow-up/commit instead of pulling in the stale #13088 diff.

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants