fix(bash,prometheus): handle string timeout and improve unparseable command validation

[Aladex]

Related Issues

• Fixes Bash toolset: TypeError when LLM passes timeout as string #1727 — Bash toolset: TypeError when LLM passes timeout as string
• Fixes Bash toolset: commands with unquoted parentheses always require approval even when all segments are allowed #1728 — Bash toolset: commands with unquoted parentheses always require approval even when all segments are allowed
• Related to An error occurred when configuring Prometheus：TypeError: '>' not supported between instances of 'str' and 'int' #1677 — Prometheus toolset: same timeout TypeError

Changes

1. Timeout type coercion (bash + prometheus toolsets)

LLMs sometimes pass the timeout tool parameter as a string ("30") instead of an integer. This causes:

• Bash toolset: TypeError: unsupported operand type(s) for +: 'float' and 'str' in subprocess.communicate(timeout=timeout)
• Prometheus toolset: TypeError: '>' not supported between instances of 'str' and 'int' in timeout comparison

Fix: cast timeout to int with a try/except fallback to the default value.

2. shlex-based fallback for unparseable commands (bash toolset)

When bashlex.parse() fails (e.g. unquoted parentheses in PromQL queries like query=topk(10, metric)), the current code always returns APPROVAL_REQUIRED. In server mode (e.g. Keep/Robusta integrations), there is no user to approve, so these commands silently fail.

Added a fallback that uses shlex.split() (stdlib, respects shell quoting) to tokenize the command, then groups tokens into segments by shell operators (|, &&, ;, &) and validates each segment against the allow/deny lists. If all segments are allowed, the command is permitted.

Security is preserved:

• Hardcoded blocks and deny list are checked on the raw string before the fallback
• Each segment in the fallback goes through the full validate_segment() deny → allow → approval pipeline
• If any segment is not in the allow list, the behavior is unchanged (APPROVAL_REQUIRED)
• If shlex.split() also fails, falls through to APPROVAL_REQUIRED

Tests

Added 3 test cases to test_bash_toolset_validation.py:

• curl with parentheses in args piped to jq (both allowed) → ALLOWED
• curl with parentheses piped to non-allowed command → APPROVAL_REQUIRED
• curl with parentheses piped to denied command → DENIED

Summary by CodeRabbit

• Bug Fixes

  • Improved timeout parameter handling in Bash and Prometheus tools to gracefully convert string values to integers, preventing type errors.

• New Features

  • Added fallback command validation for complex Bash commands that may not parse with the primary parser.

• Tests

  • Expanded test coverage for edge cases in command validation with special characters and piped operations.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Aladex / name: Andrey (9ecb9f1)

[coderabbitai]

Walkthrough

Adds defensive timeout parameter handling in bash and Prometheus toolsets to gracefully convert string timeouts from LLMs to integers, and introduces a fallback command validation path using shlex parsing for cases where bashlex fails on unquoted parentheses in command arguments.

Changes

Cohort / File(s)	Summary
Timeout Parameter Normalization holmes/plugins/toolsets/bash/bash_toolset.py, holmes/plugins/toolsets/prometheus/prometheus.py	Added try-except logic to cast timeout parameters to integers with fallback to default values (30 for bash, default_timeout for Prometheus) when conversion fails, preventing TypeErrors from string-type timeouts.
Fallback Command Validation holmes/plugins/toolsets/bash/validation.py	Introduced fallback validation path using shlex to tokenize and split commands by shell operators (pipe, logical operators, semicolon) when bashlex parsing fails; validates each segment against allow/deny lists and returns ALLOWED if all segments pass, otherwise APPROVAL_REQUIRED.
Validation Test Coverage tests/test_bash_toolset_validation.py	Added three test cases covering curl commands with parentheses in quoted arguments, piping to allowed/denied commands, exercising the new fallback validation path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Allow loops, conditionals, and subshells with user approval #1511 — Modifies command validation and segment parsing in holmes/plugins/toolsets/bash/validation.py with related fallback handling logic.
• Claude/tool approval all tools l mw xe #1355 — Modifies bash toolset implementation including RunBashCommand/_invoke path and approval behavior.
• Add bash whitelist to cli #1522 — Modifies validation.py command validation behavior and fallback parsing alongside allowlist configuration.

Suggested reviewers

• aantn
• arikalon1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes both main changes: timeout string handling in bash/prometheus toolsets and improved fallback validation for unparseable bash commands.
Linked Issues check	✅ Passed	Code changes fully address objectives from #1727 (timeout coercion with fallback) and #1728 (shlex-based fallback for unparseable commands with per-segment validation).
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing timeout type handling and improving command validation; no unrelated refactoring or unintended modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[netlify]

✅ Deploy Preview for holmes-docs ready!

Name	Link
🔨 Latest commit	9ecb9f1
🔍 Latest deploy log	https://app.netlify.com/projects/holmes-docs/deploys/69affe8c2358b90008631bee
😎 Deploy Preview	https://deploy-preview-1729--holmes-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_bash_toolset_validation.py (1)

1070-1080: Consider adding deny_reason assertion for consistency.

Other similar denial tests in this file (e.g., test_subshell_with_deny_listed_command_still_denied at lines 1001-1002) assert both result.status and result.deny_reason. Adding the assertion here would improve consistency and provide better failure diagnostics.

🔧 Proposed addition

         assert result.status == ValidationStatus.DENIED
+        assert result.deny_reason == DenyReason.DENY_LIST
 

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_bash_toolset_validation.py` around lines 1070 - 1080, The test
test_curl_with_parentheses_denied_command only asserts result.status; update it
to also assert the denial reason for consistency by adding an assertion on
result.deny_reason (e.g., assert result.deny_reason and "rm" in
result.deny_reason) so the test verifies a non-empty deny_reason and that it
references the denied command; reference symbols:
test_curl_with_parentheses_denied_command, result, result.deny_reason, and
ValidationStatus.DENIED.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/test_bash_toolset_validation.py`:
- Around line 1070-1080: The test test_curl_with_parentheses_denied_command only
asserts result.status; update it to also assert the denial reason for
consistency by adding an assertion on result.deny_reason (e.g., assert
result.deny_reason and "rm" in result.deny_reason) so the test verifies a
non-empty deny_reason and that it references the denied command; reference
symbols: test_curl_with_parentheses_denied_command, result, result.deny_reason,
and ValidationStatus.DENIED.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8bfdb79e-52a7-4231-8c95-e5b1a5bc667a

📥 Commits

Reviewing files that changed from the base of the PR and between 6e4b84b and 9ecb9f1.

📒 Files selected for processing (4)

• holmes/plugins/toolsets/bash/bash_toolset.py
• holmes/plugins/toolsets/bash/validation.py
• holmes/plugins/toolsets/prometheus/prometheus.py
• tests/test_bash_toolset_validation.py

[Aladex]

@aantn could you take a look when you get a chance? This fixes the timeout TypeError you asked about in #1727 (we're using gpt-5-mini) and also the bashlex parse failure for PromQL queries (#1728) which silently breaks commands in server mode.

[aantn]

@Aladex can you try this with #1790 and see if it fixes it?
We're trying to address with broader coercion mechanism that isn't tool specific.

Re the bashlex.parse() failure, isn't the generated bash command invalid?

[Aladex]

@aantn thanks for the pointer!

Timeout coercion — I'll rebase on top of #1790 and drop the per-tool timeout casts from this PR. The universal coercion approach is cleaner.

bashlex fallback — the bash command itself is valid. The typical case is something like:

curl 'http://prometheus:9090/api/v1/query?query=topk(10, node_cpu_seconds_total)' | jq .status

The parentheses are inside single quotes, so bash handles them fine — but bashlex.parse() chokes on them. In server mode (Robusta/Keep integrations) there's no human to click "approve", so these commands silently fail even though both curl and jq are in the allow list.

The shlex fallback only kicks in when bashlex fails, tokenizes by shell operators, and validates each segment against the same allow/deny lists. If any segment isn't explicitly allowed, it still returns APPROVAL_REQUIRED.

[aantn]

@Aladex are you sure about that?

cat > /tmp/test.py << 'EOF'
import bashlex
try:
  bashlex.parse("curl 'query=topk(10,x)' | jq ."); print("quoted: OK")
except:
  print("quoted: FAIL")
try:
  bashlex.parse("curl query=topk(10,x) | jq ."); print("unquoted: OK")
except:
  print("unquoted: FAIL")
EOF
python3 /tmp/test.py

By me bashlex handles it just fine:

quoted: OK
unquoted: FAIL

In the original issue you opened, the example command was unquoted, so I think the LLM was just generating an invalid command.

In any event, can we fix by giving you the option to auto-disapprove all commands on the server flow and continue the investigation?

[Aladex]

@aantn you're right — I double-checked and bashlex does handle the quoted version fine, so the fallback wouldn't actually trigger in that case. And the unquoted version is indeed invalid bash.

I need to dig into our production logs a bit more to catch the exact commands the LLM is generating and understand where it's actually breaking. I'll follow up once I have concrete examples.

The auto-disapprove + continue idea for server mode sounds good regardless — happy to help with that if needed.

[Aladex]

@aantn you're right on both counts. Confirmed on production — the LLM does quote the arguments properly, so bashlex handles it fine. The shlex fallback isn't needed.

The timeout issue is also confirmed to be a real problem on 0.20.0, but #1790 addresses it with the universal coercion approach, which is the right fix.

Closing this PR in favor of #1790.

[aantn]

Awesome, thank you. Adding docs on how to disable approval in server mode - turns out we support it but its not documented!

#1800

Let me know if that helps.