Fix the eight findings from the second /code-review xhigh pass

Two of these are misses in the previous commit's own fixes.

- The act check moved above the argument *validation* rather than below
  it, so five semantic errors (both --skip flags, --db-path missing or
  nonexistent, --images-zip-path missing or nonexistent) were still
  answered with "act is required" on a machine without act. It now sits
  immediately before the act invocation; all five report themselves, and
  act is still required once the arguments are good.
- possible_output_names' undetermined case claimed both names, which was
  widened from GIFs to every PNG and TIFF by the last commit. It now
  claims nothing: optimize_raster repeats the same Image.open and
  is_animated access, so whatever makes the probe fail makes process_file
  fail too, and the source writes no file at all. Holding two names for
  it de-conflicted a real image against an output that never appears.

And the deeper one behind both of the last two rounds' F26-shaped bugs:
optimize_directory now checks that the name process_file actually wrote
was one the planning pass claimed for that source, and fails loudly
otherwise. The prediction lives in a second place from the code it
predicts and has drifted twice, silently both times; this is the missing
connection between the halves.

Also:
- _is_animated_raster reports a probe failure through the logger instead
  of swallowing it - it runs for every PNG now, and a systematic failure
  would otherwise shift the whole de-confliction pass unexplained.
- outside_fences' `source` is required, not defaulted: the warning is the
  entire point of that change, and an optional argument is how a later
  call site quietly gets the old silence back.
- The unterminated-fence check runs once on spans[-1]. Only the last span
  can be unterminated (fenced_spans appends the run-to-EOF one after its
  loop), so checking every span re-split every code block for nothing.
- `animated: Optional[bool]`, and a non-bool argument is refused - the
  parameter took a Path one revision ago, and a Path is truthy, so a
  stale call reported "animated" for every file with no error.
- The case-handling test asserts the property rather than the
  "{stem}-{ext}" naming literal.

6 new tests, and every changed behaviour fails against the previous
commit. 192 + 33 + 189 pass; scripts/ unchanged from base. The real
299-image corpus is byte-for-byte identical again, and the local
workflow under act produces the same database md5 as the last two
rounds - with the new written-vs-claimed check silent across every path
the corpus exercises, including SVG rasterization.

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

Author: alexmmiller

ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/find_missing_assets.py

@@ -61,7 +61,7 @@ def is_unterminated(span_text: str) -> bool:
     return sum(1 for line in span_text.splitlines() if FENCE_LINE_RE.match(line.lstrip())) < 2
 
 
-def outside_fences(text: str, source: str = None) -> str:
+def outside_fences(text: str, source: str) -> str:
     """`text` with every fenced code block removed, so the <include> scan below
     doesn't report a sample as a broken reference.
 
@@ -74,21 +74,29 @@ def outside_fences(text: str, source: str = None) -> str:
     exposed the rest of the block. fenced_spans handles both, and is already
     what extract_title trusts to stay out of code samples.
 
-    `source` names the file in the warning an unterminated fence earns. That
-    warning is the point: the old regex needed a *closing* fence to match
-    anything, so an unpaired one left the rest of the file scannable, where
-    this correctly treats it as one long code block and stops checking. That
-    is a false negative in a report whose value is catching what's missing,
-    so it has to be said out loud rather than inferred from a short report."""
+    `source` names the file in the warning an unterminated fence earns, and is
+    required rather than defaulting to None. That warning is the point: the
+    old regex needed a *closing* fence to match anything, so an unpaired one
+    left the rest of the file scannable, where this correctly treats it as one
+    long code block and stops checking. That is a false negative in a report
+    whose value is catching what's missing, so it has to be said out loud
+    rather than inferred from a short report - and an optional argument is
+    exactly how a later call site would drop it and quietly get the old
+    silence back."""
     spans = fenced_spans(text)
     if not spans:
         return text
+    # Only the last span can be unterminated: fenced_spans appends a closed
+    # block as it meets its closing fence, and the run-to-end-of-document span
+    # only after the loop ("if open_at is not None"). Checking every span
+    # re-split every code block in the file to ask a question already answered
+    # False for all but one of them - and obscured that invariant.
+    if is_unterminated(text[spans[-1][0]:spans[-1][1]]):
+        print(f"warning: {source} has an unterminated code fence; everything after it is being read as "
+              "code, so any <include> below it is not being checked", file=sys.stderr)
     parts = []
     pos = 0
     for start, end in spans:
-        if source and is_unterminated(text[start:end]):
-            print(f"warning: {source} has an unterminated code fence; everything after it is being read as "
-                  "code, so any <include> below it is not being checked", file=sys.stderr)
         parts.append(text[pos:start])
         pos = end
     parts.append(text[pos:])

ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/optimize_media.py

@@ -53,7 +53,7 @@
 import subprocess
 import sys
 from pathlib import Path
-from typing import NamedTuple
+from typing import NamedTuple, Optional
 
 from PIL import Image
 
@@ -462,7 +462,7 @@ class OptimizeResult(NamedTuple):
     written: list
 
 
-def possible_output_names(stem: str, suffix: str, cfg: dict, animated: bool = None) -> set:
+def possible_output_names(stem: str, suffix: str, cfg: dict, animated: Optional[bool] = None) -> set:
     """Every basename process_file could write for a source named
     `stem + suffix`.
 
@@ -486,37 +486,68 @@ def possible_output_names(stem: str, suffix: str, cfg: dict, animated: bool = No
     the collision this pre-pass exists to prevent, reached through the single
     output name the suffix alone cannot determine.
 
-    Pass True/False from _is_animated_raster. None means "not determined"
-    (not probed, or the file could not be read), and a multi-frame-capable
-    extension then claims both names, safe in the same way the SVG branch is.
+    Pass True/False from _is_animated_raster. None means "could not be
+    determined", and a multi-frame-capable extension then claims *nothing*
+    rather than claiming both.
+
+    That is the opposite of the SVG branch's conservatism, and deliberately
+    so: the two ambiguities are different. An SVG genuinely might write
+    either name, so both have to be held. A raster whose animation could not
+    be determined is one optimize_raster is about to fail on for the same
+    reason - it repeats this exact `Image.open` plus `is_animated` access, so
+    whatever raised here raises there, process_file counts an error and
+    returns None, and the source writes no file at all. Holding two names for
+    it would de-conflict an unrelated source against an output that is never
+    produced, renaming a real image and rewriting every stored URL that
+    pointed at it. Claiming nothing is the accurate prediction; if it is ever
+    wrong, optimize_directory's written-vs-claimed check says so out loud.
+
     Taking the answer rather than the path keeps this function free of I/O:
     it is called once per de-confliction attempt, and re-opening the source
     on each attempt bought nothing - the answer is a property of the file,
     not of the candidate name being tried."""
+    if animated is not None and not isinstance(animated, bool):
+        # This parameter took a Path one revision ago. A Path is truthy and is
+        # not None, so a call site left on the old signature would silently
+        # report "animated" for every file - no conversion, no error, and
+        # nothing in the output to suggest why.
+        raise TypeError(f"animated must be True, False or None, not {type(animated).__name__}")
     if suffix.lower() == SVG_EXTENSION:
         rasterized = ".webp" if cfg["webp"] else ".png"
         return {f"{stem}{SVG_EXTENSION}", f"{stem}{rasterized}"}
     if suffix.lower() in RASTER_EXTENSIONS:
         if not cfg["webp"]:
             return {f"{stem}{suffix}"}
         if suffix.lower() in ANIMATABLE_EXTENSIONS:
-            if animated is None:  # undetermined: claim both rather than guess
-                return {f"{stem}{suffix}", f"{stem}.webp"}
+            if animated is None:  # undetermined: this source writes nothing
+                return set()
             return {f"{stem}{suffix}"} if animated else {f"{stem}.webp"}
         return {f"{stem}.webp"}
     return {f"{stem}{suffix}"}  # passthrough copy, extension never changes
 
 
-def _is_animated_raster(src: Path):
+def _is_animated_raster(src: Path, logger: Logger = None) -> Optional[bool]:
     """True/False for a readable raster, or None when it can't be determined.
-    The same `is_animated` test optimize_raster makes later, just made early
-    enough for the name-planning pass to predict the output extension."""
+    The same `Image.open` + `is_animated` test optimize_raster makes later,
+    just made early enough for the name-planning pass to predict the output
+    extension.
+
+    A failure here is reported rather than swallowed. It is not fatal -
+    process_file repeats the same access and counts the error properly - but
+    it does change what this source claims (see possible_output_names), and
+    it now runs for every PNG and TIFF in the tree rather than only the
+    handful of GIFs. A systematic probe failure - a broken Pillow plugin, an
+    unreadable work tree - would otherwise shift the whole de-confliction
+    pass with nothing in the log to explain the renames that follow."""
     if src is None:
         return None
     try:
         with Image.open(src) as img:
             return bool(getattr(img, "is_animated", False))
-    except Exception:  # noqa: BLE001 - unreadable here is not fatal; process_file reports it
+    except Exception as exc:  # noqa: BLE001 - one file's problem, not the run's
+        if logger is not None:
+            logger.error(f"warning: could not determine whether {src} is animated ({exc}); "
+                         "planning for it to produce no output")
         return None
 
 
@@ -635,6 +666,7 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
     # 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 = {}
+    claimed_for = {}
     for src in sources:
         rel = src.relative_to(input_dir)
         stem, suffix = rel.stem, rel.suffix
@@ -643,7 +675,7 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
         # attempt was pure waste. False without a probe - an extension that
         # 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) if suffix.lower() in ANIMATABLE_EXTENSIONS else False
+        animated = _is_animated_raster(src, logger) if suffix.lower() in ANIMATABLE_EXTENSIONS else False
         candidate = stem
         attempt = 0
         while True:
@@ -661,6 +693,7 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
                 )
         for name in names:
             claimed[name.lower()] = src
+        claimed_for[src] = names
         dst_rel_for[src] = rel.parent / f"{candidate}{suffix}"
 
     renamed = {}
@@ -671,6 +704,27 @@ def optimize_directory(input_dir: Path, output_dir: Path, *, cfg: dict, pngquant
         dst_final = process_file(src, dst, cfg=cfg, pngquant_path=pngquant_path, stats=stats, logger=logger)
         if dst_final is None:
             continue
+        # The name this actually wrote has to be one the planning pass held
+        # for it. possible_output_names predicts what process_file ->
+        # optimize_raster/optimize_svg -> encode_raster will do, in a second
+        # place, and that prediction has already drifted twice: it said .webp
+        # for animated GIFs while optimize_raster wrote .gif, and then again
+        # for animated PNG/TIFF once the first fix special-cased .gif rather
+        # than the branch optimize_raster really takes. Both times the symptom
+        # was silent - a name claimed but never written, an unrelated image
+        # de-conflicted against the phantom and its stored URLs rewritten, or
+        # 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 set lookup per file and turns
+        # every future drift into an immediate, named failure instead of a
+        # corrupted image corpus that still exits 0.
+        if dst_final.name not in claimed_for[src]:
+            raise RuntimeError(
+                f"internal error: {src} wrote {dst_final.name}, which possible_output_names did not predict "
+                f"(it claimed {sorted(claimed_for[src]) 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

@@ -773,7 +773,7 @@ def test_includes_inside_any_fence_are_not_scanned(source, expected):
     exposed the rest of the block. Both warned about files nobody meant to
     ship. md_to_json.fenced_spans - which extract_title already relies on -
     knows both fence characters and the "at least as long" close rule."""
-    assert INCLUDE_RE.findall(outside_fences(source)) == expected
+    assert INCLUDE_RE.findall(outside_fences(source, "topics/x.md")) == expected
 
 
 # =============================================================================
@@ -815,8 +815,13 @@ def test_names_differing_only_by_case_are_both_kept(tmp_path):
     written = sorted(p.name for p in result.written)
     assert len(written) == 2, "neither source may be dropped - both are referenceable"
     assert len(set(n.lower() for n in written)) == 2, "and they must not collide once flattened"
-    # The de-conflicted one is reported, so stored URLs follow it.
-    assert result.renamed == {"b/logo.png": "b/logo-png.png"}
+    # The de-conflicted one is reported, so stored URLs follow it. Asserted as
+    # a property rather than against the "{stem}-{ext}" literal: the naming
+    # scheme is not what this test is about, and pinning it here would make a
+    # rename of that convention look like a case-handling regression.
+    assert list(result.renamed) == ["b/logo.png"], "only the second, de-conflicted source moved"
+    new_name = Path(result.renamed["b/logo.png"]).name
+    assert new_name in written and new_name.lower() != "logo.png"
 
 
 def test_identical_basenames_are_still_skipped(tmp_path):
@@ -864,7 +869,8 @@ def test_animated_png_is_not_predicted_as_webp(tmp_path):
 @pytest.mark.parametrize("suffix, animated, expected", [
     (".gif", True, {"x.gif"}),
     (".gif", False, {"x.webp"}),
-    (".gif", None, {"x.gif", "x.webp"}),
+    (".gif", None, set()),            # undetermined: optimize_raster fails too, so nothing is written
+    (".png", None, set()),
     (".png", True, {"x.png"}),
     (".png", False, {"x.webp"}),
     (".tiff", True, {"x.tiff"}),
@@ -921,3 +927,66 @@ def test_repair_refuses_half_a_scan(tmp_path):
         assert rmf.repair(conn, [], [])["chains_renumbered"] == 0  # both: uses them
     finally:
         conn.close()
+
+
+# --- what process_file writes has to be what the planner claimed ------------
+
+def test_a_write_the_planner_did_not_predict_is_refused(tmp_path, monkeypatch):
+    """possible_output_names predicts, in a second place, what process_file ->
+    optimize_raster -> encode_raster will do, and that prediction has drifted
+    twice. Nothing connected the two halves, so both drifts were silent: a
+    name claimed but never written de-conflicts an unrelated image and
+    rewrites its stored URLs, and a name written but never claimed is free to
+    overwrite another source's output."""
+    src, out = tmp_path / "in", tmp_path / "out"
+    src.mkdir(), out.mkdir()
+    Image.new("RGB", (20, 20), (1, 1, 1)).save(src / "photo.png")
+
+    # Stand in for a future encoder change the planner doesn't know about.
+    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(RuntimeError, match="possible_output_names did not predict"):
+        om.optimize_directory(src, out, cfg=dict(om.BUILTIN_DEFAULTS), pngquant_path=om.find_pngquant(),
+                               logger=om.Logger(sys.stdout), stats=_new_stats())
+
+
+def test_an_unprobeable_source_claims_no_names(tmp_path, capsys):
+    """A raster whose animation can't be determined is one optimize_raster is
+    about to fail on for the same reason, so it writes nothing and must hold
+    nothing. Claiming both names instead de-conflicted a perfectly good
+    unrelated image against an output that never appears - renaming it, and
+    rewriting every stored URL that pointed at it."""
+    src, out = tmp_path / "in", tmp_path / "out"
+    src.mkdir(), out.mkdir()
+    (src / "broken.png").write_bytes(b"not a png at all")
+    Image.new("RGB", (20, 20), (2, 2, 2)).save(src / "broken.jpg")
+
+    cfg = dict(om.BUILTIN_DEFAULTS) | {"webp": True}
+    stats = _new_stats()
+    result = om.optimize_directory(src, out, cfg=cfg, pngquant_path=om.find_pngquant(),
+                                    logger=om.Logger(sys.stdout), stats=stats)
+
+    # broken.jpg keeps its own stem: nothing real ever claimed broken.webp.
+    assert [p.name for p in result.written] == ["broken.webp"]
+    assert result.renamed == {"broken.jpg": "broken.webp"}
+    assert stats["errors"] == 1, "the unreadable file is still counted as one file's error"
+    # And the probe said why, rather than swallowing it.
+    assert "could not determine whether" in capsys.readouterr().out
+
+
+def test_the_probe_reports_its_own_failure(tmp_path, capsys):
+    """_is_animated_raster now runs for every PNG and TIFF in the tree, not
+    just the handful of GIFs, so a systematic failure would shift the whole
+    de-confliction pass. Swallowing it left no way to find out why."""
+    bad = tmp_path / "corrupt.png"
+    bad.write_bytes(b"still not a png")
+
+    assert om._is_animated_raster(bad, om.Logger(sys.stdout)) is None
+    assert "could not determine whether" in capsys.readouterr().out
+    # Silent without a logger, so a standalone caller isn't forced to have one.
+    assert om._is_animated_raster(bad) is None
+    assert capsys.readouterr().out == ""

run-build-kotlin-docs-with-act.sh

@@ -126,16 +126,6 @@ while [ $# -gt 0 ]; do
   esac
 done
 
-# Checked here rather than as this script's first act: a mistyped flag should
-# be reported as a mistyped flag on any machine, not shadowed by "act is
-# required" on the machines that don't have it. Nothing above this line needs
-# act, and the argument checks below are pure string/path validation - the
-# first thing that actually uses it is the `act` invocation at the end.
-if ! command -v act >/dev/null 2>&1; then
-  echo "error: act is required - see https://github.com/nektos/act#installation" >&2
-  exit 1
-fi
-
 if [ "$SKIP_WEBSITE_DOCS" = "true" ] && [ "$SKIP_STDLIB_DOCS" = "true" ]; then
   echo "error: --skip-website-docs and --skip-stdlib-docs together skip every step that" >&2
   echo "error: changes the database, leaving nothing for the run to do." >&2
@@ -187,6 +177,18 @@ if [ "$SKIP_WEBSITE_DOCS" != "true" ]; then
   WORKFLOW_IMAGES_ZIP_PATH="$CONTAINER_IMAGES_ZIP_PATH"
 fi
 
+# Last, after every argument check above, and immediately before the only
+# thing that needs it. Anything earlier shadows a real complaint about the
+# arguments with "act is required" on the machines that don't have act -
+# which is most of them, including CI. Moving it off line 1 fixed that for
+# the parse errors; it has to sit below the semantic checks too, or a missing
+# --db-path or a --skip-website-docs/--skip-stdlib-docs pair still gets
+# answered with the wrong message.
+if ! command -v act >/dev/null 2>&1; then
+  echo "error: act is required - see https://github.com/nektos/act#installation" >&2
+  exit 1
+fi
+
 if [ "$DRY_RUN" = "true" ]; then
   echo "note: dry_run=true - '$DB_PATH' will NOT be modified; the built database is" >&2
   echo "note: written to '$OUTPUT_DIR' only. Both Slack notifications still fire if" >&2