Repository navigation
ROB-320 Claude/remove edit command step2 - #2202
Merged
Merged
Conversation
Three-part plan for hardening tool approval against the forged-approval primitive reported in GHSA-6m4w-cmhp-f95f: - tool-approval-tickets.md: original draft (raw HMAC), kept for context - step-2-remove-edit-command.md: remove the edit-command flow so an approved tool call cannot be mutated between approval and execution - step-3-tool-approval-tickets-jwt.md: signed approval tickets via JWT (HS256/PyJWT, 7-day TTL, collapsed reason codes) Step 1 (deny_list-always-runs in _invoke) was considered and dropped: requires_approval already filters DENIED commands before they reach the user, so after Steps 2+3 close the mutation+forgery paths, the deny check at _invoke with user_approved=True is unreachable in the legitimate flow. Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Closes the post-approval mutation primitive: today the approval UI can substitute any value for a bash tool_call's command argument after the user approves the original, and the executor runs the substituted command with user_approved=True (so bash_toolset's deny list is skipped). An attacker who can submit a tool_decisions blob (forged pending_approval, or a legitimate but malicious user) can take any benign approved bash command and swap in rm -rf, kubectl delete namespace, token exfil, etc. Pure removal: - holmes/core/models.py: drop ToolApprovalDecision.edit_command - holmes/core/tool_calling_llm.py: drop the splice block in _execute_tool_decisions - tests/test_tool_decision_edit_command.py: deleted - tests/core/conversations_worker/integration/test_conversation_integration.py: drop test_approval_with_edit_command - tests/test_edit_command_removed.py: regression test that an old client still POSTing edit_command (a) is silently accepted (Pydantic v2 default extra="ignore" drops the field) and (b) the original command runs, not the substituted one FE coordination: the Robusta frontend's matching PR removes the inline edit affordance and stops sending edit_command. Rollout order is FE-first; this Holmes change is safe to deploy after the FE rolls. Per specs/step-2-remove-edit-command.md. Signed-off-by: Roi Glinik <groi.tech@gmail.com>
The three step-2/step-3 design docs were carried in the first commit on this branch for review context. Removing them now — they live locally outside the repo and the merged PR should reflect only the code change. No content lost. Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Contributor
✅ Results of HolmesGPT evalsAutomatically triggered by commit 551586c on branch Results of HolmesGPT evals
Benchmark Comparison DetailsMaster baseline: latest master-* experiment (post-merge regression eval)
Benchmark baseline: latest ci-benchmark experiment on master
Time comparison (seconds):
Cost comparison:
Total tokens comparison:
Cached tokens comparison:
Turns comparison:
Tool calls 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: |
Contributor
WalkthroughThe PR removes the ChangesRemove edit_command from tool approval flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Contributor
|
✅ 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:9698e36f5
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:9698e36f5 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9698e36f5
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9698e36f5
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:9698e36f5
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:9698e36f5 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:9698e36f5
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:9698e36f5Patch 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:9698e36f5 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:9698e36f5Robusta 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:9698e36f5 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:9698e36f5 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_edit_command_removed.py (1)
1-103: ⚡ Quick winMove this regression test under the
tests/core/...hierarchy.This module validates behavior in
holmes/core/models.pyandholmes/core/tool_calling_llm.py, but it currently sits attests/test_edit_command_removed.py. Please place it under the matchingtests/core/...tree to keep test/source structure aligned.As per coding guidelines: “Tests must match source structure under
tests/.”🤖 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 `@tests/test_edit_command_removed.py` around lines 1 - 103, The test module containing the test_edit_command_in_payload_is_silently_dropped_by_pydantic and test_original_command_runs_even_when_edit_command_was_sent tests is currently located at the root level of the tests directory. Since these tests validate behavior in holmes.core.models and holmes.core.tool_calling_llm, move this entire test file to align with the source code structure by placing it in the core subdirectory of tests, matching the hierarchy of the source modules being tested.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/test_edit_command_removed.py`:
- Around line 1-103: The test module containing the
test_edit_command_in_payload_is_silently_dropped_by_pydantic and
test_original_command_runs_even_when_edit_command_was_sent tests is currently
located at the root level of the tests directory. Since these tests validate
behavior in holmes.core.models and holmes.core.tool_calling_llm, move this
entire test file to align with the source code structure by placing it in the
core subdirectory of tests, matching the hierarchy of the source modules being
tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8fda671-d5eb-43c1-9dad-024eaed16bf2
📒 Files selected for processing (5)
holmes/core/models.pyholmes/core/tool_calling_llm.pytests/core/conversations_worker/integration/test_conversation_integration.pytests/test_edit_command_removed.pytests/test_tool_decision_edit_command.py
💤 Files with no reviewable changes (4)
- holmes/core/models.py
- holmes/core/tool_calling_llm.py
- tests/test_tool_decision_edit_command.py
- tests/core/conversations_worker/integration/test_conversation_integration.py
Avi-Robusta
approved these changes
Jun 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Removed Features
Bug Fixes