diff --git a/tests/tools/test_todo_tool.py b/tests/tools/test_todo_tool.py index 87ff577979..d09f9a28fe 100644 --- a/tests/tools/test_todo_tool.py +++ b/tests/tools/test_todo_tool.py @@ -187,3 +187,79 @@ class TestTodoStoreBounds: for i in range(5000) ]) assert len(store.read()) == MAX_TODO_ITEMS + + def test_normal_list_is_unchanged(self): + """No regression: ordinary plans pass through untouched (no marker, + same content, same order).""" + store = TodoStore() + store.write([ + {"id": "1", "content": "write the report", "status": "in_progress"}, + {"id": "2", "content": "review PR", "status": "pending"}, + ]) + items = store.read() + assert [i["content"] for i in items] == ["write the report", "review PR"] + assert "[truncated]" not in items[0]["content"] + + +class TestRejectEmptyContent: + """Empty or missing content must fail the call instead of substituting + '(no description)'. See #44496.""" + + def test_write_rejects_empty_content(self): + store = TodoStore() + try: + store.write([{"id": "1", "content": "", "status": "pending"}]) + assert False, "should have raised ValueError" + except ValueError as e: + assert "empty or missing content" in str(e) + + def test_write_rejects_missing_content_key(self): + store = TodoStore() + try: + store.write([{"id": "2", "status": "pending"}]) + assert False, "should have raised ValueError" + except ValueError as e: + assert "empty or missing content" in str(e) + + def test_write_rejects_whitespace_content(self): + store = TodoStore() + try: + store.write([{"id": "3", "content": " ", "status": "pending"}]) + assert False, "should have raised ValueError" + except ValueError as e: + assert "empty or missing content" in str(e) + + def test_todo_tool_returns_error_on_empty_content(self): + result = json.loads(todo_tool( + todos=[{"id": "1", "content": "", "status": "pending"}], + store=TodoStore(), + )) + assert "error" in result + assert "empty or missing content" in result["error"] + + def test_merge_new_item_rejects_empty_content(self): + """Merge mode: new items with empty content are rejected.""" + store = TodoStore() + store.write([{"id": "1", "content": "Existing", "status": "pending"}]) + try: + store.write([{"id": "2", "content": "", "status": "pending"}], merge=True) + assert False, "should have raised ValueError" + except ValueError as e: + assert "empty or missing content" in str(e) + + def test_merge_existing_item_keeps_content_on_status_only_update(self): + """Merge mode: updating only status (no content key) preserves existing content.""" + store = TodoStore() + store.write([{"id": "1", "content": "Keep this", "status": "pending"}]) + store.write([{"id": "1", "status": "completed"}], merge=True) + items = store.read() + assert items[0]["content"] == "Keep this" + assert items[0]["status"] == "completed" + + def test_valid_content_still_works(self): + """Regression: valid items still pass through normally.""" + store = TodoStore() + items = store.write([ + {"id": "1", "content": "A real task", "status": "pending"}, + ]) + assert items[0]["content"] == "A real task" diff --git a/tools/todo_tool.py b/tools/todo_tool.py index b73b21e7f5..94577dc24f 100644 --- a/tools/todo_tool.py +++ b/tools/todo_tool.py @@ -36,12 +36,17 @@ class TodoStore: return self._normalize_order([self._validate(t) for t in self._dedupe_by_id(todos)]) def write(self, todos: List[Dict[str, Any]], merge: bool = False) -> List[Dict[str, str]]: - """Replace the list (default) or merge by id; returns the full list after writing.""" + """Replace the list (default) or merge by id; returns the full list after writing. + Raises ValueError (leaving the list untouched) if any item is invalid.""" before = self.read() - if merge: - self._merge(todos) - else: - self._items = self._fresh_items(todos) + try: + if merge: + self._merge(todos) + else: + self._items = self._fresh_items(todos) + except ValueError: + self._items = before # rejected write must not leave a partial list behind + raise del self._items[MAX_TODO_ITEMS:] # keep the priority head; replays can't grow unbounded self._sanitize_parents(self._items) if self._items != before: @@ -139,9 +144,14 @@ class TodoStore: return {"id": "?", "content": "(invalid item)", "status": "pending"} item_id = str(item.get("id", "")).strip() or "?" content = str(item.get("content", "")).strip() + if not content: + raise ValueError( + f"Todo item '{item_id}' has empty or missing content. " + "Each item must have a non-empty description." + ) status = str(item.get("status", "pending")).strip().lower() result = {"id": item_id, - "content": TodoStore._cap_content(content) if content else "(no description)", + "content": TodoStore._cap_content(content), "status": status if status in VALID_STATUSES else "pending"} parent = str(item.get("parent") or "").strip() if parent and parent != item_id: @@ -198,7 +208,10 @@ def todo_tool(todos: Optional[List[Dict[str, Any]]] = None, merge: bool = False, else: if not isinstance(todos, list): return tool_error(f"todos must be a list, got {type(todos).__name__}") - items = store.write(todos, merge) + try: + items = store.write(todos, merge) + except ValueError as e: + return tool_error(str(e)) summary = {"total": len(items)} for status in ("pending", "in_progress", "completed", "cancelled"): summary[status] = sum(1 for i in items if i["status"] == status)