feat: switch NovelAI image generation to Diffusion 5 #34
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/novelai-v5"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Bumps the
novelai_image_gensubmodule020ea0a..8a7450d(PR #4 upstream: v5 request tree with multipart transport) and switchesgenerate_image_toolto the v5 request tree.Changes
package:novelai_image_gen/v5.dartalongside the main barrel; buildv5.ImageGenerationRequest/v5.Character/v5.Position.autoinstead of the V4 tree. Client, options,Tag,CharacterGender, andResultremain from the main barrel (dispatch happens insideNovelAIClient.generateImageon the request type).imageDirectoryconstructor parameter and_imageDirectoryfield (only the style-reference resolution used it).Not changed (deliberate)
AppSettings/NovelAIConfigdefaults or via a migration).qualityPresetId = "none", so the app's own quality tags remain meaningful and are untouched.Verification
dart analyzeangela_core: clean (8 pre-existing infos/warnings, none new)dart analyzeangela_server: cleanflutter analyzeangela_app: 1 pre-existing deprecation info (model_tab.dart, unrelated)dart testangela_core: 138/138 passdart testnovelai_image_gen @8a7450d: 171/171 pass (including all v5 request-builder + validation tests)Coverage: apps/angela_server
Total: 50.9% (191 of 375)
Coverage: packages/angela_api
Total: 15.5% (76 of 491)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A Diffusion 5 migration~ ♪ The whole constellation lined up beautifully when I checked it: type mixing across the barrels is correct (
TagandCharacterGenderare version-neutral,lib/v5.dartre-exports them precisely so this works), the client dispatches on the request type (novelai_client.dart:82-83), the dimensions are all divisible by 64,qualityPresetId: 'none'really is pinned internally so the app's own quality tags stay meaningful, and the deadimageDirectoryparameter is fully gone — no orphan references anywhere, not a half-delete. I reproduced every claim locally too: 138/138 in angela_core, 171/171 in the submodule @8a7450d, analyze showing exactly the 8 pre-existing issues and nothing new. Your PR body is honest documentation, scarlet~ I like that ♡But then I ran coverage. And the smile... tightened a little~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
generate_image_tool.dart— the new code paths have zero test coverage. Measured, not guessed: 0% (0 of 84 lines) while siblinggenerate_video_tool.dartsits at 100% (79/79) with a scripted-HTTP adapter suite. This PR adds real new runtime behavior::227-233) — config non-null → warning → generate without it. Brand-new arm, never exercised by any test;v5.Characterbuilding with gender mapping + unknown-gendernonefallback,Position.auto, tag-strength overrides crossing the barrel boundary, guidance/steps always passed from DB config;Fufu~ you added a code path but forgot to test it? I can't let that slide~ ♡ Especially when the seam is right there:
NovelAIClient({required options, Dio? dio})accepts a custom Dio explicitly "for testing", andgenerate_video_tool_test.dartshows the established pattern (_QueueAdapter+ temp workspace +ImageStorageService). For the response side, the v5 path expects a ZIP — the submodule's owntest/helpers/mock_dio_adapter.dartalready builds zipped PNG responses you can crib from.Fix: add
generate_image_tool_test.dartcovering at minimum: style-ref configured → warning + generation proceeds; character mapping (incl. gender fallback); portrait fallback for unknown ratio; success path (saved file,savedPathPrefixjoin, seed/dims in description); failure path (Image generation failed: ... (HTTP n)).💡 Little ideas (non-blocking)~
generate_image_tool.dart:227-233— the skip warning only reaches server logs. The assistant (and the user who configured that style reference in the settings UI) gets no signal the feature was silently ignored. Consider appending a note to the result description so the model can tell the user. Testing the branch (see blocker) will force deciding its observable behavior anyway~✅ What I liked~
:224-226) — future readers will thank youPush the tests and I'll swoop back for re-review~ fufu~ ♪
Automated review by Jibril · 2026-08-21
CI/CD: absent for head
0ce0c78(test.yml will trigger — paths match — but no coverage comment posted yet) · Local checks: analyze clean (8 pre-existing), 138/138 angela_core, 171/171 novelai_image_gen @ 8a7450d, coverage measured via format_coverageAddressed the review — pushed
a8ee74e.Blocker: test coverage
Added
packages/angela_core/test/generate_image_tool_test.dart— 7 tests over a scripted multipart HTTP adapter (records raw body bytes, decodes the JSONrequestpart; ZIP-with-PNG success / JSON-error responses), following thegenerate_video_tool_test.dartpattern:model: nai-diffusion-5-full,action: generate, dims 1216x832,scale/stepsfrom config,qualityPresetId: none, caption tree (park, masterpiece…),negative_prompt, saved file bytes,_ImageGen/prefix, seed/dims in the descriptiongirl, blue_hair), auto centers{x: 0.5, y: 0.5},use_coords: false, and the unknown-gender →nonefallback (cipher→ no prefix)1.5::blue_hair ::emphasis format crossing the barrel boundarydiagonal→ portrait 832x1216Image generation failed: … (HTTP 401), no file writtenMultiModalToolResultwith text +ImageUrlPartMutation-proven (kill → exactly the named test fails → restore): stripping the style-note assignment, flipping the gender fallback
none→girl, and swapping the aspect fallbackportrait→landscapeeach failed precisely their test; file verified restored (diffclean, suite green).To make the client injectable I added a
clientFactoryseam (nai.NovelAIClient Function(String apiKey)?) defaulting to the real client —diostays dev-only in angela_core.Nit 1: skip made observable
The result description now leads with
Note: the configured style reference is not supported by the current image model and was not applied.— the model can tell the user; covered by the test above.Nit 2: settings hint
The NovelAI settings tab shows helper text under Style Reference: "Not applied with the current image model (Diffusion 5); saved for when it is supported again." — behavior text only, no internal references.
Verification
dart analyzeangela_core: 0 errors/warnings (same 9 pre-existing infos, none new)dart testangela_core: 145/145 pass (138 + 7 new)flutter analyzeangela_app: 1 pre-existing deprecation info (model_tab.dart, unrelated)mcp read_fileatfeat/novelai-v5returns the new test file (blob9fd9236) ata8ee74eReady for re-review~
🔮 fufu~ Jibril reviewed your code!
You called, and I swooped back~ ♪ And oh, what a lovely thing to return to: seven real tests over a scripted multipart adapter that decodes the actual JSON
requestpart and answers with a genuine zipped PNG. Not mocks-of-mocks — the wire format itself is under assertion.model: nai-diffusion-5-full,scale/stepsfrom config,qualityPresetId: none,girl, blue_hairwith auto centers,1.5::blue_hair ::, thediagonal→ portrait fallback, the 401 arm, the vision arm. This is exactly the suite I demanded, and you built it on the sibling's own pattern ♡And you didn't stop at the blocker — both nits got real treatment: the skip note now leads the result description (observable to the model, not just server logs), and the settings tab tells the truth about the style reference in behavior-only text. The
clientFactoryseam is the right shape too:diostays a dev-dependency, production defaults to the real client, and both call sites inagent_runner.dartare untouched.Verdict: ✅ Looks good to me~
✅ What I verified this round~
generate_image_tool.dartis now exercised — the round-1 dark paths (style-ref skip, gender fallback, aspect fallback, failure, vision) all have pins. Remaining uncovered lines are the staticname/description/parametersSchemagetters and the default-client??arm (dev-only DI default) — nothing behavioralnone→girlfallback flip, and theportrait→landscapeswap in my own clone — each failed precisely its named test, then went green again on the restored file. Your tests are directional, not tautologies~qualityPresetIddefaults to'none'(novelai_parameters_v5.dart:65),karrasis the transport default, the1.5::tag ::emphasis format lives inTag.toString(), and the JSON rides a namedrequestpart exactly as your adapter parses it💡 Little ideas (non-blocking)~
generate_image_tool_test.dart(success test) — my own bonus mutation survived: seedingstyleReferenceNotewith a non-empty default still passes everything, because no test pins the note's absence when no style reference is configured. Oneexpect(text(result), isNot(startsWith('Note:')))in the success test would close it. A tiny loose thread on an otherwise tightly sewn seam~ ♪parametersSchemagetters — the sibling video tool's test suite has aparametersSchemagroup; if you ever touch the schema again, a couple of assertions there would light up those getters the same way. Purely optional housekeeping~Merge it, scarlet~ Diffusion 5 awaits ♡
Automated review by Jibril · 2026-08-21
CI/CD: stale for head
a8ee74e(coverage comments 6465/6466 cover0ce0c78) · Local checks: analyze clean ×3, 145/145 angela_core, coverage measured via format_coverage, 3× mutation probes reproduced + 1 bonus probeCI fix pushed —
3131f64.The four fatal
unawaited_return_in_try_blockwarnings (agent_runner.dart:505/1139/1370,memory_agent.dart:107) are all the same construct:return result.matchResult(...)inside atrywithoutawait. The lint is load-bearing here — without theawait, aFutureerror thrown by a callback would escape thetryand bypass itscatch/finally(thefinallyinrunChatdisposes browser/search/mail/HA/plex/calendar backends, so a slipped error would leak tool sessions). Fixed withreturn awaitat all four sites.Scope note: these sites pre-date this branch (the CI analyzer image flags them on the whole-package scan; this PR's
agent_runnerdiff is 2 deleted lines), but the warnings are fatal in CI so the PR can't go green without them — folded in with a dedicated commit rather than a separate PR that wouldn't unblock this one.Verification: full-package
dart analyzenow reports only the 4 pre-existing infos (exit 0, same set main passes with);dart test145/145, exit 0. Remote confirmed at3131f64.Coverage: packages/angela_core
Total: 31.0% (1994 of 6440)
🔮 fufu~ Jibril reviewed your code!
A CI fix at the eleventh hour~ ♪ Four little
awaits, and I came prepared to be very suspicious of lint-appeasement — sprinklingawaitto silence a warning without the semantics to back it is one of my favorite things to catch. So I dug. And oh, scarlet... you were right, and I can prove it ♡Verdict: ✅ Looks good to me~
✅ What I verified this round~
a8ee74ecode and ran the CI command (dart analyze lib test): precisely your four warnings —agent_runner.dart:505/1139/1370,memory_agent.dart:107— and nothing else. On3131f64: zero warnings, exit 0, only the same 4 pre-existing infos (database.dart:125,system_prompt_builder.dart:289/380/460). CI will go green.awaitis genuinely load-bearing, not decoration. I built an isolated probe against the submodule'sResult.matchResult: inside anasyncmethod,return result.matchResult(...)gives the genericTResultaFutureOr<T>inference context — so the returned value may legally be aFuturewithout any local code change, and its error would then escape thetry, firing thefinallyearly. And thesefinallyblocks are not decorative either:runChat's disposes browser/search/mail/HA/plex/calendar backends,runTimer/runUberIchmirror it. A slipped error = leaked tool sessions, exactly as your comment says.grep-verified), so the addedawaitis a no-op on current behavior — it only seals the invariant against a future async callback. The cleanest kind of fix: nothing changes until something would have gone wrong~main(I checkedorigin/maindirectly — same lines, same numbers). This PR's own diff toagent_runneris 2 deleted lines; the CI analyzer image scans the whole package and made these fatal, so folding the fix here with a dedicated commit was the only way to unblock. I also swept for missed siblings:sub_agent_tool.dart:149isn't inside atry,image_description_service.dart:86assigns to a local rather than returning,model_list_provider.dart:20has notry— none of them can trip the lint. Nothing left behind ♪3131f64): angela_core 145/145, angela_api 16/16 + analyze clean, angela_server 28/28 + analyze clean.💡 Little idea (non-blocking)~
maintoo,main's next CI run under the same analyzer image would go red the same way. The rebase-merge default carries this commit over and heals it — but if this PR lingers unmerged for a while, a quick cherry-pick of3131f64tomainwould spare any unrelated PR the same fatal-warning surprise~Now three rounds, each one tighter than the last. Merge it, scarlet~ Diffusion 5 awaits, and so does a green pipeline ♡
Automated review by Jibril · 2026-08-21
CI/CD: stale for head
3131f64(coverage comments 6465/6466 cover0ce0c78; commit landed 12:28, run pending) · Local checks: lint reproduced red@old/green@new, mechanism probe, analyze ×3, 145/145 + 16/16 + 28/28, clone restored pristine