media: address gadfly review — single-pass elide, drop helpers, stronger test
Review fixes (no behavior change): - Fold the over-cap elide INTO the existing copy-on-write normalize pass: one loop now replaces the first toElide (oldest) images with the placeholder and size-normalizes the rest, so the Messages slice is copied at most once (the prior dropOldestImages + the normalize loop double-copied when overflow and a transform both applied — the dominant review finding, 5 models). - Remove dropOldestImages (the name implied removal; it substituted) and the one-shot hasImagePart helper — both subsumed by the single pass. - Trim the 9-line inline comment that restated the package doc. - Test: rename TestNormalizeTooManyImages_DropsOldest → TestNormalizeOverCount (file convention) and assert the EXACT survivors ([b, c], in order) + a content-based non-mutation check (first input part is still image a, which a len check wouldn't catch). Build + media + majordomo suites green (-race). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
+13
-15
@@ -149,10 +149,10 @@ func TestNormalizeImagesUnsupported(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestNormalizeTooManyImages_DropsOldest(t *testing.T) {
|
||||
// 3 distinguishable images across 2 messages; cap = 2. Overflow no longer
|
||||
func TestNormalizeOverCount(t *testing.T) {
|
||||
// 3 distinguishable images across 2 messages; cap = 2. Over-count no longer
|
||||
// errors — the OLDEST image is replaced with a placeholder and the most-recent
|
||||
// two (the relevant ones in an iterative run) are kept.
|
||||
// two (the relevant ones in an iterative run) are kept, in order.
|
||||
a := llm.Image("image/png", encPNG(t, gradient(2, 2))).(llm.ImagePart)
|
||||
b := llm.Image("image/png", encPNG(t, gradient(4, 4))).(llm.ImagePart)
|
||||
c := llm.Image("image/png", encPNG(t, gradient(8, 8))).(llm.ImagePart)
|
||||
@@ -163,7 +163,7 @@ func TestNormalizeTooManyImages_DropsOldest(t *testing.T) {
|
||||
caps := llm.Capabilities{MaxImagesPerReq: 2, MaxImageDimension: 64, MaxImageBytes: 1 << 20, AllowedImageMIME: []string{"image/png"}}
|
||||
out, err := Normalize(req, caps)
|
||||
if err != nil {
|
||||
t.Fatalf("drop-oldest overflow should not error: %v", err)
|
||||
t.Fatalf("over-count should not error: %v", err)
|
||||
}
|
||||
var imgs []llm.ImagePart
|
||||
placeholders := 0
|
||||
@@ -179,20 +179,18 @@ func TestNormalizeTooManyImages_DropsOldest(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
if len(imgs) != 2 {
|
||||
t.Fatalf("kept %d images, want 2 (the cap)", len(imgs))
|
||||
// The exact survivors are the most-recent two, in order: b then c (a elided).
|
||||
if len(imgs) != 2 || !bytes.Equal(imgs[0].Data, b.Data) || !bytes.Equal(imgs[1].Data, c.Data) {
|
||||
t.Fatalf("kept %d images; want exactly [b, c] (the most-recent two)", len(imgs))
|
||||
}
|
||||
if placeholders != 1 {
|
||||
t.Errorf("placeholders = %d, want 1 for the dropped oldest image", placeholders)
|
||||
t.Errorf("placeholders = %d, want 1 for the elided oldest image", placeholders)
|
||||
}
|
||||
for _, im := range imgs {
|
||||
if bytes.Equal(im.Data, a.Data) {
|
||||
t.Errorf("oldest image was kept; the most-recent two should survive")
|
||||
}
|
||||
}
|
||||
// The input request must be untouched (copy-on-write).
|
||||
if len(req.Messages[0].Parts) != 2 {
|
||||
t.Errorf("input request was mutated: %+v", req.Messages[0].Parts)
|
||||
// Input request untouched (copy-on-write): the first part is still image a,
|
||||
// not a placeholder — a len check alone wouldn't catch in-place substitution.
|
||||
first, ok := req.Messages[0].Parts[0].(llm.ImagePart)
|
||||
if !ok || !bytes.Equal(first.Data, a.Data) {
|
||||
t.Errorf("input request was mutated; first part = %+v", req.Messages[0].Parts[0])
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user