Closes#80. Prerequisite for #81 (seed-packet capture). Part of #86.
Why
majordomo's media pipeline is stdlib-based and cannot decode HEIC or webp. HEIC is the iPhone camera default, so the capture feature's very first input — a photo from a phone — would fail on the device that motivates it. Normalizing at the upload boundary means everything downstream (and majordomo's vision path) only ever sees JPEG.
What
internal/imagenorm.Normalize(r, opts) (jpeg []byte, format string, err error):
reads at most MaxBytes (25 MiB) → ErrTooLarge,
checks the decoded pixel count from DecodeConfigbefore allocating the bitmap (decompression-bomb guard) → ErrTooLarge,
decodes jpeg/png/heic/webp,
downscales to fit MaxDim (2048, ollama-cloud's limit) on the longest edge, Catmull-Rom (sharp on the text a packet is mostly made of),
re-encodes JPEG q85.
CGO stays off
github.com/gen2brain/heic runs libheif as WASM via wazero — pure Go, no cgo.
golang.org/x/image/webp is pure Go.
Both register with image.Decode.
The ~5 MB these add only links into the binary when #81 imports this package — cmd/pansy doesn't import it yet, so main's size is unchanged for now (verified: 32.8 MB before and after, CGO_ENABLED=0). The cost lands honestly with the feature that uses it.
The test that matters
TestNormalizeAllFormats round-trips all four formats to a valid JPEG. Its real job is catching a dropped blank import — where the failure is backwards from intuition: delete _ "image/png" and PNG uploads break with "unknown format" while the exotic HEIC still works. Plus downscale (aspect preserved, small images untouched), oversize rejection, and garbage/truncated → clean ErrUnsupported, no panic.
heic/webp fixtures live in testdata/ (Go has no encoder for either). The webp is a known-good 16×16 from Python's stdlib test corpus, verified to decode with x/image/webp before committing; the heic was made with sips.
GOWORK=off go test ./... green; gofmt -l internal/ clean; static build confirmed.
Closes #80. Prerequisite for #81 (seed-packet capture). Part of #86.
## Why
majordomo's media pipeline is stdlib-based and **cannot decode HEIC or webp**. HEIC is the iPhone camera default, so the capture feature's very first input — a photo from a phone — would fail on the device that motivates it. Normalizing at the upload boundary means everything downstream (and majordomo's vision path) only ever sees JPEG.
## What
`internal/imagenorm.Normalize(r, opts) (jpeg []byte, format string, err error)`:
1. reads at most `MaxBytes` (25 MiB) → `ErrTooLarge`,
2. checks the decoded pixel count from `DecodeConfig` **before** allocating the bitmap (decompression-bomb guard) → `ErrTooLarge`,
3. decodes jpeg/png/heic/webp,
4. downscales to fit `MaxDim` (2048, ollama-cloud's limit) on the longest edge, Catmull-Rom (sharp on the text a packet is mostly made of),
5. re-encodes JPEG q85.
## CGO stays off
- `github.com/gen2brain/heic` runs libheif as **WASM via wazero** — pure Go, no cgo.
- `golang.org/x/image/webp` is pure Go.
- Both register with `image.Decode`.
The ~5 MB these add **only links into the binary when #81 imports this package** — `cmd/pansy` doesn't import it yet, so `main`'s size is unchanged for now (verified: 32.8 MB before and after, `CGO_ENABLED=0`). The cost lands honestly with the feature that uses it.
## The test that matters
`TestNormalizeAllFormats` round-trips **all four** formats to a valid JPEG. Its real job is catching a dropped blank import — where the failure is backwards from intuition: delete `_ "image/png"` and PNG uploads break with "unknown format" while the exotic HEIC still works. Plus downscale (aspect preserved, small images untouched), oversize rejection, and garbage/truncated → clean `ErrUnsupported`, no panic.
heic/webp fixtures live in `testdata/` (Go has no encoder for either). The webp is a known-good 16×16 from Python's stdlib test corpus, verified to decode with `x/image/webp` before committing; the heic was made with `sips`.
`GOWORK=off go test ./...` green; `gofmt -l internal/` clean; static build confirmed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Prerequisite for seed-packet capture (#81). majordomo's media pipeline is
stdlib-based and cannot decode HEIC or webp — and HEIC is the iPhone camera
default, so the capture feature's very first input would fail on the device
that motivates it. Normalizing at the upload boundary means everything
downstream only ever sees JPEG.
internal/imagenorm.Normalize(reader, opts) decodes any of jpeg/png/heic/webp,
downscales to fit MaxDim (2048, ollama-cloud's limit) on the longest edge with
Catmull-Rom (sharp on the text a packet is mostly made of), and re-encodes JPEG.
CGO stays off: github.com/gen2brain/heic runs libheif as WASM via wazero (pure
Go), golang.org/x/image/webp is pure Go. Both register with image.Decode. The
~5 MB these add only links into the binary when #81 imports this package — it's
not pulled into cmd/pansy yet, so main is unchanged in size for now.
Guards against hostile uploads: input capped at MaxBytes (ErrTooLarge) before a
full read, and the decoded pixel count checked from DecodeConfig BEFORE the
bitmap is allocated, so a small file claiming a huge canvas (a decompression
bomb) is refused up front.
TestNormalizeAllFormats round-trips all four formats to a valid JPEG — its real
job is catching a dropped blank import, where the intuition-breaking failure is
that the COMMON format (png) breaks while the exotic one (heic) still works.
heic/webp fixtures live in testdata (Go has no encoder for them); the webp is a
known-good 16x16 from Python's stdlib test corpus, verified to decode.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 03:17:12Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 11 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 11 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
⚪Package doc references non-existent TestNormalize; actual test is TestNormalizeAllFormats
maintainability · flagged by 1 model
internal/imagenorm/imagenorm.go:25 — stale doc reference. The package doc says TestNormalize exercises all four formats — there is no TestNormalize; the actual test is TestNormalizeAllFormats. Minor, but the CLAUDE.md rule "stale comments are worse than none" applies. Update the reference.
🪰 Gadfly · advisory
⚪ **Package doc references non-existent TestNormalize; actual test is TestNormalizeAllFormats**
_maintainability · flagged by 1 model_
- **`internal/imagenorm/imagenorm.go:25` — stale doc reference.** The package doc says `TestNormalize exercises all four formats` — there is no `TestNormalize`; the actual test is `TestNormalizeAllFormats`. Minor, but the CLAUDE.md rule "stale comments are worse than none" applies. Update the reference.
<sub>🪰 Gadfly · advisory</sub>
🟠maxDecodePixels (100 MP) permits ~400 MB peak allocation per upload — memory-exhaustion DoS surface, and the huge intermediate bitmap is discarded since MaxDim=2048 downscales everything larger
maintainability, security · flagged by 1 model
internal/imagenorm/imagenorm.go:61 — maxDecodePixels (100 MP) permits ~400 MB peak allocation per upload. The guard at line 120 rejects only more than 100 MP, so a decoded bitmap up to 100 MP is allowed through to image.Decode (line 124). At 4 bytes/px (RGBA) that is ~400 MB allocated before the downscale step even runs. A hostile, low-entropy JPEG/HEIC well under the 25 MiB input cap (line 57) can legitimately decode to that size — the byte cap does not bound decoded size for co…
🪰 Gadfly · advisory
🟠 **maxDecodePixels (100 MP) permits ~400 MB peak allocation per upload — memory-exhaustion DoS surface, and the huge intermediate bitmap is discarded since MaxDim=2048 downscales everything larger**
_maintainability, security · flagged by 1 model_
- **`internal/imagenorm/imagenorm.go:61` — `maxDecodePixels` (100 MP) permits ~400 MB peak allocation per upload.** The guard at line 120 rejects only *more* than 100 MP, so a decoded bitmap up to 100 MP is allowed through to `image.Decode` (line 124). At 4 bytes/px (RGBA) that is ~400 MB allocated before the downscale step even runs. A hostile, low-entropy JPEG/HEIC well under the 25 MiB *input* cap (line 57) can legitimately decode to that size — the byte cap does not bound decoded size for co…
<sub>🪰 Gadfly · advisory</sub>
🟡Sentinel errors use fmt.Errorf instead of errors.New, diverging from repo convention
maintainability · flagged by 4 models
internal/imagenorm/imagenorm.go:90-91 — fmt.Errorf for sentinel errors instead of errors.New. Every other sentinel error declaration in the repo uses errors.New (internal/domain/domain.go:14-64, internal/service/password.go:29, internal/agentmodel/agentmodel.go:39, internal/agent/runtime.go:52). fmt.Errorf("imagenorm: image too large") has no format verbs and pays a fmt-formatting cost for a constant string. errors.Is still works, but the package diverges from the estab…
🪰 Gadfly · advisory
🟡 **Sentinel errors use fmt.Errorf instead of errors.New, diverging from repo convention**
_maintainability · flagged by 4 models_
- **`internal/imagenorm/imagenorm.go:90-91` — `fmt.Errorf` for sentinel errors instead of `errors.New`.** Every other sentinel error declaration in the repo uses `errors.New` (`internal/domain/domain.go:14-64`, `internal/service/password.go:29`, `internal/agentmodel/agentmodel.go:39`, `internal/agent/runtime.go:52`). `fmt.Errorf("imagenorm: image too large")` has no format verbs and pays a fmt-formatting cost for a constant string. `errors.Is` still works, but the package diverges from the estab…
<sub>🪰 Gadfly · advisory</sub>
🟠Doc contract overpromises: read/encode I/O errors don't actually map to the documented ErrUnsupported/ErrTooLarge sentinels
error-handling, security · flagged by 3 models
internal/imagenorm/imagenorm.go:103 — Normalize(r io.Reader, opts Options) takes no context.Context, so once wired to an HTTP handler, decoding an untrusted, WASM-interpreted HEIC (or a large PNG/webp under the 100 MP cap) can't be cancelled. Verified in cmd/pansy/main.go:60-67 that the server sets WriteTimeout: 30s, but http.Server's WriteTimeout only closes the client connection — it does not cancel the in-flight handler goroutine, which will keep burning CPU/decoding to co…
🪰 Gadfly · advisory
🟠 **Doc contract overpromises: read/encode I/O errors don't actually map to the documented ErrUnsupported/ErrTooLarge sentinels**
_error-handling, security · flagged by 3 models_
- **`internal/imagenorm/imagenorm.go:103`** — `Normalize(r io.Reader, opts Options)` takes no `context.Context`, so once wired to an HTTP handler, decoding an untrusted, WASM-interpreted HEIC (or a large PNG/webp under the 100 MP cap) can't be cancelled. Verified in `cmd/pansy/main.go:60-67` that the server sets `WriteTimeout: 30s`, but `http.Server`'s `WriteTimeout` only closes the client connection — it does not cancel the in-flight handler goroutine, which will keep burning CPU/decoding to co…
<sub>🪰 Gadfly · advisory</sub>
🟠Missing panic recovery around image decoders exposes goroutine DoS from malicious uploads
security · flagged by 2 models
internal/imagenorm/imagenorm.go:116-127 — Normalize does not recover from panics inside image.DecodeConfig or image.Decode. Third-party decoders (especially the WASM-backed HEIC path) are complex C-in-WASM parsers and historically have had crash-on-malformed-input bugs. A single hostile upload can crash the processing goroutine. Since the PR explicitly frames this as a hostile-upload boundary, add a defer recover() guard around the decode phase and map recovered panics to `ErrUns…
🪰 Gadfly · advisory
🟠 **Missing panic recovery around image decoders exposes goroutine DoS from malicious uploads**
_security · flagged by 2 models_
- **`internal/imagenorm/imagenorm.go:116-127`** — `Normalize` does not recover from panics inside `image.DecodeConfig` or `image.Decode`. Third-party decoders (especially the WASM-backed HEIC path) are complex C-in-WASM parsers and historically have had crash-on-malformed-input bugs. A single hostile upload can crash the processing goroutine. Since the PR explicitly frames this as a hostile-upload boundary, add a `defer recover()` guard around the decode phase and map recovered panics to `ErrUns…
<sub>🪰 Gadfly · advisory</sub>
🔴 *Integer overflow in decompression-bomb guard: int64(w)int64(h) wraps negative for crafted ~2^32-dimension headers, bypassing the maxDecodePixels check and allowing an OOM/panic in image.Decode instead of ErrTooLarge
error-handling, security · flagged by 3 models
internal/imagenorm/imagenorm.go:120 — Integer-overflow bypass of the decompression-bomb guard. int64(cfg.Width)*int64(cfg.Height) wraps negative for adversarial headers (e.g. a PNG IHDR claiming ~2³² on each edge: (2³²−1)² = 2⁶⁴ − 2³³ + 1, which overflows int64 to ≈ −8.59×10⁹), so the > maxDecodePixels check is false and image.Decode proceeds with those claimed dimensions. The guard's whole purpose (documented at lines 58–61, 114–115) is to stop exactly this; it is bypassab…
🪰 Gadfly · advisory
🔴 **Integer overflow in decompression-bomb guard: int64(w)*int64(h) wraps negative for crafted ~2^32-dimension headers, bypassing the maxDecodePixels check and allowing an OOM/panic in image.Decode instead of ErrTooLarge**
_error-handling, security · flagged by 3 models_
- **`internal/imagenorm/imagenorm.go:120`** — Integer-overflow bypass of the decompression-bomb guard. `int64(cfg.Width)*int64(cfg.Height)` wraps negative for adversarial headers (e.g. a PNG IHDR claiming ~2³² on each edge: `(2³²−1)²` = `2⁶⁴ − 2³³ + 1`, which overflows `int64` to ≈ −8.59×10⁹), so the `> maxDecodePixels` check is `false` and `image.Decode` proceeds with those claimed dimensions. The guard's whole purpose (documented at lines 58–61, 114–115) is to stop exactly this; it is bypassab…
<sub>🪰 Gadfly · advisory</sub>
🟠EXIF orientation is discarded on decode/re-encode, causing portrait phone photos to be uploaded sideways
correctness, security · flagged by 2 models
EXIF orientation is dropped during normalization — internal/imagenorm/imagenorm.go:124-134
🪰 Gadfly · advisory
🟠 **EXIF orientation is discarded on decode/re-encode, causing portrait phone photos to be uploaded sideways**
_correctness, security · flagged by 2 models_
**EXIF orientation is dropped during normalization** — `internal/imagenorm/imagenorm.go:124-134`
<sub>🪰 Gadfly · advisory</sub>
🟡 *downscale() always targets image.RGBA, forcing jpeg.Encode's slow generic per-pixel path instead of the YCbCr fast path (package has no caller yet in this diff)
performance · flagged by 1 model
internal/imagenorm/imagenorm.go:154 (downscale) — When an image needs resizing, downscale draws into a freshly allocated *image.RGBA (dst := image.NewRGBA(...), draw.CatmullRom.Scale(dst, ...), confirmed at lines 154–155), and that *image.RGBA is what Normalize hands to jpeg.Encode at line 132. Go's stdlib JPEG encoder only has a bulk-plane fast path for *image.YCbCr and *image.Gray; any other concrete type — including *image.RGBA — falls back to a generic per-pixel pat…
🪰 Gadfly · advisory
🟡 **downscale() always targets *image.RGBA, forcing jpeg.Encode's slow generic per-pixel path instead of the YCbCr fast path (package has no caller yet in this diff)**
_performance · flagged by 1 model_
- `internal/imagenorm/imagenorm.go:154` (`downscale`) — When an image needs resizing, `downscale` draws into a freshly allocated `*image.RGBA` (`dst := image.NewRGBA(...)`, `draw.CatmullRom.Scale(dst, ...)`, confirmed at lines 154–155), and that `*image.RGBA` is what `Normalize` hands to `jpeg.Encode` at line 132. Go's stdlib JPEG encoder only has a bulk-plane fast path for `*image.YCbCr` and `*image.Gray`; any other concrete type — including `*image.RGBA` — falls back to a generic per-pixel pat…
<sub>🪰 Gadfly · advisory</sub>
internal/imagenorm/imagenorm_test.go:89 — TestNormalizeDownscales silently couples itself to DefaultMaxDim. It passes Options{} and then hardcodes expected dimensions (2048, 512). If DefaultMaxDim is ever changed, this test will fail with opaque assertions that don't reveal the root cause. Fix: Make the dependency explicit by passing Options{MaxDim: 2048} (or compute expectations from DefaultMaxDim directly).
🪰 Gadfly · advisory
🟡 **Test silently depends on DefaultMaxDim constant**
_maintainability · flagged by 1 model_
- `internal/imagenorm/imagenorm_test.go:89` — `TestNormalizeDownscales` silently couples itself to `DefaultMaxDim`. It passes `Options{}` and then hardcodes expected dimensions (`2048`, `512`). If `DefaultMaxDim` is ever changed, this test will fail with opaque assertions that don't reveal the root cause. **Fix:** Make the dependency explicit by passing `Options{MaxDim: 2048}` (or compute expectations from `DefaultMaxDim` directly).
<sub>🪰 Gadfly · advisory</sub>
🟡image.DecodeConfig error ignored in small-image downscale test
error-handling · flagged by 1 model
internal/imagenorm/imagenorm_test.go:111 — image.DecodeConfig error discarded. In TestNormalizeDownscales the small-image branch does cfg, _, _ = image.DecodeConfig(bytes.NewReader(out)). If the normalized JPEG is somehow corrupt, the test reports a misleading dimension mismatch instead of the real decode failure. Check the error with t.Fatalf like the big-image branch does.
🪰 Gadfly · advisory
🟡 **image.DecodeConfig error ignored in small-image downscale test**
_error-handling · flagged by 1 model_
* **`internal/imagenorm/imagenorm_test.go:111`** — `image.DecodeConfig` error discarded. In `TestNormalizeDownscales` the small-image branch does `cfg, _, _ = image.DecodeConfig(bytes.NewReader(out))`. If the normalized JPEG is somehow corrupt, the test reports a misleading dimension mismatch instead of the real decode failure. Check the error with `t.Fatalf` like the big-image branch does.
<sub>🪰 Gadfly · advisory</sub>
🟠TestNormalizeRejectsGarbage has a stale comment header describing a non-existent TestNormalizeRejectsBombDimensions; the decompression-bomb (maxDecodePixels) path is untested
correctness, error-handling, maintainability · flagged by 5 models
internal/imagenorm/imagenorm_test.go:127-130 — The doc comment above TestNormalizeRejectsGarbage is stale: it describes a TestNormalizeRejectsBombDimensions scenario ("a small file claiming a huge canvas is refused before the bitmap is allocated... the guard is on the DECODED pixel count") but the function's body only exercises garbage bytes and a truncated PNG — it never constructs an oversized-DecodeConfig fixture. No test anywhere in the file (or package) actually drives the `maxDec…
🪰 Gadfly · advisory
🟠 **TestNormalizeRejectsGarbage has a stale comment header describing a non-existent TestNormalizeRejectsBombDimensions; the decompression-bomb (maxDecodePixels) path is untested**
_correctness, error-handling, maintainability · flagged by 5 models_
- `internal/imagenorm/imagenorm_test.go:127-130` — The doc comment above `TestNormalizeRejectsGarbage` is stale: it describes a `TestNormalizeRejectsBombDimensions` scenario ("a small file claiming a huge canvas is refused before the bitmap is allocated... the guard is on the DECODED pixel count") but the function's body only exercises garbage bytes and a truncated PNG — it never constructs an oversized-`DecodeConfig` fixture. No test anywhere in the file (or package) actually drives the `maxDec…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 12 findings (6 with multi-model agreement)
Finding
Where
Models
Lens
🟠
TestNormalizeRejectsGarbage has a stale comment header describing a non-existent TestNormalizeRejectsBombDimensions; the decompression-bomb (maxDecodePixels) path is untested
internal/imagenorm/imagenorm_test.go:127
5/5
correctness, error-handling, maintainability
🟡
Sentinel errors use fmt.Errorf instead of errors.New, diverging from repo convention
internal/imagenorm/imagenorm.go:90
4/5
maintainability
🔴
Integer overflow in decompression-bomb guard: int64(w)*int64(h) wraps negative for crafted ~2^32-dimension headers, bypassing the maxDecodePixels check and allowing an OOM/panic in image.Decode instead of ErrTooLarge
internal/imagenorm/imagenorm.go:120
3/5
error-handling, security
🟠
Doc contract overpromises: read/encode I/O errors don't actually map to the documented ErrUnsupported/ErrTooLarge sentinels
internal/imagenorm/imagenorm.go:101
3/5
error-handling, security
🟠
Missing panic recovery around image decoders exposes goroutine DoS from malicious uploads
internal/imagenorm/imagenorm.go:116
2/5
security
🟠
EXIF orientation is discarded on decode/re-encode, causing portrait phone photos to be uploaded sideways
internal/imagenorm/imagenorm.go:124
2/5
correctness, security
6 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
maxDecodePixels (100 MP) permits ~400 MB peak allocation per upload — memory-exhaustion DoS surface, and the huge intermediate bitmap is discarded since MaxDim=2048 downscales everything larger
internal/imagenorm/imagenorm.go:61
opencode/glm-5.2:cloud
maintainability, security
🟡
downscale() always targets *image.RGBA, forcing jpeg.Encode's slow generic per-pixel path instead of the YCbCr fast path (package has no caller yet in this diff)
internal/imagenorm/imagenorm.go:154
claude-code/sonnet
performance
🟡
Test silently depends on DefaultMaxDim constant
internal/imagenorm/imagenorm_test.go:89
kimi-k2.6:cloud
maintainability
🟡
image.DecodeConfig error ignored in small-image downscale test
internal/imagenorm/imagenorm_test.go:111
opencode/kimi-k2.6:cloud
error-handling
🟡
Binary test fixtures lack provenance documentation in repository
internal/imagenorm/testdata:1
opencode/kimi-k2.6:cloud
maintainability
⚪
Package doc references non-existent TestNormalize; actual test is TestNormalizeAllFormats
internal/imagenorm/imagenorm.go:25
opencode/glm-5.2:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
This PR adds a self-contained imagenorm package that isn't imported anywhere yet (verified: no non-test caller in the tree), so there is no live attack surface today — the actual exposure will appear when a future handler wires an upload endpoint to Normalize. Reviewing the function as written for what it will face once that happens:
Good: input is bounded before read (io.LimitReader(r, maxBytes+1), imagenorm.go:105), and the decoded-pixel-count guard runs from image.DecodeConfig before the full bitmap is allocated (imagenorm.go:116-122), which is the right order to defend against a decompression bomb.
internal/imagenorm/imagenorm.go:103 — Normalize(r io.Reader, opts Options) takes no context.Context, so once wired to an HTTP handler, decoding an untrusted, WASM-interpreted HEIC (or a large PNG/webp under the 100 MP cap) can't be cancelled. Verified in cmd/pansy/main.go:60-67 that the server sets WriteTimeout: 30s, but http.Server's WriteTimeout only closes the client connection — it does not cancel the in-flight handler goroutine, which will keep burning CPU/decoding to completion regardless. This isn't exploitable today since nothing calls Normalize yet, but it's a gap worth closing (accept/honor a context.Context, or have the future handler impose a deadline) before this is wired to a real upload endpoint.
Nothing else in this diff touches auth, injection surfaces, secrets, or deserialization of anything other than image pixel data through Go's own image package machinery, which is the intended defense here.
🎯 Correctness — Minor issues
The finding checks out against the actual source. internal/imagenorm/imagenorm.go decodes with the stdlib image.Decode (which does not read EXIF Orientation) and re-encodes with image/jpeg's jpeg.Encode (which writes no EXIF at all) — confirmed lines 124–135. A repo-wide grep for exif/orientation (case-insensitive, all .go files) returns nothing, and imagenorm is only referenced by its own package and test file, so there's no caller-side rotation correction either. Verdict and finding both survive.
Verdict: Minor issues
EXIF orientation is dropped during normalization — internal/imagenorm/imagenorm.go:124-134
Normalize decodes with image.Decode and re-encodes with image/jpeg's jpeg.Encode. Go's standard image/jpeg decoder does not read or apply the EXIF Orientation tag — it decodes only the raw pixel matrix. jpeg.Encode writes no EXIF data into its output. There is no EXIF-reading import anywhere in this package or the rest of the repo (confirmed via repo-wide grep for exif/orientation), and imagenorm is called from no other file yet, so there is no caller-side correction either.
Concrete failure: many cameras — most Android phones, and many third-party iOS camera/sharing apps that re-encode HEIC to JPEG before an upload flow — store a photo taken in portrait as landscape pixel data plus an Orientation tag (e.g. 6/8) rather than physically rotating the pixels. After Normalize, that tag is silently discarded and the pixel data is re-encoded unrotated, so a packet photo taken in portrait comes out sideways (or upside-down/mirrored for orientations 3/2/4/5/7). Since the PR's stated goal is "everything downstream (majordomo's vision path) only ever sees JPEG" — i.e. this normalized output is what the model reads — a mis-rotated packet photo directly degrades the seed-packet-capture feature's primary use case, not just a cosmetic display issue.
HEIC is likely less affected in practice (HEIF containers commonly apply irot/imir transform properties at decode time rather than relying on a separate orientation tag, so gen2brain/heic's output pixels are plausibly already correctly oriented — unverified here, no network/vendor access to confirm), but this doesn't help the JPEG/PNG-from-Android or re-encoded-JPEG case, which is very plausibly the majority of non-iPhone camera input.
Suggested fix: read the EXIF Orientation tag from the raw JPEG bytes before/alongside decode and apply the corresponding rotate/flip to img before the final JPEG encode.
Everything else checked out:
Byte cap (imagenorm.go:104-112): LimitReader(MaxBytes+1) + len(raw) > maxBytes correctly distinguishes "exactly at cap" from "over."
Decompression-bomb guard (imagenorm.go:116-122): pixel-count check runs on DecodeConfig before the full image.Decode allocates a bitmap.
Downscale math (imagenorm.go:142-157): scale/nw/nh derivation is consistent, small images are left untouched, and the longest == 0 guard avoids a divide-by-zero.
Error mapping (ErrUnsupported vs ErrTooLarge) is applied consistently.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the real source. Confirms no other package uses fmt.Errorf without a verb, and confirms TestNormalizeRejectsGarbage's doc comment is indeed a leftover description of a bomb-dimensions test that doesn't exist, with no other test in the file or elsewhere covering maxDecodePixels.
VERDICT: Minor issues
internal/imagenorm/imagenorm_test.go:127-130 — The doc comment above TestNormalizeRejectsGarbage is stale: it describes a TestNormalizeRejectsBombDimensions scenario ("a small file claiming a huge canvas is refused before the bitmap is allocated... the guard is on the DECODED pixel count") but the function's body only exercises garbage bytes and a truncated PNG — it never constructs an oversized-DecodeConfig fixture. No test anywhere in the file (or package) actually drives the maxDecodePixels branch at imagenorm.go:120-122, so the comment misleads a future reader into believing that guard is covered when it isn't.
internal/imagenorm/imagenorm.go:90-91 — ErrTooLarge/ErrUnsupported use fmt.Errorf with no formatting verb, where errors.New is called for elsewhere. Every sentinel error in internal/domain/domain.go (14 instances) uses errors.New, and this file itself uses fmt.Errorf correctly for wrapping at lines 108 and 133 with %w. This is the only place in internal/ that declares a plain sentinel with fmt.Errorf.
⚡ Performance — Minor issues
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:154 (downscale) — When an image needs resizing, downscale draws into a freshly allocated *image.RGBA (dst := image.NewRGBA(...), draw.CatmullRom.Scale(dst, ...), confirmed at lines 154–155), and that *image.RGBA is what Normalize hands to jpeg.Encode at line 132. Go's stdlib JPEG encoder only has a bulk-plane fast path for *image.YCbCr and *image.Gray; any other concrete type — including *image.RGBA — falls back to a generic per-pixel path that calls .At(x, y) and does an RGBA→YCbCr conversion per pixel. This is well-established, widely documented Go stdlib behavior; I was not able to re-read image/jpeg/writer.go directly in this sandbox (out-of-repo file reads are blocked here too), so, like the original author, I'm not treating it as tool-verified against the stdlib source itself — confidence stays medium on that specific sub-claim.
One correction to the draft's framing: this PR only addsinternal/imagenorm and its test — a repo-wide grep turns up zero callers of imagenorm.Normalize outside the package itself. So the "upload hot path" / "essentially 100% of real-world uploads" language overstates the current, verifiable impact of this diff — there is no wired-up caller yet for this to cost CPU on. The underlying code observation (resize path always targets RGBA, forcing the slow encode path once this is wired to an upload handler) still holds and is worth flagging before the package gets its first caller.
Suggested fix (before wiring this into a request path): if downscaled-photo encode latency matters once there's a real caller, encode via a *image.YCbCr target instead of *image.RGBA (e.g. image.NewYCbCr + a scaler that can write into it, or convert post-scale) to hit the encoder's fast path.
🧯 Error handling & edge cases — Minor issues
Confirmed both findings are accurate. There is no other test elsewhere in the repo exercising the maxDecodePixels/bomb-dimensions branch, and the package isn't yet wired into any caller, so the doc-contract mismatch is real (no other file's error handling masks it). Both findings survive verification unchanged.
Corrected review
VERDICT: Minor issues
Reviewed internal/imagenorm/imagenorm.go and its test file. The core guards (byte cap, pre-decode pixel-count check, graceful ErrUnsupported on garbage/truncated input, no re-sampling on already-small images) are implemented correctly — no panics on empty/degenerate dimensions, no unguarded type assertions, negative/zero Options fields fall back safely to defaults (imagenorm.go:73-85).
internal/imagenorm/imagenorm.go:99-102 — the doc comment states "A read/format error maps to ErrUnsupported," but that's only true for the format-decode branch (imagenorm.go:117-118, 125-126). A genuine I/O failure from io.ReadAll (imagenorm.go:106-109) returns a distinct wrapped error (fmt.Errorf("imagenorm: read: %w", err)), not ErrUnsupported, and the jpeg.Encode failure path (imagenorm.go:132-134) likewise returns an unwrapped-from-sentinel error. A caller relying on errors.Is(err, ErrUnsupported) per the documented contract will silently miscategorize these two paths. Fix: either wrap read/encode failures so they satisfy a documented sentinel, or correct the doc to describe the actual error shape.
internal/imagenorm/imagenorm_test.go:127-130 — the comment above TestNormalizeRejectsGarbage is titled TestNormalizeRejectsBombDimensions and describes "a small file claiming a huge canvas... refused before the bitmap is allocated," but the function it labels only tests garbage bytes and a truncated PNG (imagenorm_test.go:131-140) — it never constructs an input that trips the maxDecodePixels/ErrTooLarge branch at imagenorm.go:120-122. Confirmed via repo-wide grep: no test anywhere exercises that branch. The decompression-bomb guard — the safety property called out as the point of the pixel-count pre-check — has zero test coverage. Fix: add the actual bomb-dimensions test the comment describes (e.g. a spoofed IHDR/DecodeConfig claiming a huge width/height) so a regression in that branch (e.g. > flipped to >=) would be caught.
The code confirms the first finding. maxDecodePixels = 100_000_000 (line 61), the check at line 120 uses strict >, so exactly 100,000,000 px (e.g. 10000×10000) passes and decodes to a ~400 MB RGBA bitmap, while a flat-color PNG of that size is only kilobytes on disk — well under the 25 MiB input cap. downscale then allocates an additional RGBA plus the JPEG output buffer on top. No concurrency bound exists in the package. This is a real memory-amplification DoS vector at an untrusted upload boundary.
The second finding is explicitly low-confidence defense-in-depth (and not in the JSON block); the draft itself notes it couldn't confirm a bypass. Dropping it.
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:120 — The maxDecodePixels guard (100 MP) is too permissive and undermines the decompression-bomb protection it claims to provide. PNG is a pixel-raster format whose byte size is decoupled from its decoded bitmap size: a flat-color 10000×10000 image is ~kilobytes on disk but decodes to a 10000×10000×4 = 400 MB RGBA bitmap. The input cap (25 MiB) and the DecodeConfig check don't prevent this, because the byte count is tiny and the dimension check uses > (strictly greater), so exactly 100,000,000 px passes. So a hostile-but-valid ~KB PNG forces a ~400 MB allocation per request — a ~40,000× memory amplification — and nothing in this package bounds concurrent decodes. On top of that, downscale allocates a second RGBA (up to 2048²×4 ≈ 16 MB) plus the JPEG output buffer. For an upload boundary ingesting untrusted photos, that's a DoS vector. The fix is to lower maxDecodePixels to something tied to the actual use case (a real phone photo is ~12 MP; 24 MP is generous) rather than "any photo ever." Verified the math against downscale/jpeg.Encode (which allocate the RGBA + output buffer on top of the decoded image) by reading imagenorm.go:103-157.
🎯 Correctness — No material issues found
I've verified the two draft findings against the actual code and Go's image/draw semantics. Here is my assessment.
Finding 1 (imagenorm.go:155, draw.Over vs draw.Src): I read the code (lines 142-157): downscale allocates dst := image.NewRGBA(...) — a freshly-zeroed, fully-transparent destination (all 0,0,0,0) — then calls draw.CatmullRom.Scale(dst, dst.Bounds(), img, b, draw.Over, nil).
The draft's central actionable claim is that switching draw.Over → draw.Src would fix the alleged darkening. I cannot confirm that, and in fact it is contradicted by Porter-Duff semantics: with a destination whose alpha is zero, Over reduces to result = src (premultiplied), and Src is defined as result = src (premultiplied). The two operators produce byte-identical output on a zero/transparent destination — the draft even states this ("over a zero dst gives dstRGB = srcRGB*srcA") but then claims draw.Src would instead "copy the original RGB verbatim." That is incorrect: draw.Src writes the same premultiplied value that draw.Over writes here. So the recommended fix is a no-op and the finding's technical premise does not hold. Whether any darkening occurs downstream depends entirely on how image/jpeg reads a premultiplied *image.RGBA (stdlib code not present in this repo, so I cannot positively confirm or refute it), but the proposed remedy would not address it regardless. Per the gate rule (drop what I cannot confirm, and drop findings the code contradicts), this is dropped.
Finding 2 (imagenorm.go:99-104, doc prose "reads at most opts.MaxBytes"): Verified: the comment at line 99 says "It reads at most opts.MaxBytes," and the code at lines 104-105 reads via LimitReader(r, maxBytes()+1) then rejects if len(raw) > maxBytes(). The +1 is an intentional boundary-detection detail; the function never accepts more than opts.MaxBytes for processing, so the contract-level statement is defensible. This is a trivial prose nuance at most, and not a material issue.
No surviving material findings.
VERDICT: No material issues found
internal/imagenorm/imagenorm.go:155 — examined the downscale compositing. The destination is a freshly-allocated, fully-transparent *image.RGBA (image.NewRGBA). Under Porter-Duff rules, draw.Over over a zero-alpha destination and draw.Src both reduce to result = src (premultiplied) and produce identical Pix bytes. Therefore the draft's claim that switching to draw.Src would "copy the original RGB verbatim" and fix darkening is not supported by the operator semantics; the suggested fix would be a no-op. Any downstream darkening (if it exists) would depend on image/jpeg's reading of a premultiplied *image.RGBA, which cannot be verified from this repository, and is not addressed by draw.Src regardless. Dropped.
internal/imagenorm/imagenorm.go:99 — the comment says "It reads at most opts.MaxBytes"; the implementation reads maxBytes()+1 to distinguish at-cap from over-cap, then rejects anything over maxBytes(). The accepted/processed input is in fact capped at opts.MaxBytes, so the contract-level statement holds; the +1 is an intentional implementation detail. Trivial at most, not material. Dropped.
I also re-derived the downscale math for the 4000×1000 test case against downscale (lines 142-157): longest=4000, scale=2048/4000=0.512, nw=int(2048.0)=2048, nh=int(512.0)=512, matching the asserted 2048×512 with aspect preserved. The byte-cap logic (LimitReader(maxBytes+1) then len > maxBytes) correctly distinguishes exactly-at-cap from over-cap, and the bomb guard (cfg.Width*cfg.Height > maxDecodePixels at line 120) is correctly ordered before the full image.Decode at line 124.
🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed against the actual source.
VERDICT: Minor issues
imagenorm_test.go:127 — mismatched test name and doc comment. The comment at lines 127–129 says TestNormalizeRejectsBombDimensions: a small file claiming a huge canvas... but the function declared at line 130 is TestNormalizeRejectsGarbage, which only feeds garbage/truncated bytes ("not an image at all" and a halved PNG) and asserts ErrUnsupported. The maxDecodePixels decompression-bomb path (imagenorm.go:120, which returns ErrTooLarge for a huge claimed canvas) is never exercised by any test in the file (confirmed by grep — no other test references maxDecodePixels or constructs an oversize-canvas fixture). The stale comment claims a guard test that does not exist. Fix: either add a TestNormalizeRejectsBombDimensions that feeds a PNG with a huge declared canvas under the byte cap, or reword the comment to describe what TestNormalizeRejectsGarbage actually does.
imagenorm.go:90-91 — exported sentinel errors built via fmt.Errorf rather than errors.New.fmt.Errorf("imagenorm: image too large") and fmt.Errorf("imagenorm: unsupported or corrupt image") contain no verbs, so errors.New(...) is the idiomatic choice. Cosmetic, but inconsistent with the otherwise careful style of the surrounding code.
⚡ Performance — No material issues found
VERDICT: No material issues found
This is new code on the upload boundary (a cold path processing one image at a time), and the resource costs are bounded and deliberate. Reviewed through the performance lens:
Double header parse (internal/imagenorm/imagenorm.go:116 and :124): DecodeConfig and image.Decode each get a fresh bytes.Reader over the same in-memory raw, so the format header is parsed twice. This is negligible relative to the full bitmap decode/resample/encode that follows, and the two-pass structure is what enables the pre-allocation decompression-bomb guard. Not worth changing.
Full read into memory (:105–:106): up to MaxBytes (25 MiB) is buffered entirely before decoding, rather than streaming. This is by design (two passes over the bytes for the size guard), and the cap keeps the transient footprint bounded. Acceptable.
draw.CatmullRom.Scale (:155): Catmull-Rom is the costliest kernel scaler in x/image/draw, but the PR justifies it for text sharpness, it only runs when an image actually exceeds MaxDim, and it's off the hot path. A deliberate quality/speed trade-off, not a regression.
HEIC via wazero: running libheif as WASM is markedly slower than a native codec, but (a) it's the only CGO-free option and (b) it runs per-upload, not in a loop. Not a material concern for this path.
Decoded-bitmap bound (maxDecodePixels = 100MP ≈ 400 MB at 4 B/px): the cap is generous and a 100 MP image that compresses under 25 MiB could force a large transient allocation. This is acknowledged in the code comments and is a design choice on a cold path, not an inefficiency to flag.
No unbounded growth, N+1 patterns, hot-loop regressions, or avoidable quadratic behavior were found.
🧯 Error handling & edge cases — No material issues found
I've verified the error-handling paths against the checked-out code. The implementation is solid in my lane.
Verification notes:
Byte-cap boundary (imagenorm.go:104-112): LimitReader(r, max+1) then len(raw) > maxBytes() correctly distinguishes "exactly at cap" (accepted) from "over" (ErrTooLarge). Read errors are wrapped, not swallowed (:108).
Decompression-bomb guard (:116-122): DecodeConfig dimensions checked in int64 arithmetic (no overflow for MaxInt32-scale dims) beforeimage.Decode allocates the bitmap. Correct ordering.
Garbage/truncated path (:116-127): both DecodeConfig and Decode failures map to ErrUnsupported; format from the successful DecodeConfig is carried through on the partial-failure path. No panic surface introduced by the stdlib decoders.
downscale (:142-157): zero-dimension guarded (longest == 0 → return unchanged); max(..., 1) prevents a zero-height/width NewRGBA when the short edge rounds down to 0 (e.g. 2049×1 → nh=1). Scaling math can't overflow int: w*h ≤ maxDecodePixels (100M) is enforced upstream, and scale < 1 in the downscale branch, so nw,nh ≤ w,h ≤ 100M — well within int32.
Negative Options.MaxDim/MaxBytes fall through to defaults via the > 0 checks (:73-85), so a misconfigured caller can't drive a zero/negative cap into LimitReader or the pixel check.
One thing I could not locally verify (heic source isn't vendored in the repo): whether github.com/gen2brain/heic registers image.DecodeConfig such that the bomb guard at :116 runs for HEIC before image.Decode. If it only registered Decode, every HEIC would fail at :117 with ErrUnsupported — but the PR's TestNormalizeAllFormats heic case exercises exactly this path and is reported green, which is strong evidence the registration is correct. Not a finding against the reviewed code.
No material issues found in the error-handling & edge-case lens.
internal/imagenorm/imagenorm.go:116-127 — Normalize does not recover from panics inside image.DecodeConfig or image.Decode. Third-party decoders (especially the WASM-backed HEIC path) are complex C-in-WASM parsers and historically have had crash-on-malformed-input bugs. A single hostile upload can crash the processing goroutine. Since the PR explicitly frames this as a hostile-upload boundary, add a defer recover() guard around the decode phase and map recovered panics to ErrUnsupported so a bad file is a clean error, not a goroutine (or process) kill.
🎯 Correctness — No material issues found
No material issues found
I scrutinized the normalization logic, dimensions, thresholds, and format handling:
Byte cap / LimitReader logic: io.LimitReader(r, int64(opts.maxBytes())+1) followed by len(raw) > opts.maxBytes() correctly distinguishes “exactly at the cap” from “over the cap” and prevents unbounded reads. Edge cases (empty reader, exact-size input) behave correctly.
Decompression-bomb guard: int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels uses int64 to avoid overflow on the product, and the check happens before image.Decode allocates the bitmap. The threshold (100 MP ≈ 400 MB RGBA) is consistent with the stated intent.
Downscale math: scale := float64(maxDim) / float64(longest) with max(int(float64(w)*scale), 1) preserves aspect ratio and guarantees at least a 1×1 output. The default MaxDim=2048 keeps the post-scale image well under the pixel cap, and the 4000×1000 test case evaluates to exactly 2048×512 with IEEE-754 double precision, so the aspect test is deterministic.
Format coverage / registration: The four blank imports (image/jpeg, image/png, github.com/gen2brain/heic, golang.org/x/image/webp) register with image.Decode by side effect. The test TestNormalizeAllFormats round-trips every claimed format and would catch a dropped import.
Error handling: Truncated or garbage input fails at DecodeConfig/Decode and is mapped cleanly to ErrUnsupported. Oversize bytes and oversize dimensions both map to ErrTooLarge. No panic paths were found.
JPEG re-encode: jpeg.Encode with quality 85 on an *image.RGBA destination is well-formed; the output is validated in tests.
Nothing in the diff introduces a logic bug, unit mismatch, boundary error, or semantic domain issue.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm_test.go:127 — Comment names TestNormalizeRejectsBombDimensions but the actual function is TestNormalizeRejectsGarbage, violating the standard Go convention that a comment should introduce the function it precedes. Worse, the described decompression-bomb scenario (a small file claiming huge decoded dimensions) is never exercised in the test body; only garbage bytes and truncated PNGs are tested. A future maintainer will reasonably believe the bomb guard has direct regression coverage when it does not. Fix: Rename the test to match the comment (or vice-versa) and add an actual bomb-dimension test case—e.g. a minimal crafted header with a huge Width/Height—so the safety feature is locked.
internal/imagenorm/imagenorm_test.go:89 — TestNormalizeDownscales silently couples itself to DefaultMaxDim. It passes Options{} and then hardcodes expected dimensions (2048, 512). If DefaultMaxDim is ever changed, this test will fail with opaque assertions that don't reveal the root cause. Fix: Make the dependency explicit by passing Options{MaxDim: 2048} (or compute expectations from DefaultMaxDim directly).
⚡ Performance — No material issues found
No material issues found
The code is well-bounded and avoids the common performance traps for an image-pipeline package:
Capped reads: io.LimitReader + io.ReadAll with DefaultMaxBytes = 25 MiB prevents unbounded memory consumption from a malicious stream.
DecodeConfig pre-check: maxDecodePixels = 100M is enforced before image.Decode allocates the bitmap, blocking decompression bombs.
Downscale before encode: The largest possible decoded bitmap that actually reaches jpeg.Encode is bounded by DefaultMaxDim = 2048, so the destination RGBA is at most ~16 MiB.
No redundant work in hot paths: downscale returns the original image unchanged when it already fits, avoiding an unnecessary CatmullRom resample.
One-shot usage: This is an upload-boundary function, not a hot loop; the bounded allocations (~25 MiB input + decoded image + destination + output buffer) are acceptable for per-request execution.
🧯 Error handling & edge cases — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:120 — Missing dimension positivity validation. cfg.Width or cfg.Height could be zero or negative from a malformed image or buggy third-party decoder. The product int64(cfg.Width)*int64(cfg.Height) then yields a negative or zero value that passes the > maxDecodePixels guard, allowing the decode to proceed. Fix: add if cfg.Width <= 0 || cfg.Height <= 0 { return nil, "", ErrUnsupported } before the pixel count check.
internal/imagenorm/imagenorm.go:111 vs imagenorm.go:121 — Inconsistent format return on ErrTooLarge. The byte-cap path returns "" for format, but the pixel-count path returns the detected format. Callers inspecting format when err != nil see different behavior depending on which limit triggered. Fix: return nil, "", ErrTooLarge in both cases for consistency.
internal/imagenorm/imagenorm.go:105 — Potential int64 overflow in io.LimitReader limit. If a caller passes Options{MaxBytes: math.MaxInt64}, int64(opts.maxBytes())+1 wraps to a negative limit, causing io.ReadAll to read nothing and the function to return ErrUnsupported instead of a meaningful error. Fix: avoid the +1 by reading at most maxBytes and detecting truncation separately (e.g., compare len(raw) to the limit, or read one extra byte with overflow-safe math).
opencode/glm-5.2:cloud (opencode) — Blocking issues found
The draft's single finding is confirmed against the code:
Line 61: maxDecodePixels = 100_000_000 ✓
Line 120: the guard checks > maxDecodePixelsbefore full decode, so up to 100 MP is allowed ✓
Line 124: image.Decode then allocates the full bitmap — 100 MP × 4 bytes/px (RGBA) = ~400 MB ✓
Line 53/129: DefaultMaxDim = 2048 and downscale discards anything larger, so the huge intermediate serves no functional purpose ✓
The byte cap (25 MiB input) does not bound decoded size for low-entropy/compressible payloads ✓
No callers wire imagenorm yet (grep confirms only the package itself references the name), so this is latent attack surface rather than a live endpoint, but the constants and decode semantics are as the draft describes. The HEIC DecodeConfig note in the draft is explicitly not a finding, so nothing to verify there.
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:61 — maxDecodePixels (100 MP) permits ~400 MB peak allocation per upload. The guard at line 120 rejects only more than 100 MP, so a decoded bitmap up to 100 MP is allowed through to image.Decode (line 124). At 4 bytes/px (RGBA) that is ~400 MB allocated before the downscale step even runs. A hostile, low-entropy JPEG/HEIC well under the 25 MiB input cap (line 57) can legitimately decode to that size — the byte cap does not bound decoded size for compressible payloads. Since DefaultMaxDim = 2048 (line 53) and downscale (line 129) immediately throw away and re-sample anything larger on its longest edge, a 400 MB intermediate bitmap serves no functional purpose and is pure attack surface: one request can force ~400 MB allocation, and concurrent uploads multiply it. The guard exists but the ceiling is far too generous for the actual use. Suggested fix: tie the pixel cap to the output need (e.g. maxDecodePixels = maxDim*maxDim * 4 ≈ 16 MP at default, still ample headroom for Catmull-Rom downscaling) so a hostile upload cannot force an allocation many times larger than anything that survives normalization. Verified by reading the code; the 25 MiB → 100 MP → 400 MB path follows directly from the constants and image.Decode semantics.
🎯 Correctness — Minor issues
Confirmed. Comment at line 127 says TestNormalizeRejectsBombDimensions but function at line 130 is TestNormalizeRejectsGarbage. The body tests garbage + truncated PNG only, never the maxDecodePixels guard. Verified.
VERDICT: Minor issues
Verified by reading /tmp/gadfly/repo/internal/imagenorm/imagenorm.go and imagenorm_test.go in full.
internal/imagenorm/imagenorm_test.go:127-130 — misleading/stale doc comment; the decompression-bomb guard is untested. The comment block says TestNormalizeRejectsBombDimensions: a small file claiming a huge canvas is refused before the bitmap is allocated, but the function actually defined immediately below is TestNormalizeRejectsGarbage, which only feeds in "not an image at all" and a truncated PNG. Neither case exercises the int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels check at imagenorm.go:120 — there is no test anywhere that constructs a small file with huge claimed dimensions to confirm the guard rejects it. The maxDecodePixels guard is the central safety property the PR description advertises ("decompression-bomb guard"), and it has zero coverage. A maintainer reading the comment would believe the guard is tested when it is not. Suggested fix: add a real bomb-dimension case (e.g. a crafted PNG/HEIC header claiming huge dimensions within the byte cap) asserting ErrTooLarge, and fix the comment to match the function name.
The downscale math, byte-cap logic, aspect preservation, format-string handling, and the four-format round-trip assertions all check out on re-derivation (4000×1000 → 2048×512; small images untouched; oversize input → ErrTooLarge; format from DecodeConfig is preserved through the Decode call).
🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual checked-out files.
Now let me verify the convention claims about sentinel errors elsewhere in the repo.
Now let me verify the maxBytes() call count claim in finding #4. The draft says "called three times" — let me count precisely.
Looking at the code:
Line 105: int64(opts.maxBytes())+1 — call 1
Line 110: len(raw) > opts.maxBytes() — call 2
Only two calls in Normalize, not three. The draft's "three times" claim is factually wrong.
All other findings verified:
fmt.Errorf for sentinels (lines 90-91) — CONFIRMED. Every other sentinel declaration in the repo uses errors.New (domain.go, password.go, agentmodel.go, runtime.go, journal.go). The other two fmt.Errorf("...") matches (revisions.go:713, instance_settings.go:41) are inline returns, not sentinel declarations.
Test comment/name mismatch (lines 127-130) — CONFIRMED. Comment says TestNormalizeRejectsBombDimensions: but function is TestNormalizeRejectsGarbage; body tests garbage + truncated input, never bomb dimensions.
Stale doc reference (line 25) — CONFIRMED. Package doc says TestNormalize exercises all four formats; no such function exists, actual test is TestNormalizeAllFormats.
maxDecodePixels not in Options (line 61) — CONFIRMED. It's an unexported const while MaxDim/MaxBytes are tunable via Options.
Finding #4 drops: the "three times" claim is false (it's two calls), and the underlying suggestion is trivial enough that the incorrect premise disqualifies it.
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:90-91 — fmt.Errorf for sentinel errors instead of errors.New. Every other sentinel error declaration in the repo uses errors.New (internal/domain/domain.go:14-64, internal/service/password.go:29, internal/agentmodel/agentmodel.go:39, internal/agent/runtime.go:52). fmt.Errorf("imagenorm: image too large") has no format verbs and pays a fmt-formatting cost for a constant string. errors.Is still works, but the package diverges from the established convention for no benefit. Use errors.New.
internal/imagenorm/imagenorm_test.go:127-130 — misleading comment / name mismatch. The doc comment for TestNormalizeRejectsGarbage leads with // TestNormalizeRejectsBombDimensions: — a function name that does not exist and describes a test the function does not perform (it never feeds a small-file/huge-dimension input; that case isn't covered anywhere). The body tests garbage + truncated input. Either rename to match the comment and add the bomb-dimensions case (a header claiming huge width/height is exactly what the maxDecodePixels guard is for, and currently has no coverage), or fix the comment to describe what the test actually does.
internal/imagenorm/imagenorm.go:25 — stale doc reference. The package doc says TestNormalize exercises all four formats — there is no TestNormalize; the actual test is TestNormalizeAllFormats. Minor, but the CLAUDE.md rule "stale comments are worse than none" applies. Update the reference.
internal/imagenorm/imagenorm.go:61 — maxDecodePixels is an unexported constant while MaxDim/MaxBytes are exposed via Options. The decompression-bomb pixel cap is a hard unexported constant, inconsistent with the "Options tunes Normalize" framing — a caller can't tighten it. That's a defensible safety choice, but the asymmetry is undocumented and looks accidental. A one-line comment explaining why it's intentionally not in Options would help the next reader.
⚡ Performance — No material issues found
I'll review this through the performance lens. Let me examine the actual code.
The file matches the diff. Now let me consider the performance profile of this code carefully.
The key concern from a performance lens:
1. Full file buffered into memory twice over.Normalize reads the entire input (up to 25 MiB) into raw via io.ReadAll, then passes bytes.NewReader(raw) to both image.DecodeConfig and image.Decode. The DecodeConfig→Decode pattern requires two passes, which is why the author buffers fully — that's a reasonable, even necessary, trade-off given the stdlib's image package re-reads from a Reader (which isn't seekable in the general case). But combined with the decoded bitmap (up to ~400 MB at the maxDecodePixels cap) and the output buffer, peak memory is raw + decoded_bitmap + dst_bitmap (if downscaled) + output. This is inherent to the design and bounded by the caps; not a regression.
2. Downscaling allocation when the image is already within bounds but re-encoded.downscale returns img unchanged when longest <= maxDim, so no extra RGBA allocation happens for in-bounds images. Good.
3. Catmull-Rom on a 2048×2048 RGBA destination. This is the most expensive part per-call but runs once per upload (a cold path — a user submitting a seed packet photo), not in a hot loop. Catmull-Rom is O(dst_pixels × kernel²) which is fine for a 4 MP output. Not a concern.
Let me check whether image.Decode for HEIC via wazero has any notable per-call cost (e.g. re-instantiating a WASM runtime per decode) that would matter on a request path.
The module cache isn't vendored/checked out, so I can't inspect the heic library source directly. Let me check if there's a vendor dir or go module cache accessible.
No vendor dir. The heic/wazero sources aren't available for me to inspect directly. Let me reason about what I can verify from the code itself.
The package isn't imported anywhere yet (per the PR description, #81 will import it), so this code is not on any current hot path. Let me confirm no current caller exists.
Confirmed — no production caller yet. This is library code that will be wired in by #81.
Now, the one performance concern worth flagging that I can verify from the code: the image.DecodeConfig → image.Decode double-parse of the full buffered bytes. For HEIC specifically, this means the wazero/libheif WASM pipeline parses the HEIC header twice. But more importantly, there's a subtler issue I want to verify: does image.DecodeConfig actually avoid the heavy work for all four formats? For JPEG/PNG it reads only headers; for HEIC via wazero, DecodeConfig may still instantiate the WASM runtime and parse the container. I can't verify the heic library's internals without its source, so I'll note this as unverified rather than assert it.
Let me look at whether there's a meaningful allocation concern in the bytes.NewReader(raw) wrapping — it's trivial (a small struct), so no.
Let me double-check one thing: the downscale output is *image.RGBA always, even when the source was already RGBA and within bounds — but in that case it returns img unchanged, so no copy. Good. And when downscaled, the JPEG encoder will read the RGBA — standard.
The one genuinely worth-flagging performance observation I can verify: the entire input is buffered into memory (raw, up to 25 MiB) even when it could be streamed. But this is necessary because image.DecodeConfig and image.Decode both consume the reader and the stdlib image package doesn't support seeking/peeking generically. So this is a justified trade-off, not a regression. I won't flag it.
Let me reconsider the maxDecodePixels = 100_000_000 cap from the performance lens: at 4 bytes/px (RGBA), that's a ~400 MB decoded bitmap ceiling per request. For an upload-boundary function on a single-user garden app, that's a large but bounded transient allocation. It's a documented, deliberate choice (the comment explains it). Not a regression — it's a guard. I won't flag it as a performance issue; if anything it's a memory-safety concern belonging to another lens.
I've verified everything I can from the checked-out code. The code is not on a hot path (no caller yet), the expensive operations (Catmull-Rom downscale, JPEG re-encode) run once per upload on a cold path, the downscale correctly skips work for in-bounds images, and the full-buffering is forced by the stdlib's two-pass decode pattern. The double parse (DecodeConfig then Decode) is a minor, unavoidable cost given the API shape.
VERDICT: No material issues found.
Buffered read + double parse (lines 106, 116, 124):Normalize reads the full input into raw (≤25 MiB) then runs image.DecodeConfig and image.Decode over bytes.NewReader(raw) — two parses of the same bytes. This is forced by the stdlib image package: a generic io.Reader isn't seekable, so to check dimensions before allocating the bitmap (the decompression-bomb guard) you must either buffer or re-open the source. Buffering is the correct choice here. The cost is bounded by DefaultMaxBytes and runs once per upload on a cold path (no caller imports this yet — confirmed via grep for imagenorm across the repo: only the package and its own tests reference it). Not a regression.
Downscale skips in-bounds images (lines 149–151):downscale returns img unchanged when longest <= maxDim, so a 2048×2048-or-smaller upload pays no extra RGBA allocation and no resample — only the JPEG round-trip. Correct and efficient.
Catmull-Rom on a ≤4 MP destination (line 155): O(dst_pixels × kernel²) once per upload. Catmull-Rom is the pricier kernel choice but the author justifies it (text sharpness on packet photos) and it's a cold path. Fine.
maxDecodePixels = 100 MP → ~400 MB decoded bitmap ceiling (line 61): Large transient allocation, but it's a deliberate, documented guard against dimension bombs and is bounded. This is a memory-safety/DoS consideration, not a perf regression — and it belongs to another lens anyway.
I could not inspect the gen2brain/heic or tetratelabs/wazero sources (module cache not present in the checkout), so I could not verify whether image.DecodeConfig on HEIC re-instantiates the wazero runtime per call (a potential per-request setup cost). If that turned out to be expensive it would matter on the request path in #81, but I have no evidence either way from what's checked in, so I'm not reporting it as a finding.
🧯 Error handling & edge cases — Blocking issues found
I have enough to verify. The arithmetic of the overflow claim is correct: image.Config{Width, Height int} carries the PNG IHDR's uint32 dimensions directly into int64 on a 64-bit platform, and int64(4294967295) * int64(4294967295) wraps to a negative value (~−8.59×10⁹), which is <= maxDecodePixels, so the guard at line 120 is bypassed. The repo's own comments (lines 58–61, 114–115) confirm the guard's stated purpose is exactly to refuse such claimed dimensions, and I confirmed there's no recover in Normalize. The minor format inconsistency is also confirmed by reading the error returns at lines 108/111/118 vs 121/126/133.
VERDICT: Blocking issues found
internal/imagenorm/imagenorm.go:120 — Integer-overflow bypass of the decompression-bomb guard. int64(cfg.Width)*int64(cfg.Height) wraps negative for adversarial headers (e.g. a PNG IHDR claiming ~2³² on each edge: (2³²−1)² = 2⁶⁴ − 2³³ + 1, which overflows int64 to ≈ −8.59×10⁹), so the > maxDecodePixels check is false and image.Decode proceeds with those claimed dimensions. The guard's whole purpose (documented at lines 58–61, 114–115) is to stop exactly this; it is bypassable by the input shape it claims to defend against, and there is no recover in Normalize, so the result is an OOM / makeslice: len out of range panic rather than the documented ErrTooLarge. I verified the arithmetic directly against the source; I could not read the stdlib/x/image decoder sources in this sandbox to confirm whether a downstream decoder independently caps dimensions, so exploitability of the allocation depends on that — but this code's own guard is broken. Fix: bound each dimension before multiplying, e.g. if cfg.Width > 0 && cfg.Height > 0 && (int64(cfg.Width) > maxDecodePixels || int64(cfg.Height) > maxDecodePixels || int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels), and handle cfg.Width <= 0 || cfg.Height <= 0 explicitly rather than letting a degenerate header fall through to a confusing decode.
internal/imagenorm/imagenorm.go:108 (minor) — format is returned as "" on read (108), byte-cap (111), and DecodeConfig (118) error paths, but as the decoded format on the pixel-overflow (121), image.Decode (126), and jpeg.Encode (133) error paths. Inconsistent error-return shape; harmless if callers ignore format on err != nil, but a footgun. Suggest always returning "" on error.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
internal/imagenorm/imagenorm.go:116 and :124 — Missing panic recovery from third-party image decoders. image.DecodeConfig and image.Decode invoke registered decoders directly; Go's image package does not recover from panics inside them. The HEIC path in particular runs libheif inside wazero, where malformed input can trigger WASM traps that surface as panics. A hostile upload therefore kills the goroutine (DoS / 500 storm) instead of returning the intended ErrUnsupported. Fix: Wrap both calls in func() { defer recover(); … }() and map recovered panics to ErrUnsupported.
internal/imagenorm/imagenorm.go:43 — Supply-chain opacity in the HEIC path (informational).
The PR presents the HEIC decoder as "pure Go, no cgo," but github.com/gen2brain/heic executes a bundled WASM blob of libheif. The origin and build provenance of that blob are opaque to downstream consumers, and the project's security posture is not established. This is not a bug in the PR's code, but the feature's critical input path now depends on unauditable compiled C code inside a WASM sandbox. Ensure wazero resource limits are explicitly configured by the library (or document that they are) before this is exposed to user uploads.
🎯 Correctness — No material issues found
No material issues found.
The implementation is correct within its stated scope. Verified by reading the source:
internal/imagenorm/imagenorm.go:103-136 — Normalize correctly caps the read at MaxBytes+1, checks decoded pixel count via image.DecodeConfig before full bitmap allocation, downscales preserving aspect ratio with Catmull-Rom, and re-encodes as JPEG.
internal/imagenorm/imagenorm.go:142-157 — downscale math is sound: scale = float64(maxDim) / float64(longest), new dimensions clamped with max(..., 1), and the longest <= maxDim || longest == 0 early-return prevents unnecessary resampling.
internal/imagenorm/imagenorm_test.go — Tests cover all four documented formats, oversize rejection, aspect-preserving downscale, and garbage/truncated inputs.
Confirmed via grep that no other package in the repo imports internal/imagenorm, so there are no cross-file correctness issues to check.
No logic bugs, incorrect constants, unit mismatches, or semantic errors introduced by this change.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:90-91 — ErrTooLarge and ErrUnsupported are defined with fmt.Errorf instead of errors.New. The rest of the codebase (internal/domain/domain.go:14, internal/service/password.go:29, internal/agent/runtime.go:52, etc.) consistently uses errors.New for static sentinel errors. This is inconsistent and carries unnecessary formatting overhead. Fix: import "errors" and use errors.New("imagenorm: ...").
internal/imagenorm/imagenorm_test.go:127-130 — The comment above TestNormalizeRejectsGarbage describes a TestNormalizeRejectsBombDimensions test that the function body does not implement (the body tests garbage/truncated input). There is no test anywhere for the maxDecodePixels path. Fix: rewrite the comment to describe what the function actually tests, and add the missing dimension-bomb test.
internal/imagenorm/testdata/ — The binary fixtures (sample.heic, sample.webp) have no in-repo provenance. A future maintainer cannot recreate or audit them. Fix: add internal/imagenorm/testdata/README.md documenting each fixture's source and how to regenerate it.
⚡ Performance — No material issues found
No material issues found
From the Performance lens, the package is well-bounded: input is capped at 25 MiB, decoded pixels are capped at 100 MP before any bitmap is allocated, and downscaling targets a modest 2048 px ceiling. Memory churn is O(max input + max decode + output buffer) with no unbounded growth, no hot loops, and no repeated work per row or pixel. The HEIC path goes through WASM (inherent to the CGO constraint), but that is a library-level trade-off, not a code-level regression introduced here. No N+1 patterns, no unnecessary copies inside loops, and no blocking calls on a path that should be async. The design keeps the fast JPEG re-encode path for in-bounds JPEG inputs by only downscaling when needed.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
internal/imagenorm/imagenorm_test.go:111 — image.DecodeConfig error discarded.
In TestNormalizeDownscales the small-image branch does cfg, _, _ = image.DecodeConfig(bytes.NewReader(out)). If the normalized JPEG is somehow corrupt, the test reports a misleading dimension mismatch instead of the real decode failure. Check the error with t.Fatalf like the big-image branch does.
internal/imagenorm/imagenorm_test.go:127-141 — The dimension-bomb guard is untested.
The comment claims this tests “a small file claiming a huge canvas”, but TestNormalizeRejectsGarbage only exercises garbage bytes and a truncated PNG. No test verifies that a valid header with Width*Height > maxDecodePixels returns ErrTooLarge before image.Decode allocates. Because this is the main memory-safety guard, it deserves a fixture or a hand-crafted header (e.g. a PNG IHDR with 50 000×50 000 pixels and a tiny IDAT).
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 12 findings (6 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | TestNormalizeRejectsGarbage has a stale comment header describing a non-existent TestNormalizeRejectsBombDimensions; the decompression-bomb (maxDecodePixels) path is untested | `internal/imagenorm/imagenorm_test.go:127` | 5/5 | correctness, error-handling, maintainability |
| 🟡 | Sentinel errors use fmt.Errorf instead of errors.New, diverging from repo convention | `internal/imagenorm/imagenorm.go:90` | 4/5 | maintainability |
| 🔴 | Integer overflow in decompression-bomb guard: int64(w)*int64(h) wraps negative for crafted ~2^32-dimension headers, bypassing the maxDecodePixels check and allowing an OOM/panic in image.Decode instead of ErrTooLarge | `internal/imagenorm/imagenorm.go:120` | 3/5 | error-handling, security |
| 🟠 | Doc contract overpromises: read/encode I/O errors don't actually map to the documented ErrUnsupported/ErrTooLarge sentinels | `internal/imagenorm/imagenorm.go:101` | 3/5 | error-handling, security |
| 🟠 | Missing panic recovery around image decoders exposes goroutine DoS from malicious uploads | `internal/imagenorm/imagenorm.go:116` | 2/5 | security |
| 🟠 | EXIF orientation is discarded on decode/re-encode, causing portrait phone photos to be uploaded sideways | `internal/imagenorm/imagenorm.go:124` | 2/5 | correctness, security |
<details><summary>6 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | maxDecodePixels (100 MP) permits ~400 MB peak allocation per upload — memory-exhaustion DoS surface, and the huge intermediate bitmap is discarded since MaxDim=2048 downscales everything larger | `internal/imagenorm/imagenorm.go:61` | opencode/glm-5.2:cloud | maintainability, security |
| 🟡 | downscale() always targets *image.RGBA, forcing jpeg.Encode's slow generic per-pixel path instead of the YCbCr fast path (package has no caller yet in this diff) | `internal/imagenorm/imagenorm.go:154` | claude-code/sonnet | performance |
| 🟡 | Test silently depends on DefaultMaxDim constant | `internal/imagenorm/imagenorm_test.go:89` | kimi-k2.6:cloud | maintainability |
| 🟡 | image.DecodeConfig error ignored in small-image downscale test | `internal/imagenorm/imagenorm_test.go:111` | opencode/kimi-k2.6:cloud | error-handling |
| 🟡 | Binary test fixtures lack provenance documentation in repository | `internal/imagenorm/testdata:1` | opencode/kimi-k2.6:cloud | maintainability |
| ⚪ | Package doc references non-existent TestNormalize; actual test is TestNormalizeAllFormats | `internal/imagenorm/imagenorm.go:25` | opencode/glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
## Review — Security lens
**VERDICT: Minor issues**
This PR adds a self-contained `imagenorm` package that isn't imported anywhere yet (verified: no non-test caller in the tree), so there is no live attack surface today — the actual exposure will appear when a future handler wires an upload endpoint to `Normalize`. Reviewing the function as written for what it will face once that happens:
- **Good**: input is bounded before read (`io.LimitReader(r, maxBytes+1)`, `imagenorm.go:105`), and the decoded-pixel-count guard runs from `image.DecodeConfig` before the full bitmap is allocated (`imagenorm.go:116-122`), which is the right order to defend against a decompression bomb.
- **`internal/imagenorm/imagenorm.go:103`** — `Normalize(r io.Reader, opts Options)` takes no `context.Context`, so once wired to an HTTP handler, decoding an untrusted, WASM-interpreted HEIC (or a large PNG/webp under the 100 MP cap) can't be cancelled. Verified in `cmd/pansy/main.go:60-67` that the server sets `WriteTimeout: 30s`, but `http.Server`'s `WriteTimeout` only closes the client connection — it does not cancel the in-flight handler goroutine, which will keep burning CPU/decoding to completion regardless. This isn't exploitable today since nothing calls `Normalize` yet, but it's a gap worth closing (accept/honor a `context.Context`, or have the future handler impose a deadline) before this is wired to a real upload endpoint.
Nothing else in this diff touches auth, injection surfaces, secrets, or deserialization of anything other than image pixel data through Go's own `image` package machinery, which is the intended defense here.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding checks out against the actual source. `internal/imagenorm/imagenorm.go` decodes with the stdlib `image.Decode` (which does not read EXIF `Orientation`) and re-encodes with `image/jpeg`'s `jpeg.Encode` (which writes no EXIF at all) — confirmed lines 124–135. A repo-wide grep for `exif`/`orientation` (case-insensitive, all `.go` files) returns nothing, and `imagenorm` is only referenced by its own package and test file, so there's no caller-side rotation correction either. Verdict and finding both survive.
## Verdict: Minor issues
**EXIF orientation is dropped during normalization** — `internal/imagenorm/imagenorm.go:124-134`
`Normalize` decodes with `image.Decode` and re-encodes with `image/jpeg`'s `jpeg.Encode`. Go's standard `image/jpeg` decoder does not read or apply the EXIF `Orientation` tag — it decodes only the raw pixel matrix. `jpeg.Encode` writes no EXIF data into its output. There is no EXIF-reading import anywhere in this package or the rest of the repo (confirmed via repo-wide grep for `exif`/`orientation`), and `imagenorm` is called from no other file yet, so there is no caller-side correction either.
Concrete failure: many cameras — most Android phones, and many third-party iOS camera/sharing apps that re-encode HEIC to JPEG before an upload flow — store a photo taken in portrait as landscape pixel data plus an `Orientation` tag (e.g. `6`/`8`) rather than physically rotating the pixels. After `Normalize`, that tag is silently discarded and the pixel data is re-encoded unrotated, so a packet photo taken in portrait comes out sideways (or upside-down/mirrored for orientations 3/2/4/5/7). Since the PR's stated goal is "everything downstream (majordomo's vision path) only ever sees JPEG" — i.e. this normalized output *is* what the model reads — a mis-rotated packet photo directly degrades the seed-packet-capture feature's primary use case, not just a cosmetic display issue.
HEIC is likely less affected in practice (HEIF containers commonly apply `irot`/`imir` transform properties at decode time rather than relying on a separate orientation tag, so `gen2brain/heic`'s output pixels are plausibly already correctly oriented — unverified here, no network/vendor access to confirm), but this doesn't help the JPEG/PNG-from-Android or re-encoded-JPEG case, which is very plausibly the majority of non-iPhone camera input.
Suggested fix: read the EXIF `Orientation` tag from the raw JPEG bytes before/alongside decode and apply the corresponding rotate/flip to `img` before the final JPEG encode.
Everything else checked out:
- Byte cap (`imagenorm.go:104-112`): `LimitReader(MaxBytes+1)` + `len(raw) > maxBytes` correctly distinguishes "exactly at cap" from "over."
- Decompression-bomb guard (`imagenorm.go:116-122`): pixel-count check runs on `DecodeConfig` before the full `image.Decode` allocates a bitmap.
- Downscale math (`imagenorm.go:142-157`): scale/nw/nh derivation is consistent, small images are left untouched, and the `longest == 0` guard avoids a divide-by-zero.
- Error mapping (`ErrUnsupported` vs `ErrTooLarge`) is applied consistently.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verified against the real source. Confirms no other package uses `fmt.Errorf` without a verb, and confirms `TestNormalizeRejectsGarbage`'s doc comment is indeed a leftover description of a bomb-dimensions test that doesn't exist, with no other test in the file or elsewhere covering `maxDecodePixels`.
**VERDICT: Minor issues**
- `internal/imagenorm/imagenorm_test.go:127-130` — The doc comment above `TestNormalizeRejectsGarbage` is stale: it describes a `TestNormalizeRejectsBombDimensions` scenario ("a small file claiming a huge canvas is refused before the bitmap is allocated... the guard is on the DECODED pixel count") but the function's body only exercises garbage bytes and a truncated PNG — it never constructs an oversized-`DecodeConfig` fixture. No test anywhere in the file (or package) actually drives the `maxDecodePixels` branch at `imagenorm.go:120-122`, so the comment misleads a future reader into believing that guard is covered when it isn't.
- `internal/imagenorm/imagenorm.go:90-91` — `ErrTooLarge`/`ErrUnsupported` use `fmt.Errorf` with no formatting verb, where `errors.New` is called for elsewhere. Every sentinel error in `internal/domain/domain.go` (14 instances) uses `errors.New`, and this file itself uses `fmt.Errorf` correctly for wrapping at lines 108 and 133 with `%w`. This is the only place in `internal/` that declares a plain sentinel with `fmt.Errorf`.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `internal/imagenorm/imagenorm.go:154` (`downscale`) — When an image needs resizing, `downscale` draws into a freshly allocated `*image.RGBA` (`dst := image.NewRGBA(...)`, `draw.CatmullRom.Scale(dst, ...)`, confirmed at lines 154–155), and that `*image.RGBA` is what `Normalize` hands to `jpeg.Encode` at line 132. Go's stdlib JPEG encoder only has a bulk-plane fast path for `*image.YCbCr` and `*image.Gray`; any other concrete type — including `*image.RGBA` — falls back to a generic per-pixel path that calls `.At(x, y)` and does an RGBA→YCbCr conversion per pixel. This is well-established, widely documented Go stdlib behavior; I was not able to re-read `image/jpeg/writer.go` directly in this sandbox (out-of-repo file reads are blocked here too), so, like the original author, I'm not treating it as tool-verified against the stdlib source itself — confidence stays medium on that specific sub-claim.
- One correction to the draft's framing: this PR only *adds* `internal/imagenorm` and its test — a repo-wide grep turns up zero callers of `imagenorm.Normalize` outside the package itself. So the "upload hot path" / "essentially 100% of real-world uploads" language overstates the current, verifiable impact of this diff — there is no wired-up caller yet for this to cost CPU on. The underlying code observation (resize path always targets RGBA, forcing the slow encode path once this *is* wired to an upload handler) still holds and is worth flagging before the package gets its first caller.
- Suggested fix (before wiring this into a request path): if downscaled-photo encode latency matters once there's a real caller, encode via a `*image.YCbCr` target instead of `*image.RGBA` (e.g. `image.NewYCbCr` + a scaler that can write into it, or convert post-scale) to hit the encoder's fast path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Confirmed both findings are accurate. There is no other test elsewhere in the repo exercising the `maxDecodePixels`/bomb-dimensions branch, and the package isn't yet wired into any caller, so the doc-contract mismatch is real (no other file's error handling masks it). Both findings survive verification unchanged.
## Corrected review
**VERDICT: Minor issues**
Reviewed `internal/imagenorm/imagenorm.go` and its test file. The core guards (byte cap, pre-decode pixel-count check, graceful `ErrUnsupported` on garbage/truncated input, no re-sampling on already-small images) are implemented correctly — no panics on empty/degenerate dimensions, no unguarded type assertions, negative/zero `Options` fields fall back safely to defaults (`imagenorm.go:73-85`).
- `internal/imagenorm/imagenorm.go:99-102` — the doc comment states "A read/format error maps to ErrUnsupported," but that's only true for the format-decode branch (`imagenorm.go:117-118`, `125-126`). A genuine I/O failure from `io.ReadAll` (`imagenorm.go:106-109`) returns a distinct wrapped error (`fmt.Errorf("imagenorm: read: %w", err)`), not `ErrUnsupported`, and the `jpeg.Encode` failure path (`imagenorm.go:132-134`) likewise returns an unwrapped-from-sentinel error. A caller relying on `errors.Is(err, ErrUnsupported)` per the documented contract will silently miscategorize these two paths. Fix: either wrap read/encode failures so they satisfy a documented sentinel, or correct the doc to describe the actual error shape.
- `internal/imagenorm/imagenorm_test.go:127-130` — the comment above `TestNormalizeRejectsGarbage` is titled `TestNormalizeRejectsBombDimensions` and describes "a small file claiming a huge canvas... refused before the bitmap is allocated," but the function it labels only tests garbage bytes and a truncated PNG (`imagenorm_test.go:131-140`) — it never constructs an input that trips the `maxDecodePixels`/`ErrTooLarge` branch at `imagenorm.go:120-122`. Confirmed via repo-wide grep: no test anywhere exercises that branch. The decompression-bomb guard — the safety property called out as the point of the pixel-count pre-check — has zero test coverage. Fix: add the actual bomb-dimensions test the comment describes (e.g. a spoofed IHDR/DecodeConfig claiming a huge width/height) so a regression in that branch (e.g. `>` flipped to `>=`) would be caught.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
The code confirms the first finding. `maxDecodePixels = 100_000_000` (line 61), the check at line 120 uses strict `>`, so exactly 100,000,000 px (e.g. 10000×10000) passes and decodes to a ~400 MB RGBA bitmap, while a flat-color PNG of that size is only kilobytes on disk — well under the 25 MiB input cap. `downscale` then allocates an additional RGBA plus the JPEG output buffer on top. No concurrency bound exists in the package. This is a real memory-amplification DoS vector at an untrusted upload boundary.
The second finding is explicitly low-confidence defense-in-depth (and not in the JSON block); the draft itself notes it couldn't confirm a bypass. Dropping it.
**VERDICT: Minor issues**
- `internal/imagenorm/imagenorm.go:120` — The `maxDecodePixels` guard (100 MP) is too permissive and undermines the decompression-bomb protection it claims to provide. PNG is a pixel-raster format whose byte size is decoupled from its decoded bitmap size: a flat-color 10000×10000 image is ~kilobytes on disk but decodes to a 10000×10000×4 = 400 MB RGBA bitmap. The input cap (25 MiB) and the `DecodeConfig` check don't prevent this, because the byte count is tiny *and* the dimension check uses `>` (strictly greater), so exactly 100,000,000 px passes. So a hostile-but-valid ~KB PNG forces a ~400 MB allocation per request — a ~40,000× memory amplification — and nothing in this package bounds concurrent decodes. On top of that, `downscale` allocates a second RGBA (up to 2048²×4 ≈ 16 MB) plus the JPEG output buffer. For an upload boundary ingesting untrusted photos, that's a DoS vector. The fix is to lower `maxDecodePixels` to something tied to the actual use case (a real phone photo is ~12 MP; 24 MP is generous) rather than "any photo ever." Verified the math against `downscale`/`jpeg.Encode` (which allocate the RGBA + output buffer on top of the decoded image) by reading `imagenorm.go:103-157`.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified the two draft findings against the actual code and Go's image/draw semantics. Here is my assessment.
**Finding 1 (`imagenorm.go:155`, draw.Over vs draw.Src):** I read the code (lines 142-157): `downscale` allocates `dst := image.NewRGBA(...)` — a freshly-zeroed, fully-transparent destination (all `0,0,0,0`) — then calls `draw.CatmullRom.Scale(dst, dst.Bounds(), img, b, draw.Over, nil)`.
The draft's central actionable claim is that switching `draw.Over` → `draw.Src` would fix the alleged darkening. I cannot confirm that, and in fact it is contradicted by Porter-Duff semantics: with a destination whose alpha is zero, `Over` reduces to `result = src` (premultiplied), and `Src` is defined as `result = src` (premultiplied). The two operators produce byte-identical output on a zero/transparent destination — the draft even states this ("over a zero dst gives dstRGB = srcRGB*srcA") but then claims `draw.Src` would instead "copy the original RGB verbatim." That is incorrect: `draw.Src` writes the same premultiplied value that `draw.Over` writes here. So the recommended fix is a no-op and the finding's technical premise does not hold. Whether any darkening occurs downstream depends entirely on how `image/jpeg` reads a premultiplied `*image.RGBA` (stdlib code not present in this repo, so I cannot positively confirm or refute it), but the proposed remedy would not address it regardless. Per the gate rule (drop what I cannot confirm, and drop findings the code contradicts), this is dropped.
**Finding 2 (`imagenorm.go:99-104`, doc prose "reads at most opts.MaxBytes"):** Verified: the comment at line 99 says "It reads at most opts.MaxBytes," and the code at lines 104-105 reads via `LimitReader(r, maxBytes()+1)` then rejects if `len(raw) > maxBytes()`. The `+1` is an intentional boundary-detection detail; the function never *accepts* more than `opts.MaxBytes` for processing, so the contract-level statement is defensible. This is a trivial prose nuance at most, and not a material issue.
No surviving material findings.
VERDICT: No material issues found
- `internal/imagenorm/imagenorm.go:155` — examined the `downscale` compositing. The destination is a freshly-allocated, fully-transparent `*image.RGBA` (`image.NewRGBA`). Under Porter-Duff rules, `draw.Over` over a zero-alpha destination and `draw.Src` both reduce to `result = src` (premultiplied) and produce identical `Pix` bytes. Therefore the draft's claim that switching to `draw.Src` would "copy the original RGB verbatim" and fix darkening is not supported by the operator semantics; the suggested fix would be a no-op. Any downstream darkening (if it exists) would depend on `image/jpeg`'s reading of a premultiplied `*image.RGBA`, which cannot be verified from this repository, and is not addressed by `draw.Src` regardless. Dropped.
- `internal/imagenorm/imagenorm.go:99` — the comment says "It reads at most opts.MaxBytes"; the implementation reads `maxBytes()+1` to distinguish at-cap from over-cap, then rejects anything over `maxBytes()`. The accepted/processed input is in fact capped at `opts.MaxBytes`, so the contract-level statement holds; the `+1` is an intentional implementation detail. Trivial at most, not material. Dropped.
I also re-derived the downscale math for the `4000×1000` test case against `downscale` (lines 142-157): `longest=4000`, `scale=2048/4000=0.512`, `nw=int(2048.0)=2048`, `nh=int(512.0)=512`, matching the asserted `2048×512` with aspect preserved. The byte-cap logic (`LimitReader(maxBytes+1)` then `len > maxBytes`) correctly distinguishes exactly-at-cap from over-cap, and the bomb guard (`cfg.Width*cfg.Height > maxDecodePixels` at line 120) is correctly ordered before the full `image.Decode` at line 124.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings confirmed against the actual source.
**VERDICT: Minor issues**
- **`imagenorm_test.go:127` — mismatched test name and doc comment.** The comment at lines 127–129 says `TestNormalizeRejectsBombDimensions: a small file claiming a huge canvas...` but the function declared at line 130 is `TestNormalizeRejectsGarbage`, which only feeds garbage/truncated bytes (`"not an image at all"` and a halved PNG) and asserts `ErrUnsupported`. The `maxDecodePixels` decompression-bomb path (`imagenorm.go:120`, which returns `ErrTooLarge` for a huge claimed canvas) is never exercised by any test in the file (confirmed by grep — no other test references `maxDecodePixels` or constructs an oversize-canvas fixture). The stale comment claims a guard test that does not exist. Fix: either add a `TestNormalizeRejectsBombDimensions` that feeds a PNG with a huge declared canvas under the byte cap, or reword the comment to describe what `TestNormalizeRejectsGarbage` actually does.
- **`imagenorm.go:90-91` — exported sentinel errors built via `fmt.Errorf` rather than `errors.New`.** `fmt.Errorf("imagenorm: image too large")` and `fmt.Errorf("imagenorm: unsupported or corrupt image")` contain no verbs, so `errors.New(...)` is the idiomatic choice. Cosmetic, but inconsistent with the otherwise careful style of the surrounding code.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
This is new code on the upload boundary (a cold path processing one image at a time), and the resource costs are bounded and deliberate. Reviewed through the performance lens:
- **Double header parse** (`internal/imagenorm/imagenorm.go:116` and `:124`): `DecodeConfig` and `image.Decode` each get a fresh `bytes.Reader` over the same in-memory `raw`, so the format header is parsed twice. This is negligible relative to the full bitmap decode/resample/encode that follows, and the two-pass structure is what enables the pre-allocation decompression-bomb guard. Not worth changing.
- **Full read into memory** (`:105`–`:106`): up to `MaxBytes` (25 MiB) is buffered entirely before decoding, rather than streaming. This is by design (two passes over the bytes for the size guard), and the cap keeps the transient footprint bounded. Acceptable.
- **`draw.CatmullRom.Scale`** (`:155`): Catmull-Rom is the costliest kernel scaler in `x/image/draw`, but the PR justifies it for text sharpness, it only runs when an image actually exceeds `MaxDim`, and it's off the hot path. A deliberate quality/speed trade-off, not a regression.
- **HEIC via wazero**: running libheif as WASM is markedly slower than a native codec, but (a) it's the only CGO-free option and (b) it runs per-upload, not in a loop. Not a material concern for this path.
- **Decoded-bitmap bound** (`maxDecodePixels = 100MP` ≈ 400 MB at 4 B/px): the cap is generous and a 100 MP image that compresses under 25 MiB could force a large transient allocation. This is acknowledged in the code comments and is a design choice on a cold path, not an inefficiency to flag.
No unbounded growth, N+1 patterns, hot-loop regressions, or avoidable quadratic behavior were found.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've verified the error-handling paths against the checked-out code. The implementation is solid in my lane.
**Verification notes:**
- Byte-cap boundary (`imagenorm.go:104-112`): `LimitReader(r, max+1)` then `len(raw) > maxBytes()` correctly distinguishes "exactly at cap" (accepted) from "over" (ErrTooLarge). Read errors are wrapped, not swallowed (`:108`).
- Decompression-bomb guard (`:116-122`): `DecodeConfig` dimensions checked in `int64` arithmetic (no overflow for MaxInt32-scale dims) *before* `image.Decode` allocates the bitmap. Correct ordering.
- Garbage/truncated path (`:116-127`): both `DecodeConfig` and `Decode` failures map to `ErrUnsupported`; `format` from the successful `DecodeConfig` is carried through on the partial-failure path. No panic surface introduced by the stdlib decoders.
- `downscale` (`:142-157`): zero-dimension guarded (`longest == 0` → return unchanged); `max(..., 1)` prevents a zero-height/width `NewRGBA` when the short edge rounds down to 0 (e.g. 2049×1 → nh=1). Scaling math can't overflow int: `w*h ≤ maxDecodePixels` (100M) is enforced upstream, and `scale < 1` in the downscale branch, so `nw,nh ≤ w,h ≤ 100M` — well within int32.
- Negative `Options.MaxDim`/`MaxBytes` fall through to defaults via the `> 0` checks (`:73-85`), so a misconfigured caller can't drive a zero/negative cap into `LimitReader` or the pixel check.
One thing I could *not* locally verify (heic source isn't vendored in the repo): whether `github.com/gen2brain/heic` registers `image.DecodeConfig` such that the bomb guard at `:116` runs for HEIC before `image.Decode`. If it only registered `Decode`, every HEIC would fail at `:117` with `ErrUnsupported` — but the PR's `TestNormalizeAllFormats` heic case exercises exactly this path and is reported green, which is strong evidence the registration is correct. Not a finding against the reviewed code.
No material issues found in the error-handling & edge-case lens.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
**Minor issues**
- **`internal/imagenorm/imagenorm.go:116-127`** — `Normalize` does not recover from panics inside `image.DecodeConfig` or `image.Decode`. Third-party decoders (especially the WASM-backed HEIC path) are complex C-in-WASM parsers and historically have had crash-on-malformed-input bugs. A single hostile upload can crash the processing goroutine. Since the PR explicitly frames this as a hostile-upload boundary, add a `defer recover()` guard around the decode phase and map recovered panics to `ErrUnsupported` so a bad file is a clean error, not a goroutine (or process) kill.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I scrutinized the normalization logic, dimensions, thresholds, and format handling:
- **Byte cap / LimitReader logic**: `io.LimitReader(r, int64(opts.maxBytes())+1)` followed by `len(raw) > opts.maxBytes()` correctly distinguishes “exactly at the cap” from “over the cap” and prevents unbounded reads. Edge cases (empty reader, exact-size input) behave correctly.
- **Decompression-bomb guard**: `int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels` uses `int64` to avoid overflow on the product, and the check happens before `image.Decode` allocates the bitmap. The threshold (100 MP ≈ 400 MB RGBA) is consistent with the stated intent.
- **Downscale math**: `scale := float64(maxDim) / float64(longest)` with `max(int(float64(w)*scale), 1)` preserves aspect ratio and guarantees at least a 1×1 output. The default `MaxDim=2048` keeps the post-scale image well under the pixel cap, and the 4000×1000 test case evaluates to exactly 2048×512 with IEEE-754 double precision, so the aspect test is deterministic.
- **Format coverage / registration**: The four blank imports (`image/jpeg`, `image/png`, `github.com/gen2brain/heic`, `golang.org/x/image/webp`) register with `image.Decode` by side effect. The test `TestNormalizeAllFormats` round-trips every claimed format and would catch a dropped import.
- **Error handling**: Truncated or garbage input fails at `DecodeConfig`/`Decode` and is mapped cleanly to `ErrUnsupported`. Oversize bytes and oversize dimensions both map to `ErrTooLarge`. No panic paths were found.
- **JPEG re-encode**: `jpeg.Encode` with quality 85 on an `*image.RGBA` destination is well-formed; the output is validated in tests.
Nothing in the diff introduces a logic bug, unit mismatch, boundary error, or semantic domain issue.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `internal/imagenorm/imagenorm_test.go:127` — Comment names `TestNormalizeRejectsBombDimensions` but the actual function is `TestNormalizeRejectsGarbage`, violating the standard Go convention that a comment should introduce the function it precedes. Worse, the described decompression-bomb scenario (a small file claiming huge decoded dimensions) is **never exercised** in the test body; only garbage bytes and truncated PNGs are tested. A future maintainer will reasonably believe the bomb guard has direct regression coverage when it does not.
**Fix:** Rename the test to match the comment (or vice-versa) and add an actual bomb-dimension test case—e.g. a minimal crafted header with a huge `Width`/`Height`—so the safety feature is locked.
- `internal/imagenorm/imagenorm_test.go:89` — `TestNormalizeDownscales` silently couples itself to `DefaultMaxDim`. It passes `Options{}` and then hardcodes expected dimensions (`2048`, `512`). If `DefaultMaxDim` is ever changed, this test will fail with opaque assertions that don't reveal the root cause.
**Fix:** Make the dependency explicit by passing `Options{MaxDim: 2048}` (or compute expectations from `DefaultMaxDim` directly).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The code is well-bounded and avoids the common performance traps for an image-pipeline package:
- **Capped reads**: `io.LimitReader` + `io.ReadAll` with `DefaultMaxBytes = 25 MiB` prevents unbounded memory consumption from a malicious stream.
- **DecodeConfig pre-check**: `maxDecodePixels = 100M` is enforced before `image.Decode` allocates the bitmap, blocking decompression bombs.
- **Downscale before encode**: The largest possible decoded bitmap that actually reaches `jpeg.Encode` is bounded by `DefaultMaxDim = 2048`, so the destination RGBA is at most ~16 MiB.
- **No redundant work in hot paths**: `downscale` returns the original image unchanged when it already fits, avoiding an unnecessary `CatmullRom` resample.
- **One-shot usage**: This is an upload-boundary function, not a hot loop; the bounded allocations (~25 MiB input + decoded image + destination + output buffer) are acceptable for per-request execution.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- `internal/imagenorm/imagenorm.go:120` — Missing dimension positivity validation. `cfg.Width` or `cfg.Height` could be zero or negative from a malformed image or buggy third-party decoder. The product `int64(cfg.Width)*int64(cfg.Height)` then yields a negative or zero value that passes the `> maxDecodePixels` guard, allowing the decode to proceed. **Fix:** add `if cfg.Width <= 0 || cfg.Height <= 0 { return nil, "", ErrUnsupported }` before the pixel count check.
- `internal/imagenorm/imagenorm.go:111` vs `imagenorm.go:121` — Inconsistent `format` return on `ErrTooLarge`. The byte-cap path returns `""` for format, but the pixel-count path returns the detected `format`. Callers inspecting `format` when `err != nil` see different behavior depending on which limit triggered. **Fix:** return `nil, "", ErrTooLarge` in both cases for consistency.
- `internal/imagenorm/imagenorm.go:105` — Potential `int64` overflow in `io.LimitReader` limit. If a caller passes `Options{MaxBytes: math.MaxInt64}`, `int64(opts.maxBytes())+1` wraps to a negative limit, causing `io.ReadAll` to read nothing and the function to return `ErrUnsupported` instead of a meaningful error. **Fix:** avoid the `+1` by reading at most `maxBytes` and detecting truncation separately (e.g., compare `len(raw)` to the limit, or read one extra byte with overflow-safe math).
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
The draft's single finding is confirmed against the code:
- Line 61: `maxDecodePixels = 100_000_000` ✓
- Line 120: the guard checks `> maxDecodePixels` *before* full decode, so up to 100 MP is allowed ✓
- Line 124: `image.Decode` then allocates the full bitmap — 100 MP × 4 bytes/px (RGBA) = ~400 MB ✓
- Line 53/129: `DefaultMaxDim = 2048` and `downscale` discards anything larger, so the huge intermediate serves no functional purpose ✓
- The byte cap (25 MiB input) does not bound decoded size for low-entropy/compressible payloads ✓
No callers wire `imagenorm` yet (grep confirms only the package itself references the name), so this is latent attack surface rather than a live endpoint, but the constants and decode semantics are as the draft describes. The HEIC `DecodeConfig` note in the draft is explicitly not a finding, so nothing to verify there.
VERDICT: Minor issues
- **`internal/imagenorm/imagenorm.go:61` — `maxDecodePixels` (100 MP) permits ~400 MB peak allocation per upload.** The guard at line 120 rejects only *more* than 100 MP, so a decoded bitmap up to 100 MP is allowed through to `image.Decode` (line 124). At 4 bytes/px (RGBA) that is ~400 MB allocated before the downscale step even runs. A hostile, low-entropy JPEG/HEIC well under the 25 MiB *input* cap (line 57) can legitimately decode to that size — the byte cap does not bound decoded size for compressible payloads. Since `DefaultMaxDim = 2048` (line 53) and `downscale` (line 129) immediately throw away and re-sample anything larger on its longest edge, a 400 MB intermediate bitmap serves no functional purpose and is pure attack surface: one request can force ~400 MB allocation, and concurrent uploads multiply it. The guard exists but the ceiling is far too generous for the actual use. Suggested fix: tie the pixel cap to the output need (e.g. `maxDecodePixels = maxDim*maxDim * 4` ≈ 16 MP at default, still ample headroom for Catmull-Rom downscaling) so a hostile upload cannot force an allocation many times larger than anything that survives normalization. Verified by reading the code; the 25 MiB → 100 MP → 400 MB path follows directly from the constants and `image.Decode` semantics.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Confirmed. Comment at line 127 says `TestNormalizeRejectsBombDimensions` but function at line 130 is `TestNormalizeRejectsGarbage`. The body tests garbage + truncated PNG only, never the `maxDecodePixels` guard. Verified.
## VERDICT: Minor issues
Verified by reading `/tmp/gadfly/repo/internal/imagenorm/imagenorm.go` and `imagenorm_test.go` in full.
- **`internal/imagenorm/imagenorm_test.go:127-130` — misleading/stale doc comment; the decompression-bomb guard is untested.** The comment block says `TestNormalizeRejectsBombDimensions: a small file claiming a huge canvas is refused before the bitmap is allocated`, but the function actually defined immediately below is `TestNormalizeRejectsGarbage`, which only feeds in `"not an image at all"` and a truncated PNG. Neither case exercises the `int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels` check at `imagenorm.go:120` — there is no test anywhere that constructs a small file with huge claimed dimensions to confirm the guard rejects it. The `maxDecodePixels` guard is the central safety property the PR description advertises ("decompression-bomb guard"), and it has zero coverage. A maintainer reading the comment would believe the guard is tested when it is not. Suggested fix: add a real bomb-dimension case (e.g. a crafted PNG/HEIC header claiming huge dimensions within the byte cap) asserting `ErrTooLarge`, and fix the comment to match the function name.
The downscale math, byte-cap logic, aspect preservation, format-string handling, and the four-format round-trip assertions all check out on re-derivation (4000×1000 → 2048×512; small images untouched; oversize input → `ErrTooLarge`; `format` from `DecodeConfig` is preserved through the `Decode` call).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I'll verify each finding against the actual checked-out files.
Now let me verify the convention claims about sentinel errors elsewhere in the repo.
Now let me verify the `maxBytes()` call count claim in finding #4. The draft says "called three times" — let me count precisely.
Looking at the code:
- Line 105: `int64(opts.maxBytes())+1` — call 1
- Line 110: `len(raw) > opts.maxBytes()` — call 2
Only two calls in `Normalize`, not three. The draft's "three times" claim is factually wrong.
All other findings verified:
1. **`fmt.Errorf` for sentinels (lines 90-91)** — CONFIRMED. Every other sentinel declaration in the repo uses `errors.New` (domain.go, password.go, agentmodel.go, runtime.go, journal.go). The other two `fmt.Errorf("...")` matches (revisions.go:713, instance_settings.go:41) are inline returns, not sentinel declarations.
2. **Test comment/name mismatch (lines 127-130)** — CONFIRMED. Comment says `TestNormalizeRejectsBombDimensions:` but function is `TestNormalizeRejectsGarbage`; body tests garbage + truncated input, never bomb dimensions.
3. **Stale doc reference (line 25)** — CONFIRMED. Package doc says `TestNormalize exercises all four formats`; no such function exists, actual test is `TestNormalizeAllFormats`.
5. **`maxDecodePixels` not in `Options` (line 61)** — CONFIRMED. It's an unexported const while `MaxDim`/`MaxBytes` are tunable via `Options`.
Finding #4 drops: the "three times" claim is false (it's two calls), and the underlying suggestion is trivial enough that the incorrect premise disqualifies it.
**VERDICT: Minor issues**
- **`internal/imagenorm/imagenorm.go:90-91` — `fmt.Errorf` for sentinel errors instead of `errors.New`.** Every other sentinel error declaration in the repo uses `errors.New` (`internal/domain/domain.go:14-64`, `internal/service/password.go:29`, `internal/agentmodel/agentmodel.go:39`, `internal/agent/runtime.go:52`). `fmt.Errorf("imagenorm: image too large")` has no format verbs and pays a fmt-formatting cost for a constant string. `errors.Is` still works, but the package diverges from the established convention for no benefit. Use `errors.New`.
- **`internal/imagenorm/imagenorm_test.go:127-130` — misleading comment / name mismatch.** The doc comment for `TestNormalizeRejectsGarbage` leads with `// TestNormalizeRejectsBombDimensions:` — a function name that does not exist and describes a test the function does *not* perform (it never feeds a small-file/huge-dimension input; that case isn't covered anywhere). The body tests garbage + truncated input. Either rename to match the comment and add the bomb-dimensions case (a header claiming huge width/height is exactly what the `maxDecodePixels` guard is for, and currently has no coverage), or fix the comment to describe what the test actually does.
- **`internal/imagenorm/imagenorm.go:25` — stale doc reference.** The package doc says `TestNormalize exercises all four formats` — there is no `TestNormalize`; the actual test is `TestNormalizeAllFormats`. Minor, but the CLAUDE.md rule "stale comments are worse than none" applies. Update the reference.
- **`internal/imagenorm/imagenorm.go:61` — `maxDecodePixels` is an unexported constant while `MaxDim`/`MaxBytes` are exposed via `Options`.** The decompression-bomb pixel cap is a hard unexported constant, inconsistent with the "Options tunes Normalize" framing — a caller can't tighten it. That's a defensible safety choice, but the asymmetry is undocumented and looks accidental. A one-line comment explaining *why* it's intentionally not in `Options` would help the next reader.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I'll review this through the performance lens. Let me examine the actual code.
The file matches the diff. Now let me consider the performance profile of this code carefully.
The key concern from a performance lens:
**1. Full file buffered into memory twice over.** `Normalize` reads the entire input (up to 25 MiB) into `raw` via `io.ReadAll`, then passes `bytes.NewReader(raw)` to both `image.DecodeConfig` and `image.Decode`. The `DecodeConfig`→`Decode` pattern requires two passes, which is why the author buffers fully — that's a reasonable, even necessary, trade-off given the stdlib's `image` package re-reads from a `Reader` (which isn't seekable in the general case). But combined with the decoded bitmap (up to ~400 MB at the `maxDecodePixels` cap) and the output buffer, peak memory is `raw + decoded_bitmap + dst_bitmap (if downscaled) + output`. This is inherent to the design and bounded by the caps; not a regression.
**2. Downscaling allocation when the image is already within bounds but re-encoded.** `downscale` returns `img` unchanged when `longest <= maxDim`, so no extra RGBA allocation happens for in-bounds images. Good.
**3. Catmull-Rom on a 2048×2048 RGBA destination.** This is the most expensive part per-call but runs once per upload (a cold path — a user submitting a seed packet photo), not in a hot loop. Catmull-Rom is O(dst_pixels × kernel²) which is fine for a 4 MP output. Not a concern.
Let me check whether `image.Decode` for HEIC via wazero has any notable per-call cost (e.g. re-instantiating a WASM runtime per decode) that would matter on a request path.
The module cache isn't vendored/checked out, so I can't inspect the heic library source directly. Let me check if there's a vendor dir or go module cache accessible.
No vendor dir. The heic/wazero sources aren't available for me to inspect directly. Let me reason about what I can verify from the code itself.
The package isn't imported anywhere yet (per the PR description, #81 will import it), so this code is not on any current hot path. Let me confirm no current caller exists.
Confirmed — no production caller yet. This is library code that will be wired in by #81.
Now, the one performance concern worth flagging that I *can* verify from the code: **the `image.DecodeConfig` → `image.Decode` double-parse of the full buffered bytes**. For HEIC specifically, this means the wazero/libheif WASM pipeline parses the HEIC header twice. But more importantly, there's a subtler issue I want to verify: does `image.DecodeConfig` actually avoid the heavy work for all four formats? For JPEG/PNG it reads only headers; for HEIC via wazero, `DecodeConfig` may still instantiate the WASM runtime and parse the container. I can't verify the heic library's internals without its source, so I'll note this as unverified rather than assert it.
Let me look at whether there's a meaningful allocation concern in the `bytes.NewReader(raw)` wrapping — it's trivial (a small struct), so no.
Let me double-check one thing: the `downscale` output is `*image.RGBA` always, even when the source was already RGBA and within bounds — but in that case it returns `img` unchanged, so no copy. Good. And when downscaled, the JPEG encoder will read the RGBA — standard.
The one genuinely worth-flagging performance observation I can verify: **the entire input is buffered into memory (`raw`, up to 25 MiB) even when it could be streamed**. But this is *necessary* because `image.DecodeConfig` and `image.Decode` both consume the reader and the stdlib `image` package doesn't support seeking/peeking generically. So this is a justified trade-off, not a regression. I won't flag it.
Let me reconsider the `maxDecodePixels = 100_000_000` cap from the performance lens: at 4 bytes/px (RGBA), that's a ~400 MB decoded bitmap ceiling per request. For an upload-boundary function on a single-user garden app, that's a large but bounded transient allocation. It's a documented, deliberate choice (the comment explains it). Not a regression — it's a guard. I won't flag it as a performance issue; if anything it's a memory-safety concern belonging to another lens.
I've verified everything I can from the checked-out code. The code is not on a hot path (no caller yet), the expensive operations (Catmull-Rom downscale, JPEG re-encode) run once per upload on a cold path, the downscale correctly skips work for in-bounds images, and the full-buffering is forced by the stdlib's two-pass decode pattern. The double parse (`DecodeConfig` then `Decode`) is a minor, unavoidable cost given the API shape.
VERDICT: No material issues found.
- **Buffered read + double parse (lines 106, 116, 124):** `Normalize` reads the full input into `raw` (≤25 MiB) then runs `image.DecodeConfig` and `image.Decode` over `bytes.NewReader(raw)` — two parses of the same bytes. This is forced by the stdlib `image` package: a generic `io.Reader` isn't seekable, so to check dimensions *before* allocating the bitmap (the decompression-bomb guard) you must either buffer or re-open the source. Buffering is the correct choice here. The cost is bounded by `DefaultMaxBytes` and runs once per upload on a cold path (no caller imports this yet — confirmed via grep for `imagenorm` across the repo: only the package and its own tests reference it). Not a regression.
- **Downscale skips in-bounds images (lines 149–151):** `downscale` returns `img` unchanged when `longest <= maxDim`, so a 2048×2048-or-smaller upload pays no extra RGBA allocation and no resample — only the JPEG round-trip. Correct and efficient.
- **Catmull-Rom on a ≤4 MP destination (line 155):** O(dst_pixels × kernel²) once per upload. Catmull-Rom is the pricier kernel choice but the author justifies it (text sharpness on packet photos) and it's a cold path. Fine.
- **`maxDecodePixels` = 100 MP → ~400 MB decoded bitmap ceiling (line 61):** Large transient allocation, but it's a deliberate, documented guard against dimension bombs and is bounded. This is a memory-safety/DoS consideration, not a perf regression — and it belongs to another lens anyway.
I could not inspect the `gen2brain/heic` or `tetratelabs/wazero` sources (module cache not present in the checkout), so I could not verify whether `image.DecodeConfig` on HEIC re-instantiates the wazero runtime per call (a potential per-request setup cost). If that turned out to be expensive it would matter on the request path in #81, but I have no evidence either way from what's checked in, so I'm not reporting it as a finding.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
I have enough to verify. The arithmetic of the overflow claim is correct: `image.Config{Width, Height int}` carries the PNG IHDR's uint32 dimensions directly into `int64` on a 64-bit platform, and `int64(4294967295) * int64(4294967295)` wraps to a negative value (~−8.59×10⁹), which is `<= maxDecodePixels`, so the guard at line 120 is bypassed. The repo's own comments (lines 58–61, 114–115) confirm the guard's stated purpose is exactly to refuse such claimed dimensions, and I confirmed there's no recover in `Normalize`. The minor `format` inconsistency is also confirmed by reading the error returns at lines 108/111/118 vs 121/126/133.
**VERDICT: Blocking issues found**
- **`internal/imagenorm/imagenorm.go:120`** — Integer-overflow bypass of the decompression-bomb guard. `int64(cfg.Width)*int64(cfg.Height)` wraps negative for adversarial headers (e.g. a PNG IHDR claiming ~2³² on each edge: `(2³²−1)²` = `2⁶⁴ − 2³³ + 1`, which overflows `int64` to ≈ −8.59×10⁹), so the `> maxDecodePixels` check is `false` and `image.Decode` proceeds with those claimed dimensions. The guard's whole purpose (documented at lines 58–61, 114–115) is to stop exactly this; it is bypassable by the input shape it claims to defend against, and there is no `recover` in `Normalize`, so the result is an OOM / `makeslice: len out of range` panic rather than the documented `ErrTooLarge`. I verified the arithmetic directly against the source; I could not read the stdlib/`x/image` decoder sources in this sandbox to confirm whether a downstream decoder independently caps dimensions, so exploitability of the *allocation* depends on that — but this code's own guard is broken. Fix: bound each dimension before multiplying, e.g. `if cfg.Width > 0 && cfg.Height > 0 && (int64(cfg.Width) > maxDecodePixels || int64(cfg.Height) > maxDecodePixels || int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels)`, and handle `cfg.Width <= 0 || cfg.Height <= 0` explicitly rather than letting a degenerate header fall through to a confusing decode.
- **`internal/imagenorm/imagenorm.go:108`** (minor) — `format` is returned as `""` on read (108), byte-cap (111), and `DecodeConfig` (118) error paths, but as the decoded format on the pixel-overflow (121), `image.Decode` (126), and `jpeg.Encode` (133) error paths. Inconsistent error-return shape; harmless if callers ignore `format` on `err != nil`, but a footgun. Suggest always returning `""` on error.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
**Verdict: Minor issues**
* **`internal/imagenorm/imagenorm.go:116` and `:124` — Missing panic recovery from third-party image decoders.**
`image.DecodeConfig` and `image.Decode` invoke registered decoders directly; Go's `image` package does not recover from panics inside them. The HEIC path in particular runs libheif inside wazero, where malformed input can trigger WASM traps that surface as panics. A hostile upload therefore kills the goroutine (DoS / 500 storm) instead of returning the intended `ErrUnsupported`.
**Fix:** Wrap both calls in `func() { defer recover(); … }()` and map recovered panics to `ErrUnsupported`.
* **`internal/imagenorm/imagenorm.go:43` — Supply-chain opacity in the HEIC path (informational).**
The PR presents the HEIC decoder as "pure Go, no cgo," but `github.com/gen2brain/heic` executes a bundled WASM blob of libheif. The origin and build provenance of that blob are opaque to downstream consumers, and the project's security posture is not established. This is not a bug in the PR's code, but the feature's critical input path now depends on unauditable compiled C code inside a WASM sandbox. Ensure wazero resource limits are explicitly configured by the library (or document that they are) before this is exposed to user uploads.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
The implementation is correct within its stated scope. Verified by reading the source:
- `internal/imagenorm/imagenorm.go:103-136` — `Normalize` correctly caps the read at `MaxBytes+1`, checks decoded pixel count via `image.DecodeConfig` before full bitmap allocation, downscales preserving aspect ratio with Catmull-Rom, and re-encodes as JPEG.
- `internal/imagenorm/imagenorm.go:142-157` — `downscale` math is sound: `scale = float64(maxDim) / float64(longest)`, new dimensions clamped with `max(..., 1)`, and the `longest <= maxDim || longest == 0` early-return prevents unnecessary resampling.
- `internal/imagenorm/imagenorm_test.go` — Tests cover all four documented formats, oversize rejection, aspect-preserving downscale, and garbage/truncated inputs.
- Confirmed via `grep` that no other package in the repo imports `internal/imagenorm`, so there are no cross-file correctness issues to check.
No logic bugs, incorrect constants, unit mismatches, or semantic errors introduced by this change.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`internal/imagenorm/imagenorm.go:90-91`** — `ErrTooLarge` and `ErrUnsupported` are defined with `fmt.Errorf` instead of `errors.New`. The rest of the codebase (`internal/domain/domain.go:14`, `internal/service/password.go:29`, `internal/agent/runtime.go:52`, etc.) consistently uses `errors.New` for static sentinel errors. This is inconsistent and carries unnecessary formatting overhead. **Fix:** import `"errors"` and use `errors.New("imagenorm: ...")`.
- **`internal/imagenorm/imagenorm_test.go:127-130`** — The comment above `TestNormalizeRejectsGarbage` describes a `TestNormalizeRejectsBombDimensions` test that the function body does not implement (the body tests garbage/truncated input). There is no test anywhere for the `maxDecodePixels` path. **Fix:** rewrite the comment to describe what the function actually tests, and add the missing dimension-bomb test.
- **`internal/imagenorm/testdata/`** — The binary fixtures (`sample.heic`, `sample.webp`) have no in-repo provenance. A future maintainer cannot recreate or audit them. **Fix:** add `internal/imagenorm/testdata/README.md` documenting each fixture's source and how to regenerate it.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
From the Performance lens, the package is well-bounded: input is capped at 25 MiB, decoded pixels are capped at 100 MP before any bitmap is allocated, and downscaling targets a modest 2048 px ceiling. Memory churn is O(max input + max decode + output buffer) with no unbounded growth, no hot loops, and no repeated work per row or pixel. The HEIC path goes through WASM (inherent to the CGO constraint), but that is a library-level trade-off, not a code-level regression introduced here. No N+1 patterns, no unnecessary copies inside loops, and no blocking calls on a path that should be async. The design keeps the fast JPEG re-encode path for in-bounds JPEG inputs by only downscaling when needed.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
* **`internal/imagenorm/imagenorm_test.go:111`** — `image.DecodeConfig` error discarded.
In `TestNormalizeDownscales` the small-image branch does `cfg, _, _ = image.DecodeConfig(bytes.NewReader(out))`. If the normalized JPEG is somehow corrupt, the test reports a misleading dimension mismatch instead of the real decode failure. Check the error with `t.Fatalf` like the big-image branch does.
* **`internal/imagenorm/imagenorm_test.go:127-141`** — The dimension-bomb guard is untested.
The comment claims this tests “a small file claiming a huge canvas”, but `TestNormalizeRejectsGarbage` only exercises garbage bytes and a truncated PNG. No test verifies that a valid header with `Width*Height > maxDecodePixels` returns `ErrTooLarge` before `image.Decode` allocates. Because this is the main memory-safety guard, it deserves a fixture or a hand-crafted header (e.g. a PNG IHDR with 50 000×50 000 pixels and a tiny IDAT).
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Security / correctness (multi-model):
- Wrap the decoders in a recover (decodeSafely): a malformed HEIC/WebP that
panics libheif-via-WASM or x/image/webp now fails this one request as
ErrUnsupported instead of taking the process down.
- Make the pixel-bomb guard overflow-safe: check each side against maxDimension
BEFORE multiplying, so a header claiming ~2^32 on a side can't wrap
int64(w)*int64(h) negative and slip past the pixel-count cap.
- Lower maxDecodePixels 100 MP → 50 MP (~200 MB peak), still above any current
phone sensor, capping the amplification a small hostile header can force.
- Sentinel errors use errors.New, not fmt.Errorf without a verb.
The finding 5 models agreed on: the decompression-bomb guard was UNTESTED and a
stale comment implied otherwise. Added TestNormalizeRejectsPixelBomb, which
crafts a ~40-byte PNG (valid IHDR + CRC) claiming a huge canvas and asserts
ErrTooLarge from DecodeConfig alone, before any bitmap is allocated — covering
both the per-side and the area trip. Fixed the stale comment.
Error-handling / docs:
- format is now "" on ALL error paths (was populated on some, empty on others).
- Doc rewritten to state the actual error contract (read/encode I/O → wrapped,
not a sentinel) and to stop referencing a non-existent TestNormalize / TODO.
- Guard io.LimitReader's +1 against an int64 overflow at an absurd MaxBytes.
Tests / provenance:
- Downscale test derives its numbers from DefaultMaxDim instead of hard-coding.
- Check the previously-ignored DecodeConfig error in the small-image case.
- testdata/README documents where sample.heic/webp came from.
Deferred to the #81 upload handler, documented in the Normalize doc: EXIF
orientation (best fixed and tested with a real oriented photo end-to-end) and
context cancellation (image.Decode isn't cancellable mid-decode; the caller
runs it under a timeout, and the size guards keep the work finite regardless).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 20bf7ee03d into main2026-07-22 03:27:32 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #80. Prerequisite for #81 (seed-packet capture). Part of #86.
Why
majordomo's media pipeline is stdlib-based and cannot decode HEIC or webp. HEIC is the iPhone camera default, so the capture feature's very first input — a photo from a phone — would fail on the device that motivates it. Normalizing at the upload boundary means everything downstream (and majordomo's vision path) only ever sees JPEG.
What
internal/imagenorm.Normalize(r, opts) (jpeg []byte, format string, err error):MaxBytes(25 MiB) →ErrTooLarge,DecodeConfigbefore allocating the bitmap (decompression-bomb guard) →ErrTooLarge,MaxDim(2048, ollama-cloud's limit) on the longest edge, Catmull-Rom (sharp on the text a packet is mostly made of),CGO stays off
github.com/gen2brain/heicruns libheif as WASM via wazero — pure Go, no cgo.golang.org/x/image/webpis pure Go.image.Decode.The ~5 MB these add only links into the binary when #81 imports this package —
cmd/pansydoesn't import it yet, somain's size is unchanged for now (verified: 32.8 MB before and after,CGO_ENABLED=0). The cost lands honestly with the feature that uses it.The test that matters
TestNormalizeAllFormatsround-trips all four formats to a valid JPEG. Its real job is catching a dropped blank import — where the failure is backwards from intuition: delete_ "image/png"and PNG uploads break with "unknown format" while the exotic HEIC still works. Plus downscale (aspect preserved, small images untouched), oversize rejection, and garbage/truncated → cleanErrUnsupported, no panic.heic/webp fixtures live in
testdata/(Go has no encoder for either). The webp is a known-good 16×16 from Python's stdlib test corpus, verified to decode withx/image/webpbefore committing; the heic was made withsips.GOWORK=off go test ./...green;gofmt -l internal/clean; static build confirmed.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 03:17:12Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 11 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +22,4 @@// Go's image decoders register via blank imports, and the failure mode is// backwards from intuition: forget "image/png" and PNG uploads fail with// "unknown format" while the exotic HEIC still works. So the blank imports below// are load-bearing, and TestNormalize exercises all four formats to keep them so.⚪ Package doc references non-existent TestNormalize; actual test is TestNormalizeAllFormats
maintainability · flagged by 1 model
internal/imagenorm/imagenorm.go:25— stale doc reference. The package doc saysTestNormalize exercises all four formats— there is noTestNormalize; the actual test isTestNormalizeAllFormats. Minor, but the CLAUDE.md rule "stale comments are worse than none" applies. Update the reference.🪰 Gadfly · advisory
@@ -0,0 +58,4 @@// maxDecodePixels bounds the decoded bitmap regardless of input byte size — a// small file can claim enormous dimensions (a decompression bomb). 100 MP is// ~400 MB at 4 bytes/px decoded, already generous for any real photo.maxDecodePixels = 100_000_000🟠 maxDecodePixels (100 MP) permits ~400 MB peak allocation per upload — memory-exhaustion DoS surface, and the huge intermediate bitmap is discarded since MaxDim=2048 downscales everything larger
maintainability, security · flagged by 1 model
internal/imagenorm/imagenorm.go:61—maxDecodePixels(100 MP) permits ~400 MB peak allocation per upload. The guard at line 120 rejects only more than 100 MP, so a decoded bitmap up to 100 MP is allowed through toimage.Decode(line 124). At 4 bytes/px (RGBA) that is ~400 MB allocated before the downscale step even runs. A hostile, low-entropy JPEG/HEIC well under the 25 MiB input cap (line 57) can legitimately decode to that size — the byte cap does not bound decoded size for co…🪰 Gadfly · advisory
@@ -0,0 +87,4 @@// ErrTooLarge means the input exceeded the byte cap or decoded to an absurd// pixel count. ErrUnsupported means the bytes weren't a decodable image format.var (ErrTooLarge = fmt.Errorf("imagenorm: image too large")🟡 Sentinel errors use fmt.Errorf instead of errors.New, diverging from repo convention
maintainability · flagged by 4 models
internal/imagenorm/imagenorm.go:90-91—fmt.Errorffor sentinel errors instead oferrors.New. Every other sentinel error declaration in the repo useserrors.New(internal/domain/domain.go:14-64,internal/service/password.go:29,internal/agentmodel/agentmodel.go:39,internal/agent/runtime.go:52).fmt.Errorf("imagenorm: image too large")has no format verbs and pays a fmt-formatting cost for a constant string.errors.Isstill works, but the package diverges from the estab…🪰 Gadfly · advisory
@@ -0,0 +98,4 @@//// It reads at most opts.MaxBytes and refuses a bitmap over maxDecodePixels, so a// hostile upload can neither exhaust memory by byte count nor by claimed// dimensions. A read/format error maps to ErrUnsupported; an over-limit input to🟠 Doc contract overpromises: read/encode I/O errors don't actually map to the documented ErrUnsupported/ErrTooLarge sentinels
error-handling, security · flagged by 3 models
internal/imagenorm/imagenorm.go:103—Normalize(r io.Reader, opts Options)takes nocontext.Context, so once wired to an HTTP handler, decoding an untrusted, WASM-interpreted HEIC (or a large PNG/webp under the 100 MP cap) can't be cancelled. Verified incmd/pansy/main.go:60-67that the server setsWriteTimeout: 30s, buthttp.Server'sWriteTimeoutonly closes the client connection — it does not cancel the in-flight handler goroutine, which will keep burning CPU/decoding to co…🪰 Gadfly · advisory
@@ -0,0 +113,4 @@// Check dimensions BEFORE a full decode, so a decompression bomb is refused// before it allocates its bitmap.cfg, format, err := image.DecodeConfig(bytes.NewReader(raw))🟠 Missing panic recovery around image decoders exposes goroutine DoS from malicious uploads
security · flagged by 2 models
internal/imagenorm/imagenorm.go:116-127—Normalizedoes not recover from panics insideimage.DecodeConfigorimage.Decode. Third-party decoders (especially the WASM-backed HEIC path) are complex C-in-WASM parsers and historically have had crash-on-malformed-input bugs. A single hostile upload can crash the processing goroutine. Since the PR explicitly frames this as a hostile-upload boundary, add adefer recover()guard around the decode phase and map recovered panics to `ErrUns…🪰 Gadfly · advisory
@@ -0,0 +117,4 @@if err != nil {return nil, "", ErrUnsupported}if int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels {🔴 *Integer overflow in decompression-bomb guard: int64(w)int64(h) wraps negative for crafted ~2^32-dimension headers, bypassing the maxDecodePixels check and allowing an OOM/panic in image.Decode instead of ErrTooLarge
error-handling, security · flagged by 3 models
internal/imagenorm/imagenorm.go:120— Integer-overflow bypass of the decompression-bomb guard.int64(cfg.Width)*int64(cfg.Height)wraps negative for adversarial headers (e.g. a PNG IHDR claiming ~2³² on each edge:(2³²−1)²=2⁶⁴ − 2³³ + 1, which overflowsint64to ≈ −8.59×10⁹), so the> maxDecodePixelscheck isfalseandimage.Decodeproceeds with those claimed dimensions. The guard's whole purpose (documented at lines 58–61, 114–115) is to stop exactly this; it is bypassab…🪰 Gadfly · advisory
@@ -0,0 +121,4 @@return nil, format, ErrTooLarge}img, _, err := image.Decode(bytes.NewReader(raw))🟠 EXIF orientation is discarded on decode/re-encode, causing portrait phone photos to be uploaded sideways
correctness, security · flagged by 2 models
EXIF orientation is dropped during normalization —
internal/imagenorm/imagenorm.go:124-134🪰 Gadfly · advisory
@@ -0,0 +151,4 @@}scale := float64(maxDim) / float64(longest)nw, nh := max(int(float64(w)*scale), 1), max(int(float64(h)*scale), 1)dst := image.NewRGBA(image.Rect(0, 0, nw, nh))🟡 *downscale() always targets image.RGBA, forcing jpeg.Encode's slow generic per-pixel path instead of the YCbCr fast path (package has no caller yet in this diff)
performance · flagged by 1 model
internal/imagenorm/imagenorm.go:154(downscale) — When an image needs resizing,downscaledraws into a freshly allocated*image.RGBA(dst := image.NewRGBA(...),draw.CatmullRom.Scale(dst, ...), confirmed at lines 154–155), and that*image.RGBAis whatNormalizehands tojpeg.Encodeat line 132. Go's stdlib JPEG encoder only has a bulk-plane fast path for*image.YCbCrand*image.Gray; any other concrete type — including*image.RGBA— falls back to a generic per-pixel pat…🪰 Gadfly · advisory
@@ -0,0 +86,4 @@// longest edge with aspect ratio preserved, and a small one is left alone.func TestNormalizeDownscales(t *testing.T) {// 4000x1000 → longest 4000, MaxDim 2048 → scaled to 2048x512.big := pngBytes(t, 4000, 1000)🟡 Test silently depends on DefaultMaxDim constant
maintainability · flagged by 1 model
internal/imagenorm/imagenorm_test.go:89—TestNormalizeDownscalessilently couples itself toDefaultMaxDim. It passesOptions{}and then hardcodes expected dimensions (2048,512). IfDefaultMaxDimis ever changed, this test will fail with opaque assertions that don't reveal the root cause. Fix: Make the dependency explicit by passingOptions{MaxDim: 2048}(or compute expectations fromDefaultMaxDimdirectly).🪰 Gadfly · advisory
@@ -0,0 +108,4 @@if err != nil {t.Fatalf("Normalize small: %v", err)}cfg, _, _ = image.DecodeConfig(bytes.NewReader(out))🟡 image.DecodeConfig error ignored in small-image downscale test
error-handling · flagged by 1 model
internal/imagenorm/imagenorm_test.go:111—image.DecodeConfigerror discarded. InTestNormalizeDownscalesthe small-image branch doescfg, _, _ = image.DecodeConfig(bytes.NewReader(out)). If the normalized JPEG is somehow corrupt, the test reports a misleading dimension mismatch instead of the real decode failure. Check the error witht.Fatalflike the big-image branch does.🪰 Gadfly · advisory
@@ -0,0 +124,4 @@}}// TestNormalizeRejectsBombDimensions: a small file claiming a huge canvas is🟠 TestNormalizeRejectsGarbage has a stale comment header describing a non-existent TestNormalizeRejectsBombDimensions; the decompression-bomb (maxDecodePixels) path is untested
correctness, error-handling, maintainability · flagged by 5 models
internal/imagenorm/imagenorm_test.go:127-130— The doc comment aboveTestNormalizeRejectsGarbageis stale: it describes aTestNormalizeRejectsBombDimensionsscenario ("a small file claiming a huge canvas is refused before the bitmap is allocated... the guard is on the DECODED pixel count") but the function's body only exercises garbage bytes and a truncated PNG — it never constructs an oversized-DecodeConfigfixture. No test anywhere in the file (or package) actually drives the `maxDec…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 12 findings (6 with multi-model agreement)
internal/imagenorm/imagenorm_test.go:127internal/imagenorm/imagenorm.go:90internal/imagenorm/imagenorm.go:120internal/imagenorm/imagenorm.go:101internal/imagenorm/imagenorm.go:116internal/imagenorm/imagenorm.go:1246 single-model findings (lower confidence)
internal/imagenorm/imagenorm.go:61internal/imagenorm/imagenorm.go:154internal/imagenorm/imagenorm_test.go:89internal/imagenorm/imagenorm_test.go:111internal/imagenorm/testdata:1internal/imagenorm/imagenorm.go:25Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Review — Security lens
VERDICT: Minor issues
This PR adds a self-contained
imagenormpackage that isn't imported anywhere yet (verified: no non-test caller in the tree), so there is no live attack surface today — the actual exposure will appear when a future handler wires an upload endpoint toNormalize. Reviewing the function as written for what it will face once that happens:Good: input is bounded before read (
io.LimitReader(r, maxBytes+1),imagenorm.go:105), and the decoded-pixel-count guard runs fromimage.DecodeConfigbefore the full bitmap is allocated (imagenorm.go:116-122), which is the right order to defend against a decompression bomb.internal/imagenorm/imagenorm.go:103—Normalize(r io.Reader, opts Options)takes nocontext.Context, so once wired to an HTTP handler, decoding an untrusted, WASM-interpreted HEIC (or a large PNG/webp under the 100 MP cap) can't be cancelled. Verified incmd/pansy/main.go:60-67that the server setsWriteTimeout: 30s, buthttp.Server'sWriteTimeoutonly closes the client connection — it does not cancel the in-flight handler goroutine, which will keep burning CPU/decoding to completion regardless. This isn't exploitable today since nothing callsNormalizeyet, but it's a gap worth closing (accept/honor acontext.Context, or have the future handler impose a deadline) before this is wired to a real upload endpoint.Nothing else in this diff touches auth, injection surfaces, secrets, or deserialization of anything other than image pixel data through Go's own
imagepackage machinery, which is the intended defense here.🎯 Correctness — Minor issues
The finding checks out against the actual source.
internal/imagenorm/imagenorm.godecodes with the stdlibimage.Decode(which does not read EXIFOrientation) and re-encodes withimage/jpeg'sjpeg.Encode(which writes no EXIF at all) — confirmed lines 124–135. A repo-wide grep forexif/orientation(case-insensitive, all.gofiles) returns nothing, andimagenormis only referenced by its own package and test file, so there's no caller-side rotation correction either. Verdict and finding both survive.Verdict: Minor issues
EXIF orientation is dropped during normalization —
internal/imagenorm/imagenorm.go:124-134Normalizedecodes withimage.Decodeand re-encodes withimage/jpeg'sjpeg.Encode. Go's standardimage/jpegdecoder does not read or apply the EXIFOrientationtag — it decodes only the raw pixel matrix.jpeg.Encodewrites no EXIF data into its output. There is no EXIF-reading import anywhere in this package or the rest of the repo (confirmed via repo-wide grep forexif/orientation), andimagenormis called from no other file yet, so there is no caller-side correction either.Concrete failure: many cameras — most Android phones, and many third-party iOS camera/sharing apps that re-encode HEIC to JPEG before an upload flow — store a photo taken in portrait as landscape pixel data plus an
Orientationtag (e.g.6/8) rather than physically rotating the pixels. AfterNormalize, that tag is silently discarded and the pixel data is re-encoded unrotated, so a packet photo taken in portrait comes out sideways (or upside-down/mirrored for orientations 3/2/4/5/7). Since the PR's stated goal is "everything downstream (majordomo's vision path) only ever sees JPEG" — i.e. this normalized output is what the model reads — a mis-rotated packet photo directly degrades the seed-packet-capture feature's primary use case, not just a cosmetic display issue.HEIC is likely less affected in practice (HEIF containers commonly apply
irot/imirtransform properties at decode time rather than relying on a separate orientation tag, sogen2brain/heic's output pixels are plausibly already correctly oriented — unverified here, no network/vendor access to confirm), but this doesn't help the JPEG/PNG-from-Android or re-encoded-JPEG case, which is very plausibly the majority of non-iPhone camera input.Suggested fix: read the EXIF
Orientationtag from the raw JPEG bytes before/alongside decode and apply the corresponding rotate/flip toimgbefore the final JPEG encode.Everything else checked out:
imagenorm.go:104-112):LimitReader(MaxBytes+1)+len(raw) > maxBytescorrectly distinguishes "exactly at cap" from "over."imagenorm.go:116-122): pixel-count check runs onDecodeConfigbefore the fullimage.Decodeallocates a bitmap.imagenorm.go:142-157): scale/nw/nh derivation is consistent, small images are left untouched, and thelongest == 0guard avoids a divide-by-zero.ErrUnsupportedvsErrTooLarge) is applied consistently.🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the real source. Confirms no other package uses
fmt.Errorfwithout a verb, and confirmsTestNormalizeRejectsGarbage's doc comment is indeed a leftover description of a bomb-dimensions test that doesn't exist, with no other test in the file or elsewhere coveringmaxDecodePixels.VERDICT: Minor issues
internal/imagenorm/imagenorm_test.go:127-130— The doc comment aboveTestNormalizeRejectsGarbageis stale: it describes aTestNormalizeRejectsBombDimensionsscenario ("a small file claiming a huge canvas is refused before the bitmap is allocated... the guard is on the DECODED pixel count") but the function's body only exercises garbage bytes and a truncated PNG — it never constructs an oversized-DecodeConfigfixture. No test anywhere in the file (or package) actually drives themaxDecodePixelsbranch atimagenorm.go:120-122, so the comment misleads a future reader into believing that guard is covered when it isn't.internal/imagenorm/imagenorm.go:90-91—ErrTooLarge/ErrUnsupportedusefmt.Errorfwith no formatting verb, whereerrors.Newis called for elsewhere. Every sentinel error ininternal/domain/domain.go(14 instances) useserrors.New, and this file itself usesfmt.Errorfcorrectly for wrapping at lines 108 and 133 with%w. This is the only place ininternal/that declares a plain sentinel withfmt.Errorf.⚡ Performance — Minor issues
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:154(downscale) — When an image needs resizing,downscaledraws into a freshly allocated*image.RGBA(dst := image.NewRGBA(...),draw.CatmullRom.Scale(dst, ...), confirmed at lines 154–155), and that*image.RGBAis whatNormalizehands tojpeg.Encodeat line 132. Go's stdlib JPEG encoder only has a bulk-plane fast path for*image.YCbCrand*image.Gray; any other concrete type — including*image.RGBA— falls back to a generic per-pixel path that calls.At(x, y)and does an RGBA→YCbCr conversion per pixel. This is well-established, widely documented Go stdlib behavior; I was not able to re-readimage/jpeg/writer.godirectly in this sandbox (out-of-repo file reads are blocked here too), so, like the original author, I'm not treating it as tool-verified against the stdlib source itself — confidence stays medium on that specific sub-claim.internal/imagenormand its test — a repo-wide grep turns up zero callers ofimagenorm.Normalizeoutside the package itself. So the "upload hot path" / "essentially 100% of real-world uploads" language overstates the current, verifiable impact of this diff — there is no wired-up caller yet for this to cost CPU on. The underlying code observation (resize path always targets RGBA, forcing the slow encode path once this is wired to an upload handler) still holds and is worth flagging before the package gets its first caller.*image.YCbCrtarget instead of*image.RGBA(e.g.image.NewYCbCr+ a scaler that can write into it, or convert post-scale) to hit the encoder's fast path.🧯 Error handling & edge cases — Minor issues
Confirmed both findings are accurate. There is no other test elsewhere in the repo exercising the
maxDecodePixels/bomb-dimensions branch, and the package isn't yet wired into any caller, so the doc-contract mismatch is real (no other file's error handling masks it). Both findings survive verification unchanged.Corrected review
VERDICT: Minor issues
Reviewed
internal/imagenorm/imagenorm.goand its test file. The core guards (byte cap, pre-decode pixel-count check, gracefulErrUnsupportedon garbage/truncated input, no re-sampling on already-small images) are implemented correctly — no panics on empty/degenerate dimensions, no unguarded type assertions, negative/zeroOptionsfields fall back safely to defaults (imagenorm.go:73-85).internal/imagenorm/imagenorm.go:99-102— the doc comment states "A read/format error maps to ErrUnsupported," but that's only true for the format-decode branch (imagenorm.go:117-118,125-126). A genuine I/O failure fromio.ReadAll(imagenorm.go:106-109) returns a distinct wrapped error (fmt.Errorf("imagenorm: read: %w", err)), notErrUnsupported, and thejpeg.Encodefailure path (imagenorm.go:132-134) likewise returns an unwrapped-from-sentinel error. A caller relying onerrors.Is(err, ErrUnsupported)per the documented contract will silently miscategorize these two paths. Fix: either wrap read/encode failures so they satisfy a documented sentinel, or correct the doc to describe the actual error shape.internal/imagenorm/imagenorm_test.go:127-130— the comment aboveTestNormalizeRejectsGarbageis titledTestNormalizeRejectsBombDimensionsand describes "a small file claiming a huge canvas... refused before the bitmap is allocated," but the function it labels only tests garbage bytes and a truncated PNG (imagenorm_test.go:131-140) — it never constructs an input that trips themaxDecodePixels/ErrTooLargebranch atimagenorm.go:120-122. Confirmed via repo-wide grep: no test anywhere exercises that branch. The decompression-bomb guard — the safety property called out as the point of the pixel-count pre-check — has zero test coverage. Fix: add the actual bomb-dimensions test the comment describes (e.g. a spoofed IHDR/DecodeConfig claiming a huge width/height) so a regression in that branch (e.g.>flipped to>=) would be caught.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
The code confirms the first finding.
maxDecodePixels = 100_000_000(line 61), the check at line 120 uses strict>, so exactly 100,000,000 px (e.g. 10000×10000) passes and decodes to a ~400 MB RGBA bitmap, while a flat-color PNG of that size is only kilobytes on disk — well under the 25 MiB input cap.downscalethen allocates an additional RGBA plus the JPEG output buffer on top. No concurrency bound exists in the package. This is a real memory-amplification DoS vector at an untrusted upload boundary.The second finding is explicitly low-confidence defense-in-depth (and not in the JSON block); the draft itself notes it couldn't confirm a bypass. Dropping it.
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:120— ThemaxDecodePixelsguard (100 MP) is too permissive and undermines the decompression-bomb protection it claims to provide. PNG is a pixel-raster format whose byte size is decoupled from its decoded bitmap size: a flat-color 10000×10000 image is ~kilobytes on disk but decodes to a 10000×10000×4 = 400 MB RGBA bitmap. The input cap (25 MiB) and theDecodeConfigcheck don't prevent this, because the byte count is tiny and the dimension check uses>(strictly greater), so exactly 100,000,000 px passes. So a hostile-but-valid ~KB PNG forces a ~400 MB allocation per request — a ~40,000× memory amplification — and nothing in this package bounds concurrent decodes. On top of that,downscaleallocates a second RGBA (up to 2048²×4 ≈ 16 MB) plus the JPEG output buffer. For an upload boundary ingesting untrusted photos, that's a DoS vector. The fix is to lowermaxDecodePixelsto something tied to the actual use case (a real phone photo is ~12 MP; 24 MP is generous) rather than "any photo ever." Verified the math againstdownscale/jpeg.Encode(which allocate the RGBA + output buffer on top of the decoded image) by readingimagenorm.go:103-157.🎯 Correctness — No material issues found
I've verified the two draft findings against the actual code and Go's image/draw semantics. Here is my assessment.
Finding 1 (
imagenorm.go:155, draw.Over vs draw.Src): I read the code (lines 142-157):downscaleallocatesdst := image.NewRGBA(...)— a freshly-zeroed, fully-transparent destination (all0,0,0,0) — then callsdraw.CatmullRom.Scale(dst, dst.Bounds(), img, b, draw.Over, nil).The draft's central actionable claim is that switching
draw.Over→draw.Srcwould fix the alleged darkening. I cannot confirm that, and in fact it is contradicted by Porter-Duff semantics: with a destination whose alpha is zero,Overreduces toresult = src(premultiplied), andSrcis defined asresult = src(premultiplied). The two operators produce byte-identical output on a zero/transparent destination — the draft even states this ("over a zero dst gives dstRGB = srcRGB*srcA") but then claimsdraw.Srcwould instead "copy the original RGB verbatim." That is incorrect:draw.Srcwrites the same premultiplied value thatdraw.Overwrites here. So the recommended fix is a no-op and the finding's technical premise does not hold. Whether any darkening occurs downstream depends entirely on howimage/jpegreads a premultiplied*image.RGBA(stdlib code not present in this repo, so I cannot positively confirm or refute it), but the proposed remedy would not address it regardless. Per the gate rule (drop what I cannot confirm, and drop findings the code contradicts), this is dropped.Finding 2 (
imagenorm.go:99-104, doc prose "reads at most opts.MaxBytes"): Verified: the comment at line 99 says "It reads at most opts.MaxBytes," and the code at lines 104-105 reads viaLimitReader(r, maxBytes()+1)then rejects iflen(raw) > maxBytes(). The+1is an intentional boundary-detection detail; the function never accepts more thanopts.MaxBytesfor processing, so the contract-level statement is defensible. This is a trivial prose nuance at most, and not a material issue.No surviving material findings.
VERDICT: No material issues found
internal/imagenorm/imagenorm.go:155— examined thedownscalecompositing. The destination is a freshly-allocated, fully-transparent*image.RGBA(image.NewRGBA). Under Porter-Duff rules,draw.Overover a zero-alpha destination anddraw.Srcboth reduce toresult = src(premultiplied) and produce identicalPixbytes. Therefore the draft's claim that switching todraw.Srcwould "copy the original RGB verbatim" and fix darkening is not supported by the operator semantics; the suggested fix would be a no-op. Any downstream darkening (if it exists) would depend onimage/jpeg's reading of a premultiplied*image.RGBA, which cannot be verified from this repository, and is not addressed bydraw.Srcregardless. Dropped.internal/imagenorm/imagenorm.go:99— the comment says "It reads at most opts.MaxBytes"; the implementation readsmaxBytes()+1to distinguish at-cap from over-cap, then rejects anything overmaxBytes(). The accepted/processed input is in fact capped atopts.MaxBytes, so the contract-level statement holds; the+1is an intentional implementation detail. Trivial at most, not material. Dropped.I also re-derived the downscale math for the
4000×1000test case againstdownscale(lines 142-157):longest=4000,scale=2048/4000=0.512,nw=int(2048.0)=2048,nh=int(512.0)=512, matching the asserted2048×512with aspect preserved. The byte-cap logic (LimitReader(maxBytes+1)thenlen > maxBytes) correctly distinguishes exactly-at-cap from over-cap, and the bomb guard (cfg.Width*cfg.Height > maxDecodePixelsat line 120) is correctly ordered before the fullimage.Decodeat line 124.🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed against the actual source.
VERDICT: Minor issues
imagenorm_test.go:127— mismatched test name and doc comment. The comment at lines 127–129 saysTestNormalizeRejectsBombDimensions: a small file claiming a huge canvas...but the function declared at line 130 isTestNormalizeRejectsGarbage, which only feeds garbage/truncated bytes ("not an image at all"and a halved PNG) and assertsErrUnsupported. ThemaxDecodePixelsdecompression-bomb path (imagenorm.go:120, which returnsErrTooLargefor a huge claimed canvas) is never exercised by any test in the file (confirmed by grep — no other test referencesmaxDecodePixelsor constructs an oversize-canvas fixture). The stale comment claims a guard test that does not exist. Fix: either add aTestNormalizeRejectsBombDimensionsthat feeds a PNG with a huge declared canvas under the byte cap, or reword the comment to describe whatTestNormalizeRejectsGarbageactually does.imagenorm.go:90-91— exported sentinel errors built viafmt.Errorfrather thanerrors.New.fmt.Errorf("imagenorm: image too large")andfmt.Errorf("imagenorm: unsupported or corrupt image")contain no verbs, soerrors.New(...)is the idiomatic choice. Cosmetic, but inconsistent with the otherwise careful style of the surrounding code.⚡ Performance — No material issues found
VERDICT: No material issues found
This is new code on the upload boundary (a cold path processing one image at a time), and the resource costs are bounded and deliberate. Reviewed through the performance lens:
internal/imagenorm/imagenorm.go:116and:124):DecodeConfigandimage.Decodeeach get a freshbytes.Readerover the same in-memoryraw, so the format header is parsed twice. This is negligible relative to the full bitmap decode/resample/encode that follows, and the two-pass structure is what enables the pre-allocation decompression-bomb guard. Not worth changing.:105–:106): up toMaxBytes(25 MiB) is buffered entirely before decoding, rather than streaming. This is by design (two passes over the bytes for the size guard), and the cap keeps the transient footprint bounded. Acceptable.draw.CatmullRom.Scale(:155): Catmull-Rom is the costliest kernel scaler inx/image/draw, but the PR justifies it for text sharpness, it only runs when an image actually exceedsMaxDim, and it's off the hot path. A deliberate quality/speed trade-off, not a regression.maxDecodePixels = 100MP≈ 400 MB at 4 B/px): the cap is generous and a 100 MP image that compresses under 25 MiB could force a large transient allocation. This is acknowledged in the code comments and is a design choice on a cold path, not an inefficiency to flag.No unbounded growth, N+1 patterns, hot-loop regressions, or avoidable quadratic behavior were found.
🧯 Error handling & edge cases — No material issues found
I've verified the error-handling paths against the checked-out code. The implementation is solid in my lane.
Verification notes:
imagenorm.go:104-112):LimitReader(r, max+1)thenlen(raw) > maxBytes()correctly distinguishes "exactly at cap" (accepted) from "over" (ErrTooLarge). Read errors are wrapped, not swallowed (:108).:116-122):DecodeConfigdimensions checked inint64arithmetic (no overflow for MaxInt32-scale dims) beforeimage.Decodeallocates the bitmap. Correct ordering.:116-127): bothDecodeConfigandDecodefailures map toErrUnsupported;formatfrom the successfulDecodeConfigis carried through on the partial-failure path. No panic surface introduced by the stdlib decoders.downscale(:142-157): zero-dimension guarded (longest == 0→ return unchanged);max(..., 1)prevents a zero-height/widthNewRGBAwhen the short edge rounds down to 0 (e.g. 2049×1 → nh=1). Scaling math can't overflow int:w*h ≤ maxDecodePixels(100M) is enforced upstream, andscale < 1in the downscale branch, sonw,nh ≤ w,h ≤ 100M— well within int32.Options.MaxDim/MaxBytesfall through to defaults via the> 0checks (:73-85), so a misconfigured caller can't drive a zero/negative cap intoLimitReaderor the pixel check.One thing I could not locally verify (heic source isn't vendored in the repo): whether
github.com/gen2brain/heicregistersimage.DecodeConfigsuch that the bomb guard at:116runs for HEIC beforeimage.Decode. If it only registeredDecode, every HEIC would fail at:117withErrUnsupported— but the PR'sTestNormalizeAllFormatsheic case exercises exactly this path and is reported green, which is strong evidence the registration is correct. Not a finding against the reviewed code.No material issues found in the error-handling & edge-case lens.
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:116-127—Normalizedoes not recover from panics insideimage.DecodeConfigorimage.Decode. Third-party decoders (especially the WASM-backed HEIC path) are complex C-in-WASM parsers and historically have had crash-on-malformed-input bugs. A single hostile upload can crash the processing goroutine. Since the PR explicitly frames this as a hostile-upload boundary, add adefer recover()guard around the decode phase and map recovered panics toErrUnsupportedso a bad file is a clean error, not a goroutine (or process) kill.🎯 Correctness — No material issues found
No material issues found
I scrutinized the normalization logic, dimensions, thresholds, and format handling:
io.LimitReader(r, int64(opts.maxBytes())+1)followed bylen(raw) > opts.maxBytes()correctly distinguishes “exactly at the cap” from “over the cap” and prevents unbounded reads. Edge cases (empty reader, exact-size input) behave correctly.int64(cfg.Width)*int64(cfg.Height) > maxDecodePixelsusesint64to avoid overflow on the product, and the check happens beforeimage.Decodeallocates the bitmap. The threshold (100 MP ≈ 400 MB RGBA) is consistent with the stated intent.scale := float64(maxDim) / float64(longest)withmax(int(float64(w)*scale), 1)preserves aspect ratio and guarantees at least a 1×1 output. The defaultMaxDim=2048keeps the post-scale image well under the pixel cap, and the 4000×1000 test case evaluates to exactly 2048×512 with IEEE-754 double precision, so the aspect test is deterministic.image/jpeg,image/png,github.com/gen2brain/heic,golang.org/x/image/webp) register withimage.Decodeby side effect. The testTestNormalizeAllFormatsround-trips every claimed format and would catch a dropped import.DecodeConfig/Decodeand is mapped cleanly toErrUnsupported. Oversize bytes and oversize dimensions both map toErrTooLarge. No panic paths were found.jpeg.Encodewith quality 85 on an*image.RGBAdestination is well-formed; the output is validated in tests.Nothing in the diff introduces a logic bug, unit mismatch, boundary error, or semantic domain issue.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm_test.go:127— Comment namesTestNormalizeRejectsBombDimensionsbut the actual function isTestNormalizeRejectsGarbage, violating the standard Go convention that a comment should introduce the function it precedes. Worse, the described decompression-bomb scenario (a small file claiming huge decoded dimensions) is never exercised in the test body; only garbage bytes and truncated PNGs are tested. A future maintainer will reasonably believe the bomb guard has direct regression coverage when it does not.Fix: Rename the test to match the comment (or vice-versa) and add an actual bomb-dimension test case—e.g. a minimal crafted header with a huge
Width/Height—so the safety feature is locked.internal/imagenorm/imagenorm_test.go:89—TestNormalizeDownscalessilently couples itself toDefaultMaxDim. It passesOptions{}and then hardcodes expected dimensions (2048,512). IfDefaultMaxDimis ever changed, this test will fail with opaque assertions that don't reveal the root cause.Fix: Make the dependency explicit by passing
Options{MaxDim: 2048}(or compute expectations fromDefaultMaxDimdirectly).⚡ Performance — No material issues found
No material issues found
The code is well-bounded and avoids the common performance traps for an image-pipeline package:
io.LimitReader+io.ReadAllwithDefaultMaxBytes = 25 MiBprevents unbounded memory consumption from a malicious stream.maxDecodePixels = 100Mis enforced beforeimage.Decodeallocates the bitmap, blocking decompression bombs.jpeg.Encodeis bounded byDefaultMaxDim = 2048, so the destination RGBA is at most ~16 MiB.downscalereturns the original image unchanged when it already fits, avoiding an unnecessaryCatmullRomresample.🧯 Error handling & edge cases — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:120— Missing dimension positivity validation.cfg.Widthorcfg.Heightcould be zero or negative from a malformed image or buggy third-party decoder. The productint64(cfg.Width)*int64(cfg.Height)then yields a negative or zero value that passes the> maxDecodePixelsguard, allowing the decode to proceed. Fix: addif cfg.Width <= 0 || cfg.Height <= 0 { return nil, "", ErrUnsupported }before the pixel count check.internal/imagenorm/imagenorm.go:111vsimagenorm.go:121— Inconsistentformatreturn onErrTooLarge. The byte-cap path returns""for format, but the pixel-count path returns the detectedformat. Callers inspectingformatwhenerr != nilsee different behavior depending on which limit triggered. Fix: returnnil, "", ErrTooLargein both cases for consistency.internal/imagenorm/imagenorm.go:105— Potentialint64overflow inio.LimitReaderlimit. If a caller passesOptions{MaxBytes: math.MaxInt64},int64(opts.maxBytes())+1wraps to a negative limit, causingio.ReadAllto read nothing and the function to returnErrUnsupportedinstead of a meaningful error. Fix: avoid the+1by reading at mostmaxBytesand detecting truncation separately (e.g., comparelen(raw)to the limit, or read one extra byte with overflow-safe math).opencode/glm-5.2:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
The draft's single finding is confirmed against the code:
maxDecodePixels = 100_000_000✓> maxDecodePixelsbefore full decode, so up to 100 MP is allowed ✓image.Decodethen allocates the full bitmap — 100 MP × 4 bytes/px (RGBA) = ~400 MB ✓DefaultMaxDim = 2048anddownscalediscards anything larger, so the huge intermediate serves no functional purpose ✓No callers wire
imagenormyet (grep confirms only the package itself references the name), so this is latent attack surface rather than a live endpoint, but the constants and decode semantics are as the draft describes. The HEICDecodeConfignote in the draft is explicitly not a finding, so nothing to verify there.VERDICT: Minor issues
internal/imagenorm/imagenorm.go:61—maxDecodePixels(100 MP) permits ~400 MB peak allocation per upload. The guard at line 120 rejects only more than 100 MP, so a decoded bitmap up to 100 MP is allowed through toimage.Decode(line 124). At 4 bytes/px (RGBA) that is ~400 MB allocated before the downscale step even runs. A hostile, low-entropy JPEG/HEIC well under the 25 MiB input cap (line 57) can legitimately decode to that size — the byte cap does not bound decoded size for compressible payloads. SinceDefaultMaxDim = 2048(line 53) anddownscale(line 129) immediately throw away and re-sample anything larger on its longest edge, a 400 MB intermediate bitmap serves no functional purpose and is pure attack surface: one request can force ~400 MB allocation, and concurrent uploads multiply it. The guard exists but the ceiling is far too generous for the actual use. Suggested fix: tie the pixel cap to the output need (e.g.maxDecodePixels = maxDim*maxDim * 4≈ 16 MP at default, still ample headroom for Catmull-Rom downscaling) so a hostile upload cannot force an allocation many times larger than anything that survives normalization. Verified by reading the code; the 25 MiB → 100 MP → 400 MB path follows directly from the constants andimage.Decodesemantics.🎯 Correctness — Minor issues
Confirmed. Comment at line 127 says
TestNormalizeRejectsBombDimensionsbut function at line 130 isTestNormalizeRejectsGarbage. The body tests garbage + truncated PNG only, never themaxDecodePixelsguard. Verified.VERDICT: Minor issues
Verified by reading
/tmp/gadfly/repo/internal/imagenorm/imagenorm.goandimagenorm_test.goin full.internal/imagenorm/imagenorm_test.go:127-130— misleading/stale doc comment; the decompression-bomb guard is untested. The comment block saysTestNormalizeRejectsBombDimensions: a small file claiming a huge canvas is refused before the bitmap is allocated, but the function actually defined immediately below isTestNormalizeRejectsGarbage, which only feeds in"not an image at all"and a truncated PNG. Neither case exercises theint64(cfg.Width)*int64(cfg.Height) > maxDecodePixelscheck atimagenorm.go:120— there is no test anywhere that constructs a small file with huge claimed dimensions to confirm the guard rejects it. ThemaxDecodePixelsguard is the central safety property the PR description advertises ("decompression-bomb guard"), and it has zero coverage. A maintainer reading the comment would believe the guard is tested when it is not. Suggested fix: add a real bomb-dimension case (e.g. a crafted PNG/HEIC header claiming huge dimensions within the byte cap) assertingErrTooLarge, and fix the comment to match the function name.The downscale math, byte-cap logic, aspect preservation, format-string handling, and the four-format round-trip assertions all check out on re-derivation (4000×1000 → 2048×512; small images untouched; oversize input →
ErrTooLarge;formatfromDecodeConfigis preserved through theDecodecall).🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual checked-out files.
Now let me verify the convention claims about sentinel errors elsewhere in the repo.
Now let me verify the
maxBytes()call count claim in finding #4. The draft says "called three times" — let me count precisely.Looking at the code:
int64(opts.maxBytes())+1— call 1len(raw) > opts.maxBytes()— call 2Only two calls in
Normalize, not three. The draft's "three times" claim is factually wrong.All other findings verified:
fmt.Errorffor sentinels (lines 90-91) — CONFIRMED. Every other sentinel declaration in the repo useserrors.New(domain.go, password.go, agentmodel.go, runtime.go, journal.go). The other twofmt.Errorf("...")matches (revisions.go:713, instance_settings.go:41) are inline returns, not sentinel declarations.Test comment/name mismatch (lines 127-130) — CONFIRMED. Comment says
TestNormalizeRejectsBombDimensions:but function isTestNormalizeRejectsGarbage; body tests garbage + truncated input, never bomb dimensions.Stale doc reference (line 25) — CONFIRMED. Package doc says
TestNormalize exercises all four formats; no such function exists, actual test isTestNormalizeAllFormats.maxDecodePixelsnot inOptions(line 61) — CONFIRMED. It's an unexported const whileMaxDim/MaxBytesare tunable viaOptions.Finding #4 drops: the "three times" claim is false (it's two calls), and the underlying suggestion is trivial enough that the incorrect premise disqualifies it.
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:90-91—fmt.Errorffor sentinel errors instead oferrors.New. Every other sentinel error declaration in the repo useserrors.New(internal/domain/domain.go:14-64,internal/service/password.go:29,internal/agentmodel/agentmodel.go:39,internal/agent/runtime.go:52).fmt.Errorf("imagenorm: image too large")has no format verbs and pays a fmt-formatting cost for a constant string.errors.Isstill works, but the package diverges from the established convention for no benefit. Useerrors.New.internal/imagenorm/imagenorm_test.go:127-130— misleading comment / name mismatch. The doc comment forTestNormalizeRejectsGarbageleads with// TestNormalizeRejectsBombDimensions:— a function name that does not exist and describes a test the function does not perform (it never feeds a small-file/huge-dimension input; that case isn't covered anywhere). The body tests garbage + truncated input. Either rename to match the comment and add the bomb-dimensions case (a header claiming huge width/height is exactly what themaxDecodePixelsguard is for, and currently has no coverage), or fix the comment to describe what the test actually does.internal/imagenorm/imagenorm.go:25— stale doc reference. The package doc saysTestNormalize exercises all four formats— there is noTestNormalize; the actual test isTestNormalizeAllFormats. Minor, but the CLAUDE.md rule "stale comments are worse than none" applies. Update the reference.internal/imagenorm/imagenorm.go:61—maxDecodePixelsis an unexported constant whileMaxDim/MaxBytesare exposed viaOptions. The decompression-bomb pixel cap is a hard unexported constant, inconsistent with the "Options tunes Normalize" framing — a caller can't tighten it. That's a defensible safety choice, but the asymmetry is undocumented and looks accidental. A one-line comment explaining why it's intentionally not inOptionswould help the next reader.⚡ Performance — No material issues found
I'll review this through the performance lens. Let me examine the actual code.
The file matches the diff. Now let me consider the performance profile of this code carefully.
The key concern from a performance lens:
1. Full file buffered into memory twice over.
Normalizereads the entire input (up to 25 MiB) intorawviaio.ReadAll, then passesbytes.NewReader(raw)to bothimage.DecodeConfigandimage.Decode. TheDecodeConfig→Decodepattern requires two passes, which is why the author buffers fully — that's a reasonable, even necessary, trade-off given the stdlib'simagepackage re-reads from aReader(which isn't seekable in the general case). But combined with the decoded bitmap (up to ~400 MB at themaxDecodePixelscap) and the output buffer, peak memory israw + decoded_bitmap + dst_bitmap (if downscaled) + output. This is inherent to the design and bounded by the caps; not a regression.2. Downscaling allocation when the image is already within bounds but re-encoded.
downscalereturnsimgunchanged whenlongest <= maxDim, so no extra RGBA allocation happens for in-bounds images. Good.3. Catmull-Rom on a 2048×2048 RGBA destination. This is the most expensive part per-call but runs once per upload (a cold path — a user submitting a seed packet photo), not in a hot loop. Catmull-Rom is O(dst_pixels × kernel²) which is fine for a 4 MP output. Not a concern.
Let me check whether
image.Decodefor HEIC via wazero has any notable per-call cost (e.g. re-instantiating a WASM runtime per decode) that would matter on a request path.The module cache isn't vendored/checked out, so I can't inspect the heic library source directly. Let me check if there's a vendor dir or go module cache accessible.
No vendor dir. The heic/wazero sources aren't available for me to inspect directly. Let me reason about what I can verify from the code itself.
The package isn't imported anywhere yet (per the PR description, #81 will import it), so this code is not on any current hot path. Let me confirm no current caller exists.
Confirmed — no production caller yet. This is library code that will be wired in by #81.
Now, the one performance concern worth flagging that I can verify from the code: the
image.DecodeConfig→image.Decodedouble-parse of the full buffered bytes. For HEIC specifically, this means the wazero/libheif WASM pipeline parses the HEIC header twice. But more importantly, there's a subtler issue I want to verify: doesimage.DecodeConfigactually avoid the heavy work for all four formats? For JPEG/PNG it reads only headers; for HEIC via wazero,DecodeConfigmay still instantiate the WASM runtime and parse the container. I can't verify the heic library's internals without its source, so I'll note this as unverified rather than assert it.Let me look at whether there's a meaningful allocation concern in the
bytes.NewReader(raw)wrapping — it's trivial (a small struct), so no.Let me double-check one thing: the
downscaleoutput is*image.RGBAalways, even when the source was already RGBA and within bounds — but in that case it returnsimgunchanged, so no copy. Good. And when downscaled, the JPEG encoder will read the RGBA — standard.The one genuinely worth-flagging performance observation I can verify: the entire input is buffered into memory (
raw, up to 25 MiB) even when it could be streamed. But this is necessary becauseimage.DecodeConfigandimage.Decodeboth consume the reader and the stdlibimagepackage doesn't support seeking/peeking generically. So this is a justified trade-off, not a regression. I won't flag it.Let me reconsider the
maxDecodePixels = 100_000_000cap from the performance lens: at 4 bytes/px (RGBA), that's a ~400 MB decoded bitmap ceiling per request. For an upload-boundary function on a single-user garden app, that's a large but bounded transient allocation. It's a documented, deliberate choice (the comment explains it). Not a regression — it's a guard. I won't flag it as a performance issue; if anything it's a memory-safety concern belonging to another lens.I've verified everything I can from the checked-out code. The code is not on a hot path (no caller yet), the expensive operations (Catmull-Rom downscale, JPEG re-encode) run once per upload on a cold path, the downscale correctly skips work for in-bounds images, and the full-buffering is forced by the stdlib's two-pass decode pattern. The double parse (
DecodeConfigthenDecode) is a minor, unavoidable cost given the API shape.VERDICT: No material issues found.
Normalizereads the full input intoraw(≤25 MiB) then runsimage.DecodeConfigandimage.Decodeoverbytes.NewReader(raw)— two parses of the same bytes. This is forced by the stdlibimagepackage: a genericio.Readerisn't seekable, so to check dimensions before allocating the bitmap (the decompression-bomb guard) you must either buffer or re-open the source. Buffering is the correct choice here. The cost is bounded byDefaultMaxBytesand runs once per upload on a cold path (no caller imports this yet — confirmed via grep forimagenormacross the repo: only the package and its own tests reference it). Not a regression.downscalereturnsimgunchanged whenlongest <= maxDim, so a 2048×2048-or-smaller upload pays no extra RGBA allocation and no resample — only the JPEG round-trip. Correct and efficient.maxDecodePixels= 100 MP → ~400 MB decoded bitmap ceiling (line 61): Large transient allocation, but it's a deliberate, documented guard against dimension bombs and is bounded. This is a memory-safety/DoS consideration, not a perf regression — and it belongs to another lens anyway.I could not inspect the
gen2brain/heicortetratelabs/wazerosources (module cache not present in the checkout), so I could not verify whetherimage.DecodeConfigon HEIC re-instantiates the wazero runtime per call (a potential per-request setup cost). If that turned out to be expensive it would matter on the request path in #81, but I have no evidence either way from what's checked in, so I'm not reporting it as a finding.🧯 Error handling & edge cases — Blocking issues found
I have enough to verify. The arithmetic of the overflow claim is correct:
image.Config{Width, Height int}carries the PNG IHDR's uint32 dimensions directly intoint64on a 64-bit platform, andint64(4294967295) * int64(4294967295)wraps to a negative value (~−8.59×10⁹), which is<= maxDecodePixels, so the guard at line 120 is bypassed. The repo's own comments (lines 58–61, 114–115) confirm the guard's stated purpose is exactly to refuse such claimed dimensions, and I confirmed there's no recover inNormalize. The minorformatinconsistency is also confirmed by reading the error returns at lines 108/111/118 vs 121/126/133.VERDICT: Blocking issues found
internal/imagenorm/imagenorm.go:120— Integer-overflow bypass of the decompression-bomb guard.int64(cfg.Width)*int64(cfg.Height)wraps negative for adversarial headers (e.g. a PNG IHDR claiming ~2³² on each edge:(2³²−1)²=2⁶⁴ − 2³³ + 1, which overflowsint64to ≈ −8.59×10⁹), so the> maxDecodePixelscheck isfalseandimage.Decodeproceeds with those claimed dimensions. The guard's whole purpose (documented at lines 58–61, 114–115) is to stop exactly this; it is bypassable by the input shape it claims to defend against, and there is norecoverinNormalize, so the result is an OOM /makeslice: len out of rangepanic rather than the documentedErrTooLarge. I verified the arithmetic directly against the source; I could not read the stdlib/x/imagedecoder sources in this sandbox to confirm whether a downstream decoder independently caps dimensions, so exploitability of the allocation depends on that — but this code's own guard is broken. Fix: bound each dimension before multiplying, e.g.if cfg.Width > 0 && cfg.Height > 0 && (int64(cfg.Width) > maxDecodePixels || int64(cfg.Height) > maxDecodePixels || int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels), and handlecfg.Width <= 0 || cfg.Height <= 0explicitly rather than letting a degenerate header fall through to a confusing decode.internal/imagenorm/imagenorm.go:108(minor) —formatis returned as""on read (108), byte-cap (111), andDecodeConfig(118) error paths, but as the decoded format on the pixel-overflow (121),image.Decode(126), andjpeg.Encode(133) error paths. Inconsistent error-return shape; harmless if callers ignoreformatonerr != nil, but a footgun. Suggest always returning""on error.opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Verdict: Minor issues
internal/imagenorm/imagenorm.go:116and:124— Missing panic recovery from third-party image decoders.image.DecodeConfigandimage.Decodeinvoke registered decoders directly; Go'simagepackage does not recover from panics inside them. The HEIC path in particular runs libheif inside wazero, where malformed input can trigger WASM traps that surface as panics. A hostile upload therefore kills the goroutine (DoS / 500 storm) instead of returning the intendedErrUnsupported.Fix: Wrap both calls in
func() { defer recover(); … }()and map recovered panics toErrUnsupported.internal/imagenorm/imagenorm.go:43— Supply-chain opacity in the HEIC path (informational).The PR presents the HEIC decoder as "pure Go, no cgo," but
github.com/gen2brain/heicexecutes a bundled WASM blob of libheif. The origin and build provenance of that blob are opaque to downstream consumers, and the project's security posture is not established. This is not a bug in the PR's code, but the feature's critical input path now depends on unauditable compiled C code inside a WASM sandbox. Ensure wazero resource limits are explicitly configured by the library (or document that they are) before this is exposed to user uploads.🎯 Correctness — No material issues found
No material issues found.
The implementation is correct within its stated scope. Verified by reading the source:
internal/imagenorm/imagenorm.go:103-136—Normalizecorrectly caps the read atMaxBytes+1, checks decoded pixel count viaimage.DecodeConfigbefore full bitmap allocation, downscales preserving aspect ratio with Catmull-Rom, and re-encodes as JPEG.internal/imagenorm/imagenorm.go:142-157—downscalemath is sound:scale = float64(maxDim) / float64(longest), new dimensions clamped withmax(..., 1), and thelongest <= maxDim || longest == 0early-return prevents unnecessary resampling.internal/imagenorm/imagenorm_test.go— Tests cover all four documented formats, oversize rejection, aspect-preserving downscale, and garbage/truncated inputs.grepthat no other package in the repo importsinternal/imagenorm, so there are no cross-file correctness issues to check.No logic bugs, incorrect constants, unit mismatches, or semantic errors introduced by this change.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:90-91—ErrTooLargeandErrUnsupportedare defined withfmt.Errorfinstead oferrors.New. The rest of the codebase (internal/domain/domain.go:14,internal/service/password.go:29,internal/agent/runtime.go:52, etc.) consistently useserrors.Newfor static sentinel errors. This is inconsistent and carries unnecessary formatting overhead. Fix: import"errors"and useerrors.New("imagenorm: ...").internal/imagenorm/imagenorm_test.go:127-130— The comment aboveTestNormalizeRejectsGarbagedescribes aTestNormalizeRejectsBombDimensionstest that the function body does not implement (the body tests garbage/truncated input). There is no test anywhere for themaxDecodePixelspath. Fix: rewrite the comment to describe what the function actually tests, and add the missing dimension-bomb test.internal/imagenorm/testdata/— The binary fixtures (sample.heic,sample.webp) have no in-repo provenance. A future maintainer cannot recreate or audit them. Fix: addinternal/imagenorm/testdata/README.mddocumenting each fixture's source and how to regenerate it.⚡ Performance — No material issues found
No material issues found
From the Performance lens, the package is well-bounded: input is capped at 25 MiB, decoded pixels are capped at 100 MP before any bitmap is allocated, and downscaling targets a modest 2048 px ceiling. Memory churn is O(max input + max decode + output buffer) with no unbounded growth, no hot loops, and no repeated work per row or pixel. The HEIC path goes through WASM (inherent to the CGO constraint), but that is a library-level trade-off, not a code-level regression introduced here. No N+1 patterns, no unnecessary copies inside loops, and no blocking calls on a path that should be async. The design keeps the fast JPEG re-encode path for in-bounds JPEG inputs by only downscaling when needed.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
internal/imagenorm/imagenorm_test.go:111—image.DecodeConfigerror discarded.In
TestNormalizeDownscalesthe small-image branch doescfg, _, _ = image.DecodeConfig(bytes.NewReader(out)). If the normalized JPEG is somehow corrupt, the test reports a misleading dimension mismatch instead of the real decode failure. Check the error witht.Fatalflike the big-image branch does.internal/imagenorm/imagenorm_test.go:127-141— The dimension-bomb guard is untested.The comment claims this tests “a small file claiming a huge canvas”, but
TestNormalizeRejectsGarbageonly exercises garbage bytes and a truncated PNG. No test verifies that a valid header withWidth*Height > maxDecodePixelsreturnsErrTooLargebeforeimage.Decodeallocates. Because this is the main memory-safety guard, it deserves a fixture or a hand-crafted header (e.g. a PNG IHDR with 50 000×50 000 pixels and a tiny IDAT).Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.