fix(mcp): list tools return rows that say what a record is, not what it says (#4061)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m31s
CI & Build / Build & push image (push) Successful in 22s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m31s
CI & Build / Build & push image (push) Successful in 22s
list_tasks returned every row's to_dict(), body included: 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 #4045 failure, one call over). - notes.brief_row: id, title, type, project, tags, updated_at; for tasks, status, kind, priority, milestone id and title; description, parent and due date only when set. - milestones.titles_for: one query for the milestone titles a page of rows names. - Brief rows on list_tasks, list_notes, get_milestone's steps, get_system and list_system_records. get_task / get_note / get_snippet read a record in full, and each docstring says so. - tests/test_list_rows_brief.py pins the ceiling: 100 rows of ~5k-character step plans stay under 40k characters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -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"])
|
||||
@@ -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"}]
|
||||
|
||||
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user