broker: add storage read/write and live-signal actions to the contract - #6922
broker: add storage read/write and live-signal actions to the contract#6922jrobotham-square wants to merge 7 commits into
Conversation
Memory read/write graduates from a deferred operation into v1. A keyless agent
could address an encrypted-memory record (storage.address) but not read or
write one, so a keyless runtime stays amnesiac across wakes.
Both actions are slug-addressed and mirror the existing nine:
- storage.get { slug } -> { value? } (value absent = no record, not an error)
- storage.put { slug, value } -> EventPublished
The host derives the address, encrypts on put, and decrypts on get, so the
secret never leaves the key holder -- the same reason storage.address routes
through the interface. value is non-empty and bounded by MAX_CONTENT_BYTES. No
patch/rm: a client composes those from read-modify-write, as profile.set
already does for partial updates.
storage.address is left in place; whether get/put make it redundant is an open
question for review.
Contract crate only. buzz-cli's exhaustive matches gain the two variants when
the keyless-client branch rebases onto this.
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>
ee63980 to
b25a202
Compare
…age-rw * origin/main: (21 commits) fix(desktop): resolve exact typed mentions on space (#6862) perf(desktop): restore project context during startup (#6939) fix(desktop): lift right auxiliary pane above shared header backdrop (#6966) fix(ci): bump Codex CLI to 0.150.1 to unhang security review jobs (#6962) feat(desktop): implement 30178 team catalog backend (#5112) feat(model-capabilities): humanize Databricks UC model families (#6955) feat(agent): discover Databricks Unity Catalog models (#6918) 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) ... Signed-off-by: Joel Robotham <jrobotham@squareup.com>
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes at fcf2e9bc753e9ce047b21710c4262e106c2f706b (base b49675894b39f87215e4dfc8c1ad4a3c28c6097e). The six variants cover the SDK's closed sets, but two new request/response contracts accept states their protocols cannot safely fulfill.
[P1] Validate the encoded NIP-AE body, not only storage.put.value
StoragePutArgs::validated accepts a value up to MAX_CONTENT_BYTES = 65,536 bytes (crates/buzz-sdk/src/broker/actions/mod.rs:38-39, actions/args.rs:330-353). NIP-AE permits at most 65,535 bytes for the entire serialized plaintext body, and the canonical encoder adds the slug, field names, JSON structure, and escaping (crates/buzz-core/src/engram.rs:26-28,187-210; docs/nips/NIP-AE.md:109-130). A request can therefore pass SDK validation but be impossible for every compliant host to publish. Even an ASCII value below the advertised cap can overflow once the envelope is added; quotes and control characters expand it further.
Construct the appropriate NIP-AE Body, measure its canonical to_json_bytes(), and reject when that complete plaintext exceeds NIP44_PLAINTEXT_MAX. Add boundary tests for core, mem/..., and escaping expansion.
[P2] Bound observer batches and correlate the accepted count
observer.emit permits 256 frames of 65,536 payload bytes each, but validates only per-frame size and frame count (actions/mod.rs:62-73, actions/args.rs:438-496). This admits a roughly 16 MiB opaque request before JSON escaping and forces the future host to buffer/re-batch/encrypt it, despite the observer protocol's 65,535-byte plaintext ceiling. Add an aggregate serialized-byte budget rather than relying on a future dispatcher to invent a stricter contract.
The response side is also uncorrelated: ObserverReceipt.accepted is an unrestricted u32, outcome validation intentionally does nothing for it, and request-aware validation checks only channel.read (actions/outcomes.rs:185-195,320-346; broker/mod.rs:731-766). A one-frame request therefore accepts a successful receipt claiming 4,294,967,295 accepted frames. In validate_for, require accepted <= request.frames.len() and cover zero, exact, and over-count receipts.
The authoritative enum/serde/correlation surfaces otherwise include all six actions, and there is no in-repo host dispatcher to break. cargo test -p buzz-sdk broker passed 40 tests at this head; git diff --check passes; exact-head required CI is green.
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
|
🤖 Updated by Codex in d9ab189: storage.put now validates the complete canonical NIP-AE body, observer.emit has an aggregate serialized-byte bound, and observer receipts are correlated to the submitted frame count. Boundary and escaping regressions are included. |
…age-rw * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) Signed-off-by: Joel Robotham <jrobotham@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-reviewed head 37230b5dc0601b40950a526c192b508fbd81c43a against base 2c99ee7af5a20d239e95ec5368407887449795c9. Both blockers from review 5052594543 are resolved; no remaining actionable defect found in this contract-only change.
- Storage P1 resolved:
StoragePutArgs::validatednow measures the complete canonical NIP-AE Core/Memory body, matching the actual engram encoder and 65,535-byte plaintext cap. Exact-limit, one-over, and escaping tests cover the repair. - Observer P2 resolved: validation caps the complete normalized serialized batch, including metadata and escaping. Request-aware response validation rejects
accepted > submitted frames, with zero/exact/over-count coverage; the client’s mandatoryexecutewrapper invokes that validation. - Integration checked: all 15 actions agree across the action enum, enumeration list, arguments, outcomes, wire names and fixtures. The six additions follow the existing strict-schema and authority boundaries. In
cratesanddesktop/src-tauri, broker request/response/client references remain confined to the SDK contract, so this change does not modify a running host dispatcher.
Validation at this exact head and a clean working tree: cargo test -p buzz-sdk 305 passed, 0 failed, and git diff --check passed. Independent storage and observer review lanes were integrated. The current-head GitHub check snapshot shows Rust lint, unit tests, and integration/build checks passing. The separate Codex security-review comment is stale for this range and its execution was skipped; that remains a merge-process gate, not an additional code finding. No new host/relay integration was exercised locally. Host authorization, publication pacing, watchdog lifecycle and real keyless-client behavior remain implementation-stage verification, not shipped by this PR.
This is a non-approving follow-up review, not a GitHub approval.
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Additional boundary verification at
|
Signed-off-by: Joel Robotham <jrobotham@squareup.com>
…age-rw * origin/main: feat: render agent avatars as squircles (#7106) fix(ci): salvage Codex review output on PTY-shutdown hang (#7042) fix: retrieving cold memories; add regression task (#6950) Enforce NIP-OA authorization time bounds (#7004) feat(db): configurable writer session timeouts (lock, idle-txn, statement) (#6229) feat(desktop): use segmented controls for channel creation (#6845) feat(buzz-agent): surface stop reason and silent-turn WARN in telemetry (#7038) fix(desktop): surface channel history load failures (#7013) fix(composer): polish automatic mentions (#6956) Signed-off-by: Joel Robotham <jrobotham@squareup.com>
|
🤖 Updated by Codex at
Local focused tests and the full pre-push suite pass. The new-head CI unit and lint checks are also passing, with the remaining CI lanes still completing. Ready for re-review and re-approval. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-reviewed head 115e7975a11d7d4d95043cb847abd13c1a32f370 against base bc006f67087b049e2f9c4d8a2f26faceff628225. No actionable blockers found for this contract-only change.
- The broker module is byte-identical to previously cleared
37230b5dc0601b40950a526c192b508fbd81c43a. Complete canonical NIP-AE body bounds, aggregate serialized observer limits, and request-correlated accepted-frame counts remain intact. Core protocol code, CLI storage implementation and locked dependencies are unchanged from that review; merged SDK auth changes do not introduce a broker dependency. All 15 actions still agree across the enum, enumeration list, arguments, outcomes and fixture sets. - The test-coverage gap is closed: the CI Unit Tests job calls
just test-unit, whose nextest branch now runs the wholebuzz-sdkpackage; the fallback runner also invokes the full package. Independently exercised both runner paths with fake Cargo commands: each invokes the SDK suite, succeeds when it passes, and exits nonzero when the SDK step fails. These were runner-control-flow checks, not fresh Rust suite executions. Shell syntax and base-to-head diff checks passed; current-head GitHub unit/lint checks report success. Broad suites were not duplicated locally. - The description now correctly defers removal/core clearing. The known 65,408-byte implementation ceiling in the existing NIP-44 codec remains a separate pre-existing integration limitation. No concrete broker host/transport was added, and no live keyless-host workflow was exercised. Authorization, publication pacing and watchdog behavior still require implementation-stage validation. The exact-range Codex security review remains skipped/review-required, a separate merge-process gate.
Storage, live-signal and schema/CI review lanes were integrated. This is a non-approving review; no GitHub approval is being submitted.
What
Extends the broker action contract (
buzz-sdk::broker) with six actions in two groups. Four were named as deferred by #6742 (memory read/write,presence.set,typing.set); the other two,observer.emitandliveness.ping, are net-new — the trajectory and keepalive planes the contract never enumerated.Storage — memory read/write (deferred by #6742):
storage.get{ slug }{ value? }—valueabsent = no record (not an error)storage.put{ slug, value }EventPublishedLive signals — the ephemeral signals a running agent emits so an owner and channel can see it work:
presence.set{ status }EventPublishedtyping.set{ channelId }EventPublishedobserver.emit{ frames: [{ kind, payload }] }{ accepted }(batch receipt)liveness.ping{ channelId, turnId }EventPublishedAll six mirror the existing nine in shape, validation, strict-wire rules, and test coverage. The four signal actions are best-effort — a host that doesn't offer one refuses it and the agent carries on.
Why
A keyless agent routes everything through the broker host and holds no relay connection, so anything a local agent emits directly to the relay needs an action or it silently can't happen. #6742 shipped the nine core actions and named four deferrals; this PR:
storage.addresscould address an encrypted-memory record but not read or write one (a keyless runtime was amnesiac across wakes), and presence/typing are how an owner and channel see an agent is alive and composing (the presence dot, the typing bubble);observer.emitandliveness.ping, which broker: define the agent-to-broker action contract #6742 didn't enumerate — the observer/trajectory stream in Buzz Desktop and a turn keepalive. These fell out of the keyless-observability discussion: without them a keyless agent runs invisibly, with no way to surface its tool calls or prove a long turn is still alive.Decisions baked in (all up for discussion)
observer.emitis a batch — trajectory is high-volume, so frames are sent per call and the host re-batches/paces. Each frame is{ kind, payload }wherepayloadis opaque (a serialized frame body the host encrypts verbatim, never parses);kindstays top-level so a host can apply per-kind policy without decrypting. The outcome is a batch receipt{ accepted }, since re-batched frames have no stable per-frame id.liveness.pingis its own action despite overlappingobserver.emiton the wire (a keepalive could be one more frame). Keeping it separate lets a host attach meaning — resetting a stall watchdog — rather than only forwarding it. Flagged in the module docs; happy to collapse it if preferred.typing.sethas no stop counterpart — the indicator is ephemeral and lapses on its own; a client signals by re-sending and stops by falling silent. Thread-scoped typing is a purely-additive later refinement.presence.setreusesbuzz_core::presence::PresenceStatus(online/away/offline) rather than minting a parallel enum.value, nopatch/rm— get/put cover reads and nonempty writes only; removal and core clearing remain deferred.Open questions for review
liveness.pingvsobserver.emit— worth a first-class action, or fold it in?MAX_OBSERVER_FRAMES = 256per call,MAX_OBSERVER_FRAME_BYTES = 64 KiBper frame. Reasonable for trajectory volume?storage.addressfate — with slug-addressed get/put, doesaddressstill earn its place (existence checks / coordinates without a payload)? Left in place either way.Scope / sequencing
The contract changes are self-contained and mergeable on their own; this PR also wires
buzz-sdkinto the existing unit-test runners so its contract regressions are enforced in CI. Nothing onmainmatches these enums exhaustively outsidebuzz-sdk. When the keyless client branch rebases onto this,buzz-cli's exhaustive matches gain the six variants — the forcing function to implement them over the host seam — which is where the client's memory refusals and disabled observer path get removed.The contract remains reviewable in two logical parts:
storage.get/storage.put, then the four signals.Verification
cargo test -p buzz-sdk— 307 passed. The self-enforcing coverage, round-trip, null-injection, canonicalization, and exact-schema tests all extend to the new actions automatically.cargo clippy -p buzz-sdk --all-targets -- -D warnings— clean.cargo fmt -p buzz-sdk -- --check— clean.cargo check --workspace— clean.Update Aug 29, 09:18: Tightened the contract bounds following review.
storage.putagainst the complete canonical NIP-AE plaintext, including slug, envelope, and JSON escaping.observer.emitbatch and reject receipts that accept more frames than the request supplied.Update Aug 31, 11:09: Corrected storage scope and CI coverage.
buzz-sdksuite in both unit-test paths.🤖 Generated with Claude Code