Proj/task cancellation

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 6m 49s —— View job

---

Claude PR Review – #680 (Task Cancellation)

• Read CLAUDE.md and PR diff
• Review new oclif cancel pipeline (cancel-task.ts, task-client.ts, query/curate/dream SIGINT wiring)
• Review daemon cancel path (task-router.ts, agent-cancel-listener.ts, agent-executor-error.ts, agent-pool.ts)
• Review agent-side cancel path (cipher-agent.ts, chat-session.ts)
• Review TUI cancel hooks (use-cancel-running-task-keybind.ts, cancel-task.ts, select-cancel-target.ts)
• Post inline issues + summary

---

Summary

Overall score: 4.5 / 5

This is a thoughtful, well-tested implementation of an end-to-end cancel pipeline that threads cleanly through the CLI ↔ daemon ↔ agent process boundaries. The design splits responsibilities correctly: oclif owns SIGINT and --cancel <id>, the daemon owns idempotency + queue-vs-agent routing + lifecycle hooks, the agent owns per-task AbortController fan-out across sessions, and the TUI owns the Ctrl+Q affordance. The 457-line integration test (test/integration/cancel-pipeline.test.ts) exercises the five real-world scenarios (running cancel, queued cancel, idempotent double-cancel, post-cancel follow-up, agent stays warm) and gives me high confidence the user-observable behavior is correct.

Strengths

• TaskCancelResponse semantics doc — the four-case matrix (queued-local / forward-to-agent / idempotent / not-found) is exactly the kind of upfront definition cancel APIs usually lack; both runCancelTask and the TUI helper honor it consistently. (praise comment)
• RESERVED CHORD WARNING in use-cancel-running-task-keybind.ts — clear, calls out the global interception risk, and points at the double-tap mitigation if it ever bites. (praise comment)
• Extracted handlers (agent-cancel-listener.ts, agent-executor-error.ts) — neat split that makes the agent-process dispatch logic unit-testable without forking a child.
• Durable idempotency via persistent history store — late retries across daemon restarts return success: true instead of misleading "Task not found." Nice follow-through.
• Strict TDD discipline — 13 new test files, including a 457-line integration spec, hit the project's TDD policy in CLAUDE.md.

Issues / suggestions (inline)

1. installSigintCancel swallows async rejections (comment) — client.request(...) returns a Promise; the try/catch only catches sync throws. An async rejection becomes an unhandledRejection (process-fatal on Node 22+ default). One-line fix.
2. handleTaskCancel durable-path I/O has no timeout (comment) — a slow store.getById can stall the cancel ACK indefinitely. Worth bounding.
3. Double-forward to agent on rapid retries (comment) — no cancelling status stamp, so duplicate cancels re-forward to the agent. Functionally idempotent (agent handles it) but chatty. Optional polish.
4. cancelTaskLocally passes pre-mutation task to hooks (comment) — hook sees TaskInfo without status: 'cancelled' / completedAt. Tiny consistency fix.
5. TUI helper throws on "Task not found" (comment) — documented-idempotent outcome treated as error. Worth clarifying in JSDoc or returning the response unchanged.
6. Test timeoutMs is unused (comment) — silently dropped across 8 scenarios. Nit.

None of these block landing. Issues 1 and 2 are the only ones with real production-impact potential; the rest are quality/consistency polish.
· Branch: proj/task-cancellation

[github-actions]

issue (bug): The try/catch only catches synchronous throws, but client.request(...) is declared to return a Promise (see WaitForTaskClient = Pick<ITransportClient, 'on' | 'onStateChange' | 'request'>). If the request rejects asynchronously (network blip, daemon disconnected mid-send), the promise becomes unhandled — which can crash Node 22+ via the default unhandledRejection behavior, defeating the whole point of "Ctrl-C should never make things worse than they were."

Suggest attaching a .catch so async rejections take the same logging path:

Suggested change

	try {
	deps.client.request(TaskEvents.CANCEL, {taskId: deps.taskId})
	} catch (error) {
	// Transport unavailable — the wait loop will time out or see a
	// disconnect event and surface that path instead. Log the swallowed
	// throw to stderr so a genuine bug (bad payload, transport regression)
	// leaves a breadcrumb instead of failing silently.
	const message = error instanceof Error ? error.message : String(error)
	process.stderr.write(`[brv] cancel request failed locally: ${message}\n`)
	}
	try {
	void Promise.resolve(deps.client.request(TaskEvents.CANCEL, {taskId: deps.taskId})).catch((error: unknown) => {
	const message = error instanceof Error ? error.message : String(error)
	process.stderr.write(`[brv] cancel request failed locally: ${message}\n`)
	})
	} catch (error) {
	// Transport unavailable — the wait loop will time out or see a
	// disconnect event and surface that path instead. Log the swallowed
	// throw to stderr so a genuine bug (bad payload, transport regression)
	// leaves a breadcrumb instead of failing silently.
	const message = error instanceof Error ? error.message : String(error)
	process.stderr.write(`[brv] cancel request failed locally: ${message}\n`)
	}

[github-actions]

suggestion (quality): The forward-to-agent path right below this comment does not stamp any "cancel-in-flight" marker on the live TaskInfo before returning. A rapid double-cancel (or a retried cancel request) will re-enter this branch and forward task:cancel to the agent a second time before the first cancel's terminal task:cancelled has been emitted.

handleAgentCancelEvent correctly returns silently on the second call, so this isn't a correctness bug — but it does mean an extra socket round-trip and an extra "task:cancel received" log line per duplicate.

Consider stamping this.tasks.set(taskId, {...task, status: 'cancelling'}) and short-circuiting subsequent cancels of an already-cancelling task with {success: true}. The fast-path in the no-task branch above (prior?.task.status === 'cancelled') already established a precedent for status-based idempotency.

Not blocking — scenario 4 in cancel-pipeline.test.ts already proves the user-observable outcome is one terminal event. Flagging in case you want to tighten the daemon↔agent chatter.

[github-actions]

nit (consistency): Line 623 stamps status: 'cancelled' on the in-memory entry, but the task argument forwarded to notifyHooksCancelled on line 625 is still the un-mutated reference captured by the caller — hooks see a TaskInfo without status: 'cancelled' and without completedAt. The TaskHistoryHook.onTaskCancelled builds its persisted entry from a separate code path so this doesn't break persistence, but a future hook that reads task.status directly would observe an inconsistent snapshot.

Trivial fix: forward the just-set value so the hook sees what it expects.

Suggested change

	this.tasks.set(taskId, {...task, completedAt: Date.now(), status: 'cancelled'})
	this.moveToCompleted(taskId)
	this.notifyHooksCancelled(taskId, task).catch(() => {})
	const cancelledTask = {...task, completedAt: Date.now(), status: 'cancelled' as const}
	this.tasks.set(taskId, cancelledTask)
	this.moveToCompleted(taskId)
	this.notifyHooksCancelled(taskId, cancelledTask).catch(() => {})

[github-actions]

praise: This RESERVED CHORD WARNING block is exactly the kind of why-this-exists doc that prevents accidental regressions. The fact that it explicitly calls out the global interception, the "no confirmation" risk, and the proposed double-tap mitigation path makes future debates much easier.

[github-actions]

praise: The semantics matrix on TaskCancelResponse.success is genuinely excellent — distinguishing "queued-for-forward" from "agent-confirmed" from "idempotent-retry" upfront removes the kind of "wait, what does true actually mean?" ambiguity that bites cancel APIs everywhere. The accompanying Callers must rely on the terminal event broadcast directive is the right escape hatch and is consistently honored by both runCancelTask (oclif) and useCancelRunningTaskKeybind (TUI).

[github-actions]

nit (consistency): This helper throws on !response.success, but the only caller (useCancelRunningTaskKeybind → .catch(() => {})) intentionally silences errors per its comment. That's a legitimate design choice, but it means:

1. The error: response.error ?? 'Cancel failed' message is built and immediately discarded — dead code at the integration point.
2. A "Task not found" idempotent retry (which the daemon doc-comments explicitly bless as expected) is treated as an error by this helper.

If you want to keep the throw semantics for future callers, that's fine — but consider noting in the JSDoc that the daemon's success: false for "Task not found" is a documented idempotent-retry outcome, not a failure, so future consumers don't wire it as user-visible error UI. Alternatively, return the full TaskCancelResponse unchanged and let each caller decide.

[github-actions]

nit: Multiple test cases pass timeoutMs: 5000 to waitForTaskCompletion, but WaitForTaskOptions (in src/oclif/lib/task-client.ts) does not declare a timeoutMs field — only the deprecated --timeout flag still exists in the command layer, and the heartbeat-driven stale check is the actual liveness signal now.

These extra keys are silently dropped (test tsconfig isn't strict about excess props on inline literals), so the tests pass — but they read as if there's a per-test override. Either drop the key throughout this file or thread it through to a real option to keep the test surface honest. Same pattern appears in scenarios at lines 95, 121, 147, 178, 206, 232, 259, 288.

[github-actions]

issue (resilience): This durable-idempotency path is unbounded — store.getById(taskId) runs without a timeout, and the daemon will hold the cancel request open until disk I/O completes. On a healthy filesystem this is microseconds, but clearCompleted running on the same project can compact the index concurrently, and task:list pass-2 already races against getById in handleTaskList (line 1466). A stuck store call here will hang every subsequent task:cancel from the same client because the wire layer serializes ACK responses per request id.

Two cheap improvements:

1. Race the getById against a bounded setTimeout (1–2s is generous) so a misbehaving store falls through to Task not found rather than blocking the response.
2. The catch block already logs and falls through to the not-found return — verify by reading: ✅ it does. So the bug is purely "no timeout," not "no recovery."

Not blocking — the integration test suite never exercises a slow store — but cheap insurance for production.

[github-actions]

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

---

Claude PR Review – #680 (Task Cancellation) — Re-review

• Read CLAUDE.md + diff against origin/main
• Check whether the 6 items from the last review were addressed
• Re-review the latest fix commits (cancelTaskLocally cache miss, durable idempotency, stale timeoutMs removal, JSON success flag flip)
• Spot-check anything new
• Post inline comments + updated summary

---

Summary (re-review)

Updated overall score: 4.5 / 5 — unchanged. The branch is ship-ready modulo two real issues (#1, #2 below) that are cheap to fix and have known production-impact paths.

What was addressed since the last review (5/6 acknowledged, 1/6 actually closed)

#	Item	Status
1	Async rejection in installSigintCancel	❌ Still open at src/oclif/lib/task-client.ts:204-213 — client.request(...) returns a Promise and is neither awaited nor .catch()-ed; the existing try/catch only handles synchronous throws
2	Unbounded store.getById on durable cancel path	❌ Still open at task-router.ts:683 — extensive comments were added but no timeout wrapper
3	Double-forward to agent on rapid cancels	❌ Still open — the forward branch (task-router.ts:717-720) does not stamp a cancelling status. Marked optional last time; still optional
4	cancelTaskLocally passes pre-mutation task to hooks	❌ Still open — task-router.ts:625 still forwards the original task argument; the in-memory map is mutated correctly via this.tasks.set(...) on line 623, but the hook callee gets the stale snapshot. Trivial fix
5	TUI helper throws on documented-idempotent "Task not found"	❌ Still open at src/tui/features/tasks/api/cancel-task.ts:23 — no JSDoc note added; behavior unchanged
6	Unused timeoutMs: 5000 in SIGINT tests	✅ Closed by 06b9c07

The follow-up commits did land valuable adjacent improvements that weren't on the original list — calling those out so they don't go unnoticed:

• acb2acc durable idempotency via the persistent task history store is the right fix for the cache-miss race after TASK_CLEANUP_GRACE_PERIOD_MS. Without it, late cancel retries would have returned a misleading "Task not found" even though the task was persisted as cancelled. The two new tests (returns success on a retry after completedTasks has aged out / still returns Task not found when the history store has a non-cancelled status) lock the contract down well.
• c6d77da aligning JSON success: false with exit code 130 removes the contradictory-signal problem (success: true with non-zero exit). The cancel state still lives in data.status: 'cancelled', so consumers that want to distinguish cancel-from-error still can. Right call.
• 45a0ff0 cancelTaskLocally cache miss correctly stamps status: 'cancelled' on the in-memory entry before moveToCompleted, which closes the race in the queued/no-agent cancel paths. Nice catch.
• TaskCancelResponse JSDoc in shared/transport/events/task-events.ts:56-71 now spells out all four cases of success. Worth keeping.
• runCancelBranchWithRetry inlined into curate/dream/query removed three identical 9-line wrappers — small DRY win.

New finding from this pass

7. agent-cancel-listener.ts:42 has the same async-rejection bug as Issue Feat/init #1, but with a sharper blast radius — transport.request(...) is fire-and-forget and not .catch()-ed, and agent-process.ts:942 installs unhandledRejection → process.exit(1). A transient transport hiccup while emitting the post-cancel echo would kill the agent child process. Inline comment with suggested fix.

What's still solid

• The 457-line cancel-pipeline.test.ts integration suite remains the strongest part of the PR — five real scenarios end-to-end.
• New unit coverage in task-router.test.ts for both the in-memory and durable idempotency branches is exactly the level of test discipline CLAUDE.md asks for.
• The RESERVED CHORD WARNING block on use-cancel-running-task-keybind.ts:12-26 is even better than before — it explicitly names the global-interception risk and points at the double-tap mitigation pattern.

Recommendation

Land it. Issues #1, #2, and the new #7 are the only items I'd want addressed in this branch (each is a 1–3 line change); #3, #4, #5 are polish that can ship as a follow-up. Two of three blockers are async-rejection patterns and share the same one-line fix shape — Promise.resolve(...).catch(...).
· Branch: proj/task-cancellation

[github-actions]

issue (bug): Same async-rejection class as Issue #1 from the previous review — but with a sharper consequence here. transport.request(...) is declared to return a Promise, but it's not awaited and isn't .catch()-ed. The surrounding try/catch only handles the synchronous throw path and the await agent.cancelTask(taskId) rejection; an async rejection from transport.request(...) escapes both.

That matters more on this path than on the SIGINT one because agent-process.ts:942 installs a process-level unhandledRejection handler that calls process.exit(1). So a transient transport hiccup while emitting the post-cancel task:cancelled echo would kill the whole agent child process — exactly the surface the cancel pipeline was built to keep cheap and isolated.

Cheap fix — make the fire-and-forget intent explicit and contain the failure:

Suggested change

	transport.request(TransportTaskEventNames.CANCELLED, {taskId})
	const cancelled = await agent.cancelTask(taskId)
	if (!cancelled) return
	Promise.resolve(transport.request(TransportTaskEventNames.CANCELLED, {taskId})).catch((emitError: unknown) => {
	const emitMessage = emitError instanceof Error ? emitError.message : String(emitError)
	log(`task:cancelled emit failed taskId=${taskId} err=${emitMessage}`)
	})
	} catch (error) {
	const message = error instanceof Error ? error.message : String(error)
	log(`task:cancel handler error taskId=${taskId} err=${message}`)
	}

Note: same fix is still needed at src/oclif/lib/task-client.ts:205 (Issue #1 from the prior review, still open on this commit).

[github-actions]

Claude finished @ncnthien's task in 3m 40s —— View job

---

Claude PR Review – #680 (Task Cancellation) — Re-review of latest commits

• Read CLAUDE.md and PR diff vs origin/main
• Re-check status of previously flagged issues (1–7)
• Spot-check the latest commits (WebUI cancel button, unified cancel state, pending feedback)
• Post inline comments + summary

---

Summary (re-review)

Overall score: 4.5 / 5 — unchanged. Branch remains ship-ready. The new WebUI cancel surface (commits 5a559ff + 98c0eb3) is well-shaped and well-tested; the few remaining items are quality/consistency polish, not blockers.

Status of items from the prior two reviews

#	Item	Status
1	installSigintCancel swallows async rejections (task-client.ts:204-213)	❌ Still open — client.request(...) returns a Promise, neither awaited nor .catch()-ed. The comment on lines 198-203 now explicitly acknowledges "wrapped in try/catch so a synchronously-throwing transport… doesn't propagate," which confirms the gap rather than closing it
2	Unbounded store.getById on durable cancel path (task-router.ts:683)	❌ Still open — extensive comments, no timeout wrapper
3	Double-forward to agent on rapid cancels (task-router.ts:717-720)	❌ Still open — optional polish
4	cancelTaskLocally passes pre-mutation task to hooks (task-router.ts:625)	❌ Still open — trivial fix
5	TUI helper throws on documented-idempotent "Task not found" (tui/.../cancel-task.ts:23)	❌ Still open
6	Unused timeoutMs: 5000 in SIGINT tests	✅ Closed by 06b9c07
7	agent-cancel-listener.ts:42 async-rejection (kills agent child via unhandledRejection → process.exit(1))	❌ Still open — transport.request(...) is fire-and-forget; agent-process.ts:947 confirmed process.exit(1) on unhandledRejection

Findings from this pass (the latest two commits)

5a559ff — WebUI cancel button on rows + detail header

8. WebUI cancelTask helper inherits the same idempotent footgun as the TUI helper (inline) — src/webui/features/tasks/api/cancel-task.ts:17 throws on success: false, including for Task not found which is contractually an idempotent retry. Sharper here than in the TUI because the single caller pipes the throw straight into toast.error(err.message) in task-list-view.tsx:264. Test on line 41 of cancel-task.test.ts explicitly locks in this behavior, so it's a deliberate choice — but worth at least a JSDoc note matching the TaskCancelResponse contract.

9. handleCancel in task-list-view.tsx:260-269 flickers Spinner → Cancel → Delete (inline) — clearing cancellingIds on onSuccess runs before the daemon's terminal task:cancelled event arrives (the forward-to-agent path returns success: true immediately). The row briefly renders the Cancel button again between ACK and terminal event. Fix: drive clearance from a useEffect on tasks watching for terminal status, or factor cancellingIds into rowActionKind. Not a correctness bug.

10. Detail header has no Delete action for terminal tasks (inline) — the row shows Cancel-or-Delete via rowActionKind, but task-detail-header.tsx only renders the Cancel button when isActive. A user in the detail sheet of a completed/cancelled/error task has no Delete affordance — they have to close the sheet and find the row. Minor symmetry nit.

98c0eb3 — unify cancel state across list and detail + pending feedback

• Lifting cancellingIds into TaskListView and threading cancelling={cancellingIds.has(selectedTaskId)} into TaskDetailView is exactly the right shape — the Sheet always sees the same per-task state the row sees. Clean fix.

What I'd ship as-is vs. what I'd want fixed before landing

Blocking-shaped (still 1–3 lines each):

• Feat/init #1 and feat(llm): add Ollama as local LLM provider #7 (async-rejection — same fix pattern: void Promise.resolve(...).catch(...))
• Feat/refactor structure #2 (cheap Promise.race against a 1–2s timeout on store.getById)

Polish that can ship as a follow-up:

• add the app cli #3, feat: update readme and contribuing guide #4, feat: Add MemAgent automatic chat+embedding storage and demo script #5, feat: add internal tools and storage #8, fix: test fail on this tools and storage #9, Feat/memory operation tool #10

What remains rock-solid

• The 457-line cancel-pipeline.test.ts integration suite — five real scenarios end-to-end.
• The TaskCancelResponse four-case JSDoc remains the strongest single piece of contract-shaping in this PR.
• The RESERVED CHORD WARNING block on use-cancel-running-task-keybind.ts.
• The new WebUI test files (row-action-kind.test.ts, cancel-task.test.ts) — small, fast, locked-in behavior.
• Mutually-exclusive row action column via rowActionKind is a clean design.

Recommendation

Land it after addressing Issues #1, #2, #7 (each is a one- to three-line change with a known fix pattern). The rest are quality polish.
· Branch: proj/task-cancellation

[github-actions]

issue (consistency / UX): This helper is the WebUI twin of src/tui/features/tasks/api/cancel-task.ts:23 and inherits the same documented-idempotent footgun: TaskCancelResponse.success: false with error: 'Task not found' is the expected outcome for late retries (see the TaskCancelResponse JSDoc in src/shared/transport/events/task-events.ts:56-71, which now explicitly enumerates the four cases). Throwing here turns that idempotent outcome into a red-toast user error at the only callsite (task-list-view.tsx:264 toast.error(err.message)).

Concrete scenario: a user clicks Cancel, the daemon forwards to the agent, the agent terminates, the row transitions to cancelled, then later the user pulls up history and clicks Cancel again on a stale row (or the table state lags by one tick) — the user sees a "Task not found" error toast for what is contractually a success.

Two reasonable fixes:

1. Treat success: false && error === 'Task not found' as idempotent success and return the response unchanged.
2. Return TaskCancelResponse unchanged and let the caller decide. The cancelMutation.mutate({onError, onSuccess}) shape in task-list-view.tsx already encodes that branching cleanly.

If you want to keep the throw semantics for future callers, at minimum add a JSDoc note matching the contract on TaskCancelResponse so future consumers don't wire success: false straight into user-visible error UI.

[github-actions]

suggestion (UX): Clearing cancellingIds on onSuccess causes a visible button flicker: Spinner → "Cancel task" button → Delete button.

The race:

1. User clicks Cancel — cancellingIds.add(taskId) runs, row shows LoaderCircle + title="Cancelling…".
2. Daemon ACKs {success: true} immediately (the forward-to-agent path at task-router.ts:720 returns before the agent confirms — see the JSDoc on TaskCancelResponse and the comment at task-router.ts:710-716).
3. onSuccess: clear runs. cancellingIds.has(taskId) is now false, but the in-store status is still 'started' (terminal event hasn't arrived yet), so rowActionKind(task.status) still returns 'cancel' → the row briefly renders the Cancel button again.
4. Eventually the agent emits task:cancelled, the store transitions to terminal, and the row switches to the Delete button.

Two cheap fixes:

1. Don't clear on onSuccess. Instead, drive cancel-state clearance from a useEffect watching tasks — when isTerminalStatus(task.status) becomes true for a tracked id, drop it from the Set. That keeps the spinner visible from click → terminal event, which is the user's mental model.
2. Or stamp the row visually as "cancelling" until it transitions: extend the rowActionKind decision to consider cancellingIds as well so the row always shows the spinner variant while a cancel is in flight, regardless of the daemon ACK.

Not a correctness bug, but the flicker is the kind of thing that erodes user trust in the Cancel button on low-latency local daemons.

[github-actions]

praise: Mutually-exclusive row-action column based on rowActionKind(task.status) is a clean design — Cancel for active tasks, Delete for terminal — and the extracted utility with dedicated tests (row-action-kind.test.ts) keeps the decision easy to evolve as new statuses are added.

The companion Spinner / Cancelling… title on CancelRowAction (line 265) is also a nice touch — it communicates intent without needing a separate toast on click.

[github-actions]

nit (consistency): The table row (task-list-table.tsx:212-218) shows a Delete action for terminal tasks via rowActionKind, but the detail header here only renders the Cancel button when isActive and shows nothing for terminal tasks — so a user who opened the detail sheet for a completed / error / cancelled task has to close the sheet and find the row again to delete it. The two surfaces should probably mirror each other.

Cheap follow-up: when isTerminal, render a Delete button in the same slot (or via the same rowActionKind decision) so the detail header and the row action stay symmetric. Not blocking — calling out so the asymmetry doesn't ossify.