Restructure into version-neutral core + V4 tree for upcoming v5 support

scarlet commented

2026-07-07 12:20:14 +02:00

Member

Summary

Prepares the library for NovelAI Diffusion v5 by separating version-neutral code from v4-specific code, so a future v5 tree can slot in alongside v4 without touching it.

Structure changes

Models/V4 + Models/Internal/V4 — the generation request tree (ImageGenerationRequest, Character, Position, CharacterGender, ReferenceType, all *Options) and the wire models (NovelAIParameters, V4Prompt, …) move into V4 namespaces. ⚠️ Breaking namespace change — consumers need using NovelAI.ImageGen.Models.V4;.
Internal/Transport/NovelAIHttpTransport — HTTP execution, error-body parsing and status-code messages extracted from the client. This is the part v5 inherits for free.
Internal/V4/V4ApiRequestBuilder — request → v4 wire-format building extracted from the client; NovelAIClient is now pure orchestration (~500 → ~230 lines) and the duplicated ZIP-extraction/error handling is gone.
SerializeWithGender moved from the common TagSerializer into V4PromptBuilder, so the shared serializer doesn't depend on the V4 tree.
Removed dead untracked NovelAI.ImageGen.McpServer leftovers (the MCP server lives in a separate repository).

Versioning strategy (documented in CLAUDE.md)

Version dispatch happens by request type, not by enum: when v5 ships, INovelAIClient gets a GenerateImageAsync(Models.V5.ImageGenerationRequest) overload and a Models/V5 + Internal/V5 tree — overload resolution selects the version at compile time. Shared enums (Model, Sampler, NoiseSchedule) stay common until a version actually diverges. No speculative v5 models are added yet; this PR only builds the seam.

Verification

Build clean with TreatWarningsAsErrors, 0 warnings
All 143 tests pass unchanged (behavior-only contract held; only using directives updated in tests/sample)

🤖 Generated with Claude Code

## Summary Prepares the library for NovelAI Diffusion v5 by separating version-neutral code from v4-specific code, so a future v5 tree can slot in alongside v4 without touching it. ### Structure changes - **`Models/V4` + `Models/Internal/V4`** — the generation request tree (`ImageGenerationRequest`, `Character`, `Position`, `CharacterGender`, `ReferenceType`, all `*Options`) and the wire models (`NovelAIParameters`, `V4Prompt`, …) move into V4 namespaces. ⚠️ **Breaking namespace change** — consumers need `using NovelAI.ImageGen.Models.V4;`. - **`Internal/Transport/NovelAIHttpTransport`** — HTTP execution, error-body parsing and status-code messages extracted from the client. This is the part v5 inherits for free. - **`Internal/V4/V4ApiRequestBuilder`** — request → v4 wire-format building extracted from the client; `NovelAIClient` is now pure orchestration (~500 → ~230 lines) and the duplicated ZIP-extraction/error handling is gone. - `SerializeWithGender` moved from the common `TagSerializer` into `V4PromptBuilder`, so the shared serializer doesn't depend on the V4 tree. - Removed dead untracked `NovelAI.ImageGen.McpServer` leftovers (the MCP server lives in a separate repository). ### Versioning strategy (documented in CLAUDE.md) Version dispatch happens **by request type, not by enum**: when v5 ships, `INovelAIClient` gets a `GenerateImageAsync(Models.V5.ImageGenerationRequest)` overload and a `Models/V5` + `Internal/V5` tree — overload resolution selects the version at compile time. Shared enums (`Model`, `Sampler`, `NoiseSchedule`) stay common until a version actually diverges. No speculative v5 models are added yet; this PR only builds the seam. ### Verification - Build clean with `TreatWarningsAsErrors`, 0 warnings - All 143 tests pass unchanged (behavior-only contract held; only `using` directives updated in tests/sample) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit

2026-07-07 12:20:15 +02:00

Restructure into version-neutral core + V4 tree for upcoming v5 support

CI / build (pull_request) Successful in 34s

Details

85542ea555

Prepares the library for NovelAI Diffusion v5 by separating what is
version-neutral from what is v4-specific, so a v5 tree can slot in
alongside v4 without touching it:

- Models/V4 + Models/Internal/V4: generation request tree and wire
  models move to V4 namespaces (breaking namespace change)
- Internal/Transport/NovelAIHttpTransport: HTTP execution, error-body
  parsing and status messages extracted from the client
- Internal/V4/V4ApiRequestBuilder: request-to-wire-format building
  extracted from the client; NovelAIClient is orchestration only
- SerializeWithGender moves from TagSerializer into V4PromptBuilder so
  the common serializer does not depend on the V4 tree
- Version dispatch happens via typed GenerateImageAsync overloads per
  version, not a runtime enum (documented in CLAUDE.md)
- Remove dead untracked NovelAI.ImageGen.McpServer leftovers (the MCP
  server lives in a separate repository)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

scarlet requested review from bjoern

2026-07-07 12:20:15 +02:00

jibril commented

2026-07-07 12:34:23 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh oh oh~! A version-tree refactor! Separating the V4 tree so v5 can slot in alongside it without touching anything? giddy flutter This is the kind of structural thinking that makes my wings twitch with joy, fufu~ ♡ I read every line of the diff AND the full current contents of the changed files, and traced the old behavior against the new behavior very carefully. You don't fool this Flugel~

Verdict: ✅ Looks good to me~

I could not find a single behavior regression. And believe me, I tried. The yandere in me wanted to catch you slipping a missing .Include() somewhere... but no. This refactor is faithful. ♡

🔍 What I specifically verified (so you know I actually checked, fufu~)

Seed reporting — Old GenerateImageAsync returned Seed = apiRequest.Parameters.Seed, where the seed was generated inside BuildApiRequest. New code resolves seed up front in the client and passes it to V4ApiRequestBuilder.Build(request, seed), then reports Seed = seed via ExtractImage. Same value, same reporting path. ✓
Exception surface — Old code caught HttpRequestException / TaskCanceledException / JsonException in the client. New code moves the HTTP/network catches into NovelAIHttpTransport.PostAsync/GetAsync, preserving the exact error strings ("Network error: {msg}", "Request was cancelled.") and the Result.Fail(message, statusCode) propagation through response.Match(...). ✓
UnknownImageFormatException propagation — Critically, GenerateImageAsync does not catch it (it propagates), while AugmentEmotionAsync does (friendly message). Both behaviors are preserved: V4ApiRequestBuilder.Build → ImageScaler calls sit outside the JsonException catch, so the exception still escapes unchanged from GenerateImageAsync. AugmentEmotionAsync correctly isolates GetImageDimensions in its own try/catch. ✓
AugmentEmotionAsync seed — seed: 0 hardcoded (was Seed = 0 before). Behavior-equivalent — augment has no seed concept. ✓
SerializeWithGender move — Moved from public TagSerializer into a private method on V4PromptBuilder. Logic is byte-identical, and this correctly stops the shared serializer from depending on the V4 tree. chef's kiss ✓
Version-neutral stuff stays neutral — AugmentImageApiRequest, Result<T>, Tag, GeneratedImage, ImageScaler, ZipResponseExtractor, the shared enums all stay in the version-neutral core. Only V4-specific request models + wire models moved. Exactly right. ✓
INovelAIClient signatures — Unchanged (only using directives added). The breaking namespace change (Models.Requests → Models.V4) is real for consumers but is loudly documented in the PR body. ✓

✅ What I liked~

Dispatch-by-overload instead of a runtime version enum — oh, this is elegant, fufu~ Compile-time version selection via GenerateImageAsync(Models.V4.ImageGenerationRequest) vs a future Models.V5 overload is so much cleaner than a runtime switch. The CLAUDE.md rationale ("the Model enum already implies the version") is correct and well-reasoned. ♪
NovelAIClient going from ~500 → ~230 lines of pure orchestration — the duplicated error-body parsing and ZIP extraction across 4 methods is gone. Each method now reads like a recipe: validate → build → transport → extract. Beautiful.
NovelAIHttpTransport is genuinely version-neutral — it knows nothing about payload shapes, just PostAsync/GetAsync → Result<byte[]>. v5 inherits it for free. This is the seam done right.
Build clean with TreatWarningsAsErrors, 0 warnings — and I verified locally: 143/143 tests pass. No regressions. The behavior-only contract held exactly as claimed.

💡 Little ideas (non-blocking)~

NovelAIHttpTransport.ReadResponseAsync doesn't dispose HttpResponseMessage — This is actually a pre-existing pattern (the old client didn't dispose it either, so it's NOT a regression you introduced). But now that transport is the single chokepoint for all HTTP execution, it'd be the natural place to add using var response = await _httpClient.PostAsync(...) for tidiness. Truly optional — HttpClient on .NET pools connections and the GC handles the rest, so this is a nicety, not a correctness issue. ♡
NovelAIHttpTransport.PostAsync/GetAsync are near-identical — the try/catch wrapping is duplicated between them. A tiny ExecuteAsync(Func<...>) helper could DRY it up, but with only 2 call sites this is a matter of taste. Not worth blocking on.

Automated review by Jibril · 2026-07-07
CI/CD: absent for head SHA 85542ea — local build + tests run instead · Local checks: ✅ build 0 warnings, ✅ 143/143 tests pass

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh oh oh~! A version-tree refactor! Separating the V4 tree so v5 can slot in alongside it without touching anything? *giddy flutter* This is the kind of structural thinking that makes my wings twitch with joy, fufu~ ♡ I read every line of the diff AND the full current contents of the changed files, and traced the old behavior against the new behavior very carefully. You don't fool this Flugel~ ### Verdict: ✅ Looks good to me~ I could not find a single behavior regression. And believe me, I *tried*. The yandere in me wanted to catch you slipping a missing `.Include()` somewhere... but no. This refactor is faithful. ♡ #### 🔍 What I specifically verified (so you know I actually checked, fufu~) 1. **Seed reporting** — Old `GenerateImageAsync` returned `Seed = apiRequest.Parameters.Seed`, where the seed was generated *inside* `BuildApiRequest`. New code resolves `seed` up front in the client and passes it to `V4ApiRequestBuilder.Build(request, seed)`, then reports `Seed = seed` via `ExtractImage`. **Same value, same reporting path.** ✓ 2. **Exception surface** — Old code caught `HttpRequestException` / `TaskCanceledException` / `JsonException` in the client. New code moves the HTTP/network catches into `NovelAIHttpTransport.PostAsync/GetAsync`, preserving the exact error strings (`"Network error: {msg}"`, `"Request was cancelled."`) and the `Result.Fail(message, statusCode)` propagation through `response.Match(...)`. ✓ 3. **`UnknownImageFormatException` propagation** — Critically, `GenerateImageAsync` does *not* catch it (it propagates), while `AugmentEmotionAsync` *does* (friendly message). Both behaviors are preserved: `V4ApiRequestBuilder.Build` → `ImageScaler` calls sit outside the `JsonException` catch, so the exception still escapes unchanged from `GenerateImageAsync`. `AugmentEmotionAsync` correctly isolates `GetImageDimensions` in its own try/catch. ✓ 4. **`AugmentEmotionAsync` seed** — `seed: 0` hardcoded (was `Seed = 0` before). Behavior-equivalent — augment has no seed concept. ✓ 5. **`SerializeWithGender` move** — Moved from public `TagSerializer` into a private method on `V4PromptBuilder`. Logic is byte-identical, and this correctly stops the shared serializer from depending on the V4 tree. *chef's kiss* ✓ 6. **Version-neutral stuff stays neutral** — `AugmentImageApiRequest`, `Result<T>`, `Tag`, `GeneratedImage`, `ImageScaler`, `ZipResponseExtractor`, the shared enums all stay in the version-neutral core. Only V4-specific request models + wire models moved. Exactly right. ✓ 7. **`INovelAIClient` signatures** — Unchanged (only `using` directives added). The breaking namespace change (`Models.Requests` → `Models.V4`) is real for consumers but is loudly documented in the PR body. ✓ #### ✅ What I liked~ - **Dispatch-by-overload instead of a runtime version enum** — oh, this is *elegant*, fufu~ Compile-time version selection via `GenerateImageAsync(Models.V4.ImageGenerationRequest)` vs a future `Models.V5` overload is so much cleaner than a runtime switch. The CLAUDE.md rationale ("the `Model` enum already implies the version") is correct and well-reasoned. ♪ - **`NovelAIClient` going from ~500 → ~230 lines of pure orchestration** — the duplicated error-body parsing and ZIP extraction across 4 methods is *gone*. Each method now reads like a recipe: validate → build → transport → extract. Beautiful. - **`NovelAIHttpTransport` is genuinely version-neutral** — it knows nothing about payload shapes, just `PostAsync/GetAsync` → `Result<byte[]>`. v5 inherits it for free. This is the seam done right. - **Build clean with `TreatWarningsAsErrors`, 0 warnings** — and I verified locally: **143/143 tests pass.** No regressions. The behavior-only contract held exactly as claimed. #### 💡 Little ideas (non-blocking)~ 1. **`NovelAIHttpTransport.ReadResponseAsync` doesn't dispose `HttpResponseMessage`** — This is actually a *pre-existing* pattern (the old client didn't dispose it either, so it's NOT a regression you introduced). But now that transport is the single chokepoint for all HTTP execution, it'd be the natural place to add `using var response = await _httpClient.PostAsync(...)` for tidiness. Truly optional — `HttpClient` on .NET pools connections and the GC handles the rest, so this is a nicety, not a correctness issue. ♡ 2. **`NovelAIHttpTransport.PostAsync`/`GetAsync` are near-identical** — the try/catch wrapping is duplicated between them. A tiny `ExecuteAsync(Func<...>)` helper could DRY it up, but with only 2 call sites this is a matter of taste. Not worth blocking on. --- *Automated review by Jibril · 2026-07-07* *CI/CD: absent for head SHA `85542ea` — local build + tests run instead · Local checks: ✅ build 0 warnings, ✅ 143/143 tests pass*

bjoern commented

2026-07-07 12:40:15 +02:00

Owner

Please double check if the readme needs any updates, particular for the code examples. It is fine should you determine nothing needs to be changed. I was simply interested in confirming

bjoern added 1 commit

2026-07-07 17:21:31 +02:00

Address review: dispose responses, DRY transport, README namespace note

CI / build (pull_request) Successful in 21s

Details

4e766f5ca0

- NovelAIHttpTransport: PostAsync/GetAsync share an ExecuteAsync helper
  that also disposes the HttpResponseMessage
- README: document the using directives the examples assume, including
  the new Models.V4 namespace

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

scarlet commented

2026-07-07 17:21:49 +02:00

Author

Member

Addressed in 4e766f5:

Jibril #1 (response disposal): NovelAIHttpTransport now disposes the HttpResponseMessage (using var response).
Jibril #2 (Post/Get duplication): both now delegate to a shared ExecuteAsync helper that owns the try/catch and disposal — took this since the disposal change would otherwise have to be duplicated too.
README check: all code examples reference types by bare name and every type name/signature is unchanged, so no example was wrong. However, the request types now live in Models.V4 and the README never stated which namespaces the examples assume — added a short usings block under Quick Start covering that (the one real copy-paste gotcha from the namespace move).

Build clean with 0 warnings, 143/143 tests pass.

🤖 Generated with Claude Code

Addressed in 4e766f5: - **Jibril #1 (response disposal)**: `NovelAIHttpTransport` now disposes the `HttpResponseMessage` (`using var response`). - **Jibril #2 (Post/Get duplication)**: both now delegate to a shared `ExecuteAsync` helper that owns the try/catch and disposal — took this since the disposal change would otherwise have to be duplicated too. - **README check**: all code examples reference types by bare name and every type name/signature is unchanged, so no example was wrong. However, the request types now live in `Models.V4` and the README never stated which namespaces the examples assume — added a short usings block under Quick Start covering that (the one real copy-paste gotcha from the namespace move). Build clean with 0 warnings, 143/143 tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

bjoern merged commit d0e216a3fd into main

2026-07-07 17:23:11 +02:00

bjoern deleted branch refactor/v4-v5-version-trees

2026-07-07 17:23:11 +02:00

bjoern referenced this pull request from a commit

2026-07-07 17:23:12 +02:00

Merge pull request 'Restructure into version-neutral core + V4 tree for upcoming v5 support' (#1) from refactor/v4-v5-version-trees into main

jibril referenced this pull request

2026-08-21 07:27:04 +02:00

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