diff --git a/CODE_REVIEW_FINDINGS.md b/CODE_REVIEW_FINDINGS.md index f98822f..a6b8486 100644 --- a/CODE_REVIEW_FINDINGS.md +++ b/CODE_REVIEW_FINDINGS.md @@ -4,107 +4,115 @@ Full-codebase review (no git history, so whole project was the scope). Status: `pending` = not yet fixed, `fixed` = applied, `verify-failed` = finding was wrong on closer look, `skipped` = intentionally not fixed. Resume point: fix `pending` items top-to-bottom (they are ranked by severity). +**2026-07-06 update (branch `bugfix-code-review`):** every finding re-verified against +current code. #1/#2/#3/#4/#6/#12 had already been fixed in earlier sessions (statuses were +stale); #5/#7/#8/#9/#10/#13/#14 (backend) and #15–#21 (frontend) fixed in this pass. +#11 is skipped: real AI Dungeon's `addStoryCard` also returns the new card's index +(0-falsy included) per the official scripting guidebook, so changing it would break +compatibility — documented in `engine.py`'s prelude instead. The cleanup backlog +(R/S/E/A items) below remains open by design. + ## Correctness bugs -### 1. [pending] seed_demo.py doesn't stamp schema version → server crashes on next start +### 1. [fixed] seed_demo.py doesn't stamp schema version → server crashes on next start - `backend/seed_demo.py:10` - Fresh DB created via `Base.metadata.create_all` leaves `PRAGMA user_version` at 0. Next server start sees tables exist, replays every ALTER TABLE migration → `duplicate column name` crash. - Fix: stamp user_version to latest after create_all (reuse migrations.bootstrap logic). -### 2. [pending] Turn-lock race: two simultaneous turns can run on the same adventure +### 2. [fixed] Turn-lock race: two simultaneous turns can run on the same adventure - `backend/app/routers/adventures.py:302` - `ensure_not_generating()` runs in the route handler but `_active_turns.add()` only happens when the StreamingResponse generator is first iterated. Double-click Continue → both requests pass the 409 check → duplicate Action.index rows, interleaved generations. - Fix: atomically test-and-set the lock in the request phase, release in the stream's `finally`. -### 3. [pending] Migration 10 renumbers indexes with a correlated subquery on the table being updated +### 3. [fixed] Migration 10 renumbers indexes with a correlated subquery on the table being updated - `backend/app/migrations.py:34` - SQLite may evaluate the subquery against partially-updated rows → duplicate indexes survive the "repair". - Fix: compute new indexes in Python (SELECT ordered, then UPDATE per row). -### 4. [pending] SQLite foreign keys never enabled → CASCADE/SET NULL clauses are dead +### 4. [fixed] SQLite foreign keys never enabled → CASCADE/SET NULL clauses are dead - `backend/app/database.py:8` - Deleting a Script leaves orphaned `scenario_scripts` rows; SQLite rowid reuse can attach a future script to an old scenario. - Fix: `PRAGMA foreign_keys=ON` via engine connect event. -### 5. [pending] Provider generate() silently yields nothing for non-SSE 200 responses +### 5. [fixed] Provider generate() silently yields nothing for non-SSE 200 responses - `backend/app/providers/openai_compatible.py:84` - Server that ignores `stream=true` and returns plain JSON → no `data:` lines → empty AI action, no error. - Fix: buffer non-SSE body and fall back to parsing it as a single JSON completion. -### 6. [pending] Fire-and-forget asyncio task can be GC'd mid-run and wedge the memory bank +### 6. [fixed] Fire-and-forget asyncio task can be GC'd mid-run and wedge the memory bank - `backend/app/memorybank.py:146` - `asyncio.create_task` result not referenced; task can vanish silently; adventure ID can stay stuck in `_running`. - Fix: keep strong refs in a set, discard in done-callback. -### 7. [pending] Memory cursors are list positions but Memory.source_start/end are Action.index values +### 7. [fixed] Memory cursors are list positions but Memory.source_start/end are Action.index values - `backend/app/memorybank.py:213` - After any action deletion (indexes keep gaps, positions shift), summarization skips/duplicates blocks and `_update_story_summary` folds the wrong memories. - Fix: use one space consistently — track cursors by Action.index (position-independent), or renumber on delete. -### 8. [pending] Pinned memories don't count toward memory_top_k cap +### 8. [fixed] Pinned memories don't count toward memory_top_k cap - `backend/app/memorybank.py:120` - 6 pinned + top_k=5 → 11 memories injected, blowing token budget. - Fix: fill with unpinned only up to `top_k - len(pinned)` (min 0). -### 9. [pending] Embedding-model change → cosine() zips different-dimension vectors silently +### 9. [fixed] Embedding-model change → cosine() zips different-dimension vectors silently - `backend/app/memorybank.py:69` - Old 768-dim embeddings scored against new 1536-dim query → garbage similarity, no error, never re-embedded. - Fix: return 0.0 on length mismatch (and ideally clear stale embeddings so _embed_pending redoes them). -### 10. [pending] MAX_STORY_CARDS cap is a no-op for cards created in one hook +### 10. [fixed] MAX_STORY_CARDS cap is a no-op for cards created in one hook - `backend/app/scripting/pipeline.py:59` - `len(existing) + len(seen_ids) < MAX...` never counts newly added cards (seen_ids ⊂ existing). A script can insert unbounded cards in one turn. - Fix: count inserts made during the loop. -### 11. [pending] addStoryCard returns 0 (falsy) for the first card, indistinguishable from `false` rejection +### 11. [skipped] addStoryCard returns 0 (falsy) for the first card, indistinguishable from `false` rejection - `backend/app/scripting/engine.py:39` - `if (!addStoryCard(...))` misfires when the card list was empty. - Fix: return `storyCards.length` (1-based, always truthy) or `true`; document. -### 12. [pending] scenario_id=0 truthiness bug in create_story_card +### 12. [fixed] scenario_id=0 truthiness bug in create_story_card - `backend/app/routers/story_cards.py:28` - `scenario_id or ...` picks the wrong owner when id is 0. Use `is not None`. -### 13. [pending] test_connection 500s on non-dict JSON from /models +### 13. [fixed] test_connection 500s on non-dict JSON from /models - `backend/app/routers/settings.py:54` - Only ValueError caught; `data.get`/`m.get` on non-dict raises AttributeError → 500 instead of `{ok:false}`. - Fix: catch (ValueError, AttributeError, TypeError) or validate shapes. -### 14. [pending] AI Dungeon exports with `worldInformation` key lose all story cards silently +### 14. [fixed] AI Dungeon exports with `worldInformation` key lose all story cards silently - `backend/app/routers/scenarios.py:133` - Import reads only `storyCards`/`worldInfo`; `worldInformation` is in _IGNORED_KEYS so it's dropped and not reported. - Fix: accept `worldInformation` as a card source too. -### 15. [pending] Shared debounce timer loses edits (Play PlotPanel) +### 15. [fixed] Shared debounce timer loses edits (Play PlotPanel) - `frontend/src/pages/Play.jsx:28` - One `saveTimer` shared by all plot fields AND story-card saves; editing a second thing within 600ms cancels the first pending PATCH → silent data loss. - Fix: per-key timers (e.g. a Map keyed by field/card id). -### 16. [pending] Same shared-debounce data loss in ScenarioEditor +### 16. [fixed] Same shared-debounce data loss in ScenarioEditor - `frontend/src/pages/ScenarioEditor.jsx:22` - Same fix as #15. -### 17. [pending] Continue button silently discards typed input text +### 17. [fixed] Continue button silently discards typed input text - `frontend/src/pages/Play.jsx:469` - Clicking Continue with text in the box sends type 'continue' (backend ignores text) and clears the input. - Fix: don't clear input on continue (or treat non-empty input as a normal send). -### 18. [pending] retry() optimistically deletes last AI action with no rollback on failure +### 18. [fixed] retry() optimistically deletes last AI action with no rollback on failure - `frontend/src/pages/Play.jsx:475` - Failed retry (409/network) leaves UI missing an action that still exists server-side. - Fix: restore the removed action in the catch path (or only remove on first stream event). -### 19. [pending] Settings test()/save() have no error handling → stuck on "Testing…" +### 19. [fixed] Settings test()/save() have no error handling → stuck on "Testing…" - `frontend/src/pages/Settings.jsx:66` - Rejection leaves `{pending:true}` forever + unhandled rejection. - Fix: try/catch → setTestResult({ok:false, error:msg}). -### 20. [pending] InsightsPanel race: slow earlier request overwrites newer report +### 20. [fixed] InsightsPanel race: slow earlier request overwrites newer report - `frontend/src/pages/Play.jsx:302` - No staleness guard; slow getAdventureContext can clobber a newer action snapshot. - Fix: track a request id / cancelled flag in the effect. -### 21. [pending] extractPlaceholders ignores ${...} in story-card trigger keys +### 21. [fixed] extractPlaceholders ignores ${...} in story-card trigger keys - `frontend/src/pages/Scenarios.jsx:50` - Backend fills placeholders in card.keys but the modal never prompts for those names → literal `${hero}` keys never match. - Fix: also scan card.keys when collecting placeholder names. diff --git a/backend/app/memorybank.py b/backend/app/memorybank.py index 6975ad5..fd49767 100644 --- a/backend/app/memorybank.py +++ b/backend/app/memorybank.py @@ -71,6 +71,10 @@ def embedding_provider(settings: models.Settings) -> OpenAICompatibleProvider: def cosine(a: list[float], b: list[float]) -> float: + # Different lengths means the embedding model changed since this vector was + # stored; zip() would silently score garbage. + if len(a) != len(b): + return 0.0 dot = sum(x * y for x, y in zip(a, b)) norm = math.sqrt(sum(x * x for x in a)) * math.sqrt(sum(y * y for y in b)) return dot / norm if norm else 0.0 @@ -118,10 +122,12 @@ async def retrieve_memories( key=lambda pair: pair[0], reverse=True, ) - # Pinned memories are always used; the rest fill up to top_k by similarity. + # Pinned memories are always used and count toward top_k, so the injected + # set never exceeds the configured budget (unless pinned alone exceed it). top_k = max(1, settings.memory_top_k) used = [(score, m) for score, m in scored if m.pinned] - used += [(score, m) for score, m in scored if not m.pinned][:top_k] + remaining = max(0, top_k - len(used)) + used += [(score, m) for score, m in scored if not m.pinned][:remaining] used.sort(key=lambda pair: pair[0], reverse=True) if update_stats: @@ -162,6 +168,11 @@ async def run_post_turn(adventure_id: int) -> None: settings = db.get(models.Settings, 1) if adventure is None or settings is None: return + # Undo/retry can shrink the action list below a stored cursor, which + # would stall summarization until the story grew past it again. + count = len(story_actions(adventure)) + adventure.memory_cursor = min(adventure.memory_cursor, count) + adventure.summary_cursor = min(adventure.summary_cursor, count) if adventure.auto_summarize: await _create_due_memories(adventure, settings, db) await _update_story_summary(adventure, settings, db) @@ -213,10 +224,17 @@ async def _update_story_summary( # Fold in memories covering the uncovered stretch; fall back to raw story # text if memory creation is lagging (e.g. it just failed). + # summary_cursor is a position into story_actions(); Memory.source_end is + # an Action.index. Translate the cursor to an index boundary before + # comparing — the two spaces diverge once actions are deleted or empty. + if adventure.summary_cursor < len(actions): + boundary = actions[adventure.summary_cursor].index + else: + boundary = actions[-1].index + 1 if actions else 0 new_events = [ m.text for m in adventure.memories - if m.source_end is not None and m.source_end >= adventure.summary_cursor + if m.source_end is not None and m.source_end >= boundary ] if new_events: events_text = "\n".join(f"- {t}" for t in new_events) diff --git a/backend/app/providers/openai_compatible.py b/backend/app/providers/openai_compatible.py index 5ac9313..d1c283d 100644 --- a/backend/app/providers/openai_compatible.py +++ b/backend/app/providers/openai_compatible.py @@ -119,9 +119,16 @@ class OpenAICompatibleProvider(Provider): if resp.status_code != 200: detail = (await resp.aread()).decode(errors="replace")[:500] raise ProviderError(self._friendly_http_error(resp.status_code, detail)) + # Some servers ignore stream=true and return one plain JSON + # body; buffer non-SSE lines so we can fall back to it. + saw_sse = False + raw_lines: list[str] = [] async for line in resp.aiter_lines(): if not line.startswith("data:"): + if not saw_sse: + raw_lines.append(line) continue + saw_sse = True data = line[5:].strip() if data == "[DONE]": debuglog.finish_entry(log, response="".join(received)) @@ -137,6 +144,27 @@ class OpenAICompatibleProvider(Provider): if chunk: received.append(chunk) yield "text", chunk + if not saw_sse: + body_text = "\n".join(raw_lines).strip() + try: + payload = json.loads(body_text) + except ValueError: + raise ProviderError( + "AI endpoint returned neither an SSE stream nor JSON: " + + body_text[:200] + ) + reasoning = self._extract_reasoning(payload) + if reasoning: + yield "reasoning", reasoning + chunk = self._extract_chunk(payload) + if chunk: + received.append(chunk) + yield "text", chunk + if not received: + raise ProviderError( + "AI endpoint returned a response with no text: " + + body_text[:200] + ) debuglog.finish_entry(log, response="".join(received)) except httpx.ConnectError as exc: error = f"Could not connect to {self.base_url} — is the AI server running?" diff --git a/backend/app/routers/scenarios.py b/backend/app/routers/scenarios.py index b8eb721..8ee4a1b 100644 --- a/backend/app/routers/scenarios.py +++ b/backend/app/routers/scenarios.py @@ -130,7 +130,13 @@ def import_scenario(bundle: dict = Body(...), db: Session = Depends(get_db)): db.add(scenario) db.flush() - cards = bundle.get("storyCards") or bundle.get("worldInfo") or [] + # AI Dungeon exports have used all three names for the same list. + cards = ( + bundle.get("storyCards") + or bundle.get("worldInfo") + or bundle.get("worldInformation") + or [] + ) for card in cards: if not isinstance(card, dict): continue diff --git a/backend/app/routers/settings.py b/backend/app/routers/settings.py index 047e17f..a25d173 100644 --- a/backend/app/routers/settings.py +++ b/backend/app/routers/settings.py @@ -25,8 +25,17 @@ def read_settings(db: Session = Depends(get_db)): @router.put("", response_model=schemas.SettingsOut) def update_settings(payload: schemas.SettingsUpdate, db: Session = Depends(get_db)): settings = get_settings(db) - for field, value in payload.model_dump(exclude_unset=True).items(): + fields = payload.model_dump(exclude_unset=True) + embedding_model_changed = ( + "embedding_model" in fields + and fields["embedding_model"] != settings.embedding_model + ) + for field, value in fields.items(): setattr(settings, field, value) + if embedding_model_changed: + # Vectors from the old model have a different dimensionality/space; + # clear them so the post-turn task re-embeds with the new model. + db.query(models.Memory).update({"embedding": None}) db.commit() return settings @@ -52,6 +61,6 @@ async def test_connection(db: Session = Depends(get_db)): try: data = resp.json() models_available = [m.get("id", "?") for m in data.get("data", [])] - except ValueError: - pass + except (ValueError, AttributeError, TypeError): + pass # non-JSON or unexpected shape — connectivity is still confirmed return {"ok": True, "models": models_available} diff --git a/backend/app/scripting/engine.py b/backend/app/scripting/engine.py index 91caec8..80defcf 100644 --- a/backend/app/scripting/engine.py +++ b/backend/app/scripting/engine.py @@ -31,6 +31,8 @@ function log(msg) { } var console = { log: log }; +// Returns the new card's index, or false if a card with those keys exists — +// matching real AI Dungeon. Note index 0 is falsy; that quirk is upstream's. function addStoryCard(keys, entry, type) { for (var i = 0; i < storyCards.length; i++) { if (storyCards[i].keys === keys) return false; diff --git a/backend/app/scripting/pipeline.py b/backend/app/scripting/pipeline.py index 9b677ac..a14555a 100644 --- a/backend/app/scripting/pipeline.py +++ b/backend/app/scripting/pipeline.py @@ -45,6 +45,7 @@ class ScriptPipeline: def _apply_cards(self, returned: list) -> None: existing = {c.id: c for c in self.adventure.story_cards} seen_ids = set() + added = 0 for item in returned: if not isinstance(item, dict): continue @@ -56,13 +57,14 @@ class ScriptPipeline: seen_ids.add(card_id) card = existing[card_id] card.keys, card.entry, card.type = keys, entry, card_type - elif len(existing) + len(seen_ids) < MAX_STORY_CARDS: + elif len(existing) + added < MAX_STORY_CARDS: self.db.add( models.StoryCard( adventure_id=self.adventure.id, keys=keys, entry=entry, type=card_type, ) ) + added += 1 for card_id, card in existing.items(): if card_id not in seen_ids: self.db.delete(card) diff --git a/frontend/src/pages/Play.jsx b/frontend/src/pages/Play.jsx index e099722..61f4100 100644 --- a/frontend/src/pages/Play.jsx +++ b/frontend/src/pages/Play.jsx @@ -50,12 +50,17 @@ const SECTION_LABELS = { } function PlotPanel({ adventure, setAdventure }) { - const saveTimer = useRef(null) + // One timer per field/card: a single shared timer would cancel the pending + // save of whatever was edited previously within the debounce window. + const saveTimers = useRef(new Map()) + const debounceSave = (key, fn) => { + clearTimeout(saveTimers.current.get(key)) + saveTimers.current.set(key, setTimeout(fn, 600)) + } const setField = (field, value) => { setAdventure({ ...adventure, [field]: value }) - clearTimeout(saveTimer.current) - saveTimer.current = setTimeout(() => api.updateAdventure(adventure.id, { [field]: value }), 600) + debounceSave(field, () => api.updateAdventure(adventure.id, { [field]: value })) } const addCard = async () => { @@ -68,12 +73,11 @@ function PlotPanel({ adventure, setAdventure }) { ...adventure, story_cards: adventure.story_cards.map((c) => (c.id === card.id ? card : c)), }) - clearTimeout(saveTimer.current) - saveTimer.current = setTimeout(() => { + debounceSave(`card-${card.id}`, () => { api.updateStoryCard(card.id, { name: card.name, type: card.type, keys: card.keys, entry: card.entry, notes: card.notes, }) - }, 600) + }) } const deleteCard = async (cardId) => { @@ -324,11 +328,15 @@ function InsightsPanel({ advId, inspectActionId, onClearInspect, refreshKey }) { const [error, setError] = useState(null) useEffect(() => { + let stale = false // a slow earlier request must not clobber a newer one setError(null) const load = inspectActionId ? api.getActionContext(advId, inspectActionId) : api.getAdventureContext(advId) - load.then(setReport).catch((err) => { setReport(null); setError(err.message) }) + load + .then((r) => { if (!stale) setReport(r) }) + .catch((err) => { if (!stale) { setReport(null); setError(err.message) } }) + return () => { stale = true } }, [advId, inspectActionId, refreshKey]) if (error) return
{error}
@@ -502,6 +510,11 @@ export default function Play() { function send(type = mode) { const text = input.trim() + if (type === 'continue') { + // Continue never consumes typed text — leave it in the box. + runTurn((signal) => api.sendAction(id, { type: 'continue', text: '' }, handleEvent, signal)) + return + } const payload = { type: text ? type : 'continue', text } setInput('') runTurn((signal) => api.sendAction(id, payload, handleEvent, signal)) @@ -510,7 +523,16 @@ export default function Play() { function retry() { setActions((prev) => prev.length && prev[prev.length - 1].type === 'ai' ? prev.slice(0, -1) : prev) - runTurn((signal) => api.retry(id, handleEvent, signal)) + runTurn(async (signal) => { + try { + await api.retry(id, handleEvent, signal) + } catch (err) { + // Failed retry (409, network): the optimistically removed action may + // still exist server-side — resync instead of guessing. + api.getAdventure(id).then((adv) => setActions(adv.actions)).catch(() => {}) + throw err + } + }) } async function undo() { diff --git a/frontend/src/pages/ScenarioEditor.jsx b/frontend/src/pages/ScenarioEditor.jsx index 3a79589..ecc5522 100644 --- a/frontend/src/pages/ScenarioEditor.jsx +++ b/frontend/src/pages/ScenarioEditor.jsx @@ -9,7 +9,13 @@ export default function ScenarioEditor() { const [scenario, setScenario] = useState(null) const [allScripts, setAllScripts] = useState([]) const [status, setStatus] = useState('') - const saveTimer = useRef(null) + // One timer per field/card: a single shared timer would cancel the pending + // save of whatever was edited previously within the debounce window. + const saveTimers = useRef(new Map()) + const debounceSave = (key, fn) => { + clearTimeout(saveTimers.current.get(key)) + saveTimers.current.set(key, setTimeout(fn, 600)) + } useEffect(() => { api.getScenario(id).then(setScenario).catch(() => navigate('/scenarios')) @@ -19,12 +25,11 @@ export default function ScenarioEditor() { const setField = (field, value) => { const next = { ...scenario, [field]: value } setScenario(next) - clearTimeout(saveTimer.current) - saveTimer.current = setTimeout(async () => { + debounceSave(field, async () => { await api.updateScenario(id, { [field]: value }) setStatus('Saved') setTimeout(() => setStatus(''), 1500) - }, 600) + }) } const addCard = async () => { @@ -37,12 +42,11 @@ export default function ScenarioEditor() { ...scenario, story_cards: scenario.story_cards.map((c) => (c.id === card.id ? card : c)), }) - clearTimeout(saveTimer.current) - saveTimer.current = setTimeout(() => { + debounceSave(`card-${card.id}`, () => { api.updateStoryCard(card.id, { name: card.name, type: card.type, keys: card.keys, entry: card.entry, notes: card.notes, }) - }, 600) + }) } const deleteCard = async (cardId) => { diff --git a/frontend/src/pages/Scenarios.jsx b/frontend/src/pages/Scenarios.jsx index e8d20a5..20e8800 100644 --- a/frontend/src/pages/Scenarios.jsx +++ b/frontend/src/pages/Scenarios.jsx @@ -49,7 +49,8 @@ export default function Scenarios() { const scenario = await api.getScenario(scenarioId) const names = extractPlaceholders( scenario.prompt, scenario.memory, scenario.authors_note, scenario.ai_instructions, - ...scenario.story_cards.map((c) => c.entry), + // Cards can carry ${placeholders} in trigger keys too, not just entries. + ...scenario.story_cards.flatMap((c) => [c.keys, c.entry]), ) if (names.length === 0) return begin(scenarioId) setPending({ scenario, names }) diff --git a/frontend/src/pages/Settings.jsx b/frontend/src/pages/Settings.jsx index 5d1c1d2..6165a31 100644 --- a/frontend/src/pages/Settings.jsx +++ b/frontend/src/pages/Settings.jsx @@ -56,16 +56,23 @@ export default function Settings() { const setField = (field, value) => setSettings({ ...settings, [field]: value }) const save = async () => { - await api.updateSettings(settings) - setSaved('Settings saved') - setTimeout(() => setSaved(''), 2000) + try { + await api.updateSettings(settings) + setSaved('Settings saved') + } catch (err) { + setSaved(`Save failed: ${err.message}`) + } + setTimeout(() => setSaved(''), 4000) } const test = async () => { setTestResult({ pending: true }) - await api.updateSettings(settings) - const result = await api.testConnection() - setTestResult(result) + try { + await api.updateSettings(settings) + setTestResult(await api.testConnection()) + } catch (err) { + setTestResult({ ok: false, detail: err.message }) + } } if (!settings) return null