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

Review M1: accept with two follow-up tasks

Browse Source 
The branch passes the checklist: seven commits, copied files unchanged,
gate and audit green. Reading and probing found that AuditRecord and
ToolCall accept unknown fields, that large Timestamps panic when
formatted, and that the dependency-direction scripts miss table-form
dependencies and pass when their inputs are missing. The last two
families were gaps in the tasks, not only in the code.

Tasks 08 and 09 carry the fixes, defined by an exhaustive unknown-field
test, a bounded-Timestamp test and an extended gate-script self-test.
All three were checked against the reference implementation and fail on
the current branch.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
kyle
2026-09-17 09:14:29 -07:00
co-authored by Claude Fable 5.1
parent 8c1852ec47
commit dcdcf65d83
9 changed files with 388 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans/M1/08-proto-strictness.md
+86
View File
@@ -0,0 +1,86 @@
# M1 task 08: close two gaps in `proto` (review follow-up)
**Branch:** `m1` (run `git switch m1`; `git status --short` must be empty, otherwise stop)
**Commit subject:** `Reject unknown fields in every proto struct and bound Timestamp`
## Goal
The M1 review found two defects in `proto`. Fix both. Each is defined by a test file that you copy
in and must not edit.
1. `AuditRecord` and `ToolCall` accept unknown JSON fields. The rule is that unknown fields are
rejected everywhere. The reviewer's probe showed an audit line with an extra `"forged":true`
field decoding without error.
2. `Timestamp::from_unix_millis` accepts any `u64`, but `to_rfc3339` can only spell times up to
the year 9999. For a larger value, `humantime`'s formatter returns an error, `to_string()`
panics, and so does serializing the value. Library code must never panic. This one was a
mistake in the task you were given, not in your code.
## Files
- Copy (replacing the old one): `crates/proto/tests/ids.rs`
- Copy (new): `crates/proto/tests/strict.rs`
- Modify: `crates/proto/src/ids.rs`, `crates/proto/src/audit.rs`, `crates/proto/src/log.rs`,
`docs/implementer-log.md`
- Check, and modify only if the test says so: every other file in `crates/proto/src/`
## Interfaces
Changes to `Timestamp` in `crates/proto/src/ids.rs`. Everything else keeps its signature.
```rust
impl Timestamp {
/// 9999-12-31T23:59:59.999Z, the last instant RFC 3339 can spell with a four-digit year.
pub const MAX: Timestamp; // 253_402_300_799_999 ms
// Was: -> Self. Now fails with ValueError::Timestamp when ms > MAX.
pub fn from_unix_millis(ms: u64) -> Result<Self, ValueError>;
}
```
Rules:
- No `Timestamp` value above `MAX` can exist. `from_unix_millis` and `parse` reject such values,
and `now()` never produces one (clamp it to `MAX`; a clock that far off is not worth an error).
- With that guarantee `to_rfc3339` cannot fail. Keep it returning `String`.
- `strict.rs` takes every fixture, adds one unknown key to one JSON object at a time, at every
depth, and requires decoding to fail. If it reports a type other than `AuditRecord` or
`ToolCall`, fix that type too and say so in your log row.
## Steps
- [ ] **1. Copy the tests.**
```sh
git switch m1
cp docs/plans/M1/files/crates/proto/tests/ids.rs docs/plans/M1/files/crates/proto/tests/strict.rs crates/proto/tests/
```
- [ ] **2. See them fail.** `cargo test -p proto --test strict`. Expected: 2 of 4 tests fail with
`accepted an unknown key`. `cargo test -p proto --test ids`. Expected: it does not compile,
because `Timestamp::MAX` does not exist and `from_unix_millis` does not return a `Result`.
- [ ] **3. Fix the code.**
- [ ] **4. See them pass.** `cargo test -p proto`. Expected: `ids` 12 passed, `strict` 4 passed,
and 45 passed in total across the six test files.
- [ ] **5. Format, then run the gate.** `cargo fmt --all`, then `make gate`. Expected last line:
`gate: ok`.
- [ ] **6. Log and commit.**
```sh
git add crates/proto docs/implementer-log.md
git commit
```
## Done when
- `cargo test -p proto` reports 45 passed.
- `make gate` prints `gate: ok`.
- `diff -r crates/proto/tests docs/plans/M1/files/crates/proto/tests` prints nothing.
## Stop and report if
- `strict.rs` fails for a type after you have added `deny_unknown_fields` to it.
docs/plans/M1/09-gate-scripts-table-form.md
+93
View File
@@ -0,0 +1,93 @@
# M1 task 09: make the gate scripts fail closed (review follow-up)
**Branch:** `m1` (run `git switch m1`; `git status --short` must be empty, otherwise stop)
**Commit subject:** `Make gate scripts handle table-form dependencies and fail closed`
## Goal
The M1 review found that the dependency checks can be bypassed by accident, and that all three
scripts pass when they cannot find what they are supposed to check. Fix the scripts so the
extended self-test passes. The self-test is copied in and must not be edited.
The first problem was a gap in the task you were given: it did not mention that TOML lets a
dependency be written as its own table. The reviewer's probe added this to `crates/loopd/Cargo.toml`
(with `brokerd` also listed in `[workspace.dependencies]`), and both scripts passed although
`loopd` then depended on another role:
```toml
[dependencies.brokerd]
workspace = true
```
## Files
- Copy (replacing the old one): `scripts/test-gate-scripts.sh`
- Modify: `scripts/check-lines.sh`, `scripts/check-crate-deps.sh`, `scripts/check-dep-docs.sh`,
`docs/implementer-log.md`
## Required behaviour
Everything from task 01 still holds. In addition:
1. **A dependency's name can appear in three places.** All three count, in any section whose name
ends in `dependencies` (`[dependencies]`, `[dev-dependencies]`, `[build-dependencies]`,
`[target.'cfg(unix)'.dependencies]`):
- a plain key: `brokerd = { path = "../brokerd" }` or `brokerd = "1"`
- a dotted key: `brokerd.workspace = true` or `brokerd.path = "../brokerd"`
- a table header whose last part is the name: `[dependencies.brokerd]` or
`[dev-dependencies.brokerd]`. The lines inside that table (`path = …`, `workspace = true`,
`features = […]`) are settings of that dependency, not dependencies themselves.
2. **`check-crate-deps.sh`** applies its rule to names found in all three places.
3. **`check-dep-docs.sh`**, second rule: a plain or dotted dependency must have `workspace = true`
on its line, as before. A table-form dependency must have a `workspace = true` line inside its
table. `[dependencies.serde]` followed by `workspace = true` and `features = ["derive"]` is
fine; `[dependencies.rand]` followed by `version = "0.9"` is an error.
4. **Fail closed.** Each script exits 1 with a message if `ROOT/crates` does not exist.
`check-dep-docs.sh` also exits 1 if `ROOT/Cargo.toml` or `ROOT/docs/dependencies.md` does not
exist. Do not hide errors from `find`, `awk` or `grep` with `2>/dev/null`: if a tool fails, the
script must not report success.
5. `check-lines.sh` prints the line count of each file that is too long, for example
`check-lines: crates/x/src/a.rs has 612 lines (limit 500)`.
Still POSIX `sh` only: no bash features, no Python, no `jq`.
## Steps
- [ ] **1. Copy the self-test.**
```sh
git switch m1
cp docs/plans/M1/files/scripts/test-gate-scripts.sh scripts/
```
- [ ] **2. See it fail.** `sh scripts/test-gate-scripts.sh`. Expected: 7 failures, among them
`role depends on another role, written as a table` and
`check-lines without a crates directory`.
- [ ] **3. Fix the three scripts.** Read the new cases in `scripts/test-gate-scripts.sh` first; each
one has a comment or a name that says what it checks.
- [ ] **4. See it pass.** `sh scripts/test-gate-scripts.sh`. Expected: `test-gate-scripts: ok`.
Then run each script on the real tree: `sh scripts/check-lines.sh`,
`sh scripts/check-crate-deps.sh`, `sh scripts/check-dep-docs.sh`. Expected: no output, exit 0.
- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`.
- [ ] **6. Log and commit.**
```sh
git add scripts docs/implementer-log.md
git commit
```
## Done when
- `sh scripts/test-gate-scripts.sh` prints `test-gate-scripts: ok`.
- `make gate` prints `gate: ok`.
- `cmp scripts/test-gate-scripts.sh docs/plans/M1/files/scripts/test-gate-scripts.sh` prints
nothing.
- `grep -n '2>/dev/null' scripts/check-*.sh` prints nothing.
## Stop and report if
- A case in the self-test contradicts the rules above.
docs/plans/M1/README.md
+5 -2
View File
@@ -1,7 +1,7 @@
# M1 implementation plan: workspace, `proto`, gate
> **For the implementing model:** do not work from this file. The owner gives you one task file at
> a time (`01-…` to `07-…`). This file is the index for the owner and the reviewer.
> a time (`01-…` to `09-…`). This file is the index for the owner and the reviewer.
**Goal:** A Cargo workspace whose gate passes, with the real shared types in `proto` and a
`Decision` type in `brokerd` that code outside its policy module cannot construct.
@@ -24,7 +24,8 @@ and verified against a private reference implementation; the implementer writes
`docs/dependencies.md`. Crates are `publish = false`.
- Unknown fields are rejected everywhere. Field order is the wire format.
- `make gate` runs offline and must print `gate: ok` at the end of every task.
- Branch `m1`. One task, one fresh OpenCode session, one commit. Review happens once, after task 07.
- Branch `m1`. One task, one fresh OpenCode session, one commit. Review happened once, after task
07; tasks 08 and 09 are its follow-ups.
## Tasks
@@ -37,6 +38,8 @@ and verified against a private reference implementation; the implementer writes
| 05 | `05-proto-grant.md` | `Grant`, `Mode`, `Constraints` | `tests/grant.rs`, `fixtures/grant/` |
| 06 | `06-proto-records.md` | `DecisionRecord`, `AuditRecord`, `ToolCall`, `LogRecord` | `tests/records.rs`, `fixtures/records/` |
| 07 | `07-brokerd-decision.md` | `brokerd::policy::Decision`, `decide`, `brokerd::runner::run` | doctests and unit tests in `policy.rs` |
| 08 | `08-proto-strictness.md` | Review follow-up: unknown fields rejected in every struct; `Timestamp` bounded at year 9999 | `tests/strict.rs`, updated `tests/ids.rs` |
| 09 | `09-gate-scripts-table-form.md` | Review follow-up: gate scripts see table-form and dotted dependencies, and fail closed | updated `scripts/test-gate-scripts.sh` |
`files/` holds everything the tasks copy into place. Tests for a later task do not compile until
that task's types exist, which is why they are copied task by task and not all at once.
docs/plans/M1/files/crates/proto/tests/ids.rs
+28 -3
View File
@@ -100,7 +100,7 @@ fn hash32_rejects_wrong_length_uppercase_and_non_hex() {
#[test]
fn timestamp_has_one_spelling() {
let t = Timestamp::from_unix_millis(1_789_632_300_000);
let t = Timestamp::from_unix_millis(1_789_632_300_000).unwrap();
assert_eq!(t.unix_millis(), 1_789_632_300_000);
assert_eq!(t.to_rfc3339(), "2026-09-17T08:05:00.000Z");
assert_eq!(Timestamp::parse("2026-09-17T08:05:00.000Z").unwrap(), t);
@@ -109,11 +109,13 @@ fn timestamp_has_one_spelling() {
r#""2026-09-17T08:05:00.000Z""#
);
assert_eq!(
Timestamp::from_unix_millis(0).to_rfc3339(),
Timestamp::from_unix_millis(0).unwrap().to_rfc3339(),
"1970-01-01T00:00:00.000Z"
);
assert_eq!(
Timestamp::from_unix_millis(1_789_632_300_007).to_rfc3339(),
Timestamp::from_unix_millis(1_789_632_300_007)
.unwrap()
.to_rfc3339(),
"2026-09-17T08:05:00.007Z"
);
}
@@ -141,6 +143,29 @@ fn timestamp_rejects_other_spellings() {
assert!(serde_json::from_str::<Timestamp>("1789632300000").is_err());
}
#[test]
fn timestamp_range_ends_with_year_9999() {
assert_eq!(Timestamp::MAX.unix_millis(), 253_402_300_799_999);
assert_eq!(Timestamp::MAX.to_rfc3339(), "9999-12-31T23:59:59.999Z");
assert_eq!(
Timestamp::parse("9999-12-31T23:59:59.999Z").unwrap(),
Timestamp::MAX
);
assert_eq!(
Timestamp::from_unix_millis(253_402_300_799_999).unwrap(),
Timestamp::MAX
);
// One millisecond later has no RFC 3339 spelling, so it must not become a Timestamp at all.
assert_eq!(
Timestamp::from_unix_millis(253_402_300_800_000),
Err(ValueError::Timestamp)
);
assert_eq!(
Timestamp::from_unix_millis(u64::MAX),
Err(ValueError::Timestamp)
);
}
#[test]
fn timestamp_now_is_after_2026() {
assert!(Timestamp::now() > Timestamp::parse("2026-01-01T00:00:00.000Z").unwrap());
docs/plans/M1/files/crates/proto/tests/strict.rs
+109
View File
@@ -0,0 +1,109 @@
//! Every JSON object in every fixture must reject an unknown key. Do not edit.
//!
//! The other test files check unknown fields in a few hand-picked places. This one checks all of
//! them: it walks each fixture, adds one unknown key to one object at a time, at every depth, and
//! requires that the result no longer decodes.
use proto::{AuditRecord, Envelope, Grant, LogRecord};
use serde::de::DeserializeOwned;
use serde_json::Value;
/// Every copy of `value` that has exactly one extra key in exactly one object.
fn with_one_unknown_key(value: &Value) -> Vec<Value> {
let mut out = Vec::new();
match value {
Value::Object(map) => {
let mut extended = map.clone();
extended.insert("zz_unknown".to_string(), Value::Bool(true));
out.push(Value::Object(extended));
for (key, child) in map {
for changed in with_one_unknown_key(child) {
let mut copy = map.clone();
copy.insert(key.clone(), changed);
out.push(Value::Object(copy));
}
}
}
Value::Array(items) => {
for (i, child) in items.iter().enumerate() {
for changed in with_one_unknown_key(child) {
let mut copy = items.clone();
copy[i] = changed;
out.push(Value::Array(copy));
}
}
}
_ => {}
}
out
}
/// Returns how many variations were tried, so callers can check the walk reached nested objects.
fn check<T: DeserializeOwned>(what: &str, text: &str) -> usize {
let value: Value = serde_json::from_str(text).unwrap_or_else(|e| panic!("{what}: {e}"));
assert!(
serde_json::from_value::<T>(value.clone()).is_ok(),
"{what}: fixture must decode"
);
let variations = with_one_unknown_key(&value);
for changed in &variations {
assert!(
serde_json::from_value::<T>(changed.clone()).is_err(),
"{what}: accepted an unknown key: {changed}"
);
}
variations.len()
}
fn fixture(path: &str) -> String {
let full = format!("{}/tests/fixtures/{path}", env!("CARGO_MANIFEST_DIR"));
std::fs::read_to_string(&full).unwrap_or_else(|e| panic!("{full}: {e}"))
}
#[test]
fn envelopes_reject_unknown_keys_at_every_depth() {
for name in [
"tool_request.json",
"tool_response_pending.json",
"tool_response_result.json",
"tool_response_failed.json",
"tool_response_denied.json",
"error.json",
] {
// Envelope, msg and body: three objects.
assert_eq!(
check::<Envelope>(name, &fixture(&format!("wire/{name}"))),
3,
"{name}"
);
}
}
#[test]
fn audit_records_reject_unknown_keys_at_every_depth() {
for (i, line) in fixture("records/audit.jsonl").lines().enumerate() {
// The record and its decision: two objects.
assert_eq!(
check::<AuditRecord>(&format!("audit.jsonl:{}", i + 1), line),
2
);
}
}
#[test]
fn log_records_reject_unknown_keys_at_every_depth() {
let mut tried = 0;
for (i, line) in fixture("records/session.jsonl").lines().enumerate() {
tried += check::<LogRecord>(&format!("session.jsonl:{}", i + 1), line);
}
// Seven records, plus the one tool call inside the first assistant record.
assert_eq!(tried, 8);
}
#[test]
fn grants_reject_unknown_keys_at_every_depth() {
let grant: Grant = toml::from_str(&fixture("grant/full.toml")).unwrap();
let text = serde_json::to_string(&grant).unwrap();
// The grant and its constraints: two objects.
assert_eq!(check::<Grant>("full.toml as JSON", &text), 2);
}
docs/plans/M1/files/scripts/test-gate-scripts.sh
+21
View File
@@ -63,6 +63,15 @@ tree "$tmp/c-dev"; printf '\n[dev-dependencies]\nbrokerd.workspace = true\n' >>
expect fail "role dev-depends on another role" check-crate-deps.sh "$tmp/c-dev"
tree "$tmp/c-proto"; manifest "$tmp/c-proto/crates/proto" proto 'loopd.workspace = true'
expect fail "proto depends on a role" check-crate-deps.sh "$tmp/c-proto"
# A dependency can also be written as its own table: [dependencies.NAME]
tree "$tmp/c-sect"; printf '\n[dependencies.brokerd]\npath = "../brokerd"\n' >> "$tmp/c-sect/crates/loopd/Cargo.toml"
expect fail "role depends on another role, written as a table" check-crate-deps.sh "$tmp/c-sect"
tree "$tmp/c-sectw"; printf '\n[dev-dependencies.brokerd]\nworkspace = true\n' >> "$tmp/c-sectw/crates/loopd/Cargo.toml"
expect fail "role dev-depends on another role, written as a table" check-crate-deps.sh "$tmp/c-sectw"
tree "$tmp/c-dot"; manifest "$tmp/c-dot/crates/loopd" loopd 'proto.workspace = true' 'brokerd.path = "../brokerd"'
expect fail "role depends on another role, written with a dotted key" check-crate-deps.sh "$tmp/c-dot"
tree "$tmp/c-sectok"; manifest "$tmp/c-sectok/crates/loopd" loopd; printf '\n[dependencies.proto]\nworkspace = true\n' >> "$tmp/c-sectok/crates/loopd/Cargo.toml"
expect pass "role depends on proto, written as a table" check-crate-deps.sh "$tmp/c-sectok"
# check-dep-docs.sh
tree "$tmp/d-ok"
@@ -73,6 +82,18 @@ tree "$tmp/d-prose"; printf 'rand = "0.9"\n' >> "$tmp/d-prose/Cargo.toml"; print
expect fail "a mention in prose is not a table row" check-dep-docs.sh "$tmp/d-prose"
tree "$tmp/d-loose"; manifest "$tmp/d-loose/crates/loopd" loopd 'proto.workspace = true' 'rand = "0.9"'
expect fail "crate declares a dependency outside the workspace table" check-dep-docs.sh "$tmp/d-loose"
tree "$tmp/d-sect"; printf '\n[dependencies.rand]\nversion = "0.9"\n' >> "$tmp/d-sect/crates/loopd/Cargo.toml"
expect fail "table-form dependency without workspace = true" check-dep-docs.sh "$tmp/d-sect"
tree "$tmp/d-sectok"; printf '\n[dependencies.serde]\nworkspace = true\nfeatures = ["derive"]\n' >> "$tmp/d-sectok/crates/loopd/Cargo.toml"
expect pass "table-form dependency with workspace = true" check-dep-docs.sh "$tmp/d-sectok"
# A check that cannot find what it checks must fail, not pass.
mkdir -p "$tmp/empty"
expect fail "check-lines without a crates directory" check-lines.sh "$tmp/empty"
expect fail "check-crate-deps without a crates directory" check-crate-deps.sh "$tmp/empty"
expect fail "check-dep-docs without a crates directory" check-dep-docs.sh "$tmp/empty"
tree "$tmp/d-nodoc"; rm "$tmp/d-nodoc/docs/dependencies.md"
expect fail "check-dep-docs without docs/dependencies.md" check-dep-docs.sh "$tmp/d-nodoc"
if [ "$fails" -ne 0 ]; then
echo "test-gate-scripts: $fails failure(s)" >&2