holmes/core/tools.py (3)

432-433: Avoid magic-number timeout; centralize it and align the timeout message

Nice addition. To prevent drift between the timeout argument and the error message (and to make it configurable), single-source the timeout via a module-level constant. While here, prefer showing the fully interpolated command in the timeout error for easier repro, and truncate very large partial outputs to keep status logs readable.

Apply this diff:

- timeout=60,
+ timeout=PREREQUISITE_TIMEOUT_SECONDS,
...
- except subprocess.TimeoutExpired as e:
+ except subprocess.TimeoutExpired as e:
 self.status = ToolsetStatusEnum.FAILED
- output_info = (
- f"\nPartial stdout: {e.stdout}\nPartial stderr: {e.stderr}"
- if e.stdout or e.stderr
- else ""
- )
- self.error = (
- f"`{prereq.command}` timed out after 60 seconds{output_info}"
- )
+ stdout = (e.stdout or "")
+ stderr = (e.stderr or "")
+ max_len = 10_000
+ if len(stdout) > max_len or len(stderr) > max_len:
+ stdout = stdout[:max_len] + "... [truncated]"
+ stderr = stderr[:max_len] + "... [truncated]"
+ output_info = f"\nPartial stdout: {stdout}\nPartial stderr: {stderr}"
+ self.error = (
+ f"`{command}` timed out after {PREREQUISITE_TIMEOUT_SECONDS} seconds{output_info}"
+ )

Add this near the imports to define a single source of truth (optionally configurable via env):

# at module top
PREREQUISITE_TIMEOUT_SECONDS = int(os.getenv("HOLMES_PREREQ_TIMEOUT_SECONDS", "60"))

Also applies to: 450-459

---

442-449: Great diagnostics; prefer interpolated command and truncate outputs to avoid log bloat

Including stdout/stderr is helpful. Two small improvements:

• Use the interpolated command (command) rather than the template (prereq.command) in the error for easier repro.
• Truncate very large outputs to avoid flooding the status/error UI.

- output_info = (
- f"\nStdout: {e.stdout}\nStderr: {e.stderr}"
- if e.stdout or e.stderr
- else ""
- )
- self.error = (
- f"`{prereq.command}` returned {e.returncode}{output_info}"
- )
+ stdout = (e.stdout or "")
+ stderr = (e.stderr or "")
+ max_len = 10_000
+ if len(stdout) > max_len or len(stderr) > max_len:
+ stdout = stdout[:max_len] + "... [truncated]"
+ stderr = stderr[:max_len] + "... [truncated]"
+ output_info = f"\nStdout: {stdout}\nStderr: {stderr}"
+ self.error = (
+ f"`{command}` returned {e.returncode}{output_info}"
+ )

---

474-476: Ensure config type aligns with the CallablePrerequisite contract

Defaulting None to {} is good. To avoid runtime surprises when self.config is set to a non-dict (its type is Optional[Any]), guard the type before passing it to the callable which expects dict[str, Any].

- config = self.config if self.config is not None else {}
- (enabled, error_message) = prereq.callable(config)
+ cfg = self.config
+ config: dict[str, Any] = cfg if isinstance(cfg, dict) else {}
+ if cfg is not None and not isinstance(cfg, dict):
+ logging.debug(
+ "CallablePrerequisite expected dict config; got %s",
+ type(cfg).__name__,
+ )
+ (enabled, error_message) = prereq.callable(config)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

holmes/core/tools.py (5)

442-449: Include the expanded command in error messages for better diagnosability.

When a check fails, showing the post-interpolation command (command) is more actionable than the original template (prereq.command).

- self.error = (
- f"`{prereq.command}` returned {e.returncode}{output_info}"
- )
+ self.error = (
+ f"`{command}` returned {e.returncode}{output_info}"
+ )

---

450-459: Reuse the computed timeout and show the expanded command in timeout errors.

This keeps messages consistent and accurate if the timeout is made configurable.

- self.error = (
- f"`{prereq.command}` timed out after 60 seconds{output_info}"
- )
+ self.error = (
+ f"`{command}` timed out after {prereq_timeout} seconds{output_info}"
+ )

---

425-433: Optional: ensure child processes are terminated on timeout.

shell=True may spawn a shell that then spawns children; the default subprocess.run kill on timeout targets only the shell process. If a prerequisite command starts grandchildren, consider starting a new session and killing the process group on timeout. This requires switching from subprocess.run to Popen + communicate and calling os.killpg on TimeoutExpired.

Happy to provide a concrete refactor if needed.

---

425-433: Make subprocess timeout configurable and disable stdin prompts

Nice work adding a timeout to the subprocess invocation. To avoid a magic constant and prevent commands from blocking on stdin prompts, consider these refinements:

• In holmes/core/tools.py (around the subprocess.run call):

- result = subprocess.run(
+ # Determine timeout: use per-prerequisite override or fall back to module default
+ prereq_timeout = getattr(prereq, "timeout_seconds", None) or DEFAULT_PREREQ_TIMEOUT_S
+ result = subprocess.run(
 command,
 shell=True,
 check=True,
 text=True,
+ stdin=subprocess.DEVNULL,
 stdout=subprocess.PIPE,
 stderr=subprocess.PIPE,
- timeout=60,
+ timeout=prereq_timeout,
 )

• At the top of holmes/core/tools.py, add a module-level default:

DEFAULT_PREREQ_TIMEOUT_S: int = 60

• In the ToolsetCommandPrerequisite model (around line 333), add an optional override field:

class ToolsetCommandPrerequisite(BaseModel):
 command: str
 expected_output: Optional[str] = None
 timeout_seconds: Optional[int] = Field(
 default=None,
 ge=1,
 description="Override the default command pre-check timeout (seconds)."
 )

All existing instantiations of ToolsetCommandPrerequisite (in code and tests) use keyword args for command/expected_output and will continue to work without changes.

---

442-459: Cap and Sanitize Subprocess Error Output

It’s easy for e.stdout/e.stderr to become very large or leak sensitive data; I recommend truncating and sanitizing before assigning to self.error:

• Add a module-level constant and helper in holmes/core/tools.py:

MAX_ERROR_OUTPUT_CHARS: int = 4096
def _truncate_text(s: Optional[str], max_chars: int = MAX_ERROR_OUTPUT_CHARS) -> str:
 if not s:
 return ""
 return s if len(s) <= max_chars else s[:max_chars] + "... [truncated]"

• In both exception handlers (CalledProcessError and TimeoutExpired), build output_info with truncated stdout/stderr:

- output_info = (
- f"\nStdout: {e.stdout}\nStderr: {e.stderr}"
- if e.stdout or e.stderr
- else ""
- )
+ out = _truncate_text(e.stdout)
+ err = _truncate_text(e.stderr)
+ output_info = ""
+ if out:
+ output_info += f"\nStdout: {out}"
+ if err:
+ output_info += f"\nStderr: {err}"

• Sanitize the final error string via your toolset’s sanitizer (e.g. the existing _sanitize_error in the Git toolset) before storing:

- self.error = (
- f"`{prereq.command}` returned {e.returncode}{output_info}"
- )
+ raw = f"`{prereq.command}` returned {e.returncode}{output_info}"
+ self.error = self.toolset._sanitize_error(raw)

• Repeat the same pattern in the TimeoutExpired block.

This will (1) limit log/UI spam, (2) reduce risk of leaking tokens or PII, and (3) ensure you don’t accidentally expose sensitive data in error messages.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025年07月08日T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.

holmes/core/tools.py (3)

441-449: Make timeout configurable and prevent stdin hangs

Good addition. Recommend making the timeout configurable per prerequisite and closing stdin to avoid interactive blocks.

Apply within this hunk:

 result = subprocess.run(
 command,
 shell=True,
 check=True,
 text=True,
+ stdin=subprocess.DEVNULL,
 stdout=subprocess.PIPE,
 stderr=subprocess.PIPE,
- timeout=60,
+ timeout=getattr(prereq, "timeout_seconds", 60),
 )

Add a field to ToolsetCommandPrerequisite (outside this hunk) to keep strong typing and mypy happy:

class ToolsetCommandPrerequisite(BaseModel):
 command: str # must complete successfully (error code 0) for prereq to be satisfied
 expected_output: Optional[str] = None # optional
 timeout_seconds: int = 60 # seconds

---

458-465: Harden error message formatting and avoid "None" leakage; prefer interpolated command

Avoid printing "None" and cap output size to keep errors readable. Also show the interpolated command.

- except subprocess.CalledProcessError as e:
- self.status = ToolsetStatusEnum.FAILED
- output_info = (
- f"\nStdout: {e.stdout}\nStderr: {e.stderr}"
- if e.stdout or e.stderr
- else ""
- )
- self.error = (
- f"`{prereq.command}` returned {e.returncode}{output_info}"
- )
+ except subprocess.CalledProcessError as e:
+ self.status = ToolsetStatusEnum.FAILED
+ def _trim(s: str) -> str:
+ return s if len(s) <= 10000 else s[:10000] + "... [truncated]"
+ output_info = ""
+ if e.stdout:
+ output_info += f"\nStdout:\n{_trim(e.stdout)}"
+ if e.stderr:
+ output_info += f"\nStderr:\n{_trim(e.stderr)}"
+ self.error = (
+ f"`{command}` returned {e.returncode}{output_info}"
+ )

---

466-475: Timeout path: use dynamic timeout value, avoid "None", cap output, and show interpolated command

Make the message reflect the actual timeout and keep partial outputs tidy.

- except subprocess.TimeoutExpired as e:
- self.status = ToolsetStatusEnum.FAILED
- output_info = (
- f"\nPartial stdout: {e.stdout}\nPartial stderr: {e.stderr}"
- if e.stdout or e.stderr
- else ""
- )
- self.error = (
- f"`{prereq.command}` timed out after 60 seconds{output_info}"
- )
+ except subprocess.TimeoutExpired as e:
+ self.status = ToolsetStatusEnum.FAILED
+ def _trim(s: str) -> str:
+ return s if len(s) <= 10000 else s[:10000] + "... [truncated]"
+ output_info = ""
+ if e.stdout:
+ output_info += f"\nPartial stdout:\n{_trim(e.stdout)}"
+ if e.stderr:
+ output_info += f"\nPartial stderr:\n{_trim(e.stderr)}"
+ timeout_s = getattr(prereq, "timeout_seconds", 60)
+ self.error = (
+ f"`{command}` timed out after {timeout_s} seconds{output_info}"
+ )

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025年07月08日T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.

holmes/core/tools.py (2)

671-679: Make timeout configurable and prevent interactive hangs.

Use a per-prereq timeout and close stdin so checks can’t block on prompts.

- result = subprocess.run(
+ timeout_secs = getattr(prereq, "timeout_seconds", 60)
+ result = subprocess.run(
 command,
 shell=True,
 check=True,
 text=True,
+ stdin=subprocess.DEVNULL,
 stdout=subprocess.PIPE,
 stderr=subprocess.PIPE,
- timeout=60,
+ timeout=timeout_secs,
 )

Add the field to the model (outside this hunk):

 class ToolsetCommandPrerequisite(BaseModel):
- command: str # must complete successfully (error code 0) for prereq to be satisfied
- expected_output: Optional[str] = None # optional
+ command: str # must complete successfully (error code 0) for prereq to be satisfied
+ expected_output: Optional[str] = None # optional
+ timeout_seconds: Optional[int] = Field(
+ default=60, description="Timeout for the prerequisite command"
+ )

---

688-695: Use the interpolated command in error messages.

This makes diagnostics clearer than showing the unexpanded template.

- self.error = (
- f"`{prereq.command}` returned {e.returncode}{output_info}"
- )
+ self.error = (
+ f"`{command}` returned {e.returncode}{output_info}"
+ )

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025年07月08日T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.

holmes/core/tools.py (1)

701-710: TimeoutExpired bug: use e.output (not e.stdout) and print the actual timeout; include e.cmd.

Using e.stdout here will raise AttributeError and mask the real timeout failure. Also avoid hard-coding "60 seconds".

- except subprocess.TimeoutExpired as e:
- self.status = ToolsetStatusEnum.FAILED
- output_info = (
- f"\nPartial stdout: {e.stdout}\nPartial stderr: {e.stderr}"
- if e.stdout or e.stderr
- else ""
- )
- self.error = (
- f"`{prereq.command}` timed out after 60 seconds{output_info}"
- )
+ except subprocess.TimeoutExpired as e:
+ self.status = ToolsetStatusEnum.FAILED
+ # TimeoutExpired exposes .output for captured stdout
+ partial_stdout = getattr(e, "output", None)
+ partial_stderr = e.stderr
+ output_info = (
+ f"\nPartial stdout: {partial_stdout}\nPartial stderr: {partial_stderr}"
+ if partial_stdout or partial_stderr
+ else ""
+ )
+ cmd = getattr(e, "cmd", command)
+ self.error = f"`{cmd}` timed out after {secs} seconds{output_info}"

#!/bin/bash
# Verify there are no other TimeoutExpired handlers using e.stdout
rg -nP 'except\s+subprocess\.TimeoutExpired.*\n(?s).*e\.stdout' -C2

holmes/core/tools.py (2)

676-684: Avoid hard-coded 60s; make timeout configurable and reuse the value in errors.

Expose a per-prereq timeout and pass it to subprocess.run. This also lets you print the actual value in the TimeoutExpired message.

Apply within this block:

- result = subprocess.run(
+ secs = getattr(prereq, "timeout_seconds", DEFAULT_PREREQ_TIMEOUT)
+ result = subprocess.run(
 command,
 shell=True,
 check=True,
 text=True,
 stdout=subprocess.PIPE,
 stderr=subprocess.PIPE,
- timeout=60,
+ timeout=secs,
 )

Additions outside this block:

+DEFAULT_PREREQ_TIMEOUT: int = 60 # default prereq timeout (seconds)

And extend the model:

 class ToolsetCommandPrerequisite(BaseModel):
 command: str # must complete successfully (error code 0) for prereq to be satisfied
 expected_output: Optional[str] = None # optional
+ timeout_seconds: Optional[int] = Field(
+ default=None, gt=0, description="Max seconds to wait before failing the prereq"
+ )

---

691-700: Use the executed command from the exception (e.cmd) instead of prereq.command.

Improves accuracy when env vars expand or templates modify the command. Keep current stdout/stderr inclusion.

 except subprocess.CalledProcessError as e:
 self.status = ToolsetStatusEnum.FAILED
- output_info = (
+ output_info = (
 f"\nStdout: {e.stdout}\nStderr: {e.stderr}"
 if e.stdout or e.stderr
 else ""
 )
- self.error = (
- f"`{prereq.command}` returned {e.returncode}{output_info}"
- )
+ cmd = getattr(e, "cmd", command)
+ self.error = f"`{cmd}` returned {e.returncode}{output_info}"

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025年07月08日T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.