From 9b2de3552f22a8d63a6ca4cfa7027583520c7788 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 19:48:24 -0400 Subject: [PATCH] fix(mcp): project reads list milestones without their plans, and cap done ones (#4045) enter_project returned every milestone's full plan body. On a project with 39 milestones the handshake came to ~222k characters, 168k of them bodies (110k from done milestones). That is past what an MCP client accepts as a tool result, so the call meant to orient a session arrived as a file to page through. It grows with a project's history, so any long-lived project on any install gets there. - brief_milestone_summary (services/milestones.py) trims summary rows to the listing fields: id, title, description, status, order_index and progress. The plan is get_milestone's job. user_id, project_id and timestamps repeat what the caller knows. - enter_project and get_project share one block: every open milestone plus the 5 most recently updated done ones, in order. milestone_summary_omitted is attached only when older done ones were left out, and names list_milestones and get_milestone. - list_milestones lists every milestone, done included, without bodies. It is the call the omitted line points to, and it had the same size problem. - The REST project summary is unchanged; the web UI reads it. Tests: trimming, the done cap and its order, the omitted key present and absent, get_project and list_milestones, and a size ceiling on enter_project's milestone block for a 200-milestone history. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/milestones.py | 9 +- src/scribe/mcp/tools/projects.py | 47 +++++++-- src/scribe/services/milestones.py | 35 +++++++ tests/test_mcp_tool_milestones.py | 2 +- tests/test_mcp_tool_projects.py | 4 +- tests/test_milestone_summary_brief.py | 141 ++++++++++++++++++++++++++ 6 files changed, 223 insertions(+), 15 deletions(-) create mode 100644 tests/test_milestone_summary_brief.py diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 97f73a3..6244b49 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -22,12 +22,15 @@ from scribe.services.record_refs import refuse_guessed_ids async def list_milestones(project_id: int) -> dict: """List milestones for a Scribe project, ordered by order_index. - Returns id, title, description, body (the plan/design), status - (active/done), order_index, and task counts. + Returns every milestone, done ones included: id, title, description, + status (active/done), order_index and progress (total, completed, pct, + status_counts). The plan itself is not listed: get_milestone(id) returns a + milestone's body and its steps. """ uid = current_user_id() rows = await milestones_svc.get_project_milestone_summary(uid, project_id) - return {"milestones": rows} + brief, _ = milestones_svc.brief_milestone_summary(rows) + return {"milestones": brief} async def get_milestone(milestone_id: int) -> dict: diff --git a/src/scribe/mcp/tools/projects.py b/src/scribe/mcp/tools/projects.py index 1dfc43b..df349fd 100644 --- a/src/scribe/mcp/tools/projects.py +++ b/src/scribe/mcp/tools/projects.py @@ -42,6 +42,32 @@ async def list_projects() -> dict: return {"projects": [p.to_dict() for p in rows]} +# Done milestones a project read still lists: the recent ones say what just +# finished, and older ones are a list_milestones call away (#4045). +_DONE_MILESTONES_KEPT = 5 + + +async def _milestone_block(uid: int, project_id: int) -> dict: + """`milestone_summary` for a project read, brief and bounded (#4045). + + Every open milestone plus the most recent done ones, without plan bodies. + `milestone_summary_omitted` is attached only when older done milestones + were left out, and names the calls that reach them (#2483). + """ + rows = await milestones_svc.get_project_milestone_summary(uid, project_id) + brief, omitted = milestones_svc.brief_milestone_summary( + rows, done_kept=_DONE_MILESTONES_KEPT, + ) + out: dict = {"milestone_summary": brief} + if omitted: + out["milestone_summary_omitted"] = ( + f"{omitted} older done milestone(s) not listed. " + f"list_milestones({project_id}) lists every milestone; " + "get_milestone(id) has one milestone's plan and steps." + ) + return out + + async def enter_project(project_id: int) -> dict: """Session-start handshake: load full context for working on a project. @@ -63,6 +89,11 @@ async def enter_project(project_id: int) -> dict: open_tasks, recent_notes, design_system, systems, pattern_coverage — plus systems_bootstrap, present only when it applies (see below). + `milestone_summary` lists every open milestone and the most recently + finished done ones, each with its description and progress but NOT its + plan: get_milestone(id) reads a plan. `milestone_summary_omitted` appears + only when older done milestones were left out, and says how many. + `pattern_coverage` (usually null) is the shape-accounting line — how many of the bound repo's extracted shapes carry a classification against canon (note 2786) — e.g. "shape accounting: 3100/4573 shapes accounted for — @@ -120,9 +151,7 @@ async def enter_project(project_id: int) -> dict: applicable = await rulebooks_svc.get_applicable_rules( project_id=project_id, user_id=uid, ) - milestone_summary = await milestones_svc.get_project_milestone_summary( - uid, project_id, - ) + milestones = await _milestone_block(uid, project_id) open_tasks, _ = await notes_svc.list_notes( uid, is_task=True, project_id=project_id, status=["todo", "in_progress"], sort="updated_at", limit=10, @@ -206,7 +235,7 @@ async def enter_project(project_id: int) -> dict: for s in systems ], "design_system": design_system, - "milestone_summary": milestone_summary, + **milestones, **rulebooks_svc.rules_payload(applicable, user_id=uid, source="enter_project"), "open_tasks": [ { @@ -236,8 +265,10 @@ async def enter_project(project_id: int) -> dict: async def get_project(project_id: int) -> dict: """Fetch a Scribe project by ID. - Returns full project fields, a milestone_summary list, and the - rulebook-applicable_rules / subscribed_rulebooks pair the assistant + Returns full project fields, a milestone_summary list (shaped as in + enter_project: open milestones and the recent done ones, no plan bodies, + with milestone_summary_omitted when older done ones were left out), and + the rulebook-applicable_rules / subscribed_rulebooks pair the assistant should consult when working on this project. """ uid = current_user_id() @@ -245,9 +276,7 @@ async def get_project(project_id: int) -> dict: if project is None: raise ValueError(f"project {project_id} not found") data = project.to_dict() - data["milestone_summary"] = await milestones_svc.get_project_milestone_summary( - uid, project_id, - ) + data.update(await _milestone_block(uid, project_id)) applicable = await rulebooks_svc.get_applicable_rules( project_id=project_id, user_id=uid, ) diff --git a/src/scribe/services/milestones.py b/src/scribe/services/milestones.py index 5de7b59..2b976cf 100644 --- a/src/scribe/services/milestones.py +++ b/src/scribe/services/milestones.py @@ -238,3 +238,38 @@ async def get_project_milestone_summary(user_id: int, project_id: int) -> list[d """Ordered milestones with progress — the one-project view of get_project_milestone_summaries (two queries, not N+1).""" return (await get_project_milestone_summaries(user_id, [project_id])).get(project_id, []) + + +# What a milestone LISTING needs: enough to say what each plan is and how far +# along it is. The plan itself (`body`) is get_milestone's job. Summaries once +# carried it, and on a project with 39 milestones enter_project came to ~222k +# characters, 168k of them plan bodies. That is past what an MCP client will +# accept as a tool result, so the session handshake arrived as a file to page +# through (#4045). user_id / project_id / timestamps repeat what the caller +# already knows. +_BRIEF_FIELDS = ( + "id", "title", "description", "status", "order_index", + "total", "completed", "pct", "status_counts", +) + + +def brief_milestone_summary( + rows: list[dict], *, done_kept: int | None = None, +) -> tuple[list[dict], int]: + """Trim summary rows to the listing fields, optionally capping done ones. + + `done_kept` keeps only the N most recently updated done milestones (every + open one stays), in the original order_index order. A done plan is + history, and a handshake that grows with a project's whole history grows + past what a client accepts. None keeps every row. Returns (rows, omitted), + where `omitted` counts the done milestones left out. + """ + if done_kept is None: + kept = rows + else: + done = [r for r in rows if r.get("status") == "done"] + recent = sorted(done, key=lambda r: r.get("updated_at") or "", reverse=True) + keep_ids = {r.get("id") for r in recent[:done_kept]} + kept = [r for r in rows if r.get("status") != "done" or r.get("id") in keep_ids] + brief = [{k: r[k] for k in _BRIEF_FIELDS if k in r} for r in kept] + return brief, len(rows) - len(kept) diff --git a/tests/test_mcp_tool_milestones.py b/tests/test_mcp_tool_milestones.py index 907d0f1..6588760 100644 --- a/tests/test_mcp_tool_milestones.py +++ b/tests/test_mcp_tool_milestones.py @@ -14,7 +14,7 @@ pytestmark = pytest.mark.usefixtures("_bind_user") @pytest.mark.asyncio async def test_list_milestones_returns_dict_with_progress(): - rows = [{"id": 1, "title": "MS1", "status": "active", "task_count": 2}] + rows = [{"id": 1, "title": "MS1", "status": "active", "total": 2}] with patch( "scribe.mcp.tools.milestones.milestones_svc.get_project_milestone_summary", AsyncMock(return_value=rows), diff --git a/tests/test_mcp_tool_projects.py b/tests/test_mcp_tool_projects.py index 9586d6b..6bd55b6 100644 --- a/tests/test_mcp_tool_projects.py +++ b/tests/test_mcp_tool_projects.py @@ -71,7 +71,7 @@ async def test_list_projects_wraps_in_dict(): @pytest.mark.asyncio async def test_get_project_enriches_with_milestone_summary(): p = fake_project(id=5, title="found") - milestone_summary = [{"id": 10, "title": "MS", "task_count": 3}] + milestone_summary = [{"id": 10, "title": "MS", "status": "active", "total": 3}] applicable_payload = { "rules": [], "truncated": False, "subscribed_rulebooks": [], } @@ -175,7 +175,7 @@ async def test_enter_project_composes_full_context(): "truncated": False, "subscribed_rulebooks": [{"id": 2, "title": "rb"}], } - milestone_summary = [{"id": 10, "title": "MS", "task_count": 3}] + milestone_summary = [{"id": 10, "title": "MS", "status": "active", "total": 3}] task1 = MagicMock() task1.id = 100; task1.title = "T1"; task1.status = "in_progress" diff --git a/tests/test_milestone_summary_brief.py b/tests/test_milestone_summary_brief.py new file mode 100644 index 0000000..6e46a0d --- /dev/null +++ b/tests/test_milestone_summary_brief.py @@ -0,0 +1,141 @@ +"""Milestone listings stay brief and bounded (#4045). + +enter_project once carried every milestone's full plan body. On a project with +39 milestones that came to ~222k characters, past what an MCP client accepts +as a tool result, so the session handshake arrived as a file to page through. +""" +import contextlib +import json +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.mcp.tools.milestones import list_milestones +from scribe.mcp.tools.projects import enter_project, get_project +from scribe.services.milestones import brief_milestone_summary +from tests.helpers import fake_project + + +pytestmark = pytest.mark.usefixtures("_bind_user") + +PLAN = "A plan paragraph long enough to matter. " * 125 # ~5k chars + + +def _row(mid: int, status: str, updated: str) -> dict: + """A summary row as get_project_milestone_summary returns it.""" + return { + "id": mid, "user_id": 7, "project_id": 5, "title": f"M{mid}", + "description": f"what M{mid} is for", "body": PLAN, "status": status, + "order_index": mid, "created_at": "2026-01-01T00:00:00+00:00", + "updated_at": updated, "total": 4, "completed": 2, "pct": 50.0, + "status_counts": {"todo": 2, "in_progress": 0, "done": 2, "cancelled": 0}, + } + + +def _history(done: int, active: int) -> list[dict]: + """`done` done milestones (higher id = more recently updated), then `active` open ones.""" + rows = [_row(i, "done", f"2026-08-{i + 1:02d}T00:00:00+00:00") for i in range(done)] + rows += [_row(done + i, "active", "2026-01-01T00:00:00+00:00") for i in range(active)] + return rows + + +def test_brief_rows_leave_out_the_plan_and_what_the_caller_already_knows(): + brief, omitted = brief_milestone_summary([_row(1, "active", "2026-09-01")]) + assert omitted == 0 + assert brief == [{ + "id": 1, "title": "M1", "description": "what M1 is for", "status": "active", + "order_index": 1, "total": 4, "completed": 2, "pct": 50.0, + "status_counts": {"todo": 2, "in_progress": 0, "done": 2, "cancelled": 0}, + }] + + +def test_cap_keeps_every_open_milestone_and_the_most_recent_done_ones_in_order(): + rows = _history(done=8, active=3) + brief, omitted = brief_milestone_summary(rows, done_kept=2) + assert omitted == 6 + # The two most recently updated done ones (ids 6, 7) and all open ones, + # still in order_index order. + assert [r["id"] for r in brief] == [6, 7, 8, 9, 10] + + +def test_no_cap_keeps_every_row(): + brief, omitted = brief_milestone_summary(_history(done=8, active=3)) + assert omitted == 0 + assert len(brief) == 11 + + +def _enter_stubs(rows: list[dict]): + return [ + patch("scribe.mcp.tools.projects.projects_svc.get_project", + AsyncMock(return_value=fake_project(id=5))), + patch("scribe.mcp.tools.projects.rulebooks_svc.get_applicable_rules", + AsyncMock(return_value={"rules": [], "truncated": False, + "subscribed_rulebooks": []})), + patch("scribe.mcp.tools.projects.milestones_svc.get_project_milestone_summary", + AsyncMock(return_value=rows)), + patch("scribe.mcp.tools.projects.notes_svc.list_notes", + AsyncMock(side_effect=[([], 0), ([], 0)])), + patch("scribe.mcp.tools.projects.systems_svc.list_systems", + AsyncMock(return_value=[])), + patch("scribe.mcp.tools.projects.systems_tools.bootstrap_systems_ask", + AsyncMock(return_value=None)), + patch("scribe.mcp.tools.projects.coverage_svc.cached_coverage", + AsyncMock(return_value=None)), + patch("scribe.mcp.tools.projects.spawn"), + ] + + +@pytest.mark.asyncio +async def test_enter_project_stays_small_however_long_the_history(): + """The ceiling is the point: a long-lived project's handshake must not + grow with its history. 200 milestones with 5k-character plans would be + ~1M characters if bodies rode along.""" + with contextlib.ExitStack() as stack: + for cm in _enter_stubs(_history(done=190, active=10)): + stack.enter_context(cm) + out = await enter_project(project_id=5) + + summary = out["milestone_summary"] + assert all("body" not in m for m in summary) + assert sum(m["status"] == "done" for m in summary) == 5 + assert sum(m["status"] == "active" for m in summary) == 10 + assert out["milestone_summary_omitted"].startswith("185 older done milestone(s)") + assert "list_milestones(5)" in out["milestone_summary_omitted"] + assert len(json.dumps(out["milestone_summary"], indent=2)) < 10_000 + + +@pytest.mark.asyncio +async def test_omitted_key_is_absent_when_nothing_was_left_out(): + """Attached only when it applies (#2483).""" + with contextlib.ExitStack() as stack: + for cm in _enter_stubs(_history(done=3, active=2)): + stack.enter_context(cm) + out = await enter_project(project_id=5) + assert len(out["milestone_summary"]) == 5 + assert "milestone_summary_omitted" not in out + + +@pytest.mark.asyncio +async def test_get_project_uses_the_same_brief_block(): + with patch("scribe.mcp.tools.projects.projects_svc.get_project", + AsyncMock(return_value=fake_project(id=5))), \ + patch("scribe.mcp.tools.projects.milestones_svc.get_project_milestone_summary", + AsyncMock(return_value=_history(done=9, active=1))), \ + patch("scribe.mcp.tools.projects.rulebooks_svc.get_applicable_rules", + AsyncMock(return_value={"rules": [], "truncated": False, + "subscribed_rulebooks": []})): + out = await get_project(project_id=5) + assert len(out["milestone_summary"]) == 6 + assert all("body" not in m for m in out["milestone_summary"]) + assert out["milestone_summary_omitted"].startswith("4 older done milestone(s)") + + +@pytest.mark.asyncio +async def test_list_milestones_lists_every_milestone_without_plans(): + """The call milestone_summary_omitted points to: every milestone, done + ones included, and no bodies (get_milestone has the plan).""" + with patch("scribe.mcp.tools.milestones.milestones_svc.get_project_milestone_summary", + AsyncMock(return_value=_history(done=30, active=2))): + out = await list_milestones(project_id=5) + assert len(out["milestones"]) == 32 + assert all("body" not in m for m in out["milestones"])