From aada69d6ebc2c854160074245da1875747faa723 Mon Sep 17 00:00:00 2001 From: Jer Miller Date: Sun, 7 Jun 2026 10:09:04 -0600 Subject: [PATCH] refactor(todos): collapse 3 divergent moves onto atomic move_entry; drain bare-returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The todos app had three independent two-step "move a todo" implementations (route /move cross-day, route edit-with-facet-change, CLI move cross-facet), each doing append-to-target then cancel-source as separate locked steps — a replayed request duplicated a todo (the /move route appended a second copy on a same-item-moved-twice). Add TodoChecklist.move_entry(src_day, src_facet, line_number, dst_day, dst_facet, *, text=None): one atomic op that holds both files' locks in canonical (sorted-path) order, loads + re-validates the source under-lock, appends to the target, then cancels the source last — the safe direction, so a crash between the two writes leaves the item in the target AND still open in the source, never lost from both. Returns (created, cancelled). Typed exceptions (TodoNotMovableError, TodoMovePartialError) let each surface translate uniformly; routes and CLI become thin adapters. Guards the src==dst self-block (nested same-path hold_lock would LockTimeout). move_entry is policy-neutral on completion (it carries completed state); each caller keeps its own precondition. Drain the 5 JSON-governed `return "", 404` invalid-day guards to error_response(INVALID_DAY, status=404) and drop the todos bare-return entry from the api-conventions allowlist. CPO note — completed-item move policy diverges by surface: the CLI rejects a completed move pre-call; HTTP carries completion. Preserved here, not reconciled; reconciling it is an observable contract change (CPO lane, not VPE). Related nuance: the CLI no longer re-checks completion under-lock (move_entry is policy-neutral); the pre-call check covers every non-racing input. Discovered, deferred: solstone/think/tools/call.py facet-merge moves open todos with the same append-then-cancel two-step. Out of scope (merge = separate hardening task); fold it onto move_entry there once that task runs. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/check_api_conventions.py | 1 - solstone/apps/todos/call.py | 54 ++----- solstone/apps/todos/routes.py | 129 ++++++--------- solstone/apps/todos/tests/test_call.py | 21 +++ solstone/apps/todos/tests/test_routes.py | 198 +++++++++++++++++++++++ solstone/apps/todos/tests/test_todo.py | 158 ++++++++++++++++++ solstone/apps/todos/todo.py | 95 +++++++++++ 7 files changed, 535 insertions(+), 121 deletions(-) create mode 100644 solstone/apps/todos/tests/test_routes.py diff --git a/scripts/check_api_conventions.py b/scripts/check_api_conventions.py index ce50eab26..de22cb519 100644 --- a/scripts/check_api_conventions.py +++ b/scripts/check_api_conventions.py @@ -83,7 +83,6 @@ ALLOWLIST: dict[tuple[str, str], int] = { ("solstone/apps/import/routes.py", "bare-array"): 1, ("solstone/apps/link/routes.py", "abort"): 1, ("solstone/apps/observer/routes.py", "bare-array"): 2, - ("solstone/apps/todos/routes.py", "bare-return"): 5, ("solstone/convey/services_scout.py", "abort"): 1, ("solstone/convey/services_scout.py", "inline-error"): 2, } diff --git a/solstone/apps/todos/call.py b/solstone/apps/todos/call.py index 70fc8249d..10f6c75ea 100644 --- a/solstone/apps/todos/call.py +++ b/solstone/apps/todos/call.py @@ -14,6 +14,7 @@ from pathlib import Path import typer from solstone.apps.todos import todo +from solstone.apps.todos.todo import TodoMovePartialError, TodoNotMovableError from solstone.think.facets import log_call_action from solstone.think.journal_io.errors import LockTimeout from solstone.think.utils import get_journal, require_solstone @@ -309,12 +310,17 @@ def move_todo( ) raise typer.Exit(1) + if from_facet == to_facet: + typer.echo("Error: source and destination facet are the same.", err=True) + raise typer.Exit(1) + try: source_checklist = todo.TodoChecklist.load(day, from_facet) if not source_checklist.exists: raise FileNotFoundError() todo.validate_line_number(line_number, len(source_checklist.items)) item = source_checklist.items[line_number - 1] + # Completion is enforced pre-call; move_entry intentionally stays policy-neutral. if item.completed: raise todo.TodoError("Cannot move a completed todo.") if item.cancelled: @@ -333,45 +339,15 @@ def move_todo( raise typer.Exit(1) try: - - def _append_dest( - checklist: todo.TodoChecklist, - ) -> tuple[todo.TodoChecklist, todo.TodoItem]: - new_item = checklist.append_entry( - item.text, - item.nudge, - created_at=item.created_at, - ) - return checklist, new_item - - _, new_item = todo.TodoChecklist.locked_modify(day, to_facet, _append_dest) - except Exception as exc: - typer.echo( - f"Error: Failed to append to destination facet '{to_facet}': {exc}. Source todo is unchanged.", - err=True, + new_item, cancelled = todo.TodoChecklist.move_entry( + day, from_facet, line_number, day, to_facet ) + except (IndexError, TodoNotMovableError) as exc: + typer.echo(f"Error: {exc}", err=True) raise typer.Exit(1) - - try: - - def _cancel_source( - checklist: todo.TodoChecklist, - ) -> tuple[todo.TodoChecklist, todo.TodoItem]: - todo.validate_line_number(line_number, len(checklist.items)) - current_item = checklist.items[line_number - 1] - if current_item.completed: - raise todo.TodoError("Cannot move a completed todo.") - if current_item.cancelled: - raise todo.TodoError("Cannot move an already cancelled todo.") - cancelled_item = checklist.cancel_entry( - line_number, - cancelled_reason="moved_to_facet", - moved_to=to_facet, - ) - return checklist, cancelled_item - - _, item = todo.TodoChecklist.locked_modify(day, from_facet, _cancel_source) - except (FileNotFoundError, IndexError, todo.TodoError, LockTimeout): + except LockTimeout: + _exit_todo_busy() + except TodoMovePartialError: typer.echo( f"Warning: Item was appended to '{to_facet}' but could not cancel source in '{from_facet}'. Cancel it manually with: sol call todos cancel {line_number} --day {day} --facet {from_facet}", err=True, @@ -382,7 +358,7 @@ def move_todo( "moved_from": from_facet, "moved_to": to_facet, "line_number": line_number, - "text": item.text, + "text": cancelled.text, } params_in: dict[str, object] = { "moved_from": from_facet, @@ -396,7 +372,7 @@ def move_todo( log_call_action(facet=from_facet, action="todo_move_out", params=params_out) log_call_action(facet=to_facet, action="todo_move_in", params=params_in) typer.echo( - f"Moved todo {line_number} ('{item.text}') from '{from_facet}' to '{to_facet}'." + f"Moved todo {line_number} ('{cancelled.text}') from '{from_facet}' to '{to_facet}'." ) diff --git a/solstone/apps/todos/routes.py b/solstone/apps/todos/routes.py index 294b2ceae..a0d1c26de 100644 --- a/solstone/apps/todos/routes.py +++ b/solstone/apps/todos/routes.py @@ -22,6 +22,8 @@ from solstone.apps.todos.todo import ( TodoChecklist, TodoEmptyTextError, TodoItem, + TodoMovePartialError, + TodoNotMovableError, format_nudge, get_todos, ) @@ -216,7 +218,11 @@ def todos_page() -> str: @todos_bp.route("/", methods=["GET", "POST"]) def todos_day(day: str): # type: ignore[override] if not DATE_RE.fullmatch(day): - return "", 404 + return error_response( + INVALID_DAY, + status=404, + detail="Day must be in YYYYMMDD format.", + ) if request.method == "POST": action = request.form.get("action") @@ -404,25 +410,9 @@ def todos_day(day: str): # type: ignore[override] # If facet changed, move the todo (cancel source, add to target) if new_facet != facet: - source_item = checklist.get_item(index) - old_text = source_item.text - - def _add_to_new(cl: TodoChecklist) -> TodoItem: - created = cl.append_entry( - text, - nudge=source_item.nudge, - created_at=source_item.created_at, - ) - if source_item.completed: - cl.mark_done(created.index) - return created - - TodoChecklist.locked_modify(day, new_facet, _add_to_new) - - def _cancel_source(cl: TodoChecklist) -> None: - cl.cancel_entry(index) - - TodoChecklist.locked_modify(day, facet, _cancel_source) + _created, cancelled = TodoChecklist.move_entry( + day, facet, index, day, new_facet, text=text + ) log_app_action( app="todos", @@ -430,7 +420,7 @@ def todos_day(day: str): # type: ignore[override] action="todo_edit", params={ "line_number": index, - "old_text": old_text, + "old_text": cancelled.text, "new_text": text, "old_facet": facet, "new_facet": new_facet, @@ -465,6 +455,10 @@ def todos_day(day: str): # type: ignore[override] flash("Cannot update todo to empty text", "error") except IndexError: flash(f"Todo list changed, {CONVEY_RELOAD_HINT}", "error") + except TodoNotMovableError: + flash(f"Todo list changed, {CONVEY_RELOAD_HINT}", "error") + except TodoMovePartialError: + flash(f"Todo list changed, {CONVEY_RELOAD_HINT}", "error") except LockTimeout: flash(f"Todo list changed, {CONVEY_RELOAD_HINT}", "error") @@ -602,7 +596,11 @@ def todos_overflow(day: str, facet: str, section: str): def move_todo(day: str): # type: ignore[override] """Move a todo to a different day by cancelling source and adding to target.""" if not DATE_RE.fullmatch(day): - return "", 404 + return error_response( + INVALID_DAY, + status=404, + detail="Day must be in YYYYMMDD format.", + ) payload = request.get_json(silent=True) or {} target_day = (payload.get("target_day") or "").strip() @@ -631,49 +629,26 @@ def move_todo(day: str): # type: ignore[override] } ) + # L9: replaying an already-moved item returns 409 without appending again. try: - source_checklist = TodoChecklist.load(day, facet) - except RuntimeError as exc: - current_app.logger.debug( - "Failed to load source todo list for %s/%s: %s", facet, day, exc + _created, cancelled = TodoChecklist.move_entry( + day, facet, index, target_day, facet ) - return error_response( - OPERATION_NO_LONGER_AVAILABLE, - status=409, - detail=f"Todo list changed, {CONVEY_RELOAD_HINT}", - ) - - try: - TodoChecklist.load(target_day, facet) - except RuntimeError as exc: + except (IndexError, TodoNotMovableError, TodoMovePartialError, LockTimeout) as exc: current_app.logger.debug( - "Failed to load target todo list for %s/%s: %s", facet, target_day, exc - ) - return error_response( - TODO_OPERATION_FAILED, - detail="Unable to access target day.", + "Failed to move todo %s from %s/%s to %s/%s: %s", + index, + facet, + day, + facet, + target_day, + exc, ) - - try: - source_item = source_checklist.get_item(index) - except IndexError as exc: - current_app.logger.debug("Failed to locate todo %s on %s: %s", index, day, exc) return error_response( OPERATION_NO_LONGER_AVAILABLE, status=409, detail=f"Todo list changed, {CONVEY_RELOAD_HINT}", ) - - def _append_target(cl: TodoChecklist) -> TodoItem: - created = cl.append_entry( - source_item.text, nudge=source_item.nudge, created_at=source_item.created_at - ) - if source_item.completed: - cl.mark_done(created.index) - return created - - try: - _new_item = TodoChecklist.locked_modify(target_day, facet, _append_target) except TodoEmptyTextError as exc: current_app.logger.debug("Failed to append todo to %s: %s", target_day, exc) return error_response( @@ -681,26 +656,6 @@ def move_todo(day: str): # type: ignore[override] status=400, detail="Unable to move todo to the selected day.", ) - except LockTimeout as exc: - current_app.logger.debug("Todo list busy for %s: %s", target_day, exc) - return error_response( - OPERATION_NO_LONGER_AVAILABLE, - status=409, - detail=f"Todo list changed, {CONVEY_RELOAD_HINT}", - ) - - def _cancel_source(cl: TodoChecklist) -> None: - cl.cancel_entry(index) - - try: - TodoChecklist.locked_modify(day, facet, _cancel_source) - except LockTimeout as exc: - current_app.logger.debug("Todo list busy for %s: %s", day, exc) - return error_response( - OPERATION_NO_LONGER_AVAILABLE, - status=409, - detail=f"Todo list changed, {CONVEY_RELOAD_HINT}", - ) log_app_action( app="todos", @@ -710,8 +665,8 @@ def move_todo(day: str): # type: ignore[override] "source_day": day, "target_day": target_day, "line_number": index, - "text": source_item.text, - "completed": source_item.completed, + "text": cancelled.text, + "completed": cancelled.completed, }, day=day, ) @@ -731,7 +686,11 @@ def move_todo(day: str): # type: ignore[override] @todos_bp.route("//generate", methods=["POST"]) def generate_todos(day: str): # type: ignore[override] if not DATE_RE.fullmatch(day): - return "", 404 + return error_response( + INVALID_DAY, + status=404, + detail="Day must be in YYYYMMDD format.", + ) payload = request.get_json(silent=True) or {} facet = (payload.get("facet") or "personal").strip() @@ -791,7 +750,11 @@ Write the generated checklist to facets/{facet}/todos/{day}.jsonl""" @todos_bp.route("//generation-status") def todo_generation_status(day: str): # type: ignore[override] if not DATE_RE.fullmatch(day): - return "", 404 + return error_response( + INVALID_DAY, + status=404, + detail="Day must be in YYYYMMDD format.", + ) facet = request.args.get("facet", "personal") use_id = request.args.get("use_id") @@ -838,7 +801,11 @@ def todo_generation_status(day: str): # type: ignore[override] def generate_weekly_todos(day: str, facet: str): # type: ignore[override] """Spawn todo_weekly agent for a specific facet.""" if not DATE_RE.fullmatch(day): - return "", 404 + return error_response( + INVALID_DAY, + status=404, + detail="Day must be in YYYYMMDD format.", + ) day_date = datetime.strptime(day, "%Y%m%d") diff --git a/solstone/apps/todos/tests/test_call.py b/solstone/apps/todos/tests/test_call.py index 77c30197f..441e13ed3 100644 --- a/solstone/apps/todos/tests/test_call.py +++ b/solstone/apps/todos/tests/test_call.py @@ -511,6 +511,27 @@ class TestTodosMove: assert result.exit_code == 1 assert "does not exist" in result.output + def test_move_same_facet_rejected(self, move_env): + _, src_facet, _dst_facet = move_env([{"text": "Ship feature"}]) + + result = runner.invoke( + call_app, + [ + "todos", + "move", + "1", + "--day", + "20240101", + "--from", + src_facet, + "--to", + src_facet, + ], + ) + + assert result.exit_code == 1 + assert "source and destination facet are the same" in result.output + class TestSolEnvResolution: """Tests for SOL_* env var resolution in todos commands.""" diff --git a/solstone/apps/todos/tests/test_routes.py b/solstone/apps/todos/tests/test_routes.py new file mode 100644 index 000000000..05a8e34f1 --- /dev/null +++ b/solstone/apps/todos/tests/test_routes.py @@ -0,0 +1,198 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright (c) 2026 sol pbc + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from solstone.apps.todos.todo import TodoChecklist +from solstone.convey import create_app +from solstone.think.call import call_app + +runner = CliRunner() +STABLE_KEYS = ( + "text", + "nudge", + "created_at", + "completed", + "cancelled", + "cancelled_reason", + "moved_to", +) + + +@pytest.fixture +def todos_client(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(tmp_path)) + + def _create(): + app = create_app(journal=str(tmp_path)) + app.config["TESTING"] = True + client = app.test_client() + with client.session_transaction() as session: + session["logged_in"] = True + session.permanent = True + return client + + return _create + + +def _ensure_facet(root: Path, facet: str) -> None: + facet_dir = root / "facets" / facet + facet_dir.mkdir(parents=True, exist_ok=True) + (facet_dir / "facet.json").write_text( + json.dumps( + { + "title": facet.title(), + "description": f"{facet} facet", + "color": "#6b7280", + } + ) + + "\n", + encoding="utf-8", + ) + + +def _write_todo(root: Path, facet: str, day: str, entry: dict) -> Path: + _ensure_facet(root, facet) + todos_dir = root / "facets" / facet / "todos" + todos_dir.mkdir(parents=True, exist_ok=True) + path = todos_dir / f"{day}.jsonl" + path.write_text(json.dumps(entry, ensure_ascii=False) + "\n", encoding="utf-8") + return path + + +def _read_jsonl(path: Path) -> list[dict]: + return [json.loads(line) for line in path.read_text(encoding="utf-8").splitlines()] + + +def _stable_items(path: Path) -> list[dict]: + return [ + {key: item[key] for key in STABLE_KEYS if key in item} + for item in _read_jsonl(path) + ] + + +def _client_for_journal(journal: Path): + app = create_app(journal=str(journal)) + app.config["TESTING"] = True + client = app.test_client() + with client.session_transaction() as session: + session["logged_in"] = True + session.permanent = True + return client + + +def test_move_same_item_twice_via_route(tmp_path: Path, todos_client) -> None: + day = "20260415" + target_day = "20260416" + facet = "personal" + _write_todo( + tmp_path, + facet, + day, + {"text": "Move once", "created_at": 1704067200000}, + ) + + client = todos_client() + payload = {"target_day": target_day, "facet": facet, "index": 1} + + first = client.post(f"/app/todos/{day}/move", json=payload) + target_path = tmp_path / "facets" / facet / "todos" / f"{target_day}.jsonl" + assert first.status_code == 200 + assert [item["text"] for item in _read_jsonl(target_path)] == ["Move once"] + + second = client.post(f"/app/todos/{day}/move", json=payload) + assert second.status_code == 409 + assert [item["text"] for item in _read_jsonl(target_path)] == ["Move once"] + + +def test_move_route_partial_failure_preserves_source( + tmp_path: Path, todos_client, monkeypatch: pytest.MonkeyPatch +) -> None: + day = "20260417" + target_day = "20260418" + facet = "personal" + source_path = _write_todo( + tmp_path, + facet, + day, + {"text": "Partial route", "created_at": 1704067200000}, + ) + + def fail_cancel( + self: TodoChecklist, + line_number: int, + cancelled_reason: str | None = None, + moved_to: str | None = None, + ): + raise RuntimeError("cancel failed") + + monkeypatch.setattr(TodoChecklist, "cancel_entry", fail_cancel) + + response = todos_client().post( + f"/app/todos/{day}/move", + json={"target_day": target_day, "facet": facet, "index": 1}, + ) + + target_path = tmp_path / "facets" / facet / "todos" / f"{target_day}.jsonl" + assert response.status_code == 409 + assert response.get_json()["reason_code"] == "operation_no_longer_available" + assert _read_jsonl(source_path)[0].get("cancelled") is None + assert _read_jsonl(target_path)[0]["text"] == "Partial route" + + +def test_edit_move_route_cli_byte_parity( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + day = "20260419" + entry = { + "text": "Parity task", + "nudge": "20260419T10:30", + "created_at": 1704067200000, + } + + route_journal = tmp_path / "route" + _write_todo(route_journal, "work", day, entry) + _ensure_facet(route_journal, "personal") + monkeypatch.setenv("SOLSTONE_JOURNAL", str(route_journal)) + route_response = _client_for_journal(route_journal).post( + f"/app/todos/{day}", + data={ + "action": "edit", + "facet": "work", + "index": "1", + "text": "Parity task #personal", + }, + ) + assert route_response.status_code == 302 + + cli_journal = tmp_path / "cli" + _write_todo(cli_journal, "work", day, entry) + _ensure_facet(cli_journal, "personal") + monkeypatch.setenv("SOLSTONE_JOURNAL", str(cli_journal)) + cli_result = runner.invoke( + call_app, + [ + "todos", + "move", + "1", + "--day", + day, + "--from", + "work", + "--to", + "personal", + ], + ) + assert cli_result.exit_code == 0, cli_result.output + + for facet in ("work", "personal"): + relpath = Path("facets") / facet / "todos" / f"{day}.jsonl" + assert _stable_items(route_journal / relpath) == _stable_items( + cli_journal / relpath + ) diff --git a/solstone/apps/todos/tests/test_todo.py b/solstone/apps/todos/tests/test_todo.py index bc7cedf5d..49c30947d 100644 --- a/solstone/apps/todos/tests/test_todo.py +++ b/solstone/apps/todos/tests/test_todo.py @@ -12,7 +12,10 @@ import pytest from solstone.apps.todos.todo import ( TodoChecklist, + TodoEmptyTextError, TodoItem, + TodoMovePartialError, + TodoNotMovableError, find_cross_facet_matches, get_facets_with_todos, get_todos, @@ -37,6 +40,10 @@ def _write_todos(root: Path, facet: str, day: str, items: list[dict]) -> Path: return todo_path +def _read_todos(path: Path) -> list[dict]: + return [json.loads(line) for line in path.read_text(encoding="utf-8").splitlines()] + + def test_get_todos_returns_none_when_missing(monkeypatch, journal_root): monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) assert get_todos("20240101", "personal") is None @@ -476,6 +483,157 @@ def test_checklist_cancel_entry(monkeypatch, journal_root): assert data["cancelled"] is True +def test_move_entry_cross_facet_same_day(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + _write_todos( + journal_root, + "work", + "20240105", + [ + { + "text": "Ship feature", + "nudge": "20240105T10:30", + "created_at": 1704067200000, + } + ], + ) + + created, cancelled = TodoChecklist.move_entry( + "20240105", "work", 1, "20240105", "personal" + ) + + assert created.text == "Ship feature" + assert created.nudge == "20240105T10:30" + assert created.created_at == 1704067200000 + assert cancelled.cancelled is True + assert cancelled.cancelled_reason == "moved_to_facet" + assert cancelled.moved_to == "personal" + source = _read_todos(journal_root / "facets" / "work" / "todos" / "20240105.jsonl") + dest = _read_todos( + journal_root / "facets" / "personal" / "todos" / "20240105.jsonl" + ) + assert source[0]["cancelled"] is True + assert source[0]["moved_to"] == "personal" + assert dest[0]["text"] == "Ship feature" + assert dest[0]["created_at"] == 1704067200000 + + +def test_move_entry_cross_day_same_facet_carries_completion(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + _write_todos( + journal_root, + "personal", + "20240105", + [ + { + "text": "Done thing", + "completed": True, + "created_at": 1704067200000, + } + ], + ) + + created, cancelled = TodoChecklist.move_entry( + "20240105", "personal", 1, "20240106", "personal" + ) + + assert created.completed is True + assert cancelled.completed is True + dest = _read_todos( + journal_root / "facets" / "personal" / "todos" / "20240106.jsonl" + ) + assert dest[0]["completed"] is True + + +def test_move_entry_text_override(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + _write_todos(journal_root, "work", "20240105", [{"text": "Old text"}]) + + created, cancelled = TodoChecklist.move_entry( + "20240105", "work", 1, "20240105", "personal", text="New text" + ) + + assert cancelled.text == "Old text" + assert created.text == "New text" + + +def test_move_entry_empty_text_override_preserves_source(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + source_path = _write_todos(journal_root, "work", "20240105", [{"text": "Old text"}]) + + with pytest.raises(TodoEmptyTextError): + TodoChecklist.move_entry("20240105", "work", 1, "20240105", "personal", text="") + + assert "cancelled" not in _read_todos(source_path)[0] + assert not ( + journal_root / "facets" / "personal" / "todos" / "20240105.jsonl" + ).exists() + + +def test_move_entry_rejects_identical_source_and_destination(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + _write_todos(journal_root, "personal", "20240105", [{"text": "Same place"}]) + + with pytest.raises(ValueError, match="identical"): + TodoChecklist.move_entry("20240105", "personal", 1, "20240105", "personal") + + +def test_move_entry_rejects_out_of_range(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + _write_todos(journal_root, "work", "20240105", [{"text": "Only one"}]) + + with pytest.raises(IndexError): + TodoChecklist.move_entry("20240105", "work", 2, "20240105", "personal") + + +def test_move_entry_rejects_cancelled_source(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + _write_todos( + journal_root, + "work", + "20240105", + [{"text": "Already moved", "cancelled": True}], + ) + + with pytest.raises(TodoNotMovableError, match="already cancelled"): + TodoChecklist.move_entry("20240105", "work", 1, "20240105", "personal") + + +def test_move_entry_partial_failure_preserves_source(monkeypatch, journal_root): + monkeypatch.setenv("SOLSTONE_JOURNAL", str(journal_root)) + source_path = _write_todos( + journal_root, + "work", + "20240105", + [{"text": "Keep source", "created_at": 1704067200000}], + ) + + def fail_cancel( + self: TodoChecklist, + line_number: int, + cancelled_reason: str | None = None, + moved_to: str | None = None, + ): + raise RuntimeError("cancel failed") + + monkeypatch.setattr(TodoChecklist, "cancel_entry", fail_cancel) + + with pytest.raises(TodoMovePartialError) as exc_info: + TodoChecklist.move_entry("20240105", "work", 1, "20240105", "personal") + + assert exc_info.value.src_day == "20240105" + assert exc_info.value.src_facet == "work" + assert exc_info.value.dst_day == "20240105" + assert exc_info.value.dst_facet == "personal" + assert exc_info.value.new_index == 1 + source = _read_todos(source_path) + dest = _read_todos( + journal_root / "facets" / "personal" / "todos" / "20240105.jsonl" + ) + assert source[0].get("cancelled") is None + assert dest[0]["text"] == "Keep source" + + def test_checklist_display_includes_cancelled_with_strikethrough( monkeypatch, journal_root ): diff --git a/solstone/apps/todos/todo.py b/solstone/apps/todos/todo.py index 7d3070836..c1c8ef9a4 100644 --- a/solstone/apps/todos/todo.py +++ b/solstone/apps/todos/todo.py @@ -26,6 +26,8 @@ __all__ = [ "TodoItem", "TodoError", "TodoEmptyTextError", + "TodoNotMovableError", + "TodoMovePartialError", "find_cross_facet_matches", "parse_nudge", "format_nudge", @@ -152,6 +154,47 @@ class TodoEmptyTextError(TodoError): super().__init__("todo text cannot be empty") +class TodoNotMovableError(TodoError): + """Raised when a todo cannot be moved from its source.""" + + line_number: int + + def __init__(self, line_number: int) -> None: + self.line_number = line_number + super().__init__(f"todo {line_number} is already cancelled") + + +class TodoMovePartialError(TodoError): + """Raised when a move append succeeded but source finalization failed.""" + + src_day: str + src_facet: str + line_number: int + dst_day: str + dst_facet: str + new_index: int + + def __init__( + self, + src_day: str, + src_facet: str, + line_number: int, + dst_day: str, + dst_facet: str, + new_index: int, + ) -> None: + self.src_day = src_day + self.src_facet = src_facet + self.line_number = line_number + self.dst_day = dst_day + self.dst_facet = dst_facet + self.new_index = new_index + super().__init__( + f"todo {line_number} was appended to {dst_facet}/{dst_day} " + f"as line {new_index}, but the source could not be finalized" + ) + + @dataclass(slots=True) class TodoItem: """Structured representation of a todo entry.""" @@ -340,6 +383,58 @@ class TodoChecklist: checklist = cls.load(day, facet) return modify_fn(checklist) + @classmethod + def move_entry( + cls, + src_day: str, + src_facet: str, + line_number: int, + dst_day: str, + dst_facet: str, + *, + text: str | None = None, + ) -> tuple[TodoItem, TodoItem]: + """Move a todo entry between todo files under ordered source/dest locks.""" + src_path = todo_file_path(src_day, src_facet) + dst_path = todo_file_path(dst_day, dst_facet) + if src_path == dst_path: + raise ValueError("todo move source and destination are identical") + + first, second = sorted((src_path, dst_path), key=str) + with hold_lock(first), hold_lock(second): + source = cls.load(src_day, src_facet) + _index, item = source._get_item(line_number) + if item.cancelled: + raise TodoNotMovableError(line_number) + + effective_text = item.text if text is None else text + dest = cls.load(dst_day, dst_facet) + created = dest.append_entry( + effective_text, + nudge=item.nudge, + created_at=item.created_at, + ) + + try: + if item.completed: + dest.mark_done(created.index) + cancelled = source.cancel_entry( + line_number, + cancelled_reason="moved_to_facet", + moved_to=dst_facet, + ) + except Exception as exc: + raise TodoMovePartialError( + src_day, + src_facet, + line_number, + dst_day, + dst_facet, + created.index, + ) from exc + + return created, cancelled + def save(self) -> None: """Persist the checklist to disk via durable atomic replace.""" lines = [json.dumps(item.to_jsonl(), ensure_ascii=False) for item in self.items] -- 2.51.2