Fix full error log file upload and inline rendering for non-changepoi…#122
Merged
vishnuchalla merged 1 commit intoJul 6, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates ChangesSlack Error Log Handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bugzooka/integrations/slack_fetcher.py (1)
302-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreview text isn't sanitized the same way.
ANSI codes are stripped from the uploaded file content but not from
errors_log_previewused in the chat message (Line 248). If preview content can also contain ANSI escapes, the inline message will still render raw control codes while the uploaded file is clean.♻️ Suggested consistency fix
+ANSI_ESCAPE_RE = re.compile(r'\x1b\[[0-9;]*[a-zA-Z]') + ... - errors_log_preview = errors_preview[:preview_limit] + errors_log_preview = ANSI_ESCAPE_RE.sub('', errors_preview[:preview_limit]) ... - clean_content = re.sub(r'\x1b\[[0-9;]*[a-zA-Z]', '', content) + clean_content = ANSI_ESCAPE_RE.sub('', content)🤖 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 `@bugzooka/integrations/slack_fetcher.py` around lines 302 - 305, The preview text used in the Slack message is not sanitized consistently with the uploaded file content. Update the SlackFetcher flow in the same area that cleans content before files_upload_v2 so that errors_log_preview is passed through the same ANSI-stripping logic before it is inserted into the chat message, keeping the preview and attachment rendering consistent.
🤖 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 `@bugzooka/integrations/slack_fetcher.py`:
- Line 243: The needs_file check in SlackFetcher is too broad because
errors_for_file != errors_preview can force file uploads even when the
changepoint is still short; tighten the condition around needs_file so only logs
exceeding preview_limit or otherwise requiring truncation upload a file. Add a
changepoint test covering the short-but-different case in
slack_fetcher/SlackFetcher to verify that a short changepoint does not
unexpectedly attach a file.
---
Nitpick comments:
In `@bugzooka/integrations/slack_fetcher.py`:
- Around line 302-305: The preview text used in the Slack message is not
sanitized consistently with the uploaded file content. Update the SlackFetcher
flow in the same area that cleans content before files_upload_v2 so that
errors_log_preview is passed through the same ANSI-stripping logic before it is
inserted into the chat message, keeping the preview and attachment rendering
consistent.
🪄 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: Enterprise
Run ID: e3f71a19-96b2-4995-b55e-d178d28aa1d0
📒 Files selected for processing (1)
bugzooka/integrations/slack_fetcher.py
| needs_file = errors_for_file != errors_preview and ( | ||
| is_changepoint or len(errors_for_file) > preview_limit | ||
| ) | ||
| needs_file = errors_for_file != errors_preview or len(errors_for_file) > preview_limit |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for tests covering changepoint short-report file-upload behavior
rg -n "changepoint" -g '*test*' -C5 bugzookaRepository: redhat-performance/BugZooka
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant file outline ==\n'
ast-grep outline bugzooka/integrations/slack_fetcher.py --view expanded || true
printf '\n== surrounding lines around the condition ==\n'
nl -ba bugzooka/integrations/slack_fetcher.py | sed -n '210,330p'
printf '\n== test references to slack_fetcher / file upload behavior ==\n'
rg -n "slack_fetcher|needs_file|errors_preview|errors_for_file|preview_limit|upload a file|changepoint" bugzooka -g '*test*' -C 3 || trueRepository: redhat-performance/BugZooka
Length of output: 1183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file size ==\n'
wc -l bugzooka/integrations/slack_fetcher.py
printf '\n== function/method map for slack_fetcher ==\n'
ast-grep outline bugzooka/integrations/slack_fetcher.py --view expanded | sed -n '1,260p'
printf '\n== exact code around the branch ==\n'
nl -ba bugzooka/integrations/slack_fetcher.py | sed -n '230,310p'
printf '\n== search for changepoint-related tests or docs ==\n'
rg -n "changepoint|preview_limit|errors_for_file|errors_preview|needs_file" . -g '*test*' -g '*.md' -g '*.py' -C 2 || trueRepository: redhat-performance/BugZooka
Length of output: 1250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search for slack_fetcher usage and related tests ==\n'
rg -n "slack_fetcher|SlackFetcher|needs_file|errors_preview|errors_for_file|preview_limit" bugzooka -C 2 || true
printf '\n== locate tests under repository ==\n'
find . -path '*/.git' -prune -o -name '*test*.py' -o -name 'test_*.py' -o -name '*_test.py' -printRepository: redhat-performance/BugZooka
Length of output: 7394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('bugzooka/integrations/slack_fetcher.py')
print('exists', p.exists(), 'lines', sum(1 for _ in p.open()))
with p.open() as f:
for i, line in enumerate(f, 1):
if 220 <= i <= 310:
print(f"{i:4d}: {line.rstrip()}")
PY
printf '\n== nearby tests referencing SlackFetcher or file uploads ==\n'
rg -n "SlackFetcher|needs_file|errors_for_file|errors_preview|preview_limit|upload.*file|file.*upload" . -g '*test*' -g '*.py' -C 2 || trueRepository: redhat-performance/BugZooka
Length of output: 20085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== slack_fetcher call site ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('bugzooka/integrations/slack_fetcher.py')
for i, line in enumerate(p.open(), 1):
if 630 <= i <= 665:
print(f"{i:4d}: {line.rstrip()}")
PY
printf '\n== prow_analyzer changepoint branch ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('bugzooka/analysis/prow_analyzer.py')
for i, line in enumerate(p.open(), 1):
if 330 <= i <= 356:
print(f"{i:4d}: {line.rstrip()}")
PY
printf '\n== tests around error upload behavior ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('tests/test_slack_fetcher.py')
for i, line in enumerate(p.open(), 1):
if 110 <= i <= 260:
print(f"{i:4d}: {line.rstrip()}")
PYRepository: redhat-performance/BugZooka
Length of output: 10658
Short changepoint reports may now upload a file unexpectedly. At bugzooka/integrations/slack_fetcher.py:243, errors_for_file != errors_preview makes needs_file true whenever the full changepoint log differs from the preview, even if both are still under preview_limit. Add a changepoint test for the short-but-different case.
🤖 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 `@bugzooka/integrations/slack_fetcher.py` at line 243, The needs_file check in
SlackFetcher is too broad because errors_for_file != errors_preview can force
file uploads even when the changepoint is still short; tighten the condition
around needs_file so only logs exceeding preview_limit or otherwise requiring
truncation upload a file. Add a changepoint test covering the
short-but-different case in slack_fetcher/SlackFetcher to verify that a short
changepoint does not unexpectedly attach a file.
…nt failures Signed-off-by: Nabeel Mohd <mmnabeel317@gmail.com>
mmnabeel317
force-pushed
the
fix/restore-error-log-file-upload
branch
from
6bbca27 to
cdef80c
Compare
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.
Summary
full_errors.txtfile attachment for non-changepoint failures (logjuicer, logmine, cluster-operator error paths) which was broken since 0b73e6cRoot Cause
The
needs_filecondition in_send_error_logs_preview()usedandto gate onerrors_for_file != errors_preview. For all non-changepoint failure paths,full_errors_for_fileisNone, soerrors_for_filefalls back toerrors_preview— making them identical andneeds_filealwaysFalse. No file was ever uploaded for these failures, even when the preview truncated the content.Found by: Testing BugZooka against real Prow failures on a test Slack channel. A ROSA cluster install failure and a telco virt-6nodes orion-report failure both showed truncated previews with no full log attached. Tracing the code and git history (
git show 0b73e6c) revealed the regression — the original condition wasneeds_file = len(errors_for_file) > preview_limitwhich worked correctly, but was changed to anandgate that silently broke non-changepoint uploads.Rendering fix found by: After restoring the file upload, certain jobs rendered the file as a binary download card instead of an inline snippet. Testing with multiple real Prow jobs identified three categories of non-text content in build logs that trigger Slack's binary detection:
\x1b[1m,\x1b[31m) from Prow colored outputChanges
bugzooka/integrations/slack_fetcher.pyand→orinneeds_filecondition; sanitize file upload content (strip ANSI, control chars, emojis)Behavior
Test plan
/cc @vishnuchalla