You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "fitzee (via GitHub)" <gi...@apache.org> on 2026/09/20 23:41:44 UTC
[PR] fix(reports): validate final capture artifacts before delivery [superset]
fitzee opened a new pull request, #44465:
URL: https://github.com/apache/superset/pull/44465
## TL;DR
Validate the actual screenshot bytes used in scheduled PDF/PNG delivery, and require the delivered PDF to match the bytes assembled from validated images. This is the capture-only split from #44336, based directly on master; it does not depend on that PR or add retry fields or migrations.
### SUMMARY
Master already includes capture readiness, perceptual blank detection, capture-time stability checks and sticky capture rejection (#43784, #44034, #44114, #44191). This adds final-image validation, exact-PDF provenance and fail-closed handling when standard capture cannot establish content state. It also adds chart-holder diagnostics.
The detection-policy changes require separate scrutiny: more neutral light-grey backgrounds are classified as blank, while sparse readable text and solid-colour visualizations are retained. A title-only page or dark background can still pass. Visible query-error panels remain deliverable, with diagnostic warnings. This is not a guarantee that every rendering defect is detected.
PDF provenance validates its source images and exact assembled bytes; it does not rasterize and visually inspect the finished PDF. The diff includes realistic image fixtures and diagnostics tests alongside the delivery-boundary checks. Whole-execution retries and ownership fencing remain in #44336.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI change. Previously, successful capture provenance alone could allow corrupt or unvalidated final bytes through. These additional checks reject those artifacts before delivery. The classifier fixtures cover white/light-grey placeholders, sparse labels and solid-colour output; live browser canaries remain necessary.
### TESTING INSTRUCTIONS
- 357 unit tests passed across report execution, screenshot utilities, webdriver and execution-context tests.
- 146 PostgreSQL report/dashboard integration tests passed.
- All changed-file pre-commit hooks passed, including mypy, Ruff and pylint.
- Regression tests cover undecodable images, near-white placeholders, sparse content and substitution of PDF bytes after validated assembly.
- Manually exercise scheduled PDF and PNG output, including legitimate empty/error panels, sparse charts and tall dashboards. Confirm rejected captures never reach notification transport and inspect holder-state diagnostics.
### ADDITIONAL INFORMATION
- [ ] Has associated issue
- [ ] Required new feature flags
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Risk: false rejection can suppress delivery; false negatives remain possible. The classifier also affects tiled-thumbnail retry decisions. Reverting this PR restores master's existing capture safeguards, not an unchecked capture path. No schema rollback is needed. CI and live scheduled-report canaries must pass before rollout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4066846264
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error is
+not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate. It correctly identifies that the term 'terminal' in the documentation could be misinterpreted as a complete capture failure, whereas the code indicates that terminal chart errors are still considered capture-ready and are handled with a warning. Updating the wording to clarify that the readiness wait stops without retry, rather than blocking delivery, improves the accuracy of the documentation.
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error is
+not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
+
+This is not a guarantee that every delivered image contains useful data. A large
+enough title or logo can still exceed the detector's content thresholds while the
+rest of the page is incomplete; removing the sparse-content override does not
+solve that general limitation. Recognized terminal chart errors are capture-ready
+and may be delivered with a warning, provided the
+image passes visual validation. Operators requiring all charts to contain data
+must not treat this policy as that guarantee. The strict invariant is that an
Review Comment:
<!-- Bito Reply -->
The suggestion to consolidate the disclaimer text is appropriate. The updated text in the provided diff successfully merges the three separate caveats into a single, cohesive paragraph, which improves the readability and scannability of the troubleshooting section.
**docs/admin_docs/configuration/alerts-reports.mdx**
```
+Final screenshot bytes are validated independently of the capture implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
```
##########
tests/unit_tests/utils/test_report_execution.py:
##########
@@ -44,6 +46,48 @@ def _report_config(**overrides: int) -> dict[str, int | bool]:
return config
+def test_chart_holder_diagnostics_separate_terminal_errors_from_success() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "rendered"},
+ {"chartId": "3", "state": "empty"},
+ {"chartId": "4", "state": "error"},
+ {"chartId": "5", "state": "virtualized"},
+ ]
+ )
+
+ assert diagnostics.mounted_holders == 5
+ assert diagnostics.ready_holders == 5
+ assert diagnostics.rendered_holders == 2
+ assert diagnostics.empty_holders == 1
+ assert diagnostics.error_holders == 1
+ assert diagnostics.virtualized_holders == 1
+ assert diagnostics.unready_holders == 0
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_count_unready_holders() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "waiting_on_database"},
+ {"chartId": "3", "state": "nothing_mounted"},
+ ]
+ )
+
+ assert diagnostics.ready_holders == 1
+ assert diagnostics.unready_holders == 2
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_do_not_treat_zero_holders_as_success() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states([])
+
+ assert diagnostics.mounted_holders == 0
+ assert diagnostics.semantic_success is False
Review Comment:
<!-- Bito Reply -->
The user has addressed the reviewer's suggestion by adding docstrings to the three new test functions in `tests/unit_tests/utils/test_report_execution.py`. This change aligns the code with the repository's documentation requirements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4059857465
##########
superset/utils/webdriver.py:
##########
@@ -333,13 +336,16 @@ def _get_validated_screenshot(
else REPORT_HAS_RENDERED_CHART_HOLDERS_JS
)
)
- except PlaywrightError:
- has_rendered_content = False
+ except PlaywrightError as ex:
+ report_execution_context.reject_capture("content_state_unknown")
logger.warning(
"report_capture_content_state_failed capture=standard%s",
context_suffix,
exc_info=True,
)
+ raise ScreenshotBlankCaptureError(
Review Comment:
`for_delivery=False` doesn't bypass this, so the CSV/XLSX/TEXT query-context bootstrap still passes the execution context and a flaky content-state `evaluate` now fails the tabular report (previously `has_rendered_content=False` and it continued). That seems to contradict "retaining the existing browser readiness/capture contract" for bootstrap captures.
##########
superset/utils/screenshot_utils.py:
##########
@@ -145,6 +148,31 @@ class ScreenshotBlankCaptureError(RuntimeError):
"""Raised when Chromium repeatedly returns a perceptually blank capture."""
+def validate_report_screenshot(
+ screenshot: bytes,
+ context: ReportExecutionContext,
+) -> None:
+ """Validate exact image bytes independently of browser or cache provenance."""
+ if context.capture_was_rejected:
+ raise ScreenshotBlankCaptureError("Capture was already rejected")
+ if context.artifact_was_validated(screenshot):
+ return
+ try:
+ with Image.open(io.BytesIO(screenshot)) as image:
+ image.verify()
+ blankness = get_screenshot_blankness_metrics(screenshot)
+ except (SoftTimeLimitExceeded, ReportExecutionBudgetExceededError):
+ context.reject_capture("validation_interrupted")
+ raise
+ except Exception as ex:
+ context.reject_capture("invalid_image")
+ raise ScreenshotBlankCaptureError("Unable to validate screenshot bytes") from ex
+ if blankness.is_blank:
Review Comment:
Both capture paths accept a blank image when no holder has rendered content (`is_blank = has_rendered_content and ...`), but this gate then rejects the same bytes with no retry. Is failing an empty/error-only chart or dashboard on the first attempt intended?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44465:
URL: https://github.com/apache/superset/pull/44465
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on PR #44465:
URL: https://github.com/apache/superset/pull/44465#issuecomment-5768230031
Reviewed the latest Bito suggestions. Clarified readiness versus delivery, consolidated the heuristic caveat, explained the logo threshold distinction, and added the requested diagnostics-test docstrings in f4ef4cc50fd26c02e4963240c523e285019a4541. All 141 selected tests and PR pre-commit checks pass.
The tiled logging observation is a real scope limitation, not a new delivery check: final counts aggregate terminal states observed across captured viewports, rather than every holder ever present in the DOM. Documented that limitation and the need to compare per-tile diagnostics with expected holder counts. No claim that never-visible holders were captured.
Leaving the small semantic-success helper extraction, threshold constant extraction, inferable local annotations, and broad test-style cleanup out of this fix. The nested blank check intentionally distinguishes blank pixels with no rendered chart content; the shared validator still checks those bytes before approval. No runtime behaviour changed in this cleanup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4060292971
##########
superset/utils/webdriver.py:
##########
@@ -333,13 +336,16 @@ def _get_validated_screenshot(
else REPORT_HAS_RENDERED_CHART_HOLDERS_JS
)
)
- except PlaywrightError:
- has_rendered_content = False
+ except PlaywrightError as ex:
+ report_execution_context.reject_capture("content_state_unknown")
logger.warning(
"report_capture_content_state_failed capture=standard%s",
context_suffix,
exc_info=True,
)
+ raise ScreenshotBlankCaptureError(
Review Comment:
Fixed in 857769fed8fe5d4cb2c0c5419cabb8310723cb61. Bootstrap-only captures no longer receive the delivery-validation context, preserving the existing CSV/XLSX/TEXT query-context bootstrap behavior.
##########
superset/utils/screenshot_utils.py:
##########
@@ -145,6 +148,31 @@ class ScreenshotBlankCaptureError(RuntimeError):
"""Raised when Chromium repeatedly returns a perceptually blank capture."""
+def validate_report_screenshot(
+ screenshot: bytes,
+ context: ReportExecutionContext,
+) -> None:
+ """Validate exact image bytes independently of browser or cache provenance."""
+ if context.capture_was_rejected:
+ raise ScreenshotBlankCaptureError("Capture was already rejected")
+ if context.artifact_was_validated(screenshot):
+ return
+ try:
+ with Image.open(io.BytesIO(screenshot)) as image:
+ image.verify()
+ blankness = get_screenshot_blankness_metrics(screenshot)
+ except (SoftTimeLimitExceeded, ReportExecutionBudgetExceededError):
+ context.reject_capture("validation_interrupted")
+ raise
+ except Exception as ex:
+ context.reject_capture("invalid_image")
+ raise ScreenshotBlankCaptureError("Unable to validate screenshot bytes") from ex
+ if blankness.is_blank:
Review Comment:
Fixed in 857769fed8fe5d4cb2c0c5419cabb8310723cb61. A blank capture is approved as a screenshot when browser state positively establishes that no holder rendered content, so empty/error-only results pass the final gate without weakening validation of other blank captures.
##########
superset/utils/screenshot_utils.py:
##########
@@ -145,6 +148,31 @@ class ScreenshotBlankCaptureError(RuntimeError):
"""Raised when Chromium repeatedly returns a perceptually blank capture."""
+def validate_report_screenshot(
+ screenshot: bytes,
+ context: ReportExecutionContext,
+) -> None:
+ """Validate exact image bytes independently of browser or cache provenance."""
+ if context.capture_was_rejected:
+ raise ScreenshotBlankCaptureError("Capture was already rejected")
+ if context.artifact_was_validated(screenshot):
+ return
+ try:
+ with Image.open(io.BytesIO(screenshot)) as image:
+ image.verify()
+ blankness = get_screenshot_blankness_metrics(screenshot)
Review Comment:
Fixed in 857769fed8fe5d4cb2c0c5419cabb8310723cb61. Final screenshot validation now propagates decoding failures from the blankness helper and rejects the artifact as `invalid_image`. A regression test covers a PNG whose IDAT payload is corrupted with a recomputed CRC.
##########
superset/utils/report_execution.py:
##########
@@ -245,6 +257,14 @@ def reject_capture(self, reason: str) -> None:
self._capture_rejection_reasons.append(reason)
+ def approve_artifact(self, artifact: bytes) -> None:
+ """Record exact bytes validated by the capture or PDF assembly stage."""
+ self._validated_artifacts.add(hashlib.sha256(artifact).hexdigest())
Review Comment:
Fixed in 857769fed8fe5d4cb2c0c5419cabb8310723cb61. Artifact approvals are now keyed by both kind and digest, preventing screenshot approval from authorizing identical bytes as a PDF.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44465:
URL: https://github.com/apache/superset/pull/44465#issuecomment-5769836293
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44465:
URL: https://github.com/apache/superset/pull/44465#issuecomment-5753620617
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44465?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `18.18182%` with `117 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.31%. Comparing base ([`c9fd9bf`](https://app.codecov.io/gh/apache/superset/commit/c9fd9bf94f45163afd24f39f5ed9eec23a999150?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ae2cfdf`](https://app.codecov.io/gh/apache/superset/commit/ae2cfdf6bd418460da64b01c78828c701493365a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44465?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/screenshot\_utils.py](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&filepath=superset%2Futils%2Fscreenshot_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvc2NyZWVuc2hvdF91dGlscy5weQ==) | 4.28% | [67 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/report/execute.py](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&filepath=superset%2Fcommands%2Freport%2Fexecute.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L2V4ZWN1dGUucHk=) | 9.09% | [30 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/webdriver.py](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&filepath=superset%2Futils%2Fwebdriver.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvd2ViZHJpdmVyLnB5) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/report\_execution.py](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&filepath=superset%2Futils%2Freport_execution.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvcmVwb3J0X2V4ZWN1dGlvbi5weQ==) | 68.96% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (c9fd9bf) and HEAD (ae2cfdf). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (c9fd9bf) | HEAD (ae2cfdf) |
>|------|------|------|
>|python|26|2|
>|presto|5|1|
>|hive|5|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44465 +/- ##
===========================================
- Coverage 80.65% 57.31% -23.34%
===========================================
Files 2942 2942
Lines 175691 175817 +126
Branches 40788 40811 +23
===========================================
- Hits 141703 100773 -40930
- Misses 31326 74084 +42758
+ Partials 2662 960 -1702
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.12% <18.18%> (-0.03%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.01% <18.18%> (-0.03%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.05% <18.18%> (-45.91%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44465?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4062016943
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error is
+not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Ambiguous terminal wording</b></div>
<div id="fix">
'Chart error and empty states are terminal for readiness' is ambiguous: 'terminal' suggests capture fails, but lines 498-499 say recognized terminal chart errors are 'capture-ready and may be delivered with a warning'. Code agrees errors only warn (`report_semantic_status` at `screenshot_utils.py:1642-1659`). Please reword to say the readiness wait stops without retry, not that delivery is blocked.
</div>
</div>
<small><i>Code Review Run #b4ab75</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error is
+not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
+
+This is not a guarantee that every delivered image contains useful data. A large
+enough title or logo can still exceed the detector's content thresholds while the
+rest of the page is incomplete; removing the sparse-content override does not
+solve that general limitation. Recognized terminal chart errors are capture-ready
+and may be delivered with a warning, provided the
+image passes visual validation. Operators requiring all charts to contain data
+must not treat this policy as that guarantee. The strict invariant is that an
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Triplicated disclaimer text</b></div>
<div id="fix">
The same 'not a guarantee' caveat is stated three times in the added text: lines 484-485 ('Detection is a heuristic, not a guarantee...'), 495-496 ('This is not a guarantee that every delivered image contains useful data'), and 500-501 ('Operators requiring all charts to contain data must not treat this policy as that guarantee'). Consolidate into one statement to keep the troubleshooting section scannable.
</div>
</div>
<small><i>Code Review Run #b4ab75</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -90,6 +96,45 @@ def test_stable_readiness_skips_when_budget_below_polling_margin() -> None:
page.wait_for_function.assert_not_called()
+def test_holder_diagnostics_include_offscreen_errors_for_full_capture() -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstring</b></div>
<div id="fix">
New test function lacks a docstring, which BITO.md adaptive rule 12148 requires for every newly added test function; sibling helpers in this file (e.g. `_report_context`, line 431) include one. Add a brief docstring stating the scenario: full-capture diagnostics must report `virtualized` for off-screen holders and `error` for alert holders.
</div>
</div>
<small><i>Code Review Run #b4ab75</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/utils/test_report_execution.py:
##########
@@ -136,6 +180,23 @@ def test_capture_rejection_is_sticky_for_the_execution() -> None:
)
+def test_artifact_approval_is_namespaced_by_kind() -> None:
+ context = ReportExecutionContext(
+ execution_id=UUID("084e7ee6-5557-4ecd-9632-b7f39c9ec524"),
+ report_schedule_id=7,
+ deadline=ReportExecutionDeadline(total_seconds=900),
+ )
+ artifact = b"same exact bytes"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing local type annotation</b></div>
<div id="fix">
Local `artifact` lacks an explicit type annotation. Repo rule [13153] requires explicit annotations for all local variables in test files, even when inferable; the surrounding test code is otherwise fully typed. ([BITO.md adaptive rule 13153])
</div>
</div>
<small><i>Code Review Run #b4ab75</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/utils/test_report_execution.py:
##########
@@ -44,6 +46,48 @@ def _report_config(**overrides: int) -> dict[str, int | bool]:
return config
+def test_chart_holder_diagnostics_separate_terminal_errors_from_success() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "rendered"},
+ {"chartId": "3", "state": "empty"},
+ {"chartId": "4", "state": "error"},
+ {"chartId": "5", "state": "virtualized"},
+ ]
+ )
+
+ assert diagnostics.mounted_holders == 5
+ assert diagnostics.ready_holders == 5
+ assert diagnostics.rendered_holders == 2
+ assert diagnostics.empty_holders == 1
+ assert diagnostics.error_holders == 1
+ assert diagnostics.virtualized_holders == 1
+ assert diagnostics.unready_holders == 0
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_count_unready_holders() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "waiting_on_database"},
+ {"chartId": "3", "state": "nothing_mounted"},
+ ]
+ )
+
+ assert diagnostics.ready_holders == 1
+ assert diagnostics.unready_holders == 2
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_do_not_treat_zero_holders_as_success() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states([])
+
+ assert diagnostics.mounted_holders == 0
+ assert diagnostics.semantic_success is False
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
These three new tests have no docstrings. Repo rule [12148] requires every newly added test function to include a docstring documenting purpose, scenario, and expected outcome; sibling tests in this file (e.g. `test_working_timeout_caps_report_budget`) already follow it. ([BITO.md adaptive rule 12148])
</div>
</div>
<small><i>Code Review Run #b4ab75</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4060343285
##########
superset/commands/report/execute.py:
##########
@@ -808,12 +810,16 @@ def _get_screenshots(self) -> list[bytes]:
imge = screenshot.get_screenshot(
user=user,
log_context=self._log_context,
- report_execution_context=self._report_execution_context,
+ report_execution_context=(
+ self._report_execution_context if for_delivery else None
+ ),
)
if imge is None:
raise ReportScheduleScreenshotFailedError(
"Screenshot failed; aborting to avoid sending a partial report"
)
+ if for_delivery:
+ self._validate_screenshot(imge)
Review Comment:
Could we carry the tiled capture's validation result through this gate too? The latest fix records screenshot approval only in `_get_validated_screenshot()`. `take_tiled_screenshot()` returns its combined image without that approval, so an accepted empty/error-only dashboard still fails here with `blank_final_image` when it uses tiling.
This also affects a dashboard that does contain validated chart content: extending `test_sparse_contentful_tile_is_not_diluted_by_empty_tiles` with `validate_report_screenshot(result, report_context)` reproduces the failure. Its content-bearing tile and the corresponding combined region both pass, but the surrounding empty tiles make the whole-image classifier reject it. Could we extend both that test and `test_blank_combined_image_is_allowed_for_terminal_empty_states` through final validation, preserving the tiled result while still checking the final bytes for corruption?
##########
superset/utils/webdriver.py:
##########
@@ -721,25 +755,50 @@ def _wait_for_charts_ready( # noqa: C901
)
return
chart_holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
- mounted_holders = len(chart_holder_states)
- ready_holders = sum(
- holder.get("state") in ready_states for holder in chart_holder_states
+ diagnostics = ChartHolderDiagnostics.from_holder_states(chart_holder_states)
Review Comment:
Could the final diagnostics use a scan that includes the actual states of off-screen holders for a standard report? `FIND_CHART_HOLDER_STATES_JS` returns `virtualized` before checking for an error whenever a holder is below the viewport. This path forces those holders to render and includes them in the full-page screenshot, so their errors are part of the delivered report.
With one rendered chart in view and an error panel below it, the actual JS scan produces `rendered` + `virtualized`; this logs `error_holders=0`, `semantic_success=True`, and no `report_semantic_status` warning. A full-dashboard diagnostic scan here would make the warning policy cover the same content as the readiness check and screenshot.
##########
superset/commands/report/execute.py:
##########
@@ -808,12 +810,16 @@ def _get_screenshots(self) -> list[bytes]:
imge = screenshot.get_screenshot(
user=user,
log_context=self._log_context,
- report_execution_context=self._report_execution_context,
+ report_execution_context=(
+ self._report_execution_context if for_delivery else None
Review Comment:
Could we preserve the execution deadline when opting bootstrap captures out of delivery validation? Passing `None` also changes `_wait_for_charts_ready()` from the report's remaining budget to `SCREENSHOT_LOAD_WAIT`, which defaults to 60 seconds. A scheduled CSV/XLSX/TEXT report with no saved query context can therefore fail on a chart taking 90 seconds to render, even with plenty of execution budget left.
I reproduced the timeout change with the actual readiness helper and a mocked browser: a 900-second context with the usual reserves allows 690 seconds; removing that context caps the same wait at 60. Keeping deadline propagation separate from the delivery-validation opt-out would avoid this regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4066846162
##########
tests/unit_tests/utils/test_report_execution.py:
##########
@@ -44,6 +46,48 @@ def _report_config(**overrides: int) -> dict[str, int | bool]:
return config
+def test_chart_holder_diagnostics_separate_terminal_errors_from_success() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "rendered"},
+ {"chartId": "3", "state": "empty"},
+ {"chartId": "4", "state": "error"},
+ {"chartId": "5", "state": "virtualized"},
+ ]
+ )
+
+ assert diagnostics.mounted_holders == 5
+ assert diagnostics.ready_holders == 5
+ assert diagnostics.rendered_holders == 2
+ assert diagnostics.empty_holders == 1
+ assert diagnostics.error_holders == 1
+ assert diagnostics.virtualized_holders == 1
+ assert diagnostics.unready_holders == 0
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_count_unready_holders() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "waiting_on_database"},
+ {"chartId": "3", "state": "nothing_mounted"},
+ ]
+ )
+
+ assert diagnostics.ready_holders == 1
+ assert diagnostics.unready_holders == 2
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_do_not_treat_zero_holders_as_success() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states([])
+
+ assert diagnostics.mounted_holders == 0
+ assert diagnostics.semantic_success is False
Review Comment:
Fixed in f4ef4cc50fd26c02e4963240c523e285019a4541. Added short scenario/outcome docstrings to the three holder-diagnostics tests.
##########
tests/unit_tests/utils/test_report_execution.py:
##########
@@ -136,6 +180,23 @@ def test_capture_rejection_is_sticky_for_the_execution() -> None:
)
+def test_artifact_approval_is_namespaced_by_kind() -> None:
+ context = ReportExecutionContext(
+ execution_id=UUID("084e7ee6-5557-4ecd-9632-b7f39c9ec524"),
+ report_schedule_id=7,
+ deadline=ReportExecutionDeadline(total_seconds=900),
+ )
+ artifact = b"same exact bytes"
Review Comment:
Leaving this unchanged. The bytes literal has an unambiguous inferred type and passes the repository's MyPy checks. An explicit local annotation would not improve correctness or readability here; keeping this PR focused rather than applying a blanket test-local annotation rewrite.
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -90,6 +96,45 @@ def test_stable_readiness_skips_when_budget_below_polling_margin() -> None:
page.wait_for_function.assert_not_called()
+def test_holder_diagnostics_include_offscreen_errors_for_full_capture() -> None:
Review Comment:
Fixed in f4ef4cc50fd26c02e4963240c523e285019a4541. Added a docstring describing the actual distinction: viewport scans mark off-screen holders virtualized; full-dashboard scans expose their error state. Full-capture diagnostics should not mark that off-screen error as virtualized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4061065758
##########
superset/commands/report/execute.py:
##########
@@ -808,12 +810,16 @@ def _get_screenshots(self) -> list[bytes]:
imge = screenshot.get_screenshot(
user=user,
log_context=self._log_context,
- report_execution_context=self._report_execution_context,
+ report_execution_context=(
+ self._report_execution_context if for_delivery else None
Review Comment:
Fixed in 289ec89ea800d8a97f92820a7b3b54e22264aa15. Bootstrap captures retain the shared execution deadline and reserves, with delivery validation disabled separately. Tests cover CSV/XLSX/TEXT bootstrap and verify that readiness retains the 690-second budget rather than falling back to 60 seconds.
##########
superset/utils/webdriver.py:
##########
@@ -721,25 +755,50 @@ def _wait_for_charts_ready( # noqa: C901
)
return
chart_holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
- mounted_holders = len(chart_holder_states)
- ready_holders = sum(
- holder.get("state") in ready_states for holder in chart_holder_states
+ diagnostics = ChartHolderDiagnostics.from_holder_states(chart_holder_states)
Review Comment:
Fixed in 289ec89ea800d8a97f92820a7b3b54e22264aa15. Standard report diagnostics scan every dashboard holder, including off-screen errors. Tiled diagnostics remain viewport-scoped. Tests execute both JavaScript scans and verify the standard path emits the terminal-error warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4061065133
##########
superset/commands/report/execute.py:
##########
@@ -808,12 +810,16 @@ def _get_screenshots(self) -> list[bytes]:
imge = screenshot.get_screenshot(
user=user,
log_context=self._log_context,
- report_execution_context=self._report_execution_context,
+ report_execution_context=(
+ self._report_execution_context if for_delivery else None
+ ),
)
if imge is None:
raise ReportScheduleScreenshotFailedError(
"Screenshot failed; aborting to avoid sending a partial report"
)
+ if for_delivery:
+ self._validate_screenshot(imge)
Review Comment:
Fixed in 289ec89ea800d8a97f92820a7b3b54e22264aa15. Tiled captures preserve their region-level validation through the final gate, while the combined bytes must still decode successfully before approval. Tests cover sparse content surrounded by empty tiles, empty/error-only dashboards, and corrupt combined output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4061669597
##########
superset/utils/webdriver.py:
##########
@@ -361,6 +372,11 @@ def _get_validated_screenshot(
context_suffix,
)
if not is_blank:
+ if blankness.is_blank:
+ report_execution_context.approve_artifact(
Review Comment:
Fixed in 43cde6c572670aadeafa21ae967a8a60c954dce2. Standard empty/error-state captures use the shared validator before recording approval, preserving the content-state exception without bypassing file-integrity checks. Regression tests corrupt an IDAT checksum for both chart and dashboard captures and verify rejection, no approval, and rejection again at the final gate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44465:
URL: https://github.com/apache/superset/pull/44465#issuecomment-5753606870
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | ae2cfdf6bd418460da64b01c78828c701493365a |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ab06f38e56cba00080f1295 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44465--superset-docs-preview.netlify.app](https://deploy-preview-44465--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoQwDADAv-SsGFcsrj_wGWmbYkFtCHFBF_9ub3OYP4iWXQxmgAa80hHW6pTtZu6Upai1gcRO5fZHW45kuRz1RpatXEus25FHl4aJRxc8IeKEqf98R3he) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44465--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44465:
URL: https://github.com/apache/superset/pull/44465#issuecomment-5759812756
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4060938523
##########
superset/utils/webdriver.py:
##########
@@ -361,6 +372,11 @@ def _get_validated_screenshot(
context_suffix,
)
if not is_blank:
+ if blankness.is_blank:
+ report_execution_context.approve_artifact(
Review Comment:
Could this call `validate_report_screenshot(image, report_execution_context, content_validated=True)` before recording approval, as the tiled path does? A terminal empty/error capture reaches this branch without running `Image.verify()`, and the cached hash then makes every later validator return early.
I reproduced this by flipping one byte of the IDAT checksum in an otherwise valid blank PNG. The pixel metrics still classify it as blank, but `Image.verify()` raises `SyntaxError: broken PNG file (bad header checksum in b'IDAT')`. The shared validator and tiled path reject those bytes; `_get_validated_screenshot()` followed by final validation accepts them. Using the shared validator here would preserve the empty/error exception while still checking the file's integrity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4059904867
##########
superset/utils/screenshot_utils.py:
##########
@@ -145,6 +148,31 @@ class ScreenshotBlankCaptureError(RuntimeError):
"""Raised when Chromium repeatedly returns a perceptually blank capture."""
+def validate_report_screenshot(
+ screenshot: bytes,
+ context: ReportExecutionContext,
+) -> None:
+ """Validate exact image bytes independently of browser or cache provenance."""
+ if context.capture_was_rejected:
+ raise ScreenshotBlankCaptureError("Capture was already rejected")
+ if context.artifact_was_validated(screenshot):
+ return
+ try:
+ with Image.open(io.BytesIO(screenshot)) as image:
+ image.verify()
+ blankness = get_screenshot_blankness_metrics(screenshot)
Review Comment:
Could we make pixel-decoding failures propagate here? Image.verify() checks the PNG structure and CRCs, but it does not decode the IDAT stream. get_screenshot_blankness_metrics() then catches a decoding OSError and returns is_blank=False, so these bytes are approved below. I reproduced this with a PNG whose IDAT payload was corrupted and whose CRC was recomputed: verify() passed, convert("RGB").load() raised OSError: broken data stream, and validate_report_screenshot() still called approve_artifact(). For this final delivery gate, a decode failure should reject the artifact as invalid_image, for example by letting the helper surface decode errors to this caller.
##########
superset/utils/report_execution.py:
##########
@@ -245,6 +257,14 @@ def reject_capture(self, reason: str) -> None:
self._capture_rejection_reasons.append(reason)
+ def approve_artifact(self, artifact: bytes) -> None:
+ """Record exact bytes validated by the capture or PDF assembly stage."""
+ self._validated_artifacts.add(hashlib.sha256(artifact).hexdigest())
Review Comment:
Could we namespace approvals by artifact type? _get_pdf() validates the source screenshots first, so their hashes are already in this same set. If notification_content.pdf is later replaced with one of those PNG byte strings, the PDF gate sees artifact_was_validated(...) == True and accepts bytes that were never produced by PDF assembly. Storing (kind, digest) or separate screenshot/PDF sets would preserve the exact-PDF provenance this check is meant to enforce.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4066845175
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error is
+not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
Review Comment:
Fixed in f4ef4cc50fd26c02e4963240c523e285019a4541. The docs explicitly say recognized empty/error states satisfy the readiness wait and do not themselves trigger capture retries or block delivery. Error-state delivery is distinguished from successful chart data.
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working. Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error is
+not successful chart data. Logs distinguish `rendered_holders`, `empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
+
+This is not a guarantee that every delivered image contains useful data. A large
+enough title or logo can still exceed the detector's content thresholds while the
+rest of the page is incomplete; removing the sparse-content override does not
+solve that general limitation. Recognized terminal chart errors are capture-ready
+and may be delivered with a warning, provided the
+image passes visual validation. Operators requiring all charts to contain data
+must not treat this policy as that guarantee. The strict invariant is that an
Review Comment:
Fixed in f4ef4cc50fd26c02e4963240c523e285019a4541. Consolidated the heuristic limitation into one paragraph, preserving the fail-closed invariant and explaining that a sufficiently large title/logo can exceed the content threshold without a special exemption.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4066847724
##########
tests/unit_tests/utils/test_report_execution.py:
##########
@@ -136,6 +180,23 @@ def test_capture_rejection_is_sticky_for_the_execution() -> None:
)
+def test_artifact_approval_is_namespaced_by_kind() -> None:
+ context = ReportExecutionContext(
+ execution_id=UUID("084e7ee6-5557-4ecd-9632-b7f39c9ec524"),
+ report_schedule_id=7,
+ deadline=ReportExecutionDeadline(total_seconds=900),
+ )
+ artifact = b"same exact bytes"
Review Comment:
<!-- Bito Reply -->
The decision to omit the explicit type annotation is reasonable. Since the bytes literal has an unambiguous inferred type and the code passes existing repository checks, adding an explicit annotation would not provide additional clarity or correctness in this specific test context.
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -90,6 +96,45 @@ def test_stable_readiness_skips_when_budget_below_polling_margin() -> None:
page.wait_for_function.assert_not_called()
+def test_holder_diagnostics_include_offscreen_errors_for_full_capture() -> None:
Review Comment:
<!-- Bito Reply -->
The suggestion to add a docstring to the new test function is appropriate. It improves code maintainability by documenting the specific diagnostic scenario being tested, as required by the project's documentation standards.
**tests/unit_tests/utils/test_screenshot_utils.py**
```
def test_holder_diagnostics_include_offscreen_errors_for_full_capture() -> None:
"""Verifies that full-capture diagnostics report off-screen errors correctly."""
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4059904867
##########
superset/utils/screenshot_utils.py:
##########
@@ -145,6 +148,31 @@ class ScreenshotBlankCaptureError(RuntimeError):
"""Raised when Chromium repeatedly returns a perceptually blank capture."""
+def validate_report_screenshot(
+ screenshot: bytes,
+ context: ReportExecutionContext,
+) -> None:
+ """Validate exact image bytes independently of browser or cache provenance."""
+ if context.capture_was_rejected:
+ raise ScreenshotBlankCaptureError("Capture was already rejected")
+ if context.artifact_was_validated(screenshot):
+ return
+ try:
+ with Image.open(io.BytesIO(screenshot)) as image:
+ image.verify()
+ blankness = get_screenshot_blankness_metrics(screenshot)
Review Comment:
Could we make pixel-decoding failures propagate here? checks the PNG structure and CRCs, but it does not decode the IDAT stream. then catches a decoding and returns , so these bytes are approved below. I reproduced this with a PNG whose IDAT payload was corrupted and whose CRC was recomputed: passed, raised , and still called . For this final delivery gate, a decode failure should reject the artifact as (for example, by letting the helper surface decode errors to this caller).
##########
superset/utils/report_execution.py:
##########
@@ -245,6 +257,14 @@ def reject_capture(self, reason: str) -> None:
self._capture_rejection_reasons.append(reason)
+ def approve_artifact(self, artifact: bytes) -> None:
+ """Record exact bytes validated by the capture or PDF assembly stage."""
+ self._validated_artifacts.add(hashlib.sha256(artifact).hexdigest())
Review Comment:
Could we namespace approvals by artifact type? validates the source screenshots first, so their hashes are already in this same set. If is later replaced with one of those PNG byte strings, the PDF gate sees and accepts bytes that were never produced by PDF assembly. Storing or separate screenshot/PDF sets would preserve the exact-PDF provenance this check is meant to enforce.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(reports): validate final capture artifacts before delivery [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44465:
URL: https://github.com/apache/superset/pull/44465#issuecomment-5753597726
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org