fix(execute_code): forward session_id to nested tool calls via RPC

[Christopher-Schulze]

What does this PR do?

• _rpc_server_loop and _rpc_poll_loop dispatched nested tool calls (e.g. read_file invoked by execute_code) via handle_function_call without passing session_id. Plugin hooks on_pre_tool_call / on_post_tool_call therefore saw an empty session_id and could not correlate nested calls with the originating turn.
• Follow-up for review feedback: the public registry path now preserves the explicit dispatcher contract too. registry.dispatch("execute_code", ..., session_id="...") forwards that value through the execute_code registry handler instead of dropping it.
• execute_code() now accepts an explicit session_id, forwards it into both local (UDS/TCP) and remote (file-based) sandbox RPC paths, and only falls back to the legacy session context when no explicit session id is supplied.
• Current main's per-session rpc_token authorization remains intact in both loop signatures, thread arguments, request payloads, and constant-time validation.
• Added regression tests for the registry-dispatch contract, local RPC forwarding, remote RPC forwarding, and default empty-string compatibility.

Related Issue

Fixes #51931

Type of Change

• Bug fix
• Tests

Changes Made

• tools/code_execution_tool.py: threads explicit session_id through the registry handler and both nested RPC transports while preserving current rpc_token authorization.
• tests/tools/test_code_execution.py: covers registry and local RPC forwarding.
• tests/tools/test_code_execution_modes.py: covers remote file-RPC forwarding and the compatibility default.
• tests/tools/test_approved_command_clean_slate.py: keeps the existing remote-dispatch regression stub aligned with the extended _execute_remote signature.

How to Test

• Rebased onto exact origin/main 07be37d99; local ancestry and diff checks are clean
• scripts/check.sh --project hermes-agent --worktree worktrees/hermes-agent/51931 - all blocking gates passed
• Changed tests via scripts/run_tests.sh: 123/123 passed across all three changed test files
• TestRpcSessionIdForwarding - 4/4 passed
• Base-regression proof: all 4 final regressions fail on exact origin/main and pass on this branch
• Repository-wide Ruff and uv lock --check - clean
• ty check remains advisory: focused ty output only shows existing diagnostics in the touched file/tool area; full-project ty still panics on pre-existing tools/checkpoint_manager.py, outside this change

Checklist

• The PR is limited to session forwarding and its regression tests.
• Current-main RPC-token authorization is preserved.
• All blocking project checks pass on the rebased branch.
• No dependency, lockfile, configuration, or documentation change is required.

[alt-glitch]

This was generated by AI during triage.

Active salvage for #51931: #52000 now covers the registry-dispatch contract and both RPC transports while retaining current rpc-token authorization. #52001 is closed, so this is related work rather than an active competing duplicate.

[harjothkhara]

I compared this with #52001 against current main (77d2b5075) because the earlier triage comment marks this PR as the chosen one by timestamp. I noticed one mismatch in the contract: current model_tools.handle_function_call already passes session_id into registry.dispatch(...) for execute_code, but the current registry handler in tools/code_execution_tool.py drops it and only calls execute_code(code=..., task_id=..., enabled_tools=...).

This PR forwards a session id inside _rpc_server_loop / _rpc_poll_loop, but it derives that value from get_session_env("HERMES_SESSION_ID", "") rather than threading the dispatcher kwarg through the registry handler. That likely covers many CLI/gateway turns where the ContextVar/env is synchronized, but it does not cover the explicit registry/dispatcher contract: a direct registry.dispatch("execute_code", ..., session_id="session-contract") on current main calls execute_code without session_id at all.

So I think the safer merge shape is either #52001's approach, or this PR plus the same handler/signature threading and a registry-level regression test. That would cover both local/remote RPC internals and the public dispatch path that originally receives the session_id.

[Christopher-Schulze]

Addressed the registry-dispatch gap called out above in commit 88b042f75.

What changed:

• registry.dispatch("execute_code", ..., session_id=...) now forwards the explicit session_id through the execute_code registry handler.
• execute_code(...) carries that value into both local and remote sandbox RPC paths, with the existing session-context lookup kept as fallback only.
• Added a registry-level regression test; it fails on current origin/main with session_id=None and passes on this branch.

Validation is in the updated PR body: ./scripts/check.sh blocking gates passed and tests/tools/test_code_execution.py is 73/73 green.

[teknium1]

Thanks for tracing this through both RPC transports. Current main still passes session_id into the execute_code registry dispatch at model_tools.py:1265-1270, while the registered handler drops it at tools/code_execution_tool.py:1903-1906; both nested dispatch sites also pass only task_id at tools/code_execution_tool.py:588-590 and 870-872. The proposed propagation covers those verified gaps.

Current main added the per-session rpc_token parameter and thread argument to both RPC paths (tools/code_execution_tool.py:487-496, 763-773, 984-991, and 1270-1277) after this PR's base. Salvage should retain that token while adding the new session argument.

Automated hermes-sweeper review.

[Christopher-Schulze]

Rebased and verified in 94d89ebbb. Current main's per-session rpc_token remains intact in both RPC loop signatures, thread arguments, sandbox request payloads, and secrets.compare_digest authorization checks; session_id is added after the token and forwarded independently.

All 4 final regressions fail on exact origin/main (8fa8aabbb) and pass on this branch. The complete changed test files pass 114/114, and all blocking harness gates pass.

[Christopher-Schulze]

Maintenance update

Rebased onto current main and re-verified focused tests. CI should re-run on the new head.

[Christopher-Schulze]

CI retrigger

Previous Python tests / e2e failed during setup-uv (fetch failed from the versions manifest) — setup infrastructure, not the test suite. All Python test slices, Docker builds, and Desktop E2E had already passed.

Pushed an empty commit (6ff17d6ac) to re-run required checks.

[Christopher-Schulze]

Rebased onto current main (fa83af3f9). The only conflict was registry wiring: kept main's _execute_code_handler (command/non-string recovery) and forwarded session_id=kwargs.get("session_id") into execute_code so nested RPC hooks still see the parent session.

Targeted: tests/tools/test_code_execution.py + tests/tools/test_code_execution_modes.py — 83 passed.