Skip to content

fix(approval): isolate long command classification - #101965

Open
lemontree9223 wants to merge 1 commit into
NousResearch:mainfrom
lemontree9223:fix/command-guard-worker-timeout
Open

lemontree9223 wants to merge 1 commit into
NousResearch:mainfrom
lemontree9223:fix/command-guard-worker-timeout

Conversation

@lemontree9223

@lemontree9223 lemontree9223 commented Sep 3, 2026

Copy link
Copy Markdown

What does this PR do?

Long shell commands can drive command normalization and dangerous-command detection through expensive Python parser/regex paths. In a single-process gateway, a pathological input can monopolize the interpreter and stall unrelated messaging work.

This change moves only the pure classification of long commands into a subprocess with a 10-second hard deadline. Approval prompts, session state, YOLO/mode decisions, and the final allow/block decision remain in the parent process. Worker timeout, process failure, malformed JSON, and invalid per-verdict schemas all fail closed.

The existing parent-side parser cap remains the first cheap rejection step. Commands shorter than 4,096 characters keep the existing in-process path.

This is adjacent to, but does not duplicate:

Related Issue

Related: #7485, #44473, #72500

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • Add tools/approval_guard_worker.py for the parent-side subprocess protocol and tools/command_guard_worker.py for classification over stdin.
  • Classify commands of at least 4,096 characters in a subprocess with a 10-second deadline. Short commands retain the in-process path.
  • Integrate the worker through tools/approval.py, tools/approval_detection.py, and tools/approval_floors.py; approval decisions and mutable session state remain in the parent.
  • Snapshot profile-scoped sudo-password presence and deny patterns; propagate the active Hermes home. Use explicit UTF-8 for the protocol.
  • Validate verdict fields and restrict verdict kinds by full/user-deny mode. Operational and protocol errors block rather than falling back to inline classification.
  • Add regression tests for a real Unicode subprocess round trip, avoiding parent-side reclassification, mode-invalid verdicts, and timeout blocking even with YOLO enabled.

Review follow-up

The earlier automated review referenced an older revision. The current parent error includes only the worker exit code, not captured stderr; the worker reports only the exception type. No persistent-worker or caching architecture has been added.

Validation for the current revision

Current head: 6fee02dce4cab50b801eafdd0325eb16a3f59350.

The commands below have been updated to paths present at this head. They are commands to run, not new passing results. Tests, lint, and compilation were not rerun as part of this description-only update. Previously reported pass counts and root E2E results belong to earlier validation and must not be treated as evidence for this head.

scripts/run_tests.sh tests/tools/test_command_guard_worker.py tests/tools/test_approval.py tests/tools/test_approval_deny_rules.py tests/tools/test_approval_mode_parity.py tests/tools/test_approval_outcome_parity.py tests/tools/test_single_query_approval_mode.py tests/tools/test_cron_approval_mode.py tests/tools/test_smart_approval_policy.py
ruff check tests/tools/test_command_guard_worker.py tools/approval.py tools/approval_detection.py tools/approval_floors.py tools/approval_guard_worker.py tools/command_guard_worker.py
python -m py_compile tests/tools/test_command_guard_worker.py tools/approval.py tools/approval_detection.py tools/approval_floors.py tools/approval_guard_worker.py tools/command_guard_worker.py
git diff --check

At the latest status check, this head had no check runs or commit-status results. The repository requires All required checks pass, which is not yet satisfied; absence of results is not a confirmed test failure. Full-suite success is not claimed. Earlier local full-suite collection was blocked by the optional ACP dependency.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Sep 3, 2026
@lemontree9223
lemontree9223 force-pushed the fix/command-guard-worker-timeout branch from 732f885 to 759e962 Compare September 3, 2026 08:57
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Overall: Isolates long-command classification out of the gateway process: commands ≥4096 chars run the pure checks (hardline, sudo-stdin, user deny globs, dangerous verdict) in a tools.command_guard_worker subprocess over stdin (never argv) with a 10s hard deadline (approval.py:277); every operational/protocol failure fails closed to an unconditional block with a "don't retry inline, use write_file/patch" message.

Correctness:

  • Security posture preserved: cheap parser cap still enforced in-parent first; unconditional guards still precede yolo/mode-off (the moved block in check_all_command_guards keeps ordering); worker verdicts are schema-validated (kind + required string fields) with unknown kinds failing closed.
  • Secret hygiene: only sudo_password_configured bool + deny patterns cross into the worker; HERMES_HOME propagated for path rewrites; command travels over stdin.
  • Short-command path is byte-for-byte the old logic (extracted _match_user_deny_globs shared by both).

Non-blocking nits:

  • Each long command spawns a Python subprocess (~100ms+); bursts of long commands could add latency — consider a persistent worker or caching by command hash if this shows up in profiles. Correctness-first is the right default for a guard.
  • completed.stderr[-200:] in the failure message (approval.py:323) could echo part of the command if the worker traceback includes stdin content; truncating to the last line or stripping to the exception type would be safer.
  • _ISOLATED_GUARD_MIN_CHARS (4096) coincides with _MAX_SEPARATOR_FREE_COMMAND_CHARS — confirm the equality is intentional vs coincidental, with a comment linking them if so.

Verdict: LGTM. Strong fail-closed design; nits are perf/hardening follow-ups.

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 comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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.

3 participants