From fd3f15c9154bbf1b1ae76ec638f430181a261425 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 17:33:21 +0000 Subject: [PATCH] fix(todo): signal removal intent so agent stops rebuilding cleared todos The /todo rm reminder was a generic "user manually modified" message showing an empty list, so the model read the cleared list as missing and re-populated it on the next turn. buildSystemReminder now emits an explicit do-not-recreate / do-not-re-add directive for removals while keeping status mutations (done/drop) neutral. Fixes #5258 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../controllers/todo-command-controller.ts | 32 +++++++----- .../todo-command-controller.test.ts | 51 ++++++++++++++++++- 3 files changed, 72 insertions(+), 15 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 87cd5f7e2..2e8571b22 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the agent rebuilding a todo list the user just cleared with `/todo rm`: the manual-edit system reminder now states removal intent ("intentionally cleared/removed … Do NOT recreate/re-add") so the model stops re-populating the list without an explicit request. ([#5258](https://github.com/can1357/oh-my-pi/issues/5258)) + ## [16.4.6] - 2026-07-12 ### Added diff --git a/packages/coding-agent/src/modes/controllers/todo-command-controller.ts b/packages/coding-agent/src/modes/controllers/todo-command-controller.ts index 9df53fa87..33715655a 100644 --- a/packages/coding-agent/src/modes/controllers/todo-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/todo-command-controller.ts @@ -116,16 +116,18 @@ function findTaskFuzzy(phases: TodoPhase[], query: string): { task: TodoItem; ph // Build system reminder // ============================================================================= -function buildSystemReminder(action: string, phases: TodoPhase[]): string { +function buildSystemReminder(action: string, phases: TodoPhase[], removed = false): string { const md = phases.length === 0 ? "(empty)" : phasesToMarkdown(phases).trimEnd(); - return [ - "", - `The user manually modified the todo list (${action}).`, - "Current todo list:", - "", - md, - "", - ].join("\n"); + const lines = ["", `The user manually modified the todo list (${action}).`]; + if (removed) { + lines.push( + phases.length === 0 + ? "The user intentionally cleared the todo list. Do NOT recreate or re-populate it unless the user explicitly asks; continue the current request without a todo list." + : "The user intentionally removed the entries no longer shown below. Do NOT re-add them unless the user explicitly asks.", + ); + } + lines.push("Current todo list:", "", md, ""); + return lines.join("\n"); } export class TodoCommandController { @@ -366,7 +368,7 @@ export class TodoCommandController { const current = this.#currentPhases(); const trimmed = rest.trim(); if (!trimmed) { - this.#commit([], "/todo rm (all)"); + this.#commit([], "/todo rm (all)", { removed: true }); this.ctx.showStatus("Cleared all todos."); return; } @@ -377,7 +379,7 @@ export class TodoCommandController { this.ctx.showError(errors.join("; ")); return; } - this.#commit(phases, `/todo rm ${taskHit.task.content}`); + this.#commit(phases, `/todo rm ${taskHit.task.content}`, { removed: true }); this.ctx.showStatus(`Removed: ${taskHit.task.content}`); return; } @@ -388,7 +390,7 @@ export class TodoCommandController { this.ctx.showError(errors.join("; ")); return; } - this.#commit(phases, `/todo rm ${phaseHit.name}`); + this.#commit(phases, `/todo rm ${phaseHit.name}`, { removed: true }); this.ctx.showStatus(`Removed phase: ${phaseHit.name}`); return; } @@ -454,7 +456,7 @@ export class TodoCommandController { } } - #commit(nextPhases: TodoPhase[], action: string): void { + #commit(nextPhases: TodoPhase[], action: string, opts?: { removed?: boolean }): void { // 1. In-memory + UI state this.ctx.session.setTodoPhases(nextPhases); this.ctx.setTodos(nextPhases); @@ -463,7 +465,9 @@ export class TodoCommandController { this.ctx.sessionManager.appendCustomEntry(USER_TODO_EDIT_CUSTOM_TYPE, { phases: nextPhases }); // 3. Inject system reminder so the agent learns about the change next turn. - const reminderText = buildSystemReminder(action, nextPhases); + // Removals carry explicit intent so the agent does not rebuild the + // cleared/removed items on its next turn (issue #5258). + const reminderText = buildSystemReminder(action, nextPhases, opts?.removed ?? false); const message = { role: "developer" as const, content: [{ type: "text" as const, text: reminderText }], diff --git a/packages/coding-agent/test/modes/controllers/todo-command-controller.test.ts b/packages/coding-agent/test/modes/controllers/todo-command-controller.test.ts index c5171c1db..513e3e1a0 100644 --- a/packages/coding-agent/test/modes/controllers/todo-command-controller.test.ts +++ b/packages/coding-agent/test/modes/controllers/todo-command-controller.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from "bun:test"; +import { afterEach, describe, expect, it, type Mock, vi } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -152,4 +152,53 @@ describe("TodoCommandController", () => { expect(ctx.showError).toHaveBeenCalledWith(expect.stringContaining("Failed to write todos:")); expect(ctx.showError).toHaveBeenCalledWith(expect.stringContaining("internal scheme")); }); + + function reminderTextFrom(ctx: InteractiveModeContext): string { + const appendMessage = ctx.agent.appendMessage as unknown as Mock<(message: unknown) => void>; + const message = appendMessage.mock.calls[0][0] as { content: Array<{ text: string }> }; + return message.content[0].text; + } + + it("tells the model not to recreate the list after /todo rm (all)", async () => { + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "pi-tui-todo-rm-all-")); + const phases: TodoPhase[] = [ + { name: "Foundation", tasks: [{ content: "Scaffold crate", status: "in_progress" }] }, + ]; + const ctx = createContext(tempRoot, phases); + const controller = new TodoCommandController(ctx); + + await controller.handleTodoCommand("rm"); + + expect(ctx.agent.appendMessage).toHaveBeenCalledTimes(1); + const text = reminderTextFrom(ctx); + expect(text).toContain("intentionally cleared the todo list"); + expect(text).toMatch(/Do NOT recreate/i); + }); + + it("tells the model not to re-add a removed phase after /todo rm ", async () => { + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "pi-tui-todo-rm-phase-")); + const phases: TodoPhase[] = [ + { name: "Foundation", tasks: [{ content: "Scaffold crate", status: "completed" }] }, + { name: "Auth", tasks: [{ content: "Port credential store", status: "pending" }] }, + ]; + const ctx = createContext(tempRoot, phases); + const controller = new TodoCommandController(ctx); + + await controller.handleTodoCommand("rm Auth"); + + expect(reminderTextFrom(ctx)).toMatch(/Do NOT re-add them/i); + }); + + it("keeps status-mutation reminders neutral (no do-not-recreate directive)", async () => { + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "pi-tui-todo-done-")); + const phases: TodoPhase[] = [ + { name: "Foundation", tasks: [{ content: "Scaffold crate", status: "in_progress" }] }, + ]; + const ctx = createContext(tempRoot, phases); + const controller = new TodoCommandController(ctx); + + await controller.handleTodoCommand("done"); + + expect(reminderTextFrom(ctx)).not.toMatch(/Do NOT/i); + }); });