test(buzz-sdk): split broker/tests.rs into schema/validation/client modules - #6759
Draft
baxen wants to merge 9 commits into
Draft
test(buzz-sdk): split broker/tests.rs into schema/validation/client modules#6759baxen wants to merge 9 commits into
baxen wants to merge 9 commits into
Conversation
An agent that holds no secret key still has to work: read its channels, answer a mention, react, keep its profile, address its memory, and create the agents it needs. This is the contract for asking a host to do those things — the interface half of #6467, with no host, no transport, and no signing. The open question in that RFC is per-operation versus a single "sign this" primitive. This answers per-operation. A sign-anything primitive tells the broker who is asking but not what for, so the only policies it can express are "all" and "none" — it is a signing oracle with a nicer name. Named operations let a host serve channel.read while refusing agents.create, and give a later policy or IFC layer a per-operation surface to attach to. The cost is that a new operation needs a new variant, which is the point: adding one becomes a reviewable change to the contract instead of another use of an existing blank cheque. For the same reason signing, publishing, and NIP-42/44/98 are not actions. They are mechanisms internal to whoever holds the key, and this contract never names them. Identity is a pubkey and nothing else. `PubkeyHex` has no counterpart here for the corresponding secret, every args type is `deny_unknown_fields`, and no outcome can structurally hold key material — agents.create returns a pubkey and a handle, never the minted nsec. A test scans the module's own source for secret-key tokens, because a deserialization test can only reject fields someone thought to add, while the scanner fails the moment such a type appears in a signature at all. Both halves were checked against deliberate mutations rather than assumed. The envelope carries no requester, owner, or scope. Those come from the session credential the host authenticated, so authority is never something a body can assert: a request that could name its own subject would let any caller act as anyone. Ownership of a created agent follows the same rule — the owner is the requester, implicitly, which is what makes an agent able to own the agents it creates while the chain still ends at a human. Bounding that chain's depth needs resources and policy this contract cannot see, so it stays a host concern. Reads are actions too, because #6467 asks for a deployment where the host is the only route to the relay. One `channel.read` covers channel, thread, and mention-feed scopes: they differ by filter, not by permission, and three names for one decision would be three policy knobs pretending to be independent. Its outcome is a projection rather than a signed event, since a caller with no relay cannot verify signatures anyway — trust here is trust in the host, stated rather than implied. `presence.set` and `typing.set` are left out of v1. They are housekeeping a host may decline regardless, and a closed enum makes adding them purely additive. Carried over from #6543: the envelope, requestId validation, the three-way Succeeded/Failed/Indeterminate result, the error codes, the retry-is-identical- bytes contract, and the agents.* argument shapes. Changed deliberately: pubkeys are a validated `PubkeyHex` instead of `String`, `capability` is renamed `action`, `CapabilityFailed` becomes `ActionFailed`, and `Unsupported` is new so a host can decline a best-effort operation without it reading as a fault. #6543 also lands in this module path; whichever merges second needs a deliberate reconcile, which I own. Addresses the interface half of #6467. 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>
Five contract changes from the quality review, each closing a gap where the
contract documented a promise it did not structurally keep.
Retry means identical bytes, so the client trait can no longer take a typed
value it would have to reserialize. `BrokerRequest::prepare` validates and
serializes once into a `PreparedRequest`; `BrokerClient::execute` takes that,
and every attempt sends the same body. Two serializations of one value may
differ across serde versions, which would have surfaced as a spurious
requestId conflict on retry.
The read cursor is now an opaque host-issued string, not inclusive Unix
seconds. Second-precision paging can loop or skip when more events than
`limit` share a second, and a timestamp cursor would have committed every
future host to one ordering strategy. Cursors are validated for shape only
(bounded, printable ASCII) and never parsed.
Reads return signed Nostr events. `BrokerMessage` is a transparent newtype
over `nostr::Event` with a local `verify()`; the previous projection gave up
authorship provenance for nothing, since Schnorr verification needs no relay.
Ancestry and mentions are derived from the signed tags rather than stored
beside them, so no field can disagree with the signature.
Response validation is request-aware. `validate_for(&PreparedRequest)`
correlates requestId and rejects an outcome whose action is not the request's,
so a caller matching on the outcome enum cannot silently take the wrong
branch. `ActionOutcome::validate` checks the identifiers and cursors an
outcome asserts, and `OutcomeUnknown` paired with `Failed` is now rejected as
self-contradictory.
Auth is a host verdict, not transport. `CredentialRejected` is gone;
`Unauthenticated` arrives inside `Failed`, which carries the promise that
nothing ran. Transport errors are reserved for the absence of a usable
envelope (`NoEnvelope { status, detail }`), and clients are told to parse an
envelope regardless of HTTP status.
The no-secret invariant no longer rests on a source scanner, which aliases and
neutral names bypass and legitimate names trip. It is now an exact wire
key-set table over all nine args and nine outcome types plus
`deny_unknown_fields`, so no field can be added without a reviewer changing
that table. Fixtures populate every optional field, since the table can only
pin what serializes.
Density: fluent builders and per-outcome validators removed, repeated
rationale reduced to one canonical place with cross-references, tautological
tests dropped or merged.
buzz-sdk: 283 tests pass. Workspace clippy, fmt, and doc clean.
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>
Four follow-ups from the delta review, all about closing the gap between what
the contract says and what it structurally allows.
`PreparedRequest::request()` is gone. It handed back the typed value whose
reserialization freezing the bytes exists to prevent, so an implementation
could have sent a second rendering of the same request and drawn a spurious
requestId conflict on retry. A transport needs correlation metadata, not the
value: `request_id()` and `action()` remain.
Response correlation is no longer advisory. `BrokerClient` is now the transport
primitive — `send` takes frozen bytes and returns an envelope unjudged — and
the blanket, unoverridable `BrokerClientExt::execute` is what callers use. It
runs `validate_for` and yields a `ValidatedResponse`, the only response type a
caller can obtain, so a mismatched envelope cannot arrive as `Ok` because an
implementation forgot a step. `send` additionally takes a `Dispatch` token
whose field is private to this module, which is what stops a caller from
skipping `execute` and calling the primitive itself; verified by probing all
three construction paths (tuple constructor, `Default`, struct literal) from
outside the crate. It does not defend against an implementation that stashes
its own input — that is deliberate code, not a forgotten check.
`actions.rs` is split into `actions/{mod,args,outcomes}.rs` for review
ergonomics: `Action` and the shared validators in the parent, one file per side
of a call. No logic moved — the item set is identical and the only content
changes are intra-doc link paths.
Dropped `only_housekeeping_actions_are_best_effort`, which asserted a constant
against itself.
Addresses the interface half of #6467.
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>
Four bug-pass findings on #6742, all one shape: a rule the contract states in prose but does not enforce at some layer, so the layer above it decides. **status/code contradictions.** `Indeterminate` carrying a known pre-dispatch code passed validation, so `execute` returned Ok for a response contradicting itself: `unauthenticated` means the host refused before running anything — a known fate — while `indeterminate` means the fate is unknown. A caller reading the status would reconcile side effects that provably never happened. We already rejected the mirror case, `outcome_unknown` under `failed`. Rather than add the reverse one-off check, the pairing is now a closed table. `BrokerErrorCode::may_be_failed` and `may_be_indeterminate` are exhaustive matches with no wildcard, so a new code cannot inherit an answer — it forces a decision at both ends. `indeterminate` admits only `outcome_unknown` and `internal`; a host fault mid-execution is the one legitimate "I don't know", and every other code names a fate the host knows. The rationale lives once, in the `BrokerErrorCode` docs, and `BrokerResponse::validate` consults the predicates instead of restating them. **A read page bounded only by the protocol cap.** `ActionOutcome::validate` never sees the request, so it could only check `MAX_PAGE_LIMIT`: a host could answer a one-message read with five hundred, within the cap and still an overrun of what was asked. `validate_for` now applies the request's own number, the one place both halves are in scope. An absent `limit` is held to the new `DEFAULT_PAGE_LIMIT` (100) rather than treated as consent to an unbounded page — the cap is what a host may ever send, the default is what it may send unasked, so the two constants are deliberately different numbers. **A response envelope that was silently non-strict.** `#[serde(flatten)]` disables `deny_unknown_fields` — serde cannot combine them — and the note saying so concluded strictness was enforced elsewhere. It was not: the envelope accepted and discarded an unknown top-level key, and accepted an `error` beside a succeeded outcome or an `outcome` beside a failure, dropping whichever half it could not represent. Deserialization now routes through a private strict wire form with an exact key set per status; serialization is untouched, so the wire format does not change. The request envelope has the same `flatten` and was checked for the same hole: it does not have one, because `ActionArgs` is adjacently tagged and contributes exactly `action` and `args`, leaving `deny_unknown_fields` in force. That is now pinned by test rather than assumed. **Events could smuggle members past the schema.** `nostr`'s `Event` deserializer accepts and discards unknown members, so a genuinely signed event could carry an extra `secretKey` and parse clean — the no-secret rule stopping at the envelope instead of reaching inside it. `BrokerMessage` now deserializes through a `deny_unknown_fields` intermediary with exactly the seven canonical NIP-01 members, and the module docs state that the invariant extends inside event objects. The exact-key-set table now covers both envelopes and the error payload, not just args and outcomes: a key set nobody pins is a key set a field can be added to. A new test parses raw bytes through a transport, because the strict-envelope and strict-event guards live in `Deserialize` and the typed test double could never reach them — it hands back a value that was never on a wire. Each guard was falsified separately against 5116ca2, since one passing guard is no evidence for the others: reverting the limit check, the strict envelope, the strict event, collapsing `DEFAULT_PAGE_LIMIT` onto the cap, dropping `deny_unknown_fields` from the request envelope, and adding a field to the response envelope each fail a different named test. Reported by ss-bugs-02 on #6742. Addresses the interface half of #6467. 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>
The strict response envelope could be walked straight past with JSON `null`.
`deny_unknown_fields` constrains *which* members may appear, not what they may
hold, and `#[serde(default)] Option<T>` maps an explicit `null` to `None` --
which is indistinguishable from absent. The per-status match reads absence as
"this status does not carry that member", so
{"status":"failed","action":null,"outcome":null}
and a succeeded response with `"error":null` both parsed as well-formed and
skipped the contradiction check entirely. `validate()` returned Ok on an
envelope that contradicts itself. The previous commit closed the value-shaped
version of this hole and left the null-shaped one open, which is the same class
of bug one spelling short.
The fix is a rule rather than three checks: **no optional member anywhere in
this contract accepts `null`.** Nothing here emits one -- `skip_serializing_if`
omits the member instead -- so `null` was never a value this contract defined,
only one serde happened to accept. A shared `absent_or_valued` deserializer
rejects it, applied to all 17 optional members across the response envelope,
every args type, and `MessagePage::next_cursor`.
Rejecting outright beats tracking presence beside the value, which was the other
option. Presence tracking preserves the ambiguity and moves the decision to
every reader: each one then has to answer what a present-but-null member meant,
and the answer only has to be gotten wrong once. With one spelling of absence
there is no such question -- and a host implementer never has to guess whether
`{"limit": null}` means the default or no limit, because it means neither.
Two members did not need the helper and are documented as such rather than
changed: `mentionsOnly` and `replayed` are plain `bool`, and `null` already
fails as a type error. Both rejections are pinned by test anyway, so the doc
cannot drift and a later change to `Option<bool>` -- which would need the guard
-- fails loudly.
Audited the other two strict layers for the same path, both clean: the request
envelope's flattened `ActionArgs` is adjacently tagged, so `action` and `args`
are required and reject `null` as a type error; `StrictEvent`'s seven canonical
members are all required, and each rejects `null` on type. Reported either way
per the review request.
Coverage is derived, not listed. `no_member_of_any_payload_accepts_an_explicit_null`
walks every JSON-pointer path in the real serialized fixtures -- every action,
every outcome, nested members and array elements included -- and nulls each in
turn, so an optional field added later is covered without anyone remembering to.
It asserts the untouched fixture parses first, since a probe that rejects for the
wrong reason proves nothing.
`a_status_incompatible_member_is_rejected_as_null_not_only_as_a_value` carries
the original repro, which the fixtures structurally cannot reach: a serialized
response never contains the member its own status forbids. Both assert on the
parse rather than on `validate()` -- a malformed envelope should never become a
value that some caller has to remember to check.
Falsified four ways against 5e2879a, since one passing guard is no evidence
for the others: reverting the response envelope, the args optionals, or
`nextCursor` each fails a named test, and neutering the helper to accept `null`
fails both.
Reported by ss-bugs-00 on #6742. Addresses the interface half of #6467.
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>
The rule that omission is the only spelling of absence was already enforced in code and pinned by test, but the only prose stating it was a paragraph inside "The no-secret rule" — a section about key material. A host written in another language does not read a secrets section looking for JSON encoding rules, so the first place that implementer would learn the rule is a rejected request. Promote it to its own top-level contract section, `Optional members: omission is the only spelling of absence`, and name the concrete failure mode: a serializer that emits `null` for an unset field (Go without `omitempty`, a `None` attribute through `json.dumps`, a map that assigns the key unconditionally) produces a payload this contract rejects in full. The fix on their side is one sentence long — omit unset members — which is worth saying outright rather than leaving to be inferred from an error string. Also point at it from the HTTP binding docs, since that is where someone configures the serializer that will get this wrong, and keep the reference from the no-secret rule so the third strictness hole is still traceable from where the other two are described. Docs only; no behavior change. The anchor both links resolve to is verified present in the generated HTML, since rustdoc does not check intra-doc fragments. Requested by ss-core-02 on #6742 as the doc half of the explicit-null fix. 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>
…tness holes 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>
baxen
force-pushed
the
ss-dev-00/broker-tests-split
branch
from
August 25, 2026 08:34
6e51470 to
760bc78
Compare
Two P2 review findings on #6742, both reproduced from an out-of-crate consumer before being fixed, and each fix falsified by reverting it. 1. Identity spellings broke correlation (bugs-00, correlate.rs:42). `channel()` validated a UUID with `Uuid::parse_str` and returned the caller's spelling untouched, while `correlate_identities` compared the request's and the outcome's `channelId` as strings. An uppercase request against a host's canonical lowercase echo of the same channel therefore failed `validate_for` — a *correct* answer rejected, which is worse than the mismatch the check exists to catch, because it makes a working host unusable. Fixed as a class, not as a UUID case. The finding named case; the validator also admits the unhyphenated, `{braced}`, and `urn:uuid:` forms, and all four froze un-canonicalized. Hex has the same property. So: - Canonicalize where a value enters. `channel()` now returns the lowercase hyphenated form, and every member holding an identity canonicalizes on deserialize too (`channel_id`, `hex64_field`, `absent_or_valued_hex64`). The wire is the door no `validated()` covers: those fields are public `String`s, so a parsed payload reaches a caller without passing through any validator. Both doors delegate to the same function, so they cannot drift. - Compare parsed identities, not bytes. `correlate_identities` parses each `channelId` as a `Uuid`; pubkeys are compared as `PubkeyHex`, which lowercases in its only constructor and its serde path. The two guards are deliberately independent and asserted separately, so neither is the only thing holding. A genuinely different identity is still rejected. Coverage is derived as well as enumerated: one test walks the real fixtures, re-spells every identity-named member, and requires the canonical value back, so a member added later with the wrong `deserialize_with` fails without anyone extending a list. 2. `BrokerResult` was a second, lax wire door (bugs-00, mod.rs:586). The envelope's strict reader requires the exact key set the declared status admits, but the exported result type also derived its own reader, and that one accepted and dropped arbitrary siblings: `status: failed` beside an `error` and a `secretKey`, or a succeeded result beside an `error`. A consumer parsing the result type directly got an `Ok` value whose complete wire shape had never been vetted, from bytes that fail through the envelope. Decision: take the type off the wire — it is no longer `Deserialize`. The alternative, its own strict reader, would be a second copy of the per-status contradiction rules, and two copies of a strictness check drift; this hole exists precisely because one layer's strictness did not reach another's. Removing the door leaves one implementation and nothing to keep in sync. Nothing is lost: a bare `{"status": …}` object is not a payload this contract defines, and the whole module is new in this PR, so it had no consumers. `Serialize` is retained — it produces the envelope's flattened wire form — so the wire form is unchanged and this is a read-side restriction only. The absence is pinned at compile time, since a runtime test cannot call a `Deserialize` impl that does not exist: a probe resolves to an inherent method only when the bound holds, and it is first asserted against two types that *do* have readers, so a broken probe cannot pass silently. The contract docs gain a section on identity spelling, `validate_for`'s nine-row table records that comparison is on parsed identities, and the strictness section records the one-door rule. Also: adding the identity docs pushed mod.rs to 1,021 lines, past the repo's 1,000-line ceiling, so the strict response reader moved to `broker/wire.rs` (mod 913 / wire 121) — a pure move, same treatment `correlate.rs` already got. The derived-coverage test matches identity members case-insensitively. An earlier revision of it matched the suffix `"EventId"` exactly, which silently skipped the outcome member spelled `eventId` and left every response-side wire door unpinned — mutation testing caught it by removing the outcome `channelId` door and surviving. It now walks response fixtures as well as request ones, and floors the two directions separately, because a single combined floor would be satisfied by the request side alone. Validation: 299 tests pass in buzz-sdk (295 before), workspace clippy `-D warnings` clean, fmt clean, `cargo doc` warning-free with the new section anchor verified in the generated HTML, every touched file under 1,000 lines. Seven mutations, one per guard, each killed by a named test. `cargo test --workspace` is green except `test_120s_timeout` / `test_cancellation_immediate` in buzz-pair-relay, a pre-existing flake reproduced 2-in-8 on unmodified main at c5166f2 where no broker module exists. 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> Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
`crates/buzz-sdk/src/broker/tests.rs` is 2,638 lines against the repo's 1,000-line ceiling, and the next broker test addition would push it further. Split along the `// ── Section ──` headers the file already carried, so the cut lines are the ones the author already drew. | module | lines | holds | |---|---|---| | `tests/mod.rs` | 184 | shared fixtures, helpers, `mod` declarations | | `tests/schema.rs` | 689 | action coverage, envelope round-trip and rejection | | `tests/wire_schema.rs` | 361 | exact-key-set tables, the no-secret invariant | | `tests/validation.rs` | 714 | argument bounds, read provenance, result/status tables | | `tests/identities.rs` | 423 | one spelling per identity; the result type's absent reader | | `tests/client.rs` | 312 | identical-bytes retry, client trait, test doubles | Six modules, not the four #6746 sketched and not the five of the previous revision. Two sections each force their own module for the same reason: the wire-schema section would leave `schema.rs` at 1,045 lines, and the identity-canonicalization section added by #6742's review revision would leave `validation.rs` at roughly 1,130. Either would be a split that still violates the cap it exists to satisfy. Splitting them out keeps every file under the ceiling without moving a single test across a concern boundary. Verified as a pure move rather than asserted. Concatenating the six modules in the original section order and stripping only the added scaffolding — module docs, five `mod` declarations, per-file `use super::*` — reproduces the original file exactly, with a single deliberate exception: `member_paths` becomes `pub(super)` because it is now shared across two modules. That one-token visibility change is the whole diff of the reconstruction: -fn member_paths(value: &serde_json::Value, prefix: &str, out: &mut Vec<String>) { +pub(super) fn member_paths(value: &serde_json::Value, prefix: &str, out: &mut Vec<String>) { And the test set, not merely its size: the same 37 broker tests exist before and after, compared with module paths normalized away, so a rename plus a deletion could not hide behind a matching count. Validation: `cargo test -p buzz-sdk --all-targets` 299 passed / 0 failed, identical to the baseline at 7c853e0; package clippy `-D warnings` exit 0 with no dead-code warning from the moved helper; `cargo fmt --all --check` clean; `cargo doc` warning-free; every file under 1,000 lines. Closes #6746. Stacked on #6742. Contract context: #6467. Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com> Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
baxen
force-pushed
the
ss-dev-00/broker-tests-split
branch
from
August 25, 2026 11:36
760bc78 to
df26470
Compare
baxen
added a commit
that referenced
this pull request
Aug 27, 2026
Addresses the interface half of #6467. This is the contract for how a keyless agent asks a host to act on its behalf — and only that. No host, no transport, no signing, no Desktop code; the only implementation is a test double. An agent that holds no secret key still has to read its channels, answer a mention, react, keep its profile, address its memory, and create the agents it needs. This PR defines the vocabulary for those requests and the rules both sides must follow. **#6467's open question, answered: per-operation, not "sign this".** A raw signing primitive tells the host *who* is asking but not *what for*, so the only policies it can express are all-or-nothing. A closed set of named operations lets a host serve channel reads while refusing agent creation, and gives any later policy layer something to attach to. The cost is that adding an operation means changing the contract — which is the point. Signing, publishing, and the auth/encryption NIPs are deliberately *not* operations; they are mechanisms inside whoever holds the key, and a test pins that none of those names resolve. **Nine operations in v1:** read a channel (whole channel, one thread, or mentions-only — these differ by filter, not permission, so they share one operation), post, reply, react, set profile, derive a storage address, and create / update / delete agents. Presence and typing are deferred as housekeeping a host may decline regardless; streaming is deferred, so waking on a mention is a polled mentions-only read. Each operation has typed, validated arguments and a typed outcome. **What the contract enforces structurally rather than by instruction:** - *Retry means identical bytes.* A request is validated and serialized exactly once and the transport only ever sees that frozen form. There is no way to re-render it, so a retry can't drift and be mistaken for a conflicting request. - *Read cursors are opaque.* Host-issued strings, validated for shape only, never parsed or compared. The host owns ordering and paging stability instead of being locked into timestamps. - *Reads return signed Nostr events.* A keyless agent verifies authorship and content locally and trusts the host only for completeness and authorization. Reply ancestry and mentions are derived from the signed tags, so nothing can disagree with the signature. - *Validation is the only door to a response.* The transport primitive hands back an unjudged envelope; the only response a caller can actually obtain is one already checked against the request it answers — request id correlates, the outcome's operation matches, identifiers and cursors are well-formed. A mismatch counts as "no answer", not a failure verdict. The raw primitive can't be invoked from outside the crate (pinned by compile-failure checks), so skipping that step isn't an option. - *Authority is never asserted by a body.* The envelope has no requester, owner, scope, or credential field; all of that comes from the session the host authenticated. Ownership of a created agent is implicitly the requester, never a field — which is what lets agents own agents while the chain still ends at a human. - *No secret crosses the wire.* Agent creation returns a pubkey and handle, never the minted key. Every payload type rejects unknown fields, and a test pins the exact JSON key set of all eighteen argument and outcome types, so no field can be added without a reviewer touching that table. Two honest limits: a string can physically hold secret text, so keeping secrets out of message content is host policy; and nothing stops a host from *holding* keys — it just may not hand them over. - *Null is never legal; omission is the only spelling of absence.* Every optional member rejects an explicit null. Otherwise null and omitted would be indistinguishable, and a contradiction check on the response envelope could be skipped by sending one (a real hole in an earlier revision). Coverage is derived by walking every path in the real fixtures, so new optional members are covered automatically. Serializers that emit null by default (Go without `omitempty`, Python's default dump) need configuring — called out in the module docs. **Wire mapping.** One POST endpoint, JSON body is the frozen request, opaque bearer credential in the auth header. Every verdict the host reached — including failure and including a rejected credential — comes back as a well-formed envelope with 200, because a second copy of the verdict in the status line could only disagree with it. Clients still try to parse an envelope regardless of status (intermediaries may remap) and fall back to the status only when none is present. A rejected credential is a host verdict carrying the promise "the action did not run"; transport errors are reserved for "nothing can be concluded about side effects." Binding the credential to a specific agent and conversation is the host's job and deliberately unenforceable here. **Relationship to #6543.** Envelope, request-id rules, protocol version, the three-way succeeded / failed / indeterminate result, error codes, and the agent-management argument shapes carry over. Changed deliberately: pubkeys are a validated type rather than bare strings, "capability" became "action" throughout, and a new "unsupported" error lets a host decline a best-effort operation without it reading as a fault. Both PRs land in the same module path — whichever merges second needs a hand reconcile, which I own as the author of #6543. **Validation at `7c853e0fb`.** 299 tests in the sdk crate (full package run), workspace tests green apart from the pre-existing relay timeout flake, which I reproduced on unmodified `main`. Clippy with warnings denied, fmt, warning-free docs, and the pre-push gate all pass. Review findings were closed with mutation tests — each fix verified by reverting it and watching a named test fail. One of those exposed a gap in my own coverage (response-side fields weren't pinned); that's fixed and floored separately from the request side so neither can mask the other. **Size.** About 5,400 lines across 8 files, roughly half tests. Every source file is under the repo's 1,000-line cap except the test file; #6759 splits it mechanically, is stacked on this branch, retargets to `main` after merge, and lands before PR 2 of the stack. --------- Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Signed-off-by: Bradley Axen <baxen@squareup.com> Signed-off-by: ss-core-02 <cca2c695c61d409321106cc399009c2cc674d72cc8900b2999a68fe54577a411@buzz.block.builderlab.xyz> Co-authored-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz> Co-authored-by: ss-core-02 <cca2c695c61d409321106cc399009c2cc674d72cc8900b2999a68fe54577a411@buzz.block.builderlab.xyz>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mechanical split of
crates/buzz-sdk/src/broker/tests.rs(2,638 lines, against the repo's 1,000-line guidance) into six modules along the// ── Section ──headers the file already carried.Draft until #6742 merges. Stacked on
ss-dev-00/broker-action-contract(7c853e0fb), so the diff here reads clean only once PR 1 lands. Rebased onto PR 1's latest review rev; if PR 1 takes further changes I rebase again.The split
tests/mod.rsmoddeclarationstests/schema.rstests/wire_schema.rstests/validation.rstests/identities.rstests/client.rsSix modules, not the four #6746 sketched and not the five of the previous revision. Two sections each force their own module for the same reason. With the wire-schema section still inside
schema.rs, that file lands at 1,045; with the identity-canonicalization section from PR 1's latest review revision still insidevalidation.rs, that file lands at roughly 1,130. Either would be a split that violates the cap it exists to satisfy. Both sections are self-contained behind their own headers, so pulling them out keeps every file under the ceiling without moving a single test across a concern boundary. All six under the cap.Reviewing this as a no-op
The issue asks for the split to be reviewable as a pure move, so I verified that instead of asserting it.
1. Byte-identical reconstruction, with one deliberate exception. Concatenating the six modules in the original section order and stripping only the added scaffolding — module docs, five
moddeclarations, per-fileuse super::*— reproduces the original file exactly except for a single one-token visibility change, which is the entire diff of the reconstruction:member_pathsis now shared across two modules, so it needspub(super). Nothing else differs across 2,683 lines.Stronger than the SHA alone: every hunk in the raw concat diff is a pure addition (no
<lines, no change hunks), so no original line was reordered, reindented, or edited. My first attempt usedcat -s, which squeezes blank lines and could have masked whitespace drift — redone with no squeeze and no trim.2. The test set, not just the count. Same 37 broker tests before and after, compared module-path-agnostically against
cargo test --lib -- --listat7c853e0fb. A matching count alone could hide a rename plus a deletion.3. Gate at the committed tree (
df2647096):cargo test -p buzz-sdk --all-targets299 passed / 0 failed, identical to the baseline at7c853e0fb— the guard number moved 291 → 295 → 299 as PR 1 took review revisions, so #6746's pinned 291 is stale twice over;cargo clippy -p buzz-sdk --all-targets -- -D warningsexit 0, with no dead-code warning from the now-shared helper;cargo fmt --all --checkclean;cargo docclean; pre-push gate (push-head-scope, check-push-org, branch-skew, file-size-check, rust-tests, desktop-tauri-checks) all pass.Two judgment calls worth flagging
Dropped
use nostr::Keys;from the child modules.use super::*already re-exports it from the parent. An unnecessary import is noise in a change whose whole value is being obviously mechanical. Re-ran the reconstruction check after removing it — still byte-identical.Corrected my own rationale for staying in-crate. I first wrote that these tests must live in-crate because the strictness guards sit behind private intermediaries (
WireResponse,StrictEvent) and an out-of-crate test "would still pass while proving less." Before shipping that claim I tried to falsify it: I copied the modules verbatim intocrates/buzz-sdk/tests/, changed only the import preamble, and all tests compiled and passed. The claim was false — the tests name only public API, and they reach those private types through the publicDeserializeimpls, which is the same door a host author uses.So the honest reason is narrower, and the doc comment now says it: they stay in-crate to keep this change a pure move rather than a move plus a scope change. One guard does rest on privacy, but the crate's, not this module's —
Dispatch's field is private toclient, so nothing outside it can forge the token and bypassexecute; tests here implementsendand never call it. Probe deleted;git statusshows only the split.Closes #6746. Stacked on #6742. Contract context: #6467.