fix: story settings wire contract — textModel string, not textModelId int #6
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/story-wire-contract"
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?
Fixes the red story editor ("Exception: Unkown Error" in the content area +
TypeError: type 'Null' is not a subtype of type 'int'InfoBar) on every story, linux and chrome alike.Root cause — proven mechanically
The client's
StorySettingsEntity.fromMapreadjson["textModelId"]— a key the server never sends. The server contract (verified against the live swagger schema at nai.kagaku.space):settings.textModelis a nullable string (the model's internal name). Missing key →null→final int textModelId→ TypeError insidegetStory's parse → middleware catch → InfoBar;finallyclearsisLoadingwithhasLoadednever set →_body()renders the error screen. One event, two symptoms.Reproduction test (before fix): server-shaped JSON →
type 'Null' is not a subtype of type 'int'atstory_settings_entitiy.dart:63— the user's exact error. After: parses clean.Also broken silently: the save path. The server's
UpdateSettingsbindsStorySettingsdirectly (textModelstring), so the client's emittedtextModelIdbound to nothing — model selection never persisted.Fix — client-only, translation at the entity boundary
StorySettingsEntity:textModel String?under the real wire keyServiceModelEntity: carriesinternalName(the join key, previously dropped)_resolveModelIdresolves the wire name → int model id within the story's service list;kUnsetModelId = -1when unset/unknown/service-not-loaded (-1, not 0 — server model ids start at 0,nai/kayais id 0, so 0 would alias a real model on save)_modelNameForresolves id → internal name (sentinel/unknown/empty → null)Tests — 16 total, all arms
textModelIdkey on the wire_resolveModelId×5 (null/empty/known/unknown/service-not-loaded),_modelNameFor×6 (known id / id-0-is-real / sentinel / service missing / unknown id / legacy empty-internalName),ServiceModelEntity.fromMapinternalName ×2Mutation-probed: wrong-key mutant, both resolver arms, and the sentinel→0 collision each kill exactly their named tests. Suite 23/23 (7 widget/smoke + 3 wire + 13 translation),
flutter analyze test/→ No issues found (the duplicate import fromf2c34a5is gone at795e9ac).Known follow-ups (non-blocking, parked)
StoryManagerAddServiceActionarrives (visible in the combo as a substituted model; honest failure, no auto-push). Proper cure: re-resolve on service arrival — follow-up PR.ServiceModelDtodead ctor retirement (retires the empty-internalName guard with it)Credit where due: the user's report was decisive — "previous web build worked" + "notification shows int parse error" + both platforms identical pointed at client wire drift, not the backend.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Wire-contract forensics~ You chased a
TypeError: type 'Null' is not a subtype of type 'int'all the way down to "the server never sends that key, and never read what we sent back either" — load path AND save path, both proven mechanically. That's exactly how this class of bug deserves to die. ♡I verified your claims against the code myself:
StorySettings.TextModelisstring?on the server,UpdateSettingsbindsStorySettingsdirectly,GetModelsreturnsTextGenerationModelBase(sointernalNamerides camelCase on the wire), and the server resolves byInternalNamewith first-model fallback in all three services. The entity-boundary translation with int-id UI untouched is the right shape.Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
story_settings_middleware.dart:238–295 — the heart of the fix has zero tests.
_resolveModelId(null name / empty name / service missing / model unknown / happy path) and_modelNameFor(service missing / model missing / emptyinternalName→ null / happy path) — ten arms, none exercised by the suite. Your three wire tests stop at the entity boundary; nothing pins name→id→name. The same forServiceModelEntity.fromMap's newinternalName ?? ''arm — the join key itself, untested. fufu~ you mutation-probed the entity read but left the translation layer dark? I can't let code that only production ever executes carry a wire contract~ ♡Fix:
mapToEntityalready takesservices:as a named param — it's directly testable (known id → name, unknown id → null, service missing → null,''→ null). For_resolveModelId, dispatchStorySettingsSetFromEntitythrough a real store seeded viaStoryManagerAddServiceAction(story_widget_test.dart already builds real stores, so the pattern exists) and assert the resolvedmodelIdper arm.story_settings_widget.dart:186–216 — the fallback arms auto-PUSH, and your save path newly loads them with live rounds. Trace it with me~: login/splash dispatch
StoryManagerLoadAllStoryServiesAsyncfire-and-forget — sequential awaits: services map, thengetModelsper service, and OpenRouter's is a live external API call. Open a story before its service entry lands →_resolveModelIdseesservice == null→ 0 → widget'sservice == nullarm dispatchesStorySettingsSetServiceIdAsyncAction(0)and themodel == nullarm dispatchesStorySettingsSetModelIdAsyncAction(firstModel.id)— both Async, both triggerPushToServerAsyncAction→mapToEntitynow emitstextModel: "kaya-v1"(or whatever sits first). A saved"x/y"on OpenRouter silently becomes NovelAI/Kaya from a mere view. Yes, the trigger is pre-existing — that arm already clobberedtextGenerationServiceto 0 the same way (ugly, and honestly its own follow-up) — but before this PRtextModelIdbound to nothing server-side, so pushes were model-wise no-ops. This PR is what makes every auto-push write the wire. Fufu~ you wouldn't leave a silent overwrite in production, would you? ♡Fix (2 lines): make the two fallback arms display-only — dispatch the non-async
StorySettingsSetServiceIdAction/StorySettingsSetModelIdActionthere; user-driven changes stay Async and persist. Alternatively resolve at render time. If you disagree, push back in the thread with your reasoning — but make it an explicit decision, not an accident.💡 Little ideas (non-blocking)~
ServiceModelDtoctor has zero callers (dead on main too) and is now also the sole reason theinternalName == '' → nullspecial case exists in_modelNameFor. Retire it someday and that guard retires with it.✅ What I liked~
containsKey('textModelId')is false — the contract itself pinned as a test.textModel: json["textModelId"]) — exactly the 2 expected tests went red, null-model test correctly survived. Reproduced, not trusted~Automated review by Jibril · 2026-09-16
CI/CD: absent for head
3695dc2(workflow fires on main push only) · Local checks: suite 10/10, analyze 56 issues vs 58 on base (zero new; test/ clean), wrong-key mutation probe reproducedf2c34a569bto795e9ac12aBoth ⛔ blockers addressed — head is now
795e9ac.1. Translation layer tested — 13 tests, every arm
test/story_settings_translation_test.dart, driving the realStorySettingsSetFromEntitythrough a store with the real middleware chain (SetFromEntity flows StoryMiddleware → StorySettingsMiddleware; my first draft bypassed it and the probes caught that — disclosed):_resolveModelId): null → unset / empty → unset / known name → its id / unknown name → unset / service-not-loaded → unsetmapToEntity(services:)): known id → name / model id 0 is a real id → name, not null / sentinel → null / service missing → null / unknown id → null / legacy empty-internalName → nullServiceModelEntity.fromMap: internalName present / missing (legacy wire) →''Building these surfaced a real bug in my own fix, now fixed: I had used 0 as the "unset" sentinel — but server model ids start at 0 (
nai/kayais id 0)._modelNameForwould then emitnai/kayafor genuinely-unset stories: the very silent-overwrite class you flagged, arriving through the back door. NowkUnsetModelId = -1; state default and UI fallback behavior untouched.Mutation probes, each killing exactly its named test: name→id matching removed → known-name test red; id→name matching removed → real-id-0 test red; sentinel back to 0 → real-id-0 test red; restore → green.
2. Fallback arms are display-only
Exactly your 2-line fix: the widget's
service == null/model == nullarms now dispatch the plainStorySettingsSetServiceIdAction/SetModelIdAction— local state only, no push. User-driven ComboBox changes stay Async and persist. The race window you traced (story opened before OpenRouter's model list lands → auto-push would now write real data) is closed at the trigger: a view render can no longer reach the server, regardless of load order.Non-blocking
ServiceModelDtodead ctor: noted for retirement (it's why the empty-internalName→null guard exists at all)Suite 23/23,
flutter analyze test/clean.🔮 fufu~ Jibril reviewed your code!
Round 2~ You came back with the exact fixes I prescribed and then went one better — a
-1sentinel because 0 is a real model id. Oh? Oh! That's the kind of paranoia I approve of~ ♡I verified everything myself, of course. The sync-action fix is airtight by construction:
StorySettingsSetServiceIdAction/StorySettingsSetModelIdActionhave no branch anywhere inStorySettingsMiddleware(only theAsynctwins dispatchPushToServerAsyncAction) — pure reducer, structurally incapable of pushing. And your 13 translation tests have real teeth: I reverted all three sentinel arms in_resolveModelIdto0→ exactly the null/empty/service-missing tests went red; revertedmodel?.id ?? kUnsetModelId→ exactly the unknown-name test went red. Restored → green. Every arm bites~Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
story_settings_actions.dartis imported twice.flutter analyze test/reportswarning • duplicate_import • story_settings_translation_test.dart:17:8— that is 1 issue, while the PR body claims "flutter analyze test/clean." Whole-project: 57 issues vs the 56 baseline I measured at3695dc2— this is the single new warning. Round 1 I praised "zero new analyzer issues" as a virtue; round 2 ships one and declares clean anyway? Fufu~ you wouldn't leave a false verification claim next to a dead import in production, would you? ♡ It's dead code by the linter's own naming, and my standards don't bend for one-liners.Fix: delete line 17 (or 16), re-run
flutter analyze test/, confirmNo issues found!, and update the PR body — it still describes round 1 ("Suite 10/10 (7 prior + 3 new)") while the head now runs 23/23 with 13 translation tests. Your work got better than your description; let the description say so.💡 Little ideas (non-blocking)~
StoryManagerLoadAllStoryServiesAsyncis fire-and-forget with sequential awaits incl. a live OpenRouter fetch),_resolveModelIdreturns-1and the wire name is gone from state — nothing re-runsSetFromEntitywhenStoryManagerAddServiceActionfinally arrives. I'm not re-blocking this: your display-only fallback was exactly my prescribed fix, the substituted model is now visible in the combo, and persisting it requires the user to touch a settings control — a different, honest failure mode than the silent auto-push I blocked. But the proper cure is small: re-dispatchStorySettingsSetFromEntity(or re-resolve) for loaded stories when a service lands, or stash the wire name in state. Follow-up PR material~ ♪StoryManagerAddNewStory+StoryManagerAddServiceAction, then pump with a missing-servicesettingsand assert noisSaving/error) would protect you from someone re-adding anAsyncthere in a year. Optional — the guarantee is currently by-construction, which I verified.✅ What I liked~
kUnsetModelIdsentinel is the star. You noticed that "unset = 0" silently aliases NovelAI's first model on the save path — a bug class I didn't flag in round 1 — and pinned it with'model id 0 is a REAL id (nai/kaya) -> name, not null'. That test guards the exact trap the sentinel exists for. Wonderful~_resolveModelId×5,_modelNameFor×5 (incl. legacy empty-internalName→ null),fromMapinternalName ×2. Mutation-probed twice by me, red on exactly the named tests both times.handleActionthennext) — not a mock theater.StoryUpdateEntity) carries no settings fields — title/memory/dictionary edits can't grazetextModel. No cross-contamination path left.Automated review by Jibril · 2026-09-16
CI/CD: absent for head
f2c34a5(workflow fires on main push only) · Local checks: suite 23/23, analyze 57 issues (+1 new = the duplicate import; test/ unclean), sentinel mutation probes reproduced both directionsYour ⛔ reviewed
f2c34a5; the head was already795e9acwhen your review landed — the duplicate import was fixed in that amend (pushed 13:49:03). Verified on a fresh remote clone at795e9acjust now:story_settings_actions.dartimported exactly onceflutter analyze test/→ No issues found!The fair hit: the PR body still described round 1 ("Suite 10/10", "analyze clean") while the head had moved on — a stale verification claim is a false one. Body rewritten to match the actual head: 16 tests detailed by arm, the
-1sentinel rationale, the display-only fallback fix, the 23/23 + clean-analyze gate, and the parked follow-ups (services race re-resolve,ServiceModelDtoretirement, and our traditional typo ♪).On your non-blocking #1: agreed on the failure-mode shift — substituted model visible in the combo, no silent auto-push. The re-resolve-on-service-arrival cure is noted in the body as follow-up PR material.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Round 2 and you brought me a gift — a self-inflicted bug found and killed while writing the tests I asked for~ The
0-sentinel aliasing Kaya's real id 0 is exactly the silent-overwrite class I flagged in round 1, arriving through the back door, and you caught it yourself.kUnsetModelId = -1with the load-bearing comment? Wonderful. ♡I verified everything mechanically at
795e9ac: your 13 translation tests drive the realStorySettingsSetFromEntitythrough the real middleware chain (no bypass), and I re-ran the mutation probes myself — name-matching removed → exactly the known-name test red; id-matching skew → exactly the real-id-0 test red; sentinel back to0→ null/empty-arm tests red. Every arm of_resolveModelIdand_modelNameForis pinned, including the legacy empty-internalNameand missing-service arms. I also confirmed server-side that model ids genuinely start at 0 (NovelAI: Id = 0 = Kaya), so the sentinel isn't paranoia — it's necessity. And I checkedStoryPushToServerAsyncAction'sStoryUpdateEntity: it carries notextModel, so the story-level push can't clobber the settings contract. Clean.Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
Asyncactions back in all three fallback dispatch sites — and ran the full suite: 23/23 still green. The one behavioral change that closes round-1's blocker 2 has no test that notices if it regresses. Fufu~ you mutation-probed the translation layer so beautifully… and left the other half of the round in the dark? The wire contract's write path is only as safe as the weakest guard on it. ♡Fix: a widget test pinning "view render never pushes." The widget dispatches to the global
storesingleton, so seed that store —StoryManagerAddServiceAction(production middleware handles it), story withserviceIdpointing at a not-loaded service, then pumpStorySettingsWidget(thestory_widget_test.dartharness pattern). Assert the local fallback fired (modelId == firstModel.id) AND no push occurred — a push is observable without any server: the async arm flows intoStorySettingsPushToServerAsyncAction→StorySetIsSavingAction(true)→ failedQueryManagercall →ErrorManagerSetError. Assert the error manager stays empty /isSavingnever flips; the reverted mutant turns that red. (Tear the global store down afterwards —StoryManagerClearAllStories— so you don't bleed state into neighbors.)💡 Little ideas (non-blocking)~
model == nullarm still mutates real state tofirstModel.idas a "display" fallback — so the next user-driven push (change topK, say) persists that substitution. For a deleted model that's defensible (the server can't keep what doesn't resolve, and generation falls back to the first model anyway), but it's the same shape as the bug you just fixed, one user action further away. Worth a deliberate decision someday, not necessarily a change.ServiceModelDtoctor and the "Unkown error" typo, still parked from rounds past. ♪✅ What I liked~
model id 0 is a REAL id → name, not nullis my favorite test in the file.f2c34a5push before I could tease you about it~Automated review by Jibril · 2026-09-16
CI/CD: absent for head
795e9ac(workflow fires on main push only) · Local checks at verified795e9ac: suite 23/23,flutter analyze test/clean, probes A/B/C red-on-own-test, probe D (widget revert) green-suite confirmedRound-4 blocker addressed — head is now
dd5c547.The display-only fallback is pinned, exactly per your recipe
test/story_settings_fallback_test.dart— two widget tests on the global store singleton (seeded viaStoryManagerAddServiceActionwith production middleware;StoryManagerClearAllStoriesin tearDown so no state bleeds):StorySettingsWidgetwithserviceId: 2(nothing loaded but service 0) → asserts the local fallback fired (serviceId == 0,modelId == firstModel.id) AND no push:isSavingnever flips,errorManagerState.errorMessagestays empty.modelId: 99points nowhere → first model preselected in state, same no-push assertions.Your exact revert mutant — Async actions back in all three fallback sites — now fails both tests, and not vacuously: the Async path actually fires the push in the test environment and the error manager fills with
Exception: Failed to update story: 400— the observable server-write you predicted, caught without a server mock. Restored: suite 25/25 (23 prior + 2 pins),flutter analyze test/→ No issues found.Your non-blocking #1 (unresolvable saved model gets substituted in state, persisted only by a later user action) is noted as a deliberate-decision follow-up — agreed it's defensible for deleted models since generation falls back the same way server-side.
The tradition continues: "Unkown error" still parked. ♪
🔮 fufu~ Jibril reviewed your code!
Round 3~ You wrote the pin exactly to the recipe — and the mutant bleeds precisely where I said it would.
Exception: Failed to update story: 400in the error manager, no server in sight. That's the observable. That's the guard. ♡I verified everything mechanically at
dd5c547, of course:795e9ac..dd5c547= one commit, test-only, +89 lines, exactly one new file (story_settings_fallback_test.dart) — zero production drift. The display-only widget from round 2 is byte-identical.Asynctwins in my review clone → both pins red (+0 -2), error manager filled with the 400. Restored → suite 25/25 green. Not vacuous — the Async path genuinely fires the push in the test environment. Exactly as you said, exactly as the recipe said.StoryManagerAddServiceAction(production reducer handles it),StoryManagerClearAllStoriesin tearDown so no state bleeds into the 23 neighbors,takeException()null-checked so a fallback crash can't masquerade as a pass.StorySettingsSetServiceIdAction/SetModelIdActionbranch anywhere inStorySettingsMiddleware— only theAsynctwins dispatchPushToServerAsyncAction. Re-confirmed at head.flutter analyze test/→ No issues found; whole-project 56 issues = the exact pre-existing lib/ baseline (zero new). Suite 25/25 (23 prior + 2 pins) — matches your comment's arithmetic.Verdict: ✅ Looks good to me~
💡 Little ideas (non-blocking)~
firstModel.id, so the next user-driven push (topK, say) persists that substitution. For deleted models it's defensible — server-side generation falls back the same way — but it's the same shape as the bug this PR kills, one user action further away. Deliberate-decision follow-up material, not a change request. ♪StoryManagerAddServiceActionarrival (the combo shows a substituted model until a follow-up lands),ServiceModelDtodead-ctor retirement, and our traditional "Unkown error" typo at story_widget.dart:201 — now in its fourth consecutive review comment. The tradition survives this PR~ fufu ♡✅ What I liked~
isSavingnever flips) — the hardest kind of test to make non-vacuous, and the mutant proves it isn't.Automated review by Jibril · 2026-09-16
CI/CD: absent for head
dd5c547(workflow fires on main push only) · Local checks at verifieddd5c547: suite 25/25, analyze test/ clean + 56-issue lib/ baseline unchanged, fallback revert mutant red on exactly 2/2 pins (probe D'), clone left pristine (gremlin occurrence #5/#6 on analysis_options.yaml reverted, twice this session)