Claude
7adc43220d
Findings reported, not fixed (Captain Bob's Law). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017RzZgrf7V7Zk2iczBXDRbm
128 lines
11 KiB
Markdown
128 lines
11 KiB
Markdown
# 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).
|
||
|
||
## kernel v4 identity, the rest (identity.c 320–657) — hot-plug, offer, fixed screen
|
||
- **L — `sk_v4_idle` reallocates `g_usb[xc].slots` when `count < max_slots + 1` without copying or freeing the
|
||
old array.** `g_offered`, `g_spare`, `g_pending_real`, `g_real_drive` and `g_zuse_dev` (= `&d->part` inside
|
||
the old array) would then point into the abandoned array. Only reachable if a controller's `max_slots` grows
|
||
after the first allocation, which it does not today — latent, plus a leak.
|
||
- **L — partial state on a failed identity attach:** in `sk_v4_drive_arrived`, Artemis is attached and
|
||
`g_stick_in = 1` before the identity partition's `blk_subsys_attach_device`; if that fails the function
|
||
returns with Artemis attached and no identity, and `d->used` unset. A later pull detaches Artemis correctly
|
||
(`art_used`), so it is recoverable; the message names xhci, not the partition.
|
||
- **Ruled, not a finding:** the fixed screen lets anyone holding the system stick press R and mint a
|
||
replacement real Zuse (STICK.md §6a, Bob 2026-10-29: "the old stick dies for good"; copying a system stick
|
||
is "not guarded against", §6a line 252).
|
||
- OK: marker cleared when the system stick leaves; a real-Zuse stick seen before the system stick is held
|
||
(`g_pending_real`) and judged when it arrives; the offered stick is never the system stick
|
||
(`sk_v4_is_this_stick`); waits use `arch_halt()` (#151).
|
||
|
||
## kernel/src/gpt.c
|
||
- Sound overall: header CRC, entry-array CRC, `my_lba == 1`, entries inside the usable range, entry array before
|
||
first_usable and 1 KiB-aligned; writes go backup first, then primary, then the protective MBR last, with
|
||
flushes between, so a power cut leaves one valid table. `gpt_write_one` zeroes the MBR first.
|
||
- **L — `gpt_read` stops validating at `max` entries:** `for (i < nent && n < max)` — with `max = 4`
|
||
(`sk_v4_mint_zuse`) or `SK_V4_GPT_PARTS`, a disk whose identity/Artemis entry is past the first `max` used
|
||
entries reads as having none, and the entries after `max` are not range-checked. Harmless for this
|
||
system's own sticks (one or two partitions); worth knowing for foreign disks.
|
||
- **L — no backup-header fallback:** a damaged primary GPT returns GPT_BAD ("damaged partition table -- not
|
||
used") though the backup may be good. A choice, not a bug; the message is honest.
|
||
- **L — 32-bit fblock arithmetic** (`(uint32_t)(ent_lba / 2u) + i`, `st->first_fblock`): bounded by
|
||
`total_blocks` (uint32 of 1 KiB = 4 TiB), consistent with the block layer.
|
||
|
||
## kernel/src/usb (xhci.c descriptor walk, blkio_usb.c, usb_kbd.c)
|
||
- OK: device-controlled lengths are clamped — `xhci_ep0_get_config_descriptor` clamps the request to the
|
||
128-byte buffer, every descriptor walk checks `off + field < len` and breaks on `bLength == 0`; HID packet
|
||
capped at 64; keyboard report capped at 8 bytes, usage lookup bounds-checked; SCSI block size must divide
|
||
1 KiB, capacity saturates at 2^32 Forth blocks.
|
||
- **L/M — a device whose `wTotalLength <= 9` loops enumeration:** the second GET_DESCRIPTOR is again classed
|
||
`XHCI_XFER_CONFIG_DESC_SHORT` (`length <= 9`), whose handler asks again for `total_len` — for ever, for that
|
||
slot. A hostile or broken device only; `wTotalLength = 0` also issues a zero-length data stage.
|
||
- **L — config descriptors longer than 128 bytes are cut at 128.** Interfaces past byte 128 are never seen.
|
||
BOT is normally the first interface and a boot keyboard is small, so QEMU never shows it; a composite
|
||
device on real hardware (SER5 bring-up) could. No message says the descriptor was cut.
|
||
|
||
## scripts/nightly.sh, scripts/qemu_session.py, .gitea/workflows/acceptance.yml
|
||
- OK: `set -euo pipefail`, so a failing `qemu_session.py` or `make test-32` behind `| tee` fails the stage;
|
||
`POST: PASSED` is printed by `v4/system/boot.c` only when `v4_post_run` reported no failures, after the
|
||
`PARITY:V4_SYSTEM` line; one QEMU at a time via the shared `qemu` concurrency group;
|
||
`git checkout -- disk/zuse-scratch` before each architecture.
|
||
- **L — the dict_hash agreement check is two-of-three tolerant:** the stick-high stage collects hashes with
|
||
`sort -u` and passes if exactly one distinct value appears. An architecture whose log has no
|
||
`PARITY:V4_SYSTEM` line (or whose `LOG:` line is missing) contributes nothing and the check still passes.
|
||
Today each arch's own stage would have failed first (no POST: PASSED), so it is belt without braces: count
|
||
three hashes, not one distinct value.
|
||
- **L — the POST test count is not asserted.** The harness checks `POST: PASSED` (and `fail=0` only in the
|
||
`stick_v3` mode); a POST that lost cases would still pass. CLAUDE.md says "tests=538 (the count grows)", so
|
||
this is policy, but a floor would catch a silently shrinking POST.
|
||
- **L — acceptance without `CAPSULE_SIGN_KEY` runs unsigned with a warning, and passes.** Fine for a fork;
|
||
for the gate into `master` an unsigned pass may not be what is meant.
|
||
|
||
## Not yet reviewed (honest)
|
||
kernel_main.c (1986 lines) and repl.c (the v3 path), hal, the UEFI loader, virtio, the block subsystem
|
||
(`block_subsystem.c`), capsule_zuse_boot / homeblocks_sig / artemis_sig, v4/system/post_cases.c, text.c,
|
||
image.c, v4/tools (mkrom, mkmap, hosted), capsules/v4/*.4th, tools/mkcapsule.c beyond the #143 warnings,
|
||
qemu_session.py modes other than zuse/stick_genesis, build/nightly/release workflows, Makefiles, v3.
|