fix: review findings — chain NaN/Inf + id hygiene, percent-escape jobPath, shared singleVideoResult

- SubmitChain rejects NaN/±Inf segment seconds with ErrUnsupported
  (previously an obscure json.Marshal error; NaN fails every comparison
  and +Inf passed the >= 0 check).
- ChainStatus skips segment entries with no usable id — JSON null
  (which no-op-unmarshals into a string, previously appending ""),
  empty strings, and id-less objects; the unfiltered list survives in
  Raw. ChainJob.SegmentIDs doc now also says ChainSegmentResult takes
  the segment index, not an id string.
- jobPath rejects '%' in job ids — %2F/%2E%2E percent-escapes decode
  back into path structure server-side, bypassing the literal check on
  this upstream-echoed value.
- singleVideoResult moves to video.go next to videoMIME, and the two
  remaining hand-rolled copies of the video-result validation
  (videoModel.Generate, Interpolate) now use it — one validation, one
  message shape.
- videogen.LipSyncer renamed to videogen.Lipsyncer for consistency with
  the rest of the surface's Lipsync* naming (LipsyncProvider,
  LipsyncModel, LipsyncRequest); not yet consumed downstream, so the
  rename is free now and never again.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT

provider/llamaswap/lipsync.go

@@ -22,7 +22,7 @@ import (
 
 // LipsyncModel implements videogen.LipsyncProvider. The id selects which
 // upstream llama-swap loads.
-func (p *Provider) LipsyncModel(id string, opts ...videogen.LipsyncModelOption) (videogen.LipSyncer, error) {
+func (p *Provider) LipsyncModel(id string, opts ...videogen.LipsyncModelOption) (videogen.Lipsyncer, error) {
 	if err := p.requireBaseURL(); err != nil {
 		return nil, err
 	}
@@ -35,7 +35,7 @@ type lipsyncModel struct {
 	id string
 }
 
-// Lipsync implements videogen.LipSyncer.
+// Lipsync implements videogen.Lipsyncer.
 func (m *lipsyncModel) Lipsync(ctx context.Context, req videogen.LipsyncRequest, opts ...videogen.LipsyncOption) (*videogen.Result, error) {
 	req = req.Apply(opts...)
 	if len(req.Image.Data) == 0 {
@@ -95,19 +95,3 @@ func (m *lipsyncModel) Lipsync(ctx context.Context, req videogen.LipsyncRequest,
 	}
 	return singleVideoResult(m.p.name, m.id, "lipsync", raw, respType)
 }
-
-// singleVideoResult wraps one raw video body into a videogen.Result,
-// requiring positive evidence of video-ness (declared video/* Content-Type
-// or sniffed mp4/webm magic) so a proxy error page can never become "the
-// clip" — the video sibling of singleImageResult.
-func singleVideoResult(provider, model, verb string, raw []byte, contentType string) (*videogen.Result, error) {
-	if len(raw) == 0 {
-		return nil, &llm.APIError{Provider: provider, Model: model, Message: verb + " response contained no video"}
-	}
-	mimeType := videoMIME(contentType, raw)
-	if mimeType == "" {
-		return nil, &llm.APIError{Provider: provider, Model: model,
-			Message: fmt.Sprintf("%s response is not a video (Content-Type %q)", verb, contentType)}
-	}
-	return &videogen.Result{Video: videogen.Video{Data: raw, MIME: mimeType}}, nil
-}

provider/llamaswap/mediautil.go

@@ -207,12 +207,5 @@ func (m *interpolatorModel) Interpolate(ctx context.Context, req videogen.Interp
 	if err != nil {
 		return nil, err
 	}
-	if len(raw) == 0 {
-		return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "interpolate response contained no video"}
-	}
-	mimeType := videoMIME(respType, raw)
-	if mimeType == "" {
-		return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "interpolate response is not a video"}
-	}
-	return &videogen.Result{Video: videogen.Video{Data: raw, MIME: mimeType}}, nil
+	return singleVideoResult(m.p.name, m.id, "interpolate", raw, respType)
 }

provider/llamaswap/video.go

@@ -100,22 +100,7 @@ func (m *videoModel) Generate(ctx context.Context, req videogen.Request, opts ..
 	if err != nil {
 		return nil, err
 	}
-	if len(videoBytes) == 0 {
-		return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "video response contained no video"}
-	}
-	mimeType := videoMIME(contentType, videoBytes)
-	if mimeType == "" {
-		// A 2xx body that is neither declared nor sniffable as video is a
-		// misconfigured upstream (a JSON job envelope, an HTML error page
-		// behind a proxy) — fail loud rather than hand back garbage as a
-		// playable clip.
-		return nil, &llm.APIError{
-			Provider: m.p.name,
-			Model:    m.id,
-			Message:  fmt.Sprintf("video response is not a video (content-type %q)", contentType),
-		}
-	}
-	return &videogen.Result{Video: videogen.Video{Data: videoBytes, MIME: mimeType}}, nil
+	return singleVideoResult(m.p.name, m.id, "video", videoBytes, contentType)
 }
 
 // videoMIME resolves the result MIME type: the response Content-Type when it
@@ -132,6 +117,24 @@ func videoMIME(contentType string, data []byte) string {
 	return ""
 }
 
+// singleVideoResult wraps one raw video body into a videogen.Result,
+// requiring positive evidence of video-ness (declared video/* Content-Type
+// or sniffed mp4/webm magic) so a 2xx body that is anything else — a JSON
+// job envelope, an HTML error page behind a proxy — fails loud instead of
+// coming back as "the clip". The video sibling of singleImageResult, shared
+// by every surface whose response body IS the encoded clip.
+func singleVideoResult(provider, model, verb string, raw []byte, contentType string) (*videogen.Result, error) {
+	if len(raw) == 0 {
+		return nil, &llm.APIError{Provider: provider, Model: model, Message: verb + " response contained no video"}
+	}
+	mimeType := videoMIME(contentType, raw)
+	if mimeType == "" {
+		return nil, &llm.APIError{Provider: provider, Model: model,
+			Message: fmt.Sprintf("%s response is not a video (Content-Type %q)", verb, contentType)}
+	}
+	return &videogen.Result{Video: videogen.Video{Data: raw, MIME: mimeType}}, nil
+}
+
 // initImageFilename picks the multipart filename hint for the conditioning
 // frame from its MIME subtype. The name is provider-chosen (never
 // caller-supplied), so no sanitization is needed.

provider/llamaswap/videochain.go

@@ -19,6 +19,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
+	"math"
 	"net/http"
 	"strconv"
 	"strings"
@@ -71,8 +72,10 @@ func (m *chainerModel) SubmitChain(ctx context.Context, req videogen.ChainReques
 		if strings.TrimSpace(seg.Prompt) == "" {
 			return "", fmt.Errorf("%w: video chain segment %d requires a prompt", llm.ErrUnsupported, i)
 		}
-		if seg.Seconds < 0 {
-			return "", fmt.Errorf("%w: video chain segment %d seconds must be >= 0, got %g", llm.ErrUnsupported, i, seg.Seconds)
+		// NaN/±Inf would otherwise surface as an obscure json.Marshal error
+		// (NaN fails every comparison; +Inf passes the >= 0 check).
+		if seg.Seconds < 0 || math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, 0) {
+			return "", fmt.Errorf("%w: video chain segment %d seconds must be a finite value >= 0, got %g", llm.ErrUnsupported, i, seg.Seconds)
 		}
 		wire.Segments = append(wire.Segments, chainSegmentWire{Prompt: seg.Prompt, Seconds: seg.Seconds})
 	}
@@ -133,10 +136,17 @@ func (m *chainerModel) ChainStatus(ctx context.Context, jobID string) (*videogen
 		Total:   out.Total,
 		Raw:     raw,
 	}
+	// Entries that carry no usable id — JSON null (which unmarshals into a
+	// string as a no-op, leaving ""), an empty string, or an object with
+	// neither key — are SKIPPED, never appended as "": SegmentIDs promises
+	// fetchable artifacts, and the full payload stays in Raw for callers
+	// that want the unfiltered list.
 	for _, entry := range out.Segments {
 		var s string
 		if json.Unmarshal(entry, &s) == nil {
+			if s != "" {
 				job.SegmentIDs = append(job.SegmentIDs, s)
+			}
 			continue
 		}
 		var obj struct {
@@ -197,7 +207,10 @@ func (m *chainerModel) jobPath(jobID, suffix string) (string, error) {
 	if strings.TrimSpace(jobID) == "" {
 		return "", fmt.Errorf("llama-swap: chain job call requires a job id")
 	}
-	if strings.ContainsAny(jobID, "/?#") || strings.Contains(jobID, "..") {
+	// '%' is rejected alongside the literal path characters: job ids never
+	// legitimately carry percent-escapes, and %2F/%2E%2E would decode back
+	// into path structure server-side.
+	if strings.ContainsAny(jobID, "/?#%") || strings.Contains(jobID, "..") {
 		return "", fmt.Errorf("llama-swap: invalid chain job id %q (contains path structure)", jobID)
 	}
 	return upstreamPath(m.id, "/v1/jobs/"+jobID+suffix)

provider/llamaswap/videochain_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"errors"
+	"math"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
@@ -111,6 +112,17 @@ func TestSubmitChainRejectsBadArgs(t *testing.T) {
 	}); !errors.Is(err, llm.ErrUnsupported) {
 		t.Errorf("negative seconds: err = %v, want ErrUnsupported", err)
 	}
+	for name, bad := range map[string]float64{
+		"NaN":  math.NaN(),
+		"+Inf": math.Inf(1),
+		"-Inf": math.Inf(-1),
+	} {
+		if _, err := ch.SubmitChain(context.Background(), videogen.ChainRequest{
+			Segments: []videogen.ChainSegment{{Prompt: "x", Seconds: bad}},
+		}); !errors.Is(err, llm.ErrUnsupported) {
+			t.Errorf("%s seconds: err = %v, want ErrUnsupported", name, err)
+		}
+	}
 }
 
 func TestSubmitChainRejectsMissingJobID(t *testing.T) {
@@ -175,6 +187,27 @@ func TestChainStatusToleratesObjectSegments(t *testing.T) {
 	}
 }
 
+func TestChainStatusSkipsIDLessSegments(t *testing.T) {
+	// null, "", an id-less object, and a mistyped entry must all be
+	// skipped — never appended as "" (SegmentIDs promises fetchable
+	// artifacts; the unfiltered list stays in Raw).
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, _ = w.Write([]byte(`{"status":"running","segment":3,"total":5,` +
+			`"segments":[null,"seg-0","",{},{"id":"seg-1"},{"segment_id":""},42]}`))
+	}))
+	defer srv.Close()
+
+	p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
+	ch, _ := p.ChainerModel("videoutils")
+	job, err := ch.ChainStatus(context.Background(), "job-123")
+	if err != nil {
+		t.Fatalf("ChainStatus: %v", err)
+	}
+	if !reflect.DeepEqual(job.SegmentIDs, []string{"seg-0", "seg-1"}) {
+		t.Errorf("SegmentIDs = %v, want [seg-0 seg-1]", job.SegmentIDs)
+	}
+}
+
 func TestChainStatusRejectsStatuslessPayload(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		_, _ = w.Write([]byte(`{"detail":"no such job"}`))
@@ -193,7 +226,7 @@ func TestChainStatusRejectsStatuslessPayload(t *testing.T) {
 func TestChainJobPathRejectsHostileIDs(t *testing.T) {
 	p := New(WithBaseURL("http://unused"))
 	ch, _ := p.ChainerModel("videoutils")
-	for _, bad := range []string{"", "a/b", "a?b", "a#b", "..", "a..b"} {
+	for _, bad := range []string{"", "a/b", "a?b", "a#b", "..", "a..b", "a%2Fb", "%2e%2e", "a%b"} {
 		if _, err := ch.ChainStatus(context.Background(), bad); err == nil {
 			t.Errorf("ChainStatus(%q) succeeded; want error", bad)
 		}

videogen/chain.go

@@ -42,9 +42,13 @@ type ChainJob struct {
 	Segment int
 	Total   int
 
-	// SegmentIDs are the COMPLETED per-segment artifacts, fetchable via
-	// ChainSegmentResult — a mid-chain failure still leaves these
-	// retrievable, so multi-minute GPU output is never discarded.
+	// SegmentIDs name the COMPLETED per-segment artifacts, in order. A
+	// mid-chain failure still leaves those segments retrievable via
+	// ChainSegmentResult — note it takes the segment's index in the chain,
+	// not an id string; this list tells you WHICH segments completed. So
+	// multi-minute GPU output is never discarded. Entries the backend
+	// reports without a usable id are skipped (the unfiltered list survives
+	// in Raw).
 	SegmentIDs []string
 
 	// Raw is the provider-native job payload. May be nil.

videogen/lipsync.go

@@ -57,16 +57,16 @@ func (r LipsyncRequest) Apply(opts ...LipsyncOption) LipsyncRequest {
 	return r
 }
 
-// LipSyncer animates still portraits into talking-head clips. Its own small
+// Lipsyncer animates still portraits into talking-head clips. Its own small
 // interface rather than a method on Model: lip-syncers are not text-to-video
 // generators — they bind to a different backend id entirely.
-type LipSyncer interface {
+type Lipsyncer interface {
 	// Lipsync returns the talking-head clip. Generation is slow (minutes);
 	// bound the call with a context deadline.
 	Lipsync(ctx context.Context, req LipsyncRequest, opts ...LipsyncOption) (*Result, error)
 }
 
-// LipsyncModelOption configures a LipSyncer at construction time. Reserved
+// LipsyncModelOption configures a Lipsyncer at construction time. Reserved
 // for future per-model settings.
 type LipsyncModelOption func(*LipsyncModelConfig)
 
@@ -82,12 +82,12 @@ func ApplyLipsyncModelOptions(opts []LipsyncModelOption) LipsyncModelConfig {
 	return cfg
 }
 
-// LipsyncProvider mints LipSyncers bound to one backend.
+// LipsyncProvider mints Lipsyncers bound to one backend.
 type LipsyncProvider interface {
 	// Name is the registry identifier for the provider.
 	Name() string
 
-	// LipsyncModel returns a LipSyncer bound to the given id (passed through
+	// LipsyncModel returns a Lipsyncer bound to the given id (passed through
 	// to the backend verbatim; no catalog validation).
-	LipsyncModel(id string, opts ...LipsyncModelOption) (LipSyncer, error)
+	LipsyncModel(id string, opts ...LipsyncModelOption) (Lipsyncer, error)
 }