From cada3028ceda7ebdae61329f15b5364ca0d23dd4 Mon Sep 17 00:00:00 2001 From: thomaspblock Date: Wed, 26 Aug 2026 20:59:47 -0400 Subject: [PATCH 01/37] fix(desktop): keep project sheets independent from threads (#6901) ## Summary Project workspace sheets now slide in from the right as an independent layer over an open conversation thread instead of replacing the thread or rendering inside its panel. Closing the sheet restores the unchanged thread, Escape applies only to the topmost surface, and the covered thread's resize divider no longer leaks through the overlay. ### Related issue Related: #6602. No matching issue found. ### Testing - `pnpm --dir desktop exec biome check src/features/channels/ui/RightAuxiliaryPane.tsx tests/e2e/project-commit-detail.spec.ts` - `pnpm exec playwright test tests/e2e/project-commit-detail.spec.ts --project=smoke --grep "project workspace sheet stays independent from an open thread"` - Pre-push desktop formatting, file-size, typecheck, and unit-test checks --------- Signed-off-by: Thomas Petersen --- .../channels/ui/ChannelPane.helpers.test.mjs | 16 ++ .../channels/ui/ChannelPane.helpers.ts | 6 +- .../src/features/channels/ui/ChannelPane.tsx | 69 +++++--- .../channels/ui/FocusThreadDrawer.tsx | 61 ++++++- .../channels/ui/RightAuxiliaryPane.tsx | 2 +- .../channels/ui/ThreadPanelSurface.tsx | 66 ++++++++ .../channels/ui/useFocusDrawerPresence.ts | 18 ++- .../tests/e2e/project-commit-detail.spec.ts | 150 +++++++++++++++++- 8 files changed, 352 insertions(+), 36 deletions(-) create mode 100644 desktop/src/features/channels/ui/ThreadPanelSurface.tsx diff --git a/desktop/src/features/channels/ui/ChannelPane.helpers.test.mjs b/desktop/src/features/channels/ui/ChannelPane.helpers.test.mjs index 10a948e705..9d2cd6a32d 100644 --- a/desktop/src/features/channels/ui/ChannelPane.helpers.test.mjs +++ b/desktop/src/features/channels/ui/ChannelPane.helpers.test.mjs @@ -42,6 +42,22 @@ test("focus idle drawers yield to every higher-priority auxiliary surface", () = } }); +test("an explicit thread override keeps the idle panel in its own focus drawer", () => { + assert.equal( + shouldUseFocusIdleDrawer({ + channelManagementOpen: false, + hasAgentSession: false, + hasIdleAuxiliaryPanel: true, + hasIdlePanelCloseHandler: true, + hasProfilePanel: false, + hasThreadSurface: true, + overrideThread: true, + useSplitAuxiliaryPane: false, + }), + true, + ); +}); + test("getChannelIntroKind names project homes ahead of regular streams", () => { assert.equal(getChannelIntroKind(channel(), true), "project channel"); assert.equal(getChannelIntroKind(channel(), false), "regular channel"); diff --git a/desktop/src/features/channels/ui/ChannelPane.helpers.ts b/desktop/src/features/channels/ui/ChannelPane.helpers.ts index 695fef166f..f59568779d 100644 --- a/desktop/src/features/channels/ui/ChannelPane.helpers.ts +++ b/desktop/src/features/channels/ui/ChannelPane.helpers.ts @@ -10,6 +10,7 @@ export function shouldUseFocusIdleDrawer({ hasIdlePanelCloseHandler, hasProfilePanel, hasThreadSurface, + overrideThread = false, useSplitAuxiliaryPane, }: { channelManagementOpen: boolean; @@ -18,14 +19,15 @@ export function shouldUseFocusIdleDrawer({ hasIdlePanelCloseHandler: boolean; hasProfilePanel: boolean; hasThreadSurface: boolean; + overrideThread?: boolean; useSplitAuxiliaryPane: boolean; }): boolean { return ( - useSplitAuxiliaryPane && + (useSplitAuxiliaryPane || overrideThread) && !channelManagementOpen && !hasAgentSession && !hasProfilePanel && - !hasThreadSurface && + (!hasThreadSurface || overrideThread) && hasIdleAuxiliaryPanel && hasIdlePanelCloseHandler ); diff --git a/desktop/src/features/channels/ui/ChannelPane.tsx b/desktop/src/features/channels/ui/ChannelPane.tsx index 0d9a5eeb1e..38ebd9234a 100644 --- a/desktop/src/features/channels/ui/ChannelPane.tsx +++ b/desktop/src/features/channels/ui/ChannelPane.tsx @@ -30,6 +30,10 @@ import { AgentSessionThreadPanel } from "@/features/channels/ui/AgentSessionThre import { ChannelManagementAuxiliaryPanel } from "@/features/channels/ui/ChannelManagementAuxiliaryPanel"; import { IdleAuxiliaryPanel } from "@/features/channels/ui/IdleAuxiliaryPanel"; import { RightAuxiliaryPane } from "@/features/channels/ui/RightAuxiliaryPane"; +import { + ThreadPanelSurface, + useThreadPanelSurface, +} from "@/features/channels/ui/ThreadPanelSurface"; import { ThreadViewModeToggle } from "@/features/channels/ui/ThreadViewModeToggle"; import { FocusThreadDrawer } from "@/features/channels/ui/FocusThreadDrawer"; import { THREAD_SURFACE_KEY } from "@/features/channels/lib/threadFocusLayout"; @@ -419,10 +423,10 @@ export const ChannelPane = React.memo(function ChannelPane({ const isOverlay = useIsThreadPanelOverlay(); const useSplitAuxiliaryPane = !isSinglePanelView && !isOverlay; const threadViewMode = useThreadViewMode(); + const hasThreadSurface = + Boolean(threadHeadMessage) || shouldShowThreadSkeleton; const useFocusThreadDrawer = - threadViewMode === "focus" && - useSplitAuxiliaryPane && - (Boolean(threadHeadMessage) || shouldShowThreadSkeleton); + threadViewMode === "focus" && useSplitAuxiliaryPane && hasThreadSurface; const selectedAgent = React.useMemo( () => agentSessionSelection.resolveSelectedAgentSession({ @@ -435,19 +439,26 @@ export const ChannelPane = React.memo(function ChannelPane({ ); const hasIdleAuxiliary = Boolean(idleAuxiliaryPanel) && Boolean(onCloseIdleAuxiliaryPanel); + const priorityIdleAuxiliary = shouldPrioritizeIdleAuxiliary( + idleAuxiliaryOverridesThread, + hasIdleAuxiliary, + ); + const overlayIdleAuxiliaryOverThread = + priorityIdleAuxiliary && hasThreadSurface && !isOverlay; + const replaceThreadWithIdleAuxiliary = + priorityIdleAuxiliary && hasThreadSurface && isOverlay; const useFocusIdleDrawer = shouldUseFocusIdleDrawer({ channelManagementOpen, hasAgentSession: Boolean(activeChannel && selectedAgent), hasIdleAuxiliaryPanel: Boolean(idleAuxiliaryPanel), hasIdlePanelCloseHandler: Boolean(onCloseIdleAuxiliaryPanel), hasProfilePanel: Boolean(profilePanelPubkey), - hasThreadSurface: Boolean(threadHeadMessage) || shouldShowThreadSkeleton, + hasThreadSurface, + overrideThread: overlayIdleAuxiliaryOverThread, useSplitAuxiliaryPane, }); - const priorityIdleAuxiliary = shouldPrioritizeIdleAuxiliary( - idleAuxiliaryOverridesThread, - hasIdleAuxiliary, - ); + const showIdleAuxiliaryOverThread = + overlayIdleAuxiliaryOverThread && useFocusIdleDrawer; const { channelIsCovered, markExitComplete } = useFocusDrawerPresence( useFocusThreadDrawer || useFocusIdleDrawer, priorityIdleAuxiliary @@ -456,6 +467,10 @@ export const ChannelPane = React.memo(function ChannelPane({ ? onCloseThread : (onCloseIdleAuxiliaryPanel ?? onCloseThread), ); + const threadSurface = useThreadPanelSurface( + showIdleAuxiliaryOverThread, + markExitComplete, + ); const { changeThreadViewMode, layoutScrollTargetId, resolveScrollTarget } = useThreadViewModeSwitch({ activeThreadHeadId: threadHeadMessage?.id ?? null, @@ -506,19 +521,19 @@ export const ChannelPane = React.memo(function ChannelPane({ ) : ( {panel} ); - const wrapThreadPanel = (panel: React.ReactNode) => - useFocusThreadDrawer ? ( - - {panel} - - ) : ( - wrapAux(panel, "message-thread-panel", { key: THREAD_SURFACE_KEY }) - ); + const wrapThreadPanel = (panel: React.ReactNode) => ( + + {useFocusThreadDrawer ? panel : wrapAux(panel, "message-thread-panel")} + + ); const wrapIdlePanel = (panel: React.ReactNode) => useFocusIdleDrawer && onCloseIdleAuxiliaryPanel ? ( {panel} @@ -779,10 +795,8 @@ export const ChannelPane = React.memo(function ChannelPane({ } showTopBorder={false} /> - {/* The activity accessory is anchored in the dock's reserved - bottom rail, so fading it cannot change the observed - overlay height or move the conversation. Its natural - content height remains responsive. */} + {/* The reserved bottom rail keeps accessory fades from moving + the conversation while content remains responsive. */} - ) : priorityIdleAuxiliary && idleAuxiliarySurface ? ( + ) : replaceThreadWithIdleAuxiliary && idleAuxiliarySurface ? ( idleAuxiliarySurface ) : threadHeadMessage ? ( (() => { @@ -977,6 +991,9 @@ export const ChannelPane = React.memo(function ChannelPane({ idleAuxiliarySurface )} + + {showIdleAuxiliaryOverThread ? idleAuxiliarySurface : null} + ); }); diff --git a/desktop/src/features/channels/ui/FocusThreadDrawer.tsx b/desktop/src/features/channels/ui/FocusThreadDrawer.tsx index 626dbbddc2..82ecfe6440 100644 --- a/desktop/src/features/channels/ui/FocusThreadDrawer.tsx +++ b/desktop/src/features/channels/ui/FocusThreadDrawer.tsx @@ -11,10 +11,14 @@ import { cn } from "@/shared/lib/cn"; type FocusThreadDrawerProps = { channelName: string; children: React.ReactNode; + /** Prevent a covered drawer from handling Escape before its overlay. */ + escapeEnabled?: boolean; /** Accessible name for the drawer. Channel threads leave the default. */ label?: string; hasActiveEdit?: boolean; onClose: () => void; + /** Resolve an explicit focus target after this drawer has been dismissed. */ + restoreFocusTarget?: () => HTMLElement | null; }; /** @@ -120,6 +124,46 @@ const EXIT_TRANSITION = { */ const REDUCED_MOTION_TRANSITION = { duration: 0.12, ease: "linear" } as const; +function useViewportRightInsetPx( + overlayRef: React.RefObject, +) { + const [rightInsetPx, setRightInsetPx] = React.useState(0); + + React.useLayoutEffect(() => { + const layoutRoot = overlayRef.current?.parentElement; + if (!layoutRoot) return; + + const updateRightInset = () => { + const bounds = layoutRoot.getBoundingClientRect(); + const overflowPx = Math.max(0, bounds.right - window.innerWidth); + const nextRightInsetPx = Math.min(bounds.width, Math.ceil(overflowPx)); + setRightInsetPx((current) => + current === nextRightInsetPx ? current : nextRightInsetPx, + ); + }; + + updateRightInset(); + window.addEventListener("resize", updateRightInset); + + const observer = + typeof ResizeObserver === "undefined" + ? null + : new ResizeObserver(updateRightInset); + let ancestor: HTMLElement | null = layoutRoot; + while (ancestor) { + observer?.observe(ancestor); + ancestor = ancestor.parentElement; + } + + return () => { + observer?.disconnect(); + window.removeEventListener("resize", updateRightInset); + }; + }, [overlayRef]); + + return rightInsetPx; +} + /** * Right-anchored thread drawer that overlays the channel content area. * @@ -142,16 +186,22 @@ const REDUCED_MOTION_TRANSITION = { duration: 0.12, ease: "linear" } as const; export function FocusThreadDrawer({ channelName, children, + escapeEnabled = true, label = "Thread", hasActiveEdit = false, onClose, + restoreFocusTarget, }: FocusThreadDrawerProps) { const prefersReducedMotion = useReducedMotion(); const travelPx = prefersReducedMotion ? 0 : THREAD_FOCUS_DRAWER_TRAVEL_PX; const drawerRef = React.useRef(null); + const overlayRef = React.useRef(null); const previousFocusRef = React.useRef(null); + const viewportRightInsetPx = useViewportRightInsetPx(overlayRef); React.useEffect(() => { + if (!escapeEnabled) return; + function handleEscape(event: KeyboardEvent) { if (event.key !== "Escape") return; const target = event.target; @@ -171,7 +221,7 @@ export function FocusThreadDrawer({ return () => { window.removeEventListener("keydown", handleEscape, { capture: true }); }; - }, [hasActiveEdit, onClose]); + }, [escapeEnabled, hasActiveEdit, onClose]); React.useLayoutEffect(() => { previousFocusRef.current = @@ -183,6 +233,11 @@ export function FocusThreadDrawer({ return () => { const previousFocus = previousFocusRef.current; requestAnimationFrame(() => { + const explicitTarget = restoreFocusTarget?.(); + if (explicitTarget) { + explicitTarget.focus({ preventScroll: true }); + return; + } // A real dismissal keeps focus mode selected; a presentation switch // has already selected split mode and owns focus inside the new panel. if (getThreadViewMode() === "focus") { @@ -190,12 +245,14 @@ export function FocusThreadDrawer({ } }); }; - }, []); + }, [restoreFocusTarget]); return (
void; +}; + +/** Keeps a thread mounted while controlling its focus-drawer presentation. */ +export const ThreadPanelSurface = React.forwardRef< + HTMLDivElement, + ThreadPanelSurfaceProps +>(function ThreadPanelSurface( + { channelName, children, covered, hasActiveEdit, isFocusDrawer, onClose }, + ref, +) { + return ( +
+ {isFocusDrawer ? ( + + {children} + + ) : ( + children + )} +
+ ); +}); + +/** Supplies covered-thread lifecycle and focus ownership for an overlay drawer. */ +export function useThreadPanelSurface( + open: boolean, + onExitComplete: () => void, +) { + const ref = React.useRef(null); + const coverage = usePresenceCoverage(open); + const markExitComplete = React.useCallback(() => { + coverage.markExitComplete(); + onExitComplete(); + }, [coverage.markExitComplete, onExitComplete]); + const restoreFocusTarget = React.useCallback( + () => + ref.current?.querySelector( + '[data-testid="auxiliary-panel-close"]', + ) ?? null, + [], + ); + return { ...coverage, markExitComplete, ref, restoreFocusTarget }; +} diff --git a/desktop/src/features/channels/ui/useFocusDrawerPresence.ts b/desktop/src/features/channels/ui/useFocusDrawerPresence.ts index 271c867ae3..660e8f741c 100644 --- a/desktop/src/features/channels/ui/useFocusDrawerPresence.ts +++ b/desktop/src/features/channels/ui/useFocusDrawerPresence.ts @@ -2,22 +2,32 @@ import * as React from "react"; import { subscribeToFocusedThreadCloseRequest } from "@/features/channels/focusedThreadCloseRequest"; -/** Keeps the covered channel inert and owns external dismissal while open. */ -export function useFocusDrawerPresence(open: boolean, onClose: () => void) { +/** Retains coverage until the owning presence boundary completes its exit. */ +export function usePresenceCoverage(open: boolean) { const [present, setPresent] = React.useState(false); React.useEffect(() => { if (open) setPresent(true); }, [open]); + const markExitComplete = React.useCallback(() => setPresent(false), []); + return { + covered: open || present, + markExitComplete, + }; +} + +/** Keeps the covered channel inert and owns external dismissal while open. */ +export function useFocusDrawerPresence(open: boolean, onClose: () => void) { + const { covered, markExitComplete } = usePresenceCoverage(open); + React.useEffect(() => { if (!open) return; return subscribeToFocusedThreadCloseRequest(onClose); }, [onClose, open]); - const markExitComplete = React.useCallback(() => setPresent(false), []); return { - channelIsCovered: open || present, + channelIsCovered: covered, markExitComplete, }; } diff --git a/desktop/tests/e2e/project-commit-detail.spec.ts b/desktop/tests/e2e/project-commit-detail.spec.ts index 6c84a170a9..2ee454aefc 100644 --- a/desktop/tests/e2e/project-commit-detail.spec.ts +++ b/desktop/tests/e2e/project-commit-detail.spec.ts @@ -781,7 +781,7 @@ test("latest files commit opens its detail without a divider", async ({ await expect(page.getByTestId("project-commit-detail")).toBeVisible(); }); -test("project workspace sheet enters at its settled width", async ({ +test("project workspace sheet stays independent from an open thread", async ({ page, }) => { await enableProjectsFeature(page); @@ -792,6 +792,7 @@ test("project workspace sheet enters at its settled width", async ({ await page.getByTestId("create-project-name").fill("sheet-motion-demo"); await page.getByTestId("create-project-submit").click(); await expect(page.getByTestId("project-channel-home")).toBeVisible(); + await page.setViewportSize({ height: 720, width: 820 }); const summaryColumn = page.getByTestId("project-home-summary-column"); const resizeHandle = summaryColumn.getByTestId( @@ -822,6 +823,153 @@ test("project workspace sheet enters at its settled width", async ({ expect( Math.abs(settledDrawerWidth - enteringDrawerWidth), ).toBeLessThanOrEqual(1); + + await focusDrawer.getByTestId("auxiliary-panel-close").click(); + await expect(page.getByTestId("project-home-workspace-sheet")).toHaveCount(0); + await expect(page.getByTestId("project-home-summary-column")).toBeVisible(); + await waitForMockLiveSubscription(page, "sheet-motion-demo"); + const threadRootContent = "Workspace drawer thread root"; + await page.evaluate((content) => { + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "sheet-motion-demo", + content, + }); + }, threadRootContent); + const threadRoot = page + .getByTestId("message-timeline") + .getByTestId("message-row") + .filter({ hasText: threadRootContent }); + await expect(threadRoot).toBeVisible(); + await threadRoot.hover(); + await threadRoot.getByRole("button", { name: "Reply" }).click(); + await expect(page.getByTestId("message-thread-panel")).toBeVisible(); + + await page.getByTestId("project-home-context-tasks").click(); + await expect(page.getByTestId("project-home-workspace-sheet")).toBeVisible(); + const workspaceDrawer = page.getByTestId("focus-thread-drawer"); + await expect(workspaceDrawer).toHaveCount(1); + await expect( + workspaceDrawer.getByTestId("project-home-workspace-sheet"), + ).toBeVisible(); + await expect(page.getByTestId("message-thread-panel")).toHaveCount(1); + await expect(workspaceDrawer.getByTestId("message-thread-panel")).toHaveCount( + 0, + ); + const coveredThreadSurface = page.getByTestId("thread-surface"); + await expect(coveredThreadSurface).toHaveAttribute("inert", ""); + await expect(coveredThreadSurface).toHaveAttribute("aria-hidden", "true"); + const coveredSnapshot = await page.locator("body").ariaSnapshot(); + expect(coveredSnapshot).not.toContain(threadRootContent); + expect(coveredSnapshot).toContain("Tasks"); + expect(coveredSnapshot).toContain("Close panel"); + + const workspaceClose = workspaceDrawer.getByTestId("auxiliary-panel-close"); + await workspaceClose.focus(); + await expect(workspaceClose).toBeFocused(); + for (let index = 0; index < 8; index += 1) { + await page.keyboard.press("Tab"); + expect( + await page.evaluate( + () => + document.activeElement?.closest('[data-testid="thread-surface"]') !== + null, + ), + ).toBe(false); + } + + await workspaceClose.click(); + const exitingWorkspaceState = await page.evaluate(() => { + const workspaceSheet = document.querySelector( + '[data-testid="project-home-workspace-sheet"]', + ); + const threadSurface = document.querySelector( + '[data-testid="thread-surface"]', + ); + return { + threadAriaHidden: threadSurface?.getAttribute("aria-hidden"), + threadInert: threadSurface?.hasAttribute("inert") ?? false, + workspaceSheetMounted: workspaceSheet !== null, + }; + }); + expect(exitingWorkspaceState).toEqual({ + threadAriaHidden: "true", + threadInert: true, + workspaceSheetMounted: true, + }); + await expect(page.getByTestId("project-home-workspace-sheet")).toHaveCount(0); + await expect(page.getByTestId("message-thread-panel")).toBeVisible(); + const threadClose = coveredThreadSurface.getByTestId("auxiliary-panel-close"); + await expect(threadClose).toBeFocused(); + await expect(coveredThreadSurface).not.toHaveAttribute("inert", ""); + await expect(coveredThreadSurface).not.toHaveAttribute("aria-hidden", "true"); + + await page.evaluate(() => { + document.documentElement.style.fontSize = "140%"; + }); + await page.getByTestId("project-home-context-tasks").click(); + await expect(page.getByTestId("project-home-workspace-sheet")).toBeVisible(); + const enlargedTextWorkspaceClose = page + .getByTestId("focus-thread-drawer") + .getByTestId("auxiliary-panel-close"); + await expect + .poll(() => + enlargedTextWorkspaceClose.evaluate((element) => { + const bounds = element.getBoundingClientRect(); + return { + leftInsideViewport: bounds.left >= 0, + rightInsideViewport: bounds.right <= window.innerWidth, + viewportWidth: window.innerWidth, + }; + }), + ) + .toEqual({ + leftInsideViewport: true, + rightInsideViewport: true, + viewportWidth: 820, + }); + await enlargedTextWorkspaceClose.click(); + await expect(page.getByTestId("project-home-workspace-sheet")).toHaveCount(0); + await expect(page.getByTestId("message-thread-panel")).toBeVisible(); + await page.evaluate(() => { + document.documentElement.style.removeProperty("font-size"); + }); + + await page.setViewportSize({ height: 1080, width: 1920 }); + const splitThreadPane = page + .locator( + '[data-testid="message-thread-panel"]:has([data-testid="right-auxiliary-pane-resize-handle"])', + ) + .first(); + await expect(splitThreadPane).toBeVisible(); + const threadResizeHandle = splitThreadPane.getByTestId( + "right-auxiliary-pane-resize-handle", + ); + const threadResizeHandleBox = await threadResizeHandle.boundingBox(); + expect(threadResizeHandleBox).not.toBeNull(); + + await page.getByTestId("project-home-context-tasks").click(); + await expect(page.getByTestId("project-home-workspace-sheet")).toBeVisible(); + await waitForAnimations(page); + const workspaceCoversThreadDivider = await page.evaluate( + ({ x, y }) => { + return Boolean( + document + .elementFromPoint(x, y) + ?.closest('[data-testid="focus-thread-drawer"]'), + ); + }, + { + x: + (threadResizeHandleBox?.x ?? 0) + + (threadResizeHandleBox?.width ?? 0) / 2, + y: (threadResizeHandleBox?.y ?? 0) + 40, + }, + ); + expect(workspaceCoversThreadDivider).toBe(true); + await page.keyboard.press("Escape"); + await expect(page.getByTestId("project-home-workspace-sheet")).toHaveCount(0); + await expect(splitThreadPane).toBeVisible(); + await expect(threadClose).toBeFocused(); }); test("commit detail opens from the commits feed with a diff", async ({ From b622003f74aa5bf9b659786452813299a25e4897 Mon Sep 17 00:00:00 2001 From: Bradley Axen Date: Wed, 26 Aug 2026 19:00:45 -0700 Subject: [PATCH 02/37] broker: define the agent-to-broker action contract (#6742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the interface half of #6467. This is the contract for how a keyless agent asks a host to act on its behalf — and only that. No host, no transport, no signing, no Desktop code; the only implementation is a test double. An agent that holds no secret key still has to read its channels, answer a mention, react, keep its profile, address its memory, and create the agents it needs. This PR defines the vocabulary for those requests and the rules both sides must follow. **#6467's open question, answered: per-operation, not "sign this".** A raw signing primitive tells the host *who* is asking but not *what for*, so the only policies it can express are all-or-nothing. A closed set of named operations lets a host serve channel reads while refusing agent creation, and gives any later policy layer something to attach to. The cost is that adding an operation means changing the contract — which is the point. Signing, publishing, and the auth/encryption NIPs are deliberately *not* operations; they are mechanisms inside whoever holds the key, and a test pins that none of those names resolve. **Nine operations in v1:** read a channel (whole channel, one thread, or mentions-only — these differ by filter, not permission, so they share one operation), post, reply, react, set profile, derive a storage address, and create / update / delete agents. Presence and typing are deferred as housekeeping a host may decline regardless; streaming is deferred, so waking on a mention is a polled mentions-only read. Each operation has typed, validated arguments and a typed outcome. **What the contract enforces structurally rather than by instruction:** - *Retry means identical bytes.* A request is validated and serialized exactly once and the transport only ever sees that frozen form. There is no way to re-render it, so a retry can't drift and be mistaken for a conflicting request. - *Read cursors are opaque.* Host-issued strings, validated for shape only, never parsed or compared. The host owns ordering and paging stability instead of being locked into timestamps. - *Reads return signed Nostr events.* A keyless agent verifies authorship and content locally and trusts the host only for completeness and authorization. Reply ancestry and mentions are derived from the signed tags, so nothing can disagree with the signature. - *Validation is the only door to a response.* The transport primitive hands back an unjudged envelope; the only response a caller can actually obtain is one already checked against the request it answers — request id correlates, the outcome's operation matches, identifiers and cursors are well-formed. A mismatch counts as "no answer", not a failure verdict. The raw primitive can't be invoked from outside the crate (pinned by compile-failure checks), so skipping that step isn't an option. - *Authority is never asserted by a body.* The envelope has no requester, owner, scope, or credential field; all of that comes from the session the host authenticated. Ownership of a created agent is implicitly the requester, never a field — which is what lets agents own agents while the chain still ends at a human. - *No secret crosses the wire.* Agent creation returns a pubkey and handle, never the minted key. Every payload type rejects unknown fields, and a test pins the exact JSON key set of all eighteen argument and outcome types, so no field can be added without a reviewer touching that table. Two honest limits: a string can physically hold secret text, so keeping secrets out of message content is host policy; and nothing stops a host from *holding* keys — it just may not hand them over. - *Null is never legal; omission is the only spelling of absence.* Every optional member rejects an explicit null. Otherwise null and omitted would be indistinguishable, and a contradiction check on the response envelope could be skipped by sending one (a real hole in an earlier revision). Coverage is derived by walking every path in the real fixtures, so new optional members are covered automatically. Serializers that emit null by default (Go without `omitempty`, Python's default dump) need configuring — called out in the module docs. **Wire mapping.** One POST endpoint, JSON body is the frozen request, opaque bearer credential in the auth header. Every verdict the host reached — including failure and including a rejected credential — comes back as a well-formed envelope with 200, because a second copy of the verdict in the status line could only disagree with it. Clients still try to parse an envelope regardless of status (intermediaries may remap) and fall back to the status only when none is present. A rejected credential is a host verdict carrying the promise "the action did not run"; transport errors are reserved for "nothing can be concluded about side effects." Binding the credential to a specific agent and conversation is the host's job and deliberately unenforceable here. **Relationship to #6543.** Envelope, request-id rules, protocol version, the three-way succeeded / failed / indeterminate result, error codes, and the agent-management argument shapes carry over. Changed deliberately: pubkeys are a validated type rather than bare strings, "capability" became "action" throughout, and a new "unsupported" error lets a host decline a best-effort operation without it reading as a fault. Both PRs land in the same module path — whichever merges second needs a hand reconcile, which I own as the author of #6543. **Validation at `7c853e0fb`.** 299 tests in the sdk crate (full package run), workspace tests green apart from the pre-existing relay timeout flake, which I reproduced on unmodified `main`. Clippy with warnings denied, fmt, warning-free docs, and the pre-push gate all pass. Review findings were closed with mutation tests — each fix verified by reverting it and watching a named test fail. One of those exposed a gap in my own coverage (response-side fields weren't pinned); that's fixed and floored separately from the request side so neither can mask the other. **Size.** About 5,400 lines across 8 files, roughly half tests. Every source file is under the repo's 1,000-line cap except the test file; #6759 splits it mechanically, is stacked on this branch, retargets to `main` after merge, and lands before PR 2 of the stack. --------- Signed-off-by: ss-dev-00 Signed-off-by: Bradley Axen Signed-off-by: ss-core-02 Co-authored-by: ss-dev-00 Co-authored-by: ss-core-02 --- Cargo.toml | 2 +- crates/buzz-sdk/src/broker/actions/args.rs | 563 ++++ crates/buzz-sdk/src/broker/actions/mod.rs | 414 +++ .../buzz-sdk/src/broker/actions/outcomes.rs | 298 ++ crates/buzz-sdk/src/broker/client.rs | 223 ++ crates/buzz-sdk/src/broker/correlate.rs | 92 + crates/buzz-sdk/src/broker/mod.rs | 754 +++++ crates/buzz-sdk/src/broker/tests.rs | 2811 +++++++++++++++++ crates/buzz-sdk/src/broker/wire.rs | 107 + crates/buzz-sdk/src/lib.rs | 1 + 10 files changed, 5264 insertions(+), 1 deletion(-) create mode 100644 crates/buzz-sdk/src/broker/actions/args.rs create mode 100644 crates/buzz-sdk/src/broker/actions/mod.rs create mode 100644 crates/buzz-sdk/src/broker/actions/outcomes.rs create mode 100644 crates/buzz-sdk/src/broker/client.rs create mode 100644 crates/buzz-sdk/src/broker/correlate.rs create mode 100644 crates/buzz-sdk/src/broker/mod.rs create mode 100644 crates/buzz-sdk/src/broker/tests.rs create mode 100644 crates/buzz-sdk/src/broker/wire.rs diff --git a/Cargo.toml b/Cargo.toml index 78816ff482..8f5f949d74 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -77,7 +77,7 @@ postcard = { version = "1", default-features = false, features = ["use-std"] # Inter-relay mesh transport (buzz-relay-mesh) iroh = { version = "1.0.0-rc.0", default-features = false, features = ["tls-ring"] } -serde_json = "1" +serde_json = { version = "1", features = ["raw_value"] } serde_yaml = "0.9" evalexpr = "11" cron = "0.16" diff --git a/crates/buzz-sdk/src/broker/actions/args.rs b/crates/buzz-sdk/src/broker/actions/args.rs new file mode 100644 index 0000000000..1be4546ee1 --- /dev/null +++ b/crates/buzz-sdk/src/broker/actions/args.rs @@ -0,0 +1,563 @@ +//! Argument types — one per [`Action`], plus the tagged union that pairs a +//! wire action name with its arguments. Each type carries a `validated()` +//! returning a normalized copy; the shared validators and the contract-wide +//! strictness rules live in the [parent module](super). + +use serde::{Deserialize, Serialize}; + +use super::{ + absent_or_valued, absent_or_valued_hex64, channel, channel_id, content, cursor, event_id, + hex64_field, is_false, limit, mentions, optional, required, respond_to, validate_slug, Action, + PubkeyHex, DEFAULT_PAGE_LIMIT, MAX_ABOUT_CHARS, MAX_EMOJI_CHARS, MAX_NAME_CHARS, + MAX_PROMPT_CHARS, MAX_SCALAR_CHARS, +}; +use crate::SdkError; + +/// Arguments for `channel.read` — the one read action. +/// +/// 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. + #[serde(deserialize_with = "channel_id")] + pub channel_id: String, + /// Narrow to one thread by its root event. + #[serde( + default, + deserialize_with = "absent_or_valued_hex64", + skip_serializing_if = "Option::is_none" + )] + pub root_event_id: Option, + /// Narrow to messages mentioning the requester — the wake path. The + /// requester is never named; no body names its own subject. + #[serde(default, skip_serializing_if = "is_false")] + pub mentions_only: bool, + /// Opaque position to resume from, as returned in [`super::outcomes::MessagePage::next_cursor`]. + /// + /// Absent on a first read, which starts at the host's default window. + /// Callers must round-trip a cursor verbatim, never parse or synthesize + /// one: the host defines ordering and cursor stability, including whether + /// a cursor stays valid across restarts. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub cursor: Option, + /// Maximum events to return, capped at [`super::MAX_PAGE_LIMIT`]. + /// + /// Absent means [`super::DEFAULT_PAGE_LIMIT`], not "unbounded": see + /// [`Self::effective_limit`]. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub limit: Option, +} + +impl ChannelReadArgs { + /// The page size a response to these arguments is held to: explicit + /// `limit` when set, otherwise [`super::DEFAULT_PAGE_LIMIT`] — omitting a + /// limit asks for a sensible page, not an unbounded one. + #[must_use] + pub fn effective_limit(&self) -> u32 { + self.limit.unwrap_or(DEFAULT_PAGE_LIMIT) + } + + /// 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(), + ..Self::default() + } + } + + /// Validate and normalize. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed channel UUID, a + /// 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)?, + root_event_id: self + .root_event_id + .as_deref() + .map(|id| event_id(id, "rootEventId")) + .transpose()?, + mentions_only: self.mentions_only, + cursor: self.cursor.as_deref().map(cursor).transpose()?, + limit: limit(self.limit)?, + }) + } +} + +// ── Write arguments ───────────────────────────────────────────────────────── + +/// Arguments for `message.post`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct MessagePostArgs { + /// Channel to post in. + #[serde(deserialize_with = "channel_id")] + pub channel_id: String, + /// Message body. + pub content: String, + /// Pubkeys to notify. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub mentions: Vec, +} + +impl MessagePostArgs { + /// Validate and normalize. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed channel UUID or empty + /// content, [`SdkError::ContentTooLarge`] for oversized content, and + /// [`SdkError::TooManyMentions`] past [`super::MAX_MENTIONS`]. + pub fn validated(&self) -> Result { + Ok(Self { + channel_id: channel(&self.channel_id)?, + content: content(&self.content)?, + mentions: mentions(&self.mentions)?, + }) + } +} + +/// Arguments for `message.reply`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct MessageReplyArgs { + /// Channel containing the parent. + #[serde(deserialize_with = "channel_id")] + pub channel_id: String, + /// Event being replied to. + #[serde(deserialize_with = "hex64_field")] + pub reply_to_event_id: String, + /// Reply body. + pub content: String, + /// Pubkeys to notify. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub mentions: Vec, +} + +impl MessageReplyArgs { + /// Validate and normalize. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed channel UUID, a + /// malformed event id, or empty content; [`SdkError::ContentTooLarge`] for + /// oversized content; [`SdkError::TooManyMentions`] past [`super::MAX_MENTIONS`]. + pub fn validated(&self) -> Result { + Ok(Self { + channel_id: channel(&self.channel_id)?, + reply_to_event_id: event_id(&self.reply_to_event_id, "replyToEventId")?, + content: content(&self.content)?, + mentions: mentions(&self.mentions)?, + }) + } +} + +/// Arguments for `reaction.add`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct ReactionAddArgs { + /// Channel containing the target. + #[serde(deserialize_with = "channel_id")] + pub channel_id: String, + /// Event being reacted to. + #[serde(deserialize_with = "hex64_field")] + pub target_event_id: String, + /// Reaction payload — an emoji or a `:shortcode:`. + pub reaction: String, +} + +impl ReactionAddArgs { + /// Validate and normalize. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed channel UUID, a + /// malformed event id, or an empty reaction, and [`SdkError::EmojiTooLong`] + /// past [`MAX_EMOJI_CHARS`]. + pub fn validated(&self) -> Result { + let reaction = self.reaction.trim(); + if reaction.is_empty() { + return Err(SdkError::InvalidInput("reaction must not be empty".into())); + } + if reaction.chars().count() > MAX_EMOJI_CHARS { + return Err(SdkError::EmojiTooLong); + } + Ok(Self { + channel_id: channel(&self.channel_id)?, + target_event_id: event_id(&self.target_event_id, "targetEventId")?, + reaction: reaction.to_owned(), + }) + } +} + +/// Arguments for `profile.set`. +/// +/// Only the requester's own profile is addressable, so there is no subject +/// field. Absent fields are left as they are; the host does not clear them. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct ProfileSetArgs { + /// Replacement display name. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub display_name: Option, + /// Replacement bio. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub about: Option, + /// Replacement avatar URL. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub picture: Option, +} + +impl ProfileSetArgs { + /// Validate and normalize, requiring at least one field to change. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for an over-long field or a request + /// that changes nothing. + pub fn validated(&self) -> Result { + let normalized = Self { + display_name: optional(self.display_name.as_ref(), "display name", MAX_NAME_CHARS)?, + about: optional(self.about.as_ref(), "about", MAX_ABOUT_CHARS)?, + picture: optional(self.picture.as_ref(), "picture", MAX_SCALAR_CHARS)?, + }; + if normalized.display_name.is_none() + && normalized.about.is_none() + && normalized.picture.is_none() + { + return Err(SdkError::InvalidInput( + "include at least one profile field to set".into(), + )); + } + Ok(normalized) + } +} + +/// Arguments for `storage.address`. +/// +/// Deriving a record's address needs the secret this contract exists to avoid +/// holding, which is why it routes through the interface. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct StorageAddressArgs { + /// Memory slug — `core` or `mem/…`, per NIP-AE. + pub slug: String, +} + +impl StorageAddressArgs { + /// Validate and normalize. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] when the slug fails the NIP-AE + /// grammar. + pub fn validated(&self) -> Result { + let slug = required(&self.slug, "slug", 255)?; + validate_slug(&slug).map_err(|e| SdkError::InvalidInput(e.to_string()))?; + Ok(Self { slug }) + } +} + +// ── Agent arguments ───────────────────────────────────────────────────────── + +/// 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 { + /// Target by agent pubkey. + Pubkey(PubkeyHex), + /// Target by the agent's current name. + Name(String), +} + +impl AgentTarget { + /// Validate and normalize the selector. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for an empty or over-long name. + pub fn validated(&self) -> Result { + match self { + Self::Pubkey(pubkey) => Ok(Self::Pubkey(PubkeyHex::parse(pubkey.as_str())?)), + Self::Name(name) => Ok(Self::Name(required(name, "agent name", MAX_NAME_CHARS)?)), + } + } +} + +/// Arguments for `agents.create`. +/// +/// There is no owner field: the owner is whoever the host authenticated. See +/// the [contract docs](crate::broker) on ownership recursion. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct AgentsCreateArgs { + /// Channel the new agent is attached to. + #[serde(deserialize_with = "channel_id")] + pub channel_id: String, + /// Name for the new agent. + pub display_name: String, + /// Instructions the new agent runs with. + pub system_prompt: String, + /// Preferred harness id; the host refuses a runtime it cannot resolve. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub runtime: Option, + /// Inference provider. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub provider: Option, + /// Model identifier, interpreted relative to the runtime. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub model: Option, + /// Inbound author gate mode; absent = the host's owner-only default. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub respond_to: Option, +} + +impl AgentsCreateArgs { + /// Validate and normalize. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed channel UUID, an + /// empty or over-long name or prompt, or an unsupported respond-to mode. + pub fn validated(&self) -> Result { + Ok(Self { + channel_id: channel(&self.channel_id)?, + display_name: required(&self.display_name, "display name", MAX_NAME_CHARS)?, + system_prompt: required(&self.system_prompt, "system prompt", MAX_PROMPT_CHARS)?, + runtime: optional(self.runtime.as_ref(), "runtime", MAX_SCALAR_CHARS)?, + provider: optional(self.provider.as_ref(), "provider", MAX_SCALAR_CHARS)?, + model: optional(self.model.as_ref(), "model", MAX_SCALAR_CHARS)?, + respond_to: respond_to(self.respond_to.as_ref())?, + }) + } +} + +/// Arguments for `agents.update`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct AgentsUpdateArgs { + /// Which agent to patch. + pub target: AgentTarget, + /// Rename the agent. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub display_name: Option, + /// Replacement instructions. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub system_prompt: Option, + /// Harness id to pin. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub runtime: Option, + /// Inference provider. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub provider: Option, + /// Model identifier. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub model: Option, + /// Inbound author gate mode. + #[serde( + default, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub respond_to: Option, +} + +impl AgentsUpdateArgs { + /// Validate and normalize, requiring at least one field to change. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed target, an over-long + /// field, an unsupported respond-to mode, or a request that changes nothing. + pub fn validated(&self) -> Result { + let normalized = Self { + target: self.target.validated()?, + display_name: optional(self.display_name.as_ref(), "display name", MAX_NAME_CHARS)?, + system_prompt: optional( + self.system_prompt.as_ref(), + "system prompt", + MAX_PROMPT_CHARS, + )?, + runtime: optional(self.runtime.as_ref(), "runtime", MAX_SCALAR_CHARS)?, + provider: optional(self.provider.as_ref(), "provider", MAX_SCALAR_CHARS)?, + model: optional(self.model.as_ref(), "model", MAX_SCALAR_CHARS)?, + respond_to: respond_to(self.respond_to.as_ref())?, + }; + let unchanged = normalized.display_name.is_none() + && normalized.system_prompt.is_none() + && normalized.runtime.is_none() + && normalized.provider.is_none() + && normalized.model.is_none() + && normalized.respond_to.is_none(); + if unchanged { + return Err(SdkError::InvalidInput( + "include at least one field to update".into(), + )); + } + Ok(normalized) + } +} + +/// Arguments for `agents.delete`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct AgentsDeleteArgs { + /// Which agent to remove. + pub target: AgentTarget, +} + +impl AgentsDeleteArgs { + /// Validate and normalize. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed target selector. + pub fn validated(&self) -> Result { + Ok(Self { + target: self.target.validated()?, + }) + } +} + +// ── Action union ──────────────────────────────────────────────────────────── + +/// An action name paired with its strictly typed arguments. +/// +/// Flattened into [`crate::broker::BrokerRequest`], so the wire form is +/// `{ "action": "message.post", "args": { … } }` and an args shape can never be +/// paired with the wrong action name. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "action", content = "args", deny_unknown_fields)] +pub enum ActionArgs { + /// Read a channel, thread, or mention feed. + #[serde(rename = "channel.read")] + ChannelRead(ChannelReadArgs), + /// Post a message. + #[serde(rename = "message.post")] + MessagePost(MessagePostArgs), + /// Reply to a message. + #[serde(rename = "message.reply")] + MessageReply(MessageReplyArgs), + /// React to a message. + #[serde(rename = "reaction.add")] + ReactionAdd(ReactionAddArgs), + /// Set the requester's profile. + #[serde(rename = "profile.set")] + ProfileSet(ProfileSetArgs), + /// Derive an encrypted-memory address. + #[serde(rename = "storage.address")] + StorageAddress(StorageAddressArgs), + /// Mint a managed agent. + #[serde(rename = "agents.create")] + AgentsCreate(AgentsCreateArgs), + /// Patch a managed agent. + #[serde(rename = "agents.update")] + AgentsUpdate(AgentsUpdateArgs), + /// Remove a managed agent. + #[serde(rename = "agents.delete")] + AgentsDelete(AgentsDeleteArgs), +} + +impl ActionArgs { + /// The action these args belong to. + #[must_use] + pub fn action(&self) -> Action { + match self { + Self::ChannelRead(_) => Action::ChannelRead, + Self::MessagePost(_) => Action::MessagePost, + Self::MessageReply(_) => Action::MessageReply, + Self::ReactionAdd(_) => Action::ReactionAdd, + Self::ProfileSet(_) => Action::ProfileSet, + Self::StorageAddress(_) => Action::StorageAddress, + Self::AgentsCreate(_) => Action::AgentsCreate, + Self::AgentsUpdate(_) => Action::AgentsUpdate, + Self::AgentsDelete(_) => Action::AgentsDelete, + } + } + + /// Return a normalized copy with every field validated. + /// + /// There is deliberately no non-consuming `validate(&self)` beside this; + /// see [`crate::broker::BrokerRequest::validated`] for the trap it was. + /// + /// # Errors + /// + /// Propagates the per-action validation error. + pub fn validated(&self) -> Result { + Ok(match self { + Self::ChannelRead(args) => Self::ChannelRead(args.validated()?), + Self::MessagePost(args) => Self::MessagePost(args.validated()?), + Self::MessageReply(args) => Self::MessageReply(args.validated()?), + Self::ReactionAdd(args) => Self::ReactionAdd(args.validated()?), + Self::ProfileSet(args) => Self::ProfileSet(args.validated()?), + Self::StorageAddress(args) => Self::StorageAddress(args.validated()?), + Self::AgentsCreate(args) => Self::AgentsCreate(args.validated()?), + Self::AgentsUpdate(args) => Self::AgentsUpdate(args.validated()?), + Self::AgentsDelete(args) => Self::AgentsDelete(args.validated()?), + }) + } +} diff --git a/crates/buzz-sdk/src/broker/actions/mod.rs b/crates/buzz-sdk/src/broker/actions/mod.rs new file mode 100644 index 0000000000..c6f15cf42e --- /dev/null +++ b/crates/buzz-sdk/src/broker/actions/mod.rs @@ -0,0 +1,414 @@ +//! Broker actions — the closed set of operations an agent may ask a host to +//! perform. [`Action`] and the shared validators live here; the payload types +//! are split into [`args`] and [`outcomes`] so each side of a call reviews on +//! its own. + +use serde::{Deserialize, Serialize}; + +use crate::SdkError; +use buzz_core::engram::validate_slug; + +pub mod args; +pub mod outcomes; + +pub use args::{ + ActionArgs, AgentTarget, AgentsCreateArgs, AgentsDeleteArgs, AgentsUpdateArgs, ChannelReadArgs, + MessagePostArgs, MessageReplyArgs, ProfileSetArgs, ReactionAddArgs, StorageAddressArgs, +}; +pub use outcomes::{ + ActionOutcome, AgentsCreateOutcome, AgentsDeleteOutcome, AgentsUpdateOutcome, BrokerMessage, + EventPublished, MessagePage, StorageAddress, +}; + +/// Maximum characters in a display name or agent name. +pub const MAX_NAME_CHARS: usize = 120; + +/// Maximum characters in a system prompt. +pub const MAX_PROMPT_CHARS: usize = 20_000; + +/// Maximum characters in a short scalar field (runtime, provider, model). +pub const MAX_SCALAR_CHARS: usize = 300; + +/// Maximum characters in a profile `about` blurb. +pub const MAX_ABOUT_CHARS: usize = 2_000; + +/// Maximum bytes of message content, matching the SDK's channel-message cap. +pub const MAX_CONTENT_BYTES: usize = 64 * 1024; + +/// Maximum characters in a reaction payload (emoji or `:shortcode:`). +pub const MAX_EMOJI_CHARS: usize = 66; + +/// Maximum mentions attachable to one message. +pub const MAX_MENTIONS: usize = 50; + +/// Maximum events a single read may return. +pub const MAX_PAGE_LIMIT: u32 = 500; + +/// Events a read returns when the request sets no explicit `limit`. +/// +/// A caller that omits `limit` is not agreeing to an unbounded page, so this is +/// the number a response is held to in that case — see +/// [`crate::broker::BrokerResponse::validate_for`]. It is deliberately well +/// under [`MAX_PAGE_LIMIT`]: the cap is what a host may ever send, this is what +/// it may send unasked. +pub const DEFAULT_PAGE_LIMIT: u32 = 100; + +/// 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 +/// shape does not carry, and a mode without its list would mint an agent +/// nobody can talk to. +pub const RESPOND_TO_MODES: [&str; 2] = ["owner-only", "anyone"]; + +/// A public key in lowercase hex — the only identity this contract has. No +/// secret-key counterpart exists in this module (#6467's identity/signing +/// separation, made structural). +/// +/// A value of this type is a **real x-only secp256k1 point**, not just 64 hex +/// characters — most 32-byte values lie on no curve. Accepting shape alone +/// would defer the first real rejection to whichever consumer eventually +/// converts the string to a key, after the request was already accepted. The +/// curve check is the `nostr` crate's, so the contract and the events it +/// carries agree on what a key is by construction. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(try_from = "String", into = "String")] +pub struct PubkeyHex(String); + +impl PubkeyHex { + /// Parse a 64-character hex x-only public key, normalizing to lowercase. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] unless `value` is exactly 64 hex + /// characters **and** those bytes are a point on secp256k1. + pub fn parse(value: impl AsRef) -> Result { + let value = value.as_ref().trim(); + if value.len() != 64 || !value.chars().all(|c| c.is_ascii_hexdigit()) { + return Err(SdkError::InvalidInput( + "pubkey must be 64 hex characters".into(), + )); + } + let value = value.to_ascii_lowercase(); + // `from_hex` only decodes hex; `xonly` is what actually rejects a + // value that is not on the curve. + nostr::PublicKey::from_hex(&value) + .and_then(|key| key.xonly().map(|_| ())) + .map_err(|_| { + SdkError::InvalidInput("pubkey is not a valid secp256k1 x-only public key".into()) + })?; + Ok(Self(value)) + } + + /// The hex representation. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl TryFrom for PubkeyHex { + type Error = SdkError; + + fn try_from(value: String) -> Result { + Self::parse(value) + } +} + +impl From for String { + fn from(value: PubkeyHex) -> Self { + value.0 + } +} + +impl std::fmt::Display for PubkeyHex { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +/// An action name the broker can dispatch. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum Action { + /// Read messages from a channel, thread, or mention feed after a cursor. + ChannelRead, + /// Post a top-level channel message. + MessagePost, + /// Reply to an existing message. + MessageReply, + /// React to an existing message. + ReactionAdd, + /// Publish the requester's own profile metadata. + ProfileSet, + /// Derive the address of one encrypted-memory record. + StorageAddress, + /// Mint a managed agent owned by the requester. + AgentsCreate, + /// Patch a managed agent the requester owns. + AgentsUpdate, + /// Remove a managed agent the requester owns. + AgentsDelete, +} + +impl Action { + /// Every action in this protocol version, in wire-name order. + pub const ALL: [Self; 9] = [ + Self::AgentsCreate, + Self::AgentsDelete, + Self::AgentsUpdate, + Self::ChannelRead, + Self::MessagePost, + Self::MessageReply, + Self::ProfileSet, + Self::ReactionAdd, + Self::StorageAddress, + ]; + + /// Stable wire name. + #[must_use] + pub fn as_str(self) -> &'static str { + match self { + Self::ChannelRead => "channel.read", + Self::MessagePost => "message.post", + Self::MessageReply => "message.reply", + Self::ReactionAdd => "reaction.add", + Self::ProfileSet => "profile.set", + Self::StorageAddress => "storage.address", + Self::AgentsCreate => "agents.create", + Self::AgentsUpdate => "agents.update", + Self::AgentsDelete => "agents.delete", + } + } + + /// The action contract version this build implements. + #[must_use] + pub fn current_version(self) -> u16 { + 1 + } + + /// Whether a host may refuse this action without harming the agent. + /// + /// #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) + } + + /// Resolve a wire name. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for an unknown action name. + pub fn parse(name: &str) -> Result { + Self::ALL + .into_iter() + .find(|action| action.as_str() == name) + .ok_or_else(|| SdkError::InvalidInput(format!("unknown broker action \"{name}\""))) + } +} + +// ── Shared validators ─────────────────────────────────────────────────────── + +fn is_false(value: &bool) -> bool { + !*value +} + +/// Deserialize an optional member that may be **absent but never `null`**. +/// +/// This is the contract's one spelling-of-absence rule, and this is its +/// canonical rationale. `#[serde(default)] Option` maps an explicit `null` +/// to `None`, *indistinguishable from absent* to downstream code — so a reader +/// that decides something from absence (the status match in +/// [`crate::broker::BrokerResponse`], or +/// [`args::ChannelReadArgs::effective_limit`]) would silently treat a member +/// the sender did supply as one it did not. In the response envelope that was +/// a real hole: `{"status":"failed","outcome":null}` parsed as a plain failure +/// and skipped the per-status contradiction check. Rejecting `null` outright +/// leaves exactly one way to say "absent" and no layer guessing what a +/// present-but-empty member meant. +/// +/// Used with `#[serde(default, deserialize_with = "…")]`: serde calls this only +/// when the key is present, so reaching the `None` arm below means the member +/// was present and `null`. `deny_unknown_fields` stays in force alongside it. +/// +/// A required member of a non-`Option` type already rejects `null` as a type +/// error; the guard is only load-bearing where `Option` plus `default` would +/// otherwise conflate `null` with absent. +pub(super) fn absent_or_valued<'de, T, D>(deserializer: D) -> Result, D::Error> +where + T: Deserialize<'de>, + D: serde::Deserializer<'de>, +{ + use serde::de::Error as _; + + match Option::::deserialize(deserializer)? { + Some(value) => Ok(Some(value)), + None => Err(D::Error::custom( + "must not be null; omit the member to mean absent", + )), + } +} + +fn required(value: &str, label: &str, max: usize) -> Result { + let value = value.trim(); + if value.is_empty() { + return Err(SdkError::InvalidInput(format!("{label} must not be empty"))); + } + if value.chars().count() > max { + return Err(SdkError::InvalidInput(format!( + "{label} is too long (max {max} characters)" + ))); + } + Ok(value.to_owned()) +} + +fn optional(value: Option<&String>, label: &str, max: usize) -> Result, SdkError> { + value.map(|value| required(value, label, max)).transpose() +} + +/// Validate a channel id and return its **canonical** spelling — lowercase +/// hyphenated. `Uuid::parse_str` accepts several spellings of one channel, and +/// freezing the caller's spelling would make the host's canonical echo of the +/// same identity look like a mismatch in +/// [`crate::broker::BrokerResponse::validate_for`]. Same treatment +/// [`PubkeyHex::parse`] gives the other identity in this contract. +fn channel(value: &str) -> Result { + let value = required(value, "channel", 128)?; + uuid::Uuid::parse_str(&value) + .map(|id| id.as_hyphenated().to_string()) + .map_err(|_| SdkError::InvalidInput(format!("invalid channel UUID: {value}"))) +} + +/// Deserialize a `channelId`, canonicalizing it and rejecting a non-UUID. +/// +/// The wire is the one door a validator cannot cover: fields holding a channel +/// id are public `String`s, so a payload parsed from JSON reaches a caller +/// without passing through any `validated()`. Delegating to [`channel`] keeps +/// the wire form and the constructed form canonicalized by the same code. +pub(super) fn channel_id<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + use serde::de::Error as _; + + let raw = String::deserialize(deserializer)?; + channel(&raw).map_err(D::Error::custom) +} + +fn event_id(value: &str, label: &str) -> Result { + let value = required(value, label, 64)?; + if value.len() != 64 || !value.chars().all(|c| c.is_ascii_hexdigit()) { + return Err(SdkError::InvalidInput(format!( + "{label} must be 64 hex characters" + ))); + } + Ok(value.to_ascii_lowercase()) +} + +/// Deserialize a 64-hex identifier (`eventId`, `dTag`), lowercasing it — +/// the [`channel_id`] rule applied to the contract's other multi-spelling +/// identities. The label is generic because serde already reports which +/// member failed. +pub(super) fn hex64_field<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + use serde::de::Error as _; + + let raw = String::deserialize(deserializer)?; + event_id(&raw, "identifier").map_err(D::Error::custom) +} + +/// [`hex64_field`] for an optional member: `null` is still rejected (see +/// [`absent_or_valued`]). One function because `deserialize_with` takes one, +/// and both rules apply to the same member. +pub(super) fn absent_or_valued_hex64<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + use serde::de::Error as _; + + match Option::::deserialize(deserializer)? { + Some(raw) => Ok(Some( + event_id(&raw, "identifier").map_err(D::Error::custom)?, + )), + None => Err(D::Error::custom( + "must not be null; omit the member to mean absent", + )), + } +} + +fn content(value: &str) -> Result { + if value.trim().is_empty() { + return Err(SdkError::InvalidInput("content must not be empty".into())); + } + if value.len() > MAX_CONTENT_BYTES { + return Err(SdkError::ContentTooLarge { + max: MAX_CONTENT_BYTES, + got: value.len(), + }); + } + Ok(value.to_owned()) +} + +fn mentions(values: &[PubkeyHex]) -> Result, SdkError> { + if values.len() > MAX_MENTIONS { + return Err(SdkError::TooManyMentions); + } + values + .iter() + .map(|pubkey| PubkeyHex::parse(pubkey.as_str())) + .collect() +} + +fn limit(value: Option) -> Result, SdkError> { + match value { + None => Ok(None), + Some(0) => Err(SdkError::InvalidInput("limit must be at least 1".into())), + Some(limit) if limit > MAX_PAGE_LIMIT => Err(SdkError::InvalidInput(format!( + "limit exceeds {MAX_PAGE_LIMIT} (got {limit})" + ))), + Some(limit) => Ok(Some(limit)), + } +} + +/// 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() { + if !RESPOND_TO_MODES.contains(&mode) { + return Err(SdkError::InvalidInput(format!( + "respond-to must be one of {}", + RESPOND_TO_MODES.join(", ") + ))); + } + } + Ok(value) +} diff --git a/crates/buzz-sdk/src/broker/actions/outcomes.rs b/crates/buzz-sdk/src/broker/actions/outcomes.rs new file mode 100644 index 0000000000..064f3ed3aa --- /dev/null +++ b/crates/buzz-sdk/src/broker/actions/outcomes.rs @@ -0,0 +1,298 @@ +//! Outcome types — the success payload of each [`Action`], and the tagged union +//! that pairs a wire action name with its outcome. Outcomes are shared where +//! actions agree on what success means: the four event-publishing actions all +//! return [`EventPublished`]. + +use serde::{Deserialize, Serialize}; + +use super::{ + absent_or_valued, channel, channel_id, cursor, event_id, hex64_field, required, Action, + PubkeyHex, MAX_NAME_CHARS, MAX_PAGE_LIMIT, +}; +use crate::SdkError; +use nostr::{Event, EventId, Kind, PublicKey, Tags, Timestamp}; + +/// The seven canonical members of a Nostr event object, and nothing else. +/// +/// `nostr`'s own `Event` deserializer accepts and *discards* unknown members, +/// which would put read results outside the contract's strict-wire rule. +/// Routing through a `deny_unknown_fields` intermediary restores the rule at +/// the one place the contract does not own the type. +/// +/// Field names are the wire names from NIP-01 (`created_at`, not `createdAt`) — +/// this is the event's own encoding, not ours to rename. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct StrictEvent { + id: EventId, + pubkey: PublicKey, + created_at: Timestamp, + kind: Kind, + tags: Tags, + content: String, + sig: nostr::secp256k1::schnorr::Signature, +} + +/// One message returned by a read: the signed Nostr event, verbatim. +/// +/// The event is carried whole — signature and tags included — rather than +/// reduced to a projection, because Schnorr verification is local (see +/// [`Self::verify`]): a keyless agent gets independently verifiable authorship +/// and content, and only trusts the host for *completeness* and authorization. +/// Ancestry and mentions are derived accessors rather than sibling fields, so +/// nothing can disagree with the signed bytes. +/// +/// Deserialization is **strict**, via a private `deny_unknown_fields` +/// intermediary; serialization is the event's own, so the wire form is +/// unchanged. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(transparent)] +pub struct BrokerMessage(pub Event); + +impl<'de> Deserialize<'de> for BrokerMessage { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let strict = StrictEvent::deserialize(deserializer)?; + Ok(Self(Event::new( + strict.id, + strict.pubkey, + strict.created_at, + strict.kind, + strict.tags, + strict.content, + strict.sig, + ))) + } +} + +impl BrokerMessage { + /// The signed event. + #[must_use] + pub fn event(&self) -> &Event { + &self.0 + } + + /// Verify the event's id and Schnorr signature — entirely local; a host + /// that fabricated or altered a message fails here regardless of what it + /// claims. Deliberately *not* called by + /// [`crate::broker::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. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct MessagePage { + /// Messages in the host's declared order. + pub messages: Vec, + /// Opaque cursor to pass as [`super::args::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, + deserialize_with = "absent_or_valued", + skip_serializing_if = "Option::is_none" + )] + pub next_cursor: Option, +} + +/// Outcome of an action that published one event. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct EventPublished { + /// The published event's id (hex). + #[serde(deserialize_with = "hex64_field")] + pub event_id: String, + /// The published event's kind. + pub kind: u32, + /// Creation time the host stamped, Unix seconds. + pub created_at: u64, +} + +/// Outcome of `storage.address`. +/// +/// Addressing material only. A `d` tag is a keyed hash of the slug, so it +/// identifies a record without revealing the slug or the key that derived it. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct StorageAddress { + /// Author the record is addressed under. + pub author_pubkey: PubkeyHex, + /// Event kind holding the record. + pub kind: u32, + /// Derived `d` tag (64 hex characters). + #[serde(deserialize_with = "hex64_field")] + pub d_tag: String, +} + +/// Outcome of a successful `agents.create`. +/// +/// Carries the new agent's **public** identity only — there is no field for +/// the minted secret, and `deny_unknown_fields` plus the key-set test 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 { + /// The new agent's pubkey. + pub agent_pubkey: PubkeyHex, + /// The new agent's name as stored. + pub display_name: String, + /// Channel the agent was attached to. + #[serde(deserialize_with = "channel_id")] + pub channel_id: String, +} + +/// Outcome of a successful `agents.update`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct AgentsUpdateOutcome { + /// The patched agent's pubkey. + pub agent_pubkey: PubkeyHex, + /// The agent's name after the update. + pub display_name: String, + /// Names of the fields the host actually changed, sorted. + pub updated_fields: Vec, +} + +/// Outcome of a successful `agents.delete`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct AgentsDeleteOutcome { + /// The removed agent's pubkey. + pub agent_pubkey: PubkeyHex, + /// The removed agent's name. + pub display_name: String, +} + +/// An action-specific success payload. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "action", content = "outcome", deny_unknown_fields)] +pub enum ActionOutcome { + /// `channel.read` succeeded. + #[serde(rename = "channel.read")] + ChannelRead(MessagePage), + /// `message.post` succeeded. + #[serde(rename = "message.post")] + MessagePost(EventPublished), + /// `message.reply` succeeded. + #[serde(rename = "message.reply")] + MessageReply(EventPublished), + /// `reaction.add` succeeded. + #[serde(rename = "reaction.add")] + ReactionAdd(EventPublished), + /// `profile.set` succeeded. + #[serde(rename = "profile.set")] + ProfileSet(EventPublished), + /// `storage.address` succeeded. + #[serde(rename = "storage.address")] + StorageAddress(StorageAddress), + /// `agents.create` succeeded. + #[serde(rename = "agents.create")] + AgentsCreate(AgentsCreateOutcome), + /// `agents.update` succeeded. + #[serde(rename = "agents.update")] + AgentsUpdate(AgentsUpdateOutcome), + /// `agents.delete` succeeded. + #[serde(rename = "agents.delete")] + AgentsDelete(AgentsDeleteOutcome), +} + +impl ActionOutcome { + /// The action that produced this outcome. + #[must_use] + pub fn action(&self) -> Action { + match self { + Self::ChannelRead(_) => Action::ChannelRead, + Self::MessagePost(_) => Action::MessagePost, + Self::MessageReply(_) => Action::MessageReply, + Self::ReactionAdd(_) => Action::ReactionAdd, + Self::ProfileSet(_) => Action::ProfileSet, + Self::StorageAddress(_) => Action::StorageAddress, + Self::AgentsCreate(_) => Action::AgentsCreate, + Self::AgentsUpdate(_) => Action::AgentsUpdate, + 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. Signature verification is deliberately *not* here — see + /// [`BrokerMessage::verify`]. + /// + /// # 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(()) + } +} diff --git a/crates/buzz-sdk/src/broker/client.rs b/crates/buzz-sdk/src/broker/client.rs new file mode 100644 index 0000000000..3de4dbb896 --- /dev/null +++ b/crates/buzz-sdk/src/broker/client.rs @@ -0,0 +1,223 @@ +//! Client trait and HTTP binding for the broker contract. +//! +//! # HTTP binding +//! +//! ```text +//! POST /v1/action +//! Authorization: Bearer +//! Content-Type: application/json +//! +//! +//! ``` +//! +//! The response body is a [`BrokerResponse`] as JSON. Every terminal +//! disposition the *host* reached — including a rejected credential — is a +//! well-formed envelope returned with `200`: the verdict lives in `status`, +//! and a second copy in the status line could only ever disagree with it. A +//! client must nonetheless **attempt to parse an envelope regardless of HTTP +//! status** (an intermediary may map dispositions onto statuses); if a valid +//! envelope is present, it is the answer. Only when no envelope can be parsed +//! does the status matter, and then only as operator detail — see +//! [`BrokerTransportError`]. +//! +//! # The credential +//! +//! The credential is **opaque to this contract**: a bearer token the agent +//! received at startup and can only replay — not a key, not a signature. A +//! rejected credential is a host verdict, not a transport failure: it arrives +//! as `Failed` with [`super::BrokerErrorCode::Unauthenticated`], which carries +//! the promise that the action did not run. +//! +//! Binding a credential to a specific (agent, conversation) pair **is the +//! host's concern**; the request body carries no requester and no scope, +//! precisely so the host's binding is the only thing that decides authority. +//! Since the credential is the whole of the agent's authority, serving this +//! over anything but a loopback socket or TLS is publishing it. + +use std::future::Future; +use std::pin::Pin; + +use super::{BrokerResponse, BrokerResult, PreparedRequest}; + +/// Path of the single broker endpoint. +pub const BROKER_ACTION_PATH: &str = "/v1/action"; + +/// Header carrying the opaque session credential, as `Bearer `. +pub const BROKER_CREDENTIAL_HEADER: &str = "authorization"; + +/// No usable [`BrokerResponse`] was obtained, so the request's fate is unknown. +/// +/// 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 — 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), + /// 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, a malformed outcome, or a status + /// contradicting its own error code. + /// + /// A host that answers something other than what was asked has given no + /// verdict at all, which is why this is a transport failure rather than a + /// `Failed` result. + #[error("malformed broker response: {0}")] + MalformedResponse(String), +} + +/// A future returned by [`BrokerClient::send`]. +/// +/// Spelled as a boxed future rather than `async fn` in the trait because this +/// trait must be object-safe: the harness holds one client and must not know +/// whether it talks to an in-process host or an HTTP one. +pub type BrokerFuture<'a> = + Pin> + Send + 'a>>; + +/// A future returned by [`BrokerClientExt::execute`]. +pub type ValidatedFuture<'a> = + Pin> + Send + 'a>>; + +/// A host response that has been checked against the request it answers. +/// +/// The only way to obtain one is [`ValidatedResponse::validate`], which +/// [`BrokerClientExt::execute`] calls — so correlation is not advice an +/// implementation may skip. [`BrokerResponse::validate_for`] remains public for +/// a host validating its own output, but a client never has to remember to +/// call it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ValidatedResponse(BrokerResponse); + +impl ValidatedResponse { + /// Check `response` against the request it claims to answer. + /// + /// # Errors + /// + /// Returns [`BrokerTransportError::MalformedResponse`] when the response + /// does not correlate, carries an outcome for a different action, asserts a + /// malformed identifier, or pairs a status with a code that contradicts it. + /// A response that fails here is not a host verdict — nothing can be + /// concluded about side effects from it. + pub fn validate( + response: BrokerResponse, + request: &PreparedRequest, + ) -> Result { + response + .validate_for(request) + .map_err(|e| BrokerTransportError::MalformedResponse(e.to_string()))?; + Ok(Self(response)) + } + + /// The terminal disposition the host reached. + #[must_use] + pub fn result(&self) -> &BrokerResult { + &self.0.result + } + + /// The correlated `requestId`. + #[must_use] + pub fn request_id(&self) -> &str { + &self.0.request_id + } + + /// Whether the host replayed a previously recorded outcome. + #[must_use] + pub fn replayed(&self) -> bool { + self.0.replayed + } + + /// The underlying envelope, for logging or re-serialization. + #[must_use] + pub fn envelope(&self) -> &BrokerResponse { + &self.0 + } + + /// Consume this wrapper, yielding the validated envelope. + #[must_use] + pub fn into_envelope(self) -> BrokerResponse { + self.0 + } +} + +/// Permission to call [`BrokerClient::send`], which only +/// [`BrokerClientExt::execute`] can mint. +/// +/// This is what makes validation *structurally* the only door. `send` must be +/// public — an out-of-crate implementation has to define it — but a caller +/// must not be able to invoke it and receive an uncorrelated envelope. A token +/// with a private field satisfies both: any crate can accept one in a +/// signature, only this module can construct one. An implementation should +/// ignore its value; it carries no data. +/// +/// An implementation that stashes and exposes the envelope it saw has +/// deliberately built an exfiltrating transport — a different thing from a +/// caller forgetting to correlate. Closing the accidental path is the goal. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Dispatch(()); + +/// Something that can execute broker requests. +/// +/// One method, because there is one endpoint: every operation is an +/// [`super::Action`] inside the request, so adding an action never changes +/// this trait. This is the **transport primitive** — frozen bytes out, one +/// envelope back — not the caller's interface; callers use +/// [`BrokerClientExt::execute`], where response correlation happens. +/// +/// Implementations must be usable as `dyn BrokerClient`. +pub trait BrokerClient: Send + Sync { + /// Send `request`'s frozen bytes and return whatever envelope came back. + /// + /// An implementation's whole job is transport: send + /// [`PreparedRequest::body`] verbatim, parse an envelope regardless of + /// HTTP status, and return it unjudged. The [`Dispatch`] argument is why + /// this cannot be called directly by an outside caller; that is deliberate. + /// + /// # Errors + /// + /// Returns [`BrokerTransportError`] when no envelope could be obtained or + /// parsed. A host that answered — even to refuse — returns `Ok`, with the + /// verdict in [`BrokerResponse::result`]. + fn send<'a>(&'a self, request: &'a PreparedRequest, dispatch: Dispatch) -> BrokerFuture<'a>; +} + +/// The caller-facing half of [`BrokerClient`]: send, then validate. +/// +/// Blanket-implemented for every [`BrokerClient`], including `dyn BrokerClient`, +/// and **not overridable** — coherence forbids a second implementation, so there +/// is exactly one definition of what validating a response means and no client +/// can weaken it. +pub trait BrokerClientExt: BrokerClient { + /// Send `request` and return a response already checked against it. + /// + /// # Errors + /// + /// Returns [`BrokerTransportError`] when no envelope arrived, or + /// [`BrokerTransportError::MalformedResponse`] when the envelope that + /// arrived does not answer `request`. Both mean the same thing to a caller: + /// no verdict, so nothing is known about side effects. + fn execute<'a>(&'a self, request: &'a PreparedRequest) -> ValidatedFuture<'a>; +} + +impl BrokerClientExt for C { + fn execute<'a>(&'a self, request: &'a PreparedRequest) -> ValidatedFuture<'a> { + Box::pin(async move { + let response = self.send(request, Dispatch(())).await?; + ValidatedResponse::validate(response, request) + }) + } +} diff --git a/crates/buzz-sdk/src/broker/correlate.rs b/crates/buzz-sdk/src/broker/correlate.rs new file mode 100644 index 0000000000..b6d4baf6ef --- /dev/null +++ b/crates/buzz-sdk/src/broker/correlate.rs @@ -0,0 +1,92 @@ +//! Response-to-request identity correlation. +//! +//! Split from [`super`] to keep that file within the repo's 1,000-line +//! ceiling; the rule it implements is part of response validation. + +use super::{ActionArgs, ActionOutcome, AgentTarget, PubkeyHex, SdkError}; + +/// Reject an outcome that echoes back a different identity than the request +/// supplied. +/// +/// What is and is not compared, and why comparison is on parsed identities +/// rather than bytes, is documented once on +/// [`BrokerResponse::validate_for`][super::BrokerResponse::validate_for] — the +/// public entry point a host author reads. +/// +/// The match below is exhaustive over [`ActionArgs`], so adding an action is a +/// compile error here rather than a silent default to "not compared". +pub(super) fn correlate_identities( + args: &ActionArgs, + outcome: &ActionOutcome, +) -> Result<(), SdkError> { + /// Compare two channel ids as UUIDs, so two spellings of one channel match. + /// + /// An unparseable id on either side is a mismatch rather than an error: + /// `validate`/`validated` already reject a malformed channel id with a + /// precise message, and duplicating that verdict here would report a + /// correlation failure for what is really a malformed payload. + fn same_channel(requested: &str, returned: &str) -> Result<(), SdkError> { + let parse = |value: &str| uuid::Uuid::parse_str(value).ok(); + match (parse(requested), parse(returned)) { + (Some(requested_id), Some(returned_id)) if requested_id == returned_id => Ok(()), + _ => Err(mismatch("channelId", requested, returned)), + } + } + + /// Compare two pubkeys. [`PubkeyHex::parse`] is the type's only + /// constructor and lowercases, so comparing typed values *is* the parsed + /// comparison. + fn same_pubkey(requested: &PubkeyHex, returned: &PubkeyHex) -> Result<(), SdkError> { + if requested == returned { + return Ok(()); + } + Err(mismatch( + "agentPubkey", + requested.as_str(), + returned.as_str(), + )) + } + + /// The error every mismatch reports, naming the field and both spellings. + fn mismatch(field: &str, requested: &str, returned: &str) -> SdkError { + SdkError::InvalidInput(format!( + "response {field} \"{returned}\" does not match the requested \"{requested}\"" + )) + } + + /// The pubkey a target names immutably, if it names one. + fn targeted(target: &AgentTarget) -> Option<&PubkeyHex> { + match target { + AgentTarget::Pubkey(pubkey) => Some(pubkey), + AgentTarget::Name(_) => None, + } + } + + match (args, outcome) { + (ActionArgs::AgentsCreate(args), ActionOutcome::AgentsCreate(outcome)) => { + same_channel(&args.channel_id, &outcome.channel_id) + } + (ActionArgs::AgentsUpdate(args), ActionOutcome::AgentsUpdate(outcome)) => { + match targeted(&args.target) { + Some(requested) => same_pubkey(requested, &outcome.agent_pubkey), + None => Ok(()), + } + } + (ActionArgs::AgentsDelete(args), ActionOutcome::AgentsDelete(outcome)) => { + match targeted(&args.target) { + Some(requested) => same_pubkey(requested, &outcome.agent_pubkey), + None => Ok(()), + } + } + // These outcomes echo no identity the request supplied. + (ActionArgs::ChannelRead(_), _) + | (ActionArgs::MessagePost(_), _) + | (ActionArgs::MessageReply(_), _) + | (ActionArgs::ReactionAdd(_), _) + | (ActionArgs::ProfileSet(_), _) + | (ActionArgs::StorageAddress(_), _) + | (ActionArgs::AgentsCreate(_), _) + | (ActionArgs::AgentsUpdate(_), _) + | (ActionArgs::AgentsDelete(_), _) => Ok(()), + } +} diff --git a/crates/buzz-sdk/src/broker/mod.rs b/crates/buzz-sdk/src/broker/mod.rs new file mode 100644 index 0000000000..e1117cc810 --- /dev/null +++ b/crates/buzz-sdk/src/broker/mod.rs @@ -0,0 +1,754 @@ +//! Agent ↔ broker contract — the operations an agent asks a host to perform. +//! +//! This module is a **contract only**: the request envelope, the closed set of +//! [`Action`]s, the result shape, the HTTP binding, and a client trait. No +//! host, no transport, no signing. The full design rationale lives in the +//! English spec (`docs/agent-broker.md`); doc comments here explain only what +//! the code cannot say itself. +//! +//! ```text +//! agent → BrokerRequest → (POST /v1/action, bearer credential) → host +//! host: authenticate → authorize → validate → execute → BrokerResponse +//! ``` +//! +//! The agent holds its public key and a session credential — no secret key, no +//! relay connection. Everything it wants to do, reading included, is an action. +//! Actions are named business operations rather than a `sign(bytes)` primitive +//! so a host can hold per-operation policy; that and the rest of the +//! [#6467](https://github.com/block/buzz/issues/6467) mapping are covered in +//! the spec. +//! +//! # Contract-wide rules +//! +//! - **No secret crosses this boundary, in either direction.** Every wire type +//! is strict — unknown members are rejected at every depth, and each type's +//! exact key set is pinned by test. Where a derive would have left a lax +//! reader ([`BrokerResponse`], [`BrokerMessage`], [`BrokerResult`]), the type +//! documents how its strictness is restored. +//! - **Identities have exactly one spelling.** UUIDs and hex admit several +//! legal spellings, so every identity is canonicalized at both doors — +//! `validated()` and deserialization. See [`actions`]'s shared validators. +//! - **Omission is the only spelling of absence.** An explicit `null` is +//! rejected anywhere, at any depth. Canonical rationale on the +//! `absent_or_valued` guard in [`actions`]; host implementers must configure +//! serializers to omit unset members. +//! - **No request names its own subject.** Requester, owner, and scope are +//! derived from the authenticated credential; see [`BrokerRequest`]. This is +//! also why `agents.create` has no owner field — the creator owns the agent, +//! and the ownership chain always terminates at a human. Bounding its depth +//! is a host concern. +//! +//! Two limits worth stating: a `String` field can physically hold secret text, +//! so keeping secrets out of content and error messages is host policy; and +//! nothing stops a host from *holding* keys — that is the point. It stops one +//! from handing them over. +//! +//! # Deferred operations +//! +//! Not in v1, all purely additive later: memory read/write (intent-level +//! operations over the encrypted store — until then [`Action::StorageAddress`] +//! only addresses a record, and the key holder remains the only reader/writer), +//! `presence.set`, `typing.set`, and streaming reads (waking on a mention is +//! `channel.read` with `mentionsOnly`, polled). +//! +//! # Non-goals +//! +//! Hosts (auth, idempotency storage, execution), transports, relay changes, +//! grant/authorization fields (added when a real verifier exists, not before), +//! and secret-key custody. [`BrokerClient`] exists so in-process and HTTP +//! implementations are interchangeable; neither is here. + +use serde::{Deserialize, Serialize}; + +use crate::SdkError; + +pub mod actions; +pub mod client; +mod correlate; +mod wire; + +use actions::absent_or_valued; +pub use actions::{ + Action, ActionArgs, ActionOutcome, AgentTarget, AgentsCreateArgs, AgentsCreateOutcome, + AgentsDeleteArgs, AgentsDeleteOutcome, AgentsUpdateArgs, AgentsUpdateOutcome, BrokerMessage, + ChannelReadArgs, EventPublished, MessagePage, MessagePostArgs, MessageReplyArgs, + ProfileSetArgs, PubkeyHex, ReactionAddArgs, StorageAddress, StorageAddressArgs, +}; +pub use client::{ + BrokerClient, BrokerClientExt, BrokerFuture, BrokerTransportError, Dispatch, ValidatedFuture, + ValidatedResponse, BROKER_ACTION_PATH, BROKER_CREDENTIAL_HEADER, +}; + +/// Wire `type` discriminator for a broker request payload. +pub const BROKER_REQUEST_TYPE: &str = "broker_request"; + +/// Wire `type` discriminator for a broker response payload. +pub const BROKER_RESULT_TYPE: &str = "broker_result"; + +/// Current broker protocol version. +/// +/// There is no "absent means 1" compatibility rule: the protocol is unshipped, +/// so `protocolVersion` is required and an unknown value is rejected outright. +pub const BROKER_PROTOCOL_VERSION: u16 = 1; + +/// Maximum accepted length of a `requestId`, in bytes. +pub const MAX_REQUEST_ID_LEN: usize = 128; + +/// A request to execute one broker action. +/// +/// There is deliberately 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.** +/// +/// # Retry contract +/// +/// Retrying means resending the identical bytes with the same `requestId` — +/// the host compares a digest of the bytes against what it recorded under that +/// idempotency key (same digest → replay the recorded outcome; different → +/// [`BrokerErrorCode::RequestIdConflict`]). Two serializations of one value +/// can differ in bytes, so a client never sends this type directly: call +/// [`Self::prepare`] to freeze it into a [`PreparedRequest`] and hand *that* +/// to [`BrokerClientExt::execute`]. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields, rename_all = "camelCase")] +pub struct BrokerRequest { + /// Payload discriminator — must equal [`BROKER_REQUEST_TYPE`]. + pub r#type: String, + /// Protocol version — must equal [`BROKER_PROTOCOL_VERSION`]. + pub protocol_version: u16, + /// Caller-chosen idempotency key, unique per logical operation. + pub request_id: String, + /// Action contract version the caller wrote `args` against. + pub action_version: u16, + /// The action to invoke, with its strictly typed arguments. + #[serde(flatten)] + pub action: ActionArgs, +} + +impl BrokerRequest { + /// Build a request for `action` at the current protocol version. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] if `request_id` is empty, longer than + /// [`MAX_REQUEST_ID_LEN`], or not printable ASCII, or if the action's + /// arguments fail validation. + pub fn new(request_id: impl Into, action: ActionArgs) -> Result { + let request_id = request_id.into(); + validate_request_id(&request_id)?; + // Store the normalized copy, not the caller's, so a padded-but-valid + // value cannot travel in the frozen body. + let action = action.validated()?; + Ok(Self { + r#type: BROKER_REQUEST_TYPE.to_string(), + protocol_version: BROKER_PROTOCOL_VERSION, + request_id, + action_version: action.action().current_version(), + action, + }) + } + + /// The action this request invokes. + #[must_use] + pub fn action(&self) -> Action { + self.action.action() + } + + /// Validate and normalize into the only form execution-side code accepts. + /// + /// This is the **one normalization door**, and it consumes the request. + /// There is deliberately no non-consuming `validate(&self)`: a verdict + /// about a value it does not replace can drift from the value the caller + /// keeps holding (an earlier one validated a normalized copy, discarded + /// it, and let the caller execute the un-normalized original). The only + /// way to learn a request is valid is to receive the normalized + /// [`ValidatedRequest`]. + /// + /// # Errors + /// + /// Returns [`SdkError`] for a wrong `type`, an unsupported + /// `protocolVersion` or `actionVersion`, a malformed `requestId`, or + /// arguments that fail their own validation. + pub fn validated(mut self) -> Result { + self.validate_envelope()?; + self.action = self.action.validated()?; + Ok(ValidatedRequest(self)) + } + + /// Validate and normalize, then serialize once into the bytes every attempt + /// will send — [`Self::validated`] followed by [`ValidatedRequest::prepare`]. + /// + /// # Errors + /// + /// Returns [`SdkError`] when [`Self::validated`] fails, or + /// [`SdkError::InvalidInput`] if serialization fails. + pub fn prepare(self) -> Result { + self.validated()?.prepare() + } + + /// Validate everything except the action arguments, which + /// [`Self::validated`] normalizes in the same step. + fn validate_envelope(&self) -> Result<(), SdkError> { + if self.r#type != BROKER_REQUEST_TYPE { + return Err(SdkError::InvalidInput(format!( + "broker request type must be \"{BROKER_REQUEST_TYPE}\", got \"{}\"", + self.r#type + ))); + } + if self.protocol_version != BROKER_PROTOCOL_VERSION { + return Err(SdkError::InvalidInput(format!( + "unsupported broker protocolVersion {} (expected {BROKER_PROTOCOL_VERSION})", + self.protocol_version + ))); + } + validate_request_id(&self.request_id)?; + let action = self.action(); + if self.action_version != action.current_version() { + return Err(SdkError::InvalidInput(format!( + "unsupported actionVersion {} for {} (expected {})", + self.action_version, + action.as_str(), + action.current_version() + ))); + } + Ok(()) + } +} + +/// Validate a `requestId`: non-empty, bounded, printable ASCII without spaces. +/// +/// The bound and character set exist because this value becomes part of a +/// durable idempotency key and appears in audit records. +/// +/// # Errors +/// +/// Returns [`SdkError::InvalidInput`] when the id is empty, exceeds +/// [`MAX_REQUEST_ID_LEN`] bytes, or contains a byte outside `0x21..=0x7e`. +pub fn validate_request_id(request_id: &str) -> Result<(), SdkError> { + if request_id.is_empty() { + return Err(SdkError::InvalidInput("requestId must not be empty".into())); + } + if request_id.len() > MAX_REQUEST_ID_LEN { + return Err(SdkError::InvalidInput(format!( + "requestId exceeds {MAX_REQUEST_ID_LEN} bytes (got {})", + request_id.len() + ))); + } + if let Some(bad) = request_id + .bytes() + .find(|b| !(0x21..=0x7e).contains(b)) + .map(|b| format!("0x{b:02x}")) + { + return Err(SdkError::InvalidInput(format!( + "requestId must be printable ASCII without spaces (found byte {bad})" + ))); + } + Ok(()) +} + +/// A [`BrokerRequest`] that has been validated **and normalized**. +/// +/// The type execution-side code accepts. The only way to obtain one is +/// [`BrokerRequest::validated`], which normalizes on the way through, so +/// holding one proves the value carries what the validator approved — not the +/// caller's spelling of it. +/// +/// The inner request is private with no borrowing accessor: a borrow would let +/// execution-side code clone it, mutate a public field, and execute the result. +/// [`Self::into_request`] consumes the wrapper for a host that needs to move +/// the envelope onward; what it yields is no longer evidence of anything. +/// +/// A host that receives bytes builds one the same way a client does — parse, +/// call `validated()`, execute what comes back — since only its own verdict is +/// authoritative. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ValidatedRequest(BrokerRequest); + +impl ValidatedRequest { + /// The action to execute, with its normalized arguments. + #[must_use] + pub fn args(&self) -> &ActionArgs { + &self.0.action + } + + /// The action being invoked. + #[must_use] + pub fn action(&self) -> Action { + self.0.action() + } + + /// The idempotency key the host keys replay on. + #[must_use] + pub fn request_id(&self) -> &str { + &self.0.request_id + } + + /// Freeze the normalized request into the bytes every attempt will send. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] if serialization fails. + pub fn prepare(self) -> Result { + let body = serde_json::to_vec(&self.0).map_err(|e| { + SdkError::InvalidInput(format!("broker request is not serializable: {e}")) + })?; + Ok(PreparedRequest { + request: self.0, + body, + }) + } + + /// Consume this wrapper, yielding the normalized envelope — a plain + /// [`BrokerRequest`] with public fields, no longer evidence that anything + /// was validated, which is why this consumes rather than borrows. + #[must_use] + pub fn into_request(self) -> BrokerRequest { + self.0 + } +} + +/// A validated request together with the exact bytes to send. +/// +/// This is what [`BrokerClient::send`] takes, so the retry contract is +/// structural: every attempt sends `body` verbatim, and no implementation gets +/// the chance to reserialize. The typed request is deliberately not exposed — +/// only the correlation metadata ([`Self::request_id`], [`Self::action`]) an +/// implementation legitimately needs. +#[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 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. +/// +/// # Which status a code may carry +/// +/// A code and a [`BrokerResult`] status are two statements about the same +/// thing — whether side effects landed — so they cannot be paired freely. +/// `Failed` promises no side effects took hold; `Indeterminate` promises +/// nothing. This is the whole table, and it lives only here: +/// +/// | Code | with `failed` | with `indeterminate` | +/// |---|---|---| +/// | `outcome_unknown` | no | yes | +/// | `internal` | yes | yes | +/// | every other code | yes | no | +/// +/// [`Self::Internal`] is the one code legitimately either: a host fault before +/// dispatch is a known no-op, the same fault mid-execution is not. +/// [`Self::may_be_failed`] and [`Self::may_be_indeterminate`] are this table in +/// code, consulted by [`BrokerResponse::validate`], which rejects a mismatched +/// pairing as malformed rather than trusting either half. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum BrokerErrorCode { + /// The envelope or action arguments failed validation. + InvalidRequest, + /// The `protocolVersion` is not supported by this host. + UnsupportedProtocolVersion, + /// The action name is unknown to this host. + UnknownAction, + /// The `actionVersion` is not supported for this action. + UnsupportedActionVersion, + /// The host knows this action but does not offer it. + /// + /// For an action where [`Action::is_best_effort`] holds, this is a normal + /// answer and the agent carries on. Otherwise the agent cannot do its job + /// 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, + /// Reuse of a `requestId` with different request content. + RequestIdConflict, + /// The action ran and reported a domain failure. + ActionFailed, + /// The host could not determine whether side effects occurred. + OutcomeUnknown, + /// An unexpected host-side fault. + Internal, +} + +impl BrokerErrorCode { + /// Stable wire string for this code. + #[must_use] + pub fn as_str(self) -> &'static str { + match self { + Self::InvalidRequest => "invalid_request", + Self::UnsupportedProtocolVersion => "unsupported_protocol_version", + Self::UnknownAction => "unknown_action", + Self::UnsupportedActionVersion => "unsupported_action_version", + Self::Unsupported => "unsupported", + Self::Unauthenticated => "unauthenticated", + Self::Unauthorized => "unauthorized", + Self::RequestIdConflict => "request_id_conflict", + Self::ActionFailed => "action_failed", + Self::OutcomeUnknown => "outcome_unknown", + Self::Internal => "internal", + } + } + + /// Whether this code may appear with [`BrokerResult::Failed`]. + /// + /// One half of the table documented on [`BrokerErrorCode`], written as an + /// exhaustive match so adding a code forces a decision here. + #[must_use] + pub fn may_be_failed(self) -> bool { + match self { + Self::InvalidRequest + | Self::UnsupportedProtocolVersion + | Self::UnknownAction + | Self::UnsupportedActionVersion + | Self::Unsupported + | Self::Unauthenticated + | Self::Unauthorized + | Self::RequestIdConflict + | Self::ActionFailed + | Self::Internal => true, + Self::OutcomeUnknown => false, + } + } + + /// Whether this code may appear with [`BrokerResult::Indeterminate`] — + /// the other half of the table documented on [`BrokerErrorCode`]. + #[must_use] + pub fn may_be_indeterminate(self) -> bool { + match self { + Self::OutcomeUnknown | Self::Internal => true, + Self::InvalidRequest + | Self::UnsupportedProtocolVersion + | Self::UnknownAction + | Self::UnsupportedActionVersion + | Self::Unsupported + | Self::Unauthenticated + | Self::Unauthorized + | Self::RequestIdConflict + | Self::ActionFailed => false, + } + } +} + +/// A broker error: a machine-readable code plus a human-readable message. +/// +/// Messages are for operators and must never carry secrets — no nsec, no +/// credentials, no decrypted payloads. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct BrokerError { + /// Machine-readable failure code. + pub code: BrokerErrorCode, + /// Operator-facing description. Secret-free. + pub message: String, +} + +impl BrokerError { + /// Construct an error from a code and message. + pub fn new(code: BrokerErrorCode, message: impl Into) -> Self { + Self { + code, + message: message.into(), + } + } + + /// An [`BrokerErrorCode::InvalidRequest`] error. + pub fn invalid_request(message: impl Into) -> Self { + Self::new(BrokerErrorCode::InvalidRequest, message) + } + + /// An [`BrokerErrorCode::Unsupported`] error. + pub fn unsupported(message: impl Into) -> Self { + Self::new(BrokerErrorCode::Unsupported, message) + } + + /// An [`BrokerErrorCode::Unauthorized`] error. + pub fn unauthorized(message: impl Into) -> Self { + Self::new(BrokerErrorCode::Unauthorized, message) + } +} + +/// The terminal disposition of a broker request. +/// +/// A discriminated union, so "succeeded with an error" and "failed with an +/// outcome" are unrepresentable. [`Self::Indeterminate`] is distinct from +/// [`Self::Failed`] on purpose: `Failed` promises no side effects took hold, +/// `Indeterminate` promises nothing and demands reconciliation. Which +/// [`BrokerErrorCode`] may carry which status is a closed table on that type. +/// +/// # Why this type is not [`Deserialize`] +/// +/// Its members reach the wire only flattened into [`BrokerResponse`], whose +/// strict reader enforces the exact key set per status. A derived reader here +/// was a second, laxer door onto the same bytes — it accepted and dropped +/// members the envelope rejects — and two copies of a strictness check drift. +/// [`Serialize`] is retained (it produces the envelope's flattened wire form), +/// so this is a read-side restriction only. Nothing is lost: a bare +/// `{"status": …}` object is not a payload this contract defines. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(tag = "status", rename_all = "snake_case")] +pub enum BrokerResult { + /// The action completed and produced this outcome. + Succeeded { + /// Action-specific success payload. + #[serde(flatten)] + outcome: ActionOutcome, + }, + /// The action did not complete; no side effects are expected to persist. + Failed { + /// Why it failed. + error: BrokerError, + }, + /// Whether side effects occurred could not be determined. + Indeterminate { + /// What is unknown, and why. + error: BrokerError, + }, +} + +impl BrokerResult { + /// A successful result carrying `outcome`. + #[must_use] + pub fn succeeded(outcome: ActionOutcome) -> Self { + Self::Succeeded { outcome } + } + + /// A failed result carrying `error`. + #[must_use] + pub fn failed(error: BrokerError) -> Self { + Self::Failed { error } + } + + /// An indeterminate result carrying `error`. + #[must_use] + pub fn indeterminate(error: BrokerError) -> Self { + Self::Indeterminate { error } + } + + /// The outcome, when this is a success. + #[must_use] + pub fn outcome(&self) -> Option<&ActionOutcome> { + match self { + Self::Succeeded { outcome } => Some(outcome), + Self::Failed { .. } | Self::Indeterminate { .. } => None, + } + } + + /// The error, for the two non-success variants. + #[must_use] + pub fn error(&self) -> Option<&BrokerError> { + match self { + Self::Succeeded { .. } => None, + Self::Failed { error } | Self::Indeterminate { error } => Some(error), + } + } +} + +/// A broker result addressed back to the requester. +/// +/// `replayed` is **response metadata**: it describes this delivery, not the +/// domain outcome, and is never persisted as part of the stored result. A +/// replayed response is byte-identical in `result` to the original. +/// +/// # Why deserialization goes through an intermediary +/// +/// `#[serde(flatten)]` on `result` silently disables `deny_unknown_fields`, so +/// the derived reader accepted and discarded unknown members — exactly how a +/// secret-bearing host field crosses a boundary unnoticed. [`Deserialize`] +/// therefore routes through a private strict wire form (see `wire.rs`) with an +/// exact key set per status; anything else fails to parse and surfaces as +/// [`BrokerTransportError::MalformedResponse`]. Serialization is unchanged, and +/// a round-trip test pins that the strict reader accepts what the writer emits. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct BrokerResponse { + /// Payload discriminator — must equal [`BROKER_RESULT_TYPE`]. + pub r#type: String, + /// Protocol version — must equal [`BROKER_PROTOCOL_VERSION`]. + pub protocol_version: u16, + /// Correlates with the originating [`BrokerRequest::request_id`]. + pub request_id: String, + /// The terminal disposition. + #[serde(flatten)] + pub result: BrokerResult, + /// True when this response replays a previously recorded outcome. + /// + /// A plain `bool`, so it needs no explicit null guard: `null` already fails + /// as a type error rather than defaulting to `false`. + #[serde(default, skip_serializing_if = "is_false")] + pub replayed: bool, +} + +fn is_false(value: &bool) -> bool { + !*value +} + +impl BrokerResponse { + /// Build a fresh (non-replayed) response for `request_id`. + pub fn new(request_id: impl Into, result: BrokerResult) -> Self { + Self { + r#type: BROKER_RESULT_TYPE.to_string(), + protocol_version: BROKER_PROTOCOL_VERSION, + request_id: request_id.into(), + result, + replayed: false, + } + } + + /// Mark this response as replaying a recorded outcome. + #[must_use] + pub fn replayed(mut self) -> Self { + self.replayed = true; + self + } + + /// 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`, 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!( + "broker result type must be \"{BROKER_RESULT_TYPE}\", got \"{}\"", + self.r#type + ))); + } + if self.protocol_version != BROKER_PROTOCOL_VERSION { + return Err(SdkError::InvalidInput(format!( + "unsupported broker protocolVersion {} (expected {BROKER_PROTOCOL_VERSION})", + self.protocol_version + ))); + } + validate_request_id(&self.request_id)?; + match &self.result { + BrokerResult::Succeeded { outcome } => outcome.validate()?, + // A forbidden code/status pairing is a response contradicting + // itself; neither half can be trusted, so it is rejected. The + // table lives on `BrokerErrorCode`. + BrokerResult::Failed { error } if !error.code.may_be_failed() => { + return Err(SdkError::InvalidInput(format!( + "{} is not a valid code for a failed status", + error.code.as_str() + ))); + } + BrokerResult::Indeterminate { error } if !error.code.may_be_indeterminate() => { + return Err(SdkError::InvalidInput(format!( + "{} is not a valid code for an indeterminate status", + error.code.as_str() + ))); + } + 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 success for a different action, or for the wrong subject. A client + /// never calls this directly: [`BrokerClientExt::execute`] runs it for + /// every implementation and returns a [`ValidatedResponse`]. It stays + /// public for a host validating its own output. 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, a + /// success outcome names a different action than the request, a success + /// outcome echoes a different identity than the request supplied, or a + /// read returned more messages than the request allowed. + /// + /// # What identity correlation compares + /// + /// Every identity the request supplies and the outcome echoes must name + /// the same thing. Most outcomes echo nothing prior (host-minted ids, or a + /// page with no `channelId` echo; `storage.address` deliberately omits the + /// slug, whose `d` tag is a keyed hash of it). What remains: `agents.create` + /// compares `channelId` as UUIDs; `agents.update`/`agents.delete` compare + /// `agentPubkey` when targeted by pubkey. A name target is resolved + /// host-side and unverifiable by construction — a rename may be the very + /// thing the call performed. + /// + /// **Comparison is on parsed identities, never on bytes**: both identity + /// types admit more than one legal spelling, and a byte comparison would + /// reject a correct answer spelled differently — a worse failure than the + /// one this check exists to catch. + 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() + ))); + } + correlate::correlate_identities(&request.request.action, outcome)?; + // `ActionOutcome::validate` never sees the request, so it can only + // enforce the protocol-wide cap; the request's own limit is + // applied here, the one place both halves are in scope. + if let ( + ActionArgs::ChannelRead(args), + ActionOutcome::ChannelRead(MessagePage { messages, .. }), + ) = (&request.request.action, outcome) + { + let allowed = args.effective_limit() as usize; + if messages.len() > allowed { + return Err(SdkError::InvalidInput(format!( + "read returned {} messages for a limit of {allowed}", + messages.len() + ))); + } + } + } + Ok(()) + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/buzz-sdk/src/broker/tests.rs b/crates/buzz-sdk/src/broker/tests.rs new file mode 100644 index 0000000000..14a55005ae --- /dev/null +++ b/crates/buzz-sdk/src/broker/tests.rs @@ -0,0 +1,2811 @@ +//! 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"; +const EVENT: &str = "78d47c4f36a2d048f45b57a31d964a3ce239f0fc46162c5d7c90db2b5aa52bc6"; + +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) +} + +/// Every [`BrokerErrorCode`] variant, so code-driven tables cannot silently skip +/// one: [`error_codes_have_stable_wire_strings`] pins this list against the enum. +fn all_error_codes() -> [BrokerErrorCode; 11] { + use BrokerErrorCode as E; + [ + E::InvalidRequest, + E::UnsupportedProtocolVersion, + E::UnknownAction, + E::UnsupportedActionVersion, + E::Unsupported, + E::Unauthenticated, + E::Unauthorized, + E::RequestIdConflict, + E::ActionFailed, + E::OutcomeUnknown, + E::Internal, + ] +} + +/// One valid `args` value per action, so table-driven tests cannot silently +/// skip an action: [`fixtures_cover_every_action`] pins the coverage. +fn action_fixtures() -> Vec { + vec![ + 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(), + mentions: vec![pubkey()], + }), + ActionArgs::MessageReply(MessageReplyArgs { + channel_id: CHANNEL.into(), + reply_to_event_id: EVENT.into(), + content: "agreed".into(), + mentions: vec![pubkey()], + }), + ActionArgs::ReactionAdd(ReactionAddArgs { + channel_id: CHANNEL.into(), + target_event_id: EVENT.into(), + reaction: "🎉".into(), + }), + ActionArgs::ProfileSet(ProfileSetArgs { + display_name: Some("ss-dev-00".into()), + about: Some("implementation".into()), + picture: Some("https://example.invalid/avatar.png".into()), + }), + ActionArgs::StorageAddress(StorageAddressArgs { + slug: "mem/broker-foundation".into(), + }), + ActionArgs::AgentsCreate(AgentsCreateArgs { + channel_id: CHANNEL.into(), + display_name: "Research helper".into(), + system_prompt: "Find sources.".into(), + 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: 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()), + }), + ] +} + +/// One outcome per action, matching the fixture order above. +fn outcome_fixtures(keys: &Keys) -> Vec { + let page = MessagePage { + messages: vec![signed_message(keys)], + next_cursor: Some("opaque-host-cursor-v2".into()), + }; + let published = EventPublished { + event_id: EVENT.into(), + kind: 9, + created_at: 1_764_000_003, + }; + vec![ + ActionOutcome::ChannelRead(page), + ActionOutcome::MessagePost(published.clone()), + ActionOutcome::MessageReply(published.clone()), + ActionOutcome::ReactionAdd(published.clone()), + ActionOutcome::ProfileSet(published), + ActionOutcome::StorageAddress(StorageAddress { + author_pubkey: pubkey(), + kind: 30174, + d_tag: EVENT.into(), + }), + ActionOutcome::AgentsCreate(AgentsCreateOutcome { + agent_pubkey: pubkey(), + display_name: "Research helper".into(), + channel_id: CHANNEL.into(), + }), + ActionOutcome::AgentsUpdate(AgentsUpdateOutcome { + agent_pubkey: pubkey(), + display_name: "Research helper v2".into(), + updated_fields: vec!["displayName".into()], + }), + ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: pubkey(), + display_name: "Research helper".into(), + }), + ] +} + +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 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(&keys) + .iter() + .map(|outcome| outcome.action().as_str()) + .collect(); + let mut declared: Vec<&str> = Action::ALL.iter().map(|a| a.as_str()).collect(); + + from_args.sort_unstable(); + from_outcomes.sort_unstable(); + declared.sort_unstable(); + + assert_eq!(from_args, declared, "every action needs an args fixture"); + assert_eq!( + from_outcomes, declared, + "every action needs an outcome fixture" + ); + + let mut unique = declared.clone(); + unique.dedup(); + assert_eq!(unique.len(), declared.len(), "wire names must be unique"); +} + +// ── Envelope round-trip ───────────────────────────────────────────────────── + +#[test] +fn every_action_round_trips_through_a_request_envelope() { + for args in action_fixtures() { + let action = args.action(); + let request = BrokerRequest::new("req-1", args) + .unwrap_or_else(|e| panic!("{} fixture must validate: {e}", action.as_str())); + + let json = serde_json::to_value(&request).expect("request serializes"); + assert_eq!(json["type"], BROKER_REQUEST_TYPE); + assert_eq!(json["protocolVersion"], 1); + assert_eq!(json["requestId"], "req-1"); + assert_eq!(json["actionVersion"], 1); + assert_eq!( + json["action"], + action.as_str(), + "{} must name itself on the wire", + action.as_str() + ); + assert!( + json.get("args").is_some(), + "{} must carry an args object", + action.as_str() + ); + + let parsed: BrokerRequest = serde_json::from_value(json) + .unwrap_or_else(|e| panic!("{} must deserialize: {e}", action.as_str())); + assert_eq!(parsed, request); + parsed.validated().expect("round-tripped request is valid"); + } +} + +#[test] +fn every_outcome_round_trips_through_a_response_envelope() { + 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"); + + let json = serde_json::to_value(&response).expect("response serializes"); + assert_eq!(json["type"], BROKER_RESULT_TYPE); + assert_eq!(json["status"], "succeeded"); + assert_eq!(json["action"], action.as_str()); + assert!(json.get("error").is_none(), "a success carries no error"); + // `replayed` is delivery metadata and stays off the wire when false. + assert!(json.get("replayed").is_none()); + + let parsed: BrokerResponse = serde_json::from_value(json) + .unwrap_or_else(|e| panic!("{} outcome must deserialize: {e}", action.as_str())); + assert_eq!(parsed, response); + assert_eq!(parsed.result.outcome(), Some(&outcome)); + assert!(parsed.result.error().is_none()); + } +} + +/// Args and outcome share the `action` discriminator, so a payload can never +/// pair one action's name with another's shape. +#[test] +fn an_args_shape_cannot_be_paired_with_another_action_name() { + let json = serde_json::json!({ + "type": BROKER_REQUEST_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "actionVersion": 1, + "action": "agents.delete", + "args": { "channelId": CHANNEL, "content": "not a delete" }, + }); + assert!(serde_json::from_value::(json).is_err()); +} + +/// `#[serde(flatten)]` silently disables `deny_unknown_fields`, so the response +/// envelope — the one payload here that needs `flatten` for its wire shape — read +/// as strict while accepting and discarding extra keys. Every rejection below +/// parsed cleanly before the strict intermediary existed. +/// +/// The request envelope has the same `flatten` but *not* the same hole: its +/// `ActionArgs` is adjacently tagged, contributing exactly `action` and `args`, +/// so `deny_unknown_fields` still applies to the whole set. That is pinned in +/// [`a_request_envelope_rejects_anything_outside_its_exact_key_set`] rather than +/// assumed. +#[test] +fn a_response_envelope_rejects_anything_outside_its_exact_key_set() { + let succeeded = || { + serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "status": "succeeded", + "action": "agents.delete", + "outcome": { "agentPubkey": PUBKEY, "displayName": "Gone" }, + }) + }; + let failed = || { + serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "status": "failed", + "error": { "code": "action_failed", "message": "no" }, + }) + }; + assert!(serde_json::from_value::(succeeded()).is_ok()); + assert!(serde_json::from_value::(failed()).is_ok()); + + let mut rejected: Vec<(&str, serde_json::Value)> = Vec::new(); + + // An unknown top-level key, including one that reads as key material. + for extra in ["hostNote", "secretKey", "credential"] { + let mut json = succeeded(); + json[extra] = serde_json::json!("nsec1deadbeef"); + rejected.push((extra, json)); + let mut json = failed(); + json[extra] = serde_json::json!("nsec1deadbeef"); + rejected.push((extra, json)); + } + + // Members the declared status does not admit. Each of these is a + // contradiction the type system already forbids in Rust, and the envelope + // used to accept it on the wire and drop the half it could not represent. + let mut error_beside_success = succeeded(); + error_beside_success["error"] = serde_json::json!({ "code": "internal", "message": "?" }); + rejected.push(("error beside a success", error_beside_success)); + + let mut outcome_beside_failure = failed(); + outcome_beside_failure["action"] = serde_json::json!("agents.delete"); + outcome_beside_failure["outcome"] = + serde_json::json!({ "agentPubkey": PUBKEY, "displayName": "Gone" }); + rejected.push(("outcome beside a failure", outcome_beside_failure)); + + let mut outcome_beside_indeterminate = failed(); + outcome_beside_indeterminate["status"] = serde_json::json!("indeterminate"); + outcome_beside_indeterminate["error"] = + serde_json::json!({ "code": "outcome_unknown", "message": "?" }); + outcome_beside_indeterminate["action"] = serde_json::json!("agents.delete"); + outcome_beside_indeterminate["outcome"] = + serde_json::json!({ "agentPubkey": PUBKEY, "displayName": "Gone" }); + rejected.push(( + "outcome beside an indeterminate", + outcome_beside_indeterminate, + )); + + // Missing the member its status requires. + let mut no_outcome = succeeded(); + no_outcome.as_object_mut().unwrap().remove("outcome"); + rejected.push(("success with no outcome", no_outcome)); + let mut no_error = failed(); + no_error.as_object_mut().unwrap().remove("error"); + rejected.push(("failure with no error", no_error)); + + // An unknown status is not a fourth disposition to ignore. + for status in ["succeeded_partially", "pending", "SUCCEEDED", ""] { + let mut json = failed(); + json["status"] = serde_json::json!(status); + rejected.push(("unknown status", json)); + } + + // Strictness still reaches inside the outcome. + let mut extra_in_outcome = succeeded(); + extra_in_outcome["outcome"]["secretKey"] = serde_json::json!("nsec1deadbeef"); + rejected.push(("unknown key inside the outcome", extra_in_outcome)); + + let mut extra_in_error = failed(); + extra_in_error["error"]["secretKey"] = serde_json::json!("nsec1deadbeef"); + rejected.push(("unknown key inside the error", extra_in_error)); + + for (what, json) in rejected { + assert!( + serde_json::from_value::(json.clone()).is_err(), + "{what} must not deserialize: {json}" + ); + } +} + +/// Strict deserialization must not have narrowed what the writer emits: the +/// wire form is still the flattened one, and the strict reader is its inverse for +/// every status, with and without the optional `replayed`. +#[test] +fn the_strict_reader_accepts_exactly_what_the_writer_emits() { + let signer = Keys::generate(); + let mut results: Vec = outcome_fixtures(&signer) + .into_iter() + .map(BrokerResult::succeeded) + .collect(); + results.push(BrokerResult::failed(BrokerError::new( + BrokerErrorCode::ActionFailed, + "runtime not installed", + ))); + results.push(BrokerResult::indeterminate(BrokerError::new( + BrokerErrorCode::OutcomeUnknown, + "host restarted mid-execution", + ))); + + for result in results { + for replayed in [false, true] { + let response = if replayed { + BrokerResponse::new("req-1", result.clone()).replayed() + } else { + BrokerResponse::new("req-1", result.clone()) + }; + let json = serde_json::to_value(&response).expect("response serializes"); + let parsed: BrokerResponse = serde_json::from_value(json.clone()) + .unwrap_or_else(|e| panic!("strict reader rejected our own bytes {json}: {e}")); + assert_eq!(parsed, response); + assert_eq!(parsed.replayed, replayed); + } + } +} + +/// The request envelope flattens too, so it was checked for the same hole. It +/// does not have one — `ActionArgs` is adjacently tagged and contributes exactly +/// `action` and `args`, leaving `deny_unknown_fields` in force — and this pins +/// that, so the request side cannot regress into the response side's bug. +#[test] +fn a_request_envelope_rejects_anything_outside_its_exact_key_set() { + let valid = || { + serde_json::json!({ + "type": BROKER_REQUEST_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "actionVersion": 1, + "action": "channel.read", + "args": { "channelId": CHANNEL }, + }) + }; + assert!(serde_json::from_value::(valid()).is_ok()); + + // Unknown top-level key, beside the flattened discriminator, and inside the + // args — all four positions a smuggled field could take. + for extra in ["hostNote", "secretKey", "onBehalfOf", "envVars"] { + let mut json = valid(); + json[extra] = serde_json::json!("nsec1deadbeef"); + assert!( + serde_json::from_value::(json.clone()).is_err(), + "a request carrying top-level \"{extra}\" must not deserialize: {json}" + ); + + let mut json = valid(); + json["args"][extra] = serde_json::json!("nsec1deadbeef"); + assert!( + serde_json::from_value::(json.clone()).is_err(), + "a request carrying \"{extra}\" inside args must not deserialize: {json}" + ); + } + + // A second discriminator-shaped key is not a place to hide one either. + let mut extra_tag = valid(); + extra_tag["outcome"] = serde_json::json!({}); + assert!(serde_json::from_value::(extra_tag).is_err()); + + // Missing required members, so the pin cannot pass by accepting anything. + for missing in [ + "type", + "protocolVersion", + "requestId", + "actionVersion", + "args", + ] { + let mut json = valid(); + json.as_object_mut().unwrap().remove(missing); + assert!( + serde_json::from_value::(json).is_err(), + "a request missing \"{missing}\" must not deserialize" + ); + } +} + +/// Every JSON-pointer path to an object member reachable in `value`, including +/// members nested inside arrays, so a null-injection table cannot miss one. +fn member_paths(value: &serde_json::Value, prefix: &str, out: &mut Vec) { + match value { + serde_json::Value::Object(map) => { + for (key, child) in map { + // Escape per RFC 6901, so a key containing `/` or `~` still + // addresses the member it names. + let escaped = key.replace('~', "~0").replace('/', "~1"); + let path = format!("{prefix}/{escaped}"); + out.push(path.clone()); + member_paths(child, &path, out); + } + } + serde_json::Value::Array(items) => { + for (index, child) in items.iter().enumerate() { + member_paths(child, &format!("{prefix}/{index}"), out); + } + } + _ => {} + } +} + +/// The bug this guards: `#[serde(default)] Option` maps an explicit `null` to +/// `None`, which is indistinguishable from *absent*. The response envelope decides +/// its shape from absence, so `{"status":"failed","action":null,"outcome":null}` +/// and a succeeded response with `"error":null` both parsed as well-formed and +/// skipped the per-status contradiction check entirely — a malformed envelope +/// validating `Ok`. +/// +/// The rule adopted in response is uniform and therefore checkable: **no member +/// anywhere in this contract accepts an explicit `null`.** Nothing here emits one +/// (`skip_serializing_if` omits instead), so `null` is a second spelling of +/// "absent" that the contract simply does not define. One spelling means no layer +/// has to decide what a present-but-null member meant. +/// +/// This walks the real fixtures rather than a hand-written list of members, so an +/// optional field added later is covered without anyone remembering to add it +/// here. +#[test] +fn no_member_of_any_payload_accepts_an_explicit_null() { + let keys = Keys::generate(); + + // Requests: every action, with every optional member populated. + for args in action_fixtures() { + let request = BrokerRequest::new("req-1", args).expect("fixture request builds"); + let valid = serde_json::to_value(&request).expect("request serializes"); + // The untouched fixture must parse, or nulling members below would + // "reject" for a reason that has nothing to do with null. + assert_eq!( + serde_json::from_value::(valid.clone()).expect("fixture parses"), + request, + ); + + let mut paths = Vec::new(); + member_paths(&valid, "", &mut paths); + assert!( + paths.len() > 1, + "fixture should expose several members: {valid}" + ); + for path in paths { + let mut json = valid.clone(); + *json.pointer_mut(&path).expect("path addresses a member") = serde_json::Value::Null; + assert!( + serde_json::from_value::(json.clone()).is_err(), + "request with null at \"{path}\" must not deserialize: {json}" + ); + } + } + + // Responses: every outcome, plus both error-carrying statuses. + let mut results: Vec = outcome_fixtures(&keys) + .into_iter() + .map(BrokerResult::succeeded) + .collect(); + results.push(BrokerResult::failed(BrokerError::new( + BrokerErrorCode::ActionFailed, + "runtime not installed", + ))); + results.push(BrokerResult::indeterminate(BrokerError::new( + BrokerErrorCode::OutcomeUnknown, + "host restarted mid-execution", + ))); + + for result in results { + let response = BrokerResponse::new("req-1", result).replayed(); + let valid = serde_json::to_value(&response).expect("response serializes"); + assert_eq!( + serde_json::from_value::(valid.clone()).expect("fixture parses"), + response, + ); + + let mut paths = Vec::new(); + member_paths(&valid, "", &mut paths); + for path in paths { + let mut json = valid.clone(); + *json.pointer_mut(&path).expect("path addresses a member") = serde_json::Value::Null; + assert!( + serde_json::from_value::(json.clone()).is_err(), + "response with null at \"{path}\" must not deserialize: {json}" + ); + } + } + // The two `bool` members carry no explicit guard, because `null` already + // fails as a type error rather than defaulting to `false`. Pin that, so the + // docs saying so cannot drift and so a later change to `Option` — which + // *would* need the guard — fails here. + let mut json = serde_json::json!({ + "type": BROKER_REQUEST_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "actionVersion": 1, + "action": "channel.read", + "args": { "channelId": CHANNEL, "mentionsOnly": serde_json::Value::Null }, + }); + assert!( + serde_json::from_value::(json.clone()).is_err(), + "a null mentionsOnly must not deserialize: {json}" + ); + json = serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "status": "failed", + "error": { "code": "action_failed", "message": "no" }, + "replayed": serde_json::Value::Null, + }); + assert!( + serde_json::from_value::(json.clone()).is_err(), + "a null replayed must not deserialize: {json}" + ); +} + +/// The exact repro that reached `Ok`: a member the declared status does not admit, +/// supplied as `null` rather than as a value. The fixtures above cannot cover this +/// — a serialized response never contains the member its status forbids — so each +/// status-incompatible member is injected here by name. +/// +/// This is the case that makes the null hole a contract bug rather than a +/// tidiness one: these envelopes contradict themselves, and before the fix +/// `validate()` returned `Ok(())` on all of them. +#[test] +fn a_status_incompatible_member_is_rejected_as_null_not_only_as_a_value() { + let succeeded = || { + serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "status": "succeeded", + "action": "agents.delete", + "outcome": { "agentPubkey": PUBKEY, "displayName": "Gone" }, + }) + }; + let failed = || { + serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "status": "failed", + "error": { "code": "action_failed", "message": "no" }, + }) + }; + + let mut cases: Vec<(String, serde_json::Value)> = Vec::new(); + + // `error` is the member a success does not admit. + let mut json = succeeded(); + json["error"] = serde_json::Value::Null; + cases.push(("null error beside a success".into(), json)); + + // `action` and `outcome` are the members the two failure statuses do not + // admit — individually and together, since the original report showed both. + for status in ["failed", "indeterminate"] { + let base = || { + let mut json = failed(); + json["status"] = serde_json::json!(status); + if status == "indeterminate" { + json["error"] = serde_json::json!({ "code": "outcome_unknown", "message": "?" }); + } + json + }; + for member in ["action", "outcome"] { + let mut json = base(); + json[member] = serde_json::Value::Null; + cases.push((format!("null {member} beside a {status}"), json)); + } + let mut json = base(); + json["action"] = serde_json::Value::Null; + json["outcome"] = serde_json::Value::Null; + cases.push((format!("null action and outcome beside a {status}"), json)); + } + + for (what, json) in cases { + let parsed = serde_json::from_value::(json.clone()); + // Assert on the parse, not on `validate()`: a response that parses and + // then fails validation would still have to be *reported* by a caller + // that remembered to validate. Rejecting at the boundary means a + // malformed envelope never becomes a value at all. + assert!( + parsed.is_err(), + "{what} must not deserialize, but parsed as {:?} which validates {:?}: {json}", + parsed.as_ref().ok(), + parsed.as_ref().map(BrokerResponse::validate).ok(), + ); + } +} + +// ── 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 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(rejected).is_err(), + "\"{rejected}\" must not parse as an action" + ); + } + + let json = serde_json::json!({ + "type": BROKER_REQUEST_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "actionVersion": 1, + "action": "agents.exfiltrate", + "args": {}, + }); + assert!(serde_json::from_value::(json).is_err()); +} + +#[test] +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()).unwrap(); + request.protocol_version = bad; + let error = request.validated().unwrap_err().to_string(); + assert!(error.contains("protocolVersion"), "unexpected: {error}"); + + let mut response = BrokerResponse::new("req-1", failed()); + response.protocol_version = bad; + assert!(response.validate().is_err()); + } + + let mut wrong_action_version = BrokerRequest::new("req-1", args()).unwrap(); + wrong_action_version.action_version = 7; + let error = wrong_action_version.validated().unwrap_err().to_string(); + assert!(error.contains("actionVersion"), "unexpected: {error}"); + + let mut wrong_request_type = BrokerRequest::new("req-1", args()).unwrap(); + wrong_request_type.r#type = BROKER_RESULT_TYPE.into(); + assert!(wrong_request_type.validated().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] +fn request_id_must_be_present_bounded_and_printable() { + let args = || { + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Pubkey(pubkey()), + }) + }; + 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()); +} + +/// Duplicate object keys are rejected everywhere the envelope reads, including +/// inside the `outcome` object. +/// +/// serde's derived readers reject a repeated field, so most of this contract got +/// duplicate rejection for free. `outcome` did not: the strict intermediary held +/// it as a `serde_json::Value` before re-deserializing it under its action tag, +/// and buffering through `Value` silently collapses duplicates last-wins. That +/// made `outcome` the one place where a reader could see a value the envelope's +/// own strictness never vetted — so it now re-parses the original bytes via +/// `RawValue`. +/// +/// Each case asserts the de-duplicated form parses first, so a rejection cannot +/// be a rejection of the surrounding fixture. +#[test] +fn a_duplicate_object_key_is_rejected_at_every_depth() { + let outcome = format!(r#"{{"agentPubkey":"{PUBKEY}","displayName":"n"}}"#); + let response = |body: &str| { + format!( + r#"{{"type":"{BROKER_RESULT_TYPE}","protocolVersion":1,"requestId":"r","status":"succeeded","action":"agents.delete","outcome":{body}}}"# + ) + }; + + serde_json::from_str::(&response(&outcome)) + .expect("the de-duplicated response parses"); + let cases = [ + ( + "inside the outcome object", + response(&format!( + r#"{{"agentPubkey":"{PUBKEY}","displayName":"first","displayName":"second"}}"# + )), + ), + ( + "a top-level envelope member", + format!( + r#"{{"type":"{BROKER_RESULT_TYPE}","protocolVersion":1,"requestId":"r","requestId":"evil","status":"succeeded","action":"agents.delete","outcome":{outcome}}}"# + ), + ), + ( + "a flattened member", + format!( + r#"{{"type":"{BROKER_RESULT_TYPE}","protocolVersion":1,"requestId":"r","status":"succeeded","action":"agents.delete","action":"agents.update","outcome":{outcome}}}"# + ), + ), + ( + "inside a typed error payload", + format!( + r#"{{"type":"{BROKER_RESULT_TYPE}","protocolVersion":1,"requestId":"r","status":"failed","error":{{"code":"unauthorized","message":"a","message":"b"}}}}"# + ), + ), + ]; + for (where_, json) in cases { + assert!( + serde_json::from_str::(&json).is_err(), + "a duplicate key {where_} must not deserialize" + ); + } + + // The request envelope too, where `args` is typed rather than buffered. + let request = |args: &str| { + format!( + r#"{{"type":"{BROKER_REQUEST_TYPE}","protocolVersion":1,"requestId":"r","actionVersion":1,"action":"agents.delete","args":{args}}}"# + ) + }; + serde_json::from_str::(&request(r#"{"target":{"name":"good"}}"#)) + .expect("the de-duplicated request parses"); + assert!( + serde_json::from_str::(&request( + r#"{"target":{"name":"good"},"target":{"name":"evil"}}"# + )) + .is_err(), + "a duplicate key inside args must not deserialize" + ); +} + +// ── 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 — plus the two +/// envelopes, whose own key sets are now equally enforceable. +/// +/// 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. +/// +/// The envelopes are here because a key set nobody pins is a key set a field can +/// be added to. The response envelope in particular admits a *different* exact +/// set per status, which is what its strict deserializer enforces. +#[test] +fn every_payload_has_an_exact_and_secret_free_wire_schema() { + let signer = Keys::generate(); + let expected: Vec<(&str, Vec<&str>)> = vec![ + // Envelopes. Every optional member present, so the pin covers the + // widest shape each may take. + ( + "request/envelope", + vec![ + "action", + "actionVersion", + "args", + "protocolVersion", + "requestId", + "type", + ], + ), + ( + "response/envelope/succeeded", + vec![ + "action", + "outcome", + "protocolVersion", + "replayed", + "requestId", + "status", + "type", + ], + ), + ( + "response/envelope/failed", + vec![ + "error", + "protocolVersion", + "replayed", + "requestId", + "status", + "type", + ], + ), + ( + "response/envelope/indeterminate", + vec![ + "error", + "protocolVersion", + "replayed", + "requestId", + "status", + "type", + ], + ), + ("error", vec!["code", "message"]), + // 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(); + // Envelopes first, in the same order as the table above. `replayed` is set + // so the widest shape is what gets pinned. + let request = BrokerRequest::new( + "req-1", + ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL)), + ) + .expect("envelope fixture builds"); + actual.push(( + "request/envelope".to_string(), + keys_of(&serde_json::to_value(&request).expect("request serializes")), + )); + for (name, result) in [ + ( + "succeeded", + BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: pubkey(), + display_name: "Gone".into(), + })), + ), + ( + "failed", + BrokerResult::failed(BrokerError::new(BrokerErrorCode::ActionFailed, "no")), + ), + ( + "indeterminate", + BrokerResult::indeterminate(BrokerError::new(BrokerErrorCode::OutcomeUnknown, "?")), + ), + ] { + let response = BrokerResponse::new("req-1", result).replayed(); + actual.push(( + format!("response/envelope/{name}"), + keys_of(&serde_json::to_value(&response).expect("response serializes")), + )); + } + actual.push(( + "error".to_string(), + keys_of( + &serde_json::to_value(BrokerError::new(BrokerErrorCode::Internal, "?")) + .expect("error serializes"), + ), + )); + 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 — the same reason `agents.create` has no owner. +#[test] +fn no_payload_can_name_its_own_authority() { + let request = serde_json::to_value( + BrokerRequest::new( + "req-1", + ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL)), + ) + .unwrap(), + ) + .unwrap(); + assert_eq!( + keys_of(&request), + vec![ + "action", + "actionVersion", + "args", + "protocolVersion", + "requestId", + "type" + ] + ); + + // Authority-naming fields are rejected, not ignored, wherever they appear. + for rejected in [ + serde_json::json!({ + "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": "A", "systemPrompt": "B", + "secretKey": "nsec1deadbeef", + }), + ] { + assert!( + serde_json::from_value::(rejected.clone()).is_err(), + "must reject: {rejected}" + ); + } + + // 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, + }); + outcome[extra] = serde_json::json!("nsec1deadbeef"); + let json = serde_json::json!({ "action": "agents.create", "outcome": outcome }); + assert!( + serde_json::from_value::(json).is_err(), + "an outcome carrying \"{extra}\" must not deserialize" + ); + } +} + +/// The nested action enums are strict about their own key set, not just about +/// the payload inside it. +/// +/// `ActionArgs`/`ActionOutcome` are adjacently tagged, so their wire form is the +/// two-key object `{action, args}` / `{action, outcome}`. Without +/// `deny_unknown_fields` on the enum itself, a *sibling* of those two keys is +/// silently ignored — and these types are public and wire-facing, so a host +/// author can deserialize one directly rather than through the envelope. The +/// envelope's own strictness does not cover that door. +#[test] +fn a_nested_action_object_rejects_siblings_of_its_two_keys() { + // The valid two-key forms must pass untouched, so a rejection below cannot + // be a rejection of the fixture itself. + let args = serde_json::json!({ + "action": "agents.delete", "args": { "target": { "name": "helper" } }, + }); + let outcome = serde_json::json!({ + "action": "agents.delete", + "outcome": { "agentPubkey": PUBKEY, "displayName": "Gone" }, + }); + serde_json::from_value::(args.clone()).expect("the exact args shape deserializes"); + serde_json::from_value::(outcome.clone()) + .expect("the exact outcome shape deserializes"); + + for extra in ["secretKey", "nsec", "outcome", "unexpected"] { + let mut probe = args.clone(); + probe[extra] = serde_json::json!("x"); + assert!( + serde_json::from_value::(probe).is_err(), + "ActionArgs must reject the sibling key \"{extra}\"" + ); + } + for extra in ["secretKey", "nsec", "args", "unexpected"] { + let mut probe = outcome.clone(); + probe[extra] = serde_json::json!("x"); + assert!( + serde_json::from_value::(probe).is_err(), + "ActionOutcome must reject the sibling key \"{extra}\"" + ); + } +} + +#[test] +fn pubkey_hex_rejects_anything_but_a_public_key() { + assert!(PubkeyHex::parse("nothex").is_err()); + assert!(PubkeyHex::parse(&PUBKEY[..40]).is_err()); + assert!(PubkeyHex::parse(format!("{PUBKEY}00")).is_err()); + assert!(PubkeyHex::parse("nsec1deadbeef").is_err()); + // Normalizes case, so two spellings of one key cannot look like two keys. + assert_eq!( + PubkeyHex::parse(PUBKEY.to_ascii_uppercase()).unwrap(), + pubkey() + ); + // And it enforces that through serde, not only through the constructor. + assert!(serde_json::from_value::(serde_json::json!("nothex")).is_err()); +} + +/// 64 hex characters is a *shape*; a public key is a point on secp256k1. Most +/// 32-byte values are not one, so accepting shape alone let this type's name +/// promise something it never checked, and deferred the first real rejection to +/// whichever consumer eventually converted the string to a key — by which point +/// the request had already been accepted. +/// +/// The fixtures are ordered the way the type is used: the real key must pass +/// untouched first, so a rejection below is about the curve check and not about a +/// probe that would have failed for any input. +#[test] +fn a_pubkey_must_be_a_point_on_the_curve_not_merely_hex() { + /// The check this type now delegates to, spelled out independently: a value + /// is a key only if it converts to an x-only key, which is what `xonly` + /// does. `from_hex` alone is a hex decode and answers nothing — asking only + /// it is how the gap survived a `nostr`-backed check in the first place. + fn is_a_point(hex: &str) -> bool { + nostr::PublicKey::from_hex(hex) + .and_then(|key| key.xonly().map(|_| ())) + .is_ok() + } + + // A real key passes, in both spellings, and is unchanged by the new check. + assert!(is_a_point(PUBKEY), "fixture must be a real point"); + assert_eq!( + PubkeyHex::parse(PUBKEY).expect("a real key parses"), + pubkey() + ); + assert_eq!( + PubkeyHex::parse(PUBKEY.to_ascii_uppercase()).expect("case is still normalized"), + pubkey() + ); + + // Well-formed hex that is not on the curve. Each is asserted to be a + // non-point first, so the rejection cannot be for an unrelated reason. + // + // The last fixture is the important one: `x = 5` is a perfectly in-range + // field element, so it is not rejected for overflowing the field the way the + // first three are — there is simply no y with y² = x³ + 7. A check that + // only bounds the value against the field prime would accept it, so this is + // what pins the test to a real curve check rather than a range check. + for junk in [ + "f".repeat(64), + "0".repeat(64), + // The field prime p itself: out of range by exactly one. + "fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f".into(), + format!("{:0>64}", 5), + ] { + assert!( + !is_a_point(&junk), + "fixture \"{junk}\" must not be a valid point" + ); + let error = PubkeyHex::parse(&junk) + .expect_err("64 hex characters that are not a point must be rejected") + .to_string(); + assert!( + error.contains("x-only"), + "rejection must name the curve, not the shape: {error}" + ); + // The serde door takes the same check: `PubkeyHex` deserializes through + // `parse`, so a host cannot ship a non-point where a constructor would + // have refused one. + assert!( + serde_json::from_value::(serde_json::json!(junk)).is_err(), + "the wire door must reject the non-point \"{junk}\" too" + ); + // And through a payload, since that is the shape a host actually sends. + // The honest form parses, so this rejects for the key and not the shape. + let target = |value: &str| { + serde_json::json!({ + "action": "agents.delete", + "args": { "target": { "pubkey": value } }, + }) + }; + serde_json::from_value::(target(PUBKEY)) + .expect("a real key in a target payload must parse"); + assert!( + serde_json::from_value::(target(&junk)).is_err(), + "an agents.delete target must reject the non-point \"{junk}\"" + ); + } +} + +// ── Argument validation ───────────────────────────────────────────────────── + +/// Boundaries of every shared validator, in one table. +#[test] +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() + }; + 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![]).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]).unwrap_err(), + SdkError::TooManyMentions + )); + assert!(react(" ".into()).is_err()); + assert!(react(":shipit:".into()).is_ok()); + assert!(matches!( + react("a".repeat(actions::MAX_EMOJI_CHARS + 1)).unwrap_err(), + SdkError::EmojiTooLong + )); + + // 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() + }; + 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()), + } + .validated() + .is_err()); +} + +/// Validation must be **inseparable from normalization**: there must be no way +/// to learn that a request is valid while still holding the un-normalized value. +/// +/// The bug this pins: `validate(&self)` called the arguments' `validated()`, +/// which *computes* a normalized copy, then dropped the copy and returned +/// `Ok(())`. A hand-built request targeting `" helper "` therefore passed +/// validation and still carried the padding, so a host that trusted the verdict +/// and executed the struct looked up a name the validator never approved. +/// `prepare()` was safe, but a host cannot force its callers through the client's +/// outgoing path. +/// +/// The fix is typed, so this test asserts the *shape* of the API and not just one +/// call's behaviour: the only route to a verdict is `validated()`, which consumes +/// the request and hands back a `ValidatedRequest` whose arguments are already +/// normalized. The un-normalized value is gone rather than sitting beside its +/// approved copy. That the old method no longer exists is enforced at compile +/// time by every other caller in this file having had to change. +#[test] +fn a_request_cannot_be_validated_without_being_normalized() { + let padded = || { + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name(" helper ".into()), + }) + }; + let trimmed = ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name("helper".into()), + }); + + // A request built by hand, bypassing `new` — the shape the reviewer used. + let hand_built = BrokerRequest { + r#type: BROKER_REQUEST_TYPE.into(), + protocol_version: BROKER_PROTOCOL_VERSION, + request_id: "req-trap".into(), + action_version: 1, + action: padded(), + }; + let validated = hand_built + .validated() + .expect("a padded name is valid, just not canonical"); + + // The verdict and the normalized value are the same object, so an executor + // holding the verdict cannot be holding the padding. + assert_eq!( + validated.args(), + &trimmed, + "a validated request must carry the normalized arguments" + ); + assert_eq!(validated.action(), Action::AgentsDelete); + assert_eq!(validated.request_id(), "req-trap"); + + // Freezing from the verdict carries the normalized value onto the wire. + let body = String::from_utf8( + validated + .clone() + .prepare() + .expect("prepares") + .body() + .to_vec(), + ) + .expect("body is utf8"); + assert!( + body.contains(r#""name":"helper""#) && !body.contains(" helper "), + "frozen body still carries the unnormalized name: {body}" + ); + + // Moving the envelope onward yields the normalized request, not the input. + assert_eq!(validated.into_request().action, trimmed); + + // The envelope is still checked, so `validated` is not merely a normalizer: + // an invalid envelope produces no verdict at all. + let bad_version = BrokerRequest { + r#type: BROKER_REQUEST_TYPE.into(), + protocol_version: 99, + request_id: "req-trap".into(), + action_version: 1, + action: padded(), + }; + assert!(bad_version.validated().is_err()); + + // And arguments that cannot be normalized are rejected rather than + // normalized to something the caller did not ask for. + let empty_name = BrokerRequest { + r#type: BROKER_REQUEST_TYPE.into(), + protocol_version: BROKER_PROTOCOL_VERSION, + request_id: "req-trap".into(), + action_version: 1, + action: ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name(" ".into()), + }), + }; + assert!(empty_name.validated().is_err()); +} + +/// Validation normalizes, so the frozen body must carry the normalized value — +/// not the caller's. Otherwise a padded selector passes validation and the host +/// executes something the validator never approved: it looks up `" helper "`, +/// or publishes a padded reaction. +/// +/// Both construction paths are checked, because `BrokerRequest`'s fields are +/// public and it is `Deserialize`, so `prepare` is reachable without ever going +/// through `new`. +#[test] +fn the_frozen_body_carries_exactly_what_validation_approved() { + // Path 1: through `new`, which stores the normalized action. + let request = BrokerRequest::new( + "req-normalize", + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name(" helper ".into()), + }), + ) + .expect("a padded name is valid, just not canonical"); + assert_eq!( + request.action, + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name("helper".into()), + }), + "`new` must store the normalized copy" + ); + let body = String::from_utf8(request.prepare().expect("prepares").body().to_vec()) + .expect("body is utf8"); + assert!( + body.contains(r#""name":"helper""#) && !body.contains(" helper "), + "frozen body still carries the unnormalized name: {body}" + ); + + // Path 2: a struct literal that bypasses `new` entirely. + let bypassed = BrokerRequest { + r#type: BROKER_REQUEST_TYPE.to_string(), + protocol_version: BROKER_PROTOCOL_VERSION, + request_id: "req-bypass".into(), + action_version: 1, + action: ActionArgs::ReactionAdd(ReactionAddArgs { + channel_id: CHANNEL.into(), + target_event_id: EVENT.into(), + reaction: " \u{1f41d} ".into(), + }), + }; + let body = String::from_utf8(bypassed.prepare().expect("prepares").body().to_vec()) + .expect("body is utf8"); + assert!( + body.contains("\"reaction\":\"\u{1f41d}\""), + "frozen body did not normalize a padded reaction: {body}" + ); + + // Normalization is idempotent, so a second freeze is byte-identical: the + // retry contract still holds through the new path. + let once = BrokerRequest::new( + "req-idem", + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name(" helper ".into()), + }), + ) + .unwrap() + .prepare() + .unwrap(); + let twice = BrokerRequest::new( + "req-idem", + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name("helper".into()), + }), + ) + .unwrap() + .prepare() + .unwrap(); + assert_eq!( + once.body(), + twice.body(), + "a padded and a pre-trimmed request must freeze to the same bytes" + ); +} + +/// Correlation must reject an outcome that echoes a different identity than the +/// request supplied — `requestId` plus action is not enough, because a host +/// routing bug can return a well-formed success for the wrong subject. +/// +/// Table-driven over every request/outcome identity pair, so the enumeration in +/// `correlate_identities`' doc table is pinned by a test rather than asserted in +/// prose. Each case builds the *matching* response first and requires it to pass, +/// so a case cannot "reject" for an unrelated reason. +#[test] +fn correlation_rejects_an_outcome_naming_a_different_subject() { + let requested = pubkey(); + let other = + PubkeyHex::parse("b02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971") + .expect("valid hex"); + let other_channel = "c2c38ca8-9ec3-411e-bab5-f9deab34d52e"; + + // (action, matching outcome, mismatched outcome or None when nothing is + // comparable). A `None` documents an inherent gap, not an oversight. + let cases: Vec<(&str, ActionArgs, ActionOutcome, Option)> = vec![ + ( + "agents.create echoes channelId", + ActionArgs::AgentsCreate(AgentsCreateArgs { + channel_id: CHANNEL.into(), + display_name: "Helper".into(), + system_prompt: "be useful".into(), + runtime: None, + provider: None, + model: None, + respond_to: None, + }), + ActionOutcome::AgentsCreate(AgentsCreateOutcome { + agent_pubkey: requested.clone(), + display_name: "Helper".into(), + channel_id: CHANNEL.into(), + }), + Some(ActionOutcome::AgentsCreate(AgentsCreateOutcome { + agent_pubkey: requested.clone(), + display_name: "Helper".into(), + channel_id: other_channel.into(), + })), + ), + ( + "agents.update targeted by pubkey echoes agentPubkey", + ActionArgs::AgentsUpdate(AgentsUpdateArgs { + target: AgentTarget::Pubkey(requested.clone()), + display_name: Some("Renamed".into()), + system_prompt: None, + runtime: None, + provider: None, + model: None, + respond_to: None, + }), + ActionOutcome::AgentsUpdate(AgentsUpdateOutcome { + agent_pubkey: requested.clone(), + display_name: "Renamed".into(), + updated_fields: vec!["displayName".into()], + }), + Some(ActionOutcome::AgentsUpdate(AgentsUpdateOutcome { + agent_pubkey: other.clone(), + display_name: "Renamed".into(), + updated_fields: vec!["displayName".into()], + })), + ), + ( + "agents.delete targeted by pubkey echoes agentPubkey", + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Pubkey(requested.clone()), + }), + ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: requested.clone(), + display_name: "Gone".into(), + }), + Some(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: other.clone(), + display_name: "Gone".into(), + })), + ), + ( + // Inherent gap: the host resolves the name, and the rename may be + // exactly what this call performed, so no pubkey is comparable. + "agents.delete targeted by name compares nothing", + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Name("helper".into()), + }), + ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: other.clone(), + display_name: "helper".into(), + }), + None, + ), + ( + // Host-minted identifiers only; nothing the request supplied is echoed. + "message.post echoes no requested identity", + ActionArgs::MessagePost(MessagePostArgs { + channel_id: CHANNEL.into(), + content: "hi".into(), + mentions: vec![], + }), + ActionOutcome::MessagePost(EventPublished { + event_id: EVENT.into(), + kind: 9, + created_at: 1, + }), + None, + ), + ]; + + for (label, args, matching, mismatched) in cases { + let request = BrokerRequest::new("req-correlate", args) + .expect("fixture args validate") + .prepare() + .expect("fixture prepares"); + BrokerResponse::new("req-correlate", BrokerResult::succeeded(matching)) + .validate_for(&request) + .unwrap_or_else(|e| panic!("{label}: the matching outcome must pass, got {e}")); + if let Some(mismatched) = mismatched { + let err = BrokerResponse::new("req-correlate", BrokerResult::succeeded(mismatched)) + .validate_for(&request) + .expect_err(&format!("{label}: a mismatched identity must be rejected")); + assert!( + matches!(err, SdkError::InvalidInput(_)), + "{label}: expected InvalidInput, got {err:?}" + ); + } + } +} + +// ── Identities have one spelling ──────────────────────────────────────────── + +/// Every legal spelling of a channel UUID names one channel, so a request and a +/// response that spell it differently must still correlate. +/// +/// The bug: `Uuid::parse_str` accepts uppercase, unhyphenated, braced, and +/// `urn:uuid:` forms, `channel()` returned the caller's spelling untouched, and +/// correlation compared bytes — so an uppercase request against a host's +/// canonical lowercase echo of the *same* channel failed `validate_for`. That is +/// worse than the mismatch the check exists to catch: it makes a correct host +/// unusable. +/// +/// Two independent guards close it, and each is asserted separately below so +/// neither can be the only thing holding: canonicalize where a value enters, and +/// compare parsed identities rather than bytes. +#[test] +fn one_identity_spelled_two_ways_still_correlates() { + let spellings = [ + CHANNEL.to_ascii_uppercase(), + CHANNEL.replace('-', ""), + format!("{{{CHANNEL}}}"), + format!("urn:uuid:{CHANNEL}"), + CHANNEL.to_string(), + ]; + + let create = |channel_id: &str| { + ActionArgs::AgentsCreate(AgentsCreateArgs { + channel_id: channel_id.into(), + display_name: "Helper".into(), + system_prompt: "be useful".into(), + runtime: None, + provider: None, + model: None, + respond_to: None, + }) + }; + let echo = |channel_id: &str| { + BrokerResult::succeeded(ActionOutcome::AgentsCreate(AgentsCreateOutcome { + agent_pubkey: pubkey(), + display_name: "Helper".into(), + channel_id: channel_id.into(), + })) + }; + + for spelling in &spellings { + // Guard 1: the frozen body carries the canonical spelling, not the + // caller's, so what the host receives is what correlation will compare. + let request = BrokerRequest::new("req-spelling", create(spelling)) + .expect("every legal UUID spelling validates"); + let body = String::from_utf8(request.prepare().expect("prepares").body().to_vec()) + .expect("body is utf8"); + assert!( + body.contains(&format!("\"channelId\":\"{CHANNEL}\"")), + "frozen body did not canonicalize \"{spelling}\": {body}" + ); + + // And through the wire door too, which no `validated()` covers: a parsed + // request reaches a caller canonical. + let parsed: BrokerRequest = serde_json::from_value(serde_json::json!({ + "type": BROKER_REQUEST_TYPE, + "protocolVersion": 1, + "requestId": "req-spelling", + "actionVersion": 1, + "action": "channel.read", + "args": { "channelId": spelling }, + })) + .unwrap_or_else(|e| panic!("\"{spelling}\" must parse: {e}")); + assert_eq!( + parsed.action, + ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL)), + "the wire door did not canonicalize \"{spelling}\"" + ); + + // Guard 2: correlation compares parsed identities, so every spelling on + // either side correlates even if guard 1 were absent. + let prepared = BrokerRequest::new("req-spelling", create(spelling)) + .expect("validates") + .prepare() + .expect("prepares"); + for returned in &spellings { + BrokerResponse::new("req-spelling", echo(returned)) + .validate_for(&prepared) + .unwrap_or_else(|e| { + panic!("request \"{spelling}\" vs echo \"{returned}\" must correlate: {e}") + }); + } + } + + // A genuinely different channel is still rejected, so the fix widened what + // counts as equal without weakening the check. + let prepared = BrokerRequest::new("req-spelling", create(CHANNEL)) + .expect("validates") + .prepare() + .expect("prepares"); + let err = BrokerResponse::new("req-spelling", echo("c2c38ca8-9ec3-411e-bab5-f9deab34d52e")) + .validate_for(&prepared) + .expect_err("a different channel must still be rejected"); + assert!(matches!(err, SdkError::InvalidInput(_)), "{err:?}"); +} + +/// The same treatment for the contract's other multi-spelling identities: hex. +/// +/// A pubkey was already canonicalized by `PubkeyHex::parse`, which is also its +/// serde path — this pins that it is, so the `agentPubkey` rows of the +/// correlation table cannot regress into a byte comparison of two cases. Event +/// ids and `d` tags are plain `String`s and were *not* normalized on the wire, +/// only in `validated()`, so those are the ones this changes. +#[test] +fn hex_identities_are_canonical_through_every_door() { + // Pubkey: mixed-case target vs lowercase echo correlates, both directions. + let upper = PubkeyHex::parse(PUBKEY.to_ascii_uppercase()).expect("valid hex"); + let request = BrokerRequest::new( + "req-hex", + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Pubkey(upper), + }), + ) + .expect("validates") + .prepare() + .expect("prepares"); + BrokerResponse::new( + "req-hex", + BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: pubkey(), + display_name: "Gone".into(), + })), + ) + .validate_for(&request) + .expect("two cases of one pubkey are one identity"); + + // Event ids and d tags: the wire door lowercases, so a parsed value equals a + // constructed one and neither carries the sender's case. + let parsed: ActionArgs = serde_json::from_value(serde_json::json!({ + "action": "reaction.add", + "args": { + "channelId": CHANNEL, + "targetEventId": EVENT.to_ascii_uppercase(), + "reaction": "\u{1f41d}", + }, + })) + .expect("an uppercase event id parses"); + assert_eq!( + parsed, + ActionArgs::ReactionAdd(ReactionAddArgs { + channel_id: CHANNEL.into(), + target_event_id: EVENT.into(), + reaction: "\u{1f41d}".into(), + }), + "the wire door did not lowercase targetEventId" + ); + + let parsed: ActionOutcome = serde_json::from_value(serde_json::json!({ + "action": "storage.address", + "outcome": { + "authorPubkey": PUBKEY.to_ascii_uppercase(), + "kind": 30078, + "dTag": EVENT.to_ascii_uppercase(), + }, + })) + .expect("an uppercase d tag parses"); + assert_eq!( + parsed, + ActionOutcome::StorageAddress(StorageAddress { + author_pubkey: pubkey(), + kind: 30078, + d_tag: EVENT.into(), + }), + "the wire door did not lowercase dTag or authorPubkey" + ); + + // The optional identity member takes the same door, and still rejects null. + let read: ActionArgs = serde_json::from_value(serde_json::json!({ + "action": "channel.read", + "args": { "channelId": CHANNEL, "rootEventId": EVENT.to_ascii_uppercase() }, + })) + .expect("an uppercase root event id parses"); + assert_eq!( + read, + ActionArgs::ChannelRead(ChannelReadArgs { + channel_id: CHANNEL.into(), + root_event_id: Some(EVENT.into()), + ..ChannelReadArgs::default() + }), + ); + assert!( + serde_json::from_value::(serde_json::json!({ + "action": "channel.read", + "args": { "channelId": CHANNEL, "rootEventId": serde_json::Value::Null }, + })) + .is_err(), + "canonicalizing must not have replaced the null guard" + ); + + // A malformed identity is still a parse failure, so the new doors reject + // rather than merely normalize. + for bad in ["nothex", "", &EVENT[..40], &format!("{EVENT}00")] { + assert!( + serde_json::from_value::(serde_json::json!({ + "action": "channel.read", + "args": { "channelId": CHANNEL, "rootEventId": bad }, + })) + .is_err(), + "rootEventId \"{bad}\" must not deserialize" + ); + } + assert!( + serde_json::from_value::(serde_json::json!({ + "action": "channel.read", + "args": { "channelId": "not-a-uuid" }, + })) + .is_err(), + "a non-UUID channelId must not deserialize" + ); +} + +/// `BrokerResult` must have **no wire door of its own**, so the strict envelope is +/// the only way to read a result. +/// +/// The bug: the exported result type derived its own reader, which accepted and +/// dropped arbitrary siblings — `status: failed` beside an `error` and a +/// `secretKey`, or a succeeded result beside an `error`. A consumer parsing the +/// result type directly therefore got an `Ok` value whose complete wire shape had +/// never been vetted, while the identical bytes failed through the envelope. +/// +/// Removing the door is checked at compile time, because a runtime test cannot +/// call a `Deserialize` impl that does not exist. `absence_of_a_reader` resolves to +/// the inherent function only when the bound holds, so this is a genuine negative +/// assertion rather than a comment. +#[test] +fn the_result_type_has_no_deserializer_of_its_own() { + struct Probe(std::marker::PhantomData); + + trait NoReader { + fn absence_of_a_reader() -> bool { + true + } + } + impl NoReader for Probe {} + + impl Probe { + fn absence_of_a_reader() -> bool { + false + } + } + + // The probe must be able to see a reader that *is* there, or its `true` + // means nothing. + assert!( + !Probe::::absence_of_a_reader(), + "probe is broken: it reports no reader for a type that has one" + ); + assert!( + !Probe::::absence_of_a_reader(), + "probe is broken: it reports no reader for a type that has one" + ); + assert!( + Probe::::absence_of_a_reader(), + "BrokerResult must not be Deserialize: it is a second, lax wire door" + ); + + // And the exact byte sequences the old direct reader accepted are rejected + // through the one door that remains. Each is the envelope form of what + // bugs-00 reported, since a bare result object is no longer parseable at all. + let envelope = |extra: serde_json::Value| { + let mut json = serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + }); + for (key, value) in extra.as_object().expect("object").clone() { + json[key] = value; + } + json + }; + let reported = [ + ( + "failed with an error and a secretKey", + serde_json::json!({ + "status": "failed", + "error": { "code": "action_failed", "message": "no" }, + "secretKey": "nsec1deadbeef", + }), + ), + ( + "succeeded beside an error", + serde_json::json!({ + "status": "succeeded", + "action": "agents.delete", + "outcome": { "agentPubkey": PUBKEY, "displayName": "Gone" }, + "error": { "code": "action_failed", "message": "no" }, + }), + ), + ]; + for (what, body) in reported { + let json = envelope(body); + assert!( + serde_json::from_value::(json.clone()).is_err(), + "{what} must not deserialize through the envelope either: {json}" + ); + } +} + +/// Derived coverage for the canonicalization rule, so a *newly added* identity +/// member is covered without anyone remembering to extend a list. +/// +/// The two tests above name the members that exist today. This one walks the real +/// fixtures — requests *and* responses, since both directions carry identities +/// through separate code — finds every member whose name marks it as an identity, +/// re-spells its value, and requires the payload to parse back to the canonical +/// value. A field added later with the wrong (or no) `deserialize_with` fails here. +/// +/// Matching on the member *name* is the point: the naming convention is what a +/// reviewer sees, so if a member is named like an identity it is held to the +/// identity rule. A member holding an identity under some other name would escape +/// this, which is why the audit above is by type as well. +/// +/// The suffix match is case-insensitive on purpose. An earlier revision matched +/// `"EventId"` exactly, which silently skipped the outcome member spelled +/// `eventId` and left every response-side door unpinned — a mutation removing +/// that door survived. Matching how a *reader* groups these names, rather than +/// how one of them happens to be capitalized, is what closes that gap. +#[test] +fn every_identity_shaped_member_is_canonicalized_on_the_wire() { + /// A member-name suffix and how a sender might legally re-spell its value. + type Respelling = (&'static str, fn(&str) -> String); + + // Every identity in this contract is hex or a UUID, so case is the + // re-spelling they all admit; `channelId` additionally admits the forms + // covered by `one_identity_spelled_two_ways_still_correlates`. + let respellings: [Respelling; 4] = [ + ("channelid", |v| v.to_ascii_uppercase()), + ("eventid", |v| v.to_ascii_uppercase()), + ("pubkey", |v| v.to_ascii_uppercase()), + ("dtag", |v| v.to_ascii_uppercase()), + ]; + + /// Re-spell every identity-named member of `valid` in turn and require the + /// payload to parse back to `original`. Returns how many members it checked. + fn respell_each(valid: &serde_json::Value, original: &T, respellings: &[Respelling]) -> usize + where + T: serde::de::DeserializeOwned + PartialEq + std::fmt::Debug, + { + let mut checked = 0; + let mut paths = Vec::new(); + member_paths(valid, "", &mut paths); + for path in paths { + let Some(name) = path.rsplit('/').next() else { + continue; + }; + let lowered = name.to_ascii_lowercase(); + let Some((_, respell)) = respellings + .iter() + .find(|(suffix, _)| lowered.ends_with(suffix)) + else { + continue; + }; + let Some(current) = valid + .pointer(&path) + .expect("path addresses a member") + .as_str() + else { + continue; + }; + let respelled = respell(current); + if respelled == current { + continue; + } + + let mut json = valid.clone(); + *json.pointer_mut(&path).expect("path addresses a member") = + serde_json::Value::String(respelled.clone()); + let parsed: T = serde_json::from_value(json) + .unwrap_or_else(|e| panic!("\"{respelled}\" at {path} must parse: {e}")); + assert_eq!( + &parsed, original, + "member {path} did not canonicalize \"{respelled}\" back to \"{current}\"" + ); + checked += 1; + } + checked + } + + let mut request_members = 0; + for args in action_fixtures() { + let request = BrokerRequest::new("req-canon", args).expect("fixture request builds"); + let valid = serde_json::to_value(&request).expect("request serializes"); + request_members += respell_each(&valid, &request, &respellings); + } + + // The response side carries identities too — `agents.create` echoes a + // `channelId`, `storage.address` a `dTag`, the publishing outcomes an + // `eventId` and an `authorPubkey` — and those doors are separate code from + // the request side's. + let keys = Keys::generate(); + let mut response_members = 0; + for outcome in outcome_fixtures(&keys) { + let response = BrokerResponse::new("req-canon", BrokerResult::succeeded(outcome)); + let valid = serde_json::to_value(&response).expect("response serializes"); + response_members += respell_each(&valid, &response, &respellings); + } + + // Guard the guard: a rule that silently matched nothing would pass forever. + // The two directions are floored *separately* on purpose — one combined + // total would be satisfied by the request side alone, which is exactly the + // blind spot that let a response-side door go unpinned. + assert!( + request_members >= 8, + "expected identity members across the request fixtures, checked {request_members}" + ); + assert!( + response_members >= 6, + "expected identity members across the response fixtures, checked {response_members}" + ); +} + +// ── 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 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" + ] + ); +} + +/// The one type here the contract does not own. `nostr`'s `Event` deserializer +/// accepts and discards unknown members, so a genuinely signed event could carry +/// an extra `secretKey` and parse clean — the no-secret rule stopping at the +/// envelope boundary instead of reaching inside it. Deserializing through a +/// `deny_unknown_fields` intermediary closes that, and this drives the injection +/// on a real signed event so nothing is rejected for a bad signature instead. +#[test] +fn an_event_object_cannot_smuggle_a_member_past_the_seven_canonical_ones() { + let signer = Keys::generate(); + let message = signed_message(&signer); + let wire = serde_json::to_value(&message).expect("event serializes"); + + // The baseline: untouched, this same JSON parses and verifies. + let parsed: BrokerMessage = + serde_json::from_value(wire.clone()).expect("a signed event round-trips"); + parsed.verify().expect("and still verifies"); + + for extra in ["secretKey", "nsec", "seckey", "credential", "hostNote"] { + let mut smuggled = wire.clone(); + smuggled[extra] = serde_json::json!("nsec1deadbeef"); + assert!( + serde_json::from_value::(smuggled.clone()).is_err(), + "an event carrying \"{extra}\" must not deserialize: {smuggled}" + ); + + // And not through the outcome or the envelope either — the rejection has + // to hold at every depth a read result travels. + let outcome = serde_json::json!({ + "action": "channel.read", + "outcome": { "messages": [smuggled.clone()] }, + }); + assert!( + serde_json::from_value::(outcome).is_err(), + "an outcome holding an event with \"{extra}\" must not deserialize" + ); + let envelope = serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": "req-1", + "status": "succeeded", + "action": "channel.read", + "outcome": { "messages": [smuggled] }, + }); + assert!( + serde_json::from_value::(envelope).is_err(), + "a response holding an event with \"{extra}\" must not deserialize" + ); + } + + // Dropping a canonical member is a parse failure too, not a default. + for missing in [ + "id", + "pubkey", + "created_at", + "kind", + "tags", + "content", + "sig", + ] { + let mut json = wire.clone(); + json.as_object_mut().unwrap().remove(missing); + assert!( + serde_json::from_value::(json).is_err(), + "an event missing \"{missing}\" must not deserialize" + ); + } +} + +#[test] +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()); +} + +/// The protocol cap is not the caller's limit. `ActionOutcome::validate` never +/// sees the request, so on its own it would let a host answer a one-message read +/// with five hundred — within the cap, and still an overrun of what was asked. +/// The request's own number is therefore enforced where both halves are in +/// scope, and an absent `limit` is held to [`actions::DEFAULT_PAGE_LIMIT`] +/// rather than treated as consent to an unbounded page. +#[test] +fn a_read_page_is_bounded_by_the_limit_its_own_request_asked_for() { + let signer = Keys::generate(); + let page = |count: usize| { + BrokerResult::succeeded(ActionOutcome::ChannelRead(MessagePage { + messages: vec![signed_message(&signer); count], + next_cursor: None, + })) + }; + + // Explicit limits, and the absent case — which is the one a host could + // otherwise read as "as many as you like". + for limit in [Some(1_u32), Some(2), Some(actions::MAX_PAGE_LIMIT), None] { + let args = ChannelReadArgs { + channel_id: CHANNEL.into(), + limit, + ..ChannelReadArgs::default() + }; + let allowed = limit.unwrap_or(actions::DEFAULT_PAGE_LIMIT) as usize; + assert_eq!( + args.effective_limit() as usize, + allowed, + "effective_limit must not diverge from the documented default" + ); + let request = prepared(ActionArgs::ChannelRead(args)); + + BrokerResponse::new(request.request_id(), page(allowed)) + .validate_for(&request) + .unwrap_or_else(|e| panic!("a page exactly at a limit of {allowed} is allowed: {e}")); + BrokerResponse::new(request.request_id(), page(allowed - 1)) + .validate_for(&request) + .unwrap_or_else(|e| panic!("a short page is allowed: {e}")); + + // One over is rejected — including one over the default, which is the + // case an unlimited request would have smuggled through. At the + // protocol cap the outcome's own bound fires first, which is a rejection + // for a different (and also correct) reason, so only the message below + // the cap is pinned to the request's number. + let over = + BrokerResponse::new(request.request_id(), page(allowed + 1)).validate_for(&request); + let error = over.unwrap_err().to_string(); + if allowed < actions::MAX_PAGE_LIMIT as usize { + assert!( + error.contains(&format!("limit of {allowed}")), + "unexpected error for a limit of {allowed}: {error}" + ); + } + } + + // The default is a real bound, not the cap under another name: a host that + // answers an unlimited read with a cap-sized page is still overrunning it. + const { + assert!(actions::DEFAULT_PAGE_LIMIT < actions::MAX_PAGE_LIMIT); + } + let unlimited = prepared(ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL))); + assert!(BrokerResponse::new( + unlimited.request_id(), + page(actions::MAX_PAGE_LIMIT as usize) + ) + .validate_for(&unlimited) + .is_err()); +} + +// ── Results ───────────────────────────────────────────────────────────────── + +#[test] +fn failed_and_indeterminate_are_distinct_and_carry_no_outcome() { + let failed = BrokerResult::failed(BrokerError::new( + BrokerErrorCode::ActionFailed, + "runtime not installed", + )); + let failed_json = serde_json::to_value(BrokerResponse::new("r", failed.clone())).unwrap(); + assert_eq!(failed_json["status"], "failed"); + assert_eq!(failed_json["error"]["code"], "action_failed"); + assert!(failed_json.get("outcome").is_none()); + + let indeterminate = BrokerResult::indeterminate(BrokerError::new( + BrokerErrorCode::OutcomeUnknown, + "host restarted mid-execution", + )); + let json = serde_json::to_value(BrokerResponse::new("r", indeterminate.clone())).unwrap(); + assert_eq!(json["status"], "indeterminate"); + assert_eq!(json["error"]["code"], "outcome_unknown"); + assert!(json.get("outcome").is_none()); + + assert_ne!(failed, indeterminate); + assert!(failed.outcome().is_none()); + assert!(indeterminate.outcome().is_none()); +} + +/// A code and a status are two statements about the same fact — whether side +/// effects landed — so the contract fixes which pairings are meaningful and +/// rejects the rest. Driven across every code × both statuses, so adding a code +/// forces a decision here. +#[test] +fn status_and_error_code_must_agree_about_side_effects() { + use BrokerErrorCode as E; + for code in all_error_codes() { + let failed = + BrokerResponse::new("req-1", BrokerResult::failed(BrokerError::new(code, "?"))) + .validate(); + let indeterminate = BrokerResponse::new( + "req-1", + BrokerResult::indeterminate(BrokerError::new(code, "?")), + ) + .validate(); + + // The table, spelled out independently of the predicates it checks: a + // second copy is the point, since a test that asked `may_be_failed()` + // would pass for any implementation of it. Exhaustive with no wildcard, + // so a new code cannot inherit an answer — it must be decided here too. + let (failed_ok, indeterminate_ok) = match code { + E::InvalidRequest + | E::UnsupportedProtocolVersion + | E::UnknownAction + | E::UnsupportedActionVersion + | E::Unsupported + | E::Unauthenticated + | E::Unauthorized + | E::RequestIdConflict + | E::ActionFailed => (true, false), + E::OutcomeUnknown => (false, true), + E::Internal => (true, true), + }; + + assert_eq!( + failed.is_ok(), + failed_ok, + "{} with a failed status: {failed:?}", + code.as_str() + ); + assert_eq!( + indeterminate.is_ok(), + indeterminate_ok, + "{} with an indeterminate status: {indeterminate:?}", + code.as_str() + ); + assert_eq!(code.may_be_failed(), failed_ok); + assert_eq!(code.may_be_indeterminate(), indeterminate_ok); + } + + // The two directions review found, named: a rejected credential is a + // known-fate refusal and cannot claim not to know, and `outcome_unknown` + // cannot claim a clean failure. + let error = BrokerResponse::new( + "req-1", + BrokerResult::indeterminate(BrokerError::new(E::Unauthenticated, "credential rejected")), + ) + .validate() + .unwrap_err() + .to_string(); + assert!(error.contains("unauthenticated"), "unexpected: {error}"); + let error = BrokerResponse::new( + "req-1", + BrokerResult::failed(BrokerError::new(E::OutcomeUnknown, "?")), + ) + .validate() + .unwrap_err() + .to_string(); + assert!(error.contains("outcome_unknown"), "unexpected: {error}"); +} + +#[test] +fn replay_metadata_rides_the_response_not_the_result() { + let result = BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: pubkey(), + display_name: "Gone".into(), + })); + let fresh = BrokerResponse::new("req-9", result.clone()); + let replayed = BrokerResponse::new("req-9", result.clone()).replayed(); + + // The domain outcome is identical; only the delivery metadata differs. + assert_eq!(fresh.result, replayed.result); + assert!(!fresh.replayed); + assert!(replayed.replayed); + assert_eq!( + serde_json::to_value(&replayed).unwrap()["replayed"], + serde_json::json!(true) + ); + + // `replayed` is not part of the stored result encoding. + assert!(serde_json::to_value(&result) + .unwrap() + .get("replayed") + .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 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" + ); + // Correlation metadata is all a transport gets. There is deliberately no + // accessor for the typed request: one would let an implementation serialize + // the value a second time, which is the possibility freezing removes. + 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. +/// This is also what pins [`all_error_codes`] against the enum: a new variant +/// missing from that fixture changes the joined string and fails here. +#[test] +fn error_codes_have_stable_wire_strings() { + let codes = all_error_codes(); + for code in codes { + assert_eq!( + serde_json::to_value(code).unwrap(), + serde_json::json!(code.as_str()), + "as_str and the serde name must not drift" + ); + } + assert_eq!( + codes.map(BrokerErrorCode::as_str).join(","), + "invalid_request,unsupported_protocol_version,unknown_action,\ +unsupported_action_version,unsupported,unauthenticated,unauthorized,\ +request_id_conflict,action_failed,outcome_unknown,internal" + ); +} + +// ── Client trait ──────────────────────────────────────────────────────────── + +/// A test double, and the only implementation in this crate. It exists to prove +/// the trait is object-safe and usable behind `dyn`, which is what lets an +/// in-process host and an HTTP client be interchangeable. +/// +/// Note what it does *not* do: it never calls `validate_for`. It cannot — it has +/// no way to build a [`ValidatedResponse`] except through the blanket +/// [`BrokerClientExt::execute`], which is the whole point of splitting the +/// trait. A deliberately hostile implementation is still forced through the +/// same check. +struct DoubleBroker { + response: Result, +} + +impl BrokerClient for DoubleBroker { + fn send<'a>(&'a self, request: &'a PreparedRequest, _: Dispatch) -> BrokerFuture<'a> { + // A real implementation sends `request.body()` verbatim. The double + // stands in for a host that answers under the id it was asked with, and + // returns the envelope unjudged. + let response = self.response.clone().map(|mut response| { + response.request_id = request.request_id().to_string(); + response + }); + Box::pin(async move { response }) + } +} + +fn block_on(future: F) -> F::Output { + // A hand-rolled park-free executor: the double's future is always ready, so + // one poll suffices and pulling in a runtime would be the heavier choice. + use std::task::{Context, Poll, Wake, Waker}; + struct NoopWake; + impl Wake for NoopWake { + fn wake(self: std::sync::Arc) {} + } + let waker = Waker::from(std::sync::Arc::new(NoopWake)); + let mut context = Context::from_waker(&waker); + let mut future = Box::pin(future); + match future.as_mut().poll(&mut context) { + Poll::Ready(output) => output, + Poll::Pending => panic!("test double must not park"), + } +} + +#[test] +fn the_client_trait_is_object_safe_and_returns_a_validated_host_verdict() { + 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( + "placeholder", + BrokerResult::succeeded(ActionOutcome::ChannelRead(MessagePage { + messages: vec![], + next_cursor: None, + })), + )), + }); + // `execute` is available on `dyn BrokerClient` and is the only way to a + // `ValidatedResponse` — the caller does no correlation of its own. + let response = block_on(succeeded.execute(&request)).expect("double answers"); + assert_eq!(response.request_id(), "req-1"); + assert!(response.result().outcome().is_some()); + assert!(!response.replayed()); + + // 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); + } +} + +/// The double returns whatever it is given, unvalidated — a hostile client +/// cannot do otherwise. `execute` is still the only door, so the mismatch +/// surfaces as a transport failure and never reaches a caller as `Ok`. +#[test] +fn a_client_cannot_hand_back_a_response_that_answers_a_different_request() { + let request = prepared(ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL))); + + // Wrong action for this request. + let confused: Box = Box::new(DoubleBroker { + response: Ok(BrokerResponse::new( + "placeholder", + BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: pubkey(), + display_name: "Gone".into(), + })), + )), + }); + // The envelope is well-formed in isolation — that is exactly why `send` + // cannot be the caller's door. `execute` is the only reachable one (a + // `Dispatch` token cannot be built outside the client module), and it + // rejects the mismatch rather than passing it on. + assert!(matches!( + block_on(confused.execute(&request)).unwrap_err(), + BrokerTransportError::MalformedResponse(_) + )); + + // Malformed identifiers inside an otherwise well-shaped outcome, too. + let bad_cursor: Box = Box::new(DoubleBroker { + response: Ok(BrokerResponse::new( + "placeholder", + BrokerResult::succeeded(ActionOutcome::ChannelRead(MessagePage { + messages: vec![], + next_cursor: Some("not a cursor".into()), + })), + )), + }); + assert!(matches!( + block_on(bad_cursor.execute(&request)).unwrap_err(), + BrokerTransportError::MalformedResponse(_) + )); + + // A status contradicting its own code, which is how the review reached this: + // `unauthenticated` is a known pre-dispatch refusal, so claiming not to know + // the fate is not a verdict `execute` may pass on as `Ok`. + let contradictory: Box = Box::new(DoubleBroker { + response: Ok(BrokerResponse::new( + "placeholder", + BrokerResult::indeterminate(BrokerError::new( + BrokerErrorCode::Unauthenticated, + "credential rejected", + )), + )), + }); + assert!(matches!( + block_on(contradictory.execute(&request)).unwrap_err(), + BrokerTransportError::MalformedResponse(_) + )); +} + +/// A second double, parsing bytes the way a real HTTP client does, because the +/// strict-envelope and strict-event guards live in `Deserialize` and the typed +/// double above can never exercise them: it hands back a value that was never on +/// a wire. +/// +/// This is the shape the bug actually had — bytes arriving from a host — and what +/// the caller sees now is [`BrokerTransportError::MalformedResponse`], not an +/// `Ok` whose extra members were quietly dropped. +struct WireBroker { + body: Vec, +} + +impl BrokerClient for WireBroker { + fn send<'a>(&'a self, _: &'a PreparedRequest, _: Dispatch) -> BrokerFuture<'a> { + // Exactly a transport's job: parse an envelope, and report the absence + // of one as a transport failure. + let parsed = serde_json::from_slice::(&self.body) + .map_err(|e| BrokerTransportError::MalformedResponse(e.to_string())); + Box::pin(async move { parsed }) + } +} + +#[test] +fn bytes_carrying_more_than_the_contract_declares_never_reach_a_caller_as_ok() { + let signer = Keys::generate(); + let request = prepared(ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL))); + let event = serde_json::to_value(signed_message(&signer)).expect("event serializes"); + let envelope = || { + serde_json::json!({ + "type": BROKER_RESULT_TYPE, + "protocolVersion": 1, + "requestId": request.request_id(), + "status": "succeeded", + "action": "channel.read", + "outcome": { "messages": [event.clone()] }, + }) + }; + + // The honest bytes are accepted, so the rejections below are about the + // smuggled members and not about this fixture being unparseable. + let client = WireBroker { + body: serde_json::to_vec(&envelope()).unwrap(), + }; + let response = block_on(client.execute(&request)).expect("honest bytes are an answer"); + assert!(response.result().outcome().is_some()); + + // A key at each depth: on the envelope, inside the outcome, and inside the + // signed event — the last being the one `nostr` would have discarded. + let mut on_envelope = envelope(); + on_envelope["secretKey"] = serde_json::json!("nsec1deadbeef"); + let mut in_outcome = envelope(); + in_outcome["outcome"]["secretKey"] = serde_json::json!("nsec1deadbeef"); + let mut in_event = envelope(); + in_event["outcome"]["messages"][0]["secretKey"] = serde_json::json!("nsec1deadbeef"); + // And the contradiction the envelope could previously hold on the wire. + let mut error_beside_success = envelope(); + error_beside_success["error"] = serde_json::json!({ "code": "internal", "message": "?" }); + + for (what, json) in [ + ("on the envelope", on_envelope), + ("inside the outcome", in_outcome), + ("inside the event", in_event), + ("an error beside a success", error_beside_success), + ] { + let client = WireBroker { + body: serde_json::to_vec(&json).unwrap(), + }; + assert!( + matches!( + block_on(client.execute(&request)), + Err(BrokerTransportError::MalformedResponse(_)) + ), + "{what}: must not reach the caller as Ok" + ); + } +} diff --git a/crates/buzz-sdk/src/broker/wire.rs b/crates/buzz-sdk/src/broker/wire.rs new file mode 100644 index 0000000000..851d1a165f --- /dev/null +++ b/crates/buzz-sdk/src/broker/wire.rs @@ -0,0 +1,107 @@ +//! The strict wire form of a [`BrokerResponse`]. +//! +//! Split from [`super`] to keep that file within the repo's 1,000-line ceiling. +//! This is the response side's only reader, so it is the one place the envelope's +//! strictness is defined. + +use serde::Deserialize; + +use super::{absent_or_valued, ActionOutcome, BrokerError, BrokerResponse, BrokerResult}; + +/// The strict wire form of a [`BrokerResponse`]: every key spelled out, no +/// `flatten`, so `deny_unknown_fields` is actually in force. +/// +/// The status-specific members are `Option` only because one struct describes +/// three shapes; the status match below requires the exact set per status. +/// They deserialize through [`absent_or_valued`] because the match reads +/// `None` as *absent*, and plain `#[serde(default)]` would map an explicit +/// `null` to the same `None` — letting a contradictory response like +/// `{"status":"failed","outcome":null}` skip the check. +/// +/// `outcome` is held as a `RawValue` and re-parsed, so this reader is +/// JSON-specific — which is fine, JSON is the only encoding this contract has +/// ever specified. +#[derive(Deserialize)] +#[serde(deny_unknown_fields, rename_all = "camelCase")] +struct WireResponse { + r#type: String, + protocol_version: u16, + request_id: String, + status: String, + #[serde(default, deserialize_with = "absent_or_valued")] + action: Option, + #[serde(default, deserialize_with = "absent_or_valued")] + outcome: Option>, + #[serde(default, deserialize_with = "absent_or_valued")] + error: Option, + #[serde(default)] + replayed: bool, +} + +impl<'de> Deserialize<'de> for BrokerResponse { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error as _; + + let wire = WireResponse::deserialize(deserializer)?; + + // One arm per status, each naming the members that status may carry. + // Anything present that this status does not admit is rejected here, so + // "succeeded with an error" cannot be parsed and then ignored. + let result = match wire.status.as_str() { + "succeeded" => { + if wire.error.is_some() { + return Err(D::Error::custom( + "a succeeded response must not carry an error", + )); + } + let action = wire + .action + .ok_or_else(|| D::Error::missing_field("action"))?; + let outcome = wire + .outcome + .ok_or_else(|| D::Error::missing_field("outcome"))?; + // Re-parse the outcome under its action tag from the original + // bytes — not via `serde_json::Value`, which collapses + // duplicate keys last-wins — so each outcome type's + // `deny_unknown_fields` applies to the bytes as sent. + let tagged = format!( + "{{\"action\":{},\"outcome\":{}}}", + serde_json::to_string(&action).map_err(D::Error::custom)?, + outcome.get() + ); + let outcome: ActionOutcome = + serde_json::from_str(&tagged).map_err(D::Error::custom)?; + BrokerResult::Succeeded { outcome } + } + status @ ("failed" | "indeterminate") => { + if wire.action.is_some() || wire.outcome.is_some() { + return Err(D::Error::custom(format!( + "a {status} response must not carry an action or outcome" + ))); + } + let error = wire.error.ok_or_else(|| D::Error::missing_field("error"))?; + if status == "failed" { + BrokerResult::Failed { error } + } else { + BrokerResult::Indeterminate { error } + } + } + other => { + return Err(D::Error::custom(format!( + "unknown broker result status \"{other}\"" + ))) + } + }; + + Ok(Self { + r#type: wire.r#type, + protocol_version: wire.protocol_version, + request_id: wire.request_id, + result, + replayed: wire.replayed, + }) + } +} diff --git a/crates/buzz-sdk/src/lib.rs b/crates/buzz-sdk/src/lib.rs index 4ee0cd4c88..845505c56d 100644 --- a/crates/buzz-sdk/src/lib.rs +++ b/crates/buzz-sdk/src/lib.rs @@ -12,6 +12,7 @@ //! The caller signs with their own keys: `builder.sign_with_keys(&keys)?`. //! No keys are held here. No network calls are made. +pub mod broker; pub mod builders; pub mod mentions; pub mod nip_oa; From c856be0fb954c9e5267d622841098c24e3381e8f Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 27 Aug 2026 17:37:42 +1000 Subject: [PATCH 03/37] fix(desktop): keep the draft space when typing right after a mention pick (#6875) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Desktop Smoke E2E flaked on `mentions.spec.ts` ("typing a mention before existing text does not interleave spaces"): picking a suggestion mid-draft and typing immediately could swallow the draft's own space, producing `hello @bob abcworld` instead of `hello @bob a world`. With `hello world` drafted and ` @bo` typed after `hello`, picking the suggestion leaves `hello @bob world` — the mention's trailing space followed by the draft's space. Chromium reconciles that whitespace run by re-emitting it as a replacement (`replace(" " -> " a")`), anchored at *either* edge of the run. The settling-time text-input redirect only recognized a caret or a rewrite anchored at the chip edge, so the other shapes fell through to ProseMirror's destructive default and ate the separator. The race pre-dates this branch and reproduces on `main`. ## Fix `mentionHighlightExtension.ts` now redirects on the invariant instead of on a specific rewrite shape. While autocomplete is settling the user cannot have a selection (a selection cancels settlement), so a whitespace-only replacement adjacent to a mention's trailing space is always the browser normalizing whitespace — never an intentional delete. The handler keeps every space already in the document and inserts only the typed characters after the token, whichever edge the rewrite is anchored at. Non-whitespace replacements still fall through as real edits. `insertPosForMentionTextInput` / `mentionTextInputInsertPos` become `insertionForMentionTextInput` / `mentionTextInputInsertion`, returning `{insertAt, text}` so the handler can rewrite the payload, not just the position. ## Tests - Helper-level cases for all four anchors (caret at chip edge, caret past the trailing space, run rewritten from the chip edge, draft space rewritten alone), plus space/NBSP variants and a non-whitespace replacement that must be left alone. - A plugin-level test drives the real `handleTextInput` prop and falls back to ProseMirror's default when it declines — exactly as the browser does with the return value — so it fails against the previous production code rather than depending on winning a timing race in a headless browser. Desktop unit suite passes on pre-push (5558/5558). --------- Signed-off-by: Matt Toohey Co-authored-by: Claude Fable 5 --- .../lib/mentionHighlightExtension.test.mjs | 159 ++++++++++++++++-- .../messages/lib/mentionHighlightExtension.ts | 89 +++++++--- 2 files changed, 213 insertions(+), 35 deletions(-) diff --git a/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs b/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs index 6c7055c20d..106b036934 100644 --- a/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs +++ b/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs @@ -10,9 +10,10 @@ import { buildHighlightPatterns, createMentionCaretSettlement, findHighlightMatches, - insertPosForMentionTextInput, + insertionForMentionTextInput, MentionHighlightExtension, - mentionTextInputInsertPos, + mentionHighlightKey, + mentionTextInputInsertion, positionAfterArrowLeftThroughMentionSpace, selectionAfterMentionTrailingSpace, shouldAdvanceMentionCaret, @@ -252,35 +253,79 @@ test("createMentionCaretSettlement keeps two editors independent", () => { assert.equal(composerB.peek(), 12); }); -test("insertPosForMentionTextInput redirects a caret at the chip edge", () => { +test("insertionForMentionTextInput redirects a caret at the chip edge", () => { const doc = document(paragraph(text("@quinn "))); const spacePos = 1 + "@quinn".length; + assert.deepEqual(insertionForMentionTextInput(doc, spacePos, spacePos, "x"), { + insertAt: spacePos + 1, + text: "x", + }); assert.equal( - insertPosForMentionTextInput(doc, spacePos, spacePos), - spacePos + 1, - ); - assert.equal( - insertPosForMentionTextInput(doc, spacePos + 1, spacePos + 1), + insertionForMentionTextInput(doc, spacePos + 1, spacePos + 1, "x"), null, ); }); -test("insertPosForMentionTextInput keeps a selected trailing space", () => { +test("insertionForMentionTextInput keeps a selected trailing space", () => { const doc = document(paragraph(text("@quinn "))); const spacePos = 1 + "@quinn".length; - assert.equal( - insertPosForMentionTextInput(doc, spacePos, spacePos + 1), - spacePos + 1, + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 1, "x"), + { insertAt: spacePos + 1, text: "x" }, ); }); -test("mentionTextInputInsertPos honors a deliberate caret after settlement", () => { +test("insertionForMentionTextInput keeps the draft space in a whitespace-run rewrite", () => { + // Chromium can rewrite the whole space run when typing between the + // mention's trailing space and a pre-existing draft space, emitting + // replace(" " -> " a") — usually with a non-breaking space, and anchored + // at either edge of the run. The draft's space must survive every shape. + const doc = document(paragraph(text("hello @bob world"))); + const spacePos = 1 + "hello @bob".length; + const kept = { insertAt: spacePos + 1, text: "a" }; + // Anchored at the chip edge, replacing the whole run. + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 2, " a"), + kept, + ); + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 2, "\u00A0a"), + kept, + ); + // Anchored past the trailing space, rewriting only the draft's own space. + // This shape used to fall through to the destructive default and produce + // "hello @bob abcworld" — the failure CI caught. + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos + 1, spacePos + 2, "\u00A0a"), + kept, + ); + // Trailing whitespace is the run being re-emitted on the other side. + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 2, "a\u00A0"), + kept, + ); + // Whatever the shape, replaced whitespace is never dropped while settling. + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 2, "x"), + { insertAt: spacePos + 1, text: "x" }, + ); + // Replacing something other than whitespace is a real edit — leave it. + assert.equal( + insertionForMentionTextInput(doc, spacePos, spacePos + 3, " a"), + null, + ); +}); + +test("mentionTextInputInsertion honors a deliberate caret after settlement", () => { const doc = document(paragraph(text("@bob "))); const spacePos = 1 + "@bob".length; - assert.equal(mentionTextInputInsertPos(doc, spacePos, spacePos, false), null); assert.equal( - mentionTextInputInsertPos(doc, spacePos, spacePos, true), - spacePos + 1, + mentionTextInputInsertion(doc, spacePos, spacePos, "x", false), + null, + ); + assert.deepEqual( + mentionTextInputInsertion(doc, spacePos, spacePos, "x", true), + { insertAt: spacePos + 1, text: "x" }, ); }); @@ -378,3 +423,85 @@ test("typing after a completed mention keeps the separator intact", () => { const typed = typeAt(state, 1 + "@quinn world".length, "!"); assert.equal(typed.doc.textContent, "@quinn world!"); }); + +// ── the browser branch: Chromium's whitespace-run rewrite ───────────── +// +// The helper tests above model the payload. These drive the plugin's real +// `handleTextInput` prop and fall back to ProseMirror's default insertion +// when it declines, exactly as the browser does with the return value — so +// the assertion is sensitive to the production branch itself rather than to +// winning a timing race in a headless browser. + +/** Apply an autocomplete pick: replace the typed token and settle the caret. */ +function pickMentionAt(state, tokenFrom, tokenTo, inserted) { + const tr = state.tr.insertText(inserted, tokenFrom, tokenTo); + tr.setSelection(TextSelection.create(tr.doc, tokenFrom + inserted.length)); + tr.setMeta(mentionHighlightKey, true); + return state.apply(tr); +} + +/** Route a text-input event through the plugin, then the default handling. */ +function textInput(state, from, to, text) { + let current = state; + const view = { + get state() { + return current; + }, + dispatch(tr) { + current = current.apply(tr); + }, + domAtPos: () => ({ node: {}, offset: 0 }), + root: undefined, + }; + const handled = current.plugins.some( + (plugin) => plugin.props?.handleTextInput?.(view, from, to, text) === true, + ); + return handled + ? current + : current.apply(current.tr.insertText(text, from, to)); +} + +test("a whitespace-run rewrite after a mention pick keeps the draft space", () => { + // The CI failure: with "hello world" drafted, the caret placed after + // "hello", " @bo" typed and the "bob" suggestion picked, the document is + // "hello @bob world" — the mention's trailing space followed by the + // draft's own space. The next keystroke arrives as one of these shapes + // depending on how Chromium reconciles that whitespace run, and all of + // them have to keep both spaces. + const tokenFrom = 1 + "hello ".length; + const spacePos = 1 + "hello @bob".length; + const shapes = [ + { name: "caret at the chip edge", from: spacePos, to: spacePos, text: "a" }, + { + name: "caret past the trailing space", + from: spacePos + 1, + to: spacePos + 1, + text: "a", + }, + { + name: "run rewritten from the chip edge", + from: spacePos, + to: spacePos + 2, + text: "\u00A0a", + }, + { + name: "draft space rewritten on its own", + from: spacePos + 1, + to: spacePos + 2, + text: "\u00A0a", + }, + ]; + + for (const shape of shapes) { + // A fresh editor per shape: settlement is per-plugin closure state. + const picked = pickMentionAt( + editorStateWithMentionHighlight("hello @bo world", ["bob"]), + tokenFrom, + tokenFrom + "@bo".length, + "@bob ", + ); + assert.equal(picked.doc.textContent, "hello @bob world", shape.name); + const typed = textInput(picked, shape.from, shape.to, shape.text); + assert.equal(typed.doc.textContent, "hello @bob a world", shape.name); + } +}); diff --git a/desktop/src/features/messages/lib/mentionHighlightExtension.ts b/desktop/src/features/messages/lib/mentionHighlightExtension.ts index 3d1d5d6ef7..e55c79c9a3 100644 --- a/desktop/src/features/messages/lib/mentionHighlightExtension.ts +++ b/desktop/src/features/messages/lib/mentionHighlightExtension.ts @@ -58,25 +58,71 @@ export function shouldAdvanceMentionCaret({ return next !== from && settling; } +export type MentionTextInsertion = { + insertAt: number; + text: string; +}; + +const SPACE_RUN = /^[ \u00A0]+$/; +const OUTER_SPACES = /^[ \u00A0]+|[ \u00A0]+$/g; + /** - * Where to insert typed text when the caret (or a one-character selection) - * sits on the trailing space after an `@name` / `#channel` token. - * A selected trailing space would otherwise be replaced, producing - * `@bobhello`. + * Position just after the trailing space of the mention token that `pos` is + * adjacent to, or `null` when `pos` is nowhere near one. + * + * `pos` may sit at the token end (before the space) or already past the + * space: when Chromium rewrites the whitespace run around the caret it + * anchors the replacement at either edge, and both mean the same boundary. */ -export function insertPosForMentionTextInput( +function mentionTrailingSpaceBoundary( + doc: ProseMirrorNode, + pos: number, +): number | null { + const afterSpace = selectionAfterMentionTrailingSpace(doc, pos); + if (afterSpace !== pos) return afterSpace; + if (pos > 0 && selectionAfterMentionTrailingSpace(doc, pos - 1) === pos) { + return pos; + } + return null; +} + +/** + * Where (and what) to insert when typed text arrives at the trailing space + * after an `@name` / `#channel` token. + * + * - Caret on the space: insert after it, so the next keystroke lands after + * the token (`@bobhello` fix). + * - Whitespace replaced next to that space: keep every space the document + * already has and insert only the typed characters after the token's + * trailing space. + * + * The second rule matters because typing between the mention's trailing + * space and a pre-existing draft space makes Chromium re-emit the whole + * whitespace run — `replace(" " -> " a")`, usually with a non-breaking + * space, and anchored at either edge of the run. Applying any of those + * verbatim deletes the draft's space (`hello @bob abcworld`). + * + * Only whitespace is ever redirected, and only while autocomplete is + * settling — a window in which the user cannot have selected anything, + * because a selection cancels settlement. So a replacement arriving here + * is the browser normalizing whitespace, never an intentional delete, and + * preserving the document's spaces is the whole invariant. Recognizing one + * specific rewrite shape instead is what left the draft space exposed. + */ +export function insertionForMentionTextInput( doc: ProseMirrorNode, from: number, to: number, -): number | null { - const next = selectionAfterMentionTrailingSpace(doc, from); + text: string, +): MentionTextInsertion | null { if (from === to) { - return next === from ? null : next; + const next = selectionAfterMentionTrailingSpace(doc, from); + return next === from ? null : { insertAt: next, text }; } - if (to === next && next === from + 1) { - return next; - } - return null; + const boundary = mentionTrailingSpaceBoundary(doc, from); + if (boundary === null) return null; + if (!SPACE_RUN.test(doc.textBetween(from, to, "\n", "\0"))) return null; + return { insertAt: boundary, text: text.replace(OUTER_SPACES, "") }; } /** @@ -84,14 +130,15 @@ export function insertPosForMentionTextInput( * deliberate ArrowLeft or chip click, honor the caret so `x` lands in the * token (`@bobx`) instead of after the space (`@bob x`). */ -export function mentionTextInputInsertPos( +export function mentionTextInputInsertion( doc: ProseMirrorNode, from: number, to: number, + text: string, settling: boolean, -): number | null { +): MentionTextInsertion | null { if (!settling) return null; - return insertPosForMentionTextInput(doc, from, to); + return insertionForMentionTextInput(doc, from, to, text); } /** Caret just after a mention trailing space: ArrowLeft lands on the token end. */ @@ -396,18 +443,22 @@ export const MentionHighlightExtension = Extension.create({ return this.getState(state) ?? DecorationSet.empty; }, handleTextInput(view, from, to, text) { - const insertAt = mentionTextInputInsertPos( + const insertion = mentionTextInputInsertion( view.state.doc, from, to, + text, settlement.peek() !== null, ); - if (insertAt == null) { + if (insertion == null) { settlement.cancel(); return false; } - const tr = view.state.tr.insertText(text, insertAt); - const caret = tr.mapping.map(insertAt, 1); + const tr = view.state.tr.insertText( + insertion.text, + insertion.insertAt, + ); + const caret = tr.mapping.map(insertion.insertAt, 1); tr.setSelection(TextSelection.create(tr.doc, caret)); view.dispatch(tr); settlement.cancel(); From 4d77175c78f4df48e13398b25b271e3ac1b31dd3 Mon Sep 17 00:00:00 2001 From: klopez4212 Date: Thu, 27 Aug 2026 15:29:26 +0100 Subject: [PATCH 04/37] fix(client): resurface hidden DMs from live activity (#6885) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Hiding a DM row and then receiving a new message from that person never brought the row back — the reader was left with only an Inbox notification. This restores the row from live activity, client-side, with no relay or database changes. - Subscribe to hidden-DM activity **channel-scoped** over the hidden-DM id set (`#h`), re-registering whenever the set changes. The relay's `fan_out_scoped` only routes channel-scoped events (every kind:9 DM carries a `channel_id`) to channel-scoped subscriptions, so a community-global `#p` filter receives nothing. Hiding a DM never drops membership, so `#h` authorization holds. Because every delivered event is already for a hidden DM the reader belongs to, eligibility no longer needs a self `p` tag — untagged CLI/agent DMs resurface too — and no per-event visibility fetch is required. - On qualifying activity, invoke the existing open-DM action (`41010`) to reopen the hidden one-to-one or group DM, validating that the relay reopened the same channel. - Preserve the existing relay-synced hide snapshot (`41012` → `30622`); hide state still syncs across devices. - Let Inbox reopen a hidden DM before navigating to its message, with a busy state on the row and context-menu "Open in channel" action while a reopen is pending. - Coalesce resurface attempts per channel: a concurrent message that arrives while a reopen is in flight triggers a retry on failure instead of being marked handled and dropped. - Write exact dm-visibility query keys on open/hide mutations. This is the client-only replacement agreed in the Buzz design thread and supersedes #6713. The accepted limitation is that resurface only fires while a client is running — a message that arrives while all clients are closed does not restore the row on next launch (the `30622` snapshot has no watermark); Inbox click-through is the safety net for that case. Addresses #6316. --------- Signed-off-by: kenny lopez Signed-off-by: Will Pfleger Co-authored-by: Duncan Co-authored-by: Will Pfleger Co-authored-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> --- desktop/src/app/AppShell.tsx | 6 + .../features/channels/dmResurface.test.mjs | 93 ++ desktop/src/features/channels/dmResurface.ts | 66 ++ .../channels/hiddenDmResurfaceAction.test.mjs | 121 +++ .../channels/hiddenDmResurfaceAction.ts | 49 ++ .../hiddenDmResurfaceCoordinator.test.mjs | 92 ++ .../channels/hiddenDmResurfaceCoordinator.ts | 60 ++ desktop/src/features/channels/hooks.ts | 29 +- .../channels/useDmResurfaceBatching.test.mjs | 278 ++++++ .../channels/useDmResurfaceFromMessages.ts | 141 ++++ .../src/features/channels/useHiddenDmIds.ts | 62 ++ .../home/hiddenDmInboxAction.test.mjs | 141 ++++ .../src/features/home/hiddenDmInboxAction.ts | 76 ++ desktop/src/features/home/ui/HomeScreen.tsx | 33 +- desktop/src/features/home/ui/HomeView.tsx | 50 +- .../src/features/home/ui/InboxDetailPane.tsx | 47 ++ .../src/features/home/ui/InboxListPane.tsx | 73 +- .../home/ui/inboxReopenNavigation.test.mjs | 794 ++++++++++++++++++ .../home/useHiddenDmInboxNavigation.ts | 151 ++++ desktop/src/shared/api/relayClientShared.ts | 9 + .../lib/features/activity/activity_page.dart | 51 +- .../features/activity/activity_provider.dart | 301 ++++++- .../lib/features/activity/dm_resurface.dart | 27 + .../features/channels/channels_provider.dart | 20 +- .../channels/channels_provider_lifecycle.dart | 18 + mobile/lib/shared/relay/nostr_filters.dart | 7 + .../activity/activity_provider_test.dart | 744 +++++++++++++++- .../features/activity/dm_resurface_test.dart | 61 ++ 28 files changed, 3511 insertions(+), 89 deletions(-) create mode 100644 desktop/src/features/channels/dmResurface.test.mjs create mode 100644 desktop/src/features/channels/dmResurface.ts create mode 100644 desktop/src/features/channels/hiddenDmResurfaceAction.test.mjs create mode 100644 desktop/src/features/channels/hiddenDmResurfaceAction.ts create mode 100644 desktop/src/features/channels/hiddenDmResurfaceCoordinator.test.mjs create mode 100644 desktop/src/features/channels/hiddenDmResurfaceCoordinator.ts create mode 100644 desktop/src/features/channels/useDmResurfaceBatching.test.mjs create mode 100644 desktop/src/features/channels/useDmResurfaceFromMessages.ts create mode 100644 desktop/src/features/channels/useHiddenDmIds.ts create mode 100644 desktop/src/features/home/hiddenDmInboxAction.test.mjs create mode 100644 desktop/src/features/home/hiddenDmInboxAction.ts create mode 100644 desktop/src/features/home/ui/inboxReopenNavigation.test.mjs create mode 100644 desktop/src/features/home/useHiddenDmInboxNavigation.ts create mode 100644 mobile/lib/features/activity/dm_resurface.dart create mode 100644 mobile/lib/features/channels/channels_provider_lifecycle.dart create mode 100644 mobile/test/features/activity/dm_resurface_test.dart diff --git a/desktop/src/app/AppShell.tsx b/desktop/src/app/AppShell.tsx index 468435e15e..eb5ab5a95d 100644 --- a/desktop/src/app/AppShell.tsx +++ b/desktop/src/app/AppShell.tsx @@ -34,6 +34,7 @@ import { useHideDmMutation, useOpenDmMutation, } from "@/features/channels/hooks"; +import { useDmResurfaceFromMessages } from "@/features/channels/useDmResurfaceFromMessages"; import { useUnreadChannels } from "@/features/channels/useUnreadChannels"; import { useMembershipNotifications } from "@/features/channels/useMembershipNotifications"; import { useFeedItemState } from "@/features/home/useFeedItemState"; @@ -505,6 +506,11 @@ export function AppShell() { const { applyCanvas, applyAgents } = useApplyTemplate(); const openDmMutation = useOpenDmMutation(); const hideDmMutation = useHideDmMutation(); + useDmResurfaceFromMessages({ + pubkey: identityQuery.data?.pubkey, + relayUrl: communitiesHook.activeCommunity?.relayUrl, + reopen: openDmMutation.mutateAsync, + }); const { browseDialogType, openBrowseChannels: handleOpenBrowseChannels, diff --git a/desktop/src/features/channels/dmResurface.test.mjs b/desktop/src/features/channels/dmResurface.test.mjs new file mode 100644 index 0000000000..81e09fa89e --- /dev/null +++ b/desktop/src/features/channels/dmResurface.test.mjs @@ -0,0 +1,93 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + dmPeerPubkeysFromMembers, + isIncomingChannelMessageFromOther, + markHiddenDmFeedItems, +} from "./dmResurface.ts"; + +const SELF = "1".repeat(64); +const ALICE = "2".repeat(64); +const BOB = "3".repeat(64); + +function item(overrides = {}) { + return { + id: "event-1", + kind: 9, + pubkey: ALICE, + content: "hello", + createdAt: 10, + channelId: "dm-1", + channelName: "", + tags: [ + ["h", "dm-1"], + ["p", SELF], + ["p", BOB], + ], + category: "mention", + ...overrides, + }; +} + +function relayEvent(overrides = {}) { + return { + id: "event-1", + kind: 40002, + pubkey: ALICE, + content: "hello", + created_at: 10, + tags: [ + ["h", "dm-1"], + ["p", SELF], + ["p", BOB], + ], + sig: "", + ...overrides, + }; +} + +test("DM resurface derives peers from authoritative membership", () => { + const members = [{ pubkey: SELF }, { pubkey: ALICE }, { pubkey: BOB }]; + assert.deepEqual(dmPeerPubkeysFromMembers(members, SELF), [ALICE, BOB]); + assert.deepEqual(dmPeerPubkeysFromMembers([{ pubkey: ALICE }], SELF), []); +}); + +test("only external channel messages qualify, regardless of p tags", () => { + // #h-scoped delivery already guarantees relevance, so eligibility no longer + // requires a self `p` tag — an untagged DM from another sender still counts. + assert.equal(isIncomingChannelMessageFromOther(relayEvent(), SELF), true); + assert.equal( + isIncomingChannelMessageFromOther(relayEvent({ kind: 7 }), SELF), + false, + ); + assert.equal( + isIncomingChannelMessageFromOther(relayEvent({ pubkey: SELF }), SELF), + false, + ); + assert.equal( + isIncomingChannelMessageFromOther(relayEvent({ tags: [] }), SELF), + false, + ); + assert.equal( + isIncomingChannelMessageFromOther( + relayEvent({ tags: [["h", "dm-1"]] }), + SELF, + ), + true, + ); +}); + +test("hidden feed items are projected as DMs for Inbox presentation", () => { + const feed = { + feed: { + mentions: [item()], + needsAction: [], + activity: [], + agentActivity: [], + }, + meta: { since: 0, total: 1, generatedAt: 10 }, + }; + const marked = markHiddenDmFeedItems(feed, new Set(["dm-1"])); + assert.equal(marked.feed.mentions[0].channelType, "dm"); +}); diff --git a/desktop/src/features/channels/dmResurface.ts b/desktop/src/features/channels/dmResurface.ts new file mode 100644 index 0000000000..48e0c2e24c --- /dev/null +++ b/desktop/src/features/channels/dmResurface.ts @@ -0,0 +1,66 @@ +import type { + ChannelMember, + FeedItem, + HomeFeedResponse, + RelayEvent, +} from "@/shared/api/types"; +import { CHANNEL_MESSAGE_EVENT_KINDS } from "@/shared/constants/kinds"; +import { normalizePubkey } from "@/shared/lib/pubkey"; + +const CHANNEL_MESSAGE_KINDS = new Set(CHANNEL_MESSAGE_EVENT_KINDS); +const HEX_PUBKEY = /^[0-9a-f]{64}$/; + +export function dmPeerPubkeysFromMembers( + members: readonly Pick[], + currentPubkey: string | undefined, +): string[] { + const self = normalizePubkey(currentPubkey ?? ""); + const normalized = [ + ...new Set(members.map((member) => normalizePubkey(member.pubkey))), + ].filter((pubkey) => HEX_PUBKEY.test(pubkey)); + if (!HEX_PUBKEY.test(self) || !normalized.includes(self)) return []; + return normalized.filter((pubkey) => pubkey !== self); +} + +// The resurface subscription is `#h`-scoped to the hidden-DM set, so the relay +// only delivers events already addressed to a hidden channel the reader belongs +// to. Eligibility therefore drops the `#p` requirement — an untagged DM (a CLI +// or agent send that omits participant `p` tags) still resurfaces the row. +export function isIncomingChannelMessageFromOther( + event: RelayEvent, + currentPubkey: string | undefined, +): boolean { + const self = normalizePubkey(currentPubkey ?? ""); + return ( + self.length > 0 && + CHANNEL_MESSAGE_KINDS.has(event.kind) && + relayEventChannelId(event) !== null && + normalizePubkey(event.pubkey) !== self + ); +} + +export function relayEventChannelId(event: RelayEvent): string | null { + return event.tags.find((tag) => tag[0] === "h" && tag[1])?.[1] ?? null; +} + +export function markHiddenDmFeedItems( + feed: HomeFeedResponse, + hiddenDmIds: ReadonlySet, +): HomeFeedResponse { + if (hiddenDmIds.size === 0) return feed; + + const mark = (item: FeedItem): FeedItem => + item.channelId && hiddenDmIds.has(item.channelId) + ? { ...item, channelType: "dm" } + : item; + + return { + ...feed, + feed: { + mentions: feed.feed.mentions.map(mark), + needsAction: feed.feed.needsAction.map(mark), + activity: feed.feed.activity.map(mark), + agentActivity: feed.feed.agentActivity.map(mark), + }, + }; +} diff --git a/desktop/src/features/channels/hiddenDmResurfaceAction.test.mjs b/desktop/src/features/channels/hiddenDmResurfaceAction.test.mjs new file mode 100644 index 0000000000..78d8f0da99 --- /dev/null +++ b/desktop/src/features/channels/hiddenDmResurfaceAction.test.mjs @@ -0,0 +1,121 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { resurfaceHiddenDmMessage } from "./hiddenDmResurfaceAction.ts"; + +const SELF = "1".repeat(64); +const ALICE = "2".repeat(64); +const BOB = "3".repeat(64); + +function event() { + return { + id: "event-1", + kind: 40002, + pubkey: ALICE, + content: "hello", + created_at: 10, + // Message p tags are intentionally incomplete for this group DM. + tags: [ + ["h", "hidden-dm"], + ["p", SELF], + ], + sig: "", + }; +} + +function member(pubkey) { + return { + pubkey, + role: "member", + isAgent: false, + joinedAt: "", + displayName: null, + }; +} + +test("reopens the source hidden group DM from authoritative membership", async () => { + const inputs = []; + assert.equal( + await resurfaceHiddenDmMessage({ + event: event(), + expectedRelayUrl: "wss://relay.example", + expectedSignerPubkey: SELF, + hiddenDmIds: new Set(["hidden-dm"]), + fetchMembers: async () => [member(SELF), member(ALICE), member(BOB)], + isCurrent: () => true, + reopen: async (input) => { + inputs.push(input); + return { id: "hidden-dm" }; + }, + }), + true, + ); + assert.deepEqual(inputs, [ + { + pubkeys: [ALICE, BOB], + expectedRelayUrl: "wss://relay.example", + expectedSignerPubkey: SELF, + }, + ]); +}); + +test("ignores an event for a channel outside the hidden set", async () => { + let reopenCount = 0; + assert.equal( + await resurfaceHiddenDmMessage({ + event: event(), + expectedRelayUrl: "wss://relay.example", + expectedSignerPubkey: SELF, + hiddenDmIds: new Set(["other-dm"]), + fetchMembers: async () => [member(SELF), member(ALICE)], + isCurrent: () => true, + reopen: async () => { + reopenCount += 1; + return { id: "hidden-dm" }; + }, + }), + false, + ); + assert.equal(reopenCount, 0); +}); + +test("a suspended old-community read cannot reopen a DM", async () => { + let current = true; + let resume; + const members = new Promise((resolve) => { + resume = resolve; + }); + let reopenCount = 0; + const result = resurfaceHiddenDmMessage({ + event: event(), + expectedRelayUrl: "wss://old.example", + expectedSignerPubkey: SELF, + hiddenDmIds: new Set(["hidden-dm"]), + fetchMembers: async () => members, + isCurrent: () => current, + reopen: async () => { + reopenCount += 1; + return { id: "hidden-dm" }; + }, + }); + await Promise.resolve(); + current = false; + resume([member(SELF), member(ALICE)]); + assert.equal(await result, false); + assert.equal(reopenCount, 0); +}); + +test("rejects a reopen result for any channel other than the source", async () => { + await assert.rejects( + resurfaceHiddenDmMessage({ + event: event(), + expectedRelayUrl: "wss://relay.example", + expectedSignerPubkey: SELF, + hiddenDmIds: new Set(["hidden-dm"]), + fetchMembers: async () => [member(SELF), member(ALICE)], + isCurrent: () => true, + reopen: async () => ({ id: "alternate-dm" }), + }), + /different DM conversation/, + ); +}); diff --git a/desktop/src/features/channels/hiddenDmResurfaceAction.ts b/desktop/src/features/channels/hiddenDmResurfaceAction.ts new file mode 100644 index 0000000000..78ee087475 --- /dev/null +++ b/desktop/src/features/channels/hiddenDmResurfaceAction.ts @@ -0,0 +1,49 @@ +import type { ChannelMember, RelayEvent } from "@/shared/api/types"; +import type { OpenDmInput } from "@/shared/api/tauriChannels"; +import { + dmPeerPubkeysFromMembers, + isIncomingChannelMessageFromOther, + relayEventChannelId, +} from "./dmResurface"; + +type HiddenDmResurfaceActionOptions = { + event: RelayEvent; + expectedRelayUrl: string; + expectedSignerPubkey: string; + hiddenDmIds: ReadonlySet; + fetchMembers: (channelId: string) => Promise; + isCurrent: () => boolean; + reopen: (input: OpenDmInput) => Promise<{ id: string }>; +}; + +export async function resurfaceHiddenDmMessage({ + event, + expectedRelayUrl, + expectedSignerPubkey, + hiddenDmIds, + fetchMembers, + isCurrent, + reopen, +}: HiddenDmResurfaceActionOptions): Promise { + if (!isIncomingChannelMessageFromOther(event, expectedSignerPubkey)) { + return false; + } + const channelId = relayEventChannelId(event); + if (!channelId || !hiddenDmIds.has(channelId)) return false; + + const members = await fetchMembers(channelId); + if (!isCurrent()) return false; + const pubkeys = dmPeerPubkeysFromMembers(members, expectedSignerPubkey); + if (pubkeys.length === 0) return false; + + const opened = await reopen({ + pubkeys, + expectedRelayUrl, + expectedSignerPubkey, + }); + if (!isCurrent()) return false; + if (opened.id !== channelId) { + throw new Error("Relay reopened a different DM conversation."); + } + return true; +} diff --git a/desktop/src/features/channels/hiddenDmResurfaceCoordinator.test.mjs b/desktop/src/features/channels/hiddenDmResurfaceCoordinator.test.mjs new file mode 100644 index 0000000000..b3ea41b4ce --- /dev/null +++ b/desktop/src/features/channels/hiddenDmResurfaceCoordinator.test.mjs @@ -0,0 +1,92 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { createHiddenDmResurfaceCoordinator } from "./hiddenDmResurfaceCoordinator.ts"; + +function deferred() { + let resolve; + let reject; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +test("a follower arriving mid-attempt retries from the latest event after a failure", async () => { + const seen = []; + const gates = [deferred(), deferred()]; + const errors = []; + const coordinator = createHiddenDmResurfaceCoordinator({ + resurface: async (event) => { + const index = seen.length; + seen.push(event.id); + await gates[index].promise; + }, + isCurrent: () => true, + onError: (channelId, error) => errors.push([channelId, error]), + }); + + coordinator.handle("dm-1", { id: "event-a" }); + await Promise.resolve(); + // Follower B lands while attempt A is still in flight. + coordinator.handle("dm-1", { id: "event-b" }); + assert.deepEqual(seen, ["event-a"]); + + // A fails; the retry re-runs from the latest event (B), which succeeds. + gates[0].reject(new Error("boom")); + await Promise.resolve(); + await Promise.resolve(); + assert.deepEqual(seen, ["event-a", "event-b"]); + + gates[1].resolve(); + await gates[1].promise; + await Promise.resolve(); + + assert.equal(errors.length, 1); + assert.equal(errors[0][0], "dm-1"); +}); + +test("a stale generation's attempt does not delete the live generation's entry", async () => { + // Model two generations: each real subscription generation creates its own + // coordinator, so a torn-down generation's cleanup touches only its own map. + let staleCurrent = true; + const staleGate = deferred(); + const staleSeen = []; + const stale = createHiddenDmResurfaceCoordinator({ + resurface: async (event) => { + staleSeen.push(event.id); + await staleGate.promise; + }, + isCurrent: () => staleCurrent, + }); + + const liveSeen = []; + const live = createHiddenDmResurfaceCoordinator({ + resurface: async (event) => { + liveSeen.push(event.id); + }, + isCurrent: () => true, + }); + + // Attempt A begins on the stale generation and suspends. + stale.handle("dm-1", { id: "event-a" }); + await Promise.resolve(); + assert.deepEqual(staleSeen, ["event-a"]); + + // Generation flips; event B is delivered to the live coordinator and reopens. + staleCurrent = false; + live.handle("dm-1", { id: "event-b" }); + await Promise.resolve(); + assert.deepEqual(liveSeen, ["event-b"]); + + // Stale A now retires. Its cleanup cannot touch the live coordinator's map, + // so a subsequent live follower still starts a fresh attempt. + staleGate.resolve(); + await staleGate.promise; + await Promise.resolve(); + + live.handle("dm-1", { id: "event-c" }); + await Promise.resolve(); + assert.deepEqual(liveSeen, ["event-b", "event-c"]); +}); diff --git a/desktop/src/features/channels/hiddenDmResurfaceCoordinator.ts b/desktop/src/features/channels/hiddenDmResurfaceCoordinator.ts new file mode 100644 index 0000000000..ec3e08150c --- /dev/null +++ b/desktop/src/features/channels/hiddenDmResurfaceCoordinator.ts @@ -0,0 +1,60 @@ +import type { RelayEvent } from "@/shared/api/types"; + +type CoordinatorOptions = { + resurface: (event: RelayEvent) => Promise; + isCurrent: () => boolean; + onError?: (channelId: string, error: unknown) => void; +}; + +/** + * Per-channel coalescing for hidden-DM resurface attempts. + * + * The reopen action is idempotent, so concurrent messages for the same DM + * share one in-flight attempt. A follower that lands while an attempt is + * running flags it for retry (from the latest event) instead of being + * dropped, so a failed reopen re-runs rather than leaving the row hidden. + * + * A coordinator owns its own pending map, so callers create one per + * subscription generation: an attempt from a torn-down generation can never + * delete or coalesce into an entry owned by the live one. + */ +export function createHiddenDmResurfaceCoordinator({ + resurface, + isCurrent, + onError, +}: CoordinatorOptions) { + const pending = new Map(); + const latestEventByChannel = new Map(); + + const attempt = async (channelId: string) => { + const state = { retry: false }; + pending.set(channelId, state); + try { + do { + state.retry = false; + const event = latestEventByChannel.get(channelId); + if (!event) return; + try { + await resurface(event); + return; + } catch (error) { + onError?.(channelId, error); + } + } while (state.retry && isCurrent()); + } finally { + pending.delete(channelId); + } + }; + + return { + handle(channelId: string, event: RelayEvent) { + latestEventByChannel.set(channelId, event); + const existing = pending.get(channelId); + if (existing) { + existing.retry = true; + return; + } + void attempt(channelId); + }, + }; +} diff --git a/desktop/src/features/channels/hooks.ts b/desktop/src/features/channels/hooks.ts index 9069b052da..18eb2699d2 100644 --- a/desktop/src/features/channels/hooks.ts +++ b/desktop/src/features/channels/hooks.ts @@ -53,6 +53,7 @@ import { CHANNEL_MEMBERS_STALE_TIME_MS, channelMembersQueryKey, } from "@/features/channels/rosterFreshness"; +import { dmVisibilityQueryKeyFor } from "@/features/channels/useHiddenDmIds"; export const channelsQueryKey = ["channels"] as const; /** Keeps focused polling at the established one-minute cadence. */ @@ -530,6 +531,12 @@ export function useCreateChannelMutation() { export function useOpenDmMutation() { const queryClient = useQueryClient(); + const { activeCommunity } = useCommunities(); + const identityQuery = useIdentityQuery(); + const dmVisibilityKey = dmVisibilityQueryKeyFor( + activeCommunity?.relayUrl, + identityQuery.data?.pubkey, + ); return useMutation({ mutationFn: (input: OpenDmInput) => openDm(input), @@ -537,6 +544,11 @@ export function useOpenDmMutation() { queryClient.setQueryData(channelsQueryKey, (current) => upsertCachedChannel(current, openedChannel), ); + queryClient.setQueryData>(dmVisibilityKey, (current) => { + const next = new Set(current); + next.delete(openedChannel.id); + return next; + }); }, onSettled: () => { // The relay-returned DM is already in the cache. Mark the list stale so @@ -546,6 +558,7 @@ export function useOpenDmMutation() { queryKey: channelsQueryKey, refetchType: "none", }); + void queryClient.invalidateQueries({ queryKey: dmVisibilityKey }); }, }); } @@ -575,6 +588,12 @@ export function useUpsertCachedChannel() { export function useHideDmMutation() { const queryClient = useQueryClient(); + const { activeCommunity } = useCommunities(); + const identityQuery = useIdentityQuery(); + const dmVisibilityKey = dmVisibilityQueryKeyFor( + activeCommunity?.relayUrl, + identityQuery.data?.pubkey, + ); return useMutation({ mutationFn: (channelId: string) => hideDm(channelId), @@ -591,8 +610,16 @@ export function useHideDmMutation() { queryClient.setQueryData(channelsQueryKey, context.previous); } }, + onSuccess: (_data, channelId) => { + queryClient.setQueryData>(dmVisibilityKey, (current) => + new Set(current).add(channelId), + ); + }, onSettled: async () => { - await queryClient.invalidateQueries({ queryKey: channelsQueryKey }); + await Promise.all([ + queryClient.invalidateQueries({ queryKey: channelsQueryKey }), + queryClient.invalidateQueries({ queryKey: dmVisibilityKey }), + ]); }, }); } diff --git a/desktop/src/features/channels/useDmResurfaceBatching.test.mjs b/desktop/src/features/channels/useDmResurfaceBatching.test.mjs new file mode 100644 index 0000000000..da426d8faf --- /dev/null +++ b/desktop/src/features/channels/useDmResurfaceBatching.test.mjs @@ -0,0 +1,278 @@ +/** + * Batching lifecycle for useDmResurfaceFromMessages. + * + * The relay rejects a REQ whose aggregate explicit `#h` values exceed + * MAX_EXPLICIT_CHANNEL_VALUES, so a hidden set larger than the cap must be + * split into multiple subscriptions or every hidden DM loses its resurface + * trigger. These tests exercise exactly that split and its teardown, which a + * pure helper test cannot reach because the batching only happens inside the + * mounted effect. + */ + +import assert from "node:assert/strict"; +import { after, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +before(() => { + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + localStorage: dom.window.localStorage, + }); + globalThis.__TAURI_INTERNALS__ = { + invoke: (cmd) => { + if (cmd === "get_channel_members") { + return Promise.resolve({ + members: [ + { pubkey: VIEWER, role: "member", is_agent: false }, + { pubkey: "b".repeat(64), role: "member", is_agent: false }, + ], + }); + } + return Promise.reject(new Error(`unmocked Tauri command: ${cmd}`)); + }, + transformCallback: () => Math.random(), + }; + dom.window.__TAURI_INTERNALS__ = globalThis.__TAURI_INTERNALS__; +}); + +after(() => dom.window.close()); + +const RELAY_URL = "wss://relay.example"; +const VIEWER = "a".repeat(64); + +function seedCommunity() { + window.localStorage.setItem( + "buzz-communities", + JSON.stringify([ + { + id: "community-a", + name: "Community A", + relayUrl: RELAY_URL, + addedAt: "2026-01-01T00:00:00Z", + }, + ]), + ); + window.localStorage.setItem("buzz-active-community-id", "community-a"); +} + +function hiddenDmSnapshot(count) { + return { + id: "snapshot-1", + kind: 30622, + pubkey: VIEWER, + content: "", + created_at: 1, + tags: Array.from({ length: count }, (_, index) => [ + "h", + `dm-${String(index).padStart(4, "0")}`, + ]), + sig: "", + }; +} + +async function mount(hiddenCount, subscribeImpl, reopen) { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const React = (await import("react")).default; + const { QueryClient, QueryClientProvider } = await import( + "@tanstack/react-query" + ); + const { CommunitiesProvider } = await import( + "@/features/communities/useCommunities.tsx" + ); + const { relayClient } = await import("@/shared/api/relayClient"); + const { useDmResurfaceFromMessages } = await import( + "./useDmResurfaceFromMessages.ts" + ); + + const originalFetchEvents = relayClient.fetchEvents; + const originalSubscribeLive = relayClient.subscribeLive; + + relayClient.fetchEvents = async () => + hiddenCount > 0 ? [hiddenDmSnapshot(hiddenCount)] : []; + relayClient.subscribeLive = subscribeImpl; + + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false, gcTime: 0 } }, + }); + + const wrapper = ({ children }) => + React.createElement( + QueryClientProvider, + { client: queryClient }, + React.createElement(CommunitiesProvider, null, children), + ); + + const hook = renderHook( + () => + useDmResurfaceFromMessages({ + pubkey: VIEWER, + relayUrl: RELAY_URL, + reopen: reopen ?? (async () => ({ id: "x" })), + }), + { wrapper }, + ); + + return { + act, + async settle() { + for (let i = 0; i < 6; i++) { + await act(async () => { + await new Promise((r) => setTimeout(r, 5)); + }); + } + }, + unmount: hook.unmount, + restore() { + hook.unmount(); + queryClient.clear(); + queryClient.unmount(); + cleanup(); + relayClient.fetchEvents = originalFetchEvents; + relayClient.subscribeLive = originalSubscribeLive; + }, + }; +} + +test("128 hidden DMs use a single subscription within the cap", async () => { + seedCommunity(); + const batches = []; + const harness = await mount(128, async (filter) => { + batches.push(filter["#h"]); + return async () => {}; + }); + try { + await harness.settle(); + assert.equal(batches.length, 1); + assert.equal(batches[0].length, 128); + } finally { + harness.restore(); + } +}); + +test("129 hidden DMs split into two subscriptions, both within the cap", async () => { + seedCommunity(); + const batches = []; + const harness = await mount(129, async (filter) => { + batches.push(filter["#h"]); + return async () => {}; + }); + try { + await harness.settle(); + assert.equal(batches.length, 2); + assert.deepEqual( + batches.map((batch) => batch.length), + [128, 1], + ); + // Every hidden id appears exactly once across the batches. + const all = batches.flat(); + assert.equal(new Set(all).size, 129); + for (const batch of batches) { + assert.ok(batch.length <= 128); + } + } finally { + harness.restore(); + } +}); + +test("activity delivered on the final batch resurfaces its DM", async () => { + seedCommunity(); + const handlers = []; + const reopened = []; + const targetIdRef = { id: null }; + const harness = await mount( + 129, + async (filter, onEvent) => { + handlers.push({ ids: filter["#h"], onEvent }); + return async () => {}; + }, + async ({ pubkeys }) => { + reopened.push(pubkeys); + // The reopen contract returns the resurfaced channel id; the action + // rejects a mismatch, so echo the target the event carried. + return { id: targetIdRef.id }; + }, + ); + try { + await harness.settle(); + const finalBatch = handlers[1]; + const targetId = finalBatch.ids[0]; + targetIdRef.id = targetId; + // Deliver a peer message on the FINAL batch's subscription. If batching + // dropped that batch's handler, this event would never reach the + // coordinator and reopen would never fire. + await harness.act(async () => { + finalBatch.onEvent({ + id: "evt-1", + kind: 9, + pubkey: "b".repeat(64), + content: "hi", + created_at: 2, + tags: [["h", targetId]], + sig: "", + }); + await new Promise((r) => setTimeout(r, 5)); + }); + assert.equal(reopened.length, 1); + assert.deepEqual(reopened[0], ["b".repeat(64)]); + } finally { + harness.restore(); + } +}); + +test("a batch subscription failure does not abort the other batches", async () => { + seedCommunity(); + const batches = []; + let call = 0; + const harness = await mount(129, async (filter) => { + call += 1; + if (call === 1) throw new Error("relay rejected batch 1"); + batches.push(filter["#h"]); + return async () => {}; + }); + try { + await harness.settle(); + // The second batch still subscribed even though the first threw. + assert.equal(batches.length, 1); + assert.equal(batches[0].length, 1); + } finally { + harness.restore(); + } +}); + +test("teardown while batch setup is pending disposes every settled batch", async () => { + seedCommunity(); + let disposeCount = 0; + const releases = []; + const harness = await mount(129, async () => { + // Each subscribe parks until the test releases it, so the effect can be + // torn down while batch setup is still in flight. + await new Promise((resolve) => releases.push(resolve)); + return async () => { + disposeCount += 1; + }; + }); + try { + await harness.settle(); + // Unmount before any subscribe resolves. + await harness.act(async () => { + harness.unmount(); + }); + // Now let both subscribes resolve; each must be disposed since its owning + // generation is gone. + await harness.act(async () => { + for (const release of releases) release(); + await new Promise((r) => setTimeout(r, 5)); + }); + assert.equal(disposeCount, 2); + } finally { + harness.restore(); + } +}); diff --git a/desktop/src/features/channels/useDmResurfaceFromMessages.ts b/desktop/src/features/channels/useDmResurfaceFromMessages.ts new file mode 100644 index 0000000000..fddd056f25 --- /dev/null +++ b/desktop/src/features/channels/useDmResurfaceFromMessages.ts @@ -0,0 +1,141 @@ +import * as React from "react"; + +import { relayClient } from "@/shared/api/relayClient"; +import type { RelayEvent } from "@/shared/api/types"; +import { MAX_EXPLICIT_CHANNEL_VALUES } from "@/shared/api/relayClientShared"; +import { + getChannelMembers, + type OpenDmInput, +} from "@/shared/api/tauriChannels"; +import { CHANNEL_MESSAGE_EVENT_KINDS } from "@/shared/constants/kinds"; +import { relayEventChannelId } from "./dmResurface"; +import { createHiddenDmResurfaceCoordinator } from "./hiddenDmResurfaceCoordinator"; +import { resurfaceHiddenDmMessage } from "./hiddenDmResurfaceAction"; +import { useHiddenDmIds } from "./useHiddenDmIds"; + +type UseDmResurfaceFromMessagesOptions = { + pubkey: string | undefined; + relayUrl: string | undefined; + reopen: (input: OpenDmInput) => Promise<{ id: string }>; +}; + +/** + * Resurfaces a hidden DM row the moment new activity lands in it. + * + * The subscription is `#h`-scoped to the current hidden-DM id set: channel + * messages carry a `channel_id`, and the relay only fans channel-scoped events + * to channel-scoped subscriptions (`fan_out_scoped`), so a community-global + * `#p` filter would never receive them. Scoping to `#h` also means every + * delivered event is already for a hidden DM the reader belongs to (hiding + * never drops membership), so no per-event visibility fetch is needed and + * untagged CLI/agent DMs resurface too. + * + * The relay rejects a REQ whose aggregate explicit `#h` values exceed + * {@link MAX_EXPLICIT_CHANNEL_VALUES}, so the hidden set is split into batches + * of at most that size, each its own subscription. Every batch is owned by the + * current effect generation and disposed together on hidden-set/relay/signer + * change, so an over-limit hidden set no longer silently disables resurfacing. + */ +export function useDmResurfaceFromMessages({ + pubkey, + relayUrl, + reopen, +}: UseDmResurfaceFromMessagesOptions) { + const hiddenDmIds = useHiddenDmIds(pubkey); + const generationRef = React.useRef(0); + const reopenLatest = React.useEffectEvent(reopen); + + // Stable dependency for the hidden-set membership, order-independent. + const hiddenDmKey = React.useMemo( + () => [...hiddenDmIds].sort().join(","), + [hiddenDmIds], + ); + + React.useEffect(() => { + const expectedSignerPubkey = pubkey?.trim().toLowerCase() ?? ""; + const expectedRelayUrl = relayUrl?.trim() ?? ""; + const channelIds = hiddenDmKey.length > 0 ? hiddenDmKey.split(",") : []; + const generation = ++generationRef.current; + if (!expectedSignerPubkey || !expectedRelayUrl || channelIds.length === 0) { + return; + } + + const hiddenDmIdSet = new Set(channelIds); + let disposed = false; + const unsubscribers: Array<() => Promise> = []; + const isCurrent = () => !disposed && generationRef.current === generation; + + // A coordinator owned by this generation: coalescing and cleanup touch only + // its private map, so a torn-down generation's in-flight attempt can never + // drop a follower coalesced onto the replacement subscription. One + // coordinator spans every batch — the map is keyed by channel id, so which + // batch delivered an event is irrelevant. + const coordinator = createHiddenDmResurfaceCoordinator({ + resurface: (event) => + resurfaceHiddenDmMessage({ + event, + expectedRelayUrl, + expectedSignerPubkey, + hiddenDmIds: hiddenDmIdSet, + fetchMembers: getChannelMembers, + isCurrent, + reopen: reopenLatest, + }), + isCurrent, + onError: (channelId, error) => { + if (isCurrent()) { + console.error("Failed to resurface hidden DM", channelId, error); + } + }, + }); + + const handleEvent = (event: RelayEvent) => { + if (!isCurrent()) return; + const channelId = relayEventChannelId(event); + if (!channelId || !hiddenDmIdSet.has(channelId)) return; + coordinator.handle(channelId, event); + }; + + const since = Math.floor(Date.now() / 1_000) - 5; + for ( + let start = 0; + start < channelIds.length; + start += MAX_EXPLICIT_CHANNEL_VALUES + ) { + const batch = channelIds.slice( + start, + start + MAX_EXPLICIT_CHANNEL_VALUES, + ); + void relayClient + .subscribeLive( + { + kinds: [...CHANNEL_MESSAGE_EVENT_KINDS], + "#h": batch, + since, + limit: 100, + }, + handleEvent, + ) + .then((dispose) => { + if (!isCurrent()) { + void dispose().catch(() => {}); + return; + } + unsubscribers.push(dispose); + }) + .catch((error) => { + if (isCurrent()) { + console.error("Failed to subscribe to hidden DM activity", error); + } + }); + } + + return () => { + disposed = true; + generationRef.current += 1; + for (const unsubscribe of unsubscribers) { + void unsubscribe().catch(() => {}); + } + }; + }, [pubkey, relayUrl, hiddenDmKey]); +} diff --git a/desktop/src/features/channels/useHiddenDmIds.ts b/desktop/src/features/channels/useHiddenDmIds.ts new file mode 100644 index 0000000000..865d434e45 --- /dev/null +++ b/desktop/src/features/channels/useHiddenDmIds.ts @@ -0,0 +1,62 @@ +import * as React from "react"; +import { useQuery } from "@tanstack/react-query"; + +import { useCommunities } from "@/features/communities/useCommunities"; +import { relayClient } from "@/shared/api/relayClient"; +import type { RelayEvent } from "@/shared/api/types"; +import { KIND_DM_VISIBILITY } from "@/shared/constants/kinds"; +import { normalizePubkey } from "@/shared/lib/pubkey"; + +export const dmVisibilityQueryKey = ["dm-visibility"] as const; + +/** Exact query key for one relay+identity scope's DM-visibility snapshot. */ +export function dmVisibilityQueryKeyFor( + relayUrl: string | undefined, + pubkey: string | undefined, +) { + return [ + ...dmVisibilityQueryKey, + relayUrl ?? "", + normalizePubkey(pubkey ?? ""), + ] as const; +} + +export function extractHiddenDmIds(events: readonly RelayEvent[]): Set { + const latest = events.reduce( + (current, event) => + current === null || event.created_at > current.created_at + ? event + : current, + null, + ); + return new Set( + (latest?.tags ?? []) + .filter((tag) => tag[0] === "h" && tag[1]) + .map((tag) => tag[1]), + ); +} + +export async function fetchHiddenDmIds(pubkey: string): Promise> { + const normalizedPubkey = normalizePubkey(pubkey); + if (normalizedPubkey.length === 0) return new Set(); + const events = await relayClient.fetchEvents({ + kinds: [KIND_DM_VISIBILITY], + "#p": [normalizedPubkey], + limit: 1, + }); + return extractHiddenDmIds(events); +} + +export function useHiddenDmIds(pubkey: string | undefined) { + const { activeCommunity } = useCommunities(); + const normalizedPubkey = normalizePubkey(pubkey ?? ""); + const relayUrl = activeCommunity?.relayUrl ?? ""; + const query = useQuery({ + queryKey: dmVisibilityQueryKeyFor(relayUrl, normalizedPubkey), + queryFn: () => fetchHiddenDmIds(normalizedPubkey), + enabled: relayUrl.length > 0 && normalizedPubkey.length > 0, + staleTime: 30_000, + }); + + return React.useMemo(() => query.data ?? new Set(), [query.data]); +} diff --git a/desktop/src/features/home/hiddenDmInboxAction.test.mjs b/desktop/src/features/home/hiddenDmInboxAction.test.mjs new file mode 100644 index 0000000000..0e98a9a4cb --- /dev/null +++ b/desktop/src/features/home/hiddenDmInboxAction.test.mjs @@ -0,0 +1,141 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { openHiddenDmInboxContext } from "./hiddenDmInboxAction.ts"; + +const SELF = "1".repeat(64); +const ALICE = "2".repeat(64); +const BOB = "3".repeat(64); +const inboxItem = { + id: "event-1", + item: { + channelType: "dm", + // Incomplete message tags must not choose the recreated membership. + tags: [ + ["h", "hidden-dm"], + ["p", SELF], + ], + }, +}; + +function member(pubkey) { + return { + pubkey, + role: "member", + isAgent: false, + joinedAt: "", + displayName: null, + }; +} + +function options(overrides = {}) { + return { + item: inboxItem, + channelId: "hidden-dm", + messageId: "event-1", + availableChannelIds: new Set(), + expectedRelayUrl: "wss://relay.example", + expectedSignerPubkey: SELF, + pendingChannelIds: new Set(), + fetchMembers: async () => [member(SELF), member(ALICE), member(BOB)], + openDm: async () => ({ id: "hidden-dm" }), + isCurrent: () => true, + onOpenContext: () => {}, + onError: () => {}, + onPendingChange: () => {}, + ...overrides, + }; +} + +test("Inbox reopens the original hidden group DM from channel membership", async () => { + const inputs = []; + const navigations = []; + const result = await openHiddenDmInboxContext( + options({ + openDm: async (input) => { + inputs.push(input); + return { id: "hidden-dm" }; + }, + onOpenContext: (...args) => navigations.push(args), + }), + ); + assert.equal(result, true); + assert.deepEqual(inputs[0].pubkeys, [ALICE, BOB]); + assert.deepEqual(navigations, [["hidden-dm", "event-1", undefined]]); +}); + +test("double activation is deduplicated while a reopen is pending", async () => { + let resume; + const members = new Promise((resolve) => { + resume = resolve; + }); + let openCount = 0; + const shared = options({ + fetchMembers: async () => members, + openDm: async () => { + openCount += 1; + return { id: "hidden-dm" }; + }, + }); + const first = openHiddenDmInboxContext(shared); + const second = openHiddenDmInboxContext(shared); + resume([member(SELF), member(ALICE)]); + assert.equal(await second, false); + assert.equal(await first, true); + assert.equal(openCount, 1); +}); + +test("a failed reopen stays put, reports an error, and can be retried", async () => { + let attempts = 0; + let errors = 0; + let navigations = 0; + const shared = options({ + openDm: async () => { + attempts += 1; + if (attempts === 1) throw new Error("offline"); + return { id: "hidden-dm" }; + }, + onError: () => { + errors += 1; + }, + onOpenContext: () => { + navigations += 1; + }, + }); + assert.equal(await openHiddenDmInboxContext(shared), false); + assert.equal(navigations, 0); + assert.equal(errors, 1); + assert.equal(shared.pendingChannelIds.size, 0); + + assert.equal(await openHiddenDmInboxContext(shared), true); + assert.equal(attempts, 2); + assert.equal(navigations, 1); +}); + +test("an unmounted Inbox action cannot navigate after reopen settles", async () => { + let current = true; + let resume; + const reopened = new Promise((resolve) => { + resume = resolve; + }); + let navigations = 0; + let pendingChanges = 0; + const result = openHiddenDmInboxContext( + options({ + openDm: async () => reopened, + isCurrent: () => current, + onOpenContext: () => { + navigations += 1; + }, + onPendingChange: () => { + pendingChanges += 1; + }, + }), + ); + await Promise.resolve(); + current = false; + resume({ id: "hidden-dm" }); + assert.equal(await result, false); + assert.equal(navigations, 0); + assert.equal(pendingChanges, 1); +}); diff --git a/desktop/src/features/home/hiddenDmInboxAction.ts b/desktop/src/features/home/hiddenDmInboxAction.ts new file mode 100644 index 0000000000..60b2ef15d2 --- /dev/null +++ b/desktop/src/features/home/hiddenDmInboxAction.ts @@ -0,0 +1,76 @@ +import { dmPeerPubkeysFromMembers } from "@/features/channels/dmResurface"; +import type { InboxItem } from "@/features/home/lib/inbox"; +import type { ChannelMember } from "@/shared/api/types"; +import type { OpenDmInput } from "@/shared/api/tauriChannels"; + +type HiddenDmInboxActionOptions = { + item: InboxItem; + channelId: string; + messageId: string; + threadRootId?: string | null; + availableChannelIds: ReadonlySet; + expectedRelayUrl: string; + expectedSignerPubkey: string; + pendingChannelIds: Set; + fetchMembers: (channelId: string) => Promise; + openDm: (input: OpenDmInput) => Promise<{ id: string }>; + isCurrent: () => boolean; + onOpenContext: ( + channelId: string, + messageId: string, + threadRootId?: string | null, + ) => void; + onError: () => void; + onPendingChange: () => void; +}; + +export async function openHiddenDmInboxContext({ + item, + channelId, + messageId, + threadRootId, + availableChannelIds, + expectedRelayUrl, + expectedSignerPubkey, + pendingChannelIds, + fetchMembers, + openDm, + isCurrent, + onOpenContext, + onError, + onPendingChange, +}: HiddenDmInboxActionOptions): Promise { + if (availableChannelIds.has(channelId) || item.item.channelType !== "dm") { + if (isCurrent()) onOpenContext(channelId, messageId, threadRootId); + return true; + } + if (pendingChannelIds.has(channelId)) return false; + + pendingChannelIds.add(channelId); + onPendingChange(); + try { + const members = await fetchMembers(channelId); + if (!isCurrent()) return false; + const pubkeys = dmPeerPubkeysFromMembers(members, expectedSignerPubkey); + if (pubkeys.length === 0) { + throw new Error("Could not determine the DM membership."); + } + const opened = await openDm({ + pubkeys, + expectedRelayUrl, + expectedSignerPubkey, + }); + if (!isCurrent()) return false; + if (opened.id !== channelId) { + throw new Error("Relay reopened a different DM conversation."); + } + onOpenContext(channelId, messageId, threadRootId); + return true; + } catch { + if (isCurrent()) onError(); + return false; + } finally { + pendingChannelIds.delete(channelId); + if (isCurrent()) onPendingChange(); + } +} diff --git a/desktop/src/features/home/ui/HomeScreen.tsx b/desktop/src/features/home/ui/HomeScreen.tsx index b6512816e3..3dcfbbc383 100644 --- a/desktop/src/features/home/ui/HomeScreen.tsx +++ b/desktop/src/features/home/ui/HomeScreen.tsx @@ -1,6 +1,8 @@ import * as React from "react"; import { useAppShell } from "@/app/AppShellContext"; +import { markHiddenDmFeedItems } from "@/features/channels/dmResurface"; +import { useHiddenDmIds } from "@/features/channels/useHiddenDmIds"; import { useHomeFeedQuery } from "@/features/home/hooks"; import { HomeView } from "@/features/home/ui/HomeView"; import type { HomeFeedResponse } from "@/shared/api/types"; @@ -26,24 +28,25 @@ export function HomeScreen({ }: HomeScreenProps) { const homeFeedQuery = useHomeFeedQuery(); const { threadActivityFeedItems } = useAppShell(); + const hiddenDmIds = useHiddenDmIds(currentPubkey); const augmentedFeed = React.useMemo((): HomeFeedResponse | undefined => { if (!homeFeedQuery.data) return undefined; - if (threadActivityFeedItems.length === 0) { - return homeFeedQuery.data; - } - - return { - ...homeFeedQuery.data, - feed: { - ...homeFeedQuery.data.feed, - activity: [ - ...homeFeedQuery.data.feed.activity, - ...threadActivityFeedItems, - ], - }, - }; - }, [homeFeedQuery.data, threadActivityFeedItems]); + const withThreadActivity = + threadActivityFeedItems.length === 0 + ? homeFeedQuery.data + : { + ...homeFeedQuery.data, + feed: { + ...homeFeedQuery.data.feed, + activity: [ + ...homeFeedQuery.data.feed.activity, + ...threadActivityFeedItems, + ], + }, + }; + return markHiddenDmFeedItems(withThreadActivity, hiddenDmIds); + }, [hiddenDmIds, homeFeedQuery.data, threadActivityFeedItems]); return (
diff --git a/desktop/src/features/home/ui/HomeView.tsx b/desktop/src/features/home/ui/HomeView.tsx index 893b3c309c..0a16f27c4d 100644 --- a/desktop/src/features/home/ui/HomeView.tsx +++ b/desktop/src/features/home/ui/HomeView.tsx @@ -2,9 +2,8 @@ import * as React from "react"; import { RefreshCcw } from "lucide-react"; import { useAppShell } from "@/app/AppShellContext"; -import { useAppNavigation } from "@/app/navigation/useAppNavigation"; import { useKnownAgentPubkeys } from "@/features/agents/useKnownAgentPubkeys"; -import { useChannelsQuery, useOpenDmMutation } from "@/features/channels/hooks"; +import { useChannelsQuery } from "@/features/channels/hooks"; import { RightAuxiliaryPane } from "@/features/channels/ui/RightAuxiliaryPane"; import { ChannelManagementSheet } from "@/features/channels/ui/ChannelManagementSheet"; import { @@ -28,6 +27,7 @@ import { useHomeInboxAutoSelection } from "@/features/home/useHomeInboxAutoSelec import { useHomeInboxContextMessages } from "@/features/home/useHomeInboxContextMessages"; import { useHomePersonalInbox } from "@/features/home/useHomePersonalInbox"; import { useInboxThreadContext } from "@/features/home/useInboxThreadContext"; +import { useHiddenDmInboxNavigation } from "@/features/home/useHiddenDmInboxNavigation"; import { type ProfilePanelTab, type ProfilePanelView, @@ -171,9 +171,6 @@ export function HomeView({ const [membersChannel, setMembersChannel] = React.useState( null, ); - const { goChannel } = useAppNavigation(); - const openDmMutation = useOpenDmMutation(); - const openDm = openDmMutation.mutateAsync; const handleUserSelectItem = React.useCallback( (itemId: string | null) => { setAutoSelectedEventId(null); @@ -219,13 +216,6 @@ export function HomeView({ const [emptyDeleteId, setEmptyDeleteId] = React.useState(null); const [editTargetId, setEditTargetId] = React.useState(null); const [isSendingReply, setIsSendingReply] = React.useState(false); - const handleOpenDm = React.useCallback( - async (pubkeys: string[]) => { - const dm = await openDm({ pubkeys }); - await goChannel(dm.id); - }, - [goChannel, openDm], - ); const { activeReminderEventIds, openReminder } = useRemindLater(); const [localRepliesByItemId, setLocalRepliesByItemId] = React.useState< Record @@ -460,6 +450,19 @@ export function HomeView({ } return null; }, [filteredItems, selectedConversationId, selectedEventId]); + const { + canOpenSelected, + handleOpenDirect, + handleOpenDm, + handleOpenSelectedContext, + isReopenPending, + isReopenErrored, + } = useHiddenDmInboxNavigation({ + availableChannelIds, + currentPubkey, + onOpenContext, + selectedItem, + }); const deleteInboxMessage = React.useCallback( async (eventId: string) => { const channelId = selectedItem?.item.channelId; @@ -700,17 +703,9 @@ export function HomeView({ onFilterChange={handleFilterChange} onMarkRead={markItemRead} onMarkUnread={markItemUnread} - onOpenDirect={(item) => { - const channelId = item.item.channelId; - if (!channelId) { - return; - } - onOpenContext( - channelId, - item.id, - getThreadReference(item.item.tags).rootId, - ); - }} + onOpenDirect={handleOpenDirect} + isReopenPending={isReopenPending} + isReopenErrored={isReopenErrored} onRemindLater={(item) => { const channelId = item.item.channelId; if (!channelId) { @@ -788,10 +783,7 @@ export function HomeView({ void; + /** True while the selected hidden DM is being reopened on the relay. */ + reopenPending?: boolean; + /** True when the last reopen of the selected hidden DM failed. */ + reopenErrored?: boolean; onSendReply: (input: { content: string; mediaTags?: string[][]; @@ -189,6 +193,8 @@ function InboxMessageDetailPane({ onRequestEmptyEditDelete, onManageChannel, onOpenContext, + reopenPending = false, + reopenErrored = false, onSendReply, onToggleReaction, }: InboxDetailPaneProps) { @@ -589,6 +595,47 @@ function InboxMessageDetailPane({
+ {reopenPending || reopenErrored ? ( +
+ {reopenPending ? ( + <> + + Reopening… + + ) : ( + <> + + Couldn’t reopen + {contextChannelId ? ( + + ) : null} + + )} +
+ ) : null} {canOpenChannel && contextChannelId ? ( diff --git a/desktop/src/features/home/ui/InboxListPane.tsx b/desktop/src/features/home/ui/InboxListPane.tsx index f2d4421081..94ec11db59 100644 --- a/desktop/src/features/home/ui/InboxListPane.tsx +++ b/desktop/src/features/home/ui/InboxListPane.tsx @@ -1,4 +1,12 @@ -import { Bell, Clock, Ellipsis, ExternalLink, MailOpen } from "lucide-react"; +import { + AlertCircle, + Bell, + Clock, + Ellipsis, + ExternalLink, + LoaderCircle, + MailOpen, +} from "lucide-react"; import * as React from "react"; import { @@ -215,6 +223,8 @@ type InboxListPaneProps = { onMarkRead: (itemId: string) => void; onMarkUnread: (itemId: string) => void; onOpenDirect: (item: InboxItem) => void; + isReopenPending?: (channelId: string | null | undefined) => boolean; + isReopenErrored?: (channelId: string | null | undefined) => boolean; onRemindLater: (item: InboxItem) => void; onSelect: (itemId: string) => void; onSelectDraft: (draftKey: string) => void; @@ -243,6 +253,8 @@ export function InboxListPane({ onMarkRead, onMarkUnread, onOpenDirect, + isReopenPending, + isReopenErrored, onRemindLater, onSelect, onSelectDraft, @@ -303,6 +315,14 @@ export function InboxListPane({ (eventId) => activeReminderEventIds?.has(eventId) ?? false, ); const hasChannelTarget = Boolean(item.item.channelId); + const isReopening = isReopenPending?.(item.item.channelId) ?? false; + const hasReopenError = isReopenErrored?.(item.item.channelId) ?? false; + const canOpen = hasChannelTarget && !isReopening; + const openLabel = !hasChannelTarget + ? "No channel link" + : isReopening + ? "Reopening…" + : "Open in channel"; const typeLabel = getInboxTypeLabel(item); const videoReviewCommentRootId = getInboxVideoReviewCommentRootId(item); const isSenderAgent = @@ -431,6 +451,47 @@ export function InboxListPane({
) : null} + {isReopening || hasReopenError ? ( +
+ {isReopening ? ( + <> + + Reopening… + + ) : ( + <> + + Couldn’t reopen + {canOpen ? ( + + ) : null} + + )} +
+ ) : null} +
)} onOpenDirect(item)} > @@ -509,15 +570,15 @@ export function InboxListPane({ )} { - if (hasChannelTarget) { + if (canOpen) { onOpenDirect(item); } }} > - {hasChannelTarget ? "Open in channel" : "No channel link"} + {openLabel} null;\n", + }; + } + if (url === "buzz-inbox-stub:UpdateIndicator") { + // The real UpdateIndicator pulls in UpdaterProvider's background-check + // setInterval, which keeps the event loop alive past the test. It has + // nothing to do with the reopen contract, so stub it to a null render. + return { + format: "module", + shortCircuit: true, + source: "export const UpdateIndicator = () => null;\n", + }; + } + return nextLoad(url, context); + }, +}); + +const SELF = "1".repeat(64); +const PEER = "2".repeat(64); +const HIDDEN_DM_ID = "hidden-dm-channel"; +const SOURCE_EVENT_ID = "e".repeat(64); +const RELAY_URL = "wss://relay.example"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +class NoopObserver { + disconnect() {} + observe() {} + unobserve() {} +} + +// Neither pane nor the reopen path needs a live relay socket; a real +// (undici) WebSocket would open a connection to the seeded relay URL and +// leak an open handle that keeps the test process alive. Stub it to a +// non-connecting shell. +class NoopWebSocket { + close() {} + send() {} + addEventListener() {} + removeEventListener() {} +} +globalThis.WebSocket = NoopWebSocket; +dom.window.WebSocket = NoopWebSocket; + +Object.assign(globalThis, { + IS_REACT_ACT_ENVIRONMENT: true, + IntersectionObserver: NoopObserver, + MutationObserver: dom.window.MutationObserver, + ResizeObserver: NoopObserver, + document: dom.window.document, + localStorage: dom.window.localStorage, + self: dom.window, + window: dom.window, +}); +// Bulk-copy DOM constructors Radix / React reference without a window prefix. +for (const key of Object.getOwnPropertyNames(dom.window)) { + if ( + !(key in globalThis) && + (key.startsWith("HTML") || + key.startsWith("SVG") || + [ + "Element", + "DOMRect", + "DOMRectReadOnly", + "Node", + "NodeFilter", + "NodeList", + "NamedNodeMap", + "Event", + "CustomEvent", + "MouseEvent", + "KeyboardEvent", + "FocusEvent", + "InputEvent", + "PointerEvent", + "Text", + "Comment", + "DocumentFragment", + "Range", + "Selection", + ].includes(key)) + ) { + const value = dom.window[key]; + if (value !== undefined) globalThis[key] = value; + } +} +Object.defineProperty(globalThis, "navigator", { + configurable: true, + value: dom.window.navigator, + writable: true, +}); +globalThis.getComputedStyle = dom.window.getComputedStyle.bind(dom.window); +dom.window.matchMedia = () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, +}); +globalThis.matchMedia = dom.window.matchMedia; +dom.window.requestAnimationFrame = (callback) => setTimeout(callback, 0); +dom.window.cancelAnimationFrame = (id) => clearTimeout(id); +globalThis.requestAnimationFrame = dom.window.requestAnimationFrame; +globalThis.cancelAnimationFrame = dom.window.cancelAnimationFrame; + +// Radix DismissableLayer/FocusScope dispatch plain objects; JSDOM's strict +// Event validation throws on them. Drop non-Event objects silently. +const _origDispatch = dom.window.EventTarget.prototype.dispatchEvent; +dom.window.EventTarget.prototype.dispatchEvent = function dispatchEvent(event) { + if (!(event instanceof dom.window.Event)) return false; + return _origDispatch.call(this, event); +}; +globalThis.EventTarget = dom.window.EventTarget; + +// JSDOM does not perform a native
- +
Received @@ -352,7 +358,7 @@ function FeedbackList() { className="record-open-link" > - Open feedback from {item.communityHost} + Open feedback from {hostLabel(item.communityHost)} @@ -372,13 +378,101 @@ function FeedbackList() { ); } +const STATUS_LABELS: Record = { + new: "New", + reviewed: "Reviewed", + archived: "Archived", +}; + +const STATUS_OPTIONS: FeedbackStatus[] = ["new", "reviewed", "archived"]; + +/// The per-row lifecycle control. In writable mode it PATCHes the relay and +/// adopts the status from the PATCH response (never optimistically — a failed +/// write leaves the prior status and surfaces an error). In read-only mode it +/// shows the authoritative status as a static badge, since disabled-mode +/// mutations are rejected server-side. +function FeedbackStatusControl({ + id, + status, + authMode, + canWrite, + onApplied, +}: { + id: string; + status: FeedbackStatus; + authMode: AuthMode; + canWrite: boolean; + onApplied: (id: string, status: FeedbackStatus) => void; +}) { + const [pending, setPending] = useState(false); + const [error, setError] = useState(false); + + if (!canWrite) { + return ( + + + {STATUS_LABELS[status]} + + + ); + } + + const onChange = async (event: ChangeEvent) => { + const next = event.target.value as FeedbackStatus; + setPending(true); + setError(false); + try { + const updated = await mutate<{ status: FeedbackStatus }>( + `/feedback/${encodeURIComponent(id)}`, + "PATCH", + { status: next }, + authMode, + ); + onApplied(id, updated.status); + } catch { + setError(true); + } finally { + setPending(false); + } + }; + + return ( + + ); +} + +/// Feedback whose source community was purged carries no host. Render an +/// explicit provenance-unavailable marker rather than an empty slot. +function Provenance({ host }: { host: string | null }) { + if (host) return host; + return Community unavailable; +} + +function hostLabel(host: string | null): string { + return host ?? "an unavailable community"; +} + function FeedbackResults({ items, query, community, timeRange, statusFilter, - statuses, + overrides, children, }: { items: FeedbackSummary[]; @@ -386,7 +480,7 @@ function FeedbackResults({ community: string; timeRange: string; statusFilter: string; - statuses: FeedbackStatuses; + overrides: Record; children: (results: { communities: string[]; filtered: FeedbackSummary[]; @@ -394,14 +488,14 @@ function FeedbackResults({ }) { const results = useMemo(() => { const communities = [...new Set(items.map((item) => item.communityHost))] - .filter(Boolean) + .filter((host): host is string => Boolean(host)) .sort((left, right) => left.localeCompare(right)); const normalizedQuery = query.trim().toLocaleLowerCase(); const after = timeRangeStart(timeRange); const filtered = items.filter((item) => { + const status = overrides[item.id] ?? item.status; if (community !== "all" && item.communityHost !== community) return false; - if (statusFilter === "pending" && statuses[item.id]) return false; - if (statusFilter === "acted-on" && !statuses[item.id]) return false; + if (statusFilter !== "all" && status !== statusFilter) return false; if (after !== undefined) { const receivedAt = new Date(item.receivedAt).valueOf(); if (Number.isNaN(receivedAt) || receivedAt < after) return false; @@ -409,19 +503,25 @@ function FeedbackResults({ if (!normalizedQuery) return true; return [ item.bodySummary, - item.communityHost, + item.communityHost ?? "", item.category ?? "uncategorized", item.submitterPubkey, ].some((value) => value.toLocaleLowerCase().includes(normalizedQuery)); }); return { communities, filtered }; - }, [items, query, community, timeRange, statusFilter, statuses]); + }, [items, query, community, timeRange, statusFilter, overrides]); return children(results); } -function FeedbackDetailView({ id }: { id: string }) { +function FeedbackDetailView({ + id, + authMode, +}: { + id: string; + authMode: AuthMode; +}) { const resource = useResource( - () => request(`/feedback/${id}`), + () => request(`/feedback/${id}`, authMode), id, ); return ( @@ -434,11 +534,16 @@ function FeedbackDetailView({ id }: { id: string }) { > {(feedback) => { - const attachments = feedbackAttachments( - feedback.id, - feedback.tags, - feedback.communityHost, - ); + // Without an authoritative host we cannot validate attachment URLs or + // derive their fetch paths safely, so we render none and mark the + // provenance unavailable rather than guessing an origin. + const attachments = feedback.communityHost + ? feedbackAttachments( + feedback.id, + feedback.tags, + feedback.communityHost, + ) + : []; const body = stripAttachmentMarkdown(feedback.body, attachments); return (
@@ -448,7 +553,9 @@ function FeedbackDetailView({ id }: { id: string }) {
-

{feedback.communityHost}

+

+ +

@@ -460,8 +567,9 @@ function FeedbackDetailView({ id }: { id: string }) {
{attachments.map((attachment) => ( ))}
@@ -488,10 +596,8 @@ function FeedbackDetailView({ id }: { id: string }) { ); } -type FeedbackStatuses = Record; - interface FeedbackAttachment { - url: string; + path: string; sourceUrl: string; mimeType: string; hash: string; @@ -500,25 +606,6 @@ interface FeedbackAttachment { filename?: string; } -const FEEDBACK_STATUS_KEY = "buzz-admin-feedback-status"; - -function loadFeedbackStatuses(): FeedbackStatuses { - try { - const stored = localStorage.getItem(FEEDBACK_STATUS_KEY); - return stored ? (JSON.parse(stored) as FeedbackStatuses) : {}; - } catch { - return {}; - } -} - -function saveFeedbackStatuses(statuses: FeedbackStatuses) { - try { - localStorage.setItem(FEEDBACK_STATUS_KEY, JSON.stringify(statuses)); - } catch { - // The controls remain useful for the current session if storage is blocked. - } -} - function timeRangeStart(range: string) { const durations: Record = { day: 24 * 60 * 60 * 1000, @@ -551,7 +638,7 @@ function feedbackAttachments( const parsedSize = Number(values.get("size")); return [ { - url: `/api/admin/v1/feedback/${encodeURIComponent(feedbackId)}/attachments/${hash}`, + path: `/feedback/${encodeURIComponent(feedbackId)}/attachments/${hash}`, sourceUrl: safeUrl, mimeType, hash, @@ -595,8 +682,17 @@ function stripAttachmentMarkdown( .trim(); } -function Attachment({ attachment }: { attachment: FeedbackAttachment }) { - const url = attachment.url; +function Attachment({ + attachment, + authMode, +}: { + attachment: FeedbackAttachment; + authMode: AuthMode; +}) { + const [objectUrl, setObjectUrl] = useState(); + const [objectType, setObjectType] = useState(); + const [failed, setFailed] = useState(false); + const path = attachment.path; const name = attachment.filename ?? `attachment-${attachment.hash.slice(0, 8)}`; const metadata = [ @@ -607,33 +703,74 @@ function Attachment({ attachment }: { attachment: FeedbackAttachment }) { .filter(Boolean) .join(" · "); - if (attachment.mimeType.startsWith("image/")) { + useEffect(() => { + // The API requires an Authorization header, so the bytes are fetched here + // and handed to the DOM as an object URL revoked on replacement/unmount. + let url: string | undefined; + let active = true; + setObjectUrl(undefined); + setObjectType(undefined); + setFailed(false); + requestObjectUrl(path, authMode) + .then((created) => { + if (!active) { + URL.revokeObjectURL(created.url); + return; + } + url = created.url; + setObjectUrl(created.url); + setObjectType(created.type); + }) + .catch(() => { + if (active) setFailed(true); + }); + return () => { + active = false; + if (url) URL.revokeObjectURL(url); + }; + }, [path, authMode]); + + const detail = failed ? "Could not load attachment" : metadata; + + // Inline rendering is gated on the server-VERIFIED blob type, never the + // reporter-supplied `attachment.mimeType`: the relay only labels sniffed + // passive raster images as `image/*` and forces everything else to + // `application/octet-stream`, so a hostile payload can never render inline. + if (objectUrl && objectType?.startsWith("image/")) { return (
- - {name} + + {name}
{name} - {metadata} + {detail}
); } - return ( - + const label = ( + <> {name} - {metadata} + {detail} + + ); + + // Until the bytes are fetched there is nothing a link could point at: the + // API path itself would open unauthenticated in a new tab. + if (!objectUrl) return
{label}
; + + // Non-image (or not-yet-typed) payloads are download-only. The object URL + // already wraps `application/octet-stream` bytes, and the `download` + // attribute with no `target="_blank"` means clicking saves the file rather + // than navigating to a typed document on the admin origin. + return ( + + {label} ); @@ -797,18 +934,72 @@ function ArrowIcon() { ); } +/// Shown in nip98 mode when no NIP-07 extension is available. Instructs the +/// operator to install nos2x or Alby before continuing. +function Nip07Screen() { + return ( +
+
+

Nostr extension required

+

+ This relay uses NIP-98 HTTP Auth. Install a NIP-07 browser extension + such as{" "} + + nos2x + {" "} + or{" "} + + Alby + + , then reload this page. Your Nostr key will be used to sign each + request. +

+ +
+
+ ); +} + export function App() { const { path } = usePath(); + + // Probe the relay once to discover the auth mode. `null` means the probe is + // still in flight. Once resolved, the mode is stable for the session. + const [authMode, setAuthMode] = useState(null); + useEffect(() => { + let active = true; + probeAuthMode().then((mode) => { + if (active) setAuthMode(mode); + }); + return () => { + active = false; + }; + }, []); + const report = path.match(/^\/reports\/([^/]+)$/); const feedback = path.match(/^\/feedback\/([^/]+)$/); + + // Probe still in flight — render nothing to avoid a visible flash. + if (authMode === null) return null; + + // NIP-98 mode: require a NIP-07 extension. + if (authMode === "nip98" && !(window as Window & { nostr?: unknown }).nostr) + return ; + const content = report ? ( - + ) : feedback ? ( - + ) : path === "/feedback" ? ( - + ) : ( - + ); return (
diff --git a/admin-web/src/api.ts b/admin-web/src/api.ts index 9e5aa7569f..e096dcbf3b 100644 --- a/admin-web/src/api.ts +++ b/admin-web/src/api.ts @@ -9,11 +9,114 @@ export class ApiFailure extends Error { } } -export async function request(path: string): Promise { - const response = await fetch(`${PREFIX}${path}`, { - credentials: "same-origin", - headers: { accept: "application/json" }, +/// The authentication mode the relay requires, discovered via the probe. +/// - `nip98` — NIP-98 HTTP Auth; each request signed with a NIP-07 extension +/// - `disabled` — relay returned 200 with no auth; no credential needed +export type AuthMode = "nip98" | "disabled"; + +/// Sign a NIP-98 kind-27235 event for the given URL + method via window.nostr. +/// Throws if window.nostr is not available or signing fails. +/// +/// A fresh random `nonce` tag is generated on every call. Without it, two +/// same-URL requests in the same second produce byte-identical events (the +/// signed fields are only `u`, `method`, and `created_at` at 1-second +/// resolution), so their event IDs collide — the relay's NIP-98 replay guard +/// rejects the second, and the 401 retry re-signs the same fields and can +/// never recover. The verifier ignores unknown tags, so the nonce is inert to +/// verification and serves only to make each signed event unique. +/// +/// For body-bearing methods the caller passes `body`; a `payload` tag carrying +/// the hex SHA-256 of the exact bytes is added and signed. The relay's verifier +/// rejects a body-bearing request whose `payload` tag is absent or mismatched +/// (auth.rs), so the hash must be over the identical bytes the request sends. +async function signNip98( + url: string, + method: string, + body?: Uint8Array, +): Promise { + const nostr = (window as Window & typeof globalThis & { nostr?: Nostr98 }) + .nostr; + if (!nostr) throw new Error("No NIP-07 extension available"); + const nonce = crypto.getRandomValues(new Uint8Array(16)); + const nonceHex = toHex(nonce); + const tags: string[][] = [ + ["u", url], + ["method", method], + ["nonce", nonceHex], + ]; + if (body !== undefined) { + tags.push(["payload", await sha256Hex(body)]); + } + const event = await nostr.signEvent({ + kind: 27235, + created_at: Math.floor(Date.now() / 1000), + tags, + content: "", }); + return `Nostr ${btoa(JSON.stringify(event))}`; +} + +function toHex(bytes: Uint8Array): string { + return Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join(""); +} + +async function sha256Hex(bytes: Uint8Array): Promise { + const digest = await crypto.subtle.digest("SHA-256", bytes as BufferSource); + return toHex(new Uint8Array(digest)); +} + +// Minimal type for the NIP-07 window.nostr interface. +interface Nostr98 { + signEvent(event: { + kind: number; + created_at: number; + tags: string[][]; + content: string; + }): Promise>; +} + +/// Every admin API call goes through here. Attaches the correct credential +/// for the current auth mode: +/// - `nip98` mode: sign a kind-27235 event via NIP-07 for each request +/// - `disabled` mode: no credential +/// +/// A 401 in nip98 mode re-signs once (handles key rotation / clock skew) +/// and retries the request exactly once; a second 401 surfaces the error — +/// no infinite loop. +async function send( + path: string, + accept: string, + authMode: AuthMode, + init?: { method?: string; body?: Uint8Array; contentType?: string }, +): Promise { + const method = init?.method ?? "GET"; + const body = init?.body; + const doRequest = async () => { + const headers: Record = { accept }; + if (init?.contentType) headers["content-type"] = init.contentType; + if (authMode === "nip98") { + const url = `${location.protocol}//${location.host}${PREFIX}${path}`; + headers.authorization = await signNip98(url, method, body); + } + return fetch(`${PREFIX}${path}`, { + method, + credentials: "same-origin", + headers, + body: body as BodyInit | undefined, + }); + }; + + let response = await doRequest(); + + if (response.status === 401 && authMode === "nip98") { + // Re-sign with a fresh event and retry exactly once (handles clock + // skew or key rotation). A second 401 surfaces the error below. + response = await doRequest(); + } + + if (response.status === 401) { + throw new ApiFailure(401, "The admin credential was rejected."); + } if (!response.ok) { const envelope = await response.json().catch(() => null); throw new ApiFailure( @@ -21,5 +124,62 @@ export async function request(path: string): Promise { envelope?.error?.message ?? `Request failed (${response.status})`, ); } + return response; +} + +export async function request(path: string, authMode: AuthMode): Promise { + const response = await send(path, "application/json", authMode); return response.json() as Promise; } + +/// Send a body-bearing mutation (PATCH/PUT/POST) and parse the JSON response. +/// The body is serialized once and signed over those exact bytes so the NIP-98 +/// `payload` tag matches what the relay verifies. +export async function mutate( + path: string, + method: string, + body: unknown, + authMode: AuthMode, +): Promise { + const bytes = new TextEncoder().encode(JSON.stringify(body)); + const response = await send(path, "application/json", authMode, { + method, + body: bytes, + contentType: "application/json", + }); + return response.json() as Promise; +} + +/// Probe whether the relay requires authentication. +/// Issues one unauthenticated request: +/// - 200 → `disabled` (no auth needed) +/// - anything else → `nip98` (the only authenticated mode; fail-secure) +export async function probeAuthMode(): Promise { + try { + const response = await fetch(`${PREFIX}/reports`, { + credentials: "same-origin", + headers: { accept: "application/json" }, + }); + return response.status === 200 ? "disabled" : "nip98"; + } catch { + // Network error — default to nip98 so a credential is required. + return "nip98"; + } +} + +/// Attachments cannot be fetched by `` or `` because those +/// carry no Authorization header. Callers render the object URL and must +/// revoke it when it is replaced or unmounted. +/// +/// Returns the server-verified `type` alongside the URL. The relay sniffs the +/// stored bytes and only labels verified passive raster images as `image/*`; +/// everything else is `application/octet-stream`. Callers must decide inline +/// rendering from this type, never from the untrusted reporter-supplied MIME. +export async function requestObjectUrl( + path: string, + authMode: AuthMode, +): Promise<{ url: string; type: string }> { + const response = await send(path, "*/*", authMode); + const blob = await response.blob(); + return { url: URL.createObjectURL(blob), type: blob.type }; +} diff --git a/admin-web/src/styles.css b/admin-web/src/styles.css index 8f0ba3d6c3..c55b033a66 100644 --- a/admin-web/src/styles.css +++ b/admin-web/src/styles.css @@ -304,14 +304,31 @@ h1 { gap: 0.4rem; color: rgb(35 30 30 / 62%); font-size: 0.78rem; +} + +.feedback-status select { + font: inherit; + padding: 0.25rem 0.4rem; + border-radius: 0.4rem; + border: 1px solid rgb(35 30 30 / 18%); + background: #fff; + color: #231e1e; cursor: pointer; } -.feedback-status input { - width: 1rem; - height: 1rem; - margin: 0; - accent-color: #231e1e; +.feedback-status select:disabled { + opacity: 0.55; + cursor: progress; +} + +.status-error { + color: #b3261e; + font-size: 0.72rem; +} + +.provenance-unavailable { + font-style: italic; + color: rgb(35 30 30 / 45%); } .record-open-link { @@ -670,6 +687,18 @@ dd { color: #9f2424; } +.auth-prompt button { + margin-top: 1rem; +} + +.attachment-placeholder { + min-height: 8rem; + display: grid; + place-content: center; + color: rgb(35 30 30 / 48%); + font-size: 0.8rem; +} + @media (max-width: 720px) { .app-header { width: min(100% - 2rem, 1120px); diff --git a/admin-web/src/types.ts b/admin-web/src/types.ts index 6c10837758..042ec434bf 100644 --- a/admin-web/src/types.ts +++ b/admin-web/src/types.ts @@ -23,25 +23,33 @@ export interface ReportDetail extends Report { message: ReportedMessage | null; } +export type FeedbackStatus = "new" | "reviewed" | "archived"; + export interface FeedbackSummary { id: string; - communityId: string; - communityHost: string; + /// `null` once the source community is purged (provenance severed). + communityId: string | null; + /// `null` when `communityId` is severed — feedback retained without origin. + communityHost: string | null; submitterPubkey: string; category?: string; bodySummary: string; + status: FeedbackStatus; receivedAt: string; } export interface FeedbackDetail { id: string; - communityId: string; - communityHost: string; + /// `null` once the source community is purged (provenance severed). + communityId: string | null; + /// `null` when `communityId` is severed — feedback retained without origin. + communityHost: string | null; eventId: string; submitterPubkey: string; category?: string; body: string; tags: string[][]; + status: FeedbackStatus; eventCreatedAt: string; receivedAt: string; } diff --git a/admin-web/tests/auth.spec.ts b/admin-web/tests/auth.spec.ts new file mode 100644 index 0000000000..7f9ca37637 --- /dev/null +++ b/admin-web/tests/auth.spec.ts @@ -0,0 +1,521 @@ +import { expect, type Page, test } from "@playwright/test"; + +interface ObjectUrlLog { + created: string[]; + revoked: string[]; +} + +declare global { + interface Window { + objectUrlLog: ObjectUrlLog; + } +} + +/// Injects a minimal window.nostr stub that returns a fake signed event, so a +/// test can drive nip98 mode without a real NIP-07 extension. The stub derives +/// the event `id` from the signed fields (tags + created_at + content), so two +/// signings collide iff their signed payloads are byte-identical — exactly the +/// property the relay's replay guard keys on. +async function seedNip98(page: Page) { + await page.addInitScript(() => { + (window as Window & { nostr?: unknown }).nostr = { + signEvent: async (event: { + kind: number; + created_at: number; + tags: string[][]; + content: string; + }) => { + const serialized = JSON.stringify([ + event.kind, + event.created_at, + event.tags, + event.content, + ]); + // Cheap non-crypto digest of the signed fields, hex-padded to 64 chars. + let h = 0; + for (let i = 0; i < serialized.length; i++) { + h = (Math.imul(31, h) + serialized.charCodeAt(i)) | 0; + } + const id = (h >>> 0).toString(16).padStart(8, "0").repeat(8); + return { + ...event, + id, + pubkey: "b".repeat(64), + sig: "c".repeat(128), + }; + }, + }; + }); +} + +/// Decode an `Authorization: Nostr ` header to the signed event. +function decodeNostrHeader(header: string): { id: string; tags: string[][] } { + return JSON.parse(atob(header.replace(/^Nostr /, ""))); +} + +test("nip98 mode: attachments are fetched with a signed credential and rendered from blob urls", async ({ + page, +}) => { + const id = "feedback-with-attachments"; + const imageHash = "a".repeat(64); + const fileHash = "b".repeat(64); + const imageUrl = `https://design.buzz.xyz/media/${imageHash}.png`; + const fileUrl = `https://design.buzz.xyz/media/${fileHash}.txt`; + await seedNip98(page); + + const attachmentRequests: { path: string; authorization?: string }[] = []; + await page.route(`**/api/admin/v1/feedback/${id}/attachments/**`, (route) => { + attachmentRequests.push({ + path: new URL(route.request().url()).pathname, + authorization: route.request().headers().authorization, + }); + route.fulfill({ contentType: "application/octet-stream", body: "bytes" }); + }); + await page.route(`**/api/admin/v1/feedback/${id}`, (route) => + route.fulfill({ + contentType: "application/json", + body: JSON.stringify({ + id, + communityId: "one", + communityHost: "design.buzz.xyz", + eventId: "31".repeat(32), + submitterPubkey: "21".repeat(32), + category: "bug", + body: "Composer froze.", + tags: [ + [ + "imeta", + `url ${imageUrl}`, + "m image/png", + `x ${imageHash}`, + "filename screenshot.png", + ], + [ + "imeta", + `url ${fileUrl}`, + "m text/plain", + `x ${fileHash}`, + "filename diagnostics.txt", + ], + ], + eventCreatedAt: "2026-07-17T17:25:00Z", + receivedAt: "2026-07-17T17:30:00Z", + }), + }), + ); + + await page.goto(`/feedback/${id}`); + await expect( + page.getByRole("img", { name: "screenshot.png" }), + ).toHaveAttribute("src", /^blob:/); + await expect( + page.getByRole("link", { name: /diagnostics.txt/ }), + ).toHaveAttribute("href", /^blob:/); + + expect(attachmentRequests.map((request) => request.path).sort()).toEqual( + [ + `/api/admin/v1/feedback/${id}/attachments/${imageHash}`, + `/api/admin/v1/feedback/${id}/attachments/${fileHash}`, + ].sort(), + ); + for (const request of attachmentRequests) { + expect(request.authorization).toMatch(/^Nostr /); + } +}); + +/// Records every object URL the SPA creates and revokes, so a test can prove a +/// blob handed to the DOM is released rather than merely replaced. +async function instrumentObjectUrls(page: Page) { + await page.addInitScript(() => { + const log: ObjectUrlLog = { created: [], revoked: [] }; + window.objectUrlLog = log; + const create = URL.createObjectURL.bind(URL); + const revoke = URL.revokeObjectURL.bind(URL); + URL.createObjectURL = (source: Blob | MediaSource) => { + const url = create(source); + log.created.push(url); + return url; + }; + URL.revokeObjectURL = (url: string) => { + log.revoked.push(url); + revoke(url); + }; + }); +} + +const FEEDBACK_ID = "feedback-with-attachments"; +const IMAGE_HASH = "a".repeat(64); +const FILE_HASH = "b".repeat(64); + +/// A feedback detail carrying one image and one non-image attachment. The +/// probe to `/reports` returns 200 so the SPA runs in disabled mode: these +/// tests exercise object-URL lifecycle, not authentication. +async function routeFeedbackDetail(page: Page) { + const host = "design.buzz.xyz"; + await page.route(`**/api/admin/v1/reports**`, (route) => + route.fulfill({ contentType: "application/json", body: "[]" }), + ); + await page.route(`**/api/admin/v1/feedback?**`, (route) => + route.fulfill({ contentType: "application/json", body: "[]" }), + ); + await page.route(`**/api/admin/v1/feedback/${FEEDBACK_ID}`, (route) => + route.fulfill({ + contentType: "application/json", + body: JSON.stringify({ + id: FEEDBACK_ID, + communityId: "one", + communityHost: host, + eventId: "31".repeat(32), + submitterPubkey: "21".repeat(32), + category: "bug", + body: "Composer froze.", + tags: [ + [ + "imeta", + `url https://${host}/media/${IMAGE_HASH}.png`, + "m image/png", + `x ${IMAGE_HASH}`, + "filename screenshot.png", + ], + [ + "imeta", + `url https://${host}/media/${FILE_HASH}.txt`, + "m text/plain", + `x ${FILE_HASH}`, + "filename diagnostics.txt", + ], + ], + eventCreatedAt: "2026-07-17T17:25:00Z", + receivedAt: "2026-07-17T17:30:00Z", + }), + }), + ); +} + +test("attachment object urls are revoked when the view is left", async ({ + page, +}) => { + await instrumentObjectUrls(page); + await routeFeedbackDetail(page); + await page.route( + `**/api/admin/v1/feedback/${FEEDBACK_ID}/attachments/**`, + (route) => + route.fulfill({ contentType: "application/octet-stream", body: "bytes" }), + ); + + await page.goto(`/feedback/${FEEDBACK_ID}`); + const imageUrl = await page + .getByRole("img", { name: "screenshot.png" }) + .getAttribute("src"); + const fileUrl = await page + .getByRole("link", { name: /diagnostics.txt/ }) + .getAttribute("href"); + expect(imageUrl).toMatch(/^blob:/); + expect(fileUrl).toMatch(/^blob:/); + expect(await page.evaluate(() => window.objectUrlLog.revoked)).toEqual([]); + + await page.getByRole("link", { name: "Back to feedback" }).click(); + await expect(page.getByRole("heading", { name: "Feedback" })).toBeVisible(); + + await expect + .poll(() => page.evaluate(() => window.objectUrlLog.revoked)) + .toEqual(expect.arrayContaining([imageUrl, fileUrl])); +}); + +test("an attachment that arrives after the view is left is revoked immediately", async ({ + page, +}) => { + await instrumentObjectUrls(page); + await routeFeedbackDetail(page); + let release = () => {}; + const held = new Promise((resolve) => { + release = resolve; + }); + await page.route( + `**/api/admin/v1/feedback/${FEEDBACK_ID}/attachments/**`, + async (route) => { + await held; + await route.fulfill({ + contentType: "application/octet-stream", + body: "bytes", + }); + }, + ); + + await page.goto(`/feedback/${FEEDBACK_ID}`); + // Both fetches are held, so no blob exists yet. + await expect(page.getByText("Loading…")).toBeVisible(); + expect(await page.evaluate(() => window.objectUrlLog.created)).toEqual([]); + + // Leave before either fetch resolves, then let both complete. + await page.getByRole("link", { name: "Back to feedback" }).click(); + await expect(page.getByRole("heading", { name: "Feedback" })).toBeVisible(); + release(); + + await expect + .poll(() => page.evaluate(() => window.objectUrlLog.revoked.length)) + .toBe(2); + const log = await page.evaluate(() => window.objectUrlLog); + expect(log.revoked.sort()).toEqual(log.created.sort()); + // Nothing was ever handed to the DOM: the blobs outlived their view. + await expect(page.getByRole("img", { name: "screenshot.png" })).toHaveCount( + 0, + ); +}); + +test("probe: disabled mode renders directly when the probe returns 200", async ({ + page, +}) => { + // The probe to /api/admin/v1/reports returns 200, indicating the relay runs + // in disabled mode. The dashboard must render directly with no credential. + await page.route("**/api/admin/v1/reports**", (route) => + route.fulfill({ contentType: "application/json", body: "[]" }), + ); + + await page.goto("/reports"); + + await expect( + page.getByRole("heading", { name: "Open reports" }), + ).toBeVisible(); + await expect( + page.getByRole("heading", { name: "Nostr extension required" }), + ).toHaveCount(0); +}); + +test("probe: nip98 mode without a NIP-07 extension shows the installation screen", async ({ + page, +}) => { + // The probe returns 401 and window.nostr is NOT injected, so the dashboard + // must show the extension installation screen instead of the dashboard. + await page.route("**/api/admin/v1/**", (route) => + route.fulfill({ + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "nip98 required" }, + }), + }), + ); + + await page.goto("/reports"); + + await expect( + page.getByRole("heading", { name: "Nostr extension required" }), + ).toBeVisible(); + await expect(page.getByRole("heading", { name: "Open reports" })).toHaveCount( + 0, + ); +}); + +test("probe: nip98 mode with a mocked NIP-07 extension signs requests and renders the dashboard", async ({ + page, +}) => { + await seedNip98(page); + + const authorizationHeaders: (string | undefined)[] = []; + await page.route("**/api/admin/v1/**", async (route) => { + const headers = route.request().headers(); + authorizationHeaders.push(headers.authorization); + // Probe: return 401 to trigger nip98 mode detection. + if (!headers.authorization) { + await route.fulfill({ + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "nip98 required" }, + }), + }); + } else { + // Any Authorization: Nostr header → accept. + await route.fulfill({ contentType: "application/json", body: "[]" }); + } + }); + + await page.goto("/reports"); + + await expect( + page.getByRole("heading", { name: "Open reports" }), + ).toBeVisible(); + await expect( + page.getByRole("heading", { name: "Nostr extension required" }), + ).toHaveCount(0); + // The authenticated request used Authorization: Nostr. + const authenticatedHeaders = authorizationHeaders.filter(Boolean); + expect(authenticatedHeaders.length).toBeGreaterThan(0); + for (const h of authenticatedHeaders) { + expect(h).toMatch(/^Nostr /); + } +}); + +test("nip98 mode: same-second retry re-signs with a distinct event id", async ({ + page, +}) => { + // Freeze the clock so both signings share created_at (1s resolution). With + // only u+method+created_at signed, the two events would be byte-identical + // and collide in the relay's replay guard, so the 401 retry could never + // recover. The per-signing random nonce tag must make the second event's id + // distinct despite the frozen clock. + await page.addInitScript(() => { + const FROZEN = 1_760_000_000_000; + const RealDate = Date; + // biome-ignore lint/suspicious/noExplicitAny: minimal Date shim for the test + (globalThis as any).Date = class extends RealDate { + constructor(...args: unknown[]) { + // biome-ignore lint/suspicious/noExplicitAny: forward constructor args + super(...(args.length ? (args as any) : [FROZEN])); + } + static now() { + return FROZEN; + } + }; + }); + await seedNip98(page); + + const authCalls: string[] = []; + let signCount = 0; + await page.route("**/api/admin/v1/**", async (route) => { + const headers = route.request().headers(); + if (!headers.authorization) { + await route.fulfill({ + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "nip98 required" }, + }), + }); + return; + } + authCalls.push(headers.authorization); + signCount++; + // First authenticated attempt → reject, forcing the re-sign + retry. + await route.fulfill( + signCount === 1 + ? { + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "rejected" }, + }), + } + : { contentType: "application/json", body: "[]" }, + ); + }); + + await page.goto("/reports"); + await expect( + page.getByRole("heading", { name: "Open reports" }), + ).toBeVisible(); + await page.waitForLoadState("networkidle"); + + expect(authCalls).toHaveLength(2); + const [first, second] = authCalls.map(decodeNostrHeader); + // Same frozen created_at, yet distinct ids — the nonce tag did its job. + expect(first.id).not.toBe(second.id); + const nonceOf = (tags: string[][]) => tags.find((t) => t[0] === "nonce")?.[1]; + expect(nonceOf(first.tags)).toBeTruthy(); + expect(nonceOf(second.tags)).toBeTruthy(); + expect(nonceOf(first.tags)).not.toBe(nonceOf(second.tags)); +}); + +test("nip98 mode: first-401-then-200 retries once and renders the dashboard", async ({ + page, +}) => { + // Models a credential that is momentarily rejected (clock skew, key + // rotation) then accepted on the second attempt. + let signCount = 0; + await seedNip98(page); + + const authCalls: string[] = []; + await page.route("**/api/admin/v1/**", async (route) => { + const headers = route.request().headers(); + if (!headers.authorization) { + // Probe — announce nip98 mode. + await route.fulfill({ + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "nip98 required" }, + }), + }); + return; + } + authCalls.push(headers.authorization); + signCount++; + if (signCount === 1) { + // First authenticated attempt → reject. + await route.fulfill({ + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "rejected" }, + }), + }); + } else { + // Second attempt → accept. + await route.fulfill({ contentType: "application/json", body: "[]" }); + } + }); + + await page.goto("/reports"); + + // The retry should succeed. Wait for network to settle (both attempts + // complete) before asserting the authCalls count. + await page.waitForLoadState("networkidle"); + // Exactly two distinct Nostr credentials were sent (one per attempt). + expect(authCalls).toHaveLength(2); + for (const h of authCalls) { + expect(h).toMatch(/^Nostr /); + } +}); + +test("nip98 mode: persistent 401 surfaces error after exactly one retry", async ({ + page, +}) => { + // Every authenticated request returns 401. The SPA must attempt exactly + // two requests (first attempt + one retry) and then surface the error — + // never a third attempt. + await seedNip98(page); + + const authCalls: string[] = []; + await page.route("**/api/admin/v1/**", async (route) => { + const headers = route.request().headers(); + if (!headers.authorization) { + await route.fulfill({ + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "nip98 required" }, + }), + }); + return; + } + authCalls.push(headers.authorization); + await route.fulfill({ + status: 401, + headers: { "www-authenticate": "Nostr" }, + contentType: "application/json", + body: JSON.stringify({ + error: { code: "unauthorized", message: "rejected" }, + }), + }); + }); + + await page.goto("/reports"); + + // After the retry fails, the error state renders. The StateView shows + // "Could not load data" inside a role=alert region. + await expect(page.getByRole("alert")).toBeVisible(); + await expect( + page.getByRole("heading", { name: "Could not load data" }), + ).toBeVisible(); + // Exactly two Nostr credentials sent — no third attempt. + await page.waitForLoadState("networkidle"); + expect(authCalls).toHaveLength(2); +}); diff --git a/admin-web/tests/csp.spec.ts b/admin-web/tests/csp.spec.ts new file mode 100644 index 0000000000..7cc0d830c1 --- /dev/null +++ b/admin-web/tests/csp.spec.ts @@ -0,0 +1,105 @@ +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { expect, test } from "@playwright/test"; + +const ROUTER_RS = fileURLToPath( + new URL("../../crates/buzz-relay/src/router.rs", import.meta.url), +); + +/// The exact policy the relay serves on admin SPA documents. Read from the +/// relay source rather than copied, so this test can never pass against a +/// policy operators do not actually get. The preview server used here serves +/// no CSP of its own, so it is injected below. +function adminCsp() { + const source = readFileSync(ROUTER_RS, "utf8"); + const match = source.match(/const ADMIN_CSP: &str = "([^"]+)";/); + if (!match) throw new Error(`ADMIN_CSP not found in ${ROUTER_RS}`); + return match[1]; +} + +test("the relay admin csp does not break the built dashboard", async ({ + page, +}) => { + const csp = adminCsp(); + expect(csp).toContain("frame-ancestors 'none'"); + expect(csp).not.toContain("unsafe-inline"); + + // Every admin API call returns 200, so the probe resolves to disabled mode + // and the dashboard renders without a credential. + await page.route("**/api/admin/v1/**", (route) => + route.fulfill({ contentType: "application/json", body: "[]" }), + ); + // Only the document request: the API call to /reports carries a query string. + await page.route( + (url) => url.pathname === "/reports" && url.search === "", + async (route) => { + const response = await route.fetch(); + await route.fulfill({ + response, + headers: { ...response.headers(), "content-security-policy": csp }, + }); + }, + ); + + const violations: string[] = []; + page.on("console", (message) => { + if (message.text().includes("Content Security Policy")) + violations.push(message.text()); + }); + + await page.goto("/reports"); + + // Rendering at all proves the bundle's script and stylesheet loaded, and the + // empty state proves the fetch to /api/admin/v1 survived `connect-src 'self'`. + await expect( + page.getByRole("heading", { name: "Open reports" }), + ).toBeVisible(); + await expect(page.getByText("No records.")).toBeVisible(); + expect(violations).toEqual([]); +}); + +test("the linked favicon loads under the admin csp", async ({ page }) => { + const csp = adminCsp(); + + await page.route( + (url) => url.pathname === "/" && url.search === "", + async (route) => { + const response = await route.fetch(); + await route.fulfill({ + response, + headers: { ...response.headers(), "content-security-policy": csp }, + }); + }, + ); + + const violations: string[] = []; + page.on("console", (message) => { + if (message.text().includes("Content Security Policy")) + violations.push(message.text()); + }); + + await page.goto("/"); + + const href = await page.locator("link[rel=icon]").getAttribute("href"); + expect(href).toBe("/favicon.svg"); + + // Headless Chromium never issues the `` request itself, so + // load the same file the same way the policy sees it: an image fetch under + // `img-src 'self'`. A blocked fetch rejects `decode()`; a successful one + // proves the icon renders, and that the SVG's own inline