Review M3b: accept with follow-ups; add the Nix expression for the image
On straylight, with real containers from deploy/tools-image.nix, every claim held: no network without a grant, the limits, the file tools, http_fetch's host checks including a redirect and a tailnet name, and no leftovers. Two plan defects found there (curl globbing, podman pulling a missing image), five lower findings. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -431,3 +431,53 @@ task 01's stopped row carried the notes of M2b task 11. Each is moved back or re
|
||||
bracketed mark, and pipes inside code are escaped so every row has its eight cells.
|
||||
| M3b/03-brokerd-grant-mount-rule | 2026-09-22 | done | 1 | pass | none | Copied `tests/grants_mount.rs` from the plan's `files/`. Added a third arm to the path loop in `check_grant` (grants.rs:263), an `else if path.contains([':', ','])` checked only when the first two arms did not apply, reporting `"{:?} cannot be mounted: it contains ':' or ','"`. The `else if` chain means a path already reported as invalid is not reported twice. `cargo fmt --all` kept the `push` multi-line (the single-line form in the task exceeds 100 columns); the wording matches the task verbatim. Both suites pass (17 grants, 2 mount); `make gate` prints `gate: ok` on the first run. | ? |
|
||||
|
||||
### M3b, tasks 01 to 13 — reviewed 2026-09-23 by the design model (Claude)
|
||||
|
||||
Accepted, with follow-ups. All work by Ornith-1.5-35B-A3B through `tools/run-plan.sh`. The code
|
||||
does what the spec says, and on straylight, with real containers from the Nix-built image, every
|
||||
claim of the milestone held.
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| 13 task commits, each with the trailer; 6 plan commits by the design model during the run | pass |
|
||||
| All 28 given test and fixture files identical to the plan | pass (`container.rs` differs from its skeleton, as intended) |
|
||||
| `make gate` on Talos | `gate: ok`, 638 tests, the same count as the reference |
|
||||
| `toolkit` and `brokerd` suites ten times in a row | no failure |
|
||||
| Banned constructs in new library code | `thread::spawn` four times, `as u64` on two constants (findings 3 and 7) |
|
||||
| Independent review by a separate agent, given only the code, the spec and the plan | no serious defect; its points are below |
|
||||
|
||||
**On straylight** (2026-09-23; image `localhost/boxmaker-tools@sha256:04459bec…`, 16 MB, built by
|
||||
`deploy/tools-image.nix`; `brokerd` from this branch with `[runner]`):
|
||||
|
||||
| Claim | Seen |
|
||||
|---|---|
|
||||
| A granted file is read; a symlink in the granted directory to `~/.ssh/id_ed25519` is not | `hello from the notes`; `read_file: …/notes/key: no such file` |
|
||||
| `write_file` writes as the owner | the file is owned by uid 1000 |
|
||||
| No network without a grant | from `shell`: `100.100.100.100` unreachable, `1.1.1.1` unreachable, no DNS, only `lo` |
|
||||
| Hardening | no capabilities, read-only root, writable `/tmp` |
|
||||
| Limits | 8 s limit stopped `sleep 60` at 8 s; 64 processes stopped a fork loop; 256m killed a memory hog |
|
||||
| `http_fetch` reaches only allowed hosts | `example.com` 200; redirect `google.com` → `www.google.com` refused at the proxy (reply 2); an allowed name resolving to the tailnet (`100.88.197.9`) refused (reply 4); a host with no grant denied before any container |
|
||||
| No container outlives its call | `podman ps -a --filter label=boxmaker` empty after every call; egress directories removed |
|
||||
|
||||
| # | Severity | Owner | Finding | Fix |
|
||||
|---|---|---|---|---|
|
||||
| 1 | medium | plan (task 06, spec 6) | `curl` expands globs in the URL: `https://example.com/[1-3]` made three requests (seen on straylight), so `[1-99999999]` would hammer an allowed host and buffer every body. The fixed argument list lacks `--globoff`. | Follow-up |
|
||||
| 2 | low | plan (task 10, spec 6) | `podman run` has no `--pull=never`: with an image that is not loaded, Podman tries to pull it (seen on straylight). Here the name starts `localhost/`, so the pull fails, but a pull is unlisted egress and the call should fail at once. | Follow-up |
|
||||
| 3 | low | implementer (11), plan | `std::thread::spawn` in `container.rs` (three) and `toolkit/src/fetch.rs` panics if a thread cannot be made; after the spawn of the container, a panic drops the `Child` without `podman kill`, so the container runs on without its limit. The reference had the same; the task did not say. | Follow-up |
|
||||
| 4 | low | implementer (11) | The time limit bounds the wait, not the joins after it: if another process held the pipes, `run` would block until it let go (shown with a fake `podman` without `exec`: 6 s for a 0.3 s limit). Real Podman released them at the kill (8 s limit, 8 s seen). | Follow-up: join with a grace deadline |
|
||||
| 5 | low | implementer (11) | Podman's standard error, which the tool can write to, goes into `brokerd`'s log unescaped, so a tool can forge log lines (a fake runbook pointer). It never reaches a `RunError`. | Follow-up: escape |
|
||||
| 6 | low | spec (section 5) | `is_public` passes local-use NAT64 `64:ff9b:1::/48` and 6to4 `2002::/16` with a private IPv4 inside. Neither is in use on straylight. A host with its own public address would be reachable by an allowed name that points at it; straylight has none (its addresses are LAN, tailnet and Tailscale's ULA, all refused). | Spec, when next touched |
|
||||
| 7 | nit | implementer | The runtime notice lacks its `brokerd:` prefix; two config messages use `{}` where the task gave `{:?}`; `chunk[..take]` where the skeleton said `get`; `egress-proxy` accepts trailing arguments; `as u64` on two constants. | When next touched |
|
||||
|
||||
What was good: the proxy handshake reads exactly what the protocol gives against one deadline, has
|
||||
no panic path, and tries only public addresses (the independent review probed it and found it
|
||||
sound); the egress guard is created before anything can fail and cleans up on every path, a panic
|
||||
included; nothing from the model reaches `podman`'s command line.
|
||||
|
||||
**The run.** 11 of 13 tasks committed on the first attempt. Task 04 committed but left `Cargo.lock`
|
||||
out (the plan's `git add` line). Task 08 needed two attempts (the first did not skip `argv[0]` and
|
||||
joined each handler thread). Task 11 needed four: the task could not be written as given (a field
|
||||
nothing read, and `#[allow]` forbidden), then two sessions ran out of room planning the whole file
|
||||
in one turn; a skeleton, and then a finer one with `run` as glue over small helpers, got it done.
|
||||
Every stop was a task-writing problem or a turn-size problem, not a wrong implementation.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user