From 0945f3e7681a5f7e31e3abe29b74cb39fc1e4a3f Mon Sep 17 00:00:00 2001 From: Ramon Bartl Date: Tue, 9 Jun 2026 23:31:22 +0200 Subject: [PATCH] Fix three correctness regressions in the scheduling and sync work surfaced by the post-feature audit Three independent bugs sharing one PR because they were all uncovered in one code-review pass and they all turn on the same theme: an over-eager 'clear' / 'overwrite' on the cloud-pull path. Closes the HIGH-severity items 1, 2, 3, and 4 from the review. 1. Cloud pull silently cleared local scheduled/deadline. services/cloud_sync.py:_apply_task_update used to do scheduled=incoming.get('scheduled') or '', deadline=incoming.get('deadline') or '', ... so any wire payload whose scheduled key was None (PWA, iOS, or pre-v2.5.2 cloud that doesn't carry the field) wiped the locally-set date on the next unrelated remote edit. Pass the value through unchanged -- None means 'leave the local field alone' which exactly matches the rest of the PATCH convention. The trade-off: a cloud-side clear by a newer peer is also 'leave alone' until the user wakes/resets the date locally. Acceptable until the wire format carries an explicit cleared marker. Regression test: test_cloud_pull_does_not_clear_local_dates seeds an org task with both dates set, runs _apply_task_update with scheduled=None / deadline=None, asserts both survive. 2. JSON backend's list_tasks / list_archived diverged from SQL / markdown on the task dict shape. SQL normalises via _task_row_to_dict and markdown normalises via _section_to_task, both of which always emit scheduled / deadline as None when not set. The JSON backend returned the raw _read_json result, so a task written before #159 came back with the keys missing entirely. Wrap the read with _normalize_task that setdefaults both fields to None. Round-trip test for fresh-set JSON dates was also missing -- the markdown test did this, the JSON test only checked the raw file. Added. test_json_legacy_row_normalises_missing_dates pins the shape contract. 3. PAUSED-on-pull clear was too aggressive. v2.5.1 cleared PAUSED unconditionally on every cloud-origin sync to fix the resume-doesn't-clear bug. But every unrelated remote edit (notes appended on iPhone, customer renamed) routed through the same code path and wiped the local pause intent. Now both backends compare incoming end to the row's previous end and only clear PAUSED when they differ -- resume sets end to None, stop sets it from None to a value, a notes-only edit leaves end unchanged. Mirror tests in both test_clocks_paused_clear_on_pull and test_sql_backend_sync confirm PAUSED survives a notes-only sync. --- CHANGELOG.md | 19 +++++ kaisho/backends/json_backend/__init__.py | 27 ++++++- kaisho/backends/sql/__init__.py | 31 +++++--- kaisho/services/clocks.py | 23 ++++-- kaisho/services/cloud_sync.py | 17 +++- tests/test_clocks_paused_clear_on_pull.py | 46 +++++++++++ tests/test_sql_backend_sync.py | 36 +++++++++ tests/test_task_scheduled_deadline.py | 96 +++++++++++++++++++++++ 8 files changed, 275 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 964c7087..9c174ba3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/kaisho/backends/json_backend/__init__.py b/kaisho/backends/json_backend/__init__.py index 296efc94..e6025358 100644 --- a/kaisho/backends/json_backend/__init__.py +++ b/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 ----------------------------------------------- diff --git a/kaisho/backends/sql/__init__.py b/kaisho/backends/sql/__init__.py index 27f59993..32d01105 100644 --- a/kaisho/backends/sql/__init__.py +++ b/kaisho/backends/sql/__init__.py @@ -1410,6 +1410,15 @@ 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", @@ -1417,21 +1426,25 @@ def _apply_clock_fields(row: ClockRow, fields: dict) -> None: 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 # ==================================================================== diff --git a/kaisho/services/clocks.py b/kaisho/services/clocks.py index c6acb010..776f027d 100644 --- a/kaisho/services/clocks.py +++ b/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 = ( diff --git a/kaisho/services/cloud_sync.py b/kaisho/services/cloud_sync.py index 5633a5e3..5efd910e 100644 --- a/kaisho/services/cloud_sync.py +++ b/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( diff --git a/tests/test_clocks_paused_clear_on_pull.py b/tests/test_clocks_paused_clear_on_pull.py index 96b967be..73b24eb5 100644 --- a/tests/test_clocks_paused_clear_on_pull.py +++ b/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 diff --git a/tests/test_sql_backend_sync.py b/tests/test_sql_backend_sync.py index e9bf46d4..d5b3aaa7 100644 --- a/tests/test_sql_backend_sync.py +++ b/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()) diff --git a/tests/test_task_scheduled_deadline.py b/tests/test_task_scheduled_deadline.py index 45198645..0e77bbde 100644 --- a/tests/test_task_scheduled_deadline.py +++ b/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"