Fix the seven findings from the fourth /code-review xhigh pass

The drift check's exemption keyed off the wrong thing, and let real
drift through.

`if src in unpredicted` read as "the planner claimed nothing for this
source", but it recorded "the probe failed" - and those come apart under
--webp false, where possible_output_names returns the source's own name
before it ever consults `animated`. So for any file whose probe failed,
a writer that produced a name nobody claimed got a warning and an exit 0
instead of the error the check exists to raise. The warning even said
"the de-confliction pass held no name for it" while `claimed` held one.

Rather than correct the condition, remove the case: optimize_raster now
takes the planner's `animated` answer instead of re-deriving it with its
own `getattr(img, "is_animated", False)` a few lines later. Two answers
to one question was the whole reason a source could be unpredictable
here and processable there. A source whose animation is undetermined is
now refused in optimize_raster, counted by process_file as one file's
error like any other, and never reaches the check - so the planner's
"claims nothing" is always accurate and the exemption has nothing left
to describe. Both answers already came from the same access on the same
file, so this only changes behaviour on a transient.

The per-source plan is one record (Plan: dst_rel, names, animated)
instead of a dst dict beside a parallel set - every parallel structure
this pass has grown ended up disagreeing with the one next to it, which
is what both of the last two rounds' bugs were.

Also:
- The drift message reports `plan.names`, the claim as the planner wrote
  it, not `claimed`'s casefolded keys - reporting "diagram.svg" for a
  source that claimed "Diagram.SVG" points the reader at a case mismatch
  that isn't the problem.
- The check compares with == rather than `is`: Path defines __eq__, and
  identity held only because two loops happened to share list objects.
- The uppercase test asserts the written name per case, not just that one
  file came out.

Dropped test_drift_is_refused_even_when_the_probe_failed as written: it
stubbed the probe to False - a probe that succeeded - so it demonstrated
nothing about the exemption and passed against the unfixed tree. The
regression for that hole is the --webp False half of
test_an_undetermined_probe_skips_one_file_and_no_more, which fails
against it.

206 + 33 + 189 pass; scripts/ unchanged from base. Real 299-image corpus
byte-for-byte identical, and act gives the same database md5 for the
fourth round running.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: alexmmiller

ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/optimize_media.py

@@ -296,9 +296,28 @@ def resize_animated_gif(img: Image.Image, dst: Path, max_width: int) -> Path:
     return dst
 
 
-def optimize_raster(src: Path, dst: Path, **encode_kwargs) -> Path:
+def optimize_raster(src: Path, dst: Path, *, animated: Optional[bool], **encode_kwargs) -> Path:
+    """`animated` is decided once, by _is_animated_raster during the
+    name-planning pass, and passed in rather than re-derived here.
+
+    This used to repeat the planner's `getattr(img, "is_animated", False)`
+    a few lines after the planner ran it, which is two answers to one
+    question - and the possibility of them differing is the whole reason
+    optimize_directory had to carry a set of sources it had "declined to
+    predict" and special-case them when checking what got written. Both
+    answers came from the same access on the same file, so in practice they
+    could only differ on a transient (the file changed or was locked between
+    the two opens); taking the planner's answer as authoritative removes the
+    case rather than handling it.
+
+    None means the planner could not tell. It refuses rather than guessing:
+    the planner has already recorded that this source is expected to produce
+    nothing, and process_file turns this into one file's counted error."""
+    if animated is None:
+        raise ValueError("could not determine whether this file is animated (see the earlier warning); "
+                         "skipping it rather than guessing at its output format")
     with Image.open(src) as img:
-        if getattr(img, "is_animated", False):
+        if animated:
             if src.suffix.lower() == ".gif":
                 return resize_animated_gif(img, dst, encode_kwargs["max_width"])
             # Animated non-GIF (e.g. webp): per-frame resizing/re-encoding is
@@ -382,12 +401,19 @@ def optimize_svg(src: Path, dst: Path, *, precision: int, rasterize_threshold: i
     return dst, False
 
 
-def process_file(src: Path, dst: Path, *, cfg: dict, pngquant_path: str, stats: dict, logger: Logger) -> Path:
+def process_file(src: Path, dst: Path, *, cfg: dict, pngquant_path: str, stats: dict, logger: Logger,
+                  animated: Optional[bool]) -> Path:
     """Optimizes (or copies through) one file. Returns the path actually
     written on success (which may differ from `dst` - webp conversion or
     SVG rasterization changes the extension), or None on error (already
     logged; `stats["errors"]` is incremented so callers can tell without
-    inspecting the return value)."""
+    inspecting the return value).
+
+    `animated` is required, not defaulted: it is the name-planning pass's
+    answer for this source (see optimize_raster), and a default would let a
+    caller silently skip the planning this function's output is checked
+    against. Irrelevant for SVG and passthrough files, which have no
+    animation to honour."""
     dst.parent.mkdir(parents=True, exist_ok=True)
     suffix = src.suffix.lower()
 
@@ -410,7 +436,8 @@ def process_file(src: Path, dst: Path, *, cfg: dict, pngquant_path: str, stats:
                 kind = "svg"
         elif suffix in RASTER_EXTENSIONS:
             dst_final = optimize_raster(
-                src, dst, max_width=cfg["max_width"], jpeg_quality=cfg["jpeg_quality"], webp=cfg["webp"],
+                src, dst, animated=animated,
+                max_width=cfg["max_width"], jpeg_quality=cfg["jpeg_quality"], webp=cfg["webp"],
                 webp_quality=cfg["webp_quality"], pngquant_path=pngquant_path, pngquant_speed=cfg["pngquant_speed"],
                 logger=logger,
             )
@@ -446,6 +473,19 @@ def process_file(src: Path, dst: Path, *, cfg: dict, pngquant_path: str, stats:
     return dst_final
 
 
+class Plan(NamedTuple):
+    """What the name-planning pass decided for one source: where it will be
+    written, every basename it holds against the other sources, and the
+    animation answer process_file is to use. One record rather than a dict
+    per field - the three are decided together, in one loop, and every
+    parallel structure this pass has grown so far ended up disagreeing with
+    the one beside it."""
+
+    dst_rel: Path
+    names: set
+    animated: Optional[bool]
+
+
 class OptimizeResult(NamedTuple):
     """What optimize_directory did.
 
@@ -666,18 +706,13 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
         deduped.append(src)
     sources = deduped
 
-    dst_rel_for = {}
+    plan_for = {}
     # Keyed casefolded: the work dir is written on whatever filesystem the run
     # happens to use, and on a case-insensitive one (APFS, NTFS) "logo.PNG" and
     # "logo.png" are the same file - so comparing the names as written would
     # miss a collision that still costs an image. insert_optimized_media's own
     # seen_names guard is case-sensitive too and would not catch it either.
     claimed = {}
-    # Sources whose animation could not be determined, and which therefore
-    # claimed no names at all (see possible_output_names). Kept so the check
-    # below can tell "the planner and the writer disagree" - a bug - from
-    # "the planner declined to predict this one" - one file's bad luck.
-    unpredicted = set()
     for src in sources:
         rel = src.relative_to(input_dir)
         stem, suffix = rel.stem, rel.suffix
@@ -687,8 +722,6 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
         # cannot carry a second frame is never animated, and opening every
         # JPEG to learn that would cost an extra pass over the whole corpus.
         animated = _is_animated_raster(src, logger) if suffix.lower() in ANIMATABLE_EXTENSIONS else False
-        if animated is None:
-            unpredicted.add(src)
         candidate = stem
         attempt = 0
         while True:
@@ -706,14 +739,16 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
                 )
         for name in names:
             claimed[name.lower()] = src
-        dst_rel_for[src] = rel.parent / f"{candidate}{suffix}"
+        plan_for[src] = Plan(dst_rel=rel.parent / f"{candidate}{suffix}", names=names, animated=animated)
 
     renamed = {}
     written = []
     for src in sources:
         rel = src.relative_to(input_dir)
-        dst = output_dir / dst_rel_for[src]
-        dst_final = process_file(src, dst, cfg=cfg, pngquant_path=pngquant_path, stats=stats, logger=logger)
+        plan = plan_for[src]
+        dst = output_dir / plan.dst_rel
+        dst_final = process_file(src, dst, cfg=cfg, pngquant_path=pngquant_path, stats=stats, logger=logger,
+                                  animated=plan.animated)
         if dst_final is None:
             continue
         # The name this actually wrote has to be one the planning pass held
@@ -728,43 +763,39 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
         # a name written but never claimed, free to overwrite another source's
         # output. Nothing connected the two halves, so nothing noticed.
         #
-        # This is that connection. It costs one dict lookup per file and turns
-        # a future drift into a named failure instead of a corrupted image
+        # This is that connection. It costs one lookup per file and turns a
+        # future drift into a named failure instead of a corrupted image
         # corpus that still exits 0.
         #
-        # Asked of `claimed` rather than of a second per-source dict, and so
-        # casefolded like every other lookup in this pass. De-confliction
-        # guarantees no two sources hold the same lowercased name, so "is this
-        # the source that claimed this name" is exactly `claimed[name] is src`.
-        # Comparing exact case instead made the check stricter than the
-        # invariant it protects, and turned an SVG spelled ".SVG" - which the
-        # planner claimed as ".svg" - into an aborted run.
-        if claimed.get(dst_final.name.lower()) is not src:
-            held = sorted(name for name, owner in claimed.items() if owner is src)
-            if src in unpredicted:
-                # Not drift: the planner declined to predict this one because
-                # its animation could not be determined, expecting
-                # optimize_raster to fail on the same access. It didn't. The
-                # file itself is fine - it is kept rather than dropped, since
-                # discarding a successfully optimized image is the worse
-                # outcome and insert_optimized_media's seen_names guard still
-                # catches a flat-namespace collision downstream - but the
-                # planning pass never de-conflicted it, so say so.
-                logger.error(
-                    f"warning: {src} produced {dst_final.name} after its animation could not be determined, "
-                    "so the de-confliction pass held no name for it; keeping the file, but it was not "
-                    "checked against the other sources' outputs"
-                )
-            else:
-                # Genuine drift. ValueError, not RuntimeError: that is what
-                # insert_optimized_media's `except ValueError` around this call
-                # turns into a clean "error: ..." line in the log file, the
-                # same as this function's input_dir == output_dir refusal.
-                raise ValueError(
-                    f"{src} wrote {dst_final.name}, which possible_output_names did not predict "
-                    f"(it claimed {held or 'nothing'}). The output-name planning pass and process_file "
-                    "have drifted apart - see possible_output_names."
-                )
+        # Asked of `claimed`, and so casefolded like every other lookup in
+        # this pass: de-confliction guarantees no two sources hold the same
+        # lowercased name, so "is this the source that claimed this name" is
+        # exactly `claimed[name] == src`. Comparing exact case instead made
+        # the check stricter than the invariant it protects, and turned an SVG
+        # spelled ".SVG" - which the planner claimed as ".svg" - into an
+        # aborted run.
+        #
+        # No exemption for a source the planner could not predict: it does not
+        # arise any more. optimize_raster takes the planner's answer instead
+        # of re-deriving it, so a source whose animation was undetermined
+        # raises there, is counted as one file's error, and never reaches
+        # here. The exemption that used to sit in this branch keyed off "the
+        # probe failed" rather than "the planner claimed nothing" - which
+        # differ under --webp false, where the planner claims the source's own
+        # name regardless - and so waved genuine drift through as a warning.
+        if claimed.get(dst_final.name.lower()) != src:
+            # ValueError, not RuntimeError: that is what insert_optimized_media's
+            # `except ValueError` around this call turns into a clean
+            # "error: ..." line, the same as the input_dir == output_dir
+            # refusal above. `plan.names` rather than `claimed`'s keys, so the
+            # message shows the claim as the planner wrote it - reporting a
+            # casefolded "diagram.svg" for a source that claimed "Diagram.SVG"
+            # points the reader at a case mismatch that isn't the problem.
+            raise ValueError(
+                f"{src} wrote {dst_final.name}, which possible_output_names did not predict "
+                f"(it claimed {sorted(plan.names) or 'nothing'}). The output-name planning pass and "
+                "process_file have drifted apart - see possible_output_names."
+            )
         written.append(dst_final)
         rel_final = dst_final.relative_to(output_dir)
         if rel_final != rel:

ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/tests/test_review_findings.py

@@ -997,9 +997,15 @@ def test_the_probe_reports_its_own_failure(tmp_path, capsys):
 
 # --- the written-vs-claimed check must not fire on ordinary inputs ----------
 
-@pytest.mark.parametrize("name", ["Diagram.SVG", "Diagram.Svg", "Photo.PNG", "Photo.JPG", "Notes.TXT"])
+@pytest.mark.parametrize("name, expected_webp, expected_plain", [
+    ("Diagram.SVG", "Diagram.SVG", "Diagram.SVG"),   # small enough not to rasterize
+    ("Diagram.Svg", "Diagram.Svg", "Diagram.Svg"),
+    ("Photo.PNG", "Photo.webp", "Photo.PNG"),
+    ("Photo.JPG", "Photo.webp", "Photo.JPG"),
+    ("Notes.TXT", "Notes.TXT", "Notes.TXT"),         # passthrough copy
+])
 @pytest.mark.parametrize("webp", [True, False])
-def test_an_uppercase_extension_is_not_mistaken_for_drift(tmp_path, name, webp):
+def test_an_uppercase_extension_is_not_mistaken_for_drift(tmp_path, name, expected_webp, expected_plain, webp):
     """possible_output_names' SVG branch returned the lowercase SVG_EXTENSION
     literal while optimize_svg writes `dst`, which carries the source's own
     spelling - so "Diagram.SVG" was claimed as "Diagram.svg". Harmless while
@@ -1019,7 +1025,10 @@ def test_an_uppercase_extension_is_not_mistaken_for_drift(tmp_path, name, webp):
                                     pngquant_path=om.find_pngquant(), logger=om.Logger(sys.stdout),
                                     stats=_new_stats())
 
-    assert len(result.written) == 1, "the file was optimized, so it must survive the check"
+    # The name matters, not just the count: the bug was a claim spelled
+    # ".svg" for a file written as ".SVG", and asserting only that one file
+    # came out would pass just as happily if the output had been lowercased.
+    assert [p.name for p in result.written] == [expected_webp if webp else expected_plain]
 
 
 def test_the_svg_branch_claims_the_source_s_own_spelling():
@@ -1029,21 +1038,49 @@ def test_the_svg_branch_claims_the_source_s_own_spelling():
     assert om.possible_output_names("d", ".svg", cfg) == {"d.svg", "d.webp"}
 
 
-def test_an_unpredicted_source_that_does_write_is_kept_not_fatal(tmp_path, monkeypatch, capsys):
-    """A probe that fails where optimize_raster then succeeds is one file's
-    bad luck, not drift between the planner and the writer. This module's
-    rule is that a single file's problem is reported and the run continues -
-    a dangling symlink killing the whole run was fixed as a bug in this same
-    PR - so the check must not reintroduce that shape here."""
+@pytest.mark.parametrize("webp", [True, False])
+def test_an_undetermined_probe_skips_one_file_and_no_more(tmp_path, monkeypatch, capsys, webp):
+    """optimize_raster takes the planner's answer rather than re-deriving it,
+    so a source whose animation could not be determined is refused there,
+    counted as one file's error, and never reaches the written-vs-claimed
+    check. The run continues - this module's rule is that a single file's
+    problem is reported and the rest of the tree still processes.
+
+    The --webp False half is the case the old exemption got wrong. The
+    planner claims the source's own name there regardless of `animated` (that
+    branch returns before consulting it), so "the probe failed" and "the
+    planner claimed nothing" came apart - and the exemption, keyed on the
+    former, waved genuine drift through as a warning."""
     src, out = tmp_path / "in", tmp_path / "out"
     src.mkdir(), out.mkdir()
     Image.new("RGB", (20, 20), (5, 5, 5)).save(src / "photo.png")
+    Image.new("RGB", (20, 20), (6, 6, 6)).save(src / "other.jpg")
     monkeypatch.setattr(om, "_is_animated_raster", lambda source, logger=None: None)
 
     stats = _new_stats()
-    result = om.optimize_directory(src, out, cfg=dict(om.BUILTIN_DEFAULTS) | {"webp": True},
+    result = om.optimize_directory(src, out, cfg=dict(om.BUILTIN_DEFAULTS) | {"webp": webp},
                                     pngquant_path=om.find_pngquant(), logger=om.Logger(sys.stdout),
                                     stats=stats)
 
-    assert [p.name for p in result.written] == ["photo.webp"], "a good file is kept, not discarded"
-    assert "the de-confliction pass held no name for it" in capsys.readouterr().out
+    assert stats["errors"] == 1, "the undetermined file, and only it"
+    assert [p.name for p in result.written] == ["other.webp" if webp else "other.jpg"]
+    assert "could not determine whether this file is animated" in capsys.readouterr().out
+
+
+def test_the_drift_message_names_the_claim_as_the_planner_wrote_it(tmp_path, monkeypatch):
+    """Built from `claimed`'s keys, the message reported a casefolded
+    "diagram.svg" for a source that claimed "Diagram.SVG" - pointing whoever
+    reads it at a case mismatch that is not the problem."""
+    src, out = tmp_path / "in", tmp_path / "out"
+    src.mkdir(), out.mkdir()
+    (src / "Diagram.SVG").write_text('<svg xmlns="http://www.w3.org/2000/svg" width="10" height="10">'
+                                     '<rect width="10" height="10"/></svg>')
+    real_process_file = om.process_file
+    def drifting_process_file(source, dst, **kwargs):
+        written = real_process_file(source, dst, **kwargs)
+        return written.with_suffix(".avif") if written else written
+    monkeypatch.setattr(om, "process_file", drifting_process_file)
+
+    with pytest.raises(ValueError, match=r"it claimed \['Diagram\.SVG', 'Diagram\.png'\]"):
+        om.optimize_directory(src, out, cfg=dict(om.BUILTIN_DEFAULTS), pngquant_path=om.find_pngquant(),
+                               logger=om.Logger(sys.stdout), stats=_new_stats())