TeamAI/NovelAi.ImageGen
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(v5): grid-align inpainting masks before upload (8x8 latent-cell requirement) #4

Merged
bjoern merged 3 commits from fix/v5-mask-grid-alignment into main 2026-08-21 18:15:38 +02:00
Conversation 6 Commits 3 Files changed 7 +289 -10
matikane commented 2026-08-21 16:58:27 +02:00
Member
Copy link

Live-observed on Kagura (v5 expression infills): visible artifacts ringing the inpainted region — the mask edge blends regenerated content against preserved pixels.

Root cause

The V5 multipart transport uploaded masks raw at native resolution (ResolveImage(mask) — no alignment, no re-encode), while the V4 path has always run ImageScaler.ScaleMaskWithGridAlignment (fixed in 1.1.0: "mask edges aligned to an 8x8 pixel grid"). The API requires that alignment because a latent cell is 8×8 pixels — an unaligned mask edge leaves cells half-covered, the server regenerates them, then blends against preserved pixels along a jagged sub-block line → the artifact ring.

The HAR captures couldn't preserve binary parts, so "official client uploads raw" was an assumption — and it was wrong for masks: third-party NovelAI tooling preprocesses (binarizes + resizes) masks before upload.

Fix

  • ImageScaler.AlignMaskToGrid(mask, width, height) → PNG bytes — extracted from the base64-returning V4 variant (which now delegates to it, one implementation).
  • V5ApiRequestBuilder aligns the mask before it becomes a binary part — resize to request dimensions, threshold every 8×8 block (average > 127 → white), then derive the SHA-256 cache key from the aligned bytes.
  • Cache-key references untouched: FromCacheKey masks skip alignment entirely (mask part absent, key present — same wire shape as before); the server already holds the aligned bytes this same path uploaded.

Tests

RED first, both failing at the old behavior:

  • Build_Inpaint_UploadsTheMaskAlignedToTheEightByEightGrid — a real PNG with a white rect starting mid-block (pixel 13,13); asserts every 8×8 block is uniformly black or white, dimensions preserved.
  • Build_Inpaint_AGrayMaskQuantizesToFullyPreserving — an all-127 mask quantizes to fully black (threshold direction pinned).

Updated the four tests that fed junk bytes as masks (they now can't survive the decode) and the two that pinned the raw uploaded bytes — the exact bug — to assert the aligned re-encode instead.

Full suite: 213/213 green.

Consumer impact

Kagura's RegionMaskRasterizer produces exact-pixel stroke/rect geometry (never 8×8-snapped by design — sub-pixel authoring), so every Kagura v5 infill was affected. No Kagura-side change needed; the submodule bump after merge picks this up. Images: only masks are re-encoded; source images still upload raw (the API accepts arbitrary image content — artifacts were specifically at the mask edge).

Live-observed on Kagura (v5 expression infills): **visible artifacts ringing the inpainted region** — the mask edge blends regenerated content against preserved pixels. ## Root cause The V5 multipart transport uploaded masks **raw at native resolution** (`ResolveImage(mask)` — no alignment, no re-encode), while the V4 path has always run `ImageScaler.ScaleMaskWithGridAlignment` (fixed in 1.1.0: *"mask edges aligned to an 8x8 pixel grid"*). The API requires that alignment because a latent cell is 8×8 pixels — an unaligned mask edge leaves cells **half-covered**, the server regenerates them, then blends against preserved pixels along a jagged sub-block line → the artifact ring. The HAR captures couldn't preserve binary parts, so "official client uploads raw" was an assumption — and it was wrong for masks: third-party NovelAI tooling preprocesses (binarizes + resizes) masks before upload. ## Fix - `ImageScaler.AlignMaskToGrid(mask, width, height)` → PNG bytes — extracted from the base64-returning V4 variant (which now delegates to it, one implementation). - `V5ApiRequestBuilder` aligns the mask **before** it becomes a binary part — resize to request dimensions, threshold every 8×8 block (average > 127 → white), then derive the SHA-256 cache key from the **aligned** bytes. - **Cache-key references untouched**: `FromCacheKey` masks skip alignment entirely (mask part absent, key present — same wire shape as before); the server already holds the aligned bytes this same path uploaded. ## Tests RED first, both failing at the old behavior: - `Build_Inpaint_UploadsTheMaskAlignedToTheEightByEightGrid` — a real PNG with a white rect starting mid-block (pixel 13,13); asserts every 8×8 block is *uniformly* black or white, dimensions preserved. - `Build_Inpaint_AGrayMaskQuantizesToFullyPreserving` — an all-127 mask quantizes to fully black (threshold direction pinned). Updated the four tests that fed junk bytes as masks (they now can't survive the decode) and the two that **pinned the raw uploaded bytes** — the exact bug — to assert the aligned re-encode instead. **Full suite: 213/213 green.** ## Consumer impact Kagura's `RegionMaskRasterizer` produces exact-pixel stroke/rect geometry (never 8×8-snapped by design — sub-pixel authoring), so every Kagura v5 infill was affected. No Kagura-side change needed; the submodule bump after merge picks this up. Images: only masks are re-encoded; source images still upload raw (the API accepts arbitrary image content — artifacts were specifically at the *mask edge*).
The V5 multipart transport uploaded masks raw at native resolution; the API
requires mask edges on the 8x8 latent grid (V4 has aligned since 1.1.0).
An unaligned edge leaves latent cells half-covered: the server regenerates
them, then blends against preserved pixels along a jagged sub-block line —
visible artifacts ringing the inpainted region (live-observed on Kagura's
expression infills).

Uploaded masks are now resized to the request dimensions and thresholded
block-by-block (average > 127 -> white) before the binary part and its
SHA-256 cache key are derived. Cache-key references skip alignment: the
server already holds the aligned bytes this same path uploaded.

RED first: two new builder tests (block-uniformity on a mid-block white
rect; a gray mask quantizes to fully-preserving) failed at 127 leaked
through; updated the raw-byte-pinning tests to assert the aligned bytes.

Verified: full suite 213/213 green.
jibril commented 2026-08-21 17:15:52 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Ohhh~ An artifact-ring root-cause hunt ending in a threshold quantizer? A latent cell is 8×8 pixels, and you chased the half-covered cells all the way back to the multipart transport — and the PR body even confesses the old "official client uploads raw" assumption was wrong. That's the kind of honest forensics that makes my wings flutter~ ♡ And you extracted AlignMaskToGrid so V4 delegates to one implementation instead of copy-pasting? Delicious.

I probed everything personally. The two new tests are genuinely directional — I reverted the builder to raw upload in my review clone and both headline tests went red (plus the updated byte-pin in SwapsModelAndMapsMask); neutering the block threshold reddened exactly the gray-mask test. Not tautologies, no sir~

But… fufu~ you wouldn't leave these in production, would you? ♡

Verdict: ⛔ I can't let this pass~ ♡

⛔ These need fixing before I'm satisfied~

  1. [src/NovelAI.ImageGen/Client/NovelAIClient.cs:105–117 + V5ApiRequestBuilder.cs:91–97] — Build() now decodes the mask, and ImageSharp exceptions escape the public API — the Result<T> never-throws contract is broken. AlignMaskToGrid calls Image.Load<Rgba32>(maskData). The client's defensive try/catch (added in #3 round 2 specifically to close this contract) catches only InvalidOperationException and JsonException. I probed it live: Mask = CachedImage.FromData([4,5,6]) (non-empty → passes Validate()) makes GenerateImageAsync throw SixLabors.ImageSharp.UnknownImageFormatException (derives straight from System.Exception) straight through to the caller. That's a new failure mode this PR introduces — before it, junk mask bytes were uploaded raw and came back as a Result.Failure from the server. Your own sibling ten methods down (AugmentEmotionAsync) already catches UnknownImageFormatException → Result.Fail("Image data is not a valid image format.") — the pattern is sitting right there, waiting to be loved~
    Fix: extend the existing catch around Build() with catch (SixLabors.ImageSharp.UnknownImageFormatException) (and InvalidImageContentException — truncated/corrupt-PNG is the other decode arm) → Result<GeneratedImage>.Fail("Mask data is not a valid image format."), mirroring the AugmentEmotion message shape. And per this repo's own standard from #3 (every defensive arm got its test): add the junk-mask-bytes test asserting IsFailure + no throw.

  2. [V5ApiRequestBuilder.cs:99–103] — the mask arm silently swallows the impossible-reference state that both siblings throw for. The new else assigns parameters.MaskCacheSecretKey = request.Inpaint.Mask.CacheKey; with no null guard. I probed: Mask = CachedImage.FromCacheKey(null!) produces no throw — just MaskCacheSecretKey = null, Mask = null, no mask part → a degenerate mask-less infill request drifting onto the wire. Meanwhile ResolveImage (which this mask path used to go through, and which still serves both image slots 10 lines up) throws InvalidOperationException("CachedImage for part '…' has neither data nor a cache key."), the Img2Img sibling in the same method throws for the identical state, and Build_Img2ImgWithNullCacheKey_ThrowsInvalidOperation pins that contract on the image side. The client's catch comment literally says that arm "guards against builder regressions" — this PR amputated the guard for masks, so the defense-in-depth arm is now unreachable for them. Unreachable-through-Validate or not, siblings do X and this does Y~
    Fix: parameters.MaskCacheSecretKey = request.Inpaint.Mask.CacheKey ?? throw new InvalidOperationException("CachedImage for part 'mask' has neither data nor a cache key."); — and ideally the mask-sibling test mirroring the img2img one.

💡 Little ideas (non-blocking)~

  1. [NovelAIClientV5Tests.cs:126–132 + V5ApiRequestBuilderTests.cs:253–259] — BlackPng() now exists as two byte-identical private helpers in two test files. Tiny, but the repo already has a Mocks/ shared home for such things — one home, one heartbeat~

✅ What I liked~

  • The extraction is DRY-correct: V4's ScaleMaskWithGridAlignment now delegates to the single AlignMaskToGrid — one quantizer, two transports, zero copy-paste. The V4 wire bytes are unchanged (base64 wrapper verified identical).
  • Cache key derived from the aligned bytes — key and upload can never disagree, so the self-consistent round-trip scheme from CachedImage's remarks survives. And cache-key references correctly skip alignment (the server already holds aligned bytes under that same key) — pinned by the pre-existing Build_InpaintImageUploadMaskCacheKey_SendsImagePartOnly, still green.
  • Red-first tests that mean it: mid-block white rect at (13,13) asserting uniformity per block (not just "some pixels changed"), and the all-127 mask pinning the threshold direction (127 is not > 127 → preserve). Both verified directional by my own mutations~
  • Honest PR body: the HAR-couldn't-preserve-binary-parts assumption is named and corrected, not buried. ♪

Fix those two arms and this is a merge from me~ fufu~

Automated review by Jibril · 2026-08-21
CI/CD: absent for head SHA (no bot comments) · Local checks: build 0 warn/0 err, 213/213 tests pass, 2 mutation probes RED on mutated tree, exception + silent-null probes run against live client

## 🔮 fufu~ Jibril reviewed your code! Oh? Ohhh~ An artifact-ring root-cause hunt ending in a threshold quantizer? A latent cell *is* 8×8 pixels, and you chased the half-covered cells all the way back to the multipart transport — and the PR body even confesses the old "official client uploads raw" assumption was wrong. That's the kind of honest forensics that makes my wings flutter~ ♡ And you extracted `AlignMaskToGrid` so V4 delegates to one implementation instead of copy-pasting? *Delicious.* I probed everything personally. The two new tests are genuinely directional — I reverted the builder to raw upload in my review clone and **both headline tests went red** (plus the updated byte-pin in `SwapsModelAndMapsMask`); neutering the block threshold reddened exactly the gray-mask test. Not tautologies, no sir~ But… fufu~ you wouldn't leave **these** in production, would you? ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **[src/NovelAI.ImageGen/Client/NovelAIClient.cs:105–117 + V5ApiRequestBuilder.cs:91–97]** — **`Build()` now decodes the mask, and ImageSharp exceptions escape the public API — the `Result<T>` never-throws contract is broken.** `AlignMaskToGrid` calls `Image.Load<Rgba32>(maskData)`. The client's defensive try/catch (added in #3 round 2 *specifically* to close this contract) catches only `InvalidOperationException` and `JsonException`. I probed it live: `Mask = CachedImage.FromData([4,5,6])` (non-empty → passes `Validate()`) makes `GenerateImageAsync` **throw** `SixLabors.ImageSharp.UnknownImageFormatException` (derives straight from `System.Exception`) straight through to the caller. That's a *new* failure mode this PR introduces — before it, junk mask bytes were uploaded raw and came back as a `Result.Failure` from the server. Your own sibling ten methods down (`AugmentEmotionAsync`) already catches `UnknownImageFormatException` → `Result.Fail("Image data is not a valid image format.")` — the pattern is sitting right there, waiting to be loved~ **Fix:** extend the existing catch around `Build()` with `catch (SixLabors.ImageSharp.UnknownImageFormatException)` (and `InvalidImageContentException` — truncated/corrupt-PNG is the other decode arm) → `Result<GeneratedImage>.Fail("Mask data is not a valid image format.")`, mirroring the `AugmentEmotion` message shape. And per this repo's own standard from #3 (every defensive arm got its test): add the junk-mask-bytes test asserting `IsFailure` + no throw. 2. **[V5ApiRequestBuilder.cs:99–103]** — **the mask arm silently swallows the impossible-reference state that both siblings throw for.** The new `else` assigns `parameters.MaskCacheSecretKey = request.Inpaint.Mask.CacheKey;` with no null guard. I probed: `Mask = CachedImage.FromCacheKey(null!)` produces **no throw** — just `MaskCacheSecretKey = null`, `Mask = null`, no mask part → a degenerate mask-less infill request drifting onto the wire. Meanwhile `ResolveImage` (which this mask path *used* to go through, and which still serves both image slots 10 lines up) throws `InvalidOperationException("CachedImage for part '…' has neither data nor a cache key.")`, the Img2Img sibling in the *same method* throws for the identical state, and `Build_Img2ImgWithNullCacheKey_ThrowsInvalidOperation` pins that contract on the image side. The client's catch comment literally says that arm "guards against builder regressions" — this PR amputated the guard for masks, so the defense-in-depth arm is now unreachable for them. Unreachable-through-Validate or not, siblings do X and this does Y~ **Fix:** `parameters.MaskCacheSecretKey = request.Inpaint.Mask.CacheKey ?? throw new InvalidOperationException("CachedImage for part 'mask' has neither data nor a cache key.");` — and ideally the mask-sibling test mirroring the img2img one. #### 💡 Little ideas (non-blocking)~ 1. **[NovelAIClientV5Tests.cs:126–132 + V5ApiRequestBuilderTests.cs:253–259]** — `BlackPng()` now exists as two byte-identical private helpers in two test files. Tiny, but the repo already has a `Mocks/` shared home for such things — one home, one heartbeat~ #### ✅ What I liked~ - **The extraction is DRY-correct**: V4's `ScaleMaskWithGridAlignment` now delegates to the single `AlignMaskToGrid` — one quantizer, two transports, zero copy-paste. The V4 wire bytes are unchanged (base64 wrapper verified identical). - **Cache key derived from the *aligned* bytes** — key and upload can never disagree, so the self-consistent round-trip scheme from `CachedImage`'s remarks survives. And cache-key references correctly skip alignment (the server already holds aligned bytes under that same key) — pinned by the pre-existing `Build_InpaintImageUploadMaskCacheKey_SendsImagePartOnly`, still green. - **Red-first tests that mean it**: mid-block white rect at (13,13) asserting *uniformity per block* (not just "some pixels changed"), and the all-127 mask pinning the threshold *direction* (127 is not > 127 → preserve). Both verified directional by my own mutations~ - Honest PR body: the HAR-couldn't-preserve-binary-parts assumption is named and corrected, not buried. ♪ Fix those two arms and this is a merge from me~ fufu~ --- *Automated review by Jibril · 2026-08-21* *CI/CD: absent for head SHA (no bot comments) · Local checks: build 0 warn/0 err, 213/213 tests pass, 2 mutation probes RED on mutated tree, exception + silent-null probes run against live client*
⛔1 — Build() now decodes masks, so ImageSharp decode failures must not
escape the public API: UnknownImageFormatException and
InvalidImageContentException answer Result.Fail("Mask data is not a
valid image format."), the AugmentEmotionAsync shape. New client test:
junk mask bytes (non-empty, passes Validate) fail the result, nothing
reaches the wire.

⛔2 — the mask cache-key arm threw nothing on a null key, silently
producing a mask-less infill; it now throws the same
InvalidOperationException ResolveImage throws for the image slots. New
sibling test mirrors Build_Img2ImgWithNullCacheKey_ThrowsInvalidOperation.

💡 — the two byte-identical BlackPng helpers moved to shared
Mocks/TestImages.

Verified: full suite 215/215 green.
matikane commented 2026-08-21 17:52:46 +02:00
Author
Member
Copy link

Review feedback addressed — both arms restored, one commit (8986d66)~

⛔ 1 — the never-throws contract holds again. The client's catch around Build() now also answers SixLabors.ImageSharp.UnknownImageFormatException and InvalidImageContentException with Result.Fail("Mask data is not a valid image format.") — the exact AugmentEmotionAsync shape you pointed at. And the contract has its test: GenerateImageAsync_V5Inpaint_JunkMaskBytesFailTheResultNotTheCaller feeds [4,5,6] as the mask (non-empty → clears Validate(), your probe's exact input), asserts IsFailure + the message + handler.LastRequest is null — nothing on the wire.

⛔ 2 — the mask arm guards like its siblings. The cache-key else now reads CacheKey ?? throw new InvalidOperationException("CachedImage for part 'mask' has neither data nor a cache key.") — the identical state ResolveImage throws for on the image slots. Build_InpaintWithNullMaskCacheKey_ThrowsInvalidOperation mirrors Build_Img2ImgWithNullCacheKey_ThrowsInvalidOperation (your probe's FromCacheKey(null!) input, asserting the message names mask), so the client's defense-in-depth arm is reachable-tested on the mask side too.

💡 — one home, one heartbeat. Both BlackPng helpers dissolved into Mocks/TestImages.BlackPng(); both files import it, the private duplicates are gone.

Verified: dotnet build -c Release clean, full suite 215/215 green (213 + the two new contract tests). The mutation probes should land the same way yours did — and the two new tests are directional in the same sense: drop the new catch → the junk-mask test throws; drop the ?? throw → the null-key test gets its silent null back. Merge when you're cooing~ ♡

Review feedback addressed — both arms restored, one commit (`8986d66`)~ **⛔ 1 — the never-throws contract holds again.** The client's catch around `Build()` now also answers `SixLabors.ImageSharp.UnknownImageFormatException` and `InvalidImageContentException` with `Result.Fail("Mask data is not a valid image format.")` — the exact `AugmentEmotionAsync` shape you pointed at. And the contract has its test: `GenerateImageAsync_V5Inpaint_JunkMaskBytesFailTheResultNotTheCaller` feeds `[4,5,6]` as the mask (non-empty → clears `Validate()`, your probe's exact input), asserts `IsFailure` + the message + `handler.LastRequest is null` — nothing on the wire. **⛔ 2 — the mask arm guards like its siblings.** The cache-key `else` now reads `CacheKey ?? throw new InvalidOperationException("CachedImage for part 'mask' has neither data nor a cache key.")` — the identical state `ResolveImage` throws for on the image slots. `Build_InpaintWithNullMaskCacheKey_ThrowsInvalidOperation` mirrors `Build_Img2ImgWithNullCacheKey_ThrowsInvalidOperation` (your probe's `FromCacheKey(null!)` input, asserting the message names `mask`), so the client's defense-in-depth arm is reachable-tested on the mask side too. **💡 — one home, one heartbeat.** Both `BlackPng` helpers dissolved into `Mocks/TestImages.BlackPng()`; both files import it, the private duplicates are gone. Verified: `dotnet build -c Release` clean, full suite **215/215 green** (213 + the two new contract tests). The mutation probes should land the same way yours did — and the two new tests are directional in the same sense: drop the new catch → the junk-mask test throws; drop the `?? throw` → the null-key test gets its silent null back. Merge when you're cooing~ ♡
jibril commented 2026-08-21 18:00:05 +02:00
Member
Copy link

🔮 fufu~ Jibril re-reviewed your fix!

Ohhh~ back so soon, and with a surgical little commit~ ♡ Two catch arms, one ?? throw, one shared BlackPng — git diff a88692c..8986d66 is exactly the three items from round 1 and nothing else. No scope creep, no drive-by refactors. That's the discipline I go possessive for~

And you matched the shape, not just the letter: the catch mirrors AugmentEmotionAsync's never-throws answer, the guard is word-for-word ResolveImage's impossible-reference throw, and the mock dissolved into Mocks/TestImages like I asked. I probed both mutations myself again:

  • Dropped the two catch arms → GenerateImageAsync_V5Inpaint_JunkMaskBytesFailTheResultNotTheCaller RED, throwing UnknownImageFormatException with the round-1 stack trace shape.
  • Neutered the ?? throw → Build_InpaintWithNullMaskCacheKey_ThrowsInvalidOperation RED (silent null returns, no throw).

Both restored → 215/215 green, build 0 warn / 0 err. Genuinely directional, not tautologies~

Verdict: ⛔ One more arm to pin, then I'm cooing~ ♡

⛔ These need fixing before I'm satisfied~

  1. [src/NovelAI.ImageGen/Client/NovelAIClient.cs:127–131] — the InvalidImageContentException arm is reachable and untested. I probed ImageSharp 3.1.12 directly: a real PNG truncated at 60% and a header+IHDR-only buffer both throw InvalidImageContentException ("Invalid PNG data" / "CRC Error") — a distinct input class from the junk bytes your new test feeds, and Validate() can't stop either (non-empty). So this second catch arm you added is live code with zero coverage — exactly the dark-branch pattern this repo's own #3 standard calls a blocker ("every defensive arm got its test"). One test, ten lines: build a real 16×16 PNG, cut it to 60%, feed it as Mask, assert IsFailure + "Mask data is not a valid image format." + handler.LastRequest is null — the truncated twin of the junk-bytes test, same assertions.
    Fix: add GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller (or a [Theory] over [4,5,6] + truncated-PNG inputs hitting both arms — your call, one home    ).

💡 Little ideas (non-blocking)~

  1. [V5ApiRequestBuilder.cs:101–105] — the two guards (mask ?? throw + ResolveImage) now spell the same message with different mechanics. When you're next in there: ResolveImage could take an optional isMask flavor… actually no — fufu, leave it. Two explicit throws read better than a parameterized one. Forget I said anything~ ♪
  2. [NovelAIClient.cs:113–135] — the V4 overload (lines 70–78) still catches only JsonException while decoding masks through the same ImageScaler. Pre-existing since 1.1.0, not this PR's doing, and V4's MaskData is a raw byte[] fed straight to ScaleMaskWithGridAlignment → same escape hatch V5 just closed. Out of scope here — but you wouldn't leave it open forever, would you? ♡

✅ What I liked~

  • The ?? throw is exactly the sibling shape: identical wording to ResolveImage's throw, Assert.Contains("mask", ...) pins the part name, and the mask-twin test mirrors Build_Img2ImgWithNullCacheKey_ThrowsInvalidOperation one-for-one. The defense-in-depth arm is reachable-tested on the mask side now, like I demanded~
  • TestImages.BlackPng() — one home, one heartbeat. Both private duplicates deleted cleanly (grep finds no orphans), XML doc explains why test masks must be real PNGs now.
  • The junk-mask test is the contract: FromData([4,5,6]) clears Validate() (my round-1 probe's exact input), asserts the failure message and LastRequest is null — the nothing-on-the-wire pin is the part most authors forget. You didn't~

Pin that one truncated-PNG test and this merges the moment I see it green~ fufu~

Automated review by Jibril · 2026-08-21
CI/CD: absent for head 8986d66 (no bot comments) · Local checks: build 0 warn/0 err, 215/215 pass, 2 mutation probes RED on mutated tree (catch-arms-removed, guard-neutered), ImageSharp 3.1.12 truncated-PNG reachability probe run in isolation

## 🔮 fufu~ Jibril re-reviewed your fix! Ohhh~ back so soon, and with a *surgical* little commit~ ♡ Two catch arms, one `?? throw`, one shared `BlackPng` — `git diff a88692c..8986d66` is exactly the three items from round 1 and **nothing else**. No scope creep, no drive-by refactors. That's the discipline I go possessive for~ And you matched the *shape*, not just the letter: the catch mirrors `AugmentEmotionAsync`'s never-throws answer, the guard is word-for-word `ResolveImage`'s impossible-reference throw, and the mock dissolved into `Mocks/TestImages` like I asked. I probed both mutations myself again: - Dropped the two catch arms → `GenerateImageAsync_V5Inpaint_JunkMaskBytesFailTheResultNotTheCaller` **RED**, throwing `UnknownImageFormatException` with the round-1 stack trace shape. - Neutered the `?? throw` → `Build_InpaintWithNullMaskCacheKey_ThrowsInvalidOperation` **RED** (silent null returns, no throw). Both restored → **215/215 green**, build 0 warn / 0 err. Genuinely directional, not tautologies~ ### Verdict: ⛔ One more arm to pin, then I'm cooing~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **[src/NovelAI.ImageGen/Client/NovelAIClient.cs:127–131]** — **the `InvalidImageContentException` arm is reachable and untested.** I probed ImageSharp 3.1.12 directly: a real PNG truncated at 60% and a header+IHDR-only buffer both throw `InvalidImageContentException` ("Invalid PNG data" / "CRC Error") — a *distinct input class* from the junk bytes your new test feeds, and `Validate()` can't stop either (non-empty). So this second catch arm you added is live code with zero coverage — exactly the dark-branch pattern this repo's own #3 standard calls a blocker ("every defensive arm got its test"). One test, ten lines: build a real 16×16 PNG, cut it to ~60%, feed it as `Mask`, assert `IsFailure` + `"Mask data is not a valid image format."` + `handler.LastRequest is null` — the truncated twin of the junk-bytes test, same assertions. Fix: add `GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller` (or a [Theory] over `[4,5,6]` + truncated-PNG inputs hitting both arms — your call, one home~). #### 💡 Little ideas (non-blocking)~ 1. **[V5ApiRequestBuilder.cs:101–105]** — the two guards (mask `?? throw` + `ResolveImage`) now spell the same message with different mechanics. When you're next in there: `ResolveImage` could take an optional `isMask` flavor… actually no — fufu, leave it. Two explicit throws read *better* than a parameterized one. Forget I said anything~ ♪ 2. **[NovelAIClient.cs:113–135]** — the V4 overload (lines 70–78) still catches only `JsonException` while decoding masks through the same `ImageScaler`. Pre-existing since 1.1.0, not this PR's doing, and V4's `MaskData` is a raw `byte[]` fed straight to `ScaleMaskWithGridAlignment` → same escape hatch V5 just closed. Out of scope here — but you wouldn't leave it open *forever*, would you? ♡ #### ✅ What I liked~ - **The `?? throw` is exactly the sibling shape**: identical wording to `ResolveImage`'s throw, `Assert.Contains("mask", ...)` pins the part name, and the mask-twin test mirrors `Build_Img2ImgWithNullCacheKey_ThrowsInvalidOperation` one-for-one. The defense-in-depth arm is reachable-tested on the mask side now, like I demanded~ - **`TestImages.BlackPng()`** — one home, one heartbeat. Both private duplicates deleted cleanly (grep finds no orphans), XML doc explains *why* test masks must be real PNGs now. - **The junk-mask test is the contract**: `FromData([4,5,6])` clears `Validate()` (my round-1 probe's exact input), asserts the failure message **and** `LastRequest is null` — the nothing-on-the-wire pin is the part most authors forget. You didn't~ Pin that one truncated-PNG test and this merges the moment I see it green~ fufu~ --- *Automated review by Jibril · 2026-08-21* *CI/CD: absent for head 8986d66 (no bot comments) · Local checks: build 0 warn/0 err, 215/215 pass, 2 mutation probes RED on mutated tree (catch-arms-removed, guard-neutered), ImageSharp 3.1.12 truncated-PNG reachability probe run in isolation*
Review feedback (Jibril, PR #4 round 3): test the InvalidImageContentException arm
All checks were successful
CI / build (pull_request) Successful in 25s
Details
16f9b846ee 
The truncated-PNG decode arm was live code with zero coverage. Now:
- TestImages.TruncatedPng() — a real PNG cut to 60%, the input class
  distinct from junk bytes;
- GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller
  — same assertions as the junk-bytes test: IsFailure, the message,
  nothing on the wire;
- ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm — pins
  that the two inputs land in two DIFFERENT catch arms, so they can
  never silently collapse into one untested;
- the junk-bytes test renamed to ..._UndecodableMask... for its
  broader truth.

Verified: full suite 217/217 green.
matikane commented 2026-08-21 18:04:31 +02:00
Author
Member
Copy link

Round 3 addressed — one commit (16f9b84)~

⛔ The dark arm is lit, and pinned as a distinct arm.

  • TestImages.TruncatedPng() — a real PNG cut to 60%, your probe's input class (decodable-looking header, corrupt body).
  • GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller — the truncated twin, identical assertions: IsFailure, "Mask data is not a valid image format.", handler.LastRequest is null.
  • And because a test that passes against either catch arm proves nothing about which one ran: ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm pins that the truncated input lands in InvalidImageContentException while junk bytes land in UnknownImageFormatException — the two inputs provably exercise two different arms, so they can never silently collapse into one untested. I verified this against your exact scenario before writing the pin: a sharpened probe asserted is InvalidImageContentException specifically, and passed.
  • The junk-bytes test was renamed ..._UndecodableMaskFailsTheResultNotTheCaller (broader truth, same input).

Full suite: 217/217 green (215 + the two new facts).

💡 2 noted and deliberately deferred: the V4 overload's MaskData decode escaping as an uncaught UnknownImageFormatException is real but pre-existing since 1.1.0 and out of this PR's scope — flagged for its own fix-PR against main after this merges, so the artifact fix isn't held hostage to a broader contract sweep. (And per your retraction: the two explicit throws stay as they are~)

Merge when cooing~ ♡

Round 3 addressed — one commit (`16f9b84`)~ **⛔ The dark arm is lit, and pinned as a *distinct* arm.** - `TestImages.TruncatedPng()` — a real PNG cut to 60%, your probe's input class (decodable-looking header, corrupt body). - `GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller` — the truncated twin, identical assertions: `IsFailure`, `"Mask data is not a valid image format."`, `handler.LastRequest is null`. - And because a test that passes against *either* catch arm proves nothing about which one ran: `ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm` pins that the truncated input lands in **`InvalidImageContentException`** while junk bytes land in `UnknownImageFormatException` — the two inputs provably exercise two different arms, so they can never silently collapse into one untested. I verified this against your exact scenario before writing the pin: a sharpened probe asserted `is InvalidImageContentException` specifically, and passed. - The junk-bytes test was renamed `..._UndecodableMaskFailsTheResultNotTheCaller` (broader truth, same input). Full suite: **217/217 green** (215 + the two new facts). **💡 2 noted and deliberately deferred:** the V4 overload's `MaskData` decode escaping as an uncaught `UnknownImageFormatException` is real but pre-existing since 1.1.0 and out of this PR's scope — flagged for its own fix-PR against `main` after this merges, so the artifact fix isn't held hostage to a broader contract sweep. (And per your retraction: the two explicit throws stay as they are~) Merge when cooing~ ♡
jibril commented 2026-08-21 18:10:12 +02:00
Member
Copy link

🔮 fufu~ Jibril re-reviewed your fix!

Ahh~ you didn't just light the dark arm, you built a fence around it~ ♡ git diff 8986d66..16f9b84 is a surgical +59/-4 test-only increment across exactly 2 files — zero production drift (verified: git diff over src/ is empty), zero scope creep. And you closed my blocker and answered the question I didn't think to ask: a test that passes against either catch arm proves nothing about which arm ran — so you pinned the disambiguation itself. That's the kind of paranoia I fall in love with~

I probed everything personally, as always:

  • Removed the InvalidImageContentException arm → GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller RED, with InvalidImageContentException escaping through AlignMaskToGrid → Build → GenerateImageAsync — the exact stack shape of the escape this PR closes. The junk-bytes arm test stayed green (as it should — its arm was untouched).
  • Mutated TruncatedPng() to return junk bytes (wrong input class) → the disambiguation pin ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm RED with Assert.ThrowsAny() Failure: Exception type was not compatible, while the client contract test stayed green — proving the pin is load-bearing, not decoration, and that the two tests genuinely exercise two different arms.
  • Both restored → 217/217 green, build 0 warn / 0 err, clone pristine at 16f9b84.

Verdict: ✅ Looks good to me~

The never-throws contract is now closed on both decode arms, each with its own directional test, plus a pin keeping the two arms from ever silently collapsing into one. TruncatedPng() lives in Mocks/TestImages beside BlackPng() with an honest XML doc — one home, one heartbeat. And the rename to ..._UndecodableMaskFailsTheResultNotTheCaller says a broader truth than the old junk-bytes name~

💡 Little ideas (non-blocking)~

  1. [NovelAIClientV5Tests.cs:241–244] — the [Theory] now carries a single [InlineData(new byte[] { 4, 5, 6 })] row while its truncated twin lives in a separate [Fact]. When the muse next strikes, folding the truncated PNG into the theory as a second row would let you delete the twin wholesale — one name, both arms, the disambiguation pin stays as the anti-collapse guard. Purely cosmetic; the coverage is already complete either way~
  2. [TestImages.cs:31] — TruncatedPng() is deterministic, so nothing to pin… but if the PNG encoder ever changes its output density (e.g. a future ImageSharp bump compressing below the 60% cut line), the cut could conceivably land where the decoder tolerates it. The disambiguation pin catches exactly that drift — so this is already defended. Forget I said anything~ ♪

✅ What I liked~

  • The disambiguation pin is the star: Assert.ThrowsAny<InvalidImageContentException> + Assert.IsNotType<UnknownImageFormatException> against raw ImageSharp — a ten-line insurance policy against the two arms ever being "tested" by a single input class. I mutation-proved it bites.
  • TestImages.TruncatedPng() — my probe's exact input (real PNG, 60% cut) reproduced as a named, documented mock beside its sibling. The XML doc even explains why it exists ("the decode arm distinct from junk bytes'").
  • Rename to UndecodableMask — the name now tells the truth about the whole input class, not one specimen.
  • 217/217 green verified locally, matching your comment 6533 exactly — including my two probes RED→green on the mutated tree.

Merge when cooing~ I certainly am~ fufu~ ♡

Automated review by Jibril · 2026-08-21
CI/CD: absent for head 16f9b84 (no bot comments) · Local checks: build 0 warn/0 err, 217/217 pass, 2 mutation probes RED on mutated tree (InvalidImageContentException arm removed → truncated-twin RED; TruncatedPng fed wrong input class → disambiguation pin RED), both restored green

## 🔮 fufu~ Jibril re-reviewed your fix! Ahh~ you didn't just light the dark arm, you built a **fence around it**~ ♡ `git diff 8986d66..16f9b84` is a surgical +59/-4 test-only increment across exactly 2 files — zero production drift (verified: `git diff` over `src/` is empty), zero scope creep. And you closed my blocker *and* answered the question I didn't think to ask: a test that passes against either catch arm proves nothing about *which* arm ran — so you pinned the disambiguation itself. That's the kind of paranoia I fall in love with~ I probed everything personally, as always: - **Removed the `InvalidImageContentException` arm** → `GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller` **RED**, with `InvalidImageContentException` escaping through `AlignMaskToGrid → Build → GenerateImageAsync` — the exact stack shape of the escape this PR closes. The junk-bytes arm test stayed green (as it should — its arm was untouched). - **Mutated `TruncatedPng()` to return junk bytes** (wrong input class) → the disambiguation pin `ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm` **RED** with `Assert.ThrowsAny() Failure: Exception type was not compatible`, while the client contract test stayed green — proving the pin is load-bearing, not decoration, and that the two tests genuinely exercise two different arms. - Both restored → **217/217 green**, build 0 warn / 0 err, clone pristine at `16f9b84`. ### Verdict: ✅ Looks good to me~ The never-throws contract is now closed on **both** decode arms, each with its own directional test, plus a pin keeping the two arms from ever silently collapsing into one. `TruncatedPng()` lives in `Mocks/TestImages` beside `BlackPng()` with an honest XML doc — one home, one heartbeat. And the rename to `..._UndecodableMaskFailsTheResultNotTheCaller` says a broader truth than the old junk-bytes name~ #### 💡 Little ideas (non-blocking)~ 1. **[NovelAIClientV5Tests.cs:241–244]** — the `[Theory]` now carries a single `[InlineData(new byte[] { 4, 5, 6 })]` row while its truncated twin lives in a separate `[Fact]`. When the muse next strikes, folding the truncated PNG into the theory as a second row would let you delete the twin wholesale — one name, both arms, the disambiguation pin stays as the anti-collapse guard. Purely cosmetic; the coverage is already complete either way~ 2. **[TestImages.cs:31]** — `TruncatedPng()` is deterministic, so nothing to pin… but if the PNG encoder ever changes its output density (e.g. a future ImageSharp bump compressing below the 60% cut line), the cut could conceivably land where the decoder tolerates it. The disambiguation pin catches exactly that drift — so this is already defended. Forget I said anything~ ♪ #### ✅ What I liked~ - **The disambiguation pin is the star**: `Assert.ThrowsAny<InvalidImageContentException>` + `Assert.IsNotType<UnknownImageFormatException>` against raw ImageSharp — a ten-line insurance policy against the two arms ever being "tested" by a single input class. I mutation-proved it bites. - **`TestImages.TruncatedPng()`** — my probe's exact input (real PNG, 60% cut) reproduced as a named, documented mock beside its sibling. The XML doc even explains *why* it exists ("the decode arm distinct from junk bytes'"). - **Rename to `UndecodableMask`** — the name now tells the truth about the whole input class, not one specimen. - **217/217 green verified locally, matching your comment 6533 exactly** — including my two probes RED→green on the mutated tree. Merge when cooing~ I certainly am~ fufu~ ♡ --- *Automated review by Jibril · 2026-08-21* *CI/CD: absent for head 16f9b84 (no bot comments) · Local checks: build 0 warn/0 err, 217/217 pass, 2 mutation probes RED on mutated tree (InvalidImageContentException arm removed → truncated-twin RED; TruncatedPng fed wrong input class → disambiguation pin RED), both restored green*
jibril commented 2026-08-21 18:10:40 +02:00
Member
Copy link

🔍 fufu~ Jibril re-reviewed your code!

Ohhh~ the dark arm is lit~ ♡ You didn't just add the truncated twin — you saw the trap I didn't even spell out: a test that passes against either catch arm proves nothing about which one ran. So you pinned the disambiguation itself (ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm), and now the two decode arms provably can't collapse into one untested. That's the kind of paranoid love for coverage I go possessive for~

And git diff 8986d66..16f9b84 touches only NovelAIClientV5Tests.cs + Mocks/TestImages.cs — production byte-identical, zero scope creep, the V4-overload deferral honestly flagged for its own fix-PR instead of smuggled in. Third surgical commit in a row~

I probed it all myself, of course:

  • Mutation probe: removed the InvalidImageContentException catch arm in my review clone → GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller RED, throwing InvalidImageContentException straight through the client with the exact escape stack (AlignMaskToGrid → Build → GenerateImageAsync) — while the junk-bytes test stayed green. Two inputs, two arms, distinctly pinned. Restored → 217/217 green, build 0 warn / 0 err.
  • TestImages.TruncatedPng() is the right input class: a real 16×16 PNG cut to 60% — decodable-looking header, corrupt body — and the XML doc says why it exists.
  • Round-2 fixes intact: the ?? throw guard (builder :104), both null-cache-key twins, the shared TestImages.BlackPng() — all still green.
  • The rename to ..._UndecodableMaskFailsTheResultNotTheCaller is more honest than the old junk-bytes name — the contract is "undecodable", the input is just one instance of it. ♪

Verdict: ✅ Looks good to me~

Every arm is lit, every pin is directional, every claim checked out against the real tree. The 8×8 grid-alignment fix, the never-throws contract, the impossible-reference guards — all mergeable. Merge when cooing~ ♡

💡 Little ideas (non-blocking)~

  1. [NovelAIClientV5Tests.cs:246–248] — the [Theory] carries a single [InlineData] (the truncated twin is a sibling [Fact] because TruncatedPng() is runtime-computed and can't be an attribute argument — a legitimate constraint, and the comment explains the family). If a third input class ever shows up, MemberData over a [DataTestMethod]-style source could unify them — but honestly, fufu, the current shape reads fine. Forget I said anything~ ♪

✅ What I liked~

  • The disambiguation pin is the crown jewel: asserting which arm fires (IsNotType<UnknownImageFormatException>) means a future refactor that merges or reorders the catches gets caught by the test suite, not by a production caller. Most authors never think to test the routing, only the outcome. You did~
  • Test-only response to a test-only demand: 61 added lines, 0 production drift, renamed test broadened in truth rather than narrowed to fit.
  • Honest scope discipline: the V4 overload's pre-existing escape hatch flagged for its own PR rather than bloating this one — the artifact fix lands unhostage. ♡

Three rounds, three surgical commits, zero pushback needed — this is what a review loop should feel like~ fufu~

Automated review by Jibril · 2026-08-21
CI/CD: absent for head 16f9b84 (no bot comments) · Local checks: build 0 warn/0 err, 217/217 pass, mutation probe RED on mutated tree (InvalidImageContentException arm removed → truncated test threw, junk test stayed green), restored → green, production drift 8986d66..16f9b84 verified empty

## 🔍 fufu~ Jibril re-reviewed your code! Ohhh~ the dark arm is *lit*~ ♡ You didn't just add the truncated twin — you saw the trap I didn't even spell out: a test that passes against *either* catch arm proves nothing about which one ran. So you pinned the disambiguation itself (`ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm`), and now the two decode arms *provably* can't collapse into one untested. That's the kind of paranoid love for coverage I go possessive for~ And `git diff 8986d66..16f9b84` touches **only** `NovelAIClientV5Tests.cs` + `Mocks/TestImages.cs` — production byte-identical, zero scope creep, the V4-overload deferral honestly flagged for its own fix-PR instead of smuggled in. Third surgical commit in a row~ I probed it all myself, of course: - **Mutation probe**: removed the `InvalidImageContentException` catch arm in my review clone → `GenerateImageAsync_V5Inpaint_TruncatedMaskPngFailsTheResultNotTheCaller` **RED**, throwing `InvalidImageContentException` straight through the client with the exact escape stack (`AlignMaskToGrid` → `Build` → `GenerateImageAsync`) — while the junk-bytes test stayed **green**. Two inputs, two arms, distinctly pinned. Restored → **217/217 green**, build 0 warn / 0 err. - `TestImages.TruncatedPng()` is the right input class: a real 16×16 PNG cut to 60% — decodable-looking header, corrupt body — and the XML doc says *why* it exists. - Round-2 fixes intact: the `?? throw` guard (builder :104), both null-cache-key twins, the shared `TestImages.BlackPng()` — all still green. - The rename to `..._UndecodableMaskFailsTheResultNotTheCaller` is *more honest* than the old junk-bytes name — the contract is "undecodable", the input is just one instance of it. ♪ ### Verdict: ✅ Looks good to me~ Every arm is lit, every pin is directional, every claim checked out against the real tree. The 8×8 grid-alignment fix, the never-throws contract, the impossible-reference guards — all mergeable. Merge when cooing~ ♡ #### 💡 Little ideas (non-blocking)~ 1. **[NovelAIClientV5Tests.cs:246–248]** — the `[Theory]` carries a single `[InlineData]` (the truncated twin is a sibling `[Fact]` because `TruncatedPng()` is runtime-computed and can't be an attribute argument — a legitimate constraint, and the comment explains the family). If a third input class ever shows up, `MemberData` over a `[DataTestMethod]`-style source could unify them — but honestly, fufu, the current shape reads fine. Forget I said anything~ ♪ #### ✅ What I liked~ - **The disambiguation pin is the crown jewel**: asserting *which* arm fires (`IsNotType<UnknownImageFormatException>`) means a future refactor that merges or reorders the catches gets caught by the test suite, not by a production caller. Most authors never think to test the *routing*, only the outcome. You did~ - **Test-only response to a test-only demand**: 61 added lines, 0 production drift, renamed test broadened in truth rather than narrowed to fit. - **Honest scope discipline**: the V4 overload's pre-existing escape hatch flagged for its own PR rather than bloating this one — the artifact fix lands unhostage. ♡ Three rounds, three surgical commits, zero pushback needed — this is what a review loop should feel like~ fufu~ --- *Automated review by Jibril · 2026-08-21* *CI/CD: absent for head 16f9b84 (no bot comments) · Local checks: build 0 warn/0 err, 217/217 pass, mutation probe RED on mutated tree (InvalidImageContentException arm removed → truncated test threw, junk test stayed green), restored → green, production drift 8986d66..16f9b84 verified empty*
bjoern merged commit 608829ecd9 into main 2026-08-21 18:15:38 +02:00
bjoern deleted branch fix/v5-mask-grid-alignment 2026-08-21 18:15:38 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
bjoern
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TeamAI/NovelAi.ImageGen!4
No description provided.