feat: --gif mode (frames → inline animated GIF) (#33)

* feat: add gifenc package + go-quantize dep (frames to animated GIF)

* test: lock delay-rounding and zero-size-frame in gifenc

* feat: add assembleGIF cli helper (frame files to temp gif)

* fix: reject bare "." / ".." in assembleGIF opts.name

filepath.Base has no separator to strip from a bare "." or "..", so
it passed them through unchanged and Join resolved outside the temp
dir. Fall back to the default name for those two literal cases and
pin the fix with a regression test.

* fix: reject bare separator in assembleGIF opts.name too

filepath.Base of an all-separator path returns a single separator
unchanged (nothing to strip), so a "/" name passed the existing "."/
".." guard and Join collapsed it onto tmpDir itself, failing the
write with an is-a-directory error instead of falling back to
clip.gif. Extend the guard and pin it with a test case.

* feat: add --gif mode to upload flow

Wires --gif/--delay/--colors flags into runWithDeps and runUpload so
the CLI can collapse ordered frame files into one animated GIF and
upload that instead of the individual frames. --name now also labels
the --gif output basename and is routed through validateName (not
just assembleGIF's internal filepath.Base guard). --gif rejects stdin
input, and --colors/--delay are validated at the boundary rather than
silently coerced.

* fix: bound --delay and require .gif extension on --gif --name

Council review flagged two real gaps: --delay had no upper bound
(gifenc rounds it into a 16-bit GIF centisecond field, so values above
655350ms would silently wrap and corrupt playback speed instead of
erroring), and --gif --name didn't require a .gif extension (comment.go
picks inline-vs-link rendering purely from the uploaded basename's
extension, so a mismatched name would defeat --gif's whole point).
Three other findings from the same review were checked against the
actual code and refuted — no fix needed, see task-3-report.md.

* test: lock the --delay=655350 boundary for --gif

Adds the missing upper-boundary case for the --delay range check
(655350ms accepted) so an off-by-one in the > vs >= comparison would
be caught. The second council review round's other findings (claiming
the .gif-extension guard on --name was reachable with an empty name,
without --gif, or via stdin) were all refuted: the guard is nested
inside opts.gif && opts.name != "", and --gif + stdin is already
rejected earlier in the same function — both already covered by
passing tests.

* test: avoid cap() shadowing, decode-verify max-delay gif output

Renames the cap variable to captured across the added gif validation
tests (the brief's own verbatim Step-1 test still uses cap and is
left untouched). Also strengthens TestRun_GifMode_AcceptsMaxDelay to
decode the produced GIF and assert the encoded delay is exactly 65535
centiseconds, via a gifBytesCapturingGitClient that reads the pushed
file's bytes inside PushAttachments (assembleGIF's deferred cleanup
removes the temp file before runWithDeps returns, so bytes can't be
read back afterward).

* fix: drop unrequested .gif-name restriction (false rationale; keep --delay bound)

* feat: add frame-cap and size-ceiling guards to --gif

* test: add dedicated sampleEvenly boundary coverage

* test: use slices.Equal and cover nil/empty in sampleEvenly test

* fix: clamp gif delay to uint16 max to prevent re-encode-path wrap

The per-frame delay is written to the GIF stream as a uint16 by
encoding/gif; only the low side was clamped, so a delay above 65535
centiseconds (e.g. doubled delay on the --size-ceiling re-encode path)
silently truncated to a wrong, too-fast value instead of erroring or
saturating.

* fix: check DecodeAll error in new upper-clamp test

Council review flagged the ignored gif.DecodeAll error: a malformed
GIF would leave g nil and panic on the next line instead of failing
cleanly. Declined the accompanying boundary-value (65535 vs 65536 cs)
suggestion as out of scope for this fix.

* fix: document guard flags, reject negative guard values, clarify re-encode comment

Council review found README missing docs for --max-frames/--size-ceiling,
inconsistent boundary validation (negative values silently treated as
"disabled" instead of rejected like --colors/--delay), and an inaccurate
comment claiming delay-doubling always preserves playback speed.

Claude-Session: https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B

Author: Hinne1

README.md

@@ -50,12 +50,42 @@ gh attach --json 123 screenshot.png
 # Read file bytes from stdin with --name BASENAME (see "Reading from stdin" below)
 screencapture -i -t png - | gh attach --name shot.png 123 -
 
+# Assemble ordered frames into one inline animated GIF and attach to PR 123
+gh attach --gif 123 frames/frame-*.png
+
+# Name the clip and tune playback (per-frame delay in ms) + palette size
+gh attach --gif --name verify.gif --delay 80 --colors 128 123 frames/frame-*.png
+
 # Download files back to disk (see "gh attach get" below)
 gh attach get 123 --output ./restored
 ```
 
 By default `gh attach` reads the target repo from the current clone's `origin` remote. Pass `--repo OWNER/NAME` (or a full GitHub URL) to target a different repo or to run from outside any git clone. Whenever `--repo` is used, `NUMBER` or `--key` must be passed explicitly — PR auto-detection only works inside a clone of the target repo.
 
+### Animated GIF mode (`--gif`)
+
+`--gif` assembles the supplied image frames (PNG or JPEG) into a single
+animated GIF and uploads *that* — the GIF renders inline and autoplays in
+the PR comment, unlike an `.mp4`, which GitHub can only link. Frames play
+in filename order, so zero-pad them (`frame-000.png`, `frame-001.png`, …).
+All frames must share the same dimensions. `--delay` sets per-frame time in
+milliseconds (default 80 ≈ 12 fps); `--colors` sets the per-frame palette
+size (default 256). No ffmpeg and no external binaries — encoding is pure Go.
+
+Two guard flags bound runaway captures. `--max-frames` (default 300) caps
+the frame count — if the supplied frames exceed it, they're evenly sampled
+down to `--max-frames` so playback still covers the whole clip rather than
+truncating the tail (`0` disables the cap). `--size-ceiling` (default
+5242880 bytes = 5 MB) bounds the encoded GIF's size — if the first encode
+exceeds it, `gh attach` re-encodes once with fewer colors and half the
+frames (`0` disables the ceiling); if the reduced encode is still over,
+it's uploaded anyway with a warning.
+
+```bash
+# Bound a long capture: cap frames and re-encode if the GIF is too big
+gh attach --gif --max-frames 200 --size-ceiling 3000000 123 frames/frame-*.png
+```
+
 ### JSON output
 
 Pass `--json` to get a structured result object on stdout instead of the markdown table. Stderr is suppressed in JSON mode (no progress line, no `Uploaded:` URL list) so the output is pipe-friendly:

go.mod

@@ -1,3 +1,5 @@
 module github.com/enthus-appdev/gh-attach
 
 go 1.26
+
+require github.com/ericpauley/go-quantize v0.0.0-20200331213906-ae555eb2afa4

go.sum

@@ -0,0 +1,2 @@
+github.com/ericpauley/go-quantize v0.0.0-20200331213906-ae555eb2afa4 h1:BBade+JlV/f7JstZ4pitd4tHhpN+w+6I+LyOS7B4fyU=
+github.com/ericpauley/go-quantize v0.0.0-20200331213906-ae555eb2afa4/go.mod h1:H7chHJglrhPPzetLdzBleF8d22WYOv7UM/lEKYiwlKM=

internal/cli/gif.go

@@ -0,0 +1,150 @@
+package cli
+
+import (
+	"fmt"
+	"image"
+	_ "image/jpeg" // register JPEG decoder for image.Decode
+	_ "image/png"  // register PNG decoder for image.Decode
+	"os"
+	"path/filepath"
+	"strings"
+
+	"github.com/enthus-appdev/gh-attach/internal/gifenc"
+)
+
+// gifAssembleOptions carries the flag-derived tunables for --gif mode.
+type gifAssembleOptions struct {
+	name        string // output basename; empty → "clip.gif"
+	delayMS     int
+	numColors   int
+	maxFrames   int   // cap on frames; 0 → no cap. Excess frames are evenly sampled out.
+	sizeCeiling int64 // byte ceiling; 0 → no ceiling. Over → one reduced re-encode.
+}
+
+// assembleGIF decodes the ordered frame files (PNG or JPEG), applies
+// the frame cap, encodes an animated GIF, and (if a size ceiling is
+// set and exceeded) re-encodes once with a smaller palette and half
+// the frames. framePaths are used in slice order — globbed inputs
+// arrive lexicographically, so zero-padded frame-000.png … frame-NNN.png
+// names sort into playback order. Returns the temp gif path, a cleanup
+// closure (removes the temp dir; must be deferred by the caller), and
+// a human-readable warning — every guard that fired contributes a
+// message, joined with "; "; empty when none fired.
+func assembleGIF(framePaths []string, opts gifAssembleOptions) (string, func(), string, error) {
+	if len(framePaths) == 0 {
+		return "", nil, "", fmt.Errorf("no frames to assemble")
+	}
+
+	// Accumulate — both the frame-cap and size-ceiling guards can fire
+	// in one call, and neither message should silently clobber the other.
+	var warnings []string
+
+	// Frame cap: evenly sample down to maxFrames so playback covers the
+	// whole clip rather than truncating the tail.
+	paths := framePaths
+	if opts.maxFrames > 0 && len(paths) > opts.maxFrames {
+		paths = sampleEvenly(paths, opts.maxFrames)
+		warnings = append(warnings, fmt.Sprintf("capped %d frames to %d (--max-frames)", len(framePaths), len(paths)))
+	}
+
+	frames := make([]image.Image, 0, len(paths))
+	for _, p := range paths {
+		img, err := decodeImageFile(p)
+		if err != nil {
+			return "", nil, "", err
+		}
+		frames = append(frames, img)
+	}
+
+	data, err := gifenc.Encode(frames, gifenc.Options{DelayMS: opts.delayMS, NumColors: opts.numColors})
+	if err != nil {
+		return "", nil, "", err
+	}
+
+	// Size ceiling: one reduced re-encode — fewer colors, half the
+	// frames (delay doubled so playback speed is preserved — except at
+	// extreme per-frame delays, where the doubled value is clamped at
+	// the encoder's uint16 centisecond max instead of preserving speed).
+	// The palette is never raised above what the caller asked for, or
+	// the "reduction" could grow the file for a low --colors value.
+	if opts.sizeCeiling > 0 && int64(len(data)) > opts.sizeCeiling {
+		reducedColors := 64
+		if opts.numColors > 0 && opts.numColors < reducedColors {
+			reducedColors = opts.numColors
+		}
+		reduced := sampleEvenly(frames, (len(frames)+1)/2)
+		data2, err2 := gifenc.Encode(reduced, gifenc.Options{DelayMS: opts.delayMS * 2, NumColors: reducedColors})
+		if err2 != nil {
+			return "", nil, "", err2
+		}
+		data = data2
+		if int64(len(data)) > opts.sizeCeiling {
+			warnings = append(warnings, fmt.Sprintf("gif is %d bytes, over the %d-byte ceiling even after reduction — uploaded anyway", len(data), opts.sizeCeiling))
+		} else {
+			warnings = append(warnings, fmt.Sprintf("gif exceeded the %d-byte ceiling — reduced to %d colors / %d frames", opts.sizeCeiling, reducedColors, len(reduced)))
+		}
+	}
+
+	// filepath.Base defends assembleGIF as a standalone function: the CLI
+	// validates --name upstream, but a path in opts.name must never let
+	// the write escape the temp dir. Base alone isn't enough for three
+	// degenerate inputs it returns unchanged (nothing to strip): ".."
+	// (Join would resolve outside tmpDir), "." (Join collapses to
+	// tmpDir itself), and a bare separator "/" (same collapse — Base
+	// of an all-separator path is a single separator). Reject all
+	// three and fall back to the default name.
+	name := "clip.gif"
+	if opts.name != "" {
+		base := filepath.Base(opts.name)
+		if base != "." && base != ".." && base != "/" && base != string(filepath.Separator) {
+			name = base
+		}
+	}
+	tmpDir, err := os.MkdirTemp("", "gh-attach-gif-*")
+	if err != nil {
+		return "", nil, "", fmt.Errorf("create temp dir: %w", err)
+	}
+	cleanup := func() { _ = os.RemoveAll(tmpDir) }
+
+	gifPath := filepath.Join(tmpDir, name)
+	if err := os.WriteFile(gifPath, data, 0o600); err != nil {
+		cleanup()
+		return "", nil, "", fmt.Errorf("write gif: %w", err)
+	}
+	return gifPath, cleanup, strings.Join(warnings, "; "), nil
+}
+
+// sampleEvenly returns n items drawn at even intervals across in,
+// always including the first and (when n>1) the last. If n>=len(in)
+// or n<=0 it returns in unchanged.
+func sampleEvenly[T any](in []T, n int) []T {
+	if n <= 0 || n >= len(in) {
+		return in
+	}
+	if n == 1 {
+		return in[:1]
+	}
+	out := make([]T, 0, n)
+	// step across [0, len-1] so both ends are represented.
+	for i := 0; i < n; i++ {
+		idx := i * (len(in) - 1) / (n - 1)
+		out = append(out, in[idx])
+	}
+	return out
+}
+
+// decodeImageFile opens and decodes a single frame file. Format is
+// detected by image.Decode from the registered PNG/JPEG decoders.
+func decodeImageFile(path string) (image.Image, error) {
+	f, err := os.Open(path)
+	if err != nil {
+		return nil, fmt.Errorf("open frame %s: %w", path, err)
+	}
+	defer func() { _ = f.Close() }()
+
+	img, _, err := image.Decode(f)
+	if err != nil {
+		return nil, fmt.Errorf("decode frame %s: %w", path, err)
+	}
+	return img, nil
+}