feat: [ENG-2930] brv channel transport events + validating handler skeleton#747
Conversation
|
Claude finished @leehpham's task in 4m 18s —— View job Claude Code ReviewOverall score: 4 / 5 — Solid skeleton. Wire contract is well-scoped, errors are correctly typed for the transport envelope, the feature gate is right (opt-out + stubs to avoid client hangs), and 36 unit tests are present. A handful of concrete issues to address before merge — none blocking by themselves, but worth fixing while the surface has no consumers yet. Findings (inline comments posted)
What's good
Acceptance criteria
Suggested next stepAddress items #1-4 (the issues), then file follow-ups for #5-6 (the questions) so the M1 author has a clean seam to build on. I have not verified
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import type {ITransportServer} from "../../../../core/interfaces/transport/index.js"; | ||
| import type {ITransportHandler} from "../i-transport-handler.js"; | ||
|
|
||
| import {type ChannelCreateRequest, ChannelCreateRequestSchema, type ChannelCreateResponse, ChannelEvents} from "../../../../../shared/transport/events/channel-events.js"; | ||
| import {ChannelNotImplementedError} from "../../../../core/domain/channel/errors.js"; | ||
| import {parseOrThrow} from "./parse-or-throw.js"; | ||
|
|
||
| export class ChannelCreateHandler implements ITransportHandler<ChannelCreateRequest, ChannelCreateResponse> { | ||
| private readonly event: string | ||
| private readonly transportServer: ITransportServer | ||
|
|
||
| public constructor(transportServer: ITransportServer) { | ||
| this.event = ChannelEvents.CREATE | ||
| this.transportServer = transportServer | ||
| } | ||
|
|
||
| public async handle(request: ChannelCreateRequest): Promise<ChannelCreateResponse> { | ||
| parseOrThrow(ChannelCreateRequestSchema, request) | ||
| throw new ChannelNotImplementedError(this.event) | ||
| } | ||
|
|
||
| public setup(): void { | ||
| this.transportServer.onRequest<ChannelCreateRequest, ChannelCreateResponse>(this.event, request => this.handle(request)) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
issue (style): This file is the odd one out — all 9 sibling handlers use single quotes, no semicolons, multi-line imports, a class-level JSDoc, and (request) => … arrow syntax. This handler uses double quotes, semicolons, an inlined import, no JSDoc, and request => …. The PR body claims npm run lint is clean (so perhaps Prettier isn't enforced cross-file), but the visual drift across an otherwise uniform skeleton is jarring and will keep showing up in future diffs to this directory.
| import type {ITransportServer} from "../../../../core/interfaces/transport/index.js"; | |
| import type {ITransportHandler} from "../i-transport-handler.js"; | |
| import {type ChannelCreateRequest, ChannelCreateRequestSchema, type ChannelCreateResponse, ChannelEvents} from "../../../../../shared/transport/events/channel-events.js"; | |
| import {ChannelNotImplementedError} from "../../../../core/domain/channel/errors.js"; | |
| import {parseOrThrow} from "./parse-or-throw.js"; | |
| export class ChannelCreateHandler implements ITransportHandler<ChannelCreateRequest, ChannelCreateResponse> { | |
| private readonly event: string | |
| private readonly transportServer: ITransportServer | |
| public constructor(transportServer: ITransportServer) { | |
| this.event = ChannelEvents.CREATE | |
| this.transportServer = transportServer | |
| } | |
| public async handle(request: ChannelCreateRequest): Promise<ChannelCreateResponse> { | |
| parseOrThrow(ChannelCreateRequestSchema, request) | |
| throw new ChannelNotImplementedError(this.event) | |
| } | |
| public setup(): void { | |
| this.transportServer.onRequest<ChannelCreateRequest, ChannelCreateResponse>(this.event, request => this.handle(request)) | |
| } | |
| } | |
| import type {ITransportServer} from '../../../../core/interfaces/transport/index.js' | |
| import type {ITransportHandler} from '../i-transport-handler.js' | |
| import { | |
| ChannelCreateRequest, | |
| ChannelCreateRequestSchema, | |
| ChannelCreateResponse, | |
| ChannelEvents, | |
| } from '../../../../../shared/transport/events/channel-events.js' | |
| import {ChannelNotImplementedError} from '../../../../core/domain/channel/errors.js' | |
| import {parseOrThrow} from './parse-or-throw.js' | |
| /** | |
| * Handles `channel:create`. `handle` validates the payload, then throws | |
| * {@link ChannelNotImplementedError}; the create behavior lands in a later | |
| * milestone, written after the validation step in `handle`. | |
| */ | |
| export class ChannelCreateHandler | |
| implements ITransportHandler<ChannelCreateRequest, ChannelCreateResponse> | |
| { | |
| private readonly event: string | |
| private readonly transportServer: ITransportServer | |
| public constructor(transportServer: ITransportServer) { | |
| this.event = ChannelEvents.CREATE | |
| this.transportServer = transportServer | |
| } | |
| public async handle(request: ChannelCreateRequest): Promise<ChannelCreateResponse> { | |
| parseOrThrow(ChannelCreateRequestSchema, request) | |
| throw new ChannelNotImplementedError(this.event) | |
| } | |
| public setup(): void { | |
| this.transportServer.onRequest<ChannelCreateRequest, ChannelCreateResponse>(this.event, (request) => | |
| this.handle(request), | |
| ) | |
| } | |
| } |
| const STUBBABLE_EVENTS = [ | ||
| ChannelEvents.CREATE, | ||
| ChannelEvents.LIST, | ||
| ChannelEvents.GET, | ||
| ChannelEvents.INVITE, | ||
| ChannelEvents.ONBOARD, | ||
| ChannelEvents.MENTION, | ||
| ChannelEvents.SHOW, | ||
| ChannelEvents.LIST_TURNS, | ||
| ChannelEvents.SUBSCRIBE, | ||
| ChannelEvents.CANCEL, | ||
| ] as const |
There was a problem hiding this comment.
issue (maintainability): STUBBABLE_EVENTS re-lists the same 10 events that feature-handlers.ts instantiates handlers for (lines 153-162). If a new channel:* request event is added later (e.g. M4's channel:permission-decision), three places need to stay in sync: the per-event handler instantiation in feature-handlers.ts, this list, and ChannelEvents itself. A drift here means a request event registered in the "enabled" branch silently hangs in the "disabled" branch with no ack.
One option: drive both from a single source — e.g. a CHANNEL_REQUEST_EVENTS tuple exported from channel-events.ts, used by both feature-handlers.ts (as keys for the handler-instance map) and registerDisabledStubs. Out of scope for the skeleton, but worth a TODO if you don't want to do it now.
| * Reports whether the channel surface is enabled. Opt-out: enabled unless | ||
| * `BRV_CHANNELS_ENABLED` is `0`, `false`, `no`, or `off` (case-insensitive). | ||
| */ | ||
| export const channelsEnabled = (env: NodeJS.ProcessEnv = process.env): boolean => { |
There was a problem hiding this comment.
nitpick (code quality): Default-arg evaluation is fine here, but the gate is only consulted once at boot in feature-handlers.ts — using process.env directly inside is a tiny readability win (the parameter signals "this is testable" — keep it). The bigger note: BRV_CHANNELS_ENABLED should land in CLAUDE.md's Environment section so future contributors can find it; right now the only documentation of this env var is this file's docstring.
| export * from './agent-events.js' | ||
| export * from './auth-events.js' | ||
| export * from './billing-events.js' | ||
| export * from './channel-events.js' |
There was a problem hiding this comment.
question: ChannelEvents is exported via export * here but is NOT added to AllEventGroups (lines 69-98). AllEventGroups is currently unused inside the repo, so there's no immediate functional gap — but the convention every other events module follows is "if it's a …Events const, it goes in AllEventGroups". Was the omission intentional (e.g. channels stay out of broad subscriptions/diagnostics), or just an oversight?
If intentional, a one-line comment in the array explaining why channels is excluded would help future readers. If oversight:
| export * from './channel-events.js' | |
| import {ChannelEvents} from './channel-events.js' |
(plus an entry in the AllEventGroups tuple.)
| export const ChannelListTurnsRequestSchema = z.object({ | ||
| channelId: z.string(), | ||
| cursor: z.string().optional(), | ||
| limit: z.number().int().positive().optional(), | ||
| }) |
There was a problem hiding this comment.
issue (security): limit is z.number().int().positive().optional() with no upper bound. Once the orchestrator lands and this actually queries a turn store, a malicious or buggy client can request limit: Number.MAX_SAFE_INTEGER and force an unbounded scan/serialization. The wire contract is the cheapest place to clamp this — e.g. .max(200) (pick a sensible cap). Catching it at the schema means M1 implementers don't have to remember to defend against it.
| export const ChannelListTurnsRequestSchema = z.object({ | |
| channelId: z.string(), | |
| cursor: z.string().optional(), | |
| limit: z.number().int().positive().optional(), | |
| }) | |
| export const ChannelListTurnsRequestSchema = z.object({ | |
| channelId: z.string(), | |
| cursor: z.string().optional(), | |
| limit: z.number().int().positive().max(200).optional(), | |
| }) |
| /** | ||
| * channel:invite — `invocation` is intentionally loose at the skeleton layer; | ||
| * the launch-spec shape is tightened when the driver/profile machinery lands. | ||
| */ | ||
| export const ChannelInviteRequestSchema = z.object({ | ||
| channelId: z.string(), | ||
| handle: HandleSchema, | ||
| invocation: z.unknown().optional(), | ||
| profileName: z.string().optional(), | ||
| }) |
There was a problem hiding this comment.
issue (correctness): invocation: z.unknown().optional() is documented as "intentionally loose at the skeleton layer", which is reasonable — but z.unknown() accepts any JSON value including arrays, primitives, and null. When M1 tightens this to a LaunchSpec shape, that's a wire-breaking change to clients that have been sending invocation: "hello" or invocation: [1, 2, 3] and getting valid acks.
If you want to keep this open without painting yourself into a wire-break, z.record(z.unknown()).optional() (or even z.object({}).passthrough().optional()) at least asserts "object-shaped" now, so the future tightening only narrows record keys instead of also rejecting whole new value kinds. Same note applies to ChannelOnboardRequestSchema.invocation (line 103).
| } | ||
|
|
||
| return parsed.data | ||
| } |
There was a problem hiding this comment.
praise: Nice small utility — single responsibility, throws the right typed error with flattened zod details, and the per-handler call sites stay one line. Forwarding the parsed T (rather than just returning void) is also the right shape for M1, when handlers actually need the narrowed value.
| export interface ITransportHandler<Request, Response> { | ||
| handle(request: Request): Promise<Response> | ||
| setup(): void | ||
| } No newline at end of file |
There was a problem hiding this comment.
question: Is this intended to become the canonical handler contract that other (non-channel) handlers in index.ts migrate to as well? Right now it's a single 4-line file with no JSDoc, used only by the 10 channel handlers. If it's general, a brief docstring on the interface explaining the handle vs setup split (and the setup → onRequest → handle lifecycle) would help. If it's channel-specific, the file is fine where it is but should probably move under handlers/channel/ to avoid implying broader scope.
Also: handle discards clientId because setup() calls onRequest(event, (request) => this.handle(request)). That's correct for the not-implemented skeleton, but the PR body notes "withAuth / projectRootFromCtx(ctx) deferred to M1" — when those land, the handler will need clientId, which means either widening handle to (request, clientId) or wrapping at setup(). Worth a one-liner here flagging that.
1 participant
M0-3 — Transport events + handler skeleton
Lays down the
brv channelwire contract and a validating daemon handler skeleton. No agent behavior yet — every handler validates its payload, then returns a typed not-implemented, so M1–M4 fill in behavior without touching the wire shape or command layer.Linear: ENG-2930 · builds on M0-1 (ENG-2928) + M0-2 (ENG-2929).
What's included
src/shared/transport/events/channel-events.ts:ChannelEvents(10 request events + 3 broadcasts) with<Op>RequestSchema/<Op>ResponseSchemaZod pairs reusing the M0-1 domain schemas (mention/showcarryTurn/TurnEventunchanged).src/server/core/domain/channel/errors.ts:CHANNEL_NOT_IMPLEMENTED/CHANNEL_DISABLED/CHANNEL_INVALID_REQUEST(thrownChannelErrorsubclasses carrying.code, which the Socket.IO transport serializes into the{success:false, code}envelope).src/server/infra/transport/handlers/channel/, each implementing the new generalITransportHandler<Request, Response>(handle+setup).setup()wiresonRequest → handle;handle()runsparseOrThrowthen throws — the M1 seam for the orchestrator call.channel-disabled-handler.ts:channelsEnabled()(opt-out,BRV_CHANNELS_ENABLED) +registerDisabledStubs(). Registered inline infeature-handlers.tsbehind the gate.Acceptance criteria
CHANNEL_INVALID_REQUEST; valid →CHANNEL_NOT_IMPLEMENTED(flag on); flag off →CHANNEL_DISABLED. ✅npm run typecheck+npm run lintclean. ✅Deviations from the issue spec (deliberate)
ITransportHandlerinstead of oneChannelHandlerwithregisterOn(maintainability; shared contract for future handler refactors).withAuth/projectRootFromCtx(ctx)deferred to M1 — the repo'sonRequestis(data, clientId)(noctx), and not-implemented bodies need neither.feature-handlers.ts(the registration hub), notbrv-server.ts.test/unit/infra/transport/handlers/channel/(repo convention drops theserver/segment).Out of scope (later milestones)
IChannelOrchestratorwiring + realhandlebodies (M1),channel:permission-decision(M4), oclif command stubs +brv channel --help(ENG-2933 / M0-4).