From 7b2e207957533a8b75ec4d87e515fa720f585333 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 18:38:12 -0700 Subject: [PATCH] Harden the broker contract against review 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 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- crates/buzz-sdk/src/broker/actions.rs | 303 ++++--- crates/buzz-sdk/src/broker/client.rs | 82 +- crates/buzz-sdk/src/broker/mod.rs | 175 +++- crates/buzz-sdk/src/broker/tests.rs | 1161 ++++++++++++++----------- 4 files changed, 1073 insertions(+), 648 deletions(-) diff --git a/crates/buzz-sdk/src/broker/actions.rs b/crates/buzz-sdk/src/broker/actions.rs index ee51a3a860..d440f7a40b 100644 --- a/crates/buzz-sdk/src/broker/actions.rs +++ b/crates/buzz-sdk/src/broker/actions.rs @@ -1,22 +1,19 @@ //! Broker actions — the closed set of operations an agent may ask a host to perform. //! -//! An action is the unit of policy. `message.post` and `agents.delete` are -//! separate names so a host can permit one without permitting the other, and so -//! a later policy layer has something per-operation to attach to. There is -//! deliberately no `sign` action and no `publish` action: an interface that can -//! sign arbitrary bytes is a signing oracle, and a host holding it could not -//! reason about what it authorized. +//! An action is the unit of policy; see the [module docs](super) for why this is +//! an operation enum rather than a signing primitive. //! //! Two consequences of that choice are visible in the types below. Every args -//! type is `deny_unknown_fields`, so a credential or environment map smuggled -//! into a payload fails to deserialize instead of reaching an executor. Every -//! outcome type can structurally hold only public identifiers — no field of any -//! outcome can transport key material. +//! and outcome type is `deny_unknown_fields`, so a credential or environment map +//! smuggled into a payload fails to deserialize instead of reaching an executor. +//! And the wire key set of every type is pinned by test, so a secret-bearing +//! field cannot be added without a test failing. use serde::{Deserialize, Serialize}; use crate::SdkError; use buzz_core::engram::validate_slug; +use nostr::Event; /// Maximum characters in a display name or agent name. pub const MAX_NAME_CHARS: usize = 120; @@ -42,6 +39,9 @@ pub const MAX_MENTIONS: usize = 50; /// Maximum events a single read may return. pub const MAX_PAGE_LIMIT: u32 = 500; +/// Maximum accepted length of a read cursor, in bytes. +pub const MAX_CURSOR_LEN: usize = 256; + /// Inbound author gate modes a requester may ask for. /// /// `allowlist` is deliberately absent: it needs a pubkey list this request @@ -53,8 +53,7 @@ pub const RESPOND_TO_MODES: [&str; 2] = ["owner-only", "anyone"]; /// /// #6467 asks for identity to be separable from signing. This type is that /// separation made structural: it holds 64 hex characters of public key and has -/// no counterpart in this module for the corresponding secret. Nothing here can -/// name, hold, or ask for one. +/// no counterpart in this module for the corresponding secret. #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(try_from = "String", into = "String")] pub struct PubkeyHex(String); @@ -164,11 +163,9 @@ impl Action { /// Whether a host may refuse this action without harming the agent. /// - /// #6467 requires non-essential signed housekeeping to be skippable, so - /// that an agent can still run where it is unavailable. For a best-effort - /// action, [`super::BrokerErrorCode::Unsupported`] is a normal answer and a - /// caller must carry on. For every other action, `Unsupported` means the - /// agent cannot do its job on this host and should say so out loud. + /// #6467 requires non-essential signed housekeeping to be skippable, so an + /// agent can still run where it is unavailable. See + /// [`super::BrokerErrorCode::Unsupported`] for how a caller reacts. #[must_use] pub fn is_best_effort(self) -> bool { matches!(self, Self::ReactionAdd) @@ -194,16 +191,10 @@ impl Action { /// Reads are actions like any other, because #6467 requires a host with no /// relay route of its own to be able to serve them. /// -/// One action covers three scopes, because they differ only by filter and a -/// separate name per scope would be a separate policy decision for the same -/// underlying permission — *may this agent see this channel*. `rootEventId` -/// narrows to one thread; `mentionsOnly` narrows to messages that mention the -/// requester, which is the wake path. -/// -/// `since` is an inclusive lower bound in Unix seconds, matching the relay's -/// filter granularity. A caller must de-duplicate by event id: second-precision -/// cursors can straddle events, so consecutive pages may overlap. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +/// One action covers channel, thread, and mention-feed scope, because they +/// differ only by filter and a name per scope would split one permission — +/// *may this agent see this channel* — across three policy decisions. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct ChannelReadArgs { /// Channel to read. @@ -211,68 +202,45 @@ pub struct ChannelReadArgs { /// Narrow to one thread by its root event. #[serde(default, skip_serializing_if = "Option::is_none")] pub root_event_id: Option, - /// Narrow to messages mentioning the requester. + /// Narrow to messages mentioning the requester — the wake path. /// - /// The requester is never named. The host answers for the identity it - /// authenticated; a body that could ask "what mentions *someone else*" - /// would be a body that picks its own subject. + /// The requester is never named; see [`super::BrokerRequest`] on why no body + /// names its own subject. #[serde(default, skip_serializing_if = "is_false")] pub mentions_only: bool, - /// Inclusive lower bound, Unix seconds. Absent = host's default window. + /// Opaque position to resume from, as returned in [`MessagePage::next_cursor`]. + /// + /// Absent on a first read, which starts at the host's default window. A + /// cursor is **opaque**: callers must round-trip it verbatim and must not + /// parse, compare, or synthesize one. That is deliberate — a timestamp + /// cursor cannot page safely when more events than `limit` share one + /// second, and it would commit every future host to one relay ordering + /// strategy. The host defines ordering and cursor stability, including + /// whether a cursor stays valid across restarts. #[serde(default, skip_serializing_if = "Option::is_none")] - pub since: Option, + pub cursor: Option, /// Maximum events to return, capped at [`MAX_PAGE_LIMIT`]. #[serde(default, skip_serializing_if = "Option::is_none")] pub limit: Option, } impl ChannelReadArgs { - /// Read a whole channel. + /// Read a whole channel from the host's default window. #[must_use] pub fn channel(channel_id: impl Into) -> Self { Self { channel_id: channel_id.into(), - root_event_id: None, - mentions_only: false, - since: None, - limit: None, + ..Self::default() } } - /// Narrow this read to one thread. - #[must_use] - pub fn in_thread(mut self, root_event_id: impl Into) -> Self { - self.root_event_id = Some(root_event_id.into()); - self - } - - /// Narrow this read to messages mentioning the requester. - #[must_use] - pub fn mentions_only(mut self) -> Self { - self.mentions_only = true; - self - } - - /// Start this read at `since` (inclusive, Unix seconds). - #[must_use] - pub fn since(mut self, since: u64) -> Self { - self.since = Some(since); - self - } - - /// Cap this read at `limit` events. - #[must_use] - pub fn limit(mut self, limit: u32) -> Self { - self.limit = Some(limit); - self - } - /// Validate and normalize. /// /// # Errors /// /// Returns [`SdkError::InvalidInput`] for a malformed channel UUID, a - /// malformed root event id, or an out-of-range limit. + /// malformed root event id, an over-long or non-printable cursor, or an + /// out-of-range limit. pub fn validated(&self) -> Result { Ok(Self { channel_id: channel(&self.channel_id)?, @@ -282,7 +250,7 @@ impl ChannelReadArgs { .map(|id| event_id(id, "rootEventId")) .transpose()?, mentions_only: self.mentions_only, - since: self.since, + cursor: self.cursor.as_deref().map(cursor).transpose()?, limit: limit(self.limit)?, }) } @@ -434,10 +402,9 @@ impl ProfileSetArgs { /// Arguments for `storage.address`. /// -/// Encrypted-memory records are addressed by a tag derived from a conversation -/// key, so deriving one needs the secret this contract exists to avoid holding. -/// #6467 lists that derivation among the operations to route through the -/// interface, which is why it is an action rather than a local computation. +/// Deriving a record's address needs the secret this contract exists to avoid +/// holding, which is why #6467 lists it among the operations to route through +/// the interface rather than compute locally. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct StorageAddressArgs { @@ -461,10 +428,8 @@ impl StorageAddressArgs { // ── Agent arguments ───────────────────────────────────────────────────────── -/// Which agent an update or delete targets. -/// -/// Exactly one selector, because a request naming the target twice is ambiguous -/// and the host would have to guess which wins. +/// Which agent an update or delete targets — exactly one selector, so a host +/// never has to guess which of two names wins. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum AgentTarget { @@ -490,10 +455,9 @@ impl AgentTarget { /// Arguments for `agents.create`. /// -/// There is no owner field. The owner of a created agent is whoever the host -/// authenticated for this request — an agent creating an agent owns it, and the -/// chain of ownership ends at a human. A request that could name its own -/// authority would let any caller mint agents under someone else. +/// There is no owner field: the owner is whoever the host authenticated, so an +/// agent creating an agent owns it. See the [module docs](super) on ownership +/// recursion, and [`super::BrokerRequest`] on why no body names its own subject. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct AgentsCreateArgs { @@ -707,48 +671,88 @@ impl ActionArgs { // ── Outcomes ──────────────────────────────────────────────────────────────── -/// One message returned by a read. +/// One message returned by a read: the signed Nostr event, verbatim. /// -/// A projection, not a Nostr event: no signature, no tag array. A caller that -/// needs to verify signatures needs the relay, which is exactly what a -/// broker-only deployment does not have. Trust here is trust in the host. +/// The event is authoritative. It is carried whole — signature and tags +/// included — rather than reduced to a projection, because Schnorr verification +/// is local and needs no relay: see [`Self::verify`]. A keyless agent therefore +/// still gets independently verifiable authorship and content, and only has to +/// trust the host for *completeness* (that no message was withheld) and for +/// authorization. Reducing to a projection would have widened that trust to +/// cover authorship too, and dropped the tags later consumers need. +/// +/// The wire form is the event's own JSON, so this adds no envelope of its own. +/// Ancestry and mentions are exposed as derived accessors rather than sibling +/// fields, so there is nothing that can disagree with the signed bytes. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct BrokerMessage { - /// Event id (hex). - pub event_id: String, - /// Author's pubkey. - pub author_pubkey: PubkeyHex, - /// Event kind. - pub kind: u32, - /// Creation time, Unix seconds. - pub created_at: u64, - /// Message body. - pub content: String, - /// Thread root, when this message is threaded. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub root_event_id: Option, - /// Immediate parent, when this message is a reply. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub parent_event_id: Option, - /// Pubkeys this message mentions. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub mentions: Vec, +#[serde(transparent)] +pub struct BrokerMessage(pub Event); + +impl BrokerMessage { + /// The signed event. + #[must_use] + pub fn event(&self) -> &Event { + &self.0 + } + + /// Verify the event's id and Schnorr signature. + /// + /// This is the caller's provenance check and it is entirely local. A host + /// that fabricated or altered a message fails here regardless of what it + /// claims. It is deliberately *not* called by + /// [`super::BrokerResponse::validate_for`]: whether to pay for verification, + /// and what to do when it fails, is the caller's policy. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] when the id does not match the + /// content or the signature does not match the author. + pub fn verify(&self) -> Result<(), SdkError> { + self.0.verify().map_err(|e| { + SdkError::InvalidInput(format!("broker returned an unverifiable event: {e}")) + }) + } + + /// The author's pubkey, in this contract's identity type. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] if the event's author is not + /// expressible as 64 hex characters. + pub fn author(&self) -> Result { + PubkeyHex::parse(self.0.pubkey.to_hex()) + } + + /// NIP-10 `root`/`reply` ancestry, parsed from the signed tags. + #[must_use] + pub fn thread(&self) -> buzz_core::nip10::ThreadMarkers { + buzz_core::nip10::parse_thread_markers(&self.0.tags) + } + + /// Pubkeys this message mentions, from the signed `p` tags. + #[must_use] + pub fn mentions(&self) -> Vec { + self.0 + .tags + .public_keys() + .map(nostr::PublicKey::to_hex) + .collect() + } } /// Outcome of any read action. -/// -/// `nextCursor` is the value to pass as `since` on the following call. It is -/// absent when the host has nothing further, which is how a caller learns to -/// stop rather than by comparing lengths against a limit it may not have set. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct MessagePage { - /// Messages in ascending `createdAt` order. + /// Messages in the host's declared order. pub messages: Vec, - /// Cursor for the next page, if more may exist. + /// Opaque cursor to pass as [`ChannelReadArgs::cursor`] on the next call. + /// + /// Absent when the host has nothing further, which is how a caller learns + /// to stop rather than by comparing lengths against a limit it may not have + /// set. #[serde(default, skip_serializing_if = "Option::is_none")] - pub next_cursor: Option, + pub next_cursor: Option, } /// Outcome of an action that published one event. @@ -780,9 +784,10 @@ pub struct StorageAddress { /// Outcome of a successful `agents.create`. /// -/// Carries the new agent's **public** key only. There is no field for the -/// minted secret: it stays on the host, and this type could not transport it -/// even if a handler tried. +/// Carries the new agent's **public** key only: pubkey, name, channel, and +/// nothing else. There is no field for the minted secret — it stays on the +/// host — and `deny_unknown_fields` plus a test pinning this exact key set is +/// what enforces that rather than a comment. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct AgentsCreateOutcome { @@ -865,6 +870,50 @@ impl ActionOutcome { Self::AgentsDelete(_) => Action::AgentsDelete, } } + + /// Validate the identifiers and cursors this outcome asserts. + /// + /// A well-typed outcome can still carry a malformed id or an unusable + /// cursor, and a caller that trusted either would fail later and further + /// away. Signature verification is deliberately *not* here — that is + /// [`BrokerMessage::verify`], and whether to pay for it is the caller's + /// choice. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed event id, `d` tag, + /// channel UUID, or cursor, an empty name, or an over-long page. + pub fn validate(&self) -> Result<(), SdkError> { + match self { + Self::ChannelRead(page) => { + if page.messages.len() > MAX_PAGE_LIMIT as usize { + return Err(SdkError::InvalidInput(format!( + "page holds {} messages, over the {MAX_PAGE_LIMIT} cap", + page.messages.len() + ))); + } + page.next_cursor.as_deref().map(cursor).transpose()?; + } + Self::MessagePost(published) + | Self::MessageReply(published) + | Self::ReactionAdd(published) + | Self::ProfileSet(published) => { + event_id(&published.event_id, "eventId")?; + } + Self::StorageAddress(address) => { + event_id(&address.d_tag, "dTag")?; + } + Self::AgentsCreate(outcome) => { + channel(&outcome.channel_id)?; + required(&outcome.display_name, "display name", MAX_NAME_CHARS)?; + } + Self::AgentsUpdate(AgentsUpdateOutcome { display_name, .. }) + | Self::AgentsDelete(AgentsDeleteOutcome { display_name, .. }) => { + required(display_name, "display name", MAX_NAME_CHARS)?; + } + } + Ok(()) + } } fn is_false(value: &bool) -> bool { @@ -941,6 +990,30 @@ fn limit(value: Option) -> Result, SdkError> { } } +/// Validate an opaque read cursor: printable ASCII, bounded, never parsed. +/// +/// The bound exists so a host cannot be made to store an unbounded token; the +/// character set keeps it safe to log. Nothing here interprets the value. +fn cursor(value: &str) -> Result { + if value.is_empty() { + return Err(SdkError::InvalidInput( + "cursor must not be empty (omit it to start from the host's default window)".into(), + )); + } + if value.len() > MAX_CURSOR_LEN { + return Err(SdkError::InvalidInput(format!( + "cursor exceeds {MAX_CURSOR_LEN} bytes (got {})", + value.len() + ))); + } + if !value.bytes().all(|b| (0x21..=0x7e).contains(&b)) { + return Err(SdkError::InvalidInput( + "cursor must be printable ASCII without spaces".into(), + )); + } + Ok(value.to_owned()) +} + fn respond_to(value: Option<&String>) -> Result, SdkError> { let value = optional(value, "respond-to", MAX_SCALAR_CHARS)?; if let Some(mode) = value.as_deref() { diff --git a/crates/buzz-sdk/src/broker/client.rs b/crates/buzz-sdk/src/broker/client.rs index f161e4d518..3793e43672 100644 --- a/crates/buzz-sdk/src/broker/client.rs +++ b/crates/buzz-sdk/src/broker/client.rs @@ -7,21 +7,21 @@ //! Authorization: Bearer //! Content-Type: application/json //! -//! +//! //! ``` //! //! The response body is a [`BrokerResponse`] as JSON. Every terminal -//! disposition — including [`super::BrokerResult::Failed`] — is a well-formed -//! envelope, so a host that has an answer returns `200` and puts the verdict in -//! `status`. HTTP status codes carry no information the envelope does not, and a -//! client must read the envelope rather than the status line. +//! disposition the *host* reached — including [`super::BrokerResult::Failed`], +//! and including a rejected credential — is a well-formed envelope returned with +//! `200`, because the verdict lives in `status` and a second copy in the status +//! line could only ever disagree with it. //! -//! A non-2xx response is only meaningful when there is *no* envelope: the -//! credential was rejected before dispatch, the path does not exist, or an -//! intermediary answered instead of the host. Those surface as -//! [`BrokerTransportError`], which is not a `Failed` result — it means no answer -//! arrived, so the caller learned nothing about side effects and may retry with -//! identical bytes per the [`BrokerRequest`] retry contract. +//! A client must nonetheless **attempt to parse an envelope regardless of HTTP +//! status**. A host or intermediary may map dispositions onto conventional +//! statuses for observability or for middleware that cannot read bodies; if a +//! valid envelope is present, it is the answer and the status line is +//! decoration. Only when no envelope can be parsed does the status matter, and +//! then only as detail for an operator: see [`BrokerTransportError`]. //! //! # The credential //! @@ -29,6 +29,12 @@ //! received at startup and can only replay; it is not a key, not a signature, //! and not derived from anything the agent holds. //! +//! A rejected credential is a host verdict, not a transport failure: it arrives +//! as `Failed` with [`super::BrokerErrorCode::Unauthenticated`], which carries +//! the promise every `Failed` carries — the action did not run. Modelling it as +//! a transport error would have thrown that knowledge away and told the caller +//! to reconcile something that provably never happened. +//! //! Binding a credential to a specific (agent, conversation) pair — so that a //! leaked token cannot act as a different agent or outside the conversation it //! was issued for — **is the host's concern**. This contract documents the @@ -43,7 +49,7 @@ use std::future::Future; use std::pin::Pin; -use super::{BrokerRequest, BrokerResponse}; +use super::{BrokerResponse, PreparedRequest}; /// Path of the single broker endpoint. pub const BROKER_ACTION_PATH: &str = "/v1/action"; @@ -51,20 +57,38 @@ pub const BROKER_ACTION_PATH: &str = "/v1/action"; /// Header carrying the opaque session credential, as `Bearer `. pub const BROKER_CREDENTIAL_HEADER: &str = "authorization"; -/// A transport-level failure: no [`BrokerResponse`] was obtained. +/// No usable [`BrokerResponse`] was obtained, so the request's fate is unknown. /// -/// Distinct from [`super::BrokerResult::Failed`], which is a host's considered -/// verdict. These variants all mean the same thing to a caller — the request's -/// fate is unknown — and differ only in what to tell an operator. +/// Every variant means the same thing to a caller: nothing can be concluded +/// about side effects, and the only safe next step is to retry the identical +/// bytes (which the host will deduplicate) or to reconcile by reading state. +/// The variants differ only in what to tell an operator. +/// +/// Host verdicts never appear here — not even a rejected credential, which is +/// [`super::BrokerErrorCode::Unauthenticated`] inside a `Failed` envelope. This +/// type is strictly for the absence of an answer. #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] pub enum BrokerTransportError { /// The host could not be reached, or the connection failed mid-request. #[error("broker unreachable: {0}")] Unreachable(String), - /// The credential was rejected before the host produced an envelope. - #[error("broker rejected the session credential")] - CredentialRejected, - /// A response arrived but was not a well-formed [`BrokerResponse`]. + /// An HTTP response arrived carrying no parseable envelope. + /// + /// Typically an intermediary answering instead of the host: a proxy `401`, + /// a `404` for a missing route, a `502`. The status is recorded for + /// operators and carries no contractual meaning — an intermediary's `401` + /// does not prove the host never ran the action. + #[error("no broker envelope in HTTP {status} response: {detail}")] + NoEnvelope { + /// The HTTP status observed. + status: u16, + /// Operator-facing detail about what arrived instead. + detail: String, + }, + /// An envelope arrived but did not validate against the request that was + /// sent — wrong `requestId`, wrong action, or a malformed outcome. + /// + /// See [`BrokerResponse::validate_for`]. #[error("malformed broker response: {0}")] MalformedResponse(String), } @@ -83,14 +107,22 @@ pub type BrokerFuture<'a> = /// [`super::Action`] inside the request, so adding an action never changes this /// trait and an implementation never needs updating to carry one. /// +/// The argument is a [`PreparedRequest`] rather than a typed request so that an +/// implementation cannot reserialize between attempts; see the +/// [`super::BrokerRequest`] retry contract. +/// /// Implementations must be usable as `dyn BrokerClient`. pub trait BrokerClient: Send + Sync { - /// Execute `request` and return the host's terminal response. + /// Send `request`'s frozen bytes and return the host's terminal response. + /// + /// Implementations should call [`BrokerResponse::validate_for`] before + /// returning `Ok`, so a caller never has to consider a response that does + /// not answer its request. /// /// # Errors /// - /// Returns [`BrokerTransportError`] when no response could be obtained. - /// A host that answered — even to refuse — returns `Ok` with the verdict in - /// [`BrokerResponse::result`]. - fn execute<'a>(&'a self, request: &'a BrokerRequest) -> BrokerFuture<'a>; + /// Returns [`BrokerTransportError`] when no usable response could be + /// obtained. A host that answered — even to refuse — returns `Ok` with the + /// verdict in [`BrokerResponse::result`]. + fn execute<'a>(&'a self, request: &'a PreparedRequest) -> BrokerFuture<'a>; } diff --git a/crates/buzz-sdk/src/broker/mod.rs b/crates/buzz-sdk/src/broker/mod.rs index 48645de288..925b26fc51 100644 --- a/crates/buzz-sdk/src/broker/mod.rs +++ b/crates/buzz-sdk/src/broker/mod.rs @@ -38,14 +38,28 @@ //! //! | #6467 requirement | Where | //! |---|---| -//! | Identity separable from signing; pubkey-only identity | [`PubkeyHex`] is the only identity type. No secret-key type exists in this module, and no args or outcome field can hold one. | -//! | Secret-dependent operations funnelled through the interface | Event signing is subsumed by the write actions (`message.post`, `message.reply`, `reaction.add`, `profile.set`, `agents.*`) — the agent states intent, the host signs. Encrypted-storage address derivation is [`Action::StorageAddress`]. NIP-42 relay auth, NIP-44 encryption, and NIP-98 request auth are **not** actions: they are mechanisms internal to whoever holds the key, and this contract never asks for them by name. | -//! | Relay auth is a control-flow choice | Nothing here mentions relay authentication. An agent using this contract does not authenticate to a relay, so there is no local auth step to skip. | +//! | Identity separable from signing; pubkey-only identity | [`PubkeyHex`] is the only identity type; no secret-key type exists in this module. | +//! | Secret-dependent operations funnelled through the interface | Event signing is subsumed by the write actions (`message.post`, `message.reply`, `reaction.add`, `profile.set`, `agents.*`) — the agent states intent, the host signs. Encrypted-storage address derivation is [`Action::StorageAddress`]. NIP-42 relay auth, NIP-44 encryption, and NIP-98 request auth are **not** actions: they are mechanisms internal to whoever holds the key. | +//! | Relay auth is a control-flow choice | Nothing here mentions relay authentication, so there is no local auth step to skip. | //! | Every relay-touching op, reads included, routable | [`Action::ChannelRead`] covers channel, thread, and mention-feed reads. No action assumes the caller can reach a relay. | //! | Non-essential housekeeping skippable | [`Action::is_best_effort`] marks such actions; a host answers [`BrokerErrorCode::Unsupported`] and the agent carries on. | //! | No secret leaks to children via env | No args type carries an environment map, and `deny_unknown_fields` means one cannot be smuggled in. Process spawning is a host concern this contract cannot express. | //! | Lives in the shared client layer | `buzz-sdk`, which the CLI and the harness already depend on. | //! +//! # The no-secret rule +//! +//! No secret key material crosses this boundary in either direction. What +//! enforces it is the wire schema, not a comment: every args and outcome type is +//! `deny_unknown_fields`, and tests pin each type's exact key set, so a +//! secret-bearing field cannot be added without a test failing. +//! [`AgentsCreateOutcome`] is the case that matters — it returns public identity +//! only, never the key it just minted. +//! +//! Two limits are worth stating. A `String` field can physically hold secret +//! text, so keeping secrets out of message content and error messages is host +//! policy this contract cannot enforce. And nothing stops a host from *holding* +//! keys — that is the point; it stops one from handing them over. +//! //! # Ownership recursion //! //! [`Action::AgentsCreate`] has no owner field. The owner of a created agent is @@ -115,23 +129,30 @@ pub const MAX_REQUEST_ID_LEN: usize = 128; /// /// # What this envelope deliberately omits /// -/// There is no requester, owner, or relay field. Those are derived by the host -/// from the authenticated session credential. A body that could name its own -/// subject would let any caller act as anyone. +/// There is no requester, owner, scope, or relay field. Those are derived by the +/// host from the authenticated session credential. **A body that could name its +/// own subject would let any caller act as anyone** — that one rule is why +/// `channel.read` cannot ask about another identity's mentions, why +/// `profile.set` has no subject, and why `agents.create` has no owner. /// /// # Retry contract /// -/// Retrying means **resending the identical serialized request** with the same -/// `requestId`. The host hashes what it receives and compares that digest -/// against the digest recorded under the same idempotency key: +/// Retrying means resending the identical serialized request with the same +/// `requestId`, which is why a client never sends this type directly: call +/// [`Self::prepare`] to freeze it into a [`PreparedRequest`] and hand *that* to +/// [`BrokerClient::execute`]. The host hashes the bytes it receives and compares +/// that digest against the digest recorded under the same idempotency key: /// /// - same key, same digest → the recorded outcome is replayed, nothing re-runs /// - same key, different digest → rejected as a request-ID conflict /// -/// Callers must therefore not regenerate, re-order, or re-render the payload -/// between attempts. There is no client-computed digest field: idempotency is -/// decided host-side, and a caller-supplied digest would be a claim the host -/// has to recompute anyway. +/// A typed value cannot carry that guarantee. Two serializations of one value +/// can differ in bytes across serde versions or implementations, and the +/// difference would surface as a spurious [`BrokerErrorCode::RequestIdConflict`] +/// on retry. Serializing once removes the possibility rather than warning about +/// it. There is no client-computed digest field: idempotency is decided +/// host-side, and a caller-supplied digest would be a claim the host has to +/// recompute anyway. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(deny_unknown_fields, rename_all = "camelCase")] pub struct BrokerRequest { @@ -174,6 +195,23 @@ impl BrokerRequest { self.action.action() } + /// Validate, then serialize once into the bytes every attempt will send. + /// + /// # Errors + /// + /// Returns [`SdkError`] when [`Self::validate`] fails, or + /// [`SdkError::InvalidInput`] if serialization fails. + pub fn prepare(self) -> Result { + self.validate()?; + let body = serde_json::to_vec(&self).map_err(|e| { + SdkError::InvalidInput(format!("broker request is not serializable: {e}")) + })?; + Ok(PreparedRequest { + request: self, + body, + }) + } + /// Validate every field the host must agree on before executing anything. /// /// Callers validate before sending; the host revalidates on receipt, @@ -242,8 +280,48 @@ pub fn validate_request_id(request_id: &str) -> Result<(), SdkError> { Ok(()) } -/// Machine-readable broker error code. +/// A validated request together with the exact bytes to send. /// +/// This is what [`BrokerClient::execute`] takes, so the retry contract is +/// structural rather than documented: the first attempt and every retry send +/// `body` verbatim, and no implementation gets the chance to reserialize. +/// +/// Construct one with [`BrokerRequest::prepare`]. The typed [`BrokerRequest`] is +/// kept alongside so a client can correlate the response without reparsing — +/// see [`BrokerResponse::validate_for`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PreparedRequest { + request: BrokerRequest, + body: Vec, +} + +impl PreparedRequest { + /// The frozen JSON body. Every attempt sends exactly these bytes. + #[must_use] + pub fn body(&self) -> &[u8] { + &self.body + } + + /// The typed request these bytes encode. + #[must_use] + pub fn request(&self) -> &BrokerRequest { + &self.request + } + + /// The idempotency key the host keys replay on. + #[must_use] + pub fn request_id(&self) -> &str { + &self.request.request_id + } + + /// The action being invoked. + #[must_use] + pub fn action(&self) -> Action { + self.request.action() + } +} + +/// Machine-readable broker error code. /// These name failures the *broker* is responsible for. Failures inside an /// action arrive as [`BrokerErrorCode::ActionFailed`] with detail in the /// message. @@ -265,6 +343,10 @@ pub enum BrokerErrorCode { /// on this host. Unsupported, /// The session credential was missing, malformed, or rejected. + /// + /// A host verdict, delivered as [`BrokerResult::Failed`], never as a + /// transport error: the request was refused before execution, so the caller + /// knows no side effects occurred. Unauthenticated, /// The requester is authenticated but not permitted this action. Unauthorized, @@ -456,10 +538,16 @@ impl BrokerResponse { /// Validate discriminator, version, and request id. /// + /// This checks only what a response asserts about itself. It cannot tell + /// whether the response answers the request that was sent — for that, and + /// for outcome-field validation, use [`Self::validate_for`]. A client should + /// always prefer `validate_for`. + /// /// # Errors /// /// Returns [`SdkError::InvalidInput`] on a wrong `type`, an unsupported - /// `protocolVersion`, or a malformed `requestId`. + /// `protocolVersion`, a malformed `requestId`, an outcome with malformed + /// identifiers, or an error code paired with the wrong status. pub fn validate(&self) -> Result<(), SdkError> { if self.r#type != BROKER_RESULT_TYPE { return Err(SdkError::InvalidInput(format!( @@ -473,7 +561,62 @@ impl BrokerResponse { self.protocol_version ))); } - validate_request_id(&self.request_id) + validate_request_id(&self.request_id)?; + match &self.result { + BrokerResult::Succeeded { outcome } => outcome.validate()?, + // `OutcomeUnknown` is the one code that describes not knowing + // whether side effects landed. Paired with `Failed` — which promises + // they did not — it would be self-contradictory, so it is rejected + // rather than left for each caller to notice. + BrokerResult::Failed { error } if error.code == BrokerErrorCode::OutcomeUnknown => { + return Err(SdkError::InvalidInput( + "outcome_unknown is only valid with an indeterminate status".into(), + )); + } + BrokerResult::Failed { .. } | BrokerResult::Indeterminate { .. } => {} + } + Ok(()) + } + + /// Validate this response *as the answer to `request`*. + /// + /// A response that validates in isolation can still be the wrong answer: a + /// host (or a confused proxy) could return a `message.post` success to a + /// `channel.read`, and a caller matching on the outcome enum would quietly + /// take the wrong branch. This is the check that makes such a response + /// unusable instead of merely surprising, which is why + /// [`BrokerClient::execute`] implementations are expected to run it before + /// returning `Ok` and to report a failure as + /// [`BrokerTransportError::MalformedResponse`]. + /// + /// Signature verification of read results is deliberately not included; see + /// [`BrokerMessage::verify`]. + /// + /// # Errors + /// + /// Returns everything [`Self::validate`] returns, plus + /// [`SdkError::InvalidInput`] when the `requestId` does not correlate or a + /// success outcome names a different action than the request. + pub fn validate_for(&self, request: &PreparedRequest) -> Result<(), SdkError> { + self.validate()?; + if self.request_id != request.request_id() { + return Err(SdkError::InvalidInput(format!( + "response requestId \"{}\" does not match request \"{}\"", + self.request_id, + request.request_id() + ))); + } + if let BrokerResult::Succeeded { outcome } = &self.result { + let expected = request.action(); + if outcome.action() != expected { + return Err(SdkError::InvalidInput(format!( + "response carries a {} outcome for a {} request", + outcome.action().as_str(), + expected.as_str() + ))); + } + } + Ok(()) } } diff --git a/crates/buzz-sdk/src/broker/tests.rs b/crates/buzz-sdk/src/broker/tests.rs index a362cd2da3..05aef211cd 100644 --- a/crates/buzz-sdk/src/broker/tests.rs +++ b/crates/buzz-sdk/src/broker/tests.rs @@ -1,6 +1,7 @@ //! Contract tests for the broker envelope, actions, and client trait. use super::*; +use nostr::{EventBuilder, Keys, Kind, Tag}; const CHANNEL: &str = "b2c38ca8-9ec3-411e-bab5-f9deab34d52e"; const PUBKEY: &str = "a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971"; @@ -10,17 +11,31 @@ fn pubkey() -> PubkeyHex { PubkeyHex::parse(PUBKEY).expect("fixture pubkey is valid hex") } +/// A genuinely signed event, so read fixtures exercise real verification rather +/// than a hand-built value that could never verify. +fn signed_message(keys: &Keys) -> BrokerMessage { + let event = EventBuilder::new(Kind::Custom(9), "hello") + .tags([ + Tag::parse(["h", CHANNEL]).expect("h tag"), + Tag::parse(["e", EVENT, "", "root"]).expect("e tag"), + Tag::parse(["p", PUBKEY]).expect("p tag"), + ]) + .sign_with_keys(keys) + .expect("fixture event signs"); + BrokerMessage(event) +} + /// One valid `args` value per action, so table-driven tests cannot silently -/// skip an action: [`action_fixtures_cover_every_action`] pins the coverage. +/// skip an action: [`fixtures_cover_every_action`] pins the coverage. fn action_fixtures() -> Vec { vec![ - ActionArgs::ChannelRead( - ChannelReadArgs::channel(CHANNEL) - .in_thread(EVENT) - .mentions_only() - .since(1_764_000_000) - .limit(50), - ), + ActionArgs::ChannelRead(ChannelReadArgs { + channel_id: CHANNEL.into(), + root_event_id: Some(EVENT.into()), + mentions_only: true, + cursor: Some("opaque-host-cursor-v1".into()), + limit: Some(50), + }), ActionArgs::MessagePost(MessagePostArgs { channel_id: CHANNEL.into(), content: "shipping the contract".into(), @@ -30,7 +45,7 @@ fn action_fixtures() -> Vec { channel_id: CHANNEL.into(), reply_to_event_id: EVENT.into(), content: "agreed".into(), - mentions: vec![], + mentions: vec![pubkey()], }), ActionArgs::ReactionAdd(ReactionAddArgs { channel_id: CHANNEL.into(), @@ -40,7 +55,7 @@ fn action_fixtures() -> Vec { ActionArgs::ProfileSet(ProfileSetArgs { display_name: Some("ss-dev-00".into()), about: Some("implementation".into()), - picture: None, + picture: Some("https://example.invalid/avatar.png".into()), }), ActionArgs::StorageAddress(StorageAddressArgs { slug: "mem/broker-foundation".into(), @@ -49,19 +64,19 @@ fn action_fixtures() -> Vec { channel_id: CHANNEL.into(), display_name: "Research helper".into(), system_prompt: "Find sources.".into(), - runtime: None, - provider: None, - model: None, + runtime: Some("buzz-acp".into()), + provider: Some("anthropic".into()), + model: Some("claude-sonnet-4-5".into()), respond_to: Some("owner-only".into()), }), ActionArgs::AgentsUpdate(AgentsUpdateArgs { target: AgentTarget::Pubkey(pubkey()), display_name: Some("Research helper v2".into()), - system_prompt: None, - runtime: None, - provider: None, - model: None, - respond_to: None, + system_prompt: Some("Find better sources.".into()), + runtime: Some("buzz-acp".into()), + provider: Some("anthropic".into()), + model: Some("claude-sonnet-4-5".into()), + respond_to: Some("anyone".into()), }), ActionArgs::AgentsDelete(AgentsDeleteArgs { target: AgentTarget::Name("Research helper".into()), @@ -70,19 +85,10 @@ fn action_fixtures() -> Vec { } /// One outcome per action, matching the fixture order above. -fn outcome_fixtures() -> Vec { +fn outcome_fixtures(keys: &Keys) -> Vec { let page = MessagePage { - messages: vec![BrokerMessage { - event_id: EVENT.into(), - author_pubkey: pubkey(), - kind: 9, - created_at: 1_764_000_001, - content: "hello".into(), - root_event_id: Some(EVENT.into()), - parent_event_id: None, - mentions: vec![pubkey()], - }], - next_cursor: Some(1_764_000_002), + messages: vec![signed_message(keys)], + next_cursor: Some("opaque-host-cursor-v2".into()), }; let published = EventPublished { event_id: EVENT.into(), @@ -117,17 +123,37 @@ fn outcome_fixtures() -> Vec { ] } +fn prepared(args: ActionArgs) -> PreparedRequest { + BrokerRequest::new("req-1", args) + .expect("fixture request builds") + .prepare() + .expect("fixture request prepares") +} + +/// Sorted JSON object keys of `value`, for exact-schema assertions. +fn keys_of(value: &serde_json::Value) -> Vec { + let mut keys: Vec = value + .as_object() + .expect("expected a JSON object") + .keys() + .cloned() + .collect(); + keys.sort(); + keys +} + // ── Coverage ──────────────────────────────────────────────────────────────── /// The fixture tables are the input to every table-driven test below, so an /// action added without a fixture would be silently untested. This is the guard. #[test] -fn action_fixtures_cover_every_action() { +fn fixtures_cover_every_action() { + let keys = Keys::generate(); let mut from_args: Vec<&str> = action_fixtures() .iter() .map(|args| args.action().as_str()) .collect(); - let mut from_outcomes: Vec<&str> = outcome_fixtures() + let mut from_outcomes: Vec<&str> = outcome_fixtures(&keys) .iter() .map(|outcome| outcome.action().as_str()) .collect(); @@ -142,19 +168,10 @@ fn action_fixtures_cover_every_action() { from_outcomes, declared, "every action needs an outcome fixture" ); -} -#[test] -fn action_names_are_unique_and_round_trip_through_parse() { - let mut names: Vec<&str> = Action::ALL.iter().map(|a| a.as_str()).collect(); - let count = names.len(); - names.sort_unstable(); - names.dedup(); - assert_eq!(names.len(), count, "action wire names must be unique"); - - for action in Action::ALL { - assert_eq!(Action::parse(action.as_str()).unwrap(), action); - } + let mut unique = declared.clone(); + unique.dedup(); + assert_eq!(unique.len(), declared.len(), "wire names must be unique"); } // ── Envelope round-trip ───────────────────────────────────────────────────── @@ -192,7 +209,8 @@ fn every_action_round_trips_through_a_request_envelope() { #[test] fn every_outcome_round_trips_through_a_response_envelope() { - for outcome in outcome_fixtures() { + let signer = Keys::generate(); + for outcome in outcome_fixtures(&signer) { let action = outcome.action(); let response = BrokerResponse::new("req-1", BrokerResult::succeeded(outcome.clone())); response.validate().expect("response is valid"); @@ -230,12 +248,34 @@ fn an_args_shape_cannot_be_paired_with_another_action_name() { // ── Envelope rejection ────────────────────────────────────────────────────── +/// Unknown names must not resolve, and neither must the *mechanism* names this +/// contract deliberately refuses to expose: an interface that can sign arbitrary +/// bytes is a signing oracle. #[test] -fn unknown_action_name_is_rejected() { - for unknown in ["channel.write", "agents.exfiltrate", "", "channel.read "] { +fn only_declared_action_names_resolve() { + for action in Action::ALL { + assert_eq!(Action::parse(action.as_str()).unwrap(), action); + } + for rejected in [ + "channel.write", + "agents.exfiltrate", + "", + "channel.read ", + "sign", + "sign_event", + "publish", + "nip44.encrypt", + "nip44.decrypt", + "nip42.auth", + "nip98.auth", + "keys.export", + "identity.nsec", + "presence.set", + "typing.set", + ] { assert!( - Action::parse(unknown).is_err(), - "\"{unknown}\" must not parse as an action" + Action::parse(rejected).is_err(), + "\"{rejected}\" must not parse as an action" ); } @@ -250,83 +290,38 @@ fn unknown_action_name_is_rejected() { assert!(serde_json::from_value::(json).is_err()); } -/// Signing and publishing are mechanisms, not actions. An interface that can -/// sign arbitrary bytes is a signing oracle, so these names must not resolve. #[test] -fn signing_and_credential_access_are_not_actions() { - for forbidden in [ - "sign", - "sign_event", - "publish", - "nip44.encrypt", - "nip44.decrypt", - "nip42.auth", - "nip98.auth", - "keys.export", - "identity.nsec", - "tool.exec", - "agents.manage", - ] { - assert!( - Action::parse(forbidden).is_err(), - "\"{forbidden}\" must not be an action" - ); - } -} - -#[test] -fn unknown_protocol_version_is_rejected() { - let args = ActionArgs::AgentsDelete(AgentsDeleteArgs { - target: AgentTarget::Pubkey(pubkey()), - }); +fn envelope_metadata_must_match_this_protocol_version() { + let args = || { + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Pubkey(pubkey()), + }) + }; + let failed = || BrokerResult::failed(BrokerError::unsupported("no")); for bad in [0_u16, 2, 999] { - let mut request = BrokerRequest::new("req-1", args.clone()).unwrap(); + let mut request = BrokerRequest::new("req-1", args()).unwrap(); request.protocol_version = bad; let error = request.validate().unwrap_err().to_string(); assert!(error.contains("protocolVersion"), "unexpected: {error}"); - let mut response = BrokerResponse::new( - "req-1", - BrokerResult::failed(BrokerError::unsupported("no")), - ); + let mut response = BrokerResponse::new("req-1", failed()); response.protocol_version = bad; assert!(response.validate().is_err()); } -} -#[test] -fn unknown_action_version_is_rejected() { - let mut request = BrokerRequest::new( - "req-1", - ActionArgs::AgentsDelete(AgentsDeleteArgs { - target: AgentTarget::Pubkey(pubkey()), - }), - ) - .unwrap(); - request.action_version = 7; - let error = request.validate().unwrap_err().to_string(); + let mut wrong_action_version = BrokerRequest::new("req-1", args()).unwrap(); + wrong_action_version.action_version = 7; + let error = wrong_action_version.validate().unwrap_err().to_string(); assert!(error.contains("actionVersion"), "unexpected: {error}"); -} -#[test] -fn wrong_type_discriminator_is_rejected() { - let mut request = BrokerRequest::new( - "req-1", - ActionArgs::AgentsDelete(AgentsDeleteArgs { - target: AgentTarget::Pubkey(pubkey()), - }), - ) - .unwrap(); - request.r#type = BROKER_RESULT_TYPE.into(); - assert!(request.validate().is_err()); + let mut wrong_request_type = BrokerRequest::new("req-1", args()).unwrap(); + wrong_request_type.r#type = BROKER_RESULT_TYPE.into(); + assert!(wrong_request_type.validate().is_err()); - let mut response = BrokerResponse::new( - "req-1", - BrokerResult::failed(BrokerError::unsupported("no")), - ); - response.r#type = BROKER_REQUEST_TYPE.into(); - assert!(response.validate().is_err()); + let mut wrong_response_type = BrokerResponse::new("req-1", failed()); + wrong_response_type.r#type = BROKER_REQUEST_TYPE.into(); + assert!(wrong_response_type.validate().is_err()); } #[test] @@ -336,21 +331,161 @@ fn request_id_must_be_present_bounded_and_printable() { target: AgentTarget::Pubkey(pubkey()), }) }; - assert!(BrokerRequest::new("", args()).is_err()); + for (id, valid) in [ + ("", false), + ("has space", false), + ("has\nnewline", false), + ("has\u{7f}del", false), + ("req/1-a.b:c", true), + ] { + assert_eq!( + BrokerRequest::new(id, args()).is_ok(), + valid, + "requestId {id:?} validity" + ); + } assert!(BrokerRequest::new("a".repeat(MAX_REQUEST_ID_LEN), args()).is_ok()); assert!(BrokerRequest::new("a".repeat(MAX_REQUEST_ID_LEN + 1), args()).is_err()); - assert!(BrokerRequest::new("has space", args()).is_err()); - assert!(BrokerRequest::new("has\nnewline", args()).is_err()); - assert!(BrokerRequest::new("has\u{7f}del", args()).is_err()); - assert!(BrokerRequest::new("req/1-a.b:c", args()).is_ok()); +} + +// ── Wire schemas: the enforceable no-secret invariant ─────────────────────── + +/// The exact wire key set of every args and outcome type, with every optional +/// field populated so nothing escapes the pin by being absent. +/// +/// This table *is* the no-secret invariant. Combined with +/// `deny_unknown_fields`, it means no field — secret-bearing or otherwise — can +/// be added to this contract without a reviewer changing a line here. The +/// `agents.create` outcome is the case that matters: public identity only, never +/// the key the host just minted. +#[test] +fn every_payload_has_an_exact_and_secret_free_wire_schema() { + let signer = Keys::generate(); + let expected: Vec<(&str, Vec<&str>)> = vec![ + // Args, fully populated (optional fields present). + ( + "channel.read/args", + vec![ + "channelId", + "cursor", + "limit", + "mentionsOnly", + "rootEventId", + ], + ), + ( + "message.post/args", + vec!["channelId", "content", "mentions"], + ), + ( + "message.reply/args", + vec!["channelId", "content", "mentions", "replyToEventId"], + ), + ( + "reaction.add/args", + vec!["channelId", "reaction", "targetEventId"], + ), + ("profile.set/args", vec!["about", "displayName", "picture"]), + ("storage.address/args", vec!["slug"]), + ( + "agents.create/args", + vec![ + "channelId", + "displayName", + "model", + "provider", + "respondTo", + "runtime", + "systemPrompt", + ], + ), + ( + "agents.update/args", + vec![ + "displayName", + "model", + "provider", + "respondTo", + "runtime", + "systemPrompt", + "target", + ], + ), + ("agents.delete/args", vec!["target"]), + // Outcomes. + ("channel.read/outcome", vec!["messages", "nextCursor"]), + ("message.post/outcome", vec!["createdAt", "eventId", "kind"]), + ( + "message.reply/outcome", + vec!["createdAt", "eventId", "kind"], + ), + ("reaction.add/outcome", vec!["createdAt", "eventId", "kind"]), + ("profile.set/outcome", vec!["createdAt", "eventId", "kind"]), + ( + "storage.address/outcome", + vec!["authorPubkey", "dTag", "kind"], + ), + ( + "agents.create/outcome", + vec!["agentPubkey", "channelId", "displayName"], + ), + ( + "agents.update/outcome", + vec!["agentPubkey", "displayName", "updatedFields"], + ), + ("agents.delete/outcome", vec!["agentPubkey", "displayName"]), + ]; + + let mut actual: Vec<(String, Vec)> = Vec::new(); + for args in action_fixtures() { + let json = serde_json::to_value(&args).expect("args serialize"); + actual.push(( + format!("{}/args", args.action().as_str()), + keys_of(&json["args"]), + )); + } + for outcome in outcome_fixtures(&signer) { + let json = serde_json::to_value(&outcome).expect("outcome serializes"); + actual.push(( + format!("{}/outcome", outcome.action().as_str()), + keys_of(&json["outcome"]), + )); + } + + let expected: Vec<(String, Vec)> = expected + .into_iter() + .map(|(name, keys)| { + ( + name.to_string(), + keys.into_iter().map(str::to_string).collect(), + ) + }) + .collect(); + assert_eq!( + actual, expected, + "a payload's wire keys changed — confirm no field can carry key material" + ); + + // And no key anywhere in the contract even *looks* like secret material. + for (name, keys) in &actual { + for key in keys { + let lower = key.to_ascii_lowercase(); + for forbidden in ["secret", "private", "nsec", "seckey", "credential", "token"] { + assert!( + !lower.contains(forbidden), + "{name} exposes \"{key}\", which reads as secret material" + ); + } + } + } } /// The envelope must not carry requester, owner, or scope: those are derived by /// the host from the credential. A body that could name its own subject would -/// let any caller act as anyone. +/// let any caller act as anyone — the same reason `agents.create` has no owner. #[test] -fn request_carries_no_caller_supplied_authority() { - let json = serde_json::to_value( +fn no_payload_can_name_its_own_authority() { + let request = serde_json::to_value( BrokerRequest::new( "req-1", ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL)), @@ -358,163 +493,58 @@ fn request_carries_no_caller_supplied_authority() { .unwrap(), ) .unwrap(); - let object = json.as_object().unwrap(); - - for forbidden in [ - "ownerPubkey", - "owner", - "requesterPubkey", - "requester", - "agentPubkey", - "authorization", - "credential", - "token", - "scope", - "expiry", - "relayUrl", - "requestDigest", - ] { - assert!( - !object.contains_key(forbidden), - "broker request must not carry \"{forbidden}\"" - ); - } - - let mut keys: Vec<&str> = object.keys().map(String::as_str).collect(); - keys.sort_unstable(); assert_eq!( - keys, + keys_of(&request), vec![ "action", "actionVersion", "args", "protocolVersion", "requestId", - "type", + "type" ] ); -} -/// `agents.create` must not let a request name its own authority: ownership is -/// the authenticated requester, which is why there is no field for it. -#[test] -fn agents_create_has_no_owner_field() { - let json = serde_json::to_value(AgentsCreateArgs { - channel_id: CHANNEL.into(), - display_name: "A".into(), - system_prompt: "B".into(), - runtime: None, - provider: None, - model: None, - respond_to: None, - }) - .unwrap(); - for forbidden in ["owner", "ownerPubkey", "createdBy", "onBehalfOf"] { - assert!(json.get(forbidden).is_none()); - } - - let with_owner = serde_json::json!({ - "channelId": CHANNEL, - "displayName": "A", - "systemPrompt": "B", - "ownerPubkey": PUBKEY, - }); - assert!(serde_json::from_value::(with_owner).is_err()); -} - -// ── The no-secret invariant ───────────────────────────────────────────────── - -/// No type in this contract may name, hold, or accept secret key material. -/// -/// Scanning the source is the load-bearing half of this invariant: a -/// deserialization test can only reject fields someone thought to add, while -/// this fails the moment a secret-key type appears in a signature at all. -#[test] -fn no_secret_key_type_appears_in_the_contract_source() { - // `tests.rs` is excluded: this file necessarily names the forbidden tokens. - let sources = [ - ("mod.rs", include_str!("mod.rs")), - ("actions.rs", include_str!("actions.rs")), - ("client.rs", include_str!("client.rs")), - ]; - // Substrings, so `Option` and `nostr::Keys` are caught too. - let forbidden = [ - "SecretKey", - "nostr::Keys", - "Keys::", - "secret_key", - "seckey", - "private_key", - "nsec1", - "sign_event", - "ConversationKey", - ]; - - for (name, source) in sources { - for token in forbidden { - for (index, line) in source.lines().enumerate() { - // Doc comments may discuss what is absent and why. - let code = line.trim_start(); - if code.starts_with("//") { - continue; - } - assert!( - !code.contains(token), - "{name}:{} names \"{token}\" in code: {code}", - index + 1 - ); - } - } - } -} - -/// A secret smuggled into args must fail to deserialize rather than reach an -/// executor. -#[test] -fn secret_bearing_args_fail_to_deserialize() { - let smuggled = [ + // Authority-naming fields are rejected, not ignored, wherever they appear. + for rejected in [ serde_json::json!({ - "channelId": CHANNEL, - "displayName": "Sneaky", - "systemPrompt": "hi", + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", + "ownerPubkey": PUBKEY, + }), + serde_json::json!({ + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", + "onBehalfOf": PUBKEY, + }), + serde_json::json!({ + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", "envVars": { "ANTHROPIC_API_KEY": "sk-live" }, }), serde_json::json!({ - "channelId": CHANNEL, - "displayName": "Sneaky", - "systemPrompt": "hi", + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", "secretKey": "nsec1deadbeef", }), - serde_json::json!({ - "channelId": CHANNEL, - "displayName": "Sneaky", - "systemPrompt": "hi", - "privateKey": "hexsecret", - }), - ]; - for args in smuggled { + ] { assert!( - serde_json::from_value::(args.clone()).is_err(), - "must reject: {args}" + serde_json::from_value::(rejected.clone()).is_err(), + "must reject: {rejected}" ); } -} -/// An outcome must be structurally unable to carry secret material, including -/// the freshly minted key of an agent it just created. -#[test] -fn outcomes_cannot_carry_secrets() { - for extra in [ - "privateKeyNsec", - "nsec", - "secretKey", - "seckey", - "credential", - ] { + // A read cannot ask about someone else's mentions, and a profile write + // cannot name a subject. + assert!(serde_json::from_value::( + serde_json::json!({ "channelId": CHANNEL, "mentionsOf": PUBKEY }) + ) + .is_err()); + assert!(serde_json::from_value::( + serde_json::json!({ "displayName": "A", "pubkey": PUBKEY }) + ) + .is_err()); + + // An outcome cannot smuggle a minted secret past the schema either. + for extra in ["nsec", "secretKey", "seckey", "credential"] { let mut outcome = serde_json::json!({ - "agentPubkey": PUBKEY, - "displayName": "A", - "channelId": CHANNEL, + "agentPubkey": PUBKEY, "displayName": "A", "channelId": CHANNEL, }); outcome[extra] = serde_json::json!("nsec1deadbeef"); let json = serde_json::json!({ "action": "agents.create", "outcome": outcome }); @@ -525,26 +555,6 @@ fn outcomes_cannot_carry_secrets() { } } -/// A `storage.address` outcome carries addressing material only — never the key -/// that derived it. -#[test] -fn storage_address_outcome_carries_addressing_only() { - let json = serde_json::to_value(StorageAddress { - author_pubkey: pubkey(), - kind: 30174, - d_tag: EVENT.into(), - }) - .unwrap(); - let mut keys: Vec<&str> = json - .as_object() - .unwrap() - .keys() - .map(String::as_str) - .collect(); - keys.sort_unstable(); - assert_eq!(keys, vec!["authorPubkey", "dTag", "kind"]); -} - #[test] fn pubkey_hex_rejects_anything_but_a_public_key() { assert!(PubkeyHex::parse("nothex").is_err()); @@ -562,137 +572,102 @@ fn pubkey_hex_rejects_anything_but_a_public_key() { // ── Argument validation ───────────────────────────────────────────────────── +/// Boundaries of every shared validator, in one table. #[test] -fn every_fixture_validates_and_normalization_is_idempotent() { - for args in action_fixtures() { - let once = args.validated().expect("fixture validates"); - let twice = once.validated().expect("normalized form validates"); - assert_eq!( - once, - twice, - "{} normalization must settle", - args.action().as_str() - ); - } -} - -#[test] -fn reads_reject_a_bad_channel_thread_or_limit() { - assert!(ChannelReadArgs::channel("not-a-uuid").validated().is_err()); - assert!(ChannelReadArgs::channel(CHANNEL) - .in_thread("nothex") - .validated() - .is_err()); - assert!(ChannelReadArgs::channel(CHANNEL) - .limit(0) - .validated() - .is_err()); - assert!(ChannelReadArgs::channel(CHANNEL) - .limit(actions::MAX_PAGE_LIMIT) - .validated() - .is_ok()); - assert!(ChannelReadArgs::channel(CHANNEL) - .limit(actions::MAX_PAGE_LIMIT + 1) - .validated() - .is_err()); -} - -/// A bare channel read must not imply a thread or a mention filter the caller -/// never asked for. -#[test] -fn a_bare_channel_read_carries_no_unset_narrowing() { - let json = serde_json::to_value(ChannelReadArgs::channel(CHANNEL)).unwrap(); - let mut keys: Vec<&str> = json - .as_object() - .unwrap() - .keys() - .map(String::as_str) - .collect(); - keys.sort_unstable(); - assert_eq!(keys, vec!["channelId"]); -} - -#[test] -fn writes_reject_empty_oversized_and_over_mentioned_content() { - let post = |content: String, mentions: Vec| MessagePostArgs { - channel_id: CHANNEL.into(), - content, - mentions, +fn validators_accept_and_reject_at_their_boundaries() { + let read = |mutate: fn(&mut ChannelReadArgs)| { + let mut args = ChannelReadArgs::channel(CHANNEL); + mutate(&mut args); + args.validated().is_ok() }; - assert!(post(" ".into(), vec![]).validated().is_err()); + let post = |content: String, mentions: Vec| { + MessagePostArgs { + channel_id: CHANNEL.into(), + content, + mentions, + } + .validated() + }; + let react = |reaction: String| { + ReactionAddArgs { + channel_id: CHANNEL.into(), + target_event_id: EVENT.into(), + reaction, + } + .validated() + }; + let slug = |slug: &str| StorageAddressArgs { slug: slug.into() }.validated().is_ok(); + + // Channel UUID, thread id, limit, and opaque cursor. + assert!(ChannelReadArgs::channel("not-a-uuid").validated().is_err()); + assert!(!read(|a| a.root_event_id = Some("nothex".into()))); + assert!(read(|a| a.root_event_id = Some(EVENT.into()))); + assert!(!read(|a| a.limit = Some(0))); + assert!(read(|a| a.limit = Some(actions::MAX_PAGE_LIMIT))); + assert!(!read(|a| a.limit = Some(actions::MAX_PAGE_LIMIT + 1))); + assert!(!read(|a| a.cursor = Some(String::new()))); + assert!(!read(|a| a.cursor = Some("has space".into()))); + assert!(read( + |a| a.cursor = Some("a".repeat(actions::MAX_CURSOR_LEN)) + )); + assert!(!read( + |a| a.cursor = Some("a".repeat(actions::MAX_CURSOR_LEN + 1)) + )); + + // Content, mentions, reaction payload. + assert!(post(" ".into(), vec![]).is_err()); assert!(matches!( - post("x".repeat(actions::MAX_CONTENT_BYTES + 1), vec![]) - .validated() - .unwrap_err(), + post("x".repeat(actions::MAX_CONTENT_BYTES + 1), vec![]).unwrap_err(), SdkError::ContentTooLarge { .. } )); + assert!(post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS]).is_ok()); assert!(matches!( - post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS + 1]) - .validated() - .unwrap_err(), + post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS + 1]).unwrap_err(), SdkError::TooManyMentions )); - assert!(post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS]) - .validated() - .is_ok()); -} - -#[test] -fn reaction_rejects_empty_and_oversized_payloads() { - let react = |reaction: String| ReactionAddArgs { - channel_id: CHANNEL.into(), - target_event_id: EVENT.into(), - reaction, - }; - assert!(react(" ".into()).validated().is_err()); + assert!(react(" ".into()).is_err()); + assert!(react(":shipit:".into()).is_ok()); assert!(matches!( - react("a".repeat(actions::MAX_EMOJI_CHARS + 1)) - .validated() - .unwrap_err(), + react("a".repeat(actions::MAX_EMOJI_CHARS + 1)).unwrap_err(), SdkError::EmojiTooLong )); - assert!(react(":shipit:".into()).validated().is_ok()); -} -#[test] -fn profile_set_requires_a_change_and_names_no_subject() { - let empty = ProfileSetArgs { + // NIP-AE slug grammar for encrypted-memory addressing. + assert!(slug("core")); + assert!(slug("mem/broker-foundation")); + assert!(!slug("")); + assert!(!slug("Core")); + assert!(!slug("secrets")); + assert!(!slug("mem/Bad Slug")); + + // Patch-shaped writes must change something, and reject unknown modes. + let profile_error = ProfileSetArgs { display_name: None, about: None, picture: None, + } + .validated() + .unwrap_err() + .to_string(); + assert!(profile_error.contains("at least one"), "{profile_error}"); + let update = |respond_to: Option<&str>, name: Option<&str>| { + AgentsUpdateArgs { + target: AgentTarget::Pubkey(pubkey()), + display_name: name.map(str::to_string), + system_prompt: None, + runtime: None, + provider: None, + model: None, + respond_to: respond_to.map(str::to_string), + } + .validated() }; - let error = empty.validated().unwrap_err().to_string(); - assert!(error.contains("at least one"), "unexpected: {error}"); - - let with_subject = serde_json::json!({ "displayName": "A", "pubkey": PUBKEY }); - assert!(serde_json::from_value::(with_subject).is_err()); -} - -#[test] -fn storage_address_enforces_the_nip_ae_slug_grammar() { - let slug = |slug: &str| StorageAddressArgs { slug: slug.into() }.validated(); - assert!(slug("core").is_ok()); - assert!(slug("mem/broker-foundation").is_ok()); - assert!(slug("").is_err()); - assert!(slug("Core").is_err()); - assert!(slug("secrets").is_err()); - assert!(slug("mem/Bad Slug").is_err()); -} - -#[test] -fn agent_update_requires_a_change_and_delete_requires_a_target() { - let empty = AgentsUpdateArgs { - target: AgentTarget::Pubkey(pubkey()), - display_name: None, - system_prompt: None, - runtime: None, - provider: None, - model: None, - respond_to: None, - }; - let error = empty.validated().unwrap_err().to_string(); - assert!(error.contains("at least one field"), "unexpected: {error}"); - + assert!(update(None, None) + .unwrap_err() + .to_string() + .contains("at least one field")); + assert!(update(Some("anyone"), None).is_ok()); + assert!(update(Some("allowlist"), Some("A")).is_err()); assert!(AgentsDeleteArgs { target: AgentTarget::Name(" ".into()), } @@ -700,41 +675,70 @@ fn agent_update_requires_a_change_and_delete_requires_a_target() { .is_err()); } +// ── Reads carry verifiable provenance ─────────────────────────────────────── + +/// A read returns the signed event, so a keyless caller can check authorship +/// itself. A host that tampered with content fails verification locally, with no +/// relay involved — which is why this contract does not settle for a projection. #[test] -fn agents_create_rejects_an_unsupported_respond_to_mode() { - let args = AgentsCreateArgs { - channel_id: CHANNEL.into(), - display_name: "A".into(), - system_prompt: "B".into(), - runtime: None, - provider: None, - model: None, - respond_to: Some("allowlist".into()), - }; - assert!(args.validated().is_err()); +fn read_results_are_signed_events_a_keyless_caller_can_verify() { + let signer = Keys::generate(); + let message = signed_message(&signer); + message.verify().expect("a genuinely signed event verifies"); + assert_eq!( + message.author().unwrap().as_str(), + signer.public_key().to_hex() + ); + assert_eq!(message.thread().root.as_deref(), Some(EVENT)); + assert_eq!(message.mentions(), vec![PUBKEY.to_string()]); + + // Tamper with the content: the id no longer matches, so verification fails + // even though every other field is untouched. + let mut json = serde_json::to_value(&message).unwrap(); + json["content"] = serde_json::json!("a message the author never wrote"); + let tampered: BrokerMessage = + serde_json::from_value(json).expect("a tampered event still parses"); + assert!( + tampered.verify().is_err(), + "tampering must be locally detectable" + ); + + // The wire form is the event's own JSON — no wrapper of its own to disagree + // with the signed bytes. + let wire = serde_json::to_value(&message).unwrap(); + assert_eq!( + keys_of(&wire), + vec![ + "content", + "created_at", + "id", + "kind", + "pubkey", + "sig", + "tags" + ] + ); } -/// Update and delete carry no channel: channel appears only where the operation -/// needs it, which is the create attachment. #[test] -fn agent_update_and_delete_carry_no_channel() { - let update = serde_json::to_value(AgentsUpdateArgs { - target: AgentTarget::Pubkey(pubkey()), - display_name: Some("New".into()), - system_prompt: None, - runtime: None, - provider: None, - model: None, - respond_to: None, - }) - .unwrap(); - assert!(update.get("channelId").is_none()); - - let delete = serde_json::to_value(AgentsDeleteArgs { - target: AgentTarget::Name("Gone".into()), - }) - .unwrap(); - assert!(delete.get("channelId").is_none()); +fn a_page_is_bounded_and_its_cursor_opaque() { + let signer = Keys::generate(); + let page = |messages: Vec, next_cursor: Option<&str>| { + ActionOutcome::ChannelRead(MessagePage { + messages, + next_cursor: next_cursor.map(str::to_string), + }) + .validate() + }; + assert!(page(vec![], None).is_ok()); + assert!(page(vec![signed_message(&signer)], Some("c1")).is_ok()); + assert!(page(vec![], Some("")).is_err()); + assert!(page(vec![], Some("has space")).is_err()); + assert!(page( + vec![signed_message(&signer); actions::MAX_PAGE_LIMIT as usize + 1], + None + ) + .is_err()); } // ── Results ───────────────────────────────────────────────────────────────── @@ -764,6 +768,39 @@ fn failed_and_indeterminate_are_distinct_and_carry_no_outcome() { assert!(indeterminate.outcome().is_none()); } +/// A rejected credential is a host verdict with a known fate — the action did +/// not run — so it belongs in a `Failed` envelope, not in the transport error +/// type where it would tell the caller to reconcile something that never +/// happened. `OutcomeUnknown` is the mirror image: it may only ever be +/// `Indeterminate`. +#[test] +fn status_and_error_code_must_agree_about_side_effects() { + let auth_failure = BrokerResponse::new( + "req-1", + BrokerResult::failed(BrokerError::new( + BrokerErrorCode::Unauthenticated, + "credential rejected", + )), + ); + auth_failure + .validate() + .expect("a rejected credential is a valid Failed verdict"); + + let contradiction = BrokerResponse::new( + "req-1", + BrokerResult::failed(BrokerError::new(BrokerErrorCode::OutcomeUnknown, "?")), + ); + let error = contradiction.validate().unwrap_err().to_string(); + assert!(error.contains("outcome_unknown"), "unexpected: {error}"); + + BrokerResponse::new( + "req-1", + BrokerResult::indeterminate(BrokerError::new(BrokerErrorCode::OutcomeUnknown, "?")), + ) + .validate() + .expect("outcome_unknown is valid with indeterminate"); +} + #[test] fn replay_metadata_rides_the_response_not_the_result() { let result = BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { @@ -789,54 +826,167 @@ fn replay_metadata_rides_the_response_not_the_result() { .is_none()); } +/// A response that validates in isolation can still be the wrong answer. This is +/// the check that makes a mismatched outcome unusable rather than merely +/// surprising. #[test] -fn every_error_code_has_a_stable_wire_string() { - for (code, expected) in [ - (BrokerErrorCode::InvalidRequest, "invalid_request"), - ( - BrokerErrorCode::UnsupportedProtocolVersion, - "unsupported_protocol_version", - ), - (BrokerErrorCode::UnknownAction, "unknown_action"), - ( - BrokerErrorCode::UnsupportedActionVersion, - "unsupported_action_version", - ), - (BrokerErrorCode::Unsupported, "unsupported"), - (BrokerErrorCode::Unauthenticated, "unauthenticated"), - (BrokerErrorCode::Unauthorized, "unauthorized"), - (BrokerErrorCode::RequestIdConflict, "request_id_conflict"), - (BrokerErrorCode::ActionFailed, "action_failed"), - (BrokerErrorCode::OutcomeUnknown, "outcome_unknown"), - (BrokerErrorCode::Internal, "internal"), - ] { - assert_eq!(code.as_str(), expected); +fn response_validation_is_request_aware() { + let signer = Keys::generate(); + let request = prepared(ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL))); + let page = ActionOutcome::ChannelRead(MessagePage { + messages: vec![signed_message(&signer)], + next_cursor: None, + }); + + BrokerResponse::new(request.request_id(), BrokerResult::succeeded(page.clone())) + .validate_for(&request) + .expect("the right outcome for the right request"); + + // Wrong action: a post receipt is not an answer to a read. + let wrong_action = BrokerResponse::new( + request.request_id(), + BrokerResult::succeeded(ActionOutcome::MessagePost(EventPublished { + event_id: EVENT.into(), + kind: 9, + created_at: 1, + })), + ); + wrong_action + .validate() + .expect("it is well-formed on its own — that is the point"); + let error = wrong_action.validate_for(&request).unwrap_err().to_string(); + assert!(error.contains("message.post"), "unexpected: {error}"); + + // Wrong correlation id. + let error = BrokerResponse::new("req-other", BrokerResult::succeeded(page)) + .validate_for(&request) + .unwrap_err() + .to_string(); + assert!(error.contains("requestId"), "unexpected: {error}"); + + // Malformed identifiers inside an otherwise well-shaped outcome. + let bad_id = BrokerResponse::new( + request.request_id(), + BrokerResult::succeeded(ActionOutcome::ChannelRead(MessagePage { + messages: vec![], + next_cursor: Some("not a cursor".into()), + })), + ); + assert!(bad_id.validate_for(&request).is_err()); + + let post = prepared(ActionArgs::MessagePost(MessagePostArgs { + channel_id: CHANNEL.into(), + content: "hi".into(), + mentions: vec![], + })); + let bad_event_id = BrokerResponse::new( + post.request_id(), + BrokerResult::succeeded(ActionOutcome::MessagePost(EventPublished { + event_id: "nothex".into(), + kind: 9, + created_at: 1, + })), + ); + assert!(bad_event_id.validate_for(&post).is_err()); + + // A failure needs no outcome to match, only correlation. + BrokerResponse::new( + request.request_id(), + BrokerResult::failed(BrokerError::unauthorized("not your channel")), + ) + .validate_for(&request) + .expect("a refusal answers any action"); +} + +// ── Retry is identical bytes ──────────────────────────────────────────────── + +/// The retry contract is byte identity, so the client takes frozen bytes rather +/// than a typed value it would have to reserialize. Preparing once and reading +/// `body()` twice is the only way to send the same request twice. +#[test] +fn preparing_a_request_freezes_the_bytes_every_attempt_sends() { + let request = BrokerRequest::new( + "req-idem", + ActionArgs::MessagePost(MessagePostArgs { + channel_id: CHANNEL.into(), + content: "exactly once".into(), + mentions: vec![pubkey()], + }), + ) + .unwrap(); + let prepared = request.clone().prepare().expect("valid request prepares"); + + assert_eq!( + prepared.body(), + prepared.body(), + "body is frozen, not re-rendered" + ); + assert_eq!(prepared.request(), &request); + assert_eq!(prepared.request_id(), "req-idem"); + assert_eq!(prepared.action(), Action::MessagePost); + + // The frozen bytes are the envelope, and they parse back to the same value. + let parsed: BrokerRequest = + serde_json::from_slice(prepared.body()).expect("frozen body is the envelope"); + assert_eq!(parsed, request); + + // Preparing validates, so an invalid request never reaches a transport. + let invalid = BrokerRequest { + r#type: BROKER_REQUEST_TYPE.into(), + protocol_version: 99, + request_id: "req-bad".into(), + action_version: 1, + action: ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Pubkey(pubkey()), + }), + }; + assert!(invalid.prepare().is_err()); +} + +/// The hand-written [`BrokerErrorCode::as_str`] and serde's derived name are two +/// encodings of one wire string, so each is pinned against the other and the +/// whole set is pinned against this literal — a rename in either fails here. +#[test] +fn error_codes_have_stable_wire_strings() { + use BrokerErrorCode as E; + let codes = [ + E::InvalidRequest, + E::UnsupportedProtocolVersion, + E::UnknownAction, + E::UnsupportedActionVersion, + E::Unsupported, + E::Unauthenticated, + E::Unauthorized, + E::RequestIdConflict, + E::ActionFailed, + E::OutcomeUnknown, + E::Internal, + ]; + for code in codes { assert_eq!( serde_json::to_value(code).unwrap(), - serde_json::json!(expected) + serde_json::json!(code.as_str()), + "as_str and the serde name must not drift" ); } + assert_eq!( + codes.map(E::as_str).join(","), + "invalid_request,unsupported_protocol_version,unknown_action,\ +unsupported_action_version,unsupported,unauthenticated,unauthorized,\ +request_id_conflict,action_failed,outcome_unknown,internal" + ); } /// Only housekeeping is best-effort. A host refusing a read or a write has /// broken the agent, and the agent must not treat that as routine. #[test] fn only_housekeeping_actions_are_best_effort() { - assert!(Action::ReactionAdd.is_best_effort()); - for essential in [ - Action::ChannelRead, - Action::MessagePost, - Action::MessageReply, - Action::ProfileSet, - Action::StorageAddress, - Action::AgentsCreate, - Action::AgentsUpdate, - Action::AgentsDelete, - ] { - assert!( - !essential.is_best_effort(), - "{} must not be best-effort", - essential.as_str() + for action in Action::ALL { + assert_eq!( + action.is_best_effort(), + action == Action::ReactionAdd, + "{} best-effort classification", + action.as_str() ); } } @@ -851,10 +1001,15 @@ struct DoubleBroker { } impl BrokerClient for DoubleBroker { - fn execute<'a>(&'a self, request: &'a BrokerRequest) -> BrokerFuture<'a> { - let response = self.response.clone().map(|mut response| { - response.request_id = request.request_id.clone(); + fn execute<'a>(&'a self, request: &'a PreparedRequest) -> BrokerFuture<'a> { + // A real implementation sends `request.body()` verbatim. The double + // stands in for a host that answers the request it was given. + let response = self.response.clone().and_then(|mut response| { + response.request_id = request.request_id().to_string(); response + .validate_for(request) + .map_err(|e| BrokerTransportError::MalformedResponse(e.to_string()))?; + Ok(response) }); Box::pin(async move { response }) } @@ -879,11 +1034,11 @@ fn block_on(future: F) -> F::Output { #[test] fn the_client_trait_is_object_safe_and_returns_a_host_verdict() { - let request = BrokerRequest::new( - "req-42", - ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL).mentions_only()), - ) - .unwrap(); + let request = prepared(ActionArgs::ChannelRead(ChannelReadArgs { + channel_id: CHANNEL.into(), + mentions_only: true, + ..ChannelReadArgs::default() + })); let succeeded: Box = Box::new(DoubleBroker { response: Ok(BrokerResponse::new( @@ -895,35 +1050,57 @@ fn the_client_trait_is_object_safe_and_returns_a_host_verdict() { )), }); let response = block_on(succeeded.execute(&request)).expect("double answers"); - response.validate().expect("double's response is valid"); - assert_eq!(response.request_id, "req-42"); + response + .validate_for(&request) + .expect("double's response answers the request"); + assert_eq!(response.request_id, "req-1"); assert!(response.result.outcome().is_some()); - // A refusal is still an answer: `Ok` with the verdict in the envelope. - let refused: Box = Box::new(DoubleBroker { + // A refusal — including a rejected credential — is still an answer: `Ok` + // with the verdict in the envelope. + for code in [ + BrokerErrorCode::Unauthorized, + BrokerErrorCode::Unauthenticated, + ] { + let refused: Box = Box::new(DoubleBroker { + response: Ok(BrokerResponse::new( + "placeholder", + BrokerResult::failed(BrokerError::new(code, "no")), + )), + }); + let response = + block_on(refused.execute(&request)).expect("a refusal is not a transport error"); + assert_eq!(response.result.error().map(|e| e.code), Some(code)); + } + + // No usable answer at all is a transport error, and says nothing about side + // effects. An intermediary's status is operator detail, not a verdict. + for error in [ + BrokerTransportError::Unreachable("connection reset".into()), + BrokerTransportError::NoEnvelope { + status: 401, + detail: "proxy denied".into(), + }, + BrokerTransportError::MalformedResponse("not json".into()), + ] { + let broken: Box = Box::new(DoubleBroker { + response: Err(error.clone()), + }); + assert_eq!(block_on(broken.execute(&request)).unwrap_err(), error); + } + + // A host answering the wrong action is rejected by the client, not passed on. + let confused: Box = Box::new(DoubleBroker { response: Ok(BrokerResponse::new( "placeholder", - BrokerResult::failed(BrokerError::unauthorized("not your channel")), + BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: pubkey(), + display_name: "Gone".into(), + })), )), }); - let response = block_on(refused.execute(&request)).expect("a refusal is not a transport error"); - assert_eq!( - response.result.error().map(|e| e.code), - Some(BrokerErrorCode::Unauthorized) - ); - - // No answer at all is a transport error, and says nothing about side effects. - let unreachable: Box = Box::new(DoubleBroker { - response: Err(BrokerTransportError::CredentialRejected), - }); - assert_eq!( - block_on(unreachable.execute(&request)).unwrap_err(), - BrokerTransportError::CredentialRejected - ); -} - -#[test] -fn the_http_binding_is_a_single_path_with_a_bearer_credential() { - assert_eq!(BROKER_ACTION_PATH, "/v1/action"); - assert_eq!(BROKER_CREDENTIAL_HEADER, "authorization"); + assert!(matches!( + block_on(confused.execute(&request)).unwrap_err(), + BrokerTransportError::MalformedResponse(_) + )); }