feat: [ENG-1540] style localhost OAuth callback page to match brv brand

[cuongdo-byterover]

The local HTTP callback served after brv login was responding with plain text on a default white page, which clashed with the brv dark theme. Replace it with self-contained HTML that mirrors the design spec (Figma: CLI-focused, node 9584-10575): black background with a soft green/warm radial glow, faint grid overlay, BYTEROVER wordmark, and a centered card carrying a status icon, heading, and supporting copy. Adds a matching error variant that shows the OAuth error message in a red-tinted code block.

Summary

• Problem: After brv login, the localhost OAuth callback rendered plain text on a default white background — visually jarring against the rest of brv's dark surface.
• Why it matters: It's the first impression after authenticating with the CLI. Reported via design review; tracked in ENG-1540.
• What changed: The /callback route on the localhost OAuth callback server now responds with self-contained HTML (success + error variants) styled to match the Figma spec. Plus a firstQueryParam guard that coerces Express's string | string[] | ParsedQs | ParsedQs[] query values to a single string, and full unit coverage of both helpers and the three response variants.
• What did NOT change: Token exchange, OAuth state validation, the redirect-URI contract, the /callback route shape, env vars, or any other CLI surface. Auto-closing the tab is also out of scope (browser security restriction — see Risks).

Type of change

• Bug fix
• New feature
• Refactor (no behavior change)
• Documentation
• Test
• Chore (build, dependencies, CI)

Scope (select all touched areas)

• TUI / REPL
• Agent / Tools
• LLM Providers
• Server / Daemon
• Shared (constants, types, transport events)
• CLI Commands (oclif)
• Hub / Connectors
• Cloud Sync
• CI/CD / Infra

Linked issues

• Closes ENG-1540

Root cause (bug fixes only, otherwise write N/A)

• Root cause: src/server/infra/http/callback-server.ts responded to GET /callback with res.status(200).send('Authentication successful. You can close this window.') — plain text against the browser default white. No theming, no brand, no logo.
• Why this was not caught earlier: The localhost handoff page was added to the OAuth flow before the rest of the brv surface adopted dark theming; nobody loops back through this view often, so the visual mismatch went unaddressed until raised in design review.

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s): test/unit/infra/http/callback-server.test.ts
• Key scenario(s) covered:
  • escapeHtml — five metachars, safe-text passthrough, ampersand-first ordering (avoids double-escaping), realistic XSS payload
  • firstQueryParam — string / array of strings / empty array / non-string-first array / object / number / null / undefined
  • GET /callback success → 200, Content-Type: text/html, body contains Authentication Successful + BYTEROVER
  • GET /callback with error + error_description → 400, body contains Authentication Failed + the escaped description
  • GET /callback with <script> in error_description → escaped entities present, raw tag absent
  • GET /callback with missing state → 400, body contains Missing code or state parameter
  • All 21 tests in the suite pass.

User-visible changes

• After a successful brv login, the browser tab now shows a dark, branded confirmation page instead of plain text on white.
• After a failed OAuth round-trip, the same dark layout is shown with the provider's error message rendered safely (HTML-escaped) inside a red-tinted code block.
• The page no longer attempts to auto-close: browsers refuse window.close() on tabs they did not open themselves, so the script was a silent no-op. The copy directs the user to close manually, which is the correct affordance.

Evidence

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint)
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format
• Documentation updated (if applicable) — N/A
• No breaking changes (or clearly documented above)
• Branch is up to date with main

Risks and mitigations

• Risk: HTML/CSS is inlined in the response; future design tweaks require a CLI release.
  • Mitigation: Trade-off is explicit. Alternative is redirecting to a hosted web-app page; deferred because the localhost flow is light-weight and works offline. Easy to swap later — res.send(SUCCESS_HTML) becomes res.redirect(...).
• Risk: Inline <style> on the response would be blocked if a strict CSP is ever added to this localhost server.
  • Mitigation: Today the localhost callback server has no CSP. If one is added later, the page should be served from a hosted URL anyway, which removes the inline-style concern.

[github-actions]

Claude finished @cuongdo-byterover's task in 1m 14s —— View job

---

PR Review: feat: [ENG-1540] Style localhost OAuth callback page

• Read changed files
• Analyze for security, correctness, and code quality
• Post review

---

Summary

The change replaces a plain-text HTTP response with a self-contained, branded HTML page for the OAuth callback. The implementation is clean and well-structured. XSS is correctly handled via the escapeHtml helper, and no external resources are fetched (fully self-contained). However, there are mandatory issues per repo standards.

Overall score: 3 / 5

---

Issues

blocker — Missing tests (src/server/infra/http/callback-server.ts)

Per CLAUDE.md, TDD is non-negotiable: "You MUST follow Test-Driven Development. This is non-negotiable." This PR adds a new escapeHtml function and new HTML rendering paths with no test coverage. At minimum:

• escapeHtml should have unit tests covering the five escape characters (&, <, >, ", ') and passthrough for safe input.
• The GET /callback route behavior for error, error_description, and code/state should be covered (the HTML response body, HTTP status codes).

Fix this →

---

issue — req.query values can be arrays (src/server/infra/http/callback-server.ts:233-237)

Express allows repeated query parameters (e.g. ?error=foo&error=bar), so req.query.error and req.query.error_description are typed as string | string[] | ParsedQs | ParsedQs[]. The current code applies String(errorMessage) which, for an array, produces "foo,bar" — functionally harmless here, but it bypasses type safety. A type guard keeps the intent explicit:

// suggestion
function firstQueryParam(val: unknown): string | undefined {
  if (typeof val === 'string') return val
  if (Array.isArray(val) && typeof val[0] === 'string') return val[0]
  return undefined
}

Then use firstQueryParam(error_description) ?? firstQueryParam(error) ?? 'Unknown error' to avoid a potential "undefined" string reaching errorHtml.

Fix this →

---

suggestion — Inline <script> and CSP future-proofing (src/server/infra/http/callback-server.ts:122)

<script>setTimeout(() => window.close(), 2500);</script>

window.close() is fine for OAuth flows (the tab is browser-opened). However, the inline script would be blocked if a Content-Security-Policy header is ever added to this server. If CSP becomes a requirement, this will silently stop working. A comment noting this would prevent confusion, or the script could be moved to an external inline data: approach. Low priority but worth keeping in mind as the security posture of the callback server evolves.

---

Positives

• nitpick (praise) — escapeHtml is correct and thorough: all five HTML special characters are escaped using replaceAll, and it's applied consistently everywhere user-controlled content (error_description, error) appears in the HTML output. No XSS risk.
• praise — Fully self-contained HTML: no external fonts, CDN stylesheets, or images are fetched. This is important for a localhost page that may run without internet access.
• praise — SUCCESS_HTML is a module-level constant (no dynamic content, no function call overhead per request). errorHtml is correctly a function only where dynamic content is needed. Clean separation.
• praise — The escapeHtml function is pure and well-isolated — easy to unit-test once tests are added.

---

[github-actions]

Claude finished @cuongdo-byterover's task in 1m 28s —— View job

---

PR Review: feat: [ENG-1540] Style localhost OAuth callback page

• Read changed files
• Analyze for security, correctness, and code quality
• Post review

---

Summary

Both blockers from the previous review are now resolved: escapeHtml and firstQueryParam both have thorough unit tests, and firstQueryParam is properly implemented and wired into setupRoutes. The implementation is clean, XSS-safe, and fully self-contained. One new issue introduced by the tests will fail npm run typecheck.

Overall score: 4 / 5

---

Issues

issue — firstQueryParam() called with no arguments will fail typecheck (test/unit/infra/http/callback-server.test.ts:205)

expect(firstQueryParam()).to.equal(undefined)

The function signature is value: unknown — required, not optional. TypeScript strict mode will reject a call with zero arguments:

error TS2554: Expected 1 arguments, but got 0.

Fix option A (preferred): make the parameter explicitly optional in the implementation so callers can forward possibly-undefined values directly:

// src/server/infra/http/callback-server.ts
export function firstQueryParam(value?: unknown): string | undefined {

Fix option B: pass undefined explicitly in the test:

expect(firstQueryParam(undefined)).to.equal(undefined)

Option A is more ergonomic — it matches the documented intent of handling untrusted/potentially-absent wire values and avoids needing to write firstQueryParam(req.query.error) through undefined literals.

Fix this →

---

nitpick — Pre-existing timeout test is a no-op (test/unit/infra/http/callback-server.test.ts:51-59)

try {
  server.waitForCallback('test-state', 100)  // not awaited
} catch (error) { ... }

The returned Promise is never awaited, so the rejection will never be caught by the try/catch. The test always "passes" without exercising any assertion. This is pre-existing and out of scope for this PR, but worth noting for a follow-up.

---

Positives

• praise — Both blockers from the first review are addressed cleanly. escapeHtml tests cover all five metacharacters, double-escape order, passthrough, and a realistic XSS payload — that's more thorough than the minimum asked for.
• praise — firstQueryParam tests cover the full type space Express can produce: string, string[], [], [{nested}], plain object, number, null, and undefined. Excellent defensive coverage.
• praise — The four callback HTML responses tests cover every branch in setupRoutes (success 200, error 400, missing-params 400, XSS escaping) and assert both HTTP status and body content.
• praise — The CSP note comment (``) is exactly the right approach: non-intrusive, self-documenting, and leaves a clear breadcrumb if CSP headers are ever added.
• praise — firstQueryParam JSDoc accurately describes the Express type surface and the defensive rationale. Helpful for future maintainers.

---

[github-actions]

Claude finished @cuongdo-byterover's task in 1m 56s —— View job

---

PR Review: feat: [ENG-1540] Style localhost OAuth callback page

• Read changed files
• Analyze for correctness, security, and code quality
• Post review

---

Summary

All issues from the two previous review rounds are now resolved. The remaining typecheck failure (firstQueryParam() with no args) has been fixed by making the parameter optional (value?: unknown), and the inline auto-close <script> was dropped entirely in the latest commit — which is even better than the suggested CSP comment approach, since there are now zero inline scripts to worry about.

Overall score: 5 / 5

---

Changes since last review

Fixed — firstQueryParam parameter is now value?: unknown (src/server/infra/http/callback-server.ts:119). The no-argument call at test/unit/infra/http/callback-server.test.ts:205 now passes TypeScript strict mode cleanly.

Dropped — The setTimeout(() => window.close(), 2500) script was removed (commit bfe1e3a). This eliminates the inline-script/CSP concern noted in the previous review entirely. The success page copy ("You can now safely close this tab") remains accurate for manual close.

---

Remaining nitpick (pre-existing, out of scope)

nitpick — The timeout test at test/unit/infra/http/callback-server.test.ts:51-59 still does not await the Promise, so the catch block is unreachable and the test never exercises the timeout path. This is pre-existing and not introduced by this PR.

---

Positives

• praise — firstQueryParam now correctly typed as value?: unknown, matching the documented intent of forwarding possibly-absent wire values directly from req.query.* without needing explicit undefined at call sites.
• praise — Dropping the auto-close script rather than just commenting it out is the cleaner call: zero inline scripts means the page is fully CSP-compatible without any further work.
• praise — escapeHtml and firstQueryParam unit tests are thorough and cover the full type surface Express can produce; the integration-style callback response tests hit every branch in setupRoutes. Coverage is well above the 80% minimum.
• praise — The eslint-disable camelcase comment was removed after error_description was moved from a destructured variable to a property access — correct, since the camelcase rule doesn't flag dot-notation accesses on external objects.

---