Replace CairoSVG with resvg-py and migrate PDF generation to reportlab - #2136
Conversation
Pin the current behavior of the two code paths that depend on LGPL-licensed libraries (fpdf2 and CairoSVG), so they can be safely replaced or removed: - test_scan_report_rendering.py: Transformer.scanReportBlock_to_fileblock() KRR/Popeye PDF generation - happy paths with content verification, block pass-through, empty results, and hostile cell content (previously untested) - test_scan_sink_gating.py: the EnrichmentAnnotation.SCAN branch in the Slack and Mail sinks - scan blocks must be converted to report files before block conversion, uploaded/attached with correct filename and valid contents - test_svg_conversion.py: convert_svg_to_png / add_pngs_for_all_svgs and the MS Teams SVG->JPEG data-URL path, including a chart built through the real pipeline with custom CSS injection (previously untested) - test_ai_integration.py: strengthen PNG assertions with full image decoding, pin the pass-through-unchanged behavior for unconvertible graph events, and cover all ChartValuesFormat axis formats plus unknown-format fallback Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTG88e6gtCnRrrf8Pgyhdo
Removes the two LGPL-3.0 dependencies flagged in the license review, keeping behavior identical (validated by the pinning tests added in the previous commit - all 24 pass unchanged except the PDF-stream decoding in the test helper, plus 83 adjacent tests): - fpdf2 -> reportlab (BSD-3-Clause): scanReportBlock_to_fileblock() rewritten with reportlab platypus; same landscape-A4 layout, header with grade/score, accent config section, per-kind tables with markdown bold and repeated header rows. The import is now lazy, so the sink layer no longer requires the PDF toolchain at import time. - CairoSVG -> resvg (MPL-2.0, via MIT-licensed resvg-py bindings): convert_svg_to_png() now uses resvg_py; rendering fidelity verified side-by-side on a real robusta-styled pygal chart (custom CSS included), both rasterize identically at 1280x500. resvg-py ships manylinux wheels for x86_64 and aarch64, matching the multi-arch image build. The MS Teams sink now reuses the shared helper instead of importing cairosvg directly. - Dockerfile: libcairo2 is no longer needed; add fonts-dejavu-core explicitly since it previously arrived only as a transitive dependency of libcairo2 and chart text rendering still needs a system font. - playbooks/pyproject.toml: drop the CairoSVG dependency (nothing in the playbooks package imports it; the runner venv provides the rasterizer). - pyproject.toml: resvg-py added as a regular dependency, CairoSVG removed from the 'all' extra, pillow pin comment updated (it is a direct dependency of the MS Teams image conversion, no longer transitive via fpdf2/CairoSVG). - ATTRIBUTION.md: swap CairoSVG for resvg and ReportLab. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTG88e6gtCnRrrf8Pgyhdo
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:4e82af1
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:4e82af1 me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:4e82af1
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:4e82af1Patch Helm values in one line: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set runner.image=me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:4e82af1 |
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change replaces CairoSVG with resvg-py for SVG-to-PNG conversion and fpdf2 with ReportLab for scan-report PDFs. It updates runtime dependencies, Microsoft Teams image handling, graph rendering tests, PDF rendering tests, and Slack and Mail sink tests. ChangesRendering backend replacement
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GraphTool
participant convert_svg_to_png
participant resvg_py
participant MicrosoftTeams
participant PNGToJPEGEncoder
GraphTool->>convert_svg_to_png: SVG bytes
convert_svg_to_png->>resvg_py: UTF-8 SVG
resvg_py-->>convert_svg_to_png: PNG bytes
MicrosoftTeams->>convert_svg_to_png: SVG image
convert_svg_to_png-->>MicrosoftTeams: PNG bytes
MicrosoftTeams->>PNGToJPEGEncoder: PNG bytes
PNGToJPEGEncoder-->>MicrosoftTeams: JPEG data URL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/robusta/core/sinks/transformer.py (1)
298-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the column-width computation out of the loop.
scan.table_widthsdoes not change per section. Computingwidth_totalandcolumn_widthsonce before the loop removes repeated work and makes a zero-sum guard easier to add. Ifsum(scan.table_widths)is ever0, the current code raisesZeroDivisionError.🤖 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 `@src/robusta/core/sinks/transformer.py` around lines 298 - 299, Move the width_total and column_widths calculations out of the section loop in the surrounding transformer flow, computing them once from scan.table_widths before iteration. Add a zero-sum guard for width_total so zero-valued widths do not cause division by zero, while preserving the existing column-width behavior for positive totals.tests/test_scan_report_rendering.py (2)
180-181: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe grade assertions are too weak to fail on a regression.
assert "B" in textandassert "C" in textmatch any capitalBorCanywhere in the report."DaemonSet"alone satisfies neither, but"Deployment"-adjacent content, container names, and config text make these checks near-unconditional. A wrong grade would still pass.Assert the grade next to the score, for example
assert "B85" in text.replace(" ", ""), or assert on the concatenated header text.Also applies to: 203-204
🤖 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_scan_report_rendering.py` around lines 180 - 181, Strengthen the grade assertions in the scan report rendering tests by verifying each grade is adjacent to its expected score, such as checking the normalized header text for “B85” and the corresponding grade-score combination at the other referenced assertion. Replace broad standalone letter checks while preserving the existing score validations.
30-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe extractor handles only
Tj, but the docstring claimsTj/TJ.Line 77 matches
Tjoperators only. ReportLab can emitTJarrays for kerned or word-spaced runs. If that happens, text silently disappears and the assertions fail with a confusing message, or worse, pass for the wrong reason.Either extend the regex to
TJarrays or correct the docstring to state theTj-only limitation.🤖 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_scan_report_rendering.py` around lines 30 - 79, Update extract_pdf_text and its docstring consistently: either implement extraction for TJ array text-showing operators in addition to Tj, or revise the docstring to explicitly document that only Tj operators are supported. Prefer extending the extractor so ReportLab-generated kerned or word-spaced text is retained, while preserving existing unescaping and decoding behavior.tests/test_scan_sink_gating.py (1)
166-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
FileMock.instancesis shared state that leaks between tests.
instancesis a class attribute and no test resets it. The assertionlen(FileMock.instances) == 1on Line 194 passes only because one test usesFileMock. A second test that patchesNamedTemporaryFilewith this class makes the count assertion fail, and the failure looks unrelated to the new test.Define the list inside the fixture or clear it before use.
♻️ Proposed fix
class FileMock(BytesIO): instances = [] def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.name = f"tmpfile-{len(self.instances)}" FileMock.instances.append(self) def close(self): self._final_contents = self.getvalue() return super().close() + FileMock.instances = [] +🤖 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_scan_sink_gating.py` around lines 166 - 176, Update the FileMock test helper so its instances collection is reset for each test, preferably by defining or clearing it in the relevant fixture before use. Preserve FileMock.__init__ registration and the len(FileMock.instances) assertions while preventing state from leaking across tests.Source: Linters/SAST tools
🤖 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 `@pyproject.toml`:
- Around line 39-40: Update the license description in the comment near the
SVG-to-PNG rasterizer dependency, replacing “MPL-2.0” with “Apache-2.0/MIT”
while keeping the existing resvg-py MIT licensing text unchanged.
In `@src/robusta/core/sinks/transformer.py`:
- Around line 275-277: Update the score handling in scanReportBlock_to_fileblock
to parse scan.score defensively, catching invalid or empty values such as
decimals, "N/A", or blank strings; only append the score badge and header width
when parsing succeeds and the numeric score is non-negative, otherwise skip the
badge without aborting the report.
- Around line 294-311: Update the scan table construction in
scan_row_content_format to enable ReportLab in-row splitting by setting the
table’s splitInRow option to 1. Preserve the existing rows, column widths,
repeated header row, and table styling.
---
Nitpick comments:
In `@src/robusta/core/sinks/transformer.py`:
- Around line 298-299: Move the width_total and column_widths calculations out
of the section loop in the surrounding transformer flow, computing them once
from scan.table_widths before iteration. Add a zero-sum guard for width_total so
zero-valued widths do not cause division by zero, while preserving the existing
column-width behavior for positive totals.
In `@tests/test_scan_report_rendering.py`:
- Around line 180-181: Strengthen the grade assertions in the scan report
rendering tests by verifying each grade is adjacent to its expected score, such
as checking the normalized header text for “B85” and the corresponding
grade-score combination at the other referenced assertion. Replace broad
standalone letter checks while preserving the existing score validations.
- Around line 30-79: Update extract_pdf_text and its docstring consistently:
either implement extraction for TJ array text-showing operators in addition to
Tj, or revise the docstring to explicitly document that only Tj operators are
supported. Prefer extending the extractor so ReportLab-generated kerned or
word-spaced text is retained, while preserving existing unescaping and decoding
behavior.
In `@tests/test_scan_sink_gating.py`:
- Around line 166-176: Update the FileMock test helper so its instances
collection is reset for each test, preferably by defining or clearing it in the
relevant fixture before use. Preserve FileMock.__init__ registration and the
len(FileMock.instances) assertions while preventing state from leaking across
tests.
🪄 Autofix
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: d69b3620-7f1c-4e27-8bb5-f92753cab24e
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
ATTRIBUTION.mdDockerfileplaybooks/pyproject.tomlpyproject.tomlsrc/robusta/core/reporting/utils.pysrc/robusta/core/sinks/transformer.pysrc/robusta/integrations/msteams/msteams_adaptive_card_files_image.pytests/test_ai_integration.pytests/test_scan_report_rendering.pytests/test_scan_sink_gating.pytests/test_svg_conversion.py
💤 Files with no reviewable changes (1)
- playbooks/pyproject.toml
The previous relock used Poetry 2.3.3, which rewrote the whole file in the 2.1 lock format. Regenerate with Poetry 1.8.5 (the version that produced the original lock) via 'poetry lock --no-update' so the format stays 2.0 and the diff against master is limited to the intended dependency swap: fpdf2 and CairoSVG (plus their orphaned transitives cairocffi, cssselect2, tinycss2, webencodings, defusedxml) out; reportlab and resvg-py in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTG88e6gtCnRrrf8Pgyhdo
…t tall table rows - pyproject.toml: resvg-py 0.3.4 bundles resvg 0.48.0, which is licensed Apache-2.0 OR MIT (the MPL-2.0 note was resvg's pre-linebender license) - verified against the v0.48.0 Cargo.toml and license files. - transformer.py: a non-integer scan score (e.g. "85.5", "N/A", "") no longer raises out of scanReportBlock_to_fileblock and kills the notification; the score badge is skipped with a warning instead. Matches the old fpdf2-era guard semantics for valid integer scores. Note scan.grade parses the score too, so the whole badge block sits inside the guard. - transformer.py: enable splitByRow/splitInRow on scan tables so a single row taller than the page (e.g. a Popeye resource with hundreds of issues in one cell) splits across pages instead of raising LayoutError. - tests: pin both behaviors (parametrized bad-score cases and a 200-issue tall-row case). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTG88e6gtCnRrrf8Pgyhdo
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 `@tests/test_scan_report_rendering.py`:
- Around line 232-241: Strengthen
test_non_integer_score_skips_badge_without_breaking_report by asserting the
score-specific header text is absent from the extracted PDF text for every
bad_score, including empty and None values, while retaining the existing
report-rendering assertions.
- Around line 244-258: Update test_row_taller_than_page_splits_across_pages to
assert the rendered PDF has more than one page and verify every generated issue
number 0 through 199 appears in the extracted text, preserving the full expected
message text rather than checking only the endpoints.
🪄 Autofix
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: b0792f1b-e30d-4dba-a51a-d4d1e8ea4a9e
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/robusta/core/sinks/transformer.pytests/test_scan_report_rendering.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- src/robusta/core/sinks/transformer.py
…ination - test_non_integer_score_skips_badge_without_breaking_report now asserts the invalid score text is absent from the rendered PDF, not only that the report still renders. - test_row_taller_than_page_splits_across_pages now asserts the PDF spans multiple pages and that every one of the 200 issue messages survives the in-row split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTG88e6gtCnRrrf8Pgyhdo
Supply-chain hardening: the resvg-py binding package has a small maintainer base, so pin the exact version instead of a caret range. Lock file content-hash refreshed with Poetry 1.8.5 (no dependency versions changed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTG88e6gtCnRrrf8Pgyhdo
Summary
This PR replaces the LGPL-licensed CairoSVG library with the MIT-licensed resvg-py for SVG-to-PNG rasterization, and migrates PDF report generation from fpdf2 to reportlab. These changes improve licensing compliance and provide better control over report formatting.
Key Changes
SVG Rasterization
convert_svg_to_png()incore/reporting/utils.pyto use resvg-py instead of cairosvgPDF Report Generation
scanReportBlock_to_fileblock()incore/sinks/transformer.pyfrom fpdf2 to reportlabcell_markup()helper to escape and convert markdown formatting (bold and newlines) to reportlab paragraph markupMS Teams Integration
MsTeamsAdaptiveCardFilesImageto use the newconvert_svg_to_png()functionTesting
tests/test_scan_report_rendering.pywith:tests/test_scan_sink_gating.pyto verify scan enrichments are properly converted before sink processingtests/test_svg_conversion.pyto pin SVG-to-PNG conversion behaviortests/test_ai_integration.pywith PNG validation helpers and tests for graph tool output handlingImplementation Details
https://claude.ai/code/session_01LTG88e6gtCnRrrf8Pgyhdo