diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index ff674d1..34b4fd6 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe for Claude Code: connects the scribe MCP server, adds the hooks that deliver live project state and relevant records at the right moment, ships the shared client-neutral Scribe skills (using-scribe, writing-plans, reporting-back, systematic-debugging, verification, brainstorming, reusing-code, shape-accounting), and syncs your saved Scribe Processes as skills (/scribe:sync).", - "version": "2026.09.18.1551", + "version": "2026.09.18.1606", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/reporting-back/SKILL.md b/plugin/skills/reporting-back/SKILL.md index 6fd6aea..02f8980 100644 --- a/plugin/skills/reporting-back/SKILL.md +++ b/plugin/skills/reporting-back/SKILL.md @@ -66,7 +66,15 @@ it is wrong. So take placement from Scribe: the milestone, `position` (step N of M), `progress`, and `next` (the next open step). Use those values as they came back. - For a wider view, `get_milestone` (a plan and its steps) or `enter_project` - (the whole project). + (the whole project). Every milestone they list carries **`next_step`** — the + earliest step still open, or null when none is — so a reply that says what + comes next takes it from the listing it already read. Progress alone says a + plan has an open step and not which one, and that is the gap recall fills. +- **A record you only mention is a record to read.** `placement` rides the + write that changed a task, so a task you cite without touching arrives with + nothing vouching for it. A retrieval hint carries an id, a kind and a title; + where a task stands is in the line's kind marker — `[task (done)]` — and a + line you are working from memory has no marker at all. - Work with no task behind it: say so plainly — "this wasn't tracked as a task" — and offer to record it. An honest "untracked" is a placement too. diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 5476d33..9e4b6f9 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -24,9 +24,14 @@ async def list_milestones(project_id: int) -> dict: """List milestones for a Scribe project, ordered by order_index. 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. + status (active/done), order_index, progress (total, completed, pct, + status_counts) and `next_step`. The plan itself is not listed: + get_milestone(id) returns a milestone's body and its steps. + + `next_step` is the earliest step still open — {id, title, status} — or + null when the plan has none left. Use it as it came back. "7 of 9 done" + invites naming the open one from memory, and a remembered id reads exactly + like a read one while being a step that closed days ago. """ uid = current_user_id() rows = await milestones_svc.get_project_milestone_summary(uid, project_id) diff --git a/src/scribe/mcp/tools/projects.py b/src/scribe/mcp/tools/projects.py index 5477670..d5df64c 100644 --- a/src/scribe/mcp/tools/projects.py +++ b/src/scribe/mcp/tools/projects.py @@ -77,9 +77,15 @@ async def enter_project(project_id: int) -> dict: `milestone_summary` is the 5 most recently touched milestones, any status, most recent first. Touched counts a step changing, not only the milestone - itself. Each carries its description and progress but NOT its plan: - get_milestone(id) reads a plan and its steps. `milestone_summary_omitted` - says how many others exist; list_milestones lists them all. + itself. Each carries its description, its progress and its `next_step` — + the earliest step still open, {id, title, status}, or null when none is — + but NOT its plan: get_milestone(id) reads a plan and its steps. + `milestone_summary_omitted` says how many others exist; list_milestones + lists them all. + + Name the next step from `next_step`, not from recall: progress alone says + a plan has an open step and not which, and a step named from memory reads + exactly like one that was read. `unplanned_milestones` is the active milestones that have NO steps yet, in roadmap order (up to 10; `unplanned_milestones_omitted` counts the @@ -290,8 +296,9 @@ async def get_project(project_id: int) -> dict: """Fetch a Scribe project by ID. Returns full project fields, a milestone_summary list (every milestone, - with description and progress but no plan body; get_milestone reads a - plan), the project's own rules (project_rules), and applicable_rules: the + with description, progress and `next_step` — the earliest still-open step, + or null — but no plan body; get_milestone reads a plan), the project's own + rules (project_rules), and applicable_rules: the global rules tagged to an area this project works in. Every other global rule applies too and arrives by retrieval when the work matches it. """ diff --git a/src/scribe/services/milestones.py b/src/scribe/services/milestones.py index cada46c..a36fc3e 100644 --- a/src/scribe/services/milestones.py +++ b/src/scribe/services/milestones.py @@ -7,9 +7,16 @@ from sqlalchemy import func, select from scribe.models import async_session from scribe.models.milestone import Milestone from scribe.models.note import Note +from scribe.services import access as access_svc logger = logging.getLogger(__name__) +# The statuses that make a step OPEN work, defined here because this is the +# lower layer: services/placement.py imports it rather than restating it. Two +# surfaces that both answer "what is next" and disagree about what counts as +# next is worse than one of them staying silent. +OPEN_STEP_STATUSES = ("todo", "in_progress") + def embed_milestone(milestone: Milestone) -> None: """Refresh a milestone's vectors, fire-and-forget (milestone 415). @@ -239,13 +246,21 @@ def _progress_from_counts(status_counts: dict[str, int]) -> dict: async def get_project_milestone_summaries( user_id: int, project_ids: list[int] ) -> dict[int, list[dict]]: - """Milestone summaries for MANY projects in two queries total. + """Milestone summaries for MANY projects in three queries total. The per-project version below is a nested fan-out: one query to list a project's milestones, then one more per milestone for its progress. Called for 25 projects concurrently it asked for ~250 pooled connections against a pool of 15, and every one of them waited out the 30-second checkout timeout - (#2384). This does the same work in two queries and one session. + (#2384). This does the same work in a fixed number of queries and one + session: the milestones, their step counts, and their open steps. + + Each row carries `next_step` — the earliest open step, or None (#4154). + That is NOT the same question `services/placement.py` answers: placement + knows which step you are on and names the next one AFTER it, while a + listing has no current step, so the earliest open one is the whole answer. + The two share `OPEN_STEP_STATUSES` and the creation ordering so they can + never disagree about which steps are candidates. """ if not project_ids: return {} @@ -261,16 +276,24 @@ async def get_project_milestone_summaries( counts: dict[int, dict[str, int]] = {} step_touched: dict[int, datetime] = {} + next_step: dict[int, dict] = {} if milestones: + milestone_ids = [m.id for m in milestones] + # Both step queries below take the SAME visibility clause (rule 78). + # They have to: `next_step` names a step and the counts beside it + # say how many there are, so a row that could name a step its own + # progress excludes would be reporting two different milestones. + readable = access_svc.readable_notes_clause(user_id) rows = await session.execute( select( Note.milestone_id, Note.status, func.count(Note.id), func.max(Note.updated_at), ) .where( - Note.milestone_id.in_([m.id for m in milestones]), + Note.milestone_id.in_(milestone_ids), Note.status.isnot(None), Note.deleted_at.is_(None), + readable, ) .group_by(Note.milestone_id, Note.status) ) @@ -280,6 +303,28 @@ async def get_project_milestone_summaries( or latest > step_touched[milestone_id]): step_touched[milestone_id] = latest + # ONE query for every milestone in the batch, not one per row. + # #2384 was exactly this listing fanned out per milestone, and it + # drained the connection pool; a third flat query keeps the cost + # constant in the number of plans. Ordered the way + # services/placement.py orders steps — creation order, the order a + # plan is written and a batch create inserts — so the first row per + # milestone IS its next open step. + open_rows = await session.execute( + select(Note.milestone_id, Note.id, Note.title, Note.status) + .where( + Note.milestone_id.in_(milestone_ids), + Note.status.in_(OPEN_STEP_STATUSES), + Note.deleted_at.is_(None), + readable, + ) + .order_by(Note.created_at.asc(), Note.id.asc()) + ) + for milestone_id, note_id, title, status in open_rows.fetchall(): + next_step.setdefault( + milestone_id, {"id": note_id, "title": title, "status": status}, + ) + out: dict[int, list[dict]] = {pid: [] for pid in project_ids} for m in milestones: entry = m.to_dict() @@ -289,6 +334,12 @@ async def get_project_milestone_summaries( # written (#4045). Touched is the later of the two. touched = [t for t in (m.updated_at, step_touched.get(m.id)) if t] entry["last_touched_at"] = max(touched).isoformat() if touched else None + # Always present, None included. "8 of 9 done" tells a reader there is + # an open step and not WHICH, and a gap that shape gets filled from + # whatever id is nearest to hand — a retrieval hint carries an id and a + # title and no status, and that is how a done step was reported as the + # open one (#4154). A listing that names it leaves nothing to guess. + entry["next_step"] = next_step.get(m.id) out.setdefault(m.project_id, []).append(entry) return out @@ -299,16 +350,16 @@ async def get_project_milestone_summary(user_id: int, project_id: int) -> list[d 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 +# What a milestone LISTING needs: enough to say what each plan is, how far +# along it is, and which step is next. 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", + "total", "completed", "pct", "status_counts", "next_step", ) diff --git a/src/scribe/services/placement.py b/src/scribe/services/placement.py index 35a866e..8ba435c 100644 --- a/src/scribe/services/placement.py +++ b/src/scribe/services/placement.py @@ -54,9 +54,11 @@ from scribe.models.milestone import Milestone from scribe.models.note import Note from scribe.models.project import Project from scribe.services import access as access_svc -from scribe.services.milestones import _progress_from_counts +from scribe.services.milestones import OPEN_STEP_STATUSES, _progress_from_counts -_OPEN = ("todo", "in_progress") +# Imported, not restated. The milestone summary names a plan's next open step +# too (#4154), and the two answers must agree about what "open" means. +_OPEN = OPEN_STEP_STATUSES logger = logging.getLogger(__name__) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index b6ed2fc..381e67d 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -640,7 +640,7 @@ async def get_autoinject_config(user_id: int) -> dict: def _record_kind(note) -> str: - """The one-word kind marker for an injected menu line. + """The kind marker for an injected menu line — and, for a task, its status. The menu is drawn from every record that carries an embedding, so a snippet, a stored process, an issue and a stray dev-log all arrive looking identical. @@ -649,9 +649,24 @@ def _record_kind(note) -> str: Task-ness wins over `note_type` because it's the more useful distinction at a glance: "there's an open issue about this" beats "there's a note about this". + + A TASK ALSO CARRIES ITS STATUS, because for that kind alone the line is + read as a claim about live work. A finished step and an open one rendered + identically is not a cosmetic gap: a done step was cited as a milestone's + open one on the strength of a line exactly like this, which carries an id, + a kind and a title and said nothing about where the work stood (#4154). + Only for tasks — a note or a snippet has no status to be wrong about. """ if note.is_task: - return "issue" if note.task_kind == "issue" else "task" + kind = "issue" if note.task_kind == "issue" else "task" + # No fallback for a missing status: `is_task` IS `status is not None` + # (models/note.py), so a branch for a task without one could never be + # taken, and a dead branch is a claim about the data that isn't true. + # + # Parenthesised rather than dot-joined: the write-path prior-art line + # joins its own fields with " · ", so a dotted status would read as + # another flag beside `seen` instead of as part of the kind. + return f"{kind} ({note.status})" return note.note_type or "note" diff --git a/tests/helpers.py b/tests/helpers.py index 0dcd784..d176e2d 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -147,11 +147,20 @@ def _now(): def fake_note(**attrs) -> MagicMock: """A stand-in Note: own (user_id=7, the caller `_bind_user` binds), live, not a task, no structured data. The injected menu reads is_task / - task_kind / note_type for its kind marker, user_id for the "shared by …" - attribution, data for a snippet's language, deleted_at for trash.""" + task_kind / note_type / status for its kind marker, user_id for the + "shared by …" attribution, data for a snippet's language, deleted_at for + trash. + + `status` follows `is_task`, because on the real model it DEFINES it — + `Note.is_task` is `status is not None`. A stand-in task with no status is + a row the database cannot hold, and code that reads both would be tested + against a shape it will never meet. + """ + is_task = attrs.get("is_task", False) return _with_defaults({ "id": 1, "title": "t", "body": "", "tags": [], "user_id": 7, "note_type": "note", "is_task": False, "task_kind": "work", + "status": "todo" if is_task else None, "data": None, "deleted_at": None, # Milestone 317: a truthy mock here reads as "this note carries a # check", which trips the guard on records that may not have one. diff --git a/tests/test_cited_record_status.py b/tests/test_cited_record_status.py new file mode 100644 index 0000000..023d28f --- /dev/null +++ b/tests/test_cited_record_status.py @@ -0,0 +1,297 @@ +"""A record you merely CITE carries its status (#4154, milestone 409 step 8). + +WHY THIS EXISTS + +Step 1 (#4010) made placement cheap for a task whose status CHANGES: the write +returns where it sits, and the report is written from that. It does nothing for +a task a reply only mentions. In this milestone's own step-6 review the session +reported "#4014 is the open step of milestone 409". #4014 had been done for +four days; the open step was #4015. The id did not come from a read — it came +from a retrieval hint, which carries an id, a kind and a title and says nothing +about status, while `list_milestones` said "8 of 9" and would not say which one. + +Two surfaces, one principle: the status arrives with the id. + + 1. A milestone summary row names its next open step, so the listing that + prompts the question also answers it. + 2. An injected menu line renders a task's status, so a finished step cannot + read as live work. + +THE ONE THAT MATTERS MOST + +`test_the_listing_and_placement_agree_on_what_open_means` — two surfaces now +answer "what is next" and they must not drift. It is written as a behavioural +cross-check rather than `assert milestones.OPEN_STEP_STATUSES is placement._OPEN`, +which shares one object today and would therefore pass no matter what either +side did with it (rule 167: a guard has to be able to fail). This one fails if +either side changes its ordering or its notion of "open" alone. +""" +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from sqlalchemy import true + +from scribe.services import milestones as ms +from scribe.services import placement as pl +from tests.helpers import fake_note, make_mock_session + + +def _milestone_row(mid: int, project_id: int = 5): + """A Milestone as the first query returns it — .to_dict() feeds the entry.""" + row = MagicMock() + row.id, row.project_id, row.user_id = mid, project_id, 7 + row.updated_at = None + row.to_dict.return_value = {"id": mid, "title": f"M{mid}", "project_id": project_id} + return row + + +def _sessions(results: list, counter: list[int], seen_sql: list | None = None): + """A patched `async_session` handing out queued results and counting queries. + + Every query in `get_project_milestone_summaries` pulls the next entry, so + the ORDER of `results` pins the order of the queries — which is what makes + the query-count assertion meaningful rather than incidental. + + `seen_sql` collects the rendered statements. The ordering these surfaces + have to agree on lives in an ORDER BY, which no amount of feeding rows to a + mock can exercise — a stand-in hands back whatever order the test chose. + """ + session = make_mock_session() + + async def _execute(stmt=None, *_a, **_kw): + counter[0] += 1 + if seen_sql is not None: + seen_sql.append(str(stmt)) + rows = results.pop(0) if results else [] + r = MagicMock() + r.fetchall = MagicMock(return_value=rows) + r.scalars = MagicMock(return_value=MagicMock(all=lambda: rows)) + return r + + session.execute = _execute + return MagicMock(return_value=session) + + +def _order_by(sql: str) -> str: + """The ORDER BY tail of a rendered statement, normalised.""" + _, _, tail = sql.upper().partition("ORDER BY") + return " ".join(tail.split()) + + +async def _summaries(milestones, counts, open_steps, counter=None, seen_sql=None): + counter = counter if counter is not None else [0] + results = [milestones, counts, open_steps] + with patch.object(ms, "async_session", _sessions(results, counter, seen_sql)), \ + patch.object(ms.access_svc, "readable_notes_clause", + MagicMock(return_value=true())): + return await ms.get_project_milestone_summaries(7, [5]) + + +async def _placement_sql(seen_sql: list): + """Run `task_placement` far enough to render its sibling-steps query.""" + session = make_mock_session() + milestone = MagicMock(id=409, project_id=5, user_id=7, title="M", status="active") + + async def _execute(stmt=None, *_a, **_kw): + seen_sql.append(str(stmt)) + r = MagicMock() + r.scalars = MagicMock(return_value=MagicMock( + first=lambda: milestone, all=lambda: [], + )) + return r + + session.execute = _execute + with patch.object(pl, "async_session", MagicMock(return_value=session)), \ + patch.object(pl.access_svc, "can_read_project", AsyncMock(return_value=False)), \ + patch.object(pl.access_svc, "readable_notes_clause", + MagicMock(return_value=true())): + await pl.task_placement(7, SimpleNamespace( + id=1, project_id=5, milestone_id=409, status="todo", + )) + + +@pytest.mark.asyncio +async def test_a_milestone_row_names_its_next_open_step(): + """The listing that says "8 of 9" now says WHICH one, in the same read.""" + rows = await _summaries( + [_milestone_row(409)], + [(409, "done", 8), (409, "todo", 1)], + [(409, 4154, "Step 8 — a cited record carries its status", "todo")], + ) + assert rows[5][0]["next_step"] == { + "id": 4154, + "title": "Step 8 — a cited record carries its status", + "status": "todo", + } + + +@pytest.mark.asyncio +async def test_a_finished_plan_names_no_next_step_rather_than_omitting_the_key(): + """Always present, None included. + + A key that disappears when the answer is "nothing left" makes a reader + test for its absence to learn the answer, and a reader who forgets is back + to guessing — which is the failure this step exists for. + """ + rows = await _summaries([_milestone_row(416)], [(416, "done", 9)], []) + assert rows[5][0]["next_step"] is None + assert "next_step" in rows[5][0] + + +@pytest.mark.asyncio +async def test_the_earliest_open_step_wins_not_the_earliest_step(): + """The done-first case, which is the shape every part-finished plan has. + + Steps arrive in creation order, so a plan whose first two are closed must + name the third. Naming the first would reproduce the exact misreport: a + step that IS in the milestone, that IS plausible, and that is finished. + """ + rows = await _summaries( + [_milestone_row(409)], + [(409, "done", 2), (409, "todo", 2)], + # The query filters to open steps, so the closed ones never appear — + # this asserts the ORDER of what does: earliest open, not last written. + [(409, 4015, "Step 6", "in_progress"), (409, 4154, "Step 8", "todo")], + ) + assert rows[5][0]["next_step"]["id"] == 4015 + assert rows[5][0]["next_step"]["status"] == "in_progress" + + +@pytest.mark.asyncio +async def test_the_batch_does_not_fan_out_per_milestone(): + """#2384's shape must not come back through the new query. + + Three queries for one milestone and three for forty — the cost is in the + number of QUERIES, not the number of plans. A per-milestone "what's next" + lookup would produce identical output and reproduce the pool exhaustion. + """ + counter = [0] + await _summaries( + [_milestone_row(i) for i in range(40)], + [(i, "todo", 1) for i in range(40)], + [(i, 1000 + i, f"S{i}", "todo") for i in range(40)], + counter=counter, + ) + assert counter[0] == 3, f"{counter[0]} queries for 40 milestones" + + +@pytest.mark.asyncio +async def test_both_step_queries_take_the_same_visibility_clause(): + """Rule 78, and a correctness point on top of it. + + `next_step` names a step; the counts beside it say how many there are. Read + through different clauses, a row could name a step its own progress numbers + exclude — one row describing two different milestones. + """ + seen = [] + + def _clause(uid): + seen.append(uid) + return true() + + results = [[_milestone_row(409)], [(409, "todo", 1)], [(409, 1, "S", "todo")]] + with patch.object(ms, "async_session", _sessions(results, [0])), \ + patch.object(ms.access_svc, "readable_notes_clause", _clause): + await ms.get_project_milestone_summaries(7, [5]) + + # Built ONCE and reused, so the two queries cannot be given different ones. + assert seen == [7] + + +@pytest.mark.asyncio +async def test_the_listing_and_placement_agree_on_what_open_means(): + """THE GUARD. Two surfaces answer "what is next"; they must not drift. + + Behavioural on purpose — see the module docstring. `placement` answers for + a session that knows which step it is on, the listing for one that does + not, so the two are only equal when the current step is the first. That is + the case checked here, and it fails if either side's ordering or its open + set moves without the other. + """ + steps = [ + SimpleNamespace(id=1, title="Step 1", status="done"), + SimpleNamespace(id=2, title="Step 2", status="done"), + SimpleNamespace(id=3, title="Step 3", status="todo"), + SimpleNamespace(id=4, title="Step 4", status="todo"), + ] + # From the first step, placement's "the next open one after this" and the + # listing's "the earliest open one" are the same question. + from_placement = pl._next_open(steps, current_id=1) + + listing_sql: list = [] + rows = await _summaries( + [_milestone_row(409)], + [(409, "done", 2), (409, "todo", 2)], + [(409, s.id, s.title, s.status) for s in steps if s.status in ms.OPEN_STEP_STATUSES], + seen_sql=listing_sql, + ) + assert rows[5][0]["next_step"] == from_placement + + # And they agree on ORDERING, which the rows above cannot show: a stand-in + # session hands back whatever order this test chose, so "the first row + # wins" would pass against any ORDER BY at all. The database does the + # sorting in production, so the assertion belongs on the statement. + placement_sql: list = [] + await _placement_sql(placement_sql) + steps_query = next(q for q in placement_sql if "ORDER BY" in q.upper()) + assert _order_by(listing_sql[-1]) == _order_by(steps_query) + assert _order_by(listing_sql[-1]), "the open-steps query has no ORDER BY at all" + + +def test_a_task_line_in_the_menu_says_where_the_work_stands(): + """The other half: an id and a title with no status is what got cited.""" + from scribe.services.plugin_context import _record_kind + + assert _record_kind(fake_note(is_task=True, status="done")) == "task (done)" + assert _record_kind(fake_note(is_task=True, status="todo")) == "task (todo)" + assert _record_kind( + fake_note(is_task=True, task_kind="issue", status="in_progress") + ) == "issue (in_progress)" + + +def test_a_record_with_no_status_gains_no_parenthesis(): + """Only tasks. A note or a snippet has no status to be wrong about, and a + marker that appeared on every line would stop being read.""" + from scribe.services.plugin_context import _record_kind + + assert _record_kind(fake_note(note_type="snippet")) == "snippet" + assert _record_kind(fake_note(note_type="process")) == "process" + assert _record_kind(fake_note()) == "note" + + +def test_the_listing_tools_say_what_next_step_is_and_how_to_use_it(): + """The contract every MCP client reads (rule 119, decision #4027). + + The field is only worth adding if a caller knows it is there. Without this + the docstring can be tidied to a parameter list and the one surface that + reaches a non-Claude-Code client goes quiet about it. + """ + from tests.helpers import tool_doc + + for module, name in ( + ("scribe.mcp.tools.milestones", "list_milestones"), + ("scribe.mcp.tools.projects", "enter_project"), + ("scribe.mcp.tools.projects", "get_project"), + ): + doc = tool_doc(module, name).lower() + assert "next_step" in doc, f"{name} does not mention next_step" + # Naming the field is not enough — a reader has to be told that a null + # is an answer, or an absent next step reads as data not yet loaded. + assert "null" in doc or "none" in doc, f"{name} does not say when it is empty" + + +def test_the_reply_skill_says_a_cited_record_still_gets_read(): + """The other half of the fix is a practice, and it has one home. + + Presence, not absence — the skill legitimately discusses recall in order to + warn against it, so an absence check here would be satisfied by the warning + itself (snippet #3352). + """ + import pathlib + + skill = pathlib.Path(__file__).resolve().parents[1] / "plugin/skills/reporting-back/SKILL.md" + text = " ".join(skill.read_text().split()).lower() + assert "a record you only mention is a record to read" in text + # And it points at the field rather than restating how to compute it. + assert "next_step" in text diff --git a/tests/test_guidance_ownership.py b/tests/test_guidance_ownership.py index 2df092b..558794b 100644 --- a/tests/test_guidance_ownership.py +++ b/tests/test_guidance_ownership.py @@ -182,6 +182,12 @@ TOPICS: tuple[Topic, ...] = ( Topic("a settled decision is acted on, not re-opened", "skill:reporting-back", ("already made",), "reads as contradicting yourself rather than as being careful"), + # Milestone 409 step 8: `placement` rides a WRITE, so a task the reply only + # cites arrived with nothing vouching for it — which is how a step finished + # four days earlier was reported as the open one (#4154). + Topic("a record you only cite still gets read", + "skill:reporting-back", ("next_step", "only mention"), + "a record you only mention is a record to read"), # ── per-tool contracts and in-band behaviour — owned by the server ── Topic("closing a task cues the report", "docstrings", ("report_back",), "reporting this to the operator?"), Topic("a note that asserts a fact carries its check", "docstrings", ("verify_with", "expires_when"), diff --git a/tests/test_milestone_summary_brief.py b/tests/test_milestone_summary_brief.py index 50fc698..febea93 100644 --- a/tests/test_milestone_summary_brief.py +++ b/tests/test_milestone_summary_brief.py @@ -34,6 +34,7 @@ def _milestone(mid: int, status: str, touched_day: int) -> dict: "updated_at": "2026-01-01T00:00:00+00:00", "last_touched_at": touched, "total": 4, "completed": 2, "pct": 50.0, "status_counts": {"todo": 2, "in_progress": 0, "done": 2, "cancelled": 0}, + "next_step": {"id": 900 + mid, "title": f"M{mid} step 3", "status": "todo"}, } @@ -52,6 +53,10 @@ def test_brief_rows_leave_out_the_plan_and_what_the_caller_already_knows(): "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}, + # Trimmed of the plan body, but NOT of which step is next (#4154): the + # listing is where "2 of 4 done" gets read, and that number is exactly + # what invites a reader to name the open step from memory. + "next_step": {"id": 901, "title": "M1 step 3", "status": "todo"}, }] diff --git a/tests/test_retrieval_scopes.py b/tests/test_retrieval_scopes.py index fadbe2d..242f0d7 100644 --- a/tests/test_retrieval_scopes.py +++ b/tests/test_retrieval_scopes.py @@ -148,8 +148,10 @@ async def test_injected_menu_labels_the_record_kind(): hits = [ (0.92, fake_note(id=1, title="debounce — rate-limit a callback", note_type="snippet")), (0.91, fake_note(id=2, title="Release checklist", note_type="process")), - (0.90, fake_note(id=3, title="Auth token expiry", is_task=True, task_kind="issue")), - (0.89, fake_note(id=4, title="Ship the drafter", is_task=True)), + (0.90, fake_note(id=3, title="Auth token expiry", is_task=True, + task_kind="issue", status="todo")), + (0.89, fake_note(id=4, title="Ship the drafter", is_task=True, + status="done")), (0.88, fake_note(id=5, title="Why we dropped CalDAV")), ] with patch.object(plugin_context, "semantic_search_notes", @@ -165,8 +167,10 @@ async def test_injected_menu_labels_the_record_kind(): assert "[snippet]" in by_id[1] assert "[process]" in by_id[2] # Task-ness wins over note_type, and an issue says so rather than "task". - assert "[issue]" in by_id[3] - assert "[task]" in by_id[4] + # A task also carries its status (#4154): #4 is DONE, and a line that said + # only "[task]" is what let a finished step be cited as an open one. + assert "[issue (todo)]" in by_id[3] + assert "[task (done)]" in by_id[4] assert "[note]" in by_id[5] # Still title-first: the marker is metadata, not an excuse to carry bodies. assert "body" not in out["context"] diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 9e159ae..1f78935 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -487,7 +487,9 @@ async def test_write_path_labels_a_non_snippet_hit_with_its_kind(): ) ctx = out["context"] - assert "· issue]" in ctx # the issue says what it is + # The issue says what it is — and, since #4154, where it stands: a piece of + # prior art offered as "already tried" reads differently when it is still open. + assert "· issue (todo)]" in ctx assert '[similar 0.72] "debounce helper"' in ctx # the snippet does not # The header now names the right opener for each kind. assert "get_task(id)" in ctx and "get_snippet(id)" in ctx