LithosAnanke/.claude/memory/code-review.md

# Code review notes (session 1, 2026-11-11, on StarForth-v4.0.0 @ 06f1d41a)

Per Captain Bob's Law these are FINDINGS TO REPORT, not fixes. Severity is my judgement:
**H** can give wrong results or lose data in a path that runs today; **M** latent (not reached by the products
yet, or needs unusual input); **L** comments, naming, hygiene.

## v4 engine (`v4/src`) — read: exec.c, node.c, stack.[ch], fabric.c, wire.c, manage.c, heat.c
Overall: careful C99. All arithmetic in `v4_ucell` (no signed-overflow UB), D-14/D-16 guards before every
opcode, the engine is libc-free so the kernel links it unchanged. Comments state the rulings they implement.

- **M — `manage.c` pops and pushes without checking stack depth.** `v4_manage_serve()` takes 1–4 arguments
  with `v4_dstack_pop()` and never checks `n->ds.depth` (contrast `v4/system/blocks.c:99`, which refuses a block
  request with too few values — added after the MESH step-6 review found the same hole there). `v4_dstack_pop`
  on an empty stack does not fault; it returns whatever is in the ring. So `NODE-KILL`, `NODE-WIRE`,
  `NODE-SLEEP` etc. typed with too few values act on stale numbers, and the result pushes can saturate a full
  stack silently. Latent today: only tests call it (products are one node, MESH steps 8–9 not built).
- **M — `1u << (V4_PORTS + k)` in `fabric.c:293` (look_about)** is undefined once `V4_PORTS > 16`; nothing
  checks it at compile time. This is exactly issue #20, confirmed in code. `node.h` defaults `V4_PORTS` to 8.
- **L — stale comments, already known:** `stack.h` lines 20–30 ("no overflow detection, and there never will
  be") (DATASHEET §13 item 4) and `stack.c` lines 27–30 say the same and hardcode "mod 8" though the ring size
  is a build parameter (host node 30/31).
- **L — `heat.c` `call_index()` comment reads as an unfinished thought** ("hash-mod or just take low bits? Or
  simply if in range use it", "ignore for now? Or clamp?"). The code is the documented D-6 stand-in, and ENGINE
  step-4 Part A (A1) plans to delete `call[]`, the freeze mask and `v4_heat_on_call` — Part A not started.
  Under the "no TODO-shaped comments" standing rule this comment is exactly the kind that should not stand.
- **L — `node.c` store/fetch compare against attach registers that are `-1` when detached** ("no valid address
  equals"). Safe for opcode paths (addresses are checked first) but a direct C caller passing `-1` would hit the
  console/stack-register branch. Only reachable from C.
- OK, checked: branch target page-relative (iword), `unext` restart without refetch, resume after blocking,
  fault cancels resume, store refused by the node does not advance A/P (the 11-02 review fix `fd208825`),
  wire put/take/move all-or-nothing, queue pool free list, wire cut gives queues back at both ends.

### manage.c finding — confirmed latent (2026-11-11)
`v4_manage_serve` has no caller outside `v4/tests/test_manage.c` (grep of kernel/src, v4/system, v4/tools).
`boot.c:266` serves only blocks and the kernel's word table. test_manage.c tests refusals and depth *after*,
never a request made with too few values. So: real hole, not reachable from a product today; becomes
reachable at MESH steps 8–9. A test written first (Bob's rule) would show it.

## v4 system (`v4/system`) — read: boot.c run_line request loop, post.c judge loop, blocks.c guards
- **L/M — `boot.c` run_line, DONE path:** after the text ends, `for (i < 4096u && !doing && !stopped && !asking)
  boot_exec(b)` — if the node has not settled in 4096 steps the line is reported done anyway, with no sign.
  Bounded, but a silent cut-off (the method's dominant failure mode). Worth a named check or a comment of why
  4096 is enough.
- OK: unknown kernel request → error 12 (D-18), not ignored; port with no one → V4_ERROR_NO_ONE; step limit
  → STOPPED; POST reads the stack before judging, resets it, reports a stopped node and ends POST.

## kernel v4 path (`kernel/src/v4`) — read: sk_v4.c, sk_v4_mint.c, sk_v4_marker.c, sk_v4_grow.c, identity.c 160–320
- **M — MINT-ZUSE says "failed -- nothing changed" when something has changed.** `failed()` is called after
  `gpt_write_one()` has rewritten the stick's GPT and after the first 32 Forth blocks are zeroed, so a mint
  that fails at `capsule_zuse_boot_genesis_mint`/read-back/marker write has already erased the stick. The
  message is false on those paths (only the size and YES refusals are truly "nothing written").
- **L — secrets left in stack memory on failure paths.** `sk_v4_mint_zuse` wipes `seed`/`rseed` only on success;
  every `return failed()` after the mint leaves them. `sk_v4_genesis` (identity.c ~237) never wipes `seed`/`rseed`
  at all, while `sk_v4_real_zuse_arrived` wipes `seed`. Inconsistent; the seed is also kept in `g_zuse_seed`
  by design, so this is hygiene, not a leak path I can show.
- **L — fatal paths spin.** `sk_v4_run` ends in `for (;;) { }` on "not started", "did not let go of POST's
  blocks" and "stopped on a fault": a 100% core spin, the thing #151 fixed at the prompt with `arch_halt()`.
- **L — `sk_v4_grow.c` `dev_rw4k`: `(uint32_t)devblock * 4u`** truncates for volumes past 4 TiB of 1 KiB blocks.
  Unreachable with the 256 GiB Artemis cap (`STICK_ARTEMIS_CAP_GIB`), so latent only.
- **L — `sk_v4_serve` ignores request numbers other than 1 and 2** silently; safe because boot.c only calls it
  for 1..word_count (2), but the guard lives in the caller.
- OK: marker v1/v2 read with CRC and `has_real > 1` refused; marker written only after mint read-back
  (STICK.md 6 order); line editor's over-long line refused whole (not truncated and run); logout ends a
  waiting line (Bob 2026-10-26); size message buffer sized right (28 bytes incl. NUL).

## Not yet reviewed (honest)
identity.c 320–657 (hot-plug, offered stick, beat), gpt.c, usb/xhci.c, usb_kbd.c, hal, boot loader, the rest of
kernel/src (kernel_main.c 1986 lines, repl.c — v3 path), v4/system/post_cases.c, text.c, image.c, v4/tools,
capsules/v4, tools/mkcapsule.c, scripts/nightly.sh + qemu_session.py, .gitea/workflows, Makefiles, v3.