feat(assets): acquire still images and capture webpage evidence - #172
DonIsmaelito wants to merge 4 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_asset_delivery.py">
<violation number="1" location="tests/test_asset_delivery.py:1">
P3: The emoji acquisition path is the only `@staged_asset` helper without a delivery test. This file covers `fetch_image` and `fetch_logo`, but `fetch_emoji` — including its staged publish, sidecar enrichment (`sha256`/`rights_verified`), and preservation of existing outputs — is never exercised anywhere; `tests/test_fetch_asset.py` only tests `render_emoji` directly and `tests/test_web_shot.py` does not touch it. A regression in the emoji sidecar or publish flow would pass the whole suite. Add a test analogous to `test_logo_svg_provenance` that mocks `render_emoji` (or writes a tiny PNG via the real renderer when a font exists) and asserts the published artifact, `sha256`, and `rights_verified is False`.</violation>
</file>
<file name="tests/test_fetch_asset.py">
<violation number="1" location="tests/test_fetch_asset.py:36">
P2: These tests do not verify the stated “before network” behavior and can issue live requests during the suite. Monkeypatch `fetch_asset.download` with a failing spy and assert that invalid logo slugs/colors and non-HTTP image URLs reject without invoking it.</violation>
</file>
<file name="helpers/_asset_io.py">
<violation number="1" location="helpers/_asset_io.py:70">
P2: A slow streaming server can hold acquisition open indefinitely because the 60-second Requests timeout is a per-read inactivity timeout, not an overall deadline. Enforce an absolute download deadline while iterating the response.</violation>
</file>
<file name="helpers/fetch_asset.py">
<violation number="1" location="helpers/fetch_asset.py:61">
P3: `sha256_of` duplicates the chunked SHA-256 implementation already provided by `_asset_io.file_hash`. Import and reuse `file_hash` so hash behavior has one implementation.</violation>
<violation number="2" location="helpers/fetch_asset.py:84">
P2: DNS, timeout, and TLS failures from `download()` bypass `main()`'s exception handler and print a traceback. Catch `requests.RequestException` and convert it to the helper's normal CLI error.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
|
||
|
|
||
| # bad slugs and colors are rejected before any request | ||
| def test_logo_rejects_bad_slug_before_network(tmp_path: Path) -> None: |
There was a problem hiding this comment.
P2: These tests do not verify the stated “before network” behavior and can issue live requests during the suite. Monkeypatch fetch_asset.download with a failing spy and assert that invalid logo slugs/colors and non-HTTP image URLs reject without invoking it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_fetch_asset.py, line 36:
<comment>These tests do not verify the stated “before network” behavior and can issue live requests during the suite. Monkeypatch `fetch_asset.download` with a failing spy and assert that invalid logo slugs/colors and non-HTTP image URLs reject without invoking it.</comment>
<file context>
@@ -0,0 +1,49 @@
+
+
+# bad slugs and colors are rejected before any request
+def test_logo_rejects_bad_slug_before_network(tmp_path: Path) -> None:
+ args = argparse.Namespace(slug="not a slug!", output=tmp_path / "x.png", color="ffffff", size=100)
+ with pytest.raises(SystemExit):
</file context>
| def download(url, *, headers, max_bytes): | ||
| import requests | ||
|
|
||
| with requests.get(url, headers=headers, timeout=60, stream=True) as response: |
There was a problem hiding this comment.
P2: A slow streaming server can hold acquisition open indefinitely because the 60-second Requests timeout is a per-read inactivity timeout, not an overall deadline. Enforce an absolute download deadline while iterating the response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/_asset_io.py, line 70:
<comment>A slow streaming server can hold acquisition open indefinitely because the 60-second Requests timeout is a per-read inactivity timeout, not an overall deadline. Enforce an absolute download deadline while iterating the response.</comment>
<file context>
@@ -0,0 +1,81 @@
+def download(url, *, headers, max_bytes):
+ import requests
+
+ with requests.get(url, headers=headers, timeout=60, stream=True) as response:
+ if response.status_code != 200:
+ raise ValueError(f"download failed with HTTP {response.status_code}")
</file context>
| @@ -0,0 +1,268 @@ | |||
| """Verify bounded acquisition provenance and preservation of existing assets.""" | |||
There was a problem hiding this comment.
P3: The emoji acquisition path is the only @staged_asset helper without a delivery test. This file covers fetch_image and fetch_logo, but fetch_emoji — including its staged publish, sidecar enrichment (sha256/rights_verified), and preservation of existing outputs — is never exercised anywhere; tests/test_fetch_asset.py only tests render_emoji directly and tests/test_web_shot.py does not touch it. A regression in the emoji sidecar or publish flow would pass the whole suite. Add a test analogous to test_logo_svg_provenance that mocks render_emoji (or writes a tiny PNG via the real renderer when a font exists) and asserts the published artifact, sha256, and rights_verified is False.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_asset_delivery.py, line 1:
<comment>The emoji acquisition path is the only `@staged_asset` helper without a delivery test. This file covers `fetch_image` and `fetch_logo`, but `fetch_emoji` — including its staged publish, sidecar enrichment (`sha256`/`rights_verified`), and preservation of existing outputs — is never exercised anywhere; `tests/test_fetch_asset.py` only tests `render_emoji` directly and `tests/test_web_shot.py` does not touch it. A regression in the emoji sidecar or publish flow would pass the whole suite. Add a test analogous to `test_logo_svg_provenance` that mocks `render_emoji` (or writes a tiny PNG via the real renderer when a font exists) and asserts the published artifact, `sha256`, and `rights_verified is False`.</comment>
<file context>
@@ -0,0 +1,268 @@
+"""Verify bounded acquisition provenance and preservation of existing assets."""
+
+import io
</file context>
|
|
||
|
|
||
| # hash a file in chunks and return the hex digest | ||
| def sha256_of(path: Path) -> str: |
There was a problem hiding this comment.
P3: sha256_of duplicates the chunked SHA-256 implementation already provided by _asset_io.file_hash. Import and reuse file_hash so hash behavior has one implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/fetch_asset.py, line 61:
<comment>`sha256_of` duplicates the chunked SHA-256 implementation already provided by `_asset_io.file_hash`. Import and reuse `file_hash` so hash behavior has one implementation.</comment>
<file context>
@@ -0,0 +1,345 @@
+
+
+# hash a file in chunks and return the hex digest
+def sha256_of(path: Path) -> str:
+ digest = hashlib.sha256()
+ with path.open("rb") as handle:
</file context>
|
@cubic-dev-ai please review the latest commits again. Preserve concurrent asset publications, bound image and capture dimensions, trim using alpha, normalize transport errors, and report the final published path. Validation: 47 branch tests passed. The combined core preview passed 765 tests with two optional skips. Existing threads remain open for rechecking; this follow-up does not claim every previous finding is resolved. |
@DonIsmaelito I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
8 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="helpers/fetch_asset.py">
<violation number="1" location="helpers/fetch_asset.py:112">
P3: Any exception inside the `Image.open` block is reported as "downloaded data is not a decodable image", including the intentional `ValueError`s raised for oversized images (>80M pixels) and animated images. A structurally valid 85-million-pixel photo therefore prints a message claiming the download is not decodable, which points the editor at the wrong failure mode. Re-raise the ValueError so the specific messages reach `main()`; keep the catch-all only for genuine decode errors.</violation>
<violation number="2" location="helpers/fetch_asset.py:161">
P2: When CairoSVG is unavailable and the output basename contains `#` or `?`, the fallback HTML interprets the raw filename as a fragment or query and cannot load the SVG. Use the file URI for `svg_path` or percent-encode the filename.</violation>
<violation number="3" location="helpers/fetch_asset.py:203">
P2: When the CDN returns malformed or non-XML data, `ElementTree.fromstring` raises `ParseError`, which `main` does not catch, so the CLI prints a traceback instead of a normalized asset error. Catch the parse error and re-raise `ValueError` with the existing invalid-SVG message.</violation>
<violation number="4" location="helpers/fetch_asset.py:207">
P2: When the output is named `logo.SVG`, validation accepts it but publication expects `logo.SVG` while this line creates `logo.svg`, so the command fails without publishing the logo. Keep the original path for SVG outputs.</violation>
</file>
<file name="helpers/web_shot.py">
<violation number="1" location="helpers/web_shot.py:131">
P2: Failures in the Playwright path are not normalized: `page.goto` (a 60 s `networkidle` timeout on polling/HMR pages), a hidden `--selector` element, or a launch error raise `playwright.sync_api.Error`/`TimeoutError`, which `main()` does not catch (`except (ValueError, FileExistsError)`), so users get a raw traceback instead of the clean one-line `SystemExit` the Chrome path produces. Wrap `capture_playwright` calls and convert failures to a message plus exit code, and use `wait_until="load"` (or `domcontentloaded`) with the existing `--wait` so a busy page doesn't time out after 60 s.</violation>
<violation number="2" location="helpers/web_shot.py:179">
P2: Chrome's stderr is discarded (`stderr=subprocess.DEVNULL`) but the failure message appends `(stderr or '')[-800:]`, so that tail is always empty. When Chrome produces no screenshot (bad URL, DNS failure, watchdog kill), the user only sees `browser produced no screenshot for <url>\n` with no reason, which contradicts the PR's "normalize transport errors" goal and defeats debugging. Route stderr to a pipe or a temp file (so a large log cannot block the polling loop) and include its tail in the `SystemExit` message at line 210.</violation>
<violation number="3" location="helpers/web_shot.py:373">
P1: When `card` receives a large local image, `opened.convert("RGBA")` decodes it before `--crop` or `--max-*` can reduce it. Reject sources above the project’s 80-million-pixel budget before conversion to prevent a large or crafted input from exhausting memory.</violation>
</file>
<file name="tests/test_web_shot.py">
<violation number="1" location="tests/test_web_shot.py:14">
P3: `size` is a parameter of `_source`, but the dark rectangle coordinates (range(40,360), range(40,160)) are hardcoded for the default 400x200 size. A caller passing a custom size gets an off-center rectangle whose trim/size expectations no longer hold, so the parameter silently promises something the helper does not deliver. Either derive the rectangle from `size` or drop the parameter and keep the fixed 400x200 source. The mutable tuple default `size=(400, 200)` is also a lint flag here.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if not math.isfinite(rotate): | ||
| raise ValueError("rotation must be finite") | ||
| with Image.open(source) as opened: | ||
| image = opened.convert("RGBA") |
There was a problem hiding this comment.
P1: When card receives a large local image, opened.convert("RGBA") decodes it before --crop or --max-* can reduce it. Reject sources above the project’s 80-million-pixel budget before conversion to prevent a large or crafted input from exhausting memory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_shot.py, line 373:
<comment>When `card` receives a large local image, `opened.convert("RGBA")` decodes it before `--crop` or `--max-*` can reduce it. Reject sources above the project’s 80-million-pixel budget before conversion to prevent a large or crafted input from exhausting memory.</comment>
<file context>
@@ -0,0 +1,561 @@
+ if not math.isfinite(rotate):
+ raise ValueError("rotation must be finite")
+ with Image.open(source) as opened:
+ image = opened.convert("RGBA")
+ box = parse_crop(crop, image.width, image.height)
+ if box:
</file context>
| image = opened.convert("RGBA") | |
| if opened.width * opened.height > 80_000_000: | |
| raise ValueError("image exceeds 80 million pixels") | |
| image = opened.convert("RGBA") |
| html = svg_path.with_suffix(".html") | ||
| html.write_text( | ||
| f'<html><body style="margin:0;background:transparent">' | ||
| f'<img src="{svg_path.name}" style="width:{size}px;height:{size}px;display:block"></body></html>', |
There was a problem hiding this comment.
P2: When CairoSVG is unavailable and the output basename contains # or ?, the fallback HTML interprets the raw filename as a fragment or query and cannot load the SVG. Use the file URI for svg_path or percent-encode the filename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/fetch_asset.py, line 161:
<comment>When CairoSVG is unavailable and the output basename contains `#` or `?`, the fallback HTML interprets the raw filename as a fragment or query and cannot load the SVG. Use the file URI for `svg_path` or percent-encode the filename.</comment>
<file context>
@@ -0,0 +1,348 @@
+ html = svg_path.with_suffix(".html")
+ html.write_text(
+ f'<html><body style="margin:0;background:transparent">'
+ f'<img src="{svg_path.name}" style="width:{size}px;height:{size}px;display:block"></body></html>',
+ encoding="utf-8",
+ )
</file context>
| ) | ||
| from xml.etree import ElementTree | ||
|
|
||
| root = ElementTree.fromstring(data) |
There was a problem hiding this comment.
P2: When the CDN returns malformed or non-XML data, ElementTree.fromstring raises ParseError, which main does not catch, so the CLI prints a traceback instead of a normalized asset error. Catch the parse error and re-raise ValueError with the existing invalid-SVG message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/fetch_asset.py, line 203:
<comment>When the CDN returns malformed or non-XML data, `ElementTree.fromstring` raises `ParseError`, which `main` does not catch, so the CLI prints a traceback instead of a normalized asset error. Catch the parse error and re-raise `ValueError` with the existing invalid-SVG message.</comment>
<file context>
@@ -0,0 +1,348 @@
+ )
+ from xml.etree import ElementTree
+
+ root = ElementTree.fromstring(data)
+ if root.tag.split("}")[-1] != "svg":
+ raise ValueError("logo response is not an SVG")
</file context>
| root = ElementTree.fromstring(data) | |
| try: | |
| root = ElementTree.fromstring(data) | |
| except (ElementTree.ParseError, UnicodeDecodeError) as exc: | |
| raise ValueError("logo response is not an SVG") from exc |
| if root.tag.split("}")[-1] != "svg": | ||
| raise ValueError("logo response is not an SVG") | ||
| out.parent.mkdir(parents=True, exist_ok=True) | ||
| svg_path = out.with_suffix(".svg") |
There was a problem hiding this comment.
P2: When the output is named logo.SVG, validation accepts it but publication expects logo.SVG while this line creates logo.svg, so the command fails without publishing the logo. Keep the original path for SVG outputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/fetch_asset.py, line 207:
<comment>When the output is named `logo.SVG`, validation accepts it but publication expects `logo.SVG` while this line creates `logo.svg`, so the command fails without publishing the logo. Keep the original path for SVG outputs.</comment>
<file context>
@@ -0,0 +1,348 @@
+ if root.tag.split("}")[-1] != "svg":
+ raise ValueError("logo response is not an SVG")
+ out.parent.mkdir(parents=True, exist_ok=True)
+ svg_path = out.with_suffix(".svg")
+ svg_path.write_bytes(data)
+ tool = "svg"
</file context>
| svg_path = out.with_suffix(".svg") | |
| svg_path = out if out.suffix.lower() == ".svg" else out.with_suffix(".svg") |
| user_agent=user_agent, | ||
| ) | ||
| page = context.new_page() | ||
| page.goto(url, wait_until="networkidle", timeout=60_000) |
There was a problem hiding this comment.
P2: Failures in the Playwright path are not normalized: page.goto (a 60 s networkidle timeout on polling/HMR pages), a hidden --selector element, or a launch error raise playwright.sync_api.Error/TimeoutError, which main() does not catch (except (ValueError, FileExistsError)), so users get a raw traceback instead of the clean one-line SystemExit the Chrome path produces. Wrap capture_playwright calls and convert failures to a message plus exit code, and use wait_until="load" (or domcontentloaded) with the existing --wait so a busy page doesn't time out after 60 s.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_shot.py, line 131:
<comment>Failures in the Playwright path are not normalized: `page.goto` (a 60 s `networkidle` timeout on polling/HMR pages), a hidden `--selector` element, or a launch error raise `playwright.sync_api.Error`/`TimeoutError`, which `main()` does not catch (`except (ValueError, FileExistsError)`), so users get a raw traceback instead of the clean one-line `SystemExit` the Chrome path produces. Wrap `capture_playwright` calls and convert failures to a message plus exit code, and use `wait_until="load"` (or `domcontentloaded`) with the existing `--wait` so a busy page doesn't time out after 60 s.</comment>
<file context>
@@ -0,0 +1,561 @@
+ user_agent=user_agent,
+ )
+ page = context.new_page()
+ page.goto(url, wait_until="networkidle", timeout=60_000)
+ if wait:
+ page.wait_for_timeout(int(wait * 1000))
</file context>
| url, | ||
| ] | ||
| proc = subprocess.Popen( | ||
| command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, text=True |
There was a problem hiding this comment.
P2: Chrome's stderr is discarded (stderr=subprocess.DEVNULL) but the failure message appends (stderr or '')[-800:], so that tail is always empty. When Chrome produces no screenshot (bad URL, DNS failure, watchdog kill), the user only sees browser produced no screenshot for <url>\n with no reason, which contradicts the PR's "normalize transport errors" goal and defeats debugging. Route stderr to a pipe or a temp file (so a large log cannot block the polling loop) and include its tail in the SystemExit message at line 210.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_shot.py, line 179:
<comment>Chrome's stderr is discarded (`stderr=subprocess.DEVNULL`) but the failure message appends `(stderr or '')[-800:]`, so that tail is always empty. When Chrome produces no screenshot (bad URL, DNS failure, watchdog kill), the user only sees `browser produced no screenshot for <url>\n` with no reason, which contradicts the PR's "normalize transport errors" goal and defeats debugging. Route stderr to a pipe or a temp file (so a large log cannot block the polling loop) and include its tail in the `SystemExit` message at line 210.</comment>
<file context>
@@ -0,0 +1,561 @@
+ url,
+ ]
+ proc = subprocess.Popen(
+ command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, text=True
+ )
+ deadline = time.time() + timeout
</file context>
| ) | ||
| converted.save(out) | ||
| width, height = converted.size | ||
| except Exception as exc: |
There was a problem hiding this comment.
P3: Any exception inside the Image.open block is reported as "downloaded data is not a decodable image", including the intentional ValueErrors raised for oversized images (>80M pixels) and animated images. A structurally valid 85-million-pixel photo therefore prints a message claiming the download is not decodable, which points the editor at the wrong failure mode. Re-raise the ValueError so the specific messages reach main(); keep the catch-all only for genuine decode errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/fetch_asset.py, line 112:
<comment>Any exception inside the `Image.open` block is reported as "downloaded data is not a decodable image", including the intentional `ValueError`s raised for oversized images (>80M pixels) and animated images. A structurally valid 85-million-pixel photo therefore prints a message claiming the download is not decodable, which points the editor at the wrong failure mode. Re-raise the ValueError so the specific messages reach `main()`; keep the catch-all only for genuine decode errors.</comment>
<file context>
@@ -0,0 +1,348 @@
+ )
+ converted.save(out)
+ width, height = converted.size
+ except Exception as exc:
+ tmp.unlink(missing_ok=True)
+ raise SystemExit(
</file context>
|
|
||
|
|
||
| # save a white image with a dark rectangle in the middle | ||
| def _source(tmp_path: Path, size=(400, 200)) -> Path: |
There was a problem hiding this comment.
P3: size is a parameter of _source, but the dark rectangle coordinates (range(40,360), range(40,160)) are hardcoded for the default 400x200 size. A caller passing a custom size gets an off-center rectangle whose trim/size expectations no longer hold, so the parameter silently promises something the helper does not deliver. Either derive the rectangle from size or drop the parameter and keep the fixed 400x200 source. The mutable tuple default size=(400, 200) is also a lint flag here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_web_shot.py, line 14:
<comment>`size` is a parameter of `_source`, but the dark rectangle coordinates (range(40,360), range(40,160)) are hardcoded for the default 400x200 size. A caller passing a custom size gets an off-center rectangle whose trim/size expectations no longer hold, so the parameter silently promises something the helper does not deliver. Either derive the rectangle from `size` or drop the parameter and keep the fixed 400x200 source. The mutable tuple default `size=(400, 200)` is also a lint flag here.</comment>
<file context>
@@ -0,0 +1,72 @@
+
+
+# save a white image with a dark rectangle in the middle
+def _source(tmp_path: Path, size=(400, 200)) -> Path:
+ image = Image.new("RGB", size, (255, 255, 255))
+ for x in range(40, 360):
</file context>
1 participant
Why
Edits often need a webpage screenshot, image or logo. These helpers prepare those still assets and retain where they came from so the editor can review them before use.
Changes
Capture webpages and turn images into cropped, rounded, shadowed or rotated transparent cards.
Download still images and logo SVGs, and render emoji using an installed font.
Record source metadata and content hashes, bound download sizes, and protect existing assets and sidecars.
Feature commits and review fixes cover capture, acquisition and documentation. All 47 branch tests pass, including 29 Assets cases. A real Chrome capture and treated card from an original local page were visually inspected.
Review follow-up: Preserve concurrent asset publications, bound image and capture dimensions, trim using alpha, normalize transport errors, and report the final published path.
Limits
No third-party images, logos, emoji artwork or fonts are bundled. Rights metadata remains explicitly unverified; acquiring an asset does not establish permission to reuse it.
Browser capture, SVG rasterization and emoji rendering require optional local tools. Selector and full-page capture require Playwright. Live external downloads and Playwright capture were not tested; download tests use mocked responses. Multi-file publication is not atomic.
This targets main independently. It complements web footage sourcing in #148 and source tracking in #164; still-image treatment overlaps the purpose of #169 without modifying its timeline effects or renderer.