feat: [ENG-2857] surface port-in-use error in brv webui

[ncnthien]

Summary

• Replace the generic Failed to get web UI port. Use \brv restart`...with a specific message that points the user atbrv webui --port when the Web UI port (default 7700) is held by another process. Same guidance applies whether the conflict shows up at daemon boot (webui:getPort) or mid-run via --port (webui:setPort`).
• New WebUiPortInUseError thrown by WebUiServer on EADDRINUSE. Daemon catches it, tracks the failure, and propagates a typed reason over the transport instead of swallowing or throwing a plain Error.
• New src/shared/transport/events/webui-events.ts with WebuiEvents constant and discriminated-union response types replacing the inline {port?: number} shapes — TypeScript now narrows the CLI branches automatically.
• Daemon log entries tightened so user-actionable guidance (--port, BRV_WEBUI_PORT) lives only in the CLI message — single source of truth.

Test plan

• npm run typecheck — passes
• npx eslint src/ — 0 errors on touched files
• npx mocha "test/unit/core/domain/errors/webui-error.test.ts" "test/unit/infra/webui/**/*.test.ts" — 21 passing
• Manual: hog 127.0.0.1:7700, run ./bin/run.js webui → boot-conflict message fires
• Manual: hog 127.0.0.1:9090, run ./bin/run.js webui -p 9090 → setPort-conflict message fires
• Manual: recovery — after boot conflict, brv webui --port <free> clears the failure state
• Manual: regression — daemon spawn failure still routes through formatConnectionError (verified with BRV_IAM_BASE_URL='' ./bin/run.js webui)

[github-actions]

Claude finished @ncnthien's task in 5m 18s —— View job

---

Claude PR Review — feat: [ENG-2857] surface port-in-use error in brv webui

• Read PR diff and understand scope
• Review src/server/core/domain/errors/webui-error.ts
• Review src/server/infra/webui/webui-server.ts
• Review src/server/infra/daemon/brv-server.ts
• Review src/oclif/commands/webui.ts
• Review src/shared/transport/events/webui-events.ts
• Review tests
• Post inline comments for findings
• Post summary with score

---

Summary

Overall: 4 / 5 — Solid, well-scoped UX improvement with proper typed errors and a clean discriminated-union transport contract. One real bug in the setPort failure path (misleading log + stale state) and a handful of small consistency/design polish items, all called out inline.

Highlights (what's good)

• praise: Typed WebUiPortInUseError carrying the conflicting port cleanly replaces ad-hoc string matching at the daemon boundary, with handlers in brv-server.ts translating it into a transport-friendly discriminated response.
• praise: Splitting webui-events.ts out of inline {port?: number} shapes is the right move — single source of truth, narrowing works, and the file slots neatly into AllEventGroups.
• praise: Tests cover the new behavior end-to-end (port-in-use rejection, double-start guard, idempotent stop) and the error class is unit-tested.
• praise: User-actionable guidance (--port, BRV_WEBUI_PORT) lives only in the CLI message — daemon logs stay terse. Good single-source-of-truth discipline.

Findings (see inline comments)

#	File	Severity	Issue
1	src/server/infra/daemon/brv-server.ts:660–672	issue	setPort stops the previous webui server before attempting the new port; on port_in_use the log says "keeping previous configuration" but in reality the daemon is now left with no Web UI running. Plus webuiBootFailure is not cleared on non-EADDRINUSE failures, causing stale port_in_use responses to later getPort calls.
2	src/server/infra/webui/webui-server.ts:32	nit	"Already running" branch throws a plain Error while EADDRINUSE throws WebUiPortInUseError — inconsistent with the typed-error story.
3	src/oclif/commands/webui.ts:56–65	suggestion (style)	resolvePortOrExit doesn't use return this.error(...) like the rest of the file. Also: not_started branch routes the user to brv restart with no log breadcrumb.
4	src/shared/transport/events/webui-events.ts:6–15	suggestion (design)	Implicit discriminant ('port' in result) is fragile; a status: 'ok' | 'port_in_use' | 'not_started' field would survive future success variants.
5	test/unit/infra/webui/webui-server.test.ts:38–66	nit	WebUiServer doesn't clear this.httpServer after the EADDRINUSE rejection — leaked handle until GC. Worth fixing in webui-server.ts and asserting the reset in this test.

Other notes (no inline)

• Pre-existing: setupFeatureHandlers({ webuiPort: webuiServer?.getPort() }) at brv-server.ts:733 snapshots the port at startup, so a successful webui:setPort doesn't propagate the new port into PushHandler/StatusHandler review URLs. Not introduced by this PR — flagging for follow-up.
• The 'error' listener on httpServer (webui-server.ts:38) isn't removed after listen() resolves; post-startup TCP errors will hit a Promise that's already settled. Pre-existing, harmless today, mentioned for completeness.

Verification

• Inspected the diff against origin/main. Confirmed no other internal call sites consume webui:getPort / webui:setPort strings (only the event constants in webui-events.ts), so the response-shape change is safe.
• Did not run typecheck/lint/tests in this review (read-only review).
  — Branch: feat/ENG-2857

[github-actions]

Claude finished @ncnthien's task in 4m 25s —— View job

---

Claude PR Review — feat: [ENG-2857] surface port-in-use error in brv webui (round 2)

• Read PR diff (commits since prior review)
• Verify prior findings addressed (status discriminator, typed already-running, stale state)
• Re-review webui-error.ts
• Re-review webui-server.ts (httpServer reset, already-running error type)
• Re-review brv-server.ts (setPort stale-state + log fix)
• Re-review webui.ts (status discriminator usage)
• Re-review webui-events.ts (discriminator design)
• Re-review tests
• Post inline comments + summary

---

Summary

Overall: 4 / 5 — Round 1 feedback is almost fully addressed: explicit status discriminator is in, WebUiServerAlreadyRunningError replaces the plain Error, the misleading "keeping previous configuration" log is now accurate, and webuiBootFailure no longer goes stale on non-EADDRINUSE failures. One real regression introduced by the round-1 leak fix in webui-server.ts, and the deeper UX concern on setPort (probe-first) is still outstanding. Score holds at 4 because the new regression is a fairly narrow edge case.

What was addressed (prior round)

• praise (resolved): WebUiServerAlreadyRunningError now thrown in the "already running" branch — typed-error story is consistent (webui-server.ts:32).
• praise (resolved): resolvePortOrExit now uses return this.error(...) uniformly across all three branches (webui.ts:57–64). Reads cleanly.
• praise (resolved): Explicit status: 'ok' | 'port_in_use' | 'not_started' discriminator replaces the implicit 'port' in result narrowing (webui-events.ts:6–15). Survives future success variants.
• praise (resolved, half): Round-1 finding on setPort — misleading keeping previous configuration log is now accurate (Web UI unavailable), and webuiBootFailure is cleared on both success and non-EADDRINUSE failure so getPort no longer returns stale boot-time port info (brv-server.ts:660–675).
• praise (resolved): Tests updated for WebUiServerAlreadyRunningError; new webui-error.test.ts covers the typed errors.

Findings this round (see inline comments)

#	File	Severity	Issue
1	src/server/infra/webui/webui-server.ts:39	issue (correctness, blocking)	The new this.httpServer = undefined line lives inside a persistent 'error' listener. Post-startup errors will null httpServer while running=true, wedging stop() (which guards on !this.httpServer). Gate the clear behind a resolved flag, or use once('error', ...) for the startup phase only.
2	src/server/infra/daemon/brv-server.ts:661–672	suggestion (non-blocking)	Partial follow-up from round-1. Stale-state + log fixes are in, but the "probe new port before stopping old server" half wasn't picked up. Users running brv webui --port <busy> still lose their working Web UI on the previous port. Probe-first is strictly better; happy to defer.

Other notes (no inline, pre-existing)

• setupFeatureHandlers({webuiPort: webuiServer?.getPort()}) at brv-server.ts:734 still snapshots the port at startup. A successful webui:setPort does not propagate the new port into PushHandler / StatusHandler review URLs. Same call site as round 1 — flagging for follow-up, not introduced here.

Verification

• Inspected the round-2 diff (commit 7045f28) and the current state of every changed file. Searched for any remaining inline webui:getPort / webui:setPort string literals outside webui-events.ts — none. Searched for stale 'port' in result narrowings — none.
• Did not run typecheck / lint / test suite (read-only review). PR description states all three pass on the author's machine.
  — Branch: feat/ENG-2857

[github-actions]

Claude finished @ncnthien's task in 1m 57s —— View job

---

Claude PR Review — feat: [ENG-2857] surface port-in-use error in brv webui (round 3)

• Read latest diff (commit 3b3896d)
• Verify round-2 wedge fix in webui-server.ts (resolved-flag gating)
• Verify new unit test asserts post-startup 'error' does NOT clear httpServer
• Re-check brv-server.ts setPort path and other touched files for regressions
• Post summary

---

Summary

Overall: 5 / 5 — Round-2 blocking issue is cleanly resolved. Score bumps from 4 → 5 because all blockers are now addressed and only the explicitly-deferred UX item remains (probe-then-stop in setPort, which both sides agreed is a follow-up).

What was addressed (round 2)

• praise (resolved): The 'error' listener at webui-server.ts:39 is now gated behind a resolved flag that flips inside listen()'s callback. Post-startup TCP errors no longer null httpServer while running=true, so stop() is no longer wedged. Inverted the EADDRINUSE branch into a cleaner ternary — nice tidy-up.

  this.httpServer.on('error', (err: NodeJS.ErrnoException) => {
    if (resolved) return
    this.httpServer = undefined
    reject(err.code === 'EADDRINUSE' ? new WebUiPortInUseError(port) : err)
  })
  
  this.httpServer.listen(port, TRANSPORT_HOST, () => {
    resolved = true
    ...
  })

• praise (resolved): New unit test at webui-server.test.ts:85–100 asserts the exact scenario from the round-2 finding: after start() resolves, emitting 'error' on the underlying HttpServer does not clear httpServer, isRunning() stays true, getPort() is unchanged, and stop() still completes cleanly. Matches the requested coverage 1:1.

Findings this round

None blocking. One small observation, no inline:

• With the gated listener, post-startup server-level errors are now silently swallowed (the listener returns early, no logging). For the HttpServer-level 'error' event this is rare and acceptable (per-connection errors fire elsewhere), and round 2 explicitly called this trade-off out as fine. If observability into late server errors ever matters, a single log() call before return would do it — not a blocker for this PR.

Carried-over (acknowledged out of scope, no action needed)

• brv webui --port <busy> still tears down the working server before probing the new port. Both reviewer and author agreed to defer to a follow-up; the L667 log now states reality, so the current behavior is at least honest.
• setupFeatureHandlers({webuiPort: webuiServer?.getPort()}) snapshot at brv-server.ts:734 — pre-existing, not introduced here.

Verification

• Inspected round-3 diff (commit 3b3896d) and current state of every changed file. Confirmed:
  • webui-events.ts properly registered in AllEventGroups (events/index.ts:95).
  • setPort handler (brv-server.ts:660–680) correctly clears webuiBootFailure on success (L675) and on non-port failure (L671), and sets it on port-in-use (L666).
  • resolvePortOrExit uses result.status === 'ok' narrowing consistently (webui.ts:57).
• Did not run typecheck / lint / tests in this review (read-only). PR description states all pass on the author's machine.

LGTM — ready to ship.
— Branch: feat/ENG-2857