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

Record task 23: the M3a review fixes, done by the design model

Browse Source 
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
kyle
2026-09-22 21:12:22 -07:00
co-authored by Claude Opus 5.5
parent f6841f1155
commit b1afcd5734
2 changed files with 38 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/implementer-log.md
+7 -6
View File
@@ -6,6 +6,7 @@ reviewer adds findings under "Reviews" once per milestone.
| Task | Date | Status | Gate runs | First gate | Deviations | Notes | Model |
|---|---|---|---|---|---|---|---|
| M3a/23-review-fixes | 2026-09-22 | done | 1 | pass | none | Done by the design model, not an implementer run: Ornith was under heavy contention. Findings 1 to 13 fixed, each with a test that failed first except 6, 11 and 13 (checked by reading); 14 moves to M3b's first task; of 15, the log-name rule, `MAX_PATH`'s doc and the unused push are fixed, and the two that change the baseline wait for an epoch change. The server expectations in the device tests were updated to the new layout. | Claude Opus 5.5 |
| M3a/22-end-to-end | 2026-09-22 | done | 1 | pass | none | Copied `end_to_end.rs`, `Makefile` (workspace build then ignored e2e with `BOXMAKER_BROKERD`) and `tools/check-m3a-device.sh`. Without the variable: 0 passed, 1 ignored. With it: 1 passed five runs. `--ignored` without the variable fails with "set BOXMAKER_BROKERD". `make gate` ran the e2e line (`1 passed`) and prints `gate: ok`. Did not run the straylight script. | Grok 4.6 |
| M3a/21-runbook-check | 2026-09-22 | done | 1 | pass | none | Wrote `scripts/check-runbook.sh`: find `*.rs` under crates (prune `target/`), awk out every `docs/runbook.md#` pointer, empty anchors fail, each remaining anchor must match a whole `## <anchor>` line, every missing one is reported with its files, then one exit. Step 5: dropping `-x` from grep failed with "the entry is the whole line, at level two"; `exit 1` at a missing-anchor report failed with "both missing entries and their files are reported". Real tree exits 0. `make gate` prints `gate: ok`. | Grok 4.6 |
| M3a/20-bxctl-chat-approvals | 2026-09-22 | done | 1 | pass | `AdminError` has no `Io` variant (task 18 maps write failures to `Protocol` via `From<io::Error>`), so `handle_pending` exit 8 reports every `cmd_approve`/`cmd_refuse` `Err` as `approval {id}: {e}` rather than returning `Err(AdminError::Io(e))`. Writes inside `handle_pending` itself still use `?`. | Moved `stream_turn` into `chat.rs` with `OnPending`/`Approvals`/`TurnIo`/`handle_pending`. The owner is shown `brokerd`'s list item, never the event's tool/args. Only the id typed in full (after stripping one trailing `\n` then one `\r`) approves; anything else refuses. `run` holds one `BufReader` on stdin for both modes. `Printer::event` escapes model text and tool names, prints the three fail-closed runbook lines as whole literals, and prints nothing for `ApprovalPending`. 21/12/20/9/12/8 tests five runs; `make gate` prints `gate: ok`. | Grok 4.6 |
@@ -349,12 +350,12 @@ refusing runner, the model reported the failure in its own words, and `bxctl aud
| 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 |
| 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 23 |
| 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 23 |
| 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 23 |
| 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 23 |
| 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 23 |
| 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 23 |
| 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 |
docs/plans/M3a/23-review-fixes.md
+31
View File
@@ -0,0 +1,31 @@
# Task 23: the M3a review fixes
Done by the design model directly (2026-09-22): Ornith was under heavy contention, and the fixes are
small. The findings are in `docs/implementer-log.md`, "M3a, tasks 01 to 22". Most fixes land with a
test that fails without it; the rest are checked by reading, as the last column says.
| Finding | Fix | Test |
|---|---|---|
| 1 | `Writer::open`: a log with no complete record chains from the last line of the file before it, or from zero; no indexing | `brokerd/tests/audit_edges.rs`: one zero-length log file; one with a torn record only |
| 2 | One definition of a log file name, `proto::is_audit_log_name`, used by `brokerd` and `bxctl` | `proto/tests/log_names.rs`; `brokerd/tests/audit_edges.rs`: `2026-0x-19.jsonl` is neither verified nor written |
| 3 | `brokerd`'s config, directory and socket failures end with `#brokerd-start-failed`; a lost listener or a refused thread with `#brokerd-listener-lost`; both entries are new | `brokerd/tests/serve_pointers.rs` (the listener path by reading) |
| 4 | `Writer` no longer unlinks `audit/.lock` | `brokerd/tests/audit_edges.rs` |
| 5 | No `unwrap` in `audit.rs`, no `as` in `grants.rs` | gate |
| 6 | `serve` starts threads with `thread::Builder`; a refused per-connection thread closes that connection and prints a line; an aborted connection no longer stops the daemon | by reading |
| 7 | Every role's `main` reads `args_os`. `brokerd` and `loopd` keep the config path as a path, so one that is not UTF-8 works; `bxctl` and `inferproxy` take text and answer such an argument with their usage | `brokerd/tests/serve_pointers.rs`, `loopd/tests/args_os.rs`, `bxctl/tests/args_os.rs`, `inferproxy/tests/args_os.rs` |
| 8 | `BrokerPort` waits at most 24 hours after a pending frame, whatever `expires` says | `loopd/tests/broker_port_cap.rs` |
| 9 | `bxctl`'s admin requests time out after 30 s | `bxctl/tests/admin_timeout.rs` |
| 10 | `bxctl` escapes `retrying` errors and every `error` detail | `bxctl/tests/escape_details.rs` |
| 11 | The `the requester went away` result is recorded at the time it happens | by reading |
| 12 | `bxctl`'s usage says what `audit verify` does | `bxctl/tests/cli.rs` (unchanged) |
| 13 | `AdminError::Io`, and a failed write is returned at once | by reading |
| 14 | Not in this task: `ToolArgs::HttpFetch`'s `url` and `host` become one sealed type in M3b's first task, where the runtime starts reading `url` | — |
| 15 | `verify.rs` shares the name rule (2); `MAX_PATH`'s doc; the unused push in `grants.rs`. `find_tool` and `additionalProperties` change the baseline and wait for the next epoch change | — |
Also, because the server changed: `crates/loopd/tests/device.rs` keeps the expected server in one
constant, `EXPECT`, now four slots over one 262,144-token pool, and `tools/check-m3a-device.sh`
matches it. They stay recorded expectations rather than values read from `/props`, which would
make the self-test's own check pass by definition.
Result: six code commits (`eed0a22` to `f6841f1`) and this record; every new test failed before its fix; `make gate`
ok; `make verify-device` 6 of 6 and `tools/check-m3a-device.sh` ok on straylight.