feat(toolsets): code_execution toolset ("code mode") to cut tool-call token cost - #2333
naomi-robusta wants to merge 12 commits into
Conversation
…ll tokens Adds an opt-in `code_execution` toolset whose `run_python_code` tool lets the LLM write one Python script that composes many read-only Holmes tools and filters results in code, so only what it print()s enters the model context — cutting token cost on multi-step investigations. - Subprocess execution with a unix-socket bridge: the script's `holmes` object relays each tool call back to the parent, which dispatches into the real ToolExecutor. Credentials never leave the parent process. - Read-only surface only: is_core, bash/kubectl_run, and approval-gated tools are excluded from the generated client; an APPROVAL_REQUIRED result at dispatch is denied (a synchronous script cannot pause for approval). - Reuses the ulimit memory cap; oversized stdout flows through the existing spill-to-disk guard in the loop. Off by default (feature-flagged). - ToolCallingLLM gains a generic set_tool_executor wiring hook. Tests: 21 unit/integration tests (real subprocess) covering eligibility, call consolidation, result filtering, and failure modes (syntax/runtime error, timeout, unavailable/approval-gated/erroring sub-tools, unwired executor, timeout clamping). Design: relay/docs/design/2026-07-28_holmes-code-mode.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an opt-in Python code execution toolset. User scripts run in a subprocess and call eligible Holmes tools through a Unix socket bridge, with timeout handling, structured results, dynamic instructions, executor wiring, and integration tests. ChangesCode mode execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ToolCallingLLM
participant CodeExecutionToolset
participant ToolCallBridge
participant runner.py
participant ToolExecutor
ToolCallingLLM->>CodeExecutionToolset: wire shared ToolExecutor
CodeExecutionToolset->>ToolCallBridge: start bridge and runner process
runner.py->>ToolCallBridge: request eligible tool call
ToolCallBridge->>ToolExecutor: dispatch tool and params
ToolExecutor-->>ToolCallBridge: return structured result
ToolCallBridge-->>runner.py: send JSON response
runner.py-->>CodeExecutionToolset: return script output and status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:3bb208bb2
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:3bb208bb2 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:3bb208bb2
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:3bb208bb2
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:3bb208bb2
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:3bb208bb2 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:3bb208bb2
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:3bb208bb2Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:3bb208bb2 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:3bb208bb2Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:3bb208bb2 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:3bb208bb2 |
|
The Benchmark Master and Benchmark PR checks both fail with the identical This change adds a new opt-in, off-by-default toolset, so I'm not touching the benchmark. I'm watching the actual unit-test gate ( Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 216-237: Avoid invoking _wire_toolset_executors during every
ToolCallingLLM construction, especially clones created by with_executor(),
because it mutates shared toolset state and can race across requests. Move
wiring to the owning initialization path or otherwise ensure each request
receives isolated toolset instances before set_tool_executor is called, while
preserving sibling tool dispatch behavior.
In `@holmes/plugins/toolsets/code_execution/code_execution_toolset.py`:
- Around line 337-345: Make the executor association used by run_python_code
request-scoped rather than mutating shared CodeExecutionToolset state in
set_tool_executor. Update the ToolCallingLLM wiring and relevant execution path
so concurrent with_executor() requests cannot overwrite each other’s executor,
while preserving instruction rendering with the current executor context.
- Around line 1-12: Update the subprocess environment construction in the code
execution toolset to use an explicit minimal allowlist instead of merging
os.environ. Preserve only the required runtime variables, such as PATH and the
configured Python/runtime settings, while excluding credentials and unrelated
parent environment values; apply the same change to the additional environment
construction around the second referenced location.
- Around line 302-305: Update get_parameterized_one_liner to treat a null code
value the same as a missing or empty value before calling string methods,
returning an empty one-liner safely while preserving the existing first-line
truncation behavior for valid strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 70806f45-45cb-4ad0-aab1-a79b18477673
📒 Files selected for processing (10)
holmes/core/tool_calling_llm.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/code_execution/__init__.pyholmes/plugins/toolsets/code_execution/bridge.pyholmes/plugins/toolsets/code_execution/client_generator.pyholmes/plugins/toolsets/code_execution/code_execution_config.pyholmes/plugins/toolsets/code_execution/code_execution_toolset.pyholmes/plugins/toolsets/code_execution/runner.pytests/plugins/toolsets/code_execution/test_code_execution.pytests/plugins/toolsets/code_execution/test_code_execution_wiring.py
| self._wire_toolset_executors() | ||
|
|
||
| def _wire_toolset_executors(self) -> None: | ||
| """Give toolsets that opt in (via a ``set_tool_executor`` method) a | ||
| back-reference to the ToolExecutor so they can dispatch sibling tool | ||
| calls — used by the code_execution ("code mode") toolset. Generic hook: | ||
| no code-mode-specific coupling here. | ||
| """ | ||
| toolsets = getattr(self.tool_executor, "toolsets", None) | ||
| if not isinstance(toolsets, (list, tuple)): | ||
| return # e.g. a mocked tool_executor in tests | ||
| for toolset in toolsets: | ||
| setter = getattr(toolset, "set_tool_executor", None) | ||
| if callable(setter): | ||
| try: | ||
| setter(self.tool_executor) | ||
| except Exception: | ||
| logging.warning( | ||
| "failed wiring tool_executor into toolset %s", | ||
| getattr(toolset, "name", "?"), | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wiring on every construction mutates shared toolset state — see cross-file race in code_execution_toolset.py.
_wire_toolset_executors() runs on every ToolCallingLLM.__init__, including the per-request clones created by with_executor() (Line 239-252, whose docstring says it exists to avoid "mutating the shared ToolCallingLLM instance"). Since it writes into the toolset objects themselves (not into this new instance), and those toolset instances appear to be reused across requests, concurrent with_executor() calls can race on CodeExecutionToolset._tool_executor. See consolidated comment for the full analysis and fix options.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@holmes/core/tool_calling_llm.py` around lines 216 - 237, Avoid invoking
_wire_toolset_executors during every ToolCallingLLM construction, especially
clones created by with_executor(), because it mutates shared toolset state and
can race across requests. Move wiring to the owning initialization path or
otherwise ensure each request receives isolated toolset instances before
set_tool_executor is called, while preserving sibling tool dispatch behavior.
CodeExecutionConfig must subclass ToolsetConfig (not BaseModel) so it implements build_schema_entry(); otherwise holmes_sync_toolsets_status -> get_config_schema() raises AttributeError, failing test_toolsets_dumpable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
…st-scoped executor
- Security (critical): the subprocess no longer inherits the parent os.environ.
A minimal allowlist (PATH/locale + the bridge handoff vars) is built instead,
so LLM-authored code can't read Holmes's credentials. Matches the "credentials
never leave the parent" contract. Adds a regression test.
- Stability: get_parameterized_one_liner tolerates {"code": null} (runs outside
the invoke try/except, so it must not raise). Adds a test.
- Concurrency: run_python_code resolves the ToolExecutor from the request-scoped
ToolInvokeContext.tool_executor (set by the loop) instead of shared toolset
state, so concurrent with_executor() clones can't race. set_tool_executor is
kept only for rendering llm_instructions.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU
Signed-off-by: Claude <noreply@anthropic.com>
|
Thanks for the review — all four findings were valid and are addressed in
All 23 code-execution tests + the Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holmes/core/tools.py`:
- Around line 268-271: Exclude the request-scoped tool_executor field from
ToolInvokeContext.model_dump() serialization by declaring it with
Field(exclude=True), preserving its runtime availability while preventing
executor, toolset, and configuration state from appearing in dumped context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8093bebb-446a-4bd0-a4e3-3efb1d57fe03
📒 Files selected for processing (4)
holmes/core/tool_calling_llm.pyholmes/core/tools.pyholmes/plugins/toolsets/code_execution/code_execution_toolset.pytests/plugins/toolsets/code_execution/test_code_execution.py
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/core/tool_calling_llm.py
- holmes/plugins/toolsets/code_execution/code_execution_toolset.py
- tests/plugins/toolsets/code_execution/test_code_execution.py
…dump Mark ToolInvokeContext.tool_executor with Field(exclude=True) so the executor (and the toolset/config state it references) never appears in serialized/logged context. Runtime access is unchanged. Adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
Move the code-mode design doc into the holmesgpt repo (docs/design/) alongside the implementation, instead of tracking it in the relay repo. This keeps the design and its implementation in a single PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/2026-07-28_holmes-code-mode.md`:
- Around line 274-280: Update both fenced documentation blocks in the design
document, including the block containing the holmes module listing and the block
around the additional referenced section, to specify an appropriate language
identifier such as text. Preserve the existing diagram and code-like content
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d126761-2abb-47dc-bb7e-94e25ed81901
📒 Files selected for processing (1)
docs/design/2026-07-28_holmes-code-mode.md
Specify 'text' on the two bare code fences (module listing + data-flow diagram) so markdownlint MD040 passes and rendering stays consistent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
Two ask_holmes evals exercising the new code_execution toolset as an A/B matrix over toolset config (toolsets_matrix): - 285_code_mode_count_configmaps: count ConfigMaps across namespaces ([default] classic vs [codemode]). Targets the call-consolidation sink. - 286_code_mode_large_configmap_filter: find one needle in a ~56k-token ConfigMap ([default] vs [codemode]). Targets the intermediate-result bloat sink; classic path mirrors the passing eval 247. Both are correctness-gated on hallucination-proof exact values (parity: code mode must not regress the answer); per-variant token/LLM-call counts recorded by the harness quantify the win. Runnable in eval-regression.yaml on demand. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
|
@naomi-robusta Your eval run has finished. ✅ Completed successfully 🧪 Manual Eval Results
Results of HolmesGPT evals
Benchmark Comparison DetailsMaster baseline: latest master-* experiment (post-merge regression eval)
Benchmark baseline: latest ci-benchmark experiment on master
No baseline data available for comparison. Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
Measures the exact context-token cost of a tool's output — classic (result returned directly) vs code mode (filtered in a script, only the summary returned) — using the SAME formatter (format_tool_result_data) and the SAME tokenizer (litellm.token_counter, gpt-4o) the product uses to size/spill results. No LLM, no cluster, no RNG. Measured on this data: - result filtering: 250,029 -> 136 tokens (99.95% reduction), answer correct - call consolidation (8 sub-calls -> 1 result): 30,952 -> 275 (99.11%) Unlike the live A/B evals (which only assert correctness parity, since Holmes's server-side filters already keep k8s data out of context and the model may not choose code mode), this deterministically proves the token-saving mechanism. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
Make the code-mode trust boundary explicit and tested: - Prove the boundary is the parent-side allow-list, not the generated client: a script is given HOLMES_CODE_SOCKET, so tests forge raw socket requests (bypassing holmes.* stubs) to excluded tools (bash, is_core, approval-gated, unknown) and assert each is denied without the tool running; positive control confirms an eligible tool is reachable. - Document the accepted v1 residual risk (no fs sandbox) with a test that a local file is still readable, to be flipped if isolation is added. - Add a Security model section to the design doc: enforced boundary (allow-list, approval-not-bypassable, env isolation, per-tool validation, ulimit/timeout) and residual risk (no language/OS sandbox -> network + on-disk secret exfil under prompt injection; off by default; sandboxing a Future Goal). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
Rewrite the Open Questions section to capture the current state: mark the execution-bridge and tool-eligibility questions resolved (with how), keep the still-open design questions (routing, cost attribution), and add the implementation-stage questions surfaced while building — sandbox-before-default, the live-eval demonstration gap, default-on criteria, live sub-call streaming, serial sub-calls, prompt-overhead crossover, and the docs page. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
Rewrite to match the relay docs/design house template and read top-down: overview/background/goals first, code-level detail moved into Implementation & Verification and Detailed Implementation & Context. Present the current design directly (no 'option A -> option B' history; resolved bridge/eligibility decisions are stated as the design). Add a Code-mode-vs-bash-mode comparison across memory, CPU, security, approval, whitelisting, and injection patterns. Open Questions now lists only genuinely-open items. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
Evals 285/286 assert correctness parity only; this one verifies the model actually invokes code mode end-to-end. The prompt directs use of run_python_code, bash is disabled so the model cannot shell-script around it, and include_tool_calls lets the evaluator assert the run_python_code tool was called. A pass means: the model chose code mode, the script dispatched the real kubernetes tools through the bridge, and the aggregated count came back correct (hallucination-proof exact values). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMU8v729xhwopf4QcrJcPU Signed-off-by: Claude <noreply@anthropic.com>
This branch has not been deployed
2 participants
Summary
Adds an opt-in
code_executiontoolset implementing "code mode": instead of the LLM emitting one tool call per agentic step (re-sending all prior results each turn), it writes one Python script that composes many read-only Holmes tools and filters results in code, so only what itprint()s enters the model's context. This targets the biggest token sink in production — intermediate-result bloat on multi-step investigations (≥6-iteration requests are ~66% of spend).Design doc:
docs/design/2026-07-28_holmes-code-mode.md(included in this PR — design and implementation reviewed together). Ticket: ROB-723.How it works
run_python_code(code, timeout?)runs the script in a subprocess. A generatedholmesobject exposes one function per eligible tool; each call is relayed over a unix-domain socket back to the parent, which dispatches into the realToolExecutor. Credentials never leave the parent process — the subprocess can only ask the parent to run an allow-listed tool.is_coretoolsets,bash/kubectl_run, and anyapproval_required_toolsare excluded from the generated client. Because a synchronous script can't pause for interactive approval, anAPPROVAL_REQUIREDresult at dispatch is denied (the model is told to call that tool directly) — mirroring the trust model in the remote-tool-execution design.ulimitmemory cap; oversized stdout flows through the loop's existingspill_oversized_tool_resultguard. Off by default (feature-flagged viaenabled=False).ToolCallingLLMgains a small genericset_tool_executorwiring hook (no code-mode-specific coupling).Testing
Deterministic token-reduction proof (
test_code_mode_token_reduction.py) — measures the exact context-token cost of a tool's output, classic vs code mode, for identical data, using the same formatter (format_tool_result_data) and tokenizer (litellm.token_counter, gpt-4o) the product uses to size/spill results. No LLM, no cluster, no RNG:(Correctness asserted too — code mode computes the right answer from the same data.)
Unit / integration — 21 tests (real subprocess, no mocking of the runner): eligibility (
is_core/bash/approval-gated/self excluded), the token-saving property, and failure modes (syntax error, runtime exception, timeout, unavailable tool, erroring sub-tool, approval-gated sub-tool denied-not-paused, unwired executor, timeout clamping). No regressions intest_tool_calling_llm.py/test_tool_executor.py(47 passing).LLM evals — two ask_holmes A/B evals (
toolsets_matrix:[default]vs[codemode]), 4/4 passing in CI on opus-4.6 (run 30478931611). These assert correctness parity (code mode doesn't regress the answer) — they are deliberately not the token-win proof: Holmes's server-side filters (kubernetes_jq_query, logfilter) already keep k8s data out of context, and the live A/B run confirmed the model kept using those rather than code mode, so the small fixtures show no token delta. The deterministic test above is where the token win is proven. Run on demand ineval-regression.yaml.Toolset checklist
config_classesdefined (CodeExecutionConfig)python3availability)toolsets_matrix, 4/4 passing in CI (correctness parity)docs/data-sources/builtin-toolsets/code-execution.md)Notes / follow-ups
parent_tool_call_idfield (see the design doc; frontend PR to follow).ulimit, no language-level sandbox). Hardening the sandbox is a documented Future Goal.🤖 Generated with Claude Code
Generated by Claude Code