From b5df9d6dcaeb4b32a7afc291034847642104f0bf Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 18 Sep 2026 11:52:14 -0400 Subject: [PATCH 1/7] feat(plugin): a reply's sections are chosen, not filled (#4153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone 409 step 6 measured the scaffold on live sessions and found its two halves disagreeing: adherence passed and the read test failed. Completion replies carried every section the table asks for and were still hard to read. The cause was in the skill, not in compliance with it. It said to pick a kind of reply "then fill its sections... keep them even when one is short", which is an instruction to complete a form, and nothing anywhere set a ceiling. A faithful reply and an unreadable one were the same reply. Four changes to the discipline around the scaffold. The categories and their sections are untouched. - Sections are what to consider including, not a form to complete. A section answering a standing question ("does anything need me?") is always answered, even with "nothing"; a section that explains earns its place only when it changes what the operator does. Otherwise it belongs in the record's log, where it is available and not in the way. - Write the shortest reply that carries the answer, with named exceptions so this cannot be read as "always be terse". - "Needs you" takes BOTH tests: theirs to decide, AND work is waiting on it. A question answerable by reading something or taking an available measurement is work not yet done, not a request — settle it, say which way you went, and leave them free to overrule. - A decision already made gets acted on. Re-arguing a settled question reads as contradicting yourself rather than as being careful, and costs the operator the decision twice. "Before sending" gains a second pass for what can go, since the existing check asks what is MISSING, which a bloated reply passes. Guards in tests/test_reply_discipline.py, three topics registered for ownership. Every guard was falsified against the pre-change text before committing (rule 167): all five fail on it and pass on the fix, and the sixth deliberately passes both since it guards the scaffold against collateral damage. No absence checks — the skill legitimately discusses filling in order to warn against it, so asserting "fill" is absent would false-alarm on the corrected text (snippet #3352). Instance-agnostic per rule 115: the added text carries no record ids, no software-specific terms and no verbatim quotes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- plugin/.claude-plugin/plugin.json | 2 +- plugin/skills/reporting-back/SKILL.md | 45 ++++++++- tests/test_guidance_ownership.py | 12 +++ tests/test_reply_discipline.py | 137 ++++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 4 deletions(-) create mode 100644 tests/test_reply_discipline.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 35f110d..ff674d1 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.0436", + "version": "2026.09.18.1551", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/reporting-back/SKILL.md b/plugin/skills/reporting-back/SKILL.md index a2f1288..6fd6aea 100644 --- a/plugin/skills/reporting-back/SKILL.md +++ b/plugin/skills/reporting-back/SKILL.md @@ -10,9 +10,25 @@ while you worked: they don't hold the files you read, the names you used or the order you did things in. A reply that follows *your* path is accurate and still unreadable to them. Shape it around **where the work stands**. -Pick the kind of reply first (the tables below), then fill its sections. The -sections are what lets the operator find things at a glance, so keep them even -when one is short — "**Needs you:** nothing" is an answer they were looking for. +Pick the kind of reply first (the tables below). Its sections are **what to +consider including, not a form to complete.** + +Two kinds of section, and they behave differently: + +- **A section that answers a standing question** — *does anything need me? what + happens next?* — is always answered, even when the answer is nothing. + "**Needs you:** nothing" is what they were looking for. +- **A section that explains** — how it was done, why that way, what else you + noticed — earns its place only when it changes what the operator does or + decides. When it would not, leave it out: that detail belongs in the record's + log, where it is available and not in the way. + +**Write the shortest reply that carries the answer.** To someone reading +quickly, length is not thoroughness — it is work handed back to them. A reply +that fills every heading faithfully and runs a full screen is worse than four +lines naming the two things that changed their position. Extra length has to be +earned: a comparison they asked for, options that need laying side by side, a +measurement whose numbers are the point. ## Every reply @@ -33,6 +49,13 @@ when one is short — "**Needs you:** nothing" is an answer they were looking fo working. If a term has to appear, explain it once. - **Place the work in Scribe.** Name the task, issue or milestone it belongs to, by id *and* title (using-scribe: "Name the record, never just its number"). +- **A decision already made gets acted on, and the reply says what you did with + it.** Once the operator has chosen, that is the input to the work, not a topic + to revisit. If something you have since learned genuinely overturns the + choice, say so once and plainly — name the new evidence and what it changes — + and otherwise let the decision stand. Laying out the trade-offs of a settled + question again reads as contradicting yourself rather than as being careful, + and it costs the operator the decision twice. ## Take the placement from the record @@ -137,6 +160,15 @@ Notes on each section: - **Needs you** — an action, an approval, a decision, or "nothing". If it's an action, give the reason with it. An approval you are holding for also gets its own **Approval requested** section, and this line points at it. + + **Two tests, and it takes both: is this theirs to decide, and is work waiting + on it?** A choice that is genuinely theirs — a priority, a trade-off only they + can price, something they have to live with afterwards — belongs here. A + question you could settle by reading something, by taking a measurement you + already have access to, or by choosing the obvious default does not: that is + work not yet done, and sending it moves your uncertainty onto them. Settle it, + say which way you went and why, and leave them free to overrule you. This + section is for what blocks them, not for what you are unsure about. - **Next** — from `placement.next`, or say the milestone is finished. If you found something you didn't fix, the offer to fix it goes here. @@ -146,3 +178,10 @@ Read the reply as the operator will: someone who wasn't there, reading quickly. Can they tell **what was done, whether anything needs them, and what happens next** without asking a follow-up? If not, the sections are what's missing — not more detail. + +Then read it once more for **what can go**. A section filled because it was in +the table, reasoning supporting a conclusion nobody is going to dispute, a +finding already written to the record — none of it changes what the operator +does, so none of it belongs in the reply. Cutting is not hiding: the log holds +it, and the reply stays readable. A reply that has been cut twice is the one +they can act on. diff --git a/tests/test_guidance_ownership.py b/tests/test_guidance_ownership.py index 656a6c3..2df092b 100644 --- a/tests/test_guidance_ownership.py +++ b/tests/test_guidance_ownership.py @@ -170,6 +170,18 @@ TOPICS: tuple[Topic, ...] = ( Topic("the operator's own reply shapes come first", "skill:reporting-back", ("reply_preferences", 'content_type="rule"'), "the operator's own shapes come first"), + # Milestone 409 step 7: the sections were being FILLED rather than chosen, + # so a reply could satisfy every heading and still be unreadable. These + # three are the discipline around the scaffold, not the scaffold itself. + Topic("sections are chosen, not filled, and the reply is cut twice", + "skill:reporting-back", ("not a form to complete", "what can go"), + "write the shortest reply that carries the answer"), + Topic("a needs-you item is theirs to decide and blocks work", + "skill:reporting-back", ("is this theirs to decide",), + "this section is for what blocks them, not for what you are unsure about"), + Topic("a settled decision is acted on, not re-opened", + "skill:reporting-back", ("already made",), + "reads as contradicting yourself rather than as being careful"), # ── 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_reply_discipline.py b/tests/test_reply_discipline.py new file mode 100644 index 0000000..72df2d7 --- /dev/null +++ b/tests/test_reply_discipline.py @@ -0,0 +1,137 @@ +"""A reply's sections are chosen, not filled (#4153, milestone 409 step 7). + +WHY THIS EXISTS + +Step 6 measured the scaffold on live sessions and found the two halves +disagreeing: **adherence passed and the read test failed.** Completion replies +carried every section the table asks for — placement, what changed, what needs +the operator, what comes next — and the operator still could not read them. + +The cause was in the skill rather than in compliance with it. It said to pick a +kind of reply "then fill its sections… keep them even when one is short", which +is an instruction to complete a form. Nothing anywhere set a ceiling, and a +section with a heading and nothing to say gets filled rather than dropped. So a +faithful reply and an unreadable one were the same reply. + +WHAT IS PINNED + +The discipline, not the scaffold. The fifteen categories and their sections are +unchanged and are not this file's subject: + + 1. Sections are chosen — a standing question is always answered, an + explanation earns its place. + 2. The reply is as short as the answer allows, and gets a second pass for + what can go. + 3. "Needs you" takes BOTH tests: theirs to decide, and blocking. + 4. A decision already made is acted on rather than re-argued. + +RULE 167, AND WHY THERE IS NO ABSENCE CHECK HERE + +The obvious guard — assert the skill no longer tells anyone to "fill" a +section — is the trap snippet #3352 names. This skill legitimately discusses +filling in order to warn against it ("not a form to complete", "a section +filled because it was in the table"), so an absence check would fail on the +corrected text: a false alarm about the very thing it protects. + +So every assertion here is a PRESENCE check, and one is POSITIONAL — +`test_the_needs_you_test_sits_with_the_needs_you_section` pins where the test +lives, not merely that the words occur somewhere in the file. All four were +falsified against the pre-#4153 text before being committed. +""" +from __future__ import annotations + +import pathlib + +ROOT = pathlib.Path(__file__).resolve().parents[1] +SKILL = ROOT / "plugin/skills/reporting-back/SKILL.md" + + +def _flat() -> str: + """Whitespace-flattened: the file is hard-wrapped, so phrases straddle lines.""" + return " ".join(SKILL.read_text().split()).lower() + + +def test_sections_are_chosen_rather_than_completed(): + """The framing the operator reacted to was "fill its sections".""" + text = _flat() + assert "not a form to complete" in text + # Both halves of the distinction, or "choose" collapses back into "fill": + # a standing question is answered even when the answer is nothing, an + # explanation is dropped when it changes nothing. + assert "always answered, even when the answer is nothing" in text + assert "changes what the operator does or decides" in text + + +def test_the_reply_carries_a_length_discipline(): + """Without a ceiling, every section is an invitation to keep writing.""" + text = _flat() + assert "write the shortest reply that carries the answer" in text + # And the ceiling has named exceptions, so this cannot be read as + # "always be terse" — a measurement the operator asked for still earns room. + assert "extra length has to be earned" in text + + +def test_the_reply_gets_a_second_pass_for_what_can_go(): + """Cutting is a separate act from writing, and needs saying separately. + + The pre-existing check asked whether anything was MISSING, which a bloated + reply passes. + """ + text = _flat() + assert "what can go" in text + # Cutting must not read as withholding, or it will not be done. + assert "cutting is not hiding" in text + + +def test_the_needs_you_test_sits_with_the_needs_you_section(): + """POSITIONAL. The test has to be where the section is defined. + + A reader reaches this while writing that section; stated anywhere else it + is a paragraph nobody is reading at the moment it applies. Asserted by + offset rather than by presence, so moving it away fails here. + """ + raw = " ".join(SKILL.read_text().split()) + lowered = raw.lower() + + needs_you = lowered.index("- **needs you** — an action") + the_test = lowered.index("is this theirs to decide") + next_bullet = lowered.index("- **next** —", needs_you) + + assert needs_you < the_test < next_bullet, ( + "the needs-you test has moved out of the Needs you bullet; a reader " + "writing that section will not meet it" + ) + # Both halves are load-bearing: "theirs" alone still admits a question the + # session could have answered, "blocking" alone admits one that is not + # theirs to make. + assert "work waiting on it" in lowered + assert "not for what you are unsure about" in lowered + + +def test_a_settled_decision_is_acted_on_rather_than_re_argued(): + """Re-opening a decision reads as self-contradiction, not as diligence. + + Phrased as a practice rather than a prohibition (rule 165): the instruction + is what to DO with a decision, with the failure named after it. + """ + text = _flat() + assert "a decision already made gets acted on" in text + assert "reads as contradicting yourself rather than as being careful" in text + # The escape hatch stays open, or this becomes a rule against ever + # correcting anything — which is the opposite of what is wanted. + assert "genuinely overturns the choice" in text + + +def test_the_scaffold_itself_is_untouched(): + """This step changed the discipline AROUND the sections, not the sections. + + If a future edit deletes a category while tightening the prose, that is a + change to #4009's subject and should not ride along silently here. + """ + text = _flat() + for section in ("where this sits", "what now works", "how / why", + "needs you", "next"): + assert section in text, f"completion-report section {section!r} is gone" + for kind in ("completion", "finding", "blocked / failed", "progress", + "decision", "clarification", "handoff", "approval", "conflict"): + assert kind in text, f"reply kind {kind!r} is gone" From 5c6175ad977af8980963f858cc703f98bfd3fa7f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 18 Sep 2026 12:08:00 -0400 Subject: [PATCH 2/7] feat(placement): a record you only cite carries its status (#4154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- plugin/.claude-plugin/plugin.json | 2 +- plugin/skills/reporting-back/SKILL.md | 10 +- src/scribe/mcp/tools/milestones.py | 11 +- src/scribe/mcp/tools/projects.py | 17 +- src/scribe/services/milestones.py | 67 +++++- src/scribe/services/placement.py | 6 +- src/scribe/services/plugin_context.py | 19 +- tests/helpers.py | 13 +- tests/test_cited_record_status.py | 297 ++++++++++++++++++++++++++ tests/test_guidance_ownership.py | 6 + tests/test_milestone_summary_brief.py | 5 + tests/test_retrieval_scopes.py | 12 +- tests/test_services_plugin_context.py | 4 +- 13 files changed, 440 insertions(+), 29 deletions(-) create mode 100644 tests/test_cited_record_status.py 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 From 94ecb633a04231dc239df3b23f36604ba781e416 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 18 Sep 2026 12:11:39 -0400 Subject: [PATCH 3/7] fix(tests): the counts query selects four columns, not three (#4154) The new fixtures fed (milestone_id, status, count) and the query also carries max(updated_at), which last_touched_at is computed from. Six tests in the new module died unpacking it; the product path was never reached, and the rest of the suite was green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_cited_record_status.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/test_cited_record_status.py b/tests/test_cited_record_status.py index 023d28f..4474414 100644 --- a/tests/test_cited_record_status.py +++ b/tests/test_cited_record_status.py @@ -53,6 +53,10 @@ def _sessions(results: list, counter: list[int], seen_sql: list | None = None): the ORDER of `results` pins the order of the queries — which is what makes the query-count assertion meaningful rather than incidental. + Counts rows are 4-tuples — (milestone_id, status, count, max(updated_at)) — + because the counts query also carries the touched-at that `last_touched_at` + is computed from. A 3-tuple unpacks as a ValueError, not as a wrong answer. + `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. @@ -116,7 +120,7 @@ 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, "done", 8, None), (409, "todo", 1, None)], [(409, 4154, "Step 8 — a cited record carries its status", "todo")], ) assert rows[5][0]["next_step"] == { @@ -134,7 +138,7 @@ async def test_a_finished_plan_names_no_next_step_rather_than_omitting_the_key() 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)], []) + rows = await _summaries([_milestone_row(416)], [(416, "done", 9, None)], []) assert rows[5][0]["next_step"] is None assert "next_step" in rows[5][0] @@ -149,7 +153,7 @@ async def test_the_earliest_open_step_wins_not_the_earliest_step(): """ rows = await _summaries( [_milestone_row(409)], - [(409, "done", 2), (409, "todo", 2)], + [(409, "done", 2, None), (409, "todo", 2, None)], # 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")], @@ -169,7 +173,7 @@ async def test_the_batch_does_not_fan_out_per_milestone(): counter = [0] await _summaries( [_milestone_row(i) for i in range(40)], - [(i, "todo", 1) for i in range(40)], + [(i, "todo", 1, None) for i in range(40)], [(i, 1000 + i, f"S{i}", "todo") for i in range(40)], counter=counter, ) @@ -190,7 +194,7 @@ async def test_both_step_queries_take_the_same_visibility_clause(): seen.append(uid) return true() - results = [[_milestone_row(409)], [(409, "todo", 1)], [(409, 1, "S", "todo")]] + results = [[_milestone_row(409)], [(409, "todo", 1, None)], [(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]) @@ -222,7 +226,7 @@ async def test_the_listing_and_placement_agree_on_what_open_means(): listing_sql: list = [] rows = await _summaries( [_milestone_row(409)], - [(409, "done", 2), (409, "todo", 2)], + [(409, "done", 2, None), (409, "todo", 2, None)], [(409, s.id, s.title, s.status) for s in steps if s.status in ms.OPEN_STEP_STATUSES], seen_sql=listing_sql, ) From 7038e41ec71ec14be85a907a041862027f64cb16 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 18 Sep 2026 12:39:13 -0400 Subject: [PATCH 4/7] feat(rules): preferences are writable, and their drift arrives (#3895) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone 399 step 5. Steps 1-4 put preferences into the backend: a kind column, an inverted write path, a third register in the injected block, a delivery slot. Nothing the operator could touch. Rule 27 forbids leaving it there, and here it matters more than usual, because the UI is the only guard against the risk the milestone named up front — an agent misreads one session, rewrites a preference, and follows the rewritten version forever while the operator never sees the moment it changed. Four things ship. A preference is DISTINGUISHABLE. `kind` reaches the client (the server has always sent it in rule_brief) and a preference carries a chip. Force is the one thing a list of instructions must not leave the reader to infer, and a row that renders identically to a rule teaches the opposite of both facts about a preference: it does not bind, and a session may rewrite it. A preference is WRITABLE. The editor gains the kind as a first-class choice with the test beside it — what happens when someone does not do this — and says plainly, when preference is chosen, that sessions rewrite these without asking and every rewrite is kept. DRIFT ARRIVES. `GET /api/rules/drift` returns one row per rewritten preference carrying its latest rewrite: what it said, what it says now, and the record named by `arose_from_id` that taught the change. Both texts ride along so the list shows the diff without a call per row. The new pane sits beside the staleness sweep, because drift belongs to no one rulebook, and it answers a question the operator would not have thought to ask. REVERSION IS ONE ACTION, and this is the carve-out worth arguing with. Milestone 323 refused a one-click restore for rules — "a binding instruction should not be revertible in one click", because a silent revert erases the only record of why the rewrite happened. That reasoning turns on the rewrite being the operator's own decision. A preference's is not: the agent makes it mid-work without asking, so reverting is a veto over someone else's edit rather than an undo of your own, and a veto costing more than a shrug is not supervision. The route refuses anything but a preference (409), and nothing is erased: the restore goes through update_rule, so it snapshots too and the history GAINS the revert. An integration test pins that, because it is the whole basis for the exception. Tested against real Postgres — every claim is about which rows come back and in what order, which a stand-in session cannot judge. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- frontend/src/api/rulebooks.ts | 69 ++++- frontend/src/assets/rules-shared.css | 13 + .../components/rules/PreferenceDriftPane.vue | 223 ++++++++++++++ .../components/rules/RuleEditorSlideOver.vue | 51 ++- .../src/components/rules/RuleListPane.vue | 18 +- .../src/components/rules/RulebookListPane.vue | 24 +- frontend/src/stores/rulebooks.ts | 48 ++- frontend/src/views/RulesView.vue | 30 +- src/scribe/routes/rulebooks.py | 52 +++- src/scribe/services/rulebooks.py | 210 ++++++++++++- tests/test_integration_preference_drift.py | 291 ++++++++++++++++++ 11 files changed, 1008 insertions(+), 21 deletions(-) create mode 100644 frontend/src/components/rules/PreferenceDriftPane.vue create mode 100644 tests/test_integration_preference_drift.py diff --git a/frontend/src/api/rulebooks.ts b/frontend/src/api/rulebooks.ts index 5681571..331bd34 100644 --- a/frontend/src/api/rulebooks.ts +++ b/frontend/src/api/rulebooks.ts @@ -39,12 +39,27 @@ export interface RulebookTopic { updated_at: string | null; } +/** + * What kind of instruction this is, and it is about FORCE, not importance. + * + * A `rule` must be FOLLOWED — ignoring it breaks something. It is the + * operator's decision, so it changes when they change it. A `preference` is + * how they want work DONE — ignoring it costs consistency, not correctness — + * and the agent rewrites it in the ordinary course of working, which is what + * makes the drift surface necessary. + * + * Always sent by the server, never inferred from an absent key: "no kind + * field" and "kind is rule" must not be the same payload. + */ +export type RuleKind = "rule" | "preference"; + export interface Rule { id: number; topic_id: number | null; project_id: number | null; title: string; statement: string; + kind: RuleKind; /** WHEN this rule fires — the trigger, not the instruction. */ when_to_apply: string; why: string; @@ -79,6 +94,8 @@ export interface RuleHeader { title: string; statement: string; topic_id: number | null; + /** Unconditional on the wire (services.rulebooks.rule_brief). */ + kind: RuleKind; /** A date (YYYY-MM-DD), not a timestamp. */ updated_at: string | null; when_to_apply?: string; @@ -180,6 +197,9 @@ export async function getRule(id: number): Promise { export interface RuleWrite { title: string; statement: string; + /** Writable from the editor: a preference is not a lesser rule, it is a + * different force, and the person writing it is the one who knows which. */ + kind: RuleKind; when_to_apply: string; why: string; how_to_apply: string; @@ -256,9 +276,56 @@ export async function getRuleVersion( return apiGet(`/api/rules/${ruleId}/versions/${versionId}`); } -// No restoreRuleVersion, deliberately (milestone 323). Putting an old wording +// No restore FOR A RULE, deliberately (milestone 323). Putting an old wording // back goes through updateRule, which snapshots what it replaces — so the // undo stays visible in the history like any other edit. +// +// A preference is the exception and the server refuses anything else (409). +// 323's reasoning is that a rewrite is the operator's own decision; a +// preference's rewrite is the agent's, made mid-work without asking, so +// putting it back is a veto rather than an undo — and a veto that costs more +// than shrugging is not really supervision. Nothing is erased either way: +// the restore snapshots too, so the history GAINS the revert. +export async function restoreRuleVersion( + ruleId: number, versionId: number, +): Promise { + return apiPost(`/api/rules/${ruleId}/versions/${versionId}/restore`, {}); +} + +/** + * One preference that has been rewritten: what it said, what it says now, and + * what taught the change. + * + * Both texts ride along so the list can show the diff without a follow-up call + * per row — a listing that needs N round-trips to say what it means is one + * nobody scrolls, which would leave the drift as unsupervised as before. + */ +export interface PreferenceDrift { + rule: RuleHeader; + /** What it said BEFORE the latest rewrite. */ + previous: { + id: number; + created_at: string | null; + title: string; + statement: string; + when_to_apply: string; + }; + current: { title: string; statement: string; when_to_apply: string }; + /** + * The record named by `arose_from_id` — what the change was learned from. + * Absent when the preference carries none, which is every one written + * before that field was required. + */ + taught_by?: { id: number; title: string }; +} + +export async function listPreferenceDrift( + limit?: number, +): Promise { + const qs = limit ? `?limit=${limit}` : ""; + const data = await apiGet<{ drift: PreferenceDrift[] }>(`/api/rules/drift${qs}`); + return data.drift; +} export async function deleteRule(id: number): Promise { return apiDelete(`/api/rules/${id}`); diff --git a/frontend/src/assets/rules-shared.css b/frontend/src/assets/rules-shared.css index f2345f3..f521439 100644 --- a/frontend/src/assets/rules-shared.css +++ b/frontend/src/assets/rules-shared.css @@ -40,3 +40,16 @@ padding: 0.05rem 0.4rem; vertical-align: middle; } + +/* A PREFERENCE, marked because force is the one thing a list of instructions + must not leave the reader to infer. A preference does not bind — ignoring it + costs consistency, not correctness — and it is the one kind the agent + rewrites on its own, so a row that renders identically to a rule teaches the + opposite of both facts. + + The accent, not the warning colour: nothing is wrong with a preference. It + is a different KIND, and the marker says which. */ +.rule-chip-preference { + color: var(--fs-accent); + background: var(--fs-accent-soft); +} diff --git a/frontend/src/components/rules/PreferenceDriftPane.vue b/frontend/src/components/rules/PreferenceDriftPane.vue new file mode 100644 index 0000000..187e0cf --- /dev/null +++ b/frontend/src/components/rules/PreferenceDriftPane.vue @@ -0,0 +1,223 @@ + + + + + diff --git a/frontend/src/components/rules/RuleEditorSlideOver.vue b/frontend/src/components/rules/RuleEditorSlideOver.vue index 955892c..19ebc16 100644 --- a/frontend/src/components/rules/RuleEditorSlideOver.vue +++ b/frontend/src/components/rules/RuleEditorSlideOver.vue @@ -4,7 +4,7 @@ import { useRulebooksStore } from "@/stores/rulebooks"; import { useCanonicalSystemsStore } from "@/stores/canonicalSystems"; import RuleHistoryPanel from "@/components/rules/RuleHistoryPanel.vue"; import RuleHomePicker from "@/components/rules/RuleHomePicker.vue"; -import type { Rule } from "@/api/rulebooks"; +import type { Rule, RuleKind } from "@/api/rulebooks"; const props = defineProps<{ ruleId: number | null; topicId: number | null }>(); const emit = defineEmits<{ close: [] }>(); @@ -14,6 +14,11 @@ const canon = useCanonicalSystemsStore(); const title = ref(""); const statement = ref(""); const whenToApply = ref(""); +// Defaults to `rule`, matching the server's column default. The safe +// direction is the one that binds: a preference mislabelled as a rule is +// followed too faithfully, where a rule mislabelled as a preference is one a +// session may quietly rewrite. +const kind = ref("rule"); const systemIds = ref([]); const why = ref(""); const howToApply = ref(""); @@ -70,6 +75,7 @@ async function load() { title.value = r.title; statement.value = r.statement; whenToApply.value = r.when_to_apply || ""; + kind.value = r.kind; systemIds.value = (r.systems ?? []).map((sys) => sys.id); why.value = r.why || ""; howToApply.value = r.how_to_apply || ""; @@ -80,6 +86,7 @@ async function load() { title.value = ""; statement.value = ""; whenToApply.value = ""; + kind.value = "rule"; systemIds.value = []; why.value = ""; howToApply.value = ""; @@ -98,6 +105,7 @@ async function save() { title: title.value, statement: statement.value, when_to_apply: whenToApply.value, + kind: kind.value, // Always sent, so clearing the last area actually clears it — the server // reads a list as "these ARE the areas now". system_ids: systemIds.value, @@ -140,7 +148,7 @@ watch(() => props.ruleId, load);