fix(telegram): escape MarkdownV2 to stop 'can't parse entities' (#1982)#2105
Open
arun2dot0 wants to merge 12 commits into
Open
fix(telegram): escape MarkdownV2 to stop 'can't parse entities' (#1982)#2105arun2dot0 wants to merge 12 commits into
arun2dot0 wants to merge 12 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…busta-dev#1982) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…busta-dev#1982) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…v#1982) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…hod (robusta-dev#1982) Final-review fixes: - escape ) and \ inside link URLs per MarkdownV2 spec - remove unused TelegramTransformer.to_markdownv2 batch method (YAGNI) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a-dev#1982) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR fixes Telegram "can't parse entities" errors caused by unescaped Markdown in Kubernetes resource names. It adds a configurable ChangesTelegram MarkdownV2 Escaping and Configurable parse_mode
Sequence DiagramsequenceDiagram
participant Sink as TelegramSink
participant Transformer as TelegramTransformer
participant Client as TelegramClient
participant API as Telegram API
Sink->>Sink: __init__(sink_config)
Sink->>Transformer: TelegramTransformer(params.parse_mode)
Sink->>Client: TelegramClient(..., parse_mode=params.parse_mode)
Sink->>Sink: __get_message_text(blocks)
Sink->>Transformer: block_to_markdownv2(block)
Transformer->>Transformer: escape_markdownv2 or plain text format
Transformer-->>Sink: formatted block text
Sink->>Client: send_message(formatted_text)
Client->>API: POST with parse_mode in payload (or omit if None)
API-->>Client: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_telegram_transformer.py (1)
143-166: ⚡ Quick winAdd a regression test for action-link aggregation in sink text generation.
Given the sink wiring changes, add a case with multiple
finding.links(and optionallyplatform_enabled=True) to ensure actions are appended, not replaced.🤖 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_telegram_transformer.py` around lines 143 - 166, Add a new test function after the existing tests in the file that creates a Finding object with multiple links (in the `finding.links` attribute) to verify that action links are properly aggregated and appended in the sink text output rather than being replaced. The test should call the _render_sink_text helper function (potentially with an updated version that accepts links as a parameter) and assert that all links appear in the generated message text. Additionally, consider testing with platform_enabled=True to ensure the behavior is consistent across different platform enablement states.
🤖 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 `@src/robusta/core/sinks/telegram/telegram_client.py`:
- Around line 21-31: Add a timeout parameter to the requests.post call in the
send_message method to prevent indefinite hangs when the Telegram API is slow or
unreachable. Also apply the same fix to the send_file method mentioned in the
comment. Include a reasonable timeout value (e.g., 10-30 seconds) as a parameter
in all external network calls to ensure the notification sink remains responsive
and available.
In `@src/robusta/core/sinks/telegram/telegram_sink.py`:
- Around line 96-97: The actions_content variable is being overwritten on each
iteration of the finding.links loop instead of accumulating the results. In the
loop where you iterate through finding.links and call self.transformer.link,
change the assignment operation to accumulate the link content rather than
replace it (use an accumulation pattern like += instead of = assignment) so that
all links are preserved in the final actions_content variable instead of only
the last link being retained.
In `@src/robusta/core/sinks/telegram/telegram_transformer.py`:
- Around line 19-24: The bold pattern in the _INLINE_RE regex currently only
matches single asterisks (*bold*) but not GitHub-style double asterisks
(**bold**). Update the bold regex pattern to handle both single and double
asterisks for bold text formatting. Additionally, review the code that processes
these regex groups (around lines 87-110) to ensure it correctly handles both
bold pattern variations when converting them to Telegram formatting.
---
Nitpick comments:
In `@tests/test_telegram_transformer.py`:
- Around line 143-166: Add a new test function after the existing tests in the
file that creates a Finding object with multiple links (in the `finding.links`
attribute) to verify that action links are properly aggregated and appended in
the sink text output rather than being replaced. The test should call the
_render_sink_text helper function (potentially with an updated version that
accepts links as a parameter) and assert that all links appear in the generated
message text. Additionally, consider testing with platform_enabled=True to
ensure the behavior is consistent across different platform enablement states.
🪄 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
Run ID: 53a999d4-0433-45d6-9708-b76b080c0cb0
📒 Files selected for processing (6)
docs/configuration/sinks/telegram.rstsrc/robusta/core/sinks/telegram/telegram_client.pysrc/robusta/core/sinks/telegram/telegram_sink.pysrc/robusta/core/sinks/telegram/telegram_sink_params.pysrc/robusta/core/sinks/telegram/telegram_transformer.pytests/test_telegram_transformer.py
arun2dot0
force-pushed
the
fix/telegram-markdownv2-escaping
branch
from
72ec427 to
021b18a
Compare
…sta-dev#1982) Address PR review: - pass a 30s timeout to both Telegram API calls so a slow/unreachable API can't hang the notification path - accumulate finding.links into actions_content instead of overwriting, so all links (and Investigate/Silence) survive - regression tests for link aggregation and the request timeout Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_telegram_transformer.py (1)
180-187: ⚡ Quick winImport and assert against the timeout constant instead of hardcoding the expected value.
The test hardcodes
timeout == 30, but the actual code uses theTELEGRAM_REQUEST_TIMEOUT_SECONDSconstant. This creates unnecessary coupling; if the constant changes, the test will fail even though the behavior is correct.♻️ Proposed refactor to reference the constant
def test_client_send_message_passes_timeout(): - from robusta.core.sinks.telegram.telegram_client import TelegramClient + from robusta.core.sinks.telegram.telegram_client import TelegramClient, TELEGRAM_REQUEST_TIMEOUT_SECONDS client = TelegramClient(chat_id=1, thread_id=None, bot_token="x") with patch("robusta.core.sinks.telegram.telegram_client.requests.post") as post: post.return_value.status_code = 200 client.send_message("hi") - assert post.call_args.kwargs["timeout"] == 30 + assert post.call_args.kwargs["timeout"] == TELEGRAM_REQUEST_TIMEOUT_SECONDS🤖 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_telegram_transformer.py` around lines 180 - 187, The test_client_send_message_passes_timeout function hardcodes the expected timeout value as 30 instead of referencing the actual constant used in the implementation. Import the TELEGRAM_REQUEST_TIMEOUT_SECONDS constant from the robusta.core.sinks.telegram.telegram_client module and replace the hardcoded value 30 in the assertion with this constant so that the test will automatically stay in sync if the timeout value changes in the future.
🤖 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_telegram_transformer.py`:
- Around line 180-187: The test_client_send_message_passes_timeout function
hardcodes the expected timeout value as 30 instead of referencing the actual
constant used in the implementation. Import the TELEGRAM_REQUEST_TIMEOUT_SECONDS
constant from the robusta.core.sinks.telegram.telegram_client module and replace
the hardcoded value 30 in the assertion with this constant so that the test will
automatically stay in sync if the timeout value changes in the future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d4cea64-9ee9-45f9-bc75-cb394529f60e
📒 Files selected for processing (3)
src/robusta/core/sinks/telegram/telegram_client.pysrc/robusta/core/sinks/telegram/telegram_sink.pytests/test_telegram_transformer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/robusta/core/sinks/telegram/telegram_sink.py
…obusta-dev#1982) Address PR review: handle double-asterisk bold (and strip it in plain mode) so it renders as MarkdownV2 *bold* instead of leaving stray literal asterisks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
Fixes #1982 — the Telegram sink fails with
Bad Request: can't parse entitieswhen alert content contains Markdown special characters (e.g. Kubernetes names with underscores likecrowdsec-agent_k8vkt).Why the obvious fixes don't work
*…*), code-formatted source (`…`), and clickable action links ([…](…)). So escaping the whole assembled message would break the sink's own formatting."Markdown"mode has no escape mechanism — you cannot backslash-escape a_. Escaping only works in MarkdownV2.**bold**, tables, dotted paths), which is not valid MarkdownV2 either — so the fix has to render the body specifically for Telegram.What this PR does
TelegramTransformer(telegram_transformer.py) which owns MarkdownV2 escaping and block→text rendering. Structural formatting is built by the sink with dynamic values escaped, so intentional bold/code/links are preserved while pod names, titles, descriptions, etc. are made safe.parse_modesink param:"MarkdownV2"(default) ornullfor guaranteed-safe plain text. Validated to those two values; threaded throughTelegramClient(theparse_modekey is omitted entirely when plain).MarkdownBlocktext goes through a small inline converter that preserves the constructs Robusta actually emits (*bold*,`code`,<url|text>and[text](url)links) and escapes everything else. Worst case for exotic markup is a literal*rendering rather than a parse crash.)and\per the MarkdownV2 spec.parse_modeindocs/configuration/sinks/telegram.rst.Testing
New pure-Python suite
tests/test_telegram_transformer.py(22 tests), covering the#1982repro (crowdsec-agent_k8vkt), every special char, the inline converter (bold/code/both link forms, adversarial unbalanced markers), URL escaping, theparse_modeparam/validator, client request shaping, and an end-to-end sink render in both modes.tests/test_app_imports.pyconfirms no import regressions. Black/isort clean (line-length 120).Backwards compatibility
Default behavior changes from legacy
MarkdowntoMarkdownV2, which is what makes messages robust by default. Users who want unformatted messages can setparse_mode: null.Follow-up recommendations (not in this PR)
MarkdownBlocktext (italic_…_, strikethrough~…~, spoiler||…||, blockquote). The converter has an explicit extension point comment for this.TelegramTransformerfollows a pattern that generalizes well: a per-platform transformer that (a) exposes structural builders (bold/code/link), (b) centralizes escaping for the target markup, and (c) renders typed blocks. Today each markup-based sink (Slack, MS Teams, Discord, Mattermost, …) handles these concerns ad hoc via the sharedTransformer. Extracting a small base interface (e.g.escape,bold,code,link,block_to_text) that each sink implements would give consistent, individually-testable, escaping-correct rendering across platforms and reduce per-sink markup bugs like this one. Worth its own design issue.