fix(todo): reject empty content instead of substituting '(no description)' (#44496)

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"

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)