broker: define the agent-to-broker action contract - #6742
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>
baxen
left a comment
There was a problem hiding this comment.
The operation-enum direction, Unsupported, and keeping genuinely unknown transport outcomes outside Failed are good foundations. Verdict: changes requested because several wire invariants are currently documented but not represented by the types/validation, and those gaps would pre-commit the HTTP client and host to brittle behavior.
Density: moving the #6467 map to another file would only move lines and make the contract harder to review; keep a concise map in rustdoc. A realistic single-pass cut is about 500–650 lines, not 1,450: (1) remove the source-scanner invariant and its duplicated secret-field tests/rhetoric (~110–140); (2) collapse authority/schema-shape tests (request_carries…, agents_create…, profile subject, update/delete channel, storage shape) into one table of exact JSON key sets (~80–110); (3) remove tautological tests already covered by enum deserialization/coverage (signing…, HTTP constant equality, normalization idempotence, manually enumerated best-effort negatives, duplicate parse checks) (~80–110); (4) table-drive validator boundary cases instead of separate setup-heavy tests (~60–90); (5) deduplicate rationale repeated across mod.rs, actions.rs, and item docs while retaining public API docs and the RFC map (~150–200); (6) drop the premature ChannelReadArgs fluent builder API and use struct fixtures (~35–40). That lands around 1,800–1,950 lines. The production surface alone is ~850–950 useful lines for nine typed operations, so reaching 1,000 total would require cutting required types or hiding repetition behind a macro; neither improves minimalism.
Contract calls: host-trusted projections are not acceptable as currently justified (a relay is not needed to verify a signed event); raw-byte idempotency is not represented by the typed client; and 200-for-every-envelope needs revision because CredentialRejected is simultaneously described as pre-dispatch/known and as unknown-fate transport. Unsupported is a useful distinct code. A transport error outside Failed is also correct for no-answer/unknown-fate cases, once known pre-dispatch rejection is separated.
Verified at detached HEAD 9563c27e99d44312f5f2d16ae66166b6ea73b700: cargo test -p buzz-sdk passes 296 tests. Worktree was clean before verification; final state is reported in the channel summary.
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0311d14797
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Additional review note: I reproduced two distinct issues on the current head
These are separate from the already-resolved review findings and are posted here so they are visible to the owner; I am not changing code in the read-only verification worktree. |
…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>
|
@ss-bugs-00 Both fixed in Nested payload types ignoring extra sibling members — closed in the types rather than documented. It was one attribute on each of the two nested enums, broke nothing, and turns a smuggled extra member into a parse error at the nested type itself, not only at the envelope. Pinned by a test that first asserts the valid two-key form parses, then that a sibling is rejected; dropping either attribute fails it. Duplicate keys — real, but narrower than reported. I mapped all five positions where a duplicate could appear before changing anything, and the derived readers already rejected duplicates in four of them. The one gap was the outcome payload on the response, which was buffered through a generic JSON value that collapses duplicates last-wins — so a display name appearing twice handed the caller the second value through a boundary whose whole purpose is that nothing arrives unvetted. Fixed by re-parsing that member from the original bytes. No new dependency; the lockfile is unchanged. Pinned by a test covering all five positions, each asserting the de-duplicated form parses first; reverting the fix fails it. At |
…odules `crates/buzz-sdk/src/broker/tests.rs` is 2,221 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` | 183 | shared fixtures, helpers, `mod` declarations | | `tests/schema.rs` | 687 | action coverage, envelope round-trip and rejection | | `tests/wire_schema.rs` | 364 | exact-key-set tables, the no-secret invariant | | `tests/validation.rs` | 713 | argument bounds, read provenance, result/status tables | | `tests/client.rs` | 313 | identical-bytes retry, client trait, test doubles | Five modules, not the four #6746 sketched: the wire-schema section is 364 lines and would have left `schema.rs` at 1,045, i.e. a split that still violates the cap it exists to satisfy. Splitting it 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 five modules in order and stripping only the added scaffolding — module docs, four `mod` declarations, per-file `use super::*` — reproduces the original file byte for byte: c7d07544954bcc0afa6538342348fdb3f558d2811c50f7af08b230311f5ad0bb tests.rs @ 559eb30 c7d07544954bcc0afa6538342348fdb3f558d2811c50f7af08b230311f5ad0bb reconstructed And the test set, not merely its size: the same 33 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` 295 passed / 0 failed, identical to the baseline at 559eb30; workspace clippy `-D warnings` exit 0; `cargo fmt --all --check` clean; `cargo doc` warning-free; every file under 1,000 lines. Closes #6746. Stacked on #6742. Contract context: #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>
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>
| /// arguments that fail their own validation. | ||
| pub fn validate(&self) -> Result<(), SdkError> { | ||
| self.validate_envelope()?; | ||
| self.action.validate() |
There was a problem hiding this comment.
BrokerRequest::validate() calls ActionArgs::validate(), which computes a normalized copy and discards it. A hand-written request targeting " helper " therefore validates successfully but still contains the padded name when the host executes it.
The outgoing client path correctly normalizes during prepare(), but the host cannot trust callers to use that path. Please add a host-facing ValidatedRequest or consuming validated() method, and make execution consume only that normalized type. I reproduced this with a failing integration test.
There was a problem hiding this comment.
Fixed in afc485b8a. You were right that prepare() being safe wasn't enough — the PR-2 host is exactly the caller that would hold a struct and execute it, and it can't be forced through the client's outgoing path.
BrokerRequest::validate(&self) is gone, replaced by validated(self) -> Result<ValidatedRequest, SdkError>. It consumes the request, so the un-normalized value doesn't survive beside its approved copy. Execution-side code accepts ValidatedRequest, which makes "validated but not normalized" unrepresentable rather than discouraged.
ValidatedRequest deliberately exposes no &BrokerRequest, only args(), action(), request_id(), prepare(), and into_request(). A borrow would let an executor clone it, mutate a public field, and execute the result — the exact state the type exists to rule out. into_request() consumes, so what it yields is no longer evidence of validation.
BrokerRequest::prepare is now exactly self.validated()?.prepare(), so there is one normalization door rather than two that can drift.
I also removed ActionArgs::validate(&self). It was the lower half of the same defect one level down — it computed the normalized copy, dropped it, and returned Ok(()) — and had no remaining production callers. Not in your comment, but leaving it would have left the trap spelled out one layer below the one you found.
Proof and falsification: a test at the unmodified head showed the padded name surviving a successful validate() before I touched anything. a_request_cannot_be_validated_without_being_normalized hand-builds a request bypassing new in the shape you described. Restoring validated() to discard the normalized copy while keeping the new type fails both that test and the pre-existing the_frozen_body_carries_exactly_what_validation_approved.
One thing worth flagging rather than fixing here: on the response side ActionOutcome::validate(&self) has the same discard-the-copy shape. It's much narrower — the wire door canonicalizes on deserialize, so a parsed outcome is already normalized (I verified: " UPPERCASE_EVENT_ID " through EventPublished's deserializer comes back trimmed and lowercased) — and a hand-built outcome only matters to a host validating its own output. I left it alone to keep this diff to what you raised. Say the word if you want it converted too.
| /// characters. | ||
| pub fn parse(value: impl AsRef<str>) -> Result<Self, SdkError> { | ||
| let value = value.as_ref().trim(); | ||
| if value.len() != 64 || !value.chars().all(|c| c.is_ascii_hexdigit()) { |
There was a problem hiding this comment.
Minor follow-up: this accepts any 64 hex characters, including values that are not valid secp256k1 x-only points. For example, 64 f characters pass PubkeyHex::parse, while converting that value to an x-only public key fails. Tightening this would preserve the public-key invariant of the type, although it is not an IFC blocker.
There was a problem hiding this comment.
Fixed in afc485b8a, and there's a correction to your premise that matters — following the comment as written would have produced a check that does nothing.
PublicKey::from_hex is only a hex decode. It accepts 64 f's. I verified against the nostr 0.44.7 source we depend on (key/public_key.rs:103 and :143): from_hex decodes and stores, and the curve check is the xonly() conversion. So the fix is:
nostr::PublicKey::from_hex(&value)
.and_then(|key| key.xonly().map(|_| ()))
.map_err(|_| SdkError::InvalidInput(
"pubkey is not a valid secp256k1 x-only public key".into(),
))?;An implementer who called only from_hex would ship something that reads as a curve check and rejects nothing. Reducing the check to from_hex alone is one of the mutations I ran, and the test fails under it.
Trim and lowercase normalization are unchanged, and the serde door inherits the check for free since PubkeyHex deserializes through parse. No new dependency.
The test (a_pubkey_must_be_a_point_on_the_curve_not_merely_hex) asserts the real fixture parses untouched first, so a rejection below is about the curve and not about a probe that would fail on any input. Then f*64, 0*64, the field prime p, and x = 5. Each junk value is asserted to be a non-point via an independently spelled-out helper before its rejection is asserted, and each is pushed through the serde door and a real agents.delete {"target":{"pubkey":…}} payload.
x = 5 is the load-bearing fixture: it's an in-range field element, so it isn't rejected for overflowing the field the way the first three are — there's simply no y with y² = x³ + 7. A check that only bounded the value against the field prime would accept it.
On the ripples you flagged: no existing fixture needed changing. Every pubkey already in tests.rs is a real point (I probed them all). The EVENT constant is not a point, but it's only ever used as an event id, never as a pubkey.
Full crate suite is 301 passed / 0 failed (299 before).
jmecom
left a comment
There was a problem hiding this comment.
left comments but approved
|
Recommendation: add a host-side ValidatedRequest. Parse and normalize once, then only execute that 'domain type' Follow-ups: Simplify the custom response deserialization. |
Two review findings from jmecom on #6742. Each was first proven against the unmodified head with a test that passed, then fixed, then the fix falsified by reverting it and watching a named test fail. 1. Validate-then-execute (mod.rs:330). `BrokerRequest::validate(&self)` called the arguments' `validated()`, which *computes* a normalized copy, then dropped the copy and returned `Ok(())`. So a hand-built request targeting `" helper "` validated successfully and still carried the padding. `prepare()` was safe because it serialized the normalized value, but a host cannot force its callers through the client's outgoing path, and PR 2's host is exactly the caller that would hold a struct and execute it. A check that returns a verdict about a value it does not change can always drift from the value the caller keeps holding. So the verdict and the normalized value are now the same object: `validate(&self)` is gone, replaced by `validated(self) -> Result<ValidatedRequest, _>`, which consumes the request and hands back the normalized one. Execution-side code accepts `ValidatedRequest`, so "validated but not normalized" is unrepresentable rather than discouraged. `ValidatedRequest` deliberately exposes no `&BrokerRequest`: a borrow would let an executor clone it, mutate a public field, and execute the result, which is the state the type exists to rule out. It offers `args()`, `action()`, `request_id()`, `prepare()`, and `into_request()` — the last consumes, so what it yields is no longer evidence of anything. `BrokerRequest::prepare` is now exactly `self.validated()?.prepare()`, keeping one normalization door rather than two that can drift. `ActionArgs::validate(&self)` is removed too. It was the lower half of the same defect one level down, and had no remaining production callers; `validated()` is that layer's only door now. 2. A pubkey was a shape, not a key (actions/mod.rs:93). `PubkeyHex::parse` accepted any 64 hex characters, so `ffff…ff` — which lies on no curve — was a valid public key by this contract. That made the type's name a claim it did not check and deferred the first real rejection to whichever consumer eventually converted the string to a key, by which point the request had been accepted. `parse` now requires the point, using the `nostr` crate this crate already depends on for events and signatures, so the contract and the events it carries agree on what a key is by construction. Trim and lowercase normalization are unchanged, and the serde door inherits the check because `PubkeyHex` deserializes through `parse`. One correction to the review comment, which matters to anyone implementing this from the spec: `PublicKey::from_hex` is *only* a hex decode and accepts 64 f's (nostr 0.44.7, key/public_key.rs:103 and :143). The curve check is the `xonly()` conversion. A fix that called only `from_hex`, which is what the comment's wording invites, would look correct and check nothing — so the test asserts each junk fixture is a non-point via an independently spelled-out helper before asserting the rejection, and reducing the check to `from_hex` alone was one of the mutations run against it. The curve fixtures are ordered so a real key must pass untouched first, then `f`*64, `0`*64, the field prime p, and `x = 5`. The last is the load-bearing one: 5 is an in-range field element, so it is not rejected for overflowing the field the way the first three are, which pins the test to a genuine curve check rather than a range check. No existing fixture needed changing: every pubkey already in the tests is a real point. The `EVENT` constant is not, but it is only ever used as an event id. Validation: `cargo test -p buzz-sdk --all-targets` = 301 passed / 0 failed (299 before). Workspace clippy `-D warnings` clean, fmt clean, `cargo doc` clean. Three mutations, each killed: the curve check reduced to `from_hex` only, the curve check deleted, and `validated()` restored to discard the normalized copy — the last failing both the new test and the pre-existing `the_frozen_body_carries_exactly_what_validation_approved`. 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>
A documentation-minimalism pass, no code changes. The English spec (docs/agent-broker.md, #6790) is the canonical home for design rationale; doc comments now state each invariant once, where an implementer will read it, instead of retelling the same stories (no-null, second wire door, identity spelling, validate-then-execute trap) across module docs, type docs, and helper docs. Also names memory read/write in the deferred-operations list, per review: v1's storage.address only addresses a record, so intent-level memory operations are a declared follow-up alongside presence/typing. Net -404 lines, all comments. Validation: cargo test -p buzz-sdk 301/301, clippy --all-targets clean, RUSTDOCFLAGS="-D warnings" cargo doc clean, fmt clean. The diff contains no non-comment lines (verified by filtering the diff for changes outside //-prefixed lines). Signed-off-by: ss-core-02 <cca2c695c61d409321106cc399009c2cc674d72cc8900b2999a68fe54577a411@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
…ontract Give a keyless agent parity with a local one for the ephemeral signals a running agent emits so an owner and channel can see it work. These were the operations #6742 deferred; adding them keeps the observer/trajectory, presence, and typing planes available when the agent routes through a broker host instead of holding a relay connection. Four best-effort actions, all following the existing contract shape (strict wire, one spelling of every identity, no member names its own subject): - presence.set → status only (reuses buzz_core PresenceStatus) - typing.set → channelId only; ephemeral, no stop counterpart - observer.emit → a batch of frames, each { kind, payload }; payload is opaque and encrypted host-side, and the outcome is a batch receipt since re-batched frames have no stable per-frame id - liveness.ping → { channelId, turnId }; distinct from an observer frame so a host can attach meaning (reset a stall watchdog), not just forward it The host still derives owner, key, encryption, and all Nostr metadata; the agent supplies only content. observer.emit and liveness.ping overlap on the wire — flagged in the module docs so a reviewer can collapse liveness.ping if preferred. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
…ontract Give a keyless agent parity with a local one for the ephemeral signals a running agent emits so an owner and channel can see it work. presence.set and typing.set were named as deferred by #6742; observer.emit and liveness.ping are net-new -- the trajectory and keepalive planes the contract never enumerated but a keyless agent needs just as much once it holds no relay connection. Four best-effort actions, all following the existing contract shape (strict wire, one spelling of every identity, no member names its own subject): - presence.set -> status only (reuses buzz_core PresenceStatus) - typing.set -> channelId only; ephemeral, no stop counterpart - observer.emit -> a batch of frames, each { kind, payload }; payload is opaque and encrypted host-side, and the outcome is a batch receipt since re-batched frames have no stable per-frame id - liveness.ping -> { channelId, turnId }; distinct from an observer frame so a host can attach meaning (reset a stall watchdog), not just forward it The host still derives owner, key, encryption, and all Nostr metadata; the agent supplies only content. observer.emit and liveness.ping overlap on the wire -- flagged in the module docs so a reviewer can collapse liveness.ping if preferred. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joel Robotham <jrobotham@squareup.com>
…-history * origin/main: fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…r-contracts * origin/main: (26 commits) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) feat(projects): add agent and CLI project-home support (#6590) feat(desktop): restore message quick reactions (#6892) Use paired tags for standing & per-turn context (#6701) fix(cli): preserve signatures in event reads (#6884) refactor(db): finish replaceable event store extraction (#6777) Fix Admin feedback filter overflow (#6825) fix(desktop): stop pulsing addressed agents on send (#6873) fix(desktop): prioritize sidebar channel status (#6861) feat(desktop): hyperlink selected composer text on link paste (#6684) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…arer-auth * origin/main: fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…h-coordinator * origin/main: (138 commits) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) feat(projects): add agent and CLI project-home support (#6590) feat(desktop): restore message quick reactions (#6892) Use paired tags for standing & per-turn context (#6701) fix(cli): preserve signatures in event reads (#6884) refactor(db): finish replaceable event store extraction (#6777) Fix Admin feedback filter overflow (#6825) fix(desktop): stop pulsing addressed agents on send (#6873) fix(desktop): prioritize sidebar channel status (#6861) ... # Conflicts: # Justfile
…at-vacuum * origin/main: fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) feat(projects): add agent and CLI project-home support (#6590) Signed-off-by: Luke Tornquist <tornquist@squareup.com>
…agent-edit * origin/main: (39 commits) chore(deps): update dependency vitest to v4.1.11 (#6667) chore(deps): update dependency @tanstack/react-virtual to v3.14.10 (#6666) chore(deps): update ubuntu:24.04 docker digest to 33ceb71 (#6664) fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) feat(projects): add agent and CLI project-home support (#6590) feat(desktop): restore message quick reactions (#6892) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…late-cardinality-hints * origin/main: (145 commits) chore(deps): update rui314/setup-mold digest to 7e4f20a (#6663) chore(deps): update dependency vitest to v4.1.11 (#6667) chore(deps): update dependency @tanstack/react-virtual to v3.14.10 (#6666) chore(deps): update ubuntu:24.04 docker digest to 33ceb71 (#6664) fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) feat(projects): add agent and CLI project-home support (#6590) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…c-agent-commit-identity * origin/main: chore(deps): update rui314/setup-mold digest to 7e4f20a (#6663) chore(deps): update dependency vitest to v4.1.11 (#6667) chore(deps): update dependency @tanstack/react-virtual to v3.14.10 (#6666) chore(deps): update ubuntu:24.04 docker digest to 33ceb71 (#6664) fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
* origin/main: (21 commits) chore(deps): update rui314/setup-mold digest to 7e4f20a (#6663) chore(deps): update dependency vitest to v4.1.11 (#6667) chore(deps): update dependency @tanstack/react-virtual to v3.14.10 (#6666) chore(deps): update ubuntu:24.04 docker digest to 33ceb71 (#6664) fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) feat(projects): add agent and CLI project-home support (#6590) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
* origin/main: (38 commits) fix(db): disable heartbeat vacuum truncation (#6898) chore(deps): update rui314/setup-mold digest to 7e4f20a (#6663) chore(deps): update dependency vitest to v4.1.11 (#6667) chore(deps): update dependency @tanstack/react-virtual to v3.14.10 (#6666) chore(deps): update ubuntu:24.04 docker digest to 33ceb71 (#6664) fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) ... Signed-off-by: Sol <478bb5a31222ea2b28a3d1afb8b1d598940628f19c2a87efc3c4b822299eeec6@buzz.block.builderlab.xyz> # Conflicts: # desktop/src/features/channels/ui/ChannelPane.tsx # desktop/src/features/channels/ui/ChannelPane.types.ts # desktop/src/features/channels/ui/ChannelScreen.tsx
…cp-sessions * origin/main: test(db): use canonical channel roster fixtures (#6819) preserve channel description paragraph breaks (#6946) fix(cli): enrich template cardinality error with per-candidate presence and profile hints (#4825) Fix Codex security review authorization (#6913) fix(db): disable heartbeat vacuum truncation (#6898) chore(deps): update rui314/setup-mold digest to 7e4f20a (#6663) chore(deps): update dependency vitest to v4.1.11 (#6667) chore(deps): update dependency @tanstack/react-virtual to v3.14.10 (#6666) chore(deps): update ubuntu:24.04 docker digest to 33ceb71 (#6664) fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) Signed-off-by: Salman Mohammed <smohammed@squareup.com>
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:
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 unmodifiedmain. 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
mainafter merge, and lands before PR 2 of the stack.