diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 4fd6943..5476d33 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -44,7 +44,10 @@ async def get_milestone(milestone_id: int) -> dict: rules surface again on recall. Returns: milestone (incl. body), progress, steps (its tasks ordered by - status then update), and applicable_rules / project_rules. + status then update), and applicable_rules / project_rules. Each step says + what it is and where it stands, not its whole body — read a step in full + with get_task(id). A plan with forty long steps is otherwise too big to + arrive inline, and the design is in the milestone body. """ uid = current_user_id() milestone = await milestones_svc.get_milestone(uid, milestone_id) @@ -61,7 +64,7 @@ async def get_milestone(milestone_id: int) -> dict: out.update(progress) return { "milestone": out, - "steps": [t.to_dict() for t in steps], + "steps": [notes_svc.brief_row(t, {milestone.id: milestone.title}) for t in steps], **rulebooks_svc.rules_payload(applicable, user_id=uid, source="get_milestone"), } diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index 51b3cd4..d13b490 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -44,6 +44,9 @@ async def list_notes( Prefer `search` for a pure lookup — it returns scores and reaches everything readable; this adds the lifecycle filters on top. + Each row says what the note is — id, title, description, type, project, + tags, updated_at — and not what it says: read one in full with get_note(id). + Args: project_id: Scope to one project. PASS THE ACTIVE PROJECT'S ID whenever a project is in scope so you list that project's notes, not every @@ -60,7 +63,7 @@ async def list_notes( limit=max(1, min(limit, 100)), offset=max(0, offset), ) - return {"notes": [n.to_dict() for n in rows], "total": total} + return {"notes": [notes_svc.brief_row(n) for n in rows], "total": total} diff --git a/src/scribe/mcp/tools/systems.py b/src/scribe/mcp/tools/systems.py index df0050e..bd8784b 100644 --- a/src/scribe/mcp/tools/systems.py +++ b/src/scribe/mcp/tools/systems.py @@ -14,6 +14,7 @@ from __future__ import annotations from scribe.mcp._context import current_user_id from scribe.services import canonical_systems as canonical_systems_svc +from scribe.services import milestones as milestones_svc from scribe.services import notes as notes_svc from scribe.services import systems as systems_svc @@ -267,16 +268,18 @@ async def get_system(system_id: int) -> dict: """Fetch a System plus the records associated with it. Returns the system, plus its associated records split into `issues`, - `tasks` (work/plan), and `notes`. + `tasks` (work/plan), and `notes` — each saying what the record is and where + it sits, not what it says (open one with get_note / get_task). """ uid = current_user_id() system = await systems_svc.get_system(uid, system_id) if system is None: raise ValueError(f"system {system_id} not found") records = await systems_svc.list_records_for_system(uid, system_id) + titles = await milestones_svc.titles_for({r.milestone_id for r in records}) issues, tasks, notes = [], [], [] for r in records: - d = r.to_dict() + d = notes_svc.brief_row(r, titles) if r.status is None: notes.append(d) elif r.task_kind == "issue": @@ -339,12 +342,16 @@ async def list_system_records( kind: filter by task_kind — 'issue', 'work', 'spike' (or the retired 'plan'). Omit for all. open_only: limit to tasks not done/cancelled (e.g. open issues only). + + Each record says what it is and where it sits, not what it says — open one + with get_note / get_task / get_snippet. """ uid = current_user_id() rows = await systems_svc.list_records_for_system( uid, system_id, kind=kind or None, open_only=open_only, ) - return {"records": [r.to_dict() for r in rows]} + titles = await milestones_svc.titles_for({r.milestone_id for r in rows}) + return {"records": [notes_svc.brief_row(r, titles) for r in rows]} async def delete_system(system_id: int) -> dict: diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index d3fcbb7..093dfce 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -22,6 +22,7 @@ from scribe.mcp._context import current_user_id 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 milestones as milestones_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 @@ -58,7 +59,10 @@ async def list_tasks( kind: Filter by task kind — 'work', 'issue', 'spike' (or the retired 'plan'). Omit (empty) for all kinds. - Results are ordered by last-updated descending. + Results are ordered by last-updated descending. Each row says what the task + is and where it sits — id, title, status, kind, priority, milestone (id and + title), tags, updated_at, plus description, parent and due date when set — + and not what it says: read one in full with get_task(id). """ uid = current_user_id() rows, total = await notes_svc.list_notes( @@ -70,7 +74,8 @@ async def list_tasks( limit=max(1, min(limit, 100)), offset=max(0, offset), ) - return {"tasks": [n.to_dict() for n in rows], "total": total} + titles = await milestones_svc.titles_for({n.milestone_id for n in rows}) + return {"tasks": [notes_svc.brief_row(n, titles) for n in rows], "total": total} async def get_task(task_id: int) -> dict: diff --git a/src/scribe/services/milestones.py b/src/scribe/services/milestones.py index 0e99a8e..cada46c 100644 --- a/src/scribe/services/milestones.py +++ b/src/scribe/services/milestones.py @@ -72,6 +72,26 @@ async def get_milestone(user_id: int, milestone_id: int) -> Milestone | None: return result.scalars().first() +async def titles_for(milestone_ids: set[int]) -> dict[int, str]: + """{id: title} for the given milestones, for rows that name where a record sits. + + No ownership filter, deliberately: the callers are listings whose rows the + caller could already read, and a milestone title is part of "where does + this record sit". Filtering here would blank the placement of a shared + task in someone else's plan while still showing the task. + """ + ids = {i for i in milestone_ids if i} + if not ids: + return {} + async with async_session() as session: + rows = (await session.execute( + select(Milestone.id, Milestone.title).where( + Milestone.id.in_(ids), Milestone.deleted_at.is_(None), + ) + )).all() + return {mid: title for mid, title in rows} + + async def get_milestone_in_project(project_id: int, milestone_id: int) -> Milestone | None: """Fetch a milestone by id within a project, without a user_id ownership check. Callers must verify project access separately before using this.""" diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 31d1c8e..758e963 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -7,6 +7,7 @@ from sqlalchemy import func, or_, select, text from scribe.models import async_session from scribe.models.note import Note, TaskKind, TaskPriority, TaskStatus +from scribe.models.base import iso logger = logging.getLogger(__name__) @@ -1057,3 +1058,40 @@ async def get_note_for_user( async with async_session() as session: note = await session.get(Note, note_id) return (note, perm) if note else None + + +# What a LISTING row carries (#4061): what the record is and where it sits, +# never what it says. list_tasks once returned every row's to_dict(), body +# included, and a project's todo list came to 93-165k characters, past what an +# MCP client accepts inline, so the list arrived as a file to page through. The +# same failure #4045 fixed for enter_project, one call over. get_task / get_note +# read a record in full; a list is for choosing which one to open. +# +# A field most rows leave empty (a one-line description, a parent, a due date) +# is attached only when set: a hundred rows of `null` are a hundred chances to +# learn to skip the key (#2483), and the bytes are the thing being cut. +def brief_row(note: Note, milestone_titles: dict[int, str] | None = None) -> dict: + row = { + "id": note.id, + "title": note.title, + "note_type": note.note_type or "note", + "project_id": note.project_id, + "tags": note.tags or [], + "updated_at": iso(note.updated_at), + } + if note.description: + row["description"] = note.description + if note.is_task: + row.update({ + "status": note.status, + "task_kind": note.task_kind, + "priority": note.priority, + "milestone_id": note.milestone_id, + }) + if milestone_titles is not None and note.milestone_id: + row["milestone_title"] = milestone_titles.get(note.milestone_id) + if note.parent_id: + row["parent_id"] = note.parent_id + if note.due_date: + row["due_date"] = iso(note.due_date) + return row diff --git a/tests/test_list_rows_brief.py b/tests/test_list_rows_brief.py new file mode 100644 index 0000000..faab13e --- /dev/null +++ b/tests/test_list_rows_brief.py @@ -0,0 +1,93 @@ +"""List tools return rows that say what a record is, never what it says (#4061). + +list_tasks once returned every row's to_dict(), body included. A project's todo +list came to 93-165k characters, past what an MCP client accepts as a tool +result, so the list arrived as a file to page through — the failure #4045 fixed +for enter_project, one call over. A list is for choosing which record to open; +get_task / get_note read one in full. +""" +import json +from datetime import datetime, timezone +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.mcp.tools.notes import list_notes +from scribe.mcp.tools.systems import list_system_records +from scribe.mcp.tools.tasks import list_tasks +from scribe.services.notes import brief_row + +pytestmark = pytest.mark.usefixtures("_bind_user") + +STEP_PLAN = "A step plan paragraph long enough to matter. " * 110 # ~5k chars +WHEN = datetime(2026, 9, 15, tzinfo=timezone.utc) + + +def _row(nid: int, *, is_task: bool = True, milestone_id: int | None = 415) -> SimpleNamespace: + """A real-shaped Note: every attribute brief_row or to_dict could read.""" + return SimpleNamespace( + id=nid, title=f"Step {nid} — something worth doing", body=STEP_PLAN, + description=None, note_type="note", project_id=2, tags=["retrieval"], + updated_at=WHEN, created_at=WHEN, is_task=is_task, + status="todo" if is_task else None, task_kind="work", priority="high", + milestone_id=milestone_id if is_task else None, parent_id=None, due_date=None, + ) + + +def test_a_task_row_names_what_it_is_and_where_it_sits_but_not_its_body(): + row = brief_row(_row(7), {415: "An existing plan is found"}) + assert "body" not in row + assert row["milestone_title"] == "An existing plan is found" + assert {"id", "title", "status", "task_kind", "priority", "milestone_id", + "tags", "updated_at", "project_id"} <= set(row) + # Empty optional fields are left off rather than sent as null (#2483). + assert not {"description", "parent_id", "due_date"} & set(row) + assert row["updated_at"] == WHEN.isoformat() + + +def test_a_note_row_carries_no_task_fields(): + row = brief_row(_row(8, is_task=False)) + assert "body" not in row and "status" not in row and "milestone_title" not in row + + +def test_a_task_outside_any_milestone_says_so_rather_than_inventing_a_title(): + row = brief_row(_row(9, milestone_id=None), {}) + assert row["milestone_id"] is None and "milestone_title" not in row + + +@pytest.mark.asyncio +async def test_a_full_page_of_long_tasks_fits_inline(): + """The ceiling. 100 rows (list_tasks' own limit cap) of ~5k-character step + plans: as to_dict() rows this was over half a million characters.""" + rows = [_row(i) for i in range(100)] + with patch("scribe.mcp.tools.tasks.notes_svc.list_notes", + AsyncMock(return_value=(rows, 100))), \ + patch("scribe.mcp.tools.tasks.milestones_svc.titles_for", + AsyncMock(return_value={415: "An existing plan is found"})) as titles: + out = await list_tasks(project_id=2, limit=100) + size = len(json.dumps(out)) + assert size < 40_000, f"list_tasks page is {size} characters" + assert titles.await_args.args[0] == {415} + assert out["tasks"][0]["milestone_title"] == "An existing plan is found" + + +@pytest.mark.asyncio +async def test_list_notes_rows_leave_the_body_to_get_note(): + rows = [_row(i, is_task=False) for i in range(3)] + with patch("scribe.mcp.tools.notes.notes_svc.list_notes", + AsyncMock(return_value=(rows, 3))): + out = await list_notes(project_id=2) + assert all("body" not in r for r in out["notes"]) + + +@pytest.mark.asyncio +async def test_list_system_records_rows_leave_the_body_to_the_getters(): + rows = [_row(1), _row(2, is_task=False)] + with patch("scribe.mcp.tools.systems.systems_svc.list_records_for_system", + AsyncMock(return_value=rows)), \ + patch("scribe.mcp.tools.systems.milestones_svc.titles_for", + AsyncMock(return_value={415: "Plan"})): + out = await list_system_records(system_id=3) + assert [r["id"] for r in out["records"]] == [1, 2] + assert all("body" not in r for r in out["records"]) diff --git a/tests/test_mcp_tool_milestones.py b/tests/test_mcp_tool_milestones.py index 9390761..19a4b07 100644 --- a/tests/test_mcp_tool_milestones.py +++ b/tests/test_mcp_tool_milestones.py @@ -6,7 +6,7 @@ import pytest from scribe.mcp.tools.milestones import ( list_milestones, get_milestone, create_milestone, update_milestone, ) -from tests.helpers import fake_milestone +from tests.helpers import fake_milestone, fake_task pytestmark = pytest.mark.usefixtures("_bind_user") @@ -111,8 +111,7 @@ async def test_update_milestone_sends_body(): @pytest.mark.asyncio async def test_get_milestone_returns_body_steps_and_rules(): m = fake_milestone(id=5, project_id=3, body="## Goal") - step = MagicMock() - step.to_dict.return_value = {"id": 9, "title": "step 1", "status": "todo"} + step = fake_task(id=9, title="step 1", milestone_id=5, body="a long step body") applicable = {"rules": [{"id": 1, "title": "r"}], "truncated": False, "project_rules": [{"id": 3, "title": "own"}]} with patch("scribe.mcp.tools.milestones.milestones_svc.get_milestone", @@ -126,7 +125,10 @@ async def test_get_milestone_returns_body_steps_and_rules(): out = await get_milestone(milestone_id=5) assert out["milestone"]["body"] == "## Goal" assert out["milestone"]["total"] == 1 - assert out["steps"] == [{"id": 9, "title": "step 1", "status": "todo"}] + assert [(s["id"], s["title"], s["status"]) for s in out["steps"]] == [(9, "step 1", "todo")] + # A step's body is get_task's job (#4061); its placement is not. + assert "body" not in out["steps"][0] + assert out["steps"][0]["milestone_title"] == m.title assert out["applicable_rules"] == [{"id": 1, "title": "r"}] diff --git a/tests/test_mcp_tool_systems.py b/tests/test_mcp_tool_systems.py index 9432553..6f8c79e 100644 --- a/tests/test_mcp_tool_systems.py +++ b/tests/test_mcp_tool_systems.py @@ -2,7 +2,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note, fake_system +from tests.helpers import fake_note, fake_system, fake_task # The name assessment both doors run before minting (milestone 307). A test @@ -87,9 +87,9 @@ async def test_create_system_duplicate_names_the_existing_one_and_creates_nothin @pytest.mark.asyncio async def test_get_system_splits_records_by_kind(): - issue = MagicMock(); issue.to_dict.return_value = {"id": 10}; issue.task_kind = "issue"; issue.status = "todo" - work = MagicMock(); work.to_dict.return_value = {"id": 11}; work.task_kind = "work"; work.status = "todo" - note = MagicMock(); note.to_dict.return_value = {"id": 12}; note.task_kind = "work"; note.status = None + issue = fake_task(id=10, task_kind="issue", milestone_id=None) + work = fake_task(id=11, milestone_id=None) + note = fake_note(id=12, status=None, milestone_id=None) with patch("scribe.mcp.tools.systems.current_user_id", return_value=1), \ patch("scribe.mcp.tools.systems.systems_svc") as svc: svc.get_system = AsyncMock(return_value=fake_system(id=3)) diff --git a/tests/test_mcp_tool_tasks.py b/tests/test_mcp_tool_tasks.py index 47db67a..bcbc8d0 100644 --- a/tests/test_mcp_tool_tasks.py +++ b/tests/test_mcp_tool_tasks.py @@ -16,7 +16,7 @@ pytestmark = pytest.mark.usefixtures("_bind_user") @pytest.mark.asyncio async def test_list_tasks_passes_is_task_true_and_repackages(): - rows = [fake_task(id=1), fake_task(id=2)] + rows = [fake_task(id=1, milestone_id=None), fake_task(id=2, milestone_id=None)] mock = AsyncMock(return_value=(rows, 2)) with patch("scribe.mcp.tools.tasks.notes_svc.list_notes", mock): out = await list_tasks()