diff --git a/frontend/src/stores/tasks.ts b/frontend/src/stores/tasks.ts index c9f27c3..d55609f 100644 --- a/frontend/src/stores/tasks.ts +++ b/frontend/src/stores/tasks.ts @@ -55,6 +55,9 @@ export const useTasksStore = defineStore("tasks", () => { async function updateTask( id: number, + // IssueFields carries `kind`, which the PATCH route now reads. It has + // always been SENT by the task editor; until #3129 the route dropped it + // and the save reported success while changing nothing. data: Partial< Pick > & IssueFields diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 1c81988..c0221e1 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -23,6 +23,11 @@ from scribe.mcp.tools import systems as systems_tools from scribe.services import access as access_svc from scribe.services import dedup as dedup_svc from scribe.services import notes as notes_svc +# Imported by NAME, not reached through notes_svc: minted_kind is pure +# validation, not a service call, and a test that stubs the service module to +# avoid the database would otherwise stub the validation too — turning a +# guard into a MagicMock that approves anything. +from scribe.services.notes import minted_kind from scribe.services import planning as planning_svc from scribe.services import rulebooks as rulebooks_svc from scribe.services import systems as systems_svc @@ -194,7 +199,7 @@ async def create_task( milestone_id=milestone_id or None, parent_id=parent_id or None, tags=tags, - task_kind=kind, + task_kind=minted_kind(kind), arose_from_id=arose_from_id or None, ) if system_ids: @@ -214,6 +219,7 @@ async def update_task( milestone_id: int = 0, system_ids: list[int] | None = None, arose_from_id: int = 0, + kind: str = "", ) -> dict: """Update an existing Scribe task. Only explicitly provided fields are changed. @@ -233,6 +239,12 @@ async def update_task( (set-semantics). None = leave unchanged; [] = clear all. arose_from_id: Provenance (issue → originating task). 0 = leave unchanged, -1 = clear, positive = set. + kind: Re-file this task as 'work', 'issue' or 'spike'. Omit (empty) to + leave unchanged. Correcting a kind is ordinary — what a task turns + out to BE is often clear only once it is under way, and a piece of + work that becomes an investigation should say so. 'plan' is + refused: plans are milestones (start_planning), and the value + survives only so historical plan-tasks stay writable. """ uid = current_user_id() fields: dict = {} @@ -258,6 +270,8 @@ async def update_task( fields["arose_from_id"] = None elif arose_from_id: fields["arose_from_id"] = arose_from_id + if kind: + fields["task_kind"] = minted_kind(kind) note = await notes_svc.update_note(uid, task_id, **fields) if note is None: raise ValueError(f"task {task_id} not found") diff --git a/src/scribe/models/note.py b/src/scribe/models/note.py index 762883b..86b995c 100644 --- a/src/scribe/models/note.py +++ b/src/scribe/models/note.py @@ -23,6 +23,24 @@ class TaskPriority(str, enum.Enum): high = "high" +class TaskKind(str, enum.Enum): + """What KIND of work a task is. Mirrors CHECK notes_task_kind_check. + + Every value the COLUMN may hold, including `plan`. That is deliberate: + plans became milestones in 0066, but historical plan-tasks still carry + the value and must stay readable and writable. Refusing to MINT a new + plan is a door policy (see the create/update task tools), not a + statement about what the column accepts — conflating the two would make + old rows unwritable, which is how a retired value turns into corrupt + data. + """ + + work = "work" + issue = "issue" + spike = "spike" + plan = "plan" + + class Note(Base, TimestampMixin, SoftDeleteMixin): __tablename__ = "notes" diff --git a/src/scribe/routes/tasks.py b/src/scribe/routes/tasks.py index 52662f9..64d6631 100644 --- a/src/scribe/routes/tasks.py +++ b/src/scribe/routes/tasks.py @@ -13,6 +13,7 @@ from scribe.services.notes import ( list_notes, update_note, ) +from scribe.services.notes import minted_kind as notes_minted_kind from scribe.services.note_usage import record_pulled from scribe.services.planning import start_planning as svc_start_planning from scribe.services.recurrence import calculate_next_due, validate_recurrence_rule @@ -239,6 +240,16 @@ async def update_task_route(task_id: int): if "tags" in data: fields["tags"] = data["tags"] + # Re-filing a task's kind. The editor has always SENT this field and the + # route silently dropped it — reporting "Task saved" and reverting on the + # next load (#3129). Validated at the same layer as status and priority so + # an unrecognised value is a 400 rather than a database CHECK violation. + if "kind" in data: + try: + fields["task_kind"] = notes_minted_kind(data["kind"]) + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 + for key in ("project_id", "milestone_id", "parent_id", "arose_from_id"): if key in data: fields[key] = data[key] diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 7d46938..cf24f0f 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -5,7 +5,7 @@ from datetime import date, datetime, timezone from sqlalchemy import func, or_, select, text from scribe.models import async_session -from scribe.models.note import Note, TaskPriority, TaskStatus +from scribe.models.note import Note, TaskKind, TaskPriority, TaskStatus logger = logging.getLogger(__name__) @@ -366,6 +366,37 @@ async def get_or_create_note_by_title(user_id: int, title: str) -> Note: return await create_note(user_id, title=title) +# Kinds a caller may MINT. Narrower than what the COLUMN holds: `plan` is a +# valid stored value — historical plan-tasks carry it and must stay writable — +# but plans became milestones in 0066, so no door hands out a new one. The +# CHECK whitelist and this policy answer different questions, which is why +# they are deliberately not the same list. +MINTABLE_KINDS = ("work", "issue", "spike") + + +def minted_kind(kind: str) -> str: + """Validate a kind a caller is asking to WRITE, or raise saying why. + + Lives here rather than in either door so both share one copy: the REST + route cannot import an MCP tool module, and a second spelling of this + list is how the two doors would come to disagree. + + Raises rather than falling back to 'work'. A silently-corrected kind is + the defect this exists to end (#3129: the editor's Kind select reported + success and changed nothing), and a caller naming a kind we do not know + has a wrong idea that an error corrects and a default hides. + """ + if kind in MINTABLE_KINDS: + return kind + if kind == "plan": + raise ValueError( + "kind='plan' is retired — plans are milestones. Call " + "start_planning(project_id, title) to begin one. Existing " + "plan-tasks keep the value and stay editable." + ) + raise ValueError(f"kind must be one of {MINTABLE_KINDS}, got {kind!r}") + + async def update_note(user_id: int, note_id: int, **fields: object) -> Note | None: async with async_session() as session: result = await session.execute( @@ -391,6 +422,20 @@ async def update_note(user_id: int, note_id: int, **fields: object) -> Note | No value = TaskPriority(value).value except ValueError: raise ValueError(f"Invalid priority: {value!r}. Must be one of: {[p.value for p in TaskPriority]}") + elif key == "task_kind" and isinstance(value, str): + # Same shape as status/priority above, and for the same + # reason: a kind the column will refuse should fail here with + # a readable message, not as a CheckViolationError from the + # database. Before this, `task_kind` reached setattr through + # the hasattr guard with no validation at all — but no door + # ever offered it, so a task's kind was write-once (#3129). + try: + value = TaskKind(value).value + except ValueError: + raise ValueError( + f"Invalid kind: {value!r}. Must be one of: " + f"{[k.value for k in TaskKind]}" + ) elif key == "tags" and isinstance(value, list): value = _normalize_tags(value) setattr(note, key, value) diff --git a/tests/test_integration_task_kind_spike.py b/tests/test_integration_task_kind_spike.py index 3f8a2ea..467c437 100644 --- a/tests/test_integration_task_kind_spike.py +++ b/tests/test_integration_task_kind_spike.py @@ -72,3 +72,30 @@ async def test_an_unknown_kind_is_still_refused(owner_id): """ with pytest.raises(IntegrityError): await _write(owner_id, "investigation") + + +async def test_a_kind_can_be_corrected_after_the_fact(owner_id): + """The write-once bug, asserted against a real column (#3129). + + The read-back is the whole test. A version that only asserted the update + call succeeded would have passed against the broken code — the route + returned 200 while dropping the field, which is exactly how this survived + long enough to be found by hand. + """ + from scribe.services import notes as notes_svc + + note_id = await _write(owner_id, "work") + await notes_svc.update_note(owner_id, note_id, task_kind="spike") + async with async_session() as s: + assert (await s.get(Note, note_id)).task_kind == "spike" + + +async def test_an_invalid_kind_is_refused_before_the_database(owner_id): + """A readable ValueError, not a CheckViolationError surfacing as a 500.""" + from scribe.services import notes as notes_svc + + note_id = await _write(owner_id, "work") + with pytest.raises(ValueError, match="Invalid kind"): + await notes_svc.update_note(owner_id, note_id, task_kind="investigation") + async with async_session() as s: + assert (await s.get(Note, note_id)).task_kind == "work" diff --git a/tests/test_mcp_tool_tasks_kind.py b/tests/test_mcp_tool_tasks_kind.py index 832f013..06c735e 100644 --- a/tests/test_mcp_tool_tasks_kind.py +++ b/tests/test_mcp_tool_tasks_kind.py @@ -47,3 +47,53 @@ async def test_create_task_passes_spike(): from scribe.mcp.tools.tasks import create_task await create_task(title="Can the runner be given a bash shell?", kind="spike") assert mock.call_args.kwargs["task_kind"] == "spike" + + +@pytest.mark.asyncio +async def test_update_task_can_re_file_a_kind(): + """A task's kind must be CORRECTABLE, not write-once (#3129). + + What a piece of work turns out to be is often clear only once it is under + way. Before this, `update_task` had no `kind` parameter at all and the + REST route dropped the field the editor was already sending — the save + reported success and reverted on reload. + """ + mock = AsyncMock(return_value=fake_note(task_kind="spike")) + with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): + from scribe.mcp.tools.tasks import update_task + await update_task(task_id=1, kind="spike") + assert mock.call_args.kwargs["task_kind"] == "spike" + + +@pytest.mark.asyncio +async def test_update_task_leaves_kind_alone_when_not_given(): + """"" means leave unchanged, as it does for every other field here.""" + mock = AsyncMock(return_value=fake_note()) + with patch("scribe.mcp.tools.tasks.notes_svc.update_note", mock): + from scribe.mcp.tools.tasks import update_task + await update_task(task_id=1, title="renamed") + assert "task_kind" not in mock.call_args.kwargs + + +@pytest.mark.asyncio +async def test_minting_a_plan_is_refused_at_both_doors(): + """`plan` is a valid stored value and NOT a mintable one. + + Historical plan-tasks carry it and must stay writable, so the column + keeps accepting it — but plans became milestones in 0066, so no door + hands out a new one. The error names start_planning rather than just + refusing, because a caller reaching for kind='plan' wants a plan. + """ + from scribe.services.notes import minted_kind + + with pytest.raises(ValueError, match="start_planning"): + minted_kind("plan") + + +@pytest.mark.asyncio +async def test_an_unknown_kind_raises_rather_than_defaulting(): + """A silently-corrected kind is the defect this whole fix exists to end.""" + from scribe.services.notes import minted_kind + + with pytest.raises(ValueError, match="kind must be one of"): + minted_kind("investigation")