From 22a26aad45da1e4fca55972f020a6827473931bf Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Tue, 22 Sep 2026 20:48:43 -0700 Subject: [PATCH] Review M3a: accept with fifteen findings; record the server change 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) --- AGENTS.md | 4 +++ docs/implementer-lessons.md | 7 +++- docs/implementer-log.md | 68 +++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 5dd5b83..89495eb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -69,6 +69,10 @@ These come from defects found in review. The evidence is in `docs/implementer-le - A rule about one path applies to every path that does the same thing, including early returns and error paths the task did not walk through. - A file that exists but cannot be read is an error. Only a missing file may count as absent. +- A file your own program can create before it writes anything must load like an empty one. Test + the state a crash leaves behind, not only the states you write on purpose. +- When two programs must agree about a set of things (which files are a log, which names are ids), + one of them deciding alone is a bug. Write the case that walks both. ## The gate diff --git a/docs/implementer-lessons.md b/docs/implementer-lessons.md index 3b2a819..c8c5aa7 100644 --- a/docs/implementer-lessons.md +++ b/docs/implementer-lessons.md @@ -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 diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 94d6725..f5c9a29 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -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. +