Merge pull request 'fix(gif): stop making agents discover the API by failing (closes mort#1522)' (#4) from fix/1522-gifsmith-api-discoverable into main

Closes mort#1522.

23% of all code_exec calls across two production runs (9 of 39) failed on
gifsmith API misuse, hitting 7 of 14 workers. The recovery was always the same
shape: another round trip spent printing gifsmith's own source — or, in one
case, running `dir(gifsmith)` — to learn what the module contains.

Three distinct causes, three fixes.

**Unguessable re-exports → `__all__`, plus the two that were missing.** The
module exported SOME matplotlib artists and not others, and the only way to find
out which was to fail: two workers wrote `from gifsmith import PathPatch` and
got an ImportError while `FancyBboxPatch` beside it worked. `PathPatch` and
`Path` are now exported, on the rule "the drawing vocabulary, whole". `__all__`
states the surface — and it also fixes what made self-service expensive:
`dir(gifsmith)` returned 24 names of which nine were incidental imports (glob,
os, shutil, subprocess, sys, traceback...), several being things a scene author
should never touch. SKILL.md now carries the same list, so the question should
not arise.

**Near-miss constructor kwargs → an error that names the miss.** `background=`
is accepted as an alias for `bg` (it is the obvious synonym, and an agent
reached for it). `force=` is rejected with the reason it was tempting —
`render(force="mp4")` really does take it, which makes it read like an
Animation-level setting. Anything else gets a difflib near-match and the valid
list. Both failures were one round trip each, and both are the kind a message
fixes for free.

**Scenes fail at render time → dry-run every scene at t=0 first.** A NameError
in scene 4 used to surface only after scenes 0-3 were fully rendered, and with
a streaming pipe open, after frames had gone into the encoder. Two workers lost
a whole render each to `RuntimeError: every scene failed`, learning one broken
name per attempt. One frame per scene now reports EVERY broken scene in a single
pass, before any scratch dir or ffmpeg pipe exists. Per-scene isolation is
preserved exactly: a scene that fails the dry run is marked and skipped rather
than re-run to fail identically, the others still ship, and only an
all-scenes-broken program raises early — which is precisely the case that was
paying for a full setup to learn nothing.

Verified against the production failures: `Animation(background=…)` now works,
`Animation(force=…)` explains itself, `Animation(backgrnd=…)` suggests
`background`, every name in `__all__` resolves, no incidental import leaks into
it, and the dry run marks exactly the broken scenes while leaving the good ones
untouched.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

gif/SKILL.md

@@ -76,9 +76,18 @@ bottom of this skill) as `{"name": "gifsmith.py", "file_id": "<that id>"}`, then
 write ONE `code_exec` call:
 
 ```python
-# gifsmith re-exports the drawing vocabulary, so this one import is enough:
+# gifsmith re-exports the drawing vocabulary, so this one import is enough.
+# The COMPLETE importable list (there is nothing else — don't guess, and don't
+# spend a call on dir(gifsmith)):
+#   Animation
+#   Arc Circle Ellipse FancyBboxPatch PathPatch Path Polygon Rectangle Wedge
+#   Image ImageDraw ImageFont          (PIL, for mode="pil")
+#   np plt                             (numpy / pyplot, for anything else)
+#   GIF_MAX_SECONDS GIF_MAX_BYTES
 from gifsmith import Animation, Circle, Ellipse, Rectangle, Polygon, Wedge, np
 anim = Animation(width=640, height=480, fps=15)     # canvas locked here
+# Animation() takes ONLY width, height, fps, dpi, bg (background= also works).
+# Everything about the output — format, size, audio — is set on render().
 
 # ---- cast: defined ONCE, called from every scene ----
 def draw_steve(ax, x, y, rage=0.0):
@@ -149,9 +158,16 @@ gifsicle are already in the sandbox.
 ## 5. Self-critique (AT MOST 3 render passes)
 
 Before delivering, LOOK at what you made: call `image_describe` on the
-**`still.png` file_id** — the harness already built it as a labelled contact
+**`still.png` file_id** — the harness already built it as a contact sheet of
-sheet, one tile per scene. Ask specifically about subject size, caption
+frames sampled evenly ACROSS the animation, each tile labelled with its `t`
-readability and whether the depicted action matches the request. Then answer:
+(0.00 = first frame). Ask specifically about subject size, caption readability
+and whether the depicted action matches the request.
+
+**A thing missing from one tile is not a thing missing from the animation.**
+The tiles are time points, so a transient — a tear rolling, a blink, a wipe —
+appears in some and not others. If the critique says something is absent, check
+the other tiles for it before re-rendering; that mistake once cost a worker
+three full re-renders chasing a tear that was there all along. Then answer:
 
 1. Does it depict what was asked — every named character, prop and action?
 2. Is the subject **big enough to read** (≥ ⅓ frame height)?

gif/scripts/gifsmith.py

@@ -37,6 +37,7 @@ fix just that scene. It also writes /workspace/still.png: a labelled contact
 sheet, one tile per scene, for the critique pass.
 """
 
+import difflib
 import glob
 import math
 import os
@@ -52,10 +53,34 @@ from PIL import Image, ImageDraw, ImageFont  # noqa: E402
 
 # Re-exported so one import line covers the whole drawing vocabulary — a missing
 # `from matplotlib.patches import ...` is otherwise a NameError that costs a pass.
+#
+# PathPatch and Path are here because agents assumed they already were: two
+# separate workers wrote `from gifsmith import PathPatch` and got an ImportError,
+# because the module exported SOME matplotlib artists and not others with no way
+# to tell which from outside. The rule now is "the drawing vocabulary, whole".
 from matplotlib.patches import (  # noqa: E402,F401
-    Arc, Circle, Ellipse, FancyBboxPatch, Polygon, Rectangle, Wedge)
+    Arc, Circle, Ellipse, FancyBboxPatch, PathPatch, Polygon, Rectangle, Wedge)
+from matplotlib.path import Path  # noqa: E402,F401
 import numpy as np  # noqa: E402,F401
 
+# The public surface, stated rather than discovered. Without this, `dir(gifsmith)`
+# returned 24 names of which nine were incidental imports (glob, os, shutil,
+# subprocess, sys, traceback...) — several of them things a scene author should
+# never touch — and a worker spent a whole code_exec call running exactly that
+# to find out what it could import. Keep in sync when adding a re-export.
+__all__ = [
+    "Animation",
+    # matplotlib artists (mode="mpl")
+    "Arc", "Circle", "Ellipse", "FancyBboxPatch", "PathPatch", "Path",
+    "Polygon", "Rectangle", "Wedge",
+    # PIL (mode="pil")
+    "Image", "ImageDraw", "ImageFont",
+    # the two libraries themselves, for anything not re-exported above
+    "np", "plt",
+    # caps a scene author may want to read
+    "GIF_MAX_SECONDS", "GIF_MAX_BYTES",
+]
+
 # A GIF longer than this balloons past what's worth shipping as a GIF.
 GIF_MAX_SECONDS = 20
 # Keep a GIF comfortably under Discord's ~10 MiB inline limit.
@@ -67,6 +92,11 @@ GIF_MAX_BYTES = 9 * 1024 * 1024
 # turns a borderline short piece into an MP4.
 PNG_BYTES_PER_PIXEL = 0.29
 
+# Contact-sheet budget. Nine tiles is a 3x3 grid that vision models read
+# reliably; spread across TIME for one scene, across scenes for many.
+CONTACT_SHEET_MAX_TILES = 9
+CONTACT_TILE_W = 300
+
 _FONT_CANDIDATES = [
     "/usr/share/fonts/truetype/dejavu/DejaVuSans-Bold.ttf",
     "/usr/share/fonts/truetype/dejavu/DejaVuSans.ttf",
@@ -110,13 +140,44 @@ class _Scene:
         self.mode, self.hold = mode, float(hold)
         self.frames = 0
         self.error = None
-        self.first_frame = None
+        # (t, thumbnail) pairs for the contact sheet. Thumbnails, not full
+        # frames: nine 640x480 RGB copies is ~8 MiB of RAM held for the whole
+        # render, and the sheet downscales them anyway.
+        self.samples = []
 
 
 class Animation:
     """Collects scenes, renders them to a locked-size frame sequence, encodes."""
 
-    def __init__(self, width=640, height=480, fps=15, dpi=100, bg="white"):
+    def __init__(self, width=640, height=480, fps=15, dpi=100, bg="white", **kwargs):
+        # `background` is accepted as an alias for `bg` because it is the obvious
+        # synonym and agents reached for it; `force` is rejected loudly because
+        # `render(force="mp4")` DOES take it, which makes it read like an
+        # Animation-level setting. Both were real failures, one code_exec round
+        # trip each, and both are the kind a better message fixes for free.
+        if "background" in kwargs:
+            bg = kwargs.pop("background")
+        if kwargs:
+            valid = ["width", "height", "fps", "dpi", "bg (or background)"]
+            bad = sorted(kwargs)
+            hints = []
+            for k in bad:
+                near = difflib.get_close_matches(
+                    k, ["width", "height", "fps", "dpi", "bg", "background"], n=1, cutoff=0.6)
+                if near:
+                    hints.append(f"{k!r} — did you mean {near[0]!r}?")
+                elif k == "force":
+                    hints.append("'force' belongs to render(force='mp4'/'gif'), "
+                                 "not to Animation()")
+                else:
+                    hints.append(repr(k))
+            raise TypeError(
+                "Animation() got unexpected keyword argument(s): "
+                + "; ".join(hints)
+                + ". Valid: " + ", ".join(valid)
+                + ". Everything about the OUTPUT (format, size, audio) is set on "
+                  "render(), not here.")
+
         # H.264 needs even dimensions; lock them here so the encoder never fails.
         self.width = int(width) // 2 * 2
         self.height = int(height) // 2 * 2
@@ -209,10 +270,46 @@ class Animation:
 
     # ------------------------------------------------------------------- render
 
+    def _dry_run_scenes(self):
+        """Render every scene ONCE at t=0 before the real pass.
+
+        Scene functions fail at render time, not definition time, so a NameError
+        in scene 4 used to surface only after scenes 0-3 had been fully rendered
+        — and, when the pipe was open, after frames had already gone into the
+        encoder. Two production workers lost a whole render each to
+        `RuntimeError: every scene failed`, learning about one broken name per
+        attempt.
+
+        One frame per scene costs almost nothing and reports EVERY broken scene
+        in a single pass. A scene that fails here is marked and skipped by the
+        real loop rather than re-run to fail identically; if they ALL fail there
+        is nothing to encode, so say so now instead of after the setup.
+        """
+        for i, sc in enumerate(self._scenes):
+            try:
+                (self._render_pil if sc.mode == "pil" else self._render_mpl)(sc, 0.0)
+            except Exception:  # noqa: BLE001
+                sc.error = traceback.format_exc(limit=6)
+                print(f"[gifsmith] SCENE {i} ({sc.title!r}) FAILED its t=0 dry "
+                      f"run — skipped, other scenes continue:\n{sc.error}",
+                      file=sys.stderr, flush=True)
+        broken = [i for i, sc in enumerate(self._scenes) if sc.error]
+        if broken and len(broken) == len(self._scenes):
+            raise RuntimeError(
+                "every scene failed before rendering started — see the "
+                f"{len(broken)} traceback(s) above. They usually share ONE "
+                "cause: a helper called with a keyword it does not take, or a "
+                "name that differs by a character. Fix it and re-run the whole "
+                "program in a fresh code_exec call, remembering gifsmith.py in "
+                "files_in. Nothing was encoded and no GPU time was spent.")
+        return broken
+
     def render(self, long_edge=None, force=None, audio=None):
         if not self._scenes:
             raise RuntimeError("no scenes registered — decorate at least one "
                                "function with @anim.scene(...)")
+        # Before ANY setup: no scratch dir, no ffmpeg pipe, no frames.
+        self._dry_run_scenes()
         self._scratch, free_mb = _pick_scratch()
         long_edge = long_edge or max(self.width, self.height)
 
@@ -241,20 +338,33 @@ class Animation:
         print(f"[gifsmith] {len(self._scenes)} scene(s), {total_s:.1f}s at "
               f"{self.fps}fps, canvas {self.width}x{self.height}", flush=True)
 
+        # Tile budget for the contact sheet, spread across the scenes. One scene
+        # gets the whole budget in TIME, which is the case that was broken: a
+        # loop's interesting content is by construction not at any single
+        # instant.
+        per_scene = max(1, CONTACT_SHEET_MAX_TILES // len(self._scenes))
+
         for i, sc in enumerate(self._scenes):
+            if sc.error:
+                continue  # already reported by the dry run; don't fail it twice
             start = self._count
             n = max(1, int(round(sc.seconds * self.fps)))
             try:
+                want = self._sample_frames(n, per_scene)
                 for k in range(n):
                     t = k / max(n - 1, 1)
                     img = (self._render_pil if sc.mode == "pil" else self._render_mpl)(sc, t)
-                    if sc.first_frame is None and k >= n // 2:
+                    if k in want:
-                        sc.first_frame = img.copy()
+                        sc.samples.append((t, img.resize(
+                            (CONTACT_TILE_W, self._tile_h()), Image.LANCZOS)))
                     self._emit(img)
                 for _ in range(int(round(sc.hold * self.fps))):
                     self._emit(img)
             except Exception:  # noqa: BLE001
                 sc.error = traceback.format_exc(limit=6)
+                # Its partial frames are dropped below; its samples must go too,
+                # or the sheet advertises a scene that never shipped.
+                sc.samples = []
                 if not self._streaming:
                     # Drop this scene's partial frames so it can't ship half-drawn.
                     # (Streamed frames are already in the encoder — a broken scene
@@ -292,15 +402,19 @@ class Animation:
                     os.remove(leftover)
             shutil.rmtree(self._scratch, ignore_errors=True)
             raise
-        self._contact_sheet()
+        sheet_tiles = self._contact_sheet() or 0
         broken = [f"{i}:{s.title!r}" for i, s in enumerate(self._scenes) if s.error]
         print(f"[gifsmith] {self._count} frames, scratch "
               + ("streamed (0 MiB on disk)" if self._streaming
                  else f"peak {self._peak_mb:.1f} MiB"))
         print(f"[gifsmith] scene errors: {', '.join(broken) if broken else 'none'}")
         print(f"[gifsmith] ART {art}")
-        print("[gifsmith] still: /workspace/still.png (contact sheet — call "
+        ts = ", ".join(f"{t:.2f}" for sc in self._scenes for (t, _th) in sc.samples)
-              "image_describe on ITS file_id from files_out)")
+        print(f"[gifsmith] still: /workspace/still.png — {sheet_tiles} frames "
+              f"sampled across the animation at t={ts} (each tile is labelled "
+              "with its t). Call image_describe on ITS file_id from files_out; "
+              "if something is missing from one tile, check whether it is "
+              "present in another before re-rendering.")
         shutil.rmtree(self._scratch, ignore_errors=True)
         return art
 
@@ -367,13 +481,36 @@ class Animation:
             _run(["gifski", "-o", out, "--fps", str(self.fps), "--quality", "90",
                   "--width", str(min(long_edge, self.width))] + fs)
         else:
-            vf = (f"fps={self.fps},scale='min({long_edge},iw)':-1:flags=lanczos,"
+            # -framerate applies to the INPUT. Without it ffmpeg reads a PNG
+            # sequence at its default 25fps, and a fps= filter then resamples
+            # 25 -> self.fps: on the #1510 render that silently dropped 91 of
+            # 207 frames (44%) and played the rest too fast. _mp4 below always
+            # passed -framerate; this path never did. With the input rate set
+            # correctly the fps= filter is redundant — and removing it is what
+            # takes the output from 205 frames back to all 207.
+            vf = (f"scale='min({long_edge},iw)':-1:flags=lanczos,"
                   f"split[s0][s1];[s0]palettegen=stats_mode=diff[p];"
                   f"[s1][p]paletteuse=dither=bayer:bayer_scale=3")
-            _run(["ffmpeg", "-y", "-loglevel", "error", "-pattern_type", "glob",
+            _run(["ffmpeg", "-y", "-loglevel", "error",
+                  "-framerate", str(self.fps), "-pattern_type", "glob",
                   "-i", os.path.join(self._scratch, "frame_*.png"), "-vf", vf, out])
         if _which("gifsicle"):
-            _run(["gifsicle", "-O3", "--lossy=60", out, "-o", out])
+            # --colors 256 is load-bearing for DISCORD, not for size.
+            #
+            # gifski gives every frame its OWN local colour table (that is where
+            # its quality comes from). Stack gifsicle's cross-frame transparency
+            # on top and the result is a GIF that gifsicle itself refuses to
+            # reopen: "too complex to unoptimize — local colour tables or complex
+            # transparency". PIL, ffmpeg and macOS Preview all render it fine;
+            # Discord's decoder does not, and paints later scenes with stale rows,
+            # ghosted captions and holes punched in solid shapes (#1510).
+            #
+            # Collapsing to ONE global palette costs ~6/255 mean colour error
+            # (visible as slight banding in gradients, since one palette now
+            # spans every scene) and is the price of playing in the client that
+            # actually matters. It also came out SMALLER on the reported file:
+            # 1210 KB -> 915 KB.
+            _run(["gifsicle", "--colors", "256", "-O3", "--lossy=60", out, "-o", out])
 
     def _mp4(self, out, long_edge, audio=None):
         cmd = ["ffmpeg", "-y", "-loglevel", "error", "-framerate", str(self.fps),
@@ -388,25 +525,64 @@ class Animation:
 
     # ------------------------------------------------------------- critique aid
 
+    def _tile_h(self):
+        return max(1, round(self.height * CONTACT_TILE_W / self.width))
+
+    @staticmethod
+    def _sample_frames(n, count):
+        """Frame indices to keep for the contact sheet, spread evenly over the
+        scene: t = 0, 1/count, ... (count-1)/count.
+
+        A scene with very few frames yields fewer samples than asked for, since
+        distinct t values collide onto the same frame index; the sheet simply
+        has fewer tiles, which is the honest outcome.
+
+        Deliberately NOT ending at t=1.0. A loop is built to return to its
+        starting state, so its last frame is the one instant guaranteed to show
+        none of the motion — which is exactly how a worker spent three
+        re-renders chasing a tear its critic could not see, because the still
+        was the frame after the tear had faded."""
+        if n <= 1:
+            return {0}
+        count = max(1, min(count, n))
+        return {min(n - 1, round(j / count * (n - 1))) for j in range(count)}
+
     def _contact_sheet(self, path="/workspace/still.png"):
-        tiles = [(i, s) for i, s in enumerate(self._scenes) if s.first_frame is not None]
+        """A grid sampled across TIME, one labelled tile per sample.
+
+        It used to be one frame per SCENE, and a single-scene animation — the
+        common case — short-circuited to a bare frame save. Every single-scene
+        worker in two production runs told us so unprompted, in the words of
+        the vision model: "this is a single frame (not a contact sheet)". The
+        critique loop was being fed one time point out of ~84 and asked to
+        judge motion.
+        """
+        tiles = [(i, sc, t, thumb)
+                 for i, sc in enumerate(self._scenes)
+                 for (t, thumb) in sc.samples]
         if not tiles:
             return
-        if len(tiles) == 1:
-            tiles[0][1].first_frame.save(path)
-            return
         cols = min(3, len(tiles))
         rows = math.ceil(len(tiles) / cols)
-        tw = 300
+        tw, th = CONTACT_TILE_W, self._tile_h()
-        th = max(1, round(self.height * tw / self.width))
         sheet = Image.new("RGB", (cols * tw, rows * (th + 20)), (20, 20, 24))
         d = ImageDraw.Draw(sheet)
         f = self.font(14)
-        for k, (i, sc) in enumerate(tiles):
+        multi = len(self._scenes) > 1
+        for k, (i, sc, t, thumb) in enumerate(tiles):
             x, y = (k % cols) * tw, (k // cols) * (th + 20)
-            sheet.paste(sc.first_frame.resize((tw, th), Image.LANCZOS), (x, y + 20))
+            sheet.paste(thumb, (x, y + 20))
-            d.text((x + 4, y + 3), f"scene {i}: {sc.title[:38]}", fill=(235, 235, 240), font=f)
+            # The t label is the point: it tells the critic WHEN it is looking,
+            # so "the tear is missing" can be read as "missing at t=0.75"
+            # rather than as a fault in the drawing.
+            label = f"t={t:.2f}"
+            if multi:
+                label = f"scene {i} {label}: {sc.title[:26]}"
+            elif sc.title:
+                label = f"{label}  {sc.title[:30]}"
+            d.text((x + 4, y + 3), label, fill=(235, 235, 240), font=f)
         sheet.save(path)
+        return len(tiles)
 
 
 def _run(cmd):