fix(approval): route check_execute_code_guard through the CLI fall-through too

[pierrenode]

What does this PR do?

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 is None 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.

The fix

Compute is_cli/approval_callback the same way the two already-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.

Testing

• 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 green: tests/tools/*approval* (291+ tests), 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.
• 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 (test-order pollution / /tmp symlink resolution, unrelated to this diff).
• ruff check clean.

Adjacent open PR (checked, no semantic overlap)

#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 is None branch; a small rebase may be needed depending on merge order.

Type of Change

• Bug fix (non-breaking change that fixes an issue)

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(approval): route check_execute_code_guard through the CLI fall-through too

Closing the sibling leak in check_execute_code_guard is the right fix, and the tests cover the important branches (approve/deny/timeout/session-cache/headless-pending). Observations:

1. tools/approval.py (check_execute_code_guard fall-through block): the new code duplicates the CLI approval handling that check_all_command_guards already implements — pre/post _fire_approval_hook, prompt_dangerous_approval, the timeout/deny/session/always branch logic, and _reset_denials. The PR itself notes the two are "same class". Extracting the CLI prompt-and-choice-processing into a shared helper would stop the two guards from drifting (e.g. one gains a new outcome string the other doesn't handle).

2. The "always" branch persists a permanent allowlist entry for the matched pattern_key — for whole-script execute_code this permanently whitelists that pattern on the user's machine. Verify pattern_key here is the same stable identity used by the command guard (so always decisions are consistent across both guards), and that the smart_denied_for_owner interplay (allow_permanent vs smart-deny) mirrors check_all_command_guards exactly.

3. Test coverage drives the fall-through via HERMES_EXEC_ASK, but the leak scenario is also described as "sometimes a session platform marker" — that non-env-var trigger for _should_fall_through_to_cli_approval isn't covered. A test exercising the marker path (interactive CLI, callback registered, no HERMES_EXEC_ASK) would pin the fix against the other half of the leak.

[kshitijk4poor]

Merged via #90224 — #90224

Your commit was cherry-picked with authorship preserved and rebase-merged, so it's on main under your name as 16af3bed8c.

Thanks for catching this — the sibling-guard framing was exactly right. check_execute_code_guard genuinely was the last copy of that notify_cb is None short-circuit left unfixed after e37a0321eb, and your test structure (mirroring the existing check_all_command_guards pair) mutation-verified cleanly: reverting tools/approval.py to base fails exactly your 4 new cases and nothing else.

Verification that backed the merge, in case it's useful:

• Side-by-side E2E through the real caller (_execute_code_handler → check_execute_code_guard) on pre-fix main vs your branch confirmed the panel goes from never-invoked to invoked, that a deny actually blocks the script (no side-effect file written), that secrets in the script stay redacted in the panel text, and that the headless pending_approval contract is unchanged.
• No regression to the desktop pending-approval replay: the TUI gateway sets HERMES_INTERACTIVE=1 process-wide, but _wire_callbacks() never calls set_approval_callback, so approval_callback is None there and the fall-through can't fire.
• Your note about the 7 pre-existing failures was right, and slightly understated — the wider approval sweep shows 18 pre-existing failures with identical names on both sides of the diff. Zero new ones.

I added one follow-up commit on top rather than sending it back to you, closing three parity gaps found in review:

1. The timeout arm was missing the denial-breaker addendum that the same function's gateway arm and check_all_command_guards' CLI tail both append.
2. The deny arm called _record_denial(). That tally counts guardian LLM DENY verdicts (per its own docstring); neither sibling CLI tail advances it on a human deny, so three deliberate user denials were escalating to breaker hard-stop text.
3. Added a test for the platform-marker half of the leak — the case your comment described as "sometimes a session platform marker." It does reach the same branch with HERMES_EXEC_ASK entirely absent, and now it's pinned.

On the automated review's first observation (extracting a shared helper for the three CLI tails): I looked at it and deliberately left it alone. 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, while _run_approval_gate has no smart-deny gate, no breaker, and no _reset_denials. A shared helper would need roughly 8 parameters to save ~30 lines. Your inline version was the right call.