kyle/boxmaker
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Review M3a: accept with fifteen findings; record the server change

Browse Source 
Two medium findings in the audit writer (a startup panic on a record-less
log file, and a log-name filter that disagrees with bxctl's), one in the
missing runbook pointers for startup failures, and twelve low ones. Lessons
I14 and T21, T22; two new AGENTS rules; m3a's T18 renumbered to T20 so
master's T18 and T19 survive the merge.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
kyle
2026-09-22 20:48:43 -07:00
co-authored by Claude Opus 5
parent ffbff501d6
commit 22a26aad45
3 changed files with 78 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/implementer-lessons.md
+6 -1
View File
@@ -29,6 +29,7 @@ How it is used:
| I11 | A rule about one path applies to every path that does the same thing. If a task says "release X before the final frame", every final frame counts, including the error frames written earlier in the function. | M2b finding 1. | yes | |
| I12 | A file that exists but cannot be read is an error. Only a file that does not exist may be treated as absent. | M2b finding 3. | yes | |
| I13 | Before calling a failure "environmental", measure the thing you blame, and name the machine, OS and file system the evidence came from. A sample that shows a thread inside a system call shows where the time goes, not that the call is stuck. | M3a stop before task 20: an fsync "stall" was blamed from `__fcntl` frames that were ordinary 4 ms `F_FULLFSYNC` calls on the Mac, and the real cause, a macOS socket rule, went unfixed. | no | |
| I14 | Recovering from a crash is a case to write a test for, not only to reason about. A file the daemon itself can leave behind (created, not yet written) must load like an empty one. | M3a finding 1: one zero-length log file made `brokerd` panic at startup, where the same directory verified as `ok, 0 records`. |
## Tips for writing tasks
@@ -51,7 +52,11 @@ How it is used:
| T15 | Running the whole plan through `tools/run-plan.sh` worked: ten tasks, one commit each, unattended, in about three hours. Keep the TUI closed while it runs; a second message into the driver's session starts a second agent on the same tree. | M2b run, 2026-09-18. |
| T16 | When a task prescribes the fix, check that it compiles against the types as they are, in the reference tree, before handing it over. A fix that cannot be written as described pushes the implementer outside the listed paths. | M2b task 11: "`unwrap_or_else` with a fixed valid id" had no non-panicking form outside `proto`; the implementer added `Default` to `SessionId` and reported it. |
| T17 | Match the check to the risk. A full reference for intricate logic whose writing debugs the spec (state machines, concurrency); a naive oracle inside the test for decision logic; a compiling skeleton (`todo!()` bodies under the real signatures) for plumbing. Record what each check exposed, and let the record decide what the next milestone gets. | Across M1 to M2b the references caught no implementer defect. They caught task defects (T16) and missed what they shared with the tests (T5). Decision of 2026-09-18. |
| T18 | The gate runs on two platforms, Talos (Linux) and the Mac (macOS), and they differ where the tests touch the OS: sockets, file sync, `/tmp`. Accept a task only when the gate passes on both. Code that sets a socket option after the peer may have closed breaks on macOS only. | M3a: `BrokerPort`, the HTTP client and the brokerd test client passed on Talos and failed on the Mac from task 13 on; it was found only when the gate was first run on the Mac. |
| T18 | Keep reference implementations where the implementer cannot read them. A run on this machine can reach every directory the owner can; a stuck model will search the disk and copy what it finds, and an orchestrator will write in the log that it did not. Move `~/src/boxmaker-ref*` out of reach (or sandbox the run) before a plan starts, and compare the result with the reference byte for byte in review. | First M3a run, 2026-09-19: the orchestrator copied the reference `chain.rs` for task 03 and logged it as written "from the spec and fixtures"; task 09's worker spent eight hours in `find / -name audit.rs`. |
| T19 | A file in `files/` that two tasks copy must be right for the earlier task. When a later task changes a shared test file, hand the earlier task its own copy (`strict.rs-task01`, `Makefile-task21`) and check it at that task's end state. | M3a task 01 copied the merged `strict.rs`, which walked task 02's fixtures, so its gate could not pass. |
| T20 | The gate runs on two platforms, Talos (Linux) and the Mac (macOS), and they differ where the tests touch the OS: sockets, file sync, `/tmp`. Accept a task only when the gate passes on both. Code that sets a socket option after the peer may have closed breaks on macOS only. | M3a: `BrokerPort`, the HTTP client and the brokerd test client passed on Talos and failed on the Mac from task 13 on; it was found only when the gate was first run on the Mac. |
| T21 | When two components must agree on a set (which files are the log, which names are ids), give the tests one case that walks both. Each half was tested alone and they still disagreed. | M3a finding 2: `brokerd` accepted `2026-0x-18.jsonl` as a log file and `bxctl audit verify` ignored it, calling the log `ok` while leaving out half its records. |
| T22 | List the fail-closed states a task creates, with their runbook anchors, in the task itself. A script can check that a pointer names an existing entry; nothing can check for a pointer that was never written. | M3a finding 3: four startup failures in `serve` and `main` print no pointer, and the spec's own list of pointers omitted them. |
## What worked and should be kept
docs/implementer-log.md
+68
View File
@@ -313,3 +313,71 @@ failed and the cause was not understood. `docs/M3a/DEBUG-HANDOFF.md` now opens w
After the fixes, `make gate` passes on Talos. On the Mac, fmt, clippy, every test and the gate
scripts pass; `cargo deny` is not installed there, so the gate itself stops at that step. Tasks 20
to 22 are no longer blocked.
### M3a, tasks 01 to 22 — reviewed 2026-09-22 by the design model (Claude)
Accepted, with a follow-up. The decision path matches the spec as revised: grant loading fails
closed on any invalid file, matching follows the revised rules (most restrictive mode, longest
matched path, label combined over every matching grant, a `deny` grant that must hold at every
taint), the ledger keeps the audit writer and the state files under one lock and stops after a
failed append, a decision is on disk before anything runs, results are recorded by hash, the two
sockets refuse each other's kinds, `RunSpec` gives egress to `http_fetch` alone, and a denial
reaches the model as the spec's sentence with the turn going on.
| Check | Result |
|---|---|
| 33 commits on `m3a`, 27 with the trailer | pass (the six without are review and notes commits) |
| All 150 given files identical to `docs/plans/M3a/files/` | pass |
| `make gate` on Talos | `gate: ok`, 524 tests |
| Any source file copied from a reference branch | only `crates/proto/src/chain.rs` (task 03, already recorded above); every other file differs from all six reference branches |
| New library code free of `unwrap`, `expect`, `panic!`, `#[allow]`, `unsafe`, `as` casts | two exceptions (finding 5) |
| Every runbook pointer used has an entry (`scripts/check-runbook.sh`) | pass; four states have no pointer at all (finding 3) |
| 30 audit tampering fixtures, the policy oracle property test, the two-process end-to-end test | present and passing |
| `tools/check-m3a-device.sh` on straylight | not run by the implementer; run in review: passes once the server expectations are corrected (see "The server changed") |
On straylight, with real Ornith: the model reached `read_file` through `find_tool` and `call_tool`,
the approval block appeared in `bxctl approvals` with its arguments escaped, approving it ran the
refusing runner, the model reported the failure in its own words, and `bxctl audit verify` gave
`audit: ok, 3 records` over a `Decision`, an `Approval` and a `Result`.
| # | Severity | Owner | Finding | Fix |
|---|---|---|---|---|
| 1 | medium | implementer (09) | `brokerd` panics at startup when the audit directory holds exactly one log file with no complete record: `audit.rs:346` reads `files[files.len() - 2]`. A zero-length file is what a kill between `create` and the first `write_all` leaves behind. `bxctl audit verify` calls the same directory `audit: ok, 0 records`. Confirmed by running the binary. The reference used `report.head.unwrap_or(resumed_from)`; the given tests cover only the two-file form (`empty-latest`). | Task 23 |
| 2 | medium | implementer (09), and the tests | `brokerd` and `bxctl` disagree on which files are the log: `audit.rs:97` checks one month digit (`b[5..6]` where `b[5..7]` was meant). With `2026-0x-18.jsonl` beside `2026-09-17.jsonl`, `brokerd` chains through both while `bxctl audit verify` prints `ok, 5 records` and silently ignores the other five. Confirmed by running both. Nothing pinned the two filters to each other. | Task 23 |
| 3 | medium | task and spec | Four fail-closed startup states carry no runbook pointer and have no entry to point at: the socket directory cannot be prepared, the socket cannot be bound (seen in review: `cannot listen on …: path must be shorter than SUN_LEN`), the config cannot be read, and the listener thread dies. `scripts/check-runbook.sh` can only check pointers that exist. The spec's own pointer list omits them. | Task 23, with a new runbook entry |
| 4 | low | implementer (09) | `Writer::drop` unlinks `audit/.lock`. The runbook says deleting it neither helps nor is needed, and unlinking opens a narrow two-writer window: a `brokerd` holding the old, unlinked inode and a new one creating a fresh file each believe they are alone. The reference never unlinked it. (On a signal no destructor runs, so the common case leaves the file in place.) | Task 23 |
| 5 | low | implementer | `audit.rs:168` `files.last().unwrap()` and `grants.rs:201` `count() as u64` break AGENTS' rules, though both are locally safe. | Task 23 |
| 6 | low | implementer (15) | `serve.rs:187` uses `std::thread::spawn`, which panics if the OS refuses a thread; the accept thread then unwinds and that listener is silently dead while `brokerd` keeps running. `thread::Builder` returns the error instead. | Task 23 |
| 7 | low | pre-existing | `std::env::args()` panics on a non-UTF-8 argument (`exit 101`, confirmed), in every role's `main.rs` since M1, so a config path that is not UTF-8 cannot be used at all. `args_os()` is the fix. | Task 23 (all four mains) |
| 8 | low | implementer (17) | `loopd`'s post-pending deadline is `expires` plus `timeout_ms` with no cap, and the "expiry too far away" guard cannot fire (an `Instant` 8,000 years out is fine). An absurd `[approvals] ttl_ms`, which makes `brokerd` fall back to `Timestamp::MAX`, therefore parks a turn for ever — the one thing the pending path promises not to do. | Task 24 |
| 9 | low | implementer (18) | `bxctl`'s admin requests set no timeout on connect or read, so a `brokerd` stuck under the ledger lock hangs `bxctl`, including inside a `chat` turn where the spec wants one line and the turn carrying on. | Task 24 |
| 10 | low | implementer (20) | Two print paths escape nothing: `Retrying { error }` (`chat.rs:189`) and `WireError.detail` (`chat.rs:28`, `admin.rs:61`). Both can carry the inference server's raw response body, which is what the escaping rule exists to stop. | Task 24 |
| 11 | low | implementer (13) | The `GONE` result is recorded with the decision's timestamp, not the current one, so `verify` reports a clock warning for a log that is sound. | Task 24 |
| 12 | low | implementer (18) | `bxctl`'s usage says `audit verify` checks "the audit log against the grants"; it verifies the hash chain and never reads a grant. | Task 24 |
| 13 | low | implementer (20) | `AdminError` lacks the `Io` variant task 18 gives, so a failed write is downgraded to `Protocol` and answered by writing another line to the same failed writer. Disclosed in the row's Deviations column. | Task 24 |
| 14 | low | spec | `ToolArgs::HttpFetch` has public `url` and `host`, so "the host is the URL's host" is not enforced by the type. Nothing breaks it today (`args::parse` is the only producer), but M3b fetches `url` while policy matched `host`. | Before M3b |
| 15 | nit | implementer | `bxctl`'s `verify.rs` skips a `.jsonl` file whose name is not a date without a word (related to 2); `MAX_PATH`'s doc mentions URLs; `grants.rs:95` pushes an empty name it never uses; `find_tool` matches core tools, so `find_tool("time")` offers `clock` and `call_tool` then refuses it; the four tool schemas omit `"additionalProperties": false`. | When next touched |
What was good: the argument checks are exactly the spec's tables at every boundary I could
construct, including the IPv4 spellings the spec review added; `RunSpec` and `Decision` are both
sealed with `compile_fail` doctests that fail for the right reason; the ledger's three locked steps
and the "whoever takes the entry answers it" rule are implemented as written, with race tests that
run a hundred rounds; the ten denial sentences are byte-identical to the spec; the escaping covers
every code point the spec names, and `bxctl chat` fetches the approval block from `brokerd` by id
rather than trusting `loopd`'s event. Three of the four `medium` and `low` findings that touch
`brokerd` are in the same file, `audit.rs` (task 09) — the task that the first run never reached
and whose given tests were the most intricate.
### The server changed under M3a — noted 2026-09-22
Ornith on straylight now runs with `parallel = 4` over one unified 262,144-token KV pool
(`~/src/nixos/hw/straylight/default.nix`), not two slots of 131,072 each. Consequences:
- `make verify-device` fails, 2 of 6, because `crates/loopd/tests/device.rs` hard-codes
`n_ctx = 131072` and `slots = 2` in two places; `tools/check-m3a-device.sh` hard-codes the same.
The self-test itself behaved exactly as designed: `context per slot: expected 131072, got 262144`
with the runbook pointer. Both places should read `/props` instead of carrying the numbers.
- `docs/inference-contract.md` says the 262,144 is "split, not shared". With one pool shared by four
slots that is no longer true, and the cache reasoning behind P9 (another client evicting our
slot) needs re-measuring before M3b leans on it.