feat: [ENG-2933] add brv channel oclif command stubs + daemon round-t…#748
Conversation
|
Claude finished @leehpham's task in 3m 52s —— View job Claude PR Review
SummaryThis PR lands the Strengths
Findings (see inline)
Conformance with
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| public static description = 'List channels' | ||
| public static examples = [ | ||
| '<%= config.bin %> <%= command.id %>', | ||
| '<%= config.bin %> <%= command.id %> --archived --json', | ||
| ] | ||
| public static flags = { | ||
| archived: Flags.boolean({description: 'Include archived channels'}), | ||
| } |
There was a problem hiding this comment.
issue (non-blocking, formatting): public static description, public static examples, and public static flags are flush against column 0 rather than indented two spaces as class members. Same pattern appears in invite.ts, list-turns.ts, mention.ts, onboard.ts, and show.ts — only new.ts is correctly indented. This will almost certainly fail npm run lint (Prettier) and reads awkwardly next to the indented args / run() members.
| public static description = 'List channels' | |
| public static examples = [ | |
| '<%= config.bin %> <%= command.id %>', | |
| '<%= config.bin %> <%= command.id %> --archived --json', | |
| ] | |
| public static flags = { | |
| archived: Flags.boolean({description: 'Include archived channels'}), | |
| } | |
| public static description = 'List channels' | |
| public static examples = [ | |
| '<%= config.bin %> <%= command.id %>', | |
| '<%= config.bin %> <%= command.id %> --archived --json', | |
| ] | |
| public static flags = { | |
| archived: Flags.boolean({description: 'Include archived channels'}), | |
| } |
| let nextPort = 9740 | ||
|
|
||
| /** A booted in-process channel daemon plus the seam a command connects through. */ | ||
| export type ChannelHarness = { | ||
| /** Connector to inject into a command's `channelClientOptions()` (DI seam). */ | ||
| connector: TransportConnector | ||
| /** Port the in-process transport server is bound to. */ | ||
| port: number | ||
| /** Disconnects clients, stops the server, and restores mutated env. */ | ||
| teardown: () => Promise<void> | ||
| } | ||
|
|
||
| /** | ||
| * Boots a real {@link SocketIOTransportServer} in-process and registers the | ||
| * channel handler surface exactly the way production does — gated through the | ||
| * real {@link channelsEnabled} check. With `enabled` the 10 per-event handlers | ||
| * are wired (so a `channel:*` request resolves to `CHANNEL_NOT_IMPLEMENTED`); | ||
| * without it, {@link registerDisabledStubs} is used (resolving to | ||
| * `CHANNEL_DISABLED`). The returned `connector` drives the actual oclif command | ||
| * against this server with no subprocess and no daemon spawn. | ||
| */ | ||
| export async function startChannelHarness(options: {enabled: boolean; port?: number}): Promise<ChannelHarness> { | ||
| const previousSessionLog = process.env.BRV_SESSION_LOG | ||
| const previousChannelsFlag = process.env.BRV_CHANNELS_ENABLED | ||
| process.env.BRV_SESSION_LOG = '/dev/null' | ||
| process.env.BRV_CHANNELS_ENABLED = options.enabled ? '1' : '0' |
There was a problem hiding this comment.
suggestion (low): Two pieces of shared global state make this harness brittle to parallel execution:
let nextPort = 9740at module scope is allocated per-process. With Mocha's default sequential mode this is fine; if anyone later flips on--parallel, two workers would each start at 9740 and collide.process.env.BRV_SESSION_LOGandprocess.env.BRV_CHANNELS_ENABLEDare mutated globally with the previous value snapshotted onstartChannelHarness. If two harnesses overlap in the same process, the second snapshot captures the first harness's value, and teardown order can leak state into adjacent tests.
For (1), await server.start(0) and read server.address()?.port (or whatever the equivalent on SocketIOTransportServer is) to let the OS pick a free port. For (2), worth a comment in the harness noting "single-harness-at-a-time per process" so a future contributor doesn't combine harnesses inside one describe.
Neither is blocking — just guardrails for the next person.
| /** Fallback wire code when a thrown error carries no channel-specific code. */ | ||
| const CHANNEL_REQUEST_FAILED = 'CHANNEL_REQUEST_FAILED' | ||
| /** TransportRequestError appends this suffix to its message; strip it for display. */ | ||
| const EVENT_SUFFIX_PATTERN = / for event '[^']+'$/ | ||
|
|
||
| /** Daemon options a channel command forwards to the shared transport layer. */ | ||
| export type ChannelClientOptions = Pick< | ||
| DaemonClientOptions, | ||
| 'maxRetries' | 'projectPath' | 'projectRootFlag' | 'retryDelayMs' | 'transportConnector' | ||
| > | ||
|
|
||
| /** A channel-domain error surfaced to the CLI, carrying the wire `code`. */ | ||
| export class ChannelClientError extends Error { | ||
| public readonly code: string | ||
| public readonly details?: unknown | ||
|
|
||
| public constructor(code: string, message: string, details?: unknown) { | ||
| super(message) | ||
| this.name = 'ChannelClientError' | ||
| this.code = code | ||
| this.details = details | ||
| } | ||
| } | ||
|
|
||
| /** Minimal channel request seam over the daemon transport. */ | ||
| export interface ChannelClient { | ||
| disconnect(): Promise<void> | ||
| request<TResponse = unknown>(event: string, payload?: unknown): Promise<TResponse> | ||
| } | ||
|
|
||
| /** | ||
| * Normalizes any thrown transport/daemon error into a {@link ChannelClientError}. | ||
| * A daemon-thrown channel error arrives as a {@link TransportRequestError} whose | ||
| * `code` is forwarded verbatim from the `{success, code, error}` ack envelope; | ||
| * connection/spawn failures fall back to {@link formatConnectionError}. | ||
| */ | ||
| export function toChannelClientError(error: unknown): ChannelClientError { | ||
| if (error instanceof ChannelClientError) return error | ||
| if (error instanceof TransportRequestError) { | ||
| return new ChannelClientError(error.code ?? CHANNEL_REQUEST_FAILED, error.message.replace(EVENT_SUFFIX_PATTERN, '')) | ||
| } | ||
|
|
||
| return new ChannelClientError(CHANNEL_REQUEST_FAILED, formatConnectionError(error)) | ||
| } |
There was a problem hiding this comment.
nitpick: The / for event '[^']+'$/ regex and the "strip the event suffix from a TransportRequestError" logic also live in src/oclif/lib/daemon-client.ts:234. Two copies of the same scrub rule will eventually drift. Consider exporting the pattern (or a stripEventSuffix(msg) helper) from daemon-client.ts and reusing it here — daemon-client.ts is already the source of formatConnectionError, which is the natural home.
Not a blocker; just one source of truth.
| export interface ChannelClient { | ||
| disconnect(): Promise<void> | ||
| request<TResponse = unknown>(event: string, payload?: unknown): Promise<TResponse> | ||
| } |
There was a problem hiding this comment.
question: disconnect() is exposed on the ChannelClient interface but isn't called anywhere in the current PR — withChannelClient delegates cleanup to withDaemonRetry, and there are no callers of connectChannelClient yet. Is it intentionally kept for the M0-2 callers (e.g. the channel subscribe long-lived stream that isn't wired through oclif yet)? If so, fine to leave; if not, dropping it until a consumer exists would tighten the surface (per CLAUDE.md Outside-In: "Only what the consumer actually requires").
| public channelClientOptions(): ChannelClientOptions { | ||
| return {projectPath: process.cwd()} | ||
| } |
There was a problem hiding this comment.
thought: Making channelClientOptions() a regular public method that the integration test reassigns on the instance (command.channelClientOptions = () => (...) in test/integration/channel-command-surface.test.ts:69) works because the call site is this.channelClientOptions() and the method isn't bound — but instance reassignment of class methods is a slightly unusual seam and easy to miss when refactoring (e.g. if anyone ever converts this to an arrow property or destructures it).
Two more conventional alternatives:
- Make it
protectedand have tests use a tiny subclass override. - Accept a
channelClientOptions?: ChannelClientOptionsvia the constructor or asetChannelClientOptions(opts)setter, and remove the method-as-seam.
Functional today; just noting it for the next person who has to extend the base.
| describe('enabled surface (BRV_CHANNELS_ENABLED=1)', () => { | ||
| let harness: ChannelHarness | ||
|
|
||
| beforeEach(async () => { | ||
| harness = await startChannelHarness({enabled: true}) | ||
| }) | ||
|
|
||
| afterEach(async () => { | ||
| await harness.teardown() | ||
| }) | ||
|
|
||
| for (const {argv, commandClass, label} of COMMAND_CASES) { | ||
| it(`\`channel ${label}\` builds a schema-valid request that reaches CHANNEL_NOT_IMPLEMENTED`, async () => { | ||
| const {exitCode, logs} = await runInProcess(commandClass, config, harness.connector, argv) | ||
|
|
||
| expect(exitCode, label).to.equal(1) | ||
| const parsed = parseEnvelope(logs) | ||
| expect(isErrorEnvelope(parsed), label).to.equal(true) | ||
| if (isErrorEnvelope(parsed)) { | ||
| expect(parsed.code, label).to.equal('CHANNEL_NOT_IMPLEMENTED') | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| it('emits a `{success:false, code, error}` envelope on stdout in JSON mode', async () => { | ||
| const {exitCode, logs} = await runInProcess(ChannelNew, config, harness.connector, ['x', '--json']) | ||
|
|
||
| expect(exitCode).to.equal(1) | ||
| const parsed = parseEnvelope(logs) | ||
| expect(isErrorEnvelope(parsed)).to.equal(true) | ||
| if (isErrorEnvelope(parsed)) { | ||
| expect(parsed.code).to.equal('CHANNEL_NOT_IMPLEMENTED') | ||
| expect(parsed.success).to.equal(false) | ||
| expect(parsed.error).to.have.length.greaterThan(0) | ||
| } | ||
| }) | ||
|
|
||
| it('renders a [CODE] message to stderr in text mode', async () => { | ||
| const {exitCode, stderrLogs} = await runInProcess(ChannelNew, config, harness.connector, ['x']) | ||
|
|
||
| expect(exitCode).to.equal(1) | ||
| expect(stderrLogs.join('\n')).to.include('[CHANNEL_NOT_IMPLEMENTED]') | ||
| }) | ||
|
|
||
| it('surfaces CHANNEL_INVALID_REQUEST when an invited handle is missing the @ prefix', async () => { | ||
| const {exitCode, logs} = await runInProcess(ChannelInvite, config, harness.connector, ['my-channel', 'codex', '--json']) | ||
|
|
||
| expect(exitCode).to.equal(1) | ||
| const parsed = parseEnvelope(logs) | ||
| expect(isErrorEnvelope(parsed)).to.equal(true) | ||
| if (isErrorEnvelope(parsed)) { | ||
| expect(parsed.code).to.equal('CHANNEL_INVALID_REQUEST') | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
praise: Nice end-to-end coverage here — booting a real SocketIOTransportServer, registering the production handler set behind the real channelsEnabled() gate, and driving the actual oclif command class (no mocks, no subprocess) is exactly the kind of test that catches transport-shape regressions. The schema-failure case (CHANNEL_INVALID_REQUEST when @ is missing) is also a great smoke check that the zod schemas are actually wired through. 👍
1 participant
…rip client.
Summary
Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
User-visible changes
List user-visible changes (including defaults, config, or CLI output).
If none, write
None.Evidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
List real risks for this PR. If none, write
None.