feat: [ENG-2487] TaskRouter stamps provider + model on task:create

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 4m 10s —— View job

---

PR Review: feat: [ENG-2487] TaskRouter stamps provider + model on task:create

• Gather context and read changed files
• Analyze changes for correctness, security, and code quality
• Post inline comments (PR already merged — see general review below)
• Post general summary with overall score

---

Summary

Overall score: 4/5

The feature is well-structured and follows the codebase patterns. The fail-open design, conditional-await optimization, and test coverage are all solid. A few issues worth tracking for follow-up.

---

What changed (full scope, incl. files not listed in the PR header)

The PR actually touches 7 files across multiple layers:

File	Change
src/server/core/domain/transport/task-info.ts	Added model?: string, provider?: string to TaskInfo
src/server/core/domain/transport/schemas.ts	Added model/provider to TaskListItemSchema + TaskCreatedSchema
src/shared/transport/events/task-events.ts	Added model/provider to TaskListItem interface
src/server/infra/process/task-router.ts	resolveActiveProvider option, conditional await, safeResolveActiveProvider, toListItem update
src/server/infra/process/transport-handlers.ts	Pass-through of resolveActiveProvider
test/unit/infra/process/task-router.test.ts	6 new test cases for stamping behavior
test/unit/core/domain/transport/schemas.test.ts	New schema validation tests

---

Issues

src/server/infra/process/task-router.ts — Race window in idempotency guard

// Line ~499 in handleTaskCreate
const {model, provider} = this.resolveActiveProvider
  ? await this.safeResolveActiveProvider()
  : {}

this.tasks.set(taskId, { ... model, provider ... })

The idempotency guard runs at the top of handleTaskCreate, but the task isn't stored until after the await. A concurrent duplicate task:create for the same taskId (client retry during the resolver round-trip) would bypass the guard and create a second entry. This race window only exists when resolveActiveProvider is configured. Pre-existing code was synchronous between check and store.

Suggested fix: reserve the slot before the await, then patch in provider/model after:

// Store placeholder before any await (closes the idempotency race)
this.tasks.set(taskId, { clientId, content: data.content, createdAt: Date.now(), taskId, type: data.type, status: 'created' })

const {model, provider} = this.resolveActiveProvider ? await this.safeResolveActiveProvider() : {}

// Patch in resolved fields
const stored = this.tasks.get(taskId)
if (stored) {
  this.tasks.set(taskId, { ...stored, ...(model ? {model} : {}), ...(provider ? {provider} : {}) })
}

Fix this →

---

src/server/infra/process/task-router.ts — Conditional sync/async is a fragile invariant

The comment on the conditional await pattern explains a test-fragility problem rather than a production concern:

"an unconditional await would yield a microtask even on an immediately-resolved Promise, breaking tests that assert on broadcasts without awaiting the handler"

If the tests can't handle an async handler, the tests should be fixed (tests should await handler!(...)) rather than adding a production-code branch. As new contributors add features, this invariant may not be obvious and could lead to bugs.

---

src/server/core/domain/entities/provider-registry.ts — Unrelated, possibly unintended revert

- defaultModel: 'gemini-3-flash-preview',
+ defaultModel: 'gemini-2.5-flash',

Commit 4e70643 (directly preceding this PR's base) was titled "fix: [ENG-2469] update Google default model to gemini-3-flash-preview". This PR appears to revert that fix because proj/persis-task-history diverged from main before that commit landed. Verify this is intentional and not a stale-base merge artifact.

---

src/server/infra/process/task-router.ts — provider/model not in TaskExecute

The stamped fields are stored on TaskInfo and included in task:created / task:list, but NOT forwarded in the executeMsg: TaskExecute sent to the agent. If the task history persistence layer (the broader proj/persis-task-history feature) needs to know the active provider/model when processing the task, it can't retrieve it from the execute message alone. Looks intentional since the agent uses its own provider config — just worth confirming.

---

src/server/infra/process/task-router.ts — task:started broadcast omits model/provider

handleTaskStarted broadcasts content, clientCwd, files, taskId, and type to the project room — but not model or provider. Clients that join the project room after task:created but before task:started will see the started event without provider info and must correlate back to the created event. Document this if the intent is for clients to always use task:created as the source of truth for provider/model.

---

Praise

• Fail-open: safeResolveActiveProvider wraps the resolver in try/catch and returns {} on failure. Dispatch is never blocked. Good.
• Test coverage: 6 behavioral tests + 4 schema tests cover the full matrix (external provider, byterover internal, no resolver, empty resolver, resolver rejects, task:list round-trip). Strong.
• Type safety: TaskInfo, TaskListItemSchema, TaskCreatedSchema, and TaskListItem are all updated consistently. No any or unsafe casts.
• Type-only imports: All new type imports correctly use import type.