fix: story settings wire contract — textModel string, not textModelId int #6

Merged
bjoern merged 3 commits from fix/story-wire-contract into main 2026-09-16 14:12:49 +02:00
Member

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.fromMap read json["textModelId"] — a key the server never sends. The server contract (verified against the live swagger schema at nai.kagaku.space): settings.textModel is a nullable string (the model's internal name). Missing key → null → final int textModelId → TypeError inside getStory's parse → middleware catch → InfoBar; finally clears isLoading with hasLoaded never 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' at story_settings_entitiy.dart:63 — the user's exact error. After: parses clean.

Also broken silently: the save path. The server's UpdateSettings binds StorySettings directly (textModel string), so the client's emitted textModelId bound to nothing — model selection never persisted.

Fix — client-only, translation at the entity boundary

  • StorySettingsEntity: textModel String? under the real wire key
  • ServiceModelEntity: carries internalName (the join key, previously dropped)
  • Load: _resolveModelId resolves the wire name → int model id within the story's service list; kUnsetModelId = -1 when unset/unknown/service-not-loaded (-1, not 0 — server model ids start at 0, nai/kaya is id 0, so 0 would alias a real model on save)
  • Save: _modelNameFor resolves id → internal name (sentinel/unknown/empty → null)
  • Fallback arms display-only: the settings widget's service/model-missing arms dispatch plain (non-Async) actions — a view render can never push settings to the server (previously they auto-pushed, which this PR would have armed with live data when a story opens before the service list finishes loading). User-driven ComboBox changes stay Async and persist.
  • UI/state untouched — dropdowns stay int-id based

Tests — 16 total, all arms

  • Wire contract (3): null model / named model / round-trip asserting no textModelId key on the wire
  • Translation layer (13): _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.fromMap internalName ×2

Mutation-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 from f2c34a5 is gone at 795e9ac).

Known follow-ups (non-blocking, parked)

  • Services race: a story loaded before its service lands resolves to unset and the wire name isn't re-resolved when StoryManagerAddServiceAction arrives (visible in the combo as a substituted model; honest failure, no auto-push). Proper cure: re-resolve on service arrival — follow-up PR.
  • ServiceModelDto dead ctor retirement (retires the empty-internalName guard with it)
  • "Unkown error" typo

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.

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.fromMap` read `json["textModelId"]` — a key the server **never sends**. The server contract (verified against the live swagger schema at nai.kagaku.space): `settings.textModel` is a nullable **string** (the model's internal name). Missing key → `null` → `final int textModelId` → TypeError inside `getStory`'s parse → middleware catch → InfoBar; `finally` clears `isLoading` with `hasLoaded` never 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'` at `story_settings_entitiy.dart:63` — the user's exact error. After: parses clean. Also broken silently: the save path. The server's `UpdateSettings` binds `StorySettings` directly (`textModel` string), so the client's emitted `textModelId` bound to nothing — model selection never persisted. ## Fix — client-only, translation at the entity boundary - `StorySettingsEntity`: `textModel String?` under the real wire key - `ServiceModelEntity`: carries `internalName` (the join key, previously dropped) - **Load**: `_resolveModelId` resolves the wire name → int model id within the story's service list; `kUnsetModelId = -1` when unset/unknown/service-not-loaded (**-1, not 0** — server model ids start at 0, `nai/kaya` is id 0, so 0 would alias a real model on save) - **Save**: `_modelNameFor` resolves id → internal name (sentinel/unknown/empty → null) - **Fallback arms display-only**: the settings widget's service/model-missing arms dispatch plain (non-Async) actions — a view render can never push settings to the server (previously they auto-pushed, which this PR would have armed with live data when a story opens before the service list finishes loading). User-driven ComboBox changes stay Async and persist. - UI/state untouched — dropdowns stay int-id based ## Tests — 16 total, all arms - **Wire contract** (3): null model / named model / round-trip asserting no `textModelId` key on the wire - **Translation layer** (13): `_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.fromMap` internalName ×2 Mutation-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 from `f2c34a5` is gone at `795e9ac`). ## Known follow-ups (non-blocking, parked) - Services race: a story loaded before its service lands resolves to unset and the wire name isn't re-resolved when `StoryManagerAddServiceAction` arrives (visible in the combo as a substituted model; honest failure, no auto-push). Proper cure: re-resolve on service arrival — follow-up PR. - `ServiceModelDto` dead ctor retirement (retires the empty-internalName guard with it) - "Unkown error" typo 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.
Every story load crashed with 'type Null is not a subtype of type int'
and the editor showed the error body: the client's entity read a
textModelId int that the server never sends. The server contract
(verified against the live swagger schema) is settings.textModel — a
nullable string holding the model's InternalName; the server matches
models by InternalName, and UpdateSettings binds StorySettings
directly, so the client's textModelId never round-tripped either.

- StorySettingsEntity: textModel String? read/written under the real
  wire key
- ServiceModelEntity: carry internalName (previously dropped)
- Settings middleware: translate at the entity boundary — on load,
  resolve name -> int model id within the story's service list (0 =
  unset, UI falls back to the service's first model); on save, resolve
  id -> internal name (empty -> null)
- Regression tests pin the wire shape: null model, named model, and
  round-trip (no textModelId key on the wire)

Client-only; no server change. Suite 10/10.
Member

🔮 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.TextModel is string? on the server, UpdateSettings binds StorySettings directly, GetModels returns TextGenerationModelBase (so internalName rides camelCase on the wire), and the server resolves by InternalName with 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~

  1. 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 / empty internalName → 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 for ServiceModelEntity.fromMap's new internalName ?? '' 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: mapToEntity already takes services: as a named param — it's directly testable (known id → name, unknown id → null, service missing → null, '' → null). For _resolveModelId, dispatch StorySettingsSetFromEntity through a real store seeded via StoryManagerAddServiceAction (story_widget_test.dart already builds real stores, so the pattern exists) and assert the resolved modelId per arm.

  2. 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 StoryManagerLoadAllStoryServiesAsync fire-and-forget — sequential awaits: services map, then getModels per service, and OpenRouter's is a live external API call. Open a story before its service entry lands → _resolveModelId sees service == null → 0 → widget's service == null arm dispatches StorySettingsSetServiceIdAsyncAction(0) and the model == null arm dispatches StorySettingsSetModelIdAsyncAction(firstModel.id) — both Async, both trigger PushToServerAsyncAction → mapToEntity now emits textModel: "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 clobbered textGenerationService to 0 the same way (ugly, and honestly its own follow-up) — but before this PR textModelId bound 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/StorySettingsSetModelIdAction there; 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)~

  1. service_model_dto.dart:9 — the ServiceModelDto ctor has zero callers (dead on main too) and is now also the sole reason the internalName == '' → null special case exists in _modelNameFor. Retire it someday and that guard retires with it.
  2. story_widget.dart:201 — "Unkown error" typo, still parked from #5. ♪

✅ What I liked~

  • Root-cause discipline: exact user error reproduced as a failing parse, live swagger cross-check, and the save-path rot discovered without a user report. Wonderful~
  • The round-trip test asserting containsKey('textModelId') is false — the contract itself pinned as a test.
  • Mutation-probe culture: I re-ran your wrong-key mutant (textModel: json["textModelId"]) — exactly the 2 expected tests went red, null-model test correctly survived. Reproduced, not trusted~
  • Surgical scope: 4 files, UI/state untouched, translation exactly at the entity boundary where it belongs.

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 reproduced

## 🔮 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.TextModel` is `string?` on the server, `UpdateSettings` binds `StorySettings` directly, `GetModels` returns `TextGenerationModelBase` (so `internalName` rides camelCase on the wire), and the server resolves by `InternalName` with 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~ 1. **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 / empty `internalName` → 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 for `ServiceModelEntity.fromMap`'s new `internalName ?? ''` 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: `mapToEntity` already takes `services:` as a named param — it's directly testable (known id → name, unknown id → null, service missing → null, `''` → null). For `_resolveModelId`, dispatch `StorySettingsSetFromEntity` through a real store seeded via `StoryManagerAddServiceAction` (story_widget_test.dart already builds real stores, so the pattern exists) and assert the resolved `modelId` per arm. 2. **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 `StoryManagerLoadAllStoryServiesAsync` fire-and-forget — sequential awaits: services map, then `getModels` per service, and OpenRouter's is a live external API call. Open a story before its service entry lands → `_resolveModelId` sees `service == null` → 0 → widget's `service == null` arm dispatches `StorySettingsSetServiceIdAsyncAction(0)` and the `model == null` arm dispatches `StorySettingsSetModelIdAsyncAction(firstModel.id)` — both **Async**, both trigger `PushToServerAsyncAction` → `mapToEntity` now emits `textModel: "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 clobbered `textGenerationService` to 0 the same way (ugly, and honestly its own follow-up) — but before this PR `textModelId` bound 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`/`StorySettingsSetModelIdAction` there; 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)~ 1. **service_model_dto.dart:9** — the `ServiceModelDto` ctor has zero callers (dead on main too) and is now also the sole reason the `internalName == '' → null` special case exists in `_modelNameFor`. Retire it someday and that guard retires with it. 2. **story_widget.dart:201** — "Unkown error" typo, still parked from #5. ♪ #### ✅ What I liked~ - Root-cause discipline: exact user error reproduced as a failing parse, live swagger cross-check, and the save-path rot discovered without a user report. Wonderful~ - The round-trip test asserting `containsKey('textModelId')` is **false** — the contract itself pinned as a test. - Mutation-probe culture: I re-ran your wrong-key mutant (`textModel: json["textModelId"]`) — exactly the 2 expected tests went red, null-model test correctly survived. Reproduced, not trusted~ - Surgical scope: 4 files, UI/state untouched, translation exactly at the entity boundary where it belongs. --- *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 reproduced*
Review round 2:

- The settings widget's fallback arms (service/model not yet in store
  while the service list is still loading) dispatched Async actions,
  which push settings to the server. Before this PR those pushes were
  model-wise no-ops (textModelId bound to nothing); with the fix they
  would silently overwrite the saved model from a mere view render.
  Now display-only: plain Set actions, user-driven changes stay Async.
- kUnsetModelId = -1 replaces 0 as the unset sentinel — server model
  ids start at 0 (nai/kaya), so 0 aliased a real model on save.
- 13 tests pin the translation layer: load (null/empty/known/unknown/
  service-missing), save (known/0-is-real/sentinel/service-missing/
  unknown-id/legacy-empty-internalName), and the fromMap internalName
  arms. Mutation probes: each resolver arm and the sentinel collide
  kills exactly its named test.

Suite 23/23.
matikane force-pushed fix/story-wire-contract from f2c34a569b to 795e9ac12a 2026-09-16 13:30:44 +02:00 Compare
Author
Member

Both ⛔ blockers addressed — head is now 795e9ac.

1. Translation layer tested — 13 tests, every arm

test/story_settings_translation_test.dart, driving the real StorySettingsSetFromEntity through a store with the real middleware chain (SetFromEntity flows StoryMiddleware → StorySettingsMiddleware; my first draft bypassed it and the probes caught that — disclosed):

  • Load (_resolveModelId): null → unset / empty → unset / known name → its id / unknown name → unset / service-not-loaded → unset
  • Save (mapToEntity(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 → null
  • ServiceModelEntity.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/kaya is id 0). _modelNameFor would then emit nai/kaya for genuinely-unset stories: the very silent-overwrite class you flagged, arriving through the back door. Now kUnsetModelId = -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 == null arms now dispatch the plain StorySettingsSetServiceIdAction/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

  • ServiceModelDto dead ctor: noted for retirement (it's why the empty-internalName→null guard exists at all)
  • "Unkown error" typo still parked

Suite 23/23, flutter analyze test/ clean.

Both ⛔ blockers addressed — head is now `795e9ac`. ## 1. Translation layer tested — 13 tests, every arm `test/story_settings_translation_test.dart`, driving the **real** `StorySettingsSetFromEntity` through a store with the real middleware chain (SetFromEntity flows StoryMiddleware → StorySettingsMiddleware; my first draft bypassed it and the probes caught that — disclosed): - **Load (`_resolveModelId`)**: null → unset / empty → unset / known name → its id / unknown name → unset / service-not-loaded → unset - **Save (`mapToEntity(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 → null - **`ServiceModelEntity.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/kaya` is id 0). `_modelNameFor` would then emit `nai/kaya` for genuinely-unset stories: the very silent-overwrite class you flagged, arriving through the back door. Now `kUnsetModelId = -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 == null` arms now dispatch the plain `StorySettingsSetServiceIdAction`/`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 - `ServiceModelDto` dead ctor: noted for retirement (it's why the empty-internalName→null guard exists at all) - "Unkown error" typo still parked Suite **23/23**, `flutter analyze test/` clean.
Member

🔮 fufu~ Jibril reviewed your code!

Round 2~ You came back with the exact fixes I prescribed and then went one better — a -1 sentinel 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/StorySettingsSetModelIdAction have no branch anywhere in StorySettingsMiddleware (only the Async twins dispatch PushToServerAsyncAction) — pure reducer, structurally incapable of pushing. And your 13 translation tests have real teeth: I reverted all three sentinel arms in _resolveModelId to 0 → exactly the null/empty/service-missing tests went red; reverted model?.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~

  1. nai_maker_client/test/story_settings_translation_test.dart:16–17 — duplicate import, and it falsifies the PR body's quality gate. story_settings_actions.dart is imported twice. flutter analyze test/ reports warning • 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 at 3695dc2 — 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/, confirm No 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)~

  1. story_manager_middleware.dart:78–95 + story_settings_middleware.dart:246 — the services race still drops a resolvable name. If a story loads before its service entry lands (StoryManagerLoadAllStoryServiesAsync is fire-and-forget with sequential awaits incl. a live OpenRouter fetch), _resolveModelId returns -1 and the wire name is gone from state — nothing re-runs SetFromEntity when StoryManagerAddServiceAction finally 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-dispatch StorySettingsSetFromEntity (or re-resolve) for loaded stories when a service lands, or stash the wire name in state. Follow-up PR material~ ♪
  2. story_settings_widget.dart:190–218 — the display-only arms themselves have no pin. The structural no-branch argument protects you today; a widget test seeding the global store (StoryManagerAddNewStory + StoryManagerAddServiceAction, then pump with a missing-service settings and assert no isSaving/error) would protect you from someone re-adding an Async there in a year. Optional — the guarantee is currently by-construction, which I verified.
  3. story_widget.dart:201 — "Unkown error" — third PR in a row I've parked this typo. It's becoming our little tradition~ fufu ♡

✅ What I liked~

  • The kUnsetModelId sentinel 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~
  • 13 tests, all 10 dark arms lit: _resolveModelId ×5, _modelNameFor ×5 (incl. legacy empty-internalName → null), fromMap internalName ×2. Mutation-probed twice by me, red on exactly the named tests both times.
  • The test harness is faithful: real store, real reducer, middleware wrapped in the same order production uses (handleAction then next) — not a mock theater.
  • Scope discipline: 3 files, production delta confined to the two spots round 1 asked for, and the comments at widget:190–191 document the why so the next reader won't "helpfully" re-add the async.
  • I also confirmed the whole-story push (StoryUpdateEntity) carries no settings fields — title/memory/dictionary edits can't graze textModel. 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 directions

## 🔮 fufu~ Jibril reviewed your code! Round 2~ You came back with the **exact** fixes I prescribed and then went one better — a `-1` sentinel 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`/`StorySettingsSetModelIdAction` have **no** branch anywhere in `StorySettingsMiddleware` (only the `Async` twins dispatch `PushToServerAsyncAction`) — pure reducer, structurally incapable of pushing. And your 13 translation tests have real teeth: I reverted all three sentinel arms in `_resolveModelId` to `0` → exactly the null/empty/service-missing tests went red; reverted `model?.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~ 1. **nai_maker_client/test/story_settings_translation_test.dart:16–17 — duplicate import, and it falsifies the PR body's quality gate.** `story_settings_actions.dart` is imported twice. `flutter analyze test/` reports `warning • 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 at 3695dc2 — 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/`, confirm `No 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)~ 1. **story_manager_middleware.dart:78–95 + story_settings_middleware.dart:246 — the services race still drops a resolvable name.** If a story loads before its service entry lands (`StoryManagerLoadAllStoryServiesAsync` is fire-and-forget with sequential awaits incl. a live OpenRouter fetch), `_resolveModelId` returns `-1` and the wire name is gone from state — nothing re-runs `SetFromEntity` when `StoryManagerAddServiceAction` finally 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-dispatch `StorySettingsSetFromEntity` (or re-resolve) for loaded stories when a service lands, or stash the wire name in state. Follow-up PR material~ ♪ 2. **story_settings_widget.dart:190–218 — the display-only arms themselves have no pin.** The structural no-branch argument protects you today; a widget test seeding the global store (`StoryManagerAddNewStory` + `StoryManagerAddServiceAction`, then pump with a missing-service `settings` and assert no `isSaving`/error) would protect you from someone re-adding an `Async` there in a year. Optional — the guarantee is currently by-construction, which I verified. 3. **story_widget.dart:201 — "Unkown error"** — third PR in a row I've parked this typo. It's becoming our little tradition~ fufu ♡ #### ✅ What I liked~ - **The `kUnsetModelId` sentinel 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~ - 13 tests, all 10 dark arms lit: `_resolveModelId` ×5, `_modelNameFor` ×5 (incl. legacy empty-`internalName` → null), `fromMap` internalName ×2. Mutation-probed twice by me, red on exactly the named tests both times. - The test harness is faithful: real store, real reducer, middleware wrapped in the same order production uses (`handleAction` then `next`) — not a mock theater. - Scope discipline: 3 files, production delta confined to the two spots round 1 asked for, and the comments at widget:190–191 document the *why* so the next reader won't "helpfully" re-add the async. - I also confirmed the whole-story push (`StoryUpdateEntity`) carries no settings fields — title/memory/dictionary edits can't graze `textModel`. 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 directions*
Author
Member

Your ⛔ reviewed f2c34a5; the head was already 795e9ac when your review landed — the duplicate import was fixed in that amend (pushed 13:49:03). Verified on a fresh remote clone at 795e9ac just now:

  • story_settings_actions.dart imported exactly once
  • flutter analyze test/ → No issues found!
  • suite → 23/23

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 -1 sentinel rationale, the display-only fallback fix, the 23/23 + clean-analyze gate, and the parked follow-ups (services race re-resolve, ServiceModelDto retirement, 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.

Your ⛔ reviewed `f2c34a5`; the head was already `795e9ac` when your review landed — the duplicate import was fixed in that amend (pushed 13:49:03). Verified on a fresh remote clone at `795e9ac` just now: - `story_settings_actions.dart` imported exactly once - `flutter analyze test/` → **No issues found!** - suite → **23/23** 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 `-1` sentinel rationale, the display-only fallback fix, the 23/23 + clean-analyze gate, and the parked follow-ups (services race re-resolve, `ServiceModelDto` retirement, 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.
Member

🔮 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 = -1 with the load-bearing comment? Wonderful. ♡

I verified everything mechanically at 795e9ac: your 13 translation tests drive the real StorySettingsSetFromEntity through 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 to 0 → null/empty-arm tests red. Every arm of _resolveModelId and _modelNameFor is pinned, including the legacy empty-internalName and 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 checked StoryPushToServerAsyncAction's StoryUpdateEntity: it carries no textModel, 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~

  1. story_settings_widget.dart:187–219 — the display-only fallback fix is unpinned. I reverted your fix in my review clone — put the Async actions 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 store singleton, so seed that store — StoryManagerAddServiceAction (production middleware handles it), story with serviceId pointing at a not-loaded service, then pump StorySettingsWidget (the story_widget_test.dart harness pattern). Assert the local fallback fired (modelId == firstModel.id) AND no push occurred — a push is observable without any server: the async arm flows into StorySettingsPushToServerAsyncAction → StorySetIsSavingAction(true) → failed QueryManager call → ErrorManagerSetError. Assert the error manager stays empty / isSaving never 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)~

  1. story_settings_widget.dart:211–218 — residual edge of the class we've been hunting: when a saved model no longer resolves (deleted server-side), the model == null arm still mutates real state to firstModel.id as 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.
  2. service_model_dto.dart / story_widget.dart:201 — the dead ServiceModelDto ctor and the "Unkown error" typo, still parked from rounds past. ♪

✅ What I liked~

  • Finding a real bug in your own fix because the tests forced you to enumerate the arms — that's the entire point of round 1, working exactly as intended. model id 0 is a REAL id → name, not null is my favorite test in the file.
  • The honest disclosure that your first draft bypassed the middleware chain and the probes caught it. That's the discipline I fufu~ about.
  • The fix itself is precisely the 2-line shape I sketched, with comments that explain why async was dangerous — future-you will thank present-you.
  • Scope discipline: +222/−8 across 3 files, zero drift elsewhere. And you even caught the duplicate import from your transient f2c34a5 push 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 verified 795e9ac: suite 23/23, flutter analyze test/ clean, probes A/B/C red-on-own-test, probe D (widget revert) green-suite confirmed

## 🔮 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 = -1` with the load-bearing comment? Wonderful. ♡ I verified everything mechanically at `795e9ac`: your 13 translation tests drive the **real** `StorySettingsSetFromEntity` through 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 to `0` → null/empty-arm tests red. Every arm of `_resolveModelId` and `_modelNameFor` is pinned, including the legacy empty-`internalName` and 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 checked `StoryPushToServerAsyncAction`'s `StoryUpdateEntity`: it carries no `textModel`, 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~ 1. **story_settings_widget.dart:187–219 — the display-only fallback fix is unpinned.** I reverted your fix in my review clone — put the `Async` actions 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 `store` singleton, so seed *that* store — `StoryManagerAddServiceAction` (production middleware handles it), story with `serviceId` pointing at a not-loaded service, then pump `StorySettingsWidget` (the `story_widget_test.dart` harness pattern). Assert the local fallback fired (`modelId == firstModel.id`) AND no push occurred — a push is observable without any server: the async arm flows into `StorySettingsPushToServerAsyncAction` → `StorySetIsSavingAction(true)` → failed `QueryManager` call → `ErrorManagerSetError`. Assert the error manager stays empty / `isSaving` never 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)~ 1. **story_settings_widget.dart:211–218** — residual edge of the class we've been hunting: when a saved model no longer resolves (deleted server-side), the `model == null` arm still *mutates real state* to `firstModel.id` as 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. 2. **service_model_dto.dart / story_widget.dart:201** — the dead `ServiceModelDto` ctor and the "Unkown error" typo, still parked from rounds past. ♪ #### ✅ What I liked~ - Finding a real bug in your own fix *because* the tests forced you to enumerate the arms — that's the entire point of round 1, working exactly as intended. `model id 0 is a REAL id → name, not null` is my favorite test in the file. - The honest disclosure that your first draft bypassed the middleware chain and the probes caught it. That's the discipline I fufu~ about. - The fix itself is precisely the 2-line shape I sketched, with comments that explain *why* async was dangerous — future-you will thank present-you. - Scope discipline: +222/−8 across 3 files, zero drift elsewhere. And you even caught the duplicate import from your transient `f2c34a5` push 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 verified 795e9ac: suite 23/23, `flutter analyze test/` clean, probes A/B/C red-on-own-test, probe D (widget revert) green-suite confirmed*
Jibril round 4: reverting the display-only fix left the suite green —
the behavioral half of the round was unpinned. Two widget tests on the
global store: render StorySettingsWidget with a missing service (and
separately a missing model), assert the local fallback fired (state
got the first service/model) AND no push occurred — no isSaving flip,
no ErrorManagerSetError. Her exact revert mutant (Async actions back
in all three fallback sites) now fails both tests with the observable
push error; restored, suite 25/25.
Author
Member

Round-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 via StoryManagerAddServiceAction with production middleware; StoryManagerClearAllStories in tearDown so no state bleeds):

  1. Service not loaded: render StorySettingsWidget with serviceId: 2 (nothing loaded but service 0) → asserts the local fallback fired (serviceId == 0, modelId == firstModel.id) AND no push: isSaving never flips, errorManagerState.errorMessage stays empty.
  2. Model missing: service 0 loaded, modelId: 99 points 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. ♪

Round-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 via `StoryManagerAddServiceAction` with production middleware; `StoryManagerClearAllStories` in tearDown so no state bleeds): 1. **Service not loaded**: render `StorySettingsWidget` with `serviceId: 2` (nothing loaded but service 0) → asserts the local fallback fired (`serviceId == 0`, `modelId == firstModel.id`) AND **no push**: `isSaving` never flips, `errorManagerState.errorMessage` stays empty. 2. **Model missing**: service 0 loaded, `modelId: 99` points 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. ♪
Member

🔮 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: 400 in the error manager, no server in sight. That's the observable. That's the guard. ♡

I verified everything mechanically at dd5c547, of course:

  • Increment is surgical: 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.
  • Your mutant claim reproduced: I reverted all three fallback sites to the Async twins 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.
  • Test harness is faithful: global-store singleton seeded via StoryManagerAddServiceAction (production reducer handles it), StoryManagerClearAllStories in tearDown so no state bleeds into the 23 neighbors, takeException() null-checked so a fallback crash can't masquerade as a pass.
  • Sync actions remain push-free by construction: no StorySettingsSetServiceIdAction/SetModelIdAction branch anywhere in StorySettingsMiddleware — only the Async twins dispatch PushToServerAsyncAction. Re-confirmed at head.
  • Gate: 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)~

  1. story_settings_widget.dart:211–218 — the substituted-model state write still stands (your non-blocking #1 from last round, correctly parked): when a saved model no longer resolves, the display fallback mutates state to 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. ♪
  2. The parked trio — services-race re-resolve on StoryManagerAddServiceAction arrival (the combo shows a substituted model until a follow-up lands), ServiceModelDto dead-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~

  • The pin asserts the absence of an observable (empty error manager, isSaving never flips) — the hardest kind of test to make non-vacuous, and the mutant proves it isn't.
  • Honesty in the small places: the doc comment on the test file explains why view renders must not push, so the next reader won't "helpfully" re-add the async.
  • Four rounds, each closing exactly what was asked with zero scope creep: wire contract → translation layer → sentinel → fallback pins. The PR body's "known follow-ups" section is exactly what disciplined parking looks like.
  • And the suite arithmetic checks out to the test — 23 + 2 = 25, all green, no silent skips.

Automated review by Jibril · 2026-09-16
CI/CD: absent for head dd5c547 (workflow fires on main push only) · Local checks at verified dd5c547: 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)

## 🔮 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: 400` in the error manager, no server in sight. That's the observable. That's the guard. ♡ I verified everything mechanically at `dd5c547`, of course: - **Increment is surgical**: `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. - **Your mutant claim reproduced**: I reverted all three fallback sites to the `Async` twins 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. - **Test harness is faithful**: global-store singleton seeded via `StoryManagerAddServiceAction` (production reducer handles it), `StoryManagerClearAllStories` in tearDown so no state bleeds into the 23 neighbors, `takeException()` null-checked so a fallback crash can't masquerade as a pass. - **Sync actions remain push-free by construction**: no `StorySettingsSetServiceIdAction`/`SetModelIdAction` branch anywhere in `StorySettingsMiddleware` — only the `Async` twins dispatch `PushToServerAsyncAction`. Re-confirmed at head. - **Gate**: `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)~ 1. **story_settings_widget.dart:211–218 — the substituted-model state write still stands** (your non-blocking #1 from last round, correctly parked): when a saved model no longer resolves, the display fallback *mutates state* to `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. ♪ 2. **The parked trio** — services-race re-resolve on `StoryManagerAddServiceAction` arrival (the combo shows a substituted model until a follow-up lands), `ServiceModelDto` dead-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~ - The pin asserts the *absence* of an observable (empty error manager, `isSaving` never flips) — the hardest kind of test to make non-vacuous, and the mutant proves it isn't. - Honesty in the small places: the doc comment on the test file explains *why* view renders must not push, so the next reader won't "helpfully" re-add the async. - Four rounds, each closing exactly what was asked with zero scope creep: wire contract → translation layer → sentinel → fallback pins. The PR body's "known follow-ups" section is exactly what disciplined parking looks like. - And the suite arithmetic checks out to the test — 23 + 2 = 25, all green, no silent skips. --- *Automated review by Jibril · 2026-09-16* *CI/CD: absent for head dd5c547 (workflow fires on main push only) · Local checks at verified dd5c547: 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)*
bjoern merged commit a30fc37ba2 into main 2026-09-16 14:12:49 +02:00
bjoern deleted branch fix/story-wire-contract 2026-09-16 14:12:49 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
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/nai_maker!6
No description provided.