diff --git a/docs/api.md b/docs/api.md index cec1d622..de319688 100644 --- a/docs/api.md +++ b/docs/api.md @@ -156,8 +156,10 @@ For streaming, use the WebSocket endpoint. ### Tasks -#### `GET /api/tasks?status=pending` -List tasks with optional status filter. Valid statuses: `pending`, `in_progress`, `done`, `deferred`, or empty (all non-done). +#### `GET /api/tasks?status=pending&tag=backend&sort=position` +List tasks. All filters optional. Statuses are configurable — see +`GET /api/task-statuses`; empty means all non-done. `sort` accepts `deadline` +(default), `updated_at`, `created_at`, or `position` (board order). #### `GET /api/tasks/search?q=keyword&status=` Full-text search on task titles and content (FTS5). Optional status filter. @@ -166,11 +168,37 @@ Full-text search on task titles and content (FTS5). Optional status filter. Response: { "tasks": [{ "id": "2026-03-01-fix-bug", "title": "Fix bug", "status": "pending", ... }] } ``` +#### `GET /api/tasks/board?limit=100&tag=` +Every lane in one round trip — the board's only read. Returns the configured +statuses plus one page of each, ordered by `position`. `total` is the lane's +true count so a column can offer "+N more"; the `done` lane is capped tighter +than the rest since it grows without bound. + +```json +Response: { + "statuses": [{ "name": "pending", "label": "Pending", "color": "#...", ... }], + "lanes": [{ "status": "pending", "total": 12, "tasks": [ ... ] }] +} +``` + +#### `GET /api/tasks/tags?include_done=false` +Distinct tags with their task counts, most-used first (filter-bar facets). + +```json +Response: { "tags": [{ "name": "backend", "count": 7 }] } +``` + #### `POST /api/tasks` -Create a task. +Create a task. Returns the created row; **409** if the duplicate guard refuses +(retry with `confirm_duplicate: true` to override), **422** if `status` names a +status that does not exist — a retry cannot fix that one, so it is kept +distinct from the collision. ```json -Request: { "title": "Fix bug", "content": "Details...", "deadline": "2026-03-01" } +Request: { "title": "Fix bug", "content": "Details...", "deadline": "2026-03-01", "tags": "backend,urgent" } +Response: { "task": { "id": "2026-03-01-fix-bug", ... }, "message": "Task created: ..." } +409: { "detail": { "reason": "duplicate", "duplicates": [ ... ], "message": "..." } } +422: { "detail": { "reason": "invalid_status", "duplicates": [], "message": "..." } } ``` #### `GET /api/tasks/{id}` @@ -181,11 +209,33 @@ Response: { "id": "2026-03-01-fix-bug", "title": "Fix bug", "status": "pending", ``` #### `PATCH /api/tasks/{id}` -Update a task. All fields are optional. `content` replaces the full markdown file; title and deadline are re-synced to SQLite. +Update a task. All fields are optional. `content` replaces the full markdown +file; title and deadline are re-synced to SQLite. Returns the full updated row. + +`deadline` and `tags` read by **presence**: omit the key to leave the field +alone, or send `""` to clear it. An invalid status is a 400 (previously +reported as a success that changed nothing). + +```json +Request: { "status": "done", "note": "Fixed in PR #123" } +Request: { "content": "# Updated Title\n\n**Deadline:** 2026-03-15\n\nNew details..." } +Request: { "deadline": "" } +Response: { "task": { ... }, "task_id": "2026-03-01-fix-bug", "updated": true } +``` + +#### `POST /api/tasks/{id}/move` +Place a task in a lane — the board's drag-and-drop write. Send *intent* +("between these two cards"), not a computed rank: the server resolves the +neighbours itself, so a stale client board can't corrupt the ordering. +`before_id` is the card that ends up directly above, `after_id` the one +directly below; omit both to append. Omit `status` to reorder in place. + +Moving into or out of `done` moves the markdown file between `active/` and +`done/` as a side effect. ```json -Request: { "status": "done", "note": "Fixed in PR #123" } -Request: { "content": "# Updated Title\n\n**Deadline:** 2026-03-15\n\nNew details..." } +Request: { "status": "in_progress", "before_id": "2026-03-01-a", "after_id": "2026-03-01-b" } +Response: { "task": { "id": "...", "status": "in_progress", "position": 3072.0, ... } } ``` ### Workflow Runs diff --git a/docs/tasks.md b/docs/tasks.md index 58467ae9..2ab541a8 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -77,6 +77,53 @@ Setting a task's status to `done` via `task_update` automatically delegates to ` This prevents orphan tasks (status=done in DB but file still in active/). +Leaving `done` delegates the same way, to the inverse path: `task_update` +routes a task whose stored status is `done` through the reopen handler, which +moves the file back to `active/` and appends a `REOPENED` line. Both +directions check the tracked-config guard *before* the status flip, so a +refused move never leaves a task whose status and file disagree. + +The inverse matters because the disagreement is silent rather than loud. +`reindex()` treats a file under `done/` as terminal by definition, so a row +pointing there with an active status is an orphan it force-resets back to +`done` — a status change that appeared to work would quietly undo itself the +next time anything reindexed. A *missing* file is worse: `reindex()` only +walks files that exist, so it never sees that row to repair it. Hence the +reopen refuses outright when the file is already gone. + +### Ordering (`position`) + +Board lanes are hand-ordered, so the order is stored rather than derived: +`tasks.position` is a sparse REAL rank, ascending (lower sorts higher in the +lane), added in migration v043 and backfilled per status. + +A move sends *intent* — "put this card between A and B" — and the server takes +the midpoint of the two neighbours' ranks. One drag is one UPDATE, with no +renumbering of the lane. If repeated midpoint inserts ever exhaust float +precision between two neighbours, that lane is re-spaced at even intervals and +the move retried. + +Two rules keep ranks meaningful: + +- **Preserved on omit.** `upsert_task(position=None)` keeps the stored rank. + Nothing in the markdown file encodes a rank, so callers that rebuild a row + from disk (`reindex`, `task_write`, the PATCH route) *cannot* supply one — + under replace semantics, appending a note would silently reset the card's + place in its lane. +- **Re-ranked across lanes.** A rank only means something relative to its own + lane, so a status change places the card at the top of its destination + instead of carrying over a number it was never ordered against. + +### Live Updates + +Every task mutation broadcasts a `task_updated` WebSocket event on the +`__global__` channel carrying the whole row (`event` is one of `created`, +`updated`, `moved`, `done`). It fires from both the HTTP routes and the tool +handlers, so a card moves on any open board whether the change came from the +web UI, another tab, or the agent working in an unrelated session. Broadcast +failures are logged and swallowed — a stale card is never worth failing a +write over. + ### FTS Index Tasks are indexed in an FTS5 virtual table (`tasks_fts`) for fast full-text search. The index is synced on every `upsert_task()` call. On startup, an integrity check compares task count vs FTS count — if they diverge, the index is automatically reseeded from the database. diff --git a/nerve/agent/streaming.py b/nerve/agent/streaming.py index b5c33e2a..fdaf3967 100644 --- a/nerve/agent/streaming.py +++ b/nerve/agent/streaming.py @@ -317,6 +317,26 @@ async def broadcast_workflow_progress( "workflow": workflow, }) + async def broadcast_task_event(self, task: dict[str, Any], event: str) -> None: + """A task row changed — ``created``/``updated``/``moved``/``done``. + + Fanned out on ``__global__`` rather than a session channel because + the task board is a global view: a card has to move whether the + mutation came from the HTTP API, another browser tab, or the agent + working in some unrelated session. ``session_id`` is None for the + same reason — the frontend drops view-scoped events addressed to a + session the client isn't looking at, and this one is for everyone. + + The whole row travels with the event so a client can upsert it + without a follow-up fetch (same shape as ``workflow_run_update``). + """ + await self.broadcast("__global__", { + "type": "task_updated", + "session_id": None, + "event": event, + "task": task, + }) + async def broadcast_error(self, session_id: str, error: str) -> None: await self.broadcast(session_id, {"type": "error", "session_id": session_id, "error": error}) @@ -338,3 +358,19 @@ async def broadcast_file_changed( # Global broadcaster instance broadcaster = StreamBroadcaster() + + +async def emit_task_event(task: dict[str, Any] | None, event: str) -> None: + """Best-effort task broadcast — never let the UI break a mutation. + + Every task write site calls this, including ones that run with no + websocket listeners at all (CLI, cron, tests). A broadcast failure is + a cosmetic problem — a stale card until the next refresh — so it is + logged at debug and swallowed rather than propagated into the write. + """ + if not task: + return + try: + await broadcaster.broadcast_task_event(task, event) + except Exception: # noqa: BLE001 + logger.debug("task event broadcast failed", exc_info=True) diff --git a/nerve/agent/tools/handlers/tasks.py b/nerve/agent/tools/handlers/tasks.py index 93648fc9..79be2391 100644 --- a/nerve/agent/tools/handlers/tasks.py +++ b/nerve/agent/tools/handlers/tasks.py @@ -119,6 +119,21 @@ def _done_dir(ctx: ToolContext) -> Path: return d +async def _emit_task_event(ctx: ToolContext, task_id: str, event: str) -> None: + """Push the post-mutation row to every connected task board. + + Re-reads the row rather than assembling a dict from local variables so + the broadcast always carries what actually landed in the database — + including the columns the handler never touched (``position``, the + refreshed ``updated_at``). + """ + if not ctx.db: + return + from nerve.agent.streaming import emit_task_event + + await emit_task_event(await ctx.db.get_task(task_id), event) + + async def task_search_handler(ctx: ToolContext, args: dict) -> ToolResult: query = args["query"] raw_status = (args.get("status", "") or "").strip().lower() @@ -185,7 +200,9 @@ async def task_create_handler(ctx: ToolContext, args: dict) -> ToolResult: # Reject unknown statuses up front with the list of valid options. err = await _validate_status(ctx, status) if err: - return ToolResult.text(err) + return ToolResult.text( + err, structured={"created": False, "reason": "invalid_status"}, + ) # Duplicate check (skip if explicitly confirmed) if not confirm: @@ -197,7 +214,17 @@ async def task_create_handler(ctx: ToolContext, args: dict) -> ToolResult: lines.append(f" - [{t['status']}] {t['title']}{deadline_str} — {t['id']}") lines.append("") lines.append("Task NOT created. To create anyway, call task_create again with confirm_duplicate=true.") - return ToolResult.text("\n".join(lines)) + return ToolResult.text( + "\n".join(lines), + structured={ + "created": False, + "reason": "duplicate", + "duplicates": [ + {"id": t["id"], "title": t["title"], "status": t["status"]} + for t in dupes + ], + }, + ) task_id = _make_task_id(title, ctx) file_path = _task_dir(ctx) / f"{task_id}.md" @@ -232,13 +259,17 @@ async def task_create_handler(ctx: ToolContext, args: dict) -> ToolResult: ) _tasks_read.add(task_id) + await _emit_task_event(ctx, task_id, "created") # Creating directly in the terminal status: route through task_done so # the file is moved into done/ and stays consistent with the done-flow. if status == TERMINAL_STATUS and ctx.db: await task_done_handler(ctx, {"task_id": task_id, "note": ""}) - return ToolResult.text(f"Task created: {task_id} (status: {status})\nFile: {file_path}") + return ToolResult.text( + f"Task created: {task_id} (status: {status})\nFile: {file_path}", + structured={"created": True, "task_id": task_id, "status": status}, + ) async def task_list_handler(ctx: ToolContext, args: dict) -> ToolResult: @@ -277,10 +308,21 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult: task_id = args["task_id"] status = (args.get("status", "") or "").strip().lower() note = args.get("note", "") - deadline = args.get("deadline", "") - raw_tags = (args.get("tags", "") or "").strip() new_title = (args.get("title", "") or "").strip() + # ``deadline`` and ``tags`` use presence, not truthiness: an absent key + # means "leave it alone", a present-but-empty one is an explicit clear. + # Clearing has to be expressible — otherwise the board's detail modal + # can add a deadline or a tag but never remove the last one. Every + # in-tree caller omits these keys entirely, so nothing else changes. + deadline = args.get("deadline") + raw_tags = args.get("tags") + if raw_tags is not None: + raw_tags = raw_tags.strip() + has_field_edits = ( + deadline is not None or raw_tags is not None or bool(new_title) + ) + # Reject unknown statuses with the list of valid options. if status: err = await _validate_status(ctx, status) @@ -291,13 +333,30 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult: if status == TERMINAL_STATUS: return await task_done_handler(ctx, {"task_id": task_id, "note": note}) + # ...and the mirror. A task leaving the terminal status has to have its + # markdown moved back out of done/ *before* anything else edits it — + # otherwise the row points into done/ with a non-done status, which + # TaskManager.reindex() treats as an orphan and force-resets to done. + if ctx.db and status: + current = await ctx.db.get_task(task_id) + if current and current.get("status") == TERMINAL_STATUS: + reopened = await task_reopen_handler( + ctx, {"task_id": task_id, "status": status, "note": note}, + ) + if reopened.is_error or not has_field_edits: + return reopened + # More fields to apply: fall through and re-read the row below + # so those edits land on the file's new location, not the stale + # done/ path. The note has already been recorded by the reopen. + note = "" + if ctx.db: task = await ctx.db.get_task(task_id) if not task: return ToolResult.text(f"Task not found: {task_id}", is_error=True) new_tags_str = "" - if raw_tags: + if raw_tags is not None: current_tags = set(parse_tags_string(task.get("tags", "") or "")) if raw_tags.startswith("+") or raw_tags.startswith("-"): for part in raw_tags.split(","): @@ -310,7 +369,7 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult: else: new_tags_str = tags_to_string(parse_tags_string(raw_tags)) - if ctx.workspace and (note or deadline or raw_tags or new_title): + if ctx.workspace and (note or has_field_edits): file_path = ctx.workspace / task["file_path"] ensure_path_not_tracked_config(file_path, "write") if file_path.exists(): @@ -327,7 +386,11 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult: content = re.sub(r"\*\*Deadline:\*\* .*", f"**Deadline:** {deadline}", content) else: content = content.replace("\n\n", f"\n**Deadline:** {deadline}\n\n", 1) - if raw_tags: + elif deadline is not None: + # Explicit clear — drop the line entirely so the file + # stops projecting a deadline back onto the column. + content = re.sub(r"\*\*Deadline:\*\* .*\n?", "", content, count=1) + if new_tags_str: display_tags = ", ".join(parse_tags_string(new_tags_str)) if "**Tags:**" in content: content = re.sub(r"\*\*Tags:\*\* .*", f"**Tags:** {display_tags}", content) @@ -341,18 +404,22 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult: ) if "**Tags:**" not in content: content = content.replace("\n\n", f"\n**Tags:** {display_tags}\n\n", 1) + elif raw_tags is not None: + # Every tag removed (an empty set, or "-last_tag"). + content = re.sub(r"\*\*Tags:\*\* .*\n?", "", content, count=1) await asyncio.to_thread( file_path.write_text, content, encoding="utf-8", ) final_title = new_title or task["title"] final_status = status or task["status"] - # Only the columns this call edits. The rest keep their - # stored values, so nothing has to be read back off ``task``. + # Presence carries through to the row the same way it does to + # the file above: an omitted column keeps its stored value, a + # present-but-empty one clears it. edits: dict = {} - if deadline: - edits["deadline"] = deadline - if raw_tags: + if deadline is not None: + edits["deadline"] = deadline or None + if raw_tags is not None: edits["tags"] = new_tags_str await ctx.db.upsert_task( task_id=task_id, @@ -362,13 +429,15 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult: content=content, **edits, ) + await _emit_task_event(ctx, task_id, "updated") return ToolResult.text(f"Task {task_id} updated.") # Fall back to metadata updates when no task file was changed. if status: await ctx.db.update_task_status(task_id, status) - if raw_tags: + if raw_tags is not None: await ctx.db.update_task_tags(task_id, new_tags_str) + await _emit_task_event(ctx, task_id, "updated") return ToolResult.text(f"Task {task_id} updated.") @@ -451,6 +520,7 @@ async def task_write_handler(ctx: ToolContext, args: dict) -> ToolResult: **edits, ) + await _emit_task_event(ctx, task_id, "updated") return ToolResult.text(f"Task {task_id} written ({len(new_content)} chars).") @@ -502,9 +572,95 @@ async def task_done_handler(ctx: ToolContext, args: dict) -> ToolResult: content=content, ) + await _emit_task_event(ctx, task_id, "done") + return ToolResult.text(f"Task {task_id} marked as done.") +async def task_reopen_handler(ctx: ToolContext, args: dict) -> ToolResult: + """Mirror of ``task_done``: bring a completed task back to life. + + ``task_done`` only moves a file one way, into done/, and nothing moved it + back. That was fine while the only way out of ``done`` was never taking + it, but the board lets a card be dragged out of the Done lane. Without + the file travelling with the status, the row points into done/ while + claiming to be active, and + ``TaskManager.reindex()`` classifies exactly that as an orphan and + force-resets it to done — silently undoing the move on the next reindex. + + Not registered as a standalone tool: ``task_update`` routes here on its + own when it sees a task leaving the terminal status, so the agent-facing + surface stays the same size. + """ + task_id = args["task_id"] + status = (args.get("status", "") or "").strip().lower() or DEFAULT_STATUS + note = args.get("note", "") + + if status == TERMINAL_STATUS: + return ToolResult.text( + "task_reopen needs a non-terminal status — use task_done to complete a task.", + is_error=True, + ) + + err = await _validate_status(ctx, status) + if err: + return ToolResult.text(err, is_error=True) + + if not ctx.db: + return ToolResult.text("Database not available.", is_error=True) + + task = await ctx.db.get_task(task_id) + if not task: + return ToolResult.text(f"Task not found: {task_id}", is_error=True) + if task["status"] != TERMINAL_STATUS: + return ToolResult.text( + f"Task {task_id} is not done (status: {task['status']}) — nothing to reopen.", + is_error=True, + ) + + # Same ordering rationale as task_done: the refusal has to land before + # the status flip, or a rejected move leaves a task marked active whose + # file never left done/. + src = ctx.workspace / task["file_path"] if ctx.workspace else None + if src is not None: + ensure_path_not_tracked_config(src, "move") + # A missing file is a refusal for that same reason. With nothing to + # move, the flip on its own leaves the row claiming an active status + # while file_path still points into done/ — and reindex() cannot + # repair it, because it only walks files that exist. + if not src.exists(): + return ToolResult.text( + f"Task {task_id} is done but its file is missing " + f"({task['file_path']}) — not reopening it.", + is_error=True, + ) + + await ctx.db.update_task_status(task_id, status) + + if src is not None: + # Re-checked because the guard above is not atomic with the move. + if src.exists(): + content = await asyncio.to_thread(src.read_text, encoding="utf-8") + today = datetime.now(timezone.utc).strftime("%Y-%m-%d") + suffix = f" — {note}" if note else "" + content += f"\n- {today}: REOPENED ({status}){suffix}" + + dst = _task_dir(ctx) / src.name + + await asyncio.to_thread(move_task_file, src, dst, content) + + await ctx.db.upsert_task( + task_id=task_id, + file_path=str(dst.relative_to(ctx.workspace)), + title=task["title"], + status=status, + content=content, + ) + + await _emit_task_event(ctx, task_id, "updated") + return ToolResult.text(f"Task {task_id} reopened (status: {status}).") + + async def task_status_list_handler(ctx: ToolContext, args: dict) -> ToolResult: if not ctx.db: return ToolResult.text("Database not available.") diff --git a/nerve/agent/tools/registry.py b/nerve/agent/tools/registry.py index ed9c3049..f6078c38 100644 --- a/nerve/agent/tools/registry.py +++ b/nerve/agent/tools/registry.py @@ -64,9 +64,12 @@ class ToolResult: ``content`` is a list of MCP content blocks — typically ``[{"type": "text", "text": "..."}]``. - ``structured`` is reserved for future use; the current adapters ignore - it. Tools that want to return JSON payloads should still serialize them - into a text block for now so behavior is identical across runtimes. + ``structured`` carries the same outcome as data, for in-process callers + that need to branch on it — an HTTP route invoking a handler through + the registry can read a task id or a duplicate list from here instead + of parsing it back out of the prose. The tool adapters ignore it, so an + agent sees identical behavior whether a handler sets it or not; text + remains the contract for anything crossing the tool boundary. """ content: list[dict] @@ -74,9 +77,27 @@ class ToolResult: structured: dict | None = None @classmethod - def text(cls, message: str, *, is_error: bool = False) -> "ToolResult": + def text( + cls, + message: str, + *, + is_error: bool = False, + structured: dict | None = None, + ) -> "ToolResult": """Convenience: build a ToolResult wrapping a single text block.""" - return cls(content=[{"type": "text", "text": message}], is_error=is_error) + return cls( + content=[{"type": "text", "text": message}], + is_error=is_error, + structured=structured, + ) + + @property + def text_content(self) -> str: + """Flatten the content blocks back into plain text.""" + return "\n".join( + block.get("text", "") for block in self.content + if block.get("type") == "text" + ) def to_dict(self) -> dict: """Serialize to the dict shape Claude Agent SDK tools return.""" diff --git a/nerve/agent/tools/schemas.py b/nerve/agent/tools/schemas.py index ffc3a563..8c921e0d 100644 --- a/nerve/agent/tools/schemas.py +++ b/nerve/agent/tools/schemas.py @@ -112,15 +112,16 @@ "description": "Update note to append to the task file", "default": "", }, + # No "default" on deadline/tags: these two use presence rather than + # truthiness, so an omitted key and an empty string mean different + # things. Declaring a default invites callers to send "" as a no-op. "deadline": { "type": "string", - "description": "New deadline in YYYY-MM-DD format", - "default": "", + "description": "New deadline in YYYY-MM-DD format. Send an empty string to remove the deadline; omit the field entirely to leave it unchanged.", }, "tags": { "type": "string", - "description": "Replace tags (comma-separated). Use '+tag' to add, '-tag' to remove, or 'tag1,tag2' to set.", - "default": "", + "description": "Replace tags (comma-separated). Use '+tag' to add, '-tag' to remove, or 'tag1,tag2' to set. Send an empty string to remove all tags; omit the field entirely to leave them unchanged.", }, "title": { "type": "string", diff --git a/nerve/db/migrations/v043_task_position.py b/nerve/db/migrations/v043_task_position.py new file mode 100644 index 00000000..8abccb38 --- /dev/null +++ b/nerve/db/migrations/v043_task_position.py @@ -0,0 +1,60 @@ +"""V43: manual ordering rank for the task board. + +The board lets a card be dragged to an arbitrary slot inside its lane, so +lane order becomes user-owned state rather than something derived from +deadline/created_at. ``position`` stores it as a *sparse* REAL rank: a +move takes the midpoint of its two neighbours (see +:meth:`nerve.db.tasks.TaskStore.move_task`), so one drag is one UPDATE +instead of a renumber of the whole lane. + +Existing rows are backfilled per status, walking the order the task list +already used (deadline first, then newest) with a wide gap between ranks. +Without the backfill every row would sit at the column default of 0 and +lane order would be arbitrary until each card had been dragged once. +""" + +from __future__ import annotations + +import logging + +import aiosqlite + +logger = logging.getLogger(__name__) + +# Spacing between backfilled ranks. Keep in sync with +# ``nerve.db.tasks.POSITION_GAP`` — duplicated rather than imported so the +# migration keeps describing the schema as it was at v43 even if the +# runtime constant is retuned later. +_GAP = 1024.0 + + +async def up(db: aiosqlite.Connection) -> None: + await db.execute("ALTER TABLE tasks ADD COLUMN position REAL NOT NULL DEFAULT 0") + await db.execute( + "CREATE INDEX IF NOT EXISTS idx_tasks_status_position " + "ON tasks(status, position)" + ) + + # One pass over every task, grouped by lane. The secondary keys match + # ``TaskStore._SORT_CLAUSES["deadline"]`` so the board's initial order + # is the order people were already looking at. + async with db.execute( + "SELECT id, status FROM tasks " + "ORDER BY status ASC, deadline ASC NULLS LAST, created_at DESC, id DESC" + ) as cursor: + rows = await cursor.fetchall() + + updates: list[tuple[float, str]] = [] + current_lane: object = object() # sentinel: never equal to a status string + rank = 0.0 + for task_id, status in rows: + if status != current_lane: + current_lane = status + rank = 0.0 + rank += _GAP + updates.append((rank, task_id)) + + if updates: + await db.executemany("UPDATE tasks SET position = ? WHERE id = ?", updates) + + logger.info("v043: added tasks.position, backfilled %d row(s)", len(updates)) diff --git a/nerve/db/tasks.py b/nerve/db/tasks.py index 86e7e6bd..70deb02f 100644 --- a/nerve/db/tasks.py +++ b/nerve/db/tasks.py @@ -6,6 +6,16 @@ from datetime import datetime, timezone +# Spacing between task ranks (``tasks.position``). Wide enough that a +# midpoint insert has ~50 levels of headroom before float precision runs +# out, so renormalization is a theoretical path rather than a routine one. +POSITION_GAP = 1024.0 + +# Two neighbours closer than this can't yield a meaningful midpoint, so a +# move between them re-spaces the lane first. +_MIN_POSITION_GAP = 1e-6 + + class _Keep: """Type of the :data:`KEEP` sentinel.""" @@ -49,25 +59,28 @@ async def upsert_task( deadline: str | None | _Keep = KEEP, tags: str | _Keep = KEEP, content: str = "", + position: float | None = None, ) -> 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. + ``source``, ``source_url``, ``deadline``, ``tags`` and ``position`` + are preserve-on-omit. An omitted column keeps its stored value. To + clear one, pass it: ``None`` for the nullable columns, ``""`` for + ``tags``. + + These five columns need that rule because a markdown file does not + always carry them. No file carries ``position``: board order lives + only in this column. A file carries the other four only while it has + the frontmatter for them. Under a full replace, every caller must + read the row and pass all five 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 the other four when it moved a file into done/. + ``test_task_position.py``, ``test_task_reindex.py`` and + ``test_task_completion.py`` hold the regression coverage. """ now = datetime.now(timezone.utc).isoformat() async with self._atomic(): @@ -79,14 +92,17 @@ async def upsert_task( source_url = _resolve_kept(source_url, stored, "source_url", None) deadline = _resolve_kept(deadline, stored, "deadline", None) tags = _resolve_kept(tags, stored, "tags", "") + if position is None: + position = await self._resolve_upsert_position(stored, status) await self.db.execute( - """INSERT INTO tasks (id, file_path, title, status, source, source_url, deadline, tags, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """INSERT INTO tasks (id, file_path, title, status, source, source_url, deadline, tags, position, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(id) DO UPDATE SET file_path=excluded.file_path, title=excluded.title, status=excluded.status, source=excluded.source, source_url=excluded.source_url, - deadline=excluded.deadline, tags=excluded.tags, updated_at=?""", - (task_id, file_path, title, status, source, source_url, deadline, tags, now, now, now), + deadline=excluded.deadline, tags=excluded.tags, + position=excluded.position, updated_at=?""", + (task_id, file_path, title, status, source, source_url, deadline, tags, position, now, now, now), ) # Sync FTS index — include tags and content so they're all searchable. # The join key is the RAW task_id; readers join on `f.task_id = t.id`. @@ -105,21 +121,177 @@ async def get_task(self, task_id: str) -> dict | None: row = await cursor.fetchone() return dict(row) if row else None + # ── Board ordering (tasks.position) ────────────────────────────────── + # + # Lanes sort by ``position ASC``, so a *lower* rank is *higher* in the + # lane. Every helper below assumes it is running inside ``_atomic()``: + # they read on the shared connection between the caller's writes, which + # is only consistent while the caller holds the write lock. + + async def _lane_top_position(self, status: str) -> float: + """A rank that lands above everything currently in ``status``.""" + async with self.db.execute( + "SELECT MIN(position) FROM tasks WHERE status = ?", (status,), + ) as cursor: + lane_min = (await cursor.fetchone())[0] + # Going negative is fine: this is an ordering key, not a count. + return POSITION_GAP if lane_min is None else float(lane_min) - POSITION_GAP + 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 = ?", + "SELECT status, position, source, source_url, deadline, tags " + "FROM tasks WHERE id = ?", (task_id,), ) as cursor: row = await cursor.fetchone() return dict(row) if row else None + async def _resolve_upsert_position( + self, stored: dict | None, status: str, + ) -> float: + """Rank for an upsert that supplied none: keep it, or mint one. + + A rank is preserved only *within* a lane. A row whose status is + changing gets re-ranked to the top of its destination, because a + rank carried across lanes lands the card at an arbitrary depth of a + list it was never ordered against — the same rule + :meth:`update_task_status` applies, kept here so it also holds for + the callers that flip status through a full-row upsert instead + (``task_update`` with a note, the PATCH route saving content). + """ + if stored is not None and stored["status"] == status: + return float(stored["position"]) + # Brand new task, or one changing lanes — surface it at the top, + # where whoever just created or moved it is looking. + return await self._lane_top_position(status) + + async def _neighbour_position( + self, neighbour_id: str | None, lane: str, moving_id: str, + ) -> float | None: + """Rank of an anchor card, or None if it can't anchor this move. + + An anchor is ignored when it is missing, is the moved card itself, + or has since left the lane — all of which happen routinely when a + drag lands against a board the client rendered moments ago. + """ + if not neighbour_id or neighbour_id == moving_id: + return None + async with self.db.execute( + "SELECT position FROM tasks WHERE id = ? AND status = ?", + (neighbour_id, lane), + ) as cursor: + row = await cursor.fetchone() + return float(row[0]) if row else None + + async def _renormalize_lane(self, lane: str) -> None: + """Re-space a lane at ``POSITION_GAP`` intervals, order preserved.""" + async with self.db.execute( + "SELECT id FROM tasks WHERE status = ? " + "ORDER BY position ASC, created_at DESC, id DESC", + (lane,), + ) as cursor: + ids = [row[0] async for row in cursor] + if ids: + await self.db.executemany( + "UPDATE tasks SET position = ? WHERE id = ?", + [((i + 1) * POSITION_GAP, tid) for i, tid in enumerate(ids)], + ) + + async def _compute_move_position( + self, task_id: str, lane: str, + before_id: str | None, after_id: str | None, + ) -> float: + """Resolve "between these two cards" into a concrete rank.""" + for attempt in (0, 1): + above = await self._neighbour_position(before_id, lane, task_id) + below = await self._neighbour_position(after_id, lane, task_id) + + if above is not None and below is not None: + gap = below - above + if gap >= _MIN_POSITION_GAP: + return (above + below) / 2.0 + if gap >= 0 and attempt == 0: + # Ranks have converged after many midpoint inserts. + # Re-space once, then take a real midpoint on the retry. + await self._renormalize_lane(lane) + continue + # gap < 0 means the anchors arrived swapped; re-spacing + # preserves order so it can't help. Fall through to the tail. + elif above is not None: + return above + POSITION_GAP + elif below is not None: + return below - POSITION_GAP + break + + # No usable anchor: append to the end of the lane. + async with self.db.execute( + "SELECT MAX(position) FROM tasks WHERE status = ? AND id != ?", + (lane, task_id), + ) as cursor: + tail = (await cursor.fetchone())[0] + return POSITION_GAP if tail is None else float(tail) + POSITION_GAP + + async def move_task( + self, + task_id: str, + *, + status: str | None = None, + before_id: str | None = None, + after_id: str | None = None, + ) -> dict | None: + """Place a task at an explicit slot in a lane; return the new row. + + The client sends *intent* — "put this between A and B" — instead of + a computed rank. Lanes are paginated, so the client may not be + holding the neighbours' true ranks, and resolving them server-side + means two people dragging at once converge on a sane order rather + than overwriting each other with stale absolute values. + + ``before_id`` is the card that ends up directly **above** the moved + task, ``after_id`` the one directly **below**; either may be None + for "top of lane" / "bottom of lane". Returns None if the task does + not exist. + + Note this writes ``status`` directly rather than going through + :meth:`update_task_status` — a move already carries an explicit + rank, so it must not be re-ranked to the top of the lane on top of + it. Callers are responsible for any *file* movement a status change + implies (see ``task_done`` / ``task_reopen``). + """ + async with self._atomic(): + async with self.db.execute( + "SELECT status FROM tasks WHERE id = ?", (task_id,), + ) as cursor: + row = await cursor.fetchone() + if row is None: + return None + + lane = status or row[0] + position = await self._compute_move_position( + task_id, lane, before_id, after_id, + ) + now = datetime.now(timezone.utc).isoformat() + await self.db.execute( + "UPDATE tasks SET status = ?, position = ?, updated_at = ? WHERE id = ?", + (lane, position, now, task_id), + ) + async with self.db.execute( + "SELECT * FROM tasks WHERE id = ?", (task_id,), + ) as cursor: + moved = await cursor.fetchone() + return dict(moved) if moved else None + # Supported sort keys → ORDER BY clause. Keep deterministic with a # secondary key so equal timestamps don't flicker between pages. _SORT_CLAUSES = { "deadline": "deadline ASC NULLS LAST, created_at DESC, id DESC", "updated_at": "updated_at DESC, id DESC", "created_at": "created_at DESC, id DESC", + # Board order. Rows predating v043's backfill (or written by a + # caller that never set a rank) share position 0, so the tiebreak + # keeps them in the list view's familiar newest-first order. + "position": "position ASC, created_at DESC, id DESC", } async def list_tasks( @@ -197,11 +369,33 @@ async def count_tasks( return row[0] if row else 0 async def update_task_status(self, task_id: str, status: str) -> None: + """Move a task to another status, re-ranking it into the new lane. + + A rank only means anything relative to its own lane, so carrying + the old one across a status change would drop the card at an + arbitrary depth of its destination — the agent marking something + in_progress would land it in the middle of the lane rather than + somewhere a person would look. Same-lane calls skip the re-rank so + a redundant update doesn't reshuffle the board. + """ now = datetime.now(timezone.utc).isoformat() - await self._write( - "UPDATE tasks SET status = ?, updated_at = ? WHERE id = ?", - (status, now, task_id), - ) + async with self._atomic(): + async with self.db.execute( + "SELECT status FROM tasks WHERE id = ?", (task_id,), + ) as cursor: + row = await cursor.fetchone() + if row is None: + return + if row[0] == status: + await self.db.execute( + "UPDATE tasks SET updated_at = ? WHERE id = ?", (now, task_id), + ) + return + position = await self._lane_top_position(status) + await self.db.execute( + "UPDATE tasks SET status = ?, position = ?, updated_at = ? WHERE id = ?", + (status, position, now, task_id), + ) async def update_task_tags(self, task_id: str, tags: str) -> None: now = datetime.now(timezone.utc).isoformat() @@ -210,6 +404,34 @@ async def update_task_tags(self, task_id: str, tags: str) -> None: (tags, now, task_id), ) + async def count_tasks_by_status(self) -> dict[str, int]: + """Task counts keyed by status — one query for every board lane.""" + async with self.db.execute( + "SELECT status, COUNT(*) FROM tasks GROUP BY status", + ) as cursor: + return {row[0]: row[1] async for row in cursor} + + async def distinct_task_tags(self, include_done: bool = False) -> list[dict]: + """Every distinct tag with its task count, most-used first. + + ``tags`` is a comma-separated TEXT column, so the split happens in + Python rather than in a recursive CTE: the table is small (hundreds + of rows), and the SQL version would be both slower to run and much + harder to read for no benefit at this scale. + """ + where = "" if include_done else " WHERE status != 'done'" + counts: dict[str, int] = {} + async with self.db.execute(f"SELECT tags FROM tasks{where}") as cursor: + async for row in cursor: + for raw in (row[0] or "").split(","): + tag = raw.strip().lower() + if tag: + counts[tag] = counts.get(tag, 0) + 1 + return [ + {"name": name, "count": count} + for name, count in sorted(counts.items(), key=lambda kv: (-kv[1], kv[0])) + ] + # ── FTS query building ─────────────────────────────────────────────── # Anything that is NOT a word character (unicode letters, digits, diff --git a/nerve/gateway/routes/tasks.py b/nerve/gateway/routes/tasks.py index 3b532e75..0451c982 100644 --- a/nerve/gateway/routes/tasks.py +++ b/nerve/gateway/routes/tasks.py @@ -7,8 +7,9 @@ from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel +from nerve.agent.streaming import emit_task_event from nerve.config import ensure_path_not_tracked_config, get_config -from nerve.db.task_statuses import STATUS_NAME_RE, normalize_color +from nerve.db.task_statuses import STATUS_NAME_RE, TERMINAL_STATUS, normalize_color from nerve.gateway.auth import require_auth from nerve.gateway.routes._deps import ( build_route_tool_context, @@ -25,14 +26,41 @@ class TaskCreateRequest(BaseModel): source: str = "manual" source_url: str = "" deadline: str = "" + tags: str = "" + status: str = "" + confirm_duplicate: bool = False class TaskUpdateRequest(BaseModel): + """Partial task edit. + + ``deadline`` and ``tags`` are ``None``-by-default rather than + ``""``-by-default: the handler reads them by *presence*, so omitting + the key leaves the field alone while sending an empty string clears + it. The remaining fields keep truthiness semantics — there is no + meaningful "clear" for a title, a status, or an appended note. + """ + status: str = "" note: str = "" - deadline: str = "" content: str = "" title: str = "" + deadline: str | None = None + tags: str | None = None + + +class TaskMoveRequest(BaseModel): + """Place a task in a lane, relative to its new neighbours. + + ``before_id`` is the card that ends up directly above the moved task, + ``after_id`` the one directly below; omit both to append to the lane. + Ranks are resolved server-side from these anchors — see + :meth:`nerve.db.tasks.TaskStore.move_task`. + """ + + status: str | None = None + before_id: str | None = None + after_id: str | None = None class TaskStatusCreateRequest(BaseModel): @@ -49,12 +77,19 @@ class TaskStatusUpdateRequest(BaseModel): sort_order: int | None = None -_ALLOWED_SORTS = {"deadline", "updated_at", "created_at"} +_ALLOWED_SORTS = {"deadline", "updated_at", "created_at", "position"} + +# Per-lane page size for the board. Done accumulates without bound and is +# collapsed by default in the UI, so it gets a tighter cap — every lane +# reports its true ``total`` so the column can offer "+N more". +_BOARD_LANE_LIMIT = 100 +_BOARD_DONE_LANE_LIMIT = 25 @router.get("/api/tasks") async def list_tasks( status: str = "", + tag: str = "", sort: str = "deadline", limit: int = 50, offset: int = 0, @@ -68,10 +103,11 @@ async def list_tasks( sort = "deadline" status_filter = status or None + tag_filter = tag.strip().lower() or None tasks = await deps.db.list_tasks( - status=status_filter, sort=sort, limit=limit, offset=offset, + status=status_filter, tag=tag_filter, sort=sort, limit=limit, offset=offset, ) - total = await deps.db.count_tasks(status=status_filter) + total = await deps.db.count_tasks(status=status_filter, tag=tag_filter) return {"tasks": tasks, "total": total, "limit": limit, "offset": offset} @@ -84,6 +120,63 @@ async def search_tasks(q: str, status: str = "", user: dict = Depends(require_au return {"tasks": tasks, "total": len(tasks), "limit": len(tasks), "offset": 0} +# ── Board ──────────────────────────────────────────────────────────────── +# +# NOTE: /board and /tags must stay above /{task_id} — FastAPI matches in +# declaration order, so a dynamic segment declared first would swallow both. + + +@router.get("/api/tasks/board") +async def task_board( + limit: int = _BOARD_LANE_LIMIT, + tag: str = "", + user: dict = Depends(require_auth), +): + """Every lane in one round trip: statuses + their ordered tasks. + + The board needs one page of each configured status plus that status's + full count. Fetching it here rather than as N parallel filtered calls + keeps the lanes consistent with each other (a task that moves mid-load + can't appear in two lanes or neither) and keeps the client from having + to know the status list before it can start fetching. + """ + deps = get_deps() + limit = max(1, min(limit, 200)) + tag_filter = tag.strip().lower() or None + + statuses = await deps.db.list_task_statuses() + # One GROUP BY covers every lane's total; only a tag filter (which the + # grouped count can't express) needs the per-lane fallback. + counts = {} if tag_filter else await deps.db.count_tasks_by_status() + + lanes = [] + for status_def in statuses: + name = status_def["name"] + lane_limit = ( + min(limit, _BOARD_DONE_LANE_LIMIT) if name == TERMINAL_STATUS else limit + ) + tasks = await deps.db.list_tasks( + status=name, tag=tag_filter, sort="position", limit=lane_limit, + ) + total = ( + await deps.db.count_tasks(status=name, tag=tag_filter) + if tag_filter + else counts.get(name, 0) + ) + lanes.append({"status": name, "total": total, "tasks": tasks}) + + return {"statuses": statuses, "lanes": lanes} + + +@router.get("/api/tasks/tags") +async def list_task_tags( + include_done: bool = False, user: dict = Depends(require_auth), +): + """Tag facets for the board filter bar, most-used first.""" + deps = get_deps() + return {"tags": await deps.db.distinct_task_tags(include_done=include_done)} + + @router.post("/api/tasks") async def create_task(req: TaskCreateRequest, user: dict = Depends(require_auth)): # Route into the unified handler surface via the live registry — no @@ -97,9 +190,75 @@ async def create_task(req: TaskCreateRequest, user: dict = Depends(require_auth) "source": req.source, "source_url": req.source_url, "deadline": req.deadline, + "tags": req.tags, + "status": req.status, + "confirm_duplicate": req.confirm_duplicate, }, ) - return result.to_dict() + + # The handler reports its outcome as data as well as prose; branch on + # that rather than on the wording of the message. + outcome = result.structured or {} + message = result.text_content + if not outcome.get("created"): + # The duplicate guard is a refusal, not a success — say so with a + # status code instead of a 200 carrying an apology. The body keeps + # the near-matches so the client can offer "create anyway". + # + # An unknown status is the other way this refuses, and it is a bad + # field rather than a collision: offering "create anyway" cannot fix + # it. Give it the 422 that /move already returns for the same + # mistake, so a client can tell the two apart by status code. + reason = outcome.get("reason", "refused") + raise HTTPException( + status_code=422 if reason == "invalid_status" else 409, + detail={ + "message": message, + "reason": reason, + "duplicates": outcome.get("duplicates", []), + }, + ) + + deps = get_deps() + task = await deps.db.get_task(outcome["task_id"]) + return {"task": task, "message": message} + + +@router.post("/api/tasks/{task_id}/move") +async def move_task( + task_id: str, req: TaskMoveRequest, user: dict = Depends(require_auth), +): + """Reorder a task within a lane, or move it to another one.""" + deps = get_deps() + task = await deps.db.get_task(task_id) + if not task: + raise HTTPException(status_code=404, detail="Task not found") + + target = (req.status or "").strip().lower() or task["status"] + if target not in await deps.db.task_status_names(): + raise HTTPException(status_code=422, detail=f"Unknown status: '{target}'") + + # A lane change is more than a column write — `done` owns its own + # directory on disk. Hand the status flip to task_update, which routes + # to task_done / task_reopen so the markdown travels with the status, + # then stamp the rank on top of whatever it left behind. + if target != task["status"]: + result = await get_tool_registry().invoke( + "task_update", + build_route_tool_context(), + {"task_id": task_id, "status": target}, + ) + if result.is_error: + raise HTTPException(status_code=409, detail=result.text_content) + + moved = await deps.db.move_task( + task_id, status=target, before_id=req.before_id, after_id=req.after_id, + ) + if not moved: + raise HTTPException(status_code=404, detail="Task not found") + + await emit_task_event(moved, "moved") + return {"task": moved} @router.get("/api/tasks/{task_id}") @@ -163,23 +322,41 @@ async def update_task(task_id: str, req: TaskUpdateRequest, user: dict = Depends **edits, ) - # Update status/note/deadline/title via the unified handler (may + # Update status/note/deadline/tags/title via the unified handler (may # move the file for "done" — the handler routes to task_done in that - # case so the FTS index stays consistent). - if req.status or req.note or req.deadline or req.title: - await get_tool_registry().invoke( - "task_update", - build_route_tool_context(), - { - "task_id": task_id, - "status": req.status, - "note": req.note, - "deadline": req.deadline, - "title": req.title, - }, + # case, and to task_reopen on the way back out, so the FTS index and + # the file's directory stay consistent). + # + # Fields are added by presence, not truthiness: the handler reads + # deadline/tags that way, so forwarding an unset field as "" would turn + # "don't touch this" into "clear this". + payload: dict = {"task_id": task_id} + if req.status: + payload["status"] = req.status + if req.note: + payload["note"] = req.note + if req.title: + payload["title"] = req.title + if req.deadline is not None: + payload["deadline"] = req.deadline + if req.tags is not None: + payload["tags"] = req.tags + + if len(payload) > 1: + result = await get_tool_registry().invoke( + "task_update", build_route_tool_context(), payload, ) - - return {"task_id": task_id, "updated": True} + # Previously swallowed: an unknown status returned {"updated": true} + # while nothing had changed. Surface the handler's own message. + if result.is_error: + raise HTTPException(status_code=400, detail=result.text_content) + + updated = await deps.db.get_task(task_id) + await emit_task_event(updated, "updated") + # ``task_id``/``updated`` are kept for existing callers; ``task`` is the + # full post-write row so an optimistic client can reconcile without a + # follow-up GET. + return {"task": updated, "task_id": task_id, "updated": True} # ── Configurable task statuses ─────────────────────────────────────────── diff --git a/tests/test_task_board_api.py b/tests/test_task_board_api.py new file mode 100644 index 00000000..09b03c14 --- /dev/null +++ b/tests/test_task_board_api.py @@ -0,0 +1,390 @@ +"""HTTP tests for ``/api/tasks*`` (gateway/routes/tasks.py). + +These are the first route-level tests for the task API — until the board +went in, everything under ``/api/tasks`` was covered only through the +handler layer, so the routes' own behaviour (status codes, clamping, +declaration order, the response envelopes the frontend reads) was +unverified. + +Pattern follows ``TestWorkflowRunRoutes`` in test_workflow_routes.py: a +minimal FastAPI app with the real router, auth disabled via an empty +jwt_secret, and a real tool registry behind a stub engine so the routes +exercise the genuine handler path rather than a mock of it. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +import pytest_asyncio + +from nerve.db import Database + + +@pytest.mark.asyncio +class TestTaskBoardRoutes: + @pytest_asyncio.fixture + async def setup(self, db: Database, tmp_path): + from fastapi import FastAPI + from fastapi.testclient import TestClient + + import nerve.config as cfg_mod + from nerve.agent.tools import build_default_registry + from nerve.config import NerveConfig + from nerve.gateway.routes._deps import init_deps + from nerve.gateway.routes.tasks import router as tasks_router + + workspace = tmp_path / "ws" + (workspace / "memory" / "tasks" / "active").mkdir(parents=True) + (workspace / "memory" / "tasks" / "done").mkdir(parents=True) + + # require_auth reads get_config().auth.jwt_secret — an empty secret + # makes it a no-op. + cfg = NerveConfig() + cfg.workspace = workspace + cfg.auth.jwt_secret = "" + cfg_mod._config = cfg + + # build_route_tool_context() pulls collaborators off the engine; the + # task handlers only need workspace/db/config, so the rest are None. + engine = SimpleNamespace( + config=cfg, + registry=build_default_registry(), + _memory_bridge=None, + _xmemory_bridge=None, + _skill_manager=None, + ) + init_deps(engine=engine, db=db) # type: ignore[arg-type] + + app = FastAPI() + app.include_router(tasks_router) + + yield SimpleNamespace( + client=TestClient(app), db=db, workspace=workspace, cfg=cfg, + ) + + cfg_mod._config = None + + async def _create(self, setup, title: str, **body) -> str: + body.setdefault("content", "body") + body.setdefault("confirm_duplicate", True) + resp = setup.client.post("/api/tasks", json={"title": title, **body}) + assert resp.status_code == 200, resp.text + return resp.json()["task"]["id"] + + # ── Board envelope ─────────────────────────────────────────────────── + + async def test_board_returns_a_lane_per_configured_status(self, setup): + resp = setup.client.get("/api/tasks/board") + + assert resp.status_code == 200 + body = resp.json() + lane_names = [lane["status"] for lane in body["lanes"]] + assert lane_names == [s["name"] for s in body["statuses"]] + # Lanes follow the configured display order, so the board can render + # them without sorting. + assert "pending" in lane_names and "done" in lane_names + + async def test_board_lane_carries_tasks_and_a_true_total(self, setup): + for i in range(3): + await self._create(setup, f"Board task {i}") + + body = setup.client.get("/api/tasks/board?limit=2").json() + pending = next(l for l in body["lanes"] if l["status"] == "pending") + + assert len(pending["tasks"]) == 2, "lane should honour the page limit" + assert pending["total"] == 3, "total must count beyond the page" + + async def test_board_orders_lanes_by_position(self, setup): + first = await self._create(setup, "Board alpha") + second = await self._create(setup, "Board beta") + + setup.client.post(f"/api/tasks/{second}/move", json={"before_id": first}) + + body = setup.client.get("/api/tasks/board").json() + pending = next(l for l in body["lanes"] if l["status"] == "pending") + assert [t["id"] for t in pending["tasks"]] == [first, second] + + async def test_board_limit_is_clamped(self, setup): + await self._create(setup, "Clamp me") + # Out-of-range values must not reach the DB as-is. + assert setup.client.get("/api/tasks/board?limit=99999").status_code == 200 + assert setup.client.get("/api/tasks/board?limit=0").status_code == 200 + + async def test_board_tag_filter_narrows_lanes_and_totals(self, setup): + await self._create(setup, "Tagged one", tags="backend") + await self._create(setup, "Untagged one") + + body = setup.client.get("/api/tasks/board?tag=backend").json() + pending = next(l for l in body["lanes"] if l["status"] == "pending") + + assert [t["title"] for t in pending["tasks"]] == ["Tagged one"] + # The total has to respect the filter too, or the lane offers to load + # "+N more" tasks that the filter would exclude. + assert pending["total"] == 1 + + async def test_board_route_is_not_shadowed_by_the_id_route(self, setup): + """/board and /tags must stay declared above /{task_id}.""" + body = setup.client.get("/api/tasks/board").json() + assert "lanes" in body, "GET /api/tasks/board resolved to the detail route" + + tags = setup.client.get("/api/tasks/tags").json() + assert "tags" in tags, "GET /api/tasks/tags resolved to the detail route" + + # ── Tag facets ─────────────────────────────────────────────────────── + + async def test_tags_endpoint_counts_and_ranks(self, setup): + await self._create(setup, "Tag task one", tags="backend,ui") + await self._create(setup, "Tag task two", tags="backend") + + tags = setup.client.get("/api/tasks/tags").json()["tags"] + + assert tags[0] == {"name": "backend", "count": 2} + assert {"name": "ui", "count": 1} in tags + + async def test_tags_endpoint_excludes_done_by_default(self, setup): + task_id = await self._create(setup, "Finish me", tags="ephemeral") + setup.client.patch(f"/api/tasks/{task_id}", json={"status": "done"}) + + assert setup.client.get("/api/tasks/tags").json()["tags"] == [] + included = setup.client.get("/api/tasks/tags?include_done=true").json()["tags"] + assert included == [{"name": "ephemeral", "count": 1}] + + # ── Move ───────────────────────────────────────────────────────────── + + async def test_move_returns_the_full_updated_row(self, setup): + task_id = await self._create(setup, "Move me") + + resp = setup.client.post( + f"/api/tasks/{task_id}/move", json={"status": "in_progress"}, + ) + + assert resp.status_code == 200 + task = resp.json()["task"] + # The client reconciles its optimistic update against this, so it + # needs the whole row — not {"moved": true}. + assert task["id"] == task_id + assert task["status"] == "in_progress" + assert "position" in task + + async def test_move_reorders_within_a_lane(self, setup): + top = await self._create(setup, "Order top") + bottom = await self._create(setup, "Order bottom") + + setup.client.post(f"/api/tasks/{top}/move", json={"before_id": bottom}) + + listing = setup.client.get("/api/tasks?sort=position").json()["tasks"] + assert [t["id"] for t in listing] == [bottom, top] + + async def test_move_out_of_done_moves_the_file_back(self, setup): + """The drag-out-of-Done path, end to end through HTTP.""" + task_id = await self._create(setup, "Round trip") + setup.client.patch(f"/api/tasks/{task_id}", json={"status": "done"}) + done_dir = setup.workspace / "memory" / "tasks" / "done" + assert (done_dir / f"{task_id}.md").exists() + + resp = setup.client.post( + f"/api/tasks/{task_id}/move", json={"status": "pending"}, + ) + + assert resp.status_code == 200 + active_dir = setup.workspace / "memory" / "tasks" / "active" + assert (active_dir / f"{task_id}.md").exists() + assert not (done_dir / f"{task_id}.md").exists() + + async def test_move_rejects_an_unknown_status(self, setup): + task_id = await self._create(setup, "Bad lane") + + resp = setup.client.post( + f"/api/tasks/{task_id}/move", json={"status": "not_a_status"}, + ) + + assert resp.status_code == 422 + assert (await setup.db.get_task(task_id))["status"] == "pending" + + async def test_move_on_a_missing_task_is_404(self, setup): + resp = setup.client.post("/api/tasks/nope/move", json={"status": "pending"}) + assert resp.status_code == 404 + + # ── Create ─────────────────────────────────────────────────────────── + + async def test_create_returns_the_structured_task(self, setup): + resp = setup.client.post("/api/tasks", json={ + "title": "Structured create", + "content": "details", + "tags": "alpha,beta", + "confirm_duplicate": True, + }) + + assert resp.status_code == 200 + task = resp.json()["task"] + assert task["title"] == "Structured create" + assert task["tags"] == "alpha,beta" + + async def test_create_honours_an_initial_status(self, setup): + resp = setup.client.post("/api/tasks", json={ + "title": "Starts in progress", + "content": "details", + "status": "in_progress", + "confirm_duplicate": True, + }) + + assert resp.json()["task"]["status"] == "in_progress" + + # The duplicate guard has two strategies; these tests drive the + # ``source_url`` one because it is an exact match. The fuzzy fallback is + # BM25-ranked against a threshold, and BM25 is corpus-relative — on a + # two-document test index the IDF term collapses and even an identical + # title scores above the cutoff. Exercising the ranking itself belongs + # with the handler tests; what matters here is the route's status code. + async def test_duplicate_refusal_is_a_409_with_the_matches(self, setup): + url = "https://example.invalid/issues/1" + await self._create(setup, "Original task", source_url=url) + + resp = setup.client.post("/api/tasks", json={ + "title": "A different title, same source", + "content": "details", + "source_url": url, + }) + + # Previously a 200 carrying an apology in a text blob, which no + # client could distinguish from success. + assert resp.status_code == 409 + detail = resp.json()["detail"] + assert detail["reason"] == "duplicate" + assert detail["duplicates"], "the 409 must name what it collided with" + assert detail["duplicates"][0]["title"] == "Original task" + + async def test_confirm_duplicate_overrides_the_refusal(self, setup): + url = "https://example.invalid/issues/2" + await self._create(setup, "Original task", source_url=url) + + resp = setup.client.post("/api/tasks", json={ + "title": "Deliberate second copy", + "content": "details", + "source_url": url, + "confirm_duplicate": True, + }) + + assert resp.status_code == 200 + + async def test_create_rejects_an_unknown_status_as_422(self, setup): + """A bad field is not a collision, and must not answer like one. + + Both refusals arrive as ``created: false``, so a client that only + reads the status code would be told to offer "create anyway" for a + status that does not exist — a retry that cannot succeed. ``/move`` + already answers 422 for the same mistake; this keeps the two routes + saying the same thing. + """ + resp = setup.client.post("/api/tasks", json={ + "title": "Task in a status that does not exist", + "content": "details", + "status": "nope", + }) + + assert resp.status_code == 422 + detail = resp.json()["detail"] + assert detail["reason"] == "invalid_status" + assert "nope" in detail["message"] + + # ── Patch ──────────────────────────────────────────────────────────── + + async def test_patch_returns_the_full_row(self, setup): + task_id = await self._create(setup, "Patch me") + + resp = setup.client.patch( + f"/api/tasks/{task_id}", json={"title": "Patched title"}, + ) + + assert resp.status_code == 200 + body = resp.json() + assert body["task"]["title"] == "Patched title" + # Legacy keys stay for existing callers. + assert body["task_id"] == task_id and body["updated"] is True + + async def test_patch_writes_content_before_flipping_status(self, setup): + """Ordering guard for routes/tasks.py. + + ``done`` moves the file out of ``active/`` and unlinks the source, + so a status flip applied before the content write would write into + a path that is about to disappear — losing the edit. + """ + task_id = await self._create(setup, "Content then status") + + resp = setup.client.patch(f"/api/tasks/{task_id}", json={ + "content": f"# Content then status\n\nfinal body\n", + "status": "done", + }) + + assert resp.status_code == 200 + done_file = setup.workspace / "memory" / "tasks" / "done" / f"{task_id}.md" + assert done_file.exists() + assert "final body" in done_file.read_text() + + async def test_patch_can_clear_a_deadline(self, setup): + task_id = await self._create(setup, "Dated", deadline="2026-12-01") + assert (await setup.db.get_task(task_id))["deadline"] == "2026-12-01" + + resp = setup.client.patch(f"/api/tasks/{task_id}", json={"deadline": ""}) + + assert resp.status_code == 200 + assert not (await setup.db.get_task(task_id))["deadline"] + + async def test_patch_can_clear_all_tags(self, setup): + task_id = await self._create(setup, "Tagged", tags="alpha") + + setup.client.patch(f"/api/tasks/{task_id}", json={"tags": ""}) + + assert (await setup.db.get_task(task_id))["tags"] == "" + + async def test_patch_leaves_omitted_fields_alone(self, setup): + task_id = await self._create( + setup, "Untouched", tags="keepme", deadline="2026-12-01", + ) + + setup.client.patch(f"/api/tasks/{task_id}", json={"note": "just a note"}) + + row = await setup.db.get_task(task_id) + assert row["tags"] == "keepme" + assert row["deadline"] == "2026-12-01" + + async def test_patch_surfaces_an_invalid_status(self, setup): + task_id = await self._create(setup, "Bad status") + + resp = setup.client.patch( + f"/api/tasks/{task_id}", json={"status": "not_a_status"}, + ) + + # Previously returned 200 {"updated": true} while changing nothing. + assert resp.status_code == 400 + assert (await setup.db.get_task(task_id))["status"] == "pending" + + async def test_patch_on_a_missing_task_is_404(self, setup): + resp = setup.client.patch("/api/tasks/nope", json={"status": "pending"}) + assert resp.status_code == 404 + + # ── List ───────────────────────────────────────────────────────────── + + async def test_list_accepts_a_tag_filter(self, setup): + await self._create(setup, "Listed tagged", tags="infra") + await self._create(setup, "Listed plain") + + body = setup.client.get("/api/tasks?tag=infra").json() + + assert [t["title"] for t in body["tasks"]] == ["Listed tagged"] + assert body["total"] == 1 + + async def test_list_accepts_position_sort(self, setup): + first = await self._create(setup, "Sort one") + second = await self._create(setup, "Sort two") + + body = setup.client.get("/api/tasks?sort=position").json() + + # Newest first by default rank; an unknown sort would silently fall + # back to deadline order, which here is the same — so assert the + # explicit reorder instead. + setup.client.post(f"/api/tasks/{second}/move", json={"before_id": first}) + body = setup.client.get("/api/tasks?sort=position").json() + assert [t["id"] for t in body["tasks"]] == [first, second] diff --git a/tests/test_task_position.py b/tests/test_task_position.py new file mode 100644 index 00000000..4e8c0c81 --- /dev/null +++ b/tests/test_task_position.py @@ -0,0 +1,290 @@ +"""Board ordering: ``tasks.position`` rank math and its preservation. + +Two separable concerns live here. + +**Rank arithmetic** — ``move_task`` resolves "put this between A and B" into +a concrete REAL. The interesting cases are the degenerate ones: a missing +anchor, an anchor in the wrong lane, and neighbours that have converged far +enough that a midpoint no longer fits between them. + +**Preservation** — ``upsert_task`` is preserve-on-omit for ``position``, so a +caller that does not know the column exists cannot reset it. ``tags`` learned +that the hard way in v018 (see the module docstring of +``test_task_reindex.py``), and ``position`` is the same shape of column with a +worse failure mode, because nothing in the markdown file can restore it. +``test_task_upsert_preserve.py`` covers the rule itself. The tests below drive +the *real* callers — reindex, task_write, task_update — rather than calling +``upsert_task`` directly, so a fix that only patches the signature default +cannot pass them. +""" + +from __future__ import annotations + +import pytest + +from nerve.db import Database +from nerve.db.tasks import POSITION_GAP +from nerve.tasks.manager import TaskManager + + +async def _add(db: Database, task_id: str, status: str = "pending", **row) -> None: + row.setdefault("title", f"Task {task_id}") + row.setdefault("file_path", f"memory/tasks/active/{task_id}.md") + await db.upsert_task(task_id=task_id, status=status, **row) + + +async def _lane(db: Database, status: str = "pending") -> list[str]: + """Task ids in the order the board would render them.""" + rows = await db.list_tasks(status=status, sort="position", limit=100) + return [r["id"] for r in rows] + + +async def _position(db: Database, task_id: str) -> float: + return (await db.get_task(task_id))["position"] + + +@pytest.mark.asyncio +class TestNewTaskRanking: + async def test_first_task_in_lane_gets_the_base_gap(self, db: Database): + await _add(db, "solo") + assert await _position(db, "solo") == POSITION_GAP + + async def test_new_tasks_stack_at_the_top_of_their_lane(self, db: Database): + # Newest-first matches what the list view showed before the board, + # and puts a just-created task where its author is looking. + for tid in ("first", "second", "third"): + await _add(db, tid) + assert await _lane(db) == ["third", "second", "first"] + + async def test_lanes_rank_independently(self, db: Database): + await _add(db, "p1", status="pending") + await _add(db, "d1", status="deferred") + # A fresh lane restarts at the base gap rather than inheriting the + # global minimum, so lanes can't drift apart over time. + assert await _position(db, "d1") == POSITION_GAP + + +@pytest.mark.asyncio +class TestMoveTask: + async def test_move_between_two_cards_takes_the_midpoint(self, db: Database): + for tid in ("c", "b", "a"): # → lane order a, b, c + await _add(db, tid) + await _add(db, "mover") + + moved = await db.move_task("mover", before_id="a", after_id="b") + + assert moved is not None + expected = (await _position(db, "a") + await _position(db, "b")) / 2 + assert moved["position"] == expected + assert await _lane(db) == ["a", "mover", "b", "c"] + + async def test_move_to_top_with_only_a_lower_anchor(self, db: Database): + for tid in ("b", "a"): + await _add(db, tid) + await _add(db, "mover") + + await db.move_task("mover", after_id="a") + + assert await _lane(db) == ["mover", "a", "b"] + + async def test_move_to_bottom_with_only_an_upper_anchor(self, db: Database): + for tid in ("b", "a"): + await _add(db, tid) + await _add(db, "mover") + + await db.move_task("mover", before_id="b") + + assert await _lane(db) == ["a", "b", "mover"] + + async def test_move_with_no_anchors_appends(self, db: Database): + for tid in ("b", "a"): + await _add(db, tid) + await _add(db, "mover") + + await db.move_task("mover") + + assert await _lane(db) == ["a", "b", "mover"] + + async def test_missing_task_returns_none(self, db: Database): + assert await db.move_task("nope") is None + + async def test_anchor_in_another_lane_is_ignored(self, db: Database): + # The client's board can be a few seconds stale; an anchor that has + # since moved lanes must degrade to "append here", not corrupt the + # rank by borrowing a number from an unrelated lane. + await _add(db, "elsewhere", status="deferred") + await _add(db, "here") + await _add(db, "mover") + + await db.move_task("mover", before_id="elsewhere") + + assert await _lane(db) == ["here", "mover"] + + async def test_self_anchor_is_ignored(self, db: Database): + await _add(db, "a") + await _add(db, "mover") + + await db.move_task("mover", before_id="mover", after_id="mover") + + # Degenerates to "no usable anchor" → append, not a rank built from + # the moved card's own (about to be overwritten) position. + assert await _lane(db) == ["a", "mover"] + + +@pytest.mark.asyncio +class TestCrossLaneMove: + async def test_move_changes_status_and_reranks(self, db: Database): + await _add(db, "target", status="in_progress") + await _add(db, "mover", status="pending") + + moved = await db.move_task("mover", status="in_progress", after_id="target") + + assert moved["status"] == "in_progress" + assert await _lane(db, "in_progress") == ["mover", "target"] + assert await _lane(db, "pending") == [] + + async def test_move_into_an_empty_lane(self, db: Database): + await _add(db, "mover", status="pending") + + moved = await db.move_task("mover", status="deferred") + + assert moved["status"] == "deferred" + assert moved["position"] == POSITION_GAP + + async def test_status_change_reranks_to_top_of_destination(self, db: Database): + # A rank only means something within its own lane. Carrying it across + # would drop the card at an arbitrary depth of a list it was never + # ordered against — here, below `settled` instead of above it. + for tid in ("deep3", "deep2", "deep1"): + await _add(db, tid, status="pending") + await _add(db, "settled", status="in_progress") + + await db.update_task_status("deep3", "in_progress") + + assert await _lane(db, "in_progress") == ["deep3", "settled"] + + async def test_same_status_update_does_not_rerank(self, db: Database): + await _add(db, "a") + await _add(db, "b") + before = await _position(db, "a") + + await db.update_task_status("a", "pending") + + assert await _position(db, "a") == before + assert await _lane(db) == ["b", "a"] + + +@pytest.mark.asyncio +class TestRenormalization: + async def test_converged_ranks_are_respaced_and_the_move_still_lands( + self, db: Database, + ): + await _add(db, "top") + await _add(db, "bottom") + await _add(db, "mover") + # Collapse two neighbours onto ranks no midpoint can separate. + await db.move_task("top", before_id=None, after_id=None) + async with db._atomic(): + await db.db.execute( + "UPDATE tasks SET position = 5.0 WHERE id = 'top'", + ) + await db.db.execute( + "UPDATE tasks SET position = 5.0000000001 WHERE id = 'bottom'", + ) + + await db.move_task("mover", before_id="top", after_id="bottom") + + lane = await _lane(db) + assert lane == ["top", "mover", "bottom"] + # Post-renormalize the lane is back on wide integral spacing, so the + # next drag has room again instead of failing the same way. + gaps = [await _position(db, t) for t in lane] + assert gaps[1] - gaps[0] >= POSITION_GAP / 2 + assert gaps[2] - gaps[1] >= POSITION_GAP / 2 + + async def test_swapped_anchors_fall_back_to_append(self, db: Database): + # No amount of re-spacing fixes anchors handed over in the wrong + # order (re-spacing preserves order), so the move must not spin. + for tid in ("b", "a"): + await _add(db, tid) + await _add(db, "mover") + + await db.move_task("mover", before_id="b", after_id="a") + + assert await _lane(db) == ["a", "b", "mover"] + + +@pytest.mark.asyncio +class TestPositionSurvivesItsCallers: + """The v018 regression class: a column no caller knows about. + + Every test here drives a *production* write path and asserts the lane + order is unchanged afterwards. None of these callers passes a position, + and none could reconstruct one — the markdown file has no rank in it. + """ + + async def test_upsert_without_position_preserves_it(self, db: Database): + await _add(db, "a") + await _add(db, "b") + ranked = await _lane(db) # ["b", "a"] + + # A plain re-save: same status, no position argument. + await db.upsert_task( + task_id="b", + file_path="memory/tasks/active/b.md", + title="Task b (edited)", + status="pending", + ) + + assert await _lane(db) == ranked + + async def test_explicit_position_still_wins(self, db: Database): + await _add(db, "a") + await _add(db, "b") + + await db.upsert_task( + task_id="b", + file_path="memory/tasks/active/b.md", + title="Task b", + status="pending", + position=99_999.0, + ) + + assert await _lane(db) == ["a", "b"] + + async def test_reindex_preserves_lane_order(self, db: Database, tmp_path): + directory = tmp_path / "memory" / "tasks" / "active" + directory.mkdir(parents=True) + for tid in ("alpha", "beta", "gamma"): + (directory / f"{tid}.md").write_text(f"# {tid}\n", encoding="utf-8") + await _add(db, tid) + (tmp_path / "memory" / "tasks" / "done").mkdir(parents=True) + + # Put the lane in an order no default sort would produce, so a + # reindex that dropped position could not accidentally reproduce it. + # (Creation order alone would give gamma, beta, alpha.) + await db.move_task("gamma", before_id="beta") + ranked = await _lane(db) + assert ranked == ["beta", "gamma", "alpha"] + + await TaskManager(tmp_path, db).reindex() + + assert await _lane(db) == ranked + + async def test_task_update_note_preserves_lane_order( + self, db: Database, tmp_path, + ): + from nerve.agent.tools.handlers.tasks import task_update_handler + from nerve.agent.tools.registry import ToolContext + + directory = tmp_path / "memory" / "tasks" / "active" + directory.mkdir(parents=True) + for tid in ("alpha", "beta"): + (directory / f"{tid}.md").write_text(f"# {tid}\n\nbody\n", encoding="utf-8") + await _add(db, tid) + ranked = await _lane(db) + + ctx = ToolContext(session_id="t", workspace=tmp_path, db=db) + await task_update_handler(ctx, {"task_id": "beta", "note": "still working"}) + + assert await _lane(db) == ranked diff --git a/tests/test_task_reopen.py b/tests/test_task_reopen.py new file mode 100644 index 00000000..992fc635 --- /dev/null +++ b/tests/test_task_reopen.py @@ -0,0 +1,249 @@ +"""Leaving the terminal status: the inverse of ``task_done``. + +``task_done`` is a one-way door — it writes the markdown into ``done/`` and +unlinks the source. Nothing put it back, which was fine only while nothing +could take a task *out* of ``done``. The board can, so the file has to +travel with the status. + +The failure this guards against is quiet rather than loud. A row left +pointing into ``done/`` while claiming an active status is exactly what +``TaskManager.reindex()`` calls an orphan (manager.py: "a file under done/ +is terminal by definition, so the directory wins"), and it force-resets the +status back to ``done``. So a half-done reopen doesn't error — it just +silently undoes itself the next time anything reindexes. +""" + +from __future__ import annotations + +import pytest + +from nerve.agent.tools.handlers.tasks import ( + task_create_handler, + task_done_handler, + task_reopen_handler, + task_update_handler, +) +from nerve.agent.tools.registry import ToolContext +from nerve.db import Database +from nerve.tasks.manager import TaskManager + + +@pytest.fixture +def workspace(tmp_path): + (tmp_path / "memory" / "tasks" / "active").mkdir(parents=True) + (tmp_path / "memory" / "tasks" / "done").mkdir(parents=True) + return tmp_path + + +@pytest.fixture +def ctx(workspace, db: Database) -> ToolContext: + return ToolContext(session_id="test", workspace=workspace, db=db) + + +async def _completed_task(ctx: ToolContext, title: str = "Ship the thing") -> str: + """Create a task, finish it, and return its id.""" + result = await task_create_handler( + ctx, {"title": title, "content": "body", "confirm_duplicate": True}, + ) + task_id = result.structured["task_id"] + await task_done_handler(ctx, {"task_id": task_id, "note": ""}) + return task_id + + +def _active(workspace) -> list[str]: + return sorted(p.name for p in (workspace / "memory" / "tasks" / "active").glob("*.md")) + + +def _done(workspace) -> list[str]: + return sorted(p.name for p in (workspace / "memory" / "tasks" / "done").glob("*.md")) + + +@pytest.mark.asyncio +class TestReopen: + async def test_reopen_moves_the_file_back_to_active(self, ctx, workspace, db): + task_id = await _completed_task(ctx) + assert _done(workspace) == [f"{task_id}.md"] + + result = await task_reopen_handler( + ctx, {"task_id": task_id, "status": "in_progress"}, + ) + + assert not result.is_error + assert _active(workspace) == [f"{task_id}.md"] + assert _done(workspace) == [] + + async def test_reopen_updates_the_row_to_match(self, ctx, db): + task_id = await _completed_task(ctx) + + await task_reopen_handler(ctx, {"task_id": task_id, "status": "in_progress"}) + + row = await db.get_task(task_id) + assert row["status"] == "in_progress" + # The stored path has to follow the file, or the next read 404s and + # the next reindex calls the row an orphan. + assert row["file_path"] == f"memory/tasks/active/{task_id}.md" + + async def test_reindex_leaves_a_reopened_task_alone(self, ctx, workspace, db): + """The actual regression: reopen must survive a reindex.""" + task_id = await _completed_task(ctx) + await task_reopen_handler(ctx, {"task_id": task_id, "status": "in_progress"}) + + await TaskManager(workspace, db).reindex() + + row = await db.get_task(task_id) + assert row["status"] == "in_progress", ( + "reindex reset a reopened task to done — its file is still in done/" + ) + + async def test_reopen_appends_an_audit_line(self, ctx, workspace): + task_id = await _completed_task(ctx) + + await task_reopen_handler( + ctx, + {"task_id": task_id, "status": "pending", "note": "shipped too early"}, + ) + + content = (workspace / "memory" / "tasks" / "active" / f"{task_id}.md").read_text() + assert "REOPENED (pending)" in content + assert "shipped too early" in content + # The DONE line stays: the history is the point of the file. + assert "DONE" in content + + async def test_reopen_defaults_to_the_default_status(self, ctx, db): + task_id = await _completed_task(ctx) + + await task_reopen_handler(ctx, {"task_id": task_id}) + + assert (await db.get_task(task_id))["status"] == "pending" + + async def test_reopen_rejects_the_terminal_status(self, ctx, db): + task_id = await _completed_task(ctx) + + result = await task_reopen_handler(ctx, {"task_id": task_id, "status": "done"}) + + assert result.is_error + assert (await db.get_task(task_id))["status"] == "done" + + async def test_reopen_rejects_an_unknown_status(self, ctx, db): + task_id = await _completed_task(ctx) + + result = await task_reopen_handler( + ctx, {"task_id": task_id, "status": "not_a_status"}, + ) + + assert result.is_error + assert (await db.get_task(task_id))["status"] == "done" + + async def test_reopen_rejects_a_task_that_is_not_done(self, ctx, db): + result = await task_create_handler( + ctx, {"title": "Still going", "content": "b", "confirm_duplicate": True}, + ) + task_id = result.structured["task_id"] + + outcome = await task_reopen_handler( + ctx, {"task_id": task_id, "status": "in_progress"}, + ) + + assert outcome.is_error + assert "not done" in outcome.text_content + + async def test_reopen_rejects_a_missing_task(self, ctx): + result = await task_reopen_handler(ctx, {"task_id": "nope"}) + assert result.is_error + + +@pytest.mark.asyncio +class TestReopenGuardOrdering: + async def test_tracked_config_guard_fires_before_the_status_flip( + self, ctx, db, monkeypatch, + ): + """A refused move must not leave a task claiming to be active. + + Same ordering rationale as ``task_done``: check first, flip second. + Reversed, a rejected reopen leaves the row active while its file + never left ``done/`` — the orphan state this module exists to + prevent, arrived at from the other direction. + """ + import nerve.agent.tools.handlers.tasks as handlers + + task_id = await _completed_task(ctx) + + def _refuse(path, operation): + raise PermissionError(f"Cannot {operation} tracked config: {path}") + + monkeypatch.setattr(handlers, "ensure_path_not_tracked_config", _refuse) + + with pytest.raises(PermissionError): + await task_reopen_handler( + ctx, {"task_id": task_id, "status": "in_progress"}, + ) + + assert (await db.get_task(task_id))["status"] == "done" + + async def test_a_missing_file_is_refused_before_the_status_flip( + self, ctx, workspace, db, + ): + """No file to move means there is no move to make. + + Flipping anyway strands the row in exactly the shape this module + exists to prevent — an active status whose ``file_path`` still + points into ``done/`` — and this is the one way of reaching it that + ``reindex()`` cannot repair, because it only walks files that exist. + """ + task_id = await _completed_task(ctx) + (workspace / "memory" / "tasks" / "done" / f"{task_id}.md").unlink() + + result = await task_reopen_handler( + ctx, {"task_id": task_id, "status": "in_progress"}, + ) + + assert result.is_error + row = await db.get_task(task_id) + assert row["status"] == "done" + assert row["file_path"] == f"memory/tasks/done/{task_id}.md" + + +@pytest.mark.asyncio +class TestTaskUpdateRoutesToReopen: + """``task_update`` is the single entry point; it dispatches both ways.""" + + async def test_status_change_out_of_done_reopens(self, ctx, workspace, db): + task_id = await _completed_task(ctx) + + await task_update_handler( + ctx, {"task_id": task_id, "status": "in_progress"}, + ) + + assert _active(workspace) == [f"{task_id}.md"] + assert (await db.get_task(task_id))["status"] == "in_progress" + + async def test_reopen_applies_other_edits_in_the_same_call( + self, ctx, workspace, db, + ): + # The reopen moves the file first; the remaining edits then have to + # land on the new path, not the stale done/ one. + task_id = await _completed_task(ctx) + + await task_update_handler( + ctx, + {"task_id": task_id, "status": "pending", "tags": "urgent,backend"}, + ) + + row = await db.get_task(task_id) + assert row["status"] == "pending" + assert row["tags"] == "backend,urgent" + content = (workspace / "memory" / "tasks" / "active" / f"{task_id}.md").read_text() + assert "**Tags:** backend, urgent" in content + + async def test_a_normal_status_change_does_not_touch_files( + self, ctx, workspace, db, + ): + result = await task_create_handler( + ctx, {"title": "Ordinary", "content": "b", "confirm_duplicate": True}, + ) + task_id = result.structured["task_id"] + + await task_update_handler(ctx, {"task_id": task_id, "status": "in_progress"}) + + assert _active(workspace) == [f"{task_id}.md"] + assert _done(workspace) == [] diff --git a/tests/test_task_upsert_preserve.py b/tests/test_task_upsert_preserve.py index 7eb34aa5..a5afa9ba 100644 --- a/tests/test_task_upsert_preserve.py +++ b/tests/test_task_upsert_preserve.py @@ -9,9 +9,11 @@ after v018 added the column, and ``task_done`` nulled all four when it moved a file into done/. -The tests here drive ``upsert_task`` directly, because the rule itself is the -subject. The caller-level regressions live with their callers -(``test_task_completion.py``, ``test_task_reindex.py``). +``position`` obeys the same rule and has its own file +(``test_task_position.py``). The tests here drive ``upsert_task`` directly, +because the rule itself is the subject; the caller-level regressions live with +their callers (``test_task_completion.py``, ``test_task_reindex.py``, +``test_task_reopen.py``). """ from __future__ import annotations