feat: [ENG-2499] Daemon handlers (list paginated, get, delete, bulk, …

[bao-byterover]

No description provided.

[github-actions]

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

---

PR Review — feat: [ENG-2499] Daemon handlers (list paginated, get, delete, bulk, clear)

• Gather context and read changed files
• Review src/server/core/domain/transport/schemas.ts
• Review src/server/infra/process/task-router.ts
• Review src/server/infra/process/transport-handlers.ts
• Review src/shared/transport/events/task-events.ts
• Review src/server/infra/daemon/brv-server.ts
• Review src/webui/features/tasks/hooks/use-task-subscriptions.ts
• Review test/integration/infra/process/task-router-extended.test.ts
• Post inline comments
• Post final summary with score

---

Summary

Overall score: 3 / 5

Good structural work — the DIP-injected getTaskHistoryStore factory, graceful degradation when the store is absent, and the broadcast-on-delete pattern are all sound. The test suite is the highlight: real FileTaskHistoryStore on a tmpdir with full lifecycle coverage, including stale-recovery, store-error fallback, and broadcast verification.

That said there are two correctness bugs, one performance issue, and two smaller concerns that should be addressed before merge.

---

Bugs (must fix)

#	Location	Issue
1	task-router.ts:1149 + task-router.ts:142	type filter is silently dropped — data.type is never destructured, never passed to store.list(), and matchesListFilters has no typeFilter parameter. The field is declared in the schema and the interface but has zero effect at runtime. See inline comments for the three-point fix.
2	task-router.ts:1157–1161	store.list() called without limit — all persisted entries are loaded from disk before the page slice is applied. ITaskHistoryStore.list already accepts limit; pass it through to avoid O(N) reads.

Issues (should fix)

#	Location	Issue
3	task-router.ts:1190	Cursor tie-breaking — nextCursor = paged.at(-1)?.createdAt is not unique. Two tasks sharing the same millisecond timestamp straddle the page boundary; the second is silently excluded on the next fetch. Use createdAt - 1 as the cursor or document the known gap.
4	task-events.ts:84	error.name promoted to required — a type-level breaking change. Serialised entries written before this commit (no name field) will now be structurally non-conformant. Either add a normalisation step in FileTaskHistoryStore.getById or keep the field optional and default at call sites.

Nitpicks / minor

#	Location	Note
5	task-router.ts:1055–1059	deletedCount in handleTaskDeleteBulk counts every success: true, including "not found" idempotent deletes. The semantics would be clearer as "actually removed" count.
6	task-router.ts:1016–1022	status !== undefined guard is dead code — TaskInfo.status is always initialised to 'created' on construction.
7	task-router.ts:154–221	synthesizeEntryFromTaskInfo intentionally mirrors TaskHistoryHook logic; the comment flags the drift risk. A task-history-entry-builder.ts extraction (mentioned in the comment) would eliminate this class of skew — worth scheduling soon.

What's good

• Clean DIP via getTaskHistoryStore?: (projectPath) => ITaskHistoryStore — pre-M2.09 tests are fully unaffected.
• broadcastTaskDeleted correctly skips when projectPath is undefined; broadcast-on-clear correctly dedupes in-memory + store IDs with a Set.
• handleTaskDelete refusal for non-terminal in-memory tasks prevents deleting a running agent's task out from under it.
• Test file is production-quality: real disk I/O, merge-wins verification, stale-recovery override, store-error fallback, idempotency, and broadcast assertions all covered.

[github-actions]

bug: type filter is silently dropped

data.type is never destructured here, so the newly-added type?: string[] filter has no effect even though it is declared in the schema and the interface.

matchesListFilters also has no typeFilter parameter, and store.list() is called without type. You need all three:

Suggested change

	const {before, status: statusFilter} = data
	const limit = data.limit ?? DEFAULT_TASK_LIST_LIMIT
	const {before, status: statusFilter, type: typeFilter} = data

Then pass typeFilter into matchesListFilters and into the store.list() call (type: typeFilter).

[github-actions]

bug: type filter missing from matchesListFilters

The signature has no typeFilter parameter, so even after handleTaskList is fixed to extract data.type, the per-item check will not apply it to in-memory entries.

Suggested change

	filters: {before?: number; projectFilter: string; statusFilter?: TaskListItemStatus[]},
	function matchesListFilters(
	item: TaskListItem,
	filters: {before?: number; projectFilter: string; statusFilter?: TaskListItemStatus[]; typeFilter?: string[]},
	): boolean {

Add a corresponding guard in the body:

if (filters.typeFilter && filters.typeFilter.length > 0 && !filters.typeFilter.includes(item.type)) return false

[github-actions]

performance: store.list() is called without a limit, loading unbounded data

The limit is only applied after merging all persisted entries with all in-memory entries. For a project with thousands of history entries this eagerly reads everything from disk before discarding most of it.

Pass a ceiling to the store so it can short-circuit early. A safe upper bound is limit + in-memory count, but even just limit is better than nothing:

Suggested change

	persisted = await store.list({
	projectPath: projectFilter,
	...(before === undefined ? {} : {before}),
	...(persistedStatuses === undefined ? {} : {status: persistedStatuses}),
	})
	persisted = await store.list({
	projectPath: projectFilter,
	...(before === undefined ? {} : {before}),
	...(persistedStatuses === undefined ? {} : {status: persistedStatuses}),
	...(typeFilter === undefined ? {} : {type: typeFilter}),
	limit,
	})

(ITaskHistoryStore.list already accepts limit.)

[github-actions]

issue: cursor is not unique — same-createdAt entries will be skipped across pages

nextCursor is the raw createdAt of the last item on the page. If a client uses this as before on the next request, any entry with the exact same timestamp as the cursor is excluded (item.createdAt >= before drops it). In practice two tasks can share the same millisecond timestamp (especially in tests or rapid automation), causing silent data loss across pages.

Consider using nextCursor = paged.at(-1)!.createdAt - 1 so the boundary is exclusive by one millisecond, or document the known limitation clearly.

[github-actions]

issue: deletedCount overcounts — idempotent "not found" deletes report success: true

handleTaskDelete always returns {success: true} for a missing task (idempotent by design). That means deletedCount here is really "number of taskIds that were not refused due to non-terminal status", not "number of entries actually removed". A caller passing 10 already-deleted IDs gets deletedCount: 10.

If the contract is "how many were actually live", count wasInMemory || wasLive instead, or return a richer response from handleTaskDelete (e.g. {deleted: true, success: true} vs {deleted: false, success: true}).

[github-actions]

nitpick: status !== undefined guard is dead code

TaskInfo.status defaults to 'created' at construction (line 850), so it is always defined for any task in this.tasks. The outer guard if (liveTask !== undefined) ensures a task exists, so status is always a TaskListItemStatus here.

Removing the !== undefined check makes the intent clearer and the TypeScript types more accurate:

Suggested change

	// Refusal: non-terminal in-memory tasks must not be deleted out from under the agent.
	const liveTask = this.tasks.get(taskId)
	if (liveTask !== undefined) {
	const {status} = liveTask
	if (status !== undefined && !TERMINAL_STATUSES.has(status)) {
	return {error: `cannot delete task in status '${status}'`, success: false}
	}
	if (liveTask !== undefined) {
	const {status} = liveTask
	if (!TERMINAL_STATUSES.has(status ?? 'created')) {
	return {error: `cannot delete task in status '${status}'`, success: false}
	}
	}

[github-actions]

issue: making error.name required is a breaking wire-format change

Any TaskListItem serialised before this commit (on-disk history entries, cached API responses, existing clients) that omits the error.name field will now fail TypeScript strict-mode checks on the reading side. The Zod schema in schemas.ts (TaskListItemStatusSchema) does not enforce error.name, so runtime parsing won't catch the mismatch — only TypeScript will.

If name must be required, add a migration step (e.g. normalise to 'UnknownError' on read in FileTaskHistoryStore.getById). Otherwise revert to name?: string and use a non-null assertion or default at call sites that actually need it.