feat(placement): a record you only cite carries its status (#4154)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 50s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / Python tests (push) Failing after 1m4s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 50s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / Python tests (push) Failing after 1m4s
CI & Build / Build & push image (push) Skipped
Step 1 made placement cheap for a task whose status CHANGES: create_task and update_task return where it sits, and the report is written from that. It did nothing for a task a reply merely cites. This milestone's own step-6 review 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. The gap was there to be filled and the nearest-looking id filled it. Two surfaces, one principle: the status arrives with the id. 1. get_project_milestone_summaries gains next_step — the earliest open step, {id, title, status} or None — carried through _BRIEF_FIELDS to enter_project, get_project and list_milestones. One extra flat query for the whole batch, so #2384's fan-out does not come back. OPEN_STEP_STATUSES moves to services/milestones.py and placement.py imports it; both surfaces now answer "what is next" and must not drift on what counts as open. Both step queries take the same readable_notes_clause (rule 78), so a row cannot name a step its own progress numbers exclude. 2. _record_kind renders a task's status: [task (done)], [issue (todo)]. A finished step and an open one read identically before, which is exactly the line the misreport was taken from. Only tasks — is_task IS status-is-not-None on the model, so there is no fallback branch. reporting-back gains the practice, owned and registered in the guidance ownership table: a record you only mention is a record to read. The guards are structural and each fails on the regression it names: the query count is asserted rather than the payload shape, and the two surfaces' agreement is pinned on the rendered ORDER BY, since a mocked session hands back whatever order the test chose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
{
|
{
|
||||||
"name": "scribe",
|
"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).",
|
"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": {
|
"author": {
|
||||||
"name": "Bryan Van Deusen"
|
"name": "Bryan Van Deusen"
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -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
|
the milestone, `position` (step N of M), `progress`, and `next` (the next open
|
||||||
step). Use those values as they came back.
|
step). Use those values as they came back.
|
||||||
- For a wider view, `get_milestone` (a plan and its steps) or `enter_project`
|
- 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
|
- 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.
|
task" — and offer to record it. An honest "untracked" is a placement too.
|
||||||
|
|
||||||
|
|||||||
@@ -24,9 +24,14 @@ async def list_milestones(project_id: int) -> dict:
|
|||||||
"""List milestones for a Scribe project, ordered by order_index.
|
"""List milestones for a Scribe project, ordered by order_index.
|
||||||
|
|
||||||
Returns every milestone, done ones included: id, title, description,
|
Returns every milestone, done ones included: id, title, description,
|
||||||
status (active/done), order_index and progress (total, completed, pct,
|
status (active/done), order_index, progress (total, completed, pct,
|
||||||
status_counts). The plan itself is not listed: get_milestone(id) returns a
|
status_counts) and `next_step`. The plan itself is not listed:
|
||||||
milestone's body and its steps.
|
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()
|
uid = current_user_id()
|
||||||
rows = await milestones_svc.get_project_milestone_summary(uid, project_id)
|
rows = await milestones_svc.get_project_milestone_summary(uid, project_id)
|
||||||
|
|||||||
@@ -77,9 +77,15 @@ async def enter_project(project_id: int) -> dict:
|
|||||||
|
|
||||||
`milestone_summary` is the 5 most recently touched milestones, any status,
|
`milestone_summary` is the 5 most recently touched milestones, any status,
|
||||||
most recent first. Touched counts a step changing, not only the milestone
|
most recent first. Touched counts a step changing, not only the milestone
|
||||||
itself. Each carries its description and progress but NOT its plan:
|
itself. Each carries its description, its progress and its `next_step` —
|
||||||
get_milestone(id) reads a plan and its steps. `milestone_summary_omitted`
|
the earliest step still open, {id, title, status}, or null when none is —
|
||||||
says how many others exist; list_milestones lists them all.
|
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,
|
`unplanned_milestones` is the active milestones that have NO steps yet,
|
||||||
in roadmap order (up to 10; `unplanned_milestones_omitted` counts the
|
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.
|
"""Fetch a Scribe project by ID.
|
||||||
|
|
||||||
Returns full project fields, a milestone_summary list (every milestone,
|
Returns full project fields, a milestone_summary list (every milestone,
|
||||||
with description and progress but no plan body; get_milestone reads a
|
with description, progress and `next_step` — the earliest still-open step,
|
||||||
plan), the project's own rules (project_rules), and applicable_rules: the
|
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
|
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.
|
rule applies too and arrives by retrieval when the work matches it.
|
||||||
"""
|
"""
|
||||||
|
|||||||
@@ -7,9 +7,16 @@ from sqlalchemy import func, select
|
|||||||
from scribe.models import async_session
|
from scribe.models import async_session
|
||||||
from scribe.models.milestone import Milestone
|
from scribe.models.milestone import Milestone
|
||||||
from scribe.models.note import Note
|
from scribe.models.note import Note
|
||||||
|
from scribe.services import access as access_svc
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
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:
|
def embed_milestone(milestone: Milestone) -> None:
|
||||||
"""Refresh a milestone's vectors, fire-and-forget (milestone 415).
|
"""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(
|
async def get_project_milestone_summaries(
|
||||||
user_id: int, project_ids: list[int]
|
user_id: int, project_ids: list[int]
|
||||||
) -> dict[int, list[dict]]:
|
) -> 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
|
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
|
project's milestones, then one more per milestone for its progress. Called
|
||||||
for 25 projects concurrently it asked for ~250 pooled connections against a
|
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
|
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:
|
if not project_ids:
|
||||||
return {}
|
return {}
|
||||||
@@ -261,16 +276,24 @@ async def get_project_milestone_summaries(
|
|||||||
|
|
||||||
counts: dict[int, dict[str, int]] = {}
|
counts: dict[int, dict[str, int]] = {}
|
||||||
step_touched: dict[int, datetime] = {}
|
step_touched: dict[int, datetime] = {}
|
||||||
|
next_step: dict[int, dict] = {}
|
||||||
if milestones:
|
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(
|
rows = await session.execute(
|
||||||
select(
|
select(
|
||||||
Note.milestone_id, Note.status, func.count(Note.id),
|
Note.milestone_id, Note.status, func.count(Note.id),
|
||||||
func.max(Note.updated_at),
|
func.max(Note.updated_at),
|
||||||
)
|
)
|
||||||
.where(
|
.where(
|
||||||
Note.milestone_id.in_([m.id for m in milestones]),
|
Note.milestone_id.in_(milestone_ids),
|
||||||
Note.status.isnot(None),
|
Note.status.isnot(None),
|
||||||
Note.deleted_at.is_(None),
|
Note.deleted_at.is_(None),
|
||||||
|
readable,
|
||||||
)
|
)
|
||||||
.group_by(Note.milestone_id, Note.status)
|
.group_by(Note.milestone_id, Note.status)
|
||||||
)
|
)
|
||||||
@@ -280,6 +303,28 @@ async def get_project_milestone_summaries(
|
|||||||
or latest > step_touched[milestone_id]):
|
or latest > step_touched[milestone_id]):
|
||||||
step_touched[milestone_id] = latest
|
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}
|
out: dict[int, list[dict]] = {pid: [] for pid in project_ids}
|
||||||
for m in milestones:
|
for m in milestones:
|
||||||
entry = m.to_dict()
|
entry = m.to_dict()
|
||||||
@@ -289,6 +334,12 @@ async def get_project_milestone_summaries(
|
|||||||
# written (#4045). Touched is the later of the two.
|
# written (#4045). Touched is the later of the two.
|
||||||
touched = [t for t in (m.updated_at, step_touched.get(m.id)) if t]
|
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
|
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)
|
out.setdefault(m.project_id, []).append(entry)
|
||||||
return out
|
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, [])
|
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
|
# What a milestone LISTING needs: enough to say what each plan is, how far
|
||||||
# along it is. The plan itself (`body`) is get_milestone's job. Summaries once
|
# along it is, and which step is next. The plan itself (`body`) is
|
||||||
# carried it, and on a project with 39 milestones enter_project came to ~222k
|
# get_milestone's job. Summaries once carried it, and on a project with 39
|
||||||
# characters, 168k of them plan bodies. That is past what an MCP client will
|
# 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
|
# 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
|
# through (#4045). user_id / project_id / timestamps repeat what the caller
|
||||||
# already knows.
|
# already knows.
|
||||||
_BRIEF_FIELDS = (
|
_BRIEF_FIELDS = (
|
||||||
"id", "title", "description", "status", "order_index",
|
"id", "title", "description", "status", "order_index",
|
||||||
"total", "completed", "pct", "status_counts",
|
"total", "completed", "pct", "status_counts", "next_step",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -54,9 +54,11 @@ from scribe.models.milestone import Milestone
|
|||||||
from scribe.models.note import Note
|
from scribe.models.note import Note
|
||||||
from scribe.models.project import Project
|
from scribe.models.project import Project
|
||||||
from scribe.services import access as access_svc
|
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__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
|||||||
@@ -640,7 +640,7 @@ async def get_autoinject_config(user_id: int) -> dict:
|
|||||||
|
|
||||||
|
|
||||||
def _record_kind(note) -> str:
|
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,
|
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.
|
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
|
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".
|
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:
|
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"
|
return note.note_type or "note"
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+11
-2
@@ -147,11 +147,20 @@ def _now():
|
|||||||
def fake_note(**attrs) -> MagicMock:
|
def fake_note(**attrs) -> MagicMock:
|
||||||
"""A stand-in Note: own (user_id=7, the caller `_bind_user` binds), live,
|
"""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 /
|
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 …"
|
task_kind / note_type / status for its kind marker, user_id for the
|
||||||
attribution, data for a snippet's language, deleted_at for trash."""
|
"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({
|
return _with_defaults({
|
||||||
"id": 1, "title": "t", "body": "", "tags": [], "user_id": 7,
|
"id": 1, "title": "t", "body": "", "tags": [], "user_id": 7,
|
||||||
"note_type": "note", "is_task": False, "task_kind": "work",
|
"note_type": "note", "is_task": False, "task_kind": "work",
|
||||||
|
"status": "todo" if is_task else None,
|
||||||
"data": None, "deleted_at": None,
|
"data": None, "deleted_at": None,
|
||||||
# Milestone 317: a truthy mock here reads as "this note carries a
|
# Milestone 317: a truthy mock here reads as "this note carries a
|
||||||
# check", which trips the guard on records that may not have one.
|
# check", which trips the guard on records that may not have one.
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -182,6 +182,12 @@ TOPICS: tuple[Topic, ...] = (
|
|||||||
Topic("a settled decision is acted on, not re-opened",
|
Topic("a settled decision is acted on, not re-opened",
|
||||||
"skill:reporting-back", ("already made",),
|
"skill:reporting-back", ("already made",),
|
||||||
"reads as contradicting yourself rather than as being careful"),
|
"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 ──
|
# ── 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("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"),
|
Topic("a note that asserts a fact carries its check", "docstrings", ("verify_with", "expires_when"),
|
||||||
|
|||||||
@@ -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,
|
"updated_at": "2026-01-01T00:00:00+00:00", "last_touched_at": touched,
|
||||||
"total": 4, "completed": 2, "pct": 50.0,
|
"total": 4, "completed": 2, "pct": 50.0,
|
||||||
"status_counts": {"todo": 2, "in_progress": 0, "done": 2, "cancelled": 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",
|
"id": 1, "title": "M1", "description": "what M1 is for", "status": "active",
|
||||||
"order_index": 1, "total": 4, "completed": 2, "pct": 50.0,
|
"order_index": 1, "total": 4, "completed": 2, "pct": 50.0,
|
||||||
"status_counts": {"todo": 2, "in_progress": 0, "done": 2, "cancelled": 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"},
|
||||||
}]
|
}]
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -148,8 +148,10 @@ async def test_injected_menu_labels_the_record_kind():
|
|||||||
hits = [
|
hits = [
|
||||||
(0.92, fake_note(id=1, title="debounce — rate-limit a callback", note_type="snippet")),
|
(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.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.90, fake_note(id=3, title="Auth token expiry", is_task=True,
|
||||||
(0.89, fake_note(id=4, title="Ship the drafter", 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")),
|
(0.88, fake_note(id=5, title="Why we dropped CalDAV")),
|
||||||
]
|
]
|
||||||
with patch.object(plugin_context, "semantic_search_notes",
|
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 "[snippet]" in by_id[1]
|
||||||
assert "[process]" in by_id[2]
|
assert "[process]" in by_id[2]
|
||||||
# Task-ness wins over note_type, and an issue says so rather than "task".
|
# Task-ness wins over note_type, and an issue says so rather than "task".
|
||||||
assert "[issue]" in by_id[3]
|
# A task also carries its status (#4154): #4 is DONE, and a line that said
|
||||||
assert "[task]" in by_id[4]
|
# 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]
|
assert "[note]" in by_id[5]
|
||||||
# Still title-first: the marker is metadata, not an excuse to carry bodies.
|
# Still title-first: the marker is metadata, not an excuse to carry bodies.
|
||||||
assert "body" not in out["context"]
|
assert "body" not in out["context"]
|
||||||
|
|||||||
@@ -487,7 +487,9 @@ async def test_write_path_labels_a_non_snippet_hit_with_its_kind():
|
|||||||
)
|
)
|
||||||
|
|
||||||
ctx = out["context"]
|
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
|
assert '[similar 0.72] "debounce helper"' in ctx # the snippet does not
|
||||||
# The header now names the right opener for each kind.
|
# The header now names the right opener for each kind.
|
||||||
assert "get_task(id)" in ctx and "get_snippet(id)" in ctx
|
assert "get_task(id)" in ctx and "get_snippet(id)" in ctx
|
||||||
|
|||||||
Reference in New Issue
Block a user