Redesigned bash toolset that have a prefix based approvals

[Sheeproid]

• Allows prefixes to be approved
• Comes with a built-in list of prefixes (for server mode)
• Upgraded CLI approval experience

Summary by CodeRabbit

Release Notes

• New Features

  • Bash tool now enabled by default with simplified prefix-based command validation
  • Interactive approval flow for non-approved commands with option to save prefixes for future sessions
  • Session prefix memory persists approved command prefixes across conversations
  • Added --bash-always-deny and --bash-always-allow CLI flags for controlling bash command approval behavior
  • Configurable allow/deny lists for bash commands via toolset configuration

• Improvements

  • Enhanced command validation clarity with better error messaging for blocked operations
  • Reduced approval friction through prefix persistence

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[Avi-Robusta]

some renaming and refactoring comments
there is a comment about using an existing file for the commands instead of creating a new one, see if there is a quick workaround, if not than ignore it

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@holmes/core/tool_calling_llm.py`:
- Around line 801-814: The _is_tool_call_already_approved function builds a
ToolInvokeContext but omits session_approved_prefixes, so approval re-checks
ignore previously approved prefixes; update _is_tool_call_already_approved to
accept a session_approved_prefixes argument and pass it into the
ToolInvokeContext constructor (ToolInvokeContext(...,
session_approved_prefixes=session_approved_prefixes)), then update every call
site that invokes _is_tool_call_already_approved (the place that re-checks
approvals after tool execution) to forward the current session_approved_prefixes
so tool.requires_approval sees session-level approvals.

♻️ Duplicate comments (5)

holmes/plugins/toolsets/bash/bash_toolset.py (4)

133-150: Add return type annotation for _validate_command.

The method is missing a return type annotation. Based on the implementation, it returns ValidationResult from validate_command().

♻️ Proposed fix

First, update the import:

 from holmes.plugins.toolsets.bash.validation import (
     DenyReason,
+    ValidationResult,
     ValidationStatus,
     get_effective_lists,
     validate_command,
 )

Then update the signature:

     def _validate_command(
-        self, command_str: str, suggested_prefixes: list, context: ToolInvokeContext
-    ):
+        self,
+        command_str: str,
+        suggested_prefixes: list[str],
+        context: ToolInvokeContext,
+    ) -> ValidationResult:

---

265-283: Add type annotation for validation_result parameter.

The validation_result parameter lacks a type annotation. Add ValidationResult type (requires importing it from the validation module).

♻️ Proposed fix

-    def _build_deny_error_message(self, validation_result) -> str:
+    def _build_deny_error_message(self, validation_result: ValidationResult) -> str:

---

35-81: Include stderr in error results for LLM self-correction.

The bash_result_to_structured function omits stderr from error responses. Per coding guidelines, toolsets must return detailed error messages including full API/command error output. Including stderr helps the LLM understand what went wrong and self-correct.

🔧 Proposed fix

 def bash_result_to_structured(
     result: BashResult, cmd: str, timeout: int, params: dict
 ) -> StructuredToolResult:
+    stderr = result.stderr if hasattr(result, 'stderr') else None
+    
     if result.timed_out:
+        data_parts = [cmd, result.stdout, stderr]
+        data = "\n".join(p for p in data_parts if p)
         return StructuredToolResult(
             status=StructuredToolResultStatus.ERROR,
-            error=f"Error: Command '{cmd}' timed out after {timeout} seconds.",
-            data=f"{cmd}\n{result.stdout}" if result.stdout else None,
+            error=f"Error: Command '{cmd}' timed out after {timeout} seconds."
+                  + (f" stderr: {stderr}" if stderr else ""),
+            data=data or None,
             params=params,
             invocation=cmd,
         )
 
-    result_data = f"{cmd}\n{result.stdout}"
+    data_parts = [cmd, result.stdout, stderr]
+    result_data = "\n".join(p for p in data_parts if p)
     
     # ... in the else branch for non-zero return code:
-        error = (
-            f'Error: Command "{cmd}" returned non-zero exit status {result.return_code}'
-        )
+        error = (
+            f'Error: Command "{cmd}" returned non-zero exit status {result.return_code}'
+            + (f". stderr: {stderr}" if stderr else "")
+        )

As per coding guidelines, toolsets must return detailed error messages from underlying APIs.

---

162-170: Guard against invalid suggested_prefixes type in requires_approval.

requires_approval() calls _validate_command() before _invoke() performs type validation. If suggested_prefixes is not a list or contains non-strings, validation may throw or mis-evaluate. Add a defensive check to let _invoke() handle type errors.

✅ Proposed defensive check

         command_str = params.get("command", "")
         suggested_prefixes = params.get("suggested_prefixes", [])
 
         if not command_str or not suggested_prefixes:
             return None  # Let _invoke() handle validation errors
+        
+        # Type check - let _invoke() handle type errors with proper error messages
+        if not isinstance(suggested_prefixes, list) or not all(
+            isinstance(p, str) for p in suggested_prefixes
+        ):
+            return None

holmes/core/tool_calling_llm.py (1)

124-135: Regex pattern fails to capture JSON with nested structures.

The pattern [^}]+ stops at the first }, which breaks when the JSON contains arrays or nested objects. For example, {"bash_session_approved_prefixes": ["kubectl", "helm"]} would be truncated at the first ], causing JSON parse failure.

🐛 Proposed fix using balanced brace matching

-        # Extract tool_call_metadata from the content string
-        # Format: tool_call_metadata={"tool_name": "...", ...}
-        match = re.search(r"tool_call_metadata=(\{[^}]+\})", content)
-        if not match:
-            continue
-
-        try:
-            metadata = json.loads(match.group(1))
+        # Extract tool_call_metadata from the content string
+        # Format: tool_call_metadata={"tool_name": "...", ...}
+        marker = "tool_call_metadata="
+        start_idx = content.find(marker)
+        if start_idx == -1:
+            continue
+
+        json_start = start_idx + len(marker)
+        if json_start >= len(content) or content[json_start] != '{':
+            continue
+
+        # Find matching closing brace using balanced counting
+        brace_count = 0
+        json_end = json_start
+        for i, char in enumerate(content[json_start:], json_start):
+            if char == '{':
+                brace_count += 1
+            elif char == '}':
+                brace_count -= 1
+                if brace_count == 0:
+                    json_end = i + 1
+                    break
+        else:
+            continue  # No matching brace found
+
+        try:
+            metadata = json.loads(content[json_start:json_end])

🧹 Nitpick comments (5)

holmes/plugins/toolsets/bash/common/cli_prefixes.py (3)

52-59: Narrow exception handling for YAML loading.

The except Exception catches all exceptions including unexpected ones like KeyboardInterrupt. Consider narrowing to the specific exceptions that can occur here: OSError for file operations and yaml.YAMLError for parsing errors.

♻️ Proposed fix

-        except Exception as e:
+        except (OSError, yaml.YAMLError) as e:
             logging.warning(f"Failed to load approved prefixes: {e}")

---

74-81: Silent exception swallowing hides failures.

The try-except-pass block silently ignores read errors when loading existing prefixes during save. This could mask issues like permission problems or corrupted YAML. Consider logging a warning so operators can diagnose persistence issues.

♻️ Proposed fix

-        except Exception:
-            pass
+        except (OSError, yaml.YAMLError) as e:
+            logging.debug(f"Could not load existing prefixes for merge: {e}")

---

85-89: Use logging.exception to preserve traceback.

When logging errors, logging.exception automatically includes the traceback, which is helpful for debugging write failures. Also consider narrowing the exception type.

♻️ Proposed fix

-    except Exception as e:
-        logging.error(f"Failed to save approved prefixes: {e}")
+    except (OSError, yaml.YAMLError) as e:
+        logging.exception(f"Failed to save approved prefixes: {e}")

holmes/interactive.py (2)

626-636: Remove unused console parameter.

The console parameter is declared but never used in _run_inline_menu. Either remove it or use it (e.g., for fallback output if the Application fails).

♻️ Proposed fix

-def _run_inline_menu(options: list[str], console: Console) -> Optional[int]:
+def _run_inline_menu(options: list[str]) -> Optional[int]:

And update the call site at line 746:

-    result = _run_inline_menu(options, console)
+    result = _run_inline_menu(options)

---

43-48: Consider consolidating imports from the same module.

The two separate import statements from cli_prefixes can be combined for clarity.

♻️ Proposed fix

-from holmes.plugins.toolsets.bash.common.cli_prefixes import (
-    enable_cli_mode,
-)
-from holmes.plugins.toolsets.bash.common.cli_prefixes import (
-    save_cli_bash_tools_approved_prefixes as _save_approved_prefixes,
-)
+from holmes.plugins.toolsets.bash.common.cli_prefixes import (
+    enable_cli_mode,
+    save_cli_bash_tools_approved_prefixes as _save_approved_prefixes,
+)