A task's kind is correctable — the Kind select stops lying (#3129) #133
@@ -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<Task, "title" | "body" | "tags" | "status" | "priority" | "due_date" | "project_id" | "milestone_id" | "parent_id" | "recurrence_rule">
|
||||
> & IssueFields
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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"
|
||||
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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")
|
||||
|
||||
Reference in New Issue
Block a user