fix(tasks): a task's kind is correctable — the Kind select stops lying (#3129)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 29s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Failing after 48s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 29s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Failing after 48s
CI & Build / Build & push image (push) Skipped
`kind` was accepted at CREATE on both doors and dropped at UPDATE on both: update_task had no such parameter, and the REST PATCH allow-list never read the field. So a task filed under the wrong kind could never be corrected. The frontend made it worse by looking like it worked. TaskEditorView binds a Kind select, marks the form dirty, and HAS ALWAYS SENT `kind` in the update payload — the store even types it. The route ignored it, returned 200, the view optimistically updated, the toast said "Task saved", and the old value came back on reload. Silent success, same class as #2709. Found by trying to re-file #3126 as a spike after deploying 0091. It could not be done; the task had to be recreated as #3128 and the original cancelled. One seam, not two doors. `minted_kind()` lives in services/notes.py because the REST route cannot import an MCP tool module and a second spelling of the list is how the doors would come to disagree. Both create and update route through it, so a bogus kind is now a readable error rather than a CheckViolationError surfacing as a 500. TaskKind joins TaskStatus and TaskPriority as a real enum, and update_note validates task_kind exactly as it already validated those two — the field had been reaching setattr through the hasattr guard with no validation at all, unnoticed only because no door ever offered it. The `-> plan` question #3129 raised is answered in code rather than left implicit: MINTABLE_KINDS is work/issue/spike, deliberately NARROWER than the column's CHECK. `plan` stays a valid stored value because historical plan-tasks carry it and must stay writable; it is simply not a value any door hands out, and the refusal names start_planning because a caller reaching for it wants a plan. The whitelist and the policy answer different questions and are not the same list. Every new test reads the value BACK. One that only asserted the call succeeded would have passed against the broken code — the route returned 200 while dropping the field, which is how this survived long enough to be found by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -194,7 +194,7 @@ async def create_task(
|
||||
milestone_id=milestone_id or None,
|
||||
parent_id=parent_id or None,
|
||||
tags=tags,
|
||||
task_kind=kind,
|
||||
task_kind=notes_svc.minted_kind(kind),
|
||||
arose_from_id=arose_from_id or None,
|
||||
)
|
||||
if system_ids:
|
||||
@@ -214,6 +214,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 +234,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 +265,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"] = notes_svc.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