feat(cli): stream broker startup progress with --verbose#1218
Conversation
…ose` `up` previously surfaced only a terse error (e.g. "Service Unavailable") when broker startup raced or stalled, with no visibility into which step failed. `--verbose` now logs CLI-side step markers (port resolution, broker spawn, handshake retries, fleet sidecar, node delivery wait, agent spawns) and streams the broker's own startup-phase logs and stderr live instead of only buffering them for crash reports.
📝 WalkthroughWalkthroughThis PR adds an optional verbose mode to ChangesVerbose Startup Logging
Sequence Diagram(s)sequenceDiagram
participant CoreCommand
participant createRuntimeClient
participant HarnessDriverClient
participant BrokerProcess
CoreCommand->>createRuntimeClient: createRuntimeClient(onStep, onStderr)
createRuntimeClient->>HarnessDriverClient: spawn(onStep, onStderr)
HarnessDriverClient->>BrokerProcess: spawn broker binary
HarnessDriverClient-->>CoreCommand: onStep(broker binary resolved)
HarnessDriverClient-->>CoreCommand: onStep(API listening)
HarnessDriverClient-->>CoreCommand: onStep(handshake retry)
HarnessDriverClient-->>CoreCommand: onStep(handshake complete)
HarnessDriverClient-->>CoreCommand: onStep(event stream connected)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --verbose flag to the agent-relay up command, enabling step-by-step startup progress logging and streaming of the broker's logs and stderr. It propagates the verbose option through the CLI commands, broker lifecycle management, and the harness driver client. The review feedback suggests improving the dependency injection pattern in createDefaultRelay by allowing an optional logError parameter instead of hardcoding console.error directly, which would facilitate custom logging and testing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async function createDefaultRelay( | ||
| cwd: string, | ||
| apiPort = 0, | ||
| brokerName?: string, | ||
| verbose = false | ||
| ): Promise<CoreRelay> { |
There was a problem hiding this comment.
To maintain the dependency injection pattern of the codebase and allow custom logging (e.g., in tests or programmatic wrappers), consider adding an optional logError parameter to createDefaultRelay that defaults to console.error.
| async function createDefaultRelay( | |
| cwd: string, | |
| apiPort = 0, | |
| brokerName?: string, | |
| verbose = false | |
| ): Promise<CoreRelay> { | |
| async function createDefaultRelay( | |
| cwd: string, | |
| apiPort = 0, | |
| brokerName?: string, | |
| verbose = false, | |
| logError: (message: string) => void = console.error | |
| ): Promise<CoreRelay> { |
| ...(verbose | ||
| ? { | ||
| onStep: (message: string) => console.error(`[agent-relay][verbose] ${message}`), | ||
| onStderr: (line: string) => console.error(`[broker] ${line}`), | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
Use the injected logError function instead of hardcoding console.error directly. This ensures that verbose logs can be captured or redirected by custom loggers.
| ...(verbose | |
| ? { | |
| onStep: (message: string) => console.error(`[agent-relay][verbose] ${message}`), | |
| onStderr: (line: string) => console.error(`[broker] ${line}`), | |
| } | |
| : {}), | |
| ...(verbose | |
| ? { | |
| onStep: (message: string) => logError(`[agent-relay][verbose] ${message}`), | |
| onStderr: (line: string) => logError(`[broker] ${line}`), | |
| } | |
| : {}), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/harness-driver/src/client.ts (1)
293-393: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded
onStepcalls can abort broker startup if a consumer-supplied callback throws.
HarnessDriverClient.spawn()is a public entry point andonStepis now part of its external contract, but everyonStep?.(...)call here is invoked directly — an exception thrown from a caller's callback would propagate and fail the whole startup. This file already establishes the opposite pattern for callbacks inrunBeforeSpawn, which explicitly catches handler exceptions so they "do not abort the chain."🛡️ Proposed fix to guard callback invocation
static async spawn(options?: RuntimeSpawnOptions): Promise<HarnessDriverClient> { - const onStep = options?.onStep; + const onStep = options?.onStep + ? (message: string) => { + try { + options.onStep!(message); + } catch { + // step callback failures must not abort broker startup + } + } + : undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness-driver/src/client.ts` around lines 293 - 393, The startup flow in HarnessDriverClient.spawn is vulnerable because each onStep callback is invoked directly, so a consumer-thrown exception can abort broker startup. Update the onStep?.(...) calls in spawn to use the same guarded callback pattern already used for runBeforeSpawn, catching and suppressing callback errors so they do not break the spawn sequence. Keep the fix localized around the existing onStep usage and preserve the current startup logic for binary resolution, broker process spawning, handshake polling, and event stream setup.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim implementation detail from the changelog bullet.
The new entry enumerates internal startup phases (port resolution, broker process spawn, handshake retries, fleet sidecar, node-delivery wait, agent spawns) rather than staying impact-first and concise. As per coding guidelines, "Changelog entries should be concise and impact-first, with one short bullet per user-visible change... Omit issue/PR links, internal notes, and implementation details."
📝 Suggested concise rewrite
-- `agent-relay up --verbose` now prints step-by-step startup progress (port resolution, broker process spawn, handshake retries, fleet sidecar, node-delivery wait, agent spawns) and streams the broker's own startup-phase logs and stderr live, instead of only surfacing a terse error if startup fails. +- `agent-relay up --verbose` now prints step-by-step startup progress and streams the broker's startup logs live, instead of only surfacing a terse error if startup fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 13, Trim the CHANGELOG bullet for the agent-relay up --verbose update so it stays impact-first and concise; remove the internal startup-phase enumeration and any implementation detail, and rewrite the entry to focus only on the user-visible behavior change in a single short bullet.Source: Coding guidelines
packages/cli/src/cli/commands/core.ts (1)
165-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerbose hooks bypass the
depslogging abstraction and split output across streams.
onStep/onStderrcallconsole.errordirectly instead ofdeps.log/deps.error, whilevlog()inbroker-lifecycle.tswrites verbose messages viadeps.log(stdout). This means verbose progress lines end up split across stdout (broker-lifecycle'svlog) and stderr (this file'sonStep/onStderr) for what is conceptually the same--verbosefeature, and these calls can't be intercepted viadepsmocking in tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/commands/core.ts` around lines 165 - 170, The verbose callbacks in core.ts are bypassing the shared deps logging API and sending output directly to stderr, which splits the verbose experience across streams and makes testing harder. Update the object passed from the verbose branch so onStep and onStderr route through the injected deps.log/deps.error methods instead of console.error, and keep their formatting aligned with broker-lifecycle.ts’s vlog() usage so all verbose output is handled consistently through deps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/harness-driver/src/client.ts`:
- Around line 302-308: The verbose step logging is exposing the workspace key in
cleartext. Update the logging in client.ts around buildBrokerSpawnConfig and the
onStep calls so any args string is redacted before being emitted, especially the
--workspace-key value; also fix the later log that prints session?.workspace_key
directly by masking or omitting the secret. Keep the existing step messages, but
route them through a small redaction helper so broker spawn diagnostics remain
useful without leaking credentials.
---
Outside diff comments:
In `@packages/harness-driver/src/client.ts`:
- Around line 293-393: The startup flow in HarnessDriverClient.spawn is
vulnerable because each onStep callback is invoked directly, so a
consumer-thrown exception can abort broker startup. Update the onStep?.(...)
calls in spawn to use the same guarded callback pattern already used for
runBeforeSpawn, catching and suppressing callback errors so they do not break
the spawn sequence. Keep the fix localized around the existing onStep usage and
preserve the current startup logic for binary resolution, broker process
spawning, handshake polling, and event stream setup.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 13: Trim the CHANGELOG bullet for the agent-relay up --verbose update so
it stays impact-first and concise; remove the internal startup-phase enumeration
and any implementation detail, and rewrite the entry to focus only on the
user-visible behavior change in a single short bullet.
In `@packages/cli/src/cli/commands/core.ts`:
- Around line 165-170: The verbose callbacks in core.ts are bypassing the shared
deps logging API and sending output directly to stderr, which splits the verbose
experience across streams and makes testing harder. Update the object passed
from the verbose branch so onStep and onStderr route through the injected
deps.log/deps.error methods instead of console.error, and keep their formatting
aligned with broker-lifecycle.ts’s vlog() usage so all verbose output is handled
consistently through deps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9624234-57e3-487d-a920-5ad571922b95
📒 Files selected for processing (7)
CHANGELOG.mdpackages/cli/src/cli/commands/core.test.tspackages/cli/src/cli/commands/core.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/client-factory.tspackages/harness-driver/src/client.tspackages/harness-driver/src/spawn-config.ts
| onStep?.(`Resolved broker binary: ${binaryPath}`); | ||
| const apiKey = `br_${randomBytes(16).toString('hex')}`; | ||
| const { cwd, timeoutMs, args, env } = buildBrokerSpawnConfig(options, apiKey); | ||
| const stderrLines: string[] = []; | ||
| const stdoutLines: string[] = []; | ||
|
|
||
| onStep?.(`Spawning broker process: ${binaryPath} ${args.join(' ')}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Verbose step log leaks the workspace key in cleartext.
buildBrokerSpawnConfig embeds the workspace key directly into args (...(workspaceKey ? ['--workspace-key', workspaceKey] : [])), and this line logs args.join(' ') verbatim via onStep. Since --verbose is added specifically so users can capture and share this output when relay local up fails, this credential is very likely to end up pasted into bug reports/chat logs. Same issue at line 389, which logs session?.workspace_key directly.
🔒 Proposed fix to redact the workspace key from verbose output
onStep?.(`Resolved broker binary: ${binaryPath}`);
const apiKey = `br_${randomBytes(16).toString('hex')}`;
const { cwd, timeoutMs, args, env } = buildBrokerSpawnConfig(options, apiKey);
const stderrLines: string[] = [];
const stdoutLines: string[] = [];
- onStep?.(`Spawning broker process: ${binaryPath} ${args.join(' ')}`);
+ const redactedArgs = args.map((arg, i) => (args[i - 1] === '--workspace-key' ? '[REDACTED]' : arg));
+ onStep?.(`Spawning broker process: ${binaryPath} ${redactedArgs.join(' ')}`);And further down:
- onStep?.(`Broker handshake complete (workspace: ${session?.workspace_key ?? 'unknown'})`);
+ onStep?.('Broker handshake complete.');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onStep?.(`Resolved broker binary: ${binaryPath}`); | |
| const apiKey = `br_${randomBytes(16).toString('hex')}`; | |
| const { cwd, timeoutMs, args, env } = buildBrokerSpawnConfig(options, apiKey); | |
| const stderrLines: string[] = []; | |
| const stdoutLines: string[] = []; | |
| onStep?.(`Spawning broker process: ${binaryPath} ${args.join(' ')}`); | |
| onStep?.(`Resolved broker binary: ${binaryPath}`); | |
| const apiKey = `br_${randomBytes(16).toString('hex')}`; | |
| const { cwd, timeoutMs, args, env } = buildBrokerSpawnConfig(options, apiKey); | |
| const stderrLines: string[] = []; | |
| const stdoutLines: string[] = []; | |
| const redactedArgs = args.map((arg, i) => (args[i - 1] === '--workspace-key' ? '[REDACTED]' : arg)); | |
| onStep?.(`Spawning broker process: ${binaryPath} ${redactedArgs.join(' ')}`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/harness-driver/src/client.ts` around lines 302 - 308, The verbose
step logging is exposing the workspace key in cleartext. Update the logging in
client.ts around buildBrokerSpawnConfig and the onStep calls so any args string
is redacted before being emitted, especially the --workspace-key value; also fix
the later log that prints session?.workspace_key directly by masking or omitting
the secret. Keep the existing step messages, but route them through a small
redaction helper so broker spawn diagnostics remain useful without leaking
credentials.
1 participant
Summary
agent-relay up --verbosenow logs CLI-side step markers as it starts the broker: API port resolution, orphan-process cleanup, broker process spawn, handshake retry attempts, status checks, fleet sidecar startup, node-delivery wait, and per-agent spawns.[agent-relay][startup +Nms] ...phase logs, gated byAGENT_RELAY_STARTUP_DEBUG, already on by default) to live console output when--verboseis set — previously those lines were only buffered for crash reports and invisible on a successful-but-slow or transient-503 startup.onStepcallback toHarnessDriverClient.spawn()/RuntimeSpawnOptionsso consumers can observe binary resolution, process spawn, API bind, and handshake retry progress.This came out of debugging a
relay local upfailure that surfaced only asFailed to start broker: Service Unavailablewith no indication of which startup phase had stalled.Test plan
npx vitest run packages/cli/src/cli/commands/core.test.ts packages/cli/src/cli/lib/broker-lifecycle.test.ts packages/cli/src/cli/lib/client-factory.test.ts packages/harness-driver/src— all passing, including a newup --verbose forwards the flag to createRelay and logs startup step markerstest.npx tsc --noEmit -p packages/cli/tsconfig.json— no new errors (pre-existing unrelated@relayfile/clienterrors persist onmain).npm --prefix packages/cli run lint— no new warnings/errors.