feat: add Telegram and data platform integrations - #2319
Conversation
Signed-off-by: Vladimir.Yakovlev <striker_vlad@mail.ru>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThis change adds Airflow, Flink, and Trino built-in HTTP toolsets, a shared HTTP API foundation, and a Telegram long-polling adapter. It registers the integrations, adds Helm deployment support and tests, updates documentation, and normalizes Docker checksum files before verification. ChangesData platform integrations
Telegram adapter
Docker build verification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TelegramUser
participant TelegramAPI
participant HolmesTelegramBot
participant HolmesAPI
TelegramUser->>TelegramAPI: Send question
HolmesTelegramBot->>TelegramAPI: Poll update
TelegramAPI-->>HolmesTelegramBot: Return update
HolmesTelegramBot->>HolmesAPI: Forward question and chat history
HolmesAPI-->>HolmesTelegramBot: Return analysis and updated history
HolmesTelegramBot->>TelegramAPI: Send analysis
TelegramAPI-->>TelegramUser: Display response
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
docs/data-sources/builtin-toolsets/airflow.md (2)
40-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommon Use Cases blocks don't follow the documented
holmes askhouse style.See consolidated comment covering this file and
flink.md.🤖 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 `@docs/data-sources/builtin-toolsets/airflow.md` around lines 40 - 48, Update the “Common Use Cases” examples in the Airflow documentation to follow the documented `holmes ask` house style, matching the format used by the consolidated guidance and the corresponding Flink documentation examples.Source: Learnings
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommon Use Cases sections don't follow the documented
holmes askhouse style.Both new toolset docs present raw prompt strings in
text fences rather than individualbash blocks with aholmes ask "..."command, which is the documented house style for this section.
docs/data-sources/builtin-toolsets/airflow.md#L40-48: wrap each example as its own ```bash block containingholmes ask "...".docs/data-sources/builtin-toolsets/flink.md#L32-40: same change.Based on learnings: "document each individual
holmes askcommand in its own separate fenced bash code block (bash ...)... follow the existing house style used across the other toolset docs."🤖 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 `@docs/data-sources/builtin-toolsets/airflow.md` at line 1, Update the Common Use Cases examples in the Airflow and Flink toolset documentation to use the established house style: place each prompt in its own separate ```bash block containing a holmes ask "..." command, replacing the existing raw prompt text fences.Source: Learnings
docs/data-sources/builtin-toolsets/flink.md (1)
32-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommon Use Cases blocks don't follow the documented
holmes askhouse style.See consolidated comment covering this file and
airflow.md.🤖 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 `@docs/data-sources/builtin-toolsets/flink.md` around lines 32 - 40, Update the Common Use Cases examples in the Flink documentation to follow the documented `holmes ask` house style, including the required command format for each example. Preserve the existing Flink questions and intent while aligning both code blocks with the conventions used in the consolidated guidance.Source: Learnings
holmes/plugins/toolsets/http_api_base.py (2)
91-102: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a shared
requests.Session()for connection reuse.Each call to
_requestissuesrequests.request(...), opening a new connection every time. Airflow/Flink/Trino tools will call this repeatedly during an investigation; a persistentSessionper toolset instance would reduce connection setup overhead.🤖 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/plugins/toolsets/http_api_base.py` around lines 91 - 102, Update the HTTP request flow in _request to reuse a persistent requests.Session owned by each toolset instance instead of calling requests.request directly. Initialize the session during toolset construction and preserve the existing method, URL, parameters, authentication, timeout, SSL verification, response validation, and return behavior.
23-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
SecretStrforbearer_token/password.These are stored as plain
str. If the config object or a validation error is ever logged/repr'd (e.g._health_check's error message wraps arbitrary exceptions), the secret could leak into logs/tracebacks. Pydantic'sSecretStrmasks the value inrepr()/str()while still allowing.get_secret_value()where needed.🔒 Proposed fix
+from pydantic import Field, SecretStr, model_validator - bearer_token: Optional[str] = Field( + bearer_token: Optional[SecretStr] = Field( default=None, description="Bearer token used for API authentication" ) username: Optional[str] = Field( default=None, description="Username used for HTTP basic authentication" ) - password: Optional[str] = Field( + password: Optional[SecretStr] = Field( default=None, description="Password used for HTTP basic authentication" )Update usages in
_requestaccordingly (self.http_config.bearer_token.get_secret_value(), and passself.http_config.password.get_secret_value()toHTTPBasicAuth).🤖 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/plugins/toolsets/http_api_base.py` around lines 23 - 31, Change the bearer_token and password fields in the HTTP configuration model to Optional[SecretStr], then update _request to unwrap them with get_secret_value() before constructing authorization headers or HTTPBasicAuth; preserve existing behavior when either value is None.holmes/plugins/toolsets/airflow/airflow.py (1)
74-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_boundedimplementation (shared with Flink).See consolidated comment (anchored in
holmes/plugins/toolsets/flink/flink.py) covering this duplication and the related unbounded-fetch guideline concern.🤖 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/plugins/toolsets/airflow/airflow.py` around lines 74 - 88, Remove the duplicate _bounded implementation from _AirflowTool and reuse the shared bounded-result helper used by the Flink toolset. Update the Airflow tool flow to call that common implementation while preserving collection-key truncation and holmes_truncated behavior.holmes/plugins/toolsets/trino/trino.py (1)
141-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated
cast(TrinoToolset, self._toolset)into a local variable.The same cast is repeated four times in this method; hoisting it once at the top improves readability without behavior change.
♻️ Proposed refactor
+ trino_toolset = cast(TrinoToolset, self._toolset) try: response = self._toolset._request( "POST", "/v1/statement", data=query, request_context=context.request_context, - headers=cast(TrinoToolset, self._toolset)._trino_headers(), + headers=trino_toolset._trino_headers(), ) while True: ... if ( not next_uri - or pages >= cast(TrinoToolset, self._toolset).trino_config.max_pages + or pages >= trino_toolset.trino_config.max_pages or len(rows) - >= cast(TrinoToolset, self._toolset).trino_config.max_rows + >= trino_toolset.trino_config.max_rows ): break ... - max_rows = cast(TrinoToolset, self._toolset).trino_config.max_rows + max_rows = trino_toolset.trino_config.max_rows🤖 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/plugins/toolsets/trino/trino.py` around lines 141 - 190, In the method containing the paginated Trino request loop, assign cast(TrinoToolset, self._toolset) to a local variable once near the start, then reuse that variable for _trino_headers() and both trino_config accesses. Preserve all existing request and pagination behavior.docs/data-sources/builtin-toolsets/trino.md (1)
38-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommon Use Cases examples don't follow the established
holmes askbash-block convention.Other toolset docs wrap each example as an individual
holmes ask "..."command in its own bash fence, not bare text blocks.📝 Proposed fix
-```text -Show failed Trino queries from the last hour and summarize their error types -``` - -```text -Explain the query plan for this SELECT without running it -``` +```bash +holmes ask "Show failed Trino queries from the last hour and summarize their error types" +``` + +```bash +holmes ask "Explain the query plan for this SELECT without running it" +```Based on learnings, "document each individual
holmes askcommand in its own separate fenced bash code block... follow the existing house style used across the other toolset docs."🤖 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 `@docs/data-sources/builtin-toolsets/trino.md` around lines 38 - 46, Update the Common Use Cases examples in the Trino toolset documentation so each example is a complete holmes ask command, with its prompt quoted, inside its own separate bash-fenced code block. Preserve both existing example prompts and follow the established formatting used by the other toolset docs.Source: Learnings
🤖 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/data-sources/builtin-toolsets/.nav.yml`:
- Around line 26-30: Move the “Trino: trino.md” entry out of the Grafana/Helm
group in the navigation list and place it in the later section containing the
other S/T entries, preserving the intended alphabetical order and Grafana-family
grouping.
In `@holmes/plugins/telegram/bot.py`:
- Around line 29-30: Add the required -> None return annotations to the
constructors, including the __init__ methods near Telegram bot initialization,
while preserving their existing parameters and behavior.
- Line 22: Make Telegram access fail closed by requiring a non-empty
allowed_chat_ids configuration in holmes/plugins/telegram/bot.py:22-22, unless
an explicitly named public-access setting is enabled; update the runtime
validation and preserve membership enforcement by default. In
docs/installation/telegram-installation.md:20-22, remove the claim that
TELEGRAM_ALLOWED_CHAT_IDS is optional and document the explicit public-access
opt-in if retained.
- Around line 143-160: Update handle_update so Holmes request/processing
failures are handled separately from Telegram delivery failures. Ensure every
error notification sent via send_message is best-effort and cannot propagate if
delivery fails, allowing polling in run to continue after transient Telegram
send errors.
- Around line 155-160: Update the exception handling around the Telegram request
flow to avoid exposing raw error details: use generic user-facing messages in
both self.telegram.send_message calls, and sanitize or redact token-bearing
Telegram URLs before logging requests.exceptions.RequestException and other
caught errors via logging.exception.
In `@holmes/plugins/toolsets/flink/flink.py`:
- Around line 55-68: Update the Flink fetch flow associated with
_FlinkTool._bounded so the underlying API request applies flink_config.max_items
before retrieving the collection, rather than fetching all values and truncating
afterward. Reuse the existing max_items configuration and preserve
holmes_truncated semantics for responses exceeding the limit; align the
implementation with Airflow’s _AirflowTool._bounded without duplicating
client-side-only logic.
- Line 1: Add a shared HTTPAPITool/toolset helper using JsonFilterMixin that
declares Flink’s supported collection fields and applies jq/max_depth filtering
during JSON handling. Update FlinkGetJob, FlinkGetExceptions, and
FlinkGetCheckpoints to use this helper instead of loading full responses and
manually slicing collections with the Airflow _bounded pattern.
- Around line 89-100: Update _FlinkJobTool._invoke to percent-encode the
validated job_id with path-segment encoding (quote using safe="") before
interpolating it into the /jobs/ request path, preventing ../ or
slash-containing IDs from altering URL resolution. Preserve the existing
missing-parameter handling and add a regression test covering percent-encoding
of a job ID containing path separators.
---
Nitpick comments:
In `@docs/data-sources/builtin-toolsets/airflow.md`:
- Around line 40-48: Update the “Common Use Cases” examples in the Airflow
documentation to follow the documented `holmes ask` house style, matching the
format used by the consolidated guidance and the corresponding Flink
documentation examples.
- Line 1: Update the Common Use Cases examples in the Airflow and Flink toolset
documentation to use the established house style: place each prompt in its own
separate ```bash block containing a holmes ask "..." command, replacing the
existing raw prompt text fences.
In `@docs/data-sources/builtin-toolsets/flink.md`:
- Around line 32-40: Update the Common Use Cases examples in the Flink
documentation to follow the documented `holmes ask` house style, including the
required command format for each example. Preserve the existing Flink questions
and intent while aligning both code blocks with the conventions used in the
consolidated guidance.
In `@docs/data-sources/builtin-toolsets/trino.md`:
- Around line 38-46: Update the Common Use Cases examples in the Trino toolset
documentation so each example is a complete holmes ask command, with its prompt
quoted, inside its own separate bash-fenced code block. Preserve both existing
example prompts and follow the established formatting used by the other toolset
docs.
In `@holmes/plugins/toolsets/airflow/airflow.py`:
- Around line 74-88: Remove the duplicate _bounded implementation from
_AirflowTool and reuse the shared bounded-result helper used by the Flink
toolset. Update the Airflow tool flow to call that common implementation while
preserving collection-key truncation and holmes_truncated behavior.
In `@holmes/plugins/toolsets/http_api_base.py`:
- Around line 91-102: Update the HTTP request flow in _request to reuse a
persistent requests.Session owned by each toolset instance instead of calling
requests.request directly. Initialize the session during toolset construction
and preserve the existing method, URL, parameters, authentication, timeout, SSL
verification, response validation, and return behavior.
- Around line 23-31: Change the bearer_token and password fields in the HTTP
configuration model to Optional[SecretStr], then update _request to unwrap them
with get_secret_value() before constructing authorization headers or
HTTPBasicAuth; preserve existing behavior when either value is None.
In `@holmes/plugins/toolsets/trino/trino.py`:
- Around line 141-190: In the method containing the paginated Trino request
loop, assign cast(TrinoToolset, self._toolset) to a local variable once near the
start, then reuse that variable for _trino_headers() and both trino_config
accesses. Preserve all existing request and pagination behavior.
🪄 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: cb1f330b-cd43-4dbe-b233-876b0e65496c
📒 Files selected for processing (25)
README.mddocs/data-sources/builtin-toolsets/.nav.ymldocs/data-sources/builtin-toolsets/airflow.mddocs/data-sources/builtin-toolsets/flink.mddocs/data-sources/builtin-toolsets/index.mddocs/data-sources/builtin-toolsets/trino.mddocs/installation/.nav.ymldocs/installation/telegram-installation.mddocs/why-holmesgpt.mdholmes/plugins/telegram/__init__.pyholmes/plugins/telegram/bot.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/airflow/__init__.pyholmes/plugins/toolsets/airflow/airflow.pyholmes/plugins/toolsets/airflow/instructions.jinja2holmes/plugins/toolsets/flink/__init__.pyholmes/plugins/toolsets/flink/flink.pyholmes/plugins/toolsets/flink/instructions.jinja2holmes/plugins/toolsets/http_api_base.pyholmes/plugins/toolsets/trino/__init__.pyholmes/plugins/toolsets/trino/instructions.jinja2holmes/plugins/toolsets/trino/trino.pypyproject.tomltests/plugins/test_telegram_bot.pytests/plugins/toolsets/test_data_platform_toolsets.py
| - Grafana Dashboards: grafanadashboards.md | ||
| - Loki: grafanaloki.md | ||
| - Tempo: grafanatempo.md | ||
| - Trino: trino.md | ||
| - Helm: helm.md |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Trino entry misplaced in the nav order.
Trino: trino.md is inserted between the Grafana-family cluster (Grafana (MCP)/Grafana Dashboards/Loki/Tempo) and Helm, breaking both the alphabetical continuation and that intentional grouping. It belongs near the other "S"/"T" entries further down.
📝 Proposed fix
- Tempo: grafanatempo.md
- - Trino: trino.md
- Helm: helm.mdand further down:
- SQLite: database-sqlite.md
+ - Trino: trino.md
- VictoriaLogs: victorialogs.md🤖 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 `@docs/data-sources/builtin-toolsets/.nav.yml` around lines 26 - 30, Move the
“Trino: trino.md” entry out of the Grafana/Helm group in the navigation list and
place it in the later section containing the other S/T entries, preserving the
intended alphabetical order and Grafana-family grouping.
| class TelegramBotConfig(BaseModel): | ||
| bot_token: str | ||
| holmes_api_url: str = "http://localhost:8080" | ||
| allowed_chat_ids: set[int] = Field(default_factory=set) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when no Telegram allowlist is configured.
An empty set disables the membership check at runtime, so any chat that can reach the bot can invoke HolmesGPT. Require at least one allowed chat ID by default, or add an explicit, deliberately named public-access opt-in.
holmes/plugins/telegram/bot.py#L22-L22: require a non-empty allowlist unless an explicit public-access setting is enabled.docs/installation/telegram-installation.md#L20-L22: remove the claim thatTELEGRAM_ALLOWED_CHAT_IDSis optional and document the explicit public-access opt-in if retained.
📍 Affects 2 files
holmes/plugins/telegram/bot.py#L22-L22(this comment)docs/installation/telegram-installation.md#L20-L22
🤖 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/plugins/telegram/bot.py` at line 22, Make Telegram access fail closed
by requiring a non-empty allowed_chat_ids configuration in
holmes/plugins/telegram/bot.py:22-22, unless an explicitly named public-access
setting is enabled; update the runtime validation and preserve membership
enforcement by default. In docs/installation/telegram-installation.md:20-22,
remove the claim that TELEGRAM_ALLOWED_CHAT_IDS is optional and document the
explicit public-access opt-in if retained.
| def __init__(self, config: TelegramBotConfig): | ||
| self.config = config |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add -> None to the constructors.
These methods lack required return type annotations.
Proposed fix
-class TelegramAPI:
- def __init__(self, config: TelegramBotConfig):
+class TelegramAPI:
+ def __init__(self, config: TelegramBotConfig) -> None:
-class HolmesAPI:
- def __init__(self, config: TelegramBotConfig):
+class HolmesAPI:
+ def __init__(self, config: TelegramBotConfig) -> None:
class HolmesTelegramBot:
def __init__(
self,
config: TelegramBotConfig,
telegram: Optional[TelegramAPI] = None,
holmes: Optional[HolmesAPI] = None,
- ):
+ ) -> None:As per coding guidelines, “Type hints required - use mypy for type checking.”
Also applies to: 74-75, 104-110
🤖 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/plugins/telegram/bot.py` around lines 29 - 30, Add the required ->
None return annotations to the constructors, including the __init__ methods near
Telegram bot initialization, while preserving their existing parameters and
behavior.
Source: Coding guidelines
| try: | ||
| response = self.holmes.ask( | ||
| prompt, | ||
| chat_id=chat_id, | ||
| history=self._histories.get(chat_id), | ||
| ) | ||
| history = response.get("conversation_history") | ||
| if isinstance(history, list): | ||
| self._histories[chat_id] = trim_conversation_history( | ||
| history, self.config.history_messages | ||
| ) | ||
| self.telegram.send_message(chat_id, response["analysis"]) | ||
| except requests.exceptions.RequestException as error: | ||
| logging.exception("Telegram request failed") | ||
| self.telegram.send_message(chat_id, f"HolmesGPT request failed: {error}") | ||
| except Exception as error: | ||
| logging.exception("Telegram update failed") | ||
| self.telegram.send_message(chat_id, f"HolmesGPT could not answer: {error}") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep a transient Telegram send failure from stopping polling.
A failure in send_message() enters an except block that immediately calls send_message() again. If that attempt also fails, it escapes handle_update() and terminates run(). Split Holmes failures from delivery failures, and make error-notification delivery best-effort.
🤖 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/plugins/telegram/bot.py` around lines 143 - 160, Update handle_update
so Holmes request/processing failures are handled separately from Telegram
delivery failures. Ensure every error notification sent via send_message is
best-effort and cannot propagate if delivery fails, allowing polling in run to
continue after transient Telegram send errors.
| except requests.exceptions.RequestException as error: | ||
| logging.exception("Telegram request failed") | ||
| self.telegram.send_message(chat_id, f"HolmesGPT request failed: {error}") | ||
| except Exception as error: | ||
| logging.exception("Telegram update failed") | ||
| self.telegram.send_message(chat_id, f"HolmesGPT could not answer: {error}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import requests
response = requests.Response()
response.status_code = 502
response.url = "https://api.telegram.org/botTOKEN_REDACTED/sendMessage"
try:
response.raise_for_status()
except requests.HTTPError as error:
print(str(error))
PYRepository: HolmesGPT/holmesgpt
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the Telegram bot adapter around the reported handler.
if [ -f holmes/plugins/telegram/bot.py ]; then
echo "### holmes/plugins/telegram/bot.py outline"
ast-grep outline holmes/plugins/telegram/bot.py || true
echo
echo "### holmes/plugins/telegram/bot.py lines 120-175"
sed -n '120,175p' holmes/plugins/telegram/bot.py | nl -ba -v120
else
echo "missing holmes/plugins/telegram/bot.py"
fi
echo
echo "### send_message implementation/usages"
rg -n "def send_message|send_message\(|Telegram|bot_token|api.telegram.org|logging\.exception" -S .Repository: HolmesGPT/holmesgpt
Length of output: 884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### holmes/plugins/telegram/bot.py lines 120-175"
sed -n '120,175p' holmes/plugins/telegram/bot.py
echo
echo "### send_message and telegram-api call sites"
python3 - <<'PY'
from pathlib import Path
p = Path('holmes/plugins/telegram/bot.py')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if i < 50 or i > 165:
continue
if 'send_message' in line or 'TelegramAPI' in line or 'api.telegram.org' in line or 'bot_token' in line or 'logger.exception' in line or 'logging.exception' in line:
print(f"{i}: {line}")
PYRepository: HolmesGPT/holmesgpt
Length of output: 3210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### holmes/plugins/telegram/bot.py lines 1-75"
sed -n '1,75p' holmes/plugins/telegram/bot.pyRepository: HolmesGPT/holmesgpt
Length of output: 2654
Do not expose raw request errors to Telegram users or logs.
This handler catches Telegram API request failures as well, and requests.HTTPError includes the full token-bearing URL in str(error). Use generic user-facing messages and redact the token-containing Telegram URL before logging.
🤖 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/plugins/telegram/bot.py` around lines 155 - 160, Update the exception
handling around the Telegram request flow to avoid exposing raw error details:
use generic user-facing messages in both self.telegram.send_message calls, and
sanitize or redact token-bearing Telegram URLs before logging
requests.exceptions.RequestException and other caught errors via
logging.exception.
| @@ -0,0 +1,178 @@ | |||
| import os | |||
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate JsonFilterMixin to confirm its interface before integrating
rg -n -A 15 'class JsonFilterMixin' --type=py
rg -nP 'JsonFilterMixin' --type=py -g '!**/test*'Repository: HolmesGPT/holmesgpt
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== matching file =="
fd -a 'flink\.py|airflow\.py|http_api_base\.py|pyproject\.toml' . | sed "s#$(pwd)/##"
echo
echo "== broad mixin/similar symbols in python files =="
rg -n 'JsonFilterMixin|_bounded|max_depth|jq|JsonFilter|filter' --type=py .
echo
echo "== toolset files sizes =="
for f in holmes/plugins/toolsets/flink/flink.py holmes/plugins/toolsets/airflow/airflow.py holmes/plugins/toolsets/http_api_base.py
do
if [ -f "$f" ]; then
wc -l "$f"
fi
done
echo
echo "== relevant slices =="
sed -n '1,160p' holmes/plugins/toolsets/flink/flink.py 2>/dev/null || true
echo "---"
sed -n '1,180p' holmes/plugins/toolsets/airflow/airflow.py 2>/dev/null || true
echo "---"
sed -n '1,200p' holmes/plugins/toolsets/http_api_base.py 2>/dev/null || trueRepository: HolmesGPT/holmesgpt
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
MODIFIED_FILES=$(git diff --name-only -- diff --stat 2>/dev/null || true)
echo "== files under holmes/plugins/toolsets =="
git ls-files 'holmes/plugins/toolsets/*.py' 'holmes/plugins/toolsets/**/.py' | sed 's#^\./##' | head -200
echo
echo "== JsonFilterMixin exact =="
rg -n 'class JsonFilterMixin|JsonFilterMixin|filter_parameters|max_depth|jq' --type=py holmes/plugins/toolsets holmes/plugins tests/plugins/toolsets 2>/dev/null | head -240
echo
echo "== Flink/Airflow/HTTPAPI relevant files =="
for f in holmes/plugins/toolsets/flink/flink.py holmes/plugins/toolsets/airflow/airflow.py holmes/plugins/toolsets/http_api_base.py; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,180p' "$f"
fi
done
echo
echo "== imports/usages of _bounded =="
rg -n 'def _bounded|_bounded\(|bounded' --type=py .Repository: HolmesGPT/holmesgpt
Length of output: 43828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full JsonFilterMixin =="
sed -n '1,190p' holmes/plugins/toolsets/json_filter_mixin.py
echo
echo "== Flink exception/checkpoint classes rest =="
sed -n '120,180p' holmes/plugins/toolsets/flink/flink.pyRepository: HolmesGPT/holmesgpt
Length of output: 9026
Consolidate unbounded response slicing into the shared JSON filter helper.
Flink’s FlinkGetJob, FlinkGetExceptions, and FlinkGetCheckpoints slice result collections only after requests.json() has loaded the full response, and Flink repeats the Airflow _bounded(..., collection_key) slicing pattern. For endpoints without server-side list filters, add a shared HTTPAPITool/toolset helper that exposes supported collection fields plus jq/max_depth filtering via JsonFilterMixin, so future additions don’t hand-roll the same unbounded-memory pattern.
🤖 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/plugins/toolsets/flink/flink.py` at line 1, Add a shared
HTTPAPITool/toolset helper using JsonFilterMixin that declares Flink’s supported
collection fields and applies jq/max_depth filtering during JSON handling.
Update FlinkGetJob, FlinkGetExceptions, and FlinkGetCheckpoints to use this
helper instead of loading full responses and manually slicing collections with
the Airflow _bounded pattern.
Source: Coding guidelines
| class _FlinkTool(HTTPAPITool): | ||
| def _bounded( | ||
| self, result: StructuredToolResult, collection_key: str | ||
| ) -> StructuredToolResult: | ||
| if result.status != StructuredToolResultStatus.SUCCESS or not isinstance( | ||
| result.data, dict | ||
| ): | ||
| return result | ||
| values = result.data.get(collection_key) | ||
| if isinstance(values, list): | ||
| max_items = cast(FlinkToolset, self._toolset).flink_config.max_items | ||
| result.data[collection_key] = values[:max_items] | ||
| result.data["holmes_truncated"] = len(values) > max_items | ||
| return result |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Unbounded fetch before client-side truncation; duplicated with Airflow's _bounded.
See consolidated comment covering both this file and airflow.py's _AirflowTool._bounded.
Also applies to: 89-100
🤖 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/plugins/toolsets/flink/flink.py` around lines 55 - 68, Update the
Flink fetch flow associated with _FlinkTool._bounded so the underlying API
request applies flink_config.max_items before retrieving the collection, rather
than fetching all values and truncating afterward. Reuse the existing max_items
configuration and preserve holmes_truncated semantics for responses exceeding
the limit; align the implementation with Airflow’s _AirflowTool._bounded without
duplicating client-side-only logic.
Source: Coding guidelines
| class _FlinkJobTool(_FlinkTool): | ||
| endpoint_suffix: ClassVar[str] = "" | ||
|
|
||
| def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: | ||
| job_id = str(params.get("job_id", "")).strip() | ||
| if not job_id: | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error="Missing required parameter 'job_id'", | ||
| params=params, | ||
| ) | ||
| return self._get_json(f"/jobs/{job_id}{self.endpoint_suffix}", params, context) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
URL-encode job_id before interpolating into the request path.
Unlike Airflow's dag_id/dag_run_id/task_id (percent-encoded via quote(..., safe="")), job_id here is inserted into the URL path raw. A job_id containing ../ sequences can escape the /jobs/ prefix and reach other paths on the same Flink host via urljoin's relative-path resolution in http_api_base.py's _request.
🔒 Proposed fix
+from urllib.parse import quote
...
def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult:
- job_id = str(params.get("job_id", "")).strip()
+ job_id = quote(str(params.get("job_id", "")).strip(), safe="")
if not job_id:
return StructuredToolResult(
status=StructuredToolResultStatus.ERROR,
error="Missing required parameter 'job_id'",
params=params,
)
return self._get_json(f"/jobs/{job_id}{self.endpoint_suffix}", params, context)Consider a regression test similar to Airflow's orders%2Fdaily percent-encoding test to cover this. Want me to draft one?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class _FlinkJobTool(_FlinkTool): | |
| endpoint_suffix: ClassVar[str] = "" | |
| def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: | |
| job_id = str(params.get("job_id", "")).strip() | |
| if not job_id: | |
| return StructuredToolResult( | |
| status=StructuredToolResultStatus.ERROR, | |
| error="Missing required parameter 'job_id'", | |
| params=params, | |
| ) | |
| return self._get_json(f"/jobs/{job_id}{self.endpoint_suffix}", params, context) | |
| from urllib.parse import quote | |
| class _FlinkJobTool(_FlinkTool): | |
| endpoint_suffix: ClassVar[str] = "" | |
| def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: | |
| job_id = quote(str(params.get("job_id", "")).strip(), safe="") | |
| if not job_id: | |
| return StructuredToolResult( | |
| status=StructuredToolResultStatus.ERROR, | |
| error="Missing required parameter 'job_id'", | |
| params=params, | |
| ) | |
| return self._get_json(f"/jobs/{job_id}{self.endpoint_suffix}", params, context) |
🤖 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/plugins/toolsets/flink/flink.py` around lines 89 - 100, Update
_FlinkJobTool._invoke to percent-encode the validated job_id with path-segment
encoding (quote using safe="") before interpolating it into the /jobs/ request
path, preventing ../ or slash-containing IDs from altering URL resolution.
Preserve the existing missing-parameter handling and add a regression test
covering percent-encoding of a job ID containing path separators.
Signed-off-by: Vladimir.Yakovlev <striker_vlad@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/installation/telegram-installation.md`:
- Around line 34-36: Update the kubectl secret creation command in the Telegram
installation instructions to use the Helm release namespace instead of
hardcoding holmes, ensuring the created Secret matches the Deployment’s
.Release.Namespace for installations in any namespace.
In `@helm/holmes/templates/telegram-deployment.yaml`:
- Around line 17-18: Update the Telegram Deployment strategy near replicas to
prevent overlapping pollers during rollouts: configure Recreate, or use
RollingUpdate with maxSurge set to 0 and maxUnavailable set to 1. Preserve the
existing replica count and other deployment settings.
🪄 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: d037af66-f884-4eb7-b245-5d81efeb211e
📒 Files selected for processing (6)
docs/installation/telegram-installation.mdhelm/holmes/templates/telegram-deployment.yamlhelm/holmes/values.yamlholmes/plugins/telegram/bot.pytests/plugins/test_telegram_bot.pytests/test_telegram_helm.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/telegram/bot.py
| kubectl create secret generic holmes-telegram \ | ||
| --from-literal=bot-token="<bot token>" \ | ||
| --namespace holmes |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Create the Secret in the release namespace.
The Deployment references Secrets in .Release.Namespace, but this command always creates it in holmes. Parameterize the namespace so installs in another namespace do not fail to start.
Proposed fix
+NAMESPACE=<helm-release-namespace>
kubectl create secret generic holmes-telegram \
--from-literal=bot-token="<bot token>" \
- --namespace holmes
+ --namespace "$NAMESPACE"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl create secret generic holmes-telegram \ | |
| --from-literal=bot-token="<bot token>" \ | |
| --namespace holmes | |
| NAMESPACE=<helm-release-namespace> | |
| kubectl create secret generic holmes-telegram \ | |
| --from-literal=bot-token="<bot token>" \ | |
| --namespace "$NAMESPACE" |
🤖 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 `@docs/installation/telegram-installation.md` around lines 34 - 36, Update the
kubectl secret creation command in the Telegram installation instructions to use
the Helm release namespace instead of hardcoding holmes, ensuring the created
Secret matches the Deployment’s .Release.Namespace for installations in any
namespace.
| spec: | ||
| replicas: 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent overlapping pollers during rollouts.
The default RollingUpdate strategy can create a second Pod before terminating the old one. That temporarily runs two long-pollers for the same bot token, risking duplicate or split update handling. Use Recreate (or maxSurge: 0 with maxUnavailable: 1).
Proposed fix
spec:
replicas: 1
+ strategy:
+ type: Recreate
selector:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec: | |
| replicas: 1 | |
| spec: | |
| replicas: 1 | |
| strategy: | |
| type: Recreate |
🤖 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 `@helm/holmes/templates/telegram-deployment.yaml` around lines 17 - 18, Update
the Telegram Deployment strategy near replicas to prevent overlapping pollers
during rollouts: configure Recreate, or use RollingUpdate with maxSurge set to 0
and maxUnavailable set to 1. Preserve the existing replica count and other
deployment settings.
1 participant
Summary by CodeRabbit
/reset, and safe long-message splitting.holmes-telegramconsole entry point and an opt-in Helm deployment for Telegram.