From ea93a9bd91c0de51e86e5b88c7eac2598815973f Mon Sep 17 00:00:00 2001 From: Nicholas Charriere Date: Wed, 29 Jul 2026 08:09:54 -0700 Subject: [PATCH] docs: add design review, decision log, and file-based task system Groundwork for restarting the project. Three additions, no code changes: - REVIEW.md: formal review of the prototype. Security findings verified against a running instance, the design forks worth reconsidering (execution model, .src.md grammar, session state ownership), and a code-quality pass. - DECISIONS.md: why the system is the way it is. Records the reasoning behind the choices that aren't obvious from the code, and flags which ones are under review. - tasks/: one markdown file per task, status in frontmatter, no tooling. Seeded with the ten tasks the review turned up. --- DECISIONS.md | 197 +++++++++++ REVIEW.md | 322 ++++++++++++++++++ tasks/0001-lock-down-network-surface.md | 33 ++ tasks/0002-fix-path-and-shell-injection.md | 34 ++ tasks/0003-stop-the-crashers.md | 36 ++ tasks/0004-test-safety-net.md | 33 ++ tasks/0005-fix-process-registry-clobbering.md | 42 +++ tasks/0006-decide-execution-model.md | 40 +++ .../0007-stop-sending-prompts-to-analytics.md | 28 ++ tasks/0008-cleanup-app-builder-remnants.md | 50 +++ tasks/0009-websocket-request-correlation.md | 40 +++ tasks/0010-session-state-ownership.md | 34 ++ tasks/README.md | 49 +++ 13 files changed, 938 insertions(+) create mode 100644 DECISIONS.md create mode 100644 REVIEW.md create mode 100644 tasks/0001-lock-down-network-surface.md create mode 100644 tasks/0002-fix-path-and-shell-injection.md create mode 100644 tasks/0003-stop-the-crashers.md create mode 100644 tasks/0004-test-safety-net.md create mode 100644 tasks/0005-fix-process-registry-clobbering.md create mode 100644 tasks/0006-decide-execution-model.md create mode 100644 tasks/0007-stop-sending-prompts-to-analytics.md create mode 100644 tasks/0008-cleanup-app-builder-remnants.md create mode 100644 tasks/0009-websocket-request-correlation.md create mode 100644 tasks/0010-session-state-ownership.md create mode 100644 tasks/README.md diff --git a/DECISIONS.md b/DECISIONS.md new file mode 100644 index 00000000..b578a39c --- /dev/null +++ b/DECISIONS.md @@ -0,0 +1,197 @@ +# Design decisions + +Why the system is the way it is. Append new entries at the bottom; don't rewrite history — +if a decision is reversed, add a new entry that supersedes it and mark the old one. + +Format: **status** is `active`, `superseded by #N`, or `under review`. + +--- + +## 1. A notebook is a real npm project on disk, not a document + +**Status:** active + +Each srcbook is a directory under `~/.srcbook/srcbooks//` containing a real +`package.json`, `tsconfig.json`, `node_modules/`, and one file per code cell under `src/`. +The `README.md` in that directory is the canonical source, written in the `.src.md` format +with cells linked to their files rather than inlined. + +**Why:** it makes the notebook runnable and debuggable by ordinary tooling. `tsserver` gets a +genuine project to analyse, so hover/completions/diagnostics are real TypeScript answers +rather than an approximation. `npm install` means any package on the registry works with no +special support. The escape hatch is good: a user can `cd` into the directory and run the +code with plain `node`. + +**What it costs:** a full `node_modules` per notebook (hundreds of MB across a few +notebooks), install latency before a notebook is usable, and two representations of the same +thing that must be kept in sync (the in-memory session and the files on disk). + +--- + +## 2. Cells execute as fresh child processes, not in a persistent kernel + +**Status:** active — but this is the single biggest fork in the design, see `REVIEW.md` + +Running a cell spawns a new process: `node ` for JavaScript, the notebook's own +`node_modules/.bin/tsx ` for TypeScript. The process exits when the cell finishes. + +**Why:** it's simple, it's honest, and it inherits Node's real module semantics. There is no +hidden interpreter state to explain, no "restart kernel" concept, and no divergence between +what the notebook does and what `node file.ts` would do. Cells share state the way normal +programs do — by importing each other. + +**What it costs:** the thing most people mean by "notebook" — incremental REPL state — does +not exist. You cannot define a variable in one cell and use it in the next. Every run pays +process startup plus TypeScript transpile (~300ms-1s). This makes srcbook a _literate +program_ rather than a _scratchpad_, which is a legitimate choice but a different product +from Jupyter or Livebook, and it should be a deliberate, stated position rather than an +accident of implementation. + +--- + +## 3. `.src.md` is markdown, and cell structure is encoded in heading levels + +**Status:** active + +A notebook serialises to markdown with an HTML-comment metadata header. `#` (h1) is the +title. `######` (h6) marks a filename, and the block immediately after it is that file's +contents. Everything else is prose. + +**Why:** the file renders as a readable document in any markdown viewer, on GitHub, in an +editor preview. Diffs are legible. There is no custom parser to maintain for consumers — a +`.src.md` degrades gracefully into ordinary markdown. This is the format's main advantage +over `.ipynb`, which is JSON that renders as noise and diffs as garbage. + +**What it costs:** the format is structurally ambiguous. An h6 in ordinary prose becomes a +filename marker. The grammar is defined by the decoder's behaviour rather than by a spec, +and the validation rules are thin (exactly one h1, at most one package.json, h6 must be +followed by code). See `REVIEW.md` for the specific fragilities. + +--- + +## 4. Cell outputs are not persisted + +**Status:** active + +Execution output is streamed to connected clients over the websocket and then forgotten. +It is not written into `.src.md`, not stored in sqlite, and not held in the session. + +**Why:** this is deliberate and correct. Persisting outputs is what makes `.ipynb` files +diff badly, bloat repositories, and leak secrets into version control. A `.src.md` describes +a computation; it does not memoise one. + +**What it costs:** reloading the page loses all output, and a client that connects while a +cell is running sees only the tail. The gap is not the storage decision — it's that there is +no server-side ring buffer for the _current_ run, which would fix the reload/late-join +problem without touching the file format. Worth doing. + +--- + +## 5. Notebook state lives in memory; sqlite holds only config and secrets + +**Status:** under review + +`sessions` is a module-level `Record` in `packages/api/session.mts`, +populated by scanning `~/.srcbook/srcbooks` at import time. sqlite (via drizzle) stores only +the config singleton, secrets, and secret↔session associations. + +**Why:** the filesystem is already the database for notebooks — decision #1 means the +directory is the truth. Duplicating it into sqlite would create a second source of truth to +reconcile. + +**What it costs:** boot cost is O(notebooks on disk) because every one is eagerly decoded. +More importantly, the store mixes mutation styles — some paths mutate `session.cells` in +place, others replace the whole object in the map — so any closure holding a session +reference silently diverges. The source comments admit this (`// TODO: Real state management +pls.`). See `REVIEW.md`; this needs a single owner for session state. + +--- + +## 6. Cell IDs are generated at decode time and are not persisted + +**Status:** under review + +`decode()` calls `randomid()` for every cell it produces. IDs are stable for the lifetime of +a session in memory, but a fresh decode of the same file yields entirely new IDs. + +**Why:** the format stays clean — no machine identifiers cluttering a human-readable +document. + +**What it costs:** nothing can reference a cell durably across a reload. Comments, saved +outputs, per-cell metadata, deep links, and collaborative editing all need a stable handle. +Note that code cells _already_ have one — the filename — and it's what the tsserver +integration actually keys on. The tension is only real for markdown cells. Worth deciding +explicitly rather than leaving implicit. + +--- + +## 7. The websocket protocol is a hand-rolled Phoenix-style channel layer + +**Status:** active + +Messages are 3-tuples `[topic, event, payload]`. Topics support `` wildcards +(`session:`). Each event registers a zod schema, and payloads are validated on +receipt. There is exactly one channel in the app. + +**Why:** the tuple wire format is compact and the topic/event split maps cleanly onto +"which notebook" and "what happened". Per-event zod schemas shared between client and server +via `@srcbook/shared` mean the protocol is typed end to end, which is genuinely nice and +catches real mistakes. + +**What it costs:** about 270 lines of framework for one channel, and it reimplements things +a mature library gets right — the envelope itself isn't validated, handler rejections are +unhandled, and there is no request/response correlation, so every tsserver round-trip is a +broadcast that clients have to guess is theirs. See `REVIEW.md`. + +--- + +## 8. Secrets are injected as environment variables into the cell process + +**Status:** active + +Secrets live in sqlite, are associated with sessions explicitly, and are merged into the +child process env at spawn. An `env.d.ts` is generated so `process.env.FOO` type-checks. + +**Why:** environment variables are the idiom Node code already expects, so notebook code +needs no srcbook-specific API. Generating the type declaration is a genuinely nice touch — +it makes the secret discoverable through autocomplete. + +**What it costs:** secrets are stored in plaintext, and cells inherit the _server's_ full +environment on top of the associated secrets, so a notebook sees every variable the server +was started with, not just the ones the user associated. + +--- + +## 9. AI operates on the `.src.md` format itself + +**Status:** active + +Generation prompts serialise the notebook to inline `.src.md`, insert a placeholder cell to +mark the insertion point, and parse the model's reply back with the same decoder. + +**Why:** one representation for humans, disk, and the model. No separate AI-facing schema to +keep in sync, and the model's output is validated by the same parser that validates +everything else. + +**What it costs:** a malformed generation fails at the parser with a message aimed at the +document rather than the user, and there's no repair step. All calls are non-streaming, so +whole-notebook generation is a long blank wait. `finishReason !== 'stop'` only logs a +warning, so truncated output is silently accepted as if complete. + +--- + +## 10. The app trusts its network position — and shouldn't + +**Status:** under review — actively being fixed + +There is no authentication, no CSRF protection, and no origin checking anywhere. Every HTTP +route is wrapped in bare `cors()` (which is `Access-Control-Allow-Origin: *`), the websocket +accepts any origin, and the server binds `0.0.0.0`. + +**Why it ended up this way:** "it runs locally on your machine" was treated as a security +boundary. It isn't one. Localhost is reachable from every web page the user visits, and +`0.0.0.0` is reachable from every device on the network. + +**What it costs:** verified against a running instance — any website the user visits can read +their API keys, read arbitrary files from disk, and execute arbitrary code. This is the +top-priority fix; see `REVIEW.md` §1 and `tasks/`. diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 00000000..e3d5d446 --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,322 @@ +# Design review — July 2026 + +A formal review of srcbook as a prototype being restarted. Written from a full read of the +codebase plus empirical testing against a running instance (`srcbook@0.0.19`, Node 24). + +**Baseline health:** the monorepo installs, builds, and passes its 11 tests cleanly on Node +24 with no changes. That's better than most two-year-dormant projects. + +The review is in three parts: security (fix before touching anything else), the design forks +worth reconsidering, and the code-quality pass. + +--- + +## 1. Security: the local-app assumption is the central bug + +This is not a hypothetical. All of the following were confirmed against a live server. + +The server binds `0.0.0.0`, every route is wrapped in bare `cors()` (`Access-Control-Allow- +Origin: *`), the websocket does no origin check, and there is no authentication anywhere. +"It runs locally" was treated as a trust boundary. It is not one — every web page the user +visits can reach `localhost`, and every device on the network can reach `0.0.0.0`. + +Verified: + +``` +$ lsof -nP -iTCP:2150 -sTCP:LISTEN +node ... TCP *:2150 (LISTEN) # all interfaces + +$ curl -H "Origin: https://evil.example.com" localhost:2150/api/settings +Access-Control-Allow-Origin: * +{"result":{"openaiKey":...,"anthropicKey":...,"xaiKey":...}} # all provider keys + +$ curl -H "Origin: https://evil.example.com" -d '{"file":"/etc/hosts"}' localhost:2150/api/file +{"result":{"content":"##\n# Host Database\n..."}} # arbitrary file read + +$ node -e "new WebSocket('ws://localhost:2150/websocket', {headers:{Origin:'https://evil.example.com'}})" +WS ACCEPTED from foreign Origin https://evil.example.com # no origin check +``` + +The websocket acceptance is what turns this from disclosure into remote code execution: a +malicious page can list sessions over HTTP, then push `cell:update` followed by `cell:exec` +to run arbitrary code as the user. No user interaction beyond visiting the page. + +Two more, found by reading rather than testing (not exercised, because both are destructive): + +- **Path traversal to recursive delete.** `DELETE /api/srcbooks/:id` passes `req.params.id` + straight into `Path.join(SRCBOOKS_DIR, id)` and then `fs.rm(dir, {recursive: true})` + ([http.mts:106](packages/api/server/http.mts:106) → [index.mts:225](packages/api/srcbook/index.mts:225)). + Express URL-decodes params, so `..%2F..%2F..%2FDocuments` escapes the srcbooks directory. +- **Shell injection through a cell filename.** `formatCode` builds + `` `npx prettier ${codeFilePath}` `` and runs it through `exec`, which uses a shell + ([session.mts:330](packages/api/session.mts:330)). Renames are validated by `validFilename`, + but filenames arriving from a _decoded `.src.md`_ are not — the decoder only checks the + extension. An imported notebook containing `###### $(id).mjs` reaches a shell. Same shape at + [deps.mts:56](packages/api/deps.mts:56). + +Also worth fixing: `POST /api/settings` passes `req.body` unvalidated into +`db.update(configs).set(...)`, so an attacker can set `aiBaseUrl` and redirect the user's +subsequent AI calls — and their API key — to an endpoint of their choosing. + +**Recommendation.** Bind `127.0.0.1` by default. Drop blanket `cors()` in favour of an +explicit localhost allowlist. Add origin verification to the websocket upgrade. Delete +`POST /api/file` or restrict it to `SRCBOOKS_DIR`. Resolve-and-contain every path built from +user input. Use `execFile` rather than `exec` everywhere. Validate the settings body with zod. + +None of this compromises the local-first design; it just stops treating "local" as "safe". + +--- + +## 2. Design forks worth reconsidering + +### 2.1 Execution model — the big one + +Fresh process per cell is a real choice with real benefits (see `DECISIONS.md` #2): honest +Node semantics, no hidden kernel state, and the notebook is a program you can also run with +plain `node`. But it means srcbook is not a notebook in the sense most users expect. You +cannot bind a value in one cell and use it in the next. Every run pays process startup plus +tsx transpile. + +The interesting comparison is Livebook, which srcbook takes inspiration from. Livebook has +persistent BEAM process state _and_ reproducibility, because Elixir gives it immutability +and supervised processes for free. JavaScript gives you neither, so you have to pick. + +Three coherent positions: + +1. **Stay stateless, and say so.** Lean into "literate program, not scratchpad". Then invest + in what that model makes possible: fast reruns, a "run all" that's genuinely reproducible, + watch mode, importable notebooks. The main fix needed is latency — a warm tsx process pool + would cut most of the per-run cost. +2. **Add an opt-in kernel.** A long-lived worker per notebook that evaluates cells into a + shared context. This is what unlocks data-exploration workflows. It costs you a + "restart kernel" concept and the out-of-order-execution confusion that plagues Jupyter. +3. **Hybrid.** Stateless by default, with a per-notebook toggle. Attractive, but two + execution semantics means every feature is designed twice — this is the expensive option + dressed as a compromise. + +My read: (1) is the honest position and the one the current architecture already earns. +The differentiator against Jupyter isn't "notebook for TypeScript" — it's "a runnable, +reviewable, diffable document that happens to have a nice editor". Chasing kernel state +gives up that advantage to compete on Jupyter's turf. But this deserves an explicit decision, +because right now it reads as an implementation detail rather than a stance. + +Regardless of which is chosen: there are no execution timeouts, no output limits, and no +memory limits. A cell containing `while(true) console.log('x')` will stream unbounded output +over the websocket until the browser dies. `SIGTERM` is sent to the `tsx` wrapper, not the +process group, so grandchildren survive a stop. + +### 2.2 The `.src.md` format + +The core instinct — markdown that renders and diffs well — is right, and it's the most +defensible thing about the project. `.ipynb` diffs are notoriously unreadable; a `.src.md` +reviews like prose. Keep this. + +The fragilities are in the grammar, not the concept: + +- **h6 is overloaded.** Any h6 in prose becomes a filename marker. The delimiter is doing + double duty as both markdown structure and cell boundary. A fenced-block info-string + convention (` ```ts filename=foo.ts `) would be unambiguous and still render fine. + That's a breaking format change, so it belongs in a v2 discussion — but the current + ambiguity is worth documenting even if you keep it. +- **The grammar is defined by the decoder, not a spec.** There's no formal description of + what's valid, so the only way to know is to read `decoding.mts`. For a format whose whole + pitch is portability, that's a gap. A one-page spec plus a corpus of fixture files + (valid and invalid) would make third-party implementations possible. +- **`encode()` assumes cell ordering that `decode()` never enforces.** + [encoding.mts:17](packages/api/srcmd/encoding.mts:17) casts `cells[0]` to a title and + `cells[1]` to a package.json positionally. The decoder has a standing TODO admitting it + doesn't guarantee that. A notebook with a paragraph before `###### package.json` — easily + produced by AI generation — throws a `TypeError` on export. +- **Round-trip is tested for the inline form only,** by a single assertion. The external + (directory) form, which is what's actually on disk, has no round-trip test at all. +- **`decode()` throws instead of returning `{error: true}`** when metadata is missing + ([decoding.mts:92](packages/api/srcmd/decoding.mts:92)), violating the `DecodeResult` + contract every caller checks. + +Highest-value work here is a property test: generate arbitrary cell arrays, round-trip them, +assert equality. That would have caught all of the above. + +### 2.3 Session state ownership + +`packages/api/session.mts` mixes two update styles. `updateCellWithRollback` mutates +`session.cells` on the existing object; `updateSession` replaces the object in the map +(`sessions[id] = {...session, ...updates}`). Every exec callback closes over a session +reference captured at spawn time. + +The concrete bug: delete a cell while it's running, and the exit handler looks the cell up in +its _stale_ `session.cells` array, finds it, sets `status: 'idle'`, and broadcasts it — the +client resurrects a deleted cell. The code comments know: `// TODO: Real state management pls.` + +Fix is not exotic — make the session store the only thing that mutates sessions, have handlers +re-read by id rather than closing over the object, and pick one of mutable or immutable. But +it needs to happen before any multi-client or collaboration work, because it's the foundation +those would sit on. + +### 2.4 The websocket layer + +The typed-schema-per-event design shared across client and server is genuinely good and worth +keeping. Three structural gaps: + +- **No request/response correlation.** tsserver replies (`quickinfo`, `completions`, + `definition`) are _broadcasts_ carrying no `cellId` and no request id + ([websockets.mts:115-130](packages/shared/src/schemas/websockets.mts:115)). The client + registers a global listener and assumes the next response is its own. Two cells requesting + completions concurrently resolve each other's promises. Adding a `ref` field to the envelope + fixes an entire class of bug. +- **Subscriptions don't survive reconnect.** The client reconnects with backoff and flushes + its queue, but never re-sends `subscribe`; `Channel.subscribed` stays `true` so it never + re-handshakes. Server-side subscriptions are per-connection and die with the socket. After + any reconnect the UI still sends but receives nothing — run a cell after a server restart + and it hangs on "running" forever. +- **Handler rejections are unhandled.** [ws-client.mts:250](packages/api/server/ws-client.mts:250) + calls `handler(...)` with no `await` or `.catch()`, and every handler is `async` and throws + freely (`findSession` throws on unknown ids). A stale session id from a reconnecting client + crashes the server. + +### 2.5 Per-notebook `node_modules` + +Correct for compatibility (decision #1), expensive in practice: a full dependency tree per +notebook, and a slow first-run before anything works. Worth exploring a shared content- +addressed store with per-notebook symlinks — pnpm's model — which would preserve the "it's a +real project" property while collapsing disk and install time. Not urgent, but it's the thing +that will feel worst as a user accumulates notebooks. + +### 2.6 Privacy: analytics capture user prompts + +`posthog.capture` sends the user's AI prompt text as an event property — the `query` for +notebook generation ([http.mts:151](packages/api/server/http.mts:151)) and `prompt` for cell +edits ([ws.mts:399](packages/api/server/ws.mts:399)). Prompts routinely contain proprietary +code, business context, and personal information. + +The privacy policy states no PII is collected. That claim and this code cannot both be true. +This is on by default in production builds, opt-_out_ via `SRCBOOK_DISABLE_ANALYTICS`. Drop +the prompt contents from the payload (keep the event and its length if the metric matters), +and consider making analytics opt-in on first run. + +--- + +## 3. Code quality + +### Crashers + +The server has several paths where a single bad input takes the whole process down. For a +tool that holds unsaved user work, these matter more than their individual severity suggests. + +1. **`deps.mts:64` throws inside an `exec` callback.** Not in the promise executor — in the + callback — so the surrounding `try/catch` cannot catch it and the promise never settles. + Any depcheck output without a `{...}` match kills the process. This runs on _every cell + execution_. +2. **Unhandled rejections from every websocket handler** (§2.4). +3. **Unvalidated envelope.** [ws-client.mts:204](packages/api/server/ws-client.mts:204) does + `JSON.parse(message)` inside a synchronous listener. One malformed frame ends the process. + (Per-event payloads _are_ safely parsed — only the envelope isn't.) +4. **`process.send` is called unguarded** ([server.mts:59](srcbook/src/server.mts:59)) behind a + `@ts-ignore`. It only exists when spawned with IPC, so running the server directly crashes + it _after_ printing "running at http://localhost:2150" — it looks healthy, then dies. + Confirmed by running it. +5. **tsserver stdout parse errors are fatal** — `parse()` throws inside a `'data'` listener. +6. **Process registry clobbering.** [processes.mts:17](packages/api/processes.mts:17) keys on + `sessionId:cellId`. A double-exec overwrites the entry, and the _first_ process's exit + handler then deletes the key belonging to the _second_ — leaving an orphaned, unkillable + process while `cell:stop` reports "no process exists". +7. **tsserver request promises never settle on failure.** + [tsserver.mts:84](packages/api/tsserver/tsserver.mts:84) stores a resolver by `seq` with no + timeout and no reject path. If tsserver dies, every pending hover/completion hangs forever + and the resolver map leaks. + +### Testing + +11 tests, all in `packages/api`, covering the srcmd decoder, tsserver message framing, and a +module that is dead code. Zero tests in `web`, `components`, `shared`, or the CLI. No HTTP +tests, no websocket tests, no execution tests. + +The framing tests are genuinely good — they cover multi-byte UTF-8 split across chunks, which +is exactly the subtle thing worth testing. The gap is that the highest-risk code has none: +`server/ws.mts` (892 lines, every handler), `session.mts` (cell CRUD and rollback), +`exec.mts`, `processes.mts`, `deps.mts`, and the channel-matching logic in `ws-client.mts`. + +Best first targets, in order — all pure or easily isolated, all currently zero-coverage: + +- `Channel.match` topic/wildcard parsing — pure, total, trivially testable +- `Processes` add/kill/clobber semantics — pure state machine +- srcmd round-trip as a property test, including the external form +- `shouldNpmInstall` dependency comparison — pure given fixture files +- HTTP route tests via supertest, especially the security boundaries + +### Dead code + +The app-builder removal (`c7a52cc`) left a lot behind: `ai/stream-xml-parser.mts` (207 lines, +still tested), `test/plan-chunks*.txt`, `server/utils.mts`, `constants.mts` `APPS_DIR`, +`packages/api/apps/templates/react-typescript/.gitignore`, and an orphaned drizzle migration +(`0011_apps_external_id_unique.sql`, absent from the journal). + +`packages/components` is now vestigial. It existed to share UI between the notebook and the +app-builder; with the app-builder gone there's exactly one consumer. Worse, `web` imports it +by deep source path (`@srcbook/components/src/components/ui/button`) rather than through its +entry point, so the built `dist/` and the whole barrel file are unused — while still being +published to npm at 0.0.7. The published barrel is also broken: `src/index.tsx:11` is missing +the `.js` extension that every sibling line has, so `dist/index.js` is unimportable under +Node ESM. Either fold `components` back into `web` or make `web` import it properly; the +current arrangement has the costs of a package boundary with none of the benefits. + +### Packaging + +- **No `.dockerignore`.** `Dockerfile:8-9` copies `packages/` and `srcbook/` wholesale, so a + local build drags the host's `node_modules` — including darwin-compiled `better-sqlite3` + binaries — into an Alpine image. Highest-value one-line fix in the repo. +- **`srcbook` ships 10 unused dependencies** (`@ai-sdk/*`, `ai`, `better-sqlite3`, + `drizzle-orm`, `marked`, `posthog-node`, `zod`, `cors`) duplicated from `api`, plus + `depcheck` as a production dependency. Every `npx srcbook` user installs all of it. +- **No `files` field on `srcbook`**, so `.mts` sources and tsconfigs publish. `packages/api` + gets this right. +- **`web`'s build writes across a package boundary** (`cp -R ./dist/. ../../srcbook/public`) + with no dependency edge from `srcbook` to `@srcbook/web` to order it. `turbo start` cannot + guarantee `public/` is populated. +- `docker-compose.yml` sets `HOST` and `SRCBOOK_INSTALL_DEPS`; neither is read by any code. + `HOST` in particular reads as though it controls the bind address — it doesn't, which is + part of why the server binds `0.0.0.0`. + +### CI + +Runs build, lint, format check, and test on Node 18 and 22. Gaps: `pnpm install` without +`--frozen-lockfile` (lockfile drift never caught), the `check-types` turbo task is defined +but never invoked by anything, and Node 18 is tested but never used to build or ship — it's +been EOL since April 2025. Two commits sit on `main` with no changeset, so no release has +been cut for the app-builder removal or the AI SDK bump. + +### Frontend + +Reviewed in less depth than the server, but the load-bearing issues: + +- `use-cell.tsx` implements the cell store as four `useRef`s plus a `forceComponentRerender`, + and rebuilds the context value every render. Every keystroke in any cell re-renders every + cell, and `React.memo` below it can't help because the array identity is unstable. +- The refs exist explicitly to dodge stale closures around the websocket, which is treating + the symptom. An external store with `useSyncExternalStore` is the shape this wants. +- `install-package-modal.tsx:71` transitions to `success` whenever loading finishes, + regardless of outcome. `setMode('error')` is never called anywhere, so the error branch is + dead code and a failed npm install reports "Successfully added ". +- Loader data and props are mutated in place by `.sort()` in four places. +- A 28-prop component (`components/cells/code.tsx`), with all 28 spelled out at the call site + and 18 re-drilled one level deeper. + +--- + +## 4. What I'd do first + +In order, and roughly what the task files in `tasks/` reflect: + +1. **Security hardening** (§1). Nothing else matters until a web page can't run code on the + user's machine. Small, well-scoped, testable. +2. **Crash fixes** (§3), starting with `deps.mts` — it's on the cell-execution path. +3. **Tests for both of the above**, plus the pure modules that have none. This is how the + restart avoids re-breaking things. +4. **Decide the execution model explicitly** (§2.1) and write it into `DECISIONS.md`. Much of + what to build next depends on that answer. +5. **Session state ownership** (§2.3) before any multi-client work. +6. **Cleanup**: dead code, `.dockerignore`, unused deps, the `components` question. + +The prototype is in better shape than its dormancy suggests. The architecture is coherent, +the format instinct is right, and the tsserver integration is the kind of thing that's hard +to get working at all. What it needs is a security pass, a safety net, and one explicit +decision about what kind of notebook it wants to be. diff --git a/tasks/0001-lock-down-network-surface.md b/tasks/0001-lock-down-network-surface.md new file mode 100644 index 00000000..624a1527 --- /dev/null +++ b/tasks/0001-lock-down-network-surface.md @@ -0,0 +1,33 @@ +--- +id: 0001 +title: Lock down the network surface (bind, CORS, websocket origin) +status: TODO +created: 2026-07-29 +area: api +--- + +## Why + +Confirmed against a running instance: the server binds `0.0.0.0`, every route sends +`Access-Control-Allow-Origin: *`, the websocket accepts any origin, and there is no auth. + +Any web page the user visits can read their provider API keys from `GET /api/settings`, read +arbitrary files via `POST /api/file`, and — because the websocket accepts foreign origins — +push `cell:update` + `cell:exec` to run arbitrary code as the user. + +See `REVIEW.md` §1 for the reproduction. + +## Acceptance + +- Server binds `127.0.0.1` by default; a `HOST` env var opts into wider binding + (`docker-compose.yml` already sets `HOST`, which today is read by nothing) +- CORS replaced with an explicit localhost allowlist; cross-origin requests are rejected +- Websocket upgrade verifies `Origin` against the same allowlist +- The vite dev setup (`:5173` → `:2150`) still works +- Tests covering: allowed origin passes, foreign origin rejected, non-localhost bind requires + explicit opt-in + +## Notes + +Keep the allowlist shared between the HTTP and websocket paths — one source of truth, or they +will drift. diff --git a/tasks/0002-fix-path-and-shell-injection.md b/tasks/0002-fix-path-and-shell-injection.md new file mode 100644 index 00000000..688d7c28 --- /dev/null +++ b/tasks/0002-fix-path-and-shell-injection.md @@ -0,0 +1,34 @@ +--- +id: 0002 +title: Fix path traversal and shell injection +status: TODO +created: 2026-07-29 +area: api +--- + +## Why + +Two injection paths, both found by reading. Neither was exercised — both are destructive. + +**Path traversal → recursive delete.** `DELETE /api/srcbooks/:id` puts `req.params.id` +straight into `Path.join(SRCBOOKS_DIR, id)` and then `fs.rm(dir, {recursive: true})` +(`server/http.mts:106` → `srcbook/index.mts:225`). Express URL-decodes params, so +`..%2F..%2F..%2FDocuments` escapes the srcbooks directory. The call is also un-awaited, so a +failure surfaces as an unhandled rejection. + +**Shell injection via filename.** `formatCode` builds `` `npx prettier ${codeFilePath}` `` and +runs it through `exec`, which spawns a shell (`session.mts:330`). Renames go through +`validFilename`, but filenames from a decoded `.src.md` do not — the decoder only checks the +extension. An imported notebook with `###### $(id).mjs` reaches a shell. Same shape at +`deps.mts:56`. + +## Acceptance + +- Every path built from user input is resolved and asserted to be inside its expected root +- `validFilename` enforced at the decoder boundary, not just on rename +- No `exec` with an interpolated string anywhere; use `execFile` with an argument array +- Tests: traversal ids rejected, shell metacharacters in filenames rejected at decode + +## Notes + +A `containedPath(root, ...segments)` helper used everywhere beats auditing each call site. diff --git a/tasks/0003-stop-the-crashers.md b/tasks/0003-stop-the-crashers.md new file mode 100644 index 00000000..208b18bf --- /dev/null +++ b/tasks/0003-stop-the-crashers.md @@ -0,0 +1,36 @@ +--- +id: 0003 +title: Stop the process crashers +status: TODO +created: 2026-07-29 +area: api +--- + +## Why + +Several paths take the whole server down on a single bad input. The server holds unsaved user +work, so a crash is worse than its individual severity suggests. + +1. **`deps.mts:64` throws inside an `exec` callback** — not the promise executor, so the + caller's `try/catch` can't catch it and the promise never settles. Any depcheck output + without a `{...}` match kills the process. This runs on _every cell execution_. +2. **Unhandled websocket handler rejections** — `ws-client.mts:250` calls `handler(...)` with + no `await`/`.catch()`; every handler is async and throws freely (`findSession` throws on + unknown ids). A stale session id from a reconnecting client is enough. +3. **Unvalidated envelope** — `ws-client.mts:204` does `JSON.parse` in a sync listener. One + malformed frame ends the process. (Per-event payloads are safely parsed; the envelope isn't.) +4. **`process.send` called unguarded** — `srcbook/src/server.mts:59`, behind a `@ts-ignore`. It + only exists under IPC, so running the server directly crashes it _after_ printing + "running at http://localhost:2150". Confirmed by running it. +5. **tsserver stdout parse errors are fatal** — `parse()` throws inside a `'data'` listener. + +## Acceptance + +- Each of the five paths logs and degrades instead of exiting +- Tests: malformed websocket frame doesn't kill the server; depcheck garbage output resolves + to an empty list; server starts standalone without IPC + +## Notes + +Worth adding a top-level `unhandledRejection` handler as a backstop — but as a safety net +that logs loudly, not as the fix. diff --git a/tasks/0004-test-safety-net.md b/tasks/0004-test-safety-net.md new file mode 100644 index 00000000..e61151ae --- /dev/null +++ b/tasks/0004-test-safety-net.md @@ -0,0 +1,33 @@ +--- +id: 0004 +title: Build a test safety net for the untested core +status: TODO +created: 2026-07-29 +area: api +--- + +## Why + +11 tests exist, all in `packages/api`. They cover the srcmd decoder, tsserver message framing, +and one module that is dead code. Zero tests in `web`, `components`, `shared`, or the CLI. +No HTTP tests, no websocket tests, no execution tests. + +Restarting a dormant project without a safety net means re-breaking things silently. The +framing tests show the instinct is there — they cover multi-byte UTF-8 split across chunk +boundaries, which is exactly the right kind of test. The gap is that the riskiest code has none. + +## Acceptance + +Cover, in rough priority order: + +- `Channel.match` — topic parsing and wildcard extraction. Pure, total, currently untested +- `Processes` — add/kill/clobber semantics, including the double-exec case in task 0005 +- srcmd round-trip as a property test over generated cell arrays, including the external + (directory) form, which has no round-trip test today +- `shouldNpmInstall` — pure given fixture package.json/package-lock.json pairs +- HTTP routes via supertest, especially the security boundaries from tasks 0001 and 0002 + +## Notes + +`packages/api/package.json` runs bare `vitest`, relying on CI auto-detection to avoid watch +mode. Prefer `vitest run` for the CI script and keep `vitest` as a separate `test:watch`. diff --git a/tasks/0005-fix-process-registry-clobbering.md b/tasks/0005-fix-process-registry-clobbering.md new file mode 100644 index 00000000..78762183 --- /dev/null +++ b/tasks/0005-fix-process-registry-clobbering.md @@ -0,0 +1,42 @@ +--- +id: 0005 +title: Fix process registry clobbering on double-exec +status: TODO +created: 2026-07-29 +area: api +--- + +## Why + +`Processes` keys running children on `sessionId:cellId` (`processes.mts:44`). Executing the +same cell twice before the first exits overwrites the map entry — and then the _first_ +process's `exit` handler deletes the key now owned by the _second_: + +```js +this.processes[key] = process; +process.on('exit', () => { + delete this.processes[key]; +}); +``` + +Result: an orphaned process nobody can kill, while `cell:stop` reports "no process for +session … exists" even though something is very much running. + +Related, same area: `SIGTERM` goes to the `tsx` wrapper rather than the process group, so +grandchildren can survive a stop. Worth verifying whether `tsx` forks a child node process — +if it does, stop is only cosmetic for TypeScript cells. + +## Acceptance + +- Exit handler only deletes the entry if it still owns it (compare the process reference) +- Starting a cell that is already running either kills the old process first or is rejected — + pick one and make it explicit +- Test covering the double-exec sequence +- Verified answer on whether stop actually kills TypeScript cell processes + +## Notes + +Guarding the delete is a two-line fix. The "what should double-exec even do" question is the +real decision — the UI currently disables run while running, so this is reachable mainly via +a second client or a race, which makes it exactly the kind of bug that shows up once and is +never reproduced. diff --git a/tasks/0006-decide-execution-model.md b/tasks/0006-decide-execution-model.md new file mode 100644 index 00000000..af45d183 --- /dev/null +++ b/tasks/0006-decide-execution-model.md @@ -0,0 +1,40 @@ +--- +id: 0006 +title: Decide the execution model explicitly +status: TODO +created: 2026-07-29 +area: docs +--- + +## Why + +Cells run as fresh child processes, so there is no shared state between them — you cannot bind +a value in one cell and use it in the next. That's a legitimate, defensible design (see +`DECISIONS.md` #2), but right now it reads as an implementation detail rather than a stance, +and a lot of downstream work depends on which way this goes. + +The three coherent positions, expanded in `REVIEW.md` §2.1: + +1. **Stay stateless and lean in** — "literate program, not scratchpad". Invest in fast reruns, + reproducible run-all, watch mode. Needs a warm process pool to fix latency. +2. **Add an opt-in kernel** — long-lived worker per notebook evaluating into a shared context. + Unlocks data exploration; costs you "restart kernel" and out-of-order-execution confusion. +3. **Hybrid** — toggle per notebook. Looks like a compromise, actually the most expensive + option, because every feature gets designed twice. + +## Acceptance + +- A decision, written into `DECISIONS.md` as an entry that supersedes #2 +- If (1): a follow-up task for the warm-process pool +- If (2) or (3): a design sketch for kernel lifecycle before any code + +## Notes + +My read is (1). The differentiator against Jupyter isn't "notebook for TypeScript" — it's the +runnable, reviewable, diffable document. Chasing kernel state trades that advantage for a +fight on Jupyter's turf. But it's your call, and it's the highest-leverage open question in +the project. + +Independent of the answer: there are no execution timeouts, no output caps, and no memory +limits. `while(true) console.log('x')` streams unbounded output over the websocket until the +browser dies. That needs fixing either way. diff --git a/tasks/0007-stop-sending-prompts-to-analytics.md b/tasks/0007-stop-sending-prompts-to-analytics.md new file mode 100644 index 00000000..ef3bc1a6 --- /dev/null +++ b/tasks/0007-stop-sending-prompts-to-analytics.md @@ -0,0 +1,28 @@ +--- +id: 0007 +title: Stop sending user AI prompts to analytics +status: TODO +created: 2026-07-29 +area: api +--- + +## Why + +`posthog.capture` sends the user's prompt text as an event property — `query` for notebook +generation (`server/http.mts:151`) and `prompt` for cell edits (`server/ws.mts:399`). + +Prompts routinely contain proprietary code, business context, and personal information. The +privacy policy states no PII is collected; that claim and this code cannot both be true. +Analytics are on by default in production builds, opt-_out_ via `SRCBOOK_DISABLE_ANALYTICS`. + +## Acceptance + +- Prompt and query contents removed from all analytics payloads +- If the metric matters, keep a non-identifying proxy (length bucket, cell language) +- `PRIVACY-POLICY.md` matches what the code actually sends +- Consider opt-in on first run rather than opt-out + +## Notes + +Also audit the rest: `sessionId` and `cellId` are captured on every cell run, which is +high-cardinality and of dubious value. diff --git a/tasks/0008-cleanup-app-builder-remnants.md b/tasks/0008-cleanup-app-builder-remnants.md new file mode 100644 index 00000000..5f138834 --- /dev/null +++ b/tasks/0008-cleanup-app-builder-remnants.md @@ -0,0 +1,50 @@ +--- +id: 0008 +title: Clean up app-builder remnants and packaging debt +status: TODO +created: 2026-07-29 +area: ci +--- + +## Why + +`c7a52cc` removed the app-builder but left a trail, and packaging has drifted alongside it. +None of this is urgent; all of it is cheap, and it's the kind of debt that makes a restart +feel worse than it is. + +**Dead code** + +- `packages/api/ai/stream-xml-parser.mts` (207 lines) — only importer is its own test +- `packages/api/test/plan-chunks.txt`, `plan-chunks-2.txt` +- `packages/api/server/utils.mts` (`streamJsonResponse`), `constants.mts` `APPS_DIR` +- `packages/api/apps/templates/react-typescript/.gitignore` — sole survivor of a deleted tree +- `packages/api/drizzle/0011_apps_external_id_unique.sql` — absent from the journal +- `packages/shared/src/ai.mts` `isValidProvider` — zero call sites +- `packages/components/src/ui/heading.tsx` — duplicate of `src/components/ui/heading.tsx` +- `docker-compose.yml` `HOST` and `SRCBOOK_INSTALL_DEPS` — read by nothing + +**Packaging** + +- No `.dockerignore`, so `Dockerfile:8-9` drags the host's `node_modules` (including + darwin-built `better-sqlite3`) into an Alpine image. Highest-value one-line fix in the repo +- `srcbook/package.json` ships 10 unused deps duplicated from `api`, plus `depcheck` as a + production dependency; every `npx srcbook` user installs all of it +- `srcbook` has no `files` field, so `.mts` sources and tsconfigs publish +- CI uses bare `pnpm install`, so lockfile drift is never caught +- `turbo.json` defines `check-types`; nothing invokes it +- Two commits on `main` with no changeset — no release cut for the app-builder removal or + the AI SDK bump + +## Acceptance + +- Dead files deleted, unused deps dropped, `.dockerignore` added +- `--frozen-lockfile` in CI, `check-types` actually wired up +- Changesets added for the unreleased commits + +## Notes + +Separate question, worth its own task once decided: `packages/components` is now vestigial — +one consumer, imported by deep source path rather than through its entry, so its built `dist/` +and barrel are unused while still being published. The barrel is also broken under Node ESM +(`src/index.tsx:11` is missing the `.js` extension its siblings all have). Either fold it into +`web` or make `web` import it properly. diff --git a/tasks/0009-websocket-request-correlation.md b/tasks/0009-websocket-request-correlation.md new file mode 100644 index 00000000..9450885c --- /dev/null +++ b/tasks/0009-websocket-request-correlation.md @@ -0,0 +1,40 @@ +--- +id: 0009 +title: Add request/response correlation and reconnect resubscribe to the websocket +status: TODO +created: 2026-07-29 +area: shared +--- + +## Why + +Two structural gaps in the websocket layer, both producing bugs that look like flakiness. + +**No request/response correlation.** tsserver replies (`quickinfo`, `completions`, +`definition`) are broadcasts carrying neither a `cellId` nor a request id +(`packages/shared/src/schemas/websockets.mts:115-130`). The client registers a global listener +and assumes the next response is its own. Two cells requesting completions concurrently +resolve each other's promises; hovering in one cell can fill another's tooltip. A `ref` field +on the envelope fixes the entire class. + +Related: `get-completions.ts:27` returns early when the response is `null` without ever +resolving, so CodeMirror's autocomplete promise hangs forever whenever tsserver returns null. + +**Subscriptions don't survive reconnect.** The client reconnects with backoff and flushes its +queue but never re-sends `subscribe`; `Channel.subscribed` stays `true`, so it never +re-handshakes. Server-side subscriptions are per-connection and die with the socket. After any +reconnect the client still sends but receives nothing — run a cell after a server restart and +the UI hangs on "running" forever. + +## Acceptance + +- Envelope carries an optional `ref`; request-shaped events echo it in the response +- tsserver client helpers resolve/reject their own request only +- `null` responses settle their promise +- Reconnect re-subscribes every channel; test covers it +- Test: two concurrent completion requests get their own answers + +## Notes + +The reconnect bug is the more user-visible of the two and the cheaper fix — reset +`subscribed` on close and re-run the handshake on open. Do that first. diff --git a/tasks/0010-session-state-ownership.md b/tasks/0010-session-state-ownership.md new file mode 100644 index 00000000..73690e2c --- /dev/null +++ b/tasks/0010-session-state-ownership.md @@ -0,0 +1,34 @@ +--- +id: 0010 +title: Give session state a single owner +status: TODO +created: 2026-07-29 +area: api +--- + +## Why + +`packages/api/session.mts` mixes two update styles. `updateCellWithRollback` mutates +`session.cells` on the existing object; `updateSession` replaces the object in the map +(`sessions[id] = {...session, ...updates}`). Meanwhile every exec callback closes over the +session reference it captured at spawn time. + +The concrete bug: delete a cell while it is running. The exit handler looks the cell up in its +_stale_ `session.cells`, finds it, sets `status: 'idle'`, and broadcasts — so the client +resurrects a deleted cell. The same lookup is cast with `as CodeCellType` and immediately +dereferenced (`ws.mts:180`, `:214`), so a miss is a `TypeError` rather than a no-op. + +The code knows: `// TODO: Real state management pls.` + +## Acceptance + +- One module owns session mutation; nothing else writes to `sessions` +- Handlers re-read by id at the point of use rather than closing over the object +- Pick mutable or immutable and apply it consistently +- Test: delete a cell mid-run, assert no resurrect and no throw + +## Notes + +This is the foundation any multi-client or collaboration work would sit on, so it should land +before that rather than after. Also relevant to task 0006 — if execution ever becomes +stateful, the session store is where that state would live. diff --git a/tasks/README.md b/tasks/README.md new file mode 100644 index 00000000..15f1ff79 --- /dev/null +++ b/tasks/README.md @@ -0,0 +1,49 @@ +# Tasks + +A file-based task tracker. One markdown file per task, no tooling, no database. +Plain files so they diff, review, and grep like everything else in the repo. + +## Conventions + +Filename: `NNNN-short-slug.md`, zero-padded, monotonically increasing. + +Every task starts with YAML frontmatter: + +```yaml +--- +id: 0001 +title: Short imperative title +status: TODO # TODO | IN_PROGRESS | DONE +created: 2026-07-29 +area: api # api | web | shared | cli | ci | docs | format +--- +``` + +Then free-form markdown. Useful sections, in rough order of value: + +- **Why** — what breaks, or what's worse, without this +- **Acceptance** — how we know it's done +- **Notes** — findings, decisions made along the way, links to PRs + +## Lifecycle + +`TODO` → `IN_PROGRESS` → `DONE`. Edit the `status` field in place; don't move or rename the +file. A task's history then lives in git where it belongs, and `id` stays a stable reference. + +Prefer small tasks. If a task can't be described in a paragraph, it's probably two tasks. + +When a task turns up something worth remembering — a design decision, a reason, a constraint +that wasn't obvious — write it into `DECISIONS.md` rather than leaving it in the task file. +Tasks are ephemeral; decisions aren't. + +## Listing + +```bash +grep -h -e '^id:' -e '^title:' -e '^status:' tasks/*.md | paste - - - +``` + +Just the open ones: + +```bash +grep -l 'status: TODO' tasks/*.md +```