Repository navigation
Foundation: image-file contract, checker, scope rule and notebook image policy (#307) - #322
Conversation
…metadata) with a checker, scope rule, JPEG output normalisation and output cap (#307) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Opus reviewer (independent review 1 of 2): Claude Opus 5.5 ( Reviewed head Summary. The design is right: a stdlib checker, a PNG chunk allowlist, JFIF-only JPEGs,
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 (
|
| 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
APP0that is exactly a 16-byte JFIF header withXthumbnail = Ythumbnail = 0,DQT,DHT,DRI(length exactly 4), oneSOF0(precision 8, 1 or 3 components),SOS,RSTnand EOI. Reject everything else asjpeg-metadata, or asjpeg-structurefor unknown and reserved markers. - PNG: check lengths. Check the length of each allowed ancillary chunk:
gAMA4,cHRM32,sRGB1,pHYs9,sBITup to 4,bKGD1/2/6 by colour type,tRNSwithin the palette size or 2/6. AllowPLTEonly for colour type 3. Decompress the IDAT stream withzlib.decompressobjand requireeofwith nounused_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/jpegoutput carries EXIF (Artist plus aC:\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'scheck_notebook_fresh.pyrejects the same pair:data.image/jpeg: committed '/9j/4AAQ…RXhpZg…', fresh '/9j/4AAQ…'.check_hygiene.pyskips 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.)
- The PR's
- Images in Markdown are not covered. A Markdown cell with
attachmentsholding 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 passescheck()(Markdown attachments are not outputs, so the cap does not count them) andcheck_hygiene.py(attachmentsis skipped,tools/check_hygiene.py:390). That is an image outsideassets/, with metadata, of any size.
What to change:
- Run the asset checker's metadata rules on every committed
image/jpegoutput (JFIF only, as in B1). - Do the same for every committed
image/pngoutput, withtEXtallowed only for the keywordSoftware. Matplotlib writes that chunk: all 134 PNG outputs onmainholdIHDR, pHYs, tEXt, IDAT, IEND. - Either reject Markdown
attachmentsanddata:image/URIs in recipe notebooks (a recipe can linkassets/x.pngby relative path), or count them towardMAX_OUTPUT_BYTESand 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.mdrule 4 anddocs/fixtures.md.
B3. The contract cannot be found from the contributor documents, and this PR makes two of them inaccurate
-
.pre-commit-config.yaml:1says it "Mirrors the CI checks (ci.ymlandhygiene.yml)", anddocs/development.md:349-350says the same. After this PRhygiene.ymlhas 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 forHygiene (secrets and notebook outputs)lists onlypython tools/check_hygiene.py. The job now also runspython 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 linkfixtures.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 noassets/. 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<svgafter 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)
- S1. Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321 consistency (280 px floor).
- The checker (280–1568, 78,400–2,458,624) matches PR Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321's new
jev_cookbook.vision.IMAGE_LIMITS(head424790b) exactly. - On
main,vision.pystill accepts 256–279. That direction is harmless (the checker is stricter), and Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321 removes it. - Whichever of Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321 and Foundation: image-file contract, checker, scope rule and notebook image policy (#307) #322 merges second should add one test that imports both and asserts
(MIN_SIDE, MAX_SIDE, MIN_PIXELS, MAX_PIXELS) == tuple(IMAGE_LIMITS[k] for k in ("min_side", "max_side", "min_area", "max_area")). pytest has the package installed even though the Hygiene job does not. Without it, the pinned-in-two-places numbers can drift silently. The currenttest_the_limits_are_the_documented_onespins literals, not the shared table. docs/vision.md:43-44("256–1568", "a 256 × 256 fixture does not") is fixed by Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321, not here.docs/multimodal-track.mdno longer quotes 256 on this head, so the PR body's note about it is out of date.
- The checker (280–1568, 78,400–2,458,624) matches PR Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321's new
- S2. Decide and state whether Adam7-interlaced and 16-bit PNGs are allowed. A 16-bit
I;16PNG passes today; this is the PNG counterpart of "baseline JPEG". Also check the SOF0 precision byte (= 8) and component count (1 or 3: no CMYK). - S3.
output_bytesruns only inside the freshness check, i.e. on notebooks the Notebook workflow executes. That is every recipe today (60 Notebook jobs on this PR), so it is fine. A one-line note innotebook-ci.mdsaying the cap is checked there, not in Hygiene, would avoid surprise. - S4.
docs/notebook-ci.mdrule 2 and rule 3 now have over-long, unwrapped lines. Cosmetic. - S5. The bad-CRC case in
tests/test_check_recipe_assets.py:259-261is not tested.corrupt[33] ^= 0xFFis commented "inside the IHDR body", but byte 33 is the first byte of the next chunk's length field. The IHDR body is bytes 16–28 and its CRC is bytes 29–32.- The file is therefore rejected as "runs past the file", not for its CRC. Replacing the CRC comparison with
if False:leaves the whole test file green. - Use e.g.
corrupt[20]and assert that the message containsbad CRC.
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, all100644. 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-onlytests/test_notebook_ci_sandbox.pysignal.SIGKILLtests (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 underrecipes/).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-classificationand24-incident-priority-composition(all have figures; the PR changes the freshness check for every notebook). Each executed (rc 0),check_notebook_fresh.pyreported "matches a fresh offline run", andcheck_hygiene.pypassed 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
acTLcheck, the non-baseline SOF check, both trailing-data raises, the total cap, the content sniff outsideassets/, the suffix-case check, theCOMcheck, the symlink/submodule mode check, JPEG inIMAGE_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.
- Killed (13): the
- Attack harness for B1/B2/B4: in-memory files fed to
check_entries, Pillowload()andvision._image_format. The notebook pair was fed to the PR's and tomain'scheck_notebook_fresh.check. Results are as quoted above.
Acceptance criteria (#307)
docs/fixtures.mdstates 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). .gitattributesadds*.jpeg binary: met.docs/recipe-template.mdgains anassets/row linkingdocs/vision.md: met.
OPUS-REVIEW: CHANGES-NEEDED 367a1e3
|
Opus reviewer (independent review 2 of 2). Model: Claude Opus 5.5 ( 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 Blocking1. When a notebook displays a
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 The cap itself is reasonable and I would keep it. What needs fixing is the guidance:
Suggestions (not blocking)
Pillow compliance (what the builder will meet)Everything a The only failures were the ones the rules intend, and each message named the file and the rule: Code review notes
Checks run (fresh venv, Python 3.14, Windows, no
|
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>
|
Opus reviewer (independent review 1 of 2, re-review): Claude Opus 5.5 ( Reviewed head 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 Status of my round-1 findings
BlockingR1. The "no image in a text cell" rule can be bypassed, and notebook/cell metadata is not covered (
|
|
Opus reviewer (independent review 2 of 2, re-review). Model: Claude Opus 5.5 ( I reviewed fix round 1 ( My earlier findings
The stricter allowlists against real builder outputMy 33 scratch images through the new checker. I got exactly the same 7 findings as before and no new failures:
A wider matrix of 173 in-memory saves with Pillow 13.0.0 and matplotlib, run through
All pass, except Notebooks executed with nbclient, then judged by
All 51 committed Suggestions (not blocking)
Mutation checksEach mutation was applied, the relevant test file run, and the source restored. The worktree was clean afterwards.
Scope and diff
Checks run on
|
…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>
|
Opus reviewer (independent review 1 of 2, re-review 2): Claude Opus 5.5 ( Reviewed head 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 Status of my round-2 findings (
|
| 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 withzlib.compress(rows)[:-4]would pin it. - S10. Cap the inflate at the expected length.
_MAX_INFLATEDis 256 MiB.expectedis known before inflating, soinflater.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, all100644(two added, 13 modified). No symlink, submodule, mode change or type change. This round changes onlytools/check_recipe_assets.py,tools/check_notebook_fresh.py, their two test files, and one sentence ofdocs/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.pysignal.SIGKILLtests (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 oftests/test_new_recipe.py(84 passed). It is a timing-sensitive kernel-restart probe under load. This PR does not touch it ortools/execute_notebook.py/tools/new_recipe.py, and CI'sTests (py3.10)andTests (py3.14)pass.
- Two failures are the known Windows-only
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_KEYof_template,01-sentiment-classificationand24-incident-priority-composition. All rc 0;check_notebook_fresh.pyreported "matches a fresh offline run" (the new metadata scan raised nothing on fresh output), andcheck_hygiene.pypassed on each fresh copy. output_problemsover 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 twoSoftwaremutants are killed bytests/test_check_recipe_assets.py::test_only_one_matplotlib_software_line_is_allowed_in_an_embedded_png.
- The tree was clean afterwards.
- Re-run unchanged:
Acceptance criteria (#307)
docs/fixtures.mdstates 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. .gitattributesadds*.jpeg binary: met.docs/recipe-template.mdhas anassets/row linkingdocs/vision.md: met.CONTRIBUTING.md,docs/development.mdand.pre-commit-config.yamlare also updated.
OPUS-REVIEW: APPROVE 171240f
|
Opus reviewer (independent review 2 of 2, re-review 2). Model: Claude Opus 5.5 ( I reviewed everything changed since What changed, and whether it is right
Ordinary output still passesMy 173-save corpus, rerun on this head with Pillow 13.0.0 and matplotlib 3.11.2:
Result: 171 pass. The other 2 are My 33-file scratch repository, through Notebooks executed with nbclient on this head, judged by
Every inline PNG in those notebooks carries exactly one Mutation checks on this headEach mutation was applied, the relevant test files run, and the source restored. The worktree was clean afterwards. All 13 were killed:
Suggestions (not blocking)
Checks run on
|
…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>
…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>
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 indocs/notebook-ci.md(staleness check rules 2-5).tools/check_recipe_assets.py(stdlib only), second step of theHygiene (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 butrecipes/<slug>/assets/is rejected (deletion allowed).tools/check_notebook_fresh.py:image/jpegis normalised likeimage/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.mdassets/row;CONTRIBUTING.md,docs/development.md,.pre-commit-config.yamlupdated.Fix round 1 (both Opus reviews on 367a1e3 were CHANGES-NEEDED)
Review 1:
jpeg-metadataorjpeg-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, ...).bad CRC in chunk IHDR(mutation of the CRC check is killed; I checked).image/pngandimage/jpegoutput (and everydata:imageURI in an output) is checked with the asset rules in both files; matplotlib's shorttEXt Softwareis 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/gifetc. outputs are problems. All notebooks on main still pass (tested)..pre-commit-config.yamlhook,docs/development.mdHygiene row and section,CONTRIBUTING.mdassets/entry; each pinned by a test.sniffnow 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.jev_cookbook.vision.IMAGE_LIMITSexists (PR Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321), to it;vision.pyuntouched.Review 2:
docs/notebook-ci.mdand thetools/check_notebook_fresh.pycomment with the measured 8.6x to 22x factors; both recommendIPython.display.Image(filename=path)andImage.thumbnail((640, 640));docs/fixtures.mdcarries the same guidance.savefig(metadata={"Software": None})documented;file-too-largemessage and doc suggest JPEG for photographic content; ICC warning corrected to PNG only; CMYK/Adobe note. Not taken: Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321 ordering (vision.md still says 256 until Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321 merges).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 WindowsSIGKILLones (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), andtest_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 useDisplayFormatter(); 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 --checkexit 0;check_hygiene.py606 files pass;check_recipe_assets.py461 files pass;notebook_ci.py fixtures51 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)
data:\s*image/case-insensitively with any parameters, base64 or not; cell metadata and notebook metadata (values, nested values and keys such asimage/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.tEXtchunk, and it must match matplotlib's own wording (Software\0Matplotlib version<N>, https://matplotlib.org/).docs/fixtures.mdsays the rules remove metadata ordinary tools write and are not a detector for deliberately hidden bytes; the output-cap failure message recommendsIPython.display.Image(filename=path).render_catalog --check0; hygiene 606 files;check_recipe_assets461 files; fixtures 51/0; fullpytest: 2 failed, 5284 passed, 58 skipped, 50 deselected, 31 xfailed. The 2 failures are the known WindowsSIGKILLtests (Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302).