parththakkar106andClaude Opus 5
e0bf2b61d9
Two items from Stage 2 of `plan/17-refactor.md`. `frontend/src/hooks/useDebouncedSave.js` replaces three copies of the same debounce. `PlotPanel` and `ScenarioEditor` held identical per-key timer maps. `ScriptEditor` held a single shared timer, so editing two fields inside the same 600 ms window canceled the first save. The hook gives every key its own timer, which fixes that. `Settings.stream` was dead state. Nothing read it and every turn streams. This removes the column, both schema fields, and adds migration 65 to drop it. It is item S1 in `docs/self-review.md`. Migration 65 needs a new guard. `_column_already_gone` is the counterpart to `_column_already_there`: `create_all` builds the current schema, which is already missing every dropped column, so a fixture that stamps an old version and replays would fail on a column that is not there. Verified: 549 backend tests pass, lint and build are clean, and migration 65 runs both ways, once against a database that still has the column and once against one that does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Dix4oGV3njgWRdu7P9t6r
157 lines
11 KiB
Markdown
157 lines
11 KiB
Markdown
# Self-review log
|
|
|
|
**Every correctness bug on this page is resolved.** 20 are fixed, and 1 is intentionally
|
|
skipped, with the reasoning recorded below. The record is kept because the reasoning outlives
|
|
the verdicts, and several of these are traps worth remembering. The *cleanup backlog* at the
|
|
bottom is a deliberately open list of non-bugs (reuse, simplification, efficiency), not
|
|
outstanding defects.
|
|
|
|
Original review: 2026-07-05, whole project in scope (no git history at the time).
|
|
Status key: `fixed` means applied, `skipped` means intentionally not fixed, and `pending` means
|
|
outstanding (none remain).
|
|
|
|
**2026-07-06 update (branch `bugfix-code-review`):** every finding was re-verified against
|
|
current code. #1, #2, #3, #4, #6, and #12 had already been fixed in earlier sessions, so their
|
|
statuses were stale. #5, #7, #8, #9, #10, #13, #14 (backend) and #15 through #21 (frontend)
|
|
were 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. This is documented in `engine.py`'s prelude instead. The cleanup backlog
|
|
(R/S/E/A items) below remains open by design.
|
|
|
|
## Correctness bugs
|
|
|
|
### 1. [fixed] seed_demo.py doesn't stamp schema version → server crashes on next start
|
|
- `backend/seed_demo.py:10`
|
|
- A fresh DB created via `Base.metadata.create_all` leaves `PRAGMA user_version` at 0. The next server start sees the tables exist and replays every ALTER TABLE migration, causing a `duplicate column name` crash.
|
|
- Fix: stamp user_version to latest after create_all (reuse migrations.bootstrap logic).
|
|
|
|
### 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-clicking Continue sends both requests past the 409 check, producing duplicate Action.index rows and interleaved generations.
|
|
- Fix: atomically test-and-set the lock in the request phase, release in the stream's `finally`.
|
|
|
|
### 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, so duplicate indexes can survive the "repair".
|
|
- Fix: compute new indexes in Python (SELECT ordered, then UPDATE per row).
|
|
|
|
### 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 then attach a future script to an old scenario.
|
|
- Fix: `PRAGMA foreign_keys=ON` via engine connect event.
|
|
|
|
### 5. [fixed] Provider generate() silently yields nothing for non-SSE 200 responses
|
|
- `backend/app/providers/openai_compatible.py:84`
|
|
- A server that ignores `stream=true` and returns plain JSON produces no `data:` lines, so the result is an empty AI action with no error.
|
|
- Fix: buffer the non-SSE body and fall back to parsing it as a single JSON completion.
|
|
|
|
### 6. [fixed] Fire-and-forget asyncio task can be GC'd mid-run and wedge the memory bank
|
|
- `backend/app/memorybank.py:146`
|
|
- The result of `asyncio.create_task` is not referenced, so the task can vanish silently and leave an adventure ID stuck in `_running`.
|
|
- Fix: keep strong refs in a set, discard in done-callback.
|
|
|
|
### 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 and positions shift, so summarization skips or 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. [fixed] Pinned memories don't count toward memory_top_k cap
|
|
- `backend/app/memorybank.py:120`
|
|
- 6 pinned plus top_k=5 injects 11 memories, blowing the token budget.
|
|
- Fix: fill with unpinned only up to `top_k - len(pinned)` (min 0).
|
|
|
|
### 9. [fixed] Embedding-model change → cosine() zips different-dimension vectors silently
|
|
- `backend/app/memorybank.py:69`
|
|
- Old 768-dimension embeddings scored against a new 1536-dimension query produce garbage similarity with no error, and are never re-embedded.
|
|
- Fix: return 0.0 on length mismatch (and ideally clear stale embeddings so _embed_pending redoes them).
|
|
|
|
### 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, since seen_ids is a subset of existing. A script can insert an unbounded number of cards in one turn.
|
|
- Fix: count inserts made during the loop.
|
|
|
|
### 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. [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. [fixed] test_connection 500s on non-dict JSON from /models
|
|
- `backend/app/routers/settings.py:54`
|
|
- Only ValueError is caught. `data.get`/`m.get` on non-dict input raises AttributeError, producing a 500 instead of `{ok:false}`.
|
|
- Fix: catch (ValueError, AttributeError, TypeError) or validate shapes.
|
|
|
|
### 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 is dropped without being reported.
|
|
- Fix: accept `worldInformation` as a card source too.
|
|
|
|
### 15. [fixed] Shared debounce timer loses edits (Play PlotPanel)
|
|
- `frontend/src/pages/Play.jsx:28`
|
|
- One `saveTimer` is shared by all plot fields and story-card saves. Editing a second field within 600ms cancels the first pending PATCH, causing silent data loss.
|
|
- Fix: per-key timers (e.g. a Map keyed by field/card id).
|
|
|
|
### 16. [fixed] Same shared-debounce data loss in ScenarioEditor
|
|
- `frontend/src/pages/ScenarioEditor.jsx:22`
|
|
- Same fix as #15.
|
|
|
|
### 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' (the backend ignores the text) and clears the input.
|
|
- Fix: don't clear input on continue (or treat non-empty input as a normal send).
|
|
|
|
### 18. [fixed] retry() optimistically deletes last AI action with no rollback on failure
|
|
- `frontend/src/pages/Play.jsx:475`
|
|
- A failed retry (409 or network error) leaves the 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. [fixed] Settings test()/save() have no error handling → stuck on "Testing…"
|
|
- `frontend/src/pages/Settings.jsx:66`
|
|
- A rejection leaves `{pending:true}` forever and produces an unhandled rejection.
|
|
- Fix: try/catch → setTestResult({ok:false, error:msg}).
|
|
|
|
### 20. [fixed] InsightsPanel race: slow earlier request overwrites newer report
|
|
- `frontend/src/pages/Play.jsx:302`
|
|
- There is no staleness guard, so a slow getAdventureContext call can overwrite a newer action snapshot.
|
|
- Fix: track a request id or cancelled flag in the effect.
|
|
|
|
### 21. [fixed] extractPlaceholders ignores ${...} in story-card trigger keys
|
|
- `frontend/src/pages/Scenarios.jsx:50`
|
|
- The backend fills placeholders in card.keys, but the modal never prompts for those names, so a literal `${hero}` key never matches.
|
|
- Fix: also scan card.keys when collecting placeholder names.
|
|
|
|
## Cleanup backlog (reuse / simplification / efficiency / altitude — not bugs, apply later)
|
|
|
|
- **R1** ~~three copies of the debounced-autosave handler.~~
|
|
**Half applied in phase 17 (2026-08).** All three use
|
|
`frontend/src/hooks/useDebouncedSave.js`. A shared StoryCardList component is
|
|
still open.
|
|
- **R2** `backend/seed_demo.py:228`: re-implements create_adventure. Call the router logic instead.
|
|
- **R3** `backend/app/providers/openai_compatible.py:122`: complete() duplicates _request()'s body building. Add a `stream` param to _request().
|
|
- **R4** ~~six copies of child-resource get, owner-check, and 404.~~
|
|
**Applied in phase 17 (2026-08).** All 32 handlers take the `current_adventure`
|
|
dependency from `routers/adventures/deps.py`.
|
|
- **R5** `frontend/src/api.js:26`: streamSSE duplicates request()'s error extraction. Extract `throwIfNotOk(resp)`.
|
|
- **S1** ~~`Settings.stream` is dead state (never read).~~
|
|
**Applied in phase 17 (2026-08).** The column, both schema fields, and migration 65
|
|
drop it. `R4` went with it: the ownership check is the `current_adventure` dependency.
|
|
- **S2** `frontend/src/pages/Play.jsx:6`: MODES and PLAYER_TYPES are identical constants; lastIsAi/canUndo are computed twice.
|
|
- **E1** `backend/app/context/builder.py:119`: joins and tokenizes the entire adventure history every turn for the trigger window. Walk reversed(actions) until budget instead.
|
|
- **E2** `backend/app/scripting/pipeline.py:77`: rebuilds full history dicts, JSON, and a blocking commit per script per hook. Build once per hook, slice to HISTORY_WINDOW first, and commit once.
|
|
- **E3** `frontend/src/pages/Play.jsx:561`: every SSE chunk re-renders all action rows. Isolate streaming text in a child component or React.memo rows.
|
|
- **E4** `backend/app/context/builder.py:48`: Section.tokens is uncached, so the whole context gets tokenized 2 to 3 times per turn. Cache counts and sum sections.
|
|
- **E5** `frontend/src/pages/Play.jsx:490`: the keydown effect has no dependency array, so the listener is re-registered every render.
|
|
- **E6** `backend/app/memorybank.py:182`: catch-up summarization awaits blocks sequentially. Gather independent blocks instead.
|
|
- **A1** ~~no UniqueConstraint('adventure_id','index'); index allocation is ad-hoc per writer.~~
|
|
**Overtaken by phase 14 (2026-08).** `index` is a legacy column that nothing reads: ordering
|
|
is `(branch_id, depth)` now, allocated in one place (`tree.place_action`). The column is kept
|
|
unread for one release and then dropped, so a constraint on it would apply to a column that
|
|
no longer does anything.
|
|
- **A2** `backend/app/providers/openai_compatible.py:45`: CHAT_CONTINUE_HINT is appended below the budgeting layer. Assemble prompts in the context builder instead.
|
|
- **A3** `backend/app/routers/adventures.py:390`: import endpoints hand-coerce raw dicts. Use a Pydantic bundle schema.
|
|
- **A4** `backend/app/routers/adventures.py:207`: onModelContext flattens (system, story) and ships everything as user content if modified. Pass structure through the hook instead.
|
|
- **A5** `frontend/src/pages/Home.jsx:87`: the client appends 'Z' to naive datetimes. Emit ISO-8601 with an offset from the API instead.
|