[0/8] Stop a task write from dropping metadata or deleting the file

nerve/agent/tools/handlers/tasks.py

@@ -38,6 +38,7 @@
     normalize_color,
     random_status_color,
 )
+from nerve.tasks.files import move_task_file
 
 logger = logging.getLogger(__name__)
 
@@ -346,18 +347,20 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult:
 
                 final_title = new_title or task["title"]
                 final_status = status or task["status"]
-                final_deadline = deadline or task.get("deadline")
-                final_tags = new_tags_str if raw_tags else (task.get("tags") or "")
+                # Only the columns this call edits. The rest keep their
+                # stored values, so nothing has to be read back off ``task``.
+                edits: dict = {}
+                if deadline:
+                    edits["deadline"] = deadline
+                if raw_tags:
+                    edits["tags"] = new_tags_str
                 await ctx.db.upsert_task(
                     task_id=task_id,
                     file_path=task["file_path"],
                     title=final_title,
                     status=final_status,
-                    source=task.get("source"),
-                    source_url=task.get("source_url"),
-                    deadline=final_deadline,
-                    tags=final_tags,
                     content=content,
+                    **edits,
                 )
                 return ToolResult.text(f"Task {task_id} updated.")
 
@@ -432,18 +435,20 @@ async def task_write_handler(ctx: ToolContext, args: dict) -> ToolResult:
     # The deadline column is a pure projection of the file's Deadline line, so it
     # comes from the content we just wrote, never from the pre-write snapshot.
     new_deadline = frontmatter.get("deadline", "")
-    new_tags = tags_to_string(parse_tags_string(frontmatter.get("tags", task.get("tags", ""))))
+    # Tags follow the reindex rule: the file wins whenever it carries the
+    # field, and an absent field keeps the stored column.
+    edits: dict = {}
+    if "tags" in frontmatter:
+        edits["tags"] = tags_to_string(parse_tags_string(frontmatter["tags"]))
 
     await ctx.db.upsert_task(
         task_id=task_id,
         file_path=task["file_path"],
         title=new_title,
         status=task["status"],
-        source=task.get("source"),
-        source_url=task.get("source_url"),
         deadline=new_deadline or None,
-        tags=new_tags,
         content=new_content,
+        **edits,
     )
 
     return ToolResult.text(f"Task {task_id} written ({len(new_content)} chars).")
@@ -458,9 +463,9 @@ async def task_done_handler(ctx: ToolContext, args: dict) -> ToolResult:
         if not task:
             return ToolResult.text(f"Task not found: {task_id}", is_error=True)
 
-        # Done is a write like any other — it copies the file into done/ and
-        # unlinks the source, so a stored ``file_path`` inside the tracked config
-        # subtree would delete config. Refuse before the status flip, or a
+        # Done is a write like any other — it appends to the file and moves it
+        # into done/, so a stored ``file_path`` inside the tracked config
+        # subtree would rewrite config. Refuse before the status flip, or a
         # refusal leaves a task marked done whose file never moved.
         if ctx.workspace:
             ensure_path_not_tracked_config(ctx.workspace / task["file_path"], "move")
@@ -486,22 +491,14 @@ async def task_done_handler(ctx: ToolContext, args: dict) -> ToolResult:
 
                 dst = _done_dir(ctx) / src.name
 
-                def _move_to_done() -> None:
-                    dst.write_text(content, encoding="utf-8")
-                    src.unlink()
-
-                await asyncio.to_thread(_move_to_done)
+                await asyncio.to_thread(move_task_file, src, dst, content)
 
                 rel_path = str(dst.relative_to(ctx.workspace))
                 await ctx.db.upsert_task(
                     task_id=task_id,
                     file_path=rel_path,
                     title=task["title"],
                     status="done",
-                    source=task.get("source"),
-                    source_url=task.get("source_url"),
-                    deadline=task.get("deadline"),
-                    tags=task.get("tags") or "",
                     content=content,
                 )
 

nerve/db/tasks.py

@@ -6,6 +6,35 @@
 from datetime import datetime, timezone
 
 
+class _Keep:
+    """Type of the :data:`KEEP` sentinel."""
+
+    __slots__ = ()
+
+    def __repr__(self) -> str:
+        return "KEEP"
+
+
+# Default for the preserve-on-omit columns of :meth:`TaskStore.upsert_task`.
+# A separate sentinel is necessary because ``None`` is a real value there: it
+# clears the column.
+KEEP = _Keep()
+
+
+def _resolve_kept(value, stored: dict | None, column: str, default):
+    """Resolve one preserve-on-omit argument against the stored row.
+
+    A stored NULL gives ``default``. A row written before its column existed
+    can hold NULL, and ``tags`` must stay a ``str``.
+    """
+    if value is not KEEP:
+        return value
+    if stored is None:
+        return default
+    kept = stored[column]
+    return default if kept is None else kept
+
+
 class TaskStore:
     """Mixin providing task CRUD, FTS search, and escalation operations."""
 
@@ -15,14 +44,41 @@ async def upsert_task(
         file_path: str,
         title: str,
         status: str = "pending",
-        source: str | None = None,
-        source_url: str | None = None,
-        deadline: str | None = None,
-        tags: str = "",
+        source: str | None | _Keep = KEEP,
+        source_url: str | None | _Keep = KEEP,
+        deadline: str | None | _Keep = KEEP,
+        tags: str | _Keep = KEEP,
         content: str = "",
     ) -> None:
+        """Insert or update a task row and its FTS entry.
+
+        ``file_path``, ``title``, ``status`` and ``content`` are a full
+        replace. Every caller rebuilds them from the markdown file.
+
+        ``source``, ``source_url``, ``deadline`` and ``tags`` are
+        preserve-on-omit. An omitted column keeps its stored value. To clear
+        one, pass it: ``None`` for the nullable columns, ``""`` for ``tags``.
+
+        Those four columns need that rule because a markdown file carries
+        them only while it has the frontmatter for them. Under a full
+        replace, every caller has to read the row and pass all four back,
+        and a caller that forgets one deletes user data. That happened
+        twice. ``TaskManager.reindex`` dropped ``tags`` after v018 added the
+        column, and ``task_done`` nulled all four when it moved a file into
+        done/. ``test_task_upsert_preserve.py`` holds the rule itself;
+        ``test_task_reindex.py`` and ``test_task_completion.py`` hold the
+        caller-level regressions.
+        """
         now = datetime.now(timezone.utc).isoformat()
         async with self._atomic():
+            # One read resolves every preserve-on-omit column. The statement
+            # below stays a plain full replace, and the FTS text sees the
+            # tags that actually land in the row.
+            stored = await self._stored_task_columns(task_id)
+            source = _resolve_kept(source, stored, "source", None)
+            source_url = _resolve_kept(source_url, stored, "source_url", None)
+            deadline = _resolve_kept(deadline, stored, "deadline", None)
+            tags = _resolve_kept(tags, stored, "tags", "")
             await self.db.execute(
                 """INSERT INTO tasks (id, file_path, title, status, source, source_url, deadline, tags, created_at, updated_at)
                    VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
@@ -49,6 +105,15 @@ async def get_task(self, task_id: str) -> dict | None:
             row = await cursor.fetchone()
             return dict(row) if row else None
 
+    async def _stored_task_columns(self, task_id: str) -> dict | None:
+        """The columns an upsert can preserve, or ``None`` for a new task."""
+        async with self.db.execute(
+            "SELECT source, source_url, deadline, tags FROM tasks WHERE id = ?",
+            (task_id,),
+        ) as cursor:
+            row = await cursor.fetchone()
+        return dict(row) if row else None
+
     # Supported sort keys → ORDER BY clause. Keep deterministic with a
     # secondary key so equal timestamps don't flicker between pages.
     _SORT_CLAUSES = {

nerve/gateway/routes/tasks.py

@@ -147,16 +147,20 @@ async def update_task(task_id: str, req: TaskUpdateRequest, user: dict = Depends
             )
             new_title = parse_task_title(req.content)
             fields = parse_task_frontmatter(req.content)
+            # Only the columns the saved markdown carries. The rest keep their
+            # stored values, so nothing is read back off ``task`` here.
+            edits: dict = {}
+            if fields.get("deadline"):
+                edits["deadline"] = fields["deadline"]
+            if fields.get("tags"):
+                edits["tags"] = tags_to_string(parse_tags_string(fields["tags"]))
             await deps.db.upsert_task(
                 task_id=task_id,
                 file_path=task["file_path"],
                 title=new_title,
                 status=req.status or task["status"],
-                source=task.get("source"),
-                source_url=task.get("source_url"),
-                deadline=fields.get("deadline") or task.get("deadline"),
-                tags=tags_to_string(parse_tags_string(fields.get("tags") or task.get("tags", ""))),
                 content=req.content,
+                **edits,
             )
 
     # Update status/note/deadline/title via the unified handler (may

nerve/tasks/files.py

@@ -0,0 +1,26 @@
+"""Filesystem moves for task markdown files.
+
+Shared by the task tool handlers and :class:`~nerve.tasks.manager.TaskManager`,
+which both move a file between ``memory/tasks/active/`` and
+``memory/tasks/done/`` as a task changes status.
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+
+def move_task_file(src: Path, dst: Path, content: str) -> None:
+    """Write ``content`` to ``src``, then move it to ``dst``.
+
+    The move is a rename, not a copy followed by an unlink. A copy deletes the
+    file whenever ``src`` and ``dst`` are the same path, and that happens every
+    time a task is completed twice: the first completion stores a ``file_path``
+    under done/, and the second one reads that path back as its source. A
+    rename onto one path is a no-op, so the same sequence now only appends the
+    second note.
+
+    Blocking. Call it through ``asyncio.to_thread``.
+    """
+    src.write_text(content, encoding="utf-8")
+    src.replace(dst)

nerve/tasks/manager.py

@@ -9,6 +9,7 @@
 
 from nerve.config import ensure_path_not_tracked_config
 from nerve.db import Database
+from nerve.tasks.files import move_task_file
 from nerve.tasks.models import (
     Task,
     TaskStatus,
@@ -35,14 +36,16 @@ def __init__(self, workspace: Path, db: Database):
     async def reindex(self) -> int:
         """Scan task files and rebuild the SQLite + FTS index.
 
-        ``upsert_task`` replaces the whole row, so every column this loop does
-        not supply is written back as its signature default. Values that live
-        only in the DB (status) are carried from the stored row. For values the
-        file may carry, the rule is per column, each matching the save path that
-        already writes it: ``tags`` takes the file whenever the field is
-        present, so a present-but-empty field is an explicit clear;
-        ``source_url`` and ``deadline`` take any non-empty file value and
-        otherwise keep the stored row.
+        This loop supplies only the columns a markdown file carries. The rest
+        keep their stored values, because ``upsert_task`` is preserve-on-omit
+        for them. Which file values win is a per-column rule, each matching
+        the save path that already writes the column: ``tags`` takes the file
+        whenever the field is present, so a present-but-empty field is an
+        explicit clear; ``source_url`` and ``deadline`` take any non-empty
+        file value.
+
+        ``status`` is different. It lives only in the DB, so this loop reads
+        it from the stored row and always supplies it.
         """
         await self.db.rebuild_fts()
         count = 0
@@ -72,24 +75,26 @@ async def reindex(self) -> int:
                         prev = stored.get("status")
                         status = prev if prev and prev != "done" else default_status
 
+                    edits: dict = {}
+                    # **Source:** carries the URL (handlers/tasks.py writes
+                    # source_url there); `source` is a DB-only vocabulary, so
+                    # no file value ever reaches it.
+                    if fields.get("source"):
+                        edits["source_url"] = fields["source"]
+                    if fields.get("deadline"):
+                        edits["deadline"] = fields["deadline"]
+                    # Presence, not truthiness: a present-but-empty field is
+                    # an explicit clear, an absent one is "no information".
+                    if "tags" in fields:
+                        edits["tags"] = tags_to_string(parse_tags_string(fields["tags"]))
+
                     await self.db.upsert_task(
                         task_id=task_id,
                         file_path=rel_path,
                         title=title,
                         status=status,
-                        # **Source:** carries the URL (handlers/tasks.py writes
-                        # source_url there); `source` is a DB-only vocabulary.
-                        source=stored.get("source"),
-                        source_url=fields.get("source") or stored.get("source_url"),
-                        deadline=fields.get("deadline") or stored.get("deadline"),
-                        # Presence, not truthiness: a present-but-empty field is
-                        # an explicit clear, an absent one is "no information".
-                        tags=(
-                            tags_to_string(parse_tags_string(fields["tags"]))
-                            if "tags" in fields
-                            else (stored.get("tags") or "")
-                        ),
                         content=content,
+                        **edits,
                     )
                     count += 1
                 except Exception as e:
@@ -128,9 +133,9 @@ async def mark_done(self, task_id: str) -> bool:
             return False
 
         src = self.workspace / row["file_path"]
-        # Done is a write like any other — it copies the file into done/ and
-        # unlinks the source, so a stored ``file_path`` inside the tracked config
-        # subtree would delete config. Refuse before any of it runs, or the
+        # Done is a write like any other — it appends to the file and moves it
+        # into done/, so a stored ``file_path`` inside the tracked config
+        # subtree would rewrite config. Refuse before any of it runs, or the
         # refusal still leaves that config file mirrored into done/.
         ensure_path_not_tracked_config(src, "move")
         if src.exists():
@@ -139,11 +144,7 @@ async def mark_done(self, task_id: str) -> bool:
             today = datetime.now(timezone.utc).strftime("%Y-%m-%d")
             content += f"\n- {today}: DONE"
 
-            def _move_to_done() -> None:
-                dst.write_text(content, encoding="utf-8")
-                src.unlink()
-
-            await asyncio.to_thread(_move_to_done)
+            await asyncio.to_thread(move_task_file, src, dst, content)
 
             # Update DB
             rel_path = str(dst.relative_to(self.workspace))
@@ -152,10 +153,6 @@ def _move_to_done() -> None:
                 file_path=rel_path,
                 title=row["title"],
                 status="done",
-                source=row.get("source"),
-                source_url=row.get("source_url"),
-                deadline=row.get("deadline"),
-                tags=row.get("tags") or "",
                 content=content,
             )
 

tests/test_lockdown.py

@@ -1562,9 +1562,10 @@ async def test_update_is_still_refused(self, tmp_path, db, monkeypatch):
         assert "lockdown: true" in (ws / "config" / "settings.yaml").read_text()
 
     @pytest.mark.asyncio
-    async def test_done_is_refused_before_it_unlinks(self, tmp_path, db, monkeypatch):
-        """``task_done`` copies the file into ``done/`` and unlinks the source —
-        a delete of tracked config, and the most destructive of the three."""
+    async def test_done_is_refused_before_it_moves(self, tmp_path, db, monkeypatch):
+        """``task_done`` appends to the file and renames it into ``done/`` —
+        a rewrite and a move of tracked config, the most destructive of the
+        three."""
         from nerve.agent.tools.handlers.tasks import task_done_handler
 
         ws, ctx = await self._locked_task(tmp_path, db, monkeypatch)
@@ -1608,10 +1609,10 @@ async def test_an_ordinary_task_is_untouched_by_any_of_them(self, tmp_path, db,
 
 
 class TestLockdownTaskManagerGuardsTheMove:
-    """``TaskManager.mark_done`` has the same read-copy-unlink shape as the
-    ``task_done`` tool — a stored ``file_path`` joined to the workspace, copied
-    into ``done/`` and then unlinked — so it needs the same guard, or it is a
-    second route to deleting tracked config.
+    """``TaskManager.mark_done`` has the same append-and-rename shape as the
+    ``task_done`` tool — a stored ``file_path`` joined to the workspace,
+    appended to and then renamed into ``done/`` — so it needs the same guard,
+    or it is a second route to rewriting and moving tracked config.
 
     Both cases run locked, so the second one is what stops the guard from being
     written as "refuse every move".