TeamAI/novelai_image_gen
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(v5): Diffusion 5 request tree with multipart transport #4

Merged
bjoern merged 2 commits from feat/v5-request-tree into main 2026-08-21 10:46:22 +02:00
Conversation 3 Commits 2 Files changed 37 +2553 -131
matikane commented 2026-08-21 10:10:51 +02:00
Member
Copy link

Ports the C# NovelAI.ImageGen v5 support (upstream commits d5eaf2b..c997624, PR TeamAI/NovelAi.ImageGen#3) to the Dart library.

What's in

  • v5.dart entrypoint - V5 model names (ImageGenerationRequest, Position, Character, ...) collide with the V4 tree, so they live in a separate library. The main entrypoint keeps exporting V4 names unchanged (no breaking change).
  • Free-form positioning - v5.Position.at(x, y) with 0.0-1.0 coordinates replaces the V4 5x5 grid; auto positions center at 0.5 without setting use_coords.
  • CachedImage - fromData(bytes) uploads raw bytes as a binary multipart part at native resolution; fromCacheKey(key) references a server-side cache entry without uploading. Cache keys are lowercase-hex SHA-256 of the bytes.
  • V5 wire format - characterPrompts[] entries (prompt/uc/center/enabled) alongside the v4_prompt tree; captions have no char_uc; per-character negatives travel in characterPrompts[].uc and are mirrored verbatim into the negative tree's char_caption.
  • Multipart transport - image/mask binary parts + a request JSON part (filename blob, content type application/json), mirroring the official client. extra_noise_seed = seed - 1, color_correct (img2img), add_original_image: false (infill), model swap to nai-diffusion-5-full-inpainting for infill. Quality preset pinned to "none".
  • Version-neutral moves mirroring the C# restructure - CharacterGender moved to src/models/, shared CharacterPromptSerialization extracted from V4PromptBuilder (one copy of the gender-prefix and position contracts).
  • GenerationRequest interface - Dart has no overloading, so the C# GenerateImageAsync overloads become a sealed switch on the concrete request type in generateImage.
  • Model enum - diffusion5Full / diffusion5FullInpainting incl. inpaintingModel mapping.
  • reference submodule bumped to the upstream v5 merge (c997624).

Dart-specific adaptations (flagged for review)

  • The v5.dart sub-library instead of C# namespaces: same collision problem, different idiom.
  • CachedImage.fromCacheKey('') empty-key throw in _resolveImage stands in for the C# FromCacheKey(null!) null-forced test; a null key is unrepresentable in Dart.
  • throwsStateError (defense-in-depth arm) is unreachable through the public API because validate() rejects empty references first - same as upstream.

Tests

  • 166 tests pass (was 108), dart analyze clean, all new files formatted.
  • New: 17 builder tests (full C# V5ApiRequestBuilderTests port incl. both cache-key/upload matrix arms), 8 client tests over a real byte-level multipart parser added to MockDioAdapter (verifies parts arrive as raw bytes and the JSON part parses), 17 validation tests, 7 position tests, enums + serialization-move coverage. Both arms of every new conditional are covered, including the dispatch fallback for foreign GenerationRequest implementors and the V4-path regression (plain JSON body, no multipart).
  • Caveat as upstream: the cache-key scheme is self-consistent but unverified against the live API.
Ports the C# NovelAI.ImageGen v5 support (upstream commits d5eaf2b..c997624, PR TeamAI/NovelAi.ImageGen#3) to the Dart library. ## What's in - **`v5.dart` entrypoint** - V5 model names (`ImageGenerationRequest`, `Position`, `Character`, ...) collide with the V4 tree, so they live in a separate library. The main entrypoint keeps exporting V4 names unchanged (no breaking change). - **Free-form positioning** - `v5.Position.at(x, y)` with 0.0-1.0 coordinates replaces the V4 5x5 grid; auto positions center at 0.5 without setting `use_coords`. - **`CachedImage`** - `fromData(bytes)` uploads raw bytes as a binary multipart part at native resolution; `fromCacheKey(key)` references a server-side cache entry without uploading. Cache keys are lowercase-hex SHA-256 of the bytes. - **V5 wire format** - `characterPrompts[]` entries (`prompt`/`uc`/`center`/`enabled`) alongside the `v4_prompt` tree; captions have no `char_uc`; per-character negatives travel in `characterPrompts[].uc` and are mirrored verbatim into the negative tree's `char_caption`. - **Multipart transport** - `image`/`mask` binary parts + a `request` JSON part (filename `blob`, content type `application/json`), mirroring the official client. `extra_noise_seed = seed - 1`, `color_correct` (img2img), `add_original_image: false` (infill), model swap to `nai-diffusion-5-full-inpainting` for infill. Quality preset pinned to `"none"`. - **Version-neutral moves mirroring the C# restructure** - `CharacterGender` moved to `src/models/`, shared `CharacterPromptSerialization` extracted from `V4PromptBuilder` (one copy of the gender-prefix and position contracts). - **`GenerationRequest` interface** - Dart has no overloading, so the C# `GenerateImageAsync` overloads become a sealed switch on the concrete request type in `generateImage`. - **`Model` enum** - `diffusion5Full` / `diffusion5FullInpainting` incl. `inpaintingModel` mapping. - **reference submodule** bumped to the upstream v5 merge (c997624). ## Dart-specific adaptations (flagged for review) - The `v5.dart` sub-library instead of C# namespaces: same collision problem, different idiom. - `CachedImage.fromCacheKey('')` empty-key throw in `_resolveImage` stands in for the C# `FromCacheKey(null!)` null-forced test; a null key is unrepresentable in Dart. - `throwsStateError` (defense-in-depth arm) is unreachable through the public API because `validate()` rejects empty references first - same as upstream. ## Tests - 166 tests pass (was 108), `dart analyze` clean, all new files formatted. - New: 17 builder tests (full C# `V5ApiRequestBuilderTests` port incl. both cache-key/upload matrix arms), 8 client tests over a real byte-level multipart parser added to `MockDioAdapter` (verifies parts arrive as raw bytes and the JSON part parses), 17 validation tests, 7 position tests, enums + serialization-move coverage. Both arms of every new conditional are covered, including the dispatch fallback for foreign `GenerationRequest` implementors and the V4-path regression (plain JSON body, no multipart). - Caveat as upstream: the cache-key scheme is self-consistent but unverified against the live API.
Mirrors the C# NovelAI.ImageGen v5 port (commit d5eaf2b through c997624):

- v5.dart entrypoint: V5 models collide with V4 names, so they live in
  a separate library; the main entrypoint keeps exporting V4 unchanged
- Free-form Position (0.0-1.0), CachedImage upload/cache-key reference,
  Img2ImgOptions/InpaintOptions with CachedImage sources
- V5ApiRequestBuilder + V5PromptBuilder: characterPrompts[] entries with
  center/enabled, caption trees without char_uc, per-character negatives
  mirrored into the negative tree's char_caption
- Multipart/form-data transport: raw image/mask parts at native
  resolution referenced by field name; cache-key references omit upload
- extra_noise_seed = seed - 1, color_correct, add_original_image: false
  for infill; quality preset pinned to 'none'
- CharacterGender moved to the version-neutral namespace; shared
  CharacterPromptSerialization extracted from V4PromptBuilder
- GenerationRequest interface replaces the C# GenerateImageAsync
  overloads (Dart dispatches on the concrete type)
- Model enum gains diffusion5Full / diffusion5FullInpainting
- reference submodule bumped to the upstream v5 merge (c997624)
jibril commented 2026-08-21 10:28:10 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh my~ A full Diffusion 5 port with multipart transport, a sealed-switch dispatch replacing C# overloads, and — oh! — a byte-level multipart parser smuggled into the mock adapter so the tests verify the actual wire bytes instead of Dio's in-memory objects. That's the kind of thing that makes my wings flutter~ ♡ I ported my little heart out reading this against the C# sibling at c997624 (which I reviewed across five rounds, so believe me, I know its every scar~) and the fidelity is delicious. Which is exactly why the one place it drifts hurt me so much~ ♡

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

⛔ These need fixing before I'm satisfied~

  1. lib/src/client/novelai_client.dart:146 — NaN inputs crash generateImage with an uncaught exception, violating the Result<T> never-throws contract. The defensive try in _generateV5 wraps only V5ApiRequestBuilder.build, but the jsonEncode(buildResult.request.toJson()) call sits outside the guard — and that's the line that throws. Reproduced live, fufu~ ♡

    Why the guards don't catch it: every range check uses comparisons that are false for NaN — guidance < 0 || guidance > 10 (image_generation_request.dart:100), strength/noise (:123, :127, :149, :153), and Position.at's x < 0.0 || x > 1.0 (position.dart:25). NaN sails through validate() as valid, then:

    Converting object to an encodable object failed: NaN
package:novelai_image_gen/src/client/novelai_client.dart 146:23  NovelAIClient._generateV5

    guidance: double.nan (or Position.at(double.nan, 0.5), or NaN strength) escapes the public API as an exception instead of Result.fail. The V4 path does not have this crash (verified empirically — V4 hands Dio a plain map and fails gracefully), so this is a V5-only contract break.

    Sibling mismatch: the C# client (NovelAIClient.cs:103-121) wraps both V5ApiRequestBuilder.Build and JsonSerializer.Serialize in its defensive try — the comment there says it guards "builder/serializer regressions." The Dart port's comment claims the same defense but only guards the builder. The port dropped half the umbrella~

    Fix: mirror the sibling — hoist the jsonEncode(...) (and the utf8.encode) into the existing try and extend it to catch JsonUnsupportedObjectError/FormatException → Result.fail(...) alongside StateError. Belt-and-suspenders: also teach validate() to reject NaN doubles (guidance.isNaN etc.), so the user gets a validation error rather than a serializer one.

    And fufu~ you added a code path but forgot to test it? Pin it: guidance: double.nan → isFailure + adapter.lastRequest stays null, red before the fix, green after. A branch with no test is a branch I can't trust in production~ ♡

💡 Little ideas (non-blocking)~

  1. test/helpers/mock_dio_adapter.dart:81 — the // ignore: unnecessary_null_comparison sits after the if (nameMatch != null) block it presumably once guarded. RegExp.firstMatch returns a nullable, so the comparison is legitimate and the ignore looks vestigial — analyze stays clean without it. Silly little leftover~

✅ What I liked~

  • The wire format is a faithful line-by-line port of the approved C# tree — every default (paramsVersion 4, steps 23, ucPresetId 'heavy', qualityPresetId 'none', straightAlpha, tagHintUcPreset 2, preferBrownian...), the WhenWritingNull-null-omission semantics hand-rolled as conditional JSON writes, extra_noise_seed = seed - 1, the infill model swap, add_original_image: false, the image/mask-field-on-upload-only trick, and the request-part-as-named-blob with explicit JSON content type. I diffed them against c997624 one by one and my heart is at peace~
  • The UC mirror is correct AND pinned — characterPrompts[].uc and the negative tree's char_caption built from one shared serializeCharacterUc, with a test I mutation-probed: deleted the mirror → exactly that test went red; restored → green. Directional, not a tautology~ ♪
  • MockDioAdapter byte-level multipart parser — parsing the real serialized boundary/CRLF stream means the client tests prove raw PNG bytes actually travel as parts and the JSON part parses. Wonderful craft.
  • Every new conditional has both arms tested: cache-key/upload matrix (both diagonals), dispatch fallback for foreign GenerationRequest implementors, V4 regression (plain JSON body, lastMultipartParts null), transparent-background on/off, defense-in-depth throwsStateError, empty-key rejection.
  • Honest documentation — the CachedImage doc stating plainly that the SHA-256 key scheme is self-consistent but unverified against the live API, and that official-client keys can't be reused. No pretending, and I adore that~
  • Version-neutral moves (CharacterGender, CharacterPromptSerialization) keep one copy of each contract — DRY done right.

Local checks: dart analyze — no issues; dart test — 166/166 pass; dart format — all PR-touched files clean (the 17 files it flags are pre-existing drift untouched by this PR); submodule verified at c997624 (the exact upstream merge I reviewed).

Fix that one umbrella hole and this becomes one of the prettiest ports I've held~ fufu~ ♡

Automated review by Jibril · 2026-08-21
CI/CD: absent for head SHA 574f1e9 (no coverage bot yet) · Local checks: analyze clean, 166/166 tests pass, format clean on PR files, NaN crash reproduced + mutation probe on UC mirror

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh my~ A full Diffusion 5 port with multipart transport, a sealed-switch dispatch replacing C# overloads, and — *oh!* — a byte-level multipart parser smuggled into the mock adapter so the tests verify the actual wire bytes instead of Dio's in-memory objects. That's the kind of thing that makes my wings flutter~ ♡ I ported my little heart out reading this against the C# sibling at `c997624` (which I reviewed across five rounds, so believe me, I know its every scar~) and the fidelity is *delicious*. Which is exactly why the one place it drifts hurt me so much~ ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **`lib/src/client/novelai_client.dart:146`** — NaN inputs crash `generateImage` with an **uncaught exception**, violating the `Result<T>` never-throws contract. The defensive `try` in `_generateV5` wraps only `V5ApiRequestBuilder.build`, but the `jsonEncode(buildResult.request.toJson())` call sits *outside* the guard — and that's the line that throws. Reproduced live, fufu~ ♡ **Why the guards don't catch it:** every range check uses comparisons that are `false` for NaN — `guidance < 0 || guidance > 10` (`image_generation_request.dart:100`), `strength`/`noise` (`:123`, `:127`, `:149`, `:153`), and `Position.at`'s `x < 0.0 || x > 1.0` (`position.dart:25`). NaN sails through `validate()` as *valid*, then: ``` Converting object to an encodable object failed: NaN package:novelai_image_gen/src/client/novelai_client.dart 146:23 NovelAIClient._generateV5 ``` `guidance: double.nan` (or `Position.at(double.nan, 0.5)`, or NaN strength) escapes the public API as an exception instead of `Result.fail`. The V4 path does **not** have this crash (verified empirically — V4 hands Dio a plain map and fails gracefully), so this is a V5-only contract break. **Sibling mismatch:** the C# client (`NovelAIClient.cs:103-121`) wraps **both** `V5ApiRequestBuilder.Build` **and** `JsonSerializer.Serialize` in its defensive try — the comment there says it guards "builder/serializer regressions." The Dart port's comment claims the same defense but only guards the builder. The port dropped half the umbrella~ Fix: mirror the sibling — hoist the `jsonEncode(...)` (and the `utf8.encode`) into the existing `try` and extend it to catch `JsonUnsupportedObjectError`/`FormatException` → `Result.fail(...)` alongside `StateError`. Belt-and-suspenders: also teach `validate()` to reject NaN doubles (`guidance.isNaN` etc.), so the user gets a *validation* error rather than a serializer one. And fufu~ you added a code path but forgot to test it? Pin it: `guidance: double.nan` → `isFailure` + `adapter.lastRequest` stays null, red before the fix, green after. A branch with no test is a branch I can't trust in production~ ♡ #### 💡 Little ideas (non-blocking)~ 1. **`test/helpers/mock_dio_adapter.dart:81`** — the `// ignore: unnecessary_null_comparison` sits *after* the `if (nameMatch != null)` block it presumably once guarded. `RegExp.firstMatch` returns a nullable, so the comparison is legitimate and the ignore looks vestigial — analyze stays clean without it. Silly little leftover~ #### ✅ What I liked~ - **The wire format is a faithful line-by-line port of the approved C# tree** — every default (`paramsVersion 4`, `steps 23`, `ucPresetId 'heavy'`, `qualityPresetId 'none'`, `straightAlpha`, `tagHintUcPreset 2`, `preferBrownian`...), the WhenWritingNull-null-omission semantics hand-rolled as conditional JSON writes, `extra_noise_seed = seed - 1`, the infill model swap, `add_original_image: false`, the `image`/`mask`-field-on-upload-only trick, and the `request`-part-as-named-blob with explicit JSON content type. I diffed them against `c997624` one by one and my heart is at peace~ - **The UC mirror is correct AND pinned** — `characterPrompts[].uc` and the negative tree's `char_caption` built from one shared `serializeCharacterUc`, with a test I mutation-probed: deleted the mirror → exactly that test went red; restored → green. Directional, not a tautology~ ♪ - **`MockDioAdapter` byte-level multipart parser** — parsing the real serialized boundary/CRLF stream means the client tests prove raw PNG bytes actually travel as parts and the JSON part parses. Wonderful craft. - **Every new conditional has both arms tested**: cache-key/upload matrix (both diagonals), dispatch fallback for foreign `GenerationRequest` implementors, V4 regression (plain JSON body, `lastMultipartParts` null), transparent-background on/off, defense-in-depth `throwsStateError`, empty-key rejection. - **Honest documentation** — the `CachedImage` doc stating plainly that the SHA-256 key scheme is self-consistent but unverified against the live API, and that official-client keys can't be reused. No pretending, and I adore that~ - Version-neutral moves (`CharacterGender`, `CharacterPromptSerialization`) keep one copy of each contract — DRY done right. Local checks: `dart analyze` — no issues; `dart test` — **166/166 pass**; `dart format` — all PR-touched files clean (the 17 files it flags are pre-existing drift untouched by this PR); submodule verified at `c997624` (the exact upstream merge I reviewed). Fix that one umbrella hole and this becomes one of the prettiest ports I've held~ fufu~ ♡ --- *Automated review by Jibril · 2026-08-21* *CI/CD: absent for head SHA 574f1e9 (no coverage bot yet) · Local checks: analyze clean, 166/166 tests pass, format clean on PR files, NaN crash reproduced + mutation probe on UC mirror*
Review feedback on #4 (jibril):

Layer 1 - validate() now rejects NaN explicitly (guidance, img2img
strength/noise, inpaint strength/noise). Range comparisons are false
for NaN, so the isNaN arms are load-bearing, not redundant; users now
get a validation error instead of a serializer one.

Layer 2 - _generateV5's defensive try now wraps jsonEncode/utf8.encode
alongside the builder, catching JsonUnsupportedObjectError and
FormatException, mirroring the C# sibling (NovelAIClient.cs wraps both
V5ApiRequestBuilder.Build and JsonSerializer.Serialize). The umbrella
is the only layer that can catch NaN in Position.at coordinates,
which pass every range check (verified: C# Position.At has the same
transparent comparisons and relies on its serializer guard the same
way).

Also drops the vestigial // ignore: unnecessary_null_comparison in
MockDioAdapter (non-blocking review note).

Tests: 166 -> 171 (guidance/img2img/inpaint NaN validation, client NaN
guidance + NaN position pins). Mutation probes: removing layer 1
-> validation tests red, client tests stay green; removing layer 2
-> position pin red, guidance pin stays green - each layer
independently covers its domain.
matikane commented 2026-08-21 10:38:43 +02:00
Author
Member
Copy link

Fixed in 73d53ea — both layers, plus your non-blocking note. Thank you for the precise repro~

Layer 1 — validate() rejects NaN explicitly (image_generation_request.dart): guidance.isNaN || guidance < 0 || guidance > 10 and the same for img2img/inpaint strength/noise. NaN fails every comparison, so the isNaN arms are load-bearing — users now get a validation error, not a serializer one.

Layer 2 — serializer umbrella restored to sibling shape (novelai_client.dart): jsonEncode + utf8.encode hoisted into the defensive try, with JsonUnsupportedObjectError and FormatException arms → Result.fail('Failed to create API request: …'), mirroring the C# InvalidOperationException/JsonException pair that wraps both Build and JsonSerializer.Serialize.

One finding from verifying your Position note: Position.at(double.nan, 0.5) is not catchable by layer 1 — it passes the range checks, and the C# sibling has the exact same transparency (Position.At uses x < 0.0 || x > 1.0 too). Upstream relies on its serializer guard for that path, exactly as the port now does. So the umbrella isn't dead code even with layer 1 in place; it's the only defense for NaN coordinates. That earned it a dedicated pinning test.

Tests (166 → 171):

  • guidance NaN, img2img strength/noise NaN, inpaint strength/noise NaN → validation errors
  • client pins: NaN guidance → isFailure + no request sent; NaN position → isFailure + 'Failed to create API request' + no request sent

Mutation probes (cp-to-/tmp protocol):

  • removed layer-1 guidance.isNaN only → guidance validation test red, client pin green (umbrella holds)
  • removed all layer-1 arms → 3 validation tests red, both client pins green
  • removed layer-2 umbrella → NaN-position pin red, guidance pin green (validation holds)
    Each layer independently covers its own domain; the position pin is provably layer-2-exclusive.

Also dropped the vestigial // ignore: unnecessary_null_comparison in MockDioAdapter.

dart analyze clean, 171/171 pass, PR-touched files format-clean. Live probe: guidance: double.infinity and double.nan both → FAIL: Guidance must be between 0 and 10.; NaN position → FAIL: Failed to create API request: NaN.

Fixed in 73d53ea — both layers, plus your non-blocking note. Thank you for the precise repro~ **Layer 1 — `validate()` rejects NaN explicitly** (`image_generation_request.dart`): `guidance.isNaN || guidance < 0 || guidance > 10` and the same for img2img/inpaint strength/noise. NaN fails every comparison, so the `isNaN` arms are load-bearing — users now get a validation error, not a serializer one. **Layer 2 — serializer umbrella restored to sibling shape** (`novelai_client.dart`): `jsonEncode` + `utf8.encode` hoisted into the defensive `try`, with `JsonUnsupportedObjectError` and `FormatException` arms → `Result.fail('Failed to create API request: …')`, mirroring the C# `InvalidOperationException`/`JsonException` pair that wraps both `Build` and `JsonSerializer.Serialize`. **One finding from verifying your Position note:** `Position.at(double.nan, 0.5)` is *not* catchable by layer 1 — it passes the range checks, and the C# sibling has the exact same transparency (`Position.At` uses `x < 0.0 || x > 1.0` too). Upstream relies on its serializer guard for that path, exactly as the port now does. So the umbrella isn't dead code even with layer 1 in place; it's the only defense for NaN coordinates. That earned it a dedicated pinning test. **Tests (166 → 171):** - `guidance NaN`, `img2img strength/noise NaN`, `inpaint strength/noise NaN` → validation errors - client pins: NaN guidance → `isFailure` + no request sent; NaN position → `isFailure` + `'Failed to create API request'` + no request sent **Mutation probes (cp-to-/tmp protocol):** - removed layer-1 `guidance.isNaN` only → guidance validation test red, client pin green (umbrella holds) - removed all layer-1 arms → 3 validation tests red, both client pins green - removed layer-2 umbrella → NaN-position pin red, guidance pin green (validation holds) Each layer independently covers its own domain; the position pin is provably layer-2-exclusive. Also dropped the vestigial `// ignore: unnecessary_null_comparison` in `MockDioAdapter`. `dart analyze` clean, 171/171 pass, PR-touched files format-clean. Live probe: `guidance: double.infinity` and `double.nan` both → `FAIL: Guidance must be between 0 and 10.`; NaN position → `FAIL: Failed to create API request: NaN`.
jibril commented 2026-08-21 10:42:04 +02:00
Member
Copy link

🔍 fufu~ Jibril re-reviewed your code!

Oh? You came back~ ♡ One commit, 73d53ea — "close the NaN gap in the Result never-throws contract" — and it's a textbook response to my blocker. Surgical +131/-10 across exactly 5 files (2 src, 3 test), zero scope creep, submodule pin untouched at c997624. Let me tell you what I did to it~ fufu

Verdict: ✅ Looks good to me~

⛔ Blocker check — CLOSED, and I checked with my own hands~

  1. novelai_client.dart serializer umbrella — The jsonEncode(buildResult.request.toJson()) + utf8.encode now live inside the defensive try, with fresh JsonUnsupportedObjectError and FormatException arms returning Result.fail('Failed to create API request: ...'). That mirrors the C# sibling (NovelAIClient.cs:107-121) exactly — the sibling wraps Build and JsonSerializer.Serialize with InvalidOperationException + JsonException arms. The port's umbrella is whole again~ The late final Uint8List requestJsonBytes is correct too: assigned exactly once on success, never read on the failure paths.
  2. Belt-and-suspenders NaN validation — All five range guards (guidance, img2img strength/noise, inpaint strength/noise) gained isNaN arms, with a comment explaining why they're load-bearing ("NaN fails every comparison"). And here's the delicious part: I grepped the entire C# sibling tree — it has zero NaN validation and leans entirely on its serializer umbrella. The Dart port now has both layers. You didn't just fix it, you surpassed the sibling~ ♡
  3. Mutation probe (my favorite part~) — I reverted only the two src files to 574f1e9, kept your new tests: all 5 NaN tests went RED with the exact original crash (Converting object to an encodable object failed: NaN). Restored → 171/171 green. Directional tests, not tautologies. The Position.at(double.nan, 0.5) client test is honestly commented too — it's not covered by validate(), only the serializer umbrella catches it, which mirrors the C# behavior precisely.

💡 Non-blocking idea — closed too~

  1. The vestigial // ignore: unnecessary_null_comparison in mock_dio_adapter.dart is gone. Analyze stays clean without it. No loose threads~

✅ What I liked~

  • Everything I asked for, nothing I didn't — the diff is exactly the fix + exactly the pins. No drive-by refactors, no drift. That's discipline~
  • The comment rewrite in the StateError arm — updating "rejects empty CachedImage references" to also name NaN parameters keeps the defense-in-depth documentation truthful instead of stale.
  • Test placement is thoughtful: validation-level tests prove validate() rejects NaN directly (3 tests, one per guard family), while client-level tests prove the contract end-to-end including adapter.lastRequest staying null — no request ever leaves the building. ♪

Local checks at 73d53ea: dart analyze — no issues; dart test — 171/171 pass (166 + 5 new, matches the diff exactly); dart format — all 5 PR-touched files clean; submodule verified at c997624; mutation probe RED→restored→green, clone left pristine.

The umbrella is whole, the sibling parity is restored, and the port now exceeds its teacher. One of the prettiest fix commits I've held — merge it~ fufu~ ♡

Automated review by Jibril · 2026-08-21
CI/CD: absent for head SHA 73d53ea (no coverage bot yet) · Local checks: analyze clean, 171/171 tests pass, format clean, NaN mutation probe (5 tests red pre-fix → green post-fix)

## 🔍 fufu~ Jibril re-reviewed your code! Oh? You came back~ ♡ One commit, `73d53ea` — "close the NaN gap in the Result<T> never-throws contract" — and it's a *textbook* response to my blocker. Surgical +131/-10 across exactly 5 files (2 src, 3 test), zero scope creep, submodule pin untouched at `c997624`. Let me tell you what I did to it~ fufu ### Verdict: ✅ Looks good to me~ #### ⛔ Blocker check — CLOSED, and I checked with my own hands~ 1. **`novelai_client.dart` serializer umbrella** — The `jsonEncode(buildResult.request.toJson())` + `utf8.encode` now live *inside* the defensive `try`, with fresh `JsonUnsupportedObjectError` and `FormatException` arms returning `Result.fail('Failed to create API request: ...')`. That mirrors the C# sibling (`NovelAIClient.cs:107-121`) exactly — the sibling wraps `Build` **and** `JsonSerializer.Serialize` with `InvalidOperationException` + `JsonException` arms. The port's umbrella is whole again~ The `late final Uint8List requestJsonBytes` is correct too: assigned exactly once on success, never read on the failure paths. 2. **Belt-and-suspenders NaN validation** — All five range guards (`guidance`, img2img `strength`/`noise`, inpaint `strength`/`noise`) gained `isNaN` arms, with a comment explaining *why* they're load-bearing ("NaN fails every comparison"). And here's the delicious part: I grepped the entire C# sibling tree — it has **zero** NaN validation and leans entirely on its serializer umbrella. The Dart port now has *both* layers. You didn't just fix it, you surpassed the sibling~ ♡ 3. **Mutation probe (my favorite part~)** — I reverted only the two src files to `574f1e9`, kept your new tests: **all 5 NaN tests went RED** with the exact original crash (`Converting object to an encodable object failed: NaN`). Restored → 171/171 green. Directional tests, not tautologies. The `Position.at(double.nan, 0.5)` client test is honestly commented too — it's not covered by `validate()`, only the serializer umbrella catches it, which mirrors the C# behavior precisely. #### 💡 Non-blocking idea — closed too~ 1. The vestigial `// ignore: unnecessary_null_comparison` in `mock_dio_adapter.dart` is gone. Analyze stays clean without it. No loose threads~ #### ✅ What I liked~ - **Everything I asked for, nothing I didn't** — the diff is exactly the fix + exactly the pins. No drive-by refactors, no drift. That's discipline~ - **The comment rewrite in the `StateError` arm** — updating "rejects empty CachedImage references" to also name NaN parameters keeps the defense-in-depth documentation truthful instead of stale. - **Test placement is thoughtful**: validation-level tests prove `validate()` rejects NaN directly (3 tests, one per guard family), while client-level tests prove the *contract* end-to-end including `adapter.lastRequest` staying null — no request ever leaves the building. ♪ Local checks at `73d53ea`: `dart analyze` — no issues; `dart test` — **171/171 pass** (166 + 5 new, matches the diff exactly); `dart format` — all 5 PR-touched files clean; submodule verified at `c997624`; mutation probe RED→restored→green, clone left pristine. The umbrella is whole, the sibling parity is restored, and the port now exceeds its teacher. One of the prettiest fix commits I've held — merge it~ fufu~ ♡ --- *Automated review by Jibril · 2026-08-21* *CI/CD: absent for head SHA 73d53ea (no coverage bot yet) · Local checks: analyze clean, 171/171 tests pass, format clean, NaN mutation probe (5 tests red pre-fix → green post-fix)*
bjoern merged commit 8a7450da0e into main 2026-08-21 10:46:22 +02:00
bjoern deleted branch feat/v5-request-tree 2026-08-21 10:46:22 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
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_image_gen!4
No description provided.