parththakkar106andClaude Opus 5
40d2555f84
The README, the project page and the engineering guide all describe a linear story. The tree shipped two days ago. Every published surface is a phase behind, and the guide is not merely behind — it is wrong in a way that costs a reader time. Its 2.2 was "Two coordinate systems, and the bug class they create", and it explained the codebase through position_of_index, note_action_removed and settled_story_actions. All three were deleted in SP3. 2.3 explained retry through Action.variants and state_before. Somebody reading either would go looking for machinery that is not there, which is worse than a gap. So 2.2 is now "The story is a tree", written at the depth 1.2 and 1.3 are written at: the seven bugs that turned out to be one bug, the lineage clause and the two properties that make fork count free, why takes group by parent_id rather than by coordinate, cursors becoming anchors, and a closing list of what the design is honest about. 2.3 is rewritten around state_after and takes, and 1.1 and 1.5 follow, because the pipeline no longer snapshots before the call and the memory bank no longer holds an action back. The numbers were simply old: 151 tests where there are 440, 37 migrations where there are 64, twelve phases where there are fourteen. They appear in four places across the README, the project page's stat tiles and the guide's results table. The measured branch cost — 103 B, and 1.007x the page load of the same story flat — is added beside the egress and turn-cost figures it belongs with, since it is the number that answers "what does branching cost me". Three screenshots, on a new tools/shots_fixture.py: the Bandit Camp demo driven through eight written turns with written deltas, three discarded takes forked onto branches of their own, one off a branch so the map has to nest. Same reason tree_fixture.py is committed — the shots have to be reproducible and the frontend still has no test runner. play-world-state.jpg is reshot because it predates the entire tree UI; the map and the branches panel are new. Note for next time: docs/guide.html is hand-written, not generated from the Markdown, so every guide edit is two edits in two vocabularies. Both files were checked for tag balance and both pages rendered locally before this landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfMCsN1KBLsTqMkj5hSgrY
148 lines
10 KiB
Markdown
148 lines
10 KiB
Markdown
# Self-review log
|
||
|
||
**Every correctness bug on this page is resolved** — 20 fixed, 1 intentionally skipped with the
|
||
reasoning recorded below. The record is kept because the reasoning outlives the verdicts;
|
||
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` = applied, `skipped` = intentionally not fixed, `pending` = outstanding
|
||
(none remain).
|
||
|
||
**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. [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. [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. [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. [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. [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. [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. [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. [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. [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. [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. [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 caught; `data.get`/`m.get` on non-dict raises AttributeError → 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's dropped and not 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` 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. [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' (backend ignores 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`
|
||
- 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. [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. [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. [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.
|
||
|
||
## Cleanup backlog (reuse / simplification / efficiency / altitude — not bugs, apply later)
|
||
|
||
- **R1** `frontend/src/pages/Play.jsx:26` + `ScenarioEditor.jsx:19` + `ScriptEditor.jsx:34` — three copies of the debounced-autosave + story-card handlers; extract a `useDebouncedSave` hook / shared StoryCardList component. (Fixing bugs #15/#16 properly may accomplish this.)
|
||
- **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() body building; add a `stream` param to _request().
|
||
- **R4** `backend/app/routers/adventures.py:476` — six copies of child-resource get+owner-check+404; extract `get_owned_or_404`.
|
||
- **R5** `frontend/src/api.js:26` — streamSSE duplicates request()'s error extraction; extract `throwIfNotOk(resp)`.
|
||
- **S1** `backend/app/models.py:210` — `Settings.stream` is dead state (never read); delete column + schema fields.
|
||
- **S2** `frontend/src/pages/Play.jsx:6` — MODES and PLAYER_TYPES are identical constants; lastIsAi/canUndo computed twice.
|
||
- **E1** `backend/app/context/builder.py:119` — joins+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 + a blocking commit per script per hook; build once per hook, slice to HISTORY_WINDOW first, commit once.
|
||
- **E3** `frontend/src/pages/Play.jsx:561` — every SSE chunk re-renders all action rows; isolate streaming text in a child component / React.memo rows.
|
||
- **E4** `backend/app/context/builder.py:48` — Section.tokens uncached, whole context tokenized 2-3×/turn; cache counts, sum sections.
|
||
- **E5** `frontend/src/pages/Play.jsx:490` — keydown effect has no dep array → listener re-registered every render.
|
||
- **E6** `backend/app/memorybank.py:182` — catch-up summarization awaits blocks sequentially; gather independent blocks.
|
||
- **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 be a constraint on a corpse.
|
||
- **A2** `backend/app/providers/openai_compatible.py:45` — CHAT_CONTINUE_HINT appended below the budgeting layer; assemble prompts in context builder.
|
||
- **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.
|
||
- **A5** `frontend/src/pages/Home.jsx:87` — client appends 'Z' to naive datetimes; emit ISO-8601 with offset from the API instead.
|