feat: [ENG-2488] Wire resolveActiveProvider in daemon bootstrap

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 3m 43s —— View job

---

PR Review: feat: [ENG-2488] Wire resolveActiveProvider in daemon bootstrap

• Gather context and read changed files
• Review for correctness, security, and code quality
• Post summary with score

---

Summary

Overall score: 4/5

This is a focused, well-scoped change that wires resolveActiveProvider into the daemon bootstrap so the Web UI can display which provider/model handled each task. The design is intentionally best-effort — errors are swallowed by safeResolveActiveProvider in TaskRouter, so task dispatch is never blocked. A few minor concerns below.

---

Observations

suggestion — Per-task I/O overhead

// src/server/infra/daemon/brv-server.ts
async resolveActiveProvider() {
  const config = await resolveProviderConfig({
    authStateStore,
    providerConfigStore,
    providerKeychainStore,
    tokenRefreshManager,
  })
  ...
}

resolveProviderConfig does a providerConfigStore.read() (disk read) and potentially a keychain lookup or OAuth token refresh on every task creation. Since the callback is async and errors are swallowed (safeResolveActiveProvider), this never blocks dispatch — but it does add per-task I/O overhead. Worth considering a short-lived in-memory cache (e.g. keyed by activeProvider, TTL of a few seconds) to coalesce rapid bursts of task creation. Not a blocker given the fire-and-forget semantics, but worth tracking if task throughput grows.

---

nitpick — Repeated parameter construction

// GET_PROVIDER_CONFIG handler
resolveProviderConfig({authStateStore, providerConfigStore, providerKeychainStore, tokenRefreshManager})

// resolveActiveProvider callback (new)
resolveProviderConfig({authStateStore, providerConfigStore, providerKeychainStore, tokenRefreshManager})

The exact same 4-dependency object is constructed in two places in brv-server.ts. This is acceptable given the inline closure style used throughout, but if the params evolve, both spots need updating. A small shared helper like resolveCurrentProviderConfig() bound once in the outer scope would centralise this. Low priority.

Fix this →

---

nitpick — Redundant truthy guard on activeProvider

return {
  ...(config.activeModel ? {model: config.activeModel} : {}),
  ...(config.activeProvider ? {provider: config.activeProvider} : {}),
}

activeProvider is typed as string (non-optional in ProviderConfigResponse), so in practice it should never be an empty string unless the config is in a degraded state. The falsy guard is harmless and defensive, but worth noting that it deviates from the type contract. activeModel is string | undefined so its guard is fully correct.

---

praise — Failure isolation is correct

The comment "Failures are swallowed by TaskRouter's safeResolveActiveProvider — never blocks dispatch" correctly describes the fault-tolerance contract. Provider stamping is enrichment metadata, not critical path, so this is exactly the right design.

---

observation — No new tests in this PR

Given the repo's strict TDD policy, it's worth confirming that safeResolveActiveProvider in TaskRouter (on the base branch) has test coverage for the error-swallowing path, and that the resolveActiveProvider wiring in brv-server.ts is exercised by an integration test. If not, a follow-up test would be warranted.