feat(protocol,relay): enforce union membership in both directions

.changeset/union-membership-enforced.md

@@ -0,0 +1,48 @@
+---
+'@tapflowio/protocol': minor
+'@tapflowio/relay': minor
+---
+
+fix: enforce union membership in both directions, not just narrowing
+
+The wire-contract program made every message's **fields** checked and left its **set membership**
+checked in one direction only. Narrowing was held by the compiler and held well — `sendTo` refuses a
+message outside its union. Widening was free: measured on `main`, adding `DeviceBooting` to
+`BrowserToRelay` left `pnpm typecheck` at zero errors and all 294 static tests green.
+
+## The copy with the security consequence
+
+`AGENT_MSG_TYPES` in the relay is a hand-maintained second list of what an agent produces, and the
+door check closes a `browser`-role socket with 1008 for any member. The forwards it guards mostly
+resolve a session from the message and send to *that session's* browser with no check that the sender
+is that session's agent — `clipboard:*` is the deliberate exception. So an agent→browser message added
+to the protocol and forgotten in that list makes a viewer drivable by anyone who knows a session id,
+with the type union claiming otherwise.
+
+Measured before this change: dropping `keyboard:toggled` from the set left both suites green.
+`clipboard:data` was held only because somebody had written that one test by hand.
+
+Types erase, so no runtime array can be derived from a union. What is available is the compiler
+checking two lists against each other, and that is what this adds — as type-level assertions, so a
+violated invariant is a compile error at the declaration rather than a test somebody has to run.
+
+Three invariants now hold:
+
+- the relay's `MessageType` covers every protocol literal and invents none. It was missing
+  `stream:request-idr` — the exact drift `protocol/AGENTS.md` cites as this package's reason to exist,
+  still alive in the copy underneath it;
+- `AGENT_MSG_TYPES` equals what the agent directions declare, both ways;
+- **nothing a browser may send is something an agent produces.** This is the one that catches widening
+  without restating 63 literals, and it is the invariant the door enforces at runtime. Not blanket
+  disjointness: `device:shutdown` is deliberately a member of both `RelayToAgent` and `BrowserToRelay`.
+
+## And the half a type cannot state about itself
+
+A message declared in the protocol but placed in **no** direction reaches none of the above — it is
+absent from the union those assertions read, so nothing is ever obliged to know it. Types cannot
+enumerate their own declarations, so that one is checked as source text alongside the two facts
+`protocolMessageNames.test.mjs` already checks that way. All 65 declared messages reach a direction
+today.
+
+`AnyWireMessage` is new and public: the seven directions unioned, so a consumer can assert its own
+list is complete rather than merely correct so far.

packages/protocol/src/index.ts

@@ -932,6 +932,27 @@ export type AppClearStateReplyBody<T = AppClearStateReply> = T extends unknown ?
  *  at its own send site in `agent-core/src/utils/stream.ts`. */
 export type AgentControlOutbound = AgentToRelay | AgentToBrowser
 
+/**
+ * Every message this protocol declares, reached through the seven directions.
+ *
+ * Not a fourteenth union for its own sake: it is what lets a consumer assert that its own list of
+ * literals is *complete* rather than merely correct so far. `relay/src/types.ts` keeps such a list —
+ * hand-maintained, 62 entries, and missing `stream:request-idr` until this change, which is the exact
+ * drift this package was created to end (see AGENTS.md).
+ *
+ * The fact it restates is small and stable — **which directions exist** — not the 63 literals inside
+ * them. A message added to a direction flows in here for free; a message added to *no* direction does
+ * not, and nothing here can see that. That gap is real and is not this union's to close.
+ */
+export type AnyWireMessage =
+  | BrowserToRelay
+  | RelayToBrowser
+  | AgentToRelay
+  | AgentToBrowser
+  | RelayToAgent
+  | StreamToRelay
+  | RelayToStream
+
 // ── browser → relay ──────────────────────────────────────────────────────────
 
 /** Key input. The payload carries `code` — a `KeyboardEvent.code` name — and `modifiers` as a

packages/protocol/src/typeAssertions.ts

@@ -17,6 +17,7 @@ import type {
   InputTypeError, KeyboardToggled, OpenUrl, OpenUrlDone, OpenUrlError, RelayOutbound, ScreenshotDone,
   ScreenshotError, ScreenshotRequest, SessionAgentAway, SessionChrome, SessionDeviceInfo, SessionEnd,
   SessionJoined, SessionLeave, SessionRebound, SessionStart, SessionTerminated, StreamRegister,
+  AgentControlOutbound, StreamToRelay,
   StreamRegistered, StreamRequestIdr, UiTreeError, UiTreeRequest, UiTreeResponse,
 } from './index.js'
 
@@ -177,3 +178,23 @@ export const _ScreenshotError: ScreenshotError['type'] = 'screenshot:error'
 export const _StreamRegister: StreamRegister['type'] = 'stream:register'
 export const _UiTreeResponse: UiTreeResponse['type'] = 'ui:tree:response'
 export const _UiTreeError: UiTreeError['type'] = 'ui:tree:error'
+
+// ── membership: what a browser may send, and what an agent produces, do not overlap ──────────────
+//
+// The relay's door closes a `browser`-role socket with 1008 for any agent-produced type, because the
+// forwards it guards mostly resolve a session from the message and send to *that session's* browser
+// with no check that the sender is that session's agent. So the two sets overlapping is not an
+// untidiness — it is a message a browser can inject into a stranger's viewer.
+//
+// Stated as `Extract` rather than as a list, which is what lets it catch a widening without restating
+// 63 literals. Measured before this existed: adding `DeviceBooting` to `BrowserToRelay` left
+// `pnpm typecheck` at zero errors and all 294 static tests green.
+//
+// **Not blanket disjointness between directions.** `device:shutdown` is deliberately a member of both
+// `RelayToAgent` and `BrowserToRelay`, identical in both; it is not agent-produced, so it is not here.
+// And a *relay*-produced message added to `BrowserToRelay` is outside this claim — `route()` has no
+// case for one, and #557 is where the forwarding half is tracked.
+type AgentProduced = (AgentControlOutbound | StreamToRelay)['type']
+type AssertTrue<T extends true> = T
+type NoOverlap<A, B> = [Extract<A, B>] extends [never] ? true : false
+export type _BrowserSendsNothingAgentProduced = AssertTrue<NoOverlap<BrowserToRelay['type'], AgentProduced>>

packages/relay/src/RelayServer.ts

@@ -7,8 +7,10 @@ import { randomUUID } from 'crypto'
 import { WebSocketServer, WebSocket } from 'ws'
 import { SessionManager } from './SessionManager.js'
 import type { Session } from './SessionManager.js'
-import type { DeviceDetails, RelayMessage, UIElement } from './types.js'
-import type { ChromePayload, InputErrorReason, RelayOutbound } from '@tapflowio/protocol'
+import type { Assert, DeviceDetails, IsEmpty, RelayMessage, UIElement } from './types.js'
+import type {
+  AgentControlOutbound, ChromePayload, InputErrorReason, RelayOutbound, StreamToRelay,
+} from '@tapflowio/protocol'
 import { Router, json } from './router.js'
 import { requireViewAuth, requireAuth, getAuth, verifyPat } from './middleware/auth.js'
 import { classifyConnection } from './lib/connectionAuth.js'
@@ -168,7 +170,7 @@ function isCorrelated(msg: RelayMessage): msg is RelayMessage & { requestId: str
   return false
 }
 
-const AGENT_MSG_TYPES = new Set([
+const AGENT_MSG_TYPE_LIST = [
   'agent:register', 'agent:resources', 'screenshot:done', 'screenshot:error',
   'ui:tree:response', 'ui:tree:error',
   'app:clear-state-done', 'app:clear-state-error',
@@ -183,7 +185,44 @@ const AGENT_MSG_TYPES = new Set([
   // stream:register binds a session's stream socket — agent-only, or a browser
   // (view PAT / cookie) could hijack an existing session's video feed.
   'stream:register',
-])
+] as const
+
+/** Typed `ReadonlySet<string>`, deliberately: the door below tests a `MessageType`, which is wider
+ *  than the literals above, and `Set<T>.has` takes a `T`. The literal union stays reachable through
+ *  the array, which is where the assertions read it from. */
+const AGENT_MSG_TYPES: ReadonlySet<string> = new Set(AGENT_MSG_TYPE_LIST)
+
+// ── the list above is checked against the protocol, both ways (#532) ─────────────────────────────
+//
+// It is a hand-maintained second copy of "what an agent produces", and it is the copy with the
+// security consequence: the door below closes a `browser`-role socket with 1008 for any member. The
+// forwards it guards mostly resolve a session from the message and send to *that session's* browser
+// with no check that the sender is that session's agent — `clipboard:*` is the deliberate exception.
+// So an agent→browser message added to the protocol and forgotten here makes a viewer drivable by
+// anyone who knows a session id, with the type union claiming otherwise.
+//
+// Measured before this change: dropping `keyboard:toggled` from the set left the static suite and
+// the relay suite green. `clipboard:data` was held only because somebody wrote that one test by
+// hand. The other 28 entries were held by nothing.
+//
+// Derivation is not available — types erase, so no runtime array can come out of a union. What is
+// available is the compiler checking two lists against each other, which is what these two lines do.
+type AgentProduced = (AgentControlOutbound | StreamToRelay)['type']
+type Listed = (typeof AGENT_MSG_TYPE_LIST)[number]
+// **`satisfies` first, and it is the security direction's real floor.** `Exclude<AgentProduced, string>`
+// is `never`, so if the list ever widens past its literals — dropping `as const`, or one non-literal
+// element — the covering assertion below passes while checking nothing. Only its sibling would fail,
+// and that sibling's own comment calls its direction the one that "gates nothing", so an author
+// trusting the comment could delete the wrong one. This line fails first and names the list.
+AGENT_MSG_TYPE_LIST satisfies readonly AgentProduced[]
+type _AgentSetCoversProtocol = Assert<IsEmpty<Exclude<AgentProduced, Listed>>>
+type _AgentSetInventsNothing = Assert<IsEmpty<Exclude<Listed, AgentProduced>>>
+
+// The invariant the door *enforces* — that nothing a browser may send is something an agent produces
+// — is asserted in `protocol/src/typeAssertions.ts` instead. It is a claim about the protocol's own
+// directions, not about this file, and naming `BrowserToRelay` here made `clientOutboundTyped` read
+// the relay as a browser-role sender. That guard was right to say so.
+
 
 export class RelayServer {
   private httpServer: http.Server | https.Server

packages/relay/src/types.ts

@@ -15,6 +15,10 @@ export type MessageType =
   | 'session:rebound'
   | 'stream:register'
   | 'stream:registered'
+  // Was missing. The relay sends it from two places and its own union did not declare it — the
+  // very drift `protocol/AGENTS.md` cites as this package's reason to exist, still alive in the
+  // copy underneath it. The assertion below is what makes a third omission a compile error.
+  | 'stream:request-idr'
   | 'device:boot'
   | 'device:booting'
   | 'device:ready'
@@ -67,11 +71,36 @@ export type { AgentResources, UIElement }
 
 // The wire contract lives in @tapflowio/protocol so the relay, the dashboard and mcp-server cannot
 // drift apart. `DeviceInfo` is kept as an alias while call sites move over.
-import type { DeviceSummary, SessionInfo, SessionTerminatedReason } from '@tapflowio/protocol'
+import type { AnyWireMessage, DeviceSummary, SessionInfo, SessionTerminatedReason } from '@tapflowio/protocol'
 export type { DeviceDetails, DeviceReport, DeviceSummary, SessionInfo, SessionTerminatedReason } from '@tapflowio/protocol'
 
 export type DeviceInfo = DeviceSummary
 
+// ── membership, enforced in both directions (#532) ───────────────────────────────────────────────
+//
+// The wire-contract program made every message's *fields* checked and left its *set membership*
+// unchecked in one direction. Narrowing was held by the compiler and held well — `sendTo` refuses a
+// message outside its union. **Widening was free**: measured on `main`, adding `DeviceBooting` to
+// `BrowserToRelay` left `pnpm typecheck` at zero errors and all 294 static tests green.
+//
+// These are type-level and cost nothing at runtime. `Assert` fails to instantiate when its argument
+// is not `true`, so a violated invariant is a compile error at the declaration rather than a test
+// somebody has to run.
+export type Assert<T extends true> = T
+/** `[T] extends [never]` rather than `T extends never`: a bare conditional distributes over a union
+ *  and answers `never` for an empty one *and* for a non-empty one, which would pass either way. */
+export type IsEmpty<T> = [T] extends [never] ? true : false
+
+/**
+ * The relay's own literal list is complete against the protocol.
+ *
+ * Both directions. Missing an entry is what happened to `stream:request-idr`; an extra one means a
+ * literal the protocol no longer declares, which reads as deliberate and gates nothing.
+ */
+type _MessageTypeCoversProtocol = Assert<IsEmpty<Exclude<AnyWireMessage['type'], MessageType>>>
+type _MessageTypeInventsNothing = Assert<IsEmpty<Exclude<MessageType, AnyWireMessage['type']>>>
+
+
 // `agents:listed` groups devices by agent machine. Protocol owns the shape; this file used to
 // declare an identical copy. Its `devices` element is protocol's `DeviceSummary`, which the alias
 // below already points at.

scripts/__tests__/clientOutboundTyped.test.mjs

@@ -112,7 +112,12 @@ describe('browser-role outbound is typed against the wire contract', () => {
     const browserRole = new Set(
       tracked
         .filter((f) => pkgOf(f) !== 'protocol')
-        .filter((f) => /BrowserToRelay/.test(readFileSync(join(root, f), 'utf8')))
+        // **Comments stripped first.** A mention in prose is not a usage, and the raw form counted one:
+        // a comment in `relay/src/RelayServer.ts` explaining why that file must *not* name the union
+        // put the relay in this set. Same shape as the `browserInboundRouting` defect this repo
+        // already recorded, where a comment mentioning `{ type: 'error' }` was counted as a union
+        // member — and the same shape as the offender scan two lines down, which already strips.
+        .filter((f) => /BrowserToRelay/.test(stripComments(readFileSync(join(root, f), 'utf8'))))
         .map(pkgOf),
     )
     expect([...browserRole].sort()).toEqual(['dashboard', 'flow-runner', 'mcp-server'])