Fix three correctness regressions surfaced by the post-feature audit (cloud-pull clobber, JSON shape divergence, PAUSED over-clear)

CHANGELOG.md

@@ -2,6 +2,25 @@
 
 ## Unreleased
 
+- Fix three correctness regressions from the
+  scheduling + sync work. (1) A cloud-pull update for a
+  task whose wire payload omits `scheduled`/`deadline`
+  (legacy peer, PWA, iOS on an older build) no longer
+  silently clears the local dates; `None` on the wire
+  is now "leave unchanged", matching the rest of the
+  PATCH convention. (2) The JSON backend's `list_tasks`
+  / `list_archived` normalise legacy task dicts so
+  `scheduled`/`deadline` always come back as `None`
+  instead of missing keys — the SQL and markdown
+  backends already did this, the JSON one diverged.
+  (3) The PAUSED-on-pull clear from v2.5.1 was too
+  aggressive: it wiped the local pause flag on every
+  cloud-origin sync, including unrelated edits (notes
+  appended, customer renamed). Now PAUSED only clears
+  when the cloud actually touched the entry's `end` —
+  resume or stop — so editing notes on the iPhone for
+  a paused entry leaves the desktop pause flag alone.
+
 - Make the 5-minute cloud-sync poller's
   `_broadcast_sync_changes` actually refresh the kanban.
   It was sending `resource: "tasks"` while the frontend's

kaisho/backends/json_backend/__init__.py

@@ -129,6 +129,23 @@ def _guess_inbox_type(text: str) -> str:
     return "note"
 
 
+def _normalize_task(task: dict) -> dict:
+    """Ensure every task dict carries the full
+    documented shape — used on the read path so legacy
+    rows (written before a field was added) match what
+    SQL and markdown backends emit and what
+    ``frontend/src/types.ts:Task`` expects.
+
+    Today the only fields needing a missing → None fill
+    are ``scheduled`` and ``deadline``. The helper is
+    deliberately additive so future schema growth lands
+    in one place.
+    """
+    task.setdefault("scheduled", None)
+    task.setdefault("deadline", None)
+    return task
+
+
 # ====================================================================
 #  TaskBackend
 # ====================================================================
@@ -156,7 +173,10 @@ def list_tasks(
         include_done=False,
     ) -> list[dict]:
         """Return tasks matching the given filters."""
-        tasks = _read_json(self._tasks_file)
+        tasks = [
+            _normalize_task(t)
+            for t in _read_json(self._tasks_file)
+        ]
         if not include_done:
             tasks = [
                 t for t in tasks
@@ -198,7 +218,10 @@ def list_all_tags(self) -> list[dict]:
 
     def list_archived(self) -> list[dict]:
         """Return all archived tasks."""
-        return _read_json(self._archive_file)
+        return [
+            _normalize_task(t)
+            for t in _read_json(self._archive_file)
+        ]
 
     # -- mutations -----------------------------------------------
 

kaisho/backends/sql/__init__.py

@@ -1410,28 +1410,41 @@ def _match_clock_by_content(session, fields):
 def _apply_clock_fields(row: ClockRow, fields: dict) -> None:
     """Set ClockRow columns from a sync payload. The caller
     enforces last-writer-wins before calling this."""
+    # Snapshot ``row.end`` before the new value lands so
+    # we can tell whether the cloud-origin change touched
+    # the entry's timing — see PAUSED handling below.
+    prev_end = row.end
+    new_end = (
+        fields["end"] or None if "end" in fields
+        else row.end
+    )
+
     for col in (
         "customer", "description", "start",
         "task_id", "contract", "notes",
     ):
         if col in fields:
             setattr(row, col, fields[col] or "")
     if "end" in fields:
-        row.end = fields["end"] or None
+        row.end = new_end
     if "invoiced" in fields:
         row.invoiced = bool(fields["invoiced"])
     row.sync_id = fields["sync_id"]
     row.updated_at = (
         fields.get("updated_at") or _local_now().isoformat()
     )
-    # ``paused`` is a desktop-only UI affordance and never
-    # crosses the wire. Any cloud-origin pull is therefore
-    # authoritative evidence that the entry is *not* paused
-    # — otherwise the running-timer card stays stuck on a
-    # stale Resume affordance for an entry the cloud has
-    # since resumed or stopped on another device. Mirrors
-    # the same clear in ``services/clocks.py:apply_sync_payload``.
-    row.paused = False
+    # ``paused`` is a desktop-only UI affordance that
+    # never crosses the wire. Only clear it when the
+    # cloud actually touched this entry's timing — resume
+    # clears ``end``, stop sets it. A non-timing change
+    # (notes appended on the PWA, customer renamed, tag
+    # adjusted) leaves ``end`` unchanged and must NOT
+    # wipe the local pause flag: the user paused on this
+    # device and still intends to resume. Mirrors the
+    # narrower clear in
+    # ``services/clocks.py:apply_sync_payload``.
+    if prev_end != new_end:
+        row.paused = False
 
 
 # ====================================================================

kaisho/services/clocks.py

@@ -750,6 +750,11 @@ def apply_sync_payload(
         else None
     )
 
+    # Snapshot the previous ``end`` *before* the new value
+    # lands so we can tell whether the cloud-origin
+    # change actually touched the entry's timing.
+    prev_end = clock.end
+
     clock.start = start
     clock.end = end
     if end is not None:
@@ -780,13 +785,17 @@ def apply_sync_payload(
         props.pop("INVOICED", None)
     props["SYNC_ID"] = fields["sync_id"]
     props["UPDATED_AT"] = fields["updated_at"]
-    # ``PAUSED`` is a desktop-only UI affordance and never
-    # crosses the wire. Any cloud-origin pull is therefore
-    # authoritative evidence that the entry is *not* paused
-    # — otherwise the running-timer card stays stuck on a
-    # stale Resume affordance for an entry the cloud has
-    # since resumed or stopped on another device.
-    props.pop("PAUSED", None)
+    # ``PAUSED`` is a desktop-only UI affordance that never
+    # crosses the wire. Only clear it when the cloud
+    # actually touched the entry's timing — resume sets
+    # ``end`` from a value to None, stop sets it from None
+    # (or a previous value) to a new one. A non-timing
+    # change (notes appended on the PWA, customer renamed,
+    # tag adjusted) leaves ``end`` unchanged and must NOT
+    # wipe the local pause flag — the user paused on this
+    # device and still intends to resume.
+    if prev_end != end:
+        props.pop("PAUSED", None)
 
     notes = fields.get("notes") or ""
     heading.body = (

kaisho/services/cloud_sync.py

@@ -1698,14 +1698,27 @@ def _apply_task_update(
     status (only when changed, since move_task is the
     write that moves between sections in the org file).
     """
+    # Pass scheduled / deadline through as-is. ``None``
+    # means "leave the local value alone" -- which is
+    # exactly the right behaviour for two cases that we
+    # can't distinguish at this layer: a wire payload
+    # from an older peer that doesn't know about these
+    # fields, and a fresh wire payload whose field was
+    # never set on the cloud side. Coercing ``None`` to
+    # ``""`` (the previous code) would silently clear a
+    # local date whenever either happened. The
+    # trade-off: a cloud-side clear by a newer peer is
+    # also seen as "leave alone" until the user wakes /
+    # resets the date locally. Acceptable until the wire
+    # carries an explicit "cleared" marker.
     backend.tasks.update_task(
         existing["id"],
         title=incoming["title"],
         customer=incoming["customer"],
         body=incoming["body"],
         github_url=incoming["github_url"],
-        scheduled=incoming.get("scheduled") or "",
-        deadline=incoming.get("deadline") or "",
+        scheduled=incoming.get("scheduled"),
+        deadline=incoming.get("deadline"),
     )
     if incoming.get("tags") != existing.get("tags"):
         backend.tasks.set_tags(

tests/test_clocks_paused_clear_on_pull.py

@@ -110,3 +110,49 @@ def test_pull_clears_paused_for_resumed_running_entry(
     # And the clock is now running (no end).
     clock = org.headings[0].logbook[0]
     assert clock.end is None
+
+
+def test_pull_preserves_paused_on_non_timing_edit(
+    tmp_path,
+):
+    """A cloud-origin update that does NOT touch the
+    entry's timing (notes appended, customer renamed,
+    tag adjusted) must leave the local PAUSED flag
+    alone. The user paused on this device and still
+    intends to resume — an unrelated remote edit must
+    not silently wipe that intent."""
+    clocks_file = tmp_path / "clocks.org"
+    sync_id = "abc-notes"
+    _seed_paused_entry(clocks_file, sync_id)
+
+    clocks_svc.update_clock_entry_by_sync_id(
+        clocks_file=clocks_file,
+        sync_id=sync_id,
+        fields={
+            "sync_id": sync_id,
+            "customer": "KAISHO",
+            "description": "paused-entry",
+            "start": "2026-06-09T10:29:00",
+            # End matches the seed exactly — the cloud
+            # only changed the notes.
+            "end": "2026-06-09T10:30:00",
+            "updated_at": "2099-01-01T11:00:01",
+            "invoiced": False,
+            "task_id": None,
+            "contract": None,
+            "notes": "remote added a note",
+        },
+    )
+
+    org = parse_org_file(clocks_file, CLOCK_KEYWORDS)
+    assert (
+        org.headings[0].properties.get("PAUSED")
+        == "true"
+    ), (
+        "PAUSED must survive a cloud pull that did not "
+        "change the entry's end timestamp — only resume "
+        "or stop should clear the local pause intent"
+    )
+    # And the note made it through.
+    body_text = "\n".join(org.headings[0].body)
+    assert "remote added a note" in body_text

tests/test_sql_backend_sync.py

@@ -88,6 +88,42 @@ def test_apply_clears_local_paused_flag(tmp_path):
     assert after["paused"] is False
 
 
+def test_apply_keeps_paused_when_end_unchanged(tmp_path):
+    """A cloud-origin update that does NOT touch ``end``
+    (notes appended, customer renamed) must leave the
+    local ``paused`` flag alone. The user paused on this
+    device and still intends to resume; an unrelated
+    remote edit must not silently wipe that intent.
+
+    Mirror of
+    ``test_pull_preserves_paused_on_non_timing_edit`` for
+    the org backend."""
+    clocks = _clocks(tmp_path)
+    clocks.apply_sync_payload(_payload(end=None))
+    clocks.stop(paused=True)
+    paused_row = clocks.list_entries(period="all")[0]
+    assert paused_row["paused"] is True
+    paused_end = paused_row["end"]
+
+    # Cloud sends a newer update with the SAME end
+    # timestamp — only the notes / description moved.
+    clocks.apply_sync_payload(_payload(
+        end=paused_end,
+        description="remote updated description",
+        updated_at="2099-01-01T13:00:01",
+    ))
+    after = clocks.list_entries(period="all")[0]
+    assert after["paused"] is True, (
+        "paused must survive a cloud pull that did not "
+        "change end -- only resume or stop should clear "
+        "the local pause intent"
+    )
+    assert (
+        after["description"]
+        == "remote updated description"
+    )
+
+
 def test_delete_entry_by_sync_id(tmp_path):
     clocks = _clocks(tmp_path)
     clocks.apply_sync_payload(_payload())

tests/test_task_scheduled_deadline.py

@@ -206,6 +206,43 @@ def test_json_add_and_read_dates(tmp_path):
     assert raw[0]["scheduled"] == "2099-06-15"
     assert raw[0]["deadline"] == "2099-06-20"
 
+    # Round-trip via a fresh backend instance so the dates
+    # survive a save/load cycle (the markdown test does
+    # this; the JSON one was missing it before the audit).
+    fresh = _json_backend(tmp_path)
+    again = fresh.list_tasks(include_done=True)[0]
+    assert again["scheduled"] == "2099-06-15"
+    assert again["deadline"] == "2099-06-20"
+
+
+def test_json_legacy_row_normalises_missing_dates(
+    tmp_path,
+):
+    """A JSON file written before scheduled/deadline
+    existed must come back with both keys present and
+    set to None — otherwise the dict shape diverges from
+    what SQL and markdown emit and from what
+    ``frontend/src/types.ts:Task`` expects."""
+    legacy = [{
+        "id": "abc",
+        "customer": "Acme",
+        "title": "Old task",
+        "status": "TODO",
+        "tags": [],
+        "body": "",
+        "github_url": "",
+        "created": "2026-01-01T10:00:00",
+    }]
+    (tmp_path / "tasks.json").write_text(
+        json.dumps(legacy), encoding="utf-8",
+    )
+    backend = _json_backend(tmp_path)
+    task = backend.list_tasks(include_done=True)[0]
+    assert "scheduled" in task
+    assert "deadline" in task
+    assert task["scheduled"] is None
+    assert task["deadline"] is None
+
 
 # -- Markdown backend -------------------------------------
 
@@ -287,3 +324,62 @@ def test_wire_handles_missing_dates_as_none():
     back = wire_to_task(wire)
     assert back["scheduled"] is None
     assert back["deadline"] is None
+
+
+# -- Cloud pull does not clobber local dates --------------
+
+class _BackendShim:
+    """Minimal stand-in for the kaisho backend façade so
+    cloud-sync helpers can be called against a single
+    sub-backend in tests. The production code expects
+    ``backend.tasks`` (and other domain attributes); this
+    wrapper exposes just what ``_apply_task_update``
+    needs."""
+
+    def __init__(self, tasks_backend):
+        self.tasks = tasks_backend
+
+
+def test_cloud_pull_does_not_clear_local_dates(tmp_path):
+    """A wire entry from an older peer (or a fresh peer
+    that never set the dates) carries ``scheduled: None``
+    / ``deadline: None``. The cloud-pull path must
+    interpret ``None`` as "leave the local value alone",
+    not as "clear". Coercing ``None`` → ``""`` (the
+    previous code) silently wiped a local-set date every
+    time a remote edited the task for any other reason."""
+    from kaisho.services.cloud_sync import _apply_task_update
+    tasks = _org_backend(tmp_path)
+    backend = _BackendShim(tasks)
+    task = tasks.add_task(
+        customer="Acme",
+        title="Locally scheduled",
+        scheduled="2099-06-15",
+        deadline="2099-06-20",
+    )
+    existing = tasks.list_tasks(include_done=True)[0]
+    assert existing["scheduled"] == "2099-06-15"
+
+    incoming = {
+        "sync_id": task["sync_id"],
+        "customer": "Acme",
+        "title": "Locally scheduled",
+        "status": "TODO",
+        "tags": [],
+        "body": "edited remotely",
+        "github_url": "",
+        "scheduled": None,
+        "deadline": None,
+        "updated_at": "2099-12-31T00:00:00",
+    }
+    _apply_task_update(backend, incoming, existing)
+
+    after = tasks.list_tasks(include_done=True)[0]
+    assert after["scheduled"] == "2099-06-15", (
+        "cloud pull with scheduled=None must NOT wipe "
+        "the local scheduled date"
+    )
+    assert after["deadline"] == "2099-06-20"
+    # The remote did change something — confirm the
+    # legitimate field went through.
+    assert after["body"] == "edited remotely"