kyleandClaude Opus 5
e3f37da232
Task files, the files they copy in (byte-identical to the reference on m3a-ref), each area's check record, and a README with the per-task table of what each check exposed. The handoff note is done with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
101 lines
7.2 KiB
Markdown
101 lines
7.2 KiB
Markdown
# M3a checks, fork D: `bxctl` (tasks 18, 20) and the runbook gate script (task 21)
|
|
|
|
What each task's given tests were checked against before hand-over, and what doing so exposed.
|
|
Branch `m3a-ref-d`, three commits: `292780f` (the task 18 state), `5f27808` (the task 20 state,
|
|
skeleton), and the one after it (reference bodies, written afterwards as a measurement).
|
|
|
|
## What was checked how
|
|
|
|
| Task | Check | Ran green | Compiled only |
|
|
|---|---|---|---|
|
|
| 21 `check-runbook.sh` | Real script, run for real. Self-test passes; five mutations of the script (no `-x`, exit at the first missing anchor, no `-prune`, a narrower anchor pattern, no "no pointers" check) each make the self-test fail. | all | none |
|
|
| 18 `escape.rs` | Written for real: 12 lines of logic, cheaper than a stub. | `escape` 8 of 8 | none |
|
|
| 18 `cli.rs`, `main.rs` | Written for real. Not by choice: the 12 existing `chat` tests drive the binary, and the binary cannot parse its arguments through a `todo!()`. | `cli` 11 of 12; `chat` 12 of 12 | `cli` 1 (it needs `admin`) |
|
|
| 18 `admin.rs` | **Skeleton** (`todo!()` bodies; `reason_name` real, because the printer calls it). | `admin` 1 of 21 | `admin` 20 |
|
|
| 20 `Printer`, `stream_turn`, `main.rs` | Written for real, for the same reason as `cli.rs`. | `chat_print` 9 of 9; `chat_approvals` 3 of 20 | none |
|
|
| 20 `handle_pending` | **Skeleton.** | none | `chat_approvals` 17 |
|
|
|
|
So "skeleton" came to two things: the bodies of `admin.rs` and one function in `chat.rs`. 38 of
|
|
the 82 tests only compiled at hand-over quality. Everything was clippy-clean and formatted, and
|
|
the task 18 state (new command dispatch, M2b chat code) was built and tested on its own commit,
|
|
because `main.rs` is rewritten in both tasks.
|
|
|
|
## Measurement: what a reference found that the skeleton had not
|
|
|
|
After the skeleton state was committed, the missing bodies were written (about 150 lines) and the
|
|
38 compile-only tests were run: **all passed on the first run**, then six more runs under CPU
|
|
load with no failure. The reference exposed no defect that the skeleton and a desk-check had
|
|
missed. For this kind of task (formatting and a request/answer client, exact strings) the
|
|
skeleton was enough. The reference bodies are kept in the last commit because the straylight
|
|
check needs a working `bxctl approve`; they are not handed to the implementer.
|
|
|
|
## Defects the checks exposed
|
|
|
|
### Task 21
|
|
|
|
1. **Spec 11 does not say what a pointer is when its anchor is not written out.** A pointer built
|
|
with `format!("see docs/runbook.md#{anchor}")`, or the placeholder `docs/runbook.md#<anchor>`
|
|
in a comment, has no anchor the script can read, and a script that skipped these would let a
|
|
computed pointer through unchecked. The script fails on them. **This binds every crate:** under
|
|
`crates/`, in source and in tests, a pointer must be a literal, and no comment may hold
|
|
`docs/runbook.md#` followed by a placeholder. Run `sh scripts/check-runbook.sh` on the merged
|
|
reference tree before hand-over. Proposed wording for spec 11: "An anchor must be written out
|
|
in the source; a pointer whose anchor the script cannot read is an error."
|
|
2. **Spec 11 says "in `crates/`" without saying which files.** Chosen: `*.rs`, test files
|
|
included, `target/` excluded. `grep` on the binary frame fixture would otherwise need `-a`,
|
|
which is not POSIX.
|
|
3. **No pointer at all is a failure** (tip I2): otherwise a change to the message format would
|
|
turn the check off silently. Not in the spec; proposed: "It fails if it finds no pointer."
|
|
4. Not fixed, out of scope: `scripts/check-lines.sh` still exits at the first long file, inside a
|
|
pipeline subshell (M1 finding 8).
|
|
|
|
### Task 18
|
|
|
|
5. **Spec 9's example block contradicts spec 6.** The example shows
|
|
`{"command": "rm …", "cwd": "…"}` with spaces; section 6 says the arguments are `serde_json`'s
|
|
serialisation of the typed value, which has none. `bxctl` prints what `brokerd` sent, escaped,
|
|
and the tests use the compact form. The example should be compact.
|
|
6. **Spec 9 leaves the block's details open**; the tests now fix them: two spaces between parts;
|
|
spans are whole seconds under a minute, whole minutes under an hour, else whole hours, rounded
|
|
down; `expired` once `now >= expires`; a clock behind the broker's shows `0 s ago`; a session
|
|
id over 10 characters is its first 9 and `…`; an empty list prints `no pending approvals`;
|
|
`approve_result` with outcome `ask` prints `runs` (the re-decision lets an `ask` run); any
|
|
other error frame prints `bxctl: <code>: <detail>` on stderr and exits 1.
|
|
7. **`GrantProblem.problem` can be several lines** (`toml`'s errors are), which would break
|
|
`<file>:<line>: <problem>`. `bxctl` puts `file` and `problem` through `escape_json_text`, so
|
|
each problem is one line whatever arrives. Better fixed at the source too: task 06 should give
|
|
one-line problems.
|
|
8. **The error type had to change while the test was being written** (the T16 kind of defect):
|
|
a message that names the socket needs `AdminError::Connect(PathBuf, io::Error)`, not
|
|
`Connect(io::Error)`. Found by the skeleton.
|
|
9. **`"+41".parse::<u64>()` succeeds.** An id is now "ASCII digits only, then parse".
|
|
10. **Task order.** Task 18 creates `main.rs`'s `audit verify` arm, but `verify::run` is task
|
|
19's. Task 18 therefore creates `crates/bxctl/src/verify.rs` with the final signature and a
|
|
placeholder body, and adds all four `pub mod` lines. **Task 19 must say "Modify
|
|
`verify.rs`", not "Create", and must not touch `lib.rs` or `main.rs`.**
|
|
11. **A contract file changed:** `crates/bxctl/src/lib.rs` gained `pub mod cli;`, and
|
|
`chat::code_name` became `pub`.
|
|
|
|
### Task 20
|
|
|
|
12. **Spec 9, "`--say` and `--json` print the event only", is ambiguous**: without `--json`
|
|
there is no printed form of the event. Chosen, and tested: `--json` prints the event's JSON
|
|
line and never contacts `brokerd`; `--say` shows `brokerd`'s block and asks nothing, and the
|
|
owner answers with `bxctl approve` from another terminal. Proposed wording: "`--say` shows
|
|
the block and does not ask. `--json` prints the event as JSON and nothing else."
|
|
13. **Spec 9 escapes "reasoning and content" only.** The model also chooses tool names, which
|
|
were printed raw in three places, and the final answer on stdout was raw. All are escaped
|
|
now. Proposed wording: "everything the model wrote: reasoning, content, tool names, and the
|
|
answer on stdout."
|
|
14. **Exits the spec does not cover**, now defined: the end of the input at the question
|
|
refuses; `brokerd` unreachable, an approval that expired between the list and the answer,
|
|
and any other failed answer are each reported on one line and the turn goes on; a failed
|
|
write to the terminal is an error and nothing is answered.
|
|
15. **One reader for stdin.** Designing the binary-level test showed that the approval's answer
|
|
must come from the same `BufReader` as the chat lines: with a pipe, everything typed is in the
|
|
first reader's buffer, and a second reader sees the end of the input. `main.rs` holds one
|
|
reader for the whole run; `interactive_mode_reads_the_answer_from_the_same_input_as_the_chat`
|
|
fails otherwise.
|
|
16. `tests/admin.rs` is 497 lines and `tests/chat_approvals.rs` 487, against the limit of 500.
|
|
A test added later to either needs a new file.
|