Clamp padding to the canvas the artwork actually gets [patch]

[matt-edmondson]

Fixes #110

Arguments.Validate checks padding < size / 2. But sizing is downscale-only, so the canvas is min(trimmedSquareSize, size) — and the two differ for any artwork that trims smaller than --size:

iconhelper --input ./icons --output ./out --size 100 --padding 45

45 < 100/2, so this validates. Artwork trimming to 80 square then asks for 90 pixels of padding on an 80 pixel canvas, and finalContentSize comes out as 80 - 90 = -10.

System.InvalidOperationException: Target width -10 and height -10 must be greater than zero.

ProcessDirectory's broad per-file catch turns that into a skipped file, a non-zero Failed count and exit code 2 — on a batch configured exactly as the help text and README documented.

The fix, and why clamping rather than reporting

EffectivePadding(canvasSize, padding) clamps to (canvasSize - 1) / 2, which leaves at least one pixel of content on any canvas (an even canvas keeps 2, an odd one keeps 1).

The issue offered two routes — clamp, or report a per-file "padding too large" error. This takes the clamp, because the requested padding is not wrong, it is only unsatisfiable on that particular input. A directory of assorted icon sizes is the normal case for this tool, and the reporting route would fail exactly the small ones while the large ones succeeded, which is the same partial-failure exit code the user is trying to get away from. Padding that already fits is applied unchanged, so the clamp engages only where the tool used to throw.

Scope

• IconHelper/IconHelper.cs — EffectivePadding, called from CropSquareAndResize. That is the whole behaviour change.
• IconHelper/Arguments.cs, README.md, CLAUDE.md — all three stated the constraint as though --size were the bound. Correcting the code without correcting them would leave three places asserting the check is complete when it is not.

Tests

Four cases, at both layers.

test	asserts
ProcessImageTests.PaddingValidAgainstTheRequestedSizeStillWorksOnSmallerArtwork	the issue's acceptance criterion — the documented scenario no longer throws
ProcessImageTests.PaddingTooLargeForTheCanvasStillLeavesVisibleContent	the clamp leaves artwork behind, and still insets it
ProcessImageTests.PaddingThatFitsIsAppliedExactlyAndNotClamped	padding of 8 on an 80 canvas is applied as exactly 8, not clamped
ProcessDirectoryTests.SmallArtworkWithValidatedPaddingDoesNotFailTheBatch	end to end: Validate passes, Failed is 0, exit code is 0

The batch-level test is the one that matters, because the throw was never visible as a throw — the broad catch converted it into a count. A ProcessImage test alone would prove the exception is gone without proving the user-facing symptom is.

Confirmed the tests depend on the change by loosening the clamp bound to Math.Min(padding, canvasSize) and re-running: 3 failed, 6 passed, with the original symptom back in the batch test's captured output:

Failed to process .../small.png: InvalidOperationException: Target width -10 and height -10 must be greater than zero.

PaddingThatFitsIsAppliedExactlyAndNotClamped passes either way by design — it is the guard against the clamp reaching inputs that were always valid, not evidence for the fix.

Release build clean, zero warnings.

One environmental caveat

The 8 GoldMasterTests fail in my container, and they fail identically on an unmodified tree — I stashed everything and re-ran to check: 69 tests / 8 failed before, 73 tests / 8 failed after, same 8 cases. The cause is the one CLAUDE.md already documents: the fixtures are Git LFS pointers where git lfs is not installed, so ImageSharp rejects them as version https://git-lfs.... They should pass on a runner with LFS.

Worth flagging because the gold master matrix includes (midtone-grey-shape, #3366CC, 96, 8) and (antialiased-circle, #FF8800, 64, 4), which are the committed padding cases — so CI is the first thing that will actually exercise whether the clamp leaves those two byte-identical. It should: both have padding well inside (canvasSize - 1) / 2 for their canvases, so EffectivePadding returns the requested value untouched. If either moves, that is a real finding rather than noise.

Not covered

Validate's padding >= size / 2 check is left as it is. It still catches the genuinely impossible request up front, and it is the only check available at argument-parsing time, since the effective canvas is not known until an individual file has been decoded and trimmed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XH3MikiXVCii3abMojvibs

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud