mirror of
https://github.com/block/buzz.git
synced 2026-11-02 15:31:27 +00:00
559eb306ea
Four review findings on #6742. Each was reproduced with a failing test before the fix, and each fix was mutation-tested by reverting it and confirming the new test dies. 1. Freeze normalized action arguments (Codex, mod.rs:230). `BrokerRequest::new` and `prepare` both called `validate()`, which is `validated().map(|_| ())` — it computes the normalized copy and throws it away. So `AgentTarget::Name(" helper ")` passed validation and stayed padded in the frozen wire body: the host would look up an agent the validator never approved. `new` now stores the normalized action, and `prepare` normalizes before serializing. `prepare` deliberately still validates. It is not redundant: every field of `BrokerRequest` is public and the type is `Deserialize`, so a value can reach `prepare` without passing through `new` — struct literal, parsed JSON, or mutated after construction. Freezing bytes is the last checkable point, so both paths now satisfy one invariant: the frozen body holds exactly what validation approved. To avoid validating args twice on that path, the envelope checks are split into `validate_envelope` and `prepare` takes the normalized value from a single `validated` call. 2. Correlate every echoed identity, not just the pubkey (Codex mod.rs:856, extended per bugs-00's agents.create finding). `validate_for` compared only requestId, action, and read limit, so a response with a matching requestId but a different `agentPubkey` — or a different `channelId` on agents.create — validated, and a host routing bug could hand a caller an outcome for another subject. Generalized rather than patched per action: `correlate::correlate_identities` matches exhaustively over `ActionArgs`, so adding an action is a compile error instead of a silent default to "not compared". The full table of what is and is not compared, with the reason for each omission, is on `validate_for` where a host author reads it; the private fn points there so there is one copy to keep true. Extracted to `broker/correlate.rs` because the table pushed mod.rs to 1,019 lines, past the repo's 1,000-line ceiling. 3. Nested action enums ignored sibling members (bugs-00). `ActionArgs`/`ActionOutcome` are adjacently tagged and lacked `deny_unknown_fields`, so a sibling of their two keys — `secretKey` beside `args` — was silently dropped when either type was deserialized directly. Both are public and wire-facing, so that is a real door the envelope's own strictness does not cover. Closed at the type level rather than documented as a limitation: it turned out to be a one-attribute fix that breaks nothing. 4. Duplicate object keys were accepted inside `outcome` (bugs-00). Confirmed and fixed, but narrower than reported: serde's derived readers already reject repeated fields, so top-level members, flattened members, typed error payloads, and `args` all rejected duplicates. `outcome` did not, because `WireResponse` buffered it through `serde_json::Value`, which collapses duplicates last-wins. That made it the one place a reader could observe a value the envelope's strictness never vetted — so it is a real divergence, not a style question, and `outcome` now re-parses the original bytes via `RawValue`. This enables serde_json's `raw_value` feature, which adds no dependency and leaves Cargo.lock unchanged. Validation: 295 tests pass in buzz-sdk (291 before, plus four new), full `cargo test --workspace` green, workspace clippy `-D warnings` clean, fmt clean, `cargo doc` warning-free, every touched file under 1,000 lines. Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>