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 7s
CI & Build / integration (push) Successful in 28s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m6s
CI & Build / Build & push image (push) Successful in 15s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / integration (push) Successful in 28s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m6s
CI & Build / Build & push image (push) Successful in 15s
This commit was merged in pull request #133.
This commit is contained in:
@@ -55,6 +55,9 @@ export const useTasksStore = defineStore("tasks", () => {
|
|||||||
|
|
||||||
async function updateTask(
|
async function updateTask(
|
||||||
id: number,
|
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<
|
data: Partial<
|
||||||
Pick<Task, "title" | "body" | "tags" | "status" | "priority" | "due_date" | "project_id" | "milestone_id" | "parent_id" | "recurrence_rule">
|
Pick<Task, "title" | "body" | "tags" | "status" | "priority" | "due_date" | "project_id" | "milestone_id" | "parent_id" | "recurrence_rule">
|
||||||
> & IssueFields
|
> & 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 access as access_svc
|
||||||
from scribe.services import dedup as dedup_svc
|
from scribe.services import dedup as dedup_svc
|
||||||
from scribe.services import notes as notes_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 planning as planning_svc
|
||||||
from scribe.services import rulebooks as rulebooks_svc
|
from scribe.services import rulebooks as rulebooks_svc
|
||||||
from scribe.services import systems as systems_svc
|
from scribe.services import systems as systems_svc
|
||||||
@@ -194,7 +199,7 @@ async def create_task(
|
|||||||
milestone_id=milestone_id or None,
|
milestone_id=milestone_id or None,
|
||||||
parent_id=parent_id or None,
|
parent_id=parent_id or None,
|
||||||
tags=tags,
|
tags=tags,
|
||||||
task_kind=kind,
|
task_kind=minted_kind(kind),
|
||||||
arose_from_id=arose_from_id or None,
|
arose_from_id=arose_from_id or None,
|
||||||
)
|
)
|
||||||
if system_ids:
|
if system_ids:
|
||||||
@@ -214,6 +219,7 @@ async def update_task(
|
|||||||
milestone_id: int = 0,
|
milestone_id: int = 0,
|
||||||
system_ids: list[int] | None = None,
|
system_ids: list[int] | None = None,
|
||||||
arose_from_id: int = 0,
|
arose_from_id: int = 0,
|
||||||
|
kind: str = "",
|
||||||
) -> dict:
|
) -> dict:
|
||||||
"""Update an existing Scribe task. Only explicitly provided fields are changed.
|
"""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.
|
(set-semantics). None = leave unchanged; [] = clear all.
|
||||||
arose_from_id: Provenance (issue → originating task). 0 = leave unchanged,
|
arose_from_id: Provenance (issue → originating task). 0 = leave unchanged,
|
||||||
-1 = clear, positive = set.
|
-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()
|
uid = current_user_id()
|
||||||
fields: dict = {}
|
fields: dict = {}
|
||||||
@@ -258,6 +270,8 @@ async def update_task(
|
|||||||
fields["arose_from_id"] = None
|
fields["arose_from_id"] = None
|
||||||
elif arose_from_id:
|
elif arose_from_id:
|
||||||
fields["arose_from_id"] = 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)
|
note = await notes_svc.update_note(uid, task_id, **fields)
|
||||||
if note is None:
|
if note is None:
|
||||||
raise ValueError(f"task {task_id} not found")
|
raise ValueError(f"task {task_id} not found")
|
||||||
|
|||||||
@@ -23,6 +23,24 @@ class TaskPriority(str, enum.Enum):
|
|||||||
high = "high"
|
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):
|
class Note(Base, TimestampMixin, SoftDeleteMixin):
|
||||||
__tablename__ = "notes"
|
__tablename__ = "notes"
|
||||||
|
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ from scribe.services.notes import (
|
|||||||
list_notes,
|
list_notes,
|
||||||
update_note,
|
update_note,
|
||||||
)
|
)
|
||||||
|
from scribe.services.notes import minted_kind as notes_minted_kind
|
||||||
from scribe.services.note_usage import record_pulled
|
from scribe.services.note_usage import record_pulled
|
||||||
from scribe.services.planning import start_planning as svc_start_planning
|
from scribe.services.planning import start_planning as svc_start_planning
|
||||||
from scribe.services.recurrence import calculate_next_due, validate_recurrence_rule
|
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:
|
if "tags" in data:
|
||||||
fields["tags"] = data["tags"]
|
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"):
|
for key in ("project_id", "milestone_id", "parent_id", "arose_from_id"):
|
||||||
if key in data:
|
if key in data:
|
||||||
fields[key] = data[key]
|
fields[key] = data[key]
|
||||||
|
|||||||
@@ -5,7 +5,7 @@ from datetime import date, datetime, timezone
|
|||||||
from sqlalchemy import func, or_, select, text
|
from sqlalchemy import func, or_, select, text
|
||||||
|
|
||||||
from scribe.models import async_session
|
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__)
|
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)
|
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 def update_note(user_id: int, note_id: int, **fields: object) -> Note | None:
|
||||||
async with async_session() as session:
|
async with async_session() as session:
|
||||||
result = await session.execute(
|
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
|
value = TaskPriority(value).value
|
||||||
except ValueError:
|
except ValueError:
|
||||||
raise ValueError(f"Invalid priority: {value!r}. Must be one of: {[p.value for p in TaskPriority]}")
|
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):
|
elif key == "tags" and isinstance(value, list):
|
||||||
value = _normalize_tags(value)
|
value = _normalize_tags(value)
|
||||||
setattr(note, key, value)
|
setattr(note, key, value)
|
||||||
|
|||||||
@@ -72,3 +72,30 @@ async def test_an_unknown_kind_is_still_refused(owner_id):
|
|||||||
"""
|
"""
|
||||||
with pytest.raises(IntegrityError):
|
with pytest.raises(IntegrityError):
|
||||||
await _write(owner_id, "investigation")
|
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
|
from scribe.mcp.tools.tasks import create_task
|
||||||
await create_task(title="Can the runner be given a bash shell?", kind="spike")
|
await create_task(title="Can the runner be given a bash shell?", kind="spike")
|
||||||
assert mock.call_args.kwargs["task_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