Skip to content

fix(approval): route check_execute_code_guard through the CLI fall-through too (salvage of #86270) - #90224

Merged
kshitijk4poor merged 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/86270-execute-code-cli-approval
Aug 19, 2026
Merged

kshitijk4poor merged 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/86270-execute-code-cli-approval

Conversation

@kshitijk4poor

@kshitijk4poor kshitijk4poor commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

execute_code now shows the CLI Dangerous Command panel instead of silently queuing an invisible pending_approval, in the one surface where the user is sitting there able to answer.

Root cause: e37a0321eb fixed the leaked-ask-mode-into-CLI case for _run_approval_gate and check_all_command_guards, but check_execute_code_guard — the whole-script gate, a separate function with its own copy of the same notify_cb is None short-circuit — never got the same treatment. Same leak, same missing fall-through.

Salvage of #86270 by @pierrenode (cherry-picked, authorship preserved), plus a follow-up commit closing three parity gaps found in review.

Changes

  • tools/approval.pycheck_execute_code_guard: when _should_fall_through_to_cli_approval() says yes, run the same hook-fire → prompt_dangerous_approval → hook-fire → choice-branch sequence the sibling guards use. Falls back to the existing pending_approval behaviour when no CLI callback is available. (@pierrenode)
  • tools/approval.py — follow-up parity fixes (review findings):
    • timeout arm now appends the denial-breaker addendum, matching the same function's gateway arm and check_all_command_guards' CLI tail
    • human deny no longer calls _record_denial() — that tally counts guardian LLM DENY verdicts; neither sibling CLI tail advances it, so three deliberate user denials were escalating to breaker hard-stop text
  • tests/tools/test_cli_approval_exec_ask_leak.py — 4 cases mirroring the existing check_all_command_guards pair (approve/deny/timeout/session-persistence) (@pierrenode), plus a platform-marker regression test and two breaker-parity guards; the shared fixture now clears the process-global _denial_tally so a leaked tally can't bleed the escalated addendum into unrelated assertions.

Validation

Side-by-side E2E through the real production caller (_execute_code_handlercheck_execute_code_guard), real imports, isolated HERMES_HOME:

case pre-fix main this branch
CLI panel registered panel never invoked, pending_approval panel invoked, approved: true
tool entry point + deny no denial wording denied, no side-effect file written
secrets in script (panel never shown) panel shown, raw secret not leaked
headless, no CLI callback pending_approval pending_approval (unchanged)
gateway notify_cb present gateway notified gateway notified, CLI panel not hijacked
  • Tests: tests/tools/test_cli_approval_exec_ask_leak.py 11 passed. Wider approval sweep (12 files) 18 failed, 201 passed; the identical 18 failures with identical names occur on pre-fix main (18 failed, 136 passed over the 4 files containing them) — zero new failures. Pre-existing causes are test-order pollution and /tmp symlink resolution, unrelated to this diff.
  • Mutation-checked, four ways: reverting tools/approval.py to base fails exactly the 4 original guards; disabling the fall-through (if False:) fails all 7 execute_code guards while the 4 check_all_command_guards/import tests correctly still pass; re-adding _record_denial fails the parity guard; stripping the timeout addendum fails the addendum guard. Working tree checksum-verified restored after every probe.
  • No regression to the desktop pending-approval replay (#38b9005b95 / #f703e70618): the TUI gateway sets HERMES_INTERACTIVE=1 process-wide, but _wire_callbacks() registers sudo/project/secret callbacks and never set_approval_callbackgrep set_approval_callback tui_gateway/ is empty — so approval_callback is None there, the fall-through cannot fire, and submit_pending → replay is preserved. Confirmed by the gateway E2E case above.
  • Reachability confirmed (not dead code): check_execute_code_guard runs on a tool-worker thread, and agent/tool_executor.py wraps every submit in propagate_context_to_thread, which copies the parent's thread-local approval callback (tools/thread_context.py). Also reachable from the ACP adapter and the slash worker.
  • ruff check clean on both changed files (and clean on base for the same files — no new issues).

Notes

Two items deliberately left out of scope:

  • No shared helper for the three CLI tails. Profiled side by side, only 4 of 10 behavioural axes match: check_all_command_guards loops a (key, desc, is_tirith) warnings list with tirith always→session downgrade and multi-key persistence; _run_approval_gate has no smart-deny gate, no breaker, no _reset_denials, no allow_permanent. A shared helper would need ~8 parameters to save ~30 lines while making all three paths harder to audit — a subset/superset target, not behaviour-equivalent duplication.
  • always persisting the coarse execute_code key is broad (an unrelated later script then auto-approves), but the gateway arm of the same function already does exactly this and did so before this PR. The new branch is consistent with its own function's existing semantics; changing that breadth is a separate scoped change.

Closes #86270


Approval gate infographic — PR #90224

pierrenode and others added 2 commits August 20, 2026 00:00
…rough too

e37a032 fixed _run_approval_gate and check_all_command_guards: when
HERMES_EXEC_ASK (or a session platform marker) leaks into an interactive CLI
process with no gateway notify callback registered, those two functions now
prefer the registered CLI Dangerous Command panel over a silent
pending_approval nobody can see.

check_execute_code_guard — the whole-script gate for execute_code, a
separate function with its own copy of the same notify_cb-less
short-circuit — never got the same treatment. It doesn't even accept an
approval_callback parameter. In the same leaked-ask-mode-into-CLI scenario,
execute_code calls still silently drop into pending_approval with the panel
never shown, even though a CLI callback is registered.

Compute is_cli/approval_callback the same way the two fixed functions do,
and when _should_fall_through_to_cli_approval() says yes, run the same
hook-fire -> prompt_dangerous_approval -> hook-fire -> choice-branch
sequence _run_approval_gate's tail already uses, adapted to this function's
own message/persistence conventions (smart-denied session/permanent
suppression, denial-breaker addendum). Falls back to the existing
pending_approval behavior when no CLI callback is available.

Tests: 4 new cases in tests/tools/test_cli_approval_exec_ask_leak.py
mirroring the existing check_all_command_guards pair (approve/deny/timeout/
session-persistence). Mutation-verified: all 4 fail against the pre-fix code
and pass with it restored.

Neighbor suites: tests/tools/*approval* (291+ tests) and
tests/gateway/{test_approval_prompt_redaction,test_tui_approval_redaction,
test_plaintext_approval_routing,test_discord_exec_approval_content} +
tests/cli/test_cli_approval_ui.py all green. The 7 test_approval_mode_parity
/ test_nonrecursive_verification_artifact_cleanup failures seen in one full
batch run are pre-existing and independent of this change — confirmed by
re-running the identical batch with tools/approval.py stashed back to
pre-fix: the same 7 fail for the same reasons either way.

Note on an adjacent open PR: NousResearch#65592 also touches check_execute_code_guard,
but an earlier, unrelated region of the function (adding an AST dangerous-
operation scanner to the "not is_gateway and not is_ask" auto-approve
branch). No semantic overlap with this fix's notify_cb-less branch; a small
rebase may be needed depending on merge order.
…ng guards

Follow-up to the salvaged fix. Three parity gaps in the new CLI branch:

- Timeout arm dropped the denial-breaker addendum that the same function's
  gateway arm and check_all_command_guards' CLI tail both append, so a
  tripped breaker went unreported on a timeout.
- Human deny called _record_denial(), advancing a tally scoped to guardian
  LLM DENY verdicts. Neither sibling CLI tail does this, so three
  deliberate user denials escalated to breaker hard-stop text.
- The platform-marker half of the leak (HERMES_SESSION_PLATFORM set, no
  HERMES_EXEC_ASK) was unpinned; it reaches the same branch.

Adds a platform-marker regression test plus two breaker-parity guards, and
clears the process-global _denial_tally in the shared fixture so a leaked
tally can't bleed the escalated addendum into unrelated assertions.
@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/code-exec execute_code sandbox tool/terminal Terminal execution and process management area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 19, 2026
@kshitijk4poor
kshitijk4poor merged commit f0ffcbc into NousResearch:main Aug 19, 2026
50 checks passed
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/code-exec execute_code sandbox 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