Skip to content

Foundation: image-file contract, checker, scope rule and notebook image policy (#307) - #322

Merged
CompleteDotTech merged 4 commits into
mainfrom
foundation/image-assets-contract
Oct 21, 2026
Merged

CompleteDotTech merged 4 commits into
mainfrom
foundation/image-assets-contract

Conversation

@CompleteDotTech

@CompleteDotTech CompleteDotTech commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Closes #307

What this adds

The image-file contract for multimodal recipes, a checker for it, and the fixes to the scope, freshness and attribute handling of binary files. Contract in docs/fixtures.md (Image fixtures); notebook rules in docs/notebook-ci.md (staleness check rules 2-5).

  • tools/check_recipe_assets.py (stdlib only), second step of the Hygiene (secrets and notebook outputs) job and a pre-commit hook: location (recipes/<slug>/assets/ only), PNG/baseline JPEG with lowercase suffix and matching bytes, 280-1568 px / 78,400-2,458,624 area, 1 MiB per file, 8 MiB per folder, no metadata, no symlinks. Parsing is allowlist-based for both formats (see fix round 1).
  • tools/check_recipe_scope.py: an image suffix added/modified/renamed anywhere but recipes/<slug>/assets/ is rejected (deletion allowed).
  • tools/check_notebook_fresh.py: image/jpeg is normalised like image/png (Pillow displays every image as both); output cap 2 MiB; embedded images are metadata-checked in both files.
  • .gitattributes *.jpeg binary; docs/recipe-template.md assets/ row; CONTRIBUTING.md, docs/development.md, .pre-commit-config.yaml updated.

Fix round 1 (both Opus reviews on 367a1e3 were CHANGES-NEEDED)

Review 1:

  • B1 metadata smuggling: JPEG is now a marker allowlist (SOI, one exactly-16-byte JFIF APP0 with no thumbnail, whole DQT/DHT tables only, DRI of length 4, one SOF0 8-bit with 1 or 3 components, SOS with consistent length, RSTn, EOI); everything else is jpeg-metadata or jpeg-structure. PNG: exact length for gAMA/cHRM/sRGB/pHYs/sBIT/bKGD/tRNS, PLTE only in palette images, IHDR validated, IDAT consecutive and one zlib stream with nothing after it. One negative test per carrier from the review (text after JFIF, second APP0, thumbnail, DNL, JPG0, 0xFF02, DRI, PLTE in RGB, pHYs/bKGD with text, IDAT trailing text, ...).
  • S5 CRC test now corrupts byte 20 and asserts bad CRC in chunk IHDR (mutation of the CRC check is killed; I checked).
  • B2 notebook outputs: every committed image/png and image/jpeg output (and every data:image URI in an output) is checked with the asset rules in both files; matplotlib's short tEXt Software is the only text allowed. Test: EXIF JPEG committed vs clean fresh JPEG now fails. Markdown/raw cells may not embed an image (attachment or data URI); image/gif etc. outputs are problems. All notebooks on main still pass (tested).
  • B3 .pre-commit-config.yaml hook, docs/development.md Hygiene row and section, CONTRIBUTING.md assets/ entry; each pinned by a test.
  • B4 sniff now also detects BMP, ICO, HEIF/AVIF, JPEG 2000 and SVG by content; docs list exactly the detected set and state that base64/data: images inside text files are not detected.
  • S2 documented: 8/16-bit and interlaced PNGs accepted; JPEG precision 8, 1 or 3 components enforced. S3 noted the cap runs in the Notebook job. S1 test pins the checker limits to the documented 280-1568 and, if jev_cookbook.vision.IMAGE_LIMITS exists (PR Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321), to it; vision.py untouched.

Review 2:

Verification (fresh venv, Windows, after merging origin/main 7fa806e with a signed merge commit)

  • ruff check . pass; ruff format --check . 397 files formatted.
  • pytest: the full run before the last test-only fix had 4 failures: the 2 known Windows SIGKILL ones (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302), test_a_scripted_scaffold_runs_from_the_scaffold_to_a_byte_identical_notebook (kernel start timeout under load; passes on rerun), and test_style_notebook_display.py::test_apply_style_does_nothing_outside_ipython (caused by a new test of mine creating a global IPython shell; fixed to use DisplayFormatter(); rerun of the three affected files: 143 passed). The final head was not re-run in full; it differs from the full run only by that test fix.
  • render_catalog.py --check exit 0; check_hygiene.py 606 files pass; check_recipe_assets.py 461 files pass; notebook_ci.py fixtures 51 folders, 0 failed.

Two independent Opus reviews requested by the issue; this is a new head, so earlier approvals do not apply.

🤖 Generated with Claude Code

Fix round 2 (head 171240f; re-review 1 CHANGES-NEEDED, re-review 2 APPROVE on 3456211)

  • R1 (blocking): the text-cell rule now matches data:\s*image/ case-insensitively with any parameters, base64 or not; cell metadata and notebook metadata (values, nested values and keys such as image/jpeg) are scanned too; the output data-URI regex is case-insensitive and accepts parameters. One test per spelling (upper-case scheme, mixed-case type, one and two parameters, whitespace, non-base64 SVG, markdown syntax; markdown and raw cells) and per metadata placement.
  • S7 (cheap PNG hardening): PNG chunks the spec allows once (gAMA, cHRM, sRGB, sBIT, pHYs, bKGD, tRNS) are rejected when repeated; the inflated IDAT length must equal the length implied by width, height, depth, colour type and interlace (including the Adam7 passes; the test computes the expected size independently). Tests include text appended inside the zlib stream.
  • S8: an embedded PNG output may hold at most one tEXt chunk, and it must match matplotlib's own wording (Software\0Matplotlib version<N>, https://matplotlib.org/).
  • S6: tests for a second exact JFIF APP0, a late APP0, and the marker allowlist (unknown markers with a valid table body are refused with the allowlist message). I confirmed by mutation that removing either guard now fails a test.
  • Docs: docs/fixtures.md says the rules remove metadata ordinary tools write and are not a detector for deliberately hidden bytes; the output-cap failure message recommends IPython.display.Image(filename=path).
  • Not added, per the scope decision: table-content and pre-EOI-data rules.
  • Verification on this head (origin/main had not moved; no merge needed): ruff check and format clean; render_catalog --check 0; hygiene 606 files; check_recipe_assets 461 files; fixtures 51/0; full pytest: 2 failed, 5284 passed, 58 skipped, 50 deselected, 31 xfailed. The 2 failures are the known Windows SIGKILL tests (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302).

…metadata) with a checker, scope rule, JPEG output normalisation and output cap (#307)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2): Claude Opus 5.5 (claude-opus-5-5).

Reviewed head 367a1e3daba1ef1bca6482acfe9239ff8c46c3d9 against base 9a7f152202f13a1b1e4fc004d978390fc7c3232f (current main). I did not read the other review before posting this one.

Summary. The design is right: a stdlib checker, a PNG chunk allowlist, JFIF-only JPEGs, path: [rule] message output, a suffix rule in Scope, and normalising JPEG outputs with a cap. The tests cover every named rule, and my mutation runs (below) show they catch it when a rule is removed. I tried to get bad assets past the checks and four things got through:

  1. The JPEG marker check is really a denylist, and the allowed PNG chunks have no length limit. Arbitrary text gets through in files that Pillow and vision.py load without complaint.
  2. Normalising image/jpeg opens a new hole. A committed JPEG output can now carry EXIF that main rejects today. Images in Markdown attachments and data: URIs fall outside both the location rule and the output cap.
  3. This PR makes docs/development.md and .pre-commit-config.yaml inaccurate, and CONTRIBUTING.md (the recipe contract) never mentions assets/.
  4. docs/fixtures.md says files are caught "under any name" by their image signature. The checker only knows five signatures.

All four are cheap to fix. Because 40 recipes copy this contract, I am asking for the fixes in this PR.

Blocking findings (most severe first)

B1. Metadata gets past the "no metadata" rule inside segments and chunks the checker allows (tools/check_recipe_assets.py:162-215, :131-153, :61-63)

Each row below is a file I built in memory. The text payload in every case was Author: Timothy Gregg; C:\Users\timot\Desktop\patient_scan.jpg; GPS 51.5,-0.12; sk-ant-XXXX. Each row passed check_entries, Pillow's load() and jev_cookbook.vision._image_format (the main loader):

Carrier Checker Pillow / vision.py
JFIF APP0 with text after its 14-byte header (body.startswith(b"JFIF\x00") is the only test, :197-199) passes load / accept
A second APP0 segment JFIF\0 + text passes load / accept
A DNL (0xFFDC) segment with a text body before the frame passes load / accept
PNG PLTE (a "suggested palette") in a truecolour PNG, filled with text bytes passes load / accept
PNG pHYs / bKGD with text after their fixed fields passes load / accept
PNG IDAT with text after the end of the zlib stream passes load / accept
JPG0 (0xFFF0), reserved 0xFF02, and DRI with an oversized body passes libjpeg cannot decode (the checker calls a file valid that is not even a JPEG it can load)

Only APPn and COM are refused for JPEG; every other marker with a length field is skipped. A JFIF APP0 can also legally hold a thumbnail of the original, uncropped picture, which is the classic accidental leak and the one case here that real tools produce.

What to change:

  • JPEG: allow markers, do not deny them. Accept SOI, at most one APP0 that is exactly a 16-byte JFIF header with Xthumbnail = Ythumbnail = 0, DQT, DHT, DRI (length exactly 4), one SOF0 (precision 8, 1 or 3 components), SOS, RSTn and EOI. Reject everything else as jpeg-metadata, or as jpeg-structure for unknown and reserved markers.
  • PNG: check lengths. Check the length of each allowed ancillary chunk: gAMA 4, cHRM 32, sRGB 1, pHYs 9, sBIT up to 4, bKGD 1/2/6 by colour type, tRNS within the palette size or 2/6. Allow PLTE only for colour type 3. Decompress the IDAT stream with zlib.decompressobj and require eof with no unused_data (stdlib only, cheap at ≤ 1 MiB).
  • Add one negative test per carrier above, built the same way the existing tests splice chunks.

B2. Notebook image outputs: JPEG normalisation lets EXIF through where main refused it, and Markdown images escape both the location rule and the cap (tools/check_notebook_fresh.py:41-44, 139-170)

  • Regression: committed JPEG outputs no longer have to match byte for byte. I built a committed notebook whose image/jpeg output carries EXIF (Artist plus a C:\Users\... description) and a "fresh" run whose JPEG has the same pixels and no EXIF:
    • The PR's check() returns no problems ("figure matches, 0 unmatched 2x2 blocks").
    • main's check_notebook_fresh.py rejects the same pair: data.image/jpeg: committed '/9j/4AAQ…RXhpZg…', fresh '/9j/4AAQ…'.
    • check_hygiene.py skips image MIME types, so nothing else looks at those bytes.
    • This happens naturally. A builder runs once locally with an unstripped source, displayed with IPython.display.Image(filename=…), which embeds the raw file bytes. They then strip the asset and commit. CI re-runs on the stripped asset, the pixels match, and the committed notebook ships the GPS/author EXIF.
    • (PNG outputs always had this gap. Now that recipes will display photographs instead of matplotlib charts, it matters.)
  • Images in Markdown are not covered. A Markdown cell with attachments holding a 6.7 MB PNG plus an EXIF JPEG, and a <img src="data:image/jpeg;base64,…"> in its source, gives a 9 MB notebook. It passes check() (Markdown attachments are not outputs, so the cap does not count them) and check_hygiene.py (attachments is skipped, tools/check_hygiene.py:390). That is an image outside assets/, with metadata, of any size.

What to change:

  • Run the asset checker's metadata rules on every committed image/jpeg output (JFIF only, as in B1).
  • Do the same for every committed image/png output, with tEXt allowed only for the keyword Software. Matplotlib writes that chunk: all 134 PNG outputs on main hold IHDR, pHYs, tEXt, IDAT, IEND.
  • Either reject Markdown attachments and data:image/ URIs in recipe notebooks (a recipe can link assets/x.png by relative path), or count them toward MAX_OUTPUT_BYTES and run the same metadata check on them.
  • Add a test for each, including the EXIF-JPEG pair that passes today.
  • Document it in docs/notebook-ci.md rule 4 and docs/fixtures.md.

B3. The contract cannot be found from the contributor documents, and this PR makes two of them inaccurate

  • .pre-commit-config.yaml:1 says it "Mirrors the CI checks (ci.yml and hygiene.yml)", and docs/development.md:349-350 says the same. After this PR hygiene.yml has a step that pre-commit does not run. Add a local hook:

    - id: recipe-assets
      name: recipe image assets (location, format, size, metadata)
      entry: python tools/check_recipe_assets.py
      language: system
      pass_filenames: false
      files: ^recipes/

    Mention it in the development.md sentence.

  • docs/development.md:77: the row for Hygiene (secrets and notebook outputs) lists only python tools/check_hygiene.py. The job now also runs python tools/check_recipe_assets.py. The "Repository hygiene" section (:271) should say in one sentence that image files are checked by the new tool, and link fixtures.md#image-fixtures.

  • CONTRIBUTING.md:13-20, "Where things go": this is the recipe contract that every recipe issue points to, and its tree has no assets/. A multimodal builder who reads only the contract will not find the rule until CI fails. Add ├── assets/ multimodal only: the images sent to Jev (PNG/baseline JPEG, ≤ 1 MiB each, no metadata); see docs/fixtures.md.

The PR body says these files were left out because they are not in the issue's "Files likely touched" list. That list is a guess, not a scope limit. Foundation PRs are not bound by the recipe allowlist, and updating these files is part of making the contract "documented and enforced". It is about ten lines.

B4. The location rule says "image signature under any name", but sniff knows five formats (tools/check_recipe_assets.py:96-109, docs/fixtures.md Location row)

These pass the checker:

  • BMP bytes saved as recipes/99-x/helpers/chart.dat.
  • ICO bytes saved as recipes/99-x/fixtures/x.bin.
  • An SVG saved as helpers/diagram.xml.

The suffix rule in Scope does not see them, and the checker reads them as non-images. Either:

  • extend sniff: BM, \0\0\1\0, ftyp + heic/heix/avif/mif1, the JP2 signature \0\0\0\x0cjP , and <svg after an optional XML prolog/BOM/whitespace (check these against a test per format), or
  • narrow the doc to "a PNG, JPEG, GIF, WebP or TIFF signature".

The first is better because it matches what the docs promise. Base64 or data: images inside helpers.py/README.md also pass. I count those as out of scope for a file checker, but say so in the doc rather than implying full coverage.

Suggestions (not blocking)

Checks run (fresh venv, Windows, Python 3.14.3, pip install -e ".[dev]", then .[dev,ml] under .github/constraints-notebooks.txt for notebook runs)

  • git diff --raw -M origin/main...HEAD: 12 files, all 100644. No symlink, submodule, mode change or type change.
  • ruff check .: All checks passed. ruff format --check .: 397 files already formatted.
  • pytest: 2 failed, 5038 passed, 58 skipped, 50 deselected, 31 xfailed. The 2 failures are the known Windows-only tests/test_notebook_ci_sandbox.py signal.SIGKILL tests (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302), which this PR does not touch.
  • python tools/render_catalog.py --check: exit 0.
  • python tools/check_hygiene.py: passed (606 files).
  • python tools/check_recipe_assets.py: passed (461 files under recipes/).
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • Offline execution, from a clean kernel with no TYPESAFE_API_KEY, of _template, 01-sentiment-classification and 24-incident-priority-composition (all have figures; the PR changes the freshness check for every notebook). Each executed (rc 0), check_notebook_fresh.py reported "matches a fresh offline run", and check_hygiene.py passed on the fresh copy. CI's 60 Notebook jobs also pass.
  • gh pr checks 322: 59 pass, Notebooks (execute) pending when read.
  • Mutation runs (each mutant applied alone, then the relevant test file run, then the source restored):
    • Killed (13): the acTL check, the non-baseline SOF check, both trailing-data raises, the total cap, the content sniff outside assets/, the suffix-case check, the COM check, the symlink/submodule mode check, JPEG in IMAGE_MIMES, the output-size cap, the Scope image rule, and the Scope case-folding.
    • Survived (1): the PNG CRC check (see S5).
    • The tree was clean afterwards.
  • Attack harness for B1/B2/B4: in-memory files fed to check_entries, Pillow load() and vision._image_format. The notebook pair was fed to the PR's and to main's check_notebook_fresh.check. Results are as quoted above.

Acceptance criteria (#307)

  • docs/fixtures.md states the contract (location, formats/suffixes, API limits, 1 MiB / 8 MiB caps, no text/EXIF/XMP chunks): met as written. It is not fully enforced (B1), and the "any name" wording overclaims (B4).
  • Checker in CI naming file and rule, with positive and negative tests (oversize, wrong place, metadata chunk, GIF renamed .png, symlink): met.
  • Scope or checker rejects images outside recipes/<slug>/assets/, with a test: met for image files. Not met for images embedded in notebooks (B2).
  • Notebook output policy: JPEG normalised, a per-notebook cap with a test, documented in notebook-ci.md: met on paper, not met in effect. Normalisation removes the byte-equality guard against committed metadata, and Markdown-embedded images escape the cap (B2).
  • .gitattributes adds *.jpeg binary: met.
  • docs/recipe-template.md gains an assets/ row linking docs/vision.md: met.

OPUS-REVIEW: CHANGES-NEEDED 367a1e3

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2). Model: Claude Opus 5.5 (claude-opus-5-5).
Reviewed head 367a1e3daba1ef1bca6482acfe9239ff8c46c3d9 against base 9a7f152202f13a1b1e4fc004d978390fc7c3232f.

I reviewed this as the builder of a multimodal recipe would use it: I generated synthetic images with Pillow 13.0.0 the way a build_fixtures.py would, committed them to a scratch repo under recipes/61-demo/assets/ and ran tools/check_recipe_assets.py --root on it. I also measured what IPython actually puts in a notebook when one of those images is displayed. The checker itself is solid and Pillow's default output goes through it cleanly. One documented number is wrong by an order of magnitude, though, and every image recipe will plan its notebook around that number.

Blocking

1. docs/notebook-ci.md:489-491 and tools/check_notebook_fresh.py (the MAX_OUTPUT_BYTES comment): "a displayed fixture can weigh twice its file, and several still fit" is false. The real factor is 8-22x, so one photo-like frame can fill the whole 2 MiB cap.

When a notebook displays a PIL.Image, IPython calls _repr_png_() and _repr_jpeg_(). Both re-encode the decoded pixels. Neither reuses the file's bytes. A JPEG fixture is shown as a lossless PNG of its JPEG artefacts, and that PNG is large. I measured with InteractiveShell.display_formatter.format(Image.open(path)), which gives the exact payload nbclient stores, base64 included:

fixture file (passes the checker) file bytes displayed bytes (png + jpeg, base64) factor
384x320 flat shapes, .jpg 10,622 91,740 8.6x
1280x720 flat shapes, .png 7,194 89,044 12.4x
1024x768 "photo-ish" (light noise + blur), .jpg 52,087 1,125,164 21.6x
1280x720 noisy frame, .jpg 119,761 1,979,728 16.5x
1568x1568 noisy, .jpg 300,935 4,928,344 16.4x

A single 120 KB frame, a twelfth of the 1 MiB file cap, takes 1.98 MB of the 2,097,152-byte notebook cap. Two such frames, or one frame plus its draw_boxes overlay, fail the Hygiene/freshness job. Flat synthetic screenshots are fine: about 90 KB each, so around 20 fit. The recipes that will hit this are the textured ones: material/swatch photos, factory frames, before/after frame pairs. A builder who trusts "twice its file" finds out only when CI fails.

The cap itself is reasonable and I would keep it. What needs fixing is the guidance:

  • Correct the sentence. Say the displayed weight depends on the pixels, not the file: Pillow re-encodes the image as PNG and JPEG, which for noisy or photographic content is 10-20x the file.
  • Give the cheap way to display a fixture: IPython.display.Image(filename=path) embeds the file's own bytes once, under its own MIME type. I measured 159,684 bytes for the 119,761-byte frame, against 1,979,728 for the PIL.Image. Also give the thumbnail route with a concrete size: the same frame thumbnailed to 640 px is 494,828 bytes.
  • Optional, but it would save every builder the same discovery: name the cap and the recommended display call in docs/vision.md or in docs/fixtures.md's new section, where the builder actually looks.

Suggestions (not blocking)

  1. matplotlib's default PNG fails the contract. fig.savefig("x.png") writes a tEXt Software chunk, and the checker reports [png-metadata] chunk tEXt is not allowed. savefig(..., metadata={"Software": None}) passes. The .jpg output passes. A recipe drawing synthetic charts or dashboards as inputs is likely, so add this one line to the "To make a file comply" paragraph in docs/fixtures.md.
  2. Add a format hint to the file-too-large message and the doc. The 1 MiB cap is only a problem for PNGs with noise or texture. My 1280x720 noisy frame was 1,357,240 bytes as PNG and 119,761 as JPEG, and the 1568x1568 noisy image was 3.6 MB as PNG and 301 KB as JPEG. A message like "photographic or noisy content: save it as JPEG" would save a round trip. Flat synthetic content is tiny: 3-14 KB even at 1568x1568.
  3. The ICC sentence in docs/fixtures.md:318-320 is only true for PNG. In Pillow 13.0.0, re-saving an opened PNG that has an ICC profile keeps iCCP, and the checker fails it. Re-saving it as .jpg writes no APP2, and the checker passes it. Re-saving a PNG with tEXt drops the text, and re-saving a JPEG with an orientation EXIF drops the EXIF. So the "copy the pixels" advice is correct and safe, but "it does carry a source's ICC profile across" should say "into a PNG".
  4. CMYK JPEGs fail with APP14 segment (EXIF, XMP, ICC, IPTC or vendor data). Pillow writes an Adobe segment for CMYK. The snippet's convert("RGB") handles it. Mentioning "Adobe" in the message would make that finding easier to understand.
  5. The 256 vs 280 contradiction. docs/recipe-template.md's new assets/ row sends the builder to docs/vision.md, which still says "each dimension 256-1568", while the checker requires 280. The PR body says Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321/Foundation: vision.py accepts 256–279 px images that the Images API rejects; fix limits and close #297's carried-over geometry and key defects before the first image fixture #306 fixes this. Merge Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321 first, or together with this, so a builder never reads both numbers at once.
  6. Follow-ups the PR lists itself: CONTRIBUTING.md "Where things go", the docs/development.md Hygiene row, and a .pre-commit-config.yaml hook. I agree they should be done. They are outside this issue's file list.

Pillow compliance (what the builder will meet)

Everything a build_fixtures.py would do by default passed: Image.save to .png (RGB, RGBA, L, palette from quantize, palette with transparency=, optimize=True, dpi= which writes pHYs) and to .jpg/.jpeg (default, quality=95, optimize=True, dpi=). Pillow's default JPEG is baseline JFIF-only. The documented "copy the pixels" snippet produces compliant .png and .jpg from an ICC-tagged source. exif_transpose followed by the copy produces a compliant upright JPEG. The size limits suit frames and tiles: a 280x280 minimum, a 320x280 tile (the vision.md example size), a 1280x720 frame and a flat 1568x1568 all pass, and a 30-frame JPEG sequence at about 120 KB each is 3.6 MB, well under 8 MiB.

The only failures were the ones the rules intend, and each message named the file and the rule: progressive=True gave jpeg-not-baseline; exif= gave jpeg-metadata APP1; a CMYK JPEG gave APP14; a noisy PNG over 1 MiB gave file-too-large; a re-saved ICC PNG gave png-metadata iCCP; matplotlib's default PNG gave png-metadata tEXt.

Code review notes

  • tools/check_recipe_assets.py: I read the PNG chunk walk (CRC, IHDR first, acTL, IEND, trailing data) and the JPEG marker walk (fill bytes, RSTn, stuffed bytes in the scan, SOF0 only, APP0 must be JFIF, APP1-15 and COM rejected, trailing data). I found no hole a synthetic builder could fall into. Allowlisting chunks, rather than blocklisting them, is the right default. The location rule sniffs content, so an image hidden under another name outside assets/ is caught. The scope rule (tools/check_recipe_scope.py) is suffix-only, correctly documented as such, and its suffix tuple is pinned equal to the checker's by a test.
  • git diff --raw -M origin/main...HEAD: 12 files, all 100644, no symlinks, submodules or mode changes, all inside the issue's file list.
  • tools/check_notebook_fresh.py: JPEG normalisation reuses compare_png, and the tests cover a re-encoded JPEG matching, a changed one going stale, a missing one and an undecodable one. The cap is checked on both files and tested at exactly the cap and one byte over.

Checks run (fresh venv, Python 3.14, Windows, no TYPESAFE_API_KEY)

  • ruff check .: all checks passed. ruff format --check .: 397 files already formatted.
  • python tools/render_catalog.py --check: exit 0.
  • python tools/check_hygiene.py: passed, 606 files.
  • python tools/check_recipe_assets.py: passed, 461 files under recipes/.
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • pytest tests/test_notebook_ci_fresh.py: 45 passed.
  • pytest (full): 2 failed, 5038 passed, 58 skipped, 50 deselected, 31 xfailed. The 2 failures are the known Windows-only tests/test_notebook_ci_sandbox.py signal.SIGKILL tests (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302), not touched here. The template notebook's offline execution and freshness check run inside tests/test_notebook_ci_fresh.py and passed.
  • Mutation checks (each applied, the relevant test file run, then reverted): allowing tEXt, allowing COM, doubling the file cap, dropping the folder cap, dropping content-sniffing outside assets/, dropping the scope image rule, dropping the output cap, and no longer normalising image/jpeg were all killed (1-13 failing tests each).
  • Scratch run of the checker on 33 Pillow/matplotlib images: results above.
  • gh pr checks 322: all 60 pass on this head

Acceptance criteria (#307)

  • docs/fixtures.md states the contract (location, PNG/baseline JPEG with lowercase suffixes, API limits plus the 1 MiB / 8 MiB caps, no text/EXIF/XMP): met.
  • Checker in CI naming file and rule, with positive and negative tests (oversize, wrong place, metadata chunk, GIF renamed .png, symlink): met. I confirmed it end to end on real Pillow output.
  • Scope guard rejects images outside recipes/<slug>/assets/, with a test: met.
  • Notebook output policy (JPEG normalised, per-notebook cap with test, docs/notebook-ci.md says which): enforced, but documented with a wrong weight model (blocking finding 1). Not met until the guidance is corrected.
  • .gitattributes *.jpeg binary: met.
  • docs/recipe-template.md assets/ row linking docs/vision.md: met.

OPUS-REVIEW: CHANGES-NEEDED 367a1e3

CompleteDotTech and others added 2 commits October 20, 2026 19:23
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…s, wider image sniffing, corrected display guidance and contributor docs (#307, fix round 1)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2, re-review): Claude Opus 5.5 (claude-opus-5-5).

Reviewed head 34562113fb5a684996d393109fb378ed6f810b38 against base 7fa806ea2e588ff3db24063d05abb002c3eee3c5 (current main). I reviewed the fix commit 3456211, the merge 8f01272, and the full diff against origin/main. I did not read the other reviewer's comments first.

Summary. Fix round 1 resolves all four of my earlier blocking findings, B1–B4. Every carrier from my first harness is now caught, the new code is well tested, and the contributor docs are updated. One gap is left, and it is in the new rule itself: "a Markdown or raw cell may not embed an image" can be bypassed with a capital DATA:, a ;charset= parameter, or a non-base64 SVG URI. Those are the cells the freshness check never re-executes, so nothing else catches them. The fix is a regex flag plus a test, so I am asking for it before merge. Everything else below is a suggestion.

Status of my round-1 findings

Finding Status Evidence on this head (attack.py / nb_attack.py re-run unchanged)
B1: metadata inside allowed JPEG segments / PNG chunks Resolved All 11 carriers are now caught: fat JFIF APP0, second APP0, DNL, JPG0, reserved 0xFF02, oversized DRI, PLTE in an RGB image, oversized pHYs/bKGD, and bytes after the zlib stream. The JPEG check is now an allowlist of markers (tools/check_recipe_assets.py:282-367). PNG chunk lengths are fixed per colour type (:154-180), and IDAT is inflated and must end cleanly (:254-264).
B2: EXIF in committed JPEG outputs; Markdown attachments / data URIs; no cap Resolved as reported (gap below) committed ... image/jpeg that breaks [jpeg-metadata]: APP1 ... now appears even though the pixels match. The 6.7 MB Markdown attachment is now refused in both files. All 51 committed notebooks pass the new rules.
B3: pre-commit / development.md / CONTRIBUTING.md Resolved There is a recipe-assets local hook (files: ^recipes/, pass_filenames: false). The Hygiene row and the pre-commit sentence in docs/development.md are updated, the hygiene section points to the sibling tool, and CONTRIBUTING.md has an assets/ line.
B4: sniffing overclaimed Resolved BMP-as-.dat, ICO-as-.bin and SVG-as-.xml are now caught (sniff, :98-127). Base64 and data: images inside text files are now stated as not detected.
S1: pin to vision.IMAGE_LIMITS Open (follow-up) Still for whichever of #321/#322 merges second.
S2: interlace / 16-bit / SOF0 precision Resolved Documented as accepted. SOF0 must be 8-bit with 1 or 3 components and the exact length.
S3: where the cap runs Resolved notebook-ci.md rule 4 now says the cap runs in the Notebook (<recipe>) job.
S5: CRC test did not test the CRC Resolved test_a_png_crc_error_is_reported_as_a_crc_error flips byte 20, and my CRC mutant is now killed.

Blocking

R1. The "no image in a text cell" rule can be bypassed, and notebook/cell metadata is not covered (tools/check_notebook_fresh.py:70, :210-217)

DATA_IMAGE_URI = re.compile(r"data:image/([A-Za-z0-9.+-]+);base64,(...)") is case-sensitive and requires ;base64, straight after the type. I built Markdown cells around an EXIF-bearing JPEG (Artist "Timothy Gregg"). Each of these returns no problems from embedded_image_problems:

  • <img src='DATA:image/jpeg;base64,…'>: browsers and Jupyter treat the scheme as case-insensitive.
  • <img src='data:image/jpeg;charset=utf-8;base64,…'>: a valid data URI with a parameter.
  • <img src="data:image/svg+xml,%3Csvg…%3E%3Ctext%3ETimothy…">: not base64.
  • A cell metadata value, or a notebook-level metadata key, holding data:image/jpeg;base64,… or {"image/jpeg": …}. Only source and outputs are searched.

Text cells and metadata are never executed, so the committed and fresh copies are always equal. This rule is the only guard on them, and check_hygiene.py skips base64 images. This is the same "image outside assets/, any size, with metadata" hole that B2 was about, reached through a different spelling.

What to change:

  • Text cells: nothing needs decoding, because any embedded image is a problem. Flag re.search(r"data:\s*image/", source, re.IGNORECASE) in text cells.
  • Notebook-wide: run the same search over every string in notebook and cell metadata, and flag any image/* key there.
  • Outputs: make DATA_IMAGE_URI case-insensitive and allow parameters: data:image/([\w.+-]+)(?:;[^;,]*)*;base64,(...). A non-base64 data:image/svg+xml, in an output is safe to leave, because text/html and SVG outputs are compared byte for byte.
  • Add one test per spelling above.

Suggestions (not blocking)

  • S6. Test gaps found by mutation. I ran 31 mutants: my round-1 set unchanged plus 20 on the new code. 29 were killed, 2 survived:

    • Removing if saw_app0 or size is not None: survives. A second, exact 16-byte JFIF APP0 is never tested; the existing test's second APP0 carries text, so the length check catches it.
    • Removing the marker allowlist (:303) survives. An unknown marker then falls into _jpeg_check_tables, which rejects the test bodies as a "DHT" with the same rule name. With the allowlist gone, a DNL with 17 zero bytes would parse as an empty Huffman table and pass.
    • Fix both by asserting the message ("is not one a baseline JPEG fixture may contain") and by adding a body that is a valid table.
  • S7. The remaining carriers are about steganography; one sentence in the docs would cover them. These all still pass the checker, Pillow load() and vision.py:

    • ASCII text placed between the end of the entropy-coded data and EOI;
    • extra DQT segments redefining table 3 with text (8 segments, 512 bytes);
    • an extra DHT redefining AC table 3 with text as symbol values;
    • repeated cHRM/pHYs chunks with arbitrary values;
    • text appended to the image rows inside the zlib stream;
    • a 256-entry PLTE of text in a palette image that uses one entry.

    None of these is written by any ordinary tool, and pixel values can always hide data, so I am not asking for a JPEG Huffman decoder. Two cheap fixes match what the PNG specification already says:

    • Reject a second copy of any ancillary chunk (each of gAMA, cHRM, sRGB, sBIT, pHYs, bKGD, tRNS may appear at most once).
    • Require the inflated IDAT length to equal the length the IHDR implies (rows × (1 + bytes per row), summed over the Adam7 passes when interlaced). You already inflate it.

    Then add one sentence to docs/fixtures.md: the check removes every place where tools write metadata, but it does not prove that an image hides nothing.

  • S8. Pin the embedded tEXt Software value. An output PNG may hold any Software\0<up to 191 bytes>, and any number of those chunks: five chunks of personal text passed. Allow at most one, matching Matplotlib version[0-9.]+, https://matplotlib\.org/, which is what all 134 committed figures hold.

  • S1 (carried over): pin the checker constants to jev_cookbook.vision.IMAGE_LIMITS once Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321 is in. Today's numbers agree.

Checks run (fresh venv, Windows, Python 3.14.3; .[dev,ml] under .github/constraints-notebooks.txt for notebook execution)

  • git diff --raw -M origin/main...HEAD: 15 files, all 100644. No symlink, submodule, mode change or type change. The merge brings in only main's own commits (Foundation: evaluation guidance and recall_at_budget eligibility #318, Foundation: shared review-queue/action-log fill helpers and a side-effect invocation guard (#210, #212) #319). docs/recipe-template.md's other changes come from main.
  • ruff check .: All checks passed. ruff format --check .: 397 files already formatted.
  • pytest (full, on this head): 3 failed, 5244 passed, 58 skipped, 50 deselected, 31 xfailed.
  • python tools/render_catalog.py --check: exit 0.
  • python tools/check_hygiene.py: passed (606 files).
  • python tools/check_recipe_assets.py: passed (461 files).
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • Offline execution from a clean kernel with no TYPESAFE_API_KEY of _template, 01-sentiment-classification and 24-incident-priority-composition. All rc 0; check_notebook_fresh.py (now with rule 5) reported "matches a fresh offline run", and check_hygiene.py passed on each fresh copy.
  • output_problems over all 51 committed notebooks: no problems.
  • gh pr checks 322: 60 pass, none pending or failing.
  • Harnesses:
    • attack.py and nb_attack.py re-run unchanged: results in the table above.
    • New attack2.py: the R1 and S7 carriers and the S8 text.
    • mutate.py re-run unchanged: 11 of 11 killed. The old elif marker == 0xFE site moved; mutated at its new place, it is also killed.
    • New mutate2.py: 18 of 20 killed (S6).
    • The tree was clean afterwards.

Acceptance criteria (#307)

  • docs/fixtures.md states the contract: met.
  • Checker in CI naming file and rule, with positive and negative tests (oversize, wrong place, metadata chunk, GIF-as-.png, symlink): met.
  • Images outside recipes/<slug>/assets/ are rejected, with a test: met for files and outputs. Not fully met for text cells and metadata (R1).
  • Notebook output policy (JPEG normalised, per-notebook cap with a test, documented): met. The new embedded-metadata rule is met for outputs; text cells are R1.
  • .gitattributes adds *.jpeg binary: met.
  • docs/recipe-template.md assets/ row linking vision.md: met.

OPUS-REVIEW: CHANGES-NEEDED 3456211

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2, re-review). Model: Claude Opus 5.5 (claude-opus-5-5).
Reviewed head 34562113fb5a684996d393109fb378ed6f810b38 against base 7fa806ea2e588ff3db24063d05abb002c3eee3c5. My first review was on 367a1e3d (comment 6304024126).

I reviewed fix round 1 (3456211), the merge of origin/main (8f01272), and the full diff against origin/main. Nothing is blocking.

My earlier findings

# Finding Status
Blocking 1 "a displayed fixture can weigh twice its file" was wrong by 8-22x Resolved. docs/notebook-ci.md rule 4, docs/fixtures.md and the MAX_OUTPUT_BYTES comment now carry the measured numbers. They recommend IPython.display.Image(filename=path) and thumbnail((640, 640)) on a copy. I checked that the advice holds when the notebooks actually run (below).
Suggestion 1 matplotlib's default PNG fails Resolved. docs/fixtures.md gives savefig(path, metadata={"Software": None}).
Suggestion 2 file-too-large should point to JPEG Resolved. Both the message and the doc now do.
Suggestion 3 The ICC sentence is true only for PNG Resolved. It now says "saving to PNG".
Suggestion 4 CMYK APP14 finding is hard to read Resolved. The message names "Adobe", and the doc explains convert("RGB").
Suggestion 5 vision.md says 256 while the checker says 280 Unchanged here, as expected. It belongs to #321. A test now pins the checker constants to the 280-1568 contract and to the package's limits once it exports them. Merging #321 first, or together, is still my recommendation.
Suggestion 6 CONTRIBUTING.md, development.md and pre-commit should mention the checker Resolved. The assets/ line, the CI-table row and the recipe-assets hook were all added, with tests.

The stricter allowlists against real builder output

My 33 scratch images through the new checker. I got exactly the same 7 findings as before and no new failures:

  • the CMYK APP14 segment
  • an EXIF APP1 block
  • 2 noisy PNGs over 1 MiB, whose message now carries the JPEG hint
  • matplotlib's default PNG (tEXt)
  • a progressive JPEG
  • a re-saved PNG carrying an ICC profile (iCCP)

mpl_nometa.png (metadata={"Software": None}) passes, as do mpl_default.jpg, the doc's copy snippet, and the image copied after exif_transpose.

A wider matrix of 173 in-memory saves with Pillow 13.0.0 and matplotlib, run through check_image:

  • JPEG at quality 1/5/10/25/50/75/85/95/100, crossed with subsampling 0/1/2, each with and without optimize, on flat and noisy content.
  • JPEG in mode L, with restart_marker_blocks, with restart_marker_rows, with dpi=(0, 0), and with quality="web_high".
  • PNG in modes RGB, RGBA, L, LA, 1 and I;16.
  • Palette PNGs with 2/4/16/256 colours, with and without optimize or bits=, and with transparency= on palette, L and RGB images.
  • PNG at compress_level 0, 1 and 9.
  • matplotlib savefig as PNG with no Software line, as JPEG, and as JPEG with pil_kwargs.

All pass, except keep_rgb=True. That option writes an Adobe APP14 segment, it is obscure, and the rejection is correct.

Notebooks executed with nbclient, then judged by check_notebook_fresh.output_problems:

Notebook Output weight Problems
8 fixtures (PNG and JPEG, including the 120 KB and 301 KB noisy frames) shown with IPython.display.Image(filename=...) 679,285 none
%matplotlib inline bar chart (PNG with the Software line) 11,457 none
A small PIL.Image as the last expression 23,339 none
Two noisy frames shown as PIL.Image 3,105,220 over the cap, as the doc now predicts
The same two frames after thumbnail((640, 640)) 1,039,126 none
IPython.display.Image(filename="exif.jpg") 17,224 jpeg-metadata APP1, so the new embedded-metadata rule fires on real output

All 51 committed recipes/*/notebook.ipynb pass embedded_image_problems.

Suggestions (not blocking)

  1. The cap message still says only "Show fewer images, or a downscaled copy (Image.thumbnail)" (tools/check_notebook_fresh.py, in output_problems). Since the doc's main advice is now IPython.display.Image(filename=path), put that in the message as well. The builder who sees this message is the one who needs it.
  2. The JPEG marker allowlist is not pinned by a test. I disabled if marker not in (_JPEG_APP0, _JPEG_SOF0, ...) and all 142 tests in tests/test_check_recipe_assets.py still passed. Any unknown marker then falls through to _jpeg_check_tables and is usually still rejected as jpeg-structure, so the behaviour stays safe today, but nothing asserts the allowlist's own message. A one-line test would pin it: insert a 0xFFF0 segment whose body is shaped like a DHT table and assert "is not one a baseline JPEG fixture may contain". I confirmed the current code produces that message.
  3. Say what the allowlist is for. An extra DHT table, for a table id the scan never uses, can carry up to 256 arbitrary bytes and passes: I tested a 49-byte "GPS ... author ... token" payload. Palette entries and DQT values can carry bytes the same way. That is fine for this threat model, which is metadata written by tools and editors, and no tool writes such tables. The docs and code comments ("so no byte is spare", "whole tables only") could lead a reader to expect more. One sentence in docs/fixtures.md saying the rules remove the metadata tools write, not deliberately hidden bytes, would set that expectation honestly.

Mutation checks

Each mutation was applied, the relevant test file run, and the source restored. The worktree was clean afterwards.

  • Killed: APP0 length/thumbnail check off; bytes after the IDAT zlib stream allowed; ancillary-chunk lengths unchecked (14 failures); SVG sniff off; embedded-image checks off (7 failures); any tEXt allowed in outputs; Markdown image embedding allowed.
  • Survived: JPEG marker allowlist off (suggestion 2).
  • Earlier round, still covered: my 8 first-round mutations (tEXt and COM allowed, file and folder caps, content sniffing, the scope image rule, the output cap, JPEG normalisation) were all killed. The tests for those rules are unchanged or extended.

Scope and diff

  • git diff --raw -M origin/main...HEAD: 15 files, all 100644. No symlinks, submodules, mode or type changes.
  • The new files are .pre-commit-config.yaml, CONTRIBUTING.md and docs/development.md. The issue's list did not name them, but they are the follow-ups I asked for, and they only describe the checker.
  • The merge of origin/main brought no PR-side changes to the simulation or evaluation files: the three-dot diff contains none of them.

Checks run on 34562113 (fresh venv, Python 3.14, Windows, no TYPESAFE_API_KEY)

  • ruff check .: all checks passed. ruff format --check .: 397 files already formatted.
  • python tools/render_catalog.py --check: exit 0.
  • python tools/check_hygiene.py: passed, 606 files.
  • python tools/check_recipe_assets.py: passed, 461 files under recipes/.
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • pytest (full, on this head): 2 failed, 5245 passed, 58 skipped, 50 deselected, 31 xfailed, in 16m38s. The 2 failures are the known Windows-only tests/test_notebook_ci_sandbox.py signal.SIGKILL tests (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302), which this PR does not touch. tests/test_notebook_ci_fresh.py executes the template notebook offline and checks it for freshness, and it passed within this run.
  • gh pr checks 322: all 60 pass on this head.

Acceptance criteria (#307)

  • docs/fixtures.md contract (location; PNG or baseline JPEG with lowercase suffixes; API limits plus the 1 MiB / 8 MiB caps; no text, EXIF or XMP): met.
  • A checker in CI that names file and rule, with positive and negative tests (oversize, wrong place, metadata chunk, GIF renamed .png, symlink): met. Confirmed on real Pillow and matplotlib output.
  • Scope guard rejects images outside recipes/<slug>/assets/, with a test: met.
  • Notebook output policy (JPEG normalised, a per-notebook cap with a test, documented in docs/notebook-ci.md): met. The display guidance is now correct and measured, and embedded images are also held to the metadata rules.
  • .gitattributes *.jpeg binary: met.
  • docs/recipe-template.md assets/ row linking docs/vision.md: met.

OPUS-REVIEW: APPROVE 3456211

…hunk and inflated-length checks, matplotlib Software pin, more mutation tests (#307, fix round 2)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2, re-review 2): Claude Opus 5.5 (claude-opus-5-5).

Reviewed head 171240fc2d95264b06832eb14e1a3e158b388728 against base 7fa806ea2e588ff3db24063d05abb002c3eee3c5 (current main). I reviewed fix commit 171240f (everything since 34562113) and the full diff against origin/main. I did not read the other reviewer's comments first.

Summary. My one blocking finding from the last round (R1) is resolved, and so are the in-scope suggestions S6, S7 and S8. Per the orchestrator's scope decision for this round, hand-built hidden bytes are out of scope: JPEG table contents, ASCII before EOI, and the entries of a legitimate palette. They still pass, as expected, and docs/fixtures.md now says so: "These rules remove the places where ordinary tools write metadata into an image; they are not a detector for deliberately hidden bytes". I have no blocking findings.

Status of my round-2 findings (attack2.py re-run unchanged)

Finding Status Evidence
R1: the text-cell data:image rule could be bypassed; metadata was not scanned Resolved All six spellings and locations are now refused: DATA:image/jpeg;base64,, ;charset=utf-8;base64,, the non-base64 data:image/svg+xml,%3Csvg…, percent-encoded base64, a cell metadata data URI, and a notebook-level {"image/jpeg": …}. ANY_DATA_IMAGE = re.compile(r"data:\s*image/", re.IGNORECASE) now applies to text cells and to every key and string under notebook and cell metadata (tools/check_notebook_fresh.py:75, 196-242). Output metadata (where Jupyter legitimately keeps image/png sizes) is not walked. Output data URIs are case-insensitive and allow parameters (:70-72).
S6: mutation survivors (second exact JFIF APP0; marker allowlist) Resolved Both mutants are now killed.
S7: repeated ancillary chunks; data inside the zlib stream Resolved 4× cHRM and 10× pHYs give chunk … appears more than once. Text after the rows inside the stream gives IDAT inflates to 360,379 bytes; the image header implies 360,300. png_raw_length handles Adam7. It is tested against an independent per-pixel count (tests/test_check_recipe_assets.py:852-886), and both arithmetic mutants are killed.
S7 (out of scope per orchestrator): JPEG DQT/DHT contents, ASCII before EOI, palette entries Still pass, documented Not blocking.
S8: the Software value was free text and could repeat Resolved Personal text and five chunks are both refused. The chunk must fullmatch Software\0Matplotlib version[0-9][0-9A-Za-z.+_-]*, https://matplotlib\.org/, at most once. All 51 committed notebooks still pass.
S1: pin to vision.IMAGE_LIMITS Open (follow-up) For whichever of #321/#322 merges second. Today's numbers agree.

I also had the checker inspect PNGs that Pillow writes in modes 1, L, LA, RGBA, I;16 and P (8/4/2/1-bit, with tRNS), and an optimize=True RGB PNG, all at odd sizes (301 × 283). Every one was accepted, so the new exact-length and duplicate rules raise no false positives on ordinary files.

Blocking findings

None.

Suggestions (not blocking)

  • S9. One survivor among the new mutants. Removing if not inflater.eof or inflater.unconsumed_tail: (tools/check_recipe_assets.py:298) now survives. The new length check catches a truncated stream first, with the same rule, so the check only matters when the deflate data is complete but the zlib end (final block or Adler-32) is missing. A test with zlib.compress(rows)[:-4] would pin it.
  • S10. Cap the inflate at the expected length. _MAX_INFLATED is 256 MiB. expected is known before inflating, so inflater.decompress(idat, expected + 1) bounds memory to about 20 MB for the largest legal asset. An embedded matplotlib figure is small, so this changes nothing in practice. It just stops a 1 MiB deflate bomb from allocating 256 MiB in the Hygiene job.

Checks run (fresh venv, Windows, Python 3.14.3; .[dev,ml] under .github/constraints-notebooks.txt for notebook execution)

  • git diff --raw -M origin/main...HEAD: 15 files, all 100644 (two added, 13 modified). No symlink, submodule, mode change or type change. This round changes only tools/check_recipe_assets.py, tools/check_notebook_fresh.py, their two test files, and one sentence of docs/fixtures.md.
  • ruff check .: All checks passed. ruff format --check .: 397 files already formatted.
  • pytest (full, on this head): 3 failed, 5283 passed, 58 skipped, 50 deselected, 31 xfailed.
    • Two failures are the known Windows-only tests/test_notebook_ci_sandbox.py signal.SIGKILL tests (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302).
    • The third is tests/test_new_recipe.py::test_a_failed_start_leaves_no_exit_time_noise. It failed in the full run, as it did last round, but passed when re-run alone and in a full run of tests/test_new_recipe.py (84 passed). It is a timing-sensitive kernel-restart probe under load. This PR does not touch it or tools/execute_notebook.py/tools/new_recipe.py, and CI's Tests (py3.10) and Tests (py3.14) pass.
  • python tools/render_catalog.py --check: exit 0.
  • python tools/check_hygiene.py: passed (606 files).
  • python tools/check_recipe_assets.py: passed (461 files).
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • Offline execution from a clean kernel with no TYPESAFE_API_KEY of _template, 01-sentiment-classification and 24-incident-priority-composition. All rc 0; check_notebook_fresh.py reported "matches a fresh offline run" (the new metadata scan raised nothing on fresh output), and check_hygiene.py passed on each fresh copy.
  • output_problems over all 51 committed notebooks: none.
  • gh pr checks 322: 60 pass.
  • Harnesses:
    • Re-run unchanged:
      • attack.py: all 13 carriers caught. Only the base64 strings inside text files pass, which the docs now state.
      • nb_attack.py: both carriers caught.
      • attack2.py: table above.
    • Mutation runs:
      • mutate.py: 10 of 10 killed (one site moved, see below).
      • mutate2.py: 16 of 17 killed, S9 the survivor (three sites moved).
      • New mutate3.py, on the round-2 code and the moved sites: 15 of 15 killed. The two Software mutants are killed by tests/test_check_recipe_assets.py::test_only_one_matplotlib_software_line_is_allowed_in_an_embedded_png.
    • The tree was clean afterwards.

Acceptance criteria (#307)

  • docs/fixtures.md states the contract (location, formats/suffixes, API limits, 1 MiB / 8 MiB caps, no text/EXIF/XMP chunks), now including its threat model: met.
  • A checker in CI names the file and the rule, with positive and negative tests (oversize, wrong place, metadata chunk, GIF renamed .png, symlink): met.
  • Images outside recipes/<slug>/assets/ are rejected, with a test: met, for files (Scope and the checker) and for notebooks (outputs, text cells, metadata).
  • Notebook output policy (JPEG normalised, per-notebook output cap with a test, documented in docs/notebook-ci.md): met, plus metadata rules for embedded images.
  • .gitattributes adds *.jpeg binary: met.
  • docs/recipe-template.md has an assets/ row linking docs/vision.md: met. CONTRIBUTING.md, docs/development.md and .pre-commit-config.yaml are also updated.

OPUS-REVIEW: APPROVE 171240f

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2, re-review 2). Model: Claude Opus 5.5 (claude-opus-5-5).
Reviewed head 171240fc2d95264b06832eb14e1a3e158b388728 against base 7fa806ea2e588ff3db24063d05abb002c3eee3c5. My earlier reviews were 6304024126 (367a1e3d) and 6304638208 (34562113).

I reviewed everything changed since 34562113, which is one commit, 171240f: tools/check_recipe_assets.py, tools/check_notebook_fresh.py, docs/fixtures.md and the two test files. I also checked the full diff against origin/main again. Nothing is blocking.

What changed, and whether it is right

  • PNG _PNG_ONCE: gAMA, cHRM, sRGB, sBIT, pHYs, bKGD and tRNS may each appear at most once, as the PNG spec says. This is correct.
  • PNG png_raw_length: the IDAT must inflate to exactly the length the header implies, with one filter byte per row and Adam7 passes when the image is interlaced. I checked the pass geometry against the spec. It is correct for widths and heights smaller than 8, where empty passes are skipped. interlace is set when IHDR is unpacked, so the interlace = 0 initialiser is only a default.
  • Embedded tEXt: a notebook output may now hold at most one chunk, and it must match Software\x00Matplotlib version<ver>, https://matplotlib.org/ exactly. This is stricter than round 1's "any Software line of 200 bytes or less", and it still matches what matplotlib 3.11.2 actually writes (below).
  • Data URIs: ANY_DATA_IMAGE (data:\s*image/, case-insensitive) is applied to text-cell sources and to every key and string in notebook and cell metadata. The output data-URI regex now accepts any case and parameters such as ;charset=... before ;base64.
  • Cap message: it now names IPython.display.Image(filename=path) (my round-1 suggestion 1). Resolved.
  • JPEG marker allowlist: now pinned by a test (suggestion 2). Disabling the allowlist now fails a test, where in round 1 it survived. Resolved.
  • docs/fixtures.md: now says the rules "are not a detector for deliberately hidden bytes" (suggestion 3). Resolved.

Ordinary output still passes

My 173-save corpus, rerun on this head with Pillow 13.0.0 and matplotlib 3.11.2:

  • JPEG at quality 1-100, crossed with subsampling 0/1/2, each with and without optimize, on flat and noisy content.
  • JPEG in mode L, with restart markers, with dpi=(0, 0), and with web_high.
  • PNG in modes RGB, RGBA, L, LA, 1 and I;16.
  • Palette PNGs with 2-256 colours, with and without bits/optimize, and with transparency= on palette, L and RGB images.
  • PNG at compress_level 0, 1 and 9.
  • matplotlib savefig as PNG (metadata={"Software": None}) and as JPEG.

Result: 171 pass. The other 2 are keep_rgb=True, which writes an Adobe APP14 segment and is rightly rejected, the same as round 1.

My 33-file scratch repository, through tools/check_recipe_assets.py --root: the same 7 intended findings as both earlier rounds (CMYK APP14, EXIF APP1, two noisy PNGs over 1 MiB, matplotlib's default PNG, a progressive JPEG, a re-saved PNG with iCCP). Nothing new.

Notebooks executed with nbclient on this head, judged by check_notebook_fresh.output_problems:

Notebook Weight Problems
8 fixtures via IPython.display.Image(filename=...) 679,285 none
%matplotlib inline bar chart 11,457 none
matplotlib variants: a figure as the last expression, figure_formats=['retina'], ['png', 'jpeg'], and a 2x2 imshow grid via display(fig) 144,345 none
jev_cookbook.vision.draw_boxes overlay as the last expression 25,251 none
A small PIL.Image 23,339 none
Two noisy frames shown as PIL.Image 3,105,220 over the cap; the message now recommends IPython.display.Image(filename=path)
The same frames after thumbnail((640, 640)) 1,039,126 none
IPython.display.Image(filename="exif.jpg") 17,224 jpeg-metadata APP1, as intended

Every inline PNG in those notebooks carries exactly one tEXt chunk, Software\x00Matplotlib version3.11.2, https://matplotlib.org/, and it matches the new pattern. The retina output's {"image/png": {"width": ..., "height": ...}} is output metadata, so the new notebook- and cell-metadata rule correctly leaves it alone.

Mutation checks on this head

Each mutation was applied, the relevant test files run, and the source restored. The worktree was clean afterwards. All 13 were killed:

  • Rules from round 1: the APP0 length check; bytes after the IDAT zlib stream; ancillary-chunk lengths; the JPEG marker allowlist (survived in round 1, now killed); the embedded-image checks.
  • New rules: allowing two Software chunks; allowing any Software text; allowing duplicate ancillary chunks; skipping the inflated-length check; computing interlaced length as non-interlaced; turning off the metadata image check; reverting text cells to the strict base64-only regex; making the output data-URI regex case-sensitive.

Suggestions (not blocking)

  1. The Software pattern is tied to matplotlib's current wording (Matplotlib version...). If a future matplotlib changes it, every chart-bearing notebook fails at once after a constraints bump. The failure is loud and the message is clear, so this is acceptable. A code comment saying "update _EMBEDDED_SOFTWARE if matplotlib changes its signature" would help whoever bumps the constraints.
  2. The new metadata rule flags any metadata string that starts with image/, or contains data:image/, anywhere in notebook or cell metadata. That is right for embedded images. A markdown cell that merely explains data:image/... as literal text would also be flagged. No recipe needs to do that today.

Checks run on 171240fc (fresh venv, Python 3.14, Windows, no TYPESAFE_API_KEY)

  • ruff check .: all checks passed. ruff format --check .: 397 files already formatted.
  • python tools/render_catalog.py --check: exit 0.
  • python tools/check_hygiene.py: passed, 606 files.
  • python tools/check_recipe_assets.py: passed, 461 files under recipes/.
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • pytest (full, on this head): 4 failed, 5282 passed, 58 skipped, 50 deselected, 31 xfailed, in 11m00s.
    • Two failures are the known Windows-only tests/test_notebook_ci_sandbox.py signal.SIGKILL tests (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302).
    • The other two were kernel start-up flakes on this Windows machine, in files this PR does not touch. tests/test_new_recipe.py::test_a_warning_in_a_cell_counts_as_stderr failed with zmq.error.ZMQError: Connection refused, and tests/test_notebook_ci_netguard.py::test_offline_notebook_runs_clean_under_the_guard hit execute_notebook.py timing out at 180 s.
    • Rerun on their own on the same head, both passed (2 passed in 15.30s). Both passed in my round-1 full run, and they pass in CI.
    • An earlier full-run attempt on this head hung in the same kernel-executing tests and I stopped it. That attempt overlapped with my own nbclient runs.
    • All tests in tests/test_check_recipe_assets.py, tests/test_notebook_ci_fresh.py and tests/test_notebook_ci_scope.py passed, including the committed-notebook embedded-image test and the template's offline execution and freshness check.
  • gh pr checks 322: all 60 pass on this head.
  • git diff --raw -M origin/main...HEAD: 15 files, all 100644. No symlinks, submodules, mode or type changes. The scope is unchanged from round 1.

Acceptance criteria (#307)

  • docs/fixtures.md contract (location; PNG or baseline JPEG with lowercase suffixes; API limits plus the 1 MiB / 8 MiB caps; no text, EXIF or XMP): met.
  • A checker in CI that names file and rule, with positive and negative tests (oversize, wrong place, metadata chunk, GIF renamed .png, symlink): met.
  • Scope guard rejects images outside recipes/<slug>/assets/, with a test: met.
  • Notebook output policy (JPEG normalised, a per-notebook cap with a test, documented in docs/notebook-ci.md): met. The guidance is accurate and the cap message is actionable.
  • .gitattributes *.jpeg binary: met.
  • docs/recipe-template.md assets/ row linking docs/vision.md: met.

OPUS-REVIEW: APPROVE 171240f

@CompleteDotTech
CompleteDotTech merged commit 327b0a0 into main Oct 21, 2026
61 of 62 checks passed
@CompleteDotTech
CompleteDotTech deleted the foundation/image-assets-contract branch October 21, 2026 01:14
CompleteDotTech added a commit that referenced this pull request Oct 21, 2026
…table to Windows (#302) (#325)

Closes #302

## What this adds

Two minimal, Windows-only portability fixes. Linux behaviour is
unchanged.

- `tools/execute_notebook.py` (CI gate): `signal.SIGKILL` ->
`_KILL_SIGNAL = getattr(signal, "SIGKILL", signal.SIGTERM)`. On Linux
(CI) this is exactly `signal.SIGKILL`, so the `os.kill` call is
identical. The helper only signals anything when
`in_an_isolated_pid_namespace()` is true (needs `/proc`, Linux), so the
SIGTERM fallback is never used on a real process; it just lets the unit
tests (which mock `os.kill`) run on Windows. No test is skipped or
loosened.
- `tests/test_docs_examples.py`: `test_block_executes` and
`test_block_four_equivalence_holds` skipped on Windows (no `SIGALRM`). A
`_time_limit()` context manager keeps the `SIGALRM` path byte-for-byte
equivalent on POSIX and uses a `threading.Timer` +
`_thread.interrupt_main` elsewhere, so these now run on Windows with a
10 s limit. (Superseded by fix round 1 below: the Windows path now uses
a dedicated `SIGBREAK` handler.)

Not touched: other executor hardening (PR #316's issue).
`docs/notebook-ci.md` needs no change.
The one remaining skip in the sandbox tests (`NO_PROCFS`, reads real
`/proc/self/stat`) is pre-existing and genuinely Linux-only.

## Recipe checklist

Skipped: foundation work.

## How it was verified

Fresh venv on Windows 11, `pip install -e ".[dev]"`:
- `pytest`: 4937 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed
(previously 2 failures in `tests/test_notebook_ci_sandbox.py`).
- `pytest tests/test_docs_examples.py
tests/test_notebook_ci_sandbox.py`: 32 passed, 1 skipped (`NO_PROCFS`).
- `ruff check .`: all checks passed. `ruff format --check .`: 395 files
already formatted.
- Linux behaviour: diff in `execute_notebook.py` is a one-name
substitution resolving to `signal.SIGKILL` on Linux; Linux CI runs the
same sandbox tests. Not run on Linux locally.
- Not run: `render_catalog --check`, `check_hygiene`, fixture validator,
notebook execution (no recipe, catalog or notebook touched).

## Fix round 1 (head 1f1c2cd)

Both independent reviews of 4c6dbb3 asked for changes. Resolutions:

- **Windows timeout race (review 1 blocking 1, review 2 B1):**
`timer.is_alive()` could not tell the timer's interrupt from a real
Ctrl-C, so a few percent of hangs escaped as a bare `KeyboardInterrupt`.
`_time_limit` in `tests/test_docs_examples.py` now follows review 2's
design on Windows: the timer calls
`_thread.interrupt_main(signal.SIGBREAK)`, a handler installed only by
this helper raises `_Timeout` while an `armed` flag is set, and
`finally` clears the flag, cancels and joins the timer, then restores
the old handler. A real Ctrl-C (SIGINT) is untouched. No wait-it-out
loop. POSIX `SIGALRM` path unchanged.
- **Regression test for the old race shape:**
`test_windows_time_limit_always_raises_timeout_for_a_busy_loop` runs 100
limits of 0.01 s on a busy loop and requires every one to raise
`_Timeout` (about 3 s; passed 5 of 5 separate runs). Skipped on POSIX
with a stated reason: it exercises the `SIGBREAK` path, which exists
only on Windows.
- **Residual window (correcting the earlier "no stray interrupt"
claim):** if a body finishes within microseconds of the deadline, a
`_Timeout` can still be raised at the edge. This is documented in the
`_time_limit` docstring. These blocks take milliseconds against a 10 s
limit. I did not re-measure it.
- **Pin the Linux kill signal (review 1 S2, review 2 S1):** the kill
test now records the signal and asserts it equals `_KILL_SIGNAL`; a new
test asserts `_KILL_SIGNAL == signal.SIGKILL` wherever `SIGKILL` exists.
- **Comment fix:** the `execute_notebook.py` comment no longer claims
the fallback "lets the module import"; it says it lets the helper's unit
tests (which mock `os.kill`) run on Windows.
- **Docs (review 2 S2, S3):** the module docstring of
`tests/test_docs_examples.py` describes both timeout paths; the
`_time_limit` docstring notes the Windows path cannot interrupt a
blocking call such as `time.sleep`.

Verification at this head (Windows 11): `ruff check .` and `ruff format
--check .` clean; `render_catalog --check` rc 0; `check_hygiene` passed;
fixture validator 51 recipe folders, 0 failed;
`tests/test_docs_examples.py` + `tests/test_notebook_ci_sandbox.py` 34
passed, 1 skipped (existing `/proc` skip).

**Full `pytest` on Windows: one run, 1 failed, 5077 passed, 49 skipped,
31 xfailed.** The failure was
`tests/test_new_recipe.py::test_a_looping_cell_hits_the_timeout_with_one_line_and_leaves_the_file`,
a kernel-startup test this PR does not touch. I did not capture its
error text; review 2 saw the same test fail once with
`zmq.error.ZMQError: Address in use`, which is the likely cause but is
not confirmed for my run. Re-running the whole file passed (84 passed).
The orchestrator has filed this flake as a separate issue. I do not have
a single clean 0-failure full run at this head.

## Fix round 2 (head dc7cfff)

Main advanced to 327b0a0 (#322), so the branch took a signed merge of
`origin/main`. Taken from the approving reviews' non-blocking
suggestions, nothing else:
- `_time_limit` (Windows path) now nests the try/finally so the previous
`SIGBREAK` handler is always restored, even if the timer trips between
the end of the body and `armed = False`.
- The race-shape test converts a bare `KeyboardInterrupt` into
`pytest.fail(...)`. Added review 2's two deterministic Windows-only
tests (a real Ctrl-C passes through; a timer trip after the block has no
effect), skipped on POSIX with a stated reason.
- Rewrapped the two over-100-character module-docstring lines in
`tests/test_docs_examples.py`.

Verification (Windows 11): full `pytest` 5299 passed, 49 skipped, 50
deselected, 31 xfailed, 0 failed, in one run (no #328 kernel flake
appeared this time); `ruff check .` and `ruff format --check` clean;
`render_catalog --check` rc 0; `check_hygiene` passed (606 files);
`check_recipe_assets` passed (461 files); fixture validator 51 recipe
folders, 0 failed; the two touched test files 36 passed, 1 skipped
(existing `/proc` skip).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
CompleteDotTech added a commit that referenced this pull request Oct 21, 2026
…or (#308) (#330)

Closes #308

## What this adds

A per-recipe image provenance manifest and its enforcement in the
fixture validator. Foundation work (no recipe touched); this defines a
file format every multimodal recipe copies, so it needs two independent
reviews.

- `recipes/<slug>/assets-manifest.json`, one per recipe, required
exactly when the recipe has files under `assets/`. Simple JSON:
`{"schema": 1, "assets": [entry, ...]}`. Each entry: `path`, `sha256`,
`format`, `width`, `height`, `origin` (`generated` only), `generator`
(`build_fixtures.py:draw_scene` or `jev_cookbook.<module>:<function>`),
`parameters` (object, seed included), `licence` (`MIT` or `CC0-1.0`),
`description`, `attestations` (`synthetic` true; `real_photo`,
`real_brand`, `real_person`, `third_party_content` false), optional
`shared_across_splits`.
- JSON Schema `src/jev_cookbook/fixtures/assets-manifest.schema.json`
(packaged; `_schema.py` gains `const` and `minimum`).
- `src/jev_cookbook/fixtures/_assets.py`, called from `validate_recipe`,
so `python -m jev_cookbook.fixtures validate <recipe>`, `--all` and
`tools/notebook_ci.py fixtures` all enforce it. Errors are `path: [rule]
message`; the rules are listed in `docs/fixtures.md` ("Image provenance
manifest", with a worked example and a `manifest_entry(recipe_dir, path,
...)` generator helper for #314).
- Docs: `docs/fixtures.md`, `docs/recipe-template.md`, one bullet in
`CONTRIBUTING.md` section 5.

## Decisions for the reviewers

1. **Location differs from the issue's proposal.** The issue proposed
`assets/manifest.json`. `tools/check_recipe_assets.py` (merged in #322)
says `assets/` holds image files only (`non-image-in-assets`), and that
file is outside this issue's paths, so the manifest sits beside the
folder as `recipes/<slug>/assets-manifest.json`. A recipe PR can edit it
(it is inside the recipe folder).
2. **`synthetic: true` attestation added** to the issue's four false
ones, so the manifest says "synthetic" explicitly as well as "no real
photo/brand/person/third party".
3. **Licence is a closed list** (`MIT`, `CC0-1.0`) because third-party
material is banned; widen it if a real need appears.
4. **fields image-path convention**: any string value in `fields` (any
depth, any key) with no whitespace that starts with `assets/` is an
image path; in a recipe that has an `assets/` folder or a manifest, a
string that ends `.png/.jpg/.jpeg` and contains `/` or a backslash is
one too. Bare file names are never image paths, and a recipe without
images only checks `assets/`-prefixed strings. A path must be written
exactly `assets/<name>.png` (forward slashes); any other spelling
(`./assets/x.png`, `C:/x.png`, `assets/../assets/x.png`) is
`[asset-path]` on every OS, judged by its text before any filesystem
call. Each valid path must equal a manifest `path` exactly (else
`[unlisted-asset]`) and pass `resolve_asset_path(root=recipe)`.
5. **Split leak** compares manifest SHA-256 of images named by
`train`/`validation`/`test` examples (demo exempt), under one path or
two. It does not decode pixels, so two encodings of one picture are two
hashes (stated in the docs). The allow-list is `shared_across_splits:
true` on every entry carrying those bytes.
6. "An image that nothing references" is not enforced: an image may be
used only by question criteria, which the validator cannot see. It must
still be listed.
7. `pyproject.toml` (package-data for the new schema file) and
`tests/test_multimodal_backends.py` / `tests/test_fixtures.py` are
outside the issue's list but needed: the schema file must ship; the
multimodal test built an image recipe with no manifest (now correctly
rejected) so it gained one; the unsupported-keyword test used `minimum`
as its example of an unsupported keyword and now uses `maximum`.
8. `docs/multimodal-track.md` now points to the manifest section
(allowed by the orchestrator after the PR opened).
9. The positive multimodal fixture is built in `tmp_path` by the tests
(Pillow-drawn images) rather than committed binaries under `tests/`.

## How it was verified

Final head e1957fc (signed merge of origin/main e299977 into edb9202,
after two fix rounds), Linux, fresh Python 3.14 venv:

- `ruff check .`: All checks passed. `ruff format --check .`: 399 files
already formatted.
- `pytest`: 5892 passed, 49 skipped, 50 deselected, 31 xfailed, 0
failed. An earlier Linux CI run failed one test,
`test_fields_path_traversal_is_an_asset_path_problem[C:/Windows/x.png]`,
because the rule depended on the OS; the `fields` path check now judges
the text before any filesystem call.
- `python tools/render_catalog.py --check`: exit 0. `python
tools/check_hygiene.py`: passed, 609 files. `python
tools/check_recipe_assets.py`: passed. `python tools/notebook_ci.py
fixtures`: 51 recipe folders, 0 failed. `python -m jev_cookbook.fixtures
validate --all`: all valid (no recipe has `assets/`, so unchanged).
- Notebook execution: no notebook or recipe changed, so none run.
- New tests in `tests/test_fixtures_assets.py` cover each rule positive
and negative: missing manifest, unlisted file, orphan entry,
hash/format/size mismatch, licence, attestations, required and unknown
fields, path shapes (a table of non-canonical spellings,
OS-independent), text-only and image recipes, unreadable manifest,
duplicate entry, split leak and allow-list, plus checks that the docs
example and helper pass the schema and validator, that the schema
patterns equal the code's, and that every schema pattern is anchored (an
unanchored pattern would accept `./assets/x.png` under a partial-match
validator).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant