From b1772c6e21be3d6592af44c830563f0eec7d73db Mon Sep 17 00:00:00 2001 From: parththakkar106 Date: Sat, 29 Aug 2026 00:36:21 +0530 Subject: [PATCH] Plan the readability refactor, and clear the tree for it Phase 17 splits the four files that hold most of the code, finishes the schema migration SP8 left half done, and stops the published guide from drifting away from its Markdown source. `plan/17-refactor.md` carries the plan and the progress table, and `plan/STATUS.md` points at it. Stage 0 is hygiene only. Both abandoned worktrees are gone, which freed about 104 MB. Removing `sp7-tree-ui` needed one extra step: a Vite dev server had been running out of it since 2026-08-18, holding `frontend/.vite` open and owning port 5173, and serving a tree 54 commits behind `main`. The three stale `.db` files are deleted; `data.db` is not. `AIDND_TRUSTED_PROXY_HOPS` is now documented. It was read at `limits.py:55` and named in no `.env.example`, README, or blueprint. It sets how many proxy hops the rate limiter trusts in `X-Forwarded-For`, so a deployment that adds a hop without setting it gets the bucket-rotation bypass back. The 19 squash-landed branches are still there. `git branch -D` is blocked by the permission classifier; the verified command is in the plan file. 549 tests pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Dix4oGV3njgWRdu7P9t6r --- README.md | 6 + backend/.env.example | 12 ++ plan/17-refactor.md | 406 +++++++++++++++++++++++++++++++++++++++++++ plan/STATUS.md | 4 + 4 files changed, 428 insertions(+) create mode 100644 plan/17-refactor.md diff --git a/README.md b/README.md index 7382a27..210f393 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,12 @@ that takes about 30 to 60 seconds to wake it. Point any keep-warm pinger at `/ap which deliberately doesn't touch the database: waking the database around the clock costs far more than the cold start saves. +If you put another proxy or CDN in front of Render, set `AIDND_TRUSTED_PROXY_HOPS` to the +number of proxies in the chain. It defaults to 1. The rate limiter reads the client IP that +many entries from the right of `X-Forwarded-For`, because the trusted edge appends the real +one last. Leave it at 1 behind two proxies and the limiter reads an entry the caller +supplied, so anyone can rotate the header for a fresh rate-limit bucket per request. + ## Repo notes - `plan/` holds the phased implementation plan this project was built from, kept as a build diff --git a/backend/.env.example b/backend/.env.example index 335e2f7..97e7e5c 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -25,6 +25,18 @@ AIDND_DATABASE_URL= # Default: http://localhost:5173,http://127.0.0.1:5173 (the Vite dev server). AIDND_CORS_ORIGINS= +# How many proxy hops the rate limiter trusts in `X-Forwarded-For`. It reads +# the entry that many places from the right, because the trusted edge appends +# the real client IP last. Set this to the number of proxies in front of the +# app. Default: 1, which is correct for a single edge such as Render. +# +# Get it wrong in either direction and the rate limits weaken. Too low reads an +# entry the caller supplied, so anyone can rotate the header for a fresh +# rate-limit bucket per request and walk past the auth and guest limits. Too +# high reads past the real client. Only multi-user mode rate-limits at all, so +# local installs can ignore this. +AIDND_TRUSTED_PROXY_HOPS= + # --------------------------------------------------------------------------- # Phase 8 — optional accounts & multi-user (all optional; defaults keep the # app in frictionless single-user "local mode") diff --git a/plan/17-refactor.md b/plan/17-refactor.md new file mode 100644 index 0000000..9e9ff05 --- /dev/null +++ b/plan/17-refactor.md @@ -0,0 +1,406 @@ +# Phase 17: refactor for readability + +The code works and it is tested. It is also hard to read, because four files hold +most of it, one schema migration is half finished, and the published guide is a +hand-maintained copy of another file. This phase fixes those three things without +changing what the app does. + +## How to use this file + +This file is both the plan and the running status. Update the progress table at +the end of every stage. Read the table first when you pick the work back up. + +Each stage lands as its own commit on `refactor-17`. No stage starts until the +stage before it is green. + +## Progress + +| Stage | Work | Status | Landed | +|---|---|---|---| +| 0 | Hygiene: worktrees, branches, undocumented settings | done, except the branch deletion | 2026-08-29 | +| 1 | Split the four largest files | not started | | +| 2 | Remove duplication | not started | | +| 3 | SP8: drop the legacy columns | not started | | +| 4 | Documentation | not started | | +| 5 | A frontend test runner | not started | | + +## Baseline + +Record these numbers before you start. They are how you tell a refactor from a +rewrite. + +- 549 backend tests pass in 103 seconds. +- `backend/app` holds 13,486 lines across 45 files. +- `backend/tests` holds 12,221 lines across 38 files. +- `frontend/src` holds 9,171 lines across 21 files. +- The app exposes 70 endpoints and applies 64 migrations on boot. + +Run the suite with `cd backend && .venv/Scripts/python.exe -m pytest tests/ -q`. + +## What the review found + +### The code is clean at the statement level + +A scan for unreferenced functions across `backend/app` returned only FastAPI route +handlers, which the decorator references rather than the name. A scan of +`index.css` found 5 unused class names out of 373, and every one of the 5 is either +CodeMirror's or built from a template string. There is no dead code to delete and +no `TODO` or `FIXME` anywhere in the tree. + +Read that as a constraint. The gains in this phase come from moving code, not from +finding rot. + +### Four files hold most of the complexity + +| File | Lines | What it holds | +|---|---|---| +| `backend/app/routers/adventures.py` | 2353 | Nine concerns: the turn engine, branches, takes, import and export, adventure scripts, refresh from scenario, insights, memory CRUD, and action CRUD | +| `frontend/src/pages/Play.jsx` | 2280 | 27 components, plus a 722-line `Play()` holding 18 `useState` calls and 7 `useEffect` calls | +| `frontend/src/index.css` | 2865 | One stylesheet. The `max-width: 720px` block at line 2655 overrides rules written 2000 lines above it | +| `backend/app/worldstate/engine.py` | 918 | Four jobs: parse a delta, apply a delta, render the context sections, and instantiate a schema | + +### The schema is half migrated + +`plan/14-phase-story-tree.md` defines SP8, which drops the columns the tree +replaced. SP8 was gated on the tree running in production. It now does. Eight +columns are still written on every turn and read by nothing: + +- `actions.index`, `variants`, `variant_index`, `variant_count` +- `actions.state_before`, `world_state_before` +- `adventures.memory_cursor`, `summary_cursor` + +Until they go, `models.py` carries two vocabularies for one idea, and every reader +has to be told which one is live. + +### The published guide is a hand-written copy + +`docs/guide.html` is 84 KB of hand-written HTML covering the same material as the +63 KB `docs/GUIDE.md`. Nothing generates one from the other. They have already +drifted: the HTML last changed on 2026-08-20 and the Markdown on 2026-08-28, and +section 3.6, "Counting visits", exists only in the Markdown. GitHub Pages publishes +the HTML, so readers get the stale copy. `docs/architecture.html` is 87 KB of +hand-written HTML with no Markdown source at all. + +### Duplication the tests already protect + +- `worldstate.apply_delta` and `apply_override` each re-implement the same path + routing for `flags.`, `milestones.`, and `npc..`. That is six parallel + branches at `engine.py:573`, `587`, `617`, `675`, `697`, and `740`. +- 35 of the 38 test files repeat the same 8-line temporary-database prologue. There + is no `conftest.py`. +- `ScriptedProvider` or `FakeProvider` is defined 12 times, once per test file that + needs a fake model. +- The debounced autosave helper is copied into `Play.jsx:204`, + `ScenarioEditor.jsx:23`, and `ScriptEditor.jsx:22`. +- `get_adventure_or_404` is called by hand in about 20 handlers. It is a function, + not a dependency, so every handler repeats the call. +- `routers/chat.py:22` imports `SSE_HEADERS` and `sse` from `routers/adventures.py`. + One router reaches into another for shared plumbing. + +### One setting is documented nowhere + +`AIDND_TRUSTED_PROXY_HOPS` is read at `limits.py:55`. It does not appear in +`backend/.env.example`, `README.md`, or `render.yaml`. It sets how many proxy hops +`client_ip()` trusts, which is what stops a rotated `X-Forwarded-For` header from +buying a fresh rate-limit bucket. If a deployment adds a proxy hop and nobody sets +this variable, the bypass comes back silently. + +The unprefixed `DATABASE_URL` that `database.py:24` also accepts turned out to be +documented already, inside the `AIDND_DATABASE_URL` entry in +`backend/.env.example`. No change needed there. + +### `Settings.stream` is dead + +`models.py:622` defines the column, and `schemas.py:452` and `499` expose it. +Nothing reads it in the backend or the frontend. This is item S1 in +`docs/self-review.md`. + +### Local clutter + +Everything here is gitignored, so it costs repository weight nothing. It costs disk +and attention. + +- Two abandoned worktrees, each a full copy of the tree: + `.claude/worktrees/simplify-comments` and `.claude/worktrees/sp7-tree-ui`. Both + branches landed on `main` as squashes. +- 21 local branches. 13 read as unmerged to `git branch --no-merged`, and + `plan/STATUS.md` already records that they landed as squashes. +- Three stale SQLite files in `backend/`: `data.backup-2026-07-06.db`, + `data.backup-pre-phase8.db`, and `scroll_fixture.db`, 2.9 MB together. The live + local database, `backend/data.db`, is not one of them and is not touched. + +## Stage 0: hygiene + +No source changes. Do this first, so later stages run against a quiet tree. + +1. Remove both worktrees with `git worktree remove`, then delete the branches that + landed as squashes. Verify each one first: a branch is safe to delete when + `git log main..` names only commits whose message already appears in + `git log main`. +2. Delete the three stale `.db` files. Leave `backend/data.db` alone. +3. Add `AIDND_TRUSTED_PROXY_HOPS` to `backend/.env.example`, with the reason it + exists. Add it to the deploy section of `README.md` too, because that is where + somebody putting the app behind another proxy looks. + +**Check:** the suite still passes, and `git status` is clean. + +### What Stage 0 actually did, 2026-08-29 + +Both worktrees are gone, which freed about 104 MB. Removing `sp7-tree-ui` needed one +extra step: a Vite dev server had been running out of that worktree since +2026-08-18, holding `frontend/.vite` open and owning port 5173. It was serving a tree +54 commits behind `main`. Stopping it freed the directory. Restart the real one with +`start.ps1`. + +The three stale `.db` files are deleted. `backend/data.db` is untouched. + +`AIDND_TRUSTED_PROXY_HOPS` is now in `backend/.env.example` and in the README deploy +section. + +**Still owed.** The 19 squash-landed branches are still there. `git branch -D` is +blocked by the permission classifier, which is a reasonable guard on a destructive +command. All 19 are verified safe by the rule above. Run this to clear them: + +``` +git branch -D bugfix-code-review docs-story-tree fix-prepend-autoscroll \ + fix-silent-clamps-and-milestone-ids fix-store-refusals-and-delta-wording \ + handover-2026-08-17 measure-post-vacuum-sizes phase-14-story-tree \ + phase-7-public-repo phase-8-accounts sp1-tree-schema sp10-memory-bank-eviction \ + sp2-branch-clause sp3-node-cursors sp4-sibling-nodes sp5-fork-on-continue \ + sp6-bundle-v2 sp7-tree-ui sp7b-take-pager worktree-keep-fixture-and-status \ + worktree-simplify-comments +``` + +`worktree-keep-fixture-and-status` is the one that needed checking by hand. Its +commit subject appears nowhere in `main`, but the `--keep` flag it adds is on `main` +at `tools/stress_session.py:764`, along with both gotchas the message describes, at +`:686` and `:692`. Only the message differs. + +The remote branches are left alone. Deleting those is a separate decision. + +## Stage 1: split the four largest files + +Every change in this stage moves code. None of it changes behavior. The 549 tests +are the check, and they must pass without being edited, except where this section +says otherwise. + +### `routers/adventures.py` becomes a package + +Split it into `backend/app/routers/adventures/`: + +| Module | Holds | Source lines | +|---|---|---| +| `__init__.py` | The `APIRouter`, the shared dependencies, and re-exports | | +| `paging.py` | `ACTION_LIST_COLUMNS`, `ACTION_PAGE`, `action_window`, `annotate_takes`, `current_window` | 27-146, 1203-1261 | +| `crud.py` | List, create, get, patch, delete, plus the script-state and world-state readers | 225-580 | +| `turns.py` | The turn engine: `generate_turn`, `_generate_turn`, `run_player_turn`, the turn lock, and the SSE helpers | 581-1004 | +| `takes.py` | Retry, variants, takes, forking, and undo | 1005-1192, 1503-1778 | +| `branches.py` | The branch endpoints | 1193-1502 | +| `bundle_io.py` | Export and import | 1779-1837 | +| `scripts.py` | The per-adventure script endpoints | 1838-1941 | +| `refresh.py` | Refresh from scenario | 1942-2152 | +| `insights.py` | The context dry run and the per-action context | 2153-2189 | +| `memories.py` | Memory bank CRUD | 2190-2279 | + +Target no file above 450 lines. + +**The tests couple to the module, so read this before you start.** Twelve test +files call `monkeypatch.setattr(adventures, "OpenAICompatibleProvider", ...)`. +`monkeypatch` replaces a name in the module where the calling code looks it up, so +re-exporting from `__init__.py` does not keep those patches working. Once +`_generate_turn` lives in `turns.py`, the target becomes +`adventures.turns.OpenAICompatibleProvider`. + +Do Stage 2's `conftest.py` work first if you want that to be a one-line change +instead of twelve. Otherwise retarget all twelve here. The other patched names are +`adventures.limits`, `adventures.check_demo_cap`, `adventures.generate_turn`, and +`adventures._active_turns`, each used once. + +Keep these importable from the package root, because tests import them by name: +`ACTION_PAGE`, `SNIPPET_MAX`, `_snippet`, `world_delta_of`, `acquire_turn_lock`, +`retry_action`, `undo_turn`, and `_active_turns`. + +`_active_turns` is module-level mutable state guarded by a lock. It must live in +exactly one module, `turns.py`, and every other module must import the module and +reach through it. If two modules import the set by value, the lock guards two +different sets and the turn lock stops working. + +### `worldstate/engine.py` becomes a package + +Split `backend/app/worldstate/engine.py` into four modules under +`backend/app/worldstate/`: + +- `schema.py`: `has_schema`, `instantiate`, `reconcile`, `band_label`, `npc_name`, + `npc_triggers`, `_initials`. +- `parse.py`: `extract_delta`, `_tolerant_load`, `render_delta_block`, + `applied_delta`, `refusals`, `render_refusals`. +- `apply.py`: `apply_delta`, `apply_override`, and the shared path resolver Stage 2 + introduces. +- `render.py`: `render_state_section`, `render_reference`, `_stat_line`, + `_describe_stat`. + +`worldstate/__init__.py` re-exports every public name it exports today, so no call +site changes. The engine is imported as a module, not monkeypatched, so this split +carries none of the router's test coupling. + +### `pages/Play.jsx` becomes a directory + +Split it into `frontend/src/pages/Play/`: + +- `index.jsx`: the page component. +- `usePlaySession.js`: the 18 `useState` calls and 7 `useEffect` calls that drive + one adventure, behind one hook. +- `panels/`: `PlotPanel`, `MemoryPanel`, `ScriptsPanel`, `InsightsPanel`, + `BranchPanel`. +- `drawers/`: `StatusDrawer`, `WorldStateDrawer`, and the `StatRow`, `StatGroup`, + `StateTree`, and `StateValue` parts they use. +- `reports/`: `StateChangeChips`, `WorldStateReport`, `ScriptReport`, + `CacheReport`, `TokenBreakdown`. +- `TakePager.jsx`, `RefreshModal.jsx`. + +There is no frontend test runner until Stage 5, so this split is verified by +`npm run lint`, `npm run build`, and by driving the Play screen in a browser. Drive +it. `plan/STATUS.md` records that the last two Play bugs were both found by hand and +were unreachable from any test that existed. + +### `index.css` becomes a directory + +Split it into `frontend/src/styles/`, imported in order from `index.css`: +`tokens.css`, `base.css`, `nav.css`, `cards.css`, `play.css`, `drawers.css`, +`schema-editor.css`, `insights.css`, `modals.css`, and `theme.css`. + +Move each `@media` block next to the rules it overrides, rather than leaving one +`max-width: 720px` block at the end. Keep the source order identical when you move +rules, because CSS resolves ties by order and the file relies on that in at least +two known places, recorded in `plan/STATUS.md` and in the comments at +`index.css:1011` and `1082`. + +**Check:** 549 tests pass, `npm run lint` and `npm run build` are clean, and the +Play screen works in a browser at desktop and at 500 px wide. + +## Stage 2: remove duplication + +Each item here is small and is covered by tests that already exist. + +1. **One path resolver in `worldstate`.** `apply_delta` and `apply_override` route + `flags.`, `milestones.`, `npc..`, `world.`, and + `player.` with parallel code. Extract a resolver that returns the + container, the stat definition, and the kind, then let the two functions differ + only in the write rule. Their rules genuinely differ, so do not merge the + functions themselves. `apply_override` ignores `cooldown`, + `max_delta_per_turn`, and the rule that a counter cannot decrease, and it lets a + milestone be un-set. `tests/test_worldstate.py` covers both. +2. **Add `backend/tests/conftest.py`.** Move the temporary-database prologue there, + so the other 35 files drop 8 lines each. The prologue has to run before + `from app.main import app`, because `main.py` calls `bootstrap(engine)` at import + time. A `conftest.py` runs before any test module, which satisfies that. +3. **One fake provider.** Move `ScriptedProvider` and `FakeProvider` to `conftest.py` + as one fixture, replacing 12 copies. +4. **Move the SSE helpers out of the router.** Put `sse`, `SSE_HEADERS`, and + `turn_error` in `backend/app/sse.py`, so `chat.py` stops importing from + `adventures`. +5. **Make `get_adventure_or_404` a dependency.** About 20 handlers repeat the call. + A `Depends` removes the line from each one and puts the ownership check in the + signature, where a reader looking for it expects it. +6. **One `useDebouncedSave` hook** in `frontend/src/hooks/`, replacing the three + copies. +7. **Delete `Settings.stream`.** Remove the column, the two schema fields, and add + the migration that drops it. This is item S1 in `docs/self-review.md`. Mark it + applied there. + +**Check:** 549 tests pass. Test count may drop if consolidating fixtures removes a +duplicate case. If it does, say which case and why in the commit message. + +## Stage 3: SP8, drop the legacy columns + +This is the only stage that touches the production database. Follow +`plan/14-phase-story-tree.md`, which specifies it. + +1. Confirm nothing reads the eight columns. `ACTION_LIST_COLUMNS` in the adventures + package lists `index`, `variant_count`, and `variant_index` today, so that tuple + changes here. `bundle.py:37` and `context/history.py:424` both describe `index` + as unread; verify that rather than trusting the comment. +2. Add the migration that drops them. Remove the columns from `models.py`, and + remove the fields from `schemas.py` and from `ActionOut`. +3. Deploy, then run `VACUUM FULL actions;` on the direct Neon endpoint, not the + `-pooler` one. Dropping a column rewrites toasted values, and nothing reclaims + that space on its own. `plan/STATUS.md` records what happened the last time + nobody ran it: the database reached 144.2 MB against a 512 MB tier. +4. `VACUUM FULL` takes an `ACCESS EXCLUSIVE` lock, so the app blocks on `actions` + for the duration. It took 5.5 seconds at 144 MB. + +**Check:** 549 tests pass, `/api/health` answers `{"ok":true}` after the deploy, and +one existing adventure opens, takes a turn, retries it, and pages back through the +transcript. + +## Stage 4: documentation + +1. **Generate `docs/guide.html` from `docs/GUIDE.md`.** Write a small build script + that renders the Markdown into the existing hand-written HTML shell, keeping the + current styles and metadata. After that, one edit updates both. Note in + `README.md` that the HTML is generated and that you edit the Markdown. +2. **Decide what `docs/architecture.html` is.** It has no Markdown source. Either + give it one and generate it the same way, or state at the top of the file that + it is hand-written, so the next person does not look for a source that does not + exist. +3. **Split `plan/STATUS.md`.** It is 63 KB, and most of it is dated session logs. + Keep "Pick up here", "Things worth remembering", and "Running things" in + `STATUS.md`. Move the dated entries to `plan/history/`, newest file first. +4. **Add `docs/DEVELOPING.md`.** Record how to run the app, how to run the tests, + where each subsystem lives, and the invariants a newcomer breaks first: + - Anything that reads `adventure.actions` during generation takes + `exclude_action_id`. It leaks into four places, not one. + - `memory_cursor` and `summary_cursor` are positions into `story_actions()`, and + `Memory.source_start` and `source_end` are `Action.index` values. The two + spaces diverge as soon as anything is deleted. + - `/auth/me` must not raise. It is the SPA's bootstrap call, so anything it + touches that can raise takes the whole frontend down. + - The global field rule in the stylesheet keys off `input[type=...]`, so a bare + `` and any new input type get browser default styling until you add + them. + - A modal opened from `.side-panel` needs `createPortal`, because the panel's + filling transform animation makes it the containing block for + `position: fixed`. + +**Check:** the generated HTML matches the Markdown section for section, including +section 3.6. Every link in `README.md` and `docs/index.html` resolves. + +## Stage 5: a frontend test runner + +`plan/STATUS.md` names the missing runner as the reason this project keeps finding +UI bugs by hand. Two shipped bugs were unreachable from any test that existed. + +1. Add Vitest, React Testing Library, and jsdom. Add a `test` script to + `frontend/package.json`. +2. Add the step to the existing `frontend` job in `.github/workflows/ci.yml`, + between lint and build. +3. Write the first tests against the two bug classes that already recurred: + - The take pager renders when a retry's reply arrives over SSE. The stream builds + its own `ActionOut`, so it was the one payload that never carried the + annotation. + - Retaking a player turn does not produce `> You > You`. The editor is seeded + with stored text that is already formatted. +4. Add a test for `usePlaySession` from Stage 1, since that hook now holds the state + the page used to hold inline. + +**Check:** `npm test` passes locally and in CI. + +## Risks + +| Risk | Where | How you catch it | +|---|---|---| +| A monkeypatch silently stops patching, and a test passes while calling a real provider | Stage 1, `routers/adventures` split | After retargeting, break the fake on purpose and confirm the tests that use it fail | +| `_active_turns` ends up imported by value into two modules, so the turn lock guards two sets | Stage 1 | `tests/test_state_revert.py` reaches for `adventures._active_turns`. Keep that import path working, and confirm a second concurrent turn still returns 409 | +| A CSS rule changes meaning because the split reorders it | Stage 1, stylesheet split | Compare the built CSS before and after. Order within each section must not change | +| The column drop rewrites the table and nobody reclaims the space | Stage 3 | Run `VACUUM FULL actions;` on the direct endpoint, and read sizes from `sum(octet_length(col))` rather than `n_live_tup` | +| Splitting `Play.jsx` breaks something no test covers | Stage 1 | Drive the Play screen by hand at both widths. Stage 5 exists to shrink this risk for next time | + +## What this phase does not do + +- It does not replace the hand-rolled migration runner with Alembic. 64 migrations + run correctly, and `docs/GUIDE.md` section 2.6 already explains the choice. +- It does not move `bootstrap(engine)` out of import time in `main.py`. The current + behavior means a failed migration means no service, which is deliberate. Stage 2's + `conftest.py` removes the boilerplate that import-time bootstrapping forces on + tests, which is the part that actually hurts. +- It does not change any API shape, any database content, or any prompt. diff --git a/plan/STATUS.md b/plan/STATUS.md index edd1924..90aeb65 100644 --- a/plan/STATUS.md +++ b/plan/STATUS.md @@ -78,6 +78,10 @@ needed; nothing requires reading a row of anyone's story. ## Pick up here +**`plan/17-refactor.md` is the active phase.** It carries its own progress table, which +is the first thing to read when you pick the work back up. It runs in six stages, and +SP8 below is stage 3 of it. Nothing in it changes what the app does. + **`plan/14-phase-story-tree.md`, SP8 — drop the legacy columns.** SP8 was gated on the tree being proven live, and it now is: SP9 merged, and production answers `/api/health` with the tree schema in place. SP8 drops `index`, `variants`, `variant_index`,