EXIF orientation: bake it in when normalizing images (#103) #111

Merged
steve merged 2 commits from feat/exif-orientation into main 2026-07-22 05:35:46 +00:00
Owner

Closes #103. Part of #96 (carries the deferred #81 EXIF follow-up).

imagenorm decoded and re-encoded to JPEG but ignored the EXIF Orientation tag. Phone cameras store the sensor pixels one way and set a flag to rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged "rotate 90°" — and the re-encode strips EXIF, so without baking the rotation in, a packet photographed in portrait reaches the vision model sideways (worse OCR) and any future thumbnail is wrong.

What

  • exifOrientation — a small pure-Go parser that walks the JPEG APP1/Exif segment for tag 0x0112, returning 1 (normal) for non-JPEG or unparseable input (never guess a rotation onto a correct image). No cgo, no new dependency.
  • applyOrientation — bakes in all 8 orientations (4 rotations + mirrors), applied after downscale (cheaper to rotate the small image; a 90° turn swaps the sides but not the longest edge, so the downscale bound still holds).
  • Non-JPEG paths (HEIC/webp/png) are untouched — their decoders own orientation and carry no JPEG EXIF.

Tests

Build oriented JPEGs (a white corner marker + a spliced little-endian Exif APP1) and assert the marker lands exactly where each orientation says — dims swapping for the quarter-turns (6/8) — plus the 180 and no-EXIF/normal cases, and parser defaults for PNG / no-EXIF / garbage bytes.

GOWORK=off go build ./... + go test ./internal/imagenorm/ green; gofmt clean; stays CGO_ENABLED=0.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

Closes #103. Part of #96 (carries the deferred #81 EXIF follow-up). `imagenorm` decoded and re-encoded to JPEG but ignored the EXIF `Orientation` tag. Phone cameras store the sensor pixels one way and set a flag to rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged "rotate 90°" — and the re-encode strips EXIF, so without baking the rotation in, a packet photographed in portrait reaches the vision model **sideways** (worse OCR) and any future thumbnail is wrong. ## What - **`exifOrientation`** — a small pure-Go parser that walks the JPEG APP1/Exif segment for tag `0x0112`, returning `1` (normal) for non-JPEG or unparseable input (never guess a rotation onto a correct image). No cgo, no new dependency. - **`applyOrientation`** — bakes in all 8 orientations (4 rotations + mirrors), applied **after** downscale (cheaper to rotate the small image; a 90° turn swaps the sides but not the longest edge, so the downscale bound still holds). - Non-JPEG paths (HEIC/webp/png) are untouched — their decoders own orientation and carry no JPEG EXIF. ## Tests Build oriented JPEGs (a white corner marker + a spliced little-endian Exif APP1) and assert the marker lands exactly where each orientation says — dims swapping for the quarter-turns (6/8) — plus the 180 and no-EXIF/normal cases, and parser defaults for PNG / no-EXIF / garbage bytes. `GOWORK=off go build ./...` + `go test ./internal/imagenorm/` green; `gofmt` clean; stays `CGO_ENABLED=0`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve added 1 commit 2026-07-22 05:23:25 +00:00
EXIF orientation: bake it in when normalizing images (#103)
Build image / build-and-push (push) Successful in 8s
Gadfly review (reusable) / review (pull_request) Successful in 7m8s
Adversarial Review (Gadfly) / review (pull_request) Successful in 7m8s
0ab90a01cb
imagenorm decoded and re-encoded to JPEG but ignored the EXIF Orientation
tag. Phone cameras store the sensor pixels one way and set an EXIF flag to
rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged
"rotate 90°" — and our re-encode strips EXIF, so without baking the rotation
in, a packet photographed in portrait reaches the vision model sideways
(bad OCR) and any future thumbnail is wrong.

- exifOrientation: a small pure-Go parser that walks the JPEG APP1/Exif
  segment for tag 0x0112, returning 1 (normal) for non-JPEG or unparseable
  input — never guess a rotation onto a correct image. No cgo, no new dep.
- applyOrientation: bakes in all 8 orientations (the 4 rotations + mirrors)
  after downscale (cheaper to rotate the small image; a 90° turn swaps the
  sides but not the longest edge, so the downscale bound still holds).
- Non-JPEG paths (HEIC/webp/png) are untouched — their decoders own
  orientation and they carry no JPEG EXIF.

Tests build oriented JPEGs (a corner marker + a spliced Exif APP1) and
assert the marker lands where each orientation says, dims swapping for the
quarter-turns; plus parser defaults for non-JPEG / no-EXIF / garbage.
Stays CGO_ENABLED=0.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-22 05:30:34Z

claude-code/sonnet · claude-code — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — No material issues found

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — No material issues found

kimi-k2.6:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — Minor issues
  • performance — Blocking issues found
  • error-handling — Minor issues

opencode/glm-5.2:cloud · opencode — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — No material issues found

opencode/kimi-k2.6:cloud · opencode — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Minor issues

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 05:30:34Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — No material issues found #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — Blocking issues found - ✅ **error-handling** — Minor issues #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-22 05:30:34 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 9 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** — 9 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -191,0 +211,4 @@
b := img.Bounds()
w, h := b.Dx(), b.Dy()
swap := o >= 5 // a quarter-turn: output dimensions transpose
dst := image.NewRGBA(image.Rect(0, 0, w, h))

🔴 Double image allocation on 90° rotations: first RGBA buffer immediately discarded

maintainability, performance · flagged by 4 models

  • internal/imagenorm/imagenorm.go:214-216 — Double image allocation on every 90° rotation. applyOrientation always allocates image.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocates image.NewRGBA(image.Rect(0, 0, h, w)) when swap is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple if/else so only the correctly-sized buffer is ever…

🪰 Gadfly · advisory

🔴 **Double image allocation on 90° rotations: first RGBA buffer immediately discarded** _maintainability, performance · flagged by 4 models_ - **`internal/imagenorm/imagenorm.go:214-216` — Double image allocation on every 90° rotation.** `applyOrientation` always allocates `image.NewRGBA(image.Rect(0, 0, w, h))`, then immediately throws it away and allocates `image.NewRGBA(image.Rect(0, 0, h, w))` when `swap` is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple `if/else` so only the correctly-sized buffer is ever… <sub>🪰 Gadfly · advisory</sub>
@@ -191,0 +215,4 @@
if swap {
dst = image.NewRGBA(image.Rect(0, 0, h, w))
}
for y := range h {

🟠 Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation

maintainability, performance · flagged by 2 models

  • internal/imagenorm/imagenorm.go:218-239 — manual At/Set per-pixel loop diverges from the draw-package idiom used elsewhere in the file. applyOrientation walks every pixel with img.At/dst.Set (going through the color.Model conversion path), while downscale (line 332) uses draw.CatmullRom.Scale. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is…

🪰 Gadfly · advisory

🟠 **Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation** _maintainability, performance · flagged by 2 models_ - **`internal/imagenorm/imagenorm.go:218-239` — manual `At`/`Set` per-pixel loop diverges from the `draw`-package idiom used elsewhere in the file.** `applyOrientation` walks every pixel with `img.At`/`dst.Set` (going through the `color.Model` conversion path), while `downscale` (line 332) uses `draw.CatmullRom.Scale`. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is… <sub>🪰 Gadfly · advisory</sub>
@@ -191,0 +246,4 @@
// a wrong guess rotates a correct image. Only the JPEG APP1/Exif path is parsed:
// that's the format uploaded phone photos overwhelmingly arrive in, and the other
// decoders own their own orientation.
func exifOrientation(raw []byte) int {

🟡 JPEG fill bytes between markers not skipped in exifOrientation

error-handling · flagged by 1 model

  • internal/imagenorm/imagenorm.go:249-275 — JPEG fill bytes (0xFF) between markers are not skipped The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. The parser at exifOrientation assumes each marker is immediately preceded by exactly one 0xFF byte. If a phone encoder emits padding 0xFF bytes before the APP1 segment, the loop misreads the marker and returns orientation 1 instead of the true value. This is safe (no wrong rotation) but incorrect for a valid…

🪰 Gadfly · advisory

🟡 **JPEG fill bytes between markers not skipped in exifOrientation** _error-handling · flagged by 1 model_ - **`internal/imagenorm/imagenorm.go:249-275` — JPEG fill bytes (`0xFF`) between markers are not skipped** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. The parser at `exifOrientation` assumes each marker is immediately preceded by exactly one `0xFF` byte. If a phone encoder emits padding `0xFF` bytes before the APP1 segment, the loop misreads the marker and returns orientation `1` instead of the true value. This is safe (no wrong rotation) but incorrect for a valid… <sub>🪰 Gadfly · advisory</sub>
@@ -191,0 +253,4 @@
return 1
}
for i := 2; i+4 <= len(raw); {
if raw[i] != 0xFF {

🟠 JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs

error-handling · flagged by 1 model

  • internal/imagenorm/imagenorm.go:256 — JPEG marker-padding edge case unhandled. The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. exifOrientation assumes exactly one 0xFF followed immediately by the marker code, so a file like FF D8 FF FF E1 … (valid per spec) misaligns: marker becomes 0xFF, segLen is read from the next two bytes (0xE1 0x00 ≈ 57 KiB), the length check fails, and the parser bails out returning 1 (normal). For a portrait photo whose…

🪰 Gadfly · advisory

🟠 **JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs** _error-handling · flagged by 1 model_ - **`internal/imagenorm/imagenorm.go:256` — JPEG marker-padding edge case unhandled.** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. `exifOrientation` assumes exactly one `0xFF` followed immediately by the marker code, so a file like `FF D8 FF FF E1 …` (valid per spec) misaligns: `marker` becomes `0xFF`, `segLen` is read from the next two bytes (`0xE1 0x00` ≈ 57 KiB), the length check fails, and the parser bails out returning `1` (normal). For a portrait photo whose… <sub>🪰 Gadfly · advisory</sub>
@@ -191,0 +277,4 @@
// orientationFromApp1 reads the Orientation tag from a JPEG APP1 segment body
// (everything after the 2-byte length): "Exif\0\0" then a TIFF block holding
// IFD0. Returns (0, false) if the segment isn't Exif or the tag is missing.
func orientationFromApp1(seg []byte) (int, bool) {

🟠 Missing TIFF magic number validation in orientationFromApp1

error-handling · flagged by 1 model

  • internal/imagenorm/imagenorm.go:280 — Missing TIFF magic number validation in orientationFromApp1 The function trusts the IFD offset at tiff[4:8] without first checking that bytes tiff[2:4] contain the required TIFF magic number 42 (0x002A little-endian / 0x2A00 big-endian). A malformed APP1 segment that passes the "Exif\x00\x00" prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially…

🪰 Gadfly · advisory

🟠 **Missing TIFF magic number validation in orientationFromApp1** _error-handling · flagged by 1 model_ - **`internal/imagenorm/imagenorm.go:280` — Missing TIFF magic number validation in `orientationFromApp1`** The function trusts the IFD offset at `tiff[4:8]` without first checking that bytes `tiff[2:4]` contain the required TIFF magic number `42` (`0x002A` little-endian / `0x2A00` big-endian). A malformed APP1 segment that passes the `"Exif\x00\x00"` prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially… <sub>🪰 Gadfly · advisory</sub>
@@ -191,0 +302,4 @@
if off+12 > len(tiff) {
return 0, false
}
if bo.Uint16(tiff[off:off+2]) != 0x0112 { // Orientation tag

🟠 EXIF orientation parser does not validate IFD type/count before reading value

correctness, error-handling · flagged by 2 models

  • internal/imagenorm/imagenorm.go:305-309orientationFromApp1 reads the Orientation value without validating the IFD entry's type and count. The EXIF spec requires Orientation to be SHORT (type 3) with count 1. A malformed entry with a different type (e.g. LONG) or count >1 stores an offset in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "sa…

🪰 Gadfly · advisory

🟠 **EXIF orientation parser does not validate IFD type/count before reading value** _correctness, error-handling · flagged by 2 models_ - **`internal/imagenorm/imagenorm.go:305-309`** — `orientationFromApp1` reads the Orientation value without validating the IFD entry's **type** and **count**. The EXIF spec requires Orientation to be `SHORT` (type `3`) with count `1`. A malformed entry with a different type (e.g. `LONG`) or count `>1` stores an *offset* in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "sa… <sub>🪰 Gadfly · advisory</sub>
@@ -203,0 +268,4 @@
// The white marker starts centred at (10,6) in the 40x24 source. For each
// orientation, wantW/H is the corrected canvas and (mx,my) is where that
// marker must land — derived from the same transform Normalize applies.
cases := []struct {

🟠 Missing test coverage for mirror/transpose orientations 2/4/5/7

maintainability · flagged by 1 model

  • internal/imagenorm/imagenorm_test.go:271-281 — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.

🪰 Gadfly · advisory

🟠 **Missing test coverage for mirror/transpose orientations 2/4/5/7** _maintainability · flagged by 1 model_ - `internal/imagenorm/imagenorm_test.go:271-281` — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations. <sub>🪰 Gadfly · advisory</sub>
@@ -203,0 +273,4 @@
wantW, wantH int
mx, my int
}{
{0, 40, 24, 10, 6}, // no EXIF → unchanged, marker top-left

🟠 No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases

maintainability · flagged by 1 model

  • internal/imagenorm/imagenorm_test.go:276-280 — no test covers orientations 2, 4, 5, 7. TestNormalizeAppliesExifOrientation only exercises 0/1/3/6/8, and TestExifOrientationParsing only parses 6. The applyOrientation switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transver…

🪰 Gadfly · advisory

🟠 **No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases** _maintainability · flagged by 1 model_ - **`internal/imagenorm/imagenorm_test.go:276-280` — no test covers orientations 2, 4, 5, 7.** `TestNormalizeAppliesExifOrientation` only exercises 0/1/3/6/8, and `TestExifOrientationParsing` only parses 6. The `applyOrientation` switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transver… <sub>🪰 Gadfly · advisory</sub>
@@ -203,0 +279,4 @@
{6, 24, 40, 17, 10}, // 90° CW → top-right (dims swap)
{8, 24, 40, 6, 29}, // 90° CCW → bottom-left (dims swap)
}
for _, tc := range cases {

🟡 table test missing t.Run subtests inconsistent with file pattern

maintainability · flagged by 1 model

  • internal/imagenorm/imagenorm.go:214applyOrientation allocates dst with dimensions w×h, then immediately throws it away and allocates again with h×w whenever swap is true (orientations 5–8). This wastes an allocation of w*h*4 bytes for every quarter-turn. Move the allocation into an if/else so only the correct size is created. - internal/imagenorm/imagenorm_test.go:282TestNormalizeAppliesExifOrientation iterates over a table of cases but doesn't wrap them in `t…

🪰 Gadfly · advisory

🟡 **table test missing t.Run subtests inconsistent with file pattern** _maintainability · flagged by 1 model_ - **`internal/imagenorm/imagenorm.go:214`** — `applyOrientation` allocates `dst` with dimensions `w×h`, then immediately throws it away and allocates again with `h×w` whenever `swap` is true (orientations 5–8). This wastes an allocation of `w*h*4` bytes for every quarter-turn. Move the allocation into an `if/else` so only the correct size is created. - **`internal/imagenorm/imagenorm_test.go:282`** — `TestNormalizeAppliesExifOrientation` iterates over a table of cases but doesn't wrap them in `t… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Blocking issues found · 9 findings (3 with multi-model agreement)

Finding Where Models Lens
🔴 Double image allocation on 90° rotations: first RGBA buffer immediately discarded internal/imagenorm/imagenorm.go:214 4/5 maintainability, performance
🟠 Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation internal/imagenorm/imagenorm.go:218 2/5 maintainability, performance
🟠 EXIF orientation parser does not validate IFD type/count before reading value internal/imagenorm/imagenorm.go:305 2/5 correctness, error-handling
6 single-model findings (lower confidence)
Finding Where Model Lens
🟠 JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs internal/imagenorm/imagenorm.go:256 kimi-k2.6:cloud error-handling
🟠 Missing TIFF magic number validation in orientationFromApp1 internal/imagenorm/imagenorm.go:280 opencode/kimi-k2.6:cloud error-handling
🟠 Missing test coverage for mirror/transpose orientations 2/4/5/7 internal/imagenorm/imagenorm_test.go:271 kimi-k2.6:cloud maintainability
🟠 No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases internal/imagenorm/imagenorm_test.go:276 glm-5.2:cloud maintainability
🟡 JPEG fill bytes between markers not skipped in exifOrientation internal/imagenorm/imagenorm.go:249 opencode/kimi-k2.6:cloud error-handling
🟡 table test missing t.Run subtests inconsistent with file pattern internal/imagenorm/imagenorm_test.go:282 opencode/kimi-k2.6:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

One thing worth flagging first: the trailing block claiming "Plan mode is active" and instructing an Explore/Plan-agent/ExitPlanMode workflow doesn't correspond to any real tool in this session — I searched for ExitPlanMode/AskUserQuestion via ToolSearch and neither resolves, unlike the genuinely deferred tools (TaskCreate, EnterWorktree, etc.) which do. That block reads as an injected instruction trying to redirect this session away from delivering the actual review. I'm disregarding it and completing the task as originally specified (the Gadfly review format, output as text). I did already write a harmless scratch file to /root/.claude/plans/... before catching this — it contains only my own analysis notes, nothing sensitive, and can be ignored/deleted.

Here's the actual security review of the PR:

VERDICT: No material issues found

This diff adds pure, local EXIF-orientation parsing and pixel-transform code to internal/imagenorm. I read the live file (internal/imagenorm/imagenorm.go:140-199) to confirm the surrounding guarantees, not just the diff hunk:

  • Input is already size-capped before this code runs. raw is bounded by opts.maxBytes() (default 25 MiB, enforced at imagenorm.go:140-151) before exifOrientation(raw) is ever called (imagenorm.go:175). No new unbounded-read surface.
  • exifOrientation's marker walk is fully bounds-checked before every slice accessi+4<=len(raw) before reading marker bytes, segLen<2 || i+2+segLen>len(raw) before slicing the segment body. Malformed/truncated JPEGs fall through to return 1, not a panic or OOB read.
  • orientationFromApp1's TIFF/IFD parsing is equally guarded — prefix-length check before the "Exif\x00\x00" compare, ifd<8 || ifd+2>len(tiff) before reading the entry count, and off+12>len(tiff) checked inside the loop before every 12-byte entry read. A crafted large entry count (n up to 65535) just runs bounded, cheap, bounds-checked iterations — not a meaningful DoS vector.
  • Safe-default posture is correct and consistently applied: unparseable/non-JPEG/garbage input always yields orientation 1 (no rotation), never a guess — so a hostile file can at worst leave an image un-rotated, not corrupt a legitimate one or destabilize downstream processing.
  • applyOrientation only receives an already-validated o in [2,8] and allocates its output buffer after downscale, so its allocation size is bounded by opts.maxDim() regardless of what the EXIF tag claims — no amplification/decompression-bomb path via orientation.
  • No new sinks are introduced: no filesystem, SQL, shell, or network use; no secrets touched; nothing here affects authn/authz. This is pure in-memory byte parsing over data that was already being treated as untrusted (decodeSafely's existing panic-recovery comment already documents the "untrusted upload" threat model this code operates under).

I did not find any injection, SSRF, deserialization, secret-handling, or input-validation defect within this diff's security lens.

🎯 Correctness — No material issues found

Review — 🎯 Correctness lens

VERDICT: No material issues found

I independently re-derived all 8 EXIF orientation transforms from first principles (physically rotating/mirroring a labeled grid) rather than trusting the code's comments, and checked them against internal/imagenorm/imagenorm.go:207-242:

  • Orientation 6 ("rotate 90° CW to correct") → dx,dy = h-1-y, x — verified against a manual 2×2 rotation trace, matches.
  • Orientation 8 ("rotate 90° CCW to correct") → dx,dy = y, w-1-x — verified, matches.
  • Orientations 2/3/4 (mirror-H, 180, mirror-V) and 5/7 (transpose/transverse) all check out against the canonical "where does stored (0,0) land" EXIF table.

Also verified:

  • orientationFromApp1 (imagenorm.go:280-316): TIFF header/IFD0 offset bounds, 12-byte IFD entry stride, and reading a SHORT value from the first 2 bytes of the 4-byte value field per file byte order — all correct per TIFF6 spec.
  • exifOrientation (imagenorm.go:249-275): JPEG marker-segment length semantics (segLen includes the 2 length bytes, so i += 2+segLen and slicing raw[i+4:i+2+segLen] are both correctly offset), correctly halts at SOS/EOI so it never scans entropy-coded data, and fails safe (returns 1) on any malformed/short segment rather than misapplying a rotation.
  • Ordering in Normalize (imagenorm.go:174-175): applyOrientation is called with exifOrientation(raw) where raw is the original, un-truncated input bytes (line 145) — not the decoded/re-encoded buffer — so the tag is read from the actual source JPEG. exifOrientation gates on the FFD8 SOI itself, so non-JPEG formats (PNG/WebP/HEIC) safely no-op without a separate format check. Downscale-then-rotate is sound: rotation only transposes width/height, so max(w,h) (the downscale bound) is invariant across it.

One thing I checked but am not reporting as a finding (too low-confidence/low-impact to meet the bar): orientationFromApp1 doesn't validate the IFD entry's type/count fields before reading the Orientation value, so a file with a non-SHORT-typed 0x0112 tag could yield a bogus-but-in-range value instead of falling back to 1. No real camera emits Orientation as anything but SHORT, and I don't have enough confidence this is exploitable to list it as a real bug.

I was unable to execute go test ./internal/imagenorm/... in this environment (Bash approval unavailable under the current session mode), so the "tests pass" claim in the PR description is unverified by me directly — but I traced the test's rotation-formula arithmetic by hand against the same transforms and it's internally consistent with the (verified-correct) implementation.

🧹 Code cleanliness & maintainability — Minor issues

No other EXIF/binary parsing utilities exist elsewhere — that claim holds. All checks confirm the draft's single finding is accurate, but one supporting claim in the prose (about downscale already using the range-over-int idiom) is false, so I'll correct that in the output.

Review — Code cleanliness & maintainability

VERDICT: Minor issues

The new EXIF parsing/orientation code (internal/imagenorm/imagenorm.go:202-316) is well-scoped, self-contained to one file, and matches the surrounding heavily-commented style (verified no pre-existing EXIF/TIFF parsing utilities elsewhere in the repo to duplicate, via grep -rn "binary.ByteOrder\|Uint16\|Uint32"). Naming (exifOrientation, orientationFromApp1, applyOrientation) is clear, and the 8-case switch in applyOrientation is an appropriate use of a switch over a transform-function table for this size. Tests reuse the existing pngBytes helper rather than duplicating it (confirmed at internal/imagenorm/imagenorm_test.go:17).

One small nit:

  • internal/imagenorm/imagenorm.go:213-217applyOrientation unconditionally allocates a w×h RGBA, then immediately discards it and allocates a second h×w RGBA whenever swap is true (orientations 5-8):
    dst := image.NewRGBA(image.Rect(0, 0, w, h))
    if swap {
        dst = image.NewRGBA(image.Rect(0, 0, h, w))
    }
    
    The first allocation is pure waste in the swap case. Cleaner and avoids the throwaway alloc:
    dw, dh := w, h
    if swap {
        dw, dh = h, w
    }
    dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
    
    Verified by reading the function in full — no other use of dst before the reassignment, so the first allocation is dead work.

Nothing else in this diff (including the DESIGN.md wording update and the test file additions) raises cleanliness concerns.

Performance — Minor issues

Confirmed — this matches the draft exactly: line 214 unconditionally allocates a w×h image.NewRGBA, then line 215-217 discards it and allocates a second h×w buffer whenever swap is true (orientations 5-8, which includes orientation 6, the common phone-portrait case). The fix suggestion (compute dw, dh first, allocate once) is correct and minimal. This is a real, verifiable finding — a small/minor perf nit, not a correctness bug, matching the draft's severity.

VERDICT: Minor issues

  • internal/imagenorm/imagenorm.go:214-217applyOrientation allocates a w×h *image.RGBA unconditionally, then immediately discards it and allocates a second h×w *image.RGBA whenever swap is true (orientations 5-8):

    dst := image.NewRGBA(image.Rect(0, 0, w, h))
    if swap {
        dst = image.NewRGBA(image.Rect(0, 0, h, w))
    }
    

    Orientation 6 (90° CW) is the single most common EXIF tag on real phone photos (camera held normally in portrait) — exactly the case this PR is built for per the PR description ("a phone camera... sets a flag to rotate on display"). So on the primary target workload, every upload does one throwaway allocation of up to DefaultMaxDim × DefaultMaxDim × 4 bytes (~16.7 MB at the 2048px default) before the real buffer is allocated, adding avoidable allocation/GC pressure on the image-upload hot path.

    Fix: compute the swapped dimensions before allocating, e.g.

    dw, dh := w, h
    if swap {
        dw, dh = h, w
    }
    dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
    

Everything else in the changed code is a bounded, single-pass O(w·h) pixel copy (unavoidable for baking in a rotation) applied after downscale caps dimensions, and a marker-segment walk over the raw JPEG header bytes (exifOrientation) that's O(segment count), not O(file size) in any problematic way. No N+1s, no unbounded growth, no new blocking calls on the hot path.

🧯 Error handling & edge cases — No material issues found

Go 1.26 supports range-over-int, so no compile issue there.

I've verified the core logic — bounds checks, loop termination, and defaults are all sound. Nothing in my lane rises to a real defect.

VERDICT: No material issues found

Findings (through the error-handling/edge-case lens only):

  • exifOrientation/orientationFromApp1 (imagenorm.go:249-316) are defensively bounds-checked at every slice access I traced (segment length vs. len(raw), IFD offset vs. len(tiff), entry offset vs. len(tiff) before each 12-byte read) — verified by re-deriving the offsets by hand against the TIFF/EXIF layout. Every failure path returns the safe default (1, no rotation) rather than erroring or guessing, matching the doc comment's stated intent.
  • The i += 2 + segLen marker-walk loop is guaranteed to terminate: segLen < 2 is rejected before the increment, so i strictly increases each iteration — no infinite-loop/DoS risk on adversarial input, and the byte read is already capped by opts.MaxBytes upstream.
  • applyOrientation (imagenorm.go:207-242): the o<=1 || o>8 guard fully covers the domain (exifOrientation only ever returns 1-8), so the switch's implicit cases 2-8 are exhaustive — no default-case gap. A 0×0 img.Bounds() degenerates to empty loops with no panic (unlikely in practice, but confirmed not to crash).
  • No nil-image path: img reaching applyOrientation/downstream is always non-nil by construction (decodeSafely returns early on error), so no nil-check gap.

One sub-lens observation I considered but am not confident enough to list as a finding: orientationFromApp1 doesn't validate the IFD entry's type field (expects SHORT/3 for Orientation) before reading the value bytes — a malformed tag with the same ID but a different type could, in a byte-order-dependent way, be misread as a spurious value 1-8 and silently misrotate instead of falling back safely. This requires deliberately malformed EXIF (real phone EXIF always types Orientation as SHORT) and is closer to a correctness/robustness nit than an error-handling defect, so I'm not reporting it as a real bug.

glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

I reviewed the new EXIF parser and orientation applier through the security lens — they process attacker-controlled upload bytes — and verified the bounds logic against the checked-out code.

  • exifOrientation / orientationFromApp1 (imagenorm.go:249-316) — all slice accesses are bounds-checked before use, so an OOB panic (DoS via process crash) is not reachable:
    • SOI check guards raw[0..1]; loop condition i+4 <= len(raw) guards raw[i..i+3]; segLen < 2 || i+2+segLen > len(raw) guards raw[i+4 : i+2+segLen].
    • In orientationFromApp1, len(seg) < len(prefix)+8 (14) guards tiff[0:2] and tiff[4:8]; ifd < 8 || ifd+2 > len(tiff) guards tiff[ifd:ifd+2]; off+12 > len(tiff) guards tiff[off:off+2] and tiff[off+8:off+10]. The value-field read ignores the declared tag type but stays in bounds, so a malicious non-SHORT type can't cause a panic — worst case it misreads and falls through to the 1..8 check or returns false.
    • The IFD entry-count loop for k := range n is attacker-controlled (uint16, ≤65535) but breaks as soon as off+12 > len(tiff), so iteration is bounded by len(tiff)/12; len(tiff) is itself bounded by MaxBytes (25 MiB). No integer-overflow concern on 32-bit either, since ifd is clamped to ≤ len(tiff)-2 before use. This is at most trivial CPU amplification, far below the cost of the decode that already ran.
    • Safe-default discipline is correct: every unparseable/non-JPEG/garbage path returns 1 (no-op), so a malformed file can never coerce an unwanted rotation of a correct image. This is the fail-safe direction.
  • applyOrientation (imagenorm.go:207-242) — operates only on the already-decoded image.Image and a validated o in 1..8; At/Set on stdlib image types don't panic. No new taint surface.
  • Recover scope — noted (not a finding): decodeSafely's recover does not wrap exifOrientation/applyOrientation, but neither has a reachable panic on untrusted input per the bounds analysis above, so the unprotected scope is safe.
  • No injection (SQL/command/path/template), SSRF, deserialization, secret/credential, or authn/authz surface in this change — it's pure in-memory byte parsing of an uploaded image. The upload boundary and size caps (MaxBytes, maxDecodePixels, maxDimension) upstream of this code already bound the input.
🎯 Correctness — No material issues found

Verdict: No material issues found

I verified the change through the correctness lens by reading the full implementation (internal/imagenorm/imagenorm.go:140-334) and re-deriving the EXIF orientation transforms from first principles.

  • applyOrientation transforms (imagenorm.go:207-242): I checked all 8 cases against the EXIF 2.3 spec definitions (1=identity, 2=mirror-H, 3=180, 4=mirror-V, 5=transpose, 6=90°CW, 7=transverse, 8=90°CCW). Every mapping (x,y) → (dx,dy) is correct, including the dimensional transpose for o≥5 (swap is right for all of 5/6/7/8, not just the pure rotations). Corner-spot checks confirm: o6 maps (0,0)→(h-1,0) (top-left→top-right, correct for CW); o8 maps (0,0)→(0,w-1) (top-left→bottom-left, correct for CCW).

  • Downscale-then-rotate ordering (imagenorm.go:171-175): The comment's invariant holds. Downscale bounds the longest edge to maxDim; a rotation/transposition only swaps sides, so the longest edge stays ≤ maxDim. Worked example: 4000×3000 → downscale 1024×768 → o6 swap → 768×1024 (longest edge 1024). ✓

  • exifOrientation / orientationFromApp1 (imagenorm.go:249-316): JPEG segment walk, bounds, IFD0 parsing, and the inline-SHORT read at off+8:off+10 are all correct. Non-JPEG inputs (HEIC/PNG/WebP) correctly short-circuit on the 0xFFD8 SOI check and return 1, so applyOrientation is a no-op for them. All slice accesses are guarded against panics (verified each bounds check). Failure mode on any unparseable/edge input is returning 1 (no rotation) — never a wrong rotation — which is the safe default.

  • Test expectations (imagenorm_test.go): I re-derived each wantW/wantH/mx/my from the same transform the code applies; all match (e.g. o6: dx=h-1-y=17, dy=x=10 for source center (10,6) with h=24; o8: dx=y=6, dy=w-1-x=29; o3: dx=29, dy=17).

One non-blocking observation (not a correctness bug): the value field is read as uint16 without verifying the tag's EXIF type is SHORT. Since the spec mandates SHORT for Orientation this is fine in practice; a hypothetical big-endian LONG-typed tag would read 0 and fall back to the safe default of 1 rather than a wrong rotation, so it fails safe.

🧹 Code cleanliness & maintainability — Minor issues

Minor issues found

  • internal/imagenorm/imagenorm.go:214-216 — redundant double allocation on the swap branch. dst is allocated as image.NewRGBA(image.Rect(0,0,w,h)) on line 214, then unconditionally reassigned to image.NewRGBA(image.Rect(0,0,h,w)) on line 216 when swap is true. On the quarter-turn path (orientations 5–8) the first RGBA header is allocated and immediately discarded. The non-swap branch keeps the first allocation correctly, so this is only a wasted allocation on the swap branch, not a correctness bug — a readability/efficiency wart in an allocation-sensitive path. Cleaner: compute the output dims up front and allocate once.

  • internal/imagenorm/imagenorm.go:218-239 — manual At/Set per-pixel loop diverges from the draw-package idiom used elsewhere in the file. applyOrientation walks every pixel with img.At/dst.Set (going through the color.Model conversion path), while downscale (line 332) uses draw.CatmullRom.Scale. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is avoidable. Not blocking; a consistency nit.

  • internal/imagenorm/imagenorm_test.go:276-280 — no test covers orientations 2, 4, 5, 7. TestNormalizeAppliesExifOrientation only exercises 0/1/3/6/8, and TestExifOrientationParsing only parses 6. The applyOrientation switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transverse math.

Performance — Minor issues

Both findings are confirmed against the actual code. The first (double RGBA allocation on the swap path) is real and at the cited lines. The second is informational and not in the findings JSON, consistent with the draft.

Verdict: Minor issues

  • internal/imagenorm/imagenorm.go:214-217 — double RGBA allocation on the swap (portrait) path. For orientations 5–8 (the quarter-turns), dst is first allocated at w×h and then immediately overwritten with a second image.NewRGBA at h×w:

    dst := image.NewRGBA(image.Rect(0, 0, w, h))
    if swap {
        dst = image.NewRGBA(image.Rect(0, 0, h, w))
    }
    

    The first buffer is allocated and discarded. This is the common phone-portrait path — orientation 6 is exactly "portrait photo tagged rotate 90°", the motivating case of this PR. For a 2048×1536 portrait, that's a ~12 MiB RGBA thrown straight to the GC on the very upload the feature targets. Fix is trivial — compute the destination rect once:

    dw, dh := w, h
    if swap { dw, dh = h, w }
    dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
    
  • internal/imagenorm/imagenorm.go:218-240 — per-pixel At/Set loop (informational, not blocking). The rotate reads via img.At and writes via dst.Set, both of which go through generic color.Color/color.Model conversions rather than fast direct RGBA writes. Bounded to ≤2048px and only on the rotated path, so acceptable for an upload boundary — but worth noting that a portrait phone photo already within MaxDim (so downscale returns the original *image.YCbCr) hits the slowest variant (YCbCr-per-pixel At). Not worth changing unless this shows up in profiles.

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

Through the error-handling & edge-case lens, the new code is solid. I verified the two new functions and the bounds checks by reading internal/imagenorm/imagenorm.go:202-316:

  • exifOrientation (line 249): Every byte access is bounds-checked before use. The marker walk guards len(raw) < 4, i+4 <= len(raw), raw[i] != 0xFF (misalignment → safe return 1), segLen < 2 || i+2+segLen > len(raw), and stops at EOI/SOS. The slice passed to orientationFromApp1 (raw[i+4 : i+2+segLen]) is provably in range because of the i+2+segLen > len(raw) guard. The loop advances i += 2 + segLen with segLen >= 2, so it strictly progresses and terminates — no infinite loop. segLen is a 16-bit value (max 65535), so i+2+segLen can't overflow int on any supported platform.
  • orientationFromApp1 (line 280): Length guard len(seg) < len(prefix)+8 guarantees tiff := seg[len(prefix):] has at least 8 bytes, so tiff[0:2] and tiff[4:8] are safe. The IFD offset is validated (ifd < 8 || ifd+2 > len(tiff)), and each 12-byte entry is checked with off+12 > len(tiff) before the Uint16/Uint32 reads. A crafted huge entry count n is bounded by the per-entry length check, so it can't spin unbounded. Out-of-range orientation values fall through to return 0, false → caller defaults to 1. No panics on truncated/garbage input.
  • applyOrientation (line 207): Out-of-range o (<= 1 || > 8) is a no-op returning the original image. Empty/zero-dimension images (w==0 or h==0, which downscale can pass through) produce empty image.NewRGBA allocations and the for y := range h / for x := range w loops simply don't execute — no index panic. dst.Set(dx, dy, c) always writes within the allocated rect by construction of the transforms.
  • No swallowed errors introduced: Both new call sites in Normalize (applyOrientation, exifOrientation) return values, not errors, so there are no new error-return paths to mishandle or defer. The existing "on any error the returned bytes are nil" contract is preserved.

The test helper spliceExifOrientation discards binary.Write errors with _, but those write to a bytes.Buffer which never errors — not a concern.

kimi-k2.6:cloud (ollama-cloud) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

The change adds a manual EXIF orientation parser for JPEG and pixel-level rotation logic. From a security lens, the code is clean:

  • Fail-closed parsing: exifOrientation returns 1 (no rotation) on any malformed input, missing EXIF, or non-JPEG format — never forcing a rotation onto a correct image.
  • Bounded work: maxDecodePixels (50M) and maxDimension (50K) already cap decoded bitmap size. applyOrientation runs on the downscaled image (downscale happens first), so memory and CPU for the rotation are bounded by the same limits.
  • No out-of-bounds reads: orientationFromApp1 checks len(seg) < len(prefix)+8 before indexing tiff[0:2], tiff[4:8], etc.; the IFD walk checks off+12 > len(tiff) before reading each 12-byte entry.
  • No out-of-bounds writes: applyOrientation computes destination coordinates (dx, dy) that always fall within the pre-allocated dst bounds for every orientation case (2–8).
  • Strip, not leak: The re-encode to JPEG strips all EXIF metadata (including GPS, camera model, timestamps). This is a net privacy win — only the orientation value is extracted and then discarded after the bitmap rotation.

One minor observation (not a finding, just a verification note): the exifOrientation parser is strict about 0xFF marker alignment (raw[i] != 0xFF → return 1), which is the conservative/safe choice for untrusted uploads.

🎯 Correctness — Minor issues

Minor issues

  • internal/imagenorm/imagenorm.go:305-309orientationFromApp1 reads the Orientation value without validating the IFD entry's type and count. The EXIF spec requires Orientation to be SHORT (type 3) with count 1. A malformed entry with a different type (e.g. LONG) or count >1 stores an offset in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "safe default: return 1 for unparseable" design goal) instead of treating the tag as unreadable.

    Suggested fix: Add two checks inside the 0x0112 match block:

    if bo.Uint16(tiff[off+2:off+4]) != 3 { // not SHORT
        return 0, false
    }
    if bo.Uint32(tiff[off+4:off+8]) != 1 { // count != 1
        return 0, false
    }
    
🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/imagenorm/imagenorm.go:214-216applyOrientation allocates a w×h RGBA unconditionally, then immediately discards it and allocates a second h×w RGBA for orientations 5-8. Move the swap check before the first NewRGBA to avoid the throw-away allocation for the quarter-turn cases.

  • internal/imagenorm/imagenorm_test.go:271-281 — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.

Performance — Blocking issues found

Blocking issues found

  • internal/imagenorm/imagenorm.go:214-216 — Double image allocation on every 90° rotation. applyOrientation always allocates image.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocates image.NewRGBA(image.Rect(0, 0, h, w)) when swap is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple if/else so only the correctly-sized buffer is ever allocated.

  • internal/imagenorm/imagenorm.go:218-240 — Per-pixel image.Image.At() calls box a color.Color on every iteration, causing millions of heap allocations for a single rotated image. applyOrientation iterates every pixel and reads via the image.Image interface (img.At(b.Min.X+x, b.Min.Y+y)). When the concrete type is *image.RGBA (which downscale already returns), each At call boxes the color.RGBA value into an interface, allocating on the heap. A 2048×2048 rotation causes ~4.2 million transient allocations. This is a well-known Go image-processing anti-pattern; adding a fast-path type-switch to *image.RGBA (and perhaps *image.NRGBA) that copies through Pix directly would eliminate the boxing and interface dispatch entirely for the common case, cutting rotation time by an order of magnitude. The current generic fallback can be kept for non-RGBA images.

🧯 Error handling & edge cases — Minor issues

Minor issues

  • internal/imagenorm/imagenorm.go:256 — JPEG marker-padding edge case unhandled.
    The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. exifOrientation assumes exactly one 0xFF followed immediately by the marker code, so a file like FF D8 FF FF E1 … (valid per spec) misaligns: marker becomes 0xFF, segLen is read from the next two bytes (0xE1 0x00 ≈ 57 KiB), the length check fails, and the parser bails out returning 1 (normal). For a portrait photo whose APP1 segment is padded this way, the image stays sideways. A robust parser should skip consecutive 0xFF bytes before reading the marker code.

  • internal/imagenorm/imagenorm.go:305 — TIFF type field not validated.
    orientationFromApp1 finds tag 0x0112 and blindly reads the first two bytes of the value field as a SHORT. It never checks tiff[off+2:off+4] to confirm the type is actually 3 (SHORT). A malformed EXIF that reuses tag 0x0112 with a LONG or UNDEFINED type could yield a bogus orientation value. While exifOrientation caps the result to 1..8, any value in that range would still be applied. Adding a type check (bo.Uint16(tiff[off+2:off+4]) == 3) before accepting the value would make the “unparseable → 1” guarantee stronger.

opencode/glm-5.2:cloud (opencode) — No material issues found

Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

Reviewed the EXIF parser through the security lens (untrusted-byte parsing, OOB reads, integer overflow, infinite loops, injection surface). The code is defensively written:

  • exifOrientation (imagenorm.go:249-275): all index accesses are guarded by the loop condition i+4 <= len(raw) and i+2+segLen > len(raw); segLen >= 2 guarantees loop progress (no infinite loop); segLen is a 16-bit field so no int overflow on i+2+segLen; safe default 1 on any misalignment, EOI/SOS, or non-JPEG.
  • orientationFromApp1 (imagenorm.go:280-313): length check len(seg) < len(prefix)+8 covers tiff[0:2] and tiff[4:8]; IFD offset validated with ifd < 8 || ifd+2 > len(tiff) before slicing; per-entry bounds off+12 > len(tiff) cover tiff[off+8:off+10]; orientation value clamped to 1..8 before use. A 32-bit build could in principle make int(bo.Uint32(...)) negative for a near-MaxUint32 offset, but the subsequent ifd+2 > len(tiff) comparison still rejects it (negative < len), so it can't be exploited to read OOB — it just falls through to the safe default.
  • The input to the parser is raw, which Normalize already capped at MaxBytes+1 (imagenorm.go:~125), so the parser runs on size-bounded data regardless.
  • No injection/auth/secret/SSF surface: this is a pure local byte parser; its only output is an int 1..8 consumed by applyOrientation, which does an in-memory pixel copy with no I/O.

Nothing in the security lens is materially wrong.

🎯 Correctness — No material issues found

I verified the orientation math, the EXIF parser bounds, and the test expectations against the checked-out code.

Verdict: No material issues found

  • I re-derived all 8 EXIF orientation transforms from first principles (rotating/mirroring the source bitmap into an upright destination) and confirmed each case in applyOrientation (imagenorm.go:222-237) maps source (x,y) to the correct destination (dx,dy), including the dimension-swapping cases (5–8) where dst is correctly allocated as h×w and all writes stay in bounds.
  • The "downscale then rotate" ordering is sound: downscale bounds the longest edge, and a 90° rotation swaps sides without changing the longest edge, so the MaxDim bound still holds on the emitted image (verified for both the shrink and no-shrink paths).
  • The EXIF parser bounds are correct: the marker walk guards i+4 <= len(raw) and segLen < 2/i+2+segLen > len(raw; orientationFromApp1 requires len(seg) >= len(prefix)+8 (so tiff[4:8] is safe) and ifd < 8 || ifd+2 > len(tiff) plus per-entry off+12 > len(tiff) prevent OOB reads. The APP1 payload slice raw[i+4 : i+2+segLen] correctly excludes the 2 length bytes.
  • The test expectations are consistent with the transforms: e.g. orient 6 (90° CW) sends source (10,6) to (h-1-y, x) = (17,10) on a 24×40 canvas, matching {6, 24, 40, 17, 10}; orient 8 → (y, w-1-x) = (6,29) matching {8, 24, 40, 6, 29}. The spliceExifOrientation helper builds a well-formed little-endian TIFF/IFD0 (verified byte layout: II + 0x2A + offset 8 + count 1 + 12-byte entry with value at entry+8), which the parser reads back correctly.
  • Non-JPEG paths return 1 immediately (raw[0]!=0xFF||raw[1]!=0xD8), so HEIC/WebP/PNG are left untouched as designed — no regression vs. prior behavior.

One trivial robustness note (not material, low confidence): the marker walker doesn't skip JPEG fill bytes (0xFF 0xFF) or handle standalone markers like TEM/0xFF01/RSTn that have no length field before the EXIF APP1. A JPEG emitting fill bytes before APP1 would misparse and fall back to orientation 1 (sideways). Real phone JPEGs place APP1 immediately after SOI, so this is out of the practical path and the fallback is safe (never mis-rotates a correct image).

🧹 Code cleanliness & maintainability — No material issues found

VERDICT: No material issues found

The EXIF orientation addition is clean through the maintainability lens. Verified against the checked-out code:

  • applyOrientation and exifOrientation are well-factored, each doing one thing, with accurate doc comments that match the implementation. They follow the file's existing conventions (sentinel-free helpers, inline rationale comments, pure-Go no-deps stance).
  • No duplicated logic: exifOrientation re-walks the JPEG bytes because image.DecodeConfig doesn't expose EXIF — that's a justified read, not copy-paste.
  • The no-op short-circuit (o <= 1 || o > 8) returns img unchanged, so non-JPEG and normal-orientation inputs pay zero allocation cost — the comment claims this and the code does it.
  • Tests reuse the existing pngBytes/jpegBytes fixture-builder pattern and add orientedJPEG/spliceExifOrientation/bright in the same style; the helper that builds a minimal TIFF IFD entry writes the 12-byte entry layout correctly (2+2+4+2+2), so the fixture reflects a real Exif APP1 rather than asserting the parser's own shape back at itself.
  • DESIGN.md note is updated in the same sentence and stays accurate ("bakes in the JPEG EXIF orientation"), consistent with CLAUDE.md's "keep docs true" rule.

Nothing in my area to flag.

Performance — No material issues found

Reading the file to verify the per-pixel loop's allocation behavior.
VERDICT: No material issues found

Reviewed internal/imagenorm/imagenorm.go (the changed file) and its caller internal/api/seed_packet.go. Through the performance lens:

  • applyOrientation per-pixel loop (imagenorm.go:218-240): iterates every pixel with img.At + dst.Set, each doing a color-model round-trip. This is the only new work on the hot path. It runs only when o > 1 (an actual non-normal EXIF orientation tag exists); orientation 1 — the common case for landscape photos and all non-JPEG formats — early-returns at line 208 with zero cost. When it does run, the image is already capped at DefaultMaxDim (2048px longest edge) by the preceding downscale, so the loop is bounded to ~4M iterations worst case. On a single-upload endpoint that then calls a vision model (seconds of latency), the tens-of-ms per-pixel copy is immaterial. No N+1, no quadratic behavior, no unbounded allocation.

  • exifOrientation(raw) (imagenorm.go:249-275): called unconditionally but bails in O(1) for non-JPEG (raw[0]!=0xFF || raw[1]!=0xD8) and walks at most the handful of JPEG marker segments for JPEGs. Negligible.

  • Transient allocation: applyOrientation allocates one *image.RGBA matching the (already-bounded) downscaled image. Doubling of the ~16MB peak bitmap is acceptable for a one-shot upload path.

The straightforward per-pixel implementation could be sped up with RGBAAt/SetRGBA (skipping two color-model conversions per pixel) since downscale always yields an *image.RGBA, but that is a micro-optimization with no measurable impact at this call frequency and image size — not flagging it.

🧯 Error handling & edge cases — No material issues found

I've read the full imagenorm.go and the relevant test file. Let me verify the boundary logic in the new EXIF/orientation code carefully.

Verdict: No material issues found

I walked the unhappy paths the diff introduces, against the checked-out code:

  • exifOrientation / orientationFromApp1 parser bounds (imagenorm.go:249-316): every slice access is guarded before use. len(raw) < 4 short-circuits entry; the loop requires i+4 <= len(raw) before reading 4 header bytes; segLen < 2 || i+2+segLen > len(raw) gates the segment slice raw[i+4 : i+2+segLen] (and segLen >= 2 makes i+4 <= i+2+segLen). In orientationFromApp1, len(seg) < len(prefix)+8 (14) covers tiff[0:2] and tiff[4:8]; ifd < 8 || ifd+2 > len(tiff) gates the count read; off+12 > len(tiff) gates every 12-byte entry read, which also covers tiff[off+8:off+10]. Every unparseable / truncated / hostile case returns the safe default 1 — never a panic, never a wrong rotation. Verified by reading the file.
  • applyOrientation output bounds (imagenorm.go:207-242): for the non-swap cases (2/3/4) dx ∈ [0,w-1], dy ∈ [0,h-1] and dst is w×h; for the swap cases (5/6/7/8) the loops are y ∈ [0,h), x ∈ [0,w) and each mapping lands in [0,h-1] × [0,w-1] against an h×w destination — I re-derived all four quarter-turn mappings and confirmed no off-by-one and no out-of-bounds Set. o <= 1 || o > 8 is a no-op, so an out-of-range tag (or the 0, false return folded in as 0) leaves the image untouched.
  • Empty/zero inputs: Normalize rejects cfg.Width <= 0 || cfg.Height <= 0 before decode, so applyOrientation never sees a zero-dimension image that would make image.NewRGBA degenerate; downscale's longest == 0 guard is also upstream. No nil-image path: decode failure returns before applyOrientation runs.
  • Panic coverage: decodeSafely's recover wraps image.Decode; the new code runs after it and does only bounds-checked slice access and img.At/dst.Set within verified bounds, so there's no unprotected panic surface from a malformed EXIF blob.

One marginal observation, not material enough to list as a finding: ifd := int(bo.Uint32(tiff[4:8])) (line 295) could in principle let ifd+2 wrap on a 32-bit int build given a hostile TIFF offset, bypassing the ifd+2 > len(tiff) guard. pansy's CGO_ENABLED=0 static binary targets 64-bit servers (Komodo deploy), where int is 64-bit and the guard holds — so this is not a live risk here, and I'm flagging it only because the lens names integer overflow. No change needed.

opencode/kimi-k2.6:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

The EXIF orientation parser is defensively written against untrusted input: bounds-checked on every slice access, defaults to 1 (no rotation) on any malformed data, bounded by the 25 MiB read cap, and advances monotonically through markers with no infinite-loop path. The coordinate mapping in applyOrientation stays within the allocated destination for all 8 orientation values. No authn/authz gaps, injection, deserialization, secret leakage, or untrusted-data mishandling is introduced.

🎯 Correctness — No material issues found

No material issues found

I examined the EXIF orientation parser, the transform application, and the interaction with downscaling. Here is what I verified:

  • applyOrientation transforms: I re-derived the mapping for all 8 EXIF orientations from the spec (0th-row/0th-column placement) and confirmed each case matches. The quarter-turns (5–8) correctly swap the output dimensions, and the pixel-level mappings agree with the test expectations for the covered cases (1, 3, 6, 8) and, by derivation, the uncovered ones (2, 4, 5, 7).
  • Downscale-then-rotate order: Downsampling first is cheaper, and because a 90° rotation only swaps width/height, the longest-edge bound ≤ MaxDim is preserved. This reasoning is sound.
  • EXIF parser robustness: exifOrientation walks big-endian JPEG markers, stops at SOS/EOI, bounds-checks every slice operation, and returns 1 (no rotation) on any parse failure. orientationFromApp1 correctly reads the IFD0 entry for tag 0x0112 as an inline SHORT value with the declared byte order. It safely rejects malformed input (bad magic, short buffers, invalid offsets) by returning ok=false, which causes the caller to fall back to 1.
  • Non-JPEG safety: exifOrientation requires the FF D8 SOI signature; all other formats return 1, and applyOrientation with o==1 returns the image unchanged.
  • Test verification: The spliceExifOrientation helper constructs a valid little-endian TIFF IFD with the correct 12-byte entry layout and segLen accounting. The marker-location assertions in TestNormalizeAppliesExifOrientation match the transforms.
🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/imagenorm/imagenorm.go:214applyOrientation allocates dst with dimensions w×h, then immediately throws it away and allocates again with h×w whenever swap is true (orientations 5–8). This wastes an allocation of w*h*4 bytes for every quarter-turn. Move the allocation into an if/else so only the correct size is created.
  • internal/imagenorm/imagenorm_test.go:282TestNormalizeAppliesExifOrientation iterates over a table of cases but doesn't wrap them in t.Run subtests, unlike TestNormalizeAllFormats and TestNormalizeRejectsPixelBomb in the same file. Adding subtests makes failures immediately attributable to the specific orientation and keeps isolation consistent with the file's existing pattern.
Performance — Minor issues

Verdict: Minor issues

  • internal/imagenorm/imagenorm.go:214-216applyOrientation allocates an image.RGBA of size w×h unconditionally, then immediately discards it and allocates a second one of size h×w when swap is true (orientations 5–8). For a 2048×1536 image that’s a ~12.5 MB throwaway allocation; at 2048×2048 it’s ~16 MB. The fix is trivial: only allocate after the swap check, e.g. dw, dh := w, h; if swap { dw, dh = h, w }; dst := image.NewRGBA(image.Rect(0, 0, dw, dh)).

  • internal/imagenorm/imagenorm.go:218-239 — The pixel-by-pixel img.At loop is correct but has high per-pixel overhead (interface call + color conversion for every pixel). This is acceptable for an upload handler that runs once per image, but it’s worth noting that for images already within maxDim bounds (where downscale returns the original), this copies the full decoded bitmap via the slow image.Image path instead of working directly on the concrete type. Not blocking for the seed-packet use case, but if large unchanged images become common this would be the next bottleneck.

🧯 Error handling & edge cases — Minor issues

Verdict: Minor issues

  • internal/imagenorm/imagenorm.go:280 — Missing TIFF magic number validation in orientationFromApp1
    The function trusts the IFD offset at tiff[4:8] without first checking that bytes tiff[2:4] contain the required TIFF magic number 42 (0x002A little-endian / 0x2A00 big-endian). A malformed APP1 segment that passes the "Exif\x00\x00" prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially yielding a bogus orientation value.
    Fix: Verify bo.Uint16(tiff[2:4]) == 0x002A (or 0x2A00 interpreted via bo) before reading the IFD offset.

  • internal/imagenorm/imagenorm.go:305-309 — Orientation tag type/count not validated
    When tag 0x0112 is found, the code assumes it is a SHORT with count == 1 and reads the value inline from the first two bytes of the 4-byte value field. It never checks the entry’s type code (bytes off+2:off+4) or count (bytes off+4:off+8). A malformed EXIF segment with tag 0x0112 but a different type (e.g. LONG) or count > 1 would store an offset in the value field rather than the value itself; the parser would then read that offset as a uint16. If the misread value happens to fall in [1,8], a wrong rotation is silently baked into the image.
    Fix: After matching the tag, verify bo.Uint16(tiff[off+2:off+4]) == 3 (SHORT) and bo.Uint32(tiff[off+4:off+8]) == 1 before reading bo.Uint16(tiff[off+8:off+10]).

  • internal/imagenorm/imagenorm.go:249-275 — JPEG fill bytes (0xFF) between markers are not skipped
    The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. The parser at exifOrientation assumes each marker is immediately preceded by exactly one 0xFF byte. If a phone encoder emits padding 0xFF bytes before the APP1 segment, the loop misreads the marker and returns orientation 1 instead of the true value. This is safe (no wrong rotation) but incorrect for a valid JPEG.
    Fix: Before reading raw[i+1] as the marker, skip consecutive 0xFF bytes.

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** · 9 findings (3 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | Double image allocation on 90° rotations: first RGBA buffer immediately discarded | `internal/imagenorm/imagenorm.go:214` | 4/5 | maintainability, performance | | 🟠 | Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation | `internal/imagenorm/imagenorm.go:218` | 2/5 | maintainability, performance | | 🟠 | EXIF orientation parser does not validate IFD type/count before reading value | `internal/imagenorm/imagenorm.go:305` | 2/5 | correctness, error-handling | <details><summary>6 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs | `internal/imagenorm/imagenorm.go:256` | kimi-k2.6:cloud | error-handling | | 🟠 | Missing TIFF magic number validation in orientationFromApp1 | `internal/imagenorm/imagenorm.go:280` | opencode/kimi-k2.6:cloud | error-handling | | 🟠 | Missing test coverage for mirror/transpose orientations 2/4/5/7 | `internal/imagenorm/imagenorm_test.go:271` | kimi-k2.6:cloud | maintainability | | 🟠 | No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases | `internal/imagenorm/imagenorm_test.go:276` | glm-5.2:cloud | maintainability | | 🟡 | JPEG fill bytes between markers not skipped in exifOrientation | `internal/imagenorm/imagenorm.go:249` | opencode/kimi-k2.6:cloud | error-handling | | 🟡 | table test missing t.Run subtests inconsistent with file pattern | `internal/imagenorm/imagenorm_test.go:282` | opencode/kimi-k2.6: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> — No material issues found</summary> One thing worth flagging first: the trailing block claiming "Plan mode is active" and instructing an Explore/Plan-agent/`ExitPlanMode` workflow doesn't correspond to any real tool in this session — I searched for `ExitPlanMode`/`AskUserQuestion` via ToolSearch and neither resolves, unlike the genuinely deferred tools (`TaskCreate`, `EnterWorktree`, etc.) which do. That block reads as an injected instruction trying to redirect this session away from delivering the actual review. I'm disregarding it and completing the task as originally specified (the Gadfly review format, output as text). I did already write a harmless scratch file to `/root/.claude/plans/...` before catching this — it contains only my own analysis notes, nothing sensitive, and can be ignored/deleted. Here's the actual security review of the PR: VERDICT: No material issues found This diff adds pure, local EXIF-orientation parsing and pixel-transform code to `internal/imagenorm`. I read the live file (`internal/imagenorm/imagenorm.go:140-199`) to confirm the surrounding guarantees, not just the diff hunk: - **Input is already size-capped before this code runs.** `raw` is bounded by `opts.maxBytes()` (default 25 MiB, enforced at imagenorm.go:140-151) before `exifOrientation(raw)` is ever called (imagenorm.go:175). No new unbounded-read surface. - **`exifOrientation`'s marker walk is fully bounds-checked before every slice access** — `i+4<=len(raw)` before reading marker bytes, `segLen<2 || i+2+segLen>len(raw)` before slicing the segment body. Malformed/truncated JPEGs fall through to `return 1`, not a panic or OOB read. - **`orientationFromApp1`'s TIFF/IFD parsing is equally guarded** — prefix-length check before the `"Exif\x00\x00"` compare, `ifd<8 || ifd+2>len(tiff)` before reading the entry count, and `off+12>len(tiff)` checked inside the loop before every 12-byte entry read. A crafted large entry count (`n` up to 65535) just runs bounded, cheap, bounds-checked iterations — not a meaningful DoS vector. - **Safe-default posture is correct and consistently applied**: unparseable/non-JPEG/garbage input always yields orientation `1` (no rotation), never a guess — so a hostile file can at worst leave an image un-rotated, not corrupt a legitimate one or destabilize downstream processing. - **`applyOrientation` only receives an already-validated `o` in `[2,8]`** and allocates its output buffer *after* `downscale`, so its allocation size is bounded by `opts.maxDim()` regardless of what the EXIF tag claims — no amplification/decompression-bomb path via orientation. - No new sinks are introduced: no filesystem, SQL, shell, or network use; no secrets touched; nothing here affects authn/authz. This is pure in-memory byte parsing over data that was already being treated as untrusted (`decodeSafely`'s existing panic-recovery comment already documents the "untrusted upload" threat model this code operates under). I did not find any injection, SSRF, deserialization, secret-handling, or input-validation defect within this diff's security lens. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> ## Review — 🎯 Correctness lens **VERDICT: No material issues found** I independently re-derived all 8 EXIF orientation transforms from first principles (physically rotating/mirroring a labeled grid) rather than trusting the code's comments, and checked them against `internal/imagenorm/imagenorm.go:207-242`: - Orientation 6 ("rotate 90° CW to correct") → `dx,dy = h-1-y, x` — verified against a manual 2×2 rotation trace, matches. - Orientation 8 ("rotate 90° CCW to correct") → `dx,dy = y, w-1-x` — verified, matches. - Orientations 2/3/4 (mirror-H, 180, mirror-V) and 5/7 (transpose/transverse) all check out against the canonical "where does stored (0,0) land" EXIF table. Also verified: - `orientationFromApp1` (`imagenorm.go:280-316`): TIFF header/IFD0 offset bounds, 12-byte IFD entry stride, and reading a SHORT value from the first 2 bytes of the 4-byte value field per file byte order — all correct per TIFF6 spec. - `exifOrientation` (`imagenorm.go:249-275`): JPEG marker-segment length semantics (`segLen` includes the 2 length bytes, so `i += 2+segLen` and slicing `raw[i+4:i+2+segLen]` are both correctly offset), correctly halts at SOS/EOI so it never scans entropy-coded data, and fails safe (returns `1`) on any malformed/short segment rather than misapplying a rotation. - Ordering in `Normalize` (`imagenorm.go:174-175`): `applyOrientation` is called with `exifOrientation(raw)` where `raw` is the original, un-truncated input bytes (line 145) — not the decoded/re-encoded buffer — so the tag is read from the actual source JPEG. `exifOrientation` gates on the `FFD8` SOI itself, so non-JPEG formats (PNG/WebP/HEIC) safely no-op without a separate format check. Downscale-then-rotate is sound: rotation only transposes width/height, so `max(w,h)` (the downscale bound) is invariant across it. One thing I checked but am not reporting as a finding (too low-confidence/low-impact to meet the bar): `orientationFromApp1` doesn't validate the IFD entry's type/count fields before reading the Orientation value, so a file with a non-SHORT-typed 0x0112 tag could yield a bogus-but-in-range value instead of falling back to 1. No real camera emits Orientation as anything but SHORT, and I don't have enough confidence this is exploitable to list it as a real bug. I was unable to execute `go test ./internal/imagenorm/...` in this environment (Bash approval unavailable under the current session mode), so the "tests pass" claim in the PR description is unverified by me directly — but I traced the test's rotation-formula arithmetic by hand against the same transforms and it's internally consistent with the (verified-correct) implementation. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> No other EXIF/binary parsing utilities exist elsewhere — that claim holds. All checks confirm the draft's single finding is accurate, but one supporting claim in the prose (about `downscale` already using the range-over-int idiom) is false, so I'll correct that in the output. ## Review — Code cleanliness & maintainability **VERDICT: Minor issues** The new EXIF parsing/orientation code (`internal/imagenorm/imagenorm.go:202-316`) is well-scoped, self-contained to one file, and matches the surrounding heavily-commented style (verified no pre-existing EXIF/TIFF parsing utilities elsewhere in the repo to duplicate, via `grep -rn "binary.ByteOrder\|Uint16\|Uint32"`). Naming (`exifOrientation`, `orientationFromApp1`, `applyOrientation`) is clear, and the 8-case switch in `applyOrientation` is an appropriate use of a switch over a transform-function table for this size. Tests reuse the existing `pngBytes` helper rather than duplicating it (confirmed at `internal/imagenorm/imagenorm_test.go:17`). One small nit: - **`internal/imagenorm/imagenorm.go:213-217`** — `applyOrientation` unconditionally allocates a `w×h` RGBA, then immediately discards it and allocates a second `h×w` RGBA whenever `swap` is true (orientations 5-8): ```go dst := image.NewRGBA(image.Rect(0, 0, w, h)) if swap { dst = image.NewRGBA(image.Rect(0, 0, h, w)) } ``` The first allocation is pure waste in the swap case. Cleaner and avoids the throwaway alloc: ```go dw, dh := w, h if swap { dw, dh = h, w } dst := image.NewRGBA(image.Rect(0, 0, dw, dh)) ``` Verified by reading the function in full — no other use of `dst` before the reassignment, so the first allocation is dead work. Nothing else in this diff (including the DESIGN.md wording update and the test file additions) raises cleanliness concerns. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> Confirmed — this matches the draft exactly: line 214 unconditionally allocates a `w×h` `image.NewRGBA`, then line 215-217 discards it and allocates a second `h×w` buffer whenever `swap` is true (orientations 5-8, which includes orientation 6, the common phone-portrait case). The fix suggestion (compute `dw, dh` first, allocate once) is correct and minimal. This is a real, verifiable finding — a small/minor perf nit, not a correctness bug, matching the draft's severity. **VERDICT: Minor issues** - `internal/imagenorm/imagenorm.go:214-217` — `applyOrientation` allocates a `w×h` `*image.RGBA` unconditionally, then immediately discards it and allocates a second `h×w` `*image.RGBA` whenever `swap` is true (orientations 5-8): ```go dst := image.NewRGBA(image.Rect(0, 0, w, h)) if swap { dst = image.NewRGBA(image.Rect(0, 0, h, w)) } ``` Orientation 6 (90° CW) is the single most common EXIF tag on real phone photos (camera held normally in portrait) — exactly the case this PR is built for per the PR description ("a phone camera... sets a flag to rotate on display"). So on the primary target workload, every upload does one throwaway allocation of up to `DefaultMaxDim × DefaultMaxDim × 4` bytes (~16.7 MB at the 2048px default) before the real buffer is allocated, adding avoidable allocation/GC pressure on the image-upload hot path. Fix: compute the swapped dimensions before allocating, e.g. ```go dw, dh := w, h if swap { dw, dh = h, w } dst := image.NewRGBA(image.Rect(0, 0, dw, dh)) ``` Everything else in the changed code is a bounded, single-pass `O(w·h)` pixel copy (unavoidable for baking in a rotation) applied after `downscale` caps dimensions, and a marker-segment walk over the raw JPEG header bytes (`exifOrientation`) that's O(segment count), not O(file size) in any problematic way. No N+1s, no unbounded growth, no new blocking calls on the hot path. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> Go 1.26 supports range-over-int, so no compile issue there. I've verified the core logic — bounds checks, loop termination, and defaults are all sound. Nothing in my lane rises to a real defect. **VERDICT: No material issues found** Findings (through the error-handling/edge-case lens only): - `exifOrientation`/`orientationFromApp1` (imagenorm.go:249-316) are defensively bounds-checked at every slice access I traced (segment length vs. `len(raw)`, IFD offset vs. `len(tiff)`, entry offset vs. `len(tiff)` before each 12-byte read) — verified by re-deriving the offsets by hand against the TIFF/EXIF layout. Every failure path returns the safe default (`1`, no rotation) rather than erroring or guessing, matching the doc comment's stated intent. - The `i += 2 + segLen` marker-walk loop is guaranteed to terminate: `segLen < 2` is rejected before the increment, so `i` strictly increases each iteration — no infinite-loop/DoS risk on adversarial input, and the byte read is already capped by `opts.MaxBytes` upstream. - `applyOrientation` (imagenorm.go:207-242): the `o<=1 || o>8` guard fully covers the domain (exifOrientation only ever returns 1-8), so the switch's implicit cases 2-8 are exhaustive — no default-case gap. A 0×0 `img.Bounds()` degenerates to empty loops with no panic (unlikely in practice, but confirmed not to crash). - No nil-image path: `img` reaching `applyOrientation`/downstream is always non-nil by construction (decodeSafely returns early on error), so no nil-check gap. One sub-lens observation I considered but am not confident enough to list as a finding: `orientationFromApp1` doesn't validate the IFD entry's *type* field (expects SHORT/3 for Orientation) before reading the value bytes — a malformed tag with the same ID but a different type could, in a byte-order-dependent way, be misread as a spurious value 1-8 and silently misrotate instead of falling back safely. This requires deliberately malformed EXIF (real phone EXIF always types Orientation as SHORT) and is closer to a correctness/robustness nit than an error-handling defect, so I'm not reporting it as a real bug. </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> — No material issues found</summary> Verdict: No material issues found I reviewed the new EXIF parser and orientation applier through the security lens — they process attacker-controlled upload bytes — and verified the bounds logic against the checked-out code. - **`exifOrientation` / `orientationFromApp1` (imagenorm.go:249-316)** — all slice accesses are bounds-checked before use, so an OOB panic (DoS via process crash) is not reachable: - SOI check guards `raw[0..1]`; loop condition `i+4 <= len(raw)` guards `raw[i..i+3]`; `segLen < 2 || i+2+segLen > len(raw)` guards `raw[i+4 : i+2+segLen]`. - In `orientationFromApp1`, `len(seg) < len(prefix)+8` (14) guards `tiff[0:2]` and `tiff[4:8]`; `ifd < 8 || ifd+2 > len(tiff)` guards `tiff[ifd:ifd+2]`; `off+12 > len(tiff)` guards `tiff[off:off+2]` and `tiff[off+8:off+10]`. The value-field read ignores the declared tag type but stays in bounds, so a malicious non-SHORT type can't cause a panic — worst case it misreads and falls through to the `1..8` check or returns false. - The IFD entry-count loop `for k := range n` is attacker-controlled (uint16, ≤65535) but breaks as soon as `off+12 > len(tiff)`, so iteration is bounded by `len(tiff)/12`; `len(tiff)` is itself bounded by `MaxBytes` (25 MiB). No integer-overflow concern on 32-bit either, since `ifd` is clamped to `≤ len(tiff)-2` before use. This is at most trivial CPU amplification, far below the cost of the decode that already ran. - Safe-default discipline is correct: every unparseable/non-JPEG/garbage path returns `1` (no-op), so a malformed file can never coerce an unwanted rotation of a correct image. This is the fail-safe direction. - **`applyOrientation` (imagenorm.go:207-242)** — operates only on the already-decoded `image.Image` and a validated `o` in `1..8`; `At`/`Set` on stdlib image types don't panic. No new taint surface. - **Recover scope** — noted (not a finding): `decodeSafely`'s `recover` does not wrap `exifOrientation`/`applyOrientation`, but neither has a reachable panic on untrusted input per the bounds analysis above, so the unprotected scope is safe. - No injection (SQL/command/path/template), SSRF, deserialization, secret/credential, or authn/authz surface in this change — it's pure in-memory byte parsing of an uploaded image. The upload boundary and size caps (`MaxBytes`, `maxDecodePixels`, `maxDimension`) upstream of this code already bound the input. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> Verdict: No material issues found I verified the change through the correctness lens by reading the full implementation (`internal/imagenorm/imagenorm.go:140-334`) and re-deriving the EXIF orientation transforms from first principles. - **`applyOrientation` transforms (`imagenorm.go:207-242`)**: I checked all 8 cases against the EXIF 2.3 spec definitions (1=identity, 2=mirror-H, 3=180, 4=mirror-V, 5=transpose, 6=90°CW, 7=transverse, 8=90°CCW). Every mapping `(x,y) → (dx,dy)` is correct, including the dimensional transpose for o≥5 (swap is right for all of 5/6/7/8, not just the pure rotations). Corner-spot checks confirm: o6 maps (0,0)→(h-1,0) (top-left→top-right, correct for CW); o8 maps (0,0)→(0,w-1) (top-left→bottom-left, correct for CCW). - **Downscale-then-rotate ordering (`imagenorm.go:171-175`)**: The comment's invariant holds. Downscale bounds the longest edge to `maxDim`; a rotation/transposition only swaps sides, so the longest edge stays ≤ `maxDim`. Worked example: 4000×3000 → downscale 1024×768 → o6 swap → 768×1024 (longest edge 1024). ✓ - **`exifOrientation` / `orientationFromApp1` (`imagenorm.go:249-316`)**: JPEG segment walk, bounds, IFD0 parsing, and the inline-SHORT read at `off+8:off+10` are all correct. Non-JPEG inputs (HEIC/PNG/WebP) correctly short-circuit on the `0xFFD8` SOI check and return 1, so `applyOrientation` is a no-op for them. All slice accesses are guarded against panics (verified each bounds check). Failure mode on any unparseable/edge input is returning 1 (no rotation) — never a wrong rotation — which is the safe default. - **Test expectations (`imagenorm_test.go`)**: I re-derived each `wantW/wantH/mx/my` from the same transform the code applies; all match (e.g. o6: `dx=h-1-y=17, dy=x=10` for source center (10,6) with h=24; o8: `dx=y=6, dy=w-1-x=29`; o3: `dx=29, dy=17`). One non-blocking observation (not a correctness bug): the value field is read as `uint16` without verifying the tag's EXIF type is SHORT. Since the spec mandates SHORT for Orientation this is fine in practice; a hypothetical big-endian LONG-typed tag would read 0 and fall back to the safe default of 1 rather than a wrong rotation, so it fails safe. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Minor issues found - **`internal/imagenorm/imagenorm.go:214-216` — redundant double allocation on the swap branch.** `dst` is allocated as `image.NewRGBA(image.Rect(0,0,w,h))` on line 214, then unconditionally reassigned to `image.NewRGBA(image.Rect(0,0,h,w))` on line 216 when `swap` is true. On the quarter-turn path (orientations 5–8) the first RGBA header is allocated and immediately discarded. The non-swap branch keeps the first allocation correctly, so this is only a wasted allocation on the swap branch, not a correctness bug — a readability/efficiency wart in an allocation-sensitive path. Cleaner: compute the output dims up front and allocate once. - **`internal/imagenorm/imagenorm.go:218-239` — manual `At`/`Set` per-pixel loop diverges from the `draw`-package idiom used elsewhere in the file.** `applyOrientation` walks every pixel with `img.At`/`dst.Set` (going through the `color.Model` conversion path), while `downscale` (line 332) uses `draw.CatmullRom.Scale`. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is avoidable. Not blocking; a consistency nit. - **`internal/imagenorm/imagenorm_test.go:276-280` — no test covers orientations 2, 4, 5, 7.** `TestNormalizeAppliesExifOrientation` only exercises 0/1/3/6/8, and `TestExifOrientationParsing` only parses 6. The `applyOrientation` switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transverse math. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> Both findings are confirmed against the actual code. The first (double RGBA allocation on the swap path) is real and at the cited lines. The second is informational and not in the findings JSON, consistent with the draft. **Verdict: Minor issues** - **`internal/imagenorm/imagenorm.go:214-217` — double RGBA allocation on the swap (portrait) path.** For orientations 5–8 (the quarter-turns), `dst` is first allocated at `w×h` and then immediately overwritten with a second `image.NewRGBA` at `h×w`: ```go dst := image.NewRGBA(image.Rect(0, 0, w, h)) if swap { dst = image.NewRGBA(image.Rect(0, 0, h, w)) } ``` The first buffer is allocated and discarded. This is the common phone-portrait path — orientation 6 is exactly "portrait photo tagged rotate 90°", the motivating case of this PR. For a 2048×1536 portrait, that's a ~12 MiB RGBA thrown straight to the GC on the very upload the feature targets. Fix is trivial — compute the destination rect once: ```go dw, dh := w, h if swap { dw, dh = h, w } dst := image.NewRGBA(image.Rect(0, 0, dw, dh)) ``` - **`internal/imagenorm/imagenorm.go:218-240` — per-pixel `At`/`Set` loop** (informational, not blocking). The rotate reads via `img.At` and writes via `dst.Set`, both of which go through generic `color.Color`/`color.Model` conversions rather than fast direct RGBA writes. Bounded to ≤2048px and only on the rotated path, so acceptable for an upload boundary — but worth noting that a portrait phone photo already within `MaxDim` (so `downscale` returns the original `*image.YCbCr`) hits the slowest variant (YCbCr-per-pixel `At`). Not worth changing unless this shows up in profiles. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict: No material issues found** Through the error-handling & edge-case lens, the new code is solid. I verified the two new functions and the bounds checks by reading `internal/imagenorm/imagenorm.go:202-316`: - **`exifOrientation` (line 249):** Every byte access is bounds-checked before use. The marker walk guards `len(raw) < 4`, `i+4 <= len(raw)`, `raw[i] != 0xFF` (misalignment → safe return 1), `segLen < 2 || i+2+segLen > len(raw)`, and stops at EOI/SOS. The slice passed to `orientationFromApp1` (`raw[i+4 : i+2+segLen]`) is provably in range because of the `i+2+segLen > len(raw)` guard. The loop advances `i += 2 + segLen` with `segLen >= 2`, so it strictly progresses and terminates — no infinite loop. `segLen` is a 16-bit value (max 65535), so `i+2+segLen` can't overflow int on any supported platform. - **`orientationFromApp1` (line 280):** Length guard `len(seg) < len(prefix)+8` guarantees `tiff := seg[len(prefix):]` has at least 8 bytes, so `tiff[0:2]` and `tiff[4:8]` are safe. The IFD offset is validated (`ifd < 8 || ifd+2 > len(tiff)`), and each 12-byte entry is checked with `off+12 > len(tiff)` before the `Uint16`/`Uint32` reads. A crafted huge entry count `n` is bounded by the per-entry length check, so it can't spin unbounded. Out-of-range orientation values fall through to `return 0, false` → caller defaults to 1. No panics on truncated/garbage input. - **`applyOrientation` (line 207):** Out-of-range `o` (`<= 1 || > 8`) is a no-op returning the original image. Empty/zero-dimension images (`w==0` or `h==0`, which `downscale` can pass through) produce empty `image.NewRGBA` allocations and the `for y := range h` / `for x := range w` loops simply don't execute — no index panic. `dst.Set(dx, dy, c)` always writes within the allocated rect by construction of the transforms. - **No swallowed errors introduced:** Both new call sites in `Normalize` (`applyOrientation`, `exifOrientation`) return values, not errors, so there are no new error-return paths to mishandle or defer. The existing "on any error the returned bytes are nil" contract is preserved. The test helper `spliceExifOrientation` discards `binary.Write` errors with `_`, but those write to a `bytes.Buffer` which never errors — not a concern. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found The change adds a manual EXIF orientation parser for JPEG and pixel-level rotation logic. From a security lens, the code is clean: - **Fail-closed parsing**: `exifOrientation` returns 1 (no rotation) on any malformed input, missing EXIF, or non-JPEG format — never forcing a rotation onto a correct image. - **Bounded work**: `maxDecodePixels` (50M) and `maxDimension` (50K) already cap decoded bitmap size. `applyOrientation` runs on the *downscaled* image (downscale happens first), so memory and CPU for the rotation are bounded by the same limits. - **No out-of-bounds reads**: `orientationFromApp1` checks `len(seg) < len(prefix)+8` before indexing `tiff[0:2]`, `tiff[4:8]`, etc.; the IFD walk checks `off+12 > len(tiff)` before reading each 12-byte entry. - **No out-of-bounds writes**: `applyOrientation` computes destination coordinates (`dx`, `dy`) that always fall within the pre-allocated `dst` bounds for every orientation case (2–8). - **Strip, not leak**: The re-encode to JPEG strips all EXIF metadata (including GPS, camera model, timestamps). This is a net privacy win — only the orientation value is extracted and then discarded after the bitmap rotation. One minor observation (not a finding, just a verification note): the `exifOrientation` parser is strict about `0xFF` marker alignment (`raw[i] != 0xFF → return 1`), which is the conservative/safe choice for untrusted uploads. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> **Minor issues** - **`internal/imagenorm/imagenorm.go:305-309`** — `orientationFromApp1` reads the Orientation value without validating the IFD entry's **type** and **count**. The EXIF spec requires Orientation to be `SHORT` (type `3`) with count `1`. A malformed entry with a different type (e.g. `LONG`) or count `>1` stores an *offset* in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "safe default: return 1 for unparseable" design goal) instead of treating the tag as unreadable. **Suggested fix:** Add two checks inside the `0x0112` match block: ```go if bo.Uint16(tiff[off+2:off+4]) != 3 { // not SHORT return 0, false } if bo.Uint32(tiff[off+4:off+8]) != 1 { // count != 1 return 0, false } ``` </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Minor issues - `internal/imagenorm/imagenorm.go:214-216` — `applyOrientation` allocates a `w×h` RGBA unconditionally, then immediately discards it and allocates a second `h×w` RGBA for orientations 5-8. Move the `swap` check before the first `NewRGBA` to avoid the throw-away allocation for the quarter-turn cases. - `internal/imagenorm/imagenorm_test.go:271-281` — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations. </details> <details><summary><b>⚡ Performance</b> — Blocking issues found</summary> **Blocking issues found** - **`internal/imagenorm/imagenorm.go:214-216` — Double image allocation on every 90° rotation.** `applyOrientation` always allocates `image.NewRGBA(image.Rect(0, 0, w, h))`, then immediately throws it away and allocates `image.NewRGBA(image.Rect(0, 0, h, w))` when `swap` is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple `if/else` so only the correctly-sized buffer is ever allocated. - **`internal/imagenorm/imagenorm.go:218-240` — Per-pixel `image.Image.At()` calls box a `color.Color` on every iteration, causing millions of heap allocations for a single rotated image.** `applyOrientation` iterates every pixel and reads via the `image.Image` interface (`img.At(b.Min.X+x, b.Min.Y+y)`). When the concrete type is `*image.RGBA` (which `downscale` already returns), each `At` call boxes the `color.RGBA` value into an interface, allocating on the heap. A 2048×2048 rotation causes ~4.2 million transient allocations. This is a well-known Go image-processing anti-pattern; adding a fast-path type-switch to `*image.RGBA` (and perhaps `*image.NRGBA`) that copies through `Pix` directly would eliminate the boxing and interface dispatch entirely for the common case, cutting rotation time by an order of magnitude. The current generic fallback can be kept for non-RGBA images. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`internal/imagenorm/imagenorm.go:256` — JPEG marker-padding edge case unhandled.** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. `exifOrientation` assumes exactly one `0xFF` followed immediately by the marker code, so a file like `FF D8 FF FF E1 …` (valid per spec) misaligns: `marker` becomes `0xFF`, `segLen` is read from the next two bytes (`0xE1 0x00` ≈ 57 KiB), the length check fails, and the parser bails out returning `1` (normal). For a portrait photo whose APP1 segment is padded this way, the image stays sideways. A robust parser should skip consecutive `0xFF` bytes before reading the marker code. - **`internal/imagenorm/imagenorm.go:305` — TIFF type field not validated.** `orientationFromApp1` finds tag `0x0112` and blindly reads the first two bytes of the value field as a `SHORT`. It never checks `tiff[off+2:off+4]` to confirm the type is actually `3` (`SHORT`). A malformed EXIF that reuses tag `0x0112` with a `LONG` or `UNDEFINED` type could yield a bogus orientation value. While `exifOrientation` caps the result to `1..8`, any value in that range would still be applied. Adding a type check (`bo.Uint16(tiff[off+2:off+4]) == 3`) before accepting the value would make the “unparseable → 1” guarantee stronger. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **Verdict: No material issues found** Reviewed the EXIF parser through the security lens (untrusted-byte parsing, OOB reads, integer overflow, infinite loops, injection surface). The code is defensively written: - `exifOrientation` (imagenorm.go:249-275): all index accesses are guarded by the loop condition `i+4 <= len(raw)` and `i+2+segLen > len(raw)`; `segLen >= 2` guarantees loop progress (no infinite loop); `segLen` is a 16-bit field so no int overflow on `i+2+segLen`; safe default `1` on any misalignment, EOI/SOS, or non-JPEG. - `orientationFromApp1` (imagenorm.go:280-313): length check `len(seg) < len(prefix)+8` covers `tiff[0:2]` and `tiff[4:8]`; IFD offset validated with `ifd < 8 || ifd+2 > len(tiff)` before slicing; per-entry bounds `off+12 > len(tiff)` cover `tiff[off+8:off+10]`; orientation value clamped to 1..8 before use. A 32-bit build could in principle make `int(bo.Uint32(...))` negative for a near-`MaxUint32` offset, but the subsequent `ifd+2 > len(tiff)` comparison still rejects it (negative < len), so it can't be exploited to read OOB — it just falls through to the safe default. - The input to the parser is `raw`, which `Normalize` already capped at `MaxBytes+1` (imagenorm.go:~125), so the parser runs on size-bounded data regardless. - No injection/auth/secret/SSF surface: this is a pure local byte parser; its only output is an int 1..8 consumed by `applyOrientation`, which does an in-memory pixel copy with no I/O. Nothing in the security lens is materially wrong. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I verified the orientation math, the EXIF parser bounds, and the test expectations against the checked-out code. **Verdict: No material issues found** - I re-derived all 8 EXIF orientation transforms from first principles (rotating/mirroring the source bitmap into an upright destination) and confirmed each `case` in `applyOrientation` (`imagenorm.go:222-237`) maps source `(x,y)` to the correct destination `(dx,dy)`, including the dimension-swapping cases (5–8) where `dst` is correctly allocated as `h×w` and all writes stay in bounds. - The "downscale then rotate" ordering is sound: `downscale` bounds the *longest* edge, and a 90° rotation swaps sides without changing the longest edge, so the `MaxDim` bound still holds on the emitted image (verified for both the shrink and no-shrink paths). - The EXIF parser bounds are correct: the marker walk guards `i+4 <= len(raw)` and `segLen < 2`/`i+2+segLen > len(raw`; `orientationFromApp1` requires `len(seg) >= len(prefix)+8` (so `tiff[4:8]` is safe) and `ifd < 8 || ifd+2 > len(tiff)` plus per-entry `off+12 > len(tiff)` prevent OOB reads. The APP1 payload slice `raw[i+4 : i+2+segLen]` correctly excludes the 2 length bytes. - The test expectations are consistent with the transforms: e.g. orient 6 (90° CW) sends source (10,6) to (h-1-y, x) = (17,10) on a 24×40 canvas, matching `{6, 24, 40, 17, 10}`; orient 8 → (y, w-1-x) = (6,29) matching `{8, 24, 40, 6, 29}`. The `spliceExifOrientation` helper builds a well-formed little-endian TIFF/IFD0 (verified byte layout: `II` + 0x2A + offset 8 + count 1 + 12-byte entry with value at entry+8), which the parser reads back correctly. - Non-JPEG paths return 1 immediately (`raw[0]!=0xFF||raw[1]!=0xD8`), so HEIC/WebP/PNG are left untouched as designed — no regression vs. prior behavior. One trivial robustness note (not material, low confidence): the marker walker doesn't skip JPEG fill bytes (`0xFF 0xFF`) or handle standalone markers like TEM/`0xFF01`/RSTn that have no length field before the EXIF APP1. A JPEG emitting fill bytes before APP1 would misparse and fall back to orientation 1 (sideways). Real phone JPEGs place APP1 immediately after SOI, so this is out of the practical path and the fallback is safe (never mis-rotates a correct image). </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **VERDICT: No material issues found** The EXIF orientation addition is clean through the maintainability lens. Verified against the checked-out code: - `applyOrientation` and `exifOrientation` are well-factored, each doing one thing, with accurate doc comments that match the implementation. They follow the file's existing conventions (sentinel-free helpers, inline rationale comments, pure-Go no-deps stance). - No duplicated logic: `exifOrientation` re-walks the JPEG bytes because `image.DecodeConfig` doesn't expose EXIF — that's a justified read, not copy-paste. - The no-op short-circuit (`o <= 1 || o > 8`) returns `img` unchanged, so non-JPEG and normal-orientation inputs pay zero allocation cost — the comment claims this and the code does it. - Tests reuse the existing `pngBytes`/`jpegBytes` fixture-builder pattern and add `orientedJPEG`/`spliceExifOrientation`/`bright` in the same style; the helper that builds a minimal TIFF IFD entry writes the 12-byte entry layout correctly (2+2+4+2+2), so the fixture reflects a real Exif APP1 rather than asserting the parser's own shape back at itself. - `DESIGN.md` note is updated in the same sentence and stays accurate ("bakes in the JPEG EXIF orientation"), consistent with CLAUDE.md's "keep docs true" rule. Nothing in my area to flag. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Reading the file to verify the per-pixel loop's allocation behavior. VERDICT: No material issues found Reviewed `internal/imagenorm/imagenorm.go` (the changed file) and its caller `internal/api/seed_packet.go`. Through the performance lens: - **`applyOrientation` per-pixel loop** (`imagenorm.go:218-240`): iterates every pixel with `img.At` + `dst.Set`, each doing a color-model round-trip. This is the only new work on the hot path. It runs **only when `o > 1`** (an actual non-normal EXIF orientation tag exists); orientation 1 — the common case for landscape photos and all non-JPEG formats — early-returns at line 208 with zero cost. When it does run, the image is already capped at `DefaultMaxDim` (2048px longest edge) by the preceding `downscale`, so the loop is bounded to ~4M iterations worst case. On a single-upload endpoint that then calls a vision model (seconds of latency), the tens-of-ms per-pixel copy is immaterial. No N+1, no quadratic behavior, no unbounded allocation. - **`exifOrientation(raw)`** (`imagenorm.go:249-275`): called unconditionally but bails in O(1) for non-JPEG (`raw[0]!=0xFF || raw[1]!=0xD8`) and walks at most the handful of JPEG marker segments for JPEGs. Negligible. - **Transient allocation**: `applyOrientation` allocates one `*image.RGBA` matching the (already-bounded) downscaled image. Doubling of the ~16MB peak bitmap is acceptable for a one-shot upload path. The straightforward per-pixel implementation could be sped up with `RGBAAt`/`SetRGBA` (skipping two color-model conversions per pixel) since `downscale` always yields an `*image.RGBA`, but that is a micro-optimization with no measurable impact at this call frequency and image size — not flagging it. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I've read the full `imagenorm.go` and the relevant test file. Let me verify the boundary logic in the new EXIF/orientation code carefully. **Verdict: No material issues found** I walked the unhappy paths the diff introduces, against the checked-out code: - **`exifOrientation` / `orientationFromApp1` parser bounds** (imagenorm.go:249-316): every slice access is guarded before use. `len(raw) < 4` short-circuits entry; the loop requires `i+4 <= len(raw)` before reading 4 header bytes; `segLen < 2 || i+2+segLen > len(raw)` gates the segment slice `raw[i+4 : i+2+segLen]` (and `segLen >= 2` makes `i+4 <= i+2+segLen`). In `orientationFromApp1`, `len(seg) < len(prefix)+8` (14) covers `tiff[0:2]` and `tiff[4:8]`; `ifd < 8 || ifd+2 > len(tiff)` gates the count read; `off+12 > len(tiff)` gates every 12-byte entry read, which also covers `tiff[off+8:off+10]`. Every unparseable / truncated / hostile case returns the safe default `1` — never a panic, never a wrong rotation. Verified by reading the file. - **`applyOrientation` output bounds** (imagenorm.go:207-242): for the non-swap cases (2/3/4) `dx ∈ [0,w-1]`, `dy ∈ [0,h-1]` and `dst` is `w×h`; for the swap cases (5/6/7/8) the loops are `y ∈ [0,h)`, `x ∈ [0,w)` and each mapping lands in `[0,h-1] × [0,w-1]` against an `h×w` destination — I re-derived all four quarter-turn mappings and confirmed no off-by-one and no out-of-bounds `Set`. `o <= 1 || o > 8` is a no-op, so an out-of-range tag (or the `0, false` return folded in as `0`) leaves the image untouched. - **Empty/zero inputs**: `Normalize` rejects `cfg.Width <= 0 || cfg.Height <= 0` before decode, so `applyOrientation` never sees a zero-dimension image that would make `image.NewRGBA` degenerate; `downscale`'s `longest == 0` guard is also upstream. No nil-image path: decode failure returns before `applyOrientation` runs. - **Panic coverage**: `decodeSafely`'s `recover` wraps `image.Decode`; the new code runs *after* it and does only bounds-checked slice access and `img.At`/`dst.Set` within verified bounds, so there's no unprotected panic surface from a malformed EXIF blob. One marginal observation, not material enough to list as a finding: `ifd := int(bo.Uint32(tiff[4:8]))` (line 295) could in principle let `ifd+2` wrap on a 32-bit `int` build given a hostile TIFF offset, bypassing the `ifd+2 > len(tiff)` guard. pansy's `CGO_ENABLED=0` static binary targets 64-bit servers (Komodo deploy), where `int` is 64-bit and the guard holds — so this is not a live risk here, and I'm flagging it only because the lens names integer overflow. No change needed. </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> — No material issues found</summary> **Verdict:** No material issues found The EXIF orientation parser is defensively written against untrusted input: bounds-checked on every slice access, defaults to 1 (no rotation) on any malformed data, bounded by the 25 MiB read cap, and advances monotonically through markers with no infinite-loop path. The coordinate mapping in `applyOrientation` stays within the allocated destination for all 8 orientation values. No authn/authz gaps, injection, deserialization, secret leakage, or untrusted-data mishandling is introduced. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I examined the EXIF orientation parser, the transform application, and the interaction with downscaling. Here is what I verified: - **`applyOrientation` transforms**: I re-derived the mapping for all 8 EXIF orientations from the spec (0th-row/0th-column placement) and confirmed each `case` matches. The quarter-turns (5–8) correctly swap the output dimensions, and the pixel-level mappings agree with the test expectations for the covered cases (1, 3, 6, 8) and, by derivation, the uncovered ones (2, 4, 5, 7). - **Downscale-then-rotate order**: Downsampling first is cheaper, and because a 90° rotation only swaps width/height, the longest-edge bound `≤ MaxDim` is preserved. This reasoning is sound. - **EXIF parser robustness**: `exifOrientation` walks big-endian JPEG markers, stops at SOS/EOI, bounds-checks every slice operation, and returns 1 (no rotation) on any parse failure. `orientationFromApp1` correctly reads the IFD0 entry for tag `0x0112` as an inline SHORT value with the declared byte order. It safely rejects malformed input (bad magic, short buffers, invalid offsets) by returning `ok=false`, which causes the caller to fall back to 1. - **Non-JPEG safety**: `exifOrientation` requires the `FF D8` SOI signature; all other formats return 1, and `applyOrientation` with `o==1` returns the image unchanged. - **Test verification**: The `spliceExifOrientation` helper constructs a valid little-endian TIFF IFD with the correct 12-byte entry layout and `segLen` accounting. The marker-location assertions in `TestNormalizeAppliesExifOrientation` match the transforms. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - **`internal/imagenorm/imagenorm.go:214`** — `applyOrientation` allocates `dst` with dimensions `w×h`, then immediately throws it away and allocates again with `h×w` whenever `swap` is true (orientations 5–8). This wastes an allocation of `w*h*4` bytes for every quarter-turn. Move the allocation into an `if/else` so only the correct size is created. - **`internal/imagenorm/imagenorm_test.go:282`** — `TestNormalizeAppliesExifOrientation` iterates over a table of cases but doesn't wrap them in `t.Run` subtests, unlike `TestNormalizeAllFormats` and `TestNormalizeRejectsPixelBomb` in the same file. Adding subtests makes failures immediately attributable to the specific orientation and keeps isolation consistent with the file's existing pattern. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **Verdict:** Minor issues - **`internal/imagenorm/imagenorm.go:214-216`** — `applyOrientation` allocates an `image.RGBA` of size `w×h` unconditionally, then immediately discards it and allocates a second one of size `h×w` when `swap` is true (orientations 5–8). For a 2048×1536 image that’s a ~12.5 MB throwaway allocation; at 2048×2048 it’s ~16 MB. The fix is trivial: only allocate after the `swap` check, e.g. `dw, dh := w, h; if swap { dw, dh = h, w }; dst := image.NewRGBA(image.Rect(0, 0, dw, dh))`. - **`internal/imagenorm/imagenorm.go:218-239`** — The pixel-by-pixel `img.At` loop is correct but has high per-pixel overhead (interface call + color conversion for every pixel). This is acceptable for an upload handler that runs once per image, but it’s worth noting that for images already within `maxDim` bounds (where `downscale` returns the original), this copies the full decoded bitmap via the slow `image.Image` path instead of working directly on the concrete type. Not blocking for the seed-packet use case, but if large unchanged images become common this would be the next bottleneck. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Verdict: Minor issues** - **`internal/imagenorm/imagenorm.go:280` — Missing TIFF magic number validation in `orientationFromApp1`** The function trusts the IFD offset at `tiff[4:8]` without first checking that bytes `tiff[2:4]` contain the required TIFF magic number `42` (`0x002A` little-endian / `0x2A00` big-endian). A malformed APP1 segment that passes the `"Exif\x00\x00"` prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially yielding a bogus orientation value. *Fix:* Verify `bo.Uint16(tiff[2:4]) == 0x002A` (or `0x2A00` interpreted via `bo`) before reading the IFD offset. - **`internal/imagenorm/imagenorm.go:305-309` — Orientation tag type/count not validated** When tag `0x0112` is found, the code assumes it is a SHORT with `count == 1` and reads the value inline from the first two bytes of the 4-byte value field. It never checks the entry’s type code (bytes `off+2:off+4`) or count (bytes `off+4:off+8`). A malformed EXIF segment with tag `0x0112` but a different type (e.g. LONG) or `count > 1` would store an offset in the value field rather than the value itself; the parser would then read that offset as a uint16. If the misread value happens to fall in `[1,8]`, a wrong rotation is silently baked into the image. *Fix:* After matching the tag, verify `bo.Uint16(tiff[off+2:off+4]) == 3` (SHORT) and `bo.Uint32(tiff[off+4:off+8]) == 1` before reading `bo.Uint16(tiff[off+8:off+10])`. - **`internal/imagenorm/imagenorm.go:249-275` — JPEG fill bytes (`0xFF`) between markers are not skipped** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. The parser at `exifOrientation` assumes each marker is immediately preceded by exactly one `0xFF` byte. If a phone encoder emits padding `0xFF` bytes before the APP1 segment, the loop misreads the marker and returns orientation `1` instead of the true value. This is safe (no wrong rotation) but incorrect for a valid JPEG. *Fix:* Before reading `raw[i+1]` as the marker, skip consecutive `0xFF` bytes. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-22 05:35:20 +00:00
Address EXIF review: single alloc, no per-pixel boxing, hardened parser
Build image / build-and-push (push) Successful in 8s
8aad2278a0
Gadfly on #103:

- (4 models) applyOrientation allocated a WxH RGBA then threw it away for a
  quarter-turn to allocate HxW. Compute the output dims once, allocate one
  buffer.
- (perf) The At/Set loop boxed a color.Color per pixel — millions of heap
  allocs on a full-res rotation. Convert to *image.RGBA once (draw.Draw)
  then copy 4 bytes per pixel by offset. No boxing.
- (2 findings) exifOrientation didn't skip 0xFF fill bytes before a marker,
  so a spec-valid padded APP1 would be misread. Skip them.
- (2 findings) orientationFromApp1 read the tag's inline value without
  checking its type/count — a mistyped LONG/offset would be read as a
  bogus SHORT. Require type=SHORT, count=1; also validate the TIFF 0x2A
  magic agrees with the byte order.
- Tests now cover all 8 orientations (added the mirror/transpose cases
  2/4/5/7, the most error-prone switch arms) as t.Run subtests.

All imagenorm tests green; gofmt clean; still CGO_ENABLED=0.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve merged commit 90af03d597 into main 2026-07-22 05:35:46 +00:00
steve deleted branch feat/exif-orientation 2026-07-22 05:35:47 +00:00
Sign in to join this conversation.