fix(tools): empty execute_code capability set denies all sandbox tools

[Christopher-Schulze]

What does this PR do?

Fixes a security-hardening gap where execute_code conflated an explicit empty sandbox grant with an omitted grant. An explicit enabled_tools=[] or a non-overlapping grant previously broadened to every sandbox tool in both the local UDS and remote file-RPC paths.

The resolver now preserves the tri-state contract:

• enabled_tools=None → legacy default: all sandbox tools.
• enabled_tools=[] → deny-all.
• non-empty → exact intersection with SANDBOX_ALLOWED_TOOLS.

Related Issue

Part of the SECURITY-AUDIT-42 hardening campaign (EPIC #82591), class SECURITY-CLASS-faf9d60580300e16.

Fixes #84271

Type of Change

• 🔒 Security fix

Changes Made

• tools/code_execution_tool.py now shares _resolve_sandbox_tools() across local UDS and remote file-RPC execution.
• Added direct generated-module _call() boundary tests for empty, non-overlapping, and partial grants on both transports; denied requests never reach the underlying handler.
• Made _sandbox_failure_hint() preserve the same tri-state semantics and report none instead of advertising unavailable tools.
• Kept the detached-HEAD workspace snapshot isolated in the plugin prompt cache regression test.

How to Test

1. scripts/run_tests.sh tests/tools/test_code_execution.py tests/agent/test_plugin_prompt_sections.py → 58 passed.
2. scripts/check.sh --project hermes-agent --worktree worktrees/hermes-agent/84271 → all blocking gates passed (uv lock --check, Ruff, and 58 changed-file tests).
3. On exact origin/main, direct local UDS and remote file-RPC requests with enabled_tools=[] reached their handlers; on this branch, the corresponding tests reject them and record no handler dispatch.

Final verification: head 5fb6edfc786c41efedfa4d73dc5cc817ec1f4b07, rebased onto origin/main 3aee290899e478c5fdfb6a241ef62758a49829b3; the changed-file suite reports 58 passed and scripts/check.sh --project hermes-agent --worktree worktrees/hermes-agent/84271 passes all blocking gates.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/tools/test_code_execution.py -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 15 (arm64)

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Not applicable; this is a Python security fix with automated local and remote RPC regression coverage.

[Enough1122]

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

fix(tools): empty execute_code capability set denies all sandbox tools — solid security fix with good tri-state tests. Observations:

1. Verified the caller side: model_tools.py:1461 substitutes _last_resolved_tool_names only when enabled_tools is None, so an empty list reaching execute_code/_execute_remote is always an explicit grant — the deny-all semantics are sound and no existing caller is accidentally broadened.

2. The enabled_tools is None / else-intersection logic is duplicated in execute_code (~line 1340) and _execute_remote (~line 1079), but only execute_code is covered by the new tests. If the two copies drift, the remote-path regression goes untested. Suggest extracting a shared helper (e.g. _resolve_sandbox_tools(enabled_tools)) and unit-testing it for all three states.

3. Minor: frozenset(SANDBOX_ALLOWED_TOOLS & set(enabled_tools)) — if enabled_tools contains names outside SANDBOX_ALLOWED_TOOLS, the intersection silently shrinks. Deny-all for an empty intersection is the documented intent, but a logger.debug on dropped names would help operators debug "why did my sandbox lose tool X".

[Christopher-Schulze]

Extracted _resolve_sandbox_tools() so the local UDS and remote file-RPC paths share one tri-state helper (None = legacy default, [] / non-overlap = deny-all). Covered by TestResolveSandboxTools. A debug log for dropped names is optional polish and not part of this change.

[Christopher-Schulze]

CI slice 1 failed on test_real_aiagent_builds_section_once_and_keeps_it_out_of_static_prefix (first == rebuilt). The plugin-section cache is correct; the live coding-workspace git snapshot is not. Merge checkouts are detached HEAD, so a second git status can drop the branch line and change the dynamic prompt.

Pinned resolve_context_cwd to a non-repo temp dir in that test so it only covers plugin-section caching. Not related to the execute_code deny-all change.

[andrexibiza]

Code review — changes requested

The shared _resolve_sandbox_tools() helper is the right implementation, but this does not yet satisfy #84271’s security acceptance contract.

1. Please test the actual RPC authorization boundary. The current regressions only prove that a named terminal stub is absent in the local generated module. hermes_tools._call() remains present, so stub omission does not prove the local or remote RPC servers reject a deliberately constructed request. The issue explicitly requires local and remote denial for both terminal and write_file.

Please add local UDS and remote file-RPC tests that call _call() directly under enabled_tools=[] and a non-overlapping grant, assert both tools are rejected, assert the underlying handler is never invoked, and verify a partial grant permits only its exact intersection.

2. Carry the tri-state semantics through _sandbox_failure_hint() during rebase. Current main uses enabled_tools or SANDBOX_ALLOWED_TOOLS. Because the call site supplies the resolved sandbox set, deny-all becomes an empty set and the returned hint incorrectly advertises every sandbox tool as available. Use an explicit is None branch—or the shared resolver—and add a test covering the hint for an empty grant.

The resolver itself looks correct. These are closure and integration gaps around the security boundary, not objections to the central implementation.

[Christopher-Schulze]

Rebased this existing PR onto current main after CI slice 7 reported the unrelated test_gateway_loop_goal_note_when_goal_active base-drift failure. The rebase incorporates the current fix; focused verification now passes: tests/gateway/test_loop_command.py, tests/agent/test_plugin_prompt_sections.py, and tests/tools/test_code_execution.py — 66 tests passed. Updated head: 8a903472bd.

[Christopher-Schulze]

The arm64 Docker job failed before the build because GitHub returned HTTP 429 while downloading the pinned setup-uv action. No source/build error was reached; rerunning failed jobs is restricted to repository administrators.

[Christopher-Schulze]

Rebased once more onto the latest main after the arm64 job hit the GitHub action-download rate limit. The focused loop/plugin/code-execution verification remains 66/66; updated head is 31d7f3670c.

[Christopher-Schulze]

Maintenance update on the existing PR:

• Rebased onto current main at 6f31cfad78.
• Added and verified direct _call() authorization coverage for both local UDS and remote file-RPC: empty and non-overlapping grants reject terminal and write_file without invoking the handler; partial grants dispatch only the exact intersection.
• _sandbox_failure_hint() now preserves the same tri-state semantics and reports no available tools for an empty grant.
• scripts/check.sh --project hermes-agent --worktree worktrees/hermes-agent/84271: all blocking gates passed; changed-file tests 56/56.

The existing branch was updated in place. No new PR was opened.

[Christopher-Schulze]

Maintenance update: rebased onto exact current main (f0187332d1); new head f6f90cd9d1. The rebase preserves _format_interrupted_output together with the shared _resolve_sandbox_tools authorization path. Direct local UDS and remote file-RPC coverage still proves empty/non-overlapping grants deny dispatch and partial grants expose only the intersection. Local verification: 58 changed-file tests and every blocking project gate passed.

[andrexibiza]

Reviewed exact head f6f90cd9d15ad0b94299dced678feb39ade0ac17 against live main@68518c1f9bca11d9f5dbdf59ecf7e024cce057ba, including the complete three-file diff, both sandbox RPC transports, the public dispatch handoff in model_tools.py, the #84271 acceptance contract, existing review thread, exact-head CI, and the current execute-code PR collision set.

Verdict: clear on code at this head. I do not find a remaining blocker in SECURITY-CLASS-faf9d60580300e16.

The important thing this revision gets right is that the capability decision now has one meaning all the way to the mutation boundary:

• _resolve_sandbox_tools() makes the distinction structural rather than truthiness-based: None is the explicitly documented legacy default, while [] and a non-overlapping grant remain deny-all.
• Both execute_code() and _execute_remote() consume that same resolver, so local and remote execution no longer carry two independently drifting interpretations.
• This is not only a stub-generation fix. _rpc_server_loop and _rpc_poll_loop both enforce tool_name not in allowed_tools at dispatch, and the new tests deliberately invoke the generated module's private _call() against the real local/file RPC authorization loops. Empty and non-overlapping manifests reject both terminal and write_file, record zero underlying dispatches, and the partial-grant cases prove exact intersection rather than blanket denial.
• _sandbox_failure_hint() now consumes the same tri-state contract, so deny-all no longer produces an operator-facing lie that every sandbox tool is importable.
• The public handoff in model_tools.handle_function_call() preserves an explicit empty list with enabled_tools if enabled_tools is not None else _last_resolved_tool_names; the repaired sandbox therefore receives the caller's actual empty grant instead of re-broadening it before this PR's resolver sees it.

That closes the two concrete gaps called out earlier on this thread: direct RPC authorization is now tested on both transports, and the diagnostic projection no longer widens an empty capability set. The shared-helper observation from the earlier automated review is also fully absorbed rather than copied twice. Nice repair work, @Christopher-Schulze. 🚀

Exact-object evidence

The exact head is green across the hosted workflows:

• CI 33004900173 — success, including check-attribution, common-ancestor, OSV, and supply-chain jobs.
• Docker 33004897830 — success.
• Nix 33004898028 — success.

The branch is currently 6 commits ahead / 2 behind live main with merge base f0187332d13679e2ace74d1f0eb399eb91a90571. I checked both main-only commits (01e9b9ab and 68518c1f): they touch only Desktop use-background-sync.ts + its test, so the landing drift is path-disjoint from this PR's tools/code_execution_tool.py, tests/tools/test_code_execution.py, and tests/agent/test_plugin_prompt_sections.py. I do not see a semantic reason to churn this security patch solely for those two Desktop ticks; normal landing-edge CI remains the release authority.

Interlocks / merge order

This is the small security invariant I would land before the larger execute-code changes currently in flight. It owns nested sandbox capability admission, not session provenance, output formatting, redaction, kernel lifetime, or container realization:

• #52000 (also @Christopher-Schulze) is complementary nested-RPC session_id provenance and directly touches code_execution_tool.py / test_code_execution.py. When restacked, it should preserve this exact resolved capability set while threading session identity through the same loops.
• #94875 is complementary stable programmatic read_file semantics and also crosses these RPC loops. Its dispatcher-routing work must retain this allow-list before applying the read-mode specialization.
• #95515 is complementary output confidentiality. It changes execute-code redaction after execution; it must not disturb the admission manifest established here.
• #94647 is the larger persistent-kernel feature. Because a kernel outlives one call, this deny-all/partial-grant invariant is even more important there: any reused kernel/RPC server must be keyed or refreshed so it cannot inherit a broader capability set from an earlier cell.
• #90050 is complementary container realization/config projection; no capability ownership transfer belongs there.
• #65592 is complementary approval/AST policy. Approval decides whether Python execution may start; this PR independently decides which Hermes tools that execution may invoke. Neither should stand in for the other.

So I would treat #86148 as the capability-admission base and make later shared-file PRs semantically re-read against it rather than resolving conflicts textually.

The unrelated test_plugin_prompt_sections.py edits are test-harness isolation only: they pin workspace context so detached CI merge checkouts do not make the cache assertion nondeterministic; they do not change runtime behavior.

This is a good example of closing the whole security shape instead of just changing the conditional: grant construction, generated surface, local enforcement, remote enforcement, diagnostics, and exact negative witnesses now agree. 🚀

[Christopher-Schulze]

Maintenance update: rebased onto current main at 3aee290899e478c5fdfb6a241ef62758a49829b3. Final head is 5fb6edfc786c41efedfa4d73dc5cc817ec1f4b07. The canonical checker passes all blocking gates and the changed-file suite is 58/58. The shared tri-state resolver and direct local UDS plus remote file-RPC authorization tests still prove that explicit empty and non-overlapping grants deny dispatch. Please rerun CI on this exact head.