fix: preserve todo metrics and line-change display

Match todos by subtaskId first to retain tokens/cost/added/removed when
content or derived IDs change. Improve UI rendering to suppress
misleading +0/−0 while running unless values are explicitly provided.

Relates to #5376

src/core/tools/UpdateTodoListTool.ts

@@ -32,7 +32,12 @@ export class UpdateTodoListTool extends BaseTool<"update_todo_list"> {
 			const historyHasMetadata =
 				Array.isArray(previousFromHistory) &&
 				previousFromHistory.some(
-					(t) => t?.subtaskId !== undefined || t?.tokens !== undefined || t?.cost !== undefined,
+					(t) =>
+						t?.subtaskId !== undefined ||
+						t?.tokens !== undefined ||
+						t?.cost !== undefined ||
+						t?.added !== undefined ||
+						t?.removed !== undefined,
 				)
 
 			const previousTodos: TodoItem[] =
@@ -77,6 +82,8 @@ export class UpdateTodoListTool extends BaseTool<"update_todo_list"> {
 				subtaskId: t.subtaskId,
 				tokens: t.tokens,
 				cost: t.cost,
+				added: t.added,
+				removed: t.removed,
 			}))
 
 			const approvalMsg = JSON.stringify({
@@ -215,45 +222,55 @@ function normalizeStatus(status: string | undefined): TodoStatus {
 }
 
 /**
- * Preserve metadata (subtaskId, tokens, cost) from previous todos onto next todos.
+ * Preserve metadata (subtaskId, tokens, cost, added, removed) from previous todos onto next todos.
  *
  * Matching strategy (in priority order):
- * 1. **ID match**: If both todos have an `id` field and they match exactly, preserve metadata.
+ * 1. **Subtask ID match**: If the next todo has a `subtaskId`, match against previous todos with
+ *    the same `subtaskId`. This is the most stable identifier when content (and derived IDs)
+ *    changes.
+ * 2. **ID match**: If both todos have an `id` field and they match exactly, preserve metadata.
  *    This handles the common case where ID is stable across updates.
- * 2. **Content match with position awareness**: For todos without matching IDs, fall back to
- *    content-based matching. Duplicates are matched in order (first unmatched previous with
- *    same content gets matched to first unmatched next with same content).
+ * 3. **Content match with position awareness**: For todos without matching subtask IDs or IDs,
+ *    fall back to content-based matching. Duplicates are matched in order (first unmatched
+ *    previous with same content gets matched to first unmatched next with same content).
  *
- * This approach ensures metadata survives status changes (which can alter the derived ID)
+ * This approach ensures metadata survives status/content changes (which can alter the derived ID)
  * and handles duplicates deterministically.
  */
 function preserveTodoMetadata(nextTodos: TodoItem[], previousTodos: TodoItem[]): TodoItem[] {
 	const safePrevious = previousTodos ?? []
 	const safeNext = nextTodos ?? []
 
-	// Build ID -> todo mapping for O(1) lookup
-	const previousById = new Map<string, TodoItem>()
-	for (const prev of safePrevious) {
-		if (prev?.id && typeof prev.id === "string") {
-			// Only store the first occurrence for each ID (handle duplicates deterministically)
-			if (!previousById.has(prev.id)) {
-				previousById.set(prev.id, prev)
-			}
-		}
-	}
-
 	// Track which previous todos have been used (by their index) to avoid double-matching
 	const usedPreviousIndices = new Set<number>()
 
-	// Build content -> queue mapping for fallback (content-based matching)
-	// Each queue entry includes the original index for tracking
+	// Build lookup maps for matching strategies.
+	// - Subtask ID: may have duplicates; match in order for determinism.
+	// - ID: should be unique; store first occurrence.
+	// - Content: may have duplicates; match in order for determinism.
+	const previousBySubtaskId = new Map<string, Array<{ todo: TodoItem; index: number }>>()
+	const previousById = new Map<string, { todo: TodoItem; index: number }>()
 	const previousByContent = new Map<string, Array<{ todo: TodoItem; index: number }>>()
+
 	for (let i = 0; i < safePrevious.length; i++) {
 		const prev = safePrevious[i]
-		if (!prev || typeof prev.content !== "string") continue
-		const list = previousByContent.get(prev.content)
-		if (list) list.push({ todo: prev, index: i })
-		else previousByContent.set(prev.content, [{ todo: prev, index: i }])
+		if (!prev) continue
+
+		if (typeof prev.subtaskId === "string") {
+			const list = previousBySubtaskId.get(prev.subtaskId)
+			if (list) list.push({ todo: prev, index: i })
+			else previousBySubtaskId.set(prev.subtaskId, [{ todo: prev, index: i }])
+		}
+
+		if (typeof prev.id === "string" && !previousById.has(prev.id)) {
+			previousById.set(prev.id, { todo: prev, index: i })
+		}
+
+		if (typeof prev.content === "string") {
+			const list = previousByContent.get(prev.content)
+			if (list) list.push({ todo: prev, index: i })
+			else previousByContent.set(prev.content, [{ todo: prev, index: i }])
+		}
 	}
 
 	return safeNext.map((next) => {
@@ -262,19 +279,29 @@ function preserveTodoMetadata(nextTodos: TodoItem[], previousTodos: TodoItem[]):
 		let matchedPrev: TodoItem | undefined = undefined
 		let matchedIndex: number | undefined = undefined
 
-		// Strategy 1: Try ID-based matching first (most reliable)
-		if (next.id && typeof next.id === "string") {
-			const byId = previousById.get(next.id)
-			if (byId) {
-				// Find the index of this todo in the original array
-				const idx = safePrevious.findIndex((p) => p === byId)
-				if (idx !== -1 && !usedPreviousIndices.has(idx)) {
-					matchedPrev = byId
-					matchedIndex = idx
+		// Strategy 0: Try subtaskId-based matching first (most stable across content/ID changes)
+		if (typeof next.subtaskId === "string") {
+			const candidates = previousBySubtaskId.get(next.subtaskId)
+			if (candidates) {
+				for (const candidate of candidates) {
+					if (!usedPreviousIndices.has(candidate.index)) {
+						matchedPrev = candidate.todo
+						matchedIndex = candidate.index
+						break
+					}
 				}
 			}
 		}
 
+		// Strategy 1: Try ID-based matching first (most reliable)
+		if (!matchedPrev && next.id && typeof next.id === "string") {
+			const byId = previousById.get(next.id)
+			if (byId && !usedPreviousIndices.has(byId.index)) {
+				matchedPrev = byId.todo
+				matchedIndex = byId.index
+			}
+		}
+
 		// Strategy 2: Fall back to content-based matching if ID didn't match
 		if (!matchedPrev && typeof next.content === "string") {
 			const candidates = previousByContent.get(next.content)
@@ -298,6 +325,8 @@ function preserveTodoMetadata(nextTodos: TodoItem[], previousTodos: TodoItem[]):
 				subtaskId: next.subtaskId ?? matchedPrev.subtaskId,
 				tokens: next.tokens ?? matchedPrev.tokens,
 				cost: next.cost ?? matchedPrev.cost,
+				added: next.added ?? matchedPrev.added,
+				removed: next.removed ?? matchedPrev.removed,
 			}
 		}
 

src/core/tools/__tests__/updateTodoListTool.spec.ts

@@ -292,4 +292,259 @@ describe("UpdateTodoListTool.execute", () => {
 			}),
 		)
 	})
+
+	it("should treat added/removed as metadata and prefer history todos when present", async () => {
+		const md = "[ ] Task 1"
+
+		const previousFromMemory = parseMarkdownChecklist(md)
+		const previousFromHistory: TodoItem[] = previousFromMemory.map((t) => ({
+			...t,
+			added: 10,
+			removed: 3,
+		}))
+
+		const task = {
+			todoList: previousFromMemory,
+			clineMessages: [
+				{
+					type: "ask",
+					ask: "tool",
+					text: JSON.stringify({ tool: "updateTodoList", todos: previousFromHistory }),
+				},
+			],
+			consecutiveMistakeCount: 0,
+			recordToolError: vi.fn(),
+			didToolFailInCurrentTurn: false,
+			say: vi.fn(),
+		} as any
+
+		const tool = new UpdateTodoListTool()
+		await tool.execute({ todos: md }, task, {
+			pushToolResult: vi.fn(),
+			handleError: vi.fn(),
+			askApproval: vi.fn().mockResolvedValue(true),
+			removeClosingTag: vi.fn(),
+			toolProtocol: "xml",
+		})
+
+		expect(task.todoList).toHaveLength(1)
+		expect(task.todoList[0]).toEqual(
+			expect.objectContaining({
+				content: "Task 1",
+				added: 10,
+				removed: 3,
+			}),
+		)
+	})
+
+	it("should preserve metadata by subtaskId even when content (and derived id) changes", async () => {
+		// This test simulates the "user edited todo list" flow. The tool re-applies metadata
+		// after approval; subtaskId should be used as the primary match when content/id changes.
+		const md = "[ ] Old text"
+
+		const previousFromMemory: TodoItem[] = parseMarkdownChecklist(md).map((t) => ({
+			...t,
+			subtaskId: "subtask-1",
+			tokens: 123,
+			cost: 0.01,
+			added: 10,
+			removed: 3,
+		}))
+
+		const task = {
+			todoList: previousFromMemory,
+			clineMessages: [],
+			consecutiveMistakeCount: 0,
+			recordToolError: vi.fn(),
+			didToolFailInCurrentTurn: false,
+			say: vi.fn(),
+		} as any
+
+		// Simulate user-edited todo list with updated content and a different id, but the same subtaskId.
+		const userEditedTodos: TodoItem[] = [
+			{
+				id: "new-id",
+				content: "New text",
+				status: "completed",
+				subtaskId: "subtask-1",
+				// tokens/cost/added/removed intentionally omitted to verify preservation
+			},
+		]
+
+		const tool = new UpdateTodoListTool()
+		await tool.execute({ todos: md }, task, {
+			pushToolResult: vi.fn(),
+			handleError: vi.fn(),
+			askApproval: vi.fn().mockImplementation(async () => {
+				setPendingTodoList(userEditedTodos)
+				return true
+			}),
+			removeClosingTag: vi.fn(),
+			toolProtocol: "xml",
+		})
+
+		expect(task.todoList).toHaveLength(1)
+		expect(task.todoList[0]).toEqual(
+			expect.objectContaining({
+				id: "new-id",
+				content: "New text",
+				status: "completed",
+				subtaskId: "subtask-1",
+				tokens: 123,
+				cost: 0.01,
+				added: 10,
+				removed: 3,
+			}),
+		)
+	})
+
+	it("should preserve added/removed through normalization", async () => {
+		const md = "[x] Task 1"
+
+		const previousFromMemory: TodoItem[] = parseMarkdownChecklist("[ ] Task 1").map((t) => ({
+			...t,
+			added: 10,
+			removed: 3,
+		}))
+
+		const task = {
+			todoList: previousFromMemory,
+			clineMessages: [],
+			consecutiveMistakeCount: 0,
+			recordToolError: vi.fn(),
+			didToolFailInCurrentTurn: false,
+			say: vi.fn(),
+		} as any
+
+		const tool = new UpdateTodoListTool()
+		await tool.execute({ todos: md }, task, {
+			pushToolResult: vi.fn(),
+			handleError: vi.fn(),
+			askApproval: vi.fn().mockResolvedValue(true),
+			removeClosingTag: vi.fn(),
+			toolProtocol: "xml",
+		})
+
+		expect(task.todoList).toHaveLength(1)
+		expect(task.todoList[0]).toEqual(
+			expect.objectContaining({
+				content: "Task 1",
+				status: "completed",
+				added: 10,
+				removed: 3,
+			}),
+		)
+	})
+
+	it("should not cross-contaminate metadata when no subtaskId is present", async () => {
+		const initialMd = "[ ] Task 1\n[ ] Task 2"
+		const md = "[x] Task 1\n[ ] Task 2" // status changes for Task 1 -> derived id changes
+
+		const previousFromMemory: TodoItem[] = parseMarkdownChecklist(initialMd).map((t) =>
+			t.content === "Task 1"
+				? { ...t, tokens: 111, cost: 0.11, added: 11, removed: 1 }
+				: { ...t, tokens: 222, cost: 0.22, added: 22, removed: 2 },
+		)
+
+		const task = {
+			todoList: previousFromMemory,
+			clineMessages: [],
+			consecutiveMistakeCount: 0,
+			recordToolError: vi.fn(),
+			didToolFailInCurrentTurn: false,
+			say: vi.fn(),
+		} as any
+
+		const tool = new UpdateTodoListTool()
+		await tool.execute({ todos: md }, task, {
+			pushToolResult: vi.fn(),
+			handleError: vi.fn(),
+			askApproval: vi.fn().mockResolvedValue(true),
+			removeClosingTag: vi.fn(),
+			toolProtocol: "xml",
+		})
+
+		expect(task.todoList).toHaveLength(2)
+
+		const task1 = task.todoList.find((t: TodoItem) => t.content === "Task 1")
+		const task2 = task.todoList.find((t: TodoItem) => t.content === "Task 2")
+
+		expect(task1).toEqual(
+			expect.objectContaining({
+				content: "Task 1",
+				status: "completed",
+				tokens: 111,
+				cost: 0.11,
+				added: 11,
+				removed: 1,
+			}),
+		)
+
+		expect(task2).toEqual(
+			expect.objectContaining({
+				content: "Task 2",
+				status: "pending",
+				tokens: 222,
+				cost: 0.22,
+				added: 22,
+				removed: 2,
+			}),
+		)
+	})
+
+	it("should not preserve metadata when content changes and there is no subtaskId", async () => {
+		const initialMd = "[ ] Task 1\n[ ] Task 2"
+		const md = "[x] Task 1 (updated)\n[ ] Task 2"
+
+		const previousFromMemory: TodoItem[] = parseMarkdownChecklist(initialMd).map((t) =>
+			t.content === "Task 1"
+				? { ...t, tokens: 111, cost: 0.11, added: 11, removed: 1 }
+				: { ...t, tokens: 222, cost: 0.22, added: 22, removed: 2 },
+		)
+
+		const task = {
+			todoList: previousFromMemory,
+			clineMessages: [],
+			consecutiveMistakeCount: 0,
+			recordToolError: vi.fn(),
+			didToolFailInCurrentTurn: false,
+			say: vi.fn(),
+		} as any
+
+		const tool = new UpdateTodoListTool()
+		await tool.execute({ todos: md }, task, {
+			pushToolResult: vi.fn(),
+			handleError: vi.fn(),
+			askApproval: vi.fn().mockResolvedValue(true),
+			removeClosingTag: vi.fn(),
+			toolProtocol: "xml",
+		})
+
+		expect(task.todoList).toHaveLength(2)
+
+		const updated = task.todoList.find((t: TodoItem) => t.content === "Task 1 (updated)")
+		const task2 = task.todoList.find((t: TodoItem) => t.content === "Task 2")
+
+		expect(updated).toEqual(
+			expect.objectContaining({
+				content: "Task 1 (updated)",
+				status: "completed",
+			}),
+		)
+		expect(updated?.tokens).toBeUndefined()
+		expect(updated?.cost).toBeUndefined()
+		expect(updated?.added).toBeUndefined()
+		expect(updated?.removed).toBeUndefined()
+
+		expect(task2).toEqual(
+			expect.objectContaining({
+				content: "Task 2",
+				status: "pending",
+				tokens: 222,
+				cost: 0.22,
+				added: 22,
+				removed: 2,
+			}),
+		)
+	})
 })

webview-ui/src/components/chat/TodoListDisplay.tsx

@@ -105,7 +105,8 @@ export function TodoListDisplay({ todos, subtaskDetails, onSubtaskClick }: TodoL
 			{!isCollapsed && (
 				<ul ref={ulRef} className="list-none max-h-[300px] overflow-y-auto mt-2 -mb-1 pb-0 px-2 cursor-default">
 					{todos.map((todo, idx: number) => {
-						const icon = getTodoIcon(todo.status as TodoStatus)
+						const todoStatus = (todo.status as TodoStatus) ?? "pending"
+						const icon = getTodoIcon(todoStatus)
 						const isClickable = Boolean(todo.subtaskId && onSubtaskClick)
 						const subtaskById =
 							subtaskDetails && todo.subtaskId
@@ -115,16 +116,33 @@ export function TodoListDisplay({ todos, subtaskDetails, onSubtaskClick }: TodoL
 						const displayCost = todo.cost ?? subtaskById?.cost
 						const shouldShowCost = typeof displayTokens === "number" && typeof displayCost === "number"
 
-						const displayAdded = todo.added ?? subtaskById?.added
-						const displayRemoved = todo.removed ?? subtaskById?.removed
-						const hasValidSubtaskLink = typeof todo.subtaskId === "string" && todo.subtaskId.length > 0
-						const shouldShowLineChanges =
-							hasValidSubtaskLink && (Number.isFinite(displayAdded) || Number.isFinite(displayRemoved))
+						const todoAddedIsFinite = typeof todo.added === "number" && Number.isFinite(todo.added)
+						const todoRemovedIsFinite = typeof todo.removed === "number" && Number.isFinite(todo.removed)
 
-						const hasAdded =
-							typeof displayAdded === "number" && Number.isFinite(displayAdded) && displayAdded > 0
-						const hasRemoved =
-							typeof displayRemoved === "number" && Number.isFinite(displayRemoved) && displayRemoved > 0
+						const displayAdded = todoAddedIsFinite ? todo.added : subtaskById?.added
+						const displayRemoved = todoRemovedIsFinite ? todo.removed : subtaskById?.removed
+
+						const displayAddedIsFinite = typeof displayAdded === "number" && Number.isFinite(displayAdded)
+						const displayRemovedIsFinite =
+							typeof displayRemoved === "number" && Number.isFinite(displayRemoved)
+						const hasValidSubtaskLink = typeof todo.subtaskId === "string" && todo.subtaskId.length > 0
+
+						// Upstream aggregation may coerce missing stats to 0.
+						// To avoid showing misleading `+0/−0` for in-progress/pending rows,
+						// only render 0 while running if it was explicitly provided on the todo itself.
+						const canRenderAdded =
+							displayAddedIsFinite &&
+							(todoStatus === "completed" || displayAdded !== 0 || todoAddedIsFinite)
+						const canRenderRemoved =
+							displayRemovedIsFinite &&
+							(todoStatus === "completed" || displayRemoved !== 0 || todoRemovedIsFinite)
+
+						const shouldShowLineChanges = hasValidSubtaskLink && (canRenderAdded || canRenderRemoved)
+
+						const isAddedPositive = canRenderAdded && (displayAdded as number) > 0
+						const isRemovedPositive = canRenderRemoved && (displayRemoved as number) > 0
+						const isAddedZero = canRenderAdded && displayAdded === 0
+						const isRemovedZero = canRenderRemoved && displayRemoved === 0
 
 						return (
 							<li
@@ -132,8 +150,8 @@ export function TodoListDisplay({ todos, subtaskDetails, onSubtaskClick }: TodoL
 								ref={(el) => (itemRefs.current[idx] = el)}
 								className={cn(
 									"font-light flex flex-row gap-2 items-start min-h-[20px] leading-normal mb-2",
-									todo.status === "in_progress" && "text-vscode-charts-yellow",
-									todo.status !== "in_progress" && todo.status !== "completed" && "opacity-60",
+									todoStatus === "in_progress" && "text-vscode-charts-yellow",
+									todoStatus !== "in_progress" && todoStatus !== "completed" && "opacity-60",
 								)}>
 								{icon}
 								<span
@@ -161,16 +179,18 @@ export function TodoListDisplay({ todos, subtaskDetails, onSubtaskClick }: TodoL
 												<span
 													className={cn(
 														" text-right",
-														hasAdded ? "font-medium text-vscode-charts-green" : "",
+														isAddedPositive ? "font-medium text-vscode-charts-green" : "",
+														isAddedZero ? "opacity-50" : "",
 													)}>
-													{hasAdded ? `+${displayAdded}` : "\u00A0"}
+													{canRenderAdded ? `+${displayAdded}` : "\u00A0"}
 												</span>
 												<span
 													className={cn(
 														" text-right",
-														hasRemoved ? "font-medium text-vscode-charts-red" : "",
+														isRemovedPositive ? "font-medium text-vscode-charts-red" : "",
+														isRemovedZero ? "opacity-50" : "",
 													)}>
-													{hasRemoved ? `−${displayRemoved}` : "\u00A0"}
+													{canRenderRemoved ? `−${displayRemoved}` : "\u00A0"}
 												</span>
 											</span>
 										)}

webview-ui/src/components/chat/__tests__/TodoListDisplay.spec.tsx

@@ -206,6 +206,117 @@ describe("TodoListDisplay", () => {
 			expect(screen.getByText("−9")).toBeInTheDocument()
 		})
 
+		it("shows +0/−0 for completed subtask when fallback metrics are explicitly zero", () => {
+			const todosMissingDirectLineChanges = [
+				{
+					id: "1",
+					content: "Task 1: Zero changes",
+					status: "completed",
+					subtaskId: "subtask-1",
+				},
+			]
+			const subtaskDetailsWithZeroLineChanges: SubtaskDetail[] = [
+				{
+					id: "subtask-1",
+					name: "Task 1: Zero changes",
+					tokens: 1,
+					cost: 0.01,
+					added: 0,
+					removed: 0,
+					status: "completed",
+					hasNestedChildren: false,
+				},
+			]
+
+			render(
+				<TodoListDisplay
+					todos={todosMissingDirectLineChanges}
+					subtaskDetails={subtaskDetailsWithZeroLineChanges}
+				/>,
+			)
+
+			// Expand
+			const header = screen.getByText("1 to-dos done")
+			fireEvent.click(header)
+
+			const addedEl = screen.getByText("+0")
+			const removedEl = screen.getByText("−0")
+
+			expect(addedEl).toBeInTheDocument()
+			expect(removedEl).toBeInTheDocument()
+
+			// Zero values should be visually muted (not green/red emphasized)
+			expect(addedEl.className).toContain("opacity-50")
+			expect(addedEl.className).not.toContain("text-vscode-charts-green")
+
+			expect(removedEl.className).toContain("opacity-50")
+			expect(removedEl.className).not.toContain("text-vscode-charts-red")
+		})
+
+		it("in-progress: does not show +0/−0 when zeros only come from fallback", () => {
+			const todosMissingDirectLineChanges = [
+				{
+					id: "1",
+					content: "Task 1: Zero changes (running)",
+					status: "in_progress",
+					subtaskId: "subtask-1",
+				},
+			]
+			const subtaskDetailsWithZeroLineChanges: SubtaskDetail[] = [
+				{
+					id: "subtask-1",
+					name: "Task 1: Zero changes (running)",
+					tokens: 1,
+					cost: 0.01,
+					added: 0,
+					removed: 0,
+					status: "active",
+					hasNestedChildren: false,
+				},
+			]
+
+			render(
+				<TodoListDisplay
+					todos={todosMissingDirectLineChanges}
+					subtaskDetails={subtaskDetailsWithZeroLineChanges}
+				/>,
+			)
+
+			// Expand
+			const header = screen.getByText("Task 1: Zero changes (running)")
+			fireEvent.click(header)
+
+			expect(screen.queryByText("+0")).not.toBeInTheDocument()
+			expect(screen.queryByText("−0")).not.toBeInTheDocument()
+		})
+
+		it("in-progress: shows +0/−0 when explicitly present on todo", () => {
+			const todosWithDirectLineChanges = [
+				{
+					id: "1",
+					content: "Task 1: Zero changes (explicit)",
+					status: "in_progress",
+					subtaskId: "subtask-1",
+					added: 0,
+					removed: 0,
+				},
+			]
+
+			render(<TodoListDisplay todos={todosWithDirectLineChanges} subtaskDetails={subtaskDetails} />)
+
+			// Expand
+			const header = screen.getByText("Task 1: Zero changes (explicit)")
+			fireEvent.click(header)
+
+			const addedEl = screen.getByText("+0")
+			const removedEl = screen.getByText("−0")
+			expect(addedEl).toBeInTheDocument()
+			expect(removedEl).toBeInTheDocument()
+
+			expect(addedEl.className).toContain("opacity-50")
+			expect(removedEl.className).toContain("opacity-50")
+		})
+
 		it("falls back to subtaskDetails when todo added/removed are missing", () => {
 			const todosMissingDirectLineChanges = [
 				{