From 7adc43220d72300a81c83165243b11304ca3f9fc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 16:22:54 +0000 Subject: [PATCH] Code review notes, pass 2: identity hot-plug, GPT, USB, acceptance scripts Findings reported, not fixed (Captain Bob's Law). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017RzZgrf7V7Zk2iczBXDRbm --- .claude/memory/code-review.md | 64 +++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 3 deletions(-) diff --git a/.claude/memory/code-review.md b/.claude/memory/code-review.md index e1a36b59..4230628e 100644 --- a/.claude/memory/code-review.md +++ b/.claude/memory/code-review.md @@ -63,7 +63,65 @@ reachable at MESH steps 8–9. A test written first (Bob's rule) would show it. (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) -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. +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.