Record the second review of task 23 and the findings first left out
The review table gains findings 16 to 20, which the two review agents reported and the first write-up dropped. The independent review of the fix commits, and what was changed for it, is recorded; task 23's claims about its tests are corrected. The spec and decisions record the day-long cap, the ttl_ms bound, the socket-directory rule and the listener's retry. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+26
-1
@@ -6,7 +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/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 and 16 fixed. Tests that failed without their fix cover 1, 2, 3, 4, 7, 8, 9, 10 and 16; two more (a torn first record, an empty later file) passed before the fix and are regression guards; 6, 11 and 13 were 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 |
|
||||
@@ -358,6 +358,11 @@ refusing runner, the model reported the failure in its own words, and `bxctl aud
|
||||
| 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 |
|
||||
| 16 | low | spec | `serve` set 0700 on whatever directory held a socket, following a symbolic link (the link's target changed) and falling back to `/` for a socket with no parent. Omitted from this table when first written. | Task 23 (`ba369f8`) |
|
||||
| 17 | low | implementer (17) | `BrokerPort`'s deadline does not cover `connect` or the request write; a peer that accepts a few bytes at a time can stretch the send. The request is small and fits a socket buffer. Omitted when first written. | Open; M3b |
|
||||
| 18 | nit | plan (17) | A pending frame marked `final` returns the generic "the tool broker is unavailable", so the spec's "the tool broker gave no final answer" cannot be reached over the wire; task 17 said to do it this way. Omitted when first written. | Open; spec to match |
|
||||
| 19 | low | tests | `--accept-break` on a real break was tested only at library level, and `serve`'s directory and socket failures had no tests. Omitted when first written. | Task 23 (`cfa0247`, `serve_pointers.rs`); the listener-lost path is still untested |
|
||||
| 20 | nit | spec | A relative or empty `home` gives paths relative to the working directory; an `approvals` insert with an id already present drops the earlier waiter. Neither can happen today (ids are audit `seq`s; the deployed config is absolute). Omitted when first written. | Open |
|
||||
|
||||
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
|
||||
@@ -382,3 +387,23 @@ Ornith on straylight now runs with `parallel = 4` over one unified 262,144-token
|
||||
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.
|
||||
|
||||
### M3a, task 23 — reviewed 2026-09-22 by a separate agent, then by the design model
|
||||
|
||||
The design model wrote the spec, the reference, the review and these fixes, so the six fix commits
|
||||
(`eed0a22` to `f6841f1`) were given to an agent that saw only the code, the spec and the finding
|
||||
descriptions. It found no serious new defect, confirmed the audit resume logic and the lock change,
|
||||
and ran every crate's tests. It found five problems, all fixed in `e08deb3` and `bb4d7c0`:
|
||||
|
||||
| # | Severity | Finding | Fix |
|
||||
|---|---|---|---|
|
||||
| R1 | low | `ChatError::Frame` and `AdminError::Frame` printed serde's message, which quotes a bad frame's text after decoding: escape sequences from a compromised peer reached the terminal. Confirmed by running. | Escaped |
|
||||
| R2 | low | Pre-existing: a torn last line followed by an empty later file had its recovery written into the later file, breaking the chain for good. Confirmed by running. | The line is ended in its own file |
|
||||
| R3 | low | The shared name rule accepted month 13 and day 99. | Real months and days only |
|
||||
| R4 | low | The 24-hour cap departed from spec section 8 without a record, and a `ttl_ms` over a day would have been given up by `loopd` while listed. | Spec amended; `brokerd` refuses `ttl_ms` over a day |
|
||||
| R5 | nit | The `MAX_PATH` doc edit added a line instead of replacing one. | Fixed |
|
||||
|
||||
It also showed that "an aborted connection no longer stops the daemon" meant nothing on Linux: the
|
||||
errors skipped there do not occur, and running out of file descriptors still stopped `brokerd`. It
|
||||
now pauses and retries instead. A timed-out admin request now says whether `brokerd` acted is
|
||||
unknown. And two of task 23's tests passed before their fix; the record above says so.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user