Rewrite docs and READMEs in Google developer documentation style
Trim em dashes, convert first-person plural to second person, and tighten sentences in README.md, docs/GUIDE.md, docs/self-review.md, and frontend/README.md, matching the style already applied to code comments. No technical content, numbers, or code blocks changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N7vUFpYkLrJnuSgwcRjqSx
This commit is contained in:
parththakkar106
committed by
Parth
Parth
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
e7d75c3b05
commit
446ceb6cab
+164
-163
@@ -108,19 +108,19 @@ player input
|
||||
Two design choices are visible in that list before any of the details.
|
||||
|
||||
**The prompt is snapshotted, not reconstructed.** Every AI action stores the exact text
|
||||
that was sent to the model. That's what powers the Insights panel — open any turn and see
|
||||
that was sent to the model. That's what powers the Insights panel: open any turn and see
|
||||
each context component, its token cost, and why it was included. It's also what makes
|
||||
prompt bugs findable. The cost is storage (~74 KB per turn), which turns into a real
|
||||
performance problem later — see [2.5](#25-the-189x-egress-fix).
|
||||
performance problem later. See [2.5](#25-the-189x-egress-fix).
|
||||
|
||||
**Every node records the state it leaves behind.** `state_after` and `world_state_after`
|
||||
are stapled onto the action once its hooks and its delta have run, so a node carries the
|
||||
scoreboard and the RPG stats as they stood when that turn finished. Rewinding to *before*
|
||||
a turn is then a read of the node in front of it, which is the same move as switching to
|
||||
another branch — one mechanism, and it is the whole reason undo, retry and branch
|
||||
switching all put the numbers back rather than only rewriting text. (These were `*_before`
|
||||
pictures originally; a tree wants the *after*, because a branch's tip is what a reader
|
||||
standing on it should see.)
|
||||
are attached to the action once its hooks and its delta have run, so a node carries the
|
||||
stats and the RPG values as they stood when that turn finished. Rewinding to *before*
|
||||
a turn is then a read of the node in front of it, which is the same operation as switching
|
||||
to another branch: one mechanism, and the reason undo, retry, and branch switching all put
|
||||
the numbers back instead of only rewriting text. (These were `*_before` fields originally; a
|
||||
tree needs the *after* value, because a branch's tip is what a reader standing on it should
|
||||
see.)
|
||||
|
||||
---
|
||||
|
||||
@@ -138,7 +138,7 @@ whether the story stays coherent.
|
||||
|
||||
Send the last N turns. That breaks in two directions: N turns of short exchanges wastes the
|
||||
window, and N turns of long ones overflows it. It also throws away the things that matter
|
||||
most — the premise, the character sheet, the fact that you promised the innkeeper you'd
|
||||
most: the premise, the character sheet, the fact that you promised the innkeeper you'd
|
||||
return.
|
||||
|
||||
### What this app does
|
||||
@@ -149,12 +149,12 @@ Fixed (always included, whatever they cost):
|
||||
|
||||
| Section | What it is |
|
||||
|---|---|
|
||||
| `narrator` | The system prompt — how to write. |
|
||||
| `narrator` | The system prompt: how to write. |
|
||||
| `world_state_guide` | The stat legend: what each stat means, its range, its bands. |
|
||||
| `world_state` | Current values of every stat, plus NPCs in scene. |
|
||||
| `world_state_rule` | How to report changes. |
|
||||
| `ai_instructions` | Per-adventure steering. |
|
||||
| `plot_essentials` | AI Dungeon's "Memory" — the premise. |
|
||||
| `plot_essentials` | AI Dungeon's "Memory": the premise. |
|
||||
| `story_summary` | The auto-maintained running summary. |
|
||||
| `used_memories` | Top-K retrievals from the memory bank. |
|
||||
|
||||
@@ -184,7 +184,7 @@ of "the model has no idea what just happened". Cards that don't fit are still *r
|
||||
to Insights with `included: false`, so the UI can show the lore that got squeezed out.
|
||||
|
||||
**History fills newest-first and stops.** Oldest turns fall out. This is the right
|
||||
direction because the old material is not actually lost — it has been summarized into
|
||||
direction because the old material is not actually lost: it has been summarized into
|
||||
memories and the running summary, which are in the fixed section.
|
||||
|
||||
**If even the single newest turn is over budget, it gets hard-truncated** rather than
|
||||
@@ -193,8 +193,8 @@ of the last turn produces something.
|
||||
|
||||
**The author's note is injected 3 actions from the end**, not at the top.
|
||||
`AUTHORS_NOTE_DEPTH = 3`. Instructions placed near the end of a prompt have more influence
|
||||
on what comes next than instructions at the top — recency. The author's note is a steering
|
||||
control ("keep it tense"), so it goes where steering works.
|
||||
on what comes next than instructions at the top, because of recency. The author's note is a
|
||||
steering control ("keep it tense"), so it goes where steering works.
|
||||
|
||||
**The world-state reminder goes dead last.** The full emit rule lives up in the system
|
||||
block, hundreds of tokens away from where the model starts writing. A one-line reminder
|
||||
@@ -203,15 +203,15 @@ forgotten.
|
||||
|
||||
**Past AI turns get their state block re-attached.** The state block is stripped from the
|
||||
text before it's stored, so a replayed history would show the model twenty of its own past
|
||||
turns that *contain no state block* — which teaches it, by imitation, to stop emitting one.
|
||||
turns that *contain no state block*. That teaches it, by imitation, to stop emitting one.
|
||||
So `_history_text()` reconstructs the block from the stored delta and re-appends it when
|
||||
building history. The model sees its own pattern and keeps following it.
|
||||
|
||||
### The performance trap hiding in this
|
||||
|
||||
Building the context needs the newest ~6,000 tokens of story. The obvious implementation
|
||||
reads `adventure.actions` — which loads every row of the adventure — and then throws 90%
|
||||
of it away. At turn 200 that was 839 KB of database reads to use maybe 70 KB, and it grew
|
||||
reads `adventure.actions`, which loads every row of the adventure, and then throws 90% of
|
||||
it away. At turn 200 that was 839 KB of database reads to use maybe 70 KB, and it grew
|
||||
every single turn.
|
||||
|
||||
`backend/app/context/history.py` fixes it by serving three shapes directly from SQL: a
|
||||
@@ -225,13 +225,13 @@ projected = int(budget / average * 1.15) + 8
|
||||
```
|
||||
|
||||
Each round fetches only what it doesn't already hold, so no row is read twice. Result: the
|
||||
same turn costs 129 KB instead of 839 KB, and stops growing at around turn 50 — the cost
|
||||
is bounded by the context budget instead of by the length of the story.
|
||||
same turn costs 129 KB instead of 839 KB, and stops growing at around turn 50. The cost is
|
||||
bounded by the context budget instead of by the length of the story.
|
||||
|
||||
There's a second rule in that module worth naming: **if the actions are already loaded in
|
||||
memory, slice them instead of querying.** The scripting pipeline hands the whole history to
|
||||
user scripts (AI Dungeon's API requires it), so on a scripted adventure the rows are
|
||||
already there — issuing a query beside them would mean paying twice.
|
||||
user scripts (AI Dungeon's API requires it), so on a scripted adventure the rows are already
|
||||
there. Issuing a query beside them would mean paying twice.
|
||||
|
||||
---
|
||||
|
||||
@@ -241,19 +241,19 @@ Source: `backend/app/worldstate/engine.py`, `plan/12-phase-rpg-world-state.md`.
|
||||
|
||||
### The problem
|
||||
|
||||
You want an RPG layer — hit points, trust, quest progress. Who owns the numbers?
|
||||
You want an RPG layer: hit points, trust, quest progress. Who owns the numbers?
|
||||
|
||||
### Three options, and why two lose
|
||||
|
||||
**Option A: a deterministic dice engine.** The player types "attack the goblin", the
|
||||
engine rolls, applies damage, and the model narrates the result. This is what a real RPG
|
||||
does. It loses here because the action space is unbounded — the player can type anything,
|
||||
and mapping arbitrary natural language onto a fixed rules system is a harder problem than
|
||||
the one being solved.
|
||||
does. It loses here because the action space is unbounded: the player can type anything, and
|
||||
mapping arbitrary natural language onto a fixed rules system is a harder problem than the
|
||||
one being solved.
|
||||
|
||||
**Option B: the model owns the numbers.** Let it track hp in the prose and trust it. This
|
||||
fails immediately. Models are bad at arithmetic, worse at remembering a number across
|
||||
twenty turns, and completely unable to obey their own frequency rules — tell one "only
|
||||
twenty turns, and completely unable to obey their own frequency rules. Tell one "only
|
||||
change this every 5 turns" and it will change it every turn.
|
||||
|
||||
**Option C, chosen: the model proposes, the engine disposes.** The model narrates and
|
||||
@@ -279,10 +279,10 @@ The engine then applies, in order:
|
||||
| `max_delta_per_turn` | Losing 90 hp to a stubbed toe |
|
||||
| Clamp to `min`/`max` | Negative hp, trust above 100 |
|
||||
| Milestones are sticky, `true` only | Un-completing a quest |
|
||||
| Flags are two-way booleans | (Deliberately unrestricted — that's what flags are for) |
|
||||
| Flags are two-way booleans | (Deliberately unrestricted: that's what flags are for) |
|
||||
|
||||
Everything it rejects is *reported*, not silently swallowed — the Insights panel shows
|
||||
applied, clamped and rejected paths per turn, and the chip under each narration shows what
|
||||
Everything it rejects is *reported*, not silently swallowed. The Insights panel shows
|
||||
applied, clamped, and rejected paths per turn, and the chip under each narration shows what
|
||||
actually changed.
|
||||
|
||||
### The reliability mechanism: word bands
|
||||
@@ -295,33 +295,33 @@ A stat can carry **bands**:
|
||||
[60,90,"healthy"],[90,100,"full health"]] }
|
||||
```
|
||||
|
||||
Two things use them. The live state block shows the current band label — `hp 55/100 (minor
|
||||
damage)` — so the model reads a *word*, not just a number. And the stat guide shows the
|
||||
Two things use them. The live state block shows the current band label, `hp 55/100 (minor
|
||||
damage)`, so the model reads a *word*, not just a number. And the stat guide shows the
|
||||
whole ladder once per turn, so the model can see the full scale it's reasoning across.
|
||||
|
||||
The point: models reason well over semantics and badly over arithmetic. "He's badly hurt,
|
||||
so a solid hit should take him to very weak" is a judgement a model can make. "55 minus 22
|
||||
is 33" is one it will get wrong often enough to matter.
|
||||
so a solid hit should take him to very weak" is a judgment a model can make. "55 minus 22 is
|
||||
33" is one it will get wrong often enough to matter.
|
||||
|
||||
### The failure philosophy
|
||||
|
||||
Nothing in the world-state engine raises. A malformed delta returns `{}` and the turn
|
||||
continues. The parser is deliberately tolerant — it strips trailing commas and leading `+`
|
||||
continues. The parser is deliberately tolerant: it strips trailing commas and leading `+`
|
||||
signs on numbers, both of which weaker free models emit and strict JSON rejects. It accepts
|
||||
a fence labelled `state`, one labelled `json`, or an unlabelled one, and falls back to a
|
||||
bare JSON object hugging the end of the text — but only if it parses into something that
|
||||
looks like a delta, so prose ending in `}` is never eaten.
|
||||
a fence labeled `state`, one labeled `json`, or an unlabeled one, and falls back to a bare
|
||||
JSON object at the end of the text, but only if it parses into something that looks like a
|
||||
delta, so prose ending in `}` is never eaten.
|
||||
|
||||
This matters because the hosted demo runs on free-tier models. A stricter parser would mean
|
||||
a good model works and a free one doesn't.
|
||||
|
||||
### One call, not two
|
||||
|
||||
The model narrates *and* emits the delta in a single request. The alternative — narrate,
|
||||
then a second call to extract structured state — is more reliable per call and costs twice
|
||||
the latency and twice the rate-limit budget. On the free tier (20 requests/minute) that
|
||||
would halve the playable turn rate. The tolerant parser plus the terminal reminder was the
|
||||
cheaper way to buy the same reliability.
|
||||
The model narrates *and* emits the delta in a single request. The alternative, narrating and
|
||||
then making a second call to extract structured state, is more reliable per call and costs
|
||||
twice the latency and twice the rate-limit budget. On the free tier (20 requests/minute)
|
||||
that would halve the playable turn rate. The tolerant parser plus the terminal reminder was
|
||||
the cheaper way to buy the same reliability.
|
||||
|
||||
---
|
||||
|
||||
@@ -339,9 +339,9 @@ Tell the model its budget: *"keep this turn under about N words"*.
|
||||
|
||||
### What the measurement showed
|
||||
|
||||
Average turn length went from **174 words to 246** — every run longer than every unhinted
|
||||
run (n=5). Phrased as a budget, the number reads as a *target to fill*. The hint pushed
|
||||
turns toward the very wall it existed to protect.
|
||||
Average turn length went from **174 words to 246**, and every run was longer than every
|
||||
unhinted run (n=5). Phrased as a budget, the number reads as a *target to fill*. The hint
|
||||
pushed turns toward the very wall it existed to protect.
|
||||
|
||||
### The fix
|
||||
|
||||
@@ -361,12 +361,12 @@ Average came back to 170 words, and the state block survived at tight caps.
|
||||
words = int((max_output_tokens - 50) * 0.75 * 0.90)
|
||||
```
|
||||
|
||||
- `- 50` (`LENGTH_HEADROOM`) — tokens held back for the state block itself.
|
||||
- `* 0.75` (`WORDS_PER_TOKEN`) — models can't count their own tokens, but they do follow a
|
||||
- `- 50` (`LENGTH_HEADROOM`): tokens held back for the state block itself.
|
||||
- `* 0.75` (`WORDS_PER_TOKEN`): models can't count their own tokens, but they do follow a
|
||||
word budget. English prose is roughly 0.75 words per token.
|
||||
- `* 0.90` (`LENGTH_BUFFER`) — a word budget is a suggestion the model overshoots; the cap
|
||||
- `* 0.90` (`LENGTH_BUFFER`): a word budget is a suggestion the model overshoots, and the cap
|
||||
it protects is a hard wall. Aim 10% short so the overshoot lands in slack.
|
||||
- Below 40 words the hint is dropped entirely — it stops earning its tokens.
|
||||
- Below 40 words the hint is dropped entirely: it stops earning its tokens.
|
||||
|
||||
---
|
||||
|
||||
@@ -377,7 +377,7 @@ Source: `backend/app/memorybank.py`.
|
||||
### The problem
|
||||
|
||||
Story history falls out of the context window as the adventure grows. Turn 4 said you
|
||||
promised the innkeeper you'd return. At turn 90 that's long gone from the prompt — but if
|
||||
promised the innkeeper you'd return. At turn 90 that's long gone from the prompt, but if
|
||||
you walk back into the inn, it should come back.
|
||||
|
||||
### The three layers
|
||||
@@ -401,23 +401,23 @@ back into the prompt.
|
||||
|
||||
### The decisions inside it
|
||||
|
||||
**A memory hangs off the node whose block it ends on.** Not off the adventure, and not off
|
||||
a *position* in a list of actions — off a `(branch_id, depth)` coordinate. That is what
|
||||
**A memory attaches to the node whose block it ends on.** Not to the adventure, and not to
|
||||
a *position* in a list of actions, but to a `(branch_id, depth)` coordinate. That is what
|
||||
makes "which memories described this turn?" an indexed lookup rather than a scan for rows
|
||||
whose covered range has fallen off the end of the story, and it is what makes memories
|
||||
inherit correctly across a fork: the ones above the fork point already sit on ancestors
|
||||
both lines read.
|
||||
|
||||
It is also the repair. When a turn's text is replaced or removed — a retry, an undo, a
|
||||
deleted action — `forget_node` withdraws the memory hanging off that coordinate *and*
|
||||
This is also how repair works. When a turn's text is replaced or removed (a retry, an undo,
|
||||
a deleted action), `forget_node` withdraws the memory attached to that coordinate and
|
||||
rewinds both marks to just before the stretch it covered, so the ground is summarized again
|
||||
from what the story now says. An earlier version instead held the newest action back a turn
|
||||
so it could never be summarized before it stopped being retryable; that is no longer needed,
|
||||
because the repair exists whether or not the invalidation happens at the tip.
|
||||
so it could never be summarized before it stopped being retryable. That is no longer
|
||||
needed, because the repair exists whether or not the invalidation happens at the tip.
|
||||
|
||||
**Cursors only advance on success.** Every AI call in this module is best-effort. If
|
||||
summarization fails, the function returns and the cursor is unchanged, so the same block is
|
||||
retried on a later turn. There is no retry loop, no dead-letter queue, no backoff — the
|
||||
retried on a later turn. There is no retry loop, no dead-letter queue, and no backoff: the
|
||||
cadence *is* the retry mechanism. Failures are logged to the debug page.
|
||||
|
||||
**Summarization is fire-and-forget, in a background task with its own DB session.** The
|
||||
@@ -437,7 +437,7 @@ truncate and score garbage silently. An explicit length check returns 0.0 instea
|
||||
|
||||
**Eviction is LRU-ish, and evicted memories are kept.** Over capacity (default 200), the
|
||||
least-used, least-recently-used unpinned memories are marked `forgotten` rather than
|
||||
deleted — so the UI can still show them and you can un-forget one.
|
||||
deleted, so the UI can still show them and you can un-forget one.
|
||||
|
||||
**Background calls never spend the shared demo key.** The summarization and embedding
|
||||
providers are built directly from the user's own settings and never from the demo config,
|
||||
@@ -454,7 +454,7 @@ anything makes a 20-second generation feel broken.
|
||||
|
||||
**Server-Sent Events (SSE)** is the mechanism: an HTTP response that stays open and pushes
|
||||
`data: {...}` lines as they become available. It's one-directional (server → browser),
|
||||
which is exactly what's needed here — WebSockets would be a bidirectional connection for a
|
||||
which is exactly what's needed here. WebSockets would be a bidirectional connection for a
|
||||
unidirectional problem.
|
||||
|
||||
The chain:
|
||||
@@ -473,7 +473,7 @@ text), `stopped` (a script blocked the turn), `error`, `done`.
|
||||
|
||||
Two production details that only show up when hosted:
|
||||
|
||||
- `X-Accel-Buffering: no` — nginx-style reverse proxies buffer responses by default, which
|
||||
- `X-Accel-Buffering: no`: nginx-style reverse proxies buffer responses by default, which
|
||||
turns a stream into one big delivery at the end. This header tells them to flush each
|
||||
event.
|
||||
- The security-headers and body-size middlewares are written as **pure ASGI** rather than
|
||||
@@ -481,7 +481,7 @@ Two production details that only show up when hosted:
|
||||
break streaming.
|
||||
|
||||
**The empty-reply case is diagnosed, not reported as "empty".** If a reasoning model
|
||||
streams thinking but no story text, it spent its whole budget thinking — the error says so
|
||||
streams thinking but no story text, it spent its whole budget thinking. The error says so
|
||||
and tells you which three settings to change.
|
||||
|
||||
---
|
||||
@@ -498,18 +498,18 @@ The safety properties are mostly structural:
|
||||
|
||||
| Property | How |
|
||||
|---|---|
|
||||
| No filesystem, network, or process access | QuickJS has none by default — nothing was removed, nothing was added |
|
||||
| No filesystem, network, or process access | QuickJS has none by default: nothing was removed, nothing was added |
|
||||
| Memory cap | 16 MB per run |
|
||||
| CPU cap | 2 seconds per run |
|
||||
| No shared state between runs | A fresh `Context` per hook execution |
|
||||
| A broken script can't break a turn | Every failure comes back as `.error` with text/state/cards unchanged; the pipeline logs it and continues |
|
||||
|
||||
Data crosses the boundary as JSON — Python serializes `{state, text, history, storyCards,
|
||||
Data crosses the boundary as JSON: Python serializes `{state, text, history, storyCards,
|
||||
info}` in, and the script's results out. There is no object bridge to exploit.
|
||||
|
||||
One deliberate bug-compatibility: `addStoryCard` returns the new card's *index*, so the
|
||||
first card returns `0`, which is falsy, so `if (!addStoryCard(...))` misfires. That's
|
||||
upstream AI Dungeon's behaviour. It's documented in the code and left alone, because
|
||||
upstream AI Dungeon's behavior. It's documented in the code and left alone, because
|
||||
matching real scripts is the whole point of the feature.
|
||||
|
||||
---
|
||||
@@ -517,7 +517,7 @@ matching real scripts is the whole point of the feature.
|
||||
## 1.8 Why there is no agent framework
|
||||
|
||||
Graph-based agent frameworks (LangGraph and similar) earn their complexity with
|
||||
**branching, cyclic, multi-step control flow** — a graph of nodes where the path depends on
|
||||
**branching, cyclic, multi-step control flow**: a graph of nodes where the path depends on
|
||||
what the model decides, with loops, retries, tool calls, and persisted state between steps.
|
||||
|
||||
This turn pipeline is a **fixed linear sequence with exactly one model call**. There is no
|
||||
@@ -533,13 +533,13 @@ abstraction, its serialization model, and its debugging surface to express a str
|
||||
There is also a specific reason a framework's context handling wouldn't fit: **the
|
||||
budgeting logic is the product.** Buffer-window and summary-memory abstractions are
|
||||
opinionated about how to fit history into a window. Here the Insights panel exposes each
|
||||
context component, its token cost, and the trigger word that pulled it in — which means the
|
||||
assembly has to be explicit and inspectable.
|
||||
context component, its token cost, and the trigger word that pulled it in, so the assembly
|
||||
has to be explicit and inspectable.
|
||||
|
||||
**When it would be the right call:** if the design went toward the two-call version —
|
||||
narrate, then a separate structured-extraction step, with a retry branch when extraction
|
||||
fails and a tool-calling path for dice — that is a graph, and hand-rolling it would get
|
||||
ugly fast.
|
||||
**When it would be the right call:** if the design went toward the two-call version, where
|
||||
narration is followed by a separate structured-extraction step, with a retry branch when
|
||||
extraction fails and a tool-calling path for dice, that is a graph, and hand-rolling it
|
||||
would get ugly fast.
|
||||
|
||||
---
|
||||
|
||||
@@ -576,8 +576,8 @@ behind it.
|
||||
### The problem
|
||||
|
||||
The story used to be a list, and a mutable one. Retry rewrote the last entry in place;
|
||||
undo and delete removed entries from the middle. Everything derived from the story —
|
||||
the memories, the running summary, the two marks saying how far each had got — was
|
||||
undo and delete removed entries from the middle. Everything derived from the story, such as
|
||||
the memories, the running summary, and the two marks saying how far each had got, was
|
||||
indexed by *position in that list*, and a position means something different after
|
||||
anything in front of it is deleted.
|
||||
|
||||
@@ -603,8 +603,8 @@ Make the story a tree, and none of them are reachable.
|
||||
|
||||
Every action is a **node** with a `branch_id` and a `depth`. A **branch** is one line
|
||||
through the tree; it holds the nodes played on it and *borrows* everything before its
|
||||
fork point from its ancestors. Nothing is ever copied and — apart from an explicit
|
||||
delete — nothing is ever removed.
|
||||
fork point from its ancestors. Nothing is ever copied, and apart from an explicit delete,
|
||||
nothing is ever removed.
|
||||
|
||||
```
|
||||
branches(id, adventure_id, parent_branch_id, fork_depth, lineage, name)
|
||||
@@ -636,38 +636,38 @@ keep the shape they already had.
|
||||
### The lineage, and why fork count doesn't cost anything
|
||||
|
||||
The OR-clause above is not reconstructed per read. It is stored on the branch row as
|
||||
`lineage` — `[(C, ∞), (B, 5), (A, 3)]` — computed once when the fork happens, from the
|
||||
parent's lineage plus one entry. `context/lineage.py` is the only module that knows how
|
||||
to turn it into a query, which is deliberate: one forgotten clause shows the wrong story
|
||||
and reports nothing.
|
||||
`lineage`, for example `[(C, ∞), (B, 5), (A, 3)]`, computed once when the fork happens,
|
||||
from the parent's lineage plus one entry. `context/lineage.py` is the only module that
|
||||
knows how to turn it into a query, which is deliberate: one forgotten clause shows the
|
||||
wrong story and reports nothing.
|
||||
|
||||
Two properties of the shape do the real work:
|
||||
|
||||
- **The ranges are disjoint and descending.** A branch's own nodes always sit deeper than
|
||||
its fork point, and each ancestor is capped at the fork depth of the branch beneath it.
|
||||
So ordering the whole clause by `depth DESC` reads entry 0's nodes, then entry 1's,
|
||||
then entry 2's — which means a tail read can use the newest few entries and stop.
|
||||
then entry 2's, which means a tail read can use the newest few entries and stop.
|
||||
- **Clause count is bounded by the context window, not by fork count.** A 200-fork story
|
||||
whose newest branch is 40 turns long reads with *one* clause, because the window is
|
||||
covered before the second entry is reached.
|
||||
|
||||
A branch stores no story of its own, so a fork costs an id, a parent, a fork depth and a
|
||||
cached ancestry. Measured on a 40-turn story forked twenty times against the same story
|
||||
flat: a page load of **31,652 B against 31,433 B — 1.007×**, or about **103 bytes per
|
||||
branch**. No migration, no vacuum, no copy.
|
||||
flat: a page load of **31,652 B against 31,433 B, a 1.007× ratio**, or about **103 bytes
|
||||
per branch**. No migration, no vacuum, no copy.
|
||||
|
||||
### What a player actually does
|
||||
|
||||
None of the above is what the screen shows. In the player's words:
|
||||
|
||||
> Any turn can gain another **take**. On an AI turn that means regenerate; on your own
|
||||
> message it means type something else. Stepping between takes with `‹ 2/4 ›` is free —
|
||||
> message it means type something else. Stepping between takes with `‹ 2/4 ›` is free:
|
||||
> the story below simply empties, because that take has no children yet. **A branch is
|
||||
> created when you write below a take that is not the live one**, never before.
|
||||
|
||||
That rule collapses two operations into one and deletes a distinction from the UI. The
|
||||
first version of this screen had a chip that *switched* at the tip and only *previewed*
|
||||
above it, with a second button to take that line — one control whose meaning depended on
|
||||
above it, with a second button to take that line: one control whose meaning depended on
|
||||
where the reader was standing. The rule above replaced it with a pager that only ever
|
||||
steps, a fork button on every turn, and no tip-versus-past distinction at all. The
|
||||
distinction survives in the implementation, where it decides whether a write needs a
|
||||
@@ -678,7 +678,7 @@ switch and no branch is created.
|
||||
|
||||
The load-bearing detail, and the one that is not obvious.
|
||||
|
||||
The natural way to find "the other takes of this turn" is by coordinate — same branch,
|
||||
The natural way to find "the other takes of this turn" is by coordinate: same branch,
|
||||
same depth. It is wrong in both directions:
|
||||
|
||||
```
|
||||
@@ -693,8 +693,8 @@ land on different branches. It gets `C` wrong: once C has been forked onto a bra
|
||||
its own it is alone at its coordinate and reads `1/1`, having lost C1 and C2 from a pager
|
||||
that must still say `1/3`.
|
||||
|
||||
So a node carries `parent_id`, read for nothing but this. The alternative — making a
|
||||
branch's fork point a *node* rather than a depth, so a promoted take never moves — was
|
||||
So a node carries `parent_id`, read for nothing but this. The alternative, making a
|
||||
branch's fork point a *node* rather than a depth so a promoted take never moves, was
|
||||
rejected: the whole point of `lineage` is that a read is an OR-clause per branch instead
|
||||
of a walk up parent pointers, and re-pointing the fork at a node changes path resolution
|
||||
itself, dragging in the cursors, memory depths and both bundle formats. `parent_id` is
|
||||
@@ -702,7 +702,7 @@ one indexed lookup, never a walk, and nothing about how a path resolves changes.
|
||||
|
||||
### Cursors become anchors
|
||||
|
||||
The two marks — how far the memory bank has got, how far the summary has got — used to
|
||||
The two marks, how far the memory bank has got and how far the summary has got, used to
|
||||
be counts. A count is a position in a list, and every rule about sliding them, rewinding
|
||||
them and translating between positions and `Action.index` existed to patch up the fact
|
||||
that the list moves.
|
||||
@@ -720,23 +720,23 @@ existed because a retry could rewrite an action the mark had already passed.
|
||||
|
||||
### Derived work attaches to the node that produced it
|
||||
|
||||
Generalise the rule and a lot falls out: *anything derived hangs off the node that
|
||||
produced it*. A memory covering depths 37–42 hangs off that branch's node 42 and is
|
||||
Generalize the rule and a lot falls out: *anything derived attaches to the node that
|
||||
produced it*. A memory covering depths 37–42 attaches to that branch's node 42 and is
|
||||
invisible to any path that does not run through it. Shared ancestors are therefore shared
|
||||
automatically, so **a fork needs nothing recreated** — the memories above the fork point
|
||||
automatically, so **a fork needs nothing recreated**: the memories above the fork point
|
||||
are already on the ancestors both lines read.
|
||||
|
||||
The subtle case is the memory sitting *at* the forked coordinate. The first cut moved it
|
||||
onto the new branch and re-anchored the marks naming it. Both are wrong for the same
|
||||
reason: that memory describes whichever attempt was live at that coordinate, which is the
|
||||
one staying on the parent. The right answer needs no code — the lineage caps the parent
|
||||
one depth short of the fork, so the memory is simply out of range from the new branch,
|
||||
one staying on the parent. The right answer needs no code: the lineage caps the parent one
|
||||
depth short of the fork, so the memory is simply out of range from the new branch,
|
||||
invisible to both the retrieval clause and the anchor read. The new line summarizes that
|
||||
ground again, from the text it actually tells.
|
||||
|
||||
Hand-written memories obey the same rule. One used to carry a NULL depth, described as
|
||||
"belongs to the adventure rather than to a path" — which sounds harmless and is not: a
|
||||
NULL is a coordinate no fork can cap, so a note typed on one line followed the reader onto
|
||||
"belongs to the adventure rather than to a path". That sounds harmless and is not: a NULL
|
||||
is a coordinate no fork can cap, so a note typed on one line followed the reader onto
|
||||
branches whose events it never described. They are anchored at the head instead: *the
|
||||
story you were reading when you wrote it*.
|
||||
|
||||
@@ -747,7 +747,7 @@ also why branch management could not be a nice-to-have: without a way to delete
|
||||
storage grows without limit.
|
||||
|
||||
The delete rule has two halves and the second is easy to miss. Refusing to delete the
|
||||
line being read is obvious. The other half is refusing any line it was **forked from** —
|
||||
line being read is obvious. The other half is refusing any line it was **forked from**:
|
||||
`parent_branch_id` cascades, so deleting an ancestor takes the head with it and leaves
|
||||
`head_branch_id` pointing at a row that is gone. One membership test against the head's
|
||||
own lineage covers both, because a lineage already names itself and every branch it
|
||||
@@ -757,7 +757,7 @@ button can say so before it is pressed.
|
||||
### The migration, and what it deliberately did not do
|
||||
|
||||
There is no feature flag. **A linear story is a tree with one branch**, so the
|
||||
intermediate states were not half-migrated — they were the same product with a superset
|
||||
intermediate states were not half-migrated: they were the same product with a superset
|
||||
schema underneath, which made "existing adventures are unaffected" a literal, testable
|
||||
pass condition at every step. A flag would have bought two live code paths through the
|
||||
context builder, the memory bank, undo and retry at once.
|
||||
@@ -767,8 +767,8 @@ were kept unread for a release rather than dropped with the migration that stopp
|
||||
them, so that a redeploy of the previous build is still a way out. Dropping columns is the
|
||||
one step that isn't.
|
||||
|
||||
One operational note that generalises: on Postgres, a migration that rewrites every row of
|
||||
`actions` roughly doubles the table and only `VACUUM FULL` gives it back — 79 MB reclaimed
|
||||
One operational note that generalizes: on Postgres, a migration that rewrites every row of
|
||||
`actions` roughly doubles the table, and only `VACUUM FULL` gives it back: 79 MB reclaimed
|
||||
in 5.5 s on one occasion. But bloat scales with the **heap**, and `context_snapshot` is 94%
|
||||
of this table and lives out of line, so a migration touching only small columns reuses the
|
||||
existing TOAST pointer and costs a tenth of that. Read the sizes from `sum(octet_length())`
|
||||
@@ -779,14 +779,14 @@ makes bloat look smaller.
|
||||
|
||||
- **The two marks are one pair on the adventure**, not one per branch. Switching branches
|
||||
makes the mark on the line being left unreadable from the new one, and that ground is
|
||||
summarized again. It answers "nothing covered", which is the safe direction — redo the
|
||||
work, never skip it — but switching back and forth costs AI calls. Per-branch cursors
|
||||
summarized again. It answers "nothing covered", which is the safe direction: redo the
|
||||
work, never skip it. But switching back and forth costs AI calls. Per-branch cursors
|
||||
are the fix if it ever matters.
|
||||
- **Story cards stay adventure-wide.** A card invented on branch B shows on branch A.
|
||||
Event-sourcing card changes onto nodes was considered and rejected.
|
||||
- **Editing an already-summarized action still leaves its memory stale.** The machinery to
|
||||
fix it now exists — an edit could write a sibling take and switch to it, which is a retry
|
||||
the player typed — but it does not do that yet.
|
||||
fix it now exists: an edit could write a sibling take and switch to it, which is a retry
|
||||
the player typed. It does not do that yet.
|
||||
|
||||
## 2.3 Undo and retry that actually rewind
|
||||
|
||||
@@ -794,17 +794,17 @@ Most implementations of undo delete the last message. That's wrong here, because
|
||||
mutates three things: the text, the scripting scoreboard (`script_state`), and the RPG
|
||||
stats (`world_state`).
|
||||
|
||||
**The mechanism:** every node carries `state_after` and `world_state_after` — deep copies
|
||||
**The mechanism:** every node carries `state_after` and `world_state_after`, deep copies
|
||||
of what the adventure looked like once that turn had played. Rewinding to before a turn is
|
||||
a read of the node in front of it, so undo, retry and a branch switch are the same
|
||||
restore. The cooldown clock comes along for free: it lives inside the world state, in
|
||||
`_meta.last_changed`, so each line of the story carries its own without anything having to
|
||||
know there is one.
|
||||
|
||||
**Nothing a retry replaces is thrown away.** The old attempt stays as another **take** of
|
||||
that turn — a sibling node at the same coordinate, `live` false — and the pager steps
|
||||
between them. Which is to say retry is not a special case: it is the tree, with the branch
|
||||
not yet created. See [2.2](#22-the-story-is-a-tree).
|
||||
**Nothing a retry replaces is discarded.** The old attempt stays as another **take** of
|
||||
that turn, a sibling node at the same coordinate with `live` set to false, and the pager
|
||||
steps between them. Retry is not a special case: it is the tree, with the branch not yet
|
||||
created. See [2.2](#22-the-story-is-a-tree).
|
||||
|
||||
Three details that are easy to get wrong:
|
||||
|
||||
@@ -825,9 +825,10 @@ take belonging to a line nobody asked about. Anything that reads a take group an
|
||||
*writes* has to say whether it means the turn or the coordinate.
|
||||
|
||||
**If the regeneration fails, the rollback is reversed.** `generate_turn` wraps the
|
||||
generator in a `try/finally`: if it ends without saving — a provider error, an empty reply,
|
||||
a script `stop`, or the browser hanging up — the previous take is put back in charge.
|
||||
Otherwise the state on the server would drift from the text still on the user's screen.
|
||||
generator in a `try/finally`: if it ends without saving, whether from a provider error, an
|
||||
empty reply, a script `stop`, or the browser hanging up, the previous take is put back in
|
||||
charge. Otherwise the state on the server would drift from the text still on the user's
|
||||
screen.
|
||||
|
||||
## 2.4 The turn lock
|
||||
|
||||
@@ -854,11 +855,11 @@ async def with_turn_lock(adventure_id, gen): # wraps the SSE generator
|
||||
```
|
||||
|
||||
In-memory, so it's a single-process guarantee. That's honest for the deployment this
|
||||
targets — one Render web service. Two processes would need the lock in the database.
|
||||
targets: one Render web service. Two processes would need the lock in the database.
|
||||
|
||||
## 2.5 The 189x egress fix
|
||||
|
||||
**The setup:** `Action.context_snapshot` holds the entire assembled prompt for a turn —
|
||||
**The setup:** `Action.context_snapshot` holds the entire assembled prompt for a turn,
|
||||
about 74 KB per row, 94% of the database.
|
||||
|
||||
**The bug:** every adventure load pulled that column for every action, to read two small
|
||||
@@ -869,7 +870,7 @@ report). SQLAlchemy loads all columns by default.
|
||||
|
||||
1. Move the two small things that *are* needed for every action into their own column
|
||||
(`Action.world_delta`).
|
||||
2. Mark the heavy columns `deferred` — `context_snapshot`, `variants`, `reasoning` — so
|
||||
2. Mark the heavy columns `deferred` (`context_snapshot`, `variants`, `reasoning`), so
|
||||
they're only fetched when explicitly asked for.
|
||||
3. Backfill the new column with dialect-specific server-side SQL, so the old data is
|
||||
extracted inside the database and never crosses the wire.
|
||||
@@ -883,7 +884,7 @@ not on a timing.
|
||||
|
||||
One more detail from that test's design: the count query is written as a real
|
||||
`SELECT count(...)` rather than `query.count()`, because SQLAlchemy's `.count()` wraps the
|
||||
entity select in a subquery, so the emitted SQL names every column — including the deferred
|
||||
entity select in a subquery, so the emitted SQL names every column, including the deferred
|
||||
ones. No bytes come back either way, but the database still has to read them, and a guard
|
||||
that greps SQL cannot tell the two apart.
|
||||
|
||||
@@ -899,7 +900,7 @@ No Alembic. An append-only list of `(version, SQL)` pairs, with the current vers
|
||||
in SQLite's `PRAGMA user_version` or a one-row table on Postgres. 64 versions so far.
|
||||
|
||||
- A **fresh** database is created by `Base.metadata.create_all()` (always current) and
|
||||
stamped at the latest version — it never replays history.
|
||||
stamped at the latest version; it never replays history.
|
||||
- An **existing** database runs every migration above its stored version, in order.
|
||||
|
||||
Why this and not Alembic: for a single-file SQLite app that a user might have been running
|
||||
@@ -909,9 +910,9 @@ environment. This is 250 lines and you can read all of it.
|
||||
|
||||
The constraint it creates is written at the top of the file: change `models.py` (so fresh
|
||||
databases are current) *and* append a pair here (so existing ones upgrade). Migrations 2–23
|
||||
predate Postgres support and use SQLite-only syntax — harmless, because every Postgres
|
||||
database starts fresh and never replays them, but anything added since must run on both
|
||||
dialects.
|
||||
predate Postgres support and use SQLite-only syntax. This is harmless, because every
|
||||
Postgres database starts fresh and never replays them, but anything added since must run
|
||||
on both dialects.
|
||||
|
||||
One migration worth reading (#10, repairing duplicate action indexes) uses `UPDATE … FROM`
|
||||
with a window function rather than a correlated subquery, because SQLite may evaluate a
|
||||
@@ -929,7 +930,7 @@ correlated subquery against partially-updated rows and produce duplicates again
|
||||
| | Local (default) | Hosted |
|
||||
|---|---|---|
|
||||
| Users | One auto-created "local user" | Guest on first visit, optional account |
|
||||
| Auth | None — no cookies, no login UI | Signed session cookie |
|
||||
| Auth | None: no cookies, no login UI | Signed session cookie |
|
||||
| Rate limits | Off | On |
|
||||
| Row caps | Off | On |
|
||||
| API docs (`/docs`) | On | Off |
|
||||
@@ -941,13 +942,13 @@ deployment needs all four of those to be the opposite. Rather than two builds, t
|
||||
differences are gated at each site.
|
||||
|
||||
**Guests upgrade in place.** A visitor gets a guest `User` row on first load. Registering
|
||||
sets `email` and `password_hash` on that *same row* — so every adventure they played as a
|
||||
sets `email` and `password_hash` on that *same row*, so every adventure they played as a
|
||||
guest survives with no re-parenting and no migration step. Three kinds of row share the
|
||||
users table: local (email NULL, not guest), guest (email NULL, guest), registered (email
|
||||
set).
|
||||
|
||||
**Guests expire; accounts don't.** One row per curious visitor adds up, so `cleanup.py`
|
||||
deletes guests idle for `AIDND_GUEST_RETENTION_DAYS` (default 5) — measured as
|
||||
deletes guests idle for `AIDND_GUEST_RETENTION_DAYS` (default 5), measured as
|
||||
`COALESCE(last_seen_at, created_at)`, because `_touch` only writes `last_seen_at` hourly
|
||||
and a guest minted by `/auth/me` has NULL until its second request. The filter requires
|
||||
both `is_guest` *and* `email IS NULL`, so upgrading in place is also how you opt out of
|
||||
@@ -957,7 +958,7 @@ every few hours.
|
||||
It's a single Core `DELETE`, not `db.delete(user)`: the ORM path would SELECT every
|
||||
adventure, action and memory into Python purely to delete them, and the FK graph is
|
||||
`ON DELETE CASCADE` from `users` all the way down, so the database can do the whole graph
|
||||
in one statement. Nothing a guest owns is visible to anyone else either — `is_public` is
|
||||
in one statement. Nothing a guest owns is visible to anyone else either: `is_public` is
|
||||
output-only, so shared content is exactly the seeded scenarios, which have `user_id NULL`
|
||||
and never match the filter.
|
||||
|
||||
@@ -969,11 +970,11 @@ is a spending surface, so it's the most defended code in the project.
|
||||
`resolve_provider_config()` is the single place the BYOK-vs-demo decision is made, and on
|
||||
the demo branch it pins **two** things:
|
||||
|
||||
- **The model** — to a whitelist. A caller-supplied override or a hand-edited settings row
|
||||
can't aim a server-funded key at an expensive model. Anything unrecognised falls back to
|
||||
the first whitelisted model.
|
||||
- **The endpoint** — to the configured demo URL. Otherwise the key could be redirected to a
|
||||
URL the user controls and harvested.
|
||||
- **The model**, pinned to a whitelist. A caller-supplied override or a hand-edited
|
||||
settings row can't aim a server-funded key at an expensive model. Anything unrecognized
|
||||
falls back to the first whitelisted model.
|
||||
- **The endpoint**, pinned to the configured demo URL. Otherwise the key could be
|
||||
redirected to a URL the user controls and harvested.
|
||||
|
||||
Plus a daily per-user turn cap (default 20), checked *before* the player's input is stored
|
||||
so a capped player doesn't get their message saved with no reply, and counted only after a
|
||||
@@ -982,12 +983,12 @@ successful turn.
|
||||
There's a defensive `__post_init__` on the config object that raises if a demo config
|
||||
somehow carries a non-whitelisted model. The comment on it records a real bug: the check
|
||||
tests `using_demo`, **not** `api_key == DEMO_API_KEY`. Keying on the key value looks
|
||||
stricter but is wrong — the demo key is an ordinary OpenRouter key, so a user can
|
||||
stricter but is wrong: the demo key is an ordinary OpenRouter key, so a user can
|
||||
legitimately paste that same key into their own settings as BYOK, and then every resolution
|
||||
raised, 500ing even `GET /auth/me` and taking the whole SPA down. `using_demo` is what
|
||||
actually means "the server is paying".
|
||||
|
||||
Background work (summarization, embeddings) is excluded from the demo key entirely — those
|
||||
Background work (summarization, embeddings) is excluded from the demo key entirely: those
|
||||
are unmetered calls, and unmetered calls on a server-funded key is a bill.
|
||||
|
||||
## 3.3 Secrets
|
||||
@@ -997,11 +998,11 @@ Everything derives from one server-side secret (`AIDND_SECRET_KEY`).
|
||||
| Thing | Mechanism |
|
||||
|---|---|
|
||||
| Passwords | `hashlib.scrypt`, N=2^14, r=8, p=1, per-password salt, constant-time compare. Stdlib, so no extra dependency. |
|
||||
| Sessions | `v1.<user_id>.<HMAC-SHA256>`, no expiry — long-lived guest sessions are the point. A cookie can outlive a swept guest row; that resolves to a 401, which the frontend already turns into a fresh session. |
|
||||
| Stored LLM API keys | Fernet (AES) encryption at rest, key derived from the secret, `enc:` prefix so legacy plaintext rows are recognisable and migratable. |
|
||||
| Sessions | `v1.<user_id>.<HMAC-SHA256>`, no expiry: long-lived guest sessions are the point. A cookie can outlive a swept guest row; that resolves to a 401, which the frontend already turns into a fresh session. |
|
||||
| Stored LLM API keys | Fernet (AES) encryption at rest, key derived from the secret, `enc:` prefix so legacy plaintext rows are recognizable and migratable. |
|
||||
|
||||
The secret auto-generates into a file next to the database for local installs (zero config),
|
||||
but **multi-user mode refuses to start without the env var** — with an error message that
|
||||
but **multi-user mode refuses to start without the env var**, with an error message that
|
||||
explains why and gives you the command to generate one. Hosted filesystems are ephemeral; a
|
||||
regenerated secret on every deploy would silently log out every user and orphan their stored
|
||||
API keys.
|
||||
@@ -1025,11 +1026,11 @@ rather than raising, so the user just re-enters their key instead of hitting a 5
|
||||
Rate limits are keyed per user when one is known (accounts survive IP changes) and per IP
|
||||
otherwise, in fixed windows held in memory, with a pruning pass so the per-IP dict can't
|
||||
grow without bound. Import endpoints check bundle list lengths against the same caps live
|
||||
creation enforces — otherwise the cap is trivially bypassed by uploading a file.
|
||||
creation enforces, otherwise the cap is trivially bypassed by uploading a file.
|
||||
|
||||
Security headers on every response: `nosniff`, `X-Frame-Options: DENY`,
|
||||
`Referrer-Policy: same-origin`, and a CSP allowing exactly what the SPA uses — same-origin
|
||||
everything, inline styles (React needs them), Google Fonts.
|
||||
`Referrer-Policy: same-origin`, and a CSP allowing exactly what the SPA uses: same-origin
|
||||
everything, inline styles (React needs them), and Google Fonts.
|
||||
|
||||
## 3.5 Deployment
|
||||
|
||||
@@ -1053,13 +1054,13 @@ push.
|
||||
|
||||
A hosted demo raises a question a local app never does: is anyone using it, and do they get
|
||||
anywhere? The answer is an owner-only dashboard at `/analytics`, gated on
|
||||
`AIDND_ANALYTICS_EMAILS` — a list kept separate from `AIDND_POWER_USERS`, since an unmetered
|
||||
`AIDND_ANALYTICS_EMAILS`, a list kept separate from `AIDND_POWER_USERS`, since an unmetered
|
||||
tester is not automatically someone who should see the traffic.
|
||||
|
||||
**Why it isn't a third-party script.** The CSP allows `script-src 'self'`, so a tracker would
|
||||
mean loosening it; adblockers eat the popular ones, which silently biases exactly the
|
||||
technical audience this project is shown to; and none of them can see the measurement that
|
||||
matters here — a *turn*. The interesting funnel step is not a pageview.
|
||||
mean loosening it. Ad blockers block the popular ones, which silently biases exactly the
|
||||
technical audience this project is shown to. And none of them can see the measurement that
|
||||
matters here: a *turn*. The interesting funnel step is not a pageview.
|
||||
|
||||
**Egress is the budget.** After the 189x fix (§2.5) it would be perverse to add a feature
|
||||
that reads rows per request. So counts accumulate in a process-local dict and flush every 60
|
||||
@@ -1068,23 +1069,23 @@ seconds as UPSERTs: **a visit is a write and never a read**. Storage is a generi
|
||||
Every dashboard query is a `GROUP BY` that returns tens of rows regardless of the traffic
|
||||
behind it, so a month costs a few kilobytes to read back. The cost of the buffer is that a
|
||||
hard restart can lose up to a minute; the flusher also runs on shutdown, and on a tier that
|
||||
sleeps when idle the buffer it sleeps on is empty anyway.
|
||||
sleeps when idle, the buffer it sleeps on is empty anyway.
|
||||
|
||||
**The numbers are the server's, not the browser's.** The client reports one fact — which page
|
||||
was viewed — and even that is normalized to a route (`/play/12` → `/play/:id`) against a
|
||||
**The numbers are the server's, not the browser's.** The client reports one fact, which page
|
||||
was viewed, and even that is normalized to a route (`/play/12` → `/play/:id`) against a
|
||||
whitelist, so the page list cannot be polluted by anything a stranger posts. Everything that
|
||||
means something — a turn, an adventure, a sign-up — is recorded by the code that performs it.
|
||||
That also fixes a blind spot: a failed turn is an HTTP 200 with a bad ending, so a
|
||||
status-code tally cannot see it, and a demo whose model has started refusing looks perfectly
|
||||
healthy from outside. `turn_error` is counted where the SSE error is written.
|
||||
means something, such as a turn, an adventure, or a sign-up, is recorded by the code that
|
||||
performs it. That also fixes a blind spot: a failed turn is an HTTP 200 with a bad ending, so
|
||||
a status-code tally cannot see it, and a demo whose model has started refusing looks
|
||||
perfectly healthy from outside. `turn_error` is counted where the SSE error is written.
|
||||
|
||||
The funnel counts **people, not clicks** — a player who starts six adventures is one person
|
||||
who started an adventure — which is the entire reason the per-visitor-day table exists.
|
||||
The funnel counts **people, not clicks**: a player who starts six adventures is one person
|
||||
who started an adventure, which is the entire reason the per-visitor-day table exists.
|
||||
|
||||
One smaller decision worth naming: error buckets are labelled by the matched *route template*,
|
||||
never the requested path. That gives one bucket per endpoint instead of one per adventure id,
|
||||
and — the reason it isn't merely tidier — an unmatched path is entirely attacker-chosen, so
|
||||
labelling by it would let anyone mint rows.
|
||||
One smaller decision worth naming: error buckets are labeled by the matched *route
|
||||
template*, never the requested path. That gives one bucket per endpoint instead of one per
|
||||
adventure id. The reason it isn't merely tidier is that an unmatched path is entirely
|
||||
attacker-chosen, so labeling by it would let anyone mint rows.
|
||||
|
||||
---
|
||||
|
||||
@@ -1092,9 +1093,9 @@ labelling by it would let anyone mint rows.
|
||||
|
||||
For the parts that are just how the web works, not decisions.
|
||||
|
||||
**Frontend and backend are two programs.** In development they're two servers — Vite on
|
||||
5173 serving React, FastAPI on 8000 serving the API — and Vite proxies `/api` to FastAPI so
|
||||
the browser thinks it's all one origin (which avoids CORS entirely). In production there's
|
||||
**Frontend and backend are two programs.** In development they're two servers: Vite on 5173
|
||||
serving React, and FastAPI on 8000 serving the API. Vite proxies `/api` to FastAPI so
|
||||
the browser thinks it's all one origin, which avoids CORS entirely. In production there's
|
||||
one server: FastAPI serves the built React files as static assets from the same port.
|
||||
|
||||
**SPA routing.** React Router handles URLs like `/play/3` in the browser without a round
|
||||
@@ -1105,7 +1106,7 @@ unaffected.
|
||||
|
||||
**Sessions.** A cookie is a small value the browser stores and automatically attaches to
|
||||
every request to that site. Here it holds `v1.<user_id>.<signature>`. The server doesn't
|
||||
store sessions anywhere — it re-verifies the signature on each request, which is why there's
|
||||
store sessions anywhere; it re-verifies the signature on each request, which is why there's
|
||||
no session table.
|
||||
|
||||
**The 401 retry.** If the cookie is missing or stale, any API call returns 401. The frontend
|
||||
@@ -1136,7 +1137,7 @@ text appears to type itself.
|
||||
| Context defaults | author's note at depth 3, cards capped at 40% of elastic budget |
|
||||
| Memory cadence | memory / 6 turns, summary / 15 turns, top-5 retrieval |
|
||||
|
||||
Two of the tests encode a performance property rather than a behaviour:
|
||||
Two of the tests encode a performance property rather than a behavior:
|
||||
`test_egress.py` asserts on the SQL the ORM emits, and `test_history_window.py` asserts
|
||||
that the read cost stops growing with story length.
|
||||
|
||||
@@ -1150,7 +1151,7 @@ nobody has to discover them the hard way.
|
||||
lock) and the rate limiter in Redis.
|
||||
- **No vector index.** Retrieval does cosine similarity in Python over the whole bank. Fine
|
||||
at the 200-memory cap; at 10,000 it would want pgvector.
|
||||
- **Prompt snapshots are heavy** even after the egress fix — they're deferred, not smaller.
|
||||
- **Prompt snapshots are heavy** even after the egress fix; they're deferred, not smaller.
|
||||
Compressing them or expiring old ones is the real fix.
|
||||
- **In-memory rate-limit windows reset on restart**, so a restart grants a brief extra
|
||||
allowance.
|
||||
@@ -1160,7 +1161,7 @@ nobody has to discover them the hard way.
|
||||
detect after the fact by string-matching the 429 body.
|
||||
- **The two memory marks are one pair on the adventure, not one per branch.** Switching
|
||||
lines makes the mark on the line being left unreadable from the new one, so that ground is
|
||||
summarized again. It fails in the safe direction — redo, never skip — but switching back
|
||||
summarized again. It fails in the safe direction: redo, never skip. But switching back
|
||||
and forth costs AI calls. Per-branch cursors are the fix if it matters.
|
||||
- **Story cards are adventure-wide**, so a card invented on one branch shows on all of them.
|
||||
- **Editing an already-summarized turn leaves its memory stale.** Replacing a turn withdraws
|
||||
@@ -1168,8 +1169,8 @@ nobody has to discover them the hard way.
|
||||
|
||||
## Cleanup backlog
|
||||
|
||||
`docs/self-review.md` carries an open list of non-bugs — reuse, simplification and
|
||||
efficiency items — kept deliberately separate from the correctness list, which is empty.
|
||||
`docs/self-review.md` carries an open list of non-bugs (reuse, simplification, and
|
||||
efficiency items) kept deliberately separate from the correctness list, which is empty.
|
||||
The largest ones:
|
||||
|
||||
- `Section.tokens` is uncached, so the context gets tokenized two or three times a turn.
|
||||
|
||||
+55
-53
@@ -1,72 +1,73 @@
|
||||
# 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.
|
||||
**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` = applied, `skipped` = intentionally not fixed, `pending` = outstanding
|
||||
(none remain).
|
||||
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 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
|
||||
**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`
|
||||
- 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.
|
||||
- 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-click Continue → both requests pass the 409 check → duplicate Action.index rows, interleaved generations.
|
||||
- `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 → duplicate indexes survive the "repair".
|
||||
- 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 attach a future script to an old scenario.
|
||||
- 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`
|
||||
- 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.
|
||||
- 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`
|
||||
- `asyncio.create_task` result not referenced; task can vanish silently; adventure ID can stay stuck in `_running`.
|
||||
- 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, 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.
|
||||
- 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 + top_k=5 → 11 memories injected, blowing token budget.
|
||||
- 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-dim embeddings scored against new 1536-dim query → garbage similarity, no error, never re-embedded.
|
||||
- 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 (seen_ids ⊂ existing). A script can insert unbounded cards in one turn.
|
||||
- `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
|
||||
@@ -80,17 +81,17 @@ compatibility — documented in `engine.py`'s prelude instead. The cleanup backl
|
||||
|
||||
### 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}`.
|
||||
- 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's dropped and not reported.
|
||||
- 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` shared by all plot fields AND story-card saves; editing a second thing within 600ms cancels the first pending PATCH → silent data loss.
|
||||
- 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
|
||||
@@ -99,49 +100,50 @@ compatibility — documented in `engine.py`'s prelude instead. The cleanup backl
|
||||
|
||||
### 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.
|
||||
- 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`
|
||||
- Failed retry (409/network) leaves UI missing an action that still exists server-side.
|
||||
- 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`
|
||||
- Rejection leaves `{pending:true}` forever + unhandled rejection.
|
||||
- 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`
|
||||
- No staleness guard; slow getAdventureContext can clobber a newer action snapshot.
|
||||
- Fix: track a request id / cancelled flag in the effect.
|
||||
- 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`
|
||||
- Backend fills placeholders in card.keys but the modal never prompts for those names → literal `${hero}` keys never match.
|
||||
- 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** `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.
|
||||
- **R1** `frontend/src/pages/Play.jsx:26` + `ScenarioEditor.jsx:19` + `ScriptEditor.jsx:34`: three copies of the debounced-autosave and story-card handlers. Extract a `useDebouncedSave` hook and a 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()'s 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 the column and its schema fields.
|
||||
- **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 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.
|
||||
**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.
|
||||
|
||||
Reference in New Issue
Block a user