Fix remaining code-review findings (15 bugs; #11 skipped as AID-compatible)
Backend: - provider: fall back to parsing a plain JSON body when a server ignores stream=true (was: silent empty turn); error if response has no text (#5) - memorybank: clamp cursors after undo/retry shrinks the action list, and translate summary_cursor (list position) to an Action.index boundary before comparing with Memory.source_end (#7) - memorybank: pinned memories now count toward the top_k budget (#8) - memorybank: cosine() returns 0.0 on dimension mismatch; changing the embedding model clears stored vectors so they re-embed (#9) - scripting: MAX_STORY_CARDS cap now counts cards inserted during the hook, so a script can't add unbounded cards in one turn (#10) - settings: /test tolerates non-dict JSON from /models (#13) - scenarios: import accepts worldInformation as a story-card source (#14) Frontend: - per-key debounce timers in PlotPanel and ScenarioEditor — editing two things within 600ms no longer drops the first save (#15, #16) - Continue button no longer discards typed input (#17) - failed retry resyncs actions from the server instead of leaving the removed action missing (#18) - Settings save/test surface errors instead of hanging on Testing… (#19) - InsightsPanel ignores stale responses from superseded requests (#20) - placeholder scan includes story-card trigger keys (#21) addStoryCard returning the 0-based index (falsy for the first card) matches real AI Dungeon per the scripting guidebook — kept, documented (#11). Statuses updated in CODE_REVIEW_FINDINGS.md; stale entries for previously fixed items (#1-4, #6, #12) corrected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFsGHju9szibJJa2YJcdbg
This commit is contained in:
parththakkar106
co-authored by
Claude Fable 5
Claude Fable 5
parent
3ee653a631
commit
253b533d3b
+29
-21
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user